Paint rectangles to canvas using mouse Just wanted to post this here to see if anyone has any critiques of my code, if I'm drawing the background image most efficiently, or if I'm doing anything which could benefit from obvious improvements.

RectangleMover

import java.awt.BorderLayout;
import java.awt.EventQueue;
import java.awt.Graphics;
import java.awt.Image;
import java.awt.event.MouseEvent;
import java.awt.event.MouseMotionListener;
import java.io.File;
import java.io.IOException;

import javax.swing.JFrame;
import javax.swing.JPanel;

public class RectangleMover {
    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                final JFrame frame = new JFrame();

                frame.setSize(FRAME_WIDTH, FRAME_HEIGHT);
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.setContentPane(new JPanel(new BorderLayout()) {
                    /**
                     *
                     */
                    private static final long serialVersionUID = 1L;

                    public void paintComponent(Graphics g) {
                        try {
                            super.paintComponent(g);
                            final Image backgroundImage = javax.imageio.ImageIO
                                    .read(new File(
                                            "/Users/langer/Desktop/7923.jpg"));
                            g.drawImage(backgroundImage, 0, 0, FRAME_WIDTH,
                                    FRAME_HEIGHT, null);
                        } catch (IOException e) {
                            e.printStackTrace();
                        }
                    }
                });
                frame.setVisible(true);

                frame.setLocationRelativeTo(null);

                final RectangleComponent b = new RectangleComponent();
                frame.add(b);
                frame.addMouseMotionListener(new MouseMotionListener() {

                    public void mouseDragged(MouseEvent e) {
                        b.addRect(e.getX(), e.getY());

                        frame.setVisible(true);
                    }

                    public void mouseMoved(MouseEvent e) {
                    }

                });

            }
        });
    }

    private static final int FRAME_WIDTH = 800;
    private static final int FRAME_HEIGHT = 1000;
}

RectangleComponent

import java.awt.BasicStroke;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.Rectangle;
import java.util.ArrayList;

import javax.swing.JComponent;

/**
 * This component displays a rectangle that can be moved.
 */
public class RectangleComponent extends JComponent {

    private static final long serialVersionUID = 6596614645871812990L;
    private ArrayList<Rectangle> boxes = new ArrayList<Rectangle>();

    public void paint(Graphics g) {
        Graphics2D g2 = (Graphics2D) g;
        super.paintComponent(g);

        for (int i = 0; i < boxes.size(); i++) {
            float thickness = 2;
            g2.setStroke(new BasicStroke(thickness));
            g2.drawRect(boxes.get(i).x, boxes.get(i).y - 29,
                    boxes.get(i).width, boxes.get(i).height);
        }
    }

    public void addRect(int x, int y) {
        boxes.add(new Rectangle(x, y, BOX_WIDTH, BOX_HEIGHT));
        repaint();
    }

    private static final int BOX_WIDTH = 42;
    private static final int BOX_HEIGHT = 60;
}

Don't load the same resource repeatedly

I don't have much time to give a full review but I can point out one big thing that I saw while looking through the code.

                public void paintComponent(Graphics g) {
                    try {
                        super.paintComponent(g);
                        final Image backgroundImage = javax.imageio.ImageIO
                                .read(new File(
                                        "/Users/langer/Desktop/7923.jpg"));
                        g.drawImage(backgroundImage, 0, 0, FRAME_WIDTH,
                                FRAME_HEIGHT, null);
                    } catch (IOException e) {
                        e.printStackTrace();
                    }
                }

You shouldn't load and parse the background image in the paint method. This method can be called frequently even though your background image doesn't change. If you try to resize your window you'll see how you reload the background several times per second slowing down your application.

Prefer to create a program instance

Doing so allows you to encapsulate variables to the class instead of keeping them in main or as inline. Also makes your code more readable.

public class RectangleMover {
    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                final JFrame frame = new JFrame();

                frame.setSize(FRAME_WIDTH, FRAME_HEIGHT);
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.setContentPane(new JPanel(new BorderLayout()) {
                    /**
                     *
                     */
                    private static final long serialVersionUID = 1L;

should be something like this:

public class RectangleMover {

    private final Image background;

    public RectangleMover(){
        background = loadimage();
        final JFrame frame = new JFrame();
        ...
    }

    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                new RectangleMover();
            }});
    }


Achieve a dataframe from a dynamic list of lists This code works for me, but I am trying to improve my coding and am looking for advice on how to improve it. Specifically I want to simplify this so that perhaps I can have a dynamically updating dataframe that tracks the "site" that I am on, and the "cutoff" value that I am on, and records the calculations for that site/cutoff value combination. Right now, it's a little clumsy and admittedly very ugly. Through this process, though, I learned about "assign" for creating assigning variables on the fly. I think I just need maybe one or two more concepts before I can really clean this up.

calcs <- function(site, cutoff){} #Returns a 4 item list

cutoffmatrix <- c(0, 0.005, 0.01, 0.015) #matrix that I want to loop through

for(i in 1:length(cutoffmatrix)){ #Loop that uses my function for cutoffmatrix value
  test <- list()
  co <- cutoffmatrix[i]
  assign(paste("TC", i, sep = ""), calcs("TC", co))
}

l_TC <- list(TC1, TC2, TC3, TC4) #combines the loop results into a list for that site

library(plyr)
df_TC <- ldply (l_TC, data.frame) #transform lists of lists into dataframe

for(i in 1:length(cutoffmatrix)){ #same thing, but for next site
  test <- list()
  co <- cutoffmatrix[i]
  assign(paste("DC", i, sep = ""), calcs("DC", co))
}

l_DC <- list(DC1, DC2, DC3, DC4)
df_DC <- ldply(l_DC, data.frame)

final <- rbind (df_TC, df_DC) #combines both site's results into a dataframe
final <- cbind(final, cutoffmatrix) #adds cutoff values to dataframe
namelist <- c("TC", "TC", "TC", "TC", "DC", "DC", "DC", "DC") #adds sites to dataframe
final <- cbind(final, namelist)

Any help is greatly appreciated!

Reviewing line-by-line:

cutoffmatrix <- c(0, 0.005, 0.01, 0.015)

If this is a vector, why call it a matrix? I would just call it just cutoffs.

for(i in 1:length(cutoffmatrix)){
    test <- list()
    co <- cutoffmatrix[i]
    assign(paste("TC", i, sep = ""), calcs("TC", co))
}

The use of 1:length(cutoffmatrix) in a for loop is a common mistake. You'll probably agree that, by design, the code in the loop will be executed length(cutoffmatrix). Well, that's not exactly always the case. In particular, when cutoffmatrix is empty: cutoffmatrix <- c(). You would want the loop to be executed length(cutoffmatrix) i.e. zero times. However, in this unfortunate case, 1:length(cutoffmatrix) would evaluate to 1:0 i.e. c(1, 0). Instead of zero iteration, you'll have two and likely cause an error. Instead of 1:length(cutoffmatrix), you should use seq_along(cutoffmatrix) or seq_len(length(cutoffmatrix)). Both seq_along and seq_len will do the right thing when their input is empty or zero respectively.

The use of assign to dynamically create objects is, excuse my word, an abomination. I'll try to show you a couple reasons why. Via your loop, you have managed to create four objects: TC1, TC2, TC3, TC4 which you later use in your code:

l_TC <- list(TC1, TC2, TC3, TC4)

What would happen if instead of 4 objects, you had 2000 objects? Do you see yourself typing l_TC <- list(TC1, TC2, TC3, TC4, ..., TC2000)? No. Maybe you will find about the get or mget function so you can do:

l_TC <- mget(paste("TC", seq_along(cutoffmatrix), sep = ""))

But wait. This is still abominable. For one reason, you could be creating thousands of such objects and "polluting" your environment with similar objects. Why not put them all into a single object? Also imagine someone coming across TC1 in your environment and searching for "TC1" through your code to find where it got created: no luck. Finally, having to use mget to indirectly refer to the TC* objects is a bit complex, no? And you have to remember that the objects are labelled 1 through length(cutoffmatrix)...

So what is the alternative? Do not create length(cutoffmatrix) objects but just one object: a list containing length(cutoffmatrix) items. How? Use lapply or Map:

l_TC <- lapply(cutoffvec, calcs, site = "TC")

Look how shorter this is! And in the following, 1) your code will only use l_TC to refer to those 4 or 2000 objects stored within 2) Someone stumbling upon l_TC in your environment will see exactly where it was created in your code. By all means, please never use assign again and erase that function from your memory!

Next:

library(plyr)
df_TC <- ldply (l_TC, data.frame) #transform lists of lists into dataframe

So this converts the lists into data.frames and rbinds everything together. You do not really need plyr for that. You can first turn the lists into data.frames by doing lapply(l_TC, as.data.frame). And you can do the binding using do.call(rbind, ...), so:

df_TC <- do.call(rbind, lapply(l_TC, as.data.frame))

But before doing that: Why do you need to convert the list items to data.frames, then bind? Could you instead rewrite calcs so that it returns a data.frame? And better, could calc take a vector of cutoffs instead of a single value and return a data.frame with length(cutoffs) rows?

final <- rbind(df_TC, df_DC)
final <- cbind(final, cutoffmatrix)

Instead of having to add cutoffmatrix, could calcs already include a cutoff item in its output list?

namelist <- c("TC", "TC", "TC", "TC", "DC", "DC", "DC", "DC")
final <- cbind(final, namelist)

This code has a baked-in assumption that there are four cutoffs. If you were to modify cutoffmatrix so its length is not four, then this would break. Instead, you could do:

namelist <- rep(c("TC", "DC"), each = length(cutoffmatrix))

Still, not great. Similar to above, the best would be if calcs could include a site = site item in its output list.

Last. What if instead of two sites "TC" and "DC" you had many more websites to use? How could you modify your code so you do not have to write "TC" or "DC" more than once and have to rely on creating too many objects? Maybe use some of the concepts I have shown you? Good luck!


Speed optimization for block XOR In code I'm currently maintaining, there is a need to do very many repeated XOR operations of blocks of memory.  The block size in my case is always 16 bytes.  Because the code is executed very frequently, and because it's speed critical, I would like it to be as fast as possible.  Ordinarily I'd write this in assembly language for that reason, but I'd prefer that the code stay in C++ to maximize portability.  This is what I'm currently using, but I would like a review to address particularly the following questions:

Can this code be made faster?
Can the portability be improved?  In particular, I'm interested in avoiding problems on architectures for which there is a penalty for misaligned memory access.

memxor.c
const size_t BLKSIZE = 16;
typedef unsigned bigsize_t;
inline void memxor(uint8_t dst[BLKSIZE], const uint8_t *a, const uint8_t *b)
{
    bigsize_t *s=reinterpret_cast<bigsize_t *>(dst);
    const bigsize_t *au = reinterpret_cast<const bigsize_t *>(a);
    const bigsize_t *bu = reinterpret_cast<const bigsize_t *>(b);
    for (size_t i=BLKSIZE/sizeof(*s); i; --i)
        *s++ = *au++ ^ *bu++;
}

You're stuck at a point where your readability is good, and any performance improvements are going to come at the expense of making the code more ugly. Additionally, you're already
making architecture choices based on the compiler's interpretation of 'unsigned'. It's already not pretty.

Oh, and the un-braced 1-line for-loop is a problem for readability too.

I don't know of any optimizations that are available at this point that will not come with the portability, or readability cost. For a block-size of 16 bytes, and with a likely 4-byte unsigned value, the odds are that your loop will iterate 4 times only anyway.

The compiler may unroll that loop, and move on.

The above is just a waffle that really means: from here on, you cannot accomplish all three: clean code, portability, and performance. You need to compromise somewhere.

Profiling would be the logical thing to do. profile your current code, and establish a baseline.

I would then suggest that you investigate whether you can have a larger block size. Any alignment fiddling you may have to do will pay off better if you are working with larger blocks. Can you batch these blocks in to 1MB contiguous regions?

With larger contiguous blocks you could then consider vectorization, alignment, and other optimizations that have an increased, and fixed setup cost, but improved throughput.

Additionally, I would consider forcing bigsize_t to be unsigned long long, and then manually unroll the loop. If you find an exact type that is 64 bits (there has to be one? and, if not, you can have an alternate implementation...), then you can force your type to match:

#include <limits.h>

#if UINT_MAX == 18446744073709551615ULL
typedef unsigned int big64_t;
#elif ULONG_MAX == 18446744073709551615ULL
typedef unsigned long big64_t ;
#elif ULLONG_MAX == 18446744073709551615ULL
typedef unsigned long long big64_t;
#else
#error "Cannot find unsigned 64bit integer."
#endif

big64_t *s = reinterpret_cast<big64_t *>(dst);
const big64_t *au = reinterpret_cast<const big64_t *>(a);
const big64_t *bu = reinterpret_cast<const big64_t *>(b);

s[0] = au[0] ^ bu[0];
s[1] = au[1] ^ bu[1];

With systems that are not natively 64-bit, the compiler will adjust the operation to be relatively efficient anyway.

In the event that this is the year 2100, and all datatypes are 128 bit, or more, then you should possibly add the code that does a single 128-bit XOR for your input.

I see the above as being as portable, and equally readable. As for the performance, that will require a profile and benchmark.


Map associative array to another array php I have an associative array of the states of some country, and the states names are the keys:

array:13 [
  "Ontario" => null
  "Manitoba" => null
  "New Brunswick" => null
  "Yukon" => null
  "Saskatchewan" => null
  "Prince Edward Island" => null
  "Alberta" => null
  "Quebec" => null
  "Nova Scotia" => null
  "British Columbia" => null
  "Nunavut" => null
  "Newfoundland and Labrador" => null
  "Northwest Territories" => null
]

And I have anothe associative array that contains all states that have values:

array:8 [
  "Alberta" => 17
  "Cairo" => 1
  "Calgary" => 1
  "ddd" => 4
  "gfdxf" => 1
  "New Cairo" => 1
  "Ontario" => 1
  "secret" => 30
]

Now I need to map the second array to the first one so that the result would be:

array:13 [
  "Ontario" => 1
  "Manitoba" => 0
  "New Brunswick" => 0
  "Yukon" => 0
  "Saskatchewan" => 0
  "Prince Edward Island" => 0
  "Alberta" => 17
  "Quebec" => 0
  "Nova Scotia" => 0
  "British Columbia" => 0
  "Nunavut" => 0
  "Newfoundland and Labrador" => 0
  "Northwest Territories" => 0
]

I created a nested loop and it works fine, but the code is very ugly, now is there a more efficent way to do it?

My code:

foreach ($all_states as $state_x => $value_x) {

    foreach ($country_states as $state_y => &$value_y) {

        if (strtolower($state_x) == strtolower($state_y)) {

            $value_y = $value_x;
        } elseif ($value_y == NULL) {

            $value_y = 0;
        }
    }
}

array_replace() is the perfect call here -- it can be used to overwrite the master array with the array containing actual integer values.

Code: (Demo)

var_export(array_replace(array_map('intval', $all_states), $country_states));

To streamline the process futher, you should declare your master list with 0 values instead of null values, then you can omit the array_map() call like this:

Code: (Demo)

var_export(array_replace($all_states, $country_states));

Additional suggestions, caveats, and considerations:

You might reconsider your variable names, as they don't seem to do a great job of describing the data that they contain.  $all_states might be $statesLookup or $statesDefault.  I don't know what is being counted in the second array, but $country_states might be better declared as $state_counts or something.
If $country_states has any elements with keys that are not represented in $all_states, then these new elements WILL be appended to the end of the output array.  If this is a legitimate concern, you can call array_intersect_key($country_states, $all_states) to filter out any expected elements.
The order of the elements in the output array will be ordered by $all_states.  No matter what order the country_states are in.


Calculator that is part of a multipurpose program I am currently working on a Python project.  It is some sort of a hub, where you can do some cool but basic stuff, such as: setting timers, launching websites, doing basic math, ping and view source codes of URLs, and more.

I want to make this code just a little more compact. I especially want my math menu more compact because I feel there is a way of doing it.  I just can't seem to figure it out.

…
elif userIn == ("math"):
    op = str(input("    * Enter operation (+, -, *, /, exp, sqrt, log): "))
    blank()
    if op==("+") or op==("-") or op==("*") or op==("/") or op==("exp"):
        input_1 = int(input("       * Enter first number:  "))
        blank()
        input_2 = int(input("       * Enter second number: "))
        blank()
    if op == ("+"):
        print ("       > "+str(input_1 + input_2))
        blank()
    elif op == ("-"):
        print ("       > "+str(input_1-input_2))
        blank()
    elif op == ("*"):
        print ("       > "+str(input_1*input_2))
        blank()
    elif op == ("/"):
        print ("       > "+str(input_1/input_2))
        blank()
    elif op == ("exp"):
        print ("       > "+str(input_1**input_2))
        blank()
    elif op == ("sqrt"):
        oneInput=int(input("        * Enter number: "))
        blank()
        print ("       > "+str(math.sqrt(oneInput)))
        blank()
    elif op == ("log"):
        input_1 = int(input("       * Enter number:  "))
        blank()
        input_2 = int(input("       * Enter base: "))
        blank()
        print ("       > "+str(int((math.log(input_1, input_2)))))
        blank()

Link to the code (pastebin).

Note: I have tried setting a basic for loop run through a list of operation symbols, and it would set the operation if the user op. was matched.  But, as it turns out, I just couldn't use strings as math operators in the code itself.

opl=["+", "-", "*", "/"]
for i in range(len(opl)):
    userInput1 (some way of setting op)     userInput2

I would suggest using a dict as switch-statement like structure.

First some global helper functions/data structures:

math_dict_binary = {'+': lambda a, b: a+b,
                    '-': lambda a, b: a-b,
                    # fill out the rest of you binary functions
                   }
math_dict_uniary = {'sqrt': math.sqrt,
                    # rest of you uniary
                   }

def get_input(str_prompt):
    input = int(input("       * {}:  ".format(str_prompt)))
    blank()
    return input

in your main body:

elif userIn == ("math"):
    op = str(input("    * Enter operation (+, -, *, /, exp, sqrt, log): "))
    blank()
    if op in math_dict_binary:
        input_1 = get_input("Enter first number")
        input_2 = get_input("Enter second number")
        print( "    " + str(math_dict_binary[op](input_1, input_2)))
    elif op in math_dict_uniary:
        input_1 = get_input("Enter number")
        print( "    " + str(math_dict_uniary[op](input_1)))
    else:
        print("Not a vlaid math option")
        # deal with it


Java Non Reentrant Lock Implementation I have implemented a Non Reentrant Lock. I want to know if this has any mistakes, race conditions etc. I am aware of the fact that existing libraries have to be used (instead of writing our own), but this is just to see if I am understanding the java concurrency correctly. Any feedback is appreciated.

public class MyLock {
private boolean isLocked = false;
private long owner = -1;
private static String TAG = "MyLock: ";

public synchronized void Lock() throws InterruptedException, IllegalStateException {
if(owner == Thread.currentThread().getId()) {
    throw new IllegalStateException("Lock already acquired. " +
                                    "This lock is not reentrant");
} else {
    while(isLocked == true) {
        System.out.println(TAG+"Waiting for Lock, Tid = " +
                Thread.currentThread().getId());
        wait();
    }
}

isLocked = true;
owner = Thread.currentThread().getId();
System.out.println(TAG+"Lock Acquired: Owner = " + owner);
}

public synchronized void Unlock() throws IllegalStateException {
if(!isLocked || owner != Thread.currentThread().getId()) {
    throw new IllegalStateException("Only Owner can Unlock the lock");
} else {
    System.out.println(TAG+"Unlocking: Owner = " + owner);
    owner = -1;
    isLocked = false;
    notify();
}
}

Your code works, but its quality leaves some room for improvement. It should be simplified by refactoring.
Here are some suggestions:

Naming

Even if it is only an exercise, utmost care should be applied when choosing names to make sure they are descriptive and clear, but also to comply with coding conventions:

Consider calling your class NonReentrantLock instead of MyLock.
Methods should be camel-case, starting with a lower-case character, so Lock() should be lock() and Unlock() should be unlock().
Only constants should be upper-case, so TAG should be made a constant. But TAG can mean anything. The solution is to refactor your code (as shown below) such that TAG is only used once and is no longer needed as a class variable.

Constants

TAG is not a useful constant because it leads to lots of System.out.println calls which you want to avoid. All your logging should happen in a single place, not be spread out over your class.
However, you are using the magic value of -1 to show that nobody owns the lock at the moment. You should introduce a constant NOBODY and use that to set owner:

private static final long NOBODY = -1;
private long owner = NOBODY;

Redundancy

Tracking the same state twice

Look carefully at your instance variables owner and isLocked. Track their values during runtime with the debugger, and you will see: if owner == -1 then isLocked == false, but if owner != -1 then isLocked == true. In other words, you are using two variable to track the same piece of state.

You should remove isLocked and replace it it with the following method:

private synchronized boolean isLocked() {
    return owner != NOBODY;
}

Doing the same string concatenations multiple times

You have used System.out.println throughout your code, always passing TAG, a message, and a thread id. You can extract this functionality into a new method:

private void log(String message, long threadId) {
    System.out.println(String.format("MyLock: %s %s", message, threadId));
}

That way, System.out.println is only called in a single place and can easily be disabled or replaced with a real logger. For instance, this:

System.out.println(TAG+"Lock Acquired: Owner = " + owner);

can become this:

log("lock acquired, owner =", owner);

Miscellaneous redundancies

If you throw an exception, inside an if-structure, you the else branch is redundant, so code like

if (condition) {
    throw new IllegalStateException(...);
} else {
    doSomethingElse();
}

can become

if (condition) {
    throw new IllegalStateException(...);
}
doSomethingElse();

You have code duplication when checking if the current thread is the owner of the lock. Refactor out this method instead:

private boolean ownerIsCurrentThread() {
    return owner == Thread.currentThread().getId();
}

You should extract argument validation into a separate method:

private void throwIf(boolean condition, String message) throws IllegalStateException {
    if (condition) {
        throw new IllegalStateException(message);
    }
}

which will allow you to replace your argument validation if-statements with code like

throwIf(ownerIsCurrentThread(), "the lock has already been acqired");

Error messages

At the beginning of unlock(), you throw an IllegalStateException with the same message for two completely different reasons. That's not very helpful for debugging. Instead, check them separately:

public synchronized void unlock() throws IllegalStateException {
    throwIf(!isLocked(), "only a locked lock can be unlocked");
    throwIf(!ownerIsCurrentThread(), "only owner can unlock the lock");

End Result

public class NonReentrantLock {
    private static final long NOBODY = -1;
    private long owner = NOBODY;

    public synchronized void lock() throws InterruptedException, IllegalStateException {
        throwIf(ownerIsCurrentThread(), "the lock has already been acqired");
        long threadId = Thread.currentThread().getId();
        while (isLocked()) {
            log("waiting for lock on thread", threadId);
            wait();
        }
        owner = threadId;
        log("lock acquired, owner =", owner);
    }

    public synchronized void unlock() throws IllegalStateException {
        throwIf(!isLocked(), "only a locked lock can be unlocked");
        throwIf(!ownerIsCurrentThread(), "only owner can unlock the lock");
        log("unlocking, owner =", owner);
        owner = NOBODY;
        notify();
    }

    private synchronized boolean isLocked() {
        return owner != NOBODY;
    }

    private boolean ownerIsCurrentThread() {
        return owner == Thread.currentThread().getId();
    }

    private void throwIf(boolean condition, String message) throws IllegalStateException {
        if (condition) {
            throw new IllegalStateException(message);
        }
    }

    private void log(String message, long threadId) {
        System.out.println(String.format("MyLock: %s %s", message, threadId));
    }
}


Class to hold data for a fund Requirements:

To store strings into variables, check if they are valid for the field.
To be able to easily unit test the validity checks with different parameters.
To throw exception when a check fails
To scale appropiately for dozens of field types
To have a good API for users.
To be expandable.

I have the following code. A class that will hold a fund

Public Class FundElement
    Public Name As New FundDetail
    Public Price As New FundDetail
    Public Code As New CodeDetail
End Class

A base class for each individual data field.

Public Class FundDetail
    Protected _value As String

    Public Property value As String
        Get
            Return _value
        End Get

        Set(valueToAssign As String)
            If isValid(valueToAssign) Then
                _value = valueToAssign
            Else
                Throw New ArgumentException
            End If
        End Set
    End Property

    Shared Function isValid(detail As String) As Boolean
        Return True
    End Function
End Class

And some inherited classes that will hold logic specific to certain fields. This is the reason I wanted a class for rather than just a string for each field. It inherits from FundDetail, but it could have been an interface. I chose inheritance so I could have default implementations.

Public Class CodeDetail
    Inherits FundDetail

    Public Shared Shadows Function isValid(value As String) As Boolean
        Dim intPattern As Regex = New Regex("[a-zA-Z]{3}[0-9]{12}")
        Return intPattern.IsMatch(value)
    End Function
End Class

Now, my problem is that the API for my class is as follows

Dim fund as new FundElement

FundElement.Name.Value = "Google Inc"
FundElement.Code.Value = "USD451234151234"

Which means that coders have access to the NAV element, which I want to hide completely. And the API is more verbose than it needs to be. I want the interface to be as follows

FundElement.Name = "Google Inc"
FundElement.Price = "USD451234151234"

I can implement this by changing the class so that each fundDetail instance is private, and by adding a new property as follows

Public Class FundElement

    Private _Name As New FundDetail
    Public Property Name As String
        Set(valueToAssign As String)
            _Name.value = valueToAssign
        End Set
        Get
            Return _Name.value
        End Get
    End Property

    Private _Code As New FundDetails
    Public Property _Code As String
        Set(valueToAssign As String)
            _Code.value = valueToAssign
        End Set
        Get
            Return _Code.value
        End Get
    End Property

    Private _Price As New PriceDetail
    Public Property _Price As String
        Set(valueToAssign As String)
            _Price.value = valueToAssign
        End Set
        Get
            Return _Price.value
        End Get
    End Property

End Class

Now this ended up very verbose for just 3 members. Additionally, the code for each property was almost exactly the same. It's tedious to type and cluttery to read.

Is there any way I can achieve this without having to type the properties every time I want to add a new member? I feel like I should take a more simple approach. But every simple approach comes with a deficiency I have trouble overcoming.

I'm not really a VB.Net'er but you appear to be able to achieve what you're after using the Widening Operator.  It allows you to make your value property a Friend so that it can only be accessed from within your module.  However, it's not ideal, as you have to create the widening operator in your FundDetail class and any inheritted classes.  Depending on the number of different field types you may end up with more typing.  I found that I had to change the isValid method to Overridable, rather than Shared in order to get it to be called on derived classes, but that might be because of my lack of VB.Net experience.  It also works by creating new instances of objects so there is some overhead.  An example of what it looks like is below:

Public Class FundElement
    Public Name As New FundDetail
    Public Price As New FundDetail
    Public Code As New CodeDetail
End Class

Public Class FundDetail

    Protected _value As String

    Friend Property value As String
        Get
            Return _value
        End Get

        Set(valueToAssign As String)
            If isValid(valueToAssign) Then
                _value = valueToAssign
            Else
                Throw New ArgumentException
            End If
        End Set
    End Property

    Overridable Function isValid(detail As String) As Boolean
        Return True
    End Function

    Public Shared Widening Operator CType(ByVal val As String) As FundDetail
        Dim retVal As New FundDetail
        retVal.value = val
        Return retVal
    End Operator
    '
    Public Shared Widening Operator CType(ByVal val As FundDetail) As String
        Return val.value
    End Operator
End Class

Public Class CodeDetail
    Inherits FundDetail

    Public Overrides Function isValid(value As String) As Boolean
        Dim intPattern As Regex = New Regex("[a-zA-Z]{3}[0-9]{12}")
        Return intPattern.IsMatch(value)
    End Function

    Public Shared Shadows Widening Operator CType(ByVal val As String) As CodeDetail
        Dim retVal As New CodeDetail
        retVal.value = val
        Return retVal
    End Operator

    Public Shared Shadows Widening Operator CType(ByVal val As CodeDetail) As String
        Return val.value
    End Operator

End Class

And Usage:

    Dim fe As New FundElement
    fe.Name = "Google Inc"
    fe.Code = "USD451234151234"

    Dim val As New String(fe.Name)

    Console.WriteLine("{0}:{1}", CStr(fe.Name), CStr(fe.Code))

You'll notice from the usage example that whilst it can be used for String targets directly, it also doesn't default cast the object to a string when passed to something that expects an unknown / object such as the WriteLine call.  So there are some definite downsides to this approach, it's whether or not the benefits are sufficient for your case.

Bug?

As I've said, I rarely write VB.  However, this feels like a bug to me:

Shared Function isValid(detail As String) As Boolean
Shared Shadows Function isValid(value As String) As Boolean

You're using shared functions to determine if the strings are valid.  So, if you do this:

CodeDetail.isValid("USD45123415~~~~~~~~~~1234")

The shadowing works, and the correct implementation is invoked.  However, because you're invoking the implementation from the base class:

If isValid(valueToAssign) Then

Then as the base class only knows about the isValid it's implemented, it's invoking that version, not the newer version on the child.  As I've said in the comments, if I do this:

fe.Code.value = "USD45123415~~~~~~~~~~1234"

No error is thrown, however if I do this:

If CodeDetail.isValid("USD45123415~~~~~~~~~~1234") Then

Then I know the error should be thrown.

If you want it to behave the way you're expecting, you need to use Overridable functions (as in my example) so that dynamic dispatch from the base class works.


Singleton Design Pattern Implementation: Bill Pugh I have been trying to implement the Bill Pugh Singleton Design Pattern using a private static class to return the singleton retrofit instance. So far I have been able to come up with the following code. Can someone help me review this code and confirm if this is a proper implementation of the singleton design pattern suggested by Mr. Bill Pugh himself, or suggest a proper way of implementing this?

public class SingletonRetrofit {

    private SingletonRetrofit(){}

    private static class SingletonRetrofitHelper
    {   private static OkHttpClient okHttpClient;
        private static Retrofit retrofit;

        private static Retrofit makeRetrofit()
        {
            HttpLoggingInterceptor interceptor = new HttpLoggingInterceptor();
            interceptor.setLevel(HttpLoggingInterceptor.Level.BODY);
            okHttpClient = new OkHttpClient.Builder()
                    .addInterceptor(interceptor)
                    .readTimeout(20, TimeUnit.SECONDS)
                    .connectTimeout(15, TimeUnit.SECONDS)
                    .build();

            retrofit = new Retrofit.Builder()
                    .baseUrl("http://ci.draftserver.com/hornsbyapp/webservice/")
                    .client(okHttpClient)
                    .addConverterFactory(ScalarsConverterFactory.create())
                    .addConverterFactory(GsonConverterFactory.create())
                    .build();

            return retrofit;

        }

    public static Retrofit getInstance()
    {
        return SingletonRetrofitHelper.makeRetrofit();
    }

}

I read the article Java Singleton Design Pattern Best Practices with Examples, it is an interesting discussion about Singleton Pattern and thread safe.

The basic pattern:

public class TrivialSingletonTest {
    private static TrivialSingletonTest instance;

    private TrivialSingletonTest() {}

    public static TrivialSingletonTest getInstance() {
        if (instance == null) {
            instance = new TrivialSingletonTest();
        }
        return instance;
    }

    public static void main(String[] args) {
        TrivialSingletonTest st1 = getInstance();

        TrivialSingletonTest st2 = getInstance();

        String className = "[" + TrivialSingletonTest.class.getName() + "] ";

        if (st1 == st2) {
            System.out.println(className + "Same object");
        } else {
            System.out.println(className + "Is not a singleton");
        }
    }
}

This is a basic implementation of the Singleton Pattern with a main() method that test it.

The issue with this implementation is just in a multi threading program, where you could have more than 1 instances.

So the fix proposed:

public static synchronized TrivialSingletonTest getInstance() {
    if (instance == null) {
        instance = new TrivialSingletonTest();
    }
    return instance;
}

The issue here is just about performances, that's why the proposition of a different approach, using a private static inner class as an helper to obtain the singleton instance.

The main advantages of this solution is that is thread safe without the need of synchronized, and that the instance is is created only after the first getInstance(), instead of a creation when the class is loaded (thanks to the static stuff).

Now about your code.

As I saw your code is not working.

First issue: your class is SingletonRetrofit but the object type of your instance is Retrofit. So I'm not sure you want to use all the main class as an helper or just a typo.

Second issue: you create the instance every time you call getInstance() this is not the purpose of a singleton.

Your code should look like:

public class SingletonRetrofit {

    private SingletonRetrofit(){}

    private static class SingletonRetrofitHelper
    {
        private static final SingletonRetrofit INSTANCE = new SingletonRetrofit();
    }

    public static SingletonRetrofit getInstance()
    {
        return SingletonRetrofitHelper.INSTANCE;
    }

    private Retrofit makeRetrofit(final String apiBaseUrl)
    {
        HttpLoggingInterceptor interceptor = new HttpLoggingInterceptor();
        interceptor.setLevel(HttpLoggingInterceptor.Level.BODY);
        OkHttpClient okHttpClient = new OkHttpClient.Builder()
                    .addInterceptor(interceptor)
                    .readTimeout(20, TimeUnit.SECONDS)
                    .connectTimeout(15, TimeUnit.SECONDS)
                    .build();

        return new Retrofit.Builder()
                .baseUrl(apiBaseUrl)
                .client(okHttpClient)
                .addConverterFactory(ScalarsConverterFactory.create())
                .addConverterFactory(GsonConverterFactory.create())
                .build();
    }
}

So the makeRetrofit() will became a method of the singleton class that you can call in your code to create the Retrofit object as needed.

If you want to create this Retrofit object just once, and the singleton purpose was this, then you have to design your singleton class in a way that this class cover all the Retrofit class interface.


Calculating employee points I have one table on which I want to perform some calculations.  The table stores employee points that are awarded for performing various tasks.  The relevant structure is as follows (table has about 15,000 rows):

PointsTable:

  ID int IDENTITY(1, 1),
  EmployeeFirstName (varchar),
  EmployeeLastName (varchar),
  PointsAwarded (decimal(18, 2)),
  TransactionDate (date)

For each row, I want to return the following:

Employee Name
Points Awarded
Transaction Date
Employee's Running Total (cumulative sum for that one employee)
Employee's Total Points (sum of all points for that one employee)
Running Total (cumulative sum for all employees)
Total Points (sum of all points for all employees)
Employee's Running Percentage (Employee's Running Total / Employee's Total Points)
Employees' Total Percentage (Employee's Total Points / Total Points)
Total Running Percentage (Running Total / Total Points)

Here is the query I am currently using:

SELECT  EmployeeFirstName + ' ' + EmployeeLastName AS [Employee],
        PointsAwarded,
        TransactionDate,
        [Employee Running Total],
        [Employee Running Total] / [Employee Total Points] AS [Employee Running Percentage],
        [Employee Total Points],
        [Employee Total Points] / [Total Points] AS [Employee Total Percentage],
        [Running Total Points],
        [Running Total Points] / [Total Points] AS [Total Running Percentage],
        [Total Points]
FROM
    (
    SELECT  EmployeeFirstName,
            EmployeeLastName,
            PointsAwarded,
            TransactionDate,
            SUM(PointsAwarded) OVER (PARTITION BY EmployeeFirstName, EmployeeLastName ORDER BY ID) AS [Employee Running Total],
            SUM(PointsAwarded) OVER (PARTITION BY EmployeeFirstName, EmployeeLastName) AS [Employee Total Points],
            SUM(PointsAwarded) OVER(ORDER BY ID) AS [Running Total Points],
            SUM(PointsAwarded) OVER() AS [Total Points]
    FROM PointsTable
    ) t

This query works and returns the results as expected.  However, I feel like it can be optimized.  Is it possible to include the calculations within the sub query, without repeating code?

For example, this code is necessary to give the Employee Running Percentage:

SUM(PointsAwarded) OVER (PARTITION BY EmployeeFirstName, EmployeeLastName ORDER BY ID) /
SUM(PointsAwarded) OVER (PARTITION BY EmployeeFirstName, EmployeeLastName)

Is there a way to accomplish this with windowing - WITHOUT repeating the OVER functions a bunch of times?  So far, the only way I've been able to achieve this is in the manner I've posted.  I'm thinking a CTE would be more or less the same, but I'm not an expert.

Here is the execution plan.

That would be a fraction not a percentage

You should add an order by EmployeeFirstName, EmployeeLastName, ID


Making two classes work ambidextrously I've written some script in python following OOP guidelines. I've created two classes in my script in such a way so that both of them are able to make requests and produce flawless results. It can parse names, phone numbers and email addresses from yellowpage. I've already executed my script and found it working as I expected. Now it is able to parse the aforesaid category from any links from that site irrespective of how many pages it traverses. So far, I've used one class for making requests and the other for processing results. However, this time I intend to make a bridge between these two classes so that they can both be able to make requests and can do the same stuff if needed. I've tried my best to make a good class crawler. Any suggestions to make it more dynamic or improved will be highly appreciated.

from lxml import html
import requests

class web_parser:

    page_link = "https://www.yellowpages.com/search?search_terms=pizza&geo_location_terms=San%20Francisco%2C%20CA"
    main_url = "https://www.yellowpages.com"

    def __init__(self):
        self.direct_links = [self.page_link]
        self.vault = []

    def parser(self):
        for link in self.direct_links:
            self.get_link(link)
            self.processing_main_link(link)

    def processing_main_link(self, sub_links):
        page = requests.get(sub_links)
        tree = html.fromstring(page.text)
        page_links = tree.cssselect("div.pagination a")
        for page_link in page_links:
            if self.main_url + page_link.attrib["href"] not in self.direct_links:
                self.direct_links.append(self.main_url + page_link.attrib["href"])

    def get_link(self, url):
        page = requests.get(url)
        tree = html.fromstring(page.text)
        item_links = tree.cssselect('h2.n a:not([itemprop="name"]).business-name')
        for item_link in item_links:
            if self.main_url + item_link.attrib["href"] not in self.vault:
                self.vault.append(self.main_url + item_link.attrib["href"])

class get_docs(web_parser):

    def __init__(self):
        web_parser.__init__(self)

    def procuring_links(self):
        for link in self.vault:
            self.using_links(link)

    def using_links(self, newly_created_link):
        page = requests.get(newly_created_link)
        tree = html.fromstring(page.text)
        name = tree.cssselect('div.sales-info h1')[0]
        phone = tree.cssselect('p.phone')[0]
        try:
            email = tree.cssselect('div.business-card-footer a.email-business')[0].attrib["href"].replace("mailto:","")
        except IndexError:
            email = ""
        print(name.text, phone.text, email)

if __name__ == '__main__':

    crawl = get_docs()
    crawl.parser()
    crawl.procuring_links()

Naming convention: classes should normally use PascalCase.
Requests best practices: if you want to use the requests library, you can see my answer about requests usage.
Speeding up your script:

Threading: you can use the threading standard library to get data from URLs and process that data.
Concurrent Futures: this approach exists from Python 3.2, there is a good example on the docs page. But you may have problems with using requests with the concurrent.futures library, so take a look at standard.
Scrapy: modern Python for scraping data from websites. It provides lots of built-in functionality: from scrapping to storing results in a way you wish.

Logging: it is a good practice to use logging while doing any actions, e.g. scrapping.
Exceptions: adding exception handlers is good practice. At the moment, if your script fails in any line, it stops all execution.


Flattening a dictionary into a string Given:

k = {'MASTER_HOST': '10.178.226.196', 'MASTER_PORT': 9999}

I want to flatten into the string:

"MASTER_HOST='10.178.226.196', MASTER_PORT=9999"

This is the ugly way that I'm achieving this right now:

result = []
for i,j in k.iteritems():
    if isinstance(j, int):
        result.append('%s=%d' % (i,j))
    else:
        result.append("%s='%s'" % (i,j))

', '.join(result)

I'm sure my developer colleagues are going to chastize me for this code. Surely there is a better way.

For python 3.0+ (as @Serdalis suggested)

', '.join("{!s}={!r}".format(key,val) for (key,val) in k.items())

Older versions:

', '.join("%s=%r" % (key,val) for (key,val) in k.iteritems())


Methods improvements I just recently learned how to write Methods in C#. Here is one experiment I made. It calculates addition, subtraction, division, multiplication, and the square root of a number. fill free to give me suggestions on how I can improve the code. I also do know I need to set up a system that prevents the user from typing wrong answers. I just don't have enough experience to do it yet (I tried).
static void Main(string[] args)
    {
       string userAnswer;

       Console.WriteLine("Calculater or Square Root Calculater?");
       Console.WriteLine("-Please respond with Calculater or Square");
       userAnswer = Console.ReadLine().ToUpper();
       Console.Clear();

       switch(userAnswer)
       {
           case "CALCULATER":
            Calculater();
           break;
           case "SQUARE":
            SquareRootCalculater();
           break;
           default:
           Console.WriteLine("Invalid Input");
           break;
       }

        Console.ReadKey();
    }

     static void SquareRootCalculater()
    {
        int squareNumber;

        Console.WriteLine("Input Number:");
        squareNumber = Convert.ToInt32(Console.ReadLine());
        Console.Clear();

        Console.WriteLine($"The Square Root of {squareNumber} is");
        Console.WriteLine(Math.Sqrt(squareNumber));
    }

    static void Calculater()
    {
        int firstNumber;
        string sign;
        int secondNumber;
        int answer = 0;

        Console.WriteLine("Input Number:");
        firstNumber = Convert.ToInt32(Console.ReadLine());
        Console.Clear();

        Console.WriteLine("Input Operation:");
        sign = Console.ReadLine();
        Console.Clear();

        Console.WriteLine("Input Second Number:");
        secondNumber = Convert.ToInt32(Console.ReadLine());
        Console.Clear();

        switch (sign)
        {
            case "+":
                answer = firstNumber + secondNumber;
            break;
            case "-":
                answer = firstNumber - secondNumber;
            break;
            case "*":
                answer = firstNumber * secondNumber;
            break;
            case "/":
                answer = firstNumber / secondNumber;
            break;
            default:
                Console.WriteLine("Invalid Input");
            break;
        }
        Console.WriteLine($"The answer of {firstNumber} {sign} {secondNumber} is");
        Console.WriteLine(answer);

    }

You should aim to declare variables at initialization when possible. Pre-initialized variables are suspicious and make me wonder whether they are being optionally assigned or edited elsewhere.
       // string userAnswer;

       Console.WriteLine("Calculater or Square Root Calculater?");
       Console.WriteLine("-Please respond with Calculater or Square");
       string userAnswer = Console.ReadLine().ToUpper();
       Console.Clear();


Angular UI Bootstrap Modal scope I'm trying to improve my Angular skills by writing some basic applications that use a particular feature.  The following application uses the UI Bootstrap modal to add users to a list, or remove them again.

Any feedback on any part of the code is very much appreciated, but I have some specific concerns around the way data is passed between the modal and the parent scope.  It can be done by either setting scope=$scope in the $uibModal options, or by using resolve.  Which is better in this scenario and why?

Here's an extract from the main controller:

$scope.open = function() {
  var modalInstance = $uibModal.open({
    templateUrl: 'add_user.html',
    controller: 'ModalController',
    scope: $scope
  });

  modalInstance.result.then(function(res){
    $scope.userGroup = res;
  });
};

and here's an extract from the modal controller:

$scope.modalUserGroup = angular.copy($scope.userGroup);

$scope.add = function(){
  $scope.modalUserGroup.push($scope.newUser);
};

$scope.delete = function(userIndex){
  $scope.modalUserGroup.splice(userIndex, 1);
};

$scope.save = function() {
  $uibModalInstance.close($scope.modalUserGroup);
};

Here's the plunk. I've also written up my notes on it encase it's useful to anyone else.

Since your code is not huge and it seems clean so far, my suggestion will focus on your question about the best way to share controller data.

You could try the bindToController option when creating the modal, so it shares the same scope as the parent controller

In this post, you will find further information with an example.


Thread safe enqueue and dequeue method in Queue As an exercise, I have implemented a linked queue whose enqueue and dequeue methods are synchronized. There is a Producer thread which keeps inserting elements into the queue as long as there are less than 10 items in it. When the queue is full, the thread waits. The second thread Consumer keep removing elements  from queue as long as it is not empty. I would like to get a review of this code - is there any flaw? How could I improve it?

MyLinkedQueue.java

public class MyLinkedQueue<T> implements Iterable<T> {
  private Node first, last;
  private int size = 0;
  private static final int MAX_QUEUE_SIZE = 10;
  public Iterator<T> iterator() { return new QueueIterator(); }

  private class Node {
    T item;
    Node next;
  }

  public boolean isEmpty() { return first == null; }
  public boolean isFull() { return MAX_QUEUE_SIZE == size(); }

  public synchronized void enqueue(T item) throws InterruptedException {
      while (isFull()) {
        wait();
      }

      Node oldLast = last;
      last = new Node();
      last.item = item;
      last.next = null;

      if (isEmpty()) {
        first = last;
      } else {
        oldLast.next = last;
      }
      notifyAll();
      size++;
    }

  public synchronized T dequeue() throws InterruptedException {
      while (0 == size()) {
        wait();
      }

      T item = first.item;
      first = first.next;

      if (isEmpty()) { last = null; }
      size--;
      notifyAll();
      return item;
    }

  public synchronized int size() { return size; }

  private class QueueIterator implements Iterator<T> {
    private Node current = first;

    public boolean hasNext() { return current != null; }
    public void remove() {}
    public T next() {
      T item = current.item;
      current = current.next;
      return item;
    }
  }
}

Producer.java

public class Producer implements Runnable {
  private static final int DELAY = 1000;
  private MyLinkedQueue queue;
  private int count;

  public Producer(MyLinkedQueue queue, int count) {
    this.queue = queue;
    this.count = count;
  }

  public void run() {
    try {
      for (int i = 0; i < count; ++i) {
        queue.enqueue(new Date().toString());
        Thread.sleep(DELAY);
      }
    } catch(InterruptedException e) {
    }
  }
}

Consumer.java

public class Consumer implements Runnable {
  private static final int DELAY = 1000;
  private MyLinkedQueue queue;
  private int count;

  public Consumer(MyLinkedQueue queue, int count) {
    this.queue = queue;
    this.count = count;
  }

  public void run() {
    try {
      for (int i = 0; i < count; ++i) {
        queue.dequeue();
        Thread.sleep(DELAY);
      }
    } catch(InterruptedException e) {
    }
  }
}

QueueThreadrunner.java

public class QueueThreadrunner {
  public static void main(String[] args) {
    MyLinkedQueue queue = new MyLinkedQueue();
    final int THREADS = 10;
    final int REPETITIONS = 50;

    for (int i = 1; i <= THREADS; ++i) {
      Producer pr = new Producer(queue, REPETITIONS);
      Consumer cr = new Consumer(queue, REPETITIONS);

      Thread dt = new Thread(pr);
      Thread wt = new Thread(cr);

      dt.start();
      wt.start();
    }
  }
}

It is apparent that you have put some thought in to this implementation, and apart from the not-synchronized isEmpty() method, it all looks functional.

I have some minor nit-picks though, and please bear in mind that these are just my opinion...

I prefer to have the notifyAll method to be the last thing in a synchronized block, where possible... You have:

....
  }
  notifyAll();
  size++;
}

I would always put the notifyAll() after the size++. This does not make a functional difference to the program, but it makes the logic more apparent (that size++ will happen before any other thread can read the size value):

....
  }
  size++;
  notifyAll();
}

As I say, this does not affect the logic of the code, but it does affect the readability.

A second nit-pick I have is that you 'leak' logic from your synchronized blocks. What I mean by this, is that you should try to avoid any method calls from inside the block. When you call other methods from a block it becomes much easier to introduce synchronization bugs and deadlocks. Additionally, the methods you call are also synchronized, so you introduce a readability/complexity (and perhaps even a slight performance) issue by having nested synchronization calls.

EDIT: I would add another nit-pick here which is you do not use 'final' enough. 'final' is a good hint to the optimizer in Java and it allows it to make smarter choices when inlining and otherwise rearranging/compiling your code

Finally, I would choose just one mechanism to manage the empty/full condition of the queue. Currently you have a few: isEmpty depends on first == null; isFull() depends on size == MAX_QUEUE_SIZE; in your enqueue(...) you test for isFull() and isEmpty(); and in your dequeue() you test for 0 == size() and isEmpty(). I would reduce all of these to tests on size only. That way there is only one condition that you need to mentally track when you read the code.

So, if it were me, I would replace your code as follows:

public synchronized boolean isEmpty() { return size == 0; } // replace first == null
public synchronized boolean isFull()  { return size == MAX_QUEUE_SIZE; } // replace size()

public synchronized void enqueue(final T item) throws InterruptedException { // add final
  while (size == MAX_QUEUE_SIZE) {  // replace isFull() call.
    wait();
  }

  final Node oldLast = last; // add final
  last = new Node();
  last.item = item;
  last.next = null;

  if (size == 0) { // remove isEmpty()
    first = last;
  } else {
    oldLast.next = last;
  }
  size++; // move before notifyAll()
  notifyAll();
}

public synchronized T dequeue() throws InterruptedException {
  while (size == 0) { // remove size() call.
    wait();
  }

  final T item = first.item; // add final
  first = first.next;

  size--; // move size-- up a bit to make the logic below more transparent
  if (size == 0) { // remove isEmpty() call
      last = null;
  }
  notifyAll();
  return item;
}

Finally, if it really was me, I would recommend that you consider ReentrantLocks and Conditions to manage the queue.... There's a good example (almost identical to this exact problem) of how to do it in the JavaDoc.


re-write javascript array I am using a long array of over 300 var & 7,000 lines of code written like this:

var a = [];
var b = [];
var c = [];
var d = [];
var e = [];

a[0] = "a";
b[0] = "b";
c[0] = "c";
d[0] = "d";
e[0] = "e";

a[1] = "1";
b[1] = "2";
c[1] = "3";
d[1] = "4";
e[1] = "5";

a[2] = "one";
b[2] = "two";
c[2] = "three";
d[2] = "four";
e[2] = "five";

Im guessing it is the same as a much cleaner and shorter -

var a = [a,1,one];
var b = [b,2,two];
var c = [c,3,three];
var d = [d,4,four];
var e = [e,5,five];

Is there an easy or automatic way to rewrite the original array like the 2nd method?

First of all I think you meant:

a = ["a","1","one"]; // instead of [a, 1, one]

I handle these rare kind of issues as follow:

Open your browser (the following code is tested against Chrome)
Open the JavaScript console [Control -Shift -J (Chrome:Windows/Linux)]
Copy/paste your own code into the console

and then paste the following code

copy('abcde'.split('').map(function(varName){
return 'var ' + varName + ' = ' + JSON.stringify(window[varName])+';';
}).join('\n'));

If your variable names are more than 1 character use this code instead: (of course the following code won't work with the file example you specified, I changed the variable names for demonstration purpose)

copy(['var1', 'myVar2', 'blablabla'].map(function(varName){
return varName + ' = ' + JSON.stringify(window[varName])+';';
}).join('\n'));

Hit enter

the above code will copy the following JavaScript code into your clipboard:

a = ["a","1","one"];
b = ["b","2","two"];
c = ["c","3","three"];
d = ["d","4","four"];
e = ["e","5","five"];

Paste it into your file, save it and that's all!


Drawing colored circles using Ruby and Tk I am a high-school freshman who is kinda new to Ruby, and I am doing a small project on Ruby.  One of the big things that I want to get out of this project is how to follow the "Ruby standards" that programmers should follow.  Being as new as I am, I have no clue what I should/shouldn't do with this program.  Can anybody tell me what I could do to improve it to fit the community's standards?

require 'tk'

$point_A = [0,0]
$point_B = [750,750]
$rate = 1.5
$i=0
circs=Array.new

def before_drawing()
    $point_A = []
    temp_a = $point_B[0]**1/$rate
    temp_b = $point_B[1]**1/$rate
    $point_A << temp_a
    $point_A << temp_b
end

def after_drawing()
   $point_B = []
   $point_B = $point_A
end

canvas = TkCanvas.new(:width=>800, :height=>800).pack('fill' => 'both', 'expand'=>true)

while $i<10 do
    before_drawing()
    circs[$i] = TkcOval.new(canvas, $point_A, $point_B)
    if $i%2==0 then
        circs[$i][:fill] = 'blue'
    else
        circs[$i][:fill] = 'red'
    end
    after_drawing()
    $i+=1
end

Tk.mainloop

You should define arrays with [] not Array.new

circs should be $circs in case you wrap your loop in some function.

Before drawing can be turned into this :

def before_drawing()
    temp_a = $point_B[0] ** 1 / $rate
    temp_b = $point_B[1] ** 1 / $rate
    $point_A = [temp_a, temp_b]
end

You should turn $i into a local variable for the loop. There is no need for it to be global.

Then replace the loop with upto.

0.upto(10) do |i|
    before_drawing()

    circs[i] = TkcOval.new(canvas, $point_A, $point_B)
    # As suggested using ternary operator
    # circs [i] [:fill] = i % 2 == 0 ? 'blue' : 'red'

    if i % 2 == 0 then
        circs[i][:fill] = 'blue'
    else
        circs[i][:fill] = 'red'
    end

    after_drawing()
end

You seem to use too many global variables. My advice would be to prefer local variables whenever you can (Like i in the loop). All your global variables can also be made local to the loop (or function in case you wrap the loop in some function).


Drawing the flag of Liberia I am working on this assignment to draw the flag of Liberia for my essentials of computer programming class.

I finished it but had extra time to kill and noticed that a couple similar blocks of code are not very concise and could be shortened with a loop, but right now I can't seem to form the code just right!

Below I have included one of the methods that I would like to shorten:

public static void flagOfLiberia(Graphics g) {
    Expo.setBackground(g,Expo.black);
    for (int d = 1; d <= numDots; d++) {
        int x = Expo.random(0,1000);
        int y = Expo.random(0,650);

        if (x < 300 && y < 300)
            Expo.setColor(g,Expo.darkBlue);
        else if (y < 50)
            Expo.setColor(g,Expo.red);
        else if (y < 100)
            Expo.setColor(g,Expo.white);
        else if (y < 150)
            Expo.setColor(g,Expo.red);
        else if (y < 200)
            Expo.setColor(g,Expo.white);
        else if (y < 250)
            Expo.setColor(g,Expo.red);
        else if (y < 300)
            Expo.setColor(g,Expo.white);
        else if (y < 350)
            Expo.setColor(g,Expo.red);
        else if (y < 400)
            Expo.setColor(g,Expo.white);
        else if (y < 450)
            Expo.setColor(g,Expo.red);
        else if (y < 500)
            Expo.setColor(g,Expo.white);
        else if (y < 550)
            Expo.setColor(g,Expo.red);
        else if (y < 600)
            Expo.setColor(g,Expo.white);
        else if (y < 650)
            Expo.setColor(g,Expo.red);

        drawDot(g,x,y);
        Expo.setColor(g,Expo.white);
        Expo.fillStar(g,150,150,100,5);
    }
    showName(g,"Liberia");
}

public static void flagOfLiberia(Graphics g) {
    Expo.setBackground(g,Expo.black);
    for (int d = 1; d <= numDots; d++) {
        int x = Expo.random(0,1000);
        int y = Expo.random(0,650);

        if (x < 300 && y < 300)
            Expo.setColor(g,Expo.darkBlue);
        else if (y % 100 < 50)
            Expo.setColor(g,Expo.red);
        else
            Expo.setColor(g,Expo.white);

        drawDot(g,x,y);    // must be run inside the loop
    }
    Expo.setColor(g,Expo.white);
    Expo.fillStar(g,150,150,100,5);
    showName(g,"Liberia");
}

The above should be at least a little improvement. I used a modulus (%) to check every multiple of 100, seeing if the remainder is less than or greater than 50. If you desire a cutoff at 650, then a single if statement can be added before the modulus to check that y is below the threshold. Also, I placed the star after the for loop in order to execute it only once.


Speeding up for-loop over a list I have two lists with ca. 4000 elements where each each element have two columns. These are being fed into a function. Besides the function they are being fed into, is it possible to speed it up somehow? At this rate, it will take ca. 6-7 days to complete.

results=rep(0, length(as.numeric(unlist(coors))))
for(i in names(coors)){
  print(i)
  for( j in names(pols)){
    results[i]=point.in.polygon(coors[[i]][,1], coors[[i]][,2], pols[[j]][,1], pols[[j]][,2])
}}

EDIT: further description, I have the polygons for each ind in year 1 and the coordinates for year 2. I want to see how many of the locations from year 2 that falls within the polygon from year 1 for each individual.

EDIT 2:   the structure of coors

 str(coors)
 List of 4052
  $ 2225 :'data.frame': 48 obs. of  2 variables:
   ..$ cor.x: num [1:48] 635184 635215 635394 635431 635430 ...
   ..$ cor.y: num [1:48] 7151002 7151201 7151175 7151110 7151118 ...
  $ 2226 :'data.frame': 56 obs. of  2 variables:
   ..$ cor.x: num [1:56] 635945 635936 635944 635969 635947 ...
   ..$ cor.y: num [1:56] 7152813 7152847 7152834 7152785 7152810 ...
  $ 2227 :'data.frame': 56 obs. of  2 variables:
   ..$ cor.x: num [1:56] 636244 636245 636317 636450 636386 ...
   ..$ cor.y: num [1:56] 7151503 7151505 7151628 7151693 7151799 ...
  $ 2228 :'data.frame': 56 obs. of  2 variables:
   ..$ cor.x: num [1:56] 636451 636418 636408 636467 636495 ...
  ..$ cor.y: num [1:56] 7152610 7152605 7152634 7152572 7152537 ...

There are always either 48 or 56 obs per element level. Pols have similar structure, but more variable lengths as the number of observations depends on the shape of the polygon.

I assume you are using point.in.polygon from the sp package. The function can take any number of points, so you should rewrite your algorithm as a single loop: for each polygon in pols, make a single call to point.in.polygon to check if all the points in coords are inside or not. This should save you a lot of time. Then you'll only have to do a little work reformatting the output. I can help with the code if you can please clarify what coors looks like.

Edit: Here is now an example of how you could code this into a single loop. It is possible you will have to work it a bit as you have not really shown us how you want to store your output.

First, some sample data:

coors <- replicate(5, {n <- sample(5:10, 1);
                       data.frame(x = runif(n), y = runif(n))},
                   simplify = FALSE)

pols <- replicate(3, {n <- sample(5:10, 1)
                      data.frame(x = runif(n), y = runif(n))},
                  simplify = FALSE)

Here, we concatenate all the points in coors together, but keep a vector of group indices which we will use later for splitting by group:

all.coors  <- do.call(rbind, coors)
num.points <- sapply(coors, nrow)
group.idx  <- rep(seq_along(coors), num.points)

Now the single loop:

results <- vector("list", length(pols))
for (j in seq_along(pols)) {
   print(j)
   results[[j]] <- split(point.in.polygon(all.coors[,1], all.coors[,2],
                                          pols[[j]][,1], pols[[j]][,2]),
                         group.idx)
}

I am confident this will significantly speed up your computations. However, if this is still too slow, I agree you'll have to consider parallelization. Good luck.


Generating statistical reports I want to generate statistical reports and I have many different where clauses so the function became long. I did much refactoring, but it is not enough. Can someone help me with some techniques that these techniques make the function short and easily readable?

public static function allRejUserByProv($prov = '', $gender = '', $dist = '', $ttc = '')
    {
        if (!empty($gender) && !empty($prov) && !empty($dist))
        {
            return self::where('decision', '3')->where('gender', $gender)->where('p_province', $prov)->where('p_district', $dist)->count();
        }
        if (!empty($prov) && !empty($dist))
        {
            return self::where('decision', '3')->where('p_province', $prov)->where('p_district', $dist)->count();
        }
        if (!empty($prov) && !empty($gender))
        {
            return self::where('decision', '3')->where('p_province', $prov)->where('gender', $gender)->count();
        }
        if(!empty($gender) && !empty($ttc))
        {
            return self::where('decision', '3')->where('ttc_name', $ttc)->where('gender', $gender)->count();
        }
        if(!empty($ttc))
        {
            return self::where('decision', '3')->where('ttc_name', $ttc)->count();
        }
        if (!empty($gender))
        {
            return self::where('decision', '3')->where('gender', $gender)->count();
        }
        if (!empty($prov))
        {
            return self::where('decision', '3')->where('p_province', $prov)->count();
        }

        return self::where('decision', '3')->count();
    }

I am calling the function like this:

public function resultTTC($prov, $dist, $ttc)
{
return [
  'rejected'             => number_format(self::allRejUserByProv($prov, '', $dist, $ttc)),
        'rejected_male'        => number_format(self::allRejUserByProv($prov, '1', $dist, $ttc)),
        'rejected_female'      => number_format(self::allRejUserByProv($prov, '2', $dist, $ttc)),

]

If you just want to do the where for every non-empty value. You can do the following:

public static function allRejUserByProv($prov = '', $gender = '', $dist = '', $ttc = '')
{
    $fields = [
        'gender' => $gender,
        'p_district' => $dist,
        'p_province' => $prov,
        'ttc_name' => $ttc
    ];

    $result = self::where('decision', '3');

    foreach ($fields as $attr => $value) {
        if(! empty($value)) {
            $result = $result->where($attr, $value);
        }
    }

    return $result->count();
}

This way you will remove the multiple ifs.

If you will always use number_format to format the return value why not just adding it to the function:

return number_format($result->count());

Hope this helps :)


Project Euler Problem 7: 10,001st prime The problem statement is as follows

By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can see that the 6th prime is 13.
What is the 10,001st prime number?

My code that follows is what I used to solve this with.
public class Program
{
    static bool IsPrime(long value)
    {
        if (value==2)
            return true;

        if (value % 2 ==0)
            return false;

        long largestPossibleFactor = (long)Math.Sqrt(value);
        for (int possiblePrimeFactor = 3; possiblePrimeFactor <= largestPossibleFactor; possiblePrimeFactor +=2)
        {
            if (value % possiblePrimeFactor == 0)
                return false;
        }
        return true;
    }

    static long GetPrimeOccurence(int NthOccurence)
    {
        if (NthOccurence==1)
            return 2;

        int primePosition=2;
        long returnValue=0;
        bool numberIsPrime =false;
        for (long testForPrime=3; primePosition <= NthOccurence; testForPrime +=2)
        {
            numberIsPrime = IsPrime(testForPrime);

            if (numberIsPrime && primePosition == NthOccurence)
                returnValue = testForPrime;

            if (numberIsPrime)
                primePosition++;
        }
        return returnValue;
    }

    public static void Main(string[] args)
    {
        Console.WriteLine(GetPrimeOccurence(10001));
    }
}

Aside from using a naive function to find prime, the GetPrimeOccurrence function could use some tightening up.  Most specifically the loop should use a shortcut and return as soon as the nth occurrence is reached:

static long GetPrimeOccurence(int NthOccurence)
{
    if (NthOccurence==1)
        return 2;

    int primePosition=2;
    bool numberIsPrime =false;
    for (long testForPrime=3; primePosition <= NthOccurence; testForPrime +=2)
    {
        numberIsPrime = IsPrime(testForPrime);

        if (numberIsPrime)
        {
            if(primePosition == NthOccurence)
            {
                return testForPrime;
            }
            primePosition++;
        }
    }
    return 0;
}


Checking if two strings are an anagram This is my sample code to check if a given pair of strings are anagram or not.

      static bool AreAnagram(string s1, string s2)
      {

        if (s1.Length != s2.Length)
            return false;

        foreach (char c in s1)
        {
            int ix = s2.IndexOf(c);

            if (ix == -1)
                return false;
        }

        return true;
    }

The complexity of this approach is \$O(n)\$.

What boundary conditions am I missing? Can it be made better?

The complexity of your algorithm is \$O(n^2)\$, since IndexOf has complexity \$O(n)\$. You can get a \$O(n\log n)\$ by sorting the two strings and comparing them.


Nested condition subset of the top one I've got this code

Double best = nodes.get(edge.getToNodeId());
if (best == null || (best > g+h)) {
    nodes.put(edge.getToNodeId(), g+h);
    if (best == null) {
        open.add(new State(edge.getToNodeId(), g, h, top));
    } else {
        open.modify(new State(edge.getToNodeId(), g, h, top));
    }
}

I don't quite like it because I duplicate the best == null clause. Would it be better in these types of situation to duplicate the nodes.put() instead? Or is there some third, more readable, way without duplicates?

I think it's a matter of taste and preference. In either case, you would have to duplicate something. But, there's something that drew my attention. I don't claim it to be correct or be a solution to your problem. Hard to say without knowing the context and data structures that you use. Anyways, here we go...

I noticed that in both cases, whether best == null or not, you pass the same arguments, though to different methods. I suspect that this checking might occur in many other places as well. So, what if we create a method (e.g. addOrModify()) and move this condition statement to that method. And the method would internally decide whether to add a new State or modify the existing one. Somewhat similar behavior may be observed in the java.util.Set.add(E e) method, where the method checks if the element has already been added to the Set and then returns false without adding it twice...

Then, we end up with something like this in the client code:

Double best = nodes.get(edge.getToNodeId());
if (best == null || (best > g+h)) {
    nodes.put(edge.getToNodeId(), g+h);
    open.addOrModify(new State(edge.getToNodeId(), g, h, top));
}


Attempt at Abstracted YAML Configuration Loader I've been playing around with writing a simple static blog generator in Haskell (mostly for experience, since there's probably more than a few already made choices that fit my needs). I don't have very much experience with functional programming, but I've been doing a lot of C++ lately, so using typeclasses as "interfaces" seems natural to me.

For the beginnings configuration, I've come up with this typeclass:

module BlogGenerator.Config where

class Config t where
    outputPath      :: t -> FilePath
    templatePathFor :: String -> t -> FilePath

And for a YAML implementation (using this):

module BlogGenerator.YamlConfig
    ( YamlConfig
    , toYamlValue
    , toYamlConfig
    ) where

import qualified Data.HashMap.Strict as HashMap
import Data.Maybe
import Data.ByteString
import Data.Text as Text
import BlogGenerator.Config
import qualified Data.Yaml as Yaml

newtype YamlConfig = YamlConfig
    { toYamlValue :: Yaml.Value
    }

toYamlConfig :: ByteString -> YamlConfig
toYamlConfig = YamlConfig . fromJust . decode
  where
    decode = (Yaml.decode :: ByteString -> Maybe Yaml.Value)

instance Config YamlConfig where
    templatePathFor name = stringYamlLookup ["templates", name] . toYamlValue
    outputPath = stringYamlLookup ["output", "path"] . toYamlValue

-- helpers
yamlLookup :: [String] -> Yaml.Value -> Yaml.Value
yamlLookup []         _     = Yaml.Null
yamlLookup (key:[])   value = fromJust $ look key $ toObject value
  where
    look k v = HashMap.lookup (Text.pack k) v
    toObject = fromJust . toOptionalObject
    toOptionalObject (Yaml.Object object) = Just object
    toOptionalObject _                    = Nothing
yamlLookup (key:rest) value = yamlLookup rest (yamlLookup [key] value)

stringYamlLookup :: [String] -> Yaml.Value -> String
stringYamlLookup ks v = Text.unpack (toText (yamlLookup ks v))
  where
    toText = fromJust . toOptionalText
    toOptionalText (Yaml.String text) = Just text
    toOptionalText _ = Nothing

I know the YAML package contains support for automagic binding to records, but tight coupling between the external format and the internal format is fairly unappealing to me, and before I start trusting advanced code like that I feel better about writing a "low-tech" version of my own.

Is this general approach "idiomatic" for Haskell, and am I following good practices/idioms in general?

Sorry to say this is not good practice. Exactly why is a bit more technical of an explanation than I like to give off the cuff, Google for "Haskell typeclass interface" and you'll find a bunch of threads comparing and contrasting and recommending you use an ADT. Here's one from Reddit with a game programming example that I think is particularly practical.

In your case, I imagine you intend to provide different configuration methods (XML, command line flags, &c) which would also be instances of Config. You've already realized that you want to work with some abstract Config thing elsewhere in your program and not a YamlConfig, because whatever is using a configuration value doesn't care where that value came from. The next logical step is that by making Config a class, you actually are still passing a YamlConfig around everywhere, you're just relying on callers to never look more closely at it than as a generic Config instance. You've got implementation leaking all over the place.

Instead, making Config a datatype.

data Config = Config { outputPath :: FilePath
                     , template   :: String -> FilePath
                     }

Now populate it from a YAML source.

makeConfig :: Yaml.Value -> Config
makeConfig yaml = Config { outputPath = stringYamlLookup ["output", "path"] yaml
                         , template   = \name -> stringYamlLookup ["templates", name] yaml
                         }

It's easy to make arbitrary Configs now. Maybe you want a default Config.

defaultConfig :: Config
defaultConfig = Config { outputPath = "output/"
                       , template   = ("template/" ++)
                       }

You can even make lists of Configs from different sources ([defaultConfig, makeConfig yaml]).


Caesar Cipher written in Python I have written a simple Caesar Cipher in Python in my efforts to learn a language that isn't C. It works much like any other Caesar implementation and takes various command line arguments to specify options such as the spacing of the output, whether to encrypt or decrypt the input, and whether or not to try and bruteforce the cipher.

This is my first ever Python program so I'm primarily looking to learn some best practices for the language as well as remove any inefficiencies or redundancies in my code.

Thanks!

import argparse
import re

def caesar(string, shift):
    output = ''
    for char in string:
        if char.isspace():
            output += ' '
        else:
            output += chr(((ord(char) - 65 + shift) % 26) + 65)
    return output

def main():
    parser = argparse.ArgumentParser(description = "Encrypt or decrpyt a string using the Caesar Cipher")

    parser.add_argument("--bruteforce", "--brute", "-b", help = "bruteforce mode", action = "store_true")
    parser.add_argument("--encrypt", "--enc", "-e", help = "encryption mode (default)", action = "store_true")
    parser.add_argument("--decrpyt", "--dec", "-d", help = "decryption mode", action = "store_true")
    parser.add_argument("--no-spacing", "--no-space", "-n", help = "no spacing in output (default)", action = "store_true")
    parser.add_argument("--preserve-spacing", "--preserve", "-p", help = "use same spacing as original string", action = "store_true")
    parser.add_argument("--shift", "-s", help = "value for the Caesar shift", type = int, choices = range(1, 26))
    parser.add_argument("--spacing", "-x", help = "specify the spacing in output", type = int)

    args = parser.parse_args()

    if args.bruteforce:
        bruteforce = True
    else:
        bruteforce = False
        shift = args.shift

    if args.decrpyt:
        shift = -shift

    if args.preserve_spacing:
        regex = re.compile('[^A-Z\s]')
    else:
        regex = re.compile('[^A-Z]')

    string = regex.sub('', input().upper())

    if args.spacing:
        string = ' '.join([string[i:i + args.spacing] for i in range(0, len(string), args.spacing)])

    if bruteforce:
        for shift in range(1, 26):
            print("%d: %s" %(shift, caesar(string, -shift)))
    else:
        print(caesar(string, shift))

if __name__ == "__main__":
    main()

The Caesar function

First the parameter string should be renamed as it has the same name as the standard module string. Sure you don't use it, but it is a bad habit to get into and can result in problems later on. Regardless, since we're dealing with cryptography and sending messages and such, wouldn't a better word be message?

There is also no documentation. caesar is not a descriptive name if you don't already know what a Caesar cipher is. You should have a docstring that describes that this a Caesar cipher and at least link to Wikipedia for a description of the Caesar cipher if not describe it yourself.

The main function

store_true

if args.bruteforce:
    bruteforce = True
else:
    bruteforce = False
    shift = args.shift

The 'store_true' flag indicates that args.bruteforce is True when the flag is specified and False when it is not specified. Forget the bruteforce variable and just concern yourself with args.bruteforce.

store_const

There is also a store_const action that has the optional1 parameters const and default. Thus you can have const=re.compile('[^A-Z\s]') and default=re.compile('[^A-Z]'). This removes the need for:

if args.preserve_spacing:
    regex = re.compile('[^A-Z\s]')
else:
    regex = re.compile('[^A-Z]')

1 The const parameter is a paradoxically optional argument that is required


High udpclient CPU use I have a bunch of different ports I want to listen on and the iterate over the available data to send to a Dataflow pipeline. In total I'm listening on 14 ports. I'm looking for any advice on how to reduce the CPU usage of the following code.

I'm just passing in the ports to a method then adding them to a list:

   public static void AddPorts(Dictionary<int,string> ports)
{
    try
    {

            var NewEndPoints = new List<IPEndPoint>();
            foreach (var port in ports)
            {
                var endpoint = new IPEndPoint(IPAddress.Any, port.Key);
                NewEndPoints.Add(endpoint);
                if (!Endpoints.Contains(endpoint))
                {
                    Endpoints.Add(endpoint);
                    var client = new UdpClient(endpoint);
                    logger.Info("New Client added on port: {0}", endpoint.Port);
                    Clients.Add(client);

                }
                else
                {
                    if (IgnoredPorts.Contains(endpoint.Port))
                    {
                        logger.Info("Existing client enabled on port: {0}", endpoint.Port);
                        IgnoredPorts.Remove(port.Key);
                    }
                }
            }
            var differences = Endpoints.Except(NewEndPoints);
            differences.ToList().ForEach(d =>
            {
                if (!IgnoredPorts.Contains(d.Port))
                {
                    IgnoredPorts.Add(d.Port);
                    logger.Info("Client removed on port: {0}", d.Port);
                }
            });

    }
    catch (Exception ex)
    {
        logger.Error("Error creating udpclients", ex);
    }
}

I then iterate the sockets for any available data

 Task.Run(async delegate
    {

        while (Receive)
        {
            try
            {
                // get any channels that have data availble
                // Iterate over the the channels and send to Dataflow pipeline
                var readyChannels =
            (from channel in Clients
             where channel.Available > 0 && !ListenersDF.IgnoredPorts.Contains(((IPEndPoint)channel.Client.LocalEndPoint).Port)
             select channel);

                // Iterate over the the channels and send to Dataflow pipeline
                foreach (var channel in readyChannels)
                {
                    // await on the result of the task
                    await ReceiveAndRespond(channel);
                }

            }
            catch (Exception ex)
            {
                logger.Error("Error sending packet to bufferBlock", ex);
            }
        }
    });

And finally send it to the TPL Dataflow pipline

async Task ReceiveAndRespond(UdpClient channel)
{
    UdpReceiveResult? result = null;

    try
    {
        result = await channel.ReceiveAsync();
    }
    catch (Exception exc)
    {
        logger.Error("Error receiving from channel: " + exc.Message, exc);
    }

    if (result != null)
    {
        var device = (from d in Ports
                      where d.Key == ((IPEndPoint)channel.Client.LocalEndPoint).Port
                      select d.Value).FirstOrDefault();
        UdpData data = new UdpData() { Client = channel, Data = result.Value.Buffer, LocalPort = ((IPEndPoint)channel.Client.LocalEndPoint).Port, LocalIP = ((IPEndPoint)channel.Client.LocalEndPoint).Address, RemoteEndpoint = result.Value.RemoteEndPoint, Device = device };
        Flow.bufferBlock.Post(data);

        // for testing logs the hex string to a log file
        //logger.Warn(string.Format("Data received on port: {0} for device: {1} with data: {2}", data.LocalPort, data.Device, data.Data.ByteArrayToHexString()));
    }
}

Then CPU sits at 50% with hardly any traffic and I'm sure there is some performance to be gained just not sure where.

Note that Endpoints.Contains(endpoint) will never be true, because you create a new IPEndPoint, and it does not implement IEquatable interface.

In addition to @mjolka's description why you have a 50% CPU load, I would like to suggest how you can avoid it.

Currently you have a central place that manages all sockets in one place. I suggest to do it differently - have each endpoint handle the data on its own, and manage all sockets only as a collection of ongoing processing (tasks) like this:

public async Task RunListenerAsync(int port)
{
    var endpoint = new IPEndPoint(IPAddress.Any, port);
    using (var client = new UdpClient(endpoint))
    {
        while (true)
        {
            try
            {
                var result = await client.ReceiveAsync().ConfigureAwait(false);
                ProcessEndpointData(client, result);
            }
            catch (Exception exc)
            {
                logger.Error("Error receiving from channel: " + exc.Message, exc);
                return;
            }
        }
    }
}

private void ProcessEndpointData(UdpClient client, UdpReceiveResult result)
{
    var localEndPoint = (IPEndPoint)client.Client.LocalEndPoint;
    var device = (from d in Ports
                    where d.Key == localEndPoint.Port
                    select d.Value).FirstOrDefault();

    var data = new UdpData
    {
        Client = client,
        Data = result.Buffer,
        LocalPort = localEndPoint.Port,
        LocalIP = localEndPoint.Address,
        RemoteEndpoint = result.RemoteEndPoint,
        Device = device
    };
    Flow.bufferBlock.Post(data);
}

Note that in this implementation the only way to stop listening the port is to get an exception on await client.ReceiveAsync(). You might want to introduce the CancellationToken to have a control when to stop this process.

With this approach registering and listening for all ports will be as simple as:

public Task ListenPortsAsync(IEnumerable<int> ports)
{
    return Task.WhenAll(ports.Select(RunListenerAsync));
}


My attempt at FizzBuzz (Python 3) Is this a good solution for FizzBuzz? What can I do better?

for num in range(1, 101):
    if num % 3 == 0:
        if num % 5 == 0:
            print("FizzBuzz")
        else:
            print("Fizz")
    elif num % 5 == 0:
        print("Buzz")
    else:
        print(num)

It's a good attempt, it reads easily and I like that you nested the FizzBuzz check within Fizz branch.

This doesn't look like most of the code you see in the wild, though, so a good exercise would be to extract a function for the FizzBuzz logic that is independent from the loop over numbers.

def get_fizzbuzz(num:int) -> str:
    result = ""
    # fizzbuzz logic
    if num % 3 == 0:
        if num % 5 == 0:
            result = "FizzBuzz"
        # ...
    return result

You could even make it more flexible by optionally allowing different numbers to be entered instead of 3 and 5, while still using 3 and 5 as default values, like so:

 def get_fizzbuzz(num:int, fizz:int = 3, buzz:int = 5) -> str:
    result = ""
    # fizzbuzz logic
    if num % fizz == 0:
        if num % buzz == 0:
            result = "FizzBuzz"
        # ...
    return result

Then you can just call the function in a loop using the default values:

for num in range(1, 101):
    print(get_fizzbuzz(num))

Or alternatively with different values for fizz and buzz:

for num in range(1, 101):
    print(get_fizzbuzz(num, 5, 7))

That will help you make your code more modular.

Note that I used Python 3's type hints in the function definition, which are completely optional but can make the code more clear, as well as provide static code analysis with certain tools.


Helper class for accessing blobs and mmap memory This class was inspired from Microsoft's array_view.

It intended to use with mmap-ed memory for easily check bounds and so on.

I probably will need to throw exceptions, but this is not my usual way of working.

Any comments are welcome.

#include <cstring>

class BlobRef{
public:
    BlobRef(const void *mem, size_t const size) noexcept :
                mem_( (const char *) mem),
                size_(size){}

    // for test purposes
    BlobRef(const char *s) noexcept :
                BlobRef(s, strlen(s) + 1){}

    template<size_t N>
    BlobRef(const char(&mem)[N]) noexcept:
                BlobRef(mem, N){}

public:
    bool empty() const noexcept{
        return mem_ == nullptr || size_ == 0;
    }

    size_t size() const noexcept{
        return size_;
    }

    const void *data_() const noexcept{
        return mem_;
    }

public:
    const void *safeAccessMemory(size_t const pos, size_t const size) const noexcept{
        if (pos + size <= size_)
            return & mem_[pos];
        else
            return nullptr;
    }

    template <class T>
    const T *as(size_t const pos = 0, size_t const elements = 1) const noexcept{
        return (const T *) safeAccessMemory(pos, elements * sizeof(T));
    }

private:
    const char  *mem_ = nullptr;
    size_t      size_ = 0;
};

#include <cstdint>
#include <assert.h>
#include <endian.h>

int test_blobref(){
    constexpr size_t SIZE = 256;
    char mem[SIZE];

    for(int i = 0; i < SIZE; ++i)
        mem[i] = (char) i;

    BlobRef br{ mem };

    assert(*br.as<uint16_t>(0x00) == htobe16(0x0001)    );
    assert(*br.as<uint16_t>(0x0E) == htobe16(0x0E0F)    );
    assert(*br.as<uint32_t>(0x10) == htobe32(0x10111213)    );

    {
        const char *s = br.as<char>('a');
        assert(strncmp(s, "abcde", 5) == 0);
    }

    {
        struct TestStruct{
            uint16_t    i;
            char        c;
            char        s[4];
        }__attribute__((__packed__));

        const TestStruct *st = br.as<TestStruct>(0x50);
        assert(st->i == htobe16(0x5051)     );
        assert(st->c == 0x52            );

        const char *s1 = st->s;
        const char s2[] = { 0x53, 0x54, 0x55, 0x56, 0x57 };
        assert(strncmp(s1, s2, sizeof s2) == 0);
    }

    {
        size_t const pos = SIZE / sizeof(uint64_t);

        const uint64_t *u64 = br.as<uint64_t>(0, pos);
        assert(u64 != nullptr   );
        assert(u64[      0] == htobe64(0x0001020304050607)  );
        assert(u64[pos - 1] == htobe64(0xf8f9fafbFCFDFEFF)  );
    }

    {
        size_t const pos = SIZE / sizeof(uint64_t) ;

        const uint64_t *u64 = br.as<uint64_t>(0, pos + 1 );
        assert(u64 == nullptr   );
    }

    return 0;
}

int main(){
    test_blobref();
}

BlobRef details

You should provide a default constructor.
safeAccessMemory contains a bug. If you have an empty BlobRef and you call safeAccessMemory(0, 0) you will deference a nullptr (the guard fails).
Don't use c-style casts. In your cases you should use reinterpret_cast. This helps readability, search-ability and signals clear intent.
as can invoke undefined behaviour if called on certain types. The gritty details are in 3.10.10 of the C++ spec.

  If a program attempts to access the stored value of an object through
  a glvalue of other than one of the following types the behavior is
  undefined:

  the dynamic type of the object.

  a cv-qualified version of the dynamic type of the object.

  a type similar (as defined in 4.4) to the dynamic type of the object.

  a type that is the signed or unsigned type corresponding to the dynamic type of the object.

  a type that is the signed or unsigned type corresponding to a cv-qualified version of the dynamic type of the object.

  an aggregate or union type that includes one of the aforementioned types among its elements or nonstatic data members (including, recursively, an element or non-static data member of a subaggregate or contained union), a type that is a (possibly cv-qualified) base class type of the dynamic type of the object.

  a char or unsigned char type.

My understand is that there's no easy way to make this work (the gsl folks removed theirs)

Unfortunately this would remove the entire essence of this class. One approach you could take to remedy this would be to use type-traits to restrict the function to types for which it will be well defined.

template <class T, typename = std::enable_if_t<std::is_integral<T>::value>>
const T *as(size_t const pos = 0, size_t const elements = 1) const noexcept{
    return (const T *) safeAccessMemory(pos, elements * sizeof(T));
}


Attempting to efficiently compute the largest prime factor I'm working on the third Euler problem and I've written a simple little function that returns the list of prime factors of a given number.

The number for which I have to get all the factors is over 6 billion, so building this huge freakin list of numbers in memory struck me as wonky.

I decided I'd build a factor function that generates output by the chunk and spits it out so that I can write it to a text file chunk by chunk.  A buffered_factor function:

Ok, I've decided that doing the 'buffered' thing isn't needful.  The computation is just going to take a while, and making my factor function a generator will keep my program light in memory.

I'm looking for best practices and practical advice about (what I think is describable as) buffering the output of a function.

Here's the prime sieve:

def is_prime(n):
    if n == 1:
        return False
    if n == 2:
        return True

    for i in range(2, int(math.ciel(math.sqrt(n))):
        if n % i:
            return False
        return True

And the factoratorizer:

def factor(num):
    """Get factors of given number.

    :param num: get us them there factors o this number here
    :type num: integer
    :param chunk_size: number of operations to work through before crapping out a list
    :rtype: list of factors
    :raises: ValueError on num < 1
    """
    if num < 1:
        raise ValueError('Given %s. Numbers >= 1, please' % num)

    for i in (n + 1 for n in xrange(num)):
        if num % i == 0:
            if is_prime(i):
                yield i

I'm trying to use it thusly:

import os
from factor import factor
TARGET = 600851475143
OUTFILE = 'factors.txt'

with open(OUTFILE, 'w') as f:
   for output in buffered_factor(TARGET):
       f.write(str(output))
       f.flush()

Nothing's being written to file, so obviously I'm doing something wrong.

Some advice about maths and algorithms which will be useful for this problem and for future problems:

You are interested in prime factors.
You don't care about the non-biggest factors (you might go through them but there is not much point in storing them).
If a number n can be decomposed in n = m*p then at least one of m and p is smaller than sqrt(n).