Speed up a simple edge connecting algorithm I have a list of edges and a similar list of links (linking edges) telling which edges to connect (how to order them). Connected edges form polygons and there can be many disconnected polygons (ordering of which doesn't matter). For example:

edges = [((2, 1), (3, 1)), ((4, 1), (6, 1)), ((6, 4), (4, 4)), ((3, 6), (2, 6))]
links = [((2, 6), (2, 1)), ((3, 1), (3, 6)), ((4, 4), (4, 1)), ((6, 1), (6, 4))]

We would connect the edge ((2, 1), (3, 1)) with the edge ((3, 6), (2, 6)) because the linking edge ((3, 1), (3, 6)) is in links, etc. The result would be ordering of edges as:

[((2, 1), (3, 1)), ((3, 6), (2, 6)), ((4, 1), (6, 1)), ((6, 4), (4, 4))]

where ((2, 1), (3, 1)), ((3, 6), (2, 6)) forms a polygon and ((4, 1), (6, 1)), ((6, 4), (4, 4)) another one.

So the idea is to check if (edge[i][1], edge[j][0]) is in links, for all i!=j. If true, then connect the edge[i] with edge[j] by putting the latter after the former, and repeat with (edge[j][1], edge[k][0]) etc.

I use the following function to do it:

def connect_edges(edges, links):
    edges = dict(edges)
    links = dict(links)
    seen = set()
    edges_connected = []
    for e in edges.items():
        while e not in seen:
            seen.add(e)
            edges_connected.append(e)
            e = (links[e[1]], edges[links[e[1]]])
    return edges_connected

It is quite fast, but for a very large number of edges (e.g. 1 million) it takes almost 20 seconds on my old laptop. Is there a way to speed it up? I think it is more or less optimal and maybe could be cynthonized for a gain in speed, but the problem is that it is very pythonic (having lists of tuples, dictionaries, sets) and I don't know if it will work (my knowledge of cython is very limited). Simply cythonizing without declaring types gives minuscule speed-up.

Just a collection of thoughts...

Terminology

Since you're talking about graphs, it might help the readers if you talk about nodes (your edges) and edges (your links).

Your graph is directed, and as far as I can tell, you're looking for the "strongly connected components".

Use simpler objects

Lists of pairs of pairs aren't very readable IMHO. When showing examples, you can replace them with lists of strings:

edges = ['AB', 'CD', 'EF', 'GH']
links = ['HA', 'BG', 'FC', 'DE']

Python strings are basically tuples of characters, so you don't have to change much in your code. The above example is isomorphic to yours and is much more concise.

KeyError

When calling edges[links[e[1]], you're hoping there won't be any KeyError. Your example would fail with :

edges = ['AB', 'CD', 'EF', 'GH']
links = ['HA', 'BG', 'FC']

Links between multiple edges

What happens in the case of link connecting more than 2 edges, for example

edges = ['AB', 'GH', 'AC']
links = ['HA']

? Your current algorithm doesn't seem to take it into account. A defaultdict(list) might help, for example to keep a list of nodes beginning with A and another list for nodes finishing with H.

Output format

If you only sort the edges, without grouping them in connected components, you're losing information which you have to recover later. You could return a list of sets of edges instead of a flat list of edges.

[{'AB', 'GH'}, {'CD', 'EF'}]

networkx

If you work with graph theory in Python, you should take a look at networkx. It's fast, easy to use and offers many algorithms. All you need to do is to preprocess your data and feed it to a nx.DiGraph.

Your example could become:

import networkx as nx
from collections import defaultdict
import matplotlib

edges = ['AB', 'CD', 'EF', 'GH']
links = ['HA', 'BG', 'FC', 'DE']

G = nx.DiGraph()

from_dict = defaultdict(list)
to_dict = defaultdict(list)

for edge in edges:
    node = ''.join(edge)
    G.add_node(node)
    to_dict[node[0]].append(node)
    from_dict[node[1]].append(node)

for link in links:
    for node1 in from_dict[link[0]]:
        for node2 in to_dict[link[1]]:
            G.add_edge(node1, node2)

list(nx.strongly_connected_components(G))
# [{'AB', 'GH'}, {'CD', 'EF'}]
nx.draw(G, with_labels = True)

It displays:

You get more information, it's more robust, you get diagrams if you want, it can handle more complex cases and (who knows?) it might be faster than your solution for large datasets.


Javascript class to geocode Dutch addresses I am relatively new to javascript ES6, the code I've written is working fine. However I am curious if this would be the best approach. I am trying to write a reusable module that I can use in multiple projects. On top of an API I've written a simple module to geocode adresses in the Netherlands. The suggest function will provide related adresses to a specific query. The lookup function provides information about a specific adress.

class Geocoder {
    // Class to geocode adresses to coordinates or find related adresses
    // suggest(), lookup() both return a promise
    // geocoder.lookup('adr-54e85361e27f833dd8331fd85bc46ac6').then((data) => {console.log(data)});

    constructor() {
        this.url = "https://geodata.nationaalgeoregister.nl/locatieserver/v3/";
    }

    async suggest(query) {
        let url = `${this.url}suggest?q=${query}`;
        return await this._get(url);
    }

    async lookup(id) {
        let url = `${this.url}lookup?id=${id}`;
        return await this._get(url);
    }

    _get(url) {
        return new Promise ((resolve, reject) => {
            fetch(url).then((response) => {
                return response.json();
            }).then((json) => {
                resolve(json);
            }).catch((err) => {
                reject(err);
            })
        })
    }
}

Using Promise when async/await is supported is a sign of an anti-pattern. The only case you ever manually build promise in an environment that supports async/await is when the asynchronous operation cannot be written linearly/when you can't await (e.g. resolve must be called in a callback).

// You cannot really "await" this kind of API, hence the wrapper Promise.
foo() {
  return new Promise((resolve, reject){
    someAPIThatOnlyDoesCallbacks((err, data) => {
      if (err) reject(err)
      else resolve(data)
    })
  })
}

But in your case, fetch returns a promise, response.json() also returns a promise. You can just return the promise generated by the whole fetch operation itself.

_get(url) {
  return fetch(url).then(r => r.json())
}

Next, consider using Url and UrlSearchParams to construct your url. This way, the url is constructed correctly.

async suggest(url, param1, param2, ....){
  // Create the url
  const url = new URL(url)

  // Append the values
  url.searchParams.append('param1', param1)
  url.searchParams.append('param2', param2)

  // Construct the url
  return await this._get(url.toString())
}

On to other things, there's currently nothing in your class that is instance-specific (the base url is hardcoded, params are supplied at call time). You can simply export a bunch of stateless functions from the module instead of having to instantiate an instance.

const getJSON = url => fetch(url).then(r => r.json())
const geocoderUrl = "https://geodata.nationaalgeoregister.nl/locatieserver/v3/"

export const suggest = async (query) {
  const url = new URL('suggest', geocoderUrl)
  url.searchParams.append('q', query)
  return await getJSON(url.toString());
}

export const lookup = async(id) => {
  const url = new URL('lookup', geocoderUrl)
  url.searchParams.append('id', id)
  return await getJSON(url.toString());
}


Fake Marquee Text in Haskell This is my very first complete, non-completely-trivial Haskell program, and I'm extremely unfamiliar with the language.  I was hoping that I could get some pointers on how to make it more idiomatic.  I'm especially unfamiliar with built-in functions.

I'm specifically concerned with my main function and my scrollHelp guards, as it feels like there should be a better way to do both.  Also the naming of the scroll and scrollHelp functions is arbitrary, and they might be able to be combined.

The program itself is very simple, it takes a numerical width and a string as command line parameters and transforms that string into a fake marquee.

Example:

  ./scroll 4 testing

[    ]
[   t]
[  te]
[ tes]
[test]
[esti]
[stin]
[ting]
[ing ]
[ng  ]
[g   ]
[    ]

scroll.hs

import System.Environment
import System.Exit
import System.IO

slice :: Int -> Int -> [Char] -> [Char]
slice from to xs = take to $ drop from $ xs

slice0 :: Int -> [Char] -> [Char]
slice0 = slice 0

join :: [a] -> [[a]] -> [a]
join a xss = concatMap (++ a) xss

spaces :: Int -> [Char]
spaces a = concat $ take a $ repeat " "

lead = "    ["
end = "]"

format :: [Char] -> [Char]
format xs = lead ++ xs ++ end

scroll :: Int -> [Char] -> [[Char]]
scroll a [] = [format $ spaces a]
scroll a x = scrollHelp a (spaces a ++ x ++ spaces a)

scrollHelp :: Int -> [Char] -> [[Char]]
scrollHelp a xall@(_:xs)
  | length xall >= a = (format $ slice0 a xall) : scrollHelp a xs
  | otherwise      = []
scrollHelp a _ = []

main = do
  args <- getArgs
  case args of
    [widthString, text] | [(width,_)] <- reads widthString ->
      putStrLn $ join "\n" (scroll width text)
    _ -> do
      name <- getProgName
      hPutStrLn stderr $ "usage: " ++ name ++ " <marquee width> <text to marquee>"
      exitFailure

use replicate a ' ' instead of concat $ take a $ repeat " "
use pattern matching in main to check that there are exactly two arguments and use it also for read to check that the width is exactly a number. With [(width,_)] you accept also strings that only start with a number, such as "1O" where probably you made a mistake and you wrote it instead of "10"
lead and end can be specific to format to avoid exposing them. Actually you can also remove them and just write "    [" ++ xs ++ "]"
scroll can be simplified and both the helper and the format function can should be only visible to it
don't concat the output together before writing it: just write each line in output. UseforM_ for multiple IO operations over a list of values.

So, my rewrite of your code is

import Control.Monad (forM_)
import System.Environment
import System.Exit
import System.IO

scroll :: Int -> [Char] -> [[Char]]
scroll a = reverse . scrollHelp [] . addSpaces
  where
    addSpaces xs = replicate a ' ' ++ xs ++ replicate a ' '
    scrollHelp acc str
      | length str < a = acc
      | otherwise      = let group = take a str
                         in scrollHelp (format group:acc) (tail str)
    format xs = "    [" ++ xs ++ "]"

main = do
  args <- getArgs
  case args of
    (widthString:text:[]) ->
       case reads widthString of
         [(width,"")] -> forM_ (scroll width text) putStrLn
         _ -> hPutStrLn stderr $ "width must be an integer, got '" ++ widthString ++ "'"
    _ -> do
      name <- getProgName
      hPutStrLn stderr $ "usage: " ++ name ++ " <marquee width> <text to marquee>"
      exitFailure


Improve grouping in LINQ I have a collection (Child Collection) on which I am performing some operation to make another collection (Parent Collection). But that I do in a dirty foreach loop. I want to do it in an elegant way.

GroupInfo grpInfo;
List<GroupInfo> lstGroupInfo = new List<GroupInfo>();
foreach (AddressInfo addressInfo in subDetails)
{
 if (lstGroupInfo.Where(u => u.Addres1 == addressInfo.Address1).Count() > 0)
    {
        grpInfo = lstGroupInfo.Where(u => u.Addres1 == addressInfo.Address1).SingleOrDefault();

        if (addressInfo.Rural)
            grpInfo.Rural = true;
        else if (addressInfo.Urban)
            grpInfo.Urban = true;

        grpInfo.SubDetails.Add(addressInfo);
    }
    else
    {
        grpInfo = new GroupInfo();
        grpInfo.AddressID = addressInfo.AddressID;
        grpInfo.LocationID = addressInfo.NamedLocationID;

        if (addressInfo.Rural)
            grpInfo.Rural = true;
        else if (addressInfo.Urban)
            grpInfo.Urban = true;

    }
}

GroupInfo class is:

public class GroupInfo
{
    public string Address1
    {
        get;
        set;
    }

    public int AddressID
    {
        get;
        set;
    }

    public int? LocationID
    {
        get;
        set;
    }
}

I want to do it in LINQ lambda way.

There's quite a lot of logic in there, so I doubt you will get there completely by using LINQ, and, more importantly, if it would actually be more elegant.

I would rewrite as follows, and be done with it:

        foreach (AddressInfo addressInfo in subDetails)
        {
            grpInfo = lstGroupInfo.SingleOrDefault(x => x.Address1 == addressInfo.Address1);
            if (grpInfo != null)
            {
                grpInfo.Rural = addressInfo.Rural;
                grpInfo.Urban = addressInfo.Urban;
                grpInfo.SubDetails.Add(addressInfo);
            }
            else
            {
                grpInfo = new GroupInfo();
                grpInfo.AddressID = addressInfo.AddressID;
                grpInfo.LocationID = addressInfo.NamedLocationID;
                grpInfo.Rural = addressInfo.Rural;
                grpInfo.Urban = addressInfo.Urban;
            }
        }

In addition, it seems to me that Rural and Urban are mutually exclusive, so why not define an enumeration that contains those two (or more) values, or declare one boolean property IsRural to indicate if it's "Rural", and if not, it's urban. That would bring the code down to:

        foreach (AddressInfo addressInfo in subDetails)
        {
            grpInfo = lstGroupInfo.SingleOrDefault(x => x.Address1 == addressInfo.Address1);
            if (grpInfo != null)
            {
                grpInfo.IsRural = addressInfo.IsRural;
                grpInfo.SubDetails.Add(addressInfo);
            }
            else
            {
                grpInfo = new GroupInfo();
                grpInfo.AddressID = addressInfo.AddressID;
                grpInfo.LocationID = addressInfo.NamedLocationID;
                grpInfo.IsRural = addressInfo.IsRural;
            }
        }


DDD model of a purchase in NodeJS I already have some experience with Java and I'm used to modeling with a DDD approach. Therefore, now I'm beginning with JS and NodeJS and I'd like to know which is the best practice to design with a similar approach in Javascript (without ES6).

Here I have an example of what I'm trying to do, but I don't know if these is the better way. I appreciate some opinions.

function Purchase(pCustomer, pOrders, pPayment, pTotal) {
    let idPurchase = PurchaseId.purchaseId();
    let customer = pCustomer;
    let payment = pPayment;
    let orders = pOrders;
    let deliveryAddress;
    let billingAddress;
    let total = pTotal;

    // Return an object exposed to the public
    return {
        setOrders: function (o) {
            orders = o
        },
        setDeliveryAddress: function (address1, address2, zipCode, city) {
            deliveryAddress = new Address(address1, address2, zipCode, city);
        },
        setBillingAddress: function (address1, address2, zipCode, city) {
            billingAddress = new Address(address1, address2, zipCode, city);
        },
        getId: function () {
            return idPurchase;
        },
        getOrders: function () {
            return orders;
        },
        getCustomer: function () {
            return customer;
        },
        toJSON: function () {
            return {
                idPurchase: idPurchase,
                customerOnPurchase: customer.toJSON(),
                deliveryAddress: deliveryAddress.toJSON(),
                billingAddress: billingAddress.toJSON(),
                total: total,
            }
        }
    }
}

module.exports = Purchase;

Based on functions like getId and toJSON, it seems like you would naturally want to have a base class for your models. As such, it would make sense to write your functions in a way that support prototypical inheritance e.g.

Base Model

function Entity(id) {
  this._id = id;
}

Entity.prototype = {
  function getId() {
    return this._id;
  }
  ...
}

module.exports = Entity;

Derived Model

var Entity = require('./entity');
var util = require('util');

function Purchase(customer, orders, ...) {
  Entity.call(this, PurchaseId.purchaseId());
  this._customer = customer;
  this._orders = orders;
  ...
}

util.inherits(Purchase, Entity);

Purchase.prototype.getCustomer = function() {
  return this._customer;
}
Purchase.prototype.getOrders = function() {
  return this._orders;
}
...
module.exports = Purchase;

Usage

var purchase = new Purchase(...);
console.log(purchase.getId()); // Purchase will inherit everything from Entity
console.log(purchase.getCustomer()); // and have it's own functions too

Note - notice in base we set the prototype directly but in the derived we modify it? This is deliberate because in the base class the prototype is currently empty so it's fine to completely overwrite. If we did the same thing on the derived class then we'd lose the base prototype therefore we simply append to the existing prototype.


DRYing up an initialization pattern I maintain a VBA process that prints a large number of PDFs. It was awful when I inherited it and I've been making improvements over time, but I'm a bit stuck on this one. There are at least 4 functions with this exact same code used to check to see if the program has been initialized properly. They could all be called individually, or together depending on circumstances.

I do not want to initialize again if I don't have to. Hence the check in every subroutine, but I just can't figure out how to do it any differently. Thoughts? Is there a design pattern to deal with this kind of thing that I don't know of?

Please try to focus on the pattern and not the supporting intitializer. I know there are a few issues there and intend to deal with them soon. I included it in case it was relevant.

Main code:

Private rootDir As String
Private isInit As Boolean
Private mCycle As String

Public Function printPDF_VoidedNewItems()
    If Not isInit Then
        initialize
    End If
    If rootDir <> "" And mCycle <> "" Then
        If setPrinterToPDF() Then
            ' logic specific to printing this pdf
        End If
    End If
End Function

Public Function printPDF_ComsNotFed()
    If Not isInit Then
        initialize
    End If
    If rootDir <> "" And mCycle <> "" Then
        If setPrinterToPDF() Then
            ' logic specific to printing this pdf
        End If
    End If
End Function

Initializer:

Private Sub initialize()
    rootDir = folderPicker() & "\"
    mCycle = getCycleInput
    isInit = True
End Sub

Private Function folderPicker() As String
'*******************************************
' returns directory path to be printed to
'   does not allow multiple selections,
'   so returning the first item in selected
'   items is sufficient.
'
' returns empty string On Error or if the
'   user cancels
'********************************************
On Error GoTo ErrHandler

    Dim folderChosen As Integer
    Dim fd As Variant
    Set fd = Application.FileDialog(msoFileDialogFolderPicker)
    With fd
        .AllowMultiSelect = False
        .Title = "Choose Directory to Print to"
        .InitialFileName = "\\server\share\"
        .InitialView = msoFileDialogViewSmallIcons
        folderChosen = .Show
        If folderChosen <> -1 Then
            folderPicker = ""
        Else
            folderPicker = .SelectedItems(1)
        End If
    End With
Exit Function

ErrExit:
    folderPicker = ""
    Exit Function
ErrHandler:
    MsgBox "Unexpected Error: " & Err.number & vbCrLf & "Source: " & Err.Source & _
        "Description: " & Err.Description, vbCritical, "ERROR!"
End Function

Private Function getCycleInput() As String
' Displays a form with a combo box to the user.
' Returns an empty string if canceled or errored
'   Otherwise returns 8 character Cycle from dbo.MAM_ISE_SCHEDULE
On Error GoTo ErrHandler

    Dim cpkr As New Form_CyclePicker

    'wait for user input
    Do Until cpkr.Cancel = True Or cpkr.Confirmed = True 'Do Until user clicks "OK" or "Cancel"
        DoEvents 'frees up OS to actually display form and allow user to interact with it
    Loop

    If cpkr.Cancel Or Not cpkr.Confirmed Then ' not confirmed seems redundant, but certainly can't hurt to check.
        ' return
        getCycleInput = ""
        GoTo ExitSub
    Else
        ' return
        getCycleInput = cpkr.CycleNumber
    End If

ExitSub:
    Set cpkr = Nothing
    Exit Function
ErrHandler:
    MsgBox Err.Description, vbCritical, "Runtime Error: " & Err.number
    getCycleInput = ""
    Resume ExitSub
End Function

Down with the underscore
I find the naming of printPdf_xxxx functions confusing, because if that code is/were in a class module they should be private, and the class would say Implements printPdf, where printPdf would be a class with VoidedNewItems and  ComsNotFed members... or it could be that there's a private field called printPdf that's declared WithEvents and the functions are event handlers made public.
Avoid underscores in procedure & function (/method) names, the language has a meaning for identifiers with an underscore.

Initializing once
If you scratch that code and implement it in a class instead, and run initialisation in the... Class_Initialize method, which can be thought of as a parameterless constructor that runs when you create the object. Not too crazy about spawning a UI prompt in that handler though, but then the methods wouldn't have to worry about whether or not Initialize has executed.


limit.js Code Review I've written a quick little helper for event & call debounce/throttling. Since I'm at home and none of my regular code review friends are on-line I figured I'd turn to the great folks here! Would love any feedback you might have.

/**
 * debounce
 * @param {integer} milliseconds This param indicates the number of milliseconds
 *     to wait after the last call before calling the original function .
 * @return {function} This returns a function that when called will wait the
 *     indicated number of milliseconds after the last call before
 *     calling the original function.
 */
Function.prototype.debounce = function (milliseconds) {
    var baseFunction = this,
        timer = null,
        wait = milliseconds;

    return function () {
        var self = this,
            args = arguments;

        function complete() {
            baseFunction.apply(self, args);
            timer = null;
        }

        if (timer) {
            clearTimeout(timer);
        }

        timer = setTimeout(complete, wait);
    };
};

/**
* throttle
* @param {integer} milliseconds This param indicates the number of milliseconds
*     to wait between calls before calling the original function.
* @return {function} This returns a function that when called will wait the
*     indicated number of milliseconds between calls before
*     calling the original function.
*/
Function.prototype.throttle = function (milliseconds) {
    var baseFunction = this,
        lastEventTimestamp = null,
        limit = milliseconds;

    return function () {
        var self = this,
            args = arguments,
            now = Date.now();

        if (!lastEventTimestamp || now - lastEventTimestamp >= limit) {
            lastEventTimestamp = now;
            baseFunction.apply(self, args);
        }
    };
};

To help understand what the point of these helpers are I've prepared this demo: http://jsfiddle.net/zR5jV/1/

GitHub project can be found here: https://github.com/m-gagne/limit.js

The key differences between a throttled function and a debounced function are that the throttled function can return a value because it is called synchronously and the debounced version cannot because it is used asynchronously.

You have lost that on your throttle implementation. I would at least make the following change:

Function.prototype.throttle = function (milliseconds) {
    var baseFunction = this,
        lastEventTimestamp = null,
        limit = milliseconds,
        lastresult;

    return function () {
        var self = this,
            args = arguments,
            now = Date.now();

        if (!lastEventTimestamp || now - lastEventTimestamp >= limit) {
            lastEventTimestamp = now;
            lastresult = baseFunction.apply(self, args);
        }
        return lastresult;
    };
};


Finding N partitions of number M with min and max partitions sizes This code was created for a game project in which 10 houses need to be populated with 16 people. I wanted the distribution to be random, with no more than 3 and no less than 1 person per house and no remaining people or houses.

JSbin link

function getRandomPartitionsFromNumber(number, parts, min, max) {
  var result = [];

  for (var i = 0; i < parts; i++) {
    var part = randomIntFromInterval(min, max);
    result.push(part);
  }
  var sum = result.reduce(add, 0);

  // If the sum is greater than the number, we need to iterate through
  // the result and remove some numbers until it's equal
  while (sum > number) {
    for (i = 0; i < result.length; i++) {
      if (result[i] > min) {
        result[i]--;
        sum = result.reduce(add, 0);
        if (sum === number) {
          break;
        }
      }
    }
    sum = result.reduce(add, 0);
  }

  while (sum < number) {
    for (i = 0; i < result.length; i++) {
      if (result[i] < max) {
        result[i]++;
        sum = result.reduce(add, 0);
        if (sum === number) {
          break;
        }
      }
    }
  }

  return result;
}

function randomIntFromInterval(min, max) {
    return Math.floor(Math.random() * (max - min + 1) + min);
}

function randomFromArray(array) {
  return array[randomIntFromInterval(0, array.length - 1)];
}

function add(a, b) {
  return a + b;
}

console.log(getRandomPartitionsFromNumber(16, 10, 1, 3));

The way you have done your algorithm, you could just apply the same operation to your sum variable, rather than reducing the entire array.

  while (sum > number) {
    for (i = 0; i < result.length; i++) {
      if (result[i] > min) {
        result[i]--;
        sum--
        if (sum === number) {
          break;
        }
      }
    }
    sum = result.reduce(add, 0);
  }

Additionally, the last sum = result.reduce is not needed here, as any change in the result array is subsequently reflected in the sum variable a row lower.

  while (sum > number) {
    for (i = 0; i < result.length; i++) {
      if (result[i] > min) {
        result[i]--;
        sum--
        if (sum === number) {
          break;
        }
      }
    }
  }

Lastly, keep in mind if you were to feed your code an impossible situation ("stuff 5 people into 1 house where each house can only contain 3 people at most"), you will get an infinite loop. It's up to you to decide how to handle this.


Proper way of deallocating memory in SLL? So I'm making my own singly linked list class, with methods that you'd expect such as remove() and insert(). I don't use pointers that much (almost never), so I thought by making a SLL class it would be good practice for me. But I'm not sure if I'm deallocating my memory correctly.
Here's the structure of my SLL:
class singly_linked_list
{
private:
    struct Node
    {
        Node* next;
        int value;
    };

private:
    int _length;
    Node* head;

public:
    ~singly_linked_list();

public:
    void remove(int index);
    void remove_front();
    void remove_back();
};

I'm most concerned about the destructor, and the three remove methods, and those are the ones deallocating memory, and I'm not sure if I'm doing it correctly.
Here's their definitions:
class singly_linked_list
{
private:
    struct Node
    {
        Node* next;
        int value;
    };

private:
    int _length;
    Node* head;

public:
    ~singly_linked_list()
    {
        Node* node_ptr = head;
        for (int a = 0; a < _length; ++a)
        {
            node_ptr = node_ptr->next;

            delete head;
            head = node_ptr;
        }
    }

public:
    void remove(int index)
    {
        if (index < 0 || index >= _length)
            throw "index is out of range";

        if (index == 0)
        {
            this->remove_front();
            return;
        }

        Node* node_ptr = head;
        for (int a = 0; a < index - 1; ++a)
            head = head->next;

        Node* temp = head->next;
        head->next = temp->next;
        delete temp;

        _length -= 1;
    }

    void remove_front()
    {
        if (_length == 1)
        {
            // should I delete head instead?
            head = nullptr; // delete head;
            _length = 0;

            return;
        }

        Node* temp = head;
        head = head->next;
        delete temp;

        _length -= 1;
    }

    void remove_back()
    {
        if (_length == 1)
        {
            head = nullptr;
            _length = 0;

            return;
        }

        Node* node_ptr = head;
        for (int a = 0; a < _length - 2; ++a)
            node_ptr = node_ptr->next;

        Node* temp = node_ptr->next;
        node_ptr->next = nullptr;
        delete temp;

        _length -= 1;
    }
};

I checked a few other questions including this one, but it didn't go too much in depth on memory management.
So a couple questions:

Am I causing any memory leaks, and is there a better, safer way to do things?
In remove_front(), should I set head to nullptr, or should I delete it instead?

Thanks

Answers to your questions

Am I causing any memory leaks, and is there a better, safer way to do things?

Yes, see for example your second question. A safer way is to use something like std::unique_ptr to manage pointers for you.

In remove_front(), should I set head to nullptr, or should I delete it instead?

You should delete it of course. If you just set head to nullptr, the memory for the head node is still allocated.
Avoid using names that start with an underscore
Names that start with underscores are reserved for the standard library. There are some more specific rules, but in general it is best to avoid them completely. You can safely append an underscore to the end of a name though.
On the other hand, while you have _length, head is written without an underscore. If you are not consistent in marking private variables with an underscore, there is not much point to it at all.
Flawed logic in remove(int index)
There is an issue with the for-loop in remove(int index): you are moving the head pointer instead of node_ptr. In fact you are not using node_ptr at all after initializing it. The compiler should have given you a warning about this (be sure to enable compiler warnings and fix them).
To make this work correctly, even when removing the head node, you should use an extra layer of redirection: you want a pointer to the pointer you need to update, like so:
Node **node_ptr = &head;           // node_ptr is pointing to the variable `head`
for (int a = 0; a < index - 1; ++a)
    node_ptr = &(*node_ptr)->next; // node_ptr is updated to point to some node's `next` variable

Node* temp = *node_ptr;            // get the actual Node pointer to
*node_ptr = (*node_ptr)->next;     // update `head` or a `next` to skip one Node
delete temp;                       // delete the target Node

Once you grasp this you've become a two-star programmer!
Unnecessary code duplication
You have three different functions for removing an element, but they all do mostly the same. In fact, the first one, remove(int index), does everything you need. You don't need to treat index == 0 as a special case, the code will work correctly anyway. You can make remove_front() and remove_back() call the generic remove() function:
void remove(int index)
{
    if (index < 0 || index >= _length)
        throw "index is out of range";

    Node **node_ptr = &head;
    for (int a = 0; a < index - 1; ++a)
        node_ptr = &(*node_ptr)->next;

    Node* temp = *node_ptr;
    *node_ptr = (*node_ptr)->next;
    delete temp;

    _length -= 1;
}

void remove_front()
{
    remove(0);
}

void remove_back()
{
    remove(_length - 1);
}


Small script to verify hash of a downloaded file You know how whenever you download a file you should really compare the hash of the download to the one provided on the website? This makes absolute sense, but it's a pain to do it letter for letter, digit for digit. So, I wrote this little script to take care of the job. Any comments are welcome.

#!/bin/bash

# hash_checker - program to verify a downloaded file

error_exit()
{
    echo "$1" 1>&2
    exit 1
}

usage="usage: hash_checker downloaded_file hash_provided -a algorithm"
downloaded_file=
hash_given=
hash_calc=
algo="sha256"

# check if file and hash were provided
if [ $# -lt 2 ]; then
    error_exit "$usage"
fi

# parsing the provided hash and file
downloaded_file="$1"
hash_given="$2"

# parsing the algorithm, if provided
if [ "$3" != "" ]; then
    if [ "$3" = "-a" ]; then
        algo="$4"
    else
        error_exit "$usage"
    fi
fi

# check if input is a valid file
if [ ! -f "$downloaded_file" ]; then
    error_exit "Invalid file! Aborting."
fi

# calculate the hash for the file
hash_calc="$($algo'sum' $downloaded_file)"
hash_array=($hash_calc)
hash_calc=${hash_array[0]}

# compare the calculated hash to the provided one
if [ "$hash_calc" = "$hash_given" ]; then
    echo "The hashes match. File seems to be valid."
else
    echo "The hashes do not match. File does not seem to be valid."
fi

Notes:

I'd use getopts for arg parsing -- lots of examples on stackoverflow about how to use it.
always quote your variables
you should validate the algorithm:

sum_exe="${algo}sum"
if ! type -P "$sum_exe" >/dev/null; then
    error_exit "'$algo' is an unknown checksum algorithm"
fi

have the checksum program read from stdin, then you don't have to do your incorrect unsafe word parsing since the program will not print a filename

hash_calc=$( "$sum_exe" < "$downloaded_file" )

As the above doesn't work, let's use read from a process substitution

read -r hash_calc _ < <("$sum_exe" < "$downloaded_file")


Repetitive validation of Console inputs I have  this code that asks the user for inputs and validates the inputs if they are Integers (no other inputs are allowed). I'd like to ask for some help in terms of elegance of the code or performance, or just the overall code.

Also, I have found myself repeating these lines of code

Console.Write("Enter weight of parcel:  ");
string input = Console.ReadLine();

and I think I really need to refactor the code because of this, but can't think of a  simple and straightforward way to refactor it.

Here is my code below:

class Program
{
    private static int _depth = 0;
    private static int _height = 0;
    private static int _weight = 0;
    private static int _width = 0;
    private static bool _isInvalidInput = true;
    static void Main(string[] args)
    {
        GetWeightInput();
        GetHeightInput();
        GetWidthInput();
        GetDepthInput();

        Console.ReadLine();
    }
    private static void GetWeightInput()
    {
        while(_isInvalidInput)
        {
            Console.Write("Enter weight of parcel:  ");
            string input = Console.ReadLine();

            #region User Weight Input Validation
            if (int.TryParse(input, out _weight))
            {
                _isInvalidInput = false;
                //TODO: Add More Validations of User Input
            }
            else
            {
                Console.WriteLine("***Only Integer Inputs Are Allowed***");
                _isInvalidInput = true;
            }
            #endregion User Weight Input Validation
        }

        _isInvalidInput = true;//reset value to true
    }
    private static void GetHeightInput()
    {
        while (_isInvalidInput)
        {
            Console.Write("Enter weight of parcel:  ");
            string input = Console.ReadLine();

            #region User Weight Input Validation
            if (int.TryParse(input, out _height))
            {
                _isInvalidInput = false;
                //TODO: Add More Validations of User Input
            }
            else
            {
                Console.WriteLine("***Only Integer Inputs Are Allowed***");
                _isInvalidInput = true;
            }
            #endregion User Weight Input Validation
        }

        _isInvalidInput = true;//reset value to true
    }
    private static void GetWidthInput()
    {
        while (_isInvalidInput)
        {
            Console.Write("Enter weight of parcel:  ");
            string input = Console.ReadLine();

            #region User Weight Input Validation
            if (int.TryParse(input, out _width))
            {
                _isInvalidInput = false;
                //TODO: Add More Validations of User Input
            }
            else
            {
                Console.WriteLine("***Only Integer Inputs Are Allowed***");
                _isInvalidInput = true;
            }
            #endregion User Weight Input Validation
        }

        _isInvalidInput = true;//reset value to true
    }
    private static void GetDepthInput()
    {
        while (_isInvalidInput)
        {
            Console.Write("Enter weight of parcel:  ");
            string input = Console.ReadLine();

            #region User Weight Input Validation
            if (int.TryParse(input, out _width))
            {
                _isInvalidInput = false;
                //TODO: Add More Validations of User Input
            }
            else
            {
                Console.WriteLine("***Only Integer Inputs Are Allowed***");
                _isInvalidInput = true;
            }
            #endregion User Weight Input Validation
        }

        _isInvalidInput = true;//reset value to true
    }
}

As you suggested, you have a lot of redundant code. And rightfully so you want to adhere to the DRY principle.

All these input methods have the same pattern internally..

static void Main(string[] args)
{
    GetWeightInput();
    GetHeightInput();
    GetWidthInput();
    GetDepthInput();

    Console.ReadLine();
}

Road to DRY

..ideally you would like to be able to call them like this:

static void Main(string[] args)
{
    var weight = AskInteger("Enter weight of parcel: ");
    var height = AskInteger("Enter height of parcel: ");
    var width = AskInteger("Enter width of parcel: ");
    var depth = AskInteger("Enter depth of parcel: ");

    Console.WriteLine("press any key to terminate the application..");
    Console.ReadKey(true);
}

Advanced road to DRY

Or if you would provide a complex object Request with 4 properties and a lambda to provide both a message to the user as a setter expression to materialise the request:

static void Main(string[] args)
{
    var request = new Request();
    AskInteger(request, x => x.Weight);
    AskInteger(request, x => x.Height);
    AskInteger(request, x => x.Width);
    AskInteger(request, x => x.Depth);

    Console.WriteLine("press any key to terminate the application..");
    Console.ReadKey(true);
}

How you would implement AskInteger is a challenge I leave to you.

Misc

Don't pollute your code with regions that have no added value.  #region User Weight Input Validation
Try to avoid meaningless comments _isInvalidInput = true;//reset value to true


Pandas nested loop which checks if values from one series are in another series I have two different series in pandas that I have created a nested for loop which checks if the values of the first series are in the other series. But this is time consuming in pandas and I cannot work out how to change it to a pandas method. I thought to use the apply function but it did not work with method chaining. My original nested for loops look like so and they work;

for x in df_one['ser_one']:
    print(x)
    for y in df_two['ser_two']:
        if 'MBTS' not in y and x in y:
            if 'L' in y:
                print(y)

Is there a way to make this less time consuming?

Here is what I attempted using apply methods;

df_two['ser_two'].apply(lambda x: x if 'MBTS' not in df_one['ser_one'].apply(lambda y:y) and x in df_one['ser_one'].apply(lambda y:y))

Example input:

df_one.head()
Out[136]:
   type   ser_one
0  MBTS  VUMX1234
1  MBTS  VUMX6436
2  MBTS  VUMX5745
3  MBTS  VUMX5802
4  MBTS  VUMX8091

df_two.head()
Out[137]:
     ser_two
0  VUMX8091
1  VUMX8091L
2  VUMX1234
3  VUMX1234L
4  VUMX5838

Disclaimer, I am not the best at pandas, and I'm absolutely sure there is a far more readable way to accomplish this, but the following will rid you of your for loop and nested if statements, which are slower than vectorized numpy/pandas operations.

Your filter if 'MBTS' not in y won't work the way you think it will, at least, given the limited sample input, as y is a Series made from the column ser_one, not type. Let's assume that's an easy fix so in pseudocode it should be something like:

for x in df_one.ser_one:
    for y in df_two: # iterate through the rows so you get both columns
        if 'MBTS' not in y.type and x in y.ser_two:
            if 'L' not in y.ser_two:
                print(y)

This is a bit clunky, and pandas is great for vectorizing these sorts of operations, so let's filter it down to just Series operations. I'm working with a small part of your dataframes, so as a sanity check, they look like

df_one
     ser_one  type
0   VUMX1234  MBTS
1   VUMX6436  MBTS
2   VUMX5745  MBTS
3   VUMX5802  MBTS
4   VUMX8091  MBTS
5   VUMX1234  XXXX
6  VUMX1234L  XXXX

df_two
     ser_two
0   VUMX8091
1  VUMX8091L
2   VUMX1234
3  VUMX1234L
4   VUMX5838

I added a few entries that were non-MBTS to fit your problem.

The first bit, you want to find where 'MBTS' is not in df_one.type, but we want to filter the entire dataframe for that. df.loc will give you the rows that pass a given filter:

df_one.loc[df_one['type'] == 'MBTS']
    ser_one  type
0  VUMX1234  MBTS
1  VUMX6436  MBTS
2  VUMX5745  MBTS
3  VUMX5802  MBTS
4  VUMX8091  MBTS

# or
df_one.loc[df_one['type'] != 'MBTS']
     ser_one  type
5   VUMX1234  XXXX
6  VUMX1234L  XXXX

Now you can check if the results of ser_one are contained within ser_two, since the output of that previous check is a Series, like so:

df_one.loc[df_one['type'] != 'MBTS']['ser_one'].isin(df_two['ser_two'])
5    True
6    True

Just get the .loc back from that, and you should be left with two records in this example:

df_one.loc[df_one.loc[df_one['type'] != 'MBTS']['ser_one'].isin(df_two['ser_two']).index]
     ser_one  type
5   VUMX1234  XXXX
6  VUMX1234L  XXXX

It might be a bit easier to do the filtering against any ser_one that contains 'L' ahead of time:

df_one[~df_one['ser_one'].str.contains("L")]
    ser_one  type
0  VUMX1234  MBTS
1  VUMX6436  MBTS
2  VUMX5745  MBTS
3  VUMX5802  MBTS
4  VUMX8091  MBTS
5  VUMX1234  XXXX

Now, combining all of that into one big gigantic horrible expression

df_one.loc[df_one[~df_one['ser_one'].str.contains("L")].loc[df_one['type'] != 'MBTS']['ser_one'].isin(df_two['ser_two']).index]

    ser_one  type
5  VUMX1234  XXXX

The outer loc will take an array of index values as returned by the .index call near the end of the expression. The rest is just chained filters which are operations in native pandas, implemented in C and fast.


Method that return related entities using OData There is method GetFull that should use OData return related entities.

When I call GetFull(12, "entity1,entity2") then I will get to current object with id = 12 and related entity1 and entity2 (http://.../api/City/12?$expand=Region)

When I call GetFull(12, "0") then I want to automatical find and get all related entities from current object (http://.../api/City/12?$expand=Region,Country). For that I'm using reflections.

How can I improve this method?
It's working but I'm sure that I implemented it not properly.

P.S : It was done  for user-friendly link
before:http://.../api/City/12?$expand=Region,Country
now:http://.../api/City/12/GetFull or http://.../api/City/12/GetFull/Region

public virtual HttpResponseMessage GetFull(int id, string entities)
{
    string propertyList = string.Empty;

    if (!entities.Equals("0"))
    {
        propertyList = entities;
    }
    else
    {
        foreach (var prop in typeof(T).GetProperties().Where(p => p.GetGetMethod().IsVirtual))
        {
            if (prop.PropertyType.IsClass && !prop.PropertyType.FullName.StartsWith("System."))
            {
                propertyList += prop.Name + ",";
            }
        }

        propertyList = propertyList.Remove(propertyList.Length - 1);
    }

    string entityName = typeof(T).Name;
    var baseUrl = Request.RequestUri.GetLeftPart(UriPartial.Authority);
    var response = Request.CreateResponse(HttpStatusCode.Found);

    StringBuilder sb = new StringBuilder();
    sb.Append(baseUrl);
    sb.Append("/api/");
    sb.Append(entityName);
    sb.Append("/");
    sb.Append(id);
    sb.Append("?$expand=");
    sb.Append(propertyList);

    response.Headers.Location = new Uri(sb.ToString());
    return response;
}

Sorry for my English and if I have to add more informations, please let me know. Thanks

GetFull() is a methodname which doesn't tell any caller anything about what the method will do. Please choose always names which are as descriptive as possible.
this loop

foreach (var prop in typeof(T).GetProperties().Where(p => p.GetGetMethod().IsVirtual))
{
    if (prop.PropertyType.IsClass && !prop.PropertyType.FullName.StartsWith("System."))
    {
        propertyList += prop.Name + ",";
    }
}

propertyList = propertyList.Remove(propertyList.Length - 1);

could be improved by either using a StringBuilder or by using a ILIst<string> together with string.Join(). This should maybe be extracted to a separate method.
if (!entities.Equals("0")) you are using entities without doing any null check, so better use if (!"0".Equals(entities)). I usually like positive checks more so I would like to suggest to revert this if..else.
by default the newed StringBuilder has a capacity of 16 which is doubled if the added value won't fit anymore. So better initialize it with a higher number by using the overloaded constructor which takes the initial size as an argument.
In addition you can take advantage of the fluent methods of the StringBuilder which return the StringBuilder itself.

All mentioned points (except for the method name) implemented leads to

public virtual HttpResponseMessage GetFull(int id, string entities)
{
    string propertyList = string.Empty;

    if ("0".Equals(entities))
    {
        propertyList = GetJoinedPropertyList();
    }
    else
    {
        propertyList = entities;
    }

    string entityName = typeof(T).Name;
    var baseUrl = Request.RequestUri.GetLeftPart(UriPartial.Authority);
    var response = Request.CreateResponse(HttpStatusCode.Found);

    StringBuilder sb = new StringBuilder(1024);

    sb.Append(baseUrl)
      .Append("/api/")
      .Append(entityName)
      .Append("/")
      .Append(id)
      .Append("?$expand=")
      .Append(propertyList);

    response.Headers.Location = new Uri(sb.ToString());
    return response;
}
private string GetJoinedPropertyList()
{
    IList<string> propertyNames = new List<string>();

    foreach (var prop in typeof(T).GetProperties().Where(p => p.GetGetMethod().IsVirtual))
    {
        if (prop.PropertyType.IsClass && !prop.PropertyType.FullName.StartsWith("System."))
        {
            propertyNames.Add(prop.Name);
        }
    }
    return string.Join(",", propertyNames);
}


Sorting an ArrayList of Vehicles using quick sort For an assignment I have to sort an ArrayList of Vehicle objects using the quick sort method. I have come up with two implementations for this, which would be the best to use? Is method 1 considered quick sort since it splits the array?

Method 1

protected ArrayList<Vehicle> quickSort(ArrayList<Vehicle> list)
{
    if (list.size() <= 1)
        return list; // Already sorted

    ArrayList<Vehicle> sorted = new ArrayList<Vehicle>();
    ArrayList<Vehicle> lesser = new ArrayList<Vehicle>();
    ArrayList<Vehicle> greater = new ArrayList<Vehicle>();
    Vehicle pivot = list.get(list.size()-1); // Use last Vehicle as pivot
    for (int i = 0; i < list.size()-1; i++)
    {
        //int order = list.get(i).compareTo(pivot);
        if (list.get(i).compareTo(pivot) < 0)
            lesser.add(list.get(i));
        else
            greater.add(list.get(i));
    }

    lesser = quickSort(lesser);
    greater = quickSort(greater);

    lesser.add(pivot);
    lesser.addAll(greater);
    sorted = lesser;

    return sorted;
}

Method 2

protected ArrayList<Vehicle> quickSort(ArrayList<Vehicle> list, int a, int b)
{
    if (a >= b)
        return list;

    Vehicle pivot = list.get(b);

    int left = a;
    int right = b;

    while (left < right)
    {
        while(list.get(left).compareTo(pivot) < 0)
            left++;

        while(list.get(right).compareTo(pivot) > 0)
            right--;

        if (right > left);
        {
            Collections.swap(list, left, right);
        }
    }

    quickSort(list, a, right-1);
    quickSort(list, right+1, b);

    return list;
}

Now the problem with Method 2 is that my protected quickSort is called from

public ArrayList<Vehicle> quickSort()
{
    return quickSort(vehicleList);
}

which I'm not allowed to modify, so I'd need to overload the method again and call my version with the two extra integer parameters. Would this just be messy doing another overload?

In method 1 the sorted list you create here:

ArrayList<Vehicle> sorted = new ArrayList<Vehicle>();

is never used and overwritten here:

sorted = lesser;

Just drop the variable sorted and return lesser.

There is no reason to limit the method to ArrayLists or even Vehicles. Just have Vehicle implement the standard Comparableinterface and let your method have the signature:

protected <T extends Comparable<T>> List<T> quickSort(List<T> list) {
  // ...
}


Laravel routes.php optimization My goals are:

Have 2 main domains .com and .dev for production and local installations
Have 2 main subdomains. One for the site and one of the application
Site must be public and application always to be secured.

My take on the above:

<?php
$approutes =function (){

    Route::group(['middleware' => 'auth'], function(){
        Route::get('/', 'AdminController@index');

        Route::get('profile/{user}','ProfileController@index');

        Route::post('profile/{user}/update',array('as' => 'profile.update','uses' => 'ProfileController@update'));

        Route::resource('posts','PostsController');

        Route::resource('tags','TagsController');

        Route::resource('health','HealthController');

        Route::resource('health-categories','HealthCategoriesController');
    });

    Route::controllers([
        'auth' => 'Auth\AuthController',
        'password' => 'Auth\PasswordController'
    ]);
};

$siteroutes =function (){
    Route::get('/', 'SiteController@index');
};

Route::group(['domain' => 'my.xxxx.com'],$approutes);
Route::group(['domain' => 'my.xxxx.dev'],$approutes);

Route::group(['domain' => 'www.xxxx.com'],$siteroutes);
Route::group(['domain' => 'www.xxxx.dev'],$siteroutes);

First of all, I really don't like to mix up routes of two different domains/apps in the same routes.php or web.php. So, I prefer and would suggest you as well to follow the below approach, which seems much cleaner and much authentic. :)

Also, I believe www.abc.com or www.abc.dev are not required to be mentioned here... Laravel is intelligent enough to work this through for routes without subdomains... (especially when it is installed in the root directory of that domain).... If required... you can still follow the procedure I used in mapAppRoutes() below.

Go to App\Providers\RouteServiceProvider

Now in the map() function... Add these method calls...

public function map(Router $router)
{
  $this->mapWebRoutes($router);     //You can comment out this if you want... Completely your choice

  $this->mapSiteRoutes($router);

  $this->mapAppRoutes($router);

  //
}

Now add these functions at the end...

protected function mapSiteRoutes(Router $router)
{
  $router->group([
    'namespace' => $this->namespace . '\Site', 'middleware' => 'web',
  ], function ($router) {
    require app_path('Http/routes.site.php');
  });
}

protected function mapAppRoutes(Router $router)
{
  $domains = ['my.xxxxxx.com', 'my.xxxxxx.dev'];    //Sub-domains you support

  foreach($domains as $domain) {
    $router->group([
      'namespace' => $this->namespace . '\MyApp',
      'middleware' => ['web', 'auth'],
      'domain' => $domain
    ], function ($router) {
      require app_path('Http/routes.myapp.php');
    });
  }
}

Now create, app/Http/routes.site.php and app/Http/routes.myapp.php

In routes.site.php, add this...

Route::get('/', 'SiteController@index');

In routes.myapp.php, add these....

Route::get('/', 'AdminController@index');

Route::get('profile/{user}','ProfileController@index');

Route::post('profile/{user}/update',array('as' => 'profile.update','uses' => 'ProfileController@update'));

Route::resource('posts','PostsController');

Route::resource('tags','TagsController');

Route::resource('health','HealthController');

Route::resource('health-categories','HealthCategoriesController');

You're good to go... :)

Please note that I have changed the namespace as well for the routes, so that you can make separate folders for your home site and subdomain sites and easily differentiate between the two... even by looking at the tree-view. This I believe is a much better way of structuring and dividing your code... Makes things simpler to read and navigate :)


Blog/Forum implementation I've started learning PHP for university-related projects today. I tried implementing a basic blog/forum. Here's the full source code.

It has the following features/design-decisions:

There are posts, which have the following fields: title, author, date and contents.
Posts have comments, which have the following fields: author, date, contents.
There is a basic login system: users can register/login with an username and password.
JSON files are used for data storage instead of a MySQL database.

There is a JSON file for posts, posts.json, which contains posts and comments.
There is a JSON file for users, which contains usernames and password hashes.

I'm looking for a full code review: I've only read the PHP manual today and while I understand most of its features I have no idea what the best practices are and how things should be implemented.

I will list below some of my main concerns with the code/design.

I have a core.php file which is included with require in every other PHP file.

It links the stylesheet at the beginning (and it could also import JS files):

    <link rel="stylesheet" type="text/css" href="./CSS/style.css">

Is this the correct way of adding the same  to every file?

Every time I want to access the user database or the posts database, I call these functions:

function getUsersDB() { return new UsersDB(); }
function getPostsDB() { return new PostsDB(); }

However, creating a new UsersDB or PostsDB instance forces a read from the .json file. I need to access these databases in multiple PHP files.

Is there a way to "cache" these UsersDB and PostsDB instances? Something similar to C++'s static, that can be shared between all PHP scripts?

When I have to generate something between HTML tags, I just call print like this:

print("");
print($mPost["author"]);
print("");

Is there an easier/better way of generating open/close tags? Should I create a function that takes a lambda like:

function doBetweenStrong($lambda)
{
    print("");
    // execute $lambda here
    print("");
}

Every page of my website has the same menu and "login status" bar. I simply call Builder::printCommon(); at the beginning of every page. Is this the best way to do it?

I keep track of the current logged-in user in $_SESSION.

class Credentials
{
    static public function isLoggedIn()
    {
        if(!isset($_SESSION["currentUser"])) return false;
        if($_SESSION["currentUser"] == "") return false;
        return true;
    }

    static public function login($mUser)
    {
        $_SESSION["currentUser"] = $mUser;
    }

    static public function logout()
    {
        unset($_SESSION["currentUser"]);
    }

    static public function getCurrentUserName()
    {
        return $_SESSION["currentUser"];
    }
}

Is this safe? What is the correct way to keep track of the currently logged-in user?

I find it hard to show the user error/success messages (for example, after registration, or after login, or after posting something). The current solution is creating a new page like registerSuccess.php or registerFailure.php.

Should I create a general message.php and pass the desired message to it using $_SESSION?

Paths. Is there a reliable way to get the server's root folder?

I'm currently using ./, ../ and ../../ to get the files I want from scripts. What if I want to move a script in the future? I would have to change all paths manually.

What if I include a script into another? Wouldn't the paths be resolved incorrectly?

First of all, it's good that you try to implement something like a blog/forum on your own. It will teach you a lot of good/bad practices. In one comment you said, that you are trying to understand how these frameworks could be impemented, so it would be very good, if you take a look at them. Check out their APIs and see what they did.

Im used to Symfony (http://symfony.com/) so my advice will mostly use their architecture as an example. However, be sure to remember that there are many ways to success and the things I describe are my current point of view.

Your major problems (maybe not now, but in a few days/weeks/months) are the structure of your project, the structure of your source code and the mixing of php/html.

Project Structure

Currently, you have a few php files in your root directory and a few php files in your PHP directory. On a first glance, it seems, that the PHP directory contains your framework and the root folder contains the views, which use this framework. If that's the case, make it obvious.

Advice #1: Give your framework a name and rename the folder accordingly.

That way, you will have an easier time to find the place you are looking for. Which leads to the next problem: Your core.php contains multiple class definitions. If you continue doing this, your file will grow exponentially and it will take you a long time to find the line you want to modify.

Advice #2: Put each php class in a separate file.

The next step is to use namespaces to further structure your classes and files. It might be more difficult to begin with, but will help you in the long run. Additionally you might want to look into autoload. Used correctly, this should solve your troubles with include and require.

Advice #3: Use Namespaces and autoload

Another possible improvement are the files in your root directory. Currently you probably access each file directly via www.whatev.er/viewPost.php or www.whatev.er/createPost.php. I highly recommend to take a look at something like a controller/routing mechanism and only use ONE file (index.php) to access your website. Take a look at .htaccess and URL Rewriting. This is actually a pretty powerful and extensive technique. So just a short explanation: Every (nearly every) call to your website is redirected to your index.php. So www.whatev.er/blub/bla/1234 or www.whatev.er/post/create all call your index.php. In the index file, you can parse the requested uri and call the appropriate actions.

Advice #4: Use .htaccess and URL Rewriting and only one file as an entry point to your website.

Advice #5: Use some kind of controller/routing mechanism to handle actions depending on your route.

Source Code Structure

As already mentioned, only define one class per file. I like it, that you encapsulate the fields of your classes. However, you have many static functions, which basically degrades your classes to some kind of namespaces. There is nothing wrong with static functions, but be careful not to overuse them.

Advice #6: Don't overuse static functions (and statics in general)

As you mentioned, getUsersDB() and getPostsDB() is suboptimal. You have pretty much three options:

Use global objects
Pass a reference where it is needed
Use dependency injection

The use of global objects is generally discouraged, but migh be okay given your current experience. Another (slightly) better option is to instantiate the database once and pass it to the method that needs the database. The imho best option is to use dependency injection. However, this is pretty advanced stuff and something you want to look into in the future.

Advice #7: Use dependency injection if possible. Globals might be okay for now, but lead to trouble in the future.

You say, that you have posts and comments. However, in your code you only deal with assoc arrays. Why don't you create classes for Post and Comment? This is the perfect place to use a class.

Advice #8: Use classes for entites (Post/Comment) and not assoc arrays.

If you use classes for Post/Comment, you might wonder how to write them to json. PHPs jsonSerialize will help you there. However, you probably have to convert json -> Post/Comment yourself.

Advice #9: Use jsonSerialize or something similar to serialize objects to json.

Mixing of PHP/HTML

This is probably the problem, that will annoy you the most in the future. As it will make dealing with outputting stuff way more complicated than it needs to be. I highly encourage you to use some kind of templating system to render templates and display html. Don't ever output or print html code in a php file. Take a look at twig (used in eg. Symfony). Something like that will extremely improve your codebase. Your problem with printing will vanish and you can use common parts of html code without any problems.

Advice #10: Use some kind of templating mechanism to output HTML.


Finding a string in a text file and reporting full file name and size I have the following script to find files containing a certain string and then report on the location of the file along with the size in bytes.

It works, but I seem to have made heavy weather of finding the string which involves a fair bit of tidying up afterwards to produce some clean output.

Any idea how I could make this more concise?

# Ignore errors
$ErrorActionPreference= 'silentlycontinue'

# Grab user options
$ext = Read-Host -Prompt        "Enter extension   "
$huntString = Read-Host -Prompt "Enter hunt string "

# Find text files (.log, .txt etc) containing the hunt string
$entries = gci -recurse -include *.$ext -ErrorAction SilentlyContinue | select fullname, length | sort -property length

echo ''
foreach ($e in $entries)
{
    # Find files containing the hunt string
    $foundFile = (gci $e.fullname | select-string $huntString | measure-object | findstr Count)

    # Output hit count along with size and name
    $rawOutput = $foundFile.ToString() + $e.Length.ToString().PadRight(10,[char]32) + "`t" + $e.fullname

    # Only output entries with a hit count
    $cleanOutput = echo $rawOutput | select-string ": 1"

    # Remove hit count
    $finalOutput = $cleanOutput -replace "Count","" -replace ": ",""

    # Trim and output
    echo $finalOutput.TrimStart()
}

Using findstr in PowerShell is superfluous and
not very powershell'ish which is about objects and pipes.

You can directly pipe the raw output of Get-ChildItem to Select-String and parse the resulting object for the information you require.

As the size of the file isn't contained in the properties sls returns:

Context      Property
Filename     Property
IgnoreCase   Property
Line         Property
LineNumber   Property
Matches      Property
Path         Property
Pattern      Property

You've to append it, either with a calculated property

# Grab user options
$ext = Read-Host -Prompt        "Enter extension   "
$huntString = Read-Host -Prompt "Enter hunt string "

Get-ChildItem *.$ext -Recurse | Select-String $huntString -List |
    Select-Object @{Label='Size';Expression={(Get-Item $_.Path).Length}},Path

or iterate the output and build a [PSCustomObject]:

Get-ChildItem *.$ext -Recurse | Select-String $huntString -List |
    ForEach-Object {
        [PSCustomObject]@{
            Path = $_.Path
            Size = (Get-Item $_.path).Length
        }
    }

The objectsoutput will be the very same:

> Q:\Test\2018\12\17\CR_209811.ps1
Enter extension   : ps1
Enter hunt string : ::Now
Size Path
---- ----
 878 Q:\Test\2018\09\18\SO_52381514.ps1
 677 Q:\Test\2018\11\16\SO_53336923.ps1
 770 Q:\Test\2018\11\19\SO_53381881.ps1
1141 Q:\Test\2018\12\17\CR_209811.ps1
1259 Q:\Test\2018\12\17\SU_1385185.ps1


Rspec controller tests I wanted to keep my controller tests DRY, readable, and make writing new tests very simple so that it encourages good practices like one assertion per test (if you have to redo a lot of work over and over, chances are you're just gonna shove more assertions into the same test to avoid the extra work).

Here's the strategy I'm using:

RSpec.describe FireController do
    let(:do_request) { post :create, request_body }
    let(:request_body) { {} }

    describe '#create' do
       context 'when we have firewood' do
           let(:request_body) { { firewood: true } }
           it 'creates fire' do
               do_request
               expect(response.body).to eql('fire!!!')
           end
       end
    end
end

I would like some opinions on potential downsides to a strategy like this.

Usually let is used to create data or functionality that is the same for each individual spec. In this example, I don't see much difference between your code and:

it 'creates fire' do
  params = { firewood: true }
  post :create, params
  expect(response.body).to eql('fire!!!')
end

If you need to pass { firewood: true } to multiple specs, then using let to define the controller params makes sense. Furthermore, a before_each is more declarative for executing behavior. Use let to mock up data, but use before_each to execute the same behavior or methods for each spec:

RSpec.describe FireController do
  describe '#create' do
    context 'when we have firewood' do
      let :params, { { firewood: true } }

      before_each do
        post :create, params
      end

      it 'creates fire' do
        expect(response.body).to eql('fire!!!')
      end

      it 'is really really on fire' do
        expect(...).to ...
      end

      it 'does something else as well' do
        expect(...).to ...
      end
    end
  end
end

This way each spec is literally reduced to a single expect.

@jtmarmon said:

  the reason I didn't run the request in before_each is because some specs need to check if a method get called. those expectations need to be set before the request is made

In this case I would either:

Just call post :create, params in each spec, or
Group those specs inside additional contexts that contain their own before_eachs setting up the method call expectations

You aren't gaining clarity, or really reducing code by encapsulating post :create, params with a let. Every time I see a let I think it is setting up data, rather than executing behavior.


Recursively searching the Windows Registry This function is part of a the Registry class that can be found on Google Drive. The class was originally written by Steve McMahon for VB6, but I ported it to VBA and added the function below.

I would like to know if I have made any mistakes in my logic and (if possible) how I could change this to keep searching after it has found the first instance.

Public Function findSectionKey(sectToFind As String, Optional sectToLookIn As String = vbNullString) As String
'*****************************************************************************
' Christopher J. McClellan 4-16-14
'
' Returns:
'   Full section key as string
'       ex: "software\wow6432Node\ODBC\ODBCINST.INI\Oracle in OraClient11g_home1"
'   If a matching section key is not found, returns an empty string.
'   Only returns first matching section key.
'
' Params:
'       sectToFind - string representing the keynode you're searching for.
'           ex: "ODBCINST.INI"
'       sectToLookIn - String representing the keynode to start the search in.
'           If omitted, use parent reg object's sectionKey value.
'*****************************************************************************
On Error GoTo ErrHandler:
Const PROC_NAME As String = "findSectionKey"

    Dim sSect() As String   ' string array of subnodes
    Dim iSectCount As Long  ' length of sSect array
    Dim reg As Registry     ' use a clone reg so we don't damage current object

    ' Test for optional sectToLookIn param
    If sectToLookIn = vbNullString Then
        sectToLookIn = Me.sectionKey
    End If
    ' create clone
    Set reg = New Registry
    With reg
        .ClassKey = Me.ClassKey
        .sectionKey = sectToLookIn
        ' create array of sections to search
        .EnumerateSections sSect, iSectCount
        ' search each section in array
        Dim i As Long
        For i = 1 To iSectCount
            'Debug.Print .sectionKey & "\" & sSect(i)
            If findSectionKey = vbNullString Then
                If sSect(i) = sectToFind Then
                    ' found node
                    findSectionKey = .sectionKey & "\" & sSect(i)
                    Exit For
                Else
                    'search subnodes via recursion
                    findSectionKey = findSectionKey(sectToFind, .sectionKey & "\" & sSect(i))
                End If
            Else
                Exit For
            End If
        Next i
    End With

ExitFunction:
    If Not (reg Is Nothing) Then
        Set reg = Nothing
    End If
    Exit Function
ErrHandler:
    'errBox TypeName(Me), PROC_NAME
    Resume ExitFunction
End Function

This is probably a typo:

On Error GoTo ErrHandler:

It should read as:

On Error GoTo ErrHandler

The : denotes a label, or separates multiple instructions on a single line, allowing you to write something like  For i = 0 To 10 : DoSomething i : Next on a single line. Useful for the immediate pane, or for making your code harder to read by cramming multiple instructions on a single line.

I've seen this error-handling /exit pattern quite often, I find it's easier to follow/read the error-handling /exit pattern when the execution flow is more linear - the label could use a better name then:

Cleanup:

    'handle errors:
    If Err.Number <> 0 Then
        'errBox TypeName(Me), PROC_NAME
        Err.Clear
    End If

    'clean up regardless of error status:
    If Not (reg Is Nothing) Then
        Set reg = Nothing
    End If

    'clean exit
End Function

Doing this avoids resorting to jumps in the execution flow (Resume), and while it makes sense to say On Error GoTo Cleanup, it also makes sense to clean up in error cases as well, especially when dealing with external resources (a database connection, a registry key, a text file, etc.) - the only jump that needs to happen is a forward jump, only in case of an error: you want to skip everything else in the method's logic and jump straight to a clean exit.

If something goes wrong and I have a connected ADODB.Recordset object that blows up, I want to have code that closes the recordset and the connection (assuming the method opened them in the first place), and I don't want to copy that code into the happy path, ...and I don't want velociraptors to find me! To me the solution is simply to run that cleanup code whether or not an error is raised.

Dim reg As Registry

There's no reason not to New up this instance right away:

Dim reg As New Registry

That saves 1 line and a comment!

For i = 1 To iSectCount

Since the class doesn't specify Option Base 1 anywhere in the (declarations) section (just looked at the Google Drive link), it looks like there's a bug right here; you're systematically skipping index 0 - arrays are typically zero-based, and collections one-based:

MSDN (emphasis mine):

  Because the default base is 0, the Option Base statement is never required. If used, the statement must appear in a module before any procedures. Option Base can appear only once in a module and must precede array declarations that include dimensions.


Maximize a traveling employee's vacation over k weeks I have following problem statement

A lazy employee works at a multi-national corporation with N offices
spread across the globe.
They want to work as few days over k weeks as possible.
Their company will pay for them to fly up to once per week (on
weekends) to a remote office with a direct flight from their current
office.  Once in an office, they must stay there until the end of the
week.
If an office observes a holiday while the employee is working there,
they get that day off.
Find the path that maximizes the employee's vacation over k weeks.
Assume they start at a home office H on a weekend (so in the first
week, they can be in any office connected to the home office).
For example:
H<--->A (It means there is a flight path from H to A and vice versa)
H<--->B

Here is my solution in Python.
vacation = [
{"H": 1, "A": 0, "B": 2},  # week 1
{"H": 0, "A": 2, "B": 0}
]

flights = [("H", "A"), ("H", "B")]

def get_optimal_path():
    all_possible_paths = []

    for offices in list(itertools.product(*vacation)):
        total_cost = 0
        week_level = 0
        for office in offices:
            vac_level = vacation[week_level]
            total_cost += vac_level[office]
            week_level += 1
        if checkIfValidPath(offices[0], offices[1]):
            all_possible_paths.append((offices, total_cost))

    print(max(all_possible_paths, key=lambda x: x[1]))

def checkIfValidPath(source, destination):
    return any(i for i in flights if i[0] == source and i[1] == destination
or i[0] == destination and i[1] == source)

get_optimal_path()

In this example best answer is for the employee to stay at office H in the first week and then move to office A in the second week so the total vacation days will be (1+2)=3. B and A cannot be the answer here because there is no path between them.
How can I optimize this code to increase its efficiency? Right now its complexity is nk (since we have k weeks).

With this kind of problems it is important to build good mathematical model. After that, the best algorithm known to science can be easily found.

The problem in question seems to be Longest path problem, which if we build a graph properly has linear time solution.

Lets consider a graph (V, E), where V are week-offices (for example, H1, A1, B1, H2, A2, B2, ... - where H1 means home office at first week, B2 - office B at week two, and so on).

Edges are possible flights. For the example in the question we have the following edges:

H1 -> H2, H1 -> A2, H1 -> B2, H2 -> H3, A2 -> H3, B2 -> H3, H2 -> A3, H2 -> B3 (note, that there is never An -> Bn direct flights).

Here is the flights graph, which is an input, (on the left) and corresponding unrolled space-time one, for which a linear time algorithm can be found. Each node has a given weight.

Then a somewhat lengthy quotation from wikipedia:

"For instance, for each vertex v in a given DAG, the length of the longest path ending at v may be obtained by the following steps:

Find a topological ordering of the given DAG.
For each vertex v of the DAG, in the topological ordering, compute the length of the longest path ending at v by looking at its incoming neighbors and adding one to the maximum length recorded for those neighbors. If v has no incoming neighbors, set the length of the longest path ending at v to zero. In either case, record this number so that later steps of the algorithm can access it.

Once this has been done, the longest path in the whole DAG may be obtained by starting at the vertex v with the largest recorded value, then repeatedly stepping backwards to its incoming neighbor with the largest recorded value, and reversing the sequence of vertices found in this way."

Please, note, that our graph has been already topologically sorted - we know where it starts and where it ends.

A recursive algorithm can be elegant, but it's not really needed here.

This is maybe a bit unusual answer for a code review, but I strongly believe it is appropriate to point out at the possibility of a better algorithm while doing code review. I hope this is useful not only as a specific answer, but also as a good approach to apply math where it's due.

Please also note it is not necessary to memorize all possible graph and theory algorithms: At first I only had a vague idea, which became clear after some thinking and finding right terms.


Schedule matches for a season or round robin tournament The algorithm comes from wiki.

The output from the algorithm is correct but not something that a person could (easily) understand.  See the wiki link above.

Each team play every other team.  Optionally play every team twice and reverse home / away.

Assume an even number of teams.  I need to add a test for even.

Please review for style and any other advice.

public struct Match
{
    public int IDhome { get; }
    public int IDaway { get; }
    public int Week { get; }
    public override string ToString()
    {
        return $"week {Week}  IDhome {IDhome}  IDaway {IDaway}";
    }
    public Match(int idHome, int idAway, int week)
    {
        IDhome = idHome;
        IDaway = idAway;
        Week = week;
    }
}
public struct Season
{
    public int[,] ScheculeRaw { get; }
    int TeamCount { get; }
    public List<Match> Matches { get; }
    public override string ToString()
    {
        StringBuilder sb = new StringBuilder();
        foreach (Match m in Matches)
        {
            sb.AppendLine(m.ToString());
        }
        return sb.ToString();
    }
    public Season(int teamCount, bool awayHome = false)
    {
        ScheculeRaw = RoundRobinRaw(teamCount);
        TeamCount = teamCount;
        Matches = new List<Match>();
        for (int w = 0; w < TeamCount - 1; w++)
        {
            for (int p = 0; p < TeamCount/2; p++)
            {
                Matches.Add(new Match(ScheculeRaw[w, p] + 1, ScheculeRaw[w, teamCount - 1 - p] + 1, w + 1));
            }
        }
        if (awayHome)
        {
            for (int w = 0; w < TeamCount - 1; w++)
            {
                for (int p = 0; p < TeamCount / 2; p++)
                {
                    Matches.Add(new Match(ScheculeRaw[w, teamCount - 1 - p] + 1, ScheculeRaw[w, p] + 1, w + teamCount));
                }
            }
        }
    }

    public static int[,] RoundRobinRaw(int count)
    {
        int[] rr = new int[count];
        for (int i = 0; i < count; i++)
        {
            rr[i] = i;
        }
        int[,] sch = new int[count - 1, count];
        for (int w = 0; w < count - 1; w++)
        {
            for (int i = 0; i < count; i++)
            {
                sch[w, i] = rr[i];
            }

            //Debug.WriteLine("");
            //Debug.WriteLine("ww1");
            //for (int p = 0; p < count; p += 2)
            //{
            //    Debug.Write(sch[w, p] + ", ");
            //}
            //Debug.WriteLine("");
            //Debug.WriteLine("ww2");
            //for (int p = count - 1; p > 0; p -= 2)
            //{
            //    Debug.Write(sch[w, p] + ", ");
            //}

            // rotate rr
            int temp = rr[count - 1];
            for (int r = count - 2; r > 0; r--)
            {
                rr[r + 1] = rr[r];
            }
            rr[1] = temp;

            //Debug.WriteLine("");
            //Debug.WriteLine("rr");
            //for (int r = 0; r < count; r++)
            //{
            //    Debug.Write(rr[r] + ", ");
            //}
            //Debug.WriteLine(".");
        }
        return sch;
    }
}

Use readable variable names. I can guess at the meaning of sch (schedule?) and  rr (round robin), but I'm at a loss for w (week?) and p. There's no reason to shorten the names, it does not affect the execution of the code; it does affect the readability. Everything might make sense to you today, but not to other people, or you in the future.
Using arrays versus lists is (at least partially) subjective), but I do think that you should be using lists here. It keeps the syntax cleaner and doesn't require readers to keep track of the array indexes.

The for loops are adding so much to the complexity. w < TeamCount - 1 and p < TeamCount / 2 are not easy to understand the intention. If you stick to using lists, that guides you towards using a more readable foreach statement, e.g. foreach(week in weeks)

bool awayHome = false is a very confusing name. A better name would be generateReverseMatch (or whatever designated name you prefer)
Personally, I like to keep the constructor focused on property/field assignment. I would abstract the match generation into a private method, which can still be called from the constructor: Matches = GenerateMatches(teamCount);
While not wrong per se, be on the lookout of overzealously using ToString() instead of a nicer  PrintMatches() method. Most of the time, you want a custom layout, especially if you want to have different ways of showing the same season.

Switching the code over to using lists instead of arrays is going to have a strong impact on the posted code, so I'm not sure if further code review is going to be relevant here.


Unit testing HTTP requests I would like to unit test my web requests, so on top of my head I came up with the solution below.

Since it seems to be working fine, I'm posting it on this site. Though it doesn't feel very dynamic, I would like your input. For example, this solution can't simulate response headers, which I'm also interested in mocking:

public interface IWebRequest
{
    Task<string> DownloadString();
}

public class RealWebRequest : IWebRequest
{
    private readonly string url;

    public RealWebRequest(string url)
    {
        this.url = url;
    }

    public Task<string> DownloadString()
    {
        using (WebClient client = new WebClient())
        {
            return client.DownloadStringTaskAsync(new Uri(url));
        }
    }
}

public class FakeWebRequest : IWebRequest
{
    private readonly string fakeReponse;

    public FakeWebRequest(string fakeReponse)
    {
        this.fakeReponse = fakeReponse;
    }

    public Task<string> DownloadString()
    {
        return Task.Factory.StartNew(() => fakeReponse);
    }
}

public class TestClass
{
    private readonly IWebRequest request;

    public TestClass(IWebRequest request)
    {
        this.request = request;
    }

    public async Task<string> DoStuff()
    {
        return await request.DownloadString();
    }
}

"Real" usage example:

IWebRequest realRequest = new RealWebRequest("http://www.test.com");
TestClass test = new TestClass(realRequest);
string realResult = await test.DoStuff();

Unit test usage example:

IWebRequest fakeRequest = new FakeWebRequest("fake response string");
TestClass test = new TestClass(fakeRequest);
string fakeResponse = await test.DoStuff();

I really rather think you have the right idea with the interface and your real class. I do heartily endorse your use of readonly which doesn't get a lot of exposure. However, there are many good mocking frameworks out there that you don't need to write your own fake class to unit test with. For instance, we use Moq where I work and your unit test example would look like this:

Mock<IWebRequest> fakeRequest = new Mock<IWebRequest>();

fakeRequest
    .Setup(request => request.DownloadString(It.IsAny<string>())
    .Returns("fake response string");

TestClass test = new TestClass(fakeRequest.Object);
string fakeResponse = await test.DoStuff();


Can I buff up your file? Here I have a method for reading the contents of a file into a buffer and returning whether or not it is successful.

Here is what I would like reviewed:

Speed:  Am I reading in the file as fast as possible?
Bugs: Are there cases where my code could fail or give unexpected behavior?
Syntax/Styling:  How does my code look?  How could I make it look better?  Anything I'm doing wrong syntax-wise?

int getFileContents(const char *file, void **contents, size_t *size)
{
    // Open file for reading
    int fd = open(file, O_RDONLY);
    if (fd < 0) return -1;

    off_t total = lseek(fd, 0, SEEK_END);
    if (total < 0) return -1;

    lseek(fd, 0, SEEK_SET);

    ssize_t n, rest = total, readb = 0;
    unsigned char buf[BUF_SIZE];
    unsigned char *result = NULL;

    // Read the whole file into memory
    while (rest > 0) {
        n = read(fd, buf, BUF_SIZE);
        if (n < 0) {
            close(fd);
            free(result);
            return -1;
        }

        unsigned char *tmp = (unsigned char*) realloc(result, readb + n);
        if (!tmp) {
            close(fd);
            free(result);
            return -1;
        }

        result = tmp;
        memcpy(result + readb, buf, n);
        readb += n;
        rest -= n;
    }

    close(fd);
    *contents = result;
    *size = total;

    return 0;
}

1. Bugs

If the open succeeds but the lseek fails (as it might if the file is a named pipe), the function omits to close the file descriptor, thus leaking it.
If read reaches the end of the file (as it might if the file was truncated after the lseek), then it returns 0. This will cause the function to go into an infinite loop.
There's no check on the result of the second call to lseek. If this call fails, then the function will go into the infinite loop described above.

2. Review

There's no specification. What does this function do? How do I call it? What does it return?
This is a worst-case \$\Theta(n^2)\$ algorithm, because realloc may have to copy the result each time it grows, and you grow the buffer by at most BUF_SIZE bytes each time you call realloc. It's usual in this kind of situation to grow the result exponentially, by multiplying the result size by some constant on each realloc call.
Even ignoring the copying due to realloc, each byte read from the file is copied again using memcpy. It would be better to realloc the result first, and then read directly into the result.
You go to the trouble of finding the length of the file, so why not read it all in one go?

result = malloc(total);
if (!result)
    /* handle error */
n = read(fd, result, total);
if (n != total)
    /* handle error */

Consider using fstat instead of lseek to determine the size of the file.
It's not a good idea to put multiple statements on the same line, like this:

if (fd < 0) return -1;

That's because in most debuggers you won't be able to set a breakpoint on the return statement.
There's repetition in the handling of the four failure cases. This makes the function difficult to modify (because you might fail to adjust one or more of the cases). The bug in §1.1 above might have been caused by this problem.
For reading the entire contents of a file into memory, consider using mmap.

mmap has advantages over read. (i) There's no need to allocate a buffer: the kernel maps the file directly into the process's virtual memory. (ii) mmap operates lazily: it maps the file into memory but does not actually copy any part of it until that page is touched, so programs that randomly access the file don't have to wait for the whole file to load.

mmap also has disadvantages. (i) mmap only operates on ordinary files (not pipes, sockets, or other kinds of file descriptor). (ii) Mapping lots of files creates lots of entries in the process's virtual memory table, and on some operating systems (notably OS X) this can lead to poor memory performance.

3. Revised code

#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <unistd.h>

/* Map the contents of file into memory: if successful, update
 * *contents to point to the file contents, update *size to be the
 * size of the file, and return 0. Otherwise, return -1.
 */
int getFileContents(const char *file, void **contents, size_t *size)
{
    int fd = open(file, O_RDONLY);
    if (fd < 0)
        goto fail_open;

    struct stat st;
    if (fstat(fd, &st) < 0)
        goto fail_fstat;

    void *result = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
    if (result == MAP_FAILED)
        goto fail_mmap;

    *contents = result;
    *size = st.st_size;
    close(fd);
    return 0;

fail_mmap:
fail_fstat:
    close(fd);
fail_open:
    return -1;
}

4. Answers to comments

In comments, lesto suggests:

  I would return a different number for every error, to help the user of the function to identify the problem and try to recover.

In this context (Unix systems programming), it's conventional for a function to return a single distinguished value (typically −1) to indicate an error, with the cause of the error being stored in errno. See for example the POSIX specification of open:

  Upon successful completion, these functions shall open the file and return a non-negative integer representing the lowest numbered unused file descriptor. Otherwise, these functions shall return -1 and set errno to indicate the error.

It makes sense to stick to the Unix convention, so that the caller can apply the same kind of error handling to getFileContents as they do to other system interfaces.

  Every error between open and close need the call to close, and leaving it inline is easier than check the list of label is correct, IMHO. It is true that you are writing some more lines.

If you write out the error-handling code inline, then your functions have length that's quadratic in the number of error cases! This is disastrous for readability and maintainability. It is all too easy to forget one of the cases, as in the bug described in §1.1 above.

An alternative to goto is to use nested if statements. In this case the code would become:

int fd = open(file, O_RDONLY);
if (fd >= 0) {
    struct stat st;
    if (fstat(fd, &st) == 0) {
        void *result = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
        if (result != MAP_FAILED) {
            *contents = result;
            *size = st.st_size;
            close(fd);
            return 0;
        }
    }
    close(fd);
}
return -1;

but this quickly becomes hard to read because of the indentation.


Better way to create directories using File I have this code that generally creates directories within a directory.

File  userFolder = new File(rootPath+"/media/"+userBean.getUsername());
    File  userPhotosDir = new File(rootPath+"/media/"+userBean.getUsername()+"/photos");
    File  userAudioDir = new File(rootPath+"/media/"+userBean.getUsername()+"/tracks");
    userFolder.mkdir();
    userPhotosDir.mkdir();
    userAudioDir.mkdir();

When I this code runs it will generally look like this

The userFolder

\media\somename

The Photos folder

\media\somename\photos

The Audio Folder

\media\somename\folder

Where the photos and audio folder are inside the somename folder. while the somename folder is inside the media folder

Well, for the purpose of codereview:

There is a mkdirs() methods instead of the mkdir() method
You can use the new File(String, String) constructor to avoid taking care of slashes
If you have Java7, you can (and probably should) use Path. Especially, because there is a way to construct Paths with unlimited arguments, not only 2 as for File:

Path dir = Paths.get(rootPath, "media", userBean.getUsername(), arg1);
Files.createDirectories(dir);

You should check the return codes and errors. Could be helpful.


Regular expression to get a string between two strings where the last string can also be 'end of string' I intend to extract a URL search parameter value using a regular expression in plain javascript. The parameter can be in any order in the search query.
So is there a better approach than ?

const query1 = '?someBoolean=false&q=&location=&testParam=dummy_value&testParam2=dummy_value2&requiredParam=requiredValue';

const query2 = '?someBoolean=false&q=&location=&testParam=dummy_value&testParam2=dummy_value2&requiredParam=requiredValue&someMoreParam=dummy_value2';

let requiredParam = query1.match(/requiredParam=(.*?)$/) || [];
console.log('Using regex "/requiredParam=(.*?)$/"');
console.log(`For Query1: result = ${requiredParam[1]}`);

requiredParam = query2.match(/requiredParam=(.*?)&/) || [];
console.log('Using regex "/requiredParam=(.*?)&/"');
console.log(`For Query2: result = ${requiredParam[1]}`);

console.log('Combining both regex "/requiredParam=(.*?)(&|$)/"');
requiredParam = query1.match(/requiredParam=(.*?)(&|$)/) || [];
console.log(`For Query1: result = ${requiredParam[1]}`);
requiredParam = query2.match(/requiredParam=(.*?)(&|$)/) || [];
console.log(`For Query2: result = ${requiredParam[1]}`);

Edit:
My constraints:

Browser compatibility including IE 9
Using only vanilla javascript

URLSearchParams would make things significantly easier:

const query1 = '?someBoolean=false&q=&location=&testParam=dummy_value&testParam2=dummy_value2&requiredParam=requiredValue';
const query2 = '?someBoolean=false&q=&location=&testParam=dummy_value&testParam2=dummy_value2&requiredParam=requiredValue&someMoreParam=dummy_value2';

const params1 = new URLSearchParams(query1);
const params2 = new URLSearchParams(query2);

console.log(`For Query1: result = ${params1.get('requiredParam')}`);
console.log(`For Query2: result = ${params2.get('requiredParam')}`);

It's supported natively in the vast majority of browsers, but not all. For the rest, here's a polyfill. It's better not to re-invent the wheel when you don't need to, and it's good when you're able to use a standard API (with examples and documentation and Stack Overflow answers about it, etc).
As a side note - when using regular expressions, I'd recommend using capture groups only when necessary. If all you need to do is group some tokens together logically (like for a | alternation), non-capturing groups should be preferred. That is, if URLSearchParams didn't exist, better to do (?:&|$) than (&|$). Reserve capturing groups for when you need to save and use the captured result somewhere - otherwise, non-capturing groups are more appropriate, less expensive, and require less cognitive overhead.
If you had to go the regex route, another slight improvement would be to use a negative character class instead of lazy repetition. In the pattern, you have:
(.*?)(&|$)

Lazy repetition is slow; it forces the engine to advance one character at a time, then check the rest of the pattern for a match, and repeat until the match is found. Since you know that the capture group will not contain any &s, better to match anything but &s:
([^&]*)

Once you do that, you don't even need the final (&|$) or (?:&|$) due to the greedy repetition.


Using generator for buffered read of large file in Python I have a large file that I need to parse - and since it will be regenerated from external queries every time script runs so there is no way to parse it once and cache the results.

I want to save memory footprint and read and parse only logical "chunks" of that file everything between open 'product' and closing curly bracket.
Not sure what is the canonical way in Python (I am new to the language).

Here's what I tried so far:

def read_chunk(file_name, pattern_open_line, pattern_close_line):
    with open(file_name,"r") as in_file:
        chunk = []
        in_chunk = False
        open_line = re.compile(pattern_open_line);
        close_line = re.compile(pattern_close_line)
        try:
            for line in in_file:
                line = line.strip()
                if in_chunk:
                    chunk.append(line)
                if close_line.match(line):
                    yield chunk
                if open_line.match(line):
                    chunk = []
                    chunk.append(line)
                    in_chunk = True
                    continue
        except StopIteration:
            pass

def get_products_buffered(infile):
    chunks = read_chunk(infile, r'^product\s*$', r'^\s*\}\s*')
    products = []
    for lines in chunks:
        for line in lines:
            if line.startswith('productNumber:'):
                 productNumber = line[len('productNumber:'):].strip().rstrip(';').strip('"')
                 products.append(productNumber)
                 continue
    return products

def get_products_unbuffered(infile):
    with open(infile) as f:
        lines = f.readlines()
        f.close()
    products = []
    for line in lines:
        if line.startswith('productNumber:'):
             productNumber = line[len('productNumber:'):].strip().rstrip(';').strip('"')
             products.append(productNumber)
             continue
    return products

I profiled both runs and while unbuffered reading is faster:

Buffered reading
Found 9370 products:
Execution time: 3.0031037185720177
Unbuffered reading
Found 9370 products:
Execution time: 1.2247122452647523

it also incurs a much bigger memory hit when file is essentially read into memory:

Line #    Mem usage    Increment   Line Contents
================================================
    29     28.2 MiB      0.0 MiB   @profile
    30                             def get_products_buffered(infile):
    31     28.2 MiB      0.0 MiB       chunks = read_chunk(infile, '^product\s*$', '^\s*\}\s*')
    32     28.2 MiB      0.0 MiB       products = []
    33     30.1 MiB      1.9 MiB       for lines in chunks:

versus:

Line #    Mem usage    Increment   Line Contents
================================================
    42     29.2 MiB      0.0 MiB   @profile
    43                             def get_products_unbuffered(infile):
    44     29.2 MiB      0.0 MiB       with open(infile) as f:
    45    214.5 MiB    185.2 MiB           lines = f.readlines()

I would be grateful for any pointers/suggestions.

You called it unbuffered, but these lines:

with open(infile) as f:
    lines = f.readlines()
    f.close()

slurp the entire file into memory, while your 'buffered' version only pulls in a line at a time, returning chunks.

I note that you're not doing anything with the entire chunk, just the lines starting with 'productNumber:', so I think a rework of your 'unbuffered' code will actually be fastest, as well as clearest:

def get_products_unbuffered(infile):
    products = []
    with open(infile) as f:
        for line in f:
            if line.startswith('productNumber:'):
                 productNumber = line[len('productNumber:'):].strip().rstrip(';').strip('"')
                 products.append(productNumber)
    return products

as this will read the file a line at a time and only keep desired info (productNumbers) in memory..