Find the longest ascending subarray from a given array I have following task, form a given array, I want the longest ascending array.
I will give you an example

int[] a = {19,12,13,1,2,3,4,5,14,23,24,25,26,31,32};

will return array of {1,2,3,4,5} - that is the largest sequence of ascending numbers in the given array.

Below is the sample code:

int[] a = {19,12,13,1,2,3,4,5,14,23,24,25,26,27,31,32};
        List<Integer> longestArray = new ArrayList<Integer>();
        List<Integer> currentArary = new ArrayList<Integer>();
        for (int i = 1; i < a.length; i++) {
            if(currentArary.isEmpty()) {
                currentArary.add(a[i-1]);
            }
            if (a[i]-1 == a[i-1]) {
                currentArary.add(a[i]);
            } else {
                if(longestArray.size()<currentArary.size()) {
                    longestArray.clear();
                    longestArray.addAll(currentArary);
                }
                currentArary.clear();
            }
        }
        System.out.println(longestArray);

Any feedback received on this method is more than welcome.

Josay provided a lot of good feedback, so I'll try to focus on something I think is very important.

There's no need to save the current subsequence

For your solution, you manage two lists, both of which are rewritten and cleared multiple times. Instead of doing this, we can just save where the largest subsequence starts, and how long it is. This can be stored in two integers.

When looping, we also keep track of where the current subsequence started, and how long it is. Once it is broken, we simply compare the length to the previous maximum length, and update accordingly. I also added a check to handle sequences which are fully ascending (e.g. {1, 2, 3, 4, 5}).

public static int[] getLongestAscending(int[] a) {
    int maxLength = 0;
    int maxStart = 0;
    int length = 1;
    int start = 0;
    boolean fullAscension = true;
    for (int i = 1; i < a.length; i++) {
        if (a[i]-1 == a[i-1]) {
            length++;
        } else {
            fullAscension = false;
            if (length > maxLength) {
                maxLength = length;
                maxStart = start;
            }
            length = 1;
            start = i;
        }
    }
    if (fullAscension) {
        return a;
    }
    if (length > maxLength) {
        maxLength = length;
        maxStart = start;
    }       int[] ret = new int[maxLength];
    System.arraycopy(a, maxStart, ret, 0, maxLength);
    return ret;
}

According to me, this is clearer, and saves all information needed. It also has the advantage of returning data on the same format as the input, and it is also quite a bit faster. From some benchmarks it seems to be about 20-30 times faster.


Scrapy crawler to parse data recursively I've written a script in python scrapy to parse "name" and "price" of different products from a website. Firstly, it scrapes the links of different categories from the upper sided bar located in the main page then it tracks down each categories and reach their pages and then parse the links of different sub-categories from there and finally gets to the target page and parse the aforementioned data from there. I tried to do the whole thing slightly differently from the conventional method in which it is necessary to set rules. However, I got it working the way I expected using the logic I applied here. If any improvement is to be made, I'll be very glad to comply with. Here is what I've tried with:

"sth.py" aka spider contains:

import scrapy

class SephoraSpider(scrapy.Spider):

    name = "sephorasp"

    def start_requests(self):
        yield scrapy.Request(url = "https://www.sephora.ae/en/stores/", callback = self.parse_pages)

    def parse_pages(self, response):
        for link in response.xpath('//ul[@class="nav-primary"]//a[contains(@class,"level0")]/@href').extract():
            yield scrapy.Request(url = link, callback = self.parse_inner_pages)

    def parse_inner_pages(self, response):
        for links in response.xpath('//li[contains(@class,"amshopby-cat")]/a/@href').extract():
            yield scrapy.Request(url = links, callback = self.target_page)

    def target_page(self, response):
        for titles in response.xpath('//div[@class="product-info"]'):
            product = titles.xpath('.//div[contains(@class,"product-name")]/a/text()').extract_first()
            rate = titles.xpath('.//span[@class="price"]/text()').extract_first()
            yield {'name':product,'price':rate}

"items.py" includes:

import scrapy
class SephoraItem(scrapy.Item):
    name = scrapy.Field()
    price = scrapy.Field()

The command I used to get the result along with a csv output is:

scrapy crawl sephorasp -o items.csv -t csv

The code is quite clean and easy to read, good job!

I would only focus on couple things:

remove extra spaces around = when it is used to define a keyword argument
CSS selectors are more appropriate and reliable when it comes to handling multi-valued class attribute than XPath expressions. Plus, they are more concise and generally faster
naming - for links in should actually be for link in
as for the target_page method, I don't think you need a loop - if I understand correctly, there should be a single "product" parsed at this point
you may use start_urls instead of start_requests()

All things taken into account:

import scrapy

class SephoraSpider(scrapy.Spider):
    name = "sephorasp"
    start_urls = ["https://www.sephora.ae/en/stores/"]

    def parse(self, response):
        for link in response.css('ul.nav-primary a.level0::attr(href)').extract():
            yield scrapy.Request(url=link, callback=self.parse_inner_pages)

    def parse_inner_pages(self, response):
        for link in response.css('li.amshopby-cat > a::attr(href)').extract():
            yield scrapy.Request(url=link, callback=self.target_page)

    def target_page(self, response):
        name = response.css('.product-name > a::text').extract_first()
        price = response.css('span.price::text').extract_first()
        yield {'name': name, 'price': price}

(not tested)


MacPorts - Quick Install Script I wrote this script to automatically install the relevant version of MacPorts.
It's supposed to be used in a fashion similar to the official installer for HomeBrew.

FYI: The real reason that I decided to write this script is; I'm not all that confident in every aspect of my bash scripting abilities and I needed a project to practice on.

While I have a decent working knowledge of bash from an REPL standpoint, I haven't been scripting very long; so keep that in mind.

macports-installer.sh

#!/bin/bash
#
urisvn=https://svn.macports.org/repository/macports/trunk/www/includes/common.inc
uridist=https://distfiles.macports.org/MacPorts
#
mpvers=$(curl -s $urisvn | grep -m 1 -e \'*.*.*\' | cut -d \' -f 2)
osmajor=$(uname -r | cut -d . -f 1)
#
case $osmajor in
8)
    osvers=10.4
    osname=Tiger
    ;;
9)
    osvers=10.5
    osname=Leopard
    ;;
10)
    osvers=10.6
    osname=SnowLeopard
    ;;
11)
    osvers=10.7
    osname=Lion
    ;;
12)
    osvers=10.8
    osname=MountainLion
    ;;
13)
    osvers=10.9
    osname=Mavericks
    ;;
14)
    osvers=10.10
    osname=Yosemite
    ;;
15)
    osvers=10.11
    osname=ElCapitan
    ;;
*)
    echo Warning: unknown OS version
    ;;
esac
#
mppkg=MacPorts-${mpvers}-${osvers}-${osname}
mpchk=MacPorts-${mpvers}
echo $mppkg.pkg
echo $mpchk.chk.txt
curl $uridist/$mppkg.pkg -O
curl $uridist/$mpchk.chk.txt -O
sha256=$(shasum -a 256 ./$mppkg.pkg)
#
if grep $sha256 $mpchk.chk.txt;
    then sudo installer -verboseR -allowUntrusted -pkg ./$mppkg.pkg -target /;
fi
#
rm ./$mppkg.pkg
rm ./$mpchk.chk.txt
#
if ! grep -F 'export PATH="/opt/local/bin:/opt/local/sbin:$PATH"' .bash_profile;
    then echo 'export PATH="/opt/local/bin:/opt/local/sbin:$PATH"' >> .bash_profile;
fi
#

Also, don't forget to mention if there's anything good about the script. If there's anything you do like about it; or if it does something well, let me know..

You've got 0 validation in there, so the slightest hiccup will halt your script halfway. That's fine for you, but for a user, there will be a script that runs, crashes along the way, and then leaves all sorts of junk lying about.

You also have a "warning" for an unknown OS version, but then the os version seems to be required - without it, mppkg won't make sense and it's likely that the download will fail (unless there is a standard file, MacPorts-someversion-.pkg?). You'd be better off exiting the script then and there. Fail fast.

Lastly, the two paired curl calls

curl $uridist/$mppkg.pkg -O
curl $uridist/$mpchk.chk.txt -O

Could be ran asynchronously (see this serverfault question https://serverfault.com/questions/456490/execute-curl-requests-in-parallel-in-bash) with & and wait. That way the script would be a bit faster.


Check a string to see if it is a palindrome I have written two code pieces  to check if a given string is a palindrome:

public static bool FirstMethod(string str)
{
    int i = 0;
    int j = str.Length - 1;

    while (i<j)
    {
        if (str[i] != str[j])
            return false;

        i++;
        j--;
    }

    return true;
}

public static bool SecondMethod(string myString)
{
    string first = myString.Substring(0, myString.Length / 2);
    char[] arr = myString.ToCharArray();
    Array.Reverse(arr);
    string temp = new string(arr);
    string second = temp.Substring(0, temp.Length / 2);
    return first.Equals(second);
}

Please tell me which one is better.

Is there a better method than this?

I would prefer the first one for a few reasons.

It's more readable and easily understandable in my opinion.
There's only one string, so less overhead. (Which is negligible unless this is being called a lot.)
There's only one array. Reverse creates a second array.

That being said, an actual review is in order.

Methods that return a boolean value should have names of the form IsSomething or HasSomething. A great name would be IsPalindrome.
Neither method accepts anything but a string. Numbers can be palindromic too. Wouldn't it be a nice addition if we could pass an integer to it as well? (Without explicitly calling .ToString() before passing it in that is.)
I like that you're only checking for i < j instead of i <= j. It saves a useless iteration when there are an odd number of characters.
I also like the while loop. It's a lot cleaner than the for loop suggested by another answer.
I won't speak much on the second method, because I think @elios provided a nice implementation of it.


One to one record mapping without any common column between two tables in SQL server I'm trying to join two tables based on one to one mapping without any common column.

For example, I have 3 records in the first table and 2 records in the second table.

The mapping should be as follows:

As there is no third record in the second table, nulls should be displayed.

In order to implement this, I've tried the following code:

declare @FirstTable table (StudentId int, SubjectId int)
declare @SecondTable table (MarksId int, RankId int, LastRank int)

insert into @FirstTable values (1, 1)
insert into @FirstTable values (1, 2)
insert into @FirstTable values (1, 3)

insert into @SecondTable values (1, 4, 10)
insert into @SecondTable values (1, 5, 11)

;with XmlTable (RowNumber, StudentId, SubjectId) as
(
    select row_number() over(order by StudentId) as RowNumber, * from @FirstTable
)
,TechnicalIdsTable (RowNumber, MarksId, RankId, LastRank) as
(
    select row_number() over(order by MarksId) as RowNumber, * from @SecondTable
)
select x.StudentId, x.SubjectId, t.MarksId, t.RankId, t.LastRank from XmlTable x
left join TechnicalIdsTable t
on x.RowNumber = t.RowNumber

Based on the above code, I am able to achieve the output. Let me know whether this is a proper approach or not.

Here's a new example:

In this case, there are three students enrolled for sports, you can check the order in which they enrolled. But there are only two vacant sports available and the order in which they have to be selected.

Now using my query, which will be

declare @Students table (StudentId int, StudentName varchar(50), EnrolledOn datetime)
declare @Sports table (SportId int, SportName varchar(50))

insert into @Students values (1, 'John', '2013-05-25 10:00:00')
insert into @Students values (2, 'Jerry', '2013-05-25 14:30:00')
insert into @Students values (3, 'Jane', '2013-05-26 10:30:00')

insert into @Sports values (1, 'Golf')
insert into @Sports values (2, 'Foot ball')

;with Students (RowNumber, StudentId, StudentName, EnrolledOn) as
(
    select row_number() over(order by EnrolledOn) as RowNumber, * from @Students
)
,Sports (RowNumber, SportId, SportName) as
(
    select row_number() over(order by SportId) as RowNumber, * from @Sports
)
select s1.StudentId, s1.StudentName, s1.EnrolledOn, s2.SportId, s2.SportName from Students s1
left join Sports s2
on s1.RowNumber = s2.RowNumber

I'm getting the expected output.

My goal is to assign the 1st sport to the student who enrolled in the first and second sport to the second student, and for the third student there will be no sport.

Will this be proper now?

Even if your solution is (almost) correct, the actual problem is in a wrong formulation of the problem.

The main issue in your question is the assumption of a certain order of records within a table. In reality SQL Server cannot guarantee original order of records when you do a select statement without order by clause, even if you have a clustered index defined.

In order to properly map 2 tables you must have a criteria for matching records. You provided a solution that won't generate consistent results. Imagine that you need to add another student/subject (StudentId = 2, SubjectId = 1) and a mark for that. If you add one record to Student/Subject table and one record to Marks table your data will be screwed up since Marks record will be matched against 3rd record (StudentId = 1, SubjectId = 3).

In order to fix the issue you must add a field or fields that allow unique reference of records, that is you must add both StudentId and SubjectId to Marks table. Alternatively you can create a surrogate key in the Student/Subject table and use it as a reference in Marks table.

UPDATE.
The main difference between original question and example in Edit section is that records can be ordered by unique key (primary key) in latter case, while in original question order by StudentId didn't guarantee specific order and produced inconsistent results.

Your updated example is using mapping properly, according to (updated) requirements, and will produce consistent results as long as SportId and StudentId are unique in corresponding tables


Heap Sort Implementation in Python Objective: Create a heap sort that returns a unsorted list to sorted from greatest to least.

Does this implementation look correct? Do you have any tips for optimizing this?

'''HeapSort. Best: n log(n) Average: n log(n) Worst: n log(n) '''

#Calls heapify and returns sorted list from greatest to least.
def heapSort(list):
    result = []
    while list:
        result.append(heapify(list))
    return result

def heapify(struct):
    for x in range(len(struct),0,-1):
    #Base Case: Returns last integer when sequence reaches below 2
        if len(struct) < 2:
            return struct.pop(0)

        #Compares Tree Root Node vs Children.
        if struct[0] < struct[1]:
            temp = struct[0]
            struct[0] = struct[1]
            struct[1] = temp
        else:
        #Parent: x/2. Child: x-1. Compares child nodes vs parent nodes in Tree.
            if struct[x-1] > struct[x/2]:
                temp = struct[x-1]
                struct[x-1] = struct[x/2]
                struct[x/2] = temp
    return struct.pop(0)

See §1.9 for the cause of the bug identified by janos.

1. Review

The code only runs on Python 2.7, because it uses the / operator for floor division. But support for Python 2.7 is scheduled to end in 2020. It is long past the time when it was a good idea to start writing new projects in Python 2.7. Even if you really can't use Python 3, it is still a good idea to use the floor division operator // to make the code portable.
There are no docstrings. What do these functions do? In the case of heapSort there is a comment that could be turned into a docstring.
heapSort modifies its input. If I run:

>>> a = [6, 7, 2, 4, 1, 0, 3, 9, 8, 5]
>>> heapSort(a)
[9, 8, 7, 6, 5, 4, 3, 2, 1, 0]

Then a is now empty:

>>> a
[]

This is likely to be surprising to someone using this code. It would be better to either leave the input unchanged (taking a copy if necessary), like the built-in function sorted, or to modify the input in-place, like the method list.sort.
The name struct could be improved: for experienced Python programmers this name suggests a fixed-size object of the kind that is manipulated by the struct module. A name like list or seq (for "sequence") would be clearer.
The base case does not depend on x and so it should be outside the loop.
Two values can be swapped without needing a temporary variable, if you use a tuple assignment:

struct[x-1], struct[x//2] = struct[x//2], struct[x-1]

The numbers x-1 and x//2 are computed three times. The repetition of x-1 can be avoided by changing the loop from:

range(len(seq),0,-1)

to:

range(len(seq) - 1, -1, -1)

which can be simplified to:

reversed(range(len(seq)))

and the repetition of x//2 can be avoided by assigning a local variable:

parent = (x + 1) // 2

(It need to be (x + 1) // 2 rather than x // 2 because of the change in the loop range.)
It's conventional use names i, j and so on for index variables (leaving x, y and so on for coordinates).
Normally the way heaps are implemented is that element \$i\$ is the parent of elements \$2i + 1\$ and \$2i + 2\$. However, in the code in the post, element \$i\$ is the parent of elements \$2i\$ and \$2i + 1\$. But this requires element 0 to be its own parent, which makes no sense! Presumably this is why you felt the need to add the special case:

if struct[0] < struct[1]:

But this special case led to the bug.

If you changed the implementation so that it used the usual convention for the layout of the heap, then this special case would be unnecessary. Moreover, if you did this then the special case:

if len(struct) < 2:

would also be unnecessary.
heapify currently does two things: it converts the input sequence into a max-heap, in-place, and it pops the first (maximum) item. It would be clearer, I think, to separate these two features, so that heapify only does the heap-conversion, leaving the popping to the caller.

2. Revised code

def heap_sorted(iterable):
    "Return new list of all items from iterable in descending order."
    result = []
    seq = list(iterable)
    while seq:
        heapify(seq)
        result.append(seq.pop(0))
    return result

def heapify(seq):
    "Transform sequence into a max-heap, in-place."
    for i in reversed(range(1, len(seq))):
        parent = (i - 1) // 2
        if seq[i] > seq[parent]:
            seq[i], seq[parent] = seq[parent], seq[i]

3. Performance

The docstring says:

  HeapSort. Worst: \$n \log n\$

But it is easy to see that this is not the case. That's because heapify is called \$n\$ times, and on the \$i\$th call heapify loops over all the remaining \$n-i\$ elements of the sequence, and calls struct.pop(0) which also takes time proportional to the length of struct. So the overall runtime is proportional to $$ \sum_{0\le i< n} n - i = {n(n + 1)\over2} $$ which is \$Θ(n^2)\$.

To get the runtime down to \$O(n \log n)\$, you need to heapify just once, and then instead of extracting the maximum item by popping the list, swap the maximum item with the last item in the list, and then apply the "sift down" procedure to repair the heap condition in time \$O(\log n)\$.


Array view adaptor class for static polymorphism over network header parsing Some context: I have some existing code to populate network header structs from istreams. A motivating excerpt:

struct l2_eth_frame {
    using mac_address_t = std::array<std::uint8_t, 6>;
    mac_address_t dest, src;
    std::uint16_t type;
    std::uint32_t vlan;
};

template<typename StreamLike, typename T>
void read_ntoh(StreamLike &is, T &t){
    static_assert(std::is_integral<T>::value, "fixed width integer required");
    is.read(reinterpret_cast<char*>(&t), sizeof(T));

    uint8_t *p = reinterpret_cast<uint8_t*>(&t);
    std::reverse(p, p + sizeof(T));
}

template<class StreamLike>
l2_eth_frame read_l2_eth_frame(StreamLike &i)
{
    l2_eth_frame e;
    i.read(reinterpret_cast<char*>(&e.dest), sizeof(e.dest));
    i.read(reinterpret_cast<char*>(&e.src), sizeof(e.src));
    read_ntoh(i, e.type);
    if(e.type == 0x8100){
        i.read(reinterpret_cast<char*>(&e.vlan), sizeof(e.vlan));
    }
    return e;
}

int main() {
     std::ifstream ifs("test.pcap");
     auto eth = read_l2_eth_frame(ifs);
}

I'm now trying to reuse this code to also be able to generate network structs from live data (e.g. from a raw socket). To achieve this I wrote a simplistic array_view class and also an adaptor class that gives it read and ignore methods (like an istream) so that it can utilise static polymorphism in the read_{header_type} methods.

array_view

#pragma once

#include <cassert>
#include <memory>
#include <array>

template <class T>
class basic_array_view {

private:
    const T* array;
    std::size_t len;

public:
    static constexpr std::size_t npos = -1;

    basic_array_view() noexcept :
        array(nullptr),
        len(0)
    {}

    basic_array_view(const T* array, std::size_t len) noexcept :
        array(array),
        len(len)
    {}

    template<std::size_t N>
    basic_array_view(const T (& a)[N]) noexcept :
        array(std::addressof(a[0])),
        len(N)
    {}

    template<std::size_t N>
    basic_array_view(const std::array<T, N> &a) noexcept :
        array(a.data()),
        len(N)
    {}

    basic_array_view(const basic_array_view &other) = default;
    basic_array_view& operator=(const basic_array_view &other) = default;

    constexpr std::size_t size() const noexcept {
        return len;
    }

    const T& operator[](std::size_t pos) const noexcept {
        assert(pos < len);
        return *(array + pos);
    }

    const T* data() const noexcept {
        assert(len > 0);
        return array;
    }

    basic_array_view<T> subview_right(std::size_t pos=0, std::size_t count=npos){
        assert(pos <= len);
        return { array + pos, std::min(count, len - pos) };
    }
};

using array_view = basic_array_view<char>;

StreamLikeArrayView

#include "array_view.h"

class StreamLikeArrayView {

public:
    array_view av;

    template<class... Args>
    StreamLikeArrayView(Args&&... args) :
        av(std::forward<Args>(args)...) {}

    void read(char *dest, std::size_t len){
        std::copy(av.data(), av.data() + len, dest);
        av = av.subview_right(len, array_view::npos);
    }

    void ignore(std::size_t len){
        av = av.subview_right(len, array_view::npos);
    }
};

usage

std::array<char, 4096> buf;
const auto bytes_read = ::recv(sd, buf.data(), buf.size(), 0); //assume we have set up some raw socket sd
StreamLikeArrayView slav{buf.data(), bytes_read};
const auto eth = read_l2_eth_frame(slav);

I'm particularly interested in a review of the StreamLikeArrayView abstraction, and whether there's some superior way to reuse the parsing code across istreams and arrays.

Could be more generic

Let's consider one of your constructors:

template<std::size_t N>
basic_array_view(const std::array<T, N> &a) noexcept :
    array(a.data()),
    len(N)
{}

I don't see any particularly good reason this should/must be restricted to being used with an std::array. I think I'd write the code more like this:

template<class Container>
basic_array_view(Container const &a) noexcept :
    array(a.data()),
    len(a.length())
{}

Now it can be used with an std::array or other containers like std::vector, std::deque, etc.

Incomplete

The basic point of something like an array_view is (at least usually) to provide the same interface as existing collections. It does provide a substantial part of that, but it's missing a number of things like as the normal typedefs for value_type, reference_type, iterator_type, and so on. It's also missing iterator access to the contents; given that you're dealing with a contiguous array, that can be provided quite trivially (begin() and end() that return pointers to beginning and one past the end respectively). The latter would allow use with standard algorithms and such as well.

Naming

I'd try to pick one naming convention and stick with it. Either snake_case throughout or PascalCase throughout--but combining array_view with StreamLikeArrayView is somewhat jarring.

I also think subview_right is kind of a poor name--it seems as if subview is entirely adequate.

Duplication

In some ways this is the biggest question of all. You could get the functionality of your StreamLikeArrayView by simply dumping the data into a stringstream, and letting your existing code read from there. The (primary?) gain is that you avoid copying the data as you would with a normal stringstream.

If your real intent is to provide functionality like a stringstream's, but avoiding copying by having it use a buffer that isn't owned by the streambuf, then I think it would be better to provide that more directly:

struct array_buffer : public std::streambuf {
    array_buffer(char const *begin, char const *end) {
        char *b = const_cast<char *>(begin);
        char *e = const_cast<char *>(end);

        setg(b, b, e);
    }

    template <class Container>
    array_buffer(Container const &c) : array_buffer(&*std::begin(c), &*std::end(c))
    { }
};

[I should add: this is also somewhat incomplete--it doesn't try to support all possible stream buffer operations, but it supports at least as much as the classes above.]

To go with that, we can provide a stream class to save the user some work in constructing a stream based on our buffer type:

class buffer_stream : public std::istream {
    array_buffer buf;
public:
    template<class... Args>
    buffer_stream(Args&&... args) : buf(std::forward<Args>(args)...), std::istream(&buf)
    { }
};

To go with that, some demo code to show how we can build an istream of this type from any of a number of sources, and how the usual ways of reading from an istream work with this istream:

int main() {
    char input[] = "This is some input";

    // Create a buffer stream reading from an array
    buffer_stream b(input);

    std::string s;

    // Do some normal stream reading from it:
    while (b >> s)
        std::cout << s << "\n";

    // Create another from an `std::array`, read some from it:
    // Sorry, but I'm too lazy to hand-craft a very interesting array here:
    std::array<char, 3> more_input { 65, 66, 67 };

    buffer_stream c(more_input);

    while (c >> s)
        std::cout << s << "\n";

    // Create one from a string and read from it:
    std::string still_more_input { "This is the last piece of input" };

    buffer_stream d(still_more_input);

    // Read the first few words:
    char buffer[11];
    d.read(buffer, sizeof(buffer));
    std::cout.write(buffer, sizeof(buffer));
    std::cout << "\n";

    // Skip " last "
    d.ignore(6);

    // Then read the remainder into a string:
    std::getline(d, s);
    std::cout << s << "\n";
}

The obvious advantage of providing a class derived from istream is that it works with essentially everything that reads from an istream, not just with a couple of your own select functions that template the stream-like object, and restrict themselves to two specific ways of reading from the stream.

The obvious disadvantage of this approach is that is probably adds more overhead, and some people (especially anybody who hasn't delved into streams, buffers, etc.) may find it harder to understand. There's also the fact that by being a stream buffer class, people may assume this supports everything that any stream buffer might, which is a bit more than it actually delivers.


Parse data into three logs In the bit of code below:

How can I make add_day, add_ week and add_month work as a single function? They are almost copy-paste of one another. I'm trying to follow the DRY rule.
How can I set this so that I don't use the variables day/week/cur_day?

import datetime
from collections import defaultdict

class ParsedData:
    def __init__(self, date, score, status, item_name):
        year, month, self.day = date.split("-")
        self.month = datetime.date(int(year), int(month), int(self.day)).strftime("%B")
        self.score = int(score)
        self.status = status
        self.item_name = item_name

class Log:
    def __init__(self):
        self.day = {}
        self.week = {}
        self.month = {}
        self.counter = {}

        self.default_data

    def add_day(self, day, status, item_name, score):
        day = 'Day %i' % day
        if day not in self.day:
            self.day[day] = {"Comp": defaultdict(int),
                         "Miss": defaultdict(int),
                         "Post": defaultdict(int),
                         "Add": defaultdict(int),
                         "Score": 0}

        self.day[day][status][item_name] += 1
        self.day[day]['Score'] = score

    def add_week(self, week, status, item_name, score):
        week = 'Week %i' % week
        if week not in self.week:
            self.week[week] = {"Comp": defaultdict(int),
                         "Miss": defaultdict(int),
                         "Post": defaultdict(int),
                         "Add": defaultdict(int),
                         "Score": 0}

        self.week[week][status][item_name] += 1
        self.week[week]['Score'] = score

    def add_month(self, month, status, item_name, score):
        if month not in self.month:
            self.month[month] = {"Comp": defaultdict(int),
                         "Miss": defaultdict(int),
                         "Post": defaultdict(int),
                         "Add": defaultdict(int),
                         "Score": 0}

        self.month[month][status][item_name] += 1
        self.month[month]['Score'] = score

def update_views(log_file):
    log = Log()
    day = 0
    cur_day = None
    week = 1

    for line in log_file:
        parsed = ParsedData(*line.strip().split(","))

        if cur_day != parsed.day:
            cur_day = parsed.day
            day += 1
            if day % 7 == 0:
                week += 1

        log.add_day(day, parsed.status, parsed.item_name, parsed.score)
        log.add_week(week, parsed.status, parsed.item_name, parsed.score)
        log.add_month(parsed.month, parsed.status, parsed.item_name, parsed.score)

log_file =   """2015-01-1,0,Add,DW_05
                2015-01-2,-1,Post,CR_02
                2015-01-3,-1,Comp,DIY_01
                2015-01-3,-1,Post,CD_01
                2015-01-4,-1,Miss,D_03
                2015-01-4,0,Miss,D_03
                2015-01-4,-1,Miss,CD_01
                2015-01-4,0,Miss,LS_04
                2015-01-5,1,Comp,DW_05
                2015-01-6,1,Comp,ANI_06
                2015-01-6,1,Comp,LS_04
                2015-01-7,1,Comp,NMW_07
                2015-01-7,1,Post,DW_05
                2015-01-7,1,Miss,LP_08
                2015-01-8,2,Post,CR_02
                2015-01-8,2,Miss,SEV_09
                2015-01-10,3,Comp,M_10
                2015-01-10,3,Add,NID_11
                2015-01-11,2,Add,ANI_06
                2015-01-12,1,Add,VF_12
                2015-01-12,0,Miss,DIY_01
                2015-01-12,1,Add,NID_11
                2015-01-12,0,Miss,D_03
                2015-01-13,1,Miss,SEV_09
                2015-01-13,2,Add,DW_05
                2015-01-13,1,Comp,NMW_07
                2015-01-13,1,Add,CPC_12""".splitlines()

update_views(log_file)

To avoid repetitive code, make the Log class do only one of the three things it currently does, and create three instances instead:

class Log(object):
    def __init__(self, name):
        self.name = name
        self.data = {}

    def add_item(self, key, status, item_name, score):
        key = '%s %i' % (self.name, key)
        if key not in self.data:
            self.data[key] = {"Comp": defaultdict(int),
                         "Miss": defaultdict(int),
                         "Post": defaultdict(int),
                         "Add": defaultdict(int),
                         "Score": 0}

        self.data[key][status][item_name] += 1
        self.data[key]['Score'] = score

def update_views(log_file):
    daily_log = Log('Day')
    weekly_log = Log('Week')
    monthly_log = Log('Month')
    ...


Sum array of objects by unique keys I have an array of objects, each with a dimensions field for storing various metrics related to the object, e.g. length, width, weight, etc.  Each object may be different in terms of what metrics it has.

I have a solution but not sure if it's optimal.

let dimensions = [{ a: 1, b: 2, c: 3}, { a: 2, c: 1, d: 3 }, { b: 1, e: 8, f: 1 }]

// get all keys
let dimensionsFields = _.map(dimensions, Object.keys);
// => [[a, b, c], [a, c, d], [b, e, f]]

// flatten
dimensionsFields  = _.flatten(dimensionsFields );
// => [a, b, c, a, c, d, b, e, f]

// filter out duplicates
dimensionsFields  = _.uniq(dimensionsFields );
// => [a, b, c, d, e, f]

// init return data
let dimensionsData = {};

// loop through available metrics, add up the values
dimensionsFields.forEach(field => {
  dimensionsData[field] = _.reduce(dimensions, function(a, b) {
    return a + b[field];
  }, 0);
});
// => { a: 3, b: 3, c: 4, d: 3, e: 8, f: 1 }

What do you mean optimal?

Your thing transforms the object lots of times, which is liable to not be very efficient, if that's what you meant.

Here's something that I hope is simple to read that does your task.

function sumObjects(obj1, obj2) {
    return Object.keys(obj2).reduce(
        (acc, currKey) => ({
        ...acc,
        [currKey]: acc[currKey] ? acc[currKey] + obj2[currKey] : obj2[currKey]
        }),
        obj1
    );
}

const result = dimensions.reduce(sumObjects);

The observation is that you just need to see how to do your task for two objects and the rest is just a reduce.

The most efficient thing will likely be just a for loop, like

const res = {};
for (const obj of dimensions) {
    for (const key in obj) {
        res[key] = res[key] ? res[key] + obj[key] : obj[key];
    }
}


Minimize boilerplate code in Action class I perform an if-check (commented in the code) to determine whether a user can access certain actions or not. The user will be redirected to the index page if they try to access the login.action while already logged in.

I have this kind of if-check in many of my Action classes (containsKey() or !containsKey(), depending on the action) and it is looking very redundant.

How can I improve on what I am trying to achieve?

public String execute() throws Exception {
    // redirect if user is already logged in
    if(session.containsKey("currentId")) {
        return "index";
    }

    Integer currentId = mm.getCurrentId(member.getMemberId());
    String currentAccessType = mm.getMemberInfo("member_access_type", member.getMemberId());

    System.out.println("Current ID: " + currentId);
    System.out.println("Current Access Type: " + currentAccessType);

    session.put("currentId", currentId);
    session.put("currentAccessType", currentAccessType);

    System.out.println("You have logged in!");

    return SUCCESS;
}

Aspect-Oriented programming would seem to be the way to go here. Since you're using Struts2, use Interceptors to wrap around the relevant actions. Inside the interceptors, do your check and redirect. You want two, one for the affirmative, and  one for the negative.


XML document parser with XPath Here is my simple implementation on XML parser for XML which I received from a server response. Also I've tried to use XmlPathDocument but failed to get child elements for a node in XmlPathIterator.

    public OutboxResponse CheckSendResponseForOutboxPacket(Outbox outbox)
    {
        var responseContent = outbox.Outbox_content.FirstOrDefault().response_content;

        OutboxResponse outboxResponse = new OutboxResponse();
        outboxResponse.Violations = new List<OutboxResponseViolation>();

        XmlDocument responseDocument = new XmlDocument();
        responseDocument.LoadXml(responseContent);
        var rootElement = responseDocument.DocumentElement;

        //XPath templates
        string responseResult = "r:body/r:result/text()";
        string violationCode = "g:body/g:code/text()";
        string violationLevel = "g:body/g:level/text()";
        string violationName = "g:body/g:name/text()";
        string violationDescription = "g:body/g:violations/g:violation";
        string itemResultResult = "g:body/g:itemResults/g:itemResult/g:result/text()";
        string itemResultViolations = "g:body/g:itemResults/g:itemResult/g:violations/g:violation";

        XmlNamespaceManager nsmanager = new XmlNamespaceManager(responseDocument.NameTable);
        nsmanager.AddNamespace("g", "http://google.com");

        switch (rootElement.SelectSingleNode(responseResult, nsmanager).Value)
        {
            case "success":
                outboxResponse.Result = "Packet received without errors";

                foreach (XmlNode violation in rootElement.SelectNodes(itemResultViolations, nsmanager))
                {
                    outboxResponse.Violations.Add(new OutboxResponseViolation
                    {
                        Level = violation.SelectSingleNode("g:level/text()", nsmanager).Value,
                        Name = violation.SelectSingleNode("g:name/text()", nsmanager).Value,
                        Description = violation.SelectSingleNode("g:description/text()", nsmanager).Value

                    });

                }

                break;
            case "failure":
                outboxResponse.Result = "Errors was found in packet";

                foreach (XmlNode violation in rootElement.SelectNodes(violationDescription, nsmanager))
                {
                    outboxResponse.Violations.Add(new OutboxResponseViolation
                    {
                        Level = violation.SelectSingleNode("r:level/text()", nsmanager).Value,
                        Name = violation.SelectSingleNode("r:name/text()", nsmanager).Value,
                        Description = violation.SelectSingleNode("r:description/text()", nsmanager).Value

                    });

                }

                break;
        }

        return outboxResponse;

    }

Based on the naming guidelines properties shouldn't be named using some kind of Snake_Case casing.  They should be named using PascalCase casing.

Without seeing OutboxResponse's  implementation and its other usages, I would suggest initializing the Violations property in the class itself.

The loops inside the switch cases are very similiar. They only differ by the used "prefix" e and r. These should be extracted to a separate method to reduce code duplication.

You have a lot of vertical space wasted by using to many new lines. This reduces readability.

English isn't my first language so this could be wrong, but I guess "Errors was found in packet" should be "Errors were found in packet".

These variables

string violationCode = "g:body/g:code/text()";
string violationLevel = "g:body/g:level/text()";
string violationName = "g:body/g:name/text()";

string itemResultResult = "g:body/g:itemResults/g:itemResult/g:result/text()";

aren't used and should be removed.

You should always declare variables as near as possible to their usage.

By extracting the creation of the XMLDocument to a separate method you can reduce the size of the method which adds readability.

If you need a comment to describe for what a variable stands then this variable isn't  properly named.

After applying the mentioned points the refactored method we would get

public OutboxResponse CheckSendResponseForOutboxPacket(Outbox outbox)
{
    XmlDocument responseDocument = GetXMLDocument(outbox);

    XmlNamespaceManager nsmanager = new XmlNamespaceManager(responseDocument.NameTable);
    nsmanager.AddNamespace("g", "http://google.com");

    var rootElement = responseDocument.DocumentElement;

    OutboxResponse outboxResponse = new OutboxResponse();
    outboxResponse.Violations = new List<OutboxResponseViolation>();

    string prefix;
    string xpath;
    string resultPath = "r:body/r:result/text()";

    switch (rootElement.SelectSingleNode(resultPath, nsmanager).Value)
    {
        case "success":
            outboxResponse.Result = "Packet received without errors";
            xpath = "g:body/g:itemResults/g:itemResult/g:violations/g:violation";
            prefix = "g";
            break;

        case "failure":
            outboxResponse.Result = "Errors were found in packet";
            xpath = "g:body/g:violations/g:violation";
            prefix = "r";
            break;

        default:
            return outboxResponse;

    }

    XmlNodeList nodes = rootElement.SelectNodes(xpath, nsmanager);
    outboxResponse.Violations.AddRange(GetOutputViolations(nodes, nsmanager, prefix));

    return outboxResponse;

}
private XmlDocument GetXMLDocument(Outbox outbox)
{
    var content = outbox.Outbox_content.FirstOrDefault().response_content;
    XmlDocument document = new XmlDocument();
    document.LoadXml(content);
    return document;
}
private IList<OutboxResponseViolation> GetOutputViolations(XmlNodeList nodes, XmlNamespaceManager nsmanager, string prefix)
{
    IList<OutboxResponseViolation> violations = new List<OutboxResponseViolation>();
    string levelPath = prefix + ":level/text()";
    string namePath = prefix + ":name/text()";
    string descriptionPath = prefix + ":description/text()";

    foreach (XmlNode node in nodes)
    {
        violations.Add(new OutboxResponseViolation
        {
            Level = node.SelectSingleNode(levelPath, nsmanager).Value,
            Name = node.SelectSingleNode(namePath, nsmanager).Value,
            Description = node.SelectSingleNode(descriptionPath, nsmanager).Value
        });
    }
    return violations;
}


Anchoring an element to the browser's vertical scrollbar The problem

I'm trying to position a chosen element so that it's always horizontally aligned against the right-hand side of the page, and vertically aligned with the center of the web browser's vertical inner scrollbar, i.e. the part of the scrollbar that moves as you scroll up and down the page. The "handlebar", if you will.
I've been tweaking this code for so long that it's starting to do my head in, so would like a sanity check, i.e. am I going about this the right way?
The key part of this function is the line beginning offset = scroll_position ...
The code
barFollower = function (e) {
  var target = $(e.data.target),
    scrollbar_button_height = 20,
    window_height = $(window).height(),
    window_height_adj = window_height - (scrollbar_button_height * 2),
    scroll_position = $(window).scrollTop(),
    body_height = $('body').height(),

  offset = scroll_position // The top of window
    + ((scroll_position / body_height) * window_height_adj) // The position of the top of scrollbar
    + (((window_height_adj / body_height) * window_height_adj) / 2) // Half the height of the scrollbar
    + scrollbar_button_height // The scrollbar button height (depends on browser chrome, unfortunately) - see notes below
    - element_offset; // offset for chosen 'position' based on element height - see notes below

  /* Some catches for when element height might mean it would get positioned outside the window */
  if (body_height < window_height) { // If there's no scrollbar.
    offset = (window_height / 2) + (target.height() / 2); // Position halfway down the window
  } else if (offset < scroll_position) { // Top bounds
    offset = scroll_position; // Fix it to the top of the window
  } else if (offset > scroll_position + window_height - target.height()) { // Bottom bounds
    offset = scroll_position + window_height - target.height(); // Fix it to the bottom of the window
  }
  target.css('top', offset);
};

Notes
Triggering
This function is triggered once on page load, and subsequently on every window scroll event.
Scrollbar button size
The tricky part here is that I can't find a way to determine the height of the scrollbar 'up' and 'down' buttons, so I have made an assumption (20px), although this will potentially vary from browser to browser, and depends the browser chrome.
If anyone knows a way to accurately determine scrollbar button sizes, please let me know.
The element_offset variable
The element_offset is set outside this function, and can have one of three values to offset the element so that either the top, middle, or bottom of the element align with the middle of the vertical scrollbar. How it is set is not relevant at this point, so I've left it out to keep things simple.

What is wrong with:

    ...

This is the element

css:

#myfloatingElement
{
    position: fixed;
    right: 0;
    top: 50%;
}


Recreating a URL string This is what my viewObject looks like:

{
    description: "GOOG AAPL TSLA"
    id: 42
    ticker_1: "GOOG"
    ticker_2: "AAPL"
    ticker_3: "TSLA"
    timespan: "1mo"
}

Here is the function use to check the object. In a very dirty fashion, it creates a string to be used to replace the URL with:

function createUrl(viewObj) {
    var urlString  = '',
        ticker1    = '',
        ticker2    = '',
        ticker3    = '',
        time       = '';

    if (viewObj.ticker_1) {
        ticker1 = '&ticker_1='+viewObj.ticker_1;
        ulrString = ulrString+ticker1;
    }
    if (viewObj.ticker_2) {
        ticker2 = '&ticker_2='+viewObj.ticker_2;
        ulrString = ulrString+ticker2;
    }
    if (viewObj.ticker_3) {
        ticker3 = '&ticker_3='+viewObj.ticker_3;
        ulrString = ulrString+ticker3;
    }

    if (viewObj.timespan) {
        time = '&timespan='+viewObj.timespan;
        ulrString = ulrString+time;
    }

    console.log('/dashboard'+ulrString);
    return '/dashboard'+ulrString;
}

I have multiple if/else statements and code that can be repeated. How would you accomplish this?

Do you mean write code that would repeat the multiple if/else statements? cause that would be the right idea and make your code more DRY.

The first thing to take a look at is your data structure. As it is, you have hard-coded a handful of ticker properties that would make iteration difficult. You could use a for/in loop to go through the properties of your object, but we know we don't want the description or id in the string. Better to put all tickers in an array. That way it'll be easy to create a string with as many (or little) tickers as you want.

{
    description: "GOOG AAPL TSLA"
    id: 42
    tickers: ["GOOG", "AAPL", "TSLA"]
    timespan: "1mo"
}

It's now easy to iterate over the tickers and construct a string out of them. Since we always start with ticker_<n> as the beginning, we can construct that inside our loop, using the index to count which ticker # we are on.

For the function:

function createUrl(viewObject){
    // use .map to add "ticker_<n>=<value>" to each of the tickers,
    // then join with "&" for the query.
    var urlString = viewObject.tickers
        .map(function(ticker, index){
             // prepend the first query with its own '&'
            var and = index === 0 ? "&" : "";

            //create a 1-indexed count of the tickers.
            var tickerNum = index + 1;
            return and + "ticker_" + tickerNum + "=" + ticker;
        })
        .join('&'); //<--separate each query with "&"

    // for timespan, you don't need to make a seperate `time` variable,
    // this can be done in one line.
    if(viewObject.timespan) urlString += "&timespan=" + viewObject.timespan;

    console.log('/dashboard'+urlString);
    return '/dashboard'+urlString;
}


Algorithm for comparing two lists of objects, to insert added items, and delete removed items I use two lists of objects invoices, original list and a new modified list. I would insert in db invoices presents in the new modified list but not presents in the original list.

For delete, I would remove from db invoices presents in the original list but absent in the new modified list.

This is what I did and that works, but I am not satisfied with the algorithm used, someone would have a way to do this more simply and clearly?

public void updateCustomerInvoicesCouple(List<CustomerInvoicesDto> originalCouplesListDto, List<CustomerInvoicesDto> newCoupleListDto){

    // 1 - Insert
    Iterator<CustomerInvoicesDto> itFinal = newCoupleListDto.iterator();

    // Browse all couples (customer-invoice) from final list
    // Insert couples of final list not in original list
    while(itFinal.hasNext()){
        CustomerInvoicesDto finalCoupleDto = (CustomerInvoicesDto) itFinal.next();

        Integer idCustomerFinal = finalCoupleDto.getCustomer().getId();
        Integer idInvoiceFinal = finalCoupleDto.getInvoice().getId();

        boolean coupleFind = false;
        for(CustomerInvoicesDto itemInitialCouple : originalCouplesListDto)
        {
            if(idCustomerFinal == itemInitialCouple.getCustomer().getId()
                    && idInvoiceFinal == itemInitialCouple.getInvoice().getId())
            {
                coupleFind = true;
            }
        }

        if(!coupleFind){
            // dto -> entity
            CustomerInvoicesEntity finalCoupleEntity = new CustomerInvoicesEntity();
            finalCoupleEntity = mapper.map(finalCoupleDto, CustomerInvoicesEntity.class);

            // Save in db
            CustomerInvoicesDao.save(finalCoupleEntity);
        }
    }

    // 2 - Delete
    Iterator<CustomerInvoicesDto> itInit = originalCouplesListDto.iterator();

    // Browse all couples (customer-invoice) from original list
    // Insert couples of final list not in final list
    while(itInit.hasNext()){
        CustomerInvoicesDto initialCoupleDto = (CustomerInvoicesDto) itInit.next();

        Integer idCustomerInitial = initialCoupleDto.getCustomer().getId();
        Integer idInvoiceInitial = initialCoupleDto.getInvoice().getId();

        boolean coupleFind = false;
        for(CustomerInvoicesDto itemFinalCouple : newCoupleListDto)
        {
            if(idCustomerInitial == itemFinalCouple.getCustomer().getId()
                    && idInvoiceInitial == itemFinalCouple.getInvoice().getId())
            {
                coupleFind = true;
            }
        }

        if(!coupleFind){
            CustomerInvoicesDao.remove(initialCoupleDto.getId());
        }
    }
}

1) In your for-loop, when the if is true, you should not only set coupleFind = true, but also add a break since you keep looping for nothing otherwise.

2) I'm not sure why the customerId is not already part of the invoice.

3) Instead of looping over the lists, you could just create a HashSet and check if the invoice is already present. There is some extra complexity if you take this approach, but if you call that method often, it will definitely be cheaper computationally.  You should define some class InvoiceIncludingCustomerId if you don't want to put the customerId within the invoice.  And you should override (carefully) the hashCode() and equals() of that class so that the hash is unique, but quick to compute: invoiceId and customerId should be sufficient.

You then just need a HashSet<InvoiceIncludingCustomerId> knownInvoices and you just check if an invoice exists with knownInvoices.contains(invoice).  If it is missing, you add it to the DB and knownInvoices.add(invoice).


SFML game using moving circles This is my first SFML game. There are 2 players: the red circle and the yellow circle. The red circle is controlled by Up,down,left and right keys, the yellow one is controlled by WASD keys. The players are supposed to keep on trying to get behind each other in order to move them and get them inside the black circle, the player who gets in the circle first loses.

#include <iostream>
#include <SFML/Graphics.hpp>

bool intersects(const sf::CircleShape& c1, const sf::CircleShape& c2){
    sf::FloatRect circ1 = c1.getGlobalBounds();
    sf::FloatRect circ2 = c2.getGlobalBounds();

    return circ1.intersects(circ2);
}

int main(){

    sf::VideoMode videomode(400, 400);
    sf::RenderWindow window(videomode, "GAME");
    sf::CircleShape circle;
    circle.setFillColor(sf::Color::Red);
    circle.setPosition(100, 100);
    circle.setRadius(20);
    sf::CircleShape circle2;
    circle2.setFillColor(sf::Color::Yellow);
    circle2.setPosition(200, 200);
    circle2.setRadius(20);
    sf::CircleShape circle3;
    circle3.setFillColor(sf::Color::Black);
    circle3.setPosition(300, 300);
    circle3.setRadius(50);

    while (window.isOpen()&&(!intersects(circle,circle3))&&(!intersects(circle2,circle3))){
        window.clear(sf::Color::Blue);
        window.draw(circle);
        window.draw(circle2);
        window.draw(circle3);

        window.display();

        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Up)){
            circle.move(0, -0.5);

        }

        else if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Down)){
            circle.move(0, 0.5);

        }

        else if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Right)){
            circle.move(0.5, 0);

        }

        else if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Left)){
            circle.move(-0.5, 0);

        }

        else if (sf::Keyboard::isKeyPressed(sf::Keyboard::W)){
            circle2.move(0, -0.5);

        }

        else if (sf::Keyboard::isKeyPressed(sf::Keyboard::S)){
            circle2.move(0, 0.5);

        }

        else if (sf::Keyboard::isKeyPressed(sf::Keyboard::D)){
            circle2.move(0.5, 0);

        }

        else if (sf::Keyboard::isKeyPressed(sf::Keyboard::A)){
            circle2.move(-0.5, 0);

        }

        if (intersects(circle, circle2)){
            circle2.move(0.5, 0);
        }
        if (intersects(circle2, circle)){
            circle.move(0.5, 0);

        }
        if (intersects(circle, circle3)){

            std::cout << "Red loses";
        }
        if (intersects(circle2, circle3)){

            std::cout << "Yellow loses";
        }
    }
}

I do not know SFML so here some non SFML comments.

First of all - codding style.

You need to indent the code. It is very hard to read it.
Be consistent you are mixing coding styles - you are open { on different "places"

int main(){ <== open here

   while (window.pollEvent(event))
   {  <== and now here
       if (event.type == sf::Event::Closed)
           window.close();
   }

main() function is very long.

try separate the function into several small functions,
for example init_game, play_game end_game and so on.

DRY - don't repeat yourself.

consider this code:

sf::CircleShape circle;
circle.setFillColor(sf::Color::Red);
circle.setPosition(100, 100);
circle.setRadius(20);

sf::CircleShape circle2;
circle2.setFillColor(sf::Color::Yellow);
circle2.setPosition(200, 200);
circle2.setRadius(20);

sf::CircleShape circle3;
circle3.setFillColor(sf::Color::Black);
circle3.setPosition(300, 300);
circle3.setRadius(50);

it is basically same thing three times. I would do:

// to do fix color_type
sf::CircleShape createCircleShape(color_type color, int x, int y, int r ){
   sf::CircleShape c;

   c.setFillColor(color);
   c.setPosition(x, y);
   c.setRadius(r);

   return c;
}

// ...
sf::CircleShape circle  = createCircleShape(sf::Color::Red, 100, 100, 20);
sf::CircleShape circle2 = createCircleShape(sf::Color::Yellow, 200, 200, 20);
sf::CircleShape circle3 = createCircleShape(sf::Color::Black, 300, 300, 20);

this way it is much more readable.

You can do same for controlling the keyboard:

if ((event.type == sf::Event::KeyPressed) && (event.key.code == sf::Keyboard::Up)){
    circle.move(0, -0.5);
}

you can write a function that get four keyboard codes, event and circle and you can do something like this:

void controlPlayer(const event_key_code &keycode, sf::CircleShape &circle,
           const key_code_type up,
           const key_code_type down,
           const key_code_type left,
           const key_code_type right);
// then you do:

if (event.type == sf::Event::KeyPressed){
        controlPlayer(event.key.code, circle, sf::Keyboard::Up, sf::Keyboard::Down, sf::Keyboard::Left, sf::Keyboard::Right);
        controlPlayer(event.key.code, circle2, sf::Keyboard::W, sf::Keyboard::S, sf::Keyboard::A, sf::Keyboard::D);
}

Hope this helps for start!


Creative triangle stacks using for loops I was wondering if someone can help me out with achieving the following result (using for loops):

|../\..../\..|
|./\/\../\/\.|
|/\/\/\/\/\/\|

This is the code that I have written:

public static void drawBody1(){
  for (int line = 1; line <= 3; line++){
      System.out.print("|");
      for (int dot = 1; dot <= (-1 * line + 3); dot++){
        System.out.print(".");
      }
      for (int mount = 1; mount <= line; mount++){
        System.out.print("/\\");
      }
      for (int dot = 1; dot <= (-2 * line + 6); dot++){
        System.out.print(".");
      }
      for (int mount = 1; mount <= line; mount++){
        System.out.print("/\\");
      }
      for (int dot = 1; dot <= (-1 * line + 3); dot++){
        System.out.print(".");
      }
      System.out.print("|");
      System.out.println();
  }

Although I did get the figure above, I realize that the code abounds in redundancy and is, thus, far from efficient.

I was hoping if someone can show me a simple yet elegant solution to this problem.

I created a method, so it is easy to adjust the height of the pyramid, and the output it gives is just as you asked. This code is shorter and easier to understand in my opinion(maybe just because I wrote it), but the main difference is that it is easy to change the height of your pyramid. I named my function drawMounts(int height). Try changing height in the main method, and you will see that the pyramids easily adjust. This program makes one line at a time, prints it and repeats but the line changes slightly. height can be even or odd, it works the same. Here's the program:

public class Experiments {
    public static void main(String[] args) {
        drawMounts(3);
    }
    private static void drawMounts(int height) {
        for (int i = 0; i < height; i++) {
            String line = "";
            String ends = "";
            String middle = ".";
            String mount = "";
            System.out.print("|");
            for(int j = i; j < (height-1); j++)
                middle += "..";
            for (int dot = i; dot < (height-1); dot++)
                ends += ".";
            for(int body = (i+1); body > 0; body--)
                mount += "/\\";
            line = ends + mount + middle + mount + ends;
            System.out.print(line);
            System.out.println("|");
        }
    }
}

And here's the output when height is equal to 3:

|../\..../\..|
|./\/\../\/\.|
|/\/\/\/\/\/\|

And this is when height is 6, just so you get an idea of what changing height does:

|...../\........../\.....|
|..../\/\......../\/\....|
|.../\/\/\....../\/\/\...|
|../\/\/\/\..../\/\/\/\..|
|./\/\/\/\/\../\/\/\/\/\.|
|/\/\/\/\/\/\/\/\/\/\/\/\|


Apriori algorithm using Pandas I want to optimize my Apriori algorithm for speed:

from itertools import combinations
import pandas as pd
import numpy as np

trans=pd.read_table('output.txt', header=None,index_col=0)

def apriori(trans, support=0.01, minlen=1):
    ts=pd.get_dummies(trans.unstack().dropna()).groupby(level=1).sum()

    collen, rowlen  =ts.shape

    #-------------Max leng (not used)
    #tssum=ts.sum(axis=1)
    #maxlen=int(tssum.loc[tssum.idxmax()])

    pattern = []
    for cnum in range(minlen, rowlen+1):
        for cols in combinations(ts, cnum):
            patsup = ts[list(cols)].all(axis=1).sum()
            patsup=float(patsup)/collen
            pattern.append([",".join(cols), patsup])
    sdf = pd.DataFrame(pattern, columns=["Pattern", "Support"])
    results=sdf[sdf.Support >= support]

    return results

When you input a dataframe of transactions:

>>> trans
        1  2    3    4
0
11      a  b    c  NaN
666     a  d    e  NaN
10101   b  c    d  NaN
1010    a  b    c    d
414147  b  c  NaN  NaN
10101   a  b    d  NaN
1242    d  e  NaN  NaN
101     a  b    c  NaN
411     c  d    e  NaN
444     a  b    c  NaN

[10 rows x 4 columns]

It will yield:

Ap=apriori(trans)
print Ap
>>>
   Pattern  Support
0        a      0.6
1        b      0.7
2        c      0.7
3        d      0.6
4        e      0.3
5      a,b      0.5
6      a,c      0.4
7      a,d      0.3
8      a,e      0.1
9      b,c      0.6
10     b,d      0.3
12     c,d      0.3
13     c,e      0.1
14     d,e      0.3
15   a,b,c      0.4
16   a,b,d      0.2
18   a,c,d      0.1
20   a,d,e      0.1
21   b,c,d      0.2
24   c,d,e      0.1

I want to know if this can be optimized further so that it can run faster on large datasets.  I also want to know if there a way to use purely Pandas without combinations from itertools.

First off, this is just a part of the Apriori algorithm. Here, you're just finding frequent itemsets. Apriori continues to find association rules in those itemsets.

Also, using combinations() like this is not optimal. For example, if we know that the combination AB does not enjoy reasonable support, we do not need to consider any combination that contains AB anymore (ABC, ABD, etc. will all be infrequent as well). Your algorithm does not take this into consideration.

This means that your algorithm will check all the possible combinations (2n, where n is the number of possible items), while in fact we can prune the search tree as above and reduce this complexity drastically (depending on the density of the dataset).


How can I improve my Java MethodPointer? This was just an experiment to see if I could replicate something like C++ function pointers in Java.  Basically, the idea was to have an object which represented a method call, and when you call .invoke() on that object, the method would run.

It feels a little sloppy to me, and I feel like there may be some tricks of Reflection and Generics that would make the code more elegant/usable which I haven't thought of or am not aware of.  Any tips or tricks?

import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.ArrayList;
import java.util.Arrays;

import org.apache.log4j.Level;
import org.apache.log4j.Logger;
import org.jpgohlke.util.logging.LogUtil;

public class MethodPointer<T, R> {

    //Logger instance
    private static final Logger LOGGER = Logger.getLogger(MethodPointer.class);

    //Object fields
    private final Method method;
    private final Class<R> returnClass;
    private ArrayList<Object> args = new ArrayList<Object>();
    private T object = null;

    //Constructors
    public MethodPointer(Method method, Class<R> returnClass) {
        this.method = method;
        this.returnClass = returnClass;
    }

    public MethodPointer(Class<T> clazz, T object, String methodName, Class<R> returnClass, Class<?> ... paramClasses) {
        Method theMethod = null;
        try {
            theMethod = clazz.getMethod(methodName, paramClasses);
        }
        catch(NoSuchMethodException nsme) {
            LogUtil.log(LOGGER, Level.ERROR, "Unable to find method " + methodName + " in " + clazz.getSimpleName(), nsme);
            throw new IllegalArgumentException("Method signature does not exist in " + clazz.getSimpleName());
        }
        method = theMethod;
        this.object = object;
        this.returnClass = returnClass;
    }

    public MethodPointer(Class<T> clazz, String methodName, Class<R> returnClass, Class<?> ... paramClasses) {
        this(clazz, null, methodName, returnClass, paramClasses);
    }

    //Accessors and mutators
    public Method getMethod() {
        return method;
    }

    public Class<R> getReturnClass() {
        return returnClass;
    }

    public Object[] getArguments() {
        return args.toArray();
    }

    public void setArguments(Object ... args) {
        this.args = new ArrayList<Object>(Arrays.asList(args));
    }

    public void setArguments(ArrayList<Object> args) {
        this.args = args;
    }

    public T getObject() {
        return object;
    }

    public void setObject(T object) {
        this.object = object;
    }

    //Invoking the method
    public R invoke() throws Exception {

        if(Modifier.isStatic(method.getModifiers())) {
            object = null;
        }
        else if(object == null) {
            LOGGER.error("An object must be provided to invoke a non-static method upon.");
            throw new IllegalArgumentException("An object must be provided to invoke a non-static method upon.");
        }

        try {
            Object returnValue = method.invoke(object, args.toArray());
            if(returnClass.isInstance(returnValue)) {
                return returnClass.cast(returnValue);
            }
            else {
                LOGGER.error("Unable to return value from method invocation; the object is not an instance of " + returnClass.getSimpleName());
                return null;
            }
        }
        catch(Exception e) {
            LogUtil.log(LOGGER, Level.ERROR, "Unable to invoke method " + method.getName(), e);
            throw e;
        }
    }

    public R invoke(Object ... args) throws Exception {
        this.args.addAll(Arrays.asList(args));
        return invoke(returnClass);
    }

}

It seams like there is a lot of work going on there for just a simple concept. I suspect you don't need all the flexibility and structure that this implementation gives you. Instead, I suggest you use a simple interfaces. Here is the Function interfaces that Guava uses:

public interface Function<F,T> {
    T apply(F input);
    boolean equals(Object object);
}

All you have to do is implement apply() and you are done. If it is a one off, then you can just create an anonymous class. If you are calling the same type of function repeatedly in a number of places or you need something slightly different, you can make a concrete class for those cases.

The good news is that Java 8 will make this much easier. The introduction of lambdas and method references makes it much easier to pass code to be called into a function. The Method Reference page has examples of lambdas and method references.


Simple string inverter program I am currently studying c programming, but I'm at a very basic level. So for practice I made a program that reads a string and inverts it. I want to know if the code is readable and how I can improve it.

/*
* Goal: Given a string, invert it, then print the result.
* Author: Lúcio Cardoso
*/

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

/*This is the size of the arrays in this program. It's better to use a
constant, because makes easier to modify the arrays' size, if needed.*/
#define NUM 200

void invert(char *s) //Fucntion responsible for inverting the string.
{
    char aux[NUM];
    int i, j, n; //These variables are all used to control the for loops.

    /*This loop will read the size of the array's string, that's why
    strlen(s) - 1 is used. We don't need to read all contents
    from the array, just the string. As we read the s[] backwards,
    its content is stored in aux[n]. In this case, from zero to the
    size of s[]'s string - 1. Minus 1 is used because we read it from ZERO,
    not i. */
    for (i = strlen(s) - 1, n = 0; n < strlen(s), i >= 0; i--, n++) {
        aux[n] = s[i];
    }

    printf("Included with the super mojo from the string inverter, ");
    printf("this is the result: ");

    //This for loop reads all the content from the aux array, and print it.
    for (j = 0; j < strlen(s); j++) {
        printf("%c", aux[j]);
    }
}

int main(void)
{
    char string[NUM];

    printf("\nWelcome to the super, duper string inverter!\n");
    printf("If you want to see some magic, enter a string:\n>");
    gets(string); //Reads the string.

    //Now, we only need to call the function with string[] as a parameter.
    invert(string);

    return 0;
}

That's a fine small piece of software for a beginner. Here is my review:

const-modifier
Since you don't modify the contents of the char *s you can safely change your function signature to void invert(const char *s).

Too many strlen() calls
You execute strlen(s) every for-loop iteration. This is quite bad for the performance (especially for large strings) since strlen loops through your whole string each call. If your string is 100 characters long (beside the NULL-character) this ends up in 100*100 = 10000 iterations.
As a quick solution you could just create a variable length and store the length of your string once. From that point on you'll compare with length instead of strlen(x) and will get the same result (since, s doesn't change while execution).

Comparison unused
n < strlen(s) remains unused in your first for-loop. I think you want to connect the two comparisons with &&:

for (i = strlen(s) - 1, n = 0; n < strlen(s) && i >= 0; i--, n++)

Why even reverse?
Since your function prints the reversed string, but doesn't return anything. You don't really need to reverse it. You could just loop through the input string starting at the end (like you did in your first loop), printing all the characters. The extra array only makes sense, if you return a pointer to it for later use.


Journal Application in D Started picking up D recently. I wrote an application to create journal entries (text files) based on user input.

//this program will write to files. It should be able to read them.
//implement a command interface?
import std.stdio;
import std.string;
import std.datetime;
import fileio = std.file;
import std.conv;

enum Commands {exit = "EXIT", create = "CREATE"};

string menuText(){
    return "\nType a command:\n\t- create\n\t- exit\n>";
}

void checkJournalEntriesFolder(){
    string path = "journalEntries/";
    if(fileio.exists(path) == 0){
        writeln("Creating a journalEntries folder for you.");
        fileio.mkdir(path);
    }
}

string getFileName(){
    return "entry_" ~ std.conv.to!string(stdTimeToUnixTime(Clock.currStdTime())) ~ ".txt";
}

int main(string[] argv)
{
    writeln("\nWelcome to myJournal.");
    checkJournalEntriesFolder();
    string mode = "COMMAND";
    char[] buf;
    Commands command;
    while (mode != "EXIT"){
        while (mode == "COMMAND"){
            string journalContent;
            write(menuText);
            stdin.readln(buf);
            switch(toUpper(buf[0..$-1])) // cut off the /n at the end of the buffer.
            {
                case command.exit:
                    {
                        mode = "EXIT";
                        writeln("Oh, I'll exit.");
                        break;
                    }
                case command.create:
                    {
                        writeln("create journal entry selected");
                        mode = "CREATE";
                        //create file
                        writeln("You are now typing in your journal.");
                        writeln("To exit this file, type #EOF");
                        writeln("----------------------------");
                        break;
                    }
                default:
                    {
                        writeln("\nunrecognized command");
                    }
            }

            while (mode == "CREATE") {
                stdin.readln(buf);
                if(buf[0..$-1] == "#EOF"){
                    writeln("----------------------------");
                    mode = "COMMAND";
                    checkJournalEntriesFolder(); // double check, in case the user deleted it.
                    string filename = "journalEntries/" ~ getFileName;
                    fileio.write(filename, journalContent[0..$-1]); //Dont include the blank line.
                    writeln("File Saved.");
                } else {
                    journalContent ~= buf;
                }
            }
        }
    }
    return 0;
}

Possible Improvements:

Maybe be able to move throughout the document once it is open (right now if you hit enter, you cannot move to the previous line).
Make command / insert mode more like vi.
Edit existing files.

I am very new to D, so I expect this code will be tore up. I've done some C++, but it has been a while.

I'm not entirely sure what you are asking for, but if this were my program I would do a few things differently.

SQLite

I would place the journal entries in a sqlite database rather than raw files and directories.

D provides an interface to sqlite. Sqlite provides great abstraction over raw files. Keeping your files consistent, and not corrupting them is difficult. sqlite already does the right thing, and more!

Command Pattern instead of switch

I would provide a nicer abstraction for commands instead of a switch statement. This Python code works as a template.

By implementing a switch statement, each new command increases the cyclomatic-complexity of the main() function. Thus your testing requirements increase exponentially. By breaking out each command to implement an ICommand interface, your testing requirement increases linearly. The visitor pattern might also provide a nice mechanism to process each command.


Deep iterations with side effects Suppose I need to process a list of list of list of many objects, where the data usually comes from third party APIs.

For the sake of an example, lets assume we have many users, and for each user we get many days of data, and for each day you get many events. We want to process all of these objects and store in our database

I thought about some differents design patterns I could use, but I'm not sure what is considered to be the best approach, if any.

These are my thoughts:

1) Nested iterations and do the side-effect (save to database) at the lowest-level loop.

for user in users:
    for day in user.days:
        for event in day.event:
            processed_object = process_event(event)
            save_to_db(processed_object)

2) Similar to one, except that each nested iteration is 'hidden' in another method.

for user in users:
    process_user(user)

def process_user(user):
   for day in user.days:
        process_day(day)

def process_day(day):
    for event in day.events:
        process_event(event)

def process_event(event):
    save_to_db(event)

3) Return everything from deep in the nest and then do the side-effects afterwards.

def process_users(users):
    processed = []
    for user in users:
        processed.append(process_user(user))
    return processed

def process_user(user):
    processed = []
    for day in user.days:
        processed.append(process_day(day))
    return processed

def process_day(day):
    processed = []
    for event in day.events:
        processed.append(process_event)
    return processed

processed_events = process_users(users)

for event in processed_events:
    save_to_db(event)

Is there a general, agreed on approach for these type of programs, where side-effects are involved?

Your third approach is the most pythonic, in the sense that you create a function for each specific purpose, avoiding repetition and giving the user more options (process the users, only the days, etc...) and the option to save or not to save the processed data to the database.

It is also the fastest. If you benchmark it, that kind of approach (separating things into functions) will almost always be the faster one with Python.

Using your first approach it took roughly 0.00027 seconds to process a list of 100 events for each day, having two days for each user and two users.

Using the second approach it takes roughly 0.00018 seconds.

Using the third approach it takes roughly 0.00012 seconds.

Here are all the approaches and their output:
http://pastebin.com/vU12HaQW

Notice your third approach yields a list of list of lists, whereas the first and the second return a list.


Automatically growing label in Python Tkinter (just for fun) #!/usr/bin/env python3

from tkinter import *

root = Tk()

class GrowLabel(Label):
    def __init__(self, master):
        Label.__init__(self, master)
        self.counter = 12
        self.config(text=str(self.counter), fg="blue", font=("verdana", self.counter, "bold"))
        self.pack()
        button = Button(self.master, text="Stop", command=self.master.destroy)
        button.pack()

    def count(self):
        self.counter += 1
        self.config(text=str(self.counter), fg="blue", font=("verdana", self.counter, "bold"))
        self.after(1000, self.count)

label = GrowLabel(root)
label.count()

root.mainloop()

Naming

Nouns are best for class names instead of verbs.
GrowLabel sounds like an action.
GrowingLabel would sound like a label that grows.

Related to this,
having a count method on a GrowingLabel object doesn't sound natural.
The fact that you implement the growing effect using a count should be an internal detail,
and not revealed to users of the class.
The important point is that the label can grow,
not how it does it.
So, renaming the method to grow() would be more natural.

Calling super-class methods

Since you are in Python 3,
it would be better to call the super constructor of GrowLabel like this:

super().__init__(master)

If that doesn't work somehow (I don't have tkinter to try),
then try this instead:

super(GrowLabel, self).__init__(master)

See more details in this related discussion.

Avoid wildcard imports

Don't use wildcard imports like this:

from tkinter import *

Quoting from PEP8:

  Wildcard imports (from  import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools.

I guess you do this because you're using many classes from this module.
A better way would be to do like this:

import tkinter as tk

And then you can prefix the classes you need with tk., like this:

root = tk.Tk()


Algorithm that checks if a tree is full and complete I am trying to write a method that will return true if a binary tree is full and complete (each node has 2 children or none and all the leaves of the tree are at the same depth).

My idea is to use recursion. I will check for any node if its left son has number of children that is equal to its right son's number of children. If so, I will return true, otherwise false.

The algorithm will look like this:

public class Utils {

    public boolean isFullCompleteTree(Tree<Integer> t) {
            TreeInfo rootInfo = isFullCompleteTree(t.getRoot());
            return rootInfo.valid;
        }
    public TreeInfo isFullCompleteTree(Node<Integer> node) {
        boolean valid = true;

        if (node == null) {
            return new TreeInfo(true, 0);
        }

        TreeInfo rightInfo = isFullCompleteTree(node.goRight());
        TreeInfo leftInfo = isFullCompleteTree(node.goLeft());

        if ((!rightInfo.valid) || (!leftInfo.valid)) {
            valid = false;
        }
        if (rightInfo.numChildern != leftInfo.numChildern) {
            valid = false;
        }
        return new TreeInfo(valid, rightInfo.numChildern + leftInfo.numChildern
                + 1);
    }
    }

    class TreeInfo {
       public boolean valid;
       public int numChildern;

       public TreeInfo(boolean valid, int numChildern) {
           this.valid = valid;
           this.numChildern = numChildern;
    }

}

I didn't include the tree implementation but it is pretty straightforward.

The idea of the algorithm is to check if in every node the number of right son's children is equal to the left son's children. If a tree is not full and complete, then in some node this rule will not apply.

Do you think that my algorithm is correct or am I missing something?

On the whole I like your implementation. It is elegant and correct. I also like your use of a TreeInfo object.

Your calculation of valid is a little unnecessarily complex - you can make it much simpler and clearer as a single statement.

You may find (and this is only my opinion) adding some comments helps you think more clearly about the code.

Once I had finished tinkering my version looks like:

public boolean isFullCompleteTree(Tree<Integer> t) {
    TreeInfo rootInfo = isFullCompleteTree(t.getRoot());
    return rootInfo.valid;
}

public TreeInfo isFullCompleteTree(Node<Integer> node) {

    if (node == null) {
        // A null node is valid with a size of 0.
        return new TreeInfo(true, 0);
    }

    // Inspect both branches.
    TreeInfo rightInfo = isFullCompleteTree(node.goRight());
    TreeInfo leftInfo = isFullCompleteTree(node.goLeft());

    // Valid when both branches are valid and have the same child count.
    boolean valid = rightInfo.valid && leftInfo.valid
            && rightInfo.numChildern == leftInfo.numChildern;
    return new TreeInfo(valid,
            // My size is the total size of my children + 1
            rightInfo.numChildern + leftInfo.numChildern + 1);
}

Notice how the valid calculation matches your spec of its left son has number of children that is equal to its right son's number of children along with the obvious both children are valid.


Find the nth prime number It's a long time since I've written a Sieve of Eratosthenes (about 3 decades, I think, in ZX Basic).  So I decided to revisit in the light of experience.

Rather than allocating a vector of unknown size to sieve values, I keep an ordered set of upcoming composite values which are updated as they are reached.

#include <map>

unsigned long find_nth_prime(unsigned long n)
{
    if (!n) return 1;           // "0th prime"
    if (!--n) return 2;         // first prime

    // The map m contains one entry for each prime.  The key is the
    // next time to update it, and the value is the amount to increment
    // by each time it is reached.
    auto m = std::map<unsigned long, unsigned long>();
    // The overflow tests below are valid only for unsigned types.
    for (auto i = 3ul;  true;  i += 2) {
        if (i < 3ul)            // overflow test
            throw std::overflow_error("prime too large");

        const auto it = m.cbegin();
        if (it == m.end() || it->first != i) {
            // it's a prime
            if (!--n)
                return i;
            if (i*i > i)        // overflow test
                m.insert({i*i, 2*i});
        } else {
            // it's composite - advance the factor
            auto next = it->first;
            do {
                next += it->second;
            } while (next > i && !m.emplace(next, it->second).second);
            m.erase(it);
        }
    }
}

And the test program (not really part of the review):

#include <iostream>
#include <string>

int main(int, char **argv)
{
    std::cout.imbue(std::locale(""));
    while (*++argv) {
        try {
            std::cout << find_nth_prime(std::stoul(*argv)) << std::endl;
        } catch (std::exception&) {
            std::cerr << "Invalid argument: " << *argv << std::endl;
        }
    }
}

Compiled with GCC:

g++ -std=c++17 -O3 -Wall -Wextra -Wpedantic

Whilst the performance is acceptable (about 1.6 seconds on my hardware to find the one-millionth prime - 15,485,863), I'm unsure whether a std::map is really the best structure for keeping track.  As usual, any other critique of the code is welcome.

Bug: Only works if longs are 64-bit

I ran your program on my 32-bit machine and got the wrong answer for the 1 millionth prime: 15498907 instead of 15485863.  The problem is here:

        if (i*i > i)        // overflow test

If longs are 32 bits, then a number such as 0x10001 will pass this overflow test (because 0x10001 * 0x10001 = 0x20001 after overflowing).  This eventually causes problems because overflowed values are inserted into the map.  Actually, even if longs are 64-bit, you will still have a problem, but it will only happen after i reaches 0x100000001.

I fixed the problem by doing this:

    unsigned long max_i = std::sqrt(~0UL);
    // ...
        if (i <= max_i)      // overflow test

Slow compared to vector based solution

I found your program to be a lot slower than a traditional vector based sieve.  Here are timings for your program vs a traditional sieve that I wrote which used a vector<bool>:

Map    based,  1 millionth prime:  3.77 seconds
Map    based, 10 millionth prime: 47.91 seconds
Vector based,  1 millionth prime:  0.04 seconds
Vector based, 10 millionth prime:  0.93 seconds

Sample vector based code

Since there was a comment asking about how the vector based code would work, here is the full program I used:

#include <iostream>
#include <string>
#include <cstdlib>
#include <cmath>
#include <cstdio>
#include <vector>

// This function uses the prime counting function approximation:
//
// n = x / ln(x)
//
// Where n is the number of primes below the number x.  Since we are trying
// to find the nth prime, we should solve for x here to determine an
// approximate value for the nth prime.  It turns out that for n >= 6, the
// value we solve for x will always be larger than the nth prime value.
//
// If the sieve size doesn't fit in an unsigned long, this function returns 0.
unsigned long findSieveSize(unsigned long n)
{
    // For small n, the formula returns a value too low, so we can just
    // hardcode the sieve size to 5 (5th prime is 11).
    if (n < 6)
        return 13;

    // We can't find a prime that will exceed ~0UL.
    if (n >= (~0UL / std::log(~0UL)))
        return 0;

    // Binary search for the right value.
    unsigned long low  = n;
    unsigned long high = ~0UL - 1;
    do {
        unsigned long mid   = low + (high - low) / 2;
        double        guess = mid / std::log(mid);

        if (guess > n)
            high = (unsigned long) mid - 1;
        else
            low = (unsigned long) mid + 1;
    } while (low < high);
    return high + 1;
}

unsigned long find_nth_prime(unsigned long n)
{
    if (!n) return 1;           // "0th prime"
    if (!--n) return 2;         // first prime

    unsigned long sieveSize = findSieveSize(n);
    unsigned long count     = 0;
    unsigned long max_i     = std::sqrt(sieveSize-1)+1;

    if (sieveSize == 0)
        return 0;

    std::vector<bool> sieve(sieveSize);
    for (unsigned long i = 3;  true;  i += 2) {
        if (!sieve[i]) {
            if (++count == n)
                return i;
            if (i >= max_i)
                continue;
            unsigned long j    = i*i;
            unsigned long inc  = i+i;
            unsigned long maxj = sieveSize - inc;
            // This loop checks j before adding inc so that we can stop
            // before j overflows.
            do {
                sieve[j] = true;
                if (j >= maxj)
                    break;
                j += inc;
            } while (1);
        }
    }
    return 0;
}

int main(int, char **argv)
{
    std::cout.imbue(std::locale(""));
    while (*++argv) {
        try {
            std::cout << find_nth_prime(atoi(*argv)) << std::endl;
        } catch (std::exception&) {
            std::cerr << "Invalid argument: " << *argv << std::endl;
        }
    }
}


Is this a better way to validate a text box for mobile number only? Is this a better way to validate a text box for mobile number only?

Validation:

allow only numbers
will not allow to paste string including characters
allow copy from the box
length is set to 10 without country code
allow back space

Suggest some methods to help me improve my function.

Public Sub mob_validation(ByVal e As System.Windows.Forms.KeyPressEventArgs, ByVal mob As TextBox)
        If e.KeyChar = Chr(22) Then
            Dim output As String = New String((From c As Char In Clipboard.GetText.ToString Select c Where Char.IsLetter(c)).ToArray())
            If output = "" And Len(mob.Text) = 10 Then
                mob.ForeColor = Color.Green
            Else
                mob.ForeColor = Color.Red
                Clipboard.Clear()
            End If
        Else
            If Asc(e.KeyChar) < 48 Or Asc(e.KeyChar) > 57 Then
                If Asc(e.KeyChar) <> 8 Then
                    If Asc(e.KeyChar) <> 3 Then
                        e.Handled = True
                        mob.ForeColor = Color.Red
                    End If
                End If
            Else
                mob.ForeColor = Color.Green
            End If
        End If
    End Sub

Again, properly indent the code. The code was unreadable to the point that we broke our own rules and edited the code in question just to be able to review it.

Other than that, the biggest issue with this is all of the magic numbers. They mean absolutely nothing to Mr. Maintainer. Normally at this point, I tell people to create an Enum, but in this case, there's one built in. Use the Keys Enumeration to replace all of those hardcoded numbers.


Scala Codility GenomicRangeQuery performance I solved Codility's GenomicRangeQuery challenge in Scala. We are given a long DNA sequence (up to 100000 bases), where A, C, G, T are assigned values 1, 2, 3, 4, respectively. There are up to 50000 queries of the form (pi, qi), for each of which we should return the smallest value in the DNA string between positions pi and qi (inclusive, and zero-based).

I came up with the following solutions, both of which got 100% correctness but 0% performance. I'd like to know what can I do to improve the performance of these functions.

Answer #1

def solution(s: String, p: Array[Int], q: Array[Int]): Array[Int] = {
  for (i <- p.indices.toArray) yield {
    val gen = s.substring(p(i), q(i) + 1)
    gen match {
      case _ if gen.contains('A') => 1
      case _ if gen.contains('C') => 2
      case _ if gen.contains('G') => 3
      case _ if gen.contains('T') => 4
    }
  }
}

Answer #2

def solution(s: String, p: Array[Int], q: Array[Int]): Array[Int] = {
  val weights =
    for (c <- s.toCharArray) yield c match {
      case 'A' => 1
      case 'C' => 2
      case 'G' => 3
      case 'T' => 4
    }

  for (i <- p.indices.toArray) yield weights.slilce(p(i), q(i) + 1).min
}

The basic task here is to search for the minimum value in the current subset, but there is a known floor value so it would be nice if we could terminate the search early if the floor value is encountered. Unfortunately the Standard Library doesn't have such an animal so we'll have to roll our own.

def solution(s: String, p: Array[Int], q: Array[Int]): Array[Int] = {

  val arr = s.toArray.map {   // String to Array of weighted values
    case 'A' => 1
    case 'C' => 2
    case 'G' => 3
    case 'T' => 4
  }

  def subMin(start: Int, end: Int, min: Int = 4): Int =
    if (start > end) min                                          // search done
    else if (arr(start) == 1 || arr(end) == 1) 1                  // early end
    else subMin(start+1, end-1, arr(start) min arr(end) min min)  // tail recurse

  p.indices.map(x => subMin(p(x), q(x))).toArray
}

With this I got 100% correctness and 66% performance.

So a different, and perhaps larger, performance issue is that once a subset has been evaluated it would be really nice if we never have to revisit it again, just recall the previous evaluation when needed.

That's the basic idea behind a segment tree. Essentially, your data becomes the leaves of a binary tree and each node contains the result of the associative operation on all the leaf values underneath it.

I won't paste the 40 lines of segment tree code here (worthy of independent study), but with that in place...

def solution(s: String, p: Array[Int], q: Array[Int]): Array[Int] = {

  val arr = s.toArray.map {   // String to Array of weighted values
    case 'A' => 1
    case 'C' => 2
    case 'G' => 3
    case 'T' => 4
  }

  val st = SegmentTree(arr, Math.min)  // build the segment tree

  p.indices.map(x => st(p(x), q(x))).toArray
}

... this got 100% correctness and 100% performance.


MongoDB Filter between two dates Simple code for getting documents that have a creationDate between two values in mongodb.
If the user provides only one of the values the code should still work and get the documents that have either a creationDate less than or bigger than the given value.
I'm mainly looking for more readability and simplicity.
interface mongoDateFilter {
  $gte?: Date;
  $lte?: Date;
}

export const getReportsForContent = async (
  contentId: ObjectId,
  beginDate: Date | undefined,
  endDate: Date | undefined,
): Promise<Report[]> => {
  const reportsCollection = await getCollection('reports');

  const creationDateMongoFilter: mongoDateFilter = {};

  if (beginDate) {
    creationDateMongoFilter['$gte'] = beginDate;
  }

  if (endDate) {
    creationDateMongoFilter['$lte'] = endDate;
  }

  let reportsForContent: Report[] = [];

  if (beginDate || endDate) {
    reportsForContent = await reportsCollection.find({ contentId, creationDate: creationDateMongoFilter }).toArray();
  } else {
    reportsForContent = await reportsCollection.find({ contentId }).toArray();
  }

  return reportsForContent;
};
```

Prefer dot notation over bracket notation when syntax permits it - it's a bit easier to read and write. ESLint rule: dot-notation.
Construct objects all in one go rather than mutating them afterwards, if you can - it's easier to write (especially in TypeScript, since you don't have to denote the type ahead of time) and can be easier to understand at a glance when unnecessary mutation is avoided.
Don't assign expressions that won't be used - with
let reportsForContent: Report[] = [];

regardless of the situation, reportsForContent will be reassigned to something else afterwards, so you can leave off the = [] part.
Or, even better:
Return the value retrieved instead of reassigning a variable and then returning the variable. This:
  if (beginDate || endDate) {
    reportsForContent = await reportsCollection.find({ contentId, creationDate: creationDateMongoFilter }).toArray();
  } else {
    reportsForContent = await reportsCollection.find({ contentId }).toArray();
  }

can be
  if (beginDate || endDate) {
    return reportsCollection.find({ contentId, creationDate: creationDateMongoFilter }).toArray();
  } else {
    return reportsCollection.find({ contentId }).toArray();
  }

Or, even better, handle the case where no date is set at the very beginning, and only construct the creationDateMongoFilter later, if it's needed.
In all:
export const getReportsForContent = async (
    contentId: ObjectId,
    beginDate: Date | undefined,
    endDate: Date | undefined,
): Promise<Report[]> => {
    const reportsCollection = await getCollection('reports');
    if (!beginDate && !endDate) {
        return reportsCollection.find({ contentId }).toArray();
    }
    const creationDateMongoFilter = {
        (...beginDate && { $gte: beginDate }),
        (...endDate && { $lte: endDate }),
    };
    return reportsCollection.find({ contentId, creationDate: creationDateMongoFilter }).toArray();
};

No need for the mongoDateFilter interface anymore.