Compute binary sum with Python 3 I tried to make a function able to compute the binary sum. Binary numbers are passed as list composed of 1 and 0, like [1,1,0,1], [0,0,1], etc.
It seems to work. I tried to improve the code, but I don't think  this is the best way to make it work. How can I improve this?

def check(value):
    if value%2 == 0: #check if the number can be divided by 2
        amount = int(value / 2) #if so, set the value to 0 and the amount as the number divided by 2
        value = 0
    elif value%2 != 0: #if not so, set the value to 0 and the amount as the previous even number divided by 2
        amount = int((value-1)/2)
        value = 1
    return value, amount #returns the 2 value.

def binary_sum(*args):
"""The program compute the binary sum of 2+ binary lists, like [1,1,1,1] + [0,0,0,1]"""
    i = -1 #control variable
    result = [] #list that will be filled by results' binary numbers
    value = 0
    amount = 0 #amount carried over
    binaryLength = [] #list of all args length
    for x in args:  binaryLength.append(len(x)) #get the length of each args elements
    maxLength = max(binaryLength) #get the max length among args elements
    for x in args:
        if len(set(binaryLength)) != 1: #checks if all elements have the same length
            delta = maxLength - len(x) - 1 #if not so, it adds as many zeros as delta value indicates
            while (delta) >= 0:
                x[:0] = [0]
                delta -= 1
    while i >= -maxLength: #starts the sum from the end.
        for j in args: value += j[i] #get the sum of all the last list element( for each arg)
        value += amount
        value, amount = check(value) #uses binary sum properties: it returns a value between 0 and 1, and the amount, an integer
        result.insert(i, value) #inserts the value at the beginning of the list
        i-=1 # needed to iterate the process
        value = 0 #resets the value
    if amount != 0: #At the end, if there's amount it inserts it at the beginning of result list:
        while amount not in [0,1]: #if is not equal to 0 or 1 converts the int amount in binary number
            value = amount
            value, amount = check(value)
            result.insert(i, value)
            i-=1
        result.insert(i, amount) #if is equal to 1 inserts it at the start of the list.
    return result #returns the result as list.

You've got way too much code.  You are not using the built-in tools Python provides to solve this problem, making your job much, much harder.

In your check(value) function, you are checking:

if value % 2 == 0:
    # ...
elif value % 2 != 0:
    # ...

If value % 2 == 0 is False, then value % 2 != 0 is guaranteed to be True.  There is no need for the elif ...:; you could simply use an else: clause.

In both cases, you assign amount = int( ... / 2).  Python has built-in integer division operator, //, so these statements could become simply amount = ... // 2.  In the second case, because value is odd, you divide value - 1 by two.  This "subtract 1" is unnecessary.  Using integer division (or dividing and then casting to an integer) will truncate the result to an integer.

Afterwards, in the first case, when value % 2 is zero, you assign value = 0, where as in the second case, when value % 2 is one, you assign value = 1.  You could simply assign value = value % 2.

def check(value):
    amount = value // 2
    value = value % 2
    return value, amount

Or slightly shorter:

def check(value):
    return value % 2, value // 2

Or even better, use the built-in function divmod(a,b) which does the same thing.  But you will have to swap the variables you assign to, because divmod(a,b) returns the result of the division first, and the modulo remainder second:

amount, value = divmod(value, 2)

In your binary_sum(*args) function, you are given a variable number of lists, and you want to take one element from each list, then take the next element from each list, and then take the next element from each list, and so on.  In Python, this is the zip() function.  (Note: "zip" is short for "zipper", not zip-compression.)

for addends in zip(*args):
    # ...

Except we have a wrench in the works; you want to start from the end and work towards the front.  That is hard.  It is easier if each list is reversed(), so starting from the start is starting from what was the end.  We can then zip() those reversed() lists together:

for addends in zip(*map(reversed, args)):
    # ...

Except we have another wrench in the works; the lists aren't all the same length, and zip stops when any of the lists runs out of items.  You need to add 0's at the start, so they are the same length.  Except we've reversed the lists, so we need to add 0's to the end.  That's easy.  We just need itertools.zip_longest(*args, fillvalue=0) to provide the extra 0's.

from itertools import zip_longest

for addends in zip_longest(*map(reversed, args), fillvalue=0):
    # ...

Now addends is first the 1's column digits, then the 2's column digits, then the 4's column digits, and so on.  We can just sum the addends together, along with any carry from the previous column:

digits = []
carry = 0
for addends in zip_longest(*map(reversed, args), fillvalue=0):
    total = sum(addends) + carry
    carry, digit = divmod(total, 2)
    digits.append(digit)

And then, add the carry bits to end once you've run out of columns:

while carry > 0:
    carry, digit = divmod(carry, 2)
    digits.append(digit)

Finally, since we've generated the result in the wrong order, you'll have to reverse it again, so the most significant digit is first.

from itertools import zip_longest

def binary_sum(*args):
    digits = []

    carry = 0
    for addends in zip_longest(*map(reversed, args), fillvalue=0):
        total = sum(addends) + carry
        carry, digit = divmod(total, 2)
        digits.append(digit)

    while carry > 0:
        carry, digit = divmod(carry, 2)
        digits.append(digit)

    return digits[::-1]

A few tests:

>>> binary_sum([1,1,1,0])
[1, 1, 1, 0]
>>> binary_sum([1,1,1,0], [1,0])
[1, 0, 0, 0, 0]
>>> binary_sum([1,1,1,1], [1])
[1, 0, 0, 0, 0]
>>> binary_sum([1,1,1,1], [0,0,0,1])
[1, 0, 0, 0, 0]
>>> binary_sum([1,1,1,1], [1,0,0,0])
[1, 0, 1, 1, 1]
>>> binary_sum([1], [1], [1], [1], [1])
[1, 0, 1]


Calculating a total price with group pricing I'm evaluating a meeting_type value that determines what kind of pricing structure I should be using. Currently, if my meeting_type value is a w, we charge one price for every 10 registrants. If the meeting_type is anything else, it's a price per registrant model.

I'm using this logic:

if($_POST['meeting_type'] != 'w'){
    $total_price = $guests * $price;
}
else{
    $webinar_count = ceil($guests / 10);
    $total_price = $webinar_count * $price;
}

Is this logic flawed?  Can it be improved upon?

Removing intermediate variable $webinar_count

$total_price = ($_POST['meeting_type'] == 'w')
             ? (ceil($guests / 10) * $price)
             : ($guests * $price);

OR

if ($_POST['meeting_type'] != 'w') {
    $total_price = $guests * $price;
} else {
    $total_price = ceil($guests / 10) * $price;
}


Finding the first and last IP in an IP range string I'm very new to python and scripting in general. I've come up with the following function to find the first and last IP when an IP range string is inputted. I want to only use base Python (and not download any libraries essentially).

Example:

genRange("1.2.3.*"):
Start IP = 1.2.3.0
End IP = 1.2.3.255

genRange("1.*"):
Start IP = 1.0.0.0
End IP = 1.255.255.255

genRange("1.2.3.4"):
Start IP = 1.2.3.4
End IP =
NotRange

This seems very hardcoded to me, because it really is. Is there a a better way to do this that is obvious/jumps out at you?

I still need to handle invalid input.

def genRange(ipRangeString):
    startIP = ""
    endIP = ""
    if '.' in ipRangeString:
        ipV = 4
    if ':' in ipRangeString:
        ipV= 6
    if ipV == 4:
        parts = ipRangeString.split(".")
        i = 0
        for item in parts:
            if item != "*":
                if 0 <= int(item) <= 255:
                    if (i<=3):
                        startIP = startIP + item + '.'
                        if (i==3): startIP = startIP[:-1]
                    i+= 1
            else:
                tempSIP = startIP
                endIP = startIP
                while (i<=3):
                    endIP = endIP + "255"
                    startIP = startIP  + "0"
                    if (i<3):
                        endIP = endIP + '.'
                        startIP = startIP + '.'
                    i+= 1
    print "Start IP = ", startIP
    print "  End IP = ", endIP
    if not (endIP):
        print "NotRange"

The first thing that jumps out at me is that this is a single function, with no classes and only one possible use. But without context, I couldn't comment on whether or not that's good. Since the snippet is so short and limited, your only motivation for changing it is that you might want to use it in a larger context. So I'm going to assume a larger context which may or may not apply.

The second thing, then, is that there are no comments. I can look at this code, see the ipV is 4 or 6, see "Start IP" and "End IP", and figure out that this is computing an IP address range for IPv4. But the function name doesn't really tell me that. Just for maintainability sake, you might want to do something about that.

The third thing that jumps out at me is that this function does nothing useful. It has two (conditionally 3) print statements as side effects, which means that you can't use these results. You might want to separate the computation of the range from the printing of the results. This is where I have to assume a larger context. If you just want to print those 2 or 3 lines, why bother modifying working code to begin with other than to maybe add some comments for the future. What if you want to accept ranges (10.0.1.5-50)? Heck, what if you want ranges with holes (10.0.5-10.2-100)?

Fourth, your variable i is the index of item in the loop, which is used in a lot of logic in your function. See https://stackoverflow.com/questions/522563/accessing-the-index-in-python-for-loops for some discussion on that. Specifically, don't manage your own loop index variable. I see why you are doing so; to loop the necessary times between 1.* and 1.1.*. You can do this a few different ways. Statically, you need 4 parts to your IPv4 addresses. You could iterate over the range 4 and generate these 4 parts. You could iterate over your input parts and append the missing parts as needed once complete. I don't actually have a strong opinion of the right way, only that your way looks funny to me.

Fifth, tempSIP. important? used?

Punchline without assuming a larger context: Add a comment about usage. Add a comment about the program logic. Rename the function to have IP in the name. Delete tempSIP.


C++ template meta sql I wrote a wrapper around boost-mpl and transformed it to an sql engine. It is a compile time sql thing, not related to normal databases. I have not finished it completely. I have some problem with the interface.

A template meta program run in compile time. So input data always present in compile time that is why I organized the inputs to tables.

Tables are simple:

#define DEF_COLUMN(name) struct Col##name {}; \
                         template<typename T> struct name \
                         { typedef typename mpl::pair<Col##name, T> type; };
struct TestTable1
{
    DEF_COLUMN(PrimaryKey)
    DEF_COLUMN(Storage)
    DEF_COLUMN(BaseType)
    typedef mpl::vector<ColPrimaryKey, ColStorage, ColBaseType> header;

    // sample table
    // PrimaryKey | Storage |  BaseType
    // -----------+---------+-----------
    // CTypeA1    | int     | CTypeABase
    // CTypeA2    | string  | CTypeABase
    typedef mpl::vector<
        mpl::map3<PrimaryKey<CTypeA1>::type, Storage<int>::type,         BaseType<CTypeABase>::type>,
        mpl::map3<PrimaryKey<CTypeA2>::type, Storage<std::string>::type, BaseType<CTypeABase>::type>
    > type;
};

struct TestTable2
{
    DEF_COLUMN(Tag)
    DEF_COLUMN(Attribute)

    typedef mpl::vector<
        mpl::map2<Tag<CTypeA1>::type, Attribute<CTypeB1>::type>,
        mpl::map2<Tag<CTypeA2>::type, Attribute<CTypeB1>::type>,
        mpl::map2<Tag<CTypeA2>::type, Attribute<CTypeB2>::type>
    > type;
};

And you can make queries on it:

void TestQuery1()
{
    typedef Select<
            Columns<MplFunction<Column<TestTable1::ColStorage>, mpl::sizeof_> >,
            From<TestTable1,
               Join<TestTable2, Equal<TestTable1::ColPrimaryKey, TestTable2::ColTag> >
            >
    > TResultSet;
    DEBUG_TYPE((TResultSet));
    print_table<TResultSet::type>();
}

I have type debug macro and table print template helper.

So the above prints:

TResultSet => Select<Columns<MplFunction<Column<TestTable1::ColStorage>, boost::mpl::sizeof_>, mpl_::na>, From<TestTable1, Join<TestTable2, Equal<TestTable1::ColPrimaryKey, TestTable2::ColTag> > >, Where<True> >
+------------------------+
| TestTable1::ColStorage |
+------------------------+
| mpl_::size_t<8ul>      |
| mpl_::size_t<8ul>      |
| mpl_::size_t<4ul>      |
+------------------------+
3 rows in set

I have problem with the syntax, how can I improve this, to achive an easy to use meta query thing? I think I have to eliminate the Columns, Column thing from the

Select<Columns<MplFunction<Column< ...

expression. And I need good Row fetch things and others. Any idea? And where can I upload the source?

So this gives back compile time types, not runtime values.

void TestQuery3()
{
    typedef Select<
            Columns<Column<TestTable1::ColStorage> >,
            From<TestTable1>,
            Where<
                Equal<TestTable1::ColPrimaryKey, CTypeA1>
           >
    > TResultSet;
    DEBUG_TYPE((TResultSet));
    print_table<TResultSet::type>();

    // fetch one row
    typedef mpl::back<TResultSet::type>::type TResultRow;

    // get column ColStorage which is an int, TRes will be an int
    typedef mpl::at<TResultRow, TestTable1::ColStorage>::type TRes;

    // use the "int" type
    TRes testInteger = 9;

    // prints test value
    std::cout << "testInteger:" << testInteger << std::endl;
}

this prints:

TResultSet => Select<Columns<Column<TestTable1::ColStorage>, mpl_::na>, From<TestTable1, mpl_::na>, Where<Equal<TestTable1::ColPrimaryKey, CTypeA1> > >
+------------------------+
| TestTable1::ColStorage |
+------------------------+
| int                    |
+------------------------+
1 row in set

testInteger:9

You my also want to have a look at how the Poco library achieves a similar thing using a Tuple class. From the docs:

typedef Poco::Tuple<std::string, std::string, int> Person;
typedef std::vector<Person> People;

People people;
people.push_back(Person("Bart Simpson", "Springfield", 12));
people.push_back(Person("Lisa Simpson", "Springfield", 10));

Statement insert(session);
insert << "INSERT INTO Person VALUES(:name, :address, :age)",
    use(people), now;

Statement select(session);
select << "SELECT Name, Address, Age FROM Person",
    into(people), now;

for (People::const_iterator it = people.begin(); it != people.end(); ++it)
{
    std::cout << "Name: " << it->get<0>() <<
        ", Address: " << it->get<1>() <<
        ", Age: " << it->get<2>() <<std::endl;
}


Drawing a circle with text in it using CSS I'm trying to draw a circle and put text in the circle.  It'll be part of a UI I'm working on.  I have it working and it looks the way I want, but I think there must be a better way to do it.  Mainly, I don't like all the hard coded numbers and lining up everything by hand.  What I'd love is a version that could easily scale to any size without having to change a lot of the top and left properties of all the text.  Any ideas on how to improve this?

Fiddle

div.arcs {
    border: 50px solid red;
    display: inline-block;
    min-width: 14em;
    min-height: 14em;
    max-height: 14em;
    max-width: 14em;
    border-radius: 50%;
    border-top-color: rgb(109, 176, 65);
    border-bottom-color: rgb(167, 167, 167);
    border-left-color: rgb(255, 199, 45);
    border-right-color: rgb(104, 162, 219);
    box-shadow: 0 0 20px 5px #a0a0a0;
}

div.arcs > div {
    font-family: Calibri;
    font-size: 20pt;
    text-align: center;
    text-shadow: 1px 1px 3px #000000;
    color: #e0e0e0;
    position: relative;
    width: 8.45em;
    height: 1.5em;
    line-height: 1.35em;
}

div.arcs > div:hover {
    text-decoration: underline;
    cursor: pointer;
}

div.arcs > div.top {
    top: -1.55em;
}

div.arcs > div.left {
    transform: rotate(270deg);
    transform-origin: right top 0;
    left: -10em;
    top: -1.5em;
}

div.arcs > div.right {
    transform: rotate(90deg);
    transform-origin: left top 0;
    left: 10em;
    top: -3.1em;
}

div.arcs > div.bottom {
    top: 3.7em
}

Text 1

Text 4

Text 2

Text 3

Instead going against how the browser naturally renders elements, use it to your advantage.

div.arcs {
    border: 50px solid red;
    display: block;
    min-width: 14em;
    min-height: 14em;
    max-height: 14em;
    max-width: 14em;
    border-radius: 50%;
    border-top-color: rgb(109, 176, 65);
    border-bottom-color: rgb(167, 167, 167);
    border-left-color: rgb(255, 199, 45);
    border-right-color: rgb(104, 162, 219);
    box-shadow: 0 0 20px 5px #a0a0a0;
    position:relative; /*changed*/
}

div.arcs > div {
    font-family: Calibri;
    font-size: 20pt;
    text-align: center;
    text-shadow: 1px 1px 3px #000000;
    color: #e0e0e0;
    position: absolute; /*changed*/
    width: 100%; /*changed*/
    height: 1.5em;
    line-height: 1.35em;
}

div.arcs > div:hover {
    text-decoration: underline;
    cursor: pointer;
}

div.arcs > div.top {
    top: -45px; /*changed - against the size of the border*/
}

div.arcs > div.left {
    transform: rotate(270deg);
    transform-origin: right top 0;
    /*top:0;
    left:-45px; - remove */
}

div.arcs > div.right {
    transform: rotate(90deg);
    transform-origin: left top 0;
    /*left: 10em;
    top: -3.1em; - remove*/
}

div.arcs > div.bottom {
    bottom: -45px; /*changed - against the size of the border*/
}

Then just swop the two div's around:

Text 1

Text 2

Text 4

Text 3

This will allow you to change the size of the circle without having to adjust everything, however if you change the width of the border, you will need to change the line-height of the div and the bottom and top div absolute positioning, there is no way around that.


Guessing Game In Java import java.io.IOException;
import java.util.Scanner;

public class Main {

    private static int random = (int) (Math.random() * 100 + 1);
    private static int data;
    private static int go;
    private static int attempt = 10;

    public static void Start() throws IOException {

        Scanner scanInput = new Scanner(System.in);
        data = scanInput.nextInt();

        if (data > 100) {
            System.out.println("Only Enter Numbers Between 0-100");
            Start();
        } else if (data == random) {
            System.out.println("Wow, you guessed it!!");
            scanInput.close();
            System.exit(0);
        } else if (data < random) {
            go += 1;
            if (go == attempt) {
                System.out.println("You have guessed the wrong number " + go + " times, sorry but you'll have to try again! :(");
                scanInput.close();
                System.exit(0);
            }
            System.out.println("Higher");
            Start();
        } else if (data > random) {
            go += 1;
            if (go == attempt) {
                System.out.println("You have guessed the wrong number " + go + " times, sorry but you'll have to try again! :(");
                scanInput.close();
                System.exit(0);
            }
            System.out.println("Lower");
            Start();
        }
    }

    public static void main(String[] args) throws IOException {
        System.out.println("Guess A Number Between 0-100 Within " + attempt + " Goes");
        Start();
    }

}

I will try to point out things that can be improved in your code:

1. Removing complexity by using return:

    if (data > 100) {
        System.out.println("Only Enter Numbers Between 0-100");
        Start();
        return;
    }

    if (data == random) {
        System.out.println("Wow, you guessed it!!");
        scanInput.close();
        return;
    }

    if (data < random) {
        go ++;
        if (go == attempt) {
            System.out.println("You have guessed the wrong number " + go + " times, sorry but you'll have to try again! :(");
            scanInput.close();
            System.exit(0);
            return;
        }
        System.out.println("Higher");
        Start();
        return;
    }

    if (data > random) {
       // ...
    }

2. Try to avoid recursion, if it is not needed - your code can be easily converted not to use recursion:

public static void Start() throws IOException {

while (true) {

    if (go == attempt) {
        System.out.println("You have guessed the wrong number " + go + " times, sorry but you'll have to try again! :(");
        scanInput.close();
        System.exit(0);
        return;
    }

    Scanner scanInput = new Scanner(System.in);
    data = scanInput.nextInt();

    if (data > 100) {
        System.out.println("Only Enter Numbers Between 0-100");
        continue;
    }

    if (data == random) {
        System.out.println("Wow, you guessed it!!");
        scanInput.close();
        break;
    }

    if (data < random) {
        go ++;
        if (go == attempt) {
            System.out.println("You have guessed the wrong number " + go + " times, sorry but you'll have to try again! :(");
        continue;
    }
}

3. return value instead of System.exit(0);

In order to make your function more reusable, you can have it return a boolean specifying if the number was guessed or not. The caller can decide what to do with the information (maybe this should be integrated in some guessing Web service and those exits are not ok).

Also, it would make sense to allow the user to abort guessing.

// true if guesses, otherwise false
public static boolean Start( throws IOException {
    go = 0;

    while (true) {
        if (go == attempt) {
            System.out.println("You have guessed the wrong number " + go + " times, sorry but you'll have to try again! :(");
            scanInput.close();
            return false;
        }

        Scanner scanInput = new Scanner(System.in);
        data = scanInput.nextInt();

        // define an abort constant and inform the user what value abort the guessing process
        if (data == AbortNumber) {
            System.out.println("Guessing aborted by user");
            return false;
        }

        if (data > 100) {
            System.out.println("Only Enter Numbers Between 0-100");
            continue;
        }

        if (data == random) {
            System.out.println("Wow, you guessed it!!");
            scanInput.close();
            break;
        }

        if (data < random) {
            go ++;
            if (go == attempt) {
                System.out.println("You have guessed the wrong number " + go + " times, sorry but you'll have to try again! :(");
            continue;
        }
}

4. Variable initializations - go is initialized with 0, but it is better to put it explicitly. It is particularly useful, when your guessing instance is reused, as go is changed during the guessing process.

5. Proper names for variables - it is recommended to provide meaningful names for your variables. E.g.:

data -> userGuess
go -> step
attempt -> maxAttempts

6. Avoid repetition - use a constant for maximum number range and use it across your code:

const int MaxNumber = 100;
// ...
static int random = (int) (Math.random() * MaxNumber + 1);
// ...
if (data > MaxNumber)
// ...
System.out.println("Only Enter Numbers Between 0-" + MaxNumber);
// ...

7. Testing for maximum attempts - == should do the work, but >= is safer: if (go >= attempt). It sounds a little paranoiac, but in complex programs and especially when dealing with multi-threading, it is possible that the variable passes over the threshold and the cycle never ends.

Final code

By merging some parts of code in a separate function, your final code could look like (not tested and no renames performed):

private static const int MaxNumber = 100;
private static int random = (int) (Math.random() * MaxNumber + 1);
private static int data;
private static int go;
private static int attempt = 10;

private static void endGuess(String message, Scanner scanInput) {
   System.out.println(message);
   scanInput.close();
}

public static boolean StartGuess() throws IOException {
    go = 0;

    while (true) {
       Scanner scanInput = new Scanner(System.in);
       data = scanInput.nextInt();

       if (go >= attempt) {
          endGuess("You have guessed the wrong number " + go + " times, sorry but you'll have to try again! :(", scanInput);
          return false;
       }

       if (data > MaxNumber ) {
          endGuess("Only Enter Numbers Between 0-" + MaxNumber, scanInput);
          continue;
       }

       if (data == random) {
          endGuess("Wow, you guessed it!!", scanInput);
          return true;
       }

       go ++;

       if (data < random)
          endGuess("Higher", scanInput);
       else // data > random
          endGuess("Lower", scanInput);
    }
}

public static void main(String[] args) throws IOException {
    System.out.println("Guess A Number Between 0-" + MaxNumber + " Within " + attempt + " Goes");
    bool guessed = StartGuess();

    // use guessed in this context, maybe set exit code if not guessed
}


PHP program to generate random phone number I created a PHP program to generate random phone numbers. A valid phone number:

must be exactly 11 digits in length, and
must start with any of the values mentioned in the $a array, and

How can I improve it? Specifically, how can I improve

its readability, and
its performance, assuming I want to generate millions of results.

<?php
      function rand_(){
    $digits_needed=7;

    $random_number=''; // set up a blank string

    $count=0;

    while ( $count < $digits_needed ) {
        $random_digit = mt_rand(0, 8);
        $random_number .= $random_digit;
        $count++;
    }

    return $random_number;

    }

    $a=array('0812', '0813'  ,'0814' ,'0815' ,'0816' ,'0817' ,'0818','0819','0909','0908');
        $i=0;
        while($i<21){
            $website = $a[mt_rand(0, count($a) - 1)];
           print $website . rand_().'<br>';
           $i++;
        }

Use accurate and meaningful names for functions and variable to improve readability.
Condense your numeric loop conditions into a for loop versus while loop.
Avoid declaring single-use variables.
Make functions versatile by passing output-altering arguments with the call.
Write default argument values when possible/reasonable so that, in some cases, no arguments need to be passed for the most common scenario.
Avoid counting a static array in a loop.  If you need to count it, count it once before entering the loop and reference the count variable.
If you prefer mt_rand's randomness, use it in place of my array_rand() call below, but obey #6 about counting.
Obey PSR Coding Standards.  Spacing and tabbing don't improve performance, but they sure make your code easier to read.

Some suggested applications of my generalized advice:

Code: (Demo)

function randomNumberSequence($requiredLength = 7, $highestDigit = 8) {
    $sequence = '';
    for ($i = 0; $i < $requiredLength; ++$i) {
        $sequence .= mt_rand(0, $highestDigit);
    }
    return $sequence;
}

$numberPrefixes = ['0812', '0813', '0814', '0815', '0816', '0817', '0818', '0819', '0909', '0908'];
for ($i = 0; $i < 21; ++$i) {
    echo $numberPrefixes[array_rand($numberPrefixes)] , randomNumberSequence() , "\n";
}

Possible Output:

08161776623
08157676208
08188430651
08187765326
08176077144
09087477073
08127415352
08191681262
08168828747
08195023836
08198008111
09096738254
08162004285
08166810731
08130133373
09093214002
08154125422
08160702315
08143817877
08194806336
08133183466


Finding the second-largest value in an array The problem is stated as:

  You are given as input an unsorted array of n distinct numbers, where
  n is a power of 2. Give an algorithm that identifies the
  second-largest number in the array and that uses at most n+log(n)−2
  comparisons.

I've seen many references to the problem online but most focus on a recursive approach. I wonder why that is. Below is how I approached it. Is there an issue with this implementation? Why choose recurrence here?

testCase = [10, 9, 5, 4, 11, 100, 120, 110]
def getSecondHighest(a):
  hi = mid = lo = 0
  for i in range(0, len(a)):
    x = a[i]
    if ( x > hi):
      lo = mid
      mid = hi
      hi = x
    elif ( x < hi and x > mid):
      lo = mid
      mid = x
    elif ( x < lo):
      lo = x
  return mid

print(getSecondHighest(testCase))

Trust the math. When the x > hi comparison fails, there is no need to test against x < hi. The streamlined code should look like

    if x > hi:
        ....
    elif x > mid:
        ....
    else
        ....

Follow the math. In the worst case - the array sorted in the descending order - (almost) every element has go through two comparisons, making total of \$2n - 2\$, which is larger than the goal of \$n + \log{n} - 2\$
A correct approach is to arrange an elimination tournament (\$n - 1\$ comparisons to determine a champion), while keeping track of those who lose only to the champion. This makes \$log{n}\$ candidates for the second place, and \$\log{n} - 1\$ comparisons to determine the second. For the details, watch Alex Stepanov lectures starting with lecture 8, or just see the final code.

I highly recommend to watch the entire course, even if you are not interested in C++.


Calculate fractions of positive, negative and zeros Find the fractions for, all positive numbers, all negative numbers and zeros in a given set of integer array.

I wrote the below program. If you see any improvements required, please feel free to point them out.

Sample input:
6
-4 3 -9 0 4 1

Sample output:
0.500000
0.333333
0.166667

Here is my program :)

public class Solution {

    public static void main(String[] args) {

        Scanner scan = new Scanner(System.in);
        int size = scan.nextInt();
        scan.nextLine();

        String s = scan.nextLine();
        List<Double> doubleList = Arrays.stream(s.split(" ")).mapToDouble(Double::valueOf).collect(ArrayList<Double>::new, List::add, List::addAll);

        int postive = 0;
        int negative = 0;
        int zeros = 0;
        double total = (int) doubleList.stream().count();

        DoublePredicate filterPositives = d -> d > 0;
        DoublePredicate filterNegatives = d -> d < 0;
        DoublePredicate filterZeros = d -> d == 0;

        postive = countNumbers(doubleList.stream().mapToDouble(Double::valueOf), filterPositives);
        negative = countNumbers(doubleList.stream().mapToDouble(Double::valueOf), filterNegatives);
        zeros = countNumbers(doubleList.stream().mapToDouble(Double::valueOf), filterZeros);

        DecimalFormat df = new DecimalFormat("#.000000");
        System.out.println(df.format(postive/total));
        System.out.println(df.format(negative/total));
        System.out.println(df.format(zeros/total));
    }

    public static int countNumbers(DoubleStream ds, DoublePredicate fd){
        return (int) ds.filter(fd).count();
    }

}

Using the predicates in your case could be expensive for big list since you use 3 meaning 3 complete iteration on the list.

Here is a small update of you code (I can't use the Java 8 API since I only have the Java 6 at the office ...) to do the same check but without predicate (or if else)

    Scanner scan = new Scanner(System.in);
    int size = scan.nextInt();
    scan.nextLine();

    int[] sum = new int[3];

    String s = scan.nextLine();
    String[] array = s.split(" ");

    double total = array.length;
    for(String d : array ){ //Here, you shoud use your list instead of my array
        sum[Double.compare(Double.parseDouble(d), 0) + 1]++;
        //compare will return -1 | 0 | 1, adding one to increment the correct cell in the array.
    }

    DecimalFormat df = new DecimalFormat("#.000000");
    System.out.println(df.format(sum[2]/total));
    System.out.println(df.format(sum[0]/total));
    System.out.println(df.format(sum[1]/total));

PS : I could update the code to match your List but to prevent any typo, I prefer to show you the logic I used with the API I can use.


Bash/zsh function that cd to the root of git tree I'd like to get a review for the function that cd into the git tree root or does nothing if we are outside of the repository.
reviewed version:
# cd to the root of the current git directory
# If $PWD is submodule, will cd to the root of the top ancestor
# It requires to stay in the current directory, if the root is . or unknown,
# and use cd only once, to have a way to do `cd -`
function cg {
  git_root() {
    local super_root
    local top
    top="$(git rev-parse --show-cdup)"
    top="${top:-./}"
    super_root="$(git rev-parse --show-superproject-working-tree)"
    if [[ "$super_root" ]]; then
      printf '%s' "$top../"
      ( cd "$top../" && git_root || return )
    fi
    printf '%s' "$top"
  }
  local git_root
  git_root="$(git_root)"
  [ "x${git_root}" != "x./" ] && cd "$(git_root)" && return || return 0
}

updated version:
#!/bin/bash
# cd to the root of the current git directory
# If $PWD is submodule, will cd to the root of the top ancestor
# It requires to stay in the current directory, if the root is . or unknown,
# and use cd only once, to have a way to do `cd -`
function cg {
  function git_root {
    local top; top="$(git rev-parse --show-cdup)"
    top="${top:-./}"
    local super_root; super_root="$(git rev-parse --show-superproject-working-tree)"
    if [[ "$super_root" ]]; then
      printf '%s' "$top../"
      ( cd "$top../" && git_root || return )
    fi
    printf '%s' "$top"
  }
  local tree_root
  tree_root="$(git_root)"
  [[ "x${tree_root}" != "x./" ]] && cd "${tree_root}" && return || return 0
}

Great

100% pass on shellcheck which means you're doing great with quoting everything that could be potentially problematic.
scoping variables
using [[ conditional for the if
nice explanation of what it is doing and why

Could be better

a sh-bang line at the top is a good idea for scripts, even if this would usually just be sourced during your login scripts.
Your inner function is defined with different syntax than your outer function.
You can scope and define a variable in one line.  For example: local super_root="$(git rev-parse --show-superproject-working-tree)".  This makes sure you don't fail to define a local variable or fail to scope a new variable.  And it cuts out a line of code in each case.
Reusing git_root for a variable name and the function name is confusing.  I initially wondered why you were trying to scope the function after you had just defined it.
use [[ conditional for conditional in the last line.


Find busiest period in building The task

  You are given a list of data entries that represent entries and exits
  of groups of people into a building. An entry looks like this:

  {"timestamp": 1526579928, count: 3, "type": "enter"}

  This means 3 people entered the building. An exit looks like this:

  {"timestamp": 1526580382, count: 2, "type": "exit"}

  This means that 2 people exited the building. timestamp is in Unix
  time.

  Find the busiest period in the building, that is, the time with the
  most people in the building. Return it as a pair of (start, end)
  timestamps. You can assume the building always starts off and ends up
  empty, i.e. with 0 people inside.

const stamp = [
  {"timestamp": 1526579928, count: 3, "type": "enter"},
  {"timestamp": 1526580382, count: 2, "type": "exit"},
  {"timestamp": 1526579938, count: 6, "type": "enter"},
  {"timestamp": 1526579943, count: 1, "type": "enter"},
  {"timestamp": 1526579944, count: 0, "type": "enter"},
  {"timestamp": 1526580345, count: 5, "type": "exit"},
  {"timestamp": 1526580351, count: 3, "type": "exit"},
];

My imperative solution:

const findBusiestPeriod = lst => {
  let visitors = 0;
  const busiestPeriod = {
    start: null,
    end: null,
    maxVisitors: null,
  };

  lst.forEach((v, i) => {
    visitors = v.type === "enter" ?
      visitors + v.count :
      visitors - v.count;

    if (visitors > busiestPeriod.maxVisitors) {
      busiestPeriod.maxVisitors = visitors;
      busiestPeriod.start = v.timestamp;
      busiestPeriod.end = lst[i + 1].timestamp;
    } else if (visitors === busiestPeriod.maxVisitors){
      busiestPeriod.end = lst[i + 1].timestamp;
    }
  });
  return [busiestPeriod.start, busiestPeriod.end];
};

console.log(findBusiestPeriod(stamp));

My functional solution

const findBusiestPeriod2 = lst => {
  const {start, end} = lst.reduce((busiestPeriod, v, i) => {
    busiestPeriod.visitors = v.type === "enter" ?
      busiestPeriod.visitors + v.count :
      busiestPeriod.visitors - v.count;
    const visitors = busiestPeriod.visitors;
    if (!busiestPeriod.maxVisitors || visitors > busiestPeriod.maxVisitors) {
      busiestPeriod.maxVisitors = visitors;
      busiestPeriod.start = v.timestamp;
      busiestPeriod.end = lst[i + 1].timestamp;
    } else if (visitors === busiestPeriod.maxVisitors){
      busiestPeriod.end = lst[i + 1].timestamp;
    }
    return busiestPeriod;
  }, {visitors: 0, maxVisitor: null,});
  return [start, end];
};

console.log(findBusiestPeriod2(stamp));

From a short review;

I advise avoiding null, you could either not define the member or use undefined
busiestPeriod is a really long name, given how often you use it
You really only need to track the log entry with the most visitors, the start of the period would be timestamp of the entry before that
This: busiestPeriod.visitors = v.type === "enter" ?
  busiestPeriod.visitors + v.count :
  busiestPeriod.visitors - v.count; could be

busiestPeriod.visitors += (v.type == "enter" ? v.count : - v.count);

or even

busiestPeriod.visitors += (v.type == "enter" ? 1 : -1) * v.count;

This is my counter-proposal:

function findBusiestTime(logs){

  function analyzeLogEntry(acc, log, index){
    if(log.type == "enter"){
      acc.count += log.count;
    }else{
      if(acc.count > acc.maxCount){
        acc.maxCount = acc.count;
        acc.index = index;
      }
      acc.count -= log.count;
    }
    return acc;
  }

  let {index} = logs.reduce(analyzeLogEntry,{count:0, maxCount: 0});
  return [logs[index-1].timestamp,logs[index].timestamp];
}


Rust - Need help making code idiomatic and clean (with lifetimes) This is the first real program I have written in rust. What started as a clean piece of rust code, turned into a lifetime monstrosity when I decided to make my enums and structs use &str instead of String. The algorithm implemented is irrelevant; I just need help making my code more idiomatic and clean. Here is (quite simple) my code:
use std::collections::HashMap;
use std::hash::{Hash, Hasher};

#[derive(Debug, PartialEq, Eq)]
struct BasicBlock<'a> {
    block_id: u32,
    code: Vec<IRCode<'a>>,
    preds: Vec<Box<BasicBlock<'a>>>,
    succs: Vec<Box<BasicBlock<'a>>>,
}

#[derive(Debug, PartialEq, Eq)]
enum IRCode<'a> {
    Basic {
        value: &'a str,
        btype: Type,
        id: u32,
    },
    Quadruple {
        op: Op,
        params: Vec<IRCode<'a>>,
        location: Storage<'a>,
    },
    // and some more...
}

#[derive(Debug, PartialEq, Eq)]
enum Type {
    TVarRef,
    // and some more...
}

#[derive(Debug, PartialEq, Eq)]
enum Storage<'a> {
    Temp { name: &'a str },
}

#[derive(Debug, PartialEq, Eq)]
enum Op {
    Add,
    Sub,
}

impl<'a> Hash for BasicBlock<'a> {
    fn hash<H: Hasher>(&self, state: &mut H) {
        self.block_id.hash(state);
    }
}

#[derive(Debug)]
struct SSA<'a> {
    count: HashMap<&'a str, u32>,
    current_def: HashMap<&'a str, HashMap<Box<&'a BasicBlock<'a>>, Box<IRCode<'a>>>>,
}

impl<'a> SSA<'a> {
    fn new() -> SSA<'a> {
        SSA {
            count: HashMap::new(),
            current_def: HashMap::new(),
        }
    }

    fn new_variable(&mut self, name: &'a str, block: &'a BasicBlock<'a>) -> &Box<IRCode<'a>> {
        // If `name` is not in `count`, insert `(name, 0)`; else increment the count
        *self.count.entry(name).or_insert(0) += 1;
        self.current_def.entry(name).or_insert(HashMap::new());
        self.current_def.get_mut(&name).unwrap().insert(
            Box::new(block),
            Box::new(IRCode::Basic {
                value: name,
                btype: Type::TVarRef,
                id: *self.count.get(&name).unwrap(),
            }),
        );
        self.current_def.get(&name).unwrap().get(&block).unwrap()
    }
}

fn main() {
    // very simple test case
    let mut ssa = SSA::new();
    let bb0 = BasicBlock {
        block_id: 0,
        code: vec![],
        preds: vec![],
        succs: vec![],
    };
    println!("{:?}", ssa.new_variable("foo", &bb0));
    println!("{:?}", ssa);
}

Thanks!

Hello and welcome to the Rust community.
After a single read of your code, I have a couple comments.

your problems with &str as well as keying by block_id would be best remedied by putting interners in SSA. The best interner choice would be string-interner or internment.
in fn new_variable, you have repeated lookups for the same entry of current_def. LLVM will be unable to optimize out repeated accesses to HashMap. This may slow us down by something like 120~200ns per invocation of the function, because one HashMap lookup takes approx. 40~70ns.
you have Vec<Box<...>> and HashMap<Box<...>,Box<...>>. This inclusion of Box within another container immediately raises a flag in the mind of a Rust programmer that the code is not cleaned up. Wrapping in Box does not provide anything over wrapping in Vec, HashMap or any other container. All wraps provide unique ownership with memory indirection. To illustrate with an example, Box<[T]> is almost like Vec in that both are owned dynamic-length slices (indexable memory ranges) except that Box<[T]> does not allow any overprovisioning of memory (Vec can have capacity above length).
the SSA::current_def type expression is basically as long as your average tweet. Break down complex types longer than 30-40 characters into smaller definitions.

We will begin by changing SSA and fn new_variable, with new structure SSADef:
#[derive(Debug)]
struct SSA<'a> {
    content: HashMap<&'a str, SSADef<'a>>,
}

#[derive(Debug)]
struct SSADef<'a> {
    count: u32,
    current_def: HashMap<&'a BasicBlock<'a>, IRCode<'a>>,
}

impl<'a> SSA<'a> {
    fn new() -> SSA<'a> {
        SSA { content: HashMap::new() }
    }

    fn new_variable<'s>(&'s mut self, name: &'a str, block: &'a BasicBlock<'a>) -> &'s IRCode<'a> {
        // If `name` is not in `content`, insert `SSADef::new()`
        let def = self.content.entry(name).or_insert_with(SSADef::new);
        // Increment the count
        def.count += 1;
        def.current_def.entry(block).or_insert(
            IRCode::Basic {
                value: name,
                btype: Type::TVarRef,
                id: def.count,
            }
        )
    }
}

impl<'a> SSADef<'a> {
    fn new() -> SSADef<'a> {
        SSADef {
            count: 0,
            current_def: HashMap::new(),
        }
    }
}

One minor observation: you can derive Default for SSA and SSADef and call default from both news. This will put 0s and empty HashMaps and other default values in the fields of these structures.
We add the StringInterner. I assume the deeper functionality will always have easy access to top level SSA, and consequently easy access to the interner. If not, we can use some other crate such as internment which provides an appropriate API.
Changing the BasicBlocks for interning is left as an exercise for the reader. We arrive at the following code, which has very few 'as left. This is all I can do without knowledge of the algorithm, I believe:
use std::collections::HashMap;
use std::hash::{Hash, Hasher};

use string_interner::{StringInterner, DefaultSymbol as Sym};

#[derive(Debug, PartialEq, Eq)]
struct BasicBlock {
    block_id: u32,
    code: Vec<IRCode>,
    preds: Vec<BasicBlock>,
    succs: Vec<BasicBlock>,
}

#[derive(Debug, PartialEq, Eq)]
enum IRCode {
    Basic {
        value: Sym,
        btype: Type,
        id: u32,
    },
    Quadruple {
        op: Op,
        params: Vec<IRCode>,
        location: Storage,
    },
    // and some more...
}

#[derive(Debug, PartialEq, Eq)]
enum Type {
    TVarRef,
    // and some more...
}

#[derive(Debug, PartialEq, Eq)]
enum Storage {
    Temp { name: Sym },
}

#[derive(Debug, PartialEq, Eq)]
enum Op {
    Add,
    Sub,
}

impl Hash for BasicBlock {
    fn hash<H: Hasher>(&self, state: &mut H) {
        self.block_id.hash(state);
    }
}

#[derive(Debug, Default)]
struct SSA<'a> {
    content: HashMap<Sym, SSADef<'a>>,
    interner: StringInterner,
}

#[derive(Debug, Default)]
struct SSADef<'a> {
    count: u32,
    current_def: HashMap<&'a BasicBlock, IRCode>,
}

impl<'a> SSA<'a> {
    fn new() -> SSA<'a> {
        SSA::default()
    }

    fn new_variable<'s>(&'s mut self, name: &'a str, block: &'a BasicBlock) -> &'s IRCode {
        let name_sym = self.interner.get_or_intern(name);
        // If `name` is not in `content`, insert `SSADef::new()`
        let def = self.content.entry(name_sym).or_insert_with(SSADef::new);
        // Increment the count
        def.count += 1;
        def.current_def.entry(block).or_insert(
            IRCode::Basic {
                value: name_sym,
                btype: Type::TVarRef,
                id: def.count,
            }
        )
    }
}

impl<'a> SSADef<'a> {
    fn new() -> SSADef<'a> {
        SSADef::default()
    }
}

fn main() {
    // very simple test case
    let mut ssa = SSA::new();
    let bb0 = BasicBlock {
        block_id: 0,
        code: vec![],
        preds: vec![],
        succs: vec![],
    };
    println!("{:?}", ssa.new_variable("foo", &bb0));
    println!("{:?}", ssa);
}
```


An "Add" button that clones an item, and a "Remove" button that fades and removes an item I was looking into removing multiple inputs and selects at once using jQuery, but I couldn't find a solution, so I ended up with the following code:

$(function() {

  $('a.add').click(function(evt) {
    //clone lists
    //is there any way to simplify this part --begin--
    $('#s0 option').clone().appendTo('#s'+obj);
    $('#i0 option').clone().appendTo('#i'+obj);
    $('#r0 option').clone().appendTo('#r'+obj);
    $('#ieu0 option').clone().appendTo('#ieu'+obj);
    $('#ied0 option').clone().appendTo('#ied'+obj);
    //--end--
    evt.preventDefault(); //prevents scrolling
  });

  //remove inputs/selects
  $('a.remove').click(function(evt) {
    //animation is not necessary. How can I simplify this code? --begin--
    $('#comp'+x).animate({opacity:"hide"}, "slow").remove();
    $('#ppt'+x).animate({opacity:"hide"}, "slow").remove();
    $('#ct'+x).animate({opacity:"hide"}, "slow").remove();
    $('#p'+x).animate({opacity:"hide"}, "slow").remove();
    $('#ipvs'+x).animate({opacity:"hide"}, "slow").remove();
    $('#s'+x).remove();
    $('#i'+x).remove();
    $('#r'+x).remove();
    $('#ipvs'+x).remove();
    $('#other'+x).remove();
    //--end--
  }
  evt.preventDefault();
  });
});

How can the sections marked between --begin-- and --end-- be simplified/improved?

I would go with ChrisThompson's method of adding classes and referring to them.
But to answer your question straight and simple

the first block can look like

$.each( ['s','i','r','ieu','ied'], function(index,item){
        $('#' + item + '0 option').clone().appendTo('#' + item + obj );
    });

However, you did not specify what obj is

The second block can use the same technic to be simplified.

The entire code would look something like this

$(function() {
  $('a.add').click(function(evt) {
    //clone lists
    //is there any way to simplify this part --begin--
    $.each( ['s','i','r','ieu','ied'], function(index,item){
        $('#' + item + '0 option').clons().appendTo('#' + item + obj );
    });
    //--end--
    evt.preventDefault(); //prevents scrolling
  });

  //remove inputs/selects
  $('a.remove').click(function(evt) {
    //animation is not necessary. How can I simplify this code? --begin--
    $.each( ['comp','ppt','ct','p','ipvs'], function(index,item){ $('#' + item + x).animate({opacity:"hide"} ,"slow").remove();});
    $.each(['s','i','r','other'], function(index,item){ $('#'+item+x).remove()});
    //--end--
    evt.preventDefault();
  }
  });
});

However, it seems to me you are using too complex HTML structure.
you need to

Stop using IDs, and start using classes for elements that have things in common
For example :

Instead of cloning each item, you can wrap all of the in a DIV and simple use html() to clone the entire HTML. When you inject the HTML to some other div, give the new wrapper div an ID.

For example

$(".my-form").append($("<div/>").addClass(".object-item").attr("id","newObject" + $(".my-form .object-item").length ).html( $(".items-to-clone-wrapper").html() );

You then have a unique identifier for every element on the page. for example $("#newObject1 .s") instead of $("#s1").

Here is a fiddle I made on the assumptions I made about your code. I hope it will be of use to you. please let me know if there's something I can do.


Generating .xml files based on .csv files This reads in data from a .csv file and generates an .xml file with respect to the .csv data. Can you spot any parts where re-factoring can make this code more efficient?

import csv
from xml.etree.ElementTree import Element, SubElement, Comment, tostring
from xml.etree.ElementTree import ElementTree
import xml.etree.ElementTree as etree

def main():
    reader = read_csv()
    generate_xml(reader)

def read_csv():
    with open ('1250_12.csv', 'r') as data:
        return list(csv.reader(data))

def generate_xml(reader):
    root = Element('Solution')
    root.set('version','1.0')
    tree = ElementTree(root)

    head = SubElement(root, 'DrillHoles')
    head.set('total_holes', '238')

    description = SubElement(head,'description')
    current_group = None
    i = 0
    for row in reader:
        if i > 0:
            x1,y1,z1,x2,y2,z2,cost = row
            if current_group is None or i != current_group.text:
                current_group = SubElement(description, 'hole',{'hole_id':"%s"%i})

                collar = SubElement (current_group, 'collar',{'':', '.join((x1,y1,z1))}),
                toe = SubElement (current_group, 'toe',{'':', '.join((x2,y2,z2))})
                cost = SubElement(current_group, 'cost',{'':cost})
        i+=1

    def indent(elem, level=0):
        i = "\n" + level*"  "
        if len(elem):
            if not elem.text or not elem.text.strip():
                elem.text = i + "  "
            if not elem.tail or not elem.tail.strip():
                elem.tail = i
            for elem in elem:
                indent(elem, level+1)
            if not elem.tail or not elem.tail.strip():
                elem.tail = i
        else:
            if level and (not elem.tail or not elem.tail.strip()):
                elem.tail = i
    indent(root)
    tree.write(open('holes1.xml','w'))

Minor stuff:

Remove the last import - etree is not used anywhere.
Merge the two first imports

Possibly speed-improving stuff:

Avoid converting the csv.reader output before returning unless absolutely necessary.
Skip indent unless the output must be readable by a human with a non-formatting editor.
If you need to indent the output, existing solutions are probably very efficient.
Use reader.next() to skip the header line in generate_xml, then you don't need to keep checking the value of i.


Expect script that tests if a passwordless connection is setup I have written a simple expect script to tell me if a passwordless connection is set up.
#!/usr/bin/expect -f

if {[llength $argv] < 2} {
  puts "usage: test-nopass user server"
  exit 1
}

set user     [lindex $argv 0]
set server   [lindex $argv 1]

set pwd_prompt "*assword:"
set prompt "*$ "

set rc 0

log_user 0
spawn ssh $user@$server

expect {
  "$pwd_prompt" { exit 1 }
  eof           { exit 2 }
  timeout       { exit 3 }

  "$prompt"     {
    send "hostname\r"
    expect {
      "*$server*" { exit 0 }
      eof         { exit 4 }
      timeout     { exit 5 }
    }
  }
}

log_user 1

exit $rc
```

A couple of notes:

expect is an extension of Tcl, so you can use any Tcl command: http://www.tcl-lang.org/man/tcl8.6/TclCmd/contents.htm

specifically, you could write
lassign $argv user server

which is arguably a bit less readable, but is only one command.

You can speed up the timeouts: the default is 10 seconds, and you probably don't want to wait for 20 seconds to get the final exit status:
set timeout 1   ;# in seconds

you never reset the rc variable, and the default exit status is zero already, so you can remove set rc 0 and exit $rc

expect is not bash: you don't need to quote all the variables.

you don't need to reset log_user just before exiting the script.

expect code written in this style can get pretty deeply nested: The last pattern in an expect command does not need an action block
expect {
  $pwd_prompt  { exit 1 }
  eof          { exit 2 }
  timeout      { exit 3 }
  $prompt
}
send "hostname\r"
expect {
  *$server*    { exit 0 }
  eof          { exit 4 }
  timeout      { exit 5 }
}

If $prompt is seen, then that expect command ends, and the script continues with send


operations with complex numbers in C++ #include <iostream>
#include <fstream>
#include <cmath>
using namespace std;

class Complex {
private:
    double Rez, Imz, ro, fi;

public:
    Complex() {
        Rez = 0.;
        Imz = 0.;
        ro = 0.;
    fi = 0.;
}

Complex(double x, double y) {
    Rez = x;
    Imz = y;
    ro = sqrt(pow(x,2)+pow(y,2));
    fi = atan(y/x);
 }

~Complex() {}

double get_Rez() {return Rez;}
double get_Imz() {return Imz;}
double get_ro() {return ro;}
double get_fi() {return fi;}

void set_Rez(double a) {Rez = a; ro = sqrt(pow(Rez,2)+pow(Imz,2)); fi = acos(Rez/ro);}
void set_Imz(double a) {Imz = a; ro = sqrt(pow(Rez,2)+pow(Imz,2)); fi = acos(Rez/ro);}
void set_ro(double a) {ro = a; Rez = ro*cos(fi); Imz = ro*sin(fi);}
void set_fi(double a) {fi = a; Rez = ro*cos(fi); Imz = ro*sin(fi);}

Complex & inverso() {
    this->set_Rez(this->get_Rez()/pow(this->get_ro(),2));
    this->set_Imz(-this->get_Imz()/pow(this->get_ro(),2));
    return *this;
}

};

These two are the functions to work with my complex numbers. Is it possible to change them in order to make them accept also doubles as input? In that case, would it be possible to make them give a double as an output? Is it correct to say that my functions haven't got side effects? Would it be better to put the functions in the class as data members?

Complex somma(Complex a, Complex b) {
    Complex c;
    c.set_Rez(a.get_Rez()+b.get_Rez());
    c.set_Imz(a.get_Imz()+b.get_Imz());
    return c;
}

Complex prodotto(Complex a, Complex b) {
    Complex c;
    c.set_ro(a.get_ro()*b.get_ro());
    c.set_fi(a.get_fi()+b.get_fi());
    return c;
}

A complex number class is very useful! This is pretty straightforward and easy to understand, too.
The Class
First, I should mention that if you haven't already seen it, there is a complex class in the standard template library. It's great to make your own so that you can learn how it works, but just wanted to mention it in case you missed it.
I think having 2 ways of using the data (real and imaginary parts vs. magnitude and angle) is a great idea. However, I would keep only one pair around and have functions to calculate the others. For example, you might keep the real and imaginary parts and calculate the magnitude and angle only when needed. (Or the reverse if you find that it's more common to use the magnitude and angle.) The reason is that one set of the data can get out of sync with the other. The constructor that takes arguments sets the real and imaginary parts, and then calculates the magnitude and angle. The set_* methods also have to recalculate both parts. If you ever expand the class, you'll have to ensure that every method always does the calculations 2 ways - once for real/imaginary parts, and again for magnitude and angle. This is likely to be slow. If you kept only one representation, you could simply do the conversion to the other representation only when asked for data in that format.
The Functions
I'm guessing from the names that the 2 functions calculate the sum and product of 2 Complex numbers? You ask:

Is it possible to change them in order to make them accept also doubles as input?

There are 2 ways you could approach that. You could simply make an overloaded version:
double somma(double a, double b) {
    double c;
    c = a + b;
    return c;
}

Or you could use templates. To do that, you need to have an operator+() for your Complex class. Then you can write somma() as:
template <typename T>
T somma(T a, T b) {
    T c;
    c = a + b;
    return c;
}

The compiler will then change out T for whatever type the arguments are when you call it. If you call it like this:
Complex x(2, 5);
Complex y(3, 6);
Complex z = somma(x, y);

it will do what your function currently does. If x, y, and z are float or double, then it will use float for the type and return the expected result.

Is it correct to say that my functions haven't got side effects?

Yes, it is. They don't set any state that a caller can't see. They don't cause anything to be output other than the return value of the function.

Would it be better to put the functions in the class as data members?

That depends on how you're going to use them. In general, for a class that represents a type of number having operator+() and operator*() (and related ones like operator+=() etc.) is probably a good idea.


Passing a 1D array (single column of a Range object) to .NET to manipulate it without loops and returning it back to VBA Background

Kind of a follow up - slightly related to my other SO question.

I thought that if somehow I find a way in VBA to pass a single column to .NET and convert it to a native .NET type then I could finally overcome the mystery of explicitly looping on objects via COM (which is super slow as we all know). I'd then be able to use all the cool stuff and not worry about constantly looping through Excel.Range object.

Please note: this is all about a single column not an object[,].

VBA side of things

It turns out in VBA you can get a 1D array like this:

.NET Compatibility

Sample .tlb/.dll library in C# - click here to get an idea

It comes down to one method really:

public object Deduplicate1DArray(object arr)
{
    Array column = (Array)arr; // converts assumed/expected 1d array to System.Array
    List<string> list = column.OfType<string>().ToList(); // generic list to use the cool stuff
    column = list.Distinct<string>().ToArray(); // convert back to match the type
    return column;
}

So from all the research I have done I thought that passing the 1d from VBA to .NET as object type is the way to go. If you know/experienced a better way (like passing the Excel.Range please share).

A successful conversion to a native .NET System.Array is possible which is just sweet because an Array can easily be converted to a generic List<string> which gives me access to all the cool stuff.

So at this point I am not relying any longer on VBA to manipulate that Range and I can do amazing things that .NET allows me to on a generic List<T> type - I mean, how cool's that?

Testing...

Attached references in the VBE to my .tlb and now I am able to use my library like

Dim c as new COMClass

Currently, I can test it like this:

Sub Main()

    Cells.ClearContents

    [A1] = "foo"
    [A2] = "boo"
    [A3] = "doo"
    [A4] = "goo"
    [A5] = "foo"

    Dim arr1D As Variant
    arr1D = Application.Transpose(Range("A1:A5"))

    Dim c As New COMClass

    Dim noDuplicates As Variant
    noDuplicates = c.Deduplicate1DArray(arr1D)

    Range("B1").Resize(UBound(noDuplicates) + 1, 1) = Application.Transpose(noDuplicates)

End Sub

or

noDuplicates = Application.Transpose(c.Deduplicate1DArray(arr1D))
Range("B1").Resize(UBound(noDuplicates), 1) = noDuplicates

and that works as expected (either way)

What bothers me...

A user of the library has to know and remember that the COMClass.Deduplicate1DArray needs to be passed a 1d array... therefore, in VBA they have to call the Application.Transpose() on the Range object to pass it properly. Things can get very .Net'ish if the user decides to do things like:

noDuplicates = Application.Transpose(c.Deduplicate1DArray(Application.Transpose(Range("A1:A5"))))

or even worse

Range("B1").Resize(SOME_BIG_NUMBER, 1) = Application.Transpose(c.Deduplicate1DArray(Application.Transpose(Range("A1:A5"))))

Anything I could do in either VBA or C# to improve that? Creating another wrapper or maybe just accepting the Excel.Range object and try to break it down to a 1d array in .NET?
This:

Range("B1").Resize(10, 1) = c.Deduplicate1DArray(Range("A1:A5").Value)

would have been kind of good or at least look sensible in terms of syntax but the problem is I shouldn't be .Resizing my B1 - (destination) because I don't know how big the returned array after deduplication is going to be.

Any ideas of improving this are very welcome and if I have missed something please let me know I'll update with as much detail as I can.

I don't know much about VBA or COM, so I'm going to focus on the single C# function:

If you know that all elements in the input sequence are of the same type, you should use Cast, not OfType. OfType filters out all elements of the wrong type, which could easily hide bugs.

You don't need to call ToList to start using LINQ methods, they work on anything that's IEnumerable<T>, which includes the results of Cast and OfType.

When calling a method like Distinct, you don't need to specify the type parameter (string in your case), the compiler is able to infer it itself.


Check if vector of strings has matching brackets For an online screen, I had a question that would take in a vector of strings, and return a vector of strings indicating whether the input string was a valid combination of []{}(), where valid means the braces are matching and not interleaved. There are no empty string inputs.

For example, [(())] is valid, but ), {, and {[}] are invalid.

Note that input is only brackets, output is YES or NO, depending on whether or not the input is only matching brackets.

Also note using namespace std is required by the online software (hackerrank).

/*
 * Increments each value in vec by the delta.
 */
void increment(vector<int> &vec, int delta){
    transform(vec.begin(), vec.end(), vec.begin(), [delta](int x){return x+delta;});
}

/*
 * Checks if a bracket has been closed, and if it has, checks that it
 * was expected to be closed.  Returns true if there is an invalid close.
 * Note that vectors are in sorted order, since we only add indices when
 * a new bracket is opened.
 */
bool check(vector<int> &vec, bool expected){
    if(vec.empty()){ //if there are no open brackets, check if we expected one
        if (expected){
            return true;
        }
    }
    else if(vec.back() == 0){ //if the there is a bracket that was closed, check if we expected that
        vec.pop_back();
        if (!expected){
            return true;
        }
    }
    return false;
}

/*
 * Complete the function below.
 */
vector<string> braces(vector<string> values) {
    vector<pair<char, char>> bracket_types;
    bracket_types.push_back(pair<char, char>('{','}'));
    bracket_types.push_back(pair<char, char>('(',')'));
    bracket_types.push_back(pair<char, char>('[',']'));
    vector<string> results;
    for(auto const &str: values){
        /* Map of vectors containing integers indicating the number of opening brackets
         * seen since each bracket was first seen, minus the number of closing.
         * When an index is zero, that means that bracket should be closed, so we
         * check it using the check method.
         */
        map<char, vector<int> > counts;

        for(auto const &c : str){ //iterate over the string of brackets
            int incr;
            for(auto const &type : bracket_types){  //find which bracket we are using
                if (type.first == c){  //if it's an opening bracket, add a new open bracket to the vector
                    incr = 1;
                    counts[c].push_back(0);
                    break;
                }
                if (type.second == c){  //if it's a closing bracket, set incr to decrement each value
                    incr = -1;
                    break;
                }
            }
            for(auto &cnt : counts){  //for all vectors, update values, and check if we have an invalid close.
                increment(cnt.second, incr);
                if (incr < 0 && check(cnt.second, cnt.first==c)){  //if we closed a bracket, could be invalid, check
                    cnt.second.push_back(0);  //add a bracket to a vector, so that we will have the right return
                    break;
                }
            }
        }
        //Return false if there are unclosed brackets.
        results.push_back("YES");
        for(auto const &cnt : counts){
            if (!cnt.second.empty()){
                results.back() = "NO";
            }
        }
    }
    return results;
}

Comments

You have overwhelming amount of comments. It looks to be too much. Usually code should document itself by having good names and intuitive behavior. Intuitive behavior would be one of standard library, for example. Other candidate is int type. In the extreme case you should write why the code does what it does, not what it does.

Implementation

It is really hard to understand what your code does, since it's not straightforward. Code should aim at correctness first, clarity second, performance third.

It's good that you're using range loops.

   if (type.first == c){  //if it's an opening bracket, add a new open bracket to the vector
                incr = 1;
                counts[c].push_back(0);
                break;
            }
            if (type.second == c){  //if it's a closing bracket, set incr to decrement each value
                incr = -1;
                break;
            }

This place of the code is very confusing. In the beginning I thought that we could use else to chain this, but then I noticed that it's type.first and type.second, which changed my opinion. But then I found that it always compares to c, so now it's pretty clear that there have to be else, since second shouldn't be checked if first condition is met. It is good both clarity and performance wise.

        int incr;

outside loop. It should be inside, because it's logical scope belongs to the loop. If you worry about performance, compiler should be able to figure that out, so your code will be both readable and performant.

Your braces function violate single responsibility principle. The principle greatly improves the code if used correctly.

Names are ambiguous in some places.

So lets make the code better.

Suggested implementation

First of all, lets nail down the name of the "entry point" function. is_valid_brace_sequence in my opinion is alright.

std::vector<std::string> is_valid_brace_sequence(const std::vector<std::string>& input)

Notice const std::vector<std::string>&. Algorithm doesn't need a copy. Copying will probably decrease performance.

If it wouldn't be part of problem statement, I would suggest having 2 input iterators for the beginning and end of the input, and one output iterator for the beginning of the output, so algorithm wouldn't mess with allocations.

Next, the algorithm goes over input, calls checker function on every element and records the results:

std::vector<std::string> results;
results.reserve(input.size());
for (const auto& element : input)
{
    bool result = check_validity(element);
    if (result)
    {
        results.push_back("YES");
    }
    else
    {
        results.push_back("NO");
    }
}

return results;

Using reserve increases performance, since vector won't need to resize. Also, it should be used in places where the size of the input in elements is known before hand.

Next, is check_validity function. It will also preserve const correctness:

bool check_validity(const std::string& input)

The algorithm used will be the following: if either left sided brace is encountered ('(', '[','{'), right sided brace will be put into stack. If right sided brace is encountered, top of the stack is checked for the exactly same brace, if it's not, return false.

bool check_validity(const std::string& input)
{
    std::stack<char> sequence;
    for (const auto& character: input)
    {
        switch (character)
        {
        case '(':
            sequence.push(')');
            break;
        case '{':
            sequence.push('}');
            break;
        case '[':
            sequence.push(']');
            break;
        case ')':
            if (!is_expected_brace(sequence, character))
            {
                return false;
            }
            break;
        case '}':
            if (!is_expected_brace(sequence, character))
            {
                return false;
            }
            break;
        case ']':
            if (!is_expected_brace(sequence, character))
            {
                return false;
            }
            break;
        default:
            continue;
        }
    }

    return true;
}

So the reason the algorithm works is that stack is LIFO container (last in first out). Since the type of the right sided brace should match the last left sided, so stack matches needs of the algorithm perfectly. Notice the use of switch. It increases performance because it generates jump table.

I've put all the boilerplate code of checking on encounter of right sided brace into the separate function:

bool is_expected_brace(std::stack<char>& sequence, char brace)
{
    if (sequence.empty() || sequence.top() != brace)
    {
        return false;
    }
    sequence.pop();
    return true;
}

Notice the logical OR operator (||). It provides short circuit ifs, so accessing the stack won't be undefined behavior.

Although it increases readability of the caller function, both of them has tight coupling between each other, so it limits the usability of them in case they are separated.

Put together

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

bool is_expected_brace(std::stack<char>& sequence, char brace)
{
    if (sequence.empty() || sequence.top() != brace)
    {
        return false;
    }
    sequence.pop();
    return true;
}

bool check_validity(const std::string& input)
{
    std::stack<char> sequence;
    for (const auto& character: input)
    {
        switch (character)
        {
        case '(':
            sequence.push(')');
            break;
        case '{':
            sequence.push('}');
            break;
        case '[':
            sequence.push(']');
            break;
        case ')':
            if (!is_expected_brace(sequence, character))
            {
                return false;
            }
            break;
        case '}':
            if (!is_expected_brace(sequence, character))
            {
                return false;
            }
            break;
        case ']':
            if (!is_expected_brace(sequence, character))
            {
                return false;
            }
            break;
        default:
            continue;
        }
    }

    return true;
}

std::vector<std::string> is_valid_brace_sequence(const std::vector<std::string>& input)
{
    std::vector<std::string> results;
    results.reserve(input.size());
    for (const auto& element : input)
    {
        bool result = check_validity(element);
        if (result)
        {
            results.push_back("YES");
        }
        else
        {
            results.push_back("NO");
        }
    }

    return results;
}

int main()
{
    std::vector<std::string> test_input{ "{{}}", "[[[]]", "[](){}", "[[])", "{[()]}" };
    std::vector<std::string> results = is_valid_brace_sequence(test_input);
    for (const auto& element : results)
    {
        std::cout << element << " ";
    }

}


Is wrapping exceptions good practice? I have recently been refactoring some code in an effort to improve the exception handling, particularly to aid in improving the level of information given to developers during development. However I am worried that I have started down the wrong path.
For example we were getting complaints that the following code threw an exception with the message: Sequence contains no elements when a database does not contain a record with the supplied ID.
public Record GetRecord(string id)
{
    _dbContext.Records.First(r => r.Id == id);
}

After refactoring the code, it now throws an exception whose message is No record found with id: {0}.
public Record GetRecord(string id)
{
    try
    {
        _dbContext.Records.First(r => r.Id == id);
    }
    catch(InvalidOperationException ex)
    {
        throw new ApplicationException(string.Format("No record found with id: {0}". id), ex);
    }
}

I can understand that this error message might be clearer, as we now know that no record was found. However, using this logic, where should we stop? Surely the below would also be an 'improvement'
public Record GetRecord(string id)
{
    try
    {
        _dbContext.Records.First(r => r.Id == id);
    }
    catch(InvalidOperationException ex)
    {
        throw new ApplicationException(string.Format("No record found with id: {0}".
    }
    catch(SqlException ex)
    {
        throw new ApplicationException("An error occurred while processing the record"), ex);
    }
}

I would consider it bad practice to wrap exceptions like this, however I am willing to be proved wrong. Is there any reason why this would be desirable? Where should the line be drawn?
Microsoft provide the following advice

Consider wrapping specific exceptions thrown from a lower layer in a more appropriate exception, if the lower layer exception does not make sense in the context of the higher-layer operation.
This should be done rarely because it makes debugging more difficult. It is appropriate when you are certain that the lower layer is never the true source of the error.

However surely an exception can also be wrapped in a more appropriate exception?

There is little reason to do this. If you include the stacktrace in your logs you already know where the problem exists; the only thing you'd miss is the input which you might know from different channels (and often isn't necessary to figure out the problem).

Noting that your example returns void which is probably not what you intended, I should say that the change I would make is to use .FirstOrDefault() instead of First which will instead return the default value - null for a reference type. You can then work with a simple null check to see whether or not a record was returned instead of having to start using try-catches.

Yes, you could do all this to provide specific error messages. Personally I am not a fan of it and I prefer to derive this from the code itself (through for example the before-mentioned null check).


Getting NTFS permissions of all shared folders on the local machine How can I improve the speed of the script?

$Excel = New-Object -Com Excel.Application
$Excel.visible = $True
$Excel = $Excel.Workbooks.Add()

$wSheet = $Excel.Worksheets.Item(1)
$wSheet.Cells.item(1, 1) = "Folder Path:"
$wSheet.Cells.Item(1, 2) = "Users/Groups:"
$wSheet.Cells.Item(1, 3) = "Permissions:"
$wSheet.Cells.Item(1, 4) = "Permissions Inherited:"

$WorkBook = $wSheet.UsedRange
$WorkBook.Interior.ColorIndex = 8
$WorkBook.Font.ColorIndex = 11
$WorkBook.Font.Bold = $True

 ####Change the path to the folder or share you want NTFS perms on####
 $dirToAudit = Get-ChildItem -Path "c:\inetpub" -recurse | Where { $_.psIsContainer -eq $true }

 $intRow = 1
 foreach ($dir in $dirToAudit)
 {
     $colACL = Get-Acl -Path $dir.FullName

foreach ($acl in $colACL)
 {
    $intRow++
    $wSheet.Cells.Item($intRow, 1) = $dir.FullName

    foreach ($accessRight in $acl.Access)
    {
        $wSheet.Cells.Item($intRow, 2) = "$($AccessRight.IdentityReference)"
        $wSheet.Cells.Item($intRow, 3) = "$($AccessRight.FileSystemRights)"
        $wSheet.Cells.Item($intRow, 4) = $acl.AreAccessRulesProtected
        $intRow++
    }
}

}
 $WorkBook.EntireColumn.AutoFit()

The only way to improve the speed of the script is to drop the usage of Excel as a COM-object altogether. It is considered bad practice. And it is not possible on servers where Excel is not installed.

It's better to use a library like NPOI (.NET port from Apache POI (java)).

I used this (old) example as a base and adapted it to roughly match your code.

[Reflection.Assembly]::LoadFrom("C:\Users\IEUser\Desktop\Release\Net40\NPOI.dll")
[Reflection.Assembly]::LoadFrom("C:\Users\IEUser\Desktop\Release\Net40\NPOI.OOXML.dll")
[Reflection.Assembly]::LoadFrom("C:\Users\IEUser\Desktop\Release\Net40\NPOI.OpenXml4Net.dll")
[Reflection.Assembly]::LoadFrom("C:\Users\IEUser\Desktop\Release\Net40\NPOI.OpenXml4Net.dll")
[Reflection.Assembly]::LoadFrom("C:\Users\IEUser\Desktop\Release\Net40\NPOI.OpenXmlFormats.dll")
[Reflection.Assembly]::LoadFrom("C:\Users\IEUser\Desktop\Release\Net40\ICSharpCode.SharpZipLib.dll")

$wb = New-Object NPOI.XSSF.UserModel.XSSFWorkbook;
$ws = $wb.CreateSheet("output");
$ws.CreateRow(0)| out-null;

$dirToAudit = Get-ChildItem -Path "C:\Users\IEUser" -recurse | Where { $_.psIsContainer -eq $true }

$intRow = 1

foreach ($dir in $dirToAudit)
{
    $colACL = Get-Acl -Path $dir.FullName

    foreach ($acl in $colACL)
    {
        $fileNameRow = $ws.CreateRow($intRow)
        $fileNameRow.CreateCell(1).SetCellValue($dir.FullName)
        $intRow++

        foreach ($accessRight in $acl.Access)
        {
            $values = $ws.CreateRow($intRow)
            $values.CreateCell(2).SetCellValue($($AccessRight.IdentityReference).ToString())
            $values.CreateCell(3).SetCellValue($($AccessRight.FileSystemRights).ToString())
            $values.CreateCell(4).SetCellValue($($acl.AreAccessRulesProtected).ToString())
            $intRow++
        }
    }

}
$fs = new-object System.IO.FileStream("C:\Users\IEUser\Desktop\test.xlsx",[System.IO.FileMode]'Create',[System.IO.FileAccess]'Write')
$wb.Write($fs);
$fs.Close()

There is also another library for this kind of stuff: EPPLus

And someone else made some nice powershell functions for it.


FizzBuzz Implementation in Java Fairly new to learning interview questions and I saw this one and gave it a shot. I would very much appreciate some constructive criticism of how I went about it and what you would tell me if you were the interviewer and watched me implement it this way.

public class FizzBuzz {
  public static void main(String[] args) {
    // Go from 1 to 100
    for (int i = 1; i <= 100; i++) {
      String result = "";
      if (i % 3 == 0) { // If divisible by 3
        result += "Fizz";
      }
      if (i % 5 == 0) { // If divisible by 5
        result += "Buzz";
      }
      // If it was divisible by either 3 or 5 (or both), print Fizz, Buzz, or FizzBuzz accordingly.
      if (i % 3 == 0 || i % 5 == 0) {
        System.out.println(result);
      } else { // Or else just print the integer
        System.out.println(i);
      }
    }
  }

The point of FizzBuzz is to weed out obviously unqualified candidates, not to over-optimize the solution or to show off.  Your code is a fine interview answer.

Make sure that your braces all match up.  It would be more conventional to use 4 or 8 spaces per level of indentation.

My main suggestion — and this is an opinionated nitpick — is to tone down the comments, because comments that restate obvious code give the impression that you might be unfamiliar with the language.  Experienced programmers never write // Go from 1 to 100 to explain for (int i = 1; i <= 100; i++).  If you feel compelled to demonstrate your documentation skills, write some JavaDoc instead.


F# functions implementing polynomial arithmetic I have a couple of functions for doing basic arithmetic on polynomials.

let rec mulC x ys =
match (x,ys) with
| (_,[])                    -> []
| (x,y::ys) when x > 0    -> x * y::mulC x ys

let rec addE xs ys =
match (xs, ys) with
| ([],[])                   -> []
| ([], y::ys)            -> y::ys
| (x::xs,[])             -> x::xs
| (x::xs, y::ys)      -> (x + y) :: addE xs ys

let mulX xs =
match xs with
| []                    -> []
| xs                    -> 0::xs

let rec mul qs = function
| []                    -> []
| x::tail               -> addE (mulC x qs)
                                (mulX(mul qs tail))

| (x,y::ys) when x > 0    -> x * y::mulC x ys

| (x::xs, y::ys)      -> (x + y) :: addE xs ys

Why the inconsistency in parentheses and whitespace? For what it's worth, I think the second is more readable.

let rec mulC x ys =
match (x,ys) with
| (_,[])                    -> []
| (x,y::ys) when x > 0    -> x * y::mulC x ys

What about the case x = 0, ys is non-empty? Don't you get a warning? (I also find it odd that the test is >. I infer that you're only intending to handle non-negative coefficients, but that seems like an unnecessary restriction).

let rec addE xs ys =
match (xs, ys) with
| ([],[])                   -> []
| ([], y::ys)            -> y::ys
| (x::xs,[])             -> x::xs
| (x::xs, y::ys)      -> (x + y) :: addE xs ys

There's no need to pattern-match a structure if you're not using the structure. This could be simplified to

let rec addE xs ys =
  match (xs, ys) with
  | ([], ys)            -> ys
  | (xs,[])             -> xs
  | (x::xs, y::ys)      -> (x + y) :: addE xs ys

Why are the three auxiliary functions in the top-level scope? I admit that I don't know F#, but the languages that it's heavily inspired by would allow them to be defined in a let clause inside the definition of mul, keeping the top-level namespace clean.


Board game in c++ using qt I  have been working on a board game in c++ using qt. The game cosist of four palyer with four different colors. I am looking forward to improving my board looking.

This is all working fine, but I want to know if there is any other way to do this?

"Readable online" The code for this part.

  std::vector<std::pair<char,char> > directions{std::make_pair(1,-1),std::make_pair(1,1),std::make_pair(-1,1),std::make_pair(-1,-1) };
    for(size_t d =0; d < directions.size(); ++d){
        for(int i=0; i<5;++i){
            if(d % 2 == 0)
                x_pos += directions[d].first * offset;
            else
                y_pos += directions[d].second * offset;
            fieldPos.push_back(QPointF(x_pos,y_pos));
        }
        x_pos += directions[d].first * small_offset;
        y_pos += directions[d].second * small_offset;
        for(int i=0; i<5;++i){
            fieldPos.push_back(QPointF(x_pos,y_pos));
            if(d % 2 == 0)
                y_pos += directions[d].second * offset;
            else
                x_pos += directions[d].first * offset;
        }
        for(int i=0; i<2;++i){
            fieldPos.push_back(QPointF(x_pos,y_pos));
            if(d % 2 == 0)
                x_pos += directions[d].first * large_offset;
            else
                y_pos += directions[d].second * large_offset;
        }
        fieldPos.push_back(QPointF(x_pos,y_pos));
    }

I also wonder how to get my board to look like the (rounded )one on the right. That would be very big help for me :)

As mentioned in my comment, it's not entirely clear what the purpose of this code is, beyond adding points to a vector. Given that, I see several things that I think could make this more clear.
Use Types
C++ is a strongly typed language and that not only gives you type safety, it also improves comprehension. The ultimate goal of this code appears to be to add several QPointfs to an array. But it's written with 3 different types to accomplish that job - whatever type x_pos and y_pos are (presumably float or double?), chars in the directions vector, and QPointf in the fieldPos vector. This is inefficient as the compiler will end up inserting a bunch of code to convert between types, but more importantly, it obscures what's happening.
I would declare directions like this:
std::vector<QPointf> directions {
    QPointf( 1.0, -1.0),
    QPointf( 1.0,  1.0),
    QPointf(-1.0,  1.0),
    QPointf(-1.0, -1.0)
};

I would change x_pos and y_pos into a single QPointf. It implements operator+=() so you can work in a similar manner as you are currently.
In general, when you find yourself using std::pair, it's a good sign that you need to use a better type like some sort of point, (mathematical) vector, or size struct. Code that uses std::pair is very difficult to read because the members, first and second are meaningless names for whatever task your code is performing.
Put the Right Things Into the Array
It might be more clear to just make a longer array with the directions you actually want instead of all the various loops. For example, instead of cramming everything into 4 QPointfs and looping over them and flipping back and forth between x and y positions, just make the array have what you want in it:
std::vector<QPointf> directions {
    QPointf( 1.0,  0.0),
    QPointf( 0.0, -1.0),
    QPointf( 1.0,  0.0),
    QPointf( 0.0, -1.0),
    // etc.
};

This will eliminate the need for multiple loops and the need to check even or odd iterations, which allows you to do the following.
Use Range-based Looping
Rather than writing a loop based on the size of the directions array and then having magic numbers like 5 in your inner loops, you can use the new C++11 and later syntax for range-based looping:
for (auto nextPoint : directions)
{
    pos += nextPoint * offset;
    fieldPos.push_back(pos);
}

But of course, you're not multiplying them all by offset. So your vector may need to contain objects of a new type in it that's something like:
typedef struct Direction {
    QPointf direction;
    float offset;
} Direction;

I'd use better names than Direction and direction, but since I don't know what they represent, I can't. If it's for a player, then PlayerDirection, or whatever's appropriate. So the loop would be more like:
for (auto nextDir : directions)
{
    pos += nextPoint.direction * nextPoint.offset;
    fieldPos.push_back(pos);
}

It means you'll end up having a long declaration for the array, but it will make the code a lot simpler. You could even put the values into a data file and read them at runtime if you think they'll change in the future.


Kattis challenge to finding the number of different elements in a set I have started doing Kattis challenges from their website. For now I am still at the low complexity ones.

The first line of input contains a single positive integer T≤50 indicating the number of test cases. The first line of each test case also contains a single positive integer n indicating the number of elements available for the test case.

Elements only contain lowercase letters, have at least one letter, and do not contain spaces.

The number elements for a test case is at most 100 and each element contains no more than 20 characters.

For each test case, I have to output a single line containing a single integer that is the number of distinct elements found in the specific test case.

To view the challenge follow this link: I've Been Everywhere, Man

Sample input:

2
7
saskatoon
toronto
winnipeg
toronto
vancouver
saskatoon
toronto
3
edmonton
edmonton
edmonton

Sample output:

4
1

Here is the code:

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

using std::cin;
using std::cout;
using std::vector;
using std::string;

int main()
{
    int testCasesCount = 0, tripsCount = 0;
    vector<string> trips;
    vector<int> testCasesResults;

    cin >> testCasesCount;

    for (int i = 0; i < testCasesCount; i++) {
        cin >> tripsCount;

        string destination;
        int uniqueCount = 0;
        for (int j = 0; j < tripsCount; j++) {
            cin >> destination;

            if (std::find(trips.begin(), trips.end(), destination) == trips.end())
                trips.push_back(destination);
        }

        testCasesResults.push_back(uniqueCount);
    }

    for (unsigned int i = 0; i < testCasesResults.size(); i++)
        cout << testCasesResults.at(i) << "\n";

    return 0;
}

The code is working fine, BUT it's kind of ugly, at least that's how I see it.

My first question is to ask if there is a better way to check if an array/vector contains a certain element?

And the second question - can someone give me some tips as to what I can improve and what will be a good idea to avoid?

As you yourself hinted in the question title, you should be using a set. Discarding duplicates using a set is easier and more efficient than using a vector.


Unicode parsing in PHP Firstly, apologies if this is not the correct type of question for here, I had it on the stackoverflow but it was closed with a suggestion I post here.

I’m in the process of converting from Latin 15 to Unicode/UTF-8 and researched several tutorials, and am looking here for a critique of what I have implemented based on them (or, IOW, did I understand it!) :

I want to parse form data in PHP to ensure it is safe from SQL injection and email header attacks, and any other security holes I've not considered.

Although I’m using UTF-8, I just want to cater for English plus some of the extra acute, tilde etc. characters that would normally encounter, plus the Euro symbol. Everything else is disallowed and throws an error, as opposed to silently being replaced/removed.

This is my code so far:

// ensure it's valid unicode / get rid of invalid UTF8 chars
$text = iconv("UTF-8","UTF-8//IGNORE",$text);

// and just allow a basic english...ish.. chars through - no controls, chinese etc
$match_list = "\x{09}\x{0a}\x{0d}\x{20}-\x{7e}"; // basic ascii chars plus CR,LF and TAB
$match_list .= "\x{a1}-\x{ff}"; // extended latin 1 chars excluding control chars
$match_list .= "\x{20ac}"; // euro symbol

if (preg_match("/[^$match_list]/u", $text) )
    $error_text_array[] = "<b>INVALID UNICODE characters</b>";

This code should only allow the characters shown in yellow here http://solomon.ie/unicode/

My main question is does this do as I want (it seems to work) or have I missed anything? My understanding of this is that the iconv function will remove any invalid sequences (i.e. hack attempts at the bit level) and leave just valid UTF-8, then my regexp checks for the characters I want allowed.

Although it seems to work I’m still confused by the regexp and the hex notation. Am I matching the Unicode code points, or, the actual binary UTF-8 representation of those code points? * It appears to be the former.

*my understanding is that a codepoint is basically the virtual location of a specific character in the Unicode “world” or characters, i.e. Euro symbol is the 20ACth character from the start, but it’s actual binary code, and number of bytes, is depended on if you use UTF-8, or 16 or 2 etc. So the codepoint never changes but the bit sequence can.

And, I have seen both, say, \x{20}-\x{7e} and \x20-\x7e used – what is the difference with and without the braces?

I intend to use the above few lines on all form fields, then follow it by further checks depending on the nature of the input.

For example if expecting an integer of the 0-9 kind, can I just use preg_match("/[^0-9]/", $text) without the /u modifier and specify literal characters (from x00 to x7e)? And suppose I want to allow 0-9 and the Euro in, is this the correct way preg_match("/[^0-9\x{20ac}]/u", $text)? And if I’m expecting a hidden field with ”ADD” or “EDIT” is if (!preg_match("/^(ADD|EDIT)$/", $text)) is still valid to test that?

Your code seems legit to me, so I'll try to answer some of your questions.
preg_match() uses UTF-8 input based on code points, so you don't have to worry about that.
The difference between \xYY and \x{YYYY} is that the first one accepts either one or two characters after control \x (so, up to 256-th code point), and the latter one is universal and accepts range which covers all Unicode table.
The u flag for RegExp pattern indicates that engine should treat the pattern as Unicode, it doesn't do anything with the text under test. So you can omit this flag in simple patterns like /[0-9]/. The last two code snippets also correct.


Palindrome evaluator in C++ I started learning C++ a few days ago, and this is my first full program. It takes in words until an end-of-file character prompt is given, and then outputs which of those words are palindromes and the longest palindrome of those words.

I want to know if my evaluation for whether each string is a palindrome is acceptable given the little knowledge I have about C++ thus far, and by acceptable I mean like not costly or just not agreeable for good C++ code. I was unsure if it would be better to access characters by position in each string with like a "double iterator" to check if they were the same while the iterators converged, or if just making a reversed string and checking is a better approach like I did.

Also, just if my general program structure looks okay - I'm rather unsure if my desire to have void functions to clean up my main function is really a good thing to do or otherwise.

#include <iostream>
#include <list>
#include <iomanip>

using std::cin;         using std::cout;        using std::string;
using std::istream;     using std::list;        using std::endl;
using std::setw;

istream& read(istream& is, list<string>& word_list)
{
    word_list.clear();
    string word;
    while(is >> word)
        word_list.push_back(word);

    is.clear();
    return is;
}

void evaluate(list<string>& words, string& longest)
{
    longest.clear();
    typedef string::size_type str_sz;

    list<string>::iterator it = words.begin();
    while (it != words.end())
    {
        if (*it == string(it->rbegin(), it->rend()))
        {
            longest = max(longest, *it);
            ++it;
        }
        else
        {
            it = words.erase(it);
        }
    }
}

void output(const list<string>& palindrs, const string& longest)
{
    if (palindrs.empty())
        cout << "No palindromes were given :(" << endl;
    else{
        cout << "The palindrome(s) given are: " << '\n' << endl;
        for (list<string>::const_iterator it = palindrs.begin();
            it != palindrs.end(); ++it)
            cout << setw(5) << "- \"" << *it << "\"" << endl;

        cout << '\n' << "And the longest palindrome is \"" <<
             longest << "\"!";
    }
}

int main()
{
    list<string> words;

    cout << "Please enter some words." << endl;
    read(cin, words);

    string longest;
    evaluate(words, longest);

    output(words, longest);

    return 0;
}

Your code looks so good, it is quite impressive of you were learning c++ few days ago. i'm sure you will get a decent code-review but here my humble attempt.

It always preferred to pass the std::string by const reference if you don't  modify it. also, std::vector is preferred over std::list. here link for more details

In your read function. It redirects all input into this container until EOF is received. However, the used container might need to reallocate memory too often, or you will end with a std::bad_alloc exception when your system gets out of memory. In order to solve these problems, you could reserve a fixed amount N of elements and process these amount of elements. here an alternative example to your read function by using std::copy_n and std::istream_iterator like so,

words.reserve(N);
std::copy_n(std::istream_iterator<std::string>(std::cin),
    N,
    std::back_inserter(words));

In your evaluate function, it is fine but not efficient doe to the call of std::string constructor in every iteration along with deleting un-palindrome elements. it is better to create another container for palindrome elements and check the main container elements if it has palindrome element. possible implementation:

std::vector<std::string> palindromes;
for (const auto& word : words)
{
    if(is_palindrome(word))
        palindromes.emplace_back(word);
}

The is_palindrome function, we test the given string for whither it is palindrome or not, like so:

bool is_palindrome(const std::string& str)
{
    if (str.empty())
        return false;

    using str_sz = std::string::size_type;
    for (str_sz i = 0, j = str.length() -1; i < j ; ++i, --j)
    {
        if (str[i] != str[j])
            return false;
    }
    return true;
}

thanks to @screwnut for suggesting to std::copy_if instead of for-range while the for-loop is bit clear and simple. but std::copy_if will allow us to use lambda instead of calling is_palindrome function, that might be helpful if you simply write it like this:

std::copy_if(words.begin(), words.end(),
             std::back_inserter(palindromes),
             [](const std::string& str)
             {return std::equal(str.begin(), str.begin() + str.size() / 2, str.rbegin());});

Lastly, for longest string, you may take advantage of STL by using std::max_element which is return iterator.

std::max_element(palindromes.begin(), palindromes.end(),
    [](const std::string& lhs, const std::string& rhs)
{
    return lhs.size() < rhs.size();
});

put all to gather:

#include <iostream>
#include <vector>
#include <algorithm>
#include <iterator>

int main()
{
    const std::size_t TotalWords = 3;
    std::vector<std::string> words;
    words.reserve(TotalWords);

    std::cout << "Please enter some words." << std::endl;
    std::istream_iterator<std::string> in(std::cin);
    std::copy_n(in, TotalWords, std::back_inserter(words));

    std::vector<std::string> palindromes;

    auto comp = [](const std::string& str)
    {
        return std::equal(str.begin(), str.begin() + str.size() / 2, str.rbegin());
    };

    std::copy_if(words.begin(), words.end(), std::back_inserter(palindromes), comp);

    if (palindromes.empty())
    {
        std::cout << "No palindromes were given" << std::endl;
    }
    else
    {
        auto longest = std::max_element(palindromes.begin(), palindromes.end(),
            [](const std::string& lhs, const std::string& rhs)
        {
            return lhs.size() < rhs.size();
        });
        std::ostream_iterator<std::string> out(std::cout, "\n ");
        std::cout << "Palindromes: \n ";
        std::copy(palindromes.begin(), palindromes.end(), out);
        std::cout << "\rLongest: " << *longest << std::endl;
    }
}