Project Euler #16 in Swift - Power digit sum I just finished Project Euler #16 in Swift, and since there is not any version yet on Code Review, I would like to have some comments on what I did to try to improve it.

  2^15 = 32768 and the sum of its digits is 3 + 2 + 7 + 6 + 8 = 26.

  What is the sum of the digits of the number 2^1000?

import Foundation

func addMulToResult(inout results:[Int], var mulResult:Int, index:Int) {

    if index == results.count {
        results.append(0)
    }

    mulResult = results[index] + mulResult

    var result = mulResult % 10

    results[index] = result

    var retain = mulResult / 10
    if retain > 0 {
        addMulToResult(&results, retain, index + 1)
    }
}

func infMult(left:[Int], right:[Int]) -> [Int] {

    var results:[Int] = [Int]()
    for (j, rightNumber) in enumerate(reverse(right)) {

        for (i, leftNumber) in enumerate(reverse(left)) {

            let mulResult = leftNumber * rightNumber
            addMulToResult(&results, mulResult, i + j)
        }
    }

    return results.reverse()
}

func infPow(x:Int, var power:Int) -> [Int] {

    var result = [1]
    var xArray = intToIntArray(x)
    while power > 0 {

        result = infMult(result, xArray)
        power--
    }

    return result
}

func intToIntArray(var number:Int) -> [Int] {
    var intArray:[Int] = []

    while number > 0 {

        intArray.append(number % 10)
        number /= 10
    }

    return intArray.reverse()
}

func powerDigitSum(x:Int, var power:Int) -> Int {
    let powerSum = infPow(x, power)

    let result = reduce(powerSum, 0) { $0 + $1 }

    return result
}

func euler16() {

    let number = powerDigitSum(2, 1000)

    println(number)
}

func printTimeElapsedWhenRunningCode(operation:() -> ()) {
    let startTime = CFAbsoluteTimeGetCurrent()
    operation()
    let timeElapsed = CFAbsoluteTimeGetCurrent() - startTime
    println("Time elapsed : \(timeElapsed) s")
}

printTimeElapsedWhenRunningCode(euler16)

The code executes in 0.06s

Let's start with a small note: The power parameter in

func powerDigitSum(x:Int, var power:Int) -> Int { ... }

need not be variable, so you can remove the var.

You use arrays to store "large integers", and the most significant digit is
stored in the first array element (at index 0). As a consequence, the arrays
are reversed in infMult() and the result is reversed again.

It would be easier to store the least significant digit at index 0 of the
arrays. This simply means that you remove all reverse() calls in
infMult() and intToIntArray().

This reduces the computation time slightly from 0.027 to 0.02 seconds on my
computer.

Another improvement would be to store more than one decimal digit in each
array element. On the OS X platform, Int is a 64-bit integer, so that
you can safely store 8 decimal digits in the array elements without
risking an overflow when multiplying two "large digits".

So you would define

let BASE = 100000000

replace all occurrences of 10 by BASE in your code, and change
powerDigitSum() to

func digitSum(var x : Int) -> Int {
    var result = 0
    while x > 0 {
        result += x % 10
        x /= 10
    }
    return result
}

func powerDigitSum(x:Int, power:Int) -> Int {
    let powerSum = infPow(x, power)
    let result = reduce(powerSum, 0) { $0 + digitSum($1) }
    return result
}

This reduces the computation time to 0.011 seconds.

The greatest performance improvement can be achieved by using a better
exponentiation algorithm in infPow(), such as Exponentiation by squaring (see also https://codereview.stackexchange.com/a/70197/35991
for a nice explanation).

In this context this would look like

func infPow(x:Int, var power:Int) -> [Int] {

    var result = [1]
    var square = intToIntArray(x)

    if power > 0 {
        if power % 2 == 1 {
            result = infMult(result, square)
        }
        power /= 2
    }
    while power > 0 {
        square = infMult(square, square)
        if power % 2 == 1 {
            result = infMult(result, square)
        }
        power /= 2
    }
    return result
}

This reduces the computation time to 0.0002 seconds.


Checking whether a number starts with any of a list of string prefixes This code will return false if the long double rand begins with any string in the vector prefix.

bool prefix_vect(long double rand, std::vector<std::string> prefix) {
    std::string str_rand = std::to_string(rand);
    for(auto i : prefix) {
        if(boost::starts_with(str_rand, i))
            return false;
    }
    return true;
}

This code uses #include <boost/algorithm/string/predicate.hpp>.

How can I improve this code? Is there a way to make it fit into one line, such that it could be used without a function at all, and instead in an if-statement?

I am not experienced with C++ lambda expressions, is this something that could use them?

As a note: I don't care if it returns true or false, the Boolean can be switched if it makes this easier to optimize.

Edit: This isn't my main account, just a throwaway (unable to merge accounts, sadly).

It's better to pass containers as references to avoid copying like this:

bool prefix_vect(long double rand, const std::vector<std::string>& prefix)

Also, when iterating on sequence, references also can improve performance:

for(const auto& i : prefix) {

As for one-liner I'd used something like this:

bool prefix_vect2(long double rand, const std::vector<std::string>& prefix) {
    std::string str_rand = std::to_string(rand);
    return std::none_of(prefix.begin(), prefix.end(),
             [&str_rand](const std::string& s){ return boost::starts_with(str_rand, s); });
}


Sanitising user inputs and user inputted data outputs I am creating a function for sanitising user inputs and user inputted data outputs.

Please offer advise on any improvements which could be made:

function cleanse($input) {

    $search = array(
        '@<script[^>]*?>.*?</script>@si',   // Strip out javascript
        '@<[\/\!]*?[^<>]*?>@si',            // Strip out HTML tags
        '@<style[^>]*?>.*?</style>@siU',    // Strip style tags properly
        '@<![\s\S]*?--[ \t\n\r]*>@'         // Strip multi-line comments
    );

    $output = preg_replace($search, '', $input);
    return $output;
}

function sanitise($input) {
    if (is_array($input)) {
        foreach($input as $var=>$val) {
            $output[$var] = sanitize($val);
        }
    } else {
        $input  = cleanse($input);
        $output = htmlspecialchars($input, ENT_QUOTES,'UTF-8',false);
    }
    return $output;
}

Personally I tend to work with a white-list of characters.

So instead of sanitizing and removing bad stuf, I simply only accept good stuff.
e.g. [0-9]+ when validating an age field

This approach is less error prone because you don't have to think of all the bad things one can enter. And you know that the data you are serving is actualy what you tink it is (e.g. a number when referencing age).


Bit shifting and masking I was wondering if someone could review this simple bit of code that originally was in C++, but now converted to C# as practice in using bit-shifting and creating a sort of a quasi-array functionality. Consider the partition size as a size of our array which can only hold a specific size limit for said partition. It was assumed that input is always valid, and thus no fail safes were included.  The usage of unsigned integers is intentional as we want only positive values.

If you're wondering about the liberal usage of type casting, it is because it appears that C# is far stricter than C++ with respect to data types.

using System;

namespace bitShifting
{
    class Program
    {

        uint bitSize, shiftCount, mask, partionSize;

        void setValue(ref uint var, uint k, uint i, uint val)
        {

            bitSize = sizeof(uint) * 8;

            partionSize = (uint)(bitSize / k);
            shiftCount = partionSize * i;

            mask = (uint)~(((1 << (int)partionSize) - 1) << (int)shiftCount);
            var = var & mask;
            val = val << (int)shiftCount;
            var = var | val;

        }
        uint getValue(uint var, uint k, uint i)
        {
            bitSize = sizeof(uint) * 8;

            partionSize = (bitSize / k);
            shiftCount = partionSize * i;

            mask = (uint)(((1 << (int)partionSize) - 1) << (int)shiftCount);
            var = var & mask;
            var = (var >> (int)shiftCount);
            return var;
        }

        static void Main(string[] args)
        {
            Program p = new Program();

            uint k;
            uint value;
            uint maxValue;

            uint var = 0;

            Console.WriteLine("C++ Lab to C#, Practice Using Pass by Ref");

            Console.WriteLine("Valid Partition K size are: 1, 2, 4, 8, 16, 32.");
            Console.WriteLine("Enter Partition Size");

            k = Convert.ToUInt32(Console.ReadLine());

            maxValue = (uint)((1 << (int)(sizeof (uint) * 8 / k)) - 1);

            if(k == 1)
            {
                maxValue = 2147483647;
            }
            Console.WriteLine("Maximal Value for Partion of Size {0} is 0 to {1}", k, maxValue);

            bool getValue = true;
            for(uint i = 0; i < k; i++)
            {
                Console.WriteLine("Input a Value");
                value = Convert.ToUInt32(Console.ReadLine());

                if (k == 1)
                {
                    Console.WriteLine("Value at Index 0 is: {0}", value);
                    getValue = false;
                }
                else
                {
                    p.setValue(ref var, k, i, value);
                }
            }

            for(uint i = 0; i < k; i++)
            {
                if (getValue)
                {
                    value = p.getValue(var, k, i);
                        Console.WriteLine("Value At Index {0} is: {1}", i, value);
                }
            }

        }
    }
}

Layout

Put your main method on the top, since it tells main purpose of the application. And, C# isn't like C++, you don't have to define the method ahead, before calling them.

class Program
{
    uint bitSize, shiftCount, mask, partionSize;

    static void Main(string[] args);

    void setValue(ref uint var, uint k, uint i, uint val);
    uint getValue(uint var, uint k, uint i);
}

 // Even in c++, you can declare a method signature first
 // This part could also go into header file
 void setValue(PUINT var, UINT k, UINT i, UINT val);
 uint getValue(UINT var, UINT k, UINT i);

 int main(int argc, char *argv[]) { /* ... */ }

 // And, implement them later
 void setValue(PUINT var, UINT k, UINT i, UINT val) { /*...*/ }
 uint getValue(UINT var, UINT k, UINT i) { /*...*/ }

Mark the class variables and methods static, so you can call them directly.

// Instead of awkward hack/patch like this
Program p = new Program();
p.setValue(...);

Declare the local variables as you need them. Except when the assignment is inside a code block, but you also need to use them outside of that.

Naming

PascalCasing for any method, class, public property. And, camelCasing for private and local variable.
Avoid using keywords as variable name, especially var the most used one.
Avoid single letter variable, other than for for loop, lambda expression...

Misc

Avoid magic numbers.

// What is 8? numbers of bits in a byte
//var maxValue = (uint)((1 << (int)(sizeof(uint) * 8 / k)) - 1);

const uint BitsPerByte = 8;

//maxValue = 2147483647;
maxValue = int.MaxValue;

You messed up big here...

A uint should be capable to store uint.MaxValue and not int.MaxValue.
You are mixing up partition size and partition count. With a partition size of 1 (k = 1), you should be able to enter 32 values (BitSize / k = 32 / 1).

// And, not just 1 (`k`) value
for(uint i = 0; i < k; i++)

The hard coded conditions k == 1 and getValue are huge red flags!

//  why?
if(k == 1)
{
    maxValue = 2147483647;
}
Console.WriteLine("Maximal Value for Partion of Size {0} is 0 to {1}", k, maxValue);

// hack..
bool getValue = true;
for(uint i = 0; i < k; i++)
{
    Console.WriteLine("Input a Value");
    value = Convert.ToUInt32(Console.ReadLine());

    // more hack
    if (k == 1)
    {
        Console.WriteLine("Value at Index 0 is: {0}", value);
        getValue = false;
    }
    else
    {
        p.setValue(ref var, k, i, value);
    }
}

for(uint i = 0; i < k; i++)
{
    // hack++
    if (getValue)
    {
        value = p.getValue(var, k, i);
            Console.WriteLine("Value At Index {0} is: {1}", i, value);
    }
}

Let's fix this...

class Program
{
    const int BitsPerByte = 8, BitSize = sizeof(uint) * BitsPerByte;

    static void Main(string[] args)
    {
        Console.WriteLine("C++ Lab to C#, Practice Using Pass by Ref");

        Console.WriteLine("Valid Partition K size are: 1, 2, 4, 8, 16, 32.");
        Console.WriteLine("Enter Partition Size :");
        var size = int.Parse(Console.ReadLine());

        // using 1L (long) for computation to avoid overflow
        var maxValue = (uint)((1L <<  size) - 1);
        Console.WriteLine("A valid value for a Partion of Size {0} is between 0 and {1}.", size, maxValue);

        var data = default(uint);
        for (var i = 0; i < BitSize / size; i++)
        {
            Console.WriteLine("Input a Value :");
            var value = uint.Parse(Console.ReadLine());

            SetValue(ref data, size, i, value);
        }

        for (var i = 0; i < BitSize / size; i++)
        {
            Console.WriteLine("Value At Index {0} is {1}", i, GetValue(data, size, i));
        }
    }

    static void SetValue(ref uint data, int size, int index, uint value)
    {
        data |= value << (size * index);
    }
    static uint GetValue(uint data, int size, int index)
    {
        var mask = (uint)((1L << size) - 1);

        return data >> size * index & mask;
    }
}

EDIT: Explanation on the bit operation :

static void SetValue(ref uint data, int size, int index, uint value)
{
    /*  :: Given that size = 8 and index = 2 (3rd), value = 10000001, data = 10101010 00000000 00000000 00000000
        00000000 00000000 00000000 10000001 // value
        00000000 10000001 00000000 00000000 // value left-shifted by 16 (size * index)

        00000000 10000001 00000000 00000000 // value left-shifted by 16 (size * index)
        10101010 00000000 00000000 00000000 // data
        ----------------------------------- // OR
        10101010 10000001 00000000 00000000 // result */
    data |= value << (size * index);
}
static uint GetValue(uint data, int size, int index)
{
    var mask = (uint)((1L << size) - 1);
    /*  :: Given that size = 8 and index = 2 (3rd), value = 0b10000001, data = 10101010 10000001 00000000 00000000
        00000000 00000000 00000001 00000000 // (1L << size)
        00000000 00000000 00000000 11111111 // (1L << size) - 1 -> mask

        10101010 10000001 00000000 00000000 // data
        00000000 00000000 10101010 10000001 // data right-shifted by 16 (size * index)

        00000000 00000000 10101010 10000001 // data right-shifted by 16 (size * index)
        00000000 00000000 00000000 11111111 // mask
        ----------------------------------- // AND
        00000000 00000000 00000000 10000001 // result */
    return data >> size * index & mask;
}


Pythagorean Triple finder Would like some feedback on a code style exercise motivated by this PythagTriple algorihtm. I started with the Rosetta Code - "efficient C" solution  (not the naive one) and refactored it heavily into modern C++17.

It doesn't quite match their "Task spec". That that was not the focus. However, it could very easily be adopted to do so - just a few changes in main().Performance appears very similar to the C code I started with and the generated ASM seems reasonable.

But the main focus here is style, ease of readability, maintenance and correctness.

Specific feedback points I am looking for:

Use of the lambda to process each triple
Way of constraining the template to prevent unsuitable lambda's being passed. (C++20 concepts needed to do it cleanly?)
Use of initialisers for static data. Also is there a way to get away from the {{ .. {{ .. {},{},{} .. }} .. }}  craziness? 4 extra opening and closing braces.
Use of specific type for vec3 and a using alias for trans
Body of the transform() function. Is there a map() ... splat way?
General function signatures / use of const, constexpr, attributes etc
Use of "by value" vs "by ref" semantics for triples / tranforms etc. There are some "arguably redundant" const T&s in there which were just as fast as "by value" (probably copy elided).
More algorithmic question: The algorithm as implemented is constrained by max_perimeter and due to recursive DFS flow and the matrix transforms the triples come out in a (subjectively) "weird order". Several other constraints and orderings are conceivably possible. One idea is to implement several alternative .find_XX() methods to do that.

EDIT: There is updated code in a separate answer below, integrating the feedback.

#include <array>
#include <iostream>

namespace pythag {

class triple {
 public:
  const long _a;
  const long _b;
  const long _c;

  template <typename F>
  constexpr void find(long max_perim, F&& proc) {
    if (perimeter() > max_perim) return;
    proc(*this);
    for (auto& T : U) transform(T).find(max_perim, proc);
  }

  [[nodiscard]] constexpr long perimeter() const noexcept { return _a + _b + _c; }

  friend std::ostream& operator<<(std::ostream& stream, const triple t) noexcept {
    return stream << t._a << ", " << t._b << ", " << t._c;
  }

private:
  struct vec3 {  // a strong type to distinguish from triple
    const int _x;
    const int _y;
    const int _z;
  };

  using trans = std::array<vec3, 3>;

  [[nodiscard]] constexpr long dot(vec3 V) const noexcept {
    return V._x * _a + V._y * _b + V._z * _c;
  }

  [[nodiscard]] constexpr triple transform(const trans& T) const noexcept {
    return triple{dot(T[0]), dot(T[1]), dot(T[2])};
  }

  static constexpr auto U = std::array<trans, 3>{{
      // https://en.wikipedia.org/wiki/Pythagorean_triple#Parent.2Fchild_relationships
      // clang-format off
      {{{  1, -2,  2},     // vec3 U[0][0]
        {  2, -1,  2},     // vec3 U[0][1]
        {  2, -2,  3}}},   // vec3 U[0][1]

      {{{  1,  2,  2},     // vec3 U[1][0]
        {  2,  1,  2},     // vec3 U[1][1]
        {  2,  2,  3}}},   // vec3 U[1][2]

      {{{ -1,  2,  2},     // vec3 U[2][0]
        { -2,  1,  2},     // vec3 U[2][1]
        { -2,  2,  3}}},   // vec3 U[2][2]
      // clang-format on
  }};
};

} // namespace pythag

int main() {
  // basic usage demo
  long sum_peri = 0;
  long count = 0;

  pythag::triple{3, 4, 5}.find(
      100'000'000, // produces 7'023'027 "primitive" triples in <100ms on i7 2600
      // [](const auto& t) { std::cout << t << "\n"; });  // print option. slow obviously
      [&sum_peri, &count](const auto& t) { sum_peri += t.perimeter(); ++count; });

  std::cout << count << "\n";        // these 2 lines are just a way to prevent
  return (sum_peri ^ count) & 0xff;  // entire programme being optimised away
}

Here are some comments on your revised code, many of which apply to your original code as well.

for (auto& T: U) transform(T).find(max_perim, proc);

You're using the names T and U to refer to things that are not template type parameters. In fact, U is a reference to a static data member of the enclosing class, whose only declaration appears many lines below this use. This is extremely surprising code. I might instead write

for (auto&& formula : this->child_formulas) {
    this->transform(formula).find(max_perim, proc);
}

Notice the use of braces around the loop body; the use of auto&&, Which Always Works; the use of this-> to clarify for the human reader what scope we're expecting to find these names in; and especially the descriptive name child_formulas for U. (child_formulas says what U is; this-> says whose it is. Both of those things were in question, in the old code.)

class triple : public std::array<long, 3> {

Don't inherit from standard types. It's legal C++, but it's bad practice.

  If you (or your project, or your company) didn’t write class Foo, then class Foo should not be granted control over the API of your own class. And that’s what you’re doing when you inherit from a base class: you’re granting that class control over your API.

I like your original x, y, z data members much better.

template <typename F>
void find(long max_perim, F&& proc) {

You're passing proc by perfect-forwarding, but then when you use it, you're using it as a plain old lvalue. This is fine (in the wild-west anything-goes world of templates), but it doesn't express the meaning of proc quite as well as I'd like. proc is supposed to be a callback that gets called for each triple, right? Calling a callback doesn't modify the callback. So, we can and should require that proc be const-callable, and then we just write

template<class F>
void find(int max_perim, const F& proc) {

(Drive-by irrelevant style adjustments to save typing. long varies in size and is the same size as int on many platforms, so let's just use int until we're ready to step all the way up to a well-defined int64_t or __int128_t.)

If you really want to support non-const and/or rvalue-callable Fs, you can perfect-forward proc all over the place, but trust me, it's not worth it. (The std::forward<F>(proc)s will clutter your code, and all you're doing is enabling your users to write confusing and unintuitive client code.)

Vice versa, re your question about concepts: You can constrain this template to SFINAE away in C++17 (or C++11) like this:

template<class F, class = std::enable_if_t<std::is_invocable_v<const F&, triple&>>>
void find(int max_perim, const F& proc) {

This is not much more boilerplate than the C++2a concepts version:

template<class F> requires std::is_invocable<const F&, triple&>
void find(int max_perim, const F& proc) {

Notice that in C++2a you will also be able to write "simply"

void find(int max_perim, const std::invocable<triple&> auto& proc) {

but (A) this doesn't express exactly the same SFINAE-constraint as the other two, and (B) IMHO it looks more confusing and scary.

But should you constrain this function to SFINAE away when it's given a "bad" lambda type? IMHO, no, you shouldn't. There's no reason to SFINAE here. SFINAE is for when you need this version of find to drop out of the overload set so that some other more general version can pick up the call. That's not the situation that you have, here.

Compare the error messages you get from (A) (or the C++2a Concepts version (A2))

template<class F, class = std::enable_if_t<std::is_invocable_v<const F&, triple&>>>
void find(int max_perim, const F& proc) {
    proc(*this);
}

this->find(42, 7);

versus (B)

template<class F>
void find(int max_perim, const F& proc) {
    static_assert(std::is_invocable_v<const F&, triple&>);
    proc(*this);
}

this->find(42, 7);

versus (C)

template<class F>
void find(int max_perim, const F& proc) {
    proc(*this);
}

this->find(42, 7);

and see which version feels most "user-friendly" for the client programmer. (Remember that someone using find correctly will never see any of these error messages, so you should optimize to help the guy who doesn't know how to use it.)

  cnt_next_depth += U.size(); // always 3
  if (--cnt_this_depth == 0) {
    if (++depth > max_depth) return;
    cnt_this_depth = cnt_next_depth;
    cnt_next_depth = 0;
  }
  for (auto& T: U) q.push(t.transform(T));

Code comments are usually a red flag, at least on CodeReview. ;) It seems that this += corresponds to the three calls to q.push below; i.e., your algorithm requires that cnt_next_depth exactly track q.size(). But you can't use q.size() because you are reusing q to store both the elements at the current level and the elements at the next level.

It would make more sense to use two different queues:

std::vector<triple> this_level = ...;
std::vector<triple> next_level;
for (int i=0; i < max_depth; ++i) {
    for (auto&& t : this_level) {
        proc(t);
        for (auto&& formula : child_formulas) {
            next_level.push_back(t.transform(formula));
        }
    }
    this_level = std::move(next_level);
    next_level.clear();
}

As a bonus, it turns out that we don't even need them to be queues anymore; they can be plain old vectors, and we save a bunch of heap traffic.

friend std::ostream& operator<<(std::ostream& stream, const triple& t) noexcept {
  // Frustrating boiler plate. Any terse alternatives, that do it quickly and correctly?
  char comma[] = {'\0', ' ', '\0'}; // NOLINT
  for (auto d: t) {
    stream << comma << d;
    comma[0] = ',';
  }
  return stream;
}

No good answer. The idiomatic way would be

friend std::ostream& operator<<(std::ostream& stream, const triple& t) {
    bool first = true;
    for (auto&& d : t) {
        if (!first) stream << ", ";
        stream << d;
        first = false;
    }
    return stream;
}

Notice that this function is not noexcept, because any of these stream operations might throw. (For example, if you're writing to a stringstream and it throws bad_alloc.)

Also, in real life I'd write for (int d : t) to indicate exactly what type of "elements" I expect to be getting out of a triple. I'm using auto&& here only because I saw your comment that you want to stick with "Almost Always Auto" style.

  triple transform(const trans& T) const noexcept {
    auto res = triple{};
    std::transform(T.begin(), T.end(), res.begin(), [this](vec3 V) {
      return std::inner_product(V.begin(), V.end(), this->begin(), 0L);
    });
    return res;
  }

This is interesting use of STL algorithms, but algorithms are really meant for operating on big anonymous ranges of data elements, not for constant-size situations like we have here. In order to use STL algorithms, you've been forced to anonymize your formulas into faceless data ranges:

  static constexpr auto U = std::array<trans, 3>{{
      // https://en.wikipedia.org/wiki/Pythagorean_triple#Parent.2Fchild_relationships
      {{{{{1, -2, 2}},    // vec3 U[0][0]
         {{2, -1, 2}},    // vec3 U[0][1]
         {{2, -2, 3}}}}}, // vec3 U[0][1]

      {{{{{1, 2, 2}},    // vec3 U[1][0]
         {{2, 1, 2}},    // vec3 U[1][1]
         {{2, 2, 3}}}}}, // vec3 U[1][2]

      {{{{{-1, 2, 2}},    // vec3 U[2][0]
         {{-2, 1, 2}},    // vec3 U[2][1]
         {{-2, 2, 3}}}}}, // vec3 U[2][2]
  }};

Compare that code to a more "code-driven" version:

triple transform(const Formula& f) const noexcept {
    return f(x, y, z);
}

auto formulas[] = {
    +[](int x, int y, int z){ return triple{ x - 2*y + 2*z, 2*x - y + 2*z, 2*x - 2*y + 3*z}; },
    +[](int x, int y, int z){ return triple{ x + 2*y + 2*z, 2*x + y + 2*z, 2*x + 2*y + 3*z}; },
    +[](int x, int y, int z){ return triple{ -x + 2*y + 2*z, -2*x + y + 2*z, -2*x + 2*y + 3*z}; },
};

In this version, trans is gone already, and t.transform(f) is hardly pulling its weight.

if (perimeter() > max_perim) return;

Consider what you'd do if you wanted to find all triples up to some maximum side length, or up to some maximum hypotenuse length. Does this approach generalize to

triple.find(callback_on_each_triple, stopping_condition);

?


Weather forecast printing I receive a weather forecast for 5 days, but in 3 hours intervals.
This classes represent the weather forecast:

public class Forecast : ILocation
{
    [JsonIgnore()]
    public string City { get; set; }
    [JsonIgnore()]
    public string Country { get; set; }
    [JsonProperty("list")]
    public IList<WeatherTimePoint> TimePoints { get; set; }
}
public class WeatherTimePoint
{
    [JsonProperty("dt")]
    [JsonConverter(typeof(UnixTimeStampConverter))]
    public DateTime TimePoint { get; set; }
    [JsonProperty("main")]
    public ClimaticData ClimaticData { get; set; }
    [JsonProperty("weather")]
    public IList<Weather> Weather { get; set; }
    [JsonProperty("wind")]
    public Wind Wind { get; set; }
    [JsonIgnore()]
    public Units Units { get; set; }
}

This method prints the forecast:

 static void PrintForecast(Forecast forecast, int days)
    {
        days = days > 5 ? 5 : days;
        DateTime printDate = DateTime.Now.Date;
        for (int i = 0; i < days; i++)
        {
            printDate = printDate.AddDays(1);
            ChangeColor(ConsoleColor.Magenta);
            Console.WriteLine("== Day {0} ==", printDate.ToLongDateString());

            var timePointsDuringtDay = forecast.TimePoints.Where((weatherTimePoint) => weatherTimePoint.TimePoint.Date == printDate);
            for(int hour = 6; hour  <= 21 ; hour = hour + 3)
            {
                var weatherAtHour = timePointsDuringtDay.SingleOrDefault((weatherTimePoint) => weatherTimePoint.TimePoint.Hour == hour);
                if (weatherAtHour != null)
                {
                    ChangeColor(ConsoleColor.DarkCyan);
                    Console.Write("{0}/", hour);
                    ChangeColor(ConsoleColor.DarkGreen);
                    Console.Write("{0}({1})/",weatherAtHour.Weather[0].Condition, weatherAtHour.Weather[0].Description);
                    ChangeColor(ConsoleColor.Red);
                    Console.Write("{0}{1}",weatherAtHour.ClimaticData.Temperature, UnitsHelper.GetUnit(forecast.Units, UnitsHelper.WeatherField.Temperature));
                    Console.WriteLine();
                }
            }
            ChangeColor(ConsoleColor.White);
            Console.WriteLine("Average humidity:{0}%", timePointsDuringtDay.Average((weatherTimePoint) => weatherTimePoint.ClimaticData.Humidity));
            Console.WriteLine("Average pressure:{0} hPa", timePointsDuringtDay.Average((weatherTimePoint) => weatherTimePoint.ClimaticData.Pressure));
        }
    }

I thought it's quite unreadable. I am searching for any suggestion to improve the printing method. (ILocation is just a interface, that defines city and country properties. This is irrelevant for this method ;) )

To save a few lines create a Style enum first:

enum Style
{
    Header,
    Hour,
    Condition,
    Temperature
}

Then assign each style a color:

static Dictionary<Style, ConsoleColor> _styles = new Dictionary<Style, ConsoleColor>
{
    [Style.Header] = ConsoleColor.Magenta,
    ...
}

Use overloading to encapsulate the styling logic like this:

private static void Print(Style style, string format, params object[] args)
{
    Console.ForegroundColor = _styles[style];
    Console.Write(format, args);
    Console.ResetColor();
}

private static void PrintLine(Style style, string format, params object[] args)
{
    Print(style, format, args);
    Console.WriteLine();
}

I think it's a good idea to stick to the Print & PrintLine convention like the console does it.

Then you can turn this:

 ChangeColor(ConsoleColor.Magenta);
 Console.WriteLine("== Day {0} ==", printDate.ToLongDateString());

into:

Print(Style.Header, "== Day {0} ==", printDate.ToLongDateString());

You can go even further and create a special method called PrintHeader and do all the magic there like so:

PrintHeader(printDate)
{
    Print(Style.Header, "== Day {0} ==", printDate.ToLongDateString());
}

This way your printing becomes even cleaner and you can manipulate the header separately.

The same applies for other cases.

 DateTime printDate = DateTime.Now.Date;
 for (int i = 0; i < days; i++)
 {
     printDate = printDate.AddDays(1);
     ...
 }

I would remove the helper above the loop and write:

 for (int i = 0; i < days; i++)
 {
     var printDate = DateTime.Now.Date.AddDays(i + 1);
     ...
 }

Magic numbers

You should replace all those 5, 6, 21 etc with constants and give them proper names. I have no idea what they mean. You in few week will wonder too why such numbers.

 if (weatherAtHour != null)    {
 }

make it

if (weatherAtHour == null) { continue; }

to reduce nesting.

for (int hour = 6; hour <= 21; hour = hour + 3)

I would encapsulate this logic in a method like this:

private static IEnumerable<int> Hours()
{
    for (int hour = 6; hour <= 21; hour = hour + 3)
    {
        yield return hour;
    }
}

Then in your weather print you can use link with ease:

var weathersAtHour =
    Hours()
    .Select(hour => timePointsDuringtDay.SingleOrDefault(weatherTimePoint => weatherTimePoint.TimePoint.Hour == hour))
    .Where(x => x != null);

foreach (var weatherAtHour in weathersAtHour)
{
   ...
}


Lazy load class variables Following my java class is required to be reviewed and couple of clarifications about class variable loading is required. Following is my class.

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class ValidateUtil{

    private static Pattern pattern;
    private static final String REFFERENCE_PATTERN = "The regex pattern here";

    public static boolean validate(final String reference){
    if(pattern ==null){
      pattern = Pattern.compile(REFFERENCE_PATTERN ); //Line 10
    }

      Matcher  matcher = pattern.matcher(reference);
      return matcher.matches();
    }

     /* Many util Methods here
      *
      */

}

There is only one method shown above, but there are many static utility methods are in this class. Based on the requirement, those methods are invoked.
In above class, variable pattern is declared, but not initialized. Instead, it is initialized inside the method validate(). The purpose of initializing so is to avoid initializing unnecessarily and initialize variables or create objects only if they are required and they are going to be used. That means, if it is defined

private static Pattern pattern = Pattern.compile(REFFERENCE_PATTERN );

as a class variable (not initialize inside the method), there will be a pattern object created whether the validate() method is used or not.  Actually, this is only one sample, there can be many situations like variables for supporting Object or array of Objects.

My question is that, is doing so is actually worth in a code ? This may lead to increase the line of codes and sometime, the performance gain can be neglectable (For our own custom objects). On the other hand, is this really worth only for heavy, large objects (like Database connections)?

First I want to suggest a concrete implementation:

public class ValidateUtil{

    private static Pattern pattern;
    private static final String REFFERENCE_PATTERN = "The regex pattern here";

    public static boolean validate(final String reference){
        return getPattern().matcher(reference).matches();
    }

    public static Pattern getPattern() {
        if (pattern == null) {
            pattern = Pattern.compile(REFFERENCE_PATTERN);
        }
        return pattern;
    }

    /*
     * Many util Methods here
     */

}

  This may lead to increase the line of codes

Lines of code is at the most an indicator of less code quality. Do not ask for lines, do ask for responsibilities. Hypothetically a method/a class can have thousands lines of code and it will not produce any problem if it has only one responsibility. But I'll give you that it becomes more and more probable to violate the SRP (single responsible principle) the more code is available in the scope. And of course you may have other problems can be derived from many lines of code available. But code of lines alone will say you less to nothing.

  and sometime, the performance gain can be neglectable

And here you got a point. In one of my projects I discussed using "synchronized" and the performance issues. We measured a relation about 1:20. So of course it MUST be an issue... We made a second measurement. We iterated over a synchronized method 20.000.000 times and with the available pc configuration it took 20 seconds. BTW, the methods we talk about were called in maximum 20 times a second. What I want to say is: It really depends on the use case. But in MOST use cases you do not need not care about preemptive optimization. And you have to look at both: relative and absolute metrics.

  On the other hand, is this really worth only for heavy, large objects (like Database connections)

A very clear answer in THIS case of LAZY INITIALIZATION: No.

It may be a case of preemptive optimization. But that is not the reason I would prefer it. The reason why I would do so is: If you only use lazy getter-methods within or from outside your object/class you make yourself independent of the order of object creation. You use the getter and you get an object, doesn't matter if it was created before or it was created right now. This is especially useful in component trees. You often play around with the position of an ui element in the ui component tree. A lazy getter saves your ass and time. After all ui components often are heavy and even if they are not you get the optimization for free.


Basic API wrapper around a Restful service - follow-up This is a follow up from my previous post: Basic API wrapper around a Restful service

I'm writing a basic wrapper around a restful service that returns a list of Stores. Naturally, I want to make it completely testable (that's a major requirement!).

I have split out my TopmanClient class so that just deals with the rest client bits (using RestSharp).

I then inject the TopmanClient class into my TopmanRepository:

public class TopmanClient : ITopmanClient
{
    private readonly IRestClient _restClient;
    private const string BaseUrl = "https://public.Topman.com/";
    private const string AcceptTenant = "uk";
    private const string AcceptLanguage = "en-GB";

    public TopmanClient()
    {
        _restClient = new RestClient(BaseUrl);

        _restClient.AddDefaultHeader("Accept-Tenant", AcceptTenant);
        _restClient.AddDefaultHeader("Accept-Language", AcceptLanguage);
    }

    public IRestRequest Request(string url)
    {
        return new RestRequest(url);
    }

    public IRestResponse<T> Execute<T>(IRestRequest request) where T : new()
    {
        return _restClient.Execute<T>(request);
    }
}

public class TopmanRepository : ITopmanRepository<Store>
{
    private readonly ITopmanClient _topmanClient;
    public TopmanRepository(ITopmanClient topmanClient)
    {
        _topmanClient = topmanClient;
    }

    public List<Store> Get(string query)
    {
        var request = _topmanClient.Request("stores");

        request.RootElement = "Stores";
        request.AddQueryParameter("q", query);

        var response = _topmanClient.Execute<List<Store>>(request);

        return response.Data;
    }
}

As @t3chb0t says, the implementation is clean enough already.

However, if you wanted to go one step further, I would inject the implementation of IRestSharp into the TopmanClient, so that you can then test the behaviour of your default implementation when an API error occurs. For instance, if the API returns a 404 Not Found, how will your client behave? How about if a deserialisation error occurs?

For double bonus points, you could run the test once passing in a mock IRestClient; then run the same test again passing in a real RestClient - you now have a test of your client and an integration test for the API (although make sure you are pointing at a non-production API, or a test account, or a sandbox). Whether or not you include those in your automated builds or automated deployments, I'll leave up to you.


Calculating time deltas between rows in a Pandas dataframe I am trying to compute the difference in timestamps and make a delta time column in a Pandas dataframe. This is the code I am currently using:

# Make x sequential in time
x.sort_values('timeseries',ascending=False)
x.reset_index(drop=True)

# Initialize a list to store the delta values
time_delta = [pd._libs.tslib.Timedelta('NaT')]

# Loop though the table and compute deltas
for i in range(1,len(x)):
    time_delta.append(x.loc[i,'timestamp'] - x.loc[i-1,'timestamp'])

# Compute a Pandas Series from the list
time_delta = pd.Series(time_delta)

# Attach the Series back to the original df
x['time_delta'] = time_delta

It seems like there should be a more efficient / vectorized way of doing this simple operation, but I can't seem to figure it out.

Suggestions on improving this code would be greatly appreciated.

Use the diff().

    x['time_delta'] = x.timestamp.diff().fillna(x['time_delta'])

This works as below, in a simpler example.

You could use the diff() Series method (with fillna to replace the first value in the series):

s = pd.Series([11, 13, 56, 60, 65])
s.diff().fillna(s)
0    11
1     2
2    43
3     4
4     5
dtype: float64

This was compiled from the comments below the current best answer (which I failed to see and kept searching), and the stack overflow link that explained it with fillna so I am hoping this can be lifted up to the top for future seekers. Happy data processing!


Activeresource-response gem I created my first gem for Rails today. https://github.com/Fivell/activeresource-response.
This gem adds possibility to access http response object from result of activeresource call.

I don't know how to create test for this gem.
Also I want to ask if there is more simply and elegant way to do this?
Is it really thread-safe?
Other ways to write this functionllity except alias_method?

Here is source of library.

require "activeresource-response/version"
module ActiveresourceResponse
   module Connection
     def self.included(base)
       base.class_eval  <<-EOS
           alias_method :origin_handle_response, :handle_response
           attr_reader :http_response
           def handle_response(response)
             @http_response= response
             origin_handle_response(response)
           end
        EOS
     end
   end
   module AddResponseMethod
     def self.included(base)
        base.extend ClassMethods
      end
      module ClassMethods
         def add_response_method(method_name = 'http_response')
             class_eval  <<-EOS
               class << self
                 alias_method :origin_find, :find
                 def find(*arguments)
                     result = origin_find(*arguments)
                     result.class_eval("attr_reader :#{method_name}")
                     result.instance_variable_set(:"@#{method_name}", connection.http_response)
                     result
                 end
                end
             EOS
         end
      end
   end
end
ActiveResource::Connection.send :include, ActiveresourceResponse::Connection
ActiveResource::Base.send :include, ActiveresourceResponse::AddResponseMethod

UPDATE

I made some refactoring, please look at github sources.

First thing, it unnecessary to use the included hook methods. Since your purpose is to redefine some methods in ActiveresourceResponse::Connection and Activeresource::Base, why not just do so directly?

# if ActiveResource has not been loaded yet, Rails' const_missing hook
#   will load it automatically
ActiveResource::Connection.class_eval do
  alias_method :__handle_response__ :handle_response
  # ... and so on
end
ActiveResource::Base.instance_eval do
  alias_method :__find__ :find
  # ... and so on
end

By using instance_eval in the second case, we can redefine class methods with 1 less line of code, because class << self is unnecessary.

In add_response_method, it is unnecessary to pass 'http_response' as an argument. That code will never be run with any other value, so why use an argument? Just use a literal value, it's clearer and easier to read.

Every time a model object is returned from ActiveResource::Base.find, you define the http_response method on it with class_eval. This is very inefficient -- I think you should just define a http_response instance method on ActiveResource::Base, and all model classes derived from ActiveResource::Base will inherit it. (Think of the work which is done to define a method -- the Ruby parser has to run on the code, it has to be compiled to bytecode, then an entry has to be made in a method table...)

I just benchmarked Object#instance_variable_set, and it's very fast, faster than using instance_eval or instance_exec. So I think that should stay, but don't generate a new symbol using :"#{}" each time. Just use something like :@__http_response__. (Why the "special" name? Because when you are adding instance variables to a user-defined class, you want to reduce the chances of a collision with a user-defined variable.) Of course, this will mean you can't use attr_reader, but that's no problem. You can just do something like: def http_response; @__http_response__; end

For tests, why don't you look at how ActiveResource itself is tested? I'm sure all that code is open-source and should be available from a public repo.

EDIT: I just looked at the GitHub repo and noticed that you changed to a thread-local variable to try to make the code thread-safe. My question is: is a single ActiveResource::Connection object shared by the whole application? Or each time you make a request, does it create a new Connection object? If it creates a new Connection object for each request, then using thread-local variables is unnecessary, and an instance variable would be better. If the whole application is sharing only one Connection object, then using the thread-local variable is a good idea.

EDIT 2: I just found a bug. If a user tries to use add_response_method to add an alias for http_response, subsequent calls to find will go into an infinite loop. Try it and you will see.


Cleaning up Compiler class What is the best way to considerably decrease the size of the following code without sacrificing functionality, and to make it more readable, and well, make it look professional?

class Compiler
{
    public string Compile(TextReader src)
    {
        string rtn = SourceTemplate.Top;

        var preprocessor = new PreProcessor();

        Parser[] ParserRounds = {
                                    new PredefinedConstantParser(),
                                    new VariableDeclarationParser(),
                                    new ConditionalExpressionParser(),
                                    new FunctionCallParser()//,
                                  //new FunctionParser
                                };

        foreach (var l in preprocessor.Process(src))
        {
            var line = l;

            foreach (var parser in ParserRounds)
            {
                line = parser.Parse(line);
            }

            rtn += "\t\t\t" + line + "\n";
        }

        return rtn + SourceTemplate.Bottom;
    }

    public static void Start(string[] args)
    {
        TextReader src = File.OpenText(args[0]);

        var com = new Compiler();
        var source = com.Compile(src);

        string target = args[0].TrimEnd(".hack".ToCharArray());
        target += ".cs";
    }

    public void SaveToFile(string source, string target)
    {
        TextWriter wsrc = File.CreateText(target);
        wsrc.WriteLine(source);
        wsrc.Close();
    }
}

In my personal opinion, the above code looks like a mess at this stage. Also, please suggest good and expressive variable names instead of rtn , src etc.  Should I also make this class generic?

Just some generic notes since I'm not too familiar with C#:

rtn should be StringBuilder
I'd change the preprocessor and ParserRounds variables to fields and pass their values to the constructor of the class. It makes testing easier (you can test only one parser at a time, you can test with a mocked preprocessor etc.) and it results looser coupling.
Using SourceTemplate.Top and SourceTemplate.Bottom also tight coupling, pass these values to the constructor too, store them in a field and use these fields in the Compile method. (Maybe it's worth creating a SourceTemplate interface with GetTop() and GetBottom() methods and passing a proper instance to the Compiler class.)
I'm really missing the input validation. For example, if the Compile method get a null as a value of TextReader you should throw an exception immediately. It helps debugging a lot. Why would you do any compiling if it will throw an exception later since the TextReader is null?

The Start method also should check the input values.
Some variable naming ideas:

src -> sourceReader or srcReader
com -> compiler
target -> targetFileName
rtn -> result
wsrc -> targetWriter


Artificial Stock Market Algorithm I'm attempting to make a stock trading game in Javascript and I've been trying to find an algorithm to generate fake stock prices. Currently, each Stock object has a minimum value, maximum value, and volatility which I will explain later. The code is as follows:

Stock.prototype.generateValue = function () {
    //mod = probability that the value will decrease next turn
    var mod = 0.5;
    if (this.value > this.max) mod = 0.8;
    if (this.value < this.min) mod = 0.2;
    var dir = (Math.random() >= mod) ? 1 : -1;

    //maxToAdd = max amount that the value can change
    var maxToAdd = (this.max - this.min) * this.volatility / 100;
    this.value += (Math.random() * maxToAdd * dir);

    //value can't be 0 -- random number from 0 to 10
    if (this.value < 0) this.value = Math.random()*10;

    //rounds to 2 decimal places
    this.value = (Math.round(100*this.value))/100;
    this.pastValues[this.pastValues.length] = this.value;
    return this.value;
};

Right now, if the current value is within the allowed range (minimum to maximum), then it has a 50% chance of going up and a 50% chance of going down. If it is out of said range, then it has an 80% chance of heading back towards the range and a 20% chance of heading farther out.

Volatility is how much the stock changes on average. Each turn, it changes by a random number between \$0\$ and \$\frac{volatility}{100} \cdot{range}\$ - that is, the volatility is the percentage of the entire range that it can change each turn (if anyone understands this and can explain it better, I would appreciate it).

The results so far have been pretty believable, but if there's any better algorithm that can be found it would be helpful. I would like something that isn't so, well, random. My algorithm stays between the range but there's not a ton of room for something unexpected to happen.

Some screenshots of results (the second is a new chart, but zoomed in on the lower values):

This question might be a better fit for another board, since you're asking for different algorithms. CodeReview is more for reviewing your current implementation. As 200_success mentioned, Quantitative Finance can perhaps give you some ideas for what to model, Game Development might have some ideas for "steering" randomness, and of course StackOverflow, if you're unsure of how to implement a different algorithm.

Just know that since it's hideously complicated to make a stock market game that doesn't feel like a total dice roll (and practically impossible to make a truly accurate simulation), most games just use the real stock market but fake money. That way, players have all the right sources of information to help decide their trades, which is the really hard thing to simulate. Will you simulate press releases? Earnings calls? Analyst/stock pundit predictions? The local and global economy? Fraud? Savage rabbits attacking Foxconn's factory and disrupting production for practically every electronics manufacturer in the world? Yeah... it's tough.

And of course, there's the thing that a stock price actually reflects: Trading activity. All the news in the world won't move a stock's price if no one is buying or selling. But it also means that the market is a feedback system, so your players' trades may impact prices (if the trades are large enough). So again: It's hideously complicated.

The best I can offer here is a review of your current code.

Your original question's code was very, very dense. You've since added some comments and whitespace, which was otherwise the first thing I'd suggest.
Your use of randomness introduces a potential problem: The stock rising or falling forever. Improbable, yes, but possible. You guard against the stock going negative, but still. Your guard mechanism is also a bit blunt. Theoretically a stock could trade below $1 for a while, and then suddenly - because it was about to go negative - it's at $10 regardless of its volatility or range. I'd probably invest in some of those penny stocks, on the off-chance that that happens. You should probably have a mechanism for delisting a stock instead, if it falls too low.
With your current solution, there's always at least a 20% chance that the market goes your way. Having 1-in-5 odds isn't great, but in real life there are much worse odds to be had (like betting on a company that's suddenly gone into bankruptcy after an accounting scandal and its CEO being eaten by wombats or something). It just feels like the 20/50/80 odds are a bit too coarse.
Having both a min/max price and a "volatility factor" might be redundant. Setting the min/max close to the current price should reduce volatility, and setting them further away should increase volatility. Or you might only use volatility, and let the stock do what it wants with no special min/max.

In terms of code, things look ok, except that this line

this.pastValues[this.pastValues.length] = this.value;

could just be

this.pastValues.push(this.value);

I'd also consider extracting the round-to-two-decimals code, since I'm sure you'll find it useful elsewhere.

As for changes, I'd probably change the price by a percentage instead. That way, it can't fall below zero:

Stock.prototype.generateValue = function () {
  var minPercentage = this.min / this.value,
      maxPercentage = this.max / this.value,
      range = maxPercentage - minPercentage,
      change = Math.random() * range + minPercentage;

  this.value = Math.round(this.value * change * 100) / 100;

  this.pastValues.push(this.value);

  return this.value;
};

Of course, you'll want to continually update the min/max range to "steer" the price one way or the other (just make sure that max >= min >= 0).

Regardless, how you want to do all this really depends on how the game part plays.


Higher-order FizzBuzz in Clojure Help me make it better.  Requirement: support arbitrary number of factors, not just 3 & 5.

(ns fizzbuzz.core
  (:use [clojure.contrib.string :only (join)]))

(def targets (sorted-map 3 "fizz" 5 "buzz" 7 "baz"))

;; match any applicable factors
(defn match [n, factors]
  (filter #(= 0 (mod n %)) factors))

(defn fizzbuzz [n]
  (let [factors (keys targets)
        matches (match n factors)]
    (if (= 0 (count matches))
      (str n)
      (reduce str (map targets matches)))))

(join ", " (map fizzbuzz (range 1 20)))

I tried keeping your approach, although I would've made it an infinite lazy sequence. Yours is probably a better idea, since you still have the infinite sequence (mapping to range as you did) and you can map it to other non-linear sequences too.

(def targets  ;; This is more readable for me
  (sorted-map
    3 "fizz",
    5 "buzz",
    7 "baz"))

(defn fizzbuzz [targets n]  ;; Pass the targets here! Commas aren't very common in args lists
  (let [matches (keep (fn [[f w]]  ;; Destructuring is nice :P
                        (when (zero? (mod n f)) w))  ;; Got rid of your helper function
                      targets)]  ;; ^ Use 'zero?' instead of '(= 0 ...)'
    (if (empty? matches)  ;; Use 'empty?' instead of '(= 0 (count ...))'
      (str n)
      (apply str matches))))  ;; I prefer apply here, but seen both

;;;;; EXAMPLE
(map (partial fizzbuzz targets) (range 1 20))
   => ("1" "2" "fizz" "4" "buzz" "fizz" "baz" "8" "fizz" "buzz" "11" "fizz" "13" "baz" "fizzbuzz" "16" "17" "fizz" "19" "buzz")

There's an even more idiomatic way to rewrite the (if (empty? matches)...) bit:

(if matches
  (apply str matches)
  (str n))

Lazy sequence style. Consider fizzbuzz and targets are already defined:

(defn lazy-fizzbuzz
  ([targets n]
    (lazy-seq
      (cons (fizzbuzz targets n)
            (lazy-fizzbuzz targets (inc n)))))
  ([targets]
    (lazy-fizzbuzz targets 1)))

;;;;; EXAMPLES
(take 20 (lazy-fizzbuzz targets))
   => ("1" "2" "fizz" "4" "buzz" "fizz" "baz" "8" "fizz" "buzz" "11" "fizz" "13" "baz" "fizzbuzz" "16" "17" "fizz" "19" "buzz")

;; You can also specify an initial n
(take 20 (lazy-fizzbuzz targets 20))
   => ("buzz" "fizzbaz" "22" "23" "fizz" "buzz" "26" "fizz" "baz" "29" "fizzbuzz" "31" "32" "fizz" "34" "buzzbaz" "fizz" "37" "38" "fizz")

;; which is the same as...
(take 20 (drop 19 (lazy-fizzbuzz targets)))
   => ("buzz" "fizzbaz" "22" "23" "fizz" "buzz" "26" "fizz" "baz" "29" "fizzbuzz" "31" "32" "fizz" "34" "buzzbaz" "fizz" "37" "38" "fizz")


I can has(kell) cheezburger? Edit 13 June:
I have made a new and improved version here.
Original Question
This is a very rudimentary lolcats translator (it only works on the phrase "Can I have a cheeseburger") in Haskell. This is my first ever attempt at Haskell (or any functional programming language). I am sure that my code is absolutely atrocious (the documentation was dismal) but it works.
Teh Codez
import Data.Strings
main = do
    putStrLn "Enter your text to translate"
    inputText <- getLine
    let capsText = strToUpper inputText
    let a = strReplace "HAVE" "HAS" capsText
    let b = strReplace "CAN I" "I CAN" a
    let c = strReplace "CHEESEBURGER" "CHEEZBURGER" b
    let d = strReplace " A " " " c
    putStrLn (d)
    getLine

It is good practice in Haskell to separate the functional code from the IO. In this case, you could (and therefore should) define a lolcat :: String -> String function.  Be sure to put a type declaration on all functions — you didn't write one for your main.

Defining variables a, b, c, and d is overkill. I would write this as a composition of functions.

lolcat :: String -> String
lolcat = strReplace " A " " " .
         strReplace "CHEESEBURGER" "CHEEZBURGER" .
         strReplace "CAN I" "I CAN" .
         strReplace "HAVE" "HAS" .
         strToUpper


Making the same amount from different combinations of coins (top-down approach) I was reading a typical interview question found here. Given an amount and a list of denominations, count the number of possible ways to break the amount into different combinations of coins. For example, if amount = 4 and denominations=[1, 2, 3], there are 4 possibilities. Here's my Python solution.

def _get_num_of_changes(amount, denominations, i=0):
    if amount == 0:
        return 1
    elif amount < 0:
        return None
    else:
        s = 0
        for i in range(i, len(denominations)):
            coin = denominations[i]
            if amount - coin < coin:
                c = _get_num_of_changes(amount - coin, denominations, i+1)
            else:
                c = _get_num_of_changes(amount - coin, denominations, i)

            if c:
                s += c
        return s

def get_num_of_changes(amount, denominations):
    return _get_num_of_changes(amount, denominations)

print(get_num_of_changes(4, [1, 2, 3]))

This is a top-down solution in the sense that I subtract from the starting amount and not try to add coins starting at zero to get to amount (which would be a bottom-up solution). Given this difference, I am really interested to see if my solution is good enough, too.

Simpler base case

I suggest returning 0 as the base case to avoid the if c check di as x += 0 does not change it.

Iterate at a higher level

Use for item in collection not range(len. The index can be obtained with enumerate.

Avoid repetition

You repeat _get_num_of_changes(amount - coin, denominations, twice, use a ternary conditional expression to avoid it.

Avoid manual summing

Starting at \$0\$ and writing += is too manual in Python, I suggest using sum.


Checking battery voltage with ADC and interrupts - which way is best? Here are some stripped down versions of the two different programs I am deciding between.

In version one, my state machine is called within the interrupt:

void TimerInterrupt(void)
{
    int battery_voltage;

    StartADC();

    while(ADCrunning){}

    battery_voltage = ADC_value * scale_factor;

    BatteryStateMachine(battery_voltage);
}

void main (void)
{
    StartTimer();

    while(1){}
}

In version two, the interrupt populates a global variable, and my main program has a while loop wherein a function is called whenever that global variable changes:

int battery_voltage;

void TimerInterrupt(void)
{
    StartADC();

    while(ADCrunning){}

    battery_voltage = ADC_value * scale_factor;
}

void main (void)
{
    int old_battery_voltage;

    StartADC();

    while(ADCrunning){}

    old_battery_voltage = ADC_value * scale_factor;

    StartTimer();

    while(1){
        if (battery_voltage == old_battery_voltage){}
        else {
            BatteryStateMachine(battery_voltage);
        }
    }
}

What are the questions I need to ask in order to determine which of these is better for my requirements? Or is one of these methods already clearly superior to the other, and I just don't know it?

First rule when using microcontrollers is:

Do as little as you can in the interrupt function!

Explanation:
Consider having 10 or 100 interrupts configured. Each interrupt function will take 100 or 200 ms to deal with. (Sounds quite low, but when you have a bigger project with a big controller this might be possible)

Then 4 or 5 interrupts will arise and you have to configure the priorities all with priorities that are (maybe) included in microcontroller hardware.

If you don't have this hardware priorities (I this the small Microchip's don't have these?) you can't handle priorities.

Second thing: I don't know which microcontroller you use, but I think it has a flag that shows if the ADC is finished. Use this for ADCrunning (you don't show where you define it).

So my approach would be:

 volatile bool startADC;   // edit: you can use a struct, if there will be too many flags

void TimerInterrupt(void)
{
    startADC = true;
}

void main (void)
{

    int battery_voltage;
    int old_battery_voltage;

    battery_voltage = 0;
    old_battery_voltage = -1;

    StartTimer();

    while(1){
        if (startADC = true) {
            StartADC();
            startADC = false;
        }

        // use the flag as mentioned above
        if (ADCFinished = true) {
            // you might think of using the scale factor
            battery_voltage = ADC_value * scale_factor;
            ADCFinished = false;
        }

        // use unequal instead of the "== else"-construct
        if (battery_voltage != old_battery_voltage) {
            BatteryStateMachine(battery_voltage);
            old_battery_voltage = battery_voltage; // you need this here?
        }
    }
}


Project Euler 67 - Max path through triangle This is a response to this brute-force version where the algorithm was not intended for review.

In this challenge you are prompted to find the path through a large triangular set of numers, similar to:

     75
    13 92
   43 77 12
  11 55 86 7
 ....

The goal is to find a path from the top to the bottom while summing the values as you go, where the sum of the items on the path must be the maximum sum possible. Using the top-4 lines in the example above, the path would be 75 + 92 + 77 + 86. The goal is to report the sum of the path, or 330

I decided to use some Java 8-based idioms to parse and process the triangle. This is where I hope to get some focus for the review as well, whether there are better Java-8 ways to do this.

To parse the data in to a 2-dimensional int array:

private static int[][] getTriangle(Path source) throws IOException {
    try(Stream<String> lines = Files.lines(source)) {
        return lines.map(String::trim)
                .filter(line -> !line.isEmpty())
                .map(level -> parseLevel(level))
                .toArray(sz -> new int[sz][]);
    }
}

private static int[] parseLevel(String level) {
    return Stream.of(level.split("\\s+"))
                 .mapToInt(Integer::parseInt)
                 .toArray();
}

Then, to compute the maximum path:

private static int maxPath(final int[][] triangle) {

    // start at the bottom, and work to the top.
    // maintain a 'current' array which is the maximum value
    // possible at the current line for the specified values.
    // swap that with the 'previous' line when a line is complete.

    int[] previous = new int[triangle.length + 1];
    int[] current = new int[previous.length];

    for (int row = triangle.length - 1; row >= 0; row--) {
        for (int col = 0; col <= row; col++) {
            current[col] = Math.max(previous[col], previous[col + 1]) + triangle[row][col];
        }
        int[] tmp = previous;
        previous = current;
        current = tmp;
    }

    // the first value of the top row is the maximum path sum.
    return previous[0];
}

When I run with the following main method:

public static void main(String[] args) throws IOException {
    for (String p : args) {
        Path path = Paths.get(p);
        int[][] triangle = getTriangle(path);
        long nanos = System.nanoTime();
        int maxpath = maxPath(triangle);
        nanos = System.nanoTime() - nanos;
        System.out.printf("Maxpath for %s is %d (in %.3fms)\n", p, maxpath, nanos / 1000000.0);
    }
}

I get the results for #18 in 0.030ms and for #67 in 0.203ms on my computer.

I guess, you haven't got an answer until now, as there's hardly anything to say. So I must be very critical in order to get a non-empty answer.

getTriangle

Despite Java 8, it's nicely readable. I'm not sure about

            .filter(line -> !line.isEmpty())

as it may hide errors like when someone decides to put multiple input triangles into one file. We know, it won't happen, but still. All I'd do is to drop leading and trailing blank lines, so that such a problem gets immediately detected. However, I can't see a simple way how to achieve this.

The method is called getTriangle, but makes no checks about really reading a triangle. That's mostly fine as it'll blow later anyway, but I'd call it maybe getSourceAsInts in order not to suggest that the result must be a triangle.

maxPath

It ideally should start by checking that the input is indeed a triangle. Otherwise, it may be hard to find the cause of the exception thrown later.
    int[] previous = new int[triangle.length + 1];
    int[] current = new int[previous.length];

Given that the triangle already takes O(n*n), I wouldn't bother with saving memory here (speed-wise it hardly matters). When optimizing hard, I'd consider to overwrite the original triangle. That said, your choice is surely not bad.

You could also use long in order to prevent overflow (Not needed, I know).

// the first value of the top row is the maximum path sum.
return previous[0];

It's a bit confusing as when the top element is previous, what's then current? Maybe using the name combo current and next would be clearer (and maybe not).

main

Nothing to say.

  I get the results for #18 in 0.030ms and for #67 in 0.203ms on my computer.

For #67 it's about the same time as mine, with #18 I'm much slower as my solution uses my other utilities which leads to loading many classes. But I wouldn't care about such small times at all.

(from a comment)

  I don't believe it will be. Even Dijkstra has to visit every node once

Agreed. Your solution is optimized for the given input graph shape, much simpler and faster than general Dijkstra.


Basic TCP server client application I've written a basic server/client application to use in an automation application written in C#. The code is working pretty good, but I have a few thing I want to improve:

Server:

   public void socketListener()
{

    peerListener = new TcpListener(IPAddress.Any, ControlLayer.control_GlobalParam.TCP_PORT);
    peerListener.Start();
    int MessageLength = 0;
    //_infrastructure_TcpServerAndClient.PeerListener_start();
    Socket socket = null;
    while (true)
    {
        if (peerListener.Pending())
        {
            Thread.Sleep(500);
            continue;
        }
        else
        {
            byte[] StreamMessage = new byte[9632*2];
            try
            {
            socket = peerListener.AcceptSocket();

            Thread.Sleep(500);

                MessageLength = socket.Receive(StreamMessage, 0, StreamMessage.Length, SocketFlags.None);
            }
            catch (Exception ex)
            {
                //remote disconnected garcefully ?
                Console.WriteLine(ex);

            }
            if (MessageLength > 0 )
            {
                string message = System.Text.Encoding.Default.GetString(StreamMessage);

                if (Domain_GlobalParam.IsCollectTcpServerOutput)
                {
                    onDataRecieved(this, "Collecting log from Remote station", "TcpServer");
                }
                else
                {
                    onDataRecieved(this, message, "TcpServer");
                }
                new Thread(() => ParseMessage(message)).Start();

                //ParseMessage(message);
            }
        }
    }
}

Client:

        internal void CreateClient(object message)
    {
        try
        {

            _infrastructure_TcpServerAndClient.PeerClient_Connect();
            _infrastructure_TcpServerAndClient.SendMessageViaStreamWriter(message);
            _infrastructure_TcpServerAndClient.CloseConnection();

        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.ToString());
        }
    }
    internal void PeerClient_Connect()
{
    peerClient = new TcpClient();
    peerClient.Connect(ComSettings.IpAddress, ComSettings.Port);

}
internal void SendMessageViaStreamWriter(object message)
{
    string MessageOut = (string)message + "\n";
    netStream = peerClient.GetStream();
    StreamWriter sw = new StreamWriter(netStream);
    sw.Write(MessageOut);
    sw.Flush();
}
internal void CloseConnection()
{
    netStream.Close();
    peerClient.Close();
}

I've noticed that from time to time I miss out some messages (especially long ones). What do you think I can improve here?

dead code should be removed
the indention is horrible

    byte[] StreamMessage = new byte[9632*2];
    try
    {
    socket = peerListener.AcceptSocket();

    Thread.Sleep(500);

        MessageLength = socket.Receive(StreamMessage, 0, StreamMessage.Length, SocketFlags.None);
    }

should be look like

    byte[] StreamMessage = new byte[9632*2];
    try
    {
        socket = peerListener.AcceptSocket();

        Thread.Sleep(500);

        MessageLength = socket.Receive(StreamMessage, 0, StreamMessage.Length, SocketFlags.None);
    }

you should enclose IDisposable's like Stream, StreamWriter in using blocks.

internal void SendMessageViaStreamWriter(object message)
{
    string messageOut = (string)message + "\n";
    using (StreamWriter sw = new StreamWriter(peerClient.GetStream()))
    {
        sw.Write(messageOut);
        sw.Flush();
    }
}

The use of the using statement ensures that the Dispose() method is called in any case (like an exception has happened) this also ensures that the stream is closed.
methods should be named using PascalCase casing and should be made out of verbs or verb phrases. See: Naming Guidelines
local variables should be named using camelCase casing.
the Thread.Sleep(500); does make no sense after accepting the socket. Why do you want the thread to sleep ?
SendMessageViaStreamWriter is badly named. SendMessage would be much better. What would happen if you decide to change the implementation of the method but forget to change the name, ? Mr.Maintainer would be confused.


Optimized solution for a rolling dice code puzzle I was asked to provide the solution of this rolling dice problem in an interview. When I showed him my solution, he said that there is another way to find the solution. I am looking for answer in PHP script only.

Question:
Two persons are playing a game of rolling 2 dices. Each person rolls the two dices n times and records the outcomes (sum of value of two dices) of all the n attempts. So after n attempts of both the player we have two list corresponding to the n outcomes of two players.
They want to know that whether they have got all possible outcomes( 1 to 12) same number of times or not. If they got all the possible outcomes same number of times then they are called lucky otherwise unlucky.
Input: Two Integer Arrays (L1, L2) corresponding to outcomes of two players.
Output: Lucky or Unlucky depending on the case

My Answer:
<?php
function rollingdice($input1,$input2)
{
  foreach($input1 as $k=>$a)
  {
    if(in_array($a,$input2))
    {$p = array_search($a, $input2);
     unset($input2[$p]);
    }
    else
    { return 'Unlucky';}
  }
return 'Lucky';
}
?>

Based on your use of in_array(), you don't appear to be concerned with the order of the elements. In that case, you can simply sort the arrays and compare them directly.

function compare($array1, $array2)
{
    sort($array1);
    sort($array2);

    return ($array1 == $array2) ? "Lucky" : "Unlucky";
}


C# lambda expressions as array extensions in JavaScript I use a lot of lambda expressions in C#, so I've decided to create some similar functionality in JavaScript. Since jQuery is included in my projects, I'm using $.grep() instead of Array.prototype.filter() to support older browsers without including shims.

The two examples below corresponds to Where() and FirstOrDefault() in C#:

if(!Array.prototype.where){

    Array.prototype.where = function (func) {

        var that = this;

        switch (typeof func) {

            case 'function':
                return $.grep(that, func);

            case 'object':

                for(var prop in func){
                    that = $.grep(that, function(item){
                        return item[prop] === func[prop];
                    });
                }

                return that;

            default:
                throw new ReferenceError("no/wrong func type passed");
        }
    };
}

if(!Array.prototype.firstOrDefault){

    Array.prototype.firstOrDefault = function(func){
        return this.where(func)[0] || null;
    };
}

var persons = [{ name: 'foo', age: 1 }, { name: 'bar', age: 2 }];

var where1 = persons.where(function(p){
    return p.age === 1 && p.name === 'foo';
});

console.log(where1);

var where2 = persons.where({ age: 1, name: 'foo' });

console.log(where2);

var fod1 = persons.firstOrDefault(function(p){
    return p.age === 1 && p.name === 'foo';
});

console.log(fod1);

var fod2 = persons.firstOrDefault({ age: 1, name: 'foo' });

console.log(fod2);

Fiddle

I would have preferred to add the methods as non-enumerable using Object.defineProperty, but since I need to support IE >= 7, that's not an option.

However, the only downside with having the methods enumerable would be if someone is iterating an array using for..in, which they shouldn't do in the fist place.

In case of an object being passed as argument, one way to decrease the total iterations from array length * object property count to array length * 1 could be solved using the function constructor:

var o = { foo: 1, bar: true, s: 's' };

var str = Object.keys(o).reduce(function (prev, key) {

    if(typeof o[key] === 'string')
        o[key] = '\'' + o[key] + '\'';
    return (prev ? prev + ' && ' : '') + 'x.' + key + ' === ' + o[key];

}, '');

var f = new Function('x', 'return ' + str + ';');

// pass f to filter/$.grep
console.log(f);

Fiddle

I would like some feedback about possible performance improvements and what you think about the code in general (but not pros and cons with extending native objects).

Looks OK to me. I don't know that you could do anything to really squeeze more performance out of this. In the end you're mostly dependent on how $.grep performs.

I set up a jsperf test for your own alternatives and the code below, and constructing a function is by far the fastest (I put the function construction into where, and avoided using reduce since you said you'd need IE7+ support).

But! Constructing a function from text is just tremendously fragile:

Quoting a string is a pain. If the string contains single or double quotes, you can't properly quote it without a lot of parsing and escaping. You just really shouldn't try.
Your 'x.' + prop accessing can easily fail too, if the key isn't a proper "word". Any string can be a property name, including strings with spaces, reserved words, operators, etc.. You end up with code like x.hey look a string key === ...., which won't get you very far.
Using subscripting access, i.e. 'x["' + prop + '"]', will get you a bit further, but you're just back to the issue of quoting strings again.
Only primitive values can be easily compared; you'd still need a custom function for everything else.
You're changing the object that's been passed in; specifically, string values get quoted. I assume that's partly just because it's an example, but still. (I avoided this in the jsperf code.)

So yeah, it's much faster to use a function, but I'd still very much avoid constructing one on the fly. Better to just write one when necessary, or accept the performance hit otherwise.

As for the existing code, I like the iterative way of reducing the matches if the argument is an object. But I wanted to try turning the loops inside out, as I find that more straightforward:

return $.grep(this, function (item) {
  for(var prop in func) {
    if(item[prop] !== func[prop]) {
      return false;
    }
  }
  return true;
});

However, doing so is slower (somewhat to my surprise).

Otherwise, the code's fine, I only have a couple of notes.

The that variable would usually imply that you're dealing with scopes, but that isn't the case here; You can use plain this anywhere in your function. You do need a separate variable for the object-based filtering, but I'd just do something like var filtered = this.
You should throw a TypeError instead of a ReferenceError. And I'd write a slightly nicer and more helpful error message, such as "func must be either a function or an object of properties and values to filter by".
You'll probably want to add a func.hasOwnProperty(prop) check to avoid accidentally looping through a bunch of inherited properties. It's not an issue for object literals, but it can be for other objects.
You'll probably want to ensure that a copy of the original array in returned, even if nothing's been filtered. This isn't a problem if you pass in a function, but could happen if you pass in an empty object.
It's a tad heavy on the whitespace, if you ask me, but to each their own.

In the end, I get this

Array.prototype.where = function (filter) {
    switch(typeof filter) {
        case 'function':
            return $.grep(that, filter);

        case 'object':
            var filtered = this;
            for(var prop in filter) {
              if(!filter.hasOwnProperty(prop)) {
                  continue; // ignore inherited properties
              }
              filtered = $.grep(filtered, function (item) {
                  return item[prop] === filter[prop];
              });
            }
            return filtered.slice(0); // copy the array

        default:
            throw new TypeError("func must be either a function or an object of properties and values to filter by");
    }
};


Improving performance of VBA data validation script in Access with large data size I work in IT compliance and one of my main duties is ensuring the integrity of employee network accounts. Partial VBA script as currently used is shown below:

Public Function validateHireDate(hireDate as String)

    If Not IsDate(hireDate) Or IsNull(hireDate) Then
        validateHireDate = "Invalid hire date"
    Else
        validateHireDate = "Hire date reviewed"
    End if
End Function

Public Function validateEmployeeManager(employeeStatus as String, employeeManager as String)

    If employeeStatus <> "Active" Then
        Exit Function
    Else
        If IsNull(employeeManager) Then
            validateEmployeeManager = "Invalid employee manager"
        Else
            validateEmployeeManager = "Employee manager reviewed"
        End If
    End If
End Function

Given that the size of the database is around 50K of records, the above code works but is slow. There are several more validation functions such as for social security numbers, email, and user account control (disabled or enabled) depending on multiple criteria. Presently, the source data report is run bimonthly from which I import the results into Microsoft Access for auditing.

Is there a more efficient manner in which the validation can be done?

There are a couple of micro-optimizations to be made. They probably won't give you the boost you're looking for, but it's worth trying.

First, you can explicitly define the return type. When you don't define one, the compiler defines it as a Variant type, which incurs a lot of overheard compared to the "regular" data types.

For clarity, this

Public Function validateHireDate(hireDate as String)

Is equivalent to this

Public Function validateHireDate(hireDate as String) As Variant

And really should be

Public Function validateHireDate(hireDate as String) As String

But I find it a bit odd that a validation routine is returning a string. I would consider returning a boolean instead. It has a smaller memory footprint than a string and is, in theory, faster than a string.

The second thing you can do is manually short circuit the logic, but you'll have to sacrifice some DRYness to do it.

If Not IsDate(hireDate) Or IsNull(hireDate) Then
    validateHireDate = "Invalid hire date"
Else
    validateHireDate = "Hire date reviewed"
End if

VBA does't have short circuiting logical operations, so no matter what the return value of IsDate(hireDate) is, IsNull(hireDate) executes every time as well. We can unroll the logic a bit to make it more efficient, but be sure to leave a comment explaining your actions to your future self. If you're anything like me, you'll instantly want to undo this optimization the next time you see it if there's nothing telling you not to.

If IsNull(hireDate) Then
    validateHireDate = "Invalid hire date"

Else If Not IsDate(hireDate) Then
    validateHireDate = "Invalid hire date"

Else
    validateHireDate = "Hire date reviewed"
End if

Now, the order here maybe important, and I might not have it right. First, verify that IsDate() can handle a null value. If it can, then you need to study the data to determine which is the more common error state. You want the most common case to come first in order to optimize the logic.

I like your validateEmployeeManager function. You're evaluating the most likely situation first and exiting as fast as possible. You can explicitly declare the return type, but I see no further room for optimization. It's a well written function.

Finally, I would just encourage you to think about whether you need to do this in VBA at all. Access is a SQL based program and, like any SQL, it's better to use set based operations than procedure based ones. I would argue that it's perhaps even more important with Access. You've already noticed how slow the COM based VBA interaction is compared to a regular query, so why not move this logic into a query, or a series of queries? You're not doing anything here that can't be accomplished with the IIF and ISNULL functions. Even a faithful translation should be faster than even the optimized version of the VBA code. Although, I would also encourage you to investigate ways to do this truly set based instead.

Some psuedo-SQL for your first function.

SELECT IIF(ISNULL(Emp.HireDate),"Invalid",IIF(ISDATE(Emp.HireDate,"Invalid","Reviewed"))
FROM Employees

That's the faithful translation. I would find out if the requirements can be adjusted to only return the records that are in error. If the valid records are of no interest to the business, then something like this becomes possible.

SELECT * --don't actually select all
FROM Employees
WHERE HireDate IS NULL
    OR (NOT ISDATE(HireDate))

Which returns only the invalid records.


Beginner Rock, Paper, Scissors in Java I was curious on what, if anything, I could be doing better. I have been spending the last few days working with Java and learning some basics and wanted to try this challenge and making it two classes. I found the scanner is sometimes strange and doesn't take input the first time.

import java.util.Scanner;
import java.util.Random;

public class rps {
    public static void main (String args[]){
        int input;
        int b = 1;
        Scanner sage = new Scanner(System.in);
        Random rnd = new Random();
    System.out.println("Rock Paper Scissors, by Sage!");
    System.out.println("Select 1, 2, 3, for Rock, Paper, Scissors");
    //Menu Present, pretty bad still

        while (b != 0){
        int rock = 1, paper = 2, scissors = 3;
        input = sage.nextInt();
        int randomNumber = rnd.nextInt(3-1+1)+1;

            if(randomNumber == rock){
                if(input == rock){
                    System.out.println("Rock vs. Rock, ITS A TIE!");
            }    else if(input == paper){
                    System.out.println("Rock vs. Paper! You win!" );
            }    else if(input == scissors){
                    System.out.println("Rock vs. Scissors! You lose!");
            }  //These blocks establish options if the computer got Rock

            else if(randomNumber == paper){
                if(input == rock){
                    System.out.println("Paper vs. Rock! You lose!");
            }   else if(input == scissors){
                    System.out.println("Paper vs. Scissors! You win!");
            }   else if(input == paper){
                    System.out.println("Paper vs. Paper! Its a tie!");
            } //These blocks establish the options if comp. got paper

            else if(randomNumber == scissors){
                if(input == rock){
                    System.out.println("Scissors vs. Rock! You win!");
            }   else if(input == scissors){
                    System.out.println("Scissors vs. Scissors, ITS A TIE!");
            }   else if(input == paper){
                    System.out.println("Scissors vs Paper! You lose!");
            } //These blocks establish if the computer got scissors.

            }
            }
            rps2 rps2Object = new rps2();
            rps2Object.rps2();

        }

    }
    }
}

Then this is the other class

import java.util.Scanner;

public class rps2 {
    public void rps2(){
        Scanner sage = new Scanner(System.in);
        int b;
        b = 1;
        System.out.println("Play again? Y(8), N(9)?");
        int yes= 8, no = 9;
        int input;
        input = sage.nextInt();

            if(input == yes){
                System.out.println("Rock,Paper,Scissors!");

            }else{
                System.out.println("Thanks for playing!");
            }

    }
}

There are plenty of rock-paper-scissors implementations in java on this site.  I'll try to keep my suggestions at a beginner level, though.

First, I should point out that your "play again?" mechanism doesn't actually work: you ask the question, but never do anything with the result.    Putting the "play again?" prompt in its own class doesn't make much sense; it can just be a function.  That function should return a boolean result — true to play again, false to quit.  In main(), the while (b != 0) loop would best be written as a do-while loop, since you want to run the game at least once without asking.  b is a poor variable name; playAgain would be much more helpful.

The main lesson that I think you should learn from this exercise is object-oriented thinking.  Instead of input and randomNumber, how about thinking about a HumanPlayer and a RandomComputerPlayer, both of which can return a choice of "Rock", "Paper", or "Scissors" when asked to play()?  The code would better model the game.

To analyze the outcome, you have enumerated all 9 combinations.  Of those, three outcomes are ties, which can easily be detected.  It would also be better to group all of the winning outcomes and all of the losing outcomes together.

RockPaperScissors.java

import java.util.Random;
import java.util.Scanner;

public class RockPaperScissors {
    private static boolean playAgain(Scanner scanner) {
        System.out.println("Play again? Y(8), N(9)?");
        switch (scanner.nextInt()) {
        case 8:
            System.out.println("Rock, Paper, Scissors!");
            return true;
        default:
            System.out.println("Thanks for playing!");
            return false;
        }
    }

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        RPSPlayer computer = new RandomComputerPlayer(new Random());
        RPSPlayer human = new HumanPlayer(scanner);

        System.out.println("Rock Paper Scissors, by 200_success!");
        do {
            String comp = computer.play();
            String you = human.play();

            System.out.printf("%s vs. %s", comp, you);
            if (you.equals(comp)) {
                System.out.println(", IT'S A TIE!");
            } else if ( ("Rock".equals(you) && "Scissors".equals(comp)) ||
                        ("Scissors".equals(you) && "Paper".equals(comp)) ||
                        ("Paper".equals(you) && "Rock".equals(comp)) ) {
                System.out.println("! You win!");
            } else {
                assert (("Rock".equals(comp) && "Scissors".equals(you)) ||
                        ("Scissors".equals(comp) && "Paper".equals(you)) ||
                        ("Paper".equals(comp) && "Rock".equals(you)));
                System.out.println("! You lose!");
            }
        } while (playAgain(scanner));
    }
}

RPSPlayer.java

public interface RPSPlayer {
    String[] CHOICES = new String[] { "Rock", "Paper", "Scissors" };

    /**
     * Returns one of "Rock", "Paper", or "Scissors".
     */
    String play();
}

HumanPlayer.java

import java.util.Scanner;

public class HumanPlayer implements RPSPlayer {
    private final Scanner scanner;

    public HumanPlayer(Scanner scanner) {
        this.scanner = scanner;
    }

    public String play() {
        System.out.println("Select 1, 2, or 3 for Rock, Paper, Scissors");
        int choice = this.scanner.nextInt();
        // Keeping things simple, not doing any validation here
        return CHOICES[choice - 1];
    }
}

RandomComputerPlayer.java

import java.util.Random;

public class RandomComputerPlayer implements RPSPlayer {
    private final Random random;

    public RandomComputerPlayer(Random random) {
        this.random = random;
    }

    public String play() {
        return CHOICES[this.random.nextInt(CHOICES.length)];
    }
}


"Number guessing game" in Python I am new to Python and I have tried to come up with a code for the number guessing game. But I am pretty sure this is not the right way to do it. I am able to get the right output, however, my code is pretty long. Any ideas on using a different logic or to reduce the code size would help.

I also would like to know how to code profile a python code for performance.

I am using IDLE and Python 3.+

import random

def main():
    while(True):
        inputchoice = input("Are you ready? Y/N : ")
        if(inputchoice.strip() in ("y","Y","yes","Yes","YES","YEs","yeS","YeS","yEs","yES")):
            print("Ok. Lets begin")
            randomnumberguessinggame()
            break
        elif(inputchoice.strip() in ("no","No","NO","nO","n","N")):
            print("Let me know when you are ready")
            break
        else:
            print("Invalid Entry. Try again")

def randomnumberguessinggame():
    print("Get ready to start guessing")
    actualnumber = random.randrange(0,1000)
    #print("The number to be guessed is %d"%actualnumber)
    flag = True;
    while(flag):
        try:
            guessednumber = int(input("Enter your guess "))
            if(guessednumber > actualnumber):
                print("That was a wrong guess. Your guess is higher than my number")
                while(True):
                    retry = input("Would you like to try again? Y/N : ")
                    if(retry.strip() in ("y","Y","yes","Yes","YES","YEs","yeS","YeS","yEs","yES")):
                        flag = True;
                        break
                    elif(retry.strip() in ("no","No","NO","nO","n","N")):
                        flag = False;
                        break
                    else:
                        print("Invalid Entry. Try again")
            elif(guessednumber < actualnumber):
                print("That was a wrong guess. Your guess is lower than my number")
                while(True):
                    retry = input("Would you like to try again? Y/N : ")
                    if(retry.strip() in ("y","Y","yes","Yes","YES","YEs","yeS","YeS","yEs","yES")):
                        flag = True;
                        break
                    elif(retry.strip() in ("no","No","NO","nO","n","N")):
                        flag = False;
                        break
                    else:
                        print("Invalid Entry. Try again")
            else:
                print("You've got it right. Congratulations!!")
                flag = False;
        except ValueError:
            print("Your guess is invalid. Kindly guess again.")
            flag = True;

main()

print("Exiting the game...")

Advice 1

I would not abuse the user with the question whether he wants to continue; just have a command (say, quit) for quitting the game while still guessing.

Advice 2

randomnumberguessinggame

The Python way is random_number_guessing_game.

Advice 3

You don't have to use parentheses in the branch and loop conditionals.

Summa summarum

I had this in mind:

import random

def main():
    actual_number = random.randrange(0, 1000)
    while True:
        guess = input("Enter your guess: ")

        if guess.strip().lower() == "quit":
            print("Bye!")
            return

        try:
            guess_number = int(guess)
        except ValueError:
            print(guess, "is not an integer!")
            continue

        if guess_number < actual_number:
            print("Your guess is too small.")
        elif guess_number > actual_number:
            print("Your guess is too large.")
        else:
            print(guess_number, "Excellent!")
            return

main()

Hope that helps.


Rust: Flattening nested struct to Vec I have a struct which nests other structs like following:

#[derive(Debug)]
pub struct Rankings {
    conferences: Vec<Conference>,
}

#[derive(Debug)]
pub struct Conference {
    divisions: Vec<Division>,
}

#[derive(Debug)]
pub struct Division {
    teams: Vec<Team>,
}

#[derive(Debug, Clone)]
pub struct Team {
    name: String,
    market: String,
}

What I want to do is converting a Rankings instance to Vec<Team>. Here's my solution:

fn main() {
    let mut rankings = Rankings {
        conferences: vec![
            Conference {
                divisions: vec![
                    Division {
                        teams: vec![
                            Team {
                                name: String::from("Raptors"),
                                market: String::from("Toronto"),
                            },
                            Team {
                                name: String::from("Knicks"),
                                market: String::from("New York"),
                            }
                        ]
                    },
                    Division {
                        teams: vec![
                            Team {
                                name: String::from("Bucks"),
                                market: String::from("Milwaukee"),
                            },
                            Team {
                                name: String::from("Cavaliers"),
                                market: String::from("Cleveland"),
                            }
                        ]
                    },
                ]
            },
        ]
    };

    println!("- rankings:\n{:#?}\n", rankings);

    let mut raw_teams: Vec<Vec<Vec<Team>>> = rankings
        .conferences
        .iter_mut()
        .map(|c| c.divisions.iter_mut().map(|d| d.teams.clone()).collect())
        .collect();

    println!("- raw_teams:\n{:#?}\n", raw_teams);

    let flattened_teams = raw_teams
        .iter_mut()
        .fold(Vec::new(), |mut acc, val| {
            acc.append(val);
            acc
        })
        .iter_mut()
        .fold(Vec::new(), |mut acc, val| {
            acc.append(val);
            acc
        });

    println!("- flattened_teams:\n{:#?}\n", flattened_teams);
}

Playground link

First, I converted Rankings to Vec<Vec<Vec<Team>>> using iter_mut() and map(), then flattened Vec<Vec<Vec<Team>>> to Vec<Team> using iter_mut() and fold().

But I just wrote that code avoiding compile errors, which mean the code could be refactored better using idiomatic patterns. I think I might overuse mutability, and two conversion process can be simplified using appropriate iterator functions. Thanks for any advices.

First of all, there is no need to use iter_mut() on conferences, as you never change the original rankings. As we clone the teams later, we can simply use

let mut raw_teams: Vec<Vec<Vec<Team>>> = rankings
    .conferences
    .iter()
    .map(|c| c.divisions.iter().map(|d| d.teams.clone()).collect())
    .collect();

Now that we have a Vec<Vec<Vec<Team>>, we can call flatten:

let flattened_teams: Vec<Team> = raw_teams.into_iter().flatten().flatten().collect();

I used into_iter() as your original code left raw_teams empty.

However, we can skip raw_teams entirely if we use flat_map instead of map(…).flatten():

let flattened_teams: Vec<Team> = rankings
    .conferences
    .iter()
    .flat_map(|c| &c.divisions) // need to borrow due to iter()
    .flat_map(|d| &d.teams)     // need to borrow due to iter()
    .cloned()                   // clone the elements
    .collect();

If we don't want to borrow, we can of course just move everything into flattened_teams by simply removing cloned() and &:

let flattened_teams : Vec<Team> = rankings
    .conferences
    .into_iter()
    .flat_map(|c| c.divisions)
    .flat_map(|d| d.teams)
    .collect()

None of these functions use mut.