Custom timer code needs reviewing for thread safety I've created a custom timer to satisfy the following requirements:

Millisecond accuracy
I want the tick event handler to only be called once the current tick handler has completed (much like the winforms timer)
I want exceptions on the main UI thread not to be swallowed up by the thread timer so this requires Invoke/Send instead of BeginInvoke/Post

Note: I call timeBeginPeriod(1)/ timeEndPeriod(1) in order to achieve the millisecond accuracy.

I've put in a check if the timer is deleted to make sure that the deletion is complete before the timer can be created again. Does that look ok?

For the IsRunning property, is it possible that racing conditions could occur?

...or is the code a disaster waiting to happen and I should try (again) to get CreateTimerQueueTimer to work?

The code has been tested with a small interval of 1 millisecond and a large interval of 300 milliseconds with frequent start/stops, and I've had no problems so far.

ETA: I've found a problem with it. If the timer is running at an interval of 1 millisecond, and I call, say, Change(300), it locks up @ while (this.DeleteRequest). This
must be because the TimerLoop is in the this.CallbackDelegate.Invoke(null) call.

public class MyTimer : IDisposable
{

    private System.Threading.TimerCallback CallbackDelegate;
    private bool DeleteRequest;

    private System.Threading.Thread MainThread;
    public MyTimer(System.Threading.TimerCallback callBack)
    {
        this.CallbackDelegate = callBack;
    }

    public void Create(int interval)
    {
        while (this.DeleteRequest) {
            System.Threading.Thread.Sleep(0);
        }

        if (this.MainThread != null) {
            throw new Exception("");
        }

        this.MainThread = new System.Threading.Thread(TimerLoop);
        // Make sure the thread is automatically killed when the app is closed.
        this.MainThread.IsBackground = true;
        this.MainThread.Start(interval);

    }

    public void Change(int interval)
    {
        // A lock required here?
        if (!this.IsRunning()) {
            throw new Exception("");
        }
        this.Delete();
        this.Create(interval);
    }

    public void Delete()
    {
        this.DeleteRequest = true;
    }

    public bool IsRunning()
    {
        return (this.MainThread != null) && this.MainThread.IsAlive;
    }

    private void TimerLoop(object args)
    {
        int interval = (int)args;
        Stopwatch sw = new Stopwatch();
        sw.Start();

        do {
            if (this.DeleteRequest) {
                this.MainThread = null;
                this.DeleteRequest = false;
                return;
            }

            long t1 = sw.ElapsedMilliseconds;

            // I want to wait until the operation completes, so I use Invoke.
            this.CallbackDelegate.Invoke(null);

            if (this.DeleteRequest) {
                this.MainThread = null;
                this.DeleteRequest = false;
                return;
            }

            long t2 = sw.ElapsedMilliseconds;

            int temp = Convert.ToInt32(Math.Max(interval - (t2 - t1), 0));
            sw.Reset();
            if (temp > 0) {
                System.Threading.Thread.Sleep(temp);
            }

            sw.Start();
        } while (true);

    }

        // dispose calls this.Delete();

}

System.Threading.Thread.Sleep() has an accuracy (resolution) of around 15 milliseconds, and so I would stay far away from using that in your code if you need millisecond accuracy. Can you use Stopwatch instead? - this uses a high resolution timer where available, by default. You might find this post on StackOverflow here useful as well.


Bubble Sort: Time and Space Complexities Algorithm

Bubble sort, also known as sinking sort, is a sorting algorithm that repeatedly steps through a list, compares adjacent pairs and swaps them if they are not in the right order. The pass through the list is repeated, until the sorting is complete. The algorithm, which is a comparison sort, is named for the way smaller or larger elements "bubble" to the top of the list.

Solution 1

This is my BubbleSort function:

def BubbleSort(l):
    for i in range(len(l)-1):
        for j in range(len(l)-1-i):
            if (l[j]>l[j+1]):
                l[j],l[j+1]=l[j+1],l[j]
    return l

which outputs:

print(BubbleSort([1,5,-5,0,10,100]))
[-5, 0, 1, 5, 10, 100]

Solution 2

With some detailed advice, here is a second solution, limited to using one list comprehension:

def BubbleSort(l):
    [l.append(l.pop(0) if i == len(l) - 1 or l[0] < l[1] else l.pop(1)) for j in range(0, len(l)) for i in range(0, len(l))]

which somewhat outputs the same:

l = [1,5,-5,0,10,100]
BubbleSort(l)
print(l)
[-5, 0, 1, 5, 10, 100]

Would you be so kind and review it? I'd also like to know about time/space complexities, if you possibly had time.

BubbleSort is not an efficient algorithm. Talking time-complexity it runs in \$O(n^2)\$ which is okay given a very small array, but for a larger amount of numbers its almost unusable.

This is a comparison between a few sorting algorithms that i wrote in C#. As you can see the only advantage of BubbleSort over the other algorithms is that it is very easy to program.

Anyway - back to your code. I would your implementation is good although i find the first one way more readable.
All in all if you just wanted to implement BubbleSort you did a good job, but if you really want to sort a list, you should  definitely consider another algorithm that runs in \$O(log(n))\$.


Getting target IDs from promotions related to an order item I have a awful code with a big cyclomatic complexity. I have tried different variants, but haven't had good result. Please, help me to refactor this code:

public Set<String> getTargetIdsFromPromotion(String itemToRefundId, RepositoryItem orderItem) {
    Set<String> result = new HashSet<String>();
    List<RepositoryItem> relationships = getRelationships(orderItem);
    for (RepositoryItem relationship : relationships) {
        if (isValidShippingType(relationship)) {
            List<RepositoryItem> adjustments = getAdjustments(relationship);
            for (RepositoryItem adjustment : adjustments) {
                RepositoryItem promotion = getPromotion(adjustment);
                if (promotion != null){
                    Set<String> targetIds = getTargetIdsByQualifier(itemToRefundId, promotion);
                    if (!targetIds.isEmpty()) {
                        return targetIds;
                    }
                }
            }
        }
    }
    return new HashSet<String>();
}

Use the java 8 stream API:

public Set<String> getTargetIdsFromPromotion(String itemToRefundId, RepositoryItem orderItem) {
    return getRelationships(orderItem).stream()
            .filter(this::isValidShippingType)
            .map(this::getAdjustments)
            .flatMap(Collection::stream)
            .map(this::getPromotion)
            .filter(Objects::nonNull)
            .map(p -> getTargetIdsByQualifier(itemToRefundId, p))
            .filter(s -> !s.isEmpty())
            .findFirst()
            .orElseGet(Collections::emptySet);
}


Letter Combinations of a phone number using a Dictionary Problem statement  (online source)

  Given a digit string, return all possible letter combinations that the
  number could represent.

  A mapping of digit to letters (just like on the telephone buttons) is
  given below.

  Input: Digit string "23"

  Output: ["ad", "ae", "af", "bd", "be", "bf", "cd", "ce", "cf"].

  Note:

  Although the above answer is in lexicographical order, your answer
  could be in any order you want.

I have seen and read the reviews for the above problem, and made the modifications suggested. I did see the comment on using LINQ, I am learning how to use them and don't fully understand it yet. I successfully submitted my answer to LeetCode, but I would like a review to see what else I can do to improve the code. I noticed the code uses more memory than most submissions on LeetCode and I wonder if I can find out why here.

My code is based on a tutorial I watched. While it was in Java, I typed my code in C#. It recursively calls the method that looks into the objects of a Dictionary and concatenates the data inside them in the desired format. I am new to programming so I looked up stuff online and made some assumptions on equivalent data structures in C#. For example, the original code used a HashMap, and saw a recommendation online to used Dictionary<T, T> in C#.

public class Solution {
public IList<string> LetterCombinations(string digits) {
    List<String> result = new List<String>();
    if (String.IsNullOrEmpty(digits)){
        return result;
        throw new ArgumentNullException(nameof(digits));
    }
    Dictionary<Char, Char[]> lettersMap = new Dictionary<Char, char[]>();
        lettersMap.Add('1', null);
        lettersMap.Add('2', new[] { 'a', 'b', 'c' });
        lettersMap.Add('3', new[] { 'd', 'e', 'f' });
        lettersMap.Add('4', new[] { 'g', 'h', 'i' });
        lettersMap.Add('5', new[] { 'j', 'k', 'l' });
        lettersMap.Add('6', new[] { 'm', 'n', 'o' });
        lettersMap.Add('7', new[] { 'p', 'q', 'r', 's' });
        lettersMap.Add('8', new[] { 't', 'u', 'v' });
        lettersMap.Add('9', new[] { 'w', 'x', 'y', 'z' });
        lettersMap.Add('0', null);
        StringBuilder sb = new StringBuilder();
        int pos = 0;
        LetterCombinationsFunction(digits, sb, lettersMap, result, pos);
    return result;
}
private static List<String> LetterCombinationsFunction(String digits, StringBuilder sb,
                Dictionary<Char, Char[]> lettersMap, List<String> result, int pos)
    {
        if (sb.Length == digits.Count())
        {
            result.Add(sb.ToString());
            return result;
        }
        lettersMap.TryGetValue(digits[pos], out char[] values);
            foreach (var v in values)
            {
                sb.Append(v);
                LetterCombinationsFunction(digits, sb, lettersMap, result, pos+1);
                sb.Remove(sb.Length - 1, 1);
            }
        return result;
    }
}

Review

LetterCombinations returns an IList<string> but I see no reason to return a modifiable collection. Consider returning IEnumerable<string> instead.
A method's name should be a verb. Change LetterCombinations to GetLetterCombinations, FindLetterCombinations or PermutateLetterCombinations.
You did good by creating a private function that performs the recursion with intermediate parameters. This way, the user of this API only needs to think about the public endpoint. However, it could have used the same name as the public method. An overload would not resulted in a conflict. The postfix *Function is weird.
Prefer the use of var if the type can be read from code: var lettersMap = new Dictionary<char, char[]>();
You are looking to get familiar with LINQ, however the next statement did not require the LINQ wrapper Count() of a string's Length property: if (sb.Length == digits.Count()).
Use more consistent indentation for your class members.
Having the opening curly brace on the same line is ok, but it's more a convention in Java than in C#, where the opening curly brace is usually on the next line.
See Jesse C. Slicer's answer to move the static data out of the method.

Peculiarities

Did you plan to return the empty result list or throw an exception on this edge case?

if (String.IsNullOrEmpty(digits)){
    return result;
    throw new ArgumentNullException(nameof(digits));
}

The next line is followed by a loop over values but doesn't care whether the operation succeeded:

lettersMap.TryGetValue(digits[pos], out char[] values);

Did you plan to write this instead?

if (lettersMap.TryGetValue(digits[pos], out char[] values))
{
    // ..
}

You are using both char and Char in the next line. Is there a specific reason to use both these types?

Dictionary<Char, Char[]> lettersMap = new Dictionary<Char, char[]>();


Add checkbox with label to a form For a WordPress login form I have written a small plugin. It adds a checkbox with a unique name to the form and if that name is not present in the login POST request it just dies.

The idea is preventing scripted attacks made for regular WordPress login forms.

The PHP code is okay (imho), but I think the JavaScript code looks rather … inelegant. Everything works as expected, it is just not beautiful. What could I improve?

<?php  # -*- coding: utf-8 -*-
/**
 * Plugin Name: T5 Unique Log-in Field
 * Description: Adds a checkbox with a unique name to the login form to prevent scripted log-in attempts.
 * Plugin URI:
 * Version:     2013.04.14
 * Author:      Thomas Scholz
 * Author URI:  http://toscho.de
 * Licence:     MIT
 * License URI: http://opensource.org/licenses/MIT
 */

add_action( 'login_init',   't5_unique_login_field' );
add_action( 'login_footer', 't5_unique_login_field' );

/**
 * Add a checkbox with a unique name to the login form.
 *
 * @wp-hook login_init
 * @wp-hook login_footer
 * @return  void
 */
function t5_unique_login_field()
{
    if ( ! empty ( $GLOBALS['interim_login'] ) )
        return;

    if ( defined( 'LOGGED_IN_SALT' ) )
        $salt = constant( 'LOGGED_IN_SALT' );
    else
        $salt = filemtime( __FILE__ );

    $unique = md5( $_SERVER[ 'HTTP_HOST' ] . $salt );

    if ( 'POST' === $_SERVER[ 'REQUEST_METHOD' ] )
    {
        if ( ! isset ( $_POST[ 'log' ] ) )
            return;

        if ( empty ( $_POST[ $unique ] )
            or 'on' !== strtolower( $_POST[ $unique ] )
        )
            exit;

        return;
    }

    ?>
<script>
var unique    = '<?php echo $unique; ?>',
    checkBox  = document.createElement( 'input' ),
    paragraph = document.createElement( 'p' ),
    label     = document.createElement( 'label' ),
    labelText = document.createTextNode( ' I am human' );

checkBox.setAttribute( 'name', unique );
checkBox.setAttribute( 'type', 'checkbox' );
checkBox.setAttribute( 'id',   unique + '_id' );

label.setAttribute( 'for', unique + '_id' );
label.appendChild( checkBox );
label.appendChild( labelText );

paragraph.appendChild( label );
paragraph.setAttribute( 'style', 'clear:both;float:none' );
// get the same font style as the last checkbox
paragraph.setAttribute( 'class', 'forgetmenot' );

document.getElementById( 'loginform' ).appendChild( paragraph );
</script>
<?php
}

Screenshots:

Before plugin activation

After plugin activation

This is not meant to be a comprehensive answer:

If you do not want a bug that happens only in some browsers and only some of the time, use a safe, conservative format as an id

checkBox.setAttribute( 'id',   unique + '_id' );

If id is a random 64-bit integer or a GUID in the following form (b362e877-95c3-4e88-99dc-ceb4600639b5), then you would want id to be 'id_' + unique instead of unique + '_id'


Implementation of Vector in C++ This is a simple implementation of vector in C++. Please offer your reviews to improve the code.

#ifndef VECTOR_HPP
#define VECTOR_HPP

template <typename T> class vector{
    int size;//size allocated on the memory
    int index;//current number of elements
    T* arr;
    void resize(){
        T* temp = new T[size];
        for (int i = 0; i <index ; ++i) {
            temp[i] = arr[i];
        }
        delete[] arr;

        T* arr = new T[size*2];
        for (int i = 0; i < index ; ++i) {
           arr[i] = temp[i];
        }
        size = size*2;
        delete[] temp;

    }
public:
    vector(){
        size = 50;
        index = 0;
        T* arr = new T[size];
    }
    vector(int sz){
        size = sz;
        index = 0;
        T* arr = new T[size];

    }
    ~vector(){
        delete[] arr;
    }

    typedef T* iterator;

    iterator begin(){
        return arr;
    }

    iterator end(){
        return arr + index;
    }

    T* front(){
        return arr[0];
    }

    T* back(){
        return arr[index];
    }
    void push_back(const T& t){
        if((size - index) == 1){
            resize();

        }
        arr[index+1] = t;
        index++;

    }
    void pop_back(){
        index--;
    }

    void insert(iterator pos, const T& t){
        if((size -index) < 1) resize();
        int npos = pos - arr;
        for (int i = index; i > npos ; i--) {
            arr[i+1] = arr[i];
        }
        arr[npos] = t;
        index++;

    }

    void erase(iterator pos){
        int npos = arr - pos;
        if((size - index) < 1) resize();
        for (int i = npos; i <index-1 ; ++i) {
            arr[i] = arr[i+1];
        }
        index--;

    }

    T& operator[](int){

    }

    int length(){
        return index;
    }
    bool empty(){
        return index == 0;
    }

};

#endif //VECTOR_HPP

There are three severe errors in your code:

Rule of Three/Five/Zero

This is an invaluable resource. Anytime you write a user-defined destructor, you will need to write the copy constructor and assignment. You lack the copy constructor or assignment, so the compiler will happily provide one for you, and that one will not do the right thing:

vector<int> v;        // we have an array of size zero with capacity 50
vector<int> v2 = v;   // we do not have two arrays: both vectors have
                      // the SAME pointer
// <== v2 gets destroyed, we run delete [] array
// <== v gets destroyed, we run delete [] array again! boom

In addition to the double free, you get the other issue in that the user might expect a deep copy but actually v2 is basically just a Java-style reference to v. Fixing this is an absolute must.

Resizing

This is definitely non-functional. You delete your member arr, then, separately from that, you create a locally scoped array arr that you write stuff into. At the end of resize(), the vector object will have a deleted local array!

The correct way to resize() is to allocate a new array (only have to do one!) of double the size, to copy/move from your member into the new array, and then simply change your member to point to the new array. This should be one allocation (of the doubled array) and one deletion (of the old array).

Off-by-one

The first push_back() writes into arr[1]. What goes into arr[0]?

Once you fix those, there are other issues with your code:

Limits on Types

When you construct like this:

vector(){
    size = 50;
    index = 0;
    T* arr = new T[size];
}

You are adding a restriction on T. Namely, that it must be default constructible:

struct X { X(int ) { } };
vector<X> v;

leads to:

main.cpp: In instantiation of 'vector<T>::vector() [with T = X]':
main.cpp:108:15:   required from here
main.cpp:26:28: error: no matching function for call to 'X::X()'
         T* arr = new T[size];
                            ^

The way to avoid this is to use global ::operator new to allocate memory without constructing any objects.

Naming

You have member variables named size, which is actually the capacity of the vector, and index, which is its size. Those names are confusing and should be changed to reflect their actual purpose.

Object Destruction

A vector owns its objects. When you pop or erase from a vector, it must release ownership - which will involve explicitly calling the destructor on your type. You do not currently do this.

Resizing, part 2

Resizing should only happen on insertion. You also do it on erase(). This is illogical. Why would removing an element lead to doubling in the array size?

operator[]

Where's the body?

const overloads

If I have a const vector<int>, I cannot do anything with it. That's not useful. empty() and length() (which should be spelled size()) can just be const member functions. There also need to be const overloads for begin(), end(), operator[], front(), and back().


Create new columns using groupby in pandas I'm running into a large bottleneck in my program that takes hours to perform.

I have a Dataframe that is very large. I need to take the columns of the Dataframe and create new columns within same Dataframe. The new columns need to grouped by a specific date once grouped they are ranked. After they are ranked they are divided by the total number of values in that day (this number is stored in counts_date). This gives me a range of 0-1. The dataframe is a mulitindex with date as the level 0 and a unique id is level 1.

Is there a way to do this so I don't have to do it column by column and still create new columns?

df=pd.read_hdf('data.h5')

ranks2=list(df.columns.values)

for i in ranks2:
    df[i+'_rank']=df.groupby('date')[i].rank()

for i in ranks2:
    df[i+'_rank']=df[i+'_rank']/df['counts_date']

I don't know if I fully understand the end goal, it's difficult to tell without some sample data, however there are certainly better ways to make this call.

I noticed the manipulations over each column could be simplified to a Pandas apply, so that's what I went for.

suffixed = [i + '_rank' for i in df.columns]

g = df.groupby('date')

df[suffixed] = df[df.columns].apply(lambda column: g[column.name].rank() / df['counts_date'])

There could be a way to precompute the group ranks and then concatenate those columns straight to the original, but I didn't attempt that.

Apply will almost always be faster than a for loop.


Entry filter for list of directories I'm trying to filter a list composed of multiple dictionaries for useless entries and non-ideal fields. An entry is useless if Mem['visibility'] == "redacted" and then should be removed from the list. An entry is non-ideal when any of the fields are empty, and these should be filled with a 0 instead or if Mem['~whatever~'] is a list in which case the list should be transformed into a single string with all the objects in that list.

class Filter:
    @staticmethod
    def members(memberlist):
        for Mem in memberlist[:]:
            for Item in Mem:
                if not Mem[Item]:
                    Mem[Item] = 0
                if (type(Mem[Item]) is list):
                    Mem[Item] = ', '.join(Mem[Item])
            if(Mem['visibility'] == "redacted"):
                print(Mem)
                memberlist.remove(Mem)
        return(memberlist)

Sample memberlist:

[{'roles': [], 'rank': 'No SCB account', 'type': 'main', 'stars': 2, 'visibility': 'visible', 'sid': 'imperium', 'handle': 'freakyeagle'}, {'roles': [], 'rank': 'Fleet Member', 'type': 'main', 'stars': 1, 'visibility': 'visible', 'sid': 'imperium', 'handle': 'cadimus'}, {'roles': [], 'rank': 'Fleet Member', 'type': 'main', 'stars': 1, 'visibility': 'visible', 'sid': 'imperium', 'handle': 'belleal'}]

How can I make this as simple/efficient as possible?

You could use a list and generator comprehension instead of memberlist[:].
You should use snake_case instead of CamelCase for variables.
You should use isinstance rather than type() is.
This is as it works on objects that extend list. E.g. class MyCustomList(list): pass.
You should remove the print.
They are quite slow, and is probably left over from debugging.

If I were to do the above I would use:

def members(member_list):
    def handle_nonideal(item):
        if not item:
            return 0
        if isinstance(item, list):
            return ', '.join(item)
        return item

    return [
        {
            key: handle_nonideal(item)
            for key, item in member.items()
        }
        for member in member_list
        if member['visibility'] != 'redacted'
    ]


Dictionary or custom Collection I have a list of rule names coming from .xml file. These rule names needs to be validated against rule names in database (Validator.cs). If valid, then Id of these rule names are used to fetch user data from database (ExportService.cs).

I am using a Dictionary object to save name, Id pair from database -

Validator.cs

var rulesDictionary = new Dictionary<string, string>();
while (sqlDataReader.Read())
{
    string ruleName = sqlDataReader["NAME"].ToString();
    string ruleLink = sqlDataReader["LINK"].ToString();
    rulesDictionary.Add(ruleName, ruleLink);
}

The values coming from XML file are validated against this dictionary as -

ExportService.cs

if (DepositRulesDictionary.ContainsKey(xmlRuleName1) == false)
{
     //log error
}

If all the values are valid, then this dictionary is further used to pass 'ruleLink' values in ExportService.cs.

I used Dictionary object in this instance as it returns O(1) result for validating XML file values.

My XML file is not going to be very big, so efficiency is not a major concern right now.

My question is what is more recommended -

Dictionary<string, string> object or
List<T> where T is having two properties of Name and Link.

For scalability and unit-testability?

I think you are too concerned with the implementation right now.

The last part of the question

  ... recommended ... for scalability and unit-testability?

is the most important one.

Let's imaging you were to write the test for this feature. You would start off by designing the API that the user of the functionality would ultimately consume. Secondly as far as the tests go ... they could careless how it is implemented 'under the hood' it only cares that it meets the requirements performance just being on possible requirement. Many times asymptotic complexity doesn't even play a critical role unless n is large. Take for example Strassen matrix multiplication it is only practical for larger matrices.

For scalability I'm assuming you mean maintainability and extensibility. The best way to achieve that is to use the dependency inversion principle (DIP) so that you are not depending on either a list or a dict but instead on the correct abstraction.

One possible abstraction is that the rules represent a validation.

interface IValidator<T>
{
   bool IsValid(T item);
}

Your test would look as follows.

[Test]
void NoRulesEverythingIsFalse()
{
    var rules = new Rules();
    Assert.False(rules.IsValid(""));
}

[Test]
void SingleRule()
{
    var rules = new Rules(new[]
    {
        new LinkedRule("abc", "Only abc is allowed.")
    });
    Assert.True(rules.IsValid("abc"));
    Assert.AreEqual(
        rules.GetLinkForRule("abc"),
        "Only abc is allowed.");
    Assert.False(rules.IsValid("123"));
}

Now with this you can implement it however you want as long as your test pass. Feel free to add a performance test if it is required.

// LinkedRule.cs
struct LinkedRule {
    string Rule { get; set; }
    string Link { get; set; }
}

// Rules.cs
class Rules : IValidator<string>
{
    private Dictionary<string, LinkedRule> RulesToLinks { get; }

    public RuleValidator(IEnumerable<LinkedRule> linkedRules)
    {
        RulesToLinks = linkedRules.ToDictionary(
            x => x.Rule);
    }

    public bool IsValid(string rule) =>
        RulesToLinks.Keys.Contains(rule);

    public string GetLinkForRule(string rule) => RulesToLinks[rule];

    // ...
}


Making an RBG chart on Excel I was testing the RGB color mechanic on Excel, so I did the following:

sub paint()
red = 255
blue = 0
green = 0
Max = 48
For col = 1 To Max
    For Row = 1 To Max
        Cells(Row, col).Interior.Color = RGB(red - col * Int(256 / (Max * 2)) - Row * Int(256 / (Max * 2)), green + col * Int(256 / Max), blue + Row * Int(256 / Max))
    Next Row
Next col
end sub

This makes an RBG chart on Excel filling the interior color of every cell on a max x max square. It was just made as a test and I think it worked pretty well.

I am looking for improvement, if possible.

The expressions Int(256 / (Max * 2)) and Int(256 / Max) have constant values inside the loop. It would be good to save them in a local variable before the loop and reuse.

The line where you set the cell color is too long, I have to scroll horizontally to see what's on the right hand side of the line. You could improve that by putting the red, green, blue values in variables. I would also rename the current red, green, blue variables to redBase, greenBase, blueBase, or similar.

The naming of the loop variables Row and col are inconsistent in capitalization. It would be better if they were either both lowercase or both starting with a capital letter. Similarly, it would be better to consistently capitalize all keywords, like you did For and Next.


Prime number generator in C++ I am slowly learning C and C++, and decided after a few lessons to dive in myself without any help. I developed this prime number generator. It seems fast, but I'm wondering if I'm following the best practices for C++, or if I am missing anything important.

#include "stdafx.h"
#include "stdio.h"
bool checkPrime(int Number);

int _tmain(int argc, _TCHAR* argv[])
{
    int currentNum;
    currentNum = 2;
    do {
        if (checkPrime(currentNum)) {
            printf("%d ", currentNum);
        }
        currentNum++;
    } while (1 == 1);
    return 0;
}

bool checkPrime(int Number){
    for (int a = 2; a < Number; a++){
        if (Number % a == 0) {
            return false;
        }
    }
    return true;
}

A few things in addition to what @Jamal and @200_success already wrote:

g++ on Mac OS X won't compile this code, because of #include "stdafx.h" and the int _tmain(int argc, _TCHAR* argv[]) signature. It's good to keep your code portable, unless you have a special reason not to. g++ can compile if you drop the stdafx.h import and if change the main declaration.
... and since you're not using the arguments of main, you could just declare without any args: int main() { ... }.
As you already used true in the checkPrime function, why not use it in the infinite while loop in main, instead of 1 == 1.
This may be a matter of taste, but I think while (true) { ... } is generally more readable and intuitive than do { ... } while (true).
... actually, as @200_success pointed out, a for (int currentNum = 2; ; currentNum++) { ... } loop would be even better: this way currentNum is declared in the block where it is used, eliminating potential side effects, and the counter is a natural element in a for loop. Notice the empty terminating condition, making this an infinite loop.
In checkPrime you named the variable int Number, but the common convention is to not capitalize variable names, use simply int number instead.
As @leetnightshade pointed out, place the opening curly either always on the same line as the function name ("Egyption brackets"), or always on the next line.

Suggested implementation

#include <iostream>

bool isPrime(int number)
{
    for (int a = 2; a < number; a++) {
        if (number % a == 0) {
            return false;
        }
    }
    return true;
}

int main()
{
    for (int currentNum = 2; ; currentNum++) {
        if (isPrime(currentNum)) {
            std::cout << currentNum << " ";
        }
    }
}

This compiles with g++ without warnings and runs fine in Mac OS X and GNU/Linux. I would hope it works as expected in Windows too.


Print out a student's former name if it is not empty (!empty($student->former_name) ? print $student->former_name : '');

I only want to print out the former name if it is not empty, nothing else.

I imagine I do not need the else part of it, but I do not know how to do it like that without the else.

I want to shorthand this:

if(!empty($student->former_name))
 print $student->former_name;

Why not just

echo $student->former_name

If its empty, it will be an empty string and thus the same thing. There is no way to remove the else from a ternary statement. The only other way to write that would be like so:

echo empty( $student->former_name ) ? '' : $student_former_name;

Edit: Actually, I sort of lied above, you could remove the empty check altogether.

echo $student->former_name ? $student->former_name : '';

And of course, if your PHP version is >= 5.3, then you could use short ternary, assuming that your statement returns the same value that your if section should return.

echo $student->former_name ?: '';


Rails 5.2 callbacks to trigger score recalculation for relationships I have a model that is in a HABTM relationship with 2 other models:

Activity.rb

  has_and_belongs_to_many :mandates, -> { distinct }
  has_and_belongs_to_many :contacts, -> { distinct }

I had to add the following callbacks, since the models defined above have a 'score algorithm' that must be recomputed if activities are removed completely, or one is created initially:

  after_commit  :increase_contact_scores, :increase_mandate_scores
  after_destroy :decrease_contact_scores, :decrease_mandate_scores

The callbacks code seems horribly the same. Is there a way to simplify this by somehow passing the HABT relation class from the callback to a more 'generic' method that can replace these?

I also need to figure out how to ONLY trigger the relevant callback based on the type of the HABTM (ie. not call both increases and both decreases, since if a mandate is destroyed, calling the decrease_contacts_score method will fail).

  def increase_contact_scores
    contacts.each do |contact|
      if contact.activities.count == 1
        contact.calculate_score
        contact.save!
      end
    end
  end

  def increase_mandate_scores
    mandates.each do |mandate|
      if mandate.activities.count == 1
        mandate.calculate_score
        mandate.save!
      end
    end
  end

  def decrease_contact_scores
    contacts.each do |contact|
      if mandate.activities.count.zero?
        contact.calculate_score
        contact.save!
      end
    end
  end

  def decrease_mandate_scores
    mandates.each do |mandate|
      if mandate.activities.count.zero?
        mandate.calculate_score
        mandate.save!
      end
    end
  end

In testing this code it turned out there is a chicken-and-egg situation, and more specific requirements.

In order to know which object (Contact or Mandate) to update the score for, the Activity callbacks need more 'knowledge':

eg. 2 cases:

1 Contact A has 0 activities ->

if an activity is created, recalculate the score for A

2 Contact A has > 0 activities ->

if an activity is created, no need to recalculate the score

if an activity is deleted, recalculate the score only if activities = 0

The same is true for Mandate.

So, I ended up with a cleaner solution, with drier code:

Activity.rb

  before_commit :mark_objects_for_rescoring, on: :create
  before_destroy :mark_objects_for_rescoring
  after_commit :rescore_objects, on: [:create, :destroy]

  after_initialize do
    self.contacts_to_recalculate = []
  end

  private

  def mark_objects_for_rescoring
    contacts.each do |contact|
      contacts_to_recalculate << contact.id if contact.activities.count == 1
    end
    mandates.each do |mandate|
      mandates_to_recalculate << mandate.id if mandate.activities.count == 1
    end
  end

  def rescore_objects
    contacts_to_recalculate.each do |id|
      contact = Contact.find(id)
      contact.calculate_score
      contact.save!
    end
    contacts_to_recalculate.each do |id|
      mandate = Contact.find(id)
      mandate.calculate_score
      mandate.save!
    end
  end


PHP RSS/XML reader basic class I write this class to process and output xml/rss feeds. I'm still working on, but any suggestion will be appreciated.
As a little trick if the xml file come from an online resource, I've added a method to cache the xml file as a json file.

<?php
class NewsFeed{

    public $url;
    private $cachedFeed;
    private $cachedFile;
    private $feed;
    private $xml;
    private $json;

    public function __construct($url){
        $this->url = $url;
    }

    public function showLiveFeed(){
        $feed = $this->processFeed();
        return json_decode($feed, true);
    }

    public function showCachedFeed(){
        $cachedFeed = $this->loadCachedFile();
        return json_decode($cachedFeed, true);
    }

    private function processFeed(){
        $feed = $this->url;
        $xml = simplexml_load_file($feed, 'SimpleXMLElement', LIBXML_NOCDATA);
        $json = json_encode($xml);
        $cacheFeed = file_put_contents('cachedFeed.json', $json);
        return $json;
    }

    private function loadCachedFile(){
        if(file_exists('cachedFeed.json')){
          $cachedFile = file_get_contents('cachedFeed.json');
          return $cachedFile;
        }

    }
}
?>

A caching strategy is not well thought. There is no clear scenario, whether a cached or a non-cached version should be used. Besides, a cache must have a timeout after which it is considered staled and must be renewed.

There is also a filename issue. With only a filename, which makes a relative path, you will end up with having a cachedFeed.json file in the every directory. The filename must be unique and certain, which means it must include an absolute path to the file.

Another possible issue is a case when 2 feeds are used on the same site. What would happen to a cache file in this case? A filename must be unique. the simplest solution would be just md5() from the feed url.

Other minor issues include excessive naming. your class already have the word Feed in its name, no need to duplicate it in the method names. Also, there are a lot of class variables that a never actually used.

So the final code could be like

<?php
class NewsFeed
{
    protected $url;
    protected $cacheFilename;
    protected $cacheTimeout = 3600;

    public function __construct($url)
    {
        $this->url = $url;
        $this->cacheFilename = $this->buildCacheFilename();
    }

    public function get()
    {
        $json = $this->readCached();
        if(!$json) {
            $json =  $this->download();
        }
        return json_decode($json, true);
    }

    protected function download()
    {
        $xml = simplexml_load_file($this->url, 'SimpleXMLElement', LIBXML_NOCDATA);
        $json = json_encode($xml);
        file_put_contents($this->cacheFilename, $json);
        return $json;
    }

    protected function readCached()
    {
        if (!file_exists($this->cacheFilename)) {
            return false;
        }
        if (filectime($this->cacheFilename) < (time() - $this->cacheTimeout)) {
            return false;
        }
        return file_get_contents($this->filename);
    }

    protected function buildCacheFilename()
    {
        return $_SERVER['DOCUMENT_ROOT'] . "../cache/feed" . md5($this->url);
    }
}

So we have just a single method get() to read the feed contents, which is first checking for the cached version and then reads the feed online if the cache is not available. A timeout is implemented to make sure that the cached data doesn't get too old and outdated.


Repetitive For Loops in 2048 Game I have the following function defined in a program that implements the 2048 game. I tried condensing the repeating part of the code into one loop containing +1 offset variables for the indices, which are either set to 0 or 1 depending on the direction passed into the function, but the outer and inner loop variables swap depending on the direction. Could anyone give me some pointers as to how to condense this code and make it more clean, pythonic and terse?

def merge(direction):
    #if adjacent cells are equal, they are merged into one with twice the value and then moved.
    if direction == "up":
        for col in range(nColumns):
            for row in range(nRows-1):
                if grid[row][col] == grid[row+1][col]:
                    grid[row][col] = grid[row+1][col]*2
                    grid[row+1][col] = 0
                    move(direction)

    if direction == "down":
        for col in range(nColumns):
            for row in range(nRows-1):
                if grid[row][col] == grid[row+1][col]:
                    grid[row+1][col] = grid[row][col]*2
                    grid[row][col] = 0
                    move(direction)

    if direction == "left":
        for row in range(nRows):
            for col in range(nColumns-1):
                if grid[row][col] == grid[row][col+1]:
                    grid[row][col] = grid[row][col]*2
                    grid[row][col+1] = 0
                    move(direction)

    if direction == "right":
        for row in range(nRows):
            for col in range(nColumns-1):
                if grid[row][col] == grid[row][col+1]:
                    grid[row][col+1] = grid[row][col]*2
                    grid[row][col] = 0
                    move(direction)

Iterating more smartly can simplify your loops.

To start with, run this standalone example and see that the izip(…) expression produces useful pairs of coordinates:

from itertools import product, izip
nRows, nColumns = 3, 5
for dest, src in izip(product(range(nRows - 1), range(nColumns)),
                      product(range(1, nRows), range(nColumns))):
    print(dest, src)

Then, you can reuse the loop just by changing which ranges you pass in.

from itertools import product, izip

def merge(direction):
    def merge(dest_iterator, src_iterator):
        for (dr, dc), (sr, sc) in izip(dest_iterator, src_iterator):
            if grid[dr][dc] == grid[sr][sc]:
                 grid[dr][dc] == 2 * grid[sr][sc]
                 grid[sr][sc] = 0
                 move(direction)

    if direction == "up":
        merge(product(range(nRows - 1), range(nColumns)),
              product(range(1, nRows), range(nColumns)))
    elif direction == "down":
        merge(product(range(1, nRows), range(nColumns)),
              product(range(nRows - 1), range(nColumns)))
    elif direction == "left":
        merge(product(range(nRows), range(nColumns - 1)),
              product(range(nRows), range(1, nColumns)))
    elif direction == "down":
        merge(product(range(nRows), range(1, nColumns)),
              product(range(nRows), range(nColumns - 1)))


Reading bytes between two quotes from a stream and decoding it to its appropriate character set This is a component for a lexer which operates on a byte input stream that handles reading and decoding the contents of a string contained within double-quotes. The validity of the encoding is handled by CharsetDecoder#decode() which will throw an exception on an invalid buffer.

The charset argument defines the encoding of the string contents not the encoding of the quotation marks themselves (which is defined by the lexer). This allows the contents of the string to be defined as a character set other than the parent document. However, it must be a valid subset of Unicode such that its contents do not erroneously contain a byte value to match code point 34. Some form of readHeredoc() could provide an alternative for this specific use case.

String readString(
   Reader r,
   Charset charset)
   throws IOException
{
   ByteArrayOutputStream ostream = new ByteArrayOutputStream();

   int cp;

   while ((cp = r.read()) > 0) {
      if (cp == '"')
         break;

      ostream.write(cp);
   }

   return charset.newDecoder().decode(ByteBuffer.wrap(ostream.toByteArray()))
      .toString();
}

Reaching the end the buffer before a matching quotation is not handled; I haven't decided on the defined behavior for this and its absence is known.

the only advice, that I can give you, is to separate the logic in multiple methods. This will make your method shorter and allow the code to be reused.

Personally, I see two other methods.

    String readString(Reader r, Charset charset) throws IOException {
        ByteArrayOutputStream ostream = copyInputToStream(r);

        final ByteBuffer byteBuffer = ByteBuffer.wrap(ostream.toByteArray());

        return decodeBufferAsString(charset, byteBuffer);
    }

    private String decodeBufferAsString(Charset charset, ByteBuffer byteBuffer) throws CharacterCodingException {
        return charset.newDecoder().decode(byteBuffer).toString();
    }

    private ByteArrayOutputStream copyInputToStream(Reader reader) throws IOException {
        ByteArrayOutputStream ostream = new ByteArrayOutputStream();

        int cp;

        while ((cp = reader.read()) > 0) {

            if (cp == '"') {
                break;
            }

            ostream.write(cp);
        }

        return ostream;
    }


Swift replacement for C's for-loop I recently wrote an entry on my blog regarding unit testing using prime numbers as an example.

When I wrote the entry, I wrote my code keeping in minding that proposal SE-0007 has been accepted for Swift 3.0.  That is, C-style for loops will no longer be available in Swift as of 3.0, so I want to completely discontinue using them.

That is to say, where I might have written something like this:

for(int divisor = 7; divisor * divisor <= value; divisor += 30) {

I was instead stuck with some real ugliness in my prime loop.

func isPrime(value: Int) -> Bool {
    if value < 2 { return false }
    if value % 2 == 0 { return value == 2 }
    if value % 3 == 0 { return value == 3 }
    if value % 5 == 0 { return value == 5 }
    if value == 7 { return true }

    var divisor = 7
    while divisor * divisor <= value {
        if value % divisor == 0 { return false }
        if value % (divisor + 4) == 0 { return false }
        if value % (divisor + 6) == 0 { return false }
        if value % (divisor + 10) == 0 { return false }
        if value % (divisor + 12) == 0 { return false }
        if value % (divisor + 16) == 0 { return false }
        if value % (divisor + 22) == 0 { return false }
        if value % (divisor + 24) == 0 { return false }
        divisor += 30
    }

    return true
}

I am satisfied that this code works as I want.  I am also satisfied that is runs extraordinarily fast.

What I do not like about this is the very round-about way I have to achieve the same behavior a C-style for loop would have given me.  In particular, it is the termination point that concerns me.  I could easily get the same loop steps with the stride function:

for divisor in 7.stride(through: value, by: 30) {

But this doesn't allow for the same early stopping point:

divisor * divisior <= value

And making the first line of the loop check this isn't much better:

for divisor in 7.stride(through: value, by: 30) {
    if divisor * divisor > value { break }

And the biggest problem with the while loop which I'd like reviewed is that the divisor variable has a scope larger than I'd like, and the update statement doesn't come until the very end of the while loop (a problem that becomes more complex if your loop has any continue statements in it anywhere that allow iterations to break out early and jump to the next iteration).

So, in the end, I want a very, very fast isPrime() checker (it should be at least as fast as the above code), but I'd like the ugliness of my while loop to be significantly cleaned up.

Python also has no C-style for-loop.  A lot of Python novices write clumsy while-loops as a result.  However, once you learn the Pythonic idioms for looping, you'll wonder why other languages aren't more like Python.

The trick is to get to know…

the range() built-in function, which looks a lot like the stride(through: by:) that you suggested
the enumerate() built-in function, for when you need to iterate over a list and also increment a counter at the same time
the itertools library for more complex situations

If I had to translate your Swift code to Python, I would do it using itertools.count() to generate the infinite sequence [7, 37, 67, 97, …], and itertools.takewhile() to terminate it.

from itertools import count, takewhile

def is_prime(value):
    if value < 2: return False
    if value % 2 == 0: return value == 2
    if value % 3 == 0: return value == 3
    if value % 5 == 0: return value == 5
    if value == 7: return True

    for divisor in takewhile(lambda d: d * d <= value, count(7, 30)):
        if value % divisor == 0: return False
        for offset in [4, 6, 10, 12, 16, 22, 24]:
            if value % (divisor + offset) == 0: return False
    return True

Kevin Ballard has posted a proposal on the swift-evolution mailing list to introduce a takeWhile() to the standard library.  I think that a rich library for manipulating sequence types would make a good replacement for C-style for-loops.


Looping input function Currently, I have a function that loops to gather input for program execution, but I feel that it is a bit lacking, overthought and unoptimized. I would love any input on the process, coding style, or usability of the function.

I feel like the use of while (true) is not really optimized here.

My code uses curses and unistd.h.

// Heart of the display of the program
void startEvolutionLoop (Grid* grid, size_t speed) {
    clear();

    // Infinitely loops until 'q' is pressed
    while (true) {
        // First of all displays the grid with supporting information
        displayGrid(grid);
        printw("[Space] Evolve once        [Enter] Run evolutions        [q] Exit\n");

        // Gets the most recently touched character
        int c = getch();

        //  - if space (' ') evolves and loops again
        //  - if ('q') ends the loops
        //  - if enter ('\n') starts a sub-loop similar to the main one, but waits for any character to stop
        if (c == ' ') {
            grid->evolve();
        } else if (c == 'q') {
            return;
        } else if (c == '\n') {
            nodelay(stdscr, TRUE); // Getting of the input is asyncronous
            // Infinitely loops until any key is pressed
            while (true) {
                grid->evolve();
                displayGrid(grid);

                printw("Press any key to end execution...                       \n"); // Extra whitespace to override previously written stuff in this area

                // If there has been something inputted, then break out of this sub-loop
                if (getch() != ERR) {
                    break;
                }

                usleep((__useconds_t)speed * 1000); // Sleep the desired amount of time (in micro seconds converted to milliseconds)
            }
        }
    }
}

For context (which is not necessary), you can check out my GitHub Repo.

Consider following the Single Responsibility Principal and break the function up into smaller functions that do less.

Note: This answer makes use of the loopEvos() that is provided in the updated version of the source files on GitHub.

void loopEvos (Grid* grid, size_t speed) {
    nodelay(stdscr, TRUE); // Getting of the input is asyncronous
    // Infinitely loops until any key is pressed
    while (true) {
        grid->evolve();
        displayGrid(grid);

        printw("Press any key to end execution...\n");

        // If there has been something inputted, then break out of this sub-loop
        if (getch() != ERR) {
            break;
        }

        usleep((__useconds_t)speed * 1000); // Sleep the desired amount of time (in micro seconds converted to milliseconds)
    }
}

Whenever code is written future expansion of the program/function must be considered. In the function above there is the code :

        if (c == ' ') {
            grid->evolve();
        } else if (c == 'q') {
            return;
        } else if (c == '\n') {
            loopEvos(grid, speed);
            nodelay(stdscr, FALSE);
        }

It would be is easier to expand the code if another type of control statement was used instead. If the code was

        switch (c) {
            case ' ':
                grid->evolve();
                continue;
            case 'q':
                return;
            case '\n':
                loopEvos(grid, speed);
                nodelay(stdscr, FALSE);
                continue:
            default :
                printw("Invalid character input\n");
                return;
        }

It would be easier to handle any additional input characters such as
KEY_UP, KEY_LEFT, KEY_DOWN and KEY_UP.

        switch (c) {
            case ' ':
                grid->evolve();
                continue;
            case 'q':
                return;
            case '\n':
                loopEvos(grid, speed);
                nodelay(stdscr, FALSE);
                continue;
        // The following code is modified from procs/procs.hpp in the GitHub account.
            case 's':
                askFileSave(grid);
                continue;
            case 'c':
                changeSettings(&speed);
                continue;
            case KEY_UP:
                if (cursorPosY >= 1) {
                    cursorPosY -= 1;
                }
                continue;
            case KEY_LEFT:
                if (cursorPosX >= 2) {
                    cursorPosX -= 2;
                }
                continue;
            case KEY_DOWN:
                if (cursorPosY < (grid->y - 1)) {
                    cursorPosY += 1;
                }
                continue;
            case KEY_RIGHT:
                if ((cursorPosX / 2) < grid->x - 1) {
                    cursorPosX += 2;
                }
                continue;
            default :
                printw("Invalid character input\n");
                return;
        }

It might be easier to read, understand and modify the outer loop if it was a do
while loop rather than a while true loop

void startEvolutionLoop (Grid* grid, size_t speed) {
    clear();

     int userInput;
     do {
         // First of all displays the grid with supporting information
         displayGrid(grid);
         printw("[Space] Evolve once        [Enter] Run evolutions        [q] Exit\n");

         // Gets the most recently touched character
         userInput = getch();

         if (userInput == ' ') {
             grid->evolve();
         } else if (userInput == 'q') {
             continue;
         } else if (userInput == '\n') {
             loopEvos(grid, speed);  // from procs/procs.hpp in the GitHub account.
             nodelay(stdscr, FALSE);
         }
     } while (userInput != `q`);
}


Automatic creation of python conda packages I packaged a pure python library using conda.
It is tested to work with 2.7, 3.5 and 3.6 and should work on all three mayor operating systems.

I am quite surprised, that there is no command or option like conda-build-all. Instead I have to build and convert manually.

So there are two questions:

Is the idea to just put the tasks into a shell script sane? It feels wrong.

Is the shell script written in a maintaineable manner?

#!/bin/sh

# ====================================================
# Please note the bug, that for conda-build the option '--output' does
# not respect the directories given by '--output-folder':
# https://github.com/conda/conda-build/issues/1957
# ====================================================

tmp=$(dirname $(conda-build --output .))
system=$(basename $tmp)
root_dir=$(dirname $tmp)

for py_version in '2.7' '3.5' '3.6'
do
  package_name=$(basename $(conda-build --python ${py_version} --output .))
  package_path="${root_dir}/${py_version}/${system}/${package_name}"

  conda-build --no-anaconda-upload \
              --python ${py_version} \
              --output-folder "${root_dir}/${py_version}" .
  for platform in 'osx-64' 'linux-32' 'linux-64' 'win-32' 'win-64'
  do
    conda-convert -p ${platform} -o "${root_dir}/${py_version}" ${package_path}
    anaconda upload "${root_dir}/${py_version}/${platform}/${package_name}"
  done
done

PS: Here you can find the module and the shell script.

Quoting

It's recommended to quote variables in command arguments that might contain spaces, for example here:

tmp=$(dirname "$(conda-build --output .)")
system=$(basename "$tmp")
root_dir=$(dirname "$tmp")

The version numbers here don't contain characters that the shell would interpret, so you could drop all the single quotes. But you don't have to.

for py_version in '2.7' '3.5' '3.6'

Here again:

package_name=$(basename $(conda-build --python ${py_version} --output .))

It would be better to quote the command arguments:

package_name=$(basename "$(conda-build --python ${py_version} --output .)")

I wouldn't double-quote ${py_version} though,
because we have established earlier that it cannot possibly contain special characters.

This is fine:

package_path="${root_dir}/${py_version}/${system}/${package_name}"

But you could write simpler as:

package_path=$root_dir/$py_version/$system/$package_name

That is, the double-quoting is not necessary in variable assignments,
only in command line arguments.

This is fine:

for platform in 'osx-64' 'linux-32' 'linux-64' 'win-32' 'win-64'

But you could write more concisely as:

for platform in osx-64 linux-{32,64} win-{32,64}


Reverse Root Solution Solved one of the easiest problems from acm.timus.ru.

1001. Reverse Root
The input stream contains a set of integer numbers Aᵢ (0 ≤ Aᵢ ≤ 10¹⁸).
The numbers are separated by any number of spaces and line breaks.
A size of the input stream does not exceed 256 KB.
For each number Aᵢ from the last one till the first one you should
output its square root. Each square root should be printed in a
separate line with at least four digits after decimal point.

I would like to know why my solution is relatively slow (execution time is 1s) and uses so much memory (7 072 KB)? Compared to C++: 0.093s  and 408 KB of memory.

package main

import "fmt"
import "math"

var a []int64

func main() {
    for {
        var x int64
        k, _ := fmt.Scan(&x)
        if k != 1 {
            break
        }
        a = append(a,x)
    }
    n := len(a)

    for n > 0 {
        n--
        fmt.Printf("%.4f\n", math.Sqrt(float64(a[n])))
    }
}

Is the significantly larger resource consumption due to garbage collection and imported libraries or just my code is inefficient?

A benchmark is a scientific experiment.
An essential feature of a scientific experiment is that it be reproducible.
You do not provide any C++ code. You do provide some Go code. You do not define your methods for measuring CPU and memory. You provide some undefined CPU and memory measurements.
Your Go code looks inefficient.

1001. Reverse
Root
The input stream contains a set of integer numbers Aᵢ (0 ≤ Aᵢ ≤ 10¹⁸).
The numbers are separated by any number of spaces and line breaks. A
size of the input stream does not exceed 256 KB.
For each number Aᵢ from the last one till the first one you should
output its square root. Each square root should be printed in a
separate line with at least four digits after decimal point.

Compare my Go code (revroot.go) to your slower Go code (revroot.ivan.go):
$ ls -s -h revroot.in.data
256K revroot.in.data
$

$ go build revroot.go && time ./revroot < revroot.in.data > revroot.out.data
real    0m0.007s
user    0m0.007s
sys     0m0.000s
$ go build revroot.go && time ./revroot < revroot.in.data > revroot.out.data
real    0m0.008s
user    0m0.008s
sys     0m0.000s
$ ls -s -h revroot.out.data
208K revroot.out.data
$

$ go build revroot.ivan.go && time ./revroot.ivan < revroot.in.data > revroot.out.data
real    0m0.251s
user    0m0.141s
sys     0m0.113s
$ go build revroot.ivan.go && time ./revroot.ivan < revroot.in.data > revroot.out.data
real    0m0.245s
user    0m0.147s
sys     0m0.100s
$ ls -s -h revroot.out.data
208K revroot.out.data
$

revroot.go:
package main

import (
    "bufio"
    "fmt"
    "io"
    "math"
    "os"
    "strconv"
)

func RevRoot(in io.Reader, out io.Writer) error {
    var a []float64

    s := bufio.NewScanner(in)
    s.Split(bufio.ScanWords)
    for s.Scan() {
        i, err := strconv.ParseUint(s.Text(), 10, 64)
        if err != nil {
            return err
        }
        a = append(a, math.Sqrt(float64(i)))
    }
    if err := s.Err(); err != nil {
        return err
    }

    w, ok := out.(*bufio.Writer)
    if !ok {
        w = bufio.NewWriter(out)
    }
    defer w.Flush()
    buf := make([]byte, 0, 32)
    for n := len(a) - 1; n >= 0; n-- {
        buf = strconv.AppendFloat(buf[:0], a[n], 'f', 4, 64)
        buf = append(buf, '\n')
        _, err := w.Write(buf)
        if err != nil {
            return err
        }
    }
    err := w.Flush()
    if err != nil {
        return err
    }

    return nil
}

func main() {
    err := RevRoot(os.Stdin, os.Stdout)
    if err != nil {
        fmt.Fprintln(os.Stderr, err)
    }
    return
}

revroot.ivan.go:
package main

import "fmt"
import "math"

var a []int64

func main() {
    for {
        var x int64
        k, _ := fmt.Scan(&x)
        if k != 1 {
            break
        }
        a = append(a, x)
    }
    n := len(a)

    for n > 0 {
        n--
        fmt.Printf("%.4f\n", math.Sqrt(float64(a[n])))
    }
}

In Go, to measure performance we run benchmarks. We exclude I/O and other OS functions.
$ go test revroot_test.go revroot.go -run=! -bench=. -benchmem
BenchmarkRevRoot-4   249   4775861 ns/op   833577 B/op   13202 allocs/op
$

My RevRoot function takes 4,775,861 nanoseconds (0.004,775,861 seconds) for 13,177 square roots.
revroot_test.go:
package main

import (
    "bytes"
    "io/ioutil"
    "math/rand"
    "strconv"
    "strings"
    "testing"
)

func BenchmarkRevRoot(b *testing.B) {
    buf := bytes.NewBuffer(make([]byte, 0, 256*1024))
    r := rand.New(rand.NewSource(42))
    for {
        buf.WriteString(strconv.Itoa(r.Int()))
        buf.WriteString("\n")
        if buf.Len() > 256*1024-256 {
            break
        }
    }
    data := buf.String()

    b.ResetTimer()
    for N := 0; N < b.N; N++ {
        err := RevRoot(strings.NewReader(data), ioutil.Discard)
        if err != nil {
            b.Fatal(err)
        }
    }
}


Ocaml Functional Programming Tree I'm writing a function called unzip that turns:

[(1,2);(3,4);(5,6)]

to

([1;3;5],[2;4;6])

I've got a working implementation right here:

let unzip (data:(int*int) list) : int list * int list =
   match data with
   |(x,y)::tl->(x::traverse (tl,true),y::traverse (tl,false))
   |[]->([],[])
  let rec traverse tree =
    match tree with
    |((x,y)::tl,dir) -> if dir then x::traverse (tl, dir) else y::traverse (tl,dir)
    |([],_) -> [];;

However I'm not sure if this is the cleanest way to do this. As you can see, my traverse function accepts a boolean to know whether or not to go right or left, specifically because of:

(x::traverse (tl,true),y::traverse (tl,false))

Now is there even a way to do this so I only have to call one function, AND it knows where to put the data? or is this an alright way of doing it?

PS: I can't use non-core functions. (Modules, etc.)

You have the right idea, but you didn't quite know how to get there.  You don't need that traverse function to operate on the left or right lists.  Just bind them to separate variables through a simple let in expression and you can add the items to each of the lists.

let rec unzip = function
    | (x, y)::t -> let l, r = unzip t in (x::l, y::r)
    | []        -> ([], [])


Should I pass a list of objects or an object to a domain object method? I am developing a registration portal for a gym.  When a user registers; a list of sports are recommended based on their age; sex; fitness level etc.  Here is the tables in the database:

CREATE TABLE Person (ID int, name varchar(100), dateofbirth datetime, Gender char(1), primary key (ID))
CREATE TABLE Sport (ID int, description varchar(30), primary key (ID))
CREATE TABLE PersonCanPlaySport (Person ID int references Person(ID), SportID int references Sport(ID), PRIMARY KEY (SportID, PersonID))

The tables show what sports a person can participate in. Now see the class below:

public class Person
{
   public List<Sport> Sports;

   public Person()
   {
     Sports = new List<Sport>();
   }

   public void AddSports(List<Sports> allSports)
   {
      Sports.Clear();
      ((List<Sport>)Sports).AddRange(allSports
      .Where(sport => sport.CanPlay(this)));
   }

}

This works very well.  However, in this question: https://softwareengineering.stackexchange.com/questions/361040/can-two-objects-have-multiple-types-of-relationships (under question comments); a user critics the design but does not really explain why.  What is "wrong" with the design? Should I be passing single Sport objects to Person.AddSports similar to how James does it in his answer here: https://stackoverflow.com/questions/1103693/how-to-model-a-many-to-many-relationship-in-code/1103862?noredirect=1#comment81696466_1103862?

I will simply go from top to bottom without ordering by priority.

Person is not supposed to be a base class (at least now) then it should be marked as sealed.

Sports is a public field. There is not any level of protection. Caller might do anything with it: set it to null, clear the content. Add/remove items in an uncontrolled way (is, for example, null a valid item?) The very first thing to do is to make it a read-only property:

public List<Sport> Sports { get; }

You can assign an instance in your ctor (as you're doing now) or directly where it's declared:

public List<Sport> Sports { get; } = new List<Sport>();

The second problem is the type. You're exposing List<T> which is, at best, an implementation detail. Which operations the caller is allowed to do? Add/remove items? Clear the collection? You may change its type to IEnumerable<Sport> like this:

private readonly List<Sport> _sports = new List<Sport>();
public IEnumerable<Sport> Sports => _sports;

However we're just postponing the problem to the future. I'd introduce a SportCollection class which exposes exactly what you need (and to begin with it might be as little as IEnumerable<Sport>).

public SportCollection Sports { get; } = new SportCollection();

We fixed the declaration, we fixed the type but we still need to fix the name. It does not communicate almost anything. It's the list of sports that this person may play but its name is saying nothing: you have to read the code to understand how it's populated.

public SportCollection LikeableSports { get; } = new SportCollection();

I'm sure someone else can suggest a much better name but I hope you understood the point.

AddSports() method again does not say what it's actually doing. It's not adding generic sports but only sports this person may play/like. Change the name to reflect its behavior.

allSports parameters is List<Sport> but you if you change it to IEnumerable<Sport> you code compiles. Do it, always pick the less restrictive requirement for your parameters (what if the list comes from a DB? Do you want to materialize the whole thing in memory because AddSports() wants a list?)

You're clearing the list before adding the new items. It's not what I expect from a method named Add*(). Change the logic or change the name to reflect it.

You do not need to cast Sports to List<Sport>, just drop it.

Again a word about naming: I read sport.CanPlay(this) as "Can Sport play Person?" and it sounds weird. If this responsibility is in the Sport class then it should be at least named CanBePlayedBy().

Sport has the responsibility to check if it can be played/liked by Person. It's debatable if this responsibility should be in Sport, in Person or much better (IMO) in a separate class. I'd go with a separate class because:

I can switch algorithm in any moment, even from configuration and at run-time without changing anything else.
It makes testing much easier.
It keeps responsibilities separate and clear.
If you add more rules to determine a match (proximity of the structure to play that sport and public transportation to/from that place, for example) you will have a single clear place where these knowledge has to be managed (unless you want to pollute your Sport class with references to StructureCollection and PublicTransportationTripPlanner, of course...)

Do not ignore this suggestion, it's the last one but I think this is more important than most of the other considerations in my post: you can live with a non sealed class but putting responsibilities in the wrong place will make you code a nightmare to maintain. Micro design is easy to change (with better knowledge of the domain and of the language) but wrong architectural decisions are much much MUCH more expensive to deal with (on the long term).


Passing object back to Windows Form button event I have windows form application. One button event starts the script via object called ScriptStarter. The status of object is used to pass message and color to UI (toolStripStatusLabel1). I'm not sure if this is good practice. Could you please give me advice or suggestion how to better set up the communication between object and UI? I have heard little about Action(). I don't know if should I consider using of this. Any suggestions are warmly welcomed.

    private void btnStartScript_Click(object sender, EventArgs e)
    {
        ScriptStarter scriptStarter = new ScriptStarter();
        scriptStarter.StartScript();
        toolStripStatusLabel1.Text = scriptStarter.Status.Message;
        toolStripStatusLabel1.ForeColor = scriptStarter.Status.FontColor;
    }

The class ScripStarter.cs is here (I've replaced some methods body with comment in this post):

class ScriptStarter
    {
        public StatusKeeper Status = new StatusKeeper();

        public void StartScript()
        {
            string scriptFilePath = Properties.Settings.Default.MyStartCacheScriptPath;
            if (scriptFilePath.Length == 0 || !File.Exists(scriptFilePath))
                scriptFilePath = setNewScriptFile();

            if (File.Exists(scriptFilePath))
            {
                tryToRunScript(scriptFilePath);
            }
            else
            {
                Status.Message = "Script not found";
                Status.FontColor = Color.Crimson;
            }
        }

        private void tryToRunScript(string scriptFilePath)
        {
            bool succeeded = runBatScript(scriptFilePath);
            if (succeeded)
            {
                Status.Message = "Script started";
                Status.FontColor = Color.ForestGreen;
            }
            else
            {
                Status.Message = "Script not started";
                Status.FontColor = Color.Crimson;
            }
        }

        private bool runBatScript(string scriptFilePath)
        {
            // returns true if run script was successful
        }

        private string setNewScriptFile()
        {
            // returns script path
        }
    }

The class StatusKeeper.cs

public class StatusKeeper
{
    public string Message { get; set; }
    public Color FontColor { get; set; }
}

public StatusKeeper Status = new StatusKeeper();

It's a very bad idea to return the result via a public field.

If anything bad happens you should throw an exception. By doing this you automatically will need to move the stylings to somewhere else but this is what we want - separate different responsibilities. Your class is doing too much right now. The ScriptStarter shouldn't know anything about the colors etc. Its only responsibility should be to start scripts. It could be much simpler like this:

class ScriptStarter
{
    public void RunScript()
    {
        var scriptFilePath = Properties.Settings.Default.MyStartCacheScriptPath;
        if (string.IsNullOrEmpty(scriptFilePath) || !File.Exists(scriptFilePath))
        {
            scriptFilePath = setNewScriptFile();
        }

        if (!File.Exists(scriptFilePath))
        {
            throw new FileNotFoundException("Script not found.", scriptFilePath);
        }

        runBatScript(scriptFilePath);
    }

    private void runBatScript(string scriptFilePath)
    {
        // do something and throw if it didn't work
        throw new ScriptStartException(); // create a new type of exception for this
    }

    private string setNewScriptFile()
    {
        // returns script path
    }
}

How do we run this now?

We write a new method that will use the ScriptStarter and handle the results:

private void RunScript()
{
    try
    {
        ScriptStarter.RunScript();
        UpdateScriptStatusLabel("Script started", Color.ForestGreen);
    }
    catch (FileNotFoundException)
    {
        UpdateScriptStatusLabel("Script not found.", Color.Crimson);
    }
    catch (ScriptStartException)
    {
        UpdateScriptStatusLabel("Script not started.", Color.Crimson);
    }
    catch (Exception ex)
    {
        UpdateScriptStatusLabel($"Something else happened: {ex.Message}", Color.Red);
    }
}

We also need another one that will take care of the status label:

private void UpdateScriptStatusLabel(string text, Color color)
{
    toolStripStatusLabel1.Text = text;
    toolStripStatusLabel1.ForeColor = color;
}

Now that we have separated all concerns we can finally update the event handler:

private void btnStartScript_Click(object sender, EventArgs e)
{
    RunScript();
}

What did we win? We won maintainability and reusability. If you decide to add a keyboard shortcut in a future version then you just call the RunScript() from there and that's it. No repetition of the formatting or the logic how to run the script.


Secure file system utility functions For Khronos, I've had to develop these utility functions to help me deal with storing the .wav files.  However, they could also be used in a variety of applications.

A description of what the three functions do are given in the header documentation comments.

util.h:

#ifndef UTIL_H
#define UTIL_H

/**
 * @return the temporary file directory for your system
 */
const char* getTmpDir(void);

/**
 * Generates a unique temporary filename given the fileroot, creates and opens the file
 * @return file descriptor to the open file
 */
int createSafeFileDescriptor(const char* fileRoot);

/**
 * Fetches the file path given the file descriptor
 * @return file path
 */
const char* getPathFromDescriptor(int fd);

#endif // UTIL_H

util.c:

#include <stdlib.h>
#include <stdio.h>
#include <stdint.h>

#ifndef _WIN32
#include <fcntl.h>
#include <unistd.h>
#endif

#include "util.h"

const char* getTmpDir(void)
{
    char *tmpdir = NULL;
    if ((tmpdir = getenv("TEMP"))) return tmpdir;
    else if ((tmpdir = getenv("TMP"))) return tmpdir;
    else if ((tmpdir = getenv("TMPDIR"))) return tmpdir;
    else return "/tmp/";
}

int createSafeFileDescriptor(const char* fileRoot)
{
    // Creates temporary file safely
    char flacFile[FILENAME_MAX] = "";
    snprintf(flacFile, FILENAME_MAX, "%sXXXXXX.wav", fileRoot);

    // the 5 is for the length of the suffix ".wav"
    return mkstemps(flacFile, 4);
}

const char* getPathFromDescriptor(int fd)
{
    char *filename = malloc(FILENAME_MAX);
#ifdef _WIN32
    intptr_t file = _get_osfhandle(fd);
    intptr_t fileMap = CreateFileMapping(file, NULL, PAGE_READONLY, 0, 1, NULL);

    if (hFileMap)
    {
        void* pMem = MapViewOfFile(hFileMap, FILE_MAP_READ, 0, 0, 1);
        if (pMem)
        {
            if (GetMappedFileName(GetCurrentProcess(), pMem, filename, FILENAME_MAX)) return filename;
        }
    }
    return NULL;
#endif

#ifdef __APPLE__
    if (fcntl(fd, F_GETPATH, filename) != -1) return filename;
    return NULL;
#endif

#ifdef __unix__
    char proclnk[PATH_MAX] = {0};
    ssize_t r = -1;
    snprintf(proclnk, sizeof(proclnk), "/proc/self/fd/%d", fd);
    if ((r = readlink(proclnk, filename, FILENAME_MAX)) < 0) return NULL;
    filename[r] = '\0';
    return filename;
#endif
}

Good set of code.

Observations

#ifdef structure lacks uniqueness and detection if none are true.  Suggest #elif

#ifdef _WIN32
#elif __APPLE__
#elif __unix__
#else
#error Error Message
#endif

Rather than a differing #if structure in the .c file, suggest a matching one as in #1

//#ifndef _WIN32
//#endif
#ifdef _WIN32
#elif __APPLE__
...

Memory leak.  Should free(filename) before returning NULL.

if ((r = readlink(proclnk, filename, FILENAME_MAX)) < 0) return NULL;

Name-space.  Prefer some naming convention that ties these 3 functions to
their .h file.  Maybe wavutil?

wavutil.h
wavutil_getTmpDir();
wavutil_createSafeFileDescriptor()
wavutil_getPathFromDescriptor();

Since function allocates data, make that clear in the function declaration.  Also document candidate error returns

* ALLOCATES the file path given the file descriptor
* Returns NULL on ...
...
const char* getPathFromDescriptor(int fd);

Suggest error return of NULL instead.  Hoping "/tmp/" exists after failing 3 environment variables it a questionable fix.  At least "./" is sure to exist.  Also the environment variable might not end with /.  May need to check that.

// else return "/tmp/";
else return NULL;
// or
else return "./";

snprintf() is good to prevent overruns, yet if it fails, do not proceed with a runt string.

int len = snprintf(proclnk, sizeof(proclnk), "/proc/self/fd/%d", fd);
if (len < 0 || len >= sizeof(proclnk)) Handle_Error();
if ((r = readlink(proclnk, filename, FILENAME_MAX)) < 0) return NULL;

Minor

Comment inconsistent with code 5 vs. 4?  Could avoid magic number 4.

   // // the 5 is for the length of the suffix ".wav"
   // return mkstemps(flacFile, 4);

   char ext[] = ".wav";
   snprintf(flacFile, FILENAME_MAX, "%sXXXXXX%s", fileRoot, ext);
   return mkstemps(flacFile, sizeof ext - 1);

Prefer right-sizing allocations that start off large.

   char *filename = malloc(FILENAME_MAX);
   ...
   char *p = realloc(filename, strlen(filename) + 1);
   return p ? p : filename;

Curious style change.  Why sizeof() in 2nd line, yet FILENAME_MAX instead of sizeof(flacFile).  Suggest the latter.  Further, not a fan of xxx_MAX when xxx_SIZE is meant.  MAX to me means the maximum number so characters - which is 1 less than size.

char proclnk[PATH_MAX] = {0};
snprintf(proclnk, sizeof(proclnk), "/proc/self/fd/%d", fd);

char flacFile[FILENAME_MAX] = "";
snprintf(flacFile, FILENAME_MAX, "%sXXXXXX.wav", fileRoot);


Grab set of data from database that matches userid and return it to the view I want to get the row of data that matches the user ID I have. Then I want to return that view to my view to be displayed in my textboxes for editing.

Here's the action that gets called when a get request to sent to the controller:

public ActionResult EditUserAccount()
{
            UserAccountViewModel editUserAccountViewModel = new UserAccountViewModel();
            db_user user = new db_user();
            editUserAccountViewModel.UserName = UserSession.GetValue(StateNameEnum.UserName, StateNameEnum.UserName.ToString()) as string;
            var Title = context.db_user.Where(x => x.UserId == editUserAccountViewModel.UserId).Select(x => x.Title).ToString();
            var FirstName = context.db_user.Where(x => x.UserId == editUserAccountViewModel.UserId).Select(x => x.FirstName).ToString();
            var LastName = context.db_user.Where(x => x.UserId == editUserAccountViewModel.UserId).Select(x => x.LastName).ToString();
            var PhoneNumber = context.db_user.Where(x => x.UserId == editUserAccountViewModel.UserId).Select(x => x.PhoneNumber).ToString();
            var AltPhoneNumber = context.db_user.Where(x => x.UserId == editUserAccountViewModel.UserId).Select(x => x.AltPhoneNumber).ToString();
            var EmailAddress = context.db_user.Where(x => x.UserId == editUserAccountViewModel.UserId).Select(x => x.EmailAddress).ToString();

            editUserAccountViewModel.Title = Title;
            editUserAccountViewModel.FirstName = FirstName;
            editUserAccountViewModel.LastName = LastName;
            editUserAccountViewModel.PhoneNumber = PhoneNumber;
            editUserAccountViewModel.AltPhoneNumber = AltPhoneNumber;
            editUserAccountViewModel.EmailAddress = EmailAddress;
            return (PartialView("~/Views/Account/UserAccount.cshtml", editUserAccountViewModel));

}

The problem is the repetitious calls to get specific fields from the database so that I can set my viewmodel properties to them. Is there a better way to do this?

Use an anonymous type to store a single object retrieved from the database and then populate your view from this temporary (and anonymous) object.

Answering from my mobile, so I apologize if this doesn't compile.

public ActionResult EditUserAccount()
{
            UserAccountViewModel editUserAccountViewModel = new UserAccountViewModel();
            db_user user = new db_user();
            editUserAccountViewModel.UserName = UserSession.GetValue(StateNameEnum.UserName, StateNameEnum.UserName.ToString()) as string;

            var user = context.db_user.Where(x => x.UserId == editUserAccountViewModel.UserId)
                .Select(x => new {x.Title, x.FirstName, x.LastName, x.PhoneNumber, x.AltPhoneNumber, x.EmailAddress } );

            editUserAccountViewModel.Title = user.Title;
            editUserAccountViewModel.FirstName = user.FirstName;
            editUserAccountViewModel.LastName = user.LastName;
            editUserAccountViewModel.PhoneNumber = user.PhoneNumber;
            editUserAccountViewModel.AltPhoneNumber = user.AltPhoneNumber;
            editUserAccountViewModel.EmailAddress = user.EmailAddress;
            return (PartialView("~/Views/Account/UserAccount.cshtml", editUserAccountViewModel));

}

This lets you hit the database once instead of once per property.


2D lattice random walk plots in functional style To practice writing code in the functional programming style, I wrote a program to plot two-dimensional lattice random walks. I'd appreciate any feedback about how to improve its "functionaliness".

possibleSteps <- list(c(-1,0), c(0,-1), c(1,0), c(0,1))

step <- function(x) {
        return(unlist(sample(possibleSteps, 1)))
}

takeRandomWalk <- function(nSteps) {
        coordPairs <- Reduce(`+`, lapply(1:nSteps, step), accumulate = T)
        x <- sapply(coordPairs, `[`, 1)
        y <- sapply(coordPairs, `[`, 2)
        return(list(x, y))
}

plotRandomWalk <- function(nSteps, margins) {
        walkObj <- takeRandomWalk(nSteps)
        plot(seq(-margins,margins), seq(-margins,margins),
             type = 'n', xlab = "", ylab = "")
        lines(walkObj[[1]], walkObj[[2]])
}

Call plotRandomWalk(10000, 80) for an example.

EDIT

I have now compiled a far schnazier version as a shiny app:

Check it out!

Thanks for the help!

Looks good.  The code is functional with the exception of using
return, which is not necessary as the last value will be returned
anyway, so I'd simplify to the following and also in the interested of
efficiency only compute the coords once and without seq, as : is
shorter:

step <- function(x) {
        unlist(sample(possibleSteps, 1))
}

takeRandomWalk <- function(nSteps) {
        coordPairs <- Reduce(`+`, lapply(1:nSteps, step), accumulate = T)
        x <- sapply(coordPairs, `[`, 1)
        y <- sapply(coordPairs, `[`, 2)
        list(x, y)
}

plotRandomWalk <- function(nSteps, margins) {
        walkObj <- takeRandomWalk(nSteps)
        coords <- -margins:margins
        plot(coords, coords, type = 'n', xlab = "", ylab = "")
        lines(walkObj[[1]], walkObj[[2]])
}

Reduce with accumulate is pretty cool, I think this is the first
time I've seen an accumulate parameter on a reduce-like function
anywhere.

Now I think you know that, but usually R code like this would be written
with less lists and more data frames or matrixes.  That doesn't mean the
code isn't functional:  As long as it's using functions, not mutating
things and using functional abstractions instead of imperative
constructs it's fine.

So the following is probably more idiomatic and at the same time uses
more efficient representations for the data:

possibleSteps <- matrix(c(-1, 0, 0, -1, 1, 0, 0, 1), ncol = 2, byrow = TRUE,
                        dimnames = list(NULL, c("X", "Y")))

Gives a matrix with named columns like so, which will be the representation
throughout the rest of the code (which is good for consistency):

> possibleSteps
      X  Y
[1,] -1  0
[2,]  0 -1
[3,]  1  0
[4,]  0  1

The random walk will still be created by sample, except it's sampling
indexes.  cbind will create a new matrix from the two accumulated sums
(cumsum):

takeRandomWalk <- function(nSteps) {
        indexes <- sample(1:dim(possibleSteps)[1], nSteps, TRUE)
        walk <- possibleSteps[indexes,]
        cbind(X = cumsum(walk[,1]), Y = cumsum(walk[,2]))
}

And finally plotRandomWalk is a bit simpler as we can just give
lines the constructed coordinates matrix:

plotRandomWalk <- function(nSteps, margins) {
        coords <- -margins:margins
        plot(coords, coords, type = 'n', xlab = "", ylab = "")
        lines(takeRandomWalk(nSteps))
}


Combine similar methods with different text? I made the popular Guess-The-Number game and included an AI version that picks random numbers. The AI version has slightly different Console output that differ slightly, for example:

I generated a number between 1 and 100. Can you guess it?
You generated a number between 1 and 100. Let me try

First I did this:

Console.WriteLine("{0} generated a number between 1 and 100. {1}", ai? "I" : "You", ai? "Can you guess it?", "Let me try");

As this needed to be done a few more times, it looked very clumsy, but could implement both versions in one medium length method. After that I seperated the different versions and made two shorter methods GuessUser and GuessAI which are cleaner, but both share a lot of similar code, almost everything besides Console.WriteLine(x).

Is there a smart way to combine those two versions into a small method?

Complete version

I can't say that it's going to be more efficient, or better designed, but one option is to take advantage of polymorphism by creating a set of classes for this.

public abstract class GuessBase
{
   protected abstract string GuessMessage { get; }
   protected abstract string WrongMessage { get; }
   protected abstract string RightMessage { get; }

   protected abstract int GetGuess();

   public void DoGuessing()
   {
        Console.WriteLine(GuessMessage);
        while (guess != number)
        {
            if (GetGuess() != number)
                Console.WriteLine(WrongMessage, guess);
            tries++;
        }
        Console.WriteLine(RightMessage, number, tries);
        Console.ReadLine();
    }
}
public class AIGuess : GuessBase
{
   protected override string GuessMessage
   {
      get
      {
         return "You generated a number between 1 and 100. Let me try";
      }
   }
   // Similar for WrongMessage and RightMessage

   protected override int GetGuess()
   {
      return Pick(number, ref guess);
   }
   // Put Pick() method here.
}
public class UserGuess : GuessBase
{
   protected override string GuessMessage
   {
      get
      {
         return "I generated a number between 1 and 100. Can you guess it?";
      }
   }
   // Similar for WrongMessage and RightMessage

   protected override int GetGuess()
   {
      int guess;
      int.TryParse(Console.ReadLine(), out guess); // This won't do what you want if the user doesn't provide a valid number.
      return guess;
   }
}

Then, in your main code, do this:

GuessBase guess;
if (/* AI */) guess = new AIGuess();
else guess = new UserGuess();
guess.DoGuessing();


Counting the number of continuous palindromic substrings Given a string of lowercase ASCII characters, find all distinct continuous palindromic sub-strings of it.
Here is my code:

def palindrome(s):
    if s==s[::-1]:
        return True

noTest=int(input())
s=[]
for _ in range(noTest):
    s.append(input())

count=[]
for word in s:
    letters=[]
    psub=[]
    for l in word:
        letters.append(l)
        if l not in psub:
            psub.append(l)

    for i in range(len(letters)):
        for k in range(i+1,len(letters)+1):
             sub=('').join(letters[i:k])
             #print (i,k,sub)
             if palindrome(sub) and sub not in psub:
                 psub.append(sub)

    count.append(len(psub))

for c in count:
    print (c)

Input:
The first line of input contains an integer T denoting the number of test cases.    Then T test cases follow. Each test case contains a string.

Output:
Print the count of distinct continuous palindromic sub-strings of it.

First off, you should organize your code by using functions as it will help you simplify it and is better for reusability and testing. As I see it, you can use a function to generate the various substrings of the word, and a function to count the amount of palindromes in a string. You could also organize the remaining top-level code in a main() function and guard it with if __name__ == '__main__'::

def palindrome(s):
    if s==s[::-1]:
        return True

def substrings(letters):
    subs = []
    for i in range(len(letters)):
        for k in range(i+1,len(letters)+1):
             sub=('').join(letters[i:k])
             subs.append(sub)
    return subs

def count_palindromes(word):
    letters=[]
    psub=[]
    for l in word:
        letters.append(l)
        if l not in psub:
            psub.append(l)

    for sub in substrings(letters):
         if palindrome(sub) and sub not in psub:
             psub.append(sub)

    return len(psub)

def main():
    noTest=int(input())
    s=[]
    for _ in range(noTest):
        s.append(input())

    count=[]
    for word in s:
        count.append(count_palindromes(word))

    for c in count:
        print (c)

if __name__ == '__main__':
    main()

Now we can start tidying things up. For starter, apply PEP8 to make it look like other Python code and simplify main() as we don't need all those intermediate lists anymore:

def main():
    tests_count = int(input())
    for _ in range(tests_count):
        print(count_palindromes(input()))

Apply the same kind of improvements to the palindrome function:

def is_palindrome(string):
    return string == string[::-1]

Second, use a better datastructure than lists in count_palindromes as checking l not in psub or sub not in psub is an \$\mathcal{O}(n)\$ operation compared to \$\mathcal{O}(1)\$ for the same kind of check in a set. Besides, sets guarantee that each element added in is only added once, so you can get rid of these checks yourself.

def count_palindromes(word):
    palindromes = set(word)
    for sub in substrings(word):
        if is_palindrome(sub):
            palindromes.add(sub)
    return len(palindromes)

Note that I passed the whole word to substrings instead of a list of letters, as len or slicing ([i:k]) would work on both. Also, you could use the filter builtin instead of an explicit check which will allow you to write:

def count_palindromes(word):
    palindromes = set(word)
    palindromes.update(filter(is_palindrome, substrings(word)))
    return len(palindromes)

Lastly, you can optimize substring generation by:

getting rid of the join call as you already have strings when slicing a string;
using a generator rather than building a list and appending to it.

def substrings(word):
    word_length = len(word)
    for i in range(word_length):
        for k in range(i + 1, word_length + 1):
            yield word[i:k]

Full code:

def substrings(word):
    word_length = len(word)
    for i in range(word_length):
        for k in range(i + 1, word_length + 1):
            yield word[i:k]

def is_palindrome(string):
    return string == string[::-1]

def count_palindromes(word):
    palindromes = set(word)
    palindromes.update(filter(is_palindrome, substrings(word)))
    return len(palindromes)

def main():
    tests_count = int(input())
    for _ in range(tests_count):
        print(count_palindromes(input()))

if __name__ == '__main__':
    main()

You could also use the itertools module and adapt a bit the pairwise recipe to make substrings a bit more efficient:

import itertools

def substrings(string, size):
    iterators = itertools.tee(string, size)
    substrings_generator = (
            itertools.islice(i, start, None)
            for start, i in enumerate(iterators)
    )
    return map(''.join, zip(*substrings_generator))

def is_palindrome(string):
    return string == string[::-1]

def count_palindromes(word):
    palindromes = set(word)
    for sub_size in range(2, len(word) + 1):
        palindromes.update(filter(is_palindrome, substrings(word, sub_size)))
    return len(palindromes)

def main():
    tests_count = int(input())
    for _ in range(tests_count):
        print(count_palindromes(input()))

if __name__ == '__main__':
    main()


Collision detection inconsistencies I've been wondering about simple collision in C++ for a while now, and I've tried to make up my own code.

The code posted below works when walls are placed in a perfect rectangle around the player. However, once you randomly place them in the array, as in the code, weird inconsistencies seem to occur. Sometimes the player collides, sometimes the player doesn't.

If anyone would be as kind as to point out what the error is, I'd be much appreciated. In a clear step by step manner, if you would: I'm very new to this and chances are small that I can make the logical steps of thought necessary to successfully implement your suggestions.

#include "string.h"
#include "surface.h"
#include "stdlib.h"
#include "template.h"
#include "game.h"

using namespace Tmpl8;

int pacmanPosX = 32;
int pacmanPosY = 32;
int indexX;
int indexY;

bool colR;
bool colL;
bool colU;
bool colD;

Sprite pacman (new Surface( "assets/pacmanLeft.png"), 1);

Surface* tileSet[3];

int levelMap[8][8] =
{{1,1,1,1,1,1,1,1},
 {1,0,0,0,0,0,0,1},
 {1,0,0,0,0,1,0,1},
 {1,0,0,1,0,0,0,1},
 {1,0,0,0,1,0,0,1},
 {1,1,0,0,1,1,1,1},
 {1,0,0,0,0,0,1,1},
 {1,1,1,1,1,1,1,1}};

void Game::Init()
{
   // put your initialization code here; will be executed once
   tileSet[0] = new Surface( "assets/pacmanFloor.png" ); // Sets up data for tileSet
   tileSet[1] = new Surface( "assets/pacmanWall.bmp" );
   tileSet[2] = new Surface( "assets/pacmanCookie.png" );

}

void Game::Tick( float a_DT )
{
    m_Screen->Clear( 0 );

     for (indexX = 0; indexX < 8; indexX++)
    {
        for (indexY = 0; indexY < 8; indexY++)
        {
            int tile = levelMap[indexY][indexX];
            tileSet[tile]->CopyTo( m_Screen, indexX * 32, indexY * 32 );
        }
    }

    pacman.Draw(pacmanPosX, pacmanPosY, m_Screen);

    //Defining collision requirements for walls//

    if(levelMap[(pacmanPosX+32)/32][pacmanPosY/32] == 1) //Right collision
    {
        colR = true;
    }else colR = false;

    if(levelMap[(pacmanPosX-2)/32][pacmanPosY/32] == 1) //Left collision
    {
        colL = true;
    }else colL = false;

    if(levelMap[pacmanPosX/32][(pacmanPosY-2)/32] == 1) //Upward collision
    {
        colU = true;
    }else colU = false;

    if(levelMap[pacmanPosX/32][(pacmanPosY+32)/32] == 1) //Downward collision
    {
        colD = true;
    }else colD = false;

    //Setting Up Controls//

    if (GetAsyncKeyState(VK_RIGHT) && colR == false){pacmanPosX += 2;}
    if (GetAsyncKeyState(VK_LEFT) && colL == false){pacmanPosX -= 2;}
    if (GetAsyncKeyState(VK_UP) && colU == false){pacmanPosY -= 2;}
    if (GetAsyncKeyState(VK_DOWN) && colD == false){pacmanPosY += 2;}

    Sleep( 10 );
}

Bug in your code
Here you are accessing the levelMap by 'Y' then 'X'
        int tile = levelMap[indexY][indexX];

In all other places you are accessing the levelMap by 'X' then 'Y'
if(levelMap[(pacmanPosX+32)/32][pacmanPosY/32] == 1) //Right collision

So it looks like you are running around a maze that has been printed incorrectly on the screen. and thus it looks like collision detection is not working (it probably is) but the detection does not match the visual cues.
Reviewing your code:
These expressions
if(levelMap[(pacmanPosX+32)/32][pacmanPosY/32] == 1) //Right collision
{
    colR = true;
}else colR = false;

Can be simplifies too:
colR = (levelMap[(pacmanPosX+32)/32][pacmanPosY/32] == 1); //Right collision

While these
if (GetAsyncKeyState(VK_RIGHT) && colR == false){pacmanPosX += 2;}

can be simplified to:
if (GetAsyncKeyState(VK_RIGHT) && !colR)
{    pacmanPosX += 2;
}

Putting the statement on the same line as the test makes it harder to debug (when stepping through with the debugger) as you can not see when then the statements are executed. By putting it on its own line stepping through with the de-bugger does become easier as you will see it skip the statement when the condition fails.
Global state is the easy way to do things.
int pacmanPosX = 32;
int pacmanPosY = 32;
int indexX;
int indexY;

bool colR;
bool colL;
bool colU;
bool colD;

But in the long run makes your code more brittle and harder to update. Pass things as parameters to make it easier to maintain and extend your code (or create an object that wraps the information).
Also it looks like some of these properties would be better held in arrays rather than individual variables.
Edit base on comments:
This only detects a collision if the top right corner hits a wall.
colR = (levelMap[(pacmanPosX+32)/32][pacmanPosY/32] == 1); //Right collision

Change too:
colR =    (levelMap[(pacmanPosX+33)/32][(pacmanPosY+00)/32] == 1)  // Top right
       || (levelMap[(pacmanPosY+33)/32][(pacmanPosY+31)/32] == 1); // bottom right

// Use +33 because moving left you use -2
// When you move right +32 puts you in the first pixel of the next block
//                     +33 puts you in the second pixel just like move left detection

// Use +31 on the Y access because this is still inside the current sprite.

Note If you have a hole in the wall that is exactly one block wide you need to hit it dead center to pass through the gap. To give you some wiggle room you can move the corners in slightly.
colR =    (levelMap[(pacmanPosX+33)/32][(pacmanPosY+02)/32] == 1)  // Top right
       || (levelMap[(pacmanPosY+33)/32][(pacmanPosY+29)/32] == 1); // bottom right

Here you do not need to hit the whole in the wall dead center and you get some wiggle room passing through.