Extend zeroes in a matrix I built the Set Matrix Zeroes algorithm in JavaScript. I have a lot of loops in my code, so was wondering if there is a better way to implement this algorithm.
Given a m x n matrix, if an element is 0, set its entire row and column to 0. Do it in-place.
I loop through the matrix and when I find a 0, I set its entire row and column to a flag ('X'). I do not set 'X' if the position is a 0 because I might need to check this position in the future. At the end I replace all the 'X' to 0.

var setZeroes = function(matrix) {
    if(matrix === null || matrix.length === 0)
        return [];

    for(let i=0; i<matrix.length; i++){
        for(let j=0; j<matrix[i].length; j++){
            if(matrix[i][j] === 0){
                setFlag(matrix, i, j);
            }
        }
    }

    for(i=0; i<matrix.length; i++){
        for(j=0; j<matrix[i].length; j++){
            if(matrix[i][j] === 'X')
                matrix[i][j] = 0;
        }
    }
};

const setFlag = function(matrix, i, j) {
    matrix[i][j] = 'X';
    let tempI = i+1;
    while(tempI < matrix.length){
        if(matrix[tempI][j] !== 0)
            matrix[tempI][j] = 'X';
        tempI++;
    }
    tempI = i-1;
    while(tempI >= 0){
        if(matrix[tempI][j] !== 0)
            matrix[tempI][j] = 'X';
        tempI--;
    }
    let tempJ = j+1;
    while(tempJ < matrix[i].length){
        if(matrix[i][tempJ] !== 0)
            matrix[i][tempJ] = 'X';
        tempJ++;
    }
    tempJ = j-1;
    while(tempJ >= 0){
        if(matrix[i][tempJ] !== 0)
            matrix[i][tempJ] = 'X';
        tempJ--;
    }
}
```

From a short review;

You probably want to encapsulate if(matrix === null || matrix.length === 0) into an isValidMatrix function for re-use
I would drop the whole i/j thing and go for variables named after row and col
I would collect all the rows and columns with a 0 in one set of loops, and then clear the matrix based on the collected rows and columns in another set of loops.
let only declares in the block, so the for(i=0; i<matrix.length; i++){ creates a global i
I much prefer function setFlag(matrix, i, j) { over const setFlag = function(matrix, i, j) {

This is my current proposal, I did not test it, but you should get the gist of it;
//This does not catch everything, but it'll do
function isValidMatrix(matrix){
  return !(!matrix || !matrix.length);
}

function setZeroes(matrix) {

  if(!isValidMatrix(matrix))
    return [];

  const rows = [], cols = [];

  for(let row = 0; row < matrix.length; row++){
    for(let col = 0; col < matrix[row].length; col++){
      if(matrix[row][col] === 0){
        rows.push(row);
        cols.push(col);
      }
    }
  }

  for(let row = 0; row < matrix.length; row++){
    for(let col = 0; col < matrix[row].length; col++){
      if(rows.includes(row) || cols.includes(col)){
        matrix[row][col] = 0;
      }
    }
  }
}

For further refinement, rows and cols might/will get multiple times the same value, so to deal with that gracefully you should use Sets for that.


Way to delegate command to methods in different files What I want to do is for an example command have it defined in its own file - e.g. command.js - and the same for the other methods. I want to make a separate file for every module - for example "command.js", "vanish.js" and I can not think of a way to call the methods without making a big switch statement in a file where I would require all the "modules".
This is how I do it currently:
const client = require('../../../src/bot');
const logger = require('../../../config/logger');
const {isCommand} = require('../../../helpers/checkCommand');
const {addCommand, deleteCommand, updateCommand} = require('../../../db/commandFunctions')
const isAuthorized = require('../isAuthorized');
const builtInCmdHelpers = require('../builtInCommandHelpers');

const USER_MODULES = 'everyone';
const MOD_MODULES = 'moderator';

const executeBuiltInCommand = async ({channel, args}, userstate) => {
  const isSubArg = await isCommandSubArg(args[1]);
  if(isSubArg) await builtInCmdHelpers[args[1]].apply(null, [{channel, args}, userstate]);
};

const command = async ({channel, args}, userstate) => {
  const canExecute = await isAuthorized(channel, userstate.badges, MOD_MODULES);
  if(canExecute) await executeBuiltInCommand({channel, args}, userstate);
};

const vanish = async ({channel}, {username}) => {
  const canExecute = await isAuthorized(channel, username, USER_MODULES);
  if(canExecute) {
    client.timeout(channel, username, 1, "VANISH")
      .catch((err) => {
      logger.error(err);
    });
  }
};

module.exports = {
  command,
  vanish,
};

This is how i execute the commands currently:
const executeBuiltIn = async ({channel, args}, userstate) => {
  await builtin[args[0]].apply(null, [{channel, args}, userstate]);
};

Separate modules
You didn’t specify a file name for the code in the first block- is it something like builtin.js?
The code in the first block should be divisible into two files- one for command with executeBuiltInCommand and MOD_MODULES, and the other for vanish with USER_MODULES. They could be combined to a single module to replace the current file in the first block - what I presume is builtin.js.
Simplify .apply() syntax
The ES6 feature spread syntax can be used to replace calls to .apply().
For example:

if(isSubArg) await builtInCmdHelpers[args[1]].apply(null, [{channel,
args}, userstate]);

can be simplified to:
if(isSubArg) await builtInCmdHelpers[args[1]](...[{channel, args}, userstate]);

However, since there is a fixed length on that array of arguments, the method can be called directly:
if(isSubArg) await builtInCmdHelpers[args[1]]({channel, args}, userstate);


Implementing feature hashing using guava I am trying to implement feature hashing in Java. For this I am trying to use Hashing functions from guava.

This is how I am doing it now:

//seed is any integer.
HashFunction hf = Hashing.murmur3_32(seed);

//key is the input int and hd is the hashed dimension.
int hash = Hashing.consistentHash(hf.hashInt(key), hd);

I have another method for sign, not shown in this question.

To measure the performance, I am doing something like below:

int min = 0, max = 2000000, sampleSize = 3000000;
int[] hds = { 2000, 5000, 6000, 10000, 20000, 500000, 1000000, 2000000 };

List<Double> guavaCollisons = new ArrayList<>();

UniformIntegerDistribution runif = new UniformIntegerDistribution(min, max);

Arrays.stream(hds)
        .forEach(hd -> {
            int seed = runif.sample();

            HashFunction hf = Hashing.murmur3_32(seed);

            Map<Integer, Set<Integer>> guava = new HashMap<>();

            Arrays.stream(runif.sample(sampleSize))
                    .forEach(key -> {

                        int guavaHash = Hashing.consistentHash(hf.hashInt(key), hd);

                        if (!guava.containsKey(guavaHash))
                            guava.put(guavaHash, new HashSet<Integer>());

                        guava.get(guavaHash).add(key);
                    });

            double guavaCollisionHd = guava.entrySet()
                    .stream()
                    .mapToInt(entry -> entry.getValue()
                            .size())
                    .average()
                    .getAsDouble();

            guavaCollisons.add(guavaCollisionHd);

            System.out.println("Available buckets = " + hd
                + ",  unutilized = " + (hd - guava.keySet().size())
                + ", guava collision = " + guavaCollisionHd);
        });

double guavaCollision = Stats.meanOf(guavaCollisons);
System.out.println("Average collision = " + guavaCollision);

My questions are:

Is this a correct way to implement the 'hash' part of feature hashing?
Is this a correct way to measure the collision rate of the hashing technique?

Simplified code:

List<Double> guavaCollisions = Arrays.stream(hashDimensions)
    .map(dimension -> {
        HashFunction hashFunction = Hashing.murmur3_32(runif.sample());
        Map<Integer, Long> guavaCounts = Arrays.stream(runif.sample(sampleSize))
            .collect(Collectors.groupingBy(key -> Hashing.consistentHash(hashFunction.hashInt(key), dimension),
                 Collectors.counting());
        return guavaCounts.values().stream().mapToLong(id -> id).average().getAsDouble();
    }).collect(Collectors.toList());

Reasoning:

Inlined the seed. Keeping the seed variable there isn't necessary since we don't use it anymore, not even in the logging-statement.
Used collect instead of forEach(el -> collection.add(el)) to create Collections which is more idiomatic.
Dropped the Set<Integer> from the Map, since we don't actually ever use the Integers we keep in memory here, but instead only want the number of unique integers. (Note that currently I don't check for uniqueness, which means the semantics changed)
Simplified the calculation of average from guavaCounts by not streaming the entry-set and then mapping to value, which could be simplified by using Entry::getValue, but directly streaming the values().
Dropped the System.out because it's incredibly slow in comparison, a side-effect that shouldn't be happening in a Stream and doesn't add viable information to the "final" result of the code.
Renamed hds, hd and hf because shortening names is pointless. I'd have preferred to also rename runif, but didn't find a better name, which would remain succinct.


Guess the number with a twist I wrote a program where the computer guesses a random number.

To do this, I implemented a form of binary search. This correctly handles any edge-cases that I know of, including 0 and std::numeric_limits<int>::max(). This doesn't handle negative numbers (since rand will never return one) but that could easily be fixed.

The average amount of tries it takes to guess the number is around 29 or 30.

Here it is:

#include <functional>
#include <iostream>
#include <cstdlib>
#include <limits>
#include <ctime>

static int random_number;

int compare(int guess)
{
    if(guess > random_number)
        return 1;
    if(guess < random_number)
        return -1;
    return 0;
}

struct guess_stats
{
    int guesses = 0;
    int number = 0;
};

guess_stats finder(std::function<int(int)> &&cmp)
{
    int max = std::numeric_limits<int>::max();
    int min = 0;
    int guess = (min + max) / 2;
    int cmp_result;
    guess_stats results;

    do
    {
        cmp_result = cmp(guess);

        if(cmp_result < 0)
        {
            std::cout << "Greater than " << guess;
            min = guess;
            const int diff = max - guess;
            guess += diff - (diff / 2);
        }
        else if(cmp_result > 0)
        {
            std::cout << "Less than " << guess;
            max = guess;
            const int diff = guess - min;
            guess -= diff - (diff / 2);
        }

        std::cout << '\n';

        results.guesses++;
    } while(cmp_result);

    results.number = guess;

    return results;
}

int main(void)
{
    std::srand(std::time(0));
    random_number = std::rand();
    guess_stats stats = finder(compare);

    std::cout << "Number: " << stats.number << '\n'
              << "Guesses: " << stats.guesses << '\n';

    return 0;
}

What I'm looking for:

Although I don't expect there will be, are there any edge-cases I may not have seen?
Are there any algorithmic changes that I could make to have less time-complexity? I don't think so (because this is only O(log n) as it is), but I may be surprised to find out.
Are there any better ways to generate random numbers? I used srand, time, and rand in this program, but those are old solutions. I imagine C++ has something much more up-to-date for generation of random numbers.
Any other general advice on what to improve would be appreciated.

Non-const static variables

Your code shows a lot of good practices:

variables are always initialized when possible
variables have proper names (and you stuck to a single naming scheme)
no unecessary std::endl

However, there is one drawback:

static int random_number;

That line right there is, at least from my point of view, a flaw. static variables are a nice escape hatch in some circumstances, but here it's unnecessary. We can easily use another argument to finder, for example:

guess_stats finder(comparison_function && cmp, int random_number) {
   ...
}

Or we could combine the comparison function with the number to be guessed in a struct, similar to guess_stats.

Either variant makes sure that our changes in finder won't spill into the rest of our program flow. While we don't intend to change random_number in finder, it's easy to accidentally write random_number++ or press <Tab> in an IDE with the wrong completion. If we were to use multiple finder variants in separate threads, we would have a hard time to debug the behaviour, especially if there is more than a single static variable.

However, I guess you've used random_number as a static variable to ease the use of compare. There are other ways that we will inspect at the end of this review.

Note that const static or constexpr static variables on the other hand are fine, as they cannot lead to debugging troubles (unless you const_cast them, but at that point we're deep within undefined behaviour territory).

Q&A

  Although I don't expect there will be, are there any edge-cases I may not have seen?

What happens if I use the following comparison?

int compare(int unused) {
    return 1;
}

Sure, that's not a valid comparison, however, there is nothing in finder that blocks us from using such a function. Given that a binary search should take at most \$\log_2(n)+1\$ steps, we can introduce a simple limit of 33 iterations.

  Are there any algorithmic changes that I could make to have less time-complexity? I don't think so (because this is only O(log n) as it is), but I may be surprised to find out.

That's all correct, and from an \$\mathcal O\$ analysis there's nothing to improve.

  Are there any better ways to generate random numbers?

There's the <random> header. We can use the example code from uniform_int_distribution to generate a random number:

int main(void)
{
    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<int> dist(0, std::numeric_limits<int>::max());

    const int random_number = dist(gen);

    ...
}

However, keep in mind that this usually won't seed a mt19937 correctly and there are ways around that issue.

Getting rid of random_number

Given that we use <functional>, we use at least C++11. Therefore, we have lambdas at hand. We can use this to our advantage to get rid of random_number in main:

int main(void)
{
    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<int> dist(0, std::numeric_limits<int>::max());

    const int random_number = dist(gen);

    auto compare = [random_number](int guess) {
        if (guess > random_number) {
            return 1;
        } else if (guess < random_number) {
            return -1;
        }
        return 0;
    };

    guess_stats stats = finder(compare);
    ...
}

We don't even need to change finder to use the new comparison, thanks to std::function<int(int)>&&.


Action dispatch for a combat class My problem is that I have a piece of code logic that I'm having trouble fitting into an OO approach, and I was looking for design patterns so that the following code would no longer rely on the ugly

  if class is of type A do X else do Y

In Python, we can say I have three "fighting method" classes, class Fight, class Manoeuvre, and class Dodge.

I also have a control class that takes two inputs, attackMode and defendMode. The inputs will always be objects whose classes extend from or are of one of the above three.

The piece of script causing issue is as follows:

from random import Random

# Following are example "Action" objects which are to be compared
# For results

class Action:
    "Base Action all others inherit from"
    __random = Random()

    def __init__(self):
       self.health = 100

    @classmethod
    def getSuccess(cls):
        return cls.__random.randint(-2,3)

class Dodge(Action):
    def __init__(self):
       super(Dodge, self).__init__()
class Attack(Action):
    def __init__(self):
        super(Attack, self).__init__()

    def performActionOn(self, subject, sucessLevel=0):
        damage = 5 if successLevel > 1 else 3
        subject.health -= damage

class Manoeuvre(Action):
    def __init__(self):
        super(Manoeuvre, self).__init__()

    def performActionOn(self, subject, successLevel=0):
        pass

# Following is the results configuration

class CombatControl:
    def fight(self, actionAttack, actionDefend):

        if isinstance(actionDefend, Dodge):
            __attackDodge(actionAttack, actionDefend)
        else:
            __attackAttack(actionAttack, actionDefend)

    def __attackAttack(self, attack, defend):
        """
        " Defender Attacks back
        """
        resultAttack = attack.getSuccess()
        resultDefend = defend.getSuccess()

        # Both failed. No action.
        if resultAttack < 0 and resultDefend < 0:
            return
        # Attack wins or draw
        elif resultAttack >= resultDefend:
            attack.performActionOn(defend, successLevel=resultAttack)
        # Defend wins
        else:
            # Does not get damage bonus
            defend.performActionOn(attack)

    def __attackDodge(self, attack, defend):
        """
        " Defender is dodging
        """
        resultAttack = attack.getSuccess()
        resultDefend = defend.getSuccess()

        if resultAttack > resultDefend:
            attack.performActionOn(actionAttack, successLevel=resultAttack)

As you can see in the final case, there's a bit of jiggery-pokery going on where resolution is based upon the type of class. This is undesired.

Ideally I'd want a solution which would allow for something more polymorphic. The structure can change however required if it leads to an elegant solution, as long as the results are the same.

The first thing that came to mind was further encapsulation of the fight mechanics. One thing I did have to change was how success was handled; now it's a member of each action subclass and is given a new value each time Controller.fight is called.

The other part necessary to this approach was the implementation of a action mode (offensive or defensive) so that actions like Dodge would know what to do.

import random

OFFENSIVE = 0
DEFENSIVE = 1

class Action(object):

    def __init__(self):
        super(Action, self).__init__()
        self.health = 100
        self.success = 0
        self.nextSuccess()

    def nextSuccess(self):
        self.success = random.randint(-2, 3)

    def performOn(self, subject, success):
        pass

class Dodge(Action):

    def __init__(self):
        super(Dodge, self).__init__()

    def performOn(self, subject):
        if mode == DEFENSIVE and subject.success > self.success:
            subject.performOn(subject, success=subject.success)

class Attack(Action):

    def __init__(self):
        super(Attack, self).__init__()

    def performOn(self, subject, mode):
        if self.success < 0 and subject.success < 0:
            return
        if mode == OFFENSIVE and self.success >= subject.success:
            subject.health -= 5 if success > 1 else 3
        elif mode == DEFENSIVE and self.success > subject.success:
            subject.health -= 3

class Manoeuvre(Action):

    def __init__(self):
        super(Attack, self).__init__()

class Controller:

    def fight(offense, defense):
        offensive = offense.nextSuccess()
        defensive = defense.nextSuccess()
        offense.performOn(defense, OFFENSIVE)
        defense.performOn(offense, DEFENSIVE)

As far as I can tell this does what the original script did. If there are any tweaks that need to be made, let me know!


C++ Address Book (2nd follow up) This will be my third post of this program, I have received a lot of great feedback and have learned a lot from the community. I have edited this program a lot, and am here for another review to see if I got it correctly. I had a hard time implementing one minor fix with output stream delimiter which was suggested in an earlier answer, but I believe I fixed the inheritance issues. Any other fixes would be very appreciated:

Person.h

#ifndef PERSON
#define PERSON
#pragma once

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>

class Person
{
    friend std::istream& operator>>(std::istream&,Person&);
    friend std::ostream& operator<<(std::ostream&, const Person&);
public:
    //comparison operator
    bool operator<(const Person&) const;
private:
    std::string name;
    std::string address;
};

#endif // !PERSON

Person.cpp

#include "Person.h"

bool Person::operator<(const Person& other) const {
    return name < other.name;
}

std::istream& operator>>(std::istream &in, Person &p){
    in >> p.name;
    getline(in, p.address);
    return in;
}

std::ostream& operator<<(std::ostream& os, const Person &p) {
    os << "Name: " << p.name << "\nAddress: " << p.address;
    return os;
}

Address_book.h

#ifndef ADDRESS_BOOK
#define ADDRESS_BOOK
#pragma once

#include "Person.h"

class Address_book
{
    friend std::istream& operator>>(std::istream&, Address_book&);
    friend std::ostream& operator<<(std::ostream&, Address_book&);
public:
    //set vector size to match appropriate number of entries
    void Address_book::resize(size_t entries);
    void Address_book::sort();

private:
    std::vector<Person> add_book;
};

#endif

Address_book.cpp

#include "Address_book.h"

//fills address book
std::istream& operator>>(std::istream& in, Address_book& abook) {
    for (int i = 0; i < abook.add_book.size(); ++i) {
        std::cin >> abook.add_book[i];
    }
    return in;
}

//sort address book
void Address_book::sort() {
    std::sort(this->add_book.begin(), this->add_book.end());
}
//prints contents of address book
std::ostream& operator<<(std::ostream& os, Address_book& abook) {
    for (int i = 0; i < abook.add_book.size(); ++i) {
        std::cout << abook.add_book[i] << "\n";
    }
    return os;
}

void Address_book::resize(size_t entries) {
    add_book.resize(entries);
}

main.cpp

#include <iostream>
#include <vector>
#include <string>
#include "Address_book.h"
#include "Person.h"

void pause();

int main() {

    size_t entries = 0;

    std::cout << "Enter number of entries to go in addressbook: ";
    std::cin >> entries;

    Address_book abook1;
    abook1.resize(entries);

    //input into addressbook
    std::cout << "Enter the name, followed by the address:\n";
    std::cin >> abook1;
    //sort addressbook
    abook1.sort();

    //output addressbook
    std::cout << "\n";
    std::cout << abook1;

    pause();
}

void pause() {
    std::string pause;
    std::cout << "Press any key followed by enter to continue...";
    std::cin >> pause;
}

This code is greatly improved!  Well done.  Still, there are some things that may help you improve your code.

Don't overspecify member functions

Within the Address_book.h file, the member functions are declared like this

void Address_book::resize(size_t entries);
void Address_book::sort();

But they should not include the class prefix in this context.  Instead, write them like this:

void resize(size_t entries);
void sort();

Be aware of signed vs. unsigned

Within the Address_book.cpp file, both functions include a line like this:

for (int i = 0; i < abook.add_book.size(); ++i) {

However, size() often returns an unsigned number, so it would be prudent to make i unsigned as well.  I'd recommend std::size_t as in

for (std::size_t i = 0; i < abook.add_book.size(); ++i) {

Use const where practical

Usually, an extractor (operator <<) does not alter the underlying object.  This is true of your code as well, which is good.  But to make it explicit, declare the parameter as const.  That is, the friend function should be this instead:

std::ostream& operator<<(std::ostream& os, const Address_book& abook);

Avoid the use of this in member functions

The code current contains this function:

void Address_book::sort() {
    std::sort(this->add_book.begin(), this->add_book.end());
}

However, it's usually not a good idea to explictly write this unless you have to.  I'd recommend shortening to this:

void Address_book::sort() {
    std::sort(add_book.begin(), add_book.end());
}

Think of the user

It's not so convenient to count the addresses manually first and then enter them.  Why not have the computer count them?  One simple way to do that would be to have a blank line or end of file signal the end of the input, avoiding the need for the user to manually count addresses first.


Recursively checking the css class of an iFrame Element I am writing some tests in selenium webdriver (on node.js). and have made a custom function to check the css value of an iFrame Element. I'm a coding beginner.

The script tests an app where the user writes the image width they want (just putting in a number) and the image in an iFrame should change width. This is tricky because one must switch iFrames, wait for elements to become stale (as the new image width is loaded) then grab the new element and check its css value.

Often, the tests are flaky because sometimes it checks before the value has changed etc. I finally wrote a function that passed 120 times out of 120 times.

//function looks for 'el', then switches iframe and extracts the desired cssValur, then compares it to the 'value' we expect
Page.checkCssValue = function (el, cssValue, value){
    //find function, the '0' represents the iframe index
    var newEl = this.find(el, 0);
    return newEl.getCssValue(cssValue).then(function(result){
        if(result !== value){
            console.log(result + " and " + value + " Do not Match.")
            return Page.checkCssValue(el, cssValue, value);
        }
        else{
            console.log(result + " and " + value + " Do Match!")
            return result;
        }
    });
};

But I'm not sure if this is considered bad programming and if a while loop would be better?

Code structure wise, the recursive pattern in JavaScript/Selenium and Protractor code is a pretty common one. The biggest problem here is that, you don't have a recursive cycle exit condition - if result would never become equal to value, you'll eventually get the recursive call stack size overflow error.
This is a "negative" case for you and would probably mean a test failure, but, in the world end-to-end UI tests, you have to be as specific in your test failures as possible (the "difficulties in finding the root cause for a test failure" is one of the reasons we should generally write more unit tests as opposed to end-to-end tests, according to Google Testing Pyramid).
A better approach to tackle flakiness would be to use Explicit Waits, which are design to continuously execute a function until it evaluates to true, or a timeout is reached:

this.wait( condition, opt_timeout, opt_message ) → Thenable
Schedules a command to wait for a condition to hold. The condition may be specified by a Condition, as a custom function, or as any promise-like thenable.
For a Condition or function, the wait will repeatedly evaluate the condition until it returns a truthy value. If any errors occur while evaluating the condition, they will be allowed to propagate. In the event a condition returns a promise, the polling loop will wait for it to be resolved and use the resolved value for whether the condition has been satisified. Note the resolution time for a promise is factored into whether a wait has timed out.

Here is how you can apply wait() in your case:
// function looks for 'el', then switches iframe and extracts the desired cssValue, then compares it to the 'value' we expect
Page.checkCssValue = function (el, cssValue, value) {
    // find function, the '0' represents the iframe index
    var newEl = this.find(el, 0);

    // wait for the desired CSS value
    var timeout = 5000;  // in milliseconds
    this.driver.wait(function() {
        return newEl.getCssValue(cssValue).then(function (result) {
            return result === value;
        });
    }, timeout, "CSS value '" + cssValue + "' has not become equal to '" + value + "'.");

    return newEl.getCssValue(cssValue);
};

where this.driver is your selenium webdriver instance, 5000 is a timeout value in milliseconds.


My own malloc() function in C Is this acceptable? What is the next step to improve algorithm and make them more clear and faster?

typedef struct heap_block
{
    struct heap_block* next;
    size_t size;
    bool isfree;
    int x; // for alligment
}header;

#define Heap_Capacity 16777216
unsigned long long active_size;  // # bytes in active allocations

static uint64_t heap[Heap_Capacity/sizeof(uint64_t)];

static char* max_heap =(char*)(&heap[0]);
static char* min_heap = (char*)(&heap[0]);
static char *END_CHK = "\xDE\xAD\xC0\xDA";
header* last_allocated;

void* malloc(size_t sz)
{
    if(sz == 0 || sz > Heap_Capacity)
    {
        return NULL;
    }

    header* block = (header*)heap;
    if(active_size == 0)
    {
        set_data_to_block(block, sz);
        return (void*)last_allocated;
    }

    while(block->next != NULL)
    {
        block = block->next;
    }

    block->next = (header*)((char*)to_end_data(block) + 8);
    header* new_block = block->next;
    set_data_to_block(new_block, sz);

    return (void*)last_allocated;
}

void set_data_to_block(header* block, size_t sz)
{
    block->size = sz;
    block->isfree = false;
    block->next = NULL;

    active_size += sz;
    last_allocated = block + 1;
    char* end_of_data_block = (char*)to_end_data(block);

    if(max_heap < end_of_data_block)
    {
        max_heap = end_of_data_block;
    }
    strcpy(end_of_data_block, END_CHK);
}

header* to_start_metadata(header* block)
{
    return --block;
}

header* to_end_data(header* block)
{
    return (header*)((size_t)(block+1) + block->size);
}

Avoid globals

last_allocated is only used to communicate the result of set_data_to_block back to malloc. Returning the result is much cleaner.
Avoid special cases

There is no need to special case the initial allocation. If you do not want to rely on ((header *) heap)->next == NULL initially, you may want to provide an initialization method (which I recommend anyway).


DynamicArray class in C++ I am pretty new to C++ and I think I have some problems of keeping code clean and optimized, especially I have problems with writing comments. I just don't know when should I be explaining what I write. Any tips and advices will be hugely appreciated!
Here's the code:
#include <iostream>
#include <initializer_list>
#include <stdexcept>

template <typename T>
class DynamicArray
{
public:

    DynamicArray(std::initializer_list<T> elements)
        : m_size(elements.size())
        , m_capacity(elements.size())
        , m_array(new T[elements.size()])
    {
        std::copy(elements.begin(), elements.end(), m_array);
    }

    DynamicArray()
        : m_size(0)
        , m_capacity(1)
        , m_array(nullptr)
    { }

    DynamicArray(DynamicArray<T>&& other)
        : DynamicArray()
    {
        swap(*this, other);
    }

    ~DynamicArray()
    {
        delete[] m_array;
    }

    DynamicArray(const DynamicArray& other)
        : m_capacity(other.m_capacity)
        , m_size(other.m_size)
        , m_array(new T[other.m_capacity])
    {
        std::copy(other.m_array, other.m_array + other.m_size, m_array);
    }

    DynamicArray& operator=(DynamicArray other)
    {
        swap(*this, other);
        return *this;
    }

    friend void swap(DynamicArray<T>& obj1, DynamicArray<T>& obj2) noexcept
    {
        std::swap(obj1.m_size, obj2.m_size);
        std::swap(obj1.m_capacity, obj2.m_capacity);
        std::swap(obj1.m_array, obj2.m_array);
    }

    void insert(int index, T value)
    {
        if (index != m_size) m_check_range(index);
        if (m_capacity == m_size) resize(m_capacity * 2);

        m_size++;

        for (int i = m_size - 2; i >= index; i--)
        {
            (*this)[i + 1] = (*this)[i];
        }

        (*this)[index] = value;
    }

    T remove(int index)
    {
        m_check_range(index);
        T to_remove = (*this)[index];

        for (int i = index; i < m_size - 1; i++)
        {
            (*this)[i] = (*this)[i + 1];
        }

        m_size--;
        return to_remove;
    }

    // Resizes array to store n elements.
    // Guarantees strong exception safety.
    void resize(int n)
    {
        T *temp = new T[n];
        m_capacity = n;

        std::copy(m_array, m_array + m_capacity, temp);
        delete[] m_array;
        m_array = temp;
    }

    void shrink_to_fit()
    {
        resize(m_size);
    }

    int size() const
    {
        return m_size;
    }

    bool empty() const
    {
        return m_size == 0;
    }

    int capacity() const
    {
        return m_capacity;
    }

    void append(T value)
    {
        insert(m_size, value);
    }

    void prepend(T value)
    {
        insert(0, value);
    }

    T pop(int index)
    {
        return remove(m_size - 1);
    }

    T front() const
    {
        return (*this)[0];
    }

    T back() const
    {
        return (*this)[m_size - 1];
    }

    void print() const
    {
        for (int i = 0; i < m_size; i++)
        {
            std::cout << m_array[i] << " ";
        }
        std::cout << std::endl;
    }

    T& operator[](int index)
    {
        m_check_range(index);
        return m_array[index];
    }

    const T& operator[](int index) const
    {
        m_check_range(index);
        return m_array[index];
    }

private:

    T *m_array;
    int m_capacity; // Capacity of the array
    int m_size ; // Number of added elements

    void m_check_range(int index) const
    {
        if (index < 0 || index >= m_size)
        {
            throw std::out_of_range("Index out of range!");
        }
    }
};

Observation
Array allocated and initializes capacity elements. Which sort of defeats the purpose of having a capacity. The point of using a capacity is that you should not be paying the price of calling the constructor on elements that you don't use.
Code Review
You should put your code into a namespace.

This does not seem correct.
    DynamicArray()
        : m_size(0)              // Size is zero fine.
        , m_capacity(1)          // But a capacity of 1 with no memory allocated!
                                 // I would say the capacity here is zero
                                 // as you have no room to add any elements.
        , m_array(nullptr)
    { }

You have accepted the initializer list elements by value.
    DynamicArray(std::initializer_list<T> elements)

So you have already made a copy of the list.
Since you have made a copy of the list the elements in the list are yours to mutate if you want to so here you could use move semantics rather than copy semantics.
        std::copy(elements.begin(), elements.end(), m_array);
        // RATHER DO THIS
        std::move(elements.begin(), elements.end(), m_array);

Normally you expect to see the move operators marked as noexcept
    DynamicArray(DynamicArray<T>&& other)

This allows certain optimizations when you use standard algorithms and containers.

Normally I write the swap function in terms of the swap method.
    friend void swap(DynamicArray<T>& obj1, DynamicArray<T>& obj2) noexcept
    {
        std::swap(obj1.m_size, obj2.m_size);
        std::swap(obj1.m_capacity, obj2.m_capacity);
        std::swap(obj1.m_array, obj2.m_array);
    }

    // I would write it like this:

    void swap(DynamicArray<T>& other) noexcept
    {
        using std::swap;
        swap(m_size,     other.m_size);
        swap(m_capacity, other.m_capacity);
        swap(m_array,    other.m_array);
    }
    friend void swap(DynamicArray<T>& lhs, DynamicArray<T>& rhs) noexcept
    {
        lhs.swap(rhs);
    }

Now methods (like move operators) don't need to call a free function to do a swap. They simply call the method version of swap and get the same affect.

Nice try.
    // Resizes array to store n elements.
    // Guarantees strong exception safety.

Unfortunately not quite. You can not do anything that may throw while the state of the object has been mutate but not in the final state.
    void resize(int n)
    {
        T *temp = new T[n];
        m_capacity = n;                                  // Mutation here

        std::copy(m_array, m_array + m_capacity, temp);  // Throw possible here
        delete[] m_array;                                // Throw possible here.
        m_array = temp;                                  // Final mutation
    }

The way to do this is in three phases:
    void resize(int n)
    {
        // Phase 1: Make your copy.
        T *temp = new T[n];
        std::copy(m_array, m_array + m_capacity, temp);

        // Phase 2: Mutate "ALL" your state to final state.
        //          You can not call anything that is not `noexcept`
        //          This limits you to very basic operations.
        m_capacity = n;
        std::swap(temp, m_array);

        // Phase 3: Clean up
        delete[] temp;           // Note the swap above.
    }

Note we can simplify this:
    void resize(int n)
    {
        // Phase 1: Make your copy.
        DynamicArray  copy(*this, n);  // Private constructor
                                       // Allocates n spaces but copies size
                                       // objects from the passed item.

        // Phase 2: Mutate "ALL" your state to final state.
        //          You can not call anything that is not `noexcept`
        //          This limits you to very basic operations.
        //          Luckily swap method is `noexcept`
        swap(copy);

        // Phase 3: Clean up
        // the object copy's destructor will do cleanup.
    }

I am a fan of one liners for methods that are this imple.
    bool empty() const
    {
        return m_size == 0;
    }

Its OK to have a print method. But to make it more C++ like I would also add the operator<< so you can stream it. This means modifying print to take a stream.
    void print(std::ostream& out = std::cout) const
    {
        for (int i = 0; i < m_size; i++)
        {
            out << m_array[i] << " ";
        }
        out << "\n";
    }
    friend std::ostream& operator<<(std::ostream& str, DynamicArray const& data)
    {
        data.print(str);
        return str;
    }

In the standard library containers that use operator[] make it a non checked interface. It is the responsibility of the user to validate before calling. They add an additional checked interface at() when the user want to validate that the index is range.
    T& operator[](int index)
    {
        m_check_range(index);
        return m_array[index];
    }

    // Think of this situation
    for(int loop = 0; loop < dA.size(); ++loop) {
        std::cout dA[loop];
    }

    // Here we have already guaranteed that the index is within the correct
    // range but we your code still forces an additional check (that we
    // know is never going to fail).

    // If I need to check then I call the `at()`version.
    printArrayElement(std::size_t index)
    {
         std::cout << dA.at(index);   // Use checked accesses
    }

Self Plug
I have a lot of other pointers in my articles about writing a vector:
https://lokiastari.com/series/


Prepared statement inside function that then return results I've created a function that makes a query of a database table and managed to use the results in another page where the function is called.

Is this approach good?

function cart_list($db, $user){
    $stmt = $db->prepare("
SELECT
    o.username,
    o.quantity,
    p.description,
    p.product_name,
FROM
    orders as o
INNER JOIN
    products as p
ON
    p.product_id = o.product_id
INNER JOIN
    users as u
ON
    o.username = u.username
WHERE
    o.username ='$user'
AND
    o.checkout ='0'");
return $stmt;
}

In the file where I called the function, I used:

$stmt = cart_list($db, $user);
$stmt->bind_result(
    $username,
    $quantity,
    $description,
    $product_name);
$stmt->execute();

while($stmt->fetch()){
echo $product_name;
}

To begin, your indentation is a little off at places, such as in the while loop in the second code block, or the return statement in the first.

It looks like you keep o.checkout as an integers like the = '0' suggests, but use strings, as the '' suggests. If you do measure solely in integers, you ought to change the database type.

WHERE
    o.username ='$user'

You should use bind_param() instead of straight injection, as it's safer for sanitisation.


Log around a function call I've got a module I use to log around functions that performs external requests.

# Functions to track when a block begins and ends
module LogAround

  # log at begin and end of a given block
  # with a mesage and the given params
  def log_around(message, *args)
    start_time = Time.now
    Rails.logger.info "#{message}(#{args.inspect}) - start"

    result = yield

  ensure
    end_time = Time.now - start_time
    Rails.logger.info "#{message}(#{args.inspect}) - end (#{end_time}s)"

    result
  end
end

This module is used in this way:

class Wrapper
  include LogAround
  # TODO: log_around :get_artist
  alias_method :_orig_get_artist, :get_artist

  # log the external request
  def get_artist(*args)
    log_around 'Discogs::Wrapper.get_artist', *args do
      _orig_get_artist *args
    end
  end
end

This solution is much closer to what I want compared with what I had before, but ideally what I'm looking for is a function in LogAround that could be used in this way

# this class is already defined by Discogs
# and this is a customization
class Wrapper
  include LogAround
  log_around :get_artist
end

and provides the same (or better params list) output, which is:

Discogs::Wrapper.get_artist(["pink floyd"]) - start
Discogs::Wrapper.get_artist(["pink floyd"]) - end (1.919805308s)

You may not define a free message, but I think this may be a solution:

module LogAround
  # log at begin and end of a given block
  # with a mesage and the given params
  def log_around( method_to_log )
    alias_method "_orig_#{method_to_log}".to_sym, method_to_log

    define_method(method_to_log ){ | *args, &blck |
      start_time = Time.now

      puts "call  #{method_to_log} with args #{args.inspect} #{'and a block' if blck} "
      result = send "_orig_#{method_to_log}".to_sym, *args, &blck

    end_time = Time.now - start_time
    puts "called  #{method_to_log} with args #{args.inspect} - (#{end_time}s)"
    result
    }
  end #self.log_around( method_to_log )
end

class Wrapper

  def test
    puts 'in test'
    sleep 1.2
  end

  def test_with_block
    puts 'in test_with_block'
    yield
  end

  extend LogAround
  log_around :test
  log_around :test_with_block
end

Wrapper.new.test
Wrapper.new.test_with_block { puts 'inside block' }

Remarks:

Rails.logger.info is replaced by puts
I extended, not included LogAround (log_around should be a class method, not an instance method).
not well tested ;)


Unit testing an unspecific order of method calls I cannot figure out how I am supposed to unit test the implementation of GetFilteredEntites(IEnumerable<MyEntity> myEntities).

public class MyEntity
{
    public int Rank { get; set; }
    public int Points { get; set; }
}

public interface IMyMethodClass
{
    ICollection<MyEntity> RemoveBadRanking(IEnumerable<MyEntity> myEntities);
    ICollection<MyEntity> RemoveLowPoints(IEnumerable<MyEntity> myEntities);
}

public class MyMethodClass : IMyMethodClass
{
    public ICollection<MyEntity> RemoveBadRanking(IEnumerable<MyEntity> myEntities)
    {
        return myEntities.Where(m => m.Rank > 100).ToList();
    }

    public ICollection<MyEntity> RemoveLowPoints(IEnumerable<MyEntity> myEntities)
    {
        return myEntities.Where(m => m.Points < 500).ToList();
    }
}

public class MyTargetClass
{
    private readonly IMyMethodClass _myMethodClass;

    public MyTargetClass(IMyMethodClass myMethodClass)
    {
        _myMethodClass = myMethodClass;
    }

    public ICollection<MyEntity> GetFilteredEntites(IEnumerable<MyEntity> myEntities)
    {
        var a = _myMethodClass.RemoveBadRanking(myEntities);
        var b = _myMethodClass.RemoveLowPoints(a);
        return b;
    }
}

public class MyTestClass
{
    [Fact]
    public void Gets_No_Bad_Ranking_And_No_Low_Points()
    {
        var myEntities = new List<MyEntity>();
        var myMethodClass = Substitute.For<IMyMethodClass>();
        var target = new MyTargetClass(myMethodClass);
        var a = new List<MyEntity>();
        myMethodClass.RemoveBadRanking(Arg.Is(myEntities)).Returns(a);
        var b = new List<MyEntity>();
        myMethodClass.RemoveLowPoints(Arg.Is(a)).Returns(b);

        var filteredEntites = target.GetFilteredEntites(myEntities);

        Assert.Same(b, filteredEntites);
    }
}

The above code tests and requires GetFilteredEntites(IEnumerable<MyEntity> myEntities) to be implemented in a correct way. The following implementation is just as correct as the first one:

public ICollection<MyEntity> GetFilteredEntites(IEnumerable<MyEntity> myEntities)
{
    var a = _myMethodClass.RemoveLowPoints(myEntities);
    var b = _myMethodClass.RemoveBadRanking(a);
    return b;
}

The unit test I have wrote above requires the method calls to be in the right order, but this is not needed for the implementation to be correct. The second implementation of GetFilteredEntites(IEnumerable<MyEntity> myEntities) (right above here) will not pass the unit test I wrote.

How would you change the unit test of GetFilteredEntites(IEnumerable<MyEntity> myEntities), so the order of the method calls is not required to be in a specific order?

You're right, the way you're mocking the methods is tightly coupling the sequence of events, even though you don't really care about the order.  What you really care about is that given a list, both methods are called, with the input/outputs chained and the overall output is the list without the filtered items.  You can achieve this by rewriting your test:

public void Gets_No_Bad_Ranking_And_No_Low_Points()
{
    // Create three entities, one to be removed by each of the filter functions
    // and one to be left over, since checking for an empty list isn't conclusive
    var badEntity = Substitute.For<MyEntity>();
    var lowEntity = Substitute.For<MyEntity>();
    var remainingEntity = Substitute.For<MyEntity>();

    // put all of the created entities into the initial list
    var myEntities = new List<MyEntity>() { badEntity, lowEntity, remainingEntity };
    var myMethodClass = Substitute.For<IMyMethodClass>();
    var target = new MyTargetClass(myMethodClass);

    // When RemoveBadRanking is called, we match against any parameter of the correct type,
    // then we filter the supplied list to exclude the badEntity
    myMethodClass.RemoveBadRanking(Arg.Any<IEnumerable<MyEntity>>())
        .Returns(args => ((List<MyEntity>)args[0]).Where(e => e != badEntity).ToList());

    // Do the same thing, but for lowEntity
    myMethodClass.RemoveLowPoints(Arg.Any<IEnumerable<MyEntity>>())
        .Returns(args => ((List<MyEntity>)args[0]).Where(e => e != lowEntity).ToList());

    var filteredEntites = target.GetFilteredEntites(myEntities);

    // If both substitutes have been called, then the returned list should only
    // have the remainingEntity in it.
    Assert.AreEqual(1, filteredEntites.Count);
    Assert.AreSame(remainingEntity, filteredEntites.First());
}

Whether or not this is an improvement is a bit subjective... it's trading the added complexity of the test for the looser coupling.


Code smell when recursively returning a List So this is part of my AVL tree implementation, namely the infix representation of the tree. Since I wanted to use a recursive method to stay close to the original algorithm and want to return a list, I have to store the values in a parameter.

The smelly part is in getInfix when I reference that parameter to return the list. I am not even sure what's the name of this code smell but it feels wrong. I am literally only calling getInfixInternal for its side effect! But I can't simply return a list either...

How can I make this better?

private List<T> getInfix() {
    List<T> result = new ArrayList<>();
    getInfixInternal(root, result);
    return result;
}

private void getInfixInternal(Node node, List<T> infixValues) {
    if (node != null) {
        getInfixInternal(node.getLeft(), infixValues);
        infixValues.add((T) node.getValue());
        getInfixInternal(node.getRight(), infixValues);
    }
}

My Node class (excluding the getters and setters for reasons of space):

public class Node <T extends Comparable>  {
    private T value;
    private Node leftChild;
    private Node rightChild;
    private Node parent;
}

I wouldn't say there is a code smell with what you have now. However, it could be made more OOP-friendly.

An actual problem is that you're using raw types for Node, and that is never a good idea. So, first of all, consider having this instead:

private List<T> getInfix() {
    List<T> result = new ArrayList<>();
    getInfixInternal(root, result);
    return result;
}

private void getInfixInternal(Node<T> node, List<T> infixValues) {
    if (node != null) {
        getInfixInternal(node.getLeft(), infixValues);
        infixValues.add(node.getValue());
        getInfixInternal(node.getRight(), infixValues);
    }
}

and

public class Node<T extends Comparable<? super T>>

Then, to avoid this getInfixInternal, you can reason with what the method getInfix means. The first thing to realize is that it works on a given Node. In fact, it is really a property of the node itself: every node can return a list of values resulting of a infix traversal of the subtree starting at this node. Therefore, instead of having an internal method taking a Node as parameter, make it a method of Node:

public class Node<T extends Comparable<? super T>> {

    // ...

    public List<T> getInfix() {
        List<T> result = new ArrayList<>();
        if (leftChild != null) {
            result.addAll(leftChild.getInfix());
        }
        result.add(value);
        if (rightChild != null) {
            result.addAll(rightChild.getInfix());
        }
        return result;
    }

}

With such a method, you can then have:

private List<T> getInfix() {
    return root == null ? Collections.emptyList() : root.getInfix();
}

With Java 8 Optional, you could potentially write that more concisely:

public List<T> getInfix() {
    List<T> result = new ArrayList<>();
    Optional.ofNullable(leftChild).map(Node::getInfix).ifPresent(result::addAll);
    result.add(value);
    Optional.ofNullable(rightChild).map(Node::getInfix).ifPresent(result::addAll);
    return result;
}

Final note: you could also consider returning unmodifiable lists.


Program slows down significantly based on input I've been writing a program to perform a kind of pattern-matching in XML and text files.

When my program reaches this section of the code, the CPU usage gets very high and the performance slows down to a point where the program seems to be frozen.  It actually isn't, but depending on the input (number of text files and their content), it may take several hours to complete the task.

I'm looking for a more efficient way to rewrite this section of the code:

List<string> CandidatesRet = new List<string>();

for (int indexCi = 0; indexCi < Ci.Count - 1; indexCi++)
{
    // generate all sub itemset with length-1

    string[] allItems = Ci[indexCi].Split(new char[] { ' ' });
    for (int i = 0; i < allItems.Length; i++)
    {
        string tempStr = "";
        for (int j = 0; j < allItems.Length; j++)
            if (i != j)
                tempStr += allItems[j] + " ";
        tempStr = tempStr.Trim();
        subItemset.Add(tempStr);
    }

    // THE PROBLEM BEGINS HERE

    foreach (string subitem in subItemset)
    {

        int iFirtS;
        for (int indexCommon = indexCi + 1; indexCommon < Ci.Count; indexCommon++)
            if ((iFirtS = Ci[indexCommon].IndexOf(subitem)) >= 0)
            {
                string[] listTempCi = Ci[indexCommon].Split(new char[] { ' ' });
                foreach (string itemCi in listTempCi)
                    if (!subitem.Contains(itemCi))
                        commonItem.Add(itemCi);
            }
        allCommonItems.Add(commonItem);
    }

    // generate condidate from common item
    foreach (string item in oldItemsetCi)
    {
        bool flagCi = true;
        foreach (List<string> listCommItem in allCommonItems)
            if (!listCommItem.Contains(item))
            {
                flagCi = false;
                break;
            }
        if (flagCi)
            CandidatesRet.Add((Ci[indexCi] + " " + item).Trim());
    }

There are many nested loops and I know this is the problem.  What do you think should be improved?

This should help a little bit:

        List<string> CandidatesRet = new List<string>();

        var splitChars = new[] { ' ' };

        for (var indexCi = 0; indexCi < Ci.Count - 1; indexCi++)
        {
            // generate all sub itemset with length-1
            var allItems = Ci[indexCi].Split(splitChars);

            subItemset
                .AddRange(allItems.Select((s, i) => allItems.Where((t, j) => i != j)
                .Aggregate(string.Empty, (current, t) => current + (t + " ")))
                .Select(tempStr => tempStr.Trim()));

            // THE PROBLEM BEGINS HERE
            foreach (var subitem in subItemset)
            {
                for (var indexCommon = indexCi + 1; indexCommon < Ci.Count; indexCommon++)
                {
                    if (Ci[indexCommon].IndexOf(subitem, StringComparison.Ordinal) < 0)
                    {
                        continue;
                    }

                    var listTempCi = Ci[indexCommon].Split(splitChars);

                    commonItem.AddRange(listTempCi.Where(itemCi => !subitem.Contains(itemCi)));
                }

                allCommonItems.Add(commonItem);
            }

            // generate condidate from common item
            CandidatesRet.AddRange(oldItemsetCi
                .Select(item => new { item, flagCi = allCommonItems.All(listCommItem => listCommItem.Contains(item)) })
                .Where(t => t.flagCi)
                .Select(t => (Ci[indexCi] + " " + t.item).Trim()));
        }
    }

However, the big problem is the still the double loop, which will grow as a power-of-two rather than linearly with your data size.


Validating company registration numbers N.B. The next stage of this project, which covers parsing filenames and checking numbers against the list produced in this question, is covered here.

I have a list of company numbers (UK companies registered at Companies House) in an Excel spreadsheet. A CRN is an 8-character unique identifier attached to a specific registered corporation (usually 8-digits but not always).

Examples of company numbers:

02492078
OC374102
03637689

This section of code handles:

Retrieving the list of numbers
Validating the numbers
Adding any unique values to a Dictionary object

Later on, they will be used to check if a particular CRN is among the list I am targeting by using the Dictionary.Exists() method.

Option Explicit

Public Const COMPANY_NUMBER_COLUMN As Long = 1
Public Const parentFolderPath As String = "S:\Investments\Data\Companies House\Monthly Companies House Downloads\"

Public Sub ParseAllCompanyRecords()
    '/ Data Structure: "Company Numbers", once input, will be stored as strings
    '/ Company Number: 8-character string, generally 8-digits but sometimes with text prefixes E.G. "OC374102"

    '/ Folder Path for monthly CH downloads: "S:\Investments\Data\Companies House\Monthly Companies House Downloads\"
    '/ Filename Strucutre of a Monthly Folder: [parentFolderPath]"Accounts_Monthly_Data-"[Full Month Name][yyyy]"\" - Square Brackets not in filename
    '/ Filename Structure of an individual filing: [Monthly Folder Path]"Prod224_0005_"[8-character Company Registration Number]"_"[yyyymmdd][.html OR .xml] - Square Brackets not in filename

    Dim targetCompanyNumbers As Scripting.Dictionary
    Set targetCompanyNumbers = GetTargetCompanyNumbers

End Sub

Public Function GetTargetCompanyNumbers() As Scripting.Dictionary

    Dim targetCompanyNumbers As Scripting.Dictionary
    Set targetCompanyNumbers = New Scripting.Dictionary

    Dim finalRow As Long, columnRange As Range
    With wsInputs
        finalRow = .Cells(.Rows.Count, COMPANY_NUMBER_COLUMN).End(xlUp).row
        Set columnRange = .Range(.Cells(2, COMPANY_NUMBER_COLUMN), .Cells(finalRow, COMPANY_NUMBER_COLUMN))
    End With

    Dim columnArray As Variant
    columnArray = columnRange

    Dim LB1 As Long, UB1 As Long
    AssignArrayBounds columnArray, LB1, UB1

    Dim ix As Long
    Dim elementValue As Variant
    Dim companyNumber As String

    For ix = LB1 To UB1
        elementValue = columnArray(ix, 1)
        companyNumber = CompanyNumberFromValue(elementValue)
        If Not targetCompanyNumbers.Exists(companyNumber) Then targetCompanyNumbers.Add companyNumber, companyNumber
    Next ix

    Set GetTargetCompanyNumbers = targetCompanyNumbers

End Function

Public Function CompanyNumberFromValue(ByVal vValue As Variant) As String
    '/ Company Number: 8-character string, generally 8-digits but sometimes with text prefixes E.G. "OC374102"

    Dim textValue As String, trimText As String
    Dim companyNumber As String

    textValue = CStr(vValue)
    trimText = Trim(textValue)

    If IsNumeric(trimText) Then
        companyNumber = Right("00000000" & trimText, 8) '/ Add back any leading zeroes that Excel may have stripped
    ElseIf Len(trimText) = 8 Then
        companyNumber = trimText
    Else
        '/ Not a number, not the correct number of digits. Invalid input.
        companyNumber = vbNullString
    End If

    CompanyNumberFromValue = companyNumber

End Function

'/ Data Structure: "Company Numbers", once input, will be stored as strings
'/ Company Number: 8-character string, generally 8-digits but sometimes with text prefixes E.G. "OC374102"

'/ Folder Path for monthly CH downloads: "S:\Investments\Data\Companies House\Monthly Companies House Downloads\"
'/ Filename Strucutre of a Monthly Folder: [parentFolderPath]"Accounts_Monthly_Data-"[Full Month Name][yyyy]"\" - Square Brackets not in filename
'/ Filename Structure of an individual filing: [Monthly Folder Path]"Prod224_0005_"[8-character Company Registration Number]"_"[yyyymmdd][.html OR .xml] - Square Brackets not in filename

I don't see the need for the / slash here. The VBE renders comments in a very distinctive green color: really, you couldn't miss a comment block if you tried:

Why fully qualify Scripting.Dictionary, but not Excel.Range?
Seriously, your code isn't bad at all.
Except I'm not sure I understand the need for AssignArrayBounds, and LB1/UB1 are awful names, as much as ix would be better off as either i or index.

This part can be simplified:

If IsNumeric(trimText) Then
    companyNumber = Right("00000000" & trimText, 8) '/ Add back any leading zeroes that Excel may have stripped

If trimText is numeric, then you can use the Format function to perform the padding:
If IsNumeric(trimText) Then
    companyNumber = Format(trimText, "00000000")

Simple as that!
I'd do away with multiple declarations in a single instruction:

Dim textValue As String, trimText As String
Dim companyNumber As String

textValue = CStr(vValue)
trimText = Trim(textValue)

...and move everything closer to their first usage:
Dim textValue As String
textValue = CStr(vValue)

Dim trimText As String
trimText = Trim$(textValue)

Dim result As String ' clearly the local I'm returning

Notice Trim$ here. Under the hood, Trim$ is really calling the VBA.Strings._B_str_Trim function; on the other hand, Trim is calling the VBA.Strings._B_var_Trim function. One returns a String, the other returns a Variant. By using the Variant variant, you're forcing an implicit conversion to a String, implicit conversion that can completely be avoided by calling the String-returning function in the first place.
The same applies to Right/Right$, respectively VBA.Strings._B_var_Right and VBA.Strings._B_str_Right. Rubberduck's inspections wiki will be updated for the 2.0 release with a full list of all these not-quite-string functions.


Ohm's Law calculator I'm looking for ways to make this calculator more efficient and/or make the code a little easier to read.

#include <iostream>
#include <cstdlib>
using namespace std;

// variables to be used in the program
double result, valueA, valueB, valA, valB;
int choice;
char cont;

//functions to perform the calculations

double mult(double valueA, double valueB) {
result = valueA * valueB;
return result;
}

double div(double valueA, double valueB) {
result = valueA / valueB;
return result;
}

// functions that prompt the user for values to be calculated

double getValueA(string unit) {
cout << "Enter " << unit << ": " << endl;
cin >> valA;
cout << endl;
return valA;
}

double getValueB(string unit) {
cout << "Enter " << unit << ": " << endl;
cin >> valB;
cout << endl;
return valB;
}

void title() {
cout << "**** NEO-VOLT ELECTRICAL CALCULATOR ****" << endl;
cout << "*   by *" << endl;
cout << "*Adam Pate *" << endl;
cout << "****************************************" << endl;
cout << endl << endl << endl;
}

void showMenu() {
cout << " Main Menu: " << endl;
cout << endl;
cout << "Ohm's Law" << endl;
cout << " 1. Voltage " << endl;
cout << " 2. Resistance " << endl;
cout << " 3. Current " << endl;
cout << "Power Law" << endl;
cout << " 4. Wattage " << endl;
cout << " 5. Voltage " << endl;
cout << " 6. Current " << endl;
cout << endl;
cout << " 0. Exit the program " << endl;

}
//Shows results of the calculation
void displayResult(string units) {
cout << result << " " << units;
cout << endl << endl;

}
// Takes the users choice and calls functions accordingly
void processMenu() {
cin >> choice;
switch(choice) {
case 0:
exit(0);
case 1:
getValueA("Amps");
getValueB("Ohms");
mult(valA, valB);
displayResult("Volts");
break;
case 2:
getValueA("Volts");
getValueB("Amps");
div(valA, valB);
displayResult("Ohms");
break;
case 3:
getValueA("Volts");
getValueB("Ohms");
div(valA, valB);
displayResult("Amps");
break;
case 4:
getValueA("Volts");
getValueB("Amps");
mult(valA, valB);
displayResult("Watts");
break;
case 5:
getValueA("Watts");
getValueB("Amps");
div(valA, valB);
displayResult("Volts");
break;
case 6:
getValueA("Watts");
getValueB("Volts");
div(valA, valB);
displayResult("Amps");
break;
default:
cout << "Invalid choice.";
}

}

int main()
{
title();
do {
showMenu();
processMenu();
cout << "Perform another calculation? (y or n)" << endl;
cin >> cont;
} while(cont != 'n');

return 0;
}

Indent your code.
There is no need to define functions for multiplication and division. It is just as clear to say a * b as mult(a,b).
getValueA and getValueB do exactly the same thing, except with a different global. The return value of these functions is ignored. It would be better to merge these into one and use the return value.
Don't use valA and valB for variable names. Use local variables with names like volt.
Don't use a global for the result. Pass it as a parameter to displayResult, like this:

case 1:
    double amps = getValue("Amps");
    double ohms = getValue("Ohms");
    displayResult(amps * ohms, "volts");

You can, and should, avoid global variables completely here, as valueA and valueB are never used in the global context, they can be removed.
If the user types "no" or "N" it is treated as Yes. Handle this more carefully. For example, consider only the lowercase of the first letter.


DailyProgrammer Balancing Words Challenge I'm hoping for someone to help me use the best practices and further make my code better that I used to accomplish the DailyProgrammer Challenge "Balancing Words".

The challenge is to check if a word will "balance" and if it will, where it balances. For a word to balance at a letter, all of the weight to the left and to the right of the letter have to be equal. Weight is calculated by the letter's index in the alphabet (A = 1, B = 2 ... Z = 26) multiplied by the letter's distance from the balance point.

For example, with the input of STEAD, the program should output "S T EAD - 19" since STEAD balances at the T. The left hand side of T is just S. S = 19(th letter in the alphabet) * 1(distance from T) = 19. The right hand side of T is EAD = 1 * 5 + 2 * 1 + 3 * 4 = 19. Since both sides are 19 when T is the middle, T is the balance point so I print out the word STEAD with spaces around T "S T EAD" and the weight of either side (since they're equal) " - 19".

#include <iostream>
#include <string>
#include <vector>
using namespace std;

int main() {
    /*
     * Enter multiple lines of input separated by '\n', when a line is empty stop collecting input.
     */
    cout << "Enter input in capital letters, just press enter to stop." << endl;
    vector<string> input;
    string inputLine;
    while (getline(cin, inputLine)) {
        if (inputLine != "") {
            input.push_back(inputLine);
        }
        else {
            break;
        }
    }

    /*
     * This initial for loop is just cycling
     * through all of the words from the input.
     */
    bool balances = false;
    for (string &word : input) {
        /*
         * This for loop goes through all of
         * the letters in the current word.
         */
        for (int i = 0; i < word.length(); ++i) {
        int leftWeight = 0;
        int rightWeight = 0;
            /*
             * This loop goes through all of the letters to the left
             * of the currently selected "balance point" and calculates
             * the total "weight" on the left side.
             */
            for (int j = 0; j < i; ++j) { // Left of index
                leftWeight += (1 + word[j] - 'A') * (i - j);
            }
            /*
             * Same as above, but for the right-side of the "balance point."
             */
            for (int j = i; j < word.length(); ++j) {
                rightWeight += (1 + word[j] - 'A') * (j - i);
            }

            balances = leftWeight == rightWeight;
            /*
             * This exits out as soon as the working "balance point" is found!
             */
            if (balances) {
                cout << word.substr(0, i) << " " << word[i] << " " << word.substr(i + 1, word.length()) << " - " << leftWeight << endl;
                break;
            }
        }
        if (!balances) {
            cout << word << " DOES NOT BALANCE." << endl;
        }
        cout << endl;
    }
}

General Coding Advice

Do not use using namespace std.  See here and here.
Wrap your logic into a function.  This typically allows for better control flow.

// Returns the position that the word balances
// Returns 0 if the word does not balance (since words can never balance at 0)
int balances(const std::string& word)
{
    // Some loop
        // If it balances return the position
        // Also note that a word can only balance in one position
    // End of loop
    // Return 0 since it does not balance
}

for (string&& word : input) {
    int pos = balances(word);
    if(pos)
    {
        // print something
    }
    else
    {
        // print something else
    }
}

Personally I would prefer using unsigned int or better yet size_t to index the string but you can run into problems using unsigned int so you can stick to int for now.

Faster stop condition

leftWeight is monotically increasing while rightWeight is monotonically decreasing.  So a faster stop condition is leftWeight > rightWeight

Algorithm hint

You do not need to recompute everything each iteration.

Note that \$A\$, \$B\$, \$C\$, ... arbitrary values here and not letters in the alphabet.

Assume we have some number \$N_i = 5A + 4B + 3C + 2D + E\$ and we desire some number \$N_{i+1} = 4A + 3B + 2C + D\$.

It sure would be nice if we had some number \$M_i = A + B + C + D + E\$ so that \$N_{i+1} = N_i - M_i\$

But then how do we update \$M_i\$ to \$M_{i+1}\$ so that \$N_{i+2} = 3A + 2B + C = N_{i+1} - M_{i+1}\$?

And if for some reason we had \$N_{i+1}\$, \$M_{i+1}\$, and some new value \$E\$ could we use these to compute \$N_i\$?

If we could do all of the above \$O(1)\$ then our overall algorithm would be \$O(n)\$ instead of the \$O(n^2)\$ solution where you recalculate everything.


Are fetchAll() and for() good choices for my code? I use the following code: fetchAll and for($mich=0;$mich<10;$mich++).

Is this the best way?

Is there a better way, either to optimize or replace this code?

<?php
include("cache/phpfastcache.php");
phpFastCache::setup("storage","auto");
$cache = phpFastCache();
$products = $cache->get("product_page");
if($products == null) {
    $akhbarkotah1 = $conn->prepare("SELECT text,time FROM small WHERE active='0' ORDER BY time DESC LIMIT 10");
    $akhbarkotah1->execute();
    $products = $akhbarkotah1->fetchAll();
    $cache->set("product_page", $products,30);
}
for($mich=0;$mich<10;$mich++){
    $text1 =$products[$mich][0];
    $time1 =$products[$mich][1];
    ?>

<?php echo $text1; ?>

                <?php echo timeTonow($time1); ?>

    <?php
}
?>

Using fetchAll usually is not a problem. There are some things to optimize here in my opinion:

Use a foreach loop instead of for loop. Your result is already limited. No need to duplicate this here. This removes the iterator counter and assignment:

foreach ($products as $product) {
    $text1 = $product[0];
    $time1 = $product[1];
?>

Using the Object fetch mode (returns objects instead of indexed arrays) makes this even more readable and saves you the name assignments:

foreach ($products as $product) {
     // $product->text1; $product->time1;
?>

Though I'd recommend to remove these. Introducing numbers in variable names usually suggests there is a second one. E.g. time2.
As @mat's mug pointed out, indentation is very important. Same goes for empty lines. There are great to structure your code in logical blocks. For this example here, I'd have an empty line after setting up the cache and one after loading the products.


Prefix Sum in Ruby, Genomic Range Query from Codility I'm currently going through some lessons on Codility. I've just spent a couple of hours with GenomicRangeQuery, which is intended to demonstrate the use of prefix sums.

The task description is here. Broadly, the idea is that given a string and a set of arbitrary cut points one should be able to return the lowest value character in a given slice. O(N*M) complexity is what we want to avoid.

My solution scores 100%, but I would still value any feedback on style and readability.

def solution(str, slice_starts, slice_ends)
    # Initiate an array to hold rolling totals of character occurrences
    prefix_sums = [0,0,0,0]]
    # Map possible characters to their positions in the prefix sum array
    chars = { "A" => 0, "C" => 1, "G" => 2, "T" => 3 }

    str.split('').each_with_index do |char, i|
        prefix_sums[i + 1] = prefix_sums[i].clone
        prefix_sums[i + 1][chars[char]] += 1
    end

    slice_starts.each_with_index.map do |slice_start, i|
        s = slice_start
        e = slice_ends[i]

        chars.each do |char, ii|
            occurrences = prefix_sums[e + 1][ii] - prefix_sums[s][ii]
            break (ii + 1) if occurrences > 0
        end
    end
end

Updated

Here's my current preferred version using tips from some of the answers below. I should probably also use an array of hashes, rather than arrays, for the prefix sums, but I'm lazy.

def solution(str, slice_starts, slice_ends)
  prefix_sums = [[0] * 4]
  chars = { "A" => 0, "C" => 1, "G" => 2, "T" => 3 }

  str.chars.each_with_index do |char, i|
      prefix_sums[i + 1] = prefix_sums[i].clone
      prefix_sums[i + 1][chars[char]] += 1
  end

  slice_starts.zip(slice_ends).map do |s, e|
    chars.each do |char, ii|
      occurrences = prefix_sums[e + 1][ii] - prefix_sums[s][ii]
      break (ii + 1) if occurrences > 0
    end
  end
end

Some notes:

Use 2-space indentation.
[[0,0,0,0]]. Always a space after a comma.
str.split('') - >str.chars
prefix_sums = [[0,0,0,0]]. This kind of implicit structures hinder readability, I'd simply use a hash. A small space penalty (not in big-Oh, though), better readibility.
each_with_index. Use zip instead.
As you say, the problem is about getting partial sums (frequencies). You can implement and use a very generic abstraction (Enumerable#scanl), which can be used in more codility problems.

I'd write this purely functional solution:

def solution(input_str, slice_start, slice_end)
  impact_factors = {"A" => 1, "C" => 2, "G" => 3, "T" => 4}
  frequencies = input_str.chars.scanl(Hash.new(0)) do |freq, n|
    freq.merge(n => freq[n] + 1)
  end

  slice_start.zip(slice_end).map do |from, to|
    difference_count_by_factor = frequencies[to+1].map do |n, count|
      [impact_factors[n], count - frequencies[from][nucleotide]]
    end.to_h
    difference_count_by_factor.reject { |factor, count| count.zero? }.keys.min
  end
end


Shorten a custom JS sort function I have a sort function that needed to sort in a certain way. That position was sorted by mobile-top first, then empty position followed by mobile-bottom. Then on these I needed to sort by their priority, with priority 1, 2 and then blank.

  widgetInfo = [
['box1','mobile-bottom',''],
['box2','mobile-top','priority-1'],
['box3','mobile-top',''],
['box3','mobile-top',''],
['box4','mobile-bottom',''],
['box2','mobile-top','priority-1'],
['box5','mobile-top','priority-2'],
['box6','','']],
 ['box5','mobile-top','priority-2'];

var newArray = widgetInfo.sort(function(a, b){

  if(a[1] == 'mobile-top'){
    if(b[1] == 'mobile-bottom'){
      return -1;
    }
    else if(b[1] == 'mobile-top'){

      if(a[2] == 'priority-1'){

        if(b[2] == 'priority-1'){
          return 0;
        }
        else if(b[2] == 'priority-2'){
          return -1;
        }
        else{
          return -1;
        }
      }

      else if(a[2] == 'priority-2'){

        if(b[2] == 'priority-1'){
          return 1;
        }
        else if(b[2] == 'priority-2'){
          return 0;
        }
        else{
          return -1;
        }
      }

      else{
        return 1;
      }

    }
    else{
      return -1;
    }
  }
  if(a[1] == 'mobile-bottom'){
   if(b[1] == 'mobile-top'){
     return 1;
   }
    else if(b[1] == 'mobile-bottom'){
     return 0;
   }
    else{
      return 1;
    }
  }

});

It seems like one heck of a lot of code to do this and I'm certain there will be better ways. Any suggestions?

You can write a function to compare two "position" strings and a function to compare "priority" strings. Then a resulting comparison function could be written easily by calling the two already defined functions.

Now for the actual review, you don't need the if(b[1] == 'mobile-bottom') check in the if(a[1] == 'mobile-top') case, checking first if if(b[1] == 'mobile-top') is enough, in the other case, you can simply return -1.

Similarly, if(a[1] == 'mobile-bottom'), you only need to consider if(b[1] == 'mobile-bottom'), otherwise, you can simply return 1.


Trimming whitespace from the beginning and ending of a file In the code below, I want to remove the "white space character" from 2 places only, from the end of 1st line and from the end of last line. How can I optimize it ?

Platform : Linux , gcc compiler

int remove_special_chars(char *file_path, char *dest_file, int flag)
{
    PRINT_FUN_NAME

    struct stat st;
    unsigned char *fileData=NULL;
    FILE *fp = NULL, *dest_file_fp = NULL, *intermidiate_fp = NULL;
    char tmp_str[256] = {0}, str_buffer[100] = {0};
    int file_size = 0, count = 0, read_len = 0;

    /* I have taken 3 files here "file_path" is
       the source file from which i am reading
       data and intermidiate_path is the 2nd file
       in which  iam storing data after removing
       the space from 1st line. Then iam reding
       the intermidiate file and removing the space
       from last line and finally storing it into
       3rd file i.e is "dest_file". */

    if ((fp = fopen(file_path, "r")) == NULL) {
        DPRINTF("File Opening Failed!\n");
        return -1;
    }
    if((intermidiate_fp = fopen(INTERMEDIATE_FILE_PATH, "wb+")) == NULL) {
        DPRINTF("File Opening Failed!\n");
        return -1;
    }
    if((dest_file_fp = fopen(dest_file, "wb")) == NULL) {
        DPRINTF("File Opening Failed!\n");
        return -1;
    }

    while(fgets(tmp_str, sizeof(tmp_str), fp) != NULL) {
        DPRINTF("tmp_str : %s\n", tmp_str);

        fputs(tmp_str, intermidiate_fp);

        if(count == 0) {
            rewind(intermidiate_fp);
            strncpy(str_buffer, tmp_strstrlen(tmp_str));
            str_buffer[strlen(str_buffer) - 1] = '\0';
            DPRINTF("str_buffer : %s\n", str_buffer);
            fputs(str_buffer, intermidiate_fp);
            count ++;
        }
    }

    rewind(intermidiate_fp);

    stat(INTERMEDIATE_FILE_PATH, &st);
    if (st.st_size == 0) {
        return (-2);
    }
    file_size = st.st_size;
    DPRINTF("File size : %d\n", file_size);
    fileData = (unsigned char *)malloc(sizeof(char)*file_size);
    if(fileData == NULL) {
        DPRINTF("Unable to allocate size for fileData!\n");
        return -1;
    }
    if ((read_len=fread(fileData, 1, file_size, intermidiate_fp)) == 0) {
        DPRINTF("Reading Failed!\n");
        free(fileData); fileData=NULL;
        return (-3);
    }
    fileData[(file_size-1)] = '\0';
    DPRINTF("File Data : %s\n", fileData);

    fputs((char*)fileData, dest_file_fp);

    fclose(dest_file_fp); fclose(fp);
    fclose(intermidiate_fp);

    return 0;
}

A couple lines that bother me:

strncpy(str_buffer, tmp_strstrlen(tmp_str));

First, bear in mind that strncpy does not place a \0 terminator at the end of the string if the buffer size limit is reached (but it does in every other case!).  Thus, you have to be extremely careful when using strncpy.

Second, you're copying a large (256-byte) buffer into a smaller (100-byte) buffer, and you're (presumably) passing the length of the larger buffer to strncpy.  If tmp_str holds a string that's too long, you'll get a buffer overflow.

str_buffer[strlen(str_buffer) - 1] = '\0';

When the string has length zero (e.g. the file is empty or did not have a trailing newline), the array access will be out of range.  Otherwise, it will blindly delete the last character, even if it wasn't whitespace.

The lines that I highlighted will work... if you make a couple assumptions:

Every line of input has < 100 characters
The "whitespace characters" we want to delete always exist.

However, it looks like you're deleting the trailing newline returned by fgets.  I'm not even sure your code works.

If I were you, the first step I'd take is to come up with a concise specification of what you're trying to do.  Correct me if I'm wrong, but it sounds like you want to do this:

first line: s/[\t ]$//
last line:  s/[\t ]$//
Lines are terminated by '\n', except the last line might not have a terminator.

The s/[\t ]$// syntax is borrowed from sed, and means: if there's a '\t' or ' ' character at the end of the string, remove it (i.e. replace it with an empty string).

Then, observe that this operation can be done with a trivial amount of buffering.  When a newline is detected, hang on to the last character in the line, and only emit it when you're not on the first line and you know that more lines follow.  Thus, you don't need an intermediate file.

I recommend implementing a helper function with this signature:

int remove_trailing_spaces(FILE *in, FILE *out);

You should only need to fread and fwrite in large chunks.  The int lets you return an error code if one of these functions fails.


Desktop windows code What do you say about the code structure below? This is one of the recommended ways of writing JS code in my education.

What is the pros and cons with writing code this way?
Can you recommend any better and maybe more elegant ways of writing code?

var DESKTOP = DESKTOP || {};
DESKTOP.Init = DESKTOP.Init || {};
DESKTOP.Windows = DESKTOP.Windows || {};
DESKTOP.desktop = DESKTOP.desktop || {};

DESKTOP.Windows = function(width, height){
    this.width = width;
    this.height = height;
};

DESKTOP.Windows.prototype.buildWindow = function(){
    // code here...
};

DESKTOP.Windows.prototype.showMenu = function(text){
    // code here...
};

DESKTOP.Windows.bgChange = function(width, height){
    DESKTOP.Windows.call(this, width, height);

    this.content = 'the content';
    this.title =  'the title'
};

DESKTOP.Windows.bgChange.prototype = new DESKTOP.Windows();

DESKTOP.Windows.bgChange.prototype.loadImages = function(){
    // code here...
};

DESKTOP.Desktop = function(){
    // code here...
};

and so on...

Oh gosh... that looks horrible, whoever is teaching you has no idea of JS whatsoever.

I'll give you a quick start on "good" structure.

A bit on naming:

UPPERCASE = Constant
UpperCamelCase = Class
everythingElse = method / function / variable

Write a helper for defining namespaces, everything else will become unmaintainable soon.

namespace('Desktop.Windows');

Desktop.Init = function() {

};

Desktop.Windows.prototype = {

    buildWindow: function() {

    },

    showMenu: function() {

    }

};

No idea what bgChange is supposed to be, a sub-class?

If you want to have "sub class" behavior:

namespace('foo.Base');
foo.Base = function(a, b, c) {

};

// Write your self a generic extend(a, b, c...) function which copies the values from b, c... onto a
extend(foo.prototype, {

    show: function() {

    },

    other: function() {

    }

});

namespace('foo.Extended');
foo.Extended = function(a, b, c, d) {

    foo.Base.call(this, a, b, c);
    // use d

};

extend(extended.prototype, foo.prototype, {

    // "Overwrite"
    show: function() {
        foo.Base.prototype.show.call(this, ...); // "super call"
    }

});


Returning the number of records per day I'm writing a query to get the number of sessions created each day for the past 90 days. I'm currently using a prepared statement to do this - looks something like this:

for($n = 0; $n < 90; ++$n){
    $days[] = $n;
}

$st = $this->db->prepare("
    SELECT *
    FROM `sessions`
    WHERE DATE(sessionStart) = DATE_SUB(CURDATE(), INTERVAL :n DAY)
");

$bind = array(':n' => null);
$data = array();

foreach($days as $day){
    $bind[':n'] = $day;
    $st->execute($bind);
    $data[$day] = $st->rowCount();
}

Would my code perform better using either of these?

This code, which is essentially running 90 separate queries
Running one query to return everything from the sessions table and then sorting the results in PHP

Aside from @Sumurai8 recommendations, the sql query can be indeed complexified a bit to return all the results in one go:

SELECT DATE_FORMAT(`sessionStart`, '%Y-%m-%d') AS `day`, COUNT(*) AS `sessions`
FROM `sessions`
WHERE
    DATE(`sessionStart`) BETWEEN DATE_SUB(CURDATE(), INTERVAL 90 DAY) AND CURDATE()
GROUP BY
    `day`
ORDER BY
    `day`

It only needs to be executed once, and then iterated over the result set using your favorite FETCH_XXX method.


Fast-resizing uint8 buffer I wanted a buffer like std::vector<uint8_t> which is very fast at resizing, and it does not require byte initialization (it resizes to 16K, then to 2-4 bytes, then goes back to 16K, and repeat, a lot. std::vector zeroes all the bytes every time it grows back to 16K, I thought I could avoid the zeroing-overhead with a custom implementation)

I came up with this:

// buffer which should be faster than std::vector<uint8_t> when resizing a lot because it does not do byte initialization when resizing
class uint8_fast_buffer
{
public:
    uint8_fast_buffer(const size_t initial_size)
    {
        if(initial_size == 0)
        {
            // .. i don't really like the idea of buf being nullptr, this avoids that issue.
            this->internal_reserve(1);
        }
        else
        {
            this->internal_reserve(initial_size);
            this->buf_size=initial_size;
        }
    }
    ~uint8_fast_buffer() noexcept
    {
        free(this->buf);
    }
    size_t size(void) noexcept
    {
        return this->buf_size;
    }
    void reserve(const size_t reserve_size)
    {
        if(reserve_size > this->buf_size_real)
        {
            this->internal_reserve(reserve_size);
        }
    }
    // this function is supposed to be very fast when newlen is smaller than the biggest it has ever been before.
    void resize(const size_t newlen)
    {
        if(__builtin_expect(newlen > this->buf_size_real,0))
        {
            this->internal_reserve(newlen);
        }
        this->buf_size=newlen;
    }
    void append(const uint8_t *data, const size_t len){
        const size_t pos=this->size();
        const size_t new_pos=this->size()+len;
        this->resize(new_pos);
        memcpy(&this->buf[pos],data,len);
    }
    void reset(void) noexcept
    {
        this->buf_size=0;
    }
    bool empty(void) noexcept
    {
        return (this->buf_size==0);
    }
    uint8_t* data(void) noexcept
    {
        return this->buf;
    }
    uint8_t& at(const size_t pos)
    {
        if(__builtin_expect(pos >= this->size(),0))
        {
            throw std::out_of_range(std::to_string(pos)+std::string(" >= ")+std::to_string(this->size()));
        }
        return this->buf[pos];
    }
    uint8_t& operator[](const size_t pos) noexcept
    {
        return this->buf[pos];
    }
private:
    void internal_reserve(const size_t reserve_size)
    {
        uint8_t *newbuf=(uint8_t*)realloc(this->buf,reserve_size);
        if(__builtin_expect(newbuf == nullptr,0))
        {
            throw std::bad_alloc();
        }
        this->buf_size_real=reserve_size;
        this->buf=newbuf;
    }
    size_t buf_size=0;
    size_t buf_size_real=0;
    uint8_t *buf=nullptr;
};

Using this-> to refer to class members
Ditch this. It's unnecessary unless you need to have to refer to inherited members or disambiguate any other variables or parameters.


Resolve alias' original item using bookmarkData This is my first Objective-C++ code, so I'm a bit lacking in the knowledge of methods.

I need to resolve a macOS alias and get its original location. I have access to the C string, which is the file path (in a human readable form). It also needs to be verified in this code itself that it is a true alias, and contains bookmark data.

const char *class::resolveAliasFromURL(const char *filepath) const{

    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
    char filepath[] = "/Users/me/alias"; /*you need to make an alias here*/
    NSError *error = nil;
    NSString *string = [[NSString alloc] initWithUTF8String:filepath];
    NSURL *url = [NSURL fileURLWithPath:string];
    NSData *bmdata = [NSURL bookmarkDataWithContentsOfURL:url error:&error];
    if (!error) { /*needs better error catching*/
        NSLog(@"Error: %@",error);
    }
    else {
        NSLog(@"URL: %@",url); // Useless; sanity check
    }

    error = nil;
    BOOL isstale = NO;
    BOOL *isStalePointer = &isstale;
    char ref[] = "";
    NSString *relString = [[NSString alloc] initWithUTF8String:ref];
    NSURL *relativeURL = [NSURL URLWithString:relString];
    NSURLBookmarkResolutionOptions option = NSURLBookmarkResolutionWithoutUI;
    NSURL *actual = [[NSURL alloc]
        initByResolvingBookmarkData:bookmarkData
                            options:option
                      relativeToURL:relativeURL
                bookmarkDataIsStale:isStalePointer
                              error:&error]; /*resolved NSURL object file://<some numbers>*/

    char resolved[100]; /*resolved path in C; need to get length of the string, not arbitrarily 100 */
    strcpy(resolved, [actual fileSystemRepresentation]);
    printf(" %s\n",resolved);

    /*Need to check if the resolved object is a file or a directory*/

    [pool drain];
    return tempPath;
}

bookmarkDataWithContentsOfURL:

+ (NSData *)bookmarkDataWithContentsOfURL:(NSURL *)bookmarkFileURL
                                    error:(NSError * _Nullable *)error;

initByResolvingBookmarkData

- (instancetype)initByResolvingBookmarkData:(NSData *)bookmarkData
                                    options:(NSURLBookmarkResolutionOptions)options
                              relativeToURL:(NSURL *)relativeURL
                        bookmarkDataIsStale:(BOOL *)isStale
                                      error:(NSError * _Nullable *)error;

Is there some improvements I can make given the goal and constraints?

Memory management

The use of NSAutoreleasePool makes it apparent that this code is compiled  manual reference counting. That is error-prone, as you'll see now: Here

NSString *string = [[NSString alloc] initWithUTF8String:filepath];
NSString *relString = [[NSString alloc] initWithUTF8String:ref];
NSURL *actual = [[NSURL alloc] initByResolvingBookmarkData ...];

are objects allocated but never released. That makes several memory leaks on each invocation of the method.

The easiest fix would be to use automatic reference counting (ARC) instead. ARC is available since Mac OS 10.6, and there is no reason not to use it. You only have to adjust the compiler flags (i.e. do not pass -fno-objc-arc to the compiler) and replace (compare this)

NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
// ...
[pool drain];

by

@autoreleasepool {
    // ...
}

The compiler takes care of everything else and inserts retain and release calls where appropriate.

If – for whatever reason – switching to ARC is not feasible then you have to fix the above-mentioned issues. You can add the needed release calls, e.g.

NSString *string = [[NSString alloc] initWithUTF8String:filepath];
// ...
[string release];

or (perhaps simpler) create autoreleased objects (which are released on return of the function, when the autorelease pool is drained):

NSString *string = [NSString stringWithUTF8String:filepath];
NSURL *actual = [NSURL URLByResolvingBookmarkData ...];

string can also more simply be created as a “boxed C string”:

NSString *string = @(filepath);

Another error is here:

char resolved[100]; /*resolved path in C; need to get length of the string, not arbitrarily 100 */
strcpy(resolved, [actual fileSystemRepresentation]);
// ...
return tempPath;

because the function returns the address of a local variable, which is undefined behavior.

You can make the character buffer static: That seems to be what similar functions in the blender project do. The disadvantage is that subsequent calls to the function overwrite the buffer.

Or you can allocate a new string, and the caller has to release the memory eventually.

Error handling

The correct way to check for success or failure of Cocoa (Touch) methods is
documented in
"Handling Error Objects Returned From Methods" in the "Error Handling Programming Guide":

  Important: Success or failure is indicated by the return value of the
  method. Although Cocoa methods that indirectly return error objects in
  the Cocoa error domain are guaranteed to return such objects if the
  method indicates failure by directly returning nil or NO, you should
  always check that the return value is nil or NO before attempting to
  do anything with the NSError object.

In your case, the correct way to check for the success of

NSData *bmdata = [NSURL bookmarkDataWithContentsOfURL:url error:&error];

would be

if (bmdata == nil) {
    NSLog(@"Error: %@",error);
}

But NSLog()ing the error is not sufficient: You have to report the failure to the caller, e.g. by returning a nullptr:

if (bmdata == nil) {
    NSLog(@"Error reading bookmark data for %@: %@",url, error);
    [pool drain];
    return nullptr;
}

Note that (if you keep the manual reference counting) you must take care that the autorelease pool is drained on any return from the function.

The same error handling must be done when resolving the bookmark data, that is missing completely.

More remarks

relString and relativeURL are not needed, you can simply pass nil to the relativeToURL argument.
isStalePointer is not needed, you can pass &isstale directly to the bookmarkDataIsStale argument. Or pass nil is you are not interested in the outcome (at present, the staleness is not evaluated in your code).

File system paths conversions

The conversion from an NSURL back to a C string with

char resolved[100];
strcpy(resolved, [actual fileSystemRepresentation]);

suggests that the C strings represent “file system paths” and in that case the correct counterpart for the conversion from the given filepath to an NSURL is

NSURL *url = [NSURL fileURLWithFileSystemRepresentation:filepath
                                            isDirectory:NO
                                          relativeToURL:nil];

But a buffer with 100 characters may be too short: Either determine the needed length dynamically, or use MAXPATHLEN from the system include files. Also strcpy does not check if the target buffer is large enough (potentially causing a memory corruption). Better use

static char resolved[MAXPATHLEN];
if (![actual getFileSystemRepresentation:resolved maxLength:sizeof(resolved)]) {
    NSLog(@"File system path too long for %@", url);
    [pool drain];
    return nullptr;
}

Putting it together

Putting all the above suggestions together, the code (with manual reference counting) would be

const char *YourClass::resolveAlias(const char *filepath) const {

    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
    NSURL *url = [NSURL fileURLWithFileSystemRepresentation:filepath
                                                isDirectory:NO
                                              relativeToURL:nil];
    NSError *error = nil;
    NSData *bmdata = [NSURL bookmarkDataWithContentsOfURL:url error:&error];
    if (bmdata == nil) {
        NSLog(@"Error reading bookmark data for %@: %@",url, error);
        [pool drain];
        return nullptr;
    }
    NSURL *actual = [NSURL URLByResolvingBookmarkData:bmdata
                                              options:NSURLBookmarkResolutionWithoutUI
                                        relativeToURL:nil
                                  bookmarkDataIsStale:nil
                                                error:&error];
    if (actual == nil) {
        NSLog(@"Error resolving bookmark data for %@: %@", url, error);
        [pool drain];
        return nullptr;
    }

    static char resolved[MAXPATHLEN];
    if (![actual getFileSystemRepresentation:resolved maxLength:sizeof(resolved)]) {
        NSLog(@"File system path too long for %@", url);
        [pool drain];
        return nullptr;
    }

    [pool drain];
    return resolved;
}

Further simplification

As of macOS 10.10 the NSURL class has a

+ (instancetype)URLByResolvingAliasFileAtURL:(NSURL *)url
                                     options:(NSURLBookmarkResolutionOptions)options
                                       error:(NSError * _Nullable *)error;

initializer, which both reads and resolves the bookmark data. The function then simplifies to (now with automatic reference counting):

const char *YourClass::resolveAlias(const char *filepath) const {
    static char resolved[MAXPATHLEN];
    @autoreleasepool {

        NSURL *url = [NSURL fileURLWithFileSystemRepresentation:filepath
                                                    isDirectory:NO
                                                  relativeToURL:nil];
        NSError *error = nil;
        NSURL *resolvedURL = [NSURL URLByResolvingAliasFileAtURL:url
                                                      options:NSURLBookmarkResolutionWithoutUI
                                                        error:&error];
        if (![resolvedURL getFileSystemRepresentation:resolved maxLength:sizeof(resolved)]) {
            NSLog(@"File system path too long for %@", url);
            return nullptr;
        }
    }
    return resolved;
}


Parsing a configuration file in C I've written the following function to parse a configuration file. I did my best to make it simple, but it's still over 30 lines and has too much indentation caused by a series of if statements.

Any suggestions make it easier to read are welcome.

int
parse_config (FILE * conf_file, module_initializer_t init)
{
  char *line; /* Current line */
  char *type; /* Type of current module */
  char *name; /* Name of current module */
  char *envz;
  size_t envz_len;
  size_t lineno = 0;

  line = type = name = envz = NULL;
  while (getline (&line, NULL, conf_file) != EOF)
    {
      lineno++ ;
      if (!is_comment(line))
        if (line[0] == '[')
          if (update_header(line, &type, &name)) {
            if (type)
              if ((*init)(type, name, envz, envz_len)) {
                fprintf("failed to initialize module %s with name %s", type, name);
                return 1;
              }
            clear_envz(&envz, &envz_len);
            }
          else {
            fprint("syntax error on line %d", lineno);
            return 1;
          }
      else
        argz_add(&envz, &envz_len, line);
    }
  if ((*init)(type, name, envz, envz_len)) {
    fprintf("failed to initialize module %s with name %s", type, name);
    return 1;
  }
}

It parses a configuration file with the following syntax:

   #comment and empty lines ignored
   [module-type]{module-name}
   var1=val1
   #comment
   var2=val2
   [another-type]{another-name}
   var3=val3

I would do some re-arranging:
Use the quick exit pattern.

This is basically if it fails the useful criteria exit (or in this case start the next loop). Basically you test for conditions that don't satisfy your working conditions. If the data does not match then you move on.

int
parse_config (FILE * conf_file, module_initializer_t init)
{
  char *line; /* Current line */
  char *type; /* Type of current module */
  char *name; /* Name of current module */
  char *envz;
  size_t envz_len;
  size_t lineno = 0;

  line = type = name = envz = NULL;
  while (getline (&line, NULL, conf_file) != EOF)
  {
      lineno++ ;
      if (is_comment(line))
      {    continue;        // Ignore comments;
      }

      if (line[0] != '[')
      {
          argz_add(&envz, &envz_len, line);
          continue;         // Processes arguments and continue
      }

      if (!update_header(line, &type, &name))
      {
          fprint("syntax error on line %d", lineno);
          return 1;         // Not an argument and not a header
                            // must be an error.
      }

      // Processes a header
      if (type)
      {
          if ((*init)(type, name, envz, envz_len))
          {
            fprintf("failed to initialize module %s with name %s", type, name);
            return 1;
          }
          clear_envz(&envz, &envz_len);
      }
  }

  if ((*init)(type, name, envz, envz_len)) {
    fprintf("failed to initialize module %s with name %s", type, name);
    return 1;
  }

  // You should also return a value here.
  return 0;
}