Optimise runtime of filter character method I'm trying to run the method below against a very large corpus and I need to optimize/reduce the run time for this method as it already takes up about 6 seconds to execute.

Requirements:

Check the word only consist of alphabets, hyphen and apostrophe
First character of word must be alphabet
Last character of word must be alphabet or apostrophe only
Use of re library (regex) strictly not allowed

def delUnknownChar(w):
    wf = []
    for c in w:
        if (c == "'" or c == "-" or c.isalpha()):
            wf.append(c)

    w = "".join(wf)
    wf.clear()

    if (len(w) > 1):
        while(not w[0].isalpha()):
            w = w[1:]

        while (w[-1] == "-"):
            w = w[:-1]

        return w
    else:
        return None

string1 = delUnknownChar("-'test'-")
print(string1)

Output will be test'
The code above will take about 5 seconds to run.

If I change lines 2-7 of the code to this line:

w = "".join(c for c in w if c == "'" or c == "-" or c.isalpha())

The runtime somehow increases by 1 more second.

Does anyone here have a better idea or an improved optimized way to check for this at a much faster speed?

Possible performance improvements

w = w[:-1] is probably inefficient since it asks to perform a copy of every element of the list minus one. del w[-1] is the idiomatic way to delete the last element of a list.
You probably have more characters in a string corresponding to isalpha than characters exactly equal to "'" or "-". Therefore, you may notice speed improvements by checking isalpha before the other two conditions.
You could create a table containing all the values of isalpha plus "'" and "*" and then check whether a character is in this table. However, I don't think that it can bring you significant speed improvements, unless you filter with a dedicated str method that may be optimized (replace for example).

Style

A few notes about style:

Please, don't put parenthesis around the while condition, that goes against the Python style guide (PEP8).
Same goes for if.


REST API calls to BIG-IP LTM to get the status of pool members This code calls the iControl REST API provided by BIG-IP LTM. Trying to get the list of the pools, it's current status and the pool members associated with the pool.

My code works like this, based on three calls:

Get the pool names (/mgmt/tm/ltm/pool)
Based on the pool names (path), get its status (mgmt/tm/ltm/pool/~pool~name/stats)
Based on the pool names (path), get its pool members (/mgmt/tm/ltm/pool/~pool~names/members)

If I do only the first call, I get the output within milliseconds. After adding the second call, it becomes worse. With a third call, it takes good amount of time (more than 6 minutes) to get all the output for about 400 pools.

How can I optimize this code?

import requests
from requests.auth import HTTPBasicAuth

BASE_URL = "https://localhost/mgmt/tm"
username = "admin"
password = "admin"

def makeRequest(username, password, url):
    response_data = requests.get(url, auth = HTTPBasicAuth(username, password), verify = False)

    return response_data.json()

pool_data = makeRequest(username, password, BASE_URL + "/ltm/pool")

for pools in pool_data['items']:
    print pools['name']
    tildPath = pools['fullPath'].replace('/','~')

    #GET the Pool stats
    pool_stats = makeRequest(username, password, BASE_URL + "/ltm/pool/" + tildPath + "/stats")
    print pool_stats['entries']['status.availabilityState']['description']

    #GET the Pool Members
    pool_members = makeRequest(username, password, BASE_URL + "/ltm/pool/" + tildPath + "/members")

    for members in pool_members['items']:
        print members['name'] +" " + members['address'] + " " + members['state']

Regarding your performance problem there's nothing obviously wrong with the code.  I'd suggest you make sure that the server isn't throttling the responses in subsequent runs.

If that's not the case the next step would be to determine which of the calls is slow and pinpoint if it's an issue e.g. with JSON parsing or accumulating some garbage data that's not necessary.

In particular a profiler would come in handy for this, even something like cProfile would probably be enough to check for obvious problems.

The code should follow PEP8 naming conventions everywhere, i.e. make_request.
Also compatibility with Python 3 would be nice, so use print(...).


Laravel generate category list from product results I have product results with some filter, and the one is category. I'll get the category list from product results. the model category and products is one to many relationship.

This is the search query:

$results = Product::where('available','=', 1)->whereRaw('stock_available > min_quantity');
$results = $results->where(function($query) use($search){
$query->where('name', 'LIKE', '%' . $search . '%')
->orWhere('sku', 'LIKE', '%' . $search . '%')
->orWhere('part_number', 'LIKE', '%' . $search . '%');
});

I try to get the category list from the results, like this:

$arrayCategory = $results->pluck('category_id');
$arrayType = $results->pluck('product_type_id');
$arrayManufactur = $results->pluck('manufacturer_id');
$arrayOem = $results->pluck('is_oem');
$arrayUsedStatus = $results->pluck('newused');

$resultCategory = array_unique($arrayCategory);
$arrayCountCategory = array_count_values($arrayCategory);
$categories = Categories::whereIn('id', $resultCategory)->with('products')->pluck('name', 'id');

$resultType = array_unique($arrayType);
$arrayCountType = array_count_values($arrayType);
$types = ProductType::whereIn('id', $resultType)->with('products')->pluck('name', 'id');

$resultManufacturer = array_unique($arrayManufactur);
$arrayCountManufactur = array_count_values($arrayManufactur);
$manufacturList = Manufactur::whereIn('id', $resultManufacturer)->with('products')->pluck('name', 'id');

$resultOem = array_unique($arrayOem);
$arrayCountOem = array_count_values($arrayOem);

$resultUsedStatus = array_unique($arrayUsedStatus);
$arrayCountUsedStat = array_count_values($arrayUsedStatus);

This code takes too much time loading, and I want to know how I can optimize it to achieve faster loading.

Your search query will not be able to leverage an index on the fields you are searching within.  This is because you have a wildcard at the beginning of the search string and a default B-Tree index only works for prefix match use cases.  So when the LIKE condition has a wild card at the beginning, you will need to perform a full table scan.  Full table scans are REALLY bad from a performance standpoint.

It looks like you use case would work best with natural language full text search (MySQL link here - http://dev.mysql.com/doc/refman/5.7/en/fulltext-natural-language.html ).  This would require change to both your query and the database schema.

You also do a lot of individual single table queries here.  My guess is that you can possibly optimize this by utilizing joins to get date across table with a single (or fewer queries) vs. reading data from one table, then another based on that result, then another based on that next result, etc.


Calculating difference in time In table first value I have value with datatype = Start

I need to calculate difference first value - midnight

Last value is with datatype =Stop

I need to calculate difference midnight - last item

For other I need to calculate datatype.Stop - previous.datatype=Start

Here is ViewModel that I use for repo method

public class HeatMapViewModel
{
    public decimal? Latitude2 { get; set; }
    public decimal? Longitude2 { get; set; }
    public int coeff = 2;
    public int Difference;
    public DateTime Date { get; set; }
}

I wrote this code in repo method

public List<HeatMapViewModel> GetStops()
{
    using (TraxgoDB ctx = new TraxgoDB())
    {
        List<HeatMapViewModel> items = new List<HeatMapViewModel>();

        var firstitem = ctx.Logging.Where(x => x.Datatype == Datatype.Start).AsEnumerable().FirstOrDefault();

        var midnight = new DateTime(firstitem.CurrentDateTime.Year, firstitem.CurrentDateTime.Month, firstitem.CurrentDateTime.Day, 00, 00, 00);
        TimeSpan difference = (firstitem.CurrentDateTime - midnight);
        var difference_after_midnight = (int)difference.TotalMinutes;

        items.Add(new HeatMapViewModel
        {
            Latitude2 = firstitem.Latitude,
            Longitude2 = firstitem.Longitude,
            Difference = difference_after_midnight,
            Date = firstitem.CurrentDateTime
        });

       var lastItem = ctx.Logging.Where(x => x.Datatype == Datatype.Stop).AsEnumerable().LastOrDefault();
       var before_midnight = new DateTime(lastItem.CurrentDateTime.Year, lastItem.CurrentDateTime.Month, lastItem.CurrentDateTime.Day, 23, 59, 00);
       TimeSpan difference_before = (before_midnight - lastItem.CurrentDateTime);
       var difference_before_midnight = (int)difference_before.TotalMinutes;

       items.Add(new HeatMapViewModel
       {
            Latitude2 = lastItem.Latitude,
            Longitude2 = lastItem.Longitude,
            Difference = difference_before_midnight,
            Date = firstitem.CurrentDateTime
        });

        var allitems = ctx.Logging;
        var filteredQuery = allitems.Where(x => x.Datatype == Datatype.Start || x.Datatype == Datatype.Stop).OrderByDescending(x => x.LogID).ToList();

        for (int i = 1; i < filteredQuery.Count; i++)
        {
            if (filteredQuery[i].Datatype == Datatype.Stop && filteredQuery[i - 1].Datatype == Datatype.Start)
            {
                TimeSpan differenceTicks = filteredQuery[i - 1].CurrentDateTime - filteredQuery[i].CurrentDateTime;

                items.Add(new HeatMapViewModel
                {
                    Latitude2 = filteredQuery[i].Latitude,
                    Longitude2 = filteredQuery[i].Longitude,
                    Difference = (int)differenceTicks.TotalMinutes,
                    Date = firstitem.CurrentDateTime
                });
            }
        }
        return items;
    }
}

Here is data from db

It works great, but I need to know if I can modify it?

The main area where you code can be improved is in your database calls. Currently your making 3 database calls. When you use

var lastItem = ctx.Logging.Where(x => x.Datatype == Datatype.Stop).AsEnumerable().LastOrDefault();

your loading all the records into memory (using .AsEnumerable()) in order to get the last one then throwing all the rest away, and then making another database query using

var filteredQuery = allitems.Where(...)

which fetches those records all over again. You only need the one database call to materialize the records to memory, and then you can get the first and last records from that collection.

Your also not testing for null when you use .FirstOrDefault() and .LastOrDefault() which has the potential to throw an exception in the code following those queries.

You can also simplify some code such as

var midnight = new DateTime(firstitem.CurrentDateTime.Year, firstitem.CurrentDateTime.Month, firstitem.CurrentDateTime.Day, 00, 00, 00);

which can be rewritten as

var midnight = firstitem.CurrentDateTime.Date;

You could re-write your method to have only one database call as

public List<HeatMapViewModel> GetStops()
{
    using (TraxgoDB ctx = new TraxgoDB())
    {
        List<HeatMapViewModel> items = new List<HeatMapViewModel>();
        var logs = ctx.Logging.Where(x => x.Datatype == Datatype.Start || x.Datatype == Datatype.Stop).OrderByDescending(x => x.LogID).ToList();
        if (logs.Count < 2)
        {
            return items; // or return null;?
        }
        var first = logs.First();
        var currentDateTime = first.CurrentDateTime;
        double minutes = (first.CurrentDateTime - first.CurrentDateTime.Date).TotalMinutes;
        items.Add(new HeatMapViewModel
        {
            Latitude2 = first.Latitude,
            Longitude2 = first.Longitude,
            Difference = (int)minutes,
            Date = currentDateTime
        });
        var last = logs.Last();
        minutes = (last.CurrentDateTime.Date.AddDays(1) - last.CurrentDateTime).TotalMinutes;
        items.Add(new HeatMapViewModel
        {
            Latitude2 = last.Latitude,
            Longitude2 = last.Longitude,
            Difference = (int)minutes,
            Date = currentDateTime
        });
        for (int i = 1; i < logs.Count; i++)
        {
            var previous = logs[i - 1];
            var current = logs[i];
            if (current.Datatype == Datatype.Stop && previous.Datatype == Datatype.Start)
            {
                minutes = (previous.CurrentDateTime - current.CurrentDateTime).TotalMinutes;

                items.Add(new HeatMapViewModel
                {
                    Latitude2 = current.Latitude,
                    Longitude2 = current.Longitude,
                    Difference = (int)minutes,
                    Date = currentDateTime
                });
            }
        }
        return items;
    }
}

Note that I have assumed that if there were less than 2 items in the table, then your data would not make sense in a view which is the purpose of if (logs.Count < 2). That check will also prevent exceptions in the code that follows.

I have also assumed (based on the description in the question) that .First() will always return an item with DataType.Start and .Last() will always return an item with DataType.Stop so I have excluded checks for that in the code above


Python binary to decimal converter (without using built-ins) Any ideas how I could improve this? I already know that there is no checking for anything other than a 1, so you can enter a 2 or an a or a b and it will be interpreted as a 0, not interfering with the program's functionality. What I would like to know is if anyone has any suggestions for improving the logic.

def main():

  #Input
  binary = input("Enter a line of binary: ")

  #Reverses input
  toCalc = binary[::-1]

  #Finds length of input
  binLen = len(binary)

  #Variables
  total = 0
  exp = 0 #Exponent

  #loop while current character place is less than input length
  while exp < binLen:
    if toCalc[exp] == "1": #If current char is a '1', add 2^exp to the total
        total += 2 ** int(exp)
    exp += 1
  print (total)
  main()

main()

Here is a link to a working version of the script.

This algorithm could pretty easily be converted to a one-liner.  Let's walk through the logic:

You want to work over each element of a string.  You can just iterate over the string, rather than keeping track of an index.
You also need the index of that element, so you can use enumerate to get both.
You want to apply a mathematical operation if and only if the integer is not 0.  This can be done using int(val), which converts the value to the number it represents.
You want to add all the values at the end.  This can be done with sum.

To get error checking, you can convert the string to a set, and make sure the set is equal to a set with only 0 and 1 in it.

Also, you probably want the input handling in a while loop to avoid the recursion, which will fill up your memory, and then have a separate function to handle the actual conversion.

Also, I would only run main if the code is run as a script.

And keep in mind python has a built-in way to do binary to decimal conversion: int(binstr, 2), which can be used to test your implementation at the very least.

So here is how I would implement it:

def bin2int(binstr):
    if set(binstr) != {'0', '1'}:
        raise ValueError('String is not binary: "%s"' % binstr)

    return sum(int(x)*2**i for i, x in enumerate(a[::-1]))

def main():
    while True:
        binary = input("Enter a line of binary, enter 'q' to quit: ")

        if binary.lower() == 'q':
            break

        try:
            print(bin2int(binary))
        except ValueError as err:
            print(err)

if __name__ == "__main__":
    main()


Basic console calculator with four operations I'm an absolute beginner in programming, and Today I decided to put my knowledge to the test and create a basic c# calculator. I've Done it, but now I'm looking for ways to Make it more shorter and readible. If you Have any suggestions, please give them!

using System;

namespace lol
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Hi! What is your name?");
            string name = Console.ReadLine();

            Console.WriteLine(name + " What do you wanna do");

            Console.WriteLine("Type \"+\" for addition");
            Console.WriteLine("Type \"-\" for subtraction");
            Console.WriteLine("Type \"*\" for multiplication");
            Console.WriteLine("Type \"/\" for division");

            string operation = Console.ReadLine();

            Console.Write("Now, Give me number one: ");
            double num1 = Convert.ToDouble(Console.ReadLine());

            Console.Write("Now give me number two: ");
            double num2 = Convert.ToDouble(Console.ReadLine());

            if (operation == "+")
            {
                Console.WriteLine(num1 + num2);
            }
            else if (operation == "-")
            {
                Console.WriteLine(num1 - num2);
            }

            else if (operation == "*")
            {
                Console.WriteLine(num1 * num2);
            }

            else
            {
                Console.WriteLine(num1 / num2);

            }

        }
    }
}

Compactness

use a switch statement rather than verbose if-elseif-.. statements
get rid of redundant blank lines

Readability

Avoid escaping characters.

Console.WriteLine("Type \"+\" for addition");
Console.WriteLine("Type \"-\" for subtraction");
Console.WriteLine("Type \"*\" for multiplication");
Console.WriteLine("Type \"/\" for division");

You could go with..

Console.WriteLine("Type '+' for addition");
Console.WriteLine("Type '-' for subtraction");
Console.WriteLine("Type '*' for multiplication");
Console.WriteLine("Type '/' for division");

Or..

Console.WriteLine(@"Type ""+"" for addition");
Console.WriteLine(@"Type ""-"" for subtraction");
Console.WriteLine(@"Type ""*"" for multiplication");
Console.WriteLine(@"Type ""/"" for division");

User Experience

If you ever wish to design end-user interfaces, you have to work on your lingo.

You're off to a good start:

Console.WriteLine("Hi! What is your name?");

You slip up slightly next (forgot a question mark):

Console.WriteLine(name + " What do you wanna do");

But then you start pressuring the end-user. You expect a number, not in a moment, but Now! And you are not asking, you are demanding.

Console.Write("Now, Give me number one: ");

Also, one time with a comma and capital after it, and one time the other way around:

Console.Write("Now give me number two: ");

The user may also expect this application to crash on:

double num1 = Convert.ToDouble(Console.ReadLine());
double num2 = Convert.ToDouble(Console.ReadLine());
Console.WriteLine(num1 / num2); // when num2 is 0
Any other overflow

And nothing happens when an unknown operator is provided by the user.

The user is not able to verify the results, since the application immediately terminates after the calculation is evaluated.


Generating a random string of 6 characters long in Python 3.6.2 I am learning Python, so pardon me if this is the very crude approach.
I am trying to generate a random string using alphanumeric characters.
Following is my code:

#Generating random 6 character string from list 'range'.
def code():
    # List of characters [a-zA-Z0-9]
    chars = ['a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z','A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z','0','1','2','3','4','5','6','7','8','9']
    code = ''
    for i in range(6):
        n = random.randint(0,61)    # random index to select element.
        code = code + chars[n]      # 6-character string.
    return code

What I am trying to do is pick six random elements from the list of characters and append them.

Is there any better approach?

Simplicity + improvements:

chars list. Instead of hardcoding all lowercase, uppercase and digit chars - string module provides a convenient constants string.ascii_letters and string.digits
random.randint(0,61). Instead of generating random index for further search on chars sequence - random.choice already allows getting a random element from a specified sequence
for ... loop is easily replaced with generator expression

The final version:

import random
import string

def random_alnum(size=6):
    """Generate random 6 character alphanumeric string"""
    # List of characters [a-zA-Z0-9]
    chars = string.ascii_letters + string.digits
    code = ''.join(random.choice(chars) for _ in range(size))
    return code

if __name__ == "__main__":
    print(random_alnum())
    print(random_alnum())
    print(random_alnum())

Sample output:

g7CZ2G
bczX5e
KPS7vt


How can I optimise a foreach calling an API? The code calls an API and returns the first string reponse I have the following code that makes a call to an external API, processes the response and returns a string. The problem is it is taking a very long time to process. I can't do anything to the API, but I was just wondering if there is a better way of doing the rest of the code.
foreach(var item in products)
{
    item.site = await _testRepo.GetSiteLink(item.Id, item.baseLink);
}

And the GetSiteLink method in a different class.
public async Task<string> GetSiteLink(int itemId, string baseLink)
{
    var encodedUrl = HttpUtility.UrlEncode(baseLink);
    var response = await _client.GetAsync($"apilink/{itemId}/?ulp={encodedUrl}");
    if (response.IsSuccessStatusCode)
    {
        var responseContent = await response.Content.ReadAsStringAsync();
        var parsedResponse = JsonConvert.DeserializeObject<string[]>(responseContent);
        return HttpUtility.UrlDecode(parsedResponse[0]);
    }
    return "";
}

one way of solving this would be using Task.WhenAll the idea is to add all tasks into a list of tasks then we call Task.WhenAll and await the final results.
Here is an example of it :
private async Task<ProductItem> SetSiteLink(ProductItem item)
{
    item.site = await _testRepo.GetSiteLink(item.Id, item.baseLink);

    return Task.FromResult(item);
}

then call it like this :
var tasks = new List<Task<ProductItem>>();

foreach (var item in products)
{
    tasks.Add(SetSiteLink(item));
}

var results = await Task.WhenAll(tasks);


Recursively displaying json data on website I am displaying the data from a json file onto a website, with recursive and iterative tables side by side. i have the iterative code working, but I have no idea on how to make it recursive.
Here is the code:
    $dataArray = json_decode($data, true);
            if(is_array($dataArray)){ //first level
                $result = "
";
                    foreach($dataArray as $key=>$val){ //second level
                        $result.="
".$key.": 	";
                        if(gettype($val)=="array"){
                            $result .= "
";
                            foreach($val as $subkey=>$subval){ //third level
                                $result.="
".$subkey.": 	";
                                if(gettype($subval)=="array"){
                                    $result .= "
";
                                    foreach($subval as $lowkey=>$lowval){ //fourth level
                                        $result.="
".$lowkey.": 	";
                                        if(gettype($lowval)!="array"){
                                            $result.=$lowval."
";
                                        }else{
                                            $result.="
";
                                        }
                                    }
                                    $result.="
";
                                }else{
                                    $result.=$subval."
";
                                }
                            }
                            $result.="
";
                        }else{
                        $result.=$val."
";
                        }
                    }
                $result.= "
";
            }else{
                echo "Not a valid format!";
            }
            return $result;

Your code is a pain to read, please add early returns.
To go with the point above, please follow PSR-12.
iterative tables side by side. Your code creates another table within a , so it's no exactly side by side.

Here is how I refactored your code:

I completed step 1 and 2.
I have broken everything up into methods:

<?php

namespace App;

class DisplayArrayTable
{
    protected $result = '';

    /**
     * @param mixed $value
     */
    protected function setupMeaningFullName1(string $key, $data): void
    {
        $this->result .= "
{$key}: 	";

        if (!is_array($data)) {
            $this->result .= "{$data}
";

            return;
        }

        $this->result .= "
";

        foreach ($data as $key => $value) {
            $this->setupMeaningFullName2($key, $value);
        }

        $this->result .= "
";
    }

    /**
     * @param mixed $value
     */
    protected function setupMeaningFullName2(string $key, $data): void
    {
        $this->result .= "
{$key}: 	";

        if (!is_array($data)) {
            $this->result .= "{$data}
";

            return;
        }

        $this->result .= "
";

        foreach ($data as $key => $value) {
            $this->setupMeaningFullName2($key, $value);
        }

        $this->result .= "
";
    }

    /**
     * @param mixed $value
     */
    protected function setupMeaningFullName3(string $key, $data): void
    {
        $this->result .= "
{$key}: 	";

        if (!is_array($data)) {
            $this->result .= "{$data}
";

            return;
        }

        $this->result .= "
";
    }

    public function handle(array $data): string
    {
        foreach ($data as $key => $value) {
            $this->setupMeaningFullName1($key, $value);
        }

        return "
{$this->result}
";
    }
}

I have identified the pattern and further refactored your code

<?php

namespace App;

class DisplayArrayTable
{
    const MAX_ITERATIONS = 3;
    protected $currentIteration = 1;
    protected $result = '';

    /**
     * @param mixed $value
     */
    protected function iteration(string $key, $data): void
    {
        $this->result .= "
{$key}: 	";

        if (!is_array($data)) {
            $this->result .= "{$data}
";

            return;
        }

        if ($this->currentIteration === static::MAX_ITERATIONS) {
            $this->result .= '
';

            return;
        }

        $this->result .= '
';

        foreach ($data as $key => $value) {
            $this->currentIteration++;

            $this->setupMeaningFullName2($key, $value);
        }

        $this->result .= '
';
    }

    public function handle(array $data): string
    {
        foreach ($data as $key => $value) {
            $this->iteration($key, $value);
        }

        return "
{$this->result}
";
    }
}

Performed further refactoring to improve iteration method

<?php

namespace App;

class DisplayArrayTable
{
    const MAX_ITERATIONS = 3;
    protected $currentIteration = 1;
    protected $result = '';

    public function handle(array $data): string
    {
        foreach ($data as $key => $value) {
            $this->iteration($key, $value);
        }

        return "
{$this->result}
";
    }

    /**
     * @param mixed $value
     */
    protected function iteration(string $key, $data): void
    {
        $this->result .= "

            {$key}:
            ";

        if (!is_array($data)) {
            $this->result .= "{$data}
";

            return;
        }

        if ($this->currentIteration === static::MAX_ITERATIONS) {
            $this->result .= '
';

            return;
        }

        $this->setupNewTable($data);
    }

    protected function setupNewTable(array $data): void
    {
        $this->result .= '
';

        foreach ($data as $key => $value) {
            $this->currentIteration++;

            $this->iteration($key, $value);
        }

        $this->result .= '
';
    }
}

So, all you need to do now is call this call and pass in a valid array
use App\DisplayArrayTable;

...

$dataArray = json_decode($data, true);

if ($dataArray && is_array($dataArray)) {
   $html = (new DisplayArrayTable)->handle($dataArray);
}


Boolean array, NAND of [SKSpritenodes] I'm using spritekit, using on touch to detect what the users has tapped, ie., no collisions, etc.

I have an array of sprite nodes which in effect is a multi-state boolean switch for now the user can only select one node to be active at a time so the logic looks like:

Node A,  Node B, Node C .........Node M
off       off      off        initial
on        off      off        user touches node A
off       off      on          user touches node C

This code is my working template and would like to reduce a lot of this boilerplate down to something reasonable.

I'm looking for ways that are either faster, more the "Apple Way", more OOP.

    var temp1:SKSpriteNode = SKSpriteNode()
  override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?)
  {
    if let touch = touches.first
    {
      let tappedNodes:[sk_Node] = nodes(at: touch.location(in: self) )

      for node in tappedNodes
      {
    switch node.name!
        {
        case "level_1":
          pushd(node: node)
          node.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_1_selected") ], timePerFrame: K1_4.f64_t) )

          temp1 = childNode(withName: "level_2")!
          temp1.run(sk_Action.animate(with: [sk_Texture(imageNamed: "level_2_normal") ], timePerFrame: K1_4.f64_t) )
          popd(node:temp1)

          temp1 = SKSpriteNode()
          temp1 = childNode(withName: "level_3")!
          temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_3_normal") ], timePerFrame: K1_4.f64_t) )
          popd(node:temp1)

    case "level_2":
      pushd(node: node)
      node.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_2_selected") ], timePerFrame: K1_4.f64_t) )

      temp1 = childNode(withName: "level_1")!
      temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_1_normal") ], timePerFrame: K1_4.f64_t) )
      popd(node:temp1)

      temp1 = SKSpriteNode()
      temp1 = childNode(withName: "level_3")!
      temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_3_normal") ], timePerFrame: K1_4.f64_t) )
      popd(node:temp1)

    case "level_3":
      pushd(node: node)
      node.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_3_selected") ], timePerFrame: K1_4.f64_t) )

      temp1 = childNode(withName: "level_2")!
      temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_2_normal") ], timePerFrame: K1_4.f64_t) )
      popd(node:temp1)

      temp1 = SKSpriteNode()
      temp1 = childNode(withName: "level_1")!
      temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: "level_1_normal") ], timePerFrame: K1_4.f64_t) )
      popd(node:temp1)

} } } }

pushd and popd are SKActions that do some simple scaling up and down.

If it were me, I'd reduce the boiler plate by only putting what's actually changing into the switch statement, and putting everything else outside of it. So here you would only set the texture names in the switch statement. Something like this:

var selectedTextureName : String
var normalTexture1Name : String
var normalTexture2Name : String
var node1Name : String
var node2Name : String
switch node.name!
{
    case "level_1":
        selectedTextureName = "level_1_selected"
        normalTexture1Name = "level_2_normal"
        normalTexture2Name = "level_3_normal"
        node1Name = "level_2"
        node2Name = "level_3"

    case "level_2":
        selectedTextureName = "level_2_selected"
        normalTexture1Name = "level_1_normal"
        normalTexture2Name = "level_3_normal"
        node1Name = "level_1"
        node2Name = "level_3"

    case "level_3":
        selectedTextureName = "level_3_selected"
        normalTexture1Name = "level_1_normal"
        normalTexture2Name = "level_2_normal"
        node1Name = "level_1"
        node2Name = "level_2"
}

pushd(node: node)
node.run( sk_Action.animate(with: [skTexture(imageNamed:selectedTextureName)], timePerFrame: K1_4.f64_t) )

temp1 = childNode(withName: node1Name)!
temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: normalTexture1Name) ], timePerFrame: K1_4.f64_t) )
popd(node:temp1)

temp1 = SKSpriteNode()
temp1 = childNode(withName: node2Name)!
temp1.run( sk_Action.animate(with: [sk_Texture(imageNamed: normalTexture2Name) ], timePerFrame: K1_4.f64_t) )
popd(node:temp1)


Number guessing game in Kotlin Is this code missing any "Kotlinisms"?

I've just started learning Kotlin so I'm not yet familiar with the Kotlin way of doing things.

The program will guess a number you are thinking of.

package challenges.difficult

/**
 * Created by jbarnes on 04/06/2016.
 * Challenge: https://www.reddit.com/r/dailyprogrammer/comments/pii6j/difficult_challenge_1/
 */

fun main(args:Array<String>) {
    Guesser();
}

class Guesser(range:IntRange) {

    private var range = range;
    private var upperLimit:Int = range.last;
    private var lowerLimit:Int = range.first;
    private var turnCount:Int = 0;
    private var lastGuess:Int = 0;
    private var lastResponse:Response = Response.UNKNOWN;

    constructor():this(1..100)

    init {
        printIntro();
        while (lastResponse != Response.CORRECT) {
            lastResponse = presentNextGuess();
            turnCount++;
            if (lastResponse == Response.CORRECT) {
                println("I guessed your number in $turnCount turns.");
            }
            if (lastResponse == Response.UNKNOWN) {
                println("I'm sorry, I didn't understand that.");
            }
        }

    }

    fun calcNextGuess():Int {
        var roundingFactor = 0;
        if (lastResponse == Response.HIGHER) {
            lowerLimit = lastGuess;
            roundingFactor = 1; //compensates for rounding down
        } else if (lastResponse == Response.LOWER) {
            upperLimit = lastGuess;
        }
        return (lowerLimit + ((upperLimit - lowerLimit) / 2)) + roundingFactor;
    }

    fun presentNextGuess():Response {
        val nextGuess = calcNextGuess();
        println("Is your number $nextGuess?");
        val response = readLine();
        lastGuess = nextGuess;
        return parseResponse(response);
    }

    fun parseResponse(response:String?):Response {
        if (response == null) {
            return Response.UNKNOWN;
        } else if (response.equals("higher", true) || response.equals("h", true)) {
            return Response.HIGHER;
        } else if (response.equals("lower", true) || response.equals("l", true)) {
            return Response.LOWER;
        } else if (response.equals("yes", true) || response.equals("y", true)) {
            return Response.CORRECT;
        } else {
            return Response.UNKNOWN;
        }
    }

    fun printIntro() {
        println("Think of a number between 1 and 100. I am going to try and guess it.");
        println("If i guess correctly respond with 'yes' (or 'y')");
        println("Otherwise respond to with either 'higher' (or 'h') or 'lower' (or 'l')");
        println("Okay, here we go...");
    }

    enum class Response {
        HIGHER, LOWER, CORRECT, UNKNOWN
    }
}

https://github.com/JNBarnes/LearningKotlin/blob/master/src/challenges/difficult/GuessingGame.kt

Some suggestions:

Semi-colons are optional so, avoid them if possible.
Use type inference when possible. For example:

val aInteger = 1 // Int is inferred by the compiler as the type of `aInteger`

val aListOfInt = listOf(1, 2) // List<Int> is inferred by the compiler as the type of `aListOfInt`

Constructors, in particular primary constructor, are very powerful. For example: class Guesser(val range: IntRange = 1..100) is equivalent to:

class Guesser(range: IntRange) {
  val range = range

  constructor(): this(1..100)
}

val is for read-only properties. If you need to re-assign a property, use var instead.
The use of raw strings with the trimMargin() member function is preferable over multiple println invocations.
The when expression is similar to the more traditional switch. You can learn more about it here.

This is the whole thing:

class Guesser(val range: IntRange = 1..100) {
  private var upperLimit = range.last
  private var lowerLimit = range.first
  private var turnCount = 0
  private var lastGuess = 0
  private var lastResponse = Response.UNKNOWN

  init {
    printIntro()

    while (lastResponse != Response.CORRECT) {
      lastResponse = presentNextGuess()
      turnCount++

      when (lastResponse) {
        Response.CORRECT -> println("I guessed your number in $turnCount turns.")
        Response.UNKNOWN -> println("I'm sorry, I didn't understand that.")
      }
    }
  }

  fun calcNextGuess(): Int {
    var roundingFactor = 0

    when (lastResponse) {
      Response.HIGHER -> {
        lowerLimit = lastGuess
        roundingFactor = 1 //compensates for rounding down
      }
      Response.LOWER -> {
        upperLimit = lastGuess
      }
      else -> assert(false) // Assume error if this is reached
    }

    return (lowerLimit + ((upperLimit - lowerLimit) / 2)) + roundingFactor
  }

  fun presentNextGuess(): Response {
    val nextGuess = calcNextGuess()

    println("Is your number $nextGuess?")

    val response = readLine()

    lastGuess = nextGuess

    return parseResponse(response)
  }

  fun parseResponse(response: String?) = when (response?.toLowerCase()) {
    null -> Response.UNKNOWN
    "higher", "h" -> Response.HIGHER
    "lower", "l" -> Response.LOWER
    "yes", "y" -> Response.CORRECT
    else -> Response.UNKNOWN
  }

  fun printIntro() {
    println(
      """
        Think of a number between 1 and 100. I am going to try and guess it.
        If i guess correctly respond with 'yes' (or 'y')
        Otherwise respond to with either 'higher' (or 'h') or 'lower' (or 'l')
        Okay, here we go...
      """.trimIndent()
    )
  }

  enum class Response {
    HIGHER, LOWER, CORRECT, UNKNOWN
  }
}

fun main(args:Array<String>) {
  Guesser()
}

Note: I'm also learning the language and I could be wrong with some things so, the above code can be improved and is not free of mistakes and non-idiomatic constructs.


ViewModel class in Typescript I am trying to refactor a ViewModel class because it has two methods that are really similar called updateRegions and updateTowns and even another one may end up being made called updateCountries. I basically copy-paste the method and change the variable names. How can this be refactored with maybe an abstract class that has an abstract updateArea function?

class SearchFilterViewModel {
        _regionsList = [];
        _townsList = [];
        _regions = ko.observableArray();
        _towns = ko.observableArray();
        selectedRegion: KnockoutObservable<string> = ko.observable('');
        selectedTown: KnockoutObservable<string> = ko.observable('');
        selectedCategories: KnockoutObservableArray<string> = ko.observableArray([]);

        constructor() {
            this.updateRegions("2186224");
            this.selectedRegion.subscribe(this.updateTowns.bind(this));
        }

        updateRegions(geonameId) {
            var self = this;
            self._regionsList = [];
            $.ajax({
                url: `http://api.geonames.org/children?geonameId=${geonameId}&username=elion`
            }).then(function(allRegionsXML) {
                var allRegionsJSON = xml2json(allRegionsXML);
                var allRegions = JSON.parse(allRegionsJSON);
                if(allRegions.geonames.length) {
                    for (var index = 1; index < allRegions.geonames.length - 1; index++) {
                    self._regionsList.push(allRegions.geonames[index].geoname);
                    }
                } else {
                    if(allRegions.geonames) {
                        self._regionsList.push(allRegions.geonames.geoname);
                    }
                }
                self._regions(self._regionsList);
            });
        }

        updateTowns(geonameId) {
            var self = this;
            self._townsList = [];
            $.ajax({
                url: `http://api.geonames.org/children?geonameId=${geonameId}&username=elion`
            }).then(function(allTownsXML) {
                var allTownsJSON = xml2json(allTownsXML);
                var allTowns = JSON.parse(allTownsJSON);
                if(allTowns.geonames.length) {
                    for (var index = 1; index < allTowns.geonames.length - 1; index++) {
                    self._townsList.push(allTowns.geonames[index].geoname);
                    }
                } else {
                    if(allTowns.geonames) {
                        self._townsList.push(allTowns.geonames.geoname);
                    }
                }
                self._towns(self._townsList);
            });
        }
    }

I have managed to get it down to this. It had been a while since I had encountered this scenario so I had to rack my brain for a bit and do some research. I basically followed this principle. Let me know if you have any suggestions for further improvements. Cheers.

interface iUpdateArea {
    _areasList: Array<string>;
    _areas: KnockoutObservableArray<string>;
    _selectedArea: KnockoutObservable<string>;
    updateSubArea(geonameId: string);
}

class Area implements iUpdateArea  {
    _areasList = [];
    _areas = ko.observableArray([]);
    _selectedArea = ko.observable('');

    updateSubArea(geonameId){
        var self = this;
        self._areasList = [];
        $.ajax({
            url: `http://api.geonames.org/children?geonameId=${geonameId}&username=elion`
        }).then(function(allRegionsXML) {
            var allRegionsJSON = xml2json(allRegionsXML);
            var allRegions = JSON.parse(allRegionsJSON);
            if(allRegions.geonames.length) {
                for (var index = 1; index < allRegions.geonames.length - 1; index++) {
                self._areasList.push(allRegions.geonames[index].geoname);
                }
            } else {
                if(allRegions.geonames) {
                    self._areasList.push(allRegions.geonames.geoname);
                }
            }
            self._areas(self._areasList);
        });
    }
}

class SearchFilterViewModel {
    _region: Area;
    _town: Area;

    constructor() {
        this._region = new Area();
        this._town = new Area();
        this._region.updateSubArea("2186224");
        this._region._selectedArea.subscribe(this._town.updateSubArea.bind(this._region));
    }
}

$(document).ready(function() {
    var _searchFilterViewModel: SearchFilterViewModel = new SearchFilterViewModel();
    var _searchFilterForm = $("#find-vegan-products-page").find("form")[0];
    ko.applyBindings(_searchFilterViewModel, _searchFilterForm);
    $('.select-multiple').each(function(i, obj) {
        //obj[i] gets each element inside the div
        $(this).multiselect({
            noneSelectedText: "CATEGORIES"
        });
    })
});


Project Euler #12 in Racket Problem here.

It's not exactly the brute force approach but I'm not using any sort of pre-calculated primes table, and I'm definitely not using the coprimality trick shown in the PE pdf. I'm finding the divisor count for each triangle number via its representation as a product of primes. More on it here.

I've written a bunch of similarly sized programs in Chicken Scheme before, and this is my 2nd Racket program.

Is there anything that I'm doing that isn't something a Racketeer would do? Am I reinventing anything that's already implemented?

#lang racket

(define (prime? n)
  (if (<= n 1)
      #f
      (not (ormap (lambda (x) (= (modulo n x) 0))
                  (stream->list (in-range 2 (add1 (truncate (sqrt n)))))))))

(define (next-prime start)
  (define (search candidate)
    (if (prime? candidate)
        candidate
        (search (add1 candidate))))
  (search (add1 start)))

(define (prime-factors n)
  (define (decompose num prime factors)
    (cond
      ([prime? num] (append factors (list num)))
      ([not (= 0 (modulo num prime))] (decompose num (next-prime prime) factors))
      (else (decompose (quotient num prime) prime (append factors (list prime))))))
  (if (= n 1)
      empty
      (decompose n (next-prime 1) empty)))

(define (remove-duplicates numbers)
  (define (remove numbers result)
    (cond
      ([empty? numbers] result)
      ([not (member (first numbers) result)]
       (remove (rest numbers) (append result (list (first numbers)))))
      (else
       (remove (rest numbers) result))))
  (remove numbers empty))

(define (divisor-count n)
  (let* ([factors (prime-factors n)]
         [no-duplicates-factors (remove-duplicates factors)])
    (apply * (map add1 (map (lambda (x)
                              (count (lambda (y) (= x y)) factors))
                            no-duplicates-factors)))))

(define (number->triangle-number n)
  (quotient (* n (add1 n))
            2))

(define (first-triangle-num-with property)
  (define (search candidate)
    (let ([triangle-candidate (number->triangle-number candidate)])
      (if (property triangle-candidate)
          triangle-candidate
          (search (add1 candidate)))))
  (search 1))

An answer is obtained via a call like:

(first-triangle-num-with (lambda (x) (>= (divisor-count x) 10)))

stream->list

Avoid stream->list on large lists.
In your prime? function it will create a (large)
list before passing it on to ormap.

There is a stream version of ormap - namedly stream-ormap, and
this will process the stream lazily - i.e. it will only generate
as many elements of the stream which are needed.

next-prime

Another way to eliminate the recursion here is to combine
stream-first with stream-filter and in-naturals with
your prime? predicate, e.g.:

(stream-first (stream-filter prime? (in-naturals ...) ))

Not that recursion is bad, but using streams results in
a more declarative definition.

append

I would avoid append in a Lisp or a Scheme.
I'm sure it's not efficient for use on lists.
cons, however, is always efficient, so in prime-factors you should
use:

(cons num factors)

instead of:

(append factors (list num))

You build the list in reverse order, but it won't matter
in this case.

remove-duplicates

You are also using append here which should be avoided.
The standard definition again uses cons:

(define (remove-duplicates xs)
   -- if xs is empty, return xs
   -- else return x followed by remove-duplicates on the tail elements not equal to x
     (cons (head x)
           (remove-duplicates (filter (lambda (y) (not (eq? x y))) (tail xs))))
)

For short lists this is fine - but note that its running time is O(n^2).
If you need a better one, there is one available in the standard library:
remove-duplicates in the standard library. On longer lists it uses a hash - you can view the source here: (link)

divisor-count

Honestly I had trouble understanding your prime-factors and divisor-count routines.

The conventional way of computing the number of divisors of a number n is:

find a prime divisor p of n
find the exponent e of the prime p in the factorization of n
repeat this process with n / p^e until n = 1

Then you have a list of primes and their exponents: (p1, e1), (p2, e2), etc.
and the number of divisors is:

(1+e1) * (1 + e2) * ...

How about this approach:

(define (divisor-count n)
  (apply * (map add1 (prime-exponents n))))

(define (prime-exponents n)
   -- if n == 1 return the empty list
   -- otherwise, find the first prime p dividing n
   -- find largest e s.t. p^e divides n
   -- return e and recurse on n / p^e   )

first-triangle-number

This is another place where using streams can help:

Create a stream of triangle numbers
Filter the stream keeping only those with large divisor counts
Take the first element of the stream

i.e.:

(stream-first (stream-filter ... (stream-map ... (in-naturals 1))))
                                                 \___ 1, 2, ... __/
                                 \__ 1, 3, 6, 10, ...          __/
              \___ triangular numbers having #divisors >= 50  __/
\___ answer to the problem                                   __/

Note that (in-naturals 1) is an infinite stream.

for, for/list

When you get comfortable using the stream- functions, look into
Racket's sequence comprehensions: for, for/list, for/vector, etc.

See http://docs.racket-lang.org/guide/for.html for more info.


Recursively find TypeScript files imported/exported from an entry point I'd like to know what's the best practice as functional programming in JavaScript to work with arrays and passing them as an argument to a recursive function that may run itself within its branch.

In my code I'm trying to recursively find all typescript files that are imported/exported from an entry point by calling preprocessTSFiles. To do this, I read the entry and crawl to all other files. This leads to a recursive call of getChildren and I don't want to call handleFile twice on the same file.

I tried this approach but I wanted an insight of expert in functional programming (please note that I'm not very familiar with functional programming and bear with me):

function copyFile(file: VinylFile, destFilePath: string): void {
  const destDir = path.dirname(destFilePath);
  ensureMakeDir(destDir);
  writeFile(destFilePath, file.contents.toString());
}

function getVinyl(filePath: string): VinylFile {
  const contents = new Buffer(readFile(filePath));
  return new vinyl({
    base: path.dirname(filePath),
    path: filePath,
    contents,
  });
}

function getChildren(file: VinylFile): VinylFile[] {
  const statementRE = /(import|export)\s*((\{[^}]*\}|\*)(\s*as\s+\w+)?(\s+from)?\s*)?([`'"])(.*)\6/ig;;
  const result: VinylFile[] = [];

  let match: RegExpMatchArray | null;
  while ((match = statementRE.exec(file.contents.toString())) && !isNil(match)) {
    const inFile = match[7];

    if (!isEmpty(inFile) && inFile[0] === '.') {
      let resolvedPath = path.resolve(file.base, inFile);
      if (/\.ts$/.test(resolvedPath) === false) {
        resolvedPath += '.ts';
      }
      if (isFile(resolvedPath)) {

        result.push(getVinyl(resolvedPath));
      } else if (isDirectory(resolvedPath)) {

        debugger;
        const indexBarrel = path.resolve(resolvedPath, 'index.ts');
        if (isFile(indexBarrel)) {
          result.push(getVinyl(indexBarrel));
        }
      }
    }
  }
  return result;
}

async function handleFile(file: VinylFile, destDir: string, baseDir: string, plugins: HandlerPlugin[], processedList: string[]): Promise<void> {

  for (const child of getChildren(file)) {
    if (processedList.indexOf(child.path) < 0) {
      processedList.push(child.path);

      await handleFile(child, destDir, baseDir, plugins, processedList);
    }
  }

  if (isArray(plugins)) {
    for (const plugin of plugins) {
      const result = await plugin(file);

      if (!isEmpty(result) && result !== file.contents.toString()) {
        file.contents = new Buffer(result);
      }
    }
  }

  const destFilePath = file.path.replace(baseDir, destDir);
  copyFile(file, destFilePath);
}

export function preprocessTSFiles(entryFilePath: string, destDir: string, baseDir?: string): Promise<void> {
  const fileList: string[] = [];
  const plugins = [InlineTemplate, InlineStyles];

  if (isNil(baseDir)) {
    baseDir = path.dirname(entryFilePath);
  }

  return handleFile(getVinyl(entryFilePath), destDir, baseDir, plugins, fileList);
}

Update

As the OP is been updated and changed I want to change my answer, I'll live the original at the end.

I see you use Object Oriented other than functional, so I'll comment some I saw that is not ok for me.

Encapsulation broken

Looking at your code I see the broken of encapsulation. This means that from a piece of code you access or worst you try to change some internal data of an object.

You should not do that. Rather you should improve the object to provide a propper interface.

Here is an example from your code in handleFile():

...
const destFilePath = file.path.replace(baseDir, destDir);
copyFile(file, destFilePath);
...

The issue here is you have to know to that baseDir should match with the file path attribute.

A better way would be just:

copyFile(file, destDir);

in this case you conain inside the copyFile() function the details about using the path.replace... staff. So you avoid to copy a file in another part of your source code and forgot this part.

Another reason to put the logic inside the copyFile() function is that you could actually remove that line:

function copyFile(file: VinylFile, destFilePath: string): void {
    const destDir = path.dirname(destFilePath);
    ensureMakeDir(destDir);
    writeFile(destFilePath, file.contents.toString());
}

I think you just want to place the file name at the end of the new path. The first line of the function here, you just do a rollback of this part.

function copyFile(file: VinylFile, destDir: string): void {
    ensureMakeDir(destDir);
    const destFileName = path.join(destDir, path.basename(file.path));
    writeFile(destFileName, file.contents.toString());
}

This is a little bit better because you contain the VinyFile details in a single function, and you could change it without worring that something will broke in your code.

But is not the best!
A better solution will be if the file know how to copy to another path, in such a way you should not deal with the object internal details.

You should be able to do:

file.copyTo(destDir);

If you try to write such a method you will notice that the code needed is simplest and much clear.

And you could maintain all the object boundary safe from the code base.

Another small thing here. I saw you use the path package before, so please avoid some trick like:

file.path.replace(baseDir, destDir);

It is difficult to understand and easy to broke.

Also the getVinyl() could be removed, the VinylFile should know how to load his content properly.

Here on getChildren():
    while ((match = statementRE.exec(file.contents.toString())) && !isNil(match))

Correction

  I checked the Javascript RegEx documentation of exec and in the examples they show a case where you use exec() with the option g. In this case you could have a loop as the regex object save the lastIndex and consider just a substring.

  Anyway, if you remove g option exec() will behave differently, and that is dangerous.

  For that reason I found out that the following sentence is not accurate.

Not sure why you use a loop as exec should return the matching. As the file content is not changing you will loop forever.

And this part I'm sure is not working:

    debugger;
    const indexBarrel = path.resolve(resolvedPath, 'index.ts');

There is still the debugger... :)
I think you run this code on server side so that line have no effects, but please remove it, just in case.

The getChildren() function should be moved as a method of VinylFile. Here I use only es6 syntax as I don't know Typescript, but I think it will be easy to move on the proper syntax.

// should be a VinylFile attribute or a private const
const statementRE = /(import|export)\s*((\{[^}]*\}|\*)(\s*as\s+\w+)?(\s+from)?\s*)?([`'"])(.*)\6/ig;

hasChildren() {

   if (subFilesMatch != null) {

   }
   return false;
}

getChildren(): VinylFile[] {
    const children: VinylFile[] = [];

    let subFilesMatch = statementRE.exec(this.contents.toString()));
    if (!isNil(subFilesMatch)) {
        const filePath = subFilesMatch[FILE_PATH_MATCH_INDEX];

        // Again don't use triks rather use the packages features!
        if (path.extname(filePath) !== '.ts') {
            filePath += '.ts';
        }
        const childFileName = path.join(this.base, filePath);
        if (isFile(childFileName)) {
            // Here I just rearrange your code, but you have an issue here!
            // I don't know how many children you have on each file instance
            // but *getChildren()* is synchronous function, so the file
            // content load is blocking!
            const contents = loadContents(childFileName);
            const opts = {base: this.base, path: childFileName, contents}
            children.push(new VinylFile(opts));
        } else if(isDirectory(childFileName)) {
            ...
        }
    }

    return children;
 }

As prompted in the comment above, your method here, even the original function, is blocking code, as it load the file content of the children sinchronously.

You should convert this too in an async function. I don't know the syntax in Typescript.

Last but not last handleFile()
It is an async function and should return a promise and it doesn't.

Maybe Typescript is honoring the function signature and returning a promise in any case, and, I guess, in a working like mode, so you think the code is fine.

Please check this part too.

As I said before, you should create an array of promises and return it from getChildren(), and then return a Promise.all() from handleFile().

I'll do this in javascript.

Original answer

I'm not a big expert of functional programming, but your example did not fit.

I guess so.

The first point is that you create a list of random numbers. This end up in a different list each time.

In functional programming, a function should take input and produce output, and given the input it should return always the same output.

So, you just set up a trivial example, but in my above definition you just could notice other issues.

The parameter name resultList, is not the "resultList", is just the start list that is going to be fit of 10 random numbers.

function SampleRecursiveFunction(resultList) {
  // copy our list
  const result = [...resultList];

Instead of resultList you should have a different name like: inputList.

Another bad name is result.

You should use some much helpfull like: randomNumberList

  function SampleRecursiveFunction(inputList, maxLength = 10) {
      if (inputList.length >= maxLength)
          return [...inputList];

      let randomNumber = Math.random()*100;
      while(inputList.length > 0 &&
            inputList.indexOf(randomNumber) !== -1)
          randomNumber = Math.random()*100;

      return SampleRecursiveFunction([...inputList, randomNumber], maxLength);
  }

This is a small evolution of your function with recursion.

The function accept a new parameter that set the returned list max length.

An even better version could be like:

function GetRandomNumber(seed) {
    return function () {
        return Math.random() * seed;
    }
}

This function return a function that provide the proper next value.

In real case you could have a generation function instead, depending on what you're doing.

function SampleRecursiveFunction(inputList, getNewNumber, maxLength = 10) {
      if (inputList.length >= maxLength)
          return [...inputList];

      let randomNumber = getNewNumber();
      while(inputList.length > 0 &&
            inputList.indexOf(randomNumber) !== -1)
          randomNumber = getNewNumber();

      return SampleRecursiveFunction([...inputList, randomNumber], maxLength);
  }

And you could compose the function like this:

  console.log(SampleRecursiveFunction([], GetRandomNumber(100)));

The good of this is you could split your logic into small tasks and put them in functions. At the end you compose those functions and building your whole logic.

As we deal with random numbers, even my example is not pure functional, I just try to give you a review of your code example.

I think you should find a more detailed example on what you're tring to do to have a much usefull help.


Parsing argv with while-switch construct I've been thinking about how to create a robust function that parses the command line arguments for valid filenames. I came up with a while-switch construct because that allows me to reduce redundancy (no break-statement at the end of case 2).

As I am pretty new to programming, I can't really tell the quality of this code.
Can you think of a way to improve this code in robustness as well as in elegance?

void spellchecker::openFiles(int argc, char* argv[]) {
    bool filesOpen = false;
    std::string dict_filename;
    std::string text_filename;

    while (filesOpen == false) {
        switch (argc) {

            case 2:
                printf("You already entered the name of the dictionary file as \"%s\". Is this correct? (y/n): ", argv[1]);
                if (std::cin.get() == 'y') {
                    printf("Ok, please enter the name of the text file you want to check: ");
                    std::cin >> text_filename;
                    break;
                }
                /* no break so user can enter both filenames*/
            default:
            case 1:
                printf("Please enter the name of the dictionary: ");
                std::cin >> dict_filename;
                printf("Please enter the name of the text file to check: ");
                std::cin >> text_filename;
                break;

                case 3:
                    dict_filename = argv[1];
                    text_filename = argv[2];
                    break;
        } // switch

                if (!dict_file.is_open()) {
                    dict_file.open(dict_filename.c_str(), std::ifstream::in);
                }
                if (!text_file.is_open()) {
                    text_file.open(text_filename.c_str(), std::ifstream::in);
                }
                if (dict_file.good() && text_file.good()) {
                    filesOpen = true;
                }
    } // while
}

This solution does not sit well with me.  It looks like you're doing too much: checking how to take the input, taking input, and then checking whether that input worked all at once.

Before looking at the code, though, it's worth asking whether this is really the interface that you want to provide to the user.  The way you've designed it, it looks like it is meant to be run by a human, as the requests for input will only muddle the output in a script.  Furthermore, spell-checkers are usually non-destructive: running it with the wrong input file or the wrong dictionary file shouldn't lose you any data.

All in all, this makes double-checking with the user whether the dictionary file is correct questionable.  It is also inconsistent: you do validate the input in that case, but don't in the case that both files are provided, so the user has to remember that if he intends to rely on the input being checked.

My first suggestion is thus to change your interface: only prompt for things that are not already provided, and if you detect an error, report it and exit.

Now to the code: you have a few cases where the program can go in an infinite loop.  This happens if the user closes std::cin, or if the user provided two filenames via argv and then at least one failed to open: you'll keep trying to open it and filesOpen will never be true.

Secondly, you're using operator>> in most places, but use std::cin.get() to ask the user whether he's sure.  This will likely break if the user provides one command-line argument and then some file fails to open.

Thirdly, the condition of your loop seems strange to me.  Why use an extra bool that you then set at the very end instead of using break or return to get out of the loop, or, better, checking the condition directly?  You can use a do-while loop if you want to save yourself one check, though I wouldn't bother as the performance effect will be negligible, while the code will be clearer.

As for the switch construct: I'd use a helper function or two instead.  The code is sufficiently small for it to be clear, especially with the comment, but I don't think it wins you anything over two function calls.  In general, the function looks too big to me, but I've already covered that above.

Last and least, mixing std::printf and std::cin is strange.  I can't come up with any technical problems, but mixing two styles without a good reason is generally more trouble than it's worth.


Print diagonal numbers of matrix in ascending order Today I attempted an interview problem:

  Q: An n×n matrix contains numbers from 1 to n×n and is generated in the
  fashion:

  The first row is of numbers starting from 1 to n left to right, Second row is from n+1 to 2n right to left, 3rd row is from 2n+1 to 3n left to right and so on.

  Example n = 4

  Matrix would look something like

 1  2  3  4
 8  7  6  5
 9 10 11 12
16 15 14 13

  Diagonal elements: 1, 4, 6, 7, 10, 11, 13, 16.

Wrote this code, got an feedback as space and time complexity could be further improved.
How to improve it?

def create_matrix(n):
    mat = []
    num = 1
    reverse_flag = True
    for i in range(n):
        row = []
        reverse_flag = False if reverse_flag else True
        for j in range(n):
            row.append(num)
            num += 1
        if reverse_flag:
            row.reverse()
        mat.append(row)
    print_matrix(mat, n)

def print_matrix(mat, n):
    reverse_flag = True
    for i in range(n):
        reverse_flag = False if reverse_flag else True
        if reverse_flag:
            if i == n-1-i:
                print mat[i][i]
            else:
                print mat[i][n-1-i], mat[i][i]
        else:
            if i == n-1-i:
                print mat[i][i]
            else:
                print mat[i][i], mat[i][n-1-i]

create_matrix(4)

It's bizarre that create_matrix() calls print_matrix().  According the Single Responsibility Principle, each function should do just one thing.  And if the function is called create_matrix(), then it should just return a matrix, and do nothing else.

If your create_matrix() function already implements the boustrophedon arrangement, then I see no reason why print_matrix() should also have a toggling reverse_flag.  That is, I consider the output

1 4
6 7
11 10
16 13

… to be wrong, as it is neither sorted in ascending order (1, 4, 6, 7, 10, 11, 13, 16) nor alternating (1, 4, 7, 6, 10, 11, 16, 13).

You don't actually have to create the matrix to print the diagonal entries.  Just use arithmetic.

def matrix_diagonals(n):
    for row_num in xrange(n):
        row_min = n * row_num + 1
        row_max = n * row_num + n
        diag1 = row_min + row_num
        diag2 = row_max - row_num
        yield max(diag1, diag2) if row_num % 2 else min(diag1, diag2)
        if diag1 != diag2:
            yield min(diag1, diag2) if row_num % 2 else max(diag1, diag2)

print list(matrix_diagonals(4))

You could even simplify the arithmetic a bit:

def matrix_diagonals(n):
    for row_num in xrange(n):
        diag1 = (n + 1) * row_num + 1
        diag2 = (n - 1) * row_num + n
        yield max(diag1, diag2) if row_num % 2 else min(diag1, diag2)
        if diag1 != diag2:
            yield min(diag1, diag2) if row_num % 2 else max(diag1, diag2)

Since you are using Python 2, xrange() would be more efficient than range().


Doing a string replace How does the following look for doing a string replace in C? Is this approach more common than allocating memory within the function and returning a new string pointer, or is the bring-your-own-buffer more common?
#include <stdio.h>
#include <string.h>

// return size of string, -1 if error such as buffer too small
// no memory is allocated here, buffer needs to be passed in
// Example: str_replace("Replace me", "me", "ME", buffer, 64);
// Returns: 10, buffer is now "Replace ME".
ssize_t
str_replace (
        const char* input_str,
        const char* from,
        const char* to,
        char* output_buffer,
        size_t max_size
) {
    size_t input_len = strlen(input_str);
    size_t from_size = strlen(from);
    size_t to_size = strlen(to);

    // get the output string length;
    size_t output_len = input_len;
    char* str_iterator = (char*) input_str;
    while ((str_iterator=strstr(str_iterator, from))) {
        output_len += (to_size - from_size);
        str_iterator += from_size;
    }
    if (output_len > max_size) {
        fprintf(stderr, "Buffer too small for output string.\n");
        return -1;
    }

    // reset str_iterator, make sure output_buffer is clear
    str_iterator = (char*) input_str;
    output_buffer[0] = '\0';

    // copy to output buffer
    // 1. Find the first match of `from` in `input_str` (advancing pointer on input_buffer along way)
    // 2. Copy the input_str up until the first match to output_buffer
    // 3. Copy `from` into output_buffer (advance string)
    // 4. Advance input_buffer pointer by size of `from`
    // 5. Repeat 2-5 until no more matches
    // 6. Copy remainder of input_str to output_buffer
    for (char* match; (match=strstr(str_iterator, from)); str_iterator+= (match-str_iterator) + from_size) {
        strncpy(output_buffer, str_iterator, match-str_iterator);
        output_buffer += (match-str_iterator);
        strncpy(output_buffer, to, to_size);
        output_buffer += to_size;
    }
    // copy remainder
    if (strlen(str_iterator))
        strcpy(output_buffer, str_iterator);

    return output_len;
}

int main(void)
{
    char buffer[50];
    ssize_t out_size = str_replace("Replace me", "me", "ME", buffer, 50);
    if (out_size == -1)
        printf("Nope!\n");
    else
        printf("Response size: %zu.\nResponse string: %s\n", out_size, buffer);
}

$ gcc logmain.c -o log; ./log
Response size: 10.
Response string: Replace ME

And another alternative:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

// same as above but now with malloc
char* str_replace2(const char* input, const char* from, const char* to)
{
    size_t input_len = strlen(input);
    size_t from_size = strlen(from);
    size_t to_size = strlen(to);
    char* str_iterator = (char*) input;

    // get the output string length;
    size_t output_len = input_len;
    while ((str_iterator=strstr(str_iterator, from))) {
        output_len += (to_size - from_size);
        str_iterator += from_size;
    }
    char *str_out = malloc(output_len);
    char * const str_out_start = str_out; // const for emphasis it doesn't change
    if (!str_out) {
        fprintf(stderr, "Cannot malloc str of size %zu", output_len);
        return NULL;
    }

    // write to output str
    str_iterator = (char*) input;
    for (char* match; (match=strstr(str_iterator, from)); str_iterator+= (match-str_iterator) + from_size) {
        strncpy(str_out, str_iterator, match-str_iterator);
        str_out += (match-str_iterator);
        strncpy(str_out, to, to_size);
        str_out += to_size;
    }
    // copy remainder
    if (strlen(str_iterator))
        strcpy(str_out, str_iterator);

    return str_out_start;
}

Additionally, where are comments usually put in C? Above the function? In the function?

Few things to consider.

Why are you writing your own implementation of something that already exists, long history of reliable use with well defined behaviour (fun/assignment - sure, generally - waste of effort likely to lead to problems)?

What if input, from, or to are null, or not valid (not \0 terminated), OR overlap in any way (will it work)?

from and to, might imply to some people replace from here, to there.  Names are important - look for things that are self-evident.  Perhaps matchToReplace and replaceMatchWith - a bit wordy, but hopefully less ambiguous.

What happens if from is empty - what is the sensible result (document it)?

Don't you need to malloc one more than the desired length (for the terminating \0) [didn't read the algorithm carefully enough sorry].

Make sure you have test cases checking all the above, but also confirming that when you copy 0 length things, or 1 - that everything is copied properly.

Documenting - what tool are you using to publish it?  Certainly doesn't hurt to put a comment before - but remember what people are looking at is the headers - for the declaration, not the implementation (generally at least).


Java synchronization task I have the following task:

  Implement a simple command line Java application for booking bus
  seats. For simplicity suppose there is only one bus, initially all
  seats (lets say 50) are free. Users can book one seat at time through
  the application API, the application itself should try to take the
  first available seat from 1 to 50, if any left. Make sure your
  application is thread-safe (i.e. many users can book tickets at the
  same time, but cases such booking the same seat from more than one
  user must be avoided) using the conventional locking mechanisms in
  available in Java.

The program I have written so far looks like this:

public class Bus{

    private final boolean [] seats = new boolean[50];
    private int nextSeat = 0;

    public void bookSeat() throws Exception{
        if(nextSeat<seats.length){
        seats[nextSeat]=true;
        nextSeat++;
        System.out.print("Seat number " +nextSeat+ " booked");
        }else{
            System.out.println("The bus is full sorry");
        }
        }

}

public class Passenger extends Thread{

    Bus bus;
    String passengerName;

    public Passenger(Bus bus, String passengerName){
        this.bus=bus;
        this.passengerName=passengerName;
    }

    public void run(){
        synchronized(bus){
            try {
                bus.bookSeat();
                Thread.sleep(500);
            } catch (Exception ex) {
                Logger.getLogger(Passenger.class.getName()).log(Level.SEVERE, null, ex);
            }
            System.out.println("by " + passengerName);

        }
    }

    public String getPassengerName() {
        return passengerName;
    }

    public void setPassengerName(String passengerName) {
        this.passengerName = passengerName;
    }
}

public class Main {
    public static void main(String [] args) throws InterruptedException{
        Bus someCompany = new Bus();

        Passenger p1 = new Passenger(someCompany,"Name1");
        Passenger p2 = new Passenger(someCompany, "Name2");

        p1.start();
        p2.start();

    }
}

Does my solution satisfy the task? I want to make sure to do so and to improve my solution.

Yes but...

The synchronization goes against the same bus, that means that every thread in the JVM must wait for other threads who are in the synchronization-block for this bus-instance.

You solved the problem but it realy needs a review:

The prof asked you to use a API. Even if there really is an API you did not named it. The prof must have a solid and clean evidence that you know what exactly an API is.
A seat is not a boolean, the state of a seat can be an boolean. Please wrap the boolean in a class named "Seat".
A passenger neither inherits an Thread nor is to Book a seat the only work a passenger will have. If a Passenger like to book two seats, one for himself and one for his stuff. From documentation of thread this is not possible because: It is never legal to start a thread more than once. Rename the class Passenger to Booking, you can book only once but you can book for multiple persons.
As GreenNovice reason pointedly: The synchronization is not the responsibility of the Passenger, its the responsibility of the Busdriver (lets say the Bus). As GreenNovice clearly worked out, please move the synchronization into the signature of the bookSeat or into the code of bookSeat.
The synchronization works well, no need for the Thread.sleep().
The variable-name of a bus is someCompany? If i read a variable is named someCompany i like to sell all my stocks before i try to call someCompany.stop(). A better name of the variable might be someCompanysCar!

Theese are problems who must occour - because scientific programming is always collective work. No need for blame you.


Calculator with simple input This is "my" version of a basic C++ calculator, that requires a single line input (like 1+1) to calculate the result. I have been (self)learning the language for 2 months now. I don't pretend to have invented this code, but I haven't taken the time to search for this variation.

The functions included are:

Addition: 1 + 1 = 2

Input in program: 1+1
Subtraction: 1 - 1 = 0

Input in program: 1-1
Multiplication: 1 * 1 = 1

Input in program: 1*1
Division: 1 / 1 = 1

Input in program: 1/1
Exponentiation: 2 ^ 2 = 4

Input in program: 2^2

#include <iostream>
#include <string>

using namespace std;

class Math_Functions
{
private:
    double result;

public:
    double addition(double, double);
    double subtraction(double, double);
    double multiplication(double, double);
    double division(double, double);
    double exponentiation(double, double);
} math;

double Math_Functions::addition(double a, double b)
{
    result = a + b;
    return result;
}

double Math_Functions::subtraction(double a, double b)
{
    result = a - b;
    return result;
}

double Math_Functions::multiplication(double a, double b)
{
    result = a * b;
    return result;
}

double Math_Functions::division(double a, double b)
{
    result = a / b;
    return result;
}

double Math_Functions::exponentiation(double a, double b)
{
    result = 1;
    for (int i = 0; i < b; i++)
    {
        result *= a;
    }
    return result;
}

int main()
{
    double a, b;
    string function;
    bool loop = 1;

    while(loop)
    {
        system("CLS");
        cin >> a;
        function = getchar();
        cin >> b;

        switch(function[0])
        {
            case '+' : cout << math.addition(a, b); break;
            case '-': cout << math.subtraction(a, b); break;
            case '*': cout << math.multiplication(a, b); break;
            case '/': cout << math.division(a, b); break;
            case '^': cout << math.exponentiation(a, b); break;
        }
        cout << endl << "Press Any Key to Continue . . . ";
        cin.sync();
        cin.ignore();
    }
    return 0;
}

If you're about to perform division, you must first check for 0 as the given divisor.  To prevent this, it may be best to warn the user, and make sure they input a valid number before proceeding.

For the exponentiation, you can have a "shortcut" by returning 1 instantly if b is 0.  There's no need to attempt calculations since anything raised to the 0 power is always 1.  You should also handle negative powers in some way.  The code will break if the user inputs such a value for b.  This loop cannot handle all of these possible values.


Treating Optional values I have a situation when  there could be and could not be values in database, which is why i return optional, so the response object that goes back to the UI could be different depending on the existing or not values.
Examples :
 new UserLogsResponse(auditEntity.get().getTimestamp(), latestRetrievedDocumentDateAndTime.get(),emailFromAuditMessage, tokeDateAndTimeStamp)

As you can see it where it says object.get() its me getting value from the optional.
I would like to avoid numerous if blocks saying if value is Present then add object to the response constructor so currently I am not checking the optional with is present and passing values to the constructor of a response object and inside the constructor i do certain checks and based on checks i either return null or do some operations with not null value and its fine it does the job, however code does not looks good since  it highlights get() in optional objects when i do not proceed with checks before passing it to my response object.
@Getter
@Setter
@JsonInclude(JsonInclude.Include.NON_NULL)
public class UserLogsResponse {

    private final String latestSentEmailStamp;
    private final String latestDocumentRetrievalStamp;
    private final String privateEmail;
    private final String loginTimeStamp;

    public UserLogsResponse(LocalDateTime latestSentEmailStamp, LocalDateTime latestDocumentRetrievalStamp, String privateEmail, LocalDateTime loginTimeStamp) {
        this.latestSentEmailStamp = latestSentEmailStamp == null ? null : LocalDateFormatter.ddMMyyAtHHmm(latestSentEmailStamp);
        this.latestDocumentRetrievalStamp = latestDocumentRetrievalStamp == null ? null : LocalDateFormatter.ddMMyyAtHHmm(latestDocumentRetrievalStamp);
        this.privateEmail = privateEmail == null ? null : new Email(privateEmail).hideSomeEmailCharacters();
        this.loginTimeStamp = loginTimeStamp == null ? null : LocalDateFormatter.ddMMyyAtHHmm(loginTimeStamp);
    }
}

  public UserLogsResponse getUserLogs(HttpServletRequest request) {
    UserEntity userEntity = currentUser.get();
    Optional<LocalDateTime> tokeDateAndTimeStamp = tokenService.getPreviousLoginStamp(request,userEntity, LocalDateTime.now());
    Optional<LocalDateTime> latestRetrievedDocumentDateAndTime = auditRepository.getLatestRetrievalDocumentDateAndTime(userEntity.getId());
    Optional<AuditEntity> auditEntity = auditRepository.getLatestSendEmailDateAndTime(userEntity.getId());
    String emailFromAuditMessage = new AuditMessageExtractor(auditEntity.get().getMessage()).extractEmail();
    return tokeDateAndTimeStamp.map(localDateTime -> new UserLogsResponse(auditEntity.get().getTimestamp(), latestRetrievedDocumentDateAndTime.get(), emailFromAuditMessage, localDateTime))
            .orElseGet(() -> new UserLogsResponse(auditEntity.get().getTimestamp(), latestRetrievedDocumentDateAndTime.get(),emailFromAuditMessage, null));
}

Would appreciate any suggestions, as I said everything works and its good but if i was another developer and saw optional objects without ifPresent check it would be confusing.

Ah sorry guys i found a solution:
Will simply do the following and null checks will be handled in constructor
 LocalDateTime tokeDateAndTimeStamp = tokenService.getPreviousLoginStamp(request,userEntity, LocalDateTime.now()).orElseGet(null);
    LocalDateTime latestRetrievedDocumentDateAndTime = auditRepository.getLatestRetrievalDocumentDateAndTime(userEntity.getId()).orElseGet(null);
    AuditEntity auditEntity = auditRepository.getLatestSendEmailDateAndTime(userEntity.getId()).orElseGet(null);


Serve folder content using PHP route (SSI) (dynamic includes) I have just written a PHP router, but I have some security concerns about this function. I am trying to serve all the content of a folder using this function. I am trying to prevent harmful URLs.

public static function set_up($reletive_path = true)
{
    self::$url = parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH);
    if ($reletive_path && self::$url !== "/") {
        $cut_of = strlen(dirname($_SERVER['PHP_SELF']));
        self::$url = substr(self::$url, $cut_of);
    }
    if (substr(self::$url, -1) == "/") {
        self::$url = substr(self::$url, 0, -1);
    }
    if (self::$url == "") {
        self::$url = "/";
    }
}

/**
 * Serves ONLY php and html files from a folder, the full path with extension must be provided
 * @param $url_path string Route to access the folder
 * @param $foldername string Path to the folder to serve
 * @param string|array $methods String or array of methods there will execute this route, all methods MUST be in UPPERCASE
 * @return bool True if route is executed
 */
public static function serve_folder($url_path, $foldername, $methods = "ALL")
{
    /** unrelated code */
    $foldername = rtrim($foldername, "/");

    $query_patten = $url_path;
    if (substr(self::$url, 0, strlen($query_patten)) === $query_patten) {
        $sub_path = substr(self::$url, strlen($query_patten));
        if ($sub_path === false) {
            if (file_exists($foldername . "/index.php") && is_file($foldername . "/index.php")) {
                require $foldername . "/index.php";
                self::$route_found = true;
                return true;
            } else if (file_exists($foldername . "/index.html") && is_file($foldername . "/index.html")) {
                require $foldername . "/index.html";
                self::$route_found = true;
                return true;
            }
        } else if (preg_match('/^(\/[a-zA-Z0-9-_]+)*\/[a-zA-Z0-9-_]+\.(html|php)$/', $sub_path)) {  // matches urls like /folder/subfolder/file.php /subfolder/file.html file.php
            $full_file_path = $foldername . $sub_path;
            if (file_exists($full_file_path) && is_file($full_file_path)) {
                require $full_file_path;
                self::$route_found = true;
                return true;
            }
        } else if (preg_match('/^(\/[a-zA-Z0-9-_]+)*\/[a-zA-Z0-9-_]+(\/)?$/', $sub_path)) { // matches urls like /folder/subfolder/ /folder/subfolder /folder
            $full_file_path = $foldername . rtrim($sub_path, "/");
            if (file_exists($full_file_path . "/index.php") && is_file($full_file_path . "/index.php")) {
                require $full_file_path . "/index.php";
                self::$route_found = true;
                return true;
            } else if (file_exists($full_file_path . "/index.html") && is_file($full_file_path . "/index.html")) {
                require $full_file_path . "/index.html";
                self::$route_found = true;
                return true;
            }
        }
    }
    return false;
}

EDIT

A usage example would be following:

You are having following folder structure:

/router.php (all traffic is routed to this file)

/homepage.php

/functions/signup.php

/functions/login.php

/functions/admin/add_article.php

// router.php
// the libery is called Highway
Highway::get("/", function () {
    include "homepage.php";
});
Highway::serve_folder("/actions", "functions/");

Following request will include following file:

website.com -> /homepage.php

website.com/actions/signup.php -> /functions/signup.php

website.com/actions/login.php -> /functions/login.php

website.com/actions/admin/add_article.php -> /functions/admin/add_article.php

Thanks for your example.  It kind of validates my concern on how you are thinking about routing.

I like that you are thinking about the routing problem and trying to move away from legacy ways of having each URL going directly to a physical file for processing.  I would however challenge you to take this a step further, as what you have now really only gets partially away from the legacy web application, a script-for-every controller/action line of thinking, in that you still have scripts that you are redirecting to rather then using your router to instantiate an appropriate controller object in order to satisfy the request.

Most modern frameworks that utilize a front-controller routing pattern like you are trying to achieve have gone the route of auto-loading a controller (ideally auto-loaded in a PSR-4 compliant way) to handle the request passed to it by the router, not having the router just include some specific script file.

So you might see a pattern like:

$controller = Router::getController($request);
$controller->execute();

That's it.  The router is basically used on a single line in your main execution path.

Here all the information about the HTTP request (including URI) could be injected into a controller object, which then takes over application control.  In this case, the router needs to know absolutely nothing about what dependencies that controller has. It typically just needs to know how to map the
 request (oftentimes just the first segment of the URI) to a controller to be instantiated and hand off the request information and application control over to the controller.

In your case, your router actually does the following:

It parses the request (something better handled in its own class such that a request object can be passed around as a dependency to consuming code).
It uses information from the request to map the request to an action to be performed (the only thing a router should truly do).
It has to understand where dependencies for a particular action are located in the filesystem.
It actually loads/executes those dependencies.

If you are striving to adhere to the single responsibility principle, you will quickly understand that the router has too much responsibility in your application. It should own just routing the request to an appropriate controller and then get out of the way.


Simple 1-dimensional labyrinth I am training a colleague who is familiar with Python but has a limited knowledge of C# and complexity. I thought the following problem could be a good start.

You are in a desert, there is a (infinite) wall in front of you and you know there is exactly one door, but you don't know if it is on the left or on the right.

I implemented the labyrinth this way:

using System;

namespace _1d_labyrinth
{
    public class Labyrinth
    {
        private int _exitPosition;
        private int _currentPosition;
        private int _nSteps;

        public int Steps
        {
            get { return _nSteps; }
        }

        public bool Solved
        {
            get { return _currentPosition == _exitPosition; }
        }

        public Labyrinth(int exitPosition)
        {
            _nSteps = 0;
            _currentPosition = 0;
            _exitPosition = exitPosition;
        }

        public bool MoveRight()
        {
            _nSteps++;
            _currentPosition++;
            return _currentPosition == _exitPosition;
        }

        public bool MoveLeft()
        {
            _nSteps++;
            _currentPosition--;
            return _currentPosition == _exitPosition;
        }
    }
}

The strategy evaluator (which just counts the total number of steps taken to solve a fixed set of labyrinths)

using System;

namespace _1d_labyrinth
{
    public static class StrategyEvaluator
    {
        public static int Run(ISolver solver, int steps)
        {
            Random rnd = new Random(1);
            int totalSteps = 0;
            for (int i = 0; i < steps; i++)
            {
                Labyrinth lb = new Labyrinth(rnd.Next() * ((rnd.Next() % 2) - 1));
                solver.Run(lb);
                if (lb.Solved)
                    totalSteps += lb.Steps;
                else
                    throw new Exception("Not solved !");
            }
            return totalSteps;
        }

    }
}

Where a solver is just a way to solve the labyrinth.

namespace _1d_labyrinth
{
    interface ISolver
    {
        void Run(Labyrinth lb);
    }
}

And this is my solution (where multiplier will be 2 or 3)

namespace _1d_labyrinth
{
    public class Solver1 : ISolver
    {
        private int _multiplier;

        public Solver1(int multiplier)
        {
            _multiplier = multiplier;
        }

        public void Run(Labyrinth lb)
        {
            bool finished = false,
                moveRight = true;

            int stepsToTake = 1,
                stepsTaken = 0;

            while (!finished)
            {
                while (stepsTaken < stepsToTake)
                {
                    if (moveRight)
                        finished = lb.MoveRight();
                    else
                        finished = lb.MoveLeft();
                    stepsTaken++;
                }
                moveRight = !moveRight;
                stepsToTake *= _multiplier;
            }
        }
    }
}

It allows to introduce interfaces and complexity. Is there anything I could add or make better in the example ?

I'm focusing on the various naming issues.

Why _nSteps? Why not simply _steps? Same for rnd instead of random, lb instead of labyrinth. The General Naming Conventions say:

  In general, you should not use abbreviations or acronyms. These make
  your names less readable.

What kind of name is _1d_labyrinth for a namespace? The General Naming Conventions say:

  Do not use underscores, hyphens, or any other nonalphanumeric
  characters.

Also, a namespace's name should be PascalCase, so in this case OneDimensionalLabyrinth.

Why Solver1? Is there also a Solver2 and a Solver3 etc? What are their differences? Base your class name on the particulars of your class.


Programming Language Identification in JS This is a RegExp exercice from HackerRank. Basically, I have to write a program (I chose JavaScript) that receives a program as an input (string) and tries to identify the programming language used. We're only concerned by 3 languages: C, Java or Python.

function processData(input) {
   console.log(identifyLanguage(input));
}

var identifyLanguage = function (code) {
    if (isJava(code))
        return 'Java';
    else if (isC(code))
        return 'C';
    else if (isPython(code))
        return 'Python';
    else return 'C';
}

var isC = function(input) {
    const cPreprocessor = /#(include.*?>|define.*)/g;
    const cStruct = /\b(typedef.*?;|struct.*?(?=\s*?\{))/g;
    const cPointers = /\b\w+\s*->\s*\w+\b/g;
    const cFunctionPointerReturn = /[a-zA-Z]+\s+\*\s+([a-zA-Z_]\w*)\s*\([a-zA-Z]*\s*([a-zA-Z_]\w*)\)\s*\{/g;
    const cFunctionPointerArg = /[a-zA-Z]+\s*\*?\s*([a-zA-Z_]\w*)\(.*[a-zA-Z]+\s*\*\s*([a-zA-Z_]\w*).*\)\s*\{/g;
    return cPreprocessor.test(input) || cStruct.test(input) || cPointers.test(input) ||
           cFunctionPointerArg.test(input) || cFunctionPointerReturn.test(input);
}

var isPython = function (input) {
    const pythonKeywords = /(class|def|if|while|else|for).*:/g;
    const pythonLists = /\[((\d+|("|').+\3),)*(\d+|("|').+\5)\]/g;
    const pythonPrint = /\bprint(\s*("|').+\2|\(.*\))/g;
    const pythonBoolean = /True|False/g;
    return pythonKeywords.test(input) || pythonLists.test(input) ||
           pythonPrint.test(input) || pythonBoolean.test(input);
}

var isJava = function(input) {
    const javaImport = /\bimport\s*.*?;/g;
    const javaClass = /(public|private)?\s*class.*?\{/g;
    return javaImport.test(input) || javaClass.test(input);
}

I can't say I'm satisfied with this, although it passes all the test cases, but It feels kinda ugly, non-DRY and amateurish. What can I do to improve it?

Update

The previous code updated after the suggestions provided in the chosen answer by Joseph :

var langPatterns = {
    C: [
        /#(include.*?>|define.*)/g, //cPreprocessor
        /\b(typedef.*?;|struct.*?(?=\s*?\{))/g, //cStruct
        /\b\w+\s*->\s*\w+\b/g, //cPointers
        /[a-zA-Z]+\s+\*\s+([a-zA-Z_]\w*)\s*\([a-zA-Z]*\s*([a-zA-Z_]\w*)\)\s*\{/g, //cFunctionPointerReturn
        /[a-zA-Z]+\s*\*?\s*([a-zA-Z_]\w*)\(.*[a-zA-Z]+\s*\*\s*([a-zA-Z_]\w*).*\)\s*\{/g //cFunctionPointerArg
    ],
    Java: [
        /\bimport\s*.*?;/g, //javaImport
        /(public|private)?\s*class.*?\{/g //javaClass
    ],
    Python: [
        /(class|def|if|while|else|for).*:/g, //pythonKeywords
        /\[((\d+|("|').+\3),)*(\d+|("|').+\5)\]/g, //pythonLists
        /\bprint(\s*("|').+\2|\(.*\))/g, //pythonPrint
        ///True|False/g //pythonBoolean
    ],
}

var identifyLanguage2 = function (code) {
    return Object.keys(langPatterns).find(lang =>
        langPatterns[lang].some(pattern =>
            pattern.test(code)));
}

Instead of ||, you could stuff the patterns in an array and use array.some(). If one of the patterns match the string, it will return immediately with true and skipping the rest of the array.

const patterns = []
patterns.push(pattern1)
patterns.push(pattern2)
patterns.push(pattern3)
return patterns.some(p => string.match(p))

Taking it further, you can use a key-value pair of language name and patterns. Then use Object.keys and array.find to find which key has the set of patterns that match the string. For the sake of brevity, variable names are shortened.

const l = {
  java: [/* array of java patterns */],
  python: [/* array of python patterns */],
  c: [/* array of c patterns */]
}

// "For each language in languages, find the language that contains a
// pattern that matches the string"
const language = Object.keys(l).find(k => l[k].some(p => s.match(p)))

Also note that some languages have common syntaxes. If you want to go this route, you'll have to find syntax that's unique to the language or risk mistaking one language for another. For instance, True may be a Python boolean... or a variable name in Java. The same with class, is it a Python class or Java class. What if I defined a string in Java that's valid Python?