Represent json file with a set of different data members to a C++ class I want to parse a json file, where some elements are common each time while others are different based on a type.
For example a message json:

{
    "title": string,
    "body": string,
    "type": int (0 for email, 1 for sms, 2 for terminal)
    "typeProperties": {
        // Different based on type value
    }
}

I ended up to the following code to represent the json object:

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

class Message {
    public:
        int id;
        std::string title;
        std::string body;
        int type; // 0 - Email, 1 - SMS, 2 - Terminal
};

class Email : public Message {
    public:
        std::string emailRecipients;
        std::string emailCC;
};
class Sms : public Message {
    public:
        std::string  phoneNumber;
};
class Terminal : public Message {
    public:
        int terminalId;
};

std::vector<Message*> list_msg;

int main(int argc, char** argv) {
    Email mail;
    mail.id = 1;
    mail.title = "title1";
    mail.body = "body1";
    mail.type = 0;
    mail.emailRecipients = "text1";
    mail.emailCC = "text2";

    list_msg.push_back(&mail);

    Terminal tms;
    tms.id = 2;
    tms.title = "title2";
    tms.body = "body2";
    tms.type = 1;
    tms.terminalId = 123;

    list_msg.push_back(&tms);

    for (auto m : list_msg) {
        std::cout << "id: " << m->id << ", type: " << m->type << std::endl;

        if (m->type == 0) {
            Email *tmp = static_cast<Email*>(m);
            std::cout << ">>>> recipient: "  << tmp->emailRecipients << ", cc: " << tmp->emailCC << std::endl;
        } else if (m->type == 1) {
            Terminal *tmp = static_cast<Terminal*>(m);
            std::cout << ">>>> terminalId: "  << tmp->terminalId << std::endl;
        }
    }
}

Questions:

Is the above code correct?
From performance perspective, can this be optimized?
If I don't use inheritance and have one class Message with a boost::variant member - what are the pros and cons of this approach?

Correctness

I'm not sure what exactly you mean by 'correct'. Personally, I regard this code as poorly written, so I would reject it during peer review at my work. It's using C++ as if it were C, and violating several OOA/D heuristics.

Class Message knows it has subclasses

Class Message shouldn't have that 'type' member - a base class should not know anything about its subclasses, not even that any subclasses exist. At the moment, class Message knows that it has <= 2^(sizeof(int)*8 - 1) subclasses (else that int would need to be a 'uint128_t' or something). And if you changed that 'int' to be an 'enum message_type', which is what it really is, then Message would know of the existence of each of its subclasses, which would be even worse: addition of a subclass would require modifying the base class.

It seems to me you can simply delete that 'type' member, and the lines that set its value, without any loss of functionality.

The take-away point is: C++ objects (instances of some class) already 'know' the class of which they are an instance. You don't need to store this information a second time using class data members.

Encapsulation (Google for this term)

Classes shouldn't have public data - where you're setting public members directly, you should either be passing arguments to constructors, or (less preferably) calling setters after instance construction.

Performance

Do you really care about performance of this code? Think very hard about why this code is performance-critical. Chances are, it's not.

If this code really is performance-critical, for example it's executed thousands of times per second on a small embedded processor in a real-time system on which lives depend, then consider instead using an optimised binary format instead of JSON, using superior algorithms elsewhere in your code, offloading work to other parts of the system that are less time-critical or have more resources, or simply reducing the scope of the project to something easier to accomplish.

Pros and cons of using inheritance (instead of a variant member)

Pros of inheritance: Uses C++ in the way its designers intended and in the way later readers will expect. Allows for polymorphism (Google for this term). Promotes future code re-use and eases modification, by making it easier to add additional subclasses later on, or modify the superclass behaviour/implementation.

Cons of inheritance: I can't think of any. You'd be mimicking inheritance by use of containment, which may be sadly necessary in some circumstances but seems worthless to you in this circumstance.

Strange JSON

The JSON is redundant, which means it's possible for it to be inconsistent. What does it mean for the JSON to contain a 'type' attribute with the value 0, meaning email, but then inside its 'typeProperties' sub-object, it contains all of the the attributes appropriate to an SMS and none of the attributes appropriate to an email?

If this isn't possible, then why does the JSON design permit it? Why not just treat it as an SMS if it has SMS members, or an email if it has email members, and dispense with the 'type' attribute?

If this is possible, then how are you intending to handle it?


Get special sequence of integer numbers Task: Get array of random integer (less than 10) numbers and random sequence of mathematical operations (multiplication and addition) which, when applied, will give a even number

Example: genOddSeq(4) -> [[6, 6, 6, 9], ['+', '*', '*']] (6 + 6 * 6 * 9 = 330)

Code:

function genOddSeq(count) {
  let getRandomInt = (min, max) => Math.floor(Math.random() * (Math.ceil(max) - Math.ceil(min))) + Math.ceil(min)
  let result = []
  let operations = []
  let numbersComp = 0;

  for (let i = 0; i < count; i++) {
    result.push(getRandomInt(1, 10))
  }

  for (let i = 0; i < count - 1; i++) {
    operations.push(Math.random() > 0.5 ? '+' : '*')
  }

  numbersComp += operations[0] == '+' ? result[0] + result[1] : result[0] * result[1]

  for (let i = 1; i < count - 1; i++) {
    numbersComp = operations[i] == '+' ? numbersComp + result[i + 1] : numbersComp * result[i + 1]
  }

  if (numbersComp % 2 != 0) {
    result[getRandomInt(0, count)] += 1
  }

  return [result, operations]
}

Your function name is strange. You wanted to get a sequence of random numbers and operations whose result is an even number. But you named your function genOddSeq. Also, avoid abbreviations. Write code for the human, not the machine.

Instead of using an empty array and a loop to fill that array, consider using Array(), Array.fill() and array.map(). For example, creating an array of 10 random values from 0 to 1 would look like:

const tenRandomDecimals = Array(10).fill().map(Math.random)

You can do the same for the operations.

Now instead of hard-coding the logic of the operation selection inside a ternary, consider using a key-value pair where the key is + and * and the values are functions that accept two numbers which do the actual operation. When you generate an array of random + and *, just use them to get the actual function in the key-value pair.

if (numbersComp % 2 != 0) {
  result[getRandomInt(0, count)] += 1
}

This bit of logic is strange. Do you really need to get a random value? Or can you just use one from a known location, like the first number?

Last but not least, tests. It's always good practice to start with tests. Even other code review posts include a minimal form of test to verify the code is working.

Anyways, here's how I'd implement it. A bit longer, but there's no surprises. There's no state held, no variables to keep track of. Also using const both as a signal and a guarantee that the value will not change during runtime.

const operationFunctions = {
  '+': (a, b) => a + b,
  '*': (a, b) => a * b,
}
const operationKeys = Object.keys(operationFunctions)
const getRandomInteger = (min, max) => Math.floor(Math.random() * (Math.ceil(max) - Math.ceil(min))) + Math.ceil(min)
const getRandomOperationKey = () => operationKeys[Math.floor(Math.random() * operationKeys.length)]
const range = size => Array(size).fill().map((v, i) => i)

const getEvenResultSequence = count => {
  const firstNumber = getRandomInteger(1, 10)
  const numbers = range(count - 1).map(() => getRandomInteger(1, 10))
  const operations = range(count - 1).map(getRandomOperationKey)
  const result = operations.reduce((c, v, i) => operationFunctions[v](c, numbers[i]), firstNumber)
  const adjustedFirstNumber = result % 2 ? firstNumber + 1 : firstNumber

  return [[adjustedFirstNumber, ...numbers], operations]
}

console.log(getEvenResultSequence(4))
console.log(getEvenResultSequence(4))
console.log(getEvenResultSequence(4))
console.log(getEvenResultSequence(4))
console.log(getEvenResultSequence(4))


Managing course enrollment I have written some code to suspend someone's enrollment to a course. A course may have multiple presentations.

module Enrolments
  class Suspend
    attr_reader :enrolment, :responses, :comment, :user_id
    attr_writer :event_service, :suspend_enrolment_presentation_service

    def initialize
      @responses = []
    end

    def call(enrolment, user_id, comment)
      @enrolment = enrolment
      @user_id = user_id
      @comment = comment

      suspend_enrolment_presentations
      if all_enrolment_presentations_suspended?
        suspend_enrolment
        raise_suspended_enrolment_event
        create_enrolment_comment
      end
    end

    private

    attr_reader :response

    def suspend_enrolment_presentations
      enrolment.enrolment_presentations.each do |ep|
        @responses << suspend_enrolment_presentation_service.call(ep)
      end
    end

    def all_enrolment_presentations_suspended?
      @responses.all?{ |r| r[:success] }
    end

    def suspend_enrolment
      enrolment.status = 'Suspended'
      enrolment.save
    end

    def raise_suspended_enrolment_event
      event_service.call(enrolment)
    end

    def create_enrolment_comment
      EnrolmentComment.create(comment: comment, user_id: user_id, enrolment_id: enrolment.id)
    end

    def event_service
      @event_service ||= Events::Outgoing::Enrolments::SuspendedEnrolment.new
    end

    def suspend_enrolment_presentation_service
      @suspend_enrolment_presentation_service ||= Enrolments::EnrolmentPresentations::Suspend.new
    end
  end
end

The code is fairly straight forward. It has a couple of dependencies that are injected via attr_writers when testing, otherwise they are instantiated when accessed.

I have heard/read a lot of issues regarding callbacks. The problem with this code is if I ever want to just suspend an enrollment and not raise an event or create a comment, I can't. Some ways to get around this are with the decorator pattern or I guess I could compose and object of all the services and manage everything in there.

I really strive for SRP and good design in my code. I would really appreciate any input on how I could improve this code. Should I be extracting the code to a higher level of abstraction and dealing with it one level up? What if running one piece of code depends on another passing? If I was to extract this out it would land up in my controller which is as high as I can go, unless I then extracted that into a form object.

You could get rid of the service methods, initialise the values in the initialize method.  And then in a subclass set a service that doesn't actually do anything with the enrollment.

class Enrolments::Suspend
  ...
  def initialize
    @responses = []
    # get rid of the methods and set the services in the intitialiser
    @event_service ||= Events::Outgoing::Enrolments::SuspendedEnrolment.new
    @suspend_enrolment_presentation_service ||= Enrolments::EnrolmentPresentations::Suspend.new
  end
  ...
end

class EnrollmentWithoutEvent < Enrolments::Suspend
  ...
  def initialize
    super
    # do default config here
    @event_service ||= Events::DoNothing.new
  end
  ...
end

Or, you could create a subclass that overrides the raise_suspended_enrolment_event method:

class Enrolments::SuspendWithoutErrors < Enrolments::Suspend
  def raise_suspended_enrolment_event
    # change the implementation here
  end
end

Another options would be to use class_attribute for your service objects/procs:

class Enrolments::Suspend
  ...
  class_attribute :event_service, :suspend_enrolment_presentation_service
  ...
end

Then, when you subclass, you get set your services per class or instance and can  change/add the services on any sub-classes or instances of sub-classes.  Have a look at here for how it works.  This method means that you can create classes with specific services, and when you instantiate the class, the instances also use the same services.

For example:

class Enrolments::Suspend
  ...
  class_attribute :event_service, :suspend_enrolment_presentation_service

  # create the services used for this class
  self.event_service = proc {|enrollment| ... }
  self.suspend_enrolment_presentation_service = proc {|e| ... }
end

class SuspendWithoutError < Enrolments::Suspend
  # override one of the services but keep the rest
  self.suspend_enrolment_presentation_service = proc {|e| ... }
end

The final option is to use actual methods:

class Enrolments::Suspend
  ...

  def event_service(enrollment)
    ...
  end

  def suspend_enrolment_presentation_service(enrollment)
    ...
  end
end

This allows any sub-classes to override the methods.


UnionFind data structure implemented in Python This class is used to create sets out of nodes to from a graph-like structure (or grouping since I remove most of the normal graph structure and all child nodes have the same parent).

Is there anything I can do to improve the design of this class? Is the use of static methods here appropriate?

class UnionFind(object):
    '''
    Used to initialize the set and then holds the required functions
    to operate on them
    '''

    def __init__(self, vertex):
        self.rank = 0
        self.parent = self
        self.vertex = vertex

    @staticmethod
    def find(x):
        if x != x.parent:
            x.parent = UnionFind.find(x.parent)
        return x

    @staticmethod
    def union(x,y):
        xroot = UnionFind.find(x)
        yroot = UnionFind.find(y)

        if xroot == yroot:
            return
        if xroot.rank > yroot.rank:
            yroot.parent = xroot
        else:
            xroot.parent = yroot
            if xroot.rank == yroot.rank:
                yroot.rank += 1

find is not a right name. A method find(x) is expected to find objects matching x. find_root seems more appropriate.

I know that Sedgewick & Wayne use find, and they are wrong.
find (or find_root) has no reason to be static. x.find_root() is way more natural than UnionFind.find_root(x).
Python does not eliminate tail recursion. I recommend to rewrite find as

def find(x):
    while x != x.parent:
        x = x.parent
    return x


Divide the last element in an array of arrays among the remaining elements The function below is part of an overall chunk of code to partition an array  of size M into sub-arrays of size N. The function in question is written to handle the case: what if M isn't divisible by N?  Then, the way the rest of the code works, the last element in the partitioned array might have fewer than N items.  (E.g., if N = 3, there might be an array like [[A, B, C], [D, E, F], [G, H]].

There are two reasonable solutions: (a) just accept a shorter array at the end (maybe with padding, depending on use case), or (b) take the shorter array and distribute it among the remaining arrays.  This code implements solution (b).

function mushInLittleArray(arrs, size){
    var correctSize = arrs[arrs.length - 1].length == size;
    if (correctSize) {
        return arrs;
    }
    var last = arrs.pop();
    var len = last.length;
    var j = 0;
    var div = false;
    var arrlen = arrs.length;
    for (var i = 0; i < len; i++) {
        div = divisible(j, arrlen);
        if (div) {
            j = 0;
        }
        arrs[j].push(last[i]);
        j++;
    }
    return arrs;
}

This code just feels awkward and hackish to me.  In particular, the sleight of hand in the conditional toward the end beginning if (div) smells bad. It's meant to deal with the edge case where leftover group is larger than number of other groups, for example, where N = 5 and we have something like [[A, B, C, D, E], [F, G, H, I, J], [K, L, M, N]], and hence the iteration over the large array needs to reset.  The code as written seems to work fine, but having two loop variables and modifying one of them manually within the block seems like dirty pool to me.  Surely there must be a better way?

Based on your constraints, which are:

Single loop variable
No restarting of iteration over large arrays

You could calculate your looping step sizes first, then go over the array only once.

  const mushedArr = [];
  const remainderSize = arr.length % size
  const numberOfChunks = Math.floor(arr.length / size);
  let remainderStepSize = Math.floor(remainderSize / numberOfChunks);

I'm going to define the remainder with negative index slicing, so there has to be a guard against calculating a remainder even when division is exact.

let remainder = remainderSize ? arr.slice(-remainderSize) : []

Second edge case is where the remainder is smaller than the chunk size, which would cause my step size to evaluate to 0 and not loop through the remainder properly.

if(remainderStepSize === 0) remainderStepSize ++;

Finally, loop over the array and the remainder:

 let i = 0;
  while(i < numberOfChunks){
    let remainderPortion = remainder
        .slice(i*remainderStepSize, i*remainderStepSize+remainderStepSize);

    let arrayPortion = arr.slice(i*size, i*size+size);

    mushedArr.push([...arrayPortion, ...remainderPortion]);
    i++;
  };
  return mushedArr;
};

Variable names here could be shorter, and the slice can be simplified to
something like arr.slice(size*i, size*(i+1), but the idea is to loop over the array and copy the items in sizes that are equal to the chunk size.

Testing for both inputs in your question yielded:

Calling function with input: [A,B,C,D,E,F,G,H,I,J,K,L,M,N]
 chunk size: 5
[ [ 'A', 'B', 'C', 'D', 'E', 'K', 'L' ],
  [ 'F', 'G', 'H', 'I', 'J', 'M', 'N' ] ]
Calling function with input: [A,B,C,D,E,F,G,H]
 chunk size: 3
[ [ 'A', 'B', 'C', 'G' ], [ 'D', 'E', 'F', 'H' ] ]


PHP design pattern factory input for products and quotes I'm working on figuring out the best way to design this so that it's well organized and it seems like the factory design pattern makes sense.

Updated:
What I'm building:
 - Pricing calculator
 - There are 4 products each having their own specific pricing rules that vary from user input
 - The person can choose between 1 product or all products

Should I have a Calculator class to determine product pricing or leave the calculations in each product type?

/*
 * Use Factory Pattern to build objects
 */

class ProductFactory {
    public static function createProduct($type) {
        switch ($type) {
            case 'solution1':
                return new Solution1Product();
                break;

            case 'solution2':
                return new Solution2Product();
                break;

            case 'solution3':
                return new Solution3Product();
                break;

            case 'solution4':
                return new Solution4Product();
                break;
        }

    }
}

/*
 * Abstract Product
 */

abstract class AbstractProduct {
    protected $price;
    protected $max_limit;
    protected $max_amount;

    abstract public function calculatePrice($dob, $gender, $amount);
}

/*
 * Solution 1 Product
 */
class Solution1Product extends AbstractProduct {
    protected $max_limit = 50000;

    function __contruct() {
        print "Test";
        return "Solution1";
    }

    function calculatePrice($dob, $gender, $amount) {
        $rate = 1.03;
        $price = 55;

        if ($amount >= $this->max_limit) {
            return "Max Limit Reached";
        }
        //@TODO go lookup price from tables

        $this->price = $rate * $price;
        return $this->price;
    }
}
$solution1 = ProductFactory::createProduct('solution1');
print $solution1->calculatePrice("02/25/1982", "male", 100000);

Flexible approach

First defining an ultimate abstraction of the factory
Implement it with product type - class mapping
Creating an entry point to register and use the factory

interface IProductFactory {

    function createProduct($type);
}

class DefaultProductFactory implements IProductFactory {

    private $_mappings;

    public function __construct(array $mappings = array()) {
        $this->_mappings = $mappings;
    }

    public function addMappings(array $mappings) {
        $this->_mappings = array_merge($this->_mappings, $mappings);
    }

    public function createProduct($type) {
        if (!isset($this->_mappings[$type])) {
            throw new \Exception("Cannot create product of type " . $type);
        }

        $class = $this->_mappings[$type];

        //can be passed to a DI container to instantiate if needed

        return new $class();
    }
}

class ProductFactory {

    private static $_factory;

    public static function setCurrent(IProductFactory $factory) {
        $this->_factory = $factory;
    }

    public static function getCurrent() {
        return $this->_factory;
    }
}

ProductFactory::setCurrent(new DefaultProductFactory(array(
    "solution1" => "Solution1Product",
    "solution2" => "Solution2Product",
    "solution3" => "Solution3Product",
    "solution4" => "Solution4Product",
    "solution5" => "\\\\In\\\Some\\\\Other\\\\Namespace\\\\Solution5Product",
    )));

$solution1 = ProductFactory::getCurrent()->createProduct('solution1');

print $solution1->calculatePrice("02/25/1982", "male", 100000);


Find palindromes in circular rotated string I'm trying to tackle a programming challenge to determine how many palindromes exist in a given string that is being rotated character by character. My solution works, but it's too slow, and I'm struggling on where to optimize it.

For example, the string "cacbbba" would rotate 6 times, and would have 3 palindromes in each string.

cacbbba (3 palindromes)
acbbbac (3 palindromes)
cbbbaca (3 palindromes)
bbbacac (3 palindromes)
bbacacb (3 palindromes)
bacacbb (3 palindromes)
acacbbb (3 palindromes)

function circularPalindromes(s) {
    const k = s.length;
    const result = [];
    const isPalindrome = (str) => {
        const len = str.length;

        for (let i = 0; i < len / 2; i++) {
            if (str[i] !== str[len - 1 - i]) {
                return false;
            }
        }

        return true;
    };
    const subs = (s) => {
        let max = 0;

        for (let i = 0; i < s.length; i++) {
            for (let j = 1; j <= s.length - i; j++) {
                const sub = s.substring(i, i + j);

                if (sub.length < 2) continue;

                if (isPalindrome(sub) && sub.length > max) {
                    max = sub.length;
                }
            }
        }

        return max;
    };

    for (let i = 0; i < k; i++) {
        result.push(subs(s.substring(i, k) + s.substring(0, i)));
    }

    return result;
}
console.log(circularPalindromes('cacbbba'));

After trying to optimize my current solution and getting nowhere fast, I went back to the drawing board and came up with this solution. The performance gains are outstanding: 543,334 ops per/sec vs. 275 ops per/sec

Performance Test

https://jsperf.com/largest-vs-subs

Solution

function circularPalindromes(s) {
    s = s.split('');

    let currentLength, equalsLength, j1, j2;
    const length = s.length;
    const length2 = s.length - 1;
    const largest = new Array(s.length).fill(0);

    for (let i = 0; i < s.length; i++) {
        currentLength = 1;
        j1 = (i < 1) ? length2 : i - 1;
        j2 = (i >= length2) ? 0 : i + 1;

        while (s[i] === s[j2] && currentLength < length) {
            currentLength++;
            if (++j2 >= length) j2 = 0;
        }
        equalsLength = currentLength;

        if (currentLength > 1) {
            checkEqual(largest, i, currentLength);
            i += currentLength - 1;
        }

        while (s[j1] === s[j2] && currentLength < length && j1 !== j2) {
            currentLength += 2;
            if (--j1 < 0) j1 = length2;
            if (++j2 >= length) j2 = 0;
        }

        if (currentLength > equalsLength) {
            if(++j1 >= length) j1 = 0;
            checkLargest(largest, j1, currentLength, equalsLength);
        }
    }

    return largest;
}

function checkEqual(largest, position, length) {
    const limit = position + length;
    const middle = position + (length >> 1);
    const even = (length & 1) === 0;

    for (let i = (position - largest.length + length < 0 ? 0 : position - largest.length + length); i < position; i++) {
        if (largest[i] < length)  largest[i] = length;
    }

    for (let i = position + length; i < largest.length; i++) {
        if (largest[i] < length) largest[i] = length;
    }

    for (let i = position, j = position; i < limit; i++, j++) {
        if (j >= largest.length) j = i % largest.length;
        if (largest[j] < length) largest[j] = length;
        if (i < middle){
            length--;
        } else if (i > middle) {
            length++;
        } else if (even) {
            length++;
        }
    }
}

function checkLargest(largest, position, length, equalsLength) {
    const limit1 = position + (length >> 1) - (equalsLength >> 1);
    const limit2 = position + length;

    for (let i = (position - largest.length + length < 0 ? 0 : position - largest.length + length); i < position; i++) {
        if (largest[i] < length) largest[i] = length;
    }

    for (let i = position + length; i < largest.length; i++) {
        if (largest[i] < length)  largest[i] = length;
    }

    for (let i = position, j = position; i < limit1; i++, j++) {
        if (j >= largest.length) j = i % largest.length;
        if (largest[j] < length) largest[j] = length;
        length -= 2;
    }

    for (let i = limit1 + equalsLength, j = limit1 + equalsLength; i < limit2; i++, j++) {
        if (j >= largest.length) j = i % largest.length;
        if (largest[j] < length) largest[j] = length;
        length += 2;
    }
}

console.log(circularPalindromes('cacbbba'));


Which option is better for readability when dealing with blank strings? I'm working off a specification that says a bunch of fields within a record (one record being one line) must be blank. For example, it says that chars 6-14 must be blank. I am building a class hierarchy structure that represents the layout of the file, and within this class hierarchy I'm including every field for each type of record, including the blank ones.

Basically I want to know if I should make each field return a blank literal, or if I should pad string.Empty with spaces.

Example:

public string ImmediateDestinationRoutingNumber { get { return "        "; } }

Versus

public string ImmediateDestinationRoutingNumber { get { return string.Empty.PadRight(8, ' '); } }

The second option of padding string.Empty looks the most readable to me, but I don't know if the runtime cost is significant enough to worry about.

Instead of returning a "        " or string.Empty.PadRight(8, ' ') you should extract the values to some meaningful constants.

The easiest way would be for your example to just use the overloaded constructor of the String class like

private const string eightSpaces = new string(' ', 8);

this makes it clear for Mr./Mrs.Maintainer what it stands for. But because this seems to produce CS0133: The expression being assigned to <constant> must be constant. you should just use a readonly string like

private readonly string eightSpaces = new string(' ', 8);

Using your first version will lead for each bug which needs to be solved to count the spaces which are returned.

The name of this property is also poorly named. It doesn't represent an ImmediateDestinationRoutingNumber but instead ImmediateDestinationRoutingNumberSpaces.


Bringing Clojure to project Euler 19 I recently began learning Clojure for fun, and after completing the koans, I decide that project Euler would provide my next challenge. Here is the problem statement:

(If someone knows how to put this in spolier tags with out breaking the formatting, please feel free to edit or comment and let me know how to fix it.)

  Counting Sundays Problem 19

  You are given the following information, but you may prefer to do some research for yourself.

  1 Jan 1900 was a Monday.
  Thirty days has September,
  April, June and November.
  All the rest have thirty-one,
  Saving February alone,
  Which has twenty-eight, rain or shine.
  And on leap years, twenty-nine.
  A leap year occurs on any year evenly divisible by 4, but not on a century unless it is divisible by 400.

  How many Sundays fell on the first of the month during the twentieth century (1 Jan 1901 to 31 Dec 2000)?

This is the code I used to arrive at the solution. Note that I purposely avoided the date/time APIs so I could try more of the language features out.

(ns euler.n19)

(def week-days (cycle [:mon :tue :wed :thr :fri :sat :sun]))

(defn leap-year?
  ([year] (if (= 0 (mod year 100))
            (= 0 (mod year 400))
            (= 0 (mod year 4)))))

(def months [:jan :feb :mar :apr :may :jun :jul :agu :sep :oct :nov :dec])
(def month-length {:jan 31 :feb 28 :mar 31 :apr 30
                   :may 31 :jun 30 :jul 31 :agu 31
                   :sep 30 :oct 31 :nov 30 :dec 31})
(def leap-year-month-length (assoc month-length :feb 29))

(def years (->> (range 1901 2001)
               (map (fn [yr] (if (leap-year? yr)
                               leap-year-month-length
                               month-length)))))

(defn zip
  ([coll-1 coll-2] map list coll-1 coll-2))

(defn count-number-of-times-day-on-nth-of-month
  ([day date]
   (->> years
        (map (fn [month-len] (map #(% month-len) months)))
        (flatten)
        (reduce (fn [[week-days num-occurrences] curr-month-len]
                  [(drop curr-month-len week-days)
                   (if (= day (nth week-days date))
                     (inc num-occurrences)
                     num-occurrences)])
                [(drop 1 week-days) 0])
        (second))))

(defn main ([]  println (count-number-of-times-day-on-nth-of-month :sun 0)))

In particular, I'm wondering how the reduce in count-number-of-times-day-on-nth-of-month could be clarified since it looks very messy to me.

(As an aside, after a few hours of pouring over this and the koans, I noticed that it is now very hard to type my other code correctly :)

A function that gives you the sequence of month lengths in a given year:

(defn month-seq [year]
  (map
    (if (leap-year? year) leap-year-month-length month-length)
    months))

... is all that your count-number-of-times-day-on-nth-of-month function needs.

I've extended it to accept a year-range as well as day and
date.
I've replaced

flatten with the simpler mapcat and
reduce  with reductions, to simplify the algorithm.

The result is ...

(defn count-number-of-times-day-on-nth-of-month
  [year-range day date]
  {:pre [(>= (first year-range) 1900)]}
  (let [month-lengths (mapcat month-seq (range 1900 (inc (last year-range))))
        day-seqs (reductions
                   (fn [s n] (drop n s))
                   week-days
                   month-lengths)
        first-days (map first day-seqs)]
    (->> first-days
         (drop (+ (* 12 (- (first year-range) 1900)) date))
         butlast
         frequencies
         day)))

For example,

(count-number-of-times-day-on-nth-of-month (range 1901 2001) :sun 0)

Odds and ends

Your zip function (unused) and main function are each missing a pair of parentheses.
If there is only one arity, there is no need for parentheses around
the argument list and body.

For example,

(defn zip
  [coll-1 coll-2] (map list coll-1 coll-2))

Edited to correct a sign error in dealing with the date argument to count-number-of-times-day-on-nth-of-month.


Sort a list of pairs representing an acyclic, partial automorphism I have this sample data:

let trips = [
  {
    from: "DEN",
    to: "JFK"
  },
  {
    from: "SEA",
    to: "DEN"
  },
  {
    from: 'JFK',
    to: 'SEA'
  },
];

and my origin is 'JFK', I want to sort the list by how I have traveled. So for instance, this should be the end result:

let trips = [
  {
    from: 'JFK',
    to: 'SEA'
  },
  {
    from: "SEA",
    to: "DEN"
  },
  {
    from: "DEN",
    to: "JFK"
  },
];

My solution works but it's not very well written, but I tried!

function sortByLinked(trips, origin = 'JFK') {
  let sortedArray = [];

  let first = trips.filter(trip => trip.from === origin)[0];
  sortedArray.push(first);

  for(var i = 0; i < trips.length; i++) {
    if(sortedArray[0].to === trips[i].from) {
      sortedArray.push(trips[i]);
    }
  }

  for(var i = 0; i < trips.length; i++) {
    if(sortedArray[1].to === trips[i].from) {
      sortedArray.push(trips[i]);
    }
  }

  return sortedArray;
}

sortByLinked(trips)

Code style

Use constants for variables that do not change. Eg const sortedArray = [];

Don't include the type in the name, Eg const sortedArray = []; can be Eg const sorted = [];

The default parameter in this case seams inappropriate as it is likely that "JFK" is not an origin in all calls to this function. If no origin is given one could assume that the first item in trips contains the origin.  function sortByLinked(trips, origin = trips[0].from) { which will throw an error if trips is empty so the function should not be called with an empty trips array if you don't pass the from parameter.
However in this example best to leave the default as undefined if not passed as that will return an empty array which is more fitting the input parameters.

The name sort is inappropriate as in JS it implies that the array be sorted in place, that all items be sorted (may not be possible).

You have declared i two times. As a var you should put the declaration at the top of the function and not in the for loop.

Rather than use Array.filter you can use Array.find. It will find the first instance.

Using for...of rather than for(;;) reduces the code complexity.

No point continuing the search inside the for loops when you have found a match. Use the break token to stop a loop early

Put a space between if and (

Don't forget to add the ; where appropriate. It is missing from the call sortByLinked(trips)

You call the origin origin and from this can get confusing. Keep the naming unambiguous. As the trip items use from then that would be the better name for the second input argument.

Using the above points to modify your code we get
function tripFrom(trips, from) {
  const sorted = [];
  const first = trips.find(trip => trip.from === from);
  sorted.push(first);

  for (const trip of trips) {
    if (first.to === trip.from) {
      sorted.push(trip);
      break;
    }
  }
  for (const trip of trips) {
    if (sorted[sorted.length - 1].to === trip.from) {
      sorted.push(trips);
      break;
    }
  }
  return sorted;
}

sortByLinked(trips, "JFK");

This is still not a good solution. Its not at all DRY (don't repeat yourself) and is hard coded to a single use case.
Improving the function.
It can all be done within a single loop and work for any length array.
To create the function we must add some constraints on the array trips and what to do when we encounter any problems.

That the array trips contains objects that each have the property from and to that are correctly formatted static strings. The resulting array is erroneous or indeterminate if not so.
That the array does not contain circular trips shorter than the array length.
That a complete trip length is no longer than the array, or when a matching trip.to can not be found. The returned array can be 0 to trips.length in size.
Locations are case sensitive.
If there is more than one matching trip.from it is assumed that the first match in trips is the correct one. (It would be interesting to extract the longest possible trip from? or the shortest trip that returns to the origin?)

Example
function tripFrom(trips, from) {
    const result = [];
    while (result.length < trips.length) {
        const trip = trips.find(trip => trip.from === from);
        if (!trip) { break }
        from = trip.to;
        result.push(trip);
    }
    return result;
}
tripFrom(trips, "JFK");

Or if it is known that the trip is the same length as the input array.
function tripFrom(trips, from) {
    const res = [];
    while (res.length < trips.length) {
        from = (res[res.length] = trips.find(trip => trip.from === from)).to;
    }
    return res;
}
tripFrom(trips, "JFK");

It is unclear if you want the array sorted in place. If that is a requirement then the above version can be modified to do that by simply copying the results array res to the trips array. You can empty an array by setting its length to zero. The spread ... operator in this case spreads the array items over the functions arguments trips.push(...res) thus pushing all the items to the array.
function tripFrom(trips, from) {
    const res = [];
    while (res.length < trips.length) {
        from = (res[res.length] = trips.find(trip => trip.from === from)).to;
    }
    trips.length = 0;
    trips.push(...res);
    return trips;
}
tripFrom(trips, "JFK");


Acquiring an ID from a newly inserted entry in an MSSQL database In an MSSQL database, I have automatically increasing IDs for every entry.
Is it safe to acquire the ID of the newly inserted entry like this? I am worried that, if at the exact same time another entry gets inserted, I could end up with the wrong ID.

string query = "INSERT INTO subscriptions (polygon,...) VALUES (@polygon,...)";
                    SqlDataAdapter cmd = new SqlDataAdapter();
                    using (SqlCommand querySaveSubscription = new SqlCommand(query))
                    {
                        querySaveSubscription.Connection = openCon;
                        // Create Poly out of given geodata values
                        var point = SqlGeography.Point(Double.Parse(lat), Double.Parse(lon), 4326);
                        SqlGeography poly = point.BufferWithTolerance(Double.Parse(radius) * 1000, 0.01, true);

                        var param = new SqlParameter(@"polygon", poly);
                        param.UdtTypeName = "Geography";

                        querySaveSubscription.Parameters.Add(param);
        ...

                        openCon.Open();
                        querySaveSubscription.ExecuteNonQuery();

I have the following c# code directly after my INSERT INTO - query getting executed:

                    query = "SELECT ID FROM subscriptions ORDER BY ID DESC";
                    SqlCommand command = new SqlCommand(query, openCon);
                    SqlDataReader reader = command.ExecuteReader();
                    reader.Read();

                    int id = reader.GetInt32(0);

                    result.Add("success", "true");

                    result.Add("id", id.ToString());

You can use scope_identity as @RobH hinted in a comment, but the sexiest way IMO is to use the OUTPUT clause of the INSERT syntax:

DECLARE @result TABLE (Id INT NOT NULL);

INSERT INTO subscriptions (polygon,...)
VALUES (@polygon,...)
OUTPUT INSERTED.Id INTO @result;

SELECT Id FROM @result;

Now, having a whole multi-statement CommandText like this, even if it's parameterized, isn't desirable at all, and makes for pretty ugly C# code - why not put that T-SQL in a stored procedure, and call that instead?

Speaking of parameters, your code snippet, as small as it is, is showing us that your method is breaking SRP (Single Responsibility Principle):

   // Create Poly out of given geodata values
    var point = SqlGeography.Point(Double.Parse(lat), Double.Parse(lon), 4326);
    SqlGeography poly = point.BufferWithTolerance(Double.Parse(radius) * 1000, 0.01, true);

That logic doesn't belong there at all: you need a SqlGeography-type parameter, fine. That doesn't mean the logic for translating lat and lon values into one belongs in that method.

Side note, I get what lat and lon stand for in the context of creating a SqlGeography value, but these are bad variable names nonetheless: there's no reason to shorten the names like this - not even "it's faster to type"; with IntelliSense you type "lat" and it autocompletes "latitude" for you.

I'm going to make a little assumption here. This makes me think your method is returning something like a Dictionary<string,string>:

result.Add("success", "true");
result.Add("id", id.ToString());

That's a very error-prone way of working. The "success" value is useless, because if you have a value, you have a successful insert. If you don't have a value, you're dealing with an exception - and I see no try/catch block in your code, so I'm hoping the caller is handling such an exception...

You could have a class somewhere, with properties for each column of a database table / entity you're working with, and you should return that instead... but the simplest return type to use would be a Nullable<int>, so that the method either succeeded and returns the Id for the inserted record, or failed and returns null - for that to happen reliably, you'll need to wrap your logic in a try/catch block, and handle any SqlException that might be thrown in the process.

Another thing is that you're not properly disposing of your connection there. SqlConnection implements IDisposable, and the instance should be wrapped in a using block, too. Also I would try to open the connection before I start creating the command, so that I could pass the connection into the command's constructor (and not even bother instantiating it if the connection can't be opened). Keeps things cleaner I find:

try
{
    using (var connection = new SqlConnection(...))
    {
        connection.Open();
        using (var command = new SqlCommand(sql, connection))
        {
            //...
        }
    }
}
catch (SqlException)
{
    return null;
}

Lastly, I find that you're using confusing identifiers: in the top snippet your adapter is called cmd, and your command is called querySaveSubscription, as if it were a string containing the SQL statement (and the SQL string in question is called query... but doesn't query anything). ...but then in the bottom snippet a command is called a command and a reader is called a reader, and a connection that might never have successfully opened is called openCon. Would you ever work with a closedCon? Why not just call it connection then?


(Ruby version) Rotate linked list to the right by k places I saw this question (javascript - rotate linked list to the right by k places) the other day and I tried implementing it in Ruby in a totally different way. Here's the code (I also created a REPL if you want to test it - https://repl.it/repls/PrimeSomeBlock):

node.rb

class Node
  attr_accessor :value, :next_node

  def initialize(value:, next_node: nil)
    @value = value
    @next_node = next_node
  end
end

linked_list.rb

class LinkedList
  attr_reader :nodes

  def initialize(nodes: [])
    @nodes = nodes
  end

  def rotate(k)
    self.class.new(nodes: rotate_nodes(k))
  end

  def rotate!(k)
    @nodes = rotate_nodes(k)
  end

  def to_s
    @nodes.map(&:value).join("->")
  end

  private

  def rotate_nodes(k)
    if !k.between?(1, @nodes.length) || !k.is_a?(Integer)
      raise "`k` must be an integer between 1 and the length of the list"
    end

    @nodes.map do |node|
      n = @nodes.find_index(node)
      @nodes[n - k].next_node = n - k == 1 ? nil : @nodes[n - k + 1]
      @nodes[n - k]
    end
  end
end

main.rb

require_relative "./node"
require_relative "./linked_list"

n4 = Node.new(value: 5)
n3 = Node.new(value: 3, next_node: n4)
n2 = Node.new(value: 7, next_node: n3)
n1 = Node.new(value: 7, next_node: n2)

linked_list1 = LinkedList.new(
  nodes: [n1, n2, n3, n4]
)

puts <<~HEREDOC
  Rotating #{linked_list1.to_s} by 2 places.
    :: #{linked_list1.rotate(2).to_s}

HEREDOC

n9 = Node.new(value: 5)
n8 = Node.new(value: 4, next_node: n9)
n7 = Node.new(value: 3, next_node: n8)
n6 = Node.new(value: 2, next_node: n7)
n5 = Node.new(value: 1, next_node: n6)

linked_list2 = LinkedList.new(
  nodes: [n5, n6, n7, n8, n9]
)

puts <<~HEREDOC
  Rotating #{linked_list2.to_s} by 3 places.
    :: #{linked_list2.rotate(3).to_s}
HEREDOC

Thoughts?

My ruby is a little rusty, but I can give you some general pointers.

First off, this isn't really a linked list. You use an array in LinkedList. That is not how a linked list works. A linked list does not maintain an array of all of its nodes. If it is singly linked (usually the forward direction, which is what you're doing with next_node) then LinkedList should only hold the head of the list. So, first thing's first let's fix that. You also shouldn't expose Node. Your constructor is also a little strange. I'd expect it to work like the builtin Array. Namely, you don't pass nodes. You pass a size and a value or a size and a block to Array.new or through a separate method (Array()) something that is  to_ary or to_a-able.

Again my ruby is rusty, but that would probably look something like this:

class LinkedList
  attr_reader :length
  alias_method :count, :length

  def initialize(length: 0, value: nil)
    @length = length

    (0..@length).reduce(Node.new) do |last_node, i|
        node = Node.new(if block_given? yield i else value end)
        last_node.next = node
        @head = node if i == 0
        node
    end
  end

  def first
    @head.?value
  end

  # Technically incomplete
  # if !block_given?, then it should return an enumerator
  def each
    node = @head
    while !node.nil?
        yield node.value
        node = node.next
    end
  end

  def to_a
    values = []
    each { |v| values << v }
    values
  end
end

def LinkedList(values)
    values = values.to_ary if values.respond_to?(:to_ary) else values.to_a end
    LinkedList.new(values.length) { |i| values[i] }
end

There may be a more elegant way to build the list from an arrayable (without needing to first construct the array), but it's not coming to me now. For completeness's sake, you probably want to also define the usual Enumerable methods (particularly each) so that you can test this. I provided first and each as examples of following the Enumerable convention.

Differentiating between rotate and rotate! is good. And your code reuse there is pretty nice (although given my qualms with the use of the array, I'm not a fan of rotate_nodes, more on that in a second). However, I'd recommend some further refactoring. It's unclear to me whether rotate is left or right. How about making it explicit: rotate_left, rotate_left!, rotate_right, and rotate_right!? And why not accept 0 or negative rotations? Let's say we defined right rotation. We could then define left rotation like this:

class LinkedList
  # ...

  def rotate_left(delta)
    rotate_right(-delta)
  end

  def rotate_left!(delta)
    rotate_right!(-delta)
  end

That feels much cleaner to me. I also wouldn't put the restriction that delta must be less than the length of your list (something you should definitely store by the way, don't rely on storing all the nodes in an array!). Instead, modulo the delta by the list length. So if the list has 5 elements and you rotate right by 7, that's the same as rotating right by 2. And if it isn't clear, rotating left by a negative amount should rotate right and vice versa.

Now, onto a more core problem. We'll start with your map in rotate_nodes:

def rotate_nodes(k)
  # ...

  @nodes.map do |node|
    n = @nodes.find_index(node)
    # ...
  end

find_index is O(n). There's no reason to do this. This ends up being O(n^2). Instead use @nodes.each_with_index.map { |node, index| # ... }. But, like I've mentioned before, you shouldn't have @nodes in the first place. Without it, you have some concerns to deal with regarding the differences between your bang rotate methods and non-bang rotate methods. Namely this:

Let's say you added a first= method so you could change the value of the first element in the list:

  def first=(value)
    if @head.nil?
      @head = Node.new(value)
      @length = 1
    else
      @head.value = value
    end
  end

This could be used like so:

> a = LinkedList([1, 2, 3])
> a.head = 4
> a.to_a
[4, 2, 3]

Now, what do you expect when we do the following:

> a = LinkedList([1, 2, 3])
> a.rotate_right(1).rotate_left(1).head = 4
> a.to_a

rotate_left and rotate_right aren't bang methods, so we don't expect to be able to change the underlying linked list. You demonstrate this understanding in how you initialize and return a new linked list for those methods. But, returning a new linked list isn't enough. rotate_right(1) is the equivalent of taking the tail of the linked list and placing it at the head. This can be done fairly trivially by moving some of the nexts around and then setting @head. But, if you share Nodes between LinkedLists, then that .head = 4 will modify the original list. I'm not sure that's the behavior you want. You'll have to think about the semantics you desire. It's clear that the bang methods should modify the existing LinkedList in place. But, it's less clear what an example like the one above should do. On the one hand, you could copy all of the nodes so that each Node belongs to only one LinkedList. However, this incurs a high memory penalty, especially if you didn't actually need the copy (say for some reason you just did a.rotate_right(10).head, you don't actually need the copy, this is equivalent to just getting the 11th element in the list). On the other hand, you could have Nodes belong to multiple LinkedLists. In this way a LinkedList behaves much more like a view than an independent collection. What I mean by this is my_list.rotate_right(10) isn't a new LinkedList really, it's just a different way of looking at my_link. Specifically, it's looking at my_list as if it started 11 elements in instead of where it's head currently is. I feel like the first approach doesn't make the copying obvious enough. You may want to avoid it entirely for something more explicit like requiring something like:

new_list = my_list.copy
new_list.rotate_right!(10)

If you prefer the second approach, I'd recommend making the Node's value immutable and severely limiting mutation on the lists. This is a space that functional programming languages have explored extensively. Mutation and multiple aliases often lead to disaster. It's best to pick one or the other.


Manage Sessions within a Singleton I am looking for advice if the below code is a good idea or not. If not, why? I am building a website which allows a user to purchase items. The user account and cart items will be stored in Session. My approach is to create a singleton class that will manage all the sessions. Is this approach good or bad?

public class SessionFactory
    {
        private static SessionFactory instance;
        private String _UserName;
        private long _CartId { get; set; }

        public Cart UserCart {

            get {

                if (HttpContext.Current.Session["Cart"] != null) {

                    return HttpContext.Current.Session["Cart"] as Cart;
                }

                return new Cart();
            }
            set {

                HttpContext.Current.Session["Cart"] = value;

            }
        }

        private SessionFactory(String username, long cartId)
        {
            this._UserName = username;
            this._CartId = cartId;
            // build Session objects here

        }

        public long CartId{

            get {

                return _CartId;
            }

        }

        public static SessionFactory Instance {

            get {

                if (instance == null) {

                    throw new Exception("Session was not created");
                }
                return instance;
            }

        }

        public static void Create(String userName, long cartId) {

            if (instance != null) {

                throw new Exception("Session Instance already created.");
            }

            instance = new SessionFactory(userName, cartId);
        }
    }

There are several remarks I could tell on your singleton implementation but since there's a far more better expert than me on these things, I'll reference his work. His name is Jon Skeet and he has an article on the singleton pattern which can be found here. Here are several points from the article that can help:

Characteristics:

A single constructor, which is private and parameterless. This prevents other classes from instantiating it (which would be a violation of the pattern). Note that it also prevents subclassing - if a singleton can be subclassed once, it can be subclassed twice, and if each of those subclasses can create an instance, the pattern is violated.
The class is sealed. This is unnecessary, strictly speaking, due to the above point, but may help the JIT to optimise things more.
A static variable which holds a reference to the single created instance, if any.
A public static means of getting the reference to the single created instance, creating one if necessary.

Basically your implementation of the pattern boils down to his first example:
public sealed class Singleton
{
    private static Singleton instance=null;

    private Singleton()
    {
    }

    public static Singleton Instance
    {
        get
        {
            if (instance==null)
            {
                instance = new Singleton();
            }
            return instance;
        }
    }
}

And this example is bad code. I stringly suggest you read the article to understand all this and implement the correct way of the singleton pattern.

Other points:
Take your following code:
public Cart UserCart
{
    get
    {
        if (HttpContext.Current.Session["Cart"] != null)
        {
            return HttpContext.Current.Session["Cart"] as Cart;
        }
        return new Cart();
    }
    set
    {
        HttpContext.Current.Session["Cart"] = value;
    }
}

This is wrong, the setter should be used to set the value of a Cart instance, certainly not to save a value in the HttpSessionState object. Also since this has all to do with Cart and not with the SessionFactory, maybe this belongs in the Cart class.

Why mix fieldnaming with and without the underscore and capitalize some and do not so with other? Prefixing them with an underscore is personal choice but fieldnames have camelCase, according to the capitalization conventions of Microsoft.


File Renaming with Ruby This is a minimal little utility that I've found useful while reorganizing my music and book libraries. Any issues you can see? Anything I could be doing better or more elegantly? Anyone wanna take a crack at the equivalent in Python/Perl?

Take 2:

#!/usr/bin/ruby

require 'optparse'
require 'pp'
require 'fileutils'

$options = {:sub => "", :downcase => nil}
OptionParser.new do |opts|
  opts.on('-r', '--regex REGEX', String,
          'Specify the regular expression to replace') {|reg| $options[:regex] = Regexp.new(reg)}
  opts.on('-s', '--sub SUBSTITUTE', String,
          'Specify what to replace the match with. By default, the empty string (so matches are stripped).') {|$options[:sub]|}
  opts.on('-d', '--downcase', 'If passed, all filenames will be downcased.'){|$options[:downcase]|}
end.parse!

usage unless ARGV.length > 0

def rename(str)
  ($options[:downcase] ?
   str.downcase : str).gsub($options[:regex], $options[:sub])
end

ARGV.each do |target|
  File.rename(target, rename(target))
end

Some observations. The space between #! and the path to binary is not needed. (It used to be for some arcane versions of Unix but not anymore AFAIK)

Secondly, I prefer to let env find ruby for portability.

#!/usr/bin/env ruby

require 'optparse'
require 'pp'
require 'fileutils'

The options are globals. So perhaps it is better to specify that explicitly.
This will help in modularization later.

$options = {:sub => "", :downcase => nil}

In the interests of people who have to use 80 char width terminals, I advice
an 80 cols limit (or near abouts) for lines :) .
I would also advice on verbose and dryrun flags to tell the user what is going to happen, and what is happening.

The stdlib example is to just call .parse! directly and I think that that style
is more rubyish than creating a new variable optparse.

OptionParser.new do |opts|
  opts.on('-r', '--regex REGEX', String,
     'Specify the regular expression to replace'){|$options[:regex]|}
  opts.on('-s', '--sub SUBSTITUTE', String,
     'Specify what to replace the match with. By default, the \
      empty string (so the matched patterns are \
      stripped).'){|$options[:sub]|}
  opts.on('-d', '--downcase', 'If passed, all filenames will be \
      downcased.'){|$options[:downcase]|}
end.parse!

I like programs to give me the usage if they are invoked with no input values.

usage unless ARGV.length > 0

I also think that the transformation of old name to new name should be a
separate method. And why embed a string inside a string and then transform
to a regular expression when you can directly transform that string to a regexp?

def transform(str)
  ($options[:downcase] ?
      str.downcase : str).gsub(Regexp.new $options[:regex], $options[:sub])
end

And given all these, I prefer a little more functional way of processing
The advantage is that you are restricting the IO to a very small portion.

ARGV.map{|target| [target, transform(target)]}.each do |old,new|
  File.rename(old, new)
end

Now, these comments are highly subjective. But given the length of
your snippet, I suppose that is justified? Because it obviously works as given.


Throwing the Exception But Using the Finally Block Does this piece of code make sense? The idea is to throw any exceptions that may occur but always run the finally block to close the streams.

private void streamToFile(HttpResponse transferResponse) throws Exception {
    OutputStream output = null;
    InputStream input = null;
    try {
        input = new BufferedInputStream(transferResponse.getEntity().getContent());
        output = new FileOutputStream(transferFile);
        final int BUFFER_SIZE = 1024*10;
        byte data[] = new byte[BUFFER_SIZE];
        int count;
        while ((count = input.read(data)) != -1) {
            output.write(data, 0, count);
            publishProgress(count);
            remainingBytes = remainingBytes - count;
        }
    } finally {
        input.close();
        output.close();
    }
}

You could use the closeQuietly methods of IOUtils (Apache Commons IO) (or similar ones) for input streams. It ignores exceptions and accepts null values.

Its implementation is just a few lines of code, so you can copy it directly to your Android application without the jar file:

public static void closeQuietly(Closeable closeable) {
    try {
        if (closeable != null) {
            closeable.close();
        }
    } catch (IOException ioe) {
        // ignore
    }
}

You could also create a close method for ouput streams:

public static void closeQuietly(OutputStream stream) throws IOException {
    if (stream != null) {
        stream.close();
    }
}

Both methods accept null values which means that you can call them with null, closeQuietly and close will handle that and you don't have to check null values in the finally block:

} finally {
    if (input != null) {
        closeQuietly(input);
    }
    close(output);
}

Instead of this the following is completely enough:

} finally {
    closeQuietly(input);
    close(output);
}

It's much simpler.

closeQuietly ignores exceptions which means that it catches them but don't do anything with them. Notice the empty catch block:

    } catch (IOException ioe) {
        // ignore
    }

It ensures that if closeable.close() throws an IOException the close(output) will run and it will close the output stream too.

I'm a little bit paranoid about this so I always put a logging statement to the catch block:

    } catch (IOException ioe) {
        logger.warn("Could not close the stream", ioe);
    }

To be honest, I've never seen this exception, so I think it's safe to left this block empty.

Ignoring IOExceptions of output streams could be dangerous: you might miss a bug and get a corrupted output file.


Apply classes to selected XHTML elements based on content I wish to take an XHTML source document (e.g. demo.in.xhtml below) and add class attributes to every td element whose content is one of the following six strings: A1, A2, B1, B2, C1, or C2. The corresponding classes will be mytable-a1, mytable-a2, mytable-b1, mytable-b2, mytable-c1, mytable-c2. Any other td elements in the source document should be kept as they are.

demo.xsl performs this as desired, but is not DRY: it uses a six-line xsl:template element per class attribute. How can I refactor this so as to remove the repetition, or generally make demo.xsl more concise and readable?

I am using the following command on Debian Jessie GNU/Linux to perform the transformation:

saxonb-xslt -s:demo.in.xhtml -xsl:demo.xsl -o:demo.out.xhtml

demo.in.xhtml

<?xml version="1.0" encoding="utf-8" ?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <title>Title</title>

            A1
            A2
            B1
            B2
            C1
            C2

demo.xsl

<?xml version="1.0"?>
<xsl:stylesheet
        xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
        version="2.0"
        xpath-default-namespace="http://www.w3.org/1999/xhtml">
    <xsl:output method="xml" version="1.0" encoding="utf-8" indent="yes"/>

    <xsl:template match="//td[.='A1']">
        <xsl:copy>
            <xsl:attribute name="class">mytable-a1</xsl:attribute>
            <xsl:apply-templates select="@*|node()"/>
        </xsl:copy>
    </xsl:template>

    <xsl:template match="//td[.='A2']">
        <xsl:copy>
            <xsl:attribute name="class">mytable-a2</xsl:attribute>
            <xsl:apply-templates select="@*|node()"/>
        </xsl:copy>
    </xsl:template>

    <xsl:template match="//td[.='B1']">
        <xsl:copy>
            <xsl:attribute name="class">mytable-b1</xsl:attribute>
            <xsl:apply-templates select="@*|node()"/>
        </xsl:copy>
    </xsl:template>

    <xsl:template match="//td[.='B2']">
        <xsl:copy>
            <xsl:attribute name="class">mytable-b2</xsl:attribute>
            <xsl:apply-templates select="@*|node()"/>
        </xsl:copy>
    </xsl:template>

    <xsl:template match="//td[.='C1']">
        <xsl:copy>
            <xsl:attribute name="class">mytable-c1</xsl:attribute>
            <xsl:apply-templates select="@*|node()"/>
        </xsl:copy>
    </xsl:template>

    <xsl:template match="//td[.='C2']">
        <xsl:copy>
            <xsl:attribute name="class">mytable-c2</xsl:attribute>
            <xsl:apply-templates select="@*|node()"/>
        </xsl:copy>
    </xsl:template>

    <xsl:template match="@*|node()">
        <xsl:copy>
            <xsl:apply-templates select="@*|node()"/>
        </xsl:copy>
    </xsl:template>

</xsl:stylesheet>

You could just create the class based on the value of the td.

Input (I added a td that should not be modified by the stylesheet.)

        <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
        <title>Title</title>

                A1
                A2
                B1
                B2
                C1
                C2
                ignore me

XSLT 2.0

<xsl:stylesheet version="2.0"
  xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
  xpath-default-namespace="http://www.w3.org/1999/xhtml">
  <xsl:output method="xml" version="1.0" encoding="utf-8" indent="yes"/>
  <xsl:strip-space elements="*"/>

  <xsl:param name="tds" select="('A2','B1','B2','C1','C2')"/>

  <xsl:template match="@*|node()">
    <xsl:copy>
      <xsl:apply-templates select="@*|node()"/>
    </xsl:copy>
  </xsl:template>

  <xsl:template match="td[normalize-space()=$tds]">
    <xsl:copy>
      <xsl:attribute name="class"
        select="concat('mytable-',lower-case(normalize-space()))"/>
      <xsl:apply-templates select="@*|node()"/>
    </xsl:copy>
  </xsl:template>

</xsl:stylesheet>

Output

      <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
      <title>Title</title>

            A1
            A2
            B1
            B2
            C1
            C2
            ignore me

Notes:

In my match, I didn't use the // abbreviated syntax (match="//td"); I just used match="td".
I used xsl:param instead of using the sequence ('A2','B1','B2','C1','C2') directly in the match so that it could be passed in at runtime. This would allow you to change which td elements were modified without changing the actual code.
Try to use a select attribute on xsl:attribute, even when outputting plain text (<xsl:attribute name="class" select="'mytable-c2'"/>). This prevents creating unnecessary RTF's (result tree fragments).
I use normalize-space() when creating the class to strip any leading/trailing whitespace in the text() node.
After creating the class attribute, we do an xsl:apply-templates on @*. If there is an existing class attribute, it will overwrite what we just created. If this is a possibility, I'd do an xsl:apply-templates on @*, then create the new class attribute (overwrites the existing class or you can concat the new and old), and then finally do the xsl:apply-templates on node().


Get value from dictionary given a list of nested keys I would like to get a deeply-nested value, for example {"a":{"b":{"c":"myValue"}} by providing the keys to traverse. I tried chaining together .get() but that didn't work for cases where some of the keys were missing—in those cases, I want to get None instead.

Could this solution be improved?

#!/usr/bin/env python
# encoding: utf-8

def nestedGet(d,p):
    if len(p) > 1:
        try:
            return nestedGet(d.get(p[0]),p[1:])
        except AttributeError:
            return None
    if len(p) == 1:
        try:
            return d.get(p[0])
        except AttributeError:
            return None

print nestedGet({"a":{"b":{"c":1}}},["a","b","c"]) #1
print nestedGet({"a":{"bar":{"c":1}}},["a","b","c"]) #None

Unless I'm missing something you'd like to implement, I would go for a simple loop instead of using recursion:

def nested_get(input_dict, nested_key):
    internal_dict_value = input_dict
    for k in nested_key:
        internal_dict_value = internal_dict_value.get(k, None)
        if internal_dict_value is None:
            return None
    return internal_dict_value

print(nested_get({"a":{"b":{"c":1}}},["a","b","c"])) #1
print(nested_get({"a":{"bar":{"c":1}}},["a","b","c"])) #None


Beginner's attempt for Project Euler problem 12 (highly divisible triangle number) I'm a couple weeks into learning Python and I'm working through the Project Euler problems. Problem 12 asks:

  What is the value of the first triangle number to have over five hundred divisors?

My problem is that my code takes far too long.

I had a list in there to store the factors but I replaced it with a counter variable to count the number of factors. It resets after looping through the divisors.

I'm not entirely sure but I feel like there is a much better way to cycle through the divisors?

How could I optimise this piece of code?

target = 100000000

counter = 0

for value in xrange(1, target + 1):
    x = (value * (value + 1))/2
    for divisor in xrange(1, x+1):
        product = x % divisor
        if product == 0:
            counter += 1

    if counter >= 500:
        print x
        sys.exit()

    else:
        counter = 0

I appreciate that similar questions have already been asked but I am hoping to gain useful 'personalised' advice from this community

Please excuse the vague variable names and the code in general...

Iteration, flow control, naming, and clarity

Your iteration is a bit clumsy.

It seems that target is an arbitrarily chosen number that you picked just to act as a sufficiently large second parameter to xrange().  If you want an unbounded counting loop, itertools.count(1) would be better.

Instead of else: counter = 0, you should just set counter = 0 in the right place — just before the inner loop.

Avoid using sys.exit() — it's like killing your program with a sledgehammer.  You should structure your code so that it terminates naturally.

from itertools import count

for value in count(1):
    x = (value * (value + 1))/2
    for divisor in xrange(1, x+1):
        product = x % divisor
        if product == 0:
            counter += 1

    if counter >= 500:
        break

print x

There is an idiom for counting items that meet some criterion: sum() with a generator expression.

from itertools import count

for value in count(1):
    x = (value * (value + 1)) / 2
    if 500 <= sum(1 for divisor in xrange(1, x + 1) if x % divisor == 0):
        print x
        break

As you noted yourself, your variables are poorly named.  product, in particular, should be called remainder.  counter is vague; divisor_count would be more helpful.

What you want to express is a loop over the triangle numbers.  For even greater clarity, I'd break that out into a triangle number generator function.

from itertools import count

def triangle_numbers():
    """Generator of triangle numbers, starting with 1, 1+2, 1+2+3, ..."""
    n = 0
    for i in count(1):
        n += i
        yield n

def divisor_count(n):
    """Count of the divisors of n"""
    return sum(1 for divisor in xrange(1, n + 1) if n % divisor == 0)

print next(t for t in triangle_numbers() if divisor_count(t) >= 500)

Mathematics

Note that \$\dfrac{n(n+1)}{2}\$ is a product of two coprimes.  As explained here, the divisors of such a product can be computed based on the divisors of each of its two known factors.

from itertools import count

def divisor_count(n):
    """Count of the divisors of n"""
    return sum(1 for divisor in xrange(1, n + 1) if n % divisor == 0)

for n in count(1):
    tn = n * (n + 1) // 2
    # n and (n + 1) are relatively prime.  Therefore, if n is even,
    # the factors of tn can be derived from the factors of n/2 and
    # the factors of n+1. If n is odd, then the factors of tn can be
    # derived from the factors of n and the factors of ((n+1)/2).
    tn_divisor_count = (
        divisor_count(n // 2) * divisor_count(n + 1) if n % 2 == 0 else
        divisor_count(n) * divisor_count((n + 1) // 2)
    )
    if tn_divisor_count >= 500:
        print tn
        break


C# basic dictionary (hash) implementation I have implemented a very basic hash data structure with basic set/retrieve methods.
A good example of use of this implementation would be a phone-book.
I have used as underneath storage structure an array consisting of linked lists.

Please a code review within these requirements.

public class MyHash
{
    private LinkedList<string>[] storage;

    public MyHash()
    {
        storage = new LinkedList<string>[26]; //english alphabet
    }

    private int HashFunction(string key)
    {
        //maps first character of name to a 0:25 index
        return key.ToLower().ToCharArray()[0] - 97;
    }

    public string this[string key]
    {
        get {
            var list = storage[HashFunction(key)];
            if (list != null)
            {
                foreach (var item in list)
                {
                    if (item.Split(':')[0] == key)
                        return item.Split(':')[1];
                }
            }
            return "Not found!";
        }
        set {
            if (storage[HashFunction(key)] == null)
                storage[HashFunction(key)] = new LinkedList<string>();
            storage[HashFunction(key)].AddLast(key + ":" + value);
        }
    }
}

There are several (major) problems with this class:

Keys that contain a : character cannot be used to look up their value, and values that contain a : character are only partially returned. Also, this class cannot handle null values (it returns an empty string instead). Don't mash keys and values together into a single value, using a key-value object instead (such as KeyValuePair<TKey, TValue>).
Keys that start with anything but [a-zA-Z] cause an IndexOutOfRangeException. This is not documented anywhere, and there's no obvious reason why such keys shouldn't be allowed (phone books aren't limited to countries with a Latin alphabet, after all). Why not use the modulus of the hash value and the number of buckets?
Non-existing keys produce a 'magic' value, which isn't documented anywhere. Moreover, a caller won't be able to distinguish between a non-existing key and an actual (valid) "Not found!" value. You may want to throw an exception instead. Alternately, you could provide a TryGetValue method instead.
Overwriting the value for an existing key is not possible. Your class will store both values (why?), but lookups will continue to return the first value. I would expect the new entry to replace the earlier one. If that's not the intended behavior, then I'd expect that to be documented, and an appropriate exception to be thrown.
Your hash function is poorly chosen. Ideally, you'll want to spread entries across buckets as evenly as possible, so each lookup will take roughly the same amount of time on average. If items are concentrated in only a few buckets (some characters are more frequently used than others) then lookups for such keys will be slower. Why not use key.GetHashCode() instead, or let the caller provide a hashing method in the constructor?
Using a fixed number of buckets means that lookup times are essentially \$O(n)\$, not \$O(1)\$. Locating a bucket takes constant time, while searching through multiple entries in a bucket takes linear time, so you'll want to limit the number of entries per bucket (ideally to 1). That requires some kind of resizing strategy.

Other minor issues:

By calling Split(':') and HashFunction multiple times you're doing the same work repeatedly. Just call them once and store the result in a variable.
Some names could be clarified a bit: MyHashTable or MyHashMap instead of MyHash, and buckets instead of storage.
Going for a generic implementation would not only be of more practical use, it would also have helped you to avoid some of the above problems: you wouldn't have been able to mash keys and values together or to use a sub-optimal leading-character 'hash'.


Optimizing rendering with a lot of transformations and save/restore I have implemented a renderer for shapes described with a L-System grammar. Implementation of that is not important. Basically you feed it an angle and some rules, and it spits out a string of characters that represents instructions. For example, "+" means turn right n radians and vice versa. The instructions will only be computed once.
The job of the method .draw is to read the instructions and draw it on a canvas continuously. The angle is always changing (however the instructions stay the same) and therefore this method will be called repeatedly:
LSystem.prototype.draw = function(ctx, x, y) {
    var self = this;
    var path = this.path;
    var table = {
        "F": function(ctx) {
            // draw forward
            ctx.moveTo(0, 0);
            ctx.lineTo(0, -5);
            ctx.translate(0, -5);
        },
        "+": function(ctx) {
            // turn right
            ctx.rotate(self.angle);
        },
        "-": function(ctx) {
            // turn left
            ctx.rotate(-self.angle);
        },
        "[": function(ctx) {
            // save location and rotation
            ctx.save();
        },
        "]": function(ctx) {
            // restore location and rotation
            ctx.restore();
        }
    };
    ctx.save();
    ctx.beginPath();
    ctx.moveTo(x + 0.5, y + 0.5);
    ctx.translate(x + 0.5, y + 0.5);
    for (var i = 0; i < path.length; i++) {
        // take a character from the path
        // and call the corresponding function (if exists)
        var char = path[i];
        var cmd = table[char];
        cmd && cmd(ctx);
    }
    ctx.stroke();
    ctx.restore();
};

A path (in my case) consists of over 6000 characters. It has no problem in drawing at 60FPS. However the main problem is that it uses a lot of CPU and I can hear the fans in my laptop starts spinning. This is not optimal and I'm looking for a way to solve this.
By looking at the profiling tool in Chrome, we can see that it doesn't like .rotate and .restore being called repeatedly. On a 16ms budget it already takes up half of it:

What is a good way of optimizing the rendering and minimize CPU usage? Would calculating the coordinates for the entire path first then pass it to .lineTo be a better choice? Keep in mind that the angle is different every time .draw is called, so the coordinates cannot be cached and reused.
A demo can be viewed here.

I see this was the second javascript question you posted here and it is unfortunate that there have been no responses in the nearly four years since then. Perhaps you have learned a few things about JavaScript since then.
Looking at the draw method I see that it stores a reference to this in a variable called self. This pattern happens frequently in JavaScript code but it is unnecessary. I see that some of the functions in table utilize self but that could be avoided by binding those functions to this with Function.bind().
It also appears that ctx is passed to each function as a parameter, which is also unnecessary because ctx is hoisted at the top-level and visible within each function as well.
The functions in table associated with keys "[" and "]" could be simply set to ctx.save.bind(ctx) and ctx.restore.bind(ctx), respectively. Furthermore, the functions in table associated with keys "+" and "-" can be set to ctx.rotate.bind(ctx, this.angle) and ctx.rotate.bind(ctx, -this.angle), respectively.
I'm not sure if those changes will be enough to reduce the CPU load. If not, then consider taking out function calls (i.e. calling table[char]) and using a series of if statements or switch statement in the for loop.
Demo
While it may not be much, see the demo below which hopefully provides some proof that the suggestions lead to a slight performance gain (see the drawB() method). I considered using jsPerf but that might be a bit of a stretch to use for this code.

var rules = [
  //["F", "F[+F]F[-F]F"]

  //["F", "F+F--F+F"]

  //["1", "1F1F"],
  //["0", "1F[+0][-0]"]

  //["F", "F+F-F-F+F"]

  ["F", "FF"],
  ["X", "F-[[X]+X]+F[+FX]-X"]
];
var angle = 10;
const times = [{
  y: [],
  line: {
    color: "blue",
    width: 4,
    shape: "line"
  },
    name: "Original draw()"
}, {
  y: [],
  line: {
    color: "green",
    width: 4,
    shape: "line"
  },
    name: "Simplified draw()"
}];

function LSystem(rules, angle, init, level) {
  this.rules = {};
  this.angle = angle * Math.PI / 180;
  this.parseRules(rules);
  this.path = this.generateLevel(init, level);
}
LSystem.prototype.parseRules = function(rules) {
  var self = this;
  rules.forEach(function(rule) {
    self.rules[rule[0]] = rule[1];
  });
};
LSystem.prototype.applyRule = function(input) {
  var output = "";
  for (var i = 0; i < input.length; i++) {
    var char = input[i];
    if (char in this.rules) {
      output += this.rules[char];
    } else {
      output += char;
    }
  }
  return output;
};
LSystem.prototype.generateLevel = function(input, level) {
  if (level == 0) return input;
  return this.generateLevel(this.applyRule(input), level - 1);
};
LSystem.prototype.draw = function(x, y) {
  var start = +new Date();
  var self = this;
  var path = this.path;
  var table = {
    "F": function(ctx) {
      ctx.moveTo(0, 0);
      ctx.lineTo(5, 0);
      ctx.translate(5, 0);
    },
    "+": function(ctx) {
      ctx.rotate(self.angle);
    },
    "-": function(ctx) {
      ctx.rotate(-self.angle);
    },
    "[": function(ctx) {
      ctx.save();
    },
    "]": function(ctx) {
      //ctx.stroke();
      ctx.restore();
      //ctx.beginPath();
    }
  };
  ctx.save();
  ctx.clearRect(0, 0, 500, 500);
  ctx.beginPath();
  ctx.moveTo(x + 0.5, y + 0.5);
  ctx.translate(x + 0.5, y + 0.5);
  for (var i = 0; i < path.length; i++) {
    var char = path[i];
    var cmd = table[char];
    cmd && cmd(ctx);
  }
  ctx.stroke();
  ctx.restore();
  const end = +new Date();
  times[0].y.push((end - start));
  if (times[0].y.length > 1) {
    avg.innerText = times[0].y.reduce((a, b) => a + b) / times[0].y.length;
  }
};
LSystem.prototype.drawB = function(x, y) {
  var start = +new Date();
  var table = {
    "F": function() {
      ctxB.moveTo(0, 0);
      ctxB.lineTo(5, 0);
      ctxB.translate(5, 0);
    },
    "+": ctxB.rotate.bind(ctxB, this.angle),
    "-": ctxB.rotate.bind(ctxB, -this.angle),
    "[": ctxB.save.bind(ctxB),
    "]": ctxB.restore.bind(ctxB)
  };
  ctxB.save();
  ctxB.clearRect(0, 0, 500, 500);
  ctxB.beginPath();
  ctxB.moveTo(x + 0.5, y + 0.5);
  ctxB.translate(x + 0.5, y + 0.5);
  for (var i = 0; i < this.path.length; i++) {
    var char = this.path[i];
    var cmd = table[char];
    cmd && cmd();
  }
  ctxB.stroke();
  ctxB.restore();
  const end = +new Date();
  times[1].y.push((end - start));
  if (times[1].y.length > 1) {
    avgB.innerText = times[1].y.reduce((a, b) => a + b) / times[1].y.length;
    avgB.classList.toggle('faster', parseFloat(avgB.innerText) < parseFloat(avg.innerText));
    avgB.classList.toggle('slower', parseFloat(avgB.innerText) > parseFloat(avg.innerText));
    avg.classList.toggle('faster', parseFloat(avg.innerText) < parseFloat(avgB.innerText));
    avg.classList.toggle('slower', parseFloat(avg.innerText) > parseFloat(avgB.innerText));
  }
}

// main
var ctx,
  ctxB,
  s = new LSystem(rules, angle, "X", 5),
  sB = new LSystem(rules, angle, "X", 5),
  output;
window.onload = function() {
  ctx = document.querySelector("canvas").getContext("2d");
  ctxB = document.getElementById("cnvB").getContext("2d");
  output = document.querySelector("pre");

  ctx.translate(50, 500);
  ctx.rotate(Math.PI / 180 * -65);
  ctx.translate(0, -250);

  ctxB.translate(50, 500);
  ctxB.rotate(Math.PI / 180 * -65);
  ctxB.translate(0, -250);

  //ctx.translate(250, 500);
  function d(t) {
    requestAnimationFrame(d);
    ctx.clearRect(0, 0, 500, 500);
    ctxB.clearRect(0, 0, 500, 500);
    s.draw(0, 250);
    sB.drawB(0, 250);

    s.angle = Math.PI / 180 * (25 + Math.sin(t / 1000));
    sB.angle = Math.PI / 180 * (25 + Math.sin(t / 1000));
  }
  output.textContent = s.path.length;
  requestAnimationFrame(d);
};
canvas {
  border: 1px solid black;
}

output.slower {
  background-color: red;
  color: white;
}

output.faster {
  background-color: green;
  color: white;
}

 Avg time for draw()	 <output id="avg"></output> ms

  Avg time for modified draw() 	<output id="avgB"></output> ms

<canvas width="500" height="500"></canvas>
<canvas width="500" height="500" id="cnvB"></canvas>

Result
Below is a chart comparing the elapsed times of 100 calls to the original draw method and the modified draw method.


Getting and posting data use PHP OOP and MySQLi I am not very experienced in PHP OOP. Moreover, when MySQLi comes with PHP OOP, it makes me more confused about using it in the most efficient way.

Connection class:

<?php
//connectionclass.php
class connection{
public $conn;
public $warn;
public $err;

function __construct(){
  $this->connect();
}

private function connect(){
  $this->conn = @ new mysqli('localhost', 'sever_user', 'user_password');
    if ($this->conn->connect_error) {
      $this->conn = FALSE;
      $this->warn = '<br />Failed to connect database! Please try again later';
     }
}

public function get_data($qry){
  $result = $this->conn->query($qry);
  if($result->num_rows>=1){
    return $result;
  }else{
    $this->err = $this->conn->error;
    return FALSE;
  }
  $result->free();
}

public function post_data($qry){
  $this->conn->query($qry);
  if($this->conn->affected_rows>=1){
    return TRUE;
  }else{
    $this->err = $this->conn->error;
    return FALSE;
  }
}
}
?>

Structure of a PHP page which uses a MySQL database to store and get data:

<?php
//login.php
include('/include/connectionclass.php');
$db = new connection();

$query = "SELECT * FROM USERS WHERE user_country='India'";
$data = $db->get_data($query);
  if($data){
    while($row=$data->fetch_assoc()){
      echo 'User Name: ':.$row["user_name"].' Age: '.$row["age"];
    }
  }
?>

My login.php uses a connection class to get data about users. All the things are running well, but one thing confused me. In connectionclass.php, $this->conn is itself an object as it calls new mysqli, so this is an object inside another object $db. Moreover, When I am using $data = $db->get_data($query);, a result set is created inside a $db object by method get_data, then this result set is copied into a variable $data inside the login.php page.

Actually, two instances of the same result sets/data sets are creating here, one inside the db object and one inside the login page.

Is this the right way to use MySQLi and PHP to get a dataset from a MySQL database? Will it use more memory and server resources when the dataset is larger (when have to get large amount of data for many users)?

If it is not right way, please explain your points and provide another solution which can be used efficiently for PHP OOP and MySQLi.

No, this isn't the right way to use OOP or MySQLi.

First of all, you only provide two methods to access your wrapped mysqli instance: one to get data, one to post data.

This hides a lot of useful and necessary functions. You can't even use prepared statements with your code, making it incredibly insecure.

You also didn't really gain anything with your functions. With your functions, your code is:

$query = "SELECT * FROM USERS WHERE user_country='India'";
$data = $db->get_data($query);
  if($data){
    while($row=$data->fetch_assoc()){
      echo 'User Name: ':.$row["user_name"].' Age: '.$row["age"];
    }
  }

Without your functions it would be:

$query = "SELECT * FROM USERS WHERE user_country='India'";
$data = $db->query($query);
  if($data){
    while($row=$data->fetch_assoc()){
      echo 'User Name: ':.$row["user_name"].' Age: '.$row["age"];
    }
$data->free();

It's essentially the same code, making your functions unnecessary.

If you do want to wrap mysqli - which may not actually be necessary in your case -, make sure that you:

provide all the needed functionalities.
actually improve mysqli (make it easier to use, add functionalities, ...).
keep the interface as close to that of mysqli as possible, to make it easier to use.

Instead of wrapping mysqli, it may make sense to aim for less abstract methods. For example, you may have a UserDAO, which may have methods such as selectByCountry.


Tree-traversal without recursion I wrote the following code for printing all the paths to the leaf nodes in a tree (NOT a binary tree ) without using recursion. I have used a stack and performed dfs on the tree. Whenever I reach a leaf node I pop all the elements in the stack right till the root so that the function starts over again from the root and prints a path to another leaf.

The tree that I have assumed in the program is as follows

                              0
                      1            2       3
              4   5      6              7     8

Explanation:

1, 2 and 3 are children of 0;
4, 5, 6 are children of 1;
7 and 8 are children of 3.

My code

//print paths in a tree without recursion
#include<iostream>
#include<vector>
#include<stack>
using namespace std;
struct node
{
       int data;
       vector<node*>children;
};
int visited[100];
stack <node*> st;
int count=0;
int n;
void printpath(node * root)                               //dfs starts here
{
     st.push(root);
     while(visited[0]==0)
     {
         node* v=st.top();
         if(visited[v->data]!=0)                 // leaf nodes or nodes whose all children
            st.pop();                            // are visited are only popped from the stack
         if(visited[v->data])
            continue;
         cout<<v->data<<" ";
         int flag=0;
         for(int i=0;i<v->children.size();i++)      /*check if any of the children have not been visited*/
         {
                 if(visited[v->children[i]->data]==0)
                 {
                      flag=flag||1;
                      st.push(v->children[i]);
                 }
                 else
                       flag=flag||0;
         }

         /*if this is the leaf node then mark it visited and
         along with other intermediate nodes whose all the
         children have been visited.this is continued till we
         reach root which has the value 0 in its data*/

         if(flag==0)
         {
                 visited[v->data]=1;
                  while(v->data!=0)
                  {
                     v=st.top();
                     int flag=0;
                     for(int i=0;i<v->children.size();i++)
                     {
                        if(visited[v->children[i]->data]==0)
                          flag=flag||1;
                       else
                         flag=flag||0;
                     }
                        if(flag==0 && v->children.size()!=0)
                          visited[v->data]=1;
                        if(v->data!=0)
                          st.pop();
                  }
                  cout<<endl;
         }
     }
}
main()
{
   n=8;
   for(int i=0;i<9;i++)
       visited[i]=0;
   node*root = new node();                         //tree construction
   root->data=0;
   node* d = new node();
   d->data=1;
   node* m = new node();
   m->data=2;
   node* n = new node();
   n->data=3;
   root->children.push_back(d);
   root->children.push_back(m);
   root->children.push_back(n);
   node* x = new node();
   x->data=4;
   node* y = new node();
   y->data=5;
   node* z = new node();
   z->data=6;
   d->children.push_back(x);
   d->children.push_back(y);
   d->children.push_back(z);
   node* o = new node();
   o->data=7;
   node* p = new node();
   p->data=8;
   n->children.push_back(o);
   n->children.push_back(p);
   printpath(root);
}

My algorithm seems really inefficient. Can anyone suggest some ways to improve it?

The output is:

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

In order that things appear in the code:

using namespace std;

This is unnecessary and generally bad practice.  I would ask about it if I saw it in an interview question, although it is not harmful in this case.

struct node
{
    node(int d) : data(d) {}
    int data;
    boost::ptr_vector<node> children;
};

Indentation is generally done in powers of two, and should in any case be consistent.  I'm switching to four-space indent everywhere, as you don't seem to be following any style.

This is also a great chance to use pointer containers.  Each node owns all of its children, so use something that will enforce that.  (In some cases, an std::vector of std::shared_ptrs would make more sense.)

You do not need any of the globals you've defined, and are definitely bad practice.  You should also strive to be more const-correct.

void printpath(node const* root)
{
    std::stack<node const*> path;
    std::set<node const*> visited;
    path.push(root);
    while (visited.find(root) != visited.end())
    {
        node const* v = path.top();
        if (visited.find(v) != visited.end())
        {
            path.pop();
            continue;
        }
        std::cout << v->data << " ";
        bool has_unvisited_child = false; // Use bools for booleans.
        // See standard comments on iterators vs indices and preincrement
        // vs postincrement.
        for (auto it = v->children.cbegin(); it != v->children.cend(); ++it)
        {
            if (visited.find(&*it) != visited.end())
            {
                 has_unvisited_child = true;
                 st.push(&*it);
            }
        }

        // Don't rely on the root node having value 0 in its data.
        // You were given a pointer to it, use that.  Your algorithm
        // should not depend on the data stored in the tree.

        // Avoids some nesting.
        if (!has_unvisited_child)
            continue;

        visited.insert(v);
        while(v != root)
        {
            v = st.top();
            // Avoid shadowing.
            bool unvisited_child = 0;
            // Splitting this off into a function may be meaningful,
            // or using some standard algorithm such as std::any_of
            for (auto it = v->children.cbegin(); it != v->children.cend(); ++it)
                unvisited_child |= visited.find(&*it) != visited.end();
            if (!unvisited_child && !v->children.empty())
                visited.insert(v);
            if (v != root)
                st.pop();
        }
        std::cout<<endl;
    }
}

// main must return int
int main()
{
    node* root = new node(0);
    node* d = new node(1);
    node* m = new node(2);
    node* n = new node(3);
    root->children.push_back(d);
    root->children.push_back(m);
    root->children.push_back(n);
    node* x = new node(4);
    node* y = new node(5);
    node* z = new node(6);
    d->children.push_back(x);
    d->children.push_back(y);
    d->children.push_back(z);
    node* o = new node(7);
    node* p = new node(8);
    n->children.push_back(o);
    n->children.push_back(p);
    printpath(root);
}

Now for the performance part:  My advice on that end is to not bother with the stack of next nodes and just keep track of the path:

void print_path(node const* root)
{
    std::vector<node const*> path{root};
    std::set<node const*> visited;
    while (!path.empty())
    {
        node const* n = path.back();
        if (std::all_of(n->children.cbegin(), n->children.cend(), [&](node const& a) { return visited.find(&a) != visited.end(); }))
        {
            if (n->children.empty()) {
                for (auto it = path.cbegin(); it != path.cend(); ++it)
                {
                    if (it != path.cbegin())
                        std::cout << ", ";
                    std::cout << (*it)->data;
                }
                std::cout << '\n';
            }
            visited.insert(n);
            path.pop_back();
        } else {
            path.push_back(&*std::find_if(n->children.cbegin(), n->children.cend(), [&](node const& a) { return visited.find(&a) == visited.end(); }));
        }
    }
}

I am not sure which is more efficient.


Make a recursive method faster How would you refactor this method to make it more performant?

If you have refactored the code you need to prove the performance gain in numbers to get the solution :)

public static T Find<T>(
    this IEnumerable<T> source,
    Func<T, IEnumerable<T>> childrenSelector,
    Predicate<T> condition)
{
    T t = default(T);
    foreach (T item in source)
    {
        if (condition(item))
        {
            return item;
        }

        t = childrenSelector(item).Find<T>(childrenSelector, condition);

        if (!Equals(t, default(T)))
        {
            return t;
        }
    }
    return t;
}

Do you really have a performance issue?

Optimizing a recursive function is usually done by performing tail call optimization, but this is not possible here since you have multiple branchs to explore at each function call.

I would thus not try to make the code faster, but the algorithm faster. If searching in a tree actually slows your code, that means you need to change the way it works so that you know which branch to explore at each function call and having O(log n) complexity instead of O(n). There are tons of trees which have nice properties likes this: B-trees, red-black trees, splay trees and so on. You can also consider using a hash table.