Embedding python into C++ program I was following this tutorial from official python documentation site, but found its examples quite simplistic, so I've built my own wrapper.
It does what I intend it to do, but I wonder if this code is good or how it can be improved.

app.h

#pragma once

#include <iostream>
#include <string>

#include <Python.h>

class App {
public:
  App();
  ~App();

  void Run();

private:
  static const std::string PY_CLASS_NAME;
  static const std::string PY_MODULE_NAME;

  static std::string _GetString(PyObject* obj);

  PyObject* _counter;
};

app.cc

#include "app.h"

const std::string App::PY_CLASS_NAME = "Counter";
const std::string App::PY_MODULE_NAME = "lib";

App::App() {
  Py_SetProgramName(Py_DecodeLocale("App", NULL));
  Py_Initialize();
  PyRun_SimpleString("import sys");
  PyRun_SimpleString("sys.path.append(\".\")");

  PyObject* name = PyUnicode_DecodeFSDefault(PY_MODULE_NAME.data());
  PyObject* module = PyImport_Import(name);

  PyObject* consturctor = PyObject_GetAttrString(module, PY_CLASS_NAME.data());
  _counter = PyObject_CallObject(consturctor, NULL);
  std::cout << _GetString(_counter) << "\n";

  Py_XDECREF(consturctor);
  Py_XDECREF(module);
  Py_XDECREF(name);
}

App::~App() {
  Py_Finalize();
}

void App::Run() {
  PyObject* res;
  for (auto i=0; i<10; i++) {
    PyObject_CallMethod(_counter, "Inc", NULL);
    res = PyObject_CallMethod(_counter, "GetValue", NULL);
    std::cout << _GetString(res) << "\n";
  }
  PyObject_CallMethod(_counter, "Drop", NULL);
  res = PyObject_CallMethod(_counter, "GetValue", NULL);
  std::cout << _GetString(res) << "\n";
}

std::string App::_GetString(PyObject* obj) {
  PyObject* repr = PyObject_Repr(obj);
  PyObject* str = PyUnicode_AsEncodedString(repr, "utf-8", "~E~");
  const char* bytes = PyBytes_AS_STRING(str);

  Py_XDECREF(repr);
  Py_XDECREF(str);
  return std::string(bytes);
}

lib.py

class Counter(object):
    def __init__(self):
        super().__init__()

        self._c = 0

    def __repr__(self):
        return "Simple Counter" + str(self._c)

    def Drop(self):
        self._c = 0

    def GetValue(self):
        return str(self._c)

    def Inc(self):
        self._c += 1

app.h

#pragma once

This is not portable, and does not work as well as advertised. You should use include guards instead.

#include <iostream>
#include <string>

#include <Python.h>

One of the biggest problems currently in C++ is slow compile times. There are several things you can do to lower compile times, but one of the most important ones is to not include what you don't need to include - especially in headers. Obviously that won't be a problem in this small program, but it doesn't hurt to practice best practices.

This header file doesn't need any of these three includes.

<iostream>: you don't actually use anything from the <iostream> in the header.
<string> : you need this for std::string... but as I'll explain later, you don't actually need any std::string stuff in the header.
<Python.h>: because you only use a PyObject pointer in the header, you don't actually need to include it here.

So you can remove all three of these includes from the header, and move them to app.cc, where they're actually needed.

class App {
public:
  App();
  ~App();

Have you heard of the rule of three/five/zero? Since your class requires you to write a destructor, that means it almost certainly requires you to write a copy constructor, move constructor, copy assignment operator, and move assignment operator. And in fact, your class does need you to do these things.

Now you probably don't want your App class to be copyable... that doesn't really make any sense. So for the copy ops all you need is:

App(App const&) = delete;
App& operator=(App const&) = delete;

All that's left is move ops. Should your class be movable? The answer to this question is almost always "yes", unless you have very complex technical reasons why not... which you don't here.

Unfortunately, as written your class's move ops can't be defaulted. You'll have to handle moving manually. The easiest way to do that is to define _counter to be:

PyObject* _counter = nullptr;

And then your move ops are like:

App(App&& other) noexcept
{
    std::swap(_counter, other._counter);
}

App& operator=(App&& other) noexcept
{
    std::swap(_counter, other._counter);
    return *this;
}

But to avoid cleaning up Python multiple times, you need to add a check that _counter is not nullptr in the destructor:

~App()
{
    if (_counter)
        Py_Finalize();
}

That's all you need to make the class safe according to the rule of five.

static const std::string PY_CLASS_NAME;
static const std::string PY_MODULE_NAME;

These don't really need to be class members. They're only used in app.cc, and don't seem necessary anywhere else.

static std::string _GetString(PyObject* obj);

This also doesn't need to be defined in class. It's only used in app.cc. If you move this and the two static constants into app.cc, you don't need to include <string> in the header.

But the main problem with this function is that _GetString is an illegal identifier. You cannot have an identifier that starts with an underscore and a capital letter anywhere. The rules for identifiers are pretty complex, so when I teach I generally advise these simple rules:

Never start with an underscore.
Never use two underscores together.

(_counter is technically okay because it's an underscore followed by a lowercase letter, and because of its scope. But I generally advise using something like counter_.)

app.cc

const std::string App::PY_CLASS_NAME = "Counter";
const std::string App::PY_MODULE_NAME = "lib";

Strings work here, but this is actually a job that might be better handle by string views. String views are constexpr and no-fail - ideal for constants. And the Python library supports non-null-terminated strings, partially. Instead of PyUnicode_DecodeFSDefault() you'd use PyUnicode_DecodeFSDefaultAndSize(). Unfortunately, there's no equivalent for PyObject_GetAttrString(), so you'll need a string there. But at the very least PY_CLASS_NAME can be:

namespace {

constexpr std::string_view PY_CLASS_NAME = "Counter";

} // anonymous namespace

The anonymous namespace is to make sure PY_CLASS_NAME is only visible in app.cc.

On to the constructor:

Py_SetProgramName(Py_DecodeLocale("App", NULL));

This is an issue generally, but you're not doing any error checking here. Py_DecodeLocale() can return nullptr if it fails for a number of reasons. You should really check for that, and handle it - probably by throwing an exception.

There's another issue here: resource management. You're doing everything the old-fashioned C way... mostly because the Python library is a C API. Generally when you're using a C API, you should make some proper C++ RAII classes for resource management.

For example, the memory allocated by Py_DecodeLocale() needs to be freed by a call to PyMem_RawFree()... but you never do that.

The smart thing to do is to create a PyMem_RawFree() deleter:

struct pymem_rawfree_deleter
{
    auto operator()(void* p) noexcept
    {
        PyMem_RawFree(p);
    }
};

Then you can store the program name in a unique_ptr:

namespace {

std::unique_ptr<wchar_t[], pymem_rawfree_deleter> p_program_name;

} // anonymous namespace

And now the memory will no longer leak, because it will automatically be freed.

Now the code becomes something like:

p_program_name.reset(Py_DecodeLocale("App", nullptr));
if (!p_program_name)
    throw std::runtime_error{"failed decoding program name"};
Py_SetProgramName(p_program_name.get());

Also note: NULL is old-fashioned and problematic. Use nullptr.

Py_Initialize();

Another issue with the constructor is that you don't do any checking to make sure it hasn't been called multiple times. What happens if someone does?:

App a1;
App a2;

One answer is that Py_Initialize() will be called twice. This doesn't seem to be a problem, because calling it a second time is guaranteed to do nothing. However, when one of those is destroyed, it will call Py_Finalize()... and now if you try to do anything with the second one, who knows what will happen - probably a crash.

What you'd need to do is make sure that only a single App object is created at a time. You could use a static flag for that... but you kinda already have one with p_program_name. If p_program_name is not nullptr, it must have been set in the constructor, so you know a second App is being constructed. What you can do is test p_program_name first thing in the constructor, and if it is not nullptr, throw an exception. And then you can set it to nullptr in the destructor after the call to Py_Finalize().

PyRun_SimpleString("import sys");
PyRun_SimpleString("sys.path.append(\".\")");

This might benefit from a raw string constant:

constexpr auto PY_INIT_SYS_PATH_CODE = R"~python~(

import sys

sys.path.append(".")

)~python~";

PyRun_SimpleString(PY_INIT_SYS_PATH_CODE);

Next up is more resource management worries:

PyObject* name = PyUnicode_DecodeFSDefault(PY_MODULE_NAME.data());

// ...

Py_XDECREF(name);

The C++ way of doing this is to define a deleter, and use unique_ptr:

struct pyobject_deleter
{
    auto operator()(PyObject* p) noexcept
    {
        Py_XDECREF(p);
    }
};

using pyobject_ptr = std::unique_ptr<PyObject, pyobject_deleter>;

// ...

auto name = pyobject_ptr{PyUnicode_DecodeFSDefault(PY_MODULE_NAME.data())};

And there's no need for all the calls to Py_XDECREF().

Once again, there's no error checking done.

void App::Run() {
    PyObject* res;

It's generally bad practice to declare variables before they're needed. Now you do this so you can reuse res after the loop is done, but there's no real need for this.

for (auto i=0; i<10; i++) {
    PyObject_CallMethod(_counter, "Inc", NULL);
    res = PyObject_CallMethod(_counter, "GetValue", NULL);
    std::cout << _GetString(res) << "\n";
}

I'm not 100% sure with the Python API, but I believe you are leaking a lot of stuff here. Each time you call PyObject_CallMethod() it's returning a PyObject*, whether you catch it or not. That PyObject* is nullptr on failure (which you might want to check for) but otherwise it's something you need to consider.

This would be really easy with the pyobject_ptr I showed above:

for (auto i = 0; i < 10; ++i)
{
    auto r1 = pyobject_ptr{PyObject_CallMethod(_counter, "Inc", nullptr)};
    auto res = pyobject_ptr{PyObject_CallMethod(_counter, "GetValue", nullptr)};
    // should at least check res for nullptr
    std::cout << _GetString(res.get()) << '\n';
    // cleanup is automatic
}

And of course, all the same for the bit of code after the loop.

std::string App::_GetString(PyObject* obj) {
    PyObject* repr = PyObject_Repr(obj);
    PyObject* str = PyUnicode_AsEncodedString(repr, "utf-8", "~E~");
    const char* bytes = PyBytes_AS_STRING(str);

    Py_XDECREF(repr);
    Py_XDECREF(str);
    return std::string(bytes);
}

I've already mentioned the problem with the name, but I believe this function has a more serious bug in it. You get the pointer to the string representation of str as bytes, then you decrement the reference count of str... which will probably destroy it. But then you use bytes again... which probably points to freed memory. Normally this would trigger a segmentation fault, but I suspect that Python is managing its memory as a pool, so the OS doesn't detect the use-after-free.

The immediate fix for this is to simply construct the string right away:

std::string App::_GetString(PyObject* obj) {
    PyObject* repr = PyObject_Repr(obj);
    PyObject* str = PyUnicode_AsEncodedString(repr, "utf-8", "~E~");
    const char* bytes = PyBytes_AS_STRING(str);

    auto res = std::string{bytes};

    Py_XDECREF(repr);
    Py_XDECREF(str);

    return res;
}

This will copy the data from bytes right away, before it's released.

But this becomes so much simpler if you use smart pointers:

std::string App::_GetString(PyObject* obj) {
    // Of course, you should be doing error checking....
    auto repr = pyobject_ptr{PyObject_Repr(obj)};
    auto str = pyobject_ptr{PyUnicode_AsEncodedString(repr.get(), "utf-8", "~E~")};
    const char* bytes = PyBytes_AS_STRING(str.get());

    return std::string{bytes};
}

Summary

Try to keep headers as minimal as possible:

Don't include stuff that isn't necessary.
Try to move as much as possible into implementation files especially stuff that's mostly implementation details.

Always respect the rule of zero/three/five.
Consider reentrancy - what happens when a function is called multiple times or a class is instantiated multiple times.
Error checking!!!
Use RAII, even if you have to roll your own classes to do it (which you often do when working with C APIs).

And lesser points:

Don't use NULL. Use nullptr.
Watch out for illegal identifiers. Simple advice: don't start identifiers with an underscore.


String find and replace method optimization I am trying to find a specific header string from different Maps (LEDES1998Bheaders, LEDES98BIheaders and LEDES98BI_V2headers) in an errorMessage depends on the parcel type and if the errorMessage has the specific header string I need to replace it with the corresponding value.

public class ErrorMessageConverter {

    private static Map<String, String> LEDES1998Bheaders = new HashMap<>();
    private static Map<String, String> LEDES98BIheaders = new HashMap<>();
    private static Map<String, String> LEDES98BI_V2headers = new HashMap<>();

    static {
                LEDES1998Bheaders.put("(\\binv_date\\b|\\bINVOICE DATE\\b)", "INVOICE DATE");
    LEDES1998Bheaders.put("\\binv_id\\b", "INVOICE NUMBER");
    LEDES1998Bheaders.put("\\bcl_id\\b", "CLIENT ID");
    LEDES1998Bheaders.put("\\blf_matter_id\\b", "LAW FIRM MATTER ID");
    LEDES1998Bheaders.put("\\bUNITS\\b", "LINE ITEM NUMBER OF UNITS");
    LEDES1998Bheaders.put("(\\bBaseRate\\b|\\bRate\\b|\\btk_rate\\b)", "LINE ITEM UNIT COST");

    LEDES98BIheaders.put("\\blf_address/address_info/address_1\\b", "LAW FIRM ADDRESS 1");
    LEDES98BIheaders.put("\\blf_address/address_info/city\\b", "LAW FIRM CITY");
    LEDES98BIheaders.put("\\btax_rate\\b", "LINE ITEM TAX RATE");
    LEDES98BIheaders.put("\\btax_on_charge\\b", "LINE ITEM TAX TOTAL");
    LEDES98BIheaders.put("\\btax_type\\b", "LINE ITEM TAX TYPE");

    LEDES98BI_V2headers.put("\\binv_reported_tax_total\\b", "INVOICE REPORTED TAX TOTAL");
    LEDES98BI_V2headers.put("\\binv_reported_tax_currency\\b", "INVOICE TAX CURRENCY");

    }

    public static String toUserFriendlyErrorMessage(String parcelType, String message) {

        if (parcelType.equals("LEDES1998B")) {
            return updateErrorMessage(message, LEDES1998Bheaders);
        }
       else if (parcelType.equals("LEDES98BI")) {
            message = updateErrorMessage(message, LEDES1998Bheaders);
            return updateErrorMessage(message, LEDES98BIheaders);
        }
       else if (parcelType.equals("LEDES98BI V2")) {
            message = updateErrorMessage(message, LEDES1998Bheaders);
            message = updateErrorMessage(message, LEDES98BIheaders);
            return updateErrorMessage(message, LEDES98BI_V2headers);
        }
        return message;

    }

    private static String updateErrorMessage(String msg, Map<String, String> invHeaders) {
        Pattern pattern;
    for (String key : invHeaders.keySet()) {
        pattern = Pattern.compile(key);
        if (pattern.matcher(msg).find()) {
            msg = msg.replaceAll(key, invHeaders.get(key));
        }
    }
    return msg;
    }
}

Below are couple of sample error messages:

String errorMesage1 = "Line 3 : Could not parse inv_date value"
String errorMesage2 = "Line : 1 BaseRate is a required field"

Can this method simplified further in java 8 using filters/lambda expressions?

Your code looks fairly effective, but it could be a bunch more efficient if you rearrange the code a bit, do some pre-processing, and reduce your run-time checks. Let's go through that in order....

I recommend a 'set up' stage to your code. Consider an ideal situation at runtime, the code would look something like:

public static String toUserFriendlyErrorMessage(String parcelType, String message) {

    for (Rule rule : getRules(parcelType)) {
        message = rule.process(message);
    }
    return message;
}

That would be a neat solution, where you have a bunch of rules that you know apply to the messages of a particular type, and that you can then just apply whichever ones are appropriate. That would be the most efficient at use-time too.

How would you implement that? Using some pre-processing, of course, and a new Rule class.

This preprocessing is key, and the features you should look for here are:

the rules are sorted at pre-process time.
some rules are in multiple sets of operations.
Pattern instances are compiled just once, not at use-time.

Your rules are convenient in the sense that the parcelType values seems to have an 'extension' type system:

LEDES1998B is rules LEDES1998Bheaders
LEDES98BI is the same as LEDES1998B plus the rules in LEDES98BIheaders
LEDES98BI V2 is the same as LEDES98BI (and thus LEDES1998B) but also includes LEDES98BI_V2headers

How would I recommend preprocessing things?

private static final class Rule {
    private final Pattern key;
    private final String replacement;

    Rule(String search, String rep) {
        key = Pattern.compile(search);
        replacement = Matcher.quoteReplacement(rep);
    }

    String process(String msg) {
        return key.matcher(msg).replaceAll(replacement);
    }
}

So, that rule class is simple enough... how to add them together?

private static final Map<String, Rule[]> RULE_MAP = buildMap();

private static final Map<String, Rule[]> buildMap() {
    Map<String, Rule[]> result = new HashMap<>();
    List<Rule> rules = new ArrayList<>();

    addRules(rules, LEDES1998Bheaders);
    result.put("LEDES1998B", rules.toArray(new Rule[rules.size()]));

    addRules(rules, LEDES98BIheaders);
    result.put("LEDES98BI", rules.toArray(new Rule[rules.size()]));

    addRules(rules, LEDES98BI_V2headers);
    result.put("LEDES98BI V2", rules.toArray(new Rule[rules.size()]));

    return result;
}

private static final void addRules(List<Rule> target, Map<String,String> sources) {
    for (Map.Entry<String,String> me : sources.entrySet()) {
        target.add(new Rule(me.getKey(), me.getValue()));
    }
}

Now, your last part is the getRules method:

private static final Rule[] getRules(String parcelType) {
    Rule[] rules = RULE_MAP.get(parcelType);
    if (rules == null) {
        throw new IllegalStateException("No rules set for type " + parcelType);
    }
    return rules;
}

Now your code should go smoothly.


Split a list into a dictionary I am trying to split my list into a dictionary and I wrote a function for it. It takes in a list and gets the list length. If the length of the list is 253 , it creates a dictionary with 26 keys - this is calculated by rounding up the number to a higher 10 (250 will create 25 keys, 251-259 will create 26 keys). Each key will store a chuck of the original list (as of now I am storing 10 elements per list).

I want to know if I can improve it more:

def limit_files(file_list, at_a_time=10):

    l = file_list
    n = len(l)
    d, r = divmod(n, at_a_time)

    num_keys = d + 1 if r else d
    slice = n // num_keys
    vals = (l[i:i+slice] for i in range(0, n, slice+1))
    dct = dict(zip(range(1,num_keys+1),vals))
    return (dct)

First of all there is a bug in your code, range(0, n, slice+1) should be range(0, n, slice) otherwise you'll miss every (slice+1)th item.
I don't understand why you re-assigned l = file_list to l here, note that simple assignments in Python don't create a copy of the object. Also don't use l as a variable name because it looks like 1(one) in many fonts.
If you're using Python 2 then use xrange() instead of range().
dict(zip(range(1,num_keys+1),vals)) can be replace with a simpler dict(enumerate(vals, start=1))
The parenthesis around (dict) in return statement are unnecessary.
slice is a built-in function, so try not to use it as a variable name. slice_size is one alternative.

Also I don't understand why you need a dict here, you can simply use a list of lists. Then the only difference will be that you'll have to start indexing at 0. Indexing is very faster than dictionary key-lookups and will also make sure the order of items is maintained.


Generic pure functions memoization I like to create tools for memoization since it can sometimes be useful. I recently created a generic tool for memoizing the results of pure functions.

Here is an example of how it works:

// Dummy function
auto func(int a, bool b)
    -> std::string
{
    // Some heavy computations
}

int main()
{
    auto memof_f = memoized(func);
    auto a = memo_f(5, true);
    auto b = memo_f(8, false);
    // ...
}

In this example, memo_f is a wrapper that calls func and memoizes its results. When called, it checks whether the result for the given arguments is in cache and computes it only if it is not.

Here is the implementation of memoized:

template<typename Ret, typename... Args>
class MemoizedFunction
{
    public:
        MemoizedFunction(const std::function<Ret(Args...)>& func):
            _func(func)
        {}

        auto operator()(Args... args)
            -> Ret
        {
            auto tuple_args = std::make_tuple(args...);
            if (not _memory.count(tuple_args))
            {
                auto res = _func(args...);
                _memory[tuple_args] = res;
                return res;
            }
            return _memory[tuple_args];
        }

    private:
        // Stored function
        std::function<Ret(Args...)> _func;
        // Map containing the pairs args/return
        std::unordered_map<std::tuple<Args...>, Ret> _memory;
};

template<typename Ret, typename... Args>
auto memoized(Ret (*func)(Args...))
    -> MemoizedFunction<Ret, Args...>
{
    return { std::function<Ret(Args...)>(func) };
}

Also, it's not of utmost importance, but I need these little functions in order to hash a tuple:

template<typename First, typename... Rest>
auto hash_all(First first, Rest... rest)
    -> std::size_t
{
    auto seed = std::hash<First>()(first);
    seed ^= hash_all(rest...);
    return seed;
}

template<typename First, typename Second>
auto hash_all(First first, Second second)
    -> std::size_t
{
    return std::hash<First>()(first) ^ std::hash<Second>()(second);
}

namespace std
{
    // Hash function for tuples
    template<typename... Args>
    struct hash<std::tuple<Args...>>
    {
        auto operator()(const std::tuple<Args...>& args) const
            -> std::size_t
        {
            return apply(hash_all<Args...>, args);
        }
    };
}

Note: the function apply is a function that uses the elements of a tuple as parameters for a given function, as proposed in N3802.

My question is simple: since this a low-level tool, which could be used in several contexts, is there any way to improve its performance at least a little bit? And while we are at it, do not hesitate to give tips for that construct to be more versatile too :)

Note: this class is only intended to work for pure (or "well-behaved" as of N3744) functions.

Thanks to an answer to a similar question on StackOverflow (thanks @200_success), I improved the implementation of operator(). Using _memory.count is pretty inefficient compared to _memory.find since the latter will stop once the first corresponding key is found:

auto operator()(Args... args)
    -> Ret
{
    const auto t_args = std::make_tuple(args...);
    auto it = _memory.find(t_args);
    if (it == _memory.cend())
    {
        using val_t = typename std::unordered_map<std::tuple<Args...>, Ret>::value_type;
        it = _memory.insert(val_t(t_args, _func(args...))).first;
    }
    return it->second;
}

While the solution above is indeed better than what I had, I managed to improve it even more: the results of _func should be directly constructed into _memory thanks to the piecewise construction of std::pair:

template<typename Ret, typename... Args>
auto MemoizedFunction<Ret, Args...>::operator()(Args... args)
    -> Ret
{
    const auto t_args = std::make_tuple(args...);
    auto it = _memory.find(t_args);
    if (it == _memory.cend())
    {
        it = _memory.emplace(std::piecewise_construct,
                             std::forward_as_tuple(t_args),
                             std::forward_as_tuple(_func(args...))
                        ).first;
    }
    return it->second;
}

Now, operator() is far better than it originally was. I don't know whether it is still possible to improve it or not though.


Code to sort based on quantities of a data member occuring The setup is that I have a vector of a class, and I need to sort it by the quantity of a data member value, not the value itself. The below code exhibits the correct behavior, and part of that behavior is "stable" sorting where the order of elements should be preserved when possible.
Between the /*** Starting here ***/ and /*** Ending here ***/ comments, are there some Standard Library functions and/or classes that can be substituted? Per the tag, the code needs to exist in C++14. Yes, I realize that the MRE is written with C++17 (maybe some 20, but I compiled it as C++17), but the code in question [hopefully] isn't.
#include <algorithm>
#include <iostream>
#include <iterator>
#include <random>
#include <utility>
#include <vector>

struct Foo {
  int fizz = 0;
  int buzz = 0;
};

std::ostream &operator<<(std::ostream &sout, const Foo &obj) {
  return sout << "(" << obj.fizz << ", " << obj.buzz << ")";
}

template <typename Container>
void print(const Container &container, std::ostream &sout = std::cout) {
  std::copy(container.begin(), container.end(),
            std::ostream_iterator<typename Container::value_type>(sout, " "));
  sout << '\n';
}

int main() {
  std::mt19937 prng(std::random_device{}());
  std::uniform_int_distribution<int> dist(0, 9);
  std::uniform_int_distribution<int> large(500, 999);

  std::vector<Foo> devices;
  for (int i = 0; i < 100; ++i) {
    devices.push_back(Foo{.fizz = dist(prng), .buzz = large(prng)});
  }

  print(devices);
  std::cout << '\n';

  /*** Starting here ***/
  // Counting occurrences
  std::vector<std::pair<int, int>> counts;
  int index = 0;
  for (int i = 0; i < 10; ++index, ++i) {
    counts.emplace_back(index, 0);
  }
  for (auto i : devices) {
    ++counts[i.fizz].second;
  }

  // Descending sort using counts of occurrence of a fizz value
  std::stable_sort(counts.begin(), counts.end(),
                   [](auto a, auto b) { return a.second > b.second; });

  std::vector<int> sort_by(10);
  for (int i = 0; i < 10; ++i) {
    sort_by[i] = counts[i].first;
  }
  print(sort_by);  // Prints fizz values in order of descending count
  std::cout << '\n';

  // Sort devices according to highest quantity of fizz
  auto walker = devices.begin();
  for (auto i : sort_by) {
    for (auto it = devices.begin(); it != devices.end(); ++it) {
      if (it->fizz == i) {
        std::iter_swap(walker, it);
        ++walker;
      }
    }
  }
  /*** Ending here ***/

  print(devices);
}

int index = 0;
  for (int i = 0; i < 10; ++index, ++i) {
    counts.emplace_back(index, 0);
  }

Not sure why you use index here which is same as i ?

[](auto a, auto b) { return a.second > b.second; }
for (auto i : sort_by) {

a and b, i can be marked const.

 std::vector<int> sort_by(10);
  for (int i = 0; i < 10; ++i) {
    sort_by[i] = counts[i].first;
  }

Not sure that there's a big improvement, but here's a change anyway.
  std::vector<int> sort_by;
  sort_by.reserve(10);
  std::for_each(counts.begin(), counts.end(), [&sort_by](const auto &i) {
    sort_by.push_back(i.first);
  });

for (auto it = devices.begin(); it != devices.end(); ++it) {
  if (it->fizz == i) {
    std::iter_swap(walker, it);
    ++walker;
  }
}

Can be made a bit simpler by using range based for loop & address instead of iterator. Note the reference in &it.
for (auto &it : devices) {
  if (it.fizz == i) {
    std::iter_swap(walker, &it);
    ++walker;
  }
}


Serializable game loop thread I have been working on a small game lately and I was tired of looking up and copy-pasting game loops so I tried to make one as you can see. It works perfectly so if you want to feel free to use it, but for those who are good at making these I would like to ask if I can improve it, make it more efficient and overall is it a good solution to run two of these one to render, the other to tick.

package engine;

public abstract class RapidExecutor extends Thread {

    Application app;

    /*
    the field app is here because the abstract tick method can
    only acces the stuff needed to render or to tick
    objects so if you want to use it for anything else
    you can just delete it
    */

    private final int TPS;

    private boolean running;

    public RapidExecutor(Application app, int TPS) {

        this.app = app;

        this.TPS = TPS;

        if (TPS > 1000 || TPS < 1)
            try {
                throw new Exception("Invalid TPS count");
            } catch (Exception e) {
                e.printStackTrace();
            }

    }

    public abstract void tick();

    public void run() {

        try {

            this.running = true;

            long now, startTime;

            int delta, TPSCount = 1000 / this.TPS;

            while (this.running) {

                startTime = System.currentTimeMillis();

                tick();

                now = System.currentTimeMillis();

                delta = (int) (TPSCount - (now - startTime));

                if (delta < 0) this.stopThread();

                sleep(delta);

            }

        } catch (Exception e) {
            e.printStackTrace();
        }

    }

    public synchronized void stopThread() {

        try {

            this.running = false;
            this.join();

        } catch (Exception e) {
            e.printStackTrace();
        }

    }

}

Feel free to use the code and if you have any thoughts I would be very glad to hear them.

Before reviewing the code, I want to point out that this kind of functionality already exists in the core Java libraries. A ScheduledExecutorService will run an arbitrary Runnable on a fixed time schedule. Instead of extending RapidExecutor, you just implement the interface and pass it into the service.

Some thoughts on your code:

In my opinion, your code is violating the Single Responsibility Principle. It’s doing three things: defining the interface for something that executes every tick, defining a new type of Thread, and controlling the frequency that this new type of thread executes code. As mentioned above, I’d strongly suggest using ScheduledExecutorService, Thread, and implement Runnable for the code that is currently in subclasses of RapidExecutor.

Your Application app should be private, tick() should be tick(Application app), and run() should call tick(this.app). Keep variables contained in scope.

Don’t use UPPERCASE for abbreviations. It’s considered better to use camelCase (tps or tpsCount). And don’t abbreviate when you don’t need to. It took me a bit to figure out that TPS was ticksPerSecond.

Please don’t throw an exception just to catch it to get the stack trace to print it out. Let the caller figure out what to do when you tell them they gave you bad input. They might not care about the error message being written to System.err, or they might actively not want that to happen. And don’t use a generic Exception. IllegalArgumentException, thrown from the constructor, would be better.

tick() should be protected, not public. You only want it called from inside RapidExecutor.

run() must be final. If it isn’t, anybody who extends RapidExecutor can change what it means for run() to be executed.

It’s considered better form to declare variables on separate lines.

Variables should be declared in the tightest scope possible to reduce cognitive load on readers of your code. startTime, now, and delta should all be declared inside your while loop.

There’s no need to cast delta back to an int. And perhaps sleepTime would be a better variable name than delta? It’s good for variables to clearly express what the represent.

Perhaps storing final long tickDuration = System.currentTimeMillis() - startTime would be better than now.

Perhaps renaming TPSCount to millisecondsPerTick would make it more clear what was being stored.

Again, don’t randomly catch Exception. And if you do handle an exception, reset the state of RapidExecutor to something appropriate. Right now execution stops but this.running is still true. Are you trying to handle the InterruptedException? If so, declare that try-catch block immediately around sleep(). There are well-understood ways to correctly handle InterruptedException, and you should look into them. Are you trying to handle exceptions coming from tick()? If so, put a tight block around this.tick(). Maybe even declaring your own checked exception in this case would be a good idea.

It’s unclear why you stop execution when it takes longer than TPSCount to run a tick(). I’m not convinced that’s a good design. Would it be better to skip a tick and get a choppy frame rate rather than just failing silently? Make sure that this is clearly documented.

Don’t ever synchronize a method using the synchronized keyword. Locking on yourself is never a good idea because somebody else could take out a lock on you, and then stopThread() will never be able to execute. Use a private Object lock along with synchronized(this.lock), or a more complex approach when warranted.

I also don’t see why you need to synchronize either of the method calls inside stopThread(). Are you sure that’s necessary?

You’ll need to make stopThread() final to prevent subclasses from changing what it means to stop a thread.

Again, make the exception block in stopThread() as tight as possible and handle the most specific exception possible.

If you applied all my suggestions to your code, it might look something like:

public abstract class RapidExecutor extends Thread {

    private final Object lock = new Object();
    private final Application application;
    private final int ticksPerSecond;

    private boolean running;

    public RapidExecutor(final Application application, final int ticksPerSecond) {

        this.application = application;
        this.ticksPerSecond = ticksPerSecond;

        if (ticksPerSecond > 1000 || ticksPerSecond < 1) {
            throw new IllegalArgumentException("ticksPerSecond must be between 1 and 1000, was " + ticksPerSecond);
        }

    }

    protected abstract void tick(final Application application) throws TickFailedException;

    @Override
    public void run() {

        this.running = true;
        final int millisecondsPerTick = 1000 / this.ticksPerSecond;

        while (this.running) {

            final long startTime = System.currentTimeMillis();

            try {
                this.tick(this.application);
            } catch (final TickFailedException | RuntimeException e) {
                this.running = false;
                // log the exception ?
            }

            final long tickDuration = System.currentTimeMillis() - startTime;
            final long sleepTime = millisecondsPerTick - tickDuration;
            if (sleepTime < 0) {
                this.stopThread();
            }

            try {
                sleep(sleepTime);
            } catch (final InterruptedException e) {
                this.running = false;
                this.interrupt();
                // log the exception?
            }
        }
    }

    public final void stopThread() {

        synchronized (this.lock) {
            this.running = false;
            try {
                this.join();
            } catch (final InterruptedException e) {
                this.interrupt();
                e.printStackTrace();
            }
        }

    }

}


Infinite prime generator This code acts as an infinite generator of prime numbers.

As new prime numbers are found, they are added to a set. Then, a number x is found to be prime if none of the numbers in the prime set are factors of x. Only the primes less than or equal to the square root of x need to be checked.

import itertools
from math import sqrt

class stream:
    """ Class of infinite streams. """

    def prime():
        """ Stream of prime numbers. """
        prime_set = {2} # Set of prime numbers that have been found
        yield 2 # First prime
        for x in itertools.count(3, 2): # Check odd numbers, starting with 3
            primes_below_sqrt = {i for i in prime_set if i <= sqrt(x)}
            for prime in primes_below_sqrt:
                if x % prime == 0:
                    break # x is divisible by a prime factor, so it is not prime
            else:
                prime_set.add(x) # x has been shown to be prime
                yield x

Using the itertools recipe:

def take(iterable, n):
    """ Returns first n items of the iterable as a list. """
    return list(itertools.islice(iterable, n))

Output:

>>> take(stream.prime(), 10)
[2, 3, 5, 7, 11, 13, 17, 19, 23, 29]

This is a performance issue:

primes_below_sqrt = {i for i in prime_set if i <= sqrt(x)}

By using a set comprehension instead of a generator expression, you are actually making a copy of the subset.


Log the diff of a Apache httpd config file and reload the service with Python and systemd Using systemd's .path unit to run a Python script whenever a httpd config file is changed. The Python script creates a backup of the config and logs the differences of the newly modified and backup file. After that it checks the syntax of the httpd config and gracefully reloads the web server service.

The Python script:

#!/path/to/python

import difflib
import logging
import shutil
import subprocess
import sys
from pathlib import Path
import yaml

logging.basicConfig(format='%(levelname)s - %(message)s', level=logging.INFO)

class HttpdMonitor:

    def __init__(self, config_file):
        self.config_file = config_file
        self.config = self.load_config()
        self.httpd_conf = self.config['httpd-conf']
        self.backup_file = self.config['httpd-conf'] + '.bak'
        self.syntax_check_cmd = self.config['syntax-check-cmd'].split()
        self.reload_cmd = self.config['reload-cmd'].split()

    def load_config(self):
        with open(self.config_file, 'r') as stream:
            return yaml.safe_load(stream)

    def make_backup(self):
        try:
            shutil.copy2(self.httpd_conf, self.backup_file)
        except IOError as e:
            logging.exception("Making backup failed: %s", e)

    def find_diff(self):
        with open(self.httpd_conf, 'r') as f, open(self.backup_file, 'r') as bak:
            diffs = list(
                difflib.unified_diff(f.readlines(), bak.readlines(), fromfile=self.httpd_conf, tofile=self.backup_file))
            return diffs

    def log_diff(self):
        try:
            diffs = self.find_diff()
            logging.info('Differences found: \n%s', ''.join(diffs))
        except IOError as e:
            logging.exception('Finding diff failed: %s', e)

    def call_httpd_commands(self):
        subprocess.run(self.syntax_check_cmd, check=True)
        subprocess.run(self.reload_cmd, check=True)

    def reload_httpd(self):
        try:
            self.call_httpd_commands()
        except subprocess.CalledProcessError as e:
            logging.exception("Reloading failed: %s", e)

    def run(self):
        if not Path(self.backup_file).is_file():
            self.make_backup()
            return
        self.log_diff()
        self.make_backup()
        self.reload_httpd()

if __name__ == '__main__':
    httpd_monitor = HttpdMonitor(sys.argv[1])
    httpd_monitor.run()

This uses a YAML file to specify the httpd config file and also the commands to run, sort of like this:

---
httpd-conf: /path/to/file
syntax-check-cmd: httpd -t
reload-cmd: httpd -k graceful

Using systemd's .path unit the script get fired whenever there's something changed in the httpd config file. For example httpd-monitor.path:

[Unit]
Description=Monitor the httpd conf for changes

[Path]
PathChanged=/path/to/file
Unit=httpd-monitor.service

[Install]
WantedBy=multi-user.target

And httpd-monitor.service:

[Unit]
Description=Do backup, diff changes and reload httpd

[Service]
Restart=no
Type=simple
ExecStart=/path/to/httpd_monitor.py /path/to/httpd-monitor.yaml

[Install]
WantedBy=multi-user.target

So I'd first run systemd start httpd-monitor.service just to create the initial backup (the if condition within the run() method should take care of it) and then just systemd enable --now httpd-monitor.path to enable the .path unit.
Probably not the simplest of solutions so I'm very open to any feedback!

As a whole your approach looks good, except for a couple of issues I'd suggest to fix:

__init__ method
Avoid repetitive indexing of nested structures like self.config['httpd-conf'].Instead use assigned instance variable with f-string formatting:

def __init__(self, config_file):
    self.config_file = config_file
    self.config = self.load_config()
    self.httpd_conf = self.config['httpd-conf']
    self.backup_file = f'{self.httpd_conf}.bak'
    self.syntax_check_cmd = self.config['syntax-check-cmd'].split()
    self.reload_cmd = self.config['reload-cmd'].split()

find_diff method
No need to convert a generator returned by difflib.unified_diff function into list with diffs = list(...) - the subsequent ''.join(diffs) itself will take care of consuming generator.
call_httpd_commands method
This method is redundant as its main effect is reloading httpd server, but you already have the appropriate method for that purpose - reload_httpd. Thus call_httpd_commands method's statements are just moved there and the old method is eliminated:

def reload_httpd(self):
    try:
        subprocess.run(self.syntax_check_cmd, check=True)
        subprocess.run(self.reload_cmd, check=True)
    except subprocess.CalledProcessError as e:
        logging.exception("Reloading failed: %s", e)


Using Java 8 parallel streams I'm trying to get more familiar with the new Java 8 features, so I am rewriting one of my earlier projects. It includes a class that keeps track of the best entry seen so far:

import java.util.List;

public class Maxinator<T> {

    private final QualityFunction<T> qualityFunction;
    private T best;
    private double bestQuality;

    public Maxinator(QualityFunction<T> qualityFunction) {
        this.qualityFunction = qualityFunction;
        reset();
    }

    public void reset() {
        best = null;
        bestQuality = Double.NEGATIVE_INFINITY;
    }

    public T getBest() {
        return best;
    }

    public void updateBest(List<T> population) {
        population.parallelStream()
                .forEach(i -> {
                    double quality = qualityFunction.computeQuality(i);
                    if (quality > bestQuality) {
                        best = i;
                        bestQuality = quality;
                    }
                });
    }
}

Where QualityFunction is simply the following interface:

public interface QualityFunction<T> {
    public abstract double computeQuality(T individual);
}

My questions:

Are there any concurrency issues with the code in the forEach modifying best and bestQuality? Or are these automatically taken care of by the parallel stream processing?
Is there a more idiomatic way to write this, using the new APIs (collect, reduce, etc.)?

Note: the quality function could be very expensive, so I want to make sure that it is only called once per element.

Your code is not thread-safe. Each of the threads will, in parallel, be accessing both the best, and the bestQuality variables.

Your Lambda is, in essence, modifying external data from the stream, and this is an anti-pattern for streams. It has side-effects.

You should change your code to use the collect mechanism. There are a few ways to do it, but, you should look at this example for guidance: Reduction

Some Notes about my following suggestion:

I converted the method to a static method, and created an inner accumulator class.
you should possibly refine this answer to suit your needs more in the class structure.

Putting these observations together, I would suggest something like:

public class Maxinator {

    @FunctionalInterface
    public interface QualityFunction<V> {
        double computeQuality(V value);
    }

    private Maxinator() {
//        no public instances
    }

    private static final class AccumulateResult<T> {
        T bestItem = null;
        double bestScore = Double.MIN_VALUE;

        public T getBestItem() {
            return bestItem;
        }

        public void accept(QualityFunction<T> function, T item) {
            double score = function.computeQuality(item);
            if (score > bestScore) {
                bestScore = score;
                bestItem = item;
            }
        }

        public AccumulateResult<T> combine(AccumulateResult<T> r) {
            if (r.bestScore > bestScore) {
                bestScore = r.bestScore;
                bestItem = r.bestItem;
            }
            return this;
        }

    }

    public static <T> T getBest(final QualityFunction<T> qualityFunction, final List<T> population) {
        return population.parallelStream().collect(Collector.of(
                    AccumulateResult<T>::new,
                    (a,t) -> a.accept(qualityFunction, t),
                    (a, b) -> a.combine(b))
               ).getBestItem();
    }
}


Iterate over a list of list names as file names Interrogating a web API using query-url's, for each query I can get either zero hits, one hit, or multiple hits. Each of those categories needs to go into a separate CSV file for manual review or processing later. (More on this project here and here).

The input data (from a 14K line csv, one line per artist) is full of holes. Only a name is always given, but may be misspelled or in a form that the API does not recognise. Birth dates, death dates may or may not be known, with a precision like for example 'not before may 1533, not after january 1534'. It may also have exact dates in ISO format.

Using those three different output csv's, the user may go back to their source, try to refine their data, and run the script again to get a better match. Exactly one hit is what we go for: a persistent identifier for this specific artist.

In the code below, df is a Pandas dataframe that has all the information in a form that is easiest to interrogate the API with.

First, I try to get an exact match best_q (exact match of name string + any of the available input fields elsewhere in the record), if that yields zero, I try a slightly more loose match bracket_q (any of the words in the literal name string + any of the available input fields elsewhere in the record).

I output the dataframe as a separate csv, and each list of zero hits, single hits, or multiple hits also in a separate csv.

I'm seeking advice on two specific things.

Is there a more Pythonic way of handling the lists? Right now, I think the code is readable enough, but I have one line to generate the lists, another to put them in a list of lists, and another to put them in a list of listnames.
The second thing is the nested if..elif on zero hits for the first query. I know it ain't pretty, but it's still quite readable (to me), and I don't see how I could do that any other way. That is: I have to try best q first, and only if it yields zero, try again with bracket_q.

I have omitted what goes before. It works, it's been reviewed, I'm happy with it.

A final note: I'm not very concerned about performance, because the API is the bottleneck. I am concerned about readability. Users may want to tweak the script, somewhere down the line.

singles, multiples, zeroes = ([] for i in range(3))

for row in df.itertuples():
    query = best_q(row)
    hits, uri = ask_rkd(query)
    if hits == 1:
        singles.append([row.priref, row.name, hits, uri])
    elif hits > 1:
        multiples.append([row.priref, row.name, hits])
    elif hits == 0:
        query = bracket_q(row)
        hits, uri = ask_rkd(query)
        if hits == 1:
            singles.append([row.priref, row.name, hits, uri])
        elif hits > 1:
            multiples.append([row.priref, row.name, hits])
        elif hits == 0:
            zeroes.append([row.priref, str(row.name)])  # PM: str!!

lists = singles, multiples, zeroes
listnames = ['singles','multiples','zeroes']

for s, l in zip(listnames, lists):
    listfile = '{}_{}.csv'.format(input_fname, s)
    writelist(list=l, fname=listfile)

outfile = fname + '_out' + ext
df.to_csv(outfile, sep='|', encoding='utf-8-sig')

As you already noticed you have repeated code. you have variable names for your lists and also string definitions for list names which most probably should match. while this is no big problem for just 3 lists it could get cumbersome on adding another list. A simple way to avoid such name-to-string matching edits is to hold such variables in a dict() and have the string definition only.

The second problem is to have different iterables which must match in length and order to be zipped lateron. Avoid this by holding tuples (or other containers) in a single iterable from the beginning. key-value pairs in a dict() also provide this binding.

I your case I'd recommend to use the strings as keys

#avoid named variables
lists = {name:[] for name in ('singles', 'multiples' , 'zeros')}

#access lists via name
lists['singles'].append(0)

#access via temporary
l = lists['singles']
l.append[0]

#iterate for saving
for s, l in lists.items():
    writelist(list=l, fname=s + '.csv')

EDIT:

Above answer applies to the first version of code where all that list init was skipped. While all still valid this can now be applied to the real code. concise and following the KISS principle. Names could be improved but are left here for outlining changes only.

lists = {name:[] for name in ('singles', 'multiples' , 'zeros')}

for row in df.itertuples():
    query = best_q(row)
    hits, uri = ask_rkd(query)
    if hits == 0:
        query = bracket_q(row)
        hits, uri = ask_rkd(query)

    if hits == 1:
        lists['singles'].append([row.priref, row.name, hits, uri])
    elif hits > 1:
        lists['multiples'].append([row.priref, row.name, hits])
    elif hits == 0:
        lists['zeroes'].append([row.priref, str(row.name)])  # PM: str!!

for s, l in lists.items():
    listfile = '{}_{}.csv'.format(input_fname, s)
    writelist(list=l, fname=listfile)

outfile = fname + '_out' + ext
df.to_csv(outfile, sep='|', encoding='utf-8-sig')


Reducing memory usage for FizzBuzz in R I've been trying all night to get my fizzbuzz to use below 20 MB of RAM but I can't seem to get it much smaller than this.

# Sample code to read test cases
args <- commandArgs(trailingOnly=TRUE)
test.cases <- strsplit(readLines(args[[1]], warn=FALSE), '\n')
args <- NULL
for (test in test.cases) {
    if (length(test) > 0) {
        # ignore test if it is an empty line
        # 'test' represents the test case, do something with it
        test <- as.numeric(unlist(strsplit(test, " ")))
        x <- 0
        y <- 0
        for(i in 1: test[3]) {
            if(i != 1) cat(" ")
            flag <- TRUE
            out <- i
            x <- x + 1
            y <- y + 1
            if( x == test[1] ) {
                cat("F")
                x <- 0
                flag <- FALSE
            }
            if( y == test[2] ) {
                cat("B")
                y <- 0
                flag <- FALSE
            }
            if(flag) cat(out)
        }

        cat("\n")
    }
}

Thanks for providing the link, it helped a lot in understanding what you were after. I will provide two versions for you to consider. The first one is algorithmically similar to yours, except written a little more in the "R style". The second one will make a more efficient use of memory since that seems to be important too.

I will also assume that the input file in args[[1]] is like the one in the link, i.e. contains lines like the two below:

3 5 10
2 7 15

Version 1:

args <- commandArgs(trailingOnly = TRUE)
input.file <- args[[1]]
test.cases <- strsplit(readLines(input.file, warn=FALSE), ' ')
for (test in test.cases) {
   stopifnot(length(test) == 3L)
   XYN <- as.integer(test)
   X <- XYN[1]
   Y <- XYN[2]
   N <- XYN[3]
   i <- seq_len(N)
   is.divisible.by.X <- (i %% X) == 0L
   is.divisible.by.Y <- (i %% Y) == 0L
   out <- as.character(i)
   out[is.divisible.by.X & is.divisible.by.Y] <- "FB"
   out[is.divisible.by.X] <- "F"
   out[is.divisible.by.Y] <- "B"
   cat(out, sep = " ")
   cat("\n")
}

Some noticeable improvements are:

some assumption checking with stopifnot
the use of variable names that are closer to the problem write-up (X, Y, N) or more descriptive, e.g., is.divisible.by.X
the use of %% for finding if a number is divisible by X or Y
vectorization (notice how one of your for loops is gone), which should make your code faster
The use of seq_len(N) instead of 1:N is more robust in the case when N is zero.

Version 2:

args <- commandArgs(trailingOnly = TRUE)
input.file <- args[[1]]
file.handle <- file(input.file, open = "r")
repeat {
   line <- readLines(file.handle, n = 1)
   if (length(line) == 0L) break  # end of file
   test <- strsplit(line, " ")[[1]]
   XYN <- as.integer(test)
   X <- XYN[1]
   Y <- XYN[2]
   N <- XYN[3]
   i <- 1L
   while (i <= N) {
      is.divisible.by.X <- (i %% X) == 0L
      is.divisible.by.Y <- (i %% Y) == 0L
      out <- if (is.divisible.by.X & is.divisible.by.Y) "FB" else
             if (is.divisible.by.X) cat("F") else
             if (is.divisible.by.Y) cat("B") else as.character(i)
      cat(out, "")
      i <- i + 1L
   }
   cat("\n")
}
close(file.handle)

How is that different from version 1? Here we are careful to read the file and process it line-by-line instead of reading the whole file in memory. Also, when processing each line, we are careful not to generate the seq_len(N) vector in memory and pre-compute the output for all the values. Instead, we are counting from 1 to N and processing each value one at a time.

Because version 2 uses a while loop in place of the vectorization happening in version 1, it will be slower. But it will use a lot less memory, especially if the file contains a large number of rows or large values for N, both things could make version 1 fail.

I hope this helps. Let me know if you have questions.


Printing multiple different shapes on the same line Whilst scanning my C programming textbook, I saw this practice problem from a very early chapter:

I got to thinking that the problem probably wants you to draw each shape separately, which would be very easy.

But to do literally what the question asks (print them on the same line), I couldn't see any easy way! The rectangle is 9x9, but they seem 5 spaces apart, the oval is 7 wide, the arrow tip is 5x3, etc. Maybe if the arrow had tips on both ends, I could dissect the whole picture horizontally.

After dabbling with ways to produce the rectangle, oval, arrow, and diamond by themselves, and then to somehow print each row correctly with loops and ifs, I realized maybe I should just draw it literally.

#include <stdio.h>

int main() {

    puts("*********       ***         *           *");
    puts("*       *      *   *       ***         * *");
    puts("*       *     *     *     *****       *   *");
    puts("*       *     *     *       *        *     *");
    puts("*       *     *     *       *       *       *");
    puts("*       *     *     *       *        *     *");
    puts("*       *     *     *       *         *   *");
    puts("*       *      *   *        *          * *");
    puts("*********       ***         *           *");

    return 0;

}

I could have put it all in one puts with newlines but I found that was pretty unreadable.

Is there any better way to do this? Am I completely missing something / am I doing it in the most efficient way?

It really is that simple.  Your idea of putting everything into one puts() call would be an improvement, because I/O calls are relatively expensive.  Not that it really matters in this exercise.  It's just a good habit.

The second shape should be a little bit wider. =)

Note that a return 0; at the end of main() is automatically implied, as a special case.

#include <stdio.h>

int main() {

    puts("*********        ***          *           *\n"
         "*       *      *     *       ***         * *\n"
         "*       *     *       *     *****       *   *\n"
         "*       *     *       *       *        *     *\n"
         "*       *     *       *       *       *       *\n"
         "*       *     *       *       *        *     *\n"
         "*       *     *       *       *         *   *\n"
         "*       *      *     *        *          * *\n"
         "*********        ***          *           *");

    return 0;

}


Pretty print a tree from minimal data input I'm currently working on an uncrafting tool for minecraft and got a neat output working that shows all the materials needed in a tree. I wonder if the code used to print that tree could be improved somehow. I only have the recursion depth as a measurement and the ArrayList gets filled in the same order as seen in the input example below. The output format should also stay the same, I'm quite happy with that. The input format is a bit sketchy though, any ideas on that are appreciated as well.
The Algorithm
    private static void treeprint(ArrayList<Object[]> tree) {

        ArrayList<String> stem = new ArrayList<>();

        for (int i = 0; i < tree.size() - 1; i++) {
            System.out.print(String.join("", stem));
            Object[] o = tree.get(i);
            int cdepth = (int) o[0];
            String cname = (String) o[1];
            int camt = (int) o[2];

            int ndepth = (int) tree.get(i + 1)[0];
            if (ndepth > cdepth) {
                if (isLastOfDepth(tree, cdepth, i + 1)) {
                    System.out.print("\\---");
                    stem.add("    ");
                } else {
                    System.out.print("+---");
                    stem.add("|   ");
                }

            }
            if (ndepth == cdepth) {
                if (isLastOfDepth(tree, cdepth, i + 1)) {
                    System.out.print("\\---");
                } else {
                    System.out.print("+---");
                }

            }
            if (ndepth < cdepth) {
                System.out.print("\\---");
                for (int j = 0; j < cdepth - ndepth; j++) {
                    stem.remove(stem.size() - 1);
                }
            }
            System.out.println(cname + " x " + camt);
        }

        System.out.println(String.join("", stem) + "\\---" + (String) tree.get(tree.size() - 1)[1] + " x "
                + (int) tree.get(tree.size() - 1)[2]);
    }

    private static boolean isLastOfDepth(ArrayList<Object[]> tree, int depth, int start) {

        for (int i = start; i < tree.size(); i++) {
            int cdepth = (int) tree.get(i)[0];
            if (cdepth < depth) {
                return true;
            }
            if (cdepth == depth) {
                return false;
            }
        }
        return true;
    }

Test Input
Format is recuresion depth / item name / item amount
1 component heat vent 1
2 iron bars 4
3 iron 6
2 heat vent 1
3 electric motor 1
4 coil 2
5 copper cable 8
5 iron 1
4 iron 1
4 tin item casing 2
3 iron bars 4
4 iron 6
3 iron plate 4
2 tin plate 4

Nice solution, my suggestions:

Short variable names: variables like camt and cname can be extended to currentAmount and currentName. It's better to have longer names to improve readability. For example, I am not sure what stem stands for.

Types and input validation: there are a lot of casting in the methods which are not validated. Accepting an array of Object without any validation is a recipe for troubles. Using a class you can solve both problems, something like this:
class Node{
    private int depth;
    private String itemName;
    private int amount;
    public Node(int depth, String itemName, int amount) {
        // Input validation here
        this.depth = depth;
        this.itemName = itemName;
        this.amount = amount;
    }
    // Getters, etc.
}

So this code:
Object[] o = tree.get(i);
int cdepth = (int) o[0];
String cname = (String) o[1];
int camt = (int) o[2];

Can become:
Node node = tree.get(i);
int currentDepth = node.getDepth();
String currentItemName = node.getItemName();
int currentAmount = node.getAmount();

You can also make the properties of the class final to have an immutable node.

Constants: the strings \--- and +--- are repeated multiple times. Consider creating constants for such special strings so that they will be easier to change in the future.

Functionality: If the list is not properly built, the tree will not be printed correctly. For example:
The input:
1 a 1
2 b 1
3 c 1
4 d 1

Produces the correct output:
\---a x 1
   \---b x 1
      \---c x 1
         \---d x 1

But if we add one more node in the middle:
1 a 1
2 b 1
3 c 1
2 e 1
4 d 1

The output becomes:
\---a x 1
  +---b x 1
  |   \---c x 1
  \---e x 1
      \---d x 1

Now d changed its depth from 4 to 3 and e became its father.
Providing a well-formed list of nodes with corresponding depth is not a simple task if the tree grows in size. A solution might be to create a Tree class that knows how to add and remove nodes. Then the treePrint method will do a Pre-order scan of the tree and print the nodes on the way.


Project Euler 6: Sum square difference in Functional Programming (FP) I wanted to practice functional programming (fp) without using any library but using vanilla JS only. So I took a problem from Project Euler:

  The sum of the squares of the first ten natural numbers is,

  12 + 22 + ... + 102 = 385

  The square of the sum of the first ten natural numbers is,

  (1 + 2 + ... + 10)2 = 552 = 3025

  Hence the difference between the sum of the squares of the first ten natural numbers and the square of the sum is 3025 − 385 = 2640.

  Find the difference between the sum of the squares of the first one hundred natural numbers and the square of the sum.

My solution in FP:

/*jshint esversion: 6*/
(function () {
  'use strict';

  const range = f => num => Array.from(new Array(num), f);
  const quadRange = range((_, i) => (i + 1) * (i + 1));
  const simpRange = range((_, i) => i + 1);
  const sum = (acc, val) => acc + val;
  const sumOfQuads = quadRange(100)
    .reduce(sum);
  const quadsOfSum = simpRange(100)
    .reduce(sum) * simpRange(100)
    .reduce(sum);
  console.log("solution ", quadsOfSum - sumOfQuads);
})();

Is there a better way to write it in FP (without any libraries and with vanilla JS only)?

DISCLAIMER: I don't think this is actually an "answer" answer. Please consider it as a side note, because your code generally looks good to me. I also admit, this answer is highly subjective compared to other my answers.. My purpose here is to provide a point of view rather than persuade that something needs to be fixed in the original code.

[Non-functional] alternative

In a real-life scenario, if num input was huge, you might not be able to create a range array via new Array(num).

Number.MAX_VALUE
// ---> 1.7976931348623157e+308

new Array(Number.MAX_VALUE)
// VM234:1 Uncaught RangeError: Invalid array length
//    at <anonymous>:1:1

Chrome:

Instead of that you could rewrite your range function as a generator function (at a "cost" of it not being an arrow function anymore):

function* range(num) { for (let counter = 1; counter <= num; counter++) yield counter; }

Unfortunately, this is also less functional on its own (notice that the code uses a let variable...). Worse than that, you can only

either manually iterate over it via IterableIterator<> interface (requires a variable);
or use for (let _ of range(Integer.MAX_VALUE) { ... } which isn't looking functional;
or try materialize it into an array via spread operator [...range(Integer.MAX_VALUE)] (which will not work, for the same exact reason I'm mentioning the generator function).

I wish there was a commonly known first-class concept of a Range in JavaScript...

Naming Things

The only thing that pokes my eyes is variable naming. IMHO, you can do better than acc, val, or i. For example, the following is more readable, since real words are being used and the function is more explanatory.

const range = mapFunction =>
  rangeLength =>
    Array.from(new Array(rangeLength), mapFunction);

and

const summer = (subTotal, currentNumber) => subTotal + currentNumber;

summer is better than sum because a) sum as a word denotes the result of summation rather than something that encapsulates the summation logic; b) reads better in the usage. In fact, you can even write const sum = someArray.reduce(summer), or if you prefer better consumer readability over function name -- const sum = someArray.reduce(withSummer).

I know, these are tiny details, but for the reader they make a difference. Once mentioned it in another answer already -- I've witnessed a situation where in an inline function a developer interpreted acc as account. Silly? Yeah, but who cares when one needs to work on Friday evening to fix a bug?


Reading and Storing Integer from Serial Monitor Description

I wanted to understand how integers can be written and stored inside variables without the use of Serial.parseInt(). I couldn't find any code examples about this. Perhaps this is the right way to do it, although it doesn't detect and block letters and special characters. Please leave some feedback on ways to improve it. Thanks in advance! :)

void setup()
{
  Serial.begin(9600); // Opens serial port, sets data rate to 9600 bps
  Serial.println(F("Serial port opened!"));
}

void loop()
{
  byte received;
  static byte counter = 0;
  static uint32_t number = 0; // Store value from 0 to 4294967295

  while (Serial.available()) // Read data in serial receive buffer
  {
    received = Serial.read(); // Store received byte
    if (received != 10) // Terminate if newline char detected
    {
      if (counter != 0) // Put number in its correct position (base-10 system)
      {
        number *= 10;
        number += (received - '0');
      }
      else
      {
        number = (received - '0');
      }
      counter++;
    }
    else
    {
      Serial.println(number); // Print value inside number and reset
      counter = 0;
    }
  }
}

Character literals

While this is technically correct:

if (received != 10) // Terminate if newline char detected

you're better off writing

if (received != '\n')

Much of embedded electronics assumes ASCII encoding, and this is no exception, but you're still better off using the symbol instead of the code.

IO management

You loop while serial I/O is available:

while (Serial.available())

But what if there is a pause in the availability of bytes in the middle of your integer? Your code will not do the right thing. Instead:

uint32_t number = 0;
while (true) {
  int received = Serial.read();
  if (received == -1) continue;
  if (received == '\n') break;
  // ...
}
Serial.println(number);

Do not use static for number
Don't need a counter
Keep looping if no data are available
Ensure that one execution of loop maps to one full output integer

Condition in loop

If you are using the ATmega328P, it has a dedicated MUL instruction that only takes two cycles. It's more complex and expensive to have your if (counter != 0) than it is to simply unconditionally multiply-and-add.


Parsing dates from a webpage I have two inputs (TextBox) on my front end.

The following code parses them from their string representation to aDateTime object.  If it fails, it then sets default values.

DateTime dateFrom;
DateTime dateTo;
if (!DateTime.TryParse(txtDateFrom.Text, out dateFrom)) { dateFrom = DateTime.Today.AddDays(-7); }
if (!DateTime.TryParse(txtDateTo.Text, out dateTo)) { dateTo = DateTime.Now; }

However, the code looks bulky. Is there a better way to write it?

You won't be able to change the bulkiness much but there are a few things you can do:

indent your code. It will take more space but it will also be more readable:

DateTime dateFrom;
DateTime dateTo;

if (!DateTime.TryParse(txtDateFrom.Text, out dateFrom))
{
    dateFrom = DateTime.Today.AddDays(-7);
}
if (!DateTime.TryParse(txtDateTo.Text, out dateTo))
{
    dateTo = DateTime.Now;
}

Wait for C# 6 which has inline-out-variables. This will make it possible to write the TryParse like this:

if (!DateTime.TryParse(txtDateFrom.Text, out DateTime dateFrom))

or even

if (!DateTime.TryParse(txtDateFrom.Text, out var dateFrom))

Use DateTime.UtcNow. By using DateTime.Now you're dependent on the location of your server which might be a problem some time in the future when you host it elsewhere.
Consider making a datetime picker available in the frontend instead of a textbox. It's a lot more user-friendly and it will avoid invalid dates.


Securing PHP using a gateway that consults a whitelist My site was recently infected and running malicious scripts. Once I discovered this, I cleared everything, the hosting company re-initialized the VPS and I setup my site anew. This is how I run my PHP scripts currently. I have two objectives:

To keep my scripts from being directly accessible from the web
To never run a script that I do not approve of

First I moved the PHP files out of the public folder except for one gateway file

scripts/
   prepend.php
   a.php
   b.php
   ...
public_html/
   gateway.php
   html, css, js, image files

I use .htaccess to redirect all requests for my PHP scripts to gateway.php.  This is the file that actually executes the PHP script requested.  To reach a.php, the browser requests /php/a...

RewriteEngine On
RewriteBase /
RewriteRule ^php/(.*)   gateway.php?file=$1.php   [END,QSA,NC]

In php.ini, I set auto_prepend_file to point to scripts/prepend.php. This file is run before all PHP scripts, so I can use it to do some configuration. In part, it has a list of allowed PHP scripts.

prepend.php

//the only scripts that will be allowed to run
Const SAFE_SCRIPTS = '"path/to/gateway.php","/path/to/a.php","/path/to/b.php"';
Const SCRIPTS_PATH = __DIR__;

//ensure that the requested script is allowed. The check here is to prevent
//a maliciously uploaded PHP file from executing.
$file = $_SERVER['SCRIPT_FILENAME'];//typically /path/to/gateway.php
if(strpos(SAFE_SCRIPTS,"\"$file\"")===false) die("$file is not allowed");

After prepend.php, gateway.php runs; it's in charge of making sure the requested PHP file is on the allowed list and for forwarding the request to the right file

//get the file requested. Comes from the htaccess rule
$file = SCRIPTS_PATH.'/'.$_GET['file'];

//if file is not in the allowed list, abort. The check here is to prevent
//gateway from loading any file it wasn't designed to load
if(strpos(SAFE_SCRIPTS,"\"$file\"")===false) die("$file is not allowed");

//load the file that actually handles the http request
require $file;

Looking for feedback on what's the weakest link and/or how to improve.

Remove the additional information you are providing in your die statements. Just simply die quietly without revealing any information. It's similar to putting a zero byte index.html file in directories that you don't want people browsing.

Edit: Going away quietly without providing any information gives less information about your site to someone who may be trying to attack it. Any clues that you give them, like file not found for example, lets them know that they need to change their attack vector. The less information you can give them the better.


Insert sort not working for large arrays I've written an insert sort and for my assignment I must be able to sort an array of size 107. I've tested this sort with smaller arrays and it always sorts it correctly (up to size 105). If I make the array any bigger and try to output the sorted array nothing happens (I've waited 15 minutes just in case it was just talking long).

I was wondering if anybody had any information on how to fix this.  Maybe my code is just wrong or maybe I should be using something different.

This is the code for my sort. I've shuffled different sized arrays and it always gave the sorted array correctly though as stated before once an array was bigger than 105 nothing would happen!

public static void insertSort(int[] arr){

        int x, y;
        for(y=1; y< arr.length; y++) {
            int temp = arr[y];
            x = y;
            while(x>0 && arr[x-1] >= temp){
                arr[x] = arr[x-1];
                --x;
            }
            arr[x] = temp;
        }
}

This would be a good opportunity to learn some performance features and limitations of the Java Virtual Machine, and the available libraries.

I want to take your code, go through a staged progression of optimizations that will improve the way the code performs, and, while it will not change the runtime 'complexity' \$O(n^2)\$ of the insertion sort, it will improve the actual runtime.

As Nick has suggested, your variable names are not very meaningful. He's suggested insertIndex and sortIndex. I can use those, they are good. So your code becomes:

public static void insertSort(int[] data) {
    int insertIndex, sortIndex;
    for (sortIndex = 1; sortIndex < data.length; sortIndex++) {
        int temp = data[sortIndex];
        insertIndex = sortIndex;
        while (insertIndex > 0 && data[insertIndex - 1] >= temp) {
            data[insertIndex] = data[insertIndex - 1];
            --insertIndex;
        }
        data[insertIndex] = temp;
    }
}

Now, I also changed arr to data, and I added space around the operators and between loop keywords and braces. But, the above, is really your code, reformatted.

After that neatening up, there is one more things that need to happen: Variable scopes. Variables should be as narrow as possible. The sortIndex should be declared as part of the for loop control system, and the insertIndex should be declared inside the loop.

This is the result:

public static void insertSort(int[] data) {

    for (int sortIndex = 1; sortIndex < data.length; sortIndex++) {
        int temp = data[sortIndex];
        int insertIndex = sortIndex;
        while (insertIndex > 0 && data[insertIndex - 1] >= temp) {
            data[insertIndex] = data[insertIndex - 1];
            --insertIndex;
        }
        data[insertIndex] = temp;
    }
}

Now, that's a good looking insertion sort.... but, can it go faster? Yes.

The insertion sort is basically an operation where you identify where the data belongs, and then you shift all the larger data to the right, and insert the value. The code above shifts the values one at a time. If you identify the position, and shift them all at once, using an optimized system, you get better results. First, an intermediate result:

public static void insertSort(int[] data) {

    for (int sortIndex = 1; sortIndex < data.length; sortIndex++) {
        int temp = data[sortIndex];
        int insertIndex = sortIndex;
        // find the insert point.
        while (insertIndex > 0 && data[insertIndex - 1] >= temp) {
            --insertIndex;
        }
        // shift the values
        for (int i = sortIndex; i > insertIndex; i--) {
            data[i] = data[i - 1];
        }
        data[insertIndex] = temp;
    }
}

Now, that may not look faster, and you're right, it is probably not faster, but, if we replace the second loop with:

public static void insertSort(int[] data) {

    for (int sortIndex = 1; sortIndex < data.length; sortIndex++) {
        int temp = data[sortIndex];
        int insertIndex = sortIndex;
        // find the insert point.
        while (insertIndex > 0 && data[insertIndex - 1] >= temp) {
            --insertIndex;
        }
        // shift the values
        System.arraycopy(data, insertIndex, data, insertIndex + 1, sortIndex - insertIndex);
        data[insertIndex] = temp;
    }
}

Now, we're cooking. How much, though?

Before we get there, I am going to suggest another change, which is to take the insides of the loop, and make it another function too, and also make some variables 'final', so the code looks like:

private static final void subInsertSort(final int[] data, final int sortIndex) {
    final int temp = data[sortIndex];
    int insertIndex = sortIndex - 1;
    while (insertIndex >= 0 && temp < data[insertIndex]) {
        insertIndex--;
    }
    insertIndex++;
    System.arraycopy(data, insertIndex, data, insertIndex + 1, sortIndex - insertIndex);
    data[insertIndex] = temp;
}

public static void insertSortSplit(final int[] data){

    for(int sortIndex = 1; sortIndex < data.length; sortIndex++) {
        subInsertSort(data, sortIndex);
    }
}

I know, you're going to ask "Why?"... let me get to that.

I took the liberty of writing a quick test, on my system, that looks like:

private static final long time(String name, Runnable torun) {
    long nanos = System.nanoTime();
    torun.run();
    long time = System.nanoTime() - nanos;
    System.out.printf("Ran %s in %.3fms%n", name, time / 1000000.0);
    return time;
}

Now, that's got some Java8 stuff in it, but, all it really does, is time how long it takes to run a function. If I have the following 3 functions though (all do the same thing, with different names:

public static void insertSortOrig(int[] data) {
    int insertIndex, sortIndex;
    for (sortIndex = 1; sortIndex < data.length; sortIndex++) {
        int temp = data[sortIndex];
        insertIndex = sortIndex;
        while (insertIndex > 0 && data[insertIndex - 1] >= temp) {
            data[insertIndex] = data[insertIndex - 1];
            --insertIndex;
        }
        data[insertIndex] = temp;
    }
}

public static void insertSortArrayCopy(int[] data) {

    for (int sortIndex = 1; sortIndex < data.length; sortIndex++) {
        int temp = data[sortIndex];
        int insertIndex = sortIndex;
        // find the insert point.
        while (insertIndex > 0 && data[insertIndex - 1] >= temp) {
            --insertIndex;
        }
        // shift the values
        System.arraycopy(data, insertIndex, data, insertIndex + 1, sortIndex - insertIndex);
        data[insertIndex] = temp;
    }
}

private static final void subInsertSort(final int[] data, final int sortIndex) {
    final int temp = data[sortIndex];
    int insertIndex = sortIndex - 1;
    while (insertIndex >= 0 && temp < data[insertIndex]) {
        insertIndex--;
    }
    insertIndex++;
    System.arraycopy(data, insertIndex, data, insertIndex + 1, sortIndex - insertIndex);
    data[insertIndex] = temp;
}

public static void insertSortSplit(final int[] data){

    for(int sortIndex = 1; sortIndex < data.length; sortIndex++) {
        subInsertSort(data, sortIndex);
    }
}

When I run the following test on those functions, I get interesting results:

private static void runAll(int size) {
    final int[] data = new int[size];
    final int[] rand = buildData(size);

    System.arraycopy(rand, 0, data, 0, data.length);
    time("ACopy " + size, () -> insertSortArrayCopy(data));
    checkData(data);

    System.arraycopy(rand, 0, data, 0, data.length);
    time("Split " + size, () -> insertSortSplit(data));
    checkData(data);

    System.arraycopy(rand, 0, data, 0, data.length);
    time("Orig " + size, () -> insertSortOrig(data));
    checkData(data);

}

The above test (I call it with 10000) runs the three functions, yours, the one with ArrayCopy, and the one that has the sort in two parts.

It does not matter what order I run them in, but they all take an array of 10,000 random values, and sort them. The Times I get are:

Ran ACopy 10000 in 16.418ms
Ran Split 10000 in 10.262ms
Ran Orig 10000 in 24.080ms

What is that saying? It's saying that I can make the code run more than twice as fast, without changing the algorithm.... Now, if you are running your code in about 10 hours for \$10^7\$ then my improvements will bring it to... maybe 3 hours? That's a big difference.... but, wait, there's more.

Why does the 'split' function work so much faster than the single function? The reason is that the inside function gets called 10,000 times, and the outside function gets called once. A method that is called once is not 'compiled' by the Java compiler, but is run in more of an interpreted fashion. By putting the inner code in a function, the compiler sees it running many times, and it 'hotspot' compiles it.... with lots of optimization.

We can use that to our advantage.... If I take lots of small datasets (say 1000  data sets between 1 and 1000 members large, and I run those functions for those data sets, and then I run it for bigger data sets, by the time the big data set is run, the methods are 'hot' and compiled, and the Java machine is said to be 'warmed up'. What sort of difference does it make?

Well, for each of the three sorts above, let's see, first, your code:

Ran TenK Cold in 24.166ms
Ran TenK Hot in 9.689ms
Ran Warmup in 103.707ms
Ran Sort 1 in 0.002ms
Ran Sort 10 in 0.000ms
Ran Sort 100 in 0.002ms
Ran Sort 1000 in 0.178ms
Ran Sort 10000 in 9.379ms
Ran Sort 100000 in 915.304ms

The first run, of 10000 values, took 24ms. After a warmup, it took 9.5ms... and as part of the increasing data sizes, it took 9.5ms again.

Using arraycopy, it becomes:

Ran TenK Cold in 16.085ms
Ran TenK Hot in 8.073ms
Ran Warmup in 93.131ms
Ran Sort 1 in 0.003ms
Ran Sort 10 in 0.001ms
Ran Sort 100 in 0.003ms
Ran Sort 1000 in 0.169ms
Ran Sort 10000 in 7.520ms
Ran Sort 100000 in 837.687ms

Even 'hot', that has saved about.... 20%.

Now, as the split function....:

Ran TenK Cold in 10.250ms
Ran TenK Hot in 5.979ms
Ran Warmup in 78.793ms
Ran Sort 1 in 0.002ms
Ran Sort 10 in 0.001ms
Ran Sort 100 in 0.003ms
Ran Sort 1000 in 0.081ms
Ran Sort 10000 in 6.224ms
Ran Sort 100000 in 698.875ms

Now it is down to about 6ms, so, that's 30% off the original.... that's 'hot'. The cold version is even better.

Now, what if I used the native sort in Java (Arrays.sort(int[]))?

Well, that's impressive:

Ran TenK Cold in 2.576ms
Ran TenK Hot in 2.013ms
Ran Warmup in 75.236ms
Ran Sort 1 in 0.003ms
Ran Sort 10 in 0.002ms
Ran Sort 100 in 0.019ms
Ran Sort 1000 in 0.155ms
Ran Sort 10000 in 0.500ms
Ran Sort 100000 in 4.884ms

See how much better it scales \$O(n \log{n})\$? When you have the appropriate algorithm for the job it makes a real difference.


Keep the sequential nature while updating the UI of the progress of the HTTP request Let's say that I have a method that makes an HTTP request that will take a long time to finish. While that's happening the UI is not updating because I'm doing it in the UI's thread.

One solution is to:

Show the "Loading" animation.
Start the HTTP request on a thread.
On the completion callback stop the "Loading" animation and work on the response.

That's all nice and good, but it means that I went from code that was sequential in nature to something that has callbacks or something similar (promises).

So, I'm trying to keep the sequential nature while updating the UI. Having something like await/async would help here, but even in that case the caller has to be aware of this (plus I need .NET 4.5, let's assume that this is not an option). So I came up with something that keep the sequential nature of the original call, doesn't affect the caller of the function makes the HTTP request but updates the UI.

So, here it is:

public class Async<T>
{
    public static T Run(Func<T> action)
    {
        var worker = new BackgroundWorker();
        var error = new Exception[] {null};
        var result = new[] {default(T)};

        // The original call will run in a separate thread.
        worker.DoWork += (sender, ea) => { result[0] = action(); };
        worker.RunWorkerCompleted += (sender, ea) => { error[0] = ea.Error; };
        worker.RunWorkerAsync();

        // While the work is being done in a separate thread, update the UI's event loop (is this safe???)
        while (worker.IsBusy)
        {
            Application.Current.Dispatcher.Invoke(DispatcherPriority.Background, new Action(delegate { }));
            Thread.Sleep(20);
        }
        // Re raise the exception if needed
        if (error[0] != null)
            throw error[0];

        return result[0];
    }
}

//Usage
var someVariable = 123;
var responseA = Async<T>.Run(DoSuperLongTask);
var responseB = Async<T>.Run(() =>
                    {
                        return DoSuperComplicatedTask(someVariable);
                    });

It'll be something like

await Task.Run(() => DoWork());
await Task.Run(() => DoWork());

I'm not an expert in GUIs, and I'm especially not an expert in WPF. So my question to you is:

Is this safe to use?

So far it seems to work okay.

Good you have a BackgroundWorker. If I understood correctly, let's say you want to update a button, whether to enable it or not. you can use this:

delegate void EnableButton(Button b, bool enable);
public void ActuallyEnableButton(Button b, bool enable)
{
    if (b.InvokeRequired)
    {
        EnableButton del = ActuallyEnableButton;
        b.Invoke(del, new object[] { b, enable });
        //in some cases add "return" statement
    }
    b.Enabled = enable;
}

Call ActuallyEnableButton(btn, true) for example, inside DoWork event of BackgroundWorker.

This applies to Label, TextBox, ComboBox, ListView, ProgressBar, etc. No need to use this code if you want to update ToolStripStatusLabel and ToolStripProgressBar.

Hope this helped.


Displaying "C++" in ASCII art with user inputting an integer I have a school assignment and would love it if you can criticize or give a couple of pointers/advice about my piece of code (don't give me the answer, breaks the point of the exercise).

Here's my work:

#include <iostream>
#include <iomanip>

using std::cin;
using std::cout;
using std::endl;
using std::setw;

int main()
{
//Declaring our variables
int number; //Integer input from the user

cout << " Please enter an integer :\n"; //Will display text
cin >> number; //The user's input will now be stored in the variable

//These following lines will compose the ascii art which spells out "c++"

cout << number << number << number << number << number << number << number
<< number << number << number << number << endl; //line 1

cout << number << number << number << setw(17) << number << number <<
setw(17) << number << number << endl; // line 2

cout << number << number << number << setw(17) << number << number <<
setw(17) << number << number << endl; // line 3

cout << number << number << number << setw(12) << number << number << number
<< number << number << number
<< number << number << number << number << number << setw(9) << number <<
number << number << number
<< number << number << number << number << number << number << number <<
endl; //line4

cout << number << number << number << setw(17) << number << number <<
setw(17) << number << number << endl; // line 5

cout << number << number << number << setw(17) << number << number <<
setw(17) << number << number << endl; // line 6

cout << number << number << number << number << number << number << number
<< number << number << number << number << endl; //line 7

return 0;
}

Graphical representation

From your 'number << number << number' thing it is not immediately obvious what is being drawn, and how to fix it in a specific way when needed.

By using the template where you just replace placeholder characters with what you need, you get more WYSIWYG-like result.

Input validation

Your code assumes that user inputs appropriate value. What if user inputs 1234? (You can get more fancy and guard against inputting non-numbers too)

#include <iostream>

using std::cin;
using std::cout;
using std::endl;
using std::string;
using std::to_string;

const auto ASCII_template = string("\
SSSSSSSSSSS\n\
SSS               SS           SS\n\
SSS               SS           SS\n\
SSS           SSSSSSSSSS   SSSSSSSSSS\n\
SSS               SS           SS\n\
SSS               SS           SS\n\
SSSSSSSSSSS");

string replace_all(const string& base_string, const string& search, const string& replace) {
  string s = string(base_string);
  size_t pos = s.find(search);
  size_t len = replace.size();
  while ( pos != string::npos ) {
     s.replace( pos, len, replace );
     pos = s.find(search);
  }
  return s;
}

unsigned int get_user_digit() {
  unsigned int result;
  do {
    cout << "Please enter a single digit:" << endl;
    cin >> result;
  } while (result >= 10);
  return result;
}

int main() {
  cout << replace_all(ASCII_template, string("S"), to_string(get_user_digit())) << endl;
  return 0;
}

You can try it online here.

I don't remember C++ that much, but something along those lines would work. Depending on how much of С++11 you can use, std::regex_replace would simplify replace_all function. If you can not use std::string, it wouldn't change that much either, although you'd have to use some C-like workaround.


Get client information with GET request in GO API For an API I have defined a "Client" struct which contains all the fields the client has in the database. When a GET request is made with the client ID the whole struct(The struct contains about 60 fields of different types) is returned with all available data. The request can be customized with a querystring "fields" which allow the user to return specific "Client" fields only regardless if they are empty, so omitempty or "-" will not work for my usecase.
GO playground example: https://play.golang.org/p/NTGxGQFz7-Y
type Client struct {
   Firstname   string `json:"firstname"`
   AltAddress  string `json:"alt_address"`
   Lastname    string `json:"lastname"`
}

Example: GET
GET /clients/id/1
Response:
{
  "firstname":   "Michael",
  "alt_address": "",
  "lastname":    "Smith"
}

Example: GET with querystring
Get /clients/id/1?fields=firstname, alt_address
Response:
{
  "firstname":   "Michael",
  "alt_address": ""
}

I have this method(objectToFilteredMap) which converts an interface(struct for example) to a map, removes the fields that are NOT defined in "fieldNames" and outputs it as a json object. But I feel like I am using to many "for" loops. Is there a more efficient way to filter out the unneeded fields? Or any other improvements would be appreciated.
var (
    regex = regexp.MustCompile(`^\w+$`)
)

func inSlice(slice []string, val string) bool {
    for _, item := range slice {
        if item == val {
            return true
        }
    }
    return false
}

func toMap(obj interface{}, fieldNames []string) (map[string]interface{}, error) {
    for k, f := range fieldNames {
        f = strings.TrimSpace(f)
        if !regex.Match([]byte(f)) {
            return nil, fmt.Errorf("Invalid field name: `%s`", f)
        }
        fieldNames[k] = strings.ToLower(f)
    }

    j, err := json.Marshal(obj)
    if err != nil {
        return nil, err
    }

    objMap := map[string]interface{}{}
    err = json.Unmarshal(j, &objMap)
    if err != nil {
        return nil, err
    }

    for k := range objMap {
        if !inSlice(fieldNames, k) {
            delete(objMap, k)
        }
    }

    if len(objMap) == 0 {
        return nil, fmt.Errorf("field(s) not found")
    }

    return objMap, nil
}

If you have m fields and n names then you are iterating O(m * n) times. For all fields and names, circa 60 * 60 = 3600 times. Only loop O(m) times:
for field, _ := range m {
    if !names[field] {
        delete(m, field)
    }
}

With some improvements:
package main

import (
    "encoding/json"
    "fmt"
    "regexp"
    "strings"
)

var fieldNameRegex = regexp.MustCompile(`^\w+$`)

func structToMap(client interface{}, fieldNames []string) (map[string]interface{}, error) {
    names := make(map[string]bool, len(fieldNames))
    for _, name := range fieldNames {
        name = strings.TrimSpace(name)
        if !fieldNameRegex.MatchString(name) {
            return nil, fmt.Errorf("Invalid field name: `%s`", name)
        }
        name = strings.ToLower(name)
        names[name] = true
    }

    j, err := json.Marshal(client)
    if err != nil {
        return nil, err
    }
    m := map[string]interface{}{}
    err = json.Unmarshal(j, &m)
    if err != nil {
        return nil, err
    }
    for field, _ := range m {
        if !names[field] {
            delete(m, field)
        }
    }
    if len(m) != len(names) {
        return nil, fmt.Errorf("field(s) not found")
    }
    return m, nil
}

type Client struct {
    AltAddress string `json:"alt_address"`
    Firstname  string `json:"firstname"`
    Lastname   string `json:"lastname"`
}

func main() {
    client := Client{
        AltAddress: "HalLo",
        Lastname:   "smith",
    }

    m, err := structToMap(client, []string{"alt_address", "firstname"})
    if err != nil {
        fmt.Println("error1:", err)
        return
    }
    j, err := json.Marshal(m)
    if err != nil {
        fmt.Println("error2:", err)
        return
    }
    fmt.Println(string(j))
}

Playground: https://play.golang.org/p/Qf8bdygEcVT
{"alt_address":"HalLo","firstname":""}


Mixing chrome.i18n (internationalization method) into templates I'm mixing chrome.i18n into my templates as templateHelpers with Backbone.Marionette.

templateHelpers: function () {
    return {
        //  Mix in chrome to reference internationalize.
        'chrome.i18n': chrome.i18n,
        instant: this.instant
    };
}

and here's my template:

<% if( instant ) { %>
    <img class="item-thumb" src="http://img.youtube.com/vi/<%= video.get('id') %>/default.jpg" />
<% } else { %>
    <img class="item-thumb" data-original="http://img.youtube.com/vi/<%= video.get('id') %>/default.jpg" />
<% } %>

<span class="item-title" title="<%= video.get('title') %>"><%= video.get('title') %>

    <% if( video.get('highDefinition') ) { %>
        <%= chrome.i18n.getMessage('hd') %> ·
    <% } %>

        <%= video.get('prettyDuration') %>

    <button class="button-icon playInStream" title="<%= chrome.i18n.getMessage('play') %>">

    </button>

    <button class="button-icon addToStream" title="<%= chrome.i18n.getMessage('enqueue') %>">

    </button>

    <button class="button-icon save" title="<%= chrome.i18n.getMessage('save') %>">

    </button>

The number of templates being rendered could well be in the thousands. Would it be greatly more efficient to pass the strings themselves into the template, i.e. call getMessage inside of templateHelpers, or does it not make much of a difference at all? My thought is that since I don't use the rest of chrome.i18n, I shouldn't be serializing it, but I'm not sure if that's the case.

I doubt performance will take much of a hit by passing chrome.i18n into your templates, but I still don't think you should do it.

Templates should be solely for visualising data where possible, sometimes this requires a small amount of logic, but you should keep it out whenever possible. I would much rather see this:

var i18n = chrome.i18n;

...

templateHelpers: function () {
    return {
        hdMessage: i18n.getMessage('hd'),
        instant: this.instant
    };
}

-

<% if( video.get('highDefinition') ) { %>
    <%= hdMessage %> ·
<% } %>

In the long run this prevents your templates from being cluttered with internationalisation specific code, which will also be useful if you ever need to change the library.