Where should I put menu items in MVC with PHP - Model or Controller? This is my first attempt with MVC and I almost get it, but a small thing bothers me.

I have this controller in CodeIgniter:

<?php
class Page extends CI_Controller {

    public function index($page = "home")
    {
            $data['page'] = $page;
            list($data['title']) = array_keys($this->_menu(), $page);
            $data['menuItems'] = $this->_menu();

            $this->load->view('templates/header', $data);
            $this->load->view('templates/menu', $data);
            $this->load->view('pages/'.$page, $data);
            $this->load->view('templates/footer', $data);
    }

    private function _menu()
    {
        static $menuItems = array(
                           "Home page" => "home",
                           "Our history" => "history",
                           "About us" => "about",
                           "Contact page" => "contact"
                       );

        return $menuItems;
    }
}

And here is the menu view:

<?php
$menu = "";
foreach ($menuItems as $menuName => $view) {
    $menu .= '
';
        $menu .= '';
    } else {
        $menu .= '>';
    }
    $menu .= $menuName . '
' .PHP_EOL ;
}

echo $menu;

Where do the menu items belong in MVC logic?  Is it ok to store things like the menu in the Controller or do I have to make a new Model for it like this?

class Menu extends CI_Model {

    public function __construct()
    {
        parent::__construct();
    }

    public function get_menu_items()
    {
        static $menuItems = array(
                           "Home page" => "home",
                           "Our history" => "history",
                           "About us" => "about",
                           "Contact page" => "contact"
                       );

        return $menuItems;
    }
}

Then load it from Controller:

public function index($page = "home")
{
    ...
    $this->load->model('Menu');
    $data['menuItems'] = $this->Menu->get_menu_items();

    $this->load->view(...);
    $this->load->view(...);
    $this->load->view(...);
}

This a nice example: it shows how the MVC is just a pattern which should be ajusted to your needs.

The menu items you're showing seem very unlikely to change over time. This means you can simply put them in your "templates/menu" view.
If they're shared among multiple views, then the controller is OK.
If the menu items can change dynamically, then your model can provide a meaningful abstraction over your data, and then it would go into a model.


Check OpenVPN's connection status I wrote a method that will start OpenVPN on a config file, and then wait to be connected.

It relies on OpenVPN using Local Area Connection 2, which hurts code reusability, but I'm not sure how to do it any other way.

private static async void StartOpenVPN(string configFile)
{
    // Start OpenVPN on the config file...
    Process.Start("openvpn-gui", $"--connect {configFile}");

    // OpenVPN uses Local Area Connection 2.
    var adapter =
        NetworkInterface.GetAllNetworkInterfaces()
            .First(networkInterface => networkInterface.Name == "Local Area Connection 2");

    // Wait for Local Area Connection 2 to be operational...
    while (adapter.OperationalStatus != OperationalStatus.Up)
    {
        await Task.Delay(100);
        adapter =
            NetworkInterface.GetAllNetworkInterfaces()
                .First(networkInterface => networkInterface.Name == "Local Area Connection 2");
    }

    Console.WriteLine("Connected!");
}

Is there a more effective/efficient/elegant way to start OpenVPN on a config file, and then wait to be connected?

Use the Gateway or Unicast IP Address

For the case of checking if the VPN Connection is online or not, you could consider a solution that just returns a boolean value.

Secondly, I wouldn't do it by connection name, instead how about using either the gateway IP address or the unicast IP address?

public class OpenVpnConnection
{
    private IPAddress _gatewayIpAddress;
    private IPAddress _unicastIpAddress;

    public OpenVpnConnection()
    {
        _gatewayIpAddress = IPAddress.Parse("10.0.0.1");
        _unicastIpAddress = IPAddress.Parse("10.0.0.101");
    }

    public bool IsNetworkAvailable()
    {
        if (!NetworkInterface.GetIsNetworkAvailable())
            return false;

        foreach (NetworkInterface ni in NetworkInterface.GetAllNetworkInterfaces())
        {
            // discard because of standard reasons
            if (ni.OperationalStatus != OperationalStatus.Up) continue;
            if (ni.NetworkInterfaceType == NetworkInterfaceType.Loopback) continue;
            //if (ni.NetworkInterfaceType == NetworkInterfaceType.Tunnel) continue;

            // discard if the interface is not "virtual"
            //if (ni.Description.IndexOf("virtual", StringComparison.OrdinalIgnoreCase) <= 0) continue;
            //if (ni.Name.IndexOf("virtual", StringComparison.OrdinalIgnoreCase) <= 0) continue;

            if (GetIpFromGatewayAddresses(ni).Contains(_gatewayIpAddress)) return true;
            if (GetIpFromUnicastAddresses(ni).Contains(_unicastIpAddress)) return true;
        }
        return false;
    }

    private static IEnumerable<IPAddress> GetIpFromGatewayAddresses(NetworkInterface nic)
    {
        return nic.GetIPProperties().GatewayAddresses
            .Where(ip => ip.Address.AddressFamily == System.Net.Sockets.AddressFamily.InterNetwork)
            .Select(ip => ip.Address);
    }

    private static IEnumerable<IPAddress> GetIpFromUnicastAddresses(NetworkInterface nic)
    {
        return nic.GetIPProperties().UnicastAddresses
            .Where(ip => ip.Address.AddressFamily == System.Net.Sockets.AddressFamily.InterNetwork)
            .Select(ip => ip.Address);
    }
}

You can take out sections of the code that you don't need, but I included both unicast and gateway options above.

You would then simply check if the connection is online by

var vpn = new OpenVpnConnection();
bool online = vpn.IsNetworkAvailable();

of course, you'll want to pass parameters and what-not, but this is simply to illustrate the idea of moving away from the connection name and using an IP address instead.

Putting it together

A quick and dirty way of piecing it together with you current code would look something like this

private static async void StartOpenVPN(string configFile)
{
    // Start OpenVPN on the config file...
    Process.Start("openvpn-gui", $"--connect {configFile}");

    var vpn = new OpenVpnConnection(); //of course, send parameters in your actual code
    while (!vpn.IsNetworkAvailable)
    {
        await Task.Delay(100);
    }

    Console.WriteLine("Connected!");
}

Rather than using a loop, you could consider using the NetworkChange.NetworkAvailabilityChanged Event instead.


PHP code for blacklisting and rewriting/redirecting URLs We faced an issue where URLs had a wrong "subfolder" in its URL, so e.g.

example.com/match1/randomgenerated

should have been

example.com/rewritewiththis/randomgenerated

We currently use this code for redirecting URLs that (maybe it could be that it only starts at urls that are "bad"):

$oldurl = JUri::getInstance();
$newurl = str_replace(['match1','match2','match3'], 'rewritewiththis', $oldurl);

$bad_urls = array('match1','match2','match3');

foreach($bad_urls as $bad_url){
    if(strpos($oldurl, $bad_url) !== false) {
        exit(header("Location: $newurl", true, 301));
    }
}

We use the $newurl also for things like this (this rewrites the generated URLs to use a correct URL and prevent duplicate content):

$oldurl = $this->getRoute($event->url_query . '&Itemid=' . $request->query->page_id)->toString();
$data['url'] = str_replace('match1', 'rewritewiththis', $oldurl);

Are there better options to do this are are is this already a good way to solve this issue?

I think it looks good, a little verbose. Could be simplified to this

$oldurl = JUri::getInstance();
$bad_urls = ['match1','match2','match3'];
$newurl = str_replace($bad_urls, 'rewritewiththis', $oldurl);

if(in_array($oldurl, $bad_urls))
    exit(header("Location: $newurl", true, 301));

If you need to use something like this in many locations, or you need to elaborate on what are considered bad urls a router system/class may be a better way to go.

If you're asking if there's a different way to re-route without using header() I don't think there is. Any router system I have seen uses header() to re-route, no matter how much code surrounds and abstracts them.


Signal output on Raspberry Pi that acts as input for FPGA Problem

I'm writing a verilog program that does the trapezoidal integration method (where a review is also welcome, wink wink). But turns out you need input for these kind of things, so in the overly complicated setup I'm doing, I'm writing a python program on a Raspberry Pi to output inputs for the FPGA which then outputs to a d to a which outputs to an oscilloscope which is where you all go, "Thank goodness I'm not that programmer" and I go half mad.

The main requirements for the code is that it output each bit of the input for the FPGA at a separate pin, as serial data is slower, and that the output be two's complement (i.e., the signals are signed integers, so the bits given to the FPGA must also be signed).

Approach

My code can be broken up into three phases: signal generation, signal conversion, and signal output.

Signal generation

Right now, I'm just using a simple square wave. This is done using

x = 0
signal = 1
while x <= 25:
    if x % 2 == 0:
        signal*=-1
    signals.append(signal)
    x+=1

Signal conversion

This iterates over signals and applies twos_comp() to each. twos_comp() converts the integer to its binary representation (carefully slicing off all the unneeded nonsense) and then if the original value was greater than zero, just returns that. Otherwise, the number is inverted using the invert() function, and then has one added to it, and is returned. Type conversions are rife, and this whole thing is rather hack-y.

This assumes 8 bit numbers.

Signal output

Using RPi.GPIO, a clock pin (which is mapped to the FPGA's clock) is setup and a set of 8 pins is setup for the signal. Again, parallel not serial. For each item in the new list generated by twos_comp(), the program waits for the clock's rising edge (I know this is somewhat frowned upon, but the output needs to be synced to the clock and the clock is also pretty fast, so it's not like it's going to be waiting for a while (50 mega hertz, can be faster). Then, for each item in the 8-bit number, it's outputted to the appropriate pin.

Concerns/reviewing

Any and all comments are welcome in this review, with speed comments (especially in the latter part of the program) extra welcome as I want it to be able to keep up with the FPGA clock. (Speed in the first part obviously isn't as necessary as it all can be done before the FPGA starts up. I also don't know if RPi.GPIO will be able to output the bits fast enough.) As I mentioned earlier, the invert() and twos_comp() functions are...kinda hacky.

Thanks!

Code

import RPi.GPIO as GPIO

GPIO.setmode(GPIO.BCM)
x = 21
while x <= 28: #signal pins
    GPIO.setup(x, GPIO.OUT)
    x+=1
GPIO.setup(5, GPIO.IN) #clock

signals = []
bit_signals = []

def invert(bit_val):
    new_val = []
    for i in bit_val:
        new_val.append(str(int(not(int(i)))))
    return ''.join(new_val)

def twos_comp(val):
    bin_val = format(val, '09b')[-8:]
    if val < 0:
        return format(int(invert(bin_val),2) + int('1', 2), '08b')
    else:
        return bin_val

x = 0
signal = 1
while x <= 25:
    if x % 2 == 0:
        signal*=-1
    signals.append(signal)
    x+=1

for i in signals:
    bit_signals.append(twos_comp(i))

for i in bit_signals:
    GPIO.wait_for_edge(5, GPIO.RISING)
    counter = 21
    for k in str(i):
        GPIO.output(counter, k) #k will be 0 or 1 which is accepted input
        counter += 1

GPIO.cleanup()

You should start by organising your code better. Try to avoid global variables and group together code in functions.

In addition, you should learn about list comprehensions. They make initializing your signals way easier.

import RPi.GPIO as GPIO

def twos_comp(i):
    ...

def gpio_setup(inputs, outputs):
    GPIO.setmode(GPIO.BCM)
    for pin in inputs:
        GPIO.setup(pin, GPIO.IN)
    for pin in outputs:
        GPIO.setup(pin, GPIO.OUT)

def main(input_pin, output_pins):
    signals = [1 if i % 2 else -1 for i in range(26)]
    bit_signals = [twos_comp(i) for i in signals]

    for i in bit_signals:
        GPIO.wait_for_edge(input_pin, GPIO.RISING)
        for pin, k in zip(output_pins, str(i)):
            GPIO.output(pin, k) #k will be 0 or 1 which is accepted input

if __name__ == "__main__":
    in, out = [5], range(21, 29)
    gpio_setup(in, out)
    main(in[0], out)
    GPIO.cleanup()

And finally, there are better ways to do twos complement. A nice one is for example here.


Generating absolute URIs behind a reverse proxy In my ASP.NET MVC application there are occasions where I need to generate an absolute URI, such as when sending an email confirmation link to a user. The most straightforward approach is to use UrlHelper.Action() with the protocol parameter, and this has worked well for me.

However, now my server is behind a reverse proxy, so the absolute URIs that I'm generating are pointing to backendserver.myorg.local instead of publicsite.example.com, and that's not good for business.

So, I wrote this method to inspect the Origin header. Do you see any security problems or other reasons this might not be a good solution?

static class UrlHelperExtensions
{
    private const string OriginHeader = "Origin";

    /// <summary>
    /// Generates a full absolute URI, with HTTP(S), host name, and everything. Is reverse-proxy-aware,
    /// so will generate public URIs in the event that our server is behind a reverse proxy.
    /// </summary>
    /// <param name="urlHelper"></param>
    /// <param name="actionName"></param>
    /// <param name="controllerName"></param>
    /// <param name="routeValues"></param>
    /// <returns></returns>
    public static string AbsoluteAction(this UrlHelper urlHelper, string actionName, string controllerName, object routeValues = null)
    {
        if (urlHelper == null)
            throw new ArgumentNullException(nameof(urlHelper), $"{nameof(urlHelper)} is null.");

        var protocol = urlHelper.RequestContext.HttpContext.Request.Url.Scheme; // Should be "http" or "https"
        var localUriString = urlHelper.Action(actionName, controllerName, routeValues, protocol);

        var originValues = urlHelper.RequestContext.HttpContext.Request.Headers.GetValues(OriginHeader)
            .Where(x => !String.IsNullOrWhiteSpace(x))
            .ToList();

        if (!originValues.Any())
            return localUriString; // We are NOT behind a reverse proxy. Easy peasy, return local URI.

        var originUriString = originValues.First(); // There should only be only 1 Origin header, but theoretically it's possible to have more than 1.
        var originUri = parseOriginUri(originUriString);

        var uriBuilder = new UriBuilder(localUriString);
        uriBuilder.Scheme = originUri.Scheme; // AKA "protocol" (http or https)
        uriBuilder.Host = originUri.Host;
        uriBuilder.Port = originUri.Port;

        return uriBuilder.ToString();
    }

    private static Uri parseOriginUri(string originText)
    {
        try
        {
            return new Uri(originText);
        }
        catch (UriFormatException ex)
        {
            throw new UriFormatException($"{OriginHeader} header URI is in an invalid format: \"{originText}\"", ex);
        }
    }
}

Note: Right now I don't have any requirements to actually manipulate the URL path.

This is based on a misunderstanding of the Origin header. Chrome and Safari add this header to their web requests, but other browsers don't necessarily. You only have to test this from a browser like Internet Explorer or Firefox to find out it won't work.

The Forwarded header is probably more what you're looking for.

This header-based approach could be a security issue as well, depending on your environment. If your server is accessed directly somehow, then you're basically giving the remote client control over what URLs will be generated. All the client would need to do is manipulate the right headers in the right way. Giving remote clients control over your internal URLs sounds like a very bad thing.

You're probably better off using a server-side setting (perhaps in Web.config) that contains the public URI, and using that server-side setting instead.


Compares two Log Lines I am trying to compare two Log lines based on some conditions, in order to sort them:

Code:

public static final Comparator<String> HTMLcomparator = new Comparator<String>()
    {
        @Override
        public int compare(String line1, String line2)
        {
            HTMLLogLine htmlLogLine1 = new HTMLLogLine(line1);
            HTMLLogLine htmlLogLine2 = new HTMLLogLine(line2);
            int fullCompare = 0;

            String requestId1 = htmlLogLine1.getRequestId();
            String requestId2 = htmlLogLine2.getRequestId();

            if(requestId1 != null && requestId2 != null)
            {
                fullCompare = requestId1.compareTo(requestId2);
            }
            else if(requestId1 == null && requestId2 != null)
            {
                fullCompare = -1;
            }
            else if(requestId1 != null)
            {
                 fullCompare = 1;
            }
            else
            {
                fullCompare = 0;
            }

            if(fullCompare == 0)
            {
                String security1 = htmlLogLine1.getSecurity();
                String security2 = htmlLogLine2.getSecurity();
                if(security1 != null && security2 != null)
                {
                    fullCompare = security1.compareTo(security2);
                }
                else if(security1 == null && security2 != null)
                {
                    fullCompare = -1;
                }
                else if(security1 != null && security2 == null)
                {
                    fullCompare = 1;
                }
                else
                {
                    fullCompare = 0;
                }
            }

            if(fullCompare == 0)
            {
                String scenario1 = htmlLogLine1.getScenario();
                String scenario2 = htmlLogLine2.getScenario();

                if(scenario1 != null && scenario2 != null)
                {
                    fullCompare = scenario1.compareTo(scenario2);

                }
                else if(scenario1 == null && scenario2 != null)
                {
                    fullCompare = -1;
                }
                else if(scenario1 != null)
                {
                    fullCompare = 1;
                }
                else
                {
                    fullCompare = 0;
                }
            }

            if(fullCompare == 0)
            {
                Optional<Instant> timestamp1 = htmlLogLine1.getTimestamp();
                Optional<Instant> timestamp2 = htmlLogLine2.getTimestamp();

                if(timestamp1.isPresent() && timestamp2.isPresent())
                {
                    fullCompare = timestamp1.get().compareTo(timestamp2.get());
                }
                else if(!timestamp1.isPresent() && timestamp2.isPresent())
                {
                    fullCompare = -1;
                }
                else if(timestamp1.isPresent() && !timestamp2.isPresent())
                {
                    fullCompare = 1;
                }
                else
                {
                    fullCompare = 0;
                }

            }

            return fullCompare;
        }
    };

How can I reduce the duplicate code and make this compare method concise?

You can reduce the code duplication by moving the logic of comparing two values of which at least one is null to a separate method and call this method like this:

if (requestId1 != null && requestId2 != null) {
    fullCompare = requestId1.compareTo(requestId2);
} else {
    fullCompare = compareWithNull(requestId1, requestId2);
}

//...

if (security1 != null && security2 != null) {
    fullCompare = security1.compareTo(security2);
} else {
    fullCompare = compareWithNull(security1, security2);
}

//...

This reduces the code duplication, but you still have duplicate code, namely the procedure of checking whether both values are non-null and proceeding accordingly. So you could also move this logic into a separate method, for instance, into the compare(T, T) method of a separate comparator:

public int compare(T a, T b) {
    if (a == null) {
        return (b == null) ? 0 : -1;
    } else if (b == null) {
        return 1;
    } else {
        return a.compare(b);
    }
}

The good news is that you don't actually have to do this yourself, because since Java 8, there's a method Comparator.nullsFirst(Comparator) that does exactly that: It returns a comparator that considers null less than non-null (and equal to null), and if both objects are non-null, compares them using the provided comparator. The code sample above is in fact a slightly modified copy from the source of Comparator.nullsFirst(Comparator). Since you order non-null elements according to their natural ordering, you would just need to call Comparator.nullsFirst(Comparator.naturalOrder()).

So the first three comparison stages can be reduced to the functionality of these four comparators:

Comparator<String> nullsFirstStringComparator = Comparator.nullsFirst(Comparator.naturalOrder());

Comparator<HTMLLogLine> requestIdComparator = Comparator.comparing(HTMLLogLine::getRequestId, nullsFirstStringComparator);
Comparator<HTMLLogLine> securityComparator = Comparator.comparing(HTMLLogLine::getSecurity, nullsFirstStringComparator);
Comparator<HTMLLogLine> scenarioComparator = Comparator.comparing(HTMLLogLine::getScenario, nullsFirstStringComparator);

Judging by your comment, you already seem to be aware of the method Comparator.thenComparing(Comparator), so there's no need to explain how to chain those comparators using this method.

Unfortunately, the forth comparison stage does not fit into this pattern. Of course, you could ditch the nulls-first comparator and write your own generalized version of it which, instead of comparing the two values with null, checks a given Predicate against the values:

class CustomComparator<T> implements Comparator<T> {
    Predicate<? super T> predicate;
    Comparator<? super T> otherComparator;

    @Override
    public int compare(T o1, T o2) {
        if (predicate.test(o1)) {
            return (predicate.test(o2)) ? 0 : -1;
        } else if (predicate.test(o2)) {
            return 1;
        } else {
            return otherComparator.compare(o1, o2);
        }
    }
}

But I think there would only be a point in doing that if you need it more than once, because otherwise, it would not really be a remedy for code duplication, but just unnecessary code (since you don't really need it for the first three cases due to the pre-existing method Comparator.nullsFirst(Comparator)).


Coding on Point interfaces/superclasses instead of implementations From pretty much any book I've read it always said to code on interfaces/superclasses and rarely on implementations, today though that advice made a bit confused about how I should format my code.
Questions:

Should I be using Point2DAdvanced or Point2D.Double in the the methods of MedianPoint (like JavaRDD<>, the 2 private methods, the variables)? Same goes for FlagPointPairProducer.
Should I be using MedianPoint or Point2D.Double in the FlagPointPairProducer class?
Should I store the MedianPoint from the static method in a MedianPoint or Point2D.Double?

The Code:
First off I have a MedianPoint class which extends Point2D.Double. This class is responsible for outputting a median point out of a collection of points (for now only an RDD).
public class MedianPoint extends Point2D.Double {

    public MedianPoint(double x, double y) {
        super(x, y);
    }

    public static MedianPoint fromPointRDD(JavaRDD<Point2DAdvanced> points) {
        Point2DAdvanced biggestPointByXDimension = points.reduce((a, b) -> getBiggestPointByXDimension(a, b));
        Point2DAdvanced biggestPointByYDimension = points.reduce((a, b) -> getBiggestPointByYDimension(a, b));

        double xDimensionMedian = biggestPointByXDimension.getX() / 2.0;
        double yDimensionMedian = biggestPointByYDimension.getY() / 2.0;

        return new MedianPoint(xDimensionMedian, yDimensionMedian);
    }

    private static Point2DAdvanced getBiggestPointByXDimension(Point2DAdvanced first, Point2DAdvanced second) {
        return first.getX() > second.getX() ? first : second;
    }

    private static Point2DAdvanced getBiggestPointByYDimension(Point2DAdvanced first, Point2DAdvanced second) {
        return first.getY() > second.getY() ? first : second;
    }
}

I also have a FlagPointPairProducer class which according to a median point it calculates a subspace flag for a given Point2DAdvanced (another Point2D.Double extension) and returns a tuple of the flag and the point.
public class FlagPointPairProducer implements Serializable {

    private final Point2D.Double medianPoint;

    public FlagPointPairProducer(Point2D.Double medianPoint) {
        this.medianPoint = medianPoint;
    }

    public Tuple2<PointFlag, Point2DAdvanced> getFlagPointPair(Point2DAdvanced point) {
        PointFlag flag = calculateFlag(point);
        return new Tuple2<>(flag, point);
    }

    private PointFlag calculateFlag(Point2DAdvanced point) {
        double x = point.getX();
        double y = point.getY();
        double medianX = medianPoint.getX();
        double medianY = medianPoint.getY();

        int xBit = x < medianX ? 0 : 1;
        int yBit = y < medianY ? 0 : 1;

        return new PointFlag(xBit, yBit);
    }
}

So I can initialize a FlagPointPairProducer like so:
Point2D.Double medianPoint = MedianPoint.fromPointRDD(points);
FlagPointPairProducer producer = new FlagPointPairProducer(medianPoint);

Code requested for Point2DAdvanced:
public class Point2DAdvanced extends Point2D.Double implements Serializable {

    public Point2DAdvanced(double x, double y) {
        super(x, y);
    }

    public boolean dominates(Point2D.Double point) {
        return (x <= point.getX() && y < point.getY())
                || (y <= point.getY() && x < point.getX());
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append('(').append(x).append(", ").append(y).append(')');
        return builder.toString();
    }

    public static Point2DAdvanced fromTextLine(String textLine, String delimiter) {
        textLine = textLine.trim();
        String[] lineArray = textLine.split(delimiter);
        double x = java.lang.Double.parseDouble(lineArray[0]);
        double y = java.lang.Double.parseDouble(lineArray[1]);
        return new Point2DAdvanced(x, y);
    }
}

Inheritance is not to be taken lightly. Have you considered composition instead?

The MedianPoint class seems to represent a point that happens to be median in some collection of points. But this depends on the context. There is nothing in this class that could prevent creating points that are not in fact at the median. You can certainly call the constructor or a factory method with parameters that don't correspond to a median point. As such, it would make more sense to use regular point objects, and distinguish them from non-median points by naming.

The methods in this class can be static methods in a PointUtils good old fashioned utility class.

The same goes for Point2DAdvanced as well. So the answer to all your questions: use Point2D.Double everywhere. The custom point classes don't really add value, and do nothing to help reducing complexity.


How to improve this algorithm for building a graphs closures We have a large directed acyclic graph and a requirement to filter it by allowing a user to select certain nodes and then we remove all nodes that are not selected nodes, ancestors of those nodes, or descendants of those nodes.

As you can see in the above image nodes 6 and 9 have been selected and so nodes which do not route to those nodes are removed.

We are trying to build the closures (transitive closures?) of the graph so we can quickly filter it. We used the below code which was very slow. Profiling it shows around half of the time is spent in garbage collection.

public static class NodeClosureAnalyser
    {
        public static NodeClosures Analyse(Node root)
        {
            var results = new NodeClosures
                              {
                                  AncestorClosures = new Dictionary<int, HashSet<int>>(),
                                  DescendantClosures = new Dictionary<int, HashSet<int>>()
                              };
            //walk the tree and build the closures lists
            var stack = new Stack<Node>();
            stack.Push(root);

            var route = new Stack<Node>();

            while (stack.Count != 0)
            {
                var current = stack.Pop();
                //keep track of the route to root
                while (route.Any() && !route.Peek().Children.Any(n => n.Id == current.Id))
                {
                    route.Pop();
                }
                route.Push(current);

                AddNodeRelationships(results, current, route);
                foreach (var child in current.Children)
                {
                    stack.Push(child);
                }
            }
            return results;
        }

        private static void AddNodeRelationships(NodeClosures results, Node current, IEnumerable<Node> route)
        {
            foreach (var node in route)
            {
                results.AncestorClosures.GetOrCreateValuesList(current.Id).Add(node.Id);
                results.DescendantClosures.GetOrCreateValuesList(node.Id).Add(current.Id);
            }
        }
    }

    public struct NodeClosures
    {
        public Dictionary<int, HashSet<int>> AncestorClosures { get; set; }
        public Dictionary<int, HashSet<int>> DescendantClosures { get; set; }
    }

    public static class DictionaryExtension
    {
        public static HashSet<int> GetOrCreateValuesList(
            this IDictionary<int, HashSet<int>> dictionary, int key)
        {
            HashSet<int> ret;
            if (!dictionary.TryGetValue(key, out ret))
            {
                ret = new HashSet<int>(new List<int>());
                dictionary[key] = ret;
            }
            return ret;
        }
    }

A program with tests to demonstrate the issue is available on Github

Many thanks for any input!

Since your nodes are double-linked (i.e. each has a list of all parents and children), creating the list of closures for all children of a certain node (especially the root node) seems like an overkill, especially if you're doing that whenever you start to filter. If you have a doubly linked graph, descendant and ancestor lists seem pretty cheap to get for each node, and for a large graph you will have to waste an awful lot of memory to keep them around.

If I am not mistaken, for each "chosen" node, you need to mark that node as safe, mark its children as safe, then traverse back to the root and mark all nodes you encounter as safe, and finally remove all non marked nodes. That should be much less work than you're doing now, if I didn't misunderstand the problem completely.

Some minor issues:

To merge two HashSets, it's much cheaper to use HashSet.UnionWith, than to create a new HastSet each time (like you're doing in your Filter method).
Btw, by looking at your code, it seems that Node.AnyDescendant should iterate through Children, not Parents.

I.e. wouldn't this work:

public static Node Filter(Node startNode, IEnumerable<int> includedNodes)
{
    var explicitlyIncludedNodes = startNode
        .Descendants()
        .Where(n => includedNodes.Contains(n.Id));

    var nodesToKeep = new HashSet<int>();
    foreach (var node in explicitlyIncludedNodes)
    {
        nodesToKeep.UnionWith(node.Ancestors().Select(n => n.Id));
        nodesToKeep.UnionWith(node.Descendants().Select(n => n.Id));
    }

    return BreadthFirstDeletion(startNode, nodesToKeep, includedNodes.ToArray());
}

Ancestors() should simply return all ancestors all the way to the root:

public IEnumerable<Node> Ancestors()
{
    var stack = new Stack<Node>();
    stack.Push(this);
    while (stack.Count != 0)
    {
        var current = stack.Pop();
        yield return current;

        foreach (var child in current.Parents)
            stack.Push(child);
    }
}

(Descendants the same way but for Children instead of Parents)


Exception handling in URL builder method I have a method which is part of a request-handling class. It creates a URL, depending on the resource send request.

This is a part of the REST service:

private static URL addQueryToUrl(String qry) {
    try {
        if ("playerProfile".equals(requestingService)) {
            return new URL(PLAYER_URL + qry);
        } else if ("search".equals(requestingService)) {
            return new URL(SEARCH_URL   + qry);
        } else {
            throw new Exception();
        }
    } catch (MalformedURLException e) {
        System.out.println("wrong url format");
    } catch (Exception e) {
        System.out.println("no such a requestingService");
    }
    return null;
}

I have 2 questions:

What exception should I throw if requestingService is not recognized? For now, my method is throwing Exception. Or should I handle such an exception on a higher level, when requestingService has an assigned value?
If no URL is built, what value should I return? NULL? I'm now reading Clean Code by Robert C. Martin and he is stating that returning null is bad practise.

If we are speaking in terms of Clean Code, the entire method should be revised.

Here are the main points that I can criticize:

if ... else if ... branches are used to match Strings: this approach is too verbose and not very flexible.
URL is instantiated in two different places, with same semantics.
The Exception thrown in else block is immediately caught and thus is useless.
The catch blocks hide exception details and trace, which is annoying for debugging.
Returning null is ugly. A better choice would be either to return an Optional or throw the exception outside the method.

This method does two distinct things:

1) Determine which URL to use depending on the requestingService value.

2) Instantiate a URL object with query suffix.

Each of these things should be wrapped in a dedicated method.

So let's first create the getServiceUrl method:

private static String getServiceUrl() {
  switch (requestingService) {
    case "playerProfile": return PLAYER_URL;
    case "search": return SEARCH_URL;
    default: throw new IllegalStateException("Unexpected requestingService value=" + requestingService);
  }
}

Here I suppose that requestingService member of this class can take only a predefined set of values (and even should be transformed into an enum!) and any value that is not in this set denotes an internal problem that puts the application into an invalid state.

BTW, since the original method is static, I suppose that requestingService is also a static member and therefore should be renamed according to style conventions. And I also suspect that there is some misuse of statics, but can't say more about it without having the entire code.

Second, we rewrite the original method:

private static URL addQueryToUrl(String qry) {
  try {
    return new URL(getServiceUrl() + qry);
  } catch (MalformedURLException e) {
    throw new IllegalStateException(e);
  }
}

Since PLAYER_URL and SEARCH_URL are pre-validated constants, I suppose that they cannot represent a cause for a MalformedURLException. But from the original code, it is not clear whether qry is validated as part of URL pattern or not. If so, an IllegalStateException would be appropriate here. Otherwise the catch block should log the exception message and return an Optional.empty() instead (also change the return type to Optional<URL> and the return statement respectively).

I'd also suggest to rename this method to buildUrlWithQuery.


Compute the probabilities of getting each of the possible sums when throwing 2 dice, n times I'm struggling with some coding-inexperience. The code I've written below, is extremely inconvenient and ugly to look at.

My question to you is:
How can this be done more efficiently? The way I've chosen to do it, is rather inefficient. Pay attention to the def freq_2dice(n, N): section of the code, as well as the print statements. Those are the areas in which I need more efficiency as well as just nicer looking code.

The assignment is to create a function which records and stores each of the probabilities of getting each of the possible sums when throwing 2 dice, n times.

The rest of the code is comparing those probabilities to the exact probabilities.

from random import randint
import sys

def freq_2dice(n, N):
    M, A, E, R, T, Y, U, I, O, P, D = 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
    for reps in xrange(N):
        s = 0
        for dice in xrange(n):
            outcome = randint(1, 6)
            s += outcome
        if s==2:
            M += 1
        if s==3:
            A += 1
        if s==4:
            E += 1
        if s==5:
            R += 1
        if s==6:
            T += 1
        if s==7:
            Y += 1
        if s==8:
            U += 1
        if s==9:
            I += 1
        if s==10:
            O += 1
        if s==11:
            P += 1
        if s==12:
            D += 1
    return N*(float(M)/N), N*(float(A)/N), N*(float(E)/N), N*(float(R)/N), N*(float(T)/N), N*(float(Y)/N), N*(float(U)/N), N*(float(I)/N), N*(float(O)/N), N*(float(P)/N), N*(float(D)/N)

def chance_die():
    frequencies = {}
    for s in range(2, 13):
        frequency = 0
        for die1 in range(1, 7):
            for die2 in range(1, 7):
                if die1 + die2 == s:
                    frequency += 1
        frequencies[s] = frequency
    return frequencies

n = int(sys.argv[1])
N = int(sys.argv[2])

print 'No. of twos: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[0], freq_2dice(n, N)[0]/(N/100), chance_die()[2]/.36)
print 'No. of threes: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[1], freq_2dice(n, N)[0]/(N/100), chance_die()[3]/.36)
print 'No. of fours: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[2], freq_2dice(n, N)[0]/(N/100), chance_die()[4]/.36)
print 'No. of fives: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[3], freq_2dice(n, N)[0]/(N/100), chance_die()[5]/.36)
print 'No. of sixes: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[4], freq_2dice(n, N)[0]/(N/100), chance_die()[6]/.36)
print 'No. of sevens: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[5], freq_2dice(n, N)[0]/(N/100), chance_die()[7]/.36)
print 'No. of eights: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[6], freq_2dice(n, N)[0]/(N/100), chance_die()[8]/.36)
print 'No. of nines: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[7], freq_2dice(n, N)[0]/(N/100), chance_die()[9]/.36)
print 'No. of tens: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[8], freq_2dice(n, N)[0]/(N/100), chance_die()[10]/.36)
print 'No. of elevens: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[9], freq_2dice(n, N)[0]/(N/100), chance_die()[11]/.36)
print 'No. of twelves: %d, probability: %.2f, expected: %.2f' % (freq_2dice(n, N)[10], freq_2dice(n, N)[0]/(N/100), chance_die()[12]/.36)

MacBook-Air:python Leroy$ python freq_2dice.py 2 100000
No. of twos: 2680, probability: 2.80, expected: 2.78
No. of threes: 5612, probability: 5.51, expected: 5.56
No. of fours: 8169, probability: 8.43, expected: 8.33
No. of fives: 11099, probability: 10.96, expected: 11.11
No. of sixes: 13827, probability: 13.91, expected: 13.89
No. of sevens: 16610, probability: 16.51, expected: 16.67
No. of eights: 13808, probability: 13.72, expected: 13.89
No. of nines: 10947, probability: 11.22, expected: 11.11
No. of tens: 8249, probability: 8.35, expected: 8.33
No. of elevens: 5540, probability: 5.59, expected: 5.56
No. of twelves: 2805, probability: 2.74, expected: 2.78

'''

First thing to note is that since you only check up to s=12, your code won't work for more than 2 dice. What you really need is a list so how high you go can be a variable.  Also N*(float(M)/N) seems the same as float(M), so I'm not sure what you're trying to do there.

def freq_2dice(n, N):
    dice_list = [0] * (n*6+1)
    for reps in xrange(N):
        s = 0
        for dice in xrange(n):
            outcome = randint(1, 6)
            s += outcome
        dice_list[s] += 1
    return dice_list

I rewrote your freq_2dice using an list.  My version is a little different because it'll return 0 through 6*n instead of 2 through 12.  dice_list = [0] * (n*6+1) lets me start with a list populated with all zeroes that is n*6+1 long (since it starts with 0 but I still want to include 6*n in the list)

Next, you're running the really expensive part of your program (freq_2dice) 22 times! You're actually running it twice every line, so that the number of 2's won't match the corresponding probability because those are two different runs.  Running it the first time provides all of the information you need.

Instead of all your print statements, a loop would be good there too.  So first, I'm going to run each function just ONCE instead of repeatedly and I'll store the results. Then I'll use a loop for the print statement (which could let you make the print range dynamic for more than 2 dice as well, but I'll just keep it simple here)

freq_2_dice_results = freq_2dice(n, N)
dice_chance_results = chance_die()
for dice_value in range(2, 13):
    print 'No. of %d: %d, probability: %.2f, expected: %.2f' % (
        dice_value,
        freq_2_dice_results[dice_value],
        float(freq_2_dice_results[dice_value])/(N/100),
        dice_chance_results[dice_value]/.36)

I hope that gives you a good place to start.


If LIST were a two-dimensional numpy array, you could use
LIST[i,:] to get the ith row,
LIST[:,i] to get the ith column,
numpy.diagonal(LIST, i) to get the diagonal i places above the main diagonal (where -17 < i and i < 17 in order to restrict to diagonals containing at least four cells),
and numpy.diagonal(numpy.flipud(LIST), i) to get the left diagonal i places below the main left diagonal.

Each of the four operations in the previous paragraph returns either an array or an array view.
Every set of four numbers that you wanted to look at will show up as four consecutive values in one of these arrays or array views,
so you just need to look at each product of four consecutive integers
within each array or array view.


Given suffix array and a pattern, search if given pattern exists Given a suffix array for a word, check if a pattern (consecutive chars) exists.

  Example:

  A suffix array constructed for "ameya" should return true for "ame" but false for "foo" or false for "ameyp".

  This algoritm is case sensitive.

I'm looking for code-review, best practices and optimizations.

public final class SuffixArrayPatternMatch {

    private final String[] suffixArray;

    public SuffixArrayPatternMatch(String[] suffixArray) {
        this.suffixArray = suffixArray;
    }

    /**
     * Returns  0, if the string starts with the pattern.
     * Returns  1, if the string is greater than 1.
     * Returns -1, if the string is smaller than pattern.
     *
     *
     * @param str           the string to be matched with the pattern.
     * @param pattern       the pattern to be searched in the string
     * @return              -1, 0, or 1.
     */
    private int compare(String str, String pattern) {
        int length = str.length() < pattern.length() ? str.length() : pattern.length();

        for (int i = 0; i < length; i++) {
            if (str.charAt(i) < pattern.charAt(i)) return -1;
            if (str.charAt(i) > pattern.charAt(i)) return  1;
        }

        // string was greater than myself. thus return 0.
        return str.length() < pattern.length() ? -1 : 0;
    }

    private boolean binarySearch(int lb, int hb, String pattern) {
        if (lb > hb) {
            return false;
        }
        int mid = (lb + hb) / 2;

        int match = compare(suffixArray[mid], pattern);

        if (match == 0) return true;

        if (match > 0) {
            return binarySearch(lb, mid -1, pattern);
        } else {
            return binarySearch(mid + 1, hb, pattern);
        }
    }

    /**
     * Checks the pattern if present in the string
     *
     * @param pattern   the pattern to be searched in the string
     * @return          true if pattern is found else false.
     */
    public boolean search (String pattern) {
        return binarySearch(0, suffixArray.length - 1, pattern);
    }
}

public class SuffixArrayPatternArrayTest {

    @Test
    public void testTrue() {
        String[] suffix = {"a", "ameya", "eya", "meya", "ya"};
        SuffixArrayPatternMatch sapm = new SuffixArrayPatternMatch(suffix);

        for (int i = 0; i < suffix.length; i++) {
            assertTrue(sapm.search(suffix[i]));
        }

        assertTrue(sapm.search("mey"));
        assertTrue(sapm.search("ame"));
    }

    @Test
    public void testFalse() {
        String[] suffix = {"a", "ameya", "eya", "meya", "ya"};
        SuffixArrayPatternMatch sapm = new SuffixArrayPatternMatch(suffix);

        assertFalse(sapm.search("foo"));
        assertFalse(sapm.search("yap"));
        assertFalse(sapm.search("ameyap"));
    }
}

Since you have a hidden implementation requirement that the suffix be sorted (i.e. binary search), you should sort the array before assigning it to the member variable.

Your compare implementation differs from that of the String class in that it returns 0 if str starts with pattern. Why not code it that way?

private int compare(String str, String pattern) {
     return str.startsWith(pattern) ? 0 : str.compareTo(pattern);
}


RESTful API echo server using Python bottles I just wrote an echo server that responds to curl requests with the method used and the data sent. Sticking to GET and POST for now, but I would like to know if there's something I can do to improve my RESTful API.

I'm supposed to:

Only be able to call the /data endpoint
Only allow JSON parameters
Use best practices of coding a RESTful API

Expected JSON response:

{
  "method": <HTTP METHOD REQUESTED>,
  "data": <HTTP PARAMETERS SENT IN THE REQUEST>
}

Here is my current code:

from bottle import run, post, request, response, get, route, put, delete
from json import dumps
import json

@get('/data')
def data():
    #if headers are not specified we can check if body is a json the following way
    #postdata = request.body.read()
    postdata = request.json
    try:
        #rv = {"method": request.method, "data": json.loads(postdata.decode('utf-8'))}
        rv = {"method": request.method, "data": postdata}
    except:
        raise ValueError
    response.content_type = 'application/json'
    return dumps(rv)

@post('/data')
def data():
    #postdata = request.body.read()
    postdata = request.json
    try:
        #rv = {"method": request.method, "data": json.loads(postdata.decode('utf-8'))}
        rv = {"method": request.method, "data": postdata}
    except:
        raise ValueError
    response.content_type = 'application/json'
    return dumps(rv)

Everything seems to work fine for now. I'm looking to improve my code so any feedback is appreciated.

The code looks a bit incomplete, e.g. shouldn't there be a main method
somewhere?

I added the following to get it to run for now:

if __name__ == "__main__":
    run()

It's good practice to have the __name__ check in there instead of just
executing, because then you're still able to import the file elsewhere
without it automatically executing code.

Okay, so with that out of the way, there's commented out code that
should be there:  Either it's correct, then it should replace the
uncommented code, or it's not, then it should be deleted.  To me,
reading that just confuses me what the purpose is, or was.

And then there's two functions, which are identical ... why is that?  I
suppose that's a feature of the framework that allows you to have both
functions because of the annotations, but generally do not do that and
give them separate names, at least something like
data_get/data_post.

The specification is pretty vague, what are you supposed to echo, the
POST body, or the URL parameters?  Because then I don't get what the
GET one is supposed to be doing.  With that in mind I'd delete it.

So, then, trying it out:

> curl -d '{"data": 1}' -H "Content-Type: application/json" -v http://localhost:8080/data
{"method": "POST", "data": {"data": 1}}%

Seems to work.

(Also, since there's a single endpoint that echoes data ... there's
little REST here.  I'm not sure what other best practices are meant to
be followed for this exercise.)

I've no idea how you'd get an exception in there, but in any case,
raise ValueError isn't super helpful, it says exactly nothing about
what went wrong.  Consider adding some information like raise
ValueError("Couldn't format data.").  Note that in this particular
situation there's little that adds information, maybe don't catch the
exception then.

At that point it might look like this:

@post("/data")
def data():
    response.content_type = "application/json"
    return dumps({"method": request.method, "data": request.json})

However, then I was thinking, maybe this framework does the JSON
formatting for us.  And it does:

@post("/data")
def data():
    return {"method": request.method, "data": request.json}

Probably can't get it much smaller than that actually.  And then you can
delete a lot of imports too:

from bottle import run, request, post
from json import dumps

Hope that helps.  In general it's good to let readers know all you can,
like "which dependencies do I need" and "how do I run this", including
example invocations.


A program to display, update and save a dictionary as .csv I'm now looking for feedback on v2.0 of this program instead.

I'd like some feedback about my code. What bad habits do I have? What advice could help me write more Pythonic?

I'm trying to write a console application and want a tool to display, update and save settings in a dictionary.

def file_as_string(file_name):
    with open(file_name, 'r') as opened_file:
        string = opened_file.read().replace('\n', '')
        return string

def csv_as_dict(file_name):
    file_name = file_name + '.csv'
    import csv
    dictionary = {}
    for key, value in csv.reader(open(file_name)):
        dictionary[key] = value
    return dictionary

def print_formatted_dict(dictionary, key_title , value_title):
    print "{:^30} {:^15}".format('%s', '%s') % (key_title, value_title)
    for setting, value in dictionary.items():
        print "{:<30} {:<15}".format(setting, value)

def generic_prompt(prompt):
    value = raw_input(prompt)
    return value

def prompt_for_item(prompt, item):
    prompt = prompt + ' ' + item + ': '
    value = raw_input(prompt)
    return value

def change_dict_value(dictionary, prompt, item):
    value = prompt_for_item(prompt, item)
    dictionary[item] = (value)
    return dictionary

def csv_save_dict(dictionary, file_name):
    file_name = file_name + '.csv'
    import csv
    csv_file = csv.writer(open(file_name, "w"))
    for key, value in dictionary.items():
        csv_file.writerow([key, value])

def change_dict(dictionary):
    specify_question = 'Specify'
    change_question = 'Change settings? '
    setting_question = 'Setting to change: '
    save_question = 'Save settings? '

    change_answer = generic_prompt(change_question)
    while change_answer not in yes_no:
        change_answer = generic_prompt(change_question)
    while change_answer == 'yes':
        change_item = generic_prompt(setting_question)
        while change_item not in dictionary:
            change_item = generic_prompt(setting_question)
        dictionary = change_dict_value(dictionary, specify_question, change_item)
        print_formatted_dict(dictionary, key_title, value_title)
        change_answer = generic_prompt(change_question)
        while change_answer not in yes_no:
            change_answer = generic_prompt(change_question)
        if change_answer == 'no':
            save_answer = generic_prompt(save_question)
            while save_answer not in yes_no:
                save_answer = generic_prompt(save_question)
            if save_answer == 'yes':
                filename_answer = generic_prompt(filename_question)
                csv_save_dict(dictionary, filename_answer)
    return dictionary

def load_dict(dictionary):
    load_answer = generic_prompt(load_question)
    while load_answer not in yes_no:
        load_answer = generic_prompt(load_question)
    if load_answer == 'yes':
        filename_answer = generic_prompt(filename_question)
        dictionary = csv_as_dict(filename_answer)
        print_formatted_dict(dictionary, key_title, value_title)
    if load_answer == 'no':
        pass
    return (dictionary, load_answer)

def load_change_loop(dictionary, load_answer):
    while load_answer == 'yes':
        dictionary = change_dict(dictionary)
        dictionary, load_answer = load_dict(dictionary)
    return dictionary

def main():
    default_dict = csv_as_dict(default_settings)
    print_formatted_dict(default_dict, key_title, value_title)
    dictionary = load_change_loop(default_dict, load_answer)
    return dictionary

#string variables
default_settings = 'default_settings'
load_question = 'Load a dictionary? '
filename_question = 'File name: '
key_title = 'Setting'
value_title = 'Value'
load_answer = 'yes'

#list variables
yes_no = ['yes', 'no']

main()

The console with highlighted user input looks like this:

The program begins by reading a default_settings.csv file, mine looks like this:

It's a pretty good program for a beginner.

A few points:

Put your import csv once at the top of the file, not in the functions that use csv. Importing is best thought of as an action at the start of your program, not as something that happens while performing an action.

You do this a lot:

while load_answer not in yes_no:
    load_answer = generic_prompt(load_question)
if load_answer == 'yes':

Anytime you find yourself repeating code, it'd be helpful to write a function instead. Something like:

   def ask_boolean_question(prompt):
       answer = ''
       while answer not in yes_no:
           answer = generic_prompt(prompt)
       return answer == 'yes'

Now your code can simply say:

  if ask_boolean_question(load_question):

And avoid repeating the loop everytime you ask a question.

Your program doesn't make its structure very clear. Given what your program does, your main function should read something like:

def main():
    manipulate_settings(default_settings)
    while ask_boolean_question(load_question):
         filename = generic_prompt(filename_question)
         manipulate_settings(filename)

Note that by reading this, you can see the program processes first the default settings, and then as many other settings files as you have inclination to use the program.


Slow RegEx to parse from log file I have the following lines to parse from a log file with 8000 lines which takes about 5 minutes to parse:

text="2017-01-12 15:16:42,404 - RestLogger[10c059f7] - INFO - rest_server::log_request:102 - REST call: <POST /multi/ HTTP/1.1> with args {} and content {u'inv_initiators': {u'path': u'/v2/types/initiators/', u'args': }} from user admin @ 10.106.97.145\n"
text+="2016-10-06 20:58:04,025 - RestLogger - INFO - rest_server::log_request:98 - REST call: <GET /types/volumes/59 HTTP/1.1> with args {}\n"

re.findall('([12]\d{3}-[0-3]\d-[01]\d \d{2}:\d{2}:\d{2},\d{3}) .*rest_server::log_request:\d+ - REST call: <(\w+) (.*) .+?> with args ({.*?})(?: and content ({.*}))?.*?\n', text)

Since I would like to grab lines with optional text "and content" I am adding ?: and ? to grab the lines in one regex and the results are really slow.

Any advice how to make it faster?

Seems like instead of re.findall, you should be using re.match on each line of the input individually. Something like this:

for line in text.splitlines():
    m = re.match(r'([12]\d{3}-[0-3]\d-[01]\d \d{2}:\d{2}:\d{2},\d{3}) .*rest_server::log_request:\d+ - REST call: <(\w+) (.*) .+?> with args ({.*?})(?: and content ({.*}))?.*?', text)
    # and then do something with m

The main problem with your original regex is that the first instance of .* could reasonably match a whole lot of the string; if you're not going to split the lines apart first, then at least you'd want to change .* to [^\n]* and so on throughout.

As a micro-optimization, you may find that re.compileing your regex first produces faster results; and if you're not compiling it ahead of time, I wouldn't be surprised if

([12]\d{3}-[0-3]\d-[01]\d \d{2}:\d{2}:\d{2},\d{3})

is outperformed by a simple

(\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d,\d\d\d)

Basically, don't try to be clever and you'll do fine.


Code to remove columns in a (fasta) file with more than 90% '-' characters in that column Context:

This script reads in a fasta file, counts the number '-' characters, alphabetic characters and total characters for each sequence (sequence descriptions are on lines marked with '>'), and then prints out a processed file with columns where 90% (or more) of '-' characters are removed.

I've tested it with smaller alignments and it worked fine, but it's currently been running for around 4 hours on one alignment (220Mb), which seems slow and probably inefficient.

The script:

import sys
from Bio import AlignIO

alignment = AlignIO.read(open(sys.argv[1]), "fasta")
length =  alignment.get_alignment_length()

tofilter = []

for i in range(length):
    counterofgaps=0
    counterofsites=0
    counteroftotal=0
    for record in alignment:
        for site in record.seq[i]:
            if site == '-':
                counterofgaps +=1
                counteroftotal +=1
            elif site.isalpha():
                counterofsites +=1
                counteroftotal +=1
    Fractionofgaps = counterofgaps/counteroftotal * 100
    if Fractionofgaps >=90:
        tofilter.append(i)

for record in alignment:
    print ('>' + record.id)
    for i in range(length):
        if i not in tofilter:
            print (record.seq[i], end ='')
    print()

Disregarding using a better parser (biopython's SeqIO), here are some immediate speed boosts due to better use of vanilla Python:

You don't actually need the counterofsites at all. If you only want the fraction of '-', this is the same as the average of a sequence of 1 and 0, where the value is 1 (or equivalently, True) if the character is '-':

from statistics import mean

def gap_fraction(alignment, i):
    return mean(site == "-" for record in alignment for site in record.seq[i])

This uses a generator expression to flatten the sites.

The other improvement is using a set instead of a list for the to be filtered elements. This is needed since you later do if i not in tofilter, which needs to scan through the whole list in the worst case. With a set this is immediate, i.e. it is \$\mathcal{O}(1)\$ instead of \$\mathcal{O}(n)\$. You will only see a real difference if your number of columns to filter gets large (>>100), though.

to_filter = {i for i in range(length) if gap_fraction(alignment, i) > 0.9}

I also used a set comprehension to make this a lot shorter and followed Python's official style-guide, PEP8, which recommends using lower_case_with_underscores for variables and functions.

You should also keep your calling code under a if __name__ == "__main__": guard to allow importing from this script form another script without running the code.


Using SHA-512 for encryption/decryption It occurred to me that if SHA2 can be used to derive keys from passwords, then it might as well be good enough to generate random data that can be xored with a plaintext to encrypt and the other way around.

These are my assumptions:

SHA-512 produces a random-looking output that is impossible to guess
The result of SHA-512 can be fed back into it appended to a 256-bit key and produce an output with the same quality as given for a random input

If those assumptions hold, then it should provide privacy. It's much faster than AES-256-CBC.

At first I thought there must be something I'm missing, but no one has pointed out an attack that can be carried out against this construction. So what I would like to know is how secure this algorithm is and specific ways to break it.

Here's the code:

mad.h

#ifndef _MAD_H_
#define _MAD_H_

typedef struct {
  unsigned char state[64];
  unsigned char key[32];
} MadCtx;

void mad_ctx_init(MadCtx* mad, unsigned char const* key,
                  unsigned char const* iv);

void mad_encrypt(MadCtx* mad, unsigned char const* in, unsigned int in_size,
                 unsigned char* out);

void mad_decrypt(MadCtx* mad, unsigned char const* in, unsigned int in_size,
                 unsigned char* out);

#endif

mad.c

#include "mad.h"
#include <stdint.h>
#include <assert.h>
#include <string.h>
#include <openssl/sha.h>

// Private
static void _xor64(uint64_t* dest, uint64_t const* a, uint64_t* b)
{
  for(int i = 0; i < 8; ++i)
    *dest++ = *a++ ^ *b++;
}

// Public
void mad_ctx_init(MadCtx* mad, unsigned char const* key,
                  unsigned char const* iv)
{
  memcpy(mad->state, iv, 64);
  memcpy(mad->key, key, 32);
}

void mad_encrypt(MadCtx* mad, unsigned char const* in, unsigned int in_size,
                 unsigned char* out)
{
  assert(0 == in_size % 64);

  int n = in_size >> 6; // in_size / 64
  while(n){
    uint64_t x[8];
    SHA512((unsigned char const*)mad, 96, (unsigned char*)x);
    _xor64((uint64_t*)out, (uint64_t const*)in, x);
    memcpy(mad->state, out, 64);
    in += 64;
    out += 64;
    --n;
  }
}

void mad_decrypt(MadCtx* mad, unsigned char const* in, unsigned int in_size,
                 unsigned char* out)
{
  assert(0 == in_size % 64);

  int n = in_size >> 6; // in_size / 64
  while(n){
    uint64_t x[8];
    SHA512((unsigned char const*)mad, 96, (unsigned char*)x);
    memcpy(mad->state, in, 64);
    _xor64((uint64_t*)out, (uint64_t const*)in, x);
    in += 64;
    out += 64;
    --n;
  }
}

and some test code

#include "mad.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <assert.h>

void phex(void const* data, size_t size)
{
  char const* table = "0123456789abcdef";
  unsigned char const* in = data;
  while(size--){
    int c;
    c = table[*in >> 4];
    putchar(c);

    c = table[*in & 0xf];
    putchar(c);
    ++in;
  }
}

void read_or_die(FILE* file, void* dest, size_t size)
{
  if(size != fread(dest, 1, size, file)){
    perror("fread()");
    exit(EXIT_FAILURE);
  }
}

int main(int argc, char* argv[])
{
  FILE* urandom = fopen("/dev/urandom", "r");
  unsigned char iv[64];
  unsigned char key[32];
  unsigned char plain[128];
  unsigned char cipher[128];
  unsigned char decrypted[128];

  read_or_die(urandom, iv, 64);
  read_or_die(urandom, key, 32);
  memset(plain, 0xdd, 128);

  puts("plain text is:");
  for(int i = 0; i != 128; i += 32){
    phex(plain + i, 32);
    putchar('\n');
  }
  putchar('\n');

  MadCtx ctx;
  mad_ctx_init(&ctx, key, iv);
  mad_encrypt(&ctx, plain, 128, cipher);

  puts("cipher text is:");
  for(int i = 0; i != 128; i += 32){
    phex(cipher + i, 32);
    putchar('\n');
  }
  putchar('\n');

  assert(0 != memcmp(plain, cipher, 128));

  mad_ctx_init(&ctx, key, iv);
  mad_decrypt(&ctx, cipher, 128, decrypted);

  puts("decrypted text is:");
  for(int i = 0; i != 128; i += 32){
    phex(decrypted + i, 32);
    putchar('\n');
  }
  putchar('\n');

  assert(0 == memcmp(plain, decrypted, 128));
}

The above encrypt/decrypt functions can be summarized as follows:

void mad_encrypt(...)
{
  while(n)
  {
    SHA512(mad, 96, x);
    XOR(out, in, x);
    memcpy(mad->state, out, 64);
    ...
  }
}

void mad_decrypt(...)
{
  while(n)
  {
    SHA512(mad, 96, x);
    XOR(out, in, x);
    memcpy(mad->state, in, 64);
    ...
  }
}

This is basically the CFB mode. Note that the two functions are almost identical, except a small difference in memcpy.

AES in CFB mode is done in much the same way, except of course it uses AES block cipher instead of SHA512(...) function above. Also AES uses blocks of 16 bytes, so AES has to run 4 rounds to catch up with a single SHA512 round. Overall, AES is faster.

To compare the performance you can simply compare 4 rounds of AES to 1 round of SHA512. Watch out for compiler optimizations which may skew the result. Many tests have already been done for this, it shows AES is faster.

AES also uses key expansion which makes it more secure. Key expansion is relatively slow but it's done only once per file/data. This may skew the performance test depending on how the test is done.

typedef struct {
  unsigned char state[64];
  unsigned char key[32];
} MadCtx;

For improvement, don't let the key linger on in MadCtx::key. For example, you can use SHA512 to combine the key with IV (or state as you call it) so it becomes hidden during the operation, then you can drop key out of the structure.

Example:

unsigned char mad_IV[64];

void mad_init(const unsigned char *key, const unsigned char* iv)
{
    unsigned char buf[96];
    memcpy(buf, iv, 64);
    memcpy(buf + 64, key, 32);
    SHA512(buf, 96, mad_IV);
}

void mad_crypt(char const* in, int in_size, char* out, int encrypt)
{
    assert(0 == in_size % 64);

    int n = in_size >> 6; // in_size / 64
    while (n)
    {
        SHA512(mad_IV, 64, mad_IV);
        _xor64((uint64_t*)out, (uint64_t*)in, (uint64_t*)mad_IV);

        if (encrypt)
            memcpy(mad_IV, out, 64);
        else
            memcpy(mad_IV, in, 64);

        in += 64;
        out += 64;
        --n;
    }
}

int main()
{
    unsigned char iv[64] = { 0 };
    unsigned char key[32] = { 0 };
    memcpy(iv, "iv", 2);
    memcpy(key, "key", 3);

    int size = 640;
    char *plaintext = malloc(size);
    char *decrypted = malloc(size);
    char *encrypted = malloc(size);

    memset(plaintext, 0, size);
    memset(encrypted, 0, size);
    memset(decrypted, 0, size);

    strcpy_s(plaintext, size, "plainxxxxxx.");

    mad_init(key, iv);
    mad_crypt(plaintext, size, encrypted, 1);

    mad_init(key, iv);
    mad_crypt(encrypted, size, decrypted, 0);

    phex(encrypted, 64);
    printf("\n\n");

    printf("plaintext: %s\n", plaintext);
    printf("decrypted: %s\n", decrypted);

    putchar('\n');

    return 0;
}


Run directory scan as Class I've progressed with the directory statistics function and want to proceed by making it a class.

Link to the first version of this code: Link

My current state of things:

About my namings:
I first used the name fcount as I wanted to have it stand for files and folders as otherwise, the name would be too long. That's why I made the exception to shorten it. I'm still continuing with your more experienced solution for this.
I wrote pathlib into the function name because I have the same function above it with os.walk as this was my first way to try. But os.walk seem to have problems scanning my network drive as it always returned 0 bytes. Therefore I've chosen pathlib. Hope that makes sense.

About my classes: I'm starting to feel comfortable programming python but as soon as I start to use classes my whole code starts to fall apart and seems to have to be more complex. I know that's just a beginner problem, but as I usually can't solve the issues appearing, I'm careful with that route. I've now rewritten it into a class but facing a few problems now.
I started to try to structure it with the tips from the first CodeReview by writing the file search for-loop into the __init__ function but python was then saying it can't return a value from __init__ so I created a new method named def get_directory_statistics(self, scan_path):.
I'm not sure where to input my scan_path, into the __init__or the first method def get_directory_statistics(self, scan_path):.
Your advice to summarize two lines into one, sadly didn't work for me either return size_and_file_count(size_gb, all_types_count, file_count, folder_count). I couldn't get it to work. It's always saying size_and_file_count is not defined or other Errors.

Optimizing the code: I outlined above why I sadly can't use os.walk for this. So this won't work for me. And C seems at the moment, not like an option as the only programming language I am familiar with is python and I guess it would be a more complex task to program a wrapper and the code itself in C. I think most of it will be I/O bound, yes.

Again I learned a lot from the first answer here on CodeReview!

Below you'll find my solution after going over all the last notes

class get_size_and_file_count:
    """Gets the total size of a given dir and counts how many folders and files are in the given
        path directory and return a file count, folder count and all non hidden files as a sum"""
    def __init__(self, total_size = 0, non_hidden_files_count = 0, file_count = 0, folder_count = 0):
        self.total_size = total_size
        self.non_hidden_files_count = non_hidden_files_count
        self.file_count = file_count
        self.folder_count = folder_count

    def get_directory_statistics(self, scan_path):
        self.root_directory = Path(scan_path)
        for f in self.root_directory.glob('**/*'):
            if f.is_file():
                self.file_count += 1
                self.total_size += f.stat().st_size
                if not f.name.startswith("."):
                    self.non_hidden_files_count += 1
            if f.is_dir():
                self.folder_count += 1

        directory_statistics = [self.total_size, self.non_hidden_files_count, self.file_count, self.folder_count]
        return directory_statistics

    def print_directory_statistics(self):
        print('Directory path to search:     {}'.format(self.root_directory))
        print('Directory size in GB:         {:.2f}GB'.format(self.total_size / 1.0e9))
        print('Amount of non hidden files:   {}'.format(self.non_hidden_files_count))
        print('Amount of files searched:     {}'.format(self.file_count))
        print('Amount of folders searched:   {}'.format(self.folder_count))

result = get_size_and_file_count()
directory_statistics = result.get_directory_statistics("...") # Store directory statistics in var
result.print_directory_statistics()  # Print directory statistics

Style conventions

Following the PEP style guide, class names should be named with CamelCase and documentation strings should be formatted like the following.

class DirectoryStatistics:
    """Gets the total size of a given dir and counts how many folders and
    files are in the given path directory. Also offers a printing utility to output
    the diagnosis results.
    """
    def __init__(self, ...):
        # etc.

Classes

You can't return values from __init__ (aka. the class constructor), because it is called when you instantiate an object, thus the return value is the object itself. But you can call methods in your __init__ method, that's why you should move the content of your get_directory_statistics method into the __init__ method:

class DirectoryStatistics:
    """Gets the total size of a given dir and counts how many folders and
    files are in the given path directory. Also offers a printing utility to output
    the diagnosis results.
    """
    def __init__(self, file_path):
        self.root_directory = path(file_path)
        self.file_count = 0
        self.total_size = 0
        self.non_hidden_files_count = 0
        self.folder_count = 0
        for f in self.root_directory.glob('**/*'):
            if f.is_file():
                self.file_count += 1
                self.total_size += f.stat().st_size
                if not f.name.startswith("."):
                    self.non_hidden_files_count += 1
            if f.is_dir():
                self.folder_count += 1

That way, by calling:

statistics = DirectoryStatistics(file_path)

you run the directory diagnosis and save the results in your object.
Then you can pretty-print the results using your print_statistics() method.


MMMRjs a product of BYTES Genesis We have published a product today called MMMR.js this product is licensed under the MIT license and is built by Syed Mohammad Sannan.
The uniqueness of this product is that it is built with and for Javascript though there are many libraries out there such as NumPY, SciPY, and much much more but they are built for Python or another language and there isn't one for JavaScript that is where MMMR.js comes.
here.
This is the source code of MMMR.js:
// Mean - The average value.
// Median - The mid point value.
// Mode - The most common value.
// Range The smallest and the biggest value.

// This is Mean.
// Mean returns the average value from an array of numbers.
// To call/run this function we can type mean(Your array of numbers separated with commas ',');
const mean = (numbers) => {
  let total = 0,
    i;
  for (i = 0; i < numbers.length; i += 1) {
    total += numbers[i];
  }
  return total / numbers.length;
};

// This is Median.
// Median returns the middle point value from an array of numbers.
// To call/run this function we can type median(Your array of numbers separated with commas ',');
const median = (numbers) => {
  const sorted = numbers.slice().sort((a, b) => a - b);
  const middle = Math.floor(sorted.length / 2);

  if (sorted.length % 2 === 0) {
    return (sorted[middle - 1] + sorted[middle]) / 2;
  }

  return sorted[middle];
};

// This is Mode.
// Mode returns the most common/used value from an array of string, numbers, etc.
// To call/run this function we can type mode(your array of numbers);
const mode = (a) => {
  a.sort((x, y) => x - y);

  let bestStreak = 1;
  let bestElem = a[0];
  let currentStreak = 1;
  let currentElem = a[0];

  for (let i = 1; i < a.length; i++) {
    if (a[i - 1] !== a[i]) {
      if (currentStreak > bestStreak) {
        bestStreak = currentStreak;
        bestElem = currentElem;
      }

      currentStreak = 0;
      currentElem = a[i];
    }

    currentStreak++;
  }

  return currentStreak > bestStreak ? currentElem : bestElem;
};

// This is Range.
// Range is basically a function used to return the smallest and the biggest values from an array of numbers.
// To call/run this function we can type range(your array of numbers);
const range = (value) => {
  value.sort();
  return [value[0], value[value.length - 1]];
}

We just wanted you to review this code and tell us is this library user-friendly, maintainable and what improvements can we add to this.

Comments

The comments such a This is Mean. are pointless.

And the comment

// To call/run this function we can type mean(Your array of numbers separated with commas ',');

is a bit strange. Are they targeting people who don't know basic JavaScript syntax? Then why mention commas, but not the brackets ([...]) needed for an array.

It probably would be a good idea to instead use a standard comment format for documentation such as jsdoc.

Sorting

The comparison function used for sorting ((a, b) => a - b) is pointless, because that is the default.

Mutating the input array

In median you are creating a copy of the input array with .slice() before sorting it, so that it isn't modified, which is a good thing. You should do the same in mode and range.

Validating the input

You should consider validating the input. mean, mode and range break or give unexpected results if you give them an empty array. It's probably best to check if they are receiving an array at all.


Preorder, Postorder, inorder Traversal in an Binary Search Tree This is code for Inorder, Preorder, Postorder Traversal of a Tree. Suggest some ways to improve it (since I have made root public and want to make it Private).

#include<iostream.h>
#include<conio.h>

struct Node
{
  int data;
  Node *left;
  Node *right;
};

class Tree
{
  private:
    Node *p;
  public:
    Node *root;
    Tree()
    {
      root=NULL;
    }
    void Insertion();
    void Traversal();
  protected:
    void SearchPos(Node *);
    void Inorder_Traversal(Node *);
    void Preorder_Traversal(Node *);
    void Postorder_Traversal(Node *);
};

void Tree::Insertion()
{
  p=new Node;
  p->left=p->right=NULL;
  if(root==NULL)
  {
    cout<<"Enter Root Value to be inserted:";
    cin>>p->data;
    root=p;
  }
  else
  {
    cout<<"\nEnter Value to be inserted:";
    cin>>p->data;
    SearchPos(root);
  }
}

void Tree::SearchPos(Node *tmp)
{
  if(p->data<tmp->data && tmp->left==NULL)
  {
    tmp->left=p;
    return;
  }
  else if(p->data>tmp->data && tmp->right==NULL)
  {
    tmp->right=p;
    return;
  }
  else if(p->data<tmp->data && tmp->left!=NULL)
  {
    SearchPos(tmp->left);
  }
  else if(p->data>tmp->data && tmp->right!=NULL)
  {
    SearchPos(tmp->right);
  }
}

void Tree::Traversal()
{
  cout<<"\nInorder Traversal:";
  Inorder_Traversal(root);
  cout<<"\nPreorder Traversal:";
  Preorder_Traversal(root);
  cout<<"\nPostorder Traversal:";
  Postorder_Traversal(root);
}

void Tree::Inorder_Traversal(Node *tmp)
{
  if(tmp->left==NULL && tmp->right==NULL)
  {
    cout<<tmp->data<<" ";
    return;
  }
  else if(tmp->left!=NULL)
  {
    Inorder_Traversal(tmp->left);
  }
  cout<<tmp->data<<" ";
  if(tmp->right!=NULL)
  {
    Inorder_Traversal(tmp->right);
  }
}

void Tree::Preorder_Traversal(Node *tmp)
{
  if(tmp->left==NULL && tmp->right==NULL)
  {
    cout<<tmp->data<<" ";
    return;
  }
  else if(tmp->left!=NULL)
  {
    cout<<tmp->data<<" ";
    Preorder_Traversal(tmp->left);
  }
  if(tmp->right!=NULL)
  {
    Preorder_Traversal(tmp->right);
  }
}

void Tree::Postorder_Traversal(Node *tmp)
{
  if(tmp->left==NULL && tmp->right==NULL)
  {
    cout<<tmp->data<<" ";
    return;
  }
  else if(tmp->left!=NULL)
  {
    Postorder_Traversal(tmp->left);
  }
  if(tmp->right!=NULL)
  {
    Postorder_Traversal(tmp->right);
    cout<<tmp->data<<" ";
  }
}

void main()
{
  clrscr();
  char choice;
  Tree t;
  do
  {
    t.Insertion();
    cout<<"\nMore to Insert???Enter(y/n):";
    cin>>choice;
  }while(choice=='y'||choice=='Y');
  t.Traversal();
  getch();
}

Recursion

Test if the current node is valid (return if it is not).
Processes node or Recurse

All your functions can be written much more succinctly and easier to read if you follow those rules.

void Tree::preorderTraversal(Node *tmp)
{
    // Always check to see if the node is valid first.
    if (tmp == nullptr) {
        return;
    }

    //processes your data (this could come after recursion or between the calls depending on the type of the traversal needed).
    std::cout << tmp->data << " ";

    // Recursion.
    preorderTraversal(tmp->left);   // Don't care if these are null
    preorderTraversal(tmp->right);  // This will be checked at the beginning of the next call.
}

Code Review

Make this part of Tree class (ie declare this as a private member of Tree). There is no reason for anbody outside the tree to know or use this class.

struct Node
{
  int data;
  Node *left;
  Node *right;
};

Why is root public?

  public:
    Node *root;

This breaks encapsulation and allows a non member to mutate the state of the class without the class knowing or understanding about the mutation.

    Tree()
    {
      root=NULL;
    }

Your Tree class does not obey the rule of Three or Five. This needs to be fixed immediately. Currently it does not break much because you have missed the destructor but it will lead to unexpected results when you copy and mutate the tree.

Also because you don't have a destructor you are leaking memory. You basically need to add the following methods to your Tree class.

     ~Tree();
     Tree(Tree const& rhs);
     Tree& operator=(Tree const& rhs);

You should avoid protected. Though it has its uses these are mostly advanced reasons to use protected. Stick to private and public.

  protected:

Note: As all these expose Node these should be private members. Your interface to the public should not expose internal types and Node is an internal type that affects the implementation.

    void SearchPos(Node *);
    void Inorder_Traversal(Node *);
    void Preorder_Traversal(Node *);
    void Postorder_Traversal(Node *);
};

You can initialize objects created with new.

  p=new Node;
  p->left=p->right=NULL;

  // Like this:
  p = new Node{0, nullptr, nullpre};

In 2011 ( 6 years ago) we introduced nullptr to replace the nasty C macro NULL. Please use nullptr.

  if(root==NULL)

Also a bit of horizontal space makes your code less clustered and nicer to read.

 if (root == nullptr) {
 }

Better Insertion

public:
    void Tree::insert(int data)
    {
        root = insertData(root, data);
    }
private:
    Node* Tree::insertData(Node* current, int data)
    {
        if current == nullptr) {
            return new Node{data, nullptr, nullptr};
        }
        if (date <= current->data) {
            current->left  = insertData(current->left,  data);
        }
        else {
            current->right = insertData(current->right, data);
        }
        return current;
    }


Link shortening script I wrote this script for a link shortener just for fun.

Is it safe, or is it very easy to inject SQL have other security shortcomings? Also, do you have any tips against MySQL injection?

I don't know how to make it better. I don't know, but it tells me that this is mostly code, and I should add some details but I don't know what to write!

 <?
    $user     = check_user();
    $uid      = $user["id"];
    $uid2     = $_SESSION['userid'];
    $link1    = $_POST['link1'];
    $link2    = $_POST['link2'];
    $flink    = $_POST['url'];
    $action   = $_POST['action'];
    $action1  = $_POST['action1'];
    $action2  = $_POST['action2'];
    $faction  = $_POST['faction'];
    $adgrade  = $_POST['adgrade'];
    $monetize = $_POST['monetize'];
    // $re = '/^(?:https?\:\/\/)?[a-z0-9\-_]+\.[a-z0-9\-_\.\/%&=\?\+]+$/mi';
    $re2      = '/^[a-zA-Z]+[:\/\/]+[A-Za-z0-9\-_]+\\.+[A-Za-z0-9\.\/%&=\?\-_]+$/i';
    if (isset($link1) && isset($link2) && isset($flink) && !empty($link1) && !empty($link2) && !empty($flink)) {
        if (!preg_match("~^(?:f|ht)tps?://~i", $link1)) {
            $link1 = "http://" . $link1;
        }
        if (!preg_match("~^(?:f|ht)tps?://~i", $link2)) {
            $link2 = "http://" . $link2;
        }
        if (!preg_match("~^(?:f|ht)tps?://~i", $flink)) {
            $flink = "http://" . $flink;
        }
    }
    if (isset($link1) && isset($link2) && isset($flink) && !empty($link1) && !empty($link2) && !empty($flink)) {
        if (preg_match($re2, $flink, $matches, PREG_OFFSET_CAPTURE, 0) && preg_match($re2, $link1, $matches, PREG_OFFSET_CAPTURE, 0) && preg_match($re2, $link2, $matches, PREG_OFFSET_CAPTURE, 0)) {
            if (isset($action) && isset($action1) && isset($action2) && isset($faction)) {
                //$id = substr(md5(time().$flink), 0, 5);
                $id = substr(str_shuffle(str_repeat("aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpPqQrRsStTuUvVwWxYyYzZ", 5)), 0, 5);
                if ($conn->query("INSERT INTO `" . $database . "`.`link` (`id`, `link1`,`link2`,`action`,`action1`,`action2`,`faction`, `flink`,`adonly`,`userid`,`adgrade`,`monetize`) VALUES ('" . $id . "','" . $link1 . "','" . $link2 . "','" . $action . "','" . $action1 . "','" . $action2 . "','" . $faction . "', '" . $flink . "','false','" . $uid . "','" . $adgrade . "','" . $monetize . "');")) {
                    $html = 'Your STU-Link is https://example.com/' . $id . '';
                } else {
                    $error_sth_wrong = "Something went wrong!";
                }
            } else {
                $error_sl_actions = "Please select all actions!";
            }
            mysqli_close($db);
        } else {
            $error_iv_urls = "Invalid url(s)!";
        }
    } else {
        $error_iv_urls2 = "Invalid url(s)!";
    }
?>

The are a number of problems here.

You do a poor job of validating user input before working with it.  I would strongly suggest you look at using PHP's filter_input() or filter_input_array() to provide basic data validation and error handling. You can use these to easily validate URL formats for example, or provide you own custom validation rule. You probably need to further sanitize some of your input against cross-site scripting vectors depending on how you are using the data added to your database in your application.
You should be leveraging prepared statements for inserting your data to help mitigate against sql injection.
You have a lot of conditional nesting complexity.  You should consider adopting a coding style where you look for early exits and design away nested conditionals to whatever extent is possible.

For example:

if (empty($link1) || empty($link2) || empty($flink)) {
    // error and exit
}

// continue with rest of code, now without nesting.

This minimizes your number of code paths making your application less complex, easier to debug, and easier to maintain. Actively try to design away else conditions, as they are usually avoidable.

Note also, in my example above that I reduced conditionals to only use emtpy() instead of isset() and empty().  empty() would fail for same conditions but is also more specific than isset(), so it is redundant to have both.  Just use the more specific criteria.
You repeat yourself unnecessarily.  Look to factor things like you link re-writing logic into a function to minimize repeated code.  By the way, I am not sure exactly why you use preg_match() and concatenation here.  Won't that break your links?  Perhaps preg_replace() instead?
It appears to me that you a falling into classic programming anti-pattern by naming variables like $somename1, $somename2, etc.  Naming like this is a possible code smell that you should be thinking about using arrays or other data structures to describe these logically-grouped values.
Your lines of code are too long.  It makes it hard to read your code.
Variable names seem to be somewhat poorly named (not very meaningful in some cases).
Why are you closing $db, when you are working with $conn?  There is obviously context missing from example, but this seem like an error.
It is unclear from this code example how you are validating that user making the request should be allowed to modify your database in this manner. Perhaps that is in check_user()?  But typically, at a minimum any form-posting like this would have cross-site request forgery (CSRF) mitigation (i.e. session token) if not some form of user authorization.


Playing with light switches When you were a little kid, was indiscriminately flicking light switches super fun? I know it was for me. Let's tap into that and try to recall that feeling with today's challenge.
Imagine a row of N light switches, each attached to a light bulb. All the bulbs are off to start with. You are going to release your inner child so they can run back and forth along this row of light switches, flipping bunches of switches from on to off or vice versa. The challenge will be to figure out the state of the lights after this fun happens.

The input will have two parts. First, the number of switches/bulbs (N) is specified. On the remaining lines, there will be pairs of integers indicating ranges of switches that your inner child toggles as they run back and forth. These ranges are inclusive (both their end points, along with everything between them is included), and the positions of switches are zero-indexed (so the possible positions range from 0 to N-1).

#include <iostream>
#include <vector>
#include <stdexcept>

class lights;

void ifBiggerSwitch(unsigned&, unsigned&);
std::ostream &print(std::ostream&, lights&);

class lights {
public:
    friend std::ostream &print(std::ostream&, lights&);

    lights() = default;
    lights(unsigned ammount) { setLights(ammount); }

    void setLights(unsigned ammount) { lightsOn.assign(ammount, false); }
    void switchLights(unsigned num1, unsigned num2);
private:
    std::vector<bool> lightsOn;
};

void lights::switchLights(unsigned num1, unsigned num2) {
    ifBiggerSwitch(num1, num2);
    ++num2;
    for (auto beg = lightsOn.begin() + num1; beg != lightsOn.begin() + num2; ++beg) {
        *beg = !*beg;
    }
}

// Non member functions

void ifBiggerSwitch(unsigned &num1, unsigned &num2) {
    if (num1 > num2) {
        unsigned tempNum1 = num1;
        num1 = num2;
        num2 = tempNum1;
    }
}

std::ostream &print(std::ostream &os, lights &light) {
    for (auto c : light.lightsOn)
        os << c << " ";
    return os;
}

Algorithm

The solution is brute force. It has a time complexity of \$O(N R)\$ (where \$N\$ is a number of bulbs and \$R\$ a number of ranges) and I expect to take forever on a bonus input (I gave up after 5 minutes).

To get the correct solution you should first realize that flipping switches in [x, y] range has the same net result as flipping switches in [x, N) range followed by flipping switches in [y+1, N) range. Next, since the flippings commute, it doesn't matter in which order they are performed. Each flipping is an edge changing state of all switches to the right of it.

Which brings the correct solution, in pseudocode:

    set up a vector of integers representing edges
    for each input range (x, y)
        push min(x, y)
        push max(x, y) + 1
    sort the vector
    scan integers from 0 to N toggling state when the edge is crossed

The time complexity is down to \$O(N + R\log R)\$, including output (took less than 2 seconds on my system).


Inserting large amounts data into Excel spreadsheet I have a VSTO Excel Add-In which is used to retrieve data from another application and insert it into an Excel worksheet. Due to the potential for lots of pieces of data to be inserted into a spreadsheet, performance is a concern. Potentially thousands of rows could be inserted.

Here is the method which inserts data into an Excel spreadsheet.

Note: Using Microsoft.Office.Interop.Excel

private Worksheet Worksheet { get { return Globals.ThisAddIn.Application.ActiveSheet; } }
private Range Cells { get { return Globals.ThisAddIn.Application.ActiveSheet.Cells; } }

private void PopulateCurrentWorksheet(ReportData data, int columnIndex, int rowIndex)
{
    if (data == null)
        throw new ArgumentNullException("No valid ReportData available");

    // NOTE: Excel uses indexing that starts at 1
    if (columnIndex == 0)
        columnIndex = 1;

    if (rowIndex == 0)
        rowIndex = 1;

    int startingRowIndex = rowIndex;

    foreach (ReportColumn column in data.Columns)
    {
        Cells[columnIndex][rowIndex].Value = column.ColumnName;
        Cells[columnIndex][rowIndex].Font.Bold = true; // put the header in bold
        for (int i = 0; i < column.ColumnData.Length; i++)
        {
            rowIndex++; // move down a cell to the next row
            Cells[columnIndex][rowIndex] = column.ColumnData.ElementAt(i);
        }
        rowIndex = startingRowIndex;
        columnIndex++; // move right a cell to the next column
    }

    Worksheet.Columns.AutoFit();
}

Below is the ReportData class (and associated ReportColumn class) which holds data retrieved from an external application to be inserted into a worksheet

public class ReportData
{
    public ReportData(IEnumerable<ReportColumn> columns)
    {
        Columns = columns ?? throw new ArgumentNullException("No valid columns available");
    }

    public IEnumerable<ReportColumn> Columns { get; set; }
}

public class ReportColumn
{
    public ReportColumn(string columnName, object[] columnData)
    {
        ColumnName = columnName;
        ColumnData = columnData;
    }

    public string ColumnName { get; set; }
    public object[] ColumnData { get; set; }
}

When trying to insert data into a worksheet which results in over 100k rows, there is a significant performance hit. Is there anyway I could improve the code to make it faster? Any other advice is welcomed too.

public class ReportData
{
    public ReportData(IEnumerable<ReportColumn> columns)
    {
        Columns = columns ?? throw new ArgumentNullException("No valid columns available");
    }

    public IEnumerable<ReportColumn> Columns { get; set; }
}

The Columns property should be get-only.

public string ColumnName { get; set; }
public object[] ColumnData { get; set; }

Same with these two: it makes no sense to set them in the constructor, even throw an exception if it's null, if you can legally construct the object and then merrily do this:

var data = new ReportData(Enumerable.Empty<ReportColumn>()) { Columns = null };

Note that IEnumerable<T> things should indeed never be null, but any IEnumerable<T> can still be empty without being null. As such, the guard clause is insufficient, since it appears to be there to protect against creating a new ReportData that wouldn't have any columns - Enumerable.Empty<T>() is a perfectly valid input that should be guarded against.

public class ReportData
{
    public reportData(IEnumerable<ReportColumn> columns)
    {
        var reportColumns = columns ?? throw new ArgumentNullException(...);
        if (!reportColumns.Any())
        {
            throw new ArgumentException(...);
        }

        Columns = reportColumns;
    }

    public IEnumerable<ReportColumns> Columns { get; }
}

Now there's no way a ReportData object can have 0 columns, be it during or after construction.

These two could be expression-bodied auto-properties:

private Worksheet Worksheet { get { return Globals.ThisAddIn.Application.ActiveSheet; } }
private Range Cells { get { return Globals.ThisAddIn.Application.ActiveSheet.Cells; } }

Like this:

private Worksheet Worksheet => Globals.ThisAddIn.Application.ActiveSheet;
private Range Cells => Globals.ThisAddIn.Application.ActiveSheet.Cells;

This is a serious problem:

// NOTE: Excel uses indexing that starts at 1
if (columnIndex == 0)
    columnIndex = 1;

if (rowIndex == 0)
    rowIndex = 1;

Consider consistently using { } braces to avoid such implicit scopes.

If the columnIndex or rowIndex supplied was 0, then the calling code is off-by-one and this silent one-upping is literally hiding that bug, and there's a very very very high chance that the next call is going to pass a 1 that you're not going to offset, which will produce a result, but very unlikely the expected result.

Consider:

Throwing an ArgumentOutOfRangeException given 0 for column or row index.
Adding XML docs to document that the index parameters are 1-based - OR document them as 0-based, and abstract away the fact that Excel's object model is 1-based by offsetting every provided value, not just 0. In any case, you need XML docs to say this.

My experience with VSTO is that if you don't properly release all the COM objects you ever access, then when your program exits you'll be left with a ghost EXCEL.EXE process in Task Manager, because .NET will be holding on to RCW objects wrapping COM/unmanaged objects that won't be garbage-collected like managed objects would be.

Hence, avoid double-dots like this:

Globals.ThisAddIn.Application.ActiveSheet

That's essentially leaking ThisAddIn, Application, and ActiveSheet objects.

Every single access to Cells is re-dereferencing the same ActiveSheet over and over and over, every time. That makes a lot of redundant member access.

var addin = Globals.ThisAddIn;
var application = addin.Application;
var activeSheet = application.ActiveSheet;

var allCells = activeSheet.Cells;
var cell = allCells[columnIndex][rowIndex];
cell.Value = column.ColumnName;

var cellFont = cell.Font;
cellFont.Bold = true;

...and then each of these objects need to be released... in the reverse order they were accessed:

Marshal.ReleaseCOMObject(cellFont);
Marshal.ReleaseCOMObject(cell);
Marshal.ReleaseCOMObject(allCells);
...

Miss one single COM object, and the EXCEL.EXE process won't be able to shut down correctly.

That makes it a pretty good reason to avoid writing to individual cells, ...regardless of the performance hit ;-)


Copying file dependencies I have written the following code, and it works great, but I'm not satisfied. I really love the book "Clean Code" by Uncle Bob and I try to apply all his principles when coding, but I'm having a tough time cleaning this abomination. I call it an abomination because I'm sure everyone new to this method will spend more than 10 seconds trying to figure out what's going on. It's not easy to read. So I'm hoping for input on how to make it easier to read.

private void CopyFileDependencies(FileInfo fileInfo, string destinationPath, int recoveryAttempts = 1)
{
  try
  {
    foreach (var file in Directory.GetFiles(fileInfo.DirectoryName, "*.*", SearchOption.AllDirectories))
    {
      File.Copy(file, file.Replace(fileInfo.DirectoryName, destinationPath), true);
    }
  }
  catch (IOException)
  {
    if (recoveryAttempts > 0)
    {
      KillProcess(fileInfo);
      CopyFileDependencies(fileInfo, destinationPath, --recoveryAttempts);
    }
    else
    {
      throw;
    }
  }
}

Why is a method that is ostensibly about "copying file dependencies" killing a process?

It also seems awkward to use recursion for something like this, especially from the catch block. Maybe it's just me?

I would get rid of the recursion and the process killing, and let the calling code higher up deal with that. This method should just attempt to copy the files from a to b, and the calling code can run it as many times as it wants, handle failures by killing a process or whatever, etc.