Convert query string into object in JavaScript Suppose I have this query and want to convert into an object

  a1.b2.article1=0&a2.article2=1&c3!article3=0&article16-abc=2

output for this should be

article1: 'DRAFT',
article2: 'REVISION',
article3: 'DRAFT',
article16: 'READY'

Please review my code. Is this the right way to do perform this task?

let a = "a1.b2.article1=0&a2.article2=1&c3!article3=0&article16-abc=2";
let con = {'0':'DRAFT', '1':'REVISION', '2':'READY'}
let obj = {};
a = a.replace(/-abc/g, "");
a = a.replace(/[.!]/g, "&");
a = a.split("&").forEach((v, i) => {
    let b = v.split("=");
    if (b.length > 1) {
       Object.assign(obj,{[b[0]]:con[b[1]]})
    }
});

I'm wondering if there any optimized way to do this.

I suggest you use the standard query string syntax to avoid future problems. Because the weirder this query string gets, the more cases you'll have to add to this piece of code. And when a case that's not covered in your code appears, it will unexpectedly break your code.

Also, this used to be the way to parse query strings (i.e. split by &, loop through the items, split by =). Now, there is a built-in API, URLSearchParams, that parses query strings for you. It exists for both browser JS as well as NodeJS. It exposes several methods as well as methods that return iterables, which allow you to loop through the items.


Create a view with a thick inner border that has a gradient InnerGradientBorderView is a subclass of UIView that has a thick border with a gradient. I have tried to replicate the Zoom meeting UI. They apply this kind of a border during screenshare.

import UIKit

class InnerGradientBorderView: UIView {

    var gradientLayers = [CAGradientLayer]()

    override init(frame: CGRect) {
        super.init(frame: frame)
        backgroundColor = UIColor.clear
    }

    required init?(coder aDecoder: NSCoder) {
        super.init(coder: aDecoder)
        backgroundColor = UIColor.clear
    }

    override func layoutSubviews() {
        super.layoutSubviews()

        for layer in gradientLayers {
            layer.removeFromSuperlayer()
        }

        addGradientInnerBorder(width: 8, color: UIColor.orange)
    }

    func addGradientInnerBorder(width: CGFloat, color: UIColor) {

        // Setup
        let topLeftO = CGPoint(x: 0, y: 0)
        let topLeftI = CGPoint(x: width, y: width)

        let topRightO = CGPoint(x: frame.width, y: 0)
        let topRightI = CGPoint(x: frame.width - width, y: width)

        let bottomLeftO = CGPoint(x: 0, y: frame.height)
        let bottomLeftI = CGPoint(x: width, y: frame.height - width)

        let bottomRightO = CGPoint(x: frame.width, y: frame.height)
        let bottomRightI = CGPoint(x: frame.width - width, y: frame.height - width)

        // Top
        let topPoints = [topLeftO, topLeftI, topRightI, topRightO, topLeftO]
        let topGradientPoints = [CGPoint(x: 0, y: 0), CGPoint(x: 0, y: 1)]
        gradientLayers.append(addGradientToBeizerPath(path: addClosedPathForPoints(points: topPoints), color: color, gradientPoints: topGradientPoints))

        // Left
        let leftPoints = [topLeftO, topLeftI, bottomLeftI, bottomLeftO, topLeftO]
        let leftGradientPoints = [CGPoint(x: 0, y: 0), CGPoint(x: 1, y: 0)]
        gradientLayers.append(addGradientToBeizerPath(path: addClosedPathForPoints(points: leftPoints), color: color, gradientPoints: leftGradientPoints))

        // Right
        let rightPoints = [topRightO, topRightI, bottomRightI, bottomRightO, topRightO]
        let rightGradientPoints = [CGPoint(x: 1, y: 0), CGPoint(x: 0, y: 0)]
        gradientLayers.append(addGradientToBeizerPath(path: addClosedPathForPoints(points: rightPoints), color: color, gradientPoints: rightGradientPoints))

        // Bottom
        let bottomPoints = [bottomLeftO, bottomLeftI, bottomRightI, bottomRightO, bottomLeftO]
        let bottomGradientPoints = [CGPoint(x: 0, y: 1), CGPoint(x: 0, y: 0)]
        gradientLayers.append(addGradientToBeizerPath(path: addClosedPathForPoints(points: bottomPoints), color: color, gradientPoints: bottomGradientPoints))
    }

    func addClosedPathForPoints(points: [CGPoint]) -> UIBezierPath? {
        guard points.count == 5 else { return nil }

        let path = UIBezierPath()
        path.move(to: points[0])

        path.addLine(to: points[1])
        path.addLine(to: points[2])
        path.addLine(to: points[3])
        path.addLine(to: points[4])

        path.close()

        return path
    }

    func addGradientToBeizerPath(path: UIBezierPath?, color: UIColor, gradientPoints: [CGPoint]) -> CAGradientLayer {
        guard let path = path, gradientPoints.count == 2 else { return CAGradientLayer() }

        let gradient = CAGradientLayer()
        gradient.frame = path.bounds
        gradient.bounds = path.bounds // This fixes the code.
        gradient.colors = [color.cgColor, UIColor.white.cgColor]
        gradient.startPoint = gradientPoints[0]
        gradient.endPoint = gradientPoints[1]

        let shapeMask = CAShapeLayer()
        shapeMask.path = path.cgPath
        gradient.mask = shapeMask

        self.layer.insertSublayer(gradient, at: 0)

        return gradient
    }
}

Nitpick: It is "Bezier", not "Beizer".

Setting the background color

  backgroundColor = UIColor.clear

can be shortened to

backgroundColor = .clear

because the type is inferred from the context. But why do you force the
view to be fully transparent at all? Shouldn't that be left to the
designer to configure in the interface builder? Unless there is a special
reason I would just remove that from the init methods.

Your view has three characteristic properties: the outer and inner
gradient color, and the width of the border. The values of those
properties are spread in the code: width and outer gradient color are
arguments in

addGradientInnerBorder(width: 8, color: UIColor.orange)

and passed around, while the inner color is directly set in

gradient.colors = [color.cgColor, UIColor.white.cgColor]

I would define properties instead:

class InnerGradientBorderView: UIView {

    let borderWidth: CGFloat = 8.0
    let outerColor = UIColor.orange
    let innerColor = UIColor.white

    // ...
}

so that they are easily identified and modified.
Even better: Use @IBInspectable and @IBDesignable

@IBDesignable
class InnerGradientBorderView: UIView {

    @IBInspectable var borderWidth: CGFloat = 8.0 {
        didSet {
            setNeedsLayout()
        }
    }
    @IBInspectable var outerColor: UIColor = .orange {
        didSet {
            setNeedsLayout()
        }
    }
    @IBInspectable var innerColor: UIColor = .white {
        didSet {
            setNeedsLayout()
        }
    }

    // ...
}

so that they can be configured directly in the interface builder,
with a live preview (compare IBInspectable / IBDesignable:

Closing a path automatically adds a line segment from the last point
to the first point of the current path segment, i.e. the final
addLine(to:) in addClosedPathForPoints() is not needed, and it
suffices to pass 4 points to the function.

Also I would not make the return value optional. Passing a wrong number
of arguments is a programming error and should be detect early in the
development instead of silently ignored:

func addClosedPathForPoints(points: [CGPoint]) -> UIBezierPath {
    assert(points.count == 4)

    let path = UIBezierPath()
    path.move(to: points[0])
    path.addLine(to: points[1])
    path.addLine(to: points[2])
    path.addLine(to: points[3])
    path.close()

    return path
}

Instead of completely re-creating all gradient layers in
layoutSubviews() it might be more efficient to create them once
(when the view is initialized), and only update the frame and mask layer
when the layout changes.

Then your class would look like this:

@IBDesignable
class InnerGradientBorderView: UIView {

    @IBInspectable var borderWidth: CGFloat = 8.0 {
        didSet {
            setNeedsLayout()
        }
    }
    @IBInspectable var outerColor: UIColor = .orange {
        didSet {
            createGradients()
            setNeedsLayout()
        }
    }
    @IBInspectable var innerColor: UIColor = .white {
        didSet {
            createGradients()
            setNeedsLayout()
        }
    }

    var topGradient, leftGradient, rightGradient, bottomGradient: CAGradientLayer!

    override init(frame: CGRect) {
        super.init(frame: frame)
        createGradients()
    }

    required init?(coder aDecoder: NSCoder) {
        super.init(coder: aDecoder)
        createGradients()
    }

    override func layoutSubviews() {
        super.layoutSubviews()
        layoutGradients()
    }

    func createGradients() {
        topGradient = createGradient(gradientPoints: [CGPoint(x: 0, y: 0), CGPoint(x: 0, y: 1)])
        leftGradient = createGradient(gradientPoints: [CGPoint(x: 0, y: 0), CGPoint(x: 1, y: 0)])
        rightGradient = createGradient(gradientPoints: [CGPoint(x: 1, y: 0), CGPoint(x: 0, y: 0)])
        bottomGradient = createGradient(gradientPoints: [CGPoint(x: 0, y: 1), CGPoint(x: 0, y: 0)])

        layer.addSublayer(topGradient)
        layer.addSublayer(leftGradient)
        layer.addSublayer(rightGradient)
        layer.addSublayer(bottomGradient)
    }

    func createGradient(gradientPoints: [CGPoint]) -> CAGradientLayer {
        let gradient = CAGradientLayer()
        gradient.colors = [outerColor.cgColor, innerColor.cgColor]
        gradient.startPoint = gradientPoints[0]
        gradient.endPoint = gradientPoints[1]
        return gradient
    }

    func layoutGradients() {
        let topLeftO = CGPoint(x: 0, y: 0)
        let topLeftI = CGPoint(x: borderWidth, y: borderWidth)

        let topRightO = CGPoint(x: frame.width, y: 0)
        let topRightI = CGPoint(x: frame.width - borderWidth, y: borderWidth)

        let bottomLeftO = CGPoint(x: 0, y: frame.height)
        let bottomLeftI = CGPoint(x: borderWidth, y: frame.height - borderWidth)

        let bottomRightO = CGPoint(x: frame.width, y: frame.height)
        let bottomRightI = CGPoint(x: frame.width - borderWidth, y: frame.height - borderWidth)

        layoutGradient(gradient: topGradient, points: [topLeftO, topLeftI, topRightI, topRightO])
        layoutGradient(gradient: leftGradient, points: [topLeftO, topLeftI, bottomLeftI, bottomLeftO])
        layoutGradient(gradient: rightGradient, points: [topRightO, topRightI, bottomRightI, bottomRightO])
        layoutGradient(gradient: bottomGradient, points: [bottomLeftO, bottomLeftI, bottomRightI, bottomRightO])
    }

    func layoutGradient(gradient: CAGradientLayer, points: [CGPoint]) {
        let path = UIBezierPath()
        path.move(to: points[0])
        path.addLine(to: points[1])
        path.addLine(to: points[2])
        path.addLine(to: points[3])
        path.close()

        gradient.frame = path.bounds
        gradient.bounds = path.bounds

        if let shapeMask = gradient.mask as? CAShapeLayer {
            // Update mask.
            shapeMask.path = path.cgPath
        } else {
            // Create and set mask.
            let shapeMask = CAShapeLayer()
            shapeMask.path = path.cgPath
            gradient.mask = shapeMask
        }
    }
}

But this is not yet perfect! The problem is that the sublayers do not
animate correctly if the view's position or size changes, e.g.
when rotating the device.
You'll notice that clearly in the iOS Simulator if you activate
"Debug -> Slow Animations":

The solution is to embed the call to layoutGradients()
into a Core Animation transaction so that the frame and mask changes are
synchronized with the view layer's animation, see for example

How do you animate the sublayers of the layer of a UIView during a UIView animation? on Stack Overflow.

Applied to our situation (and translated to Swift) that becomes:

    override func layoutSubviews() {
        super.layoutSubviews()

        if let animation = layer.animation(forKey: "position") {
            CATransaction.begin()
            CATransaction.setAnimationDuration(animation.duration)
            CATransaction.setAnimationTimingFunction(animation.timingFunction)

            let frameAnimation = CABasicAnimation(keyPath: "frame")
            topGradient.add(frameAnimation, forKey: "frame")
            leftGradient.add(frameAnimation, forKey: "frame")
            rightGradient.add(frameAnimation, forKey: "frame")
            bottomGradient.add(frameAnimation, forKey: "frame")

            let pathAnimation = CABasicAnimation(keyPath: "path")
            topGradient.mask?.add(pathAnimation, forKey: "path")
            leftGradient.mask?.add(pathAnimation, forKey: "path")
            rightGradient.mask?.add(pathAnimation, forKey: "path")
            bottomGradient.mask?.add(pathAnimation, forKey: "path")

            layoutGradients()
            CATransaction.commit()
        } else {
            layoutGradients()
        }
    }

And now it animates perfectly:


Products excluding each element of the array Given an array of integers, return a new array such that each element at index i of the new array is the product of all the numbers in the original array except the one at i.

For example, if our input was [1, 2, 3, 4, 5], the expected output would be [120, 60, 40, 30, 24]. If our input was [3, 2, 1], the expected output would be [2, 3, 6].

Is there a better solution for this problem?

  Note: Division is not allowed.

public class DailyCodingProblem2 {
    public static void main(String args[]) {
        int[] arr = { 1, 2, 3, 4, 5 };
        int[] ans = solution(arr, arr.length);
        System.out.println(Arrays.toString(ans));

        arr = new int[] { 3, 2, 1 };
        ans = solution(arr, arr.length);
        System.out.println(Arrays.toString(ans));
    }

    private static int[] solution(int[] arr, int n) {
        int[] left = new int[n];
        int[] right = new int[n];
        int[] res = new int[n];
        left[0] = 1;
        right[n - 1] = 1;

        for (int i = 1; i < n; i++) {
            left[i] = arr[i - 1] * left[i - 1];
        }

        for (int i = n - 2; i >= 0; i--) {
            right[i] = arr[i + 1] * right[i + 1];
        }
        for (int i = 0; i < n; i++) {
            res[i] = left[i] * right[i];
        }
        return res;
    }
}

Time complexity-wise, I think not. You're required to 'visit' all numbers, and your solution is O(n), so that can't be improved.

Code clarity could be improved, as it's not very obvious what the intention is. Some comments would help that.
I think shifting indices by 1 might make it clearer (left[i+1] = arr[i] * left[i]), but then maybe not because it'd mess up the last loop.

Have you explored different algorithms? I wonder if straightforward memoization makes a very clear solution for this.


Project Euler problem 530 - GCD function is inefficient Here is a description of Project Euler problem 530:

  Every divisor \$d\$ of a number \$n\$ has a complementary divisor \$n/d\$.

  Let \$f(n)\$ be the sum of the greatest common divisor of \$d\$ and \$n/d\$ over all positive divisors \$d\$ of \$n\$, that is \$\displaystyle f(n)=\sum_{d/n}\gcd\left(d, \frac{n}{d}\right)\$.

  Let \$F\$ be the summatory function of \$f\$, that is \$\displaystyle F(k) = \sum_{n=1}^{k}f(n)\$.

  You are given that \$F(10)=32\$ and \$F(1000)=12776\$.

  Find \$F(10^{15})\$.

Yesterday, I asked a question here: Project Euler Problem 530: sum of sum of greatest common divisors

I have improved my solution, which is now as follows:

import java.math.BigInteger;

public class problem530 {

public static void main(String[] args) {

    double start = System.nanoTime();

    BigInteger result = BigInteger.ZERO;
    long target = 100000;

    for (long i = 1; i*i <=  target; i++){
                for(long j = i; i*j <= target; j++) {
                    if (i != j) {
                    result = result.add(BigInteger.valueOf((GCD(i, j))*2));
                }
                    else {
                        result = result.add(BigInteger.valueOf(i));
                    }
                }
            }

    System.out.println(result);

    double duration = (System.nanoTime() - start)/1000000000;

    System.out.println("Your code took " + duration + " seconds to execute.");

}

public static long GCD (long p, long q) {
    if (p == 0) return q;
    else return GCD (q%p,p);
}

This solution uses a simplified version of the summation (it avoids factorisation).

However, the GCD function I'm using is computationally expensive and causes the code to be incapable of finding F(x) for large values of x.

Currently:

It ran F(10^1) = 32 in 0.0027 seconds.
It ran F(10^3) = 12766 in 0.0042 seconds.
It ran F(10^5) = 2907546 in 0.091 seconds.
It ran F(10^7) = 518387613 in 8.034 seconds.
It ran F(10^8) = 6563728768 in 97.368 seconds.

Clearly, to find F(10^15) would be practically impossible.

I think the solution might be to memoize the GCD function, or find some way to make it linearly more difficult, as opposed to exponentially, which is now happening.

Suggestions?

EDIT: I am not using the summation from the question. I'm using a simplified version of it.

This was suggested in another post on codereview, and it works, as long as I account for symmetry (the multiply by 2 part of the code), except when it's a perfect square.

Okay, I've taken another look at the problem, and I have another refinement. It's not a complete solution, though; I suspect that Project Euler 530 is expecting you to have some number-theoretical insight, rather than just a lot of refinements on top of brute force.

The brute-force solution is indeed something like this (in C++ because I like C++ better than Java):

template<typename T>
T f(T n)
{
    T sum = 0;
    const T sqrtn = sqrt(n);
    for (T i = 1; i <= sqrtn; ++i) {
        if (n % i) continue;
        sum += gcd(i, n/i);
        if (i*i != n) sum += gcd(i, n/i);
    }
    return sum;
}

template<typename T>
T F(T k)
{
    T sum = 0;
    for (T i = 1; i <= k; ++i) {
        sum += f(i);
    }
    return sum;
}

By the way, gcd itself is going to be very fast no matter what, as long as you're compiling with optimizations turned on. For my C++ testing, I just used #define gcd std::__gcd, which turned out to be just a hair faster than the naive %-based algorithm and much faster than a "Binary GCD" algorithm I copied from Wikipedia.

This version performs F(1e6) in 7.7 seconds.

The answer at Optimize finding GCD of all pairs of divisors provides a very good algorithmic refinement. It reverses the loops so that you have something like this:

template<typename T>
T F(T k)
{
    T sum = 0;
    const T sqrtk = sqrt(k);
    for (T d = 1; d <= sqrtk; ++d) {
        const T k_over_d = k/d;
        sum += d;
        for (T n = d+1; n <= k_over_d; ++n) {
            const T g = gcd(n, d);
            sum += 2*g;
        }
    }
    return sum;
}

This is basically what you've implemented, except that you compute i*i sqrt(n) times instead of just computing sqrt(n) once, and so on. In other words, you're deliberately pessimizing your code, which is one reason it's hard to identify the real bottlenecks. When doing Project Euler–type speed challenges, you should always eliminate all the inessential inefficiencies that you can, so that if any inefficiencies remain, they'll be essential to your algorithm (and hopefully point the way to a better algorithm).

This version performs F(1e7) in 4.0 seconds.

So, looking at that version, the thing that jumped out at me was that we're computing gcd(n,d) in the inner loop... over a lot of values of n, like, more than d different values of n. And gcd(n,d) is never bigger than d. So we're summing up the same value of gcd(n,d) multiple times (for n equal to n1, and n1+d, and n1+d+d, and so on). So, I thought to myself, let's split that inner loop into d inner loops and compute 1/dth as many GCDs!

template<typename T>
T F(T k)
{
    T sum = 0;
    const T sqrtk = sqrt(k);
    for (T d = 1; d <= sqrtk; ++d) {
        const T k_over_d = k/d;
        for (T n_mod_d = 0; n_mod_d < d; ++n_mod_d) {
            const T g = gcd(n_mod_d, d);
            for (T n = d + n_mod_d; n <= k_over_d; n += d) {
                sum += 2*g;
            }
        }
        sum -= d;
    }
    return sum;
}

This version performs F(1e8) in 6.5 seconds.

I thought Clang (my C++ compiler of choice) was probably smart enough to optimize that inner-inner loop (the one that just adds 2*g a known number of times). Indeed, manually optimizing it to

            const T g = gcd(n_mod_d, d);
            const T iterations = (k_over_d - n_mod_d) / d;
            sum += 2*g*iterations;

didn't produce any further speedup, indicating that the compiler was already doing that optimization for me.

We can rearrange some of the obvious commutative operations, e.g. instead of subtracting d from sum for each d from 1 to sqrtk, we can just subtract (sqrtk+1) * sqrtk / 2 once... that kind of thing. That doesn't change the shape of the nested loops, so it doesn't affect the runtime of the code; but it makes the code a bit shorter and removes some of the inessential inefficiency so that we can see more clearly the essential inefficiency we're still trying to get rid of.

template<typename T>
T F(T k)
{
    T sum = 0;
    const T sqrtk = sqrt(k);
    for (T d = 1; d <= sqrtk; ++d) {
        const T k_over_d = k/d;
        for (T i = 0; i < d; ++i) {
            sum += gcd(i, d) * ((k_over_d - i) / d);
        }
    }
    return (2 * sum) - ((sqrtk + 1) * sqrtk / 2);
}

Aha! Given that i < d, that subexpression ((k_over_d - i) / d) looks suspicious. It can only ever take on one of two values: either int(k_over_d / d) if i <= k_over_d % d, or that-minus-1 otherwise. So we hoist that multiplication out of the loop, and finally we're back to a couple of loops over gcds.

I got another tiny speedup by using a lookup table for gcd(x,y) where x <= y <= 8192; that version performed F(1e8) in 5.0 seconds.

However, you don't need 10x speedups; you need 10000000x speedups, in order to perform F(1e15) in a reasonable amount of time. This implies that there's another algorithmic, number-theoretic, approach that will be the actual solution Project Euler is looking for.

You might think that another possible solution would be to split up the work and use multithreading to get the answer in parallel. But even supposing that you have a 16-core CPU at your disposal, that's only a 16x speedup at most. To beat Project Euler, you'll need another 625000x speedup from somewhere... and the only place to get that is algorithmic leaps.

You might also think that you could speed up the current algorithm by micro-optimizing gcd — by memoizing, or using a lookup table (as I did), or something like that. But notice that with the current algorithm, you'll eventually have to perform gcd(i,d) for each d up to sqrt(1e15), i.e., 31.6 million. Unless your proposed micro-optimization can deal with that many different inputs, it probably won't help.

EDITED TO ADD: I realized this morning that there's yet another rearrangement of the terms that might help. You could look for the sum
\$\sum_{x,y,g} g \cdot\delta(\text{gcd}(x,y)=1 \text{ and } xyg^2 < k)\$.
There happens to be a handy O(k) algorithm for generating all pairs (x,y) < k such that gcd(x,y)==1. Unfortunately, O(k) in your case is O(1e15), which is simply too slow. So this doesn't help. What you need is a way to count those coprime pairs in O(something small), combined with a way to use that information to compute the sum you're looking for.

(And if we had any eggs, we'd have ham and eggs... if we had any ham.)


Counting Monday and Friday from a Date Range How can I get Monday and Friday from a date range (start and end date)?

Example:

startdate - 03/24/2014
enddate - 04/01/2014

Result should be:

Monday - 2
Friday - 2

I tried this code:

Monday = CountDays(DayOfWeek.Monday, startDate, endDate);
Friday = CountDays(DayOfWeek.Friday, startDate, endDate);

static int CountDays(DayOfWeek day, DateTime start, DateTime end)
{
    TimeSpan ts = end - start;
    int count = (int)Math.Floor(ts.TotalDays / 7);
    int remainder = (int)(ts.TotalDays % 7);
    int sinceLastDay = (int)(end.DayOfWeek - day);
    if (sinceLastDay < 0) sinceLastDay += 7;

    if (remainder >= sinceLastDay) count++;

    return count > 0 ? count : 0;
}

And this code works!  Is there any other way to simplify this code?

Your code is already good enough.

static int CountDays(DayOfWeek day, DateTime start, DateTime end)
{
    start = start.Date.AddDays((7 + day - start.DayOfWeek) % 7);
    if (end < start)
        return 0;
    else
        return ((int)(end - start).TotalDays) / 7 + 1;
}


Rock Paper Scissors C++ Game I just started to learn C++ and I decided to make a Rock Paper Scissors game. It works fine but I would like to know how can I improve it further.
In particular I couldn't find an efficient way to implement a play again feature. Thanks in advance.

#include <cstdlib>
#include <ctime>
#include <iostream>
#include <string>

using namespace std;

int main() {

srand(static_cast<unsigned int>(time(NULL)));

string userChoice;
int computerNumber = rand() % 3 + 1;
string computerChoice;
string userName;

if (computerNumber == 1) {
    computerChoice = "rock";
} else if (computerNumber == 2) {
    computerChoice = "paper";
} else {
    computerChoice = "scissors";
}

std::cout << "Welcome to rock, paper, scissors! What is your name?" << endl;
getline(cin, userName);
std::cout << "Hello " << userName << "! What do you choose?" << endl;
getline (cin,userChoice);

for (int i=0;i<userChoice.length();i++){
    userChoice[i]=tolower(userChoice[i]);
}

while (userChoice != "rock" && userChoice != "paper" && userChoice != "scissors") {
    std::cout << "Sorry! Didn't get that. Please enter again." << endl;
    getline(cin, userChoice);
    for (int i=0;i<userChoice.length();i++){
        userChoice[i]=tolower(userChoice[i]);
    }
}

std::cout << "Press enter to continue..." << endl;
std::cin.ignore();

if(computerChoice == "rock" && userChoice == "rock") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "rock" && userChoice == "scissors") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else if (computerChoice == "paper" && userChoice == "paper") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "paper" && userChoice == "rock") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else if (computerChoice == "scissors" && userChoice == "scissors") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "scissors" && userChoice == "paper") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else {
    std::cout << "Congrats! You won!" << endl;
}

return 0;
}

Indententation

Normally code is indented between { and }

int main()
{
    // Start here
}

Namespace

Never Do this.

using namespace std;

If you had read any other C++ review on this board it would have told you not to do this. See: Why is “using namespace std” in C++ considered bad practice?.

There is a reason that the standard libraries are called std and not standard. Its to make prefixing them easy.

Rand

Though your use of srand() and rand() is perfectly valid. This version of the random number generated is now obsolete. You should look up how to use the new random number functionality provided by C++11.

https://stackoverflow.com/a/14009667/14065
https://stackoverflow.com/a/19666713/14065

Note:

int computerNumber = rand() % 3 + 1;

This is slightly biased against 3. Assume RAND_MAX is 32768.

If you rolled 32768 times. Then your distribution would be.

1 10923/32768
2 10923/32768
3 10922/32768  <- Notice this is one smaller.

Invalid:

} else {
    std::cout << "Congrats! You won!" << endl;
}

Do you really want to have an invalid choice. You have already enumerated all the valid choices. If you get to this point there is a serious error in your code. So you need to indicate this (so you can catch it in testing).

else
{
    throw std::runtime_error("Error");
}

Data Driven Programming

Rather than write code to decide the winner:

if(computerChoice == "rock" && userChoice == "rock") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "rock" && userChoice == "scissors") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else if (computerChoice == "paper" && userChoice == "paper") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "paper" && userChoice == "rock") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else if (computerChoice == "scissors" && userChoice == "scissors") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "scissors" && userChoice == "paper") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else {
    std::cout << "Congrats! You won!" << endl;
}

Use a data structure. Assume you translate the choices into a number for each user.

0 -> rock
1 -> paper
2 -> scissor

// This array decides if who is the winner.
// 0 => Draw.
// 1 => Player 1
// 2 => Player 2
int winner[3][3] = {{0, 2, 1},
                    {1, 0, 2},
                    {2, 1, 0}};

// Then to decide the winner.
int theWinner = winner[player1Choice][player2Choice];

Advice

  In particular I couldn't find an efficient way to implement a play again feature.

To make things like retrying the code you need to start using functions so that you can logically compartmentalize your code.

int main()
{
    std::string name = getPlayerName();
    do
    {
        int player1 = getComputerMove();
        int player2 = getPlayerMove();
        displayWinner(player1, player2);
    }
    while(doesPlayerWantToContinue());
}


Demonstration of Find Vowel Square Have the function VowelSquare(strArr) take the strArr parameter being
  passed which will be a 2D matrix of some arbitrary size filled with
  letters from the alphabet, and determine if a 2x2 square composed
  entirely of vowels exists in the matrix. For example: strArr is
  {{"a","b","c","d"},{"e","i","k","r"}, {"o","u","f","j"}} then this matrix looks like the following:

  $$\begin{array}{cccc}
a & b & c & d \\
\color{red}e & \color{red}i & k & r \\
\color{red}o & \color{red}u & f & j \\
\end{array}$$

  Within this matrix there is a 2x2 square of vowels starting in the
  second row and first column, namely, ei, ou. If a 2x2 square of vowels
  is found your program should return the top-left position (row-column)
  of the square, so for this example your program should return 1-0. If
  no 2x2 square of vowels exists, then return the string not found. If
  there are multiple squares of vowels, return the one that is at the
  most top-left position in the whole matrix. The input matrix will at
  least be of size 2x2.

Are there any improvements I can make in terms of readability?

bool must_be_2x2(std::vector<std::vector<std::string>>& letters)
{
    constexpr int MIN_SIZE = 2;
    if (letters.at(0).size() > MIN_SIZE && letters.at(1).size() > MIN_SIZE)
    {
        return true;
    }

    return false;
}

std::string matrix_vector_to_string(std::vector<std::vector<std::string>>& matrix)
{
    std::string str;

    for (int row = 0; row < matrix.size(); ++row)
    {
        for (int col = 0; col < matrix.at(row).size(); ++col)
        {
            str += matrix.at(row).at(col);
        }
    }

    return str;
}

bool alphabet_only(std::vector<std::vector<std::string>>& letters)
{
    std::string str = matrix_vector_to_string(letters);

    for (std::size_t pos = 0; pos < str.length(); ++pos)
    {
        if (!std::isalpha(str.at(pos)))
        {
            return false;
        }
    }

    return true;
}

bool equal_length(std::vector<std::vector<std::string>>& letters)
{
    const size_t SIZE = letters.at(0).size();

    for (std::size_t row = 1; row < letters.size(); ++row)
    {
        if (letters.at(row).size() != SIZE)
        {
            return false;
        }
    }

    return true;
}

bool is_vowel(std::string element) noexcept
{
    static const auto vowels = std::array<std::string,5> { "a","e","i","o","u" };

    return std::find(vowels.begin(), vowels.end(), element) != vowels.end();
}

std::string find_vowel_square(std::vector<std::vector<std::string>>& letters)
{
    for (std::size_t row = 0; row < letters.size()-1; ++row)
    {
        for (std::size_t col = 0; col < letters.at(row).size()-1; ++col)
        {
            if (is_vowel(letters.at(row).at(col)) && is_vowel(letters.at(row).at(col + 1)) && is_vowel(letters.at(row + 1).at(col)) && is_vowel(letters.at(row).at(col + 1)))
            {
                return std::to_string(row) + "-" + std::to_string(col);
            }
        }
    }

    return "not found";
}

The #1 improvement you can make for code readability is adding comments. Plain code, even the most expressive plain code, rarely makes any sense on its own. In addition to comments, a use case showing the code in action is very helpful. Tests, too.

As for the code itself:

bool must_be_2x2(std::vector<std::vector<std::string>>& letters)

Vectors of vectors are rarely a good idea, and never a good idea for matrices. If you want a 2×2 matrix, you have to allocate three vectors: the vector for the matrix itself, then one vector per row. If you fail to properly allocate every row vector, you'll probably end up triggering UB.

Plus, accessing elements is slow. When you want to access an element of a vector of vectors, first the base vector has to be accessed and loaded... then the row vector has to be accessed or loaded. That's twice as many dives into main memory, which is painfully slow. And cache misses are more likely because a vector of vectors takes up more memory.

You can see the headaches yourself right in the code of must_be_2x2()... which, by the way, I think you have messed up the logic. A 2×2 matrix done as a vector of vectors would look like { { a, b }, { c, d } }. There are at least three ways to have a matrix less than 2×2:

{ { a, b } }: a 1×2 matrix.
{ { a }, { b } }: a 2×1 matrix.
{ { a, b }, { c } } or { { a }, { c, d } }: a ragged row matrix with 2 rows, and one row with 2 elements while the other row has 1 element.

must_be_2x2() ultimately "correctly" detects all 3 failure conditions (and I put "correctly" in quotes because it doesn't correctly work, but we'll get to that)... but it only seems to do so by a fluke. If I pass in a 1×2 matrix like case 1, this function will not return false... it will throw an exception. That's because you check the size of row 1 and row 2... but you never check that there are actually 2 rows. If there are 0 rows, calling letters.at(0) or letters.at(1) will throw; if there is only 1 row, calling letters.at(1) will throw.

So for the function to work the way I think you want it to work - and I'm only guessing at that because, as mentioned, no comments to explain the intention - you would first need to test letters.size()... and only then check letters.at(0).size() and letters.at(1).size().

But there are a couple of other bugs in the function.

First, you should take the parameter by const reference, not non-const reference. You're not changing the input.

Second, MIN_SIZE shouldn't be an int. Vector's size() returns the vector's size_type, which will be unsigned, so you will be comparing signed and unsigned types... which is a big no-no.

Third, and most concerning: you are testing the size of each row as letters.at(N).size() > MIN_SIZE. That means that a 2×2 matrix will fail. Because 2 > 2 is not true. I think you really meant to use >=.

The "fixed" function might look like this:

bool must_be_2x2(std::vector<std::vector<std::string>> const& letters)
{
    constexpr std::vector<std::vector<std::string>>::size_type MIN_SIZE = 2;
    // or for less typing:
    // constexpr decltype(letters.size()) MIN_SIZE = 2;

    if (letters.size() >= MIN_SIZE && letters.at(0).size() >= MIN_SIZE && letters.at(1).size() >= MIN_SIZE)
    {
        return true;
    }

    return false;
}

But as I said, this still smells, because if you are dealing with matrices, vectors of vectors is a terrible structure. It's bloated, it's slow, and it's extra work because you have to maintain that each row is the same size.

When you need a multidimensional matrix in C++, the best solution is almost always to use a flat array or vector, and calculate the offsets yourself. You could wrap it in a class for easier use. To give the basic idea:

template <typename T>
class matrix2d
{
public:
    matrix2d(std::size_t rows, std::size_t cols) :
        _elements(rows * cols),
        _rows{rows},
        _cols{cols}
    {}

    auto element(std::size_t row, std::size_t col) -> T&
    {
        return _elements[_calculate_offset(row, col)];
    }

    auto rows() const noexcept { return _rows; }
    auto cols() const noexcept { return _cols; }

private:
    auto _calculate_offset(std::size_t row, std::size_t col) noexcept
    {
        return (row * _cols) + col;
    }

    std::vector<T> _elements;
    std::size_t _rows;
    std::size_t _cols;
};

With a matrix class like this (obviously filled out with all the useful methods you'll need), you should see significant performance gains. And your code can be much simpler. The bottom line is: don't use vectors of vectors - they're almost always the wrong choice.

std::string matrix_vector_to_string(std::vector<std::vector<std::string>>& matrix)

This function "works"... but is highly inefficient.

First, again, you should take the parameter by const reference.

But the real issue is that you are repeatedly concatenating strings. Each time you do this, you could be triggering a new memory allocation, copying the whole string, then a deallocation.

Instead, it might be worthwhile to figure out in advance how much memory in total you're going to need. For that you'll need two loops:

std::string matrix_vector_to_string(std::vector<std::vector<std::string>> const& matrix)
{
    auto size = std::string::size_type{0};
    for (auto&& row : matrix)
        for (auto&& elem : row)
            size += elem.size();

    std::string str;
    str.reserve(size);
    for (auto&& row : matrix)
        for (auto&& elem : row)
            str += elem;

    return str;
}

On to the next function.

bool alphabet_only(std::vector<std::vector<std::string>>& letters)

Now, this function - like the previous function - "works", but it is extremely inefficient. First you construct a string of all the strings in the matrix concatenated, and then check that everything in that is a letter. But... why? Why create the concatenated string? Why not just check the letters in the matrix?

bool alphabet_only(std::vector<std::vector<std::string>> const& letters) noexcept
{
    for (auto&& row : letters)
    {
        for (auto&& elem : row)
        {
            for (auto c : elem)
            {
                if (!std::isalpha(static_cast<unsigned char>(c)))
                    return false;
            }
        }
    }

    return true;
}

On to the next function:

bool equal_length(std::vector<std::vector<std::string>>& letters)

This function's name isn't exactly clear. What it's doing is verifying that every row of the matrix is the same length. The only reason this is necessary at all is because you're using a vector of vectors.

bool is_vowel(std::string element) noexcept

This function has a lot of problems.

First of all, the parameter should probably be passed by const reference, not by value.

Second, it can't really be noexcept. Inside the function you construct an array of 5 strings. The string constructors are not noexcept. (One would hope that the string uses the small-string optimization. But one shouldn't rely on it like this.)

Which leads to the question of why you're dealing with strings at all. As far as I can tell, you're dealing with only ASCII characters. You don't need strings for this. This function could take a char, and do the test using a static array<char, 5>. That would be noexcept, and a lot faster.

std::string find_vowel_square(std::vector<std::vector<std::string>>& letters)

Once again, this "works", but is not particularly efficient. It's a brute force search, which is bad enough, but what's particularly bad is that it's a brute force search over a vector of vector of strings. I don't think you could have chosen a more inefficient data structure if you'd tried.

Changing to a better data structure alone - like a vector<char> (possibly wrapped in a matrix class) instead of vector<vector<string>> - should net you significant benefits. In the worst case scenario, with a large enough data set that it won't all in cache, and a string without SSO, you could be looking at a speed-up of several thousand times.

But changing to a more efficient algorithm would be the biggest gain (especially because doing that would automatically mean changing to a better data structure). You don't need to read the entire matrix in to search for sub-matrices. In fact, you don't really need to store any part of the matrix at all.

All you need to do is create a vector of flags (call it flags) of the size of one matrix row. Then you read the first matrix row, and for each element that is a vowel, set the corresponding flag in the flag vector. Then for each subsequent row i, start by setting another flag (not one in the vector, call it previous_is_vowel_column) to false, then for each character j, if it is a vowel and flag[j] is true and previous_is_vowel_column is true... you're done. The result is to_string(i - 1) + '-' + to_string(j - 1). Otherwise, you set to previous_is_vowel_column to true if the current character is a vowel and flag[j] is true, and set flag[j] to true if the current character is a vowel, and move on. If you get to the end without a success, then you return the fail message.


Sandwich shop interface using the Decorator pattern I have implemented a very basic sandwich (sub) shop which a user can add extra toppings at an additional cost. The cost is calculated and the contents of the sandwich is outputted.

I am learning design patterns and the book I am learning from (Head First Design Patterns) is in Java, which I am not too familiar with, so I may be making some errors.

Sub.h

#ifndef SUB_H
#define SUB_H

//Class Sub: Abstract Base class.
//Following the decorator pattern

#include <string>
#include <memory>

class Sub
{
  public:
    virtual double getCost() = 0;
    virtual std::string getToppings() = 0;
    virtual ~Sub() = 0;
};

class Plain: public Sub
{
  public:
    double getCost() override;
    std::string getToppings() override;
};

class ExtraCheese: public Sub
{
  public:
    ExtraCheese(std::shared_ptr<Sub> s) : sub(s) {}
    double getCost() override;
    std::string getToppings() override;
  private:
    std::shared_ptr<Sub> sub;
};

class ExtraMeat: public Sub
{
  public:
    ExtraMeat(std::shared_ptr<Sub> s) : sub(s) {}
    double getCost() override;
    std::string getToppings() override;
  private:
    std::shared_ptr<Sub> sub;
};

class ExtraSauce: public Sub
{
  public:
    ExtraSauce(std::shared_ptr<Sub> s) : sub(s) {}
    double getCost() override;
    std::string getToppings() override;
  private:
    std::shared_ptr<Sub> sub;
};

#endif

Sub.cpp

#include "Sub.h"

inline Sub::~Sub(){}

//class Plain : public Sub
double Plain::getCost()
{
  return 3.75;
}

std::string Plain::getToppings()
{
  return "Basic Sub";
}

//class ExtraCheese : public Sub

double ExtraCheese::getCost()
{
  return sub->getCost() + 0.75;
}

std::string ExtraCheese::getToppings()
{
  return sub->getToppings() + ", Extra Cheese";
}

//class ExtraMeat : public Sub
double ExtraMeat::getCost()
{
  return sub->getCost() + 1.50;
}

std::string ExtraMeat::getToppings()
{
  return sub->getToppings() + ", Extra Meat";
}

//class ExtraSauce : public Sub
double ExtraSauce::getCost()
{
  return sub->getCost() + 0.15;
}

std::string ExtraSauce::getToppings()
{
  return sub->getToppings() + ", Extra Sauce";
}

main.cpp

#include <iostream>
#include <memory>
#include "Sub.h"

int main()
{
  std::shared_ptr<Sub> s(new Plain());

  s = std::make_shared<ExtraCheese>(s);
  s = std::make_shared<ExtraMeat>(s);
  s = std::make_shared<ExtraSauce>(s);

  std::cout<<"COST: "<< s->getCost() <<std::endl;
  std::cout<<"Toppings: " << s->getToppings() <<std::endl;
}

No:

An object "Sub" and a decorator "Topping" implement the same interface.
In addition a decorator takes an item (of the same interface) that it is decorating.
This is not quite what you have done.

What if I want extra cheese and meat?

You need something more like this:

class Price
{
    public:
       virtual     ~Price()   = 0;
       virtual int getPrice() = 0;
};

class Sub: public Price
{
    public:
        int getPrice() override {return 375;}
};

class ExtraCheese: public Price
{
    std::unique_ptr<Price>   item
    public:
        ExtraCheese(std::unique_ptr<Price>&& item)
             : item(std::move(item))
        {}
         int getPrice() override {return item->getPrice() + 25;}
};

int main()
{
    std::unique_ptr<Price>   sub(new Sub);
    std::unique_ptr<Price>   subCheese(new ExtraCheese(sub));
    std::unique_ptr<Price>   subCheeseMeat(new ExtraMeat(subCheese));

    std::cout << subCheeseMeat->getPrice();
}


Can this graph search be optimized? I have the following which searches my graph to see if a vertex is reachable from the first vertex, which everything should be connected to. I do this to ensure there are no disconnected parts.

Unfortunately it is very slow.

Is there something I could do or store to optimize this?

I want to learn about graphs and generated cities so Im not using a real graph library.

private void removeDisconnectedSquares()
{
    for(int i = 0; i < getNumXNodes(); ++i)
    {
        for(int j = 0; j < getNumYNodes(); ++j)
        {
            //removeDisconnectedSquare(i, j);
            visitedNodes.clear();
            if(!isNodeReachableFrom(getNodeAt(i, j), getNodeAt(0, 0)))
            {
                removeVertex(i, j);
            }
        }
    }
}

private boolean isNodeReachableFrom(GraphNode node, GraphNode target)
{
    if(node == null)
    {
        return false;
    }

    if(visitedNodes.contains(node))
    {
        return false;
    }
    else
    {
        visitedNodes.add(node);
    }

    if(node == target)
    {
        return true;
    }

    if(node.contains(target))
    {
        return true;
    }

    for(int i = 0; i < node.getSize(); ++i)
    {
        if(isNodeReachableFrom(node.at(i), target))
        {
            return true;
        }
    }

    return false;
}

Rather than using recursion (which is pretty slow), you can for this using a Set of all reachable nodes, and a Queue of nodes you still haven't searched.  This is a basic implementation of Breadth-First Search.

private boolean isNodeReachableFrom(GraphNode start, GraphNode target) {

    // These are GraphNodes who's connected GraphNodes we haven't looked at yet.
    // Maybe the target is connected to one of these!
    Queue nodesToSearch = new LinkedList<GraphNode>();
    nodesToSearch.add(start);

    // These are GraphsNodes which we have proved are reachable from the given node.
    // If we are ever about to add the given target to this list, we return true.
    Set reachableNodes = new HashSet<GraphNode>();

    // As long as there are still nodesToSearch, we could still find our target.
    while (nodesToSearch.peek() != null) {
        GraphNode node = nodesToSearch.remove();

        // If we have already seen this node, we don't want to look at its connected
        // nodes again, because we could get stuck in a cycle.
        boolean isNewNode = reachableNodes.add(node);

        // If this is a new node, see if the target is connected.  If it is, we are
        // done successfully.  Otherwise, add all of the connected nodes to our
        // list of nodesToSearch.
        if (isNewNode) {
            for (GraphNode connectedNode : getConnectedNodes(node)) {
                if (connetedNode.equals(target)) {
                    return true;
                }
                nodesToSearch.add(connectedNode);
            }
        }
    }

    // There are no nodes we haven't searched, so the target is not reachable.
    return false;
}

Note: I made up getConnectedNodes(node) because your code didn't show me how to do this with the GraphNode object.

Note that this is an implementation of isNodeReachableFrom.  However, you seem to want to figure out the list of nodes which aren't reachable from a starting node.  Notice that we build up the list of all reachableNodes in the call above, which is probably what you really want.  You could write a function to return that reachableNodes structure, which would reuse the above logic without the target sections.  Something like this:

public Set<GraphNode> getReachableNodes(GraphNode start) { ... }


DotNetZip: Adding files to archive faster in web app I'm looking for ways to speed up the process of adding entries to a zip-archive, in a web app, using DotNetZip.

The problem currently is that the response times out before all files has been added to the archive. There can potentially be between 200 - 500 entries in each archive, each entry between 1 - 4 meg in size. I've tried a sample of 100 files which is too slow.

Compression is not necessary since all files are .mp3-files.

I currently have this code:

private readonly IDictionary<string, Stream> _files = new Dictionary<string, Stream>();

public bool CreateZip(ref MemoryStream result, bool compress = true)
{
    using (ZipFile zip = new ZipFile())
    {
        zip.UseZip64WhenSaving = Zip64Option.AsNecessary;

        if (!compress)
            zip.CompressionLevel = CompressionLevel.None;

        if (_files.Any())
        {
            foreach (var file in _files)
            {
                ZipEntry entry = zip.AddEntry(file.Key, file.Value);
                entry.Comment = file.Key;
            }
        }

        zip.Save(result);
        result.Seek(0, SeekOrigin.Begin);
        result.Flush();
    }
    return true;
}

The files are first added to a dictionary (as streams from a RavenFS/RavenFileStorage) and then added to the archive. The zip itself is also saved to a stream.

I thought about threading but it seems that is not supported by the lib when adding entries to the archive.

Am I using the lib correctly or is any method faster?

Don't use ref parameters. It's very rarely needed and it's of no use in your CreateZip method.
Save the ZipFile directly to the Response.OutputStream instead of a temporary MemoryStream. This will allow you to start streaming the file while you're creating it.


Escalation approval tool, copy to clipboard via Python I created a simple little script for the people I work with, what it does is copy information to the users clipboard for escalation approvals. It's pretty simple and not very exciting but I want to know if there are ways to do this better:

from Tkinter import Tk
import getpass
import os
import time

class ClipBoard(object):

    full_string = ''  # Full string that will contain the entire escalation approval

    def __init__(self, string, addition):
        self.string = string
        self.addition = addition

    def add_addition_to_string(self):  # Add the string together
        return self.string + self.addition

    def copy_string_to_clipboard(self):  # Copy to the windows clipboard
        cb = Tk()
        cb.withdraw()
        cb.clipboard_clear()
        cb.clipboard_append(self.full_string)

    def create_full_string(self):  # Create the string and return it
        self.full_string = self.add_addition_to_string()
        return self.full_string

class ConfigureProgram(object):

    def __init__(self, directory, escalator):
        self.directory = directory
        self.escalator = escalator

    def check_directory(self):  # Check if the directory exists
        if os.path.exists(self.directory) is False:
            os.makedirs(self.directory)  # If it doesn't create it
        else:
            return True

    def check_esc_name(self):  # Check if there's already a file made
        if self.check_directory() is True:
            with open(self.escalator, 'a+') as esc:  # If not create it
                esc.write(raw_input('Enter what you want appear on escalations(be specific): '))

def get_user():  # Get the username that's running at the moment
    return getpass.getuser()

def create_addition_for_string():  # Where's it going to?
    return raw_input('Escalate to where? ')

def check_if_run_yet(path_to_file):  # Check if you need to configure the program
    return os.path.exists(path_to_file)

def read_username():  # Read the escalators name from the file created
    with open("C:\Users\{}\AppData\Roaming\esc\esc_name.txt".format(get_user())) as f:
        return f.readline()

if __name__ == '__main__':
    path = "C:\Users\{}\AppData\Roaming\esc\esc_name.txt".format(get_user())

    if check_if_run_yet(path) is not False:
        opening = "Ticket has been reviewed and approved by {}. Ticket assigned to ".format(read_username())
        cliptext = ClipBoard(opening, create_addition_for_string())
        cliptext.create_full_string()
        cliptext.copy_string_to_clipboard()
        print 'Press CNTRL-V to paste the approval.'
        time.sleep(1)
    else:
        config = ConfigureProgram(
            "C:\Users\{}\AppData\Roaming\esc".format(get_user()), path
        )
        config.check_directory()
        config.check_esc_name()
        print "Program configured, please run again."
        time.sleep(2)

The main part of your program is only

def copy_string_to_clipboard(self):  # Copy to the windows clipboard
    cb = Tk()
    cb.withdraw()
    cb.clipboard_clear()
    cb.clipboard_append(self.full_string)

the rest being used only as a formatting tool. You should thus focus into making such formatting as clear as possible.

But first of, you may want to read about docstrings as your comment should be one:

def copy_string_to_clipboard(self):
    """Copy to the windows clipboard"""

    cb = Tk()
    cb.withdraw()
    cb.clipboard_clear()
    cb.clipboard_append(self.full_string)

So now, onto cleaning a bit everything around this function.

First off, you don't need a class with 3 other functions just to concatenate two strings. Based on your opening variable, what you need to do is simply:

user = read_username()
assignation = raw_input('Escalate to where? ')
copy_to_clipboard(
        'Ticket has been reviewed and approved by {}. '
        'Ticket assigned to {}.'.format(user, assignation))

where copy_to_clipboard has been slightly modified to accept the whole string as argument:

def copy_to_clipboard(text):
    """Copy the content of a string to the windows clipboard"""

    cb = Tk()
    cb.withdraw()
    cb.clipboard_clear()
    cb.clipboard_append(text)

The last thing to do is to simplify a bit the reading of the user config file. It's a bit of a shame to be required to run the program twice in order to create a file if it doesn't exist. Instead, you could try to read the file and, if it fails, ask the user for its content:

def read_username(filename='esc_name.txt'):
    """Get the user's name from config file or command line"""

    try:
        with open(filename) as f:
            return f.readline()
    except OSError:
        pass

    # If we are here, we couldn't read the configuration
    # file so ask the user an try to save its choice
    username = raw_input('Enter what you want appear on escalations (be specific): ')

    try:
        with open(filename, 'w') as f:
            f.write(username)
    except OSError:
        pass

    return username

But this function assumes we are in the right directory, which may not exist yet. Let's tackle that as well:

from getpass import getuser

def setup(program_dir='esc'):
    """Create our own directory structure and move to the right place"""

    appdata = r'C:\Users\{}\AppData\Roaming'.format(getuser())
    os.chdir(appdata)
    if not os.path.exists(program_dir):
        os.mkdir(program_dir)
    os.chdir(program_dir)

as you can see, I used getuser from getpass directly without having to create a wrapper that just remove 1 level of nesting.

Also, if I recall correctly, os.environ['APPDATA'] should contain the value we are building into appdata. So you could simplify it to:

def setup(program_dir='esc'):
    """Create our own directory structure and move to the right place"""

    os.chdir(os.environ['APPDATA'])
    if not os.path.exists(program_dir):
        os.mkdir(program_dir)
    os.chdir(program_dir)

Putting it all together, you can end up with:

import os
from Tkinter import Tk

def copy_to_clipboard(text):
    """Copy the content of a string to the windows clipboard"""

    cb = Tk()
    cb.withdraw()
    cb.clipboard_clear()
    cb.clipboard_append(text)

def setup(program_dir='esc'):
    """Create our own directory structure and move to the right place"""

    os.chdir(os.environ['APPDATA'])
    if not os.path.exists(program_dir):
        os.mkdir(program_dir)
    os.chdir(program_dir)

def read_username(filename='esc_name.txt'):
    """Get the user's name from config file or command line"""

    try:
        with open(filename) as f:
            return f.readline()
    except OSError:
        pass

    # If we are here, we couldn't read the configuration
    # file so ask the user an try to save its choice
    username = raw_input('Enter what you want appear on escalations (be specific): ')

    try:
        with open(filename, 'w') as f:
            f.write(username)
    except OSError:
        pass

    return username

if __name__ == '__main__':
    setup()
    user = read_username()
    assignation = raw_input('Escalate to where? ')
    copy_to_clipboard(
            'Ticket has been reviewed and approved by {}. '
            'Ticket assigned to {}.'.format(user, assignation))
    print 'Press CTRL-V to paste the approval.'


How to avoid duplication in my document writer? I have these two methods which are generating two different xml files whose header section is same and detail section varies. The root node is different for both xml files. Here I have two methods written, one for each. I want to optimize the code by avoiding the duplication of code in these two methods. Can you please guide me achieve this?

private string GenerateType1XML(TypeHeader header, List<TypeDetail> TypeDetailList)
{

    StringBuilder builder = new StringBuilder();
    using (StringWriter stringWriter = new StringWriter(builder))
    {
        using (XmlWriter writer = XmlWriter.Create(stringWriter))
        {
            writer.WriteStartDocument();
            writer.WriteStartElement(xmlElementConfig.CreateType1RootNode);
            WriteTypeHeader(writer, header);
            foreach (TypeDetail typ in TypeDetailList)
            {
                WriteType1Detail(writer, typ);
            }
            writer.WriteEndElement();
            writer.WriteEndDocument();
        }
    }

    return builder.ToString();

}

private string GenerateType2XML(TypeHeader header, List<TypeDetail> TypeDetailList)
{

    StringBuilder builder = new StringBuilder();
    using (StringWriter stringWriter = new StringWriter(builder))
    {
        using (XmlWriter writer = XmlWriter.Create(stringWriter))
        {
            writer.WriteStartDocument();
            writer.WriteStartElement(xmlElementConfig.CreateType2RootNode);
            WriteTypeHeader(writer, header);
            foreach (TypeDetail typ in TypeDetailList)
            {
                WriteType2Detail(writer, typ);
            }
            writer.WriteEndElement();
            writer.WriteEndDocument();
        }
    }

    return builder.ToString();

}

You need to look at what's common to both methods:

StringBuilder builder = new StringBuilder();
using (StringWriter stringWriter = new StringWriter(builder))
{
    using (XmlWriter writer = XmlWriter.Create(stringWriter))
    {
        writer.WriteStartDocument();
        writer.WriteStartElement(/* different in the two methods */);
        WriteTypeHeader(writer, header);
        foreach (TypeDetail typ in TypeDetailList)
        {
            /* different in the two methods */
        }
        writer.WriteEndElement();
        writer.WriteEndDocument();
    }
}

Looks like you could have a base class with a GenerateXml method calling into abstract members that derived types (Type1XmlGenerator, Type2XmlGenerator... whatever Type1 and Type2 means) can override:

public abstract class XmlGenerator
{
    protected abstract void WriteDetails(XmlWriter writer, TypeDetail type);
    protected abstract string XmlRootNode { get; }

    public string GenerateXml(TypeHeader header, IEnumerable<TypeDetail> details)
    {
        StringBuilder builder = new StringBuilder();
        using (StringWriter stringWriter = new StringWriter(builder))
        {
            using (XmlWriter writer = XmlWriter.Create(stringWriter))
            {
                writer.WriteStartDocument();
                writer.WriteStartElement(XmlRootNode);
                WriteTypeHeader(writer, header);
                foreach (TypeDetail detail in details)
                {
                    WriteDetails(writer, detail);
                }
                writer.WriteEndElement();
                writer.WriteEndDocument();
            }
        }
    }
}

Then you can have a class Type1XmlGenerator : XmlGenerator that will override the WriteDetails method with what you currently have in WriteType1Detail, and the XmlRootNode property getter implementation will return xmlElementConfig.CreateType1RootNode.

Now, writing/generating XML is a solved problem, doing that with a StringBuilder is error-prone. You should really look into what LINQ-to-XML (System.Xml.Linq) has in store for you.


Sum up all odd numbers between 1 and 2n + 1 Assignment:

  Write a program which reads a number n from the console. Then
  calculates the sum of all odd numbers between 1 and 2n + 1 (incl.).

Assignment also on GitHub, Exercise 0.0.

The functions read_hex and print_eax as well as the header and tail of the script are provided by the tutor.

format PE console
entry start

include 'win32a.inc'

; ===============================================
section '.text' code readable executable

start:
    call    read_hex

    mov     esi, 1 ; esi becomes the counting variable.

    mov     ebx, 2
    mul     ebx
    add     eax, 2
    mov     edi, eax ; edi becomes the limit.

    mov     eax, esi
    mov     ebx, 2 ; Will be the divisor.
    mov     ecx, 0 ; ecx will store the sum.
operationSet:
    mov edx, 0 ; Make sure that the register is empty.
    div ebx

    cmp edx, 0
    JE manageOperationSet ; If 0 then skip next operation.
    add ecx, esi
manageOperationSet:
    inc esi
    mov eax, esi
    cmp esi, edi
    jne operationSet

    mov eax, ecx
    call print_eax

    push    0
    call    [ExitProcess]
include 'training.inc'

I've tested my program with the numbers from 1 to 6 and get correct results. I think it works correctly, but I'm not sure concerning my register usage.

Moreover: What could be improved? Are there weak points in my code?

Random thoughts:

A comment that indicates what/where read_hex returns might be useful.
mov ebx, 2; mul ebx could probably be replaced with shl eax,1.
While div can be used to check for 'oddness', I believe it is an expensive solution.  How about bt?
In fact, why check for oddness every iteration of the loop?  Check once at the top and count by 2's.
If you aren't using mul/div, then you don't have to have values in specific registers.  This might help you avoid moving values from one register to another so much.

But my biggest piece of advice is to junk this whole approach and start over.

I've tried to calculate some solutions here (let me know if I've got this wrong):

n   result
0   1
1   4
2   9
3   16
4   25

Do you notice anything interesting about the numbers in the right-hand column?  Here's a hint, but before you hover over it, take a moment to try to puzzle it out yourself.  You'll feel better if you do.

   1*1 = 1, 2 * 2 = 4, 3 * 3 = 9, 4*4 = 16, 5 * 5 = 25

Or in other words:

   (n+1)2

With that in mind, there's a much faster/simpler way to calculate results, especially if n is big.


find if a number divisible by the input numbers Given two numbers a and b, we have to find the nth number which is
  divisible by a or b.

  The format looks like below:

  Input: First line consists of an integer T, denoting the number of test cases. Second line contains three integers a, b and N

  Output: For each test case, print the Nth number in a new line.

  Constraints:

  1≤t≤105

  1≤a,b≤104

  1≤N≤10

  Sample Input

1

2 3 10

  Sample Output

15

  Explanation

  The numbers which are divisible by 2 or 3 are: 2,3,4,6,8,9,10,12,14,15
  and the 10th number is 15.

For single test case input 2000 3000 100000 it is taking more than one second to complete. I want to know if I can get the results in less than 1 second. Is there a time efficient approach to this problem, maybe if we can use some data structure and algorithms here?

test_case=input()

if int(test_case)<=100000 and  int(test_case)>=1:
    for p in range(int(test_case)):
        count=1
        j=1

        inp=list(map(int,input().strip('').split()))
        if inp[0]<=10000 and  inp[0]>=1 and  inp[1]<=10000 and  inp[1]>=1 and inp[1]<=1000000000 and  inp[1]>=1:
            while(True ):
             if count<=inp[2] :
               k=j
               if j%inp[0]==0 or j%inp[1] ==0:
                   count=count+1
                   j=j+1

               else       :
                   j=j+1
             else:
                 break
            print(k)
        else:
            break

You really need to make your code easier to read, and more consistant.

I highly advise you learn all of PEP8 and follow it, so that your fellow Python developers can read your code with ease. And not cringe when you start using one and three spaces as indentation.
I highly advise you get one of the linters avaliable for Python.
Use functions, they help people know the scope of your code. I don't need to know about count and j when getting input.
Rather than using list(map(fn, it)) you can use a list comprehension, [fn(i) for i in it]. This is generally the prefered way.
Use sugar when provided, j=j+1, looks like a mess. Applying PEP8 would change it to j = j + 1, but you can always use +=, to get j += 1. Reducing the amount we need to read.
There's no need for one of your else's. If you have if ...: ... a += 1 else: a += 1, then you can remove the else and move the a += 1 to run either way.
It's advised to use gaurd statements, rather than follow the arrow anti-pattern. if count<=inp[2] : could be inverted, followed with the break, and the rest of the code doesn't have another indentation level.

In all I'd highly advise changing your code to something like the following:

def get_nth_multiple(a, b, limit):
    count = 1
    j = 1
    while True:
        if count > limit:
            break

        k=j
        if j%a == 0 or j%b == 0:
            count += 1
        j += 1
    return k

test_case = int(input())
if 1<= test_case <= 100000:
    for _ in range(int(test_case)):
        a, b, limit = [int(i) for i in input().split()]
        if 1 <= a <= 10000 and 1 <= b <= 10000:
            print(get_nth_multiple(a, b, limit))
        else:
            break

This is good, however, I'm not sold on the if statements outside the function. I personally would just remove them, as they're not needed.

I'd also change get_nth_multiple. Say you were asked to get the tenth multiple of five, you'd just do \$5 * 10 = 50\$. However you could make a function that returns all multiples of five, and only take the tenth. Something like next(islice(count(5, 5), 10 - 1, None)). And so, you only really need to merge two count(n, n)'s together to make one list, which you can then take the wanted multiple from.

This should sound simple, however, you also have to account for the fact that count is an iterator, not an iterable. And so you have to make a 'peek' list, that contains the next values of a and b. Which can be implemened as:

from itertools import count, islice

def get_multiples(*nums):
    sources = [count(i, i) for i in nums]
    peek = [next(source) for source in sources]
    while True:
        num = min(peek)
        yield num
        peek = [
            next(source) if i == num else i
            for i, source in zip(peek, sources)
        ]

def get_nth_multiple(a, b, limit):
    return islice(get_multiples(a, b), limit - 1, None)

test_case = int(input())
for _ in range(int(test_case)):
    a, b, limit = [int(i) for i in input().split()]
    print(get_nth_multiple(a, b, limit))


Haskell - Caching third party API token across requests The following code is designed to cache a (UTCTime, Text) value that keeps the Text token around until it expires, preventing needing to re-fetch the token each time this third party API is called. I feel like this duplication in APIDefaults and APIEnv could be removed, perhaps through a type parameter, though there may be a better way of doing this than storing the MVar inside APIDefaults.

APIError and getApiToken are just stubbed out as their detail is not relevant here, but in my code these are not implemented as they are below, so you can just ignore those.

data APIDefaults = APIDefaults
  { apidefManager :: Manager
  , apidefPool :: ConnectionPool
  , apidefTokenRef :: MVar (UTCTime, Text)
  }

data APIEnv = APIEnv
  { apienvManager :: Manager
  , apienvPool :: ConnectionPool
  , apienvToken :: Text
  }

data APIToken = APIToken
  { apiTokenAccessToken :: Text
  , apiTokenExpiresIn :: Int
  }

data APIError

fireApiRequest
  :: MonadLogger m
  => MonadUnliftIO m
  => ReaderT APIEnv m (Either APIError a)
  -> ReaderT APIDefaults m (Either APIError a)
fireApiRequest req = runExceptT $ do
  env <- do
    t <- lift readCachedToken
    case t of
      Nothing -> fetchToken
      Just (savedExpiry, token) -> lift $ do
        updateTokenRef (savedExpiry, token)
        apd <- ask
        return (createAPIEnv apd token)
  ExceptT $ lift (runReaderT req env)

readCachedToken
  :: MonadUnliftIO m
  => ReaderT APIDefaults m (Maybe (UTCTime, Text))
readCachedToken = runMaybeT $ do
  mv <- lift (asks apidefTokenRef)
  (savedExpiry, token) <- MaybeT $ liftIO (tryTakeMVar mv)
  now <- getCurrentTime
  guard (savedExpiry > now)
  return (savedExpiry, token)

updateTokenRef
  :: MonadUnliftIO m
  => (UTCTime, Text)
  -> ReaderT APIDefaults m ()
updateTokenRef value = do
  tokenRef <- asks apidefTokenRef
  void $ liftIO (tryPutMVar tokenRef value)

fetchToken
  :: MonadLogger m
  => MonadUnliftIO m
  => ExceptT APIError (ReaderT APIDefaults m) APIEnv
fetchToken = do
  now <- getCurrentTime
  adef <- lift ask
  token <- getApiToken
  let expiration = addUTCTime (fromIntegral $ apiTokenExpiresIn token) now
  lift $ updateTokenRef (expiration, apiTokenAccessToken token)
  return $ createAPIEnv adef (apiTokenAccessToken token)

getApiToken :: Monad m => m APIToken
getApiToken =
  return undefined

createAPIEnv :: APIDefaults -> Text -> APIEnv
createAPIEnv adef token =
  APIEnv (apidefManager adef) (apidefPool adef) token

I think the type of apidefTokenRef unnecessarily exposes internal details. Perhaps it would better to parameterize APIDefaults with a monad and hide the code that gets the token behind a monadic action, like this:

data APIDefaults m = APIDefaults
  { apidefManager :: Manager
  , apidefPool :: ConnectionPool
  , apidefTokenAction :: m APIToken
  }

I'm also making it return the APIToken type instead of Text. It's good to have a more precise type for it than Text, let's not fall prey to primitive obsession!

Now APIEnv seems a bit redundant. Instead of reading the token from APIEnv, functions in need of it can simply execute the token action each time. Or perhaps we could define a function like

sequenceToken :: Monad m => APIDefaults m -> m (APIDefaults Identity)
sequenceToken r = do
    token <- apidefTokenAction r
    pure (r { apidefTokenAction = Identity token })

But how to define the token action itself? Here I will indulge in a bit of over-abstraction and define this typeclass:

class Perishable p where
    lifetime :: p -> NominalDiffTime

for values which have a certain lifetime during which they are valid. Of course, APIToken will have an instance.

What type should the function which creates the token action have? It will be something like

makeRefresher :: Perishable p => IO p -> IO (IO p)

(Working in IO for simplicity, the real function would likely work in MonadUnliftIO.)

The argument is an action that obtains the perishable value. But why the outer IO in the result? because makeRefresher will have to set up some reference—like an MVar—that will be used across successive invocations of the resulting action:

makeRefresher obtain = do
    ref <- newMVar _initialValueHole
    return $ _actualTokenActionHole

What should be on the MVar? Perhaps a Maybe (UTCTime, p). It would be Nothing initially to signify that no perishable value has been obtained yet, and later it would become Just (someTime,somePerishable). We have the time of creation and the expected lifetime of the Perishable, so we can decide whether to return the current value or invoke obtain again.

Instead of Maybe, I would perhaps use my own type, for clarity:

data PerishableState p = NoPerishableYet
                       | PerishableObtainedAt UTCTime p

Also, I would try to manipulate the MVar with functions like modifyMVar_ to avoid deadlocks if there's an exception while obtaining the token.

One potential disadvantage of this solution is that the token "state" becomes harder to inspect because it's hidden behind an opaque action. Perhaps some logging effect should be added.


Promises turning into pyramids I'm using node-ftp to download some files from an FTP server; due to nodeback difficulty and nested calls I'm promisified some of the calls, hoping that this will help get me out of pyramid code hell, but I think I'm still mis-applying something.

Before I get to my code pasted below, conceptually the pseudo-code of what I'm trying to do is:

foreach directory in a list:
    list files in directory
    filter to only files of interest
    download those files, save them to /tmp/whatever.

The objective is to end with a list of files downloaded and local paths.

I have code that works, but it's god-awful ugly and is not living up to the "promise of promises".  How can I re-arrange this to be more like the pseudo code above, and aggregate the results?   Part of the trouble is that the "list files in directory" is a promise, and each download returns a promise of a data stream.  So unless I do this smartly, it's a 3-level nested array of promises that all have to be resolved, with the very bottom level promise results the intended return value.

//Main handler: when the connection is ready, start doing stuff.
c.on('ready', function() {
    config.clientCheckPaths
    .map(function (lookInPath) {
        console.log('Examining ' + lookInPath);

        // Go get directory contents.
        listAsync(lookInPath)
        .then(function (files) {
            console.log('Files: ' + files.map(function (i) { return i.name; }));

            var wantedFiles = files.filter(function (item) {
                return config.isFileWanted(item.name);
            });

            // Filter it down to only the files we want.
            var arrayOfDownloadPromises = wantedFiles.map(function (x) {
                // For each file, go download it.
                return new Promise(function (resolve, reject) {
                    var fullPath = lookInPath + '/' + x.name;

                    console.log("Wanted file: " + lookInPath + '/' + x.name + ", " + Object.keys(x));

                    getAsync(fullPath)
                    .then(function (stream) {
                        // With the stream, name it something reasonable on the disk.
                        var localFileName = '/tmp/' + fullPath.replace(/[^A-Za-z0-9\.]/g, '_');
                        console.log("Writing stream to " + localFileName);
                        stream.once('close', function() {;});
                        stream.pipe(fs.createWriteStream(localFileName));

                        resolve(localFileName);
                    }, function(err) {
                        var msg = 'Getting file ' + fullPath + ' failed with ' + err;
                        console.error(msg);
                        reject(msg);
                    })
                });
            });

            // We then need to re-join all of the download promises and proceed
            // when they're all finished.
            return Q.all(arrayOfDownloadPromises);
        }, function(err) {
            console.error('Listing directory ' + lookInPath + ' failed with ' + err);
        });
    })
    .map(function (promise) {
        console.log('Final: ' + promise);
    });
});

This code performs the right actions now, but doesn't aggregate the results correctly, and is ugly.  How to refactor this when I have multi-level nested promises?

Here's a quick attempt to clean it up:

c.on('ready', function() {
  return Promise.all(config.clientCheckPaths.map(function (lookInPath) {
    console.log('Examining ' + lookInPath);
    // Go get directory contents.
    return listAsync(lookInPath).then(function(files){
      return files.map(function(i){
        i.lookInPath = lookInPath;
        return i;
      });
    })
  }))
  .then(flatten)
  .then(function (files) {
    console.log('Files: ' + files.map(function (i) { return i.name; }));
    // Filter it down to only the files we want.
    return Promise.all(
      files.filter(function (item) {
        return config.isFileWanted(item.name);
      }).map(downloadWantedFiles)
    );
  });
});

function downloadWantedFiles(x) {
  // For each file, go download it.
  var fullPath = x.lookInPath + '/' + x.name;
  console.log("Wanted file: " + x.lookInPath + '/' + x.name + ", " + Object.keys(x));
  return getAsync(fullPath)
    .then(writeLocalFile(fullPath))
    .catch(couldNotDownload(fullPath))
  });
}

function writeLocalFile(fullPath) {
  return function (stream) {
    // With the stream, name it something reasonable on the disk.
    var localFileName = '/tmp/' + fullPath.replace(/[^A-Za-z0-9\.]/g, '_');
    console.log("Writing stream to " + localFileName);
    var write = stream.pipe(fs.createWriteStream(localFileName));
    // Only resolve once the local file has been written to disk
    return new Promise(function(resolve, reject){
      write.on('end', resolve.bind(null, localFileName));
      write.on('error', reject);
    });
  }
}

function couldNotDownload(fullPath) {
  return function (err) {
    var msg = 'Getting file ' + fullPath + ' failed with ' + err;
    console.error(msg);
    return Promise.reject(err);
  }
}

function flatten(arr) {
  return arr.reduce(function (memo, val) {
    var args = Array.isArray(val)
      ? flatten(val)
      : [val];
    memo.push.apply(memo, args);
    return memo;
  }, []);
}


Recursive Error Handling in Python with a limited amount of tries I have a class method that is set up to make requests to an API and I'm having issues handling the possible errors that could arise by trying to communicate with that server. The idea is if an error were to pop up, then it would call on itself until either it no longer needs to (because it worked), or it reaches a limit I set up for it; in this case, I would want it to just simply raise an exception that would be caught somewhere else in the code. What I don't know is how to properly return the response if it crapped out the first time it made the call and had to call on itself from the except block.

I'm not 100% clear if this actually gets the job done. Additionally, I dont know if this is the best way to do it. I know a possible suggestion is to make error handling more specific, given the requests library myriad exceptions, but I figured that any exception should just be handled by trying X amount of time and then quitting if it doesn't work.

def getRequest(self, endpoint, attempts):
        baseUrl = self.baseUrl
        url = baseUrl + endpoint
        if self.expirationT <= datetime.datetime.now():
            self.token, self.expirationT = self.auth(client_id, client_secret)
        else:
            pass
        try:
            response = requests.get(url, auth = BearerAuth(self.token))
            response.raise_for_status()
        except:
            if attempts < 20:
                time.sleep(3)
                response = self.getRequest(endpoint, attempts + 1)
                return response
            else:
                raise Exception
        else:
            return response

I know a possible suggestion is to make error handling more specific, given the requests library myriad exceptions, but I figured that any exception should just be handled by trying X amount of time and then quitting if it doesn't work.

I disagree here. Not just any exception should be handled. It's possible to have typo'd a bug into your code inside the try, and you definitely don't want try masking a bug.

If you check the source (or documentation), you'll see that the requests exceptions all seem to inherit from RequestException. If you really want to handle every possible request exception the same, I would catch the base class RequestException instead. I still don't think this is a good idea though without doing any logging. There may very well be a RequestException that gets thrown at some point which indicates that you accidentally gave the request bad data, not that there was a problem with the request being carried out using good data. I'd check the docs for the methods used and figure out what exact exceptions you want to retry on.

This also doesn't need to be recursion. In this case, nothing bad will likely happen because you have a limit of 20 retries, which isn't enough to exhaust the stack any sane case. If you ever increase that limit up to 1000 though, you may run into real problems.

I think this could be done pretty easily using a while True loop. The first two lines of the function seem to be essentially constants, so they don't need to be recomputed every time. Everything under those lines though can be stuck in a loop.

def getRequest(self, endpoint, max_attempts=20, retry_delay=3):
    baseUrl = self.baseUrl
    url = baseUrl + endpoint

    attempts = 0
    while True:
        if self.expirationT <= datetime.datetime.now():
            self.token, self.expirationT = self.auth(client_id, client_secret)

        try:
            response = requests.get(url, auth=BearerAuth(self.token))
            response.raise_for_status()
            return response

        except requests.RequestException as e:
            attempts += 1

            if attempts < max_attempts:
                time.sleep(retry_delay)

            else:
                raise RuntimeError("Max number of retires met.")

                # Or to preserve in the trace the original problem that caused the error:
                # raise RuntimeError("Max number of retires met.") from e

Things to note:

To retry now, instead of manually recursing, I'm just letting control fall out of the except so that the while can restart again.
Instead of attempts being a parameter, I just made it a local variable which is incremented inside of the except.
I'm throwing a more specialized exception with an informative error message. Throwing the generic Exception makes life more difficult for the users of your code. Ideally, they should be able to pick and choose what exceptions they handle and when. Throwing Exception though forces them to catch your errors. RuntimeError isn't really the best exception here, but I couldn't think of a good built-in one for this purpose. You may want to make a custom exception for this case:

class TooManyRetries(Exception):
    pass

. . .

raise TooManyRetries("Max number of retires met.")

I got rid of the else: pass. That isn't necessary.
You had two "magic numbers": 20 and 3 to mean the max number of attempts and the retry delay. I don't think it's a good idea to have those hard coded though. What if you want to change either at some point? You'd have to edit the code. I made them parameters of the function, defaulting to the values that you had. If you don't specify them, the behavior will be as you had before, but now they can be easily changed as needed.


Get rid of recursion in fruit picker I have this simple code and it is working fine it is self explanatory: the user will choose a random number 10 times and based on the random number the array will be filled by fruits.

var fruit =[];

for( x=0; x<10,x++){
    var r = Math.floor(Math.random() * 100 + 1);
    choose(r);
}

function choose(num){

  if(num <30){

      fruit.push("apple");

   }else if(num <70){
   fruit.push("kiwi");

    }else if(num< 90){

    fruit.push("banana");

    }else{

    fruit.push("grapes");
};

}

However in the code above, requirement is not restrictive; so below the number of available fruit is limited, and even if the random number choose a fruit, i have first to check if it is available in store or not. if the fruit is not available I should fall through to the next available fruit. The code below is working fine; but I had to do it via recursion in case fruit is not available.

var apple = 2;
var kiwi = 1;
var banana = 5;
var grapes = 2;

var fruit =[];

for( x=0; x<10,x++){
    var r = Math.floor(Math.random() * 100 + 1);
    choose(r);
}

function choose(num){

  if(num <30){
     if(apple>0){
     fruit.push("apple");
     apple--;
     }else{
     num = 31;
     choose(num);
     };

  }else if(num <70){

      if(kiwi>0){
         fruit.push("kiwi");
         kiwi--;
         }else{
        num = 71;
        choose(num);
        };

  }else if(num< 90){
       if(banana>0){
         fruit.push("banana");
         banana--;
        }else{
          num = 91;
          choose(num);
        };
}else{
      if(grapes>0){
        fruit.push("grapes");
        grapes--;
      }else{
         num = 1;
         choose(num);
      };
};

}

This code is working and my question is:

Since this selection of food is happening many times in the page, and it involves recursion, I am worried that this will degrade the performance.

Is there any advice in this regard? or any change suggested to the code or algorithm?

Micnic was almost there, but I think that you can lose the recursion completely by separating your if statement into several if statements

function choose(num){
    var looped = false;
    while (!looped) {
        if (num < 30 && apple > 0) {
            fruit.push("apple");
            apple--;
            return "added apple";
        }
        if (num < 70 && kiwi > 0) {
            fruit.push("kiwi");
            kiwi--;
            return "added kiwi";
        }
        if (num < 90 && banana > 0) {
            fruit.push("banana");
            banana--;
            return "added banana";
        }
        if (grapes > 0) {
            fruit.push("grapes");
            grapes--;
            return "added grapes";
        } else {
            if (looped) {
                return "no fruit left";
            }
            num = 1;
            looped = true;
        }
    }
};

This will remove the recursion (sounds like that is what you want to do)  and will still cascade through the fruit.  notice that if the number is 95 and there are no grapes it will still recursively call the choose function with a number of 1 to cover the rest of the fruit, The only difference here is that you don't have the recursive call in every if statement, and you have less than half of the if statement calls.  I also made sure that the this wouldn't loop more than once by implementing a while loop that will only ever loop once.

If the number is 40 it will start at checking for kiwi's if there are none then then it will check for bananas.

I think this is exactly what you were looking for.

I don't like a function returning undefined, so I added the return "added {fruit}";  but you could just have return; if that is your preference.


Passing HTML back inside a JSON object I'm working on a piece of code in my MVC3 application and I'm struggling to decide if what I plan to do is a bad idea or not.

I have a snippet of JavaScript that calls an MVC action that in turn returns an HTML partial that I then insert into a div element on my page. Nothing complicated there.

However, things have changed and I now need to pass back a possible error message as well. I propose to send this back with the HTML partial as part of a JSON object.

It would look something like this:

{
     errorMessage: "things have gone wrong!",
     partial : "
Test	0MB

Test2	1MB
"
}

This seems pragmatic but maybe a little dirty to me, so I'd like the thoughts of the community, please.

Now the obvious alternative would be to pass the table data back as JSON as well, and parse it client-side. But this will be a pain, even with something like jQuery templates (the example I've put up is simplified).

I would consider returning with a more detailed JSON object:

{
    errorMessage: "things have gone wrong!",
    data: [
        {
            link: #,
            linkText: Test,
            size: 0MB
        },
        {
            link: #,
            linkText: Test2,
            size: 1MB
        }
}

It moves some processing from the server to the clients but it's more reusable. For example you need a second type of table later with the same data but with an extra column. With this JSON format you don't have to touch the server side code nor to parse and modify the returned HTML on the client side.

Finally, if there is an error maybe you want to omit the whole data part:

{
    errorMessage: "things have gone wrong!",
}


Take actions depending on whether a number is within X percent of another Let's say we had a baseline value of 100 and I have a value of X.

If X is 5% or less higher/lower than 100 (95 to 105) then do something. If it's greater than 5% (Less than 95 or more than 105) do something else.

My initial thought was something like the below but it looks quite messy

$diff = 100*0.05;
$lower = 100-$diff;
$higher = 100+$diff;
if ($x >= $lower && $x <= $higher) {
     // Within tolerance
}
if ($X < $lower || $x > $higher) {
     // Outside tolerance
}

Apart from replacing the second if with an else, I would define this as a separate function:

function withinTolerance($value, $target, $tolerance) {
    return $value >= $target * (1.0 - $tolerance) && $value <= $target * (1.0 + $tolerance);
}

Then your call can just be:

if (withinTolerance($x, 100, 0.05)) {
    // Within tolerance
} else {
    // Outside tolerance
}

I made everything one line as it's a simple expression, which should clearly indicate what it does, but you can feel free to break $target * (1.0 - $tolerance) to $min, and $target * (1.0 + $tolerance) to $max if you like.

This allows you to change the target and tolerance easily, and also has the added bonus of being reusable, and testable. (How do you go about verifying that your current version works, for example?)

You can define some simple tests:

if (withinTolerance(99, 100, 0.05)) { echo "Pass"; } else { echo "Fail"; }
if (!withinTolerance(10, 100, 0.05)) { echo "Pass"; } else { echo "Fail"; }

And so on.

You could also use a trick with the absolute value (as mentioned in comments on the question), but that takes a lot of the meaning away. This version allows you to easily define upper and lower bounds:

function withinSingleTolerance($value, $target, $tolerance) {
    return withinTolerance($value, $target, $tolerance, $tolerance);
}
function withinTolerance($value, $target, $toleranceBelow, $toleranceAbove) {
    return $value >= $target * (1.0 - $toleranceBelow)
           && $value <= $target * (1.0 + $toleranceAbove);
}


Music Theory: The Basics - a Ring I've recently started building an API that allows a consumer to create and manipulate musical entities such as notes, intervals, scales and chords.

The first step is to create a a foundation of base structures that are later used by the musical entities.
One such structure is a Ring.

A ring in modular arithmic

A Ring is an integer's representation clamped between 0 and the ring size. Its Value is the integer it represents.  Its Size is the group size. Its Class is the congruent value clamped in the specified range. Its Group determines the number of ranges it's away from the reference group 0.

A ring from the perspective of Music Theory

  The way I'm using a ring is to define a musical note. A note consists
  of a pitch and a degree. The pitch is a number that represents the
  frequency of a note. A ring with size 12 is used to represent all
  semi-tones within a single octave. Each octave is a ring group.
  Another ring with size 7 is used to define the degree of the note.
  Common representations of degrees are {C, D, E, F, G, A, B} or
  do-re-mi-fa-sol-la-ti. The note C on the 5th octave is considered
  the reference note with pitch 0 and degree 0.

I want to be able to change a ring's properties.
The example below shows how the pitch of a note could be represented by a ring.

val pitch = Ring(0, 12)   // the pitch of note 'C5'
pitch.Group = 1           // the pitch one octave higher 'C6'
pitch.Class = 2           // the pitch class changed to that of note 'D6'
println(pitch)            // Ring(Value=14, Size=12)

The example below shows how the degree of a note could be represented by a ring.

val degree = Ring(0, 7)   // the degree of note 'C5'
degree.Group = 1          // the degree one octave higher 'C6'
degree.Class = 1          // the degree class changed to that of note 'D6'
println(degree)           // Ring(Value=8, Size=7)

This means the note D6 can be represented by pitch Ring(Value=14, Size=12) and degree Ring(Value=8, Size=7).

I have opted to use a mutable class and data class.
I want a mutable class because I require a lot of manipulations to be chained.
And I desire a data class because of its nature of hiding boiler-plate code and providing interesting methods such as copy.

val ring = someOtherRing.copy().setClass(2).setGroup(1)

I did read that data classes should be immutable.

  So my questions are:

Is it allowed to use the data class as mutable class?
Does it make sense that I combine mutable and data class for the Ring class?
Are there better alternatives to create an API class?

Any additional feedback on Kotlin guidelines, general guidelines, fluent-ish methods and varia is welcome as well.

Ring code:

import kotlin.math.*

data class Ring(var Value: Int, val Size: Int) {

    var Class: Int
        get() = modulo(Value)
        set(value) {
            Value = Group * Size + modulo(value)
        }

    var Group: Int
        get() = Value / Size
        set(value) {
            Value = value * Size + Class
        }

    fun setValue(value: Int): Ring {
        Value = value
        return this
    }

    fun setClass(clazz: Int): Ring {
        Class = clazz
        return this
    }

    fun setGroup(group: Int): Ring {
        Group = group
        return this
    }

    private fun modulo(x: Int): Int {
        return (x % Size + Size) % Size
    }
}

Your concepts look completely misguided.

A ring is a mathematical concept, an algebraic structure. It does have a size, but it doesn't have a value.

The value would be part of a ring element. Such a ring element is a tuple (ring, value).

The pitch of a note is indeed a ring element. A ring element can only be used to store the pitch of a note, but not its octave. If it did, it would not match the mathematical concept of a ring anymore.

To represent a musical note in Western notation, my first idea is:

data class Note(
    val octave: Int,
    val name: NoteName,
    val mod: NoteModifier,
    val duration: NoteDuration
)

enum class NoteName {
    C, D, E, F, G, A, B
}

enum class NoteModifier {
    Natural, Sharp, Flat, TwoSharp, TwoFlat
}

enum class NoteDuration {
    Full, Half, Quarter, Eighth, Sixteenth
}

The above definitions are very rough and limited. To get a grasp of the actual complexity of typesetting music, have a look at LilyPond, it should have a definition of a note somewhere in the code.

If you just want to replay the music, there's no need to distinguish between c# and d\$\flat\$, which would make the above definitions much simpler.

In Kotlin you don't need setters since the copy function is more powerful than in Java. You can just say:

val note = other.copy(octave = 3, pitch = 5)

This is easier to grasp and less code to write. If you write a setter method in Kotlin, lean back and think twice. Probably you are doing something unusual.

By the way, property names in Kotlin are written in lowercase. Kotlin is based on Java, not C#.


Reading from a CSV file in C++ I wrote this code to read from a CSV text file (containing data such as 12,3,568,48,3,8 with no more than 3 digits to each number). It stores the numbers as char arrays in the vector values. However, this does seem like a clumsy way of doing this with resizing the vector and copying the chars. Is there a neater/more succinct way of doing this?

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

int main() {
    using namespace std;
    ifstream in;
    in.open("Data.txt");
    assert(in.is_open());
    vector<char*> values;
    const int MAXSIZE = 4;
    char thisVal[MAXSIZE];
    while(in.getline(thisVal,MAXSIZE,',')) {
        values.resize(values.size() + 1);
        values.back() = new char[MAXSIZE];
        strcpy(values.back(), thisVal);
    }
    in.close();
    for (char* cp: values) cout<<*cp<<endl;
    return 0;
}

Storing char* in a vector leads to leaks. instead use std::string, which will manage its own buffer and delete it as needed.

vector<string> values;

inserting to the back of a vector can be done with push_back or you can construct in place with emplace_back in C++11:

while(in.getline(thisVal,MAXSIZE,',')) {
    string str(thisVal);
    values.push_back(std::move(str));
    //std::move signals that I won't be using str anymore and that the underlying buffer can just be passed through to the new element.
    // After this str will not be valid. You can still assign to it normally but until you do any result of calling a method on it will be implementation defined.
}

or

while(in.getline(thisVal,MAXSIZE,',')) {
    values.emplace_back(thisVal);
}

Failing to open a file is common enough to use an explicit if and print an error message instead of asserting (which won't do anything when compiled as a release version):

if(!in.is_open()){
    cerr << "failed to open file.";
    return 1;
}

I return 1 here to signal to the OS that the program didn't finish successfully and a script using it can then react to it.