Retrieve words from dictionary when they meet letter requirements I have a set of functions that retrieve words from some arbitrary dictionary based on what letters they have. For example, this function gets words that use only the specified letters:

function getWordsWithOnlySpecifiedLetters(array $dictionary, string $letters)
{
    foreach ($dictionary as $key => $value) {
        if(mb_strlen($value) > mb_strlen($letters)) unset($dictionary[$key]);
    }

    $step = 0;
    $strSplit = preg_split('//u', $letters, null, PREG_SPLIT_NO_EMPTY);
    $result = [];
    foreach ($dictionary as $word) {
        $step++;
        $wordSplit = preg_split('//u', $word, null, PREG_SPLIT_NO_EMPTY);
        sort($wordSplit);
        sort($strSplit);
        if (array_map('mb_strtolower', $wordSplit) === array_map('mb_strtolower', $strSplit)) {
            //echo "All specified letters from $letters are in $word | Step $step
" . PHP_EOL;
            $result[] = $word;
        }
    }

    return $result;
}

Example usage:

$dictionary = ['apple', 'sample', 'api', 'pia', 'тоҷик'];
getWordsWithOnlySpecifiedLetters($dictionary, "aip");

This would return the words api and pia.

getWordsWithOnlySpecifiedLetters($dictionary, "leamps");

This one would return the word sample.

I also have a function that doesn't require that they exclusively use the selected letters, but rather that they use all of the specified letters (and any other letters).

function getWordsWithSpecifiedLetters(array $dictionary, string $letters)
{
    $step = 0;
    mb_internal_encoding("UTF-8");
    $result = [];
    foreach ($dictionary as $word) {
        $step++;
        $wordSplit = preg_split('//u', $word, null, PREG_SPLIT_NO_EMPTY);
        $strSplit = preg_split('//u', $letters, null, PREG_SPLIT_NO_EMPTY);
        $wordSplit = array_filter($wordSplit, function($x) use (&$strSplit) {
            if (in_array(strtolower($x), array_map('strtolower', $strSplit), true)) {
                $pos = array_search(strtolower($x), array_map('strtolower', $strSplit), true);
                unset($strSplit[$pos]);
                return false;
            }
            return true;
        });
        if (count(array_diff($strSplit,$wordSplit)) === 0) {
            //echo "$word contains all letters of $letters | Step $step
" . PHP_EOL;
            $result[] = $word;
        }
    }

    return $result;
}

Example usage:

$dictionary = ['apple', 'sample', 'api', 'pia', 'тоҷик'];
getWordsWithSpecifiedLetters($dictionary, "ple");

This returns the words sample and apple.

I have more 90000 words in my dictionary (UTF-8). This results in a very slow program; if I'm trying to find something from the full dictionary it may take tens of thousands of loops. How can I improve the performance of these functions?

You can download my dictionary from here and testing your code using dictionary words.

How about removing the dictionary preparation each time at a cost of increasing your dictionary width?
You could have an alphabetized lookup column (the rows aren't alphabetized -- each letter of each word is sorted alphabetically) and a word column:
lookup  |  word
-----------------
aelpp   |  apple
aelmps  |  sample
aip     |  api
aip     |  pia
икотҷ   |  тоҷик

Using your lowercase, alphabetized $needle, when you want to find "whole" matches, you merely search the lookup column with the = operator.
SELECT `word` FROM `dictionary` WHERE `lookup` = 'икотҷ'

When you want to match the $needle characters at a minimum, you call:
SELECT `word` FROM `dictionary` WHERE `lookup` REGEXP '.*и.*к.*о.*т.*ҷ.*'

Leveraging something like this technique: Custom REGEXP Function to be used in a SQLITE SELECT Statement with this intended usage: ~.*и.*к.*о.*т.*ҷ.*~u
This, of course, is just a theoretical suggestion -- I haven't tried to do anything like this before.
And definitely remember to sanitize and escape the $needle to be offered to the query for security reasons.
Mostly I am suggesting that you sacrifice memory for speed.  Only the $needle should be modified with character sorting and strtolower actions.  These processes are expected to be "already done" on words prior to being stored in the dictionary.
Here is another post of mine with the same basic logic: How to best compare these two strings for values even though they are in random order?

If altering the dictionary table structure is unattractive, this is how I would recommend searching for exact character matches in any order:
Code:
function getWordsContainingTheExactSpecifedLetters_inanyorder_nomore_noless(array $dictionary, string $letters, string $encoding = 'UTF-8'){
    $lettersLength = mb_strlen($letters, $encoding);  // call just once and cache
    $lettersSplit = preg_split('//u', mb_strtolower($letters, $encoding), null, PREG_SPLIT_NO_EMPTY);
    sort($lettersSplit);

    $result = [];
    foreach ($dictionary as $word) {
      if(mb_strlen($word, $encoding) == $lettersLength) {
        $wordSplit = preg_split('//u', mb_strtolower($word, $encoding), null, PREG_SPLIT_NO_EMPTY);
        sort($wordSplit);
        if ($wordSplit === $lettersSplit) {
          $result[] = $word;
        }
      }
    }
    return $result;
}

Of course, you will need to change the qualifying condition if you wish to retain larger words that merely contain the letters.


Optimizing solution of Sum of Pairs: Codewars in Python I need help to optimize my code for huge lists with approx. 10000000 elements. Is there a way to improve my algorithm or should I try to build a new one using a completely different approach?
Task: given a list of integers and a single sum value, return the first two values (parse from the left please) in order of appearance that add up to form the sum.
def sum_pairs(ints, s):
    lst1 = []
    for n in ints:
        if n in lst1:
            return [s-n, n]
        lst1.append(s-n)

lst3 = list(range(1,10000000))

sum_pairs(lst3, 20000000)

*Times out :(

#Using set() every time we add new item to the list doesn't help

def sum_pairs(ints, s):
    lst1 = []
    for n in ints:
        lst2 = set(lst1)
        if n in lst2:
            return[s-n, n]
        lst1.append(s-n)

You could use a set as lst1 instead of a list. Every time you check if n is in lst1, it’s \$ O(n) \$ time complexity. Using a set instead will lower that to \$ O(1) \$.


Live chat for website visitors I am trying to implement a live chat for my websites visitors. I am currently using a PHP script that processes in background and that is called by AJAX. I am not familiar with sockets and node.js that also allow to create a live chat so I tried to make one longpolling-like code upon my possibilities. What I am hoping for is to hear your ideas about this code, how I can improve it and what are the pros and cons for using my code.

PHP script (used for getting all the latest messages):

require ($_SERVER['DOCUMENT_ROOT'].'/includes/config.php');
$time = $_POST['time'];
$x = 0;
$allowloop = true;
while (($x < 99) && ($allowloop))
{
    if ($time == 0)
    {
        $query = mysqli_query($con, 'SELECT * FROM (SELECT * FROM chat_messages WHERE send_time > "'.$time.'" ORDER BY chat_message_id DESC LIMIT 30) T1 ORDER BY chat_message_id ASC');
    } else {
        $query = mysqli_query($con, 'SELECT * FROM chat_messages WHERE send_time > "'.$time.'"');
    }

    if ((!$query) && ($time == 0))
    {
        $allowloop = false;
    } elseif (mysqli_num_rows($query) == 0) {
        usleep(200000);
        $x = $x + 1;
    } else {
        $allowloop = false;
    }
}

if (!$allowloop)
{
    while ($messages = mysqli_fetch_assoc($query))
    {
        echo
    '

'.$messages['chat_message_text'].'

                    '.date('G:i:s', $messages['send_time']).'
                    '.$messages['send_time'].'

    ';
    }
}
}

JS function that calls the code above in the background:

$('#world_chat_input').focus();
var time = 0;
var data = '';
(function chat_get() {
    time = $('table#w_messages_t tr:last td#time').html();
    data = {time: time};
    $.ajax({
        type: 'POST',
        url: '/guild/character/guild/home/chat_get.php',
        data: data,
        async: true,
        success: function (data) {
            $('#w_messages_t tbody').append(data);
            if (data) {
                $('#w_m_table_wrap').animate({
                    scrollTop: $('#w_m_table_wrap').prop('scrollHeight')
                }, 1000);
            }
            setTimeout(chat_get);
        }
    });
})();

HTML chat table:

            Welcome to World chat	0

$allowloop = true;
while (($x < 99) && ($allowloop))
{
// More code was here. //
    if ((!$query) && ($time == 0))
    {
        $allowloop = false;
    } elseif (mysqli_num_rows($query) == 0) {
        usleep(200000);
        $x = $x + 1;
    } else {
        $allowloop = false;
    }
}

if (!$allowloop)
{

You don't need $allowloop.  You can replace these lines with

while ( $x < 99 ) {
// The code that was here stays the same. //
    if ( ! $query && ( 0 == $time ) ) {
        break;
    } elseif ( ! $query || ( 0 == $query->num_rows ) ) {
        usleep(200000);
        ++$x;
    } else {
        break;
    }
}

if ( $x < 99 ) {

I changed $x = $x + 1 to the more standard ++$x which does the same thing.

I also optimized your elseif so that it won't call mysqli_num_rows unnecessarily.  And changed it so that it uses the shorter object-oriented notation.

This hits the database every .2 seconds for almost twenty seconds.  Are you really in that much of a hurry?  Also, be careful of timeouts.  If your HTTP connection times out after five seconds, there's not much point in continuing to poll the database.

Note that if you have a lot of people chatting but no one saying anything, there will be a bunch of people doing this at once.  You may want to look into solutions that allow for sharing these results among multiple HTTP connections.  For example, something that caches the last two message times and returns a cached result for times between them (older results would be returned from the database).  APC would work for a single server or a shared memcache for multiple servers.

    $query = mysqli_query($con, 'SELECT * FROM chat_messages WHERE send_time > "'.$time.'"');

There's a security issue here.  Assuming $time is an integer, you should say,

    $query = mysqli_query($con, 'SELECT * FROM chat_messages WHERE send_time > "'.(int)$time.'"');

This avoids the possibility of SQL injection by someone who spoofs your AJAX call.

Or use mysqli_prepare (preferred).

    $statement = $con->stmt_init();
    if ($statement->prepare('SELECT * FROM chat_messages WHERE send_time > ?') ) {
        $statement->bind_param('i', $time);

Be careful about using SELECT *.  It's one thing if the three columns that you use are the only columns.  It's another thing entirely if you have a large text column that you don't need in the table.  There are some people who believe that you should never use * and always specify the columns.

Note that using * may also cause you to go to disk when you otherwise would have a covering index.  This means that even a small column can be a problem.

I don't like the name $con for the database connection.  It's too short to be obvious what it is.  Admittedly, $con is slightly more accurate than $db, but $db is accurate enough, reasonably standard, and more easily recognizable in my opinion.

You may want to replace send_time with chat_message_id as your check.  I.e. instead of checking the time, check for newer chat_message_id values.  Assuming this is an autoincrement primary key, you should already have a unique index on it.  You could put an index on send_time, but it will be a slightly less performant non-unique index.

This also avoids the problem with near simultaneous chat messages where you only have the first one and never look for the second because it has the same time.

if ($time == 0)
{
    $query = mysqli_query($con, 'SELECT * FROM (SELECT * FROM chat_messages WHERE send_time > "'.$time.'" ORDER BY chat_message_id DESC LIMIT 30) T1 ORDER BY chat_message_id ASC');

This seems more complicated than it needs to be.  We know that $time == 0, so why use $time in the query?  Are there negative times in the table?  Why not just

if ( 0 == $time ) {
    $query = $con->query('SELECT * FROM (SELECT * FROM chat_messages ORDER BY chat_message_id DESC LIMIT 30) T1 ORDER BY chat_message_id ASC');

Also switched to the shorter object-oriented notation.

Switched the order of the comparison to 0 == $time because this will throw a compile error if you leave off an = where $time = 0 will silently change the value of $time and evaluate as false.  Consistently using this order will therefore make it easier to catch that particular typo.

You switch between having the { on the same line and on a new line.  Either is a valid style, but please pick one and stick with it.  My preference is for on the same line, but both are common.

Scaling

My primary concern with this code is that it won't scale well.  You have every user in the chat polling to see if there is new data.  For ten users, this will probably work fine.  For a thousand users, this seems like it would put considerable load on the database.  Not so much from the queries as maintaining a thousand simultaneous database connections for twenty seconds.

As a general rule, a callback is better than polling.  You can generate the callback only when required whereas polling has to continually check.  PHP is probably the wrong language for implementing a callback model though.  You could add an additional layer between PHP and the database to fix this.  PHP would call your service, which would then only call the database once (or not at all if the service also manages sending a chat message) rather than once per client.

In that model, one database connection is sufficient regardless of the number of clients.  You'd only need multiple database connections if there were multiple chat channels, and even then they could share.  Some languages (e.g. Java) have connection pooling so as to share database resources transparently to the client.  PHP seems the wrong language to manage something like this, as it is optimized for HTTP request/reply connections.  PHP's not persistent, while this problem space is.

This will probably work for a small scale application, but if it grows, you'll likely have to dump the current method and swap it out for something more robust.


Suit class for card game I'm trying to make a card game, using OOP style. I decided to make suit a class rather than an enum as I would first have done. How am I doing so far? Am I over-complicating things by making it a class?

namespace Cards
{
class Suit
{
public:
    Suit();
    ~Suit();

    std::string GetName(bool plural, bool capital) const;
    bool IsRed() const;
    bool IsBlack() const;
    bool IsDifferentColour(const Suit& other) const;
    bool IsSameColour(const Suit& other) const;
    bool operator == (const Suit& other) const;
    bool operator != (const Suit& other) const;
    bool operator < (const Suit& other) const;

    static const Suit hearts, diamonds, clubs, spades;

private:
    enum Type
    {
        Type_Null,
        Type_Hearts,
        Type_Diamonds,
        Type_Clubs,
        Type_Spades
    } type;

    explicit Suit(Type type);
};
}

const Cards::Suit Cards::Suit::hearts(Type_Hearts);
const Cards::Suit Cards::Suit::diamonds(Type_Diamonds);
const Cards::Suit Cards::Suit::clubs(Type_Clubs);
const Cards::Suit Cards::Suit::spades(Type_Spades);

Cards::Suit::Suit()
    : type(Type_Null)
{}

Cards::Suit::Suit(Type type)
    : type(type)
{}

Cards::Suit::~Suit()
{}

std::string Cards::Suit::GetName(bool plural, bool capital) const
{
    std::string name;

    switch (type)
    {
    case Type_Hearts:
        {
             name = "Heart";
        }
        break;

    case Type_Diamonds:
        {
            name = "Diamond";
        }
        break;

    case Type_Clubs:
        {
            name = "Club";
        }
        break;

    case Type_Spades:
        {
            name = "Spade";
        }
        break;

    default:
        {
            assert(false);
        }
        break;
    }

    if (! capital)
    {
        name[0] = static_cast<std::string::value_type>(::tolower(name[0]));
    }

    if (plural)
    {
        name += "s";
    }

    return name;
}

bool Cards::Suit::IsRed() const
{
    bool red;

    switch (type)
    {
        case Type_Hearts:
        {
             red = true;
        }
        break;

    case Type_Diamonds:
        {
            red = true;
        }
        break;

    case Type_Clubs:
        {
            red = false;
        }
        break;

    case Type_Spades:
        {
            red = false;
        }
        break;

    default:
        {
            assert(false);
            // prevent compiler warning
            red = false;
        }
    }

    return red;
}

bool Cards::Suit::IsBlack() const
{
    return ! IsRed();
}

bool Cards::Suit::IsDifferentColour(const Suit& other) const
{
    return this->IsRed() != other.IsRed();
}

bool Cards::Suit::IsSameColour(const Suit& other) const
{
    return this->IsRed() == other.IsRed();
}

bool Cards::Suit::operator == (const Cards::Suit& other) const
{
    return this->type == other.type;
}

bool Cards::Suit::operator != (const Cards::Suit& other) const
{
    return ! (*this == other);
}

bool Cards::Suit::operator < (const Cards::Suit& other) const
{
    return this->type < other.type;
}

If the destructor does nothing remove it:

Cards::Suit::~Suit()
{}

We can shorten that switch statement to make it readable.

    switch (type)
    {
        case Type_Hearts:   name = "Heart";  break;
        case Type_Diamonds: name = "Diamond";break;
        case Type_Clubs:    name = "Club";   break;
        case Type_Spades:   name = "Spade";  break;
        default:            assert(false);   break;
    }

Or we can replace it with a look up:

    static std::string  names[] = {"NULL", "Heart", "Diamond", "Club", "Spade"};
    if (type == Type_Null)
    {    assert(false); // Or throw.
    }
    name =  names[type];

If you want more checking:

    static std::vector<std::string>  names = {"Heart", "Diamond", "Club", "Spade"};
    name =  names.at(type-1); // will throw on bad value.

This seems a bit over keen.

        name[0] = static_cast<std::string::value_type>(::tolower(name[0]));

std::string::value_type by definition is char.
tolower() may return an int unless you pass in an EOF you will get a character back.

I would have just done:

        name[0] = ::tolower(name[0]);

In this switch use fall throug:

switch (type)
{
    case Type_Hearts:    /* fall through */
    case Type_Diamonds:  return true;

    case Type_Clubs:     /* fall through */
    case Type_Spades:    return false;
    default:  assert(false);
        // prevent compiler warning
        return true;
}

Is A Null_Suit the same as another Null_Suit?
in double a NaN is not equal to another NaN you may want that kind of functionality.

bool Cards::Suit::operator == (const Cards::Suit& other) const
{
    return this->type == other.type;
}


LeetCode: Group Anagrams C# LeetCode: Group Anagrams C#
Given an array of strings, group anagrams together.

Example:

Input: ["eat", "tea", "tan", "ate", "nat", "bat"],
Output:
[
  ["ate","eat","tea"],
  ["nat","tan"],
  ["bat"]
]

Note:

All inputs will be in lowercase.
The order of your output does not matter.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace StringQuestions
{
    /// <summary>
    /// https://leetcode.com/explore/interview/card/top-interview-questions-medium/103/array-and-strings/778/
    /// </summary>
    [TestClass]
    public class GroupAnagramsTest
    {
        [TestMethod]
        public void TestMethod1()
        {
            string[] strs = {"eat", "tea", "tan", "ate", "nat", "bat"};
            var result = GroupAnagramsClass.GroupAnagrams(strs);
            List<IList<string>> expected = new List<IList<string>>();
            expected.Add( new List<string> {"eat", "tea", "ate"});
            expected.Add( new List<string> {"tan", "nat"});
            expected.Add( new List<string> {"bat"});
            for (int i = 0; i < result.Count; i++)
            {
                CollectionAssert.AreEqual(expected[i].ToList(), result[i].ToList());
            }
        }
    }

    public class GroupAnagramsClass
    {
        public static IList<IList<string>> GroupAnagrams(string[] strs)
        {
            Dictionary<char[], List<string>> dict = new Dictionary<char[], List<string>>(new CharComparer());
            foreach (var str in strs)
            {
                var key = str.ToCharArray();
                Array.Sort(key);
                if (!dict.TryGetValue(key, out var temp))
                {
                    temp = dict[key] = new List<string>();
                }
                temp.Add(str);
            }

            List<IList<string>> res = new List<IList<string>>(dict.Values.ToList());
            return res;
        }
    }

    public class CharComparer : IEqualityComparer<char[]>
    {
        public bool Equals(char[] x, char[] y)
        {
            if (x == null || y == null)
            {
                return false;
            }

            if (x.Length != y.Length)
            {
                return false;
            }

            for (int i = 0; i < x.Length; i++)
            {
                if (x[i] != y[i])
                {
                    return false;
                }
            }

            return true;
        }

        public int GetHashCode(char[] obj)
        {
            return 0;
        }
    }
}

Please review for performance. I don't like me copying the dictionary into IList<IList<string>. Is there a better way to do so, considering this the desired API defined by the question?

Deriving from IEqualityComparer versus EqualityComparer.

The MSDN docs say the following:

  We recommend that you derive from the EqualityComparer class instead of implementing the IEqualityComparer interface, because the EqualityComparer class tests for equality using the IEquatable.Equals method instead of the Object.Equals method. This is consistent with the Contains, IndexOf, LastIndexOf, and Remove methods of the Dictionary class and other generic collections.
  MSDN docs: IEqualityComparer

Dictionary versus Lookup

For grouping together objects from a sequence, where there might be a varying amount of items in each group, a Lookup is better than a Dictionary:

 var lookup = strs.ToLookup(key =>
 {
     var array = key.ToCharArray();
     Array.Sort(array);
     return array;
 }, new CharComparer());
 return lookup.Select(grouping => (IList<string>)grouping.ToList()).ToList();

Comparing char-arrays

Since we're using Linq, let's use Linq:

public override bool Equals(char[] x, char[] y)
{
    if (x == null || y == null)
    {
        return false;
    }

    if (x.Length != y.Length)
    {
        return false;
    }

    return x.SequenceEqual(y);
}

The null behaviour is different and the length shortcut may stay, but the last loop we can offload to Linq.

Hashcode

Both Dictionary and Lookup rely on the hashcode returned by the equalitycomparer to categorise the keys into bins. These bins is what allows these collections to get times in \$O(1)\$. Always returning the same value is going to cause problems when your inputs get bigger. It will effectively turn the collections into single arrays which have to be looped over to get to the correct key.

Creating good hashcodes is hard though, and I don't really know a good rule of thumb for creating them.

char[] versus string

All in all, the hashcode and equalitycomparer is such a headache, it's probably easier, and more readable to convert the sorted char[] back into a string, and use that as the key for the lookup:

 var lookup = strs.ToLookup(key =>
 {
     var array = key.ToCharArray();
     Array.Sort(array);
     return new string(array);
 });
 return lookup.Select(grouping => (IList<string>)grouping.ToList()).ToList();


Composing terminal control sequences I've seen everybody claims the nested ternary to be an evil even when an operation couldn't be written with switch-case; and here's even more, my code could be easily written with switch-case statement.

/// <summary>
/// Control sequence character
/// </summary>
private readonly string CSI = "\u001b";

public string ComposeTermSeqs1(TermActions action)
{
    switch(action) {
        case TermActions.Right:
            return CSI + "[C";
        case TermActions.Up:
            return CSI + "[A";
        case TermActions.Left:
            return CSI + "[D";
        case TermActions.Down:
            return CSI + "[B";
        case TermActions.ForwardWord: //readline lib
            return CSI + "f";
        case TermActions.BackWord: //readline lib
            return CSI + "b";
        case TermActions.BackSearch:
            return "\u0012";
        case TermActions.Interrupt:
            return "\u0003";
        default: //TermActions.SuspendApp:
            return "\u001a";
    }
}

But when I am looking at the ternary version of the function…

public string ComposeTermSeqs2(TermActions action)
{
    return (action == TermActions.Right)      ? CSI + "[C"
        : (action == TermActions.Up)          ? CSI + "[A"
        : (action == TermActions.Left)        ? CSI + "[D"
        : (action == TermActions.Down)        ? CSI + "[B"
        : (action == TermActions.ForwardWord) ? CSI + "f" //readline lib
        : (action == TermActions.BackWord)    ? CSI + "b" //readline lib
        : (action == TermActions.BackSearch)  ? "\u0012"
        : (action == TermActions.Interrupt)   ? "\u0003"
        : "\u001a";
}

I don't know, the switch-case is α) longer β) is harder to maintain because you ought to write every time one case in the «default», and γ) the ternary version is pure, i.e. here's no need to tackle with debugging of this function when something gone wrong, only with the result. You might say that the first version is pure too, but actually someone could easily add a side code to a case.

I am hesitating… Which one should I choose?

I prefer the switch statement.  Here are my reasons:

You can move each return statement onto the same line as the case statement to make the switch statement just as short as the ternary version.
The ternary version is harder to read (at least for me) due to the more ? : == and () involved.  This is not a big deal though.
I find it easier to make subtle syntax errors (not compile time errors) with the ternary operator.  For example:

x = y & mask ? 8 : 4;
a = 5 + (b > c) ? b : c;

Of course you can avoid this by using the correct parentheses:

x = (y & mask) ? 8 : 4;
a = 5 + ((b > c) ? b : c);

It's just that sometimes these kind of expressions appear across multiple lines and it gets hard to visualize which part is which.
A compiler might be able to do a better job optimizing a switch statement than the ternary if-else ladder.  For example, if your switch cases are in consecutive order (such as 0..N), then the compiler can use a jump table to simplify the switch into:

// (pseudocode)
offset = jumpTable[index];
goto offset;


Using a webservice with blocks and returning data via callback I load some data from from my webservice via a method using blocks and callbacks.

From my ViewController I make a request to the data loading class and pass the callback block:

- (void)loadRadioStationList
{
    [self.radioDataLoader getRadioStationList:^(NSArray *data, NSDictionary *dictData){

        self.alphabetizedDictionary = [[NSDictionary alloc] initWithDictionary:dictData];
        self.sectionIndexTitles = [[NSArray alloc] initWithArray:data];

        [self.radioStationTableView reloadData];
    }];
}

Then in the dataloading class the method is as so:

-(void) getRadioStationList:(void (^)(NSArray *data, NSDictionary *dictData))callback
{

    NSString *string = RadioStationListURL;
    NSURL *url = [NSURL URLWithString:string];
    NSURLRequest *request = [NSURLRequest requestWithURL:url];

    AFHTTPRequestOperation *operation = [[AFHTTPRequestOperation alloc] initWithRequest:request];
    operation.responseSerializer = [AFXMLParserResponseSerializer serializer];

    [operation setCompletionBlockWithSuccess:^(AFHTTPRequestOperation *operation, id responseObject) {

        NSXMLParser *XMLParser = (NSXMLParser *)responseObject;
        [XMLParser setShouldProcessNamespaces:YES];
        XMLParser.delegate = self;
        [XMLParser parse];

         callback((NSArray*)self.sectionIndexTitles, (NSDictionary*)self.alphabetizedDictionary);

    } failure:^(AFHTTPRequestOperation *operation, NSError *error) {

        UIAlertView *alertView = [[UIAlertView alloc] initWithTitle:@"Error Retrieving Radio List"
                                                            message:[error localizedDescription]
                                                           delegate:nil
                                                  cancelButtonTitle:@"Ok"
                                                  otherButtonTitles:nil];
        [alertView show];

    }];

    [operation start];

}

It loads the XML file and parses out the data and send the data back to my VC via the callback block.

I've a couple of questions:

Is this an acceptable way of doing this? Would you prefer to see it done another (better) way?
If I was to write an API, would passing the data from the API back to the calling class be a good way to work it, or is there a more suitable method for an API?

Here is a way how I do it:

Create a class and name it like ClientApi. This class should be a singleton link and be absolutely independent. Also, create a class APIRouter which will return a path for a request. I'm using the AFNetworking framework for communication with different API etc. and JSONModel framework for mapping. Here you can find the full answer about network architecture in iOS apps.

@interface SFAPIClient ()

@property (nonatomic, strong) AFHTTPSessionManager *manager;

@end

@implementation SFAPIClient

+ (instancetype)sharedInstance
{
    static id _sharedInstance = nil;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        _sharedInstance = [[[self class] alloc] init];
    });

    return _sharedInstance;
}

If a request should return some data, pass block as a parameter to the method:

- (void)getAllChildsWithCompletion:(void(^)(NSArray *arr, NSError *error))completion
{
    NSString *url = [SFAPIRouter childRoute];
    [self setupRequestSerializer];

    [self.manager GET:url parameters:nil progress:nil success:^(NSURLSessionDataTask * _Nonnull task, id  _Nullable responseObject) {
        NSError *error = nil;
        NSArray *arr = //map respons

        if (!error)//if everything good call block with data
            completion(arr, nil);
        else
            completion(nil, error);

    } failure:^(NSURLSessionDataTask * _Nullable task, NSError * _Nonnull error)
     {
        if (completion)
            completion(nil, error);
    }];
}

APIRouter:

static NSString * const kSFServerURL = @"http://0.0.0.0/api/v1";

@implementation SFAPIRouter

+ (NSString *)saveMediaToListRoute
{
    return [kSFServerURL stringByAppendingString:@"/childs"];
}

And then in controller call function from your ClientApi class:

//show activity indicator
[[SFAPIClient sharedInstance] getAllChildsWithCompletion:^(NSArray *arr, NSError *error){
//stop showing activity indicator

if (error)
{
//show alert
return;
}
//do what you need
}];


Perl one-liner for parsing host names from ssh config file Here is the working code (intended to be executed with perl -e '<code>' ~/.ssh/config):

Actual version:

while(<>){if(/^Host (.+)/){$_=$1;foreach $i(/([^ ]+)/g){$h{$i}=1}}}print "$_ " for keys %h;

Identical version with whitespace for reading:

while(<>) {
    if(/^Host (.+)/) {
        $_=$1;
        foreach $i(/([^ ]+)/g) {
            $h{$i}=1
        }
    }
}
print "$_ " for keys %h;

I'm sort of a perl newbie.  Is $_=$1 kosher?  Is there a straight-forward way to do this with a single regex?  Is there anything else I should be doing better here?

Example input output:

For a config file with contents like this:

Host build
    HostName build.myserver.com
Host build build.myserver.com
    User ubuntu
    IdentityFile ~/.ssh/build
Host tunnel
    LocalForward 6397 redis.production.com:6379

The output should be (order is not required, trailing space is not required but acceptable):

build build.myserver.com tunnel

I would use something like

perl -lane '@h{@F[1..$#F]}=()if/^Host\b/;END{$,=" ";print keys %h}' -- file

or

perl -lane '@h{ @F[ 1 .. $#F ] } = () if /^Host\b/;
            END {
                $, = " ";
                print keys %h;
            }' -- file

-l removes newlines from input and adds them to prints
-n runs the code for each line of the input
-a splits each line on whitespace into the @F array
the hash %h is populated by all the non-whitepsace strings following Host
at the end, all the keys are printed separated by space


Sane ranges for DB queries I have a function that has, as input, a ranges object which may define from, to and limit. Note that it might have one, two or all three of them.

I want the output to be a saneRanges object, where if partial values are provided, the other ones are inferred.

For example if only limit is provided, from should be 0 and to should be limit-1.

This is the code I wrote during prototyping. I made sure I took zero shortcuts, as I didn't want smarty pants code make debugging harder.

But now... here I am. I have this lame code, and I want to find the bestest way to write it.

How would you rewrite this, so that it doesn't look quite so terrible?

  sanitizeRanges: function( ranges, skipHardLimitOnQueries ){
    var self = this

    var saneRanges = {};
    if( ! self.hardLimitOnQueries ) var hardLimitOnQueries = 0;

    saneRanges.from = 0;
    saneRanges.to = 0;
    saneRanges.limit = 0;

    if( typeof( ranges ) === 'object' && ranges !== null ){

      // Copy values over to saneRanges
      saneRanges.from = ranges.from || -1;
      saneRanges.to = ranges.to || -1;
      saneRanges.limit = ranges.limit || -1;

      var sr = saneRanges;

      // Sorry, no shortcuts here for now. Code will be optimised later
      // (maybe)

      // Case: Nothing is set
      if( sr.from === -1 && sr.to === -1 && sr.limit === -1 ){
         sr.from = 0;
         sr.to = self.hardLimitOnQueries;
         sr.limit = self.hardLimitOnQueries;

      // Case: Only "limit" is set
      // - Set "from" and "to"
      } else if( sr.from === -1 && sr.to === -1 && sr.limit !== -1 ){
        sr.from = 0;
        sr.to = sr.limit - 1;

      // Case: Only "from" is set
      // - Set "to" and "limit"
      } else if( sr.from !== -1 && sr.to === -1 && sr.limit === -1 ){
        sr.limit =  0;
        sr.to  = 0;

      // Case: Only "to" is set
      // - Set "from" and "limit"
      } else if( sr.from === -1 && sr.to !== -1 && sr.limit === -1 ){
        sr.from = 0;
        sr.limit =  sr.to + 1;

      // Case: Only "from" and "limit" are set
      // - Set "to"
      } else if( sr.from !== -1 && sr.to === -1 && sr.limit !== -1 ){
        sr.to =  sr.from + sr.limit - 1;

      // Case: Only "from" and "to" are set
      // - Set "limit"
      } else if( sr.from !== -1 && sr.to !== -1 && sr.limit === -1 ){
        sr.limit =  sr.to - sr.from + 1;

      // Case: Only "to" and "limit" are set
      // - Set "from"
      } else if( sr.from === -1 && sr.to !== -1 && sr.limit !== -1 ){
        sr.from = 0;
      }

      // Make sure "limit" never goes over
      if(  sr.limit != 0 && sr.from + sr.limit - 1 > sr.to ){
        sr.limit =  sr.to - sr.from + 1;
      }

    }

    // Apply hard limit on queries if required to do so. Driver implementations
    // should only pass 'true' for non-cursor queries, to prevent huge toArray() on
    // a million records
    if( ! skipHardLimitOnQueries ){
      if( self.hardLimitOnQueries && ( saneRanges.limit === 0 || sr.limit > self.hardLimitOnQueries ) ){
        saneRanges.limit = self.hardLimitOnQueries;
      }
    }

    return saneRanges;

  },

The code isn't especially bad and it seems to cover all bases but I agree it's not nice to look at. Perhaps some architectural observations may assist you in making it cleaner.

Firstly, I would not recommend using - 1 as a special value to indicate that a property is set or not. In this particular context it places an additional burden on you as the developer to remember what it means. Simply rely on the fact that a property is defined or it isn't.

A second observation is that I shouldn't worry about type checking unless you permit different "kinds" of range objects. If you've published your API that states a range object should be passed to this function, then have confidence that it will be used that way. If you must have it, consider inverting it so you don't have to nest the chain of if statements inside it.

A pattern I have seen regularly in this kind of code is to merge the user object with a default one, and then ask the question "does this make sense? ". With that said, be careful with modifying what has been passed to the function, unless behaviour is clearly published somewhere. What can happen is the user of this function will create a dodgy range object that gets corrected by your function and used in the DB query. The key part of this story is the user didn't know what they did was wrong and will carry on regardless. If it's wrong, give an error or let the query take ages and timeout, they'll have to learn some time!


How to capture disk folder structure? What would be the shortest way to capture folder structure in code? I would like to have a more or less reusable strictly typed approach to use in scripts instead of stringly typed paths we all usually deal with. Here is how a part of my simple “system info” utility looks like:

static void Main(string[] args)
{
    foreach(var cursor in SystemDisk.Windows.Cursors.Animated)
        Console.WriteLine(cursor);
}

Where:

using static System.IO.Directory;

public static class SystemDisk
{
    public static string Drive { get; set; } = "C:";
    public static string AutoexecBat => $"{Drive}\\Autoexec.bat";
    public static class Windows
    {
        public static string Path => $"{Drive}\\Windows";
        public static string SystemIni => $"{Path}\\System.ini";
        public static class Cursors
        {
            public static string Path => $"{Windows.Path}\\Cursors";
            public static IReadOnlyList<string> Animated => GetFiles(Path, "*.ani");
        }
    }
}

I've got the idea you're trying to achieve, however, this would be too much work to do if you're going to implement it this way.

If you are still convinced that this is the only way, you can use struct and define the OS structure like you did, and keep it only for path something like this :

public struct SystemDisk
{
    private static string Drive => Path.GetPathRoot(Environment.SystemDirectory);

    //using Refelction, will give you the object's full name, we then convert it to string and adjust it to be parsed as an OS path.
    private static string GetPath(Type type) => @Drive + @type.FullName.Substring(type.FullName.IndexOf('.') + "SystemDisk".Length + 2).Replace('+', Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar;

    public struct Windows
    {
        public static string Root => $@"{GetPath(typeof(Windows))}";

        public static string SystemIni => $@"{Root}System.ini";

        public struct System
        {
            public static string Root => $@"{GetPath(typeof(System))}";
        }

        public struct System32
        {
            public static string Root => $@"{GetPath(typeof(System32))}";
        }

        public struct Cursors
        {
            public static string Root => $@"{GetPath(typeof(Cursors))}";
            public static IReadOnlyList<string> Animated => GetFiles(Root, "*.ani");
        }
    }
}

if you're going to continue on this road, and you need to create a struct for each directory in the system folders, you can write a method to get the directories, and then design a model for it, then returning this model as string, then just copy and paste. To do that, you'll have to get first the directories, then get each directory name, after that, you'll remove any special chars from the name, then just make a  model template and pass the directory name to it, get the representative string, and copy / paste into the class.

Here is method to camel case the name :

public static string CamelCase(string str)
{
    str = str.Replace(".", " ").Replace("-", " ").Replace("_", " ");

    return str.Contains(" ")
        ? string.Join("", str.Split(' ').Select(x => char.ToUpper(x[0]) + x.Substring(1)))
        : char.ToUpper(str[0]) + str.Substring(1);
}

now we can do a method to build the model template :

public static string StructModelBuilder(string name, int indentlevel, string body = "")
{
    var sb = new StringBuilder(string.Empty);

    string indentLv1 = indentlevel == 0 ? string.Empty : new string(' ', indentlevel);

    string indentLv2 = indentLv1 == string.Empty ? new string(' ', 4) : new string(' ', indentlevel * 2);

    sb.Append(Environment.NewLine);
    sb.Append(indentLv1);
    sb.Append($"public struct {name}");
    sb.Append(Environment.NewLine);
    sb.Append(indentLv1);
    sb.Append("{");
    sb.Append(Environment.NewLine);
    sb.Append(indentLv2);
    sb.Append($"public static string Root => $@\"{{GetPath(typeof({name}))}}\";");
    sb.Append(Environment.NewLine);
    sb.Append(body);
    sb.Append(indentLv1);
    sb.Append("}");
    sb.Append(Environment.NewLine);

    return sb.ToString();
}

now, we can create a method where we get the directories list, and just pass them to the template to get the model

public static string GetStructureModel(string path)
{
    if (string.IsNullOrEmpty(path))
        return string.Empty;

    //Directory.GetDirectories($@"{path}", "*.*", SearchOption.AllDirectories);  // to include all sub-directories (recursively).
    var root_dir = Directory.GetDirectories($@"{path}");// just get the top level directories.

    // remove the last Directory Sparator Charcter (e.g. back-slash \) and get the name of the directory
    var root_name = path.TrimEnd('\\').Substring(path.LastIndexOf(System.IO.Path.DirectorySeparatorChar) + 1);

    var rootNameCamelCase = CamelCase(path.TrimEnd('\\'));

    var sb = new StringBuilder(string.Empty);

    var model = StructModelBuilder(rootNameCamelCase, 0, "@body");

    foreach (var dir in root_dir)
    {
        var dir_name = path.TrimEnd('\\').Substring(path.LastIndexOf(System.IO.Path.DirectorySeparatorChar) + 1);

        var dirNameCamelCase = CamelCase(dir_name);

        sb.Append(StructModelBuilder(dirNameCamelCase, 4));
    }

    return model.Replace("@body", sb.ToString());
}

now you just pass the path like this :

var model = GetStructureModel(@"C:\Windows");

you can from here take the model, paste it into the class, and adjust whatever needed. this is not perfect, but it should get the job done.

personally, I wouldn't go this far if I were in your seat, instead, I'll use Window Environment Variables instead, to not re-invent the wheels. also, I'll make use of Enum and make things shorter. For instance, instead of making a full path to the System32, I'll just use an Enum and define System32, and with a simple switch statement, it'll return the full path, and so on. this would make more sense to me and would be easier to maintain.


Python functions to fetch spreadsheets through SFTP and from Dropbox I have written two functions- first one is to read files from SFTP and second one is to read files from Dropbox.

Both functions have some similar line of code like validating extension and reading/saving file that can be extracted into a separated function and thus need your suggestion how to do it.

PFB my both functions-

class SftpHelper:
    def fetch_file_from_sftp(self, file_name, sheet_name=0):
      valid_extensions = ['csv', 'xls', 'xlsx']
      extension = file_name.split('.')[-1]
      sftp, transport = self.connect_to_sftp()
      remote_path = self.remote_dir + file_name
      data = io.BytesIO()
      sftp.getfo(remote_path, data, callback=None)
      if extension == 'csv':
          file_df = pd.read_csv(io.BytesIO(data.getvalue()))
      else:
          file_df = pd.read_excel(io.BytesIO(data.getvalue()), sheet_name=sheet_name)
      self.close_sftp_connection(sftp, transport)
      return file_df

class DropBoxHelper:
    def read_file_from_dropbox(self, file_name, sheet_name=0):
      valid_extensions = ['csv', 'xls', 'xlsx']
      extension = file_name.split('.')[-1]
      dbx = self.connect_to_dropbox()
      metadata,data=dbx.files_download(file_name)
      if extension == 'csv':
          file_df = pd.read_csv(io.BytesIO(data.content))
      else:
          file_df = pd.read_excel((io.BytesIO(data.content)), sheet_name=sheet_name)
      return file_df

Can anyone please help me to extract the common logix to a seperate function and then use that one in my two functions?

From looking at the code, some things seem off:

valid_extensions is defined, but not used
connect_to_sftp(), self.remote_dir, io.BytesIO(), sftp.getfo(), pd, self.close_sftp_connection() and a bunch of other functions/fields are not defined

That being said, the core problem is addressed by creating a parent class which both your classes can inherit from. It'd look something like this:

class FileHelper:
    def parse_fetched_file(self, file_name, data, sheet_name):
        valid_extensions = ['csv', 'xls', 'xlsx']
        extension = file_name.split('.')[-1]
        if extension == 'csv':
            return pd.read_csv(io.BytesIO(data.content))
        return pd.read_excel((io.BytesIO(data.content)), sheet_name=sheet_name)

class SftpHelper(FileHelper):
    def fetch_file_from_sftp(self, file_name, sheet_name = 0):
        sftp, transport = self.connect_to_sftp()
        remote_path = self.remote_dir + file_name
        data = io.BytesIO()
        sftp.getfo(remote_path, data, callback=None)
        file_df super(SftpHelper, self).parse_fetched_file(file_name, data, sheet_name)
        self.close_sftp_connection(sftp, transport)
        return file_df

class DropBoxHelper(FileHelper):
    def read_file_from_dropbox(self, file_name, sheet_name = 0):
        dbx = self.connect_to_dropbox()
        metadata, data = dbx.files_download(file_name)
        return super(DropBoxHelper, self).parse_fetched_file(file_name, data, sheet_name)

I'm not 100% sure that it's the most efficient syntax, but it gets the job done.


Python CSV file slicer I have made CSV file slicer. This is my first and biggest piece of code on Python. It takes one .csv file from current folder and then slices it to n parts and adds a first column, if provided.

import csv
import math
import os
import re
import sys

# Reading and returning files in current directory
def read_files():
    __location__ = os.path.realpath(
        os.path.join(os.getcwd(), os.path.dirname(__file__)))
    return [f for f in os.listdir(__location__) if os.path.isfile(f)]

# Removing files which match certain pattern
def remove_old_files():
    for f in read_files():
        if re.search("^[0-9]+\.csv$", str(f)) is not None:
            os.remove(f)

# Getting file to split in current directory
def get_file_to_split():
    for f in read_files():
        if re.search(".*\.csv$", str(f)) is not None:
            split(f, int(sys.argv[1]))

# Split file into n pieces
def split(csv_file, pieces):
    first_col = None
    if len(sys.argv) > 2:
        first_col = sys.argv[2]

    with open(csv_file, 'r') as c:
        reader = csv.reader(c)
        data = list(reader)

    cols_to_write = math.ceil(data.__len__() / pieces)
    chunks = [data[x:x + cols_to_write] for x in range(0, len(data), cols_to_write)]

    for num_file in range(pieces):
        filename = str(num_file) + ".csv"

        with open(filename, 'w') as f:
            w = csv.writer(f)
            for i in range(cols_to_write):
                try:
                    if first_col is not None and i == 0:
                        w.writerow([first_col])
                    w.writerow(chunks[num_file][i])
                except IndexError:
                    pass

    print("Done")

if __name__ == "__main__":
    if int(sys.argv[1]) <= 0:
        raise SystemExit("Piece count must be natural number greater than zero.")

    remove_old_files()
    get_file_to_split()

Here are some of the things I've noticed:

switching to argparse might make the argument parsing a bit more readable
the read_files() + remove_old_files() functions could make use of glob module with the **+recursive mode:

for filename in glob.iglob('./**/[0-9]+.csv', recursive=True):
    os.remove(filename)

avoid calling "magic" methods like __len__() when not necessary - you can use len() function directly
you can define first_col in one line:

first_col = sys.argv[2] if len(sys.argv) > 2 else None

c and f are not good variable names, think of something more descriptive - input_file and output_file?..
you can use an f-string to define the "filename" for a chunk
move the comments before the functions into proper docstrings

Also, what if you would slice the CSV in an iterative manner, something along these lines (other improvements applied):

import csv
import glob
import os
import sys
from itertools import islice

def remove_old_files():
    """Removing files which match certain pattern."""
    for filename in glob.iglob('./**/[0-9]+.csv', recursive=True):
        os.remove(filename)

def chunks(it, size):
    it = iter(it)
    return iter(lambda: tuple(islice(it, size)), ())

def split(csv_file, number_of_slices, first_column):
    """Split file into number_of_slices pieces."""
    with open(csv_file, 'r') as input_file:
        reader = csv.reader(input_file)

        for num_file, chunk in enumerate(chunks(reader, number_of_slices)):
            with open(f"{num_file}.csv", 'w') as output_file:
                writer = csv.writer(output_file)

                if first_column:
                    for row in chunk:
                        writer.writerow([first_column] + row)
                else:
                    writer.writerows(chunk)

if __name__ == "__main__":
    # TODO: argparse?
    if int(sys.argv[1]) <= 0:
        raise SystemExit("Piece count must be natural number greater than zero.")
    number_of_slices = int(sys.argv[1])
    first_column = sys.argv[2] if len(sys.argv) > 2 else None

    remove_old_files()

    for filename in glob.iglob('./**/*.csv', recursive=True):
        split(filename, number_of_slices, first_column)


Golang HTTP status checker I'm writing my first somewhat production ready (or not) Go program and could do with some feedback from someone more experienced with go.

The code reads a list of URLs from a JSON file and then makes a GET request to each URL in a loop to check that they are returning a 200 OK response. I have attempted to do this concurrently using go routines but I'm concerned about my synchronization step.

monitor.go

package main

import (
    "encoding/json"
    "fmt"
    "github.com/bradfitz/gomemcache/memcache"
    "os"
    "time"
)

// Configuration encapsulates the result of reading the JSON configuration
// file.
type Configuration struct {
    URLs      []string
    Memcached string
}

// loadConfig loads a configuration file in JSON format and returns a
// Configuration instance.
func loadConfig(path string) (Configuration, error) {
    file, _ := os.Open(path)
    defer file.Close()

    decoder := json.NewDecoder(file)
    configuration := Configuration{}
    err := decoder.Decode(&configuration)

    return configuration, err
}

func main() {
    type Result struct {
        url    string
        status Status
    }

    rc := make(chan Result)

    configuration, err := loadConfig("config.json")

    if err != nil {
        fmt.Println("Error :", err)
        return
    }

    sites := make(map[string]*Site, len(configuration.URLs))

    for _, url := range configuration.URLs {
        sites[url] = &Site{url, UNCHECKED}
    }

    mc := memcache.New(configuration.Memcached)

    sites_output := make(map[string]bool)

    for {
        for _, site := range sites {
            go func(site *Site, rc chan Result) {
                status, _ := site.Status()
                rc <- Result{site.url, status}
            }(site, rc)
        }

        for i := 0; i < len(sites); i++ {
            res := <-rc
            site := sites[res.url]
            if site.last_status != res.status {
                sites[res.url].last_status = res.status
            }
        }

        for k, v := range sites {
            sites_output[k] = v.last_status == 2
        }

        site_json, err := json.Marshal(sites_output)

        if err != nil {
            panic(err)
        }
        mc.Set(&memcache.Item{
            Key:   "mission_control.sites",
            Value: site_json,
        })
        time.Sleep(time.Second * 5)
    }
}

site.go

package main

import (
    "net/http"
)

type Status int

const (
    UNCHECKED Status = iota
    DOWN
    UP
)

// The Site struct encapsulates the details about the site being monitored.
type Site struct {
    url         string
    last_status Status
}

// Site.Status makes a GET request to a given URL and checks whether or not the
// resulting status code is 200.
func (s Site) Status() (Status, error) {
    resp, err := http.Get(s.url)
    status := s.last_status

    if (err == nil) && (resp.StatusCode == 200) {
        status = UP
    } else {
        status = DOWN
    }

    return status, err
}

Style

Use CamelCase only. last_status -> lastStatus, site_json -> siteJSON, etc.
Even in constants. UP -> Up or StatusUp, etc.
Imports are usually split into groups by source, so separate memcache from stdlib with a newline.
Stuttering. E.g. configuration := Configuration{} could be c := Configuration{}.
People would search for main() in main.go. So I would rename monitor.go to main.go.

Code

file, _ := os.Open(path)

I see you skip error handling in some places. I'm not sure whether this is just a brief version for this site, or an actual code, but this is a very bad practice.

v.last_status == 2

if (err == nil) && (resp.StatusCode == 200) {

Avoid magic numbers. Use Up/StatusUp and http.StatusOK respectively.

for i := 0; i < len(sites); i++ {

I'd use range. In Go 1.4 it'll be legal to write for range sites as well.

site := sites[res.url]
if site.last_status != res.status {
    sites[res.url].last_status = res.status
}

You index the map twice and make an unnecessary comparison. Wouldn't sites[res.url] = res.status do?

Architecture

If you can change the output JSON format, I'd suggest saving the status code returned by http.Get rather than a simple bool. That will give you more info on why the site is down (4XX and 5XX error) or that the site have moved (3XX).

  I have attempted to do this concurrently using go routines but I'm concerned about my synchronization step.

Your synchronisation seems mostly fine to me. (Don't forget to test with -race though). An alternative would be to use sync.WaitGroup and a map with a sync.Mutex.


Schema for allowing adding tags to entities in unrelated tables I'm implementing a "tags" features to an already working solution.

Final users need to be able to add tags to three separate sections of the solution:

Posts
Accounts
Groups

Each section has its corresponding table in the database, and each table contains a unique ID:

PostID
AccountID
GroupID

I'm thinking about implementing a tag schema similar to the one that Wordpress uses, where I'll have:

A tagmap table that will contain a unique ID for each "tagmap".
A foreign key to each section's table ID.
Another foreign key to the ID of the tag.

CREATE TABLE Posts(
  PostID int(2) NOT NULL AUTO_INCREMENT,
  Content varchar(255),
  PRIMARY KEY(PostID)
 );

CREATE TABLE Groups(
  GroupID int(2) NOT NULL AUTO_INCREMENT,
  GroupName varchar(255),
  PRIMARY KEY(GroupID)
 );

CREATE TABLE Links(
  LinkID int(2) NOT NULL AUTO_INCREMENT,
  href varchar(255),
  PRIMARY KEY(LinkID)
 );

CREATE TABLE tagmap(
  TagmapID int(2) NOT NULL AUTO_INCREMENT,
  PostID int(2),
  GroupID int(2),
  LinkID int(2),
  TagID int(2) NOT NULL,
  PRIMARY KEY(TagmapID)
 );

CREATE TABLE tags(
  TagID int(2) NOT NULL AUTO_INCREMENT,
  TagName varchar(255) NOT NULL,
  PRIMARY KEY(TagID)
);

I like this approach because it is decently normalized, but queries might get a bit complex. Also, it will have a lot of NULL columns every time a tag is assigned to a post but not to an account or to a group, so I'm unsure how it will behave performance-wise (the table will hold around 100,000 records almost immediately).

Is there a better alternative to this?

A simple solution is to include a table reference in the tagmap instead of many id columns.

CREATE TABLE tagmap(
  TagmapID int(2) NOT NULL AUTO_INCREMENT,
  RefTable varchar(255) NOT NULL,
  RefID int(2),
  TagID int(2) NOT NULL,
  PRIMARY KEY(TagmapID),
  INDEX(RefTable, RefID),
  UNIQUE(RefTable, RefID, TagID)
);

With that approach, you can easily make other tables taggable. To retrieve the tags for a post, you'd just say

SELECT TagName FROM tags
LEFT JOIN tagmap USING TagID
WHERE RefTable = 'posts' AND RefID = 42


Sanitizing words extracted from text files and writing them to a database This code reads text files from some path, tokenizes, removes stop words, lowercases, removes punctuation and numbers, then writes the result to a database.

# -*- coding: utf-8 -*-
from __future__ import print_function
import os, codecs, re, string, mysql
import mysql.connector

'''Reading files with txt extension'''
y_ = ""
for root, dirs, files in os.walk("/Users/Documents/source-document/part1"):
    for file in files:
        if file.endswith(".txt"):
            x_ = codecs.open(os.path.join(root,file),"r", "utf-8-sig")
            for lines in x_.readlines():
                y_ = y_ + lines
#print(tokenized_docs)

'''Tokenizing sentences of the text files'''

from nltk.tokenize import sent_tokenize
raw_docs = sent_tokenize(y_)

tokenized_docs = [sent_tokenize(y_) for sent in raw_docs]

'''Removing stop words'''

stopword_removed_sentences = []
from nltk.corpus import stopwords
stopset = stopwords.words("English")
for i in tokenized_docs[0]:
    tokenized_docs = ' '.join([word for word in i.split() if word not in stopset])
    stopword_removed_sentences.append(tokenized_docs)

''' Removing punctuation marks'''

regex = re.compile('[%s]' % re.escape(string.punctuation))
nw = []
for review in stopword_removed_sentences:
    new_review = ''
    for token in review:
        new_token = regex.sub(u'', token)
        if not new_token == u'':
            new_review += new_token
    nw.append(new_review)

'''Lowercasing letters after removing puctuation marks.'''

lw = []  #lw stands for lowercase word.
for i in nw:
    k = i.lower()
    lw.append(k)

'''Removing number with a dummy symbol'''
nr = []
for j in lw:
    string = j
    regex = r'[^\[\]]+(?=\])'
# let "#" be the dummy symbol
    output = re.sub(regex,'#',string)
    nr.append(output)
nrfinal = []
for j in nr:
    rem = 0
    outr = ''
    for i in j:
        if ord(i)>= 48 and ord(i)<=57:
            rem += 1
            if rem == 1:
                outr = outr+ '#'
        else:
             rem = 0
             outr = outr+i
    nrfinal.append(outr)
'''Inserting into database'''
def connect():
    for j in nrfinal:
        conn = mysql.connector.connect(user = 'root', password = '', unix_socket = "/tmp/mysql.sock", database = 'Thesis' )
        cursor = conn.cursor()
        cursor.execute("""INSERT INTO splitted_sentences(sentence_id, splitted_sentences) VALUES(%s, %s)""",(cursor.lastrowid,j))
        conn.commit()
        conn.close()
if __name__ == '__main__':
    connect()

I am not getting any error with this code. Please review it for writing it in a more optimized way.

Memory usage

What I notice most, is that this is bound to use a lot of memory.

First you load all the files (at once) into memory.

Then you copy over (with modifications) the data into another variable. And another. And another. ...

This creates a few lists, all with the same total memory size as the entire documents you have read.

Ex-pen-sive.

Generators/Iterators to the rescue!

In Python, iterators are really nice. They allow something like continuations and just-in-time calculation. This causes memory usage to be a lot lower, on the (perhaps) extra cost of a bit more CPU processing. But... I think in this case it will overall be a saving because you have a lot less memory usage, so probably also less cache-misses.

Let's see what we can do about that.

The last step

nrfinal = []
for j in nr:
    rem = 0
    outr = ''
    for i in j:
        if ord(i)>= 48 and ord(i)<=57:
            rem += 1
            if rem == 1:
                outr = outr+ '#'
        else:
             rem = 0
             outr = outr+i
    nrfinal.append(outr)

The easiest step is to first make it a function.

def calc_nrfinal(inp):
    retval = []
    for j in nr:
        rem = 0
        outr = ''
        for i in j:
            if ord(i)>= 48 and ord(i)<=57:
                rem += 1
                if rem == 1:
                    outr = outr+ '#'
            else:
                 rem = 0
                 outr = outr+i
        retval.append(outr)
    return retval
nrfinal = calc_nrfinal(nr)

Now this does not save us memory. But by rewriting it a little bit, it will become a generator.

def calc_nrfinal(inp):
    for j in nr:
        rem = 0
        outr = ''
        for i in j:
            if ord(i)>= 48 and ord(i)<=57:
                rem += 1
                if rem == 1:
                    outr = outr+ '#'
            else:
                 rem = 0
                 outr = outr+i
        yield outr
nrfinal = calc_nrfinal(nr)

The downside is that you can only iterate over nrfinal once, but you only need to do it once.

The inner loop

One thing that bothers me is the inner loop here. ord(i)>=49 and ord(i)<=57) could be written as 49 <= ord(i) <= 57 (one of Pythons strengths!). But... I'd rather write

outr = re.sub(r'[0-9]+', '#', j)

Giving us

def calc_nrfinal(inp):
    for j in nr:
        yield re.sub(r'[0-9]+', '#', j)
nrfinal = calc_nrfinal(nr)

But now, the whole generator-function is a bit annoying, and I'd like to switch to ... (drumroll)

Generator comprehensions

Defining lots of separate generator functions can be really annoying, especially when the body of the generator is quite small. There's a solution for that: generator comprehensions.

def f(iterable):
    for val in iterable:
        yield g(val)
k = f()

Can be rewritten as

k = (g(val) for val in iterable)

It even supports multiple fors and ifs inside the loops.

Anyway, I disgres. What I mean is:

nrfinal = (re.sub(r'[0-9]+', '#', j) for j in nr)

And, nrfinal will take almost no memory at all, because all the values are evaluated just-in-time. The downside is you can not iterate twice (because it is not stored).

Continuing in similar fashion:

Here I did the same for a couple of the pieces.

# -*- coding: utf-8 -*-
from __future__ import print_function
import os, codecs, re, string, mysql
import mysql.connector

'''Reading files with txt extension'''
y_ = ""
for root, dirs, files in os.walk("/Users/Documents/source-document/part1"):
    for file in files:
        if file.endswith(".txt"):
            x_ = codecs.open(os.path.join(root,file),"r", "utf-8-sig")
            for lines in x_.readlines():
                y_ = y_ + lines
#print(tokenized_docs)

'''Tokenizing sentences of the text files'''

from nltk.tokenize import sent_tokenize
raw_docs = sent_tokenize(y_)

tokenized_docs = [sent_tokenize(y_) for sent in raw_docs]

'''Removing stop words'''

stopword_removed_sentences = []
from nltk.corpus import stopwords
stopset = stopwords.words("English")
for i in tokenized_docs[0]:
    tokenized_docs = ' '.join([word for word in i.split() if word not in stopset])
    stopword_removed_sentences.append(tokenized_docs)

''' Removing punctuation marks'''

regex = re.compile('[%s]' % re.escape(string.punctuation))
nw = []
for review in stopword_removed_sentences:
    new_review = ''
    for token in review:
        new_token = regex.sub(u'', token)
        if not new_token == u'':
            new_review += new_token
    nw.append(new_review)

lw = (i.lower() for i in nw)
nr = (re.sub(r'[^\[\]]+(?=\])', '#', j) for j in lw)
nrfinal = (re.sub('[0-9]+', '#', j) for j in nr)
'''Inserting into database'''
def connect():
    for j in nrfinal:
        conn = mysql.connector.connect(user = 'root', password = '', unix_socket = "/tmp/mysql.sock", database = 'Thesis' )
        cursor = conn.cursor()
        cursor.execute("""INSERT INTO splitted_sentences(sentence_id, splitted_sentences) VALUES(%s, %s)""",(cursor.lastrowid,j))
        conn.commit()
        conn.close()
if __name__ == '__main__':
    connect()

Unnecessary looping

Here I got a bit lost, due to the double loop in nw and new_review... For this, I first need to understand the contents of stopword_removed_sentences. Let's look a few lines up:

stopword_removed_sentences = []
from nltk.corpus import stopwords
stopset = stopwords.words("English")
for i in tokenized_docs[0]:
    tokenized_docs = ' '.join([word for word in i.split() if word not in stopset])
    stopword_removed_sentences.append(tokenized_docs)

What this tells me:

stopwords_removed_sentences is a list.
The value of each of the lists is a string (' '.join(...) must be a list).

Ok, for now I know enough. This tells me:

review is a string.
token is a 1-character substring of review.

Looking back to the troublesome code:

regex = re.compile('[%s]' % re.escape(string.punctuation))
nw = []
for review in stopword_removed_sentences:
    new_review = ''
    for token in review:
        new_token = regex.sub(u'', token)
        if not new_token == u'':
            new_review += new_token
    nw.append(new_review)

This means we call regex.sub for all characters of review.

I have three different solutions:

new_review = regex.sub(u'', review)

Which reads very nice! Or

new_review = ''.join(token for token in review if token in string.punctuation)

Which is probably a bit more expensive due to the looping in Python (instead of C). Or,

new_review = review.translate(None, string.punctuation)

Which also saves us pre-compiling a regex. More readable, I think. Any are fine, but I think the review.translate is the easiest to look at.

Using that:

# -*- coding: utf-8 -*-
from __future__ import print_function
import os, codecs, re, string, mysql
import mysql.connector

'''Reading files with txt extension'''
y_ = ""
for root, dirs, files in os.walk("/Users/Documents/source-document/part1"):
    for file in files:
        if file.endswith(".txt"):
            x_ = codecs.open(os.path.join(root,file),"r", "utf-8-sig")
            for lines in x_.readlines():
                y_ = y_ + lines
#print(tokenized_docs)

'''Tokenizing sentences of the text files'''

from nltk.tokenize import sent_tokenize
raw_docs = sent_tokenize(y_)

tokenized_docs = [sent_tokenize(y_) for sent in raw_docs]

'''Removing stop words'''

stopword_removed_sentences = []
from nltk.corpus import stopwords
stopset = stopwords.words("English")
for i in tokenized_docs[0]:
    tokenized_docs = ' '.join([word for word in i.split() if word not in stopset])
    stopword_removed_sentences.append(tokenized_docs)

nw = (j.translate(None, string.punctuation) for j in stopword_removed_sentences)
lw = (i.lower() for i in nw)
nr = (re.sub(r'[^\[\]]+(?=\])', '#', j) for j in lw)
nrfinal = (re.sub('[0-9]+', '#', j) for j in nr)
'''Inserting into database'''
def connect():
    for j in nrfinal:
        conn = mysql.connector.connect(user = 'root', password = '', unix_socket = "/tmp/mysql.sock", database = 'Thesis' )
        cursor = conn.cursor()
        cursor.execute("""INSERT INTO splitted_sentences(sentence_id, splitted_sentences) VALUES(%s, %s)""",(cursor.lastrowid,j))
        conn.commit()
        conn.close()
if __name__ == '__main__':
    connect()

Quadratic run-time in the number of sentences.

Look at this code:

raw_docs = sent_tokenize(y_)

tokenized_docs = [sent_tokenize(y_) for sent in raw_docs]

First, you get the sentences from y_. Then, you again get the sentences from y_, but as many times as there are sentences in y_. Then, you only use the first item from the second set.

raw_docs = sent_tokenize(y_)

tokenized_docs = [sent_tokenize(y_) for sent in raw_docs]

'''Removing stop words'''

stopword_removed_sentences = []
from nltk.corpus import stopwords
stopset = stopwords.words("English")
for i in tokenized_docs[0]:
    tokenized_docs = ' '.join([word for word in i.split() if word not in stopset])
    stopword_removed_sentences.append(tokenized_docs)

Becomes

raw_docs = sent_tokenize(y_)

tokenized_docs = [sent_tokenize(y_) for sent in raw_docs]

'''Removing stop words'''

stopword_removed_sentences = []
from nltk.corpus import stopwords
stopset = stopwords.words("English")
for i in raw_docs:
    tokenized_docs = ' '.join([word for word in i.split() if word not in stopset])
    stopword_removed_sentences.append(tokenized_docs)

And now switch to generators again.

But, we also want generators

raw_docs = sent_tokenize(y_)
stopword_removed_sentences = (' '.join(word for word in i.split() if word not in stopset) for i in raw_docs)

Now this becomes a bit bulky, though. The inner text is quite long. Let's make that a function.

def strip_stopwords(sentence):
    return ' '.join(word for word in sentence.split() if word not in stopset)

stopword_removed_sentences = (strip_stopwords(sentence) for sentence in raw_docs)

Recognizing a pattern.

Now, we end up with

...
stopword_removed_sentences = (strip_stopwords(sentence) for sentence in raw_docs)
nw = (j.translate(None, string.punctuation) for j in stopword_removed_sentences)
lw = (i.lower() for i in nw)
nr = (re.sub(r'[^\[\]]+(?=\])', '#', j) for j in lw)
nrfinal = (re.sub('[0-9]+', '#', j) for j in nr)
...

Let's see if we can make that more readable...

actions = [
    strip_stopwords,
    lambda sentence: sentence.translate(None, string.punctuation),
    str.lower,
    lambda blob: re.sub(r'[^\[\]]+(?=\])', '#', blob),
    lambda blob: re.sub(r'[0-9]+', '#', blob),
]

def apply_all_actions(val):
    for action in actions:
        val = action(val)
    return action

nrfinal = (apply_all_actions(val) for val in raw_docs)

(Here we abuse the fact that str.lower is the same as lambda f: f.lower() if it is certain that f is a string.)

Now it is really clear that nrfinal is constructed from raw_docs by a sequence of simple transformations on the elements. If you want, you can define functions for all of the separate actions, given

actions = [
    strip_stopwords,
    remove_punctuation,
    lowercase,
    (some magic name, I don't know what),
    squash_numbers_to_hash,
]

But, this is a matter of choice.

Connection management.

Now, for something completely different.

def connect():
    for j in nrfinal:
        conn = mysql.connector.connect(user = 'root', password = '', unix_socket = "/tmp/mysql.sock", database = 'Thesis' )
        cursor = conn.cursor()
        cursor.execute("""INSERT INTO splitted_sentences(sentence_id, splitted_sentences) VALUES(%s, %s)""",(cursor.lastrowid,j))
        conn.commit()
        conn.close()

Let's read what this does:

for every sentence in nrfinal, do the following:
** Create a connection to a mysql host.
** Create a cursor.
** Insert a value.
** Commit the connection.
** Close the connection.

Connecting is expensive. Why not do that outside the loop?

def connect():
    conn = mysql.connector.connect(user = 'root', password = '', unix_socket = "/tmp/mysql.sock", database = 'Thesis' )
    for j in nrfinal:
        cursor = conn.cursor()
        cursor.execute("""INSERT INTO splitted_sentences(sentence_id, splitted_sentences) VALUES(%s, %s)""",(cursor.lastrowid,j))
        conn.commit()
    conn.close()

I'm hesitant to do this for the cursor, because that might break things. Why, you ask? Because of the cursor.lastrowid. Before a statement has been made, it is None. So we must mimic that.

def connect():
    conn = mysql.connector.connect(user = 'root', password = '', unix_socket = "/tmp/mysql.sock", database = 'Thesis' )
    cursor = conn.cursor()
    for j in nrfinal:
        cursor.execute("""INSERT INTO splitted_sentences(sentence_id, splitted_sentences) VALUES(%s, %s)""",(None,j))
    conn.commit()
    conn.close()

In fact, I think you want to make sentence_id an AUTO_INCREMENT. Then, write

def connect():
    conn = mysql.connector.connect(user = 'root', password = '', unix_socket = "/tmp/mysql.sock", database = 'Thesis' )
    cursor = conn.cursor()
    for j in nrfinal:
        cursor.execute("""INSERT INTO splitted_sentences(splitted_sentences) VALUES (%s)""",(j,))
    conn.commit()
    conn.close()

Executing many statements

The mysql.connector allows executemany.

nrtuples = ((j,) for j in nrfinal)
def connect():
    conn = mysql.connector.connect(user = 'root', password = '', unix_socket = "/tmp/mysql.sock", database = 'Thesis' )
    cursor = conn.cursor()
    cursor.executemany("""INSERT INTO splitted_sentences(splitted_sentences) VALUES (%s)""", nrtuples)
    conn.commit()
    conn.close()

That should take care of the memory usage.

Loading all the files

y_ = ""
for root, dirs, files in os.walk("/Users/Documents/source-document/part1"):
    for file in files:
        if file.endswith(".txt"):
            x_ = codecs.open(os.path.join(root,file),"r", "utf-8-sig")
            for lines in x_.readlines():
                y_ = y_ + lines

You load all files into memory at once. This is expansive. Rather not. Let's assume that there are no cross-file sentences going on (at least I hope not!).

def get_sentences():
    for root, dirs, files in os.walk("/Users/Documents/source-document/part1"):
        for file in files:
            if file.endswith(".txt"):
                x_ = codecs.open(os.path.join(root,file),"r", "utf-8-sig")
                yield x_.read()

If you also know that there are no cross-line sentences, you can use

def get_sentences():
    for root, dirs, files in os.walk("/Users/Documents/source-document/part1"):
        for file in files:
            if file.endswith(".txt"):
                x_ = codecs.open(os.path.join(root,file),"r", "utf-8-sig")
                for lines in x_.readlines():
                    yield lines

I'm not that big a fan of this deep nesting, and would prefer to also write a generator which yields the .txt files. But I hope you can continue from here.

Also, maybe you should use with codecs.open(....) as x_, that way the file gets closed when the iteration is done.

Further remarks.

There are certain style guidelines.

Keep your imports at the top of the file.

First a module-level docstring (if present).
Then, the imports from the standard library. (preferably sorted).
Then, imports from other libraries.
Then, imports from other parts of the project.
Then class/function definitions (in whatever order you please)
Finally, only a if __name__ == '__main__': block. Preferably containing only a function-call to main() in the module.

docstrings are no comments. Comments are no docstrings.

In the code you have ''' some useful text'''. But you actually want to use a comment: # some useful text. Know to disambiguate between those.

Multi-line comments? Just start every line with #.


Simplify an if else construct with assignment before if and else I want to simplify an if: .. else: .. construct:

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        edge = propose_extension_edge(available_edges, path)
        if extension_is_possible(edge):
            extend_path(available_edges, path, edge)
        else:
            node = propose_node_for_reorder(available_edges, path)
            if reorder_is_sensible(node):
                path = reorder_path(available_edges, path, node)
            else:
                return None
    return path

It seems unnecessarily complicated, because the idea can be expressed much simpler.

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        if extension_is_possible(available_edges, path):
            extend_path(available_edges, path)
        elif reorder_is_sensible(available_edges, path):
            path = reorder_path(available_edges, path)
        else:
            return None
    return path

But now I have to do the calculations from

edge = propose_extension_edge(available_edges, path)

and

node = propose_node_for_reorder(available_edges, path)

twice because I can't keep the results in variables (edge = .., node = ..).

I am looking for an idea to keep code as simple as it is with the elif construction while doing the propose_node..-calculation only once.

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        edge = propose_extension_edge(available_edges, path)
        if extension_is_possible(edge):
            extend_path(available_edges, path, edge)
        else:
            node = propose_node_for_reorder(available_edges, path)
            if reorder_is_sensible(node):
                path = reorder_path(available_edges, path, node)
            else:
                return None
    return path

def initial_state(graph):
    available, path = graph[:], []
    if len(available) > 0:
        path.append(available[0][0])
    return available, path

def incomplete(available_edges):
    return len(available_edges) > 0

def propose_extension_edge(available_edges, path):
    last_node = path[-1]
    for edge in available_edges:
        if last_node in edge:
            return edge
    return None

def extension_is_possible(edge):
    return edge

def extend_path(available_edges, path, edge):
    if edge[0] == path[-1]:
        path.append(edge[1])
    else:
        path.append(edge[0])
    available_edges.remove(edge)

def propose_node_for_reorder(available_edges, path):
    if reorder_is_possible(path):
        return find_split(available_edges, path)
    return None

def reorder_is_possible(path):
    return is_tour(path)

def find_split(available_edges, path):
    options = edgenodes(available_edges)
    for node in options:
        if node in path:
            return node
    return None

def reorder_is_sensible(node):
    return node

def is_tour(path):
    return path[0] == path[-1]

def edgenodes(edges):
    result = []
    for edge in edges:
        for node in edge:
            if not node in result:
                result.append(node)
    return result

def reorder_path(available_edges, tour, split):
    split_location = tour.index(split)
    return tour[split_location:-1] + tour[0:split_location+1]

Example use:

graph1 = [(1, 2), (2, 3), (3, 1)]
graph2 = [(0, 1), (1, 5), (1, 7), (4, 5), (4, 8), (1, 6), (3, 7), (5, 9), (2, 4), (0, 4), (2, 5), (3, 6), (8, 9)]
graph3 = [(1, 13), (1, 6), (6, 11), (3, 13), (8, 13), (0, 6), (8, 9),(5, 9), (2, 6), (6, 10), (7, 9), (1, 12), (4, 12), (5, 14), (0, 1), (2, 3), (4, 11), (6, 9), (7, 14), (10, 13)]

print find_eulerian_tour(graph1)
print find_eulerian_tour(graph2)
print find_eulerian_tour(graph3)

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        edge = propose_extension_edge(available_edges, path)
        if extension_is_possible(edge):
            extend_path(available_edges, path, edge)
        else:
            node = propose_node_for_reorder(available_edges, path)
            if reorder_is_sensible(node):
                path = reorder_path(available_edges, path, node)
            else:
                return None
    return path

Since you are inside a while block, an if-else can be turned into if->continue. The inner if-else can be turned into if not->return:

def find_eulerian_tour(graph):
    available_edges, path = initial_state(graph)
    while incomplete(available_edges):
        edge = propose_extension_edge(available_edges, path)
        if extension_is_possible(edge):
            extend_path(available_edges, path, edge)
            continue
        node = propose_node_for_reorder(available_edges, path)
        if not reorder_is_sensible(node):
            return None
        path = reorder_path(available_edges, path, node)
    return path

Now it reads like a checklist:

Extension possible? Do that and continue.
Reordering not sensible? Quit.
Reorder.

Is it an improvement? On the original: I think so. Nested logic is generally more difficult to follow. On the if-elif-else? Not really, but that's the price to pay for caching the intermediary values.


Create a histogram of the lengths of words I am reading through K&R C 2nd Edition, and I am on exercise 1-13. The exercise is to write a program to print a histogram of the lengths of words in
its input. I wrote a program that successfully does this; however, after looking at the program, I noticed that it was somewhat messy, and I'm sure that there is a better way to implement it. I'm looking for some tips as to how I can improve programs like this in the future.

Here is the code:

#include <stdio.h>
#define IN 1
#define OUT 0
#define MAXVAL 11

int main(){
    int i, j, c, state;
    int wordLengths[MAXVAL];
    int currentWord = 0;
    int greaterThanMax = 0;

    for(i = 0; i < MAXVAL; i++){
        wordLengths[i] = 0;
    }

    while((c = getchar()) != EOF){
        ++currentWord;
        state = IN;
        if(c == '\n' || c == ' ' || c == '\t'){
            state = OUT;
            --currentWord;
        }
        if(state == OUT){
            if(currentWord < MAXVAL){
                ++wordLengths[currentWord];
            }
            else{
                ++greaterThanMax;
            }
            currentWord = 0;
        }
    }
    for(i = 1; i < MAXVAL; i++){
        printf("%d letter(s): ", i);
        for(j = 0; j <= wordLengths[i] - 1; j++){
            putchar('=');
        }
        putchar('\n');
    }
    printf(">%d: ", MAXVAL - 1);
    for(i = 0; i < greaterThanMax; i++){
        putchar('=');
    }
    return 0;
}

Do not be shy on horizontal spacing. Add space after the keywords (e.g. while (, or if (). Insert space into ){.

If you place an opening curly bracket on the same line as if, be consistent with else:

  if () {
      ....
  } else {
      ....
  }

currentWord actually refers to the current word length. Consider renaming.
Instead of testing state == OUT, consider adding a word immediately, as soon as state become OUT:

    if (c == '\n' || c == ' ' || c == '\t') {
        if (currentWordLength < MAXVAL) {
            ++wordLengths[currentWordLength - 1]; {
        } else {
            ++greaterThanMax;
        }
        currentWord = 0;
    }

Notice that with this approach you don't need to maintain state explicitly.
Every output line, except last one, starts with a single-digit number, but the last one starts with 10, and looks unaligned. Consider printing letter count with "%2d".
The greater-than-max line doesn't end with a newline. Some shells (like cmd.exe) automatically add a newline to the last output. Unixish shells do not. On my system an output looks like

....
>10: ==vnp>

Even on Windows, try to redirect your output into a file.

It is usually a good idea to terminate the output with a newline.


Continued fraction with relative precision in Python I have the following recursive function to compute the continued fraction:

$$s_i = \frac{a_{i}}{b_{i} + s_{i+1}}$$

The relative error is defined as:

$$\text{Delta} = \frac{s_n - s_{n-1}}{s_{n}}$$

Here is the implementation in Python, of Lentz's method for computing continued fractions:

import numpy as np

def n_continued_fraction(a, b, err, i_min=3):
    f = lambda i : a(i)
    g = lambda i : b(i)
    d = lambda i : 1./g(0) if i == 0 else 1./(g(i) + f(i) * d(i-1))
    h = lambda i : 0 if i == -1 else f(0)/g(0) if i == 0 else (h(i-1) * g(i) + h(i-2) * d(i-1) * f(i)) * d(i)
    delta_h = lambda i : h(i) - h(i-1)

    i_final = i_min

    while np.abs(delta_h(i_final)) > err * np.abs(h(i_final)):
        i_final += 1

    return h(i_final)

and you can test it with e.g.

a = lambda i : (i+1.)
b = lambda i : (i+5.)

n_continued_fraction(a, b, 1.e-16)

Is there a better method to do it? Am I doing anything wrong?

Nice code, it does what it should and it is short. But it can be improved :)

General python tips:

Use type annotation. You can look at the type annotation of Sam Stafford in the previous answer, it will make your code more readable and will prevent bugs.
Remove redundant imports. Is numpy really needed here? You can use a simple abs.
Don't use lambda functions. According to PEP 8 "do not assign lambda expression, use a def", there are several reasons why, mostly due to readability.

Optimisation tips:
Note these tips contradict each other. Choose one and use it but you cannot use both.

If N is the number of elements until convergence, and a and b is the calculation time of each function (Assuming equal for every x) The time complexity now is O(N ^ 2 * a * b), for each i you calculate the entire series over and over.
If you will use exponential search you can reduce the complexity to O(N * log N * a * b). Which will be much much faster!
Readability vs. Runtime. If the readability is important to you you can ignore this tip. You can replace the recursion with a dynamic programing approach, this will remove the recursion and the iteration over i, the time complexity will be O(N * a * b) but you will need to rewrite a lot of the code.


Extension method to retrieve a sample from a collection I wrote a little extension method to retrieve a random sample of items from a collection. I've tried to write clean code. What do you think?

is it clean?
does it work as you would expect?

I'll be glad to hear your comments.

using System;
using System.Collections.Generic;
using System.Linq;

namespace Extensions
{
    public static class IEnumerableExtensions
    {
        private static Random _rnd = new Random();

        public static T Sample<T>(this IEnumerable<T> values)
        {
            return Sample(values, 1).First();
        }

        public static IEnumerable<T> Sample<T>(this IEnumerable<T> values, int size)
        {
            if(!values.Any())
            {
                throw new InvalidOperationException("Collection empty.");
            }

            if (size < 1)
            {
                return new List<T>();
            }

            if (size >= values.Count())
            {
                return values;
            }

            return GetSamples(values, size);
        }
        private static IEnumerable<T> GetSamples<T>(IEnumerable<T> values, int size)
        {
            var samples = new List<T>();
            var source = new List<T>(values);
            while (samples.Count != size)
            {
                var index = _rnd.Next(source.Count());
                var value = source[index];
                samples.Add(value);
                source.Remove(value);
            }
            return samples;
        }
    }
}

public static T Sample<T>(this IEnumerable<T> values)
        {
            return Sample(values, 1).First();
        }

Nice helper method.

            if(!values.Any())
            {
                throw new InvalidOperationException("Collection empty.");
            }

I would have used an ArgumentOutOfRangeException, but that's a matter of personal preference. Should it throw an exception if size == 0 or return an empty list? That's a tricky edge case which should be clearly documented.

        public static IEnumerable<T> Sample<T>(this IEnumerable<T> values, int size)

I would prefer the Random to be an argument too, even if it's as Random rnd = null with a fallback inside the method. Being able to seed your random selection is important for testing the code and for reproducing results if it's a scientific project.

            if (size < 1)
            {
                return new List<T>();
            }

I would definitely prefer to throw an ArgumentOutOfRangeException if size < 0.

            if (size >= values.Count())
            {
                return values;
            }

If size > values.Count() then I think that should be ArgumentOutOfRangeException or InvalidOperationException. Again, whichever way you finally decide, it should be clearly documented.

Note that we've now called two methods which iterate partially or fully through values. If it was a non-deterministic iterator we have problems. An example of a non-deterministic iterator would be

public IEnumerable<int> Demo()
{
    var rnd = new Random();
    for (int i = rnd.Next(10); i > 0; i--) yield return rnd.Next();
}

If you call this once and then loop through the resulting iterator multiple times then it will not consistently return the same values. You may think that in real usage you won't come across iterators like that, but sometimes the non-determinism is a lot more subtle.

            var samples = new List<T>();
            var source = new List<T>(values);

Since we have to convert source to a list here, there's an argument for doing it earlier and ensuring that we only iterate through values once, solving the problem of non-deterministic iterators.

                source.Remove(value);

This is expensive. It's asymptotically more efficient to replace this with

                source[index] = source[source.Count - 1];
                source.RemoveAt(source.Count - 1);

Finally, on the names. In your context it might be obvious that Sample means "sample without replacement", but in some contexts it might be important to distinguish between sampling with and without replacement. Consider whether you might need to distinguish the cases in future usage of the library.

Postscript: t3chb0t's suggestion of a Randomise method which can then be combined with Take is compatible with a rewrite of your GetSamples method, and in fact this is what I have kicking around in my library of utility methods (although I call it Shuffle). As a modification of your code:

    public static IEnumerable<T> Randomise<T>(IEnumerable<T> values, Random rnd = null)
    {
        if (rnd == null) rnd = _rnd;

        var source = new List<T>(values);
        while (source.Count > 0)
        {
            var index = rnd.Next(source.Count);
            yield return source[index];
            source[index] = source[source.Count - 1];
            source.RemoveAt(source.Count - 1);
        }
    }


Get elementary values from a flagged enum I created a little function that returns the elementary values from a flagged enum. By "elementary" I mean the values that are powers of 2, excluding any combined enum values. I was a bit surprised I couldn't find a buit-in method for this in .Net (or I missed it).

Let's take this flagged enum:

[Flags]
public enum WeekDay
{
    Monday = 1 << 0,
    Tuesday = 1 << 1,
    Wednesday = 1 << 2,
    Thursday = 1 << 3,
    Friday = 1 << 4,
    Saturday = 1 << 5,
    Sunday = 1 << 6,

    WeekendDay = Saturday | Sunday,
    BusinessDay = Monday | Tuesday | Wednesday | Thursday | Friday
}

Since the binary representation of its values looks as follows...

1
10
100
1000
10000
100000
1000000

1100000
11111

...I came up with this function to extract the elementary values:

public IEnumerable<TEnum> GetElementaryValues<TEnum>()
    where TEnum: Enum
{
    return Enum.GetValues(typeof(TEnum))
        .Cast<TEnum>()
        .Where(v =>
        {
            var intValue = Convert.ToInt64(v);
            var binary = Convert.ToString(intValue, 2);
            return !binary.Skip(1).Any(c => c == '1');
        });
}

Which basically says: return each value that hasn't got a 1 beyond its first character.

Doest this look OK? Can it be improved? My feeling is that it's a lot of expensive code for such a simple task.

The way to test for a power of two, where x is an unsigned integer type is

( x != 0 ) && ( ( x & ( x - 1 ) ) == 0 )

To understand why this works, it's fairly easy to see that if x is a power of two, then x & (x -1 ) is zero.

If x is not a power of two, then only the bits up to the first non-zero bit are changed when you subtract one, and that is not the most significant bit, so the most significant bit is not cleared, and so x & (x - 1 ) is non-zero, as required.

Zero is a special case - it's not a power of two, so an extra test is needed, also ( x - 1 ) overflows if x is zero.


Find all strings matching a giving regex pattern in files in a directory (including all subdirectories) #! python3
#   `regexSearch`: Finds all lines matching a given regex in each file in a given folder.
#   Usage:
#       The directory to search and regex to be searched for are provided as a command line arguments.
#       The 1st and 2nd command line arguments are the directory and regex pattern respectively.
#       Script prompts the user to enter the regex.
#       After completion, the user is prompted to continue

import re, sys
from os import path, listdir

def regex_search(regex, directory):
    res, lst = {}, listdir(directory)
    for itm in lst:
        pth = path.join(path.abspath(directory), itm)
        if path.isdir(pth):   res.update(regex_search(regex, pth))    #Recursively traverse all sub directories.
        else:
            print(pth)
            with open(pth) as file:
                tmp = []
                for idx, line in enumerate(file.readlines()):
                    results = regex.findall(line)
                    if results:  tmp.extend([f"Line {idx+1}: {results}"])
                res[pth] = tmp
    return res

if __name__ == "__main__":
    directory, pattern = sys.argv[1:3]
    while not path.isdir(directory):
        print("Error: Please input a valid path for an existing directory:", end = "\t")
        directory = input()
    while True:
        try:
            regex = re.compile(pattern)
            break
        except TypeError:
            print("Error: Please input a valid regex:", end = "\t")
            pattern = input()
        except re.error:
            print("Error: Please input a valid regex:", end = "\t")
            pattern = input()
    matches = regex_search(regex, directory)
    for key in matches:  print(key, "\n".join(matches[key]), sep="\n", end="\n\n")

Some improvements

Style

Please indent your file properly, since indentation is important in Python, those lines like

if path.isdir(pth):   res.update(regex_search(regex, pth))

Are frowned upon, instead do

if path.isdir(pth):
    res.update(regex_search(regex, pth))

Use glob for listing files in a directory

With Python3.5+ glob is the easiest way to list all files in a directory and subdirectory, before you should use os.walk()
Use generators when appropriate

This will save some memory space, as it doesn't have to append to the temporary list all the time
Use argparse over sys.argv[]

Argparse is the module for CLI input, easy to use and has a ton of features I definitely recommend it!

Code

import argparse
import glob
import re
import os
import pathlib

def regex_search(regex, directory):
    for f in glob.glob(f"{directory}**/*.*", recursive=True):
        with open(f) as _file:
            for i, line in enumerate(_file.readlines()):
                if regex.search(line):
                    yield f"In file {f} matched: {line.rstrip()} at position: {i}"

def parse_args():
    parser = argparse.ArgumentParser(
        usage='%(prog)s [options] <regex> <directory>',
        formatter_class=argparse.RawDescriptionHelpFormatter
    )
    parser.add_argument('regex', type=str)
    parser.add_argument('directory', type=str)
    args = parser.parse_args()

    try:
        rgx = re.compile(args.regex)
    except Exception as e:
        parser.error('Regex does not compile')
    directory = pathlib.Path(args.directory)
    if not os.path.isdir(directory):
        parser.error('Directory is not valid')
    return rgx, directory

if __name__ == '__main__':
    regex, directory = parse_args()
    for match in regex_search(regex, directory):
        print(match)

Bonus Round!

grep is a Unix tool that can basically do this by default

grep -Hrn 'search term' path/to/dir

Where:

-H prints the matching line
-r Does a recursive search
-n prints the line number