Designing classes for non-standard arithmetics This question is inspired by http://anydice.com - a dice probability calculator web application.

Anydice language has three run-time types: a number, a sequence and a die. There is also a number of unary and binary operations defined on these types. Each binary operation can take any type pair (out of the 3) as arguments, there are separate definitions of what each operation does for each possible pair of types. Some operations are not commutative.

For simplicity let's consider a single binary operation, which I'll call OpAccess, or @. My goal is to design a set of classes that represent the run-time values, so that if the access operation is executed on two of such values the correct operation implementation (depending on argument types) is called.

The main problem is that at compile time it is not known yet what run-time type a value has, yet, it's required to dispatch the correct operation implementation during the run-time.

Let's start with defining the base class for our values:

abstract class Primitive
{
  public abstract Primitive OpAccess(Primitive right);
}

Our value will use itself as the left argument in the OpAccessoperation and accept the right argument as the parameter.

Having this base we can design our value classes as follows:

class Number : Primitive
{
  public override Primitive OpAccess(Primitive right)
  {
    return OpAccess((dynamic)right);
  }
  public Primitive OpAccess(Number right)
  {
    Console.WriteLine("Number @ Number");
    return null;
  }
  public Primitive OpAccess(Sequence right)
  {
    Console.WriteLine("Number @ Sequence");
    return null;
  }
  public Primitive OpAccess(Die right)
  {
    Console.WriteLine("Number @ Die");
    return null;
  }
}
class Sequence : Primitive
{
  public override Primitive OpAccess(Primitive right)
  {
    return OpAccess((dynamic)right);
  }
  public Primitive OpAccess(Number right)
  {
    Console.WriteLine("Sequence @ Number");
    return null;
  }
  public Primitive OpAccess(Sequence right)
  {
    Console.WriteLine("Sequence @ Sequence");
    return null;
  }
  public Primitive OpAccess(Die right)
  {
    Console.WriteLine("Sequence @ Die");
    return null;
  }
}
class Die : Primitive
{
  public override Primitive OpAccess(Primitive right)
  {
    return OpAccess((dynamic)right);
  }
  public Primitive OpAccess(Number right)
  {
    Console.WriteLine("Die @ Number");
    return null;
  }
  public Primitive OpAccess(Sequence right)
  {
    Console.WriteLine("Die @ Sequence");
    return null;
  }
  public Primitive OpAccess(Die right)
  {
    Console.WriteLine("Die @ Die");
    return null;
  }
}

Now if we run something like:

Primitive a = new Sequence();
Primitive b = new Die();
a.OpAccess(b);
b.OpAccess(a);

We will get:

Sequence @ Die
Die @ Sequence

Since all operation will be defined differently the number of resulting methods is not an issue, it's really this many different ways to do an operation.

What worries me more is that I had to duplicate

public override Primitive OpAccess(Primitive left)
{
  return OpAccess((dynamic)left);
}

in each class and do not have an easy way around it. Remember, it's just one operation we are considering here, there will be more than a dozen of those in reality. Also, I have no idea if the use of dynamic becomes a performance problem. (Manual dispatch with switch/case should be faster).

But maybe I'm attacking this problem from the completely wrong angle? What do you think?

Note: this is in the context of writing a parser and an interpreter for Anydice language.

Update

Additional research prompted by Peter Taylor's answer uncovered the following article, which is most illuminating: Double Dispatch is a Code Smell

You might want to define the base type like this

abstract class Primitive
{
    public Primitive OpAccess(Primitive right)
    {
        switch (right)
        {
            case Number number: return OpAccess(number);
            // ... other types
            default: throw new ArgumentOutOfRangeException();
        }
    }
    protected abstract Primitive OpAccess(Number right);
    // ... other OpAccess
}

where there is only one public method and the new C# 7 switch takes care of the dispatch and derived classes need to implement only the concrete protected overloads:

class Sequence : Primitive
{
    protected override Primitive OpAccess(Number right)
    {
        Console.WriteLine("Sequence @ Number");
        return null;
    }
    // ... other OpAccess
}

dynamic is no longer necessary.


Deserializing multiple elements from XElement I'm serializing multiple elements using this code:

public IEnumerable<Customer> GetCustomers(xmlString)
{
  var xml = XElement.Parse(xmlString);
  var customers = xml.Elements("Customer");
  var serializer = new XmlSerializer(typeof(Customer));
  return customers.Select(c =>
  {
    using (var reader = c.CreateReader())
      return (Customer)serializer.Deserialize(reader);
  });
}

I have this hunch that CreateReader is an expensive one, so:

Is there a way to serialize multiple elements out a collection of elements (without creating a Customers class)?
Do I need to dispose all those readers?
Any other way to read directly from XElement (should I use string)?

Update
I've initially asked the question with this code:

public IEnumerable<Customer> GetCustomers(xmlString)
{
  var xml = XElement.Parse(xmlString);
  var customers = xml.Elements("Customers");
  var serializer = new XmlSerializer(typeof(Customer));
  return customers.Select(c => (Customer)serializer.Deserialize(c.CreateReader()));
}

Your initial revision is fine. If you take a look at the implementation at Reference Source you'll see that the XmlReader returned by CreateReader is lightweight and that calling Dispose on it is not necessary.

...
var customers = xml.Elements("Customers");
...

It seems a little strange that you pick elements of type "Customers" and deserialize them as Customer. They should be "Customer" in the XML if they represent a single customer each.


Implementation of Sieve of Eratosthenes in Python The following is my implementation of Sieve of Eratosthenes. Can somebody please review it? Am I adhering to the algorithm and is it a reasonable implementation of it?

def SieveofEratosthenes(primeseries):

    i=1
    primeserieslist = []
    while(i<primeseries):
        i=i+1
        primeserieslist.append(i)

    x=2

    primeslist = []

    while x <= primeseries and x*2 <= primeseries:
        j=2
        while (j < primeseries):
            z = j*x
            if (z <= primeseries):
                if (z in primeserieslist):
                    primeserieslist.remove(z)
            j = j+1

        x=x+1

    print primeserieslist

SieveofEratosthenes(1000000)

Code review
Advice 1
primeslist is not used. Remove it from your code.
Advice 2
x=2: PEP 8 requires to put two spaces around the operator =.
Advice 3
Also PEP 8 complains about while (j < primeseries):. Those parentheses (()) are not required; remove them in order to reduce the visual clutter.
Advice 4
The identifier primeserieslist is misspelled: it should be prime_series_list. Actually, the role of that very list is to hold the actual prime sieve, so why not rename it simply to sieve?
Advice 5
if (z <= primeseries):
    if (z in primeserieslist):
        primeserieslist.remove(z)

could be written as
if z <= prime_series and z in prime_series_list:
    prime_series_list.remove(z)

Advice 6
SieveofEratosthenes: in Python, CamelCase identifiers are suggested to be used for naming classes. What comes to function naming, your name should be sieve_of_eratosthenes.
Advice 7
x = x + 1: you can write more succinctly x += 1.
Advice 8
Both operations prime_series_list.remove() and z in prime_series_list run in average linear time, and, thus, are inefficient. See below for more efficient implementation.
Alternative implementation
Your implementation works and seems correct, yet there is room for improvement code style -wise and efficiency-wise:
def sieve_of_eratosthenes(max_integer):
    sieve = [True for _ in range(max_integer + 1)]
    sieve[0:1] = [False, False]
    for start in range(2, max_integer + 1):
        if sieve[start]:
            for i in range(2 * start, max_integer + 1, start):
                sieve[i] = False
    primes = []
    for i in range(2, max_integer + 1):
        if sieve[i]:
            primes.append(i)
    return primes

I have set up this benchmark. From it, I get the output:

SieveofEratosthenes in 17560 milliseconds.
sieve_of_eratosthenes in 31 milliseconds.
Algorithms agree: True

Any question? Give me a message.


Sourcing data format from multiple different structures Problem

I want to read in the data to dictionary

person = {
    'name': 'John Doe',
    'email': 'johndoe@email.com',
    'age': 50,
    'connected': False
}

The data comes from different formats:

Format A.

dict_a = {
    'name': {
        'first_name': 'John',
        'last_name': 'Doe'
    },
    'workEmail': 'johndoe@email.com',
    'age': 50,
    'connected': False
}

Format B.

dict_b = {
    'fullName': 'John Doe',
    'workEmail': 'johndoe@email.com',
    'age': 50,
    'connected': False
}

There will be additional sources added in the future with additional structures.

Background

For this specific case, I'm building a Scrapy spider that scrapes the data from different APIs and web pages. Scrapy's recommended way would be to use their Item or ItemLoader, but it's ruled out in my case.

There could be potentially 5-10 different structures from which the data will be read from.

Implementation

/database/models.py

"""
Database mapping declarations for SQLAlchemy
"""

from sqlalchemy import Column, Integer, String, Boolean
from database.connection import Base

class PersonModel(Base):
    __tablename__ = 'Person'
    id = Column(Integer, primary_key=True)
    name = Column(String)
    email = Column(String)
    age = Column(Integer)
    connected = Column(Boolean)

/mappers/person.py

"""
Data mappers for Person
"""

# Abstract class for mapper
class Mapper(object):
    def __init__(self, data):
        self.data = data

# Data mapper for format A, maps the fields from dict_a to Person
class MapperA(Mapper):
    def __init__(self, data):
        self.name = ' '.join(data.get('name', {}).get(key) for key in ('first_name', 'last_name'))
        self.email = data.get('workEmail')
        self.age = data.get('age')
        self.connected = data.get('connected')

    @classmethod
    def is_mapper_for(cls, data):
        needed = {'name', 'workEmail'}
        return needed.issubset(set(data))

# Data mapper for format B, maps the fields from dict_b to Person
class MapperB(Mapper):
    def __init__(self, data):
        self.name = data.get('fullName')
        self.email = data.get('workEmail')
        self.age = data.get('age')
        self.connected = data.get('connected')

    @classmethod
    def is_mapper_for(cls, data):
        needed = {'fullName', 'workEmail'}
        return needed.issubset(set(data))

# Creates a Person instance base on the input data mapping
def Person(data):
    for cls in Mapper.__subclasses__():
        if cls.is_mapper_for(data):
            return cls(data)
    raise NotImplementedError

if __name__ == '__main__':
    from database.connection import make_session
    from database.models import PersonModel

    # Sample data for example
    dict_a = {
        'name': {
            'first_name': 'John',
            'last_name': 'Doe'
        },
        'workEmail': 'johndoe@email.com',
        'age': 50,
        'connected': False
    }
    dict_b = {
        'fullName': 'John Doe',
        'workEmail': 'johndoe@email.com',
        'age': 50,
        'connected': False
    }

    # Instantiate Person from data
    persons = [PersonModel(**Person(data).__dict__ for data in (dict_a, dict_b)]
    with make_session() as session:
        session.add_all(persons)
        session.commit()

Question

I have limited experience in Python programming and I'm building my first scraper application for a data engineering project that needs to scale to storing hundreds of thousands of Persons from tens of different structures. I was wondering if:

This is a good solution? What could be the drawbacks and problems down the line?
Currently I've implemented different subclasses for the mapping. Is there a convention or industry standard for these types of situations?

Update

For question 2, I found this question to be useful, but would still want to know if this approach in general is good.
Added style improvement suggestions from @Reinderien

Don't abuse inner lists

This:

self.name = ' '.join([data.get('name').get(key) for key in ['first_name', 'last_name']])

should be

self.name = ' '.join(data.get('name', {}}.get(key) for key in ('first_name', 'last_name'))

Note the following:

Generators don't need to go in a list if they're just being passed to a function (join) that needs an iterable
Give an empty dictionary as the default for the first get so that the second get doesn't explode
Use a tuple instead of the last list because the data are immutable

Use set logic

This:

return all(key in data for key in ('name', 'workEmail'))

is effectively asking "are both 'name' and 'workEmail' in data?" There's a better way to ask this - with a set.

needed = {'name', 'workEmail'}
return needed.issubset(set(data))

If data can be stored as a set once outside of this function, it will increase efficiency.

Read more here: https://docs.python.org/3/tutorial/datastructures.html#sets

Don't needlessly materialize generators

This:

# Instantiate Person from data
persons = [Person(data) for data in [dict_a, dict_b]]

# Store persons that fit the database model
persons = [PersonModel(**person.__dict__) for person in persons]

makes a generator, saves it to a list in memory, consumes that list, makes a second generator, and stores that generator in a second list in memory. Instead:

persons = [PersonModel(**Person(data).__dict__)
           for data in (dict_a, dict_b)]

Again, the last inner list should be a tuple.

Parsing heuristics

It's not useful to write separate parsing classes for formats A and B in this case, because they aren't declared by the API so have no meaning. Write a translation routine for every member you extract from the JSON. Do a series of attempts against known paths in the data to get the members out.


Suggestions on Python code to update/insert data in PRODUCTION system I need suggestions on improving this code, the code is working fine ,but i'm gonna deploy this to production system, so before that would like to ask the community for any improvements/suggestions.

The script insert/update MySQL database using the data from PostgreSQL DB.

I have tested with some sample values and both insert and Updates are working fine.

Note: Using Python 3.5

import psycopg2
import os
import time
import MySQLdb
import sys

from pprint import pprint
from datetime import datetime
from utils.config import Configuration as Config
from utils.postgres_helper import get_connection
from utils.utils import get_global_config

#ef db_connect():

# MySQLdb connection
try:
    source_host = 'magento'
    conf = get_global_config()
    cnx_msql = MySQLdb.connect(host=conf.get(source_host, 'host'),
                               user=conf.get(source_host, 'user'),
                               passwd=conf.get(source_host, 'password'),
                               port=int(conf.get(source_host, 'port')),
                               db=conf.get(source_host, 'db'))
    print('Magento MySQL DB Connected')
except mysql.connector.Error as e:
   print ("MYSQL: Unable to connect!", e.msg)
   sys.exit(1)

# Postgresql connection
try:
   cnx_psql = get_connection(get_global_config(), 'pg_dwh')
   print('DWH PostgreSQL DB Connected')
except psycopg2.Error as e:
   print('PSQL: Unable to connect!\n{0}').format(e)
   sys.exit(1)

# Cursors initializations
cur_msql = cnx_msql.cursor()
cur_msql_1 = cnx_msql.cursor()
cur_msql_2 = cnx_msql.cursor()
cur_psql = cnx_psql.cursor()
cur_psql_1 = cnx_psql.cursor()

now = time.strftime('%Y-%m-%d %H:%M:%S')

##################################################################################
update_sql_base="""select gr.email from unsubscribed gr
                   INNER JOIN nl_subscriber sn on sn.email=gr.email"""

msql_update_1="""UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""

msql_update_2="""UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""

cur_psql.execute(update_sql_base)

for row in cur_psql:
    email=row[0]
    cur_msql.execute(msql_update_1 %email)
    cur_msql_2.execute(msql_update_2 %email)
cnx_msql.commit()

##################################################################################

insert_sql_base="""select gr.email,c.customer_id,'',3,'',CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP),'','',CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP)
                   from unsubscribed gr
                   LEFT JOIN nl_subscriber sn on sn.email=gr.email
                   LEFT JOIN customers c on c.customer_email=gr.email
                   WHERE sn.email IS NULL"""

msql_insert_1="""INSERT INTO `nl_subscriber`(
            `email`,  `customer_id`,  `options`,  `status`,  `confirm_code`,  `timestamp`,  `ip`,  `store_id`,`confirmed_at`) SELECT %s, %s, %s, %s, %s, %s, %s, %s, %s"""

cur_psql_1.execute(insert_sql_base)

for row in cur_psql_1:
    print(msql_insert_1)
    cur_msql_1.execute(msql_insert_1, row)
cnx_msql.commit()

## Closing cursors'
cur_msql.close()
cur_psql.close()
cur_psql_1.close()
cur_msql_1.close()
cur_msql_2.close()
## Committing
#cnx_psql.commit()
## Closing database connections
cnx_msql.close()
cnx_psql.close()

#if __name__ == '__main__':
 #db_connect()

A couple thoughts.

When opening database or other resources, try to use a context manager (the with statement). At the end of your code you are closing a bunch of connections/cursors:

## Closing cursors'
## Closing cursors'
cur_msql.close()
cur_psql.close()
cur_psql_1.close()
cur_msql_1.close()
cur_msql_2.close()

## Closing database connections
cnx_msql.close()

But your code could crash while processing, all it takes is a network error. There is no guarantee this code will be executed.

One thing you can do is wrap the code in a try/finally block, and move those lines to the finally section. You don't have to have an except section. So I think it would be a good idea to use the finally for cleanup tasks.

Some lines (the SQL statements) are way too long and require scrolling eg:

insert_sql_base="""select gr.email,c.customer_id,'',3,'',CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP),'','',CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP)
                   from unsubscribed gr
                   LEFT JOIN nl_subscriber sn on sn.email=gr.email
                   LEFT JOIN customers c on c.customer_email=gr.email
                   WHERE sn.email IS NULL"""

You can reflow the text like this for example:

insert_sql_base = """SELECT gr.email,c.customer_id, '', 3, '',
CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP), '', '',
CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP)
FROM unsubscribed gr
LEFT JOIN nl_subscriber sn ON sn.email = gr.email
LEFT JOIN customers c ON c.customer_email = gr.email
WHERE sn.email IS NULL"""

Added some more spacing too. Have a look at PEP guidelines, for instance PEP8.

Also, one thing I like to do is add aliases to the expressions in SQL queries. You have some blanks ('') or hardcoded values (3) but it's not immediately apparent what they represent.

I think that you could reduce the number of variables. Too many similar-looking variables increases the risk of confusion.
Consider this code:

msql_update_1="""UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""

msql_update_2="""UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""

cur_psql.execute(update_sql_base)

for row in cur_psql:
    email=row[0]
    cur_msql.execute(msql_update_1 %email)
    cur_msql_2.execute(msql_update_2 %email)
cnx_msql.commit()

You could have one cursor and reuse it eg:

for row in cur_psql:
    email=row[0]
    msql_update = """UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""
    cur_msql.execute(msql_update %email)

    msql_update = """UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""
    cur_msql.execute(msql_update %email)
cnx_msql.commit()

Although I am not sure why you have two statements instead of just one. Again, use context managers whenever possible to limit scope as much as possible.

There are very few comments in the code, and the flow is not that straightforward because there is overlap. For example:

update_sql_base="""select gr.email from unsubscribed gr
                   INNER JOIN nl_subscriber sn on sn.email=gr.email"""

msql_update_1="""UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""

msql_update_2="""UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""

cur_psql.execute(update_sql_base)

for row in cur_psql:
    email=row[0]
    cur_msql.execute(msql_update_1 %email)
    cur_msql_2.execute(msql_update_2 %email)
cnx_msql.commit()

Note that between the definition of update_sql_base and its execution, you define msql_update_1, msql_update_2. Move these two lines, try to keep related code together.

A very modest change to better separate the two (includes proposed changes above):

update_sql_base = """SELECT gr.email FROM unsubscribed gr
INNER JOIN nl_subscriber sn ON sn.email = gr.email"""
cur_psql.execute(update_sql_base)

for row in cur_psql:
    email=row[0]
    msql_update = """UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""
    cur_msql.execute(msql_update %email)

    msql_update = """UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""
    cur_msql.execute(msql_update %email)
cnx_msql.commit()

But I find that the name update_sql_base is misleading. This is not an UPDATE statement but a SELECT statement. The names are not very helpful overall.

You can declutter the code by moving blocks of code to small functions then call them in sequence eg:

open_mysql_db()
open_postgress_db()
update_mysql_db()
update_postgress_db()
close_postgress_db()
close_mysql_db()

That would make the whole more readable and easier to follow.

Example:

def copy_subscribers(cnx_msql, cnx_psql):
    """Copy subscribers from MySQL to Postgress
    """

    try:
        cur_psql = cnx_psql.cursor()
        cur_msql = cnx_msql.cursor()

        # get subscriber list from MySQL
        insert_sql_base = """SELECT gr.email,c.customer_id, '', 3, '',
        CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP), '', '',
        CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP)
        FROM unsubscribed gr
        LEFT JOIN nl_subscriber sn ON sn.email = gr.email
        LEFT JOIN customers c ON c.customer_email = gr.email
        WHERE sn.email IS NULL"""
        cur_psql.execute(insert_sql_base)

        # insert susbcriber to Postgress
        msql_insert = """INSERT INTO `nl_subscriber`(`email`,  `customer_id`,
        `options`,  `status`,  `confirm_code`,
        `timestamp`,  `ip`,  `store_id`,`confirmed_at`)
        SELECT %s, %s, %s, %s, %s, %s, %s, %s, %s"""
        for row in cur_psql:
            print(msql_insert)
            cur_msql.execute(msql_insert, row)

    finally:
        cnx_msql.commit()
        cur_psql.close()
        cur_msql.close()

I don't know if the function description is accurate but this is an idea. Note that I am passing connection instances cnx_msql and cnx_psql as arguments to the function (I prefer to avoid global variables). The idea is to better isolate the blocks, also avoid the proliferation of variables and at the same time limit their scope.


Converting string to control I have an up-down numeric control on my form (attEdgeUpDown), which has value 0-8. I have eight checkboxes (attEdgeBox1 to attEdgeBox8). I want to display as many boxes as the numeric control shows.

Can this be done more efficiently?

private void attEdgeUpDown_ValueChanged(object sender, EventArgs e)
{
int edgeValue = Convert.ToInt32(attEdgeUpDown.Value);
    for (int i = 1; i <= 8; i++)
    {
    string checkboxName = "attEdgeBox" + Convert.ToString(i);
    Control[] checkboxControl = this.Controls.Find(checkboxName, true);
        if (i <= edgeValue)
            {
                (checkboxControl[0] as CheckBox).Visible = true;
            }
            else
            {
                (checkboxControl[0] as CheckBox).Visible = false;
                (checkboxControl[0] as CheckBox).Checked = false;
            }
    }
}

Unless you're working with an ancient version of .NET, Convert.ToString(i); is superfluous. You should be able to just do "attEdgeBox" + i;.

If you're certain that those checkboxes exist, you can do var checkBox = this.Controls.Find(checkboxName, true).FirstOrDefault() as CheckBox; and then work with checkBox instead of doing (checkboxControl[0] as CheckBox) repeatedly.

Note that CheckBox is a compound name, so checkboxName as a variable name is incorrect; it should be checkBoxName.

What type is attEdgeUpDown, and why does it return a value that isn't an int? Seems odd to me that a control that returns a value from 1 to 8 would return this in a non-numeric format.


For an array, check if an index exists where the sum of the elements to the left of it is equal to the sum of the elements right of it Problem

Extended from this HackerRank problem.

  Given an array A of length n, determine if there exists an element in the array such that the sum of the elements on
  its left is equal to the sum of the elements on its right.
  If there are no elements to the left/right, then the sum is considered to be zero.

For example, [ 1, 1, 1, 2 ], the sum of elements to the left and right of index 2 are equal.

Approach

The following description is for the left-to-right evaluation of the array of values. In my implementation, I do both the
left-to-right and right-to-left evaluations in the same loop.

Given an array of values, start with the first and last index. These will be the initial values of the left and
right sums and the initial left and right indices
Increment the left index by 1 and decrement the right index by 1

If the left index is less than the right index, add the value at the right index to the right sum
If the left index is greater than the right index, subtract the value at the left index from the right sum
If the left and right sums are the same, then return early
Else, add the value at the left index to the left sum

After exiting the loop, check if the left sum is equal to the right sum

Implementation

public class BalancedArraySumIdentifier {
    public static boolean isBalanced(int[] values) {
        int leftToRightLeftSum = 0;
        int leftToRightRightSum = 0;

        int rightToLeftRightSum = 0;
        int rightToLeftLeftSum = 0;

        for (int i = 0; i < values.length; i++) {
            int j = values.length - 1 - i;

            if (i < j) {
                leftToRightRightSum += values[j];
                rightToLeftLeftSum += values[i];
            } else if (i > j) {
                leftToRightRightSum -= values[i];
                rightToLeftLeftSum -= values[j];
            }

            if (leftToRightLeftSum == leftToRightRightSum || rightToLeftLeftSum == rightToLeftRightSum) {
                return true;
            }

            leftToRightLeftSum += values[i];
            rightToLeftRightSum += values[j];
        }

        return leftToRightLeftSum == leftToRightRightSum || rightToLeftLeftSum == rightToLeftRightSum;
    }
}

First of all, I don't think your final return statement is correct: As far as I can figure, it compares the total sum of the array numbers to 0, which is in no way relevant to the question, and it causes the program to output true for arrays like [-1,1] or [1,2,-3].

Also, as you have already discovered with the help of JS1's observation, leftToRightRightSum and rightToLeftLeftSum only contain valid values once i >= j - 1, or, in other words, once every number in the array has been iterated over by at least one of the two pointers. Likewise, once these two variables contain valid values, the two pointers combined again iterate over every number in the array to check whether one of them fulfills the condition stated in the task.

So your two pointers merely divide the work that could as well be done by one single pointer: Calculate the total sum of the numbers (first array iteration), and then go through the array and divide the total sum into left and right for every element (second array iteration). This might seem like more work because code-wise it requires two iterations whereas your code only requires one iteration, but on the other hand, your code does more work per iteration loop than this algorithm would do due to the presence of two pointer variables, so in the end, I guess that there wouldn't be much difference in performance.

public static boolean isBalanced(int[] values) {
    int totalSum = Arrays.stream(values).sum(); // first "iteration"

    for (int i = 0, leftSum = 0, rightSum = totalSum;
            i < values.length;
            i++) { //second iteration
        rightSum -= values[i];
        if (leftSum == rightSum) {
            return true;
        }
        leftSum += values[i];
    }

    return false;
}


C: QuickSort following the book "Schaum's Outlines" Please make some comment about this code of QuickSort.

My code is based on the example shown in this book.

#include <stdio.h>
#include <conio.h>
#include <math.h>

#define SIZE 12

struct StackItem
{
    int StartIndex;
    int EndIndex;
};
struct StackItem myStack[SIZE * SIZE];
int stackPointer = 0;

int myArray[SIZE] = {11,44,33,55,77,90,40,60,99,22,88,66};

void Push(struct StackItem item)
{
    myStack[stackPointer] = item;
    stackPointer++;
}

struct StackItem Pop()
{
    stackPointer--;
    return myStack[stackPointer];
}

int StackHasItem()
{
    if(stackPointer>0)
    {
        return 1;
    }
    else
    {
        return 0;
    }
}

void ShowStack()
{
    int i =0;

    printf("\n");

    for(i=0; i<stackPointer ; i++)
    {
        printf("(%d, %d), ", myStack[i].StartIndex, myStack[i].EndIndex);
    }

    printf("\n");
}

void ShowArray()
{
    int i=0;

    printf("\n");

    for(i=0 ; i<SIZE ; i++)
    {
        printf("%d, ", myArray[i]);
    }

    printf("\n");
}

void Swap(int * a, int *b)
{
    int temp = *a;
    *a = *b;
    *b = temp;
}

int Scan(int *startIndex, int *endIndex)
{
    int partition = 0;
    int i = 0;

    if(*startIndex > *endIndex)
    {
        for(i=*startIndex ; i>=*endIndex ; i--)
        {
            if(myArray[i]<myArray[*endIndex])
            {
                Swap(&myArray[i], &myArray[*endIndex]);
                *startIndex = *endIndex;
                *endIndex = i;
                partition = i;
                break;
            }
            if(i==*endIndex)
            {
                *startIndex = *endIndex;
                *endIndex = i;
                partition = i;
            }
        }
    }
    else if(*startIndex < *endIndex)
    {
        for(i=*startIndex ; i<=*endIndex ; i++)
        {
            if(myArray[i]>myArray[*endIndex])
            {
                Swap(&myArray[i], &myArray[*endIndex]);
                *startIndex = *endIndex;
                *endIndex = i;
                partition = i;
                break;
            }
            if(i==*endIndex)
            {
                *startIndex = *endIndex;
                *endIndex = i;
                partition = i;
            }
        }
    }

    return partition;
}

int GetFinalPosition(struct StackItem item1)
{
    struct StackItem item = {0};
    int StartIndex = item1.StartIndex ;
    int EndIndex = item1.EndIndex;
    int PivotIndex = -99;

    while(StartIndex != EndIndex)
    {
        PivotIndex = Scan(&EndIndex, &StartIndex);

        printf("\n");
    }

    return PivotIndex;
}

void QuickSort()
{
    int splitPoint = 0;
    struct StackItem item;
    struct StackItem item1={0};
    struct StackItem item2={0};

    item.StartIndex = 0;
    item.EndIndex = SIZE-1;

    Push(item);

    while(StackHasItem())
    {
        item = Pop();

        splitPoint = GetFinalPosition(item);

        if(splitPoint>=0 && splitPoint<=(SIZE-1))
        {
            if(item.StartIndex <= (splitPoint-1))
            {
                item1.StartIndex = item.StartIndex;
                item1.EndIndex = splitPoint-1;
                Push(item1);
            }
            if((splitPoint+1) <= item.EndIndex)
            {
                item2.StartIndex = splitPoint+1;
                item2.EndIndex = item.EndIndex;
                Push(item2);
            }
        }
    }
}

main()
{
    ShowArray();
    QuickSort();
    ShowArray();
}

You've implemented your own stack. C has a perfectly good stack, so don't make your own. Use recursive functions.

"Scan" means to look for something, but you modify the list while scanning. Its not a good name for the function

"GetFinalPosition" begins with Get which usually indicates the the function doesn't modify anything.


Neatest way to filter an array of strings by another array in ruby I have an array of strings which are file names files, and an array of allowed file types or suffixes allowed_types. I want to filter out files which don't end with an allowed suffix.

This does the trick, but I'm wondering if anyone can suggest a neater solution? Such as a way keep with ruby's usual streamlined style without using flag variables.

valid_files = Dir.entries(path).select do |f|
  valid = false
  allowed_types.each { |suffix| next valid = true if f.end_with? suffix }
  next valid
end

How about this?

valid_files = Dir.entries(path).select do |f|
  allowed_types.any? {|suffix| f.end_with? suffix}
end


Theory-based quiz grader using similarity I wrote a fairly simple program that grades theory-based quizzes. It is fairly straightforward. I would love to get insights and ideas on how it might be improved.
consine_similarity.py
"""This module uses consine distance to check the similarity between two sentences"""

from nltk.corpus import stopwords
from nltk.cluster.util import cosine_distance
from nltk.tokenize import word_tokenize

def consine_similarity(sent1: str, sent2: str) -> float:
    """Consine similarity between two sentences

        sent1: str
        sent2: str
    """

    sent1_list = word_tokenize(sent1)
    sent2_list = word_tokenize(sent2)
    stop_words = stopwords.words('english')
    all_words = list(set(sent1_list + sent2_list))

    vector1 = [0] * len(all_words)
    vector2 = [0] * len(all_words)

    for word in sent1_list:
        if word in stop_words:
            continue
        vector1[all_words.index(word)] += 1

    for word in sent2_list:
        if word in stop_words:
            continue
        vector2[all_words.index(word)] += 1

    return 1 - cosine_distance(vector1, vector2)

quiz.py
"""Theory-based quiz application"""

from dataclasses import dataclass
from random import shuffle
from consine_similarity import consine_similarity

@dataclass
class Quiz:
    """Quiz data"""

    quiz: str
    answer: str

    def __str__(self):
        return '{}'.format(self.quiz)

def start_quiz(quiz_list: list) -> None:
    """Start the quiz application"""

    shuffle(quiz_list)
    consine_list = []

    for quiz in quiz_list:
        print(quiz)
        answer = input('> ')
        consine_list.append(consine_similarity(quiz.answer, answer))
        print()

    grade = sum(consine_list) / len(consine_list) * 100

    print('Your grade is {:.2f}%'.format(grade))

if __name__ == "__main__":
    QUIZ_LIST = [
        Quiz(quiz='What is a computer?',
             answer='An electronic device for storing and processing data, \
                 typically in binary form, according to instructions given to it'),
        Quiz(quiz='What are logic gates?',
             answer='These are the basic building blocks of any digital system. It is an \
                  electronic circuit having one or more than one input and only one output'),
        Quiz(quiz='What is BIOS',
             answer='This is firmware used to perform hardware initialization during the booting \
                 process and to provide runtime services for operating systems and programs'),
    ]
    start_quiz(QUIZ_LIST)

There is no need to include stop words in the dictionary if they are filtered out of the vectors anyway. Thus, tokenization and stop words removal can be considered as a single step. Secondly, the vectors can be initialized with the correct values immediately by taking advantage of collections.Counter
from collections import Counter
from typing import List
from nltk.corpus import stopwords
from nltk.cluster.util import cosine_distance
from nltk.tokenize import word_tokenize

def tokenize_without_stop_words(sentence:str) -> List[str]:
    return [
        word
        for word in word_tokenize(sentence)
        if word not in stopwords.words('english')
    ]

def cosine_similarity(first_sentence: str, second_sentence: str) -> float:
    first_words = tokenize_without_stop_words(first_sentence)
    second_words = tokenize_without_stop_words(second_sentence)

    dictionary = list(set(first_words+second_words))

    def encode(words: List[str]) -> List[int]:
        word_counts = Counter(words)
        return [
            word_counts[word]
            for word in dictionary
        ]

    first_vector = encode(first_words)
    second_vector = encode(second_words)

    return 1 - cosine_distance(first_vector, second_vector)


Truncate port number from absolute Uri We had a requirement to remove the port number from the Request.Url.AbsoluteUr i.e

Actual:

  https://mysitename:443/Home/Index

Expected:

  https://mysitename/Home/Index

The code I used for this is

 string newUrl = context.Request.Url.AbsoluteUri.Replace(":" + context.Request.Url.Port, string.Empty);

This is working fine. but I'm eager to know is there any better way to do this?

Use UriBuilder and set Port to -1

Uri oldUri = new Uri("http://myhost:443/Home/Index");
UriBuilder builder = new UriBuilder(oldUri);
builder.Port = -1;
Uri newUri = builder.Uri;

In case you want to remove the port part only when it is default, e.g. 80 for http and 443 for https, use snippet below (credit goes to Chris for the idea)

static Uri RemovePortIfDefault(Uri uri) {
    if (uri.IsDefaultPort && uri.Port != -1) {
        UriBuilder builder = new UriBuilder(uri);
        builder.Port = -1;
        return builder.Uri;
    }
    else return uri;
}

  If the Port property is set to a value of -1, this indicates that the default port value for the protocol scheme will be used to connect to the host.

See: UriBuilder.Port Property


Encrypting a binary stream with RSA + AES in counter mode I'm writing a little script that encrypts a file using the user's SSH identity.

I read the RSA private key from ~/.ssh/id_rsa (default)
Use the private key to encrypt a random 32 byte symmetric key (PKCS#1 OAEP)
Encrypt a stream of data with AES-256 in counter mode (no extra padding)
Write all of this to the output stream in 65535 byte chunks
Append an HMAC-SHA256 of the ciphertext

The idea is that data is encrypted on a user's computer before being pushed to the cloud. I'm using the SSH identity so that there's no passwords to worry about. If the user's ssh private key is compromised, the data being encrypted is compromised as well.

If the user wants to use a password, they'll just encrypt their private key.

Here's the code:

from Crypto import Random
from Crypto.Cipher import PKCS1_OAEP
from Crypto.PublicKey import RSA
from Crypto.Cipher import AES
from Crypto.Util import Counter
from Crypto.Hash import HMAC, SHA256

def encrypt_stream_v1(identity, stdin, stdout):
    rng = Random.new()
    iv = rng.read(8)  # counter mode prefix
    key = rng.read(32)  # random AES-256 key
    # AES-256 in counter mode
    aes = AES.new(key, mode=AES.MODE_CTR, counter=Counter.new(64, iv))
    mac = HMAC.new(key, digestmod=SHA256)  # HMAC-SHA256 for ciphertext
    with open(identity) as f:
        # the AES-256 key is encrypte with the users rsa private key
        key = PKCS1_OAEP.new(RSA.importKey(f.read())).encrypt(key)
    # the output stream begins with the 8 byte iv, the length of the
    # encrypted AES-256 key in two bytes and the encrypted key itself
    stdout.write(iv + len(key).to_bytes(2, "big") + key)
    while True:
        # encrypt in chunks of 65535 bytes (a two byte integer)
        chunk = aes.encrypt(stdin.read(0xFFFF))
        if chunk:
            # write the length of the encrypted chunk and the chunk itself
            stdout.write(len(chunk).to_bytes(2, "big") + chunk)
            # update the hmac with the ciphertext
            mac.update(chunk)
        if len(chunk) < 0xFFFF:
            break
    # the two null bytes represent an empty chunk, i.e. the end of the
    # ciphertext, the last 32 bytes of the output stream are the
    # HMAC-SHA256 digest
    stdout.write(b"\x00\x00" + mac.digest())

So the file format is as follows:

7a 70 79 01              # magic bytes and version number (01)
2c 81 6a 25 97 9c 8b 68  # counter iv
01 00                    # length of...
46 3e ........ 47 55 7e  # symmetric key encrypted with public key
00 08                    # length of...
22 12 11 01 2d 66 2e 66  # first encrypted block (repeated)
00 00                    # zero-length block signifies end of stream
18 9a ........ 0f 5a 0e  # hmac of the ciphetext

The reason the IV is 8 bytes is the counter. In order to get a 128 bit counter (the block size of AES), we use an incrementing 64 bit integer (via Crypto.Util.Counter) and prepend the random 64 bit iv.

Relevant links/documentation:

PyCrypto.Util.Counter
PyCrypto.Cipher.PKCS1_OAEP
Counter mode on Wikipedia

Is there anything wrong with how I'm using RSA/OAEP or AES-256-CTR? Is there anything I can optimize with the file format? Any mistakes I'm not seeing?

Changes so far

I'm including the entire file header (magic bytes, encrypted key length, encrypted key) in the HMAC-SHA256 calculation
I'm using a random 128 bit number as the initial value for the counter instead of effectively 64 bits with zeros appended.

Update: The project is now on GitHub

Is there anything wrong with how I'm using RSA/OAEP or AES-256-CTR?

I wish I could answer that but I don't know enough crypto.
I hope somebody else can address this point for you.

  Is there anything I can optimize with the file format?

Nothing pops to mind. Same note as previous point.

  Any mistakes I'm not seeing?

Not really "mistakes", but I see a few minor things that could be improved.

Magic numbers

The chuck size 0xFFFF appears twice.
It would be better to put that in a variable,
perhaps CHUNK_SIZE at the package level.
That way if you ever change the value,
you only have to change in one place.

Duplicated logic

You do len(something).to_bytes(2, "big") twice.
I suppose both uses need to have consistent logic,
just like with magic numbers,
the best way to ensure consistency is to define things in one place,
rather than duplicating at two or more,
which risks inconsistent parallel edits in the future.

The approach

Once again, I'm no expert on this, but anyway...

  I'm using the SSH identity so that there's no passwords to worry about.

Makes perfect sense to me.
That is, the default private key should be protected by a passphrase,
normally managed by a key-chain manager,
and you can piggy-back on that.

  If the user's ssh private key is compromised, the data being encrypted is compromised as well.

That seems acceptable, because it's inevitable.


Building a modular data reader system Context

I have a project which has an importation module to import content of Excel files.

Originally, importation was possible only for .xls file (< 2003) through Interop assemblies through a pretty ugly non-modular architecture. But there was a need to support importation of .xlsx files.

I have refactored the module to build a system of data readers. Depending on the extension of the files that is imported, a data reader class is instantiated accordingly to be able to read the data from either an Excel <2003 file or an Excell >2003 file transparently.

So I have two classes Excel2003DataReader and OpenXmlDataReader both based on the following interface:

public interface IImporterDataReader: IDisposable
{
    object DataFile { get; }

    bool HasValue(string workSheetName, int rowIdx, int columnIdx);

    string GetValue(string workSheetName, int rowIdx, int columnIdx);

    List < RangeItem > GetRangeValues(string workSheetName, int rowIdx, int columnIdx, ExtractDirection direction = ExtractDirection.Horizontal, int amountExtraValues = 0);
}

The IImporterDataReader instance is then passed a parameter to the Import method:

void Import(XmlDocument calculator, IImporterDataReader dataSource, Dictionary<string, string> parameters = null)

So dataSource.GetValue() can be used inside the method without worrying what kind of Excel file is actually imported.

The problem

The problem is that this system is too tightly adressed to Excel files. What I need to be able to reader xml files or json files as well ?

I was thinking of abstracting the way the parameters are passed to the methods HasValue()and GetValue() so they could be different per data reader implementations: have workSheetName, row and column properties for excel files, have an xpath property for xml files, etc.

Could be something like this:

public interface IImporterDataReader: IDisposable
{
    object DataFile { get; }

    bool HasValue(IDataAccessor accessor);

    string GetValue(IDataAccessor accessor);

    // Let's forget about this one right now
    //List < RangeItem > GetRangeValues(string workSheetName, int rowIdx, int columnIdx, ExtractDirection direction = ExtractDirection.Horizontal, int amountExtraValues = 0);
}

Question

I haven't honestly tried implementing such a mechanism, this is still in my mind.

Is this an interesting path to go for to implement such a mechanism ?
Also what could the accessor look like ?

I have the feeling this is a bit too complicated for the purpose, but maybe not.

Since there should be a relation between your importer data reader and data accessor ( importer should know how to use data accessor to access data.) I suggest that you use generics to achieve such coherence.(Since each importer needs its unique data then IDataAccessors can't be replaced by each other thus I don't think that's a good idea to introduce an abstraction for them)

public abstract ImporterDataReader<TAccessor>:IImporterDataReader
{
public abstract bool HasValue(TAccessor accessor);

public abstract string GetValue(TAccessor accessor);
}

Then you can have concrete classes for each type of Accessor:

public class JsonDataReader:ImporterDataReader<JsonDataAccessor>
{
....
}


Avoid printing attributes of the nil object in Rails Currently, when I'm wanting to print a value for my class I do the following...

<%= @person.team.name if @person.team.present? %>

This seems really redundant to me.  I've also have done...

<%= @person.team.name_display %>

where I've created a function for each attribute to kind of hide the first case.  It seems a little much though. Is there a more preferred way to do it such as...

<%= @person.team.name || "" %>

You are right, you code is too verbose. This is a pretty common pattern and you have some alternatives. For example, active_support has the abstraction Object#try:

<%= @person.team.try(:name) %>

Another alternative is the Object#maybe proxy: https://github.com/bhb/maybe

<%= @person.team.maybe.name %>


Generating random sequences while keeping sum fixed The output is a random sequence, and the input is the sum of the sequence.

My solution is generating a random number rand_num from (0, sum_seq) at first, then draw another number randomly from (0, sum_seq - rand_num). By the way, all random numbers are integers.

import random

rand_list = [] # Random sequences buffer
def generater_random_list(num) :
    rand_num = random.randint(0, num)
    rand_list.append(rand_num)
    if num - rand_num == 0 :
        return 0
    else :
        return generater_random_list(num - rand_num)

It seems strange to create a buffer out of the function, so how can I improve it?

1. Comments on your code

The function stores its results in the global variable rand_list and always returns 0. This is a poor design of interface for two reasons. First, if you call generater_random_list twice, rand_list now contains the concatenation of the first and second lists:

>>> generater_random_list(10)
0
>>> rand_list
[2, 1, 5, 1, 0, 1]
>>> generater_random_list(10)
0
>>> rand_list
[2, 1, 5, 1, 0, 1, 9, 1]

(The caller could set rand_list = [] before the second call to generater_random_list, but it would be really annoying to have to keep remembering to do that.)

And second, what's the point of returning 0? That's no use to anyone. Python allows you just to write return as a shorthand for return None if you really have nothing worth returning, but here you do have something to return, namely the list of random numbers you just constructed.
There's no docstring. What does this function do and how do you call it?
You could choose a better name for the function than generater_random_list. (i) "Generator" is spelled with an "o". (ii) If you're going to name a function after its action, use the imperative form of the verb. (So use sort or multiply, rather than sorter or multiplier.) (iii) "Generator" has a specialized meaning in Python (referring to a generator), so best not to use it unless that's what you mean.
The Python style guide (PEP8) says, "Avoid extraneous whitespace immediately before a comma, semicolon, or colon."
When you're writing a function that produces a sequence of items, it's usually a good idea in Python to make it into a generator function that yields the items one by one (rather than a normal function that builds and returns a list of all the items). It's often convenient to process these items one by one, and a generator function allows you to do this without having to construct an intermediate list in memory. (And if you need a list, it's trivial to build one, by passing your generator to the function list.)

2. Revised code

def random_ints_with_sum(n):
    """
    Generate non-negative random integers summing to `n`.
    """
    while n > 0:
        r = random.randint(0, n)
        yield r
        n -= r

And here are some example calls:

>>> list(random_ints_with_sum(10))
[9, 1]
>>> list(random_ints_with_sum(10))
[10]
>>> list(random_ints_with_sum(10))
[0, 2, 8]

3. Comments on your design

Do you really mean to include 0 among the random integers? This suggests that you would be happy with

>>> list(random_ints_with_sum(1))
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]

and similar outputs. You don't say what you are using this function for, so I can't tell if this is right or wrong, but it looks well dodgy to me.
Even with 0 excluded, your implementation samples its range with a lot of bias. For example, the number 4 has eight ordered partitions into positive integers:

4   3, 1   2, 2   2, 1, 1   1, 3   1, 2, 1   1, 1, 2   1, 1, 1, 1

but your algorithm doesn't sample these partitions evenly. Here's a test program that calls random_ints_with_sum(4) a million times, and counts the number of times each result was generated:

>>> from collections import Counter
>>> Counter(tuple(random_ints_with_sum(4)) for _ in xrange(10 ** 6))

Here are the results:

Partition    Count
-----------  ------
4            249997
3, 1         250047
2, 2         124579
2, 1, 1      125113
1, 3          83425
1, 2, 1       83190
1, 1, 2       41875
1, 1, 1, 1    41774

If the partitions had been generated uniformly, each would have appeared about 125,000 times in the results. You can see that shorter partitions are generated more frequently, and longer partitions less frequently than in the uniform distribution.

Again, since you don't explain what your function is for, I can't tell whether this kind of bias is important or not.

4. Sampling partitions uniformly

Supposing that you wanted to sample the space of ordered integer partitions uniformly, how could you implement it?

First we have to know how to count ordered integer partitions. And to do that, there's a neat combinatorial observation. Consider a group of objects waiting to be partitioned:

If there are n objects, there are n − 1 locations where you could insert a partition (indicated by the dotted lines). For example, if we insert partitions at the first and the third locations, we get the partition 1, 2, 1:

Each subset of locations corresponds to a different ordered partition, and each ordered partition corresponds to a different subset of locations. This means that we can choose an ordered partition uniformly by choosing a subset of locations uniformly. There are 2n − 1 subsets of the n − 1 locations (for example, 4 objects have 23 = 8 ordered partitions, as noted above), and we can choose one of these subsets uniformly by putting each location into the subset with probability ½.

Here's a revised function that implements the new algorithm:

def random_ints_with_sum(n):
    """
    Generate positive random integers summing to `n`, sampled
    uniformly from the ordered integer partitions of `n`.
    """
    p = 0
    for _ in xrange(n - 1):
        p += 1
        if random.randrange(2):
            yield p
            p = 0
    yield p + 1

And here's an example distribution after a million calls to the revised function:

Partition    Count
-----------  ------
4,           125042
3, 1         124848
2, 2         125189
2, 1, 1      125126
1, 3         124861
1, 2, 1      125152
1, 1, 2      124248
1, 1, 1, 1   125534

You'll see that this is very close to uniform.

Again, I should emphasize that since I don't know what your function is going to be used for, I don't know whether changing it so that it samples uniformly from the ordered integer partitions is the right thing to do. But if it is, now you know how to do it.

(Who would have thought there would be so much to write about a six-line function?)

5. Chinese restaurant process

You updated your question to say that you are trying to simulate a Chinese restaurant process. I have to say that I can't relate the code in your question to this process, which has nothing to do with numbers that sum to a particular total. So I'm puzzled! Maybe you can say more about how the code you posted relates to this process?

Anyway, if you want to generate a sample from this distribution, the easiest thing to do is to simulate the process directly:

def chinese_restaurant_partition(n):
    """
    Return a random partition of the numbers from 1 to n generated by
    the Chinese restaurant process.
    """
    partition = []  # list of sets
    assignment = [] # set each number is assigned to
    for i in xrange(n):
        r = random.randrange(i + 1)
        if r < i:
            # assign i + 1 to existing block
            block = assignment[r]
        else:
            # create new block
            block = set()
            partition.append(block)
        block.add(i + 1)
        assignment.append(block)
    return partition

For example:

>>> chinese_restaurant_partition(4)
[set([1]), set([2, 4]), set([3])]

This distribution looks like this (again, after generating a million samples):

Partition   Count
----------  ------
1234        249231
134, 2       83453
124, 3       82664
123, 4       83569
1, 234       83161
14, 23       42126
13, 24       41688
12, 34       41726
14, 2, 3     41891
13, 2, 4     41569
12, 3, 4     42022
1, 24, 3     41522
1, 23, 4     41719
1, 2, 34     41867
1, 2, 3, 4   41792


Making a tree with hash input I initialized a tree using a nested hash as input, but when I recursively call Tree.new on the children, it doesn't seem to pass each child as a hash. As a pretty ugly, but working, alternative, I translated everything to arrays.

Is there a way to preserve the hash?

I'm doing this exercise using Ruby 1.9.3, so I can't use Ruby 2.0.0's to_h method.

class Tree
  attr_accessor :node_name, :children

  def initialize(arr)
    arr = arr.to_a unless arr.is_a?(Array)
    arr.flatten!
    @node_name = arr.first
    @children = []
    arr.last.each { |c| @children << Tree.new(c) }
  end

  def visit(&block)
    block.call self
  end

  def visit_all(&block)
    visit &block
    children.each { |c| c.visit_all(&block) }
  end
end

family_tree = Tree.new({'grandpa' => { 'dad' => { 'child 1' => {}, 'child 2' => {} },
                                      'uncle' => {'child 3' => {}, 'child 4' => {} } } })

family_tree.visit_all { |node| p node.node_name }

Some notes:

The initialize method of a class should be its most basic and fundamental constructor, don't use it as a "helper". A tree has a name and some children, so these should be the arguments. You want to construct a Tree from a hash to make things easier for the caller? then create a helper classmethod, for example Tree::new_from_hash(hash).
Tree#visit. That's an imperative construct, I'd write first the functional ones (which can also be used imperatively if needed).
node.node_name: That repetition is a cue that node_name is redundant, name is enough.
(Subjective) I prefer to write hash literals without losing indentation space.

I'd write (I'd inherit from Struct to save some lines, not really needed):

class Tree < Struct.new(:name, :children)
  def self.new_from_hash(hash)
    name, children_hash = hash.first
    children = children_hash.map { |k, v| Tree.new_from_hash({k => v}) }
    Tree.new(name, children)
  end

  def visit_all(&block)
    yield(self)
    children.each { |c| c.visit_all(&block) }
  end
end

family_tree = Tree.new_from_hash({
  'grandpa' => {
    'dad' =>   {'child 1' => {}, 'child 2' => {}},
    'uncle' => {'child 3' => {}, 'child 4' => {}},
  },
})

family_tree.visit_all { |node| puts node.name }

Additional notes:

Include module Enumerable and implement Tree#each so you can use all enumerable methods for you trees. Note that enumerable methods assume a single-level iteration, so you'd still need custom methods (like visit_all/map_all) that perform nested operations.
If that's a learning code I'd explore this path: create a NestedEnumerable module -very similar to Enumerable- which would provide some xyz_all (or xyz_nested, whatever) methods. Like Enumerable, you would only be required to implement the "each" method (i.e. each_all).


Jump Game - Leetcode Below is my solution for the LeetCode Jump Game

Given an array of non-negative integers, you are initially positioned
at the first index of the array.
Each element in the array represents your maximum jump length at that
position.
Determine if you are able to reach the last index.

Example 1:

Input: [2,3,1,1,4]
Output: true

This is my solution. It is correct but timed out the LeetCode code-checker for an array of 25000 elements.
Please can someone help me improve this code.
It seems to have a time complexity of O(N^2) and a space complexity of O(N). I cannot work out a method faster than O(N^2).
def function(nums):
    if len(nums) == 1:
        return True
    output = [0] * len(nums)
    last_index = len(nums) - 1
    output[0] = 1

    for index, number in enumerate(nums):
        if output[index] == 0:
            continue
        if output[index] == 1:
            stop_at = min(last_index, index + number)
            for i in range(index, stop_at + 1):
                output[i] = 1
        if output[last_index] == 1:
            return True
    return False

if __name__ == "__main__":
    nums = [3,2,1,0,4]
    output = function(nums)
    print(output)

O(n) Solution

You don't need to keep track of all the places you can reach.  You only need to know what is the highest index you can reach, because you can reach any lower index by choosing a shorter jump.  Scan the list from 0 to the end keeping track of the maximum reachable index.  If you ever reach an index greater than the maximum reachable index, you've hit a block and can't reach the end.

For example, for a jump list = [3,2,1,0,4].  Starting at index 0, we can reach any index up to 0 + 3 = 3.  From index 1 we can reach 1 + 2 = 3.  From 2, we can reach 2+1 = 3.  From index 3, we can reach 3 + 0 = 3.  So far the maximum reachable index is 3.  So when we reach index 4, 4 > 3 so index 4 is unreachable and we can't get to the end.

def canjump(nums):
    maximum_reachable_index = 0

    for index, max_jump in enumerate(nums):
        #print(f"{index}", end=' ')
        if index > maximum_reachable_index:
            #print(f"> {maximum_reachable_index} and is unreachable.")
            return False

        #print(f"+ {max_jump} => {index + max_jump} : {maximum_reachable_index} ")
        maximum_reachable_index = max(index + max_jump, maximum_reachable_index)

    return True

Uncomment the print statements to see how it works.


Refactoring Methods With Conditionals I have always suffered with too many conditionals in some of my methods. The following is a pseudocode/skeleton of one of my method like that:

private List<ReturnType> DoSth(string name = null)
{
    List<ReturnType> names = new List<ReturnType>();

    for (int i = 0; i < somearray.Length; i++)
    {
        if (somecondition)
        {
            if (name == null)
            {
                //'Add' to list
            }
            else
            {
                if (someothercondition)
                {
                    //'Add' to list
                }
            }
        }
    }

    return names;
}

Actual Code which returns an object filled with Controller and corresponding Action names:

private List<AppController> GetControllerActions(string ControllerName = null)
{
    Type[] typelist = Assembly.GetExecutingAssembly().GetTypes().Where(t => String.Equals(t.Namespace, "Project.Web.Controllers", StringComparison.Ordinal)).ToArray();

    List<AppController> names = new List<AppController>();

    for (int i = 0; i < typelist.Length; i++)
    {
        if (typelist[i].BaseType.Name == "Controller")
        {
            if (ControllerName == null)
            {
                names.Add(new AppController
                {
                    ControllerName = typelist[i].Name,
                    ActionName = typelist[i].GetMethods()
                    .Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
                    .Select(s => s.Name).ToList()
                });
            }
            else
            {
                if (typelist[i].Name == ControllerName)
                {
                    names.Add(new AppController
                    {
                        ControllerName = typelist[i].Name,
                        ActionName = typelist[i].GetMethods()
                        .Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
                        .Select(s => s.Name).ToList()
                    });
                }
            }
        }
    }

    return names;
}

As you can see the same Add code is being repeated twice and the 'Add' code is not an external method and I would prefer not making it one also. I can also provide the actual code if required.

Is there any scope of making this code more concise and compact like you can do with ternary operators for example?

You could just combine the logic into one statement, assuming there is not other things occurring during your if-else blocks and stuff.
If there is, then this won't necessarily work for you, and you should post the real code, because a lot of times these problems are subjective to the code.

I guess one lesson to possibly learn from this is just to realize the end result, and build a condition that matches the necessary conditions to do something.

if (somecondition && (name == null || someothercondition))
{
    //'Add' to list
}

Edit: To expand upon the OPs full code being posted.

My original answer is still valid you would write it like this:

for (int i = 0; i < typelist.Length; i++)
{
    if (typelist[i].BaseType.Name == "Controller" && (ControllerName == null || typelist[i].Name == ControllerName))
    names.Add(new AppController
    {
        ControllerName = typelist[i].Name,
        ActionName = typelist[i].GetMethods()
        .Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
        .Select(s => s.Name).ToList()
    });
}

However, now that you have actual code to review, I will also point out that there is no reason to be using a for loop vs a foreach loop. I would write that as follows.

foreach (var t in typelist)
{
    if (t.BaseType.Name == "Controller" && (ControllerName == null || t.Name == ControllerName))
    names.Add(new AppController
    {
        ControllerName = t.Name,
        ActionName = t.GetMethods()
        .Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
        .Select(s => s.Name).ToList()
    });
}

At this point I am recognizing that we are iterating through a list, and conditionally doing one thing. This appears to be the perfect candidate for using some linq.

using Where and some lambda we can make this statement to return an IEnumerable<Type> which only contains Types we need to add.

typelist.Where(t => t.BaseType.Name == "Controller" && (ControllerName == null || t.Name == ControllerName))

On our newly created IEnumerable<Type> we can Select what we want to add to the names list.

.Select(t => new AppController
{
    ControllerName = t.Name,
    ActionName = t.GetMethods()
    .Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
    .Select(s => s.Name).ToList()
});

Now if this was a time where names was an existing list we only needed to append to we could use names.AddRange() but because this list was created here for this purpose (I think).
we can just ToList() our results and save them as the list.

At this point you could have a few different variables, or you could make your code look awesome and confusing and one-line it all. because I see now that typelist is only being referenced once. (Btw it should probably named typeList, with a capital L, but we're getting rid of it anyway).

So in the very end you can have this beautiful chunk of code. Note: I am also going to use string.IsNullOrWhiteSpace instead of just checking if it is null.

private List<AppController> GetControllerActions(string ControllerName = null)
{
    return
        System.Reflection.Assembly.GetExecutingAssembly().GetTypes().Where(t => String.Equals(t.Namespace, "Project.Web.Controllers", StringComparison.Ordinal))
        .Where(t => t.BaseType.Name == "Controller" && (string.IsNullOrWhiteSpace(ControllerName) || t.Name == ControllerName))
        .Select(t => new AppController
        {
            ControllerName = t.Name,
            ActionName = t.GetMethods()
            .Where(w => w.ReturnType.BaseType != null && (w.ReturnType.BaseType.Name == "ActionResult" || w.ReturnType.Name == "ActionResult"))
            .Select(s => s.Name).ToList()
        })
        .ToList();
}


Read DOM and send values to server I have this code that basically reads the DOM and sends the values to the server. I am looking for any possible flaws that I may have in this JavaScript code and any advice to make it better and bug-free!

$(document).ready(function () {
    $('.editButton').click(function () {
        var postData = {};
        var cData = {};
        cData.Balance = $(this).parent().children('.balance').text().replace("$", "");
        cData.desiredStatus = $(this).parent().children('.status').text() == "Disbled" ? "enable" : "disable";
        $('#currentBalance').html($(this).parent().children('.balance').text());
        $('#currentStatus').html($(this).parent().children('.status').text());
        $('#desiredStatus').html(cData);

        //set post view
        postData.code = $(this).parent().children('.code').text();
        postData.giftcardaccount_id = $(this).parent().children('.giftcardaccount_id').text();

        $("#dialog").dialog({
            title:"test box",
            modal:true,
            width:700,
            buttons:{
                'Confirm':function () {
                    postData.status = $('#currentStatus').html();
                    var balanceInqury = parseFloat($('#desiredBlance').val());
                    postData.balance = (balanceInqury == 'NaN') ? cData.Balance : balanceInqury;
                    $.ajax({
                        url:'/url/to/postto',
                        type:'POST',
                        data:{
                            postData:postData
                        },
                        success:function (data) {
                            console.log(data);
                            $("#dialog").dialog("close");

                        },
                        error:function () {
                        }
                    });
                },
                Cancel:function () {
                    $(this).dialog("close");
                }
            }
        });//dialog
    });

    $('#changeStatus').click(function () {
        var c = $('#currentStatus').html() == 'Disabled' ? 'Enabled' : 'Disabled';
        var d = $('#desiredStatus').html() == 'disable' ? 'enable' : 'disable';
        $('#currentStatus').html(c);
        $('#desiredStatus').html(d);

    });

});

You're creating a lot of jQuery objects in there. It would be much more efficient to just create them once. For example:

var postData = {},
    cData = {},
    $this = $(this),
    $parent = $this.parent(),
    $balance = $parent.children(".balance");

As a general rule, I try to avoid calling any method on the same object more than once. For any methods you need to call repeatedly, just call it once and store the result.

The following line seems to be incorrect. You're passing an object to the .html() method (which it doesn't accept):

$('#desiredStatus').html(cData);

//Did you mean to do this instead?
$('#desiredStatus').html(cData.desiredStatus);

You are creating a new modal dialog every time a .editButton element is clicked. You could move the creation of the dialog outside of the event handler and just call $("#dialog").dialog("open") when you want it to appear.
The comparison balanceInqury == 'NaN' will never be true. JavaScript is funny like that, NaN !== NaN. There is a built-in isNaN function you can use though:

postData.balance = isNaN(balanceInqury) ? cData.Balance : balanceInqury;

For shorter code you could replace your call to $.ajax with a call to the shorthand $.post method. If you'd rather not do that, you can at least remove the error property from the options object, since it doesn't do anything in your case:

$.post('/url/to/postto', { postData: postData}, function (data) {
    //You make another unnecessary jQuery object in here. Cache the #dialog object!
});

Finally, you could change the code in the #changeStatus click event handler by passing a function to the .html method, which in my opinion is a bit neater:

$('#currentStatus').html(function () {
    return $(this).html() === "Disabled" ? "Enabled" : "Disabled"
});
$('#desiredStatus').html(function () {
    return $(this).html() === "disable" ? "enable" : "disable";
});


Handling errors in a Python UDP proxy The following (stripped) program receives UDP packets on port 5000 and forwards them to port 5001. It may throw socket errors on startup (e.g.: if the port is already in use), but I am not too worried about that.

import socket

RECEIVE_IP = "0.0.0.0"
RECEIVE_PORT = 5000
RECEIVE_BUFFER_SIZE = 4096

SEND_IP = "127.0.0.1"
SEND_PORT = 5001

send_socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)

receive_socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
receive_socket.bind((RECEIVE_IP, RECEIVE_PORT))

while True:
    string = receive_socket.recv(RECEIVE_BUFFER_SIZE)
    send_socket.sendto(string, (SEND_IP, SEND_PORT))

I would like to make sure the program keeps running after a successful startup. Is it enough to try/catch the socket errors?

while True:
    try:
        string = receive_socket.recv(RECEIVE_BUFFER_SIZE)
        send_socket.sendto(string, (SEND_IP, SEND_PORT))
    catch socket.error:
        print 'Oops!'

Or is there anything else I need to worry about? Would I ever need to rebind receive_socket for instance?

Bonus question: the real problem here is that I am not aware how to test this program for failure. Would anyone have a recommendation?

On the face of it, what you're suggesting seems like it should work (although it may be implementation dependant since it depends on the OS socket API).  It also depends on what you mean by 'stripped down'.  Obviously, if you're doing something with the received packets that might throw an error you'll need to handle that as well.  Your catch would be better written as:

except socket.error:
    print ('Oops!')

As far as testing goes, you're sending to the same IP address so should be pretty safe but some things to try are:

Pull out / reconnect network cables
Turn off / reenable Wi-Fi
Disable / reenable network adapters
Send unexpectedly large packets (over your 4096 buffer size)
Try running multiple clients in constant send loops to see how your app handles large demand.


Equivalent binary trees (A Tour of Go) Any suggestions on how to improve the code shown below for the Go-tour exercise?

Exercise description:

  There can be many different binary trees with the same sequence of
  values stored at the leaves.  A function to check whether two binary
  trees store the same sequence is quite complex in most languages.
  We'll use Go's concurrency and channels to write a simple solution.

  This example uses the tree package, which defines the type:

type Tree struct {
    Left  *Tree
    Value int
    Right *Tree
}

  Implement the Walk function.
  Test the Walk function.

  The function tree.New(k) constructs a randomly-structured binary tree
  holding the values k, 2k, 3k, ..., 10k.

  Create a new channel ch and kick off the walker:

  go Walk(tree.New(1), ch) Then read and print 10 values from the channel. It should be the numbers 1, 2, 3, ..., 10.
  Implement the Same function using Walk to determine whether t1 and t2 store the same values.
  Test the Same function.

  Same(tree.New(1), tree.New(1)) should return true, and Same(tree.New(1), tree.New(2)) should return false.

  Code skeleton:

package main

import "golang.org/x/tour/tree"

// Walk walks the tree t sending all values
// from the tree to the channel ch.
func Walk(t *tree.Tree, ch chan int)

// Same determines whether the trees
// t1 and t2 contain the same values.
func Same(t1, t2 *tree.Tree) bool

func main() {
}

Solution:

package main

import (
    "code.google.com/p/go-tour/tree"
    "fmt"
)

// Walk walks the tree t sending all values
// from the tree to the channel ch.
func Walk(t *tree.Tree, ch chan int) {
    if t.Left != nil {
        Walk(t.Left, ch)
    }
    ch <- t.Value
    if t.Right != nil {
        Walk(t.Right, ch)
    }
}

// Same determines whether the trees
// t1 and t2 contain the same values.
func Same(t1, t2 *tree.Tree) bool {
    c1 := make(chan int, 10)
    c2 := make(chan int, 10)
    go Walk(t1, c1)
    go Walk(t2, c2)
    for i := 0; i < 10; i++ {
        x, y := <-c1, <-c2
        fmt.Printf("x: %v, y: %v\n", x, y)
        if x != y {
            return false
        }
    }
    return true
}

func main() {
    fmt.Println(Same(tree.New(1), tree.New(1)))
    fmt.Println(Same(tree.New(1), tree.New(2)))
}

This is a nice answer to a fun challenge.  There are a couple ways I can see to improve it.

This solution only works for arrays of up to 10 values due to the for loop terminating after 10 iterations.  We can make it work for an arbitrary number of values by closeing the channel when Walk has complete, and testing for each channel's state to match.
Testing the value earlier in the walk will lead to earlier fails.  Reordering the send on the channel to be first will allow this.

Here is a refactored version of your code:

func Walk(t *tree.Tree, vals chan int) {
    if t != nil {
        vals <- t.Value
        Walk(t.Left, vals)
        Walk(t.Right, vals)
    }
}

func Same(a, b *tree.Tree) bool {
    avals := make(chan int, 8)
    bvals := make(chan int, 8)
    go func() {
        Walk(a, avals)
        close(avals)
    }()
    go func() {
        Walk(b, bvals)
        close(bvals)
    }()
    for {
        av, aopen := <-avals
        bv, bopen := <-bvals
        if aopen != bopen || av != bv {
            return false
        }
        if !aopen {
            return true
        }
    }
}

Buffering the channels is still useful since synchronization is not a requirement for evaulation, so I've left them buffered.


Console-based calculator in C++ I've recently started studying C++ (from internet) Made my very first program, calculator. It works, but i'd like to know how could i make this more "advanced"?

#include <iostream>
using namespace std;

int pluss(int num1, int num2)
{
    int answer = num1 + num2;
    cout << num1 << " + " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
    return 0;
}

int Minus(int num1, int num2)
{
    int answer = num1 - num2;
    cout << num1 << " - " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
    return 0;
}

int devide(double num1, double num2)
{
    double answer = num1 / num2;
    cout << num1 << " / " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
    return 0;
}

int multiply(double num1, double num2)
{
    double answer = num1 * num2;
    cout << num1 << " * " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
    return 0;
}

int main()
{
    int begin;
    cout << "----------------------------------------" << endl;
    cout << "-----Welcome to C++ Based Calculator!---" << endl;
    cout << "-1=Plus--2=Minus--3=devide--4=multiply--" << endl;
    cout << "----------------------------------------" << endl;
    cin >> begin;

    if (begin == 1) {
        system("CLS");
        int x;
        int y;
        cout << "Give me a first number" << endl;
        cin >> x;
        system("CLS");
        cout << "Give me a second number" << endl;
        cin >> y;
        pluss(x, y);
        return main();
    }
    else if (begin == 2) {
        system("CLS");
        int x;
        int y;
        cout << "Give me a first number" << endl;
        cin >> x;
        system("CLS");
        cout << "Give me a second number" << endl;
        cin >> y;
        Minus(x, y);
        return main();
    }
    else if (begin == 3) {
        system("CLS");
        double x;
        double y;
        cout << "Give me a first number" << endl;
        cin >> x;
        system("CLS");
        cout << "Give me a second number" << endl;
        cin >> y;
        devide(x, y);
        return main();
    }
    else if (begin == 4) {
        system("CLS");
        double x;
        double y;
        cout << "Give me a first number" << endl;
        cin >> x;
        system("CLS");
        cout << "Give me a second number" << endl;
        cin >> y;
        multiply(x, y);
        return main();
    }
    else {
        system("CLS");
    }

    system("pause");
    return 0;
}

This is a nice first project. Since you are a beginner in C++, I'm not going to worry about the using namespace std and a couple other details.

Naming

Your functions follow an inconsistent naming style and are not spelled correctly:

int pluss(int num1, int num2);

int Minus(int num1, int num2);

int devide(double num1, double num2);

int multiply(double num1, double num2);

These would be better named

int plus(int num1, int num2);

int minus(int num1, int num2);

int divide(double num1, double num2);

int multiply(double num1, double num2);

Consider using void functions

Your functions all return an int. It's not very clear what the return value would mean, so I was puzzled over that for a bit, but then I noticed that they all returned 0:

int pluss(int num1, int num2)
{
    ...
    return 0;
}

It seems that you are copying the idea of int main(), where main returns an error code if something went wrong, but there's no way for something to go wrong in these functions barring an exception, so just make these functions void:

void pluss(int num1, int num2)
{
    int answer = num1 + num2;
    cout << num1 << " + " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
}

On the other hand, the names of your functions seem to suggest that they should return the result of the operation, in which case you might do this:

int plus(int num1, int num2)
{
    return num1 + num2;
}

But don't do this. This is simply re-inventing the + operator.

System dependency

Your code uses system() all over the place. system is not cross platform, and it is better to make the code as cross-platform as possible. You really don't need to use system("CLS") everywhere; people using a console don't expect their console to clear old messages. Furthermore, system("pause") isn't necessary; the console waits whenever your program needs to obtain input.

Switch

In your main function, you have this:

if (begin == 1) {
    ...
}
else if (begin == 2) {
    ...
}
else if (begin == 3) {
    ...
}
else if (begin == 4) {
    ...
}
else {
    ...
}

This is basically a switch statement; you could just code that like so:

switch (begin) {
case 1: ...
case 2: ...
case 3: ...
case 4: ...
default: ...
}

Why only add and subtract ints?

If you think it's a good idea to be able to divide and multiply doubles, why not have the same thing for add and subtract? That's definitely something I would expect if I were using your calculator.

Functions should only do "one" thing

Your functions do multiple things:

void multiply(double num1, double num2)
{
    double answer = num1 * num2;
    cout << num1 << " * " << num2 << " = " << answer << endl;
    system("pause");
    system("CLS");
}

Notice how this function is less of a mathematical function, and more of a "perform this task". The task you are having it perform is to "display the result of multiplication, then clear the screen afterwards". That's most definitely multiple things. It would be better to make the function this:

void showMultiply(double num1, double num2)
{
    double answer = num1 * num2;
    cout << num1 << " * " << num2 << " = " << answer << endl;
}

But if you were to do this with all your functions, suddenly they are all almost the same:

void showPlus(double num1, double num2)
{
    double answer = num1 + num2;
    cout << num1 << " + " << num2 << " = " << answer << endl;
}

void showMinus(double num1, double num2)
{
    double answer = num1 - num2;
    cout << num1 << " - " << num2 << " = " << answer << endl;
}

void showMultiply(double num1, double num2)
{
    double answer = num1 * num2;
    cout << num1 << " * " << num2 << " = " << answer << endl;
}

void showDivide(double num1, double num2)
{
    double answer = num1 / num2;
    cout << num1 << " / " << num2 << " = " << answer << endl;
}

We can collapse these down to result in less duplication:

void showCalculation(double num1, double num2, double answer, char operator)
{
    cout << num1 << ' ' << operator << ' ' << num2 << " = " << answer << endl;
}

Now when we use it, it would look like this:

double answer = num1 + num2;
showCalculation(num1, num2, answer, '+');
...

However, since this function is only one line of code, I personally wouldn't use the function.

Code duplication

Notice how each block in the if statements look almost identical:

if (begin == 1) {
    system("CLS");
    int x;
    int y;
    cout << "Give me a first number" << endl;
    cin >> x;
    system("CLS");
    cout << "Give me a second number" << endl;
    cin >> y;
    pluss(x, y);
    return main();
}
else if (begin == 2) {
    system("CLS");
    int x;
    int y;
    cout << "Give me a first number" << endl;
    cin >> x;
    system("CLS");
    cout << "Give me a second number" << endl;
    cin >> y;
    Minus(x, y);
    return main();
}
else if (begin == 3) {
    system("CLS");
    double x;
    double y;
    cout << "Give me a first number" << endl;
    cin >> x;
    system("CLS");
    cout << "Give me a second number" << endl;
    cin >> y;
    devide(x, y);
    return main();
}
else if (begin == 4) {
    system("CLS");
    double x;
    double y;
    cout << "Give me a first number" << endl;
    cin >> x;
    system("CLS");
    cout << "Give me a second number" << endl;
    cin >> y;
    multiply(x, y);
    return main();
}

This is a sign that you should write functions to break it up. This is what I'd do:

double promptForInput(const string &message)
{
    double result;
    cout << message << endl;
    cin >> result;
    return result;
}

...

if (begin == 1) {
    double num1 = promptForInput("Give me the first number");
    double num2 = promptForInput("Give me the second number");
    double result = num1 + num2;
    showCalculation(num1, num2, result, '+');
}
// and so on

Calling main() is undefined behaviour

It is not legal to call main() in your C++ code. You are using this to loop; you should instead learn how to use loops. This is one example of how to do so:

int main()
{
    do {
        int begin;
        cout << "----------------------------------------" << endl;
        cout << "-----Welcome to C++ Based Calculator!---" << endl;
        cout << "-1=Plus--2=Minus--3=devide--4=multiply--" << endl;
        cout << "----------------------------------------" << endl;
        cin >> begin;

        if (begin == 1) {
            ...
        }
        ...
        else {
            break;
        }
    } while (true);

    return 0;
}

Use constants

Since you are using numbers to represent what choice the user made, I strongly recommend you store them as constants:

constexpr int PLUS = 1;
constexpr int MINUS = 2;
constexpr int DIVIDE = 3;
constexpr int MULTIPLY = 4;

Final result

After apply all my recommendations, this is my final result:

#include <string>
#include <iostream>

using namespace std;

double promptForInput(const string &message)
{
    double result;
    cout << message << endl;
    cin >> result;
    return result;
}

constexpr int PLUS = 1;
constexpr int MINUS = 2;
constexpr int DIVIDE = 3;
constexpr int MULTIPLY = 4;

int main()
{
    do {
        int choice; // choice is a better name for this
        cout << "----------------------------------------" << endl;
        cout << "-----Welcome to C++ Based Calculator!---" << endl;
        cout << "-1=Plus--2=Minus--3=Divide--4=Multiply--" << endl;
        cout << "----------------------------------------" << endl;
        cin >> choice;

        double num1 = promptForInput("Give me the first number");
        double num2 = promptForInput("Give me the second number");

        double result;
        char operatorName;

        switch (choice) {
        case PLUS:
            result = num1 + num2;
            operatorName = '+';
            break;
        case MINUS:
            result = num1 - num2;
            operatorName = '-';
            break;
        case DIVIDE:
            result = num1 / num2;
            operatorName = '/';
            break;
        case MULTIPLY:
            result = num1 * num2;
            operatorName = '*';
            break;
        default:
            cout << "Some message thanking the user for using the calculator (optional). " << endl;
            return 0; // to exit main
        }

        cout << num1 << ' ' << operatorName << ' ' << num2 << " = " << result << endl;
    } while (true);
}

Going further

If you wanted to make this "even better", this is how I'd fully implement this:

#include <iostream>
#include <sstream>
#include <string>

void printOptions() {
    std::cout << "------------------------------------------" << std::endl;
    std::cout << "----- Welcome to C++ Based Calculator! ---" << std::endl;
    std::cout << "- Plus:     +                          ---" << std::endl;
    std::cout << "- Minus:    -                          ---" << std::endl;
    std::cout << "- Divide:   /                          ---" << std::endl;
    std::cout << "- Multiply: *                          ---" << std::endl;
    std::cout << "- Quit:     Enter                      ---" << std::endl;
    std::cout << "------------------------------------------" << std::endl;
}

constexpr char QUIT = '\0';

char menu() {
    printOptions();

    std::string input;
    std::getline(std::cin, input);

    if (input.size() != 1) {
        return QUIT;
    }
    return input[0];
}

double promptForInput(const std::string &message) {
    std::cout << message << std::endl;

    double result;
    std::istringstream input;
    do {
        std::string line;
        std::getline(std::cin, line);
        input.str(line);

        input >> result;

        if (!input) {
            std::cout << "Please input a valid number" << std::endl;
        }
    } while (!input);

    return result;
}

double performOperation(double num1, double num2, char choice) {
    switch (choice) {
    case '+': return num1 + num2;
    case '-': return num1 - num2;
    case '*': return num1 * num2;
    case '/': return num1 / num2;
    }
    std::cerr << "Internal Error" << std::endl;
    std::exit(1);
}

int main() {
    do {
        char choice = menu();
        if (choice == QUIT) return 0; // The user wants to quit

        double num1 = promptForInput("Give me the first number");
        double num2 = promptForInput("Give me the second number");

        double result = performOperation(num1, num2, choice);

        std::cout << num1 << ' ' << choice << ' ' << num2 << " = " << result << std::endl;
    } while (true);
}

Here's what I did differently:

I had the user input the actual character for the operation they want to use. This makes my life a little easier, as I can just pass that around. If I didn't want to do this, I'd actually translate the number into the character.
I didn't use the using namespace std;. That statement pollutes your namespace with all the symbols from the standard namespace; there's a reason why the standard namespace is only 3 characters long
I used std::getline to get the input. This makes it so that the user has to input everything on its own line. I liked this idea better.
I moved the switch into its own function; this makes the main function shorter, making it easier to understand.


Are these set-uid scripts/binaries secure? I have a system that needs to be able to reboot a different piece of hardware partway through a script that programs it. It used to wait for me to come and reboot the hardware halfway through, but I've since automated that. I use this python script to control a USB Net Power 8800.

I have two setups to show you: First is a system that I'm sure is secure, but is pretty annoying to maintain; I'm sure the second is insecure.

First:

/usr/bin/power (not set-uid)

#!/bin/sh
if [ -e /opt/usbnetpower/_powerwrapper_$1 ] ; then
        /opt/usbnetpower/_powerwrapper_$1
else
        /opt/usbnetpower/_powerwrapper_
fi

/opt/usbnetpower/_powerwrapper_reboot.c (The source code to one of six almost identical set-uid programs)

#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <unistd.h>

int main()
{
   setuid( 0 );
   system( "/usr/bin/python /opt/usbnetpower/usbnetpower8800.py reboot" );

   return 0;
}

The reason why there are six is that I didn't trust myself to write correct sanitization code in C that wasn't vulnerable to buffer overflows and the like.

/opt/usbnetpower/usbnetpower8800.py (not set-uid)

Essentially this, but modified to add a reboot command.

This setup is what I am currently using.

Second:

/opt/usbnetpower/usbnetpower8800.py (set-uid)

Much simpler, but I'm concerned that a vulnerability like this would apply. Also, suid python programs appear to be disabled on my system. (CentOS 6)

Both of your programs are insecure. The problem is that I can set PYTHONPATH to anything I like before executing the program. I can set it to something so that it loads my own usb module which can execute whatever code I like. At that point, I'm executing code as the root user and can do whatever I please. That's why set-uid doesn't work on scripts, and your binary C program wrapper doesn't change that at all.

For more information see: https://unix.stackexchange.com/questions/364/allow-setuid-on-shell-scripts

In your case, what you should be doing is changing the permissions so that your user has access to the USB device you are supposed to be controlling. That way you don't need to run the script as another user. That's what this portion of the script you are using is referring to:

# If you have a permission error using the script and udev is used on your
# system, it can be used to apply the correct permissions. Example:
#  $ cat /etc/udev/rules.d/51-usbpower.rules
#  SUBSYSTEM=="usb", ATTR{idVendor}=="067b", MODE="0666", GROUP="plugdev"

Discussion of how to adjust your system to give you permission to access the USB device is out of scope for this site. I'm also not really an expert on that. So you may need to ask elsewhere for that.


Push List Element to Top Please evaluate this function. It takes a list [a] and an Int, i.e. the index, and returns a new list with the selected item at the top of the list. Note that it returns Maybe [a] to account for bad indexes.

pushToTop :: [a] -> Int -> Maybe [a]
pushToTop [] _   = Just []
pushToTop xs i
  | i >= length xs || i < 0 = Nothing
  | i == 0                  = Just xs
  | otherwise               = Just $ newTop : rest
                              where zipped = zip [0..] xs
                                    newTop = xs !! i
                                    rest   = (map (\x -> snd x) . filter (\x -> fst x /= i)) zipped

I'm also curious if my usage of !! is OK given the 2 guards on i.

Yes, the guards make !! safe, but your code has other issues, stylistic and functional:

Unneeded pointful style

First, it's often a good idea to try running your code through HLint.  Doing so, you get the following suggestion:

  pushToTop.hs:9:52: Error: Avoid lambda
  Found: \x -> snd x
  Why not: snd
  1 suggestion

Indeed, why not. If we're to use point-free style for this, we could also simplify

filter (\x -> fst x /= i)

to

filter ((/= i) . fst)

At some point, point-free style can get difficult to read, but in this case I think it is still readable.

Parentheses that can be replaced with a use of $

Right here:

(map (\x -> snd x) . filter (\x -> fst x /= i)) zipped

Using $, you could also inline zipped non-awkwardly:

map snd . filter ((/= i) . fst) $ zip [1..] xs

Incorrect base case

Shouldn't any index on an empty list be considered an error? As is, you're completely ignoring the index. For consistency, I would make the first case return Nothing.

Unnecessary case

This case is already handled by the latter case:

  | i == 0 = Just xs

It can be safely removed.

Failure to work on infinite lists

The operation you have defined (move an element at a certain index to the front) is well-defined on infinite lists, but your code fails to handle the case. In particular, length will not terminate on infinite lists. The current structure of your code cannot handle this. Rewriting it to make explicit the recursion and to avoid calculating the length should make it work on infinite lists.

Test case:

ghci> fmap (take 20) $ pushToTop [0..] 12
[12, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 13, 14, 15, 16, 17, 18, 19]


Calculating sum of certain values in a list I have two loops which I am using to create a file, if the clientID in the client loop is the same as the id in the valuation loop then create a row. Then if the month value is the same as the previous month value we need to add up the values.

This all works fine but i was wondering if anyone can think of a better way?

        for (Clients cl : clientList) {
            //in loop so use stringbuilder
            StringBuilder sb = new StringBuilder();
            sb.append(cl.getId());
            sb.append(",");
            sb.append(cl.getFirstName());
            sb.append(" ");
            sb.append(cl.getLastName());
            sb.append(",");
            row = sb.toString();
            for (Valuations vl : valList) {
                year = vl.getYear();
                    //if we are on a new month
                    if (!month.equals(vl.getMonth()) && !month.equals("")) {
                        //add row and reset values
                        if(!vl.isNanError()){
                        data.add(row + month + "," + year + ",£" + cash);
                        }
                        cash = 0;
                        month = "";
                        done = false;
                    }
                    if (cl.getId().equals(vl.getClientId())) {
                        if (!done) {
                            row = sb.toString();
                            cash = 0;
                            month = vl.getMonth();
                            done = true;
                        }
                    }
                    cash += vl.getValue();
            }
        }
        //add the last row after the loop breaks.
        data.add(row + month + "," + year + ",£" + cash);

If you add a toString() method inside the client class you could, replace this code:

//in loop so use stringbuilder
            StringBuilder sb = new StringBuilder();
            sb.append(cl.getId());
            sb.append(",");
            sb.append(cl.getFirstName());
            sb.append(" ");
            sb.append(cl.getLastName());
            sb.append(",");
            row = sb.toString();

for just:

row = sb.toString();

Furthermore, you can append the second with the third if condition inside the second loop, this:

if (cl.getId().equals(vl.getClientId())) {
                    if (!done) {
                        row = sb.toString();
                        cash = 0;
                        month = vl.getMonth();
                        done = true;
                    }
                }

can became this:

  if (!done && cl.getId().equals(vl.getClientId()))
    {
                            row = sb.toString();
                            cash = 0;
                            month = vl.getMonth();
                            done = true;
    }

Another thing, in the above code the row = sb.toString instruction should it be remove?
Since you are calculated the row right below the first loop:

row = sb.toString();
for (Valuations vl : valList) {