Password Protect Page with PHP I am using the following code to password protect pages on my site. This is how I am including it on a page:

<?php include("/path/to/password_protect.php"); ?>

After that line I have the rest of the page code, which is not displayed if the user is not logged in.

This is the password protect code itself, included on every page I want secured:

password_protect.php

<?php

#  SETTINGS START

$LOGIN_INFORMATION = array(
  'admin' => 'admin'
);
// request login? true - show login and password boxes, false - password box only
define('USE_USERNAME', true);
// User will be redirected to this page after logout
define('LOGOUT_URL', '/hub');
// time out after NN minutes of inactivity. Set to 0 to not timeout
define('TIMEOUT_MINUTES', 0);
// This parameter is only useful when TIMEOUT_MINUTES is not zero
// true - timeout time from last activity, false - timeout time from login
define('TIMEOUT_CHECK_ACTIVITY', true);

#  SETTINGS END

// timeout in seconds
$timeout = (TIMEOUT_MINUTES == 0 ? 0 : time() + TIMEOUT_MINUTES * 60);
// logout?
if(isset($_GET['logout'])) {
  setcookie("verify", '', $timeout, '/'); // clear password;
  header('Location: ' . LOGOUT_URL);
  exit();
}
if(!function_exists('showLoginPasswordProtect')) {
// show login form
function showLoginPasswordProtect($error_msg) {
?>

  <title>Please enter password to access this page</title>

  <form method="post">

Please enter password to access this page

    <?php echo $error_msg; ?>

<?php if (USE_USERNAME) echo 'Username:
<input type="input" name="access_login" />
Password:
'; ?>
    <input type="password" name="access_password" />

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

<?php
  // stop at this point
  die();
}
}
// user provided password
if (isset($_POST['access_password'])) {
  $login = isset($_POST['access_login']) ? $_POST['access_login'] : '';
  $pass = $_POST['access_password'];
  if (!USE_USERNAME && !in_array($pass, $LOGIN_INFORMATION)
  || (USE_USERNAME && ( !array_key_exists($login, $LOGIN_INFORMATION) || $LOGIN_INFORMATION[$login] != $pass ) )
  ) {
    showLoginPasswordProtect("Details Incorrect");
  }
  else {
    // set cookie if password was validated
    setcookie("verify", md5($login.'%'.$pass), $timeout, '/');

    // Some programs (like Form1 Bilder) check $_POST array to see if parameters passed
    // So need to clear password protector variables
    unset($_POST['access_login']);
    unset($_POST['access_password']);
    unset($_POST['Submit']);
  }
}
else {
  // check if password cookie is set
  if (!isset($_COOKIE['verify'])) {
    showLoginPasswordProtect("");
  }
  // check if cookie is good
  $found = false;
  foreach($LOGIN_INFORMATION as $key=>$val) {
    $lp = (USE_USERNAME ? $key : '') .'%'.$val;
    if ($_COOKIE['verify'] == md5($lp)) {
      $found = true;
      // prolong timeout
      if (TIMEOUT_CHECK_ACTIVITY) {
        setcookie("verify", md5($lp), $timeout, '/');
      }
      break;
    }
  }
  if (!$found) {
    showLoginPasswordProtect("");
  }
}
?>

This is not to secure anything highly sensitive (e.g. credit card details), but is to password protect analytics dashboards for client websites. To that end it needs to be moderately secure. My questions would be:

1) Are there any improvements I could make to the code to make it more streamlined?

2) Are there any large security issues that I need to address? As I said above it is not protecting hugely sensitive data but if there is a major problem or an easy improvement that would be great.

Thanks for any feedback!

I have 2 concerns with your code

Mixing PHP and HTML works, but it's difficult to follow (which means bug-prone, not scalable).  It's much better to separate your business logic from the display as much as possible.
Having a default login of admin:admin is definitely not a good idea. It's probably the first thing anyone without credentials would try.

As @fracz suggested, HTTP Basic Authentication would fit your needs well, and it's really easy to use. I would probably do something like:

<?php
$LOGIN_INFORMATION = [
    'joe' => 'fkwoeiwsfrj3', 'jill' => '9ioDdoODidosD'
];

/**
 * Returns TRUE if authentication is successful. FALSE otherwise.
 * @param array $registeredLogins All acceptable logins
 * @return bool
 */
function isAuthenticated($registeredLogins){

    $authenticated = null; //make true if authentication succeeds

    //Server vars below will contain what user entered in the login prompt
    $login = isset($_SERVER['PHP_AUTH_USER'])? $_SERVER['PHP_AUTH_USER']: null;
    $pass = isset($_SERVER['PHP_AUTH_PW'])? $_SERVER['PHP_AUTH_PW']: null;

    if (!$login || !$pass):
        //required username or password is missing
        $authenticated = false;
    else:
        //username and password were provided. Are they accurate?
        $authenticated = array_key_exists($login, $registeredLogins)
                            && $registeredLogins[$login] === $pass;
    endif;

    return $authenticated;
}

/**
 * Exits and causes the browser to show user a login prompt
 */
function requestCredentials(){
    header('WWW-Authenticate: Basic realm="Login to Analytics Dashboard"');
    http_response_code(401);
    die("Invalid username or password");
}

//check for authentication. Require it if missing.
if(!isAuthenticated($LOGIN_INFORMATION)) requestCredentials();
?>

<html>
<!-- your page goes here. Will only display if user is authenticated -->
</html>

I have simplified things a bit, so you will need to adjust it to your specific situation. With this scheme, once the user enters username and password that is accepted, the browser will keep automatically sending credentials with new requests made to the same domain. Here are the details.

A couple of important things to keep in mind:

Whatever your security logic, if you don't use TLS (HTTPS), the client and the site will be vulnerable.  Any attacker can just read your user's verify cookie, or the username and password transmitted. The attacker can then use this info to impersonate the user and gain access. This affects both your code and what I suggested. Use TLS if you can. It's free.
Basic Authentication doesn't have a clean logout system (a way for the server to tell the browser to stop sending credentials and to delete them).  This can be worked  around easily enough though. The reference I linked above goes over this. Have a logout function that redirects the user (by setting the Location header and then exiting) to a page on the same domain with bad credentials hard-coded; eg: header("Location: http://badusername:badpass@www.example.com/logout.html");exit. Call the logout function before you echo anything to the browser and the browser will forget the old credentials, remember the bad credentials and redirect the user to logout.html.  User will be forced to enter proper credentials again if he wants access.
Because browsers will automatically attach credentials to all requests made to the domain, even the pages where you do not include password_protect.php will be queried with these credentials. This means that with this solution, TLS should be used not just on the protected pages, but on all domain's pages.


Summations zu.mpfr and zstar based on a probability sample Here is my code:

library(gtools)
library(Rmpfr)
library(OBsMD)
precBits=1000000
K=9
n=1000
p=mpfr(c(K:1),precBits = precBits);p=mpfr(p,precBits = precBits)/mpfr(sum(p),precBits = precBits)
#p is a vector of probabilities

zu.mpfr <- function(freq, u, precBits){
  sum=mpfr(0,precBits = precBits)
  n=sum(freq)
  for (i in (1:length(freq))){
    prod=mpfr(1,precBits = precBits)
    for (j in (0:(u-1))){
      prod=prod*(mpfr(freq[i]-j,precBits = precBits))/mpfr(n-j,precBits=precBits)
    }
    sum=sum+prod
  }
  return(sum)
}

etastar<-rep(mpfr(0,precBits = precBits),K-1)
etastar=c(etastar)
for (i in 1:(K-1)){
  etastar[i]=sum(p^mpfr(i+1,precBits))
}

In the above codes, p is a vector of probabilities. zu.mpfr is \$z_u =\sum_k \prod_{j=0}^{u-1}\frac{Y_k-j}{n-j},\$ where \$u\$ can be any positive integer (but no more than \$n\$), and \$Y_k\$ is the observed frequency in a random sample for catagory \$k\$. The inputs for zu.mpfr are the sample frequency counts, \$u\$, and precBits (the preset accuracy). For the frequency, for example, if a sample is \$\{1,1,2,3,3,3,4\}\$, then the corresponding frequencies are \$\{2,1,3,1\}\$, since observation "1" appears twice, "2" appears once, "3" appears three times, and "4" appears once. The frequency is directly generated by rmultinom in the next paragraph of codes. etastar is a vector of length \$K-1\$. And the \$i\$-th element, \$\eta^{*}(i)\$, is \$\sum_k p_k ^{i+1}.\$ As a summary, zu.mpfr is a value calculated from a random sample, and etastar is a vector calculated from the probability distribution p.

Following is the part that I need to make it faster:

start.time<-Sys.time()
sample=rmultinom(1,n,asNumeric(p))
zstar<-rep(mpfr(0,precBits = precBits),K-1)
zstar=c(zstar)
for(i in 1:(K-1)){
  zstar[i]=zu.mpfr(sample,i+1,precBits)
}

end_time <- Sys.time()
end_time-start.time

In the latter codes, I used rmultinom to get the sample, which is the frequency counts from a random sample that generated by the probability distribution p. Then I calculated zstar, which is a vector of length \$K-1\$, with the \$i\$-th element zu.mpfr(sample, i+1, precBits). Here comes my problem. It takes too long to calculate the vector zstar. By timing it, "Time difference of 6.207007 secs" on my end. I am wondering if I can make it better. I need it faster because I need to run the simulation with thousands of iterations. I am not sure if the slowness is caused by mpfr but I do need it to enforce accuracy.

Please let me know if any further clarification is needed.

First suggestion would be to write a function for the high precision conversion because you keep using mpfr(..., precBits = precBits) over and over:

Z <- function(x) mpfr(x, precBits = 1000000)

It won't make your code faster but it will save you a lot of typing and make it easier on the eye.

Next, if you do not know why your code is slow, you need to learn to use the profiler:

Rprof(tmp <- tempfile())
[INSERT_YOUR_CODE_HERE]
Rprof()
summaryRprof(tmp)
unlink(tmp)

It will show you that 90% of the time is spent doing the division in your prod = prod * ... line of code. So a solution would be to compute the numerator (num) and denominator (den) by iteration, then do a single division at the end:

zu.mpfr <- function(freq, u) {
  tot <- Z(0)
  n <- sum(freq)
  for (i in seq_along(freq)) {
    num <- Z(1)
    den <- Z(1)
    for (j in (0:(u-1))) {
      num <- num * Z(freq[i] - j)
      den <- den * Z(n - j)
    }
    prod <- num / den
    tot <- tot + prod
  }
  return(tot)
}

This alone makes your code three to four times faster.

Next, I would recommend looking for places where you can vectorize. I understand that your code is dealing with high precision numbers so maybe there is no other way than using for loops if only the simple operators (+, *, /) are available... However, it appears that if x is a vector of mpfr numbers, prod(x) also returns a mpfr number so I am ready to trust it is doing the right thing. If so, then you could do:

zu.mpfr <- function(freq, u) {
  tot <- Z(0)
  n <- sum(freq)
  J <- 0:(u-1)
  for (i in seq_along(freq)) {
    tot <- tot + prod(Z(freq[i] - J)) / prod(Z(n - J))
  }
  return(tot)
}

Have you noticed that we are computing the same denominator over and over? Move it outside the loop to save a little more:

zu.mpfr <- function(freq, u) {
  tot <- Z(0)
  n <- sum(freq)
  J <- 0:(u-1)
  den <- prod(Z(n - J))
  for (i in seq_along(freq)) {
    tot <- tot + prod(Z(freq[i] - J)) / den
  }
  return(tot)
}

Last, if memory size is not an issue (i.e. K remains small), you could save even more time by working with matrices. It's a bit harder to digest (including the computation of row-wise cumulative products), but it's elegant how short your code becomes (notice we don't need zu.mpfr anymore). This version is 5 to 6 times faster than your original one:

  sample <- c(rmultinom(1, n, asNumeric(p)))
  x <- Z(outer(sample, 0:(K-1), FUN = "-"))
  y <- Z(sum(sample) - 0:(K-1))
  z <- apply(x, 1, cumprod) / cumprod(y)
  zstar <- tail(rowSums(z), -1)


Settings the height of rows for multiple screens within a TableViewController Is there a better way to write this? This is a class that displays a preferences menu. We do this when the user first installs the app during a walk-through where we only show them some of the settings with additional guidance on how to set them. We also show the entire preferences menu if the user selects our preferences button.

    /**
 Manages the height of the table's cells, hiding them if necessary.

 parameter: The table we'll edit
 parameter: The height in dpi of the row at a given row
*/

override func tableView(tableView: UITableView, heightForRowAtIndexPath indexPath: NSIndexPath) -> CGFloat {
    switch walkthroughStep {

        // step 1 of the walkthrough
        case 1:

            switch indexPath.row {
            case 0: return 0.1
            case 3:
                if ((promptLabel.intrinsicContentSize().height * promptLabel.font.pointSize) > 350){
                    //print ("the result of this is:  ---------- \(promptLabel.intrinsicContentSize().height * promptLabel.font.pointSize)")
                    return 80
                }
                else { return 50.0 }
            case 4: return 9.0
            case 5: return 0.1
            case 6: return 0.1
            case 7: return 0.1
            case 8: return 0.1
            case 9: return 0.1
            case 10: return 0.1
            case 11: return 0.1
            default: break
        }

        // step 2 of the walkthrough
        case 2: switch indexPath.row {
            // hide all cells of teh default methos table
            case 0: return 0.1
            case 1: return 0.1
            case 2: return 0.1
            case 3: return 0.1
            case 4: return 0.1
            case 5:
                if enableNotifications.on {
                    return selectedCellHeight
                }
                return unselectedCellHeight
            case 6: return 9.0
                // hide propmt me to modify number cell
            case 7: return 0.1
            case 8: return 0.1
            case 9: return 0.1
            case 10: return 0.1
            case 11: return 0.1
            default: break
        }

        // normal state
        default:
            switch indexPath.row {
            // hide all cells of teh default methos table
            case 0: return 50.0
            case 1: return 50.0
            case 2: return 50.0
            case 3:
                if ((promptLabel.intrinsicContentSize().height * promptLabel.font.pointSize) > 350){
                    //print ("the result of this is:  ---------- \(promptLabel.intrinsicContentSize().height * promptLabel.font.pointSize)")
                    return 80
                }
                else { return 50.0 }
            case 4: return 9.0
            case 5:
                if enableNotifications.on {
                    return selectedCellHeight
                }
                return unselectedCellHeight
            case 6: return 9.0
                // hide propmt me to modify number cell
            case 7:
                if ((modifyNumbersLabel.intrinsicContentSize().height * modifyNumbersLabel.font.pointSize) > 310){
                    //print ("the result of this is:  ---------- \(modifyNumbersLabel.intrinsicContentSize().height * modifyNumbersLabel.font.pointSize)")
                    return 90
                }
                else { return 50.0 }
            case 8: return 9.0
            case 9: return 50.0
            case 10: return 50.0
            case 11: return 50.0
            default: break
        }
    }

    return 50.0
}

I would split the table into multiple sections and set the number of rows to 0 for those sections I do not want to display and only use the default settings in heightForRowAtIndexPath

override func tableView(tableView: UITableView, heightForRowAtIndexPath indexPath: NSIndexPath) -> CGFloat {

    switch indexPath.section {
    // hide all cells of teh default methos table
    case 0: return 50.0
    case 1:
        if ((promptLabel.intrinsicContentSize().height * promptLabel.font.pointSize) > 350){
            //print ("the result of this is:  ---------- \(promptLabel.intrinsicContentSize().height * promptLabel.font.pointSize)")
            return 80
        } else { return 50.0 }
    case 2: return 9.0
    case 3:
        if enableNotifications.on {
            return selectedCellHeight
        }
        return unselectedCellHeight
    case 4: return 9.0
    // hide propmt me to modify number cell
    case 5:
        if ((modifyNumbersLabel.intrinsicContentSize().height * modifyNumbersLabel.font.pointSize) > 310){
            //print ("the result of this is:  ---------- \(modifyNumbersLabel.intrinsicContentSize().height * modifyNumbersLabel.font.pointSize)")
            return 90
        } else { return 50.0 }
    case 6: return 9.0
    case 7: return 50.0
    default: break
    }

return 50.0
}

And implement these functions:

override func numberOfSectionsInTableView(tableView: UITableView) -> Int {
    return 8
}

override func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
    switch section {
    case 0:
        // Original Rows 0, 1, 2
        switch walkthroughStep {
        case 1,2: return 0
        default: return 3
        }
    case 1:
        // Original Row 3
        switch walkthroughStep {
        case 2: return 0
        default: return 1
        }
    case 2:
        // Original Row 4
        switch walkthroughStep {
        case 2: return 0
        default: return 1
        }
    case 3:
        // Original Rows 5
        switch walkthroughStep {
        case 2: return 0
        default: return 1
        }
    case 4:
        // Original Row 6
        switch walkthroughStep {
        case 1: return 0
        default: return 1
        }
    case 5:
        // Original Row 7
        switch walkthroughStep {
        case 1,2: return 0
        default: return 1
        }
    case 6:
        // Original Row 8
        switch walkthroughStep {
        case 1,2: return 0
        default: return 1
        }
    case 7:
        // Original Rows 9, 10, 11
        switch walkthroughStep {
        case 1,2: return 0
        default: return 5
        }
     default:
         return 0
    }

So most of the display logic is in the numberOfSectionsInTableView method.


Parse in currency rates of major cities I'm creating a website that parses in currency rates of the three major cities listed below in the $cities array.

<?php

// Feed URL's //
$theMoneyConverter = 'http://themoneyconverter.com/rss-feed/';

// Define arrays //
$cities = array(
    'London',
    'New York',
    'Paris'
);

$currencyCode = array(
    'GBP',
    'USD',
    'EUR'
);

// Arguments parsed onto theMoneyConverter URL to set position of the <item> tags declared in the XML document
$currencySource = array(
    $theMoneyConverter . $currencyCode[0] . '/rss.xml?x=15',
    $theMoneyConverter . $currencyCode[1] . '/rss.xml?x=16',
    $theMoneyConverter . $currencyCode[2] . '/rss.xml?x=56'
);
?>

Above is my configuration script that a developer will use if they wish to drop a city or add a new one.  Notice in my $currencySource array that I add arguments to the end of each URL.  For example, at index 0 in the array I add ?x=15.  This argument corresponds to the <item> element from the original source feed from themoneyconvert.com.  In the case of ?x=15, this would be the 15th <item> element from the XML document.

The XML document to confirm this.  The 15th  element contains the following element just to clarify:

<item>
<title>EUR/GBP</title>

The following is stored in a separate script called getCurrency.php:

function get_currency_rate($currencySource) {

      try {
        $xml = new SimpleXmlElement(file_get_contents($currencySource));
    } catch (Exception $e) {
        echo 'Caught exception: An error has occured. Unable to get file contents';
    }

    $vars = parse_url($currencySource, PHP_URL_QUERY);
    parse_str($vars);
    switch ($x) {
        case 15:
            get_rate($xml, 15); //EUR 15
            get_rate($xml, 56); //USD 56
            break;
        case 16:
            get_rate($xml, 16); //GBP 16
            get_rate($xml, 15); //EUR 15
            break;
        case 56: default :
            get_rate($xml, 15); // EUR 15
            get_rate($xml, 56); // USD 56
            break;
    }
}

// Get and return currency rate
function get_rate(SimpleXMLElement $xml, $x) {

    $currency['rate'] = $xml->channel->item[$x]->description;

    echo $currency['rate'], '<br />';
}

I call the get_currency_rate() function from my user interface like such:

echo get_currency_rate($currencySource[$index])

After getting the file contents I extract the argument that I added to the URL from the configuration file listed above and store it as a variable $x.  The XML feed and variable $x are parsed into the get_rate function().

The whole process doesn't seem very maintainable and particular readable.  Adding a new city into the equation would become a nightmare for another developer.  Can anyone think of any other alternatives?

I see some inconsistencies in your getCurrency.php code. There are many variables that are never declared within the functions, so I don't think that's going to work as expected.

I think the best approach would be to make a parser which looks for whatever currency code you need, regardless of the item order because that might change in the future. I mean, the most performance-expensive part of the job is to parse the whole XML into a SimpleXML structure which you're doing anyway, so looping through the items seaching for the right one wouldn't be much worse and your code will be more straight-forward and escalable.

On the other hand, you might also want to have some cache of the parsed XML to avoid re-parsing the same file on each request. But that depends on the variability of the XML feed. Maybe you can check with The Money Converter how often the feed is updated.


Clean Code attempt of HOLES problem on codechef.com The problem asks you to take read an integer (number of words to read) and then process words entered counting the number of "HOLES" exist.  Letters with HOLES are A B D O P Q and R.  Of those B has 2 holes A D O P Q R have 1 and any other letter not listed counts as 0 even lowercase equivalents.

The scope is fairly limited so try to just assess the code based on the problem listed.

Full problem description: here

#include <iostream>
#include <map>
#include <string>

class LetterEvaluator {
public:
    LetterEvaluator()
    :   letterValues(createMap())
    {}

    int getValueOf(const char letter) const {
        if(retrieveValueOfLetter(letter) == letterValues.end())
            return 0;
        else{
            return retrieveValueOfLetter(letter)->second;
        }
    }

private:
    const std::map<char,int> letterValues;

    std::map<char,int> createMap() const {
        std::map<char,int> initializedMap;
        initializedMap['A'] = 1;
        initializedMap['B'] = 2;
        initializedMap['D'] = 1;
        initializedMap['O'] = 1;
        initializedMap['P'] = 1;
        initializedMap['Q'] = 1;
        initializedMap['R'] = 1;
        return initializedMap;
    }

    std::map<char,int>::const_iterator retrieveValueOfLetter(const char letter) const {
        return letterValues.find(letter);
    }
};

class HolesInCapitalizedWordFinder {
public:
    HolesInCapitalizedWordFinder()
        :   numberOfHoles(0)
        ,   letterEvaluator()
    {}

    void processWord() {
        evaluate(readWord());
        printNumberOfHolesInWord();
    }

private:
    LetterEvaluator letterEvaluator;
    int             numberOfHoles;

    std::string readWord() const {
        std::string wordToEvaluate;
        std::cin >> wordToEvaluate;
        return wordToEvaluate;
    }

    void evaluate(const std::string& wordToEvaluate ) {
        numberOfHoles = 0;
        for(auto& character : wordToEvaluate) {
            if(isNotAnUpperCaseLetter(character)){
                numberOfHoles = -1;
                break;
            }
            numberOfHoles += letterEvaluator.getValueOf(character);
        }
    }

    bool isNotAnUpperCaseLetter(const char character) const{
        return character < 65 || character > 90;
    }

    void printNumberOfHolesInWord() const {
        if(numberOfHoles == -1) {
            std::cout << "Not all characters in that word are uppercase letters!\n";
        }
        else {
            std::cout << numberOfHoles << '\n';
        }
    }
};

int main() {
    std::ios::sync_with_stdio(false);

    int timesToRun = 0;
    std::cin >> timesToRun;

    HolesInCapitalizedWordFinder holesInCaptilizedWordFinder;

    for(int numberOfTimesRan = 1; numberOfTimesRan <= timesToRun; ++numberOfTimesRan) {
        holesInCaptilizedWordFinder.processWord();
    }
    return 0;
}

LetterEvaluator

Since this is a C++11 solution, you no longer need something like createMap():

std::map<char,int> createMap() const {
        std::map<char,int> initializedMap;
        initializedMap['A'] = 1;
        initializedMap['B'] = 2;
        initializedMap['D'] = 1;
        initializedMap['O'] = 1;
        initializedMap['P'] = 1;
        initializedMap['Q'] = 1;
        initializedMap['R'] = 1;
        return initializedMap;
    }

Use C++11's list initialization to have it initialized in place:

const std::map<char, int> letterMap {
    {'A', 1},
    {'B', 2},
    {'D', 1},
    {'O', 1},
    {'P', 1},
    {'Q', 1},
    {'R', 1}
};

If you have C++14, you can avoid providing an explicit return type:

std::map<char,int>::const_iterator retrieveValueOfLetter(const char letter) const {
        return letterValues.find(letter);
    }

You can use automatic return type deduction via auto:

auto retrieveValueOfLetter(const char letter) const {
    return letterValues.find(letter);
}

This does also shorten the line, which could help add more readability.
You have two redundant calls to retrieveValueOfLetter():

int getValueOf(const char letter) const {
        if(retrieveValueOfLetter(letter) == letterValues.end())
            return 0;
        else{
            return retrieveValueOfLetter(letter)->second;
        }
    }

Just retrieve the value from one call and then return the appropriate value:

int getValueOf(const char letter) const {
    const auto retrievedLetter = retrieveValueOfLetter(letter);
    return (retrievedLetter == letterValues.end()) ? 0 : retrievedLetter->second;
}

I've used a ternary here to shorten the expression to just one line.  If it looks less-readable, or if you may need to expand on it later, you may still use if/else instead.

HolesInCapitalizedWordFinder

It looks like readWord() should be a free function (non-member function).  However, if you're trying to read in a word, then you can just overload operator>> for this class.
Do you really need isNotAnUpperCaseLetter()?  There's already std::isupper() provided in the standard library, which should be safer to use.  You may encounter some issues with your own, depending on how it's implemented.


Group built in types to category I've created the following program to assist me in some template stuff I'm working on.

The function get_type returns 0, 1, 2, 3 or nullopt corresponding to which group the argument T belongs to.

This is my first template style project, so I'm looking for any critique possible. (Note the functions are marked static intentionally, as this will only be used inside this particular file, and should not be visible to users.)

namespace ae
{
    namespace details
    {
        template <class T>
        [[nodiscard]]
        static inline constexpr auto is_integer() noexcept
        {
            return std::is_same_v<T, short>
                || std::is_same_v<T, int>
                || std::is_same_v<T, long>
                || std::is_same_v<T, long long>
                || std::is_same_v<T, unsigned short>
                || std::is_same_v<T, unsigned int>
                || std::is_same_v<T, unsigned long>
                || std::is_same_v<T, unsigned long long>;
        }

        template <class T>
        [[nodiscard]]
        static inline constexpr auto is_floating_point() noexcept
        {
            return std::is_same_v<T, float>
                || std::is_same_v<T, double>
                || std::is_same_v<T, long double>;
        }

        template <class T>
        [[nodiscard]]
        static inline constexpr auto is_charachter() noexcept
        {
            return std::is_same_v<T, char>
                || std::is_same_v<T, unsigned char>
                || std::is_same_v<T, wchar_t>;
        }

        template <class T>
        [[nodiscard]]
        static inline constexpr auto is_boolean() noexcept
        {
            return std::is_same_v<T, bool>;
        }

        // Returns 0 for integer, 1 for floating_point,
        // 2 for charachter, 3 for boolean, and nullopt otherwise
        template <class T>
        [[nodiscard]]
        static inline constexpr auto get_type() noexcept
        {
            if constexpr (is_integer<T>())
                return std::optional<unsigned int>(0u);
            else if constexpr (is_floating_point<T>())
                return std::optional<unsigned int>(1u);
            else if constexpr (is_charachter<T>())
                return std::optional<unsigned int>(2u);
            else if constexpr (is_boolean<T>())
                return std::optional<unsigned int>(3u);
            else return std::nullopt;
        }
    }
}

Example usage

template <typename T>
static typename std::enable_if<ae::details::is_integer<T>(), T>::type test()
{
    // Do something if T is integer
}

template <typename T>
static typename std::enable_if<ae::details::is_charachter<T>(), T>::type test()
{
    // Do something if T is charachter
}

int main()
{
    test<int>(); //..

    return 0;
}

In is_charachter (which is misspelled; it should be is_character), you left out signed char (which is a distinct type from char). The same applies for all of the fixed width types (char8_t, char16_t, char32_t).

get_type returns some arbitrary magic number values.  You could create named constants to represent the type.  If you add a "unknown_type" value, then you wouldn't need to use std::optional.


Generating a random hex color Is there any better way of doing this?

function getRandomColor() {
    var chars = '0123456789ABCDEF'.split('');
    var hex = '#';
    for (var i = 0; i < 6; i++) {
        hex += chars[Math.floor(Math.random() * 16)];
    }
    return hex;
 }

Hex colors can be in 3 digits or 6 digits. In the case where you want 3 digits, you can't do it because 6 is hardcoded. You may want to move it into a variable with a default value of 6.

Strings characters in JS are accessible via indices the same way as an array. So splitting the characters into an array is unnecessary, not to mention the additional memory consumed by creating an array.

Adding | 0 after a number truncates the decimal places from the number. Internally, it converts the internal representation of the number from a float into an integer, dropping the decimal information. So basically, it does the same thing as Math.floor, albeit in a more vague but shorter way.

You could also use reverse looping, and using the length as the condition. When length is zero, it's essentially falsy, thus stopping the loop.

function getRandomColor() {
  var length = 6;
  var chars = '0123456789ABCDEF';
  var hex = '#';
  while(length--) hex += chars[(Math.random() * 16) | 0];
  return hex;
}

document.write(getRandomColor());


Big-O of Kruskal/Prim Algorithm I made simple Kruskal/Prim algorithm with a graph constructed by nested Python dictionary.

Kruskal

parent = {}
rank = {}

def find(v):
    if parent[v] != v:
        parent[v] = find(parent[v])

    return parent[v]

def union(v, u):
    root1 = find(v)
    root2 = find(u)

    if rank[root1] > rank[root2]:
        parent[root2] = root1
    else:
        parent[root1] = root2
        if rank[root1] == rank[root2]:
            rank[root2] += 1

def kruskal(graph):
    for v in graph.keys():
        parent[v] = v
        rank[v] = 0

    edges = []
    mst = []

    for outer_key in graph.keys():
        for inner_key, inner_weight in graph[outer_key].items():
            edges.append((inner_weight, outer_key, inner_key))

    edges.sort()

    for edge in edges:
        weight, v, u = edge

        if find(v) != find(u):
            union(v, u)
            mst.append(edge)

    return mst

if __name__ == '__main__':
    arg_graph = {
        'A': {'B': 10, 'D': 5},
        'B': {'A': 10, 'C': 2, 'D': 7, 'E': 12},
        'C': {'B': 2, 'E': 11, 'F': 14},
        'D': {'A': 5, 'B': 7, 'E': 6, 'G': 9},
        'E': {'B': 12, 'C': 11, 'D': 6, 'F': 15},
        'F': {'C': 14, 'E': 15, 'G': 3},
        'G': {'D': 9, 'F': 3}
    }

    print(kruskal(arg_graph))

Prim

import queue

def prim(graph):
    que = queue.PriorityQueue()
    mst = []
    visited = set()

    starting_vertex = list(graph.keys())[0]
    visited.add(starting_vertex)

    for next_to, weight in graph[starting_vertex].items():
        que.put((weight, starting_vertex, next_to))

    while que.empty() is False:
        edge = que.get()
        weight, frm, to = edge

        if to in visited:
            continue

        visited.add(to)
        mst.append(edge)

        for next_to, weight in graph[to].items():
            if next_to not in visited:
                que.put((weight, to, next_to))

    return mst

if __name__ == '__main__':
    arg_graph = {
        'A': {'B': 10, 'D': 5},
        'B': {'A': 10, 'C': 2, 'D': 7, 'E': 12},
        'C': {'B': 2, 'E': 11, 'F': 14},
        'D': {'A': 5, 'B': 7, 'E': 6, 'G': 9},
        'E': {'B': 12, 'C': 11, 'D': 6, 'F': 15},
        'F': {'C': 14, 'E': 15, 'G': 3},
        'G': {'D': 9, 'F': 3}
    }

    print(prim(arg_graph))

With this algorithm, minimun spanning tree is made like this:

"""
        10         2                                   2
  [A]--------[B]-------[C]            [A]        [B]-------[C]
   |    7   / |   11  / |              |    7   /
 5 | +-----+ 12  +---+  | 14         5 | +-----+
   |/         | /       |              |/
  [D]--------[E]-------[F]    ===>    [D]--------[E]       [F]
   |     6         15   |              |     6              |
   +----+           +---+              +----+           +---+
      9 |           | 3                   9 |           | 3
        +----[G]----+                       +----[G]----+

                                       (Minimum Spanning Tree)
"""

People says time complexity of Kruskal's Algorithm is \$O(ElogE)\$ and Prim's Algorithm is \$O(ElogV)\$. (E = number of edges, V = nubmer of vertices)

Then, does my code also have same complexity?

My Kruskal's Algorithm seems have \$O(V^2)\$ because of nested for-loop, and I can't figure out how complex my Prim's Algorithm is.

def kruskal(graph):
    for v in graph.keys():      # <- O(V)
        parent[v] = v
        rank[v] = 0

    edges = []
    mst = []

    # this for loop creates a list of all edges O(E)
    for outer_key in graph.keys():
        for inner_key, inner_weight in graph[outer_key].items():
            edges.append((inner_weight, outer_key, inner_key))

    edges.sort()    # <- this is O(ElogE) and where the complexity comes from

    for edge in edges:
        weight, v, u = edge

        if find(v) != find(u):
            union(v, u)           # <- union-find, let's check this in a second
            mst.append(edge)
                                  # ... so the whole for loop is O(E*O(union))
    return mst

So, your overall complexity is O(ElogE + E*O(union))

Let's look at your union-find implementation:

def find(v):   # find with path compression
    if parent[v] != v:
        parent[v] = find(parent[v])

    return parent[v]

def union(v, u):    # union by rank
    root1 = find(v)
    root2 = find(u)

    if rank[root1] > rank[root2]:
        parent[root2] = root1
    else:
        parent[root1] = root2
        if rank[root1] == rank[root2]:
            rank[root2] += 1

According to Wikipedia, this implementation is in O(inverse Ackermann(n)), so that means for the overall implementation we get

O(ElogE), which is purely driven by the sort, or as Wikipedia says

O(T_sort(E) + E*(inverse Ackermann(V)))

In other words, your kruskal algorithm is fine complexity-wise.

Your Prims algorithm is O(ElogE), the main driver here is the PriorityQueue. Notice that your loop will be called O(E) times, and the inner loop will only be called O(E) times in total. So the main driver is adding and retriveving stuff from the Priority Queue. The operations on the PriorityQueue are in O(logE), so O(ElogE) in total for the whole loop.


Fix last n commits to previous commit automatically I often find myself doing rebase manually in an interactive form in a way as follows. Say I want to squash two last commit to third commit.
First I execute:
$ git rebase -i HEAD~3

Now editor pops up, I mark two last commits as fixup. I close the editor and then rebasing is done (if there are no errors). I'd like to automate this by executing a script with a parameter that gives a number of commits, i.e. when 2 is given as parameter, last two commits are fixup to third commit. Please see an example:

some other commits...
add nice feature
fix 1
fix 2

In that scenario I'd like add nice feature to incorporate changes from fix 1 and fix 2, and I like to fix 1 and fix 2 not be present at all.
I wrote a bash script that does what I want. It works seems to work fine.
UPDATE
In this review, I'm most interested in knowing whether my approach to git is a proper one, especially if it doesn't break anything during rebasing.
fixup() {
  local no_of_commits="$1"
  if [ -z "$no_of_commits" ]; then
    echo "You must provide a number of commits to fixup!"
    return 1
  elif ! [[ $1 =~ ^[0-9]+$ ]]; then
    echo "$no_of_commits is not a number!"
    return 2
  fi

  #git stash save

  git reset --soft "HEAD~$(no_of_commits)" &&
    git add --all &&
    git commit --fixup "$(git rev-parse HEAD)" &&
    GIT_SEQUENCE_EDITOR=true git rebase --interactive --autosquash --no-fork-point "$(git rev-parse HEAD~2)" &&
    echo "Rebased $(no_of_commits) succesfully!"

  #git stash pop
}

The error messages should go to standard error, rather than standard output:

  if [ -z "$no_of_commits" ]; then
    echo "You must provide a number of commits to fixup!" >&2
    return 1                                              ### HERE

I don't think there's a good case for returning distinct error codes for missing argument and invalid argument.  Are you ever going to make use of the different status values?


Recursive autoloader to find PHP files I created a small working autoloader based on some of the answers and ideas found on Stack Exchange (I lost the reference). However, I don't know much about PHP (I mainly know C, a bit C++ and Java). I'm trying to create a small MVC website for school project using my own templates and JavaScript as well. I mainly created this autoloader to include class files (I'm not sure if any other files can be included with autoloader too).

Can you please check this autoloader and give your insight on what can be done better and if there are any shortcomings?

It's important to know that I don't use namespaces currently because it would overcomplicate things.

<?php

    class Autoload
    {
        private $allFiles = array();

        public function __construct() {

            $this->findDir(__DIR__, $this->allFiles);
            spl_autoload_register(array($this, 'classLoader'));
        }

        private function classLoader($class) {

            if (class_exists($class, false)) {
                return true;

            } else {
                $this->includeClass($this->allFiles, $class);
                // should I return here true as well?
            }
        }

        private function findDir($currDir, &$files) {

            $dirsInCurr = glob($currDir . "/*", GLOB_ONLYDIR);
            $this->findFiles($currDir, $files);

            //recursive call
            foreach ($dirsInCurr as $dir) $this->findDir($dir, $files);
        }

        private function findFiles($dir, &$files) {

            $fPaths = glob($dir . "/*.php");
            foreach ($fPaths as $f) $files[] = $f . "\n";

        }

        private function includeClass($files, $class) {

            foreach ($files as $f) {
                if (strpos($f, strtolower($class) . ".php")) require_once trim($f);
            }
        }
    }

    $autoload = new Autoload();

?>

There are several ways to improve the code.
Let's start with the constructor.

You should not make any assumptions about the place of a file in the current file system, because that makes later changes very hard and it hurts reusability.
So get rid of the __DIR__ constant and send the directory of the classes as an argument to the constructor instead.

Registering callbacks and finding a file are two very different tasks. They don't belong to the same class. So register a method of an instance of the autoloader from the outside instead, and remove the call to spl_autoload_register() from your class.

You should use namespaces for all of your classes, interfaces and traits. This prevents collisions in case that you want to use other peoples code in your project later. Add a $namespace argument to your constructor.

In the end, your constructor shouldn't do more than setting up private members, maybe add some preparation for these. Example:
/**
 * Autoload constructor.
 *
 * @param string $namespace Pass the namespace unescaped.
 * @param string $dir
 */
public function __construct( $namespace, $dir )
{
    // Make sure it ends with a '\'.
    $namespace       = rtrim( $namespace, '\\' ) . '\\';
    $this->namespace = $namespace;
    $this->length    = strlen( $namespace );
    $this->dir       = rtrim( $dir, '/' ) . '/';
}

Now, to the actual class loader classLoader().

I wouldn't call it classLoader, because it will load traits and interfaces too. Just call it load(), that's clear enought for a class named Autoload. :) And because you will register and test it directly, make it public.

Do not return a value in that method. If you find a matching file, just include it and stop, return nothing otherwise. PHP will figure the rest out for you.

class_exists() is pointless here, because that method will be called only if the class/trait/interface doesn't exist yet. Just remove it.

The private methods called from that "parent" method don't deal with namespaces, and they are too slow. You don't need glob() here. (On a side note GLOB_ONLYDIR is not available on Solaris/Sun OS, use that constant only if portability is not a concern for your code.)
Figure out the correct complete file path first, then check if that file exists and is readable! The latter is important, because you cannot rely on the fact that you can actually include all existing files.

So that method may be reduced to something like this:
/**
 * @param string $search
 * @return void
 */
public function load( $search )
{
    if ( strncmp( $this->namespace, $search, $this->length ) !== 0 ) {
        return;
    }

    $name = substr( $search, $this->length );
    $path = $this->dir . str_replace( '\\', '/', $name ) . '.php';

    if ( is_readable( $path ) ) {
        require $path;
    }
}

Finally to the registration. As you can see, you can reuse the class now to find files in different directories mapped to a namespace. Let's say all of your own files are living under the namespace CodeReview in the directory src/ (this is a convention in PHP).
In the main file of your application, you can include the Autoloader and register its load() method now like this:
require __DIR__ . '/src/Autoload.php';
spl_autoload_register( [ new Autoload( 'CodeReview', __DIR__ . '/src/' ), 'load' ] );

Now let's say, you have a class Response living under namespace CodeReview\Http.
namespace CodeReview\Http;

class Response {}

It should be placed under /src/CodeReview/Http/Response.php.
You can create a new instance now either with:
new CodeReview\Http\Response;

… or you put a use statement at the top of the file that is creating that instance:
use CodeReview\Http;

new Response;

Your class is much simpler now, you can test it, map it to different directories and namespaces, use it in different projects, and put it in any place you like.


Find index where left sum equals to right sum Given an array, find if there is an index where left sum equals to right sum.

My idea is calculate sum, and scan again to see if there is an index for which left_sum equals to total_value - left_sum. My code needs to scan the array twice. I'm wondering if there are any solutions which only need to scan the array once.

import random
def find_index(nums):
    total_value = sum(nums)
    left_sum = 0
    for i,v in enumerate(nums):
        left_sum += v
        if total_value - left_sum == left_sum:
            return i
    return -1

if __name__ == "__main__":
    nums = []
    for i in range(10):
        nums.append(random.randint(0,5))
    print nums
    print find_index(nums)

Two minor improvments are possible:
1- splitting an array by the sum of the elements only makes sense if the elements are integers. Two equal sums of elements require the total sum over all elements to be even. Check this to shortcut the search.
2- if all elements are positive we can further shortcut the search (see comment).
On my notebook this cuts the runtime to ~ 1/3 for short arrays (len=200) and ~2/3 for long arrays (len=20k).

def find_index2(nums):
    total = sum(nums)
    if total%2 != 0:    # this shortcut only if integer array
        return False, -1
    total /= 2
    L = 0
    for i, v in enumerate(nums):
        L += v
        if L == total:
            return True, i
        if L > total:  # this shortcut only if only positive values in array
            break
    return False, i

edit:
Observing that sum() is faster than elementwise summation I changed the algorithm a bit. Now the majority (here: 50%) of the elements are summed up in one call to sum() and then the exact target value is searched for step by step. Also, I have incorporated @kyrill's comments to remove the restriction for integer values. Still, the assumed property of only positive values is used (see code comments).
Execution time is down to approximately 20% vs. OP's code:

def find_index8(nums):
    total = sum(nums) / 2.
    n = int(len(nums) * .5)  # arbitrary, well-guessed testing index
    S = sum(nums[:n])
    if S == total:
        return True, n-1
    elif S < total:                 # add elementwise
        for i, v in enumerate(nums[n:], n):
            S += v
            if S >= total:    # '>' shortcut only if only positive values in array
                return S==total, i
        return False, -1
    else:                 # subtract elementwise
        for i, v in enumerate(nums[:n][::-1], 2):
            S -= v
            if S <= total:    # '<' shortcut only if only positive values in array
                return S==total, n-i
        return False, -1


Binary Calculator in Java I'm a beginner in Java and I tried to create a program that will perform basic arithmetic operations on binary numbers.

BinaryOperations class:

package binaryoperations;

import java.io.IOException;
import static java.lang.System.in;
import java.util.Scanner;

public class BinaryOperations {
    public static void main(String[] args) throws IOException {
        try(Scanner in = new Scanner(System.in)) {
            Operations operation = new Operations();
            System.out.print("First Binary:  ");
                String binOne = in.next();
            System.out.print("Second Binary: ");
                String binTwo = in.next();
            System.out.println("Sum:        " + operation.GetBinarySum(binOne, binTwo));
            System.out.println("Difference: " + operation.GetBinaryDiff(binOne, binTwo));
            System.out.println("Product:    " + operation.GetBinaryProd(binOne, binTwo));
            System.out.println("Quotient:   " + operation.GetBinaryQuotient(binOne, binTwo));
        } catch(NumberFormatException e) {
            System.out.println("Looks like you entered a non Binary digit.");
        } finally {
                in.close();
            }
        }
    }

Operations class:

package binaryoperations;

public class Operations {

    public String GetBinarySum(String a, String b){
        int left = Integer.parseInt(a, 2);
        int right = Integer.parseInt(b, 2);
        int sum = left + right;
        return Integer.toBinaryString(sum);
    }

    public String GetBinaryDiff(String a, String b){
        int left = Integer.parseInt(a, 2);
        int right = Integer.parseInt(b, 2);
        int diff = left - right;
        return Integer.toBinaryString(diff);
    }

    public String GetBinaryProd(String a, String b){
        int left = Integer.parseInt(a, 2);
        int right = Integer.parseInt(b, 2);
        int prod = left * right;
        return Integer.toBinaryString(prod);
    }

    public String GetBinaryQuotient(String a, String b){
        int left = Integer.parseInt(a, 2);
        int right = Integer.parseInt(b, 2);
        int quotient = left / right;
        return Integer.toBinaryString(quotient);
    }
}

To get the result I used Integer.parseInt(String, radix 2) because it's binary.

Did I do something wrong? Is it okay to use radix then converting the result to Binary to get the answer? Or is it better to apply the large amount of code to get the answer? Any suggestions is appreciated.

Style

I'm sorry for making it personal here, camelCase, but the preferred styling for function names is that they're, well, camelCased. (Apologies for the bad pun). So not GetBinarySum, but getBinarySum.

Removing the Operations class

I think your approach for implementation of Operations is flawed.

You're taking two Strings, converting them to numbers, performing a simple operation and converting it to String again.

This by itself is not a problem, but...

        System.out.println("Sum:        " + operation.GetBinarySum(binOne, binTwo));
        System.out.println("Difference: " + operation.GetBinaryDiff(binOne, binTwo));
        System.out.println("Product:    " + operation.GetBinaryProd(binOne, binTwo));
        System.out.println("Quotient:   " + operation.GetBinaryQuotient(binOne, binTwo));

Here you're doing the following:

read two Strings
convert to ints
sum
convert to binary string
print
convert to ints
diff
convert to binary string
print
convert to ints
prod
convert to binary string
print
convert to ints
quotient
convert to binary string
print

If you instead converted the Strings directly after reading, you could do input validation per input ("hey, you entered a 2 for your second binary number!"), and you'd save on doing all this conversion. You'd convert once, do the math, convert back 4 times (4 different results), and done.

Something like this:

read two Strings
convert to ints
sum
convert to binary string
print
diff
convert to binary string
print
prod
convert to binary string
print
quotient
convert to binary string
print

Here's an implementation without error handling:

        int left = Integer.parseInt(binOne, 2);
        int right = Integer.parseInt(binTwo, 2);

        System.out.println("Sum:        " + Integer.toBinaryString(left + right));
        System.out.println("Difference: " + Integer.toBinaryString(left - right));
        System.out.println("Product:    " + Integer.toBinaryString(left * right));
        System.out.println("Quotient:   " + Integer.toBinaryString(left / right));

And poof! The entire Operations class is gone.

Why have an Operations class

Now, if this were some super advanced calculator which has to do more than just do basic math, it'd help to have the Operations class.

For example, "count all the positive bits in the number" or "detect if this binary number is a square". Then you'd be better served by an operations class that takes int(s) and returns int or boolean. You can later convert this to a String. If you write the implementations like that, your BinaryOperations (which needs a better name) class would be responsible for handling input and output (Like the screens and the buttons of a calculator) and the Operations class would be responsible for doing the actual calculating.

Consider why the Java creators made a java.lang.Math class that takes integers, not Strings and it should be easier to understand how to split responsibilities.

About use of Integer.parseInt with radix

Use built-ins when you can.

Built-ins apply to a larger populace than your custom code. Built-ins have been tested by the people who create the Java language. And above all, built-ins don't require you to implement them yourself.

When the resulting code is written faster, has a greater chance of being correct and uses components people are familiar with, why wouldn't you use built-in functions?

Oh, and in case you forgot what they do, they come with documentation too. So it's easy to remind yourself of how the built-ins work. Custom functions require you to re-read the code if you've forgotten and there's no documentation.


Parsing comma-separated floats and semicolon-delimited commands I wrote a cstring parser. It has to work with a relatively wide amount of arguments (usually 3 or 4, but maybe in future with a different amount), which are separated by an , or ;. My wish is to make at least Function 1 less static and save some code. However, I don't really want to use dynamic memory allocation (just if it not to avoid and would bring a benefit). The whole code should run on ATMega2560. This is why I cannot use higher library functions.

Function 1:

float *Receiver::parse_pid_substr(char* buffer) {
  static float pids[8];
  memset(pids, 0, 8*sizeof(float) );

  char cI[32], cII[32], cIII[32], cIV[32], cV[32], cVI[32], cVII[32], cVIII[32];
  size_t i = 0, c = 0, p = 0;
  for(; i < strlen(buffer); i++) {
    if(buffer[i] == '\0') {
      break;
    }
    else if(buffer[i] != ',') {
      switch(p) {
        case 0:
          cI[c] = buffer[i];
          cI[c+1] = '\0';
          break;
        case 1:
          cII[c] = buffer[i];
          cII[c+1] = '\0';
          break;
        case 2:
          cIII[c] = buffer[i];
          cIII[c+1] = '\0';
          break;
        case 3:
          cIV[c] = buffer[i];
          cIV[c+1] = '\0';
          break;
        case 4:
          cV[c] = buffer[i];
          cV[c+1] = '\0';
          break;
        case 5:
          cVI[c] = buffer[i];
          cVI[c+1] = '\0';
          break;
        case 6:
          cVII[c] = buffer[i];
          cVII[c+1] = '\0';
          break;
        case 7:
          cVIII[c] = buffer[i];
          cVIII[c+1] = '\0';
          break;
      }
      c++;
    } else {
      p++;
      c = 0;
      continue;
    }
  }

  pids[0] = atof(cI);
  pids[1] = atof(cII);
  pids[2] = atof(cIII);
  pids[3] = atof(cIV);
  pids[4] = atof(cV);
  pids[5] = atof(cVI);
  pids[6] = atof(cVII);
  pids[7] = atof(cVIII);

  return pids;
}

Function 2:

bool Receiver::parse_pid_conf(char* buffer) {
  if(m_pHalBoard == NULL) {
    return false;
  }
  else if(m_rgChannelsRC[2] > RC_THR_MIN) {        // If motors run: Do nothing!
    return false;
  }

  // process cmd
  bool bRet = false;
  char *str = strtok(buffer, "*");                  // str = roll, pit, thr, yaw
  char *chk = strtok(NULL, "*");                    // chk = chksum

  if(verf_chksum(str, chk) ) {                    // if chksum OK
    char *cstr;

    for(uint_fast8_t i = 0; i < PID_ARGS; i++) {
      if(i == 0)
        cstr = strtok (buffer, ";");
      else cstr = strtok (NULL, ";");

      float *pids = parse_pid_substr(cstr);
      switch(i) {
      case 0:
        m_pHalBoard->m_rgPIDS[PID_PIT_RATE].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_PIT_RATE].kI(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_PIT_RATE].imax(pids[2]);
        break;
      case 1:
        m_pHalBoard->m_rgPIDS[PID_ROL_RATE].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_ROL_RATE].kI(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_ROL_RATE].imax(pids[2]);
        break;
      case 2:
        m_pHalBoard->m_rgPIDS[PID_YAW_RATE].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_YAW_RATE].kI(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_YAW_RATE].imax(pids[2]);
        break;
      case 3:
        m_pHalBoard->m_rgPIDS[PID_THR_ACCL].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_THR_ACCL].kI(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_THR_ACCL].imax(pids[2]);
        break;
      case 4:
        m_pHalBoard->m_rgPIDS[PID_PIT_STAB].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_ROL_STAB].kP(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_YAW_STAB].kP(pids[2]);
        m_pHalBoard->m_rgPIDS[PID_THR_STAB].kP(pids[3]);
        bRet = true;
        break;
      }
    }
  }
  return bRet;
}

Two-dimesional array

Your first function can be aggressively reduced by replacing cI and friends by a 2D array:

float *Receiver::parse_pid_substr(char* buffer) {
  static float pids[8];
  memset(pids, 0, 8*sizeof(float) );

  char rgcPIDS[8][32];
  size_t i = 0, c = 0, p = 0;
  for(; i < strlen(buffer); i++) {
    if(buffer[i] == '\0') {
      break;
    }
    else if(buffer[i] != ',') {
      rgcPIDS[p][c] = buffer[i];
      rgcPIDS[p][c+1] = '\0';
      c++;
    } else {
      p++;
      c = 0;
      continue;
    }
  }

  for (int j = 0 ; j < 8 ; ++j) {
    pids[j] = atof(chars[j]);
  }

  return pids;
}

continue

In the following piece of code:

} else {
  p++;
  c = 0;
  continue;
}

The else clause is the last of your loop, and continue is the last instruction of that clause. You can remove continue: even without it, your function will still work and behave the same way as before.

Zero-initialization

Instead of using memset to zero-initialize your arrays, you can use the language zero-initialization for arrays whose bound is known at compile-time:

char rgcPIDS[8][32] = { 0 };

Names

The names you use are all but explicit. While i is good as a loop iterator, c and p should have more explicit names.

NOTE: Hadn't it been for the Receiver::, I would have thought that your code was written in plain old C, and not in C++. Is it intentional?


Generalizing the Initialization of Singleton Derived Classes I have the following construct for storing and restoring my applications settings to and from XML.

public interface ISettingsProvider : ICustomXmlSerializableObject
{
    void RestoreDefaultSettings(bool force = false);
}

where I use a wrapper for the XmlSerialization to prevent access from multiple threads at the same time. The interface ICustomXmlSerializableObject is merely

public interface ICustomXmlSerializableObject
{
    string GetSerializedFilePath();
}

I then have an abstract class that all my setting providers inherit from, this is

public abstract class SettingsProviderBase : ISettingsProvider
{
    public virtual string Extension { get { return ".settings"; } }

    public abstract string GetSerializedFilePath();

    public abstract void RestoreDefaultSettings(bool force = false);
}

Then an example of a setting provider concrete implementation would be

public class ConsoleSettingsProvider : SettingsProviderBase
{
    private static ConsoleSettingsProvider instance = null;

    private ConsoleSettingsProvider() { }

    public static ConsoleSettingsProvider Instance
    {
        get
        {
            if (instance == null)
            {
                instance = new ConsoleSettingsProvider();
                if (File.Exists(instance.GetSerializedFilePath()))
                {
                    ConsoleSettingsProvider o = instance.Deserialize();
                    instance = o ?? instance;
                }
                instance.RestoreDefaultSettings();
            }
            return instance;
        }
    }

    public override void RestoreDefaultSettings(bool force = false)
    {
        if (InterfaceSettings == null || force)
            InterfaceSettings = new InterfaceSettings();
    }

    public override string GetSerializedFilePath()
    {
        // Build the required storage directory.
        string path = Utils.CheckBuildRoamingAppDataDirectory(Constants.Trademark);
        return Path.Combine(path, "Console" + Extension);
    }

    [XmlElement, Browsable(false)]
    public InterfaceSettings InterfaceSettings { get; set; }
}

[Serializable]
public class InterfaceSettings : ISettings
{
    private Font font;

    public InterfaceSettings()
    {
        Font = new Font("Consolas", 9.0f);
    }

    public Font Font
    {
        get { return font; }
        set { font = value; }
    }
}

Where ISettings is a marker interface that may allow me to do something later.

My question is concerning the refactoring of this class hierarchy. It would be nice to move the instantiation of these ISettingsProvider types into the abstract class and save some code as the instantiation method is the same for each derived class. Perhaps moving the property to a method and changing the SettingProviderBase class to

public abstract class SettingsProviderBase : ISettingsProvider
{
    public static virtual T instance = null;

    public static virtual T Instance<T>() where T : ISettingsProvider
    {
        if (instance == null)
        {
            instance = new T();
            if (File.Exists(instance.GetSerializedFilePath()))
            {
                T o = instance.Deserialize();
                instance = o ?? instance;
            }
            instance.RestoreDefaultSettings();
        }
        return instance;
    }

    public virtual string Extension { get { return ".settings"; } }

    public abstract string GetSerializedFilePath();

    public abstract void RestoreDefaultSettings(bool force = false);
}

But this clearly is not possible. Can I achieve what I want and if so, how?

Personally I've always preferred to have limited number of singletons in a project. Generally there's a good need for them, but how you implement them is another question. For instance I've seen people have a single singleton before with properties that were the objects it needed to have single instances of. Obviously any objects linked in a singleton will only have one if you access them through the singleton. this allows you to have ONE singleton object as a sort of container to hold any objects that need to be singletons. Mind you this is a personal preference, so I will digress.

I think the answer you're looking for is here https://stackoverflow.com/questions/16745629/how-to-abstract-a-singleton-class

It requires both the self-referencing generic typing and the use of the new() constraint.

This is the closest I've ever seen to what you're trying to do. It is not very strictly enforced though because of the need for the new() constraint.


Adding extension methods to IServiceCollection in ASP.NET Core I have the following extension method to add themes support to my application:

public static void AddThemes(this IServiceCollection services, Action<ThemesOptions> setup) {
    services.Configure(setup);
}

Where ThemesOptions is defined as:

public class ThemesOptions {
    public IEnumerable<string> Themes { get; set; }
}

Now in my application's startup ConfigureServices method I can say:

services.AddThemes(options => {
    options.Themes = Configuration.GetSection("Themes").Get<ThemesOptions>().Themes;
});

I'm not sure that I like that I have to set every property for the options. Alternatively I tried:

services.AddThemes(options => {
    options = Configuration.GetSection("Themes").Get<ThemesOptions>();
});

And:

services.AddThemes(options => Configuration.GetSection("Themes"));

However when I inject IOptions<ThemesOptions> the Themes property is null.

An alternative to I changed my extension method to:

public static void AddThemes(this IServiceCollection services, IConfiguration configuration) {
    services.Configure<ThemesOptions>(configuration.GetSection("Themes"));
}

Now I can say the following within my application's Startup ConfigureServices method:

services.AddThemes(Configuration);

This worked fine, however, I feel the problem with this approach is that the extension method only allows the options to be set from the configuration.

I'd appreciate it if someone could confirm whether my first solution is correct and if it can be improved upon.

Instead of doing

services.AddThemes(options => {
    options = Configuration.GetSection("Themes").Get<ThemesOptions>();
});

you could  use

services.AddThemes(options => {
    Configuration.GetSection("Themes").Bind(options);
});

which will programatically set each value of options based on what's in the configuration.


Switch / Dictionary containing multiple values with same keys I have different strings that will return different integers, though some strings have the same value (usually 4 per code).

I used a Dictionary first, and still have them, but it's kinda messy. I also tried using switch, which is basically the same to me in this case.

Here are two examples:

public static int PPCFloatCalc4RegistersSwitch(string code)
    {
        switch (code)
        {
            case "fsel": return 23;
            case "fsel.": return 23;
            case "fmul": return 25;
            case "fmul.": return 25;
            case "fm": return 25;
            case "fm.": return 25;
            case "fmuls": return 25;
            case "fmuls.": return 25;
            case "fms": return 28;
            case "fms.": return 28;
            case "fmsub": return 28;
            case "fmsub.": return 28;
            case "fmsubs": return 28;
            case "fmsubs.": return 28;
            case "fmadd": return 29;
            case "fmadd.": return 29;
            case "fma": return 29;
            case "fma.": return 29;
            case "fmadds": return 29;
            case "fmadds.": return 29;
            case "fnmsub": return 30;
            case "fnmsub.": return 30;
            case "fnmsubs": return 30;
            case "fnmsubs.": return 30;
            case "fnmadd": return 31;
            case "fnmadd.": return 31;
            case "fnmadds": return 31;
            case "fnmadds.": return 31;
        }

        return -1;
    }

    public static readonly Dictionary<string, int> PPCFloatCalc4RegistersDictionary = new Dictionary<string, int>
    {
        ["fsel"] = 23,
        ["fsel."] = 23,
        ["fmul"] = 25,
        ["fmul."] = 25,
        ["fm"] = 25,
        ["fm."] = 25,
        ["fmuls"] = 25,
        ["fmuls."] = 25,
        ["fms"] = 28,
        ["fms."] = 28,
        ["fmsub"] = 28,
        ["fmsub."] = 28,
        ["fmsubs"] = 28,
        ["fmsubs."] = 28,
        ["fmadd"] = 29,
        ["fmadd."] = 29,
        ["fma"] = 29,
        ["fma."] = 29,
        ["fmadds"] = 29,
        ["fmadds."] = 29,
        ["fnmsub"] = 30,
        ["fnmsub."] = 30,
        ["fnmsubs"] = 30,
        ["fnmsubs."] = 30,
        ["fnmadd"] = 31,
        ["fnmadd."] = 31,
        ["fnmadds"] = 31,
        ["fnmadds."] = 31,
    };

As you can see, it's repetitive, which is kinda bad for readability. It's worth noting that performance is a bit needed here, so switch/Dictionary speed would be great.

I don't think I need to add any more examples be cause the functions pretty much show what it does quite clearly.

Dictionary and switch are far from the same, even in your case.

As a general guideline I use a dictionary as a lookup mechanism and a switch as flow control.

As a lookup mechanism a switch is less efficient than a dictionary.

Maintenance wise, adding new values to a dictionary is not changing code per se. Modifying the dictionary will not cause a change to the code calling it. And a dictionary has a built in error check - you can't have the same key twice with different values.

However the switch is very much involved in program flow control. Therefore changing the switch code is higher risk than changing dictionary content.

Finally, using a dictionary decouples your data from the code that uses it. It makes the data re-usable. And you don't have duplicate switch code everywhere the data is used.


Calculate True Positive, False Positive, True Negative and False negative and colourize output Objective : Calculate True Positive, False Positive, True Negative and False negative and colourize the image accordignly, based on ground-truth and prediction from my classifier model.

Problem : Very Slow

Description:

The prediction is a gray-level image that comes from my classifier.

The truth_image is also a gray-level image, but its the correct image that prediction should try to approximate.

The mask_image segmentates the ROI ( region of interest), when its pixel value is 1, means its inside the region of interest, if its 0, then we should ignore and move on to the next pixel.

The output is the array where I want to keep the colourized image.

def coloured_prediction_truth(prediction, truth_image, mask_image,output_image):
    prediction = prediction.astype(np.uint8)
    truth_image = truth_image.astype(np.uint8)
    #output_image = np.empty(shape=(submission_image.shape[0], submission_image.shape[1], 4), dtype=np.uint8)

    #binarize values , values > 127 becomes 1. Else -> 0
    for i in xrange(0, prediction.shape[0]):
        for j in xrange(0, prediction.shape[1]):
            if prediction[i, j] > 127:
                prediction[i, j] = 1
            else:
                prediction[i, j] = 0

    for i in xrange(0, truth_image.shape[0]):
        for j in xrange(0, truth_image.shape[1]):
            if truth_image[i, j] > 127:
                truth_image[i, j] = 1
            else:
                truth_image[i, j] = 0

    for i in xrange(0, mask_image.shape[0]):
        for j in xrange(0, mask_image.shape[1]):
            if mask_image[i, j] > 127:
                mask_image[i, j] = 1
            else:
                mask_image[i, j] = 0

    # B-G-R-A
    blue = [255, 0, 0, 255]
    green = [0, 255, 0, 255]
    red = [0, 0, 255, 255]
    orange = [0, 128, 255, 255]
    black = [0, 0, 0, 255]

    TruePositive = 0.00
    TrueNegative = 0.00
    FalsePositive = 0.00
    FalseNegative = 0.00

    #Count pixel by pixel
    for i in xrange(0, output_image.shape[0]):
        for j in xrange(0, output_image.shape[1]):

            pred = prediction[i, j]
            truth = truth_image[i, j]
            mask = mask_image[i, j]

            if mask == 1:

                if pred == 1 and truth == 1:
                    output_image[i, j] = blue
                    TruePositive = TruePositive + 1
                else:
                    if pred == 0 and truth == 0:
                        output_image[i, j] = green
                        TrueNegative = TrueNegative + 1
                    else:
                        if pred == 0 and truth == 1:
                            output_image[i, j] = red
                            FalseNegative = FalseNegative + 1
                        else:
                            if pred == 1 and truth == 0:
                                output_image[i, j] = orange
                                FalsePositive = FalsePositive + 1
                            else:
                                output_image[i, j] = black
            else:
                output_image[i, j] = black

    accuracy = float((TruePositive + TrueNegative)) / float((TruePositive + FalsePositive + FalseNegative + TrueNegative))
    sensitivity = float((TruePositive)) / float((TruePositive + FalseNegative))
    specificity = float((TrueNegative)) / float((TrueNegative + FalsePositive))
    try:
        positivePredictiveValue = float((TruePositive)) / float((TruePositive + FalsePositive))
    except Exception:
        positivePredictiveValue = 0

    return output_image, TruePositive, TrueNegative, FalsePositive, FalseNegative, accuracy, sensitivity, specificity, positivePredictiveValue

Example :

From these 3 images, the coloured image, would be:

Don't use manual iteration. It's slow and tedious. Use broadcasting instead.

For instance, you can create a true positives mask like this:

true_positive_mask = np.logical_and(prediction > 127, truth_image > 127)

After that, you can use this mask to index the output image:

output_image[true_positive_mask] = green # or whatever color you want

and to compute the total number of true positives:

true_positives = np.sum(true_positive_mask)

You can use the same idea to compute other values or for input binarization.


Using Reddit API in R I'm scraping some comments from Reddit using Reddit JSON API and R. Since the data does not have a flat structure, extracting it is a little tricky, but I've found a way.

To give you a flavour of what I'm having to do, here is a brief example:

x = "http://www.reddit.com/r/funny/comments/2eerfs/fifa_glitch_cosplay/.json" # example url
rawdat   = readLines(x,warn=F) # reading in the data
rawdat   = fromJSON(rawdat) # formatting
dat_list = repl = rawdat[[2]][[2]][[2]] # this will be used later
sq       = seq(dat_list)[-1]-1 # number of comments
txt      = unlist(lapply(sq,function(x)dat_list[[x]][[2]][[14]])) # comments (not replies)

# loop time:

for(a in sq){
  repl  = tryCatch(repl[[a]][[2]][[5]][[2]][[2]],error=function(e) NULL) # getting replies all replies to comment a

  if(length(repl)>0){ # in case there are no replies
    sq  = seq(repl)[-1]-1 # number of replies
    txt    = c(txt,unlist(lapply(sq,function(x)repl[[x]][[2]][[14]]))) # this is what I want

    # next level down
    for(b in sq){
      repl  = tryCatch(repl[[b]][[2]][[5]][[2]][[2]],error=function(e) NULL) # getting all replies to reply b of comment a

      if(length(repl)>0){
        sq  = seq(repl)[-1]-1
        txt    = c(txt,unlist(lapply(sq,function(x)repl[[x]][[2]][[14]])))
      }
    }
  }
}

The above example gets all comments, the first level of replies to each of these comments and the second level of replies (i.e. replies to each of the replies), but this could go down much deeper, so I'm trying to figure out an efficient way of handling this. To achieve this manually, what I'm having to do is this:

Copy the following code from the last loop:

for(b in sq){
  repl  = tryCatch(repl[[b]][[2]][[5]][[2]][[2]],error=function(e) NULL)

  if(length(repl)>0){
    sq  = seq(repl)[-1]-1
    txt = c(txt,unlist(lapply(sq,function(x)repl[[x]][[2]][[14]])))
  }
}

Paste that code right after the line that starts with txt = ... and change b in the loop to c.
Repeat this procedure approximately 20 times or so, to make sure everything is captured, which as you can imagine creates a huge loop. I was hoping that there must be a way to fold this loop somehow and make it more elegant.

Here are my main recommendations:

use recursion
use names instead of list indices, for example node$data$reply$data$children reads much better than node[[2]][[5]][[2]][[2]] and it is also more robust to data changes.
use well-named variables so you code reads easily

Now for the code:

url       <- "http://www.reddit.com/r/funny/comments/2eerfs/fifa_glitch_cosplay/.json"
rawdat    <- fromJSON(readLines(url, warn = FALSE))
main.node <- rawdat[[2]]$data$children

get.comments <- function(node) {
   comment     <- node$data$body
   replies     <- node$data$replies
   reply.nodes <- if (is.list(replies)) replies$data$children else NULL
   return(list(comment, lapply(reply.nodes, get.comments)))
}

txt <- unlist(lapply(main.node, get.comments))
length(txt)
# [1] 199


Function to rotate bitmap image Presenting you an implementation of the "shear and transpose" method of rotating a bitmap image. It works.. but it works slow, which is fully understandable. It is understandable due to the evident lack of optimization. Notwithstanding it doesn't interact with the VGA or any library for that matter.

#include <stdio.h>
#include <stdlib.h>
#include <math.h>

/* bmp offsets */
#define OFFSET_OF_ID        (0x0)
#define OFFSET_OF_SIZE      (0x2)
#define OFFSET_OF_PIXELS    (0xA)
#define OFFSET_OF_NDIB      (0xE)
#define OFFSET_OF_WIDTH     (0x12)
#define OFFSET_OF_HEIGHT    (0x16)
#define OFFSET_OF_BPP       (0x1C)
#define OFFSET_OF_NRAW      (0x22)

/* bmp is especially dedicated for windows. defining windows-specific types: */
typedef unsigned char byte, *buffer;
typedef unsigned short word;
typedef unsigned long dword;
typedef unsigned long long ddword;
typedef unsigned char pixel[3];

/* bitmapS[nBitmap][offset] */
buffer *bitmapS;

/* Helper function */
ddword bmp_find_xy (dword width, dword xp, dword yp);

buffer
bmp_rotate
(dword nBitmap, float angle)
{
    const dword src_width  = *( (dword*)&bitmapS[nBitmap][OFFSET_OF_WIDTH]);
    const dword src_height = *( (dword*)&bitmapS[nBitmap][OFFSET_OF_HEIGHT]);
    const dword src_nraw   = *( (dword*)&bitmapS[nBitmap][OFFSET_OF_NRAW]);
    const dword src_pixels = *( (dword*)&bitmapS[nBitmap][OFFSET_OF_PIXELS]);

    buffer dest = malloc( src_pixels + src_nraw );

    float midX, midY;
    float deltaX, deltaY;
    dword rotX, rotY;
    dword i, j;

    midX = src_width / 2.0f;
    midY = src_height / 2.0f;

    memcpy(dest, bitmapS[nBitmap], src_pixels);

    for(i = 0; i < src_width; i++)
    for(j = 0; j < src_height; j++)
    {
        deltaX = i - midX;
        deltaY = j - midY;

        rotX = (dword)(midX + deltaX * sin(angle) + deltaY * cos(angle));
        rotY = (dword)(midY + deltaX * cos(angle) - deltaY * sin(angle));

        if(rotX >= 0 && rotX < src_width && rotY >= 0 && rotY < src_height)
        {
            ddword dest_offset = bmp_find_xy(src_width, i, j);
            ddword src_offset = bmp_find_xy(src_width, rotX, rotY);

            deltaX = i + midX;
            deltaY = j + midY;

            memcpy(&dest[dest_offset], &bitmapS[nBitmap][src_offset], sizeof(pixel);
        }
    }

    return dest;
}

/* Definition of helper function */
ddword
bmp_find_xy
(dword width, dword xp, dword yp)
{
    dword w = width;
    const dword channels = sizeof(pixel);
    const dword bpp = 8;
    const dword single = (channels * bpp) / 7;
    const dword offset = sizeof(BMP) + sizeof(DIB);
    dword rowsize = w * single;

    dword pixAddress;

    if (rowsize % 4 != 0) rowsize += 4 - (rowsize % 4);
    pixAddress = offset + yp * rowsize + xp * single;

    return pixAddress;
}

Code compilation issues

The code didn't compile cleanly for me.

There was a missing right paren on this line:

    memcpy(&dest[dest_offset], &bitmapS[nBitmap][src_offset], sizeof(pixel);

I got a warning that memset was used without a prototype.  I added #include <string.h> to fix that.
The code was missing the BMP and DIB structures.  I substituted 14 for sizeof(BMP) and 40 for sizeof(DIB) to compensate.

Code style/formatting

Keeping in mind that style and formatting are somewhat subjective, here are some issues I spotted:

I didn't like the use of a nested for loop at the same indentation.  It confused me a little bit:

for(i = 0; i < src_width; i++)
for(j = 0; j < src_height; j++)

I didn't like the "Windows-ization" of the types.  Personally, I don't like seeing ddword, dword, and word because I really don't have any idea what those are.  I prefer to just #include <stdint.h> and use uint64_t, uint32_t, and uint15_t.  Plus, the stdint types are safer to use.  Your dword type, for example, is presumably supposed to be 32 bits wide, but you typedef'd it to unsigned long.  On a 64-bit machine, unsigned long is likely to be 64 bits wide.
Currently, bmp_rotate() takes an index as an argument, and then indexes into a global array of bitmaps.  The code would be more reusable if the function took one bitmap as an argument, like this:

buffer bmp_rotate(const buffer bitmap, float angle);

Then it could be used from anywhere without being entangled with the global variable.
There is dead code in the inner loop.  This code doesn't do anything:

    deltaX = i + midX;
    deltaY = j + midY;

Bugs in the code

The rotation is wrong.  For example, if you pass in 0 for the angle, you should wind up with the same bitmap.  Instead, you get a bitmap that is rotated 90 degrees to the right and mirror imaged.  To fix it, I changed your rotation computation to this:

rotX = (dword)(midX + deltaX * cos(angle) + deltaY * sin(angle));
rotY = (dword)(midY - deltaX * sin(angle) + deltaY * cos(angle));

There are uninitialized pixels in the rotated bitmap.  The destination bitmap is created like this:

buffer dest = malloc( src_pixels + src_nraw );
memcpy(dest, bitmapS[nBitmap], src_pixels);

This allocates a bitmap of the same size of the original and copies the header information from the original to the new bitmap.  However, the pixels in the new bitmap are all uninitialized.  Later, when doing the rotation, not all pixels in the new bitmap are set (because some are not mapped to any original pixel).  So those pixels end up with a random value.  You can fix this by simply using calloc() to allocate the new bitmap.
The offset of the pixels may not be right.  Currently, the offset from the start of the bmp data to the first pixel is calculated like this:

const dword offset = sizeof(BMP) + sizeof(DIB);

I'm not an expert on bmp files, but I believe that the DIB part is a variable length part.  So to get the right offset, you should be using the value you call src_pixels as your offset instead.

Speed issues covered up by the compiler

There are a couple of things that I spotted that you could have done better speedwise, but that the compiler (gcc -O4) seemed to fix for you.

Instead of using sin(angle) and cos(angle) in your inner loop, you could compute those once at the start of the function, and then use the precomputed versions later on:

float sin_angle = sin(angle);
float cos_angle = cos(angle);
...
rotX = (dword)(midX + deltaX * cos_angle + deltaY * sin_angle);
rotY = (dword)(midY - deltaX * sin_angle + deltaY * cos_angle);

Apparently, gcc is smart enough to know that sin() and cos() are pure functions, so it was able to optimize away the calls in the inner loop.  Still, it's good practice to do this kind of optimization yourself.  If instead of sin() and cos() you were calling your own function, then the compiler might not know the optimization was possible.
In bmp_find_xy(), all of the lines up to the computation of pixAddress are the same for a given sized bitmap.  So you could do an optimization where you compute offset, rowsize, and single just once, and then use those precomputed values in the inner loop.  Just as with #1, gcc was smart enough to do all that already.

Speed issues not covered by the compiler

The way that you order your nested loop, on every iteration, you move from one destination pixel to the one below it.  If you swapped the two loops, you would instead move from one pixel to the one to its right.  This is a better ordering, because it is more cache friendly.  In fact, when I swapped the two loops, I was able to get a 5% speedup on a 45 degree rotation.
Oftentimes with loops, you can often simplify the calculation within an inner loop if you compute an initial value and then increment the value instead of recomputing the whole thing.  For example, let's look at a sample loop:

for (i=0;i<n;i++) {
    offset = i * ROW_WIDTH + startOffset;
    do_something(offset);
}

You can simplify this to:

offset = 0 * ROW_WIDTH + startOffset;
for (i=0;i<n;i++) {
    do_something(offset);
    offset += ROW_WIDTH;
}

Using this technique, I simplified your main loop to this:

for(j = 0; j < src_height; j++) {
    ddword dest_offset = src_pixels + j * rowsize;
    float  deltaY = j - midY;
    float  deltaX = 0 - midX;
    float  x = midX + deltaX * cos_angle + deltaY * sin_angle;
    float  y = midY - deltaX * sin_angle + deltaY * cos_angle;
    for(i = 0; i < src_width; i++) {
        dword rotX = x;
        dword rotY = y;
        if(rotX >= 0 && rotX < src_width && rotY >= 0 && rotY < src_height)
        {
            ddword src_offset = src_pixels + rotY * rowsize + rotX * single;

            memcpy(&dest[dest_offset], &bitmap[src_offset], sizeof(pixel));
        }
        x += cos_angle;
        y -= sin_angle;
        dest_offset += single;
    }
}

Notice how the inner loop has replaced three complicated expressions with three simple additions.  This change resulted in a 140% speedup (it was 2.4x faster)!
Memcpy isn't so great for only 3 bytes.  I replaced your call to memcpy with some inlined code:

    // Old code:
    // memcpy(&dest[dest_offset], &bitmap[src_offset], sizeof(pixel));
    // New code:
    dest[dest_offset+0] = bitmap[src_offset+0];
    dest[dest_offset+1] = bitmap[src_offset+1];
    dest[dest_offset+2] = bitmap[src_offset+2];

This resulted in a 9% speedup.


Shipping company investigation Imagine you are a company that wants to assist some US-based companies
  to ship their goods to customers in Hamburg. To achieve this, lets
  assume that every order can be represented by a tuple (orderId,
  companyName, customerAddress, orderedItem).

  Thus, some sample data might look like this:

001, SuperTrader, Steindamm 80, Macbook
002, Cheapskates, Reeperbahn 153, Macbook
003, MegaCorp, Steindamm 80, Book "Guide to Hamburg"
004, SuperTrader, Sternstrasse 125, Book "Cooking 101"
005, SuperTrader, Ottenser Hauptstrasse 24, Inline Skates
006, MegaCorp, Reeperbahn 153, Playstation
007, Cheapskates, Lagerstrasse 11, Flux compensator
008, SuperTrader, Reeperbahn 153, Inline Skates

  Instructions:

  Using node.js, please implement a working solution to perform the following kind of operations on the data:

  show all orders from a particular company
  show all orders to a particular address
  delete a particular order given an OrderId
  display how often each item has been ordered, in descending order (ie in the above example, 2x for Macbook and Inline skates, 1x for the rest)

  Note:

  Please optimize your code and do not convolute it with handling
  exceptions/edge cases – we are more interested in readability for this
  solution.

Solution:

const data = [
  // orderId, company, custAddr, product
  ['001', 'SuperTrader', 'Steindamm 80', 'Macbook'],
  ['002', 'Cheapskates', 'Reeperbahn 153', 'Macbook'],
  ['003', 'MegaCorp', 'Steindamm 80, Book', 'Guide to Hamburg'],
  ['004', 'SuperTrader', 'Sternstrasse 125', 'Book "Cooking 101"'],
  ['005', 'SuperTrader', 'Ottenser Hauptstrasse 24', 'Inline Skates'],
  ['006', 'MegaCorp', 'Reeperbahn 153', 'Playstation'],
  ['007', 'Cheapskates', 'Lagerstrasse 11', 'Flux compensator'],
  ['008', 'SuperTrader', 'Reeperbahn 153', 'Inline Skates'],
];

// show all orders from a particular company
const allCompanyOrders = (orders, company) => { // O(n)
  return orders.filter((o) => o[1] === company);
};

// all orders to an address
const allOrdersToAddress = (orders, address) => { //O(n)
  return orders.filter((o) => o[2] === address);
};

// delete an order
const removeOrder = (orders, id) => { // O(n)
  const result = [];
  orders.forEach((o) => {
    if (o[0] !== id) {
      result.push(o);
    }
  });
  return result;
};

const itemsCount = (orders) => {
  const map = Object.create(null);
  orders.forEach((o) => {
    const name = o[3];
    const count = map[name] || 0;
    map[name] = count + 1;
  });
  return map;
};

//show often ordered items in descending ordered
const showMostFrequent = (orders) => {
  const map = itemsCount(orders);
  const result = [];

  const keys = Object.keys(map);
  for (let name of keys) {
    const count = map[name];
    result.push({name, count});
  }

  // O(nlogn) + O(n) memory
  return result.sort((a, b) => b.count - a.count)
    .map((o) => o.name);
};

console.log(allCompanyOrders(data, 'SuperTrader').length === 4);
console.log(allOrdersToAddress(data, 'Reeperbahn 153').length === 3);
console.log(removeOrder(data, '007').length === 7);
console.log(showMostFrequent(data));

Questions:

Should I preprocess data for optimisation at cost of memory?
Right now I have to use indexes everywhere which feels uncomfortable.

I'd definitely map the tuples to objects, to avoid all the index stuff later on. In this case, the question specifically says it's all about readability, so don't worry about memory usage (also because it's next to nothing).

const orders = data.map(order => {
  const [orderId, companyName, companyAddress, orderedItem] = order;
  return { orderId, companyName, companyAddress, orderedItem };
});

As for the rest of the code:

Your removeOrder could be implemented using filter, just like the all* functions; no need to manually push to a new array. And your showMostFrequent and itemsCount functions could both be implemented using map - again instead of manually pushing to a new array/object.

Overall though, I'd wrap everything in an Orders object/class, instead of passing the orders array on every call. Maybe something like this:

class Orders {
  constructor(data) {
    this.orders = data.map(order => {
      const [orderId, companyName, companyAddress, orderedItem] = order;
      return { orderId, companyName, companyAddress, orderedItem };
    });
  }

  // note: Assumes orderIds are unique
  remove(orderId) {
    const index = this.orders.findIndex(order => order.orderId === orderId);
    if (index === -1) {
      return null;
    } else {
      const order = this.orders[index];
      this.orders.splice(index, 1);
      return order;
    }
  }

  findBy(property, value) {
    return this.orders.filter(order => order[property] === value);
  }

  findByCompanyName(name) {
    return this.findBy('companyName', name);
  }

  findByCompanyAddress(address) {
    this.findBy('companyAddress', address);
  }

  itemCounts() {
    const grouped = this.orders.reduce((memo, order) => {
      memo[order.orderedItem] = memo[order.orderedItem] || 0;
      memo[order.orderedItem] += 1;
      return memo;
    }, {})

    return Object
      .keys(grouped)
      .map(key => { return { orderedItem: key, count: grouped[key] } })
      .sort((a, b) => a.count - b.count)
      .reverse();
  }
};

I've added a generic findBy method that's used by other two findBy* methods. But more importantly, the remove method now modifies the list of orders, and returns the removed order object (or null), rather than return a new array.


Sorting by checking 2 distance away elements in a list, and check the method's accuracy This code works as follows :
It accepts number of test case T. For each test case, it accepts length of sequence of integers N and the sequence (or list) itself L.

The goal is to sort the list by checking every three consecutive elements, but only comparing the ends. L[i] and L[i+2]. If the right side is less than left one, then these two have to be swapped. This algorithm will continue until nothing can be swapped.

The problem is, my code works for N < 100. Also work for N larger but very slow, exceeded the time limit which is 20 secs.

If the list ended up sorted correctly, the output must be 'OK'.

If the list ended up not sorted, the output must be the first index in which the next index has element less than it.

This is part of google code jam 2018 qualificatiom round, finished already.

How to optimize this in Python..? What are the arguments? Thanks.

    T = input()

    N = []
    L = []
    output = []

    for t in range(T):

        N.append(input())
        L.append(raw_input())

    def trouble(l, n):
        repeat = True
        while repeat:

            repeat = False
            for index in range(0,n-2):
                if l[index] > l[index+2]:
                    repeat = True
                    dum = l[index:index+3]
                    l[index] = dum[-1]
                    l[index+2] = dum[0]
                    del dum
        return l

    def search(l, n):

        for i in range(n):

            if l[i] > l[i+1]:

                return i

    for i in range(T):

        l = L[i].split()
        l = [int(num) for num in l]
        res = trouble(l, N[i])
        if res == sorted(l):
            output.append('OK')
        else:
            output.append(search(l, N[i]))

    for t in range(T):

        print('Case #'+str(t+1)+': '+str(output[t]))

With python 2.7, you have xrange. The improvement in performance might be minimal (but an improvement nonetheless):

  The advantage of xrange() over range() is minimal (since
  xrange() still has to create the values when asked for them) except
  when a very large range is used on a memory-starved machine or when
  all of the range’s elements are never used (such as when the loop is
  usually terminated with break).

Split your \$ L \$ and use map as soon as possible.

L.append(map(int, raw_input().split()))

Swapping in python is as easy as:

a, b = b, a

No need to allocate a temporary dummy (and del it later):

if l[index] > l[index + 2]:
    repeat = True
    l[index], l[index + 2] = l[index + 2], l[index]

Now, for the time limit issue, since you know that you are actually sorting the even and odd indexes sublists disjointly; splice your \$ L \$ and sort those. Then, reattach them both by weaving them together.

# L has been read from user
l_even = sorted(l[::2])
l_odd = sorted(l[1::2])

and then join them together.


JavaScript Rock, Paper, Scissors I have built a simple JS class with functional methods that create a Rock, Paper, Scissors game.

I would be happy to get a non-opinionated review in terms of code being optimal.

class Rps{
  constructor(){
    this.signs = {1:'rock',2:'paper',3:'scissors'};
    this.winMatrix = [
      {1:3},
      {2:1},
      {3:2}
    ];
  }
  randomizePlay(){
    const keys = Object.keys(this.signs);
    return 1 + Math.floor(keys.length * Math.random());
  }

  play(){
    return this.roundResult([this.randomizePlay(),this.randomizePlay()]);
  }

  roundResult(resultArray) {
    let victorious = {};

    if (resultArray[0] === resultArray[1]) {
      return 'rematch';
    }

    this.winMatrix.forEach((value, index) => {
      const p1Result = resultArray[0];
      const p2Result = resultArray[1];
      const p1WinMatrix = parseInt(Object.keys(this.winMatrix[index]));
      const p2WinMatrix =  parseInt(Object.values(this.winMatrix[index]));

      if (p1Result === p1WinMatrix && p2Result === p2WinMatrix){
        return victorious = {1:p1Result}
      }else{
        return victorious = {2:p2Result}
      }
    });
    return victorious;
  }
}

const theRpsArray = [];
const theRpsAverage = 0;

let rps = new Rps;
rps.play()

Simulate only the result
A common problem in learning to program is over complicating the code. Complicated code is longer, harder to understand, and more prone to bugs. When the app is large even tiny over complications can lead to thousands of unneeded lines that must be tested and debugged.
All that matters is what comes out the other end of a function, how that is done does not matter.
Your code is way too complicated for what it needs to do. At its most simplest Rock paper scissors can be one line...
(["Tie ","P1 WINS with ","P2 WINS with "])[Math.random()*3|0]+ ((["Rock","Paper","Scissors"])[Math.random()*3|0])
Rewrite
The object rps is equivalent to yours. Calling rps.play() returns the same results, and is statistically identical to your results
const rps = (() => {
    const rand3 = () => Math.random() * 3 | 0;
    const plays = [
        () => "rematch",
        () => ({"1": rand3() + 1}),
        () => ({"2": rand3() + 1})
    ];
    return { play() { return plays[rand3()]() } };
})();

You do not need to simulate the game mechanics, just the game results.
There are 9 possible results.

1/3rd are draws,
1/3rd player 1 wins and
1/3rd player 2 wins.

When a player wins there is a 1 in 3 chance for any move type.
Thus a result first picks a random winner or draw, if there is a winner assign that player a random hand. There is no need calculate the hand the winner beats as that is inferred by the result.
Example 2
The following example produces a more readable result as an Example

const rps = (() => {
    const moves = ["Rock", "Paper", "Scissors"];
    const rand = (range = 3) => Math.random() * range | 0;
    const win = (m = rand()) => `Player ${rand(2) + 1} ${moves[m]} beats ${moves[(m + 2) % 3]}`;
    const plays = [win, win, (m = moves[rand()]) => `${m} draws with ${m}`];
    return { play() { return plays[rand()]() } };
})();

var games = 20;
while (games --) { log(rps.play()) }

function log(textContent) {
    info.appendChild(Object.assign(document.createElement("div"),{textContent}));
}


A good example of codeigniter MVC? What is the difference between this two way and Which one is best? What do you suggest?

1.
Use of query (db) directly in CI_Controller. as:

$id = $this->input->post('id');
$query = $this->db->get_where('table1', array('id' => $id))->row();
         $this->db->get_where('table2', array('idre' => $query->idre));

Or

2.
Use of query (db) in CI_Model and call it in CI_Controller. as:

CI_Model:

function GetWhere($table,$where){
    return $this->db->get_where($table, $where);
}

CI_Controller:

$id = $this->input->post('id');
$query = $this->model_name->GetWhere('table1', array('id' => $id))->row();
         $this->model_name->GetWhere('table2', array('idre' => $query->idre));

UPDATE:

3.
I use the following code true in model as mvc?

$id = $this->input->post('checked');
        foreach ($id as $val) {
            $query_check = $this->db->get_where(table3, array('id'=>$val));
            $row = $query_check->row();
            if($query_check->num_rows() > 0 || $row->id != $this->session->userdata('id')){
                foreach ($query_check->result() as $value) {
                    $id_relation = $value->id_relation;
                    $this->db->where('idre', $idre)->delete(array(table3, 'table4'));
                }
            }
        }

Whether this is a good example of mvc? How can fix 2 example above for good mvc?

What is the difference between this two way and Which one is best? What do you suggest?

The difference is that you seperate the concerns in the second example. This means that you can change from database type (e.g. a SQL database, a XML file, an array, ect.) without changing to much code. In fact, you only create a new Model and you are ready!

  Whether this is a good example of mvc? How can fix 2 example above for good mvc?

I think you mean that you want to know if this is correct MVC and, if not, how to change it. It is better to put all those logic (except the first line) inside a Model and call that specific method inside your controller. I have used CI once, so I can't help you with code examples...