Business logic inside MVC Cart Controller method I have a controller method, invoked when a user adds the product he is viewing to the cart.

The way I understand how MVC works, this can be considered a fat controller already, but I'm still not completely sure.

And if that's the case, then how can I possibly refactor this code?

public function processAddingAndUpdatingOfCartItems()
{
    // Get values from request
    $productId  = $_POST['productId'];
    $quantity   = $_POST['quantity'];

    // Instantiate DB Connection object
    $connection = new Connection();

    // Get product details of currently viewed product to be added to cart
    $productTable   = new ProductTable($connection);
    $productDetails = $productTable->getProductDetailsByProductId($productId);

    // Get price of currently viewed product: desired quantity * unit price
    $productModel = new Product();
    $productPrice = $productModel->computeProductPrice($productId, $quantity);

    // Store properties in an array
    $cartItemProperties = array(
        'productId'     => $productId
        ,'quantity'     => $quantity
        ,'unitPrice'    => $productDetails->price
        ,'price'        => $productPrice
    );

    // Create Cart Item Model
    $cartItem   = new CartItemModel();
    $cartItem->setCartItemProperties($cartItemProperties);

    // Check if a cart already exists for the current session
    $cartValidator                  = new CartValidator();
    $doesCartExistForCurrentUser    = $cartValidator->doesCartExistForCurrentUser();

    // Instantiate CartTable and CartItemTable for database queries
    $cartTable      = new CartTable($connection);
    $cartItemTable  = new CartItemTable();

    // If a cart already exists for the current session
    if($doesCartExistForCurrentUser) {
        // Get cart id in the current session
        $session        = new SessionModel();
        $cartId         = $session->getSessionKey('cartId');

        // Insert item into cart
        $cartItemTable->insertItemIntoCart($cartItem);

        // Refresh total amount of cart
        $this->refreshCartTotalAmount($cartTable, $cartId);
    } else {
        // If no cart exists yet for the current session

        // Set default user id to 0
        $sessionUserId = 0;
        $session        = new SessionModel();

        // Check if user is logged in
        $customerValidator = new CustomerValidator();
        if($customerValidator->isUserLoggedIn())
        {
            $sessionUserId  = $session->getSessionKey('userId');
        }

        // Create new cart model containing necessary properties
        $cart = new CartModel();
        $cartProperties = array(
            'customerId' => $sessionUserId
            // other properties
        );

        // Insert into carts table
        $last_cart_id = $cartTable->insertCart($cart);

        // Set cartId property of Cart Item Model
        $cartItem->cartId = $last_cart_id;

        // Insert item into cart
        $cartItemTable->insertItemIntoCart($cartItem);

        // Refresh total amount of cart
        $this->refreshCartTotalAmount($cartTable, $last_cart_id);

        // Set session cart id

        $session->setSessionKey('cartId', $last_cart_id);
    }
}

I think there is more to discuss here then business logic inside your controller.  I'm going to generally keep it to a high level of discussion, but feel free to comment and ask for specifics.  I'm going to break it down like this:

Security issues
Architecture issues
Business logic issues
Formatting issues

Here we go!

Security

These two lines right here are actually quite dangerous:

$productId  = $_POST['productId'];
$quantity   = $_POST['quantity'];

It doesn't look like you are doing any validation/filtering/cleaning of user input.  Both of these variables are copied straight out of the user input and then sent off to various methods (getProductDetailsByProductId, computeProductPrice) that have no expectation that they are now dealing with untrusted user input.  As a result, the possibility of security vulnerabilities is high.  At the very least, you should:

$productId  = (int) $_POST['productId'];
$quantity   = (int) $_POST['quantity'];

Understanding that user input is untrustworthy, and treating it as such, is a key part of application security.  I would very carefully check the rest of your code and make sure that you are vetting all user input everywhere.  Are you familiar with the dangers of XSS vulnerabilities and SQL injection?  If you're not, now is the time to start learning.

I similarly don't see anything related to CSRF protection, which is a necessity for endpoints such as this.  Some frameworks would automatically handle CSRF protection in middleware, but given the overall details of what I see here, I'm suspicious that CSRF protection is simply not happening.

Architecture

The "fatness" of your controller has to do with more than just business logic.  You are also instantiating your dependencies directly inside your controller.  This makes automated testing impossible, but also opens you up to any number of subtle bugs.  Dependency injection is the best way to fix these issues, but if that is too much of an architectural change for you, you could always go with the old school fallback: singletons.  It's far from ideal, but it is easier to implement than dependency injection, and will at least make it so that you can't accidentally open up multiple connections to the database in response to a single HTTP request.

To be clear, these are the lines I'm talking about:

$connection = new Connection();
$session    = new SessionModel();

Although to be fair, all of your object instantiation is a symptom of the problem.  What it really comes down to is a lack of proper separation of concerns.  Your controller is in charge of everything: not just business logic, but also application logic.  This makes it substantially harder to manage your application in the long-run.

Business Logic

Getting to your question, I definitely think that you have too much business logic in your controller.  The issue is that you are looking at this from the perspective of "I've got this one place where I add items to the cart".  It never works that way.  An e-commerce system naturally starts that way, but you will find your needs will quickly grow over time.  How do you handle the situation when someone says: "Hey, can I look at my order history and click a button to add an old order back into my cart?".  That is a super common feature for an e-commerce platform.  Unfortunately, adding an item to a user's cart currently involves multiple dependencies and 50ish lines of code.  If you ever need to add to a cart, in any other way, and at any other endpoint, you will very quickly realize the pain caused by fat controllers like this.

So what should it look like?  This is what your controller method should look like:

public function processAddingAndUpdatingOfCartItems(cart $cart)
{
    // Get values from request
    $productId  = (int) $_POST['productId'];
    $quantity   = (int) $_POST['quantity'];

    if ($quantity <= 0)
        return 'Invalid quantity';

    if (!$cart->is_valid_product($productId))
        return 'Invalid product';

    if (!$cart->add_product($productId, $quantity))
        return 'There was an error processing your request';

    return redirect('/view_cart');
}

All of this business logic should be handled by the cart.  You should not instantiate the database connection: in fact, your controller shouldn't even need it.  You preferably shouldn't even create the cart: it should be injected in.  Worst-case scenario though, the cart is the only thing you should have to create.

The cart should handle validation (which it can defer to a cartValidator, if desired).  The cart should defer storage of itself to the session object, and the cart shouldn't care about whether the session exists or not.  That is the session objects job.  The session object should simply be told "store this", and the session object should automatically create the actual session, if need be.  Ideally this would all happen via dependency injection.

These are just a couple points, but it all boils down to the same thing: to get anything done, your controller has to know about everything.  It would help to read up on the single responsibility principle.  The more things your code has to "know" about to get its job done, the more room there is for things to break in the future, and the harder it is to update your application.

Keep in mind that the goal here isn't just to shuffle everything into a cart class.  Rather, the cart class itself defers most of its responsibility to its own child classes: cartValidator, cartProduct, session, etc...  A lot of the stuff you have right now needs to be better divided into some of these classes, so that each class does only the things that are directly related to its own functionality, and defers the rest to someone else.  However, you should have one cart object that acts as a single entry point for all cart-related functionality: adding products, verifying product information, etc...

Formatting

This one isn't too bad, but the way you line up your variables is a little wonky.  Many code bases avoid lining things up all together, but if you would rather do that in your code base, obviously that's perfectly reasonable.  I think though that if you are going to do this you should find some consistent stop-points for lining stuff up at.  If you put some of your lines of code right next to eachother, I think you'll see the problem:

$productTable   = new ProductTable($connection);
$productDetails = $productTable->getProductDetailsByProductId($productId);
$productModel = new Product();
$productPrice = $productModel->computeProductPrice($productId, $quantity);
$cartItem   = new CartItemModel();
$cartValidator                  = new CartValidator();
$doesCartExistForCurrentUser    = $cartValidator->doesCartExistForCurrentUser();
$cartTable      = new CartTable($connection);
$cartItemTable  = new CartItemTable();

The inconsistency can be distracting, and the last thing you want while looking at code is to be distracted.  Code gets read much more than it is written, so it is best to write it consistently.

Again, I'm happy to delve into more details on any particular item, if you are interested.

Dependency Injection

Dependency injection is not a small thing, and won't be a small change for your system architecture.  That being said, dependency injection will be an important change for your architecture, and, if done right, will substantially improve your organization and long-term maintainability.  As Sam mentioned, learning about SOLID is a good place to start.  The idea though is that you don't directly instantiate your dependencies.  Rather, they are given to you.  This is typically done via type-hinting in your constructor.  In many ways, your other classes are already doing this.  The trick therefore is to implement a dependency injection container.  Again, this isn't necessarily a small thing, so do some reading first, find an open-source one instead of building your own, and start small.  It is absolutely worth the effort.

In practice it works because your dependency injection container builds all of your objects for you, and uses reflection to determine the dependency each object needs to do its job.  So your controller would look like this:

class cart_controller{
    public function __construct(cart $cart, request $request){
        $this->cart = $cart;
        $this->request = $request; // stores details of the HTTP request
    }
}

Your cart then might look like this:

class cart{
    public function __construct(session $session, cartItemFactory $cartItemFactory, cartValidator $cartValidator){
        // store dependencies
    }
}

Your application bootstrapping process builds the controller with the dependency injection container, the dependency injection container sees that the controller needs the cart, it sees the cart needs the session, itemFactory, and validator, and then it will find any necessary dependencies of those things as well.  It starts at the bottom of the dependency tree, building the things that have no dependencies, passing those into the next objects and building them, and all the way up until you have a controller that gets a cart object when it is instantiated.

It can be complicated, which is why it is best to just use a pre-built DI container.  It is also a completely different way of building your application, which is why it is best to spend some time learning and start small.  This isn't the sort of thing you just "do" over a weekend.  The biggest advantage though is that it allows you to actually test your code.  Having maintained large applications both with and without automated testing, I can tell you that in the long run having some automated tests (unit tests, integration tests, etc...) in place is a huge boon to your ability to maintain your code-base, and introduce new features quickly.  That alone is absolutely worth the time it will take you to implement these large changes.

Then again, some of your classes are already receiving some of their dependencies through their constructor arguments, so you're also kind of halfway there.

Responding to your questions

Regarding whether or not to have the DB connection passed in to the controller constructor: As a general rule of thumb, if you have a controller with methods that have completely different dependencies, then they should probably be in different controllers.  It all comes down to the Single Responsibility Principle and proper separation-of-concerns, which you will read plenty about if you start learning about SOLID.  As another option though, it is common in some PHP MVC frameworks to do method-level dependency injection, where each controller-method can have additional dependencies injected in.  I'm not really a fan of it myself (partly for the above reasons), but it is a common pattern, so you wouldn't be crazy to do it.

Regarding my comment about how all your object instantiation is a symptom of the problem, what I mean is that directly creating objects violates the D in SOLID, which Sam mentioned in his answer.  When you directly create your dependencies, rather than allowing them to be given to you, you tightly couple a class to its sub-classes, which makes your application more rigid, harder to maintain, and impossible to test.  Again, this are large architectural changes, so it is probably best to start by reading up on these topics, and practicing them with smaller applications before doing anything crazy with your code base.


Creating an adjacency list I've created this adjacency list, but I don't like the design of the graph. The issue is that there is one pointer in the node to its adjacency list and there is a pointer in an adj_list node to the actual node. I am also using next pointers to traverse the graph in the order in which nodes were added and the list to get the adjacency list of a node.

Any suggestions?

#include <queue>

enum color{
    WHITE,
    GREY,
    BLACK
};

struct adj_list;

struct node{
    int id;
    int weight;
    color visited;
    adj_list* adj;
    node(int n){
        id = n;
        weight = _I32_MAX;
        visited = WHITE;
    }

};

    struct adj_list{
        adj_list* next;  //This next pointer for visiting each node one after another in the order in which the nodes were added to the graph
        adj_list* list;
        node* n;
        adj_list(node& _n){
            n = &(_n);
            next = NULL;
            list = NULL;
        }
    };

node* add_node(int id,std::queue<int> q , node* root)
{
    node* n = new node(id);
    adj_list* adj = new adj_list(*n);
    n->adj = adj;

    if(root == NULL){
      return n;
    }
    std::queue<adj_list*> q1;

    while(1){
        adj_list* iter = root->adj;
        if(q.empty())break;
        int k = q.front();
        q.pop();
        while(iter){
            if(iter->n->id == k){
                q1.push(iter);
                adj_list*  temp = iter->list;
                iter->list = new adj_list(*n);
                iter->list->list = temp;
                break;
            }
            iter = iter->next;
        }
    }

    adj_list* iter = root->adj;
    while(iter->next){
        iter = iter->next;
    }

    iter->next = adj;

    while(!q1.empty()){
        adj_list* temp = q1.front();
        q1.pop();
        adj_list* new_adj = new adj_list(* temp->n);
        adj->list = new_adj;
        adj = new_adj;
    }
    return root;
}

Adding a new node to the graph:

std::queue<int> q;
q.push(4);
q.push(9);

add_node(20,q , &root);

Update:

I have made some changes here to make it more C++, but an issue here I don't like is that driver has to know about the other vertexes to add to their adjacency list. I'm not sure how to get rid of that if we have to create the graph incrementally.

Another is to provide the graph with a matrix, but that just beats the idea behind adjacency matrix.

#include "iostream"
#include "vector"
#include "list"

struct edge{

int destination_vertex;

    edge(int ver){
        destination_vertex = ver;
    }
    };

struct vertex{
    int id;
    std::list<edge>  list;
    vertex(int _id){
        id = _id;
    }
};

class graph
{
private:
    std::vector<vertex*> vertexes;
    int next;

public:
    graph(void){
        next = 0;
    }

    ~graph(void){}

    void add_node(std::vector<int> edges ){

        vertex* v = new vertex(next++);
        vertexes.push_back(v);

        for(unsigned int i=0;i<edges.size();i++){

            vertex* existing_vertex = vertexes[edges[i]];
            existing_vertex->list.push_back(*(new edge (v->id)));

            edge* e = new edge(edges[i]);
            v->list.push_back(*e);
        }
    }

    void print(){
        for(unsigned int i = 0 ; i < vertexes.size();i++){
            vertex* v =  vertexes[i];
            std::cout << v->id <<"->";
            for(std::list<edge>::iterator iter = v->list.begin() ; iter != (v->list).end();iter++)
            {
                std::cout << iter->destination_vertex <<",";
            }
            std::cout << std::endl;
        }
    }
};

e.g. of adding a new node to the graph

Well for starters I don't see the graph:

I would expect the code to be:

 Graph    graph;
 graph.add_node(20,q); // Add nodes to graph.

Stop passing pointers around.

node* add_node(int id,std::queue<int> q , node* root)

Pointers have no associated ownership semantics. This is normal in C. But if you are writing C++ you should never do this. Ownership symantis defines who owns and thus who is responsible for calling delete on dynamically allocated objects. If it is an automatic variable you should probably be passing a reference (or in the case more likely making add_node() a member of the graph object.

Also you manipulate node and adj_list directly from add_node. Again fine from a C perspective but not from a C++ one. You should ask the object to manipulate itself by asking it to perform some operation (ie method). Since your code is so intertwined without methods it makes it real hard to read and understand. So that's about all I have.

Update:

Comment on update section

For every call to new there MUST be a call to delete. Without this the code will leak memory. In your case here we don't actually need any calls to new so we can remove all the leaks by just using normal automatic variables.

// might be a bit overkill
// For just holding a destination vertex.
struct edge
{
    // This should really be private
    // Once created you don't really want it changing.
    // So make it hard to change accidentally by making it private.
    // Alternatively you can make it const (but this makes copying hard
    // so I would make it private and private an accessor)
    private: int destination_vertex;
    public:  int getDest() const {return destination_vertex;}

    // Prefer to use initializer lists.
    edge(int ver){
        destination_vertex = ver;
    }

    // i.e. Prefer to write like this
    edge(int ver)
        : destination_vertex(ver)
    {}

    // A friend for printing.
    friend std::ostream& operator<<(std::ostream& s, edge const& e)
    {
        return s << e.destination_vertex;
    }
};

struct vertex
{
    int id;                 // This should probably be const
                            // as it never changes.
                            // But because this makes copying hard
                            // prefer to make it private and provide
                            // an accessor.
    std::list<edge>  list;

    // Prefer initializer lists.
    // Also prefer not to put '_' on the front of your identifers.
    // The rules are non trivial and most people don't know the actual
    // rules so its is easy to get wrong (in this case you got it correct)
    //
    vertex(int id)
        : id(id)    // The compiler can easily distinguish between these too.
    {}

    friend std::ostream& operator<<(std::ostream& s, vertex const& v)
    {
          s << v.id << "->";
          std::copy(v.list.begin(), v.list.end(),
                    std::ostream_iterator<edge>(s, ","));
          return s;
    }
};

Lets fix some memory leaks.

class graph
{
    private:
        // No need for this to be vertex*
        std::vector<vertex*> vertexes;

        // If you just make it a vector of `vetrtex` then everything works
        // as expected and you don't need to call new anywhere.
        std::vector<vertex>   vertexes;
        int                   next;

    public:
        // Prefer initializer list
        // Don't put void as the parameter list.
        graph(void)
            : next(0)
        {}

        // You should have written this so it cleaned up all the
        // allocated memory. If your destructor does nothing then
        // don't write one, let the compiler generate one do the work
        //
        // Since we are not going to have any dynamic allocation just
        // delete it now.
        //
        // ~graph(void){}

        // Comment code that is non trivial it took me a while to
        // figure this out!
        //
        // Add a new node node 'n'
        // The vector of edges defines what node(s) 'n' connects too.
        //      add an edge from 'n' to each node in `edges'
        //      add an edge from each node in `edges' to 'n'
        //
        // Note It is assumed that `edges` does not contain any nodes
        //      larger than (n-1)

        // When passing non trivial objects pass by reference
        // If your code is not supposed to change the code make it const
        // as well. Thus the compiler warns you if you try and modify it
        // accidentally.
        void add_node(std::vector<int> const  & edges)
                                   /// ^^^^^  ^
        {
            // no need for dynamic allocation.
            // vector is already doing most of the work
            // vertex* v = new vertex(next++);
            // vertexes.push_back(v);

            // Rather just add a new element to vector
            // Then grab a reference to it for convenience.
            vertexes.push_back(vertex());
            vertex&  v = vertexes.back();  // a reference to the element you
                                           // just added.

            for(unsigned int i=0;i<edges.size();i++)
            {
                // Added comment to the beginning about this
                // assumption.
                vertex& existing_vertex = vertexes[edges[i]];

                // This:    *(new edge (v->id))
                // Dynamically allocates an edge then de-references
                // so it can be copied into a list. You lost all
                // information about the pointer you created with new
                // so the memory is leaked and lost.
                // There is no need to create dynamically allocated object
                // just use the int automatic value.

                existing_vertex.list.push_back(v.id);

                // This has the same problem as the last line.
                // apart from you are doing it in two steps so keep
                // the pointer (which you should have deleted after use)
                // edge* e = new edge(edges[i]);
                // v->list.push_back(*e);

                // This can be replaced with
                v.push_back(edges[i]);

               // In fact I would get rid of all the above lines and
               // replace with code that looks symmetrical so that you
               // can see that you are adding edges in both directions.

               vertexes[edges[i]].list.push_back(v.id);
               vertexes[v.id].list.push_back(edges[i]);

            }
        }

        // Methods that do not change the state of the object should
        // be marked const.

        void print()  const
        {       ///   ^^^^^

            // Sure you can iterate by index.
            // But you could just as easily have used an iterator.
            // Also prefer to use ++i rather than i++
            // It makes no difference for integers but if you change
            // changed the way you iterate then you don't need to think
            // am I using the correct increment style.
            for(unsigned int i = 0 ; i < vertexes.size();i++){

                // Get a reference for ease of use
                vertex& v =  vertexes[i];
                std::cout << v.id << "->";

                // Nice use of iterator.
                // But prefer const_iterator if you are not going
                // to modify the container.

                for(std::list<edge>::const_ iterator iter = v.list.begin() ; iter != v.list.end();iter++)
                {
                    std::cout << iter->destination_vertex <<",";
                }

                // Prefer '\n' over std::endl
                // The difference is that std::endl() outputs a '\n'
                // then calls flush on the stream. This makes it very
                // efficient (as the whole pointer of the buffer is
                // so we only flush to the physical stream rarely).
                std::cout << '\n';
            }
        }

        // Since we have defined output operators for edge and vertex
        // We can do the same for graph:
        friend std::ostream& operator<<(std::ostream& s, graph const& g)
        {
             std::copy(v.vertexes.begin(), v.vertexes.end(),
                       std::ostream_iterator<vertex>(s, "\n"));
             return s;
        }
        // Now you can print a graph like this:
        //    std::cout << g;

};

After cleaning up all the comments.
It looks like this:

Headers

#include <iostream>
#include <list>
#include <vector>
#include <iterator>

Edge

class edge
{
    int destination_vertex;
    public:
        int getDest() const {return destination_vertex;}

        edge(int ver)
            : destination_vertex(ver)
        {}

    friend std::ostream& operator<<(std::ostream& s, edge const& e)
    {
        return s << e.destination_vertex;
    }
};

Vertex

class graph;
class vertex
{
    friend class graph;
    int id;
    std::list<edge>  list;

    public:
        vertex(int id)
            : id(id)
        {}

    friend std::ostream& operator<<(std::ostream& s, vertex const& v)
    {
          s << v.id << "->";
          std::copy(v.list.begin(), v.list.end(),
                    std::ostream_iterator<edge>(s, ","));
          return s;
    }
};

Graph

class graph
{
    private:
        std::vector<vertex>   vertexes;
        int                   next;

    public:
        graph()
            : next(0)
        {}

        //
        // Add a new node node 'n'
        // The vector of edges defines what node(s) 'n' connects too.
        //      add an edge from 'n' to each node in `edges'
        //      add an edge from each node in `edges' to 'n'
        //
        // Note It is assumed that `edges` does not contain any nodes
        //      larger than (n-1)
        void add_node(std::vector<int> const  & edges)
        {
            vertexes.push_back(vertex(next));

            for(unsigned int i=0;i<edges.size();++i)
            {
               vertexes[edges[i]].list.push_back(next);
               vertexes[next].list.push_back(edges[i]);
            }
            ++next;
        }

        friend std::ostream& operator<<(std::ostream& s, graph const& g)
        {
             std::copy(g.vertexes.begin(), g.vertexes.end(),
                       std::ostream_iterator<vertex>(s, "\n"));
             return s;
        }
};

Main

int main()
{
    graph   g;

    std::vector<int>    v;
    g.add_node(v);

    v.push_back(0);
    g.add_node(v);

    std::cout << g << "\n";
}


Web scraping and design pattern lifting I would like to get some feed back on my code; the goal is to get all agencies address of banks. I wrote a pretty simple brute force algorithm.
I was wondering if you would have any advice to improve the code, design it differently (do I need an OOP approach here) etc .
import requests
from lxml import html

groupe = "credit-agricole"
dep = '21'
groupes =["credit-agricole"]
deps = ['53', '44', '56', '35', '22', '49', '72', '29', '85']

def get_nb_pages(groupe,dep):
    """ Return nb_pages ([int]): number of pages containing banks information .
    Args:
        groupe ([string]): bank groupe ("credit-agricole",...)
        dep ([string]): departement ("01",...)
    """
    url ="https://www.moneyvox.fr/pratique/agences/{groupe}/{dep}".format(groupe=groupe,dep=dep)
    req = requests.get(url)
    raw_html = req.text
    xpath = "/html/body/div[2]/article/div/div/div[3]/div[2]/nav/a"
    tree = html.fromstring(raw_html)
    nb_pages = len(tree.xpath(xpath)) +1

    return nb_pages

def get_agencies(groupe,dep,page_num):
    """ Return agencies ([List]): description of agencies scrapped on website target page.
    Args:
        groupe ([string]): bank groupe ("credit-agricole",...)
        dep ([string]): departement ("01",...)
        page_num ([int]): target page
    """
    url ="https://www.moneyvox.fr/pratique/agences/{groupe}/{dep}/{page_num}".format(groupe=groupe,dep=dep,page_num=page_num)
    req = requests.get(url)
    raw_html = req.text
    xpath = '//div[@class="lh-bloc-agence like-text"]'
    tree = html.fromstring(raw_html)
    blocs_agencies = tree.xpath(xpath)
    agencies = []
    for bloc in  blocs_agencies:
        agence = bloc.xpath("div/div[1]/h4")[0].text
        rue = bloc.xpath("div/div[1]/p[1]")[0].text
        code_postale = bloc.xpath("div/div[1]/p[2]")[0].text
        agencies.append((agence,rue,code_postale))
    return agencies

def get_all(groupes,deps):
    """Return all_agencies ([List]): description of agencies scrapped.
    Args:
        groupes ([List]): target groups
        deps ([List]): target departments
    """
    all_agencies = []
    for groupe in groupes:
        for dep in deps:
            nb_pages = get_nb_pages(groupe,dep)
            for p in range(1,nb_pages+1):
                agencies = get_agencies(groupe,dep,p)
                all_agencies.extend(agencies)
    df_agencies = pd.DataFrame(all_agencies,columns=['agence','rue','code_postale'])
    return df_agencies

get_nb_pages(groupe,dep)
get_agencies(groupe,dep,1)
df_agencies = get_all(groupes,deps)

```

It's fine for your strings - and scraped web content - to be localised in French; but ensure that your variables are in English (groupe -> group) for consistency
Prefer tuples over lists when you have immutable data
Add PEP484 type hints when possible
Do not leave those first four variables in global scope; move them to a function
Consider using f-strings instead of format calls
Always check to see if your requests calls fail; the easiest way is via raise_for_status
Tell requests when you're done with a response via context management
Use actual integers for your department numbers instead of stringly-typed data
Consider using an intermediate dataclass for your agency data instead of implicit tuples
Consider using generator functions (yield) to simplify your iterative code

First Suggested
from dataclasses import dataclass, astuple
from typing import Iterable, Collection

import pandas as pd
import requests
from lxml import html
from lxml.html import HtmlElement

@dataclass
class Agency:
    name: str
    street: str
    postal_code: str

    @classmethod
    def from_block(cls, block: HtmlElement) -> 'Agency':
        return cls(
            name=block.xpath("div/div[1]/h4")[0].text,
            street=block.xpath("div/div[1]/p[1]")[0].text,
            postal_code=block.xpath("div/div[1]/p[2]")[0].text,
        )

def get_nb_pages(group: str, department: int) -> int:
    """ Return nb_pages ([int]): number of pages containing banks information .
    Args:
        groupe ([string]): bank groupe ("credit-agricole",...)
        department ([string]): departement ("01",...)
    """
    url = f"https://www.moneyvox.fr/pratique/agences/{group}/{department}"
    with requests.get(url) as req:
        req.raise_for_status()
        raw_html = req.text

    xpath = "/html/body/div[2]/article/div/div/div[3]/div[2]/nav/a"
    tree = html.fromstring(raw_html)
    return len(tree.xpath(xpath)) + 1

def get_agencies(group: str, department: int, page_num: int) -> Iterable[Agency]:
    """ Return agencies ([List]): description of agencies scrapped on website target page.
    Args:
        groupe ([string]): bank groupe ("credit-agricole",...)
        department ([string]): departement ("01",...)
        page_num ([int]): target page
    """
    url = f"https://www.moneyvox.fr/pratique/agences/{group}/{department}/{page_num}"
    with requests.get(url) as req:
        req.raise_for_status()
        raw_html = req.text

    xpath = '//div[@class="lh-bloc-agence like-text"]'
    tree = html.fromstring(raw_html)
    for block in tree.xpath(xpath):
        yield Agency.from_block(block)

def get_all(groups: Iterable[str], departments: Collection[int]):
    """Return all_agencies ([List]): description of agencies scrapped.
    Args:
        groupes ([List]): target groups
        departments ([List]): target departments
    """
    for group in groups:
        for department in departments:
            nb_pages = get_nb_pages(group, department)
            for page in range(1, nb_pages + 1):
                yield from get_agencies(group, department, page)

def main():
    group = "credit-agricole"
    department = 21
    groups = ("credit-agricole",)
    departments = (53, 44,)  # ...  56, 35, 22, 49, 72, 29, 85)

    n_pages = get_nb_pages(group, department)
    agencies = tuple(get_agencies(group, department, page_num=1))

    all_agencies = get_all(groups, departments)
    df_agencies = pd.DataFrame(
        (astuple(agency) for agency in all_agencies),
        columns=('agence', 'rue', 'code_postale'),
    )

if __name__ == '__main__':
    main()

All of that being the case, your approach using xpath selectors is very fragile. Here is an alternate approach that uses named elements with classes and IDs where available. It is incomplete because I think the site rate-limited my IP, which is of course a direct risk of scraping and totally within the rights of the website.
BeautifulSoup Alternate
import re
from dataclasses import dataclass, astuple
from typing import Iterable, Dict, ClassVar, Pattern
from bs4 import BeautifulSoup, Tag

import pandas as pd
from requests import Session

ROOT = 'https://www.moneyvox.fr'

@dataclass
class Branch:
    name: str
    street: str
    city: str
    postal_code: str
    path: str

    @classmethod
    def scrape_all(cls, session: Session, path: str) -> Iterable['Branch']:
        page = ''
        while True:
            with session.get(ROOT + path + page) as response:
                response.raise_for_status()
                doc = BeautifulSoup(response.text, 'xml')

            body = doc.select_one('div.main-body')
            city = None

            for head_or_cell in body.select('h2, div.lh-bloc-agence'):
                if head_or_cell.name == 'h2':
                    city = head_or_cell.text
                elif head_or_cell.name == 'div':
                    street, postal_code = head_or_cell.select('p')
                    yield cls(
                        name=head_or_cell.h4.text,
                        street=street.text,
                        city=city,
                        postal_code=postal_code.text,
                        path=head_or_cell.select_one('a.lh-btn-info')['href'],
                    )

            # perform depagination here

@dataclass
class Department:
    name: str
    code: str
    path: str
    n_branches: int

    re_count: ClassVar[Pattern] = re.compile(r'\d+')

    @classmethod
    def from_li(cls, li: Tag) -> 'Department':
        return cls(
            name=li.strong.text,
            path=li.a['href'],
            code=cls.re_count.search(li.a.text)[0],
            n_branches=int(cls.re_count.search(li.em.text)[0]),
        )

@dataclass
class Agency:
    name: str
    category: str
    path: str

    @classmethod
    def scrape_all(cls, session: Session) -> Iterable['Agency']:
        with session.get(ROOT + '/pratique/agences') as response:
            response.raise_for_status()
            doc = BeautifulSoup(response.text, 'xml')

        body = doc.select_one('div.main-body')
        category = None
        for head_or_cell in body.select('h2, a.lh-lien-bloc-liste'):
            if head_or_cell.name == 'h2':
                category = head_or_cell.text
            elif head_or_cell.name == 'a':
                yield cls(
                    name=head_or_cell.text,
                    category=category,
                    path=head_or_cell['href'],
                )

    def get_departments(self, session: Session) -> Dict[str, int]:
        with session.get(ROOT + self.path) as response:
            response.raise_for_status()
            doc = BeautifulSoup(response.text, 'xml')

        for li in doc.select('#tabs-departement li'):
            yield Department.from_li(li)

    def __str__(self):
        return self.name

def main():
    pd.set_option('display.max_columns', None)
    pd.set_option('display.width', None)

    with Session() as session:
        agencies = {a.name: a for a in Agency.scrape_all(session)}
        agency_df = pd.DataFrame(
            (astuple(a) for a in agencies.values()),
            columns=('Nom', 'Catégorie', 'Lien'),
        )
        print(agency_df)

        agency = agencies['Crédit Agricole']
        departments = {d.name: d for d in agency.get_departments(session)}

        department = departments['Ardennes']
        branches = {b.name: b for b in Branch.scrape_all(session, department.path)}

if __name__ == '__main__':
    main()


Subdividing a 4×4 array into four 2×2 arrays I want to subdivide a 4×4 2d array into 4 2×2 arrays and I have figured out how to do this in a very crude way, but am having a tougher time with a more elegant general solution.

    for (int i = 0; i < 4; i++)
    {
        for (int j = 0; j < 2; j++)
        {
            for (int k = 0; k < 2; k++)
            {
                int j2 = j;
                int k2 = k;
                if (i == 1) { k2 += 2; }
                if (i == 2) { j2 += 2; }
                if (i == 3) { j2 += 2; k2 += 2; }
                twoByTwo[i][j, k] = fourByFour[j2, k2];
            }
        }
    }

Flatten and copy array values

You can flatten your fourByFour array and then populate the twoByTwo array:

private static object convertArray1()
{
    int[,] fourByFour = new int[,]{
                        {1,2,3,4 },
                        {12,13,14,15 },
                        {21,22,23,24 },
                        {31,32,33,34 } };

    int[] arrayElems = new int[fourByFour.GetLength(0) * fourByFour.GetLength(1)];

    Buffer.BlockCopy(fourByFour, 0, arrayElems, 0, arrayElems.Length * sizeof(int));

    int[][,] twoByTwo = new int[4][,];

    int elemIndex = 0;
    for (int i = 0; i < twoByTwo.GetLength(0); i++)
    {
        twoByTwo[i] = new int[2, 2];

        for (int j = 0; j < twoByTwo[i].GetLength(0); j++)
        {
            for (int k = 0; k < twoByTwo[i].GetLength(1); k++)
            {
                twoByTwo[i][j, k] = arrayElems[elemIndex];
                elemIndex++;
            }
        }
    }

    return twoByTwo;
}

This solution has a performance penalty - Buffer.BlockCopy copies the source array fourByFour an extra time (which will also require additional memory).

Dimension counter class

Use a helper class to keep track of and increment index positions in each dimension.

This solution is more generic.

private static object convertArray2()
{
    int[,] fourByFour = new int[,]{
                        {1,2,3,4 },
                        {12,13,14,15 },
                        {21,22,23,24 },
                        {31,32,33,34 } };

    DimensionCounter dim1 = new DimensionCounter(4, 4);
    DimensionCounter dim2 = new DimensionCounter(4, 2, 2);

    int[][,] twoByTwo = new int[4][,];

    for (int i = 0; i < dim1.Capacity; i++)
    {
        int val = (int)fourByFour.GetValue(dim1.Current);

        if (twoByTwo[dim2.Current[0]] == null)
        {
            twoByTwo[dim2.Current[0]] = new int[dim2.DimensionSizes[1], dim2.DimensionSizes[2]];
        }

        twoByTwo[dim2.Current[0]][dim2.Current[1], dim2.Current[2]] = val;

        if (i + 1 < dim1.Capacity)
        {
            dim1.Next();
            dim2.Next();
        }
    }

    return twoByTwo;
}

private class DimensionCounter
{
    private readonly int[] dimSizes;
    private readonly int capacity;

    private int[] dimIndexes;
    private int position;

    public int[] DimensionSizes { get { return dimSizes; } }
    public int[] Current
    {
        get { return dimIndexes; }
    }
    public int Position { get { return position; } }
    public int Capacity { get { return capacity; } }
    public DimensionCounter(params int[] dimSizes)
    {
        this.dimSizes = dimSizes;
        this.capacity = dimSizes.Aggregate(1, (a, b) => a * b);
        this.dimIndexes = new int[this.dimSizes.Length];
    }

    public int[] Next()
    {
        for (int i = dimIndexes.Length - 1; i >= 0; i--)
        {
            if (dimIndexes[i] < dimSizes[i] - 1)
            {
                dimIndexes[i]++;
                break;
            }
            else
            {
                for (int j = i; j < dimIndexes.Length; j++)
                {
                    dimIndexes[j] = 0;
                }

                if (i == 0)
                {
                    throw new IndexOutOfRangeException("Cannot increment indexes any more!");
                }
            }
        }
        position++;
        return dimIndexes;
    }

    public void Reset()
    {
        dimIndexes = new int[dimIndexes.Length];
        position = 0;
    }
}


Winsock recv all bytes I had some issues with sending huge data and receiving them with Winsock so I made some improvements on my RecvAllBytes function and the same with SendAllBytes. I would like you to review it because I made a lot of conditions and loops.  Please let me know how to improve the code below.

constexpr int maxBufferSize = 8192;

bool Server::RecvAllBytes(std::shared_ptr<Connection> connection, char * data, int totalbytes)
{
    int totalbytesreceived = 0;
    if (totalbytes > maxBufferSize)// if buffer is bigger then maximum allowed
    {
        while (totalbytesreceived < totalbytes)//while full buffer not received
        {
            int bytesrecv = 0;
            int bytesleft = totalbytes - totalbytesreceived;
            if (bytesleft >= maxBufferSize)// if there is still more left then maximum size keep recv full size
            {
                while (bytesrecv < maxBufferSize)
                {
                    int ReturnCheck = recv(connection->socket, data + totalbytesreceived, maxBufferSize - bytesrecv, NULL);
                    if (ReturnCheck == SOCKET_ERROR)
                    {
                        return false;
                    }
                    bytesrecv += ReturnCheck;
                    totalbytesreceived += ReturnCheck;
                }
            }
            else// if left less then maximum size recv last bytes left
            {
                while (bytesrecv < bytesleft)
                {
                    int ReturnCheck = recv(connection->socket, data + totalbytesreceived, bytesleft - bytesrecv, NULL);
                    if (ReturnCheck == SOCKET_ERROR)
                    {
                        return false;
                    }
                    bytesrecv += ReturnCheck;
                    totalbytesreceived += ReturnCheck;
                }
            }
        }
    }
    else // if buffer is not bigger then maximum allowed
    {
        while (totalbytesreceived < totalbytes) {
            int ReturnCheck = recv(connection->socket, data + totalbytesreceived, totalbytes - totalbytesreceived, NULL);
            if (ReturnCheck == SOCKET_ERROR) {
                return false;
            }
            totalbytesreceived += ReturnCheck;
        }
    }
    return true;
}

Regarding improving the code, here are some suggestions:

Reduce the number of conditions/loops. The whole method is doable in a single while loop. See below for an example.
Prefer nouns for names of variables and verbs for functions/methods. For instance, instead of calling the return value ReturnCheck, call it ReturnValue.
Refactor the code to handle the connection getting closed before getting all of the expected bytes. According to a Microsoft Winsock web page for the recv call, "If the connection has been gracefully closed, the return value is zero". At which point I believe your code will try again and get SOCKET_ERROR (with an error code of WSAENOTCONN). And then return false even though it may have missed only say the last byte expected.
Recognize values that are constant with the C++ const type qualifier (or constexpr as you've done for compile-time constants).
Seeing as how you used constexpr (available since C++11), go ahead and also prefer using auto.
Consider making the RecvAllBytes method a method of a class that the client can use too (I don't demonstrate this though).

Here's how this could look (I didn't compile and test this code but hopefully it works):

int Server::RecvAllBytes(std::shared_ptr<Connection> connection, char * data, int bytesExpected)
{
    WSASetLastError(0);

    auto bytesReceived = decltype(bytesExpected){0};
    while (bytesExpected > 0)
    {
        const auto bytesRequested = (bytesExpected > maxBufferSize)? maxBufferSize: bytesExpected;
        const auto returnValue = recv(connection->socket, data + bytesReceived, bytesRequested);
        if (returnValue == -1 || returnValue == 0)
        {
            return bytesReceived;
        }
        bytesReceived += returnValue;
        bytesExpected -= returnValue;
    }
    return bytesReceived;
}

And how it could be used:

const auto bytesReceived = server.RecvAllBytes(connection, bigBuffer, bigSize);
if (bytesReceived < bigSize)
{
    if (WSAGetLastError() != 0)
    {
        // handle error.
    }
    else
    {
        // handle connection closed before all bytes received.
    }
}


Splitting the coordinates of the world into equal blocks It's a function to split the coordinates to some small blocks. For example, some country has a coordinates of north-east and south-west, two diagonals.

If I set the interval to 0.05, it will returns all the coordinates with an interval of 0.05 (or some very close value).

Note: you could regard the blocks as square, and the length of it's edge is 'interval'

For these function, how can simplify it and make it more readable?

Sample results:

[{'ne_lng': -53.0, 'sw_lat': 71.97411764705882, 'interval': 0.05, 'sw_lng': -53.04941176470588, 'ne_lat': 72.0}, {'ne_lng': -53.0, 'sw_lat': 71.94823529411764, 'interval': 0.05, 'sw_lng': -53.04941176470588, 'ne_lat': 71.97411764705882}]

Country = {
    'ne_lat' : 89.000000,
    'ne_lng' : 179.000000,
    'sw_lat' : -89.000000,
    'sw_lng' : -179.000000
}

def getCoordinate(Country, interval):
    ne_lng = Country['ne_lng']
    ne_lat = Country['ne_lat']
    sw_lng = Country['sw_lng']
    sw_lat = Country['sw_lat']
    interval = float(interval)
    i = 0
    temp = ne_lng
    lngList = []
    n = int((ne_lng - sw_lng)/interval)+1
    for _ in range(n):
        i += 1
        lngList.append(temp)
        temp -= (ne_lng - sw_lng) / n
    i = 0
    temp = ne_lat
    latList = []
    n = int((ne_lng - sw_lng)/interval)+1
    for _ in range(n):
        i += 1
        latList.append(temp)
        temp -= (ne_lat - sw_lat) / n
    coordinate = []
    for ki, vi in enumerate(lngList):
        for kj, vj in enumerate(latList):
            if ki == n-1 or kj == n-1:
                pass
            else:
                coordinate.append([vi, vj, lngList[ki+1], latList[kj+1]])
    newList = []
    for host in coordinate:
        newDict = {
            'ne_lng': host[0],
            'ne_lat': host[1],
            'sw_lng': host[2],
            'sw_lat': host[3],
            "interval": interval
        }
        # print posts.insert(newDict)
        newList.append(newDict)
    return newList

Here are three ways your code could be made more pythonic (which I consider to be more readable):

Use python keyword expansion:

Python allows the expansion of a dict object when calling a function.  This means we can change:

def getCoordinate(Country, interval):
    ne_lng = Country['ne_lng']
    ne_lat = Country['ne_lat']
    sw_lng = Country['sw_lng']
    sw_lat = Country['sw_lat']

To this:

def get_coordinate(ne_lng, ne_lat, sw_lng, sw_lat, interval):

if we call it like:

get_coordinate(interval=100, **Country)

Use list comprehension to remove loop boilerplate:

This:

i = 0
temp = ne_lng
lngList = []
n = int((ne_lng - sw_lng)/interval)+1
for _ in range(n):
    i += 1
    lngList.append(temp)
    temp -= (ne_lng - sw_lng) / n
i = 0
temp = ne_lat
latList = []
n = int((ne_lng - sw_lng)/interval)+1
for _ in range(n):
    i += 1
    latList.append(temp)
    temp -= (ne_lat - sw_lat) / n

becomes:

n = 1 + int((ne_lng - sw_lng) / interval)
lng_list = [
    ne_lng - (ne_lng - sw_lng) * i / n for i in range(n)]

n = 1 + int((ne_lng - sw_lng) / interval)
lat_list = [
    ne_lat - (ne_lat - sw_lat) * i / n for i in range(n)]

Build the output format while iterating:

This code takes the product of two lists and builds a list of lists of the results.  It then iterates through that list to generate a list of dicts.

for ki, vi in enumerate(lngList):
    for kj, vj in enumerate(latList):
        if ki == n-1 or kj == n-1:
            pass
        else:
            coordinate.append([vi, vj, lngList[ki+1], latList[kj+1]])
newList = []
for host in coordinate:
    newDict = {
        'ne_lng': host[0],
        'ne_lat': host[1],
        'sw_lng': host[2],
        'sw_lat': host[3],
        "interval": interval
    }
    # print posts.insert(newDict)
    newList.append(newDict)
return newList

But it is easy to build the dict during the first loop, which will elimnate one entire loop. In addition, the result list can be produced with a list comprehension, which gives:

return [
    {
        'ne_lng': vi,
        'ne_lat': vj,
        'sw_lng': lng_list[ki + 1],
        'sw_lat': lat_list[kj + 1],
        "interval": interval,
    }
    for ki, vi in enumerate(lng_list[:-1])
    for kj, vj in enumerate(lat_list[:-1])
]

Note:

I imagine the second line of:

n = 1 + int((ne_lng - sw_lng) / interval)

is a bug since at that point a lat is going to be calculated.

Code:

For completeness here is the entire code I ended up with:

def get_coordinate(ne_lng, ne_lat, sw_lng, sw_lat, interval):
    interval = float(interval)

    n = 1 + int((ne_lng - sw_lng) / interval)
    lng_list = [
        ne_lng - (ne_lng - sw_lng) * i / n for i in range(n)]

    n = 1 + int((ne_lng - sw_lng) / interval)
    lat_list = [
        ne_lat - (ne_lat - sw_lat) * i / n for i in range(n)]

    return [
        {
            'ne_lng': vi,
            'ne_lat': vj,
            'sw_lng': lng_list[ki + 1],
            'sw_lat': lat_list[kj + 1],
            "interval": interval,
        }
        for ki, vi in enumerate(lng_list[:-1])
        for kj, vj in enumerate(lat_list[:-1])
    ]

Country = {
    'ne_lat': 89.000000,
    'ne_lng': 179.000000,
    'sw_lat': -89.000000,
    'sw_lng': -179.000000
}

new_data = get_coordinate(interval=100, **Country)
old_data = getCoordinateOrig(Country, 100)
assert new_data == old_data


Arithmetic/Geometric series calculator I've written a small program that calculates Arithmetic and Geometric Partial Sums. I'd like feedback on anything possible, since I intend on writing a cheatsheet that encapsulates all PreCalculus equations.

partial_sum.py

"""
This is a program that calculates arithmetic and geometric
partial sums

"""

from fractions import Fraction

def find_an(parsed_series):
    """
    Finds an in the passed parsed arithmetic series

    :param parsed_series: The series to be analyzed

    """
    for i, _ in enumerate(parsed_series):
        if parsed_series[i] == ".":
            return int(parsed_series[i + 1])
    return None

def arithmetic_partial_sum(series):
    """
    Returns the partial sum of an arithmetic series

    Formula:
        S = n( (a1 + an) / 2 )

    Find an:
        an = a1 + (n - 1)d

    Find n:
        n = 1 + ( (an - a1) / d )

    :param series: Arithmetic series to solve

    """

    series = series.split("+")

    a1 = int(series[0])
    d = int(series[1]) - a1
    an = find_an(series)
    n = 1 + ((an - a1) / d)
    S = n * ((a1 + an) / 2)

    return S

def geometric_partial_sum(series):
    """
    Returns the partial sum of the geometric series

    :param series: Geometric series to solve

    Formula:
        S = (a1 * (1 - (r ** n))) / (1 - r)

    """

    series = series.split("+")

    a1 = int(series[0])
    r = int(series[1]) / a1
    n = len(series)
    S = (a1 * (1 - (r ** n))) / (1 - r)

    return str(Fraction(S).limit_denominator())

if __name__ == '__main__':
    A_SERIES = "3+7+11+15+.+99"
    G_SERIES = f"3+1+{1/3}+{1/9}+{1/27}+{1/81}"
    print(arithmetic_partial_sum(A_SERIES))
    print(geometric_partial_sum(G_SERIES))

find_an() looks like an internal function, used by arithmetic_partial_sum().  If it is not for external use, it should be named with a leading underscore, to suggest it is private.

arithmetic_partial_sum() appears to handle only integer values, yet it return a floating-point value (1275.0 in the built-in example).  It should return an integer, since it is adding up integers.  Use the Python3.x integer-division operator: //.

    n = 1 + (an - a1) // d
    S = n * (a1 + an) // 2

Or, not assume the terms are integer, and use float(...) instead.

geometric_partial_sum() fails if the first 2 values are not  int values:

>>> geometric_partial_sum(f"1+{1/3}+{1/9}+{1/27}+{1/81}")
Traceback (most recent call last):
  File "<pyshell#1>", line 1, in <module>
    geometric_partial_sum(f"1+{1/3}+{1/9}+{1/27}+{1/81}")
  File "...\partial_sum.py", line 63, in geometric_partial_sum
    r = int(series[1]) / a1
ValueError: invalid literal for int() with base 10: '0.3333333333333333'

You should convert the terms to floating-point values, not integers:

    a1 = float(series[0])
    r = float(series[1]) / a1

find_an() assumes \$a_n\$ is immediately after the '.' term, so will fail with:

arithmetic_partial_sum("3+7+11+15+.+95+99")
arithmetic_partial_sum("3+7+11+15+19")

Why not just retrieve the last term?

def find_an(parsed_series):
    return int(parsed_series[-1])

Now the following all succeed and return the correct values

arithmetic_partial_sum("3+7+11+15+.+95+99")
arithmetic_partial_sum("3+7+11+15+...+95+99")
arithmetic_partial_sum("3+7+11+15+19")

The """docstrings""" for arithmetic_partial_sum() and geometric_partial_sum() appear unhelpful.  For example:

>>> help(arithmetic_partial_sum)
Help on function arithmetic_partial_sum in module __main__:

arithmetic_partial_sum(series)
    Returns the partial sum of an arithmetic series

    Formula:
        S = n( (a1 + an) / 2 )

    Find an:
        an = a1 + (n - 1)d

    Find n:
        n = 1 + ( (an - a1) / d )

    :param series: Arithmetic series to solve

The function is not returning an or n.  Even the formula is not particularly helpful.  """docstrings""" should tell a user how to use the function.  For example (adding Python 3.6 type hints as well):

def arithmetic_partial_sum(series:str) -> int:
    """
    Returns the sum of an arithmetic series

    Example:
        s = arithmetic_partial_sum("1+3+5+.+99")  # returns 2500

    :param series: A string representing the arithmetic series to solve
    """

Now type help(arithmetic_partial_sum):

>>> help(arithmetic_partial_sum)
Help on function arithmetic_partial_sum in module __main__:

arithmetic_partial_sum(series: str) -> int
    Returns the sum of an arithmetic series

    Example:
        s = arithmetic_partial_sum("1+3+5+.+99")  # returns 2500

    :param series: A string representing the arithmetic series to solve

The user is told the function takes a string and returns an integer.  The format of the string should be clear from the example.


How to improve wordfeud solver algorithm? I created a simple wordfeud solver that checks what words can be created with the supplied letters.

public void GetResults(string characters)
{
    // place all the characters we have in a dictionary with key=letter, value=amount
    var charCount = SplitCharacters(characters);

    // get all the unique letters (the keys)
    char[] individualChars = charCount.Keys.ToArray();

    // create a regex for these letters, because only those letters are allowed in a word
    string regex = @"^[" + new string(individualChars) + "]+$";

    // get all the words that contain only the letters we have on our board
    IEnumerable<string> possibleWords = _words.Where(x => Regex.IsMatch(x, regex));

    // walk through the words that have only the letters on our board, and check if
    // there are sufficient letters on our board to create that word
    IEnumerable<string> results = possibleWords
        .Where(x => SplitCharacters(x).All(y => charCount[y.Key] >= y.Value) && x.Length > 1)
        .OrderByDescending(x => x.Length)
        .Take(25);

    // convert the words to ViewModel objects to display in the view
    foreach (string result in results)
    {
        Results.Add(new ResultViewModel { Word = result });
    }
}

private Dictionary<char, int> SplitCharacters(string characters)
{
    char[] chars = characters.ToLower().ToCharArray();

    var charCount = new Dictionary<char, int>();

    foreach (char c in chars)
    {
        if (!charCount.ContainsKey(c))
        {
            charCount[c] = 1;
        }
        else
        {
            charCount[c]++;
        }
    }

    return charCount;
}

It works, but I think this can be done more efficient. I worked this out on my own, but I feel like I'm creating too many new arrays/enumerables for all the steps in the algorithm. It runs quite fast on my PC (core i7 2600) but it's a little bit slow on my Surface tablet.

Can this be done more efficiently?

This is a rather interesting problem, first let's analyze a couple of algorithms and their respective algorithmic complexities.

We say that A is the set of characters given to us from the Wordfeud game: the set of characters from which we can form words. B is the lexigraphically sorted set of all valid words we can form, in practise, the words of a dictionary. The set of words in B that we can form from the characters in A is C.

Naive algorithms

The naive algorithm for this problem is to simply loop through all elements of B and check whether these words can be formed from the characters in B, with a regex like proposed here, or with a frequency map. If so, we put the word in C. For an average word length k and n words in B this runs in worst-case O(n k) time.

This is the fastest we can go without doing some kind of precomputation. In my tests, this algorithm runs in about 2s on the dictionary in /usr/share/dict/words in Ruby on 2 Ghz. This is the mean running time from different sets of A of 25 characters of which about 15 letters are distinct.

A simple iteration on this naive algorithm, is the observation that if a character is not in A, then we do not have to loop through all the words that start with this letter in B. E.g. if there's no letter c in A, we can skip to the words beginning with d when we encounter the first word which begins with c in B and so on. We can precompute a table that stores this information, i.e. the intervals at which the words start with a letter k. E.g. words with index 1..3482 in B all start with a. Thus if A does not contain a, we can just jump to the word with index the 3482 + 1.

The speed of this iteration depends on how many words in B start with each character in A. Say that v is the highest number of words that starts with any character in B, and i is the number of distinct characters from which we can form words, then this runs in worst-case O(v i) time. In my tests, this was about twice as fast as the previous algorithm.

Another iteration is threading it. For each segment we check (e.g. the words from a..b) we create a thread that checks this. With native threads, this halves the run time once again. This got me down to about 0.5s with the above setup running on JRuby.

Using a Trie

A trie is a tree data structure to store prefixes of strings. You start with a root node, and from there you have 25 edges, one for each character in the alphabet. If you follow one of these edges, e.g. the edge for the letter 'a' there will be X new edges. For each of those edges the prefix a + that letter exists in B. For instance one of the edges in X could be p and from there we could follow another edge e.

The vertex we landed at after following e is marked, because if we follow the edges back to the root node and save the characters on the way to the root in reverse order, we get a word in B: ape. The edge p existed because ap is a prefix of ape, and thus a prefix in B, as stated in the previous definition.

With this data structure in mind, we can design a new algorithm. Build a freqency map with default value 0 from A, i.e. a map where the key is the letter in A and the value of that key is the amount of times this letter occurs in A.

Traverse the tree with a depth-first search where you recursively traverse all options that you can do while all keys in the frequency map are > 0. Every time you hit a vertex which is marked, you traverse up to the root node to determine the word and add this word to C.

In reality you might want to pay the memory cost of storing the word at the vertex so you avoid traversing back to the root node every time you hit a marked vertex.

I won't get into algorithmic complexity analysis of this one, since it is rather complicated to analyze. However, in my tests, creating the trie from the dictionary takes less than 1s (e.g. for a web-service this penalty can just be paid when starting the app) and querying with a frequency table of A takes about 0.05s.

I do not know C#, however, I implemented this algorithm in Ruby.


Basic Queue in C++ Using Array I wrote a queue class in C++ using arrays of fixed width. Could anyone review my code ? I would appreciate any comment and recommendations. It works like a circular queue, so I handled back and front pointers in that manner.

#include <stdlib.h>
#include <iostream>

template <class T>
class Queue {
    public:
        Queue(void);
        Queue(Queue<T>& copyQueue);
        bool empty(void) const;
        std::size_t size(void) const;
        void clear(void);
        T front(void) const;
        void pop(void);
        void push(T& item);
        void print(std::ostream& os);
    private:
        static const std::size_t MAX_SIZE = 50;
        T list[MAX_SIZE];
        T* frontPtr;
        T* backPtr;
        std::size_t sizeQ;
};

template <class T>
Queue<T>::Queue(void) {
    frontPtr = nullptr;
    backPtr = nullptr;
    sizeQ = 0;
}

template <class T>
Queue<T>::Queue(Queue<T>& copyQueue) {
    frontPtr = nullptr;
    backPtr = nullptr;
    sizeQ = 0;
    if(copyQueue.backPtr >= copyQueue.frontPtr){
        for(T* i = copyQueue.frontPtr; i <= copyQueue.backPtr; i++){
            push(*i);
        }
    }
    else {
        for(T* i = copyQueue.frontPtr; i <= (copyQueue.list + MAX_SIZE -1); i++){
            push(*i);

        }
        for(T* i = copyQueue.list; i <= copyQueue.backPtr; i++) {
            push(*i);
        }
    }
}

template <class T>
bool Queue<T>::empty(void) const {
    return (sizeQ == 0);
}

template <class T>
std::size_t Queue<T>::size(void) const {
    return sizeQ;
}

template <class T>
void Queue<T>::clear(void) {
    frontPtr = nullptr;
    backPtr = nullptr;
    sizeQ = 0;
}

template <class T>
T Queue<T>::front(void) const {
    if(frontPtr == nullptr) {
        std::cerr << "Queue is empty. No front value" << '\n';
    }
    else {
        return *frontPtr;
    }
}

template <class T>
void Queue<T>::pop(void) {
    if(sizeQ == 0) {
        std::cerr << "Queue is empty. Can't pop." << '\n';
    }
    else{
        frontPtr = list + ((frontPtr - list + 1) % MAX_SIZE);
        sizeQ -= 1;
    }
}

template <class T>
void Queue<T>::push(T& item) {
    if(sizeQ == MAX_SIZE) {
        std::cerr << "Queue is full. Can't push" << '\n';
    }
    else{
        if(sizeQ == 0) {
            frontPtr = backPtr = list;
        }
        else {
            backPtr = list + ((backPtr - list + 1) % MAX_SIZE);
        }
        *backPtr = item;
        sizeQ += 1;
    }
}

template <class T>
void Queue<T>::print(std::ostream& os) {
    if(backPtr >=frontPtr){
        for(T* i = frontPtr; i <= backPtr; i++){
                os << *i << '\n';
        }
    }
    else {
        for(T* i = frontPtr; i <= (list + MAX_SIZE -1); i++){
            os << *i << '\n';
        }
        for(T* i = list; i <= backPtr; i++) {
            os << *i << '\n';
        }
    }
}

Edit: Not using googletest, but a simple main function to test the Queue.h can be found below:

#include "Queue.h"
int main() {
    Queue<int> newQueue;
    for(int i=1; i<110; i = i+2)
        newQueue.push(i);
    newQueue.pop();
    newQueue.pop();
    newQueue.pop();
    newQueue.pop();
    int i = 999;
    newQueue.push(++i);
    newQueue.push(++i);
    newQueue.push(++i);
    newQueue.push(++i);
    newQueue.print(std::cout);
    Queue<int> copiedQueue(newQueue);
    std::cout << copiedQueue.size() << '\n';
    copiedQueue.print(std::cout);
    for(int i=1; i<11; i = i+2)
        copiedQueue.pop();
    newQueue.print(std::cout);
    std::cout << copiedQueue.empty();
    std::cout << newQueue.empty();
    copiedQueue.print(std::cout);
    return 0;
}

Some general improvements:

Use <cstdlib>, not <stdlib.h>.  The latter is a deprecated header that is kept for C compatibility.  It should not be used in new C++ code.
Do not use void in an empty parameter list.  It is counterintuitive and is not necessary in C++ at all.  It is only used in C prototypes.
The copy constructor should take by const reference because it does not modify the argument.  Same for push.
You are using assignment in constructors when you ought to use member initializer clauses.  This is bad practice.
front should return by const reference, not by value.  Returning by value makes an unnecessary copy.
MAX_SIZE is not a macro and should not be in ALL_CAPS.  And it should be constexpr.  Or better, a template parameter.
It is advised in C++ to use std::array instead of raw arrays.

There are still many things to improve, but this should be enough to get you started.


Using JS/JQuery to serve ads from a self-hosted advertisements solution I'm working on a custom (self-hosted) ads solution that allows the users to create ad units (by uploading the banner image and target URL) and get the ad display code. A JSON response is returned (by the endpoint) for the requested ad. As of now, here is what I do to get and display the ad on the page where ad code is added:

JSON output at the endpoint:

{
    "success": true,
    "ad_url": "https:\/\/adlandingpage.com",
    "ad_banner": "http:\/\/localhost\/ads\/complete\/uploads\/banners\/add-banner.gif"
}

Content of rw-ads.js:

var getAds = function(ad_id, callback) {
  url = 'http://localhost/ads/complete/index.php/ads/adjson/'+ad_id+'/';
  var xhr = new XMLHttpRequest();
  xhr.open('get', url, true);
  xhr.responseType = 'json';
  xhr.onload = function() {
    var status = xhr.status;
    if(status == 200) {
      callback(null, xhr.response);
    } else {
      callback(status);
    }
  };
  xhr.send();
};

Ad code to be added to a page:

I need suggestions from experts. Am I doing this correctly or this can be improved? Thank you in advance for your help.

A few suggestions:

A function declaration is a bit simpler and more robust regarding hoisting compared to a function expression which assigns an anonymous function to a variable.
Supplying null as the first callback parameter on success is a bit quirky, either always supply the HTTP status or fail silently without calling the callback. If needed you can always introduce an error callback which might be called on xhr.onerror, too.
The async parameter of xhr.open defaults to true and can be removed
Your getAds function could be split into a generic getJSON function and a getAd function which supplies the specific ad URL to the former.
Your current getAds callback requires knowledge about the specific JSON returned by your ad server. This logic should be part of your "rw-ads.js" library and e.g. become part of the new getAd function.

Exemplary code:

function getJSON(url, success) {
  var xhr = new XMLHttpRequest();
  xhr.open('get', url);
  xhr.responseType = 'json';
  xhr.onload = function() {
    if (xhr.status === 200) {
      success(xhr.response);
    }
  };
  xhr.send();
};

function getAd(id, success) {
  getJSON('http://localhost/ads/complete/index.php/ads/adjson/' + id, function(data) {
    if (data.success) {
      success('');
    }
  });
}

getAd(1, function(html) {
  document.getElementById('ad1').innerHTML = html;
}

General considerations:

Since you don't rely on an external ad network but a self-hosted solution, you might want to inject your ads into your HTML document on the server side, without the asynchronous client side ajax request.


TicTacToe game in Ruby I would appreciate any feedback you could offer. I'm self-learning, so I don't really know a "standard" that my code should be up to. I'm also not sure how to get rid of that bit of logic at the bottom, or if it's acceptable.

class Board
    attr_reader :board, :place

    def initialize
        @board = [1, 2, 3, 4, 5, 6, 7, 8, 9]
        puts "Player 1 is X, Player 2 is O."
        show_board
    end

    def show_board
        @board.each_slice(3) { |x| puts x.join }
    end

    def get_place
        @place = gets.to_i - 1
    end

    def free_space?
        @board[place].is_a? Integer
    end

    def put_marker(mark)
        get_place
        until free_space?
            puts "Invalid move, choose again!"
            get_place
        end
        @board[place] = mark
        show_board
    end

    def player1_win_condition
        ary = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7], [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]]
        ary.any? do |array|
            array.all? { |num| @board[num - 1] == "X" }
        end
    end

    def player2_win_condition
        ary = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7], [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]]
        ary.any? do |array|
            array.all? { |num| @board[num - 1] == "O" }
        end
    end

    def draw_condition
        @board.all? { |num| num.is_a? String }
    end

    def end_game
        if player1_win_condition == true
            puts "Player 1 wins!"
            exit
        elsif player2_win_condition == true
            puts "Player 2 wins!"
            exit
        elsif draw_condition == true && player1_win_condition == false && player2_win_condition == false
            puts "It's a draw!"
            exit
        end
    end
end

class Player
    attr_accessor :mark

    def initialize(mark)
        @mark = mark
    end
end

player_1 = Player.new("X")
player_2 = Player.new("O")
new_board = Board.new

until new_board.end_game
    puts "Player 1 turn"
    new_board.put_marker(player_1.mark)
    if new_board.end_game != true
        puts "Player 2 turn"
        new_board.put_marker(player_2.mark)
    end
end

Gratulation! For  a beginner you made a good and clear game.

A first modified version:

class Board
    attr_reader :board

    def initialize(player1, player2)
        @board = (1..9).to_a    #You can use also a range to initialize the array
        puts "Player 1 is %s, Player 2 is %s." % [player1.mark, player2.mark]
        @player_1 = player1
        @player_2 = player2
        show_board
    end

    def play

      until end_game?
          puts "Player 1 turn"
          put_marker(@player_1.mark)
          if ! end_game?
              puts "Player 2 turn"
              put_marker(@player_2.mark)
          end
        end
    end

    def show_board
        puts '-'*9          #I prefer some lines in the board
        @board.each_slice(3) { |x| #Good! You use slice.
            puts x.join(' | ')
            puts '-'*9
          }
    end

    def get_place
      while
          place = gets.to_i - 1 #Use a return parameter
        #Check also if you get only values between 1 and 9 (no letters, no 0)
          if ! place.between?(0,9)
              puts "Invalid move, use 1..9. Please choose again!"
              next
          end
          if ! free_space?(place)
              puts "Invalid move, Pos. %i is occupied. Please choose again!"
              next
            end
            break #stop loop, Value is ok
        end
      return place
    end

    def free_space?(place)
        @board[place].is_a? Integer
    end

    def put_marker(mark)
        @board[get_place] = mark
        show_board
    end

    def win?(player)
        ary = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7], [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]]
        ary.any? do |array|
            array.all? { |num| @board[num - 1] == player.mark }
        end
    end

    def draw_condition
        @board.all? { |num| num.is_a? String }
    end

    def end_game?
        if win?(@player_1)  #use other check. The comparison wit == true is not necessary.
            puts "Player 1 wins!"
            show_board  #Show end situation
            exit
        elsif win?(@player_2)
            puts "Player 2 wins!"
            show_board  #Show end situation
            exit
        elsif draw_condition == true && player1_win_condition == false && player2_win_condition == false
            puts "It's a draw!"
            exit
        end
    end
end

class Player
    attr_accessor :mark

    def initialize(mark)
        @mark = mark
    end
end

new_board = Board.new(Player.new("X"), Player.new("O"))
new_board.play

I removed the reader place and the variable @place.
There is no reason why you need it.
It is only a temporary used variable, there is no need to store it in the class.
I replaced it with local variables and method return parameters.

The use of the return command is not necessary. I used it here to show explicit when I want to return a value.

I integrated the checks into get_place.
There is also a check for wrong values (like letters) and different messages.

I replaced player1_win_condition with a generic test with parameter.

I integrated the game in a method play. So it would be easier to start multiple games.

Most important: You used the fix constant 'X' and 'O' in your Game. But this data are also defined a parameter in the class Player. This can make problems if you replace X by x or something else.

What is missing:
* There is no check for similar Player.marks

What could be checked/Modified:

I don't like the end_game? in the actual version.
You could integrate it to play (player 1 can only win after his move, same for player 2. Then end_game? needs only to check for draw.
The board could be a simple array without initialization. nil would indicate free fields. This could make some checks easier. But with the initialization the board gets its initial values to be shown in show_board.

Another version!

The first version had some code repetition. For each player the code was written again.

The new version avoids this repetion:

class Board
        attr_reader :board

    def initialize(player1, player2)
        @board = (1..9).to_a    #You can use also a range to initialize the array
        @player_1 = player1
        @player_2 = player2
        puts "Player 1 is %s, Player 2 is %s." % [player1.mark, player2.mark]
        show_board
    end

    def play
      sequence = [@player_1,@player_2]
      until draw?
          puts "Player %s turn" % sequence.first.name
          put_marker(sequence.first.mark)
          if win?(sequence.first)
            puts "Player %s wins!" % sequence.first.name
            show_board  #Show end situation
            return
          end
          sequence.rotate! #switch to next player
        end
        puts "It's a draw!" #end message if not left before, becaus eone player wins
    end

    def show_board
        puts '-'*9          #I prefer some lines in the board
        @board.each_slice(3) { |x| #Good! You use slice.
            puts x.join(' | ')
            puts '-'*9
          }
    end

    def get_place
      while
          place = gets.to_i - 1 #Use a return parameter
        #Check also if you get only values between 1 and 9 (no letters, no 0)
          if ! place.between?(0,9)
              puts "Invalid move, use 1..9. Please choose again!"
              next
          end
          if ! free_space?(place)
              puts "Invalid move, Pos. %i is occupied. Please choose again!"
              next
            end
            break #stop loop, Value is ok
        end
      return place
    end

    def free_space?(place)
        @board[place].is_a? Integer
    end

    def put_marker(mark)
        @board[get_place] = mark
        show_board
    end

    def win?(player)
      ary = [[1, 2, 3], [4, 5, 6], [7, 8, 9], [1, 4, 7], [2, 5, 8], [3, 6, 9], [1, 5, 9], [3, 5, 7]]
      ary.any? do |array|
          array.all? { |num| @board[num - 1] == player.mark }
      end
    end

    def draw?
        @board.all? { |num| num.is_a? String }
    end

end

class Player
    attr_accessor :mark
    attr_accessor :name

    def initialize(name, mark)
        @name = name
        @mark = mark
    end
end

new_board = Board.new(Player.new('Fred', "X"), Player.new('Joe', "O"))
new_board.play

To identify the players I need also a name. Before you had the fix 'Player one'. You could also remove the text Player in the output (replace Player X turn with X turn).


ResizableArray based queue implementation I am implementing basic data structures in Java. I have written this simple resizing array queue implementation and looking for ways to simplify the implementation as it looks like I am using too many variables which can be reduced. I would appreciate any ideas to make it simpler, implementation advice and constructive criticisms.

import java.util.Iterator;
import java.util.NoSuchElementException;

public class ResizingArrayQueue<T> implements Iterable<T>{
    private T[] mQueue;
    private int mHead, mTail, mCurrentSize, mSize;

    public Iterator<T> iterator() {
        return new QueueIterator();
    }

    public ResizingArrayQueue() {
        mQueue = (T[]) new Object[1];
        mHead = 0;
        mSize = 1;
        mTail = mSize - 1;
        mCurrentSize = 0;
    }

    public void enQueue(T valueToEnque){
        if (mCurrentSize == mSize) {
            resize(2 * mSize);
        }
        mTail = ++mTail % mSize;
        mQueue[mTail] = valueToEnque;
        mCurrentSize++;
    }

    public T deQueue(){
        if(mCurrentSize == mSize / 4) {
            resize(mSize / 2);
        }
        T value = mQueue[mHead % mSize];
        mQueue[mHead % mSize] = null;
        mHead++;
        mCurrentSize--;
        return value;
    }

    public boolean isEmpty() {
        return mCurrentSize == 0;
    }

    public void resize(int size){
        T[] newQueue = (T[]) new Object[size];
        int mCurrent = 0;
        while(mCurrent < mCurrentSize){
            newQueue[mCurrent] = mQueue[(mCurrent + mHead) % mSize];
            mCurrent++;
        }

        mQueue = newQueue;
        mSize = size;
        mHead = 0;
        mTail = mCurrentSize - 1;
    }

    private class QueueIterator implements Iterator<T> {
        private int mCurrent = 0;

        public boolean hasNext() {
            return mCurrent < mCurrentSize;
        }

        public T next() {
            if (!hasNext()) {
                throw new NoSuchElementException();
            }
            T valueToReturn = mQueue[(mCurrent + mHead) % mSize];
            mCurrent++;
            return valueToReturn;
        }

        public void remove(){
            throw new UnsupportedOperationException();
        }
    }
}

private T[] mQueue;
    private int mHead, mTail, mCurrentSize, mSize;

The value that you are storing in mSize is available as mQueue.length.

    private T[] mQueue = (T[]) new Object[1];
    private int mHead = 0;
    private int mSize = 0;

But if you get rid of mSize, then there is no reason to differentiate mCurrentSize.  So just call that mSize.

You also don't need mTail, as mHead, mSize, and mQueue.length determine mTail uniquely.

You don't need a constructor, as you can initialize these values without further calculation.

        T[] newQueue = (T[]) new Object[size];
        int mCurrent = 0;
        while(mCurrent < mCurrentSize){
            newQueue[mCurrent] = mQueue[(mCurrent + mHead) % mSize];
            mCurrent++;
        }

        mQueue = newQueue;
        mSize = size;
        mHead = 0;
        mTail = mCurrentSize - 1;

There's a built-in for this.

        mQueue = Arrays.copyOfRange(mQueue, mHead, mHead + size);
        mHead = 0;

Now you don't newQueue or mCurrent.

As previously said, you don't need mSize or mTail.

    public void enQueue(T valueToEnque){
        if (mCurrentSize == mSize) {
            resize(2 * mSize);
        }
        mTail = ++mTail % mSize;
        mQueue[mTail] = valueToEnque;
        mCurrentSize++;
    }

    public T deQueue(){
        if(mCurrentSize == mSize / 4) {
            resize(mSize / 2);
        }
        T value = mQueue[mHead % mSize];
        mQueue[mHead % mSize] = null;
        mHead++;
        mCurrentSize--;
        return value;
    }

To implement, you could do it like

    public void enQueue(T valueToEnque) {
        if (mSize >= mQueue.length) {
            resize(2 * mQueue.length);
        }

        mQueue[(mHead + mSize) % mQueue.length] = valueToEnque;
        mSize++;
    }

    public T deQueue() {
        if (mSize <= mQueue.length / 4) {
            resize(mQueue.length / 2);
        }

        T value = mQueue[mHead];
        mQueue[mHead] = null;
        mHead = ++mHead % mQueue.length;
        mSize--;

        return value;
    }

By using <= or >= rather than strict equality, we can catch situations where the size jumped over the gating.  For example, if you implemented QueueIterator.remove without calling deQueue, you might find that you had skipped over the resize check.

See how we determine the value that would have been held in mTail as needed rather than maintaining it.

Notice how things are easier if we maintain mHead as the actual index rather than allowing it to grow unbounded.  Note that this also avoids integer overflow in an edge case (if you never resize, mHead always grows).


Factory for report handlers Below is my try.

 //DAL
 //This is my interface
 namespace iPublisher.DAL.Interfaces
 {
   public interface IReportHandler
   {
       IEnumerable<DocumentMapper> FetchDocuments(int clientId, int pager);
    }
 }

//my concrete class
public class ReportHandler : IReportHandler
{
   public IEnumerable<DocumentMapper> FetchDocuments(int clientId, int pager)
   {
       //LINQ
   }
}

//Factory Interface
namespace iPublisher.DAL.Interfaces
{
public interface IReportFactory
{
    IReportHandler Create(int factoryId);
}
}

//Factory class
namespace iPublisher.DAL.Factory
{
public class ReportFactory : IReportFactory
{
    private IReportHandler reportObj;

    public IReportHandler Create(int factoryId)
    {
        switch (factoryId)
        {
            case 1:
                reportObj = new ReportHandler();
                return reportObj;
            default:
                throw new ArgumentException("");
        }

     }
 }
}

//UI Layer

namespace iPublisher.Controllers
{
[Authorize]
[SessionExpire]
public class ReportsController : Controller
{
      ReportFactory factObj = new ReportFactory();

    public string GetAllDocuments(string url,int pager =0)
    {
        if (SessionInfo.IsAdmin)
        {
            var documentsDataJSON = GetDocumentIntoJson(SessionInfo.ClientID, pager);

            return documentsDataJSON;
        }
        else
        {
            return "Sorry!! You are not authorized to perform this action";
        }
    }

    private string GetDocumentIntoJson(int clientId, int pager)
    {

        IReportHandler reportObj = factObj.Create(1);

        var documents = reportObj.FetchDocumentsList(clientId, pager);

        string documentsDataJSON = JsonConvert.SerializeObject(documents);

        return documentsDataJSON;
    }

 }

}

Code Metrics still showing Maintainability Index to 60.

What I am missing here or how can I improve my implementation?

Not sure if that's going to improve the maintainability index, but

You're misimplementing Inversion of Control. IoC is when your dependency (namely, IReportHandler) is given to you from outside the class. It isn't the case here. It defeats the whole purpose of having an abstract dependency instead of a concrete one.

Read up on Inversion of Control and Dependency Injection and how they can improve code maintainability and testability. Don't use them unless it is an educated choice to do so.
You're using a Factory where it has no benefit.

Read up on the different kinds of factories (Factory from DDD, Factory method and Abstract Factory from Go4). Don't use Factories except where there's a clear advantage to doing so.
The code is too complicated for what it does.

Why the factoryId, switch statement and magic value 1?

The url parameter is never used.

Why the intermediate variables documentsDataJSON when you could just return <expression> and save one line of code for more clarity?
Naming is awkward and violates the Principle of Least Surprise.

Why would you expect a ReportHandler to Create a report, while ReportFactory creates a... ReportHandler?

Why does FetchDocuments() not return a list of Documents, but a list of DocumentMappers?

Don't try to mimic code samples or game a maintainability analysis tool. Learn about the philosophy behind the big design principles/patterns and decide for yourself if, where and how to apply them.


Beginner - Guessing Game In the past few days i have started to learn python 3 in my spare time and i am really enjoying it so far.

I have attempted many simple programs such as a palindrome checker and reverse a string in an attempt to improve my skills.

I was hoping that someone would be able to provide some feedback on the following program which is a guessing game program. The user is able to choose the number of guesses and the max possible value.

From my own perspective i think that possibly i am relying too much on while loops and if/else statements. Also i am not sure if my comments are sufficient, do they lack detail?
Also i use parentheses around the conditionals in my if/else statements and in my while loops as i find it easier to read the condition this way. Is this a bad idea? Could it confuse people who are trying to read my code?

# In this program, the user must try to guess a random number generated by the computer
from random import randint

def guess_game():
    replay = 1
    #Replay allows a user to replay the game without having to call the function again.
    #The values are: 1 = Replay, any other character = End program
    while (replay == 1):
        #lives = Number of chances you have to correctly guess the answer before the program finishes
        lives = int( input("How many lives would you like? ") )
        #max_value = upper bound for the randomly generated number. Starts at 0
        max_value = int( input("Please choose the largest number that can be generated: ") )
        random = randint(0, max_value)  #Generates a random int between 0 and max_value
        num_of_guesses = 1  #keeps track of the number of guesses made by the user
        while (num_of_guesses <= lives):
            guess = int( input("Please enter your guess: ") )
            if (guess == random):
                print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
                break  #if answer is correct this will exit the while loop
            else:
                print("Unlucky, that guess was incorrect. You have", (lives-num_of_guesses), "lives remaining.\n")
            num_of_guesses = num_of_guesses + 1
        if (num_of_guesses > lives):
            print("You have run out of guesses. The correct answer was:", random)
        replay = int( input("\nWould you like to play again? Enter 1 for yes, any other character for no: ") )
        if (replay != 1):
            print("Thank you for playing. See you next time")

guess_game()

Functions

Everything is in one function. You should stick to the single responsibility principle. Basically, a function should have one job.

In a way, that does mean your whole game should be in the function, but it'd be better to have parts of it separated out. For example, getting an int from the user could be a function:

def get_int(message)
    return int(input(message))

lives = get_int("How many lives would you like? ")
max_value = get_int("Please choose the largest number that can be generated: ")

Now this may seem unnecessary, but now you could add input validation. Caridorc pointed out a bug, so you should do error handling. The best way to do this in Python is to catch any ValueErrors that get thrown.

    try:
        return int(input(message))
    except ValueError:
        print("Please enter an integer only")

Now the code wont crash when an invalid string is entered, but it also wont return a value. So you want to loop over this with while until a valid number is entered and returned, ending your loop. This is how it would look:

def get_int(message)
    while True:
        try:
            return int(input(message))
        except ValueError:
            print("Please enter an integer only")

Now you have error handling set up, but you can still use one line to get each value:

lives = get_int("How many lives would you like? ")
max_value = get_int("Please choose the largest number that can be generated: ")

Pythonic style

While we're on while, there's a more Pythonic way to do your main game loop. Instead of using replay, just use another infinite loop with while True and end your loop with a break, like this:

if replay != 1:
    print("Thank you for playing. See you next time")
    break

Also you don't need brackets around the condition, Python looks nicer without them and evaluates it just fine.

Instead of using

num_of_guesses = num_of_guesses + 1

use Python's += operator:

num_of_guesses += 1

In fact, instead of that at all, you could just use a for loop instead of a while loop here:

for num_of_guesses in range(1, lives + 1):
    guess = get_int("Please enter your guess: ")
    if guess == random:
        print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
        break  #if answer is correct this will exit the while loop
    else:
        print("Unlucky, that guess was incorrect. You have", (lives - num_of_guesses), "lives remaining.\n")
if num_of_guesses > lives:
    print("You have run out of guesses. The correct answer was:", random)

range lets you just loop over a list of numbers. In this case, it loops from one up to lives. Range will iterate up to but not include the second parameter, which means if you pass lives + 1 the last loop will be on the value of lives.

You could also make use of Python's else clause for for loops. An else after a for loop in Python will be executed if the loop fully runs it's course without reaching a break command. If break gets called then the else block is skipped. This works perfect for your case, so I'd write it like this:

for num_of_guesses in range(1, lives + 1):
    guess = get_int("Please enter your guess: ")
    if guess == random:
        print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
        break  #if answer is correct this will exit the while loop
    else:
        print("Unlucky, that guess was incorrect. You have", (lives - num_of_guesses), "lives remaining.\n")
else:
    print("You have run out of guesses. The correct answer was:", random)

Readability

You have good names here. They're clear and readable, without being too long or overlapping. Although, I would advise against using random as a variable name. It doesn't cause a clash here since you used from random import randint, but it causes mild confusion. Something like answer would actually be more useful for indicating what the number is for.

However you have a lot of comments that don't need to be here. For example:

        break  #if answer is correct this will exit the while loop

This is quite a verbose comment. At most I would have # Correct answer. People reading this code probably know how break works. Your comments should focus on intent and abstract meaning. Python programmers can read your code and know what each part does, but why it does it, and what you want to do that for are useful to know in comment form. (as a beginner, perhaps you want to remind yourself of what these things do. This may be of benefit to start with, but I suggest reducing them as soon as you feel comfortable with it).


Correct approach to wait for multiple async methods to complete I have an IWorkflow interface defined as follows:

public interface IWorkflow
{
    Task ConfigureAsync();
    Task StartAsync();
    Task StopAsync();
}

And I have an Engine class:

public sealed class Engine : IEngine
{
    private readonly List<IWorkflow> workflows = new List<IWorkflow>();

    public Engine(IEnumerable<IWorkflow> workflows)
    {
        this.workflows.AddRange(workflows);
    }

    public void Start()
    {
        var configureTasks = this.workflows.Select(w => w.ConfigureAsync()).ToArray();
        Task.WaitAll(configureTasks);

        var startTasks = this.workflows.Select(w => w.StartAsync()).ToArray();
        Task.WaitAll(startTasks);
    }

    public void Stop()
    {
        var stopTasks = this.workflows.Select(w => w.StopAsync()).ToArray();
        Task.WaitAll(stopTasks);
    }
}

Is this the correct way for the Engine to invoke in parallel the configure method on all workflows and then once all are completed, invoke in parallel the start method on all workflows?

Your code is absolutely correct in case when you want to start workflows only when all of them are configured.

But if you want to start each workflow once it's configured (independently from other workflows) then it might be a good idea to use continuations... In .NET 4.5 it would look like this:

public sealed class Engine : IEngine
{
    private readonly List<IWorkflow> _workflows;

    public Engine(IEnumerable<IWorkflow> workflows)
    {
        _workflows = new List<IWorkflow>(workflows);
    }

    private async Task RunWorkflow(IWorkflow workflow)
    {
        await workflow.ConfigureAsync();
        await workflow.StartAsync();
    }

    public void Start()
    {
        var startTasks = this._workflows.Select(RunWorkflow).ToArray();
        Task.WaitAll(startTasks);
    }

    public void Stop()
    {
        var stopTasks = _workflows.Select(w => w.StopAsync()).ToArray();
        Task.WaitAll(stopTasks);
    }
}

Also I would suggest to use CancellationToken to stop asynchronous processing.

UPDATE Based on comments it is really needed to wait for all workflows to be configured before starting them. So cancellable implementation can look like this:

public interface IWorkflow
{
    Task ConfigureAsync(CancellationToken token);
    Task StartAsync(CancellationToken token);
}

public sealed class Engine : IEngine
{
    private readonly List<IWorkflow> _workflows;
    private readonly CancellationTokenSource _cancellationTokenSource;
    private Task _mainTask;

    public Engine(IEnumerable<IWorkflow> workflows)
    {
        _workflows = new List<IWorkflow>(workflows);
        _cancellationTokenSource = new CancellationTokenSource();
    }

    private async Task RunWorkflows()
    {
        await Task.WhenAll(_workflows.Select(w => w.ConfigureAsync(_cancellationTokenSource.Token)));
        if (_cancellationTokenSource.IsCancellationRequested)
            return;
        await Task.WhenAll(_workflows.Select(w => w.StartAsync(_cancellationTokenSource.Token)));
    }

    public void Start()
    {
        _mainTask = RunWorkflows();
        _mainTask.Wait();
    }

    public void Stop()
    {
        _cancellationTokenSource.Cancel();
        var mainTask = _mainTask;
        if (mainTask != null)
            mainTask.Wait();
    }
}