Retrieving a list of patients that have visited certain hospitals I have a table of patients that has a field for which hospitals they visited.  I want to retrieve a list of patients that have visited Green Hospital and have also visited other hospitals. Is this query accurate?

SELECT COUNT (DISTINCT PatientID) FROM Patients
WHERE PatientID IN (
        SELECT PatientID FROM Patients WHERE Facility = 'Green'
) AND Facility <> 'Green'

My logic is that the internal statement will get all patients that have visited Green Hospital.  The external will find the intersect of those that visited somewhere else.  I can't think of any way to validate my results which is why I'm posting here.

The DBMS is actually Intersystems Caché.

Using IN tends to have a performance hit in most DBMS, as it typically results in a row by row lookup.  As a rule of thumb, I don't use a subquery if I can express the relationship in a simple join.  I also try to stay away from DISTINCT wherever possible.  When I need to dedup records, I'll typically use a GROUP BY and return the count of records, since it's often useful in my field (reporting) and/or for QA.  For example, the below query should return the patients you are looking for, with the added benefit of how many other hospitals were visited.  You could also do a plus one for total hospitals visited (including "Green").  That may or may not be useful for your needs.

SELECT
    green.PatientID,
    COUNT(1) AS OtherHospitals,
    COUNT(1)+1 AS TotalHospitals
FROM
    Patients green
JOIN
    Patients other ON other.PatientID = green.PatientID
WHERE
    green.Facility = 'Green'
    AND other.Facility <> 'Green'
GROUP BY
    green.PatientID


Fastest method of randomly swapping array indexes / generating random alphabets in C++ std::array < char, 27 > new_alphabet() {
  std::array < int, 26 > a;

  static std::random_device rd;
  static std::mt19937 mte(rd());

  std::uniform_int_distribution < int > dist(0, 25);

  std::generate(a.begin(), a.end(), [ & ]() {
    return dist(mte);
  });

  std::array < char, 27 > alpha = {
    "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
  };

  for (int x = 0; x < 26; x++) {
    std::swap(alpha[x], alpha[a[x]]);
  }

  return alpha;
}

int main() {
  for (int i = 0; i < 10000000; i++) {
    new_alphabet();
  }

  return 0;
}

The above code works fine and creates randomised alphabets, each loop would generate for example:
Y G T X R P D U L H I M V O B E F Z Q W J K A C S N
G Q I J F O P L A K M B D T R C H V Y X S U Z E N W
B O N U C Y K T H Q J F V W L M E S X A P R D Z I G
Q B D C V Z R E W O P L M S N X T H A I K U J Y G F
R J B G M E Z P V Q D Y C H I S O F X L N K T U A W
A M E S D H Q J R K Y P N X G T F B L O U W I Z V C
H E L G M F Q K J A X R S I V N W P T D O Y Z U B C
V S O B F R K A J D H Q T P N E I M X U Y Z C L W G
D E M R P Y S K G O C B A Q I H L J T U F N V X Z W
...

Generating 10,000,000 alphabets takes ~5 seconds on my 2.6 GHz i7. Compiling with g++ -std=c++11 -O3.
Is this the most efficient approach to this, and if there any optimisations, how can I improve or speed up this code?
The above uses the Mersenne Twister engine for generating 26 random values used to swap the array indexes.
Would it be possible to use an xorshift generator here?

This method is biased. Use Fisher-Yates shuffle.
TL;DR: There are \$26^{26}\$ possible outcomes of a. There are \$26!\$ possible permutations of the alphabet. Since \$26!\$ doesn't evenly divide \$26^{26}\$, permutations are not equiprobable.

Regarding speed, I don't think that generating a beforehand is beneficial. It results in 3 memory fetches down the line: alpha[x], tmp = a[x], alpha[tmp]. Generating a random number inside the loop makes only two fetches. It is likely to not be measurable with an alphabet as short as 26 characters, but when shuffling a billion-strong alphabet you'd see the difference.


Voting system for a website I am currently adding a voting system to my website. Below is my voting class. I feel that it could be simplified and that there is repeated code regarding the functions, although to the best of my knowledge this is the simplest I can make it. I would love to hear other implementation ideas and systems that have been used.

<?php

class Vote{

    var $db;

    function __construct($db){
        $this->db = $db;
    }

    function vote($user_id, $post_id, $type){
        // check if vote already exists against user and post to prevent duplicates
        if($this->userVoteExists($user_id, $post_id)){
            // retreive vote
            $vote = $this->getVote($user_id, $post_id);
            // check  if the requested vote is the same to prevent needlessly deleting and inserting
            if($vote['type'] != $type){
                if($vote){
                    // remove the vote if it exists
                    $this->removeVote($user_id, $post_id);
                }
                // add the vote
                $this->addVote($user_id, $post_id, $type);
            }
        }
    }

    function userVoteExists($user_id, $post_id){
        $query = $this->db->prepare("SELECT * FROM votes WHERE user_id = :user_id AND post_id = :post_id LIMIT 1");
        $query->execute(array(':user_id' => $user_id, ':post_id' => $post_id));
        return $query->rowCount();
    }

    function getVote($user_id, $post_id){
        $query = $this->db->prepare("SELECT * FROM votes WHERE user_id = :user_id AND post_id = :post_id LIMIT 1");
        $query->execute(array(':user_id' => $user_id, ':post_id' => $post_id));
        return $query->fetch();
    }

    function removeVote($user_id, $post_id){
        $query = $this->db->prepare("DELETE FROM votes WHERE user_id = :user_id AND post_id = :post_id");
        $query->execute(array(':user_id' => $user_id, ':post_id' => $post_id));
    }

    function addVote($user_id, $post_id, $type){
        $timestamp = time();
        $query = $this->db->prepare("
        INSERT INTO votes (
            post_id,
            user_id,
            type,
            timestamp
        ) VALUES (
            :post_id,
            :user_id,
            :type,
            :timestamp
        )");
        $query->execute(array(
            ':post_id' => $post_id,
            ':user_id' => $user_id,
            ':type' => $type,
            ':timestamp' => $timestamp
        ));
    }

}
?>

A Vote should not have access to the database. It should be a dumb 'entity' with setters, getters and a constructor to make the required object a valid one.

You should take a look at the Repository and Data Mapper patterns. The point is, the object you want to save should not know how it is saved - meaning that you can change where you want to save it's data in the future without touching the underlying entity.

For an example implementation of this, check out Doctrine ORM. The Repository is the the object that uses a data mapper to save data to the relevant data source. Change the data source, the repository object-api (how you save / retrieve data) stays the same, but how the data is saved changes. Pretty basic concept but really powerful, and not just limited to ORMs or PHP, either. Imagine entities in JavaScript with a data mapper for local/session storage, for example. The data mapper would have get, update, delete (etc) methods, and the repository would call the underlying data mapper (relying on an interface) to actually perform the saving / retrieving of data, which you can swap out at any time.

Also, our vote needs to have it's own id (unique, primary key) completely separate from every other attribute so it can be uniquely identified. Aside from that, all lowercase, underscores for spaces and prepared statements is a good thing.

I'm not going to write your code for you, but effectively, you need to take a look at doing the following:

Abstract out the thing that saves, updates and removes data from your persistence. This is your data mapper, it is stand-alone, and just takes data and maps it to your given persistence. This is the object that would have your instance of \PDO dependency injected (yes, typehint for it).
Have a class that takes an instance of your data mapper that is responsible for saving your vote (a VoteRepository). You don't need an ORM, but you can simulate some of it's useful abstraction to help with SoC. This class contains your object-specific stuff which it forwards to it's data mapper, with your methods like userHasVote(User $user, $voteId).
You could probably get away with putting addVote(Vote $vote) and removeVote(Vote $vote) in your repository. This will require you to construct a new Vote object before you save it to the database (a Factory will help here, and your repository will give you a Vote object when you request one from the database. Remember, you're trying to use OOP here: everything is an object with a single responsibility.
The logic to remove / add a vote depending on something I would personally put in a Voting component (name it something better), with this single responsibility within it. Also, read the difference between a component and a service. Responsibility wise, you need to work upwards from the datasource, building a nice object api for each object without having it concern others (so they can work standalone)
Learn how to document your code. Phpdoc is a standard everyone and their dog's framework are employing to help other developers (and automated documentation tools) understand what code does
You're not writing php 4 any more. var has no place in your code. Use the correct access modifier, as in protected $db. The same goes for your functions (methods), make the ones you want others to consume (the public api) public function name instead.

So, all in all, a lot of refactoring to be done here. Well done for using \PDO and prepared statements though.


Saving enum value using name() Let's assume I have an enum type

enum ComponentState {
    TURNED_OFF,
    TURNED_ON,
    SUSPENDED,
    TO_REPAIR;
}

This enum describes a state of some component. Now let's assume I want to export my component to as XML file. Then, of course, I'm going to import my component. So:

//Export:
componentElement.setAttribute("state", getState.name());  // results in <element state="TURNED_ON">

//Import
try {
   state = ComponentState.valueOf(element.getAttributeValue("state"));
} catch (IllegalArgumentException e) {
   //...
}

Everything is fine, but problems starts when I change the name of some enum constants, because it sounds better:

enum ComponentState {
    TURNED_OFF,
    TURNED_ON,
    SLEEPING,   // <- the same sense, but
    CRASHED;    // <- different name (PM's "I want to have")
}

Note, that doing this I'm loosing a backward compatibility.

Question:
How do you cope normally with such situations?

1

public String getXMLValue(){
    // ... implementation independent from name()
}

and

static ComponentState parseXMLString(String s) {
    // ... checking if s matches any name()
}

OR

2

My approach is OK - it should not be expected, that the name on enum will change just because it sounds better.

I like rolfl's approach, but I have one thing to add which would make it more compact and flexible:

Use a private constructor to provide a custom save value.

The biggest benefits of using either my or rolfl's approach is that you don't need to remember to add new enum values to the HashMap. In my approach especially, it gives you a clear overview of the enum values with their corresponding save values. As long as you only use one constructor (which I highly recommend) it is impossible to forget to give a save value for the enum.

This approach also allows you to customize their save-values much easier. You can rename your enums all you want, all you need to remember is to not change the saveValue, unless you want incompatibility issues.

enum ComponentState {
    TURNED_OFF("off"),
    TURNED_ON("on"),
    SUSPENDED("susp"),
    TO_REPAIR("repair");

    private final String saveValue;

    private ComponentState(String saveAs) {
        this.saveValue = saveAs;
    }

    private static final Map<String, ComponentState> FUZZY = buildMap();

    private static Map<String, ComponentState> buildMap() {
        Map<String, ComponentState> mapto = new HashMap<>();
        for (ComponentState state : ComponentState.values()) {
            mapto.put(state.saveValue, state);
        }
        return mapto;
    }

    private static final ComponentState fuzzyValueOf(String name) {
        return FUZZY.get(name);
    }

    public String getSaveValue() {
        return this.saveValue;
    }
}

If you want to have multiple possible save values for the same enum, then use a String... parameter: (However, in this case you need to make sure that you don't use an empty constructor for a field, as that wouldn't give it a specified save value)

    private final String[] saveValues;
    private ComponentState(String... saveAs) {
        this.saveValues = saveAs;
    }
    public String getSaveValue() {
        return this.saveValues[0];
    }

I really can't recommend using the .ordinal approach as ratchet freak suggested, even though it is nice and tidy, consider this enum:

Suit {
    DIAMONDS, HEARTS, SPADES, CLUBS;
}

Now what if you'd like to re-order them?

Suit {
    CLUBS, SPADES, DIAMONDS, HEARTS;
}

Congratulations, spades has become diamonds, clubs is hearts. Down is up and up is down. Logic goes on ski vacation with it's buddy reason.

If you use the .ordinal approach, remember to never ever ever switch places of them. (Me, talking from experience? What makes you think that?)


Process CSV files that follow the first nonempty one I have a list of CSV files (all have the same fields), some of which are empty. My goal is to find the first nonempty file and use that as the "base" file for an aggregation. After finding the base file, the way I will consume the rest of the files changes, so I need to maintain the index of the file. Here is what I currently have:

def f(list_of_files):
    aggregated_file = ""
    nonempty_index = -1
    for index, file in enumerate(list_of_files):
        aggregated_file = extract_header_and_body(file)
        if aggregated_file:
            nonempty_index = index
            break

    if nonempty_index != -1:
        rest_of_files = list_of_files[nonempty_index:]
        for file in rest_of_files:
            aggregated_file += extract_body(file)

    return aggregated_file

Both extract_header_and_body and extract_body return string representations (with and without column names, respectively) of the CSV files -- if the file is empty, the empty string is returned. This seems like a clunky solution, especially for Python. Is there a more concise/readable way to accomplish this?

If we reformulate this as trying to extract the header + body of the first non-empty file and the body of all subsequent files this comes to mind:

aggregated_file = ''
for file in list_of_files:
    if aggregated_file:
        aggregated_file += extract_body(file)
    else:
        aggregated_file += extract_header_and_body(file)


React app for movie search, making AJAX call with input value from another component I am building a movie search React app using themoviedb.org API. To make ajax call to pull the list of movies I need to get input value as a variable and feed to the url, but not sure how to fetch a value that belongs to another component.

I've made an extensive online search, but they mainly refer to the case when it happens inside same component, and using ref is discouraged.

So what would be the best way to organize an app in React way, keeping components uncoupled yet able to feed input value to url of another component?

   import React from 'react';
   import './App.css';
   import axios from 'axios';

class SearchForm extends React.Component {
  constructor(props) {
    super(props);
    this.state = {value: ''};

    this.handleChange = this.handleChange.bind(this);
    this.handleSubmit = this.handleSubmit.bind(this);
  }

  handleChange(event) {
    this.setState({value: event.target.value});
  }

  handleSubmit(event) {
    console.log("state value is " + this.state.value);

    var searchValue = this.movieName.value;
    console.log("ref value is "+ searchValue)

    event.preventDefault();
  }

  render() {

    return (
      <form onSubmit={this.handleSubmit}>

          Name:
          <input className="movieName" type="text" ref={(input) => { this.movieName = input; }} value={this.state.value} onChange={this.handleChange} />

        <input type="submit" value="Submit" />

{this.state.value}

      </form>
    );
  }
}

class App extends NameForm{  /* I am extending NameForm to get access to input value, but probably coupling components too tight */
    constructor(props) {
      super(props);
      this.state ={
        movie:[]
      };
    }

componentDidMount() {

  let searchInput = "land"; /* This should be from SearchForm's value */

let sortByPop = "&sort_by=popularity.desc";
let requestUrl = 'https://api.themoviedb.org/3/search/movie?api_key=f8c4016803faf5e7f424abe98a04b8d9&query=' + searchInput + sortByPop;

  axios.get(requestUrl).then(response => {
      this.setState({movie: response.data.results})
  });

}

render() {

 let baseImgURL = "https://image.tmdb.org/t/p/w185_and_h278_bestv2";
 let posterImgPath = this.state.movie.map(movie => movie.poster_path);

 let posterLink = baseImgURL + posterImgPath;

  return(

       <SearchForm />

          {this.state.movie.map(movie =>

           <img className="posterImg" src= {`https://image.tmdb.org/t/p/w185_and_h278_bestv2/${movie.poster_path}`} alt={movie.title} />

{movie.title}

           )}

  )
}

}

export default App;

So the way you would accomplish this in my experience is to move your state up your component tree to a higher level where each component that requires access to that state can have it via props that you pass in.

So for example your SearchForm component could accept all the values and functions it requires via props. It needs a prop to handle the change of the search input as the user types, a prop to handle the submit as the user clicks the submit button and a prop to show the search input value in the input box.

So, for example:

<SearchForm
    searchInput={this.state.searchInput}
    handleChange={(event) => this.setState({ searchInput: event.target.value })}
    handleSubmit={() => this.searchForMovie(this.state.searchInput)}
/>

That means your SearchForm could become something more like:

const SearchForm = (props) => (
  <form onSubmit={props.handleSubmit}>
    <label>
      Name:
      <input
        className="movieName"
        type="text"
        value={props.searchInput}
        onChange={props.handleChange}
      />
    </label>
    <input type="submit" value="Submit" />
    <h1>{props.searchInput}</h1>
  </form>
);

All your state lives in your App component and now you can use that state to pass on as props to any other child components that may need it.

Some observations/suggestions:

As you can see above there is a searchForMovie() function, I would extract the logic from your componentDidMount hook to a function like this so you can call it whenever needed with the search input you desire
Also consider some error handling on your axios.get() function call as if anything goes wrong with the request you won't know about it because the promise will swallow that (can be confusing when debugging)
The above does not consider any performance optimisations and the amount of renders/rerenders on a large app may be problematic, but my guess is that would be premature for your app and important is first that you get it to work
For your App class I would advise against extending any other class aside from React.Component as that is all you require (if I understand your question correctly)
Also, if you are using this in your class methods you will need to remember to bind them in your constructor to avoid hitting the unknown function error, i.e. this.searchForMovie = this.searchForMovie.bind(this);


Wrapping a telephony RESTful service for sending text messages and making phone calls So I'm writing an abstraction layer that wraps a telephony RESTful service for sending text messages and making phone calls. I should build this in such a way that the low-level provider, in this case Twilio, can be easily swapped without having to re-code the higher level interactions.

I'm using a package that is pre-built for Twilio and so I'm thinking that I need to create a wrapper interface to standardize the interaction between the Twilio service package and my application. Let us pretend that I cannot modify this pre-built package.

<?php

namespace Telephony;

class Provider_Twilio implements Provider_Interface {

    public function send_sms(Provider_Request_SMS $request) {

        if (!$request->is_valid())
            throw new Provider_Exception_InvalidRequest();

        $sms = \Twilio\Twilio::request('SmsMessage');
        $response = $sms->create(array(
            'To' => $request->to,
            'From' => $request->from,
            'Body' => $request->body
        ));

        if ($this->_did_request_fail($response)) {
            throw new Provider_Exception_RequestFailed($response->message);
        }

        $response = new Provider_Response_SMS(TRUE);
        return $response;

    }

    private function _did_request_fail($api_response) {
        return isset($api_response->status);
    }

}

The idea is that I can write another file like this for any other telephony service provided that it implements Provider_Interface making them swappable. Here are my questions:

Do you think this is a good design? How could it be improved?
I'm having a hard time testing this because I need to mock out the Twilio package so that I'm not actually depending on Twilio's API for my tests to pass or fail. Do you see any strategy for mocking this out?

In circumstances like you have here; the almost lowest level layer wrapping a dependency, it can be hard to see another layer lower to allow abstraction, but remember this: The lowest level wrapper of a dependency is a literal straight pass-through.

This means, if you take the twilio api and create a direct correlary that simply passes-through everything to the twilio api, then that wrapper is the lowest level wrapper and therefore bears no need of testing at the unit level (testing at the integration and build-verification levels however is worth while) as it has no actual functionality.

If you create such a wrapper, you may then pass in a mock of it to your next-level-up which is your provider here.

So i guess there are two rules I'm stating here, and glad to hear people give points for or against:

The lowest level wrapper possible of external dependencies is a direct pass-through (which feels stupid and tedious when writing it, but actually serves a purpose)
The lowest level wrapper of an external dependency has no need of testing at the unit level as it maintains no testable functionality, but should be tested at the integration and build-verification levels as well as all external dependencies should.

Also, for your particular case and similar ones, you have an obvious default implementation of the dependency that you will be wrapping so feel free to have a default constructor so its consumer's do not need to construct the twilio wrapper, while maintaining the ability for the twilio wrapper to be settable for the unit tests. Sometimes making the accessibility of these setters protected enough that consumers will not think it's ok to change it, while accessible enough for proper unit testing can be tricky; I tend to treat those situations with a use your best-judgement approach.

Edit:
I would unit test the Provider_Interface. Though that's one man's opinion, many people would say unit testing at this level is stupid, I maintain that where there is functionality, which your Provider_Interface has, there may be bugs. You could have a typo in 'SmsMessage' for instance, and therefore should unit test for these things. However I acknowledge there is much disagreement in industry about the ROI on unit testing at this level, or maintaining pass-through wrappers for such purpose as I am advocating.


Google Codejam 2016 Round 3 Problem A I'm learning Rust by completing a variety of coding challenges, such as the Google Codejam problem stated in the title.  I completed a Python 3 implementation that I liked, which I then tried to port over to Rust.

To my surprise, the Python implementation appeared to be considerably faster: when I ran time python3 ../a.py ../A-large-practice.in > /dev/null, I got times of roughly .075s for each Python run as compared to when I ran time cargo run --release ../A-large-practice.in > /dev/null for which I got roughly .210s for each Rust run.  These times come from averaging the sums of the usr and sys outputs generated by running the time command around 10 times per language.

How can I improve my Rust implementation so that it better conforms to best practices and also ends up being faster (at least to a level comparable with the Python implementation)?

use std::env::args;
use std::fs::File;
use std::io::Read;

fn main() {
    let args: Vec<String> = args().collect();
    let mut input = String::new();
    let mut f = File::open(&args[1]).unwrap();
    f.read_to_string(&mut input).unwrap();

    let mut input = input.lines();
    let num_inputs: u32 = match input.next() {
        None => panic!("No contents."),
        Some(num) => {
            match num.trim().parse() {
                Ok(num) => num,
                Err(_) => panic!("First line in input not a number."),
            }
        }
    };

    for i in 0..num_inputs {
        let mut score = 0;
        let moods: Vec<char> = match input.next() {
            None => panic!("Missing content."),
            Some(mood) => mood.chars().collect(),
        };
        let mut problems: Vec<&char> = Vec::new();
        for (j, mood) in moods.iter().enumerate() {
            /*
             * the assumption is that you'll only request problems that're the same mood that the grader is in
             * reason being that you can receive a maximum of 10 points and a minimum of 5,
             * whereas for the other mood it'll be a max of 5 and a min of 0
             */
            if problems.is_empty() {
                problems.push(mood);
            } else if problems.len() >= moods.len() - j {
                if problems.last() == Some(&mood) {
                    score += 10;
                } else {
                    score += 5;
                }
                problems.pop();
            } else if problems.last() == Some(&mood) {
                score += 10;
                problems.pop();
            } else {
                problems.push(mood);
            }
        }
        println!("Case #{}: {}", i+1, score);
    }
}

There's no need to collect all the arguments into a vector to ignore all but one. Just grab that one via nth.
Remove most of the explicit types and prefer inferring them instead.
Use expect instead of unwrap to provide a message when it fails. It took me 10 minutes to realize that the first argument had to be a filename and not the number of lines to read from standard input.
It's shorter to use expect instead of a panic! when you can.
Don't provide full type for collect; let the inner type be inferred with Vec<_>.
Use the iterator in the for loop instead of calling next manually.
Extract functions to give structure, add names, and isolate mutability.

use std::env::args;
use std::fs::File;
use std::io::Read;

fn read_input_file(name: &str) -> String {
    let mut input = String::new();
    let mut f = File::open(&name).expect("Unable to open the input file");
    f.read_to_string(&mut input).expect("Unable to read the input file");
    input
}

fn main() {
    let filename = args().nth(1).expect("Provide an input file as an argument");

    let input = read_input_file(&filename);
    let mut input = input.lines();

    let num_inputs = input.next().expect("No contents");
    let num_inputs = num_inputs.trim().parse().expect("First line in input not a number");

    for (i, line) in input.take(num_inputs).enumerate() {
        let mut score = 0;
        let moods: Vec<_> = line.chars().collect();
        let mut problems = Vec::new();

        for (j, &mood) in moods.iter().enumerate() {
            // the assumption is that you'll only request problems
            // that're the same mood that the grader is in reason
            // being that you can receive a maximum of 10 points and a
            // minimum of 5, whereas for the other mood it'll be a max
            // of 5 and a min of 0

            if problems.is_empty() {
                problems.push(mood);
            } else if problems.len() >= moods.len() - j {
                if problems.last() == Some(&mood) {
                    score += 10;
                } else {
                    score += 5;
                }
                problems.pop();
            } else if problems.last() == Some(&mood) {
                score += 10;
                problems.pop();
            } else {
                problems.push(mood);
            }
        }
        println!("Case #{}: {}", i+1, score);
    }
}

When profiling, it is very important to compile in release mode. You will also want to make sure you aren't including compilation time. I tend to split the compile and execute steps into two parts to be sure nothing extra is included:

$ cargo build --release
$ time ./target/release/gc3 A-large-practice.in

# ...snipped output...

real    0m0.022s
user    0m0.010s
sys     0m0.010s


PHP Factory Pattern : Should I use `create()` or `__invoke()`? I currently had a colleague reviewing my code and he had a comment on a factory class I created. It's very simple:

class ResponseFactory
{
    public function create(array $curlInfo, array $rawResponse): ResponseInterface
    {
        return new Response($curlInfo, $rawResponse);
    }
}

$responseFactory = new ResponseFactory();
$response = $responseFactory->create(...);

His comment was that I should use the __invoke()-method of PHP. In that case, the code would look like this:

class ResponseFactory
{
    public function __invoke(array $curlInfo, array $rawResponse): ResponseInterface
    {
        return new Response($curlInfo, $rawResponse);
    }
}

$responseFactory = new ResponseFactory();
$response = $responseFactory(...);

Now I am wondering: is this just a matter of taste? Or is there real benefit in using __invoke() over a public create()-method?

Any thoughts or insights on this matter are more than welcome. I'm really trying to find good use cases for the __invoke()-method, but I'm not sure if a factory is one of them.

The only benefit of using __invoke() is that you can store the instance of the class in a variable as a callback and then use it without call_user_func_array(). But in my opinion using create() is more readable so you can guess easily what is the code doing.


First prime number larger than given integer How can I test this C program for "efficiency"? The most interesting usage is that it returns negative output for large enough input, otherwise the behavior is about expected. Will you suggest how to improve the code, both the ui used in main that should be both efficient and preferably platform-indepedent and not use too much libraries for portability and optimization. I think the obvious improvement that I can make is recoding the int to unsigned int:

/*
 * NextPrime
 *
 * Return the first prime number larger than the integer
 * given as a parameter. The integer must be positive.
 */
#define PRIME_FALSE   0     /* Constant to help readability. */
#define PRIME_TRUE    1     /* Constant to help readability. */
#include <stdio.h>
#include <stdlib.h>
int nextprime(int inval) {
    int perhapsprime; /* Holds a tentative prime while we check it. */
    int testfactor; /* Holds various factors for which we test perhapsprime. */
    int found; /* Flag, false until we find a prime. */

    if (inval < 3) /* Initial sanity check of parameter. */
    {
        if (inval <= 0)
            return (1); /* Return 1 for zero or negative input. */
        if (inval == 1)
            return (2); /* Easy special case. */
        if (inval == 2)
            return (3); /* Easy special case. */
    } else {
        /* Testing an even number for primeness is pointless, since
         * all even numbers are divisible by 2. Therefore, we make sure
         * that perhapsprime is larger than the parameter, and odd. */
        perhapsprime = (inval + 1) | 1;
    }
    /* While prime not found, loop. */
    for (found = PRIME_FALSE; found != PRIME_TRUE; perhapsprime += 2) {
        /* Check factors from 3 up to perhapsprime/2. */
        for (testfactor = 3; testfactor <= (perhapsprime >> 1) + 1; testfactor
                += 1) {
            found = PRIME_TRUE; /* Assume we will find a prime. */
            if ((perhapsprime % testfactor) == 0) /* If testfactor divides perhapsprime... */
            {
                found = PRIME_FALSE; /* ...then, perhapsprime was non-prime. */
                goto check_next_prime;
                /* Break the inner loop, go test a new perhapsprime. */
            }
        }
        check_next_prime: ; /* This label is used to break the inner loop. */
        if (found == PRIME_TRUE) /* If the loop ended normally, we found a prime. */
        {

            return (perhapsprime); /* Return the prime we found. */
        }
    }

    return (perhapsprime); /* When the loop ends, perhapsprime is a real prime. */
}

int main(int argc, char *argv[]) {
    int intvar;
    if (sscanf(argv[1], "%i", &intvar) != 1) {
        printf("please use an integer parameter to compute the next prime\n");
    } else
        printf("%d => %d\n", intvar, nextprime(intvar));
    return 0;
}

Things you could improve

Efficiency

As @rolfl stated, you want to use a Sieve of Eratosthenes (+1 to that answer).

I can't see if you are doing this already, but you should be compiling with your compiler's highest optimization level.  With GCC for example, this would be -O3.

Portability

Keep in mind to adhere to the standards as closely as possible, because a system that supports C must adhere to the standards as well.
If you want your program to be as portable as possible, take a look into the AutoTools suite for packaging your program.  This is a bit extreme in your case, since you aren't using external libraries, and your program doesn't have a lot of system dependencies.

Standards

You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard  knows how frequently this is used, and lets you not bother.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a
  value of 0.

Use the standard library <stdbool.h> instead of defining your own boolean values.

#define PRIME_FALSE   0     /* Constant to help readability. */
#define PRIME_TRUE    1     /* Constant to help readability. */

It still helps with readability.  And since it is part of the standard in C, systems are guaranteed to have it if they support C.

Syntax/Styling

Don't use goto.

Yes, there are some rare situations where you may find it necessary to use it.  This is not one of them.  A simple break; will suffice in it's place, since you are only breaking out of the inner loop.
Right now you are using a somewhat odd way to test if you have an integer input into the command line arguments.

if (sscanf(argv[1], "%i", &intvar) != 1)

Use argc to test the number of arguments input into the program instead, and isdigit to test if the input contains numbers.

if (argc != 2 || !isdigit(**(argv+1))) printf("Please use an integer parameter to compute the next prime.\n");

Then convert that input into an integer.

int intvar = atoi(*(argv+1));

Note that the order I put it in is important.  We want to test argc first, so we put it first in the test conditional.  If that fails, the second part on the other side of the || isn't even evaluated, and the statements following the conditional are executed.  If we switched those tests around, undefined behavior could occur if we didn't enter in a command line argument.
Use puts() instead of printf() when you aren't formatting your strings.  This allows you to not worry about the newline (\n) character at the end of your printed out strings.

puts("Please use an integer parameter to compute the next prime.");

You don't modify the value of intval within your nextprime() function, therefore the parameter should be declared constant.

int nextprime(const int inval)

You can reduce down one of your test conditionals.

if (inval == 1)
    return (2); /* Easy special case. */
if (inval == 2)
    return (3); /* Easy special case. */

if (inval == 1 || inval == 2) return (inval + 1); /* Easy special case. */


Scripting language parser I'm making a scripting language parser and it's shaping up well but it takes about 30ms to do 30 or so odd/even checks.

Profiler tells me that it's taking most of the time in the InvokeOperator.Operate function but that doesn't tell me a lot because the Operate call the Run method on the CodeBlock which in turn again calls the Operate call and makes it very hard to actually determine what's taking as long.

Here's the Github repo

Here are the most important bits of the code:

Stack<Token> solvingStack = new Stack<Token>();
for ( int i = 0 ; i < value.Count ; ++i )
{
   if ( value[ i ] is Operator )
   {
      Operator op = (Operator)value[ i ];
      if ( op.Type == OperatorType.PrefixUnary || op.Type == OperatorType.SufixUnary )
      {
         op.Operate( ( solvingStack.Pop() as Value ), new NoValue() );
      } else
      {
         Value second = (Value)solvingStack.Pop();
         Value result = op.Operate( ( solvingStack.Pop() as Value ), second );
         solvingStack.Push( result );
      }
   } else
   {
      solvingStack.Push( value[ i ] );
   }
}
Compiler.ExitScope();
if ( solvingStack.Count == 0 ) return new NoValue();
else return (Value)solvingStack.Peek();

It's how the code is parsed after the infix expression is turned into RPN.
A token can be an operator or a value and a value can be a literal, identifier, codeblock.

A somewhat special case is a function because it's not parsed to RPN directly but only the code inside of it is. Then is there's an invoke operator, it takes the array of arguments that was provided and the function before it and runs the function (which inherits from CodeBlock, so it also runs the same thing that is shown here).

What each operator does on different types of values is determined by the operator itself in the Operators.cs file.

I am not sure if these are bottlenecks, but they are at least performance improvements.

Replace Stack<Token> with Stack<Value> since you are only pushing/popping Value. This way there is no need to do expensive casts.
You can replace

if ( value[ i ] is Operator )
{
    Operator op = (Operator)value[ i ];

with

Operator op = value[ i ] as Operator;
if ( op != null ) // it was an Operator

to have only one typecheck (x as y) instead of two (x is y and (y) x).
You can replace

for ( int i = 0 ; i < value.Count ; ++i )

with

foreach(Token item in value)

to eliminate the index operations value[ i ].


LeetCode: Finding all possible letter combinations from an inputted phone number Problem Statement:

Given a string containing digits from 2-9 inclusive, return all possible letter combinations that the number could represent.
A mapping of digit to letters (just like on the telephone buttons) is given below. Note that 1 does not map to any letters.

This is a leetcode problem. I am looking for feedbacks on how to write better, more elegant code. Any suggestions on style, simplicity, comments, etc., are welcome.
My Code:
class Solution {
public:
  vector<string> letterCombinations(string digits) {
    map<char, vector<char>> num_letter_map;
    construct_num_letter_map(num_letter_map);
    vector<string> result;
    letter_combination_helper(digits, num_letter_map, 0, result, "");
    return result;
}

private:
  void construct_num_letter_map(map<char, vector<char>> &num_letter_map) {
    num_letter_map['2'] = vector<char>{'a', 'b', 'c'};
    num_letter_map['3'] = vector<char>{'d', 'e', 'f'};
    num_letter_map['4'] = vector<char>{'g', 'h', 'i'};
    num_letter_map['5'] = vector<char>{'j', 'k', 'l'};
    num_letter_map['6'] = vector<char>{'m', 'n', 'o'};
    num_letter_map['7'] = vector<char>{'p', 'q', 'r', 's'};
    num_letter_map['8'] = vector<char>{'t', 'u', 'v'};
    num_letter_map['9'] = vector<char>{'w', 'x', 'y', 'z'};

    return;
}

  void letter_combination_helper(string digits, const map<char, vector<char>> &num_letter_map, int i, vector<string> &result, string s) {
    if (i == digits.size()) {
        if (i != 0) {
            result.push_back(s);
        }
        return;
    }

    for (char c : num_letter_map.at(digits[i])) {
        letter_combination_helper(digits, num_letter_map, i+1, result, s + string({c}));
    }
}

};

Preface:

Next time you post code, please include the #includes.  It is easier to review if reviewers can simply copy the code and compile it.  In the same vein, it's helpful if you include any special compiler flags and a test case.  Here is the code as I am reviewing it: https://godbolt.org/z/zQ5mH3.

Review:

You don't write std:: before STL classes so I assume you are using namespace std;.  Many have written about why this is a bad idea so I won't repeat it here, but note it's especially bad in headers (.h files).  It's a less bad in implementations (.cpp files).  You haven't split out your code into a header/implementation, so anyone who #includes your code will also be forced to use namespace std... which is not good.
You use a map<char, vector<char>> to find possible letters from digits.  I think this is OK in cold code/as a mockup implementation while you figure out the rest of your code.  But this is a waste of time/memory in hot code because:

the map is known at compile time
there is no reason to allocate/deallocate anything, and map/vector can both use the heap

Using a function that returns initializer_list<char> instead of a map to vectors speeds up your function 1.6 times (http://quick-bench.com/eYwFgmzQmT95dOsLSRH35FT4QAI).  In case you don't already know, initializer_list is a lightweight data-structure that can be iterated but will not allocate/deallocate memory.  Note that in order to safely return initializer_list, you need to make the list static and might as well make it constexpr.

Your Solution class passes a lot of state in each recursive call.  This call happens many times, so you should try to make it cheap.  For starters you could pass the constant strings by const& to avoid unnecessary copies (this is a 1.9x speed up from the original).  But you could also opt to make the string, result, and digits member variables and then only pass the index in the recursive calls.  This has a few advantages:

Your recursive calls will only have a this pointer and an index (so the arguments will fit in almost every cache line).
It becomes natural to use the same strings the whole time and only copy when producing the result as opposed to the way it was originally written with s + string({c}) which does a heap allocation (at least with std::string and GCC 10).

The simpler recursive calls + the functional map results in a 3.3x speedup compared to the original implementation.  The recursive function now looks like:

void letter_combination_helper(int i) {
  if (i == m_digits.size()) {
    m_result.push_back(m_string);
    return;
  }

  for (char c : lettermap(m_digits[i])) {
    m_string.push_back(c);
    letter_combination_helper(i + 1);
    m_string.pop_back();
  }
}

... where I have prefixed the member variables with "m_" for clarity, and lettermap is a function returning initializer_list<char>.

Minor nit: It's nice to use camelCase or snake_case but not both.
Minor nit 2: I think the indentation of the code you posted is weird e.g. construct_num_letter_map's open bracket is indented but the closing bracket isn't.  This weirdness might have been introduced when you pasted your code into the question, so I won't expand on this.

I think if you make these changes you will have some fast and elegant code.  The general algorithm is the same -- I think you got that right from the start which is good.  I believe the algorithm is optimal since you need O(pow(len(digits), 4)) space to store the result vector, and it seems reasonable to spend that much time writing the answer down.

If I had to summarize the comments into one morsel of feedback, it would be to think more carefully about which operations need to use memory (i.e. allocate/deallocate/copy/dereference).  This was the issue with map (it has to dereference a lot to do lookups) and also with passing lots of state in the recursive calls (lots of copying).  The general rule of thumb is that memory is slow, and if you can reasonably avoid it, then you should.  If you keep at it, this will become second nature to you.


Diamond in the rough! Finding Gems in the rocks Recently a question was asked that solved a hackerrank problem: Counting gems amongst the rocks

The description of the challenge is:

  John has discovered various rocks. Each rock is composed of various
  elements, and each element is represented by a lowercase Latin letter
  from 'a' to 'z'. An element can be present multiple times in a rock.
  An element is called a 'gem-element' if it occurs at least once in
  each of the rocks.

  Given the list of rocks with their compositions, you have to print how
  many different kinds of gems-elements he has.

  Input Format

  The first line consists of N, the number of rocks. Each of the next N
  lines contain rocks’ composition. Each composition consists of small
  alphabets of English language.

  Output Format

  Print the number of different kinds of gem-elements he has.

  Constraints

  \$1 ≤ N ≤ 100\$

  Each composition consists of only small Latin letters ('a'-'z'). 1 ≤ Length of each composition ≤ 100

  Sample Input

3
abcdde
baccd
eeabg

  Sample Output

2

  Explanation

  Only "a", "b" are the two kind of gem-elements, since
  these characters occur in each of the rocks’ composition.

I was in the process of writing an answer when the question was closed because the code in the question was failing to produce the right results.

My recommended solution to the problem is likely quite fast, but it is also relatively complicated. I am sure it can be improved, and simplified.

I have included a simple main method that shows how it can be used, and produces the sample output.

public class GemCounter {

    private static final int LETTERCOUNT = 26;

    // Start with 1 bit set for each element/letter.
    private int gemsSoFar = (1 << LETTERCOUNT) - 1;

    public GemCounter() {
        // default constructor. Does nothing.
    }

    public void processRock(final String rock) {
        int gotElement = 0; // no bits are set.
        for (int i = 0; i < rock.length(); i++) {
            int element = rock.charAt(i) - 'a';
            if (element >= 0 && element < LETTERCOUNT) {
                // bitwise OR the bit that represents the element
                gotElement |= 1 << element;
            }
        }
        // only bits that were found in this rock
        // and also that have been seen before
        // will remain set.
        gemsSoFar = gemsSoFar & gotElement;
    }

    public int getGemCount() {
        // count the number of bits that are still set.
        return Integer.bitCount(gemsSoFar);
    }

    public static void main(String[] args) {
        String[] rocks = {"abcdde", "baccd", "eeabg"};

        GemCounter gemcount = new GemCounter();
        for (String rock : rocks) {
            gemcount.processRock(rock);
            System.out.printf("Processed %s, got %d gems still%n", rock, gemcount.getGemCount());
        }

        System.out.printf("%d rocks have %d gems%n", rocks.length, gemcount.getGemCount());

    }

}

You have added

public GemCounter() {
    // default constructor. Does nothing.
}

which is not needed for the program to work because,

  The compiler automatically provides a no-argument, default constructor
  for any class without constructors. This default constructor will call
  the no-argument constructor of the superclass. In this situation, the
  compiler will complain if the superclass doesn't have a no-argument
  constructor so you must verify that it does. If your class has no
  explicit superclass, then it has an implicit superclass of Object,
  which does have a no-argument constructor [1].

therefore this also violates YAGNI

  "You aren't gonna need it" (acronym: YAGNI) is a principle of extreme
  programming (XP) that states a programmer should not add functionality
  until deemed necessary. XP co-founder Ron Jeffries has written:
  "Always implement things when you actually need them, never when you
  just foresee that you need them [2].

References

[1]“Providing Constructors for Your Classes.” [Online]. Available: http://docs.oracle.com/javase/tutorial/java/javaOO/constructors.html. [Accessed: 27-Aug-2014].
[2]“You aren’t gonna need it,” Wikipedia, the free encyclopedia. 24-Aug-2014.


Decode Argon2 parameters and handling decode errors I am writing a function to decode Argon2 hashed string to extract parameters. I need to handle many decode errors, and handle those errors lead to write many repeating if err == nil check code. Is there a better way to do this kind of check?

The encodedHash string follows the format, separated by $

"$argon2id$v=%d$m=%d,t=%d,p=%d$%s$%s"

so an example string looks like

$argon2id$v=19$m=65536,t=3,p=4$ruDgwQK24h0wGXI87+lVWAbHmgNidUNPVSTdSloOlfM$py20HR7L4K6LllGsZNDbkrbh89x2tIF8JCIG0DAaoi8

What the decode function do is convert the string into this struct

type Argon2Params struct {
    Memory     uint32
    Time       uint32
    Threads    uint8
    SaltLength uint32
    KeyLength  uint32
}

Here is the decode function, I need to do a serious check during the decoding process. Every filed need to be checked to make sure no error rises.

func decodeHash(encodedHash string) (p *Params, salt, hash []byte, err error) {
    fileds := strings.Split(encodedHash, "$")
    if len(fileds) != 6 {
        return nil, nil, nil, errors.New("The encoded hash is not in the correct format")
    }

    var version int
// error check
    if _, err = fmt.Sscanf(fileds[2], "v=%d", &version); err != nil {
        return
    }
// error check
    if version != argon2.Version {
        return nil, nil, nil, errors.New("Argon2 version not match")
    }

    p = &Params{}
    _, err = fmt.Sscanf(fileds[3], "m=%d,t=%d,p=%d", &p.memory, &p.time, &p.threads)
// error check
    if err != nil {
        return
    }

    salt, p.saltLength, err = decodeBase64WithLength(fileds[4])
// error check
    if err != nil {
        return
    }
// error check
    hash, p.keyLength, err = decodeBase64WithLength(fileds[5])
    return
}
// a helper function
func decodeBase64WithLength(encodeString string) (str []byte, length uint32, err error) {
    str, err = base64.RawStdEncoding.DecodeString(encodeString)
    length = uint32(len(str))
    return
}

Because you use named parameters, the error checking can indeed be simplified.

While you critique the use of if err != nil, I enjoy the consistent error handling, and prefer it to the concepts used in many other languages.

Returning errors

Rather than explicitly returning values, such as

return nil, nil, nil, errors.New("...")

You can instead set the named return value err:

if len(fields) != 6 {
    err = errors.New("incorrect hash format")
    return
}

This does not set the other values to nil, and that shouldn't matter. In terms of consuming this API, it would be bad practice to use any other return values if err is not nil. It would be the equivalent in C of using the return value of strtoul() without checking errno.

Having the comment // error check each time is too verbose.

Misspelling

The variable fileds is misspelled, it should be fields.

Helper function

Using named return values in your helper function is superfluous. You can easily return those variables directly.

func decodeBase64WithLength(encStr string) ([]byte, uint32, error) {
    s, err := base64.RawStdEncoding.DecodeString(encStr)
    return s, uint32(len(s)), err
}

But now that the function has been cleaned up, it's easy to see that it really isn't needed. If this is the only place you use the function, then doing something small twice doesn't justify a helper function in my opinion.

Params

You reference Argon2Params as Params, and both are exported types, but you refer to its members as unexported types. I'll assume it's exported, and capitalize the member variables.

Conclusion

Here is the code I ended up with:

package main

import (
    "encoding/base64"
    "errors"
    "fmt"
    "strings"

    "golang.org/x/crypto/argon2"
)

// Argon2Params are ...
type Argon2Params struct {
    Memory     uint32
    Time       uint32
    Threads    uint8
    SaltLength uint32
    KeyLength  uint32
}

func decodeHash(encodedHash string) (p *Argon2Params, salt, hash []byte, err error) {
    fields := strings.Split(encodedHash, "$")

    if len(fields) != 6 {
        err = errors.New("incorrect hash format")
        return
    }

    var version int

    if _, err = fmt.Sscanf(fields[2], "v=%d", &version); err != nil {
        return
    }

    if version != argon2.Version {
        err = errors.New("argon2 version mismatch")
        return
    }

    p = &Argon2Params{}

    if _, err = fmt.Sscanf(fields[3], "m=%d,t=%d,p=%d", &p.Memory, &p.Time,
        &p.Threads); err != nil {
        return
    }

    salt, err = base64.RawStdEncoding.DecodeString(fields[4])
    p.SaltLength = uint32(len(salt))

    if err != nil {
        return
    }

    hash, err = base64.RawStdEncoding.DecodeString(fields[5])
    p.KeyLength = uint32(len(hash))

    return
}

func main() {
    decodeHash("$argon2id$v=19$m=65536,t=3,p=4$ruDgwQK24h0wGXI87+lVWAbHmgNidUNPVSTdSloOlfM$py20HR7L4K6LllGsZNDbkrbh89x2tIF8JCIG0DAaoi8")
}

Hope this helps!


Generating a list of primes of a given length After considerable effort, I've come up with the following code to generate a list of primes of a given length.

I would be very interested to see how an experienced coder would modify my code to make it more readable, more concise, or somehow better.  I won't be able to follow fancy-pants coding that doesn't involve basic iterations of the type I have used, so please keep it simple for me.  I've been learning the language only a few months.

Function returns number of primes indicated in call.

Algorithm: Add two to last candidate in list (starting with [2, 3, 5]) and check whether other members divide the new candidate, iterating only up to the square root of the candidate.

import math
def primeList(listLength):
    plist = [2, 3]
    j = 3
    while listLength > len(plist):
        prime = 'true'
        i = 0
        j +=2
        plist.append(j)
        while plist[i] <= math.sqrt(j) and prime == 'true':
            if j%plist[i] == 0:
                prime = 'false'
            i +=1
        if prime == 'false':
            plist.pop(-1)
    return plist

Don't be afraid of typing. Longer names are more readable and should be used except for simple counters. Split out complicated sub-parts where they are separate concepts.

Here is how I would improve it:

import math

def isPrime (primeList, candidate):
    upperLimit = math.sqrt(candidate)
    for p in primeList:
        if candidate % p == 0:
            return False
        if p >= upperLimit:
            break

    return True

def primeList(listLength):
    if listLength < 1 :
        return []
    primes = [2]

    candidate = 3
    while listLength > len(primes):
        if isPrime(primes, candidate):
            primes.append(candidate)
        candidate +=2

    return primes

To make a faster simple prime algorithm, consider the other naive algorithm in the primality test wikipedia article where all primes are of the form 6k +- 1.


Extracting and cleaning up duplicate emails Right now I am using a regex to extract emails from a text document, and filter the duplicates out of this using the Distinct function from linq. After which I run the output file through a clean up of duplicates as well, as the program can be run several times over several different files.

Now my question, would there be a more optimal way of checking the output file for duplicates? Because as it is right now, the bigger the file, the longer it will take.

And I have the feeling there should be a less intense way to check this.

// Extractor
public void Mail(string file)
{
    string strRegex = @"[A-Za-z0-9_\-\+]+@[A-Za-z0-9\-]+\.([A-Za-z]{2,3})(?:\.[a-z]{2})?";
    var myRegex = new Regex(strRegex, RegexOptions.None);

    var matches = new List<string>();
    foreach (Match myMatch in myRegex.Matches(file))
    {
        if (myMatch.Success)
        {
            matches.Add(myMatch.Value);
        }
    }

    var cleanMatch = matches.Distinct().ToList();

    for (var i = 0; i < cleanMatch.Count; i++)
    {
        Log.Mail(cleanMatch[i]);
    }

    CleanDuplicates();
}

// Duplicate Cleaner
private void CleanDuplicates()
{
    var lines = new List<string>(System.IO.File.ReadAllLines(System.Environment.GetFolderPath(System.Environment.SpecialFolder.DesktopDirectory) + "\\EmailList.txt"));

    lines = lines.Distinct().ToList();
    string target = System.Environment.GetFolderPath(System.Environment.SpecialFolder.DesktopDirectory) + "\\EmailList.txt";
    using (System.IO.StreamWriter newTask = new System.IO.StreamWriter(target, false))
    {
        for (var i = 0; i < lines.Count; i++)
        {
            newTask.WriteLine(lines[i]);
        }
    }
}

You'll be able to simplify this a lot if you use a better in memory datastructure than a List<T>. You only want distinct items, HashSet<T> will generally be better here as it has O(1) lookups.

public void Mail(string file)
{
    var strRegex = @"[A-Za-z0-9_\-\+]+@[A-Za-z0-9\-]+\.([A-Za-z]{2,3})(?:\.[a-z]{2})?";
    var myRegex = new Regex(strRegex, RegexOptions.None);
    var matches = new HashSet<string>();
    foreach (Match matchedValue in myRegex.Matches(file))
    {
        // Changed per Mjolka's comment
        if (matches.Add(matchedValue.Value))
        {
            Log.Mail(matchedValue.Value);
        }
    }
    CleanDuplicates();
}

Note that I've removed the uneccessary check for Success on the Match object.

Your naming could do with a bit of work myRegex doesn't really mean anything.

You could also store the Regex as a field on the class to declutter this method.

You can leverage a HashSet<string> in your CleanDuplicates method too:

private void CleanDuplicates()
{
    var fileLocation = System.Environment.GetFolderPath(System.Environment.SpecialFolder.DesktopDirectory) + @"\EmailList.txt";

    var lines = new HashSet<string>(File.ReadAllLines(fileLocation));

    File.WriteAllLines(fileLocation, lines.ToArray());
}

Definitely add a using System.IO it removes a lot of clutter.

Prefer built in methods e.g. File.WriteAllLines overwrites the file or creates a new one - no need to whip out a StreamWriter.

Also note that if your file becomes really large this will start getting pretty slow.

Final(?) Edit

Can't use var in the foreach loop as MatchCollection implements IEnumerable only - not the generic version. Sorry about that.

In order to use a Linq method (such as Select) we have to supply the type by calling Cast<T>:

foreach (var matchedValue in myRegex.Matches(file).Cast<Match>().Select(m => m.Value))
{
    if (matches.Add(matchedValue))
    {
        Log.Mail(matchedValue);
    }
}


Modified bucket sort The code runs fine and the algorithm is much faster than insertion sort. I built a modified bucket sort around the parameters given in the HW to sort a vector of ints with 50 iterations and 40000 items.

Being new and not familiar with bucket sorts I haven't found any code examples quite like this one. So my question is about my style and possible room for optimization. How can I streamline this algorithm?

void intVectSort2(vector<int> &V, int M)
   {
     // declare size for U and V
     int uSize = ((M * 2) + 1);
     int vectSize = V.size();

     // allocate memory for U array and initialize to zeros
     int *U = new int[uSize];
     for (int i = 0; i < uSize; i++)
       U[i] = 0;

     // step through M values and increment U when V is equal to M
     for (int j = 0; j < vectSize; j++)
       {
         int val = 0;
         int count = 0;
         int element = 0;
         int iterator = M;
         val = V[j];
         while (val != iterator)
           iterator--;
         element = iterator + M;
         count = U[element];
         count++;
         U[element] = count;
       }

     //load confirmed values in order back into V
     int index = 0;
     int currentSize = 0;
     for (int k = 0; k < uSize; k++)
       {
         int value = 0;
         int counter = 0;
         value = U[k];
         if (value != 0)
           {
             while (counter < value)
             counter++;
             currentSize += counter;
             for (index; index < currentSize; index++)
               V[index] = k - M;
           }
       }

     // free dynamic memory
     delete [] U;
   }

Pointless loops

You have a couple of loops that do nothing but waste time:

     while (val != iterator)
       iterator--;

That loop does the same thing as:

    iterator = val;

Similarly, this loop:

         while (counter < value)
         counter++;

is the same as:

         counter = value;

Way too complicated

Your code that fills the buckets is confused and complicated:

 for (int j = 0; j < vectSize; j++)
   {
     int val = 0;
     int count = 0;
     int element = 0;
     int iterator = M;
     val = V[j];
     while (val != iterator)
       iterator--;
     element = iterator + M;
     count = U[element];
     count++;
     U[element] = count;
   }

It should really be a one line loop:

for (int j = 0; j < vectSize; j++) {
    U[V[j]]++
}

Note that this uses buckets in the range 0..M, whereas yours uses buckets in the range M..2M for no good reason.

Similarly, your bucket unpacking loop could be simplified to:

int index = 0;
for (int i = 0; i <= M; i++) {
    for (int j = 0; j < U[i]; j++) {
        V[index++] = i;
    }
}

This assumes that you allocated U to hold M+1 elements, to allow for values in the range 0..M.


ArrayBlockingQueue: concurrent put and take I have implemented an ArrayBlockingQueue (put and take) on the lines of LinkedBlockingQueue i.e using two locks so that my take should not block my put.

For more information please see this Stack Overflow question.  The code is also available at GitHub.

Please review the code for any race conditions.

import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class ArrayBlockingQueue<T> {

    private final int capacity;
    private final AtomicInteger count = new AtomicInteger(0);
    private final Lock putLock = new ReentrantLock();
    private final Lock takeLock = new ReentrantLock();
    private final Condition notEmpty = takeLock.newCondition(); // putLock will signal that its not empty
    private final Condition notFull = putLock.newCondition(); // signal when queue is notFull
    private final Object [] items ;
    int takeIndex;
    int putIndex;

    public ArrayBlockingQueue(int capacity) {
        this.capacity = capacity;
        items = new Object[capacity];

    }

    public void add(T t) throws InterruptedException {
        int c = -1;
        final Lock lock = this.putLock;
        final AtomicInteger count = this.count;
        lock.lock();
        try {
            while(count.get() == capacity) {
                    notFull.await();
            }
            enqueue(t);
            c = count.getAndIncrement();
            if(c+1 < capacity) {
                notFull.signal();
            }
        }finally {
            lock.unlock();
        }
        if(c == 0) {
            signalNotEmpty();
        }
    }

    public T take() throws InterruptedException {
        int c = -1;
        final Lock lock = this.takeLock;
        final AtomicInteger count = this.count;
        T t = null;
        lock.lock();
        try {
            while(count.get() == 0) {
                notEmpty.await();
            }
            t = dequeue();
            c = count.getAndDecrement();
            if(c > 1) {
                notEmpty.signal();
            }
        }finally {
            lock.unlock();
        }
        if(c == capacity) {
            signalNotFull();
        }
        return t;
    }

    private void signalNotEmpty() {
        final Lock lock = this.takeLock;
        lock.lock();
        try {
            notEmpty.signal();
        }finally {
            lock.unlock();
        }
    }

    private void signalNotFull() {
        final Lock lock = this.putLock;
        lock.lock();
        try {
            notFull.signal();
        }finally {
            lock.unlock();
        }
    }

    private T dequeue() {
        final Object[] items = this.items;
        @SuppressWarnings("unchecked")
        T x = (T) items[takeIndex];
        items[takeIndex] = null;
        if (++takeIndex == items.length)
            takeIndex = 0;
        return x;
    }

    private void enqueue(T t) {
        final Object[] items = this.items;
        items[putIndex] = t;
        if (++putIndex == items.length)
            putIndex = 0;
    }

}

Typo

In your take() function, you wrote this:

            notFull.signal();

But I'm pretty sure you meant to write this:

            notEmpty.signal();

Unnecessary Signalling? Nope

Originally, I thought that the extra signalling in the add() and take() functions were unnecessary.  In fact, they are not required, but when they are added, it improves performance.  It is a concept called "cascading notifies".  The LinkedBlockingQueue sources  explain it like this:

  Also, to minimize need for puts to get takeLock and vice-versa, cascading notifies are used. When a put notices that it has enabled at least one take, it signals taker. That taker in turn signals others if more items have been entered since the signal. And symmetrically for takes signalling puts.

Race condition? Nope

Originally, I thought that the code had a race condition involving memory reordering when accessing the shared queue.  However, it was pointed out that AtomicInteger acts as a volatile variable and provides a memory barrier.  So therefore the accesses are safe.

Update

After reading @rolfl's answer with his multiple variants that attempt to improve upon the original function, I was inspired to come up with my own variant.  I came up with one that is simpler than any of the variants listed here so far.  It basically uses two semaphores, normal synchronization, and an array:

import java.util.concurrent.Semaphore;

public class ArrayBlockingSemQueue<T> implements BlockQueue<T>
{
    private final int capacity;
    private final Semaphore notEmpty;
    private final Semaphore notFull;
    private final Object [] items;
    int takeIndex;
    int putIndex;

    public ArrayBlockingSemQueue(int capacity) {
        this.capacity = capacity;
        notEmpty = new Semaphore(0);
        notFull  = new Semaphore(capacity);
        items = new Object[capacity];
    }

    public void add(T t) throws InterruptedException {
        notFull.acquire();

        synchronized(notFull) {
            items[putIndex++] = t;
            if (putIndex == capacity)
                putIndex = 0;
        }

        notEmpty.release();
    }

    @SuppressWarnings("unchecked")
    public T take() throws InterruptedException {
        T ret;

        notEmpty.acquire();

        synchronized(notEmpty) {
            ret = (T) items[takeIndex++];
            if (takeIndex == capacity)
                takeIndex = 0;
        }

        notFull.release();
        return ret;
    }
}

Benchmarks

I used the same code that @rolfl listed in his answer to test the timings of the various Queues.  Here are my results:

           1x1   2x2   3x3   4x4   6x6   12x12 1x12  2x6   3x4   4x3   6x2   12x1
Veritas    3.32  3.39  3.54  3.59  3.54  3.62  6.63  3.95  3.67  3.68  3.88  6.16
DoubleSync 3.72  4.97  4.87  4.94  5.07  4.78  5.43  5.01  4.96  4.93  5.01  5.41
CDLatch    2.89  2.56  4.62  5.02  5.83  8.01  8.25  5.95  5.24  3.60  2.58  2.65
JS1        3.05  3.00  3.18  3.08  3.04  3.09  3.47  3.13  3.26  3.07  3.00  3.15


Helper method to extract a specific string from long message private string ExtractExceptionMesssage(string exceptionMessage)
{
    const string startWord = "Message&gt";
    int startWordLength = startWord.Length;
    const string endWord = "/Message&gt";
    var length = exceptionMessage.Length;
    int index = 0;
    var sb = new StringBuilder();

    while (index < length)
    {
        var startIndex = exceptionMessage.IndexOf(startWord, index, StringComparison.Ordinal);
        if (startIndex < 0)
            break;
        var endIndex = exceptionMessage.IndexOf(endWord, startIndex, StringComparison.Ordinal);
        if (endIndex < 0)
        {
            break;
        }
        var len = endIndex - 4 - (startIndex + startWord.Length); // -4 is ".&lt"
        sb.AppendLine(exceptionMessage.Substring(startIndex + startWordLength, len));
        index = endIndex + endWord.Length + 1;
    }

    if (sb.Length == 0) sb.Append("Unknown error");
    return sb.ToString();
}

I'll appreciate any idea of how to make it better

Consistent style
You kind of mixing your styles with curly braces - your indentation is inconsistent, and for two duplicate ifs with single statement you once omit the braces, and on the other not. Choose a style, and stick to it - it will make your code more readable

Magic numbers
You made a curious choice to omit the "&lt;" from you start and end indicators, which made you add the mysterious - 4 to your len calculation. This is a major maintenance issue, since tomorrow you will want to extract with unescaped text, where the "&lt;" will be reduced to "<", and your code will stop working...

Use the powers of the tools at hand
Your solution could be much simpler if you used Regex to solve it:

private string ExtractExceptionMesssage(string exceptionMessage) {
    string pattern = @"&lt;Message&gt;(.+?)lt;/Message&gt;";
    Regex rgx = new Regex(pattern);
    var sb = new StringBuilder();
    MatchCollection matches = rgx.Matches(input);
    if (matches.Count > 0) {
       foreach (Match match in matches) {
         sb.AppendLine(match.Value);
       }
    } else {
       return "Unknown error";
    }
    return sb.ToString();
}


Simple pay rate calculator Follow-up posted -> Follow up to Pay Rate Calculator

This little bit of code is just some code that I wrote from a flowchart at the beginning of a programming and logic book that I have added some to. I tried to create it as an object.

I would like a review on what I am doing and if I am doing it right, and what I could do better.  I want you to look at this like I am a novice to intermediate Programmer.  I don't think that I learned as much as I should have while I was in school, and it probably shows.

Main Method

public static void Main (string[] args)
{
    var payCheck = new PayRateCalculator();
    Console.Write ("Please provide the Employees Last Name >>");
    payCheck.lastName = Console.ReadLine();
    Console.Write ("How many hours has employee {0} worked? >>", payCheck.lastName);
    payCheck.hours = Convert.ToInt32 (Console.ReadLine());
    Console.Write ("What is employee {0}'s hourly wage? >>", payCheck.lastName);
    payCheck.payrate = Convert.ToInt32 (Console.ReadLine ());
    Console.WriteLine ("{0}'s Gross pay is ${1}", payCheck.lastName, payCheck.grossPay);
    Console.WriteLine ("{0}'s Net pay is ${1}", payCheck.lastName, payCheck.netPay);
}

Class

class PayRateCalculator
{
    private float _payrate = 8.25f;
    private float _withholdingRate = 0.20F;

    public float hours { get; set; }
    public float grossPay {
        get {
            return hours * _payrate;
        }
    }
    public float netPay {
        get {
            return grossPay - (grossPay * _withholdingRate);
        }
    }
    public float payrate {
        get {
            return _payrate;
        }
        set {
            _payrate = value;
        }
    }
    public string lastName { get; set; }

    public PayRateCalculator (string lastName, float hours, float payRate)
    {
        this.hours = hours;
        this.payrate = payrate;
        this.lastName = lastName;
    }

    public PayRateCalculator ()
    {

    }
}

Just a simple console application. Help me to relearn coding the right way.

As a few people have pointed out, there is actually nothing really wrong with this code.

I mean, there needs to be some naming standardization, most notably any public property should be capital.

I would also advise a backing field for each property making the constructor a bit cleaner:

_hours = hours;
_payrate = payrate;
_lastName = lastName;

That being said from a design point of view, it is best to mirror your objects to real world  objects.

Is a PayRateCalculator actually a PayCheck?  Would it not make more sense to have a paycheck of some kind, once you have your details assigned you can return the paycheck as a separate thing?

The means with which you generate the paycheck should stand alone from the paycheck itself,
allowing the possibility in future of having multiple calculation methods or doing something interesting with the paycheck object.

Although again, these are just design considerations as your application scales.

There is nothing inherently wrong with your code, which is well-written, decently-named and functional.

Well done.


Colorful Lights in HTML I am a beginner and I have made Colorful Lights using check-boxes in HTML. I'm pretty sure it will look horrible to any developer out there, but hey, that's why I've posted it.

I'd like a general review of this. I'm especially concerned about the quality and enhancements of this form.

<title>
Javascript
</title>

1) I'd start with a better HTML template, you forgot the DOCTYPE and meta tags:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title></title>
</head>
<body>

</body>
</html>

2) Don't use topalign and align HTML attributes. Keep the styles and layout separate, use CSS:

body {
  margin: 15px 0;
  text-align: center;
}

3) In JavaScript it is ussually preferred to put braces on the same line, because the ASI (automatic semicolon insertion) feature can be problematic, consider this buggy example:

return
{
  foo: 'foo'
}

JavaScript will do this:

return; //<--!!!!
{
  foo: 'foo'
}

See the semicolon it added? Now you return undefined, and not an object.

Now, you forgot a bunch of semicolons. Again, ASI can fill them up for you, but I don't recommend you rely on it if you're a beginner, so my advice is always put the semicolons.

5) Check your code in JSHint, this is what I got:

13 warnings
1   Missing semicolon.
5   Missing semicolon.
6   document.write can be a form of eval.
6   Missing semicolon.
8   Missing semicolon.
9   document.write can be a form of eval.
9   Missing semicolon.
13  Missing semicolon.
14  document.write can be a form of eval.
14  Missing semicolon.
16  Missing semicolon.
17  document.write can be a form of eval.
17  Missing semicolon.
One undefined variable
5   randomnumber
6   randomnumber
13  randomnumber
14  randomnumber
One unused variable
1   time

Go one by one, try to fix all those warnings.

6) Avoid document.write, see Why is document.write considered bad practice. In other words, use the DOM.

7) Inline events are bad practice, again, use the DOM, with addEventListener.

8) Using the DOM can be expensive. Try to always add elements all at once. Document fragments help you improve performance, because you don't mess with the DOM on every iteration of the loop.

9) Finally, separate concerns; the single responsibility idea. This is how I would write it taking all those points into consideration:

// Use the DOM to create elements
function makeInput() {
  var input = document.createElement('input');
  input.type = 'checkbox';
  return input;
}

function append(ele, parent) {
  (parent||document.body).appendChild(ele);
}

function makeLights() {
  var i, j;
  var br = document.createElement('br');

  // Use a document frag to improve performance
  var frag = document.createDocumentFragment();

  // No need to reset "j" on every iteration
  for (i=0; i<=32; i+=2) {
    for (j=0; j<=i; j++) append(makeInput(), frag);
    append(br.cloneNode(), frag);
  }

  for (i=30; i>=0; i-=2) {
    for (j=0; j<=i; j++) append(makeInput(), frag);
    append(br.cloneNode(), frag);
  }

  // Append to document all at once
  append(frag);
}

function randomColor() {
  return Math.floor(Math.random() * 2323232);
}

// Use event delegation for best performance
document.body.addEventListener('mouseover', function(e) {
  if (e.target.tagName == 'INPUT') {
     document.bgColor = randomColor();
  }
});

makeLights(); // init

Demo: http://jsbin.com/yiyic/1


Converting an integer to binary, hex, and octal I've started caring about standards and the beauty of my code. I doubt whether I've written it correctly and aesthetically. Can you please judge this simple, exemplary program? I want to know of any mistakes I've made. It converts an integer number given in the argument to binary, hexadecimal and octal number. Ignore the "1." prefix.

1.conversion_main.cpp:

//Conversion
#include <iostream>
#include <cstdlib>
#include <cstring>
#include <sstream>
#include "1.conversion_header.h"
#include "1.convert.cpp"

int parse(int, char**);

int main(int argc, char *argv[]){
    int number = parse(argc, argv);
    if (number == 0)
      return 0;
    convert converter;
    std::cout << "HEX: " << converter.to_hex(number) << std::endl;
    std::cout << "OCT: " << converter.to_oct(number)  << std::endl;
    std::cout << "BIN: " << converter.to_bin(number)  << std::endl;
    return 0;
}

int parse(int argc, char* argv[]){
    if (argc == 2){
        if ((strcmp(argv[1], "--help") == 0) || (strcmp(argv[1], "-h") == 0)
        || (strcmp(argv[1], "/?")) == 0){
            std::cout << "Usage: a R\\{0}\n";
        }
        else{
            int check;
            std::istringstream in(argv[1]);
            if ((in >> check) && (in.eof()))
                return atoi(argv[1]);
        }
    }
    return 0;
}

1.conversion_header.h:

//Conversion header

class convert{
    public:
          std::string to_hex(int);
          std::string to_oct(int);
          std::string to_bin(int);
  protected:
          std::string int_to_bin(int);
          std::string reverse(std::string);
};

1.convert.cpp:

//Convert definition

#include <string>

std::string convert::int_to_bin(int number){
    static std::string result;
    static int level = 0;
    level++;
    if (number > 0){
        if (number % 2 == 0){
            result.append("0");
        }
        else{
            result.append("1");
        }
        int_to_bin(number / 2);
        level--;
    }
    if (level == 1) return reverse(result);
    return result;
}

std::string convert::reverse(std::string to_reverse){
    std::string result;
    for (int i = to_reverse.length()-1; i >=0 ; i--)
        result += to_reverse[i];
    return result;
}

std::string convert::to_hex(int to_convert){
    std::string result;
    std::stringstream ss;
    ss << std::hex <<to_convert;
    ss >> result;
    return result;
}

std::string convert::to_oct(int to_convert){
    std::string result;
    std::stringstream ss;
    ss << std::oct << to_convert;
    ss >> result;
    return result;
}

std::string convert::to_bin(int to_convert){
    return int_to_bin(to_convert);
}

I'll just address several things regarding good practice.  This can probably be simplified greatly overall, but I won't try to focus on that particularly.

main():

I don't see the need for if (number == 0), especially in relation to parse().  Are you just skipping further calculation if the return is 0?  If you do that, the outputs at the end will not be printed, even though 0 is a valid number.
You don't need to use std::endl in main().  That also flushes the buffer, which is unnecessary here, and you do it multiple times.  Just use "\n" within a print statement.

parse():

You can define it above main(), allowing you to remove the function prototype.
It doesn't need the command line parameters; only main() does.
Its control paths and returns don't make sense to me.  If the if statement is executed, it displays a message and returns 0.  If the else is executed, it returns a converted integer.  Instead, the function should only be called if it will parse something, which is its main job.  Functions should focus on just one thing.  Let the calling code decide if it should be called.

Header:

There's no need for protected if you're not using inheritance (I believe even Bjarne Stroustrup himself has regretted adding that keyword).  Either change it to private, or just remove the keyword since classes are private by default.
If you're not maintaining one or more data members, then this program may not need to utilize classes.  This one just contains functions, but class functions are supposed to change the state of one or more data members.  Instead, those functions could just be free functions (non-member) and the class removed entirely.

Other:

This:

if (number % 2 == 0){
    result.append("0");
}
else{
    result.append("1");
}

can just be made into a single-ternary statement:

(number % 2 == 0) ? result.append("0") : result.append("1");

Better yet, since std::string supports +=, use that instead:

result += (number % 2 == 0) ? "0" : "1";

The local variables in int_to_bin() don't need to be static.
You don't need your own reverse function; just use std::reverse().
As @rachet freak has mentioned in the comments, you can just use std::hex to manipulate the I/O when displaying this value.  In addition, you can do this with all three number systems.


C program to print a triangle of stars This C code is to print a triangle of stars.

#include <stdio.h>
int main(){
    for(int i=0;i<3;i++) {
        for(int j=0;j<i*2+1;j++){
            printf("*");
        }
        printf("\n");
    }
}

which works well though, I'd like to know if there is a better way to do the job, as it's said 2 layers of loop is inefficient, compared to vectorization.

I use a string to avoid the inner loop. The problem here is the printf() inside. A 3000-row triangle redirected to /dev/null takes 50ms, but now only 4ms.
I reformatted the output. And I left dots as fillers to see what is going on.
$ ./a.out
...*...
..***..
.*****.
*******

It starts with one * in the so-called middle, and every row-iteration only sets two more bytes to * before it prints.
The complicated part is preparing the string i.e. array of chars. You can use calloc() to clear the right side, but it still takes a step to put blanks on the left side.
#include <stdio.h>
#include <string.h>

int trihi = 4;
char PIX = '*';

int main(void){

    int width = trihi * 2 - 1;
    char s[width + 1];
    memset(s, '.', width);
    s[width] = '\0';

    int mid = trihi - 1;
    s[mid] = PIX;
    printf("%s\n", s);

    for (int i = 1; i < trihi; i++) {

        s[mid - i] =
        s[mid + i] = PIX;
        printf("%s\n", s);
    }
}

This should add up. s[mid + i] runs up to 2*mid = 2*(trihi-1).
The '\0' sits at width = 2*trihi - 1 which is one higher, and is the highest legal index for s.
It is very easy to make a small mistake and not have the \0 at the correct place.
It also works with a triangle height of 1:
$ ./a.out
*

And by setting ... = --PIX; in the loop:
$ ./a.out
.....*.....
....)*)....
...()*)(...
..'()*)('..
.&'()*)('&.
%&'()*)('&%

The So-Called Middle
This is how a triangle of height 4 is laid out. Size is 8. The middle is s[3]; there are 3 spaces to the left.
01234567  --- "offset", "index"
...*...0
|  |   |
|  |   +-- s[width]  (width = 2*trihi - 1)
|  +------ s[mid]    (mid   = trihi - 1)
+--------- s[0]

You start with trihi=4, but what you need is mostly 3 and 7...the geometrical base width is also 7.

Just any Triangle
If this is enough (with original height = 3):
$ ./a.out
*
**
***

then the code gets much simpler:
#include <stdio.h>
#include <stdlib.h>

int trihi = 3;
char PIX = '*';

int main(void){

    char *s = calloc(trihi+1, 1);

    for (int i = 0; i < trihi; i++) {
        s[i] = PIX;
        printf("%s\n", s);
    }
}

I kicked everything out except the basic idea: re-using a prepared string/array. Here the calloc-zeroes are only after the PIXes. No need for a filler like space or dots via memset() anymore. But it is also not the same user experience...and the triangle is half the size.
Overiq.com has a double-loop version of a C Program to print Half Pyramid pattern using *. It takes much longer than the above (tri2.c below).
$ gcc -O2 over.c
$ time ./a.out |wc -l
3001

real    0m0.029s
user    0m0.020s
sys     0m0.014s
$ gcc -O2 tri2.c
$ time ./a.out |wc -l
3000

real    0m0.006s
user    0m0.000s
sys     0m0.011s


Using RestTemplate for HTTP request timeout I am using RestTemplate to make an HTTP call to one of my service and I would like to have timeout for my HTTP Request:

public static String getResponse(final String url) {

    RestTemplate restTemplate = new RestTemplate(clientHttpRequestFactory());
    String response = restTemplate.getForObject(url, String.class);

    // return response

}

private ClientHttpRequestFactory clientHttpRequestFactory() {
    HttpComponentsClientHttpRequestFactory factory = new HttpComponentsClientHttpRequestFactory();
    factory.setReadTimeout(2000);
    factory.setConnectTimeout(2000);
    return factory;
}

(I came up with this after reading this.)

Is this an efficient way of using RestTemplate along with ClientHttpRequestFactory? Since with this for each call, it will make a new RestTemplate object. Earlier, I was not using any timeout for RestTemplate so I declared RestTemplate as static final.

private static final RestTemplate restTemplate = new RestTemplate();

public static String getResponse(final String url) {

    String response = restTemplate.getForObject(url, String.class);

    // return response

}

I am not sure what is the right way of using RestTemplate along with ClientHttpRequestFactory. Should they both be declared as static?

In the example you linked, RestTemplate is annotated with @Bean:

@Bean
public RestTemplate restTemplate() {
    return new RestTemplate(clientHttpRequestFactory());
}

Beans are (normally) singletons in Spring, intended for reuse. This related post also confirms that RestTemplate is thread-safe, and it would be wasteful to recreate it every time.

It would be best to configure RestTemplate as in the linked article, and inject into your setup using Spring's annotations based configuration. Or, you could make it a private final member of your class.

class RequestHandler {
    private final RestTemplate restTemplate;

    public RequestHandler() {
        restTemplate = new RestTemplate(clientHttpRequestFactory());
    }

    public RestTemplate getRestTemplate() {
        return restTemplate;
    }
}


Speed up a sampling function I have the following dataset

kkk<-data.frame(days=1:100,positive=rbinom(100,1,0.05))

Over the monitoring period of 100 days, if an event occurs then for that day kkk$positive==1 else kkk$positive==0

The following function, samples k=s.freq=1:12 number of times j=s.length=1:30 consecutive elements from the sample space and checks if at least in one of the samples an event was recorded. The sample space consists of all possible j consecutive combinations of kkk$days. The elements of the sample space are overlapping. Before each of the k=s.freq=1:12 samples, the sample space is reevaluated and elements that have common days with the sampled element are removed before the next sampling occurs (for the total of k samplings).

my.sampling<-function(days, status, s.length, s.freq, iter){

    start<-Sys.time()

    #build the data frame
    ddd<-data.frame(days=days, positive=status)

#length of each sample
    j<-s.length

#Buld the sample space
    monitor.ss<-matrix(nrow=dim(ddd)[1]-j+1, ncol=j+1)
    n.row<-dim(ddd)[1]-j+1

    #Create the elements of sample space: each row of the monitor.ss is an element of the sample space
    for (i in 1:j){
        monitor.ss[,i]<-i:(n.row-1+i)
    }
    rm(i)

    #adds if the each of the possible samples of the sample space at least one event was observed or not
    monitor.ss[,j+1]<-apply(monitor.ss,1,function(x) {
        r.low<-range(x,na.rm=T)[1]
        r.high<-range(x,na.rm=T)[2]
        return(as.numeric(sum(ddd$positive[r.low:r.high])>0))})

    #Build the initial sample space as data frame
    sampling.start<-data.frame(monitor.ss)

suc.list<-rep(NA, iter) #Initiate a vector to keep track of successes

    #Now the sampling takes place

    for (i in 1:iter) { #for each iteration

        sampling.start<-data.frame(monitor.ss)
        k<-s.freq #number of samples drawn from the sample space
        k.t=0 #controller to break the while loop
        suc<-0

        while (k.t<=k | dim(sampling.start)[1]>0) { #breaks the while loop if k reaches the specified limit (k=s.freq) or if the reduction of the sample space (see below) does not leave any elements in the sample space

            s<-sampling.start[sample(nrow(sampling.start),1),] #samples the first trial
            suc<-suc+s[1,j+1] #adds up if there is a success or not ( a tleast one event in the sample was observed)
            sampling.start <- sampling.start[apply(sampling.start, 1, function(x) !any(x %in% as.numeric(s[,1:j]))),] #sample space reduction: Elements of the sample space that intersect with the sample that was just selected are removed from the sample space before the next sampling occurs
            k.t<-k.t+1 #controller for the wile loop
        }

        suc.list[i]<-(suc/k.t)

    }
    print(Sys.time()-start)
    message("")
    #hist(suc.list, breaks=sqrt(iter))
    return(suc.list)
}

For example:

my.sampling(kkk$days, kkk$positive, s.length=3, s.freq=5, iter=300)

My problem is that the function requires about 100msec for each iteration and I need to run this function about 100.000 times with at least 1000 iterations each time. I have used preallocation of the vectors and apply to speed things up, however its still too slow.

Parallel processing is applicable to my problem as I can split the data on which the function will have to run on multiple cores, however if my calculations are right I need about 120 days of processing pro CPU core.

Is this possible to reduce the run time of the function? I have tried cpmfun but without much improvement. Otherwise I have to resort to some type of AWS EC2 solution.

Thanks a lot

This question is probably not appropriate for stackoverflow, since it requires mainly code review.

First of all, there is a logics problem with your while loop. Changing | to & reduces the number of loops considerably.

Furthermore you should not use dataframes, if you can use a matrix.

my.sampling1<-function(days, status, s.length, s.freq, iter){
    start<-Sys.time()

    #build the data frame
    ddd<-data.frame(days=days, positive=status)

#length of each sample
    j<-s.length

#Buld the sample space
    monitor.ss<-matrix(nrow=dim(ddd)[1]-j+1, ncol=j+1)
    n.row<-dim(ddd)[1]-j+1

    #Create the elements of sample space: each row of the monitor.ss is an element of the sample space
    for (i in 1:j){
        monitor.ss[,i]<-i:(n.row-1+i)
    }
    rm(i)

    #adds if the each of the possible samples of the sample space at least one event was observed or not
    monitor.ss[,j+1]<-apply(monitor.ss,1,function(x) {
        r.low<-range(x,na.rm=T)[1]
        r.high<-range(x,na.rm=T)[2]
        return(as.numeric(sum(ddd$positive[r.low:r.high])>0))})

suc.list<-rep(NA, iter) #Initiate a vector to keep track of successes

    #Now the sampling takes place

    k<-s.freq #number of samples drawn from the sample space
    for (i in 1:iter) { #for each iteration

        #do not convert to data.frame
        sampling.start<-monitor.ss

        k.t=0 #controller to break the while loop
        suc<-0

        #use & to stop the loop if one condition is not fullfilled, second condition changed because a matrix is used instead of a data.frame
        while (k.t<=k & !is.null(nrow(sampling.start))) { #breaks the while loop if k reaches the specified limit (k=s.freq) or if the reduction of the sample space (see below) does not leave any elements in the sample space
            #set.seed(k.t*i) #use set.seed to test if results are identical
            s<-sampling.start[sample(nrow(sampling.start),1),] #samples the first trial
            suc<-suc+s[j+1] #adds up if there is a success or not ( a tleast one event in the sample was observed)
            sampling.start <- sampling.start[apply(sampling.start, 1, function(x) !any(x %in% as.numeric(s[1:j]))),] #sample space reduction: Elements of the sample space that intersect with the sample that was just selected are removed from the sample space before the next sampling occurs
            k.t<-k.t+1 #controller for the wile loop
        }

        suc.list[i]<-(suc/k.t)

    }
    print(Sys.time()-start)
    message("")
    #hist(suc.list, breaks=sqrt(iter))
    return(suc.list)
}
kkk<-data.frame(days=1:100,positive=rbinom(100,1,0.05))
res1<-my.sampling1(kkk$days, kkk$positive, s.length=3, s.freq=5, iter=20)

This is already faster. To make it even more efficient, I would try to avoid apply inside the loop (could an algebraic operation be used instead?) and try to vectorize. Furthermore you can tidy up. There are some unnecessary assignments inside the function.


Adding time offsets to create future dates How can I shorten this (working) code to create future dates in ruby for use as params in URLs?

I'm doing:

require 'cgi'
require 'time'

now=Time.new()
now_year=now.strftime('%Y').to_i
now_month=now.strftime('%m').to_i
now_day=now.strftime('%d').to_i
now_hour=now.strftime('%H').to_i

start_hour=(now_hour + 1)
end_hour=(now_hour + 2)

start_datetime=Time.new(now_year, now_month, now_day, start_hour)
end_datetime=Time.new(now_year, now_month, now_day, end_hour)

starts=CGI.escape(start_datetime.utc.strftime("%Y-%m-%dT%H:%M:%S.%3NZ"))
ends=CGI.escape(end_datetime.utc.strftime("%Y-%m-%dT%H:%M:%S.%3NZ"))

and it works, but it seems long and clumsy.  Is there an easier way?

You can add seconds directly to a Time object, so you could do:

now = Time.now.utc
starts = now + 3600 # Add 1 hour's worth of seconds (60 * 60)
ends   = now + 7200 # Add 2 hours

and then do the strftime-formatting.

Of course, since you need to do this twice, I'd suggest adding a method to avoid the repetition, i.e.:

def offset_utc_timestamp(offset, format = "%Y-%m-%dT%H:%M:%S.%3NZ")
  time = Time.now.utc + offset
  time.strftime(format)
end

You may want to split it up differently (i.e. have the method just return a Time object, which you make into a string elsewhere; or have the method also do the URL escaping).

I'd also recommend using the URI.escape method instead, if this is intended for URLs

With all that your code becomes:

starts = URI.escape(offset_utc_timestamp(3600))
ends   = URI.escape(offset_utc_timestamp(7200))

Alternatively, the ActiveSupport gem has a ton of helpful time stuff, letting you do things like 2.hours.from_now.utc