Analyze huge set of sentences for word presence I have a huge file (1Gb) with English sentences and I need to filter only those containing the Alice word.
Actual tests could be more complex, e.g. matching a verb by its wordforms (go, goes, gone, went, going).
To solve the task I have designed a method, which feeds English word as it is ready to a consumer and if test returns back positive the method returns immediately.
I have improved performance twice in comparison with storing words in a list first and matching them afterwards. Still I have the following benchmarks:

read:  1.6s / 100MB;
process: 1.5s / 100MB.

Could this code be improved further:
import java.util.function.Function;

public class ExtractEnglishWordsAndTest {
    public static boolean extractEnglishWordsAndTest(String text, Function<String, Boolean> consumer) {
        if (text == null || text.isEmpty()) {
            return false;
        }

        char[] buf = new char[text.length()];
        int bufIndex = -1;

        boolean isEnglishPiece = isEnglishLetterOrHyphen(text.charAt(0));

        for (char ch : text.toCharArray()) {
            boolean isEnglishLetter = isEnglishLetterOrHyphen(ch);

            if (isEnglishPiece && !isEnglishLetter || !isEnglishPiece && isEnglishLetter) {
                if (isEnglishPiece) {
                    if (consumer.apply(new String(buf, 0, bufIndex + 1))) {
                        return true;
                    }
                }

                isEnglishPiece = !isEnglishPiece;

                bufIndex = -1;
            }

            bufIndex++;
            buf[bufIndex] = ch;
        }

        if (isEnglishPiece) {
            if (consumer.apply(new String(buf, 0, bufIndex + 1))) {
                return true;
            }
        }

        return false;
    }

    public static boolean isEnglishLetterOrHyphen(char ch) {
        return ch >= 'a' && ch <= 'z' || ch >= 'A' && ch <= 'Z' || ch == '-';
    }

    public static void main(String[] args) {
        // could be used for just splitting
        ExtractEnglishWordsAndTest.extractEnglishWordsAndTest("Some key-phrases that may hint to you " +
                "that the question is better suited for Code-Review " +
                "are like the following:", (word) -> {
            System.out.print(word + " ");
            return false;
        });

        System.out.println();

        System.out.println("test: " + ExtractEnglishWordsAndTest.extractEnglishWordsAndTest("In another moment down went Alice after it, " +
                "never once considering how in the world " +
                "she was to get out again.", (word) -> word.equals("Alice")));

        System.out.println("test: " + ExtractEnglishWordsAndTest.extractEnglishWordsAndTest("Presently she began again.",
                (word) -> word.equals("Alice")));
    }
}

Output:

Some key-phrases that may hint to you that the question is better
suited for Code-Review are like the following
test: true
test: false

The logic of word extraction seems convoluted. Consider two loops instead, along the lines of

    int text_length = text.length();
    int i = 0;
    while (true) {
        while ((i < text_length) && !isEnglishLetterOrHyphen(text.charAt(i))) {
            i++;
        }

        int wordStart = i;
        while ((i < text_length) && isEnglishLetterOrHyphen(text.charAt(i))) {
            i++;
        }

        if (consumer.apply(text.substring(wordStart, i))) {
            return true;
        }
    }

BTW,

    isEnglishPiece && !isEnglishLetter || !isEnglishPiece && isEnglishLetter

is a long (and unclear) way to say

    isEnglishPiece != isEnglishLetter


Cancelable AsyncLazy that is thread-safe I've been thinking about the AsyncLazy<T> class that Stephen Toub and Stephen Cleary present in various resources such as this.  I am trying to find a way to integrate cancellation with AsyncLazy<T>.  Microsoft has a version of AsyncLazy<T> in their Visual Studio 2019 SDK that offers a GetValueAsync(CancellationToken) method, but if you read the fine print, it says:

  Note that this will not cancel the value factory (since other callers
  may exist).

I would like something that does cancel the value factory.  After failing to get anywhere with my research, I decided to attempt my own which is shown in the code below.  However, I'm having trouble with thread safety, as I don't want multiple threads to execute the long-running value factory. I want any thread that comes in behind the first one to essentially await the completion of that first thread, and then the subsequent thread would have the trivial task of just returning the already-created value.

Here is the code:

public sealed class CancelableAsyncLazy<T>
{
    private readonly Func<CancellationToken, Task<T>> _valueFactory;
    private T _createdValue;
    private bool _isValueCreated = false;
    private volatile bool _shouldWait = false;

    public CancelableAsyncLazy(Func<CancellationToken, Task<T>> valueFactory)
        => _valueFactory = valueFactory;

    public async Task<T> GetValueAsync(CancellationToken cancellationToken)
    {
        await Task.Run(() =>
        {
            while (_shouldWait) Thread.Sleep(1);
        });

        _shouldWait = true;

        try
        {
            if (_isValueCreated) return _createdValue;
            cancellationToken.ThrowIfCancellationRequested();
            _createdValue = await _valueFactory(cancellationToken).ConfigureAwait(false);
            _isValueCreated = true;
            return _createdValue;
        }
        finally
        {
            _shouldWait = false;
        }
    }
}

I don't like the fact that I'm using Task.Run and Thread.Sleep to wait for the value to be created by an earlier thread, but I can't think of a better way to do it.  The key is that it shouldn't block.  I tried using a Mutex, but it's possible the same thread could call the method before the value is created, so the subsequent call (from the same thread) would just blow past the Mutex.

Specification Considerations

Note that the first caller that gets access is the one that provides the cancellationToken for the valueFactory. Other callers use their own cancellationToken for timing out on awaiting the result of the async operation. This is how you designed your class. I did not change this part of the behavior.

If this behavior is not wanted, consider storing a cancellation token through the constructor. This one is used solely for the factory. And each call to GetValueAsync could then use its own cancellation token to time out, without influencing possible cancellation of the factory. This is a design decision you would have to make.

Review

This is not a good solution. You know it, we all know it :-)

await Task.Run(() =>
{
    while (_shouldWait) Thread.Sleep(1);
});

The way to delay a thread the async way is to use Task.Delay (Overview TAP Guidelines). This is already better. But as you mentioned, this still feels like a hack (and it is). Using some sort of Mutex would be a better approach, as you rightfully indicated. We'll get to that.

Lets' walk through your code.

You define 2 similar variables _isValueCreated and _shouldWait. If you think about it, they have the same purpose. As long the value is not created, a caller should wait. Let's merge them into a variable named .. let's say _memoized. This is common terminology in caching.

private bool _isValueCreated = false;
private volatile bool _shouldWait = false;

Rather than the whole construct of abusing a thread's Sleep and using a volatile bool to simulate a lock, we could use a Semaphore (Async Mutex using a Semaphore).

As discussed in the previous link, this is not possible..

// example code
lock (lockObject)
{
    await Task.Delay(1000);
}

.. but this is..

// example code
await semaphoreSlim.WaitAsync();
try
{
    await Task.Delay(1000);
}
finally
{
    semaphoreSlim.Release();
}

Let's see how we can implement this in your cancellable async lazy class.

Refactoring the Code

We define our variables. Your two booleans are merged into one. And we use the semaphore described above.

private readonly Func<CancellationToken, Task<T>> _valueFactory;
private volatile bool _memoized;
private readonly SemaphoreSlim _mutex;
private T _value;

The constructor is straight-forward. Make sure to use a Semaphore than emulates a Mutex so one thread has access to the resource simultaneously.

public CancelableAsyncLazy(Func<CancellationToken, Task<T>> valueFactory)
{
    _valueFactory = valueFactory ?? throw new ArgumentNullException(nameof(valueFactory));
    _mutex = new SemaphoreSlim(1, 1);
}

The infamous GetValueAsync can then be rewritten completely. Each caller thread can provide its own cancellationToken. Use it to acquire a lock on _mutex. Once we have the lock, check whether _value is already memoized. If not (!_memoized) execute _valueFactory, and memoize the value. Now, we perform another timeout check for the calling thread ThrowIfCancellationRequested. Even though we have the value available now, the caller might still have timed out, so let him now. Don't forget to release the mutex.

public async Task<T> GetValueAsync(CancellationToken cancellationToken)
{
    await _mutex.WaitAsync(cancellationToken);
    try
    {
        if (!_memoized)
        {
            _value = await _valueFactory(cancellationToken).ConfigureAwait(false);
            _memoized = true;
            cancellationToken.ThrowIfCancellationRequested();
        }
        return _value;
    }
    finally
    {
        _mutex.Release();
    }
}

We should allow for a convenience overload if no cancellation support is required for a given caller.

public async Task<T> GetValueAsync() => await GetValueAsync(CancellationToken.None);

And since we comply to the concept of Lazy we should also provide a synchronous property Value.

public T Value => GetValueAsync().Result;

Refactored Code

public sealed class CancelableAsyncLazy<T>
{
    private readonly Func<CancellationToken, Task<T>> _valueFactory;
    private volatile bool _memoized;
    private readonly SemaphoreSlim _mutex;
    private T _value;

    public CancelableAsyncLazy(Func<CancellationToken, Task<T>> valueFactory)
    {
        _valueFactory = valueFactory
            ?? throw new ArgumentNullException(nameof(valueFactory));
        _mutex = new SemaphoreSlim(1, 1);
    }

    public T Value => GetValueAsync().Result;

    public async Task<T> GetValueAsync() => await GetValueAsync(CancellationToken.None);

    public async Task<T> GetValueAsync(CancellationToken cancellationToken)
    {
        await _mutex.WaitAsync(cancellationToken);
        try
        {
            if (!_memoized)
            {
                _value = await _valueFactory(cancellationToken).ConfigureAwait(false);
                _memoized = true;
                // at this point, the value is available, however, the caller
                // might have indicated to cancel the operation; I favor
                // checking for cancellation one last time here, but you
                // might decide against it and return the result anyway
                cancellationToken.ThrowIfCancellationRequested();
            }
            return _value;
        }
        finally
        {
            _mutex.Release();
        }
    }
}


Split a number into equal parts given the number of parts I want to split a song into multiple parts, given the song duration and number of parts.

My code achieves that, but it feels a little "stupid" and I would like to learn a more sophisticated - and shorter - way. Particularly, I feel that the marker variable is a little overkill. I would welcome any suggestions.

song_duration = 20 # these two
num_of_parts = 4   # are given

part_duration = song_duration / num_of_parts
parts = []
marker = 0

for _ in range(num_of_parts):
    part = [marker, marker + part_duration]
    marker += part_duration
    parts.append(part)

print(parts)

# parts is : [[0, 5.0], [5.0, 10.0], [10.0, 15.0], [15.0, 20.0]]

In general, building a list using a loop of the form

some_list = []
for …:
    some_list.append(…)

… would be better written using a list comprehension.

Each interval always has two elements: a start time and an end time.  These two-element lists would be better represented as tuples instead of lists.  (Tuples have a connotation that they have a fixed length, whereas lists can grow to arbitrary lengths.)

Finally, I'd package the code into a function.

def intervals(parts, duration):
    part_duration = duration / parts
    return [(i * part_duration, (i + 1) * part_duration) for i in range(parts)]


Optimizing jQuery list filtering Scenario

Currently filtering through a list of ~3100 rows of data presented to the end-user in the form of an index (think, list of car models).

This is done in such a way that every item is available in a long list to the end-user however they have the possibility to filter down their results into something more relevant to them.

I have gone through three variations of my filtering functionality which has greatly improved, however the fact that my jQuery skills lack experience, was wondering whether or not my implementation can be improved in the following ways:

Readability
Performance

var lis = $('#some-list .some-entry');
var list = $.makeArray(lis.map(function(k, v) {
    return $(v).text().toLowerCase();
}));

var filterDelay = (function() {
    var timer = 0;
    return function(callback, ms) {
        clearTimeout(timer);
        timer = setTimeout(callback, ms);
    };
})();

$('#filter').keyup(function() {
    var userInput = $(this).val();

    filterDelay(function() {
        var count = 0;
        lis.each(function(index, value) {
            if (list[index].indexOf(userInput) >= 0) {
                count++;
                $(value).show();
            } else {
                $(value).hide();
            }
        });

        if (count == 1) {
            $("#filter-results").text(count + " Result");
        } else {
            $("#filter-results").text(count + " Results");
        }
    }, 250);
});

The reason for the callback functionality is so that jQuery/JS does not start filtering through ~3100 rows of data every time the user presses a key. Instead, we want to wait for 250ms and then fall back to our filtering functionality afterwards.

This greatly helps the user-experience and is overall less stressing on the browser.

Question

My previous examples used toggle(bool) rather than show() or hide() to add or remove a row from the list (when filtered). Are there any inherent differences in terms of performance between the two?

Additionally, I am looking to implement a loading animation when:

The user is typing
The filter has starting filtering

And the removing it when the filtering is complete. Thus, would it make sense to toggle the loading animation, or add/remove it from the DOM respectively?

Toggle it off would mean that the animation is still occurring in the background regardless whether or not any filtering is being made. Adding it or removing to the DOM however will have its own performance drop.

What would one recommend? I don't want to make it seem like it's for micro-optimization purposes but simply looking to implement my filter functionality in a neater way for possible future re-use.

Update(s)

Added load animations by combining Bootstrap and jQuery like so:

$('#filter').keyup(function() {
    //...
    $('#filter-icon-load').removeClass('hidden');
    //...
    $('#filter-icon-load').addClass('hidden');
    //...
});

The root of the problem is you have ~3100 items in the page. This could be simplified by using pagination. If this is a traditional page, page the output. If this is a webapp, you may have all ~3100 in memory, but don't render it all. Besides, there's no point rendering ~3100 items unfiltered anyways. Who scrolls through ~3100 items?

Granted that you don't have any other option but to render it all and filter on the spot, the next problem with your code is that it generates a lot of jQuery objects. Every time you call the $() function, a jQuery object is generated.

// Here, the $(v)
var list = $.makeArray(lis.map(function(k, v) {
    return $(v).text().toLowerCase();
}));

// Here, the $(value)
lis.each(function(index, value) {
    if (list[index].indexOf(userInput) >= 0) {
        count++;
        $(value).show();
    } else {
        $(value).hide();
    }
});

One way to avoid this is to collectively do the show and hide instead of doing it per item. In the following code, it gathers all elements to show and to hide, wraps both arrays in jQuery and calls show and hide.

var toShow = [];
var toHide = [];
lis.each(function(index, value){
  if (list[index].indexOf(userInput) >= 0) {
    toShow.push(this);
  } else {
    toHide.push(this);
  }
});

$(toShow).show();
$(toHide).hide();

Notice that I only called $() twice, and .show() and .hide() once instead of per item. We just avoided creating ~3100 jQuery objects!

Another way to improve this code is to use native operations whenever possible. It's not that they're faster by themselves, but you reduce reliance to jQuery, avoiding creation of jQuery objects when ordinary arrays would have sufficed.

var list = $.makeArray(lis.map(function(k, v) {
  return $(v).text().toLowerCase();
}));

This code above could have been simplified into

var list = lis.get().map(function(element){
  return (element.innerText || element.textContent).toLowerCase();
});

In the "improved version", notice that 1) it uses a native array instead of jQuery in the first place, allowing the use of the native array.map and 2) it doesn't call $() in the callback at all. We just avoided ~3100 jQuery objects again!

Now back to your toggling code, I think I see an update you did before my answer by adding/removing classes. That's a good move because 1) styling should be done in CSS and 2) show and hide uses inline styles. They are hard to override from a stylesheet. The only way you could is to use !important which you should generally avoid.

As for your question about removal vs hiding, I'd say go for hiding. As far as I know, removals are slow. This is even worse when there are stuff attached to the elements in question. For instance, removing an element with jQuery handlers attached. jQuery has to clean them up, remove references to data and handlers etc. If removals aren't done properly, you could have a lot of lingering things in memory.


Hackerearth - Learning Graph Problem:

  Monk goes to graph city to learn about graphs, and meets an
  undirected graph having  \$ N \$ nodes, where each node has a value \$ i \$ such that  \$ 1 \le i \le N \$. Each node of the graph is very
  curious and wants to know something about the nodes which are directly
  connected to it.

  For each node, if we sort the nodes directly connected to it, according to their values in descending order, what is the value of the node at the \$ k \$ th position? The graph is 1-based.

  Note: If two nodes have the same value, they are sorted according to their indices in ascending order.

My accepted solution:

import sys
from collections import defaultdict

node_value_dict = defaultdict(int)
connected_graph_component = defaultdict(list)

def connect_graph():
    n, m, k = list(map(int, sys.stdin.readline().split()))
    node_values = list(map(int, sys.stdin.readline().split()))
    for i in range(1, len(node_values) + 1):
        node_value_dict[i] = node_values[i - 1]

    for m_i in range(m):
        n1, n2 = list(map(int, input().split()))
        connected_graph_component[n1].append(n2)
        connected_graph_component[n2].append(n1)

    for i in range(1, n + 1):
        if i in connected_graph_component:
            curr_co = connected_graph_component[i]
            sort_list = sorted(curr_co, key=lambda value: node_value_dict[
                               value], reverse=True)
            for i in range(len(sort_list) - 1):
                if node_value_dict[sort_list[i]] == node_value_dict[sort_list[i + 1]] and sort_list[i] < sort_list[i + 1]:
                    sort_list[i], sort_list[
                        i + 1] = sort_list[i + 1], sort_list[i]

            print(sort_list[k - 1])
        else:
            print(-1)

connect_graph()

With a runtime of 9.14092 sec.

How can I improve the code quality and further optimize runtime duration?

In case of equal values, I need to sort the nodes according to their indices in ascending order. Currently I'm doing it by looping over the sorted list and then comparing values again side by side maintaining two pointers.

Can you please suggest some elegant way of doing this.

Question Link

As for runtime duration, there is not really much you can do, as the algorithm is already optimum.

However, code can be simplified a bit. Here are some tips:

node_value_dict has same data as node_values -> not needed
Use input() instead of sys.stdin.readline().split() so you can avoid an import
Is there a need for connected_graph_component to be a dict? It should be fine as an array of arrays
As for the sorting, note that you can make the key a pair instead of only 1 value and it will sort first by first value, and if equal it sorts according to second value.
list(map(int, sys.stdin.readline().split())) is repeated multiple times, put it in a function
Sometimes batching print calls in 1 call instead of printing for each value is faster

This is my version of the code after applying these tips:

    def read_line_values():
        return list(map(int, input().strip().split(' ')))

    def connect_graph():
        n, m, k = read_line_values()
        node_values = read_line_values()

        connected_graph_component = [[] for _ in range(n + 1)]

        answer = []

        for m_i in range(m):
            n1, n2 = read_line_values()
            connected_graph_component[n1].append(n2)
            connected_graph_component[n2].append(n1)

        for i in range(1, n + 1):

            neighbours = connected_graph_component[i]
            if len(neighbours) < k:
                answer.append("-1")
            else:
                neighbours.sort(key = lambda node: (node_values[node - 1], node), reverse=True)
                answer.append(str(neighbours[k - 1]))

        print("\n".join(answer))
    connect_graph()

On a side note, if you run the same code as python 2.7.6 instead of 3.5.2, it´s about 2s faster.


Functional testing issue - Feels like one long test I'm using Selenium to test a PHP site that goes through the following process:

Register user
Log in
Fill out Form 1 and submit
Fill out Form 2 and submit
Fill out Form 3 and submit
(tear down) Delete user

A real user would go through these steps in this order.

I've read in places that tests should be bite-sized, and to avoid asserting a lot of things in each test. So I've written my tests like this:

function testRegister() {
    //...
}

/**
 * @depends testRegister
 */
function testLogin() {
    //...
}

/**
 * @depends testLogin
 */
function testFillForm1() {
    //...
}

/**
 * @depends testFillForm1
 */
function testFillForm2() {
    //...
}

/**
 * @depends testFillForm2
 */
function testFillForm3() {
    //...
}

This way my tests execute in the correct order.

However, splitting up my tests is merely cosmetic. Since each test is dependent on the success of the previous one, it may as well be one long test.

It also causes another problem. If testFillForm2 fails, I have no way of re-running the test on its own, I have to wait for all the other tests to complete each time because of the dependencies.

Is there a better approach I can take?

@depends does not define order

  [@depends] do not define the order in which the test methods are to be executed

Using @depends and not returning/passing arguments is an indicator that @depends isn't appropriate for what you're currently doing. You need to have a consistent application state before running each individual test method so that they do not depend on execution order. Otherwise tests easily become quite unwieldy and ultimately of little value.

If you want to do that via @depends, you need to return  from a test method and the following method should expect it as the first argument (and literally depend on whatever it receives).

Reduce dependencies

In tests you should aim to reduce, not add dependencies, otherwise you can be in quite a fragile position. If for example your register function breaks, it wouldn't affect existing users, or therefore all other tests, at all - but using cascading tests as shown you would be unable to test/demonstrate that.

A broken register function would certainly be a problem - but is it a "100% of the site is broken" problem? It makes test results un-representative, and potentially harder to identify the root cause. If you have one failing test - it's very obvious where to focus your attention just from the test results. If all tests fail, it is not.

Of course anything more than of 0% fails warrants developer attention.

Achieve consistent application state

So, for example, each test should:

Establish a consistent application state (setUp functionality)
Use the concept of fixtures to e.g. populate the session, cookies, db - whatever is required
test what it needs to test
cleanup after itself (tearDown functionality)

In Context, I would say that means:

Create a test user to be able to test logging in without first being forced in the test case to "test" your register function:

public funtion testLogin() {
    // Insert a user directly into the users table - simulate anything else from testRegister
    ...
    $this->assertSame("UserName", $_SESSION['user']['name']);
}

You setup the session/cookie/whatever state for each subsequent test so that you don't need to "test" your login function to be able to test other authorized functions:

public funtion testFillForm1() {
    $this->_login();
    ...
}

public funtion testFillForm2() {
    $this->_login();
    ...
}

public funtion testFillForm3() {
    $this->_login();
    ...
}

protected function _login($user = array()) {
    if (!$user) {
        $user = array('name' => 'Default');
    }
    $_SESSION['user'] = $user;
}

If any of your setup steps are generic enough to apply to all tests, put them in the setUp function. E.g. loading 10 test users, with various permissions - put that in setUp and therefore testing login, registration-"that username already exists" would need no in-test setup at all.

I'm not exactly sure how that applies to using selenium, but hopefully this gives you enough of a hint to remove some of your inter-test dependencies.


Determine whether a vector intersects the up or down unit vector I wrote an if statement that checks if an iteratively altered vector "crosses" the up vector (0,1,0) or the down vector (0,-1,0).

if ((lastDiff.x() > 0 && diff.x() < 0 && lastDiff.z() >= 0 && diff.z() <= 0) ||
    (lastDiff.x() < 0 && diff.x() > 0 && lastDiff.z() >= 0 && diff.z() <= 0) ||
    (lastDiff.x() > 0 && diff.x() < 0 && lastDiff.z() <= 0 && diff.z() >= 0) ||
    (lastDiff.x() < 0 && diff.x() > 0 && lastDiff.z() <= 0 && diff.z() >= 0) ||
    (lastDiff.z() > 0 && diff.z() < 0 && lastDiff.x() >= 0 && diff.x() <= 0) ||
    (lastDiff.z() < 0 && diff.z() > 0 && lastDiff.x() >= 0 && diff.x() <= 0) ||
    (lastDiff.z() > 0 && diff.z() < 0 && lastDiff.x() <= 0 && diff.x() >= 0) ||
    (lastDiff.z() < 0 && diff.z() > 0 && lastDiff.x() <= 0 && diff.x() >= 0))

I feel like there is a simpler or better way to implement this if guard but I can't find it.

You have four groups of repeating conditions:

bool crossX = (lastDiff.x() < 0 && diff.x() > 0) || (lastDiff.x() > 0 && diff.x() < 0);
bool crossZ = (lastDiff.z() < 0 && diff.z() > 0) || (lastDiff.z() > 0 && diff.z() < 0);
bool crossTouchX = (lastDiff.x() <= 0 && diff.x() >= 0) || (lastDiff.x() >= 0 && diff.x() <= 0);
bool crossTouchZ = (lastDiff.z() <= 0 && diff.z() >= 0) || (lastDiff.z() >= 0 && diff.z() <= 0);

Using these the original condition can be simplified to:

if ((crossX && crossTouchZ) || (crossZ && crossTouchX))
{
    // do stuff
}


Writing to a csv file in a customized way using scrapy I've written a script in scrapy to grab different names and links from different pages of a website and write those parsed items in a csv file. When I run my script, I get the results accordingly and find a data filled in csv file. I'm using python 3.5, so when I use scrapy's built-in command to write data in a csv file, I do get a csv file with blank lines in every alternate row. Eventually, I tried the below way to achieve the flawless output (with no blank lines in between). Now, It produces a csv file fixing blank line issues. I hope I did it in the right way. However, if there is anything I can/should do to make it more robust, I'm happy to cope with.

This is my script which provides me with a flawless output in a csv file:

import scrapy ,csv
from scrapy.crawler import CrawlerProcess

class GetInfoSpider(scrapy.Spider):
    name = "infrarail"

    start_urls= ['http://www.infrarail.com/2018/exhibitor-profile/?e={}'.format(page) for page in range(65,70)]

    def __init__(self):
        self.infile = open("output.csv","w",newline="")

    def parse(self, response):
        for q in response.css("article.contentslim"):
            name = q.css("h1::text").extract_first()
            link = q.css("p a::attr(href)").extract_first()
            yield {'Name':name,'Link':link}

            writer = csv.writer(self.infile)
            writer.writerow([name,link])

c = CrawlerProcess({
    'USER_AGENT': 'Mozilla/5.0',
})
c.crawl(GetInfoSpider)
c.start()

Btw, I used .CrawlerProcess() to be able to run my spider from sublime text editor.

I'd like to mention, that there is a special way of making output files in scrapy - item pipelines. So, in order to make it right, you should write your own pipeline (or modify standard one via subclassing).

Also, you does not close the file, once you're done and you keep it open most of the time. The both problems are handled nicely with pipelines.

UPD: Well, you've asked for a better way, there it is. Although, if it's not acceptable for some hard-to-explain reasons (it's understandable), here's the other approach, how to make it better.

Don't leave the file open. There is a method (__del__()) which destroys the spider object. Add the code to close the file before it.
Another one is to store only the filename in the variable and open / close the file each time you write into it.
Another option is to use NoSQL database, which does not need to be opened / closed. And after scraping is done - get the output file from it.
If you have a few values to scrape you can store it in object variable, and then export it before __del__() method.

All the ways above are NOT welcomed by the actual developer community and may lead to serious problem in future. Use them carefully. Sometimes it's easier (in the long run) to read and understand how it really should be done.

Maybe it's the exact case?


C++(11/14) intercalate implementation I wrote a simple generic intercalate function (that should be semantically equivalent to the Haskell intercalate).

template <typename Fwd_It, typename Out_It>
    auto intercalate(Fwd_It first, Fwd_It last, Out_It destFirst,
                     Fwd_It whatFirst, Fwd_It whatLast) {
        if (first == last) return destFirst;
        *destFirst = *first;
        ++first;
        ++destFirst;

        while (first != last) {
            std::copy(whatFirst, whatLast, destFirst);
            *destFirst = *first;
            ++first;
        }
        return destFirst;
    }

It appears to be working with what I've tested with (empty ranges, different iterator-supporting ranges, different types, etc.).

However, is this:

generic enough?
clean enough?
fast enough?

I would not cram in the if statement like that:

auto intercalate(Fwd_It first, Fwd_It last, Out_It destFirst,
                 Fwd_It whatFirst, Fwd_It whatLast) {
    if (first == last) return destFirst;
    *destFirst = *first;
    // ...

While this style is okay at times, it may not be too readable in all situations.  Single-line statements can still use curly braces, especially if it may need to be maintained.  You could also add a new line before the line that follows it.

auto intercalate(Fwd_It first, Fwd_It last, Out_It destFirst,
                 Fwd_It whatFirst, Fwd_It whatLast) {
    if (first == last) {
        return destFirst;
    }

    *destFirst = *first;
    // ...


Blocking and async UDP communication with an IR camera I'm writing a Java application to control a custom IR research camera that will be installed on an aircraft.  The camera connects to the the computer using UDP sockets via a direct LAN cable. The camera does not initiate any communications; it just replies to commands, 5-character strings, sent to it (the camera code is in C BTW).  There are about 16 commands most of which reply more or less instantly with the string ACKNO if the command is recognized, 3 reply with very short data strings, and the last replies with 480 packets of 640 bytes: the image data.

My intent is to send a command and either get the reply buffer(s) or a time out. It must be blocking.

I last worked on this code about 5 months ago and then had to put it on hold while I wrote the interface for the user and the camera was still being built.  I'm self taught in Java (my title is Ecologist).  I've included code for sending a command and waiting for an ACKNO or time out response.  It appears complicated and somewhat verbose to me so I'd like someone else to look at it and comment it or point me in a better direction.  I'm using Java 8.

How it might be used in a method:

if (!waitForACK(waitMS, filter).isSuccess) {
    // do something
    return "Failed to receive ACK.";
}

Top code:

private SendAndWaitForACK sendAndWaitForACK = null;
sendAndWaitForACK = new SendAndWaitForACK();

public Result waitForACK(int waitMS, CameraCommands cmd) {
    result.isSuccess = false;
    result.dataBytebuffer = ackByteBuffer;
    sendAndWaitForACK.setCommand(cmd); // set the comand
    Future<?> future = executor.submit(sendAndWaitForACK);
    try {
        result = (Result) future.get(waitMS, TimeUnit.MILLISECONDS);
    } catch (Exception e) {

    } finally {

    }

    return result;
}

private class SendAndWaitForACK implements Callable<Result> {
    private CameraCommands cmd;

    @Override
    public Result call() throws Exception {
        // does not return on timeout
        result.dataBytebuffer = postCommand(cmd);
        result.isSuccess = true;
        return result;
    }

    /**
     * setCommand must be called prior to use.
     *
     * @param cmd
     *            : CameraCommands
     */
    public void setCommand(CameraCommands cmd) {
        this.cmd = cmd;
    }
}

This is the method that actually deals with the socket.

/**
 * postCommand configures the ackByteBuffer prior to writing it to the
 * camera. It is invoked by postCommand call method.
 *
 * @return ackByteBuffer : tByteBuffer The image.
 * @throws IOException
 *
 */
    private ByteBuffer postCommand(CameraCommands command) throws IOException {
    ackByteBuffer.rewind();
    writeCommand(command); // to UDP Socket
    // blocking read reply from UDP Socket
    datagramChannel.read(ackByteBuffer);
    return ackByteBuffer;

}

Constructors and Generics
It is customary to use constructors to set values for instances in Java. This code:

private class SendAndWaitForACK implements Callable<Result> {
    private CameraCommands cmd;

    @Override
    public Result call() throws Exception {
        // does not return on timeout
        result.dataBytebuffer = postCommand(cmd);
        result.isSuccess = true;
        return result;
    }

    /**
     * setCommand must be called prior to use.
     *
     * @param cmd
     *            : CameraCommands
     */
    public void setCommand(CameraCommands cmd) {
        this.cmd = cmd;
    }
}

Would more typically be written as:
private static class SendAndWaitForACK implements Callable<Result> {
    private final CameraCommands cmd;

    SendAndWaitForACK(CameraCommands cmd) {
        this.cmd = cmd;
    }

    @Override
    public Result call() throws Exception {
        // does not return on timeout
        result.dataBytebuffer = postCommand(cmd);
        result.isSuccess = true;
        return result;
    }

}

Note that I have replaced the setter with the constructor. I have also made the class static, which saves a tiny amount of memory, but it is good practice because it makes Java GC easier, and has some other benefits.
You will need to change the way you use that class, and your calling code will change from:

Future<?> future = executor.submit(sendAndWaitForACK);

to
Future<Result> future = executor.submit(new SendAndWaitForACK(cmd));

Note, the SendAndWaitForACK implements Callable<Result>, so it will produce a Future<Result> when submitted. As a result, I have changed the generic type of the Future to Future<Result>. This also means that you no longer need the explicit cast and it can simply be:
result = future.get(waitMS, TimeUnit.MILLISECONDS);

Network
Note that your class does not do a clean handling of the network. When you submit a Callable you start a thread, and you run the Callable on that thread. Then, in your code, you set a timeout to wait for the Callable to complete. If the callable does not complete within that specified time, you get a timeout. This does not stop the Callable from running in the other thread. All that happens is that the callable keeps running, and eventually it gets something back from the camera... and, what it gets back, may be something belonging to another command sent later.
Fundamentally, your protocol/communication is a little flawed in this regard. As long as things work as expected, you won't notice it, but, if things go screwy, then your code will make the screwy situation even more screwy.
I strongly recommend, in cases like this, to not use threading at all. At least, don't use multiple threads to communicate over a single socket.
One of the benefits of UDP is the easy way to build simple protocols on top of it.
If this were me, I would:

have a single class that manages the socket, and protocol.
have a thread on that class that checks periodically to see that the camera is there, and working (perhaps every second or so, a 'heartbeat').
that class has a ConcurrentBlockingQueue on the input side that takes a SendAndWaitForACK
(I would probably make the 'heartbeat' one of these SendAndWaitForACK instances)
When a callable is queued, the class will give that command a unique number, and it will 'transmit' that command every 10 or so milliseconds, until it gets a response, or until the timeout happens
when it gets the response (or timeout), it will notify the caller.

I would make the camera-side of the system return the unique ID for the command as part of the response. That way you can make sure you are getting the right data back.
Doing things this way means that:

only one thread accesses the network socket.
only one command can be 'in transit' at any one time.
you do not need to worry about threading or concurrency on the socket.
you always have a 'state' of the camera.

Obviously, that recommendation is very different to what you have at the moment.... but, bottom line, is that almost all UDP-based communication channels will have some form of simple protocol established above the raw network layer.


Assign values to answers from a questionnaire, input into a formula and calculate score I was wondering if anyone could make any suggestions to how I could write the following more concisely and reduce the number of IF statements? If possible, I would also like to simultaneously assign values to variables and calculate the score.

I will eventually have multiple rule sets and formula scores for other question sets.

# q001 is multiple choice with three choices - q001_a01, q001_a02 & q001_a03 - only one selection can be made
# q002 is the first part of a two part question. It has two options = q002_a01 & q002_a02 - again, only one selection can be made
# q003 is only answered if q002_a01 is selected in q002 and also has two choices - q003_a01 & q003_a02
# q004 is a multiple choice question with 5 options - q004_a01, q004_a02, q004_a03, q004_a04 & q004_a05 - multiple selections can be made, q004_a05 answer is 'none of the above'
# q005 is a simple Yes/No (True/False) question
# q006 is multiple choice with three choices - q006_a01, q006_a02, q006_a03 & q006_a004 - only one selection can be made
# q007 is multiple choice with three choices - q007_a01, q007_a02 & q007_a03 - only one selection can be made
# q008 requires an input of an integer from 1-5
# all questions require a response (except q003)

# dictionary containing question and answer codes for a single individuals response
data = {'q001': 'q001_a01',
        'q002': 'q002_a01',
        'q003': 'q003_a02',
        'q004': ['q004_a01', 'q004_a02', 'q004_a04'],
        'q005': False,
        'q006': 'q006_a03',
        'q007': 'q007_a01',
        'q008': 3}

# rules which define the values of the different variables
var1 = 2 if 'q001_a01' in data['q001'] else 0
var1 = 5 if 'q001_a02' in data['q001'] or 'q001_a03' in data['q001'] else var1

var2 = -2 if 'q002_a01' in data['q002'] and 'q003_a01' in data['q003'] else 0
var2 = 2 if 'q002_a01' in data['q002'] and 'q003_a02' in data['q003'] else var2

var3 = 2 if 'q004_a01' in data['q004'] else 0
var3 = var3 + 2 if 'q004_a02' in data['q004'] else var3
var3 = var3 - 1 if 'q004_a03' in data['q004'] else var3
var3 = var3 + 1 if 'q004_a04' in data['q004'] else var3
var3 = 5 if 'q004_a05' in data['q004'] else var3

var4 = 4 if True is data['q005'] else 0

var5 = 1 if 'q006_a01' in data['q006'] else 0
var5 = 2 if 'q006_a02' in data['q006'] else var5
var5 = -10 if 'q006_a03' in data['q006'] else var5
var5 = -2 if 'q006_a04' in data['q006'] else var5

var6 = 2 if 'q007_a01' in data['q007'] else 0
var6 = 1 if 'q007_a02' in data['q007'] or 'q007_a03' in data['q007'] else var6

var7 = data['q008']

# formula to calculate score from variables
score = var1 + var2 + var3 + var4 + ((var5 + var6) * var7)

I would like to make the code as neat and fast as possible.
Also, could someone point me towards some python literature which would help me improve this myself?

Since everything in this code is defined statically rather than as a function input (and your comments indicate that there is no way to vary any part of how this code functions), there is no possibility allowed within the parameters of this question for the score to ever vary.  You can therefore make this code significantly more concise and efficient by simply computing the score statically:

score = -15

This will produce the same score result as your original code with a significantly smaller footprint.

If this doesn't seem right, the question to ask yourself (and then answer for the benefit of your code reviewers) is: which constants in this code can vary, and what types/values are permissible for them?


Add multiples of 3 or 5 below 1000, Can this code be optimised. Project Euler #1 I have seen some solution here for the same problem, this and this I have found better ways to solve this problem. I just want to know how good or bad my code and way of approaching this problem is.

using System;

public class Program
{
    public static void Main()
    {
        int sum = 0;
        for (int i = 0; 3 * i < 1000; i++)
        {
            sum += 3 * i;

            if (5 * i < 1000 && (5 * i) % 3 != 0)
            {
                sum += 5 * i;
            }
        }
        Console.WriteLine(sum );
    }

The main problem

The main issue here is that you define i in function of it being a "factor of 3", and then also try to use this number as a "factor of 5". This doesn't make any sense - as there is no inherent relationship between being a threefold and a fivefold number.

It would've made sense if you were doing it for 3 and 6, for example, because 3 and 6 are related:

//i*3 is obviously always a multiple of 3
sum += 3 * i;

if ((3*i) % 2 == 0)
{
    //Now we know that i*3 is also a multiple of 6
}

But that is not the case here.

The for loop's readability

I understand your idea, you wanted to only iterate over factors of three which keep the multiple under 1000. While I will suggest to change this approach (later in the answer), your approach could've been written in a much more readable manner:

for( int i = 0 ; i < 1000 ; i+=3 )

This will iterate over i values of 0,3,6,9,12,15,... and will skip all values inbetween.

The benefit here is that you don't need to work with i*3 all the time, you can just use i itself.

You will need to iterate over the multiple of 5 separately. However, should you keep using this approach, I would always suggest splitting these loops anyway.

The algorithm

Your approach works, but it's not the easiest approach. If I put your approach into words:

  Add every multiple of 3 to the sum.

  Also add the multiple of 5, but only if it's still below 1000, and it's not already divisble by 3.

The issue here is in how you handle the multiples of five. You're working with an i that is defined as the allowed values for threefolds. For any i > 200, you're effectively having to manually exclude this value. You're using a different approach for the 5 than you are using for the 3, even though the logic is exactly the same. That's not a reusable approach.

Secondly, there is a readability problem. Your code should be trivially readable, and I simply wasn't able to understand your intention. I had to google what the question was before I could understand what your code was trying to achieve.

So let me offer a better approach, first putting it into words:

  Check every number from 0 to 1000 (not including 1000)
  If it is divisible by 3 or it is divisible by 5, then add it to the sum.

This can be put into code, step by step:

// Check every number from 0 to 1000 (not including 1000)
for(int i = 0; i < 1000; i++)
{
    var isDivisibleBy3 = i % 3 == 0;
    var isDivisibleBy5 = i % 5 == 0;

    //If it is divisible by 3 or it is divisible by 5
    if(isDivisibleBy3 || isDivisibleBy5)
    {
        //then add it to the sum
        sum += i;
    }
}

Note how the code exactly mirrors my algorithm description.

You don't need to use the booleans. I simply added them to simplify the example. if(i % 3 == 0 || i % 5 == 0) would be equally okay to use because it's still reasonably readable.

If the calculations become more complex, I suggest always using the booleans so you neatly break your algorithm down to small and manageable steps. It will do wonders for your code readability, and it does not impact performance (the compiler will optimize this in a release build).

A LINQ variation

This can be further shortened using LINQ:

var sum = Enumerable.Range(0,1000)
              .Where(i => i % 3 == 0 || i % 5 == 0)
              .Sum();

LINQ is just a nicer syntaxt, but it uses a for/foreach iteration in the background, so I suspect it won't be much more performant than the previous example. But I do consider this highly readable.

Maximizing performance

The previous suggestion maximizes readability, but it does so at the cost of performance, as it now has to loop over 1000 values and evaluate them all. You already linked several other answer that clearly dive deeper into the code in order to maximize the performance, I hope you can see that this dramatically impacts the readability.

For example:

public void Solve(){
    result = SumDivisbleBy(3,999)+SumDivisbleBy(5,999)-SumDivisbleBy(15,999);
}

private int SumDivisbleBy(int n, int p){
    return n*(p/n)*((p/n)+1)/2;
}

By itself, I would have no idea what this code does. I can sort of understand the intention of Solve(), but it's not quite apparent how SumDivisbleBy() works.

SumDivisbleBy() has effectively become impossible to maintain. If you needed to introduce a change, you would effectively have to reverse engineer it before you can alter it. This means that starting from scratch is the better option, which is clearly not a good thing.

However, when performance is the main focus, this is acceptable. I would, however, strongly urge you to document the algorithm in comments specifically to help future readers in understanding how/why this works.

Note that AlanT's answer contains a more readable variant of the SumDivisbleBy() method, which already helps a lot with understanding the algorithm. The clear naming used clarifies what the algorithm does, which is the main goal of writing readable code (explaining why something works is only a secondary goal and not always required).


Invoking worker threads I wrote this class to ensure that anything done in any of my worker threads can be displayed on the GUI via events, but I ran into non-thread-safe problems.

This class should take an action or a function with one or no params, i.e. the event delegate.

Is this code actually thread-safe, and, how could it be improved upon (simplifying the 4 bodies into two or one)?

The class is defined as:

private class ThreadSafeCall
{

    public void Call(Action action)
    {
        try
        {
            ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
            if (invokable != null && invokable.InvokeRequired == true)
            {
                invokable.Invoke(action, null);
            }
            else
            {
                action();
            }
        }
        catch (Exception)
        {

        }
    }

    public void Call(Action<object> action, object param)
    {
        try
        {
            ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
            if (invokable != null && invokable.InvokeRequired == true)
            {
                invokable.Invoke(action, new object[] { param });
            }
            else
            {
                action(param);
            }
        }
        catch (Exception)
        {

        }
    }

    public object Call(Func<object> action)
    {
        try
        {
            ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
            if (invokable != null && invokable.InvokeRequired == true)
            {
                return invokable.Invoke(action, null);
            }
            else
            {
                return action();
            }
        }
        catch (Exception)
        {

        }

        return null;
    }

    public object Call(Func<object, object> action, object param)
    {
        try
        {
            ISynchronizeInvoke invokable = action.Target as ISynchronizeInvoke;
            if (invokable != null && invokable.InvokeRequired == true)
            {
                return invokable.Invoke(action, new object[] { param });
            }
            else
            {
                return action(param);
            }
        }
        catch (Exception)
        {

        }

        return null;
    }
}

It is used:

(new ThreadSafeCall()).Call(() => { onCompleteHandle(); });

Your specific usage seems to be thread-safe. But it's relying on implementation details (about lambdas), which you shouldn't do. It's also extremely easy to get it wrong.

I believe the only way it will work is if you're closing over this, but not any other variables and that this is a type that properly implements ISynchronizeInvoke (like a WinForms form or control). This means that it won't work if:

you're calling it from a static method
you're only calling a static method from inside the lambda
you're closing over some local variable or parameter
you're calling it from another class

Because of that, I would never use code like this.

I think that you should just use Invoke() directly, something like:

Invoke(new Action(onCompleteHandle));

If you don't like that you need to specify the delegate type, you could write a couple of simple extension methods like:

public static void Invoke(this ISynchronizeInvoke target, Action action)
{
    target.Invoke(action, null);
}

and then use it like this (that this is required):

this.Invoke(onCompleteHandle);


Filter out duplicates from an array and return only unique value I have the following script, which filters duplicates and returns only unique values for an array.
When passing false or nothing the "original" array is overwritten, when passing true a new array is created with unique values and returned instead.

The script should work only with numbers and strings.

I would like to know:

If it is possible to optimize the code for maximum performance and how,
If it is possible to improve or write in alternative way logic for createArray, maybe a better way in term of code repetition.

https://jsfiddle.net/dj7qxyL7/

    Array.prototype.getUnique = function (createArray) {
        createArray = createArray === true ? true : false;
        var temp = JSON.stringify(this);
        temp = JSON.parse(temp);
        if (createArray) {
            var unique = temp.filter(function (elem, pos) {
                return temp.indexOf(elem) == pos;
            }.bind(this));
            return unique;
        }
        else {
            var unique = this.filter(function (elem, pos) {
                return this.indexOf(elem) == pos;
            }.bind(this));
            this.length = 0;
            this.splice(0, 0, unique);
        }
    }

    var duplicates = [0, 0, 1, 1, 2, 3, 1, 1, 0, 4, 4];
    console.log('++++ ovveride')
    duplicates.getUnique();
    console.log(duplicates);
    console.log('++++ new array')
    var duplicates2 = [0, 0, 1, 1, 2, 3, 1, 1, 0, 4, 4];
    var unique = duplicates2.getUnique(true);
    console.log(unique);
    console.log('++++ original')
    console.log(duplicates2);

If is possible to improve or write in alternative way logic for
  createArray, maybe a better way in term of code repetition.

Why give the option to mutate the array? Most array methods return a new array, including filter, so it seems redundant in your code, since you get a new array anyway. Not mutating the array is usually the right thing to do. In any case, using JSON to clone an array is very inefficient, when all you need is var copy = array.slice(); and a recursive solution for nested arrays. If you get rid of this unusual requirement, and create your own function instead of extending the prototype, then you are left with what unique really is:

var unique = function(xs) {
  return xs.filter(function(x, i) {
    return xs.indexOf(x) === i
  })
}

  If it is possible to optimize the code for maximum performance and how

If you only deal with things that can be stringified properly, like numbers, and obviously strings, then you can improve performance by using a simple object to keep track of items that have been seen already:

var unique = function(xs) {
  var seen = {}
  return xs.filter(function(x) {
    if (seen[x])
      return
    seen[x] = true
    return x
  })
}

Although this would consider 1 and '1' to be a duplicate.


Any way to optimize this already fast Python solution to Project Euler 31 (coin sums)? This is a Python solution for all general cases. Any suggestions at all would be appreciated; in my opinion it's already quite good! Here's a link to the problem.

from timeit import default_timer as timer

def count_coins(target, operands):
    ways_to_make = [1] # list of ways to make x at index x
    for o in range(0, len(operands)):
        for t in range(target + 1):
            # initialise undefined target
            if len(ways_to_make) <= t:
                ways_to_make.append(0)
            # add ways to make number recursively
            if t >= operands[o]:
                ways_to_make[t] += ways_to_make[t - operands[o]]

    return ways_to_make[target]

start = timer()
ans = count_coins(200, [1, 2, 5, 10, 20, 50, 100, 200])
elapsed_time = (timer() - start) * 1000 # s --> ms

print "Found %d in %r ms." % (ans, elapsed_time)

It will be faster if you pre-generate the ways_to_make array:

def count_coins(sum, coins):
    ways_to_make = [0 for _ in range(sum + 1)]
    ways_to_make[0] = 1
    for coin in coins:
        for t in range(sum + 1):
            if t >= coin:
                ways_to_make[t] += ways_to_make[t - coin]

    return ways_to_make[sum]

This doesn't waste memory because you would allocate those elements anyway, and it's a lot faster to create a larger list once than extending it in every loop iteration. Not to mention that this gets rid of an if statement.

I also changed the for o in range(len(coins)) loop to the more intuitive for coin in coins. In my measurements this gave an extra speed boost too.

Finally, I took the liberty to rename your function parameters to more meaningful names.


Preorder traversal using stack I wrote the code for an iterative pre-order traversal using stack for binary search tree. Below is the code for same. Please help me verify the correctness of the algorithm. Here t.t is the value of node t.

static void preorder(TreeNode t)
{
    MyConcurrentStack<TreeNode> s = new MyConcurrentStack<TreeNode>();
    while(s.peek()!=null || t!=null)
    {
        if(t!=null)
        {
            System.out.print(t.t+":");
            s.push(t);
            t = t.left;
        }
        else
        {
            t = s.pop();
            t = t.right;
        }
    }
}

There are a few points that can be critizised.

The method signature suggests that the architecture of the system isn't well-designed. Traversal of a Tree should likely be encapsulated in an Iterator class. In that case, the stack would be a private field of the iterator, and the actual code would use a foreach loop.

for (TreeNode node : tree)
  System.out.print(node.t + ":");

(preorder iteration is a good default)
Even if we let the design of your classes as they are, your implementation could be improved.

Single-letter variable names should be avoided. True, there are only two variables in your method, but using node and stack would be so much less cryptic.

The stack does not have to be concurrent, because it is not shared accross multiple threads. The vanilla java.util.Stack should work just fine. If you care about concurrency, you should rather worry about the tree nodes. What happens when a node was stored on the stack, but removed from the tree before the right child can be consumed by your code? Make sure the nodes aren't mutated until you visited them.

The body of your loop does not have a single comment although the stack manipulation may not be totally obvious to everybody (incl. you).

If you are optimizing for readability and not for space, I'd strongly suggest a recursive solution. This also makes writing correct code much easier:

static void preorder(TreeNode node) {
  if (node == null) return;

  System.out.println(node.t + ":");

  preorder(node.left);
  preorder(node.right);
}

(Not that your code was incorrect, this is just much easier to verify).

Next, I would like to reorder the parts of your loop to make it more obvious what is happening:

while (true)
{
    // check if we have to pop a value of the stack
    while (node == null) {
      if (stack.empty()) return;
      node = stack.pop().right;
    }

    System.out.print(node.t + ":");

    stack.push(node);  // store the node to continue the right path later
    node = node.left;
}

Note: If we use the java.util.Stack, then peek() could throw an EmptyStackException. We should rather use the empty() method which returns a boolean.

I think it would be more elegant to put the right childs on the stack, not the nodes which we already visited. We should also test whether these are null before adding them to the stack. This would keep the stack smaller:

while (true)
{
    // check if we have to pop a value of the stack WITHOUT LOOPING:
    // It is guaranteed that there never is a null node on the stack.
    if (node == null) {
      if (stack.empty()) return;
      node = stack.pop();
    }

    System.out.print(node.t + ":");

    // store the right node to continue there later
    if (node.right != null) stack.push(node.right);

    node = node.left;
}

I am not too comfortable with a while (true) loop, but the alternatives would mean slight code duplication (e.g. in your original code, you have two t!=null tests.)

Finally, you likely want to print a newline after this iteration. (In that case, the return should be a break).


Keyboard handler to move a shape in response to arrow keys I finished Codeacademy and I'm looking to practice and get better at JavaScript. Is this coded correctly or should I have made a function for it somehow?  The purpose of this code is to move a square around the page with arrow keys or buttons.

I have the entire script hosted here.

I mostly want to "Simplify this chain of if statements"  but also the post  about using a map instead of the if statements is something I needed to know.

function anim(e){
    if((e.keyCode === 37)||(e === 37)){
      y = shipLeft;
      shipLeft -= 11;
      y -= 11;
      y.toString();
      y = y + 'px';
      ship.style.left = y;
      changeColor();
    return shipLeft
}
    else if ((e.keyCode === 39) || (e === 39)){
      y = shipLeft;
      shipLeft += 11;
      y += 11;
      y.toString();
      y = y + 'px';
      ship.style.left = y;
      changeColor();
    return shipLeft
    }
    else if ((e.keyCode === 40) || (e === 40)){
      y = shipTop;
      y += 11;
      shipTop += 11;
      y.toString();
      y = y + 'px';
      ship.style.top = y;
      changeColor();
    return shipTop;
    }
    else if ((e.keyCode === 38) || ( e === 38)){
      y = shipTop;
      y -= 11;
      shipTop -= 11;
      y.toString();
      y = y + 'px';
      ship.style.top = y;
      changeColor();
    return shipTop;
    }
 }

If you move shipLeft and shipTop into an object called shipPositions rather than just top level variables, you could also use a map-driven approach like this:

function anim(e) {
    var key = e.KeyCode || e, val, info;
    var keyMap = {
        "37": {direction: "left", ship: "shipLeft", delta: -11},
        "39": {direction: "left", ship: "shipLeft", delta: 11},
        "40": {direction: "top", ship: "shipTop", delta: 11},
        "38": {direction: "top", ship: "shipTop", delta: -11},

    };
    info = keyMap[key];
    if (!info) {
        return;
    }
    shipPositions[info.ship] += info.delta;
    val = shipPositions[info.ship];
    ship.style[info.direction] = val + 'px';
    changeColor();
    return val;
}


Reverse a word in the char array I have to write a program that reads a word to the char array and next display it reversed. I must find best own optimal solution.

My solution of this exercise:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char* reverse(char* str);

int main(void) {
    char word[50];
    printf("Podaj slowo: ");
    scanf("%s", word);
    printf("Odwrocone slowo: %s\n", reverse(word));
    return 0;
}

char* reverse(char *str) {
    char* reversed = (char*) malloc(sizeof(char) * strlen(str) + 1);
    int j = 0;
    for (int i = strlen(str) - 1; i >= 0; i--, j++) {
        reversed[j] = str[i];
    }
    reversed[j + 1] = '\0';
    return reversed;
}

I'm applying linear method, specifically iterating through every elements and store it in a new pointer array but I'm not sure if there is any better solution that I don't know. Can anyone add hints how can I optimize code and review it?

Memory management

Right now, your code leaks memory--every time you call reverse, it allocates some memory, and none of your other code frees it again.

If all you're doing is reversing one string, then exiting, that's of little consequence--but if you try to use this in real code, leaking memory like this is generally unacceptable.

const correctness

Since you're not modifying the input string, you might as well use const in the function's signature:

char* reverse(char const * str);

Buffer overrun protection

Right now you have:

scanf("%s", word);

This is essentially equivalent to gets(word);. That is to say, it provides absolutely no protection against the user entering a string longer than you've provided space to store. When using %s with scanf (or cousins like fscanf, sscanf, etc.) you need to specify the maximum length:

scanf("%49s", word);

Alternatively, consider using fgets, which also requires you to specify the buffer size.

Note that there's a difference in the size you specify though. With fgets, you specify the size of the buffer, but with scanf you specify the number of characters it's allowed to read, which is one less than the size of the buffer itself. Also note that fgets normally retains the \n at the end of what was entered though (if you get data without a \n on the end, it means the buffer you supplied wasn't large enough to hold all the data that was entered).

Unnecessary Cast

When you allocate your memory:

char* reversed = (char*) malloc(sizeof(char) * strlen(str) + 1);

...you're currently casting the result of malloc. This is generally frowned upon by C programmers--it's unnecessary and it can cover up a bug of having failed to include the right header to declare/prototype malloc correctly.

char* reversed = malloc(sizeof(char) * strlen(str) + 1);

The cast is only necessary if you decide to write C that can also compile as C++. This tends to give the worst of both worlds, and should generally be avoided--if you're going to compile this as C++, use std::string and std::reverse.

Size Computation

When you allocate memory, you multiply the length by sizeof(char), but sizeof(char) is guaranteed to be 1, so the multiplication accomplishes nothing.

Even if you decide to leave the multiplication in (since it is necessary for types other than char), I prefer to use code like this:

char* reversed = malloc(sizeof(*reversed) * strlen(str) + 1);

This has the advantage that when/if you (for example) decide to support wide characters, you can change it to something like:

wchar_t* reversed = malloc(sizeof(*reversed) * (wcslen(str) + 1));

...so the argument to sizeof doesn't need to change at all (and believe me--if you decide to do something like this, you have enough headaches to deal with, so even a small help will be welcome).


Searching online English dictionaries I do always search online English dictionaries for words that I know but want to get a better understanding of and/or to know their other meanings, or for words that I do not know at all. Therefore, I have written a simple program to help me search for English words faster and easier.

Here is my code:

import webbrowser

class OnlineDictionary:
    """Represents an online dictionary.

    Attributes:
      name: str
      url: str
    """
    def __init__(self, name, url, sep):
        """Initializes an OnlineDictionary object.

        name: str
        url: str
        sep: str
        """
        self.name = name
        self.url = url
        self.sep = sep

    def search(self, query):
        """Searches the dictionary for a given query.

        url: str
        query: str
        """
        query = self.sep.join(query.split())
        webbrowser.open(self.url + query)

def print_instructions(message, instructions, names, num_of_dics):
    """Prints the available dictionaries, their corresponding
    numerals and the instructions.

    message: str
    instructions: str
    names: list
    num_of_dics: int
    """
    print(message)

    for numeral, name in zip(range(num_of_dics), names):
        print(numeral, name)

    print()
    print(instructions)

def get_user_data(num_of_dics):
    """Asks the user for which dictionary
    to use and for what to search for.

    num_of_dics: int

    Returns: tuple
    """
    while True:
        to_use = input('Specify which dictionary or dictionaries to use.\n')
        if to_use == '*':
            break

        try:
            to_use = to_use.replace(' ', '').split(',')
            numerals = []

            for num in to_use:
                num = int(num)
                if num not in range(num_of_dics):
                    raise
                numerals.append(num)

            to_use = set(numerals)  #set is used to get rid of any duplicates
            break

        except:
            continue

    query = input('Type something to search for.\n')

    return (to_use, query)

def __main__():
    names = ('Merriam-Webster Dictionary', 'Oxford Dictionary', 'Camberidge Dictionary', 'Collins Dictionary',
             'Longman Dictionary of Contemporary English','The American Heritage Dictionary of the English Language')

    urls = ('https://www.merriam-webster.com/dictionary/', 'https://en.oxforddictionaries.com/definition/',
            'https://dictionary.cambridge.org/dictionary/english/', 'https://www.collinsdictionary.com/dictionary/english/',
            'https://www.ldoceonline.com/dictionary/', 'https://ahdictionary.com/word/search.html?q=')

    seps = ('%20', '_', '-', '-', '-', '%20')  #seperators are used if there are spaces in the query

    collection = list(zip(names, urls, seps))

    num_of_dics = len(names)

    message = "These are the available dictionaries:\n"

    instructions = """To use a dictionary type its corresponding numeral.
To use more than one dictionary seperate the numerals with ",".
To use all of them type "*". Do not type the quotation marks.\n"""

    print_instructions(message, instructions, names, num_of_dics)

    while True:
        to_use, query = get_user_data(num_of_dics)
        print()
        dics_to_use = collection if to_use == '*' else [collection[num] for num in to_use]
        dics = [OnlineDictionary(name, url, sep) for name, url, sep in dics_to_use]

        for dic in dics:
            print('Searching %s for "%s"...' % (dic.name, query))
            dic.search(query)

        print('Finished searching. Check your Web browser.\n')

if __name__ == '__main__':
    __main__()

I would appreciate any suggestions and comments.

Looks pretty good. I have a few suggestions though:

There should be a way to exit (maybe if you don't type anything when it asks which dictionaries?)
You should probably use urllib.parse.urljoin instead of +.
You should probably use recursion instead of using while True then always breaking or continueing.
You don't need parentheses when returning tuples.
Bare raise statements are for reraising exceptions. Otherwise, it gives you RuntimeError: No active exception to reraise.
Don't use bare except.
I would probably use a list of tuples instead of using zip.
You could pass around the whole list instead of its length.
You can use enumerate(iterable) instead of zip(range(len(iterable)), iterable). What enumerate does is create an iterable that when iterated, yields tuples of increasing numbers starting with 0 and the next item from the iterable (in your case a list) you are enumerating. More info at the Python Docs.
You should handle the logic for * in one place.
Contained the logic for handling numerals within the get_user_data function.
You should probably name your main function main instead of __main__.
Don't combine multiline strings and '\n'.
It might be good to have it display the result directly in the console instead of the web browser, but that would be a whole other project.
You should probably return directly instead of using break.
I might have made a few other minor changes.

Note that some of the changes are just personal preferences.

The result (also available at https://repl.it/@solly_ucko/DictionarySearchCodeReview):

import webbrowser
from urllib.parse import urljoin

class OnlineDictionary:
    """Represents an online dictionary.

    Attributes:
name: str
url: str
    """
    def __init__(self, name, url, sep):
        """Initializes an OnlineDictionary object.

        name: str
        url: str
        sep: str
        """
        self.name = name
        self.url = url
        self.sep = sep

    def search(self, query):
        """Searches the dictionary for a given query.

        url: str
        query: str
        """
        query = self.sep.join(query.split())
        webbrowser.open(urljoin(self.url, query))

def print_instructions(message, instructions, dictionaries):
    """Prints the available dictionaries, their corresponding
    numerals and the instructions.

    message: str
    instructions: str
    names: list
    num_of_dics: int
    """
    print(message)

    for numeral, (name, _, _) in enumerate(dictionaries):
        print(numeral, name)

    print()
    print(instructions)

def get_user_data(dictionaries):
    """Asks the user for which dictionary
    to use and for what to search for.

    num_of_dics: int

    Returns: tuple
    """

    while True:
        to_use = input('Specify which dictionary or dictionaries to use.\n')

        if to_use == '':
            return (), ''
        elif to_use == '*':
            to_use = dictionaries
        else:
            try:
                to_use = to_use.replace(' ', '').split(',')
                numerals = []

                for num in to_use:
                    num = int(num)

                    if num not in range(len(dictionaries)):
                        raise ValueError

                    numerals.append(num)

                to_use = set(map(dictionaries.__getitem__, numerals)) # set is used to get rid of duplicates
                query = input('Type something to search for.\n')
                return to_use, query
            except ValueError:
                continue

def main():
    dictionaries = [
        ('Merriam-Webster Dictionary', 'https://www.merriam-webster.com/dictionary/', '%20'),
        ('Oxford Dictionary', 'https://en.oxforddictionaries.com/definition/', '_'),
        ('Camberidge Dictionary', 'https://dictionary.cambridge.org/dictionary/english/', '-'),
        ('Collins Dictionary', 'https://www.collinsdictionary.com/dictionary/english/', '-'),
        ('Longman Dictionary of Contemporary English', 'https://www.ldoceonline.com/dictionary/', '-'),
        ('The American Heritage Dictionary of the English Language', 'https://ahdictionary.com/word/search.html?q=', '%20')
    ]

    message = 'These are the available dictionaries:\n'

    instructions = """To use a dictionary type its corresponding numeral.
To use more than one dictionary separate the numerals with ",".
To use all of them type "*". Do not type the quotation marks.
"""

    print_instructions(message, instructions, dictionaries)

    while True:
        to_use, query = get_user_data(dictionaries)

        if not to_use or not query:
            break

        print()
        chosen_dictionaries = [OnlineDictionary(name, url, sep) for name, url, sep in to_use]

        for dictionary in chosen_dictionaries:
            print('Searching %s for "%s"...' % (dictionary.name, query))
            dictionary.search(query)

        print('Finished searching. Check your Web browser.\n')

if __name__ == '__main__':
    main()


Tiny image scraper for xkcd.com This came out on the spur of the moment, as a quick and dirty tool to have the job done.

This is a simple image scraper for immensely popular and legendary comic website inspired by a Python Easter Egg.

For those who don't know it, run your Python interpreter and type import antigravity and hit Enter. :)

As for the code below, I'd appreciate any feedback, particularly in regards to threading, as I'm new to this.

#!/usr/bin/python3

import os
import sys
import time
import threading
from pathlib import Path
from shutil import copyfileobj

import requests
from lxml import html

BASE_URL = "https://www.xkcd.com/"
ARCHIVE = "https://www.xkcd.com/archive"
SAVE_DIRECTORY = Path('xkcd_comics')
LOGO = """
       _           _
 tiny | |  image  | | downloader for
 __  _| | _____ __| |  ___ ___  _ __ ___
 \ \/ / |/ / __/ _` | / __/ _ \| '_ ` _ \
  >  <|   < (_| (_| || (_| (_) | | | | | |
 /_/\_\_|\_\___\__,_(_)___\___/|_| |_| |_|
 version 0.1
"""

def show_logo():
    print(LOGO)

def fetch_url(url: str) -> requests.Response:
    return requests.get(url)

def head_option(values: list) -> str:
    return next(iter(values), None)

def get_penultimate(url: str) -> int:
    page = fetch_url(url)
    tree = html.fromstring(page.content)
    newest_comic = head_option(
        tree.xpath('//*[@id="middleContainer"]/a[1]/@href'))
    return int(newest_comic.replace("/", ""))

def get_images_from_page(url: str) -> str:
    page = fetch_url(url)
    tree = html.fromstring(page.content)
    return head_option(tree.xpath('//*[@id="comic"]//img/@src'))

def get_number_of_pages(latest_comic: int) -> int:
    print(f"There are {latest_comic} comics.")
    print(f"How many do you want to download? Type 0 to exit.")
    while True:
        try:
            number_of_comics = int(input(">> "))
        except ValueError:
            print("Error: Expected a number. Try again.")
            continue
        if number_of_comics > latest_comic or number_of_comics < 0:
            print("Error: Incorrect number of comics. Try again.")
            continue
        elif number_of_comics == 0:
            sys.exit()
        return number_of_comics

def clip_url(img: str) -> str:
    return img.rpartition("/")[-1]

def make_dir():
    return os.makedirs(SAVE_DIRECTORY, exist_ok=True)

def save_image(img: str):
    comic_name = clip_url(img)
    print(f"Downloading: {comic_name}")
    f_name = SAVE_DIRECTORY / comic_name
    with requests.get("https:" + img, stream=True) as img, open(f_name, "wb") \
            as output:
        copyfileobj(img.raw, output)

def show_time(seconds: int) -> int:
    minutes, seconds = divmod(seconds, 60)
    hours, minutes = divmod(minutes, 60)
    time_elapsed = f"{hours:02d}:{minutes:02d}:{seconds:02d}"
    return time_elapsed

def get_xkcd():
    show_logo()
    make_dir()

    collect_garbage = []
    latest_comic = get_penultimate(ARCHIVE)
    pages = get_number_of_pages(latest_comic)

    start = time.time()
    for page in reversed(range(latest_comic - pages + 1, latest_comic + 1)):
        print(f"Fetching page {page} out of {latest_comic}")
        try:
            url = get_images_from_page(f"{BASE_URL}{page}/")
            thread = threading.Thread(target=save_image, args=(url, ))
            thread.start()
        except (ValueError, AttributeError, requests.exceptions.MissingSchema):
            print(f"WARNING: Invalid comic image source url.")
            collect_garbage.append(f"{BASE_URL}{page}")
            continue
    thread.join()
    end = time.time()

    print(f"Downloaded {pages} comic(s) in {show_time(int(end - start))}.")

    if len(collect_garbage) > 0:
        print("However, was unable to download images for these pages:")
        print("\n".join(page for page in collect_garbage))

def main():
    get_xkcd()

if __name__ == '__main__':
    main()

Stop hammering XKCD server

You basically wrote a denial-of-service attack, since you try to connect as fast and as often as possible to the server.
By sharing this code on CodeReview, it becomes a distributed denial-of-service.

Just in case it wasn't clear : don't do that.

I suppose the load of XKCD servers is high enough that your code won't make any noticeable difference. Still, the primary goal of your code should be to stay under the radar. Seen from outside, there shouldn't be any difference between launching your script and casually browsing https://xkcd.com/.

It isn't just as a courtesy to fellow developers : you might be banned by the remote servers if you send too many requests too fast.

Solutions

Wait at least one second before downloading any picture. The desired pause might be defined as Crawl-delay in robots.txt.
Remove multi-threading or at least limit the number of threads.
Don't download a file again if it is already in SAVE_DIRECTORY. You can check if it's here, if its size is larger than 0 or if it is indeed a PNG file.

Unrelated note

You know the id for each comic. You should probably write it in the filename : geologic_time.png could be called 2187_geologic_time.png or 02187_geologic_time.png.

This way, comics are sorted chronologically inside SAVE_DIRECTORY.


Modify casing of marked letters Trying to learn coding in my 30s.

Could anybody review my code and give me some feedback if possible? It works, but I am sure there are better ways to solve this problem.

The problem:

A text is given. Write a program that modifies the casing of letters to uppercase at all places in the text surrounded by <upcase> and </upcase> tags. Tags cannot be nested.

Here is what I've come up with:

namespace Task6Casing
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.Write("Text: ");
            string text = Console.ReadLine();

            char[] textNew = new char[text.Length];

            int i = 0, j = 0, k = 0;

            while (i < text.Length)
            {
                if (text.IndexOf("<upcase>", k) < 0)            // check if there is any <upcase> tag
                {                                               // if no, copy everyting
                    while (i < text.Length)
                    {
                        textNew[j++] = text[i++];
                    }
                }
                else
                {
                    while (i < text.IndexOf("<upcase>", k))     // if there is an <upcase> tag, copy letters until the tag
                    {
                        textNew[j++] = text[i++];
                    }

                    i += 8;                                     // move index i to the position right next to the <upcase> tag
                    k = i;
                }

                if (text.IndexOf("</upcase>", k) < 0)           // check if there is any </upcase> tag
                {                                               // if no, copy everyting in CAPITAL letters
                    while (i < text.Length)
                    {
                        textNew[j++] = Char.ToUpper(text[i++]);
                    }
                }
                else
                {
                    while (i < text.IndexOf("</upcase>", k))    // if there is an </upcase> tag, copy letters in CAP letters until the tag
                    {
                        textNew[j++] = Char.ToUpper(text[i++]);
                    }

                    i += 9;                                     // move index i to the position right next to the </upcase> tag
                }
            }

            foreach(char c in textNew)
            {
                Console.Write(c);
            }
            Console.WriteLine();

        }
    }
}

Your code is easy to understand and very performant.

Some improvements:

String for start / end tag could be stored as constant. That has the advantage that it can be changed on one central location and the length of the string can be accessed like i += START_TAG.Length
When using a StringBuilder instead of of the char array newText, the running variable 'j' can be dropped.
The 2 code parts

            if (text.IndexOf("<upcase>", k) < 0)            // check if there is
            any <upcase> tag
            {                                               // if no, copy everyting
                while (i < text.Length)
                {
                    textNew[j++] = text[i++];
                }
            }
            else
            {
                while (i < text.IndexOf("<upcase>", k))     // if there is an <upcase> tag, copy letters until the tag
                {
                    textNew[j++] = text[i++];
                }

                i += 8;                                     // move index i to the position right next to the <upcase> tag
                k = i;
            }

and

            if (text.IndexOf("</upcase>", k) < 0)           // check if there is any </upcase> tag
            {                                               // if no, copy everyting in CAPITAL letters
                while (i < text.Length)
                {
                    textNew[j++] = Char.ToUpper(text[i++]);
                }
            }
            else
            {
                while (i < text.IndexOf("</upcase>", k))    // if there is an </upcase> tag, copy letters in CAP letters until the tag
                {
                    textNew[j++] = Char.ToUpper(text[i++]);
                }

                i += 9;                                     // move index i to the position right next to the </upcase> tag
            }

are very simlar. Probably it is possible to create one more generic code fragment that coveres both cases.

Since your solution is still understandable for such a simple use case, it will fast become unmaintainable if the use case becomes more complex. Therefore, it makes sense to think about a more abstract OOP concepts to model the solution.

One alternative impl. (that is probably over engineered for the given problem) gives an idea how a more object oriented design could look like:

        public class Tag
        {
            private readonly Func<char, char> map;
            public Tag(string start, string end, Func<char, char> map)
            {
                this.Start = start;
                this.End = end;
                this.map = map;
            }

            public string Start { get; }
            public string End { get; }
            public char Map(char input) => this.map(input);
        }

        public class TagProcessor
        {
            private readonly Tag tag;
            private readonly StringBuilder output = new StringBuilder();

            private string input;
            private bool isTagOpen;
            private int index;

            public TagProcessor(Tag tag)
            {
                this.tag = tag;
            }

            public string Process(string input)
            {
                this.input = input;
                this.index = 0;
                this.isTagOpen = false;
                this.output.Clear();

                do
                {
                    var tagProcessed = this.TryOpenTag() || this.TryCloseTag();
                    if (!tagProcessed)
                    {
                        this.ApplyCurrentChar();
                    }
                }
                while (this.MoveNext());

                return output.ToString();
            }

            private bool IsEndTag() => input.IndexOf(tag.End, this.index) == this.index;

            private bool IsStartTag() => input.IndexOf(tag.Start, this.index) == this.index;

            private bool MoveNext()
            {
                index++;
                return index < this.input.Length;
            }

            private void ApplyCurrentChar()
            {
                var inputChar = this.input[this.index];
                var transfomed = this.isTagOpen ? tag.Map(inputChar) : inputChar;
                this.output.Append(transfomed);
            }

            private bool TryOpenTag()
            {
                if (!isTagOpen && IsStartTag())
                {
                    this.index += this.tag.Start.Length - 1;
                    this.isTagOpen = true;
                    return true;
                }

                return false;
            }

            private bool TryCloseTag()
            {
                if (isTagOpen && IsEndTag())
                {
                    this.index += this.tag.End.Length - 1;
                    this.isTagOpen = false;
                    return true;
                }

                return false;
            }
        }

        public static void Main(string[] args)
        {
            var processor = new TagProcessor(new Tag("<upcase>", "</upcase>", char.ToUpper));

            var test = new[]
            {
                "abc<upcase>test</upcase>",
                "abc<upcase>test",
                "abc<upcase></upcase>test",
                "abc<upcase>test</upcase>test",
                "abc<upcase>te<upcase>st</upcase>test",
                "a</upcase>bc<upcase>te<upcase>st</upcase>te</upcase>st",
            };

            foreach (var t in test)
                Console.WriteLine(t + ": " + processor.Process(t));
            Console.ReadLine();
        }

The advantages are, that this solution remains readable if the complexity grows (e.g. more tags were added) and it allows to change / extend the logic without understanding the whole parsing logic. Further more, each method has a single pupose which increases comprehensibleness.


K-nearest neighbours in MATLAB I implemented K-Nearest Neighbours algorithm, but my experience using MATLAB is lacking. I need you to check the small portion of code and tell me what can be improved or modified.  I hope it is a correct implementation of the algorithm.

function test_data = knn(test_data, tr_data,k)

numoftestdata = size(test_data,1);
numoftrainingdata = size(tr_data,1);

for sample=1:numoftestdata

   %Step 1: Computing euclidean distance for each testdata
   R = repmat(test_data(sample,:),numoftrainingdata,1) ;
   euclideandistance  = (R(:,1) - tr_data(:,1)).^2;

   %Step 2: compute k nearest neighbors and store them in an array
    [dist position] = sort(euclideandistance,'ascend');
    knearestneighbors=position(1:k);
    knearestdistances=dist(1:k);

    % Step 3 : Voting
    for i=1:k
        A(i) = tr_data(knearestneighbors(i),2);
    end

    M = mode(A);

    if (M~=1)
        test_data(sample,2) = mode(A);
    else
        test_data(sample,2) = tr_data(knearestneighbors(1),2);
    end
end

To test it you can use :

test_data = [6,0; 2,0; 5,0]
tr_data = [1,1;0,2;3,2; 4,4; 5,3]

Use consistent indentation.
You switch from extremely verbose all lower case variable names like numoftrainingdata to single letter capitalized variable names like A. Make your variable names descriptive and no longer than necessary, and be consistent.
Use consistent white space between operators.
knn() doesn't need the second column of test_data, and the calling function doesn't need the first column of test_data.

Rather than calling the function like this:

test_data = knn(test_data,tr_data,k);

Call it like this:

test_data(:,2) = knn(test_data(1,:),tr_data,k);

Do you want to handle certain error conditions like 0 >= k or k > size(tr_data,1)?
Rather than squaring the distance, you can use abs().
Remove the ascend parameter from sort(), that is the default mode.
knearestdistances is unused.
You call mode() a second time rather than using M.

Simplify:

[dist position] = sort(euclideandistance,'ascend');
knearestneighbors = position(1:k);
knearestdistances = dist(1:k);
for i=1:k
    A(i) = tr_data(knearestneighbors(i),2);
end
M = mode(A);
if (M~=1)
    test_data(sample,2) = mode(A);
else
    test_data(sample,2) = tr_data(knearestneighbors(1),2);
end

to

[~,position] = sort(euclideandistance);
A = tr_data(position(1:k),2);
M = mode(A);
if (M~=1)
    test_data(sample,2) = M;
else
    test_data(sample,2) = tr_data(position(1),2);
end

After applying the above suggestions and vectorizing the function you could write it as:

function out_data = knn(test_data,tr_data,k)
    test_data_n = size(test_data,1);
    tr_data_n = size(tr_data,1);

    % absolute distance between all test and training data
    dist = abs(repmat(test_data,1,tr_data_n) - repmat(tr_data(:,1)',test_data_n,1));

    % indicies of nearest neighbors
    [~,nearest] = sort(dist,2);
    % k nearest
    nearest = nearest(:,1:k);

    % mode of k nearest
    val = reshape(tr_data(nearest,2),[],k);
    out_data = mode(val,2);
    % if mode is 1, output nearest instead
    out_data(out_data==1) = val(out_data==1,1);
end

Edit

Regarding correctness, i'm not sure why you check to see if the mode is 1. There is nothing unique about a mode of 1 in general.


Handling exceptions in Python 3 Please can you review my standard exception handler for Python 3.x. It is located under the # +=+= line. The code above the line is to create an exception and so the code is code complete and runs standalone.
The code is just a standard exception formatter, so users are presented with easier to read messages rather than stack traces. This is a common idiom in many Python projects. But many / all don't format to the extent I have.
I have searched a lot, but I can't find anything better. I expected to find something on GitHub, but was unfruitful. If you find a way to improve this, please post it here so that I and others will know.
import inspect
import os
import sys
import traceback

if __name__ == '__main__':
    try:
        x = 42/0

    # +=+=+=+=+=+=+=+=+=+=+=+=+=
    except KeyboardInterrupt as e: # Ctrl-C
        raise e
    except SystemExit as e: # sys.exit()
        raise e
    except Exception as e :
        exc_type, exc_obj, exc_tb = sys.exc_info()
        scriptName = os.path.split(exc_tb.tb_frame.f_code.co_filename)[1]
        functionName = inspect.currentframe().f_code.co_name
        print('=======================================================================================================')
        print('Unhandled exception: ', exc_type, scriptName, 'at line', exc_tb.tb_lineno)
        print('                      in function', functionName)
        print('                     ', str(e))

        traceback.print_exc()
        sys.exit(1)

Rather than using as e then raise e you can just omit e.

except KeyboardInterrupt as e:
    raise e

except KeyboardInterrupt:
    raise

Rather than specifying an except block for KeyboardInterrupt and SystemExit you can just specify them in the same except by passing a tuple.

except KeyboardInterrupt:
    raise
except SystemExit:
    raise

except (KeyboardInterrupt, SystemExit):
    raise

Both KeyboardInterrupt and SystemExit are not subclasses of Exception so you do not need to handle them. The exceptions will propagate like normal.
You may want to familiarize yourself with the exception hierarchy.

You can get all the information that sys.exc_info() gives from e.

exc_type, exc_obj, exc_tb = sys.exc_info()

exc_type, exc_obj, exc_tb = type(e), e, e.__traceback__

You can get the current frame from exc_tb rather than inspect.currentframe(). This makes the code more portable if you ever decide to move the code into a function to format tracebacks.

frame = inspect.currentframe()

frame = exc_tb.tb_frame

Please follow PEP 8 and name variables with snake_case. It is very easy to see the code you've taken from the Python docs and the code you've written yourself / taken from somewhere else.

If you're going to hard code the == at least use * to build it the size you want.

print('=======================================================================================================')

print('=' * 103)

Better yet just print to the size of the terminal.
print('=' * os.get_terminal_size().columns)

I would prefer to use f-strings if available.

print('Unhandled exception: ', exc_type, script_name, 'at line', exc_tb.tb_lineno)

print(f'Unhandled exception:  {exc_type} {script_name} at line {exc_tb.tb_lineno}')

if __name__ == '__main__':
    try:
        x = 42/0
    except Exception as e:
        exc_tb = e.__traceback__
        script_name = os.path.split(exc_tb.tb_frame.f_code.co_filename)[1]
        function_name = exc_tb.tb_frame.f_code.co_name
        print('=' * os.get_terminal_size().columns)
        print(f'Unhandled exception:  {type(e)} {script_name} at line {exc_tb.tb_lineno}')
        print(f'                      in function {function_name}')
        print(f'                      {e}')

        traceback.print_exc()
        raise SystemExit(1)


Creating an Entity, persisting it to the DB, and sending a welcome email The reason I'm posting this piece of code is because I want to know how I'm supposed to properly handle the change in requirements/specifications and how I should accommodate it in unit testing.

/**
 * Create an Affiliate
 */
public function createUser($username, $password, $email, $firstName, $lastName, $ip, $notifications = null) {
    // These values are assumed to be validated prior to this function being
    // called.

    $date = new \DateTime();

    $user = new Affiliate();
    $user->setUsername($username)
         ->setPassword(AffiliateUtils::hashPassword($password))
         ->setStatus(Affiliate::STATUS_PENDING)
         ->setEmail($email)
         ->setFirstName($firstName)
         ->setLastName($lastName)
         ->setLastIp($ip)
         ->setRegisteredIp($ip)
         ->setLastDate($date)
         ->setRegisteredDate($date);

    try {
        $this->em->persist($user);
        $this->em->flush();
    } catch (DBALException $e) { return false; }

    if ($notifications != null)
        $notifications->sendAffiliateEmail("WELCOME_AFFILIATE", $user);

    return true;
}

This function is apart of an AffiliateHandler, which is responsible for the various business-logic-related operations on an Affiliate which is a database entity (active record). This function is called when a user registers for my site.

Prior to the change in requirements, the $notifications parameter was not required. The notifications class is responsible for sending a welcome email containing a link for the user to verify their email. I added a default value to the variable because the creation of the user is not dependent upon a notification and I didn't want to break/modify the unit tests associated with this particular function.

I feel like this is bad style because this function is doing too many things. Also, it creates somewhat of a dependency on the notifications class. But at the same time, if I break up the function, I need to have access to the new user created so I can send him/her the notification.

What should I be doing to handle this change without adding too much complexity to my code?

Just some thoughts:

1) parameters instead of properties - createUser function has various user data as parameters. I think it is better to have a single object with properties instead (e.g. $userData). If information is added to the user entity, properties are added, not extra parameters in the function.

2) auto mapping - in many cases members/properties must be copied from one object to another (e.g. data coming from a form into an object to be persisted in the database). This means tedious work for the programmers, so this one reason auto mappers were invented. For PHP, Nylle is one (never used, though).

A mapper is handling copying of data from an object to another based on property names, by default (a case of convention over configuration).

For your particular case, assuming that user data comes in a $userData instance and that it has the same properties, it should look like this:

$user = $mapper->Map(new \Affiliate(), $userData);

Of course, for several properties, it is overkill (and also the configuration is more repugnant that the bigger brother's one - .NET Automapper), but may be useful when dealing with more.

3) Separation of concerns - you can (should) separate notification sending from user creation, as they different things and also automatic tests should treat them differently (e.g. you can automatically create test users on some test database, but surely you do not want to send lots of test e-mails / mock e-mail sending):

createUser may return the newly created user
another function may use this instance to send the e-mail

4) Error logging - Db persist failure is handled only by returning false to the caller. It is recommended to log these failures using a logging framework or a custom one.