Best practice for factory pattern in c++? I'm implementing a simple data reader to get image sequences from either a pre-recorded video file (In my case ROS bag file), or a camera device (In my case Intel Realsense). Basically I intend for users to use it in the following way:

When a non-empty file path is provided, the reader reads from the video file; otherwise if an empty path is specified, the reader reads from the camera device. (For now in case of any failure, throwing exceptions and terminating the program will be fine.)

The user iteratively calls bool get_next_frame(Frame&) to get all the frames, until reaching the end of the video, upon which the function returns false.

I'm trying to implement the above-mentioned functionality using the factory pattern, in C++ as follows:
class DataReader;
typedef std::shared_ptr<DataReader> datareader_ptr;

class DataReader {
  friend datareader_ptr make_data_reader(const std::string& ros_bag_path);
public:
  virtual bool get_next_frameset(rs2::frameset& frames)=0;
  virtual void finalize() {
    pipe_.stop();
  }
  virtual ~DataReader() {
    std::cout << "dtor for DataReader called." << std::endl;
    finalize();
  }
protected:
  DataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile)
      :cfg_(cfg),pipe_(pipe),pipeline_profile_(pipeline_profile) {}
  rs2::config cfg_;
  rs2::pipeline_profile pipeline_profile_;
  rs2::pipeline pipe_;
};

class RosBagDataReader : public DataReader {
  friend datareader_ptr make_data_reader(const std::string& ros_bag_path);
public:
  virtual bool get_next_frameset(rs2::frameset& frames) {
    frames = pipe_.wait_for_frames();
    return true;
  }
  virtual ~RosBagDataReader() {}
protected:
  RosBagDataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile)
      :DataReader(cfg, pipe, pipeline_profile) {}
};

class CameraDataReader : public DataReader {
  friend datareader_ptr make_data_reader(const std::string& ros_bag_path);
public:
  virtual bool get_next_frameset(rs2::frameset& frames) {
    frames = pipe_.wait_for_frames();
    return true;
  }
  virtual ~CameraDataReader() {}
protected:
  CameraDataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile)
      :DataReader(cfg, pipe, pipeline_profile) {}
};

datareader_ptr make_data_reader(const std::string& ros_bag_path) {
  rs2::config cfg;
  rs2::pipeline pipe;
  if (ros_bag_path=="") {
    rs2::pipeline_profile profile = pipe.start();
    return datareader_ptr(new CameraDataReader(cfg, pipe, profile));
  } else {
    // from ros bag file
    cfg.enable_device_from_file(ros_bag_path, false);
    rs2::pipeline_profile profile = pipe.start(cfg); //File will be opened in read mode at this point
    rs2::playback play(profile.get_device());
    play.set_real_time(false);
    return datareader_ptr(new RosBagDataReader(cfg, pipe, profile));
  }
}

However, I'm really not sure whether this is the best practice. More specifically,

Is the usage of factory pattern here justified ?
Is my implementation of factory pattern correct ? Or are there any pitfalls in the code ?

For example, I used friend function to access the protected constructors in all the classes, is this appropriate or there are better ways to do this?
Another example, protected constructor is used so that the users may not directly construct the readers, but only through the factory function. Is this done correctly?
Thanks in advance!
Update: Definitions of stuff in namespace rs2: may refer to this doc. Basically they are all just thin wrappers holding a shared_ptr to the class doing the real work. (i.e. pimpl idiom)

I’ll start by answering the questions.
Questions

Is the usage of factory pattern here justified ?

Going just by what’s visible in the code and your commentary, I don’t see any purpose to writing the code this way. I don’t see any purpose to the RosBagDataReader or CameraDataReader classes. They don’t actually do anything unique; you could just implement get_next_frameset() in the base class, and forget the whole hierarchy.
In fact, the only difference at all between the classes is not what they do, but rather how their data is set up. I suppose you could sorta-kinda justify having different classes to handle the different types of initialization… but then, you should put the initialization in the classes—that is, in the constructors. However, you could just as easily use two constructors for DataReader, and again, forget the rest of the hierarchy.
But the thing that really makes me scratch my head about why you’d want to use the factory pattern is the fact that you don’t even really have a single factory. You have two factories that you’ve just shoehorned into one by saying “go one way if there’s a path, the other way if not”. That shouldn’t be a single function; that’s poor design. That should be two functions, one taking a path, one not. And once you’ve done it that way, it kinda reveals that the factory pattern doesn’t really make sense here.
Frankly, run-time polymorphism should always be the last tool you reach for in modern C++:

It relies on reference semantics, and C++ is a value semantic language.
It usually means you need to use dynamic allocation, which is slow, cumbersome, and error-prone (though smart pointers help).
It doesn’t really work well with compile-time stuff like type-checking and constexpr… which, again, are C++’s strengths.
It’s often less efficient because of the extra memory used, and extra indirection.
And there are all kinds of nasty gotchas just waiting to bite your ass (like, slicing if you aren’t VERY careful writing copy ops, swap ops, and so on).

HOWEVER….
Of course, there are situations where the factory pattern using run-time polymorphism is the perfect solution. One that springs to mind is the case where you’re writing a library: you’d only give DataReader and the factory function as the entire library interface, and client code can use the library without knowing any details about possible concrete readers, so you could swap them out without having to recompile the client. You could even add readers as plugins.
If you were doing that kind of thing, then sure, the factory pattern makes sense. But… if you’re just writing a program—not a library, let alone an extensible library—then I don’t see the point of it.

Is my implementation of factory pattern correct ? Or are there any pitfalls in the code ?

Eh, it’s mostly okay. It’s not outright wrong, but… there are a few problems with doing things the way you’re doing them.
The first issue is that you don’t seem to know whether you’re writing an interface hierarchy, or an implementation hierarchy. Those are two very different things, and they need to be handled in different ways. If you are trying to build from an interface—in this case, the single function get_next_frameset()—then your base class should be a pure abstract class… what other languages like Java specifically call an interface: no data, only pure virtual functions. If you are building up an implementation, then you’d do things very differently.
I can’t tell you which one you’re really doing here, because, frankly, you don’t have much of an interface or implementation hierarchy, and none of the commentary explains any of the reasoning for even wanting the factory pattern in the first place. As I noted earlier, there doesn’t really seem to be much justification for a hierarchy at all.
I’m also not keen on using shared pointers as the default. It seems completely pointless, especially because you can’t even take advantage of std::make_shared() (because the constructors aren’t public). You should probably return a std::unique_ptr instead. If someone wants a shared pointer, they can always make one out of the unique pointer… but you can’t really do the opposite.

For example, I used friend function to access the protected constructors in all the classes, is this appropriate or there are better ways to do this?

I actually think pretty much all of your uses of protected are bad. But let’s set that aside for now.
If the constructors are non-public, then yeah, you have to use a friend somehow. There’s really no choice.

Another example, protected constructor is used so that the users may not directly construct the readers, but only through the factory function. Is this done correctly?

I honestly don’t see the sense of making any of the constructors protected.
Let’s start with the base class constructor. The only reason you need it protected is because you want initialize the protected data members. Now, I think the protected data members are also a bad idea, but one thing at a time. If you are going to have data members in the base class—which, again, is generally unwise—then, okay, it makes sense to have a protected constructor.
As for the constructors of the other two classes, unlike the base class these are concrete classes. What is the reasoning for hiding those constructors? If you want people to use the factory function, fine, but why make that the only option? Why don’t you want users to directly construct the readers?
By hiding those constructors, you prevent the use of std::make_shared(), which is an optimization opportunity lost. What’s the logic of that?
Also, even if you wanted to hide those constructors, what’s the sense in making them protected, and not private? That seems to imply you expect the classes to be derived from… but that’s a terrible idea: non-leaf classes should be abstract. (Again, assuming you’re doing interface inheritance. If you’re doing implementation inheritance, then there’s a whole other set of rules.)
Code review
Because the code is not presented as it would be in the actual project, and is instead all packed into a single block as some kind of code soup, with no includes and no explanation of what some of the key types are, this isn’t going to be a very comprehensive code review. I’ll do what I can with what I’ve got, but as the saying goes, GIGO.
typedef std::shared_ptr<DataReader> datareader_ptr;

There’s no indication of why shared_ptr is used here, and not unique_ptr, and in fact the rest of the code is written in a way that hamstrings the use of shared_ptr.
virtual void finalize() {
  pipe_.stop();
}
virtual ~DataReader() {
  std::cout << "dtor for DataReader called." << std::endl;
  finalize();
}

NEVER call virtual functions from constructors or destructors. It doesn’t do what you’d think, and only creates confusion for less experienced programmers.
In this case it “works” only because… well, I mean, the whole hierarchy is actually pointless. If the hierarchy were actually doing something that justified its existence, then this would almost certainly not work.
There is no visible purpose to finalize(). Just put pipe_.stop() in the destructor.
DataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile)
    :cfg_(cfg),pipe_(pipe),pipeline_profile_(pipeline_profile) {}

This is not a good way to write this constructor. (Or maybe it is? Who knows? Not enough information, forced to make guesses.)
If you are going to take the arguments by value—which is the right thing to do, because they are sink arguments—then you should move them into their final place… not copy. The copy is unnecessary, and possibly wasteful. And, if the move ops for all those types is noexcept (which it should always be), then that would mean you could make the constructor noexcept too.
The other issue is that you’ve mixed up the order of construction; it’s not the same as the order of declaration:
// order of declaration:
rs2::config           cfg_;
rs2::pipeline_profile pipeline_profile_;
rs2::pipeline         pipe_;

// order of construction:
cfg_(cfg),
pipe_(pipe),
pipeline_profile_(pipeline_profile)

I’m surprised this compiled without warning. You do have all warnings turned on, don’t you?
By the way, all the above is true of all the constructors, because they’re all identical. Which… again… is another issue.
class RosBagDataReader : public DataReader {
  // ... [snip] ...

  virtual bool get_next_frameset(rs2::frameset& frames) {

Prefer one of virtual, override, final, or none of the above. In this case, the one you want is NOT virtual, it’s override.
virtual ~RosBagDataReader() {}

You don’t need this. Don’t write code you don’t need; it just becomes a maintenance burden, noise obscuring important code, and confusion for future coders who wonder why it was put there if it wasn’t necessary.
// in base class:
virtual bool get_next_frameset(rs2::frameset& frames)=0;
rs2::pipeline pipe_;

// in derived class 1:
bool get_next_frameset(rs2::frameset& frames) override {
  frames = pipe_.wait_for_frames();
  return true;
}

// in derived class 2
bool get_next_frameset(rs2::frameset& frames) override {
  frames = pipe_.wait_for_frames();
  return true;
}

Why?
Why not:
// in base class:
virtual bool get_next_frameset(rs2::frameset& frames) {
  frames = pipe_.wait_for_frames();
  return true;
}
rs2::pipeline pipe_;

// in derived class 1:
// nothing needed

// in derived class 2:
// nothing needed

This entire class hierarchy serves no purpose. If I were to reorganize it into something logical, it might look more like this:
// truly abstract base class
class DataReader
{
public:
    virtual ~DataReader() /* = default; */
    {
        std::cout << "dtor for DataReader called." << std::endl;
    }

    virtual bool get_next_frameset(rs2::frameset& frames) = 0;
};

// concrete implementation 1
class RosBagDataReader : public DataReader
{
    // all data is private, as it should be
    rs2::config cfg_;
    rs2::pipeline_profile pipeline_profile_;
    rs2::pipeline pipe_;

public:
    RosBagDataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile) :
        cfg_(std::move(cfg)),
        pipeline_profile_(std::move(pipeline_profile)),
        pipe_(std::move(pipe))
    {}

    ~RosBagDataReader()
    {
        pipe_.stop();
    }

    bool get_next_frameset(rs2::frameset& frames) override
    {
        frames = pipe_.wait_for_frames();
        return true;
    }
};

// concrete implementation 2... same as 1... which, yeah
class CameraDataReader : public DataReader
{
    rs2::config cfg_;
    rs2::pipeline_profile pipeline_profile_;
    rs2::pipeline pipe_;

public:
    CameraDataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile) :
        cfg_(std::move(cfg)),
        pipeline_profile_(std::move(pipeline_profile)),
        pipe_(std::move(pipe))
    {}

    ~CameraDataReader()
    {
        pipe_.stop();
    }

    bool get_next_frameset(rs2::frameset& frames) override
    {
        frames = pipe_.wait_for_frames();
        return true;
    }
};

Or:
// no longer abstract, or even designed for inheritance at all:
class DataReader
{
    // all data is private, as it should be
    rs2::config cfg_;
    rs2::pipeline_profile pipeline_profile_;
    rs2::pipeline pipe_;

public:
    DataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile) :
        cfg_(std::move(cfg)),
        pipeline_profile_(std::move(pipeline_profile)),
        pipe_(std::move(pipe))
    {}

    ~DataReader()
    {
        std::cout << "dtor for DataReader called." << std::endl;
        pipe_.stop();
    }

    bool get_next_frameset(rs2::frameset& frames)
    {
        frames = pipe_.wait_for_frames();
        return true;
    }
};

// now pointless: class RosBagDataReader
// now pointless: class CameraDataReader

Honestly, I don’t even know if the suggestions I’m making make any sense whatsoever, because without the definitions of rs2::config et cetera, I have no clue whether these classes are copyable, movable, or whatever. But I don’t know what else to say: you hide code from reviewers, you get shitty reviews.
datareader_ptr make_data_reader(const std::string& ros_bag_path)

My biggest peeve with this factory function is that the argument list is bullshit. If I want to construct a data reader for a camera, I have to create a completely spurious path. That’s a waste of time for me on every level: it’s a waste of time for me to write the code to create a path I neither want nor need, and it’s a waste of resources within the program to create this useless path only to make a pointless check that it’s empty and then discard it.
If I were actually going to use this code, the very first thing I’d do is write my own wrapper functions:
auto make_camera_reader() { return make_data_reader({}); }

auto make_ros_bag_reader(std::string const& path) { return make_data_reader(path); }

And then I’d use those instead of direct calls to the factory functions.
Why? Because if I saw this in code:
auto reader = make_data_reader({}); // or make_data_reader("") or whatever

I’d immediately be forced to wonder… what is the {}? What does it mean? What am I constructing there at the call site? Will it be expensive? Does it have some semantics I should be aware of?
I like reading mystery stories; I don’t like mystery code. Code shouldn’t challenge me to wonder what’s going on, or force me to go read the docs. It should be crystal clear about what’s going on, and what it’s doing.
On the other hand, if I were to see:
auto reader = make_camera_reader();

That tells me pretty much everything I need to know. No mysteries.
This is not a problem with factory functions in general. Factory functions can still be very clear about what they’re doing. For example:
auto renderer = make_renderer("opengl"); // or make_renderer("vulkan") or whatever

// Here, make_renderer returns unique_ptr<renderer>, and renderer is an
// abstract base class, and the factory function returns concrete renderer
// implementations... but none of that matters! Just from the call above, I
// understand generally what's going on.

Compare that to:
auto reader = make_data_reader("");

// or even:
auto reader = make_data_reader("/path/to/file");

What is really going on there? “Data reader”? Well that’s damn vague. In the first case: the hell is that string supposed to mean? Make a data reader for… nothing? Emptiness? Perhaps the {} refers to wanting only default options? In the second case: okay, we’re making a data reader for some path, probably a file. That’s still pretty damned vague. Are we making a data reader for the file? If so, what kind of data? Are we making a data reader from the file?
If I were going to improve the design of your factory function, I might refactor it as follows:
auto make_data_reader(std::string_view name, std::string_view args)
{
    rs2::config cfg;
    rs2::pipeline pipe;

    if (name == "camera")
    {
        // args is unused

        rs2::pipeline_profile profile = pipe.start();
        return datareader_ptr(new CameraDataReader(cfg, pipe, profile));
    }
    else if (name == "video file")
    {
        // args is the path to the video file

        cfg.enable_device_from_file(args, false);

        rs2::pipeline_profile profile = pipe.start(cfg);

        rs2::playback play(profile.get_device());
        play.set_real_time(false);

        return datareader_ptr(new RosBagDataReader(cfg, pipe, profile));
    }
    /* future expansion:
        else if (name == "url")
        {
            // args is the url

            // ... implementation...
        }
    */
    else
    {
        throw std::logic_error{std::string{"unknown data reader type: "} + name};
    }

    // this should never happen, so it could also be an assert
    throw std::logic_error{std::string{"data reader type "} + std::string{name} + " failed to return reader"};
}

inline auto make_data_reader(std::string_view name)
{
    return make_data_reader(name, {});
}

And the usage is:
// I want to read frames from a camera:
auto reader = make_data_reader("camera");

// I want to read frames from a file:
auto reader = make_data_reader("video file", "/path/to/file");

// Hypothetical extension: I want to read frames from a web service:
auto reader = make_data_reader("url", "https://domain.com/path/to/service");

Let’s keep following the train of thought I started with my wrapper functions. What if they were more than mere wrappers?
auto make_camera_reader()
{
    rs2::config cfg;
    rs2::pipeline pipe;
    rs2::pipeline_profile profile = pipe.start();

    // Note: using make_shared because I assume constructors are public.
    return std::make_shared<CameraDataReader>(std::move(cfg), std::move(pipe), std::move(profile));
}

// Note: taking a proper path rather than a random string. You should always
// prefer proper, semantic types.
auto make_ros_bag_reader(std::filesystem::path const& path)
{
    rs2::config cfg;
    cfg.enable_device_from_file(ros_bag_path.string(), false);

    rs2::pipeline pipe;

    rs2::pipeline_profile profile = pipe.start(cfg);

    rs2::playback play(profile.get_device());
    play.set_real_time(false);

    return std::make_shared<RosBagDataReader>(std::move(cfg), std::move(pipe), std::move(profile));
}

Now there doesn’t seem to be any point to returning pointers, rather than objects directly.
In fact, let’s go one step further:
class image_sequence_reader // a better name than DataReader
{
    rs2::config cfg_;
    rs2::pipeline pipe_;
    rs2::pipeline_profile pipeline_profile_;

public:
    static auto make_for_camera() -> image_sequence_reader
    {
        rs2::config cfg;
        rs2::pipeline pipe;
        rs2::pipeline_profile profile = pipe.start();

        return {std::move(cfg), std::move(pipe), std::move(profile)};
    }

    static auto make_for_video_file(std::filesystem::path const& path) -> image_sequence_reader
    {
        rs2::config cfg;
        cfg.enable_device_from_file(ros_bag_path.string(), false);

        rs2::pipeline pipe;

        rs2::pipeline_profile profile = pipe.start(cfg);

        rs2::playback play(profile.get_device());
        play.set_real_time(false);

        return {std::move(cfg), std::move(pipe), std::move(profile)};
    }

    ~DataReader()
    {
        pipe_.stop();
    }

    auto get_next_frameset(rs2::frameset& frames) -> bool
    {
        frames = pipe_.wait_for_frames();
        return true;
    }

private:
    DataReader(rs2::config cfg, rs2::pipeline pipe, rs2::pipeline_profile pipeline_profile) :
        cfg_(std::move(cfg)),
        pipe_(std::move(pipe)),
        pipeline_profile_(std::move(pipeline_profile))
    {}
};

Usage:
// I want to read frames from a camera:
auto reader = image_sequence_reader::make_for_camera();

// I want to read frames from a file:
auto reader = image_sequence_reader::make_for_video_file("/path/to/file");

Want to extend the capabilities to read from some other source? No problem. Just add a new static member function:
    static auto make_for_nondefault_camera(camera_id const& id) -> image_sequence_reader;
    static auto make_for_device(std::filesystem::path const& device_path) -> image_sequence_reader;
    static auto make_for_web_service(url const& location) -> image_sequence_reader;
    static auto make_for_retina_brain_interface(person const& name) -> image_sequence_reader;
    // and so on...

That’s much less work than adding a whole new derived class, and then monkeying with the factory function to be able to select it.
It’s also clearer—both in the class and at the call site—safer, and, frankly, just easier to use.
Okay, but what if you really, REALLY want to use the factory pattern, because this is going to be in a library, and you want to be able to extend the library without client code having to be recompiled.
With some minor tweaks, you can even still enjoy all the other benefits of the factory pattern too:
////////////////////////////////////////////////////////////////////////
// in header used by client code:

// pure interface:
class image_sequence_reader_interface
{
public:
    virtual ~image_sequence_reader_interface() = default;

    virtual auto get_next_frameset(rs2::frameset&) -> bool = 0;

    // run-time polymorphic classes should generally not be copyable.
    // usually you'd implement a clone() function. i'll skip all that here.
};

// concrete implementation: the only thing client code really needs to care
// about.
class image_sequence_reader final : public image_sequence_reader_interface
{
    // if necessary, you can use a custom deleter to make sure the library
    // deletes its own memory, and not the application.
    std::unique_ptr<image_sequence_reader_interface> _p_impl;

public:
    static auto make_for(std::string_view name, std::string_view args) -> image_sequence_reader;

    static auto make_for(std::string_view name) -> image_sequence_reader
    {
        return make_for(name, {});
    }

    ~image_sequence_reader() = default;

    auto get_next_frameset(rs2::frameset& frames) -> bool override
    {
        return _p_impl->get_next_frames(frames);
    }

    image_sequence_reader(image_sequence_reader&&) noexcept = default;
    auto operator=(image_sequence_reader&&) noexcept -> image_sequence_reader& = default;

private:
    explicit image_sequence_reader(std::unique_ptr<impl>) noexcept;
};

////////////////////////////////////////////////////////////////////////
// in implementation file (cpp file, or module, or library source code, depending):

class camera_image_sequence_reader : public image_sequence_reader_interface
{
    rs2::config cfg_;
    rs2::pipeline pipe_;
    rs2::pipeline_profile pipeline_profile_;

public:
    camera_image_sequence_reader() :
        camera_image_sequence_reader(_construct())
    {}

    ~camera_image_sequence_reader()
    {
        pipe_.stop();
    }

    auto get_next_frameset(rs2::frameset& frames) -> bool override
    {
        frames = pipe_.wait_for_frames();
        return true;
    }

    camera_image_sequence_reader(camera_image_sequence_reader&&) noexcept = default;
    auto operator=(camera_image_sequence_reader&&) noexcept -> camera_image_sequence_reader& = default;

private:
    // All this _construct() stuff *MIGHT BE* needed because the data
    // members all seem to need to be constructed in a specific order, and
    // with references to each other. Or none of it might be necessary;
    // there's no way to know because I don't know what the data types are.

    static auto _construct() -> std::tuple<rs2::config, rs2::pipeline, rs2::pipeline_profile>
    {
        rs2::config cfg;
        rs2::pipeline pipe;
        rs2::pipeline_profile profile = pipe.start();

        return {std::move(cfg), std::move(pipe), std::move(profile)};
    }

    camera_image_sequence_reader(std::tuple<rs2::config, rs2::pipeline, rs2::pipeline_profile> t) :
        cfg_(std::move(t.cfg)),
        pipe_(std::move(t.pipe)),
        pipeline_profile_(std::move(t.profile))
    {}
};

class video_file_image_sequence_reader : public image_sequence_reader_interface
{
    rs2::config cfg_;
    rs2::pipeline pipe_;
    rs2::pipeline_profile pipeline_profile_;

public:
    video_file_image_sequence_reader() :
        video_file_image_sequence_reader(_construct())
    {}

    ~video_file_image_sequence_reader()
    {
        pipe_.stop();
    }

    auto get_next_frameset(rs2::frameset& frames) -> bool override
    {
        frames = pipe_.wait_for_frames();
        return true;
    }

    video_file_image_sequence_reader(video_file_image_sequence_reader&&) noexcept = default;
    auto operator=(video_file_image_sequence_reader&&) noexcept -> video_file_image_sequence_reader& = default;

private:
    static auto _construct() -> std::tuple<rs2::config, rs2::pipeline, rs2::pipeline_profile>
    {
        rs2::config cfg;
        cfg.enable_device_from_file(ros_bag_path.string(), false);

        rs2::pipeline pipe;

        rs2::pipeline_profile profile = pipe.start(cfg);

        rs2::playback play(profile.get_device());
        play.set_real_time(false);

        return {std::move(cfg), std::move(pipe), std::move(profile)};
    }

    video_file_image_sequence_reader(std::tuple<rs2::config, rs2::pipeline, rs2::pipeline_profile> t) :
        cfg_(std::move(t.cfg)),
        pipe_(std::move(t.pipe)),
        pipeline_profile_(std::move(t.profile))
    {}
};

// add or include any other implementations

auto image_sequence_reader::make_for(std::string_view name, std::string_view args) -> image_sequence_reader
{
    auto p = std::unique_ptr<impl>{};

    if (name == "camera")
    {
        p.reset(new camera_image_sequence_reader{});
    }
    else if (name == "video file")
    {
        p.reset(new video_file_image_sequence_reader{args});
    }

    // add any other options ...

    else
    {
        throw std::logic_error{std::string{"unknown image sequence reader type: "} + name};
    }

    // assert(p);

    return image_sequence_reader{std::move(p)};
}

And the usage is the same as before:
// I want to read frames from a camera:
auto reader = image_sequence_reader::make_for("camera");

// I want to read frames from a file:
auto reader = image_sequence_reader::make_for("video file", "/path/to/file");

And not only that, but with very little extra work you could even allow clients to register their own extension implementations. Alls you need is a register function in image_sequence_reader that takes a name and a create function (which returns a unique pointer with an appropriate deleter), and then a map that maps names to create functions, and then in image_sequence_reader::make_for(), just query the map for the name, then use the create function.
Is this a better design than what you have?  Impossible to say with the information I have.


Python-like decorators for caching I am building an ad-hoc text-editor. I have two functions which use a cache, provideHover and provideCompletionItems.

"use strict";

import * as monaco from "monaco-editor/esm/vs/editor/editor.api";
const $ = require("jquery");
const urlResolve = require("url-resolve");

const AUTOCOMPLETE_ENDPOINT = "http://localhost:7000/";
const LRU = require("lru-cache");

const options = {
    max: 500,
    length: (suggestions, key) => { return suggestions.length }
};

const cache = new LRU(options);
const definition_cache = new LRU(options);

(function () {
    // create div to avoid needing a HtmlWebpackPlugin template
    const div = document.createElement('div');
    div.id = 'root';
    div.style = 'width:800px; height:600px; border:1px solid #ccc;';
    document.body.appendChild(div);
})();

monaco.editor.create(
    document.getElementById('root'),
    {
        value: ``,
        language: 'python',
        theme: 'vs-dark'
    }
);

const getCompletions = (text) => {
    return $.ajax({
        url: urlResolve(AUTOCOMPLETE_ENDPOINT, "suggestion"),
        type: 'GET',
        "data": {
            "text": text
        }
    });
};

const getDocumentation = (cursorInfo) => {
    const line = cursorInfo.line;
    const column = cursorInfo.column;
    const text = cursorInfo.text;
    return $.ajax({
        url: urlResolve(AUTOCOMPLETE_ENDPOINT, "documentation"),
        type: 'GET',
        "data": {
            "line": line,
            "column": column,
            "text": text,
            "highlighted": cursorInfo.highlighted
        }
    });

}

monaco.languages.registerHoverProvider("python", {
    provideHover: async (model, position) => {
        const lineNumber = position.lineNumber;
        const snippet = model.getWordAtPosition(position);
        const text = model.getValueInRange({ startLineNumber: 1, startColumn: 1, endLineNumber: position.lineNumber, endColumn: snippet.endColumn });
        if (definition_cache.has(text)) {
            console.log("hit");
            return {
                contents: [{ value: definition_cache.get(text) }]

            }
        }

        const response = await getDocumentation({ "line": lineNumber, "column": snippet.endColumn, "text": text, "highlighted": snippet.word });
        definition_cache.set(text, response.documentation);
        return {
            contents: [{ value: response.documentation }]
        }
    }
});

monaco.languages.registerCompletionItemProvider('python', {
    provideCompletionItems: async (model, position) => {
        const text = model.getValueInRange({ startLineNumber: 1, startColumn: 1, endLineNumber: position.lineNumber, endColumn: position.column });
        if (cache.has(text)) {
            const suggestions = cache.get(text);
            return {
                "suggestions": suggestions
            };

        }

        const completions = await getCompletions(text)
        const suggestions = completions.map(c => ({
            label: c.name,
            insertText: c.name
        }));

        cache.set(text, suggestions)

        return {
            "suggestions": suggestions
        };

    }
});

As it stands, the introduction of the cache makes the code quite ugly (in my opinion). I'd like to refactor it so that I have only one cache usage that applies in both situations -- something like a Python decorator.

How do I make the code cleaner?

I'd look at python decorators as something, that accepts function and returns function, which is usually original function wrapped in another function and some added behaviour.

Change signature of your "decorator" functions to match that and you got something like decorators. Don't have much time at the moment to write examples, but you get the idea :-)

Edit:
I took a closer look at your code, but it seems very specific to your framework and I can't make complete sense of it to dare refactoring (doesn't mean it's bad code). At least here are some examples of wrapper functions to help you started:
https://gist.github.com/harrylove/1230566/d064e5c216384d3846f73ed555e9899be02e8f98


Grid class for games using python I made this class to eventually create a tic tac toe game. I used numpy which is a first for me. Here it is:

import numpy as np

class Grid():

    def __init__(self, data, row_length, coloumn_length):
        """Represents a Grid of data as a matrix.

        Args:
            data(list): data to be placed in grid matrix
            row_length(int): number of elements per row
            coloumn_length(int): number of elements per column
        """
        data_len = row_length*coloumn_length
        if len(data) < data_len:
            difference = data_len - len(data)
            data += [0]*(difference)
        self.grid_matrix = np.array(data, dtype=object).reshape(row_length, coloumn_length)

    def __str__(self):
        return str(self.grid_matrix)

    def replace(self, position, value):
        """Replace a value in a grid"""
        x,y = position
        rows, coloumns = len(self.grid_matrix), len(self.grid_matrix[0])
        if x < rows and y < coloumns:
            self.grid_matrix[x,y] = value

Note the goal is to make a generic grid class to make it easier to make different games with different tools so the actual rendering will be done in the game class.

I am not sure why you do not just use numpys ndarray and instead have invented your own version.
I would however suggest you make it derive from numpys ndarray, which will save you a lot of work, and it allow you to do all kinds of nice things with it. I would also use __setitem__ instead of replace, so you can use the grid[x,y] = value, notation.
There are some problems with the padding of the start data. First off it has side effects (your input is actually changed), and secondly it will only work with actual lists, other numpy arrays and Grids will fail. I recommend generating the grid first, and then slicing in the data when the input does not fit perfectly. Consider whether it would make sense to move setting the grid to have a certain value based on some data (possibly different types) to some other method.


Grouping array elements into batches of at most three const items = ['a', 'b', 'c', 'd']

const reduced = items.reduce((acc, cur, index) => {
  const arrayIndex = Math.ceil((index + 1) / 3) - 1

  if (acc[arrayIndex]) {
    acc[arrayIndex].push(cur)
  } else {
    acc.push([cur])
  }

  return acc
}, [])

I'm taking an array of items, batching them into arrays of three at most and returning them as arrays of array. Here reduced yields the correct structure of [["a", "b", "c"], ["d"]]. How do I accomplish this without the if statement (which mutates the accumulated value directly)?

If you are not beholden to using reduce, you could chunk your array like this.

let chunked = [];
for (let i = 0; i < items.length; i = i + 3) {
    chunked.push(items.slice(i, i + 3));
}

Just for kicks, I implemented a jsFiddle to understand performance of Array.reduce() (as in original post) compared to for loop I have suggested and was somewhat surprised to find the for loop to be on the order of 50% slower than reduce.

When testing for larger arrays (100K items for example) performance became similar between the two solutions.

Regardless, it is probably very much in the territory of micro-optimization to consider one approach vs. the other based on performance testing alone. I would still prefer the simpler for loop code from an code management standpoint unless I knew I was going to be running this code at very high frequency in my application.  However if the majority of the surrounding application was using more of a functional programming style, I would be happy to use reduce as well.


Dijkstra - shortest Path implementation - STL I have implemented Dijkstra algorithm to perform Shortest Path problem.
Input:
Adjacency List(Directed Graph): Description is like that
{Source Node, {edge_1, .. , edge_N}}
Cost Matrix (Same format as Adjacency List)
Queue: Priority Queue
Input to "Shortest Path" method are Start Node, End Node; it returns -1 if
there is no path from Source Node to Target Node, else it returns the cost of the minimum path selected.
Code:
#include <algorithm>
#include <iostream>
#include <limits>
#include <queue>
#include <tuple>
#include <unordered_map>
#include <vector>

namespace {
constexpr int ZERO_DISTANCE_VALUE = 0;
constexpr int INFINITY_VALUE = std::numeric_limits<int>::max();
} // namespace

using NodeType = int;
using CostType = int;
using AdjacencyListType = std::vector<std::vector<NodeType>>;
using DistanceVector = std::vector<NodeType>;
using QueueType = std::queue<NodeType>;
using CostEdgeVector = std::vector<std::vector<CostType>>;
using CostNodeTuple = std::tuple<NodeType, CostType>;
using DistanceMatrixType = std::vector<CostType>;

class Graph {
public:
  explicit Graph(const AdjacencyListType &input_list,
                 const CostEdgeVector &input_cost_list)
      : adjancecy_list(input_list), cost_list(input_cost_list) {}

  struct QueueComparator {
    bool operator()(const CostNodeTuple &left, const CostNodeTuple &right) {
      return std::get<1>(left) < std::get<1>(right);
    }
  };

  int shortest_path(const int &source, const int &target) {
    int result = 0;
    if (source == target)
      return result;
    if (!is_valid_node(source))
      return -1;
    std::vector<int> distance_node(adjancecy_list.size(), INFINITY_VALUE);
    distance_node[source] = 0;
    queue.emplace(std::make_tuple(source, 0));
    while (!queue.empty()) {
      const auto &current_node = queue.top();
      queue.pop();
      const NodeType &current_node_index = std::get<0>(current_node);
      const auto &sub_node_vector = adjancecy_list.at(current_node_index);
      const auto &sub_node_cost = cost_list.at(current_node_index);
      const int &current_distance_cost = distance_node.at(current_node_index);
      for (NodeType index = 0; index < sub_node_vector.size(); ++index) {
        const auto &child_index = sub_node_vector.at(index);
        const int relaxation_value =
            current_distance_cost + sub_node_cost.at(index);

        if ((distance_node.at(child_index) > relaxation_value)) {
          distance_node[child_index] = relaxation_value;
          queue.emplace(std::make_tuple(child_index, relaxation_value));
        }
      }
    }
    try {
      const auto &target_distance = distance_node.at(target);
      result = target_distance == INFINITY_VALUE ? -1 : target_distance;
    } catch (...) {
      result = -1;
    }

    return result;
  }

private:
  bool is_valid_node(const NodeType &node) {
    return node < adjancecy_list.size();
  }

  std::priority_queue<std::tuple<NodeType, int>,
                      std::vector<std::tuple<NodeType, int>>, QueueComparator>
      queue;

  const CostEdgeVector &cost_list;
  const AdjacencyListType &adjancecy_list;
};

int main() {
  AdjacencyListType cost_vector;
  CostEdgeVector adjancecy_list;
  NodeType source_node = 0;
  NodeType target_node = 0;
  Graph graph(adjancecy_list, cost_vector);
  const auto result = graph.shortest_path(source_node, target_node);
  std::cout << "Shortest Path value: " << result << std::endl;
  return 0;
}

First: Why use a class?

Graph graph(adjancecy_list, cost_vector);
const auto result = graph.shortest_path(source_node, target_node);

This could easily be refactored to a simple function call of:

const auto result = shortest_path(adjancency_list, cost_vector, source_node, target_node);

Since all shortest_path does is access the classes adjancency_list and cost_vector, which can just be passed in, queue, which should just be a local variable, and is_valid_node, which could just be inlined as you only called it once.

Where you have used the class, there are some minor mistakes. In your constructor, you have initialised adjancecy_list and then cost_list. This should be the other way around (Or you should swap the members). The constructor is also explicit for no reason.

class Graph {
public:
  Graph(const AdjacencyListType &input_list,
        const CostEdgeVector &input_cost_list)
    : adjancecy_list(input_list), cost_list(input_cost_list) {}
  // ...
private:
  // The other way around
  const AdjacencyListType &adjancecy_list;
  const CostEdgeVector &cost_list;
}

      const auto &current_node = queue.top();
      queue.pop();

Is undefined behaviour. You take the const reference of a node, and then you delete it with pop. current_node would then be garbage. Just take a copy:

      const auto current_node = queue.top();
      queue.pop();

You should probably not use reference at all. The objects are all ints or std::tuple<int, int>s, which are very cheap to copy.

This:

    try {
      const auto &target_distance = distance_node.at(target);
      result = target_distance == INFINITY_VALUE ? -1 : target_distance;
    } catch (...) {
      result = -1;
    }

Can be worded in a much clearer way. First, try to avoid catch (...) as much as possible. The only thing that would throw is at, which throws a std::out_of_range, so you can write catch (const std::out_of_range&). However, exceptions are very confusing here. It only throws if target is too big, so you can write something like:

    if (target >= distance_node.size()) {
      result = -1;
    } else {
      const auto target_distance = distance_node[size];
      result = target_distance == INFINITY_VALUE ? -1 : target_distance;
    }

Or even better, change if (!is_valid_node(source)) return -1 to if (!is_valid_node(source) || !is_valid_node(target)) return -1, so that the target will always be in distance_node.

And (this is personal preference) I would avoid the c-like pattern of declaring a result variable. Where you do result = at the end can be replaced with just return, and it's still very clear.

If you are able to control your input, it would probably be easier to, instead of having two vectors which name the neighbour and have the cost of the edge, have a list of neighbours of pairs of nodes and costs. I would declare a helper struct:

struct edge {
  NodeType neighbour;
  CostType cost;
};

// or `using edge = std::pair<NodeType, CostType>;`, but that has less meaning

And have one vector of vector of these.

Also, you use a std::tuple<int, int> for "CostNodeTuple". Since there are only 2 values, it might have been easier to use a std::pair, so std::get<0>(t) turns into t.first, and std::get<1>(t) turns into t.second.

And if you swapped the order, you wouldn't need a custom comparator (As a tuple or pair's operator< does mostly what you want)

If you can do even more refactoring, I would consider using unsigned instead of int, as this doesn't work for negative edge weights (i.e. if there is a negative loop, it should have a value of -infinity). Instead of returning -1 for failure, you could throw an error (if this is an exceptional circumstance) or returning a std::optional<unsigned> (if disjoint graphs are common). The current solution is fine as is though.

Currently, using signed integers, there are a bunch of places where they are implicitly converted to unsigned integers (to use for array indices). It's not too much of a problem, but it shouldn't be the case (Maybe use std::size_t for NodeType?)


Generic array group by using lambda This is K.I.S.S, but how about some error and misuse control? I am wishing it were .NET.

<?php

function array_group_by($arr, $key_selector) {
    $result = array();
    foreach($arr as $i){
        $key = $key_selector($i);
        if(!array_key_exists($key, $result)) {
            $result[$key] = array();
        }
        $result[$key][] = $i;
    }
    return $result;
}

 $data = array(
        array(1, "Andy", "PHP"),
        array(1, "Andy", "C#"),
        array(2, "Josh", "C#"),
        array(2, "Josh", "ASP"),
        array(1, "Andy", "SQL"),
        array(3, "Steve", "SQL"),
    );

$grouped = array_group_by($data, function($i){  return $i[0]; });

var_dump($grouped);

?>

Using the [] syntax for adding array items, allows PHP to create the array as necessary. Thus you can scrap the if statement for a modest performance increase off 10 to 15% in my own testing. (Unlike if you used array_push() which would throw a warning instead.
The unused $group variable appears to be a mistake, as the function appears to fulfill it's purpose without it. Code reduced to:

function array_group_by($arr, $key_selector) {
  $result = array();
  foreach ($arr as $i) {
    $key = $key_selector($i);
    $result[$key][] = $i;
  }
  return $result;
}

At this point, the $key could also be left out, but it's almost free in terms of performance, and increases readability, so I left it there.
I'm not sure how much misuse and error handling you need, the function is pretty simple. I would stick with type-hinting the arguments. array is available since PHP 5.1, callable since 5.4. The signature would become

function array_group_by(array $arr, callable $key_selector);

Then PHP will at run-time throw a fatal error at anyone trying to call the function with incorrect parameters. Generally I think that's good, but in this particular case, it introduces a problem.
array('class_A', 'method_B'), is a valid callable, that won't work with with the fast $function($arg);. For any valid callable to get executed, you need to use call_user_func() instead, which is a bit slower. In my testing however, the removal of the if has bigger impact than the use of call_user_func(), so if this isn't performance critical, stick with callable and call_user_func().

Final code:

function array_group_by(array $arr, callable $key_selector) {
  $result = array();
  foreach ($arr as $i) {
    $key = call_user_func($key_selector, $i);
    $result[$key][] = $i;
  }
  return $result;
}


Monoalphatic and Polialphabetic cipher in Ruby This code encrypts a text with  mono-alphabetic and poli-alphabetic substitutions ciphers.

For further info see:

Mono-alphabetic/Caesar Cipher
Poli-alphabetic cipher

require 'arrow_test'

ALPHABET = ("a".."z").to_a

# Moves a letter forward in the alphabet by
# the given key wrapping around.
#
#
# shift('a', 2) #=> 'c'
#
#
#
# shift('z', 2) #=> 'b'
#

def shift(letter, key)
  ALPHABET[ (ALPHABET.index(letter) + key) % ALPHABET.length]
end

# Encrypts a text using by shifting all its letters by
# the given key. All characters that are not lower and alphabetic
# are deleted.
#
#
# monoalphabetic_cipher('abcd', 2) #=> 'cdef'
#
#
#
# monoalphabetic_cipher('cdef', 2, decode=true) #=> 'abcd'
#

def monoalphabetic_cipher(text, key, decode=false)
  text
      .chars
      .select { |char| ALPHABET.include?(char) }
      .map {|char| shift(char, (decode ? 26 - key : key) )}
      .join
end

# Given a word, returns the positions of its letters
# in the aplhabet.
#
#
# word_to_alphabetic_positions('abcz') #=> [0, 1, 2, 25]
#

def word_to_alphabetic_positions(word)
  word.chars.map {|char| ALPHABET.index(char) }
end

# Encrypts a text by shifting each letter by
# the amount specified by the corresponding key number,
# wraps around over the key if the plain text is longer than it.
#
#
# polialphabetic_cipher('abcd', [1,3,4,0]) #=> 'begd'
#
#
#
# polialphabetic_cipher('begd', [1,3,4,0], false, decode=true) #=> 'abcd'
#

def polialphabetic_cipher(text, keys, keys_as_word=false, decode=false)
  keys = word_to_alphabetic_positions(keys) if keys_as_word
  text
      .chars
      .select { |char| ALPHABET.include?(char) }
      .each_with_index
      .map do |char, index|
        key = keys[index % keys.length]
        shift(char, (decode ? 26 - key : key))
      end
      .join
end

arrow_test

Extract constant expression to outside of loop

In this code, (decode ? 26 - key : key) is evaluated for each character in text:

  text
      .chars
      .select { |char| ALPHABET.include?(char) }
      .map {|char| shift(char, (decode ? 26 - key : key) )}
      .join

It would be better to move it outside the expression so that it's only evaluated once:

  shift_key = decode ? 26 - key : key

  text
      .chars
      .select { |char| ALPHABET.include?(char) }
      .map {|char| shift(char, shift_key)}
      .join

Minor formatting issues

The code formatting style is not consistent, for example compare these two lines:

      .select { |char| ALPHABET.include?(char) }
      .map {|char| shift(char, shift_key)}

In the first one, there's a space after { and before },
but in the second there isn't.
Both ways seem fine to me,
but it would be better to use one of these ways, consistently everywhere.

Similarly here,
there's a space after [ but not before ].
For the sake of consistency,
it would be better to remove the space after [ or add one before ].

  ALPHABET[ (ALPHABET.index(letter) + key) % ALPHABET.length]

Overall

This looks nice, a pleasure to read, and I don't even know Ruby ;-)


Shift positive integers to the left I would like to write a function in C that turns all negative values into zeros then shifts the positive numbers to the beginning of the array, maintaining the order of the positive integers in the array.

void shift_positives(int values[], int num_values) {
    int i;
    int last_zero = 0;
    int j;
    for (j = 0; j < num_values; j++) {
        if (values[i] < 0) values[i] = 0;

        if (values[j] > 0) {
            for (i = last_zero; i < j; i++) {
                if (values[i] == 0) {
                    last_zero = i;
                    break;
                }
            }
            if (last_zero < j) {
                values[last_zero] = values[j];
                values[j] = 0;
            }
        }
    }
}

My issue with this is that it runs at \$O(N^2)\$ because of the inner for loop. Is there a more efficient way to do this?

There is a much more efficient way to do this, involving two pointers, a 'right', and a 'left' pointer. They both start at 0.

The trick is to inspect the value at the right pointer. If it is negative, increment the right pointer. If it is positive, move it to the left pointer, and increment both pointers.

This allows you to move all the positive values to the beginning of the array, in the order they were found. All you need to do then, is to zero out all the values from the left pointer to the right....

I assume that 0 is considered to be 'not positive'....

void shift_positives(int values[], int num_values) {
    int left = 0;
    for (int right = 0; right < num_values; right++) {
        if (values[right] > 0) {
            values[left++] = values[right];
        }
    }
    while (left < num_values) {
        values[left++] = 0;
    }
}

This performs the operation in \$O(n)\$ time. See it running on ideone


Bash arrays and case statements - review my script #!/bin/bash
# Change the environment in which you are currently working.
# Actually, it calls the relevant 'lettus.sh' script

if [ "${BASH_SOURCE[0]}" == "$0" ]; then
    echo "Try running this as \". chenv $1\""
    exit 0
fi

usage(){
    echo "Usage: . ${PROG}        -- Shows a list of user-selectable environments."
    echo "       . ${PROG} [env]  -- Select environment."
    echo "       . ${PROG} -h     -- Shows this usage screen."
    return
}

showEnv(){
    # check if index0 exists, assume we have at least the first (zeroth) element
    #if [ -z "${envList}" ]; then
    if [ -z "${envList[0]}" ]; then
        echo "array \$envList is empty! " >&2
        return 1
    fi

    # Show all elements in array (0 -> n-1)
    for i in $(seq 0 $((${#envList[@]} - 1))); do
        echo ${envList[$i]}
    done
    return
}
setEnv(){
    if [ -z "$1" ]; then
        usage; return
    fi

    case $1 in
        cold)       FILE_TO_SOURCE=/u2/tip/conf/ctrl/lettus_cold.sh;;
        coles)      FILE_TO_SOURCE=/u2/tip/conf/ctrl/lettus_coles.sh;;
        fc)         FILE_TO_SOURCE=/u2/tip/conf/ctrl/lettus_fc.sh;;
        fcrm)       FILE_TO_SOURCE=/u2/tip/conf/ctrl/lettus_fcrm.sh;;
        stable)     FILE_TO_SOURCE=/u2/tip/conf/ctrl/lettus_stable.sh;;
        tip)        FILE_TO_SOURCE=/u2/tip/conf/ctrl/lettus_tip.sh;;
        uat)        FILE_TO_SOURCE=/u2/tip/conf/ctrl/lettus_uat.sh;;
        wellmdc)    FILE_TO_SOURCE=/u2/tip/conf/ctrl/lettus_wellmdc.sh;;
        *)          usage; return;;
    esac

    if $IS_SOURCED; then
        echo "Environment \"$1\" selected."
        echo "Now sourcing file \"$FILE_TO_SOURCE\"..."
        . ${FILE_TO_SOURCE}
        return
    else
        return 1
    fi
}

main(){
    if [ -z "$1" ]; then
        showEnv; return
    fi

    case $1 in
        -h) usage;;
         *) setEnv $1;;
    esac
    return
}

PROG="chenv"

# create array of user-selectable environments
envList=( cold coles fc fcrm stable tip uat wellmdc )

main "$@"
return

If I could, I'd like to get some feedback on a better way to accomplish any of the following:

run through the case statement
make script trivally simple to maintain/upgrade/update

In showEnv, why do you need to check that envList is empty when you define it right in the program?

I'd write the array elements one-per-line with:

# Show all elements in array
for elem in "${envList[@]}"; do
    echo "$elem"
done

or, more succinctly

printf "%s\n" "${envList[@]}"

In setEnv:

case $1 in
    cold|coles|fc|fcrm|stable|tip|uat|wellmdc)
        FILE_TO_SOURCE="/u2/tip/conf/ctrl/lettus_$1.sh" ;;
    *)  usage; return;;
esac

or, without duplicating the contents of envList:

FILE_TO_SOURCE=""
for elem in "${envList[@]}"; do
    if [ "$elem" = "$1" ]; then
        FILE_TO_SOURCE="/u2/tip/conf/ctrl/lettus_$1.sh"
        break
    fi
done
if [ -z "$FILE_TO_SOURCE" ];then
    usage
    return
fi

You might want to check that the relevant file exists

if [ ! -f "$FILE_TO_SOURCE" ]; then
    echo "missing config file '$FILE_TO_SOURCE'!"
    echo "contact $maintainer for assistance"
    return 1
fi

What is $IS_SOURCED? You might want to document how that environment variable is defined.

You don't need the return at the end of main or at the end of the program.


Finding the lowest and highest values in an array... with "Poor Man's DI" I am practicing my object oriented knowledge, so I created a very simple program that determines which is the lowest and highest number inside array.

This is my main method:

class Program
{
    static void Main()
    {
        Console.Write("How many numbers would you like to enter?: ");
        int num = int.Parse(Console.ReadLine());

        int[] arr = new int[num];

        for (int x = 0; x < num; x++ )
        {
            Console.Write("Enter number {0}: ", (x+1));
            arr[x] = int.Parse(Console.ReadLine());
        }

        Console.WriteLine();
        Console.Write("Lowest (1) \nHighest (2) \nEnter choice: ");
        int choice = int.Parse(Console.ReadLine());

        Math math;

        if (choice == 1){
            math = new Math(new HighestNumber(), arr);
            Console.WriteLine("Answer: " + math.returnAnswer());
        }
        else if (choice == 2){
            math = new Math(new LowestNumber(), arr);
            Console.WriteLine("Answer: " + math.returnAnswer());
        }

        Console.ReadKey();
    }
}

The IMathPerformer interface:

interface IMathPerformer
{
    int getAnswer(int[]arr);
}

...and its implementations:

class HighestNumber : IMathPerformer
{
    public int getAnswer(int[]arr)
    {
        int highNum = 0;
        for(int x = 0; x<arr.Length - 1; x++)
        {
            if(arr[highNum] > arr[x+1])
            {
                highNum = (x + 1);
            }
        }

        return arr[highNum];
    }
}

class LowestNumber : IMathPerformer
{
    public int getAnswer(int[]arr)
    {
        int lowNum = 0;
        for (int x = 0; x < arr.Length - 1; x++)
        {
            if (arr[lowNum] < arr[x + 1])
            {
                lowNum = (x + 1);
            }
        }

        return arr[lowNum];
    }

The Math class:

class Math
{
    private int[] arr;
    private readonly IMathPerformer imathPerformer;
    public Math(IMathPerformer imathPerformer, int[]arr)
    {
        this.imathPerformer = imathPerformer;
        this.arr = arr;
    }

    public int returnAnswer()
    {
        return imathPerformer.getAnswer(arr);
    }
}

I just want to know if I am doing right or this is a bad practice. If it is bad, it would be greatly appreciated if you could point out why and how to fix it and add some more advice and suggestions.

First of all, kudos for exercising OOP and DI! Coding against interfaces is going to make your life easier in so many wonderful ways!

I agree, no problem is too simple to solve with SOLID principles, especially for learning purposes. Shifting from procedural to object-oriented code takes practice.

The first key to proper OOP is abstraction.

These are the abstractions I see in your code:

IMathPerformer, an interface that gets some answer given an int[]. Implementations are responsible for computing the Min or Max value, or any other operation that returns an int given an int[].

That's unfortunately all. Unfortunate, because I see these requirements in your code:

Collecting the data (the int[] to work with)
Determining what to do with the data
Outputting the result

And from these requirements, I see these responsibilities:

Interacting with the console

Validating user input

Collecting the data
Knowing what to do with the data

Creating the menu with each available command

Performing the appropriate command
Outputting the result
Coordinating the sequence of operations

If a class should be responsible for one thing, then there should be at least 5-6 types in this program.

Back to your code: the Math class doesn't seem to really "fit in" anywhere in the above list - or rather, it's redundant: the command to execute is already known when the class gets created, and it acts as a middle man that doesn't really do anything that the IMathPerformer can't do just as well without it; it's forcing dependency injection, but there's nothing that justifies it because the abstraction level in the calling code is too low.

Abstraction levels are crucial. The entry point of your program (the Main method) should have a very high abstraction level, and is usually pretty short. For example:

public static void Main(string[] args)
{
    var app = CreateApp(args);
    app.Run();
}

We create an app object and call its Run method. The CreateApp method might look like this:

private static ArrayOpsApp CreateApp(string[] args)
{
    var validator = new UserInputValidator();
    var console = new ConsoleUserInterface(validator);
    var argsParser = new CommandLineArgsParser();
    var collector = new IntConsoleCollector(console, argsParser, args);

    var commands = new[] {
        new MinValueFinder(),
        new MaxValueFinder(),
        new AvgCalculator(),
        new StdDevCalculator(),
        //...
    };
    var commandSelector = new ConsoleCommandSelector(console, commands);
    var app = new ArrayOpsApp(console, collector, commandSelector);

    return app;
}

This method has a very special role: it's the only place in the application we're allowed to new up one of our concrete types - everywhere else, we are coding against interfaces. It's the application's composition root, where we compose the application's dependency graph.

Just by looking at this code, you can get a grasp of how the entire application is structured, because every type has its dependencies injected through the constructor.

You know that the IntConsoleCollector is the specific implementation for what the rest of the code sees as some abstract IDataCollector, or even a generic IDataCollector<int>.

You know that the user input will be validated somehow.

Because that type takes a ConsoleUserInterface implementation for what might be its IUserInterface dependency, we know that the type needs an object that's responsible for dealing with UI concerns, and because it takes an IArgsParser, we can infer that we can specify the input with command-line arguments and that the collector might just spit them back and skip prompting the user for them. Perhaps it's also used for parsing the user's input if the args array is empty or doesn't contain anything useful - but that's an implementation detail.

We know exactly which commands will be available for the commandSelector, and again we know that the selector will work with the console, because we're giving it console for its IUserInterface dependency; we can infer that the selector will be iterating the available commands to create a menu interface using the console.

The method then injects the collector and the selector dependencies into a new ArrayOpsApp, which will obviously be responsible for consuming these dependencies and coordinating the sequence of operations.

The Main method in your code has both hands into that sequence of operations; it's responsible for collecting the data, dealing with the user's input, determining which concrete types to use, ... it does pretty much everything except actually computing the min/max values.

You started implementing DI, but stopped somewhere along the way. Half-implemented DI unfortunately doesn't get half the benefits of DI: the only testable code is the code that you've extracted into an interface... which makes the IMathPerformer interface pretty much useless, since if you test the implementations you don't really need the interface it's implementing.

There is no way to write a test that documents what happens when the user enters invalid input. Sad, because you would have found out that you get a FormatException when the int.Parse call fails.

Because of the high coupling and low cohesion of the Main method, most of the code can't be unit-tested at all. You'll want to reverse that, and achieve low coupling and high cohesion.

SOLID isn't everything though: it doesn't really touch on the minute implementation details. This is where things like the DRY principle kick in:

if (choice == 1){
    math = new Math(new HighestNumber(), arr);
    Console.WriteLine("Answer: " + math.returnAnswer());
}
else if (choice == 2){
    math = new Math(new LowestNumber(), arr);
    Console.WriteLine("Answer: " + math.returnAnswer());
}

Don't Repeat Yourself. That Console.WriteLine call doesn't belong in either branch of that conditional:

if (choice == 1){
    math = new Math(new HighestNumber(), arr);
}
else if (choice == 2){
    math = new Math(new LowestNumber(), arr);
}
Console.WriteLine("Answer: " + math.returnAnswer());

But then again, implementation details are also impacted by the high-level structure of the code - for example with a higher abstraction level and injected dependencies, the entire ArrayOpsApp might look like this:

public class ArrayOpsApp
{
    private readonly IUserInterface _ui;
    private readonly IDataCollector _collector;
    private reaodnly ICommandSelector _selector;

    public ArrayOpsApp(IUserInterface ui, IDataCollector collector, ICommandSelector selector)
    {
        _ui = ui;
        _collector = collector;
        _selector = selector;
    }

    public void Run()
    {
        var data = _collector.CollectData();
        var command = _selector.SelectCommand();
        var result = command.Execute(data);
        _ui.OutputResult(result);
    }
}

Notice you can write a test that validates that ArrayOpsApp.Run() calls the CollectData member of its IDataCollector dependency, the SelectCommand member of its ICommandSelector dependency, and the OutputResult of its IUserInterface dependency. If we also mock the output of the selector's SelectCommand method we can also verify that the command's Execute method is called, so we can write tests that document what the Run method is supposed to be doing; how it does it doesn't matter.

Writing SOLID OOP code makes your code testable, right. But writing SOLID OOP code is much easier if you start by writing the tests, or rather, by asking yourself how you would want to be able to test the code. Don't just rush into implementing the functionality - of course you can implement the functionality, it's just a very simple exercise after all! It's not about the functionality itself, it's about properly separating concerns and doing things right. Remember:

Single Responsibility Principle: a class should have one single reason to change.
Open/Closed Principle: the design is extensible, so instead of modifying a class to add functionality, you just add a new one that implements it.
Liskov Substitution Principle: all implementations of an interface fulfill the same contract - substituting one implementation for another doesn't break the program. IUserInterface could very well be a WinForms or a WPF UI, makes no difference. Or a console UI. Or no UI at all, just a mock.
Interface Segregation Principle: interfaces should be specialized. Remember modifying an interface is a breaking change.
Dependency Inversion Principle: depend on abstractions, not concrete types.

(SOLID)


Extend number of arguments in std::hypot I would like to call std::hypot with many arguments (not just 2 or 3).

Mandatory requirements:

fast
run at compile time when possible

Non-mandatory requirement:

avoid overflow

#include <cmath>
#include <iostream>

template <typename T>
constexpr T sum2(const T v) {
  return v * v;
}

template<typename T1, typename... Ts>
constexpr T1 sum2(const T1 v1, const Ts... rest)
{
  return v1 * v1 + sum2(rest...);
}

template <typename T1, typename... Ts>
constexpr T1 qsum(const T1 v1, const Ts... v)
{
  return sqrt(sum2(v1, v...));
}

int main()
{
  double a = 10.;
  std::cout << qsum(1., a) << std::endl;
}

One bad thing is when I ask qsum(1, 1.) since the first argument is int and so the output. I would like that in this case the inputs are cast to the safer common type.

First of all, let me just say that qsum is a really bad name for a generic hypot function. I would have called it... hypot! In fact, I'm going to do that in this answer, because it'll make the code a lot easier to read.

Well, I'll call it my_hypot, because we'll be including <cmath>, and I don't want any confusion over whether we're accidentally referring to the standard library's 2-argument hypot.

You have an anti-pattern in your code: you're writing things like T sum2(const T v) instead of T sum2(T v). The former is exactly equivalent to the latter as far as the caller of the function is concerned; but for the implementor of the function, the latter is strictly better:

It's less typing, and more importantly, less reading for the maintainer/reviewer.
It doesn't inhibit move semantics.
For these reasons, it's idiomatic C++; and therefore you should follow the idiom because breaking idioms for no reason is distracting to the maintainer/reviewer.

I think you could simplify your code by "thinking in C++17" and then working backwards to lower the resulting code back down to C++11.

In C++17, with fold-expressions, your function would be just

template<typename... Ts> constexpr auto my_hypot(Ts... vs) {
    return sqrt(( (vs * vs) + ... ));
}

In C++14, though, we don't have fold-expressions, so we're going to have to replace the construct (pattern + ...) with some kind of function call; say, sum(pattern...). So our C++14 code should look like

template<typename... Ts>
constexpr auto my_hypot(Ts... vs) {
    return sqrt(sum( (vs * vs)... ));
}

In C++11, we need to use the "trailing decltype" trick to get the compiler to accept that code, since C++11 can't deduce return types (except in lambdas).

template<typename... Ts>
constexpr auto my_hypot(Ts... vs) -> decltype(sqrt(sum( (vs * vs)... ))) {
    return sqrt(sum( (vs * vs)... ));
}

And there we go! Other than writing the sum function, this is a very straightforward translation of the code you want to write, into code that should be acceptable to a C++11 compiler.

However, I think your code as it stands is a bit sketchy, because of the overflow issue. my_hypot(65536, 65536, 1.0) should yield 92681.9, not undefined behavior! To mitigate that issue, I agree with you that casting all of the parameters to common_type would be a good idea. Let's implement that.

#include <cmath>
#include <iostream>
#include <type_traits>

template<typename CT> constexpr CT sum() { return {}; }

template<typename CT, typename T, typename... Ts>
constexpr CT sum(T v, Ts... vs)
{
    return v + sum<CT>(vs...);
}

template <typename... Ts>
constexpr auto my_hypot(Ts... vs) -> typename std::common_type<Ts...>::type
{
    using CT = typename std::common_type<Ts...>::type;
    return sqrt(sum<CT>( (CT(vs) * CT(vs))... ));
}

int main()
{
  std::cout << my_hypot(10., 10) << std::endl;
  std::cout << my_hypot(10, 10, 10.) << std::endl;
  std::cout << my_hypot(10, 10) << std::endl;
}

This is all the same straightforward code as before, with the possible exception of sum<CT>(). Here we're making an overload set of sum<CT>() (with no arguments) and sum<CT>(T, Ts...) (with at least one argument). By construction, both T and all the elements of Ts... will always be CT; we could static_assert that, if we wanted to. sum<CT>() returns a value-initialized CT (i.e., the "zero" of whatever type CT happens to be); I used {} instead of 0 just in case CT is some wacky bignum type that doesn't have an implicit conversion from int. Depending on your assumptions, 0 might be the safer choice.

The really nice thing about this code is that because we constructed it by starting with C++17 and moving backward, it's easy to take this code and migrate it forward to C++14 (by removing the trailing return types) and then to C++17 (by replacing the implementation of sum with a one-liner, or inlining it altogether) as your compiler catches up. Looking at your original code, I think it would be a little bit harder to disentangle the C++11isms from the "business logic" of how to compute hypot. For example, you've got the subexpression v * v repeated in two different places, for no good reason that I can see. You could have factored that out... or rather, not factored it in in the first place! :) Don't repeat yourself.


just making sure this is efficient, for the null checks This is inside a method (basically its just this), I try to get the neededValue otherwise I pass back what was passed in...

private static string GetNeededValue(string param)
    {
        string tValue = "";

        if (!string.IsNullOrEmpty(param))
        {
            var map = repo.Item(string.Format("CoverageName LIKE '{0}'",param));
            tValue = map == null ? param : map.theNeededValue;
        }

        return tValue;
    }

Yes, it seems fine from an efficiency point of view. Here's a couple of simple alternatives you could consider.

Remove the need for the ! condition.  I quite like this option as I think it flows nicely and is easy to read.

private static string GetNeededValue(string param)
{
   // do we care about whitespace.  If so you could use IsNullOrWhiteSpace
   if(string.IsNullOrEmpty(param)) return param;

   var map = repo.Item(string.Format("CoverageName LIKE '{0}'",param));
   return map == null ? param : map.theNeededValue;
}

Or another alternative might be to be more explicit about when tValue is set.

private static string GetNeededValue(string param)
{
   var tValue = param;

   // do we care about whitespace.  If so you could use IsNullOrWhiteSpace
   if(!string.IsNullOrEmpty(tValue))

      var map = repo.Item(string.Format("CoverageName LIKE '{0}'",tValue));

      if(map != null)
         tValue = map.theNeededValue;
   }

   return tValue;
}


Building jQuery like controls I'm a beginner at JavaScript and I'm working on building a mini-framework just to become familiar with what good code looks like.  In my application I often have the need for the user to enter in a list of things they want to send to the server for querying purposes.  So currently I have this fiddle.  When I user enters in something into the text box the JS will check to see if that item has already been entered and disallow it if it has, or it will add the text entered into the text box and the user can see what they're going to send to the server (that part is taken out for brevity's sake).  I know this isn't quite there, but I would like to do something like $('#control').holder(optionsForHolderControl) and allow the behavior to be as dynamic as possible.  While I know that I'm not quite there yet, I would like to know what the experts out there would change about my code as it stands right now (organization, is it OK to have these closures in there, do the functions need to be properties, etc).

$(document).ready(function () {
    $('#textButton').click(function () {
        var options = {
            button: '#textButton',
            textBox: '#textBox',
            catcher: '#catcher'
        };
        //fxn call
        stick($(options.textBox), options.catcher);
    });
    //this takes text entered into a text box and makes a div with a strip class
    //and puts it inside the holder div
    function stick(textBox) {
        /***********variables*****************/
        var text = $(textBox).val();
        var holderArray = [];
        //gets all of the text elements
        $('.holder .strip').each(function () {
            holderArray.push($(this).text());
        });
        /*************functions***************/
        function addStrip() {
            var $strip = $('<div>').text(text).appendTo(catcher).addClass('strip');
        }

        function clearTextbox() {
            $(textBox).val('');
            $(textBox).focus();
        }
        //duplicates aren't allowed in the holder container
        if ($.inArray(text, holderArray) != -1) {
            alert('that item has already been entered');
        }
        else {
            addStrip();
        }
        clearTextbox();
    };
});

The starting point in plugin creation is most likely the official documentation here: http://learn.jquery.com/plugins/basic-plugin-creation/

You can start from the very simple plugin that actually contain the function stick as follows:

$.fn.holder = function (options) { /* ... stick's code goes here ... */ return this; };

and this will be the base for your plugin.

Then later you can call

$('#control').holder({ ... some options ... });

And the defined method should be called.

This approach will probably change and below is the review of the code you actually posted, assuming that later you will change it anyway.

The coding style of the presented code is good. I only recommend to remove /***********variables*****************/ and /*************functions***************/ because they are usually become inconsistent with the code blocks below them. Even now the variables are mixed with expressions.

The logic here is unclear:

$('#textButton').click(function () {
    var options = {
        button: '#textButton',
        textBox: '#textBox',
        catcher: '#catcher'
    };
    stick($(options.textBox), options.catcher);
});

this in click handler is the clicked object - $(this) === $(options.textBox) so it it makes sense to move options outside of the click handler and use options.button in $('#textButton').

Also in strip it is not necessary to use $(textBox) because textBox is a jQuery object already.

It is not clear why you have added the functions addStrip and clearTextbox - they are used only once so their code can be just inlined.

$(textBox).val('');
$(textBox).focus();

The power of jQuery allows you to chain the methods (and textBox is already a jQuery object):

textBox.focus().val('');

And the main logic issue - currently the code on the halfway between pure jQuery implementation without model at all and model-driven approach (the code recreates model-like construction on every click). So it is better to select one of the ways and make a step:

remove holderArray and just use $(''.holder .strip'').filter(function () { return $(this).text() === text; }) instead of the array routines
or (and what is better) move holderArray and add code that keeps it consistent with the list. You can check my article on JavaScript MVC for more info on this approach: http://alexatnet.com/articles/model-view-controller-mvc-javascript


Python - insertion sort Series post - I am reading through Introduction to Algorithms, 3rd Edition implementing each algorithm as I read them.

Pretty basic insertion sort. Only considering numbers initially - how could I make this better and/or have better tests?

def insertion_sort(numbs):
    for cur_index in range(1, len(numbs)):
        cur_val = numbs[cur_index]

        lookup_index = cur_index - 1

        while lookup_index >= 0 and numbs[lookup_index] >= cur_val:
            numbs[lookup_index + 1] = numbs[lookup_index]
            lookup_index -= 1
        numbs[lookup_index + 1] = cur_val

    return numbs

def check_insertion_sort(input, expected):
    result = insertion_sort(input)
    if result != expected:
        print "Result={} does not equal expected={} for input={}".format(result, expected, input)
    else:
        print "Success for input={}".format(input)

check_insertion_sort([5, 2, 4, 6, 1, 3], [1, 2, 3, 4, 5, 6])
check_insertion_sort([190, 2, 4, 6, 1, 3], [1, 2, 3, 4, 6, 190])
check_insertion_sort([-190, 2, 4, 6, 1, 3], [-190, 1, 2, 3, 4, 6])
check_insertion_sort([0, 0, 4, 6, 1, 3], [0, 0, 1, 3, 4, 6])
check_insertion_sort([0, 0], [0, 0])
check_insertion_sort([1, 0], [0, 1])

First of all, awesome that you have written tests!

However, your checker is a bit verbose on the matching case. It is often considered good practice for automated tests to be as noise-free as possible. Consider the following outputs:

Success for input=...testcase...
... 300 lines of success ...
Success for input=...testcase...
Result=... does not equal expected=... for input=...
Success for input=...testcase...
... 300 lines of success ...
Success for input=...testcase...

Ok, you have written just a few testcases, but I think you see my point: you'll miss the errors. Better would be just

Result=... does not equal expected=... for input=...

Failures: 1, Success: 601

Better: use a testing framework like unittest, py.test or doctest. Here's how it looks like doctest:

def insertion_sort(numbs):
    """
    >>> insertion_sort([5, 2, 4, 6, 1, 3])
    [1, 2, 3, 4, 5, 6]
    >>> insertion_sort([190, 2, 4, 6, 1, 3])
    [1, 2, 3, 4, 6, 190]
    >>> insertion_sort([-190, 2, 4, 6, 1, 3])
    [-190, 1, 2, 3, 4, 6]
    >>> insertion_sort([0, 0, 4, 6, 1, 3])
    [0, 0, 1, 3, 4, 6]
    >>> insertion_sort([0, 0])
    [0, 0]
    >>> insertion_sort([1, 0])
    [0, 1]
    """
    for cur_index in range(1, len(numbs)):
        cur_val = numbs[cur_index]

        lookup_index = cur_index - 1

        while lookup_index >= 0 and numbs[lookup_index] >= cur_val:
            numbs[lookup_index + 1] = numbs[lookup_index]
            lookup_index -= 1
        numbs[lookup_index + 1] = cur_val

    return numbs

if __name__ == "__main__":
    import doctest
    doctest.testmod()

Now, this is a bit verbose, so my preference would probably to use py.test or nosetests, but the advantage of doctest is that it's built-in, and for simple tests it is sufficient.

Now, for another point. Your sorting is both in-place, and returns the list again.

For instance:

>>> example = [6, 7, 1, 2]
>>> result = insertion_sort(example)
>>> result
[1, 2, 6, 7]
>>> example
[1, 2, 6, 7]

Suggestion: if you do in-place sort, make sure it is documented that the function modifies the input array. In general, I would prefer not returning the result, as to limit confusion.

I can't really see anything wrong with the implementation itself, but I'm not familiar enough with insertion sort.

Using range is fine for short lengths, but as this is a list-sort, you don't really know the values len(numbs) will take. Maybe use xrange instead? (Or better: upgrade to Python 3!).

One thing I would really really like to advice is switching to Python 3 for learning. Python 2 is only supported up-to 2020, and a lot of people would be happy if more people learn Python 3 instead.


Synchronization algorithm for deterministic context switching between threads Curious how sqlite3 would handle different race conditions in a multithreaded environment, I created a simple module called deterministic for serializing the execution of multiple threads that lets the programmer explicitly program in the spots where thread-switching happens.  Hopefully this example usage will explain the idea better:

import deterministic

def main():
    deterministic.run(consumer0, consumer1, consumer2)

def consumer0():
    print('consumer 0 yields to consumer 2')
    yield 2
    print('consumer 0 exits')

def consumer1():
    print('consumer 1 yields to next consumer')
    yield
    print('consumer 1 exits')

def consumer2():
    print('consumer 2 yields to consumer 1')
    yield 1
    print('consumer 2 yields to next consumer')
    yield
    print('consumer 2 exits')

if __name__ == '__main__':
    main()

Which outputs:

consumer 0 yields to consumer 2
consumer 2 yields to consumer 1
consumer 1 yields to next consumer
consumer 2 yields to next consumer
consumer 0 exits
consumer 1 exits
consumer 2 exits

Again, the idea is to experiment with how real multi-threaded code behaves, but in a highly controller environment.

The actual code follows below.  Some specific feedback I'm hoping for:

Is there a synchronization primitive that better models what I'm trying to do than simple locking?
Even if not, is there a better way to structure the code to make it easier to reason about?
Finally, general coding and style advice is more than welcome

import threading, time, types

def run(*consumers):
    sync = Syncrhonizer(len(consumers))
    _run_threads(
        threading.Thread(target=DeterministicRunner(sync, i, c).run)
        for i, c in enumerate(consumers)
    )

class Syncrhonizer:
    def __init__(self, num_instances):
        self._instances = list(range(num_instances))

        self._lock = threading.Lock()
        self._current = 0
        self._want_abort = False

    def lock(self, idx):
        while True:
            self._lock.acquire()

            if self._want_abort:
                self._lock.release()
                raise SyncrhonizerAbort()

            if self._current == idx:
                return

            self._lock.release()
            time.sleep(0.001)

    def die(self):
        self._want_abort = True
        if self._lock.locked():
            self._lock.release()

    def yield_to(self, idx=None):
        if idx is None:
            idx = self._get_next_instance()

        if idx not in self._instances:
            self.die()
            raise Exception('Can not yield to non-existent instance "%s"' % idx)

        self._current = idx
        self._lock.release()

    def _get_next_instance(self):
        try:
            return next(n for n in self._instances if n > self._current)
        except StopIteration:
            return self._instances[0]

    def remove(self, idx):
        self._instances.remove(idx)
        if self._lock.locked() and self._instances:
            self.yield_to()

class SyncrhonizerAbort(Exception):
    """Abort execution because of an issue in another thread"""

class DeterministicRunner:
    def __init__(self, syncer, idx, method):
        self._syncer = syncer
        self._idx = idx

        self._g = method()
        if not isinstance(self._g, types.GeneratorType):
            raise Exception('Not a generator')

    def run(self):
        try:
            while True:
                self._syncer.lock(self._idx)

                n = next(self._g)

                self._syncer.yield_to(n)
        except StopIteration:
            pass
        finally:
            self._syncer.remove(self._idx)

def _run_threads(threads):
    threads = list(threads)

    for t in threads:
        t.start()

    for t in threads:
        t.join()

Threading

Trying to use time.sleep with threading.Locks usually means you’d be better of using a threading.Condition instead. It lets you wait passively instead of using an active while True: ... time.sleep(0.001).

However, in your case, you could simplify further using a threading.Event:

class Synchronizer:
    def __init__(self, num_instances):
        ...
        self._lock = threading.Event()

    def lock(self, idx):
        while self._current != idx:
            self._lock.wait()
            if self._want_abort:
                raise SynchronizerAbort()
        self._lock.clear()

    def die(self):
        self._want_abort = True
        self._lock.set()

    def yield_to(self, idx=None):
        ...
        self._current = idx
        self._lock.set()

Every thread, except the 0\$^{th}\$ one, will enter the while self._current != idx loop and wait for the Event to become available. Once available after thread 0 exits yield_to, only the one with the right ID will exit the loop and all the others will wait on the Event to be set again.

Now what have we here? Every thread needing to wait at a synchronization point, loop until a condition becomes available, and check for a flag that indicates if an other thread went through an error… Time to change for a threading.Barrier (Python 3.2 and up).

There is a drawback to using barriers, in that they must be reached by all threads before they are all released at once. Meaning we can't let DeterministicRunner.run exit before all threads are done. So we must have Synchronizer.lock return a boolean value indicating if we are running out of instances or not.

class Synchronizer:
    def __init__(self, num_instances):
        ...
        self._barrier = threading.Barrier(num_instances)

    def lock(self, idx):
        while True: # Force any thread to enter, independently of its idx
            self._barrier.wait()
            if not self._instances:
                # All threads exit at once when everything is done
                return False
            if self._current == idx:
                # Only the active thread can exit otherwise
                return True

    # No need of `die` anymore
    def yield_to(self, idx=None):
        if idx is None:
            idx = self._get_next_instance()

        if idx not in self._instances:
            self._barrier.abort() # Make all other thread abort with BrokenBarrierError
            raise Exception('Can not yield to non-existent instance "%s"' % idx)

        self._current = idx

Coding Style

It is recommended that

import threading, time, types

is written

import threading
import time
import types

However, since you’re making very little use of time and types I’d rather recommend the from .. import .. form.

Other than that, there is little to talk about. It is mostly a matter of taste:

converting the generator parameter of _run_threads into a list could be done using the variable-length argument syntax:

def _run_threads(*threads):
    for thread in threads:
        thread.start()

    for thread in threads:
        thread.join()

and call it with _run_threads(*(<generator_expression>)).
DeterministicRunner.run might be better written with the try within the while to reduce the amount of protected code:

def run(self):
    while True:
        self._syncer.lock(self.idx)
        try:
            n = next(self._g)
        except StopIteration:
            self._syncer.remove(self.idx)
            break
        else:
            self._syncer.yield_to(n)

note that when using barriers, you have to while self._syncer.lock(self.idx): and not break.
Synchronizer._get_next_instance might be more readable using plain list iteration:

def _get_next_instance(self):
    for n in self._instances:
        if n > self._current:
            return n
    return self._instances[0]

I'd check that the methods returns an object that has a __next__ method rather than being of type GeneratorType. It allows for more flexibility for anyone using your API and wanting to provide a custom class rather than generator functions.

You are also using a custom made exception for cleanup purposes but you use generic Exceptions for errors in using your API. I'd go the other way around and use custom exceptions that are easier to handle for real errors and generic ones for cleanup.

Proposed Improvements

import threading

class SynchronizeError(Exception):
    """Abort execution because a thread does not respect the API"""

class Synchronizer:
    def __init__(self, num_instances):
        self._instances = list(range(num_instances))
        self._current = 0

        self._barrier = threading.Barrier(num_instances)

    def lock(self, idx):
        while True:
            self._barrier.wait()
            if not self._instances:
                return False
            if self._current == idx:
                return True

    def yield_to(self, idx=None):
        if idx is None:
            idx = self._get_next_instance()

        if idx not in self._instances:
            self._barrier.abort() # All other threads get BrokenBarrierError
            raise SynchronizeError(
                'Can not yield to non-existent instance "{}"'.format(idx))

        self._current = idx

    def _get_next_instance(self):
        if self._current >= self._instances[-1]:
            return self._instances[0]
        for n in self._instances:
            if n > self._current:
                return n

    def remove(self, idx):
        self._instances.remove(idx)
        if self._instances:
            self.yield_to()

class DeterministicRunner:
    def __init__(self, syncer, idx, method):
        self._syncer = syncer
        self._idx = idx

        self._g = method()
        if not hasattr(self._g, '__next__'):
            raise SynchronizeError('Not a generator')

    def run(self):
        while self._syncer.lock(self._idx):
            try:
                n = next(self._g)
            except StopIteration:
                self._syncer.remove(self._idx)
            else:
                self._syncer.yield_to(n)

def _run_threads(*threads):
    for t in threads:
        t.start()

    for t in threads:
        t.join()

def run(*consumers):
    sync = Synchronizer(len(consumers))
    _run_threads(*(
        threading.Thread(target=DeterministicRunner(sync, i, c).run)
        for i, c in enumerate(consumers)
    ))


Counting Increasing Subsequences of size K recursive The original description of the problem can be found here, its input here and its expected output here.

The problem:

  Given a integer sequence a1 ... an (-10000
  \$\leq\$ ai \$\leq\$ 10000). Where n (1 \$\leq\$ n
  \$\leq\$ 100),the size of the sequence, is a given number.

  Find the number of increasing subsequences with length equals to a
  given size k (1 \$\leq\$ k \$\leq\$ n \$\leq\$ 100).

  An increasing subsequence ai1 ... aix of a
  sequence a1 ... an is defined as
  aix-1, \$<\$ aix for all 1 \$<\$ x \$\leq\$ n).

  The input has several testcases. The first line contains the numbers
  n and k. The second line contains a sequence of length n.  0 0 ends the input. The sequence will be given in a way that the
  subsequences count can't be bigger than a signed number of 64 bits.

  Example:

10 5
1 2 3 4 5 6 7 8 9 10
3 2
3 2 1
0 0

  The output is the number of increasing subsequences:

  Example for the above input:

252
0

My solution uses a recursive idea. The sequence values are stored in an array of size n+1 with a \$-\infty\$ in the 0 index of the array. After this, a function goes from de 0 index to de n position recursively. And a comparison is made to check if the current value (picked) is less than the next value.

#include <stdio.h>
#include <string.h>
#include <limits.h>

long long memoization[102][102];

long long LCIS(int *S,int len,int picked,int LCISlen, int K){
    if(memoization[picked][LCISlen])
        return memoization[picked][LCISlen];

    long long q = 0;

    int i;
    for (i = picked + 1; i < len; i++) {
        if(S[picked] < S[i] && LCISlen < K)
            q += LCIS(S, len, i, LCISlen + 1, K);
        else if(S[picked] < S[i] && LCISlen == K)
            q++;
    }

    return memoization[picked][LCISlen] = q;
}

int main(){

    int N,K;
    while(scanf("%d %d",&N,&K), (N != 0) && (K != 0)){

        int i, S[N+1];

        S[0] = INT_MIN;

        for (i = 1; i <= N; i++)
            scanf("%d",&S[i]);

        memset(memoization,0,sizeof memoization);

        printf("%lld\n",LCIS(S,N+1,0,1,K));
    }
}

The solution got accepted by the judge, but it takes nearly 1 sec to give the answer. Other solutions take 0 seconds. This made me think that this solution isn't a good one.

Concept

You have called this a "Longest Increasing Subsequence" problem, but it's not, is it? It's counting all possible sequences of a specific length, not just the longest. As a result, I think you may have confused yourself in a few ways....

Naming

Your variable and method names are ... horrible. Variables should always be useful names, and, while N and K are referenced in the problem specification, S is not. Additionally, upper-case names are not encouraged... so you should have n and k instead. The method name LCIS is also off, it should be longest_increasing_sequence, and where does the C in LCIS come from?

Standards

Your code compiles OK (with a warning about the missing return in the main method) with basic C compiler. This makes me think you intend to use C99 which compiles it without warnings. But, in C99, you should declare your variables where they are used, not at the beginning of the blocks. For example, you should declare i inside the for-loop.

Globals

Globals are to be avoided.

Algorithm

Your concept of memoization is good, but I am not convinced the recursion is doing what you think it does. I am not convinced the memoization is used at all effectively. Frankly, I struggled to follow your algorithm, in part because memoization does not tell me much about what is being memoized...

I started hacking your code, and ended up restructuring it as a set of nested loops. The resulting complexity is somewhere around \$O(n^3)\$, which sounds really bad, but, for the problem given I am not sure can be improved on... A large part of the complexity is in the determination of the paths, not the combinatorics required to compute the count. I am likely wrong here somewhere, but my performance of the resulting code is more than good enough to be in the right ballpark...

So, I use memoization as well, the system I use is, for each node, starting from the last, to record the number of increasing paths, and how long they are, to the end of the sequence. The last node, for example, can be the last node in the sequence, and thus has a count of 1 in path-length 0 (0 because I zero-base the path length - 0 really means "there is 1 path starting from this node where this node is the last member too").

With that system, I can then work backwards through the sequence, and simply add up the paths that have the right length.

The critical method looks like:

long long count_sequences(const int *seq, const int size, const int path) {
    long long pathcounts[size][size];

    memset(pathcounts, 0, sizeof(pathcounts[0][0]) * size * size);

    long long count = 0;

    // start from end and work back.
    for (int i = size - 1; i >= 0; i--) {
        //printf("  start %d at %d\n", seq[i], i);
        // always 1 step from here to end.
        pathcounts[i][0] = 1;
        for (int j = i + 1; j < size; j++) {

            // check for increasing sequence
            if (seq[j] > seq[i]) {
                //printf("    next %d at %d\n", seq[j], j);

                for (int k = size - j; k >= 0; k--) {
                    pathcounts[i][k + 1] += pathcounts[j][k];
                }
            }
        }
        // zero-based path length - must subtract 1
        count += pathcounts[i][path - 1];
    }

    return count;
}

Note how, at each level we have to look ahead for increasing members, but, we only need to look in to the memoization for increasing members to accumulate path lengths.

Note also how we can accumulate the count as we retreat?

The entire program I have is:

#include <stdio.h>
#include <string.h>
#include <limits.h>

long long count_sequences(const int *seq, const int size, const int path) {
    long long pathcounts[size][size];

    memset(pathcounts, 0, sizeof(pathcounts[0][0]) * size * size);

    long long count = 0;

    // start from end and work back.
    for (int i = size - 1; i >= 0; i--) {
        //printf("  start %d at %d\n", seq[i], i);
        // always 1 step from here to end.
        pathcounts[i][0] = 1;
        for (int j = i + 1; j < size; j++) {

            // check for increasing sequence
            if (seq[j] > seq[i]) {
                //printf("    next %d at %d\n", seq[j], j);

                for (int k = size - j; k >= 0; k--) {
                    pathcounts[i][k + 1] += pathcounts[j][k];
                }
            }
        }
        // zero-based path length - must subtract 1
        count += pathcounts[i][path - 1];
    }

    return count;
}

int main(){

    int n,k;
    while(scanf("%d %d",&n,&k), n != 0 && k != 0) {

        int sequence[n];

        for (int i = 0; i < n; i++) {
            scanf("%d", &sequence[i]);
        }

        long long count = count_sequences(sequence, n, k);

        printf("%lld\n", count);
    }
}

On my computer, with -Wall and -O3, it runs the full dataset in 0.01 seconds.