Simple C++ program that calculates the measures of a sphere I have another home work from school and would like to see if I am on the right path to learning C++.

We have to do the calculation of the measures of a sphere. I did it two ways:

Just calculating everything at the main
Using a function

#include "stdafx.h"
#include "iostream"
#include <cmath>

double *calcSphereMesures(double r);

double *calcSphereMesures(double r)
{
const double PI = 3.1416;
double d, area, circumference, volume;
double results[4];

d = 2*r;
circumference = PI * d;
area = (4 * PI * pow(r,2));
volume = (4 * PI * pow(r,3))/3;

results[0] = area;
results[1] = circumference;
results[2] = volume;
results[3] = d;
return results;
}

int main()
{
double r;
std::cout << "Please enter the radius of a circle: ";
std::cin >> r;
double *results = calcSphereMesures(r);

std::cout << "\n**************************************" << "\n"
<< "* Area and Circumference of A Sphere *" << "\n"
<< "**************************************" << "\n"
<< "\nRadius: " << r << "\n"
<< "Diameter: " << results[3] << "\n"
<< "Area: " << results[0] << "\n"
<< "Circumference: " << results[1] << "\n"
<< "Volume: " << results[2];
return 0;
}

I tried to check all the tips that were given to me the last time. Is that still bad code? What can I improve here and remember for future exercises?

I see a number of things that may help you improve your code.

Prefer portable code

This code could be entirely portable if the #include "stdafx.h" were omitted.  It's Windows specific and doesn't really help here, even on Windows.

Understand header include paths

There is a subtle difference between #include "iostream" and #include <iostream>.  Although it's implementation defined, most compiler implementations is that the quotation mark form searches locally first (e.g. the current directory) and then searches system include directories if that fails.  The angle bracket form usually searches in system include directories.  See https://stackoverflow.com/questions/21593/what-is-the-difference-between-include-filename-and-include-filename for more.

Fix your formatting

The code is all jammed up on the left margin with no indenting.  I don't know if that's how you wrote it or if it's a cut-and-paste error when posting the question, but it really makes the program hard to read.  It makes no difference to the compiler, but you should strive for a good consistent style that includes visual clues for the human reader of your code.

Fix the bug

The calcSphereMesures routine is returning the address of a variable that's local to that routine.  That's a bug.  If you need to return multiple values, either pass in a pointer to an appropriate structure to be filled in by the routine or allocate one using new and returning that.  I'd prefer the former, because it makes ownership of the structure a little cleaner (that is, the caller must both allocate and free the structure).  Note that although I'm using the word structure, here, that isn't necessarily a struct -- it could just as well be an array or some other data structure.

Consider using an object

The area, circumference, etc., could all be attributes of a Sphere class.  This would have the advantage that one could write sphere.diameter() instead of results[3] for example.

Use name spaces

When you #include <cmath>, it brings those functions into the std:: namespace and may also put the symbols into the global namespace.  You're counting on the latter which is not guaranteed by the standard.  You can fix that by using std::pow instead of just pow.

Use a better value of \$\pi\$

There's little point in asking for the precision of a double if you're going to approximate \$\pi\$ as 3.1416.

Understand the use of function declarations

A function declaration is required before the use of the function.  Your separate declaration and implementation of the calcSphereMesures() function would make sense if the declaration were in its own header file, but that's not the case here.  For that reason, I'd suggest eliminating the declaration -- keeping them synchronized is just another task that you can eliminate.

Check your spelling

The word "measure" is consistently misspelled within the code.  It would be nice to fix that.


Storing values from checkboxes in database I'm getting values from 2 checkboxes and save those values to a database.

I don't like these if statements. How is it possible to simplify the code?

def edit_user(request, user_id):

user = User.objects.get(id=user_id)

if request.POST:

    if request.POST.get('is_staff'):
        user.is_staff = True
    else:
        user.is_staff = False

    if request.POST.get('is_active'):
        user.is_active = True
    else:
        user.is_active = False

    user.save()

    return HttpResponseRedirect(request.META.get('HTTP_REFERER', '/'))

You can use the default parameter from the get method.
Reference

if request.POST:
        user.is_staff = request.POST.get('is_staff', False)
        user.is_active = request.POST.get('is_active', False)
        user.save()

My updated code looks like that:

def edit_user(request, user_id):

    user = User.objects.get(id=user_id)

    if request.POST:

        user.is_staff = True if request.POST.get('is_staff') else False
        user.is_active = True if request.POST.get('is_active') else False
        user.save()

        return HttpResponseRedirect(request.META.get('HTTP_REFERER', '/'))


Sum of divisors in Haskell I decided to write a function divisorSum that sums the divisors of number. For instance 1, 2, 3 and 6 divide 6 evenly so:

$$ \sigma(6) = 1 + 2 + 3 + 6= 12 $$

I decided to use Euler's recurrence relation to calculate the sum of divisors:

$$\sigma(n) = \sigma(n-1) + \sigma(n-2) - \sigma(n-5) - \sigma(n-7) + \sigma(n-12) +\sigma(n-15) + \ldots$$

i.e.

$$\sigma(n) = \sum_{i\in \mathbb Z_0} (-1)^{i+1}\left( \sigma\left(n - \tfrac{3i^2-i}{2}\right) + \delta\left(n,\tfrac{3i^2-i}{2}\right)n \right)$$

(See here for the details). As such, I decided to export some other useful functions like nthPentagonal which returns the nth (generalized) pentagonal number. I created a new project with stack new and modified these two files:

src/Lib.hs

module Lib
    ( nthPentagonal,
      pentagonals,
      divisorSum,
    ) where

-- | Creates a [generalized pentagonal integer]
-- | (https://en.wikipedia.org/wiki/Pentagonal_number_theorem) integer.
nthPentagonal :: Integer -> Integer
nthPentagonal n = n * (3 * n - 1) `div` 2

-- | Creates a lazy list of all the pentagonal numbers.
pentagonals :: [Integer]
pentagonals = map nthPentagonal integerStream

-- | Provides a stream for representing a bijection from naturals to integers
-- | i.e. [1, -1, 2, -2, ... ].
integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering :: Integer -> Integer
    integerOrdering n
        | n `rem` 2 == 0 = (n `div` 2) * (-1)
        | otherwise = (n `div` 2) + 1

-- | Using Euler's formula for the divisor function, we see that each summand
-- | alternates between two positive and two negative. This provides a stream
-- | of 1 1 -1 -1 1 1 ... to utilze in assiting this property.
additiveStream :: [Integer]
additiveStream = map summandSign [0 .. ]
    where
    summandSign :: Integer -> Integer
    summandSign n
        | n `rem` 4 >= 2 = -1
        | otherwise = 1

-- | Kronkecker delta, return 0 if the integers are not the same, otherwise,
-- | return the value of the integer.
delta :: Integer -> Integer -> Integer
delta n i
    | n == i = n
    | otherwise = 0

-- | Calculate the sum of the divisors.
-- | Utilizes Euler's recurrence formula:
-- | $\sigma(n) = \sigma(n - 1) + \sigma(n - 2) - \sigma(n - 5) \ldots $
-- | See [here](https://math.stackexchange.com/a/22744/15140) for more informa-
-- | tion.
divisorSum :: Integer -> Integer
divisorSum n
    | n <= 0 = 0
    | otherwise = sum $ takeWhile (/= 0)
                                  (zipWith (+)
                                           (divisorStream n)
                                           (markPentagonal n))
    where
    pentDual :: Integer -> [Integer]
    pentDual n = [ n - x | x <- pentagonals]
    divisorStream :: Integer -> [Integer]
    divisorStream n = zipWith (*)
                              (map divisorSum (pentDual n))
                              additiveStream
    markPentagonal :: Integer -> [Integer]
    markPentagonal n = zipWith (*)
                               (zipWith (delta)
                                        pentagonals
                                        (repeat n))
                               additiveStream

app/Main.hs (mostly just to "test" it.)

module Main where

import Lib

main :: IO ()
main = putStrLn $ show $ divisorSum 8

TL;DR:

use memoization to speed up calculations
if you drive for performance use quot instead of div
if possible, try to define your sequences without complicated functions
use even n instead of n `rem` 2 == 0 or odd n instead of n `rem` 2 == 1

Type annotations and documentation

You use both type annotations as well as documentation. Great. There are only two small drawbacks:

The documentation strings sometimes miss highlighting, e.g. [1, -1, 2, -2,...] in pentagonals. Cross-references would also be great, e.g. pentagonals could point to nthPentagonal in its documentation.
Type annotations for workers (the local bindings) only contribute noise, as their type will be inferred by its surrounding function's types.

For example

integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering :: Integer -> Integer
    integerOrdering n
        | n `rem` 2 == 0 = (n `div` 2) * (-1)
        | otherwise = (n `div` 2) + 1

doesn't need the second type annotation:

integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering n
        | n `rem` 2 == 0 = (n `div` 2) * (-1)
        | otherwise = (n `div` 2) + 1

Convey ideas in code as direct as possible

Above, we use n `rem` 2 == 0 to check whether a number is even. However, there is already a function for that: even. It immediately tells us the purpose of the expression, so let's use that:

integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering n
        | even n    = (n `div` 2) * (-1)
        | otherwise = (n `div` 2) + 1

Use quotRem if you need both the result of rem and quot

integerStream is a special case, though: as we need both the reminder as well as div's result, we can use divMod or quotRem, e.g.

integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering n
        | r         = -q
        | otherwise = q + 1
       where
        (q, r) = n `quotRem` 2

Use simpler code where applicable

We stay at integerStream. As the documentation tells us, we want to have a bijection from the sequence of natural numbers to a sequence of all integers.

While the canonical mapping is indeed

$$
f(n) =
\begin{cases}
\frac{n-1}{2}+1& \text{if } n \text{ is odd}\\
-\frac{n}{2}& \text{otherwise}
\end{cases}
$$
we don't need to use that definition in our code. Instead, we can use

$$
n_1,-n_1,n_2,-n_2,\ldots.
$$

integerStream :: [Integer]
integerStream = go 1
    where
      go n = n : -n : go (n + 1)

Or, if you don't want to write it explicitly

integerStream :: [Integer]
integerStream = concatMap mirror [1..]
    where
      mirror n = [n, -n]

Both variants have the charm that no division is necessary in the computation of your list.

Use cycle for repeating lists

While the methods above arguably break down to personal preference, additiveStream can benefit from cycle:

additiveStream :: [Integer]
additiveStream = cycle [1, 1, -1, -1]

Generalize functions where applicable

delta can be written for any  type that is an instance of Num an Eq, so lets generalize:

delta :: (Eq n, Num n) => n -> n -> n
delta n i
    | n == i    = n
    | otherwise = 0

I tried writing divisorSum in terms of ZipList, but it kept shrinking until that wasn't needed anymore. Which is half the point of such rewrites!

divisorSum :: Integer -> Integer
divisorSum n
  | n <= 0 = 0
  | otherwise = sum $ takeWhile (/= 0) $ zipWith (*) additiveStream $
      map (\x -> divisorSum (n - x) + if x == n then 1 else 0) pentagonals

The call tree of with what arguments divisorSum calls itself is going to overlap with itself. In such situations, you can trade off space for time by keeping around a data structure that remembers the result for each possible argument after it's been calculated once. The memoize library captures this pattern:

divisorSum :: Integer -> Integer
divisorSum = memoFix $ \recurse n -> if n <= 0 then 0 else
  sum $ takeWhile (/= 0) $ zipWith (*) (cycle [1, 1, -1, -1]) $
    map (\x -> recurse (n - x) + if x == n then 1 else 0) pentagonals


LeetCode: Group Anagrams C# LeetCode: Group Anagrams C#
Given an array of strings, group anagrams together.

Example:

Input: ["eat", "tea", "tan", "ate", "nat", "bat"],
Output:
[
  ["ate","eat","tea"],
  ["nat","tan"],
  ["bat"]
]

Note:

All inputs will be in lowercase.
The order of your output does not matter.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace StringQuestions
{
    /// <summary>
    /// https://leetcode.com/explore/interview/card/top-interview-questions-medium/103/array-and-strings/778/
    /// </summary>
    [TestClass]
    public class GroupAnagramsTest
    {
        [TestMethod]
        public void TestMethod1()
        {
            string[] strs = {"eat", "tea", "tan", "ate", "nat", "bat"};
            var result = GroupAnagramsClass.GroupAnagrams(strs);
            List<IList<string>> expected = new List<IList<string>>();
            expected.Add( new List<string> {"eat", "tea", "ate"});
            expected.Add( new List<string> {"tan", "nat"});
            expected.Add( new List<string> {"bat"});
            for (int i = 0; i < result.Count; i++)
            {
                CollectionAssert.AreEqual(expected[i].ToList(), result[i].ToList());
            }
        }
    }

    public class GroupAnagramsClass
    {
        public static IList<IList<string>> GroupAnagrams(string[] strs)
        {
            Dictionary<char[], List<string>> dict = new Dictionary<char[], List<string>>(new CharComparer());
            foreach (var str in strs)
            {
                var key = str.ToCharArray();
                Array.Sort(key);
                if (!dict.TryGetValue(key, out var temp))
                {
                    temp = dict[key] = new List<string>();
                }
                temp.Add(str);
            }

            List<IList<string>> res = new List<IList<string>>(dict.Values.ToList());
            return res;
        }
    }

    public class CharComparer : IEqualityComparer<char[]>
    {
        public bool Equals(char[] x, char[] y)
        {
            if (x == null || y == null)
            {
                return false;
            }

            if (x.Length != y.Length)
            {
                return false;
            }

            for (int i = 0; i < x.Length; i++)
            {
                if (x[i] != y[i])
                {
                    return false;
                }
            }

            return true;
        }

        public int GetHashCode(char[] obj)
        {
            return 0;
        }
    }
}

Please review for performance. I don't like me copying the dictionary into IList<IList<string>. Is there a better way to do so, considering this the desired API defined by the question?

Deriving from IEqualityComparer versus EqualityComparer.

The MSDN docs say the following:

  We recommend that you derive from the EqualityComparer class instead of implementing the IEqualityComparer interface, because the EqualityComparer class tests for equality using the IEquatable.Equals method instead of the Object.Equals method. This is consistent with the Contains, IndexOf, LastIndexOf, and Remove methods of the Dictionary class and other generic collections.
  MSDN docs: IEqualityComparer

Dictionary versus Lookup

For grouping together objects from a sequence, where there might be a varying amount of items in each group, a Lookup is better than a Dictionary:

 var lookup = strs.ToLookup(key =>
 {
     var array = key.ToCharArray();
     Array.Sort(array);
     return array;
 }, new CharComparer());
 return lookup.Select(grouping => (IList<string>)grouping.ToList()).ToList();

Comparing char-arrays

Since we're using Linq, let's use Linq:

public override bool Equals(char[] x, char[] y)
{
    if (x == null || y == null)
    {
        return false;
    }

    if (x.Length != y.Length)
    {
        return false;
    }

    return x.SequenceEqual(y);
}

The null behaviour is different and the length shortcut may stay, but the last loop we can offload to Linq.

Hashcode

Both Dictionary and Lookup rely on the hashcode returned by the equalitycomparer to categorise the keys into bins. These bins is what allows these collections to get times in \$O(1)\$. Always returning the same value is going to cause problems when your inputs get bigger. It will effectively turn the collections into single arrays which have to be looped over to get to the correct key.

Creating good hashcodes is hard though, and I don't really know a good rule of thumb for creating them.

char[] versus string

All in all, the hashcode and equalitycomparer is such a headache, it's probably easier, and more readable to convert the sorted char[] back into a string, and use that as the key for the lookup:

 var lookup = strs.ToLookup(key =>
 {
     var array = key.ToCharArray();
     Array.Sort(array);
     return new string(array);
 });
 return lookup.Select(grouping => (IList<string>)grouping.ToList()).ToList();

Here's a one liner to have a look at.

public static IList<IList<string>> GroupAnagrams(string[] strs)
{
    return strs.GroupBy(x => string.Concat(x.OrderBy(c => c))).Select(x => (IList<string>)x.ToList()).ToList();
}


Determine Collatz sequence length for given starting values I am trying to get into the habit of solving programming puzzles in my spare time but am stuck on the first one (which I know can be for a large number of reasons, hence my posting for help). Shown below is my code that is meant to solve the 3n+1 Challenge:

from sys import stdin

def main():
    curr_Line=stdin.readline()
    while curr_Line!=None:
        curr_Line=curr_Line.split()
        min_num=int(curr_Line[0])
        max_num=int(curr_Line[1])
        maxCycleLength =0
        for curr_num in range(min_num,max_num+1):
            currCycleLength =1
            while curr_num!=1:
                currCycleLength +=1
                if curr_num%2==0:
                    curr_num=curr_num/2
                else:
                    curr_num=3*curr_num+1
            maxCycleLength=max(currCycleLength,maxCycleLength)
        print(min_num,max_num,maxCycleLength,sep=" ")
        curr_Line=stdin.readline()
    return
main()

When I run this, the UVA online judge says that I've exceeded the 3 second time limit.

I've tried the following:

changing the the while loop to for line in stdin:

Any suggestions would be much appreciated.

You need functions.
The first one you can make is the collatz_conjecture.
First off I'd say make it recursive.
If you ignore the currCycleLength you should get:

def collatz_conjecture(curr_num):
    if curr_num == 1:
        return 1
    if curr_num % 2 == 0:
        curr_num = curr_num / 2
    else:
        curr_num = 3 * curr_num + 1
    collatz_conjecture(curr_num)

The best way to make this better is to use functools.lru_cache(None).
This is a wrapper, but it'll give you speed for just adding one line! (This is what Zack was talking about)
You also should add the depth too, this is just adding one onto the previous depth. The if's are actually pretty ugly, so you can change it to the turnery operator.

@functools.lru_cache(None)
def collatz_conjecture(curr_num):
    if curr_num == 1:
        return 1
    return collatz_conjecture(3 * curr_num + 1 if curr_num % 2 else curr_num / 2) + 1

Finally I don't like that if n == 1. I'd reimplement lru_cache and supply it with that at the beggining.

def memoize(cache=None):
    if cache is None:
        cache = {}
    def wrapper(fn):
        def inner(*args):
            try:
                return cache[args]
            except KeyError:
                r = fn(*args)
                cache[args] = r
                return r
        return inner
    return wrapper

@memoize({(1,): 1})
def collatz_conjecture(n):
    return collatz_conjecture(3 * n + 1 if n % 2 else n / 2) + 1

As for the rest of your code, I'd use input with map, I'd use a comprehension passed to max and I'd also add if __name__ == '__main__':

This should get you something like:

def main():
    while True:
        user_input = input()
        if not user_input:
            break
        start, end = map(int, user_input.split()[:2])
        print(start, end, max(collatz_conjecture(i) for i in range(start, end + 1)))

if __name__ == '__main__':
    main()

This what I've come with:

import sys

cache = {1: 1}  # {number: cycle_length, ...}

def get_cycle_length(num):
    cycle_length = cache.get(num)
    if cycle_length is None:
        cycle_length = get_cycle_length(num // 2 if num % 2 == 0 else 3 * num + 1) + 1
        cache[num] = cycle_length
    return cycle_length

def main():

    for line in sys.stdin:
        min_num, max_num = map(int, line.split())
        assert 0 < min_num < 1000000 and 0 < max_num < 1000000

        max_cycle_length = max(map(get_cycle_length, range(min_num, max_num+1)))

        print(min_num, max_num, max_cycle_length)

assert get_cycle_length(1) == 1  # 1
assert get_cycle_length(2) == 2  # 2 1
assert get_cycle_length(3) == 8  # 2 10 5 16 8 4 2 1
assert get_cycle_length(22) == 16  # 22 11 34 17 52 26 13 40 20 10 5 16 8 4 2 1

main()

For the 4 test cases and the example in the task it works correctly, but unfortunately after submission the verdict is "Wrong answer".


Cleanest way to place values into strings I am trying to standardize the way I write code. Which is the best way to place the values into a string, method 1 or 2?

string myCat = "persian";

// Method 1
Console.WriteLine("I have a wild {0} cat that likes to visit", myCat);

// Method 2
Console.WriteLine("I have a wild " + myCat + " cat that likes to visit");

I would like to point out that I am intending this for simple uses, where StringBuilder would be overkill.

For simple straightforward string concatenation like your "persian" cat example, I agree with @Malachi - being slightly memory-inefficient over a few bytes will not hurt anyone, and if there's a gain in readability then it's worth it.

However, since you're looking to standardize the way you're doing string concatenations, you should go with string.Format() all the way, without hesitation*.

Say you're writing a program for some petshop (say, one that only sells cats), you'll potentially want to format/concatenate the cat's breed, birthdate/age and price tag. With string + string your only option is essentially to hard-code the date and currency formats, while this:

string.Format("{0} cat, born {1:d} ({2} weeks) | {3:C2}", cat.Breed,
                                                          cat.BirthDate,
                                                          cat.AgeInWeeks,
                                                          cat.Price)

...has an obvious advantage, especially when you consider that

"{0} cat, born {1:d} ({2} weeks) | {3:C2}"

is a string all by itself (as mentioned by Daniel Cook).

*At the end of the day, common sense should prevail. If you end up doing this:

var result = string.Format("{0}{1}", string1, string2);

...then you know you've gone overboard.

Method 1 creates two strings in memory:

"I have a wild {0} cat that likes to visit"
"I have a wild persian cat that likes to visit"

Method 2 creates four strings in memory:

"I have a wild "
" cat that likes to visit"
"I have a wild persian"
"I have a wild persian cat that likes to visit"

Method 1 is better because it always allocates fewer strings. In this case it allocates half as many.


Calculate Ranked Probability Score I have a csv-file that consists of all match outcome probabilities for soccer matches. Each math can be result in a win, draw or loss. I also included the actual outcome. In order to test how accurate my predictions are I want to use the Ranked Probability Score (RPS). Basically, the RPS compares the cumulative probability distributions of the predictions and the outcome:

  \$ RPS = \frac{1}{r-1} \sum\limits_{i=1}^{r}\left(\sum\limits_{j=1}^i p_j - \sum\limits_{j=1}^i e_j \right)^2, \$

  where \$r\$ is the number of potential outcomes, and \$p_j\$ and
  \$e_j\$ are the forecasts and observed outcomes at position \$j\$.

For additional information, see the following link.

import numpy as np
import pandas as pd

def RPS(predictions, observed):
   ncat = 3
   npred = len(predictions)
   RPS = np.zeros(npred)

   for x in range(0, npred):
      obsvec = np.zeros(ncat)
      obsvec[observed.iloc[x]-1] = 1
      cumulative = 0
      for i in range(1, ncat):
          cumulative = cumulative + (sum(predictions.iloc[x, 1:i]) - sum(obsvec[1:i])) ** 2
          RPS[x] = (1/(ncat-1)) * cumulative

    return RPS

df = pd.read_csv('test.csv', header=0)
predictions = df[['H', 'D', 'L']]
observed = df[['Outcome']]
RPS = RPS(predictions, observed)

The first argument (predictions) is a matrix with the predictions and the corresponding probabilities.  Each row is one prediction, laid out in the proper order (H, D, L), where each element is a probability and each row sum to 1. The second argument (observed) is a numeric vector that indicates which outcome that was actually observed (1, 2, 3)

Feel free to give any feedback!
Thank you

Edit:

For some reason I am not able to reproduce the results of Table 3 of the link. I use Table 1 as input for predictions and observed. Any help is much appreciated!

Edit #2:

Hereby the small sample of the paper:

predictions = {'H': [1, 0.9, 0.8, 0.5, 0.35, 0.6, 0.6, 0.6, 0.5, 0.55],
               'D': [0, 0.1, 0.1, 0.25, 0.3, 0.3, 0.3, 0.1, 0.45, 0.1],
               'L': [0, 0, 0.1, 0.25, 0.35, 0.1, 0.1, 0.3, 0.05, 0.35]}

observed = {'Outcome': [1, 1, 1, 1, 2, 2, 1, 1, 1, 1]}

Tidy up your math

cumulative = cumulative + (sum(predictions.iloc[x, 1:i]) - sum(obsvec[1:i])) ** 2

can be

cumulative += (sum(predictions.iloc[x, 1:i]) - sum(obsvec[1:i])) ** 2

and

RPS[x] = (1/(ncat-1)) * cumulative

should be

RPS[x] = cumulative / (ncat-1)

Make a main method

This is a very small script, but still benefits from pulling your global code into a main.

PEP8

By convention, method names (i.e. RPS) should be lowercase.

That's all I see for now.

Thanks for all the replies. In the end it was relatively simple. My code is based on R, matrices start with 1 in R. Python, however, starts with 0. Adjusting my original code with this insight (sorry..) I am able to reproduce the output. I also included the remarks of Reinderien.

import numpy as np
import pandas as pd

def rps(predictions, observed):
   ncat = 3
   npred = len(predictions)
   rps = np.zeros(npred)

   for x in range(0, npred):
      obsvec = np.zeros(ncat)
      obsvec[observed.iloc[x]-1] = 1
      cumulative = 0
      for i in range(1, ncat):
         cumulative += (sum(predictions.iloc[x, 0:i]) - sum(obsvec[0:i])) ** 2
         rps[x] = cumulative / (ncat-1))

      return rps

df = pd.read_csv('test.csv', header=0)
predictions = df[['H', 'D', 'L']]
observed = df[['Outcome']]
rps = rps(predictions, observed)


Solution in R

In your last code snippet, you're mixing code from tidyverse and data.table packages. I don't consider this to be completely wrong, but I would rather avoid it to increase readability and consistency.

library(magrittr)
library(data.table)
library(bench)

# data copied from OP
dat <- structure(list(Name = structure(c(1L, 2L, 3L, 4L, 1L, 2L, 3L),
                                       .Label = c("A", "B", "C", "D"),
                                       class = "factor"),
                      Date = structure(c(1L, 1L, 1L, 2L, 2L, 2L, 2L),
                                       .Label = c("1/1/2018", "1/4/2018"),
                                       class = "factor"),
                      Val1 = 1:7,
                      Val2 = 2:8),
                 class = "data.frame", row.names = 1:7)

dat
#>   Name     Date Val1 Val2
#> 1    A 1/1/2018    1    2
#> 2    B 1/1/2018    2    3
#> 3    C 1/1/2018    3    4
#> 4    D 1/4/2018    4    5
#> 5    A 1/4/2018    5    6
#> 6    B 1/4/2018    6    7
#> 7    C 1/4/2018    7    8
str(dat)
#> 'data.frame':    7 obs. of  4 variables:
#>  $ Name: Factor w/ 4 levels "A","B","C","D": 1 2 3 4 1 2 3
#>  $ Date: Factor w/ 2 levels "1/1/2018","1/4/2018": 1 1 1 2 2 2 2
#>  $ Val1: int  1 2 3 4 5 6 7
#>  $ Val2: int  2 3 4 5 6 7 8

Tidyverse Solution

tidyr::gather(dat, key = "key", value = "value", -Date, -Name) %>%
    tidyr::unite("id", key, Date, sep = ".") %>%
    tidyr::spread(id, value)
#>   Name Val1.1/1/2018 Val1.1/4/2018 Val2.1/1/2018 Val2.1/4/2018
#> 1    A             1             5             2             6
#> 2    B             2             6             3             7
#> 3    C             3             7             4             8
#> 4    D            NA             4            NA             5

data.table Solution

dt <- data.table(dat)
dt_long <- melt(dt, id.vars = c("Name", "Date"))

dcast(dt_long, Name ~ variable + Date)
#>    Name Val1_1/1/2018 Val1_1/4/2018 Val2_1/1/2018 Val2_1/4/2018
#> 1:    A             1             5             2             6
#> 2:    B             2             6             3             7
#> 3:    C             3             7             4             8
#> 4:    D            NA             4            NA             5

Benchmark

As you can see, data.table is already much faster with 1,200 rows.

nrows <- 1.2e4
# nrows <- 1.2e6
dat2 <- expand.grid(Name = LETTERS[1:4],
                    Date = seq(as.Date("2018-01-01"), by = "days", length.out = nrows/4))
dat2$Val1 <- sample(1:8, nrow(dat2), TRUE)
dat2$Val2 <- sample(1:8, nrow(dat2), TRUE)

f1 <- function(dat) {
    tidyr::gather(dat, key = "key", value = "value", -Date, -Name) %>%
        tidyr::unite("id", key, Date, sep = ".") %>%
        tidyr::spread(id, value)
}

f2 <- function(dat) {
    dt <- data.table(dat)
    dt_long <- melt(dt, id.vars = c("Name", "Date"))
    dt_wide <- dcast(dt_long, Name ~ variable + Date)
}

mark(tidyverse = f1(dat2),
     datatable = f2(dat2),
     check = function(x, y) all.equal(x, y, check.attributes = FALSE))
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 2 x 10
#>   expression     min    mean  median     max `itr/sec` mem_alloc  n_gc
#>   <chr>      <bch:t> <bch:t> <bch:t> <bch:t>     <dbl> <bch:byt> <dbl>
#> 1 tidyverse  184.4ms 189.7ms 187.9ms 196.7ms      5.27   15.73MB     5
#> 2 datatable   43.1ms  45.9ms  45.4ms  51.7ms     21.8     5.36MB     2
#> # ... with 2 more variables: n_itr <int>, total_time <bch:tm>

Created on 2019-02-26 by the reprex package (v0.2.1)


Read hex from file and convert to decimal I made a small script to read text file that contain hexadecimal and convert to another file in decimal by using command-line as a filename input.

#this is content from hex.txt
0x5B03FA01
0x42018360
0x9FF943B3

and this is what I expected my python to deliver output

#this is content from hex_1.txt
1526987265
1107395424
2683913139

After 3 hour of trial and error. I can see a result in the way I want. The code is follow;

#this was copied from Learn Python the Hard way exercise.
from sys import argv
from os.path import exists

#this is the main function to convert hexadecimal to decimal
def hex2dec (hex):
    result_dec = int(hex, 0)
    return result_dec

#this to accept parameter from command-line
script, from_file = argv

#this to make sure the output generate as a new file with _1
to_file = from_file[:-4] + "_1.txt"
out_file = open(to_file, 'w').close()

#this took me a few hour to figure out a bug. Thanks to stackoverflow
with open(from_file) as in_file:
    lines = in_file.read().splitlines()

for i in lines:
    converted = str(hex2dec (i))
    out_file = open(to_file, 'a+')
    out_file.write(converted + "\n")
    out_file.close()

    #this to print the result for reference on-screen.
    print "converted =" + str(converted)

print "done."

Even though this is done but I feel there are plenty area I could improve to this script e.g. bugs handling in hex2dec function.

However, may I ask you what would you do to enhance this further? and if you could suggest, could anyone suggest me which topic or area I can study further to improve this code or any other about Python?

Python 3

The script currently does not work in Python 3. For example, print "converted =" + str(converted) is invalid syntax in Python 3, use print() instead (as a function).

Comments

It's great that you document the code. Some comments however don't add any value for future reading: #this took me a few hour to figure out a bug. Thanks to stackoverflow. I have no idea what the bug is, and what the answer from StackOverflow was.

Try to use docstrings as well. For instance in the hex2dec function:

def hex2dec(hex):
    """Convert a hexadecimal string to a decimal number"""
    result_dec = int(hex, 0)
    return result_dec

Some other things about the above function:

Rather than hex2dec, perhaps use hex_to_decimal. In this case it's not too big of a deal though.
You're storing the result in a variable and then returning the variable without doing anything else to it. This means you could also return result immediately: return int(hex, 0)
Since it's using a built in function without any other operation, is the function necessary?

Running it!

As said above, using Python 3 failed to run it. Changing the print statements to functions fixed this issue. Python 2 also supports the print() function, so it should have no issue running on Python 2 as well.

The program ran as expected with a correct file, and produced an output file with the converted values.

Running it with an invalid file, or no arguments at all, crashes the program with either a FileNotFoundError or ValueError respectively. If a file was given which exists but has an invalid hexadecimal value in it (for instance a random string), the program also crashes:

Traceback (most recent call last):
  File "hex2dec.py", line 22, in <module>
    converted = str(hex2dec (i))
  File "hex2dec.py", line 7, in hex2dec
    result_dec = int(hex, 0)
ValueError: invalid literal for int() with base 0: 'random word'

The above errors tells us we need some error checking.

The first error to solve would be the ValueError of not giving a filename as argument when running the program. This can be solved by using a CLI module such as argparse, click, or perhaps another one you like. I personally use argparse, since it's pretty simple and it's part of the standard library.

The arguments:

import argparse

def get_cli_parser():
    parser = argparse.ArgumentParser(description='Describe what your program does')
    parser.add_argument('input_file', type=str,
                        help='The file to read the hexadecimal values from'
    return parser

if __name__ == '__main__':
    parser = get_cli_parser()
    args = vars(parser.parse_args())

At this point you can access the command line arguments args as a dictionary.

The if __name__ == '__main__': part of the code is only applicable if it's run as a script. This means that if the file is imported as a module, this will not run. See also this answer for more info.

When we run the above code without any arguments, we get the following:

usage: hex2dec2.py [-h] input_file
hex2dec2.py: error: the following arguments are required: input_file

Next up is the FileNotFoundError. This one is quite easy to solve: before opening the file, check if it exists using the already imported os module:

if not os.path.exists(args['input_file']):
    print('Input file does not exist, please try again')
    return

The last ValueError can be solved in a number of ways. One would be to use regex on the current line of the file to find any hexadecimal values. Assuming only the hexadecimal values should be on a line, it becomes a lot simpler though. We could just check if the line starts with 0x (e.g. a hexadecimal value).

Reading the file should also be done line by line as an iterator, to prevent huge files from crashing your program by too high memory consumption. Right now the program reads all the text in the file into memory. This can be done as described in the following question:

with open(input_file) as input_file:
    for line in input_file:
        # rest of code here.

Placing this in a function of its own also prevents you from opening the file continuously.

The final product

import argparse
import os

def write_and_print_result(args, result):
    """Write every result to the output file
    and print each converted value
    """
    out_file = args['input_file'][:-4] + "_1.txt"
    with open(out_file, 'a+') as output:
        for converted in result:
            output.write(converted + "\n")
            print('Converted: {}'.format(converted))

def read_and_convert_file(args):
    """Read the given input file and convert each hexadecimal value
    to decimal value
    """
    result = []
    with open(args['input_file']) as input_file:
        for line in input_file:
            if (line.startswith('0x')):
                result.append(str(int(line, 0)))
    write_and_print_result(args, result)

def get_cli_parser():
    parser = argparse.ArgumentParser(description="Describe program here")
    parser.add_argument('input_file', type=str,
                        help='The file to read the hexadecimal values from')
    return parser

def cli_runner():
    parser = get_cli_parser()
    args = vars(parser.parse_args())

    if not os.path.exists(args['input_file']):
        print('Input file does not exist, please try again.')
        return

    read_and_convert_file(args)

if __name__ == '__main__':
    cli_runner()

As you can see, most of it is either error checking or command line arguments. The actual conversion is only one line of code.

What's next

One issue I still found:

If the input file is anything but *.txt, the output file might have a weird name. For example, a file called hex would simply become _1.txt, which isn't very descriptive. To fix this, as pointed out by @Mathias Ettinger, you could use os.path.splitext. It is used as such:

filename, extension = os.path.splitext(args['input_file'])
outfile = filename + "_1.txt"

I would go with @Randyr answer but I would change the read and convert method as follows for two reasons; 1) the output method requires storing all your answers when they wont be processed again and 2) good python apologises rather then asks for permission.

def read_and_convert_file(args):
    """Read the given input file and convert each hexadecimal value
    to decimal value and output to file
    """
    line_num = 0
    out_file = args['input_file'] + ".2decimal"
    with open(out_file, 'a+') as output:
        with open(args['input_file']) as input_file:
            for line in input_file:
                line_num += 1
                try:
                    converted = str(int(line, 0))
                    output.write(converted + "\n")
                    print('Converted: {}'.format(converted))
                except ValueError:
                    print('Invalid text on line',line_num)


Expense Sharing Calculation The following is my solution to the problem presented on this CodeReview post:

Friends A, B, C, D go for a trip. They spend on various expenses. Cost of the expense is shared.
Example :

A spends 100 for breakfast for A, B, C and D
D spends 500 for cab for B and C
B spends 300 for lunch for A, B and C

Write a program to calculate how much each should get or each should give to one another. App should be scalable that number of friends can change

Design:
We need two data sets, which each respectively:

record transactions
tally amount due to each person

Simply strip off the info we need from the transactions and record either how much each person is owed, or how much each person should give.
Any input is welcome!
using System;
using System.Collections.Generic;

namespace CodeReview
{
    class Transaction
    {
        class Account
        {
            private int totalDebt = 0;
            private Dictionary<char, int> debtors = new Dictionary<char, int>();

            public Account(char initialDebtor, int initialDebt)
            {
                debtors.Add(initialDebtor, initialDebt);
            }

            public Dictionary<char, int> Debtors
            {
                get
                {
                    return debtors;
                }
            }

            public int TotalDebt
            {
                get
                {
                    foreach (char debtor in debtors.Keys)
                    {
                        totalDebt += debtors[debtor];
                    }

                    return totalDebt;
                }
            }
        }

        static void Main(string[] args)
        {
            Queue<Tuple<char, int, List<char>>> transactions = new Queue<Tuple<char, int, List<char>>>();
            Dictionary<char, Account> ledger = new Dictionary<char, Account>();

            transactions.Enqueue(Tuple.Create('A', 100, new List<char>() { 'A', 'B', 'C', 'D' }));
            transactions.Enqueue(Tuple.Create('D', 500, new List<char>() { 'B', 'C' }));
            transactions.Enqueue(Tuple.Create('B', 300, new List<char>() { 'A', 'B', 'C' }));

            while (transactions.Count > 0)
            {
                Tuple<char, int, List<char>> transaction = transactions.Dequeue();

                foreach (char beneficiary in transaction.Item3)
                {
                    if (!beneficiary.Equals(transaction.Item1))
                    {
                        if (ledger.ContainsKey(beneficiary))
                        {
                            Dictionary<char, int> debtors = ledger[beneficiary].Debtors;

                            if (debtors.ContainsKey(transaction.Item1))
                            {
                                debtors[transaction.Item1] += transaction.Item2;
                            }
                            else
                            {
                                debtors.Add(transaction.Item1, transaction.Item2);
                            }
                        }
                        else
                        {
                            ledger.Add(beneficiary, new Account(transaction.Item1, transaction.Item2));
                        }
                    }
                }
            }

            foreach (char beneficiary in ledger.Keys)
            {
                Account account = ledger[beneficiary];

                Console.WriteLine("User " + beneficiary + " owes $" + account.TotalDebt + ":");

                foreach (char debtor in account.Debtors.Keys)
                {
                    int debt = account.Debtors[debtor];

                    Console.WriteLine(" - $" + debt + " to " + debtor);
                }
            }

            Console.ReadKey();
        }
    }
}

The main issue I can see, is the excessive use of generic data types like tuples and dictionaries instead of domain classes. That may be a matter of taste, but I prefer meaningful named domain classes like your Account class.
In the Account class, the dictionary is exposed as public member. IMHO internal data structures should be encapsulated. The class should be extended by appropriated methods to manipulate that data structures. That would automatically move code like

if (debtors.ContainsKey(transaction.Item1))
{
    debtors[transaction.Item1] += transaction.Item2;
}
else
{
    debtors.Add(transaction.Item1, transaction.Item2);
}

into the domain class. Imagine use have to add items somewhere else, that kind of code must be duplicated each time.

Instead of

if (!beneficiary.Equals(transaction.Item1))
{
    ...
}

You could use

 if (beneficiary.Equals(transaction.Item1))
 {
     continue;
 }

 ....

to reduce nesting.

The instance variable debtors should be read-only.
Another small point: I would use strings to store the users because you can store names and you do not run into problems if the group becomes greater than 26 persons :P - of course, that is not relevant for the algorithm.

Generall Approach

Your approach is, to build up two dictionaries (the ledger and the debtors within the account class). The data structure of nested dictionaries matches the requirements exactly. If the requirements change, you have to restructure your whole code.

It is better to transform the data in a simple generally usable data structure and implement your algorithm for that data structure.

Such a simple data structure may be a list of Transactions:

public class Transaction
{
    public Transaction(string payer, string payee, int amount)
    {
        Payer = payer;
        Amount = amount;
        Payee = payee;
    }

    public string Payer { get; }
    public int Amount { get; }
    public string Payee { get; }
}

The algorithms could be implemented in a class like Bank:

public class Bank
{
    private readonly List<Transaction> myTransactions = new List<Transaction>();
    private readonly HashSet<string> myPayers = new HashSet<string>();
    private readonly HashSet<string> myPayees = new HashSet<string>();

    public void AddTransaction(string payer, string payee, int amount)
    {
        myTransactions.Add(new Transaction(payer, payee, amount));
        myPayers.Add(payer);
        myPayees.Add(payee);
    }

    public void PrintPayees()
    {
        foreach (var payee in myPayees)
        {
            var trans = myTransactions.Where(t => t.Payee == payee);
            Console.WriteLine($"User {payee} owes ${trans.Sum(t => t.Amount)} total:");
            foreach (var groupedTransaction in trans.GroupBy(t => t.Payer))
                Console.WriteLine($"- ${groupedTransaction.Sum(t => t.Amount)} from {groupedTransaction.Key}");
        }
    }

    public void PrintPayer()
    {
        foreach (var payer in myPayers)
        {
            var trans = myTransactions.Where(t => t.Payer == payer);
            Console.WriteLine($"User {payer} gets ${trans.Sum(t => t.Amount)} total:");
            foreach (var groupedTransaction in trans.GroupBy(t => t.Payee))
                Console.WriteLine($"- ${groupedTransaction.Sum(t => t.Amount)} to {groupedTransaction.Key}");
        }
    }
}

Usage base on the list of tuples:

void Main()
{
    var bank = new Bank();

    foreach (var transaction in new[] { Tuple.Create("A", 100, new [] { "A", "B", "C", "D" }),
                                        Tuple.Create("D", 500, new [] { "B", "C" }),
                                        Tuple.Create("B", 300, new [] { "A", "B", "C" }) })
    {
        var payer = transaction.Item1;
        var amount = transaction.Item2;
        foreach (var payee in transaction.Item3)
            if (payee != payer)
                bank.AddTransaction(payer, payee, amount);
    }

    bank.PrintPayees();
    bank.PrintPayer();
}

In the course of debating the merits of OOP with my friend Dino the Perl guy, I also implemented it as a one-liner in LINQ:

using System;
using System.IO;
using System.Linq;

namespace Expenses
{
    public class ExpensesLINQ
    {
        public void Run(string path = @"d:\temp\expenses.txt")
        {
            File.ReadAllLines(path)
            .Select(l => l.Split(','))
            .Select(x => Tuple.Create(x[0], x[1], x[2], string.Join(",", x.Skip(3))))
            .SelectMany(b => b.Item4.Split(',')
            .Where(f => f != b.Item1)
            .Select(f => new
            {
                Party1 = string.Compare(f, b.Item1) > 0 ? f : b.Item1,
                Party2 = string.Compare(f, b.Item1) > 0 ? b.Item1 : f,
                Amt = string.Compare(f, b.Item1) > 0 ? decimal.Parse(b.Item2) / b.Item4.Split(',').Count() : -decimal.Parse(b.Item2) / b.Item4.Split(',').Count(),
            }))
            .GroupBy(p => new { p.Party1, p.Party2 })
            .Select(g => new { Party1 = g.Key.Party1, Party2 = g.Key.Party2, Amount = g.Sum(w => w.Amt) })
            .Select(h => new { Payer = h.Amount < 0 ? h.Party2 : h.Party1, Recipient = h.Amount < 0 ? h.Party1 : h.Party2, Amount = Math.Abs(h.Amount) })
            .Select(o => $"{o.Payer} pays {o.Recipient} ${o.Amount}")
            .ToList()
            .ForEach(t => Console.WriteLine(t));
        }
    }
}


Program to find megaPrime numbers between 2 given integers A number is said to be megaPrime if it is prime, and all its digits are also prime.

Example:
53 is megaPrime, but 35 is not as it is divisible by 5 and 7.
13 is not a megaPrime number as it contains 1, which is non-prime.
INPUT
Two integer numbers, first and last are entered.
OUTPUT
Display the number of megaPrimes in the inteval between first and last, both inclusive.
CONSTRAINTS
1 <= first <= last <= 10^15
last - first <=  10^9
/* PROGRAM TO FIND MEGA PRIME NUMBERS BETWEEN TWO GIVEN INTEGERS */

#include<stdio.h>
int isPrime(int);
int isMegaPrime(int);

int main(void)
{
int i=0,st,sp,count=0;
    scanf("%d%d",&st,&sp);
    for(i=st;i<=sp;i++){
        if((i%2)!=0 && isMegaPrime(i)==1)
            count++;
    }

    printf("%d",count);
    return 0;
}

int isPrime(int n)
{
    int i=0,flag=0;
    if(n==1)
        return 0;
    else
    {
        int t=sqrt(n);
        for(i=2;i<=t;i++){
            if((n%i)==0){
                flag=1;
                break;
            }
        }
    }
    if(flag==1)
        return 0;
    else
        return 1;
}

int isMegaPrime( int n)
{
    int i=0,flag=0,temp=0;
    if(isPrime(n)==0)
        return 0;
    else{
        while (n!=0){
            temp=n%10;
            flag=isPrime(temp);
            if(flag==0)
                return 0;
            n/=10;

        }
    }
    if(flag==1)
        return 1;
    else
        return 0;
}

Please suggest the possible optimizations.

I think John Coleman did a good job explaining speeding up the testing for mega-primality, but you're still using trial division to test for primality. The Miller-Rabin test is much faster, and can be made deterministic given the size of your range.

The algorithm starts by checking if \$n\$ is 1 or even. These cases are trivial, and should return 0 unless \$n = 2\$.

Next we take n-1 and remove the largest power of 2. Put another way we want to find s and d such that \$2^sd = n - 1\$ and d is odd. For example, if \$n = 49\$, \$n - 1 = 2^43\$, so \$s = 4\$ and \$d = 3\$.

Next we pick a random number for \$a\$ such that \$1 \lt a \lt n - 1\$. Now we make a loop on \$r\$ where \$r\$ goes from 0 to \$s - 1\$ inclusively. If for all values of r:

\$a^{2^rd} \% n \ne n - 1\$ and \$a^d\%n\ne1 % n != 1\$, then \$n\$ is composite. Otherwise, \$n\$ has a large probability of being prime. If you test every \$a\$ where \$a = {2, 3, 5, 7, 11, 13, 17, 19, 23}\$ and \$n\$ comes up as a likely prime for every time, it is prime.

If you are unfamiliar with this you may want to do \$a^b \% c\$ one step at a time, but computing \$a^b\$ may take a long time, and will quite likely result in an overflow error.

Use this trick:

\$a^b \%c =\$
If \$b = 0\$ then 1
If \$b\$ is odd then \$(a^{b-1} \% c ) b \% c\$
If \$b\$ is even then \$(a^{\frac{b}{2}} \% c)^2 \% c\$

This way, the number is constantly being reduced, and as long as recursion is used, the time will scale logarithmically with the size of \$b\$.

#include<iostream>
using namespace std;
bool ismega(int n)
{
    bool flag=false;
    int temp;
    while(n!=0)
    {
        temp=n%10;
        if(temp==1)
        {
            flag=true;
            break;
        }
        n=n/10;
        for(int j=2;j<=(temp/2);j++)
        {
            if(temp%j==0)
            {
                flag=true;
                break;
            }

        }
        if(flag)
        {
            break;
        }
    }
    if(flag)
    {
        return false;
    }
    else
    {
        return true;
    }

}
int main()
{
    int first,last,sum=0;
    bool flag=false;
    cin>>first>>last;
    if(first==1)
    first=2;
    for(int i=first;i<=last;i++)
    {
        for(int j=2;j<=(i/2);j++)
        {
            if(i%j==0)
            {
                flag=true;
                break;
            }

        }

        if(!flag)
        {
        if(ismega(i))
        {
            sum+=i;
            cout<<i<<" ";
        }
        }
        flag=false;
    }
    cout<<endl<<sum;
    return 0;
}


Replace spaces with '%20' This is my attempted solution to Question 1.3 from "Cracking the Code Interview", 6th ed.

Question:

  Write a method to replace all spaces in a string with '%20'. You may assume that the string has sufficient space at the end to hold the additional characters, and that you are given the true length of the string.

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>
#include <string.h>
// Input:  "Mr John Smith    ", 13
// Output: "Mr%20John%20Smith"

int countSpaces(char* str) {
  int i = 0, j = 0;
  while (str[i] != '\0')
    if ( str[i++] == ' ')
      j++;

  return j;
}

char* replaceChars(char* str, int length) {
  int x = countSpaces(str);
  int stringLength = length + x - x/3;

  char* urlifiedString = malloc((stringLength + 1) * sizeof(char));
  int i = 0, j = 0;
  while (i < stringLength) {
    if (str[j] != ' ') {
      urlifiedString[i++] = str[j];
    } else if (str[j] == ' '){
      urlifiedString[i++] = '%';
      urlifiedString[i++] = '2';
      urlifiedString[i++] = '0';
    }
    j++;
  }
  urlifiedString[stringLength] = '\0';
  return urlifiedString;
}

int main() {
  char* result1 = replaceChars("Mr John Smith    ", 13);
  printf("%s\n", result1);
  printf("%li\n", strlen(result1));
  assert(strcmp("Mr%20John%20Smith", result1) == 0);
  free(result1);

  char* result2 = replaceChars("lol  lol lol      ", 12);
  printf("%s\n", result2);
  assert(strcmp("lol%20%20lol%20lol", result2) == 0);
  free(result2);

  char* result3 = replaceChars("", 0);
  assert(strcmp("", result3) == 0);
  free(result3);

  return 0;
}

I think you have misunderstood the part about "assume that the string has sufficient space at the end", and as a result, you have some memory management issues.  I think that the intention is to assure you that your replaceChars() function will be handed a buffer of sufficient size such that you may write the result in place, without allocating more memory.

In that light, your solution and tests look a bit confused — and in fact the code is wrong in the general case.  If the input is "Mr␠John␠Smith␠␠␠␠", and you countSpaces() without taking the true length (13) into account, then countSpaces() would report that there are 6 spaces rather than 2.  To compensate for the overestimate, you then use the formula int stringLength = length + x - x/3; — allocating roughly \$\frac{2}{3}\$ of an additional byte for each space, rather than two extra bytes per space.  And you are also relying on that dubious estimate of the result length to terminate your loop, with while (i < stringLength).

Suggested solution

#include <stdlib.h>
#include <assert.h>
#include <string.h>

// Count the spaces in the first n bytes of str
int countSpaces(const char* str, int n) {
  int count = 0;
  while (n --> 0) {
    if (*str++ == ' ') {
      count++;
    }
  }
  return count;
}

char* replaceChars(char* str, int length) {
  int spaces = countSpaces(str, length);
  char *in = str + length - 1,
       *out = str + length + 2 * spaces;
  *out-- = '\0';
  while (in != out) {
    if (*in == ' ') {
      out -= 3;
      in--;
      *(out + 1) = '%';
      *(out + 2) = '2';
      *(out + 3) = '0';
    } else {
      *out-- = *in--;
    }
  }
  return str;
}

int main() {
  const char *test1 = "Mr John Smith";
  char buf1[18] = {0};
  strcpy(buf1, test1);
  replaceChars(buf1, strlen(test1));
  assert(strcmp("Mr%20John%20Smith", buf1) == 0);

  const char *test2 = "lol  lol lol";
  char buf2[19] = {0};
  strcpy(buf2, test2);
  replaceChars(buf2, strlen(test2));
  assert(strcmp("lol%20%20lol%20lol", buf2) == 0);

  const char *test3 = "";
  char buf3[1] = {0};
  strcpy(buf3, test3);
  replaceChars(buf3, strlen(test3));
  assert(strcmp("", buf3) == 0);
}

Your code looks good - it seems to do the job you want it to. The only thing is where you use malloc((stringLength + 1) * sizeof(char)); you want to make it convert to type char*. Just throw it out like:

char* urlifiedString = (char*)malloc((stringlength + 1) * sizeof(char));

Also, once you free result1, just point it to new information with a malloc(). You don't need to make a second or third character pointer to assign new stuff to. The only thing you don't want to do is free(somepointer) if there isn't any memory reserved for the pointer.

Good job!


Infix expression (with negative numbers) to postfix I am writing a class to evaluate an arithmetic expression, For now my class can convert an infix expression into postfix, it doesn't support exponents yet.

    public class Evaluator {
        public Map<String, Operator> mOperators = new HashMap<>();

        public Evaluator() {
            addOperator(new Operator("+", 1) {
                @Override
                public BigDecimal eval(BigDecimal a, BigDecimal b) {
                    return a.add(b);
                }
            });

            addOperator(new Operator("-", 1) {
                @Override
                public BigDecimal eval(BigDecimal a, BigDecimal b) {
                    return a.subtract(b);
                }
            });

            addOperator(new Operator("*", 2) {
                @Override
                public BigDecimal eval(BigDecimal a, BigDecimal b) {
                    return a.multiply(b);
                }
            });

            addOperator(new Operator("/", 2) {
                @Override
                public BigDecimal eval(BigDecimal a, BigDecimal b) {
                    return a.divide(b);
                }
            });
        }

        public String toPostfix(String infix) {
            Stack<String> operators = new Stack<>();
            StringBuilder result = new StringBuilder();
            String[] tokens = infix.replaceAll("\\s+", "").split("(?<=[^.a-zA-Z\\d])|(?=[^.a-zA-Z\\d])");

            for (String token : tokens) {
                if (token.matches("-?\\d+(\\.\\d+)?")) {
                    result.append(token).append(" ");
                } else if (operators.isEmpty() || operators.peek().equals("(") || token.equals("(")) {
                    operators.push(token);
                } else if (token.equals(")")) {
                    while (!operators.peek().equals("(")) {
                        result.append(operators.pop()).append(" ");
                    }
                    operators.pop();
                } else {
                    while (!operators.isEmpty() && precedence(operators.peek()) >= precedence(token)) {
                        result.append(operators.pop()).append(" ");
                    }
                    operators.push(token);
                }
            }

            while (!operators.isEmpty()) {
                result.append(operators.pop()).append(" ");
            }

            return result.toString().trim();
        }

        public void addOperator(Operator operator) {
            mOperators.put(operator.getSymbol(), operator);
        }

        private int precedence(String token) {
            Operator operator = mOperators.getOrDefault(token, null);
            return operator == null ? -1 : operator.getPrecedence();
        }

        public abstract class Operator {
            private final String mSymbol;
            private final int mPrecedence;

            public Operator(String symbol, int precedence) {
                mSymbol = symbol;
                mPrecedence = precedence;
            }

            public abstract BigDecimal eval(BigDecimal a, BigDecimal b);

            public String getSymbol() {
                return mSymbol;
            }

            public int getPrecedence() {
                return mPrecedence;
            }
        }
}

My questions are:

How to deal with negative number, e.g: (-1) + 2
Is there any easier or better way to evaluate an arithmetic
expression without having to turn it into postfix?
What can I do to optimize my class and make it better?

The two most common approaches to evaluating simple mathematical expressions are :

Parse the expression into an Abstract Syntax Tree, and then evaluate the tree.
Process the expression using Dijkstra's Shunting-yard algorithm.

Is there any reason to not use the great built-in expression evaluator: Nashorn Javascript Engine?

It can calculate for you all sorts of expressions supported by ES6. Example of usage (in Java 9):

@Test
public void calculate() throws ScriptException {
    String MATH_EXPR = "-1 + x * 2";
    Bindings bindings = new SimpleBindings( new HashMap<>( Map.of("x", 2) ) );
    Double result = (Double) jse.eval(MATH_EXPR, bindings);
    out.printf("x = %d, %s = %.1f%n", bindings.get("x"), MATH_EXPR, result);
    assertEquals( Double.valueOf(3.0), result );
}


Capitalize the first character of all words (even when following a '-') I got it working, all variations are displayed Sint-Anna as should be, but I wonder, is there a simpler way to this, since it looks very cluttered?

String.prototype.capitalize = function(){
    var sa = this.replace(/-/g,' ');
    var saa = sa.toLowerCase();
    var sb = saa.replace( /(^|\s)([a-z])/g , function(m,p1,p2){ return p1+p2.toUpperCase(); } );
    var sc = sb.replace(/\s+/g, '-');
    return sc;
};

console.log('sint-anna'.capitalize());
console.log('sint anna'.capitalize());
console.log('sint-Anna'.capitalize());
console.log('Sint Anna'.capitalize());
console.log('SiNt anna'.capitalize());
console.log('SINT ANNA'.capitalize());

Console:

Sint-Anna
Sint-Anna
Sint-Anna
Sint-Anna
Sint-Anna
Sint-Anna

There are no ways that there is a wrong input like Si ntAn na resulting in Si-Ntan-Na.

Your problem is an interesting one,but your solution is not a great use of regular expressions, since there's the better option of using the 'word boundary' anchor \b.

The boundary anchor matches a transition from a word, to a non-word, or vice versa. For this purpose, using the \w match to target word-characters after the \b is the right solution.

Note that the replace-function you give to the replace operation only needs the m argument now, there's no longer any sub-groups to process.

If your code was written as:

    String.prototype.capitalize = function(){
        return this.toLowerCase().replace( /\b\w/g, function (m) {
            return m.toUpperCase();
        });
    };

then you would convert-to-lowercase, then locate the first letter of any word, and convert that to uppercase.

Putting that in to action, like:

    <input id="myInput" type="text" value="" size="50" />

type something in the box above

The easiest way of making all the first letters uppercase in your array:

for (var i = 0 ; i < yourArray.length ; i++) {
    curTokens[i] = yourArray[i].charAt(0).toUpperCase()+yourArray[i].substr(1);
}
console.log(yourArray);

Every first character is taken using the charAt(0) and changed to uppercase using .toUpperCase(). We append the rest of the string using yourArray.substr(1);.


Form for OTP entry with six single-character text fields I am working on an app in which I have to pass 6 digits OTP through 6 textFields in which you to provide only one character and after that it automatically goes to another textField. I created 6 textFields outlets and used this code.

override func viewDidLoad() {
    super.viewDidLoad()

txtOTP1.delegate = self
txtOTP2.delegate = self
txtOTP3.delegate = self
txtOTP4.delegate = self
txtOTP5.delegate = self
txtOTP6.delegate = self

}

And I used this logic to create the functionality

 func textField(_ textField: UITextField, shouldChangeCharactersIn range:NSRange, replacementString string: String) -> Bool {
    // Range.length == 1 means,clicking backspace
    if (range.length == 0){
        if textField == txtOTP1 {
            txtOTP2?.becomeFirstResponder()
        }
        if textField == txtOTP2 {
            txtOTP3?.becomeFirstResponder()
        }
        if textField == txtOTP3 {
            txtOTP4?.becomeFirstResponder()
        }
        if textField == txtOTP4 {
            txtOTP5?.becomeFirstResponder()
        }
        if textField == txtOTP5 {
            txtOTP6?.becomeFirstResponder()
        }
        if textField == txtOTP6 {
            txtOTP6?.resignFirstResponder()
        }
        textField.text? = string
        return false
    }else if (range.length == 1) {
        if textField == txtOTP6 {
            txtOTP5?.becomeFirstResponder()
        }
        if textField == txtOTP5 {
            txtOTP4?.becomeFirstResponder()
        }
        if textField == txtOTP4 {
            txtOTP3?.becomeFirstResponder()
        }
        if textField == txtOTP3 {
            txtOTP2?.becomeFirstResponder()
        }
        if textField == txtOTP2 {
            txtOTP1?.becomeFirstResponder()
        }
        if textField == txtOTP1 {
            txtOTP1?.resignFirstResponder()
        }
        textField.text? = ""
        return false
    }
    return true
}

But it is very long and messy. Is there anyway I can make it simple?

Your solution to this particular problem is fine in its own way. However when it comes to scaling, it will become quite messy. Since I don't see any scope of improvement/tweaking in your approach, I suggest an alternative solution.

As pointed out in the comments, you can get what you want with Outlet Collections, since they are basically an array of UIKit objects, thus making it easy to manipulate them for our specific case.

Step 1

First off, connect your textfields to an Outlet Collection, and delegates in Interface Builder itself (as shown in the image below), which gets rid of the redundant

@IBOutlet var txtOTP1: UITextField!

and

txtOTP1.delegate = self

statements.

@IBOutlet var textFieldsOutletCollection: [UITextField]!

Step 2

We are going to use a dictionary with the textfield as the key and its corresponding index in the outlet collection as the value:

var textFieldsIndexes:[UITextField:Int] = [:]

override func viewDidLoad() {
    for index in 0 ..< textFieldsOutletCollection.count {
        textFieldsIndexes[textFieldsOutletCollection[index]] = index
    }
    super.viewDidLoad()
}

Step 3

The basic idea is, we can get the index of the textfield in our collection by supplying (to the dictionary) the textfield object we get in shouldChangeCharactersIn method, and then make the next or previous textfield the first responder:

enum Direction { case left, right }

func setNextResponder(_ index:Int?, direction:Direction) {

    guard let index = index else { return }

    if direction == .left {
        index == 0 ?
            (_ = textFieldsOutletCollection.first?.resignFirstResponder()) :
            (_ = textFieldsOutletCollection[(index - 1)].becomeFirstResponder())
    } else {
        index == textFieldsOutletCollection.count - 1 ?
            (_ = textFieldsOutletCollection.last?.resignFirstResponder()) :
            (_ = textFieldsOutletCollection[(index + 1)].becomeFirstResponder())
    }

}

Step 4

Now the shouldChangeCharactersIn method becomes more simple and clean:

func textField(_ textField: UITextField, shouldChangeCharactersIn range: NSRange, replacementString string: String) -> Bool {
    if range.length == 0 {
        setNextResponder(textFieldsIndexes[textField], direction: .right)
        textField.text = string
        return true
    } else if range.length == 1 {
        setNextResponder(textFieldsIndexes[textField], direction: .left)
        textField.text = ""
        return false
    }
    return false
}

Final Code

With all the above code combined, your view controller should look like this:

import UIKit

class ViewController: UIViewController, UITextFieldDelegate {

    @IBOutlet var textFieldsOutletCollection: [UITextField]!

    var textFieldsIndexes:[UITextField:Int] = [:]

    override func viewDidLoad() {
        super.viewDidLoad()
        for index in 0 ..< textFieldsOutletCollection.count {
            textFieldsIndexes[textFieldsOutletCollection[index]] = index
        }
    }

    enum Direction { case left, right }

    func setNextResponder(_ index:Int?, direction:Direction) {

        guard let index = index else { return }

        if direction == .left {
            index == 0 ?
                (_ = textFieldsOutletCollection.first?.resignFirstResponder()) :
                (_ = textFieldsOutletCollection[(index - 1)].becomeFirstResponder())
        } else {
            index == textFieldsOutletCollection.count - 1 ?
                (_ = textFieldsOutletCollection.last?.resignFirstResponder()) :
                (_ = textFieldsOutletCollection[(index + 1)].becomeFirstResponder())
        }

    }

    func textField(_ textField: UITextField, shouldChangeCharactersIn range: NSRange, replacementString string: String) -> Bool {

        if range.length == 0 {
            textField.text = string
            setNextResponder(textFieldsIndexes[textField], direction: .right)
            return true
        } else if range.length == 1 {
            textField.text = ""
            setNextResponder(textFieldsIndexes[textField], direction: .left)
            return false
        }

        return false

    }

}

This method should theoretically work for infinite number of UITextFields with same amount of code as opposed to your approach, in which the amount of code is dependent on the number of textfields used, which is not handsome/pretty.

I would recommend using a single text box have the same background and then have letter spacing. This would give u less amount of code and better visibility.


Find all the positive divisors of a positive integer This came from this question.

Find all the positive divisors of an integer >= 2.

Can stop when i * i >= number.

Please review for speed and style.

public class IntWithDivisors
{
    public override int GetHashCode()
    {
        return Number;
    }
    public override bool Equals(object obj)
    {
        if(obj is IntWithDivisors)
        {
            return ((IntWithDivisors)obj).Number == this.Number;
        }
        return false;
    }
    public override string ToString()
    {
        return $"number {Number}    Devisors " + string.Join(", ", Divisors);
    }

    public List<int> Divisors { get; } = new List<int>();
    public int Count { get { return Divisors.Count(); } }
    public int Number { get; }
    public IntWithDivisors(int number)
    {
        if (number < 2)
        {
            throw new ArgumentOutOfRangeException();
        }
        Number = number;
        Divisors = IntDivisors(number);
    }
}
public static List<int> IntDivisors(int num)
{
    //Debug.WriteLine($"\nIntDevisors  {num}");
    List<int> intDivisors = new List<int>();
    intDivisors.Add(1);
    intDivisors.Add(num);
    int i;
    int incr;
    if(num / 2 * 2 == num)
    {
        i = 2;
        incr = 1;
    }
    else
    {
        i = 3;
        incr = 2;
    }
    for(; i*i < num; i += incr)
    {
        int numOveri = num / i;
        if (numOveri * i == num)
        {
            //Debug.WriteLine(i);
            intDivisors.Add(i);
            intDivisors.Add(numOveri);
        }
    }
    if(i*i == num)
    {
        intDivisors.Add(i);
    }
    intDivisors.Sort();
    return intDivisors;
}

Why is the implementation of the algorithm a static method outside of the class IntWithDivisors? Is it in class Program? It shouldn't be there.

It is unusual to do complex work in the constructor. A constructor should only perform initialization. See answer of Telastyn in how complex a constructor should be. There are different possibilities to solve this

E.g. lazy evaluation:

private List<int> _divisors;
public List<int> Divisors
{
    get {
        if (_divisors == null) {
            _divisors = IntDivisors(Number);
        }
        return _divisors;
    }
}

Another option is to simply call a method to get the result.

This leads us to the next question: what is the task of the class IntWithDivisors? Do we really need to store the input number together with the output? Do we really need to override Equals and GetHashCode?

I would rather opt for a minimalist but reusable and flexible approach in the LINQ style as extension method implemented as iterator.

I observed that the starting i is one bigger than incr. We can use this fact to simplify the initialization.

Testing if a number is even is usually done with the modulo operator which yields the remainder of the division num % 2 == 0.

numOveri is a strange name. I renamed it to quotient and i to divisor.

The divisors are tested in ascending order; however, the quotients accumulate in descending order. Therefore, we can return the divisors immediately with yield return and store the quotients in a list. We then need to reverse this list before we return its items.

public static class IntExtensions
{
    public static IEnumerable<int> SelectDivisors(this int num)
    {
        yield return 1;

        int incr = num % 2 == 0 ? 1 : 2;
        var largeDivisors = new List<int>();
        for (int divisor = incr + 1; divisor * divisor <= num; divisor += incr) {
            int quotient = num / divisor;
            if (quotient * divisor == num) {
                yield return divisor;
                if (quotient != divisor) {
                    largeDivisors.Add(quotient);
                }
            }
        }

        largeDivisors.Reverse();
        for (int k = 0; k < largeDivisors.Count; k++) {
            yield return largeDivisors[k];
        }

        yield return num;
    }
}

We can use this extension method like this (shown in a little test routine):

int[] numbers = new[] { 9, 12, 15, 16, 17, 27, 54 };
foreach (int num in numbers) {
    Console.Write($"Number {num} has divisors ");
    foreach (int n in num.SelectDivisors()) {
        Console.Write(n + " ");
    }
    Console.WriteLine();
}

What I ended up with

public static class IntExtensions
{
    public static IEnumerable<int> SelectDivisors(this int num)
    {
        yield return 1;

        int incr = (num / 2 * 2 == num) ? 1 : 2;
        var largeDivisors = new List<int>();
        for (int smallDivisor = incr + 1; smallDivisor * smallDivisor <= num; smallDivisor += incr)
        {
            int largeDivisor = num / smallDivisor;
            if (largeDivisor * smallDivisor == num)
            {
                yield return smallDivisor;
                if (largeDivisor != smallDivisor)
                {
                    largeDivisors.Add(largeDivisor);
                }
            }
        }

        for (int k = largeDivisors.Count - 1; k >= 0; k--)
        {
            yield return largeDivisors[k];
        }

        yield return num;
    }
}


Powershell cmdlet authenticating to Aerohive API After dipping my toes in PowerShell for over a year i think it is time for creating a real cmdlet. My first mission was creating a regular PS script to take in a .csv and create a wifi account using the Aerohive REST api.

Although it works, i find the script bloated and ugly, and want to swap large code block for proper cmdlets that can also be used by my colleagues.

This is the first step, authenticating against the api. At first i'm only trying to get the basic functions to work. In a later phase i'll add error handling and stuff.

For now i'm also using restsharp. If i ever complete this, i want to swap it for the builtin .net classes so there are no external dependencies.

The next cmdlets will be for creating and deleting wifi accounts. But at first i wanna know if i'm using the correct syntax, proper use of variables, and if everything is secure enough.

using System;
using System.Management.Automation;
using RestSharp;

namespace Aerohive
{
    [Cmdlet(VerbsCommon.New, "Token")]
    [OutputType(typeof(Token))]
    public class NewTokenCmdlet : PSCmdlet
    {
        [Parameter(
            Mandatory = true,
            ValueFromPipelineByPropertyName = true,
            HelpMessage = "The api key, as provided by the IDManager web interface.")]
        public string apiKey { get; set; }

        [Parameter(
            Mandatory = true,
            ValueFromPipelineByPropertyName = true,
            HelpMessage = "The username you use to login on the web interace of the IDManager")]
        public string username { get; set; }

        [Parameter(
            Mandatory = true,
            ValueFromPipelineByPropertyName = true,
            HelpMessage = "The password you use to login on the web interface of the IDManager")]
        public string password { get; set; }

        protected override void BeginProcessing()
        {
            base.BeginProcessing();
        }

        protected override void ProcessRecord()
        {
            WriteVerbose("Authenticating ...");

            var client = new RestClient();
            client.BaseUrl = new Uri("very.long.url/to/authenticate/against");

            var request = new RestRequest("authenticate", Method.POST);
            request.AddHeader("content-type", "application/json");
            request.AddJsonBody(new
            {
                token = apiKey,
                userName = username,
                password = password
            });

            WriteVerbose("response");
            IRestResponse<Token> response = client.Execute<Token>(request);
            WriteObject(response.Data);
        }

        protected override void EndProcessing()
        {
            base.EndProcessing();
        }
    }
    public class Token
    {
        public string token { get; set; }
        public int errorCode { get; set; }
        public string errorMessage { get; set; }
    }
}

You are writing a powershell commandlet... are you sure you want to hardcode the url?

client.BaseUrl = new Uri("very.long.url/to/authenticate/against");

I think it would be better to provide a parameter for this one too. At least make it optional with a default value if you don't want to type it everythime but hardcoding it - well, I woudn't.

It doesn't take long and you'll find another use for it but then you'll say if I only made a parameter for this :-)

What is the point of doing this?

  protected override void EndProcessing()
    {
        base.EndProcessing();
    }

You're simply delegating to the superclass, so there's no sense in overriding the method. You can remove this.


Picking weighed enumerable values at random I have a number of enumerables, and I want to randomly pick one value from one of the enumerables so that each value has a certain weight in the randomisation process. This is simplification of my current code:

private enum ActionPlan //Just an example, there can be many different enums
{
    DoNothing,
    Eat,
    StealFoodCovertly,
    AttackAndSteal,
}

private ActionPlan PickPlan()
{
    double[] odds = new double[Enum.GetNames(typeof(ActionPlan)).Length];
    odds[(int)ActionPlan.DoNothing] = 2.5;
    odds[(int)ActionPlan.Eat] = 3;
    odds[(int)ActionPlan.StealFoodCovertly] = 1;
    odds[(int)ActionPlan.AttackAndSteal] = 0.5;

    ActionPlan plan = (ActionPlan)PickOption(odds);
    return plan;
}

private int PickOption(double[] odds)
{
    double totalOdds = 0;
    foreach (double odd in odds)
    {
        totalOdds += odd;
    }
    double pick = rand.NextDouble() * totalOdds;
    for (int i = 0; i < odds.Length; ++i)
    {
        pick -= odds[i];
        if (pick <= 0)
        {
            return i;
        }
    }
    return odds.Length - 1; // In case there are some precision shenanigans
}

I can't help but to think I'm trying to force a square peg into a round hole with all the casting going on. Is there some better way to accomplish this functionality? The idea was that the PickOption method can handle any enumerable and odds I throw at it.

I understand this question is rather open ended, but it doesn't hurt to ask, I suppose.

You're right that all the casting is a sign that there's better way.  Rather than using the int value of an ActionPlan as the index to an array, you should use a structure that's designed to store two values in the first place.  There's a couple of alternatives here.

My first instinct is to use a Dictionary<ActionPlan, double>.  That most explicitly matches the concept of what you're trying to do (each A has a value B), but it's not actually the best option.  You're not going to be looking up the value based on the key, the order of the keys may change which could lead to hard-to-reproduce bugs, and it's less memory-efficient.

Instead, I'd use a List<KeyValuePair<ActionPlan, double>>, List<Tuple<ActionPlan, double>>, or (in C# 7) List<(ActionPlan, double)>.  They're all effectively the same thing in this case, and best convey the concept "here is a pair of related values" without implying the A->B relationship of the Dictionary.  (Note that you can also use arrays instead of List<>, like I do below).  To make PickOption() handle this, you would need to make it generic, so that you can pass any arbitrary type in and get that same type out.

Here's what the code would look like using C# 7's ValueTuples:

private ActionPlan PickPlan()
{
    (ActionPlan, double)[] odds = new[] {
        (ActionPlan.DoNothing, 2.5),
        (ActionPlan.Eat, 3),
        (ActionPlan.StealFoodCovertly, 1),
        (ActionPlan.AttackAndSteal, 0.5),
    };

    ActionPlan plan = PickOption(odds);
    return plan;
}

private T PickOption<T>((T, double)[] odds)
{
    double totalOdds = 0;
    foreach (var odd in odds)
    {
        totalOdds += odd.Item2;
    }
    double pick = rand.NextDouble() * totalOdds;

    for (int i = 0; i < odds.Length; ++i)
    {
        pick -= odds[i].Item2;
        if (pick <= 0)
        {
            return odds[i].Item1;
        }
    }
    return odds.Last().Item1; // In case there are some precision shenanigans
}

or better:

private T PickOption<T>((T Action, double Weight)[] odds)
{
    double totalOdds = 0;
    foreach (var odd in odds)
    {
        totalOdds += odd.Weight;
    }
    double pick = rand.NextDouble() * totalOdds;

    foreach (var odd in odds)
    {
        pick -= odd.Weight;
        if (pick <= 0) return odd.Action;
    }
    return odds.Last().Action; // In case there are some precision shenanigans
}

There are other improvements that can be made here, too, depending on how much you simplified for this question, and how far you want to go.  For a few examples:

Both methods can be made static, because they don't depend on anything specific to whatever class they're in.
Rather than redefining the odds each time that PickPlan() is called, you can create a static readonly (ActionPlan, double)[] odds and populate it once.  This also lets you do that right next to where your ActionPlan enum is defined, so that when you add a new enum value, the odds are right there.
If you have other properties associated with an ActionPlan, you may want to consider a class to consolidate them all, rather than let each place they're used keep track.

A different approach

Numbers do not need to be unique but it breaks access by number
(you will get the first of that number)

public enum Weighted { one = 1, two = 2, three = 3, four = 4, once = 1 }
static Random randWeighted = new Random();
static List<Weighted> weights = null;
public static Weighted GetWeighted()
{
    if (weights == null)
    {
        weights = new List<Weighted>();
        foreach (Weighted w in Enum.GetValues(typeof(Weighted)))
        {
            for (int i = 0; i < (int)w; i++)
            {
                weights.Add(w);
            }
        }
    }
    return weights[randWeighted.Next(weights.Count())];
}


Happy Number Program I wrote this program, to determine if any given number is happy, and would like to know of any improvements that can be made to make it better. I have tested it, but whether or not it works perfectly I have no idea, there might be some numbers that get through, although I doubt it.

import java.util.HashSet;
import java.util.Scanner;
import java.util.Set;

public class HappyNumber
{
    public static void main(String[] args)
    {
        System.out.print("Please enter a number: ");
        int number = new Scanner(System.in).nextInt(), value;

        Set<Integer> unique = new HashSet<Integer>();

        while (unique.add(number))
        {
            value = 0;
            for (char c : String.valueOf(number).toCharArray())
                value += Math.pow(Integer.parseInt(String.valueOf(c)), 2);
            number = value;
        }

        System.out.println(number == 1 ? "Happy" : "Not Happy");
    }
}

Apart from what @RubberDuck said, you can also do a bit less work by not
converting from/to String, but iterating over the digits one by one
directly.  I've also moved the value to where it is initialised, that
way its scope is better visible.

After making those changes I arrive at the following snippet.  isHappy
or (isHappyNumber) can be reused and the main function only does
interface stuff.

import java.util.HashSet;
import java.util.Scanner;
import java.util.Set;

public class HappyNumber
{
    public static boolean isHappy(int number)
    {
        Set<Integer> unique = new HashSet<Integer>();

        while (unique.add(number))
        {
            int value = 0;
            while (number > 0)
            {
                value += Math.pow(number % 10, 2);
                number /= 10;
            }
            number = value;
        }

        return number == 1;
    }

    public static void main(String[] args)
    {
        System.out.print("Please enter a number: ");
        int number = new Scanner(System.in).nextInt();
        System.out.println(isHappy(number) ? "Happy" : "Not Happy");
    }
}

public class HappyNumber {

public static void sumHappy(int number){
    int sum =0;
    int r =0;
    while(number>0){
        r = number%10;
        sum = sum + (r*r);
        number = number/10;
    }
    if (sum!= 1 && sum > 9){
        sumHappy(sum);
    }
    else if(sum == 1){
        System.out.println("Happy number");
    }
    else{
        System.out.println("Not a Happy number");
    }
}
public static void main(String[] args) {
    int num = 91;
    sumHappy(num);
}

}


Insert row into database using values from a html form submission I have a comments page on my site and I was wondering if it is vulnerable to some sort of sql attack or something else. At the moment users can type what they want, they can even write a php script and it will upload to the database. Is this a problem and how can I fix it.
This is the script I use:
$name= $_POST['name'];
$email= $_POST['e_mailaddress'];

if(isset($_POST['comments']) AND $_POST['comments']!=''){
    $comments= $_POST['comments'];
    $comments_sent = 'true';

$dbcn = new connection();

$sql= "INSERT INTO contact (name, email, comments) VALUES (:name, :email, :comments);";

$query = $dbcn->dbconnect()->prepare($sql);

$results = $query->execute(array(
":name"=> $name,
":email"=> $email,
":comments"=> $comments
));
}

With prepared statements, I don't see anything that would immediately make it susceptible to sql injection (this isn't to say there isn't something that is there at a deeper level - I'm not a security auditor nor intimately familiar with php).  The "or something else" part remains quite large.

Look at OWASP (Open Web Application Security Project) which goes through a large set of possible attack vectors through the web.

Such a page is susceptible to various scripting injection where someone inserts a script (or flash, or even something like an image from another site) to be executed on another client machine. From the OWASP top 10 - 2013 pdf the XSS attack can be seen and described on page A3 (page 9).

It is good that you used a paramterized query. I guess you are using pdo and mysqli, if I am not wrong they make a prepared statement and clean it before submitting it to your database.

To protect against sql injection make 2 classes.
One that confirms that a name is a name (could check its type that is a string, and length) and an email (check for length and use the made classes on the internet to check that u really an email). - That would be considered as creating a whitelist.

The second class would sanitize bad characters. Sanitize > < " ' % characters that are used for sql injection. The PDO/MSQLi framework that I think you are using, already does some of the filtering..one of your own could also help- That would be considered as creating a blacklist.

WhiteList is safer, but having both of them is good enough.

Make test on your code, if you dont know sql well and ways to use canocolisation to pass the filters, use a good tool which is called sqlmap.

Last note..SQL injection is possible in prepared statements. May be even a secondary sql injection!


Add multiples of 3 or 5 below 1000, Can this code be optimised. Project Euler #1 I have seen some solution here for the same problem, this and this I have found better ways to solve this problem. I just want to know how good or bad my code and way of approaching this problem is.

using System;

public class Program
{
    public static void Main()
    {
        int sum = 0;
        for (int i = 0; 3 * i < 1000; i++)
        {
            sum += 3 * i;

            if (5 * i < 1000 && (5 * i) % 3 != 0)
            {
                sum += 5 * i;
            }
        }
        Console.WriteLine(sum );
    }

The main problem

The main issue here is that you define i in function of it being a "factor of 3", and then also try to use this number as a "factor of 5". This doesn't make any sense - as there is no inherent relationship between being a threefold and a fivefold number.

It would've made sense if you were doing it for 3 and 6, for example, because 3 and 6 are related:

//i*3 is obviously always a multiple of 3
sum += 3 * i;

if ((3*i) % 2 == 0)
{
    //Now we know that i*3 is also a multiple of 6
}

But that is not the case here.

The for loop's readability

I understand your idea, you wanted to only iterate over factors of three which keep the multiple under 1000. While I will suggest to change this approach (later in the answer), your approach could've been written in a much more readable manner:

for( int i = 0 ; i < 1000 ; i+=3 )

This will iterate over i values of 0,3,6,9,12,15,... and will skip all values inbetween.

The benefit here is that you don't need to work with i*3 all the time, you can just use i itself.

You will need to iterate over the multiple of 5 separately. However, should you keep using this approach, I would always suggest splitting these loops anyway.

The algorithm

Your approach works, but it's not the easiest approach. If I put your approach into words:

  Add every multiple of 3 to the sum.

  Also add the multiple of 5, but only if it's still below 1000, and it's not already divisble by 3.

The issue here is in how you handle the multiples of five. You're working with an i that is defined as the allowed values for threefolds. For any i > 200, you're effectively having to manually exclude this value. You're using a different approach for the 5 than you are using for the 3, even though the logic is exactly the same. That's not a reusable approach.

Secondly, there is a readability problem. Your code should be trivially readable, and I simply wasn't able to understand your intention. I had to google what the question was before I could understand what your code was trying to achieve.

So let me offer a better approach, first putting it into words:

  Check every number from 0 to 1000 (not including 1000)
  If it is divisible by 3 or it is divisible by 5, then add it to the sum.

This can be put into code, step by step:

// Check every number from 0 to 1000 (not including 1000)
for(int i = 0; i < 1000; i++)
{
    var isDivisibleBy3 = i % 3 == 0;
    var isDivisibleBy5 = i % 5 == 0;

    //If it is divisible by 3 or it is divisible by 5
    if(isDivisibleBy3 || isDivisibleBy5)
    {
        //then add it to the sum
        sum += i;
    }
}

Note how the code exactly mirrors my algorithm description.

You don't need to use the booleans. I simply added them to simplify the example. if(i % 3 == 0 || i % 5 == 0) would be equally okay to use because it's still reasonably readable.

If the calculations become more complex, I suggest always using the booleans so you neatly break your algorithm down to small and manageable steps. It will do wonders for your code readability, and it does not impact performance (the compiler will optimize this in a release build).

A LINQ variation

This can be further shortened using LINQ:

var sum = Enumerable.Range(0,1000)
              .Where(i => i % 3 == 0 || i % 5 == 0)
              .Sum();

LINQ is just a nicer syntaxt, but it uses a for/foreach iteration in the background, so I suspect it won't be much more performant than the previous example. But I do consider this highly readable.

Maximizing performance

The previous suggestion maximizes readability, but it does so at the cost of performance, as it now has to loop over 1000 values and evaluate them all. You already linked several other answer that clearly dive deeper into the code in order to maximize the performance, I hope you can see that this dramatically impacts the readability.

For example:

public void Solve(){
    result = SumDivisbleBy(3,999)+SumDivisbleBy(5,999)-SumDivisbleBy(15,999);
}

private int SumDivisbleBy(int n, int p){
    return n*(p/n)*((p/n)+1)/2;
}

By itself, I would have no idea what this code does. I can sort of understand the intention of Solve(), but it's not quite apparent how SumDivisbleBy() works.

SumDivisbleBy() has effectively become impossible to maintain. If you needed to introduce a change, you would effectively have to reverse engineer it before you can alter it. This means that starting from scratch is the better option, which is clearly not a good thing.

However, when performance is the main focus, this is acceptable. I would, however, strongly urge you to document the algorithm in comments specifically to help future readers in understanding how/why this works.

Note that AlanT's answer contains a more readable variant of the SumDivisbleBy() method, which already helps a lot with understanding the algorithm. The clear naming used clarifies what the algorithm does, which is the main goal of writing readable code (explaining why something works is only a secondary goal and not always required).

i=0
i++
if i%3 OR i%5 then sum+=i
else if i%15 then sum-=i

By the way this would be hardcoded. If you want reusable then wouldn't you:

i=0
i++
a = (user input)
b = (user input)
if i%a OR i%b then sum+=i
else if i%(a*b) then sum-=i

Or something similar to that. Or does this code already account for the duplicate sums. example you don't want to 15+15 because you only need one 15.