Writing a test to validate that the repo is formatted I have a short test to validate that all committed files in the repo are formatted. I'm new to go so not sure what the best wat to do it is.

The error handling, in particular, is rather gruesome as it takes up roughly half of the lines.

package pack

import (
    "os"
    "os/exec"
    "path/filepath"
    "testing"
)

func TestFormatting(t *testing.T) {
    var commandArgs []string
    commandArgs = append(commandArgs, "-l")
    root := "../.."
    err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
        if filepath.Ext(path) != ".go" {
            return nil
        }
        absPath, e := filepath.Abs(path)
        if e != nil {
            return e
        }
        commandArgs = append(commandArgs, absPath)
        return nil
    })
    if err != nil {
        t.Logf(err.Error())
        t.Fail()
    }

    out, err := exec.Command("gofmt", commandArgs...).Output()
    if err != nil {
        t.Logf(err.Error())
        t.Fail()
    }
    output := string(out)
    if output != "" {
        t.Logf("Testing Formatting")
        t.Logf(output)
        t.Fail()
    }
}

How can it be improved?

a short test to validate that all committed files in the repo are
  formatted.

Here are some notes I made while reading your code.

For a real-world code review, code should be correct, maintainable, reasonably efficient, and, most importantly, readable.

Writing code is a process of stepwise refinement.

Define the problem with an outline of a solution.

// checkGoFmt checks that Go source files have been formatted with go fmt or gofmt.
// checkGoFmt returns a newline separated list of files
// whose formatting differs from gofmt's and any error.
func checkGoFmt(root string) (string, error) {
    // Command gofmt
    // https://golang.org/cmd/gofmt/
    // $ gofmt -help
    // usage: gofmt [flags] [path ...]
    //  -l  list files whose formatting differs from gofmt's
    // $
}

Read the documentation for the gofmt command.

Write some code to construct and run the gofmt command. The goFiles function is a stub.

func goFiles(root string) ([]string, error) {
    var files []string
    return files, nil
}

// checkGoFmt checks that Go source files have been formatted with go fmt or gofmt.
// checkGoFmt returns a newline separated list of files
// whose formatting differs from gofmt's and any error.
func checkGoFmt(root string) (string, error) {
    // Command gofmt
    // https://golang.org/cmd/gofmt/
    // $ gofmt -help
    // usage: gofmt [flags] [path ...]
    //  -l  list files whose formatting differs from gofmt's
    // $

    files, err := goFiles(root)
    if err != nil {
        return "", err
    }
    if len(files) == 0 {
        return "", nil
    }

    args := make([]string, 0, 1+len(files))
    args = append(args, "-l")
    args = append(args, files...)
    out, err := exec.Command("gofmt", args...).Output()
    return string(out), err
}

Pay attention to program structure. The specific implementation of goFiles is hidden from checkGoFmt. If the end result is not as expected, start by looking at the list of files returned from goFiles. Consider writing a test for the goFiles function. checkGoFmt can be called from a testing package function or a program main function.

The documentation for gofmt states: "Without an explicit path, it processes the standard input." To ensure that TestGoFormatting does not wait on stdin, check that at least one Go file is found. Also, if there is nothing to do then exit.

If possible, make reasonable estimates of slice capacity to minimize allocations.

Read the documentation for filepath.Walk and filepath.WalkFunc.

  func Walk

  func Walk(root string, walkFn WalkFunc) error

  Walk walks the file tree rooted at root, calling walkFn for each file
  or directory in the tree, including root. All errors that arise
  visiting files and directories are filtered by walkFn.

  type WalkFunc

  WalkFunc is the type of the function called for each file or directory
  visited by Walk. The path argument contains the argument to Walk as a
  prefix; that is, if Walk is called with "dir", which is a directory
  containing the file "a", the walk function will be called with
  argument "dir/a". The info argument is the os.FileInfo for the named
  path.

  If there was a problem walking to the file or directory named by path,
  the incoming error will describe the problem and the function can
  decide how to handle that error (and Walk will not descend into that
  directory). In the case of an error, the info argument will be nil. If
  an error is returned, processing stops. The sole exception is when the
  function returns the special value SkipDir. If the function returns
  SkipDir when invoked on a directory, Walk skips the directory's
  contents entirely. If the function returns SkipDir when invoked on a
  non-directory file, Walk skips the remaining files in the containing
  directory.

  type WalkFunc func(path string, info os.FileInfo, err error) error

Write code for the goFiles stub.

func goFiles(root string) ([]string, error) {
    var files []string

    err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
        if err != nil {
            return err
        }
        if info == nil {
            return nil
        }
        if !info.Mode().IsRegular() {
            return nil
        }
        if filepath.Ext(path) != ".go" {
            return nil
        }
        absPath, e := filepath.Abs(path)
        if e != nil {
            return e
        }
        files = append(files, absPath)
        return nil
    })
    if err != nil {
        return nil, err
    }

    return files, nil
}

If an error is passed to the WalkFunc don't ignore it.

Don't panic if the info argument is nil.

The documentation for gofmt states: "Given a file, it operates on that file; given a directory, it operates on all .go files in that directory, recursively."

Only process regular files. If the directory is processed too, the file will be duplicated. For example,

├── fmt.dir.go
│   ├── fmt.file.go

fmt.dir.go and fmt.file.go will be appended to the files list and fmt.file.go will be formatted twice, once as a member of directory fmt.dir.go and once as file fmt.file.go.

Read the documentation for the Go testing package.

Write a Go testing package function.

// TestGoFormatting checks that Go source files have been formatted.
func TestGoFormatting(t *testing.T) {
    // TODO: Set root to a variable value?
    root := "../.."
    out, err := checkGoFmt(root)
    if err != nil {
        t.Error(err)
    }
    if len(out) > 0 {
        t.Error(out)
    }
}

The Go testing package prints the test function name, TestGoFormatting, on failure, There is no need for redundant logging of a test title. TestGoFormatting is more meaningful than TestFormatting. The testing package T.Error method merges the T.Log and T.Fail methods. Eliminate redundant argument error.Error() methods.

Obtaining a value for root requires more thought.

We can also write a program main function.

func main() {
    exit := 0
    // TODO: Set root to a variable value?
    root := "../.."
    out, err := checkGoFmt(root)
    if err != nil {
        exit = 2
        fmt.Fprintln(os.Stderr, err)
    }
    if len(out) > 0 {
        exit = 2
        fmt.Fprintln(os.Stderr, out)
    }
    os.Exit(exit)
}

And so on.

In Go, don't ignore errors.

Here is the complete code for a first draft of TestGoFormatting, a testing function:

package pack

import (
    "os"
    "os/exec"
    "path/filepath"
    "testing"
)

func goFiles(root string) ([]string, error) {
    var files []string

    err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
        if err != nil {
            return err
        }
        if info == nil {
            return nil
        }
        if !info.Mode().IsRegular() {
            return nil
        }
        if filepath.Ext(path) != ".go" {
            return nil
        }
        absPath, e := filepath.Abs(path)
        if e != nil {
            return e
        }
        files = append(files, absPath)
        return nil
    })
    if err != nil {
        return nil, err
    }

    return files, nil
}

// checkGoFmt checks that Go source files have been formatted with go fmt or gofmt.
// checkGoFmt returns a newline separated list of files
// whose formatting differs from gofmt's and any error.
func checkGoFmt(root string) (string, error) {
    // Command gofmt
    // https://golang.org/cmd/gofmt/
    // $ gofmt -help
    // usage: gofmt [flags] [path ...]
    //  -l  list files whose formatting differs from gofmt's
    // $

    files, err := goFiles(root)
    if err != nil {
        return "", err
    }
    if len(files) == 0 {
        return "", nil
    }

    args := make([]string, 0, 1+len(files))
    args = append(args, "-l")
    args = append(args, files...)
    out, err := exec.Command("gofmt", args...).Output()
    return string(out), err
}

// TestGoFormatting checks that Go source files have been formatted.
func TestGoFormatting(t *testing.T) {
    // TODO: Set root to a variable value?
    root := "../.."
    out, err := checkGoFmt(root)
    if err != nil {
        t.Error(err)
    }
    if len(out) > 0 {
        t.Error(out)
    }
}


Can this Google Hurdles code be made to run any faster? Anyone can play Google Hurdles today. Here is my score: 1.1 second. Is there are way of improving this score and running faster than a second?

http://www.google.com/doodles/hurdles-2012

import java.awt.AWTException;
import java.awt.Robot;
import java.awt.event.KeyEvent;

public class RunOlim {

private volatile static boolean run=true;

public static void main(String[] args) {
    Robot robot = null;

    Thread th = new Thread(new Runnable(){

        @Override
        public void run() {
            try {
                Thread.sleep(10000);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }finally {
                run=false;
            }

        }

    });
    th.start();

    try {
        robot = new Robot();
    } catch (AWTException e) {

        e.printStackTrace();
    }
    while(run){
        robot.keyPress(KeyEvent.VK_RIGHT);
        robot.keyRelease(KeyEvent.VK_RIGHT);

        robot.keyPress(KeyEvent.VK_LEFT);
        robot.keyRelease(KeyEvent.VK_LEFT);
    }
}

}

There are some general notes about the code, not really performance improvements (however, I have not tested their performance). I guess the real bottleneck could be in the browser/OS/hardware which can't handle more key events.

Anyway, these do not need any other thread:

final long endTime = System.nanoTime()
    + TimeUnit.NANOSECONDS.convert(10, TimeUnit.SECONDS);
while (endTime > System.nanoTime()) {
    robot.keyPress(KeyEvent.VK_RIGHT);
    robot.keyRelease(KeyEvent.VK_RIGHT);

    robot.keyPress(KeyEvent.VK_LEFT);
    robot.keyRelease(KeyEvent.VK_LEFT);
}

final long endTime = System.nanoTime()
    + TimeUnit.NANOSECONDS.convert(10, TimeUnit.SECONDS);
while (endTime > System.nanoTime()) {
    for (int i = 0; i < 100; i++) {
        robot.keyPress(KeyEvent.VK_RIGHT);
        robot.keyRelease(KeyEvent.VK_RIGHT);

        robot.keyPress(KeyEvent.VK_LEFT);
        robot.keyRelease(KeyEvent.VK_LEFT);
    }
}

Maybe these were your first tries too.

Here is a 3rd one with a ScheduledExecutorService:

public static void main(final String[] args) throws AWTException {

    final ScheduledExecutorService scheduledExecutorService =
        Executors.newScheduledThreadPool(1);
    final Runnable stopCommand = new Runnable() {
        @Override
        public void run() {
            run = false;
        }
    };
    scheduledExecutorService.schedule(stopCommand, 10, TimeUnit.SECONDS);

    final Robot robot = new Robot();

    while (run) {
        robot.keyPress(KeyEvent.VK_RIGHT);
        robot.keyRelease(KeyEvent.VK_RIGHT);

        robot.keyPress(KeyEvent.VK_LEFT);
        robot.keyRelease(KeyEvent.VK_LEFT);
    }
    scheduledExecutorService.shutdown();
}

A final note from Clean Code, page 25:

  Classes and objects should have noun or noun phrase names like Customer, WikiPage,
  Account, and AddressParser. [...] A class name should not be a verb.

I'd name it HurdleRunner, for example.


Find the total number of emails in a folder (with all of the subfolders) The Folder struct definition below is given:

type Folder struct {
    ID int
    emailCount int
    childFolderIDs []int
}

Description of the fields are:

Id                 : folder id
emailCount         : number of emails in the folder
childFolderIDS     : if the folder has subfolders, this list contains folder IDS of the subfolders.

I am trying to write a function:

Function is taking two parameters:

First parameter is a list of all folders in the email application

  var folders = []Folder{
        Folder{1, 30, []int{2, 4}},
        Folder{2, 10, []int{3}},
        Folder{4, 60, []int{}},
        Folder{3, 20, []int{}},
  }

Second parameter is folder id of a folder

Function will return a total number of emails with ID=Folder Id and all of its children recursively.

For example:

If the second parameter is:

four, then the function should return 60 (there is no subfolder)
two, then the function should return 30

My code is below:

      package main

      import "fmt"

      type Folder struct {
          ID int
          emailCount int
          childFolderIDs []int
      }

      func InsertIntoMap(data_arr []Folder) map[int]Folder {
          var retval = make(map[int]Folder)
          var child_folders []int
          for _, elem := range data_arr {
            child_folders = elem.childFolderIDs
            var folder Folder
            folder.ID = elem.ID
            folder.emailCount = elem.emailCount
            folder.childFolderIDs = child_folders
            retval[elem.ID] = folder
          }

          return retval
      }

    func GetTotalEmailCount(p_map map[int]Folder, folder_id int) int {
          total := 0
          var child_folders []int
          child_folders = p_map[folder_id].childFolderIDs
          total = total + p_map[folder_id].emailCount

          if len(child_folders) == 0{
            return total
          }else{
            for e := range child_folders {
              return total + GetTotalEmailCount(p_map, child_folders[e])
              }
          }

          return total
    }

      func main() {

        var folders = []Folder{
          Folder{1, 30, []int{2, 4}},
          Folder{2, 10, []int{3}},
          Folder{4, 60, []int{}},
          Folder{3, 20, []int{}},
      }

      var m = InsertIntoMap(folders)

      fmt.Println(GetTotalEmailCount(m,1))  // result is 60
      fmt.Println(GetTotalEmailCount(m,2))  // result is 30
      fmt.Println(GetTotalEmailCount(m,3))  // result is 20
      fmt.Println(GetTotalEmailCount(m,4))  // result is 60

      }
```

First, your code doesn't give correnct answers; for (m, 1) the answer should be 120 (30 in ./1, +10 in ./1/2, +20 in ./1/2/3, +60 in ./1/4; you never count more than a single entry in the sub-folder slice).

Also, you should run go fmt (or goimports) on your code (perhaps you did and that was lost when posting). Also, adding a Go Playground link to your original code in posts is helpful.

You mention that you wanted to create a function taking two parameters, the first being []Folder but your code instead converts the slice into a map and uses that as the first argument. Which did you intend? When you have a function or functions that are answering questions based on an argument you could consider making a method. For example, perhaps something like:

type Folders []Folder
// or
type Folders struct {
    list []Folder
    byID map[int]Folder
}

func (f Folders) EMailCount(folder int) int {
    // code that uses f and folder as arguments
}

You use snake_case for some of your identifiers and camelCase for others; idiomatic Go code uses camelCase for all identifiers.

In your InsertIntoMap function:

When you make the map you know how many entries you expect so you should include that number to make as a size hint (e.g. make(map[int]Folder, len(data))). If the size is large this avoids the having to dynamically re-size the map as you add entries.
You name the map retval, I'd instead name it based on what contains (a map of folders by ID) rather than it happens to be the return value. I'd call it just m or byID.
Within the loop you effectively copy the element elem to the variable folder. You can do this with just folder := elem but at that point the entire loop body can just be rewritten as retval[elem.ID] = elem.

In your GetTotalEmailCount function:

Normally in Go instead of:

var foo []int
foo = something[bar].foo

you'll just see:

foo := something[bar].foo

not only is it shorter but it can make future code changes easier. In the latter if the field foo changes type you don't need to edit the type of the variable foo to match. (By the way, shorter isn't always better. Clarity is more important than conciseness).
Idiomatic Go code tends avoid indenting code by using early returns (see https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow). You return if len(…) == 0 so you should just drop the else clause and remove the indent (golint will suggest this). E.g, instead of:

if someCondition {
    return something
} else {
    // other code
    //     possibly with more conditional/loop indenting
}

it would be:

if someCondition {
    return something
}

// other code
//     possibly with more conditional/loop indenting

In this specific case, you don't even need the conditional since for range loops don't do anything on empty/nil slices the following return total line is sufficient.
It's in this for loop your your bug exists. On the first iteration you stop looping and return the total of this folder and it's first child without iterating to the next child.

return total + GetTotalEmailCount(p_map, child_folders[e])

should be:

total += GetTotalEmailCount(p_map, child_folders[e])

Without changing the basic structure of your code, all the above gives something like this (Go Playground):

package main

import "fmt"

type Folder struct {
    ID             int
    emailCount     int
    childFolderIDs []int
}

func InsertIntoMap(data []Folder) map[int]Folder {
    m := make(map[int]Folder, len(data))
    for _, e := range data {
        m[e.ID] = e
    }
    return m
}

func GetTotalEmailCount(m map[int]Folder, folder int) int {
    children := m[folder].childFolderIDs
    total := m[folder].emailCount

    for e := range children {
        // BUG: doesn't detect infinite loops
        total += GetTotalEmailCount(m, children[e])
    }

    return total
}

func main() {
    var folders = []Folder{
        Folder{1, 30, []int{2, 4}},
        Folder{2, 10, []int{3}},
        Folder{4, 60, []int{}},
        Folder{3, 20, []int{}},
    }

    var m = InsertIntoMap(folders)
    fmt.Println(GetTotalEmailCount(m, 1)) // result is 120, 30+10+20+60
    fmt.Println(GetTotalEmailCount(m, 2)) // result is 30
    fmt.Println(GetTotalEmailCount(m, 3)) // result is 20
    fmt.Println(GetTotalEmailCount(m, 4)) // result is 60
}


List files and folders in current dir with special handling for tty I want to write a custom ls command to learn rust.

This is what I've done as for the version 1.0. This display files and folders in a cyan or white if it's on a TTY. Otherwise I just print them.

I'm using atty and crossterm crates.

What I want reviewed

Is there a better way to deal with OsStr?
Is there a better way to convert OsStr to print-able strings?
Is there a better way to handle errors?
Any other area is fair game too.
Is my enum use good?

use std::fs::{self, DirEntry};
use std::path::Path;

use atty::Stream;
use crossterm::{Color, Colored};

enum FileEntryType {
    File,
    Dir,
}

fn main() {
    // WHY: no need to print colours on non tty
    if atty::is(Stream::Stdout) {
        visit_location(Path::new("."), &color_print);
    } else {
        visit_location(Path::new("."), &normal_print);
    }
}

fn visit_location(dir: &Path, cb: &dyn Fn(FileEntryType, &DirEntry)) {
    if !dir.is_dir() {
        return;
    }

    if let Ok(entries) = fs::read_dir(dir) {
        for entry in entries {
            if let Ok(entry) = entry {
                let path = entry.path();
                if path.is_dir() {
                    cb(FileEntryType::Dir, &entry);
                } else {
                    cb(FileEntryType::File, &entry);
                }
            }
        }
    } else {
        println!("Failed to list {}", dir.as_os_str().to_str().unwrap());
    }
}

fn color_print(file_kind: FileEntryType, p: &DirEntry) {
    match file_kind {
        FileEntryType::File => println!("{}{}", Colored::Fg(Color::White), p.file_name().as_os_str().to_str().unwrap()),
        FileEntryType::Dir => println!("{}{}", Colored::Fg(Color::Cyan), p.file_name().as_os_str().to_str().unwrap())
    }
}

fn normal_print(_: FileEntryType, p: &DirEntry) {
    println!("{}", p.file_name().as_os_str().to_str().unwrap());
}

How it looks like

OsStr conversion

According to the path source, s.as_os_str().to_str() is the same as s.to_str():

pub fn as_os_str(&self) -> &OsStr {
    &self.inner
}

pub fn to_str(&self) -> Option<&str> {
    self.inner.to_str()
}

Moreover, using unwrap() here is not safe at all as invalid Unicode is pretty common (e.g. when mounting file system with alien character encoding). There is Path::to_string_lossy that converts path to a printable string.

Using crossterm

crossterm provides helper macros to output strings. It is more idiomatic to use them instead of println!("{}{}", ...): execute and queue.

Using queue may be beneficial in case of outputting to the pipe instead of stdout.

Reporting errors

It is common practice to report errors to stderr. It also may be more helpful to show error reason when fs::read_dir() fails. You can also use crossterm to make it more colorful:

match fs::read_dir(dir) {
    Err(err) =>
        execute!(
            stdout(),
            Colored::Fg(Color::Red),
            Output(format!("Failed to list {}: {}", dir.to_string_lossy(), err)),
            Reset),
    Ok(entries) => ..
}

Callbacks

Using callbacks to print colorful vs. normal is a nice idea, but is not future-proof. It will be hard to add other output options like short/long view, hidden files, etc.


Monthly change query with SQL Server 2012 I'm new to SQL and working on a monthly change calculation.  I want to calculate the change between this months balance and the previous months balance.

This query is working for me but does not feel like the best syntax.  Do you think this is fine, or are there any suggestions?

SELECT
        ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) Rn,
        CONVERT(varchar, effectivedate, 102) effectivedate,
        SUM(EuroCurrentBalance/1000000) TotalArrears,
        SUM(CASE WHEN Multiplier > 2 THEN EuroCurrentBalance/1000000 ELSE 0 END) NPLs,
        CASE WHEN ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) = 1 THEN 0 ELSE SUM(EuroCurrentBalance/1000000) - (LAG(SUM(EuroCurrentBalance/1000000),1,0) OVER (ORDER BY (CONVERT(varchar, effectivedate, 102)))) END MonthlyChange
    FROM
        DataWarehouse.dwa.FinanceMonthEndData
    GROUP BY
        CONVERT(varchar, effectivedate, 102)
    GO

The size of the horizontal scrollbar is the first smell. I'd start by breaking down the CASE WHEN's into multiple lines:

SELECT
    ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) Rn,
    CONVERT(varchar, effectivedate, 102) effectivedate,
    SUM(EuroCurrentBalance/1000000) TotalArrears,
    SUM(CASE WHEN Multiplier > 2 THEN EuroCurrentBalance/1000000
             ELSE 0
        END) NPLs,
    CASE WHEN ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) = 1 THEN 0
         ELSE SUM(EuroCurrentBalance/1000000) - (LAG(SUM(EuroCurrentBalance/1000000),1,0) OVER (ORDER BY (CONVERT(varchar, effectivedate, 102))))
    END MonthlyChange
FROM
    DataWarehouse.dwa.FinanceMonthEndData
GROUP BY
    effectivedate
GO

Note that you can group by effectivedate without doing the conversion again.

Then I'd look at the redundant pieces: EuroCurrentBalance/1000000 is repeated quite often, and CONVERT(varchar, effectivedate, 102) as well. Consider a subquery, or a CTE:

WITH PreSelectionCTE
AS
(
    SELECT
        CONVERT(varchar, effectivedate, 102) EffectiveDate,
        EuroCurrentBalance/1000000 Arrear,
        CASE WHEN Multiplier > 2 THEN EuroCurrentBalance/1000000
             ELSE 0
             END NPL
    FROM
        DataWarehouse.dwa.FinanceMonthEndData
)
SELECT
    ROW_NUMBER() OVER(ORDER BY EffectiveDate) Rn,
    SUM(Arrear) TotalArrears,
    SUM(NPL) NPLs,
    CASE WHEN ROW_NUMBER() OVER(ORDER BY (EffectiveDate)) = 1 THEN 0
         ELSE SUM(Arrear) - (LAG(Arrear,1,0) OVER(ORDER BY EffectiveDate))
         END MonthlyChange
FROM
    PreSelectionCTE
GROUP BY
    EffectiveDate

GO

This could probably be improved further, but at least it's much easier to read now! Note that ASC is implicit in an ORDER BY - I'd simply omit it, or specify it consistently: you have it explicit in 2 of your 3 ORDER BY's.


Get an array of currency exchange prices based on asset Function getMarkets

Makes a call to get Cryptocurrency Exchange data based on asset (USD, USDC, USDT)

It calls the endpoint 3 times to return 3 arrays which are then returned to the callee.

I have an endpoint which hits gets exchange-markets/prices for any asset.

In my case I need to check all the paired cryptocurrency listings for USD, USDC and USDT.

At first I just tried calling getMarkets 3 times passing in the currency each time. I would then get an array of 3 responses from each call, however the last call would always replace the data for the 2 previous calls.

In order to fix that I had to use this ugly if statement indentation hell... which defeats the purpose of using async await. How should this be refactored to be cleaner, or is there a better way?

const nomicsAPI = 'https://api.nomics.com/v1/';
const nomicsKey = '8feb...;'

interface IParams {
  key: string;
  currency?: string;
}

interface IHeaders {
  baseURL: string,
  params: IParams
}

const headers: IHeaders = {
  baseURL: nomicsAPI,
  params: {
    key: nomicsKey
  }
};

const prepHeaders = (currency: string) => {
  headers.params.currency = currency;
  return axios.create(headers);
};

export const getMarkets = async (): Promise<any> => {
  try {
    let marketUSD;
    let marketUSDC;
    let marketUSDT;

    const nomicsUSD = prepHeaders('USD');
    marketUSD = await nomicsUSD.get('exchange-markets/prices');

    if (marketUSD) {
      const nomicsUSDC = prepHeaders('USDC');
      marketUSDC = await nomicsUSDC.get('exchange-markets/prices');

      if (marketUSDC) {
        const nomicsUSDT = prepHeaders('USDT');
        marketUSDT = await nomicsUSDT.get('exchange-markets/prices');

        return {
          marketUSD: marketUSD.data,
          marketUSDC: marketUSDC.data,
          marketUSDT: marketUSDT.data
        }
      }
    }
    else {
      throw new Error('USD Markets unavailable.');
    }
  } catch (err) {
    console.error(err);
  }
}

^ above produces the desired results:

And the actions file where this function is called from:

// Fetch USD, USDC & USDT markets to filter out Exchange List.
export const fetchMarketPrices = (asset: string) => (dispatch: any) =>
  getMarkets().then((res) => {
    console.log('res', res);

  // const combinedExchanges = res[0].data.concat(res[1].data).concat(res[2].data);
  // console.log('combinedExchanges', combinedExchanges);
  // const exchangesForAsset = combinedExchanges.filter((marketAsset: IMarketAsset) =>
  //   marketAsset.base === asset);
  // console.log('exchangesForAsset', exchangesForAsset);
  // return dispatch(actionGetMarketPrices(exchangesForAsset));
});

A common convention used in TypeScript/Javascript and other C-based languages is to use all uppercase letters for constants. This makes it easy to spot values that cannot be changed.

So instead of lowercase constants:

const nomicsAPI = 'https://api.nomics.com/v1/';
const nomicsKey = '8feb...;'

make them uppercase. The naming also could be improved - e.g. NOMICS_API_BASE_URL would better describe that the value is the base URL of the API than NOMICS_API.

const NOMICS_API_BASE_URL = 'https://api.nomics.com/v1/';
const NOMICS_KEY = '8feb...;'

You could also consider using the readonly modifier with those constants.

This code (as well as the updated code in your self-answer) is quite repetitive. It would be wise to abstract the common code. That way if something needs to be updated, it can be done in one place. This adheres to the Don't Repeat Yourself
 (i.e. D.R.Y.) principle.

I would first recommend storing each currency in an array:

const currencies = ['USD', 'USDC', 'USDT'];

And abstract out the common code of making the request and throwing an error if the request fails:

const getCurrencyPrice = async (currency) => {
  try {
    const request = prepHeaders(currency);
    const response =  await request.get(NOMICS_PRICES_ENDPOINT);
    if (!response) {
      throw new Error('USD Markets unavailable.');
    }
    return response.data;
  }
  catch(err) {
    console.error(err);
  }
};

That way if any of the requests fail, the Error will be thrown immediately. Also, this function has one job and could easily lend itself to unit testing.

Then use that function to iterate over the array of currencies - optionally with a for...of loop, Array.prototype.reduce(), Array.prototype.forEach(), etc

const getMarkets = async _ => {
  const returnObj = {};
  for (let currency of currencies) {
    const key = 'market' + currency;
    returnObj[key] = await getCurrencyPrice(currency);
  }
  return returnObj;
};

Notice that the code above uses a new constant - NOMICS_PRICES_ENDPOINT - this can be defined with the other constants:

const NOMICS_PRICES_ENDPOINT = 'exchange-markets/prices';

That way if the endpoint needs to be updated, in can be done in one place. Additionally, if the constants were stored in a separate file, you wouldn't need to alter the file that contains all of this code.


Using recursion to count substrings (with exceptions to the rule) in Java I am going through the CodingBat exercises for Java. Here is the one I have just finished:

  Given a string, compute recursively the number of times lowercase hi appears in the string, however do not count hi that have an x immedately before them.

And here is my code:

public int countHi2(String str) {

    if (str.length() < 3) {
        return str.equals("hi") ? 1 : 0;
    }

    if (str.substring(0, 3).equals("xhi")) {
        return countHi2(str.substring(2));
    }

    if (str.substring(0, 2).equals("hi")) {
        return 1 + countHi2(str.substring(2));
    } else {
        return countHi2(str.substring(1));
    }

}

This is the first in a recursion exercise which I have needed to change the base case to accommodate how the rest of the code works. I originally made it so that whenever an x is reached, it returns a substring from 2 (because the following hi is irrelevant), but of course, this is useless when there are two x's adjacent to one another.

I am still getting used to the idea of recursion, so I would like to know if this is a 'good' solution or not. Can it be made more efficient?

Similar to @Ben Aaronson above but uses indexOf() instead of startsWith()

public int countHi2(String str) {
    int pos = str.indexOf("hi");
    if (pos == -1) {
       return 0;
    }
    int increment = (pos == 0 || str.charAt(pos-1) != 'x')
                      ? 1
                      : 0;
    return countHi2(str.substring(pos+2))
           + increment;
}


Make this C# code more DRY public sealed class LinkXPathRepository : ILinkXPathRepository
{
    private static readonly ILog _log = LogManager.GetLogger(typeof(LinkXPathRepository));

    internal ILink Parse(Uri endpoint)
    {
        if (endpoint == null)
        {
            throw new ArgumentNullException("endpoint");
        }
        string leftPart = endpoint.GetLeftPart(UriPartial.Authority);

        EntityDataModelContext dc = DataContext.GetDataContext();

        IList<LinkXPath> sources = null;
        HtmlDocument document = null;

        Task t1 = Task.Factory.StartNew(() => sources = dc.LinkXPaths.Where(p => leftPart.Contains(p.Host)).OrderByDescending(p => p.Priority).ToList());
        Task t2 = Task.Factory.StartNew(() => document = Utility.DownloadHtml(endpoint));
        Task.WaitAll(new[] { t1, t2 });

        if (document == null)
        {
            return null;
        }

        Link result = new Link();
        foreach (LinkXPath source in sources)
        {
            if (result.Title.NullOrEmpty())
            {
                string title = GetFragment(document, source.Title);
                result.Title = title;
            }
            if (result.Description.NullOrEmpty())
            {
                string description = GetFragment(document, source.Description);
                result.Description = description;
            }
            if (result.Article.NullOrEmpty())
            {
                string article = GetFragment(document, source.Article);
                result.Article = article;
            }
        }

        return result;
    }

    internal string GetFragment(HtmlDocument document, string xpath)
    {
        return document.DocumentNode.GetChildNodeHtml(xpath);
    }
}

I'm particularly interested in refactoring this fragment:

            if (result.Title.NullOrEmpty())
            {
                string title = GetFragment(document, source.Title);
                result.Title = title;
            }
            if (result.Description.NullOrEmpty())
            {
                string description = GetFragment(document, source.Description);
                result.Description = description;
            }
            if (result.Article.NullOrEmpty())
            {
                string article = GetFragment(document, source.Article);
                result.Article = article;
            }

EDIT: please note that result.X and source.X come from different classes, even though their names are the same.

I propose this:

internal ILink Parse(Uri endpoint) {
    ...
    foreach (LinkXPath source in sources) {
        UpdateIfNecessary(document, ref result.Title, () => GetFragment(document, source.Title));
        UpdateIfNecessary(document, ref result.Description, () => GetFragment(document, source.Title));
        UpdateIfNecessary(document, ref result.Article, () => GetFragment(document, source.Article));
    }
    ...
}

private static void UpdateIfNecessary(Document document, ref string field, Func<string> getValue) {
    if(!string.IsNullOrEmpty(field)) {
        return;
    }
    field = getValue();
}

UpdateIfNecessary receives a lambda so it only evaluates the new value if necessary.

The field is passed by reference, so any change to the parameter in the method is reflected in the underlying argument variable in the calling method.


Open closed principle in a method I have an ASP.NET MVC project wherein I need to follow the open closed principles.

The project converts a .csv file to a model from a database, but in the future we might also have to convert Excel files to the same model from the database.

Now, I have this code in the Convertor class:

public class Convertor
{
    private static ICompanyRepository companyRepository;

    /// <summary>
    /// converts the uploaded csv data to Company model
    /// </summary>
    /// <param name="filePath">the csv data</param>
    /// <returns>a list of Compamy model</returns>
    public List<Company> ConvertCsvToCompanyModel(string filePath)
    {
        companyRepository = new CompanyRepository(new ImportContext());
        List<Company> companies = new List<Company>();

        //Read the contents of CSV file.
        string csvData = System.IO.File.ReadAllText(filePath);

        //we skip the first row, because it contain the header
        var csvLines = csvData.Split('\n').Skip(1);

        //Execute a loop over the rows.
        foreach (string row in csvLines)
        {
            if (!string.IsNullOrEmpty(row))
            {
                if (!companyRepository.CompanyExist(row.Split(',')[0]))//check if already contains the ExternalId
                {
                    companies.Add(new Company
                    {
                        //CounterPartId
                        ExternalId = row.Split(',')[0],
                        //Name
                        TradingName = row.Split(',')[1],
                        //IsBuyer
                        IsForwarder = Convert.ToBoolean(Enum.Parse(typeof(BooleanAliases), row.Split(',')[2])),
                        //IsSeller
                        IsCarrier = Convert.ToBoolean(Enum.Parse(typeof(BooleanAliases), row.Split(',')[3])),
                        //Phone
                        Phone = row.Split(',')[4],
                        //Fax
                        Fax = row.Split(',')[5]
                    });
                }
            }
        }
        return companies;
    }
}

Can you please give me some hints how to change this code in order to follow the open closed principle? In the future, as I mentioned, it is possible to need a converter for Excel files, too.

Before reviewing micro-design we may focus on overall architecture. You already know that in future you will also need to read an Excel file, it may be a good moment to introduce an abstract base class (or an interface) to abstract this detail from the client:

public abstract class Convertor
{
    public abstract IEnumerable<Company> ToCompanyModel(string path);
}

Note few changes:

I'm using Convertor as base class name then you will have, for example, CsvConvertor, no need to repeat any of those information in the method name.
I'm returning IEnumerable<Company> instead of List<Company>. The effective type you're using to store the result is an implementation detail, here I picked the most generic one but you may return IList<Company> as well (what if, for example, you will decide to store Company as an hash-table to detect duplicates?)

How to create the right Convertor instance? I assume you have DI in-place then you may have a factory class which will create the right instance according to file extension (assuming you're sure you will need Excel files). Something similar to:

public interface IConvertorFactory
{
    Convertor Create(string path);
}

If not then simply use DI to get the converter instance without any factory. You now have another problem: testing. Do you want to unit test your converter using physical files? Of course you need to but it's also handy to work with in-memory representations then you may add an overload with StreamReader:

public abstract class Convertor
{
    public abstract IEnumerable<Company> ToCompanyModel(string path);
    public abstract IEnumerable<Company> ToCompanyModel(StreamReader stream);
}

Default implementation may simply open the stream reading line-by-line and you won't need to worry about file size because it won't read everything in memory. Another benefit is that you can check for reader logic without mixing with I/O logic (and this is good to write extensive testing).

ICompanyRepository companyRepository has no reason to be static, you just make your class not thread-safe without any other benefit.

You're not doing any error handling. Things may go wrong and caller will get unknown exception. You'd better handle possible errors and return a single well-known exception (let's say InvalidDataException) with all the required details. Do not forget to document exceptions you may throw. You may even decide to ignore errors in one row and continue processing.

You're splitting each line with row.Split() multiple times, it's a waste: do it once.

To reduce nesting you can use continue and filters (example, again, without error handling):

foreach (string row in csvLines.Where(x => !String.IsNullOrEmpty(x))
{
    var fields = row.Split(',');

    var externalId = fields[0];

    if (companyRepository.CompanyExist(externalId))
        continue;

    // ...
}

However the truth is that you do not need to do CSV parsing by hand. In Microsoft.VisualBasic assembly (if you do not want to use an external library) you already have a well-tested and complete implementation:

using (var parser = new TextFieldParser(new StringReader(stream)))
{
    parser.SetDelimiters(new string[] { "," });

    // Skip header
    parser.ReadLine();

    while (!parser.EndOfData)
    {
        var fields = parser.ReadFields();

        // Same as before
    }

Note that you can catch MalformedLineException to handle errors and you can specify exactly the expected type of each field (if you want to). Also text fields may be enclosed in quotes, use TextFieldParser.HasFieldsEnclosedInQuotes property to instruct the parser. Blank lines are, by default, skipped and you may even use comments (see TextFieldParser.CommentTokens property).

Very last note: if CSV file is generated by Excel then be aware that it won't always use the comma as delimiter but the current list separator character (see CultureInfo.TextInfo.ListSeparator).


Creating folders even if already exist or removing removing their content I have a project where, every time I run it, I have to:

test if some folders exist, and create them if they don't exists.
remove their content if the folders already exist.

This is the code that I developed to do this:

import os
import pathlib

def clean_Folders(directory):
    folders = ['preprocessed', 'results', 'tmp']
    for folder in folders:
        if not os.path.exists(os.path.join(directory, folder)):
            os.makedirs(os.path.join(directory, folder))
    for filepath in pathlib.Path(directory).glob('**/*'):
        try:
            os.remove(filepath.absolute())

        except:
            print('something went wrong')

I had another idea which is creating the folders every time I run my project (without doing the existence test nor caring about what the already contain).
Here is the code:

import os
import shutil

def clean_Folders(directory):
    folders = ['preprocessed', 'results', 'tmp']
    for folder in folders:
        try:
            if os.path.exists(os.path.join(directory, folder)):
                    shutil.rmtree(os.path.join(directory, folder))
            os.makedirs(os.path.join(directory, folder))
        except:
            print('something went wrong')

Which one of the solutions is more optimized and more professional?
Thank you.

The second one is definitively safer, since in the first one you actually remove everything in all subfolders of directory, not just in the three folders you want to remove.

Some additional comments:

I would save the result of os.path.join(directory, folder) in a variable, you calculate it like three times.
Have a look at Python's official style-guide, PEP8. It recommends using lower_case both for variables and for function names, so your function should be called clean_folders.
A bare except is basically never a good idea. And even worse is a bare except with the unhelpful message 'something went wrong'. You want to either catch the exception you are looking for and handle it, or let the exception bubble to the top and halt the execution of the program. So, find out what kind of exception you actually want to catch (maybe IOError?). Or at least use except Exception, which excludes exceptions like e.g. the user pressing Ctrl+C being caught.


Implementing write sphere of values to 3D array The code below is sort of complicated to explain...
It writes a "sphere" of data to a 3D array from the given 3D-Position outwards in a spherical shape based on the given radius and value to write.

A graphical representation of this would be:

double UbytePrism :: PrismSphereSet( uint64 Xpos , uint64 Ypos , uint64 Zpos , sint64 Radius , ubyte Byte ) {
    for( sint64 w = -Radius; w <= Radius; w ++ ) {
        for( sint64 h = -Radius; h <= Radius; h ++ ) {
            for( sint64 d = -Radius; d <= Radius; d ++ ) {
                if ( Xpos + w < ByteBuffWidth && Ypos + h < ByteBuffHeight && Zpos + d < ByteBuffDepth && Xpos + w >= 0 && Ypos + h >= 0 && Zpos + d >= 0 ) {
                    if ( ( w * w ) + ( h * h ) + ( d * d ) < Radius * Radius ) {
                        BytePrism[ ( ( ByteBuffWidth * ByteBuffHeight ) * ( Zpos + d ) ) + ( ByteBuffWidth * ( Ypos + h ) ) + ( Xpos + w ) ] = Byte;
                    };
                };
            };
        };
    };

    return 0.0;
};

The general usage of this function would be:

// On a 7x7x7 grid like the image:
// PrismSphereSet( x , y , z , radius , value );
PrismSphereSet( 3 , 3 , 3 , 4 , 5 );

As you can see there is an issue where it's not drawing an exact circular shape.
If possible I'd like to fix and or modify this to work elliptical-spheres rather
than exact spheres. However, I am not sure how to modify the if-statement if ( ( w * w ) + ( h * h ) + ( d * d ) < Radius * Radius ) { of the
function to fit my needs since I pulled it from an example code.

I find it suspicious that your "Dimension 0" is completely empty. I'd at least expect to see a small "+" of 5's there. Whatever shows up in this last dimension should also show up (assuming spherical symmetry), on the top, the bottom and the sides of the sphere. This will round-out your square.

Perhaps you just need to try

if ( ( w * w ) + ( h * h ) + ( d * d ) <= Radius * Radius )

(i.e. <= instead of just <)

But I think you also need to ask yourself - should a sphere of radius 0 have a single 5 at the origin? Should a sphere of radius 1? What exactly does your radius mean?


log and throw in dot net I read a bit about log and throw and anti-pattern and so on, but I still not get exactly what's the problem.

I'm using entity framework and I want to log every exception appearing with its complete stacktrace. Since this is just an API, I'm not really interested in handling it in the context.
So, this is my database-context:

public class DatabaseContext : DbContext
{
    private readonly ILog _logger = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);

    public DatabaseContext()
        : base(Db.ConnectionString)
    {
        Database.Log = log => _logger.Debug(log);
    }

    public override async Task<int> SaveChangesAsync()
    {
        try
        {
            return await base.SaveChangesAsync();
        }
        catch (Exception exception)
        {
            _logger.Error(exception);
            throw;
        }
    }

    public override int SaveChanges()
    {
        try
        {
            return base.SaveChanges();
        }
        catch (Exception exception)
        {
            _logger.Error(exception);
            throw;
        }
    }
}

my question is: is it really bad practice to do this?

I'm not really interested in handling it in the context.

You don't know it yet but you are interested in handling it in a context. Without a context it's as useful as the generic NullReferenceException in .NET. Something happened somewhere but nobody knows where and why.

  is it really bad practice to do this?

Yes it is. The user of your API should take care of logging. You should not force him to log something he might not need or want and most probably he'll log it anyway somewhere else so he'll end up having same exceptions logged multiple times.

If you want to provide a logger for your API then doing it via a decorator would be much better than hardcoding it by inheritance.


Capitalize the first letter of all words in a String A rags-to-riches of A method to capitalize the first letter of all words in a String.

To recap the requirements:

  Please implement this method to capitalize all first letters of the words in the given String. All other symbols shall remain intact. If a word starts not with a letter, it shall remain intact too. Assume that the parameter String can only contain spaces and alphanumeric characters.

  NOTE: please keep in mind that the words can be divided by single or multiple spaces.The spaces also can be found at the beginning or the end of the parameter string,and you need to preserve them.

And the code:

public class Main
{
    public static void main(String... args) {
        for(String arg : args) {
            System.out.println(titleize(arg));
        }
    }

    /**
     * Titleize a string. Takes a string and returns a new string where all words
     * have had their first letter title cased. If the letter is already title case
     * or is not a cased letter (like a number), it will be passed through.
     * Leading, trailing, and all other whitespace is preserved.
     *
     * This method is not as robust as titleize in Rails. It does not do any magic
     * like breaking up MashedTogetherWords or replacing_underscores.
     *
     * This will blow up if null is passed in.
     */
    public static String titleize(final String input) {
        // Initialize the output to the length of the input since we know it and the
        // output and input will be the same size.
        StringBuilder output = new StringBuilder(input.length());
        // This defaults to true because we assume the beginning of the
        // string also counts as whitespace. This is to make sure the
        // first word gets capitalized as well.
        boolean lastCharacterWasWhitespace = true;

        for(int i = 0; i < input.length(); i++) {
            char currentCharacter = input.charAt(i);

            if(lastCharacterWasWhitespace && Character.isLowerCase(currentCharacter)) {
                currentCharacter = Character.toTitleCase(currentCharacter);
            }

            output.append(currentCharacter);

            lastCharacterWasWhitespace = Character.isWhitespace(currentCharacter);
        }

        return output.toString();
    }
}

The original code tried to use regular expressions and splitting words to accomplish the task. This does neither. It was partially inspired by the accepted answer, by treating the String as a stream of characters.

This is Java 7. I'm sure there's some Java 8 stream fanciness that can do this in half the lines.

This is a cool little problem that needs Java8, and regex. ... promise.

First up, the Pattern.splitAsStream(...) method is new in Java8, and converts a string in to a stream of substrings, depending on the pattern, so, what's the pattern?

Well, the pattern is to match the beginning of any word that needs to be capitalized. Your code currently finds characters, after whitespace, but that is, as you admit, too naive. Words that need capitalization are things like:

This is 'quoted' -> This Is 'Quoted'
This has-joined-words -> This Has-Joined-Words
This should not 1800not1337 -> This Should Not 1800not1337

and so on.

So, your code would not deal well with the above examples.... but, how can it be better?

private static final Pattern bound = Pattern.compile("\\b(?=\\w)");

The above regular expression looks for a word-boundary that is followed by a character (where "a character" is a letter, digit, or underscore).

Then, you have a sequence of sentence parts that need captialization, and it can be streamed easily:

private static final Pattern bound = Pattern.compile("\\b(?=\\w)");

private static final String ucFirst(String input) {
    return input.substring(0,  1).toUpperCase() + input.substring(1);
};

public static String titleize(final String input) {
    return bound.splitAsStream(input)
          .map(StringCap::ucFirst)
          .collect(Collectors.joining());
}

You can see this running here: http://ideone.com/ilmBqi

Note: Using Java7 versions of Pattern matchers, it is also actually quite easy.... Consider this pattern, which stores the first-character as a pattern group, and then the "traditional" matching replacement loop:

private static final Pattern bound = Pattern.compile("\\b(\\w)");

public static String titleize(final String input) {
    StringBuffer sb = new StringBuffer(input.length());
    Matcher mat = bound.matcher(input);
    while (mat.find()) {
        mat.appendReplacement(sb, mat.group().toUpperCase());
    }
    mat.appendTail(sb);
    return sb.toString();
}

The above is able to capitalize the words in the same output as the Java8 version above.


Efficient use of LINQ - Indexing I have a Class with a List of objects, and those objects contain a list which has lists to objects in the first class, and I'm trying to quickly index them.  Simplified:

class Person
{
    public IEnumberable<HistoricalEvent> Events
    {
        get
        {
            return World.HistoricalEvents.Values.Where(x=>x.PeopleInvolved.Contains(this));
        }
    }
}

Also

class HistoricalEvent
{
    public virtual IEnumerable<HistoricalFigure> PeopleInvolved
    {
        get { return Enumerable.Empty<People>(); }
    }
...
}

and a subclass

class HistoricalEventSubclass : HistoricalEvent
{
    Person person1 = new Person("Alice");
    Person person2 = new Person("Bob");

    public override IEnumerable<Person> PeopleInvolved
    {
        get
        {
            yield return person1;
            yield return person2;
        }
    }
    ...
}

The Events class is called from a method using: frm.grpHistoricalFigureEvents.FillListboxWith(frm.lstHistoricalFigureEvents, Events);

    internal static void FillListboxWith(this GroupBox groupbox, ListBox listbox, IEnumerable<object> objects)
    {
        if (objects == null || !objects.Any())
        {
            groupbox.Visible = false;
            return;
        }
        groupbox.Visible = true;
        listbox.BeginUpdate();
        listbox.Items.Clear();
        listbox.Items.AddRange(objects.ToArray());
        listbox.EndUpdate();
        listbox.SelectedIndex = 0;
        var title = groupbox.Text.Split('(')[0].Trim();
        groupbox.Text = string.Format("{0} ({1})", title, listbox.Items.Count);
    }

The problem is that calling Events takes a long time (as World.HistoricalEvents can be a very big List).  Is there a more time-efficient way of doing this?

Part of the slowdown is that Events is converted ToArray() and shoved into a listbox, and so I'll also be pursuing an alternative such as a listbox which can handle an IEnumberable directly.

How do you load the data for events and people? If you can denormalise your data so that a person has a materialised Set/List of events they are involved in I suspect you would gain a lot of performance (at the cost of a bit of memory).

Here is some pseudo code to explain what I mean:

var personStore = new Dictionary<string, Person>();
var eventStore = new Dictionary<string, Event>();
foreach (var eventData in eventSource)
{
  var event = new Event(eventData);
  eventStore[eventData.Name] = event;

  foreach (var personData in eventData)
  {
    // Is this the first time we've seen this person?
    if (!personStore.HasKey(personData.Name)
    {
      personStore[personData.Name] = new Person(personData);
    }

    var person = personStore[personData.Name];

    // This creates the bidirectional mapping which makes lookups
    // in either direction fast.
    event.PeopleInvolved.Add(person);
    person.Events.Add(event);
  }
}


Auto filter data from worksheet and create vlookup using a different worksheet I'm working on a large VBA project for work (around 1000+ lines of vba code) This module is called within another module where I disable everything possible like screen updating, events and setting calculation to manual to improve speed, yet this still seems to run slowly when I'm working with sheets with over 60,000 rows.

What this code does, is filter a row in one sheet and copy paste it into another sheet. Then use the data from this to vlookup onto another sheet. I don't understand the syntax of index match, otherwise I could have eliminated the cut/paste option to move the column on the right for the vlookup to speed it up.

I suspect a few other elements in this code chunk are slowing it down too. Could you please help me identify what is causing the issue? Is it the autofilter?

Sub AdgroupName()

Dim LrowBlk As Long
LrowBlk = Sheets("Bulk Sheet").Cells(Rows.Count, "H").End(xlUp).Row
Sheets("Bulk Sheet").AutoFilterMode = False
Sheets("Bulk Sheet").Range("H3:I3").AutoFilter field:=2, Criteria1:="AdGroup"
ActiveWorkbook.Sheets("Bulk Sheet").Range("H3:K3" & LrowBlk).SpecialCells(xlCellTypeVisible).Copy
Worksheets("Adgroup List").Paste Destination:=Worksheets("Adgroup List").Range("A1")
ActiveWorkbook.Sheets("Bulk Sheet").Range("P3:P" & LrowBlk).SpecialCells(xlCellTypeVisible).Copy
Worksheets("Adgroup List").Paste Destination:=Worksheets("Adgroup List").Range("E1")
With Sheets("Adgroup List")
    .Columns("C:C").Cut
    .Columns("E:E").Insert Shift:=xlToRight
 End With
Application.CutCopyMode = False
Dim ws As Worksheet
Dim LrowSTR As Long
Set ws = ThisWorkbook.Sheets("Search Term Report")
LrowSTR = Sheets("Search Term Report").Cells(Rows.Count, "H").End(xlUp).Row
Dim Strformulas(1 To 3) As Variant

With ws
    Strformulas(1) = "=VLOOKUP($K4,'Adgroup List'!$C:$D,2,0)"
    Strformulas(2) = "=VLOOKUP($K4,'Adgroup List'!$C:$E,3,0)"
    .Range("AJ4:AK4").Formula = Strformulas
    .Range("AJ4:AK" & LrowSTR).FillDown
    .Range("AJ3").Value = "Campaign ID"
    .Range("AK3").Value = "AdGroup Name"
    End With
     Sheets("Bulk Sheet").AutoFilterMode = False
     Sheets("Adgroup List").AutoFilterMode = False
     End Sub

ooh wee Because of the below statement, I'll do this in little steps which may not be the best approach, but should be easy to follow.

  I don't understand the syntax of index match, otherwise I could have eliminated ...

let me drop an article from my website right here

I'm not bashing you for not knowing index/match, but you might as well learn it! use it to look up to the left

VBA and worksheet functions

All right, so you're using VBA - you don't need to use worksheet functions at all. If you need the function to exist because the audience needs to see the functions rather than the results, then that's another issue.

ActiveWorkbook.Sheets("Bulk Sheet").Range("H3:K3" & LrowBlk).SpecialCells(xlCellTypeVisible).Copy

Is this essentially .Range("H3:K3" 130) (using 130 arbitrarily)? How does that work?

You also sort based on H3:I3 but then copy H3:K3 and P - are columns J through P filtered when you filter based on column I?

That's pretty confusing. Essentially you have a range (like "H3:P400") that you want to get elements from only if columns 2 matches a string ("adGroup")?

So what? Arrays.

There are a lot of ways to do this, but let's start with the way you've done it, but instead use an array -

bulksheet.Range("H3:P" & lastRow).AutoFilter field:=2, Crteria1:="AdGroup"
Dim bulkData As Variant
bulkData = bulksheet.Range("H3:K" & lastRow).SpecialCells(xlCellTypeVisible)

Right now you have all your data in a variant where you can do your thing instead of on the sheet. Essentially, it's the same advice as

  Be sure to avoid things like .Select - it just slows the code down
  by needing to fiddle with the spreadsheet while doing everything else
  behind the scenes. There's a good question on StackOverflow
  addressing
  this.

So now you have a variant where there are 9 columns and you need 4 of them on a new sheet. You could just put them on the new sheet like this -

For i = LBound(bulkData) To UBound(bulkData)
    adGroupSheet.Cells(i, 1) = bulkData(i, 1)
    adGroupSheet.Cells(i, 2) = bulkData(i, 2)
    adGroupSheet.Cells(i, 3) = bulkData(i, 3)
    adGroupSheet.Cells(i, 1) = bulkData(i, 9)
Next

But maybe there's a better way to do all of that (I'm not 100%, I might have read somewhere that autofilter is faster than a loop, but I digress). Simply -

Dim bulkData As Variant
Dim bulkLastRow As Long
bulkLastRow = bulksheet.Cells(bulksheet.Rows.Count, 8).End(xlUp).Row
bulkData = bulksheet.Range(bulksheet.Cells(1, 8), bulksheet.Cells(lastrow, 16))
Dim sortedData As Variant
ReDim sortedData(1 To bulkLastRow, 1 To 4)
Dim bulkIndex As Long
Dim sortedIndex As Long
sortedIndex = 1
For bulkIndex = LBound(bulkData) To UBound(bulkData)
    If bulkData(bullkindex, 2) = "AdGroup" Then
        sortedData(sortedIndex, 1) = bulkData(bulkIndex, 1)
        sortedData(sortedIndex, 2) = bulkData(bulkIndex, 2)
        sortedData(sortedIndex, 3) = bulkData(bulkIndex, 3)
        sortedData(sortedIndex, 4) = bulkData(bulkIndex, 9)
        sortedIndex = sortedIndex + 1
    End If
Next

Right, now you have your sorted data in a variant and could easily just

 sortedsheet.Range(sortedsheet.Cells(1, 1), sortedsheet(sortedIndex, 4)) = sortedData

Now you can write your formulas.

But wait

But, I see now that you moved column 3 to column 4 -

With Sheets("Adgroup List")
    .Columns("C:C").Cut
    .Columns("E:E").Insert Shift:=xlToRight
 End With

This could easily be fixed by just putting it there in the first place with something like

 ReDim sortedData(1 To bulkLastRow, 1 To 5)
 sortedData(sortedIndex, 1) = bulkData(bulkIndex, 1)
 sortedData(sortedIndex, 2) = bulkData(bulkIndex, 2)
 sortedData(sortedIndex, 4) = bulkData(bulkIndex, 3)
 sortedData(sortedIndex, 5) = bulkData(bulkIndex, 9)

And you have a blank column.

All right, so?

But, why stop there? Assuming you don't need the formulas to be present on the sheet, you can get your data from your variant. But let's examine what you're doing first -

LrowSTR = termSheet.Cells(termSheet.Rows.Count, 8).End(xlUp).Row
Dim termData As Variant
ReDim termData(1 To 3)
termData(1) = "=VLOOKUP($K4,'Adgroup List'!$C:$D,2,0)"
termData(2) = "=VLOOKUP($K4,'Adgroup List'!$C:$E,3,0)"

You make a 1 dimensional array, look up a single value to populate 2 of the 3 items and never use your LrowSTR. Then

.Range("AJ4:AK4").Formula = Strformulas
.Range("AJ4:AK" & LrowSTR).FillDown

Yikes - of course that will hang up. Even without really changing your code it would be easier to just

For i = LBound(termData) To UBound(termData)
    termData(i, 1) = "=VLOOKUP($K4,'Adgroup List'!$C:$D,2,0)"
    termData(i, 2) = "=VLOOKUP($K&" & i + 3 & ",'Adgroup List'!$C:$E,3,0)"
Next

And then paste it all at once, no need to autofill (which is definitely slow). You're also searching column "C" when you already moved it and it's empty. Why not just look in D and E? I digress

Regardless, you might just

Dim termData As Variant
termData = termsheet.Range(termsheet.Cells(4, 11), termsheet(LrowSTR, 11))
ReDim Preserve termData(1 To UBound(termData), 1 To 3)
'find things in sortedData and populate termData with values
'print to termSheet

Misc

Other things to note -

You've declared all of your variables, that's great! Your naming could be improved a bit and Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names. So LrowBlk would be bulkLastRow or something. And ws would be a terrible name, but
Worksheets have a CodeName property - View Properties window (F4) and the (Name) field (the one at the top) can be used as the worksheet name. This way you can avoid Sheets("mySheet") and instead just use mySheet.
Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know. This isn't a problem for you, but it's good practice - I have it on.
It's good practice to indent all of your code that way Labels will stick out as obvious. VBE standard is 4 spaces between levels


Thread-safety and delegates with generated numbers In the main class, loops generate numbers (0~100), and when its generated number is > 20, its value is passed to the thread where it simulates some work with this number. Meanwhile, while this number is being processed, other generated numbers > 20 must be skipped.

Is this code OK? I am not sure if I'm doing something bad or not. In fact it seems to be working fine, but I don't know if it can be written by that way or even "better.".

class Program
{
    delegate void SetNumberDelegate(int number);
    delegate bool IsBusyDelegate();

    static void Main(string[] args)
    {
        Random rnd = new Random();
        ProcessingClass processClass = new ProcessingClass();
        SetNumberDelegate setNum = new SetNumberDelegate(processClass.setNumber);
        IsBusyDelegate isBusy = new IsBusyDelegate(processClass.isBusy);
        Thread processThread = new Thread(new ThreadStart(processClass.processNumbers));
        processThread.Start();
        int num;
        int count = 0;
        while (count++ < 100)
        {
            num = rnd.Next(0, 100);
            Console.WriteLine("Generated number {0}", num);
            if (num > 20)
            {
                if (!isBusy())
                {
                    setNum(num);
                }
                else
                {
                    Console.WriteLine("Thread BUSY! Skipping number:{0}", num);
                }
            }
            Thread.Sleep(1);
        }
        processThread.Abort();
        processThread.Join();
        Console.ReadKey();
    }
}
class ProcessingClass
{
    private volatile bool busy;
    private volatile int number;

    public ProcessingClass()
    {
        busy = false;
        number = -1;
    }

    public bool isBusy()
    {
        return busy;
    }

    public void setNumber(int num)
    {
        number = num;
    }

    public void processNumbers()
    {
        while (true)
        {
            if (number > -1 && !busy)
            {
                busy = true;
                Console.WriteLine("Processing number:{0}", number);
                // simulate some work with number e.g. computing and storing to db
                Thread.Sleep(500);
                Console.WriteLine("Done");
                number = -1;
                busy = false;
            }
        }
    }
}

There is no synchronisation here and so all access to the thread's state is subject to data races. It is comprehensively not threadsafe.

From what I can tell all you need is to implement a producer/consumer pattern. Do this with a blocking queue. Please don't use busy loops. They burn CPU cycles and are horribly non-performant.


Closure, anonymous functions or binds on delayed functions I'm calling an expensive function that dynamical changes as the end user types in a textbox. To reduce the amount of calls to the function I've used setTimeout to delay calling the function, and if the function is called again whilst waiting to run, it'll also remove the last function request via clearTimeout.

The function for this is:

// To not have different functions conflict
function makeDelay() {
    var timerID = false;
    return function (fn, timeout) {
        if (timerID !== false) {
            clearTimeout(timerID);
        }
        if (timeout == undefined) {
            timeout = 500;
        }
        timerID = setTimeout(function() {
            timerID = false;
            fn();
        }, timeout);
    }
}
var delay = makeDelay();
var $peoplePicker = $('.ms-peoplePicker');
var $results = $peoplePicker.find(".ms-PeoplePicker-results");

However I don't know what the most idiomatic way to use this function is. I know that I can use one of the following:

A closure

function get(input, html_output) {
    return function () {
        if (!input) {
            return;
        }
        $.ajax('/_api/pp_search/' + input).done(function (data) {
            html_output[0].innerHTML = data;
        })
    }
}

$peoplePicker.on('input', '.ms-PeoplePicker-searchField', function(event) {
    delay(get(event.currentTarget.value, $results));
});

bind

function get(input, html_output) {
    if (!input) {
        return;
    }
    $.ajax('/_api/pp_search/' + input).done(function (data) {
        html_output[0].innerHTML = data;
    })
}

$peoplePicker.on('input', '.ms-PeoplePicker-searchField', function(event) {
    delay(get.bind(undefined, event.currentTarget.value, $results));
});

Anonymous functions

function get(input, html_output) {
    if (!input) {
        return;
    }
    $.ajax('/_api/pp_search/' + input).done(function (data) {
        html_output[0].innerHTML = data;
    })
}

$peoplePicker.on('input', '.ms-PeoplePicker-searchField', function(event) {
    delay(function () {
        get(event.currentTarget.value, $results);
    });
});

Using a closure seems like the nicest, as you don't have to explicitly make a delayed function at usage via bind or an anonymous function. However, anonymous functions seem to be the de facto way to call functions like these.

Which, if any, way should I use this function?

Notes

I ask because I made a JavaScript program that heavily used closures for this style of function, but was really hard to read.

Actually, what you are looking for is already done in Lodash via debounce or throttle. And yes, a closure is probably the best way to do this. So let's take your closure example and improve it.

Instead of returning a function, why not just define the function normally. That way, there's no developer overhead. Devs shouldn't know about your limiter function just to make their APIs.

// We'll be fine with this
function get(input, html_output) {
  if (!input) return;

  $.ajax('/_api/pp_search/' + input).done(function (data) {
    html_output[0].innerHTML = data;
  });

}

delay is a bit deceiving as the word "delay" can mean a lot of things. It can mean "pause", which then equates to sleep which does an entirely different thing. "debounce" and "throttle" are actually better names. Verbose names aren't that bad as well (except for the extra keystrokes). Something like holdExecutionUntilAfter.

Now for the implementation, let's accept a delay and the function we want to delay. Our return will be our "proxy" function that controls the real function from executing.

function holdExecutionUntilAfter(fn, delay){
  var timer = null;

  return function(){
    if(timer) clearTimeout(timer);

    setTimeout(function(){
      fn.apply(null, arguments);
      clearTimeout(timer);
    }, delay);
  }
}

As simple as that! What it does is get a reference of the real function into a closure. Then we return a "proxy" function which well be using to call the real function. When it gets called, it resets the timer and kicks off another. We use apply to pass in the arguments like you normally would in the regular call. Usage will be as simple as:

var get = delay(function(input, html_output){
  if (!input) return;
  $.ajax('/_api/pp_search/' + input).done(function (data) {
    html_output[0].innerHTML = data;
  });
}, 500);

get(event.currentTarget.value, $results);

// or verbosely

function get(input, html_output){
  if (!input) return;

  $.ajax('/_api/pp_search/' + input).done(function (data) {
    html_output[0].innerHTML = data;
  });
}
var proxiedGet = delay(get, 500);

proxiedGet(event.currentTarget.value, $results);


My implementation for std::unique_ptr I just finished learning about move semantics and realized that a nice practical example for this concept is unique_ptr (it cannot be copied, only moved).

For learning purposes, and as a personal experiment, I proceed to try to create my implementation for a smart unique pointer:

template<typename T>
class unique_ptr {
private:
    T* _ptr;
public:
    unique_ptr(T& t) {
       _ptr = &t;
    }
    unique_ptr(unique_ptr<T>&& uptr) {
       _ptr = std::move(uptr._ptr);
       uptr._ptr = nullptr;
    }
    ~unique_ptr() {
       delete _ptr;
    }
    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) {
       if (this == uptr) return *this;
       _ptr = std::move(uptr._ptr);
       uptr._ptr = nullptr;
       return *this;
    }

    unique_ptr(const unique_ptr<T>& uptr) = delete;
    unique_ptr<T>& operator=(const unique_ptr<T>& uptr) = delete;
};

For a small set of test cases, this is working like the real unique_ptr.

However, it just seems too simple enough.

I have two questions regarding this code:

Is it well-formed? i.e. does it follow common C++ standard and patterns (for example, should private members be declared before public ones?
Am I missing something regarding functionality? Is there maybe a bug in my code that I'm not seeing?

Overall

Ohhhhh.

  Is it well-formed?

It compiles, so yes.

  i.e. does it follow common C++ standard and patterns (for example, should private members be declared before public ones?

Personally I think so.

* Private Variables
* Public
    * Constuctor / Destructor
    * Copy Semantics
    * Move Semantics
    * Swap
    * Other Public Interface
    * Friends
* Private
    * Methods as appropriate

When reading the code I want to know the members so I can verify that the constructors initialize them all, as a result I usually put them first. But other people prefer to put all private stuff at the bottom.

  Am I missing something regarding functionality?

Yes. Quite a lot.

  Is there maybe a bug in my code that I'm not seeing?

Yes. It potentially leaks on assignment.

Code Review

Constructing from object!

    unique_ptr(T& t) {
       _ptr = &t;
    }

That's exceedingly dangerous:

{
    int x;
    unique_ptr<int>  data(x);
}
// Here the unique ptr calls delete on an automatic variable.

Use member initializing list.

You should always attempt to use the member initializer list for initializing members. Any non-trivial object will have its constructor called before the initializer code is called and thus it is inefficient to then re-initialize it in the code.

    unique_ptr(unique_ptr<T>&& uptr)
        : _ptr(std::move(uptr.ptr))
    {
       uptr._ptr = nullptr;
    }

Member variable Names

Prefer not to use _ as the first character in an identifier name.

T* _ptr;  // Technically OK.

Even if you know all the rules of when to use them most people don't so they are best avoided. If you must have a prefix to identify members use m_ - but if you name your member variables well then there is no need for any prefix (in my opinion prefixes makes the code worse not better, because you are relying on unwritten rules. If you have good well-defined names (see self-documenting code) then members should be obvious).

NoExcept

The move operators should be marked as noexcept.

When used with standard containers this will enable certain optimizations. This is because if the move is noexcept then certain operations can be guaranteed to work and thus provide the strong exception guarantee.

    unique_ptr(unique_ptr<T>&& uptr) noexcept
                              //     ^^^^^^^^

    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) noexcept
                                            //     ^^^^^^^^

Leak in assignment

Note: Your current assignment potentially leaks. If this currently has a pointer assigned then you overwrite it without freeing.

    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) {
       if (this == uptr) return *this;

       // Here you overwrite _ptr
       // But if it has a value then you have just leaked it.
       _ptr = std::move(uptr._ptr);

       uptr._ptr = nullptr;
       return *this;
    }

Checking for this pessimization

    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) {
       if (this == uptr) return *this;
       _ptr = std::move(uptr._ptr);
       uptr._ptr = nullptr;
       return *this;
    }

Yes you do need to make it work when there is self assignment. But in real code the self assignment happens so infrequently that this test becomes a pessimization on the normal case (same applies for copy operation). There have been studies on this (please somebody post a link; I have lost mine and would like to add it back to my notes).

The standard way of implementing move is via swap. Just like Copy is normally implemented by Copy and Swap.

    unique_ptr(unique_ptr<T>&& uptr) noexcept
        : _ptr(nullptr)
    {
        this->swap(uptr);
    }
    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) noexcept
    {
        this->swap(uptr);
        return *this;
    }
    void swap(unique_ptr<T>& other) noexcept
    {
        using std::swap;
        swap(_ptr, other._ptr);
    }

Using the swap technique also delays the calling of the destructor on the pointer for the current object. Which means that it can potentially be re-used. But if it is going out of scope the unique_ptr destructor will correctly destroy it.

Summary

Good first try but still lots of issues.

Please read the article I wrote on unique_ptr and shared_ptr for lots more things you should implement.

Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors

Some things you missed:

Constructor with nullptr
Constructor from derived type
Casting to bool
Checking for empty
Guaranteeing delete on construction failure.
Implicit construction issues
Dereferencing

When you have read all three articles then the bare bones unique_ptr looks like this:

namespace ThorsAnvil
{
    template<typename T>
    class UP
    {
        T*   data;
        public:
            UP()
                : data(nullptr)
            {}
            // Explicit constructor
            explicit UP(T* data)
                : data(data)
            {}
            ~UP()
            {
                delete data;
            }

            // Constructor/Assignment that binds to nullptr
            // This makes usage with nullptr cleaner
            UP(std::nullptr_t)
                : data(nullptr)
            {}
            UP& operator=(std::nullptr_t)
            {
                reset();
                return *this;
            }

            // Constructor/Assignment that allows move semantics
            UP(UP&& moving) noexcept
            {
                moving.swap(*this);
            }
            UP& operator=(UP&& moving) noexcept
            {
                moving.swap(*this);
                return *this;
            }

            // Constructor/Assignment for use with types derived from T
            template<typename U>
            UP(UP<U>&& moving)
            {
                UP<T>   tmp(moving.release());
                tmp.swap(*this);
            }
            template<typename U>
            UP& operator=(UP<U>&& moving)
            {
                UP<T>    tmp(moving.release());
                tmp.swap(*this);
                return *this;
            }

            // Remove compiler generated copy semantics.
            UP(UP const&)            = delete;
            UP& operator=(UP const&) = delete;

            // Const correct access owned object
            T* operator->() const {return data;}
            T& operator*()  const {return *data;}

            // Access to smart pointer state
            T* get()                 const {return data;}
            explicit operator bool() const {return data;}

            // Modify object state
            T* release() noexcept
            {
                T* result = nullptr;
                std::swap(result, data);
                return result;
            }
            void swap(UP& src) noexcept
            {
                std::swap(data, src.data);
            }
            void reset()
            {
                T* tmp = release();
                delete tmp;
            }
    };
    template<typename T>
    void swap(UP<T>& lhs, UP<T>& rhs)
    {
        lhs.swap(rhs);
    }
}

Test to make sure it compiles:

struct X {};
struct Y: public X {};

int main()
{
    ThorsAnvil::UP<X>   x(new X);
    ThorsAnvil::UP<Y>   y(new Y);

    x   = std::move(y);                  // This should be valid
    ThorsAnvil::UP<X>   z(std::move(y)); // This should be valid

    // In both these cases x represents an X* which is a base pointer
    // that should be able to point at objects of type Y* as these
    // are derived from X
}


Find website URLs that begin with http:// or https:// This is a problem from Automate the Boring Stuff from "Pattern Matching with Regular Expression". I am a beginner in Python and the purpose of this program is to match urls which start with http:// or https://, with some urls having an optional www. after https://

How can this code be improved?

#! /usr/bin/python3
#websiteURL.py - Find website URLs that begin with http:// or https://

import pyperclip, re

#protocol Regex - checks for http:// or https://
protocolRegex = re.compile(r'''
    https?://           #match http:// or https://
    (?:w{3}\.)?         #www-dot
    [a-zA-Z0-9_-]+      #domin name
    \.                  #dot
    [a-zA-Z]{2,3}       #extension
    ''', re.VERBOSE)

text = str(pyperclip.paste()) #copying data from document to clipboard and converting it into a string
matches = [] #stores all matches in this list

for website in protocolRegex.findall(text): #finding website from the string text
    matches.append(website)

if len(matches) > 0:
    pyperclip.copy('\n'.join(map(str, matches))) #copying result to clipboard after adding newline after each match
    print('Copied to clipboard:')
    print('\n'.join(matches))
else:
    print('No website found')

Running code:

chmod +x websiteURL.py
./websiteURL.py

Variable Names

Variables should be lower-cased and snake-case, meaning that multiple words should look like some_variable, not someVariable or SomeVariable. I think the name you've chosen for your regex is good and expresses exactly what it is:

protocol_regex = re.compile(...)

Checking length of a list/container data structure

It is not considered pythonic to check the emptiness of a container in python by using if len(container) == 0, most* containers have a __bool__ method built-in that allows you to do if not container for an empty one and if container for non-empty:

from collections import deque

if not {}:
    print("empty dict")
empty dict

if not []:
    print("empty list")
empty list

if not '':
    print("empty string")
empty string

if not deque():
    print("empty deque")
empty deque

if ['result1']:
    print('non-empty list')
non-empty list

One of the few that does not behave this way is queue.Queue, which I've included to give context to the most comment.

With this in mind, change your match check to:

if matches:
    # rest of code
else:
    # else code here


Random Prime generator I have this method that will give a random prime for a given range (min inclusive an max not inclusive) all comments are welcome:

public static int randomPrime(int min, int max) {
    if (min < 0)
        throw new IllegalArgumentException("min must be positive.");
    if (min >= max)
        throw new IllegalArgumentException("min must be smaller the max.");
    if (!containsPrime(min,max))
        throw new IllegalArgumentException("no Primes in this interval.");
    if (rand == null) {
        rand = new Random();
    }
    int out = rand.nextInt(max - min) + min;
    while (!isPrime(out)) {
        out = rand.nextInt(max - min) + min;
    }
    return out;
}
    private static boolean containsPrime(int min, int max) {
    if (isPrime(min)||isPrime(max-1)){
        return true;
    }
    if(min%2==0){
        min +=1;
    }
    while (min<max){
        if (isPrime(min)){
            return true;
        }
        min +=2;
    }
    return false;
}

int out = rand.nextInt(max - min) + min;
while (!isPrime(out)) {
    out = rand.nextInt(max - min) + min;
}
return out;

This duplicate code can be replaced with a do/while loop. The rule of thumb is, if you always need to perform an action at least once, do while!

int out;
do {
    out = rand.nextInt(max - min) + min;
} while(!isPrime(out));
return out;


Require config file in class I want to use the variables that have been declared in a config.php file in a class for testing. I am not sure if this is the right way like I have done it.

I have a configuration file, config.php:

class Config {

    public $hosts = array(
        'a' => '192.168.1.10'
        'b' => '192.168.1.11',
    );

    public $ports = array(
        'a' => 22,
        'b' => 22,
    );
}

These are the variables I want to include in a file called Tests.php:

require_once 'config.php';

    class Test extends PHPUnit_Framework_TestCase
    {
        protected $hosts;
        protected $ports;
        ...

        public function __setUp()
        {
            $config = new Config();
            $this->hosts = $config->hosts;
            $this->portsSsh = $config->ports;
            ...
        }

        public function testConnect()
        {
            $session = $this->connect($this->hosts['a'], $this->ports['a']);
            $this->closeConnection($session);
        }
    ...
    }

Is it right doing it like this? I have the impression it is too complicated. I have to make a class for the config.php file and in the test class I have to do like a mapping $this->hosts = $config->hosts.

That I don't like so much, because I have about 14 variables and first I have to declare them all and later I have to assign them all in the setUp() method. Isn't there a shorter or simpler way? I was thinking of using a loop and I don't want to use globals.

I wonder if I should do it like this:

require_once 'config.php';

        class Test extends PHPUnit_Framework_TestCase
        {
            protected $hosts;
            protected $ports;
            ...

            public function __setUp()
            {
                $this->config = new Config();
            }

            public function testConnect()
            {
                $session = $this->connect($this->config->hosts['a'], $this->config->ports['a']);
                $this->closeConnection($session);
            }
        ...
        }

This 'problem' of yours can be solved using Separation of concern. Your Database class should not know about config and how it is loaded. It should simply ask for the stuff it needs. Neither should your Test class know how to connect. Your test class should test the Database class. And your database class should handle the 'connection failed' error of whatever library you are using.

For instance:

class DatabaseConnection
{
    public function __construct($host, $username, $password, $port='22'){
        if connection failed throw new ConnetionFailedException
    }
}

Your would then also have a config class that loads the correct config file. e.g.:

class Config
{
    public function __construct($enviroment)
    {
        $this->data = include __path_to_config_files_.'/'.$enviroment.'.inc';
    }
}

Your could for instance use a __get method to retrieve the data instead of having a getParameter.

your config file would look something like this:

__path_to_config_files_/development.inc

<?php

return array(
    'key' => 'value'
);

And your test class would then:

$config = new Config('development'); //or whatever enviroment you are in
try {
    $connection = new DatabaseConnection($config->db['host'], $config->db['username'], $config->db['password']);
} catch(ConnetionFailedException) {
    //connection failed
}
//connection success


Convert Extension methods to generic methods I was looking at some Linq queries with multiple join statements. I was thinking that they could be cleaned up using extension methods that hide away the join / where statements.

For example:

(from myObject in _context.MyObject
join myObjectGroupName in _context.MyObjectGroupNames on myObject.TaskGroupNameKey equals myObjectGroupName.ReportGroupNameKey
join myObjectResourceType in _context.MyObjectResourceTypes on myObject.ResourceKey equals myObjectResourceType.ResourceKey
where myObjectGroupName.ReportGroupName.Trim().ToUpper().Equals(groupName.Trim().ToUpper()) && fileTypes.Contains(myObjectResourceType.ResourceType)
select myObject.PermissionKey);

Now becomes:

_context.MyObject.JoinWhere(_context.MyObjectGroupNames, groupName)
                 .JoinWhere(_context.MyObjectResourceTypes, fileTypes)
                 .Select(x => x.PermissionKey);

Using:

public static IQueryable<MyObject> JoinWhere(this IQueryable<MyObject> initialObject, IQueryable<MyObjectGroupName> customerReportGroupNames, string groupName)
{
    return (from myObject in initialObject
        join myObjectGroupName in customerReportGroupNames on myObject.TaskGroupNameKey equals myObjectGroupName.ReportGroupNameKey
        where myObjectGroupName.ReportGroupName.Trim().ToUpper().Equals(groupName.Trim().ToUpper())
        select myObject);
}

public static IQueryable<MyObject> JoinWhere(this IQueryable<MyObject> initialObject, IQueryable<MyObjectResourceType> myObjectResourceTypes, IEnumerable<string> types)
{
    return (from myObject in initialObject
        join myObjectResourceType in myObjectResourceTypes on myObject.ResourceKey equals myObjectResourceType.ResourceKey
        where types.Contains(myObjectResourceType.ResourceType)
        select myObject);
}

After more consideration I was wondering if it would be possible to make these methods generic:

public static IQueryable<T> JoinWhere(this IQueryable<T> initialObject, IQueryable<TJoinOn> joinOn, IEnumerable<string> types)
{
    return (from myObject in initialObject
                                               //Here                      //Here
        join myObjectResourceType in joinOn on myObject.ResourceKey equals myObjectResourceType.ResourceKey
                             //Here
        where types.Contains(myObjectResourceType.ResourceType)
        select myObject);
}

Where I have put my comments are specific to specific objects. Could those be used generically or would I have to create an extension method for each dataObject making this WAY more work than it's really worth?

Have you considered using in Interface on MyObject?

public static IQueryable<T> JoinWhere(this IQueryable<T> initialObject, IQueryable<TJoinOn> joinOn, IEnumerable<string> types)
where T : IMyObject
{
    return (from myObject in initialObject
                                               //Here                      //Here
        join myObjectResourceType in joinOn on myObject.ResourceKey equals myObjectResourceType.ResourceKey
                             //Here
        where types.Contains(myObjectResourceType.ResourceType)
        select myObject);
}


Hash table using open addressing with double hashing probing Looking for a general review:

#ifndef HASH_TABLE_HASH_TABLE_H
#define HASH_TABLE_HASH_TABLE_H

#include <utility>
#include <stdexcept>

namespace mds {
    template<typename K, typename V >
    class hash_table {

        std::size_t     M;
        std::pair<K, V> *table;
        bool            *mark;
        std::size_t     N;

        std::size_t hash(std::size_t h, std::size_t i) const {
            auto hash2 = (std::hash<std::size_t>()(h) % (M - 1));
            return ((h % M) + i * (hash2 | 0x1)) % M;
        }

        std::size_t power_of_2_ceiling(std::size_t x) const {
            auto bits = sizeof(std::size_t) << 3;
            --x;
            for (auto i = 1u; i != bits; i <<= 1) {
                x |= x >> i;
            }
            return x + 1;
        }

    public:

        hash_table() : hash_table(0) { }
        hash_table(std::size_t m)
            : M(power_of_2_ceiling(m)),
            table(new std::pair<K, V>[M]),
            mark(new bool[M]), N(0)
        {
            for (std::size_t i = 0; i < M; ++i) {
                mark[i] = false;
            }
        }

        hash_table(hash_table<K, V>&& map)
            : M(map.M), table(map.table),
              mark(map.mark), N(map.N)  {

            map.M = map.N = 0;
            map.mark  = nullptr;
            map.table = nullptr;
        }

        hash_table(const hash_table<K, V>& map)
            : M(map.M), table(new std::pair<K, V>[map.M]),
            mark(new bool[map.M]), N(map.N) {

            for (std::size_t i = 0; i < M; ++i) {
                mark[i] = map.mark[i];
                table[i] = map.table[i];
            }
        }

        hash_table(std::unordered_map<K, V>&& map)
            : M(power_of_2_ceiling(map.size())),
            table(new std::pair<K, V>[M]),
            mark(new bool[M]), N(0)
        {
            for (std::size_t i = 0; i < M; ++i) {
                mark[i] = false;
            }
            for (auto &p : map) {
                insert(std::move(p));
            }
        }

        hash_table<K, V>&  operator=(hash_table<K, V> other) {
            swap(*this, other);
            return *this;
        }

        friend void swap(hash_table<K, V>& first, hash_table<K, V>& second) noexcept {
            using std::swap;

            swap(first.M, second.M);
            swap(first.N, second.N);
            swap(first.table, second.table);
            swap(first.mark, second.mark);
        }

        ~hash_table() {
            delete[] table;
            delete[] mark;
        }

        const std::pair<K, V>* find(const K& key) const {
            auto hash_code = std::hash<K>()(key);
            auto i = 0u;
            auto h = hash(hash_code, i);
            while (i < M && mark[h]) {
                if (table[h].first == key ) {
                    return &table[h];
                }
                h = hash(hash_code, ++i);
            }
            return nullptr;
        }

        bool insert(std::pair<K, V>&& p) {
            auto hash_code = std::hash<K>()(p.first);
            for (std::size_t i = 0; i < M; ++i) {
                auto h = hash(hash_code, i);
                if (!mark[h]) {
                    table[h] = std::move(p);
                    ++N;
                    return mark[h] = true;
                }
                else if (table[h].first == p.first) {
                    return false;
                }
            }
            throw std::overflow_error("overflow");
        }

        bool constains(const K& key) const {
            return find(key) != nullptr;
        }

        std::size_t size() {
            return N;
        }
    };
}

#endif // !HASH_TABLE_HASH_TABLE_H

Here are a few things I noticed reading your code:

You should probably use more descriptive identifiers for M and N here.
Instead of dynamic allocation you could also use std::vector<...> for table and mark. That would allow to simplify your code considerably. For example you could drop the custom copy/move constructors/assignment operators and the destructor and use the implicitly defined ones instead.
You could also use the provided .size() method of std::vector to get M instead of saving it explicitly.

If you didn't use std::vector for performance considerations, then I doubt that is a problem. Element access is through one indirection anyway and you do not have resize operations. Simply use reserve in your constructor to allocate the proper size immediately. Only the required memory for std::vector might be a bit larger (additional integer for current size/reserved size and doubling of length M).

Even if you still don't want to use std::vector, then you should consider writting an additional class (heap_array?) to handle the memory management or you could use std::unique_ptr, which would at least make your custom move constructor/assignment operator and the destructor redundant.
In hash_table(std::size_t m): The body is simply zero-initialization of the mark array. The same can be achieved with:

hash_table(std::size_t m)
    : M(power_of_2_ceiling(m)),
    table(new std::pair<K, V>[M]),
    mark(new bool[M]{}), N(0)
{ }

In hash_table(hash_table<K, V>&& map): Is there a reason to set M and N to zero? A moved object only needs to be properly destructable, but the destructor does not use M or N.
You can refer to the current instantiation of a template class without specifying the arguments, i.e. hash_table(hash_table&& map) would be fine for the move constructor, too.
To use std::unordered_map you need to include <unordered_map>.
hash_table(std::unordered_map<K, V>&& map): You have a rvalue reference constructor here, but none for const references. That seems inconsistent.
hash_table(std::unordered_map<K, V>&& map) initially does the same as hash_table(std::size_t), you could simply call it:

hash_table(std::unordered_map<K, V>&& map)
    : hash_table(map.size())
{
    for (auto &p : map) {
        insert(std::move(p));
    }
}

(dropped)
(dropped)
You could use just one array of type std::tuple<K,V,bool> (or a custom struct) instead of two arrays table and mark. Then you would not have to repeat the memory management code twice, and with the access pattern related data is closer in memory. Together with my point 2. this would reduce overhead even more.
Your only method to return elements find returns a const std::pair<K,V>*, implying that it is impossible to modify the value of an entry in-place. I think that is a rather strong limitation. Of course one may not change the key in-place, but the value should be modifiable. Therefore the type should be std::pair<const K,V>* and table also should be of type std::pair<const K,V>.
The maximal size of your hash_table is kind of arbitarily chosen and once it is full there will be an exception thrown. This might not have been the scope of your intentions but really the size should grow when a certain load is reached and rehashing should happen.
Your insert takes a rvalue reference. This means that only temporaries or moved elements can be inserted. There should also be an overload for const references (and without move).
size() should be const.