Looking for performance improvement of my custom dataframe aggregation function (Python/Pandas) I would like to aggregate rows of my dataframe together, following those rules:

Rows are grouped by "Id", and sorted by anticipation
One row can only be aggregated with rows next to it, with the same "Id"
The "Anticipation" of the new row is the weighted mean of the "Anticipation" (weighted by "Size") of aggregated rows
One row will be included in a group of rows to be aggregated to if, without the new row, the sum of "Size" is inferior or equal to "max_size"

In other words, with this dataframe as input:

   Anticipation   Id  Size
0            10  foo    10
1             9  foo    11
2             8  foo    30
3            10  bar    10
4             9  bar     9
5             8  bar    10
6            10  baz     7

and max_size = 10, the function should return this:

    Id  Size  Anticipation
0  bar    19      9.526316
1  bar    10      8.000000
2  baz     7     10.000000
3  foo    21      9.476190
4  foo    30      8.000000

I'm looking for an improvement of my function's performance (through a more idiomatic pandas coding?).

Here is my current code

import pandas as pd
records = [{"Id": "foo", "Size": 10, "Anticipation":10},
{"Id": "foo", "Size": 11, "Anticipation":9},
{"Id": "foo", "Size": 30, "Anticipation":8},
{"Id": "bar", "Size": 10, "Anticipation":10},
{"Id": "bar", "Size": 9, "Anticipation":9},
{"Id": "bar", "Size": 10, "Anticipation":8},
{"Id": "baz", "Size": 7, "Anticipation":10}]

df = pd.DataFrame(records)
max_size = 10

def assembly_lines(df, max_size):

    df.sort_values("Anticipation", ascending=False)
    df["Cumsum"] = df[["Id", "Size"]].groupby(["Id"]).cumsum()
    df["Anticipation*Size"] = df["Anticipation"] * df["Size"]

    L_fin = [] # L_fin stores aggregated rows.

    for name, group in df.groupby(["Id"]): # Group "foo", "bar", "baz" together
        i = 0
        L = [] # L will temporarly stores rows before aggregation

        for index, row in group.iterrows():

            L.append(row.to_dict()) # Stores row in L

            if row["Cumsum"] > i + max_size: # If cumulated size of all rows in L is above maximal size authorized

                i = row["Cumsum"]
                temp = pd.DataFrame.from_dict(L).drop(["Cumsum", "Anticipation"], axis=1) \
                    .groupby("Id") \
                    .agg({"Size": "sum", "Anticipation*Size": "sum"}) #  Then rows are aggregated
                temp["Anticipation"] = temp["Anticipation*Size"] / temp["Size"] # Mean anticipation is calculated
                L_fin.append(temp) # Aggregated row is added to aggregated rows list
                L = [] # Bucket is emptyied

        if L: # If no rows remains in the grouped df

            temp = pd.DataFrame.from_dict(L).drop(["Cumsum", "Anticipation"], axis=1) \
                .groupby(["Id"]) \
                .agg({"Size": "sum", "Anticipation*Size": "sum"}) # Rows in the bucket are aggregated
            temp["Anticipation"] = temp["Anticipation*Size"] / temp["Size"]
            L_fin.append(temp) # And added to L_fin

    df = pd.concat(L_fin).drop(["Anticipation*Size"], axis=1).reset_index()

    return df

Normally, I really despise the fact that itertools.groupby only groups adjacent elements with the same key... in your case, though, this seems ideal. Forgive me for just rewriting the code rather than critique'ing what you have, but using this grouping function completely changes how the overall task is best approached.

Let's use itertools.groupby to perform the grouping rather than pandas.groupby specifically because of this adjacency behavior:

In [1]: grouped = itertools.groupby(df.itertuples(False), key=lambda x: x.Id)

In [2]: {k: list(v) for k, v in grouped}
Out[2]:
{'foo': [Pandas(Anticipation=10, Id='foo', Size=10),
  Pandas(Anticipation=9, Id='foo', Size=11),
  Pandas(Anticipation=8, Id='foo', Size=30)],
 'bar': [Pandas(Anticipation=10, Id='bar', Size=10),
  Pandas(Anticipation=9, Id='bar', Size=9),
  Pandas(Anticipation=8, Id='bar', Size=10)],
 'baz': [Pandas(Anticipation=10, Id='baz', Size=7)]}

Note that non-adjacent rows won't be aggregated together:

In [3]: [list(v) for _, v in itertools.groupby([1, 1, 2, 1], key=lambda x: x)]
Out[3]: [[1, 1], [2], [1]]

So then we just need a custom aggregation function. Let's use a generator function that can produce multiple outputs per input, so that we can aggregate each group and output one or more rows as appropriate. It'll operate on a single adjacent group of rows and handle the funkiness of your grouping logic:

def funky_aggregate(k, vs, max_size=10):
    cur_size = 0
    cur_ant = 0
    for v in vs:
        cur_size += v.Size
        cur_ant += v.Anticipation * v.Size
        if cur_size > max_size:
            yield {'Id': k, 'Size': cur_size, 'Anticipation': cur_ant / cur_size}
            cur_size = cur_ant = 0
    if cur_size != 0:
        yield {'Id': k, 'Size': cur_size, 'Anticipation': cur_ant / cur_size}

This can then be easily joined back together into a dataframe:

In [4]: pd.DataFrame([row
            for key, group in itertools.groupby(df.itertuples(False), key=lambda x: x.Id)
            for row in funky_aggregate(key, group)
        ])
Out[4]:
   Anticipation   Id  Size
0      9.476190  foo    21
1      8.000000  foo    30
2      9.526316  bar    19
3      8.000000  bar    10
4     10.000000  baz     7


Fully Generic C++ Stack Implementation Without Lists, Arrays, or Vectors I'm trying to write a fully generic Stack in C++.  I'm not sure if this is the best way to do this.

Right now I know that the Pop method needs improvement since if T is a value type, returning a nullptr will cause a compiler error.  Also, I'm pretty sure that this can be optimized in a few ways, but I don't see any obvious areas at the moment.  Are there any other specific ways this can be improved?

Any and all feedback is appreciated.

#ifndef STACK_H
#define STACK_H

template <class T>
class Stack
{
public:
    Stack();
    Stack(T& data);
    ~Stack();

    // Public Stack operations
    void Push(T& data);
    T Pop();
    void Clear();

    // Accessors
    const T& Top() const { return m_top->m_data; }
    const T& Bottom() const { return m_bottom->m_data; }
    bool IsEmpty() const { return m_size == 0; }
    int Size() const { return m_size; }

private:
    template <class T> struct StackNode
    {
        StackNode(T& data)
        {
            m_data = data;
            m_nextNode = nullptr;
        }

        T m_data;
        StackNode<T>* m_nextNode;
    };

    int m_size;
    StackNode<T>* m_top;
    StackNode<T>* m_bottom;
};

template <class T> Stack<T>::Stack()
{
    m_top = nullptr;
    m_bottom = nullptr;
    m_size = 0;
}

template <class T> Stack<T>::Stack(T& data)
{
    m_top = m_bottom = new StackNode<T>(data);
    m_size = 1;
}

template <class T> Stack<T>::~Stack()
{
    Clear();
}

template <class T> void Stack<T>::Push(T& data)
{
    StackNode<T>* newNode = new StackNode<T>(data);
    newNode->m_nextNode = m_top;
    m_top = newNode;
    ++m_size;
}

template <class T> T Stack<T>::Pop()
{
    if (!IsEmpty())
    {
        T returnData = m_top->m_data;

        StackNode<T>* temp = m_top->m_nextNode;
        delete m_top;
        m_top = temp;
        --m_size;
        return returnData;
    }

    // there is an issue here because if T is a value type, this will cause an error
    //return nullptr;
}

template <class T> void Stack<T>::Clear()
{
    if (!IsEmpty())
    {
        StackNode<T>* next = nullptr;
        do
        {
            next = m_top->m_nextNode;
            delete m_top;
            m_top = next;
            --m_size;
        } while (m_top != nullptr);

        m_top = m_bottom = nullptr;
    }
}

#endif

The best thing to do if the caller tries to Pop when the stack is empty is to throw an exception. You're right, there's no return value that makes sense.

You have a bug in void Stack<T>::Push(T& data). If the stack is initially empty, the m_top and m_bottom pointers are nullptr, but if an item is pushed onto an empty stack, the m_bottom pointer is never set.

There's no reason for Stack<T>::Clear() to decrement m_size within the loop. Just set it to 0 at any time. (Though, the compiler may be smart enough to notice this)


Finding the root of a function by Bisection Method The program has to look for a root in an interval [a,b]. The root should be declared with a certain accuracy eps. I.e it should look for a part-interval in [a,b], which has the length of eps.

The bisection algorithm should be:

Save the interval boundaries
Look if [a,b] has a root. (original given interval)
look if a-b < eps. If yes, part-interval found.
If no, divide [a,b] in half and continue with point 2. etc.

(We can assume that there is already a root in the given original interval [a,b])

I've tested it with following functions:

log x + 2

x*x*x

-3.*x*x + x + 2.

Are there any improvements possible or did I make a big mistake?

Bisection.cpp

#include "Bisection.hpp"
#include <cmath>

using namespace std;

void bisection(std::vector<double>& interval, double eps) {
  double a;
  double b;
  double c;

  a = interval[0];
  b = interval[1];

  if (b - a < eps) // interval already 'found'
  {
    return;
  }

  do
  {
    c = (a + b) / 2.0;

    if (f(c) == 0)
    {
      a = c;
      b = c;
    }
    else
    {
      if (f(a) * f(c) > 0)
      {
        a = c;
      }
      else
      {
        b = c;
      }
    }

  } while (f(c) != 0 || b - a > eps);

  /*found part-interval with the length of < eps*/
  interval[0] = a;
  interval[1] = b;
}

A std::vector<T> vec is an arbitrary collection of Ts. vec might contain none, one or many elements. However, you want to model an interval, which consists of exactly two elements. A std::vector<T> therefore isn't the correct container for an interval.

You could use std::pair<double, double> instead, or just write your own variant:

struct interval_type {
    double lower_bound;
    double upper_bound;
};

Next, try to initialize the variables instead of just declaring them. While we're at it, try to come up with better names, e.g. instead of

  double a;
  double b;
  double c;

  a = interval[0];
  b = interval[1];

write

  double lower = interval.lower_bound; // if you use the struct from above
  double upper = interval.upper_bound;

c's scope is too large, by the way. It's just necessary inside the loop, so we can limit its life time, but we need to break in this case:

  do
  {
    const double midpoint = (lower + upper) / 2.0;

    if (f(midpoint) == 0)
    {
      a = midpoint;
      b = midpoint;
      break;
    }
    else
    {
      if (f(lower) * f(midpoint) > 0)
      {
        lower = midpoint;
      }
      else
      {
        upper = midpoint;
      }
    }
  } while (upper - lower > eps);

Other than that, the test f(value) == 0 is usually too strict, since floating point arithmetic can be a finicky beast. std::abs(f(value)) <= some_eps works well enough most of the times.

Other than that, I suggest you to allow other functions than a fixed f. Something along

template <typename F>
interval_type bisection(F && f, interval_type interval, double eps = 1e-8){

}

comes to mind. As you can see, there is no reason to change the interval in place. Just return a new one.


Segmenting a color wheel I'm a fan of elegant ways to condense code. I find this ugly. Been staring at it for a while, can't think of a way to write this with less verbosity (ok, short of shorter var names but that's not what I mean). What do you guys think?

if angle < 60:
    red = 1
    green = angle / 60
    blue = 0
elif angle < 120:
    red = 1 - (angle - 60) / 60
    green = 1
    blue = 0
elif angle < 180:
    red = 0
    green = 1
    blue = (angle - 120) / 60
elif angle < 240:
    red = 0
    green = 1 - (angle - 180) / 60
    blue = 1
elif angle < 300:
    red = (angle - 240) / 60
    green = 0
    blue = 1
else:
    red = 1
    green = 0
    blue = 1 - (angle - 300) / 60

Replying to a request for context:

# a color wheel is drawn using aggdraw like so (the list of 360 colors I've obviously omitted but is contained in 'colors', as are several self-evident vars

for i in range(360):
            brush = aggdraw.Brush(colors[i])
            center = surface_width // 2
            inner_radius = radius - 2
            surface.polygon((center, center,
                           int(round(inner_radius + math.sin((rotation - .25) * PI / 180) * inner_radius)),
                           int(round(inner_radius + math.cos((rotation - .25) * PI / 180) * inner_radius)),
                           int(round(inner_radius + math.sin((rotation + 1.25) * PI / 180) * inner_radius)),
                           int(round(inner_radius + math.cos((rotation + 1.25) * PI / 180) * inner_radius))),
                          brush)
            rotation += 1
            if rotation > 360:
                rotation -= 360

Not a proper methodology nor a code-review but I'll try to explain what I've done.

First, I've put your code in a function returning the (red, green, blue) tuple.

Then, I wrote a tiny bit of code to find the different values taken by the different colors :

prev_t = None
for i in range(362):  # Going a bit further than 360 just out of curiosity
    red, green, blue = foo(i)
    t = blue  # Change this if you care about a different color
    if t != prev_t:
        print(i, t)
    prev_t = t

For the blue color, I got :

(0, 0)
(180, 1)
(360, 0)

which led me to think that the blue value was 1 for values bigger than 180 (because the comments talked about wheels, it made sense to apply a modulo 360).

I got a bit lucky because I started with the blue color which was the easiest. Now, the blue value can be computed with :

angle_blue = angle % 360
blue = angle_blue >= 180

Then, I applied a pretty similar trick to other colors by changing the line t = blue and I realized that they could be computer with a similar logic but with an additional shifting.

Now, the code can be written :

def foo2(angle):
    angle_blue = angle % 360
    blue = angle_blue >= 180

    angle_green = (angle - 60) % 360
    green = angle_green < 180

    angle_red = (angle - 120) % 360
    red = angle_red >= 180

    return red, green, blue

(This code could be written in a more concise way but this version corresponds to the fact that I've progressed by handling one color at a time).

This seems to be working for all tested values as per the assert added to the loop I originally wrote :

for i in range(362):  # Going a bit further than 360 just out of curiosity
    red, green, blue = foo(i)
    red2, green2, blue2 = foo2(i)
    assert red == red2
    assert green == green2
    assert blue == blue2


Find a String in an Array This program is used to get names and numbers of N person, print out the numbers of a person. Please help me improve this code.

Additional Question:

To avoid a very long name that exceeds char's size, I limit it to 255. Is there a better way?

To avoid buffer, I included a space before % sign when using scanf and not use getchar(). Is there a better way?

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

// Limiting string input
const unsigned int STRINGCONSTRAINT = 255;

// Person struct to store name and numbers
typedef struct {
  char name[STRINGCONSTRAINT];
  char numbers[STRINGCONSTRAINT];
} person;

void getNameNumber(person people[], int size);
void printNumber(person people[], int size, char personFind[]);

// Asked for array's size and a person's name to find
int main(void) {
  unsigned int size = 0;
  printf("Enter the amount of people: ");
  scanf(" %d", &size);

  person people[size];
  getNameNumber(people, size);

  char personFind[STRINGCONSTRAINT];
  puts("Who do you want to find?");
  printf("Person to find: ");
  scanf(" %[^\n]s", personFind);

  printNumber(people, size, personFind);

  return 0;
}

// Get name and numbers for the array
void getNameNumber(person people[], int size) {
  unsigned int i;
  for (i = 0; i < size; i++) {
    printf("Name: ");
    scanf(" %[^\n]s", people[i].name);
    printf("Numbers: ");
    scanf(" %[^\n]s", people[i].numbers);
  }
}

// Find the person and print the numbers
void printNumber(person people[], int size, char personFind[]) {
  unsigned int i;
  int flag = 1;

  for (i = 0; i < size; i++) {
    flag = strcmp(people[i].name, personFind);
    if (flag == 0) {
      puts("Found!");
      printf("You can call %s at %s.\n", personFind, people[i].numbers);
    }
  }
}

static functions

Both of your getNameNumber and printNumber should be made static, because I doubt that you plan to export them.

Simpler functions

printf("Enter the amount of people: ");

can simply use puts, which doesn't have any formatting code in it. You use puts below, so this would also help with consistency.

Stack allocation

person people[size];

is not available in all versions of C, since size is not a constant. For best portability you'd want to malloc this, or perhaps alloca if you feel strongly that the memory should be in the stack and not on the heap.

const

printNumber's array arguments should be const, since you don't mutate them.

Your questions

  To avoid a very long name that exceeds char's size, I limit it to 255. Is there a better way?

For your use case, not really. A fixed array is the most appropriate. However, your code is vulnerable to overflows. You should be putting a width in your format specifier.

  To avoid buffer, I included a space before % sign and not use getchar(). Is there a better way?

I'm not sure which usage of % you're referring to, but they all look fairly sane, so no; there's not really a better way.


Overloading the new operator This may be a naive question, but what are the dangers of the following code (allocate more memory in the new operator for the info struct)?

struct Info
{
        Info():nLineNo(0){memset(cFile, 0, 256);}
        ~Info(){}
        char cFile[256];
        unsigned int nLineNo;
};

void *operator new(size_t Size, const char *pcFile, const unsigned int nLine)
{
        Info *pData = 0;

        pData = (Info*)malloc(Size + sizeof(Info));

        strcpy(pData->cFile, pcFile);

        pData->nLineNo = nLine;

        return (void*)pData;
}

void operator delete(void *pData)
{
        Info *pInfo = (Info*)pData;
    // could use pInfo->nLineNo to check for it within hash table of allocated blocks - just an example
        free(pData);
}

You are not overloading the new operator correctly.
See: How should I write ISO C++ Standard conformant custom new and delete operators?
But you should not be overloading new at all.
All the work you are doing here is stuff that goes in the constructor.
The sole job of the new operator is to allocate memory (not initialize it).
As a side note:
Though you can actually add parameters to new.
Doing so is not a good idea.
As it is not normal, and thus makes it harder to maintain.
The code above will be called if you do:
// Note: The first parameter (ie size) will be auto inserted as the first parameter.
Info* data = new ("File Name", 12) Info;


class IMessge - returns user message types | loop similar items This class holds message types that are sent to the client which knows what to do with them.  It is basically a small registry pattern where I can consolidate message types. |P| and |F| are read by the client...and I use these for simple data passing when I don't need the complexity of JSON.  The programmer is expected to know which messages types are available.

I've added in a array_key_exists() as a defensive measure against a progammer calling a message that does not exit.  I did this as someone said this is best practice...I'm looking for a second opinion...as in this case it is not a big deal but overall "defensive coding" would increase my library size / efficiency by 10-20%.  Can I take this out?

<?php
class IMessage
{
    private $PASS = '|P|';
    private $FAIL = '|F|';
    private $messages = array();
    public function __construct()
    {
        $this->messages['PASS'] =  $this->PASS;
        $this->messages['name'] =  $this->FAIL . 'name';
        $this->messages['email'] = $this->FAIL . 'email_s';
        $this->messages['pass'] =  $this->FAIL . 'pass';
        $this->messages['url'] =   $this->FAIL . 'url';
        $this->messages['title'] = $this->FAIL . 'title';
        $this->messages['tweet'] = $this->FAIL . 'tweet';
        $this->messages['empty'] = $this->FAIL . 'empty';
        $this->messages['same'] =  $this->FAIL . 'same';
        $this->messages['taken'] = $this->FAIL . 'taken';
        $this->messages['validate'] = $this->FAIL . 'validate';
    }
    public function get( $type )
    {
        if( !array_key_exists($type, $this->messages ) )
        {
            echo "Programmer Error: You called a key that does not exist";
        }
        else
        {
            return $this->messages[ $type ];
        }
    }
}

Well, you are doing this a little redundantly...

$keys = array(
    'name',
    'email',
    //etc...
);

$messages = array(
    'PASS' => $this->PASS,
);

foreach( $keys AS $key ) { $messages[ $key ] = $this->FAIL . $key; }

$this->messages = $messages;

Also, your get function has a fatal error. You currently have a check to see if a key exists, and if it doesn't it will output an error message. This is incomplete, if that key doesn't exist it will output the error message then PHP will throw an undefined index error because $key does not exist. To avoid this you should have it return that error message when you set it or have it return FALSE. I think such a check is good, since it seems you are dealing with user supplied information. If this was a private method and you knew the information you were passing it was reliable, then I would say its unnecessary.


SPOJ - Alphacode - Exceeds Time Limit I'm trying to solve Alphacode on SPOJ.  The challenge is to count the ways to split a string of up to 5000 digits into a sequence of numbers, each ranging from 1 to 26.

It works fine for small numbers but exceeds the time limit for bigger numbers. I'm using the exhaustive recursive traversing.

Is there a way to make it optimal?

import java.util.HashSet;
import java.util.Scanner;

public class AlphaCode {

    static int n;
    static HashSet<String> set;

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);

        while (sc.hasNext()) {
            String s = sc.nextLine();
            if (s.equals("0")) {
                break;
            }
            n = s.length();
            char[] c = new char[n];
            set = new HashSet<String>();
            count = 0;
            solve(s, 0, c);
            System.out.println(count);
        }
    }

    static int count = 0;

    private static int solve(String s, int i, char[] st) {
        if (n == i) {
            // System.out.println(st);
            if (set.contains(st.toString())) {
                return 0;
            }
            count++;
            return 0;
        }
        for (int j = 0; j < 2 && i + j < n; ++j) {
            int x = Integer.parseInt(s.substring(i, i + j + 1) + "");
            if (x <= 26) {
                // System.out.println(i + " " + j + " " + s.substring(i, i + j +
                // 1));
                st[i] = (char) (x + 'A' - 1);
                solve(s, i + j + 1, st);
            }
        }
        return 0;
    }

}

Useless code

Currently, your code has some useless parts to it:

You have a set, but you never add anything to it.  When you reach the end of the string, you check your set for it containing something, but it never does.  You can just remove that whole set and do count++ when you reach the end of the string.  The way you are parsing the input, there is no way you can ever reach the same output string more than once.
Your solve() function always returns 0.  And nobody even checks the return value.  So you should make your solve() function return void.

Incorrect answer

Currently, your program will return an incorrect answer if count ever increases past \$2^{31}\$.  The question warned about this, so you should make count be long instead.

Too slow

The problem with your program is that you are just computing the answer in the brute force way, by generating every possible output.  This problem requires a dynamic programming technique to solve faster.  In simple terms, you are solving the same subproblem repeatedly.  If you just remembered the answer for each subproblem you already solved, then things would run much faster.

Here is an example of how I would fix your program to run faster:

import java.util.Scanner;

public class AlphaCode {
    static int n;
    static long [] solved;

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);

        while (sc.hasNext()) {
            String s = sc.nextLine();
            if (s.equals("0")) {
                break;
            }
            n = s.length();
            solved = new long[n];
            for (int i=0; i<n; i++) {
                solved[i] = -1;
            }
            long ret = solve(s, 0);
            System.out.println(ret);
        }
    }

    private static long solve(String s, int i)
    {
        long count = 0;

        if (i >= n) {
            return 1;
        }

        if (solved[i] != -1) {
            return solved[i];
        }

        for (int j = 0; j < 2 && i + j < n; ++j) {
            int x = Integer.parseInt(s.substring(i, i + j + 1));
            if (x <= 26) {
                count += solve(s, i + j + 1);
            }
        }
        solved[i] = count;
        return count;
    }

}


Class to parse a file sanitizing each line with a set of methods I am parsing a set of lines from a file, after reading the file content with Parser.read_file() I apply a set of methods to solve different issues each line may contain, like removing new lines, comments etc, and set a list of lines in the parser self.lines instance variable.

Filter methods are defined in Parser.filter as a list of names and executed after the file is read with self._sanitize().

import re
import os
import io
from operator import methodcaller

class Parser:

    # List of filters
    filters = ['_filter_comments',
               '_filter_new_lines',
               '_filter_empty_entries']

    def __init__(self, **kwargs):
        self.lines = kwargs.get('lines', [])
        self.file_path = kwargs.get('file_path', "data/data_ts.utf8")
        self.lines_count = kwargs.get('entries_count', None)

    def read_file(self):
        """ Reads file data applying filters from self.filters """
        __location__ = os.path.realpath(
            os.path.join(os.getcwd(), os.path.dirname(__file__)))
        with io.open(os.path.join(__location__, self.file_path), "r",
                     encoding='utf-8') as f:
            self.lines = f.readlines()
            self._sanitize()

    def _sanitize(self):
        for fn in self.filters:
            f = methodcaller(fn)
            f(self)

    def _filter_comments(self):
        """ remove lines starting with # or #! """
        self.lines = [line for line in self.lines
                      if not line.startswith(("#", "#!"))]

    def _filter_new_lines(self):
        self.lines = [line.strip('\n') for line in self.lines]

    def _filter_empty_entries(self):
        self.lines = [line for line in self.lines if line.strip()]

Issues

I think that the pythonic way to write those filters in one line as [line.strip('\n') for line in self.lines if line.strip() and if not line.startswith(("#", "#!")] would make it harder to read and unit test them or it would be better that way?
Should the _sanitize method be called after reading the file or would it make more sense to put it in the setter property of self.lines like:

@lines.setter
def lines(self, lines):
      self.lines = lines
      self._sanitize()

Is there any better option to arrange the above code or a design pattern like putting the sanitize method outside the Parser class and receiving a list of lines and returning the sanitized list instead of working always with self.list?

def __init__(self, **kwargs):

I don't understand why you didn't write def __init__(self, lines=[], file_path='data/data_ts.utf8', entries_count=None)

        __location__ = os.path.realpath(
        os.path.join(os.getcwd(), os.path.dirname(__file__)))

Better to just name it location. Also, cwd can change between calls to your class, so this code seems "too tricky". Better to have init() nail down a fully qualified pathname once, and store it.

filters = ['_filter_comments',
           '_filter_new_lines',
           '_filter_empty_entries']

It's not obvious why quoting these, and using methodcaller(), is a Good Thing. Why not simply have a list of three function references, rather than three quoted strings?

The contract on each filter is "assign some or all of lines back into lines". Rather than evaluate each filter for side effects, you might have their API return some or all of lines, and make the _sanitize driving loop responsible for assignment. Or better, you might make them functions accepting a single line, that return a single modified line or None to suppress it. When you eventually write unit tests you'll find such an API is simpler to test.

                  if not line.startswith(("#", "#!"))]

If it starts with '#', then we already saw True and won't consider '#!'.

    self.lines = [line.strip('\n') for line in self.lines]

Idiomatic code would say rstrip.

You contemplated putting _sanitize() in the setter property of self.lines. That is feasible, but a bit odd, and would put a greater documentation burden on you, starting with renaming to class SanitizingParser. Perhaps there is some motivating use case where that makes life easier on the caller, but I'm not yet seeing it.

Consider writing generator(s) that yield a line at a time, rather than manipulating the entire list of lines. Then you can process giant files using a small memory footprint.

EDIT: Suppose the three filter functions have an API of accepting a str (not None) and returning that same input line or a trimmed line or None. Then a generator might look like this:

filters = [
    _filter_comments,
    _filter_new_lines,
    _filter_empty_entries,
]

def read_sanitized_file(file_path):
    with io.open(file_path) as fin:
        for line in fin:
            for filter in filters:
                if line:
                    line = filter(line)
            if line:
                yield line


C++ CLI Console Menu Statemachine I am trying to find the best way(/most efficent way) to setup my console program's menu. The way I have it set up is that I have a Menu class and then I use a menu pointer called currentMenu and I just change where it is pointing to. it runs fairly well so far but I'd like to know what I could do to improve it

So Far it is seperated into 3 files(main.cpp,menu.h and menu.cpp)
Full Code is here: https://repl.it/@SamuelHabtu/StudyBuddy-CLI

Menu.h:

#ifndef MENU_H
#define MENU_H
#include <iostream>
#include <string>
class menu
{
public:
  std::string name;
  menu *prev;
  menu *right;
  menu *down;
  menu *up;
  void displayMenu();
};
#endif

Menu.cpp:

#include "menu.h"
void menu::displayMenu()
{
  std::string userInput;
  std::cout<<name<<" menu:\n";
  if(prev != 0)
  {
    std::cout<<"0:"<<prev->name<<" menu\n";
  }
  if(up != 0)
  {
    std::cout<<"1:"<<up->name<<" menu\n";
  }
  if(down != 0)
  {
    std::cout<<"2:"<<down->name<<" menu\n";
  }
  if(right != 0)
  {
    std::cout<<"3:"<<right->name<<" menu\n";
  }
}

Here is the function that I use to "fill" each menu

void buildMenu(menu *menu,std::string name,class menu *prev,class menu* up,class menu* down,class menu* right)
{
  menu->name = name;
  menu->prev = prev;
  menu->right = right;
  menu->down = down;
  menu->up = up;
}

This is How I am handling the Userinputs:

bool valid(std::string userInput,menu *currentMenu)
{
  if(userInput != "1" && userInput != "2" && userInput != "3" && userInput != "0")
  {
    std::cout<<"invalid input \n";
    return false;
  }
  if(userInput == "0" && currentMenu->prev == 0)
  {
    std::cout<<"invalid input \n";
    return false;
  }
  if(userInput == "1" && currentMenu->up == 0)
  {
    std::cout<<"invalid input \n";
    return false;
  }
  if(userInput == "2" && currentMenu->down == 0)
  {
    std::cout<<"invalid input \n";
    return false;
  }
  if(userInput == "3" && currentMenu->right == 0)
  {
    std::cout<<"invalid input \n";
    return false;
  }
  return true;
}

mainloop for the menu statemachine:

  while(go)
  {
    currentMenu->displayMenu();
    std::cin>>userInput;
    while(!valid(userInput,currentMenu)|| (userInput.size() > 1))
    {
      std::cin.clear();
      std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
      std::cin>>userInput;
    }
    switch(userInput[0])
    {
    case '0':
      currentMenu = currentMenu->prev;
      break;
    case '1':
      currentMenu = currentMenu->up;
      break;
    case '2':
      currentMenu = currentMenu->down;
      break;
    case '3':
      currentMenu = currentMenu->right;
    }
  }

Use a constructor to initialize class variables

Your function buildMenu() looks like it is just initializing all the members of a class menu. That is normally the task of a constructor. So why not write:

class menu
{
    public:
    ...
    menu(std::string name, menu *prev, menu* up, menu* down, menu* right):
            name(name), prev(prev), right(right), down(down), up(up) {}
};

Note that since this is a simple POD class, you can also use brace initialization without having to write a constructor yourself, like so:

menu m = {"foo", nullptr, nullptr, nullptr, nullptr};

However, there is an even better way:

You only need to link menu items to their parent

Manually specifying all four pointers is cumbersome, and it is easy to confuse the four directions. It is also not necessary to store four pointers, there is quite a bit of redundancy in them. It is better to write a constructor that just takes one pointer called parent, and have the class menu have a vector of pointers to child menus, like so:

class menu {
    std::vector<menu *> children;

    public:
    const string name;

    menu(const std::string &name, menu *parent = nullptr): name(name) {
        if (parent) {
            parent->children.push_back(this);
        }
    }

    menu *getChild(int i) {
        return children[i];
    }
};

This makes constructing a menu hierarchy much easier:

menu mainMenu("Main");
menu   toolMenu("Tools", &mainMenu);
menu   notesMenu("Notes", &mainMenu);
menu     subjectsMenu("Subjects", &notesMenu);
menu       mathMenu("Math", &subjectsMenu);
menu       sciencesMenu("Sciences", &subjectsMenu);
menu       humanMenu("Humanities", &subjectsMenu);
menu   flashCardsMenu("Flash Cards", &mainMenu);

Of course, now navigating the menu becomes more tricky, but you should just need two variables:

std::stack<menu *> menuStack;
int menuItem;

The menuStack is to keep track of which (sub)menu you are in, and menuItem tells you which item in the current menu is selected. Navigating them becomes a matter of incrementing/decrementing menuItem to go up and down the currently selected menu, and pushing/popping menus to the stack to go deeper or going out of menus.

(Instead of storing a vector of children, you can still use four pointers. Your constructor still only needs to take a pointer to the parent menu, but now you need to spend more effort in the constructor to derive the four direction pointers based on the parent pointer.)

Further improvements

With the above, you still need to manually specify parent pointers.
You could instead add a member function to the class that creates a submenu:

class menu {
    ...
    private:
    menu(const std::string &name, menu *parent): name(name) {
            parent->children.push_back(this);
    }

    public:
    menu(const std::string &name): name(name) {}
    menu subMenu(const std::string &name) {
        return menu(name, this);
    }
};

This way, you can create your menu hierarchy like this:

menu mainMenu("Main");
menu   toolMenu = mainMenu.subMenu("Tools");
menu   notesMenu = mainMenu.subMenu("Notes");
menu     subjectsMenu = notesMenu.subMenu("Subjects");
menu       mathMenu = subjectsMenu.subMenu("Math");
menu       sciencesMenu = subjectsMenu.subMenu("Sciences");
menu       humanMenu = subjectsMenu.subMenu("Humanities");
menu   flashCardsMenu = mainMenu.subMenu("Flash Cards");

Now your main() function doesn't have to deal with pointers anymore.

Start thinking about ownership

Currently, you are creating a number of menu variables, and they are linked together using pointers. Regardless of how nice you make the interface of linking them together, this can still create a problem if there is the possibility for some of the menu variables to go out of scope, and you get dangling pointers. There are several solutions:

Create a destructor that removes itself from its parent's children vector. To do this, you will also need to store a menu *parent pointer.
Use std::shared_ptr<> to manage links between menu items. This requires that you remove the public constructors, and add a static member function that will do new menu(...) and returns the result as a shared_ptr<menu>.
Store submenus directly inside a regular menu. For example:

class menu {
    vector<menu> children;
    ...
    public:
    menu &subMenu(const std::string &name) {
        return children.emplace_back(name);
    }
};

Create a cursor class for navigating the menu

Think of a cursor as a more advanced iterator. In this case, it's an iterator that can move both up and down inside a menu, and left and right to different menus. It should look like:

class menuCursor {
    menu &rootMenu;
    ...

    public:
    menuCursor(menu &rootMenu): rootMenu(rootMenu) {
        ...
    }

    bool prev();
    bool next();
    bool up();
    bool down();

    menu &operator *() {
        // return a reference to currently selected item
    }

    menu *operator ->() {
        // return a pointer to the currently selected item
    }
};

The functions prev(), next() and so on handle navigating the menu. Each should also check wether the action is correct, and return an error if not (here I recommend using a bool to signal success). Like a regular iterator, you can overload operator * to return the current menu item the cursor points to. The implementation depends on how you implement your menu class: if you have four pointers, then follow those. If you use my suggested method, then keep a stack and an int around to keep track of where you are. Then in your main(), you can do:

menu mainMenu("Main");
menuCursor cursor(mainMenu);
...

while(...get input...) {
    cursor->displayMenu();

    switch(input) {
        case '0':
            cursor.prev();
            break;
        case '1':
            cursor.up();
            break;
        ...
    }
}

Move displayMenu() out of class menu

The function displayMenu() is displaying the possible options for navigating the menu. It makes more sense to have it be a member function of class menuCursor, since the latter should know more about which navigation options are valid.

Use const where appropriate

When passing pointers or references to variables to a function that should not modify those variables, you should make those parameters const. It is quite common to pass strings as const references, like so:

menu(const std::string &name, ...);

This avoids the caller having to make a copy of the string.

Class member variables that are only set at initialization time, and will never change, can also be made const. In your case, if you know you will never change the name of a menu entry after having created it, make it const:

class menu {
    ...
    const std::string name;
    ...
};

Consider a different way of navigating the menus

Instead of offering four options (prev, next, up, down), you could instead show all possible options in the current menu, plus an option to exit the current (sub)menu. Something like:

std::stack<menu *> menuStack;
menuStack.push(&mainMenu);

while (!menuStack.empty()) {
    menu *currentMenu = menuStack.top();

    // Print current menu
    std::cout << currentMenu->name << ":\n";
    int i = 0;
    for (auto &submenu: currentMenu->children) {
         std::cout << i++ << ". " << submenu.name << "\n";
    }
    std::cout << "X. Exit current menu\n";

    // Get input
    std::string input;
    std::cin >> input;

    // Navigate based on the input
    if (input == "X") {
        menuStack.pop();
        continue;
    } else {
        int item = std::stoi(input);
        // Add error checking here to ensure item is valid
        menuStack.push(&currentMenu->children[item]);
    }
}

This should show you something like:

Main:
0. Tools
1. Notes
2. Subjects
X. Exit current menu

And when you enter 2, it should show:

Subjects:
0. Math
1. Sciences
2. Humanities
X. Exit current menu


Sed script to update a version number in an RPM SPECS file I have a specs file:

Name:           program
Version:        2.3.3
Release:        0
...

I want to update the version number every time it's built. I perform the build from a bash script so I'm calling sed to update the SPECS file and a "version" file which holds the version number.

I am using the following sed script to perform the update:

sed -ri 's/^(Version:\s+)([0-9.]+)/\1$version/g' SPECS/*.spec

with the $version being provided by the bash script.

It's designed to look for the correct line and replace the version portion while preserving whatever whitespace has been used. I use *.spec so that it will work with a spec file of any name.

I felt that a sed one-liner made the most sense given the requirement.

Variables don't interpolate inside single quotes.  You need double quotes.

No need to capture the old version that we're throwing away. The /g is redundant since the pattern can never match more than once per line (being anchored to start-of-line).

What happens with Version: 0.9a?  Version:1?  Version:?  Better to take the whole line, possibly excluding comments, and relax the space requirement.

sed -ri "s/^(Version:\s*)[^#]*/\1$version/" SPECS/*.spec

It's possible for the description or another section to contain the trigger text, and we probably don't want to change that.  Use sed's start,stop addressing to limit replacement to the top block only, stopping at the first %whatever header:

sed -ri "1,/^\s*%/s/^(Version:\s*)[^#]*/\1$version/" SPECS/*.spec


LeetCode 1320: Minimum Distance to Type a Word Using Two Fingers I I'm posting my C++ code for LeetCode's 1320. If you have time and would like to review, please do so. Thank you!
Problem

You have a keyboard layout as shown above in the XY plane, where each English uppercase letter is located at some coordinate, for
example, the letter \$A\$ is located at coordinate \$(0,0)\$, the letter
\$B\$ is located at coordinate \$(0,1)\$, the letter \$P\$ is located at
coordinate \$(2,3)\$ and the letter \$Z\$ is located at coordinate
\$(4,1)\$.

Given the string word, return the minimum total distance to type such string using only two fingers. The distance between coordinates
\$(x_1,y_1)\$ and \$(x_2,y_2)\$ is \$|x_1 - x_2| + |y_1 - y_2|\$.

Note that the initial positions of your two fingers are considered free so don't count towards your total distance, also your two fingers
do not have to start at the first letter or the first two letters.

Example 1:
Input: word = "CAKE" Output: 3
Explanation:
Using two fingers, one optimal way to type "CAKE" is:

Finger 1 on letter 'C' -> cost = 0
Finger 1 on letter 'A' -> cost = Distance from letter 'C' to letter 'A' = 2
Finger 2 on letter 'K' -> cost = 0
Finger 2 on letter 'E' -> cost = Distance from letter 'K' to letter 'E' = 1

Total distance = 3
Constraints:

\$2 \le \text{word.length} \le 300\$
Each word[i] is an English uppercase letter.

Accepted C++
#include <vector>
#include <string>
#include <algorithm>

class Solution {
    const unsigned short int ALPHABET_LEN = 26;
    const unsigned short int WIDTH = 6;
    const unsigned char A_LETTER = 'A';
public:

    std::size_t minimumDistance(const std::string word) {
        const std::size_t length = word.size();
        std::vector<int> dp(ALPHABET_LEN);
        int max_decrease = 0;
        int total_distance = 0;

        for (std::size_t index = 0; index < length - 1; index++) {
            const int second = word[index] - A_LETTER;
            const int third = word[index + 1] - A_LETTER;

            for (std::size_t first = 0; first < ALPHABET_LEN; first++) {
                dp[second] = std::max(dp[second], dp[first] + coordinates(second, third) - coordinates(first, third));
            }

            max_decrease = std::max(max_decrease, dp[second]);
            total_distance += coordinates(second, third);
        }

        return total_distance - max_decrease;
    }

private:
    unsigned short coordinates(unsigned short left, unsigned short right) {
        return std::abs(left / WIDTH - right / 6) + std::abs(left % WIDTH - right % WIDTH);
    }
};

Reference
On LeetCode, there is a class usually named Solution with one or more public functions which we are not allowed to rename.

Problem

Discuss

Ensure you match the given API
Using size_t for sizes is a good thing to do, however the LeetCode problem specifies the API, and you should not change it. minimumDistance() should return int.
Use static constexpr for compile-time constants
Use static constexpr instead of const for the constants. This allows the compiler to make more optimizations, and they can then also be used in other places where a compile-time constant is required, for example:
Use a std::array
Since dp has a compile-time constant size, you can make it a std::array:
std::array<int, ALPHABET_LEN> dp;

Avoid using small types unnecessarily
Here you have a problem where you know the maximum distance between two letters is 25. I see you have decided to make coordinates() return an unsigned short. I would not do this. While it might seem more optimal, it is not: on many 32-bit and 64-bit platforms, 16-bit integers are often less efficient than 32-bit integers, since they are smaller than the natural register size, and thus the compiler might need to add instructions to ensure all but the lower 16 bits are zeroed. There might also be a penalty because it has to convert it back to a regular int when adding it the result to  total_distance.
The right time to use shorts or even chars to hold integer values is when you have large arrays of them, since then it might reduce memory usage and memory bandwidth.
Proper naming
The function coordinates(), contrary to what the name implies, does not return coordinates. Instead, it returns a distance. So name it distance().
I would also not name the arguments left and right, but just a and b. Left and right sounds confusing here since you might think it gets the character under the left and right fingers as inputs.
Also, what does dp stand for? It seems like this vector holds the maximum distance traveled given a starting position. Try to find a more descriptive name for it. If there is no good one, then at least add a comment explaining what dp is.
The names second and third are slightly misleading for index > 0. Maybe it would be better to name them from and to.
Use WIDTH consistently
You still use one literal 6 in coordinates().


My own method for sorting numbers in array I've written some method for sorting asc and desc numbers in array. All I want to know is this good logic, and is there a space for improving this code?

-(NSArray *)sortHanded:(NSArray *)inputArray ascending:(BOOL)ascending {
    NSMutableArray *inputMutableArray = [inputArray mutableCopy];
    NSMutableArray *sortedArray = [NSMutableArray new];
    NSNumber *min;
    NSNumber *max;
    if (ascending == YES) {

        //starting while
        while (inputMutableArray.count > 0) {

            //starting switch
            switch (inputMutableArray.count) {

                    //case 0
                case 0:

                    break;

                    //case 1
                case 1:
                    [sortedArray addObject:[inputMutableArray objectAtIndex:0]];
                    [inputMutableArray removeObject:[inputMutableArray objectAtIndex:0]];
                    break;

                    //starting default switch
                default:
                    for (NSNumber *temp in inputMutableArray) {
                        if (min == nil) {
                            min = temp;
                        }

                        if (min > temp) {
                            min = temp;
                        }

                    }

                    [sortedArray addObject:min];
                    [inputMutableArray removeObject:min];
                    min = nil;
                    break;
                    //end default
                    //end start switch
            }

            //ending while
        }

    }
    else {
        //starting while
        while (inputMutableArray.count > 0) {

            //starting switch
            switch (inputMutableArray.count) {

                    //case 0
                case 0:

                    break;

                    //case 1
                case 1:
                    [sortedArray addObject:[inputMutableArray objectAtIndex:0]];
                    [inputMutableArray removeObject:[inputMutableArray objectAtIndex:0]];
                    break;

                    //starting default switch
                default:
                    for (NSNumber *temp in inputMutableArray) {
                        if (max == nil) {
                            max = temp;
                        }

                        if (max < temp) {
                            max = temp;
                        }

                    }

                    [sortedArray addObject:max];
                    [inputMutableArray removeObject:max];
                    max = nil;
                    break;
                    //end default
                    //end start switch
            }

            //ending while
        }

    }
        //returning sorted array
    return sortedArray;
}

There is a big flaw in your program: This

if (min > temp)

compares the addresses of the number objects, not their values.
The correct way is

if ([min compare:temp] == NSOrderedDescending)
// ...
if ([max compare:temp] == NSOrderedAscending)

You probably didn't notice the problem because "small" numbers are
represented as "tagged pointers", but with

NSArray *array = @[@40000000000000, @300000, @10000000000000, @16];

your code gave a wrong result in my test.

Resetting min after the loop seems a bit counter-intuitive to me.
I would do that at the beginning, this also allows to restrict the scope of that variable to where it is needed. Also the second if should be
an else-if:

NSNumber *min = nil;
for (NSNumber *temp in inputMutableArray) {
    if (min == nil) {
        min = temp;
    } else if ([min compare:temp] == NSOrderedDescending) {
        min = temp;
    }
}

Of course you could contract the if statement to

    if (min == nil || [min compare:temp] == NSOrderedDescending) {
        min = temp;
    }

that is a matter of taste.

You could get rid of this loop by using Key-Value Coding to determine the minimum
value in the array:

NSNumber *min = [inputMutableArray valueForKeyPath:@"@min.self"];

But note that this can be significantly slower for large arrays
(compare Finding the smallest and biggest value in NSArray of NSNumbers).

Having almost identical code for the two cases (ascending/descending sort) is not necessary. If you define

NSComparisonResult sortOrder = ascending ? NSOrderedAscending : NSOrderedDescending;

at the beginning, then both cases can be treated with

while (inputMutableArray.count > 0) {
    NSNumber *first = nil;
    for (NSNumber *temp in inputMutableArray) {
        if (first == nil || [temp compare:first] == sortOrder) {
            first = temp;
        }
    }

    [sortedArray addObject:first];
    [inputMutableArray removeObject:first];
}


Filtering large lists and returning values that match a test I have been working on a quiz that I came across within a job application. I am not applying for the job, but I wanted to have a go at the problem.

  An input file contains integers, both positive and negative (there
  might be some duplicates). Each line on the file has one integer.

  Compute the number of y values within the range -10000 ≤ y ≤ 10000
  such that there are distinct numbers a, b in the file where a + b = y.

  You can download the file from
  http://s3.amazonaws.com/spacebalcony/cases.tar.bz2 (57MB). There
  are two test cases there for you to verify your program. Check the
  README file for the answers.

I came up with a solution using itertools but it is very slow.

from itertools import combinations, imap

with open('test1.txt', 'rU') as f:
    integers = sorted(list(set([int(item) for item in f])))

def check_item(x):
    item_total = sum(x)
    if all([item_total >= -10000, item_total <= 10000]):
        return True
    return False

print sum(imap(check_item, combinations(integers, 2)))

Does anyone have any suggestions to speed up the code?

For example, I have tried removing the largest and smallest numbers that could never be brought down within the range:

while (integers[0] + integers[-1]) < -10000:
    integers = integers[1:]

while (integers[0] + integers[-1]) > 10000:
    integers = integers[:-1]

This did not make much of a difference

The problem with the solution posted in the question is that it goes
through all the possible combinations of two elements in the array
when this is not really needed.

The pseudocode for a more efficient strategy would be as follows:

Read and sort the integers
For each integer (a), calculate the lowest and highest values of (b) that satisfy the inequality
Look for the position in the array for those values (take advantage of the values being ordered, use binary search)
Note that all the numbers between the two positions found satisfy the inequality since they are ordered

In python code, it should be something like this:

import bisect

MIN_SUM_VALUE = -10000
MAX_SUM_VALUE = 10000

with open('large.txt', 'rU') as f:
    integers = sorted(set(int(item) for item in f))

count = 0
for index, a in enumerate(integers):
    # Low and high values of b that still satisfy the inequality
    b_low = MIN_SUM_VALUE - a
    b_high = MAX_SUM_VALUE - a

    # Look for the index in the array for the values above
    b_low_index = bisect.bisect_left(integers, b_low, index + 1)
    b_high_index = bisect.bisect_right(integers, b_high, index + 1)

    # Increment count by the number of values found
    count += b_high_index - b_low_index

print count

Note that the way to avoid repetitions is to restrict the binary search to look for b values whose position is after the a values in the array (that's the index + 1 parameter).

Edit: To get the actual sums printed the following snippet can be used:

for b_index in range(b_low_index, b_high_index):
    b = integers[b_index]
    print a, b, a + b


Average calculator with filters I am a beginner in Java and I have a hobby project to create a tool for statistical calculations.
Users can upload csv with results of a survey. The responses stored in a Response object which has two maps: Map<Integer, String> questionMap, Map<Integer, String> answerMap, where the integer(questionID) links the question and answer.

I would like implement an extra feature: to filter the responses to certain values. Therefore the Filter class stores a questionID (int) and a filterValue (String).
Now I would like to calculate the averages of the fields. I have a working implementation but I hope you can help me to make it better:

public List<AverageResult> calculateAverage(List<Response> responses,     List<Filter> filters){
    List<Response> responsesCopy = new ArrayList<>(responses);
    List<AverageResult> averages = new LinkedList<>();
    Set<Integer> questionIDSet = responses.get(0).getAnswerMap().keySet();
    for(Integer questionID :questionIDSet) {
        double sum = 0;
        int count=0;
        for(Iterator<Response> iter = responsesCopy.iterator(); iter.hasNext();) {
            Response actualResponse = iter.next();
            if(filterMatch(actualResponse,filters)) {
                if(isNumber(actualResponse, questionID)) {
                    sum += Double.valueOf(actualResponse.getAnswerMap().get(questionID));
                    count += 1;
                }
            }else {
                iter.remove();
            }
        }
        Double average = null;
        if(count != 0) {
            average = sum/count;
            AverageResult avRes = new AverageResult();
            avRes.setAverage(average);
            avRes.setQuestionID(questionID);
            avRes.setCount(count);
            averages.add(avRes);
        }
    }
    return averages;
}

private boolean isNumber(Response response, int questionID) {
    Pattern p = Pattern.compile("[0-9]*\\.?[0-9]+");
    return (p.matcher(response.getAnswerMap().get(questionID)).matches());
}

private boolean filterMatch(Response response, List<Filter> filters) {
    for(Filter filter: filters) {
        if(!response.getAnswerMap().get(filter.getQuestionID()).equals(filter.getFilterValue())) {
            return false;
        }
    }
    return true;
}

LinkedList?

    List<AverageResult> averages = new LinkedList<>();

If there is a reason why this should be a LinkedList rather than an ArrayList, you should comment it.  An ArrayList will normally use less memory if it fits exactly and allows easy random access.  So is often preferable for containers that are built all at once as this one seems to be.

That's not to say that it is wrong to use a LinkedList here.  It's to say that I don't see any reason in this segment of code to use a LinkedList.  With more context, that might make perfect sense.  So there should be a comment here letting us know that you thought about it and the basis for your decision.

Be careful

    Set<Integer> questionIDSet = responses.get(0).getAnswerMap().keySet();

This assumes that the first response has an answerMap with answers to all the possible questions.  But that is a contract that you will actively have to work to maintain.  It may be true now, but a future change could easily make it not true.  Consider instead

    Set<Integer> questionIDSet = responses.get(0).getQuestionIDs();

Yes, this means defining a new method on responses.

or even better something like

    Set<Integer> questionIDSet = sample.getQuestionIDs();
    List<Response> responses = sample.getResponses();

So you can pass one sample object rather than a container of responses.

Yes, this requires an entire new class.

Keep it simple

        for(Iterator<Response> iter = responsesCopy.iterator(); iter.hasNext();) {
            Response actualResponse = iter.next();
            if(filterMatch(actualResponse,filters)) {
                if(isNumber(actualResponse, questionID)) {
                    sum += Double.valueOf(actualResponse.getAnswerMap().get(questionID));
                    count += 1;
                }
            }else {
                iter.remove();
            }
        }

So you create a copy of the responses so that you can iterate over them and remove those that do not match the filters.  If they do match and are numeric, you calculate the sum and count.  That's fine, but it seems more complicated than necessary.  You never use responseCopy after this section.  So why do you have to remove the entries that don't match?  Why not just

        for (Response actualResponse : responses) {
            if (filterMatch(actualResponse, filters)) {
                if (isNumber(actualResponse, questionID)) {
                    sum += Double.valueOf(actualResponse.getAnswerMap().get(questionID));
                    count++;
                }
            }
        }

That will return the same sum and count values.  But it doesn't require making a copy of responses or modifying the iterator. Or consider

        for (Response actualResponse : responses) {
            Map<Integer, String> answers = actualResponse.getAnswerMap();
            if (filterMatch(answers, filters)) {
                String answer = answers.get(questionID);
                if (isNumber(answer)) {
                    sum += Double.valueOf(answer);
                    count++;
                }
            }
        }

Rather than passing the response to filterMatch and isNumber, this narrows down to the information that they actually want.  This makes isNumber general purpose.

Yes, this requires changes to the other methods.

The ++ is more idiomatic than += 1.

Declare at the smallest scope that covers

        Double average = null;
        if(count != 0) {
            average = sum/count;

Consider

        if (count > 0) {
            Double average = sum / count;

Now if the count is not greater than 0, we don't even bother allocating a place to store the average.  We only do that if we're going to calculate it.  Since we don't use average outside the if, this changes no functionality.

As a functional matter, there wouldn't seem to be a difference between not equal and greater than.  But I find it to be more readable.  This makes it clear that negative values would not be handled.  The original just implicitly assumes that there will never be negative values.  But I had to think about that.  Now I don't.  If we counted something, we calculate the average and return it as a result.

Right after writing the code is when you know the most about it.  It's the best time to make the intentions of the code clear.  Because when you look at it in six months, you won't necessarily remember all the clever things you did today.  So make the code clear or comment why you can't.

And it will really help if someone else is the next person to edit the code.  Since that person never did know all the decisions you made while developing the code.


Randomly generate spelling mistake in string My code will generate a spelling mistake inside of a string 50% of the time.

It will retrieve a letter from a random index in the string like, "t" and then duplicate that letter like, "tt" and store it in the spelling mistake variable.

It will then replace, "t" with "tt" in the string to replicate a spelling mistake. (50% of the time.)

How can I improve my code to make it complete the same task, but with less lines of code and using the least amount of resources possible?

(function() {

    function replaceStr(str, pos, value) {
        var arr = str.split('');
        arr[pos] = value;
        return arr.join('');
    }

    var myString = "Stack Overflow";
    var letterIndex = Math.floor(Math.random() * myString.length); // Example: 1
    var letter = myString.charAt(letterIndex); // Example: "t"
    var mistake = letter + letter; // Example: "tt"

    // 0 -> 9 (coin toss)
    if (Math.floor(Math.random() * 10) >= 5) {
        return replaceStr(myString, letterIndex, mistake);
    } else {
        return myString;
    }

})();

Result from 10 runs:

Stack Overflow
Stackk Overflow
Stack Overflow
Sttack Overflow
Stack Overflow
Stack Oveerflow
Stack Overrflow
Stack Overfllow
Stack Overflow
Stack Oveerflow

Break it down into one function that randomly repeats a character, and another function which randomly executes that function 50% of the time:

function repeatRandomChar(str) {
  var i = Math.floor(Math.random() * str.length);
  return str.slice(0, i+1) + str.slice(i);
}

function randomTypo(str) {
  return Math.random() > 0.5 ? repeatRandomChar(str) : str;
}

Now you can just call randomTypo('Stack Overflow') 10 times to reproduce your sample data.

Note also that the original code has needless multiplication, in that

 Math.random() > 0.5

is equivalent to:

 (Math.random() * 10) >= 5


A beginners implementation of Game of Life I'm totally new to python and coding in general and I tried for my first project to recreate Conway's game of life. After days of trial and error, I came up with this code:

import numpy as np
import copy
import png
import PIL as PIL
def a(a) :
    if a == "gen" :
        size=int(input("size ? "))
        ratio=float(input("ratio ? "))
        name=input("file name: ")
        matrix=np.random.choice(2,(size,size),p=[1-ratio,ratio])
        i = png.from_array(matrix, mode='L;1')
        i.save(str(name)+'.JPEG')

    if a == "start" :
        name=input("file name: ")
        time=input('number of rounds: ')
        matrix=np.asarray(PIL.Image.open(str(name)+'.jpeg')).astype(int)
        matrix2=copy.deepcopy(matrix)
        taille1,taille2=matrix.shape
        counter=0
        while counter<int(time):
           for x in range(0,taille1-1):
             for y in range(0,taille2-1):
                if matrix[x,y]==0 and matrix[x+1,y]+matrix[x,y+1]+matrix[x+1,y+1]+matrix[x-1,y]+matrix[x,y-1]+matrix[x-1,y-1]+matrix[x-1,y+1]+matrix[x+1,y-1]==3:
                   matrix2[x,y]=1
                if matrix[x,y]==1 and matrix[x+1,y]+matrix[x,y+1]+matrix[x+1,y+1]+matrix[x-1,y]+matrix[x,y-1]+matrix[x-1,y-1]+matrix[x-1,y+1]+matrix[x+1,y-1]<2:
                   matrix2[x,y]=0
                if matrix[x,y]==1 and matrix[x+1,y]+matrix[x,y+1]+matrix[x+1,y+1]+matrix[x-1,y]+matrix[x,y-1]+matrix[x-1,y-1]+matrix[x-1,y+1]+matrix[x+1,y-1]>3:
                   matrix2[x,y]=0
           i = png.from_array(matrix2, mode='L;1')
           i.save(str(name)+'_output_'+str(counter+1)+'.JPEG')
           matrix=copy.deepcopy(matrix2)
           counter+=1
task=input("command : ")
a(task)

It works really fine but I'm afraid it is quite unefficient, so I'd like to hear advice on how to make it faster (especially the triple loop)

First of all, please try to reformat the Python code according to Python's style guide (PEP 8).

When programming, it is important to give meaningful names to all variables (with exception to some loop indexes). What is a? Or taille1? My strong believe is, that even "research" kind of programming can benefit from this advice. Try using good Python IDE, eg, PyCharm, which will help to autocomplete names. Programs are read by humans.

It's better to convert user input to the proper type immediately, no need to do it each time in while loop. Similarly, it makes program more readable if input/output is separated from computations.

Also, I do not quite understand why do you want deep copy the matrix each time. Perhaps, more efficient strategy is to have two arrays and alternate them.

As for the computational part, I'd suggested the following simplification:

instead of computing the sum or surrounding cells several times, do it just once. You can add current cell multiplied by eg 10 so the sum will be a single parameter.

You can also have precomputed rules:

RULES = {
    1: 0,
    2: 0,
    3: 1,
    4: 0,
    ...
    10: 0,  # tens indicate current cell
    12: 1,  # continues to live
    13: 1,  #
    14: 0,  # dies due to overpopulation
    15: 0,
    ...
}

Then deciding cell's fate can be done very efficiently:

matrix[x, y] = RULES[weighted_sum]

Of course, this is quite small optimization, but it makes possible to implement any cellular automaton by just changing rules in one place.

Also, pay attention to border conditions. Padding of zeros on each side is needed for the algorithm to work correctly.

Offtopic note: The question made me nostalgic: I remember how still a schoolboy I've found a BASIC program for the Game of Life in Peter Atkins's book "The Second Law" (I read it in translation), and wondered how inefficient implementation it had. Border conditions were checked for each neighbor instead of padding zeros. I do not remember now how the sum was calculated, but this answer actually repeats my idea from that time - single array lookup (in Basic). In Python, of course, there are dicts for O(1) computation.


Two sets came to an intersection Challenge:

  Print set intersections.

Specifications:

  Your program should accept as its first argument a path to a filename.
  Each line in the file is a test case.
  Each test case contain two semicolon delimited sorted lists of numbers in ascending order, whose numbers are separated by a comma.

  Print out the ascending order sorted intersection of the lists, one per line.
  Print empty new line in case the lists have no intersection

Solution:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.LinkedHashSet;
import java.util.Scanner;
import java.util.Set;

public class SetIntersection {
    public static void main(String[] args) throws FileNotFoundException {
        Scanner input = new Scanner(new File(args[0]));
        String[] parts;

        while (input.hasNextLine()) {
            parts = input.nextLine().split(";");

            System.out.println(
                findIntersection(
                    toIntArray(parts[0].split(",")),
                    toIntArray(parts[1].split(","))
                )
            );
        }
    }

    private static String findIntersection(int[] foo, int[] bar) {
        Set<Integer> intersection = new LinkedHashSet<>();

        for (int i = 0, j = 0; i < foo.length; i++) {
            while (foo[i] > bar[j] && j < bar.length) {
                j++;
            }
            if (foo[i] < bar[j]) {
                continue;
            } else {
                intersection.add(bar[j++]);
            }
        }

        if (intersection.isEmpty()) {
            return ""; // none found
        }

        String result = intersection.toString();
        return result.substring(1, result.length() - 1);
    }

    private static int[] toIntArray(String[] array) {
        int[] nums = new int[array.length];

        for (int i = 0; i < array.length; i++) {
            nums[i] = Integer.parseInt(array[i]);
        }

        return nums;
    }
}

Sample Input:

  9,10,11;33,34,35
  3,7,8,22;11,22
  11,12,13,14;14,15,16
  20,21,22;45,46,47
  77,78,79;78,79,80,81,82
  33,35;3,18,26,35

Sample Output:

  22
  14

  78, 79
  35

I wanted to take advantage of the fact the lists were sorted, but I fear my efforts only served to render my code convoluted and inefficient. What do you think?

This is a fine implementation.
It's well-formatted and nicely written.
I find the idea of using a LinkedHashSet quite clever and interesting.
There are a couple of points to improve though.

You have a bug

Do you notice something fishy here?

while (foo[i] > bar[j] && j < bar.length) {
    j++;
}
if (foo[i] < bar[j]) {

The condition in the while checks foo[i] > bar[j] before j < bar.length.
So when j reaches bar.length,
this will throw an ArrayIndexOutOfBoundsException.

Flipping the && may seem like an obvious remedy.
But it won't be enough,
because of the next if statement after the while loop,
you'll again have the problem if j has already reached bar.length.

An example input to demonstrate this issue: 33;3,4

Unit testing

It's always handy to have some unit tests around when solving challenges like this.
For example:

@Test
public void test_9_10_11_x_33_34_35() {
    assertEquals("", findIntersection("9,10,11;33,34,35"));
}

@Test
public void test_3_7_8_22_x_11_22() {
    assertEquals("22", findIntersection("3,7,8,22;11,22"));
}

@Test
public void test_11_12_13_14_x_14_15_16() {
    assertEquals("14", findIntersection("11,12,13,14;14,15,16"));
}

@Test
public void test_20_21_22_x_45_46_47() {
    assertEquals("", findIntersection("20,21,22;45,46,47"));
}

@Test
public void test_77_78_79_x_78_79_80_81_82() {
    assertEquals("78,79", findIntersection("77,78,79;78,79,80,81,82"));
}

@Test
public void test_33_35_x_3_18_26_35() {
    assertEquals("35", findIntersection("33,35;3,18,26,35"));
}

@Test
public void test_33_x_3_4() {
    assertEquals("", findIntersection("33;3,4"));
}

An alternative implementation

Consider this alternative implementation that passes all the tests:

private String findIntersection(String input) {
    String[] parts = input.split(";");
    String[] arr1 = parts[0].split(",");
    String[] arr2 = parts[1].split(",");
    return findIntersection(arr1, arr2);
}

private String findIntersection(String[] arr1, String[] arr2) {
    StringBuilder builder = new StringBuilder();
    for (int pos1 = 0, pos2 = 0; pos1 < arr1.length && pos2 < arr2.length; ) {
        int value1 = Integer.parseInt(arr1[pos1]);
        int value2 = Integer.parseInt(arr2[pos2]);

        if (value1 == value2) {
            builder.append(value1).append(",");
            ++pos1;
            ++pos2;
        } else if (value1 < value2) {
            ++pos1;
        } else {
            ++pos2;
        }
    }
    if (builder.length() == 0) {
        return "";
    }
    return builder.substring(0, builder.length() - 1);
}

This implementation has an advantage and disadvantage compared to yours:

Advantages:

It doesn't always convert all input values to integers. For example when you have a long list and a short list, and it turns out that they cannot have an intersection, the rest of the long list will not be converted to integers
It should be faster to generate the output from a StringBuilder than from a LinkedHashSet

Disadvantage: in common cases it converts input values to numbers twice.
This is because in every step of the while loop, when only one of the positions advance, then on the next step the other number will be converted to integer again.

There is possibly one more advantage:
it's not clear if in the output the values should be separated by single commas,
or by comma + space as in yours (and in the output of Set.toString).
Since the values are separated by , in the input,
I would assume the same rule for the output as well,
but I could be wrong.
But if I'm right,
then to get the right output you would have to strip the spaces from the result of the Set.toString call.


Print the string equivalents of a phone number Task

Old mobile phones had the ability to type characters by pressing a number. The letter a could be typed by pressing 2 once. The letter b could be typed by pressing 2 twice.

Given a sequence of numbers, give all possible letter combinations.

For example: The number 23 could give an output ad, ae, af, bd, be, bf, cd, ce, cf

My recursive solution to this problem is given below.

def num_to_char(value):
    if value == 2: return ["a","b","c"]
    if value == 3: return ["d","e","f"]
    if value == 4: return ["g","h","i"]
    if value == 5: return ["j","k","l"]
    if value == 6: return ["m","n","o"]
    if value == 7: return ["p","q","r","s"]
    if value == 8: return ["t","u","v"]
    if value == 9: return ["w","x","y","z"]

def convert_num(number, current_string = ""):
    if number == []:
        print(current_string)
        return
    get_list = num_to_char(int(number[0]))
    for character in get_list:
        current_string += character
        convert_num(number[1:], current_string)
        current_string = current_string[:-1]

num_to_covert = list("234")
convert_num(num_to_covert)

You're working way too hard:

itertools.product() produces cartesian products.
You don't need to convert strings to lists; you can iterate over strings directly.
Lookups are better done using a dictionary than a chain of if statements.

from itertools import product

KEYPAD = {
                 '2': 'abc',  '3': 'def',
    '4': 'ghi',  '5': 'jkl',  '6': 'mno',
    '7': 'pqrs', '8': 'tuv',  '9': 'wxyz',
}

def convert_num(number):
    letters = [KEYPAD[c] for c in number]
    return [''.join(combo) for combo in product(*letters)]

print(convert_num('234'))


Unit testing for concatenating two int arrays Using Java 1.8, I created the following implementation to concat two int arrays:

public static int[] concat(int[] array1, int[] array2) {
    return IntStream.concat(Arrays.stream(array1), Arrays.stream(array2)).toArray();
}

The unit test I created to test this looks like this:

int[] numbers1 = new int[] {1, 4, 3, 10, 2, 11, 15, 8};
int[] numbers2 = new int[] {5, 6, 7, 9, 12, 13, 14};

@Test
public void concat() {
    int[] concatNumbers = ArrayUtils.concat(numbers1, numbers2);

    for (int i = 0; i < numbers1.length; i++) {
        assert(concatNumbers[i] == numbers1[i]);
    }

    for (int i = numbers2.length + 1; i < numbers2.length; i++) {
        assert(concatNumbers[i] - 1 == numbers1[i]);
    }
}

Questions:

Although my unit test works correctly, is there a better way to unit test this method (e.g. in terms of style, cleaner code, etc.)?
How could I change the actual implementation if I were to use varargs?

public static int[] concat(int[] ... args)

Testing

concat isn't a great name for a test, it doesn't tell me anything about what it is that the test does, it simply copies the name of the method being called.  Something like concatShouldReturnConcatenationForTwoPopulatedArrays might be a better description
Personally, for a test this simple, I'd tend to define the expected results for the test within it rather than calculating it on the fly.  This makes it less likely that you'll get a failure because your test is wrong.  It also allows you to use the junit array comparison.

Assert.assertArrayEquals( expectedResult, result );

You've only got one test, I'd tend to add at least 3 more for:

concat([], numbers2)
concat(numbers1, [])
concat([], [])


Check for empty object I have a modal for adding new contacts, in order to disable the save button if no data has entered I check if the contact object is empty, is there a shorter way to check for an empty complex object?
  let isContactEmpty = true

  Object.keys(contact).forEach(field => {
    if (typeof contact[field] === 'string' && contact[field]) {
      isContactEmpty = false
      return
    }
    if (typeof contact[field] === 'object') {
      contact[field].forEach(fieldVal => {
        if (fieldVal.value.length) {
          isContactEmpty = false
        }
      })
    }
  })

Is there a particular reason you're using a .forEach() instead of for-of?. Because .forEach() uses a function for iteration, you don't have a way to stop early once you've you know the contact is not empty. If you used for-of instead, you can just return false once you know the result will be false.
It looks like you're doing a typeof value === 'object' to check if something is an array. Instead, you can do Array.isArray(value).
You're looping over the keys of the object, but you only use the key to get the field's value. Why not just loop over the values instead with Object.values()?

With these suggestions applied, your code would look something like this:
function isContactEmpty(contact) {
  for (const field of Object.values(contact)) {
    if (typeof field === 'string' && field) {
      return false
    } else if (Array.isArray(field)) {
      for (const { value } of field) {
        if (value.length) return false
      }
    }
  }
  return true
}

But, we can take it a step further. Javascript has a higher-level function to do exactly what we're doing with these loops, but without the boilerplate - array.every(). It takes a callback that returns a boolean and gets called with each element of an array. The return value of every() is true if all the callbacks returned true, or false if one of them returned false. (It'll also stop iterating as soon as one callback returns false).
const isContactEmpty = contact => (
  Object.values(contact).every(field => {
    if (typeof field === 'string') return !field
    if (Array.isArray(field)) return areArrayEntriesEmpty(field)
    return true
  })
)

const areArrayEntriesEmpty = field => (
  field.every(({ value }) => value.length === 0)
)


Model structure for Player Team and Match in football application I'm creating an application for foosball matches.

I have models like below:

class Player(models.Model):
    match_amount = models.IntegerField(default=0)
    wins = models.IntegerField(default=0)
    avatar = models.ImageField(blank=True)
    user = models.OneToOneField(
        on_delete=models.CASCADE,
        to=User,
        primary_key=True,
        related_name='player',
        verbose_name=_('user'),
    )

    def __str__(self):
        return self.user.get_full_name()

class Team(models.Model):
    players = models.ManyToManyField(Player)
    wins = models.IntegerField(default=0)

    def __str__(self):
        return "Team %s" % self.pk

class Match(models.Model):
    date = models.DateField(default=now)

    def __str__(self):
        return "Match on %s" % self.date

class TeamMatch(models.Model):
    team = models.ForeignKey(Team)
    match = models.ForeignKey(Match)

    points = models.IntegerField()

    def __str__(self):
        return "Match %s" % self.team

Is these models are well created?

Thanks much for each tip and corrections.

It is difficult to say how good your models are, since it depends on your application requirements and demands, current and future/planned use cases.

But, few points from the top of my head:

there are some important fields missing - for example, team names
I don't particularly like the TeamMatch model. Instead, I'd expect the Match to have links to the home and away teams:

class Match(models.Model):
    home_team = models.ForeignKey(Team, related_name='home_matches')
    away_team = models.ForeignKey(Team, related_name='away_matches')

    date = models.DateField(default=now)

    def __str__(self):
        return "Match between '%s' and '%s' on %s" % (self.home_team.name, self.away_team.name, self.date)

you will also need some way of keeping the score of a match. A separate MatchResult model?
how about a use case when a player moves from one team to another? If this is something you want to keep track of, you would probably need something like a PlayerContract model to relate players and teams for a certain period of time

Also, please see these similar model design discussions:

How should I design my django models for team, player and match objects?
Models for a team


n dimensional array creator in Python Is there any way I can make this function more inline?

def nDimensionalArray(n, s):
    x = 0
    for i in range(n):
        x = [x for j in range(s[i])]
    return x

You didn't write a docstring (which you should have done) or explain your function (which you should also have done), but it looks like n is supposed to be the same as the length of s.  If so, why should n have to be specified?

Your function has some unintuitive behaviour:

>>> matrix = nDimensionalArray(3, [1,2,3])
>>> matrix
[[[0], [0]], [[0], [0]], [[0], [0]]]
>>> matrix[0][0][0] = 1
>>> matrix
[[[1], [1]], [[1], [1]], [[1], [1]]]

Changing one element changes everything!  I assume that that is unintentional.  If it is intentional, then it needs to be clearly documented.


Custom exception handling function or logging library? I want to document every exception handled in my code and some states of the code when it works properly.
What I've done is two functions, one that creates an 'error report' (function: error) and one that creates a 'state report'(function: state), and they work by being called in functions where there's stuff I'd like to have documented, they have a code parameter which I use to select the message of the report (apart from the exception, in the case of it being an error report) and then they call another function which takes this information and loads it to a JSON file(function: writeReport). If I'm being clear so far, you can skip all the code examples, my question is at the end.

Here I show you the functions I mentioned before (messages are in spanish because its my main language):

# Exception mesagges (this is the one that records exceptions)
def error(code, exception, extra=None):
    if code == 0:
        stateReport.message = ('Error indefinido')
    elif code == 1:
        stateReport.message = ('Error en la conexion o creacion de la base de datos')
    elif code == 2:
        stateReport.message = ('No se pudieron crear las tablas o ya existen')
    elif code == 3:
        stateReport.message = (f'No se pudo recuperar la tabla {extra}')
    elif code == 4:
        stateReport.message = (f'Error indefinido que no afecta al funcionamiento')
    elif code == 5:
        stateReport.message = (f'Error en manipulacion de GUI que no afecta al funcionamiento')
    elif code == 100:
        stateReport.message = (f'Error desconocido al presionar una tecla')
    stateReport.function = (inspect.stack()[1].function)
    stateReport.date_time = date_time
    stateReport.error = str(exception)
    writeReport()
    return

# State messages (this I use when some special case goes right)
def state(code, extra=None):
    if code == 0:
        stateReport.message = (f'El programa funciono correctamente')
    elif code == 1:
        stateReport.message = (f'Se accedio a la base de datos')
    elif code == 2:
        stateReport.message = (f'Se creo exitosamente la nueva tabla')
    elif code == 3:
        stateReport.message = (f'Valores ingresados incorrectos. Valor incorrecto: {extra}')
    stateReport.function = (inspect.stack()[1].function)
    stateReport.date_time = date_time
    stateReport.error = '---------->Sin errores<----------' #This means "No errors"
    writeReport()

def writeReport():
    report = {'date': stateReport.date_time,
               'function': stateReport.function,
               'message': stateReport.message,
               'error': stateReport.error}
    if not os.path.exists(reportsFile):
        with open(reportsFile, 'w', newline='') as file:
            json.dump(report, file, indent=4)
    else:
        with open(reportsFile, 'a', newline='') as file:
            json.dump(report, file, indent=4)
    return

Now, heres a function where I use this feature on, just so you see how I actually implemented it:

table = 'clientes'
    ## This is the creation of a <new> table in an sqlite database
try:
    c.execute(f'''CREATE TABLE {table} (
    id integer,
    nombre text,
    apellido text,
    valoracion real)''')
    state(2)
    ##If the table is successfully created a state report is saved with this info
except Exception as e:
    error(2, e, table)
    ##On the contrary, if an exception occurs, en error report is recorded

This works great, but today I ran into the logging library (im kinda new in python) and it made me think. Is it better to use loggings in all of this cases? It would create a file with everything, it's allready a python library and I would avoid having three separate functions just for recording stuf, but I dont know if it's logging proper way of using it.

The Python community favors having just one way to do something. As the Zen of Python puts it:

  There should be one-- and preferably only one --obvious way to do it.

Other people and packages will be using the logging module, in general. That makes it much easier for your code to interoperate with other packages, use consistent configuration, avoid non-obvious mistakes (like writing error messages to stdout), etc. As such, it's probably to your (and your code's) long-term advantage to use the logging module.

I'll say, one handy tip for logging in this particular case is the exc_info argument to .error, which makes it easy to log exceptions and stack traces while also printing your own messages.

@Peilonrayz responded first with a couple points that I planned to mention, so I'll just re-affirm those points:

You should use exceptions with messages properly; don't just log something and move on as if nothing happened. Whoever called your code (I don't care if you're the only one calling your functions) deserves to know that what they asked for didn't happen, and you're begging for bugs if you don't throw an exception to tell them.
Don't use a pile of if/else statements just to convert an integer into a string for an error message. Use named strings or a dictionary to store this data somewhere else in your code. (Once you do that, it'll make a whole lot more sense to just pass the error message to any handler function rather than have a custom function to look up error codes. This feels very much like an old habit from C/C++; welcome to Python, please don't use error codes that people need to look up, use one of the many excellent error types initialized with useful messages to communicate errors.)


Comparing two integers without logical, relational or bitwise operators I answered a question on SO today, however it did't receive any up-votes, so I want to know what's wrong with my code.

The requirement is to compare the input numbers(granted to be positive) without logical, relational or bitwise operators, and insert the corresponding comparison sign between them. For example:

input:  4 6
output: 4 < 6
input:  10 2
output: 10 > 2
input:  2 2
output: 2 = 2

Here is my posted code:

#include <stdio.h>

int main(void)
{
    unsigned a, b;
    scanf("%u %u", &a, &b);
    printf("%u ", a);
    char relationship[] = {'<', '=', '>'};
    putchar(relationship[!!(a/b) - !!(b/a) + 1]);
    printf(" %u", b);
    return 0;
}

That OP wants to know how to insert a comparison sign(<, >, =) between the two numbers. But my emphasis is on how to determine the relationship of the two numbers.

See: https://stackoverflow.com/q/35532123/5399734 for the original question.

That seems a fine solution. Some minor suggestions:

Instead of spelling out relationship as {'<', '=', '>'}, you could use simply "<=>"
Instead of printing a, the relation and b in separate statements, it will be more readable to print in a single printf
The expression !!(a/b) - !!(b/a) evaluates to -1, 0, 1, commonly returned by a cmp function. If you give the expression a name by putting it in a variable, that will ring a bell with many readers and make it easier to understand.

Something like this:

unsigned a, b;
scanf("%u %u", &a, &b);

int cmp = !!(a/b) - !!(b/a);
char relation = "<=>"[cmp + 1];
printf("%u %c %u\n", a, relation, b);