Select list items in ViewModel fetched using a Repository Doing database access in ViewModel is generally considered a bad practice. However, when you have a <select> in your view, you need to populate it using the ViewBag or the ViewModel (I prefer the ViewModel). This leads to the following code:

public class NewUserViewModel
{
    public string Username {get;set;}
    public IEnumerable<SelectListItem> Cities {get;set;}
    public User User // Helper to retrieve user
    {
        get { return new User() { Username = this.Username; }}
        set { this.Username = value.Username; }
    }
}

We now have to populate the Cities twice:

public ActionResult New()
{
    var vm = new NewUserViewModel();
    vm.Cities = addressRepository.GetAllCities().Select(c => new SelectListItem() { Text = c.Name, Value = c.Id });
    return View(vm);
}

[HttpPost]
public ActionResult Create(NewUserViewModel vm)
{
    if(ModelState.IsValid)
    {
        userRepository.Add(vm.User);
        return RedirectToAction("Index");
    }
    else
    {
        vm.Cities = addressRepository.GetAllCities().Select(c => new SelectListItem() { Text = c.Name, Value = c.Id }); // Copy this line
        return View(vm);
    }
}

I came up with an approach to solve this problem:

public NewUserViewModel(AddressRepository addressRepository) {
    this.Cities = addressRepository.GetAllCities().Select(c => new SelectListItem() { Text = c.Name, Value = c.Id });
}

I overrode the DefaultModelBinder to inject the Repository into the ViewModel.

protected override object CreateModel(ControllerContext controllerContext, ModelBindingContext bindingContext, Type modelType)
{
    return DependencyResolver.Current.GetService(modelType);
}

Now my controller is free:

public ActionResult New()
{
    // Is this bad too?
    return View(DependencyResolver.Current.GetService<NewUserViewModel>());
}

public ActionResult Create(NewUserViewModel vm)
{
    if(ModelState.IsValid)
    {
        userRepository.Add(vm.User);
        return RedirectToAction("Index");
    }
    else
    {
        return View(vm);
    }
}

If I think about it, having all the cities listed is ViewModel's job, not the controller's. This has one immediate downside: it fetches the Cities even when the ModelState.IsValid. However, that is not my concern. Is this bad practice? I won't do any more database access than this.

Yes I think its bad practise because you have introduced both complexity and dependencies when it simply isn't necessary. Its going to make your unit testing more difficult than it needs to be and it isn't the intended purpose of the your view models.

Just create a private method in your controller that loads the data and populates the ListItems and call it where necessary. Agreed that it's not pretty but it will cause you less issues in the long run than what you are doing now.

public class AdminController : Controller
{
    ...ctor etc...

    public ActionResult New()
    {
        var vm = new NewUserViewModel();
        PopulateListItems(vm);
        return View(vm);
    }

    [HttpPost]
    public ActionResult Create(NewUserViewModel vm)
    {
        if (ModelState.IsValid)
        {
            userRepository.Add(vm.User);
            return RedirectToAction("Index");
        }
        else
        {
            PopulateListItems(vm);
            return View(vm);
        }
    }

    private static void PopulateListItems(NewUserViewModel vm)
    {
        vm.Cities = _addressRepository.GetAllCities().Select(c => new SelectListItem() { Text = c.Name, Value = c.Id });
    }
}


Java - fill triangle function I am starting to work on a basic 3D renderer for fun, so right now I have made a triangle fill function to be used when I draw faces on models:

public void fillTriangle(Triangle2d t, Color color) {
    // triangle points are already sorted
    Line2d ab = t.getAB(), bc = t.getBC(), ac = t.getAC();
    Line2d leftEdge = ab.getEnd().getX() < ac.getEnd().getX() ? ab : ac;
    Line2d rightEdge = ab.getEnd().getX() > ac.getEnd().getX() ? ab : ac;
    Line2d bottomEdge = bc;

    // horizontal lines
    int startY = (int) t.getA().getY(), endY = (int) t.getC().getY();
    for (int y = startY; y <= endY; y++) {
        int startX, endX = (int) rightEdge.getX(y);
        // we are now below the middle (vertically) point, so to avoid going past the bottom edge,
        // we use it to get the startX
        if (y > leftEdge.getEnd().getY())
            startX = (int) bottomEdge.getX(y);
        else
            startX = (int) leftEdge.getX(y);

        for (int x = startX; x <= endX; x++)
            set(x, y, color);
    }

    set((int) t.getA().getX(), (int) t.getA().getY(), Color.RED);
    set((int) t.getB().getX(), (int) t.getB().getY(), Color.RED);
    set((int) t.getC().getX(), (int) t.getC().getY(), Color.RED);
}

set sets a pixel to a specific color
Triangle2d's points are already sorted vertically, so A is above B and B is above C (unless they have equal y-values)
Triangle2d#getAB returns a line going from the first point, A to the second point B
Line2d#getX returns the X-value for a given Y-value (and likewise for Line2d#getY)

The idea is that I draw horizontal lines going from the top to the bottom.

This is overall pretty reasonable code. It's very straightforward. Here are some things that could be improved:

Readabality

I find a few things hard to read. For example, putting not only multiple variable declarations on the same line, but assigning each one to a function call result is not fun to read. I would write each on its own line so it's more clear:

Line2d ab = t.getAB();
Line2d bc = t.getBC();
Line2d ac = t.getAC();

Same with the startY and endY variables:

int startY = (int) t.getA().getY();
int endY = (int) t.getC().getY();

Also, even though it's legal to omit the braces on single line if and for statements, it's a good idea to include them to avoid future bugs. I've seen plenty of code where someone added a line to a 1-line if statement without the braces and that line ended up always being executed. It ends up looking like this:

if (foobar)
    callFunction()
    callOtherFunction()

Because there are no braces, callOtherFunction() is always called regardless of the value of foobar.

Naming

Most of your function and variable naming is pretty good, but I would rename the set() function to setPixelColor() to make it more clear what you're setting. The names set() and get() are very overused in programming today, so it helps to be more specific.

Use Points in set() Function

Why separate out the x and y values of the point when calling set()? You should just make it accept a point parameter and do the getX(), getY() and type casting inside the function.

In fact, you do actually pass separate x and y values that are calculated inside the loop, so you might even want to overload it and create 2 versions, 1 that takes a point, and one that takes 2 coordinates, like this:

void setPixelColor(int x, int y, Color pixColor);

and

void setPixelColor(Point2d p, Color pixColor)
{
    setPixelColor((int)p.getX(), (int)p.getY(), pixColor);
}

Renderer Stuff

Most 3D renderers, like OpenGL, DirectX, Metal, etc. make a guarantee to never draw the same pixel twice for triangles that share an edge. I notice that your loops are including the start and end points of each row and column of pixels in the triangle. You might want to change the end condition to just < instead of <= to get this same sort of functionality.

Also, why are you coloring the corners of every triangle red? Was that just for debugging purposes?


Cross-thread label countdown Short and sweet...

I wrote a cross-thread method that displays the countdown [in seconds] of a delay on a label.

I'm fairly confident it's far from optimal, so I'm in need of that glorious optimization advice.

private async Task SnoozeAsync(int seconds)
{
    for (var i = 0; i < seconds; i++)
    {
        Invoke((MethodInvoker)(() => statusLabel.Text = $"Waiting {seconds - i} seconds..."));
        await Task.Delay(1000);
    }
}

await SnoozeAsync(60);

Invoke is a blocking call that returns only after that call has competed.

That means your loop is also including the time it takes to marshal over to the GUI thread and complete.  You probably don't want that.

I would use BeginInvoke instead, which does not wait for the method to complete on the GUI thread.

This is also the difference between SynchronizationContext methods Post and Send.

I would also prevent await from potentially capturing the current SynchronizationContext using ConfigureAwait(false).

To protect against exceptions if the control is disposed of (happens on form close and for other reasons) I'd add an IsDisposed check.

Finally I would allow this Task to be cancelled as a matter of best practices using a CancellationToken.

private async Task SnoozeAsync(int seconds, CancellationToken token)
{
    for (var i = 0; i < seconds; i++)
    {
        if (token.IsCancellationRequested)
            break;
        BeginInvoke((MethodInvoker)(() =>
        {
            if (!statusLabel.IsDisposed)
                statusLabel.Text = $"Waiting {seconds - i} seconds...";
        }));
        await Task.Delay(1000, token).ConfigureAwait(false);
    }
}


Three Cards Trick - Assigning random number to the cards At first I have three cards like this:

 ___    ___    ___
|   |  |   |  |   |
| A |  | A |  | Q |
|___|  |___|  |___|

and then I shuffle them to this:

 ___    ___    ___
|   |  |   |  |   |
| 1 |  | 2 |  | 3 |
|___|  |___|  |___|

So now I want to write a function that generates a random number between 1 and 3 and assign it to be the value of the Q card. For example if I generate number 2, the cards when turned around will have this order:

 ___    ___    ___
|   |  |   |  |   |
| A |  | Q |  | A |
|___|  |___|  |___|

So I came up with a code like this

def CreateCard(card):
    print (' _____')
    print ('|     |')
    print ('|  ',card,'  |',sep='')
    print ('|_____|')

def shuffle():
    Q = random.randint(1,4)
    if Q == 1:
        cards = ['Q','A','A']
    elif Q == 2:
        cards = ['A','Q','A']
    else:
        cards = ['A','A','Q']
    return cards
for card in shuffle():
   CreateCard(card)

I guess what I'm asking is this the optimal way since I don't think I'm allowed to use any other function of random module besides randint or randrange.

Style

The name CreateCard is pretty bad. One would expect it to create a card and return it.

Making things easier to change

At the moment, if you ever want to use 4 or 5 cards instead of 3, you have to update code in multiple places : Q = random.randint(1,HERE), cards = HERE (3 times) and add tests like elif Q == VALUE:. This is just too easy to get wrong.

Even assuming you can only use randint, you have various ways to achieve what you are doing in a less repetitive way.
First idea would be to tell that once you've generated the position for Q with q_pos = random.randint(1,nb_cards), you know you have q_pos -1 'A' cards before it and nb_cards - q_pos after.

This becomes :

def shuffle(nb_cards):
    q_pos = randint(1,nb_cards)
    return ['A'] *  (q_pos -1) + ['Q'] + ['A'] * (nb_cards - q_pos)

A different strategy would be to create a list of 'A's and just put Q somewhere.

def shuffle(nb_cards):
    cards = ['A'] * nb_cards
    q_pos = randint(0,nb_cards-1)
    cards[q_pos] = 'Q'
    return cards


Find Kth largest element in an array I am preparing for interviews where my code would be judged based on optimizations, OOP concepts, code readability as well as my knowledge of JAVA and design patterns.

What comments will you give in code review for the below code?

This is my code for Kth Largest element in an Array.
The logic is to create a K size max heap, and then add the smallest K elements to the heap.
Total array size can be equal to INT_MAX and k can be as small as 2

import java.util.Comparator;
import java.util.PriorityQueue;

public class CustomArray {

    class DecreasingComparator implements Comparator<Long> {

        @Override
        public int compare(Long firstNumber , Long secondNumber) {
            return (int) (secondNumber - firstNumber);
        }
    }

    public long findKthLargestElement(long[] input, int k) {
        if(k < 1 || k > input.length) {
            throw new IllegalArgumentException("Invalid k, should be between 1 and arra length");
        }

        PriorityQueue<Long> heap = new PriorityQueue<>(k, new DecreasingComparator());

        for(int i=0; i<k;i++) {
            heap.add(input[i]);
        }
        for(int i=k; i<input.length;i++) {
            if(heap.peek() > input[i]) {
                heap.poll();
                heap.add(input[i]);
            }
        }

        return heap.poll();
    }
}

Total array size can be equal to INT_MAX and k can be as small as 2

Why not 1?

    @Override
    public int compare(Long firstNumber , Long secondNumber) {
        return (int) (secondNumber - firstNumber);
    }

I found myself several time staring at such a line and being sure, there can be NPE there! But it can and I'd suggest to add explicit checks (throwing is correct, my point is to do it explicitly).

The Comparator is seriously broken:

casting from long to int can overflow
subtraction can overflow, too

Use Long.compare.

PriorityQueue<Long> heap = new PriorityQueue<>(k, new DecreasingComparator());

That makes it simple, but boxing is not free. While a few small values get cached in a cache in java.lang.Long, for others an object must be created. This takes 16 bytes and the heap contains a reference to it (4 or 8 bytes). So you need 20 or 24 bytes per long in addition to the 8 it takes in the input array.

This may make a few megabytes if you want the millionth element. There's also an allocation, but with current smart GCs it's usually no big deal.

There's no need for a decreasing comparator, as you could find the input.length+1-kth smallest element instead.

Or you could use both and select the one giving you a smaller queue.

    for(int i=0; i<k;i++) {
        heap.add(input[i]);
    }
    for(int i=k; i<input.length;i++) {
        if(heap.peek() > input[i]) {
            heap.poll();
            heap.add(input[i]);
        }
    }

This is smart and avoids most of the boxing (for small k). It could use some comments and a lot of tests.


shopify get jquery code more readable, usable Hi so like in my question,
i wrote some part of code for shopify to get recent orders for past 30 day
and i wondering if it is really readable, or maybe i schould change something?

I mean maybe there is a better way to achive same result.

Or maybe i schould use different names? change position for function?

$(document).ready(function() {
    const collection = [];

    //set minimum recent order limit to show
    const minimumOrders = 2;

    //set number of days for recent orders
    const numberOfDays = 30;

   let date = new Date();
   date.setDate(date.getDate() - numberOfDays)
   let startDate = date.toISOString();

    function insertData(item, quality){
      //part of code that will be executed for cert page
      if($( "input[value='"+item+"']" )){
        $( "input[value='"+item+"']" ).parent().append(""+quality+" purchased recently");
      }
      //part of code that will be executed for checkouts
      if($("tr[data-variant-id='"+item+"']")){
        $("tr[data-variant-id='"+item+"'] > .product__description").append(""+quality+" purchased recently");
      }
    }

    function countOrders(){
      $.getJSON('/admin/api/2020-01/orders.json', function (data ) {
        ordersEl = data.orders;

        ordersEl.forEach( function(element, index) {
          if(element.created_at > startDate) {
            element.line_items.forEach( function(item, index) {
              if(collection.hasOwnProperty(item.variant_id)){
                newValue = collection[item.variant_id]['quantity'] + item.quantity;
                collection[item.variant_id]['quantity'] = newValue;
              }
            });
          }
        });

        for(el in collection) {
          if(collection[el]['quantity'] > minimumOrders){
            insertData(el, collection[el]['quantity'])
          }
        }
      });
    }

    $.getJSON('/cart.json', function (data ) {
      data.items.forEach( function(element, index) {
        collection[element.id] = [];
        collection[element.id]['quantity'] = 0;
      });

      countOrders();
    });
  });

every comment, suggestion will be relevant

i do arrow functions and i think there is something more i can do but... the code just stop working...

i would like to change that one :

for(el in collection) {
   if(collection[el]['quantity'] > minimumOrders){
      insertData(el, collection[el]['quantity'])
   }
}

for:

collection.map( el => ((collection[el]['quantity'] > minimumOrders) ? insertData(el, collection[el]['quantity']) : null)

Use const & let instead of vars
Create Docblock for functions
Arrow functions ES6


Deleting folders is a risky business Deleting folders programmatically is extremely dangerous. It is enough for somebody else to change a configuration file or constant variable including the target folder's name to, say C:\, and that's it: the workstation is paralyzed!

The following suggested methods are to prevent such a situation. Their aim is to wrap the low level, possibly recursive file system operation with the necessary validation and clear facade - look twice, delete once.

Delete temporary directory created by current user (non recursive!)

/// <summary>
/// carefully remove directory and its files
/// verify the directory is located under the %temp%
/// and it is not e.g. C:\
/// </summary>
/// <param name="dir"></param>
public static void DeleteTempDirShallow(string dir)
{
    // check if it is an invalid directory path,
    // e.g. a disk drive or just a bad string
    if (! Directory.Exists(dir)) return;

    DirectoryInfo userTempDirInfo = new DirectoryInfo(Path.GetTempPath());
    DirectoryInfo dirInfo = new DirectoryInfo(dir);

    if (dirInfo.FullName.Contains(userTempDirInfo.FullName))
    {
        foreach (FileInfo file in dirInfo.GetFiles())
            file.Delete();

        dirInfo.Delete(); // just clean up the empty dir
    }
}

Delete temporary directory created by current user (recursive!)

public static void DeleteTempDirRecursive(string dir)
{
    // check if it is an invalid directory path,
    // e.g. a disk drive or just a bad string
    if (! Directory.Exists(dir)) return;

    DirectoryInfo userTempDirInfo = new DirectoryInfo(Path.GetTempPath());
    DirectoryInfo dirInfo = new DirectoryInfo(dir);

    if (dirInfo.FullName.Contains(userTempDirInfo.FullName))
    {
        dirInfo.Delete(recursive: true);
    }
}

Do I try to delete any root or system folder?

static List<string> systemDirs = new List<string>
{
    Environment.GetFolderPath(Environment.SpecialFolder.CommonProgramFiles),
    Environment.GetFolderPath(Environment.SpecialFolder.CommonProgramFilesX86),
    Environment.GetFolderPath(Environment.SpecialFolder.CommonPrograms),
    Environment.GetFolderPath(Environment.SpecialFolder.CommonStartMenu),
    Environment.GetFolderPath(Environment.SpecialFolder.CommonStartup),
    Environment.GetFolderPath(Environment.SpecialFolder.Fonts),
    Environment.GetFolderPath(Environment.SpecialFolder.NetworkShortcuts),
    Environment.GetFolderPath(Environment.SpecialFolder.PrinterShortcuts),
    Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles),
    Environment.GetFolderPath(Environment.SpecialFolder.ProgramFilesX86),
    Environment.GetFolderPath(Environment.SpecialFolder.Resources),
    Environment.GetFolderPath(Environment.SpecialFolder.StartMenu),
    Environment.GetFolderPath(Environment.SpecialFolder.Startup),
    Environment.GetFolderPath(Environment.SpecialFolder.System),
    Environment.GetFolderPath(Environment.SpecialFolder.SystemX86),
    Environment.GetFolderPath(Environment.SpecialFolder.Templates),
    Environment.GetFolderPath(Environment.SpecialFolder.UserProfile),
    Environment.GetFolderPath(Environment.SpecialFolder.Windows),
};

/// <summary>
/// check if the argument dir is adisk drive,
/// is a system (sub-)folder
/// and should not be erased
/// </summary>
/// <param name="dir"></param>
/// <returns></returns>
public static bool IsSystemOrRootDir(string dir)
{
    // check if it is an invalid directory path,
    // or just a bad string
    if (! Directory.Exists(dir)) return true;

    DirectoryInfo dirInfo = new DirectoryInfo(dir);

    // is it a root (disk drive)?
    if (dirInfo.Parent == null) return true;

    bool result = false;
    systemDirs.ForEach(sysDir => { result = result ||
                                   dirInfo.FullName.Contains(sysDir); } );
    return result;
}

References

How to check if directory 1 is a subdirectory of dir2 and vice versa
How to delete all files and folders in a directory?

When all you have is a hammer, every problem looks like a nail.

You're trying to do something with code, that's not the code's job. Delete all this code and tackle the problem at the root: make sure your code runs with the appropriate permissions.
It's that simple.
-- My humble opinion

Deleting folders is a risky business

Indeed! If your running code doesn't have permission to wipe out your hard drive, you have some exceptions to deal with, i.e. you should wrap I/O calls in a try...catch block.


Get the common values between two text files I'm just trying to get the common values from two distinct files called file1.txt and file2.txt where I'm using Python's set method to get the results and this works.

However, I'm looking forward to see if there is better way to do this.

#!/grid/common/pkgs/python/v3.6.1/bin/python3
def common_member():
    a_set = dataset1
    b_set = dataset2
    if (a_set & b_set):
        print(a_set & b_set)
    else:
        print("No common elements")

with open('file1.txt', 'r') as a:
        dataset1 = set(a)
with open('file2.txt', 'r') as b:
    dataset2 = set(b)

common_member()

file1.txt

teraform101
azure233
teraform221
teraform223
teraform224

file2.txt

teraform101
azure109
teraform223
teraform226
teraform225
azure233

Result:

{ 'teraform101\n', 'azure233\n', 'teraform223\n' }

Global variables (dataset1 and dataset2) are bad.  I'd rather see you write no function at all than a function that accepts its inputs through global variables:

with open('file1.txt') as file1, open('file2.txt') as file2:
    print((set(file1) & set(file2)) or "No common elements")

If you do write a function, then it should accept its inputs as parameters.  Name the function's parameters however you want, but avoid unnecessary reassignments like a_set = dataset1 and b_set = dataset2.

def print_common_members(a, b):
    """
    Given two sets, print the intersection, or "No common elements".
    """
    print((a & b) or "No common elements")

with open('file1.txt') as file1, open('file2.txt') as file2:
    dataset1 = set(file1)
    dataset2 = set(file2)
    print_common_members(dataset1, dataset2)


numpy performance of norm calculation using variable dimension I am looking for advice to see if the following code performance could be further improved. This is and example using a 4x3 numpy 2d array:

import numpy as np

x = np.arange(12).reshape((4,3))
n, m = x.shape
y = np.zeros((n, m))

for j in range(m):
    x_j = x[:, :j+1]
    y[:,j] = np.linalg.norm(x_j, axis=1)

print x
print y

Which is printing

[[ 0  1  2]
 [ 3  4  5]
 [ 6  7  8]
 [ 9 10 11]]
[[  0.           1.           2.23606798]
 [  3.           5.           7.07106781]
 [  6.           9.21954446  12.20655562]
 [  9.          13.45362405  17.3781472 ]]

As you can see the code is computing the norms of the vectors considering increasing number of columns, so that y[i,j] represent the norm of the vector x[i,:j+1]. I couldn't find if this operation has a name and if it is possible to vectorize further the process and get rid of the for loop.

I only found in this post that using np.sqrt(np.einsum('ij,ij->i', x_j, x_j)) is a bit faster than using np.linalg.norm(x_j, axis=1).

Indeed, there is a better way. Exponentiation aside, we can see that this operation is equivalent to multiplication by an upper triangular matrix of 1. The former is about 100x faster. Here the code (Run it online !):

Source

from time import time as time
import numpy as np
n = 1000
m = 500
size = n * m

triu = np.triu(np.ones((m, m)))
x = np.arange(size).reshape((n, m))
y = np.zeros((n, m))

# Your implementation
tic = time()
for j in range(m):
    x_j = x[:, :j + 1]
    y[:, j] = np.linalg.norm(x_j, axis=1)
tac = time()
print('Operation took {} ms'.format((tac - tic) * 1e3))

# Optimized implementation
tic = time()
y1 = np.sqrt(np.dot(x**2, triu))
tac = time()
print('Operation took {} ms'.format((tac - tic) * 1e3))

# Optimized implementation using cumsum method
tic = time()
y2= np.sqrt(np.cumsum(np.square(x), axis=1))
tac = time()
print('Operation took {} ms'.format((tac - tic) * 1e3))

Output

Operation took 1690.1559829711914 ms
Operation took 18.942832946777344 ms
Operation took 6.124973297119141 ms


JavaScript function to create a duplicate-free copy of an array I've made this function which returns a copy of a given array with no duplicated elements in it.

/**
 * Returns an array with only unique elements. Based
 *  upon an array which might possibly contain duplicates.
 * @param { array } arrayToCheck
 * @return { array }
 * @throws { Error } when given parameter is
 *  detected invalid.
 */

function createArrayWithoutDuplicates(arrayToCheck) {
  'use strict';
  var arrayWithoutDuplicates = [];

  if (!arrayToCheck || !Array.isArray(arrayToCheck)) {
    throw new TypeError('Array expected but ' +
      typeof arrayToCheck + ' found.');
  }

  arrayToCheck.forEach(function(element) {
    if (!arrayWithoutDuplicates.includes(element)) {
      arrayWithoutDuplicates.push(element);
    }
  });

  return arrayWithoutDuplicates;
}

// --------------------------------------------------
// --------- JUST USAGE-DEMO ------------------------
// -- ... not that important, folks! ;) -------------

var arr = [0, 1, 2, 3, 4, 5, 6, 7, 1, 2, 7, 8];
var arr2 = ['a', 'b', 'c', 'd', 'e', 'a', 'b'];

function testCreateArrayWithoutDuplicates(arrayWithDuplicates) {
  let unique = createArrayWithoutDuplicates(arrayWithDuplicates);

  console.log('With duplicates: %o', arrayWithDuplicates);
  console.log('Without duplicates: %o', unique);
  console.log('Array with duplicates: %s', arrayWithDuplicates.length);
  console.log('Array without duplicates: %s', unique.length);
}

testCreateArrayWithoutDuplicates(arr);
console.log('\n');
testCreateArrayWithoutDuplicates(arr2);

As far as I can say: It is doing alright.

But I would like to know:

Is there a better pattern for solving the task of getting a duplicate-free array?

If someone knows a better solution then I would appreciate his/her answer.

Moreover:

- Is my documentation understandable and done in a good way? Or do I have to improve?

- Is my parameter validation done correctly?

Looking forward to read your answers. :)

Sets are basically like magic for pulling unique values from an array; since, by their very definition, any Set will only hold one copy of any specific value, no matter how many times you try to add it.

That, combined with the ability of Sets to be easily turned back into arrays using the spread operator, makes the overall task rather straight-forward:

function noDuplicates(arr){
  return [...new Set(arr)]
}

It actually makes the task so quick and easy that it begs the question of whether a function is even needed; since anytime you need to pull only the unique values of an array you can simply use:

var unique = [...new Set(arr)]

Both the function-based and non-function-based approaches are fully viable options though, so choose whichever you prefer.  Hope this was helpful!


JDBC connection and retrieval of info If you are wondering why I ask, it is because I have numerous different methods all with very similar code (the only difference in the code in each method is what is between the generate resultset comments). I have tried any amount of times to try and shorten this but I am out of ideas. What I have been trying to get is to have the repeating code in the main method (so therefore it won't be repeating), and then to be able to call the resultSet methods from here.

/**
 * This is the Main Method.
 *
 * @param args
 */
public static void main(String[] args) {
// call method resultSet1
resultSet1();
}

/**
 * This Method shows Resultset1.
 */
public static void resultSet1() {
// declare my variables and assign values to them
String url = "jdbc:mysql://lamp.eeecs.qub.ac.uk/mmeaklim01";
Connection connection;

// declare my prepared statement and assign a value to it
PreparedStatement updateSalaries;
String updateSalariesString = "update Staff_Members set salary = ? where idStaff = ?";

// declare two arrays and put values in them
int[] staffSalaries = { 45000, 40000, 35000, 42000, 29000, 29500,
        47000, 34000, 30500, 42000, 44500, 41000, 29500, 29500, 30000,
        34000 };
String[] staffIDs = { "M001", "M002", "M003", "M004", "M005", "M006",
        "M007", "M008", "M009", "M010", "M011", "M012", "M013", "M014",
        "M015", "M016" };

// try catch block to handle exceptions
try {
    // connect to the database
    connection = DriverManager.getConnection(url, "mmeaklim01",
            "xxx");

    // start the update
    updateSalaries = connection.prepareStatement(updateSalariesString);

    // declare my variable and assign a value to it
    int length = staffIDs.length;

    // for loop to take the values from my two arrays and use them to
    // update the Staff_Members table
    for (int loop = 0; loop < length; loop++) {
        updateSalaries.setInt(1, staffSalaries[loop]);
        updateSalaries.setString(2, staffIDs[loop]);
        updateSalaries.executeUpdate();
    }

    // generate resultset1
    ResultSet resultSet1 = updateSalaries
            .executeQuery("SELECT name, position, salary FROM Staff_Members");

    // while loop
    System.out.println("Resultset 1\n");
    System.out.println("Name\t\tPosition\tSalary");
    System.out.println("----------\t----------\t----------");
    while (resultSet1.next()) {
        String name = resultSet1.getString("name");
        String position = resultSet1.getString("position");
        int salary = resultSet1.getInt("salary");

        // print out resultset1
        System.out.println(name + "\t" + position + "\t" + salary
                + "\t");
    } // end of resultset1

    // close the update
    updateSalaries.close();

    // close the connection
    connection.close();

    // add a linebreak
    System.out.println();

    // output a message to the user to indicate that the program has
    // executed correctly
    System.out.println("The Method has executed correctly.");
} catch (SQLException sqlException) {
    // catch block
    System.out.println("Error: " + sqlException.getMessage());
}
}

And all I want the resultSet Methods to have, ideally, would be:

    // generate resultset1
    ResultSet resultSet1 = updateSalaries
            .executeQuery("SELECT name, position, salary FROM Staff_Members");

    // while loop
    System.out.println("Resultset 1\n");
    System.out.println("Name\t\tPosition\tSalary");
    System.out.println("----------\t----------\t----------");
    while (resultSet1.next()) {
        String name = resultSet1.getString("name");
        String position = resultSet1.getString("position");
        int salary = resultSet1.getInt("salary");

        // print out resultset1
        System.out.println(name + "\t" + position + "\t" + salary
                + "\t");
    } // end of resultset1

I'm sure there is probably an easy way around this but I can't seem to figure it out.

Take a look as DBUtils and JDBC Template (the latter is more popular). It will give you some hints how to organize such code. Usually you want select query to be separate method and to return list of objects - PreparedStatement and ResultRow shouldn't be visible outside. While using JDBC directly it is also easy to leak DB connections, in your code sample connection.close() should be inside finally block after catch section.


Flatten Hierarchical Data with LINQ I have a model with a hierarchical structure like this:

Batch (root)
    Groups (List of groups owned by the Batch)
        Groupnames (Nameing of the Group in different Languages)
        Tags (List of Tags owned by the Group)
           Tagnames (Nameing of the Tag in different Languages)
           Logs (List of Logs owned by the Tag)

I want to display all logs with some fields from its parent Tag and parent Group plus the names in a selected Language. I have the following LINQ to select the data which produces the results I want, but queries the database 4 times per log.

        entities.LOGs.Select(log =>
            new
            {
                GroupType = log.TAG.GROUP.GROUPTYPE, //Additional Querry here
                Group = log.TAG.GROUP.GROUPNAMEs.First(gn => gn.LCID == 1031).GROUPNAME1, //Additional Querry here
                Tag = log.TAG.TAGNAMEs.First(tn => tn.LCID == 1031).TAGNAME1, //Additional Querry here
                Time = log.SERVERTIME,
                SourceTime = log.SOURCETIME,
                Information = log.INFORMATION,
                PreviousValue = log.OLDVALUE,
                Value = log.CURRENTVALUE,
                Unit = log.TAG.UNIT,//Additional Querry here
                Operator = log.OPERATOR
            }
        );

Is there a way to avoid these additional queries?

EDIT:
Here are the querries generated by the above code each subqerry is called 14 times

Main querry:

SELECT
    c."GROUPTYPE" AS COL1,
    b."GROUPID" AS COL2,
    a."TAGID" AS COL3,
    a."SERVERTIME" AS COL4,
    a."SOURCETIME" AS COL5,
    a."INFORMATION" AS COL6,
    a."OLDVALUE" AS COL7,
    a."CURRENTVALUE" AS COL8,
    b."UNIT" AS COL9,
    a."OPERATOR" AS COL10
FROM "LOGS" a
LEFT JOIN "TAGS" b
       ON (a."TAGID" = b."ID")
LEFT JOIN "GROUPS" c
       ON (b."GROUPID" = c."ID")

Subquerry group:

SELECT
    "BATCHID" AS COL1,
    "BATCHID" AS COL2,
    "GROUPTYPE" AS COL3,
    "IDENTIFIER" AS COL4,
    "SIGNATURE" AS COL5,
    "TAGCOUNT" AS COL6
FROM "GROUPS"
WHERE "ID" = CAST(@p0 AS INTEGER)

Subquerry tag:

SELECT
    "GROUPID" AS COL1,
    "GROUPID" AS COL2,
    "IDENTIFIER" AS COL3,
    "LOGCOUNT" AS COL4,
    "SIGNATURE" AS COL5,
    "UNIT" AS COL6
FROM "TAGS"
WHERE "ID" = CAST(@p0 AS INTEGER)

Subquerry groupnames:

SELECT
    b."GROUPID" AS COL1,
    b."LCID" AS COL2,
    b."GROUPID" AS COL3,
    b."GROUPNAME" AS COL4,
    b."SIGNATURE" AS COL5
FROM "GROUPS" a
LEFT JOIN "GROUPNAMES" b
       ON (a."ID" = b."GROUPID")
WHERE a."ID" = CAST(@p0 AS INTEGER)

Subquerry tagnames:

SELECT
    b."GROUPID" AS COL1,
    b."LCID" AS COL2,
    b."GROUPID" AS COL3,
    b."GROUPNAME" AS COL4,
    b."SIGNATURE" AS COL5
FROM "GROUPS" a
LEFT JOIN "GROUPNAMES" b
       ON (a."ID" = b."GROUPID")
WHERE a."ID" = CAST(@p0 AS INTEGER)

Unfortunately, you haven't included enough code for me to give a proper review. Essentially what your looking for is called "eager loading." MSDN and stack overflow have some examples.

When you're building the query for entities.LOGs do something like this:

entities.LOGs.
    Include (log => log.TAG).
    Include (log => log.TAG.GROUP).
    Include (log => log.TAG.GROUP.GROUPNAMEs).
    Include (log => log.TAG.TAGNAMEs).
    Select ( /* your select here */);


Substitute Excel-Workbook reference for another I have an Excel-Workbook, where we have an sheet where we consolidate data. Basically we link to other Excel-Workbooks. I wrote the following Macro, in order to update the path and filename. We cannot just update the whole link, since other parts need to reference the old path (and filename).

Sub neueDatei()

Dim Pfad As String, DateiName As String, DateiName_alt As String, h As String, formel As String
Dim tblRow As Long, test As Long
Dim wb As Workbook
Dim IVsh As Worksheet

Application.ScreenUpdating = False

Set wb = ActiveWorkbook

Set IVsh = wb.Sheets("Input Vektor")

With IVsh
    Pfad = .Range("C3").Value 'set the path
    DateiName = .Range("C4").Value 'set the filename
    tblRow = .Cells(.Rows.Count, 1).End(xlUp).Row 'find last row
End With

If Mid(Pfad, Len(Pfad) - 1, 1) <> "\" Then
    Pfad = Pfad & "\" 'append \ if necessary
End If

If Mid(Pfad, 1, 1) <> "'" Then
    Pfad = "'" & Pfad 'excel needs a quote at the start of the path.
End If

DateiName_alt = DateiName

'add brackets to the filename
If Mid(DateiName, 1, 1) <> "[" Then
    DateiName = "[" & DateiName
    DateiName = DateiName & "]"
End If

'substitute the filenames
For i = 19 To tblRow
    If IVsh.Cells(i, 7).Value <> "" Then
        Exit For
    End If
    h = IVsh.Cells(i, 2).Formula
    If InStr(1, h, "[") > 0 Then
        IVsh.Cells(i, 2).Formula = "=" & Pfad & DateiName & Mid(h, InStr(1, h, "]") + 1)
    Else
        IVsh.Cells(i, 2).Formula = "=" & Pfad & DateiName_alt & Mid(h, InStr(1, h, "xlsx") + 4)
    End If
Next i

Application.ScreenUpdating = True

End Sub

The problem is, that the code runs relatively slow (a few minutes for only 2-3 thousand lines). Any suggestions, how I can speed this up?

Try adding Application.Calculation = xlManual where you turn updating off, and then turn it back to xlAutomatic at the end.

In addition, turn events off (and then on again): Application.EnableEvents = False and Application.EnableEvents = True.

I suspect that everytime you change a file name in a cell, the workbook tries to access the link and update the cell.

Small point - doesn't affect speed:

If Mid(DateiName, 1, 1) <> "[" Then
    DateiName = "[" & DateiName
    DateiName = DateiName & "]"
End If

could be

If Mid(DateiName, 1, 1) <> "[" Then
    DateiName = "[" & DateiName & "]"
End If

Instead of Set wb = ActiveWorkbook, you could use ThisWorkbook - it is a little more assertive if that is what you need. I don't have the context on how this function is used, so it may be called when another workbook is active.


Better way to do multiple try/except in Python I have this code:

    lengths = []
    lengths.append(len(self.download_links))
    lengths.append(len(self.case_names))
    lengths.append(len(self.case_dates))
    lengths.append(len(self.docket_numbers))
    lengths.append(len(self.neutral_citations))
    lengths.append(len(self.precedential_statuses))

Unfortunately, any of those object properties can be None, so I need to check each with either an if not None block or a try/except block. Checking each individually will blow up the size and conciseness of the code.

I assume there must be a better way for this kind of pattern, right?

Firstly, consider whether these shouldn't be None, but instead should be empty lists. Most of the time None is better replaced by something else like an empty list.

If that's not an option, you can use a list:

for item in [self.download_links, self.case_name...]:
    if item is not None:
         length.append( len(item) )

or a function

def add_length(obj):
   if obj is not None:
            lengths.append(len(obj))

add_length(self.download_links)
add_length(self.case_name)
...


Is this reimplemented quicksort correct? #include<iostream>
using namespace std;

int partition(int arr[],int first,int last,int pivot)
{
    int tmparr[100];
    int left=first,right=last;
    for(int i=first;i<=last;i++)
    {
        if(arr[i]<pivot)
            tmparr[left++]=arr[i];
        else if(arr[i]>pivot)
            tmparr[right--]=arr[i];
    }
    //for the case when pivot is duplicated
    for(int i=left;i<=right;i++)
        tmparr[i]=pivot;
    for(int i=first;i<=last;i++)
        arr[i]=tmparr[i];
    return left;
}
void quicksort(int arr[],int first,int last)
{
    if(first<last)
    {
    int pivotindex = partition(arr,first,last,arr[first]);
    quicksort(arr,first,pivotindex-1);
    quicksort(arr,pivotindex+1,last);
    }

}
int main()
{
    int arr[6]={6,6,9,5,3,7};
    int n=6;
    quicksort(arr,0,n-1);
    for(int i=0;i<n;i++)
    cout<<arr[i];
}

How do I avoid the copying to tmparr?
How does the current method of copying to tmparr and back affect my time efficiency as opposed to an in-place swap?
Am I missing anything in this code?  It looks much simpler than implementations I could find online.

I know you haven't mentioned anything about this, but I think it should still be addressed.

You should not be passing around C arrays in C++.  The entire array is not passed; it merely decays to a pointer to the array.

Instead, use a storage container such as an std::vector.  These will not decay to pointers, and using such containers is more idiomatic C++.  If you have C++, you can use an std::array instead.

Declare and push back the elements:

std::vector<int> values;

values.push_back(6);
values.push_back(6);
// ...

If you have C++11, initialize an std::array instead:

std::array<int, 6> values = { 6, 6, 9, 5, 3, 7 };

Pass either one to a function:

void func(std::vector<int>& values) {}

void func(std::array<int, 6>& values) {}

You will also no longer need n; storage containers know their sizes.  You can access this value with values.size(), which is of the return type std::size_type.  This is also safer because the returned value will be updated as the container changes size.

If you don't have C++11, display the container with (const) iterators:

std::vector<int>::const_iterator iter;
// OR
std::array<int, 6>::const_iterator iter;

for (iter = values.cbegin(); iter != values.cend(); iter++)
{
    std::cout << *iter;
}

If you do have C++11, use a range-based for-loop:

for (auto& iter : values)
{
    std::cout << iter;
}

You can use auto here to give you the correct container type automatically.


Assigning a numerical value to each alphabet in corresponding order and then summing any inputted word I made a function that outputs the sum of the value of each letter in a word. For example if I input hello my function would output 52.

def words_to_marks(s):
    import string
    values = {}             # dictionary to store value of each alphabet
    values_of_words = []    # to store final value of words
    for index, letters in enumerate(string.ascii_lowercase, 1):     # creates the dictionary
        values[letters] = index
    final = []              # stores value of each number before adding
    for letter in s:
        letter_lower = letter.lower
        final.append(values[letter])    # appends the value of each letter into the list
    values_of_words.append(sum(final))  # sums all the values in the list
    submit = str(values_of_words)       # converts into string so I can remove the brackets around the list
    return submit[1:-1]

test = words_to_marks("friends")
print(test)

I would really appreciate it if someone could point me to the right direction for how I should condense or improve my code.

Honestly, the initial words_to_marks function is full of "anti-patterns/bad smells".

When reviewing in steps:

instead of iterating over string.ascii_lowercase and generating/filling a dictionary of letter indices on each function call with:

for index, letters in enumerate(string.ascii_lowercase, 1):     # creates the dictionary
    values[letters] = index

create a predefined top-level dict lowercase_indices_map at once:

import string

lowercase_indices_map = {c:i for i, c in enumerate(string.ascii_lowercase, 1)}

converting each letter of the word with:

for letter in s:
   letter_lower = letter.lower

is completely redundant as the whole word can be lowercased at once s = s.lower()
and the "magic" like:

values_of_words.append(sum(final))  # sums all the values in the list
submit = str(values_of_words)       # converts into string so I can remove the brackets around the list
return submit[1:-1]

is just an unnecessary thing that you should avoid doing in future. The sum of letter indices is simply calculated with a single sum(...) call.

The whole idea fits in few lines of code:

import string

lowercase_indices_map = {c:i for i, c in enumerate(string.ascii_lowercase, 1)}

def sum_letters_indices(word):
    word = word.lower()
    return sum(lowercase_indices_map[char] for char in word)

Sample usage:

print(sum_letters_indices("hello"))    # 52
print(sum_letters_indices("friends"))  # 75


Find all of the values in a list that are repeated the most I have a list of objects and I would like to see which ones are the most common. It is easy to find the or a most common object using python's Counter, but I haven't found a simple way to find all of the most common objects. The code below is working but I would guess there's a better way to do this.

from collections import Counter

input_lst = [1, 2, 3, 3, 4, 4, 0, 1]

counts = Counter(input_lst)
ranked_counts = counts.most_common().copy()
first_common_obj = ranked_counts.pop(0)
equally_common_objs = [first_common_obj[0]]
while ranked_counts:
    next_obj = ranked_counts.pop(0)
    if first_common_obj[1] == next_obj[1]:
        equally_common_objs.append(next_obj[0])
    else:
        break

print(equally_common_objs)

It would be easier, and have better performance, to iterate over Counter.most_common() rather than using .pop(0) lots.

From here you just want to use next until the value is not the same. Which is effectively what you're doing with .pop.

from collections import Counter

input_lst = [1, 2, 3, 3, 4, 4, 0, 1]

counts = Counter(input_lst)
items = iter(counts.most_common())
value, amount = next(items)
values = [value]
while True:
    value, a = next(items)
    if amount != a:
        break
    values.append(value)

print(values)

This is effectively grouping consecutive values together, and so you can instead use itertools.groupby rather than roll your own.

from collections import Counter
from itertools import groupby

input_lst = [1, 2, 3, 3, 4, 4, 0, 1]

_, values = next(groupby(Counter(input_lst).most_common(), lambda i: i[1]))
values = [value for value, _ in values]
print(values)


Speed Up Excel Macro - Autofilter Copying I am using this code which works fine, but runs painfully slow.

The code filters an Excel Table and then extracts only certain columns and pastes them into another sheet (in a different order).

Could it be sped up perhaps with a multi-dimensional array?

With Application
.ScreenUpdating = False
.Calculation = xlCalculationManual
.DisplayAlerts = False
.EnableEvents = False
End With

Set lo_b1 = x_bf1.ListObjects(1)
s_date = CLng(ThisWorkbook.Names("in_fre_m").RefersToRange(1, 1))
s_des = ThisWorkbook.Names("dr_no").RefersToRange(1, 1)
s_code = ThisWorkbook.Names("dr_co").RefersToRange(1, 1)
lastrow_d = lo_dr.Range.Columns(1).Cells.Find("*", SearchOrder:=xlByRows, SearchDirection:=xlPrevious).Row + 1
Set pasterange1 = x_drill.Range("C" & lastrow_d)

    With lo_b1.Range
    .AutoFilter Field:=13, Criteria1:=s_code
    .AutoFilter Field:=1, Criteria1:="<=" & s_date
    End With

lastrow_s = lo_b1.Range.Columns(1).Cells.Find("*", SearchOrder:=xlByRows, SearchDirection:=xlPrevious).Row

If lastrow_s > 7 Then
    Set copyrange1 = x_bf1.Range("D8:D" & lastrow_s) 'Date
    Set copyrange2 = copyrange1.Offset(0, 1)  'Description
    Set copyrange3 = copyrange1.Offset(0, 16)  'Calculation
    Set copyrange5 = copyrange1.Offset(0, 5)  'Classification
    Set copyrange6 = copyrange1.Offset(0, 6)  'Notes
    Set copyrange7 = copyrange1.Offset(0, 11) '§
    Set copyrange8 = copyrange1.Offset(0, 12) 'Code
    Set copyrange9 = copyrange1.Offset(0, 20) 'Statutory
    Set copyrange10 = copyrange1.Offset(0, 14) 'Ref

    copyrange10.SpecialCells(xlCellTypeVisible).Copy 'Ref
    pasterange1.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    copyrange1.SpecialCells(xlCellTypeVisible).Copy 'Date
    pasterange1.Offset(0, 1).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    copyrange5.SpecialCells(xlCellTypeVisible).Copy 'Account Name
    pasterange1.Offset(0, 2).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    copyrange2.SpecialCells(xlCellTypeVisible).Copy 'Notes
    pasterange1.Offset(0, 3).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    copyrange8.SpecialCells(xlCellTypeVisible).Copy 'Code
    pasterange1.Offset(0, 4).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    copyrange7.SpecialCells(xlCellTypeVisible).Copy '§
    pasterange1.Offset(0, 5).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    copyrange3.SpecialCells(xlCellTypeVisible).Copy 'Calculation
    pasterange1.Offset(0, 6).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    copyrange9.SpecialCells(xlCellTypeVisible).Copy 'Statutory
    pasterange1.Offset(0, 7).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    copyrange6.SpecialCells(xlCellTypeVisible).Copy 'Notes
    pasterange1.Offset(0, 8).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

    Set copyrange1 = Nothing
    Set copyrange2 = Nothing
    Set copyrange3 = Nothing
    Set copyrange4 = Nothing
    Set copyrange5 = Nothing
    Set copyrange6 = Nothing
    Set copyrange7 = Nothing
    Set copyrange8 = Nothing
    Set copyrange9 = Nothing
    Set copyrange10 = Nothing
    End If

Turn on Option Explicit from the menu via Tools>Options>Editor tab>Require Variable Declaration. This mandates you Dim lastRow as long before using it anywhere. Doing this will save you needless frustration later down the line when you transpose a variable name raom instead of roam and time's wasted till you find it.

If you have two Ranges that are same size and you want to copy the values over you can do Foo.Value2 = Bar.Value2 to achieve this without any copying. If you have a Range that is the same size as an array you get the same with Foo.Value2 = inMemoryArray`.

Use descriptive variable names. x_bf1 doesn't have any meaning, at least to me. If future-you comes back to this code and doesn't know what it means you'll be wishing past-you had used a descriptive name. An example of this being useful is the comments 'Date in two locations. Renaming copyrange1 to dateArea will cause these comments to become redundant and removable as your code is self documenting already describing what it is doing, save comments for why.

Set copyrange1 = x_bf1.Range("D8:D" & lastrow_s) 'Date
...
copyrange1.SpecialCells(xlCellTypeVisible).Copy 'Date
pasterange1.Offset(0, 1).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone

becomes

destinationArea.Offset(0, 1).Value2 = dateArea.Value2

Tied to naming is the use of _ underscore. If you end up using interfaces be aware that this is how interface implementation is done. Double click on your first worksheet then at the dropdowns at the top select Worksheet from the left dropdown, and SelectionChange from the right dropdown. You get Private Sub Worksheet_SelectionChange(ByVal Target As Range)  which is an example of the underscore used this way.

You turn off ScreetUpdating and the rest, but it's never turned back on in your provided code. Make sure that is restored.

With Application
    .ScreenUpdating = False
    .Calculation = xlCalculationManual
    .DisplayAlerts = False
    .EnableEvents = False
End With

You're using a named range from a workbook. Every usage of them includes RefersToRange(1, 1). If your names are single cell Ranges this is redundant.

Refactored Code.

Public Sub Refactor()
    Dim x_bf1 As Worksheet
    Dim lo_b1 As ListObject
    Set lo_b1 = x_bf1.ListObjects(1)
    Dim s_date As Long
    s_date = CLng(ThisWorkbook.Names("in_fre_m").RefersToRange(1, 1))
    Dim s_des As Range
    s_des = ThisWorkbook.Names("dr_no").RefersToRange(1, 1)
    Dim s_code As Range
    s_code = ThisWorkbook.Names("dr_co").RefersToRange(1, 1)
    Dim lastrow_d As Long
    Dim lo_dr As ListObject
    lastrow_d = lo_dr.Range.Columns(1).Cells.Find("*", SearchOrder:=xlByRows, SearchDirection:=xlPrevious).Row + 1

    With lo_b1.Range
        .AutoFilter Field:=13, Criteria1:=s_code
        .AutoFilter Field:=1, Criteria1:="<=" & s_date
    End With

    Dim lastrow_s As Long
    lastrow_s = lo_b1.Range.Columns(1).Cells.Find("*", SearchOrder:=xlByRows, SearchDirection:=xlPrevious).Row

    If lastrow_s > 7 Then
        Dim dateArea As Range
        Set dateArea = x_bf1.Range("D8:D" & lastrow_s)
        Dim description As Range
        Set description = dateArea.Offset(0, 1)
        Dim calculationArea As Range
        Set calculationArea = dateArea.Offset(0, 16)
        Dim classification As Range
        Set classification = dateArea.Offset(0, 5)
        Dim notes As Range
        Set notes = dateArea.Offset(0, 6)
        Dim parragraph As Range
        Set parragraph = dateArea.Offset(0, 11)
        Dim code As Range
        Set code = dateArea.Offset(0, 12)
        Dim statutory As Range
        Set statutory = dateArea.Offset(0, 20)
        Dim reference As Range
        Set reference = dateArea.Offset(0, 14)

        Dim x_drill As Worksheet
        Dim destinationArea As Range
        Set destinationArea = x_drill.Range("C" & lastrow_d).Resize(dateArea.Rows.Count, 9)

        Dim singlePopulationHelper() As String
        singlePopulationHelper = LoadHelperArraywWithValues(reference, _
                                                            dateArea, _
                                                            classification, _
                                                            description, _
                                                            code, _
                                                            parragraph, _
                                                            calculationArea, _
                                                            statutory, _
                                                            notes)
        destinationArea.Value2 = singlePopulationHelper
    End If
End Sub

Private Function LoadHelperArraywWithValues(ParamArray values()) As String()
    Dim rowCount As Long
    rowCount = values(0).SpecialCells(xlCellTypeVisible).Cells.Count
    Dim columnCount As Long
    columnCount = UBound(values)
    Dim helperArray() As String
    ReDim helperArray(rowCount, columnCount)

    Dim populationColumn As Long
    For populationColumn = 0 To columnCount
        Dim workingColumn As Range
        Set workingColumn = values(populationColumn)
        Dim populationRow As Long
        populationRow = 0
        Dim subArea As Range
        For Each subArea In workingColumn.SpecialCells(xlCellTypeVisible).Areas
            Dim cell As Range
            For Each cell In subArea
                helperArray(populationRow, populationColumn) = cell.Value2
                populationRow = populationRow + 1
            Next
        Next
    Next
    LoadHelperArraywWithValues = helperArray
End Function

Alternate copying to temporary worksheet.

Private Function CopyToTempSheetBeforeLoading(ParamArray values()) As Variant
    Dim populatedColumns As Long

    Dim tempSheet As Worksheet
    Set tempSheet = ThisWorkbook.Worksheets.Add

    For populatedColumns = 0 To UBound(values)
        values(populatedColumns).SpecialCells(xlCellTypeVisible).Copy
        tempSheet.Range("B2").Offset(ColumnOffset:=populatedColumns).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone
    Next
    CopyToTempSheetBeforeLoading = tempSheet.Range("B2").CurrentRegion
    tempSheet.Delete
End Function
```


String permutation using iteration and a filter I'm looking to improve my skills as a developer so I recently challenged myself to design a string permutation generator without recursion. To add to the algorithm I also decided that the string should have a filter to it. An example of a filter would be "lld" where "l" is any character a-z and "d" is any digit 0-9. The filter "lld" should produce all strings aa0-zz9. Would love feedback on how to improve this algo. Thanks.

Implementation in C++

#include <iostream>

#define CHAR_SET_LEN 26
#define INT_SET_LEN 10
#define FILTER_LEN 3

int main() {
    std::cout << "Hello, World!" << std::endl;

    char char_set[CHAR_SET_LEN] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'};
    char int_set[INT_SET_LEN] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

    bool doLoop = true;

    int filterStack[100];

    int filterLength = FILTER_LEN;

    char filter[FILTER_LEN+1] = "lld";

    for (int i=0; i < filterLength; i++) {
        filterStack[i] = 0;
    }

    while (doLoop) {
        char string[FILTER_LEN];

        for (int a = 0; a < filterLength; a++) {
            if (filter[a] == 'l') {
                string[a] = char_set[filterStack[a]];
            }
            else {
                string[a] = int_set[filterStack[a]];
            }

        }
//
        std::cout << string << std::endl;
//
        for (int z = (filterLength-1); z > -1; z--) {
            filterStack[z] = filterStack[z] + 1;

            int finalFilter = INT_SET_LEN;
            if (filter[z] == 'l') {
                finalFilter = CHAR_SET_LEN;
            }

            if (filterStack[z] < finalFilter) {
                break;
            }
            else {
                if (z == 0 && (filterStack[z] == finalFilter)) {
                    doLoop = false;
                    break;
                }

                filterStack[z] = 0;
            }
        }
    }

    return 0;
}

Are you sure this is C++?

...because this is almost 100% C. Let's go over some things:

#define is discouraged in modern C++, as is reliance on the C preprocessor in general. Just define your constants with const and (depending on the situation) constexpr.
Don't define raw arrays. Use std::array for fixed size arrays, std::vector for dynamic arrays, or any other standard container class.
Don't use char arrays if you mean strings. C++ offers std::string for general string handling and std::stringstream for string building.
Use standard algorithms. For example, your first for-loop could be replaced with a call to std::fill.

That said, there are

Other Issues

Don't use std::endl. '\n' does the same thing, but doesn't include the unnecessary flush std::endl comes with.
You don't need char_set and int_set. You can generate all lowercase letter by adding their offset in the alphabet to 'a' (i.e. 'a' + 1 == 'b') thanks to the (almost) ubiquitous ASCII encoding. The same is true for digits.
int is not the right type for index variables. Use somethings more appropriate, such as std::size_t, instead.
You should export some of your code to proper functions. Just putting everything into main makes your code hard to read and prone to bugs.
As Cris Luengo pointed out in the comments already, string is not null-terminated, and thus you're causing undefined behavior when writing it to std::cout.
filter is one character longer than it needs to be. Since you never do something with it that would require it to be null terminated (and don't even null terminate the string, just keep an unused character at the end), it suffices to make its length FILTER_LEN.


PHP validation class for username, email, and password I'm relatively new to PHP OOP, currently I'm practising how to use it, however I am not really certain I'm building my classes correctly, I think perhaps I'm missing something out but I'm not entirely sure what, any advice would be appreciated

Class

class Validation
{
    private
    $password,
    $repeatPassword,
    $username,
    $email;

    private static
    $minPassword = 7,
    $confirmPassword,
    $minUsername = 3,
    $maxUsername = 14,
    $validEmail;

    public static function validateEmail($email)
    {
        return (filter_var($email, FILTER_VALIDATE_EMAIL) != self::$validEmail);
    }
    public static function validatePassword($password)
    {
        return(strlen($password) >= self::$minPassword);
    }

    public static function validatecheckbox($agree)
    {
        return(!empty($terms) >= self::$checked);
    }
    public static function validateRepeatPassword($repeatPassword,$password)
    {
            return $repeatPassword === $password;

    }
    public static function validateUsername($username)
    {
            return strlen($username) >= self::$minUsername
            && (strlen($username)) <= self::$maxUsername
            && (filter_var($username ,  FILTER_VALIDATE_REGEXP,["options"=> [ "regexp" => "/^[\p{L}0-9\s]+$/u"]]) == TRUE);
    }
}

Usage

    $errors = array();

 $fields = array(
  'username' => array(
    'validator' => 'validateUsername',
    'message'   => 'What is your username?'
  ),
  'email' => array(
    'validator' => 'validateEmail',
    'message'   => 'Please enter a valid email',
  ),
  'password' => array(
    'validator' => 'validatePassword',
    'message'   => 'Password must be a minimum of seven characters'
  )
);

if(!Validation::validateRepeatPassword($password, $repassword))
{
    $errors[] = ["name" => "repassword", "error" => "Passwords must match"];
}
foreach($post as $key => $value)
    {
  if(isset($fields[$key]))
    {
            if(!Validation::{$fields[$key]['validator']}($value))
            {
                $errors[] = ['name' => $key, 'error' => $fields[$key]['message']];
            }
    }
}

Well, your class is good and does what it is supposed to do, except in validalidatecheckbox() (you are using variables that doesn't exist).

Also I would do this changes:

Remove this code (it's unused):

private $password,
    $repeatPassword,
    $username,
    $email;
Change this to constants, so you can use it elsewhere:

private static $minPassword = 7,
    $confirmPassword,
    $minUsername = 3,
    $maxUsername = 14,
    $validEmail;

So your class should look like this:

class Validation
{
    const MIN_PASSWORD_LENGTH = 7;
    const MIN_USERNAME_LENGTH = 3;
    const MAX_USERNAME_LENGTH = 14;

    // rest of your code

Also, I strongly recommend you to check PSR-1 and PSR-2 to make your code look standard-like.


BoundLess Grid, grid with unknown and infinite dimension The reason this was made was because of a combination of things. Firstly, it was made for me to get more acquainted with some ECMA2016 syntax. Secondly, this is a useful utility class when working with grids of undefined width and height, including negative points (I couldn't easily find some already made). Besides that, I also tried abstracting the class in a way, returning a function, I think would result in a convenient shorthand way of using the class.

Any pointers would be appreciated. If there's something I could use / use better with the new ECMA2016 features, that'd be awesome to see!

I'm betting using get and set as function names in the class isn't such a great convention to follow, either.

For some reason, only parts of the code are pasted and reformatted weird online. Otherwise, it looks correct in Atom.

GitHub
JsFiddle demonstration

/*
    A grid which can have any bounds and be checked against.
  create: let grid = new BoundlessGrid();
  set: grid( 0,0, "start point" )
  get: let myVal = grid(0,0);
  delete y:  delete grid()[0][0];
  delete x: delete grid()[0];
  iterate: grid().forEach( (x,y,grid)=> {... } );
*/
function forEach(callback) {
    for (let x in this) {
        for (let y in this[x]) {
            callback(x, y, this);
        }
    }
};

class BoundlessGrid {
  constructor() {
    this.grid = { forEach: forEach };
    Object.defineProperty(this.grid, "forEach", { enumerable: "false" });
    // return a function that abstracts the object into
    // something that has friendlier shorthand syntax
    return (x, y, val) => {return this.handler(x, y, val)};
  }

  set(x, y, val) {
    if (!this.grid.hasOwnProperty(x)) {
      let self = this;
      // automatically removes the x grid axis when
      // the last y point is deleted from that x
      this.grid[x] = new Proxy({}, {
        deleteProperty(yAxis, y) {
          delete yAxis[y];
          if (Object.keys(yAxis).length == 0) { delete self.grid[x];}
        }
      });
    }
    this.grid[x][y] = val;
  }

  get(x, y) { if (this.grid.hasOwnProperty(x)) { return this.grid[x][y]; } }

  // Single function that delegates to others
  // by determining inputs
  handler(x, y, val) {
    if (x === undefined) { return this.grid; }
    if (val !== undefined ) {this.set(x, y, val); }
        else { return this.get(x, y); }
  }
}

module.exports = BoundlessGrid;

An example of some alternatives that you could use, there may be things there that you prefer.

/*
  A grid which can have any bounds and be checked against.
  create: let grid = new BoundlessGrid();
  set: grid( 0,0, "start point" )
  get: let myVal = grid(0,0);
  delete y:  delete grid()[0][0];
  delete x: delete grid()[0];
  iterate: grid().forEach( (x,y,grid)=> {... } );
*/
function* generateValues(object) {
  for (let value in object) {
    yield value;
  }
}

class BoundlessGrid {
  constructor() {
    this.grid = Object.create(null, {
      forEach: {
        value: callback => {
          for (let x of generateValues(this)) {
            for (let y of generateValues(Reflect.get(this, x))) {
              callback(x, y, this);
            }
          }
        },
        enumerable: false,
        writable: false
      }
    });
    this.deleteProperty = Reflect.deleteProperty.bind(null, this.grid);
    // return a function that abstracts the object into
    // something that has friendlier shorthand syntax
    return (x, y, val) => this.handler(x, y, val);
  }

  has(x) {
    return Reflect.has(this.grid, x);
  }

  set(x, y, val) {
    if (!this.has(x)) {
      const deleteProperty = this.deleteProperty;
      // automatically removes the x grid axis when
      // the last y point is deleted from that x
      Reflect.set(this.grid, x, Reflect.construct(Proxy, [Object.create(null), {
        deleteProperty(yAxis, y) {
          Reflect.deleteProperty(yAxis, y);
          if (Object.keys(yAxis).length === 0) {
            deleteProperty(x);
          }
        }
      }]));
    }
    Reflect.set(Reflect.get(this.grid, x), y, val);
  }

  get(x, y) {
    if (this.has(x)) {
      return Reflect.get(Reflect.get(this.grid, x), y);
    }
  }

  // Single function that delegates to others
  // by determining inputs
  handler(x, y, val) {
    if (x === undefined) {
      return this.grid;
    }
    return val === undefined ? this.get(x, y) : this.set(x, y, val);
  }
}

const grid = Reflect.construct(BoundlessGrid, []);
grid(5, 4, "test1");
grid(5, 3, "test2");
grid().forEach((x, y, g) => console.log(`loop: val of ${x} and ${y}`, g[x][y]));
delete grid()[5][4];
console.log("First deleted key at 5x 4y:", grid(5, 4));
console.log("Value at 5x 3y:", grid(5, 3));
console.log("array of 5x", grid()[5]);
delete grid()[5][3];
console.log("second deleted key exists?", grid(5, 3) !== undefined);
console.log("array 5x defined?", grid()[5]);


Getting the current timestamp and compute the timestamp difference in minutes I have this code to get the current Timestamp and compute the last timestamp's difference with the current timestamp in minutes. I'm wondering if this can be optimized further for production.

public static Timestamp getTimestamp() {
    java.util.Date date= new java.util.Date();
    long time = date.getTime();
    java.sql.Timestamp ts = new java.sql.Timestamp(time);
    return ts;
}

public static long getLastTimestampElapse(java.sql.Timestamp oldTime){
  long milliseconds1 = oldTime.getTime();
  long milliseconds2 = getTimestamp().getTime();
  long diff = milliseconds2 - milliseconds1;
  long diffMinutes = diff / (60 * 1000);
  return diffMinutes;
}

Note that none of these suggestions will have any significant impact on the performance of your application overall. Don't micro-optimize performance until you have known, tested bottlenecks.

The getTimestamp() method is noise. If all you care about is the current timestamp in milliseconds, use System.currentTimeMillis().

You can use a constant to store the number of milliseconds in a minute, potentially saving the multiplication. Even if the compiler optimizes the math away, it's easier to read.

A java.sql.Timestamp is a kind of java.util.Date, and the getTime() method is defined there. Your method should accept a java.util.Date to support more clients at no cost.

Your method is poorly named. Something like getMinutesSince() would be more readable. Likewise, there are better variable names than what you've selected.

Use final to indicate that variables won't be reassigned. That reduces the cognitive load on the reader.

You don't really need as many variables as you have. You might even be able to get away with none and still have a reasonably clear method.

If you were to use all my suggestions, your code might look more like:

private static final long MILLISECONDS_PER_MINUTE = 60 * 1000;

public static long getMinutesSince(final java.util.Date startTime) {
  final long millisecondsSinceStart =
      System.currentTimeMillis() - startTime.getTime();
  return millisecondsSinceStart / MILLISECONDS_PER_MINUTE;
}


Grouping utility methods for View classes I have the following class:

public final class ViewUtils {

    public static void setSize(View view, int width, int height) {
        ViewGroup.LayoutParams layoutParams = (ViewGroup.LayoutParams) view.getLayoutParams();
        layoutParams.width = width;
        layoutParams.height = height;
        view.requestLayout();
    }

    public static void setWidth(View view, int width) {
        setSize(view, width, view.getHeight());
    }

    public static void setHeight(View view, int height) {
        setSize(view, view.getWidth(), height);
    }

    public static boolean isRightEdgeOfScrollViewHasBeenReached(HorizontalScrollView scrollView) {
        View childView = scrollView.getChildAt(scrollView.getChildCount() - 1);
        return childView.getRight() == (scrollView.getWidth() + scrollView.getScrollX());
    }

    // Other methods will be added later

}

But I don't like the name isRightEdgeOfScrollViewHasBeenReached. I think it would be better if the method name didn't contain word ScrollView but is placed in a class which would be specialized only on the ScrollView.

Should I place this method in an inner class ViewUtils.ScrollView or in a separate class ScrollViewUtils?

I think the first option would be better, because creating a separate class for a few methods (currently only one method) is too "expensive".

And help me choose more appropriate name for the method isRightEdgeOfScrollViewHasBeenReached.

Summarizing the question

The choice is between two options:

To have all methods inside a single class:

public class ViewUtils {

    public static void methodForAllViews1() { }

    public static void methodForAllViews2() { }

    public static void methodForAllViews3() { }

    public static class TextView {

        public static void methodForTextViews1() { }

        public static void methodForTextViews2() { }
    }

    // etc.

}

To separate all methods and to place all util classes in the same package:

- utils
    - view
        * ViewUtils.java
        * TextViewUtils.java
        * ScrollViewUtils.java

Refactoring and renaming isRightEdgeOfScrollViewHasBeenReached

How about like this:

public static boolean isAtRightEdge(ViewGroup viewGroup) {
    View childView = viewGroup.getChildAt(viewGroup.getChildCount() - 1);
    return childView.getRight() == viewGroup.getWidth() + viewGroup.getScrollX();
}

I haven't tested this, but it compiles.
As such, I don't really see a reason to limit the method to take a HorizontalScrollView as parameter.
Do let me know if you see some shortcomings in your tests.

Also, I removed the unnecessary parentheses from the return statement.

Organizing helper methods and utility classes

Here are some general tips:

Only move a method from your real class to a utility class if you will actually use it in at least two unrelated classes, right now. Don't add utility classes that "seem like a good idea", or if you're sure "you'll definitely use it sometime really soon". Don't speculate, wait until you have an actual use case.
Don't nest utility classes. Nested classes were not intended for this purpose, it would be misusing the language. This page in the docs explains when it is appropriate to use nested classes.
If you start to have too many methods in the utility class, or when it makes sense, split to multiple classes. (As you proposed in option 2.)
Avoid overloaded helper methods with the same number of parameters. (This is just a tip for the future, there was no such example in your post.)
Prevent instantiation of utility classes by adding a private parameterless constructor, for example:

private ViewUtils() {
    // utility class, should not instantiate
}


function to check if anchor tags contain href attributes with 301 I made this function to test if the content has 301 redirections.. Please can you help me optimize it better because it's too slow
function contains_http_301_link($content, $current_site){
$pattern = '~<a(.*?)href="([^"]+)"(.*?)>~';
$links = array();
// Check if there is a url in the content
if(preg_match_all($pattern, $content, $urls)) {
    foreach($urls[0] as $url){
        // get external links

        if ( !strpos( $url, $current_site )) {                                   // Is an external link
            $vowels = array('"' ,"<a", " ", "href", "=", ">");
            $links[] = str_replace($vowels, "", $url);
        }
    }
}
foreach ($links as $link) {
    $_headers = @get_headers($link,1);
    if($_headers && strpos( $_headers[0], '301')) {
        return true;
    }
}
return false;
}

I have never attempted such a task, but I will assume that you entertained the cURL techniques (including CURLOPT_NO_BODY and CURLINFO_HTTP_CODE) as suggest at 1 and 2.
As for parsing an html document, using regex may outperform a legitimate parser, but you might enjoy more accurate scaping if you use a DOM parser.
Since your focus is on performance, I'll [cringe] suggest regex to parse your html.
Your pattern ~<a(.*?)href="([^"]+)"(.*?)>~ uses lazy quantifiers and this can slow things down a little.  Also, your <a(.*?)href will allow the matching of <a data-href and <abbr href -- which is not what you intend.
Your intention is to capture ONLY the url inside of the href attribute, so don't bother matching more than you need.  You can probably get away with a wordboundary after a and a space before href, then reset the fullstring match with \K to only retain the href's value.  If this isn't strong enough validation, then you could add a lookahead to confirm that the opening tag is completed properly.

~<a\b.*? href="\K[^"]+~ or
~<a\b.*? href="\K[^"]+(?=".*?>)~

Either way, your code can skip the sanitizing step.  Thank goodness because $vowels contains data that are not all vowels.
strpos() can return false or 0 as the offset.  This means that you must not rely on the type juggled condition using merely !.  You must explicitly check for false or if you are on a php version that enjoys str_contains() -- that'll work.
I personally condemn the use of the "stfu operator" (@), but because I am not familiar with this task, I'll avoid calling it an abomination.
function contains_http_301_link(string $content, string $current_site): bool
{
    preg_match_all('~<a\b.*? href="\K[^"]+~', $content, $matches);
    foreach ($matches[0] as $url) {
        if (strpos($url, $current_site) !== false) {
            continue;
        }
        $headers = @get_headers($url, 1);
        if($headers && strpos($headers[0], '301') !== false) {
            return true;
        }
    }
    return false;
}

I could possibly include the $current_site filtration into the regex but it may do more harm than good.

If you were going to entertain (and benchmark) a DOM parser script, then perhaps use this:
function contains_http_301_link(string $content, string $current_site): bool
{
    $dom = new DOMDocument;
    $dom->loadHTML($content);
    $xpath = new DOMXPath($dom);
    foreach ($xpath->query("//a[not(contains(@href, '$current_site'))]/@href") as $href) {
        $headers = @get_headers($href->nodeValue, 1);
        if($headers && strpos($headers[0], '301') !== false) {
            return true;
        }
    }
    return false;
}


Project Euler problem 19 - How many Sundays fell on the first Here's the problem:

  How many Sundays fell on the first of the month during the twentieth
  century (1 Jan 1901 to 31 Dec 2000)?

And here's my code:

#include <stdio.h>

int main(void){
    int months[12] = { 31 , 28 , 31 , 30 , 31 , 30 , 31 , 31 , 30 , 31 , 30 , 31 };
    int year=1901;
    int dayoftheweek=2;  //put a 2 because the 1 jan 1901 is tuesday. 0-sunday 6-Saturday
    int sunday=0;
    int firstsundays=0;

    while(year<=2000){
        if(year%4==0)    //checking if it's a leap year
            months[1]=29;

        for (int i = 0; i < 12; ++i) {
            for (int d = 1; d <= months[i]; d++){
                if(dayoftheweek==7)  //reset the week
                    dayoftheweek=0;

                if(dayoftheweek==sunday && d==1)
                    firstsundays++;

                dayoftheweek++;
            }
        }

        months[1]=28;
        year++;
    }

    printf("There are %d Sundays that fell on the first of the month\n", firstsundays);

    return 0;
}

What could I improve?

The loops could be clearer:

The outermost loop is actually spread across three lines: int year=1901, while(year<=2000), and year++.  It should be written as a for loop.
i should be renamed to month or m.
The exceptional case for leap years is awkward: you sometimes clobber the February entry at the top of the loop, then reset it at the end.  I'd consider such mutations to be a hack that makes the code harder to understand.  It also makes the code harder to maintain, since you have hard-coded 29 and 28 in scattered places.

As for the algorithm itself… you should be able to do better than advancing a day at a time through an entire century.  Why not advance a month at a time?

#include <stdio.h>

static const int *month_lengths(int year) {
    static const int NON_LEAP_YEAR[] = {
        31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
    };
    static const int LEAP_YEAR[] = {
        31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
    };
    return (year % 4 == 0 && (year % 100 != 0 || year % 400 == 0)) ?
        LEAP_YEAR : NON_LEAP_YEAR;
}

int main(void) {
    int first_sundays = 0;
    int day_of_the_week = 2;    // 0 = Sun, 6 = Sat.  1 Jan 1901 was a Tuesday.
    for (int year = 1901; year <= 2000; ++year) {
        const int *lengths = month_lengths(year);
        for (int month = 0; month < 12; month++) {
            if (day_of_the_week == 0) {
                first_sundays++;
            }
            day_of_the_week = (day_of_the_week + lengths[month]) % 7;
        }
    }
    printf("There are %d Sundays that fell on the first of the month\n",
           first_sundays);
}


Braces and Brackets Autocomplete I need to complete a sequence of braces and brackets like [ ], ( ), { }, or reject the input. I can add braces/brackets only at the beginning or at the end.

Example:

input: '))[[([{([])}' output: '(())[[([{([])}])]]'
input: '[)[((({}[]'   output: 'rejected'

My implementation is based on using a stack (self-implemented to improve from standard Stack class, based on Vector), but certainly it can be done much better.

public class BraceCompliter {
    private static final char L_PAREN    = '(';
    private static final char R_PAREN    = ')';
    private static final char L_BRACE    = '{';
    private static final char R_BRACE    = '}';
    private static final char L_BRACKET  = '[';
    private static final char R_BRACKET  = ']';
    private static StringBuilder currString;

public static void main(String[] args) {
    String test1 = "))[[([{([])}";
    String test2 = "[)[((({}[]";
    System.out.println(process(test1));
}

public static String process(String s) {
    currString = new StringBuilder(s);
    ArrayStack stack = new ArrayStack(s.length());
    int count = 0;
    for (int i = 0; i < s.length(); i++) {

        if      (s.charAt(i) == L_PAREN){
            stack.push(L_PAREN);
            count++;
        }
        else if (s.charAt(i) == L_BRACE)  {
            stack.push(L_BRACE);
            count++;
        }
        else if (s.charAt(i) == L_BRACKET) {
            stack.push(L_BRACKET);
            count ++;
        }
        else if (s.charAt(i) == R_PAREN) {
            if (stack.isEmpty())
                i = completeStart();
            else if (stack.pop() != L_PAREN)
                    return "rejected";
                else count--;
        }

        else if (s.charAt(i) == R_BRACE) {
            if (stack.isEmpty())
                i = completeStart();
            else if (stack.pop() != L_BRACE)
                return "rejected";
                else count--;
        }

        else if (s.charAt(i) == R_BRACKET) {
            if (stack.isEmpty())
                i = completeStart();
            else if (stack.pop() != L_BRACKET)
                return "rejected";
                else count--;
        }
    }
    if (!stack.isEmpty()) {
        completeEnd(stack, count);
    }
    return currString.toString();
}

private static void completeEnd(ArrayStack stack, int count) {
    for (int i = 0; i < count; i++) {
        Character ch = stack.pop();
        switch (ch) {
            case L_PAREN:
                currString.append(R_PAREN);
                break;
            case L_BRACE:
                currString.append(R_BRACE);
                break;
            case L_BRACKET:
                currString.append(R_BRACKET);
                break;
        }
    }
}

private static int completeStart() {
    int count = -1;
    for (int i = 0; i < currString.length(); i++) {
        char c = currString.charAt(i);
        if (c == L_BRACE || c == L_BRACKET || c == L_PAREN) {
            break;
        }
        switch (c) {
            case R_BRACE:
                currString.insert(0, L_BRACE);
                i++;
                count++;
                break;
            case R_BRACKET:
                currString.insert(0, L_BRACKET);
                i++;
                count++;
                break;
            case R_PAREN:
                currString.insert(0, L_PAREN);
                i++;
                count++;
                break;
        }
    }
    return count;
}

class ArrayStack {

    private int top;
    private char[] storage;

    ArrayStack(int capacity) {
        storage = new char[capacity];
        top = -1;
    }

    void push(char value) {
        top++;
        storage[top] = value;
    }

    char pop() {
        return storage[top--];
    }

    boolean isEmpty() {
        return (top == -1);
    }
}
}

All feedback welcome.

Extract common code in each conditional branch

In the block:

    switch (c) {
        case R_BRACE:
            currString.insert(0, L_BRACE);
            i++;
            count++;
            break;
        case R_BRACKET:
            currString.insert(0, L_BRACKET);
            i++;
            count++;
            break;
        case R_PAREN:
            currString.insert(0, L_PAREN);
            i++;
            count++;
            break;
    }

You always execute:

        i++;
        count++;

So you can simplify and write just:

    switch (c) {
        case R_BRACE:
            currString.insert(0, L_BRACE);
            break;
        case R_BRACKET:
            currString.insert(0, L_BRACKET);
            break;
        case R_PAREN:
            currString.insert(0, L_PAREN);
            break;
    }
    i++;
    count++;


Computing the profit or loss from a list of trades I am coming from C# and I am learning how to use Python more seriously.

So I decided to try a little side project to help me see how much I earn (or lose) from my trades on the stock market.

I would like some advice about how I structured my project, in C# I am used to put each class in its own file, but I read that in Python we work with module, so I put several classes in the trades.py module. But does it make sense to put the trade importer in the same module as the calculator?

Here is a screenshot of my project hierarchy so far:

and a description of the files:

trades.py

import calculator.trades as t

from enum import Enum
import numpy as np

class TradeType(Enum):
    Buy = 1
    Sell = -1

class Trade:
    def __init__(self, id, trade_date, type, quantity, price, currency_id, company_id, account_id):
        self.id = id
        self.type = type
        self.account_id = account_id
        self.company_id = company_id
        self.currency_id = currency_id
        self.trade_date = trade_date
        self.quantity = quantity
        self.price = price

class TradePnlCalculator:
    def compute_pnl(self, trades):
        signed_quantities = np.array([-1 * t.type.value * t.quantity for t in trades])
        prices = np.array([t.price for t in trades])
        return np.dot(signed_quantities, prices)

class TradeImporter:
    def load_trades(self):
        # TODO Read the trades from a file
        return []

test_tradePnlCalculator.py

from unittest import TestCase

from calculator.trades import Trade, TradeType, TradePnlCalculator
import datetime

class TestTradePnlCalculator(TestCase):

    def setUp(self):
        now = datetime.datetime.now()
        self.trade1 = Trade(1, trade_date=now, type=TradeType.Buy, quantity=100,
                            price=34.5, currency_id=1, company_id=1, account_id=1)
        self.trade2 = Trade(2, trade_date=now, type=TradeType.Sell, quantity=100,
                            price=37.4, currency_id=1, company_id=1, account_id=1)

        self.calculator = TradePnlCalculator()

    def test_compute_pnl_returns_zero_when_there_is_no_trades(self):
        self.assertEquals(0, self.calculator.compute_pnl([]))

    def test_compute_pnl_returns_negative_flow_for_single_buy_trade(self):
        self.assertEquals(-3450, self.calculator.compute_pnl([self.trade1]))

    def test_compute_pnl_returns_positive_flow_for_single_sell_trade(self):
        self.assertEquals(3740, self.calculator.compute_pnl([self.trade2]))

    def test_compute_pnl_returns_correct_value_for_cancelling_trades(self):
        self.assertEquals(3740-3450, self.calculator.compute_pnl([self.trade1, self.trade2]))

run.py

importer = t.TradeImporter()
trades = importer.load_trades()

calculator = t.TradePnlCalculator()
pnl = calculator.compute_pnl(trades)

print('The profit for the trades is {} euros'.format(pnl))

This is too sketchy to be able to do a proper review. But just based on what you've written in the post:

None of this code updates the attributes of a Trade. So you could use a collections.namedtuple which avoids some duplication:

Trade = namedtuple('Trade', 'id trade_date type quantity price currency_id '
                   'company_id account_id')

In some languages (especially Java) everything has to be a member of a class. But this isn't the case in Python: you can just write a function if you want, without having to make a class whose only role is to act as a container for your function.

So here there's no need for the TradePnlCalculator class — you could just define a function compute_pnl. And there's no need for the TradeImporter class — you could just define a function load_trades.
In compute_pnl, you're not gaining anything from using NumPy — it costs more to build the NumPy arrays than you gain in faster operations. It would be simpler and faster to write:

-sum(t.type.value * t.quantity * t.price for t in trades)

The need for the negation of the result of this computation suggests that you have the values for Buy and Sell the wrong way round.
The code in compute_pnl doesn't consult the currency_id so it would go wrong if given a list of trades in a mixture of currencies.


Pizza-making combining two builder patterns (Bloch's and Go4) I think I just made a very simple way to implement both builder patterns:

Bloch builder (for solving telescoping problem). introduction@Medium, Telescoping problem@SO
Go4 builder pattern (The most difficult one among those 23)

Characters in Go4 builder:

Director: Pizza.Maker
ConcreteBuilder: Pizza.Recipe

I'm also thinking about how to make Builder an abstract interface so one can have more type of recipes (Hot/Sweet/Sour...).

The advantages:

The client doesn't have to know the ingredient types used by Pizza, just tune the parameters of a pizza using Bloch builder. (In my example it just coincident that they're both int.)
The client doesn't have to be an expert in making a pizza, i.e. he/she doesn't have know the actual order of steps of making a pizza, because these steps are handled by the director using the Go4 builder pattern.
But if the client doesn't have an idea for certain steps of making a pizza, he/she can customize it; just @override it.

public class Pizza
{
    final private int baseSize;
    final private int a;
    final private int b;
    final private int c;

    private Pizza(Recipe builder) {
        baseSize = builder.baseSize;
        a = builder.a;
        b = builder.b;
        c = builder.c;
    }

    public static Pizza Maker(Recipe builder) {
        builder.prepare2();
        builder.prepare1();
        builder.prepare3();
        return new Pizza(builder);
    }

    public static class Recipe {
        private int baseSize;
        private int a;
        private int b;
        private int c;
        public Recipe() {
            baseSize = 0;
            a = 0;
            b = 0;
            c = 0;
        }
        public Recipe baseSize(int s) {
            this.baseSize = s;
            return this;
        }
        public Recipe changeA(int a) {
            this.a = a;
            return this;
        }
        public Recipe changeB(int b) {
            this.b = b;
            return this;
        }
        public Recipe changeC(int c) {
            this.c = c;
            return this;
        }
        public void prepare1() {
            System.out.println("Basic preparing 1");
        }
        public void prepare2() {
            System.out.println("Basic preparing 2");
        }
        public void prepare3() {
            System.out.println("Basic preparing 3");
        }
    }
}

public class Main
{
    public static void main() {
        Pizza.Recipe myRecipe = new Pizza.Recipe() {
            @Override
            public void prepare3() {
                System.out.println("Advanced preparing 3");
            }
        };
        myRecipe.baseSize(10);
        myRecipe.changeA(20);
        myRecipe.changeB(15);
        myRecipe.changeC(25);
        Pizza myPizza = Pizza.Maker(myRecipe);
        System.out.println();
        // One line-er
        Pizza onelinePizza = Pizza.Maker(new Pizza.Recipe()
                                                  .baseSize(10)
                                                  .changeA(20)
                                                  .changeC(25));
    }
}

And the printed result:

Basic preparing 2
Basic preparing 1
Advanced preparing 3

Basic preparing 2
Basic preparing 1
Basic preparing 3

The original purpose of making Builder abstract, described in Go4 design patterns book, is that the client can decide the representation of the product later on, e.g. In my example then I would be able to create many types of pizza using new recipes (maybe real pizza, pixelized pizza, and LEGO pizza, etc).

But I don't know how to make Pizza.Recipe abstract while still make everything simple. If this can be done then the client can design he/she's own preparing order, just like he/she is the cook!

I'm not familiar with the given pattern, but there are still a few things that should be pointed out.

The method Maker in Pizza should not start with an uppercase letter.

The fields in the Recipe constructor are useless, as an int is automatically given the value of 0 if no other value is assigned to it.

public Recipe() {
    baseSize = 0;
    a = 0;
    b = 0;
    c = 0;
}

should be

public Recipe() {}

The class Recipe has the following methods: changeA(), changeB() and changeC(). However, it also has prepare1(), prepare2() and prepare3().

Though it is a wild guess from my part, I assume that prepare1() is related to the field a, prepare2() is related to b and prepare3() is related to c. Why are the methods not named prepareA(), prepareB() and prepareC() instead?

Which brings me to my next point:

Even if you are experimenting with a pattern, you should still take the time to use relevant method and field names.

Sure, you're learning on the moment, but tomorrow (or in the future), will you be able to make sense of the field names you used? Will you be able to remember what you learned if you base yourself on an unclear example?

Some good examples could have been:

changeToppings() and prepareToppings()
changeSauce() and prepareSauce()
changeCrust() and prepareCrust()

etc.

Obviously, I have no knowledge at all regarding cooking, but I am sure that a recipe with the fields baseSize, toppings, sauce and crust is much more descriptive than a recipe with baseSize, a, b and c.

The following is a tip, so take it or leave it.

When I experiment with patterns, I fully immerse myself in the example I create.

In your case, you decided to use pizzas. Commit to it - use fields like toppings, crust, sauce, cheese, etc. and methods like cook, cut, etc. If you're going to call your class Pizza, it has to be feel like a pizza -- and I've never seen a pizza made of a, b and c. If you really have no idea, then you could use toppingA, toppingB and toppingC, as long as it makes sense.

I don't know if this applies to everybody, but to me, I find it easier to learn/remember something new when I use full words rather than non-descriptive characters

e.g.:

The likelihood of remembering

  Oh yeah, that's right, the Pizza class had a topping, a crust and a sauce!

vs

  Oh yeah, that's right, the Pizza class had an a, a b and a c!"


Retrieving list of flight numbers from flight path The below is \$O(n^3)\$. I'm sure there is a way to improve this..

//vector<string> flight_path; given as parameter
//vector<int> flight_number; need to fill
//vector<segment> flight_segments; known, contains segments (dep, arr, flightnum)

//Purpose of the function is to take in a flight_path and give back a list of
// flight numbers for each segment
//
// Input: New York, London, Kolkata
// Output: 101 201 301, 102 939

vector<string> get_flight_numbers (vector<string> flight_path) {

    vector<int> flight_number;

    for (int i = 0; i < flight_path.size() - 1; i++) {

        string dep = flight_path.at(i);
        string arr = flight_path.at(i+1);

        for (int j = 0; j < flight_segments.size(); j ++) {
            if (flight_segments.at(j).departure == dep && flight_segments.at(j).arrival == arr) {
                for (int k = 0; k < flight_segments.at(j).segment_numbers.size(); k++) {
                    flight_numbers.push_back(atoi(flight_segments.at(j).segment_numbers.at(k).c_str()));
                }
            }
        }

    }
}

If I understand this, you have a directed graph that consists of a series of flight segments. You are given a series of flight segments, say, A -> B -> C, that makes up a flight path.

In addition, you have a group of flight numbers. The flight numbers correspond to paths. Flight number 1 will be tagged as [A,B,1], for example. Time is not an issue, all flights are assumed to be available at all times.

Can you, in less than \$O(n^3)\$ time, figure out what flight numbers can take you from one segment to the next?

Well, on approach might be to go through the array exactly once in order to separate out all the flights that depart from your first departure gate, your second departure gate, and so forth. That's order n.

Then go through each of those lists (once) and remove all the flights that don't have the right destination gate. That's order n again.

At that point, you're pretty much done.

So think your big mistake here is going through your big loop (the flights) INSIDE your little loop (your flight path). You'd be much better off going through the small loop inside the big loop and breaking if you find a match. This loop only nests once, also, instead of twice. If you really really wanted to, you could check each departure as you find it to see if it matches an arrival.

In psuedo-code:

for (i in each flight) do
    for (j in each pathpoint except the last one) do
        if flight[i][departure][i] == pathpoint[j] then
            if flight[j][arrival] == pathpoint[j+1]
                put flight in the good flights vector
            end if
        end if
    end for
end for

That's \$O(n*m)\$, where \$m\$ is the length of the path and n is the number of flights. This should be, in any reasonable world, \$O(n)\$.