UnitTest strong exception guarantee I want to properly test the strong exception guarantee of a method.

The method in question is SetFiles. It does not have side-effects (it only modifies the object it is called upon). It is expected to change some object fields and to modify the elements of FileList field - a collection. In case the argument is invalid then it throws ArgumentException and it is expected to not modify the object.

This is what I have:

[TestClass]
public class UnitTest_Files
{
    [TestMethod]
    public void TestFiles1()
    {
        var fs = new Files();

        // 1st call to SetFiles. valid

        fs.SetFiles(new string[]
            {
                @"C:\work\file1.txt",
                @"C:\work\file2.avi",
                @"C:\work\file3",
            });
        Assert.AreEqual(@"C:\work", fs.ParentFolder);
        Assert.AreEqual(3, fs.FileList.Count);

        Assert.AreEqual(@"C:\work\file1.txt", fs.FileList[0].Path);
        Assert.AreEqual(@"C:\work\file2.avi", fs.FileList[1].Path);
        Assert.AreEqual(@"C:\work\file3", fs.FileList[2].Path);

        // 2nd call to SetFiles. valid

        fs.SetFiles(new string[]
            {
                @"C:\muzică\Diabulus in Musica\poze\membri\Zuberoa Aznárez.jpg",
                @"C:\muzică\Diabulus in Musica\poze\membri\Gorka Elso.jpg",
            });

        Assert.AreEqual(@"C:\muzică\Diabulus in Musica\poze\membri", fs.ParentFolder);
        Assert.AreEqual(2, fs.FileList.Count);

        Assert.AreEqual(@"C:\muzică\Diabulus in Musica\poze\membri\Zuberoa Aznárez.jpg", fs.FileList[0].Path);
        Assert.AreEqual(@"C:\muzică\Diabulus in Musica\poze\membri\Gorka Elso.jpg", fs.FileList[1].Path);

        // 3rd call to SetFiles. Invalid

        // set to invalid structure (files not in same folder)
        // and expect throw
        // and no change
        try
        {
            fs.SetFiles(new string[]
                {
                    @"C:\muzică\folder1\file2.txt",
                    @"C:\muzică\folder2\file2.txt"
                });

            Assert.Fail("expected exception");
        }
        catch
        {
        }

        // test that fs was not modified

        Assert.AreEqual(@"C:\muzică\Diabulus in Musica\poze\membri", fs.ParentFolder);
        Assert.AreEqual(2, fs.FileList.Count);

        Assert.AreEqual(@"C:\muzică\Diabulus in Musica\poze\membri\Zuberoa Aznárez.jpg", fs.FileList[0].Path);
        Assert.AreEqual(@"C:\muzică\Diabulus in Musica\poze\membri\Gorka Elso.jpg", fs.FileList[1].Path);
    }
}

This is my first dive into C# unit testing and the code I wrote seems very cumbersome. I particularly don't like the copy-pasted last tests. The object is not clonable/copyable and I don't have any other reason to implement that. So I don't know how other way to test that object after is the same as before. Also the entire test doesn't seem very scalable.

Do not attempt to put all the test criteria in one test.
Make seperate (small) tests, which each cover one specific scenario.

If all tests pass, you know it is ok.
But make sure the tests do not depend on each other: that they can run in any order.

For the exception assert, it is better to use an Attribute on the test method:

Update:

As Roland Illig mentioned, it is good to check that nothing was modified.

Updated code fragment:

[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void SetFiles_FilesNotInTheSameFolder_ExpectedException()
{
    // 3rd call to SetFiles. Invalid

    // set to invalid structure (files not in same folder)
    // and expect throw
    // and no change

    var validParentFolder = @"c:\C:\muzică\folder1";
    var validFile1 = @"c:\C:\muzică\folder1\file1.txt";

    var invalidFile1 = @"C:\muzică\folder1\file2.txt";
    var invalidFile2 = @"C:\muzică\folder2\file2.txt";

    var fs = new Files();
    fs.SetFiles(new string[]
    {
        validFile1
    });

    fs.SetFiles(new string[]
        {
            invalidFile1,
            invalidFile2
        });

    // test that fs was not modified

    Assert.AreEqual(1, fs.FileList.Count);
    Assert.AreEqual(validParentFolder, fs.ParentFolder);

    Assert.AreEqual(validFile1, fs.FileList[0].Path);
}

Last remark, if this is working with real files, try to use stubs or mocks.

Update:

Below is compete test file, with the separated tests

[TestClass]
public class UnitTest_Files
{
    private string expectedFile1;

    [TestMethod]
    public void SetFiles_Valid()
    {
        var fs = new Files();

        // 1st call to SetFiles. valid

        var expectedParentFolder = @"C:\work";
        var expectedFile1 = $@"{expectedParentFolder}\file1.txt";
        var expectedFile2 = $@"{expectedParentFolder}\file2.avi";
        var expectedFile3 = $@"{expectedParentFolder}\file3";

        fs.SetFiles(new string[]
            {
                expectedFile1,
                expectedFile2,
                expectedFile3
            });

        Assert.AreEqual(3, fs.FileList.Count);
        Assert.AreEqual(@"C:\work", fs.ParentFolder);
        Assert.AreEqual(expectedFile1, fs.FileList[0].Path);
        Assert.AreEqual(expectedFile2, fs.FileList[1].Path);
        Assert.AreEqual(expectedFile3, fs.FileList[2].Path);
    }

    [TestMethod]
    public void SetFiles_Specialchars_Valid()
    {
        // 2nd call to SetFiles. valid
        var expectedParentFolder = @"C:\muzică\Diabulus in Musica\poze\membri";
        var expectedfile1 = $@"{expectedParentFolder}\Zuberoa Aznárez.jpg";
        var expectedfile2 = $@"{expectedParentFolder}\Gorka Elso.jpg";

        var fs = new Files();
        fs.SetFiles(new string[]
            {
                expectedfile1,
                expectedfile2
            });

        Assert.AreEqual(expectedParentFolder, fs.ParentFolder);
        Assert.AreEqual(2, fs.FileList.Count);

        Assert.AreEqual(expectedFile1, fs.FileList[0].Path);
        Assert.AreEqual(expectedFile2, fs.FileList[1].Path);
    }

    [TestMethod]
    [ExpectedException(typeof(ArgumentException))]
    public void SetFiles_FilesNotInTheSameFolder_ExpectedException()
    {
        // 3rd call to SetFiles. Invalid

        // set to invalid structure (files not in same folder)
        // and expect throw
        // and no change

        var validParentFolder = @"c:\C:\muzică\folder1";
        var validFile1 = @"c:\C:\muzică\folder1\file1.txt";

        var invalidFile1 = @"C:\muzică\folder1\file2.txt";
        var invalidFile2 = @"C:\muzică\folder2\file2.txt";

        var fs = new Files();
        fs.SetFiles(new string[]
        {
            validFile1
        });

        fs.SetFiles(new string[]
            {
                invalidFile1,
                invalidFile2
            });

        // test that fs was not modified

        Assert.AreEqual(1, fs.FileList.Count);
        Assert.AreEqual(validParentFolder, fs.ParentFolder);

        Assert.AreEqual(validFile1, fs.FileList[0].Path);
    }
}


Find the starting indices of all occurrences of the pattern in the string The task

  Given a string and a pattern, find the starting indices of all
  occurrences of the pattern in the string. For example, given the
  string "abracadabra" and the pattern "abr", you should return [0, 7].

My solution

function findStartingIndex(T, pattern) {
  let S = T;
  const res = [];
  while(true) {
    const i = S.indexOf(pattern);
    if (i === -1) { return res; }

    S = S.substring(i + 1);
    res.push(i ? i + 1 : i);
  }
  return res;
}

Your code contains a bug.

For banana and a, it should return [1, 3, 5], but it doesn't.

Because I'm a fan of functional programming, I want to implement this without any for or while loops, which necessarily require mutating variables.

The way I'd do this, is with a recursive function.

function findIndexes(n, h, acc = [], currentIndex = 0) {
  const index = h.indexOf(n);

  if (index < 0) {
    return acc;
  } else {

    const newHaystack = h.slice(index + 1);
    return findIndexes(n, newHaystack, [...acc, index + currentIndex], currentIndex + index + 1);
  }
}

console.log(findIndexes("abr", "abracadabra"));
console.log(findIndexes("1", "1111"));
console.log(findIndexes("12", "121212"));
console.log(findIndexes("1212", "12121212")); //This one is a tricky case, as the sub strings overlap.
console.log(findIndexes("1221", "1221221221"));
console.log(findIndexes("111", "11111111111"));
console.log(findIndexes("a", "banana"));

Now, whether you want to implement that indexOf function yourself is up to you, but in any case - this how I'd do the rest of it.


Creates a menu and sidebar in Google Spreadsheets In an effort to make my code more testable, I'm trying to make use of constructors and prototypes.

Here is a working Google Apps Script (GAS) for creating a new submenu under the Addon main menu in a Google Spreadsheet. If a user clicks the 'Show' menu item, a sidebar will appear.

var UI = function (menuName, menuFunction, sidebarFile, sidebarTitle) {
    this.menuName = menuName;
    this.menuFunction = menuFunction;
    this.sidebarFile = sidebarFile;
    this.sidebarTitle = sidebarTitle;
};

UI.prototype.createAddonMenu = function () {
    try {
        SpreadsheetApp.getUi()
            .createAddonMenu()
            .addItem(this.menuName, this.menuFunction)
            .addToUi();
        Logger.log('onOpen (success): building menu');

    } catch (e) {
        Logger.log('onOpen (fail): ' +  e);
    }
};

UI.prototype.showSidebar = function () {
    try {
        var ui = HtmlService.createTemplateFromFile(this.sidebarFile)
            .evaluate()
            .setTitle(this.sidebarTitle)
            .setSandboxMode(HtmlService.SandboxMode.IFRAME);

        SpreadsheetApp.getUi()
            .showSidebar(ui);
        Logger.log('showSidebar (success): showing sidebar');

    } catch (e) {
        Logger.log('showSidebar (fail): ' + e);
    }
};

var ui = new UI('Show', 'showSidebar', 'index', 'Awesome Title');

// onOpen is a built-in Google Apps Script function that gets called when the spreadsheet is opened
function onOpen(e) {
    return ui.createAddonMenu();
}

// showSidebar is a standalone function and returns the ui showSidebar method. needs to be this way in order to be called from createAddonMenu
function showSidebar() {
    return ui.showSidebar();
}

Questions:

I'd like to be able to write unit tests for both createAddonMenu and showSidebar, but at this moment I don't believe it's possible as I'm not returning a value from either method; I'm simply just calling Google's third party code for creating a menu and showing a sidebar. Would I need to create a wrapper around any code calling a third party's code, and then call that within my method? I know I'm not supposed to test third party code, so how would I go about testing to make sure it gets called and doesn't return an error?
I'd like a sanity check on when I should create a constructor. I feel like maybe I'm trying to shove too many arguments into it and maybe trying to do too much. Should I create two separate constructors (one for createMenu and one for showSidebar)? Should I even be using constructors at all?
Finally, any other feedback?

1. Testing:

Although your functions don't return a value and they catch any error that is thrown, the functions do log whether the call was successful or not. You could use the Logger to test if they fail like so:

Logger.clear();
ui.showSidebar();
var string = Logger.getLog();
if (string.indeOf("(fail)") === -1) throw "test for 'showSidebar' failed";

Note: If your unit tests are run in the script editor some of the third party functions could throw errors that wouldn't occur when called from the spreadsheet. The ones in your current functions are all fine last I checked, but function calls like SpreadsheetApp.getActive() cannot be made from the script editor.

Writing wrapper classes would be the safest way to ensure that your functions are correct, but that assumes that your wrappers and mocks are perfectly written. You have to consider whether the work it would take to write/debug/test the wrapper and mock classes is worth the value of having unit tests for your functions--especially since there are currently only two and they are rather small.

2. OOP in Google Apps Script

Where to call your constructor

Declaring variables in the global scope is dangerous since Google doesn't offer specifics on the order that it's interpreter/compiler (who knows which) reads the code. Basically things like:

var UI = function () {}

and

UI.prototype.showSidebar = function () {};

and

var ui = new UI();

aren't guaranteed to be executed in the intended order, specifically if they're in separate script files. If you wish to dabble in this dark art (who doesn't?), please note the following:

Functions are hoisted
The rest of the code appears to be executed in the order that your script files were created

I would recommend making one of your script files a sort of "main" file; make this file the only place you preform logic on the global scope to ensure that all the global logic is executed in the intended order.

Note: Global logic is executed every time you execute a script.

Constructor and Prototype in Google Apps Scripts

The way your code creates the UI class depends on global logic. If you declare a UI instance at the global scope in a separate script file, the UI class or it's prototype might not yet be defined.

A way around this would be to use a factory function instead:

function createUI() {
    return {
        createAddonMenu : function () {},
        showSidebar : function () {}

    };
}

The factory function will be hoisted and all is well in the global scope.

To many parameters

Consider passing a single object for your constructor's parameter:

function createUI(settings) {
    var menuName = settings.menu.name;
    var menuFunction = settings.menu.action;
    // ...
}
var ui = createUI({
    menu : {
        name : "Show",
        action : "showSidebar"
    }
    sidebar : {
        // ...
    }
});

This gives names to the parameters at the caller's end, it makes it so the order of the parameters is irrelevant, and it makes optional values easier to work with since the order of other parameters won't be affected if one is omitted. When using this technique you may want to validate that all required parameters exist inside the parameter object.

There's no real need to wrap these functions. Using Object-Oriented Programming in this case would mean to me that you are expecting to potentially create multiple instances of the UI to use else where. Instead, there's only going to be 1 custom menu be created.

The sample code provided by Google in the script editor under

  Help menu >> Welcome Screen >> Google Sheets Add-on

doesn't use constructors and nowhere in official docs have I ever seen OO programming used in snippets. Other users certainly use that pattern for larger programs though.

Instead of UI wouldn't Menu be a better class name anyhow? UI is what pops up in the sidebar or modal. Now if your Add-on has the potential to serve up lots of different UI views, that may trigger the need to organize that piece in an OO manner later.

  In an effort to make my code more testable

Was there a specific issue with testing you ran into with basic functional programming that triggered the need to convert to OO? Code reuse in a large code base is typically the argument instead of testing.

  how would I go about testing to make sure it gets called

You're already using Logger.log('showSidebar (success): showing sidebar');, what is wrong with this? Manually clicking the Show menu item or manually running the showSidebar() function from within the editor will run that line.

Unless your personal preference is OO or that's the background you're coming from, this particular section of code doesn't need it, in my opinion. Functional Programming is more clear and concise for small tasks.

Disclaimer: I don't have much OO experience, but have been working with Apps Script exclusively for over 4 years via functional programming


How to find position (array index ) of a number in a given array? I have to find the position (array index) of a number in an array, the number can lie in between any other two numbers in the array. I have written code for this but I want some alternative way to do this.

example: let the array be given as

float arr[]={1.5, 3.65, 4.9, 8.4, 10.6, 17.5, 19.45, 21.86, 25.67, 30.65};

Now I have to find the position of 12.6 in the given array, since 12.6 lies between 10.6 and 17.5 in the above array so, it should return array index as 4.

Is there any other way to do this? I know it can be done by using binary search but I don't know how to implement it (since the number is not present in array)

The array is sorted (ascending order)

#include<stdio.h>
int main()
{
int position;
int i;
float num=12.6; // position of this number has to be determined i.e. it lies between 10.6
// and 17.5 in the array given below so it should give array index as 4
float arr[]={1.5, 3.65, 4.9, 8.4, 10.6, 17.5, 19.45, 21.86, 25.67, 30.65}; // any random array

for(i=0;i<10;i++)
{
if(arr[i] > num)
{
position=i-1;
break;
}
}
printf("array index is %d",position); // this gives index as 4

return 0;
}

Indent the Code

Most computer code is indented, in some languages indentation is used to show the level of complexity of the code, in other languages the code won't work properly unless it is indented.

Declare the Variables as They are Needed

The integer variable i can be declared in the for loop and that will limit the scope to just the for loop.

for(int i = 0; i < 10; i++)
{
    if(arr[i] > num)
    {
        position = i - 1;
        break;
    }
}

Horizontal Spacing

To make the code more readable, put spaces between the operators and the operands as shown above in the for loop example. Squashing all the code together makes it very difficult to understand what the code is actually doing.

Performance

First run the -O3 optimizer during compilation and linking. If the program were more complicated than it is then use a profiling tool such as gprof.

Linear searches are known to be a slower way to do things.

I think the position reported is off by one, if I was using this code to do an insertion sort, I wouldn't subtract one from position.

Shorten the Code.

There are 2 shorter ways to subtract 1 from position.

    --position;

and

    position -= 1;

Learn basics of binary search. Understand it in and out.
Now look again at your problem statement. Maybe modifying the binary search algorithm to suit your needs may just work out.(hint :see how you can use key in different ways)
Try to find out all your test cases and end points. For instance what will you get if num = 0.5?


Text-to-Morse code converter I created a morse converter from a .txt file.

I'm learning java and I created a simple morse converter. Convert only A to Z and 0 to 9 chars. Space char remain for split words in morse-code.
At the beginning I used a big switch for any letters or number but it was no good.

public static String toMorse(byte[] bytes) {
    String text = new String(bytes);
    String morseText = "";
    text = text.toLowerCase();
    String[] morseChar
            = {".-",
                "-...",
                "-.-.",
                "-..",
                ".",
                "..-.",
                "--.",
                "....",
                "..",
                ".---",
                "-.-",
                ".-..",
                "--",
                "-.",
                "---",
                ".--.",
                "--.-",
                ".-.",
                "...",
                "-",
                "..-",
                "...-",
                ".--",
                "-..-",
                "-.--",
                "--.."};
    String[] morseNumber
            = {".----",
                "..---",
                "...--",
                "....-",
                ".....",
                "-....",
                "--...",
                "---..",
                "----.",
                "-----"};

    for (int i = 0; i < text.length(); i++) {
        if (text.charAt(i) >= 'a' && text.charAt(i) <= 'z') {
            morseText = morseText + morseChar[text.charAt(i) - 'a'];
        } else if (text.charAt(i) >= '0' && text.charAt(i) <= '9') {
            morseText = morseText + morseNumber[text.charAt(i) - '0'];
        } else if (text.charAt(i) == ' ') {
            morseText = morseText + text.charAt(i);
        }
    }
    return morseText;
}

One more review, where I'd like to point out two Java pitfalls you should avoid in professional code.

byte[] array to represent text

Your public static String toMorse(byte[] bytes) { ... } method signature gets the input as byte array and only internally creates a String out of that array.

That's dangerous, as that String(byte[] bytes) constructor is documented as (emphasis mine):

  public String(byte[] bytes)

  Constructs a new String by decoding the specified array of bytes using
  the platform's default charset. [...]

That means that the same byte array will produce different results when running under different operating systems, as even nowadays there are still many different character encodings in use all over the world and all over the various operating systems.

A computational method like your morse converter should have a String argument instead of a byte[] array, then it's run-anywhere reproducible. If necessary, have your file I/O methods handle character encodings.

Rule of thumb: to pass around things that represent text, use String.

Repeated String appending

In your loop, you build the output string by repeatedly doing lines like

morseText = morseText + morseChar[text.charAt(i) - 'a'];

That's ok if you know the text is rather short, but it will become quite slow for strings with thousands of characters.

Why? Every time the machine executes such a line, it creates a completely new morseText string, copying all the characters from the old morseText, plus the additional morse snippet. So, the first morse snippets get copied over and over and over again. To avoid that, use a single StringBuilder, and in every loop iteration, append() the morse snippet to that StringBuilder, and only after the loop, convert the StringBuilder to the String you want to return.


FizzBuzz Problem Solution I wrote this simple solution about FizzBuzz. Is there any possible way to solve or optimize the solution, such as with bitwise tricks?

public class FizzBuzz{
  public static void main(String[] args){

    for(int i = 1 ; i <= 100 ; ++i){
      if(i % (5*3) == 0){
        System.out.println("FizzBuzz");
      }else if(i % 3 == 0){
        System.out.println("Fizz");
      }else if(i % 5 == 0){
        System.out.println("Buzz");
      }else
        System.out.println(i);
    }
  }
}

That's a classic FizzBuzz solution.  I wouldn't try to do any clever optimization — it won't make any difference to performance.  One hundred iterations of anything is trivial for a computer.  Furthermore, most of the time will be dominated by the output routines, which you can't do much about.

The code formatting, on the other hand, could be improved.  The most glaring issue is the omission of the braces for the final else.  Why make your code ugly to save a couple of bytes?

Also note a de facto formatting convention: Put a space after if and for, and before any opening { brace.

To actually answer your question: "Is there any possible way to [...] optimize the solution? Like bitwise tricks?"

for (int i=1;i<=100;i++) {
    int a=((528>>i%15-1)&1)<<2;
    int b=((-2128340926>>(i%15)*2)&3)<<2;
    System.out.println("FizzBuzz".substring(a,b)+(a==b?i:""));
}


Custom encoding for BinaryReader I have a file that seems to mix encoding in it. It seems like a Unicode encoded file, but the character length string is encoded like a UTF8 or similar. Here is an example:

05 41 00 72 00 69 00 61 00 6C 00
5  A  .  r  .  i  .  a  .  l  .

In this example it stores the string like Unicode, using the extra character, but the length of the string is half of what it should be, 05 instead of 0A, as if it were encoded as UTF8.

If I use:

using (var reader = new BinaryReader(File.Open(fileName, FileMode.Open), Encoding.Unicode))
{
  temp = reader.ReadString();
}

When I run this then temp = "Ar"

I have this code that works. But is there a better way?

using (var reader = new BinaryReader(File.Open(fileName, FileMode.Open), Encoding.Unicode))
{
  tempByte = reader.ReadByte();
  var length = Convert.ToInt32(tempByte) * 2;
  byteArray = reader.ReadBytes(length);
  for (var ww = 0; ww < length; ww = ww + 2)
  {
    tempString = tempString + (char)byteArray[ww];
  }
}

BinaryReader.ReadString() expects the string prefixed with the number of bytes to read, not the number of characters (I think this is because of variable-length encodings, especially UTF-8, but also UTF-16).

So, you can't use ReadString() directly, but you also don't have to convert the characters byte by byte like you do (which wouldn't work for non-ASCII characters anyway).

For this, you can use ReadChars(), which takes as a parameter the number of characters (not bytes) to read.

You also need to figure out what format is the number of bytes saved in. It could be a simple single-byte number (which means the string can have at most 255 characters), or it could be VLQ-encoded, which you can read using Read7BitEncodedInt(). Though that method is protected, so I'm going to assume the former for simplicity.

So, the code could look like this:

using (var reader = new BinaryReader(File.Open(fileName, FileMode.Open), Encoding.Unicode))
{
    int characterCount = reader.ReadByte();
    char[] characters = reader.ReadChars(characterCount);
    return new string(characters);
}

To me it looks like somebody just used BinaryWriter.Write(String) and you should be able to extract those strings with a UTF16 encoded BinaryReader using BinaryReader.ReadString() method


Very simple events implementation I've been thinking about implementing events and wrote some abstract code:

#include <memory>
#include <vector>
#include <iostream>

template<typename Event>
class Dispatcher
{
public:
    class Listener
    {
    public:
        virtual void OnEvent(Event& event, Dispatcher& sender) {};
    };

private:
    typedef std::shared_ptr<Listener> ListenerPtr;
    typedef std::vector<ListenerPtr> Listeners;
    Listeners _listeners;

public:
    void Reg(ListenerPtr listener)
    {
        if( _listeners.end() != std::find(_listeners.begin(), _listeners.end(), listener) )
            return;

        _listeners.push_back(listener);
    }

    void Unreg(ListenerPtr listener)
    {
        Listeners::iterator iter = std::find(_listeners.begin(), _listeners.end(), listener);
        if( _listeners.end() == iter )
            return;

        _listeners.erase(iter);
    }

    void Dispatch(Event& event)
    {
        for( Listeners::iterator iter = _listeners.begin(); iter != _listeners.end(); ++iter )
            (*iter)->OnEvent(event, *this);
    }
};

struct SomeEvent
{
    int someParam;
    SomeEvent(int someParam) : someParam(someParam) {}
};

class SomeDispatcher : public Dispatcher<SomeEvent>
{
};

struct OtherEvent
{
    int otherParam;
    OtherEvent(int otherParam) : otherParam(otherParam) {}
};

class OtherDispatcher :
    public SomeDispatcher, public Dispatcher<OtherEvent>
{
};

class Consumer :
    public Dispatcher<SomeEvent>::Listener,
    public Dispatcher<OtherEvent>::Listener
{
    virtual void OnEvent(SomeEvent& event, Dispatcher<SomeEvent>& sender)
    {
        std::cout << "OnEvent SomeEvent " << event.someParam << std::endl;
    }

    virtual void OnEvent(OtherEvent& event, Dispatcher<OtherEvent>& sender)
    {
        std::cout << "OnEvent OtherEvent " << event.otherParam << std::endl;
    }
};

int main(int argc, char **argv)
{
    OtherDispatcher dispatcher;

    std::shared_ptr<Consumer> consumer(new Consumer());

    dispatcher.Dispatcher<SomeEvent>::Reg( consumer );
    dispatcher.Dispatcher<OtherEvent>::Reg( consumer );

    dispatcher.Dispatcher<SomeEvent>::Dispatch(SomeEvent(1));
    dispatcher.Dispatcher<OtherEvent>::Dispatch(OtherEvent(2));

    dispatcher.Dispatcher<SomeEvent>::Unreg( consumer );

    dispatcher.Dispatcher<SomeEvent>::Dispatch(SomeEvent(3));
    dispatcher.Dispatcher<OtherEvent>::Dispatch(OtherEvent(4));

    return 0;
}

It compiles and works as expected, but... I have to write more code to inherit from Dispatcher and Listener, to register listener and to define more exactly which dispatcher I'm registering to. And this method does not allow registering of custom class method, but only OnEvent().

Apart from this, are there any issues with this code? Is it compatible with different compilers and platforms? Any suggestions?

Listner needs a virtual destructor
and the OnEvent() method should probably be pure virtual:

class Listener
{
public:
    virtual void OnEvent(Event& event, Dispatcher& sender) {};
};

// Try

class Listener
{
    public:
        virtual ~Listener() {}
        virtual void OnEvent(Event& event, Dispatcher& sender) = 0;
};

You need the virtual destructor because delete will more than likely be called via a pointer to the base class (Listener) at some point.

No point in implementing OnEvent() to do nothing if you need sombody to over-ride it.

Personally I would prefer to use boost::ptr_vector<> rather than a std::vector<std::shared_ptr<>>. The boost vector has a couple of advantages (less lcoks required). Also its members are provided via references rather than pointers so it makes it more natural to use with the standard algorithms (not such a big deal in C++11 (with lambdas) but it makes C++03 much easier to use).

typedef std::shared_ptr<Listener> ListenerPtr;
typedef std::vector<ListenerPtr> Listeners;
Listeners _listeners;

// Replace with:

boost::ptr_vector<Listener>  listeners;

Be careful of using '_` on the front of identifiers. The rules are not obvious so best avoided to make sure you don't actually break them. Underscore Rules

Personally not a fan of the following:

   if( _listeners.end() != std::find(_listeners.begin(), _listeners.end(), listener) )

Nothing wrong with it. But reversing the order of the test just makes you pause a second. See Yoda Conditionals.

Prefer to use standard algorithms rather than hand coded loops.

   for( Listeners::iterator iter = _listeners.begin(); iter != _listeners.end(); ++iter )
        (*iter)->OnEvent(event, *this);

   // Try
   std::for_each(listeners.begin(), listeners.end(),
                 std::bind2nd(Listner::OnEvent, event)
                ); // Assume boost::ptr_vector<>

   // Or go all C++11 on us
   std::for_each(std::begin(listeners), std::end(listeners),
                 [&event](std::shared_ptr<Listener>& lis) {lis->OnEvent(event);}
                );

If you want to have listeners that only deal with a specific type of event. You can write your Listner to be a filtering lsitener.

class Listener
{
public:
    virtual void OnEvent(Event& event, Dispatcher& sender) {};
};

Change to:

template<class ForEvent>
class Listener
{
    public:
        ~Listener() {}

        // This is the method called by the dispatcher.
        void eventFilter(OnEvent& event, Dispatcher& sender)
        {
             // dynamic_cast to a pointer type.
             // Result is NULL if not the correct type.
             // If you used a value type then it would throw an exception.
             ForEvent* specialEvent = dynamic_cast<ForEvent*>(&event);
             if (specialEvent != NULL)
             {
                 // Only call the OnEvent for the types we want.
                 // put back to a references (so OnEvent does not need to
                 // check for NULL).
                 this->OnEvent(*specialEvent, sender);
             }
        }

        virtual void OnEvent(ForEvent& event, Dispatcher& sender) = 0;
};

So, I think the following scenario will break the otherwise elegant Dispatcher class:

The consumer removes itself from the Dispatcher within the OnEvent() notification.

Suppose the example were a single shot timer expiration and the consumer wants to remove itself from notifications once the timer it is interested in expires.


Shuffler with generics for C# This is something I am always asked by managers and clients : "make it random". More often than not, this isn't what they want.

Comments like "I had the same element twice in a row !" and "I don't see lement X often enough !" are common, even if they are part of the "true random".

I came up with the following solution using generics. It's basically a Urn with a safeguard so the last item isn't picked after a refill. I'm looking for ways to improve it. Is it readable ? Is it efficient enough ?

using System;
using System.Collections;
using System.Collections.Generic;

namespace Utils
{
    /// <summary>
    /// Implements "iTunes shuffle" that is to say, it's a random where you can't get twice the same value unless you
    /// already got all the values.
    ///
    /// To avoid having a chance to get twice the same value when refilling the pool, we store the last picked value in
    ///  `_last` and do not put it back when refilling the pool
    ///
    /// author: ray.hubert@riseup.net
    /// </summary>
    public class Shuffler<T> : IEnumerable
    {
        private readonly List<T> _allItems = new List<T>(); // All items used in the shuffler
        private readonly List<T> _pool = new List<T>(); // Remaining items for current series
        private T _last; //Last object extracted from the pool
        private Random _random = new Random();

        private int PoolCount
        {
            get { return _pool.Count; }
        }

        public Shuffler()
        {
        }

        public Shuffler(ICollection<T> allItems)
        {
            if (allItems.Count == 0) throw new ArgumentException("ERROR: initialization list cannot be empty !");
            _allItems.AddRange(allItems);
            PopulatePool();
        }

        public T GetNext()
        {
            if (IsPoolEmpty())
            {
                PopulatePool();
            }
            return PopRandomElement();
        }

        private T PopRandomElement()
        {
            if (PoolCount == 0) throw new ArgumentException("ERROR: initialization list cannot be empty !");
            int randomIndex = _random.Next(PoolCount);
            T item = _pool[randomIndex];
            _pool.RemoveAt(randomIndex);
            if (IsPoolEmpty()) _last = item;
            return item;
        }

        private void PopulatePool()
        {
            _pool.Clear();
            _pool.AddRange(_allItems);
            if (PoolCount > 1) // If _allItems is size 1 we don't remove from the pool : it would be empty
            {
                RemoveLastFromPool();
            }
        }

        private void RemoveLastFromPool()
        {
            _pool.Remove(_last);
            _last = default(T);
        }

        public List<T> GetAllPossibleValues()
        {
            return new List<T>(_allItems);
        }

        private bool IsPoolEmpty()
        {
            return PoolCount == 0;
        }

        public IEnumerator GetEnumerator()
        {
            while (PoolCount > 0)
            {
                yield return PopRandomElement();
            }
        }

        public void Add(T element)
        {
            _allItems.Add(element);
            PopulatePool();
        }
    }
}

Sample usage #1 :

// Loops on all alements and stop
Shuffler<int> shuffler = new Shuffler<int> {1, 2, 3, 4, 5, 6, 7, 8, 9, 0};
foreach (int number in shuffler)
{
    Console.Write("Random element : " + number);
}

Sample usage #2 :

// Loops on all elements for ever
Shuffler<string> shuffler = new Shuffler<string> {"Alice", "Bob", "John", "Bill", "Scrooge"};
bool gameIsPlaying = true;
while (gameIsPlaying)
{
    Console.Write("Random element : " + shuffler.GetNext());
}

Points I'm wanting to improve :

The empty constructor seems necessary for the Add() function to work properly (see usage)
Throwing an exception in two code path is suboptimal, I'd rather have a single place to throw it
Readability, "code-cleanness"

public class Shuffler<T> : IEnumerable

Why not IEnumerable<T>??!

        private readonly List<T> _allItems = new List<T>(); // All items used in the shuffler

The types in this line raise two red flags in my mind.

Code to the interface, not the implementation. The type of the field per se should be an interface: maybe IList<T>, maybe ICollection<T>, or maybe even something else.
With respect to maybe even something else: should it be ISet<T>? The explanation given for the use case suggests strongly that elements should not be repeated, because if they are repeated it's going to be complicated to avoid playing the same track twice in a row.

        private readonly List<T> _pool = new List<T>(); // Remaining items for current series

Again, code to the interface. Here IList<T> is justified because the ability to select by index is important for performance.

        private T _last; //Last object extracted from the pool

If "last" here means "most recent" then the methods further down seem buggy.

        private int PoolCount
        {
            get { return _pool.Count; }
        }

Are you deliberately avoiding the arrow syntax?

        private int PoolCount => _pool.Count;

is going to become increasingly common.

        public Shuffler(ICollection<T> allItems)
        {
            if (allItems.Count == 0) throw new ArgumentException("ERROR: initialization list cannot be empty !");
            _allItems.AddRange(allItems);
            PopulatePool();
        }

I concur with VisualMelon's observations on this constructor: there's no reason to prohibit empty inputs, there's no need to populate the pool yet, and there's no reason to require ICollection<T> rather than IEnumerable<T>.

        public T GetNext()
        {
            if (IsPoolEmpty())
            {
                PopulatePool();
            }
            return PopRandomElement();
        }

vs

        public IEnumerator GetEnumerator()
        {
            while (PoolCount > 0)
            {
                yield return PopRandomElement();
            }
        }

Firstly, I find this API very confusing. It would make more sense to me if both methods ran forever, repopulating the pool if it runs dry.

Secondly, IsPoolEmpty() vs PoolCount > 0 is inconsistent. IMO factoring out IsPoolEmpty() is overengineering: I would inline it.

        private T PopRandomElement()
        {
            if (PoolCount == 0) throw new ArgumentException("ERROR: initialization list cannot be empty !");
            int randomIndex = _random.Next(PoolCount);
            T item = _pool[randomIndex];
            _pool.RemoveAt(randomIndex);
            if (IsPoolEmpty()) _last = item;
            return item;
        }

You asked

  Is it efficient enough ?

Enough depends on your requirements, but the answer to the question "Is it as efficient as it could be?" is "No". _pool.RemoveAt has to shuffle along every later element in the list. In the worst case the random number generator will return a long string of 0. The standard solution is a swap.

        private T PopRandomElement()
        {
            System.Diagnostics.Debug.Assert(PoolCount > 0);
            int randomIndex = _random.Next(PoolCount);
            _last = _pool[randomIndex];
            _pool[randomIndex] = _pool[PoolCount - 1];
            _pool.RemoveAt(PoolCount - 1);
            return _last;
        }

This now takes constant time rather than linear time in the size of the pool.

        private void PopulatePool()
        {
            _pool.Clear();
            _pool.AddRange(_allItems);
            if (PoolCount > 1) // If _allItems is size 1 we don't remove from the pool : it would be empty
            {
                RemoveLastFromPool();
            }
        }

        private void RemoveLastFromPool()
        {
            _pool.Remove(_last);
            _last = default(T);
        }

Again, a linear removal could be replaced by a swap and a constant time removal.

But I wonder whether this actually meets the spec. Rather than removing _last from the pool entirely, didn't you just want to guarantee that it won't be the first element picked after repopulating the pool?

In fact, I'd be inclined to take this further. Suppose we refactor so that PoolCount is backed by its own field rather than _pool.Count, and that we have an additional field (or field-backed property) DelayCount.

If we replace _pool.RemoveAt(PoolCount - 1); with PoolCount--; then when PoolCount gets to 0, the pool contains the entire play list in reverse order of when they were last played. Now

PoolCount = _pool.Count;
DelayCount = N;
for (int i = 0; i < DelayCount; i++)
{
    var tmp = _pool[i];
    _pool[i] = _pool[PoolCount - DelayCount + i];
    _pool[PoolCount - DelayCount + i] = tmp;
}

swaps the first N elements up to the end. Adjust the PopRandomElement method a bit more:

        private T PopRandomElement()
        {
            System.Diagnostics.Debug.Assert(PoolCount > 0);
            System.Diagnostics.Debug.Assert(PoolCount > DelayCount);

            int randomIndex = _random.Next(PoolCount - DelayCount);
            var tmp = _pool[randomIndex];
            _pool[randomIndex] = _pool[PoolCount - 1];
            _pool[PoolCount - 1] = tmp;

            PoolCount--;
            if (DelayCount > 0) DelayCount--;

            return tmp;
        }

The effect is to guarantee that no track will repeat within N of the previous time it occurred. You can then make N settable with a public property or derive it from _pool.Count according to some hard-coded policy (e.g. a fixed percentage).

        public List<T> GetAllPossibleValues()
        {
            return new List<T>(_allItems);
        }

Why List<T>? In fact, is this method even useful?

        public void Add(T element)
        {
            _allItems.Add(element);
            PopulatePool();
        }

I don't see why it's desirable to populate the pool here. In addition to VisualMelon's observations, by populating the pool before reaching the end you lose the guarantee not to play the same track twice.

Add(T element) calls PopulatePool();.  That is kind of expensive.  How about just add to both List?

You could have an index on _pool and not have to remove.

   int randomIndex = _random.Next(poolIndex);
   T item = _pool[randomIndex];
   _pool[randomIndex] = _pool[poolIndex - 1];
   poolIndex--;

This basically the fisher yates shuffle on the fly

if (IsPoolEmpty()) _last = item; doe not need if (IsPoolEmpty())
In fact you don't want it to cover for Add

This can be done with less moving parts
I may have trimmed too much

public class Shuffler<T> : IEnumerable
{
    private readonly List<T> _pool = new List<T>(); // Remaining items for current series
    private int _poolIndex = -1;
    private Random _random = new Random();

    public Shuffler()
    {
    }
    public Shuffler(ICollection<T> allItems)
    {
        if (allItems.Count == 0) throw new ArgumentException("ERROR: initialization list cannot be empty !");
        _pool.AddRange(allItems);
        _poolIndex = _pool.Count();
    }

    public T GetNext()
    {
        return PopRandomElement();
    }

    private T PopRandomElement()
    {
        int randomIndex = _random.Next(_poolIndex);
        T item = _pool[randomIndex];
        _pool[randomIndex] = _pool[_poolIndex - 1];
        _poolIndex--;
        if (_poolIndex == 0)
        {
            _poolIndex = _pool.Count();  //this is basically the shuffle
            if(item != null && _poolIndex > 1)
            {
                _pool.Remove(item);
                _pool.Add(item);
                _poolIndex--;  //takes it out of rotation
            }
        }
        return item;
    }

    public List<T> GetAllPossibleValues()
    {
        return new List<T>(_pool);
    }

    public IEnumerator GetEnumerator()
    {
        yield return PopRandomElement();
    }

    public void Add(T element)
    {
        _pool.Insert(_poolIndex - 1, element);
        _poolIndex++;
        //immediately goes into the rotation
    }
}


Enumerate all items to find those that match a complex condition Currently, I have a big for..loop with many if..else inside the loop. I just wonder is there any design pattern or other approach to reduce the long code inside the for..loop. Thank in advance.

foreach ($workingTimes as $hour => $minutes) {
    foreach ($minutes as $shift) {
        $isBookable = $this->isBookabe($hour, $shift);

        if(!$isBookabe) {
            continue;
        }

        $isNotEnoughEmployee = $this->checkEmployees($hours, $shift);

        if(!$isNotEnoughEmployee) {
            continue;
        }

        $isNotEnoughResources = $this->checkResources($hour, $shift);

        if(!$isBookabe) {
            continue;
        }

        $isNotEnoughRooms = $this->checkRooms($hour, $shift);

        if(!$isNotEnoughRooms) {
            continue;
        }

        $formatted = sprintf("%s-%s", $hour, $shift);
        $timetable[$formatted] = 'Bookable';
    }
}

You can't reduce code you wrote because each condition has to be checked and you're already using variables to clarify your intent. What you can do is to move all that code out of your loop, reading that code in future you won't need to scroll over all those ifs:

foreach ($workingTimes as $hour => $minutes) {
    foreach ($minutes as $shift) {
        if (!canBookRoom($hour, $shift))
            continue;

        $formatted = sprintf("%s-%s", $hour, $shift);
        $timetable[$formatted] = 'Bookable';
    }
}

...

function canBookRoom($hour, $minute) {
    if (!isThereAvailableRoom($hour, $minute))
        return false;

    return areThereAvailableResources($hour, $minute);
}

function isThereAvailableRoom($hour, $minute) {
    if (!$this->isBookable($hours, $shift))
        return false;

    return $this->checkRooms($hour, $shift);
}

function areThereAvailableResources($hour, $minute) {
    if (!$this->checkEmployees($hour, $shift))
        return false;

    return $this->checkResources($hour, $shift);
}

What has been done

Reading your loop you're not really interested in all details about availability, there you just need to know if it's bookable or not. Pulling out a function for that makes your code easier to parse and your intent explicit. If/when you'll be interested in that details you may browse to canBookRoom() function.
canBookRoom() function itself explicitly split booking availability in two logic domains: room availability and resources availability. Each one has its own function.
areThereAvailableResources() unifies resources, from a higher level point of view both humans and materials are resources you can count.
Your local variables has been dropped, if each function name is self descriptive then you don't need them.

What should be done

Your loop is still doing many things: it searches for a bookable room inside a time range and if it finds an available time then it marks that range as bookable.

At least something should be done in a separate function. Here you have two options for your hypothetical buildTimetableForAllBookableRooms() function: refactor out function to fill timetable or add one function findAllBookableRooms() which returns all available time frames when there is a bookable room plus another function formatTimetableForBookableRoom() to perform booking, called in a foreach loop.

function findAllBookableRooms() {
    foreach ($workingTimes as $hour => $minutes) {
        foreach ($minutes as $shift) {
            if (canBookRoom($hour, $shift))
                yield sprintf("%s-%s", $hour, $shift);
        }
    }
}

function formatTimetableForBookableRoom() {
    foreach (findAllBookableRooms() as $bookableTimeFrame) {
        $timetable[$bookableTimeFrame] = 'Bookable';
    }
}

Here I used generator syntax but you may also return an array. Note that findAllBookableRooms() may also return unformatted value using yeld array($hour, $shift);.

Wherever it will be your $formatted variable has a poor name. It should describe what it contains otherwise you may simply drop it).
Another thing to do (but I don't have enough context) is to rename isBookable() and checkRooms() functions, from name alone it's not clear what they're actually doing and why/how they're different. Note I renamed isBookabe() to isBookable(), I suppose it's just a typo.

Combine assignment with control statement evaluation

Here's how you can reduce the size of your code slightly, but you cannot reduce control statements in a for loop where the whole idea is to evaluate the logic for each of those for loop items, as it seems to be here.

foreach ($workingTimes as $hour => $minutes) {
    foreach ($minutes as $shift) {
        if(!$isBookable = $this->isBookabe($hour, $shift)) continue;

        if(!$isNotEnoughEmployee = $this->checkEmployees($hours, $shift) continue;

... and so on ...

        $formatted = sprintf("%s-%s", $hour, $shift);
        $timetable[$formatted] = 'Bookable';
    }
}

Shorter variable names

Also it seems like all your variable names are a bit longer than they need to be. For example, why not $bookable rather than $isBookable and $notEnoughEmployees rather than $isNotEnoughEmployees? The is at the start of every variable name does not add information.

Single for loop

You might consider generating an array of all possible times with hours and minutes combined rather than doing a double for loop. This way in future if you want to limit certain combinations (for example you can have 17:30 but not 18:30) this would be easier to accomplish.

Use encapsulation and functions

I entirely agree with @AdrianoRepetti's commends about better encapsulation and use of functions to signal to code readers (including your future self) what your code is really trying to do.


Iteritems on a slice of a Python list iteritems on a dict can useful.

Occasionally iteritems is useful for a slice of a list and this effect can be (crudely) implemented as follows:

class List(list):
  def iteritems(self, slice=None):
    if slice is None: return enumerate(self)
    else: return itertools.izip(range(*slice.indices(len(self))), self[slice])

if __name__ == "__main__":
  l=List("hAnGtEn")
  print l
  print list(l.iteritems())
  print list(l.iteritems(slice(1,None,2)))

Output:

['h', 'A', 'n', 'G', 't', 'E', 'n']
[(0, 'h'), (1, 'A'), (2, 'n'), (3, 'G'), (4, 't'), (5, 'E'), (6, 'n')]
[(1, 'A'), (3, 'G'), (5, 'E')]

Is there a more "pythonic" list slicing syntax that should be used?

This:

range(slice.start,slice.stop,slice.step)

does not handle certain special cases very well: e.g. where stop=-1, start=None or step=None. How can the example range/slice implementation be also improved?

edit:

range(slice.start,slice.stop,slice.step)

is better handled with:

range(*slice.indices(len(self)))

Instead of

range(slice.start,slice.stop,slice.step)

you could use this expression that handles the special cases too

range(len(self))[slice]

(This works with range on both Python 2 and 3, but not with Python 2 xrange even though it is mostly equivalent to Python 3 range)


Linked list with deep copy constructor I've created a linked list template class which contains a copy constructor that performs a deep copy of the list.

LinkedList.hpp

template <class T>
class LinkedList {
protected:
    struct Node {
        T value;
        Node *next;
        Node(T val, Node *nxt=NULL) {
            value = val;
            next = nxt;
        }
    };
    Node *head;
public:
    LinkedList();
    LinkedList(LinkedList &obj);
    ~LinkedList();

};

LinkedList.cpp

template <class T>
LinkedList<T>::LinkedList() {
    head = NULL;
}

template <class T>
LinkedList<T>::LinkedList(LinkedList &obj) {
    if (obj.head == NULL) {
        head = NULL;
    }
    else {
        head = new Node(obj.head->value);
        Node *current = head;
        Node *objHead = obj.head;
        Node *currentObj = objHead;
        while (currentObj->next != NULL) {
            current->next = new Node(currentObj->next->value);
            currentObj = currentObj->next;
            current = current->next;
        }
    }
}

template <class T>
LinkedList<T>::~LinkedList() {
    Node *current = head;
    while (current != NULL) {
        Node *garbage = current;
        current = current->next;
        delete garbage;
    }
}

template class LinkedList<int>;
template class LinkedList<double>;
template class LinkedList<bool>;
template class LinkedList<string>;

From what I can tell, the default copy constructor implements this functionality naturally, but I wanted to see if I could create a deep copy of a linked list myself using a self-defined copy constructor.

When I test this using:

LinkedList<int> ll;
ll.add(1);
ll.add(2);
ll.add(3);
ll.add(4);

LinkedList<int> ll2 = ll;
ll.add(5);

ll.displayList();
ll2.displayList();

It prints 1,2,3,4,5 and then 1,2,3,4.  I believe this is correct.  Are there any improvements I can make to the implementation of this?

A few fairly obvious points:
Prefer nullptr over NULL
Unless you really need to support older compilers, you're generally better off using nullptr in new code. If you do use nullptr, you can still support older compilers if really necessary--it's basically just a class that provides conversions for pointer to T and pointer to member.
Prefer member initializer lists over assignment in a constructor
The most obvious example here would be to replace your node constructor:
Node(T val, Node *nxt=NULL) {
    value = val;
    next = nxt;
}

...with one using a member initializer list:
Node(T value, Node *next = nullptr) :
    value(value),
    next(next)
{}

Note that yes, in this case, although the parameter being used to initialize it have the same name, but the compiler can keep track of which is which anyway.
Rule of three/five/zero
Note that in virtually every case, if you provide a destructor and a copy ctor, you also want a copy assignment operator. If you want the better efficiency you can get from move construction/move assignment available in C++11 and later, you'll probably want to add overloads of those that accept rvalue references. I consider it open to debate whether the rule of zero really applies well in this case--I've written a linked list that used smart pointers, but I was never entirely happy with it. It's clumsy but workable in the case of a singly linked list. For a doubly linked list, it's...basically unmanageable.
Side Note
Contrary to your statement, if you don't include a copy ctor, you probably won't get correct results with this sort of class. In particular, the compiler will generate code that does a shallow copy, so only the head pointer in the LinkedList object will be copied. This means if you copy a linked list, you'll end up with both pointing to a single head node. In itself that's not necessarily a problem, but when either of those is destroyed, all the nodes in the list will be destroyed, so in essence you've just destroyed both linked lists, not just the one you intended to. Attempting to use or destroy the one that wasn't supposed to have been destroyed yet will give undefined behavior.

Template definitions belong in headers
I suspect your test is also in LinkedList.cpp, yes? This'll prove to be a headache in the future. Definitions as part of a template class need to be available during object file generation, which is only possible if the definition is available in the same translation unit, such as by an included header file.
Prefer modern conventions
Assuming you're using a modern compiler, the C++ 11 standard should be the default unless you've (for some reason) specified otherwise. This has implications to your use of pointers.

In modern code, prefer nullptr to NULL.
Prefer smart pointers (such as std::unique_ptr) to raw pointers. This will add a measure of exception-safety to your copy constructor ---for those occassions in the future that you might use this template for non-primitive types--- and eliminates the need to define a custom dtor.

LinkedList.hpp
#include <iostream>
#include <memory>

template <class T>
class LinkedList {
protected:
    struct Node {
        T value;
        std::unique_ptr<Node> next;

        Node(const T& val, std::unique_ptr<Node>&& nxt=nullptr) {
            value = val;
            next = std::move(nxt);
        }

        Node(T&& val, std::unique_ptr<Node>&& nxt=nullptr) {
            value = std::move(val);
            next = std::move(nxt);
        }
    };

std::unique_ptr<Node> head;
std::unique_ptr<Node>* tail = &head;

public:
    LinkedList() = default;
    LinkedList(LinkedList& obj);
    LinkedList(LinkedList&& ) = default;
    ~LinkedList() = default;

    void add(const T& element);
    void add(T&& element);

    void displayList();
};

template<class T>
void LinkedList<T>::add(const T& element){
   *tail = std::make_unique<Node>(element);
   tail = &((**tail).next);
}

template<class T>
void LinkedList<T>::add(T&& element){
   *tail = std::make_unique<Node>(std::move(element));
   tail = &((**tail).next);
}

template<class T>
void LinkedList<T>::displayList(){
   Node* node = head.get();
   while(node){
       std::cout << node->value;
       if (node->next) std::cout << ", ";
       node = node->next.get();
   }
   std::cout << std::endl;
}

template<class T>
LinkedList<T>::LinkedList(LinkedList& obj){
   Node* node = obj.head.get();
   while(node){
       *tail = std::make_unique<Node>(node->value);
       node = node->next.get();
       tail = &((**tail).next);
   }
}

Live Example
As an aside, there are a methods listed in your test code not supported by the posted implementation. In the future, either exclude references to those methods from your example or include the relevant source.


Printing hex dumps for diagnostics I have developed some free standing functions to provide hex dumps for buffer (std::string) contents.

You can see it working in action here.

The purpose is for debugging diagnostics to print buffer contents to a (logging) stream, in a format similar as wireshark does.

Any proposals for improvements are welcome.

Here's the code:

#include <iostream>
#include <iomanip>
#include <cctype>

std::ostream& render_printable_chars(std::ostream& os, const char* buffer, size_t bufsize) {
    os << " | ";
    for (size_t i = 0; i < bufsize; ++i)
    {
        if (std::isprint(buffer[i]))
        {
            os << buffer[i];
        }
        else
        {
            os << ".";
        }
    }
    return os;
}

std::ostream& hex_dump(std::ostream& os, const uint8_t* buffer, size_t bufsize, bool showPrintableChars = true)
{
    auto oldFormat = os.flags();
    auto oldFillChar = os.fill();

    os << std::hex;
    os.fill('0');
    bool printBlank = false;
    size_t i = 0;
    for (; i < bufsize; ++i)
    {
        if (i % 8 == 0)
        {
            if (i != 0 && showPrintableChars)
            {
                render_printable_chars(os, reinterpret_cast<const char*>(&buffer[i] - 8), 8);
            }
            os << std::endl;
            printBlank = false;
        }
        if (printBlank)
        {
            os << ' ';
        }
        os << std::setw(2) << std::right << unsigned(buffer[i]);
        if (!printBlank)
        {
            printBlank = true;
        }
    }
    if (i % 8 != 0 && showPrintableChars)
    {
        for (size_t j = 0; j < 8 - (i % 8); ++j)
        {
            os << "   ";
        }
        render_printable_chars(os, reinterpret_cast<const char*>(&buffer[i] - (i % 8)), (i % 8));
    }

    os << std::endl;

    os.fill(oldFillChar);
    os.flags(oldFormat);

    return os;
}

std::ostream& hex_dump(std::ostream& os, const std::string& buffer, bool showPrintableChars = true)
{
    return hex_dump(os,reinterpret_cast<const uint8_t*>(buffer.data()), buffer.length(),showPrintableChars);
}

Here's the test case:

int main()
{
    const char test[] = "abcdef123456\0zyxwvu987654";
    std::string s(test,sizeof(test));

    hex_dump(std::cout, s);

    return 0;
}

The output looks like expected:

61 62 63 64 65 66 31 32 | abcdef12
33 34 35 36 00 7a 79 78 | 3456.zyx
77 76 75 39 38 37 36 35 | wvu98765
34 00                   | 4.

Did you ever test it with buffers which are multiples of 8 long?
Because the last block won't get the nice printable display.
Are you sure the newline at the beginning of the output should be there?
Should the test-output really contain the implicit 0-terminator?
return 0; is implicit for main().

Modified test-function:

int main()
{
    const char test[] = "abcdef123456\0zyxwvu987654";
    std::string s(test, sizeof(test) - 1);

    std::cout << "---\n";
    hex_dump(std::cout, s);
    std::cout << "---\n";
    hex_dump(std::cout, "");
    std::cout << "---\n";
    hex_dump(std::cout, "12345678");
    std::cout << "---\n";
    hex_dump(std::cout, "1234567812345678");
    std::cout << "---\n";
}

Do you have the opening brace for functions on its own line or not? Seems 2+1 for, 1 against...
If you only want to output a single character, using a character is potentially more efficient than a string.
The conditional operator is excellent for choosing between two values.
Beware of the implementation-defined signedness of plain char. The character-classification-functions inherited from C expect the value of an unsigned char or EOF.
See std::isprint.
Consider marking internal functions static to avoid externally visible symbols and promote inlining.
If you don't actually ever use a return-value, why provide one?

static void render_printable_chars(std::ostream& os, const char* buffer, size_t bufsize)
{
    os << " | ";
    for (size_t i = 0; i < bufsize; ++i)
        os << (std::isprint((unsigned char)buffer[i]) ? buffer[i] : '.');
}

Don't play around with a useless flag just because. Tracing data-flow is quite a lot more involved than following the program-flow.
Testing whether a variable already has a value before setting it to the same is rarely anything but obfuscation and pessimisation.
Don't use std::endl unless you really want to flush. And if that's your intention, consider being more explicit by using std::flush.
If you use C++17, you might want to accept a std::string_view by value instead of a std::string by constant reference. It's generally more efficient.

Before I met this post I've created my own implementation (live example):

// This template accepts a std::pair of iterators with value_type of size 1 byte.
template <typename T, typename Iter>
std::enable_if_t<
   sizeof( typename std::iterator_traits<Iter>::value_type )== 1, //std::is_same_v< typename std::iterator_traits<Iter>::value_type, char >,
   std::basic_ostream<T>
> &operator<<(std::basic_ostream<T> &os, std::pair<Iter, Iter> beginend)
{
   auto prev_os_format = os.flags();
   auto prev_os_fill = os.fill();

   static_assert(sizeof(typename std::iterator_traits<Iter>::value_type) == 1); // Available fot bytes only
   os << std::setw(2) << std::setfill('0') << std::hex << std::uppercase;
   using namespace std;
   string ascii;
   auto const &[begin, end] = beginend;
   auto iter = begin;
   const char *newline = ""; // append \n at the begining of every line except first one, not at the end.
   while (iter != end)
   {
      os << setw(0) << newline;
      unsigned short offset = iter - begin;
      os << "0x" << setfill('0') << setw(4) << std::right << unsigned(offset) << ": ";

      auto const line_end = iter + 16;
      for( auto const line_part : {iter+8,line_end} ){
         while (iter != line_part && iter != end)
         {
            ascii += isprint(*iter) ? *iter : '.';
            os << setw(2) << unsigned(*iter) << " ";
            ++iter;
         }
         os << " ";
      }
      // align ascii representation in last line
      for (int i = 0; i < line_end - iter; ++i)
         os << "   ";

      os << " |" << setfill(' ') << setw(16) << std::left << ascii << "|";
      ascii = "";
      newline = "\n";
   }

   os.flags(prev_os_format);
   os.fill(prev_os_fill);
   return os;
}

And usage:

#include <algorithm>
#include <sstream>
#include <iostream>
#include <iterator>
#include <iomanip>
#include <string_view>
#include <string>
#include <vector>

int main()
{
   using namespace std;

   constexpr std::string_view sv0 = "hello\x02";
   cout << pair{begin(sv0), end(sv0)} << endl
        << endl;

   constexpr std::string_view sv1 = "hello world\x02khgavsd \x0B \x0A\x05Xasjhlasbdas jalsjdn\x13xa0";
   std::vector<char> v1 {begin(sv1),end(sv1)};  // will fail for std::vector<int>
   cout << pair{begin(v1), end(v1)} << endl
        << endl;

   constexpr std::string_view sv2 = "hello world\x02khgavsd \x0B \x0A\x05Xasjhlasbdas jalsjdn\x13  012345678asfd.hjbelfjdvn;kqewjnfd;lijvnbqe;jraf v;kqhjewrsljhfdbvi;jekbner;ifbsdvpi[ubep[ibuvqrub[iuqeb[iuabivwequniuweniupni]]]] ;afkdjvbnqe'orjnfavi;pjqerbipjvbqei[jrbwv[ipbqreo[iuwvfb[ioqeruwbvo[iubqrio[evbwsd[uibrqefp[iadubwerip[bvp[ieqwrubvipube9";
   cout << pair{begin(sv2), end(sv2)} << endl
        << endl;
}

and output

❯❯❯ c++ -std=c++17 ./test-str-hex.cpp && ./a.out
0x0000: 68 65 6C 6C 6F 02                                  |hello.          |

0x0000: 68 65 6C 6C 6F 20 77 6F  72 6C 64 02 6B 68 67 61   |hello world.khga|
0x0010: 76 73 64 20 0B 20 0A 05  58 61 73 6A 68 6C 61 73   |vsd . ..Xasjhlas|
0x0020: 62 64 61 73 20 6A 61 6C  73 6A 64 6E 13 78 61 30   |bdas jalsjdn.xa0|

0x0000: 68 65 6C 6C 6F 20 77 6F  72 6C 64 02 6B 68 67 61   |hello world.khga|
0x0010: 76 73 64 20 0B 20 0A 05  58 61 73 6A 68 6C 61 73   |vsd . ..Xasjhlas|
0x0020: 62 64 61 73 20 6A 61 6C  73 6A 64 6E 13 20 20 30   |bdas jalsjdn.  0|
0x0030: 31 32 33 34 35 36 37 38  61 73 66 64 2E 68 6A 62   |12345678asfd.hjb|
0x0040: 65 6C 66 6A 64 76 6E 3B  6B 71 65 77 6A 6E 66 64   |elfjdvn;kqewjnfd|
0x0050: 3B 6C 69 6A 76 6E 62 71  65 3B 6A 72 61 66 20 76   |;lijvnbqe;jraf v|
0x0060: 3B 6B 71 68 6A 65 77 72  73 6C 6A 68 66 64 62 76   |;kqhjewrsljhfdbv|
0x0070: 69 3B 6A 65 6B 62 6E 65  72 3B 69 66 62 73 64 76   |i;jekbner;ifbsdv|
0x0080: 70 69 5B 75 62 65 70 5B  69 62 75 76 71 72 75 62   |pi[ubep[ibuvqrub|
0x0090: 5B 69 75 71 65 62 5B 69  75 61 62 69 76 77 65 71   |[iuqeb[iuabivweq|
0x00A0: 75 6E 69 75 77 65 6E 69  75 70 6E 69 5D 5D 5D 5D   |uniuweniupni]]]]|
0x00B0: 20 3B 61 66 6B 64 6A 76  62 6E 71 65 27 6F 72 6A   | ;afkdjvbnqe'orj|
0x00C0: 6E 66 61 76 69 3B 70 6A  71 65 72 62 69 70 6A 76   |nfavi;pjqerbipjv|
0x00D0: 62 71 65 69 5B 6A 72 62  77 76 5B 69 70 62 71 72   |bqei[jrbwv[ipbqr|
0x00E0: 65 6F 5B 69 75 77 76 66  62 5B 69 6F 71 65 72 75   |eo[iuwvfb[ioqeru|
0x00F0: 77 62 76 6F 5B 69 75 62  71 72 69 6F 5B 65 76 62   |wbvo[iubqrio[evb|
0x0100: 77 73 64 5B 75 69 62 72  71 65 66 70 5B 69 61 64   |wsd[uibrqefp[iad|
0x0110: 75 62 77 65 72 69 70 5B  62 76 70 5B 69 65 71 77   |ubwerip[bvp[ieqw|
0x0120: 72 75 62 76 69 70 75 62  65 39                     |rubvipube9      |


Can my responsive-tables JS code be made more performant? I was using dataTables.net's responsive plugin before this, and am happy to say that my (much simpler) code runs about 5 times quicker than that did.  On my machine the fiddle averages about 50ms versus 250ms if I use DT responsive.

Is there anything I should do to improve performance of my code? Can it be made better?  I'm FAR from a JS expert, as I'm sure it shows.  I'm very happy to drop jQuery if it can be done better without!

Here's a working JS fiddle.

function initResponsiveTables() {
    //does this enhance performance at all?
    'use strict';

    //get an array of <th> elements
    var th = $('th');

    //initialise in top scope
    var priorities = [];

    //iterate over the <th>s, extracting their column priorities
    th.each(function (index) {
        var priority = th[index].getAttribute('data-priority');

        //TODO: this seems very messy, I feel there must be a cleaner way?
        if (priorities[priority] === undefined) {
            priorities[priority] = index;
        } else {
            priorities[priority] += ',' + index;
        }
    });

    //clean up the array so we have consecutive indices
    priorities = priorities.filter(function (x) {
        return x !== undefined && x !== null;
    });

    //initialise all of these outside of the loop
    var sCols = '';
    var aCols = [];
    var aSelectors = [];
    var sSelectors = '';
    var i;

    //while the table is wider than the window, and there's still columns to remove
    while ($('#table').width() > $(window).width() - 40 && priorities.length > 0) {
        //get the lowest priority
        sCols = (priorities.pop() + '');

        //split it, in case we have more than one column on the same priority
        //in this case we prefer to remove them as a group, rather than one at a time
        aCols = sCols.split(',');

        //clear the array from previous iteration
        aSelectors = [];
        for (i = 0; i < aCols.length; i++) {
            //build up the jQuery selectors
            aSelectors.push('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')');
        }

        //make jQuery selector string
        sSelectors = aSelectors.join(', ');

        //hide relevant columns
        $(sSelectors).css('display', 'none');
    }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() - start);

I know the people on here can do a way better job, but here it is without jQ and those array function things. Hopefully someone here will give you some real advise.  I just wanted to get rid of the jQ.

One note, you don't need to array.join strings any more.

...only found that out about a year ago myself.

Your main bottle necks will be in how you alter/touch the dom, but I'm no good at those points... google I guess.

All this does give you more speed, but I bet theres people here that would hate me not using array function stuff.  But you seemed to be into speed so that's what I gave you.  Me I can go either way...speed this, comfort that.

function initResponsiveTables() {
  //does this enhance performance at all?
  'use strict';

  //get an array of <th> elements
  var th = document.getElementsByTagName('th');

  //initialise in top scope
  var priorities = [];

  //iterate over the <th>s, extracting their column priorities
  for (var i = 0, end = th.length; i < end; i += 1) {

    var priority = th[i].getAttribute('data-priority');

    //TODO: this seems very messy, I feel there must be a cleaner way?
    if (priorities[priority] === undefined) {
      priorities[priority] = i;
    } else {
      priorities[priority] += ',' + i;
    }
  }

  for (var spot = 0, i = 0, end = priorities.length, x; i < end; i += 1) {
    x = priorities[i];
    if (x !== undefined && x !== null) {
      priorities[spot] = x;
      spot += 1;
    }

  }
  priorities.length = spot;

  //initialise all of these outside of the loop
  var sCols = '';
  var aCols = [];
  var aSelectors = [];
  var sSelectors = '';
  var i;

  var table = document.getElementById('table');

  while (table.getBoundingClientRect().width > window.document.documentElement["clientWidth"] - 40 && priorities.length > 0) {

    //get the lowest priority
    sCols = (priorities.pop() + '');

    //split it, in case we have more than one column on the same priority
    //in this case we prefer to remove them as a group, rather than one at a time
    aCols = sCols.split(',');

    //clear the string from previous iteration
    aSelectors = '';
    for (i = 0; i < aCols.length - 1; i += 1) {
      //build up the selectors
      aSelectors += ('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')') + ', ';
    }
    aSelectors += ('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')');

    var els = table.querySelectorAll(aSelectors);
    for (var i = 0, end = els.length; i < end; i += 1) {
      els[i].style.display = 'none';
    }
  }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() - start);

EDIT:
Couldn't sleep its like 40 degrees C here right now so thought Id make you an example to show the difference when you dont thrash the dom.  I think the technical term (bad memory) would be trying to avoid reflows.

Where yours is is pretty obvious, this....
while (table.getBoundingClientRect().width > window.document.documentElement["clientWidth"] - 40 && priorities.length > 0) {
...specificly....
table.getBoundingClientRect().width for sure and maybe window.document.documentElement["clientWidth"]
...combined with...
els[i].style.display = 'none';

Your, in a loop, changing the dom and then asking for a prop that definitely causes a reflow/repaint/wotevaitscalled if the dom has changed since last call and that stuff is real costly.

So what if we knew the width of the cells and only had to get the width of the table and window once?  Well youd end up with code like below and if you test it youll see it is faster, ESPECIALLY in FireFox (didnt test IE).

I pre picked the width of the cells, but you could look into maybe getting it from the already rendered table (hate that by the way) or other options, or just stick with a set width which the browser would like anyway (easier to paint).

http://plnkr.co/edit/oeJpGK?p=preview      I prfr plnkr ;)

function initResponsiveTables() {
  //does this enhance performance at all?
  'use strict';

  var th = document.getElementsByTagName('th');
  var priorities = [];
  for (var end = 0, i = th.length - 1; i >= end; i -= 1) {

    var priority = th[i].getAttribute('data-priority');

    //TODO: this seems very messy, I feel there must be a cleaner way?
    if (priorities[priority] === undefined) {
      priorities[priority] = [
        i
      ];
    } else {
      priorities[priority].push(i);
    }
  }

  var removeCols = [];
  // Looking at the idea of having a set width
  // Im not great at html, depending on how tables work you might be able to get the width after the table is created instead of using a set width. worse come to worse scan each row for biggest cell
  // its also set in the css
  var colWidth = 85;

  var table = document.getElementById('table');
  var tableWidth = table.getBoundingClientRect().width;
  var targetDiff = table.getBoundingClientRect().width - window.document.documentElement["clientWidth"] - 40;
  var currentDiff = 0;
  var colIndex = priorities.length - 1;
  var cols;

  if (targetDiff > 0) {
    // var atLeast = Math.ceil(diff / colWidth); // how many cols to remove
    while (currentDiff < targetDiff && colIndex >= 0) {

      cols = priorities[colIndex];

      if (!cols) {
        //skip sparse bits
        while (cols === undefined && colIndex >= 0) {
          cols = priorities[colIndex--];
        }
      }

      if (cols) {
        cols.forEach(function(index) {
          table.tHead.rows[0].children[index].style.display = 'none';
          for (var i = 0, end = table.children[1].children.length; i < end; i += 1) {
            var row = table.children[1].children[i];
            row.children[index].style.display = 'none';
          }
        });
        currentDiff += colWidth * cols.length;
      }
      colIndex--;
    }
  }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() -

start);

Use pure JS (VanillaJS)
Use var to store tab length so it does not have to be recalculated for each iteration
minify your code (for page load)
Object Oriented JS improve performance if it's used the right way

This article shows how to write better code for the v8 engine, this is interesting.

this is not the most optimized version but I observed a gain of about 10ms (Sorry to have renamed some variables) :

function initResponsiveTables() {
    'use strict';
    var th = document.getElementsByTagName('th'),
    thL = th.length,
    table = document.getElementById('table'),
    ww = window.innerWidth - 40,
    pp = [], i, p,
    sCols = '',
    aCols = [],
    aSelectors = [],
    sSelectors = '',
    aCL, aa, aaL, aSL;

    for (i=0; i < thL; i++) {
      p = th[i].getAttribute('data-priority');
      pp[p] =  ((!pp[p]) ? i : pp[p]+','+i);
    }
    pp = pp.filter(function (x) {
        return x !== undefined && x !== null;
    });
    while (table.offsetWidth > ww - 40 && pp.length > 0) {
        sCols = (pp.pop() + '');
        aCols = sCols.split(',');
        aSelectors = [];
        aCL = aCols.length;

        for (i = 0; i < aCL; i++)
            aSelectors.push('td:nth-child('+aCols[i]+'), th:nth-child('+aCols[i]+ ')');
                    aSL = aSelectors.length;
        for (i=0; i < aSL; i++) {
            aa = document.querySelectorAll(aSelectors[i]);
          aaL = aa.length;
          for (var j=0; j < aaL; j++)
            aa[j].style.display = 'none';
        }
    }
}

var start = performance.now();

initResponsiveTables();

alert(performance.now() - start);

Minified version :

function initResponsiveTables(){"use strict";var t,e,n,r,o,i,l=document.getElementsByTagName("th"),a=l.length,s=document.getElementById("table"),h=window.innerWidth-40,d=[],f="",c=[],u=[];for(t=0;a>t;t++)e=l[t].getAttribute("data-priority"),d[e]=d[e]?d[e]+","+t:t;for(d=d.filter(function(t){return void 0!==t&&null!==t});s.offsetWidth>h-40&&d.length>0;){for(f=d.pop()+"",c=f.split(","),u=[],n=c.length,t=0;n>t;t++)u.push("td:nth-child("+c[t]+"), th:nth-child("+c[t]+")");for(i=u.length,t=0;i>t;t++){r=document.querySelectorAll(u[t]),o=r.length;for(var p=0;o>p;p++)r[p].style.display="none"}}}var start=performance.now();initResponsiveTables(),alert(performance.now()-start);


A simple but useful error handler I am trying to make a very simple error handler that would catch and handle all kinds of errors in both development and production environments. The idea is to keep it as simple as possible yet useful in the production environment.

In order to do so I am converting all errors into exceptions. Then I will need just a single function to handle both errors and exceptions.

Then, according to my principles of error handling

in the dev environment, it should just echo the error information out, and
in the prod environment, the error information has to be logged and a generalized error message has to be shown on-screen.

It am going to distinguish the modes using display_errors php.ini parameter and also consider all CLI scripts to be in the dev mode too.

set_error_handler(function ($level, $message, $file = '', $line = 0)
{
    throw new ErrorException($message, 0, $level, $file, $line);
});

set_exception_handler(function ($e)
{
    if (ini_get('display_errors') || php_sapi_name() === 'cli') {
        echo $e;
    } else {
        error_log($e);
        header('HTTP/1.1 500 Internal Server Error', TRUE, 500);
        echo "
500 Internal Server Error

              An internal server error has been occurred.

              Please try again later.";
    }
});

There is vast room for improvement, like to create distinct exception classes for each error type (such as DeprecatedErrorException, NoticeErrorException etc.) or, based on the severity, to decide whether the script execution has to be terminated or not. Or a more creative/user-friendly error message. But I am not sure if I should include all that in a simple demonstration example. What are essential additions or modifications could be made to this code?

Perhaps a more "friendly" message would be nice and/or a way to customize it. As an example the generic catcher we're using right now reads:

echo '

Sorry but we ran into an unexpected problem.

The problem has been logged and support staff have been notified.

Please try again.

';

"been occurred" is grammatically incorrect, and I don't think this line makes any sense:

if (ini_get('display_errors') || php_sapi_name() === 'cli')

php_sapi_name() will always return either a 1 or a 0, depending on what method the server is using to access the php script[internally]. It has nothing to do with client side whether you are in development or production..

ini_get() will return a boolean value or an empty string,
as stated in the php_sapi_name manual:

  A boolean ini value of off will be returned as an empty string or "0"
  while a boolean ini value of on will be returned as "1".

So your if statement is basically saying "if 0, 1 or an empty string, run the code below," which is useless. That has no logical use to error reporting.

What is the purpose of checking what type of interface the server is using to access PHP in this scenario?


Unisex Bathroom problem without semaphore Could I ask you to evaluate my solution to Unisex Bathroom Problem?

Problem description

  An office has a bathroom that can be used by both men and women, but not both at the same time. If a man is in the bathroom, other men may enter, but any women wishing to use the bathroom should wait for it to be empty. If a woman is in the bathroom, other women may enter, but any men wishing to use the bathroom should wait it to be empty. Each person (man or woman) will spend some time using the bathroom.

Task

  Design and implement a concurrent solution to the problem. The program should show when a person (man or woman) enters or exits the bathroom; as well as how many people (men or women) are in the bathroom at the moment. Because it is a space of relatively small size, the bathroom has a limiting capacity of persons C (provided as input via the command line or prefixed as a constant value) that can use it at the same time and the time that each person passes in the bathroom is random and different to each program execution.

My solution

I tried to also solve a starvation issue and I think I succeeded.

Besides all of that, what is the necessity of semaphores that the problem is related to? Why can't size be simply compared to capacity as I did?

public class Bathroom {

    private String inUseBy;

    private int size;

    private final int capacity;

    private final Lock lock = new ReentrantLock();

    private final Condition man = lock.newCondition();

    private final Condition woman = lock.newCondition();

    public Bathroom(final int capacity, final int size) {
        this.capacity = capacity;
        this.size = size;
        inUseBy = "";
    }

    public void enter(final String sex) throws InterruptedException {
        lock.lock();
        try {
            while (size == capacity || (size > 0 && !sex.equals(inUseBy))) {
                if (sex.equals("M")) {
                    man.await();
                } else {
                    woman.await();
                }
            }
            if (size == 0) {
                inUseBy = sex;
            }
            size++;
        } finally {
            lock.unlock();
        }
        System.out.println(sex + " using bathroom. Current employees in bathroom = " + size);
        Thread.sleep(10000);
        System.out.println(sex + " done using bathroom");
        lock.lock();
        try {
            size--;
            if (size == 0) {
                if (sex.equals("M")) {
                    woman.signalAll();
                } else {
                    man.signalAll();
                }
            } else {
                if (sex.equals("M")) {
                    man.signal();
                } else {
                    woman.signal();
                }
            }
        } finally {
            lock.unlock();
        }
    }

}

System.out.println(sex + " using bathroom. Current employees in bathroom = " + size);

This line is not protected by lock, so it is possible that size can change before the output is generated.

For example, if two 'M' arrive simultaneously, you could get as output:

M using bathroom.  Current employees in bathroom = 2
M using bathroom.  Current employees in bathroom = 2

which omits outputting ... = 1.

inUseBy is left as the last occupant, even when size decreases to 0.  In the constructor, it is initialized to "", implying that the empty string might also indicate the bathroom is available to any gender.  When you decrease size to zero, you should clear inUseBy.

You have implemented a bathroom that can be occupied by only one type of genital at a time. Therefore it is not a unisex bathroom but a monosex bathroom.

You have labelled sex as "man" and "woman" which are genders. From what I understand the terms you are looking for here are "penis" and "vagina".

The code only identifies entrants as "men" hile all others are gategorized as "women". That is flaw, since the code allows all types of Strings to enter the bathroom. For example "M's that identify as F's". You need to have an unlimited amount of conditions for all self-identified Strings that may want to enter the monosex bathroom or implement some sort of genital inspection and gategorization proxy including an N-to-N mapping of which identities can use the bathroom at the same time.

Please note that StackExchange "does not tolerate any language likely to offend or alienate people based on race, gender, sexual orientation, or religion — and those are just a few examples. When in doubt, just don’t."

If you want an inclusive version of this problem, implement a "watering hole" that can only be entered by animals in the "predator" and "prey" categories at a given time.


Replacing letters with numbers with its position in alphabet If anything in the text isn't a letter, ignore it and don't return it.

  a being 1, b being 2, etc.

  As an example:

  alphabet_position("The sunset sets at twelve o' clock.") Should return
  "20 8 5 19 21 14 19 5 20 19 5 20 19 1 20 20 23 5 12 22 5 15 3 12 15 3
  11" as a string.

It is my naive solution and my code is below in python 2.7

def alphabet_position(text):
    dictt = {'a':'1','b':'2','c':'3','d':'4','e':'5','f':'6','g':'7','h':'8',
    'i':'9','j':'10','k':'11','l':'12','m':'13','n':'14','o':'15','p':'16','q':'17',
    'r':'18','s':'19','t':'20','u':'21','v':'22','w':'23','x':'24','y':'25','z':'26'
    }
    arr = []
    new_text = text.lower()
    for i in list(new_text):
        for k, j in dictt.iteritems():
            if k == i:
                arr.append(j)
    return ' '.join(arr)

First of all, you don't need to hardcode the letters and their positions in the alphabet - you can use the string.ascii_lowercase.

Also, you don't have to call list() on a new_text - you can just iterate over it character by character.

Then, what if we would construct a mapping between letters and letter indexes in the alphabet (with the help of enumerate()). Then, use a list comprehension to create an array of numbers which we then join to produce a result:

from string import ascii_lowercase

LETTERS = {letter: str(index) for index, letter in enumerate(ascii_lowercase, start=1)}

def alphabet_position(text):
    text = text.lower()

    numbers = [LETTERS[character] for character in text if character in LETTERS]

    return ' '.join(numbers)

Seems complicated with all the libraries requirements for this simple problem. I'm not proficient in Python syntax but I know this can be done. Let me explain in pseudocode (syntax can be looked up by you):

Declare a list (or array as I call it in other langs) of alphabet[]="'a', 'b',....'z'"
Their index position is ALREADY their positions...so, the position of 'a' is 0 (because Python index is 0-based; if you don't like it, you can just add 1 to the result to show results  counting from 1)
(lowercase or uppercase, doesn't matter since you're looking at the position of them in either case not their ord values)
Get the string to compare and its length: len(astring)
For that length, iterate and find the character (achar for example) in the string (astring for example). Either for loop or some magical python 1-line statements like: for achar in astring
If you use a 1-line statement instead of a loop, you'd probably have to load them into another list and loop in that. Not sure, I like for loops, so...
Inside the for loop, check each achar's index location if chr(x) == astring[i] for example where i is loop counter, then load that location into a new array (define an empty array outside of the loop first) say, "positions[]"
After the loop ends, positions[] should have the positions (0-based) of each matching char. You could add 1 to each value if you want the positions to be 1-based.

Sorry, I couldn't write the whole code mainly because my syntax knowledge isn't very good in Python yet, but the logic should make sense, right?


Stack implementation in C I have to solve an exercise from ANSI C book:

  Add the commands to print the top elements of the stack without
  popping, to duplicate it, and to swap the top two elements. Add a
  command to clear the stack.

Here is my implementation in RPN Calculator:

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

#define MAXOP 100
#define NUMBER '0'
#define MAXVAL 100
#define BUFSIZE 100

int getop(char []);
void push(double);
double pop(void);
double top(void);
void clear(void);
void swap(void);
void duplicate(void);

int getch(void);
void ungetch(int);

int sp = 0;
double val[MAXVAL];
char buf[BUFSIZE];
int bufp = 0;

int main(void) {
    int type;
    double op2;
    char s[MAXOP];

    while ((type = getop(s)) != EOF) {
        switch (type) {
        case NUMBER:
            push(atof(s));
            break;
        case '+':
            push(pop() + pop());
            break;
        case '*':
            push(pop() * pop());
            break;
        case '-':
            op2 = pop();
            push(pop() - op2);
            break;
        case '/':
            op2 = pop();
            if (op2 != 0.0)
                push(pop() / op2);
            else
                printf("error: zero divisor\n");
            break;
        case '%':
            op2 = pop();
            if (op2 != 0)
                push((int) pop() % (int) op2);
            else
                printf("error: zero divisor\n");
            break;
        case 'c':
            clear();
            printf("stack empty!\n");
            break;
        case 'd':
            duplicate();
            printf("duplicate element\n");
            break;
        case 's':
            swap();
            printf("swap elements\n");
            break;
        case '?':
            printf("top element on the stack: %f\n", top());
            break;
        case '\n':
            printf("\t%.8g\n", pop());
            break;
        default:
            printf("error: unknown command %s\n", s);
            break;
        }
    }
    return 0;
}

void push(double f) {
    if (sp < MAXVAL)
        val[sp++] = f;
    else
        printf("error: stack full, can't push %g\n", f);
}

double pop(void) {
    if (sp > 0)
        return val[--sp];
    else {
        printf("error: stack empty\n");
        return 0.0;
    }
}

double top(void) {
    if (sp > 0)
        return val[sp - 1];
    else {
        printf("error: stack empty\n");
        return 0.0;
    }
}

void clear(void) {
    if (sp > 0) {
        while (--sp > 0);
    } else {
        printf("error: stack empty\n");
    }
}

void duplicate(void) {
    if (sp > 0) {
        int elem = val[sp - 1];
        push(elem);
    } else {
        printf("error: stack empty\n");
    }
}

void swap(void) {
    if (sp > 0 && (sp - 1) > 0) {
        int elem1 = val[sp - 1];
        int elem2 = val[sp - 2];
        val[sp - 1] = elem2;
        val[sp - 2] = elem1;
    } else {
        printf("error: stack empty\n");
    }
}

int getop(char s[]) {
    int i, c;

    while ((s[0] = c = getch()) == ' ' || c == '\t')
        ;
    s[1] = '\0';
    if (!isdigit(c) && c != '.' && c != '-')
        return c;
    i = 0;
    if (c == '-')
        s[++i] = c = getch();
    if (isdigit(c))
        while (isdigit(s[++i] = c = getch()))
            ;
    if (c == '.')
        while (isdigit(s[++i] = c = getch()))
            ;
    s[i] = '\0';
    if (c != EOF)
        ungetch(c);
    return NUMBER;
}

int getch(void) {
    return (bufp > 0) ? buf[--bufp] : getchar();
}

void ungetch(int c) {
    if (bufp >= BUFSIZE)
        printf("ungetch: too many characters\n");
    else
        buf[bufp++] = c;
}

What are your opinions about it? Is it proper and optimal way to implement these functions?

Your clear function uses while (--sp > 0); when sp = 0; does the same thing in faster and clearer way. also consider not throwing an error when clearing an empty stack, because it still behaves as expected.
The condition sp > 0 && (sp - 1) > 0 is esentially just sp > 1 or sp >= 2, which I would prefer, but that is just a matter of personal style.
Instead of using val[sp-1] to get the top element of the array (in swap and duplicate) use the top() function, as it is a good idea to express your stack operations in terms of more primitive stack operations.
Follwing the concept of the defining operation in terms of primitive operations, duplicate can be written as:

void duplicate(void) {
    push(top());
}

and swap can be written as:

void swap(void) {
    int elem1 = pop();
    int elem2 = pop();
    push(elem1);
    push(elem2);
}

Notice how there is no error checking in these functions,
because push(), pop(), and top() handle the full or empty
stack for us.

Your code looks really good.

I doubt it is simple homework because you took care about the evaluation order for the - and / operators and wrote the code for + and * in the unspecified order because these operations are commutative. This level of knowledge is not typical for homework.

One thing you overlooked is calling isdigit with a possibly signed char, which invokes undefined behavior. You have to cast the argument to unsigned char.

Other than that, the code looks carefully written.

If you want to make your code reusable, you should convert the global variables into a struct stack and then define the operations on this stack. This also makes the code easily testable since you will need a function called stack_init that initializes all variables needed for the stack.


Checking for an undefined nested variable in a JS object Is there a better or shorthand way to perform a check on whether or not a variable within (possibly several levels of) an object is null or undefined?

What I would currently do to check is:

if (typeof json !== 'undefined' || typeof json.data !== 'undefined' || typeof json.data.formkeys !== 'undefined') {}

Is there a shorthand way to check for whether or not "json.data.formkeys" is undefined without the concern that one or all of the higher level properties or the object itself might not currently exist?

There's a lot of ways to do this. Here's an exotic (and costly) one:

if(((json || {}).data || {}).formkeys !== void 0){}

As commented, a try-catch will work

// JS has function scope. foo starts its life as declared but undefined
// If the try-catch fails, foo will remain undefined.
try{ var foo = json.data.formkeys; } catch(){}

if(foo !== undefined){}

A longer and possibly better way to do it is to create a resolver function. Most frameworks I have encountered use some form of this in their getter functions so that getting a value from a half-complete path will just return undefined instead of throwing midway.

function resolve(path, root){
  root = root || window;
  return path.split('.').reduce(function(parent, child){
    // A quick check for brevity. You can replace this with a better check
    // and return logic.
    return parent && parent[child];
  }, root);
}

var foo = resolve('json.data.formkeys');

if(foo !== undefined){}

Another one:

if ('formkeys' in ((json || {}).data || {})) {}

which is shorter than:

if (((json || {}).data || {}).formkeys !== void 0) {}