Running Lights - Embedded "Hello World" I'm getting my feet wet in embedded development, and like all those before me, my first task was to make an LED blink. I went a bit further than that and made a "runner" that lights up each led from 0 to 7 in sequence. The sequence begins as soon as the board is powered on. You can see it in action here.

I'm using an ATxMegaA1 chip with the avr-gcc compiler and tool chain. The document I consulted most was the Using IO pins and external interrupts doc.

I decided that creating an "Led" concept was a nice abstraction to wrap around the port they're attached to on the board. I also introduced a macro to change the delay when I'm debugging, because the simulator is orders of magnitudes slower than the real chip.

It's my first ever c, so please be brutal.

led.h

#include <avr/io.h>

#define LEDPORT PORTE_OUT
#define LEDPORT_DIR PORTE_DIR

void init(void);
void toggleLights(int ledPosition);

led.c

#include "led.h"

void init(void)
{
    LEDPORT_DIR = 0b11111111; //Configure LED port for output
    LEDPORT = 0b11111111; //LEDs are active low, this makes sure they're off on start up
}

//Turns light at provided port pin on and all others off.
void toggleLights(int ledPosition)
{
    LEDPORT = ~(1 << ledPosition);
}

Blink.c

#include <stdint.h>
#include "led.h"

#ifdef DEBUG
    #define DELAYITERATIONS 0
#else
    #define DELAYITERATIONS 10000
#endif

void delay(volatile uint32_t d)
{
    while (d-- != 0)     // loops while non-0 and decrements
    ;
}

int main(void)
{
    init();

    while(1)
    {
        for (int i = 0; i < 8; i++)
        {
            toggleLights(i);
            delay(DELAYITERATIONS);
        }
    }
}

Toggling?  Or setting?
Our toggleLights function is oddly named and doesn't do what I'd expect.  (Specifically, a toggle turns off things on and on things off.)
In C, and especially in embedded C, we shouldn't be afraid to use unsigned integers as bit arrays.
So, perhaps we want a setLights function that looks something like this:
void setLights(uint8_t lights) {
    LEDPORT = lights;
}

And to turn each of the eight lights on, one at a time, we can bitshift over them in our for loop:
for (uint8_t lights = 1; lights != 0; lights <<= 1) {
    setLights(lights);
    // delay
}

So now we're effectively doing what your code attempts to do, but the setLights function is even more flexible.  It just sets the lights to exactly whatever we tell it to set them to (arguably, the function may not need to exist at all... but it does restrict our input to a correct type and give us some sense of safety).

Using typedef
Firstly, we should be using a uint8_t here rather than an int because we're never going to use any values that we can't represent with 8 bits and that'll save us 24 bits.
But how about we use our own name for the type?
typedef uint8_t light_positions;

So now,our setLights function becomes:
void setLights(light_positions lights);

And our for loop becomes:
for (light_positions lights = 1; lights != 0; lights <<= 1) { // ...

This type helps add to the clarity of what this variable is supposed to represent and be used for.  This can be especially useful in embedded C where you might be prone to having lots of variables of the uint8_t type (or other sizes of unsigned integers).

lights <<= 1
<<= is a compound assignment operator.  Everyone knows and is familiar with +=, correct?  It's the most common of the ten compound assignment operators.
Meanwhile, << is a bitwise left shift operator (you used it in your toggleLights function).
So here's what's happening in our for loop.
We initialize lights to 0x00000001.  Run the loop body.  Update statement shifts lights to the left by 1 (lights <<= 1), and we end up with: 0x00000010.
We repeat this several times until we get to 0x10000000, and the next update statement shifts all of the bits out.  We end up with 0x00000000 (just 0) and the for loop exits, because lights != 0 returns false now.

About preprocessor macros...
Preprocessor macros are sometimes necessary, but generally, lead to code that is harder to debug.
Rather than your version of the macro, I think the code is more clear if we just inline it in the loop:
toggleLights(lights);
#ifdef DEBUG
    delay(1000);
#endif

Of course, if we're using the DELAYITERATIONS in multiple spots, then your version is fine--we don't want to be writing extra code necessarily.  But if we're only using it in one spot, write it more like this.

Use bools...
There are a number of ways to use true/false in C (which helps make your code more human readable).  Pick one and use it.

Hexadecimal vs Binary
Using hexadecimal notation is arguably more readable than counting the number of 1s in binary notation.  I think I'd prefer seeing 0xFF rather than 0b1111111.
Did you even notice there are only seven ones there?


Process list of e-mail addresses in PHP 5.3 Part of an application that I am building in CakePHP 2.1 needs to send out an e-mail based on a template from a database. The end user is able to set the from, reply to, to, cc, and bcc addresses. Because the user could enter just about anything, I need to process those addresses into a usable array that I can pass to CakeEmail. To this end, I wrote the following code and I want to make sure that it is efficient before I put it into production use (and possibly write a tutorial about it).

<?php
$parseAddress = function(&$address) {
    preg_match_all('/"(.*?)"\s*\<(.*?)\>/', $address, $matches); // Check if $address is in the format ""Test Address" <test@example.com>". TODO: Make the quotes optional.

    if (!is_array($matches) || !isset($matches[1]) || empty($matches[1]) || !isset($matches[2]) || empty($matches[2]) || is_null(array_filter($matches))) {
        return $address; // Return the plain e-mail address if name is not provided
    }

    $parsedAddress = array();

    for ($i = 0; $i < count($matches[0]); $i++) {
        $parsedAddress[$matches[2][$i]] = $matches[1][$i]; // Add e-mail address to array as "email => name"
    }

    $addressCSV = str_getcsv($address); // Split e-mail addresses into array - seems to automatically strip anything between "<" and ">" characters
    foreach($addressCSV as $key => $val) {
        if (!filter_var($val, FILTER_VALIDATE_EMAIL)) { // Take advantage of afformentioned stripping by filtering out everything that is not an e-mail address
            unset($addressCSV[$key]); // Remove the array value that is not an e-mail address
        }
    }

    return array_merge($addressCSV, $parsedAddress); // Join the parsed e-mail addresses with the filtered addresses and return the resulting array
};

echo $parseAddress('test@address.tld'); // returns (string) "test@address.tld"
echo $parseAddress('"Test Address" <test@example.com>, test@address.tld, "Smith, John"<jsmith@example.com>'); // returns (array) [0 => "test@address.tld", "test@example.com" => "Test Address", "jsmith@example.com" => "Smith, John"]

Alright, I have a number of suggestions. Some of which will invalidate others. The reason I'm keeping these "invalid" bits in is because they are just good practice and I'm trying to pass that along. In order to get the most from this you will have to read all the way through.

First off, why are you using lambda functions (also known as anonymous functions or closures)? This seems completely unnecessary. The thing to keep in mind about lambda functions is that they are not compiled at run time. This means that they should be used sparingly because each time they are called they must be compiled separately. Lambda functions should really only be used for callbacks. Though I'll be honest, I don't even use them then. They tend to make the functions they are used in bulky and therefore difficult to read. In the five, almost six, years I've been doing this I've never once used, or needed, lambda functionality. Best to just use a real function and set a return value. Though that may just be an opinion.

REGEX should only be used as a last resort. Yes it is much more powerful than any of the simpler string functions, but because of that power it requires more processing power as well, and is therefore usually slower. So, if you can do the same thing with a simpler function, it is more than likely better. For instance, you could use explode() here to separate the initial string by spaces to get the first name, last name, and email addresses. If multiple addresses are used you can use explode()'s limiter to only return 3 elements, then use explode() again on the email section. This appears to be what you are doing anyways and is more than likely quicker. Though you may not notice a difference unless you are doing it thousands of times. Besides, REGEX is almost impossible to read and if not documented properly just looks like a foreign language to those of us who come after you and try to figure out what it does. REGEX is another one of those things I've hardly ever needed to have, there is almost always another way.

$matches = explode( ' ', $address, 3 );
list( $firstName, $lastName, $addresses ) = $matches;//for illustration purposes only
$addresses = trim( $addresses, '<>' );//remove those brackets before exploding
$addresses = explode( ', ', $addresses );//could use str_getcsv() here if you wanted

Because explode() always creates an array unless the string passed to it was empty, you can now just check that the result isn't FALSE and then check the array's size to determine if the only thing it contains is an address. This looks much better than that long list of confusing if arguments.

if( $matches === FALSE ) {
    return FALSE;
}
if( count( $matches ) == 1 ) {
    return $address;
}

By the way, you should still validate and sanitize the address with filter_var() in the above scenario. Validate with FILTER_VALIDATE_EMAIL and sanitize with FILTER_SANITIZE_EMAIL. I believe you can even do both at once. Well, I know you can, but I'm not sure if it will return FALSE if its not valid, so you may have to play around with it.

$email = filter_var( $address, FILTER_VALIDATE_EMAIL | FILTER_SANITIZE_EMAIL );

For and while loops declare the functions passed to them as parameters on each iteration, so using count() as a parameter here is inefficient. This value isn't going to change so it should be declared before the loop. Foreach is the only loop that doesn't have this issue, though it is still better, in my opinion, to separate functions from foreach loops as well.

$length = count( $matches[ 0 ] );
for( $i = 0; $i < $length; $i++ ) {

Although, I believe you could get away with just using array_combine() here instead of a loop.

$parsedAddress = array_combine( $matches[ 2 ], $matches [ 1 ] );

str_getcsv() may do what you want, but only because it is a hack. CSV stands for "Comma Separated Values" and is used to store and read data separated by commas. I think, though I can't find it documented anywhere, that I remember reading somewhere that it also uses angle brackets "<>" to separate groups and that is why this works, though I'm not sure. I'm honestly not sure why this works, but you should not rely on hacks to get a job done. What if that function changes? Then if you ever upgrade your server you will be stuck trying to figure out how to fix it, or someone else with less knowledge of how this works will. Besides, you've already gotten those email addresses into an array, why do it again? Just use array_keys() on $parsedAddress to get an array of email addresses. Better yet, while you are looping over $matches the first time and setting them, why not just use filter_var() there? Or if you end up using the array_combine() example, you could use array_map() to apply a callback function, in this case filter_var(), to the array_keys(). You've not just doubled the work here, but quadrupled it. You loop over $matches, then you loop over $addressCSV, then when you merge them, PHP loops over both arrays again to find the differences.

Last thing. Why are you referencing $address? I can find no instance where you are changing its value. This is also unnecessary, and, in my opinion, is a big no-no. Variable referencing is a powerful tool, but, like most powerful tools, it does have some drawbacks. The biggest one is that they are difficult to spot and therefore make troubleshooting difficult. Another is that they aren't well known. So while I can spot this, someone else may have never seen it before. After all, it is just an ampersand and not an actual function that they can easily look up.

I hope this all helps. If you need any clarification, just ask.


Selecting CPUs and memory in Vagrant I generally like to be Rubocop-clean, but it seems like breaking up the first case statement will only make things harder to read. Any suggestions?

Vagrant.configure(VAGRANTFILE_API_VERSION) do |config|
  config.vm.provider 'virtualbox' do |vb|
    vb.customize ['modifyvm', :id, '--memory', host_hardware[:memory],
                  '--cpus', host_hardware[:cpus], '--ioapic', 'on']
  end

  private

  # rubocop:disable Metrics/AbcSize, Metrics/MethodLength
  def host_hardware
    case RUBY_PLATFORM
    when /linux/
      cpus = `nproc`.to_i
      memory = `sed -n -e '/^MemTotal/s/^[^0-9]*//p' /proc/meminfo`.to_i / 1024
    when /darwin/ # OS X
      cpus = `sysctl -n hw.ncpu`.to_i
      memory = `sysctl -n hw.memsize`.to_i / 1024 / 1024
    when /cygwin|mswin|mingw|bccwin|wince|emx/ # Windows
      cpus = `wmic computersystem get numberofprocessors`.split("\n")[2].to_i
      memory = `wmic OS get TotalVisibleMemorySize`.split("\n")[2].to_i / 1024
    else
      cpus = 2
      memory = 2048
    end
    { cpus: cpus, memory: reasonable_memory(memory) }
  end
  # rubocop:enable Metrics/AbcSize, Metrics/MethodLength

  def reasonable_memory(memory)
    half_of_memory = memory / 2
    case
    when half_of_memory < 1024
      1024
    when half_of_memory > 4096
      4096
    else
      half_of_memory
    end
  end
end

I'd start by breaking the host_hardware method into host_cpus and host_memory rather than returning a hash. A hash feels like a too "informal" structure.

Of course, you'll then need to do the case switching in multiple places, but you can streamline that a little bit by having a method that just returns a normalized platform identifier:

def host_platform
  case RUBY_PLATFORM
  when /linux/ then :linux
  when /darwin/ then :darwin
  when /cygwin|mswin|mingw|bccwin|wince|emx/ then :windows
  else :generic
  end
end

You'll still need case statements elsewhere, but at least they'll be simpler.

Alternatively, you can define the cpu/memory "getters" as procs or lambdas in a hash keyed by the platform:

# horrible name, but it's late
cpu_getters = {
  linux: -> { `nproc`.to_i },
  darwin: -> { `sysctl -n hw.ncpu`.to_i },
  windows: -> { `wmic computersystem get numberofprocessors`.split("\n")[2].to_i }
}

And a similar thing for getting memory information, of course.

Then, using the host_platform from before, you can so something like:

def host_cpus
  getter = cpu_getters[host_platform]
  getter ? getter.call : 2
end

And again, similar thing for memory.

Or, going all out, you could make "platform" classes with cpus and memory methods. If you need more than cpu and memory info,

Playing around a bit, I came up with the thing below. I know that is probably waaaay over-engineered for a simple vagrant file, but between one extreme and the other, you might find something useful.

module Platform
  def self.current
    case RUBY_PLATFORM
    when /linux/ then Linux.new
    when /darwin/ then Darwin.new
    when /cygwin|mswin|mingw|bccwin|wince|emx/ then Windows.new
    else Generic.new
    end
  end

  class Generic
    attr_reader :cpus, :memory

    def initialize
      @cpus = 2
      @memory = 2048
    end

    def reasonable_memory
      [1024, memory / 2, 4096].sort[1]
    end
  end

  class Linux < Generic
    def initialize
      @cpus = `nproc`.to_i
      @memory = `sed -n -e '/^MemTotal/s/^[^0-9]*//p' /proc/meminfo`.to_i / 1024
    end
  end

  class Darwin < Generic
    def initialize
      @cpus = `sysctl -n hw.ncpu`.to_i
      @memory = `sysctl -n hw.memsize`.to_i / 1024
    end
  end

  class Windows < Generic
    def initialize
      @cpus = `wmic computersystem get numberofprocessors`.split("\n")[2].to_i
      @memory = `wmic OS get TotalVisibleMemorySize`.split("\n")[2].to_i / 1024
    end
  end
end

With that you can call Platform.current and you'll get an object with the information you need. Note I've moved #reasonable_memory to an instance method too (and used a neat little trick to find the median value).

One obvious improvement would of course be memoizing Platform.current, so it only bothers with instantiating something once.

As a sidenote: I don't actually use Rubocop, though I generally adhere to the style guide, and agree with very nearly all of it (I often link to it in reviews, too). I tested all the code above though, and Rubocop let me off with a warning about line length (and a broken tail light).

But running Rubocop on a little script like this is somewhat overkill. Like getting out Strunk & White when writing a routine email. If you've used Rubocop for a while, I'm sure you've already learned to police your code yourself, and that's probably enough.


Find largest two numbers from list using clojure I'm trying to write a function that finds the largest two numbers from a list of positive integers in Clojure.

I've toyed around with Scheme in the past, but I'm very new to Clojure. This is what I came up with:

; Accumulator function for reduce later
(defn top-two [acc x]
  (let [big1 (first acc) big2 (second acc)]
    (cond
      (> x big1) [x big1]
      (> x big2) [big1 x]
      :else acc)))

(defn top-two-list [coll]
  (reduce top-two [0 0] coll))

This works, but I'm wondering if there is a way to simplify the top-two function, or to make this more readable or idiomatic.

This could be done much simpler if I was willing to sort the entire list, but I'm looking for an O(n) solution.

Any thoughts?

This looks like a clean way to find the max two elements. It can be shortened a little bit with destructuring:

; Accumulator function for reduce later
(defn top-two [[big1 big2 :as acc] x]
  (cond
    (> x big1) [x big1]
    (> x big2) [big1 x]
    :else acc))

(defn top-two-list [coll]
  (reduce top-two [0 0] coll))

You could also write something like so:

(defn top-two [coll]
  (let [big1 (apply max coll)
        big2 (apply max (remove #(= big1 %) coll))]
    [big1 big2]))

But I think it is programmer preference which you prefer. Both solutions are O(n), however the second ends up scanning the list twice.


Creating consecutive words from a string This is a method which creates an array of all possible consecutive substrings from a string:

def get_seperated_tokens(query)
  result = []
  length = query.split.count
  tokens = query.downcase.strip.split(' ')
  length.times do |i|
    length.times do |j|
      result << tokens[i..j].join(' ') if j >= i
    end
  end
  result
end

To get a better idea I have added rspec for it:

describe "#get_seperated_tokens" do
  it "returns an array of seperated tokens" do
    query = 'ruby is awesome'
    result = ['ruby','is', 'awesome', 'ruby is', 'is awesome','ruby is awesome']
    expect(get_seperated_tokens(query)).to include(*result)
  end
  it "returns an array of seperated tokens" do
    query = 'red blue iphones'
    result = ['red','blue', 'iphones', 'red blue', 'blue iphones','red blue iphones']
    expect(get_seperated_tokens(query)).to include(*result)
  end
end

How can this be made more idiomatic?

As @Michael Szyndel mentioned in a comment, Array#combination is the more appropriate method to use.

def get_separated_tokens query
  tokens = query.split
  (0..tokens.size).to_a.combination(2).map{ |i,j| tokens[i...j].join " " }
end


Node.js JSON searching & updating I'm working on a web-based on/off controller for multiple switches. I'm looking for a good way to manage the current state of a switch and updating the current state on change.

So far I've got a lib.json file which looks like this:

{
    "Lights":[{
        "name": "light-one",
        "pin":"3",
        "command":"gpioDo",
        "actions": {
            "on":"1",
            "off":"0"
        }
    },{
        "name": "light-two",
        "pin":"5",
        "command":"gpioDo",
        "actions": {
            "on":"1",
            "off":"0"
        }
    },{
        "name": "light-three",
        "pin":"7",
        "command":"gpioDo",
        "actions": {
            "on":"1",
            "off":"0"
        }
    }],
    "Locks":[{
        "name": "lock-one",
        "pin":"8",
        "command":"gpioDo",
        "actions": {
            "on":"1",
            "off":"0"
        }
    },{
        "name": "lock-two",
        "pin":"12",
        "command":"gpioDo",
        "actions": {
            "on":"1",
            "off":"0"
        }
    }]
}

A web-page is dynamically created using some jQuery and appropriate groups of buttons are created according to the lib.json.

On the server I take that lib.json file and create a state object via this little snippet (note that on initial run all states must be off):

var state = [];
var lib = JSON.parse(fs.readFileSync('lib.json', 'utf8'));
...
for (i in lib) {
  for (x in lib[i]){
    state.push({pin:lib[i][x].pin, val:0});
  }
}

which produces:

[ { pin: '5', val: 0 },
  { pin: '7', val: 0 },
  { pin: '8', val: 0 },
  { pin: '12', val: 0 } ]

When a web-page button is pushed, the server receives a string containing misc elements of the lib.json referencing a command with parameters to perform.

It also updates the state object to represent the change with an updateVal() command.

function updateVal(pinId, valId){
  var i;
  for (i = 0; i < state.length; ++i) {
    if (state[i].pin == pinId) {
      state[i].val = valId;
    }
  }
}

I also have a controller designer which allows you to create groups of buttons which in turn modifies the lib.json. It's basically a hacked up JSON tree editor.

How efficient is this code? Could I be doing something better, and if so, what? I can imagine that creating and updating a state key and value in lib.json could be more effective and reliable.

A few things to consider;

Is it worth abstracting the collection into the lights and locks arrays? The data format is effectively identical, and I think separating them makes the collection as a whole harder to iterate over.

To follow up from the first point, I try to avoid for loops in JavaScript because there's so many efficient and readable alternatives for working with arrays. If we were to rewrite your data structure to be the following:

{
    "switches": [{
        "name": "light-one",
        "pin": 3, // definitely store integers as ints and not wrapped in quotes!
        "command":"gpioDo",
        "actions": {
            "on": 1,
            "off": 0
        }
    }, {
        // ...
    }]
}

We can then rewrite your first example to be the following:

var state = lib.map(function(switch) {
    return {
        pin: switch.pin,
        val: 0
    };
});

This removes the index variables completely, and in my opinion makes the code more readable. In the case of not rewriting the data structure you could just iterate over the lights and locks arrays with a map function, then concat them together.

In the same way we can iterate over this when you want to set the values.

var updateVal = function(pinID, valID) {
    state.forEach(function(s) {
        s.pin === pinID && (s.val = valID);
    });
};

The function expression is just a personal preference, but I like it as it forces you to declare a function before using it.

I'm also not sure if the actions key needs to be like it is; perhaps you need it for other entities that have other actions, but I would probably replace it with a boolean active (to represent on and off), for simplicity.

Hope this helps. :)


Factorial/permutations/combinations program I wrote a factorial/permutations/combinations program in Java, and I want to do the same in C#.  I noticed (from various research) that when you use the ulong variable type, the max value is higher than Java's long.  My limitation in the Java version was that the long type was too small to do numbers like 30, or 100.  I wrote a little thing that would test if my hypothesis that C# could stretch the range of my program, but I was proven wrong.  My Java version can reach approximately 24, but the C# goes to roughly 20.  Is there anything I can do to reach my goal?

using System;

namespace codeAdmiral{
  public class Factorial{
     public static void Main(){
        ulong num;
        Console.WriteLine("Enter value");
        num = ulong.Parse(Console.ReadLine());
        ulong factorial = 1;
        for (ulong forBlockvar = num; forBlockvar > 1; forBlockvar--){
                 factorial *= forBlockvar;
        }
        Console.WriteLine("The Factorial for the Given input is:");
        Console.WriteLine(factorial);
    }
  }
}

Signed 64-bit integer (long in both Java and C#) can have values between approximately -9 · 1018 and 9 · 1018. Unsigned 64-bit integer (ulong in C#) can have values between 0 and approximately 1.8 · 1019. The value of 20! is approximately 2 · 1018, so it fits comfortably both to long and ulong. The value of 21! is approximately 5 · 1019, so it doesn't fit even into ulong.

Because of this, 20! should be computed correctly both in Java and c#, but 21! should be incorrect in both, no matter whether you use long or ulong.

If you want to make sure you are getting correct results in C#, you can use unchecked. If you do that and the computation overflows, you will get an exception.

What should you do if you want to compute larger values than 20!? The best option would be if there was some type that could represent arbitrarily large integers. Fortunately, both languages have them, there is BigInteger in C# and BigInteger in Java.


A simple testable standalone DLL for die rolling I've decided to challenge myself by writing a test standalone DLL with the objective of making all of its publicly-exposed objects able to be as intuitive and as simple-to-use as possible.

I've decided to try and create some objects that might be used in a game, like a deck of cards and a die.

Here's my Die for example:

public class Die
{
    private readonly Random _random;
    private readonly int _numberOfSides;

    public Die(int numberOfSides)
    {
        if(numberOfSides < 2)
            throw new ArgumentException(string.Format(GameComponentsResources.InvalidNumberOfSides, numberOfSides));

        _numberOfSides = numberOfSides;
        _random = new Random();
    }

    public int Roll()
    {
        return _random.Next(1, _numberOfSides);
    }
}

So obviously the random part of this is untestable unless I write a non-deterministic test, which I don't want to do.

If this was standard enterprise code I was writing, I'd mock the Random and inject it. However, let's say someone shelled out some cash for my GameComponents library. Do I think they would consider it a good product if they have to instantiate and pass in a random just to get a die for their game? Nah. I'd rather keep it concise and intuitive, like this: var die = new Die(6); or at the very most var die = dieCollection.GetDie(6);

So standard constructor injection is out of the picture for now. Property injection is better, but I still don't want to start depending on a container and writing a bootstrapper for something that is basically encapsulating a random.

I was thinking about getting hold of a seed value and passing it around (but without forcing consumers to pass in a seed value, unless they really wanted to), so I can get some deterministic values for my test, but the only way I can think to do this while maintaining good encapsulation is making a method like this:

protected virtual long GetSeed() //could be a property too
{
    return Environment.TickCount; //or whatever
}

...and then inheriting from it and overriding it in my tests. I could also do this for the Random itself. But that's generally a legacy code refactoring technique. Do I really want my shiny new code to rely on this?

I'd like to see some solutions where I can get near-100% test coverage (i.e. not creating more layers but still ultimately resulting in an area of code deemed "untestable") and maintain a simple, intuitive quasi-API as possible.

Add one more constructor that takes an extra Random param:

public Die(int numberOfSides, Random random)
{
    if(numberOfSides < 2)
        throw new ArgumentException(string.Format(GameComponentsResources.InvalidNumberOfSides, numberOfSides));

    _numberOfSides = numberOfSides;
    _random = random;
}

public Die(int numberOfSides)
{
    this(numberOfSides, new Random());
}

Most users will use the single-param version.
Some users, who want more control, for example in tests, can use the two-param version.
You can write your tests in a straightforward way using the two-param version.

If you really don't want to expose the two-param constructor in the public API,
then make it protected.
But I think it's quite fine to include in the public API,
it doesn't make the library confusing, and I don't see any harm.

If, for some reason you are against passing a Random instance (like @JasonLind, see his comment),
then you can make the constructor taking a Random above private,
and add another constructor that takes a seed parameter instead:

public Die(int numberOfSides, int seed)
{
    this(numberOfSides, new Random(seed));
}


Finding a perfect square smaller than an input number (Swift) This code runs slowly, even with numbers smaller than 1,000,000. Can someone help me optimize this thing?

// Finds the highest perfect square below a certain input (int)

let target: Int = 758865 // Swap this out for testing other numbers
var smlSqr = [Int]()
let findLimit = Int(Double(target).squareRoot().rounded(.up))   // Saves CPU time, because perfect squares less than
                                                                // number a but higher than a.squareRoot is impossible.

var index = 0
var maxSqr = 0 // Final result
var candidate = 0

repeat {
    smlSqr.append(index * index)
    index += 1
} while(index < findLimit)

while candidate < smlSqr.count {
    if smlSqr[candidate] > maxSqr {
        maxSqr = smlSqr[candidate]
    }
    candidate += 1
}

print(String(maxSqr)) // For debugging, should print out 100
print(Int(Double(maxSqr).squareRoot())) // For debugging, making sure the number is actually a perfect square

I'm using Swift 5.1 on Xcode 11.
Also using a playground (if that helps)

Also using a playground (if that helps)

Well, that is the main reason for the slow execution. A playground is an interactive environment, and the code is not optimized. For all computations in the main playground file, intermediate results are displayed in the side bar (such as the values of the variables, or how often a loop has executed). This makes longer computations really slow.

For code that consists of many computation steps you better

either move it to a function in a separate playground source file,
or use a compiled Xcode project instead of a playground.

I often prefer a compiled project over a playground because

you can debug your code (single-step, inspect variables, ...),
you can enable optimizations (compile with the “Release” configuration) for performance critical tasks.

But your code can also be simplified and optimized. This loop

var index = 0
var smlSqr = [Int]()
repeat {
    smlSqr.append(index * index)
    index += 1
} while(index < findLimit)

appends one array element in each loop iteration, so that it can be written as a map operation:

let smlSqr = (0..<findLimit).map { k in k * k }

(You may also think about better variable names: what does smlSqr stand for?)

And this

var maxSqr = 0 // Final result
var candidate = 0
while candidate < smlSqr.count {
    if smlSqr[candidate] > maxSqr {
        maxSqr = smlSqr[candidate]
    }
    candidate += 1
}

determines the largest array element. The is a dedicated method for that purpose:

let maxSqr = smlSqr.max()!

(We can force-unwrap here if we know that the array is not empty.) But the array elements are sorted in increasing order, so that the largest element is actually the last element:

let maxSqr = smlSqr.last!

So what we have so far is

let target = 758865
let findLimit = Int(Double(target).squareRoot().rounded(.up))
let smlSqr = (0..<findLimit).map { k in k * k }
let maxSqr = smlSqr.last!

and now we can see that this can be simplified even further: The last element appended to the array is (findLimit - 1) * (findLimit - 1), so that we only need

let target = 758865
let findLimit = Int(Double(target).squareRoot().rounded(.up))
let maxSqr = (findLimit - 1) * (findLimit - 1)

(And this is so short and fast that you can execute it in in a playground directly, without any of the optimization tricks mentioned above.)

To make the code reusable, you should define it as a function:

func largestSquareRoot(lessThan n: Int) -> Int {
    let root = Int(Double(n).squareRoot().rounded(.up)) - 1
    return root * root
}

Note that this gives a nonsensical result for \$ n = 0 \$ and crashes for \$ n < 0 \$. You'll have to define what result you want in those cases.


Reversing the words order of a string A couple of weeks I had a coding interview and the problem to solve was very simple: reverse the words order of a string. What did I do wrong here?

public static String reverse(String phrase) {
        String[] words = phrase.split(" ");
        StringBuffer sb = new StringBuffer();
        for (String word : words) {
            String reversed = new String();
            for (int j = word.length() - 1; j >= 0; j--) {
                reversed += word.charAt(j);
            }
            sb = sb.append(reversed).append(" ");
        }

        return sb.toString().substring(0, sb.toString().length() - 1);
    }

Thanks for your fast response guys, after reading your messages I tried to solve it different:

public static void reverse(String phrase, char delimiter) {
        char[] newPhrase = new char[phrase.length()];
        int start = 0, i = 0, currentPos = newPhrase.length;
        while (i < phrase.length()) {
            if (phrase.charAt(i) == delimiter || i == phrase.length() - 1) {
                char[] word = phrase.substring(start, i).toCharArray();
                currentPos = currentPos - word.length;
                System.arraycopy(word, 0, newPhrase, currentPos, word.length);
                if (i != phrase.length() - 1) {
                    newPhrase[--currentPos] = delimiter;
                }
                start = ++i;
            } else {
                i++;
            }
        }

        System.out.println(new String(newPhrase));
    }


What is the "proper" way to write this in Java? I have a class that has 5 similar methods, they just relay input parameters and return a result - best practice would be to skip the class, but that's not what I'm asking about :) This is a method from the class:

public String deleteItem(Integer itemId,String username, String kpNumber) {
        try {
            return getItemServiceFacade().deleteItem(itemId, username, kpNumber);
        } catch (RemoteException re) {
            String message = "5:unexpected error:" + re.toString();
            logger.error(message);
            return message;
        } catch (CoreException ce) {
            String message = "5:unexpected error:" + ce.toString();
            logger.error(message);
            return message;
        }
    }

Now, since all the methods are similar, each has the same catch block, each does the same things... so I'd like to rewrite this but I'm unsure how far should I go in "atomizing" the methods. One way is what I consider the "pure" way, so the new code looks like this:

public String deleteItem(Integer itemId,String username, String kpNumber) {
        try {
            return getItemServiceFacade().deleteItem(itemId, username, kpNumber);
        } catch (RemoteException re) {
            logException(re);
            return getMessage(re);
        } catch (CoreException ce) {
            logException(ce);
            return getMessage(re);
        }
    }

private String getMessage(Exception exception){
    return "5:unexpected error:" + exception.toString();
}

private void logException(Exception exception){
    logger.error(getMessage(exception));
}

but I'm also left wondering if I should combine these two methods into one method, like this

public String handleException (Exception e){
    String message = "5:unexpected error:" + exception.toString();
    logger.error(message);
    return message;
}

so what I guess I'm trying to ask is how far should you go in separation of concerns when you have clear and simple cases like this one.

Edit: It seems I have not managed to make myself clear, so I'll try again :) In this case simple case, is it considered better to write one method:

public String handleException (Exception e){
    String message = "5:unexpected error:" + exception.toString();
    logger.error(message);
    return message;
}

or should I have two:

private String getMessage(Exception exception){
    return "5:unexpected error:" + exception.toString();
}

private void logException(Exception exception){
    logger.error(getMessage(exception));
}

One alternative, although I don't know if you think it's too complex, would be to apply a visitor pattern:

private interface Command {
    String apply(ItemServiceFacade facade) throws RemoteException, CoreException;
}

public String deleteItem(final Integer itemId, final String username, final String kpNumber) {
    return execute(new Command() {
        @Override
        public String apply(ItemServiceFacade facade) throws RemoteException, CoreException {
            return facade.deleteItem(itemId, username, kpNumber);
        }
    });
}

    // other methods...

private String execute(Command command) {
    try {
        return command.apply(getItemServiceFacade());
    } catch (RemoteException re) {
        String message = "5:unexpected error:" + re.toString();
        logger.error(message);
        return message;
    } catch (CoreException ce) {
        String message = "5:unexpected error:" + ce.toString();
        logger.error(message);
        return message;
    }
}

I used this pattern when faced with a similar though slightly more complicated situation (implementing optimistic locking for a few different operations).


Abstract Person with Employees and Clients I have a interface with the following implementation

public abstract class PersonInterface
{
       public abstract void Update(PersonModel person);
        public abstract void Delete(int id);

        public virtual PersonModel Find(int id)
        {
            return new List<PersonModel>().Find(x => x.Id == id);
        }

        public virtual IEnumerable<Models.Person.Person> GetAll()
        {
            return new List<Models.Person.Person>();
        }
}

Since the application will handle client and employee i take that class and implement it. This is because client have a diferrent way of updating its properties and the same goes from employee

public class Client : PersonInterface{
  public override void Update(PersonModel person)
        {
           // implementing the way of how the client is going to be update it

        }

        public override void Delete(int id)
        {
            // implementing the way of how the client is going to be delete it
        }
}

And employee implementation:

public class Employee: PersonInterface{
  public override void Update(PersonModel person)
        {
           // implementing the way of how the employee is going to be update it

        }

        public override void Delete(int id)
        {
            // implementing the way of how the employee ent is going to be delete it
        }
}

And my question is: is this the correct way that solve my problem when handling cases that some classes have somethings in common but there are 2 or 3 this that may vary?

Yes. When some classes have things in common but common things different having a abstract base class is a good way to go.

Having a common base class gives you a number of advantages:

Reuse between the classes. Your Find and GetAll methods don't have to be rewritten for each type of person. So you avoid having to test this N times, and you avoid accidental (and subtle) differences in implementation (=> bugs!). (N = the number of classes extending from Person)
A consistent interface. Other code using a Person doesn't (necessarily) need to know what kind of Person it's working with. For example you could write a script to get all people and update them without having to worry about the differences between clients/employees.

But you need to make sure of a few things:

The implementations in your base class must work for all children, now and in the future. So if you want to add an Employer class you don't want to have to change the base class methods. If you do so you have to test your other class implementations to make sure they still work. That being said, minor modifications often aren't too much of a big deal.
If your classes have a small amount in common but large differences you could end up only partially being able to rely on a consistent interface. So you lose the flexibility of just operating on Person objects. If your language supports it traits might be a better way to go here. Also if you find yourself in this situation your classes might be getting too big. Try breaking them down. (Nobody likes a god object!)

But broadly speaking; Yes, go for it!


Looking for advice - Dependency Injection over Service Locator in Mef I'm wondering what people think about Dependency Injection vs Service Locator patterns. Specifically I'm using Prism with MEF. I'm also using the MVVM pattern.

So I have a service which I export. I also have a class which doesn't export its class type as I don't need that to be registered with MEF. This class is actually a View-Model which is assigned to a hierarchical data template inside a TreeView.

However the view-model needs to import an interface registered with MEF.

Now I have three possible ways to do this:

Set an [Import] on the interface and use ComposeParts in the constructor to inject the dependency.
Use SerivceLocator.Current pattern to get the instance of the service.
Pass in the service interface on the constructor, which is kind of a manual dependency injection.

Here some example code to demonstrate.

public interface IFooService
{

}

[Export(typeof(IFooService))]
[PartCreationPolicy(CreationPolicy.Shared)]
class FooSevice : IFooService
{

}

Then the three possible implementation of the View-Model are:

public class TreeItemVM_MefInjection
{
    [Import]
    public IFooService FooService { get; set; }

    public TreeItemVM_MefInjection()
    {
        var catalog = new AssemblyCatalog
          (System.Reflection.Assembly.GetExecutingAssembly());

        var container = new CompositionContainer(catalog);
        container.ComposeParts(this);
    }
}

public class TreeItemVM_ServiceLocator
{
    [Import]
    public IFooService FooService { get; set; }

    public TreeItemVM_ServiceLocator()
    {
        FooService = ServiceLocator.Current.GetInstance<IFooService>();
    }
}

public class TreeItemVM_ManualInjection
{
    public IFooService FooService { get; set; }

    public TreeItemVM_ManualInjection(IFooService fooService)
    {
        FooService = fooService;
    }
}

Each of the tree-view items actually has an ObservableCollection as the tree is hierarchical. Each ViewModel can create it's own children, based on the model data it uses (this is not shown in the above examples just to keep them simple).

So my issues with each of these are:

It seems a lot of code to write to get automatic injection and I'm worried about the performance of creating a catalog and container temporarily. Is this OK to do it like this?
The service locator seems easier but I've read that the service locator is an anti-pattern. Should I be concerned with this?
The last one will give better performance as each instance just passes the IFooService interface on the constructor, and can pass it down to it's children when they are instantiated. However if I want to add more dependency injection latter on I need to change the constructor so maybe automatic injection or service locator maybe better.

So what would people say is the best method? Is there is a defined best practices method to follow? Are all the methods valid is it is up to the company coding standards to define the pattern to use?

Obviously the TreeItems are just normally instanced with new. I don't want these to be exported to MEF as that is overkill and nothing outside of the class library needs to know about them. They are just normal class that need to have MEF dependency injection or find MEF registered interfaces.

Anyone have an opinion on the most desirable solution and any gotchas I should be aware of? I'm sure there are other possible solutions to this as well. Any info would be appreciated.

Service locator is an anti-pattern because it tends to create implicit dependencies which are not easy to see and usually cause grieve when trying to write unit tests. Especially your implementation which references what seems like a singleton instance which can cause additional problems (i.e. you need to take care to reset it between tests).
Avoid it.

I can't say much about the MEF injection but it seems roundabout having to write all that code. It also creates an implicit dependency which will probably cause grieve when unit testing your class and makes it directly dependent on MEF while there is no need for it.
Avoid it.

Clean and simple. Explicit dependency, easy to unit test and injection can be easily manged by any IoC container of your choice (Unity, Ninject, Windsor, etc.).
The way to go.


Matrix of checkboxes - only 1 allowed in a row Task
Create a matrix of checkboxes. The user must be able to select only 1 checkbox in a row or unselect all of them.

Solution
<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html;charset=utf-8" >
    <script type="text/javascript">
      function switch_checkbox(id_selected, id_pair)
      {
           //if the current checkbox is checked - uncheck the 2-nd checkbox in this row
          if(document.getElementById(id_selected).checked == true)
          {
              document.getElementById(id_pair).checked = false;
          }
      }
    </script>
  </head>
  <body>
    <form method="post" id="" action="send.php">
      <input type="checkbox" name="group1[]" id="1-1" value="1-1" onChange="switch_checkbox('1-1', '1-2');"/>
      <input type="checkbox" name="group2[]" id="1-2" value="1-2" onChange="switch_checkbox('1-2', '1-1');"/>
      <br />
      <input type="checkbox" name="group1[]" id="2-1" value="2-1" onChange="switch_checkbox('2-1', '2-2');"/>
      <input type="checkbox" name="group2[]" id="2-2" value="2-2" onChange="switch_checkbox('2-2', '2-1');"/>
    </form>
  </body>
</html>

As you see, the solution is very primitive. Is it possible to make it even simpler?

Assuming an unchecked starting condition you can eliminate a parameter and thus drop the conditional:

function switch_checkbox(id_pair)
{
    document.getElementById(id_pair).checked = false;
}

...

<input type="checkbox" name="group1[]" id="1-1" value="1-1" onChange="switch_checkbox('1-2');"/>


Basic PHP Cloud script I would like to add basic Cloud system to my website project. I have my php login system already included. Every person must be logged in to view this site.
There are several session variables like $_SESSION["id"].

I've got my files.php file - main file with list of uploaded files on server, form to upload new and delete. File data to remove/add is sent to PHP script by POST + hidden inputs keeping user id.

I've found some basic PHP functions that are successfully connected to my website and working.

uploadscript.php (w3schools)

<?php
$target_dir = "files/" . $_POST["id"] . "/";
$target_file = $target_dir . basename($_FILES["fileToUpload"]["name"]);
$uploadOk = 1;
$imageFileType = pathinfo($target_file,PATHINFO_EXTENSION);
// Check if image file is a actual image or fake image
if(isset($_POST["submit"])) {
    $check = getimagesize($_FILES["fileToUpload"]["tmp_name"]);
    if($check !== false) {
        echo "File is an image - " . $check["mime"] . ".";
        $uploadOk = 1;
    } else {
        $uploadOk = 0;
    }
}
// Check if file already exists
if (file_exists($target_file)) {
    header('Location: files.php?alert=4;');
    $uploadOk = 0;
}
// Check file size
if ($_FILES["fileToUpload"]["size"] > 500000) {
    header('Location: files.php?alert=5;');
    $uploadOk = 0;
}
// Allow certain file formats
if($imageFileType != "jpg" && $imageFileType != "png" && $imageFileType != "jpeg"
&& $imageFileType != "gif" ) {
    header('Location: files.php?alert=3;');
    $uploadOk = 0;
}
// Check if $uploadOk is set to 0 by an error
if ($uploadOk == 0) {
    echo "Sorry, your file was not uploaded.";
// if everything is ok, try to upload file
} else {
    if (move_uploaded_file($_FILES["fileToUpload"]["tmp_name"], $target_file)) {
        header('Location: files.php?alert=1;');
    } else {
        header('Location: files.php?alert=2;');
    }
}
?>

downloadscript.php (php.net/manual/en/function.readfile.php)

<?php
$file = "files/" . $_POST["id"] . "/" . $_POST["file"];

if (file_exists($file)) {
    header('Content-Description: File Transfer');
    header('Content-Type: application/octet-stream');
    header('Content-Disposition: attachment; filename="'.basename($file).'"');
    header('Expires: 0');
    header('Cache-Control: must-revalidate');
    header('Pragma: public');
    header('Content-Length: ' . filesize($file));
    readfile($file);
    exit;
}
?>

deletescript.php (php.net/manual/en/function.unlink.php)

<?php
$file = "files/" . $_POST["id"] . "/" . $_POST["file"];

if (file_exists($file)) {
    unlink($file);
    header('Location: files.php?alert=6;');
    exit;
}
?>

Are these scripts secure? Do you suggest any modifications to it to prevent some unexpected and unwanted actions by users?

Are these scripts secure?

They are not. All three scripts are vulnerable to directory traversal.

This is what an attacker can achieve with each script:

uploadscript.php: upload files with certain extensions to arbitrary directories, for example to deface the main site. This isn't that serious, but should be fixed.
downloadscript.php: download arbitrary files. This is serious, and needs to be fixed. An attacker can read any files the webserver has access to, thus revealing sensitive information.
deletescript.php: delete arbitrary files in arbitrary directories. This may possibly be used to bypass authentication in your application (eg by deleting something like authentication.php), or to make your application unusable (DOS).

You need to sanitize your inputs (basename for filenames, .. filter for directories, although you could filter stricter), and you need to check if the normalized filename + path is inside the directory you want it to be in.

Note also that even if you fix this, as ids are passed from the user, users will still be able to upload files to other users directory, read files of other users, and delete files of other users.

Note also that getimagesize is nice as defense in depth, but can easily be bypassed. I would remove the comment as to not leave a false sense of security (it's mainly a usability check).

Misc

Comments that just repeat what the code does are usually frowned upon, as they bloat your code and make it harder to notice relevant comments. If you want to structure your code, use functions (eg isValidFilesize, isValidFileExtension, etc).
500000 could actually either use an inline comment, or be put into a properly named variable.
I would get rid of the uploadOk flag, as it isn't all that nice to read. You also perform a header redirect for many of the cases anyways, so normal users will not see the echo.
don't mix snake_case and camelCase without a good reason.
imageFileType doesn't hold the image file type, but the file extension.


User php class. Should I connect to DB in this class? I have small Class which handles user's information. (name, surname, login, hash of password). Also this class can activate, register and login the user. This means I should connect to DB. Is it normal? I'm using Pear MDB2 to connect to DB.

Does it mean that every instance of User will have MDB2 object initialised?

class User {
    private $login;
    private $hash;
    private $name;

    /* some getters and setters */
    //
    // public function get_name() { return $this->name }
    // ..
    //

    private $connect; // MDB2 connection
    private $dsn = ""; // connection config

    public function exists() { // return true if User exists
        require ('MDB2.php');
        $this->connect = MDB2::connect($this->dsn);
        // check
    }
}

Should I make $connect and $dsn variables static? Or may be I should use some pattern for this idea?

Update: (kind of decision)

I store database connection in static variable:

class DataBase {
    // connections options
    static public $connect = NULL;      // MDB2 connection variable
    static public $dsn = "pgsql://user:pass@IP/database";
    static public $connected = false;

    static function reportError() {
        if (PEAR::isError(self::$connect)) {
            $res["success"]=false;
            $res["errors"][] = self::$connect->getMessage();
            echo json_encode($res);
            exit;
        }
    } //: reportError

} //: Class DataBase

So If I want to connect to DB I write:

// connect only if it's not connected
if (!DataBase::$connected) {
    require_once('MDB2.php'); // Pear MDB2 class
    DataBase::$connect = MDB2::factory(DataBase::$dsn); // connection
    DataBase::$connected = true; // Flag that we're already connected
}

It's normal? Or I should make it another way?

Also this class can activate, register and login the user. This means I should connect to DB. Is it normal?

It's up to you and your requirements. Personally I am trying to make all domain specific objects simple as possible and use it only as Value Object. So, in this case all actions such as checking for existance, creation, activation, registration etc should go to another class, for example, UserManager or similar. And in this class I will hold reference to DB and doing all SQL-queries.

Should I make $connect and $dsn variables static?

Again, it's depends from your requirements. But from my point of view -- no, you shouldn't. When you doing this you cannot have two different connections for MySQL and PostgreSQL, for example.
In any case, I suggest to change public modifier to private (or protected) for $connect, $dsn and $connected memebers.


Reading a certain number of lines from a file def getLinesFromFile(file:File, linesToGet:Int): List[String] ={
    val bufferedSource = Source.fromFile(file)
    val lb = ListBuffer[String]()
    try {
      val lines = bufferedSource.getLines()
      var counter = 0
      while (lines.hasNext && counter < linesToGet) {
        counter += 1
        lb.append(lines.next())
      }
    } finally {
      bufferedSource.close()
    }
    lb.toList
  }

I am trying to retrieve a certain amount of lines from a read text file, and return each line as a List back to the user. Validation for the file actually being a .txt file is done prior to this call, and I can say with certainty that each line won't be longer than @500 characters.

My primary concern is I'm wondering if there any reason this code would create any memory leaks?

That implementation is very procedural.  getLines() returns an Iterator, which supports a take(n) method.

def getLinesFromFile(file:File, linesToGet:Int): List[String] = {
  val bufferedSource = Source.fromFile(file)
  try {
    bufferedSource.getLines.take(linesToGet).toList
  } finally {
    bufferedSource.close
  }
}


Process multiple request from different threads in batches on one processing thread So I have a system that uses multiple threads to process data. These data could be processed individually but it would be better to process them in batches.

Lets assume we have a class Data, a class OtherData and a class Processor which implements Function<List<Data>, List<OtherData>>.

To process objects of type Data from multiple threads I designed two classes System and Evaluator.

public class System {
    private final Evaluator evalThread;
    private final Object sync = new Object();
    private Function<List<Data>, List<OtherData>> processor;
    private Map<Object, Data> inputMap;
    private Map<Object, CompletableFuture<OtherData>> futureMap;
    private List<Object> idList;

    public System() {
        processor = new Processor();
        inputMap = new HashMap<>();
        futureMap = new HashMap<>();
        idList = new LinkedList<>();

        evalThread = new Evaluator(processor, inputMap, futureMap, idList, sync);
        Thread thread = new Thread(evalThread, "EvalThread");
        thread.start();
    }

    public CompletableFuture<OtherData> process(Data data) {
        Object id = new Object();

        final CompletableFuture<OtherData> completableFuture = new CompletableFuture<>();

        synchronized (sync) {
            inputMap.put(id, data);
            futureMap.put(id, completableFuture);
            idList.add(id);

            if (idList.size() >= 32) {
                sync.notifyAll();
            }
        }

        return completableFuture;
    }
}

public class Evaluator implements Runnable {
    private final Function<List<Data>, List<OtherData>> processor;
    private final Map<Object, Data> inputMap;
    private final Map<Object, CompletableFuture<OtherData>> futureMap;
    private final List<Object> idList;
    private final Object sync;

    private AtomicBoolean keepRunning = new AtomicBoolean(true);

    public Evaluator(Function<List<Data>, List<OtherData>> processor, Map<Object, Data> inputMap, Map<Object,
                      CompletableFuture<OtherData>> futureMap, List<Object> idList, Object sync) {
        this.processor = processor;
        this.inputMap = inputMap;
        this.futureMap = futureMap;
        this.idList = idList;
        this.sync = sync;
    }

    @Override
    public void run() {
        synchronized (sync) {
            while(keepRunning.get()) {
                if (idList.size() > 0) {
                    List<Data> input = new LinkedList<>();

                    for (int i = 0; i < idList.size(); i++) {
                        input.add(inputMap.get(idList.get(i)));
                    }

                    List<OtherData> output = processor.apply(input);

                    for (int i = 0; i < idList.size(); i++) {
                        futureMap.get(idList.get(i)).complete(output.get(i));
                    }

                    idList.clear();
                    inputMap.clear();
                    futureMap.clear();
                }

                try {
                    sync.wait(20);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}

My idea was that any one can call process with singular data but the data will (if there are enough) be processed together with other Data objects.

Any suggestions for improvements or are there systems in the Java-framework that would fit this task better?
Do you might see problems according to deadlocks, etc.?

Well, technically you should really post complete code and not have
missing definitions.  Especially since Data and OtherData could
instead be generic type arguments perhaps, or interfaces, so that it's
actually clear what's happening.  I'm just gonna imagine they're
basically both Objects.  Also Processor is undefined.  If it was
really not important just pass it in as an argument to System, as it
is, this is incomplete too.

In System having a separate idList is pointless, the inputMap
already has the right size for implementing back pressure (I wanted to find a good definition, but right now at least Wikipedia doesn't have one under that term; basically if the input can grow unlimited, the system might not be able to catch up if processing takes too long and you might end up with a congested (read: out of memory) system).

In Evaluator, x.size() > 0 could be !x.isEmpty(), that might
potentially be cheaper, but it also expresses intent a bit more clearly:
The check is really whether the map "isn't empty", not about how many
items are in the container exactly.

InterruptedException is for control flow,
don't just print a stack trace.

Also now that I read it, why's there three containers, inputMap,
futureMap and idList all related to the same task?  It'd be much
easier if there was just a sequence of tuples, Tuple<Input, Future>,
and then work through them.  Then replace the List with a
BlockingQueue<Tuple<Input, Future>> and it's already supporting the
waiting / back pressure too.  An ArrayBlockingQueue could be used to
limit the number of elements, while a LinkedBlockingQueue could have
an unbounded size (that's really not advisable though).

Lastly instead of keepRunning a tombstone object could then be
inserted to cancel the thread, then there's also no need for polling via
wait.  That is, insert a new Tuple<>() (same as the IDs at the
moment via new Object()) and when dequeuing from the input, check
whether this element was inserted, due to the object identity being
unique that's then safe to do.

So without spelling it all out, the main thread would look like this
perhaps:

public void run() {
    while (true) {
        Tuple<Input, Future> tuple = input.take();
        if (tuple == tombstone) {
            return;
        }
        tuple.getFuture().complete(processor.apply(tuple.getInput()));
    }
}

With some implementation of Tuple of course; a custom class would work
too.

For the other questions:  I can't see it deadlocking right now, the check for 32 and the notifyAll are a bit odd though.

Other tools in the Java tool set?  Yes, I'd suggest starting with reading through the java.util.concurrent namespace.  In particular the ExecutorService perhaps.


Call a method from all GameObjects implementing a specific interface in Unity my player has a gun with 3 bullets. If he runs out of ammo the scene should reset itself. I don't want to load the scene again, some components should reset themselves by using an interface.

So my observer class keeps track of the bullets and when there is nothing "active" anymore it should reset the scene.

When a bullet goes inactive it starts calling the observer.

public class Observer : MonoBehaviour
{
    [SerializeField]
    private Projectile[] projectiles; // my 3 bullets

    public void ResetScene()
    {
        for (int i = 0; i < projectiles.Length; i++)
        {
            Projectile currentProjectile = projectiles[i];

            if (currentProjectile.IsActive) // Are any bullets left?
            {
                return; // Leave this method
            }
        }

        MonoBehaviour[] sceneObjects = FindObjectsOfType<MonoBehaviour>(); // find all objects in the scene

        for (int i = 0; i < sceneObjects.Length; i++)
        {
            MonoBehaviour currentObj = sceneObjects[i];
            IRewindable currentComponent = currentObj.GetComponent<IRewindable>(); // try to get the interface component

            if (currentComponent != null) // does the gameobject implement this interface?
            {
                currentComponent.Rewind(); // make it rewind itself
            }
        }
    }
}

I think the code below the first loop might not be the best. Is there a more optimized way of finding all interface components and call their Rewind method?

In the first loop I think, you could do:

if (projectiles.Any(p => p.IsActive))
{
  return;
}

In the second you could do something like:

foreach (var currentComponent in sceneObjects.Select(so => so.GetComponent<IRewindable>()).Where(r => r != null))
{
  currentComponent.Rewind();
}

But be aware, that using linq is not necessarily the most optimized approach (performance wise) , so you'll have to do some tests.


Finding missing number from series This code will accept user input from the command line.

The first user can enter the limit of series.

e.g: user may enter 5

After that, the user will be prompted for 5 numbers of a series.

e.g:

2
4
8
10
12

Here in this series, 6 is missing, so I am going to find the missing series from common difference series.

I got the correct output from this code. Is it the correct way?  Is there any other simple code to do the same?

<?php
    fscanf(STDIN, "%d\n", $count);
    $series = array();
    $common_diff = array();
    $prev_element = null;
    for($i=0;$i<$count;$i++){
        fscanf(STDIN, "%d\n", $series[$i]);
        if($prev_element != null){
            $common_diff[] = $series[$i] - $prev_element;
        }
        $prev_element = $series[$i];
    }
    $c = array_count_values($common_diff);
    asort($c);
    end($c);
    $common_d = key($c);
    $prev_element = $series[0];
    $missed_no = 0;
    for($i=1;$i<$count;$i++){
        $diff = $series[$i] - $prev_element;
        if($diff != $common_d){
            $missed_no = $prev_element + $common_d;
        }
        $prev_element = $series[$i];
    }
    echo $missed_no;
    exit;
?>

example input

5
2
4
8
10
12

Functions

The code can be more readable if it is broken into functions.  This also separates the concern of reading in the series and calculating the missing element(s).

function readSeries($fileHandle)
{
    $series = array();
    fscanf($fileHandle, "%d\n", $count);

    for ($i = 0; $i < $count; ++$i)
    {
        fscanf($fileHandle, "%d\n", $series[$i]);
    }

    return $series;
}

function findMissing(Array $series)
{
    sort($series);
    $start = reset($series);
    $finish = end($series);
    $step = ($finish - $start) / count($series);

    return array_diff(range($start, $finish, $step),
                      $series);
}

$series = readSeries(STDIN);
$missing = findMissing($series);
echo reset($missing);

The use of functions also help to make the code more reusable. The readSeries function can easily be used to read from standard input or another source.

The findMissing function takes advantage of some assumptions that can be made when a series is sorted:

It would be impossible to notice whether the beginning or end elements were missed, so they must both be present.
One the beginning and end values are known then every step along this range must be present.

It then uses the difference of the expected series calculated using the PHP range function.


QuickSort C++ Implmentation using Iterators #2 This is part 2 of the previous question asked which is at - QuickSort C++ Implementation using Iterators.

Based on Jerry Coffin's answer, I have made the changes except for using templates.

#include <iostream>
#include <vector>
#include <algorithm>
#include <iterator>
#include <utility>

typedef std::vector<int>::iterator itr;

itr partition(itr left,itr right)
{

    itr i=left-1;

    itr it=left;

    while(it<right)
    {
        if(*it<*right)
        {
            using std::swap;

            std::advance(i,1);
            swap(*i,*it);
        }
        std::advance(it,1);
    }

    using std::swap;

    swap(*(i+1),*right);
    std::advance(i,1);
    return i;
}

void quicksort(itr left, itr right)
{
    if(std::distance(left,right)>=2)
    {
        itr q=partition(left,right);
        quicksort(left,q-1);
        quicksort(q+1,right);
    }
}

int main()
{
    std::vector<int> v={6,10,4,5,3,9,13,8};

    std::copy(v.begin(),v.end(),std::ostream_iterator<int>(std::cout," "));
    std::cout<<std::endl;

    quicksort(std::begin(v),std::end(v)-1);

    std::copy(v.begin(),v.end(),std::ostream_iterator<int>(std::cout," "));
    std::cout<<std::endl;

    return 0;
}

You still have the same UB as in the original question. An unpleasant (i + 1) suggests the fix: first swap, then advance:

        itr i=left;
        itr it=left;

        while(it<right)
        {
            if(*it<*right)
            {
                using std::swap;

                swap(*i,*it);
                std::advance(i,1);
            }
            std::advance(it,1);
        }

        using std::swap;

        swap(*i,*right);
        return i;


Ruby method that is validating the payload received What is a cleaner and/or clearer way of writing the following method

def disbursements
  @invoice = Invoice.find(params[:id])
  utc_time = Time.parse.now.utc

#check if invoice_id is not nil
  if @invoice.invoice_id == nil
    @invoice.errors[:invoice_id] << "invoice_id can not be blank"
  end
#check if trip date is now or past
  if utc_time <= @invoice.trips.first
    @invoice.errors[:trips] << "Funds can not be disbursed yet."
  end
#check if trip had been cancelled
  if @invoice.cancelled == true
    @invoice.errors[:cancelled] << "This invoice was already been cancelled."
  end
#check if this invoie has been processed before
  if @invoice.service_rendered == true
    @invoice.errors[:service_rendered] << "This invoice has already been disbursed."
  end
#check all is well
  if @invoice.errors.empty? == false
    render json: @invoice.errors, status: :unprocessable_entity
    return
  end

...

Inline conditionals would make this a bit cleaner:

def disbursements
  @invoice = Invoice.find(params[:id])

  @invoice.errors[:invoice_id] << 'invoice_id can not be blank' if @invoice.invoice_id.nil?
  @invoice.errors[:trips] << 'Funds can not be disbursed yet.' if Time.now.utc <= @invoice.trips.first
  @invoice.errors[:cancelled] << 'This invoice was already been cancelled.' if @invoice.cancelled
  @invoice.errors[:service_rendered] << 'This invoice has already been disbursed.' if @invoice.service_rendered

  return render json: @invoice.errors, status: :unprocessable_entity if @invoice.errors.any?
  ...
end

We can slim the method down and save a lot of space that way.

I removed the utc_time variable because it was only used once.


Convert decimal currency to an integer I have some code that is used to convert a decimal currency to an integer with precision being a function of which currency is being converted.

This code is more of a "brute-force" method to do the conversion and just doesn't feel right to me. Is there a better way to achieve the result?

    public static int CurrencyToInt(decimal amount, string currencyCode)
    {
        var precision = 0;

        currencyCode = currencyCode?.ToUpperInvariant();

        if (new[] {
            "BHD", "IQD", "JOD", "KWD", "LYD", "OMR", "TND"
        }.Contains(currencyCode))
        {
            precision = 3;
        }
        else if (new[] {
            "AED", "AFN", "ALL", "AMD", "ANG", "AOA", "ARS", "AUD", "AWG", "AZN", "BAM", "BBD", "BDT", "BGN", "BMD",
            "BND", "BOB", "BRL", "BSD", "BTN", "BWP", "BZD", "CAD", "CDF", "CHF", "CNY", "COP", "CRC", "CUC", "CUP",
            "CZK", "DKK", "DOP", "DZD", "EGP", "ERN", "ETB", "EUR", "FJD", "FKP", "GBP", "GEL", "GGP", "GHS", "GIP",
            "GMD", "GTQ", "GYD", "HKD", "HNL", "HRK", "HTG", "HUF", "IDR", "ILS", "IMP", "INR", "IRR", "JEP", "JMD",
            "KES", "KGS", "KHR", "KPW", "KYD", "KZT", "LAK", "LBP", "LKR", "LRD", "LSL", "LTL", "LVL", "MAD", "MDL",
            "MKD", "MMK", "MNT", "MOP", "MUR", "MVP", "MVR", "MWK", "MXN", "MYR", "MZN", "NAD", "NGN", "NIO", "NOK",
            "NPR", "NZD", "PAB", "PEN", "PGK", "PHP", "PKR", "PLN", "QAR", "RON", "RSD", "RUB", "SAR", "SBD", "SCR",
            "SDG", "SEK", "SGD", "SHP", "SLL", "SOS", "SPL", "SRD", "STD", "SVC", "SYP", "SZL", "THB", "TJS", "TMT",
            "TOP", "TRY", "TTD", "TVD", "TWD", "TZS", "UAH", "USD", "UYU", "UZS", "VEF", "WST", "XCD", "XDR", "YER",
            "ZAR", "ZMW", "ZWD"
        }.Contains(currencyCode))
        {
            precision = 2;
        }
        else if (new[] {
            "MGA", "MRO"
        }.Contains(currencyCode))
        {
            precision = 1;
        }
        else if (new[]{
            "BIF", "BYR", "CLP", "CVE", "DJF", "GNF", "ISK", "JPY", "KMF", "KRW", "PYG", "RWF", "UGX", "VND", "VUV",
            "XAF", "XOF", "XPF"
        }.Contains(currencyCode))
        {
            precision = 0;
        }
        else
        {
            throw new ArgumentException("Unknown currency code " + currencyCode, nameof(currencyCode));
        }

        while (precision > 0)
        {
            amount *= 10;
            precision--;
        }

        return (int)Math.Floor(amount);
    }

Maybe it is intentionally(?):

I would expect CurrencyToInt(234.567m, "USD") to return 23457, but it returns 23456?

Instead of the rather unclear if-statements it is more readable with a switch:

public static int CurrencyToInt(decimal amount, string currencyCode)
{
  var precision = 0;

  currencyCode = currencyCode?.ToUpperInvariant();

  switch (currencyCode)
  {
    case "BHD": case "IQD": case "JOD": case "KWD": case "LYD": case "OMR": case "TND":
      precision = 3;
      break;
    case "AED": case "AFN": case "ALL": case "AMD": case "ANG": case "AOA": case "ARS": case "AUD": case "AWG": case "AZN": case "BAM": case "BBD": case "BDT": case "BGN": case "BMD":
    case "BND": case "BOB": case "BRL": case "BSD": case "BTN": case "BWP": case "BZD": case "CAD": case "CDF": case "CHF": case "CNY": case "COP": case "CRC": case "CUC": case "CUP":
    case "CZK": case "DKK": case "DOP": case "DZD": case "EGP": case "ERN": case "ETB": case "EUR": case "FJD": case "FKP": case "GBP": case "GEL": case "GGP": case "GHS": case "GIP":
....

As stated in the comments the whole thing can be boiled down to 3 lines of code, if you use the framework:

public static int CurrencyToIntByCulture(decimal amount, string currencyCode)
{
  CultureInfo culture = new CultureInfo(currencyCode);
  int precision = culture.NumberFormat.CurrencyDecimalDigits;
  return (int)(amount * (decimal)Math.Pow(10, precision));

  // Or?: return (int)Math.Round((amount * (decimal)Math.Pow(10, precision)));
}

But culture.NumberFormat.CurrencyDecimalDigits returns different numbers of decimals for certain cultures, so maybe you have special needs?


Algorithm to find path between nodes First time working with any type of graph traversal. The algorithm determines whether a path exists between two nodes using breadth first searching (BFS). The BFS method returns a tuple; indicating whether a match has been found and the length between the two nodes. I implemented it this way since it's part of a private member and wont be seen externally. When the result is seen as part of the HasPath method I modeled it after the Try-Pattern similar to TryParse(...).
The local function EnqueueNonVisitedChildrenNodes is declared as such since it would make no sense to call it anywhere else. It too uses a tuple to simplify its invocation.
Am I properly using the tuple type? Is there anything that could be improved with code structure?
using System.Collections.Generic;
using System.Diagnostics;
[DebuggerDisplay("Name = {Name}, ChildCount = {Children.Count}")]
public class Node
{
    public string Name { get; }
    public List<Node> Children { get; } = new List<Node>();
    public bool Visited { get; set; }

    public Node(string Name)
    {
        this.Name = Name;
    }

    public Node(string name, IEnumerable<Node> children)
    {
        Name = name;

        foreach (var node in children)
        {
            AddChild(node);
        }
    }

    public void AddChild(Node node)
    {
        Children.Add(node);
    }
}

The implemented algorithm.
public class PathAlgorithm
{
    Node Start { get; }
    Node End { get; }

    public PathAlgorithm(Node start, Node end)
    {
        Start = start ?? throw new ArgumentNullException(nameof(start));
        End = end ?? throw new ArgumentNullException(nameof(end));
    }

    public bool HasPath(out int length)
    {
        bool hasPath;
        (hasPath, length) = BreadthFirstSearch(Start);

        return hasPath;
    }

    public (bool, int) BreadthFirstSearch(Node root)
    {
        if (root == null)
        {
            return (false, 0);
        }

        Queue<(Node, int)> searchQueue = new Queue<(Node, int)>();
        root.Visited = true;
        searchQueue.Enqueue((root, 0));

        while (searchQueue.Any())
        {
            (Node node, int length) = searchQueue.Dequeue();
            if (node == End)
            {
                return (true, length);
            }

            EnqueueNonVisitedChildrenNodes(searchQueue, (node, length));
        }

        return (false, 0);

        void EnqueueNonVisitedChildrenNodes(Queue<(Node, int)> queue, (Node node, int length) foo)
        {
            foreach (var n in foo.node.Children)
            {
                if (!n.Visited)
                {
                    n.Visited = true;
                    searchQueue.Enqueue((n, foo.length + 1));
                }
            }
        }
    }
}

The code you have there may accomplish the task you have in mind for it, but it's not useful for a "real world" usecase.
That's because usually you'd want to be able to run an algorithm like this multiple times on the same graph, maybe even concurrently.
The problem that's posing itself here is that Node mixes two concerns: Representing the graph and traversing it.
Storing the Visited state inside the Node is really useful, but it's also quite limiting. As such you could greatly benefit from storing visited Nodes as a Set instead to separate traversal from representation.

There's also some minor optimization possibilities:

Replace the foreach in the Node constructor with AddRange

Once .NET 5 releases (or you switch to .NET Core) You can slightly simplify the while loop in the BFS itself using searchQueue.TryDequeue(var (node, length)).

Storing visited nodes in a HashSet (or similar) enables reformulating EnqueueNotVisitedChildrenNodes as:
foreach (var child in node.Children.ExceptWith(visited))
{
    searchQueue.Enqueue((child, length + 1));
}

This reformulation of course then messes with the visited state of nodes you didn't actually visit yet, since you set Visited when you enqueue the node, not when you actually visit it.
This flagging visited when enqueueing utterly breaks as soon as you start working with "weighted graphs", where the smallest number of edges may not be the optimal path (like the following)
(A) -(1)- (B) -(1)- (C)
  \                 /
   -------(5)-------

Going from (A) to (C) via (B) is cheaper than the direct route here :), if your real target was a node (D) that's only connected to (C), your current code would route you through (C) because the shorter route through (B) would be ignored since (C) already counts as visited when you dequeue (B).

Overall this code is really nice and clean, but it lacks general applicability (which is not a bad thing in itself).


Bowling calculator in Perl A bowling score calculator is my go-to exercise for learning the basics of a language. I recently had to pick up Perl so I did this.

I'm concerned that I'm doing subroutines wrong. I had a hard time figuring out the syntax for dealing with arguments and what exactly @_ is.

I'm just looking for feedback on any egregious mistakes or instances where I go against convention.

use strict;
use warnings;
use List::Util qw(sum);

sub is_strike { #is this frame a strike?
    $_[0] == 10;
}

sub is_spare { #is this frame a spare?
    ($_[0] + $_[1]) == 10;
}

sub throws_for_score { #how many extra throws contribute to this frame's score?
    (is_strike(@_) || is_spare(@_)) ? 2 : 1;
}

sub frame_advance { #how many throws should we consume to move to the next frame?
    is_strike(@_) ? 1 : 2;
}

sub score_frame { #sum the score for x throws, where x is the number of throws that count for this frame's score
    my $frame_index = $_[1];
    my $frame_end = $frame_index + 2;
    my $score_end = $frame_index + throws_for_score(@{$_[0]}[$frame_index..$frame_end]);
    sum @{$_[0]}[$frame_index..$score_end];
}

sub score_game {
    (my $game = $_[0]) =~ s/-//gi; #remove hyphens
    my @throws = split(//, $game); #create an array of throws
    find_strikes(\@throws);
    my $current = 0;
    my $score = 0;
    do {
        my $frame_score = score_frame(\@throws, $current);
        $score += $frame_score;
        $current += frame_advance(@throws[$current..($current+2)]);
    } while($current <= $#throws-2); #while the current index is not beyond the tenth frame
    $score;
}

sub find_strikes { #replace strike marks with 10
    foreach(@{$_[0]}) {
        if($_ eq 'X') {
            $_ = 10;
        }
    }
}

print score_game("X-X-X-X-X-X-X-X-X-X-XX") . "\n"; #300
print score_game("55-55-55-55-55-55-55-55-55-55-5") . "\n"; #150
print score_game("00-00-00-00-00-00-00-00-X-X-XX"); #60

It's nice that you're using strict and warnings.

Subroutine Arguments

A subroutine should first unpack the argument list, e.g. like my ($foo, $bar) = @_;, then do some processing, and explicitly return a value. While we can return implicitly and can access items in @_ directly, both can be error-prone and should be avoided in most situations.

For example, this would change the is_strike to

sub is_strike {
    my ($frame) = @_;
    return $frame == 10;
}

The line (is_strike(@_) || is_spare(@_)) ? 2 : 1; is similar obfuscation. First, we'll get rid of the unnecessary ternary operator:

return 2 if is_strike(@_) || is_spare(@_);
return 1;

Next, we make it clearer by unpacking the arguments here as well. Don't mind the implied copy, we are optimizing for readability not performance!

my ($frame_1, $frame_2) = @_;
return 2 if is_strike($frame_1) || is_spare($frame_1, $frame_2);
return 1;

Why is this better? We immediately see that this function only uses its first two parameters.

We can do the same exercise with score_frame. You are using the @_ argument array throughout the whole subroutine, instead of giving each argument a self-explaining name.

# sum the score for x throws,
# where x is the number of throws that count for this frame's score
sub score_frame {
    my ($frame, $index) = @_;
    my $frame_end = $index + 2;
    my $score_end = $index + throws_for_score(@$frame[$index .. $frame_end]);
    return sum @$frame[$frame_index .. $score_end];
}

Let's look at this excerpt:

(my $game = $_[0]) =~ s/-//gi; #remove hyphens
my @throws = split(//, $game); #create an array of throws

The /i flag (case insensitive) on the substitution makes no sense, as you aren't using any characters that have a case. Try to avoid /i, as it makes many optimizations in the regex engine impossible.

If you are only deleting a set of characters, you can use the more efficient transliteration tr/-//d, where d is “delete”. Together with proper argument unpacking, we get:

my ($game) = @_;
$game =~ tr/-//d;
my @throws = split //, $game;

Whether you use parens for the arguments to builtin functions is largely a matter of taste. I prefer to omit them as I find the resulting code cleaner (and faster to write). Others enjoy that parens make the argument list explicit, which is otherwise hard to understand (technically, Perl parsing is not decidable).

This line is hard to understand:

while($current <= $#throws-2); #while the current index is not beyond the tenth frame

I don't know what the tenth frame is about, but wouldn't a condition like $current <= 10 be better if this is about the tenth frame?.

The map Function

The find_strikes function has a number of problems. Here is it's current form:

sub find_strikes { #replace strike marks with 10
    foreach(@{$_[0]}) {
        if($_ eq 'X') {
            $_ = 10;
        }
    }
}

Modifying data that has been passed in is generally frowned upon. If you do something like that, document it. Even better, return a new list.
Whenever possible, name the iteration variable instead of using $_.
Some people prefer the statement if condition form instead of the verbose if (condition) { statement }. Note that the braces are not optional in this case.

For example, we could rewrite the code like this:

sub find_strikes {
    my ($frames) = @_;
    my @frames_with_strikes;

    for my $frame (@frames) {
        if($frame eq 'X') {
            push @frames_with_strikes, 10;
        }
        else {
            push @frames_with_strikes, $frame;
        }
    }

    return @frames_with_strikes;
}

That is excessively verbose, and actually a good example where the ?: operator can be set to good use:

sub find_strikes {
    my ($frames) = @_;
    my @frames_with_strikes;

    for my $frame (@frames) {
        push @frames_with_strikes, ($frame eq 'X') ? 10 : $frame;
    }

    return @frames_with_strikes;
}

We can shorten this further by using a map list transformation instead of an explicit loop:

sub find_strikes {
    my ($frames) = @_;
    return map { $_ eq 'X' ? 10 : $_ } @$frames;
}

It is more “perlish” to take a list of values rather than an array reference:

sub find_strikes {
    my (@frames) = @_;
    return map { $_ eq 'X' ? 10 : $_ } @frames;
}

or without unnecessary copies:

sub find_strikes {
    return map { $_ eq 'X' ? 10 : $_ } @_;
}

In either way, it could now be invoked as

my @frames = find_strikes(split //, $game);

Less action at a distance, and clearer code.

say > print

You have lines like print score_game("X-X-X-X-X-X-X-X-X-X-XX") . "\n". This can be improved

… by passing a list rather than a single string:

print score_game("X-X-X-X-X-X-X-X-X-X-XX"), "\n";

all arguments will be joined without a space (unless you change certain settings).
… by using say instead of print. The say function behaves exactly the same, but automatically appends a newline. Prefer it for normal text output. Use print when you don't want to print out a line, e.g. when handling binary data, a protocol with specific requirements on line endings, or when printing half a line.

use feature qw/say/;  # activate the "say" function, available since v5.10.0

say score_game("X-X-X-X-X-X-X-X-X-X-XX");

Testing

In comments to your print invocations, you have given the expected output. Let's make an automated test out of this!

The Test::More module is Perl's most commonly used test framework. Here is how we can write a test:

use Test::More;

is(score_game("X-X-X-X-X-X-X-X-X-X-XX"),          300, '"X"es');
is(score_game("55-55-55-55-55-55-55-55-55-55-5"), 150, 'fives');
is(score_game("00-00-00-00-00-00-00-00-X-X-XX"),   60, 'zeroes and "X"es');

done_testing;

The is test compares for string equivality (which is also good enough for numbers). There is also the ok test which asserts that some condition is true. The last argument is an optional test description.

When we now execute the script, we get the following output:

ok 1 - "X"es
ok 2 - fives
ok 3 - zeroes and "X"es
1..3

everything passed, nice! If I'm sneaky and change the expected output of one of the functions, we get:

ok 1 - "X"es
not ok 2 - fives
#   Failed test 'fives'
#   at so1.pl line 54.
#          got: '150'
#     expected: '42'
ok 3 - zeroes and "X"es
1..3
# Looks like you failed 1 test of 3.

The is test outputs a reason for the test failure when the input doesn't match our expectations.

The output format is called TAP (Test Anything Protocol), and rarely used manually. Test suites are usually executed with the prove tool, which ships with Perl:

$ prove some_test.pl
some_test.pl .. ok
All tests successful.
Files=1, Tests=3,  0 wallclock secs ( 0.04 usr  0.00 sys +  0.02 cusr  0.00 csys =  0.06 CPU)
Result: PASS


C# Asp.net core middleware for redirecting URLs I wrote a small middleware for redirecting between WWW URLs.
The most important thing that I want to change is to make it super fast.
Here's what it is doing:
If the host is localhost or mywebsite.com without any subdomains and without www, it will redirect to www.localhost or www.mywebsite.com.
If the host starts with www. AND has subdomains, it will redirect to a url without www.
using System;
using System.Threading.Tasks;
using System.Text.RegularExpressions;

using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Extensions;
using Microsoft.AspNetCore.Builder;

using Microsoft.Extensions.Logging;

namespace Website.Middlewares
{

    public class WWWRedirectionMiddleware
    {
        private readonly RequestDelegate _next;

        private string _domain;
        private int _https_port;

        public WWWRedirectionMiddleware(RequestDelegate next)
        {
            _next = next;
        }

        public async Task InvokeAsync(HttpContext context)
        {

            var request = context.Request;
            var response = context.Response;

            var host = request.Host;

            if (Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT") == "Development")
            {
                _domain = "localhost";
                _https_port = 5001;
            }
            else
            {
                _domain = "seaoftools.com";
                _https_port = 80;
            }

            // Redirect to www domain if url is home page
            if (host.Host.Equals(_domain, StringComparison.OrdinalIgnoreCase))
            {
                host = new HostString("www." + host.Host, _https_port);

                var redirectUrl = UriHelper.BuildAbsolute(
                    "https",
                    host,
                    request.PathBase,
                    request.Path,
                    request.QueryString);

                response.Redirect(redirectUrl, true);
                return;
            } // Redirect to non-www domain if url is with a subdomain
            else if (host.Value.StartsWith("www.", StringComparison.OrdinalIgnoreCase)
                     && Regex.Replace(host.Host, @"www.|" + _domain, String.Empty) != String.Empty)
            {
                host = new HostString(host.Host.Replace("www.", String.Empty), _https_port);

                var redirectUrl = UriHelper.BuildAbsolute(
                    "https",
                    host,
                    request.PathBase,
                    request.Path,
                    request.QueryString);

                response.Redirect(redirectUrl, true);
                return;
            }

            await _next(context);
        }
    }

}

What should I change to make the middleware run faster?

If you add IWebHostEnvironment as a dependency to the middleware class, you can simplify the environment check by this:
if (webHostEnvironment.IsDevelopment())
{

}


selecting an item in a listbox to assign it as a temp variable - Review Code I got this answer here: https://stackoverflow.com/questions/15902299/selecting-an-item-in-a-listbox-to-assign-it-as-a-temp-variable

Suppose you have loaded the lisBox1 with objects of type Genre.

lisBox1.Datasource = this.GetAllGenresFromTextFiles();
lisBox1.DisplayMember = "Genre";

Add an event to SelectionChanged of your lstBox1 :

lstBox1.SelectionChanged += lstBox1_SelectionChanged;

And then in this event, filter the data for listbox2 :

void lstBox1_SelectionChanged(object sender, EventArgs e)
{
    var movies = m_allMovies;
    var genre = lstBox1.SelectedItem as Genre;
    if(genre != null)
    {
        movies = movies.Where(m => m.Genre == genre.Genre).ToList();
    }
    lstbox2.Datasource = movies;
}

m_allMovies is a list of all the movies you have loaded from your textfiles.
You can also read your text file every time the user clicked a different genre, it will consume less memory but more CPU:

void lstBox1_SelectionChanged(object sender, EventArgs e)
{
    var movies = this.GetAllMoviesFromTextFiles();
    var genre = lstBox1.SelectedItem as Genre;
    if(genre != null)
    {
        movies = movies.Where(m => m.Genre == genre.Genre).ToList();
    }
    lstbox2.Datasource = movies;
}

The keypoint here is not to filter row from the Listbox. You must think of it as your Listbox contains a subset of all your movies.
And each time you want to add or remove rows from a Listbox, in fact you are reloading it with appropriate datas.

and my problem is this:

With the first 2 lines of code (At the top of the answer), where would i place this (I'm new to C# so .Datasource & DisplayMember are new things to me). Also where you have "this.GetAllGenresFromTextFiles" (and the same for var movies = this.GetAllMoviesFromTextFiles) --> would i place the location of my text file within the brackets?

Not sure if this question really fits here, but I'll try to help anyway.

  With the first 2 lines of code (At the top of the answer), where would
  i place this (I'm new to C# so .Datasource & DisplayMember are new
  things to me).

DataSource is used when you're databinding a control to a collection.
DisplayMember is used to determine which member (property) of the databound type will be used. For instance you have a class Person (id, firstname, lastname) and you want to display lastnames in your listbox. So you set listbox.DisplayMember = "lastname"

You can put these lines pretty much anywhere. When you fire up the method containing these commands, your listbox will get populated by the data specified in DataSource. You can use Load event for instance, however, I'd recommend to use an extra method called PopulateListBox and call that from Load event...

  would i place the location of my text file within the brackets?

That depends on many things..For instance what exactly are you trying to achieve, how is the rest of your application structured, what are you programming habits etc...
If you wanted to use the code from Scorpi0, the method GetAllMoviesFromTextFiles expects to have everything it needs without any input. It should contain a method body to get all movies.

Furthermore, I'd suggest you one thing - if this is too advanced for you and you still don't understand, get some books and start with something simpler. You need to understand how to divide the application into logical layers, how to separate UI from data access etc. This is a topic for multiple books and a few years experience. You can't expect just to start typing some code and immediately get it all right. From a big part it's very opinionated as well, there are rarely "the best practices". You ask 10 different programmers and you get 10 different answers. You need to learn many things, so you will be able to decide for yourself what suits you the best in the current situation you're in.


Substitution Cipher in Haskell Learn You a Haskell presents the Caesar Cipher:

  The Caesar cipher is a primitive method of encoding messages by
  shifting each character in them by a fixed number of positions in the
  alphabet

EDIT

Since I'm not limiting this cipher to letters, it's not actually the Caesarian Cipher.
Thanks to Anonymous. This implementation is only a Substitution Cipher.

Here's my implementation:

import Data.Char (ord, chr)

encode :: Int -> String -> String
encode n = map $ shift' n
                 where shift' x =  chr . (+ x) . ord

decode :: Int -> String -> String
decode n = map $ shift' n
                 where shift' x =  chr . abs . ((-) x) . ord

Test:

*Main Data.Char> encode 1 "AAA"
"BBB"
*Main Data.Char>
*Main Data.Char> decode 1 "BBB"
"AAA"

I don't like that I'm using abs in my decode. How can I write a function (through currying) that will subtract X from it?

Also, please critique my implementation.

This is a legitimate substitution cipher, but it's not a Caesarean cipher. A Caesarian cipher affects only letters, and it's modulo the alphabet size: caesar 13 "Hello, world!" ⇒ "Urzzb, jbeyq!"

encode and decode can be the same function (caesar?) called with opposite shifts. (Or with the same shift, when it's 13.)

There's no need for a shift' helper.

  How can I write a function (through currying) that will subtract X from it?

(x -)