Resolving multiple "paths" in nested attributes I need to resolve "multiple paths" specified like so:

p1 = 'item.element.[compact|fullsize].documents.[note|invoice]'

A list like this is expected:

['item.element.compact.documents.note', 'item.element.fullsize.documents.note',
 'item.element.compact.documents.invoice', 'item.element.fullsize.documents.invoice']

Code:

def resolve_paths(path):
    parts = path.split('.')
    depth = len(parts)
    new_paths = []
    for level, part in enumerate(parts):
        mult_branches = re.findall(r'\[(\w+)(?:\|(\w+))*\]', part)
        if mult_branches:
            mult_branches = flatten_iterable(mult_branches)
            for branch in mult_branches:
                interm_path = '.'.join(parts[:level] + [branch] + parts[level+1:])
                new_paths.extend(resolve_paths(interm_path))
            return new_paths
        elif level == depth - 1:
            new_paths.append(path)
    return new_paths

Several tests I wrote for this function pass, but I'm not entirely happy with this solution, it's somewhat convoluted. Better solutions? Simplifications?

What do you think this will return for part = '[compact|fullsize|x|y]'

re.findall(r'\[(\w+)(?:\|(\w+))*\]', part)

It will give:

[('compact', 'y')]

Because, when a group matches multiple times, the last match overwrites previous matches, as per the docs.
If you want to support multiple patterns, I'm afraid you will have to do in two steps: matching and then splitting.

The part I don't like about this is that you split path to parts,
then when you need to recurse,
you re-join the parts again,
and in the recursion step it will be split again, and so on.
That's a lot of splitting and joining and iterations.
The recursive logic can be also tricky to understand.

Here's an alternative, without using recursion and unnecessary splitting, joining, iteration:

from collections import deque

def resolve_paths(path):
    parts = deque(path.split('.'))
    paths = [[]]

    while parts:
        part = parts.popleft()
        branches = re.findall(r'\[(\w+)(?:\|(\w+))*\]', part)
        if branches:
            orig_paths = paths[:]
            paths = []
            for branch in branches[0]:
                for path in orig_paths:
                    paths.append(path + [branch])
        else:
            for path in paths:
                path.append(part)

    return ['.'.join(path) for path in paths]


Symbolic derivative in C# I have translated the following F# code to C# and would appreciate constructive criticism. This code computes the symbolic derivative of the expression f(x)=x³-x-1:

type Expr =
  | Int of int
  | Var of string
  | Add of Expr * Expr
  | Mul of Expr * Expr

let rec d f x =
  match f with
  | Var y when x=y -> Int 1
  | Int _ | Var _ -> Int 0
  | Add(f, g) -> Add(d f x, d g x)
  | Mul(f, g) -> Add(Mul(f, d g x), Mul(g, d f x))

let f =
  let x = Var "x"
  Add(Add(Mul(x, Mul(x, x)), Mul(Int -1, x)), Int -1)

d f "x"

My translation from F# to C# fragments the d function into member functions in classes that implement an Expr interface and adds a hand-written structural pretty printers:

using System;

public interface Expr
{
    Expr d(string x);
}

public class Int : Expr
{
    int n;

    public Int(int m)
    {
        n = m;
    }

    public int Value
    {
        get { return n; }
    }

    public Expr d(string x)
    {
        return new Int(0);
    }

    public override string ToString()
    {
        return "Int " + n.ToString();
    }
}

public class Var : Expr
{
    string x;

    public Var(string y)
    {
        x = y;
    }

    public string Value
    {
        get { return x; }
    }

    public Expr d(string y)
    {
        return (x == y ? new Int(1) : new Int(0));
    }

    public override string ToString()
    {
        return "Var \"" + x + "\"";
    }
}

public class Add : Expr
{
    Expr f, g;

    public Add(Expr a, Expr b)
    {
        f = a;
        g = b;
    }

    public Tuple<Expr, Expr> Value
    {
        get { return Tuple.Create(f, g); }
    }

    public Expr d(string y)
    {
        return new Add(f.d(y), g.d(y));
    }

    public override string ToString()
    {
        return "Add(" + f.ToString() + ", " + g.ToString() + ")";
    }
}

class Mul : Expr
{
    Expr f, g;

    public Mul(Expr a, Expr b)
    {
        f = a;
        g = b;
    }

    public Tuple<Expr, Expr> Value
    {
        get { return Tuple.Create(f, g); }
    }

    public Expr d(string y)
    {
        return new Add(new Mul(f, g.d(y)), new Mul(g, f.d(y)));
    }

    public override string ToString()
    {
        return "Mul(" + f.ToString() + ", " + g.ToString() + ")";
    }
}

class SymbolicDerivative
{
    static void Main(string[] args)
    {
        var x = new Var("x");
        var f = new Add(new Add(new Mul(x, new Mul(x, x)),
                                new Mul(new Int(-1), x)),
                        new Int(-1));
        Console.WriteLine("{0}", f.d("x").ToString());
    }
}

I have also written the derivative function externally to the class hierarchy using the is operator to inspect the type of a given value but I chose this code because I believe it is more idiomatic OOP style.

First, there are some code naming and formatting standards in C# that you should follow:

Interfaces begin with I
Method names are Pascal-cased

And here are a small smattering of my personal standards:

Specify access modifiers at all times (source code are for humans to read)
Declare non-changing fields with readonly (primarily for intent,
but there is supposed performance advantages)
Declare non-inheritable classes with sealed (again, for intent, but there could be
performance advantages)
Use this. as a prefix for class members

Simple matters:

Test for null where you would not want to have null objects (i.e. in the constructor)
Instead of returning a new Int(0) each time in D(), have it be a pre-initialized static member.
I'm thinking the same for new Int(1)? Your mileage may vary.

All in all, I really like your approach as loosely-coupled OOP using injection.

Given those, I would lightly refactor as such:

namespace SymbolicDerivative
{
    using System;

    public interface IExpr
    {
        IExpr D(string x);
    }

    public sealed class Int : IExpr
    {
        private static readonly IExpr intZero = new Int(0);

        private static readonly IExpr intOne = new Int(1);

        private readonly int n;

        public Int(int m)
        {
            this.n = m;
        }

        public static IExpr IntZero
        {
            get { return intZero; }
        }

        public static IExpr IntOne
        {
            get { return intOne; }
        }

        public int Value
        {
            get { return this.n; }
        }

        public IExpr D(string x)
        {
            return intZero;
        }

        public override string ToString()
        {
            return "Int " + this.n;
        }
    }

    public sealed class Var : IExpr
    {
        private readonly string x;

        public Var(string y)
        {
            if (y == null)
            {
                throw new ArgumentNullException("y");
            }

            this.x = y;
        }

        public string Value
        {
            get { return this.x; }
        }

        public IExpr D(string y)
        {
            return this.x == y ? Int.IntOne : Int.IntZero;
        }

        public override string ToString()
        {
            return "Var \"" + this.x + "\"";
        }
    }

    public sealed class Add : IExpr
    {
        private readonly IExpr f;

        private readonly IExpr g;

        public Add(IExpr a, IExpr b)
        {
            if (a == null)
            {
                throw new ArgumentNullException("a");
            }

            if (b == null)
            {
                throw new ArgumentNullException("b");
            }

            this.f = a;
            this.g = b;
        }

        public Tuple<IExpr, IExpr> Value
        {
            get { return Tuple.Create(this.f, this.g); }
        }

        public IExpr D(string y)
        {
            return new Add(this.f.D(y), this.g.D(y));
        }

        public override string ToString()
        {
            return "Add(" + this.f + ", " + this.g + ")";
        }
    }

    public sealed class Mul : IExpr
    {
        private readonly IExpr f;

        private readonly IExpr g;

        public Mul(IExpr a, IExpr b)
        {
            if (a == null)
            {
                throw new ArgumentNullException("a");
            }

            if (b == null)
            {
                throw new ArgumentNullException("b");
            }

            this.f = a;
            this.g = b;
        }

        public Tuple<IExpr, IExpr> Value
        {
            get { return Tuple.Create(this.f, this.g); }
        }

        public IExpr D(string y)
        {
            return new Add(new Mul(this.f, this.g.D(y)), new Mul(this.g, this.f.D(y)));
        }

        public override string ToString()
        {
            return "Mul(" + this.f + ", " + this.g + ")";
        }
    }

    internal static class SymbolicDerivative
    {
        private static void Main()
        {
            var x = new Var("x");
            var f = new Add(new Add(new Mul(x, new Mul(x, x)), new Mul(new Int(-1), x)), new Int(-1));

            Console.WriteLine("{0}", f.D("x"));
            Console.ReadLine();
        }
    }
}


Reversing strings inside parentheses How can I optimize this algorithm to reverse strings inside parentheses?

String reverseParentheses(String s) {
    String tmpCh = new String("");
    String tmpChRe = new String("");
    String tmp = new String("");
    int l = s.length();
    int n = 0;
    int j =0;
    for(int i = 0;i<l;i++){
            if(s.charAt(i)=='(')
                    n++;
    }
    int T[] = new int[n];
    for(int i=0;i<l;i++){
            if(s.charAt(i)=='('){
                    T[j]=i;
                    j++;
            }
    }
    j=0;
    while(n>0){
            j = T[n-1] + 1;
            while(s.charAt(j)!=')'){
                    tmpCh = tmpCh + s.charAt(j);
                    j++;
            }
            for(int q=tmpCh.length()-1;q>=0;q--)
                    tmpChRe = tmpChRe + tmpCh.charAt(q);
            tmp = s.substring(0,T[n-1]) + tmpChRe + s.substring(T[n-1]+tmpChRe.length()+2);
            s = tmp;
            n--;
            tmp = "";
            tmpCh = "";
            tmpChRe = "";
    }
    return s ;
}

I consider your solution to be hard to read, and also inefficient.

Readability

String tmpCh = new String("");
String tmpChRe = new String("");
String tmp = new String("");
int l = s.length();
int n = 0;
int j =0;

Of the six variables declared at the top, the only one whose meaning is obvious is l — and since you use l in only one place, I wouldn't even bother making a variable for it.

Your variable naming is problematic.  "tmp" is always meaningless as part of a local variable name; local variables are always, by definition, temporary.  Furthermore, a better way to indicate the temporary nature of these variables is to place them in the proper scope.  Instead of resetting them to empty strings at the end of your while(n>0) loop, declare and initialize them at the top of the loop, so that they are properly scoped:

// Useless assignment.  It gets reassigned right away inside the loop.
// j = 0;

// You only ever use n-1, then n is decremented at the end of the loop.
// So you might as well decrement it right away.
while (n-- > 0) {
    // Formerly j
    int parenStart = T[n] + 1;
    int parenEnd = s.indexOf(')', parenStart);

    // Formerly tmpCh
    String contents = s.substring(parenStart, parenEnd);

    // No need for tmpChRe.  No need for tmp.
    s = s.substring(0, T[n]) +
        new StringBuilder(contents).reverse() +
        s.substring(T[n] + contents.length() + 2);
}
return s;

Please put some spaces here:

for(int i=0;i<l;i++){

This would be more conventional and easier to read:

for (int i = 0; i < l; i++) {

Efficiency

Strings in Java are immutable.  Any time you concatenate strings using a + b, the compiler actually writes new StringBuilder(a).append(b).toString();.  That creates a lot of garbage, and it also involves copying the entire contents of both strings.  So, when you loops like this, which work build strings one character at a time, are hugely inefficient!

while(s.charAt(j)!=')'){
        tmpCh = tmpCh + s.charAt(j);
        j++;
}
for(int q=tmpCh.length()-1;q>=0;q--)
        tmpChRe = tmpChRe + tmpCh.charAt(q);

If you need to build a string by repeatedly appending characters, create one StringBuilder and append to it.  Note that StringBuilder also has a .reverse() method that would be helpful.

Suggested solution

This solution performs all of the manipulation within one char[] array, so there is hardly any new memory allocated.  It also validates that the input string's parentheses are properly matched; your code would either ignore mismatches or crash with an ArrayIndexOutOfBoundsException.

public static String reverseParentheses(String s) {
    char[] chars = s.toCharArray();
    int[] openPosStack = new int[chars.length];
    int openPosTop = -1;

    for (int i = 0; i < chars.length; i++) {
        switch (chars[i]) {
          case '(':
            // Push the index of the open parenthesis onto the stack
            openPosStack[++openPosTop] = i;
            break;
          case ')':
            // Reverse substring from the matching open parenthesis to here
            if (openPosTop < 0) {
                throw new IllegalArgumentException("Parenthesis mismatch");
            }
            int a, b;
            for (a = openPosStack[openPosTop--], b = i; a < b; a++, b--) {
                char swap = chars[a];
                chars[a] = chars[b];
                chars[b] = swap;
            }
        }
    }
    if (openPosTop >= 0) {
        throw new IllegalArgumentException("Parenthesis mismatch");
    }

    // Remove parentheses from output
    int o = 0;
    for (int i = 0; i < chars.length; i++) {
        switch (chars[i]) {
          case '(': case ')':
            break;
          default:
            chars[o++] = chars[i];
        }
    }
    return new String(chars, 0, o);
}


Basic queue design and implementation In general, I'd love some comments on this basic queue using dynamic allocation.
In particular, I want comments about whether this queue is good in embedded systems, for example say a a sensor network gateway, which has to queue up packets to be forwarded on say a UART.
I hear normally that mallocs are a bad idea for embedded, so what are the pitfalls in such a scenario?
One thing is, that malloc could return that it was unable to allocate the required memory. But what I thought was that if I did the calculation and I was reasonably certain that I definitely have MAX_QSIZE amount of memory available for the MAX_DATSIZE then realistically I shouldn't be able to see malloc fail. Is that a correct assumption?
I am rewriting the same code for a version with static array and no mallocs as well, so I'll try to analyze how both behave.
Thank you for your time.
#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <stdlib.h>

#define MAX_QSIZE   100
#define MAX_DATSIZE     128

struct node{
    void *data;
    struct node *next;
};
typedef struct node node_t;

typedef struct queue_t {
    node_t *head;
    node_t *tail;
    int size;
} queue_t;

queue_t *q_init(void)
{
    queue_t *q = malloc(sizeof (*q));
    q->size = 0;
    q->head = q->tail = NULL;
}

int q_push(queue_t *q, const void *pdata, int bsize)
{
    if (q == NULL)
        return -1;
    if (q->size == MAX_QSIZE)
        return -2;
    if (bsize > MAX_DATSIZE)
        return -3;

    node_t *new = malloc(sizeof (*new));
    new->next = NULL;
    new->data = malloc(bsize);
    bzero(new->data, bsize);
    memcpy(new->data, pdata, bsize);

    if (q->size == 0) {
        q->head = new;
    } else {
        q->tail->next = new;
    }
    q->tail = new;
    q->size++;

    return 0;
}

node_t *q_peek(queue_t *q)
{
    return q->head;
}

void q_pop(queue_t *q)
{
    if (q->size == 0)
        return;
    node_t *this = q->head;
    q->head = this->next;
    free(this);
    q->size--;
    if (q->size == 0)
        q->tail = NULL;
}

void freeq(queue_t *q)
{
    while (q->head != NULL) {
        node_t *t = q->head;
        q->head = t->next;
        free(t);
    }
    free(q);
}

void printq(queue_t *q)
{
    int i = 0;
    for (node_t *h = q->head; h != NULL; h = h->next) {
        printf("[%d] %s\n", i++, (char*)h->data);
    }
}

int main()
{
    char *test[] =
    {
        "January",
        "February",
        "Random"
    };
    int numtest = sizeof test / sizeof test[0];
    queue_t *nameq = q_init();
    for (int i = 0; i < numtest; i++) {
        q_push(nameq, test[i], strlen(test[i]));
    }
    printq(nameq);

    q_pop(nameq);
    printq(nameq);

    char *hname = (char*)(q_peek(nameq)->data);
    printf("head name = %s\n", hname);

    freeq(nameq);
    return 0;
}

BUG: q_init() doesn't return a value.  Your compiler should have caught this; perhaps you're running without enough diagnostics turned on?  (I think this requires a diagnostic, so perhaps you need a better compiler).

BUG:  We forgot to copy the terminating null character!

for (int i = 0; i < numtest; i++) {
    q_push(nameq, test[i], strlen(test[i]));
}

That should be strlen(test[i])+1.  This was quickly revealed by Valgrind.

Although you don't ever expect malloc() to fail, I think it's better to be in the habit of coding defensively.  "Memory available" might be fragmented, and not available in big enough chunks, for instance.  It's not hard to add the necessary tests, and it makes your code more re-usable, saving you much time in the longer term.
queue_t *q_init(void)
{
    queue_t *q = malloc(sizeof *q);
    if (q) {
        q->size = 0;
        q->head = q->tail = NULL;
    }
    return q;
}

bzero() isn't standard (i.e. portable) C.  Use memset() instead.  Actually, just remove that call entirely, as it's immediately followed by a memcpy() that completely overwrites it, making the bzero() a dead store.

We don't need to cast here:

    printf("[%d] %s\n", i++, (char*)h->data);

char *hname = (char*)(q_peek(nameq)->data);

data is a pointer to void, which can be assigned to any pointer type in C:
    printf("[%d] %s\n", i++, h->data);

const char *hname = q_peek(nameq)->data;

We've included <stdint.h> but then seemingly ignored it.  I would normally use size_t for the queue length and element sizes.  I certainly wouldn't use a signed type such as int.

Naming: why freeq() and not q_free() to match the other functions?

When pushing a value, we allocate twice, but q_pop() and freeq() don't release the value storage, only the queue node.  So we have a memory leak:
==4943==
==4943== 21 bytes in 3 blocks are definitely lost in loss record 1 of 1
==4943==    at 0x480B77F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==4943==    by 0x10922B: q_push (254594.c:40)
==4943==    by 0x10942E: main (254594.c:100)

We can fix this by adding another free() before the one that releases the node object.  But a more elegant fix is to allocate just once, for the node and stored value together:
node_t *new = malloc(sizeof *new + bsize);
if (!new) {
    return ENOMEM;
}
new->next = NULL;
new->data = (char*)(new + 1);
memcpy(new->data, pdata, bsize);

We don't really need the data member any more - the value is always at the same offset from the node pointer.

The interface can be improved - users shouldn't need to know about node_t.  Instead, just return pointer to the data member.

Modified code
#include <stdio.h>
#include <stdint.h>
#include <string.h>
#include <stdlib.h>

#define MAX_QSIZE   100
#define MAX_DATSIZE 128

typedef struct node {
    struct node *next;
    char data[];               /* flexible array member */
} node_t;

typedef struct queue_t {
    node_t *head;
    node_t *tail;
    size_t size;                /* element count */
} queue_t;

queue_t *q_init(void)
{
    queue_t *q = malloc(sizeof *q);
    if (q) {
        q->size = 0;
        q->head = q->tail = NULL;
    }
    return q;
}

void *q_push(queue_t *q, const void *pdata, size_t bsize)
{
    if (!q || q->size == MAX_QSIZE || bsize > MAX_DATSIZE) {
        return NULL;
    }

    node_t *new = malloc(sizeof *new + bsize);
    if (!new) {
        return NULL;
    }
    new->next = NULL;
    memcpy(&new->data, pdata, bsize);

    if (q->head) {
        q->tail->next = new;
    } else {
        q->head = new;
    }
    q->tail = new;
    q->size++;

    return &new->data;
}

const void *q_peek(queue_t *q)
{
    if (!q || !q->head) { return NULL; }
    return &q->head->data;
}

void q_pop(queue_t *q)
{
    if (!q || !q->size) { return; }
    node_t *this = q->head;
    q->head = this->next;
    free(this);
    if (!--q->size) {
        q->tail = NULL;
    }
}

void q_free(queue_t *q)
{
    if (!q) { return; }
    node_t *h = q->head;
    while (h) {
        node_t *t = h;
        h = t->next;
        free(t);
    }
    free(q);
}

static void printq(queue_t *q)
{
    int i = 0;
    for (const node_t *h = q->head;  h;  h = h->next) {
        printf("[%d] %s\n", i++, &h->data[0]);
    }
}

int main(void)
{
    const char *test[] =
    {
        "January",
        "February",
        "Random"
    };
    int numtest = sizeof test / sizeof test[0];
    queue_t *nameq = q_init();
    if (!nameq) {
        fprintf(stderr, "Failed to create queue\n");
        return EXIT_FAILURE;
    }
    for (int i = 0;  i < numtest;  i++) {
        if (!q_push(nameq, test[i], strlen(test[i]) + 1)) {
            fprintf(stderr, "Failed to create element %i\n", i);
            return EXIT_FAILURE;
        }
    }
    printq(nameq);

    q_pop(nameq);
    printq(nameq);

    const char *hname = q_peek(nameq);
    printf("head name = %s\n", hname);

    q_free(nameq);
}

This version has no errors when compiled by gcc -std=c17 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds  -Wstrict-prototypes    -Wconversion, and no issues reported by Valgrind.


Is catching 'expected' exceptions that bad? I had to find the corresponding process for a windows service the other day, and I kept on getting this one exception. I've spent some time researching the exception only to find that this exception will occur if the process is terminating, Hence my question, is it really that bad to catch expected exceptions ?

Sample Code:

try
{
  if (proc.MainModule.FileName == controller.ImagePath)
    {
             //Logic goes here
    }
 }
 catch (System.ComponentModel.Win32Exception ex)
 {
      // Thrown if process is already terminating,
      // the process is a Win16 exe or the process
      // could not be accessed.
      MyLogger.Debug(
                   "The following Win32Exception occurred while trying to access the MainModule information: " + ex.ToString());
}

There are some exceptions I classify as "exogenous" exceptions. That is, they are exceptions that state "the world is not how you assumed it would be, and we can't do the thing you asked". The problem with exogenous exceptions is that the world is constantly changing. You think you're going to be able to access that file, and then someone changes its permissions from another process, or the tape drive catches on fire, or someone unplugs the router, or whatever, and oops, you can't access the file after all.

Exogenous exceptions are precisely the ones you should be catching; they indicate situations that are rare enough and dangerous enough to reasonably be exceptions, but common enough that you can have some clue ahead of time as to what they are, and reasonably recover from them.

The other class of exceptions you need to catch are the "vexing" exceptions. That is, the exceptions where someone wrote you a library that communicates with you via exceptions in non-exceptional circumstances. Ideally you should never write a library that requires the developer to catch a vexing exception; rather, expose tools that allow them to prevent the exception.

For example, suppose you write a method that turns a string into an int. You might say

public static int Parse(string s) { ... }

And if the string is malformed, it throws an exception. That is vexing! The developer might have a string where they don't know whether it is well-formed or not, because a user typed it in. The work they have to do to avoid the exception is equivalent to simply not calling the method in the first place. A better way to do it is:

public static bool IsWellFormedIntString(string s) { ... }
public static int Parse(string s) { ... }

or

public static int? Parse(string s) { ... }

or

public static bool TryParse(string s, out int result) { ... }

Or whatever; there are lots of ways to avoid the vexing exception situation.

More thoughts on this topic here:

http://blogs.msdn.com/b/ericlippert/archive/2008/09/10/vexing-exceptions.aspx


Insertion Sort in Python3.6 Can this insertion sort program be improved in any way? Please suggest something in terms of efficiency and lines of code as well.

def insort(lst):
    for i in range(len(lst)-1):
        l = i + 1
        for j in range(l, -1, -1):
            print(j)
            if j == 0:
                break
            if lst[j] < lst[j-1]:
                lst[j], lst[j-1] = lst[j-1], lst[j]
    return lst

data = input("Enter the numbers separated by spaces: ")

print(insort(list(map(int, data.split()))))

Looks good to me. It's simple and easy to read. The only nit I might have:

l = i + 1
for j in range(l, -1, -1):

could be:

for j in range(i + 1, -1, -1):

since it doesn't look like you're using l anywhere else.


ASCII encryption program in Python I have recently written a program that reads information from a CSV file and puts it into lists. An encryption sub procedure converts the characters to ASCII, adds 2 then converts them back to characters.

import csv

ifile = open("messages.csv","rb")
reader= csv.reader(ifile)

plain_text= []
plain_ascii= []
encrypted_ascii= []
encrypted_text= []
latitude= []
longitude= []

for row in reader:
    latitude.append(row[0])
    longitude.append(row[1])
    plain_text.append(row[2])

encrypted_text=[''.join(chr(ord(ch)+2) for ch in string) for string in
plain_text]
print plain_text
print encrypted_text

count=0

print ""
while True:
    print ""
    response=int(raw_input("Enter 1 to see the next record, 2 to see the
previous record or 3 to end the program:"))
    print""
    if response > 3 or response < 1:
        print ("Please respond with one of the set responses.")
    elif response == 3:
        print "Process ended"
        break
    else:
        if response==1:
            count += 1
            if count >= len(plain_text):
                count = 0
        elif response==2:
            count -= 1
            if count < 0:
                count = len(plain_text) - 1
        print ""
        print("Record {0}:".format(count))
        print ""
        print("Latitude: {0}  Longitude: {1}  Unencrypted Text: {2}
Encrypted Text: {3}".format(latitude[count],longitude[count],
plain_text[count], encrypted_text[count]))

ifile.close()

Is there any way to make the entirety of this much more precise or simpler, and possibly any way to restructure this?

Just a few notes about print, I see you're not using parenthesis so i'm guessing you're using python 2.x.  I would urge you to switch to python 3.5+ now, as python 2.x is at end of life.  However if you insist on using it

-Keep consistent on using parenthesis

-You can use \n inside the string to add a newline:

print "Hello World!\n"

If you switch to python 3.x you can use the print function and pass in the end parameter to append a new line after your string:

s = "Hello World!"
print(s, end='\n') #NOTE: @Coal_ points out this is the default
# so if you didn't want there to be a newline you'd have to overwrite
# print(s, end=' ')
# @Solomon, and if you wanted a space between your outputs,
# print(s, end='\n\n')

That way you don't have seemingly useless print statements everywhere.


Pulling information from user input This code works by taking my user input and correctly pulling the information I'm giving it.

I'm having difficulty understanding the implementation of methods in classes. I'm pretty sure I'm not doing this the correct way. Can anyone show me the way it should be done?

class prog_setup:

 def __init__(self):
  """Initiates the below modules/variables."""

  self.get_starlist()
  self.get_InputKey()
  self.define_listsANDarrays()
  self.determine_normalizationpoint()
  self.create_fobs_fobserr()
  #self.calculate_taumag()
  self.search_DUSTYmodels()
  self.calculate_chi2()
  self.delete_null_components()
  self.dust_whittet_sub()

 def get_starlist(self):
  """Imports the list of stars that you would like information on."""

  self.star_list=np.array([])

  try:
   get_star_list=raw_input('Input pathname of the list of stars to be studied: ')
   incoming_stars=open(get_star_list,'rb')
   for line in incoming_stars:
     x=line.split()
     self.star_list=np.append(self.star_list,x) #Appends the individual star-IDs to the empty array star_list.
   print 'Stars imported successfully.'
  except IOError:
   print 'Star import unsuccessful. Check that the star catalog file-path is correct. Program exiting now.'
   sys.exit()
  self.indices_of_star_list=range(len(self.star_list))

def get_InputKey(self):
 """Imports the Input (.inp) file. Historically, InputUttenthaler.inp. Allows for definition
 of parameters which are utilized later in the script."""

 input_script=raw_input('Pathname of .inp file: ')
 InputKey=[]
 self.band=''
 self.Ak_scalefactor=''
 try:
  with open(input_script) as input_key:
    for line in input_key.readlines():
     x=[item for item in line.split()]
     InputKey.append(x)
    if InputKey[0][0]=='1' or InputKey[0][0]=='2':  #Checks to see if .inp file is valid or not by checking the first row/column which should be 1 or 2.
     print 'Your .inp file was successfully imported'
    else:
     print 'Your .inp file import failed. Check the validity of your file.\n Program exiting now.'
     sys.exit()
 except IOError:
  print 'The .inp file import was unsuccessful. Check that your file-path is valid.\n Program exiting now.'
  sys.exit()

The entire code is longer; I just put in the first two methods to give an example of what my structure looks like.

If you decide to implement this without using a class you can simply write:

def get_starlist():
    ...
def get_inputkey():
    ...

if __name__ == "__main__":
    get_starlist()
    get_input_key()
    define_lists_and_arrays()
    determine_normalization_point()
    create_fobs_fobserr()
    search_DUSTY_models()
    calculate_chi2()
    delete_null_components()
    dust_whittet_sub()

But if you find that some of your functions are sharing data a lot you may want to attach them to a class, or pass instances of a class (objects) between the different functions. It's hard to say more without knowing what you're trying to do.


Better way to refer to common elements in a sequence of objects? I just posted this code in SO and one person said my code sucked, and that I should post it here. So here it is (with fix I asked for included, so you don't have to worry about that).

The main concerns that I have with it myself is how I iterate through a sequence of objects that the object itself is in. Apparently id is bad, because it's like id(). Someone also mentioned that I shouldn't make members of a collection overly aware of their own collection.

    import random

    class Car:
        def __init__ (self, company, doors, id):
            self.company = company
            self.doors = doors
            self.id = id

        def printDoors(self, id):
            print 'Car ' + `self.id` + ' has ' + `self.doors` + ' doors.'

        def findSameDoors(self, id):
            for i in self.company.cars:
                if self.id != i.id and self.doors == i.doors:
                    print 'Car ' + `i.id` + ' does too!'

    class Company:
        def __init__ (self, types):
            self.types = types
            self.cars = []

        def typesToNum(self):
            result = []
            for i in self.types:
                if i == 'sedan':
                    result.append(4)
                elif i == 'convertible':
                    result.append(2)
                else:
                    result.append(0)
            return result

    porsche = Company(['sedan', 'convertible'])
    honda = Company(['sedan', 'convertible', 'motorcycle'])

    for i in range(10):
        porsche.cars.append(Car(porsche, random.choice(porsche.typesToNum()), i))

    for i in range(10):
        honda.cars.append(Car(honda, random.choice(honda.typesToNum()), i))

    porsche.cars[0].printDoors(0)
    porsche.cars[0].findSameDoors(0)

Thanks for any pointers.

It is, as you say, slightly odd that a method in Car iterates over the container that Car is in. It really ought to be in Company, and take an argument for how many doors to match:

class Company:
    ...
    def searchByDoors(self, doors):
    ''' Get an iterator of all the cars in self that have the
        specified number of doors
    '''

    for car in self.cars:
         if car.doors == doors:
             yield car

This also brings up another point: you probably want to avoid printing from inside your class (except for debugging). Even if whatever you're writing now wants it printed, you should write your classes so they can be used in any program . So, a search function should give the matches back whichever part of your program called it, and it can print them if it wants to. Similarly, Car.printDoors can go - your program can get that information and print it. If there is a canonical way that you want represent a Car for human consumption, define it this way:

 class Car:
     ...
     def __str__(self):
         return "A {} {} with {} doors".format(self.company, self.type, self.doors)

And your program can just print(a_car) to print something like A porche sedan with 4 doors.

Note that, notwithstanding what the person on SO said about not making objects aware of their containers, it probably does make sense for a Car to know which Company made it.

There are two issues with using id as an attribute name:

The local variable in the constructor shadows the built-in id() function,
More importantly, car.id is different to id(car), even though they're named similarly and have similar purposes.

Consider whether id(car) is good enough for what you would use car.id for - it identifies a unique python object, and is called for you when you do things like a is b. If id needs to reflect something external to your program, consider renaming it. car_id might work, but if it represents something concrete in your problem domain (eg, an official registration number), naming it after that is even better.

There is also one issue with the way you're generating ids, that using Python's id() avoids: your ids are only unique within a company (and even that isn't guaranteed), but you could reasonably expect it to be unique across all cars. So, overall, I would drop id entirely.

typesToNum doesn't belong in Company, and this is really begging for a dictionary rather than a method. I'd make it a module-level private constant:

 # Mapping of car type (eg, sedan) to number of doors
 _DOORS_PER_TYPE = {'sedan': 4, 'convertible': 2, 'motorcycle': 0}

A car should really know what type of car it is, and with this mapping it would make more sense to take this at construction than the number of doors:

 class Car:
      def __init__(self, company, type):
            assert type in company.types
            self.type = type

      @property
      def doors(self):
          return _DOORS_PER_TYPE[self.type]

The assert makes sure that the type of car is one that that company does make. It is optional, its just a sanity check in case something impossible happens. Its a good idea, because that is more common than it sounds. Likewise, you can do:

   class Company:
       def __init__(self, types):

The constraint that a convertible always has 2 doors, and a sedan always has 4 is a little odd. If it might make sense in your program to have a 2 or 4 door sedan, but not, say, a single-door sedan, you could do this:

 _DOORS_PER_TYPE = {'sedan': (2, 4)}

 class Car:
      def __init__(self, company, type, doors):
          assert type in company.types
          assert doors in _DOORS_PER_TYPE[type]

          self.company = company
          self.type = type
          self.doors = doors

This also makes doors an attribute rather than a property since its no longer linked 1-to-1 with type, but the rest of your program doesn't need to care about that difference.

You can also change the valid car types and number of doors to vary between each company, but only if that makes sense for your model. If the number of doors is always 1-to-1 with the type, and that is always going to be independent of the company who built it, then it is better to have your code say so.


Unity simple map generation I am trying to generate a simple map like this. Everything works as I want, but I am new to programing. Is it a good way to do something like this? And if not, could someone explain to me where and why it is bad to do something like this? And how bad is it on performance?

public GameObject cube;
public GameObject cube1;
public GameObject cube2;
Vector3 here;

void Start () {
    for (int y = 0; y < 50; y++) {
        for (int x = 0; x < 10; x++) {
            here = new Vector3(x,y,0);
            int change = (int)Random.Range(0,19);
            switch (change){
            case 0: case 1: case 2: case 3: case 4: case 5: case 6: case 7: case 8: case 9: case 10:
                Instantiate(cube,here,Quaternion.identity);
                break;
            case 11: case 12: case 13: case 14: case 15: case 16:
                Instantiate(cube1,here,Quaternion.identity);
                break;
            case 17: case 18:
                Instantiate(cube2,here,Quaternion.identity);
                break;
            }
        }
    }

Running this code generates something random like this:

Instantiate takes 3 things (GameObject you want to make, coordinates where to make it, and rotation), and it makes a copy of GameObject - prefab in those coordinates.

Cube - Red
Cube1 - Green
Cube2 - Blue

The reason for all of this is that I want to learn to make something random likea  map area of the game.

As a first step, I think this is cleaner than the switch

if (change <= 10)
{
    Instantiate(cube, here, Quaternion.identity);
}
else if (change <= 16)
{
    Instantiate(cube1, here, Quaternion.identity);
}
else
{
    Instantiate(cube2, here, Quaternion.identity);
}

But there's still repetition with the calls to Instantiate. I'd suggest creating a new method like this

private GameObject GetRandomCube()
{
    var change = (int)Random.Range(0, 19);
    if (change <= 10)
    {
        return cube;
    }

    if (change <= 16)
    {
        return cube1;
    }

    return cube2;
}

And then use it like this in your loop

Instantiate(GetRandomCube(), here, Quaternion.identity);

If here isn't being used elsewhere in the class, move its declaration inside the inner loop, or just use it in the function call. Now your method looks like this

void Start()
{
    for (var y = 0; y < 50; y++)
    {
        for (var x = 0; x < 10; x++)
        {
            Instantiate(GetRandomCube(), new Vector3(x, y, 0), Quaternion.identity);
        }
    }
}


What is the fastest way to record the listed files in a directory in C? I want to fill an array of dirent pointers with each entry of a specified directory. I either need to count the items before, allocate memory, then go over them again, or I will have to reallocate each iteration of the while loop. Both of these options seem slow. Surely there's a better way.
Are these my only option:
struct dirent **Entries;
DIR *Dir = opendir("Some dir path");
struct dirent *Entry;
unsigned int Count = 0;
while ((Entry = readdir(Dir))) {
    Count++;
}
Entries = malloc(Count*sizeof(struct dirent*));
rewinddir(Dir);
for (int i = 0; (Entry = readdir(Dir); i++) {
    Entries[i] = Entry;
}

Or
struct dirent **Entries;
DIR *Dir = opendir("Some dir path");
Dir = opendir("Some dir path");
struct dirent *Entry;
for (int i = 0; (Entry = readdir(Dir); i++) {
    Entries = realloc(Entries, i*sizeof(struct dirent*));
    Entries[i] = Entry;
}
closedir(Dir);

Or is there some faster way?

OP poses 2 approaches

Iterate twice through opendir(), readdir(), closedir() with one malloc().

Iterate one through opendir(), readdir(), closedir() with multiple realloc().

For performance, I would use a 3rd option: linked list instead of using an array of struct dirent

Iterate once through opendir(), readdir(), closedir() with one malloc() per entry.

As with such performance issue: strive for reduced big O, not linear ones.


Check existence of HTML tags in a row Using Python's Beautiful Soup, I analyze a number of html files (internally: soup). They are all a little different and my target comes in four different tags - or not at all. In two cases, my target may not be there but the tags may exists. In this case I find a ↵ .

Here I got a very ugly code to check for them:

#1: in

if len(soup.find_all("div", {"class":"section ack"})) > 0:
    thanktag = soup.find("div", {"class":"section ack"})
#2: in

elif len(soup.find_all("li", {"class":"fn-con"})) > 0:
    thanktag = soup.find("li", {"class":"fn-con"})
#3: in next-to-last

elif len(soup.find_all("li", {"class":"fn"})) > 1:
    thanktag = soup.find_all("li", {"class":"fn"})[-2]
#4: in first

    if "↵" in clean_string(thanktag.text):
        thanktag = soup.find_all("li", {"class":"fn"})[0]
#5: doesn't exist at all
if 'thanktag' in globals() and not "↵" in clean_string(thanktag.text):
    do_something()

For example, my code doesn't adhere to DRY. How could I make the code more pythonic/less redundant?

I'm interested to hear other approaches and any suggestions to mine. Here are two that I came up with:

Without a Helper Class

thanktag_queries = [
    ("div", {"class":"section ack"}, 0),
    ("li", {"class":"fn-con"}, 0),
    ("li", {"class":"fn"}, 1)
]

thanktag = None
for i in xrange(len(thanktag_queries)):
    thanktag_query = thanktag_queries[i]
    soup_result = soup.find_all(thanktag_query[0], thanktag_query[1])
    if len(soup_result) > thanktag_query[2]:
        thanktag = soup_result[0]

        if i == len(thanktag_queries)-1:
            if not contains_carriage_return(thanktag.text):
                thanktag = soup_result[-2]

        break
if thanktag != None and not contains_carriage_return(thanktag.text):
    do_something()

def contains_carriage_return(text):
    return "↵" in clean_string(text)

With a Helper Class

class ThanktagQuery:
    soup_result = None

    def __init__(self, tag, soup_options, threshold):
        self.tag = tag
        self.soup_options = soup_options
        self.threshold = threshold

    def execute_query():
        self.soup_result = soup.find_all(self.tag, self.soup_options)

    def contains_thanktag():
        try:
            return len(self.soup_result) > self.threshold
        except TypeError as exc:
            raise RuntimeError("Query has not been run yet for this instance.")

thanktag_queries = [
    ThanktagQuery("div", {"class":"section ack"}, 0),
    ThanktagQuery("li", {"class":"fn-con"}, 0),
    ThanktagQuery("li", {"class":"fn"}, 1))
]

thanktag = None
for i in xrange(len(thanktag_queries)):
    thanktag_query = thanktag_queries[i]
    thanktag_query.execute_query()
    if thanktag_query.contains_thanktag():
        thanktag = thanktag_query.soup_result[0]

        if i == len(thanktag_queries)-1:
            if not contains_carriage_return(thanktag.text):
                thanktag = thanktag_query.soup_result[-2]

        break
if thanktag != None and not contains_carriage_return(thanktag.text):
    do_something()

Both pieces of code, which attempt to be logically synonymous, address OP's initial concern with keeping the "soup finds" DRY. That is, the parameters of each soup find should only be hardcoded once instead of twice as in the original code.

The pieces of code accomplish keeping DRY by specifying the find parameters in a list called thanktag_queries. The list is composed of elements that each correspond to hardcoded information of the first four if-statements in the OP's code. This hardcoded information contains three elements (hence the 3-tuples in the case of the Without Helper Class): the target tag, the soup options (in this case the target class), and the threshold that the length of the executed soup query needs to exceed to deem that the thanktag exists in those DOM elements.

So far I've talked about the structure of the outside of the for-loop. What about within the for-loop? The structure within the for-loop differs in two ways from a direct translation of the structure from the OP's if statements. First, find_all(...)[0] is used in place of find(...) because the find_all(...) needs to be executed anyway. Second, the special case from the original fourth if-statement is handled in reverse (i.e. it uses find_all(...)[0] as default instead of find_all(...)[-2]). This choice was made because using find_all(...)[0] matches the pattern of the other if statements. This makes the code more brief, and in this case makes it more understandable.

At the end of the code, I use thanktag == None instead of thanktag in globals() in case of an unintended use of the thanktag variable in that namespace.

I wrote the "With a Helper Class" so that the code is written in a style that is more human readable and self-documenting.


Record transaction log I am trying to resolve following problem to improve my DS knowledge. Please advice which part of the code can be improved.

You run an e-commerce website and want to record the last N order ids
in a log. Implement a data structure to accomplish this, with the
following API:
record(order_id): adds the order_id to the log get_last(i): gets the
ith last element from the log. i is guaranteed to be smaller than or
equal to N. You should be as efficient with time and space as
possible.

Code explanation:
I choose linked list with single pointer (prev) to keep transaction logs. I suppose it would be called Stack Linked List.
stProduct -> linked list node
stProdStack -> struct to keep database information, namely ll tail and size (N)
record() examines db information struct tail pointer and order_id within range, then calls actual method to add node to the list: add_product().
I did not implement unit testing due to limited time to seat on.
PARDON MY ENGLISH xD
Code:
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

//backward/stack linked list for storing transaction action log
typedef struct stProduct
{
    struct stProduct * prev;
    unsigned int order_id;
}
stProduct;

typedef struct stProdStack
{
    struct stProduct * tail;
    unsigned int size;
}
stProdStack;

stProduct* add_product(stProduct *tail, unsigned int order_id)
{
    stProduct *product = malloc(sizeof *product);
    if (!product) return NULL;
    product->order_id = order_id;
    product->prev = tail;
    return product;
}

bool record(stProdStack *stListInfo, unsigned int order_id)
{
    if (!stListInfo)
        return false;
    stProduct *tail = add_product(stListInfo->tail, order_id);
    if (tail)
    {
        stListInfo->tail = tail;
        return true;
    }
    else
    {
        return false;
    }
}

unsigned int get_last(const stProdStack *stListInfo, unsigned int i)
{
    if (!stListInfo) return 0;
    if (i > stListInfo->size) return 0;

    stProduct *current = stListInfo->tail;
    for (int j = 0; j < i - 1; j++)
    {
        current = current->prev;
    }

    return current->order_id;
}

int main(void)
{
    unsigned int N = 100;
    stProdStack product_stack = { NULL, N  };

    for (unsigned int i = 0; i < 300; i++)
    {
        if (!record(&product_stack, 1000 + i))
        {
            printf("transaction log record failed\n");
            break;
        }
    }

    printf("10th last product ID: %u\n", get_last(&product_stack, 10));

}

Result:
10th last product ID: 1290

The code doesn't fulfill the requirements. You are supposed to log the last N orders. Instead you log all of them. The old orders shall be removed from the list as it grows beyond N.

Bug. The list is initialized with the size being set to N. However, at the beginning it may contain less than N elements. In the scenario
  List is empty
  An order is added
  get_last(2) is called

the loop
  for (int j = 0; j < i - 1; j++)
  {
      current = current->prev;
  }

is guaranteed to access a null pointer.

The above loop complexity is \$O(N)\$.

All that said, I recommend to use not a list, but a circular buffer. No need to malloc anything, and get_last is executed in \$O(1)\$.


Using basic REST API in Angular 1.x I have built real-time markdown editor in order to learn AngularJS by practice. I think what I'm trying to do can be done a better way. Any suggestions appreciated. Here's my code:

var myApp = angular.module('myApp',[]);
myApp.factory('Data',function ($window) {
    return {message: $window.localStorage['stuff']}
});

function FirstCtrl($scope,$window,Data){
    $scope.data = Data;
    $scope.$watch(
        "data.message",
        function (newval,oldval){
            $window.localStorage['stuff'] = newval;
        })
}

function SecondCtrl($scope,$location,$element,$compile,Data){
    $scope.data = Data;
    $scope.location = $location;
    $scope.compile = function( newHTML ) {
        newHTML = $compile(newHTML)($scope);
        $element.html('').append(newHTML);
    };

    $scope.$watch("data.message", function (newval, oldval) {
        var $http = angular.injector(["ng"]).get("$http");
        $http.post('http://api.github.com/markdown/raw?access_token=' + $scope.location.$$absUrl.substring($scope.location.$$absUrl.indexOf("=")+1),
            newval,
            {headers:{'Content-Type':'text/plain'}})
        .then(function(res){
            $scope.data.dddd = res.data;
            //console.log($scope.data.dddd);
            if(!$scope.data.dddd) return;
            $scope.compile($scope.data.dddd);
        });
    })
}

$scope.$watch(
    "data.message",
    function (newval,oldval){
        $window.localStorage['stuff'] = newval;
    })

Not entirely sure why this is formatted this way. Looks odd. Also, you use double and single quotes. They don't really matter for JS strings, but for consistency, use one of the two. I usually recommend single quotes because they're cleaner-looking.

$scope.$watch('data.message',function (newval,oldval){
  $window.localStorage['stuff'] = newval;
});

More compact, less weird. :D

I notice you use Github-flavored markdown but that entails sending it over the wire and waiting for a response. That's a slight overhead plus it heavily relies on a service that may go down any time. I suggest you look for an offline library. I suppose there is one for Github-flavored (MarkdownPad supports such) but other libraries could come close.

Naming could also need rework. FirstCtrl and SecondCtrl don't really tell me anything about the controllers without reading their code (which is the first sign of terrible code, having to read everything). The first controller could be Editor because it has your text area binding while the second could be GitHubMarkdownRederer because that's what it does.

I don't see why you need to compile the markup before appending it to the DOM. It doesn't require Angular facilities. You can simply pop it in as HTML in the DOM.

Appending arbitrary markup on the current is dangerous. What editors like jsFiddle do is host the renderer in a totally different domain and render it on the page using an iframe. That way, built-in browser security takes care of the rest, preventing inter-domain, inter-frame communication. There are other options out there, not sure which one is best for the situation.


Code for filtering in ArrayAdapter I have created a code for filtering a list of strings and redisplaying this list. I check if string is starting with a certain letter.
It seems to work, but I wonder if I did all right? Havent I forgot about something?

public class AlternateRowArrayAdapter extends ArrayAdapter
{
    private int[] colors;
    private List<String> list = null;
    private ArrayList<String> arrayList;

    public AlternateRowArrayAdapter(@NonNull Context context, @LayoutRes int layout,
                                    @IdRes int textViewResourceId, @NonNull List<String> objects)
    {
        super(context, layout, textViewResourceId, objects);
        colors = new int[] {ResourcesCompat.getColor(context.getResources(), R.color.white, null) ,
                            ResourcesCompat.getColor(context.getResources(), R.color.lmsLightGrey, null)};
        this.list = objects;
        this.arrayList = new ArrayList<>();
        this.arrayList.addAll(this.list);
    }
    /**
     * Display rows in alternating colors
     */
    @Override
    public View getView(int position, View convertView, ViewGroup parent) {
        View view = super.getView(position, convertView, parent);
        int colorPos = position % colors.length;
        view.setBackgroundColor(colors[colorPos]);
        return view;
    }

    @Nullable
    @Override
    public Object getItem(int position)
    {
        return list.get(position);
    }

    public void filter(String letter)
    {
        letter = letter.toLowerCase();
        list.clear();
        if (letter == null || letter == "")
        {
            list.addAll(arrayList);
        }
        else
        {
            for (String item : arrayList)
            {
                if (item.toLowerCase().startsWith(letter))
                {
                    list.add(item);
                }
            }
        }
        notifyDataSetChanged();
    }

Also it supposed to be dual color rows - but this part works fine.

I have forgot about this part:

@Override
public int getCount()
{
    return filteredItems.size();
}

Without this in getItem there was an OutOfRangeException

Use final properties

private List<String> list = null;
private ArrayList<String> arrayList;

Both properties are set in the constructor, so they should both be final.

private final List<String> list = new ArrayList<>();
private final ArrayList<String> arrayList = new ArrayList<>();

public AlternateRowArrayAdapter(@NonNull Context context, @LayoutRes int layout,
                                @IdRes int textViewResourceId, @NonNull List<String> objects)
{
   // ...
    this.list.addAll(objects); // same solution for both lists!
    this.arrayList.addAll(this.list);
}

Choose better names

Naming is the hardest part in programming. Always take your time to think about the names of your identifiers, if they express their purpose.

E.G.: Your lists should have better names like allItems and filteredItems

Work in constructors

In general constructors should not access any method on a dependency or and non private or non final method inside its own class (except super() and this()).

Therefore I'd rather pass in the colors array as constructor parameter. But I don't know much about android programming to say if that is even possible...

  Regarding using colors in constructors - it's possible to pass them as an arguement, but we already pass a context as an argument which contains these colors – Marek Czaplicki

Because you passed the context instead of the color array you class has two more dependencies: the context itself and the ResourcesCompat class. You class should have as less dependencies as possible.


Complex dashboard using presenters and/or service objects TL;DR

I need a way to refactor a complex user dashboard with several objects and some complex data to display a accounting chart. I have read about both presenters (also called decorators or view models) and service object, but I'm not sure if I should use one or the other, or both? And how to implement this? My model and controller are pretty messy, so if anyone have some hints or suggestions, would I really appreciate it.

Long version:

I'm currently working on an application that helps users to coordinate dinner clubs and all  related accounting. (A dinner club is where people in a group, take turns to cook for the rest and then you pay a small amount to participate. This is pretty normal in dorms and colleges where I'm from). When you login are you presented to a dashboard with all important information separated in three blocks: the next dinner and option to registrate, the next dinner where you have to cook, and accounting overview like current debt, spendings etc.

This gets pretty messy: a lot of instance variables in my controller, and a lot of methods to present this view in my models.

So now to the real question: can anyone tell me any good hints, design patterns or general advice to help me refactor this code? I have read about presenters, service objects, decorators etc. but I'm not sure which to use and how.

Here are some examples of how bad it looks right now (a kitchen is the group of people that have dinner together):

# app/controllers/dashboard_controller.rb
def index
  @user = current_user
  @kitchen = @user.kitchen

  @upcoming_dinner_clubs = @user.upcoming_dinner_clubs # The next dinner clubs where the current user have to cook
  @users_next_dinner_club = @user.next_dinner_club # The first of upcoming_dinner_clubs
  @unpriced_dinner_clubs = @user.unpriced_dinner_clubs # Old dinner clubs where the user haven't specified a price yet

  # The next dinner club in the kitchen
  @next_dinner_club = @kitchen.next_dinner_club if @kitchen.next_dinner_club
  @todays_dinner_club = @next_dinner_club if @next_dinner_club && @next_dinner_club.date.today?
end

This view shows some chart of the expenses and spendings of a user, rendered through JavaScript. My views are in HAML.

# app/views/dashboard/_expenses.html.haml
%h2 Dit forbrug
  %p
    = t '.usage_html', expenses: number_to_currency(@user.last_month_expenses), spendings: number_to_currency(@user.last_month_spendings), results: number_to_currency(@user.last_month_results)
    = content_tag :div, "", id: "revenue_chart", class: "chart dashboard-chart", data: { chart: @user.usage_chart_data }
    = t '.results_html', results: number_to_currency(@user.total_results)
    = content_tag :div, "", id: "result_chart", class: "chart dashboard-chart", data: { chart: @user.result_chart_data }

Don't want you to bore you with all the details, and how the methods work, but this is the methods i have, only for displaying the expenses and spendings data in the view:

# app/models/user.rb
def last_month_expenses
  expenses_for((1.month + 1.day).ago, 1.day.ago)
end

def last_month_spendings
  spendings_for((1.month + 1.day).ago, 1.day.ago)
end

def last_month_results
  results_for((1.month + 1.day).ago, 1.day.ago)
end

def spendings_for(start_date, end_date, kitchen)
end

def expenses_for(start_date, end_date, kitchen)
end

def fee_for(start_date, end_date, kitchen)
end

def accounting_query_conditions(start_date, end_date, kitchen)
  {date: start_date..end_date, kitchen_id: kitchen.id}
end

def results_for(start_date, end_date)
  spendings_for(start_date, end_date) - expenses_for(start_date, end_date)
end

def total_fee(date = Date.today, kitchen = primary_kitchen)
end

def total_spendings(date = Date.today, kitchen = primary_kitchen)
end

def total_used_on_dinner_clubs(date = Date.today, kitchen = primary_kitchen)
end

def total_expenses(date = Date.today, kitchen = primary_kitchen)
end

def total_results(date = Date.today, kitchen = primary_kitchen)
  total_expenses(date, kitchen) - total_spendings(date, kitchen)
end

There is a lot here and without diving into the pattern based approach, my first question would be have you considered using rails scopes? Maybe that answer is overly simplistic, but using scopes would get rid of a lot of your reporting methods and make you be able to do something like:

Spendings.last_month
Results.last_month

And speaking of Spendings and Results, it sounds like you may want to create a rails class or model for some of these items (like Results and Spendings and Expenses) instead of just querying them from a dashboard view? I have some tableless models in my apps that are used for reporting purposes. It seems like a much cleaner approach than having a long dashboard controller doing the querying for you. In that same vein, another data / reporting centric approach that isn't rails specific is to create reporting tables (these could be rails models) that are divorced from the transactional data just so you aren't querying reports from the live system which can lead to performance hits for both users on the transactional side and the reporting side.

Rails is very "flat" from a traditional 3rd normal form database architecture, so it isn't much work to create reporting tables.... Just some thoughts. Good luck.


A simple C# calculator I believe that building a simple calculator is one of the most popular tasks for beginners. There are several conditions, however, in this case:

Delegates should be used
The user types a string that should be parsed as a math expression with just two numbers and one of the four operators (+, -, *, /)
My first idea of using the Compute method of DataTable was, predictably, rejected

I know that there is one serious flaw in the current version of the program. The input string is not validated. Apart from that, what are your recommendations and suggestions with regards to my code?

GitHub

using System;
namespace CalculatorDelegate
{
    class Program

    {
        private static double Item1 { get; set; }
        private static double Item2 { get; set; }
        private static string Action { get; set; }
        private delegate double Del(double a, double b);

        private static void Main(string[] args)
        {
            while (true)
            {
                Console.WriteLine(ParseInput());
            }
        }

        private static double ParseInput()
        {
            Console.WriteLine("Enter a mathematical expression, please");
            var userInput = Console.ReadLine();
            var userInputLength = userInput.Length;
            Del handler;

            for (var i = 1; i < userInputLength; i++)
            {

                if ( userInput[i].Equals('+') || userInput[i].Equals('-') ||
                    userInput[i].Equals('*') | userInput[i].Equals('/'))
                {
                        Action = Convert.ToString(userInput[i]);
                        double.TryParse(userInput.Substring(0, i), out var tempItem1);
                        double.TryParse(userInput.Substring(i + 1, userInputLength - i - 1), out var tempItem2);

                        Item1 = tempItem1;
                        Item2 = tempItem2;

                        if (Action.Equals("/") && Item2 == 0)
                        {
                            Console.WriteLine("You can't divide by zero");
                            return 0;
                        }

                        break;
                }
            }

            switch (Action)
            {
                case "+":
                    handler = GetSum;
                    break;

                case "-":
                    handler = GetDiff;
                    break;

                case "*":
                    handler = GetMult;
                    break;

                case "/":
                    handler = GetDiv;
                    break;

                default:
                    return 0;
            }

            return handler(Item1, Item2);
        }

        private static double GetSum(double a, double b)
        {
            return a + b;
        }

        private static double GetDiff(double a, double b)
        {
            return a - b;
        }

        private static double GetMult(double a, double b)
        {
            return a * b;
        }

        private static double GetDiv(double a, double b)
        {
            return a / b;
        }

    }
}

I completely agree with you that such tasks is a good opportunity to learn new concepts and the language itself. And, as you asked, I have a few comments on your code

Do not call variables by reserved names

I mean, I was sweating to understand what is going on here the first time

Action = Convert.ToString(userInput[i]);

Better to call such variable as

private static string OperationString { get; set; }

Split complex task on simpler ones - one logic per function

Your function ParseInput is doing not just parsing, but also reading from standard input. So split this task - ParseInput will just do parsing.

private static double ParseInput(string inputString)

Using the == operator to compare floating-point numbers is, generally, a bad idea

So instead of

if (Action.Equals("/") && Item2 == 0)

better to use this

if (Action.Equals("/") && Math.Abs(Item2) < Double.Epsilon)

Be aware of the difference between || and | logic operations

And without a need, do no mix up this two operators. So use

if ( userInput[i].Equals('+') || userInput[i].Equals('-') || userInput[i].Equals('*') || userInput[i].Equals('/'))

instead of

if ( userInput[i].Equals('+') || userInput[i].Equals('-') || userInput[i].Equals('*') | userInput[i].Equals('/'))

Better to avoid static not-readonly field

I don't see any actual reason for you to keep variables Item1, Item2 and OperationString as private static fields - make them as local variables. In general, based on my experience, many nasty bugs (especially in multi-thread applications) happen because of an inappropriate usage of static fields. I know, this problem is far away from this simple calculator application, but anyway.

Good regular expression would solve many problems, including

code will become simpler and cleaner, so easy to debug and read
easier to solve your "serious flaw" - input validation

I would suggest to use Regular Expressions instead of buggy for-if construction.

// \d+\.? supports xxx. and xxx
// \.\d+ - .xxx
// \d+\.\d+ - xxx.xxx
Regex InputRegex = new Regex(@"^\s*([-+]?)(\d+\.?|\.\d+|\d+\.\d+)(.)(\d+\.?|\.\d+|\d+\.\d+)\s*$");

// match.Groups[0] whole pattern
// match.Groups[1] sign of the first value
// match.Groups[2] first value (without sign)
// match.Groups[3] operation
// match.Groups[4] second value
Match match = InputRegex.Match(userInput);

if (!match.Success)
{
    // we didn't find a match
}
double item1 = match.Groups[1].Value == "-" ? -Double.Parse(match.Groups[2].Value) : Double.Parse(match.Groups[2].Value);
string actionString = match.Groups[3].Value;
double item2 = double.Parse(match.Groups[4].Value);

Let user know that calculation was successful or not

Let's check which error can happen in your code:

RegexNotSuccess Regular expression doesn't find a match
DivisionByZero Second item is 0, so we have the division by zero
NoSuchOperation User specified not [+-*/] operation

I would suggest to create enumerated list of those errors, return one enum element as output of your function.

public enum ErrorCode
{
    Success,
    NoSuchOperation,
    DivisionByZero,
    RegexNotSuccess
}

public static ErrorCode ParseInput(string userInput, out double result)
{
    //...
    if (!match.Success)
    {
        result = Double.MinValue;
        return ErrorCode.RegexNotSuccess;
    }
    //...
    switch (actionString)
    {
        //...
        case "/":
            if (Math.Abs(item2) < Double.Epsilon)
            {
                result = Double.MinValue;
                return ErrorCode.DivisionByZero;
            }
        //...
        default:
            result = Double.MinValue;
            return ErrorCode.NoSuchOperation;
    }
    result = handler(item1, item2);
    return ErrorCode.Success;
}

The final code looks like this

Program.cs

using System;

namespace CalculatorTest
{
    class Program
    {
        private static void Main()
        {
            while (true)
            {
                Console.WriteLine("Enter a mathematical expression, please");
                string userInput = Console.ReadLine();
                Calculator.ErrorCode stdout = Calculator.ParseTwoNumberOperation(userInput, out double result);
                if (stdout == Calculator.ErrorCode.Success)
                {
                    Console.WriteLine(result);
                }
                else
                {
                    Console.Error.WriteLine("Error: " + stdout + "\t" + userInput);
                }
            }
        }
    }
}

Calculator.cs

using System;
using System.Text.RegularExpressions;

namespace CalculatorTest
{
    public static class Calculator
    {
        public enum ErrorCode
        {
            Success,
            NoSuchOperation,
            DivisionByZero,
            RegexNotSuccess
        }

        private delegate double Handler(double a, double b);
        /// <summary>
        /// \d+\.? supports xxx. and xxx
        /// \.\d+ - .xxx
        /// \d+\.\d+ - xxx.xxx
        /// </summary>
        private static readonly Regex InputRegex = new Regex(@"^\s*([-+]?)(\d+\.?|\.\d+|\d+\.\d+)(.)(\d+\.?|\.\d+|\d+\.\d+)\s*$");
        public static ErrorCode ParseTwoNumberOperation(string userInput, out double result)
        {
            Handler handler;
            // match.Groups[0] whole pattern
            // match.Groups[1] sign of the first value
            // match.Groups[2] first value (without sign)
            // match.Groups[3] operation
            // match.Groups[4] second value
            Match match = InputRegex.Match(userInput);
            if (!match.Success)
            {
                result = Double.MinValue;
                return ErrorCode.RegexNotSuccess;
            }
            double item1 = match.Groups[1].Value == "-" ? -Double.Parse(match.Groups[2].Value) : Double.Parse(match.Groups[2].Value);
            string actionString = match.Groups[3].Value;
            double item2 = double.Parse(match.Groups[4].Value);
            switch (actionString)
            {
                case "+":
                    handler = GetSum;
                    break;

                case "-":
                    handler = GetDif;
                    break;

                case "*":
                    handler = GetMul;
                    break;

                case "/":
                    if (Math.Abs(item2) < Double.Epsilon)
                    {
                        result = Double.MinValue;
                        return ErrorCode.DivisionByZero;
                    }
                    handler = GetDiv;
                    break;

                default:
                    result = Double.MinValue;
                    return ErrorCode.NoSuchOperation;
            }

            result = handler(item1, item2);
            return ErrorCode.Success;
        }

        private static double GetSum(double a, double b) => a + b;
        private static double GetDif(double a, double b) => a - b;
        private static double GetMul(double a, double b) => a * b;
        private static double GetDiv(double a, double b) => a / b;
    }
}


Check if array has the same number of even and odd values in Python Task:
Complete the following function that determines if the number of even and odd values in an list of Integers is the same.

| In               | Out   |         Why            |
|------------------|-------|------------------------|
| [5, 1, 0, 2]     | true  | two evens and two odds |
| [5, 1, 0, 2, 11] | false | too many odds          |
| []               | true  | both have 0            |

The function should not affect the contents of the list.

My code:

def balanced(lst):
    n = len(lst)
    if n % 2 != 0:
        return False
    if n % 2 == 0:
        count_1 = 0
        count_2 = 0
        for item in lst:
            if item % 2 == 0:      #even
                count_1 += 1
            if item % 2 != 0:       #odd
                count_2 += 1
    if count_1 == count_2:
        return True
    else:
        return False
def result(lst):
     if balanced(lst):
         print("Your list is successfully balanced! It has same number of evens and odds!!")
     else:
         print("Oh no! Sorry! Your list seems to be not balanced! Try another list please!")
def main():
     lst_1 = [1,2,3,5,6,8,5,9]
     lst_2 = []
     lst_3 = [2,4,5,7]
     lst_4 = [1,2,4,4]
     lst_5 = [1,2,3]
     result(lst_1)
     result(lst_2)
     result(lst_3)
     result(lst_4)
     result(lst_5)
main()

There's no need to keep counts at all.  All you need to do is keep track of whether the sequence is balanced or not as you check every element.  And the special tests you had for an empty list or odd list length are redundant.

def balanced(lst):
    tilt = 0
    for item in lst:
        if item % 2 == 0:      #even
            tilt += 1
        else:                  #odd
            tilt -= 1
    return tilt == 0

Or if you prefer terseness over readability, you can turn it into a one-liner.

def balanced(lst):
    return sum(1 if item % 2 else -1 for item in lst) == 0


How to break this curse by a pure functional recursive function? I have written two programs to find out a loopy path in a directed graphs.

The first version is a pure functional recursive solution, but its complexity is exponential. The second version following it achieves a linear complexity, but it doesn't look perfect either.

 def GetACycle(start: String, maps: Map[String, List[String]]): List[String] = {

    def explore(node: String, visits: List[String], steps: Int): List[String] = {
      println(List.fill(steps)("\t").mkString + node)
      if (visits.contains(node)) (visits.+:(node)).reverse
      else {
        if (maps(node).isEmpty) Nil
        else {
          val id = maps(node).indexWhere(x => !explore(x, visits.+:(node), steps + 1).isEmpty)
          if (id.!=(-1))
            explore(maps(node)(id), visits.+:(node), steps + 1)
          else
            Nil
        }
      }
    }

    explore(start, List(), 0)
  }

The second version uses mutable variables, the "visits" and "path", though it achieves a linear complexity in terms of number of visited nodes.

Would it be possible to achieve such a linear complexity in this situation while using a pure functional recursion without any mutable variables?

  def GetACycle2(start: String, maps: Map[String, List[String]]): List[String] = {
val nodes = maps.:\(Set[String]())((item, set) =>
  item._2.:\(set)(((i, set) => set.+(i))).+(item._1))
val pairs = nodes.toList.zip(List.fill(nodes.size)(false))

var visits = pairs.toMap
var path = List[String]()

def explore(node: String, steps: Int): Boolean = {
   println(List.fill(steps)("\t").mkString + node)
  path = path.+:(node)
  if (visits(node)) { visits = visits.updated(node, true); true }
  else {
    visits = visits.updated(node, true)
    if (maps(node).isEmpty) false
    else {
      maps(node).exists( x => explore(x, steps+1))
    }
  }
}

explore(start, 0)
path

}

What about something like this (Please excuse my style/form - As with the other poster, I'm not very familiar with coding in Scala either):

def GetACycle(start: String, maps: Map[String, List[String]]): List[String] = {

//entry function
def explore(node: String): List[String] = {
    explore_r(node, List(), List())
}

//main recursive
def explore_r(node: String, visited: List[String], path: List[String]): List[String] = {
    println(node)
    if (!maps.contains(node)) return Nil
    if (visited.contains(node)) return path++List(node)
    val branches = maps(node)
    for (nextnode <- branches){
        val loop = explore_r(nextnode, visited++List(node), path++List(node))
        if (loop != Nil) return loop
    }
    Nil
}
explore(start)
}


how to destructure this.props react js I'm trying to create a component that receives an array, this component is going to show a list with props data
const Mesas = [{
      id: 1,
      valor: 500,
      produtos:['COCA COLA', 'FANTA']
    },{
      id: 2,
      valor: 100,
      produtos:['PRATO1', 'PRATO2']
    }]
const App = () => {
    return (<ListaMesas mesas = {{...Mesas}} />)
}

The component:
class mesas extends Component {
    render(){
        const mesas = [this.props.mesas]
        mesas.map(function(item){
           return(
               <div key ={item.id}>
                   <text>{item.id}</text>
               </div>
           )
        })
        return(
            <div>
                {mesas}
            </div>
        )
    }
}

What is the best way to do this list?

I think the best way to refactor your problem is to use React programming pattern: a stateful, parent component passes down a prop to a stateless, child component. So, you don't ever need to declare mesas as a class, it is just a functional component because we dividing components into presentational components and container components.
But, I didn't get about the App's state, if you plan to use external storage fine, but if you need to manage the local state, just insert the mesas in your App under constructor. However, abstracting, based on your code, my  refactoring:
// App.js
import React from 'react';

const mesas = [{
      id: 1,
      valor: 500,
      produtos:['COCA COLA', 'FANTA']
    },{
      id: 2,
      valor: 100,
      produtos:['PRATO1', 'PRATO2']
    }]
const App = () => {
    return (<ListaMesas mesas = {mesas} />)
}

// Mesas.js
import React from 'react';

export const mesas = ({ mesas }) => {
    const mesasList = mesas.map((mesa) => {

            <text>{mesa.id}</text>

    })
    return (

            {mesasList}

    )
}


Bottleneck encountered returning records for my grid I am using MVC 4.0/C#/EF4.0 and I have a grid page that grabs results that are filtered, sorted and paged. It's filtering results from a table that has 45,000 records. How do I improve performance? This page takes over 30 seconds to load.

  var filteredresults = _unitOfWork.ModifiedDataRepository.Get();

            NameValueCollection nvc = HttpUtility.ParseQueryString(Request.RequestUri.Query);
            int ID = (String.IsNullOrEmpty(nvc["sSearch_0"]) ? 0 : Convert.ToInt32(nvc["sSearch_0"]));
            int TaxYear = (String.IsNullOrEmpty(nvc["sSearch_1"]) ? 0 : Convert.ToInt32(nvc["sSearch_1"]));
            int AdditionalTax = (String.IsNullOrEmpty(nvc["sSearch_2"]) ? 0 : Convert.ToInt32(nvc["sSearch_2"]));

            filteredresults = filteredresults.Where(i => (ID == 0 || i.ID == ID)
                                                                     && (TaxYear == 0 || i.TaxYear == TaxYear)
                                                                     && (AdditionalTax == 0 || i.AdditionalChildTaxCreditAmount == AdditionalTax)
                                                                     );

            int Count = filteredresults.Count();

            string sortCol = param.sColumns.Split(',')[param.iSortCol_0];
            string sortExpression = sortCol + " " + param.sSortDir_0;

            filteredresults = filteredresults.OrderBy(sortExpression);
            filteredresults = from p in filteredresults
                              orderby (sortExpression)
                              select p;

            var parents = filteredresults.Skip(param.iDisplayStart).Take(param.iDisplayLength);

            var result = from c in parents.ToList()
                         select new MODIFIED_DATA()
                         {
                             ID = c.ID,
                             TaxYear = c.TaxYear,
                             AdditionalChildTaxCreditAmount = c.AdditionalChildTaxCreditAmount,
                             ArrivalDate = c.ArrivalDate,
                             ChildCareCredit = c.ChildCareCredit,
                             DLN = c.DLN,
                             EftCd = c.EftCd,
                             EicAmt = c.EicAmt,
                             ElfSourceCd = c.ElfSourceCd,
                             FilingStatusCd = c.FilingStatusCd,
                             MakingWorkPayCreditAmount = c.MakingWorkPayCreditAmount,
                             PrimPrisonerNmCd = c.PrimPrisonerNmCd
                         };

            var CustomerPaged = new DataTablePager<MODIFIED_DATA>();

            CustomerPaged.sEcho = param.sEcho;
            CustomerPaged.iTotalRecords = Count;
            CustomerPaged.iTotalDisplayRecords = Count;
            CustomerPaged.aaData = result.ToList();

            return CustomerPaged;

Obvious slowdowns

for each property of NameValueCollection which is set, the condition can ommit the ==0 check.
you are sorting the results 2 times
using .Count(), but this seems to be needed.
calling unneccessary .ToList() on parents
posibly unneccessary call of .ToList() on result. Here you should check if CustomerPaged.aaData can take an IEnumerable<MODIFIED_DATA>

Naming

result is an IEnumerable<MODIFIED_DATA> so the plural should be used -> results
the kind of hungarian notation you use is seen as bad practice
local variables should be named using camelCase casing

General

Consider to extract the "index" strings, like "sSearch_0", into class const variables


Network arithmetic, finding the next host for the given Disclaimer: It's literally the second time I write in Haskell.

The purpose of this module is to provide some basic operations with IP addresses (and networks).

At the moment I only implemented the successor operation, that returns a next address in the range if it exists.

Eg: for 192.168.1.1 in 192.168.1.0/24 it would return Maybe 192.168.1.1

And for 192.168.1.255 (which is the last valid IP address in the given network) it would return Nothing.

module Main where

import Data.IP (IPv4, AddrRange(addr), fromIPv4, toIPv4, isMatchedTo, makeAddrRange)
import Data.Bits (shiftL, shiftR, (.|.), (.&.))
import Data.List (foldl')
import Text.Read (readMaybe)
import qualified Safe as S

parse :: String -> Maybe IPv4
parse = readMaybe

toInt :: IPv4 -> Int
toInt i = foldl' accum 0 (fromIPv4 i)
    where accum a v = (a `shiftL` 8) .|. v

fromInt :: Int -> IPv4
fromInt i = toIPv4 $ map (\s -> (i `shiftR` s) .&. 0xff) [24, 16, 8, 0]

addressesInNetwork :: AddrRange IPv4 -> [IPv4]
addressesInNetwork i = takeWhile (`isMatchedTo` i) adds
    where net = toInt $ addr i
          adds = map (\s -> fromInt $ net + s) [0..]

successor :: Int -> IPv4 -> Maybe IPv4
successor r a = S.headMay $ dropWhile (<= a) $ addressesInNetwork $ makeAddrRange a r

net24 :: IPv4 -> Maybe IPv4
net24 = successor 24

main::IO()
main = print $ parse "192.168.1.201" >>= net24

What I don't like: it just looks terrible. I look on it and see Perl, not Haskell.

Have I done something idiomatically wrong or my sense of beauty is broken?

I think what you've missed is the Enum instance for the IPv4 datatype from iproute. Using methods from Enum and Bounded you can eliminate the use of Data.Bits in your code.

Because IPv4 is Enum, your toInt method is just fromEnum :: (Enum a) => a -> Int. And similarly your fromInt is toEnum.

addressesInNetwork is largely the same logic, you just let someone else do all the work.

addressesInNetwork :: AddrRange IPv4 -> [IPv4]
addressesInNetwork range = takeWhile (`isMatchedTo` range) [low..]
    where
        (low, _) = addrRangePair range

I would use addrRangePair instead of the addr field accessor for two reasons. First, addr doesn't show up in the Haddock documentation for Data.IP which is a bit confusing. Second, on the off-chance that the author of iproute begins hiding the constructors of AddrRange from export (which is very common practice in Haskell so that library authors can change implementations without breaking users' code) you'll be safe from breaking changes.

Knowing the Enum instance and available Data.IP functions, you can now write a clearer version of successor. Unfortunately we have to be a bit cautious of bounds, Enum throws an exception if you try to get the successor of maxBound, but it's not too bad.

successor :: AddrRange IPv4 -> IPv4 -> Maybe IPv4
successor range ip | ip == maxBound              = Nothing
                   | succ ip `isMatchedTo` range = Just $ succ ip
                   | otherwise                   = Nothing

Notice I changed the type signature of your function to take an AddrRange instead of a mask length. This is kind of a separation of concerns issue, successor doesn't care about the mask length, what matters is whether an IP falls within the range it defines. So net24 would be implemented as—

net24 :: IPv4 -> Maybe IPv4
net24 ip = successor (makeAddrRange ip 24) ip

(That's kind of a strange name though, maybe successorMask24 instead?)


Using Pandas to parse adwords export I did this exercise yesterday mostly as practice, but it has some utility as well in day to day. I was basically attempting to take a string that looked like the following:
src=bing_adid=8312488564_kw=swiftcapital com_kwid=35030235383_mt=e_qs=swiftcapital.com_device=c'

And create a series of columns based on the parameters within the string. Here you can see src, kw, kwid, mt, etc.
I was given a set of rules for the string, and used those in my assumptions; as follows:

Exercise 1: Parsing out variables

Any of the ten variables might appear in a string (src, adid, kw, kwid, mt, dist, qs, adpos, device, placement).

The value for that parameter will always be after the equal sign, i.e. src=bing (bing is the value being parsed).

At this point, the length is inconsistent for any given parameter.

Not every parameter will appear in every string.

Not every parameter will appear in the same position in every string.

The parameters will always be delimited with an underscore.

Exercise 2: Absence of parameters (catch all)

If none of that ten parameters appear, then it should all be captured into a catch-all column (i.e., Site Link).

In addition, I used the assumption that an _ and = character both could occur in a string which is why I elected to use regular expressions (this was as much for practical utility as it was just to teach myself some more regex.)
Here's my solution using pandas, csv, and re, please let me know if you can think of an easier/smarter way in python. (I have a feeling I could have hard-coded this using things like FIND() in excel but I want to avoid Excel whenever possible.
# we use pandas and regular expression libraries
import pandas as pd
import re

# dropping all but the 'input' column; dropping all rows in that column that are NaN
df = pd.read_csv('Parsing.csv')
df = df[df['input'].notnull() == True]
df = df[['input']]

# defining params to search for in our keywords
parameters = ['src', 'adid', 'kw', 'kwid', 'mt', 'dist', 'qs', 'adpos', 'device', 'placement']
new_params = [params+'=' for params in parameters]

#split apart parsed data will be stored here in a mock_dataframe which we will reinject to pandas
mock_df = {'comb_str=': [],
        'src=': [],
         'adid=': [],
         'kw=': [],
         'kwid=': [],
         'mt=': [],
         'dist=': [],
         'qs=' : [],
         'adpos=' : [],
         'device=' : [],
         'placement=' : [],
          'catchall' : []}

# nested for loop that looks for the conditions using regular expressions and appends to our mock_df
a = [x for x in df['input']]
for entry in a:
    mock_df['comb_str='].append(entry)
    if any(params in entry for params in new_params):
        mock_df['catchall'].append('')
        for params in new_params:
            if params in entry:
                mock_df[params].append(re.search(params+'(.*?)(?=_[^_=\n]+=|$)',entry).group(1))
            else:
                mock_df[params].append('')
    else:
        for params in new_params:
            mock_df[params].append('')
        mock_df['catchall'].append(entry)

# to get an idea what the new df looks like
df_upd = pd.DataFrame(data=mock_df)
df_upd

# port new DF to a csv for excel use.
df_upd.to_csv(path_or_buf='parsed.csv')

I'm hoping to get help on removing complexity in my code, and if possible making the code more efficient. I would imagine an input might be 1mil or so of the above strings, with 1mil*#of params defined columns as an output. So it can get messy in a hurry.
comb_str is just re-importing the string itself and then adding it back to the DF in case it's needed to be used in an index or something later.

You don't need to comment so often. Comments are good for explaining code or the abstract reason behind code, if either of those is unclear. Your comment about imports isn't necessary:

# we use pandas and regular expression libraries
import pandas as pd
import re

The code is clear, people using Python know what imports are and so this will be perfectly readable. On the other hand, your note about how you're filtering the pandas data is useful to me. It may be just because I don't use pandas, but the explanation of these commands cleared code that otherwise made no sense.

You could make your mock_df much quicker with a dictionary comprehension of your new_params list:

mock_df = {key: [] for key in new_params}

Of course this leaves out comb_str and catchall, but you can add those manually after.

mock_df['comb_str'] = []
mock_df['catchall'] = []

If you need a copy of df['input'] to iterate over a list comprehension is a slow way. Depending on how different dataframes are you might be able to use copy from the copy module.

from copy import copy

...

for entry in copy(df['input']):

If that doesn't seem to work, I also see pandas has its own copy function.

But it's unclear why you need to duplicate it. THe data shouldn't be affected by looping over it. And in any case you don't use it or your copy after this point.

Also, you append every entry to mock_df['comb_str='], why not just assign the full list to that after the loop? It'd save you time appending on each iteration.

Using params as your name for each element of new_params is quite confusing. Surely you only have one parameter at a time that you're using? Having that as a plural throws me off and makes me think it's a list of parameters.

I think you're making the flow a bit complicated too. Instead of using any try looping over all your new_params and then set a flag if a parameter is found. Then after the loop test if this flag was ever set, if not you could append to 'catchall'. Here's what I mean:

found_param = False
for params in new_params:
    if params in entry:
        mock_df[params].append(re.search(params+'(.*?)(?=_[^_=\n]+=|$)', entry).group(1))
        found_param = True
    else:
        mock_df[params].append('')
mock_df['catchall'].append('' if found_param else entry)

This makes sure that every value is being set and it's clearer to see that happening. Also in case you're not familiar with them, I'm using a ternary expression to append to 'catchall'.

('' if found_param else entry)

A ternary will return one of 2 values based on an evaluated boolean. In this case the boolean is found_param, and if it's True then it returns the first value, which is an empty string. However if it's False then it will be set as entry instead. You can just use the normal if syntax if you prefer. Opinions differ on whether ternaries are more readable.


"Tara's Beautiful Permutations" challenge I'm practising my coding skills using Python to answer medium to difficult problems on sites like HackerRank.
Here is the challenge. Given an array of integers, where each number can appear at most twice, count the number of permutations in which no identical numbers appear consecutively.
My solution solves the base test but not the others because it is too slow. That's okay for now (I find the actual solution difficult to understand, but I first would like feedback to my own solution).
My questions today are:

What is the time complexity of my algorithm? I have commented on what I am doing, best to start from the bottom up.
I think, given n elements in the array, I have two for loops and it seems like a n*n! solution for one array, but I would really appreciate a clear explanation here on Big O for this particular piece of code.

I am new to Python's yield and I actually got this part of the solution from another SO post. I just want to know, have I used yield efficiently? What time/efficiency difference would it have made if I didn't use yield in this case and just made a list perm_lst then and there, to hold all unique permutations?

# Enter your code here. Read input from STDIN. Print output to STDOUT

def getBeautifulPerms(arr, arrlen):

    if (arrlen==1): yield arr
    else:
        for perm in getBeautifulPerms(arr[1:], arrlen-1):
            for i in range(arrlen):
                arr_before = perm[:i]
                arr_after = perm[i:]

                if ( (not(arr_before) or arr_before[-1] != arr[0])) and (not(arr_after) or arr_after[0] != arr[0]):
                    yield perm[:i] + [arr[0]] + perm[i:]

def printBeautifulPerms(arr, n):
    '''
    @param arr array of integers (not null)
    @param n length of array arr
    post: prints the number of good permutations mod (10**9 + 7)
    '''

    perm_lst = []
    for perm in getBeautifulPerms(arr, n):
        if perm not in perm_lst:
            perm_lst.append(perm) # remove duplicates

    print(len(perm_lst) % (10**9 + 7))

# main code - the problem states we'll receive this input:
q = int(input()) # no of queries (arrays)

for i in range(q):
    n = int(input()) # no of elements in array
    arr = input () # space separated array of n integers
    arr = arr.split()
    printBeautifulPerms(arr, n)

Well, the first and most obvious improvement is how you remove duplicates. Currently you iterate over all beautiful permutations (at least \$\mathcal{O}(n)\$) and for each check if it is already in the list (\$\mathcal{O}(n)\$ again). This is already \$\mathcal{O}(n^2)\$.

If you used a set instead, that in check would be \$\mathcal{O}(1)\$:

def print_beautiful_perms(arr, n):
    """
    Prints the number of beautiful permutations mod (10**9 + 7)

    arr: array of integers (not null)
    n: ?
    """
    permutations = set(map(tuple, get_beautiful_perms(arr, n)))
    print(len(permutations) % (10**9 + 7))

The map(tuple, ...) is needed because lists are not hashable in Python.

I also renamed your functions to adhere to PEP8, Python's official style-guide and reworked the docstring a bit to conform more to the conventions laid out in PEP257.

You should also put your main code under a if __name__ == "__main__" guard to allow importing your functions from another script without executing it:

if __name__ == "__main__":
    # main code - the problem states we'll receive this input:
    q = int(input())    # no of queries (arrays)

    for _ in range(q):
        n = int(input())    # no of elements in array
        arr = input().split()    # space separated array of n integers
        print_beautiful_perms(arr, n)

I also used _ as the unneeded loop variable, as is the convention.

I would also use try a different way to check if a permutation is beautiful and use itertools.permutations:

from itertools import permutations

def is_beautiful(arr):
    return all(x != y for x, y in zip(arr, arr[1:]))

def get_beautiful_perms(arr, arrlen):
    return filter(is_beautiful, permutations(arr))

Of course this also times out, because itertools.permutations is also \$\mathcal{O}(n!)\$.