Beautifulsoup scraper for sport events I've written a simple scraper that parses HTML using BeautifulSoup and collects the data (schedule of sports events), then clubs them together in a list of dicts.

The code works just fine, but the way I process the data is pretty horrible IMO. I use an if...else to parse the data selectively, because the output of the dicts are alternative, that is {venue, result (if available)}, and time of match, and team information.

So basically, is there a better, more reliable way to parse the data?

import requests
import re
from BeautifulSoup import BeautifulSoup
from datetime import datetime
class cricket(object):

    def getMatches(self, url):
        """ Scrape the given url for match schedule """

        headers = {'Accept':'text/css,*/*;q=0.1',
        'Accept-Charset':'ISO-8859-1,utf-8;q=0.7,*;q=0.3',
        'Accept-Encoding':'gzip,deflate,sdch',
        'Accept-Language':'en-US,en;q=0.8',
        'User-Agent':'Mozilla/5 (Solaris 10) Gecko'}

        page = requests.get(url, headers = headers)
        page_content = page.content
        soup = BeautifulSoup(page_content)

        result = soup.find('div', attrs={'class':'bElementBox'})
        tags = result.findChildren('tr')

        match_type_list = ['TEST', 'ODI', 'T20']
        match_info = []

        for elem in range(1,len(tags)):
            dict_ = {}
            x = tags[elem].getText()
            x = x.replace(r'&nbsp;', '')

            if 'Venue' in x:
                for a in match_type_list:
                    if a in x:
                        match_type = a

                x = x.replace('Venue', '')
                if 'Result' in x:
                    x = x.replace('Result', '')
                    x = x.split(': ')
                    # print x
                    venue = x[1]
                    result = x[2]
                    dict_.update({'venue':venue,'result':result,
                        'match_type':match_type})
                else:
                    x = x.split(': ')
                    venue = x[1]
                    dict_.update({'venue':venue})

            else:
                match = re.search(r'\b[AP]M', x)
                date_time = x[0:match.end()]
                date_time = date_time.replace(',','')[4:]
                teams = x[match.end():].split('vs')
                home_team = teams[0].strip()
                away_team = teams[1].strip()
                # print date_time, home_team, away_team

                time_obj = datetime.strptime(date_time, '%b %d %Y %I:%M %p')
                timings = time_obj.strftime('%Y-%m-%dT%H:%MZ')
                dict_.update({'home_team':home_team,
                    'away_team':away_team,
                    'timings':timings
                })

            match_info.append(dict_)

        # print match_info

        final_list = []     # final list of dicts that we need

        for i in range(0, len(match_info), 2):
            final_list.append(dict(match_info[i].items() +
                match_info[i+1].items()))

        # for i in final_list:
        #     print i
        # print final_list

if __name__ == '__main__':
    url = 'http://icc-cricket.yahoo.net/match_zone/series/fixtures.php?seriesCode=ENG_WI_2012' # change seriesCode in URL for different series.
    #url = 'http://localhost:6543/lhost/static/icc_cricket.html'
    c = cricket()
    c.getMatches(url)

import requests
import re
from BeautifulSoup import BeautifulSoup
from datetime import datetime
class cricket(object):

Python conventions state that classes should CamelCase.

    def getMatches(self, url):

Python convention states that methods should be lowercase_with_underscores. Also, there isn't really a reason to have this method in a class anyway. Seems to me that it should be a function.

        """ Scrape the given url for match schedule """

        headers = {'Accept':'text/css,*/*;q=0.1',
        'Accept-Charset':'ISO-8859-1,utf-8;q=0.7,*;q=0.3',
        'Accept-Encoding':'gzip,deflate,sdch',
        'Accept-Language':'en-US,en;q=0.8',
        'User-Agent':'Mozilla/5 (Solaris 10) Gecko'}

I'd move this out of the function into a global constant.

        page = requests.get(url, headers = headers)
        page_content = page.content
        soup = BeautifulSoup(page_content)

I'd combine these three lines

soup = BeatifulSoup(request.get(url, headers = headers).content)

        result = soup.find('div', attrs={'class':'bElementBox'})
        tags = result.findChildren('tr')

I'd avoid non-descriptive names like result. Tags is a bit better, but not by a whole lot. I'd also combine these two lines

tags = soup.find('div', attr={'class':'bElementBox'}).findChildren('tr')

        match_type_list = ['TEST', 'ODI', 'T20']
        match_info = []

        for elem in range(1,len(tags)):

It'd make more sense to process things two rows at a time.

            dict_ = {}

Useless name alert.

            x = tags[elem].getText()
            x = x.replace(r'&nbsp;', '')

At this point, you extract the text and throw out the html. But the data is divided in table cells, so why in the world wouldn't you want to take advantage of that? At this point you drop down to trying to extract data straight from the text which much harder then if you can use the tags as hints.

            if 'Venue' in x:
                for a in match_type_list:
                    if a in x:
                        match_type = a

                x = x.replace('Venue', '')
                if 'Result' in x:
                    x = x.replace('Result', '')
                    x = x.split(': ')
                    # print x
                    venue = x[1]
                    result = x[2]

All this work to extract data from the string is something you really should use a regular expression for. This is exactly the kind of situation it excells at.

                    dict_.update({'venue':venue,'result':result,
                        'match_type':match_type})

Its not clear why you would choose to update rather then simply assign. There is no way that any other line of code in this loop can assign to it.

                else:
                    x = x.split(': ')
                    venue = x[1]
                    dict_.update({'venue':venue})

            else:
                match = re.search(r'\b[AP]M', x)
                date_time = x[0:match.end()]
                date_time = date_time.replace(',','')[4:]
                teams = x[match.end():].split('vs')
                home_team = teams[0].strip()
                away_team = teams[1].strip()

                # print date_time, home_team, away_team

                time_obj = datetime.strptime(date_time, '%b %d %Y %I:%M %p')

Organization seems a little suspect. You jump from working on the date, over to the teams, and then back to the date. I'd stick with date until it was finished. You also spend a bunch of lines massaging the date. However, strptime lets you specify any format you want, so you should just be able to have it parse the data

                timings = time_obj.strftime('%Y-%m-%dT%H:%MZ')

If I'm parsing, I wouldn't convert the time back into a date in another object. I'd keep it as a date object.

                dict_.update({'home_team':home_team,
                    'away_team':away_team,
                    'timings':timings
                })

            match_info.append(dict_)

        # print match_info

        final_list = []     # final list of dicts that we need

        for i in range(0, len(match_info), 2):
            final_list.append(dict(match_info[i].items() +
                match_info[i+1].items()))

I'd do this:

for i in range(0, len(match_info), 2):
    left, right = match_info[i:i+2]
    final_list.append( dict(left.items() + right.items() )

I think it makes things a bit clearer.

        # for i in final_list:
        #     print i
        # print final_list

this function probably needs to return that list or something.

if __name__ == '__main__':
    url = 'http://icc-cricket.yahoo.net/match_zone/series/fixtures.php?seriesCode=ENG_WI_2012' # change seriesCode in URL for different series.
    #url = 'http://localhost:6543/lhost/static/icc_cricket.html'
    c = cricket()
    c.getMatches(url)

Good

Here is my reworking of your code:

def get_cricket_matches(content):
    """ Scrape the given content for match schedule """

    soup = BeautifulSoup(content)

    all_rows = soup.find('div', attrs={'class':'bElementBox'}).findChildren('tr')

    match_info = []

    for index in range(1, len(all_rows), 2):
        rows = [row.findChildren('td') for row in all_rows[index:index+2]]

        data = {
            'match_type' : rows[1][0].findChildren('b')[1].getText(),
            'home_team': rows[0][2].getText(),
            'away_team' : rows[0][5].getText(),
            'match_time' : datetime.strptime(rows[0][0].getText(), '%a, %b %d, %Y %I:%M %p'),
        }
        for line in rows[1][1].findAll('b'):
            content = str(line.nextSibling)[1:]
            if line.getText() == 'Venue':
                data['venue'] = content
            else:
                data['result'] = content

        match_info.append(data)

    return match_info

  Personally, I don't like dependencies (in this case requests and
  BeautifulSoup). Why just not to use the standard modules?

Here I have to strongly disagree with @cat_baxter. Dependencies are the best part of python. That is, the availability and ease of installing all these different libraries to make it easier to write code is a great asset of Python. You should never be worried about taking on dependencies that are easy to install if it helps.

Having said that, you don't get a whole lot out of the requests module. So an argument can be made that this code would be better off using urllib.

But then the question is whether its a good idea to parse HTML with regex. With scraping you can get away with using a regex because regardless of the technique you use, changes to the page's structure will probably break it. The question at that point is which version has simpler and easier to write code.

Compare:

re.match('
.+
(.+).+
.+
', page).group(1)

and

soup.find('div', attrs={'class':'bElementBox'})

The HTML parser method takes advantage of knowing the structure of html to make it fairly easy to find the element. Is the regex entirely correctly? (Technically no, because simply regex can't parse HTML correctly, but is it good enough for job? I assume so, but I have more confidence in the HTML parser.)

columns = re.compile('(.*?)	', re.DOTALL | re.M).findall(table)

An equivalent would be

columns = table.findall('td')

Again, HTML parser wins hands down.

        'home_team' : columns[index + 2],

VS:

        'home_team': rows[0][2].getText(),

The difference between rows[0][2] and columns[index + 2] is the fact that I choose to structure the parser after the rows, whereas cat_baxter decided to throw away the rows and look just at the columns. Either approach can be done either way, so that's incidental to this question. But here the HTML parser method is slightly more complicated because it has to use .getText()

        'venue'     : re.match("Venue: ([\s,\w\']+)", columns[index + 7]).group(1),

m = re.match('.+Result:([\w, ]+)', columns[index + 7])
if m:
   row['result'] = m.group(1)

Vs

    for line in rows[1][1].findAll('b'):
        content = str(line.nextSibling)[1:]
        if line.getText() == 'Venue':
            data['venue'] = content
        else:
            data['result'] = content

I think this one is hard to call. I think my lines of code have less going on in them, but I think it might be somewhat more obvious what is happening in the regular expression.

My ending conclusion is that HTML parsers for scraping is often much nicer then using a regex, and sometimes a little worse. On the balance, I think its a way better idea.


Android LiveData Shared between activities by Application Session I'm developing an app that use socket to receive messages from a chat.
I'm using an MVP approach and I store the data in a Singleton class named DataBridge like this:

class myApplication: Application() {

    val dataBridge: DataBridge = DataBridge()
    val deviceManager: DeviceManager = DeviceManager(this)

    override fun onCreate() {
        super.onCreate()
        instance = this

        //init Logger
        TimberImplementation.init()

        //init Maps
        Mapbox.getInstance(this, MAP_ACCESS_TOKEN)
    }

    companion object {
        lateinit var instance: InfiniteApplication
            private set
    }
}

I have the following scenario:

Main Activity: create socket connection and update data in DataBridge class. It has a ViewPagerAdapter with 4 fragments. Each Fragment has it's own Presenter.
Chat Activity: listen changes from DataBridge like onNewMessage (fired by HomeActivity) and display the chat

The question is: is a good practice to share Live Model by Application and make it accessible from presenter of different activities?

Is a good practice to share LiveData?
Yeah

Here as you are using the data in databridge as a single source of truth and it is used hold data which is needed to be shared across multiple screen with live updates, Livedata is a really good option as it will also handle the lifecycle for you.

Android docs also has sample on Extending LiveData to create your own singleton data source which you may find helpful.

Also you can leverage the feature of Object declarations in kotlin to lazy initialize your DataBridge rather than in Application class.


Read from a file and pick a random line I've been learning Java lately and had some great feedback from the previous threads (hangman and guess a number games).
I was challenged by a user to try to get this code done, so I'm posting it here for further review.
Basically my weakeast (among many) points are exception handling and proper documentation.
What do you think about it? How would you improve it?

Code can be found also in GitHub

package app;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.List;

public class RandomWord {
private String fileName = "src/app/wordlist.txt";
List<String> lines = null;

public static void main(String[] args) {
    RandomWord randomWord = new RandomWord();
    randomWord.init();
}

private void init() {
    try {
        lines = Files.readAllLines(Paths.get(fileName),
                StandardCharsets.UTF_8);
    } catch (IOException e) {
        System.out.println("File can't be opened.");
    }

    int randomWordIndex = getRandomNumber(1, lines.size());
    System.out.println(lines.get(randomWordIndex));
}

/**
 * Returns a random number in the range of the specified min and max
 * parameters.
 *
 * @param min
 *            the minimum value to return
 * @param max
 *            the maximum value to return
 * @return a random number between the specified range
 */
private int getRandomNumber(int min, int max) {
    return min + (int) (Math.random() * ((max - min) + 1));
}
}

Hardcoding

The path to the file with words is hardcoded. That's not so good. It would be better to make it a command line argument.

If you must hardcode something, make it a constant, for example:

private static final String PATH = "src/app/wordlist.txt";

fileName was not a good name for a path anyway. A "file name" usually refers to the last path segment without parent directories.

Single responsibility principle

The class violates the single responsibility principle: it picks a random word, and it reads a file. I would do something like this instead:

class RandomStringPicker {
    private final List<String> list;

    public RandomStringPicker(List<String> list) {
        this.list = list;
    }

    public String pick() {
        return list.get(pickRandomNumber(list.size()));
    }

    private int pickRandomNumber(int max) {
        return (int) (Math.random() * max);
    }
}

This class only does one thing. You could read the file somewhere else and create a RandomStringPicker class from the lines you read. But now you can reuse this class for other purposes too, for example in unit tests.

This implementation also improves the weakness of your init method. If you forget to call init, your program will compile, but not work. In the example above there is no such uncertainty: if the class is not constructed with a list, it will not compile.

Or you could even go one step further and make the class work with anything, not only strings:

class RandomPicker<T> {
    private final List<T> list;

    public RandomPicker(List<T> list) {
        this.list = list;
    }

    public T pick() {
        return list.get(pickRandomNumber(list.size()));
    }

    private int pickRandomNumber(int max) {
        return (int) (Math.random() * max);
    }
}

Bugs

If you have 10 lines, the call getRandomNumber(1, lines.size()) will return a random number between 1 and 10, inclusive, which has two problems:

The first line (index = 0) will never be picked
If 10 is picked, that will be beyond the end of your list and throw an exception

Related to this issue is that you don't really need a minimum parameter for your random number picker. So that's an unnecessary detail in your implementation, you could use something more simple like the example above.

Printing, validating and logging

It's not good practice to print things on the console. If you want to verify your code works, write unit tests. If you want to log messages during runtime, use a logger.

Nitpicks

The code is not well indented
Unnecessary brackets in (max - min) + 1. This is the same: max - min + 1


CSV email script efficiency I'm tasked with getting emails from a .csv file and using them to submit a form.  I am using the csv and mechanize Python libraries to achieve this.

import re
import mechanize
import csv

def auto_email(email):
    br = mechanize.Browser()
    br.open("URL")
    br.select_form(name="vote_session")
    br['email_add'] = '%s' % (email) #email address
    br.submit()

def csv_emails():
    ifile  = open('emails.csv', "rb")
    reader = csv.reader(ifile)
    rownum = 1
    for row in reader:
        auto_email(row[0])
        print "%d - %s processed" %(rownum, row[0])
        rownum += 1
    print 'List processed.  You are done.'
    ifile.close()

print csv_emails()

The code works, but I am very much a beginner in Python.

I was wondering whether I have any inefficiencies that you can help me get rid of and optimize the script?

I would suggest to use

with open('emails.csv', "rb") as ifile

see here for details: http://www.python.org/dev/peps/pep-0343/
in this case you don't need to do ifile.close at the end

instead of incrementing rownum by yourself, you can use

for row, rownum in enumerate(reader):

I don't see the reason to do this

br['email_add'] = '%s' % (email) #email address

instead of

br['email_add'] = email


Three biographical profiles with photos I'm an iPhone Developer mainly, and I'm very new to web development - especially jQuery.

My task: I have three divs, which each contain a short biography about a certain person. (I have three people: liz, chris and michael). Those divs are hidden at the start of my code, because they all belong to the class profile.

I have three images which are displayed throughout, and have the IDs: (lizIMG, chrisIMG, michaelIMG). When an image is clicked, any other biographies' divs should be hidden, and the selected biography's div should be shown.

Also, I have a tooltip above each image, if a div is hidden, it will say:

  (Person Name) - Click to learn more

If a div is displayed, it will say:

  (Person Name) - Click to hide

I have fully working code, but the code is far too long for the desired task. It's quite annoying because I know exactly how I would do this in Objective-C, but not in jQuery. I'm guessing re-factoring it will involve functions, and possibly arrays?

I would like to make this shorter.

$(document).ready(function() {
  $('div.profile').hide(); // Hide all the profiles
  $('#lizIMG').click(function() {
    if ($("#lizDiv").is(":visible")) {
        $('#lizTip').text('Click to learn more');
    }
    else {
        $('#lizTip').text('Click to hide');
    }
    $('#lizDiv').slideToggle('slow');
    $('#chrisDiv').hide('slow');
    $('#chrisTip').text('Click to learn more');
    $('#michaelDiv').hide('slow');
    $('#michaelTip').text('Click to learn more');
});
$('#chrisIMG').click(function() {
    if ($("#chrisDiv").is(":visible")) {
        $('#chrisTip').text('Click to learn more');
    }
    else {
        $('#chrisTip').text('Click to hide');
    }
    $('#chrisDiv').slideToggle('slow');
    $('#lizDiv').hide('slow');
    $('#lizTip').text('Click to learn more');
    $('#michaelDiv').hide('slow');
    $('#michaelTip').text('Click to learn more');
});
$('#michaelIMG').click(function() {
    $('#lizDiv').hide('slow');
    $('#lizTip').text('Click to learn more');
    $('#chrisDiv').hide('slow');
    $('#chrisTip').text('Click to learn more');
    if ($("#michaelDiv").is(":visible")) {
        $('#michaelTip').text('Click to learn more');
    }
    else {
        $('#michaelTip').text('Click to hide');
    }
    $('#michaelDiv').slideToggle('slow');
});
$("#profiles img[title]").tooltip(); // Used to set text for the tooltips
});

Look at JQuery Accordion

Or if you don't want to use the Accordion then here is a variant:

function imgClickHandler(divBiographyId, divTooltipId){
  var divBiographySelector = "#" + divBiographyId;
  var divTooltipSelector = "#" + divTooltipId;
  if ($(divBiographySelector).is(":visible")) {
    $(divTooltipSelector).text('Click to learn more');
  }
  else {
    $(divTooltipSelector).text('Click to hide');
  }
  $(divBiographySelector).slideToggle('slow');

  //.profileBiography - this css class you should add to lizDiv, chrisDiv, michaelDiv
  //.profileTip - this css class you should add to lizTip, chrisTip, michaelTip
  $(".profileBiography").not(divBiographySelector).hide('slow');
  $(".profileTip").not(divTooltipSelector).text('Click to learn more');
 }

$(document).ready(function() {
$('div.profile').hide(); // Hide all the profiles
var profiles = [{img: "lizIMG", biography: "lizDiv", tooltip: "lizTip"},
                {img: "chrisIMG", biography: "chrisDiv", tooltip: "chrisTip"},
                {img: "michaelIMG", biography: "michaelDiv", tooltip: "michaelTip"}];

$.each(profiles, function(){
  var biographyId = this.biography;
  var tooltipId = this.tooltip;
  $("#" + this.img).click(function(){
    imgClickHandler(biographyId, tooltipId);
  });
 });
}


4 solutions to leetcode 202. Happy Number have almost same performance I am working on an easy math question Happy number Happy Number - LeetCode

  Happy Number

  Write an algorithm to determine if a number is "happy".

  A happy number is a number defined by the following process: Starting with any positive integer, replace the number by the sum of the squares of its digits, and repeat the process until the number equals 1 (where it will stay), or it loops endlessly in a cycle which does not include 1. Those numbers for which this process ends in 1 are happy numbers.

  Example:

Input: 19
Output: true
Explanation:
1² + 9² = 82
8² + 2² = 68
6² + 8² = 100
1² + 0² + 0² = 1

My solutions

Solution 1, 28ms 12.1mb

string operations

class Solution1:
    def isHappy(self, n):
        s = set()
        while n != 1:
            if n in s: return False
            s.add(n)
            n = sum([int(i) ** 2 for i in str(n)])
        else:
            return True

Solution 2,  24ms, 12.3mb

class Solution2:
    def isHappy(self, n):
        """
        :type n: int
        :rtype: bool
        """
        s = set()
        while n != 1:
            if n in s: return False
            s.add(n)

            _sum = 0
            while n:
                _sum += (n % 10) ** 2
                n //= 10
            n = _sum

        return n == 1

Solution 3  the save as solution 2 minor changes  (24ms, 12.3mb)

class Solution3:
    def isHappy(self, n):
        """
        :type n: int
        :rtype: bool
        """
        s = set()
        while n:
            if 1 in s:
                return True
            if n in s:
                return False
            s.add(n)
            _sum = 0
            while n:
                _sum += (n%10)**2 #leave unit digit
                n //= 10 #remvoe unit digit
            n = _sum

Solution 4 without extra space(24ms, 12.3mb)

class Solution4:
    def isHappy(self, n):
        """
        :type n: int
        :rtype: bool
        """
        while n != 1 and n != 4:
            _sum = 0
            while n :
                _sum += (n % 10) * (n % 10)
                n //= 10
            n = _sum

        return n == 1

TestCase

class MyCase(unittest.TestCase):
    def setUp(self):
        self.solution = Solution3()

    def test_1(self):
        n = 19
        check = self.solution.isHappy(n)
        self.assertTrue(check)

It's interesting that the last 3 solutions shared the same performance, though try best possibility to improve it.

Solution #2:

class Solution2:
    def isHappy(self, n):
        # ...
        while n != 1:
            if n in s: return False
            # ...

    return n == 1

You are looping while n != 1, without any break statements. There is no need to test n == 1 at the return statement at the end.  Just return True.

Solution #3 returns None if 0 is given as input, instead of returning True or False.

Solution #4 becomes an endless loop if 0 is given as input.

Are there any other stopping conditions other that n == 0, n == 1 or n == 4?  It isn't clear that all unhappy numbers result in a loop containing the value 4, so the validity of this approach is in question.

Update: Actually Wikipedia provides a clear argument that unhappy numbers will arrive in a loop containing the value 4, so this approach is valid, but should included a comment with a link to that proof.

In all your solutions, your loop is testing at least two conditions, such as both n != 1 and n is s.  Why not initialize s to contain a 1 (or even just leave it as an empty set), and then only test n in s.  No special cases.

def is_happy(n):
    s = { 1 }

    while n not in s:
        s.add(n)
        n = sum(i * i for i in map(int, str(n)))

    return n == 1

Update:

Since Wikipedea has proof that all positive unhappy numbers end in the sequence 4 → 16 → 37 → 58 → 89 → 145 → 42 → 20 → 4 → ..., and happy numbers end in the sequence 1 → 1 → ..., you can create a set of these termination values (including 0 → 0 → ...), and no longer needed to maintain the set of "seen" values.  By using all numbers in the unhappy loop, we can terminate the search up to 8 iterations earlier over just checking for n == 1 and n == 4.

def is_happy(num):
    # See https://en.wikipedia.org/wiki/Happy_number#Sequence_behavior
    terminal = { 0, 1, 4, 16, 20, 37, 42, 58, 89, 145 }

    while num not in terminal:
        num = sum(i * i for i in map(int, str(num)))

    return n == 1

Finally:

follow the PEP-8 standards (avoid mixedCase function/method names),
Stop writing classes!


AVL tree implementation in Python Is my implementation of AVL tree correct? It is my second Python program and I am not sure if do code properly.

'''
Created on 30 sty 2016

AVL tree - my second python program :)

@author: stanek
'''

class Node:

    def __init__(self, data, left = None, right = None, height = 0):
        self.data = data
        self.left = left
        self.right = right
        self.height = height

    def add_node(self, data):
        if self.data > data:
            '''Adding to left subtree'''
            if self.left is None:
                self.left = Node(data)
            else:
                self.left.add_node(data)
                if ( self.get_left_height() - self.get_right_height() == 2):
                    ''' Then we need to balance a subtree'''
                    print("Rebalancing after inserting", data)
                    if (data < self.left.data):
                        self.rotate_left()
                    else:
                        self.double_rotate_left()

        else:
            '''Adding to right subtree'''
            if self.right is None:
                self.right = Node(data)
            else:
                self.right.add_node(data)
                if ( self.get_right_height() - self.get_left_height() == 2):
                    ''' Then we need to balance a subtree'''
                    print("Rebalancing after inserting", data)
                    if (data < self.right.data):
                        self.rotate_right()
                    else:
                        self.double_rotate_right()

    def print_nodes(self):
        if self.left is not None:
            self.left.print_nodes()
        print(self);
        if self.right is not None:
            self.right.print_nodes()

    '''
    AVL methods
    '''
    def get_right_height(self):
        if self.right is None:
            return -1
        else:
            return self.right.height

    def get_left_height(self):
        if self.left is None:
            return -1
        else:
            return self.left.height

    def set_height(self):
        self.get_height = max( self.left.get_height, self.right.get_height ) + 1

    def rotate_right(self):
        print("rotating right",self.data);
        temp = self
        self = self.right
        self.right = temp
        self.right.set_height
        self.set_height

    def rotate_left(self):
        print("rotating left",self.data);
        temp = self
        self = self.left
        self.left = temp
        self.left.set_height()
        self.set_height()

    def double_rotate_right(self):
        print("double rotating right",self.data);
        temp = self.left
        self.left = temp.right
        temp.right = self
        self = temp
        self.right.set_height
        self.set_height

    def double_rotate_left(self):
        print("double rotating left",self.data);
        temp = self.right
        self.right = temp.left
        temp.left = self
        self = temp
        self.left.set_height
        self.set_height

    def __str__(self):
        return str(self.data)

class binary_tree:

    def __init__(self):
        self.root = None

    def getRoot(self):
        return self.root

    def add(self, data):
        if self.root is None:
            self.root = Node(data)
        else:
            self.root.add_node(data)

    def print_all(self):
        if self.root is None:
            print("Empty tree")
        else:
            self.root.print_nodes();

if __name__ == '__main__':
    b = binary_tree();
    b.add(1)
    b.add(2)
    b.add(4)
    b.add(6)
    b.add(3)
    b.add(5)
    b.print_all()

Bug

No, it is not correct:

self.right.set_height

set_height is not run due to the lack of parenthesis, hence the height is not correctly set.

Swapping

In many places such as for example:

    temp = self
    self = self.right
    self.right = temp
    self.right.set_height
    self.set_height

You are swapping values in an obfuscated manner, just use the built-in a, b = b, a syntax:

>>> a, b = 1, 2
>>> a, b = b, a
>>> a, b
(2, 1)

Naming

Universally, names of classes are written in PascalCase, and you should really stick to it, even if the compiler does not enforce it:

binary_tree -> BinaryTree

getters

Getters are highly frowned upon in Python. Values are generally accessed directly.

If you really do not want the user to access values directly you may make them "private" by prefixing __ and letting a @property return them.

Decide for your nil values

None is the standard nil (nothing) value in Python but in get_right_height you return -1 if the value isNone. If so just make the default -1 and remove the getter as suggested above.

Repetition Repetition

You repeat b.add 6 times, use a loop to avoid such waste.

There must be a way to avoid repetition in the below code-blocks, the problem is that you have duplicate functions for left and right, maybe a single rotate function could help you here.

        '''Adding to left subtree'''
        if self.left is None:
            self.left = Node(data)
        else:
            self.left.add_node(data)
            if ( self.get_left_height() - self.get_right_height() == 2):
                ''' Then we need to balance a subtree'''
                print("Rebalancing after inserting", data)
                if (data < self.left.data):
                    self.rotate_left()
                else:
                    self.double_rotate_left()

        '''Adding to right subtree'''
        if self.right is None:
            self.right = Node(data)
        else:
            self.right.add_node(data)
            if ( self.get_right_height() - self.get_left_height() == 2):
                ''' Then we need to balance a subtree'''
                print("Rebalancing after inserting", data)
                if (data < self.right.data):
                    self.rotate_right()
                else:
                    self.double_rotate_right()


Vue.js checkout stepper I have made this small checkout stepper with Vue (v 2.x.x):

var app = new Vue({
  el: "#cart",
  data: {
    stepCounter: 1,
    steps: [{
        step: 1,
        completed: false,
        text: "Cart"
      },
      {
        step: 2,
        completed: false,
        text: "Shipping"
      },
      {
        step: 3,
        completed: false,
        text: "Payment"
      },
      {
        step: 4,
        completed: false,
        text: "Confirmation"
      }
    ]
  },
  methods: {
    doPrev: function() {
      if (this.stepCounter > 1) {
        this.stepCounter--;
        this.doCompleted();
      }
    },
    doNext: function() {
      if (this.stepCounter <= this.steps.length) {
        this.stepCounter++;
        this.doCompleted();
      }
    },
    doCompleted: function() {
      this.steps.forEach(item => {
        item.completed = item.step < this.stepCounter;
      });
    }
  }
});
* {
  margin: 0;
  padding: 0;
  font-family: "Poppins", sans-serif;
}

.progressbar {
  display: flex;
  list-style-type: none;
  counter-reset: steps;
  padding-top: 50px;
  justify-content: space-between;
}

.progressbar li {
  font-size: 13px;
  text-align: center;
  position: relative;
  flex-grow: 1;
  flex-basis: 0;
  color: rgba(0, 0, 0, 0.5);
  font-weight: 600;
}

.progressbar li.completed {
  color: #ccc;
}

.progressbar li.active {
  color: #4caf50;
}

.progressbar li::after {
  counter-increment: steps;
  content: counter(steps, decimal);
  display: block;
  width: 30px;
  height: 30px;
  line-height: 30px;
  border: 2px solid rgba(0, 0, 0, 0.5);
  background: #fff;
  border-radius: 50%;
  position: absolute;
  left: 50%;
  margin-left: -15px;
  margin-top: -60px;
}

.progressbar li.active::after,
.progressbar li.completed::after {
  background: #4caf50;
  border-color: rgba(0, 0, 0, 0.15);
  color: #fff;
}

.progressbar li.completed::after {
  content: '\2713';
}

.progressbar li::before {
  content: "";
  position: absolute;
  top: -26px;
  left: -50%;
  width: 100%;
  height: 2px;
  background: rgba(0, 0, 0, 0.5);
  z-index: -1;
}

.progressbar li.active::before,
.progressbar li.completed::before,
.progressbar li.active+li::before {
  background: #4caf50;
}

.progressbar li:first-child::before {
  display: none;
}

<link href="https://cdnjs.cloudflare.com/ajax/libs/twitter-bootstrap/4.4.1/css/bootstrap.min.css" rel="stylesheet" />

{{step.text}}

      <button type="button" class="btn btn-sm btn-success" v-bind:class="{ disabled : stepCounter === 1}" @click="doPrev()">Prev</button>
      <button type="button" class="btn btn-sm btn-success" v-bind:class="{ disabled : stepCounter > steps.length}" @click="doNext()">Next</button>

Questions

Is there any room for "shortening" the code?
Anything inconsistent at the logic level?

Is there any room for "shortening" the code?

Shorthands
Shorthands can simplify the markup. The click handlers already use the shorthand @click instead of v-on:click. The bindings can be simplified as well. For example - instead of:

v-bind:class="..."

the v-bind can be omitted:
:class="..."

Watch out!

Yesterday there was an answer to your TODO app post which suggested using computed properties. With the current architecture using a computed property doesn't seem feasible, but a watcher method could be employed. Instead of setting up a method like doCompleted() that must be called manually, a watcher method can be used to adjust the values whenever that value changes.
watch: {
  stepCounter: function(newValue, oldValue) {
    this.steps.forEach(item => {
      item.completed = item.step < this.stepCounter;
    });
  }
},

Note that newValue could be used instead of this.stepCounter.
While it would occupy the same number of lines as the current code, it takes the burden off the methods doPrev and doNext of having to call the method doCompleted. See the demo below for an illustration of this.

const makeStep = (text, index) => {return {text, step: ++index, completed: false}};
const steps = ['Cart', 'Shipping', 'Payment', 'Confirmation'].map(makeStep);
var app = new Vue({
  el: "#cart",
  data: {
    stepCounter: 1,
    steps
  },
  watch: {
    stepCounter: function(newValue, oldValue) {
      this.steps.forEach(item => {
        item.completed = item.step <  this.stepCounter;
      });
    }
  },
  methods: {
    doPrev: function() {
      if (this.stepCounter > 1) {
        this.stepCounter--;
      }
    },
    doNext: function() {
      if (this.stepCounter <= this.steps.length) {
        this.stepCounter++;
      }
    }
  }
});
* {
  margin: 0;
  padding: 0;
  font-family: "Poppins", sans-serif;
}

.progressbar {
  display: flex;
  list-style-type: none;
  counter-reset: steps;
  padding-top: 50px;
  justify-content: space-between;
}

.progressbar li {
  font-size: 13px;
  text-align: center;
  position: relative;
  flex-grow: 1;
  flex-basis: 0;
  color: rgba(0, 0, 0, 0.5);
  font-weight: 600;
}

.progressbar li.completed {
  color: #ccc;
}

.progressbar li.active {
  color: #4caf50;
}

.progressbar li::after {
  counter-increment: steps;
  content: counter(steps, decimal);
  display: block;
  width: 30px;
  height: 30px;
  line-height: 30px;
  border: 2px solid rgba(0, 0, 0, 0.5);
  background: #fff;
  border-radius: 50%;
  position: absolute;
  left: 50%;
  margin: -60px 0 0 -15px;
}

.progressbar li.active::after,
.progressbar li.completed::after {
  background: #4caf50;
  border-color: rgba(0, 0, 0, 0.15);
  color: #fff;
}

.progressbar li.completed::after {
  content: '\2713';
}

.progressbar li::before {
  content: "";
  position: absolute;
  top: -26px;
  left: -50%;
  width: 100%;
  height: 2px;
  background: rgba(0, 0, 0, 0.5);
  z-index: -1;
}

.progressbar li.active::before,
.progressbar li.completed::before,
.progressbar li.active+li::before {
  background: #4caf50;
}

.progressbar li:first-child::before {
  display: none;
}

<link href="https://cdnjs.cloudflare.com/ajax/libs/twitter-bootstrap/4.4.1/css/bootstrap.min.css" rel="stylesheet" />

{{step.text}}

      <button type="button" class="btn btn-sm btn-success" :class="{ disabled : stepCounter === 1}" @click="doPrev()">Prev</button>
      <button type="button" class="btn btn-sm btn-success" :class="{ disabled : stepCounter > steps.length}" @click="doNext()">Next</button>

If the data structure can be modified such that the completed property isn't needed, then the markup could use the condition step.step < stepCounter to determine when the completed class is added to each list item. See the snippet below for a demonstration of this.

const makeStep = (text, index) => {return {text, step: ++index}};
const steps = ['Cart', 'Shipping', 'Payment', 'Confirmation'].map(makeStep);
const app = new Vue({
  el: "#cart",
  data: {
    stepCounter: 1,
    steps
  },
  methods: {
    doPrev: function() {
      if (this.stepCounter > 1) {
        this.stepCounter--;
      }
    },
    doNext: function() {
      if (this.stepCounter <= this.steps.length) {
        this.stepCounter++;
      }
    }
  }
});
* {
  margin: 0;
  padding: 0;
  font-family: "Poppins", sans-serif;
}

.progressbar {
  display: flex;
  list-style-type: none;
  counter-reset: steps;
  padding-top: 50px;
  justify-content: space-between;
}

.progressbar li {
  font-size: 13px;
  text-align: center;
  position: relative;
  flex-grow: 1;
  flex-basis: 0;
  color: rgba(0, 0, 0, 0.5);
  font-weight: 600;
}

.progressbar li.completed {
  color: #ccc;
}

.progressbar li.active {
  color: #4caf50;
}

.progressbar li::after {
  counter-increment: steps;
  content: counter(steps, decimal);
  display: block;
  width: 30px;
  height: 30px;
  line-height: 30px;
  border: 2px solid rgba(0, 0, 0, 0.5);
  background: #fff;
  border-radius: 50%;
  position: absolute;
  left: 50%;
  margin: -60px 0 0 -15px;
}

.progressbar li.active::after,
.progressbar li.completed::after {
  background: #4caf50;
  border-color: rgba(0, 0, 0, 0.15);
  color: #fff;
}

.progressbar li.completed::after {
  content: '\2713';
}

.progressbar li::before {
  content: "";
  position: absolute;
  top: -26px;
  left: -50%;
  width: 100%;
  height: 2px;
  background: rgba(0, 0, 0, 0.5);
  z-index: -1;
}

.progressbar li.active::before,
.progressbar li.completed::before,
.progressbar li.active+li::before {
  background: #4caf50;
}

.progressbar li:first-child::before {
  display: none;
}

<link href="https://cdnjs.cloudflare.com/ajax/libs/twitter-bootstrap/4.4.1/css/bootstrap.min.css" rel="stylesheet" />

      <li v-for="(step, index) in steps" v-bind:class="{ active: index + 1 === stepCounter, completed: step.step < stepCounter }">{{step.text}}

      <button type="button" class="btn btn-sm btn-success" v-bind:class="{ disabled : stepCounter === 1}" @click="doPrev()">Prev</button>
      <button type="button" class="btn btn-sm btn-success" v-bind:class="{ disabled : stepCounter > steps.length}" @click="doNext()">Next</button>

CSS: margin rules
The ruleset .progressbar li::after { contains these rules:

margin-left: -15px;
margin-top: -60px;

Presuming there is no margin inherited from other elements for the right and bottom, those can be combined into a single rule:
margin: -60px 0 0 -15px;


Find the next monthly expiration date I have an agreement with a customer. When the agreement ends, the customer needs to pay. The amount which the customer needs to pay increases for each month that has passed since the agreement was initiated. The maximum duration is 3 years, so I have a final expiration date. From this final expiration date, I must be able to find the periodic expiration date (the date when the payment increases). In other words, I need a function which does this:

DateTime GetNextExpirationDate(finalExpirationDate)

One of the issues which I am struggling with is that the algorithm needs to consider whether the expiration date in the current month has already passed (there will be a date each month). It also needs to consider that the day of the month for the final expiration date, might not exist for the periodic expiration dates (e.g. if the final expiration date is the 31st of something).

I have some code which is working, but it appears somewhat redundant and with many steps. I was wondering, if perhaps it could be simplified (either a little or a lot).

public DateTime GetNextExpirationDate(DateTime finalExpirationDate)
{
    int expirationDay = finalExpirationDate.Day;
    int currentMonth = DateTime.Now.Month;
    int currentYear = DateTime.Now.Year;
    int daysInCurrentMonth = DateTime.DaysInMonth(currentYear, currentMonth);

    // If the month has less days that the day of the final expiration date,
    // then we need to use the maximum date of the month.
    int expirationDayThisMonth = Math.Min(expirationDay, daysInCurrentMonth);

    DateTime expirationDateThisMonth = new DateTime(currentYear, currentMonth, expirationDayThisMonth);

    // If the periodic expiration date in the current month is already surpassed,
    // then we need to the date in the next month.
    if (expirationDateThisMonth < DateTime.Now)
    {
        DateTime dateInOneMonth = DateTime.Now.AddMonths(1);
        int nextMonthMonth = dateInOneMonth.Month;
        int nextMonthYear = dateInOneMonth.Year;
        int daysInNextMonth = DateTime.DaysInMonth(nextMonthYear, nextMonthMonth);

        int expirationDayNextMonth = Math.Min(expirationDay, daysInNextMonth);

        DateTime expirationDateNextMonth = new DateTime(nextMonthYear, nextMonthMonth, expirationDayNextMonth);

        return expirationDateNextMonth;
    }
    else
    {
        return expirationDateThisMonth;
    }
}

Any ways to improve this?

Input/output examples:

Final expiration date: 15.12.2019
DateTime.Now: 12.03.2019
Output: 15.03.2019

Final expiration date 11.10.2019
DateTime.Now: 16.04.2019
Output: 11.05.2019

(I know that I cannot change DateTime.Now but it does work as an pseudo-input, which is why I included it).

When working with DateTime it's usually a good idea to not use a hardcoded DateTime.Now. This makes testing such methods very difficult because you cannot mock it so you have to write very tricky tests all with Now. You allso cannot easily switch to UtcNow etc. and you have to know which now it uses internally.

A much better solution would be to use another parameter like DateTime referenceDate. This way you can use any combinations of the two values and create more predictable and easier to understand tests.

Additionally I'd make this method static because it does not access any class fields etc.

Oh, I also need to add that you use excelent variable names! This is exactly how it should be done! ;-)


Finding all neighbours of a node sliding tile So I have some code which finds neighbours of a given State. So the positions the blank tile (which is represented as 0 in my code) can be moved.

I am doing the 3x3 tile puzzle. The goal state is represented like this :

1 2 3
4 5 6
7 8 0

Say I have a problem state which is:

8 5 1
3 4 7
0 6 2

I would call the findNeighbours() method on this State and it should return two states like this .

8 5 1                             8 5 1
3 4 7   (This is a right move)    0 4 7  (This is an up move)
6 0 2                             3 6 2

The reason I'm posting here is because I'm not getting the performance I'd hoped for and I'm assuming this findNeighbours() is the issue as the code is clunky to say the least! I should also point out that state is an array which looks something like state = {1,2,3,4,5,6,7,8,0} and neighbours is an ArrayList which I am adding every State to

   public void findNeighbours() {
        for (int i = 0; i < state.length; i++) {
            if (state[i] == 0) {
                if (i % 3 != 0) {
                    int[] left = new int[9];
                    System.arraycopy(state, 0, left, 0, left.length);
                    int temp = left[i];
                    left[i] = left[i - 1];
                    left[i - 1] = temp;
                    State newState = new State(left,this, "left");
                    this.neighbours.add(newState);

                }
                if (i % 3 != 2) {
                    int[] right = new int[9];
                    System.arraycopy(state, 0, right, 0, right.length);
                    int temp = right[i];
                    right[i] = right[i + 1];
                    right[i + 1] = temp;
                    State newState = new State(right, this, "right");
                    this.neighbours.add(newState);
                }
                if (i > 3) {
                    int[] up = new int[9];
                    System.arraycopy(state, 0, up, 0, up.length);

                    int temp = up[i];
                    up[i] = up[i - 3];
                    up[i - 3] = temp;
                    State newState = new State(up, this, "up");
                    this.neighbours.add(newState);
                }
                if (i < 6) {
                    int[] down = new int[9];
                    System.arraycopy(state, 0, down, 0, down.length);
                    int temp = down[i];
                    down[i] = down[i + 3];
                    down[i + 3] = temp;
                    State newState = new State(down,  this, "down");
                    this.neighbours.add(newState);
                }
                break;
            }
        }
    }

Your code is really hard to read. I suggest the following:

Have four methods slideUp, slideDown, slideLeft, slideRight, each copying the this and applying the slide operation.
In the neighbour generation routine just check all the four, and whichever of them return a non-null result add to the list; finally, return the list.
Cache the index of the zero tile.

I had this in mind:

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Scanner;

public class SlidingTilePuzzleNode {

    private int[] state = new int[9];

    private int zeroTileIndex;

    public SlidingTilePuzzleNode() {
        for (int i = 0; i < state.length - 1; ++i) {
            state[i] = i + 1;
        }

        zeroTileIndex = state.length - 1;
    }

    private SlidingTilePuzzleNode(int[] state, int zeroTileIndex) {
        this.state = state.clone();
        this.zeroTileIndex = zeroTileIndex;
    }

    public SlidingTilePuzzleNode slideUp() {
        int x = getX(zeroTileIndex);
        int y = getY(zeroTileIndex);

        if (y == 0) {
            return null;
        }

        int nextx = x;
        int nexty = y - 1;
        int nextIndex = xyToIndex(nextx, nexty);
        SlidingTilePuzzleNode node = new SlidingTilePuzzleNode(state,
                                                               nextIndex);
        int tmp = node.state[nextIndex];
        node.state[nextIndex] = node.state[zeroTileIndex];
        node.state[zeroTileIndex] = tmp;
        return node;
    }

    public SlidingTilePuzzleNode slideDown() {
        int x = getX(zeroTileIndex);
        int y = getY(zeroTileIndex);

        if (y == 2) {
            return null;
        }

        int nextx = x;
        int nexty = y + 1;
        int nextIndex = xyToIndex(nextx, nexty);
        SlidingTilePuzzleNode node = new SlidingTilePuzzleNode(state,
                                                               nextIndex);
        int tmp = node.state[nextIndex];
        node.state[nextIndex] = node.state[zeroTileIndex];
        node.state[zeroTileIndex] = tmp;
        return node;
    }

    public SlidingTilePuzzleNode slideLeft() {
        int x = getX(zeroTileIndex);
        int y = getY(zeroTileIndex);

        if (x == 0) {
            return null;
        }

        int nextx = x - 1;
        int nexty = y;
        int nextIndex = xyToIndex(nextx, nexty);
        SlidingTilePuzzleNode node = new SlidingTilePuzzleNode(state,
                                                               nextIndex);
        int tmp = node.state[nextIndex];
        node.state[nextIndex] = node.state[zeroTileIndex];
        node.state[zeroTileIndex] = tmp;
        return node;
    }

    public SlidingTilePuzzleNode slideRight() {
        int x = getX(zeroTileIndex);
        int y = getY(zeroTileIndex);

        if (x == 2) {
            return null;
        }

        int nextx = x + 1;
        int nexty = y;
        int nextIndex = xyToIndex(nextx, nexty);
        SlidingTilePuzzleNode node = new SlidingTilePuzzleNode(state,
                                                               nextIndex);
        int tmp = node.state[nextIndex];
        node.state[nextIndex] = node.state[zeroTileIndex];
        node.state[zeroTileIndex] = tmp;
        return node;
    }

    public Collection<SlidingTilePuzzleNode> getNeighbors() {
        List<SlidingTilePuzzleNode> neighborList = new ArrayList<>(4);
        SlidingTilePuzzleNode node = slideUp();

        if (node != null) {
            neighborList.add(node);
        }

        node = slideDown();

        if (node != null) {
            neighborList.add(node);
        }

        node = slideLeft();

        if (node != null) {
            neighborList.add(node);
        }

        node = slideRight();

        if (node != null) {
            neighborList.add(node);
        }

        return neighborList;
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder();
        int index = 0;

        for (int y = 0; y < 3; ++y) {
            for (int x = 0; x < 3; ++x) {
                sb.append(state[index++]);
            }
            sb.append("\n");
        }

        return sb.toString();
    }

    private static int getX(int index) {
        return index % 3;
    }

    private static int getY(int index) {
        return index / 3;
    }

    private static int xyToIndex(int x, int y) {
        return y * 3 + x;
    }

    public static void main(String[] args) {
        SlidingTilePuzzleNode node = new SlidingTilePuzzleNode();
        SlidingTilePuzzleNode tmpNode;
        Scanner scanner = new Scanner(System.in);

        while (true) {
            System.out.println(node);

            String cmd = scanner.nextLine().trim().toLowerCase();

            switch (cmd) {
                case "quit":
                case "exit":
                    System.exit(0);

                case "up":
                    tmpNode = node.slideUp();

                    if (tmpNode != null) {
                        node = tmpNode;
                    }

                    break;

                case "down":
                    tmpNode = node.slideDown();

                    if (tmpNode != null) {
                        node = tmpNode;
                    }

                    break;

                case "left":
                    tmpNode = node.slideLeft();

                    if (tmpNode != null) {
                        node = tmpNode;
                    }

                    break;

                case "right":
                    tmpNode = node.slideRight();

                    if (tmpNode != null) {
                        node = tmpNode;
                    }

                    break;
            }
        }
    }
}

Hope that helps.


Managing database connections for unit tests I have a series of unit test classes that use various connections to a database. Every test class uses two methods with the JUnit annotations BeforeClass and AfterClass to start and stop the database connections like so:

public class AccountTest
{
    private static DatabaseConnection _adminConnection;
    private static DatabaseConnection _salesUserConnection;
    private static DatabaseConnection _salesManagerConnection;
    private static DatabaseConnection _financeUserConnection;

    private static DatabaseHelper _helper;

    private static ArrayList<DatabaseConnection> _connections = new ArrayList<>();

    @BeforeClass
    public static void CreateTestEnvironment()
    {
        _helper = new DatabaseHelper();

        _adminConnection = _helper.DoLogin(SessionType.Administrator);
        _salesUserConnection = _helper.DoLogin(SessionType.SalesUser);
        _salesManagerConnection = _helper.DoLogin(SessionType.SalesManager);
        _financeUserConnection = _helper.DoLogin(SessionType.FinanceUser);

        _connections.add(_adminConnection);
        _connections.add(_salesUserConnection);
        _connections.add(_salesManagerConnection);
        _connections.add(_financeUserConnection);
    }

    @AfterClass
    public static void DestroyTestEnvironment()
    {
        for(DatabaseConnection connection: _connections)
        {
            _helper.DoLogout(connection);
        }
    }
}

I would then pass the connection to my helper object in a test like this for example:

@Test
public void AdministratorCanCreateAccount()
{
    Account account = new Account();
    account = _helper.CreateObject(account, _adminConnection);

    Assert.assertNotNull(account.getId());
}

The login/logout process in CreateTesEnvironment and DestroyTestEnvironment is duplicated throughout many different test classes, with some classes having more connections than others.

How could I manage the connections in a better way so that I'm not just copying and pasting the same BeforeClass and AfterClass code every time but keep them easily accessible to the unit tests?

Did you tried to move this logic to the helper itself ? It can receive a varargs of SessionType to connect and keep them until you tell him to destroy them. It can maintains a Map<SessionType, DatabaseConnection> to give access to them.

/* WARN; This is not production ready. Validation and exceptions handling
 * is missing and the "destroy" method can be improved.
 */
public class DatabasesHelper {
  private final Map<SessionType, DatabaseConnection> connections = new HashMap<>();

  public void prepare(SessionType... sessions) {
    for(SessionType session : sessions) {
      connections.put(session, this.DoLogin(session));
    }
  }

  public void destroy() {
    for(SessionType session : sessions) {
      this.DoLogout(connections.get(session));
    }
    sessions.clear();
  }

}

Please note that your methods names does not follow the Java, naming conventions. You should use lowerCamelCase for them.


Language switcher, consulting browser's localStorage I'm using angular 6 and ngx-translate. This is my switch language function

  switchLanguage(language: string) {
    this.translate.use(language);
    if (language !== 'ar' && document.getElementsByTagName('html')[0].hasAttribute('dir')) {
      document.getElementsByTagName('html')[0].removeAttribute('dir');
    } else if (language === 'ar' && !document.getElementsByTagName('html')[0].hasAttribute('dir')) {
      document.getElementsByTagName('html')[0].setAttribute('dir', 'rtl');
    }
    // if this line worth it?
    if (language !== JSON.parse(localStorage.getItem('lang'))) {
    localStorage.setItem('lang', JSON.stringify(language));
    }
  }

I'm asking about this line in particular

if (language !== JSON.parse(localStorage.getItem('lang')))

If I don't use it, it means that if the language is english and you switch it to english, the localStorage is going to be updated. If I use it however, it means that, every time you switch a language, it's going to read  localstorage to check its value before updating.

Is the cost of just setting the language better than the cost of checking it first?

This method is getting called whenever you switch the language and in app.ts constructor, so basically on every page load. Is there a better way to do it?

The rewrite of LocalStorage is normal practice because of a rewrite time ~equal check time.

You can change the code.

localStorage.setItem('lang', language);

By the way JSON.parse and  JSON.stringify is unnecessary actions in your code because language is already string.

I only considered the code associated with your the question.


Merge Intervals in JavaScript This is a task taken from Leetcode -

  Given a collection of intervals, merge all overlapping intervals.

  Example 1:

Input: [[1,3],[2,6],[8,10],[15,18]]

Output: [[1,6],[8,10],[15,18]]

/** Explanation: Since intervals `[1,3]` and `[2,6]` overlap, merge them into `[1,6]`. */

  Example 2:

Input: [[1,4],[4,5]]

Output: [[1,5]]

/** Explanation: Intervals `[1,4]` and `[4,5]` are considered overlapping. */

My imperative solution -

 /**
   * @param {number[][]} intervals
   * @return {number[][]}
   */
  var merge = function(intervals) {
    const sortedIntervals = intervals.sort((a,b) => a[0] - b[0]);
    const newIntervals = [];

    for (let i = 0; i < intervals.length; i++) {
      if (!newIntervals.length || newIntervals[newIntervals.length - 1][1] < sortedIntervals[i][0]) {
        newIntervals.push(sortedIntervals[i]);
      } else {
        newIntervals[newIntervals.length - 1][1] = Math.max(newIntervals[newIntervals.length - 1][1], sortedIntervals[i][1]);
      }
    }
    return newIntervals;
  };

My functional solution -

/**
 * @param {number[][]} intervals
 * @return {number[][]}
 */
function merge(intervals) {
  const mergeInterval = (ac, x) => (!ac.length || ac[ac.length - 1][1] < x[0]
    ? ac.push(x)
    : ac[ac.length - 1][1] = Math.max(ac[ac.length - 1][1], x[1]), ac);

  return intervals
    .sort((a,b) => a[0] - b[0])
    .reduce(mergeInterval, []);
};

The code is mostly readable and clear:

the variable names are descriptive (for the most part - x is a little unclear)
there is good use of const and let instead of var

Some of the lines are a little lengthy - the longest line appears to be 117 characters long (excluding indentation):

newIntervals[newIntervals.length - 1][1] = Math.max(newIntervals[newIntervals.length - 1][1], sortedIntervals[i][1]);

I considered suggesting that a for...of loop be used to replace the for loop in the imperative solution after seeing the suggestion to use arr.entries() in this answer which would allow the use of a variable like interval instead of sortedIntervals[i], though when comparing in FF and chrome with this jsPerf test it seems that would be slower and thus less optimal, perhaps because each iteration would have an added function call.

When intervals.sort() is called the array is sorted in-place so intervals could be used instead of sortedIntervals, however it does seem that using sortedIntervals. That would reduce the storage by \$O(n)\$.


Displaying data as binary in C with bitmasks I wrote a program to display data in memory as binary and it worked. (It gave me immense sense of satisfaction and joy to see this work. I guess this happens to newbies who are just testing the waters). I know this isn't some terribly smart code, and I'm sure there must be surely lots of better ways to do this. Any advice and help would be great. If I've done something wrong in this program or if there is any bad programming practice, I would be happy to be corrected.

#include<stdio.h>

int main()
{

  char mychar = 'a';
  int count = 0;
  int bitmask = 128;
  while (count < 8){

    if (mychar &  bitmask){
      printf("1");
    }
    else{
       printf("0");
    }

    bitmask = bitmask / 2;
    count = count +1;
  }

return 0;

}

You may use the shift operation;

char mask = 0x01; //define initial mask

if (mask&mychar) printf("1"); // check that spesific bit

mask<<=1; //shift left

of course if you want to start checking from right hand then shift right you can do :

char mask = 0x80;

mask>>=1;


Using Linq to select the first and last values I want to get just the first and last values in a date range. I have the following code:

  using (var myEntities = new dataEntities())
  {
      var myValues = (from values in myEntities.PointValues
                      where values.PointID == dataValue && values.DataTime >= fromDate && values.DataTime <= toDate
                      orderby values.DataTime
                      select new BarChartValue
                      {
                          Time = values.DataTime,
                          Value = values.DataValue
                      }).ToList();
      var myBarChart = new List<BarChartValue> { myValues.First(), myValues.Last() };
      return myBarChart;
  }

This seems inefficient to me as I select the entire date range values initially. I can't seem to find a way to put first and last into the select command. Surely there must be a way to do this?

In response to Svick.

So, if I do this:

      var firstValue = (from values in myEntities.PointValues
                      where values.PointID == dataValue && values.DataTime >= fromDate && values.DataTime <= toDate
                      orderby values.DataTime
                      select new BarChartValue
                      {
                          Time = values.DataTime,
                          Value = values.DataValue
                      }).First();
      var lastValue = (from values in myEntities.PointValues
                        where values.PointID == dataValue && values.DataTime >= fromDate && values.DataTime <= toDate
                        orderby values.DataTime
                        select new BarChartValue
                        {
                            Time = values.DataTime,
                            Value = values.DataValue
                        }).Last();

Is that the best way to do it? I don't have any profiling tools (or if I do, I'm unsure how to use them).

The solution as provided by svick is:

      var myValues = (from values in myEntities.PointValues
                      where values.PointID == dataValue && values.DataTime >= fromDate && values.DataTime <= toDate
                      orderby values.DataTime
                      select new BarChartValue
                          {
                              Time = values.DataTime,
                              Value = values.DataValue
                          });
      var myBarChart = new List<BarChartValue> { myValues.First(), myValues.OrderByDescending(p => p.Time).First() };

A lot of this comes from my lack of understanding of the difference between IQueryable, IEnumerable and List. Here is a good explanation that helped me understand a bit more of what's going on:

https://stackoverflow.com/questions/4844660/differences-between-iqueryable-list-ienumerator

What you could do is to remove the ToList() from your query. This would change your code from making a single query that returns many values to two queries, each returning a single value.


Finding the two largest three digit numbers missing from a text file Hear this problem: Write a C/C++ program that reads all the values from the "BAC.TXT" file and determines which are the two largest three digit numbers that are missing from the file using a time-efficient algorithm. This is the code that I wrote and it returned good values for all the input sets I tried:

#include <iostream>
#include <fstream>

using namespace std;

int main()

{   ifstream in("BAC.TXT");

    unsigned int a[1000], missing_numbers[2];
    unsigned int data, i = 0, j = 0, found_missing_numbers = 0;

    while(!in.eof())
    {   in >> data;

        if (data >= 100 && data <= 999)
            a[data] = 1;
    }

    for(i=999;i>=100;i--)
    {if(a[i]!=1)
    {   missing_numbers[j] = i;
        found_missing_numbers++;
        j++;
    }
    if (found_missing_numbers == 2)
        break;
    }

    if (found_missing_numbers == 2)
    {
        cout << "The two largest numbers of three digits that are missing from the file are:";
        cout << missing_numbers[0] << " and " << missing_numbers[1];
    }
    else if (found_missing_numbers == 1)
        cout << "Only one three digit number is missing from the file:" << missing_numbers[0];
    else
        cout << "There are no three digit numbers missing from the file.";

    cin.get();

    in.close();

    return 0;
}

Example of Input:

  125 88556 2534 12 456668 12 56324 4521 3 652 999 94 24521 6524 1112 125 997 12315 122 54 9 5546 754 998 12312 12152 745 2354 856 124 956 125 88556 2534 12 456668 12 56324 4521 3 652 999 94 24521 6524 1112 125 997 12315 122 54 9 5546 754 998 12312 12152 745 2354 856 124 956 125 88556 2534 12 456668 12 56324 4521 3 652 999 94 24521 6524 1112 125 997 12315 122 54 9 5546 754 998 12312 12152 745 2354 856 124 956 125 88556 2534 12 456668 12 56324 4521 3 652 999 94 24521 6524 1112 125 997 12315 122 54 9 5546 754 998 12312 12152 745 2354 856 124 956 125 88556 2534 12 456668 12 56324 4521 3 652 999 94 24521 6524 1112 125 997 12315 122 54 9 5546 754 998 12312 12152 745 2354 856 124 956 125 88556 2534 12 456668 12 56324 4521 3 652 999 94 24521 6524 1112 125 997 12315 122 54 9 5546 754 998 12312 12152 745 2354 856 124 956 125 88556 2534 12 456668 12 56324 4521 3 652 999 94 24521 6524 1112 125 997 12315 122 54 9 5546 754 998 12312 12152 745 2354 856 124 956 125 88556 2534 12 456668 12 56324 4521 3 652 999 94 24521 6524 11

Output:

  The two largest numbers of three digits that are missing from the file are:996 and 995

I consider the code quite efficient because the file is read a single time and it selects and works only with the data that could help to find the solution. I would like to hear your opinion, other suggestions, all that you consider that could make this solution more efficient.

P.S. I am a beginner in programming, don't know any OOP for the moment. I know the code I wrote is something like C with cout and cin...not C++. Don't kill me. This question is based on the same problem that I was solving here, but now I have a working solution. Now it's only a matter of optimisation.

Reading input

In this specific case it doesn't make a difference, but try to break the habit of using code like:

while (!some_file.eof())

Under most circumstances this leads to incorrect behavior--in particular, it will typically result in processing the last item in the file twice. In this code, we're just setting a value in an array to 1, so setting it to 1 again doesn't change anything, and the bug is hidden. Still better to avoid it.

while(in >> data)
{
    if (data >= 100 && data <= 999)
        a[data] = 1;
}

indentation

I'm a lot less than excited about your formatting, putting the first statement in a block on the same line as the block's opening brace:

while(!in.eof())
{   in >> data;

...

for(i=999;i>=100;i--)
    {if(a[i]!=1)

The usual advice is to pick a style and use it consistently. I'd add one item to that though: unless you have a really good reason to do otherwise, pick one of the styles that's already reasonably well know and widely used (e.g., K&R, OTBS, etc.)

Standard containers

Right now, your a is basically acting as a set. That it, it simply records what numbers are present/absent in a range. The standard library already provides std::set (and std::unordered_set, though it's probably not the best choice here) specifically for that purpose. It might be worth at least considering using one of these.

Simplification

Since we care about the numbers that are missing (rather than the ones that are present), we probably want to invert the sense of our set. Rather than inserting numbers as we encounter them, then searching for what's missing, we can start by inserting all the numbers from 100 to 999, then remove the numbers as we read them from the file.

This way, when we're done reading the data from the file, our set contains exactly what we care about: the numbers in the specified range that were not present in the file (if any).

Note that this can be a little bit of a tradeoff. If we expect a lot of numbers to be missing, we could be adding a fair amount of extra work by inserting all the numbers to start with, then removing them as we read. In a case like this, with only ~900 items in the range, that's probably irrelevant though--inserting 900 items in a set is fast enough that it usually won't matter nearly as much as simplifying the code can/does.

Standard algorithms

We can also use standard algorithms to ease the job a bit, such as using std::copy_n to copy the last two items in the set to std::cout, assuming we find two or more missing numbers.

Use of braces

A fair number of people advise using braces around the statement(s) controlled by every if, while, for, etc. Personally I don't like (or usually follow) this advice, but it's pretty widespread, and if you decide to do otherwise, you should (IMO) be consistent about whether to do it or not.

Summary

Code incorporating all these could look something like this:

std::set<int> s;

for (int i = 100; i < 1000; i++)
    s.insert(s.end(), i);

int data;

while (in >> data)
    s.erase(data);

if (s.empty())
    std::cout << "All numbers were present\n";
else if (s.size() == 1)
    std::cout << "The one missing number was: " << *s.begin() << "\n";
else {
    std::cout << "The two largest missing numbers were: ";
    std::copy_n(s.rbegin(), 2, std::ostream_iterator<int>(std::cout, " "));
}


Cops and robbers program to find the most frequent character in a string I made this program to train me for a competition. The hypothetical question I've used was

  The cops need help finding who is the most probable criminal to commit a crime.
  Witnesses tell them multiple different people, making it hard to find the criminal.
  Use your knowledge of coding and the one-letter identifications provided for the criminals
  to find who most likely did it and print.

It was fun making the code; however, it felt like it took too long for such a simple code. Is there any way to make this simpler or better?

import java.util.*;

public class CopsAndRobbers {

    public static void main(String[] args) {
        //The cops need help finding who is the most probable criminal to commit a crime
        //Witnesses tell them multiple different people, making it hard to find the criminal
        //Use your knowledge of coding and the one-letter identifications provided for the criminals
        //To find who most likely did it and print.

        TreeSet<String> done = new TreeSet<String>();
        ArrayList<String> all = new ArrayList<String>();
        ArrayList<Integer> num = new ArrayList<Integer>();
        ArrayList<String> some = new ArrayList<String>();
        @SuppressWarnings("resource")
        Scanner sc = new Scanner(System.in);
        String input = sc.nextLine();

        for(int i = 0; i < input.length(); i++)
        {
            done.add(String.valueOf(input.charAt(i)));
            all.add(String.valueOf(input.charAt(i)));
        }
        int amount = done.size();
        for(int i = 0; i < amount; i++)
        {
            String letter = done.first();
            int tempNum = 0;
            for(int j = 0; j < all.size(); j++)
            {
                if(all.get(j).equals(letter))
                {
                    tempNum++;
                }
            }
            num.add(tempNum);
            some.add(letter);
            done.remove(letter);
        }
        int highest = 0;
        int overallIndex = 0;
        for(int i = 0; i < num.size(); i++)
        {
            if(num.get(i) > highest)
            {
                highest = num.get(i);
                overallIndex = i;
            }
        }
        System.out.println("The most likely criminal is " + some.get(overallIndex) + " with " + highest + " witnesses.");
    }

}

Sample Input

aggdhaamfa

Sample Output

The most likely criminal is a with 4 witnesses.

for(int i = 0; i < input.length(); i++)
{
    done.add(String.valueOf(input.charAt(i)));
    all.add(String.valueOf(input.charAt(i)));
}

Instead of iterating over the input string like that, converting every single character to a new String, actually doing all of this twice, and adding each to a collection, you can convert the string to a list of Strings (for each character) like this:

List<String> inputChars = Arrays.asList(input.split(""));

Note that previously to Java 8 this results in the first element being an empty string, which must be handled accordingly.

Then add all of the characters in the array to the collections:

done.addAll(inputChars);
all.addAll(inputChars);

int highest = 0;
int overallIndex = 0;
for(int i = 0; i < num.size(); i++)
{
    if(num.get(i) > highest)
    {
        highest = num.get(i);
        overallIndex = i;
    }
}

To better structure your code, move the loop to a method findIndexOfHighest(). Your overallIndex actually is the indexOfHighest. Then you get the index of the highest element and the highest element like this:

int indexOfHighest = findIndexOfHighest();
int highest = num.get(indexOfHighest);

Add blank lines between blocks of code, e. g. between for loops. That makes your code much more readible.

int amount = done.size();
for(int i = 0; i < amount; i++)
{
    String letter = done.first();
    int tempNum = 0;
    for(int j = 0; j < all.size(); j++)
    {
        if(all.get(j).equals(letter))
        {
            tempNum++;
        }
    }
    num.add(tempNum);
    some.add(letter);
    done.remove(letter);
}

Here the inner loop can be a for-each loop. Also it should be in a separate method, because what it does is countOccurences(String s, char c). The variable tempNum should be called count or occurences.


Analyzing pair by pair DNA sequences with for loops in Python Here is a code I would need to make far faster: it is made to analyse more than 3000 DNA sequences of more than 100 000 characters each.

The matter is I have to compare it by pair, so that would be around 3000*2999/2 = 4498500 calculation and each takes 1 to 2 seconds...

Could you think of another way of doing it ? I've seen that there are algorithms that are used to go faster (like Boyer-Moore), but could we apply it here ? Or replace the for loops with something faster ? Any idea would be more than welcome.

import numpy as np
import time
import random

def calculate_distance(genom1, genom2):
    # in : two strings (sequences)
    # out : score between those 2 strings

    # example : 'AATTGCAT' compared to 'AAAACGTC'
    # => 'AA' and 'AA' = 2
    # => 'TT' and 'AA' = 0
    # => 'GC' and 'CG' = 2
    # => 'AT' and 'TC' = 1

    score = 0
    for i in range(0, len(genom1), 2):
        if (genom1[i] in genom2[i:i+2]) or (genom1[i+1] in genom2[i:i+2]):
            if sorted(genom1[i:i+2]) == sorted(genom2[i:i+2]):
                score += 2
            else :
                score += 1
    return score

def build_matrix(sequences, N):
    # in : list of lists
    # out : matrix of scores between each pair of sequences
    matrix = np.zeros((N, N))
    for i in range(N):
        for j in range(i, N):
            matrix[i][j] = calculate_distance(sequences[i], sequences[j])
    return matrix

def test(len_seq, N):
    sequences = []
    for i in range(N):
        sequences.append(''.join(random.choice(['0','A', 'T', 'C', 'G']) for x in range(len_seq)))

    start = time.clock()
    matrix = build_matrix(sequences, N)
    elapsed = time.clock() - start
    print('Run time for ' + str(N) + ' sequences of ' + str(len_seq) + ' characters : computed in ' + str(elapsed) + ' seconds')
    return matrix

test(10**6, 2)

Returns :

Run time for 2 sequences of 1000000 characters : computed in 2.742817 seconds

I notice something small, well you know the corner cases. You know that a DNA string will yield a score of that is the same as length. Obviously ^^. So don't check those cases.

You make 4 iterations in your code. While only one is interesting.

A big improvement in runtime could be to change:

def build_matrix(sequences, N):
    # in : list of lists
    # out : matrix of scores between each pair of sequences
    matrix = np.zeros((N, N))
    for i in range(N):
        for j in range(i, N):
            matrix[i][j] = calculate_distance(sequences[i], sequences[j])
    return matrix

to this:

def build_matrix(sequences, N):
    matrix = np.zeros((N, N))
    for i in range(N):
        for j in range(i+1, N):
            matrix[i][j] = calculate_distance(sequences[i], sequences[j])
    return matrix

If you could load your data as:

def get_test_arr():
    parts = ['0', 'A', 'T', 'C', 'G']
    return ["".join(
        sorted([random.choice(parts) for _ in range(2)]))
            for _ in range(5*10**5)]

you could make them np arrays as:

a = np.array(get_test_arr(), dtype=object)
b = np.array(get_test_arr(), dtype=object)

and have your distance calculation as:

def calculate_distance1(genom1, genom2):
    score = 0
    for g1, g2 in zip(genom1, genom2):
        if (g1[0] in g2) or (g1[1] in g2):
            score += 2 if g1 == g2 else 1
    return score

In the problem you've provided each induvidual pairs order does not seem to matter, so load them as such. If you do that, you don't have to sort the in the calculation function.

If you don't, you'll resort them later. Again and again. Them more samples you use, there more time is spent doing this.

Times:

Your calculation function

0.575332 s for one loop in 100 iterations.

The my solution

0.10583 s for one loop in 100 iterations.


Extension Method for Creating Types from Regular Expression Match Groups I've been doing some parsing with regular expression named capture groups and decided it might make sense to write an extension to handle this.

The code below will create an instance of a specified type and attempt to match the property names to the capture group names and then set the values.  It also attempts to cast between compatible types.

Any suggestions for improving this code? Is it ok as an extension?

 public static class RegularExpressionExtension
{
    public static T CreateType<T>(this Regex regEx, string matchString) where T : new()
    {
        MatchCollection matchCollection = regEx.Matches(matchString);
        T obj = new T();
        Type type = typeof(T);
        PropertyInfo[] properties = type.GetProperties();

        IEnumerable<string> groupNames = regEx.GetGroupNames().Skip(1); //First Group is always 0
        if (matchCollection.Count >= 1)
        {
            SetPropertyValuesFromGroupMatches<T>(groupNames, regEx, obj, matchCollection[0].Groups, properties);
        }
        return obj;
    }

    public static IEnumerable<T> CreateTypeCollection<T>(this Regex regEx, string matchString) where T : new()
    {
        MatchCollection matchCollection = regEx.Matches(matchString);
        IEnumerable<string> groupNames = regEx.GetGroupNames().Skip(1); //First Group is always 0
        Type type = typeof(T);
        PropertyInfo[] properties = type.GetProperties();

        foreach (Match match in matchCollection)
        {
            T obj = new T();
            SetPropertyValuesFromGroupMatches<T>(groupNames, regEx, obj, match.Groups, properties);
            yield return obj;
        }
    }

    private static void SetPropertyValuesFromGroupMatches<T>(IEnumerable<string> groupNames, Regex regEx, T typeToPopulate, GroupCollection groupCollection, PropertyInfo[] properties)
    {
        Type type = typeof(T);

        foreach (string group in groupNames)
        {
            var foundProperty = properties.SingleOrDefault(p => p.Name.Equals(group,StringComparison.CurrentCultureIgnoreCase));
            if (foundProperty != null)
            {
                var matchValue = groupCollection[group].Value;
                object convertedValue = System.ComponentModel.TypeDescriptor.GetConverter(foundProperty.PropertyType).ConvertFrom(matchValue);
                foundProperty.SetValue(typeToPopulate, convertedValue, null);
            }
        }
    }
}

If you are doing reflection magics the caller of your method may be as well. If that person already has a Type it can be a bit of a pain in the butt to call your generic method. Since you are already working against a Type offering an overload for it should not be too much work. Perhaps extracting the following code into its own method may help:

PropertyInfo[] properties = type.GetProperties();
IEnumerable<string> groupNames = regEx.GetGroupNames().Skip(1); //First Group is always 0
if (matchCollection.Count >= 1) {
    SetPropertyValuesFromGroupMatches<T>(groupNames, regEx, obj, matchCollection[0].Groups, properties);
}


Extracting length-3 subsequences from a list For a list li, I want to print only the values of the triplets a, b, c where 0 <= index(a) < index(b) < index(c) < len(li).

Example:

f([6, 3, 88, 4])
6 3  88
6 3  4
6 88 4
3 88 4

Here is my solution, but it is O(n**3) (time complexity):

def f(li):
    for i, n in enumerate(li):
        for j, o in enumerate(li):
            for k, p in enumerate(li):
                if i < j < k:
                    print n, o, p

However, it should be doable in O(n). Any clues?

I have the following function as well:

def f(li):
    i = 0
    j = i + 1
    k = j + 1
    n = li[i]
    o = li[j]
    p = li[k]
    while i < j < k:
        print n, o, p
        if n == li[-3]:
            break
        if p != li[-1]:
            k += 1
        elif o == li[-2]:
            i += 1
            j = i + 1
            k = j + 1
        elif p == li[-1]:
            j += 1
            k = j + 1

        n = li[i]
        o = li[j]
        p = li[k]

I believe this is O(n).

In any case, the minimum time complexity for a list of length \$n\$ would be $$\mathrm{nCr}(n, 3) = \frac{n!}{3!\ (n-3)!} = \frac{1}{6} n(n - 1)(n - 2)$$ because that is the number of results to expect.  That is \$O(n^3)\$.

  Explanation of the combination formula:

  Let's ignore the i < j < k requirement for now.  How many ways are there to pick one element?  (There are \$n\$ ways.)  How many ways are there to pick the next element from the remaining \$n-1\$ elements?  (For each of the \$n\$ first picks, there are \$n-1\$ second picks.)  How many choices are there for the third element?  (There are \$n-2\$.)  That means that there are \$n(n-1)(n-2)\$ permutations when picking 3 elements from a list of \$n\$, also written as \$\frac{n!}{(n-3)!}\$.

  Now, let's consider the ordering.  For any collection of 3 items, how many possible sequences exist?  There are 3 choices for the first item, 2 choices for the second item, and 1 obligatory choice for the third.  That's \$3!=3\cdot2\cdot1=6\$ orderings.  So, out of all the \$n(n-1)(n-2)\$ lists of of \$i, j, k\$, only \$\frac{1}{6}\$ of the them will have \$i, j, k\$ in ascending order.

What you can do, though, is eliminate the conditional, so that every iteration succeeds in producing a result.  (Your if i < j < k check succeeds less than \$\frac{1}{6}\$ of the time.)  I would also change print to yield for greater flexibility.

def subsequence_triplets(lst):
    end = len(lst)
    for i in range(0, end - 2):
        for j in range(i + 1, end - 1):
            for k in range(j + 1, end):
                yield lst[i], lst[j], lst[k]

for a, b, c in subsequence_triplets([6, 3, 88, 4]):
    print a, b, c


Split a String On A Word By Words I'm working on a text based rpg game and a couple of commands make use of a target and container pattern. So I wrote this function to split the string up on a given word and put it back together. I'm not sure how efficient it will be in the real world, but I think that it can be improved.

An example of such input and output would be:

splitOnWord("turkey leg from table", "from") -> {"turkey leg", "table"}

typedef std::vector<std::string> StringVector;

StringVector splitOnWord(std::string line, std::string word){
    std::stringstream sstr(line);
    StringVector tmp;
    std::string str;
    int rescnt = 0;

    sstr >> str;
    tmp.push_back(str);
    while(sstr >> str) {
        if(str == word) {
            ++rescnt;
            tmp.push_back(std::string());
        } else {
            tmp[rescnt] += ' ';
            tmp[rescnt] += str;
        }
    }
    return tmp;
}

I'm not overly fond of the tmp.push_back(std::string());.

I'd rather some of the names were clearer and more readable--for example, I'm not entirely sure what rescnt is supposed to mean.

Under the circumstances, I'd tend to wonder whether we're getting enough good out of the stringstream to justify its existence either. They can be handy, but here we don't seem to be putting the capabilities to a whole lot of use.

std::vector<std::string> split(std::string const &in, std::string const &word) {
    std::vector<std::string> ret;

    int begin = 0;
    int end = 0;

    while (std::string::npos != (end = in.find(word, begin))) {
        ret.push_back(in.substr(begin, end-begin));
        begin = end + word.length()+1;
        while (::isspace(in[begin]))
          ++begin;
    }
    ret.push_back(in.substr(begin));
    return ret;
}


Graph implementation adjacency list 1.0 Please note the following:

All edges are directed
Removal of edges/nodges is not implemented yet
A user of the class Graph can never (hopefully) access a Node directly
If one tries to add an edge to a node that does not exist, insert_edge() will do nothing.
A Vertex cannot point to another vertex with several edges.
While the base class Graph works perfectly, I would like to create more advance graphs that could be used for maximum flow algorithm, weighted graphed etc. by creating new classes and inherit functions from Graph. But this seems impossible by the current implementation. It seems that the only way for a smooth solution for this right now is to add more data members to Node (int weight for example). Any idea on what I can do for making this possible with inheriting from Graph instead?

class Node
{
public:

std::string get_name()const;

//protected:
Node() = default;

Node( const std::string & n )
: name(n)
{}

std::vector<Node*> neighbours;
private:
std::string name;
};

class Graph : public Node
{
public:
void insert_node( std::string);
void insert_edge( std::string, std::string);
void print_node( const std::string) const;
void print_graph() const;

protected:
void insert_edge( Node*, Node*);
Node* get_node( std::string ) const;
bool is_node( Node * );

private:
std::vector<Node*> list;
};

std::string
Node::get_name() const
{
return name;
}

void
Graph::insert_node( std::string name)
{
for( auto& node  : list  )
{
  if( name == node -> get_name() )
  {
     return;
  }
}

Node *temp = new Node(name);
list.push_back ( temp );
}

void
Graph::insert_edge( Node *n, Node *new_edge )
{
if( n == nullptr || new_edge == nullptr )
{
  return;
}

if( !is_node( n ) )
{
  return;
}

for( auto& node : list )
{
  if( n -> get_name() == node -> get_name() )
  {
     for( auto& current_edges : node -> neighbours )
     {
        if( current_edges -> get_name() == new_edge -> get_name() )
        {
           return;
        }
     }
     node -> neighbours.push_back( new_edge );
  }
}
}

void
Graph::insert_edge( std::string n, std::string new_edge)
{
Node* temp1 = get_node( n );
Node* temp2 = get_node( new_edge );
insert_edge( temp1, temp2 );
}

Node*
Graph::get_node( std::string name ) const
{
   for( auto& node : list )
{
  if( name == node -> get_name() )
  {
     return node;
  }
}
return nullptr;
}

void
Graph::print_node( std::string name ) const
{
std::cout << name << " : ";
for( auto&  node : list )
{
if(name == node -> get_name() )
  {
     for( auto& edge : node -> neighbours )
     {
        std::cout << " -> " << edge -> get_name();
     }
     std::cout << std::endl;
     break;
  }
}
}

void
Graph::print_graph() const
{
for( auto&  node : list )
{
  std::cout << node -> get_name() << " : ";
  for( auto& edge : node -> neighbours )
  {
   std::cout << " -> " << edge -> get_name();
  }
  std::cout << std::endl;
}
}

bool
Graph::is_node( Node *n )
{
for( auto& node : list )
{
  if( node -> get_name() == n -> get_name() )
  {
     return true;
  }
}
return false;
}

I don't like that std::vector<Node*> neighbours; is a data member with public scope in Node. I would suggest doing it private and provide a getter for it, or provide a Node::push_back(), etc.

As for you question of adding a data member weight in Graph, I think that's not very nice, since - in my mind - weight is an attribute every node has, thus it should be a data member of Node.


Most common letter in string, trying to use idiomatic Ruby The task:

  Write a method that takes in a string. Your method should return the
  most common letter in the array, and a count of how many times it
  appears.

Model solution:

def most_common_letter(string)
  most_common_letter = nil
  most_common_letter_count = nil

  idx1 = 0
  while idx1 < string.length
    letter = string[idx1]
    count = 0

    idx2 = 0
    while idx2 < string.length
      if string[idx2] == letter
        count += 1
      end
      idx2 += 1
    end

    if (most_common_letter_count == nil) || (count > most_common_letter_count)

      most_common_letter = letter
      most_common_letter_count = count
    end

    idx1 += 1
  end

  return [most_common_letter, most_common_letter_count]
end

My solution:

def most_common_letter(string)

most_common_letter = nil
most_common_letter_count = 0

string.delete(" ").each_char do |letter|
  letter_count = 0
  string.each_char {|test| letter_count+=1 if test == letter }

  most_common_letter = letter and most_common_letter_count = letter_count if letter_count > most_common_letter_count
end

[most_common_letter, most_common_letter_count]

end

I tried using more idiomatic Ruby to solve this beginner problem we got in the beginning of the course. I'm wondering about a few things:

Is using "and" as I did in the if statement idiomatic? I tried a few guesses as to what might work (&&, &, some random characters), but I haven't seen this before.
I think the delete method alters the original string, and from the wonderful people on this site I've learned that altering the original data is unwise. How else could I ignore the spaces?
How can I tighten this up?

Your solution has common code smell: A block that modifies closed over variables (i.e. your block has side-effects, which should be avoided).

The code is also quite inefficient. For each character in a string, you loop through the entire string in an inner loop - even if you've done it for that letter already. E.g. a string like "aaa" will be doing a lot of unnecessary work. Besides, you could get the number of occurrences with some_string.count(some_letter).

It would be much better to run through the string once, and count letter occurrences by adding them, one by one, to a hash, and when seeing a letter you've seen before, incrementing its count. After that you have each letter and its number of occurrences, making it simple to pick out the most common.

In terms of style, that and line is... not very nice at all. You seem to be insisting on post-fixing conditionals (i.e. ... if x). And that'd be ok for small stuff, like x += 1 if y. But here, you have two separate lines of code, and you're glueing them together with and instead of just, well, using two lines:

if letter_count > most_common_letter_count
  most_common_letter = letter
  most_common_letter_count = letter_count
end

The and operator has its place, but it isn't this. You should read up on it.

For your second question: No, #delete does not alter the string. It returns a new string without the parts you asked to have deleted.

Anyway, an overall much more Ruby'esque approach would be something like this:

def most_common_letter(string)
  string
    .gsub(/[^a-z]/, '')  # remove non-letters
    .chars               # get characters
    .group_by(&:itself)  # group them
    .map { |letter, list| [letter, list.count] } # get their counts
    .max_by(&:last)      # get the most common letter and its count
end

It will return nil where yours returns [nil, nil] for an empty string. I prefer the former, rather than getting a array that has to be inspected more closely to see if it contains anything. But if the method should always return an array, you can do this:

def most_common_letter(string)
  most_common = string
    .gsub(/[^a-z]/, '')  # remove non-letters
    .chars               # get characters
    .group_by(&:itself)  # group them
    .map { |letter, list| [letter, list.count] } # get their counts
    .max_by(&:last)      # get the most common letter and its count
  most_common || [nil, nil]
end