Using a recurring System.Threading.Timer in an Mvc Application I'd like your opinion on something. I have the following class:

public class ApplicationClock
{
    void tick(object _)
    {
        lock (tickLock)
        {
            try
            {
                MessageBroker.Publisher.Publish(new Tick());
            }
            catch (Exception e)
            {
                this.Logger().Error("Application clock tick error", e);
                //who knows why this happened, let's try and restart
                if (ticker != null) ticker.Dispose();
                Start();
            }
        }
    }
    readonly Object tickLock = new Object();
    Timer ticker = null;
    public ApplicationClock Start()
    {
        ticker = new Timer(tick, null, 0, 60*1000);
        return this;
    }
}

launched from the MvcApplication bootstrapper

...
ApplicationClock clock;
protected void Application_Start()
{
   //...
   clock = new ApplicationClock().Start();
}

I realize that a service and some sort of inter-process communication (whether windows inter-process communication, HTTP, or something else) is more standard and reliable but I have a frequently changing team and don't want to add another step that is necessary to run the app.

This seems to work but I've only launched it in a dev scenario. Am I missing anything that would cause problems in production?

That's a System.Threading.Timer by the way.

Looks mostly ok, just some issues around coupling:

Consider one of the following

Pass MessageBroker.Publisher via the ApplicationClock constructor as an external dependency (preferably an interface). This will make the current implicit dependency explicit and visible which should yield in better maintenance in the future and easier unit testing.
Pass the tick handler in as an Action parameter - no dependency on a specific object at all.
Expose a ClockTick event and raise the event handler on tick. This way anyone interested in the clock can subscribe and the clock doesn't need to know about them at all.

Also your tick interval is hard coded, it should be passed in as parameter.


Time limit exceeded on finding out the GCD and LCM of a Python list I'm doing this HackerRank problem:

  Consider two sets of positive integers, \$A=\{a_0, a_1, \ldots, a_{n-1}\}\$ and \$B=\{b_0, b_1, \ldots, b_{m-1}\}\$. We say that a positive integer, \$x\$, is between sets \$A\$ and \$B\$ if the following conditions are satisfied:

  All elements in \$A\$ are factors of \$x\$.
  \$x\$ is a factor of all elements in \$B\$.

  Given \$A\$ and \$B\$, find and print the number of integers (i.e., possible \$x\$'s) that are between the two sets.

Assuming lists a and b as in the problem, the main idea is finding the LCM of list a and the GCD of list b, and then finding out how many multiples of the LCM divide the GCD perfectly without remainders. I ended up with this -

from fractions import gcd

def lcm(a, b):
    for i in xrange(max(a,b), a*b+1):
        if i%a==0 and i%b==0:
            l = i
            break
    return l

def count(l, g):
    count = 1
    if l==g:
        return count
    elif g%l !=0 and l%g != 0:
        return 0
    elif l<g:
        for i in xrange(l, g, l):
            if g%i==0:
                count += 1
        return count
    else:
        for i in xrange(g, l, g):
            if l%i==0:
                count +=1
        return count

if __name__ == '__main__':
    n,m = raw_input().strip().split(' ')
    n,m = [int(n),int(m)]
    a = map(int,raw_input().strip().split(' '))
    b = map(int,raw_input().strip().split(' '))
    l = reduce(lcm, a)
    g = reduce(gcd, b)
    print count(l, g)

But I pass only 7 of the 8 test cases, the last one getting terminated due to time out. I don't understand which part of my code would result in a long loop that might end up in a timeout.

P.S. I would also be very glad if any of you could point out any other inefficiencies or styling conventions in my code.

It is a really bad idea to use  a variable named l. I is hard to distinguish it from 1.
All these functions using xrange are inefficient

the lcd function can be efficiently calculated by

from fractions import gcd
def lcd(a,b):
    return(a*b/gcd(a,b))

the count function is

count(l,g)=divisor_count(g/l)

where divisor_count is the number-of-divisors function

If the number n has the prime factor decomposition

$$n=p_1^{e_1}\cdot p_2^{e_2}\cdots p_k^{e_k}$$
then we have

$$ \text{divisor_count}(n)=(e_1+1)\cdot(e_2+1)\cdots(e_k+1)$$

This can be calculated in the following way:

def divisor_count(n):
    cnt=1
    i=2
    while i*i<=n:
        e=0
        while n%i==0:
            n//=i
            e+=1
        cnt*=e+1
        i+=1
    if n>1:
        cnt*=2
    return(cnt)

This divisor_count function runs in
$$O(\sqrt{n})$$
time, the xrange implementation uses
$$O(n)$$
time.


Entering a span value into the input When you click on a span it enters the value into the input. That works fine, but as you can see the JS code is very big and there must be a better way to do this, but like I said I'm still learning. Maybe someone can give me a pointer in the right direction.

I suppose I have to use a variable somewhere but can't really figure it out on my own.

jsFiddle

jQuery(document).ready(function() {

    $("#spanval1").click(function(){
        $('#hourvalue').val($('#spanval1').text());
    });

    $("#spanval2").click(function(){
        $('#hourvalue').val($('#spanval2').text());
    });

    $("#spanval3").click(function(){
        $('#hourvalue').val($('#spanval3').text());
    });

    jQuery('.hour_dropdown').hide()

    jQuery("#more").click(function() {
        $('.hour_dropdown').fadeToggle(200);
    });
});

Building off of Danny's answer, if you have access to the HTML, you can add a class to each of your span tags and bind an event to that class:

1
2
3

The event call can become more generic by using the event's local context 'this'. From the .bind() documentation:

  Within the handler, the keyword this refers to the DOM element to which the handler is bound. To make use of the element in jQuery, it can be passed to the normal $() function.

'this' inside of the click event will be the DOM element that was clicked on that caused the event to fire. Then you would need only the following to setup the click event for all of the elements with the class 'spanval':

$(".spanval").click(function(){
    $('#hourvalue').val($(this).text());
});

And with the rest of the script:

$(document).ready(function() {

   $(".spanval").click(function(){
       $('#hourvalue').val($(this).text());
   });

   $('.hour_dropdown').hide();

   $("#more").click(function() {
       $('.hour_dropdown').fadeToggle(200);
   });

});


Calculating the push or pull force of a molecule at a vertex The basic idea of the code is to calculate the push or pull force at a vertex, given the number of "push causing molecules" and "pull causing molecules" at a polygon vertex.

The code thus mainly performs arithmetic (including the function it calls, calculate_edge_force), and has a couple of if-statements. The function is called many times since it is required in order to compute the derivatives, which is later used by scipy.odeint for integration.

Here is my code, in plain Python (note that import numpy as np has been put in somewhere higher up in the file, and that the functions are inside a class, hence there are references to this as self.<something>):

def calculate_point_forces(self, PushMolecules, PushMolecules_iPlus1, PushMolecules_iMinus1, PullMolecules, PullMolecules_iPlus1, PullMolecules_iMinus1, point_index, num_nodes, polygon, polygon_compression, point_interpolygon_contact, push_vector, pull_vector, a1, a2):

    max_push_force = self.max_push_force
    max_pull_force = self.max_pull_force
    tangent_factor = self.tangent_factor
    rac_force_exponent = self.rac_force_exponent
    rho_force_exponent = self.rho_force_exponent

    avg_PushMolecules_plus1 = (PushMolecules + PushMolecules_iPlus1)*0.5
    avg_PullMolecules_plus1 = (PullMolecules + PullMolecules_iPlus1)*0.5

    avg_PushMolecules_minus1 = (PushMolecules + PushMolecules_iMinus1)*0.5
    avg_PullMolecules_minus1 = (PullMolecules + PullMolecules_iMinus1)*0.5

    if self.enable_compression_stiffening == True:
        if polygon_compression < 1:
            F_internal = ((1-polygon_compression)+self.cytosolic_baseline)*self.cytosolic_pressure
        else:
            F_internal = 0
    else:
        F_internal = 0

    if PushMolecules > PullMolecules:
        Fpush = int(not point_interpolygon_contact)*max_push_force*(1 - (1/(1 + (PushMolecules/a2)**rac_force_exponent)))*push_vector
        Fpull = 0
        Fe_pull = 0
    elif PushMolecules < PullMolecules:
        pull_magnitude = max_pull_force*(1 - (1/(1 + (PullMolecules/a1)**rho_force_exponent)))
        Fpull = (1-tangent_factor)*pull_magnitude*pull_vector
        Fe_pull = 0.5*tangent_factor*pull_magnitude
        Fpush = 0
    else:
        Fpush = 0
        Fpull = 0
        Fe_pull = 0

    Fe_pull_minus1 = 0
    Fe_pull_plus1 = 0
    if PushMolecules_iMinus1 < PullMolecules_iMinus1:
        pull_magnitude_iMinus1 = max_pull_force*(1 - (1/(1 + (PullMolecules_iMinus1/a1)**rho_force_exponent)))
        Fe_pull_minus1 = 0.5*tangent_factor*pull_magnitude_iMinus1
    if PushMolecules_iPlus1 < PullMolecules_iPlus1:
        pull_magnitude_iPlus1 = max_pull_force*(1 - (1/(1 + (PullMolecules_iPlus1/a1)**rho_force_exponent)))
        Fe_pull_plus1 = 0.5*tangent_factor*pull_magnitude_iPlus1

    Fe_pull  = np.max([Fe_pull_minus1, Fe_pull_plus1, Fe_pull])

    Fe_plus, Fe_plus_ = self.calculate_edge_force(
        avg_PushMolecules_plus1, avg_PullMolecules_plus1, point_index,
        (point_index+1) % num_nodes, polygon)
    Fe_minus, Fe_minus_ = self.calculate_edge_force(
        avg_PushMolecules_minus1, avg_PullMolecules_minus1, point_index,
        (point_index-1) % num_nodes, polygon)

    F = Fpush + Fpull + Fe_plus + Fe_minus + Fe_pull*Fe_plus_ + Fe_pull*Fe_minus_ + push_vector*F_internal

    return F

When I profile this, I get that the function itself takes about 3 seconds. Out of this, 1 second is spent in calls to the calculate_edge_force and 1 second is spent by np.max.

If I "naively" put in float and int wherever I can (I don't know how to type the NumPy arrays, e.g. push_vector or pull_vector, so I only do it for things like PushMolecules, max_push_force or point_index, which I know will be float or int), I actually get a very mild slowdown in speed (about 0.2 seconds)!

Clearly then, that's not the way to go about "Cythonizing" my code. What should I be doing instead?

First of all you probably should type your numpy arrays but be warned, whether you use the np.ndarray notation, or the memory view notation, only limited operations occur at the speed of c, that is indexing, 'shape', and a very few others. With a memory view you can only use these fast operations, with a ndarray type you can access all the python operations too but they still occur at the speed of python, not c.

In this case, your code doesn't actually use any array functions that would benefit from the arrays being properly typed so you'd gain nothing in the mere act of typing the arrays. The overhead of calling a python function is significant so you might benefit from not using the numpy array functions, if the numpy arrays are short and if the functionality is straightforward to reproduce as cython code using array indexing (fast) and if the function is called many times. The profiler should tell you this stuff.

I can see some low-hanging fruit in the above code, take this line, mentioned to be a major offender:

Fe_pull  = np.max([Fe_pull_minus1, Fe_pull_plus1, Fe_pull])

What happens here, is np.max is a call to a python function of a python module, the list will be constructed as a python list and the floats will be converted to python floats because a python list can't hold c floats. Then that code will all run at the speed of python. np.max will then unpack all that stuff and the result of np.max will be a python float which is converted to a c float. This will be slow!. In fact it may even be slower than python, because it involves creating extra temporary python objects.

What you would do is rewrite that to eliminate the python function call (and thus all the intermediate python objects). As it happens, if you use the python built-in 'max' on typed variables, cython is smart enough to eliminate the python call entirely:

Fe_pull = max(Fe_pull_minus1, Fe_pull_plus1, Fe_pull)

That will automatically become optimal inline c code for calculating the max of the 3 variables (Cython actually unrolls it as if/else statements)

The next low-hanging fruit, and another stated offender is this:

Fe_plus, Fe_plus_ = self.calculate_edge_force( ...

In some cases Cython optimizes away tuple-unpacking, but it doesn't in the case of function calls. So what is going to happen is cython will create an intermediate tuple (a python object), pack stuff into the tuple (Again, converting to python objects, as tuples can't hold c types), then unpack the tuple and convert the python objects to their c types. Again this should be turned into something that translates readily to c. Unfortunately in this case there is no faster better cleaner syntax, you would probably be best served by making a 'pair' struct and manually packing and unpacking it, something like:

#At the top level
cdef struct float_pair:
    float a
    float b

# In the class method definition
cdef floar_pair calculate_edge_force(...
    ...
    cdef float_pair result
    result.a = ...
    result.b = ...
    return result

# In your function
    cdef float_pair result = self.calculate_edge_force( ...
    Fe_plus = result.a
    Fe_plus_ = result.b

Using a struct is by no means the only option here, but it's going to be faster than using a small array. You could also use the C++ 'pair' template which would be just as fast as a struct but it involves bringing in C++. For that matter you could also pass in the floats by pointers. None of these are clearly superior in either speed or elegance, but all will compile down to clean c code.

I imagine if you root out those big ghoulies of python calls you'll get a significant speed improvement. After that it's a matter of using cython -a on the .pyx file. The generated html file shows 'unoptimized' lines in yellow/orange, you can click on a line to see the generated c code. From there you can figure out how to eliminate the python calls - if it is worth it.


Should I call a method (i.e. size()) multiple times or store it (if I know the value will not change) I have a function that searches the shortest vector in a vector of vectors of pointers.

Which is a better way?

std::vector<std::vector<void*>> const& ways

{
   size_t i = SIZE_MAX;
   for(auto& it : ways)
   {
      if(i > it.size())
         i = it.size();
   }
}

OR

std::vector<std::vector<void*>> const& ways

{
   size_t i = SIZE_MAX;
   for(auto& it : ways)
   {
      const auto temp = it.size();
      if(i > temp)
         i = temp;
   }
}

As far as I can see size() does not return reference so second one is a better way, is it true?

Also, is there any other approach you can suggest for this method?

Calling size() twice, like you are, almost definitely get's optimized out in any modern compiler. How you are doing it is perfectly fine. You could also do it with std::max_element...

auto max = std::max_element(ways.begin(), ways.end(),
    [](const std::vector<void*>& left, const std::vector<void*>& right)
    {
        return left.size() < right.size();
    })->size();

Or maybe you write the lambda as a functor instead and a little more generic, like:

struct ContainerSizeComparer
{
    template<typename Container>
    bool operator()(const Container& left, const Container& right)
    {
        return left.size() < right.size();
    }
};

auto max = std::max_element(ways.begin(), ways.end(), ContainerSizeComparer())->size();

This is one of those interesting times when hand rolling the loop is actually more readable and as small as using standard algorithms though. Unless you have a reason to write ContainerSizeComparer and use it in multiple places I would personally stick with what you were doing.


Extracting information from JSON Here is a simplified version of my JSON.

{
    "Relations": [
        {
            "Type": "A",
            "Categories": [
                { "Name": "Identity" }
            ]
        },
        {
            "Type": "B",
            "Categories": [
                { "Name": "Identity" },
                {
                    "Name": "Contact Information",
                    "Type": "Phone"
                },
                { "Name": "Addresses" },
                { "Name": "Bank Accounts" }
            ]
        },
        {
            "Type": "C",
            "Categories": [
                { "Name": "Identity" },
                { "Name": "Contact Information" },
                { "Name": "Service Fields" }
            ]
        }
    ]
}

My code generates an IEnumerable (List, but I am open to suggestions if there is a more compatible option) that holds all distinct categories, meaning, when joining its values (using string.Join("\n", collection)) it contains the following JSON:

{ "Name": "Identity" }
{
    "Name": "Contact Information"
    "Type": "Phone"
}
{ "Name": "Addresses" }
{ "Name": "Bank Accounts" }
{ "Name": "Contact Information" }
{ "Name": "ServiceFields" }

This is the code:

List<dynamic> Method()
{
    string jString = File.ReadAllText(@"file.json");
    dynamic jObject = JsonConvert.DeserializeObject<dynamic>(jString);

    // The variable in questions
    var categories = new List<dynamic>();
    foreach (dynamic relation in jObject.Relations)
    {
        foreach (dynamic category in relation.Categories)
        {
            if (!CollectionContainsItem(categories, category))
            {
                categories.Add(category);
            }
        }
    }

    return categories;

    bool CollectionContainsItem(IEnumerable<dynamic> collection, JToken searchedItem)
    {
        foreach (var item in collection)
        {
            if (JToken.DeepEquals(item, searchedItem))
            {
                return true;
            }
        }
        return false;
    }
}

How to improve my code? It seems very cumbersome to me, especially the local method.

Note: I need a dynamic approach. I do not want to create C# classes for the JSON data.

Thanks!

Yes, your code can indeed be simplified, as Xiaoy312 already demonstrated:

// Obtain the Relations array:
return JObject.Parse(jsonString)["Relations"]
     // Take the categories of all relations, flattening them into a single enumerable:
    .SelectMany(relation => relation["Categories"])
    // Filter out duplicate category objects (JToken.EqualityComparer uses DeepEquals internally):
    .Distinct(JToken.EqualityComparer)
    // Materialize the results into a list:
    .ToList();

This is also more efficient:

Using dynamic means that several things (such as method overload resolution) cannot be done at compile-time but must be done at run-time, which involves additional work.
Distinct uses a set internally, so it doesn't need to compare a new category against all other categories found so far - an internal hashcode lookup is often sufficient.

dynamic can be useful, but it doesn't work with extension methods (which Linq heavily depends on), so in this case it's more of a hindrance. About the only benefit here is that relation.Categories is slightly easier to write than relation["Categories"], but you do pay for that at run-time, and it handicaps your tools (no compile-time checks, no automated refactoring). Also keep in mind that dynamic is 'contagious': any expression involving something dynamic also becomes dynamic.

Another problem is that dynamic code is less self-descriptive. Is category.Name a string, or could it be a { "name": "Identity", "id": "5" } object? That sort of uncertainty makes code more difficult to maintain, at least in my experience. Static typing can be more restrictive and cumbersome, but it gives you more guarantees and tells you more about what sort of data your program expects to handle. Unless you have a good reason to stick with dynamic (in that case, throw a .Cast<dynamic>() in just before the .ToList() call) I would recommend creating a Category class.

Finally, I would make the filename a parameter and rename Method to something more descriptive such as LoadCategoriesFromJsonFile, and I would put some thoughts into error handling (what if the file doesn't exist, or if it's corrupted).


Bisection method solver I have a simple bisection method solver that I was told it has some problems in design and I need my code to be reviewed, so I was hoping for someone to give me guideline on how to improve my code.

It starts with a delegate:

 public delegate double MyFun (double x) ;

A class to add functions:

     class functions
{
    public static double fun1(double x) => Math.Pow(x,2) - 3;
    public static double fun2(double x) => 5* Math.Pow(x, 3) - 2* Math.Pow(x, 2) + 3 * x - 2;
    public static double fun3(double x) => 2 * Math.Sqrt(x) ;
}

A class that has a function that solves the equation. It has a function that takes 4 input parameters: delegate, start point, end point, guess of solution.

class Bisection
{

    public static double Bisection_method (MyFun fun , double start , double end , double? guess)
    {
        if ( fun(start) * fun(end) > 0 )
        {
            Console.WriteLine("wrong Entry");
            return -1;
        }
        if (fun(start) == 0)
        {
          return start;
        }

        double avg,tolerance,sign;

        avg = (guess.HasValue) ? guess.Value : ( (start + end) / 2 );
        do
        {
            tolerance = Math.Abs ( fun(end) - fun(start) );

            sign = fun(start) * fun(avg);
            if (sign < 0)
                end = avg;
            else if (sign > 0)
                start = avg;

            avg = (start + end) / 2;
        }
        while ( tolerance > 0.0001 );

        return end;
    }
}

Then the main method:

class Program
{
    static void Main(string[] args)
    {
        MyFun fun1 = functions.fun1;

        double start = 0;
        double end = 500;
        double guess = 10;

        double answer = Bisection.Bisection_method(fun1, start, end, guess);
        Console.WriteLine($"The Root = {answer}");
    }
}

I was hoping for someone to help me how to improve this simple code design and there is also some cases that I need to handle.

There is a of lot room for improvements in your code so let us start straight ahead with your class
functions

Based on the .NET Naming Guidelines classes should be named using PascalCase casing which isn't the only problem here. Naming things is hard but its much harder to grasp at first glance what a class, method or field is used for if one uses names like function, MyFun or fun1..fun3. If you come back in a few weeks/months to fix a bug or to add a feature you still need to understand quickly what the code is doing which will be harder if you keep these names.

Math.Pow(x,2) should always be replaced by x * x which will be faster

Bisection

You are executing fun(start) at least one time but up to four times. By storing the result of the method call inside a variable you code will be easier to read and also be faster. The same applies to fun(end).

double avg,tolerance,sign; don't do this. Its to hard to read. Always declare one variable per line.

omitting braces {} is valid in C# but it is dangerous as well because it can lead to hidden and therefor hard to find bugs. But if you decide for yourself to take the risk you should at least be consequent with your style. Right now you are sometimes using braces and sometimes you don't.

If a value of a method parameter doesn't fit in the range of an expected value one should throw an ArgumentOutOfRangeException or if the value is plainly wrong one should throw an ArgumentException instead of writing to the console and returning a magic number (-1).

Numbers used like tolerance > 0.0001 should be stored in a meaningful named constant so every reader of the code exactly knows what 0.0001 represents.


Python object modelling with ABCs I'm modelling the core entities in a callcenter-related system that deals with Operators and groups ("pools") of them. While I've written some Python before, this is my first time building a larger project in it from scratch, and I'd like to know if my approach and code is pythonic enough or will get me into trouble later on.

I really like the idea of interfaces in general and for explorational design specifically, and while Python doesn't have those, ABCs seem like the next best equivalent thing. It there anything to be said about my use of them?

Any specific hints about my use of a MutableSet?

I'm a bit worried about file structures and putting every concrete class in its own file, as that results in from Foo.Bar.Baz import Baz; is that acceptable, or is the common wisdom to include more classes in a single module?

I'm also using a lot of type hinting; while I'm aware that it doesn't actually do anything in Python, it does help me keep the ducks straight in my head while typing and helps my IDE help me. Any concerns here?

# core/abstract_base_classes/operators.py

from abc import ABCMeta, abstractmethod
from core.abstract_base_classes.attributes import AttributeBag
from collections.abc import MutableSet
from uuid import UUID

class Operator(metaclass=ABCMeta):
    """
    An abstract class defining the interface for an Operator entity
    representing a call center agent.
    """

    @property
    @abstractmethod
    def id(self) -> UUID:
        pass

    @property
    @abstractmethod
    def status(self):
        pass

    @status.setter
    @abstractmethod
    def status(self, value):
        pass

    @property
    @abstractmethod
    def attributes(self) -> AttributeBag:
        pass

class Pool(MutableSet):
    """
    An abstract class defining the interface for an operator pool.
    """

    @abstractmethod
    def __contains__(self, operator: Operator):
        pass

    @abstractmethod
    def discard(self, operator: Operator):
        pass

    @abstractmethod
    def __iter__(self):
        pass

    @abstractmethod
    def add(self, operator: Operator):
        pass

    @abstractmethod
    def __len__(self):
        pass

# core/Entities/Operator.py

from core.abstract_base_classes.operators import Operator as AbstractOperator
from core.abstract_base_classes.attributes import AttributeBag as AbstractAttributeBag
from core.abstract_base_classes.attributes import AttributeTag as AbstractAttributeTag
from core.Entities.AttributeBag import AttributeBag
from core import status
from uuid import UUID, uuid4

class Operator(AbstractOperator):
    def __init__(self, attributes: AbstractAttributeBag=None):
        if not attributes:
            attributes = AttributeBag()

        super().__init__()
        self._id = uuid4()
        self._attributes = attributes
        self._status = status.INITIALIZED

    @property
    def id(self) -> UUID:
        return self._id

    @property
    def status(self):
        return self._status

    @status.setter
    def status(self, value):
        self._status = value

    @property
    def attributes(self) -> AbstractAttributeBag:
        return self._attributes

    @attributes.setter
    def attributes(self, attributes: AbstractAttributeBag):
        self._attributes = attributes

    def add_attribute(self, attribute: AbstractAttributeTag):
        self._attributes.add(attribute)

# core/Entities/OperatorPool.py

from core.abstract_base_classes import operators

class Pool(operators.Pool):
    def __init__(self):
        super().__init__()
        self._operators = set()

    def __iter__(self):
        return self._operators.__iter__()

    def __contains__(self, operator: operators.Operator):
        return operator in self._operators

    def discard(self, operator: operators.Operator):
        self._operators.remove(operator)

    def add(self, operator: operators.Operator):
        self._operators.add(operator)

    def __len__(self):
        return len(self._operators)

I'm a bit worried about file structures and putting every concrete class in its own file, as that results in from Foo.Bar.Baz import Baz; is that acceptable, or is the common wisdom to include more classes in a single module?

There's absolutely nothing wrong with putting multiple classes in the same file, especially if it makes the code more readable. That said, there's nothing saying you can't re-export your classes from another file if you would like the code and the module structure to be somewhat orthogonalized.

  I'm also using a lot of type hinting; while I'm aware that it doesn't actually do anything in Python, it does help me keep the ducks straight in my head while typing and helps my IDE help me. Any concerns here?

No concerns; this is a good thing to do from a documentation perspective and it's even better with IDE support.

I would be hesitant with core.abstract_base_classes; the standard name is the shortened ABC, so ABCs it would be. Even so, it seems strange to put all the ABCs in one place; why not put them with the classes that implement them?

In Operator, you do

if not attributes:
    attributes = AttributeBag()

This should be the explicit

if attributes is not None:
    attributes = AttributeBag()

since you're checking against a sentinel.

You also call super().__init__(); this seems pretty superfluous. You don't need to call super() and I'd avoid it unless it serves a purpose.

I can kind'a get why you'd want a property for id - to keep it read only. However, there's no reason to have status or attributes as a property. Don't, it doesn't buy you anything (and does have costs).

But this puts into question why you have the ABCs at all... why do you? If you had multiple classes, sure. You might want something to guide it. But as it stands, you're using it once and it's just overhead. Don't, it doesn't buy you anything significant (and does have costs).

Similar applies to Pool... but why not just use set? Not only does set do everything that Pool can do, it's better tested and supports more functionality. Further, everyone knows how to use set (I hope).

Don't build abstractions for the sake of abstractions. Build abstractions in order to simplify common functionality or allow extensibility where you plan to extend things. Abstractions are meant to make code simpler to understand, but if you build too many layers you drown out the intent that you're trying to express.


Looping through a collection In the situation where I am looping through a collection looking for an item, is it better practice to break before I return or should I break and then return?

Return during loop:

foreach(string item in items)
{
    string[] split = item.Split('|');
    if(split[0] == searchFor)
        return split[1];
}
return null;

Break then return:

string result = null;
foreach(string item in items)
{
    string[] split = item.Split('|');
    if(split[0] == searchFor)
    {
        result = split[1];
        break;
    }
}
return result;

If you break, you need to introduce an additional variable.  In my opinion, it's more clear to return as soon as possible from any function.


Page download with reattempts in Scala I wrote the following code to deal with possible network problems.

def getPage(browser: Browser, page: String): Document =
    {
      var attempts = 5
      while (attempts > 0) {
        try {
          return browser.get(page)
        } catch {
          case ste: java.net.SocketTimeoutException if attempts == 1 => throw ste
          case ste: java.net.SocketTimeoutException => attempts -= 1
        }
      }
      return null
    }

I am new to scala and would appreciate if there is a better, "scala-way" of implementing similar

Document and Browser are classes from Scalascraper lib which is a wrapper around JSoup lib.

Your code uses a number of things seldom if ever seen in idiomatic Scala:

return
null
throw
var

the land of no return

The final statement in a code block is the return value of that block, so the keyword return isn't needed. If you structure your code so that each block/method/function has only one exit point, at the end, then your code is easier to read and understand, and less prone to unexpected results.

Also, under some circumstances (I won't detail here) using return will do unexpected things. Easier to just avoid it altogether.

failure is an option

Code that might fail to complete correctly usually does one of two things: return a nonsense value, like null, which the calling code will test for (hopefully), or throw an exception, indicating the source of the problem, which some routine somewhere will catch (hopefully) and process.

Scala offers a third and better option: put the failure in the type system. If a routine might fail, put that in its return type, not its return value. That way the calling code can't ignore the possibility of failure.

In this case, instead of returning a Document, return a Try[Document]. Much like an Option, which is Some(value) or None, a Try is expressed as Success(value) or Failure(exception).

iteration without mutation

Functional Programming avoids data structures with mutable state. Scala has vars but they are discouraged. There are a number of different ways to iterate without using a var. In this case here's what I'd do.

Seq.iterate(Try(browser get page), 5)(_ orElse Try(browser get page))

This is a sequence of 5 results from the page requests, but a new attempt is made only if the previous one failed. The first Success is passed down the line to all subsequent positions in the sequence.

putting it all together

import util.Try

def getPage(browser :Browser, page :String) :Try[Document] =
  Seq.iterate(Try(browser get page), 5)(_ orElse Try(browser get page))
     .last


Remove adjacent duplicate characters Introduction

This problem is essentially equivalent to this HackerRank problem.

The task is to remove all duplicate characters that are adjacent to each other in a given String, until no adjacent characters are the same. If a String only contains adjacent duplicate characters then return an empty String.

Examples:

baab => bb => "" (return an empty String)
aabcd => bcd

Implementation

The core implementation logic uses Stacks to do most of the work.

Translate the input String to a Stack made up of Characters
Create a new Stack that will contain the non-duplicate Characters which will be outputted
pop through the input Stack

If the output Stack is empty, add the character
If the first element of the output Stack is the same Character as the popped Character then
pop the output Stack because this indicates that the two adjacent Characters are duplicate
Else
push the popped input Stack element to the output Stack

Feedback

I'm looking for general feedback as I'm still pretty green to Java. So if I have some serious design pattern issues, or if I'm missing something very obvious, I'd love to get feedback around that.

A couple thoughts on my mind:

Initially I tried implementing logic that iterated through Lists which worked but isn't as efficient as this implementation. One thing that I do like about this implementation is that it identifies the duplicate characters in one iteration, which you could achieve through Lists, I think, but the logic is more intuitive when using Stacks (I think).
Naming

I think the name AdjacentDuplicateCharactersRemover is relatively appropriate, but what are good names for the underlying methods? I struggled with this for a while because they are both doing similar things (removing / filtering characters).

Actual Implementation

public class AdjacentDuplicateCharactersRemover {

  public static String removeAdjacentDuplicateCharacters(final String candidate) {
    if (candidate.length() < 2) {
      return candidate;
    }

    final Stack<Character> originalCharacters = new Stack<>();
    for (char c : candidate.toCharArray()) {
      originalCharacters.push(c);
    }
    final Stack<Character> filteredCharacters = AdjacentDuplicateCharactersRemover.filterAdjacentDuplicateCharacters(originalCharacters);
    final StringBuilder stringBuilder = new StringBuilder();
    while (!filteredCharacters.empty()) {
      stringBuilder.append(filteredCharacters.pop());
    }
    return stringBuilder.toString();
  }

  public static Stack<Character> filterAdjacentDuplicateCharacters(final Stack<Character> characters) {
    if (characters.size() < 2) {
      return characters;
    }

    final Stack<Character> filteredCharacters = new Stack<>();
    while (!characters.empty()) {
      final char poppedChar = characters.pop();
      if (filteredCharacters.empty() || poppedChar != filteredCharacters.peek()) {
        filteredCharacters.push(poppedChar);
      } else {
        filteredCharacters.pop();
      }
    }

    return filteredCharacters;
  }
}

The naming of the class is OK, I think.  It could be better named AdjacentSameCharacterPairsRemover. "Same" is shorter than "duplicate". "Pairs" corrects a misunderstanding that I initially had, which was that "aaa" → "" instead of "aaa" → "a".  The name still wouldn't convey the idea that the elimination is applied repeatedly, but there's only so much information that you can fit into a name.

I don't think that the function name needs to be that long, if the class name is already descriptive.  The parameter name, candidate, is peculiar.  What office is that string running for, or what validation are you performing on it?

I don't recommend marking parameters and local variables as final — it just adds visual clutter.  Functions should be short enough that you can easily keep track of all the variables in your head simultaneously.

You don't need the special case for if (candidate.length() < 2); the algorithm works just fine without it.  The added complexity isn't worth the potential performance benefit.

I would work directly with the char[] and dispense with the Stack and StringBuilder.  By doing so, you would eliminate the need for extra storage, the boxing of chars as Characters, the synchronized behaviour of java.util.Stack, and the semi-deprecated status of java.util.Stack.  You also would't need to push all of the original characters onto the stack and then filter it, since the character array is the input.  You wouldn't need to translate the Stack back to a StringBuilder to make the String.  The code would be simplified so much that you could eliminate the helper function altogether.

public class AdjacentSameCharacterPairsRemover {
    // Suppress the default constructor
    private AdjacentSameCharacterPairsRemover() {}

    public static String process(String s) {
        char[] chars = s.toCharArray();
        int len = 0;      // Length of result (initially empty)

        for (int i = 0; i < chars.length; i++) {
            if (len == 0 || chars[i] != chars[len - 1]) {
                chars[len++] = chars[i];   // Push one char onto "stack"
            } else {
                len--;                     // Pop one char from "stack"
            }
        }
        return new String(chars, 0, len);
    }
}


Performing a regex search and saving results to CSV I am a Python beginner, and I wrote a simple Python program, which does the following:

search the pattern in the lines of a file (contains messages)
pick the information from the lines and save it to disk
remove the messages which matches with the regex (regular expression)
saves remaining messages to another file,

Regexps are not available, so I pick one message -> create regex for it -> remove the matching messages and repeat the same with remaining messages.

# coding: utf-8

# In[50]:

import re
import csv

# ### Run this part only once in the starting. From here

# In[2]:

# ### Change the directory to working folder and give the right filename (hdfcbk),
# ### if unsure what to do go to your folder and right click and copy the filen here, it will look like /home/XYZ/.../Your_folder_name/hdfcbk
smsFile = open('hdfcbk', 'r')
data = smsFile.read()
data = data.split('\n')
main_data = data
regex_list = []
regl = []

# In[3]:
def regex_search(pattern, file_name):
   remove_arr = []
   res = []
   remain_sms = []
   for sms in file_name:
       j= re.match(pattern,sms)
       if j != None:
           res.append(j.groupdict())
           remove_arr.append(sms)
       else:
           remain_sms.append(sms)
   return res, remove_arr, remain_sms

# In[4]:

def write_to_csv(result,csv_name):
    keys = result[0][0].keys()
    with open(csv_name, 'wb') as output_file:
        dict_writer = csv.DictWriter(output_file, keys, dialect='excel')
        dict_writer.writeheader()
        dict_writer.writerows(result[0])

# In[12]:

# ### To here, now the repetitive run start

# ### Update this pattern file

# In[1]:

pat1 = 'INR (?P<Amount>(.*)) deposited to A\/c No (?P<AccountNo>(.*)) towards (?P<Towards>(.*)) Val (?P<Date>(.*)). Clr Bal is INR (?P<Balance>(.*)) subject to clearing.'

# In[8]:

A = regex_search(pat1,main_data)

# ### Updating main_data to remaining messages

# In[11]:

main_data = A[2]

# ### Writing remaining sms to a file, you don't need to change the file name as it will be updated everything as you run the script. Just look at the remaining sms and make new regex

# In[21]:

with open('remaining_sms.txt', 'w') as fp:
    fp.write('\n'.join('%s' % x for x in main_data))

# ### Update the csv file

# In[ ]:

write_to_csv(A, 'hdfc_test_3.csv')

# ### Keeping all the regexes in one list, update the index number in [i,pat1]

# In[52]:

regl.append([1,pat1])

# ### Wrting the regex index to csv, run this part in the end, or if you're unsure that you will make the mistake run this part and keep changing the output file name

# In[53]:

with open("output.csv", "wb") as f:
    writer = csv.writer(f)
    writer.writerows(regl)

I commented everything in the code. Now the thing is, I need to send this task to some people, who does not know anything in coding. That's why I commented so much.

Can you please review my code and suggest what else I can do to improve the code so that people can run the code without any hassle?

10 Tips, Problems, etc.

Don't begin comments with # ###
Put the code that you had outside of functions in a main() function. This makes it so that if the module is imported, this code won't be run inadvertently. You can use this standard framework:

def main():
    # put code here that you want to run when file is executed

if __name__ == "__main__":
    main()

Spell words in your comments correctly, and wrap them to a new line if they are getting too long. (After a few years of coding I have found that the vast majority of comments needn't be longer than one line.)
Use with open("file.txt", 'r') as f: whenever working with files
Always end your file with a new line
When comparing to None use is and not instead of == and !=
Actually catch the three return values of the regex_search method explicitly with a well-named variable
No capital letters in variable names according to pep-8 guidelines
I find it much easier to use print than write when writing to a file inside of a with open() statement:

print(some_text, file=some_file, flush=True)

Use string.format() instead of the deprecated old string formatting tools (%s...)

# coding: utf-8

import re
import csv

def regex_search(pattern, file_name):
    remove_arr = []
    res = []
    remain_sms = []
    for sms in file_name:
        j = re.match(pattern, sms)
        if j is not None:
            res.append(j.groupdict())
            remove_arr.append(sms)
        else:
            remain_sms.append(sms)
    return res, remove_arr, remain_sms

def write_to_csv(result, csv_name):
    keys = result[0][0].keys()
    with open(csv_name, 'wb') as output_file:
        dict_writer = csv.DictWriter(output_file, keys, dialect='excel')
        dict_writer.writeheader()
        dict_writer.writerows(result[0])

def main():
    # Run this part only once in the starting. From here

    # change the directory to working folder and give the right filename (hdfcbk),
    # if unsure what to do go to your folder and right click and copy the file here,
    # it will look like /home/XYZ/.../Your_folder_name/hdfcbk
    with open('hdfcbk', 'r') as smsFile:
        data = smsFile.read()
    data = data.split('\n')
    main_data = data
    regl = []

    pat1 = 'INR (?P<Amount>(.*)) deposited to A\/c No (?P<AccountNo>(.*)) towards (?P<Towards>(.*)) Val (?P<Date>(.*)). Clr Bal is INR (?P<Balance>(.*)) subject to clearing.'

    # TODO - Use much more descriptive names...no idea what's going on here without searching for a while
    a, b, c = regex_search(pat1, main_data)

    # Updating main_data to remaining messages
    main_data = c

    # Writing remaining sms to a file, you don't need to change the file name as it will be updated
    # everything as you run the script. Just look at the remaining sms and make new regex.
    with open('remaining_sms.txt', 'w') as fp:
        fp.write('\n'.join('{}'.format(x) for x in main_data))

    # Update the csv file
    write_to_csv([a, b, c], 'hdfc_test_3.csv')

    # Keeping all the regexes in one list, update the index number in [i, pat1]
    regl.append([1, pat1])

    # Writing the regex index to csv, run this part in the end, or if you're unsure that you will
    # make the mistake run this part and keep changing the output file name.
    with open("output.csv", "wb") as f:
        writer = csv.writer(f)
        writer.writerows(regl)

if __name__ == "__main__":
    main()


Set theory clock code I wrote some JS to power an HTML/CSS "set theory" clock, and I'd like to share what I have so far and ask if anyone can suggest improvements.

Specifically, I'm wondering:

Should I bother checking to see if a variable is greater than zero before running the for loop?
Is there a better way to handle togging the classes? Setting all elements to "off" and then turning "on" the correct lamps seems a little heavy handed.
Should the tick() function be broken up?

For added context, you can view the code in context on GitHub and here is a live demo.

$(document).ready(function () {
    startClock();
});

function startClock() {
    'use strict';
    var delay = 1000;

    function tick() {
        var clockDate = new Date(),
            clockHours = clockDate.getHours(),
            clockMinutes = clockDate.getMinutes(),
            clockSeconds = clockDate.getSeconds(),
            fiveHours = Math.floor(clockHours / 5),
            fiveMinutes = Math.floor(clockMinutes / 5),
            singleHours = Math.floor(clockHours - (fiveHours * 5)),
            singleMinutes = Math.floor(clockMinutes - (fiveMinutes * 5));

        $(".circle").toggleClass("off");
        $("#display").text(clockHours + ":" + ((clockMinutes < 10) ? "0" + clockMinutes : clockMinutes) + ":" + ((clockSeconds < 10) ? "0" + clockSeconds : clockSeconds));

        $("[id^=h]").addClass("off");
        $("[id^=m]").addClass("off");

        // First row of lamps
        if (fiveHours > 0) {
            for (var i = 1; i <= fiveHours; i++) {
                $("#h5-" + i).removeClass("off");
            }
        }

        // Second row of lamps
        if (singleHours > 0) {
            for (var i = 1; i <= singleHours; i++) {
                $("#h1-" + i).removeClass("off");
            }
        }

        // Third row of lamps
        if (fiveMinutes > 0) {
            for (var i = 1; i <= fiveMinutes; i++) {
                if (i % 3 == 0) {
                    $("#m5-" + i).removeClass("off");
                } else {
                    $("#m5-" + i).removeClass("off");
                }
            }
        }

        // Fourth row of lamps
        if (singleMinutes > 0) {
            for (var i = 1; i <= singleMinutes; i++) {
                $("#m1-" + i).removeClass("off");
            }
        }

        setTimeout(tick, delay);
    }

    tick();
}

To answer your questions up front:

  Should I bother checking to see if a variable is greater than zero before running the for loop?

No need. The first thing the loop does is the i <= x comparison. And since i starts at 1, there's no need for you to double-check by saying if(x > 0).

  Is there a better way to handle togging the classes? Setting all elements to "off" and then turning "on" the correct lamps seems a little heavy handed.

jQuery's toggleClass() works in a few different ways. One of them accepts a boolean as its 2nd argument, which forces the toggle state: If true, the class name is added (if missing), if false, the class name is removed (if present).

So for instance, you might do this:

lamps.each(function (index) {
  $(this).toggleClass("on", index < count);
});

Where lamps is a set of elements you want to manipulate, and count is the number of lamps that should light up.

I've also flipped the on/off states, so on is the class name you add, while the lamps by default are off. This isn't really important, but considering you call the elements "lamps" (albeit indirectly; they're in divs classed as lamp-rows), it seems ever-so-slightly more natural to me that they default to their off state.

Anyway, the point is that you loop through each "lamp", turning it on or off as needed, instead of turning them all off first.

  Should the tick() function be broken up?

You've got some repetition with the loops and all that, so it'd probably be a good idea to extract the repeated parts. For instance, you could package the each-loop above in a function:

 function updateLamps(lamps, count) { ... }

Then simply pass it a set of elements, and the "lit lamp count".

For this, it might also be worth taking a look at the HTML. Right now, you have IDs for each individual lamp. But that's a tad too specific for my liking, since they're all essentially the same kind of thing. No great need to give them unique IDs.

I'd suggesting IDing the rows instead, and use a selector like #single-hours > .lamp (for instance) to find the lamps, where #single-hours is the ID of the row. This will also allow you to find them once and simply loop through them on each tick, rather than re-finding them each time.

Incidentally, you use a class name selector to find the blinking circle, although that, if anything, should have an ID as it's a unique element.

Another alternative is to build the elements in code, and inject them into the page. The advantage would be that you don't have tight coupling between the markup and the JS (if either changes, the other might well break). The disadvantage is that you now have markup in you JS (while the CSS is still somewhere else). While I'd suggest you try it out, I won't necessarily recommend one approach over the other, when it's just a single page/single feature thing. (If it's instead intended as a widget meant to be added to any kind of page, it's a different story).

Other things:

Instead of Math.floor(clockMinutes - (fiveMinutes * 5)) you can simply use the modulo operator to get the remainder of a division: clockMinutes % 5. Same for singleHours of course.
The line used to update the #display element is exceedingly long and repetitive. Consider making a formatTime function (or similar) to handle the heavy-lifting.
You have classes like rectangle-*, yellow and red which describe appearance much to specifically. What if you decide the hours should be purple triangles? The classes they have now are suddenly misleading. Always aim for class names the describe what something is, not how it looks.
Speaking of semantics, you could consider using ol list elements for the lamps, since they are essentially ordered lists of elements.
The whole $(document).ready(...) thing can be shortened to simply $(startClock);
Lastly, you could consider skipping some steps (loops) entirely. For instance, only if singleMinutes == 0 is there a need to update "five minute" lamps; only if fiveMinutes is zero, do you need to update the "single hour" lamps, etc.. In other words, you only need to update the more "coarse" steps, if the finer-grained ones roll over.

Here's a snippet that incorporates some of the comments above.

function startClock() {
    'use strict';
    var delay = 1000,
        display = $("#display"),
        secondsLamp = $("#seconds-lamp"),
        fiveHourLamps   = $("#five-hour-lamps > div"),
        oneHourLamps    = $("#one-hour-lamps > div"),
        fiveMinuteLamps = $("#five-minute-lamps > div"),
        oneMinuteLamps  = $("#one-minute-lamps > div");

    function toggleLamps(lamps, count) {
      lamps.each(function (index) {
        $(this).toggleClass('off', index >= count);
      });
    }

    function tick() {
        var clockDate = new Date(),
            clockHours = clockDate.getHours(),
            clockMinutes = clockDate.getMinutes(),
            clockSeconds = clockDate.getSeconds(),
            fiveHours = Math.floor(clockHours / 5),
            fiveMinutes = Math.floor(clockMinutes / 5),
            singleHours = clockHours % 5,
            singleMinutes = clockMinutes % 5;

        secondsLamp.toggleClass("off");
        display.text(clockHours + ":" + ((clockMinutes < 10) ? "0" + clockMinutes : clockMinutes) + ":" + ((clockSeconds < 10) ? "0" + clockSeconds : clockSeconds));

        toggleLamps(fiveHourLamps, fiveHours);
        toggleLamps(oneHourLamps, singleHours);
        toggleLamps(fiveMinuteLamps, fiveMinutes);
        toggleLamps(oneMinuteLamps, singleMinutes);

        setTimeout(tick, delay);
    }

    tick();
}

$(startClock);
body {
    background: rgba(0,0,0,0.35);
}

.clock {
    width: 16rem;
    margin: 3em auto 0 auto;
    font-family: sans-serif;
}
.circle {
    width: 5rem;
    height: 5rem;
    border-radius: 50%;
    border: 1rem solid rgba(204, 204, 204, 1);
    margin: 0 auto 0 auto;
}
.lamp-row {
    border-radius: 1rem;
    background: rgba(204, 204, 204, 1);
    display: flex;
    padding: 0.5rem;
    margin: 0.5rem 0 0 0;
}
.rectangle-large, .rectangle-small {
    border-radius: 0.5rem;
    height: 2.5rem;
    box-sizing:border-box;
    -moz-box-sizing:border-box;
    -webkit-box-sizing:border-box;
    margin: 0.1rem;
    display: inline-block;
}
.rectangle-large {
    width: 4rem;
}
.rectangle-small {
    width: 1.45rem;
}
.red {
    background: rgba(255, 0, 0, 1);
}
.red.off {
    background: rgba(255, 0, 0, 0.5);
}
.yellow {
    background: rgba(255, 255, 0, 1);
}
.yellow.off {
    background: rgba(255, 255, 0, 0.4);
}
#display {
    margin-top: 1rem;
    text-align: center;
}


Example String Class in C I wrote this small example piece of OOP being implemented in pure C. I wanted it to be reviewed for the following points:

Portability
Performance

And especially
Usability
"How does it look"

Implementation:

typedef char    XCFString;

struct XCFStringStatic {
    void *(*initWithCString)(const char *);
};

struct XCFStringClass {
    void (*release)(void);

    size_t      length;
    char        *value;
};

// Pointer to current class
XCFString **XCFStringCTX    = NULL;

// Release method
void XCFString__release(void)
{
    assert(XCFStringCTX != NULL);
    XCFString *ctx  = *XCFStringCTX;

    free(((char *)ctx) - sizeof(struct XCFStringClass));
}

// Init method
void *XCFString__initWithCString(const char *str)
{
    size_t str_len              = strlen(str);
    struct XCFStringClass *data = malloc(sizeof(* data) + str_len + 1);

    if (!data) return NULL;

    char *str_val               = ((char *)data) + sizeof(* data);

    memcpy(str_val, str, str_len + 1);

    data->length                = str_len;
    data->value                 = str_val;
    data->release               = XCFString__release;

    return data->value;
}

struct XCFStringStatic XCFStringStatic = {.initWithCString = XCFString__initWithCString};

#define priv_getMacroName1(_0, _1, macroName, ...)          macroName
#define getMacroName1(macroName, args...)                   priv_getMacroName1(_0, ##args, macroName ## _1, macroName ## _0)(args)

#define XCFString_0()       (XCFStringStatic)
#define XCFString_1(_var)   (XCFStringCTX=&(_var), ((struct XCFStringClass *)(((char *)_var) - sizeof(struct XCFStringClass))))
// Switch between XCFString_0 and XCFString_1, GNUC only
#define XCFString(args...)                                  getMacroName1(XCFString, ##args)

Test code for main.c

int main(int argc, const char **argv)
{
    // Pointer to our String class
    XCFString   *string     = NULL;

    // Initialize string with "Hallo Welt!"
    string = XCFString().initWithCString("Hallo Welt!");

    // Print it as if it were a normal string
    fprintf(stderr, "String        : %s\n",     string);
    fprintf(stderr, "Length        : %zu\n",    strlen(string));
    // Get the properties from the string
    fprintf(stderr, "Str.value     : %s\n",     XCFString(string)->value);
    fprintf(stderr, "Str.length    : %zu\n",    XCFString(string)->length);

    // Release the string
    XCFString(string)->release();
}

I would actually avoid the attempt to mimic C++ syntax. It makes things more complex and exposes unnecessary details.

For me it would be equally object-oriented with the following C interface:

#ifdef __cplusplus
#extern "C" {
#endif

typedef struct XCFString_
{
    char unused__;
} XCFString;

XCFString* XCFString_Init();
XCFString* XCFString_InitFromCString(char const* sz);
size_t XCFString_Length(XCFString const* xcfs);
char const* XCFString_Value(XCFString const* xcfs);
void XCFString_Release(XCFString* xcfs);

#ifdef __cplusplus
}
#endif

The above interface hides the details while allowing polymorphism; you could e.g. keep the "vtable" implementation (but hidden) and provide different init-functions.

I really don't follow all that macro voodoo but having a mutable shared state - the "pointer to current class" - seems susceptible to race conditions.


Computing nth Tribonacci number at compile time using C++ template metaprogramming facilities I have this small C++ program for computing the \$n\$th Tribonacci number at compile-time. My main concern is: is it possible to get rid of the statickeyword (I had to use it in order to silence my compiler (VS 2019))?
Code
#include <cstdint>
#include <iostream>

template<uint64_t N>
struct tribonacci {
    constexpr static uint64_t value = tribonacci<N - 1>::value +
                                      tribonacci<N - 2>::value +
                                      tribonacci<N - 3>::value;
};

template<>
struct tribonacci<0> {
    constexpr static uint64_t value = 0;
};

template<>
struct tribonacci<1> {
    constexpr static uint64_t value = 0;
};

template<>
struct tribonacci<2> {
    constexpr static uint64_t value = 1;
};

int main() {
    std::cout << tribonacci<6>::value;
    return 0;
}

This is a pretty cool practice project, and your implementation is sound.
First to answer your concern… can you drop the static? The answer is no… and yes.
The way you’ve implemented the solution, no, you really can’t drop the static. The code doesn’t really make sense without it. If you are doing tribonacci<N>::value, and tribonacci<N> is a type (as it is in your code), then value must be a static data member. That’s what the :: means (in this context); tribonacci<N> is a type, and value is a property of the type.
However….
This is how you would implement a constexpr tribonacci sequence generator in C++11. That’s 3 versions old now, and counting. And one of the things that has advanced the most in each new version of C++ is constexpr capability.
If you don’t want value to be a class data member, you could make it a regular data member, like so:
template <std::size_t N>
struct tribonacci
{
    std::int_fast64_t const value = tribonacci<N - 1>{} + tribonacci<N - 2>{} + tribonacci<N - 3>{};

    constexpr operator std::int_fast64_t() const noexcept { return value; }
};

template <>
struct tribonacci<0>
{
    std::int_fast64_t const value = 0;

    constexpr operator std::int_fast64_t() const noexcept { return value; }
};

template <>
struct tribonacci<1>
{
    std::int_fast64_t const value = 0;

    constexpr operator std::int_fast64_t() const noexcept { return value; }
};

template <>
struct tribonacci<2>
{
    std::int_fast64_t const value = 1;

    constexpr operator std::int_fast64_t() const noexcept { return value; }
};

auto main() -> int
{
    constexpr auto sixth_trib_num = tribonacci<6>{}; // to prove it's constexpr

    std::cout << tribonacci<0>{} << '\n';       // will auto-convert to std::int_fast64_t
    std::cout << tribonacci<1>{}.value << '\n'; // or you can manually request the value
    std::cout << tribonacci<2>{} << '\n';
    std::cout << tribonacci<3>{} << '\n';
    std::cout << tribonacci<4>{} << '\n';
    std::cout << tribonacci<5>{} << '\n';
    std::cout << sixth_trib_num << '\n';
}

As you can see, no more static, but now you need to actually create an object (like tribonacci<6>{}) rather than just using the type (like tribonacci<6>), because now value is a property of the object, not the type (like tribonacci<6>{}.value).
Perhaps an even better option is to use variable templates. After all, each tribonacci number is just a number… it doesn’t need to be a type.
template <std::size_t N>
inline constexpr auto tribonacci = tribonacci<N - 1> + tribonacci<N - 2> + tribonacci<N - 3>;

template <>
inline constexpr auto tribonacci<0> = std::int_fast64_t{0};

template <>
inline constexpr auto tribonacci<1> = std::int_fast64_t{0};

template <>
inline constexpr auto tribonacci<2> = std::int_fast64_t{1};

auto main() -> int
{
    std::cout << tribonacci<0> << '\n';
    std::cout << tribonacci<1> << '\n';
    std::cout << tribonacci<2> << '\n';
    std::cout << tribonacci<3> << '\n';
    std::cout << tribonacci<4> << '\n';
    std::cout << tribonacci<5> << '\n';
    std::cout << tribonacci<6> << '\n';
}

With this, tribonacci<6> is a std::int_fast64_t constant. There are no classes to instantiate or any other cruft. This probably the way you want to go in C++14 and beyond; certainly in C++20 and beyond.
So, no, you can’t drop the static if you’re using the design you currently have. But there are other, better, alternatives in more modern versions of C++.
Now for your actual code review….
template<uint64_t N>
struct tribonacci {

First, if you’re going to use uint64_t, it should be spelled std::uint64_t.
But using uint64_t is generally a bad idea. You see, uint64_t is an optional type; it is not guaranteed to exist. And if it does, it is not guaranteed to be an efficient type; it could actually require significant emulation to work, and it could be tragically slow.
Better options are unsigned long long, std::uint_fast64_t, std::uint_least64_t. Which one you choose depends on what you really want:

unsigned long long: This is guaranteed to be at least 64 bits… but could (theoretically) be 128 bits, 256 bits, or anything else. If you only need 64 bits, it could be overkill. However, it will probably be at least as efficient (speed-wise, not space-wise) as any other option, if not more.
std::uint_fast64_t: If you really just need a 64-bit type, this should be your default choice. It will be the fastest type supported by the architecture that is at least 64 bits. So it may be more than 64 bits (like unsigned long long it may be 128 bits, or 256 bits, or whatever). But it may be smaller than unsigned long long, and probably as fast.
std::uint_least64_t: This is what you use when you want a type that is at least 64 bits, but you care more about conserving space than getting maximum speed. This will be the smallest type the platform supports that can hold 64-bit values… even if that type requires lots of code to emulate.

So, in a table, your options are (note that the size and speed are the worst case estimates):

Type
Size
Speed
Notes

std::uint64_t
Bad
Bad
Not guaranteed to exist

unsigned long long
Bad
Excellent
Good choice if you need at least 64 bits, but much more is fine too

std::uint_fast64_t
Good
Good
Best choice if you only need 64 bits

std::uint_least64_t
Excellent
Bad
Best choice for saving space

There is another issue with uint64_t, and that is that it’s an unsigned type. Using unsigned types for general purpose numbers is a bad idea, for a number of reasons. You should only use unsigned numbers if you are doing bit-twiddling, or if you explicitly need modulo arithmetic. Otherwise, signed should be the default choice. The problems unsigned numbers cause are not worth the one extra bit of space.
Of course, you could also let the user pick the type:
template <std::size_t N, typename T = int> // defaults to regular old int
inline constexpr auto tribonacci = T(tribonacci<N - 1> + tribonacci<N - 2> + tribonacci<N - 3>);

// note in c++20, you could also constrain the type, like so:
//  template <std::size_t N, std::integral T = int>

template <typename T>
inline constexpr auto tribonacci<0, T> = T(0);

template <typename T>
inline constexpr auto tribonacci<1, T> = T(0);

template <typename T>
inline constexpr auto tribonacci<2, T> = T(1);

// if you're working with int_fast64_t a lot, then make a handy alias
template <std::size_t N>
inline constexpr auto tribonacci64 = tribonacci<N, std::int_fast64_t>;

auto main() -> int
{
    std::cout << tribonacci<0> << '\n';                 // int(0)
    std::cout << tribonacci<1, long> << '\n';           // long(0)
    std::cout << tribonacci<2, std::int64_t> << '\n' ;  // int64_t(1)
    std::cout << tribonacci64<3> << '\n';               // int_fast64_t(1)
}

Finally, I don’t know if you noticed, but in all the examples I made, I made the index type std::size_t, and the value type something else: int, std::uint_fast64_t or whatever. That’s because when you do auto v = tribonacci<N>, the N has nothing to do with the actual tribonacci number. It’s just the counter to pick which tribonacci number you want. v is the actual tribonacci number. That’s what you want to be at least 64 bits or whatever. N doesn’t need to be 64 bits; you’re going to blow past 64 bits by the 75th tribonacci number, so you only need N to be able to hold 7 bits for 64-bit tribonacci numbers.
std::size_t is the default type in C++ for counting, indexing, sizes, and so on. So it makes sense for N to be std::size_t. As for the value type… that should be whatever the user wants. Above I went with int by default (as you always should).
That’s pretty much it for the review. My recommendations:

Don’t use uint64_t as the value type for the tribonacci number:

Don’t use unsigned types for general numbers.
Don’t use fixed size types. They’re not portable, and even when they are, they might be inefficient.
Let the user pick the type, though you can offer a sensible default (which should probably be int).
It’s std::uint64_t anyway.

Don’t use uint64_t as the index type:

The default index type is std::size_t.

Rather than old-school template meta-programming, using types for each tribonacci number, use variable templates.


Return a collection of most frequent characters in a string My task was write a method that has input parameter - string  and this method must return a collection of symbols which are most frequently , for example:

input: ааа bbb с
ouput: а, b
input:  ааа bb с
ouput: a
а, b, с

а b с

Here is my bad code:

 static void test(string s)
        {
            int[] cc = new int[255];
            char c;

            for (c = (char)0; c < 255; c++)
            {
                cc[c] = 0;
            }

            for (int i = 0; i < s.Length; i++)
            {
                cc[s[i]]++;
            }
            int cntMax = 0;

            for ( c =(char) 0; c < 255; c++)
            {
             if(cc[c] > cntMax)
                {
                    cntMax = cc[c];
                }
            }
            string L = "";
            if(cntMax >0 )
            {
                for ( c = (char)0; c < 255; c++)
                {
                    if(cc[c] == cntMax)
                    {
                        if(c > (char) 32)
                        {
                            L = L + c + " ";
                            Console.WriteLine(L);
                        }
                    }

                }
            }
        }

So  , I don't sure if understand  , I must return collection of symbols , how to do it? How can I change my code?? And also I have a question about input: if I input a b c , then my program count spaces instread of symbols , how to fix it? Sorry for my very bad code

Text and data structures

C# uses UTF-16 internally, so a char is actually 2 bytes long (and even then not all Unicode characters can be stored in a char). An array of 256 (a single byte can have 256, not 255, different values) is not sufficient. A Dictionary<char, int> would be more suitable here. For example, the following code is not limited to Latin characters, so feeding it some Cyrillic should work just fine:

var charCounts = new Dictionary<char, int>();
foreach (char c in s)
{
    if (!charCounts.ContainsKey(c))
        charCounts[c] = 1;
    else
        charCounts[c]++;
}

You said that you want to ignore space characters, but your code also ignores some control characters (but not all). You may want to use char.GetUnicodeCategory to determine which characters should be counted (there are several categories: various kinds of letters, digits, control characters and so on).

As for returning multiple characters, that's what arrays and lists are for. Instead of appending characters to a string, add them to a list and return that list when you're finished.

Other comments

Don't use 'magic values' (such as that 255 that's appearing several times). When you iterate an array, use its Length property: for (int i = 0; i < cc.Length; i++).
Using a char to index an array makes your code a little confusing to read. I don't see any advantages over using a 'standard' int.
I would also not recommend reusing an indexing variable (c): it's easy to introduce bugs by forgetting to 'reset' its value.
You don't need to initialize an array of ints with 0's: C# already does that for you (0 is the default value for int).
Try using meaningful variable names: text or input instead of s, charCounts or even characterCounts instead of cc, and so on. It'll make your code easier to understand, and that's a good thing when you have to revisit code a few months later.
The easiest way to get the maximum value from a collection is to use Linq's Max method: cc.Max(). There are also several other Linq methods that could make this method easier to write, such as GroupBy, Where and  ToDictionary.


Facebook coding interview I have joined the Facebook interview recently. And the following question has been asked on the coding interview.

Given a sequence of numbers and a total target, return whether there exists a continuous sub-sequence that sums up to target.
[1, 3, 1, 4, 23], 8 : True (because 3 + 1 + 4 = 8)
[1, 3, 1, 4, 23], 7 : False

I have written the exact pseudo-code to solve the solution.
Which describes the inside of the method. The approach is the sliding window.

The start and end index starts from 0 and through the array it continues to increment the end index and keep adding the existing value array[end] to the temp value if the temp is smaller than the target sum. If every time checks if temp reached the target value, it is it returns true, else if it is bigger form the temp it starts to increment the start index and shrink the window form the beginning. And decreasing the exact value from the temp. And keep checking if it is equal to the target value after it is decreased.

After the interview result came, they told me that "I have solved it wrong because the sequence must be contiguous". I am %100 sure that this is the correct answer, and they saying is I was wrong and I should try 12 months later.

Please put your comment about my solution? What would be the problem to cause misunderstanding?

// [1, 3, 1, 4, 23]

int start  = 0
int end  = 0

int total = 0;
int target = 8;

while(end < arr.length){

  total += arr[end]; // 1 3 1 4 = 9

  while(total>=target){

     total-=arr[start]; // 8

     start++;

     if(total == target){
       return true;
     }

  }
  end++;
}

return false;

I am %100 sure that this is the correct answer,

I'm not so sure... I dumped your code into an online compiler*. I made very minor adjustments, moving target to a parameter and adding semicolons which aren't a big deal since you probably didn't have a compiler when writing it. I did not modify your code beyond that and treated it like a black-box.

import java.util.Arrays;

class Main {
  static boolean test(int[] arr, int target) {

    int start = 0;
    int end = 0;

    int total = 0;

    while (end < arr.length) {

      total += arr[end]; // 1 3 1 4 = 9

      while (total >= target) {

        total -= arr[start]; // 8

        start++;

        if (total == target) {
          return true;
        }

      }
      end++;
    }

    return false;
  }

  static class Input {
    int[] sequence;
    int target;

    Input(int[] sequence, int target) {
      this.sequence = sequence;
      this.target = target;
    }
  }

  public static void main(String[] args) {
    Input[] inputs = new Input[] {
      new Input(new int[]{1, 3, 1, 4, 23}, 8),
      new Input(new int[]{1, 3, 1, 4, 23}, 7),
      new Input(new int[] {}, 0),
      new Input(new int[] {}, 1),
      new Input(new int[] { 0 }, 0),
      new Input(new int[] { 0 }, 1),
      new Input(new int[] { 1 }, 1),
      new Input(new int[] { -1 }, -1),
      new Input(new int[] { Integer.MAX_VALUE }, Integer.MAX_VALUE),
      new Input(new int[] { 1, Integer.MAX_VALUE }, Integer.MAX_VALUE),
      new Input(new int[] { Integer.MIN_VALUE }, Integer.MIN_VALUE),
      new Input(new int[] { 1, 1, 1 }, 2),
      new Input(new int[] { 1, 1 }, 2),
      new Input(new int[] { 1, 0, 1 }, 2),
    };

    for (Input input : inputs) {
      System.out.print(String.format("%s, %d => ", Arrays.toString(input.sequence), input.target));
      try {

      System.out.println(
          test(input.sequence, input.target));
      } catch (Exception e) {
        System.out.println("Caught exception: " + e.getMessage() );
      }
    }
  }
}

That produced this output:

OpenJDK Runtime Environment (build 10.0.2+13-Ubuntu-1ubuntu0.18.04.4)
[1, 3, 1, 4, 23], 8 => true
[1, 3, 1, 4, 23], 7 => false
[], 0 => false
[], 1 => false
[0], 0 => true
[0], 1 => false
[1], 1 => false
[-1], -1 => Caught exception: Index 1 out of bounds for length 1
[2147483647], 2147483647 => false
[1, 2147483647], 2147483647 => false
[-2147483648], -2147483648 => Caught exception: Index 1 out of bounds for length 1
[1, 1, 1], 2 => false
[1, 1], 2 => false
[1, 0, 1], 2 => false

Your code was initially not wrapped into a method. Try to make it look like production quality code. That's ostensibly what the interview is looking at and writing all day.
Since it wasn't in a method, it implies you didn't consider writing tests.
Writing tests made it trivial to reveal cases that threw exceptions.
You didn't specify what should happen with an empty list which implies you didn't ask about that.
^^^ same with overflows. "Inputs that may cause overflows will cause undefined behavior" is a perfectly acceptable precondition in an interview IF you acknowledge that you have recognized this possibility, have a  notion of how to handle it, and the interviewer allows it.
It's also possible to yield incorrect answers with fairly trivial inputs, e.g. [1, 1]

*I do not recommend this online IDE, it felt very buggy. If someone has a better alternative I would be grateful.


Injecting SelectList objects into ViewData to enable using EditorFor on dropdown based properties I have developed a system whereby I use an attribute to state which SelectList to use for an 'FK' property. I would appreciate some feedback mainly on by good practice, and also on any ways to do this better. Let me start my exposition with a demo view model:

public class DemoModel
{
    [DropDownList("LanguageSelect")]
    public int? LanguageId { get; set; }
    public SelectList LanguageSelect { get; set; }
}

Now when I use the Razor markup @EditorFor(m => m.LanguageId), I get a drop-down populated from the LanguageSelect list. I get this because the DropDownListAttrbute class attaches the select list name to the LanguageId model:

public class DropDownListAttribute : UIHintAttribute, IMetadataAware
{
    public DropDownListAttribute(string selectListName) : base(KnownUiHints.DropDown, KnownPresentationLayers.Mvc, selectListName)
    {
        SelectListName = selectListName;
    }
    public string SelectListName { get; set; }
    public void OnMetadataCreated(ModelMetadata metadata)
    {
        var listProp = metadata.ContainerType.GetProperty(SelectListName);
        metadata.AdditionalValues[KnowMetadataKeys.SelectListName] = SelectListName;
    }
}

All my view models derive from ViewModel, which offers a SelectListDictionary property:

private IDictionary<string, SelectList> _selectListdictionary;
public virtual IDictionary<string, SelectList> SelectListDictionary
{
    get
    {
        if (_selectListdictionary == null)
        {
            var props = GetType().GetProperties().Where(p => p.PropertyType == typeof(SelectList));
            _selectListdictionary = props.ToDictionary(prop => prop.Name, prop => (SelectList)prop.GetValue(this, null));
        }
        return _selectListdictionary;
    }
}

In my base controller, I override the View method to pull the entire select list dictionary from the view model, and insert it into the view's viewdata, making it available for the editor template:

protected override ViewResult View(string viewName, string masterName, object model)
{
    var result = base.View(viewName, masterName, model);
    if ((model is ViewModel) && (!ViewData.ContainsKey(KnowMetadataKeys.ViewDataSelectLists)))
    {
        var vm = (ViewModel)model;
        result.ViewData.Add(KnowMetadataKeys.ViewDataSelectLists, vm.SelectListDictionary);
    }
    return result;
}

The editor template then retrieves the list it requires from the select list dictionary and builds a select input element:

@using Erisia.Constants
@{
    var list = (SelectList)ViewData.ModelMetadata.AdditionalValues[ViewData.ModelMetadata.AdditionalValues[KnowMetadataKeys.SelectListName].ToString()];
    var listWithSelected = new SelectList(list.Items, list.DataValueField, list.DataTextField, Model);
}
@Html.DropDownListFor(m => Model, listWithSelected, " - select - ")

So what say you all?

I'm assuming this code hasn't been reviewed yet because it's... beautiful - and I'm still searching for a better word. I mean, wow that's clever, I want that!!

The only thing I can see here, is in the View method:

protected override ViewResult View(string viewName, string masterName, object model)
{
    var result = base.View(viewName, masterName, model);
    if ((model is ViewModel) && (!ViewData.ContainsKey(KnowMetadataKeys.ViewDataSelectLists)))
    {
        var vm = (ViewModel)model;
        result.ViewData.Add(KnowMetadataKeys.ViewDataSelectLists, vm.SelectListDictionary);
    }
    return result;
}

I would suggest a safe cast instead of model is ViewModel followed by a cast, so it would look more like this - note that I dropped a few redundant parentheses:

protected override ViewResult View(string viewName, string masterName, object model)
{
    var result = base.View(viewName, masterName, model);
    var vm = model as ViewModel;
    if (vm != null && !ViewData.ContainsKey(KnowMetadataKeys.ViewDataSelectLists))
    {
        result.ViewData.Add(KnowMetadataKeys.ViewDataSelectLists, vm.SelectListDictionary);
    }
    return result;
}

Other than that, I agree with your naming (except maybe vm could be called viewModel, but vm is pretty descriptive in this specific context) and naming conventions, and there's nothing much left to say as far as I'm concerned - good job! :)


C# Better logic for viewmodel Any advice on how to make this code; cleaner, more effective, just overall better!
Program setup object that is coming from the database. it loop the object adding to the viewmodel object. Then the object has a certain properties base on type. The config type will only have the setting and value objects.
class Program
{
    static void Main(string[] args)
    {
        //Setting up fake objects
        List<Meeting> meetingList = new List<Meeting>();
        meetingList.Add(new Meeting() { Id =1 ,Name="a", Type= "System"  });
        meetingList.Add(new Meeting() { Id = 2, Name = "b", Type = "System" });
        meetingList.Add(new Meeting() { Id = 3, Name = "c", Type = "Config", Setting="Control", Value="2"  });
        meetingList.Add(new Meeting() { Id = 4, Name = "d", Type = "Config", Setting = "Control", Value = "3" });
        meetingList.Add(new Meeting() { Id = 5, Name = "e", Type = "Config", Setting = "", Value = "" });

        //Real Code
        List<Meeting> meetingviewmodel = new List<Meeting>();
        foreach (var meeting in meetingList)
        {
           Meeting mvm = new Meeting();
           meeting.Id = mvm.Id;
           meeting.Name = mvm.Name;
           meeting.Type = mvm.Type;
           if (meeting.Type == "System")
           {
               meetingviewmodel.Add(mvm);
           }
           if (meeting.Type == "Config" && meeting.Setting != null)
           {
              meeting.Setting = mvm.Setting;
              meeting.Value = mvm.Value;
              meetingviewmodel.Add(mvm);

            }
        }
    }

    class Meeting
    {
        public int Id { get; set; }
        public string Name { get; set; }
        public string Type { get; set; }
        public string Setting { get; set; }
        public string Value { get; set; }
    }
}

First, I think you've flipped the assigning without noticing that you're assigning null values to the meeting properties (e.g meeting.Id = mvm.Id;) notice that you've already create a new instance of Meeting object, and yet, you replaced the actual values with the new instance values, while you are expected to do the opposite.
So, I assumed you meant to do this mvm.Id = meeting.Id;.
If you need to add the Meeting object to the model based on the Type property. You can do this :
foreach (var meeting in meetingList)
{
    // if this element's Type property is null or empty
    // then skip and go to the next one
    // this condition will be applied on all meeting types
    if(string.IsNullOrEmpty(meeting.Type)) { continue; }

    meetingviewmodel.Add(meeting);
}

this will add all meeting objects that have defined Type, this will include System and Config types along with any other defined types.
if you only need to add System and Config types  meetings, and ignore the rest, you can do this :
foreach (var meeting in meetingList)
{
    // if you want to only add System & Config meetings
    // you can use this condition
    if(!string.IsNullOrEmpty(meeting.Type) && (meeting.Type == "System" || meeting.Type == "Config"))
    {
        meetingviewmodel.Add(meeting);
    }
}

or if you into LINQ :
var meetingviewmodel = meetingList
            .Where(x=> !string.IsNullOrEmpty(x.Type) && (x.Type == "System" || x.Type == "Config"))
            .ToList();


QTableView implementation I've put together my first table using Qt's QTableView and a custom data model. Currently, some data is populated on initialisation, and the user has the option of adding and deleting entries.
#include "mainwindow.h"
#include "ui_mainwindow.h"
#include <iostream>
using namespace std;
QList<Contact> contacts;

MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::MainWindow)
{
    ui->setupUi(this);

    QList<QString> contactNames;
    QList<QString> contactPhoneNums;

    contacts.append(Contact{4,"Adam"});
    contacts.append(Contact{5,"James"});
    contacts.append(Contact{2,"Emily"});
    contacts.append(Contact{1,"Mark"});

    // Create model:
    PhoneBookModel = new TestModel(this);

    // Connect model to table view:
    ui->tableView->setModel(PhoneBookModel);
    ui->tableView->setSelectionBehavior(QAbstractItemView::SelectRows);
    ui->tableView->setSelectionMode( QAbstractItemView::SingleSelection );

    // Make table header visible and display table:
    ui->tableView->horizontalHeader()->setVisible(true);
    ui->tableView->show();

    //slots to signals

    connect(ui->tableView->selectionModel(), &QItemSelectionModel::selectionChanged, this, &MainWindow::foo);

}

void MainWindow::foo(){
    QItemSelectionModel *selected = ui->tableView->selectionModel();
    QModelIndexList rowList = selected->selectedRows();

    if (rowList.size() == 0){
        ui->label->setText("YAY");
        ui->tableView->selectRow(0);

    } else {
        ui->label->setText(contacts.at(rowList.at(0).row()).name);
    }
}

MainWindow::~MainWindow()
{
    delete ui;
}

TestModel::TestModel(QObject *parent) : QAbstractTableModel(parent)
{
}

int TestModel::rowCount(const QModelIndex &parent) const
{
    Q_UNUSED(parent);
    return contacts.length();
}

int TestModel::columnCount(const QModelIndex &parent) const
{
    Q_UNUSED(parent);
    return 2;
}

QVariant TestModel::data(const QModelIndex &index, int role) const
{
    if (!index.isValid() || role != Qt::DisplayRole) {
        return QVariant();
    }
    if (index.column() == 0) {
        return contacts[index.row()].id;
    } else if (index.column() == 1) {
        return contacts[index.row()].name;
    }
    return QVariant();
}

void TestModel::update(int row){

   QModelIndex i = this->index(row,0,QModelIndex());
   QModelIndex p = this->index(row,1,QModelIndex());

   emit dataChanged(i,p);
   emit layoutChanged();
}

QVariant TestModel::headerData(int section, Qt::Orientation orientation, int role) const
{
    if (role == Qt::DisplayRole && orientation == Qt::Horizontal) {
        if (section == 0) {
            return QString("Name");
        } else if (section == 1) {
            return QString("Phone");
        }
    }
    return QVariant();
}

void MainWindow::on_DeleteButton_clicked()
{
    QItemSelectionModel *selected = ui->tableView->selectionModel();
    QModelIndexList rowList = selected->selectedRows();

    for (int i = 0; i < rowList.count(); i++)
    {

        contacts.removeAt(rowList.at(i).row());
        if (rowList.at(i).row() == contacts.size()){ //last element
            ui->tableView->selectRow(rowList.at(i).row()-1);

        } else {

        }

        PhoneBookModel->update(rowList.at(i).row());
        foo();

    }

}

void MainWindow::on_AddButton_clicked()
{
    int id = ui->id->text().toInt();
    QString name = ui->name->text();

    contacts.append(Contact{id,name});

    PhoneBookModel->update(contacts.size()-1);

}

It all works fine, although I worry that I may be using QTableView incorrectly and/or inefficiently. Any suggestions on how to improve my code while maintaining the same functionality would be appreciated.

I can't find anything wrong.If really give some suggestions, it would be the case of the function name 'on_AddButton_clicked' and 'on_DeleteButton_clicked'.It will be better to use 'addButton' and 'deleteButton'.


Drawing a dashed line with pygame def draw_dashed_line(surf, color, start_pos, end_pos, width=1, dash_length=10):
    x1, y1 = start_pos
    x2, y2 = end_pos
    dl = dash_length

    if (x1 == x2):
        ycoords = [y for y in range(y1, y2, dl if y1 < y2 else -dl)]
        xcoords = [x1] * len(ycoords)
    elif (y1 == y2):
        xcoords = [x for x in range(x1, x2, dl if x1 < x2 else -dl)]
        ycoords = [y1] * len(xcoords)
    else:
        a = abs(x2 - x1)
        b = abs(y2 - y1)
        c = round(math.sqrt(a**2 + b**2))
        dx = dl * a / c
        dy = dl * b / c

        xcoords = [x for x in numpy.arange(x1, x2, dx if x1 < x2 else -dx)]
        ycoords = [y for y in numpy.arange(y1, y2, dy if y1 < y2 else -dy)]

    next_coords = list(zip(xcoords[1::2], ycoords[1::2]))
    last_coords = list(zip(xcoords[0::2], ycoords[0::2]))
    for (x1, y1), (x2, y2) in zip(next_coords, last_coords):
        start = (round(x1), round(y1))
        end = (round(x2), round(y2))
        pygame.draw.line(surf, color, start, end, width)

This function takes two coordinates and draws a colored dashed line from the first to the second coordinate. Line a = abs(x2 - x1) to line dy = dl * b / c calculates the amount x and y change for dl (dash_length).
Because dx and dy are floats, I had to use numpy.arange (built-in range() doesn't allow floating-point).

Here's an example:

draw_dashed_line(screen, RED, (0, 0), (800, 600), dash_length=5)

Documentation for pygame.draw.line:
http://www.pygame.org/docs/ref/draw.html#pygame.draw.line

Instead of storing all the values you're trying to draw, it might be more efficient to just get the slope of the drawn line, and compute dashes at draw time.

Considering that there's a lot of mirrored code here, consider using a point class to handle both the x and the y values at the same time!

class Point:
    # constructed using a normal tupple
    def __init__(self, point_t = (0,0)):
        self.x = float(point_t[0])
        self.y = float(point_t[1])
    # define all useful operators
    def __add__(self, other):
        return Point((self.x + other.x, self.y + other.y))
    def __sub__(self, other):
        return Point((self.x - other.x, self.y - other.y))
    def __mul__(self, scalar):
        return Point((self.x*scalar, self.y*scalar))
    def __div__(self, scalar):
        return Point((self.x/scalar, self.y/scalar))
    def __len__(self):
        return int(math.sqrt(self.x**2 + self.y**2))
    # get back values in original tuple format
    def get(self):
        return (self.x, self.y)

def draw_dashed_line(surf, color, start_pos, end_pos, width=1, dash_length=10):
    origin = Point(start_pos)
    target = Point(end_pos)
    displacement = target - origin
    length = len(displacement)
    slope = displacement/length

    for index in range(0, length/dash_length, 2):
        start = origin + (slope *    index    * dash_length)
        end   = origin + (slope * (index + 1) * dash_length)
        pygame.draw.line(surf, color, start.get(), end.get(), width)


A generic file helper for my future projects I am learning Python and I know that a lot of my future projects will be based on files and managing them. How does this look? Is anything in here redundant or overcomplicated?

import os
import zipfile
import shutil
from datetime import datetime as dt

class File:
    def __init__(self, path, datetime=True):
        self.path = path
        self.datetime = datetime

        self.abs_path = os.path.abspath(self.path)

        self._dt_manager = lambda prop: prop if not self.datetime else dt.fromtimestamp(prop)
        self._os_path = lambda func, path_=self.abs_path, *args, **kwargs: getattr(os.path, func)(path_, *args, **kwargs)

        self.readable = False
        self.data = None
        self.lines = None
        self.line_count = None

        try:
            with open(self.path, "r") as target:
                self.data = target.read()
                self.lines = self.data.splitlines()
                self.line_count = len(self.lines)
                self.readable = True

        except UnicodeDecodeError:
            pass

        self.size_in_bytes = self._os_path("getsize")
        self.size = self.format_size(self.size_in_bytes)

        self.created = self._dt_manager(self._os_path("getctime"))
        self.last_modified = self._dt_manager(self._os_path("getmtime"))
        self.last_accessed = self._dt_manager(self._os_path("getatime"))

        self.full_name = self._os_path("basename")
        self.name, self.extension = self._os_path("splitext", self.full_name)

        self.dir_path = self._os_path("dirname")
        self.dir_name = self._os_path("basename", self.dir_path)

    def __repr__(self):
        return "<File {}>".format(self.full_name)

    def __str__(self):
        return self.abs_path

    def __getitem__(self, index):
        return self.lines[index]

    def __iter__(self):
        return self.data

    def __eq__(self, other):
        return self.abs_path == (other.abs_path if isinstance(other, File) else other)

    def __len__(self):
        return self.line_count

    @staticmethod
    def format_size(size_in_bytes, suffix="B"):
        for unit in ["", "K", "M", "G", "T"]:
            if abs(size_in_bytes) < 1024.0:
                return str(size_in_bytes) + unit + suffix
            size_in_bytes /= 1024.0

        return str(size_in_bytes) + "Y" + suffix

    def unzip(self, location=None):
        if location is None:
            location = self.dir_path

        with zipfile.ZipFile(self.abs_path, "r") as target:
            return target.extractall(location)

    def rename(self, new_name):
        return os.rename(self.abs_path, new_name)

    def copy(self, location=None, new_name=None):
        if location is None:
            location = self.abs_path

        return shutil.copy(self.abs_path, location if not new_name else location + new_name)

    def duplicate(self, amount=1, splitter="-"):
        duplicate_name = lambda index: self.name + splitter + str(index) + self.extension

        if amount > 1:
            for i in range(0, amount):
                self.copy(duplicate_name(i))

            return None

        return self.copy(duplicate_name(0))

    def delete(self):
        return os.remove(self.abs_path)

    def find(self, query):
        return [line for line in self.lines if query in line]

Imports

It's really uncommon to use the as part of the import. The only time I see it is in NumPy, which is a pretty un-Pythonic library.
I recommend to only use import rather than from ... import statements. This is as it's harder to determine what has been imported from where.
When importing datetime I find it best to only use import datetime. Given that one of the classes is called datetime it can lead to situations where it's unknown what datetime is without having to then look at the imports.

Class

I think you're on the right track, but you've added too much into your class. This is commonly referred to as the god-class anti-pattern.

I think it's a good idea, as pathlib implements a lot of the same stuff that you are doing. Whilst I can understand and would possibly encourage some extensions to the class, I don't think all your changes are great additions.

The following functions can all be replaced with pathlib in some form. Note that the entire of the bottom chunk can utilize Path.stat.

self.full_name = Path.name
self.name = Path.stem
self.extension = Path.suffix
self.dir_path = Path.parent
self.dir_name = Path.parent.name

Path.stat:
    datetime:
        self.created = os.path.getctime
        self.last_modified = os.path.getmtime
        self.last_accessed = os.path.getatime
    self.size_in_bytes = os.path.getsize

Datetime

I don't think this should be re-added to your class. If you feel it's of great importance then you should interface with os.stat_result. This means that using legacy and pathlib interfaces work with your changes.

Usage could be as simple as:

stats = DateTimeStat(os.stat(...))

Size Format

This shouldn't be on the class. If you want to format a number that is in bytes then define a function, not a method, to do this for you. This has the benefit of you being able to pass any number from any origin to the function and it will still happily work, DRYing up your code.

There are three common prefixes schemes when denoting binary sizes.

Metric (SI) prefixes - These are base 1000 and are the units you see pretty much everywhere in science. Much like I can have 1km, I have have 1kB. Which means 1000unit.
ISO binary prefix - There are base 1024 and are common when addressing binary data. The kibibyte, KiB, is 1024B.
JEDEC prefixes - These are discouraged by IEEE as they are confusing. It's not immediately apparent if you are using SI or JEDEC prefixes. These are base 1024 and only define the K, M, G prefixes.

I suggest you stick to SI with base 1000 and ISO with base 1024.
An example of all of them, 1Mib is 131.072kB and 128KB.

import functools

def size_format(size, base, units):
    for unit in units:
        if size < base:
            break
        size /= base
    return size, unit

size_bytes_format = functools.partial(
    size_format,
    base=1000,
    units='B kB MB GB TB YB'.split(),
)
size_ibytes_format = functools.partial(
    size_format,
    base=1024,
    units='B KiB MiB GiB TiB YiB'.split(),
)

Duplicating

I can't see this function being that useful. Even if I can't see a use, I wouldn't recommend adding it to the class. This is just a simple for loop. It's reasonable, and probably more readable, to defer this to the client code.

Result

Given all the above I would remove all the operations that act on an opened file. And it would result in the following class.

class Path(pathlib.Path):
    def unzip(self, destination=None):
        if destination is None:
            destination = self.parent
        with zipfile.ZipFile(self, 'r') as target:
            return target.extractall(destination)

    def copy(self, destination):
        return shutil.copy(self, destination)

File interactions

As stated earlier you shouldn't open the file in Path. I don't really see much benefit your class has over just using open. For example, it doesn't allow streaming and is pretty memory intense.

How is your code better than open and how is open better than your code?