FizzBuzz using switches I'm learning about control structures now, and I want to see if I'm doing this as cleanly and efficiently as possible. Seems like there should be a less... verbose way of writing the switch but I wouldn't know what it is. Any criticism welcome!

FizzBuzz.java

public class FizzBuzz {

    public static final int fizz = 3;
    public static final int buzz = 5;

    public static void main(String[] args) {
        int status = -1;

        for (int fbNumber = 1; fbNumber <= 100; fbNumber++) {

            if ((fbNumber % fizz == 0) && (fbNumber % buzz == 0)) {
                status = 1;
            } else if ((fbNumber % fizz == 0) && (fbNumber % buzz != 0)) {
                status = 2;
            } else if ((fbNumber % fizz != 0) && (fbNumber % buzz == 0)) {
                status = 3;
            } else {
                status = 4;
            }
            switch (status) {
                case 1:
                    System.out.println("FizzBuzz");
                    break;
                case 2:
                    System.out.println("Fizz");
                    break;
                case 3:
                    System.out.println("Buzz");
                    break;
                case 4:
                    System.out.println(fbNumber);
                    break;
                default:
                    System.out.println("Number could not be evaluated");
            }
        }
    }
}

Constants fizz and buzz should be ALL_CAPS, by convention.

The use of status and the switch is unjustified.  Why complicate things by setting a variable to alter behaviour a few lines later?

public class FizzBuzz {

    public static final int FIZZ = 3;
    public static final int BUZZ = 5;

    public static void main(String[] args) {
        for (int fbNumber = 1; fbNumber <= 100; fbNumber++) {

            if ((fbNumber % FIZZ == 0) && (fbNumber % BUZZ == 0)) {
                System.out.println("FizzBuzz");
            } else if ((fbNumber % FIZZ == 0) && (fbNumber % BUZZ != 0)) {
                System.out.println("Fizz");
            } else if ((fbNumber % FIZZ != 0) && (fbNumber % BUZZ == 0)) {
                System.out.println("Buzz");
            } else {
                System.out.println(fbNumber);
            }
        }
    }
}


Lock-free multi-producer multi-consumer queue I'm looking for some feedback on my lock-free queue, based on Disruptor, mainly for any potential concurrency issues, such as where I need additional fences. It looks correct to me, and I can't seem to able to break it, but I'm only testing it on my x86 machine.

#pragma once

#include <atomic>
#include <cstdint>

template <typename T> class LockFreeMPMCQueue
{
    public:
    explicit LockFreeMPMCQueue( size_t size )
        : m_data( new T[size] ), m_size( size ), m_head_1( 0 ), m_head_2( 0 ), m_tail_1( 0 ), m_tail_2( 0 )
    {
    }

    virtual ~LockFreeMPMCQueue() { delete[] m_data; }

    // non-copyable
    LockFreeMPMCQueue( const LockFreeMPMCQueue<T>& ) = delete;
    LockFreeMPMCQueue( const LockFreeMPMCQueue<T>&& ) = delete;
    LockFreeMPMCQueue<T>& operator=( const LockFreeMPMCQueue<T>& ) = delete;
    LockFreeMPMCQueue<T>& operator=( const LockFreeMPMCQueue<T>&& ) = delete;

    bool try_enqueue( const T& value )
    {
        const std::uint64_t head = m_head_2.load( std::memory_order_relaxed );
        std::uint64_t tail = m_tail_1.load( std::memory_order_relaxed );

        const std::uint64_t count = tail - head;

        // count could be greater than size if between the reading of head, and the reading of tail, both head
        // and tail have been advanced
        if( count >= m_size )
        {
            return false;
        }

        if( !std::atomic_compare_exchange_strong_explicit(
            &m_tail_1, &tail, tail + 1, std::memory_order_relaxed, std::memory_order_relaxed ) )
        {
            return false;
        }

        m_data[tail % m_size] = value;

        while( m_tail_2.load( std::memory_order_relaxed ) != tail )
        {
            std::this_thread::yield();
        }

        // Release - read/write before can't be reordered with writes after
        // Make sure the write of the value to m_data is
        // not reordered past the write to m_tail_2
        std::atomic_thread_fence( std::memory_order_release );
        m_tail_2.store( tail + 1, std::memory_order_relaxed );

        return true;
    }

    bool try_dequeue( T& out )
    {
        const std::uint64_t tail = m_tail_2.load( std::memory_order_relaxed );
        std::uint64_t head = m_head_1.load( std::memory_order_relaxed );

        if( head >= tail )
        {
            return false;
        }

        if( !std::atomic_compare_exchange_strong_explicit(
            &m_head_1, &head, head + 1, std::memory_order_relaxed, std::memory_order_relaxed ) )
        {
            return false;
        }

        out = m_data[head % m_size];

        while( m_head_2.load( std::memory_order_relaxed ) != head )
        {
            std::this_thread::yield();
        }

        // Release - read/write before can't be reordered with writes after
        // Make sure the read of value from m_data is
        // not reordered past the write to m_head_2
        std::atomic_thread_fence( std::memory_order_release );
        m_head_2.store( head + 1, std::memory_order_relaxed );

        return true;
    }

    size_t capacity() const { return m_size; }

    private:
    T* m_data;
    size_t m_size;

    // Make sure each index is on its own cache line
    char pad1[56];
    std::atomic<std::uint64_t> m_head_1;
    char pad2[56];
    std::atomic<std::uint64_t> m_head_2;
    char pad3[56];
    std::atomic<std::uint64_t> m_tail_1;
    char pad4[56];
    std::atomic<std::uint64_t> m_tail_2;
};

Missing barrier
When I looked at your code, I could immediately tell there was something wrong because you use a release barrier without an acquire barrier that pairs with it.
You have this barrier in try_enqueue() between writing to m_data and writing to m_tail_2:

std::atomic_thread_fence( std::memory_order_release );

But you don't have any corresponding acquire barrier in try_dequeue() between reading from m_tail_2 and reading from m_data.  You should put in a barrier like this:
bool try_dequeue( T& out )
{
    // ...

    // Add this barrier which pairs with the other release barrier
    std::atomic_thread_fence( std::memory_order_acquire );

    out = m_data[head % m_size];

    // ...
}

Without this barrier, the cpu could read m_data[head] speculatively before the read of m_tail_2, and you could end up getting an older value instead of the value just written.
Sequence of events that lead to error

Initially, m_data[0] = 0
Cpu 1: try_enqueue(): m_data[0] = 5
Cpu 1: try_enqueue(): fence(release)
Cpu 1: try_enqueue(): m_tail_2 = 1
Cpu 2: try_dequeue(): reads m_tail_2 as 1
Cpu 2: try_dequeue(): exchange(head, head+1) (no effect)
Cpu 2: try_dequeue(): reads m_data[0] as 0

Due to weak memory ordering, on step 7 above, m_data[0] can be read as 0 instead of 5 because there was no acquire barrier between steps 5 and 7.  The strong exchange on step 6 did not perform a barrier because it was called with std::memory_order_relaxed, std::memory_order_relaxed as the memory ordering.
Note that there are 2 possible ways that memory accesses can be "reordered".  The first is that the compiler might shift lines of code around, and therefore, your accesses are actually performed out of order with respect to a program execution timeline.  This is not what is happening in my example above.  The second way is that the memory system may reorder accesses.  In other words, even if two reads occur one after the other in the program execution timeline, the second read may read an earlier value than the first read.  This second case is what causes step 7 above to fail.
Possible missing barrier
Similarly, you might need an acquire barrier in try_enqueue() before your write to m_data, which pairs with the release barrier in try_dequeue().  I say "might" because this one is harder to explain.  The barrier is only needed if the cpu would do a speculative write to m_data before reading m_head_2.  But because m_head_2 is used to determine whether or not to actually write to m_data, there is a data dependency there which would seem to guarantee that m_head_2 is read before writing to m_data.  However, there are processors such as the DEC Alpha which have strange behavior with respect to data dependencies.  So I'm not 100% sure about whether this barrier is needed, and if it is needed, it may only be needed for DEC Alpha and similar processors.


Rolling mean lag function for multiple variables Thanks to @josliber's help (Rolling mean lag function), I was able to speed up a rolling mean function for different groups and rollmean lengths.

Now I'd like to add to this function the ability to loop through different variables and bind everything together.

Minimum reproducible example

library(zoo)
dat <- data.frame(fips = rep(c(1001, 1003), each = 100),
                  x = rnorm(200),
                  x2 = rnorm(200),
                  x3 = rnorm(200))

allFipsRM3 = function(dat, varName, len){
  do.call(rbind, lapply(split(dat, dat$fips), function(x) {
    all.rm <- as.data.frame(sapply(len, function(l) c(rollmean(x[,varName], l), rep(NA, l-1))))
    colnames(all.rm) <- paste0(varName, "_rm", len)
    cbind(data.frame(fips=x$fips[1]), all.rm, data.frame(year=seq_len(nrow(x))-1))
  }))
}

outdat3 <- allFipsRM3(dat, "x", c(1, 2))

       fips      x_rm1      x_rm2 year
1001.1 1001  1.3482892  1.3043620    0
1001.2 1001  1.2604348  0.2990267    1
1001.3 1001 -0.6623813 -0.4243813    2
1001.4 1001 -0.1863812  0.2806624    3
1001.5 1001  0.7477061 -0.5111745    4
1001.6 1001 -1.7700551 -0.8463731    5

Answering my own question here, but certainly open to suggestions.

library(RcppRoll)

# Loop through n = 10
for (i in 1:10){

  # Create custom col labels
  lab1 <- paste0("x_", i)
  lab2 <- paste0("x2_", i)
  lab3 <- paste0("x3_", i)

  # Loop through each fips and calculate rollingmean
  dat <- dat %>%
      group_by(fips) %>%
      mutate(!!lab1 := roll_mean(x, i, align = "left", fill = "NA"),
             !!lab2 := roll_mean(x2, i, align = "left", fill = "NA"),
             !!lab3 := roll_mean(x3, i, align = "left", fill = "NA")) %>%
    ungroup()

  # Progress bar for loop
  print(i)
}

> names(dat)
 [1] "fips"  "x"     "x2"    "x3"    "x_1"   "x2_1"  "x3_1"  "x_2"   "x2_2"  "x3_2"  "x_3"   "x2_3"
[13] "x3_3"  "x_4"   "x2_4"  "x3_4"  "x_5"   "x2_5"  "x3_5"  "x_6"   "x2_6"  "x3_6"  "x_7"   "x2_7"
[25] "x3_7"  "x_8"   "x2_8"  "x3_8"  "x_9"   "x2_9"  "x3_9"  "x_10"  "x2_10" "x3_10"


Algorithm to find bucket and item indices from power of two bins? So this is building off of Algorithm for dividing a number into largest "power of two" buckets?. Here is a slight modification of the answer from there:

let numbers = [1, 2, 3, 4, 5, 6, 7, 30, 31, 32, 33, 20, 25, 36, 50, 100, 201]
numbers.forEach(n => console.log(`${n} =`, split(n).join(', ')))

function split(number) {
  const result = [0, 0, 0, 0, 0, 0]
  let unit = 32
  let index = 5
  while (unit > 0) {
    while (number >= unit) {
      result[index]++
      number -= unit
    }
    index -= 1
    unit >>= 1
  }
  return result
}

The split function creates "power of two" buckets from an overall array of items of length n. That is, it divides a single large array into multiple small arrays. The result array is 6 items long, accounting for buckets of sizes [1, 2, 4, 8, 16, 32]. Each item in the array is how many buckets of that size need to exist.
Given that, the goal is to then take an index i, and return the bucket and bucket index where you will find that corresponding item. So for example, here are some outputs, and here is my attempt at an algorithm:

let numbers = [1, 2, 3, 4, 5, 6, 7, 30, 31, 32, 33, 20, 25, 36, 50, 100, 201]
numbers.forEach(n => {
  const [c, i] = getCollectionIndexAndItemIndex(n, 300)
  console.log(`${n} = ${c}:${i}`)
})

function getCollectionIndexAndItemIndex(i, size) {
  const parts = split(size).reverse() // assume this is memoized or something
  let j = 0
  let last = 0
  let map = [1, 2, 4, 8, 16, 32].reverse()
  let k = 0
  let bucket = 0
  main:
  while (k <= i) {
    let times = parts[j]
    while (times--) {
      let value = map[j]
      last = 0
      while (value--) {
        k++
        if (value > 0) {
          last++
        } else {
          last = 0
          bucket++
        }
        if (k == i) {
          break main
        }
      }
    }
    j++
  }

  return [ bucket, last ]
}

function split(number) {
  const result = [0, 0, 0, 0, 0, 0]
  let unit = 32
  let index = 5
  while (unit > 0) {
    while (number >= unit) {
      result[index]++
      number -= unit
    }
    index -= 1
    unit >>= 1
  }
  return result
}

That outputs this:
1 = 0:1
2 = 0:2
3 = 0:3
4 = 0:4
5 = 0:5
6 = 0:6
7 = 0:7
30 = 0:30
31 = 0:31
32 = 1:0
33 = 1:1
20 = 0:20
25 = 0:25
36 = 1:4
50 = 1:18
100 = 3:4
201 = 6:9

So basically, for index i == 1, we go to the first bucket (bucket 0), second index (i == 1), represented as 1 = 0:1. For the 32nd index i == 32, that is the 33rd item, so we fill up 1 32-item bucket, and spill over 1, so index 0 in the second bucket, represented 32 = 1:0. For index 201, equals 6:9, which you can calculate as ((32 * 6) - 1) + 10 == 192 - 1 + 10 == 201.
The problem is, this algorithm is O(n), it counts k++ for every item up to k == i. I think there might be a way to optimize this so it can do larger jumps (32, 16, 8, 4, 2, 1 jumps), and cut out a lot of the iterations, I'm just not sure how. Can you find a way to maximally optimize this to the fewest number of steps, using only primitive operations and values (i.e. not fancy array.map and such, but just low-level while or for loops, and bitwise operations)? Basically, how can you optimize the getCollectionIndexAndItemIndex operation, and also simplify it so it's easier to follow.
The size parameter is set to 300, but that is the size of the array. But it could be any size, and we would then want to find the corresponding index within that array, jumping to the appropriate bucket and offset.

I think your code is too complex and can be simplified.
Also, you wrote "using only primitive operations and values (i.e. not fancy array.map and such...", yet you use array.forEach.  Where is the "line"?
I hope this is what you are looking for.
Note:

up-to 32 -> one bucket
32 up-to 32*32 -> another bucket
3232 up-to 3232*32 -> another bucket
etc.

Only after done, I got @superb rain's [i >> 5, i & 31]...
Perhaps I'll implement that in the future.
However, my code allows for "bucket-of-buckets".
The heart of my code is the recursive function:
function recBucket(number, size) {
  if(number<size) return [number];
  else {
    var whole=Math.floor(number/size);
    return [recBucket(whole, size), number-whole*size].flat();
  }
};

Here is a snippet that uses and builds on that:

var numbers = [1, 2, 3, 31, 32, 33, 100, 201, 1023, 1024, 5555];

function recBucket(number, size) {
  if(number<size) return [number];
  else {
    var whole=Math.floor(number/size);
    return [recBucket(whole, size), number-whole*size].flat();
  }
};

console.log("as is:");
numbers.forEach(n=>console.log(n+" = "+recBucket(n, 32).join(":")));

function minBuckets(number, size, buckets) {
  var result=recBucket(number, size);
  while(result.length<buckets) result.unshift(0);
  return result;
};

console.log("min 2 buckets:");
numbers.forEach(n=>console.log(n+" = "+minBuckets(n, 32,2).join(":")));

console.log("min 4 buckets:");
numbers.forEach(n=>console.log(n+" = "+minBuckets(n, 32,4).join(":")));
.as-console-wrapper { max-height: 100% !important; top: 0; }


Mocking config data in JavaScript unit tests I'd really like someone to sanity check my approach for unit testing the summarise() function and mocking its dependencies.

Background

Each option has a set of values, which come from the app state
Options also each have a config, which are defined in configs.js (potentially a large list, with 'dynamic' data-like variations)

The tests

I'm testing the summarise function. To isolate my tests, I've mocked the option configs. This means I don't have to couple the test to certain option configs, which allows settings to be changed freely.

My tests work, but I feel it has a few issues:

I can't directly spy on a config object, so I've exported a function getOptionConfig() which I can spy on. I feel like it would be cleaner to avoid an API; and just spy on the object if possible.
I feel it'd be much cleaner to pass the _optionConfigs object to summarise() (to avoid mocks completely). However, this would only pass the same issue to any functions which call summarise().
Because I'm forcing a return value in the spies, I'm not testing the parameter that I'm passing to getOptionConfig(). Is this bad?

configs.js

const _optionConfigs = {
  exampleOption: {
    type: 'red',
  },
  exampleOption2: {
    type: 'blue',
  },
  exampleOption3: {
    type: 'red',
    exclude: true,
  },
  exampleOption4: {
    type: 'red',
  },
  // ... the list goes on...
};

export const getOptionConfig(id) {
  return _optionConfigs[id];
}

summarise.js

import { getOptionConfig } from './config';

/**
 * Summarise an option's values
 * @param {string} optionName - an option name, used for referencing its config data
 * @param {array} optionValues - application state. values of a particular option
 * @returns {string} a summary of the option's values
 */
export const summarise(optionName, optionValues) {
  const optionConfig = getOptionConfig(optionName);
  if (optionConfig.exclude) {
    return '';
  }
  if (optionConfig.type === 'red') {
    return optionValues.map(value => value + ' with a dash of red').join(', ');
  } else (optionConfig.type === 'blue') {
    return optionValues.map(value => value + ' with a bit of blue').join(', ');
  }
}

summarise.test.js

// ENV - jasmine

import * as configs from './configs';

describe('summarise', () => {
  it('ignores when excluded', () => {
    spyOn(configs, 'getOptionConfig').and.returnValue({
      exclude: true,
      type: 'blue',
    });
    const summary = summarise('testOption3');
    expect(summary).toBe('');
  });
  it('summarises blue types', () => {
    spyOn(configs, 'getOptionConfig').and.returnValue({
      type: 'blue',
    });
    const summary = summarise('testOption2', [
      'Value 1',
      'Value 2',
    ]);
    expect(summary).toBe('Value 1 with a bit of blue, Value 2 with a bit of blue');
  });
  it('summarises red types', () => {
    spyOn(configs, 'getOptionConfig').and.returnValue({
      type: 'red',
    });
    const summary = summarise('testOption', [
      'Value 1',
      'Value 2',
    ]);
    expect(summary).toBe('Value 1 with a dash of red, Value 2 with a dash of red');
  });
});

I agree with point number 2, just passing in the config is better (i.e. dependency injection).

You could do it like this:

// Definition of summarize:
export const summarize(config, optionName, optionValues){
    // stuff
}

// In some other file:
const summarizeWithConfig = require('./summarize');
const config = require('./config');

// Create a new function which has it's first argument bound to the config
let summarize = summarizeWithConfig.bind(null, config);

Now summarize can be used like before, and in your tests you can simply test the unbound function.

This kind of pattern should be used in all your code, and will make it a lot easier to test.
However, binding up functions like this becomes a little tedious (with the extra name "summarizeWithConfig", the ugly bind syntax, etc), so I would really recommend
using classes instead, and have the constructors of those classes as injection points.


Multicast Socket Configuration for Pub/Sub I have a set of components that each need to both publish and receive messages using a multicast channel. In production, any one machine could host two or more of these components with 12 or more machines communicating with each other.

My question is... given that components on the same machine and components on separate machines are both sending and received. Have I set this up in the most efficient manner? (Note: messages can periodically be lost with no ill effect)

private readonly static Socket _broadcastSocket;

private readonly static Socket _broadcastListeningSocket;

_broadcastSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);

_broadcastListeningSocket = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);

IPAddress ip = IPAddress.Parse(_configuration.Server.AddressV4);
var ipep = new IPEndPoint(ip, _configuration.Server.Port);

_broadcastSocket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.AddMembership, new MulticastOption(ip));
_broadcastSocket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.MulticastTimeToLive, _configuration.Server.TimeToLive);
_broadcastSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ExclusiveAddressUse, false);
_broadcastSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, 1);
_broadcastSocket.Connect(ipep);

var listenerIpep = new IPEndPoint(IPAddress.Any, _configuration.Server.Port);

_broadcastListeningSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ExclusiveAddressUse, false);
_broadcastListeningSocket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, 1);
_broadcastListeningSocket.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.AddMembership, new MulticastOption(ip, IPAddress.Any));
_broadcastListeningSocket.Bind(listenerIpep);

Messages are received using this following:

_broadcastListeningSocket.Receive(payloadBuffer);

Messages are sent using the following:

if (_broadcastSocket.Connected)
{
    _broadcastSocket.Send(payload);
}

I'm a little new to this so I am assuming I am not setting all the options correctly, feel free to suggest setting of new options.

Unfortunately, I'm unable to comment about the actual options you are setting since

while I've used Sockets in Java, I haven't used them in C#
I'm finding it a bit hard to see what your code is doing because it isn't very expressive

So, while I can do nothing about the first point (someone with more experience will have to help you out), we can certainly remedy the second problem by refactoring your code into a more readable form with less duplication.

Let's start by declaring a new member variable to keep hold of the parsed IP address:

private static IPAddress _ipAddress = IPAddress.Parse(_configuration.Server.AddressV4);

We'll use this field in the methods that result from splitting up your monolithic function into several helpers, leaving us with the following:

_broadcastSocket = CreateBroadcastSocket();
_broadcastListeningSocket = CreateBroadcastListeningSocket();
_broadcastSocket.Connect(GetEndpointOf(_ipAddress));
_broadcastListeningSocket.Bind(GetEndpointOf(IPAddress.Any));

Those methods are defined as the following:

private static Socket CreateBroadcastSocket()
{
    Socket s = CreateSocket(new MulticastOption(_ipAddress));
    s.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.MulticastTimeToLive, _configuration.Server.TimeToLive);
    return s;
}

private static Socket CreateBroadcastListeningSocket()
{
    return CreateSocket(new MulticastOption(_ipAddress, IPAddress.Any));
}

private static Socket CreateSocket(MulticastOption multicastOption)
{
    Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
    s.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ExclusiveAddressUse, false);
    s.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, 1);
    s.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.AddMembership, multicastOption);
    return s;
}

Note that it would have been possible to set the instance variables (_broadcastListeningSocketand _broadcastSocket) directly from the helper methods, instead of returning a new Socket, however I find the above implementation more expressive because we seperate the concerns: The helper methods know about how to create a configured Socket, while the method that calls them knows about what they have to be assigned to.

What to do if you find out you need to set the options differently?

Create a new CreateXXXSocket() method
From that method, call CreateSocket() and
configure the result and pass it back to the caller


Substitution Cipher: Which One? I'm a beginner and I have a question (somehow silly and stupid :) ).  Today I decided to challenge myself and I came around the challenge that wanted me to create a program that ciphers (or encrypts) the message using the substitution cipher method.  I solved the challenge by myself but mine is way different than the solution itself. I just want to know which one is better and why? And also is there anything I missed in my own code?

So here is the code I've written:

#include <iostream>
#include <string>
using namespace std;
int main()
{
    string secretMessage {};
    string alphabet {"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"};
    string key {"XZNLWEBGJHQDYVTKFUOMPCIASRxznlwebgjhqdyvtkfuompciasr"};
    cout << "Enter your secret message: ";
    getline(cin, secretMessage);
    //Encryption
    for(size_t i{0}; i<secretMessage.length(); ++i){
        for(size_t j{0}; j<alphabet.length(); ++j){
            if (secretMessage.at(i) == alphabet.at(j)){
                secretMessage.at(i) = key.at(j);
                break;
            }
        }
    }
    cout << "Encrypting The Message..." << endl;
    cout << "Encrypted Message: " << secretMessage << endl;
    //Decryption
    for(size_t i{0}; i<secretMessage.length(); ++i){
        for(size_t j{0}; j<key.length(); ++j){
            if (secretMessage.at(i) == key.at(j)){
                secretMessage.at(i) = alphabet.at(j);
                break;
            }
        }
    }
    cout << "\nDecrypting The Encryption..." << endl;
    cout << "Decrypted: " << secretMessage << endl;
    return 0;
}

And here is the solution:

#include <iostream>
#include <string>
using namespace std;
int main()
{
    string secretMessage {};
    string alphabet {"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"};
    string key {"XZNLWEBGJHQDYVTKFUOMPCIASRxznlwebgjhqdyvtkfuompciasr"};
    string encryptedMessage {};
    string decryptedMessage {};
    cout << "Enter your secret message: ";
    getline(cin, secretMessage);
    cout << "\nEncrypting Message..." << endl;
    //Encryption
    for(char c:secretMessage){
        size_t position = alphabet.find(c);
        if (position != string::npos){
            char newChar {key.at(position)};
            encryptedMessage += newChar;
        } else{
            encryptedMessage += c;
        }
    }
    cout << "Encrypted Message: " << encryptedMessage << endl;
    //Decryption
    cout << "\nDecrypting Message..." << endl;
    for(char c:encryptedMessage){
        size_t position = key.find(c);
        if (position != string::npos){
            char newChar {alphabet.at(position)};
            decryptedMessage += newChar;
        } else{
            decryptedMessage += c;
        }
    }
    cout << "Decrypted Message: " << decryptedMessage << endl;
    return 0;
}

Note:I have also included the decryption part too

Avoid using namespace std

It's a bad idea to import large namespaces such as std into the global namespace, and it could possibly even change the meaning of your code when a later standard adds new identifiers.  Reserve using namespace for those few namespaces specifically designed to be used that way (notably std::literals).

Use const appropriately

We don't want alphabet or key to be modified, so prevent accidents by declaring them as const std::string.

getline() can fail

If the input stream is closed, getline() will read nothing, and the string will be empty.  That's probably acceptable for this program, but probably worth a comment to show that you've considered this.

Use range-based for

We have an integer loop whose value is used only for indexing into the string:

for(size_t i{0}; i<secretMessage.length(); ++i){
    // use secretMessage[i] here

That's easier to read with modern range-based syntax:

for (auto const c: secretMessage) {
    // use c here

Use standard algorithms

We can include <algorithm> to get some really useful functions from the standard library.  In particular, std::find() could replace the loop that searches in the alphabet.

Reduce duplication

Notice that there are two blocks of code that are almost identical, due to the symmetric nature of this cipher.  The only difference is that the role of alphabet and key are swapped.  This makes it a good candidate to extract as a function:

std::string substitute(const std::string& message,
                       const std::string& from,
                       const std::string& to)

Then we could just call it:

auto encrypted = substitute(secretMessage, alphabet, key);
auto decrypted = substitute(encrypted, key, alphabet);

Avoid linear search

We can be more efficient with our lookup than simple linear search.  For example, we could construct a pair of std::map<char,char> from the alphabet and key.

Clean up the plaintext

I know this is just toy encryption, but for real code, we'd want to overwrite the decrypted text when we finish using it, to reduce the ability for an attacker to access it (e.g. from a core dump).  Remembering <algorithm>, we'd use std::fill() for this.

Omit the return

In main() (and only there), we can omit the return statement and the compiler will automatically return 0 for us.  It's a fairly common convention to omit the return if we don't have any non-zero (i.e. error) returns from main().


Compare 2 List to get a List with active players I have a little problem: I have a list with all the users that have logged in the game and a list that has all the names of the active players and I make a List with all the players as a List<T> (It's a Lists  List).

public void CheckNames()
{
    results.Clear();
    for (int i = 0; i < TC.chatters.viewers.Length; i++)
    {
        if (!TotalPlayerNames.Contains(TC.chatters.viewers[i]))
        {
            TotalPlayerNames.Add(TC.chatters.viewers[i]);
            TotalPlayers.Add(new PlayerReal(TC.chatters.viewers[i]));
        }
        if (TotalPlayerNames.Contains(TC.chatters.viewers[i]))
        {
            results.AddRange(TotalPlayers.FindAll(x => x.displayName == TC.chatters.viewers[i]));
            for (int j = 0; j < results.Count; j++)
            {
                results[j].active = true;
            }

        }
    }

    ActivePlayers = results;
    for (int k = 0; k < TotalPlayers.Count; k++)
    {
        bool check = false;
        for (int i = 0; i < TC.chatters.viewers.Length; i++)
        {

            if (TotalPlayers[k].displayName == TC.chatters.viewers[i] && !check)
            {
                TotalPlayers[k].active = true;
                check = true;
            }
            else if (TotalPlayers[k].displayName != TC.chatters.viewers[i] && !check && ((TC.chatters.viewers.Length-1)==i))
            {
                TotalPlayers[k].active = false;
                check = true;
            }

        }
    }
}

There's no problem if I have 100-300 players playing but I DO have a huge problem if 1000-10000 players are playing... any help on this? This method executes each 20 seconds.

Please use conventional naming of properties. Chatters instead of chatters for example.

Also, why do you use for instead of foreach if you use index only to get an element of collection? Use foreach and your code will be much clearer.

First thing that should come into your mind when you encounter many Contains and !Contains calls which need to determine if an element should be added into a collection is to use HashSet. Use HashSet<string> for TotalPlayerNames and TC.Chatters.Viewers.

Main problem of your code is a lot of repeating loops and duplications like TC.chatters.viewers[i]. Please learn programming basics and particularly C# basics.

All your code can be rewritten like:

var viewers = TC.Chatters.Viewers;

TotalPlayers.AddRange(viewers.Where(v => TotalPlayerNames.Add(v))
                             .Select(v => new PlayerReal(v)));

var activePlayers = new List<PlayerReal>();
foreach (var player in TotalPlayers)
{
    player.Active = viewers.Contains(player.DisplayName);
    if (player.Active)
        activePlayers.Add(player);
}

ActivePlayers = activePlayers;


Check for boolean values in an object and convert them to numbers I have got a an object that has values true and false for certain keys.I need to find which keys has got a boolean value and convert them to 1 and 0 respectively.Please find the code below.I wanted  to check for keys in an object that has got boolean values and then convert them to numbers.Im a noob in Javascript and i got some help from fellow programmers in SO.This works but im sure there is a better way of doing this.

    var obj ={
    "label":'test',
    "tel":123456,
    "settings": {
      "playback": false,
      "attachfile": true,
    },
    "setTime":true,
    "setDay":false,
    "arrSettings" :[
        'Testing',
        {"playback": false,"attachfile": true},
        {"playback": false,"attachfile": true},
        123456789
    ]
}

function changeBooleanToNumber(o){

    for(var key in o){

        if(/^boolean$/i.test(typeof o[key])){
            o[key] = Number(o[key]);
        }

        if (Object.prototype.toString.call(o[key]) === '[object Object]'){

           changeBooleanToNumber(o[key]);

        }
        else if( Object.prototype.toString.call( o[key]) === '[object Array]' ) {

            for(var i=0;i<=o[key].length-1;i++){

                if(Object.prototype.toString.call(o[key][i]) === '[object Object]'){
                     changeBooleanToNumber(o[key]);
                }

            }

        }

   }
}

changeBooleanToNumber(obj);

Before I do anything else I feel compelled to clean up your spacing convert " to ' to consistantly use only one type of strings

var obj = {
    'label': 'test',
    'tel': 123456,
    'settings': {
        'playback': false,
        'attachfile': true,
    },
    'setTime': true,
    'setDay': false,
    'arrSettings': [
        'Testing',
        {'playback': false, 'attachfile': true},
        {'playback': false, 'attachfile': true},
        123456789
    ]
};

function changeBooleanToNumber(o) {
    for (var key in o) {
        if (/^boolean$/i.test(typeof o[key])) {
            o[key] = Number(o[key]);
        }

        if (Object.prototype.toString.call(o[key]) === '[object Object]') {
            changeBooleanToNumber(o[key]);
        } else if (Object.prototype.toString.call(o[key]) === '[object Array]') {
            for (var i = 0; i < o[key].length - 1; i++) {
                if(Object.prototype.toString.call(o[key][i]) === '[object Object]') {
                    changeBooleanToNumber(o[key]);
                }
            }
        }
    }
}

changeBooleanToNumber(obj);

Now:

Use +x instead of Number(x); it is shorter and faster in all cases I've ever bothered to test.
Don't use var inline, javascript doesn't have block scope (only function scope) so it is better to declare all necessary variables at the start of your functions.
Cache Object.prototype.toString to make code more readable.
Use toString instead of a regex test for checking for the bool type
simplify for loop and store the upper bound in a variable instead of computing it every time
Use a hasOwnProperty check on the for in loop.

This leaves me with this:

function changeBooleanToNumber(o) {
    var toString = {}.toString,
        hasOwn = {}.hasOwnProperty,
        key,
        i,
        len;
    for (key in o) {
        if (hasOwn.call(o,key)) {
            if (toString.call(o[key]) === '[object Boolean]') {
                o[key] = +o[key];
            } else if (toString.call(o[key]) === '[object Object]') {
                changeBooleanToNumber(o[key]);
            } else if (toString.call(o[key]) === '[object Array]') {
                len = o[key].length;
                for (i = 0; i < len; i++) {
                    if(toString.call(o[key][i]) === '[object Object]') {
                        changeBooleanToNumber(o[key]);
                    }
                }
            }
        }
    }
}

This resulting code looks to me like it has too many indents. I also notice that it fails on passing in an object that is an array or contains an array of arrays. Reordering some code we can check both of those cases at the root level:

function changeBooleanToNumber(o) {
    var toString = {}.toString,
        hasOwn = {}.hasOwnProperty,
        key,
        len;
    if (toString.call(o) === '[object Object]') {
        for (key in o) {
            if (hasOwn.call(o, key)) {
                if (toString.call(o[key]) === '[object Boolean]') {
                    o[key] = +o[key];
                } else if (toString.call(o[key]) === '[object Object]') {
                    changeBooleanToNumber(o[key]);
                } else if (toString.call(o[key]) === '[object Array]') {
                    changeBooleanToNumber(o[key]);
                }
            }
        }
    } else if (toString.call(o) === '[object Array]') {
        len = o.length;
        for (key = 0; key < len; key++) {
            if (toString.call(o[key]) === '[object Boolean]') {
                o[key] = +o[key];
            } else if (toString.call(o[key]) === '[object Object]') {
                changeBooleanToNumber(o[key]);
            } else if (toString.call(o[key]) === '[object Array]') {
                changeBooleanToNumber(o[key]);
            }
        }
    }
}

However now there is a bunch of duplicate code. This can be reduced by abstracting a function:

function changeBooleanToNumberHelper(o, key) {
    var toString = {}.toString;
    if (toString.call(o[key]) === '[object Boolean]') {
        o[key] = +o[key];
    } else if (toString.call(o[key]) === '[object Object]') {
        changeBooleanToNumber(o[key]);
    } else if (toString.call(o[key]) === '[object Array]') {
        changeBooleanToNumber(o[key]);
    }
}
function changeBooleanToNumber(o) {
    var toString = {}.toString,
        hasOwn = {}.hasOwnProperty,
        key,
        len;
    if (toString.call(o) === '[object Object]') {
        for (key in o) {
            if (hasOwn.call(o, key)) {
                changeBooleanToNumberHelper(o, key);
            }
        }
    } else if (toString.call(o) === '[object Array]') {
        len = o.length;
        for (key = 0; key < len; key++) {
            changeBooleanToNumberHelper(o, key);
        }
    }
}

Here, the if statements look very similar. It turns out we can inline this helper function:

function changeBooleanToNumber(o, key) {
    var toString = {}.toString,
        hasOwn = {}.hasOwnProperty,
        len;
    if (key) {
        if (toString.call(o[key]) === '[object Boolean]') {
            o[key] = +o[key];
            return;
        }
        o = o[key];
    }
    if (toString.call(o) === '[object Object]') {
        for (key in o) {
            if (hasOwn.call(o, key)) {
                changeBooleanToNumber(o, key);
            }
        }
    } else if (toString.call(o) === '[object Array]') {
        len = o.length;
        for (key = 0; key < len; key++) {
            changeBooleanToNumber(o, key);
        }
    }
}

Without going farther I recognize this pattern as a depth first graph traversal with a little additional functionality. I would abstract that a little and see this helper function:

function depthVisit(node, visit, key) {
    var toString = {}.toString,
        hasOwn = {}.hasOwnProperty,
        len;
    if (key) {
        if (visit(node, key) === false) {
            return;
        }
        node = node[key];
    }
    if (toString.call(node) === '[object Object]') {
        for (key in node) {
            if (hasOwn.call(node, key)) {
                depthVisit(node, visit, key);
            }
        }
    } else if (toString.call(node) === '[object Array]') {
        len = node.length;
        for (key = 0; key < len; key++) {
            depthVisit(node, visit, key);
        }
    }
}

function changeBooleanToNumber(o) {
    depthVisit(o, function (parent, key) {
        if ({}.toString.call(parent[key]) === '[object Boolean]') {
            parent[key] = +parent[key];
        }
    });
}

However I know this function has a bug: javascript can have cyclic references (causing infinite recursion). To fix that I need to either limit the traversal to a particular depth or check if I have already visited an object. Here I'll do the latter:

function depthVisit(node, visit) {
    var set = [];
    function seen(node) {
        var i = set.length - 1;
        while (set[i] !== node && i > 0) { i--; }
        set.push(node);
        return i !== -1 && set[i] === node;
    }
    function inner(node, key) {
        var toString = {}.toString,
            hasOwn = {}.hasOwnProperty,
            len;
        if (key) {
            if (visit(node, key) === false) {
                return;
            }
            node = node[key];
        }
        if (seen(node)) {
            return;
        }
        if (toString.call(node) === '[object Object]') {
            for (key in node) {
                if (hasOwn.call(node, key)) {
                    inner(node, key);
                }
            }
        } else if (toString.call(node) === '[object Array]') {
            len = node.length;
            for (key = 0; key < len; key++) {
                inner(node, key);
            }
        }
    }
    inner(node);
}

function changeBooleanToNumber2(o) {
    depthVisit(o, function (parent, key) {
        if ({}.toString.call(parent[key]) === '[object Boolean]') {
            parent[key] = +parent[key];
        }
    });
}

JsFiddle to show it works.

Interestingly this version is also about 20% faster than the original code. (see: http://jsperf.com/graph-traversal)


Toggle Event Delegation NOTE: the Javascript is all I'm concerned about, the HTML/CSS is just DEMO code. One caveat is ARIA states or attributes, if you have input on that, by all means, dive in.

I had the need for and unobtrusive extensible toggle that could be applied to a handful of items: tooltips, accordions, panels, dropdowns, etc.

With the accordions, there is a need for a sidebar that also has accordion-like functionality with sub-links that reveal once clicked, this event triggers the accordion as well, so had to make paired events that triggered the actual accordion, as those sidebar sub-links anchor/smooth scroll into the accordion panels.

We're not utilizing babeljs so wrote this in ES5 compliant JS. Any recommendations to improve or areas that you see could pose an issue in compatibility, performance, maintainability, scalability, etc. feel free to offer advice. I am not a strong JS dev, know enough to get in trouble, so any advice on improvement is welcomed.

var toggleAttribute = function (el, attr) {
  !el.hasAttribute(attr) ?
    el.setAttribute(attr, '') :
  el.removeAttribute(attr);
};

var toggleValue = function (el, attr, on, off) {
  el.setAttribute(
    attr,
    el.getAttribute(attr) === off ? on : off);
};

var eventHandler = function eventHandler(event, node, callback) {

  window.addEventListener(
    event,
    function (e) {
      if (!e.target.matches(node)) return;
      callback.call(this, e);
    },
    false
  );

};

var toggleMethod = function (target, identifier, active, attr, on, off) {

  var d = document;
  target.classList.add(active);

  toggleValue(target, attr, on, off);
  toggleAttribute(target.nextElementSibling, 'hidden')

  if (target.hasAttribute('data-controls')) {

    var pairedTarget = d.querySelector('#' + target.getAttribute('data-controls'));

    toggleValue(pairedTarget, attr, on, off);
    toggleAttribute(pairedTarget.nextElementSibling, 'hidden')

  }

  var selectorList = d.querySelectorAll(identifier);

  //for (var i = 0, len = selectorList.length; i < len; ++i)

  Array.from(selectorList)
    .forEach(function (selector) {

    if (
      selector !== target &&
      selector !== pairedTarget
    ) {

      selector.classList.remove(active);
      selector.setAttribute(attr, off);
      selector.nextElementSibling.hidden = true;

    }

  });

};

eventHandler('click', '.toggle', function (e) {

  toggleMethod(e.target, '.toggle', 'active', 'aria-expanded', 'true', 'false');

});
@charset 'utf-8';
body {
  margin: 0
}

body>div {
  width: 49%;
  display: inline-block;
}

.toggle {
  display:block;
  margin-right: 5px;
  padding: 8px 12px 8px 8px;
}

.toggle:before {
  content: '›';
  margin-right: 8px;
  display: inline-block;
}

.toggle[aria-expanded='true']:before {
  transform: rotate(90deg);
}

      paired

paired a message

      isolated

isolated a message

      <button aria-describedby="message-2" aria-expanded="false" class="toggle" data-controls="toggle-b" id="toggle-a">paired</button>

paired b message

      <button aria-describedby="message-3" aria-expanded="false" class="toggle">isolated</button>

isolated b message

The code looks pretty good. There are only a few suggestions I have about it.

While it doesn't depend on Babeljs, it appears that Array.from() is part of the Ecmascript 2015 (6th edition) Standard1. So if you really wanted to make this code ES-5 compliant, you would need to remove that. For example, this example suggests using .slice.call:

So instead of

Array.from(selectorList)
  .forEach(function (selector) {

You would need to do something like

[].slice.call(selectorList)
    .forEach(function (selector) {

The naming of selectorList is a little misleading, since querySelectorAll returns a NodeList. A more appropriate name would be selectedNodes or something similar. Similarly, selector is misleading, since it is typically passed a DOMNode object.

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#Specifications


Simple Calculator Script I written my own calculator script in javascript! It is a basic one, but works really well. I am just wondering if there is any way to make this script more efficient & clean. On first look, I feel like I did some redundant stuff in it, however I am a beginner and in no right to judge the code on my own.

window.onload = () => {
  const calculator = document.querySelector('form[name = "calculator"]')
  const btns = document.querySelectorAll(`form[name = "calculator"] table tr input[type = "button"]`)
  btns.forEach((button) => {
    if(button.value != 'c' && button.value != '='){
      button.addEventListener('click', () => {
        calculator.display.value += button.value
      })
    } else {
      if (button.value == 'c') {
        button.addEventListener('click', () => {
          calculator.display.value = '';
        })
      } else if (button.value == '=') {
        button.addEventListener('click', () => {
          calculator.display.value = eval(calculator.display.value);
        })
      }
    }
  })
}
<!DOCTYPE html>

      <form name = "calculator">

            <input type = "text" name = "display" id = "display" disabled>

            <input type = "button" name = "one" value = "1">
            <input type = "button" name = "two" value = "2">
            <input type = "button" name = "three" value = "3">
            <input type = "button" name = "plus" value = "+">

            <input type = "button" name = "four" value = "4">
            <input type = "button" name = "five" value = "5">
            <input type = "button" name = "six" value = "6">
            <input type = "button" name = "minus" value = "-">

            <input type = "button" name = "seven" value = "7">
            <input type = "button" name = "eight" value = "8">
            <input type = "button" name = "nine" value = "9">
            <input type = "button" name = "multiplicatio" value = "*">

            <input type = "button" name = "clear" value = "c">
            <input type = "button" name = "0" value = "0">
            <input type = "button" name = "equal" value = "=">
            <input type = "button" name = "division" value = "/">

      </form>

Are there any ways to make this script "better"? Thanks for the help.

Yes, It can be better in many ways just replace all your code with this:

The first thing I did is made the code formatting just a bit better.
then I added a title tag a webpage cannot go on without a title
after that I include the meta tags for the UTF-8 encoding and then the viewport meta tag for responsiveness. I also corrected your opening body tag you wrote it as the closing body tag.

Tip: Use vscode as your code editor and add the prettier extension with it to format your code better.

<!DOCTYPE html>

    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />

    <title>Your Page should have a title</title>

    <form name = "calculator">

        <input type = "text" name = "display" id = "display" disabled>

        <input type = "button" name = "one" value = "1">
        <input type = "button" name = "two" value = "2">
        <input type = "button" name = "three" value = "3">
        <input type = "button" name = "plus" value = "+">

      <input type = "button" name = "four" value = "4">
        <input type = "button" name = "five" value = "5">
        <input type = "button" name = "six" value = "6">
        <input type = "button" name = "minus" value = "-">

        <input type = "button" name = "seven" value = "7">
        <input type = "button" name = "eight" value = "8">
        <input type = "button" name = "nine" value = "9">
        <input type = "button" name = "multiplicatio" value = "*">

        <input type = "button" name = "clear" value = "c">
        <input type = "button" name = "0" value = "0">
        <input type = "button" name = "equal" value = "=">
        <input type = "button" name = "division" value = "/">

    </form>

The Js part

The only thing I did with the Javascript is made the code formatting a lot more readable

window.onload = () => {
  const calculator = document.querySelector('form[name = "calculator"]');
  const btns = document.querySelectorAll(
    `form[name = "calculator"] table tr input[type = "button"]`
  );
  btns.forEach((button) => {
    if (button.value != "c" && button.value != "=") {
      button.addEventListener("click", () => {
        calculator.display.value += button.value;
      });
    } else {
      if (button.value == "c") {
        button.addEventListener("click", () => {
          calculator.display.value = "";
        });
      } else if (button.value == "=") {
        button.addEventListener("click", () => {
          calculator.display.value = eval(calculator.display.value);
        });
      }
    }
  });
};


Simple PIN Locker in Python I am fairly new to Python but I have been improving, and I wanted to try writing a simple program that simulates locker with multiple chambers (4) for storing "objects".
I recently used lockers at a theme park with a similar functionality.

A chamber can be reserved by simply typing a unique PIN. In the real world, the PIN would be a unique ID like a credit card track, a finger print, a barcode, or RFID.

If the PIN is already associated with a chamber, the chamber is released and made available and the user is destroyed.

If the PIN is unique, and there are chambers available (not reserved),
a chamber is reserved for the user under the unique PIN.

I hope the explanation makes sense.
I am looking for some pointers and critique on code organization and flow, best practices, and use of classes, methods, etc.

You can see the code in action here.

class Locker(object):

    def __init__(self, capacity):
        self.chambers = {}

        print 'Locker Initialized.'
        for n, chamber in enumerate(range(0, capacity)):
            self.chambers[n] = Chamber(n)
        print 'Locker Created: ', self

    def find_chamber_by_pin(self, pin):
        for chamber in self.chambers.values():
            if pin == getattr(chamber.user, 'pin', None):
                return chamber
        return None

    def find_available_chamber(self):
        for chamber in self.chambers.values():
            if chamber.occupied is False:
                return chamber
        return None

    def count_available(self):
        counter = 0
        for chamber in self.chambers.values():
            if chamber.occupied is False:
                counter += 1
        return counter

    def __len__(self):
        return len(self.chambers.keys())

    def __repr__(self):
        return '<LOCKER: {} of {} Available>:'.format(self.count_available(), len(self))

class Chamber(object):

    def __init__(self, uid):
        self.uid = uid
        self.occupied = False
        self.user = None
        print 'Chamber Initialized: ', self

    def reserve(self, user):
        self.occupied = True
        self.user = user
        print 'Chamber Reserved: ', self

    def release(self):
        self.occupied = False
        user = self.user
        del user
        self.user = None

        print 'Chamber Released: ', self

    def __repr__(self):
        return '<CHAMBER {}:{}>:{}'.format(self.uid, self.occupied,
                                             self.user)

class User(object):

    def __init__(self, pin):
        self.pin = pin

        print 'User Created: ', self

    def __repr__(self):
        return '<USER PIN: {}>'.format(self.pin)

locker = Locker(4)

while True:

    pin = raw_input('Locker Idle. Enter Unique PIN:')

    chamber = locker.find_chamber_by_pin(pin)

    if chamber:
        chamber.release()

    else:
        chamber = locker.find_available_chamber()
        if chamber:
            user = User(pin)
            chamber.reserve(user)
        else:
            print 'Chambers Full. Wait for relase.'

    print 'New Status: ', locker

Though I've got a lot to say, it's mostly just me pulling out stuff from my Python toybox, which let you write what you want, rather than how to get it. In general, this is a really clean module and there's nothing utterly heinous going on in there.

def __init__(self, capacity):
    self.chambers = {}

    print 'Locker Initialized.'
    for n, chamber in enumerate(range(0, capacity)):
        self.chambers[n] = Chamber(n)
    print 'Locker Created: ', self

For starters, you don't need to use a dict for this, a list will do just fine. You're giving yourself a constant number of chambers, nothing new ever enters, and nothing ever gets deleted. You may have done this because you've got a C or Java background, know about linked lists, and are a bit worried about access times. In Python, a "list" is actually an array under the hood, so you actually gain by using one.

The range function by default goes from 0 to n-1, so adding a 0 parameter is needless here.

Enumerate will zip together an enumerable with incrementing integers. Range returns a list of incrementing integers, so calling enumerate(range(0,n)) gives you back [(0,0), (1,1), (2,2), ..., (n-1,n-1)] (effectively, it's a little subtler than that). I can't really think of a use case for this, and you don't actually use the second part of each tuple anyway, so you can strip that out.

Finally, comprehensions are a really useful way of generating lists more cleanly. Rather than having a loop that keeps appending things, describing how a result is achieved, you instead write an expression that states what the result is, which makes your intention a little clearer. I'd replace your __init__ with

def __init__(self, capacity):
    print 'Locker Initialized.'
    self.chambers = [Chamber(n) for n in range(capacity)]
    print 'Locker Created: ', self

def find_chamber_by_pin(self, pin):
    for chamber in self.chambers.values():
        if pin == getattr(chamber.user, 'pin', None):
            return chamber
    return None

getattr is awesome, but when you use it with a constant string you don't gain a great deal. You know that chamber.user will always be either an instance of User or None, and in Python None is falsey, so you can replace your condition with if chamber.user and pin == chamber.user.pin:

I'd go a little further than this, though. Your loop is over the list of occupied chambers, so let's call it that...

def occupied_chambers(self):
    return (chamber for chamber in self.chambers if self.chamber.occupied)

def find_chamber_by_pin(self, pin):
    for chamber in self.occupied_chambers():
        if chamber.user.pin == pin:
            return chamber
    return None

If you wanted to crush your number of lines down, you could also exploit next, but the resulting function starts to look a little chaotic. It's there if you like it:

def find_chamber_by_pin(self, pin):
    return next((chamber for chamber in self.occupied_chambers()
                     if chamber.user.pin == pin), None)

def find_available_chamber(self):
    for chamber in self.chambers.values():
        if chamber.occupied is False:
            return chamber
    return None

def count_available(self):
    counter = 0
    for chamber in self.chambers.values():
        if chamber.occupied is False:
            counter += 1
    return counter

There aren't all that many situations where you gain from comparing stuff to True and False. Rather than chamber.occupied is False, not chamber.occupied is preferred (but see later on).

These functions both iterate over the same thing, so abstract it out:

def available_chambers(self):
    return [chamber for chamber in self.chambers if not chamber.occupied]

def find_available_chamber(self):
    try:
        return self.available_chambers()[0]
    except IndexError:
        return None

def count_available(self):
    return len(self.available_chambers())

class Chamber(object):
    def __init__(self, uid):
        self.uid = uid
        self.occupied = False
        self.user = None
        print 'Chamber Initialized: ', self

    def reserve(self, user):
        self.occupied = True
        self.user = user
        print 'Chamber Reserved: ', self

    def release(self):
        self.occupied = False
        user = self.user
        del user
        self.user = None
        print 'Chamber Released: ', self

First off, your release method deletes its user. This doesn't actually achieve anything. You can just set the user to None and the garbage collector will sort it out for you when it notices your object no longer references it.

Second, it seems like you want to keep an invariant that occupied is True if user is set and occupied is false if user is unset. For something like this, I'd use the property decorator which, among other things, lets you create properties that only exist based on other properties of your object. Half the time though we're not interested in whether or not the chamber is occupied, we want to know that it's available, so let's add that in, too

@property
def occupied(self):
    return bool(self.user)

@property
def available(self):
    return not self.occupied

Now we don't need to keep updating that occupied property any more! You can also go back and change your Locker.available_chambers() method to use this new property.

Putting it all together, I refactored your code into:

class Locker(object):

    def __init__(self, capacity):
        print 'Locker Initialized.'
        self.chambers = [Chamber(n) for n in range(capacity)]
        print 'Locker Created: ', self

    @property
    def occupied_chambers(self):
        return [chamber for chamber in self.chambers if chamber.occupied]

    @property
    def available_chambers(self):
        return [chamber for chamber in self.chambers if chamber.available]

    def find_chamber_by_pin(self, pin):
        return next((chamber for chamber in self.occupied_chambers
                     if chamber.user.pin == pin), None)

    def find_available_chamber(self):
        try:
            return self.available_chambers[0]
        except IndexError:
            return None

    def count_available(self):
        return len(self.available_chambers)

    def __len__(self):
        return len(self.chambers)

    def __repr__(self):
        return '<LOCKER: {} of {} Available>:'.format(self.count_available(), len(self))

class Chamber(object):
    def __init__(self, uid):
        self.uid = uid
        self.user = None
        print 'Chamber Initialized: ', self

    @property
    def occupied(self):
        return bool(self.user)

    @property
    def available(self):
        return not self.occupied

    def reserve(self, user):
        self.user = user
        print 'Chamber Reserved: ', self

    def release(self):
        self.user = None
        print 'Chamber Released: ', self

    def __repr__(self):
        return '<CHAMBER {}:{}>:{}'.format(self.uid, self.occupied,
                                             self.user)

class User(object):
    def __init__(self, pin):
        self.pin = pin
        print 'User Created: ', self
    def __repr__(self):
        return '<USER PIN: {}>'.format(self.pin)

locker = Locker(4)

while True:

    pin = raw_input('Locker Idle. Enter Unique PIN:')

    chamber = locker.find_chamber_by_pin(pin)

    if chamber:
        chamber.release()

    else:
        chamber = locker.find_available_chamber()
        if chamber:
            user = User(pin)
            chamber.reserve(user)
        else:
            print 'Chambers Full. Wait for relase.'

    print 'New Status: ', locker

You asked about the advantages of using @property, and when it's appropriate to use it. I've not been able to dig out any "official" style guides on it, so what you're about to read is basically just my opinion. I should also point out that I'm a Ruby developer, too, so a lot of its idioms tend to bleed into my Python. Most notably, in Ruby you don't generally care whether you're fetching a property or calling a 0-parameter method.

Functionally, there's not really much difference between a @property-decorated function and a regular one that just takes no arguments. The difference is in the "feel" of your interface to anyone using it. If you're asking an object about its state, then you probably want a @property decorator. If you're asking it to do something, then you want a method. Getting a property should be fairly quick, and should (almost) never change the state of the object.

Also notable is that Python is not Java, so methods like def get_foo(self): return self._foo are never appropriate. If a caller might want to do something with foo, then instead consider writing a method that does it for them. Don't expect client code to repeatedly call getters and setters to mutate your object.


How can this code for applying filters to data by date and size be improved? I am going to undergo a code review shortly at work. I feel that my code is too verbose, could log more sensibly, and I feel that my if/else statements are somewhat duplicative/repetitive and can be done better. But I can't seem to be able to put my finger on what I am missing. How would you clean this up such that the code isn't verbose, logs sensible stuff and doesn't look kiddish. I would also like to make it simpler, in case additional filters need to be added in future.

FYI - getDetails method used in the following code, merely contains code for constructing a customer specific "Details" object that contains just 2 things, the name of the customer and a corresponding schedule for invocation.

    /* Filters out invokes that havent reached a size threshold.*/
    private boolean filterBySize(CustomerUnit Customer) {
        CustomerData customerData = new CustomerData(Customer);
        if (customerData.getUsage() > USAGE_THRESHOLD) {
            LOGGER.info("Customer's data usage of " + customerData.getUsage()
                    + "is higher than threshold of " + USAGE_THRESHOLD + " percent for " + Customer.getId());
            return true;
        } else {
            LOGGER.info("Customer's data usage of " + customerData.getUsage()
                    + "is lower than threshold of " + USAGE_THRESHOLD + " percent for " + Customer.getId());
            return false;
        }
    }

    /* Filters out invokes that aren't slated for today.*/
    private boolean filterByDate(CustomerUnit Customer) {
        if (getDetails(Customer).getDOW().equalsIgnoreCase(LocalDate.now().getDayOfWeek().name())) {
            LOGGER.info("Customer : " + getDetails(Customer).getName() + " is invoked to run import today");
            return true;
        } else {
            LOGGER.info("Customer : " + getDetails(Customer).getName() + " isn't invoked to run import today, ");
            return false;
        }
    }

    /* Applies aforesaid filters */
    private void invokeRequestByCustomer(CustomerUnit Customer) throws InterruptedException, UnexpectedDateOrderException {
        if (filterByDate(Customer) && filterBySize(Customer)) {
            invoker.invoke(getDetails(Customer));
            LOGGER.info("Scheduled Customer : " + getDetails(Customer).getName() + " to run import operations at" + getDetails(Customer).getDOW());
        } else {
            LOGGER.info("Customer : " + getDetails(Customer).getName() + " was filtered, no invoke will be executed for it");
        }
    }

As of now, I have thought about using an IfThenElse function, somewhat similar to the following:
IfThenElse example

The comments seem redundant, I'm assuming they're just for this review, if not, I would remove them.  The basically just duplicate what the code says.

Your function names seem misleading.  filterBySize seems to be filtering by data usage.  filterByDate seems to be filtering by the day of the week.  It would be better if they said what they did.

Your filtration functions contain two branches, both of which return.  Generally you  would only wrap the if side.  Everything that doesn't trigger the if would be the else...

CustomerData customerData = new CustomerData(Customer);
if (customerData.getUsage() > USAGE_THRESHOLD) {
    return true;
}
// The else is implicit... so just noise
return false;

For me, there's an awful lot of logging across these methods.  Maybe that's what's required by your processes, but it seems excessive.  We don't have the context for how the code is going to be called, however I would expect that 'USAGE_THRESHOLD' is the same for every invocation, do you really need to log it for each custom to say if they are above/below it?  The current day of the week is the same for each invocation, again do you really need to output it for every customer that's selected?   Each of your filters logs the specific reason that the customer has been rejected  do you need to log the generic message at the top level saying they are being skipped.  If you're looking for reuse, then maybe your filterByDate method shouldn't mention the import, that way it could be reused to filter other things (export perhaps) in the future...

As an aside, I'm not sure about this:

CustomerData customerData = new CustomerData(Customer);

It makes me wonder what the constructor for CustomerData is doing... If the information needed for the filtration isn't in the Customer, then is it reading that extra data in the the constructor for CustomerData?  That seems wrong...

Another small issue is naming.... variables should start with lower case in java.... CustomerUnit Customer should be CustomerUnit customer, you can see it's confusing the code preview...

It would also be interesting to see how invokeRequestByCustomer is being called... It looks a lot like it could fit into a stream something like:

allCustomers.stream()
    .filter(CustomerFilters::byUsage)
    .filter(CustomerFilters::byDay)
    .foreach(c->invoke(c));


Recursive find function in JavaScript / jQuery I wrote the following code as a way of plucking data from a n-depth JavaScript object.
It works a charm and even appends the parents of the the found item.

I was just wondering if you guys had any ideas on making it better.

//#region function recursiveFind
function recursiveFind(data, key, childKey, match, result) {

    ///<summary>Returns JS obj and its parents based on a data set, key and match.</summary>
    ///<param name="data" type="Object" optional="false">the data object you want to find within</param>
    ///<param name="key" type="String" optional="false">the key for the data you want to match against eg. 'id' will look for data.id</param>
    ///<param name="childKey" type="String" optional="false">the data object you want to find within</param>
    ///<param name="match" type="Any" optional="false">the value you wish to search for</param>
    ///<returns type="Object">returns the found result</returns>

    var parents = [];
    var depth = 0;
    var recurse = function (data, key, childKey, match, result) {
        // Build Parents
        depth++;

        // Check this Object
        if (data[key] === match) {
            result = data;
        }
        else if (depth === 1) {
            parents.push(data);

        }

        // If not found check children
        if (result === undefined) {
            if (data[childKey]) {
                $.each(data[childKey], function (k, v) {
                    if (v[key] === match) {
                        result = v;
                        return false;
                    } else {
                        result = recurse(v, key, childKey, match, result);
                        if (result) {
                            parents.push(v);
                            return false;
                        }
                    }
                });
            }
        }

        // Set parents object into the result
        if (result) {
            result.parents = $.extend([], parents);
        }

        // Clean up parents object
        depth--;
        if (depth === 0) {
            parents.length = 0;
        }
        return result;
    };
    return recurse(data, key, childKey, match, result);
}
//#endregion

Overal

I think the code looks too busy just to find a key/value match in an object tree-structure, it took a while to figure out which part(s) bothered me.

Doing it twice

You are doing the matching twice, once with (data[key] === match) and once with (v[key] === match), it would be cleaner to leave the second check to recursion.

Function in a function

I am assuming you use a function in a function to keep track of depth and parents, which isn't worth it. You can do this in a single function with an optional parameter.

Naming

match -> matchValue because match sounds like a boolean to me
childKey - childrenKey because the key really points to an array
result.parents -> avoid this, what if the object already has a property called parents ?

Counter proposal

If you think about it, result can be considered the parent of key, so I would build a function that returns all parents in an array, if you wish you can then have recursiveFind call that.

function findParents( data, key, matchValue, childrenKey, parents )
{
  var i , children , childrenCount;
  /* Prevent the need for an inner function by re-passing parents */
  parents = parents || [];
  parents.push( data )
  /* Return immediately if we find a match */
  if( data[key] === matchValue )
    return parents.reverse();
  /* Are there children to inspect ? */
  if( !data[childrenKey] || !data[childrenKey].length )
    return false;
  children = data[childrenKey];
  childrenCount = children.length;
  /* See if any children have it ? */
  for( i = 0 ; i < childrenCount ; i++ )
  {
    if( parents = findParents( children[i] , key , matchValue , childrenKey , parents.slice() ) )
      return parents;
  }
  /* Fail.. */
  return false;
}

Then recursiveFind becomes

function recursiveFind(data, key, childrenKey, match )
{
  var parents = findParents( data , key, match ),
      result = parents.shift()
  result.parents = parents;
  return result;
}


how to make pagination more efficient with large arrays and db support i have a pagination library in php. i want to know:

how can i make it more efficient when passing large arrays into the constructor
how can i add db support

paginator.php

public function __construct($options = [])
    {
        $this->items = $options['data'];
        $this->limit = $options['limit'] ?? 10;
        $this->currentPage = $options['currentPage'] ?? 1;
        //$this->urlPattern = $options['urlPattern'];
        $this->updateNumPages();
    }

    public function paginate(int $page = 1): PaginatorInterface
    {
        if ($page <= 0 || $this->limit <= 0) {
            throw new \LogicException("Invalid parameters.");
        }
        $offset = ($page - 1) * $this->limit;
        $pagination = new DefaultPaginator();
        $items = new Collection($this->items, $offset, $this->limit);
        if ($this->items instanceof \ArrayObject || is_array($this->items)) {
            $data = new ArrayData();
            $data->fetchItems($items);
        } else {
            throw new \RuntimeException("Data type not supported for pagination.");
        }
        $pagination->setCurrentPageNumber($page);
        $pagination->setNumberOfPages((int) ceil($items->getCount() / $items->getLimit()));
        $pagination->setItems($items->getItems());
        $pagination->setTotal($items->getCount());
        $pagination->setTotalOnCurrentPage(count($items->getItems()));
        $pagination->setTotalPerPage($this->limit);
        return $pagination;
    }

my repository is here https://github.com/shorif2000/pagination

here is where i am slicing the array.

class ArrayData implements DataInterface
{

    public function fetchItems(Collection &$items): void
    {
        if (is_array($items->getCollection())) {
            $items->setItems(array_slice(
                $items->getCollection(),
                $items->getOffset(),
                $items->getLimit()
            ));
            $items->setCount(count($items->getCollection()));
        } elseif ($items->getCollection() instanceof ArrayObject) {

            $collection = $items->getCollection();
            $items->setItems(new ArrayObject(array_slice(
                $collection->getArrayCopy(),
                $items->getOffset(),
                $items->getLimit()
            )));
            $items->setCount($collection->count());
        }
    }
}

UPDATE

I created this class to add support for db or array/ArrayObject

class Pagination
{
    public function __construct($options = [], $mode = 'Default')
    {
        eval('$this = Pagination::factory($options, $mode);');
    }

    public static function &factory($options, $mode)
    {
        $classname = ($mode == 'Default') ? 'Pagination\\Paginator' : 'Pagination\\DbPage' ;
        // If the class exists, return a new instance of it.
        if (class_exists($classname)) {
            $pagination = new $classname($options);
            return $pagination;
        }

        $null = null;
        return $null;
    }
}

UPDATE

with the answer to test i have the following

$pageNumber = 1;
        $itemsPerPage = 10;
        $input = range(0, 100);
        $input = array_slice($input, 0, 10);
        $total = count($input);
        print_r((new PaginatorFactory(new ArrayPageProvider($input)))->createPaginator($pageNumber, $itemsPerPage));

1) Simply don't pass large arrays, use some abstraction.

2) Once you add the abstraction, you can provide db support.

The following abstraction might show how to do it:

EDIT: ok so i have added some more to point at the core of your problems:

The paginator doesnt really need the setters to be part of its interface.

Also see I use Iterator to represent the page items, instead of array. The array will only be used in specific implementation. You should avoid the array type in the abstraction.

interface PaginatorInterface
{
    public function getItems();

    public function getCurrentPageNumber(): int;

    public function getNumberOfPages(): int;

    public function getTotal(): int;

    public function getTotalOnCurrentPage(): int;

    public function getTotalPerPage(): int;
}

In fact, the class implementing that interface doesnt really need those setters too. It's actualy not even desired.

Also the paginator can do simple computations. You dont have to precompute and assign static values which actualy opens space for corrupted state.

class Paginator implements PaginatorInterface
{
    private $pageNumber;
    private $itemsPerPage;
    private $items;
    private $total;

    public function __construct(int $pageNumber, int $itemsPerPage, \Iterator $pageItems, int $total)
    {

        if ($pageNumber < 1) {
            throw new \InvalidArgumentException();
        }
        if ($itemsPerPage < 1) {
            throw new \InvalidArgumentException();
        }
        $this->pageNumber = $pageNumber;
        $this->itemsPerPage = $itemsPerPage;
        $this->items = $pageItems;
        $this->total = $total;
    }

    public function getItems(): \Iterator
    {
        return $this->items;
    }

    public function getCurrentPageNumber(): int
    {
        return $this->pageNumber;
    }

    public function getNumberOfPages(): int
    {
        return \ceil($this->getTotal() / $this->itemsPerPage);
    }

    public function getTotal(): int
    {
        return $this->total;
    }

    public function getTotalOnCurrentPage(): int
    {
        if ($this->items instanceof \Countable) {
            return \count($this->items);
        }
        return \iterator_count($this->items);
    }

    public function getTotalPerPage(): int
    {
        return $this->itemsPerPage;
    }
}

Here I define the page provider (extended by getTotalCount method since my edit):

interface PageProviderInterface
{
    public function getTotalCount(): int;
    public function getPage(int $offset, int $limit): \Iterator;
}

You then use the page provider within a paginator factory (that is your pagination interface but it actualy is paginator factory so i named it that way).

interface PaginatorFactoryInterface
{
    public function createPaginator(int $page, int $pageSize): PaginatorInterface;
}

class PaginatorFactory implements PaginatorFactoryInterface
{
    /** @var PageProviderInterface */
    private $pageProvider;

    public function __construct(PageProviderInterface $pageProvider)
    {
        $this->pageProvider = $pageProvider;
    }

    public function createPaginator(int $page, int $pageSize): PaginatorInterface
    {
        $total = $this->pageProvider->getTotalCount();
        $offset = ($page - 1) * $pageSize;
        if ($offset >= $total) {
            $items = new \EmptyIterator();
        } else {
            $items = $this->pageProvider->getPage($offset, $pageSize);
        }
        return new Paginator($page, $pageSize, $items, $total);
    }
}

And lastly some implementation of the page provider.

The array one which provider slices of an array that resides in memory:

class ArrayPageProvider implements PageProviderInterface
{
    private $items;

    public function __construct(array $items) {

        $this->items = $items;
    }

    public function getTotalCount(): int
    {
        return \count($this->items);
    }

    public function getPage(int $offset, int $limit): \Iterator
    {
        return new \ArrayIterator(\array_slice($this->items, $offset, $limit));
    }
}

And the silly PDO implementation, which would need some improvement to be actualy usable, but you should get the point:

class DbPageProvider implements PageProviderInterface
{
    private $pdo;
    private $table;

    public function __construct(\PDO $pdo, string $table) {
        $this->pdo = $pdo;
        $this->table = $table;
    }

    public function getTotalCount(): int
    {
        $statement = $this->pdo->prepare('select count(*) from ' . $this->table);
        return $statement->fetchColumn();
    }

    public function getPage(int $offset, int $limit): \Iterator
    {
        $statement = $this->pdo->prepare('select * from ' . $this->table . ' limit ' . $offset . ',' . $limit);
        $result = $statement->execute();
        $i = 0;
        while ($row = $result->fetch()) {
            yield $i++ => $row;
        }
    }

}

And that should actualy be all you need. No need for some crazy collection outsourcing or too much of new new new in one method. Keep it simple and remember the naming is really important for whoever wants to understand the code.

Oh and one more, you hade a getViewData() method there, but that should be done by someone else:

function getPaginatorViewData(PaginatorInterface $paginator) {
  return [
    'elements' => $paginator->getItems(),
    // ...

  ];
}

This could be a global function, or it could be static method of its own class, or could even be elevated to an interface level. But actualy i dont think it belong to the lib, it may be part of some framework bundle, or something like that, but not the lib itself. The moment it becomes shapeless array, it becomes hard to work with...

And here is a simple example of usage:

// in DI container
$pageProvider = new DbPageProvider($pdo, 'users');
$usersPaginatorFactory = new PaginatorFactory($pageProvider);

// in lets say /users/list constroller we have the paginator factory injected
// $pageNumber is coming from request
$paginator = $this->usersPaginatorFactory->createPaginator($pageNumber, 20);

// maybe you wanna pass this to laravel view?
return view('users.list', ['paginator' => $paginator]);
// or maybe if you defined the getPaginatorViewData() function
return view('users.list', getPaginatorViewData($paginator));

PS: try to avoid constructor with $options array. It really is better to name all the parameters separately, even omit default value, let the user or a factory (if there is one) decide. Default values usualy just complicate things even if it seems the opposite at first glance...

oh and PS2: although this should have been a big warning on the beginning.
Dont use eval(), avoid references, definitely dont return references, avoid instantiation of classes by string containing class name. The Pagination::factory is completly wrong. Immediately get rid of that file and never write anything like it again!!!


R Function to Generate Predictions from Ratings and Save Results I am trying to generate predictions using the function below in combination with sapply. My actual datasets are very large, I am attempting to make 1.5 million predictions with this function and it is currently taking about 10 seconds per prediction which is... prohibitive to say the least.

mean_rating <- function(df){
  #mean_rating <- function(query_user,query_movie){
  user<-df$user
  movie<-df$movie
  u_row<-which(U_lookup == user)[1]
  m_row<-which(M_lookup==movie)

  knn_match<- knn_txt[u_row,]

  knn_match<-as.numeric(unlist(knn_match))

  dfm_mov<- dfm[,m_row]

  dfm_test<- dfm_mov[knn_match]

  c<-mean(dfm_test[dfm_test!=0])
  c1<-mean(dfm_mov[dfm_mov!=0])
  ifelse(c!="NaN",c,c1)
}
test<-sapply(1:nrow(probe),
             function(x) mean_rating(probe[x,]))

The inputs to this function include the following.
Sparse matrix dfm

library(Matrix)
    dput(dfm)
new("dgTMatrix"
    , i = c(0L, 1L, 2L, 4L, 5L, 6L, 8L, 0L, 1L, 2L, 3L, 4L, 6L, 7L, 8L,
0L, 2L, 3L, 6L, 7L, 8L, 1L, 2L, 4L, 5L, 6L, 7L, 8L, 9L, 0L, 1L,
3L, 4L, 5L, 6L, 7L, 8L, 9L, 0L, 1L, 2L, 3L, 4L, 5L, 6L, 7L, 8L,
9L, 0L, 1L, 3L, 4L, 6L, 7L, 8L, 9L, 0L, 2L, 3L, 5L, 6L, 7L, 9L,
0L, 1L, 2L, 3L, 4L, 5L, 6L, 8L, 9L, 0L, 1L, 2L, 3L, 4L, 5L, 6L,
7L, 9L)
    , j = c(0L, 0L, 0L, 0L, 0L, 0L, 0L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L,
2L, 2L, 2L, 2L, 2L, 2L, 3L, 3L, 3L, 3L, 3L, 3L, 3L, 3L, 4L, 4L,
4L, 4L, 4L, 4L, 4L, 4L, 4L, 5L, 5L, 5L, 5L, 5L, 5L, 5L, 5L, 5L,
5L, 6L, 6L, 6L, 6L, 6L, 6L, 6L, 6L, 7L, 7L, 7L, 7L, 7L, 7L, 7L,
8L, 8L, 8L, 8L, 8L, 8L, 8L, 8L, 8L, 9L, 9L, 9L, 9L, 9L, 9L, 9L,
9L, 9L)
    , Dim = c(10L, 10L)
    , Dimnames = list(NULL, NULL)
    , x = c(4, 3, 1, 2, 3, 1, 2, 1, 3, 3, 2, 3, 3, 3, 4, 2, 1, 2, 3, 2,
1, 4, 1, 2, 2, 3, 2, 3, 4, 1, 4, 1, 3, 4, 3, 2, 2, 2, 4, 1, 2,
2, 1, 2, 3, 1, 1, 1, 4, 1, 1, 2, 1, 1, 1, 4, 3, 3, 2, 1, 2, 2,
1, 1, 3, 3, 4, 1, 2, 4, 2, 4, 1, 2, 2, 3, 4, 2, 1, 2, 4)
    , factors = list()
)

My probe dataset, these are the user movie combinations I am trying to predict for.

dput(probe)
structure(list(X = c(1145185L, 951920L, 1137277L, 180365L, 353195L
), movie = c(1L, 100L, 10000L, 10002L, 10004L), user = c(10L,
1000004L, 1000033L, 1000035L, 1000053L), Rating = c(4L, 4L, 3L,
5L, 4L)), .Names = c("X", "movie", "user", "Rating"), row.names = c("1145185",
"951920", "1137277", "180365", "353195"), class = "data.frame")

My U_lookup table, this is where I convert from the actual id of a user to the row number of the matrix they are in.

dput(U_lookup[1:10,])
c(10L, 100000L, 1000004L, 1000027L, 1000033L, 1000035L, 1000038L,
1000051L, 1000053L, 1000057L)

My M_lookup table, this is where I convert from the actual id of a movie to the row number of the matrix they are in.

dput(M_lookup[1:10,])
c(1L, 10L, 100L, 1000L, 10000L, 10001L, 10002L, 10003L, 10004L,
10005L)

knn_txt is where I'm storing all the users nearest neighbors. Dfm and knn_text would have the same user in every row. I.e., row 1 in dfm would equate to the same user that is in row 1 of knn_text.

dput(knn_txt)
structure(list(V1 = c(1, 2, 1, 7, 5, 3, 2, 9, 1, 3), V2 = c(7,
9, 5, 5, 3, 4, 2, 7, 4, 9), V3 = c(2, 9, 6, 3, 8, 9, 4, 7, 1,
6)), .Names = c("V1", "V2", "V3"), row.names = c(NA, -10L), class = "data.frame")

Edit: As I have done some more testing myself it does seem like a lot of the time savings could come from more efficiently subsetting my large sparse matrix, dfm. I am planning on trying some different formats of the matrix, in addition to maybe looking at looping rather than using sapply (just because then I could say for user i, if user i=i-1 then I don't need to re-subset the matrix).

So after a lot of testing I found something that significantly reduces calculation time. Converting the format of my dfm to a different sparse format saves significant time when subsetting my main matrix (dfm).

dfm<-as(dfm,"dgCMatrix")

This took my calculation time from approximately 11-12 seconds per line, to .2 seconds per line. As I understand it, this is because dgCMatrix format is optimized for subsetting by columns/column operations which is the first subsetting operation in my function. If you were wanting to subset by rows the optimal format would be RsparseMatrix, optimized for rows.

dfm<-as(dfm,"RsparseMatrix")


Random number generation seeding in C++ I wrote a small function to return a random in a given range:

int random_in_range(int min, int max) {
    std::random_device rd;
    std::mt19937 rng(rd());
    std::uniform_int_distribution<int> uni(min, max);
    return uni(rng);
}

But I read somewhere that you should only seed a random number generator once leading me to believe that the function should really be:

std::random_device rd;
std::mt19937 rng(rd());
int random_in_range(int min, int max) {
    std::uniform_int_distribution<int> uni(min, max);
    return uni(rng);
}

I later tested both to see if one was clearly better than the other (in terms of randomness) and got results which do not make things any clearer.

First example result with 10 runs, making a decision of 1 or 0:

for (int i = 0; i < 10; i++) {
    cout << first_example(0, 1);
}
>0100100001

The second example result with 10 runs, making a decision of 1 or 0:

 for (int i = 0; i < 10; i++) {
    cout << second_example(0, 1);
}
>1011000110

The two results don't seem too strange leading me to be confused about how I should initialize random number generators.
Basically, what I am asking is: which of these two example (or something else if both are wrong) would be used in order to guarantee the lowest amount of bias?

Here's how Bjarne Stroustrup did it:

// random number generator from Stroustrup:
// http://www.stroustrup.com/C++11FAQ.html#std-random
int rand_int(int low, int high)
{
    static std::default_random_engine re {};
    using Dist = std::uniform_int_distribution<int>;
    static Dist uid {};
    return uid(re, Dist::param_type{low,high});
}

The principal difference is that the random engine re is static so there is only one initialization (and therefore seed).

Also note that a sample of 10 runs is too short to conclude much. Testing random number generators (RNGs) or psuedo-random number generators (PRNGs) is quite complex.  See http://csrc.nist.gov/groups/ST/toolkit/rng/index.html for a thorough explanation of the purpose, theory and actual source-code for tools to do a good job of testing PRNGs.


Special summation of integer array elements I looked at this question and thought that I could find a much simpler way of doing this, here is the criteria that was given:

  My current task is to find a score from an array where the highest/lowest scores have been taken away, and if the highest/lowest occur more than once (ONLY if they occur more than once), one of them can be added:

  E.g. int[] scores = [4, 8, 6, 4, 8, 5]

  therefore the final addition will be \$\sum{4,8,6, 5} = 23 \$.

  Another condition of the task is that LINQ cannot be used, as well as any of the System.Array methods

Here is what I came up with: (Link to Answer)

static void Main(string[] args)
{
    // take an array and sum the distinct numbers
    int[] numberArray = { 4, 8, 6, 4, 8, 5 };
    int[] numberArray2 = { 4, 4, 5, 6, 8, 8 };

    Console.WriteLine(sumSpecial(numberArray).ToString());
    Console.WriteLine(sumSpecial(numberArray).ToString());

    Console.ReadLine();
}

static int getHighestScore(int[] integerArray)
{
    var high = 0;
    foreach (int number in integerArray)
    {
        high = high < number ? number : high;
    }
    return high;
}

static int getLowestScore(int[] integerArray)
{
    var low = int.MaxValue;
    foreach (int number in integerArray)
    {
        low = low > number ? number : low;
    }
    return low;
}

static int sumWithoutHighAndLowScores(int[] integerarray)
{
    int sum = 0;
    int high = getHighestScore(integerarray);
    int low = getLowestScore(integerarray);
    foreach (int number in integerarray)
    {
        if (number != high && number != low)
        {
            sum += number;
        }
    }
    return sum;
}

//sum of numbers using high or low only if there is a duplicate of high or low
static int sumSpecial(int[] integerArray)
{
    var sum = sumWithoutHighAndLowScores(integerArray);
    var high = getHighestScore(integerArray);
    var low = getLowestScore(integerArray);

    var highs = 0;
    var lows = 0;
    foreach (int number in integerArray)
    {
        if (number == high) { highs++; }
        if (number == low) { lows++; }
    }
    if (lows > 1) { sum += low; }
    if (highs > 1) { sum += high; }

    return sum;
}

What could be done better or more efficiently without losing the intentions of the exercise?

I think min and max are more descriptive names than high and low.
In C# the standard naming convention for methods is PascalCase.
You could save several iterations by doing the following things:

Create a single method for determining min and max in one go and return either a Tuple or a custom type or an array of length 2 or provide out parameters. If you don't want to use any more complex types than arrays then I'd probably go for out parameters (returning an array of a certain size doesn't seem very nice). So something along these lines:

public void DetermineMinAndMax(int[] input, out int min, out int max)
{
    min = Int32.Max;
    max = In32.Min;
    foreach (int number in input)
    {
        if (number < min) { min = number; }
        else if (number > max) { max = number; }
    }
}

Keep counters of how often you have seen the min/max already - if it's 1 add it otherwise skip. Something along these lines:

public void SumSpecial(int[] input)
{
    int min, max;

    DetermineMinAndMax(input, out min, out max);

    int countMinSeen = 0;
    int countMaxSeen = 0;

    int sum = 0;

    foreach (int number in input)
    {
        if (number == min)
        {
            if (countMinSeen == 1) { sum += number; }
            countMinSeen++;
        }
        else if (number == max)
        {
            if (countMaxSeen == 1) { sum += number; }
            countMaxSeen++;
        }
        else { sum += number; }
    }
}

This should reduce the number of required iterations to 2 instead of 4.


Improving efficiency of Prim's Algorithm I have implemented Prim's Algorithm in Java.  I am wondering how it can be made more efficient.
Below is the class Node for vertices.

public class Node {

private final char label;
private final Map<Node, Integer> neighbourList;

public Node(char label) {
    this.label = label;
    this.neighbourList = new LinkedHashMap<>();
}

public void addNeighbourer(Node node, int weight) {
    neighbourList.put(node, weight);
}

public char getLabel() {
    return label;
}

public Map<Node, Integer> neighbourerList() {
    return neighbourList;
}
}

Below is a Map freeMap which contains all vertices with value Integer.MAX_VALUE except for first vertex whose value is set to 0.

    freeMap.put(nodeA, 0);
    freeMap.put(nodeB, Integer.MAX_VALUE);
    freeMap.put(nodeC, Integer.MAX_VALUE);
    freeMap.put(nodeD, Integer.MAX_VALUE);
    freeMap.put(nodeE, Integer.MAX_VALUE);
    freeMap.put(nodeF, Integer.MAX_VALUE);

And below is Prim's Algorithm.

public void primMinimumWeightSpanningTree(Map<Node, Integer> freeMap) {

    Set<Node> mstSet = new LinkedHashSet<>();
    while (freeMap.size() > 0) {

        Node minNode = null;
        /* finds minimum node in freeMap as per corresponding value.*/
        for (Map.Entry<Node, Integer> entry : freeMap.entrySet()) {
            if (minNode == null) {
                minNode = entry.getKey();
            } else {
                if (entry.getValue() < freeMap.get(minNode)) {
                    minNode = entry.getKey();
                }
            }
        }

        freeMap.remove(minNode); /* remove minimum node from freeMap*/
        mstSet.add(minNode); /* add minimum node to MST set(mstSet)*/
        /* update values of adjacent nodes in freeMap*/
        for (Map.Entry<Node, Integer> entry : minNode.neighbourerList().entrySet()) {
            if (freeMap.containsKey(entry.getKey())) {
                int value = freeMap.get(entry.getKey());
                if (value > entry.getValue()) {
                    freeMap.replace(entry.getKey(), entry.getValue());
                }
            }
        }
    }
    /* display vertices once all are added to mstSet*/
    for (Node node : mstSet) {
        System.out.print(node.getLabel() + " ");
    }
}

Single responsibility principle

The primMinimumWeightSpanningTree() is clearly doing to much.
It is

finding the minimum node
updating values in the freeMap
displaying the result

You should extract the finding of the minimum node to a separate method. Also replacing the else with an else if will save one indention.

private Node getMinimumNode(Map<Node, Integer> freeMap){
    Node minNode = null;

    for (Map.Entry<Node, Integer> entry : freeMap.entrySet()) {
        if (minNode == null) {
            minNode = entry.getKey();
        } else if (entry.getValue() < freeMap.get(minNode)){
            minNode = entry.getKey();
        }
    }

    return minNode;
}

To be able to display the result outside of this method, we need to return a Set<Node>.
So let us refactor

public Set<Node> getPrimMinimumWeightSpanningTree(Map<Node, Integer> freeMap) {

    Set<Node> mstSet = new LinkedHashSet<>();
    while (freeMap.size() > 0) {

        Node minNode = getMinimumNode(freeMap);

        freeMap.remove(minNode);
        mstSet.add(minNode);

        /* update values of adjacent nodes in freeMap*/
        for (Map.Entry<Node, Integer> entry : minNode.neighbourerList().entrySet()) {
            if (freeMap.containsKey(entry.getKey())) {
                int value = freeMap.get(entry.getKey());
                if (value > entry.getValue()) {
                    freeMap.replace(entry.getKey(), entry.getValue());
                }
            }
        }
    }

    return mstSet;
}

and add a method for displaying

Public void displayResult(Set<Node> mstSet){

    for (Node node : mstSet) {
        System.out.print(node.getLabel() + " ");
    }

}

You can also think about overriding toString() so you can decide outside of the class, if you want to print to System.out or e.g write to a file.

Style

Comments are useful, but only if they tell why something is done. So telling what is done, where it is clearly visible like

freeMap.remove(minNode); /* remove minimum node from freeMap*/
mstSet.add(minNode); /* add minimum node to MST set(mstSet)*

these comments are just useless and should be removed.


Python & C: Trying to create a dict of all declared variables and functions and their types of a .c source file Basically trying to create a dict of all declared variables and functions along with their types of a .c source file. Does anyone think I missed anything?

#!usr/python
import string
import re
from types import *
from pprint import pprint

varDict = {}

def find_substring(needle, haystack):
    index = haystack.find(needle)
    if index == -1:
        return False
    if index != 0 and haystack[index-1] not in string.whitespace:
        return False
    L = index + len(needle)
    if L < len(haystack) and haystack[L] not in string.whitespace:
        return False
    return True

f = open('example.c')
varTypeList = []

for varType in open('typesfile.txt'):
        varTypeList.append(varType[:-1])

for line in f:
        for varType in varTypeList:
                if find_substring(varType,line):
                        var = re.search('\s*[\*_a-zA-Z]+[\*_a-zA-Z0-9]?', line.split(varType)[1])
                        if type(var) is not NoneType:
                                varDict[var.group(0)] = varType
pprint(varDict)

I think this code is fundamentally broken. What about comments and the preprocessor? And just by eyeballing the code I doubt very much that it parses declarations remotely correctly.

This is the wrong approach. string.find will only take you so far. You need to parse the code to get reliable information. The easiest way to do this is to use a library, such as libclang.