Small Formatting Library I am learning C++ and currently playing with variadic template functions and formatting string, So here is my small attempt to write a formatting function and provide a simple iostream-like interface for std::FILE, I am using '%' as a format specifier
f.e

  format("const CharT *str") == "const CharT *str" format("HelloWorld -
  %", 10) == "HelloWorld - 10"

here is the code:

#include <cstdlib>
#include <string>

namespace gupta {
using string_t = std::string;
using CharT = string_t::value_type;

inline string_t format(const CharT *str) { return str; }
template <typename T> inline string_t to_string(const T &f) { return std::to_string(f); }
inline string_t to_string(const char *str) { return std::string{str}; }
inline string_t to_string(string_t s) { return (s); }

template <typename T, typename... Ts>
string_t format(const CharT *str, const T &arg, const Ts &... args) {
  string_t res;
  for(; *str; str++) {
    if(*str == '%') {
      if(*(str + 1) == '%') {
        str++;
      }
      else {
        res += to_string(arg);
        return res += format(str + 1, args...);
      }
    }
    res += *str;
  }
  return res;
}

template <typename... Ts> auto fprint(std::FILE *f, const char *str, Ts &&... args) {
  auto s = std::move(format(str, std::forward<Ts>(args)...));
  return fwrite(s.data(), 1, s.size(), f);
}

template <typename... Ts> inline auto print(const char *str, Ts &&... args) {
  return fprint(stdout, str, std::forward<Ts>(args)...);
}

template <typename... Ts> inline auto debug(const char *str, Ts &&... args) {
  return fprint(stderr, str, std::forward<Ts>(args)...);
}

namespace detail {
  class _stdout_object {};
  class _stderr_object {};
} // namespace detail

namespace printing_shortcuts {

  template <typename T> detail::_stdout_object operator<<(detail::_stdout_object f, const T &arg) {
    auto s = to_string(arg);
    fwrite(s.data(), 1, s.size(), stdout);
    return f;
  }

  template <typename T> detail::_stderr_object operator<<(detail::_stderr_object f, const T &arg) {
    auto s = to_string(arg);
    fwrite(s.data(), 1, s.size(), stderr);
    return f;
  }

  detail::_stdout_object print() { return {}; }
  detail::_stderr_object debug() { return {}; }

} // namespace printing_shortcuts

using namespace printing_shortcuts;

} // namespace gupta

using namespace gupta::printing_shortcuts;

class test {};

std::string to_string(const test &) { return "test"; }

#include <assert.h>
int main() {
  using namespace gupta;
  assert(format("const CharT *str") == "const CharT *str");
  assert(format("HelloWorld - %", 10) == "HelloWorld - 10");
  print("%s\n", format("HelloWorld - %,%", 10, test{}));
  assert(format("HelloWorld - %,%", 10, test{}) == "HelloWorld - 10,test");
}

also, does it make sense to replace class _stdout_object while their pointers with something like

namespace detail {
  class _stdout_object;
  class _stderr_object;
} // namespace detail

namespace printing_shortcuts {

  template <typename T> detail::_stdout_object *operator<<(detail::_stdout_object *, const T &arg) {
    auto s = to_string(arg);
    fwrite(s.data(), 1, s.size(), stdout);
    return nullptr;
  }

  template <typename T> detail::_stderr_object *operator<<(detail::_stderr_object *, const T &arg) {
    auto s = to_string(arg);
    fwrite(s.data(), 1, s.size(), stderr);
    return nullptr;
  }

  detail::_stdout_object *print() { return nullptr; }
  detail::_stderr_object *debug() { return nullptr; }

} // namespace printing_shortcuts

Don't make unnecessary type aliases

using string_t = std::string;
using CharT = string_t::value_type;

Those neither add readability nor make the existing facilities easy to use. When seeing those, I always recall the matlab C++ code generator, and the code it generated was frankly rubbish (Your code is good, it's just that those aliases trigger the feeling of disgust).

Take by std::string_view

I know you tagged this as C++14, but support for C++17 language is already good on all compilers, and standard library implementations are catching up. std::string_view is just more explicit way of saying that you don't need a copy of a string, just wanna read it.

Less verbose ways to overload

I'd be inclined to use if constexpr or tag based dispatch. The latter would probably end up the same length as current code.

Don't inhibit RVO

auto s = std::move(format(str, std::forward<Ts>(args)...));

RVO/NRVO (named return value optimization) is an optimization performed by compiler which is a form of copy elision. Compilers are allowed to eliminate a copy in some cases (can't recall strictly in which cases) Move would happen anyway, and in C++17 the guaranteed copy elision would work anyway, too.

Easy to use correctly, hard to use incorrectly

Perhaps I read too much of "Nobody wants to read your sh*t", but I see some similarity between what Scott Meyers writes and what Steven Pressfield describes. Both try to convey idea of being user/reader-friendly. Guiding them at the correct usage/idea, rather than expecting them to know it (or at least to try one's best). At the moment, the code doesn't throw exception if number of arguments is not equal to number of replacement symbols in the format string, nor any other warning/error. Not everybody knows that the functions don't support custom types with operator<< overloaded. C++ users usually use std::fstream, or some other custom build streams, which the code doesn't support. Very few C++ programmers are paranoid about ADL, which the code will invoke in case the intended print didn't compile. Some ADL calls are extremely evil.

People praise code for being nice, and great interface and reasonable performance is sufficient definition of nice (IMO). Implementation can be amended as time goes by, but changing interface/contracts is rarely an option. Although implementation matters, having great interface offers much greater possibilities of optimizations.

Executable size is not that important

Size of iostream gets dwarfed by the rest of application, usually. Using standard library algorithms, or especially boost, will make additional size of iostream insignificant. It might be important in some cases, but those who encounter would usually have no std::FILE either, as they want to output to embedded 8*8 LED screen or something like that.

Alternative implementation

I would use pseudo runtime tuple indexing to access elements, rather than recursing. That would eliminate the weird edge case of no arguments, and make everything more consistent.

Important excerpt:

template <std::size_t static_index, typename ... ArgumentTypes>
std::string runtime_get(std::size_t runtime_index, std::tuple<ArgumentTypes...>&& rest) {
    if constexpr (static_index == std::size_t(-1)) {
        throw std::invalid_argument("number of format symbols is more than arguments");
    }
    else {
        if (runtime_index == static_index)
            return to_string(std::get<static_index>(rest));
        else
            return to_string(runtime_get<static_index - 1>(runtime_index, std::move(rest)));
    }
}

template <typename... Ts>
string_t format(const CharT *str, const Ts &... args) {
    std::size_t current_index = 0;
    string_t res;
    for (; *str; str++) {
        if (*str != '%') {
            res += *str;
            continue;
        }
        if (*(str + 1) == '%') {
            res += '%';
            ++str;
        }
        else {
            res += runtime_get<sizeof...(Ts) - 1>(current_index,
                std::forward_as_tuple(args...));
            ++current_index;
        }
    }
    return res;
}

It is easy to see that one can only pass data into a function, but trying to get the wanted element is impossible due to type being different depending on runtime index. The function usually compiles into a loop for differing user defined types, and into jump table in case of the same or only built in types.

Full code of alternative implementation:

#include <cstdlib>
#include <string>
#include <stdexcept>
#include <tuple>

namespace gupta {
    using string_t = std::string;
    using CharT = string_t::value_type;

    template <typename T> inline string_t to_string(const T &f) { return std::to_string(f); }
    inline string_t to_string(const char *str) { return std::string{ str }; }
    inline string_t to_string(string_t s) { return (s); }

    template <std::size_t static_index, typename ... ArgumentTypes>
    std::string runtime_get(std::size_t runtime_index, std::tuple<ArgumentTypes...>&& rest) {
        if constexpr (static_index == std::size_t(-1)) {
            throw std::invalid_argument("number of format symbols is more than arguments");
        }
        else {
            if (runtime_index == static_index)
                return to_string(std::get<static_index>(rest));
            else
                return to_string(runtime_get<static_index - 1>(runtime_index, std::move(rest)));
        }
    }

    template <typename... Ts>
    string_t format(const CharT *str, const Ts &... args) {
        std::size_t current_index = 0;
        string_t res;
        for (; *str; str++) {
            if (*str != '%') {
                res += *str;
                continue;
            }
            if (*(str + 1) == '%') {
                res += '%';
                ++str;
            }
            else {
                res += runtime_get<sizeof...(Ts) - 1>(current_index,
                    std::forward_as_tuple(args...));
                ++current_index;
            }
        }
        return res;
    }

    template <typename... Ts> auto fprint(std::FILE *f, const char *str, Ts &&... args) {
        auto s = std::move(format(str, std::forward<Ts>(args)...));
        return fwrite(s.data(), 1, s.size(), f);
    }

    template <typename... Ts> inline auto print(const char *str, Ts &&... args) {
        return fprint(stdout, str, std::forward<Ts>(args)...);
    }

    template <typename... Ts> inline auto debug(const char *str, Ts &&... args) {
        return fprint(stderr, str, std::forward<Ts>(args)...);
    }

    namespace detail {
        class _stdout_object {};
        class _stderr_object {};
    } // namespace detail

    namespace printing_shortcuts {

        template <typename T> detail::_stdout_object operator<<(detail::_stdout_object f, const T &arg) {
            auto s = to_string(arg);
            fwrite(s.data(), 1, s.size(), stdout);
            return f;
        }

        template <typename T> detail::_stderr_object operator<<(detail::_stderr_object f, const T &arg) {
            auto s = to_string(arg);
            fwrite(s.data(), 1, s.size(), stderr);
            return f;
        }

        detail::_stdout_object print() { return {}; }
        detail::_stderr_object debug() { return {}; }

    } // namespace printing_shortcuts

    using namespace printing_shortcuts;

} // namespace gupta

using namespace gupta::printing_shortcuts;

class test {};

std::string to_string(const test &) { return "test"; }

#include <assert.h>
#include <iostream>

int main() {
    using namespace gupta;
    std::cout << format("%% %", 12) << '\n';
    assert(format("%% %", 12) == "% 12");
}

Demo on Wandbox


Confirming existence of lapindromes The following description is taken from CodeChef.com's Lapindrome problem.

  Lapindrome is defined as a string which when split in the middle, gives two halves having the same characters and same frequency of each character. If there are odd number of characters in the string, we ignore the middle character and check for lapindrome. For example gaga is a lapindrome, since the two halves ga and ga have the same characters with same frequency. Also, abccab, rotor and xyzxy are a few examples of lapindromes. Note that abbaab is NOT a lapindrome. The two halves contain the same characters but their frequencies do not match.
  Your task is simple. Given a string, you need to tell if it is a lapindrome.

  Input:

  First line of input contains a single integer T, the number of test cases.

  Each test is a single line containing a string S composed of only lowercase English alphabet.

  Output:

  For each test case, output on a separate line: "YES" if the string is a lapindrome and "NO" if it is not.

  Constraints:

  \$1 ≤ T ≤ 100\$
  2 \$≤ |S| ≤ 1000\$, where |S| denotes the length of S.

  Example:

  Input:

6
gaga
abcde
rotor
xyzxy
abbaab
ababc

  Output:

YES
NO
YES
YES
NO
NO

#include<stdio.h>
int main()
{
    int t;
    scanf("%d",&t);
    while(t--)
    {
        char s[1005];
        scanf("%s",s);
         int len,b[strlen(s)],z;
         int i,j,count =0;
         len=strlen(s);
         for(z=0;z<len;z++)
         {
                b[z] =1;
         }
        if(len%2 !=0)
        {
            for(i=0;i<len/2;i++)
            {
                for(j=(len/2)+1;j<len;j++)
                {
                    if(s[i] == s[j])
                    {
                        if(b[i] && b[j] )
                        {
                            b[i]=0;
                            b[j]=0;
                            count+=1;
                        }
                    }
                }
            }
        }
         else if(len%2 ==0)
        {
            for(i=0;i<len/2;i++)
            {
                for(j=(len/2);j<len;j++)
                {
                    if(s[i] == s[j])
                    {
                        if(b[i] && b[j] )
                        {
                            b[i]=0;
                            b[j]=0;
                            count+=1;
                        }
                    }
                }
            }
        }
        if(count==len/2)
            {
                printf("YES\n");
            }
            else
            {
                printf("NO\n");
            }
    }
}

Firstly, there's no reason for us to dump all of this in main.

The logic for determining whether or not a string is a lapindrome can (and should) simply exist in a function that takes a string and returns a bool.

bool isLapindrome(const char *string);

This function has the single responsibility of determining whether or not the input string is a Lapindrome, and it returns a true/false indicating as much.

 if(len%2 !=0)
    {
        for(i=0;i<len/2;i++)
        {
            for(j=(len/2)+1;j<len;j++)
            {
                if(s[i] == s[j])
                {
                    if(b[i] && b[j] )
                    {
                        b[i]=0;
                        b[j]=0;
                        count+=1;
                    }
                }
            }
        }
    }
     else if(len%2 ==0)
    {
        for(i=0;i<len/2;i++)
        {
            for(j=(len/2);j<len;j++)
            {
                if(s[i] == s[j])
                {
                    if(b[i] && b[j] )
                    {
                        b[i]=0;
                        b[j]=0;
                        count+=1;
                    }
                }
            }
        }
    }

Correct me if I'm wrong, but by my eyes, the parts inside each half of these if and else are identical except the starting position of the inner loop.

That looks to be the case, so why don't we refactor, starting from the inside and working out.

Rather than nested if statements, we can simply have a single conditional.

if (s[i] == s[j] && b[i] && b[j]) {
    b[i] = 0;
    b[j] = 0;
    ++count;
}

And rather than effectively copy & pasting our loop, we can instead calculate our starting position of the inner loop more effectively:

for (i = 0; i < len/2; ++i) {
    for (j = (len/2 + len%2); j < len; ++j) {

Notice how this produces the same result?  Your version of the code is effectively...

if (someCalculation == 1) {
    // ...
        for (j = len/2 + 1; // ...
} else if (someCalculation == 0) {
    // ...
        for (j = len/2 + 0; // ...

(Anything % 2 can only be 1 or 0.)

So we've already eliminated two levels of nesting without even moving anything out to its own function.

for (i = 0; i < len/2; ++i) {
    for (j = (len/2 + len%2); j < len; ++j) {
        if (s[i] == s[j] && b[i] && b[j]) {
            b[i] = 0;
            b[j] = 0;
            ++count;
        }
    }
}

Some other comments regarding readability...

All of your variable names are unreadably and meaninglessly short.

What is s?
What is b?
What is i?
What is j?
What is z?
I assume len is length.  Why not just use length?

We also have this mess of variable declarations:

int len,b[strlen(s)],z;
int i,j,count =0;
len=strlen(s);

I'm not familiar enough with C to know how expensive strlen is, and while I'm sure it's not horrendous, I also can't imagine it's a completely free call.  And when it does make sense to avoid duplicating function calls, we definitely should.

So for starters:

int length = strlen(inputString);

Next, I'd use any one of the options in this Stack Overflow answer to get yourself a bool type for what b is supposed to be.

bool characterChecked[length];

And then, while it is generally okay to use 1-letter variables for loop iterators, I think we should make it more clear what's going on.  If you're writing C99 code, you can just declare them straight in the loop:

for(int i = 0; // ...

And this would be ideal, especially when we consider that this limits the variables scope to only within the loop.  A variable's scope should be as limited as possible.  A variable's scope should be no larger than necessary.

But if you're on C89, this isn't an option.  As such, we should declare our variables closer to where the for loop actually exists and potentially even leave a comment.

int i;
for(i = 0; //...

Single letter variable names aren't very self-documenting.  (Loop iterators do have some special historical treatment however.  But for your other single-letter variables, we definitely need better.)


Generating a complicated data structure in a more efficient manner I need to convert some CSV data into a format recognizable by some old script my work uses. Changing the format is pretty much out of the question. So I've written this:

$rate_plan_terms = [
  "pp",
  "01",
  "12",
  "24",
  "36"
]

def generate(fname)
  puts "generating doc for #{fname}"
  file = File.open(fname)
  standard_offer_hash = {}
  features_hash = {}

  file.readlines[1..-1].each do |row|
    # standard_offers
    _region, offer_cid, _offer_code, offer_name, product_cid, product_name, component_name, component_cid = *row.split(",")
#used? no   yes        no          yes         kinda        yes           yes             yes

    standard_offer_hash[product_cid] ||= {}
    standard_offer_hash[product_cid][:offerType] = product_name # just keep it as product_cid?
    standard_offer_hash[product_cid][:ratePlan] ||= []

    $rate_plan_terms.each do |term|
      rate_plan = {
        :ratePlanName       => offer_name,
        :ratePlanCode       => offer_cid,
        :ratePlanTerm       => term,
        :offerEffectiveDate => "",
        :ratePlanMRC        => ""
      }

      standard_offer_hash[product_cid][:ratePlan] << rate_plan unless standard_offer_hash[product_cid][:ratePlan].include? rate_plan # they will be repeated because of features
    end

    # features
    features_hash[component_cid] ||= {
      :featureName          => component_name,
      :featureCode          => component_cid,
      :featureType          => "FEATURE",
      :featureTerm          => "",
      :featureEffectiveDate => "",
      :featureMRC           => ""
    }

  end
  feature_arr = features_hash.map {|k,f| f}
  standard_offer_arr = standard_offer_hash.map {|k,o| o}
  product_arr = []

  pricelist = {
    :features => { :feature => feature_arr }
    :ChannelOrgType => "OCM",
    :Region => "ALL",
    :product => product_arr,
    :businessRelationship => "",
    :standardOffer => standard_offer_arr,
  }

  {:PriceList => pricelist}
end

Yeah, complicated. This is running on up to a million lines per file, usually two files, every day. So it needs to be faster than it currently is.

FYI, the CSV is loosely separated into many products belong to many offers, and products have many features, but two different products can have the same features. So there is a lot of repeating.

There seem to be two time sinks here. One is the Array#include? I'm running over and over again. I wanted to transform that into a build hash -> convert to array later approach I took with features_arr and standard_offers_arr to avoid doing searches. But it depends on the product_cid, which (could) change per row.

The second is those two hashes to arrays at the end. The standard_offer_hash is going to be huge. That's can't be very fast.

To be completely honest I just don't see a way to optimize this further. Any suggestions?

I can show some output for what the generated structure needs to look like, if it would help.

You open the file, but you don't explicitly close it. Bad karma.
You read all lines into memory, it seems, rather than go through them one by one. Memory-wise that's pretty inefficient.
Any reason for $rate_plan_terms to be global? That too seems like bad karma. A constant would make more sense, I think.
Have you considered using Ruby's bundled CSV parser? It'll be more robust.
It'd be easier to just use Hash#values rather than map { |k, v| v }

As for the use of include?, you skip that by making :rate_plan a hash, and use a key like "#{offer_name}#{offer_cid}". Simpler to explain in code, so see below.

require "csv"

def generate(fname)
  puts "generating doc for #{fname}"

  terms = %w{pp 01 12 24 36}.freeze # just made this local here, a const would still be better
  offers = {}
  features = {}

  CSV.foreach(fname, headers: true, skip_blanks: true) do |row|
    # If the CSV file has a proper header line, you can also access cells by their name
    _region, offer_cid, _offer_code, offer_name, product_cid, product_name, component_name, component_cid = *row.fields

    offers[product_cid] ||= {
      offerType: product_name
      ratePlan: {}
    }

    key = "#{offer_name}#{offer_cid}"
    offers[product_cid][:ratePlan][key] ||= terms.map do |term|
      {
        ratePlanName: offer_name,
        ratePlanCode: offer_cid,
        ratePlanTerm: term,
        offerEffectiveDate: "",
        ratePlanMRC: ""
      }
    end

    features[component_cid] ||= {
      featureName: component_name,
      featureCode: component_cid,
      featureType: "FEATURE",
      featureTerm: "",
      featureEffectiveDate: "",
      featureMRC: ""
    }
  end

  offsers.each do |key, offer|
    offer[:ratePlan] = offer[:ratePlan].values
  end

  {
    PriceList: {
      features: { feature: features.values }
      ChannelOrgType: "OCM",
      Region: "ALL",
      product: [],
      businessRelationship: "",
      standardOffer: offers.values
    }
  }
end

Still not pretty (it's a tad over the 5-10 line limit you might strive for for methods in Ruby).

It might be nice to break things into methods like feature_hash(name, cid), which would generate the separate hashes.

require "csv"

TERMS = %w{pp 01 12 24 36}.freeze

def generate(fname)
  puts "generating doc for #{fname}"

  offers = {}
  features = {}

  CSV.foreach(fname, headers: true, skip_blanks: true) do |row|
    # If the CSV file has a proper header line, you can also access cells by their name
    _region, offer_cid, _offer_code, offer_name, product_cid, product_name, component_name, component_cid = *row.fields

    offers[product_cid] ||= {
      offerType: product_name
      ratePlan: {}
    }

    key = "#{offer_name}#{offer_cid}"
    offers[product_cid][:ratePlan][key] ||= rate_plan_array(offer_name, offer_cid)

    features[component_cid] ||= feature_hash(component_name, component_cid)
  end

  offsers.each do |key, offer|
    offer[:ratePlan] = offer[:ratePlan].values
  end

  {
    PriceList: {
      features: { feature: features.values }
      ChannelOrgType: "OCM",
      Region: "ALL",
      product: [],
      businessRelationship: "",
      standardOffer: offers.values
    }
  }
end

def rate_plan_array(name, cid)
  TERMS.map do |term|
    {
      ratePlanName: offer_name,
      ratePlanCode: offer_cid,
      ratePlanTerm: term,
      offerEffectiveDate: "",
      ratePlanMRC: ""
    }
  end
end

def feature_hash(name, cid)
  {
    featureName: name,
    featureCode: cid,
    featureType: "FEATURE",
    featureTerm: "",
    featureEffectiveDate: "",
    featureMRC: ""
  }
end

Either way you cut it, though, it probably won't win any beauty contests.


Python memoization decorator I have spent all night whipping up this recipe. It's my first Python decorator. I feel like I have a full understanding of how decorators work now and I think I came up with a good object-oriented algorithm to automatically provide memoization. Please let me know what you think.

I made a few quick changes after pasting it in here so please let me know if my changes broke something (don't have an interpreter on hand).

"""This provides a way of automatically memoizing a function. Using this eliminates the need for extra code. Below is an example of how it would be used on a recursive Fibonacci sequence function:

    def fib(n):
        if n in (0, 1): return n
        return fib(n - 1) + fib(n - 2)

    fib = memoize(fib)

That's all there is to it. That is nearly identical to the following:

    _memos = {}
    def fib(n):
        if n in _memos:
            return _memos[n]
        if n in (0, 1):
            _memos[n] = n
            return _memos[n]
        _memos[n] = fib(n - 1) + fib(n - 2)
        return _memos[n]

The above is much more difficult to read than the first method. To make things even simpler, one can use the memoize function as a decorator like so:

    @memoize
    def fib(n):
        if n in (0, 1): return n
        return fib(n - 1) + fib(n - 2)

Both the first and third solutions are completely identical. However, the latter is recommended due to its elegance. Also, note that functions using keywords will purposely not work. This is because this memoization algorithm does not store keywords with the memos as it HEAVILY increases the CPU load. If you still want this functionality, please implement it at your own risk."""
class memoize:

    """Gives the class it's core functionality."""
    def __call__(self, *args):
        if args not in self._memos:
            self._memos[args] = self._function(*args)
        return self._memos[args]

    def __init__(self, function):
        self._memos = {}
        self._function = function

    # Please don't ask me to implement a get_memo(*args) function.

    """Indicated the existence of a particular memo given specific arguments."""
    def has_memo(self, *args):
        return args in self._memos

    """Returns a dictionary of all the memos."""
    @property
    def memos(self):
        return self._memos.copy()

    """Remove a particular memo given specific arguments. This is particularly useful if the particular memo is no longer correct."""
    def remove_memo(self, *args):
        del self._memos[args]

    """Removes all memos. This is particularly useful if something that affects the output has changed."""
    def remove_memos(self):
        self._memos.clear()

    """Set a particular memo. This is particularly useful to eliminate double-checking of base cases. Beware, think twice before using this."""
    def set_memo(self, args, value):
        self._memos[args] = value

    """Set multiple memos. This is particular useful to eliminate double-checking of base cases. Beware, think twice before using this."""
    def set_memos(self, map_of_memos):
        self._memos.update(map_of_memos)

"""Gives the class it's core functionality."""
def __call__(self, *args):
    if args not in self._memos:
        self._memos[args] = self._function(*args)
    return self._memos[args]

It's more idiomatic (and faster) to "ask for forgivness rather than ask for permission", e.g.:

def __call__(self, *args):
    try:
        return self._memos[args]
    except KeyError:
        value = self._function(*args)
        self._memos[args] = value
        return value


Fizz, Buzz, or FizzBuzz? I was messing around online at work the other day when our phone systems went down. I came across the R programming language. Now I'm not guru with programming by any means but I like to mess around with different languages here and there, I believe it helps me to become a better programmer. So I decided that I would try to solve the fizzbuzz challenge in R for those of you that don't know what fizzbuzz is:

  Write a program that prints the numbers from 1 to 100. But for multiples of three print "Fizz" instead of the number and for the multiples of five print "Buzz". For numbers which are multiples of both three and five print "FizzBuzz"

This is the first thing I've ever written in R and I would like to have some critique done on what I've accomplished so far:

fizzBuzz = function(range, x, y){
    for (i in seq(1, range, by=1)){
        if (i %% x == 0 & i %% y == 0){
            print('FizzBuzz')
        }
        else if (i %% y ==0){
            print('Buzz')
        }
        else if (i %% x == 0){
            print('Fizz')
        }
        else{
            print(i)
        }
    }
}
fizzBuzz(100, 3, 5)

Some key things I'd like to focus on, per norm feel free to critique everything;

Is it a little bit over kill to put this all in a function, obviously I could do it simply without the function, but is this considered good practice in R to put your solutions inside of functions? I only ask because I looked around at peoples code and didn't see very many functions in it.
Are there easier ways to create the range? I feel like seq(from, to, by=1) is a little harder to understand then range(from, to)

Just to add to @flodel's comments, here is a properly vectorized version. One can see this is quite a bit faster, outperforming the parallel option (on 7 cores) by a factor 5. This problem is a quite nice demonstration of why is is worth your time to think about vectorized code:

flodel_fizzbuzz <- function(range = 100, fizz = 3, buzz = 5) {
  s <- 1:range
  is.fizz <- s %% fizz == 0
  is.buzz <- s %% buzz == 0

  s[is.fizz] <- 'Fizz'
  s[is.buzz] <- 'Buzz'
  s[is.fizz & is.buzz] <- 'FizzBuzz'
  return(s)
}

Running the benchmarks from @Gerard's answer (note that vectorizedFizzBuzz isn't truly vectorized):

Unit: milliseconds
                      expr       min        lq      mean    median        uq       max neval  cld
      applyFizzBuzz(range) 296.52224 303.19205 315.76901 309.18860 320.01676 384.18786    20   c
 vectorizedFizzBuzz(range) 341.41121 356.82580 374.53857 362.45300 372.81459 461.65169    20    d
   parallelFizzBuzz(range) 117.40058 128.91812 158.66095 153.87976 170.76776 288.99613    20  b
  papasmurfFizzBuzz(range) 292.22177 301.48002 316.31151 307.31605 319.05426 393.53235    20   c
    flodel_fizzbuzz(range)  27.98101  29.15677  29.93554  29.93097  30.84586  32.09304    20 a

I do think functions are a nice way to perform these tasks.
The easiest way to create a range of subsequent integers is with :.

This tidyverse version does about equally well (with ~40 ms):

tidy_fizbuzz <- function(range = 100, fizz = 3, buzz = 5) {
  x <- 1:range
  dplyr::case_when(
    x %% (fizz * buzz) == 0 ~ "Fizz Buzz",
    x %% fizz == 0 ~ "Fizz",
    x %% buzz == 0 ~ "Buzz",
    TRUE ~ as.character(x)
  )
}


Collection of schedules I hava a collection of schedules - EventSchedules. I want to select from this collection today, tomorrow and after tomorrow schedules. The day start from 06:00 and end 6:00 of next day.

Examples:

For today the query return events start in range: 30 July 2012 06:00 - 31 July 2012 06:00
For tomorrow the range is: 31 July 2012 06:00 - 01 August 2012 06:00

Any advice for refactoring this code?

private void FillSchedulesByFilter()
{
    var filterTimeStart = DateTime.Today.AddHours(6);
    var filterTimeEnd = filterTimeStart.AddDays(1);

    TodayGroupedSchedules = EventScheduleList.Where(s => s.RecurrenceStart >= filterTimeStart &&
                              s.RecurrenceStart < filterTimeEnd).
                              GroupBy(p => p.BasePlace);

    filterTimeStart = filterTimeStart.AddDays(1);
    filterTimeEnd = filterTimeEnd.AddDays(1);

    TomorrowGroupedSchedules = EventScheduleList.Where(s => s.RecurrenceStart >= filterTimeStart &&
                               s.RecurrenceStart < filterTimeEnd).
                               GroupBy(p => p.BasePlace);

    filterTimeStart = filterTimeStart.AddDays(1);
    filterTimeEnd = filterTimeEnd.AddDays(1);

    AfterTomorrowGroupedSchedules = EventScheduleList.Where(s => s.RecurrenceStart >= filterTimeStart &&
                               s.RecurrenceStart < filterTimeEnd).
                               GroupBy(p => p.BasePlace);

}

I can see two issues with your code:

It doesn't work. Because the local variables filterTimeStart and filterTimeEnd are put in the closure, and the Where() lambdas are executed only when you iterate the resulting enumerables, all of your schedules will be the same as AfterTomorrowGroupedSchedules, because the last value assigned to the locals will be used. You can fix this by using ToArray().
It's not DRY. The whole LINQ expression is always exactly the same and the interval is always one day. You should extract that into another method.

The modified code:

private IEnumerable<IGrouping<string, EventSchedule>> GroupedSchedulesForADay(
    DateTime filterTimeStart)
{
    return EventScheduleList.Where(
        s => s.RecurrenceStart >= filterTimeStart &&
            s.RecurrenceStart < filterTimeStart.AddDays(1))
        .GroupBy(p => p.BasePlace)
        .ToArray();
}

private void FillSchedulesByFilter()
{
    var filterTimeStart = DateTime.Today.AddHours(6);

    TodayGroupedSchedules = GroupedSchedulesForADay(filterTimeStart);

    filterTimeStart = filterTimeStart.AddDays(1);

    TomorrowGroupedSchedules = GroupedSchedulesForADay(filterTimeStart);

    filterTimeStart = filterTimeStart.AddDays(1);

    AfterTomorrowGroupedSchedules = GroupedSchedulesForADay(filterTimeStart);
}

Technically, the ToArray() is not necessary anymore, because each invocation of the GroupedSchedulesForADay() method has its own local variable, but I think it's still a good idea.

Also, I was worried that AddHours(6) wouldn't work correctly in the days where daylight saving time changes, but it seems it does work.

If the collection was big, it could make sense to iterate it just once, instead of three times, and build the three collections manually item by item, but it seems that's not necessary here.


Append table cells to select boxes in indexed order I am having a huge brain fart with efficiency right now.

The idea here is if I have a static table (unfortunately formatted this way with the data I've received), how would I appropriately append a select option dropdown with every value from the table categorized easily?

I currently have it working, but it seems as if I could make it more efficient than my current statement. I have the feeling with all the DOM manipulation I have going on, if I tried to use this on a table with thousands of items it would lag the browser. Let's get to the code.

JS Fiddle: http://jsfiddle.net/z2V2p/1/

HTML:

      Item 1
      Value 1
      Prop 1

      Item 2
      Value 2
      Prop 2

      Item 3
      Value 3
      Prop 3

<select id="item"></select>
<select id="value"></select>
<select id="prop"></select>

Javascript/jQuery:

$('tr td').each(function() {
    var $this = $(this);
    var text = $this.text();
    var select = '<option value="'+text+'">'+text+'</option>';
    switch($this.index()){
        case 0:
          $('#item').append(select);
          break;
        case 1:
          $('#value').append(select);
          break;
        case 2:
          $('#prop').append(select);
          break;
        default:
          alert('Unexpected Error.');
     }
});

So, I suppose the questions are:

Is there any easy way to make this more efficient?
Perhaps utilizing an array to cache the values, and append them
from that?
Or is there an easier method of using index of the items instead of
my switch case that would render quicker and be more expandable for further tds if they were implemented?

I appreciate all of the help.

I've considered just ultimately converting the darn table to a json object, but I figured I'd reach out here first. (this will deploy thousands of times per week, and I don't want anything hard coded.)

Looking for a way to add every TD in its index of its TR to be appended to an existing select option based on the index value.

Basically re-categorize columns of td elements.

First little thing: var names are important. So

var option = '<option value="'+text+'">'+text+'</option>';

Second and main. When we are making webapps with JS, the main perfomance rule is:

Reduce access to DOM

I tried you code with 15000 tr's in Firefox and Chrome. I wrapped you code with

console.time('DOM');
...
console.timeEnd('DOM');

Results was:

Chrome - about 5000ms
Firefox - about 20000ms

Then i replaced $.append() with just strings concatenation

console.time('String');
var itemOptions = '', valueOptions = '', propOptions = '';
$('tr td').each(function() {
    var $this = $(this);
    var text = $this.text();
    var option = '<option value="'+text+'">'+text+'</option>';
    switch($this.index()){
        case 0:
            itemOptions += option;
          break;
        case 1:
            valueOptions += option;
          break;
        case 2:
            propOptions += option;
          break;
        default:
          alert('Unexpected Error.');
     }
});
$('#item').append(itemOptions);
$('#value').append(valueOptions);
$('#prop').append(propOptions);
console.timeEnd('String');

Results:

Chrome - about 1100ms (bravo, Chrome :))
Firefox - about 4000ms

If you will change your data source to a JSON or something, next optimisation will not need, but you can set first argument of .each() method (index of all td elements) to avoid using .index(). I noticed about 300-500ms of speed increase. So the last version is

console.time('String');
var itemOptions = '', valueOptions = '', propOptions = '';
$('tr td').each(function(index) {
    var $this = $(this);
    var text = $this.text();
    var option = '<option value="'+text+'">'+text+'</option>';
    var modulo = index % 3;
    switch(modulo){
        case 0 :
            itemOptions += option;
          break;
        case 1:
            valueOptions += option;
          break;
        case 2:
            propOptions += option;
          break;
        default:
          alert('Unexpected Error.');
     }
});
$('#item').append(itemOptions);
$('#value').append(valueOptions);
$('#prop').append(propOptions);
console.timeEnd('String');

Results:

Chrome - about 850ms
Firefox - about 3500ms


Reading a DHT11 to a file Disclaimer: not a Pythonista, first actual Python programme written right here.

I'm reading a DHT11 sensor from GPIO pin 4 on my Raspberry Pi (thanks Thomas Ward for sending the RPi to me), and I want to log it to a file which will later be parsed into a web-page.

So I wrote the following Python script which is called by a Cron job every minute, and it reads the sensor 5 times then logs the average, min and max to a file for humidity and temperature.

Without further ado, here's my crap:

#!/usr/bin/python

import sys
import Adafruit_DHT
import time

def getLine(array):
    return str(sum(array) / float(len(array))) + "," + str(min(array)) + "," + str(max(array))

sensor = Adafruit_DHT.DHT11
gpio = 4

h = []
t = []

count = 5
date = time.strftime("%Y-%m-%d %H:%M:%S", time.gmtime())

for num in range(count):
    th, tt = Adafruit_DHT.read_retry(sensor, gpio)
    h.append(th)
    t.append(tt)

with open("MainLog.txt", "a") as file:
    file.write(date + ": ")
    # Write Humidity data
    file.write(getLine(h))
    file.write(" | ")
    file.write(getLine(t))
    # Write line break
    file.write("\n")

I have no idea how "good" or "bad" it is, if it were C# I would know but it's not. All suggestions welcome.

When I get my new hardware (looks like two-three days from now) I'm going to be adding a BMP180 sensor (barometric pressure/altitude).

The Adafruit_DHT library is on GitHub: https://github.com/adafruit/Adafruit_Python_DHT

I'm going to be replacing this with a DHT22, but that's one LoC change:

sensor = Adafruit_DHT.DHT11

To:

sensor = Adafruit_DHT.DHT22

Here is a list of things I would improve:

define constants in upper-case (PEP8 reference)
put the main program logic into the if __name__ == '__main__': to avoid it being executed on import
naming according to PEP8 - getLine() to get_line() (or even better name would be something like get_stats())
remove unused sys import
follow the PEP8 import organization guidelines
use _ as a throw-away variable name (instead of num)
use str.format() instead of string concatenation
use more meaningful variable names

The code after the suggested fixes applied:

#!/usr/bin/python
import time

import Adafruit_DHT

SENSOR = Adafruit_DHT.DHT11
GPIO = 4
MEASUREMENTS_COUNT = 5

def get_stats(array):
    average = sum(array) / float(len(array))
    return "{average},{min},{max}".format(average=average, min=min(array), max=max(array))

if __name__ == '__main__':
    date = time.strftime("%Y-%m-%d %H:%M:%S", time.gmtime())

    humidities = []
    temperatures = []
    for _ in range(MEASUREMENTS_COUNT):
        humidity, temperature = Adafruit_DHT.read_retry(SENSOR, GPIO)
        humidities.append(humidity)
        temperatures.append(temperature)

    with open("MainLog.txt", "a") as file:
        file.write("{date}: {th} | {tt}\n".format(date=date, th=get_stats(humidities), tt=get_stats(temperatures)))

By the way, I've done something similar, but using the Particle Photon and both DHT11 and BMP180, getting the average values from both sensors - I was surprised how different the values from these sensors were (may be I did something wrong there).

I was also thinking about improving the way stats are formed and using a namedtuple for it:

from collections import namedtuple

Stats = namedtuple("Stats", ["average", "min", "max"])

Then, the get_stats could be:

def get_stats(measurements):
    return Stats(average=sum(measurements) / float(len(measurements)),
                 min=min(measurements),
                 max=max(measurements))

which will change the way we output to the file:

message_template = "{date}: {th.average}, {th.min}, {th.max} | {tt.average}, {tt.min}, {tt.max}\n"
with open("MainLog.txt", "a") as file:
    file.write(message_template.format(date=date, th=get_stats(humidities), tt=get_stats(temperatures)))

This looks a little bit more pythonic - I like the way we construct a message in a single place and keep the stats in a more cleaner manner.


Permutation of list in Python so that list[x] != list[x+1] Basically I need to order a list, so that an element is not the same as the element that comes after or before it. I searched for a solution but couldn't find one so I wrote this function myself.

For example, [1,2,2,3,1,3,3] should come out as [3,1,2,3,1,2,3], [3,2,3,1,2,3,1] or [2,1,3,1,3,2,3] and so on.

def unorder(alist):
    multi_lvl_list = []
    unord_list = []

    alist.sort()

    # makes a new list that holds lists where same elements
    # are grouped together
    start = 0
    for i in range(len(alist)):
        if i == len(alist)-1:
            if alist[i] != alist[i-1]:
                multi_lvl_list.append([alist[i]])
            else:
                multi_lvl_list.append(alist[start:])
        elif alist[i] != alist[i+1]:
            multi_lvl_list.append(alist[start:i+1])
            start = i+1
        i += 1

    multi_lvl_list.sort(key=len)

    '''
    goes over every list and pops out one element to a third list
    if there are many same elements they will be put
    to the end of the list
    '''
    while len(multi_lvl_list) > 0:
        for idx, val in enumerate(multi_lvl_list):
            unord_list.append(val.pop())
            if len(val) == 0:
                del multi_lvl_list[idx]

    '''
    go over the list in reverse. if x == x-1 try to put x in the
    beginning of the list. becourse there are
    more likely different elements
    '''
    for i in range(len(unord_list)-1, -1, -1):
        if unord_list[i] == unord_list[i-1]:
            for j in range(len(unord_list)):
                if j == 0:
                    if unord_list[i] != unord_list[0]:
                        unord_list.insert(0, unord_list[i])
                        del unord_list[i+1]
                elif unord_list[i] != unord_list[j] and unord_list[i] != unord_list[j+1]:
                    unord_list.insert(j+1, unord_list[i])
                    del unord_list[i+1]
        else:
            break

    return unord_list

It seems to work in cases where it's possible to make such a list, but I'm pretty sure there is a more efficient way and the code can be optimized.

Would some pseudo-code be enough for you?

Build a collections.Counter (dictionary subclass) of the elements in the list.
output list = []
previous element = None
Loop for the length of the list:
    Pick the most frequent element that is not the previous one.
    Append this to the output list.
    Decrement its count.

return the output list

There are efficient ways to order the list count entries and maintain the ordering to facilitate choosing the proper next element.  A simple one is to order the initial list of counts by frequency.  At each iteration, choose the first element.  Decrement the count and sort it into the rest of the array, but insist on moving it down at least one position.  Repeat until all counts are 0.

I found a later use for this algorithm, so I coded it up.  I included a tracing print statement in the critical loop, so we can watch it in operation.

import collections

def unsort(in_list):
    size = len(in_list)
    if size == 0:
        return None

    # Build a list of pairs: value and count,
    #   in descending order of count.
    count_dict = collections.Counter(in_list)
    count_list = sorted(list(count_dict.iteritems()), key=lambda x: x[1], reverse=True)
    move_list = [[x[0], x[1]] for x in count_list]
    out_list = []

    # If the highest count is more than half the elements plus a half, no solution is possible.
    if move_list[0][1] * 2 > size + 1:
        print "No solution possible"
        return

    # In each iteration, grab the element at the head of the list.
    # Place that value into the output queue.
    # Sort the element back into the list,
    #   moving it at least one element down to avoid repetition.
    for _ in range(size):
        first = move_list.pop(0)
        out_list.append(first[0])
        first[1] -= 1
        for i in range(1, len(move_list)):
            if first[1] > move_list[i][1]:
                move_list.insert(i, first)
                break
        else:
            move_list.append(first)
        print move_list, out_list

    return out_list

# Main program -- test driver
a, b, c, = 'a', 'b', 'c'    # easier notation for test cases
test_case = [
    [],
    [c, c, b, b, b, a, b, c, b],
    [c, c, b, b, b, b, b, c, b],    # Too many b's'
]

for case in test_case:
    print unsort(case), '\n'

Output:

Input: []
None

Input: ['c', 'c', 'b', 'b', 'b', 'a', 'b', 'c', 'b']
[['c', 3], ['b', 4], ['a', 1]] ['b']
[['b', 4], ['c', 2], ['a', 1]] ['b', 'c']
[['c', 2], ['b', 3], ['a', 1]] ['b', 'c', 'b']
[['b', 3], ['a', 1], ['c', 1]] ['b', 'c', 'b', 'c']
[['a', 1], ['b', 2], ['c', 1]] ['b', 'c', 'b', 'c', 'b']
[['b', 2], ['c', 1], ['a', 0]] ['b', 'c', 'b', 'c', 'b', 'a']
[['c', 1], ['b', 1], ['a', 0]] ['b', 'c', 'b', 'c', 'b', 'a', 'b']
[['b', 1], ['a', 0], ['c', 0]] ['b', 'c', 'b', 'c', 'b', 'a', 'b', 'c']
[['a', 0], ['c', 0], ['b', 0]] ['b', 'c', 'b', 'c', 'b', 'a', 'b', 'c', 'b']
['b', 'c', 'b', 'c', 'b', 'a', 'b', 'c', 'b']

Input: ['c', 'c', 'b', 'b', 'b', 'b', 'b', 'c', 'b']
No solution possible
None


Does refactoring a while loop increase CPU usage? While upgrading our code base to take advantage of new features in .Net 4.5, I'm trying to refactor our take of the classic Producer/Consumer algorithm, but I'm concerned my refactoring is going to increase the CPU usage.

Each instance of my WorkItemConsumer class is basically a System.Threading.Thread that is able to process at most MaxConcurrentHandlers items, each of which is handled by a TPL System.Threading.Task.

In my previous implementation, I made use of explicit locking to read from the Queue<T>. Also, the class wrapping the queue set an AutoResetEvent to signal when a new item has been produced. Here was the code:

protected override void Run(string[] args)
{
    while (true)
    {
        var index = WaitHandle.WaitAny(new[] {StopRequested, queue_.ItemsAvailable});
        if (index == 0)
            break;

        if (tasks_.Count == MaxConcurrentHandlers)
            WaitAndDiscardAnyCompletedTask();

        var item = queue_.PopItem();

        // "null" denotes a signal to stop processing subsequent work items

        if (item == null)
            break;

        tasks_.Add(CreateWorkItemHandler(item));
    }

    while(tasks_.Count > 0)
        WaitAndDiscardAnyCompletedTask();
}

I would like to take advantage of the BlockingCollection<T> class, which gives me most of the features I want. It allows a consumer to know when no more items will ever be published and it also allows my to remove any explicit locking. However, I don't have a way to being signaled when new work items are available.

Here is the new code:

    protected override void Run(string[] args)
    {
        while (!queue_.IsCompleted)
        {
            if (StopRequested.WaitOne(0))
                break;

            if (tasks_.Count == MaxConcurrentHandlers)
                WaitAndDiscardAnyCompletedTask();

            foreach (var item in queue_.GetConsumingEnumerable().Take(MaxConcurrentHandlers - tasks_.Count))
                tasks_.Add(CreateWorkItemHandler(item));
        }

        while(tasks_.Count > 0)
            WaitAndDiscardAnyCompletedTask();
    }

How would you refactor this code?

Well, after more resarch and testing, it appears that the GetConsumingEnumerable() method of the BlockingCollection<T> is actually blocking when there are no items to consume. And it handles the case where the collection is completed as well.

The final touch, it that, instead of using a ManualResetEvent to signal the consumer to stop, like the original code, I'm using a CancellationToken instead.

This allows me to simplify the code drastically.

Here is the final snippet:

    protected override void Run(string[] args)
    {
        try
        {
            foreach (var item in queue_.GetConsumingEnumerable(cancellation_.Token))
            {
                tasks_.Add(CreateWorkItemHandler(item));
                if (tasks_.Count == MaxConcurrentHandlers)
                    WaitAndDiscardAnyCompletedTask();
            }
        }
        catch (OperationCanceledException /* e */)
        {
        }

        while (tasks_.Count > 0)
            WaitAndDiscardAnyCompletedTask();
    }

PS: I should have done more testing before posting. But, I'm leaving the question and its answer here for reference.


Naïve optimization by stepping until the error falls below a threshold As an exercise, I wanted to rewrite this toy Python code in Haskell:

def f(x):
    return abs(42-x)**2

def improve(x):
    newX = x + 0.1
    return newX, f(newX)

def optimize(f, goal):
    x = 0
    err = f(x)
    while not err < goal:
        x, err = improve(x)
    return x, err

print(optimize(f, 0.5))

Ideone

My solution works but is quite ugly:

f :: (Num a) => a -> a
f x = abs(42-x)^2

improve :: (Fractional a) => (a -> b) -> a -> (a, b)
improve f x =
    let newX = x+0.1
    in (newX, f newX)

step :: (Fractional a, Ord b) => (a -> b) -> b -> a -> (a, b)
step f goal x =
    let
        newX = x+0.1
        err = f newX
    in
        if err < goal then (newX, err) else step f goal newX

optimize :: (Fractional a, Ord b) => (a -> b) -> b -> (a, b)
optimize f goal = step f goal 0

main :: IO ()
main = print $ optimize f 0.5

Ideone

I am trying to find a solution without explicit recursion using a fold or something, but did not have an idea yet on how to do it. Can anybody help me out?

The guys at #haskell pointed me to until. This makes it satisfactory to me.

f :: Num a => a -> a
f x = abs(42-x)^2

improve :: Fractional b => (b -> c) -> (b, a) -> (b, c)
improve f (x, _) = (newX, f newX) where newX = x+0.1

optimize:: (Ord a, Fractional b) => (b -> a) -> a -> b -> (b, a)
optimize f goal x = until isDone (improve f) (0, f x)
    where isDone (_, err) = err < goal

main :: IO ()
main = print $ optimize f 0.5 0

http://ideone.com/ZlC7x8


Use of static factory methods for vectors and matrices library I've been working on a Java-based mathematics library focusing on vectors and matrices. I plan to use it for an important upcoming project, so the classes are analogous to data types available in GLSL (e.g. Vector2 for GLSL's vec2, Matrix4 for GLSL's mat4, and so on).

I'm trying to only expose interface to clients (e.g. Vector2) and keep implementation classes package-private (e.g. Vector2f). I'm trying to use the static factory methods and, since I'm using Java 1.8, I'm trying to take advantage of the new option that allows static methods to be added to interfaces.

However, there's something I currently don't like too much and I'm wondering if there's a way to improve the code design to address it.

Here's what the interface looks like (I've omitted the non-static methods for brevity because they're not relevant to the question):

public interface Vector2 extends Measurable, Bufferable {

    static Vector2 createZeroVector() {
        return Vector2f.createZeroVector();
    }

    static Vector2 createFrom(float x, float y) {
        return Vector2f.createFrom(x, y);
    }

    static Vector2 createFrom(double x, double y) {
        return Vector2f.createFrom(x, y);
    }

    static Vector2 createFrom(final float[] values) {
        return Vector2f.createFrom(values);
    }

    static Vector2 createFrom(final double[] values) {
        return Vector2f.createFrom(values);
    }

    static Vector2 createNormalizedFrom(float x, float y) {
        return Vector2f.createNormalizedFrom(x, y);
    }

    static Vector2 createNormalizedFrom(double x, double y) {
        return Vector2f.createNormalizedFrom(x, y);
    }

    static Vector2 createNormalizedFrom(final float[] values) {
        return Vector2f.createNormalizedFrom(values);
    }

    static Vector2 createNormalizedFrom(final double[] values) {
        return Vector2f.createNormalizedFrom(values);
    }

    // ... non-static methods omitted ...
}

I'll briefly note that:

final class Vector2f implements Vector2 {
    // details omitted
}

Since the interface methods are static, they must be implemented in the interface itself. Classes implementing the interface don't get overrides because this can be resolved at compile time.

However, if a client goes ahead and does something like this:

public class Vector2d implements Vector2 {
    // a double-precision implementation
}

and then tries to use the interface as intended:

// ...
Vector2 vec = Vector2.createFrom(5f, 3f);

Then there seem to be at least 2 problems:

the interface knows about a specific implementation and can only return Vector2f instances, and
a client expecting a Vector2d implementation class gets a Vector2f instead!

Since the methods in the interface are static, there seems to be no way for the client to actually get an instance of the hypothetical Vector2d implementation without changing the interface itself.

How can I improve the code's design in order to resolve this problem for clients?

I've read other articles and I'm not aware of something else that might be helpful here, though I might be wrong.

To put it simply, you're using the wrong language elements to achieve your goals.

The purpose of interfaces is to serve as a contract.
They define a set of actions that implementations must be able to perform.
Although Java 8 made it possible to add static and default methods to an interface (and I'll have to read up on why they did that),
it's best when an interface contains no implementation at all.
Before adding static or default methods,
think twice, and question yourself if there's a better way.

In this example,
it seems quite clear that all those createFrom* methods are in fact utility methods,
that would be best in a utility class.
There's no point putting them anywhere else if they cannot be extended or overridden.
So it would be better to extract these to a Vector2dUtils class or similar.

And why does a Vector2d extend Bufferable and Measurable?
What does that even mean?
It seems like this interface is trying to do too much.
I strongly suspect violations of the single responsibility principle in your implementation.

  the interface class knows about a specific implementation and can only return Vector2f instances, and

Why does an interface know about any implementations at all? It really shouldn't.

  a client expecting a Vector2d implementation class gets a Vector2f instead!

Why would a client expect a specific implementation from a method that returns an interface type? It really shouldn't.

You seem deeply confused. I recommend further reading on the subject:

In Effective Java by Joshua Bloch (in this order):

Item 19: Use interfaces only to define types
Item 17: Design and document for inheritance or else prohibit it
Item 18: Prefer interfaces to abstract classes
Item 16: Favor composition over inheritance

Consider a related pattern used by java.util.Collections (source code), in particular the facilities provided by methods like .emptyList, .unmodifiableList, .synchronizedList
Consider a related pattern used by java.util.EnumSet (source code), especially how its various factory methods create either RegularEnumSet or a JumboEnumSet depending on what is appropriate given the input params, and the client just doesn't need to know whatever is the actual implementation.


Rotation view for std::vector I have this view class for constant time rotations of a std::vector:

#include <iostream>
#include <string>
#include <vector>

template<typename T>
class vector_view {

public:
    vector_view(std::vector<T>& vec) : vec{&vec}, offset{0} {}
    const T& operator[](int index) const {
        return (*vec)[(index + offset) % vec->size()];
    }

    T& operator[](int index) {
        return (*vec)[(index + offset) % vec->size()];
    }

    void rotate(int rotation_length) {
        offset = (offset + vec->size() - rotation_length) % vec->size();
    }

    friend std::ostream& operator<<(std::ostream& os, vector_view<T>& view) {
        os << "[";
        std::string separator = "";

        for (int index = 0; index < view.vec->size(); ++index)
        {
            os << separator << view[index];
            separator = ", ";
        }

        return os << "]";
    }

private:
    std::vector<T>* vec;
    int offset;
};

int main() {
    std::vector<char> v{'a', 'b', 'c', 'd', 'e'};
    vector_view<char> view{v};

    for (int i = 0; i < v.size(); ++i) {
        std::cout << view << std::endl;
        view.rotate(1);
    }

    std::cout << std::endl;

    for (int i = 0; i < v.size(); ++i) {
        std::cout << view << std::endl;
        view.rotate(-1);
    }
}

The demo output is:

[a, b, c, d, e]
[e, a, b, c, d]
[d, e, a, b, c]
[c, d, e, a, b]
[b, c, d, e, a]

[a, b, c, d, e]
[b, c, d, e, a]
[c, d, e, a, b]
[d, e, a, b, c]
[e, a, b, c, d]

Since I am still in the process of learning modern C++, please tell me anything I could improve here; such as:

Naming,
coding conventions,
API,
...

I'm assuming that you want your vector elements to be modifiable through this view, as vec is non-const.
You store a pointer to the underlying vector, but never test it isn't null.  I think it would be better to have it as a reference member.
If you want your view to be as usable as the standard vector, you'll want to provide iterators, and begin() and end() to obtain them.  You may also want implement front(), back() and at().  Methods such as size(), reserve, capacity etc. can simply forward to the contained vector.

Like std::vector, you should override operator[] on const:
const T& operator[](int index) const {
    return (*vec)[(index + offset) % vec->size()];
}

T& operator[](int index) {
    return (*vec)[(index + offset) % vec->size()];
}

Once we've implemented size() or begin()/end(), the streaming operator << uses only the public interface, so it no longer needs to be friend.
Its loop compares (signed) int against (unsigned) size_t (g++ warns about this with -Wall).  Prefer to avoid implicit promotions, so declare i as size_type to match size().  If you're pedantic, that should be vector_view<T>::size_type, which you'll forward from std::vector<T>.  My code below avoids that entirely, by using iterators.

Beware the gotcha when T is bool.  The only thing preventing your code  working on vector<bool> is:
T& operator[](int index);

If you return a std::vector<T>::reference instead, that will do the right thing there.  (std::vector<bool>::reference is a proxy object, not a bool&).
My modified class:
#include <iterator>
#include <vector>

template<typename T>
class vector_view {
    using vector_type = std::vector<T>;

public:
    using value_type = typename vector_type::value_type;
    using allocator_type = typename vector_type::allocator_type;
    using size_type = typename vector_type::size_type;
    using difference_type = typename vector_type::difference_type;
    using reference = typename vector_type::reference;
    using const_reference = typename vector_type::const_reference;
    using pointer = typename vector_type::pointer;
    using const_pointer = typename vector_type::const_pointer;

    // Iterators: I've implemented little more than the minimum to
    // make the test program work.  You could expand this, and also
    // provide a specialization of std::iterator_traits<> for each
    // one.
    struct iterator {
        vector_view& v;
        size_type i;
        iterator& operator++() { ++i; return *this;}
        iterator& operator-- () { --i; return *this;}
        reference operator*() const { return v[i]; }
        bool operator!=(const iterator other) const
            { return &v != &other.v || i != other.i;  }
    };

    struct const_iterator {
        const vector_view& v;
        size_type i;
        const_iterator& operator++() { ++i; return *this;}
        const_iterator& operator-- () { --i; return *this;}
        const_reference operator*() const { return v[i]; }
        bool operator!=(const const_iterator other) const
            { return &v != &other.v || i != other.i;  }
    };

    using reverse_iterator = std::reverse_iterator<iterator>;
    using const_reverse_iterator = std::reverse_iterator<const_iterator>;

    explicit vector_view(vector_type& vec, int offset = 0)
        : vec{vec},
          offset{offset}
    {}

    const reference operator[](size_type index) const {
        return vec[(index + offset) % vec.size()];
    }

    reference operator[](size_type index) {
        return vec[(index + offset) % vec.size()];
    }

    void rotate(int rotation_length) {
        auto size = vec.size();
        offset = (offset + size - rotation_length) % size;
    }

    void set_rotation(int rotation_length) {
        offset = rotation_length % vec.size();
    }

    int rotation() const {
        return offset;
    }

    // Iterators
    iterator begin() {
        return iterator{*this, 0};
    }
    const_iterator begin() const {
        return const_iterator{*this, 0};
    }
    const_iterator cbegin() const {
        return begin();
    }
    iterator end() {
        return iterator{*this, size()};
    }
    const_iterator end() const {
        return const_iterator{*this, size()};
    }
    const_iterator cend() const {
        return end();
    }
    // You may wish to implement rbegin/rend and crbegin/crend

    bool empty() const { return vec.empty(); }
    size_type size() const { return vec.size(); }

    // You may also want:
    // reserve(), capacity()
    // clear(), insert(), emplace(), erase(), ..., etc.

private:
    vector_type& vec;
    int offset;
};

// Consider implementing operator==(), operator<() and other
// comparisons (as non-member functions, probably).  You could
// probably use std::mismatch() for that.

// Printing: greatly simplified using range-based `for`.
#include <iostream>
#include <string>
template<typename T>
std::ostream& operator<<(std::ostream& os, const vector_view<T>& view) {
    os << "[";

    std::string separator = "";
    for (auto element: view) {
        os << separator << element;
        separator = ", ";
    }

    return os << "]";
}

// Test program: exercises both `normal` and boolean vectors.
int main() {
    std::vector<char> v{'a', 'b', 'c', 'd', 'e'};
    vector_view<char> view{v};

    for (size_t i = 0; i < view.size(); ++i) {
        std::cout << view << std::endl;
        view.rotate(1);
    }
    std::cout << std::endl;

    for (size_t i = 0; i < view.size(); ++i) {
        std::cout << view << std::endl;
        view.rotate(-1);
    }
    std::cout << std::endl;

    std::vector<bool> bvec{{false, true, true, false, false}};
    auto bview = vector_view<bool>{bvec};

    for (size_t i = 0; i < bview.size(); ++i) {
        bview.set_rotation(i);
        std::cout << bview << std::endl;
    }
}


Ensure instance of class is initialized before using I'm beginning to work with VBA class modules more in Excel and want to make sure that no properties can be used if the class hasn't been initialized. Below is the class and my attempt to at doing so.

Private Type helper
    Prop1 As Boolean
    Prop2 As Boolean
    Prop3 As String
    Prop4 As String
End Type

Private m_initialized As Boolean

Private this As helper

Public Sub Init(ByVal Prop1 As Boolean, ByVal Prop2 As Boolean, ByVal Prop3 As String, ByVal Prop4 As String)
    this.Prop1 = Prop1
    this.Prop2 = Prop2
    this.Prop3 = Prop3
    this.Prop4 = Prop4

    m_initialized = True
End Sub

Public Property Get Prop1() As Boolean
    If Not m_initialized Then ThrowNotInitializedError

    Prop1 = this.Prop1
End Property

Public Property Get Prop2() As Boolean
    If Not m_initialized Then ThrowNotInitializedError

    Prop2 = this.Prop2
End Property

Public Property Get Prop3() As String
    If Not m_initialized Then ThrowNotInitializedError

    Prop3 = this.Prop3
End Property

Public Property Get Prop4() As String
    If Not m_initialized Then ThrowNotInitializedError

    Prop4 = this.Prop4
End Property

Public Function Self() As Foo
    Set Self = Me
End Function

Private Sub ThrowNotInitializedError()
    Err.Raise 5, "Init() not run.", "Values weren't initially assigned. Make sure they are assigned before use"
End Sub

The repetition of If Not m_initialized Then ThrowNotInitializedError is the part that I don't like. I can already imagine myself forgetting to put it on a property. Are there any other options?

Testing in a standard module to make sure it worked as I expected

Sub TestingFooInitializeProtection()
    Dim properlyInitialized As Foo
    Set properlyInitialized = New Foo
    properlyInitialized.Init True, False, "Input3", "Input4"
    Debug.Print properlyInitialized.Prop1
    Debug.Print properlyInitialized.Prop2
    Debug.Print properlyInitialized.Prop3
    Debug.Print properlyInitialized.Prop4

    Dim improperlyInitialized As Foo
    Set improperlyInitialized = New Foo
    'improperlyInitialized.Init False, True, "input3", "input4"
    Debug.Print improperlyInitialized.Prop1 'If above line is left commented out error is thrown.
    Debug.Print improperlyInitialized.Prop2
    Debug.Print improperlyInitialized.Prop3
    Debug.Print improperlyInitialized.Prop4
End Sub

A couple of quick points.

If you are setting up a class, don't encapsulate a Type inside of it - you are only repeating what a class does! I am not sure where this anti-pattern comes from.

Within VBA do not raise errors unless absolutely necessary. Error raising and trapping will lead to unintended consequences and you can never be confident that they will be handled properly. Always handle the error explicitly. For starters, they can disrupt global variables.

You can create a Function in a module (let's call it NewFoo) with the correct signature - which creates the Foo instance and runs the Init method. This will make it a bit easier in the rest of your code to ensure you have the right sequence.

In Excel, you could make use of the xlError values to return an error if not initialised. This means that all your properties would be Variants (which has its own issues), something along the line of:

Public Property Get Prop1() As Variant
    Prop1 = IIF(m_initialised, CBool(Prop1_var), CVErr(xlErrNA))
End Property

Also, explicitly set m_initialised in the Class_Initialize routine to False. In theory not needed, but I prefer to be explicit.


Very small JonTron quoter This is one of my first projects, just a quick hobby thing for fun.

#include <iostream>
#include <chrono>
#include <thread>
using namespace std;
using namespace std::chrono;
using namespace std::this_thread;
using std::chrono::system_clock;

int random();

int main()
{
    cout << "Choose a JonTron episode!" << endl;
    string JonTron;
    getline(cin,JonTron);
    cout << "You chose " << JonTron << "." << endl;
    if (JonTron == "Aquaman")
        cout << "What's this blue pants? What-- what's this blue pants?" << endl;
else if (JonTron == "Anti-Drug Games")
    cout << "Too late, talking leg!" << endl;
else if (JonTron == "Banjo-Kazooie")
    cout << "CAAAAAAAAAAAAARS??? CARS?!?!?!" << endl;
else if (JonTron == "California Games")
    {cout << "SWOOD THIS! SWOOD THAT! What the FUCK does SWOOD mean?!?!?!" << endl;
    sleep_for(milliseconds(1000));
    cout << "I used to say it all the time in Cali! Grommet this, grommet that!";
    }
else if (JonTron == "Nightshade")
    {
    cout << "Congratulations! You've just won the Jef and Paul Award for Excellence in Shopping Centers!"<< endl;
    sleep_for(milliseconds(3000));
    cout << "Actually, the crowbar snaps in two." << endl;
    sleep_for(milliseconds(3000));
    cout << "Just kidding." << endl;
    sleep_for(milliseconds(5000));
    cout << "Fool me once, I'm mad." << endl;
    sleep_for(milliseconds(5000));
    cout << "Fool me twice, how could you?" << endl;
    sleep_for(milliseconds(5000));
    cout << "Fool me three times, you're officially that guy, okay--" << endl;
    sleep_for(milliseconds(3000));
    cout << "You know the one, you go to a bar, and he's like," << endl;
    sleep_for(milliseconds(3000));
    cout << "'This suit is ehh, official, it's a Giorgio Armani, actually, my dad knows him'," << endl;
    sleep_for(milliseconds(3000));
    cout << "FUCK YOU!" << endl;
    sleep_for(milliseconds(1000));
    cout << "I AIIIIIIIIIN'T HAVIN' THAT SHIT!!" << endl;
    }
else if (JonTron == "The Skateboard Kid")
    cout << "You just wanna hang up somebody's coat, and the couch falls out!" << endl;
else if (JonTron == "The Zoo Race")
    cout << "What'd they program this in, C--??" << endl;
    else
        cout << "What the fuuuuuck?" << endl;
cout << "You done now? We done? (Press any key [and then hit enter, duy] to be done.)" << endl;
float fuckingdummyvar;
cin >> fuckingdummyvar;
}

My current main concern is the very large block of if-elseif-else. To put things concisely, is there any less gigantically ugly way to do this?

You could use a switch statement instead of the if elses to make it prettier.
Or, you could use a map<string, vector<string>>

Instead of using whole namespaces, restrict usage to single items.
In order to not pollute your own namespace with names, do single 'using' instead of 'using namespace'. The following will suffice for your code.
using namespace std;
using chrono::milliseconds;
using chrono::system_clock;
using this_thread::sleep_for;

Instead of float fuckingdummyvar; cin >> fuckingdummyvar;, do a single getchar() or use any other method described here.

Use consistent bracing.
This is a matter of opinion, but I prefer to always have braces around if and else statements, even when only one statement follows. I used to not put any braces, but then I ran into a nice bug that was seriously puzzling me when I added a return to an else and it would always return for some reason. That's because the return was the second statement after the else and there weren't brackets around it.

Use a map to avoid big 'n' dirty blocks of code..
#include <iostream>
#include <string>
#include <chrono>
#include <thread>
#include <map>
#include <vector>

using namespace std;
using chrono::milliseconds;
using chrono::system_clock;
using this_thread::sleep_for;

int main()
{
    map<string, vector<string>> quotes{
                                        {"Aquaman", {"What's this blue pants? What-- what's this blue pants?"}},
                                        {"California Games", { "SWOOD THIS! SWOOD THAT! What the FUCK does SWOOD mean?!?!?!", "I used to say it all the time in Cali! Grommet this, grommet that!" }},
                                        {"The Zoo Race", {"What'd they program this in, C--??"}}
                                      }; //In C++11 and up {"String1", "String2"} is a vector<string> = {"String1", "String2"}

    cout << "Choose a JonTron episode!" << endl;
    string JonTron;
    getline(cin, JonTron);

    if (quotes.count(JonTron)) { //Checks if there is a map key with the episode that the user inputted.
        for (auto &s : quotes[JonTron]) { //Iterates over every string in the vector<string> that's stored in quotes[JonTron].
            cout << s << endl;
            sleep_for(milliseconds(1000)); //If you want custom sleep times as well, do a map<string, customClass with vector of quotes and vector of milliseconds.
        }
    }
    else {
        cout << "Episode not found!.";
    }

    getchar();
}

Use a text file with all the quotes and episodes.
The best way would be to create a text file and retrieve the quotes from there.


Find first threshold crossing in numpy array Given the following artificially generated data:

t_steps = 30

data = np.array([
    np.arange(t_steps) * .05,
    np.arange(t_steps) * .1,
    np.arange(t_steps) * .2,
    np.arange(t_steps) * .3

])

I find the time-step the each line of data has passed a threshold. If it does not pass the given threshold, I assign a time-step of -1:

react_tms = []
thresh = 3.5

for dat in data:
    whr = np.where(dat > thresh)
    if len(whr[0]) == 0:
        react_tms.append(-1)
    else:
        react_tms.append(whr[0][0])

This gives:

[-1, -1, 18, 12]

Is there some way to do this without the for-loop? Even before the for-loop is removed, should I be using something other than np.where to find the threshold crossing?

In principle you can use numpy.argmax for this. The only problem is that if no value is above the threshold, the maximum is False, so it returns 0 as the index of the maximum. We therefore need to subtract 1 for those cases:

above_threshold = data > thresh
react_tms = np.argmax(above_threshold, axis=1)
react_tms = react_tms - (~np.any(data > thresh, axis=1)).astype(float)
print(react_tms)
# array([ -1.,  -1.,  18.,  12.])

Whether or not that is really more readable, I am not sure. It is, however slightly faster than using numpy.where (and probably also faster than a list comprehension): https://stackoverflow.com/q/16243955/4042267

In the end this does not really matter:


Project Euler: #36 Binary and Base 10 Palindromes Here is the Euler problem referenced, it says:

  The decimal number, 585 = 1001001001\$_2\$ (binary), is palindromic in both
  bases.

  Find the sum of all numbers, less than one million, which are
  palindromic in base 10 and base 2.

  (Please note that the palindromic number, in either base, may not
  include leading zeros.)

My solution is as follows:

def check_palindrome_base_ten(num):
    a = str(num)
    return a == a[::-1]

def check_palindrome_base_two(num):
    a = str(bin(num))[2:]
    return a == a[::-1]

def check_palindrome(num):
    return check_palindrome_base_ten(num) and check_palindrome_base_two(num)

def sum_palindrome_in_a_range(lower_limit, upper_limit):
    return sum(x for x in xrange(lower_limit, upper_limit+1, 2) if check_palindrome(x))

%timeit sum_palindrome_in_a_range(1,1000000)
1 loops, best of 3: 247 ms per loop

I noticed by the end of the solution after just brute forcing it that the step could be changed to 2 to use only odd numbers because the binary 1st digit should always be 1 if the number was greater than 1 and a palindrome. This cut my execution time in literal half from ~480ms to 247ms.

In addition I thought that perhaps writing mini functions and then running a reverse index on that number would be faster than doing say:

str(a) == str(a)[::-1]

Because I get to avoid running str and bin twice. Is that correct logic?

Are there any such other optimizations that I have missed that I can use to reduce runtime and in general make my code more useful. I feel as though I may be stuck in a for loop / list comprehension trap and perhaps am not thinking creatively enough when approaching these solutions. I'm scared that's going to cause my solutions when I code actual problems to be inefficient. So perhaps it's also a code methodology review in that respect.

Are there any such other optimizations that I have missed that I can use to reduce runtime and in general make my code more useful.

Yes. You're thinking about the problem backwards. As far as I can tell, your solution may be as fast as you can do in Python solving the problem directly. That is, something like:

sum(filter(satisfies_problem, xrange(1, 1000000))

But we can do way better. Palindromes are fairly sparse, so rather than go through a million numbers and checking them to see, we can actually just generate all the palindromes in one base and just check if they're a palindrome in the other base. Basically, for every number from 1 to 1000 we can just add its reverse with or without duplication (e.g. 146 can become the palindromes 14641 and 146641). That is:

def make_palindrome(p, repeat):
    result = p
    if not repeat: p //= 10
    while p > 0:
        result = result * 10 + p % 10
        p //= 10
    return result

Then we can just generate all the right palindromes, base 10, and check if they match base 2:

total = 0
cap = upper_limit / 10 ** (math.log10(upper_limit)/2)
for p in xrange(int(cap) + 1):
    for repeat in (True, False):
        pal = make_palindrome(p, repeat)
        if pal & 1 and lower_limit <= pal <= upper_limit:
            as_bin = bin(pal)[2:]
            if as_bin == as_bin[::-1]:
                total += pal

return total

Timing comparison, this is 100x faster:

Brute Force Search    0.3477s
Palindrome Generator  0.0030s


Expression template to compute the Euclidean distance I was writing some geometry-related code again and had a closer look at my function supposed to compute the Euclidean distance between two points (N-dimensional points by the way, hence the N template parameter). Here is a simplified version:

template<std::size_t N, typename T>
auto distance(const Point<N, T>& lhs, const Point<N, T>& rhs)
    -> T
{
    T res{};
    for (std::size_t i = 0 ; i < N ; ++i)
    {
        auto tmp = std::abs(lhs[i] - rhs[i]);
        res += tmp * tmp;
    }
    return std::sqrt(res);
}

So far, so good. However, one very common operation is to compare the distances. Generally speaking, when comparing the distances, the sqrt is optimized away and the sum of the squares is compared instead of the distance itself. Therefore, I tried to create some kind of expression template to represent the distance between two points, so that users will benefit from both the ease of use and the "get rid of sqrt optimization" when comparing distances. Basically, the call of sqrt is not done until the exact value of the distance is needed. Here is the class:

template<typename T>
struct DistanceExpression
{
    explicit constexpr DistanceExpression(T data):
        _data(data)
    {}

    operator T() const
    {
        return std::sqrt(_data);
    }

    bool operator==(const DistanceExpression& other) const
    {
        return _data == other._data;
    }

    bool operator!=(const DistanceExpression& other) const
    {
        return !(*this == other);
    }

private:

    T _data;
};

My new distance function is implemented as such:

template<std::size_t N, typename T>
auto distance(const Point<N, T>& lhs, const Point<N, T>& rhs)
    -> DistanceExpression<T>
{
    T res{};
    for (std::size_t i = 0 ; i < N ; ++i)
    {
        auto tmp = std::abs(lhs[i] - rhs[i]);
        res += tmp * tmp;
    }
    return DistanceExpression<T>{res};
}

Here is a minimal working code at Coliru. Is such a design reasonable or is it overkill to elegantly solve this problem?

My concern now is that you make multiple calls to get the actual distance.
Every-time you do that then you incur the expense of the sqrt() operation.

I suppose it depends on the exact use case of your application which happens more often. But if this is for general code that could be used a lot in either capacity. I would add another two fields to indicate if it was dirty or not and cache the result so you don't have to recompute the actual value each time.

But that has the side affect of adding a conditional branch into the get which can also be expensive when looking at micro optimizations.


Redirecting the web client after deleting an item In my web-app, it is possible to delete an Item when visiting any of 4 different pages:

the Show Item page
the Edit Item page
the List Items page
the List Items in Room page

If the visitor uses a delete button on either of the 2 List pages, they should remain on their current page when the deletion is complete. But if the visitor is on the Show or Edit pages, they should be redirected to  the List Items in Room page when deletion is complete.

I have the following method in a Symfony3 controller, which uses the HTTP referer header:

/**
 * @Route("/delete/{id}", name="ci_item_delete", requirements={"id"="\d+"}, options={"expose"=true})
 * @Security("has_role('ROLE_USER') && is_granted('delete', item)")
 * @param Request $request
 * @param Item $item
 * @return RedirectResponse
 */
public function deleteAction(Request $request, Item $item)
{
    $referer = $request
        ->headers
        ->get('referer');
    $rpath = parse_url($referer, PHP_URL_PATH );
    $badreferrers = array(
        $this->generateUrl('ci_item_show', ['id' => $item->getId()]),
        $this->generateUrl('ci_item_edit', ['id' => $item->getId()])
    );

    if(in_array($rpath, $badreferrers) ){ //coming from a page that won't exist once the Item is deleted
        $redirect = $this->generateUrl('ci_item_list_format_room', ['format_id'=> $item->getFormat()->getId(), 'room_id'=> $item->getRoom()->getId()]);
    } else {
        $redirect = $referer;
    }

    $em = $this->getDoctrine()->getManager();
    $em->remove($item);
    $em->flush();
    $this->addFlash('success', 'msg.delete.item.success');

    return $this->redirect($redirect);
}

Would it be better to have different routes for the different use cases? Something like this?

/**
 * @Route("/delete/{id}", name="ci_item_delete", requirements={"id"="\d+"}, options={"expose"=true})
 * @Route("/deletefromlist/{id}", name="ci_item_delete_from_list", requirements={"id"="\d+"}, options={"expose"=true})
 * @Security("has_role('ROLE_USER') && is_granted('delete', item)")
 * @param Request $request
 * @param Item $item
 * @return RedirectResponse
 */
public function deleteAction(Request $request, Item $item)
{
    $referer = $request
        ->headers
        ->get('referer');

    if($request->get('_route') == 'ci_item_delete' ){ //coming from a page that won't exist once the Item is deleted
        $redirect = $this->generateUrl('ci_item_list_format_room', ['format_id'=> $item->getFormat()->getId(), 'room_id'=> $item->getRoom()->getId()]);
    } else {
        $redirect = $referer;
    }

    $em = $this->getDoctrine()->getManager();
    $em->remove($item);
    $em->flush();
    $this->addFlash('success', 'msg.delete.item.success');

    return $this->redirect($redirect);
}

Or is there another better way altogether?

There really is not much tangible difference between the two. I think I would not want to introduce a second route that does precisely the same thing as another route. I think it is sufficient to differentiate the redirect behavior based on the request properties.

In first, code example I think $badreferrers is a bad variable name. There is nothing bad about the referrer right? Those refererr values lead to a different redirect.


Extracting certain products from a webpage using Scrapy I've written a script in Python Scrapy to harvest various product names and price from books.toscrape. The reason I submit this tiny code to Code Review is because, in Python 3 when it comes to work with Scrapy and parse some data from a web, the csv output looks awkward (if the csv is derived from default command, as in scrapy crawl toscrapesp -o items.csv -t csv). The results found in such CSV file are with a uniform gap between two lines that means there is a line gap between each two rows. I've fixed it using the below script. I didn't use default command to get the CSV output; rather, I've written few lines of code in spider class and got the desired output.

Although It is running smoothly, I'm not sure it is the ideal way of doing such thing. I expect someone to give any suggestion as to how I can improve this script.

"items.py" includes:

import scrapy

class ToscrapeItem(scrapy.Item):
    Name = scrapy.Field()
    Price = scrapy.Field()

Spider contains:

import csv
import scrapy

outfile = open("various_pro.csv", "w", newline='')
writer = csv.writer(outfile)

class ToscrapeSpider(scrapy.Spider):

    name = "toscrapesp"
    start_urls = ["http://books.toscrape.com/"]

    def parse(self, response):
        for link in response.css('.nav-list a::attr(href)').extract():
            yield scrapy.Request(url=response.urljoin(link), callback=self.collect_data)

    def collect_data(self, response):
        global writer
        for item in response.css('.product_pod'):
            product = item.css('h3 a::text').extract_first()
            value = item.css('.price_color::text').extract_first()
            yield {'Name': product, 'Price': value}
            writer.writerow([product,value])

Please click this link to see what I was having earlier. Upon executing the script, I get CSV output with no line gap or blank rows.

I don't think you should reinvent the wheel and provide your own CSV export. The following works for me as is (note the addition of .strip() calls - though I don't think they are necessary at all):

import scrapy

class ToscrapeSpider(scrapy.Spider):
    name = "toscrapesp"
    start_urls = ["http://books.toscrape.com/"]

    def parse(self, response):
        for link in response.css('.nav-list a::attr(href)').extract():
            yield scrapy.Request(url=response.urljoin(link), callback=self.collect_data)

    def collect_data(self, response):
        for item in response.css('.product_pod'):
            product = item.css('h3 a::text').extract_first().strip()
            value = item.css('.price_color::text').extract_first().strip()
            yield {'Name': product, 'Price': value}

Running it with scrapy runspider spider.py -o output.csv -t csv produces a CSV file with no blank lines:

Price,Name
£53.74,Tipping the Velvet
£29.69,Forever and Forever: The ...
£55.53,A Flight of Arrows ...
£36.95,The House by the ...
£30.25,Mrs. Houdini
£28.08,The Marriage of Opposites
...


Three implementations of observable objects I wonder, what might be the best (most pythonic) way of implementing multiple observerables in Python.  There are multiple objects, which should be able to register themselves at one or multiple observables.

Shall I simply submit the update methods, like this:

class MethodObservable():
    def __init__(self, data_source):
        self.observers = []
        self.data_source = data_source

    def add(self, callback):
        self.observers.add(callback)

    def remove(self, callback):
        try:
            self.observers.remove(callback)
        except ValueError:
            pass

    def notify():
        for callback in self.observers:
            callback(self.data_source)

class MethodObserver():
    def __init__(self, observer1, observer2):
        observer1.add(self.update_for_1)
        observer2.add(self.update_for_2)

    def update_for_1(self, data_source):
        # code

    def update_for_2(self, data_source):
        # code

data_source1 = SomeObject()
data_source2 = SomeOtherObject()
observable1 = MethodObservable(data_source1)
observable2 = MethodObservable(data_source2)
observer = MethodObserver(observable1, observable2)

Or is it better, to come up with a dictionary of classes and methods:

class DictObservable():
    def __init__(self, data_source):
        self.observers = {}
        self.data_source = data_source

    def add(self, observer, callback):
        self.observers[observer] = callback

    def remove(self, observer):
        try:
            del self.observers[observer]
        except ValueError:
            pass

    def notify():
        for callback in self.observers.values():
            callback(self.data_source)

class DictObserver():
    def __init__(self, observer1, observer2):
        observer1.add(self, self.update_for_1)
        observer2.add(self, self.update_for_2)

    def update_for_1(self, data_source):
        # code

    def update_for_2(self, data_source):
        # code

data_source1 = SomeObject()
data_source2 = SomeOtherObject()
observable1 = DictObservable(data_source1)
observable2 = DictObservable(data_source2)
observer = DictObserver(observable1, observable2)

Or is probably the best way, that a class, which registers to a specific observer should have a specified update/callback-method, which kind of enforces the implementation of an interface:

class SpecifiedObservable():
    def __init__(self, data_source, callback_method='update'):
        self.observers = []
        self.data_source = data_source
        self.callback_method = callback_method

    def add(self, observer):
        self.observers.add(observer)

    def remove(self, observer):
        try:
            self.observers.remove(observer)
        except ValueError:
            pass

    def notify():
        for observer in self.observers:
            getattr(observer, self.callback_method)(self.data_source)

class SpecifiedObserver():
    def __init__(self, observer1, observer2):
        observer1.add(self)
        observer2.add(self)

    def update_for_1_specified(self, data_source):
        # code

    def update(self, data_source):
        # code

data_source1 = SomeObject()
data_source2 = SomeOtherObject()
observable1 = SpecifiedObservable(data_source1, 'update_for_1_specified')
observable2 = SpecifiedObservable(data_source2)
observer = SpecifiedObserver(observable1, observable2)

Maybe there are even better options, I wonder, which one should be preferred?

First thing, you should use new-style classes, inheriting from object. (Not necessary if you are in Python3, but convenient for backwards compatibility).

So, in short, your choices are:

Store them in a list
Store them in a dictionary

Of the two, the dictionary is the best option, because the keys are stored in a hash table, and you can look up items in O(1), whereas in the list you have to transverse the whole thing until you find it, so it is O(N).

Now, note that your dictionary implementation is a very thin layer on top of a dict. I would recommend inheriting from dict. If you are bound to the specific interface (.add, .remove) instead of the __setitem__ and pop, you can just create aliases. This can help you reduce the amount of code and also give you easier access to all the expresiveness of Python dicts.

Note that the third method is broken. In particular:

self.observers.add(observer)

when self.observers is a list, that doesn't have the method add.


Calling one method both dynamically and statically I'm currently working on a PHP component: "Filesystem handler". I've run into a problem where I want to call one exact method in both dynamic and static ways.

I've ended up with this solution. Could you please give me a feedback ? I want to know, if there is a better or a simpler way to achieve the same result.

<?php

class File {

protected $file;

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

protected static function exists($arg) {
    var_dump($arg);
}

public function __call($method, $parameters = null) {
    if($method == 'exists') {
        return call_user_func_array(array($this, 'exists'), array(array($this->file)));
    }
}

public static function __callStatic($method, $parameters) {
    if($method == 'exists') {
        return call_user_func(__CLASS__.'::exists', $parameters);
    }
}

}

$file = new File('dynamic');
$file->exists();

File::exists('static');

The first thing you should now is that magic methods are slow __call and __callStatic, that is. The second thing I'd like to point out is that, given that you declared the method as static, you should always call it as such. Failing to do so is just going to leave you with code that is a nightmare to maintain: every call to every function will need to be checked and double checked, just to know if that method is indeed static or not.

Lastly: why aren't you simply calling the method statically on your instance?

$file = new File('dynamic');
$file::exists();

That'll work just fine, too. But all in all: a method should be either static (which isn't that useful in PHP 9/10 times anyway) or not. Trying to allow for static calls aswell as instance calls means you are writing a class that offers a messy API. Don't