"Almost Sorted" coding challenge The "Almost Sorted" problem from HackerRank:

  Given an array with \$n\$ elements, can you sort this array in ascending order using only one of the following operations?

  Swap two elements.
  Reverse one sub-segment.

  Output format

  If the array is already sorted, output yes on the first line. You do not need to output anything else.
  If you can sort this array using one single operation (from the two permitted operations) then output yes on the first line and then:

  If you can sort the array by swapping \$d_l\$ and \$d_r\$, output swap l r in the second line. \$l\$ and \$r\$ are the indices of the elements to be swapped, assuming that the array is indexed from \$1\$ to \$n\$.
  Else if it is possible to sort the array by reversing the segment \$d[l\ldots r]\$, output reverse l r in the second line.  \$l\$ and \$r\$ are the indices of the first and last elements of the subsequence to be reversed.

  If an array can be sorted by either swapping or reversing, stick to the swap-based method.
  If you cannot sort the array in either of the above ways, output no in the first line.

I am trying to improve my Python so I would appreciate if somebody could review the following piece of code. I know it can be done with less code in Python, I am just not sure how.

import sys

def checkSorted(arr):
    return all(arr[i] <= arr[i+1] for i in range(len(arr)-1))

def almostSorted(arr):
    if len(arr) == 1 or checkSorted(arr):
        print("yes")
        return

    # Find first item out of order from the left
    i = 0
    while i < len(arr) - 1 and arr[i] < arr[i+1]:
        i+=1
    first_ooo_left = i

    # Find first item out of order from the right
    i = len(arr) - 1
    while i > 0 and arr[i-1] < arr[i]:
        i-=1
    first_ooo_right = i

    # Same item?
    if (first_ooo_left == first_ooo_right):
        print("no")
        return

    l = first_ooo_left
    r = first_ooo_right
    swaps = 0
    while r-l > 0:
        if arr[l] > arr[r]:
            arr[r], arr[l] = arr[l], arr[r]
            swaps += 1
        r-=1
        l+=1

    sorted = checkSorted(arr)

    if sorted:
        if swaps == 1:
            print("yes")
            print("swap " + str(first_ooo_left+1) + " " + str(first_ooo_right+1))
        elif swaps == (first_ooo_right-first_ooo_left+1)//2:
            print("yes")
            print("reverse " + str(first_ooo_left+1) + " " + str(first_ooo_right+1))
    else:
        print("no")

Before anything else: add tests
To be on the safe side before any review of an algorithmic problem, I tend to ensure the code is testable and tested. On that point, the problem is nice for three reasons:

for a given well defined input (a list), it returns a clear output (a string)
it is easy to generate test cases for the different cases
a few example are provided

You should start by rewriting your function so that it returns things instead of printing them. Then, you can write automated tests.
For instance, you can write the following tests:
# From the problem
assert almostSorted([4, 2]) == "yes\nswap 1 2"
assert almostSorted([3, 1, 2]) == "no"
assert almostSorted([1, 5, 4, 3, 2, 6]) == "yes\nreverse 2 5"
# Generate various small strings
for i in range(10):
    l_i = list(range(i))
    assert almostSorted(l_i) == "no"  # WRONG
    result_reversed = almostSorted(list(reversed(l_i)))
    if i < 2:
        assert result_reversed == "no" # WRONG
    elif i < 4:
        assert result_reversed == "yes\nswap %d %d" % (1, i)
    else:
        assert result_reversed == "yes\nreverse %d %d" % (1, i)
    for j in range(i):
        for k in range(j+1, i):
            l_swap = list(range(i))
            l_swap[j], l_swap[k] = l_swap[k], l_swap[j]
            result_swap = almostSorted(l_swap)
            assert result_swap == "yes\nswap %d %d" % (j+1, k+1)
            l_rev = list(range(i))
            l_rev[j:k+1] = l_rev[j:k+1][::-1]
            result_rev = almostSorted(l_rev)
            if k - j >= 3:
                assert result_rev == "yes\nreverse %d %d" % (j+1, k+1)
            else:
                assert result_swap == "yes\nswap %d %d" % (j+1, k+1)

You could also use a proper testing framework but this is the first step.
It is enough to see an issue spotted by Martin. R in the comment. This is enough easy to fix and I'll let you deal with this.
Except for that, your code seems to be working fine which is a great news even if the test suite is far from perfect.
Spoiler alert: more issues are found later.
Style
There is an official standard Python style guide called PEP 8. This is highly recommended reading. It gives guidelines to help writing code that is both readable and consistent. The Python community tries to follow these guidelines, more or less strictly (a key aspect of PEP 8 is that it provides guidelines and not strict rules to follow blindly).
It deals with various aspects of the code style: naming conventions, indentation convention, etc.
You'll find various tools to try to check whether your code is PEP 8 compliant and if is it not, to try and fix this:

pycodestyle package (formerly known as pep8) to check you code

pep8online to check your code with an online tool

autopep8 package to fix your code automatically

Also, this is also checked by various linters: pylint, pyflakes, flake8, etc.

In your case, the naming convention of the function seems to be off:

checkSorted should be check_sorted or even better is_sorted.

almostSorted should be almost_sorted (or is_almost_sorted ?)

Regarding the naming, 2 other details could be improved:

arrays are usually called list in the Python universe. Thus, lst may be a better argument name than arr.

sorted is the name of a builtin. Thus, having a local variable named the same way could lead to issues in more complex code. In your case, you don't even need that variable in the first place...
(There are other PEP8 "issues" about spacing but nothing very interesting to me).

Same logic but different
The logic to check is the list is sorted and the logic to find first item out of order from the left look the same but shaped in a different way.
Actually, a closer look shows that the logic itself is different in a subtle way: identical consecutive values are not handled the same way in both places. This can be shown by writing more tests with equal values:
for i in range(10):
    l_equal = [42] * i
    assert almost_sorted(l_equal) == "yes"
    l_i = list(range(i))
    assert almost_sorted(l_i) == "yes"
    result_reversed = almost_sorted(list(reversed(l_i)))
    if i < 2:
        assert result_reversed == "yes"
    elif i < 4:
        assert result_reversed == "yes\nswap %d %d" % (1, i)
    else:
        assert result_reversed == "yes\nreverse %d %d" % (1, i)
    for j in range(i):
        for new_val in (0, 42):
            l_almost_equal = [42] * i
            l_almost_equal[j] = new_val
            assert "yes" in almost_sorted(l_almost_equal)

I'll stop the review here to let you deal with the issue found.
Before finding the problem, I was about to suggest something along the lines of:

instead of checking is the list is sorted first, you could try to find the first out of order value and if you do not find it, then the array is sorted.

the for loop in is_sorted is clearer that the while loop, I reckon you should use the former rather than the later.

For instance, you could write something like:
# Find first item out of order from the left
for i in range(len(lst)-1):
    if lst[i] > lst[i+1]:
        first_ooo_left = i
        break
else: # no break: sorted already
    return "yes"


Stopwatch exercise from training course Github

Github for easy testing

I'm currently undertaking a training course to try & further develop my skills in

C#. The latest exercise was to create a basic stopwatch class that meets the following criteria -

  Design a class called Stopwatch. The job of this class is to simulate
  a stopwatch. It should provide two methods: Start and Stop. We call
  the start method first, and the stop method next. Then we ask the
  stopwatch about the duration between start and stop. Duration should
  be a value in TimeSpan. Display the duration on the console. We should
  also be able to use a stopwatch multiple times. So we may start and
  stop it and then start and stop it again. Make sure the duration value
  each time is calculated properly. We should not be able to start a
  stopwatch twice in a row (because that may overwrite the initial start
  time). So the class should throw an InvalidOperationException if its
  started twice.

  Educational tip: The aim of this exercise is to make you understand
  that a class should be always in a valid state. We use encapsulation
  and information hiding to achieve that. The class should not reveal
  its implementation detail. It only reveals a little bit, like a
  blackbox. From the outside, you should not be able to misuse a class
  because you shouldn’t be able to see the implementation detail.

I'm sure someone is bound to mention it, but yes I'm aware that there is already a stopwatch class in the .NET framework, but as this was the exercise I wanted to try & accomplish it based on the requirements.

The class is as follows -

public class Stopwatch
{
    private TimeSpan _duration;
    // private TimeSpan _start;
    private DateTime _start;

    public Stopwatch()
    {
        ZeroStart();
    }

    public void Start()
    {
        // if (_start != TimeSpan.Zero)
        if (_start != DateTime.Min)
            throw new InvalidOperationException("The stopwatch has already been started.");

        _start = DateTime.Now;
    }

    public TimeSpan Stop()
    {
        // if (_start == TimeSpan.Zero)
        if (_start == DateTime.Min)
            throw new InvalidOperationException("The stopwatch has not been started.");

        _duration = DateTime.Now- _start;
        ZeroStart();
        return _duration;
    }

    private void ZeroStart()
    {
        // _start = TimeSpan.Zero;
        _start = DateTime.Min;
    }
}

I haven't done an awful lot of OOP or even effective work with classes. Most of the stuff I have done in the past has been more procedural based stuff, just a long list of static methods in static classes etc...

I'm not sure if this is really enough to go on for anyone to actually critique me on so I'm sorry if that's the case.

I've tested the code using the following -

class Program
{
    static void Main(string[] args)
    {
        UsingStopwatch();
    }

    static void UsingStopwatch()
    {
        Console.WriteLine("This is a stop watch. Type 'start' to start it & 'stop' to stop it.");
        var stopwatch = new Stopwatch();

        while (true)
        {
            var input = Console.ReadLine();

            switch (input.ToLower())
            {
                case "start":
                    stopwatch.Start();
                    break;
                case "stop":
                    Console.WriteLine(stopwatch.Stop());
                    break;
                default:
                    Console.WriteLine("Sorry I don't recognize that.");
                    break;
            }
        }
    }
}

Update

Thanks to jandotnet I have made some updates to how I store some of the data. _start is now DateTime & I have comparing values with DateTime.MinValue. I have updated the above with the full snippet.

I see no problem with a programming challenge that has a low accuracy stopwatch.  I have problems with your particular implementation.  Since one aim of exercise is to be fully aware of state, I am surprised in how it maintains state indirectly and privately based on the value in _start.  A simple stopwatch has a very simple state: its either running or not.  So I would just directly offer a Boolean IsRunning property with public getter and private setter.  This removes any mystery as to what the state is, and it exposes that state clearly and publicly to the consumer of the stopwatch.

I personally would prefer _start to be renamed _startTime, but that's no big deal.  I would have the Start() method do 2 things: (1) sets IsRunning to true, and (2) sets _startTime to DateTime.UtcNow.  Note that UtcNow is not only faster than Now (Now actually calls UtcNow first), but also allows the stopwatch to not have any odd side effects from Daylight Saving Time transitions (really the biggest reason to use UtcNow).

My other big gripe is the Stop() method should just stop the stopwatch.  Your implementation has it stopping and also returning the duration.  I would change Stop() to be void, and then offer a TimeSpan Duration property.  Stop() should set IsRunning to false, and somehow store the duration for later retrieval.

This now gives you flexibility that you did not possess before.  What behavior do you want the stopwatch to exhibit if someone asks for the Duration while its running?  Do you want to throw an exception because Stop() has not been issued first?  Or would you like to return a Duration while running?


Luhn Algoritm for social security numbers I just got some negative feedback on some code I wrote and I was not able to know exactly what was wrong with it (I tried). It is a program that verifies to see if a social security number is valid or not. The program is working fine.So I would really appreciate it if somebody could take a look and tell me if this can be optimized in some way.

Here is the code:

public class Logic
{
    public static bool ValidityChecker(string socialSecurityNumber)
    {
        FileInfo f = new FileInfo("C:\\Log\\log.txt"); //Checks to see if folder path exists. Otherwise creates a C:\Log\ folder.
        if (!f.Directory.Exists)
        {
            Directory.CreateDirectory(f.DirectoryName);
        }

        if (ValidityCheckIsNotNull(socialSecurityNumber) == true && ValidityCheckIsSSN(socialSecurityNumber) == true) //If the string is not null/empty and if the Social Security Number is valid, then return true.
            return true;
        else //If the string is null/empty or the social security number is invalid. Then log the invalid data and return false.
            using (StreamWriter w = new StreamWriter("C:\\Log\\log.txt", true))
            {
                w.WriteLine(DateTime.Now + " Kandidatdata: " + socialSecurityNumber.ToString());
                w.Close();
            }
        return false;
    }

    public static bool ValidityCheckIsNotNull(string socialSecurityNumber) //If string is null or empty return false. Otherwise return true.
    {
        if (string.IsNullOrEmpty(socialSecurityNumber))
            return false;
        else return true;
    }

    public static bool ValidityCheckIsSSN(string socialSecurityNumber) //Uses the algoritm (from the task documentation) to see if a Social Security number is valid or not.
    {
        int totalSSN = socialSecurityNumber.Where((e) => e >= '0' && e <= '9').Reverse().Select((e, i) => (e - 48) * (i % 2 == 0 ? 1 : 2)).Sum((e) => e / 10 + e % 10);
        return totalSSN % 10 == 0;
    }
}

Some quick remarks:

Use clear variable names: f doesn't tell me anything.
Why do you call Directory.CreateDirectory before you do the actual validity check?
Do you even need to call Directory.CreateDirectory? I'm not sure, but I wouldn't be surprised if new StreamWriter("C:\\Log\\log.txt", true) creates any missing directories etc.
"C:\\Log\\log.txt" is used in two places, so it should be at least centralized. Assign it to a variable or perhaps even a constant.
You wrote a whole method -- ValidityCheckIsNotNull -- to encapsulate string.IsNullOrEmpty? WHY? What do you gain from this? What's the point?
Your lines are waaay too long. 170 characters which contain multiple LINQ methods are simply unclear. At least split it up into multiple lines.

Here is that same code, split over multiple lines:

socialSecurityNumber
   .Where((e) => e >= '0' && e <= '9')
   .Reverse()
   .Select((e, i) => (e - 48) * (i % 2 == 0 ? 1 : 2))
   .Sum((e) => e / 10 + e % 10);

Already it's a lot clearer. However, you need to comment this: .Select((e, i) => (e - 48) * (i % 2 == 0 ? 1 : 2)) because I cannot figure out easily what this logic is supposed to do. Properly naming e and i would help, I guess. Or even better: move it to a properly named method of its own.

Comments like //If the string is not null/empty and if the Social Security Number is valid, then return true. don't tell me anything your code doesn't tell me.
ValidityChecker isn't a proper method name: "Use verbs or verb phrases to name methods."
All of your logging logic should be in a separate method. It should not be part of ValidityChecker.
Logic as the name of your class? Far too generic.


Find out whether a given text contains any of the prohibited words We have been given a list of strings which are blacklisted. The goal is to identify if a given text contains any of these blacklisted strings. The restriction here is that the blacklisted string needs to match on the word boundary e.g. consider a blacklist string "abc" and text "abc pqr", the text in this case is unsafe (i.e. it contains a blacklisted string). On the other hand if the text is "abcoqr", then the text is safe since the string "abc" is not on the word boundary. Also the relative ordering of words in a blacklisted string needs to be checked e.g. if a blacklisted string is "abc pqr", then the text "pqr abc" is safe since the ordering of the words in the text does not match that of the blacklisted string.

Here is my solution using a modified Trie data structure. https://gist.github.com/hgadre/d4e9ec576932167f01fd33970002a882

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

public class SafeText {
    static class Tuple {
       int span = 0; // the length of previous words which should have been matched if end = true.
       boolean end; // marks the identification of a blacklisted string.
       Set<String> nextWords = new HashSet<>(); // next set of words to search for matching blacklisted strings.

       public void setEnd(boolean end, int span) {
          this.span = span;
          this.end = end;
       }

       public boolean isEnd(int span) {
           return end && span == this.span;
       }

       public void addNextWord (String word) {
           this.nextWords.add(word);
       }

       public boolean containsWord(String word) {
           return this.nextWords.contains(word);
       }
    }

    private final Map<String, Tuple> m = new HashMap<>();

    public SafeText(List<String> blackList) {
        Collections.sort(blackList);
        for (String str : blackList) {
           String[] tokens = str.split("\\s");
           int i = 0;
           for (; i < tokens.length - 1; i++) {
              m.computeIfAbsent(tokens[i], x -> new Tuple()).addNextWord(tokens[i+1]);
           }
           m.computeIfAbsent(tokens[i], x -> new Tuple()).setEnd(true, tokens.length-1);
       }
    }

    public boolean isSafe(String text) {
        String[] tokens = text.split("\\s");
        for (int i = 0; i < tokens.length; i++) {
        String key = tokens[i];
        int j = i;
        while (j < tokens.length && m.containsKey(key)) {
            Tuple t = m.get(key);
            if (t.isEnd(j-i)) {
               return false;
            } else if ((j+1) < tokens.length && t.containsWord(tokens[j+1])) {
               key = tokens[j+1];
               j++;
            } else {
               break;
            }
         }
      }
      return true;
    }
 }

Is this an optimal solution? Or is there any better approach to solve this problem?

I may have missed something. But why don't you just use simple collections ?

// Naive implementation
class Text {
  public Text(String content, Set<String> blacklist) {
    this.words = new HashSet<>(Arrays.asList(content.split("\\s")));
    this.blacklist = blacklist;
  }

  public boolean isSafe() {
    for (String forbidden: this.blacklist) {
        if (this.words.contains(forbidden) ) {
            return false;
        }
    }
    return true;
  }
}


Linked List Sorting Algorithm I was working on a sorting algorithm for my linked list implementation and wanted to get other people's input/comments/critique. What do you think of it in all aspects, including style?

I was told that it is preferred to use a bool type for the changeFlag instead of using an int as it makes it more readable.  Thoughts?

void LinkedList::sort()
{
    if (head != 0)
    {
        Node* current = head;
        Node* prev = 0;
        Node* tempNode = 0;
        bool changeFlag = false;
        for (int i = 0; i < length; i++)
        {
            while (current->next != 0)
            {
                tempNode = current->next;

                if (current->value > tempNode->value)
                {
                    changeFlag = true;
                    current->next = tempNode->next;
                    tempNode->next = current;
                    if (prev != 0)
                        prev->next = tempNode;
                    prev = tempNode;
                    if (head == current)
                        head = tempNode;
                    if (current->next == 0)
                        end = current;
                }
                else
                {
                    prev = current;
                    current = current->next;
                }
            }
            if (changeFlag == false)
                break;
            else
            {
                prev = 0;
                current = head;
                changeFlag = false;
            }
        }
    }
}

What I would take away from this is how the standard library does this.

It disassociates sorting algorithms from specific container types (by using iterators). Then you can write the sorting algorithm in a way that is independent of the actual container type.

What I dislike about your code is that when you move elements you basically re-order the list (you actually move the nodes). This is a complex operation taking many checks. Personally I would leave the actual nodes where they are and move the values between nodes. std::swap() can be used for this.

This:

               current->next = tempNode->next;
                tempNode->next = current;
                if (prev != 0)
                    prev->next = tempNode;
                prev = tempNode;
                if (head == current)
                    head = tempNode;
                if (current->next == 0)
                    end = current;

Can be replaced with:

                swap(current->value, tempNode->value);

With C++ new move semantics this is now very efficient.

  EDIT: I was told that it is preferred to use a bool type for the changeFlag instead of using an int as it makes it more readable?? Thoughts?

Yes. Use the correct type. bool is a truth value it imparts meaning to the person reading the code. Humans need that extra meaning to help them understand the code better.


A password generator This a small project for a beginner, it basically generate symbols digits letters depending on the user input I want to make the code better and learn from my mistakes.
import string
import random
asklenght = 0

asku = int(input("Please choose a method:\n 1)Digits \n 2)Letters \n 3) Symbols \n 4) All \n "))
asklenght = int(input("How long do you want your password to be ?  "))

digits = string.digits
letters = string.ascii_letters
symbols = string.punctuation

if asku == 1:
    outputpass = random.choice(digits)
elif asku == 2:
    outputpass = random.choice(letters)
elif asku == 3:
    outputpass = random.choice(symbols)
else:
    outputpass = random.choice(digits)
    outputpass = random.choice(letters)
    outputpass = random.choice(symbols)

for i in range(asklenght - 1 ):
    if asku == 1:
        outputpass += random.choice(digits)
    elif asku == 2:
        outputpass += random.choice(letters)
    elif asku == 3:
        outputpass += random.choice(symbols)
    else:
        outputpass += random.choice(digits)
        outputpass += random.choice(letters)
        outputpass += random.choice(symbols)

print(outputpass)

One of the easiest ways to improve code is to reduce repetition. The
most-repeated elements in your code are random.choice and outputpass. You
can reduce bulk by importing what you need rather than importing a module containing
what you need.
from random import choice
from string import digits, ascii_letters, punctuation

Another bulk-reducer is to choose more practical variable names. By practical,
I mean names that are are more
compact but no less informative to the reader. For example, in a script that
generates a password, the context is quite clear, so you can get away with a
very short variable name: pw is one sensible option. Similarly, a simple name
like n is just as clear as asklenght, because in context everybody
understands what n means.
On the subject of naming, what does asku mean? Not very much to me. But
your input prompt text is very clear. A better name might be be method
(which your text uses)
or perhaps even chartype, which is more specific.
Your code has a bug for All: it creates a password 3x as long as it should be.
You don't need any special logic for n of 1, 2, or 3. Just set up
the loop and let it handle everything. The key is to use n rather
than n - 1.
for i in range(n):
    ...

The conditional logic inside the loop has only one purpose: selecting
the relevant characters to include in the password. Anytime you have
a situation like that, logic can often be greatly simplified by
creating a data structure.
characters = {
    1: digits,
    2: ascii_letters,
    3: punctuation,
    4: digits + ascii_letters + punctuation,
}

That change drastically reduces the code inside the loop:
pw = ''
for i in range(n):
    pw += choice(characters[chartype])

If you want to impress your friends, you can even write it
in one shot by using a comprehension:
pw = ''.join(choice(characters[chartype]) for i in range(n))

For usability, you might also consider changing the way that chartype
works: instead of asking users to type a number, which requires a small
mental translation from a meaningful thing (letters, numbers, symbols, all)
to an abstract thing (1, 2, 3, 4), you could just let them type the
first letter of the actual thing.
Also for usability, if the context is already clear, shorter messages
are easier on users, because they can scan them very quickly.
A final change to consider is whether to subject your user to interrogation by a
computer. I come from the school of thought that says humans tell computers what to
do, not the other way around. In addition to being pro-human, that policy has many practical benefits.
For example, isn't it annoying that everything time you edit the code and
re-run it, you have to answer the same damn questions. A different approach
is to take those instructions directly from the command line. Python
ships with a module called argparse that would work well for a script
like this, but you can skip that if you want and just use sys.argv directly.
You probably should do some input validation, but I'll leave that
for you to pursue or for some other reviewer to comment on. Here's
the code with those suggested changes.
from string import digits, ascii_letters, punctuation
from random import choice
from sys import argv

if len(argv) == 3:
    chartype = args[1]
    n = int(args[2])
else:
    prompt = 'Password characters:\n (D)igits\n (L)etters\n (S)ymbols\n (A)ll\n'
    chartype = input(prompt)
    n = int(input("Length?  "))
    chartype = chartype.lower()[0]

characters = {
    'd': digits,
    'l': ascii_letters,
    's': punctuation,
    'a': digits + ascii_letters + punctuation,
}

pw = ''.join(choice(characters[chartype]) for i in range(n))

print(pw)


Using dapper.net and repository pattern in Azure websites I'm currently on Azure Websites & Azure SQL, some documentation recommends using Async as much as possible as cloud services higher latency and have chances of dropping traffic, but it's also cautioning that Async/Await have overhead.

Should I...

Implement an IStoreRepository interface (although I don't know if
it's helpful to me)?
Use Async versions of Query and OpenConnection?
Am I repeating myself so much in different Repository classes like GetById, GetAll, etc?  Is there a more elegant solution?

I am pretty new to dapper.net and I would like to have some feedback on my current implementation.

StoreRepository

public class StoreRepository
{
    public async Task<Store> GetById(int id)
    {
        using (var conn = await SqlHelper.GetOpenConnectionAsync())
        {
            const string sql = "Select * from Stores where Id = @Id";
            var res = await conn.QueryAsync<Store>(sql, new { Id = id });
            return res.FirstOrDefault();
        }
    }
    ...
}

StoreController

public async Task<ActionResult> Details(int id)
{
    var db = new StoreRepository();
    var store =  await db.GetById(id);
    return View(store);
}

SQLHelper

internal static class SqlHelper
{
    public static readonly string connectionString = ConfigurationManager.ConnectionStrings["ttcn"].ConnectionString;

    public async static Task<DbConnection> GetOpenConnectionAsync()
    {
        return new ProfiledDbConnection(await GetOpenConnection(false), MiniProfiler.Current);
    }
    private async static Task<SqlConnection> GetOpenConnection(bool mars = false)
    {
        var cs = connectionString;
        if (mars)
        {
            SqlConnectionStringBuilder scsb = new SqlConnectionStringBuilder(cs);
            scsb.MultipleActiveResultSets = true;
            cs = scsb.ConnectionString;
        }
        var connection = new SqlConnection(cs);
        await connection.OpenAsync();
        return connection;
    }
}

Always program to abstraction, meaning it is always preferred to write your code against interface or abstract class rather than specific implementation.  So, IStoreRepository  would be a good idea.
If you are not going to ever change your implementation, do not use abstract or interfaces. Make it simple and readable.
If you are considering using mapper and all, use DI container too so that you don't have to do this:

var db = new StoreRepository();
// creating instance like this is not a good idea, rather inject the dependencies

Same thing goes for your SQL helper class too.  Inject as a dependency in your repository.
If you use EF/nhibernate, then your code will not be repeating as all code will have the same kind of code.  You write a generic base class for them and pass the type of data you want to retrieve it from the database.  But in case of hand-written SQL, that could not be done that easily as you need to write different SQL for different type. Again, that is your choice.
It is good to use async await for managing an SQL connection so you don't just block the currently-working thread. Overhead is small in overall picture.


Hackerrank - Sherlock and the Beast I am trying to solve the Sherlock and The Beast HackerRank challenge. Most tests timeout, however when I try a custom stretch test case (T = 20 and all N = 100000), it returns successfully, so I'm not sure what the problem is.

The idea of the algorithm is that I find the number of possible combinations of threes and fives for a given number of digits, then I treat threes as zeros and fives as ones, in order to process a binary number. So for N = 3 digits, we have 8 combinations (from 7 to 0) which in binary terms is (descending) from 111 to 000, which in three and five terms is from 555 to 333.

public class Solution {

    public static void main(String[] args) {
        Scanner in = new Scanner(System.in);
        int testCases = in.nextInt();
        for(int i = 0; i < testCases; i++){
            System.out.println( solve( in.nextInt() ) );
        }
    }

    private static String solve(int n){

        long combos = (long)Math.pow(2,n);
        for(long i = combos-1; i>=0; i--){
            String toStr = String.format("%"+n+"s", Long.toBinaryString(i)).replace(' ', '0');

            String modified = toStr.replace("0","3").replace("1","5");
            int threes =0;
            for( int j=0; j<modified.length(); j++ ) {
                if( modified.charAt(j) == '3' ) {
                    threes++;
                }
            }
            if(threes%5==0) {
                int fives =0;
                for( int k=0; k<modified.length(); k++ ) {
                    if( modified.charAt(k) == '5' ) {
                        fives++;
                    }
                }
                if(fives%3==0) return modified;
            }
        }
        return "-1";
    }
}

My non-exhaustive list of comments, considering that you seem to prefer (based on observed skill level) plain Java and do not want to use Java 8 yet.

Adhere to the Java's coding standards. One of the catches here is that method names are in camelCase, so it would be private static String solve(int n) {.
Intendation and code formatting. You seem to not be using a single standard for all your code. Furthermore I have to say that using spaces where neccessary will not cause any harm, some examples and improvements.

2.1. for(int i = 0; i < t; i++){ -> for (int i = 0; i < t; i++) {

2.2. long combos = (long)Math.pow(2,n); -> long combos = (long)Math.pow(2, n);

2.3. if(threes%5==0) { -> if (threes % 5 == 0) {

Please be consistent with your formatting is the bottom line.
On to the next point, variable names. It does not hurt to use longer variable names if that makes them more meaningful. In your main you have a variable called t, this name is not descriptive, please consider describing what it actually means. Another example is i, ii and iii in your for-loops, it is much more natural to use i, j, k for these variables.
You could use more abstraction, as your solve method is not easy to follow at all. Consider creating smaller methods that process one step at a time, this will also enable you to be able to test your methods at some point.


UPenn homework 3: histogram Please see here for the general description.
Exercise 3: histogram :: [Integer] -> String
takes as input a list of Integers between 0 and 9 (inclusive),
and outputs a vertical histogram showing how many of each number
were in the input list.
histogram :: [Integer] -> String
histogram xs = let
  (a,m) = getCountOfElements xs
  unpadded = getStrings a
  padded = pad m unpadded
  transposed = transp ([], padded)
  in mkOutput transposed

mkOutput :: [String] -> String
mkOutput xss = let nums = "0123456789\n"
                    sep = "==========\n"
                    t = foldl (\a b -> b++"\n"++a) "" xss
                in t ++ sep ++ nums

pad :: Integer -> [String] -> [String]
pad _ [] = []
pad n (xs:xss) = (xs ++ (replicate (fromInteger n - length xs) ' ')) : (pad n xss)

getCountOfElements :: [Integer] -> ([Integer],Integer)
getCountOfElements ys =
  let arr = foldr (\e acc -> count e acc) (replicate 10 0) ys
        where
          count :: Integer -> [Integer] -> [Integer]
          count n xs = let idx = fromInteger n
                       in (take idx xs) ++ [(xs!!idx + 1)] ++ (drop (idx+1) xs)
      maxElt = maximum arr
  in (arr, maxElt)

getStrings :: [Integer] -> [String]
getStrings = map (\x -> replicate (fromInteger x) '*')

-- good, descriptive name is lacking...
f :: [[a]] -> ([a], [[a]])
f xss = foldr h ([],[]) xss where
  h [] (acc1, acc2) = (acc1, [])
  h (x:xs) (acc1, acc2) = (x:acc1, xs:acc2)

transp :: ([[a]],[[a]]) -> [[a]]
transp (xs:xss, [] ) = reverse xss
transp (xss, yss) = let (r, l) = f yss
               in transp ([r] ++ xss, l)

This seems overwrought. Let's start with getCountElements.

It's often a good idea to consider not just the problem at hand, but a more general statement of what it is you're trying to solve. In this case, you're trying to get a frequency count of each distinct element in a list. At this point don't worry about the particulars of your problem (like the fact that you're only supposed to be counting Integers with values [0..9]), frequency counts are a well-defined problem for arbitrary data sets and as such lend themselves to a generalized solution.

So think about what a general solution would look like. Well, to start, your function will be handed a [a] ->, just some list of arbitrary elements. But we need to group up which ones are equal to each other so that implies that we must have an equality constraint, (Eq a) =>. After we find and count the equal elements you'll need to return that count, so -> [Int]. But that doesn't make sense now given our generalized problem, the position of each Int in the list says nothing about which as it represents, so instead we'll return -> [(a, Int)], a list of pairs of an element and the count of that element in the original list. Putting it all together we get—

frequencyCount :: (Eq a) => [a] -> [(a, Int)]

Now we could make this work as-is†, but there's an important optimization we can make by tightening our constraints further. By first sorting the list we are given we can then easily group together identical elements, improving our asymptotic performance to O(n log n). Thus—

frequencyCount :: (Eq a, Ord a) => [a] -> [(a, Int)]

Jumping back to your version before showing the implementation, let's compare what's going on between both already.

getCountOfElements :: [Integer] -> ([Integer],Integer)

Program specifications and logic are threaded all through, from the specification of the domain to Integers, to the encoding of element identity in the position of the list in the return type, through to the inclusion of the largest count in the returned pair (a particularly odd design choice given the name of the function).

Getting back to implementing frequencyCount our implementation should be almost self-evident after having given the type. First we'll sort the elements to efficiently group the similar ones, then we'll run-length encode the list.

frequencyCount :: (Eq a, Ord a) => [a] -> [(a, Int)]
frequencyCount = runLength . sort
    where
        runLength :: (Eq a) => [a] -> [(a, Int)]
        runLength = map (\es -> (head es, length es)) . group

Et, voilà! Let's now contrast some of the properties of the getCountOfElements implementation.

Due to the use of !! for indexing, if the [Integer] we're passed is out of bounds an exception will be thrown. Better filter the whole list of elements beforehand.
count uses take, drop, and of course !!, all of which are O(n) operations on Haskell lists. That's three traversals of the index-encoded list for each element, consider the worst case performer of getCountOfElements (replicate 100000 9). This would have to be cleaned up through use of splitAt.
Using ++ in count will walk the left portion of the accumulator list again, consider again that worst case performer.

In the face of all that, getCountOfElements is an O(n) algorithm (albeit with a very large constant factor) whereas frequencyCount is O(n log n). So despite the constructive beauty of frequencyCount, to do better than getCountOfElements I'll have to steal your algorithm and use it with an appropriate Haskell data structure for constant time indexing and updating.

Data.Array from the array package provides such a data structure for us, and in fact the very first visible code example in the documentation is for a histogram function.

hist :: (Ix a, Num b) => (a,a) -> [a] -> Array a b
hist bnds is = accumArray (+) 0 bnds [(i, 1) | i<-is, inRange bnds i]

To use this version we have to provide a pair of bounds, in this question's particular case (0, 9).

count09Elements :: [Integer] -> [(Integer, Int)]
count09Elements = assocs . hist (0, 9)

In conclusion, Haskell is a land of contrasts. So, we learned today that there are many different ways to skin a cat. Depending on what you know about your problem domain many different complexity classes of algorithms may be available to you. And don't use linked lists for operations requiring constant time updating.

Appendix

† An inefficient but important frequencyCount implementation. Note that performance is O(n²) (consider e.g., frequencyCount [0..100000]).

import Data.List (partition)

frequencyCount :: (Eq a) => [a] -> [(a, Int)]
frequencyCount []       = []
frequencyCount xs@(e:_) = (head es, length es) : frequencyCount xs'
    where
        (es, xs') = partition (== e) xs


Console word wrapping I wrote a text adventure game a while back and today I looked at it to see how I did the word wrapping (to use in a new project); I'm curious if I went about it the right way. I realize I could use an array instead of a list and remove 2 dependencies from the class but wanted some other feedback as I may have missed something obvious.

    public static string GetBorderedText(string Input, int AbsoluteLength, bool splitLines = true, int Offset = -4)
    {
        string outputString = "";

        if(Input.Length > AbsoluteLength + Offset && splitLines)
        {
            List<string> words = Input.Split(' ').ToList();
            string lineConstruction = "";

            foreach (string s in words)
            {
                if (lineConstruction.Length + s.Length + 1 >= AbsoluteLength + Offset)
                {
                    outputString += "+ " + GetPaddedSubstring(lineConstruction, AbsoluteLength, Offset) + " +" + Environment.NewLine;
                    lineConstruction = "";
                    lineConstruction += s + " ";
                }
                else
                {
                    lineConstruction += s + " ";
                }
            }

            outputString += "+ " + GetPaddedSubstring(lineConstruction, AbsoluteLength, Offset) + " +";

            return outputString;
        }
        else
        {
            outputString = "+ " + GetPaddedSubstring(Input, AbsoluteLength, Offset) + " +";
            return outputString;
        }
    }

where GetPaddedSubstring  is used for getting a substring padded to a certain length.

    public static string GetPaddedSubstring(string Input, int AbsoluteLength, int Offset)
    {
        int targetLength = AbsoluteLength + Offset;
        string output = "";

        if(Input.Length > targetLength)
        {
            return Input.Substring(0, targetLength);
        }
        else
        {
            output = Input.PadRight(targetLength);

            return output;
        }
    }

Lastly, for some examples. GetBorderedText can take in something like

"This room looks sturdy, but old. Lots of storage and containers. There is a door on the far side."

And return something like this.

+ This room looks sturdy, but old. Lots of storage and containers. There is a door     +
+ on the far side.                                                                     +

The + on the side are just to create a nice border within the Console window. (Hence the -4 default on Offset)

GetBorderedText(string Input, int AbsoluteLength, bool splitLines = true, int Offset = -4)

You are very inconsistent with naming parameters. Some of them are in PascalCase while only one has the correct camelCase. This makes your code look strange and unprofessional.

if (Input.Length > AbsoluteLength + Offset && splitLines)

Conditions like this are better stored in a variable that describes what its purpose is and split into two. Consider this:

var wrapRequired = input.Length > absoluteLength + offset;
if (wrapRequired && canWrap)

input.Split(' ').ToList()

You don't need to call ToList, it's already an array and can be enumerated.

foreach (string s in words)

If your collection is called words then name the item word and not s. It's very confusing when one looks at the code and wonders what that s is.

lineConstruction.Length + word.Length + 1 >= absoluteLength + offset

The same here. You should name this condition propertly and especially the magic 1.

Other than this

you should use StringBuilder for concatenating strings in loops because it's not only faster but also easier to use.
make the border as prefix & suffix parameters instead of hardcoding them (or leftBorder & rightBorder).
use variables with stronger names and use more of them so that they document your code.

As an example of how you can apply all these suggestions here's the new code with one new method for adding the border.

public static string CreateBorderedText(string input, int maxLength, bool canWrap = true, string prefix = "+ ", string suffix = " +")
{
    const string wordSeparator = " ";

    var lines = new List<string>();
    var lineBuilder = new StringBuilder();
    var borderLength = prefix.Length + suffix.Length;
    var contentLength = maxLength - borderLength;

    var wrapRequired = input.Length > contentLength;
    if (wrapRequired && canWrap)
    {
        var words = input.Split(' ');
        foreach (var word in words)
        {
            var projectedLength = lineBuilder.Length + word.Length + wordSeparator.Length + borderLength;
            var wrap = projectedLength >= maxLength;
            if (wrap)
            {
                PadRight(lineBuilder, contentLength);
                AddBorder(lineBuilder, prefix, suffix);

                lines.Add(lineBuilder.ToString());
                lineBuilder.Clear();
            }

            lineBuilder.Append(word).Append(wordSeparator);
        }
    }
    else
    {
        lineBuilder.Append(input);
    }

    PadRight(lineBuilder, contentLength);
    AddBorder(lineBuilder, prefix, suffix);
    lines.Add(lineBuilder.ToString());

    return string.Join(Environment.NewLine, lines);
}

and the two helpers

private static void AddBorder(StringBuilder lineBuilder, string prefix, string suffix)
{
    lineBuilder
        .Insert(0, prefix)
        .Append(suffix);
}

private static void PadRight(StringBuilder lineBuilder, int targetLength)
{
    lineBuilder.Append(' ', targetLength - lineBuilder.Length);
}


Rustacious way of checking if string ends with any suffix from a selection What's the best way to check if a String ends with any of multiple suffixes in Rust?

I have a working, naive solution:

fn string_ends_with_any(s: String, suffixes: Vec<&str>) -> bool {
    for suffix in &suffixes {
        if s.ends_with(suffix) {
            return true;
        }
    }
    false
}

Note: suffixes could have varying lengths

As I am quite new to Rust, I would very much appreciate any/all suggestions about how my code can better leverage Rust idioms.

Here are some test cases I need to function pass

#[test]
fn empty_suffixes() {
    let s = String::from("5m");
    let suffixes = vec![];

    assert!(!string_ends_with_any(s, suffixes))
}

#[test]
fn empty_string_has_no_suffix() {
    let s = String::from("");
    let suffixes = vec!["txt"];

    assert!(!string_ends_with_any(s, suffixes))
}

#[test]
fn string_ends_with_first_suffix() {
    let s = String::from("foo.txt");
    let suffixes = vec!["txt", "csv"];

    assert!(string_ends_with_any(s, suffixes))
}

#[test]
fn string_ends_with_last_suffix() {
    let s = String::from("foo.csv");
    let suffixes = vec!["txt", "csv"];

    assert!(string_ends_with_any(s, suffixes))
}

#[test]
fn string_doesnt_end_with_any_suffix() {
    let s = String::from("foo.csv");
    let suffixes = vec!["txt", "tsv"];

    assert!(!string_ends_with_any(s, suffixes))
}

#[test]
fn string_ends_in_suffix_but_case_is_wrong() {
    let s = String::from("foo.csv");
    let suffixes = vec!["txt", "Csv"];

    assert!(!string_ends_with_any(s, suffixes))
}

You could use Iterator:any to write this in a succinct way:

fn string_ends_with_any(s: String, suffixes: Vec<&str>) -> bool {
    return suffixes.iter().any(|&suffix| s.ends_with(suffix));
}

Because this pattern returns true on first match of the predicate:

for suffix in &suffixes {
    if s.ends_with(suffix) {
        return true;
    }
}
false

Whether this is rustacious remains to be seen. I'm by no means an expert in Rust.


Code for adjusting linelength I wrote the following groovy function for taking a string of multiple lines (or any other character for splitting the code into parts), and bringing each line/part to a specified number of characters, cutting too long lines and filling up too short lines with a fill character:

static final String adjustLineLength(Integer length, String filler, String token, String source) {
    source
    .tokenize(token)
    .collect{ String line ->
        if(line.size() > length)
            line.substring(0, length)
        else if(line.size() < length)
            line + (filler * (length - line.size()))
        else
            line
    }
    .join(token)
}

Is that a good approach? Any suggestions how this code can be improved in concerns of making it more easy to understand and maybe (but that has not priority over beeing easy to understand) improving performance?

I think your approach is fine, and it's easy to understand. I have just a few improvement ideas:

A more natural ordering of the method parameters might be: source, token, length, filler
Instead of Integer, int should be enough and shorter
Move the closure in collect to its own method
Do you really need the static final String declaration? Why not use simply def?
Do you really need the type declarations in the signature? You could omit them, and it will be still quite clear
The brackets around the multiplication are unnecessary in

line + (filler * (length - line.size()))

Suggested implementation

Putting the above suggestions together:

def adjustLineLength = { line, length, filler ->
    if (line.size() > length) {
        line.substring(0, length)
    } else if (line.size() < length) {
        line + filler * (length - line.size())
    } else {
        line
    }
}

def adjustTextLength = { source, token, length, filler ->
    source
    .tokenize(token)
    .collect { adjustLineLength(it, length, filler) }
    .join(token)
}


How can I improve this code to display contact numbers? I am making an Address Book like application .
I have a contactType enum.

public enum ContactType
{
  //just a cut down version
  MOBILE,PHONE,FAX,EMAIL;

}

Then I have a Contact class

public class Contact
{
   private ContactType contactType;
   private String value;//e.g phone number or email
   private String areaCode;//does not apply to email or mobile
   //....getters and setters
}

Then I have a Person class

public class Person
{
    private List<Contact>contacts;
    //----Other attributes and getters and setters

}

I thought I have nailed very well :) but as it turns out probably not:(  Because

I now needs to display a list of people(list of person) and in the table along with some other things (name etc)I have these columns:

Name|......|Phone|Mobile|Fax  |Email

I now realised that I can not just loop through the list of people and display contact numbers since these contacts are in list inside every object in list of people.
In simple words as you might know by now already if I have a list of people e.g

List<Person>people ;

Then this is not a valid option

for(Person person: people)
{
  //can not get phone number for example by going
  person.getPhone();
}

but this is

for(Person person: people)
    {
      //but will have to get it from the list of contacts e.g
      List<Contact>contacts = person.getContacts();
      for(Contact contact:Contacts)
      {
         if (contact.getContactType.isPhone())
         {
            contact.getValue();
         }
      }
    }

So I have now introduced a Rowitem Class

  public class Rowitem {

        private String phone;
        private String fax;
        private String mobile;
        private String email;

        ....

     }

and populating is like this

private void populateForDisplay(Rowitem item,List<Contact> contacts)
 {
        for (Contact contact : contacts)
        {
            if (contact.getType().isEmail()) {
                item.setEmail(contact.getValue());
            }
            if (contact.getType().isFax()) {
                item.setFax(contact.getValue());
            }
            if (contact.getType().isMobile()) {
                item.setMobile(contact.getValue());
            }
            if (contact.getType().isPhone()) {
                item.setPhone(contact.getValue());
            }
        }

    }

While this approach works I just do not think it is right. Firstly I do not like the fact that I have to introduce this RowItem class and then all these if statements do not look right. How can I improve this?
The front end framework that I am using is JSF2 but probably not relevant here.

I need someone to review my approach rather than line by line code.I have read https://codereview.stackexchange.com/faq and I think my question is with the rules if not then please guide me and I will move this.Because not code problem I did not think will go to StackOverFlow

At first I would suggest to get rid of the contact type enumeration and instead derive concrete contacts from your contact class. This will increase cohesion dramatically as this way you do not need to have fields like area code in your base contact class.

Regarding your problem: You could use something like a visitor pattern to navigate the "tree" of data (Persons -> Person -> Contacts) with your Person and Contact acting as elements accepting the abstract visitor. This will give you the flexibility to walk the tree for different purposes. For your scenario you could write something like a TableVisitor that creates the table structure you want to generate.


Unit Testing- simple stack class This is my first real attempt at unit testing. This is an area I'm currently focused on improving.

I say this because I would greatly appreciate any and every improvement!

Stack code:

class Stack:
    def __init__(self):
        self.items = []
        self.size = 0

    def is_empty(self):
         return not self.items

    def get_size(self):
         return self.size

    def pop(self):
        if self.is_empty():
            raise IndexError, 'Stack Empty'
        else:
            self.size =- 1
            return self.items.pop()

    def push(self, item, *args):
        if isinstance(item, list):
            self._push_list(item)
        else:
            self.items.append(item)
            self.size +=  1

        if args:
            for a in args:
                self.items.append(a)
                self.size +=  1

    def _push_list(self, l):
        for item in l:
            self.items.append(item)
        self.size += len(l)

    def get_stack(self):
        while not self.is_empty():
            return self.pop()

    def peek(self):
        if self.size == 0:
            return None
        else:
            return self.items[self.size - 1]

Unit Test Code:

import unittest

from extras import Stack

class TestStackMethods(unittest.TestCase):

    def setUp(self):
        self.s = Stack()
        self.l = [] # a generic list to append to

    def tearDown(self):
        while not self.s.is_empty():
            self.s.pop()

    def test_test(self):
        self.assertEqual(2, (4/2))

    def test_empty_function_with_items_in_stack(self):
        self.s.push(1)
        self.failIf(self.s.is_empty())

    def test_empty_function_on_empty_stack(self):
        self.failUnless(self.s.is_empty())

    def test_pop_function_with_items_in_stack(self):
        temp = 1
        self.s.push(temp)
        x = self.s.pop()
        self.assertEqual(temp, x)

    def test_pop_function_on_empty_stack(self):
        with self.assertRaises(IndexError) as c :
            self.s.pop()
        self.assertTrue('Stack Empty' in c.exception)

    def test_peek_function_with_items_in_stack(self):
        self.s.push(1)
        self.s.push(2)
        self.assertTrue(self.s.peek() == 2)

    def test_peek_function_on_empty_stack(self):
        self.assertIs(self.s.peek(), None)

    def test_push_function_for_one_item(self):
        self.s.push(10)
        self.assertTrue(self.s.size == 1)

    def test_push_function_on_a_list_arg(self):
        args = [1,2,3,4]
        self.s.push(args)
        self.assertTrue(self.s.size, 4)

    def test_push_function_on_multiple_args(self):
        self.s.push(1,2,3,4)
        self.assertTrue(self.s.size, 4)

    def test_lifo_functionality(self):
        self.s.push(1,2,3,4)
        while not self.s.is_empty():
             self.l.append(self.s.pop())

        self.assertEqual(self.l, [4,3,2,1])

if __name__ == '__main__':
     unittest.main()

I'm just going to review the unit tests here.
Overall, it looks reasonably good.  I don't have many changes to suggest.
Drop test_test
I think you probably created this to experiment with the testing infrastructure.  It doesn't contribute to testing Stack, so it no longer needs to remain.
The tear-down is unnecessary
The Stack should not need its items removing after use, so just drop the tearDown method.  In fact, the setUp is debatable - it might be as easy to have a local s = Stack() in each test, rather than sharing a member.
Avoid deprecated methods
Replace failUnless with assertTrue, and failIf with assertFalse.
Simplest tests first
This might just be opinion, but I'd start with test_empty_function_on_empty_stack before test_empty_function_with_items_in_stack, and I'd write test_pop_function_on_empty_stack before test_pop_function_with_items_in_stack.  One reason is that when I do test-driven development, it's natural to test the empty stack functions before implementing push.
Don't use assertTrue for comparison
Here, we don't get much information if the test fails:
def test_push_function_for_one_item(self):
    self.s.push(10)
    self.assertTrue(self.s.size == 1)

Instead, we want
    self.assertEqual(1, self.s.size)

Avoid loops in tests
I'd be more explicit in the final test:
def test_lifo_functionality(self):
    self.s.push(1,2,3,4)

    self.assertEqual(4, self.s.pop())
    self.assertEqual(3, self.s.pop())
    self.assertEqual(2, self.s.pop())
    self.assertEqual(1, self.s.pop())
    self.assertTrue(self.s.is_empty())

Modified code
class TestStackMethods(unittest.TestCase):

    def setUp(self):
        self.s = Stack()

    def test_empty_function_on_empty_stack(self):
        self.assertTrue(self.s.is_empty())
        self.assertEqual(0, self.s.size)

    def test_empty_function_with_items_in_stack(self):
        self.s.push(1)
        self.assertFalse(self.s.is_empty())

    def test_pop_function_on_empty_stack(self):
        with self.assertRaises(IndexError) as c :
            self.s.pop()
        self.assertTrue('Stack Empty' in c.exception)

    def test_pop_function_with_items_in_stack(self):
        self.s.push(1)
        self.assertEqual(1, self.s.pop())

    def test_peek_function_on_empty_stack(self):
        self.assertIs(self.s.peek(), None)

    def test_peek_function_with_items_in_stack(self):
        self.s.push(1)
        self.s.push(2)
        self.assertEqual(2, self.s.peek())

    def test_push_function_for_one_item(self):
        self.s.push(10)
        self.assertEqual(1, self.s.size)

    def test_push_function_on_a_list_arg(self):
        args = [1,2,3,4]
        self.s.push(args)
        self.assertEqual(4, self.s.size)

    def test_push_function_on_multiple_args(self):
        self.s.push(1,2,3,4)
        self.assertEqual(4, self.s.size)

    def test_lifo_functionality(self):
        self.s.push(1,2,3,4)

        self.assertEqual(4, self.s.pop())
        self.assertEqual(3, self.s.pop())
        self.assertEqual(2, self.s.pop())
        self.assertEqual(1, self.s.pop())
        self.assertTrue(self.s.is_empty())


Prime number generator I have a created small program that prints a sequence of prime numbers. Based on the origin code in here.

How can I improve this code?

#include <iostream>

template <std::size_t p, std::size_t i>
struct Run
{
    static const bool mValue = ((p % i) && Run<p, i - 1>::mValue);
};

template <std::size_t p>
struct Run<p, 1>
{
    static const bool mValue = true;
};

template <std::size_t i>
struct Prime
{
    Prime<i - 1> mPrime;
    static const bool mResult = Run<i, i - 1>::mValue;

    void print()
    {
        mPrime.print();

        if (mResult)
        {
            std::cout << "prime number: " << i << '\n';
        }
    }
};

template<>
struct Prime<1>
{
    static const bool mResult = false;

    void print()
    {
    }
};

int main()
{
    Prime<30> PrimeNumbers;
    PrimeNumbers.print();
}

How can I improve this code?

The code itself isn't particularly useful. You can print the primes up to N, but what if I wanted to sum all the primes up to N? In this model, you'd have to add a sum() member function to Prime<i> and again to Prime<1>. That doesn't make for a very reusable system. So let's go for something reusable, and in C++11 to boot.

Checking primality

Run isn't a particularly good name for a metafunction that determines if something is prime or not. Furthermore it doesn't even have to be a metafunction, it can be a constexpr function:

constexpr bool is_prime(std::size_t N)
{
    if (N < 2) return false;

    for (std::size_t i=2; i*i <= N; ++i) {
        if (N%i == 0) return false;
    }
    return true;
}

Check primality for lots of numbers

Your original example wanted all primes under 30. The standard metaprogramming mechanism for getting all numbers under 30 is:

using Nums = std::make_index_sequence<31>; // std::index_sequence<0, 1, 2, ..., 30>;

Although that gives us numbers and metaprogramming is all about types. Anything that isn't a type is usually a real nuisance to deal with, so let's write something to instead give us a list of types:

template <typename T>
struct type_is { using type = T; };

template <typename...>
struct typelist { };

namespace details {
    template <std::size_t N, typename = std::make_index_sequence<N>>
    struct make_type_index_sequence;

    template <std::size_t N, std::size_t... Is>
    struct make_type_index_sequence<N, std::index_sequence<Is...>> {
        using type = typelist<std::integral_constant<std::size_t, Is>...>;
    };
}

template <std::size_t N>
using make_type_index_sequence = typename details::make_type_index_sequence<N>::type;

// typelist<size_t_<0>, size_t_<1>, ..., size_t_<30>>, where size_t_<N> is integral_constant<size_t, N>
using Nums = make_type_index_sequence<31>;

Ok, we have a typelist. Now we just need to filter it down based on a condition. To filter, we need to be able to concatenate:

template <typename... Args>
struct concat;

template <typename... Args>
using concat_t = typename concat<Args...>::type;

template <typename... A1, typename... A2, typename... Args>
struct concat<typelist<A1...>, typelist<A2...>, Args...> {
    using type = concat_t<typelist<A1..., A2...>, Args...>;
};

template <typename TL>
struct concat<TL> {
    using type = TL;
};

After which filter comes naturally:

template <typename A, typename F>
using filter_one = std::conditional_t<F::template apply<A>::value,
                                      typelist<A>,
                                      typelist<>>;

template <typename TL, typename F>
struct filter;

template <typename... Args, typename F>
struct filter<typelist<Args...>, F> {
    using type = concat_t<filter_one<Args, F>...>;
};

template <typename TL, typename F>
using filter_t = typename filter<TL, F>::type;

Metafunction classes

Something needs to be mentioned about this construction:

F::template apply<A>::value

Here F is a "metafunction class". That is, it's a type of the form:

struct MetafunctionClass {
    template <typename... Args>
    using apply = ???
};

Metaprogramming is all about convention. If everybody doesn't agree on convention, everybody's metaprograms may as well be written in different languages. The most core convention is that the "return" of a metafunction lives in a type named type. But a fairly common one is that when you need a "metafunctor", you pass something like this. The advantage of something like this is that it's always easy to create a type, but usually not so easy to create a template template (which would be the alternative).

Specifically here, we need:

struct IsPrime {
    template <typename T>
    using apply = std::integral_constant<bool, is_prime(T::value)>;
};

with which getting all the primes under 30 is:

template <size_t N>
using PrimesUnder = filter_t<make_type_index_sequence<N>, IsPrime>;

What can I do with this?

Well, anything really. Now that I made it easy to generate the primes, anything we may want to do with them is easy too. Printing them?

template <typename... Ts, typename F>
void for_each(typelist<Ts...>, F f) {
    using expander = int[];
    expander{0,
        (void(f(type_is<Ts>{})), 0)...
    };
}

for_each(PrimesUnder<30>{}, [](auto t) {
    std::cout << "prime number: " << decltype(t)::type::value << std::endl;
});

Summing them?

template <typename TL, typename F, typename Z>
using foldl_t = ??? // exercise for the reader

struct Add {
    template <typename T, typename U>
    using apply = std::integral_constant<decltype(T::value + U::value),
                      T::value + U::value
                      >;
};

template <size_t N>
using SumOfPrimesUnder = foldl_t<PrimesUnder<N>, Add, std::integral_constant<int, 0>>;

Anything else you may need to do with your primes? You have the typelist - it's all in your hands.


Creating list ViewModels in the correct way I'm using Caliburn Micro to create a WPF application.

So what I want to do here is a typical Master/Detail situation. I'm displaying a list of Users and you can can add/edit a User and save the changes back to the database.

Everything works, but I'm just not sure if I'm creating the UserViewModels in the correct way. I lay awake at night yearning for a more elegant way.

Anything else that I'm doing wrong please let me know. It greatly helps me out and I appreciate every comment I get. Even if you must call me an idiot. Many thanks in advance!

PS: I left a few implementation details out to keep it brief. But let me know if anything else would help.

public class UserWorkspaceViewModel : Conductor<UserViewModel>.Collection.OneActive
{
    private IUnitOfWork _unitOfWork;
    private UserViewModelFactory _userViewModelFactory;

    public UserWorkspaceViewModel(IUnitOfWork unitOfWork, IUserViewModelFactory userViewModelFactory)
    {
        _unitOfWork = unitOfWork;
        _userViewModelFactory = userViewModelFactory;
        LoadUsers();
    }

    public void LoadUsers()
    {
        var users = _unitOfWork.Users.GetByDepartmentId(1);
        foreach (User user in users)
        {
            // Use the factory to create the ViewModel
            UserViewModel viewModel = _userViewModelFactory.CreateInstance(user);

            // Caliburn Micro specific: add user to the screen collection
            this.Items.Add(viewModel);
        }
    }
}

Here's the UserViewModelFactory implementation:

public class UserViewModelFactory
{
    private IUnitOfWork _unitOfWork;

    public UserViewModelFactory(IUnitOfWork unitOfWork)
    {
        _unitOfWork = unitOfWork;
    }

    public UserViewModel CreateInstance(User user)
    {
        UserViewModel vm = new UserViewModel(_unitOfWork);

        // Use AutoMapper to map properties from user to VM
        Mapper.Map<User, UserViewModel>(user, vm);

        return vm;
    }
}

And finally here's the UserViewModel:

public class UserViewModel : Screen
{
    private IUnitOfWork _unitOfWork;

    #region Properties
    // Properties like FirstName, LastName, etc.
    #endregion

    public UserViewModel(IUnitOfWork unitOfWork)
    {
        _unitOfWork unitOfWork;
    }

    public void Save()
    {
        if (this.Id == 0)
        {
            User user = Mapper.Map<UserViewModel, User>(this);
            _unitOfWork.Users.Add(user);
            _unitOfWork.SaveChanges();
        }

        if (this.Id > 0)
        {
            User user = _unitOfWork.Users.GetById(this.Id);
            Mapper.Map<UserViewModel, User>(this, user);
            _unitOfWork.SaveChanges();
        }
    }
}

UserWorkspaceViewModel

private UserViewModelFactory _userViewModelFactory; instead of using the actual type use the interface like private IUserViewModelFactory _userViewModelFactory;
magic number 1 (Users.GetByDepartmentId(1);) should be expressed by a meaningful constant.
LoadUsers() method. Here IMHO you have used the var wrong. I don't think it is obvious what the right hand side of var users = _unitOfWork.Users.GetByDepartmentId(1); can be.

List<User> users = _unitOfWork.Users.GetByDepartmentId(1);
foreach (var user in users)
{

}

would be more obvious.

UserViewModel

the constructor won't compile, but I assume this is a copy & paste problem.

public UserViewModel(IUnitOfWork unitOfWork)
{
    _unitOfWork = unitOfWork;
}

Save() method

I guess it won't be possible for the Id property to become <0 so a simple else instead of another if would be better.
If the chances are high that you will need to add more code, consider to extract the adding and updating to separate methods.

General

Comments should explain why something is done. What is done should be explained by the code itself by using meaningful names for classes,methods and parameters. So comments like // Use the factory to create the ViewMode should be removed.


MySQL query to select number of fields occurring more times than x I want to find out the 'rank' of a word in any given language. A word with a rank of 1 means it's the most commonly used word in the language.
There are two MySQL tables.
document:

id: 1
language: "english"

word:

id: 2
body: "the"
document_id: 1

There are about 1 million rows in the word table. Here is a query that works, but it takes 7-10 seconds. Is there a better way to write this query?
select count(*) + 1
    from (
        select word.body, count(*) as count
        from word
        join document on word.document_id = document.id
        where document.language = 'english'
        group by word.body
    ) t
    where count > (select count
    from (
        select word.body, count(*) as count
        from word
        join document on word.document_id = document.id
        where document.language = 'english'
        group by word.body
    ) t
    where body = 'the')

Initially I was thinking of suggesting the HAVING clause, but I don't think that would help in this case, since the query selects the aggregated count and uses that for a comparison.

One option is to use a Common Table Expression (CTE) but apparently those aren't introduced until MySQL version 8. One other option is to make a View:

CREATE VIEW countEnglishWord as select word.body, count(*) as count
    from word
    join document on word.document_id = document.id
    where document.language = 'english'
    group by word.body;

and use it like this:

select count(*) + 1, body
from countEnglishWord c
where count > (select count
  from countEnglishWord t
  where body = 'the'
)

I haven't tested it on a million rows... but 3 in this sqlfiddle.


calloc, malloc, free and realloc wrappers to store the size being allocated I'm currently building a node.js addon for libcurl. Right now I'm trying to correctly use v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory to update v8 on the amount of memory being allocated by libcurl, and to do that I need to wrap the above mentioned functions that libcurl uses, using curl_global_init_mem().

I saw the following code being used here that basically does the same thing, but for another lib.

I currently have the following code (almost identical to the one above):

struct MemWrapper {
    size_t size;
    double data;
};

#define MEMWRAPPER_SIZE offsetof( MemWrapper, data )

inline void* MemWrapperToClient( MemWrapper* memWrapper ) {
    return static_cast<void*>( reinterpret_cast<char*>( memWrapper ) + MEMWRAPPER_SIZE );
}

inline MemWrapper* ClientToMemWrapper( void* client ) {
    return reinterpret_cast<MemWrapper*>( static_cast<char*>( client ) - MEMWRAPPER_SIZE );
}

void AdjustMem( ssize_t diff )
{
    Nan::AdjustExternalMemory( static_cast<int>( diff ) );
}

void* MallocCallback( size_t size )
{
    size_t totalSize = size + MEMWRAPPER_SIZE;
    MemWrapper* mem = static_cast<MemWrapper*>( malloc( totalSize ) );
    if ( !mem ) return NULL;
    mem->size = size;
    AdjustMem( totalSize );
    return MemWrapperToClient( mem );
}

void FreeCallback( void* p )
{
    if ( !p ) return;
    MemWrapper* mem = ClientToMemWrapper( p );
    ssize_t totalSize = mem->size + MEMWRAPPER_SIZE;
    AdjustMem( -totalSize );
    free( mem );
}

void* ReallocCallback( void* ptr, size_t size )
{
    if ( !ptr ) return MallocCallback( size );
    MemWrapper* mem1 = ClientToMemWrapper( ptr );
    ssize_t oldSize = mem1->size;
    MemWrapper* mem2 = static_cast<MemWrapper*>( realloc( mem1, size + MEMWRAPPER_SIZE ) );
    if ( !mem2 ) return NULL;
    mem2->size = size;
    AdjustMem( ssize_t( size ) - oldSize );
    return MemWrapperToClient( mem2 );
}

char* StrdupCallback( const char* str )
{
    size_t size = strlen( str ) + 1;
    char* res = static_cast<char*>( MallocCallback( size ) );
    if ( res ) memcpy( res, str, size );
    return res;
}

void* CallocCallback( size_t nmemb, size_t size )
{
    size_t totalSize = size + MEMWRAPPER_SIZE;
    MemWrapper* mem = static_cast<MemWrapper*>( calloc( nmemb, totalSize ) );
    if ( !mem ) return NULL;
    mem->size = nmemb * size;
    AdjustMem( nmemb * totalSize );
    return MemWrapperToClient( mem );
}

I hook it up with libcurl calling:

curl_global_init_mem( CURL_GLOBAL_ALL, MallocCallback, FreeCallback, ReallocCallback, StrdupCallback, CallocCallback );

I compiled the add-on, and from what I can see, it's working fine, but as I'm unexperienced with C++, I need to know if this is correct. What kind of bad things can happen from modifying the pointer returned by calloc, malloc and prealloc like that? Is there any way to improve it?

Check alignment.

I don;t believe you are correctly aligning your objects.

2. Realloc is wrong.

if realloc returns NULL the original pointer is not deallocated.

MemWrapper* mem2 = static_cast<MemWrapper*>( realloc( mem1, size + MEMWRAPPER_SIZE ) );
if ( !mem2 ) return NULL;

Here if realloc fails you have just leaked memory (as mem1 is still valid).

The standard usage pattern for realloc is:

 void* data = malloc(size);
 ...

 void* extra = realloc(data, newSize);
 if (extra) {
     data = extra;   // only reset data if the realloc worked.

 }


Reclassifying movies by theme Any efficient way to solve the following problem assuming data is large. I solved the problem but how can I improve the code, which will make it efficient. any suggestions?

Data:

movie_sub_themes = {
'Epic': ['Ben Hur', 'Gone With the Wind', 'Lawrence of Arabia'],
'Spy': ['James Bond', 'Salt', 'Mission: Impossible'],
'Superhero': ['The Dark Knight Trilogy', 'Hancock, Superman'],
'Gangster': ['Gangs of New York', 'City of God', 'Reservoir Dogs'],
'Fairy Tale': ['Maleficent', 'Into the Woods', 'Jack the Giant Killer'],
'Romantic':['Casablanca', 'The English Patient', 'A Walk to Remember'],
'Epic Fantasy': ['Lord of the Rings', 'Chronicles of Narnia', 'Beowulf']}

movie_themes = {
'Action': ['Epic', 'Spy', 'Superhero'],
'Crime' : ['Gangster'],
'Fantasy' : ['Fairy Tale', 'Epic Fantasy'],
'Romance' : ['Romantic']}

themes_keys = movie_themes.keys()
theme_movies_keys = movie_sub_themes.keys()

#Iterate in movie_themes
#Check movie_themes keys in movie_sub_keys
#if yes append the movie_sub_keys into the newdict
newdict = {}
for i in range(len(themes_keys)):
   a = []
   for j in range(len(movie_themes[themes_keys[i]])):
     try:
         if movie_themes[themes_keys[i]][j] in theme_movies_keys:
            a.append(movie_sub_themes[movie_themes[themes_keys[i]][j]])
     except:
         pass
   newdict[themes_keys[i]] = a

# newdict contains nested lists
# Program to unpack the nested list into single list
# Storing the value into theme_movies_data
theme_movies_data = {}
for k, v in newdict.iteritems():
    mylist_n = [j for i in v for j in i]
    theme_movies_data[k] = dict.fromkeys(mylist_n).keys()

print (theme_movies_data)

Output:

{'Action': ['Gone With the Wind', 'Ben Hur','Hancock, Superman','Mission: Impossible','James Bond','Lawrence of Arabia','Salt','The Dark Knight Trilogy'],
 'Crime': ['City of God', 'Reservoir Dogs', 'Gangs of New York'],
 'Fantasy': ['Jack the Giant Killer','Beowulf','Into the Woods','Maleficent','Lord of the Rings','Chronicles of Narnia'],
 'Romance': ['The English Patient', 'A Walk to Remember', 'Casablanca']}

Apologies for not properly commenting the code.

I am more concern about the running time.

theme_movies_data and newdict are bad variable names, change them to ones easier to read. This will reduce the amount of comments you need in your code.
You can simplify your code if you stop using range and use dict.iteritems more.
You shouldn't need your try. You would know this if you use range less.
You don't need dict.fromkeys(mylist_n).keys() it's just useless.

new_dict = {}
for key, themes in movie_themes.items():
    a = []
    for theme in themes:
        if theme in movie_sub_themes:
            a.append(movie_sub_themes[theme])
    new_dict[key] = a

theme_movies = {}
for key, movie_groups in new_dict.iteritems():
    theme_movies_data[key] = [
        movie
        for movies in movie_groups
        for movie in movies
    ]

print(theme_movies)

You can remove the need for the second loop if you use a.extend.
You can change the creation of a to a comprehension.
You can change the creation of theme_movies to a dictionary comprehension.

theme_movies = {
    key: sum(
        movie_sub_themes.get(theme, [])
        for theme in themes
    )
    for key, themes in movie_themes.iteritems()
}

print(theme_movies)

Alternately if you don't like sum:

theme_movies = {
    key: [
        movie
        for theme in themes
        for movie in movie_sub_themes.get(theme, [])
    ]
    for key, themes in movie_themes.iteritems()
}

print(theme_movies)


Generating CSV strings for various 3rd party utilities I'm generating CSV strings for various 3rd party utilities and this section of code gets repeated in many classes. Is there a better way to generate this string?

public override string CsvString()
{
    return (
        string.Format("\u0022{0}\u0022,\u0022{1}\u0022,\u0022{2}\u0022,\u0022{3}\u0022,\u0022{4}\u0022,\u0022{5}\u0022,\u0022{6}\u0022,\u0022{7}\u0022,\u0022{8}\u0022,\u0022{9}\u0022,\u0022{10}\u0022,\u0022{11}\u0022,\u0022{12}\u0022,\u0022{13}\u0022",

        this.BlockType,         //  1, A_NAME
        this.Tag,               //  2, A_TAG
        this.Description,       //  3, A_DESC
        this.InitialScan,       //  4, A_ISCAN
        this.AutoManual,        //  5, A_SCAN
        this.ScanTime,          //  6, A_SCANT
        this.IoDevice,          //  7, A_IODV
        this.IoAddress,         //  8, A_IOAD
        this.InitialAmStatus,   //  9, A_IAM
        this.AlarmPriority,     // 10, A_PRI
        this.AlarmEnable,       // 11, A_ENAB
        this.EnableOutput,      // 12, A_EOUT
        this.HistDescription,   // 13, A_HIST_DESC
        this.SecurityArea1      // 14, A_SECURITYAREA1
    ));
}

I doubt you'll find a way of not listing all those properties without using reflection, but the following helps to eliminate that huge format string which is likely to become the source of bugs.

var properties = new Object[]
{
    this.BlockType,         //  1, A_NAME
    this.Tag,               //  2, A_TAG
    this.Description,       //  3, A_DESC
    this.InitialScan,       //  4, A_ISCAN
    this.AutoManual,        //  5, A_SCAN
    this.ScanTime,          //  6, A_SCANT
    this.IoDevice,          //  7, A_IODV
    this.IoAddress,         //  8, A_IOAD
    this.InitialAmStatus,   //  9, A_IAM
    this.AlarmPriority,     // 10, A_PRI
    this.AlarmEnable,       // 11, A_ENAB
    this.EnableOutput,      // 12, A_EOUT
    this.HistDescription,   // 13, A_HIST_DESC
    this.SecurityArea1      // 14, A_SECURITYAREA1
}.Select(x => String.Format("\u0022{0}\u0022", x));

return String.Join(",", properties);

A couple of things to note:

This is hardly an efficient way of doing it, but offers fairly maintainable code. If you have an extra property, just add it to the array.

This will only work in .NET 4.0. In earlier versions, you'll have to call ToArray() after that call to Select.


Excel Mapping Module I wrote this bit of python intending it to become some module to replace excel mapping programs like MapForce.  Right now it only does columns to columns. So the number of rows in always equals the number of rows out.

To declare a map, use a python defaultdict(list) object

from collections import defaultdict

demoCmd = defaultdict(list)
demoCmd = {
    'A': [mapOper.mapVal, 'Hello, World'],
    'B': [mapOper.mapSum, 0, 1, 2],
    'C': [mapOper.mapAss, 3, 4, 5],
    'D': [mapOper.mapProd, [mapOper.mapSum, 1, 0], 0, 1],
    'E': 5
    }

The keys are the destination columns name or index.  Integers can be used as keys but they must be zero offset.  Strings are converted to zero offset integers. The Lists are the functions mapping to that column.  It's in LISP style [function arg1, arg2, etc], where integers are column indexes (zero offset) and strings are raw values. And you can nest the lists like in 'D'.

mapOper is just a file containing some commonly used functions.  Some of them I don't think I need to include eg

def mapSum(*valueList):
    filter(None, valueList)
    return sum(valueList)

Anyways the main engine is this

def evalOpList(opList, fromSheet, row):
    if type(opList) is str:  # treat arg as a value
        return opList
    elif type(opList) is int:  # treat arg as index
        return fromSheet.cell(row, opList).value
    else:  # its another function
        args = list()
        itOpList = iter(opList)
        next(itOpList)
        args = [evalOpList(it, fromSheet, row) for it in itOpList]
        return opList[0](*args)

def interpColMap(mapCmd, fmSheet, toSheet, rTop=0, rBot=0, rToTop=0):
    if rBot < 0:
        rBot = fmSheet.nrows
    MapCmdConvert(mapCmd)
    assert rTop < rBot and rToTop >= 0
    for row in range(rTop, rBot):
        for key in list(mapCmd.keys()):
            toSheet.write(row, key, evalOpList(mapCmd[key], fmSheet, row))

And you call it like this

colMapper.interpColMap(demoCmd, fws, tws)

Where fws is an worksheet from xlrd to map from and tws is a workbook from xlwt to map to.

I have already tested it some and it works.  It's just that coming from C++ this seems very odd style.  Only two function make up my program and no classes.  I also have the problem of having all ints be indexes and all strs be values.  This means I can't do += 1 to a column without doing [int, '1'].  And I don't even know if that would work.

It's just a very unique style and I need to know if it's too convoluted or not.

Generally camelCase is used for both locals and functions.
Both PEP 8 and Google style guide recommend underscore lower_caps for both.
camelCase isn't wrong in python, it's just sticking with global standards in more right.

Also some names could be more descriptive.
rBot could be bottom_row.
operator_list is more verbose than opList but it's name is now exactly what it is and there is no question of the programmers intention of the variable.

Initializing the demo command is done incorrectly despite it being functional code.
It shows a lack of understanding of how variables work in python.

demoCmd = defaultdict(list)
demoCmd = {
    'A': [mapOper.mapVal, 'Hello, World'],
    'B': [mapOper.mapSum, 0, 1, 2],
    'C': [mapOper.mapAss, 3, 4, 5],
    'D': [mapOper.mapProd, [mapOper.mapSum, 1, 0], 0, 1],
    'E': 5
    }

Variables work differently in python than in C-ish languages.
There are better explanations of it that I will link to later,
but for now this will suffice:
Variables in python are essentially C-pointers that can point to any object,
which is everything in python.

demoCmd = defaultdict(list)

creates a new defaultdict(list) and assigns demoCmd to it.

demoCmd = { ... }

was intended to modify the data in the defaultdict object, demoCmd.
However it reassigns the variable demoCmd to a new dict object ({} is equivalent to dict()).
This wasn't a problem because demoCmd never needed to be a defaultdict object in the first place.
However, if it needed to be, the data could be entered using member functions or operators like so

demoCmd = defaultdict(list)
demoCmd.add('A': [1, 2, 3])
demoCmd['B'] = [4, 5, 6]

Type checking with isinstance() is preferred over type() as isinstance() will match the specified class and all of it's sub classes,
while type() will only match the exact class.
More here.

Ergo,

def evalOpList(opList, fromSheet, row):
    if type(opList) is str:  # treat arg as a value
        return opList
    elif type(opList) is int:  # treat arg as index
        return fromSheet.cell(row, opList).value
    # ...

is more flexible as,

def evalOpList(opList, fromSheet, row):
    if isinstance(opList, str):  # treat arg as a value
        return opList
    elif isinstance(opList, int):  # treat arg as index
        return fromSheet.cell(row, opList).value
    # ...

The command dictionary has the structure of {key : [func , arg1, arg2, arg3, ...]}.
Separating the arguments from the function as a tuple,

{key: (func, (arg1, arg2, arg3, ...))}

may seem more verbose but it will make extracting the pair simpler,

else:  # its another function
    func, arg_list = operator_tuple
    args = [evalOpList(arg, fromSheet, row) for arg in arg_list]
    return func(*args)

Allowing nesting of functions in the map commands creates an unnecessary complexity to the code.
Dropping that ability allows evalOplist to be split into smaller functions.

def evaluate_command(command, from_sheet, row):
    if isinstance(command, (tuple, list)):
        return evaluate_func(func, val_list, from_sheet, row)
    else:
        evaluate_value(command, from_sheet, row)

def evaluate_value(val, from_sheet, row):
    if isinstance(val, int):
        return from_sheet.cell(row, val).value
    else:
        return command

def evaluate_func(func, val_list, from_sheet, row):
    args = [evaluate_value(val, from_sheet, row) for val in val_list]
    return func(*args)

Two problems still exist:

evaluate_value is not recursing into any sequences to replace indexes, which is needed for functions like sum.
evaluate_command is unnecessarily re-interpreting items in the mapping command for each row of the sheet.
Commands could be pre-emptively interpreted into functions that take the row values and return a value.

I could walk readers through how to completely redesign the structure to pre-interpret the commands, but that seems out of codereveiw's scope.  See here and here for those solutions.


Python - checking if phrase appears on a list I have a big file with a lot of "phrases", like:

  sayndz zfxlkl attjtww cti sokkmty brx fhh suelqbp xmuf znkhaes pggrlp

  zia znkhaes znkhaes nti rxr bogebb zdwrin sryookh unrudn zrkz jxhrdo

  gctlyz bssqn wbmdc rigc zketu ketichh enkixg bmdwc stnsdf jnz mqovwg

  ixgken flawt cpott xth ucwgg xce jcubx wvl qsysa nlg qovcqn zxcz

  vojsno nqoqvc hnf gqewlkd uevax vuna fxjkbll vfge qrzf phwuf ligf xgen

  vkig elptd njdm gvqiu epfzsvk urbltg dqg sfpku viwihi fje umdkwvi

  ejzhzj qrbl sfpku sad nawnow ksnku nzhj mfudick ueaa jnhz kpy pzk

What I need to do is check if any 'phrase' didn't appear twice in a line and count lines where it didn't

def check(phrase):
    phrases = phrase.split()
    for i in range(len(phrases)):
        phrase_tmp = phrases.pop(0)
        if phrase_tmp in phrases:
            return False
        phrases.append(phrase_tmp)
    return True

with open('file_with_phrases.txt', 'r') as file:
    counter = 0
    for line in file:
        if check(line):
            counter += 1
    print(counter)

Can you tell me if I did it all good?

Clarifying terms makes both code and documentation easier to understand.  You use the word "phrase" (which usually refers to an arbitrary group of words) to refer to both a line (one of a set of linebreak-delimited strings) and a word (one of a set of whitespace-delimited strings).  If you're talking about a single word, say "word".  :)
Give your function a descriptive name!  Maybe has_unique_words?
Type annotations and a docstring also make functions easier to understand.

Here's what we've got so far:

def has_unique_words(phrase: str) -> bool:
    """Checks whether any (whitespace-delimited) words in the phrase are repeated.
    Returns True only if all words are unique."""
    words = phrase.split()

Mutating an iterable as you iterate over it is an easy way to confuse yourself, and it's usually better to iterate over the elements of an list rather than the range of its indices (although because you're mutating it that might not work well here).  It might be better to find a solution that doesn't involve mutating the list.  Here are a few possibilities that are all simpler alternatives to popping/searching/re-inserting:

# Count the number of occurrences of each word.
# If each occurs exactly once, all words are unique.
word_counts: Dict[str, int] = defaultdict(int)
for word in words:
    word_counts[word] += 1
return all(count == 1 for count in word_counts.values())

# Iterate through each word and check whether we've already seen it.
seen_words: Set[str] = set()  # this could be a list, but a set is faster to check
for word in words:
    if word in seen_words:
        return False
    seen_words.add(word)
return True

This one-line check is what I would do:

# Build a set (which guarantees uniqueness) out of the list of words.
# If the number of elements is the same, all the list elements are unique.
return len(set(words)) == len(words)

In the interest of brevity I'd use sum and a comprehension to build the count rather than a named variable:

with open('file_with_phrases.txt', 'r') as file:
    print(sum(1 for line in file if has_unique_words(line)))


fixed size memory allocator interface without fragmentation on freeing the memory I was trying to write a memory pool interface which:

Grants memory of fixed size chunks in linked list style fashion.

On returning the memory there is no risk of memory fragmentation.
//mem_struct.h
#ifndef MEM_STRUCT_H
#define MEM_STRUCT_H

struct mem_struct
{
   char arr[4];
   struct mem_struct *next;
};

#endif

Now using above structure the interface will grant memory to the user (the function which demands memory):
//mem_allocator.c
#include<stdlib.h>
#include<string.h>
#include"mem_struct.h"
#define total 10     //max no. of fixed size memory blocks initialized at once in a pool

unsigned int total_pool_size=0;   //total bytes of memory in pool available
struct mem_struct *head=NULL;     //first node of memory pool

/* function to initialize memory pool of max total no. of memory blocks or less if memory not
   available and add newly allocated blocks to existing memory pool blocks */
int mem_pool_init()
{
   struct mem_struct *temp;
   unsigned short count = 0; //no. of memory blocks/chunks successfully allocated and added to existing memory pool

   while(count < total)
   {
      temp = calloc(1,sizeof(*temp));
      if(temp)
      {
         temp->next = head;
         head = temp;
         total_pool_size += sizeof(temp->arr);
         count++;
      }
      else
         return 1;
   }
   return 0;
}

//function which grants memory in bytes to the user request
void* mem_grant(unsigned int siz)
{
    struct mem_struct *temp_head = NULL;  //pointer to first block of memory to be returned to the requesting function
    if(siz > 0)
    {
       //This loop will go on till there is sufficient memory in the memory pool to allocate memory of size - siz or the CPU is left with no memory
       while(total_pool_size < siz)
           if(mem_pool_init())
              return NULL;

       struct mem_struct *temp = head;
       unsigned int total_nodes = 0;   //total no of nodes of memory to be returned to the requesting function

       if(total_pool_size >= siz)
       {
          temp_head = head;

          if(siz%sizeof(temp_head->arr) > 0) //to determine no. of memory blocks to be returned to the user request
             total_nodes = siz/sizeof(temp_head->arr) + 1;
          else
             total_nodes = siz/sizeof(temp_head->arr);

          while(total_nodes)
          {
              temp = temp->next;
              --total_nodes;
              total_pool_size = total_pool_size - sizeof(temp->arr);
          }
          head = temp->next;
          temp->next = NULL;
       }
   }
   return (void *)temp_head;
}

//function to add the memory earlier allocated back to the memory block
void free_mem(void *temp_head) //temp_head points to the first byte of the memory allocated to the user using mem_grant
{
    if(temp_head)
    {
        struct mem_struct *temp = *temp_head;

        while(temp)
        {
           memset(temp,0,sizeof(temp->arr));

           if(!temp->next)
           {
               temp->next = head;
               break;
           }
           total_pool_size += sizeof(temp->arr);
        }
        head = temp_head;
    }
}

Here is the main function:
//main.c
#include<stdio.h>

extern void* mem_grant(unsigned int);
extern void  free_mem(void*);
extern unsigned int total_pool_size;

int main()
{
    void* ptr = mem_grant(42);
    if(ptr)
        printf("total_pool_size after grant %u\n", total_pool_size);
    free_mem(ptr);
    printf("total_pool_size after free %u\n", total_pool_size);
    return 0;
}

I am new to this part of programming and I want to know how can I improve as I don't have any reference material to lookup. Could someone please review this and tell me how this design is?

Thoughts as I read through your post:

  On returning the memory there is no risk of memory fragmentation

I'm not sure what you mean.  Do you mean that all the memory will be free i.e. not leaked?  I don't see anything related to memory fragmentation.

char arr[4];

I don't know your use case, but 4 bytes is very small for a modern computer.  You could easily spend 100 times longer dereferencing a pointer than reading 4 bytes.  Are you sure a linked list is the right choice here?

struct mem_struct *head=NULL;

If someone else makes a global variable named head in another .c file, then you will get a linker error because of duplicate symbols.  If you have to use a global variable, you should name it something distinctive like

struct mem_struct *global_mem_struct_head=NULL;

But a better idea is not to use a global variable at all, and pass a mem_struct* argument to each function.

//max no. of fixed size memory blocks initialized at once in a pool
#define total 10

How about this:

static const int max_init_memory_blocks = 10;

Now you effectively get the comment every time you use the variable, you get type checking, and even though this is a global, it's invisible outside of this translation unit/this file.

That said, you've done a good job adding comments throughout your code.

int mem_pool_init()

This function does initialization a fixed number of times.  Can you use a for loop?

calloc

If calloc succeeds and then fails, you'll leak the first allocations.

void* mem_grant

Why does this return void*?!  How is the user supposed to know they can only use the first few bytes?!

   while(total_pool_size < siz)   //insufficient memory in pool
       if(mem_pool_init())
          return NULL;

Isn't this an infinite loop?

I'm going to stop here.  I think you would find a lot of these mistakes with some very simple test cases.  Then you can post an improved version.


Fast counting of interactions between groups given users interactions and groups assignments Given:

An array c that contains the group assignment of every user (c[i]=4 indicates that user i belongs to group 4)
A matrix y of 0 and 1 that indicates whether user i interacted with user j (y[i][j] = 1 if i interacted with j, 0 otherwise)

What is the fastest way to count the number of interactions and lack of interactions between every pair of groups?

This is what I have:

Solution 1:

This is what I have:

import numpy as np

def test1(y, c):

    N = len(c) # number of users
    C = len(set(c)) # number of clusters

    # Final counts matrices
    I = np.zeros((C,C))
    notI = np.zeros((C,C))

    # Loop every row i/ column j, look at the groups where the users belong
    # and update I accordingly
    for i in range(N):
        for j in range(N):
            if y[i][j] == 1:
                I[c[i]][c[j]] += 1
            else:
                notI[c[i]][c[j]] += 1

    return I, notI

timeit:

nusers = 100
nclusters = 4
y = np.random.random_integers(0, 1, (nusers,nusers))
c = np.random.choice(nclusters, nusers)

%timeit test1(y,c)

10 loops, best of 3: 42 ms per loop

Solution 2:

def test2(y, c):

    N = len(c)
    C = len(set(c))
    I = np.zeros((C,C))
    notI = np.zeros((C,C))

    for i, jj in enumerate(y):
        for j, value in enumerate(jj):
            if value:
                I[c[i]][c[j]] += 1
            else:
                notI[c[i]][c[j]] += 1

    return I, notI

timeit:
10 loops, best of 3: 30.8 ms per loop

Re-formulating the problem into a matrix multiplication one speeds up the algorithm very much.

Algebra:

Create a matrix G of n_groups x n_users such that every column is a group mask, that is, all users (columns) that belong to that group (row) are flagged are 1 and 0 otherwise. If Y is the matrix of interactions, then the interactions between every pair of groups can be obtained by the following equation:

$$I = GYG^T$$

Code:

The code is:

import numpy as np
from numpy import dot

def test3(y, c):

    N = len(c)
    C = len(set(c))
    noty = np.mod(y+1,2)

    # create C group masks of 1 (if user is belongs to group) and 0.
    G = np.zeros((C,N))
    for group in range(C):
        G[group] = [1 if c_== group else 0 for c_ in c]

    # Groups (src) * interaction_matrix * Groups_transposed (dst)
    # Every row of Groups matrix is the mask of a group
    return dot(dot(G,y),G.T), dot(dot(G,noty),G.T)

1000 loops, best of 3: 810 µs per loop