Generating a word bigram co-occurrence matrix I have written a method which is designed to calculate the word co-occurrence matrix in a corpus, such that element(i,j) is the number of times that word i follows word j in the corpus.

Here is my code with a small example:

import numpy as np
import nltk
from nltk import bigrams

def co_occurrence_matrix(corpus):
    vocab = set(corpus)
    vocab = list(vocab)

    # Key:Value = Word:Index
    vocab_to_index = { word:i for i, word in enumerate(vocab) }

    # Create bigrams from all words in corpus
    bi_grams = list(bigrams(corpus))

    # Frequency distribution of bigrams ((word1, word2), num_occurrences)
    bigram_freq = nltk.FreqDist(bi_grams).most_common(len(bi_grams))

    # Initialise co-occurrence matrix
    # co_occurrence_matrix[current][previous]
    co_occurrence_matrix = np.zeros((len(vocab), len(vocab)))

    # Loop through the bigrams in the frequency distribution, noting the
    # current and previous word, and the number of occurrences of the bigram.
    # Get the vocab index of the current and previous words.
    # Put the number of occurrences into the appropriate element of the array.
    for bigram in bigram_freq:
        current = bigram[0][1]
        previous = bigram[0][0]
        count = bigram[1]
        pos_current = vocab_to_index[current]
        pos_previous = vocab_to_index[previous]
        co_occurrence_matrix[pos_current][pos_previous] = count

    co_occurrence_matrix = np.matrix(co_occurrence_matrix)

    return co_occurrence_matrix

test_sent = ['hello', 'i', 'am', 'hello', 'i', 'dont', 'want', 'to', 'i', 'dont']
m = co_occurrence_matrix(test_sent)

Output:

[[0. 2. 0. 0. 0. 0.]
 [0. 0. 0. 1. 0. 2.]
 [0. 1. 0. 0. 0. 0.]
 [0. 0. 0. 0. 1. 0.]
 [1. 0. 0. 0. 0. 0.]
 [0. 0. 1. 0. 0. 0.]]

Whilst the example shown works fine, when I scale this up to a much larger corpus, I get the following Killed:9 error. I assume this is because the matrix is very large.

I am looking to make this method more efficient so that I can use it for large corpuses! (A few million words.)

A 106 × 106 matrix would contain 1012 entries.  Optimistically assuming one byte per entry, that would already be 1 TB.  I would expect that most of the matrix entries will be 0. Consider looking into sparse matrices.


How can i make my Get_Numb_of_Digits_in_a_number() function more accurate? Here's my function but it sometimes has unpredicted results with entering different numbers but with the same number of digits getting different results. I figure it has something to do with a computers inaccurate decimal saving tecnique but i would like to know how to make it more accurate.

///////////////////////     Numb_Digits()   ////////////////////////////////////////////////////
        enum{DECIMALS = 10, WHOLE_NUMBS = 20, ALL = 30};
    template<typename T>
unsigned long int Numb_Digits(T numb, int scope)
{
        unsigned long int length= 0;
        unsigned long int whole_numb= (int)numb;
            // gets the number of whole numbers
        for(int mod =0; mod!=whole_numb; length++)
            mod = whole_numb% (int)pow(10, (double)length);
        int numb_of_wholes= length-1; length= 0; bool all = false;
        double test = 0, test2= 0;
    switch(scope){
        case ALL: all = true;       // different order to avoid decimal inaccuracies
                                    // like 2.4 being 2.39999                         // 10 bil (10 zeros)
        case DECIMALS:                      numb*=10000000000;      numb-=whole_numb*10000000000;
             for(; numb != 0; length++)
                numb-=((int)(numb/pow((double)10, (double)(9-length))))* pow((double)10, (double)(9-length));
             if(all != true) break;

        case WHOLE_NUMBS: length += numb_of_wholes;
             if(all != true) break;

        default: break;}
                                            return length;
};

To get the number of whole numbers, you can just use log10.

int wholeNumbers = ( number == 0 ) ? 1 : (int)log10( abs( number ) ) + 1;

I implemented your function in C#. By using the decimal type for the calculation of the numbers after the decimal sign, precision issues are automatically solved. Most likely a simple for loop is more performant than using pow as in your solution. The result of numbers after the decimal sign for infinite precise numbers (like PI) is dependent on the precision of the type. With this code, I get 14 digits after comma for PI.

public static int AmountOfDigits( double number, DecimalPart decimalPart = DecimalPart.WholeNumbers )
{
    double absNumber = Math.Abs( number );
    int amountOfDigits = 0;

    // Amount of digits before decimal sign.
    if ( decimalPart == DecimalPart.WholeNumbers || decimalPart == DecimalPart.EntireDecimal )
    {
        amountOfDigits += (absNumber == 0) ? 1 : (int)Math.Log10( absNumber ) + 1;
    }

    // Amount of digits after decimal sign.
    if ( decimalPart == DecimalPart.AfterDecimalSign || decimalPart == DecimalPart.EntireDecimal )
    {
        // Use decimal for more precision!
        decimal afterDecimalSign = (decimal)absNumber;
        while ( afterDecimalSign - Math.Truncate( afterDecimalSign ) != 0 )
        {
            amountOfDigits++;
            afterDecimalSign *= 10;
        }
    }

    return amountOfDigits;
}

You do have to ask yourself where you will use this code however. When you are going to use it to calculate correct padding for string output, it's probably better to just convert the numbers to strings and count the amount of characters. It would be an interesting benchmarking test to compare that solution to this one.


Determine whether one vector is a prefix of the other Exercise 5.17:

  Given two vectors of ints, write a program to determine
  whether one vector is a prefix of the other. For vectors of unequal
  length, compare the number of elements of the smaller vector. For
  example, given the vectors containing 0, 1, 1, and 2 and 0, 1, 1, 2,
  3, 5, 8, respectively your program should return true.

This executes correctly, but I just want to get this double-checked.

#include <iostream>
#include <vector>

using std::vector; using std::cout; using std::endl;

int main()
{
vector<int> v1{ 0, 1, 1, 2, 3, 5, 8 };
vector<int> v2{ 0, 1, 1, 2 };
if (v1.size() < v2.size()){
    cout << "Using v1 as the prefix to v2" << endl;
    for (int smallIndex = 0, largeIndex = 1; smallIndex != v1.size(); ++smallIndex, ++largeIndex){
        if (v1[smallIndex] == v2[largeIndex])
            cout << "v1 at position " << smallIndex << " is a prefix for v2 at postion " << largeIndex << endl;
    }
}
else if (v1.size() > v2.size()){
    cout << "Using v2 as the prefix to v1" << endl;
    for (int smallIndex = 0, largeIndex = 1; smallIndex != v2.size(); ++smallIndex, ++largeIndex){
        if (v2[smallIndex] == v1[largeIndex])
            cout << "v2 at position " << smallIndex << " is a prefix for v1 at position " << largeIndex << endl;
    }
}
else {      //  v1.size() == v2.size() only comparing the first as the prefix to the second
    cout << "Size of v1 and v2 are the same, using v1 as the prefix to v2" << endl;
    for (int smallIndex = 0, largeIndex = 1; largeIndex != v1.size(); ++smallIndex, ++largeIndex){
        if (v1[smallIndex] == v2[largeIndex])
                cout << "v1 at position " << smallIndex << " is a prefix for v2 at postion " << largeIndex << endl;
        }
    }
}

For a prefix search, you can use std::mismatch() in <algorithm>.

bool isPrefix(const std::vector<int>& v1, const std::vector<int>& v2) {
    if (v1.size() > v2.size()) {
        return v2.end() == std::mismatch(v2.begin(), v2.end(), v1.begin()).first;
    }
    return v1.end() == std::mismatch(v1.begin(), v1.end(), v2.begin()).first;
}


Calculating min and max with if-statements within a for-loop This code section is from the main method of my Annual Fuel Use class. The program projects my annual fuel usage based on at least three fill ups of my car.
Here, I am calculating the max and min for distance (which is miles traveled), MPG, and price per gallon.  I'd like a general review.

//initialization of array of objects
AnnualFuelUse[] fillUps = {new AnnualFuelUse (1, 1, 6500, 6800, 9.70, 3.11),
                          new AnnualFuelUse (2, 10, 6800, 7052, 8.10, 3.08),
                          new AnnualFuelUse (3, 20, 7052, 7349, 9.20, 3.15)};

//calculate Min and Max for distance, MPG, and price per gallon
double minDist = 0, maxDist = 0;
double minMPG = 0.0, maxMPG = 0.0, minPrice = 0.0, maxPrice = 0.0;

Double dMin = Double.MAX_VALUE;
Double dMax = Double.MIN_VALUE;
Double mpgMin = Double.MAX_VALUE;
Double mpgMax = Double.MIN_VALUE;
Double priceMin = Double.MAX_VALUE;
Double priceMax = Double.MIN_VALUE;

for (int i = 0; i < fillUps.length; i++) {
    if (fillUps[i].getDist() < dMin){
        dMin = fillUps[i].getDist();
        minDist = dMin;
    }
    if (fillUps[i].getDist() > dMax) {
        dMax = fillUps[i].getDist();
        maxDist = dMax;
    }
    if (fillUps[i].getMilesPerGallon() < mpgMin) {
        mpgMin = fillUps[i].getMilesPerGallon();
        minMPG = mpgMin;
    }
    if (fillUps[i].getMilesPerGallon() > mpgMax) {
        mpgMax = fillUps[i].getMilesPerGallon();
        maxMPG = mpgMax;
    }
    if (fillUps[i].getPrice() < priceMin) {
        priceMin = fillUps[i].getPrice();
        minPrice = priceMin;
    }
    if (fillUps[i].getPrice() > priceMax) {
        priceMax = fillUps[i].getPrice();
        maxPrice = priceMax;
    }
}

The code violates Tell, Don't Ask:

  Procedural code gets information then makes decisions. Object-oriented code tells objects to do things.
  ~Alec Sharp

Before we get to that, consider:

fillUps[i].getDist() > dMin

Can be written:

fillUps[i].canTravel( dMin )

Similarly:

fillUps[i].getPrice() > priceMax

Can be written:

fillUps[i].priceExceeds( priceMax );

The important question remains unanswered: why does the code check against distance and price? If all you want to do is determine how much it will cost on a given tank of gas, then write:

float maxDistance = fillUps[i].calculateDistance();

If you want to know the price to fill the remainder of the tank, write:

float price = fillUps[i].calculateFillPrice();

Move the code that uses the variables into the class that has the variables.

You can also compare objects, rather than the internal variables. For example:

AnnualFuelUse fillUps[] // ...
AnnualFuelUse minPriceFillUp;

for (int i = 0; i < fillUps.length; i++) {
  if( fillUps[i].exceedsPrice( fillUps[i-1] ) ) {
    maxPriceFillUp = fillUps[i];
  }

  if( fillUps[i].exceedsDistance( fillUps[i-1] ) ) {
    maxDistanceFillUp = fillUps[i];
  }

  // ...
}

At the end of the loop you'll have objects that contain the desired information. With those objects you can then ask them to perform specific tasks. Then make the get accessor methods private.


C# WPF Simple Calculator I've implemented simple +-*/ calculator in WPF. I know that here are few implementations of the same thing, but each is different, with different advantages or disadvantages against this one. Can you suggest me, what could be improved in my code?

// calculator operations
public enum CalcOperator
{
    None,
    Plus,
    Minus,
    Times,
    Divide
}

public partial class Calculator : Window
{
    // decimal separator of current culture
    char decimSepar = Convert.ToChar
        (CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator);

    private double a = 0;    // the first number
    private double b = 0;    // second number

    // last selected operation
    private CalcOperator lastOper = CalcOperator.None;

    public Calculator()
    {
        InitializeComponent();
    }

    // handle single digit (button)
    public void HandleDigit(int i)
    {
        string str = txtDisp.Text;

        if (lastOper == CalcOperator.None && a != Convert.ToDouble(str) ||
            lastOper != CalcOperator.None && b != Convert.ToDouble(str) ||
            str == "0")
        {
            str = string.Empty;
        }

        str += i.ToString();

        if (lastOper == CalcOperator.None)
            a = Convert.ToDouble(str);
        else
            b = Convert.ToDouble(str);

        txtDisp.Text = str;
    }

    // handle calculator operation (button)
    public void HandleOperator(CalcOperator oper)
    {
        txtDisp.Text = b.ToString();
        lastOper = oper;
    }

    // handle decimal separator selection (button)
    public void HandleDecimal()
    {
        if (!txtDisp.Text.Contains(decimSepar))
            txtDisp.Text += decimSepar;
    }

    // compute the result
    public void Compute()
    {
        double result = 0.0;

        switch (lastOper)
        {
            case CalcOperator.Plus:
                result = a + b;
                break;
            case CalcOperator.Minus:
                result = a - b;
                break;
            case CalcOperator.Times:
                result = a * b;
                break;
            case CalcOperator.Divide:
                if (b == 0.0)
                    result = 0.0;
                else
                    result = (double)a / b;
                break;
        }

        txtDisp.Text = result.ToString();
        lastOper = CalcOperator.None;
        a = 0;
        b = 0;
    }

    private void Window_Loaded(object sender, RoutedEventArgs e)
    {
        btnPlus.Tag = CalcOperator.Plus;
        btnMinus.Tag = CalcOperator.Minus;
        btnTimes.Tag = CalcOperator.Times;
        btnDivide.Tag = CalcOperator.Divide;
    }

    private void OnClickDigit(object sender, RoutedEventArgs e)
    {
        Button btn = sender as Button;
        HandleDigit(Convert.ToInt16(btn.Content.ToString()));
    }

    private void OnClickOperator(object sender, RoutedEventArgs e)
    {
        Button btn = sender as Button;
        HandleOperator((CalcOperator)btn.Tag);
    }

    private void btnEqual_Click(object sender, RoutedEventArgs e)
    {
        Compute();
    }

    private void btnDot_Click(object sender, RoutedEventArgs e)
    {
        HandleDecimal();
    }

}

I'm glad to see someone did a WPF calculator, and I hope that I can add to what was already stated. Before I get started: "yes this is a MVVM rant". Now that I got that out of the way, I can understand though why a person would do a code behind approach. One is that it is what a ton of people are used too (I was one of them coming from Java, then going to winforms and eventually landing on WPF) Two is that at first glance it looks to be more compact and easier to understand. So why should a person do it? Well for one is that you'll be less tempted to put FrameworkElement properties in your code. string str = txtDisp.Text;, txtDisp.Text = str;, txtDisp.Text = b.ToString(); ...etc

Why is that helpful? well if you were to write tests for your code to make sure your logic is sound you don't want to setup a window in your favorite test framework. (trust me..I tried) However if you setup your ViewModel using normal primitive properties and bind your Window to those properties you'll find it much easier to test. I can imagine my ViewModel looking sorta like this

public class CalculatorViewModel : INotifyPropertyChanged
{
    public string Display {get; private set;}

    public ICommand NumericButtonPressedCommand {get; private set;}

    //... other things to set this all up

    //called from the UI and state contains a string of 0-9
    private void NumericButtonPressed(object state)
    {
        //convert state to int, update display
    }
}

TIP you can find much more information about MVVM and how to properly bind and setup commands all over the internet.

The viewmodel above can easily be tested with something like this:

public class CalculatorViewModelTest
{
    [Test]
    public void WhenNumericButtonIsPressedDigitIsShownOnDisplay()
    {
        var viewModel = new CalculatorViewModel();

        NumericButtonPressedCommand.Execute("4");

        Assert.That(viewModel.Display, Is.EqualTo("4"));

        NumericButtonPressedCommand.Execute("4");
        NumericButtonPressedCommand.Execute("2");
        NumericButtonPressedCommand.Execute("4");

        Assert.That(viewModel.Display, Is.EqualTo("4,242"));
    }
}

This is another reason why it's good to separate your logic from your UI. It facilitates easier testing.

So enough of a rant about that. If you decide to give it a go you'll read that over and over and over again. Your code uses the Tag object property of your buttons which IMO is much less clear than just setting up 4 (or more) operator ClickListeners. If you were to revisit this code in a few days or weeks and say your plus button was subtracting. You'd first look at Compute and see that it looks for a CalcOperator being set to lastOper. You look to see where it is set which is by HandleOperator, that is called from OnClickOperator and uses a Tag. Now you are searching for where Tag is set and you finally see that you accidentally set the Plus button's Tag to Subtract. copy paste error. Had you had individual Operator ClickListeners you would probably have gone to the OnClickAddition and see that you accidently set lastOper to Subtract. (In reality you didn't do that, but you can see how tracking the bug is easier when some things are spelled out. Not always, but sometimes)


Generate all permutations of a list in Python This is my solution to the "Permutations" problem from Leetcode:

  Given a collection of distinct numbers, return all possible permutations.

I know this is a common routine that can be done much faster using itertools.permutations but I want to write it to learn the algorithm itself.

I would like any general feedback on the implementation of the recursive algorithm. But I am also curious whether I should use an instance helper method as it is now, or make the helper method a class or static method. If someone can show me how to re-write the helper method as a class or static method that would be appreciated.

class Solution(object):
    def permute(self, nums):
        res = []
        self._permuteHelper( nums, 0, res)
        return res

    def _permuteHelper(self, nums, start, results):  #helper method
            if start >= len(nums):
                results.append(nums[:])
            else:
                for i in range(start, len(nums)):
                    nums[i], nums[start] = nums[start], nums[i]
                    self._permuteHelper(nums, start +1, results)
                    nums[start], nums[i] = nums[i], nums[start]

s = Solution()
nums = [1, 2, 3]
print s.permute(nums)

There are a lot of suggestions in the comments and answers about using library functions. For programming challenge, I am not interested in copying and pasting library functions. Yes, this is re-inventing the wheel if you want to call it that!

Instead of calling a recursive function you could do it in place in an iterative way.

Like this:

l = [1, 2, 3, 4, 5]

def permute(l):
    n = len(l)
    result = []
    c = n * [0]

    result.append(l)

    i = 0;
    while i < n:
        if c[i] < i:
            if i % 2 == 0:
                tmp = l[0]
                l[0] = l[i]
                l[i] = tmp

            else:

                tmp = l[c[i]]
                l[c[i]] = l[i]
                l[i] = tmp

            result.append(l)
            c[i] += 1
            i = 0
        else:
            c[i] = 0
            i += 1

    return result

print(permute(l))

This is not my algorithm, it is called the Heap algorithm


Python Calculator with tkinter I've written a simple unconventional calculator application using Python and tkinter. It's unconventional because you use the "modes" (+ - * /) to add to the current sum, instead of entering in a statement and doing that calculation. Because of this, 0 is not included. The main part of my program I would appreciate being reviewed:

Creating The Number Buttons

I absolutely hate large blocks of code that are essentially the same except for one or two things. In this case, it's creating the buttons. I am certain it is possible doing it with a loop, but everything I've attempted has resulted in failure. Everything works as intended, but I'd really appreciate some suggestions pertaining to the number buttons creation. As always, other feedback is welcome and greatly appreciated.

from tkinter import Tk, Label, Button

class Calculator:

    def __init__(self, master: Tk):
        # Initial Setup #
        self.master = master
        self.master.title("Calculator")
        self.master.maxsize(500, 700)

        self.sum = 0
        self.mode = "+"

        # Number Buttons
        self.number_buttons = [
            Button(master, text="1", command=lambda: self.do_math(1)),
            Button(master, text="2", command=lambda: self.do_math(2)),
            Button(master, text="3", command=lambda: self.do_math(3)),
            Button(master, text="4", command=lambda: self.do_math(4)),
            Button(master, text="5", command=lambda: self.do_math(5)),
            Button(master, text="6", command=lambda: self.do_math(6)),
            Button(master, text="7", command=lambda: self.do_math(7)),
            Button(master, text="8", command=lambda: self.do_math(8)),
            Button(master, text="9", command=lambda: self.do_math(9))
        ]

        # Mode Button and Label
        self.mode_label = Label(master, text=f"Mode: {self.mode}")
        self.mode_button = Button(master, text="Change Mode", command=self.change_mode)

        # Sum Label Setup #
        self.sum_label = Label(master, text=str(self.sum))
        self.sum_label.grid(row=1, column=2)

        # Reset Button Setup #
        self.reset_button = Button(master, text="Reset", command=self.reset_sum)

        # Calculate/Set Layout for Buttons #
        r, c = 2, 1
        for number_button in self.number_buttons:
            number_button.configure(width=10, height=10)
            number_button.grid(row=r, column=c)
            if c % 3 == 0:
                r += 1
                c = 0
            c += 1

        # Add Mode Label/Button #
        self.mode_label.grid(row=5, column=2)
        self.mode_button.grid(row=6, column=2)

        # Add Reset Button #
        self.reset_button.grid(row=7, column=2)

    def do_math(self, num: int) -> None:
        """
        Checks the current mode and modifies the sum

        :param num -> int: Button clicked by the user

        :return: None

        """
        modes = {
            "+": self.sum + num,
            "-": self.sum - num,
            "*": self.sum * num,
            "/": self.sum / num
        }
        self.sum = modes[self.mode]
        self.sum_label.configure(text=str(self.sum))

    def change_mode(self) -> None:
        """
        Changes the mode in (+, -, *, /) rotation

        :return: None

        """
        modes = {
            "+": "-",
            "-": "*",
            "*": "/",
            "/": "+"
        }
        self.mode = modes[self.mode]
        self.mode_label.configure(text=f"Mode: {self.mode}")

    def reset_sum(self) -> None:
        """
        Resets the sum

        :return: None

        """
        self.sum = 0
        self.sum_label.configure(text=str(self.sum))

if __name__ == "__main__":
    root = Tk()
    gui = Calculator(root)
    root.resizable(width=False, height=False)
    root.mainloop()

To reduce duplication, look at what's identical in each line, and what's different. For the different parts, make them a part of a loop, or the arguments to a function. For the identical parts, make them the body of the function/loop. In this case, look at the lines:

Button(master, text="1", command=lambda: self.do_math(1)),
Button(master, text="2", command=lambda: self.do_math(2)),
Button(master, text="3", command=lambda: self.do_math(3)),

The only thing that differs are the text parameter values, and the argument to do_math. Conveniently, they're the same, so this can be a simple loop over a range of numbers. I'm going to use a list comprehension here:

self.number_buttons = [Button(master, text=str(n), command=lambda n: self.do_math(n))
                       for n in range(1, 10)]

And in most cases, that would be fine. Unfortunately though, you're needing to put n inside of a lambda, and that can cause some surprising problems. To fix it, I'm going to use the lambda n=n: . . . fix. If I didn't make this change, the text would be set fine, but each button would end up being passed 19 instead of the correct number.

self.number_buttons = [Button(master, text=str(n), command=lambda n=n: self.do_math(n))
                       for n in range(1, 10)]

modes also has some duplication that can be improved.

modes = {
    "+": self.sum + num,
    "-": self.sum - num,
    "*": self.sum * num,
    "/": self.sum / num
}

self.sum = modes[self.mode]

You're repeating self.sum and num over and over. You can just deal with the data later by mapping to functions instead of the sum. The operator module has functions that correspond to the common operators like + to make this easy:

import operator as op

mode_ops = {
    "+": op.add,
    "-": op.sub,
    "*": op.mul,
    "/": op.truediv
}

f = mode_ops[self.mode]
self.sum = f(self.sum, num)  # Now we only need to specify the data once

This also saves you from computing every possible answer ahead of time. If you ever added an expensive operator, this could save some overhead (although it would be unlikely that that would ever be a major problem).


Running a function every second Can someone review my timer class that runs some code every second, please? The void it's calling (runCode) is just a void with some code in it.

using Reality.Game.Sessions;
using System;
using System.Threading;

namespace Reality.Game.Misc
{
    public static class UserTimer
    {
        public static void Initialize()
        {
                /* Lets rev this bad boy up... */
            Timer timer = new Timer(callback, "Some state", TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));
        }

        private static void callback(object state)
        {
            foreach (Session s in SessionManager.Sessions.Values)
            {
                if (s != null && s.Authenticated && !s.Stopped)
                {
                    workTimer(s);
                }
            }
        }

        private static void workTimer(Session s)
        {
            JobCacheWorker.runCode(s);
        }
    }
}

Timers are pretty simple to use.  There are only a few things to worry about and they are called out in the documentation.  From that page:

  "As long as you are using a Timer, you must keep a reference to it. As
  with any managed object, a Timer is subject to garbage collection when
  there are no references to it. The fact that a Timer is still active
  does not prevent it from being collected."

In your code, the reference to the Timer isn't stored anywhere.  This means it's eligible for garbage collection, and when that happens it will stop firing events.  So you need to store a reference somewhere, in your code it looks like a static field on the UserTimer class would make sense.

The other thing is if it's possible for the callback to run longer than the interval, you might want to detect this case and have the second callback not do any work.  If this is a possibility I'll have a line of code like:

if (!Monitor.TryEnter(timerLock))
    return;

Of course you need to release the monitor in a finally block in your callback.


Scala: Cumulative String Tokenisation I'm trying to split an incoming stream of strings into cumulative tokens per line item using a function below,

def cumulativeTokenise(string: String): Array[String] = {
  val array = string.split(" +")
  var result: Array[String] = Array()
  array.map { i => (
                    result = result :+ (
                                       if (result.lastOption == None) i
                                       else result.lastOption.getOrElse("")+ " " + i
                                       )
                   )
             }
  result
}

Ex: output of cumulativeTokenise("TEST VALUE DESCRIPTION . AS") would be => Array(TEST, TEST VALUE, TEST VALUE DESCRIPTION, TEST VALUE DESCRIPTION ., TEST VALUE DESCRIPTION . AS)

Trying to figure out if there's another efficient in-built method in Scala or better ways of doing it with FP, without any mutable array. Any help is much appreciated.

You can get the same results a little more directly.

def cumulativeTokenise(string: String): Array[String] =
  string.split("\\s+")
        .inits
        .map(_.mkString(" "))
        .toArray
        .reverse
        .tail

Or a, perhaps simpler, two step procedure.

def cumulativeTokenise(string: String): Array[String] = {
  val tokens = string.split("\\s+")
  Array.tabulate(tokens.length)(n => tokens.take(n+1).mkString(" "))
}

One problem I see here is that you rely on whitespace to separate tokens. That might not always be the case.

def cumulativeTokenise(string: String): Array[String] =
  string.split("((?=\\W)|(?<=\\W))")
        .filter(_.trim.nonEmpty)
        .inits
        .map(_.mkString(" "))
        .toArray
        .reverse
        .tail

cumulativeTokenise("here@there")
//res0: Array[String] = Array(here, here @, here @ there)

Probably not the best solution to the problem, but it's something to think about.


Representing a property with a non-nullable getter & a nullable setter I have a class as follows, where I want the user of the class to know that setting the Slug property to null is supported, and its getter will still return a valid value:
public class Post
{
    public string Title { get; }
    public Post(string title)
    {
        Title = title;
    }

    private string? slug;
    public string Slug
    {
        get
        {
            if(slug is null)
            {
                slug = Title.Slugify();
            }
            return slug;
        }
        set => slug = value;
    }
}

There are two possible cases here:
CASE 1: No slug is set on/after initialization OR the slug is initialized with a non-null string.
var post = new Post("foo bar");
var slug = post.Slug;               // foo-bar

var post2 = new Post("foo bar 2") { Slug = "foo-bar-2" };
var slug2 = post.Slug;              // foo-bar-2

CASE 2: The slug set on is initialization with a nullable string.
string? maybeNull = viewModel.Slug; // may or may not be null
var post = new Post("foo bar") { Slug = maybeNull! };
var slug = post.Slug;               // returns an expected value

Is there a better way to do this, than using the null forgiving operator? The above code looks worse if I had to set it to null:
var post = new Post("foo bar") { Slug = null! };
var slug = post.Slug;               // returns an expected value

The reason I dont want to use the null forgiving operator is because it requires the user of the Post class to know that a null value can be safely passed into its Slug property -- something I wish to avoid.
If it's not possible to have different nullability for getters and setters (appears to me like it's not), what is the best way to communicate my intent (users can safely set Slug to null + users will always get a valid slug, irrespective of what was set).
For some more context:

the Post class is instantiated with a nullable value only while creating a Post. Every other time, it's going to be pulled from a database, and so will always be instantiated with a valid slug.
the Post class is not a ORM model.

I'm thinking you should probably change your Post class as follows:
public class Post
{
    public string Title { get; }
    public string Slug { get; set; }
    public Post(string title)
    {
        Title = title;
        Slug = title.Slugify();
    }
}

Now you have a slugified title by default. However, if a user of the Post class wants to provide an alternative slug, they can still do that:
var post = new Post("foo bar") { Slug = altSlug };

The explanation for the user should be that they can either provide a slug explicitly, or have one auto-created by omitting it from the initializer. The only thing you cannot do with this code is set Slug to null explicitly. But I think it would not be good design to allow that.
On a side note, you might even want to make the Slug property getter-only, creating an immutable class, and providing a second constructor that takes the alternative slug. This makes reasoning easier:
public class Post
{
    public string Title { get; }
    public string Slug { get; }
    public Post(string title) : Post(title, title.Slugify()) {}
    public Post(string title, string slug)
    {
        Title = title;
        Slug = slug;
    }
}


Twitter Bootstrap CMS Ok, so I started to build my own CMS. You can see Aleksandar Golubovic's Blog,and I need your help. Please, look at my code, and tell me is that safe? You can download current version from Twitter Bootstrap CMS. If you want to install it, just read instructions.

So, please, tell me if I get some errors, or some of your opinions, instructions... I really need help.

EDIT:

I have many functions like:

function DeletePost()
{
if (isset($_GET['action']))
{
    if ($_GET['action']==='delete-post' && !isset($_GET['id']))
    {
        $qry=mysql_query("SELECT * FROM posts ORDER BY id DESC");
        while ($arr=mysql_fetch_array($qry))
        {
        echo '

'.$arr['post_title'].'
 Delete this post

'.$arr['post_content'].'

';
        }
    }
    if ($_GET['action']==='delete-post' && isset($_GET['id']))
    {

            $del=$_GET['id'];
            $qry=mysql_query("DELETE FROM posts WHERE id='$del'") or die ("ERROR!!!");
            echo "This post has been successfull deleted.";

    }
}
}

And this function is included in some page. So my question is: Is this method safe?
Note: All functions is separeted pages using .htaccess

Well looking at this as a code review of the code you posted i can recommend a couple of things;

Security

The major flaw at the moment is the possibility for SQL injection. You put the $_GET values directly into the query without validation or escaping.

PHP.net has a page on SQL injection:  http://php.net/manual/en/security.database.sql-injection.php

Deprecated code

It is strongly recommend not to use the mysq_* functions. !!!

These functions are deprecated and will recieve no more support and will be removed in an upcoming version of PHP. See http://php.net/manual/en/function.mysql-connect.php and look at the big red box. This is potentially a huge security hazzard in the future as well.

User Content / Access levels

Currently you are not demonstrating any check on access level. If i am to guess your actions i could delete the entire content of your website without logging in!

Database connection

You seem to open the sql connection somewhere else. Please verify for yourself that you also close the connection when request ends, avoiding lingering MySQL connections.

Misc

Also, look into the SOLID principles and PSR-2 codestyle, your current code does not match it. These are best practices and provide a good base strategy for your coding. Also, making it more likely the open source community would accept/adopt your code.

Good luck in further development of your CMS!


How can I reverse the first K elements of an array? I have an array and I need to make one function which has two parameters, array and K. I need to return a new array with only K elements reversed, the rest becoming the same as in array. I am able to get correct results but I think its performance is too slow. Is there any better solution to get the same results?

var arry=[2,3,1,5,7,9];
var k=3
function reverseOnlyK_elment(arr,k){
    var copyarray=[];
    var j=0;
    for(var i=k-1;i>=0;i--){
        copyarray[j++]=arr[i]
    }
    for(var i=k;i<=arr.length-1;i++){
        copyarray[j++]=arr[i]
    }
    console.log(copyarray)
}
reverseOnlyK_elment(arry,k)

Fiddle

Expected output:

[1, 3, 2, 5, 7, 9]

Performance

You know exactly how long the resulting array should be, so don't make the interpreter guess.  Any time the interpreter underestimates the length of an array and has to expand it, there's a significant performance penalty, as it reallocates a larger chunk of contiguous memory and transfers the already copied elements there.

var copyarray = new Array(arr.length);

That said, do you really need to copy the array?  If not, then you should just swap the first k elements in place for best performance.

As for the looping, it doesn't much matter whether you write it as one loop or two loops.  The running time will be dominated by the memory accesses: n reads and n writes.

Code hygiene

Did you really drop a vowel from the function name to save one byte?  If so, you are repeating Ken Thompson's greatest regret.

Although JavaScript allows you to omit semicolons at the end of each statement, it's considered bad practice to do so in all but the most trivial cases, such as Hello.

It's odd to have a function that just logs its results.  Either return copyarray (and let the caller call console.log()), or reverse the entries in place.


Project Euler # 56 Powerful digit sum in Python A googol (\$10^{100}\$) is a massive number: one followed by one-hundred zeros; \$100^{100}\$ is almost unimaginably
large: one followed by two-hundred zeros. Despite their size, the sum of the digits in each number is only 1.

Considering natural numbers of the form, \$a^b\$, where \$a, b < 100\$, what is the maximum digital sum?

def get_powers(n):
    """Assumes n a range, generates all numbers to the power of all numbers within the range."""
    for num in range(n):
        for power in range(n):
            yield num ** power

def get_digit_sum(n):
    """Assumes n a number, returns sum of digits."""
    return sum(int(digit) for digit in str(n))

def get_maximum(n):
    """Assumes n a range, gets all a ** b within range, returns max sum digits."""
    return max((get_digit_sum(num) for num in get_powers(n)))

if __name__ == '__main__':
    print(get_maximum(100))

Very nice code! Here are a few small comments, most of which are not something you have to change but rather food for thoughts as you learn more.

Although this is a matter of personal style, I would turn around your docstrings so what is returned comes first, if possible.

I.e. I would write

"""Generates all numbers of the form `a ** b` for `a, b < n`."""
"""Return the sum of digits of the number `n`."""
"""Return the max sum of the digits of all numbers of the form `a ** b` for `a, b < n`."""

If a generator expression is the only argument to a function, one pair of () can be omitted:

return max(get_digit_sum(num) for num in get_powers(n))

Although sometimes discouraged, if you need those last 10% of speed, this is slightly faster since int is a built-in function implemented in C:

def get_digit_sum(n):
    return sum(map(int, str(n)))

Nested for loops can be replaced with itertools.product, although here it is arguably harder to read instead of easier. For high levels of nestedness it can help increase readability, though.

from itertools import product

def get_powers(n):
    for num, power in product(range(n), range(n)):
        yield num ** power


Safe init using failable guarded initializer I am wondering if that is a safe and proper init, with one failable init guarded and an init.

Are the force-unwrap correct in the self.init as I know they are those types ?

struct Item: Equatable {

            let id: NSNumber
            let displayName: String
            let amount: NSNumber

            init(id:NSNumber, displayName:String, amount:NSNumber) {
                self.id = id
                self.displayName = displayName
                self.amount = amount
            }

            init?(dictionnary:NSDictionary) {

                guard let id = dictionnary["id"] , id is NSNumber else  {
                    return nil
                }

                guard let displayName = dictionnary["display_name"] , displayName is String else {
                    return nil
                }

                guard let amount = dictionnary["amount"] , amount is NSNumber else  {
                    return nil
                }

                    self.init(id:id as! NSNumber, displayName: displayName as! String, amount: amount as! NSNumber)
                }

}

func ==(lhs: Item, rhs: Item) -> Bool {
        return lhs.id == rhs.id
}

The separation into a non-failable and a failable init method is
fine.

The forced casts with as! are safe in your code, because it is verified before
that the three variables id, displayName, number have the
respective types Number, String and NSNumber.

However:

The code is error-prone: Changes to the types must be done at two places.
It is not immediately obvious that it works correctly.

Forced casts should (and can!) be avoided in most cases, in the
same way as forced unwrappings. The program would be terminated
immediately if the cast fails.

In your code that is quite easy,
by replacing the type checks is T by  conditional casts as? T.
The latter return a value of the type T and that makes the forced
cast unnecessary. In addition, it simplifies and shortens the code
of  the failable init method:

init?(dictionary: NSDictionary) {

    guard let id = dictionary["id"] as? NSNumber else {
        return nil
    }
    guard let displayName = dictionary["display_name"] as? String else {
        return nil
    }
    guard let amount = dictionary["amount"] as? NSNumber else {
        return nil
    }
    self.init(id: id, displayName: displayName, amount: amount)
}

As a further simplification, combine the optional bindings into
a single guard statement (attribution goes to @Knight0fDragon who
reminded my in a comment):

init?(dictionary: NSDictionary) {
    guard
        let id = dictionary["id"] as? NSNumber,
        let displayName = dictionary["display_name"] as? String,
        let amount = dictionary["amount"] as? NSNumber else {
            return nil
    }
    self.init(id: id, displayName: displayName, amount: amount)
}

Some other points:

Your spacing is not consistent.
The indenting is totally wrong.
The correct spelling is dictionary.


LinkedList Stack Implementation I'm new to the programming and I want to ask a question about my linkedlist stack implementation if it's correct and if it meets the requirements of the linkedlist stack implementation. If there is any suggestion I'm glad to hear...

public class DynamicStack {
  private class Node {
     private Object item;
     private Node next;

     Node(Object item){
      this.item = item;
      this.next = null;
     }

     Node(Object item, Node nextNode){
      this.item = item;
      this.next  = nextNode;
     }
  }

 private int count;
 private Node top;

 public DynamicStack() {
   this.top = null;
   this.count = 0;
 }

 public void push(Object item) {
   if(top == null) {
      top = new Node(item);
   }else {
      Node newNode = new Node(item,top);
      top = newNode;
   }
   count++;
 }

 public Object peek() {
   if(top == null) {
      throw new NoSuchElementException("Underflow Exception");
   }else {
      return top.item;
   }
 }
 public Object pop() {
   Node currentNode = top;
   if(top == null) {
      throw new NoSuchElementException("Underflow Exception");
   }else {
      Node nextNode = top.next;
      top = null;
      top = nextNode;
   }
   count--;
   return currentNode.item;
 }

This looks to me like it will work as intended. I have a few suggestions for improvement though:

You keep the count variable up to date but never use it's value for anything. You can either remove it or provide a size() method.
You might change your push() method to

public void push(Object item){
   top = new Node(item, top);
   count++;
}

If top is null, you set node.next to null in the Node constructor anyways.
 - If you do this, you can subsequently delete the Node(Item) constructor

I personally would make the item and next member variables in Node final to prevent accidental mutations
(Instead of accepting Object as your items you might want to introduce Generics into your class. But this is an advanced topic and is maybe too hard for you as a beginner)


Project Euler #5 (LCM 1 - 20) Follow up I'm actually spending time on it, unlike the first time and going through, understanding and implementing the several suggestions I've landed on two different approaches.

Approach 1: Employing Java 8 Lambdas and stream functions made to be the equivalent of David Zheng's answer in Python.

import java.util.ArrayList;

public class Euler5 {
    public static void main(String[] args) {
        long startTime = System.nanoTime();
        final long MAX_NUM = 20L;

        ArrayList<Long> numbers = new ArrayList<Long>() {
            { for (long i = 1L; i <= MAX_NUM; i++) { add(i); } }
        };

        long result = numbers
            .stream().reduce(Euler5::lcm).get();

        System.out.print("Result: " + result +
                ".\nTime used for calculation in nanoseconds: " +
                (System.nanoTime() - startTime) + ".");
    }

    /*Greatest Common Divisor
    Euclidean algorithm: http://en.wikipedia.org/wiki/Euclidean_algorithm */
    public static long gcd(long a, long b) {
        return b == 0 ? a : gcd(b, a % b);
    }
    // Least Common Multiple
    public static long lcm(long a, long b) {
        return (a * b) / gcd(a, b);
    }
}

Sample output:

  Result: 232792560.
  Time used for calculation in nanoseconds: 111370744.

Approach 2: More similar to my original method, using several suggestions, including one in the OP's comment section which I'm unsure about.

public class Euler5 {
        public static void main(String[] args) {
            long startTime = System.nanoTime();
            // Multiplying by primes for a starting point * 2 for evenness
            int givenTimesPrimes = 2520 * 2 * 11 * 13 * 17 * 19,
                result = givenTimesPrimes;

            while (!isSmallestMultiple(result)) {
                result += givenTimesPrimes;
            }

            System.out.print("Result: " + result +
                ".\nTime used for calculation in nanoseconds: " +
                (System.nanoTime() - startTime) + ".");
        }

        public static boolean isSmallestMultiple(int n) {
            for (int i = 11; i <= 20; i++) {
                if (n % i != 0) {
                    return false;
                }
            }
            return true;
        }
    }

Example output:

  Result: 232792560.
  Time used for calculation in nanoseconds: 141635.

Approach 2 is clearly faster, but I'm wondering if givenPrimes qualify as "magic numbers?" Or is the explanation enough (feels tenuous/not 100% certain it would qualify as an effective strategy in a different context).
The first method seems the more understandable/applicable. Am I wrong? Which would you fellow CR folks prefer and why?

This Project Euler question is an excellent one to think about computational efficiency of algorithms.  It's a horrible place to make decisions based on benchmarks, though.

Sure, your second method is faster.  But you're basically cheating with the magic number 2520.  It's not a particularly insightful implementation for mathematical learning, either, as it's guessing rather than computing.

To illustrate what I mean, let's consider two more implementations.

"Fast and Crappy"

This algorithm computes the LCM of \$\mathrm{numbers}_i\$ based on the principle that

$$ \mathrm{LCM}(a, b) = \prod p_j^{\max(e_{j,a}, e_{j,b})} $$

when numbers are factorized as \$a = \prod p_j^{e_{j,a}}\$ and \$b = \prod p_j^{e_{j,b}}\$.

  Why do I call it "crappy"? Because it is using the same strategy as this nice-looking Ruby code, but it would take a lot of work to make it look as good in Java.

   require 'prime'

lcm_factors = Hash.new(0)       # Default entry is 0
(1..20).each do |n|
  Prime.prime_division(n).each do |factor|
    prime, exponent = *factor
    lcm_factors[prime] = [lcm_factors[prime], exponent].max
  end
end
puts lcm_factors.map { |prime, max_exponent| prime ** max_exponent }
                .inject(:*)

public class FastCrappyEuler5 {
    private static final int[] PSEUDOPRIMES_BELOW_20 = new int[] {
        2, 3, 5, 7, 11, 13, 17, 19
        // 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19
    };

    public static long lcmOfNumbersUpTo(int rangeMax) {
        if (rangeMax > 20) {
            throw new IllegalArgumentException("TODO: FIXME");
        }

        int[] numbers = new int[rangeMax];
        for (int n = 1; n <= rangeMax; n++) {
            numbers[n - 1] = n;
        }

        long lcm = 1;
        for (int p : PSEUDOPRIMES_BELOW_20) {
            int maxExponent = 0;
            for (int i = 0; i < numbers.length; i++) {
                int exponent;
                for (exponent = 0; numbers[i] % p == 0; exponent++) {
                    numbers[i] /= p;
                }
                maxExponent = Math.max(maxExponent, exponent);
            }

            while (maxExponent --> 0) {
                lcm *= p;
            }
        }

        return lcm;
    }

    public static void main(String[] args) {
        long start = System.nanoTime();
        System.out.println(lcmOfNumbersUpTo(20));
        System.err.println(System.nanoTime() - start);
    }
}

This takes about 6 times as long as your fast implementation.  (For me, it's on the order of ~105 nanoseconds.)  I've taken a shortcut of hard-coding the primes below 20 — a "fair" move, I think, as most people know them by heart.

If you replace the pseudoprimes list with a list of all integers from 2 to 20, you get the same answer, with no significant difference in running time.

Using streams

This implements exactly the same algorithm as FastCrappyEuler5, but does so using Java 8 streams to be more elegant.

import java.util.stream.IntStream;
import java.util.stream.LongStream;

public class StreamsEuler5 {
    private static LongStream pseudoPrimesBelow(int max) {
        // We should be more selective, but as we saw above,
        // it shouldn't make much difference.
        return LongStream.range(2, max);
    }

    private static long pow(long base, /* unsigned */ int exponent) {
        long power = 1;
        while (exponent --> 0) {
            power *= base;
        }
        return power;
    }

    public static long lcmOfNumbersUpTo(int rangeMax) {
        final int[] numbers = IntStream.rangeClosed(1, rangeMax).toArray();

        // Warning: computation is not parallelizable
        return pseudoPrimesBelow(rangeMax).map((p) -> {
            int maxExponent = 0;
            for (int i = 0; i < numbers.length; i++) {
                int exponent;
                for (exponent = 0; numbers[i] % p == 0; exponent++) {
                    numbers[i] /= p;
                }
                maxExponent = Math.max(maxExponent, exponent);
            }
            return pow(p, maxExponent);
        }).reduce(1, (a, b) -> a * b);
    }

    public static void main(String[] args) {
        long start = System.nanoTime();
        System.out.println(lcmOfNumbersUpTo(20));
        System.err.println(System.nanoTime() - start);
    }
}

Unfortunately, it's two orders of magnitude slower! (For me, it's on the order of 5 ⨉ 107 nanoseconds.)

Conclusion

$$\begin{array}{l|r}
\textrm{Implementation}                & \textrm{Approx. running time} \\
\hline
\textrm{Legato's streams}              & 1 \times 10^8 \mathrm{ns} \\
\textrm{Legato's cheat-and-guess}      & 5 \times 10^4 \mathrm{ns} \\
\textrm{200_success's fast-and-crappy} & 1 \times 10^5 \mathrm{ns} \\
\textrm{200_success's streams}         & 5 \times 10^7 \mathrm{ns} \\
\end{array}$$

Are the streams-based implementations worse because they are slower?  I don't think so.  The overhead of the streams seems to dwarf everything else.  But I don't mind waiting a few million nanoseconds if it means that I like the code better.  My streams-based solution is the same algorithm as "Fast and Crappy", so the complexity must be the same.  Presumably, the startup costs would be inconsequential for larger problems.

Basically, do the "right thing" for readability, maintainability, and educational value.  Time measurements mean nothing when you haven't taken scalability into account.  Worry about speed when it's noticeably slow.


Inverting index structure in C++ I am trying to create an invert index structure in C++. An invert index, in this case, refers to image retrieval system, where each invert index refers to a numbers of "Patch ID". (simply called "ID" in the code) These IDs are basically for describing many kind of pixel gradient patches from all images in the database.

The invert index is a map container and will be very sparse and each index of the invert index will contain a sub-map. A sub-map will have an "Image ID" and frequency counting integer. For example, if we have 2 images with "Image ID" 10 and 20, and both of these images have "Patch ID" 999, appear once in "Image ID" 10 and appear twice in "Image ID" 20, we will have the following:

  invertID[999] == sub-map with <10,1> and <20,2>

I've written the code below for this, but somehow I am not quite satisfied.

One instance, I have to put back the sub-map into the invert ID's index as you can see below.

Another thing is I am not quite sure whether I am using the correct type of container for the correct job or not. I have about 5000 images (so that's 5000 "Image ID") and I see that "Patch ID" could vary from 1 to numbers as high as 40 billion.

In this case, performance-wise and design-wise, should I be using map or should I go for some other container? Suggestions are welcome.

Note: this will be run in a server that doesn't have a C++11 compiler.

unsigned long long int ID, index = 0;
map<unsigned long long int, map<int, int>> invertID;

...
...

//for the sake of convenient, two for loops below are shown as pseudocode.
for each image{ //loop with imageID, incrementing variable imageID in each iteration

//for each gradient patches in the image
for each gradient patch of this image{ //loop with index, incrementing variable index in each iteration.

    //processID encapsulates the procedure to generate an ID for each interesting point in the image. We basically read values in the respective file and do some calculation.
    ID = processID(index);

    //if this ID exists in InvertID,
    map<unsigned long long int, map<int,int>>::iterator mainMapIt = invertID.find(ID);
    if (mainMapIt != invertID.end()){

        //if this ImageID key exists in InvertID sub-map,
        map<int,int> M = mainMapIt->second;
        map<int,int>::iterator subMapIt = M.find(imageID);
        if (subMapIt != M.end()){
            //the index already exists, so increment the number (frequency) of this ImageID key
            ++M[imageID];
        }
        else{
            //the index does not exist, so add ImageID key with value 1 into the InvertID
            M[imageID] = 1;
        }

        //get this sub-Map back to the invert ID's index.
        invertID[ID] = M;
    }
    else{
        //the ID does not exist in the InvertId, yet, so
        //create the first empty map with the key as image ID with value 1 and put it into the invertID.
        map<int,int> M;
        M[imageID] = 1;
        invertID[ID] = M;
    }
}
}

You are taking a copy of the inner map, modifying it, then putting it back. That could well be inefficient. You could take the inner map by reference then modify it.

However you can actually make use of the fact that map operator[] inserts if it does not find the element. As that is the behaviour here you can do what you did in 2 lines:

map< int, int > & innerMap = mainMapIt[ID]; // will be created as empty if not found
++innerMap[ imageID ];

On a style issue, one does not commonly use an upper case letter as a variable. You also have ID as a variable but elsewhere your variables begin with a lower case letter. You should be consistent throughout in your style, and ideally adopt what is the most common style for C++ programmers (which is generally agreed to be beginning with a lower-case. Some prefer camel-case and some under-score delimited).

You could also decide instead of nested maps, to have a single map with a key consisting of  std::pair<unsigned long long, int >. This might be slightly more efficient as copying maps around could be expensive. It would mean however that you could not as easily search for all the elements from ID.


C++ template to randomly choose from N elements with uniform distribution There is a nice algorithm to randomly choose an element from a list in a single pass:

Pass through the list keeping the element chosen so far (X) and the number of elements processed (N). When processing element Y, let X:=Y with probability 1/N. When nothing is left, return X.

I implemented this using templates:

template <typename T>
const T& chooseImpl(T const& cur, int total) {
  return cur;
}

template <typename T, typename... Args>
const T& chooseImpl(T const& chosen, int total,  T const& next, Args... rest) {
  const T& nextChosen = (rand() % total == 0) ? next : chosen;
  return chooseImpl(nextChosen, total + 1, rest...);
}

template <typename T, typename... Args>
const T& choose(T const& first, Args... rest) {
  return chooseImpl(first, 2, rest...);
}

Example usage:

choose(1, 2, 3, 4, 5, 6); // returns a random number between 1 and 6.
choose(1); // always returns 1.

I only have a problem when choosing from C strings:
    choose("one", "two", "three");

  note:   template argument deduction/substitution failed:

  deduced conflicting types for parameter ‘const T’ (‘char [4]’ and ‘const char*’)

Here's a version of your code which works with C-Strings by using std::decay to turn any array arguments into pointers.

This also uses perfect forwarding in the recursion, to preserve argument types, just in case unconditional conversion to a const reference is inappropriate.

I'm making this Community Wiki, because it's more of an answer to your question about C-Strings, rather than a code review (ok, little review: rand() is usually so poorly implemented, you should use something from <random>, like std::mt19937, otherwise, thumbs up!).

#include <iostream>
#include <type_traits>
#include <utility>
#include <string>
#include <cstdlib>

template <typename T>
using decay_t = typename std::decay<T>::type;

template <typename T>
decay_t<T> chooseImpl(T&& cur, int total) {
  return cur;
}

template <typename T, typename U, typename... Args>
decay_t<T> chooseImpl(T&& chosen, int total,  U&& next, Args&&... rest) {
  decay_t<T> nextChosen = (std::rand() % total == 0) ? next : chosen;
  return chooseImpl(nextChosen, total + 1, std::forward<Args>(rest)...);
}

template <typename T, typename... Args>
decay_t<T> choose(T&& first, Args&&... rest) {
  return chooseImpl(std::forward<T>(first), 2, std::forward<Args>(rest)...);
}

int main() {
    std::srand(time(0));
    for(int i=0; i<4; ++i) {
        auto c = choose("one", "two", "three");
        std::cout << c << ' ';
    }
    std::cout << '\n';
    for(int i=0; i<4; ++i) {
        auto c = choose(256, 512, 1024, 13);
        std::cout << c << ' ';
    }
    std::cout << '\n';
    const std::string s[7] = {"Aa", "Bb", "Cc", "Dd", "Ee", "Ff", "Gg"};
    for(int i=0; i<4; ++i) {
        auto c = choose(s[0],s[1],s[2],s[3],s[4],s[5],s[6]);
        std::cout << c << ' ';
    }
    std::cout << '\n';
}

Here is an example of the output:

two three one two
13 1024 1024 256
Bb Ff Ee Ee


Trip Advisor - Train routes Gives advice to the user on which trains to take to reach a destination

 public class TripAdvisor {
        private Station source;
        private Station destination;
        private double cost;
        /**
         *
         * @param path the shortest path returned by the shortestPath method implementation
         */
        TripAdvisor(ArrayList<Station> path)
        {
            Lane color=null;
            ArrayList<Station> changeStation =new ArrayList<Station>();//used to identify when the user needs to change lines
            ArrayList<Object> trains=new ArrayList<>();//list of trains the user need to take to reach a station
            for(int i=0;i<(path.size()-1);i++)
            {

                Station current=path.get(i);
                Station next=path.get(i+1);
                for (Connection e : current.adjacencies)
                {
                    if(e.getTarget().equals(next))
                    {
                        Lane train=e.getLaneColor();
                        trains.add(train);
                        if(!e.getLaneColor().equals(color)&&(color!=null))
                        {
                            changeStation.add(current);
                        }
                    }
                    color=e.getLaneColor();
                }
            }

            System.out.println("trains"+ "$" +trains);
            if(trains.size()==1)
            {
                int q;
                System.out.println("Take a "+ trains.get(0) +"liner from" + path.get(0) +"to reach" + path.get( q=path.size()));
            }
            else
            {
                System.out.println("Take a "+ trains.get(0) +"liner from" + path.get(0)+ "Get down at" + changeStation.get(0));

            }
        }

    }
    /**
     *
     * @author PrasannaAarthiB
     * This has all the lines in the metro.They are represented by different colors
     */
    public enum Lane{
        RED,BLUE,GREEN,YELLOW,BLACK
    }

First of all, you have a possible bug in your code.

If trains is empty you will get a problem at the printing of the results.

Don't code that much inside the constructor. The constructor should be used for initializing only.
Be consitent in the way you create objects

ArrayList<Station> changeStation =new ArrayList<Station>();
ArrayList<Object> trains=new ArrayList<>();

If you need comments to describe a variable, you have named the variable poorly
Let your code breathe, use some spacing

for(int i=0;i<(path.size()-1);i++)

this would be easier to read like

for (int i = 0 ; i < path.size() - 1; i++)

use methods for calculations. You should add a method which calculates the shortest path and another one that would be responsible for the output of these calculated values.
code against interfaces instead of implementations so

ArrayList<Station> changeStation =new ArrayList<Station>();
ArrayList<Object> trains=new ArrayList<>();

should be

List<Station> changeStation =new ArrayList<>();
List<Lane> trains=new ArrayList<>();

Return from methods what the methodnames implies and name the "receiving" variable accordingly

Lane train=e.getLaneColor();

So the enum Lane should be become LaneColor and train should become e.g. currentLaneColor


Finding number of primes till n - Sieve of Eratosthenes I've written a ruby implementation for Sieve of Eratosthenes algorithm, however benchmarking it against the Ruby's Prime module I have still a far slower algorithm - and I am struggling to understand why.

require 'set'

def numbers_with_multiples_less_than n
  (2...(Math.sqrt(n)).ceil)
end

def primes_till n
  possible_primes = Set.new(2...n)

  (numbers_with_multiples_less_than n).each do |i|
    if possible_primes.include? i
      possible_primes = possible_primes - Set.new(multiples i**2,i,n)
    end
  end

  possible_primes
end

def count_prime_till n
  primes_till(n).count
end

def multiples start,step_size,max
  (start...max).step(step_size)
end

Running it against,

puts Benchmark.measure { Prime::EratosthenesGenerator.new.first(1000).count }
  0.000000   0.000000   0.000000 (  0.000437)

Compared to mine,

puts Benchmark.measure {count_prime_till 7920}
  0.020000   0.000000   0.020000 (  0.025582)

Please let me know how can I improve the running time, and as always, readability.

You speed problem was caused by using the wrong data structure.  Since the sieve holds all the numbers up to n an Array is more efficient than a Set.

As for style I removed your subroutines and added comments.  This will improve speed with little if any lose of readability.

def primes_till n
  possible_primes = Array.new(n+1)

  (2..n).each do |n|
    possible_primes[n] = n
  end

  # numbers with multiples less than n
  (2..(Math.sqrt(n)).ceil).each do |i|
    if possible_primes[i]
      # remove all multiples of the prime
      (i**2..n).step(i).each do |x|
        possible_primes[x] = nil
      end
    end
  end

  possible_primes.compact
end

require 'prime'
require 'benchmark'
s = 100000
x =  Prime::EratosthenesGenerator.new.first(s).last
raise unless Prime::EratosthenesGenerator.new.first(s).count == primes_till(x).to_a.count

puts Benchmark.measure { primes_till(x).count }
puts Benchmark.measure { Prime::EratosthenesGenerator.new.first(s).count }

This is slightly faster than the Prime::EratosthenesGenerator, but it does not knows that maximum size of the sieve and must do some extra work to handle that fact.


Trie-based spell checker This is a classic problem with a classic solution and I've seen it a number of times on this site, but I wanted to know what people thought of this C# implementation, as opposed to the numerous C implementations.

Below you'll find the source code, the output of the code and then a link to repl.it, so you can run the code in your browser.

Source code

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

class MainClass {
    // An array of valid words. Would normally contain way more words.
    private static string[] dictArray = new string[] {
        "trainee",
        "train"
    };

    public static void Main (string[] args) {
        Node dictTrie = CreateDictTrie(dictArray);
        Console.WriteLine(IsWord("Train", dictTrie)); // True
        Console.WriteLine(IsWord("Traine", dictTrie)); // False
        Console.WriteLine(IsWord("Trainee", dictTrie)); // True
    }

    // Create the trie to use for spell checking, based on an array of valid words.
    private static Node CreateDictTrie(string[] dictArray) {
        Node root = new Node();
        for (int i = 0; i < dictArray.Length; i++) {
            string word = dictArray[i];
            Node node = root;
            for (int j = 0; j < word.Length; j++) {
                char character = word[j];
                if (!node.Children.ContainsKey(character)) {
                    node.Children[character] = new Node();
                }
                node = node.Children[character];
            }
            node.IsWord = true;

        }
        return root;
    }

    // Check whether a string is a valid word.
    private static bool IsWord(string word, Node dictTrie) {
        word = word.ToLower();
        Node node = dictTrie;
        for (int i = 0; i < word.Length; i++) {
            char character = word[i];
            if (!node.Children.ContainsKey(character)) {
                return false;
            }
            node = node.Children[character];
        }
        return node.IsWord;
    }
}

// Class used for the trie structure.
public class Node {
    public bool IsWord;
    public Dictionary<char, Node> Children { get; set; }

    public Node() {
        Children = new Dictionary<char, Node>();
    }
}

Output

True
False
True

Repl.it link

https://repl.it/EOlt/14

I tried to make the code pretty readable, so if you wanted to really optimize the code, you could probably remove a few of the variable declarations and so on. But other than that, are there any overall performance problems with my approach? Anything else you think I could have done better? Feedback is much appreciated. Thanks!

Edit: I updated the code to use arrays and for loops instead of Lists and foreach loops, which should improve the performance a bit.

The trie creation could by very pretty if you made it an extension then can call it on the array:

var trie = new string[]
{
    "trainee",
    "train",
    "tree"
}
.ToTrie();

The method itself can be simplified. You can use foreach to get rid of the indexes (unless performance really, really, really matters).

Sometimes it's also prettier to use the TryGetValue + else where you can assign all values in a single line to avoid duplication like:

    currentNode.Children[character] = new Node();
}
currentNode = currentNode.Children[character];

To my taste it doesn't look nice. Instead I suggest this where I also use currentNode instead of just node which I find is easier to understand.

public static Node ToTrie(this string[] values)
{
    var root = new Node();
    foreach (var value in values)
    {
        var currentNode = root;
        foreach (var c in value)
        {
            var node = (Node)null;
            if (currentNode.TryGetValue(c.ToString(), out node))
            {
                currentNode = node;
            }
            else
            {
                currentNode = (currentNode[c.ToString()] = new Node());
            }
        }
        currentNode.IsWord = true;
    }
    return root;
}

The Node can actually be simplified too by deriving it from a dictionary. By adding the search method to it you can easily use it on any node. I named it Contains.

Another adjustment you can make is to use string instead of the char so you can use one of the construtor overloads of the dictionary and make it case insensitive. This way you won't need the ToLower.

public class Node : Dictionary<string, Node>
{
    public Node() : base(StringComparer.OrdinalIgnoreCase) {}

    public bool IsWord { get; set; }

    public bool Contains(string value)
    {
        var currentNode = this;
        foreach (var c in value)
        {
            var node = (Node)null;
            if (currentNode.TryGetValue(c.ToString(), out node))
            {
                currentNode = node;
            }
            else
            {
                return false;
            }
        }
        return currentNode.IsWord;
    }
}

Usage:

var result1 = trie.Contains("train"); // True
var result2 = trie.Contains("TraINE"); // False
var result3 = trie.Contains("TRAINEE"); // True


DNA base pair match counter So my code is done it outputs exactly what it needs to I'm just wondering if it is possible to make this code a lot more simple using objects. If so could someone tell me what I would need member-wise and I will try and implement it. When I tried it got way too complex and it ended up looking easier to just write functions and a whole ton of for loops with if statements.

The purpose of the code is to analyze 2 inputs (base and template) and then see if the template matches any chunks on the base. Also called Gattaca, but A goes to T, C goes to G, and vice versa.

Here is my code:

#include <iostream>
#include <string>
using namespace std;

bool AT(char a, char b)  {
    return ((a == 'A' && b == 'T') || (b == 'A' && a == 'T'));
}
bool CG(char a, char b)  {
    return ((a == 'C' && b == 'G') || (b == 'C' && a == 'G'));

}
bool match(char a, char b) {

    return (AT(a, b) || CG(a, b));
}

int main() {
int case_count = 0;
string base;
string temp;
int base_size;
int temp_size;
int count;
cin >> case_count;

for (int i = 0; i < case_count; ++i) {
    cout << "Case " << i << ":\n";
    bool check = false;
    cin >> base;
    cin >> temp;
    base_size = base.size();
    temp_size = temp.size();

    if (base_size < temp_size) {
        cout << "None";
        continue;
    }
    if (base_size == temp_size) {
        count = 0;
        for (int index = 0; index < base_size; ++index) {
            if (!(match(base[index], temp[index]))) {
                cout << "None\n";
                break;
            }
            else
                count++;

        }
        if (count == temp_size) {
            cout << "0\n";
        }
        continue;
    }
    for (int j = 0; j < base_size - (temp_size+1); ++j) {
        count = 0;
        for (int k = j; k < j + temp_size - 1; ++k) {
            if (!(match(base[k], temp[count]))) {
                break;
            }
            else
                count++;
        }
        if (count == temp_size - 1) {
            check = true;
            cout << j << " ";
        }

    }
    if (check == false) {
        cout << "None \n";
    }
    else if (check == true) {
        cout << "\n";

    }
}
return 0;
}

I'm new to C++ programming so the best I can think is using a class to make this look simple. I'm not looking for a full written program just an idea/pseudo code or just the member functions I could use to make this simple.

It is extremely difficult to understand what's going on in your code. Try to think of things the following perspective: the goal of programming is to tell other people what you want the computer to do. And what you want to do is match the template to the base as some location. So how can we make that clearer?

Rather than character-wise matching for the appropriate inversion, we can just invert the template and then we just have to match for equality. Suppose we have a function:

std::string invertDNA(std::string const& );

which flips A/T and C/G. Once we have that inversion, we're just searching for the inverted match_template in base:

std::size_t index = base_template.find(invertDNA(match_template));
if (index == std::string::npos) {
    std::cout << "None\n";
}
else {
    std::cout << index << '\n';
}

We've now reduced dozens of lines of loops and variables, which are error prone, into a one line standard algorithm which is very easy to both reason about and for other users to understand. I don't know anything about chemistry, but I get this!

For minor comments, avoid using namespace std; and keep your indentation consistent. You do not need variables for the string length, since you can always call .size(). Avoid comparing against true and false. That part can be if (!check) { ... } else { ... }


Sort characters in a Python string by frequency I wrote a solution to the leetcode problem Sort characters by frequency and the solution passes 34/35 test cases. On the last test case, there is a "time limit exceeded" error with an input string of 100000+ "a" and "b" characters. How can the following code be optimized to handle an input of that size?

 def frequencySort(s):
        """
        :type s: str
        :rtype: str
        """
        freq = {}
        for i in s:
            if i in freq:
                freq[i] +=1
            else:
                freq[i] = 1
        output = ""
        newDict =  sorted(freq.items(), key=lambda kv: kv[1],reverse = True)
        for k,v in newDict:
            for i in range(v):
                output += k
        return output

The “set or increment dictionary value” logic

freq = {}
for i in s:
    if i in freq:
        freq[i] +=1
    else:
        freq[i] = 1

can be simplified using a defaultdict:

from collections import defaultdict

# ...

freq = defaultdict(int)
for i in s:
    freq[i] += 1

But instead of creating, updating and sorting your own dictionary you can
use the built-in Counter class:

  A counter tool is provided to support convenient and rapid tallies.

and its most_common() method:

  Return a list of the n most common elements and their counts from the most common to the least.

That simplifies the code to

from collections import Counter

def frequencySort(s):
    """
    :type s: str
    :rtype: str
    """
    counter = Counter(s)
    output = "".join(char * freq for char, freq in counter.most_common())
    return output


Printing a singly linked list with time complexity O(n) and space complexity O(sqrt(n)) My aim is to write a method which prints contents of a singly linked list in reverse order with time complexity \$O(n)\$ and space complexity \$O(\sqrt{n})\$. The best I could come up with is to store the pointers to nodes in an array and print it in reverse order.

void display_reverse(List *l)
{
    int list_size = size(l);
    Node** pointer_array = malloc(list_size*sizeof(Node*));
    Node* temp = l->head;
    int i;
    for(i = 0; i<list_size; ++i, temp=temp->next)
        pointer_array[i] = temp;
    for(i = list_size - 1; i>=0; --i)
        printf("%d\n", pointer_array[i]->data);
    free(pointer_array);
}

int size(List* l)
{
    int count = 0;
    Node* temp = l->head;
    while(temp!= NULL)
    {
        ++count;
        temp = temp->next;
    }

    return count;
}

Is there a better way of doing it, without modifying the list in any way?  There is another option where time complexity is \$O(n \log n)\$ and space complexity is \$O(\log n)\$. How can I go about solving this?

Reverse the list

A quick search on the internet will show you that you can reverse a linked list in-place using \$O(n)\$ time and \$O(1)\$ space.  All you need to do after that is print the list in order:

void display_reverse(List *list)
{
    list = reverseList(list);
    printList(list);
    list = reverseList(list);
}

This whole function will take \$O(n)\$ time and \$O(1)\$ space.

Without modifying the list

If you can't modify the list, you can do it by making copies of part of the list.  If the list is \$n\$ items, you can make \$\sqrt n\$ sized sublist copies, and reverse/print them one at a time.  To do that, you will need to use a \$\sqrt n\$ sized array to keep track of these sublists.  Here is what the program would look like:

void display_reverse(const List *list)
{
    int n     = countItemsInList(list);
    int sqrtn = sqrt(n);
    const List *subLists[sqrtn];
    const List *p;

    // Remember the head of each sqrtn sized sublist
    for (int i=0, p = list; i < n; i++, p = p->next) {
        if (i % sqrtn == 0)
            subLists[i / sqrtn] = p;
    }

    // Make a reversed copy of each subList and print it.
    for (int i=sqrtn-1; i >= 0; i--) {
        int subListSize = sqrtn;
        if (i == sqrtn-1) {
            // The last subList size may be different.
            subListSize = n - (sqrtn-1) * sqrtn;
        }
        List *tempList = createReversedList(subList[i], subListSize);
        printList(tempList);
        freeList(tempList);
    }
}

static List *createReversedList(const List *head, int size)
{
    List *ret = NULL;

    for (int i=0; i<size; i++) {
        List *newNode = malloc(sizeof(List));
        newNode->data = head->data;
        newNode->next = ret;
        ret = newNode;
        head = head->next;
    }
    return ret;
}

This program uses \$O(\sqrt n)\$ space because it uses one array of that size and one temporary list of that size.  It runs in \$O(n)\$ time.

You may want to round up using sqrtn = ceil(sqrt(n)).  That prevents the last sublist size from exceeding sqrtn, so it has a slightly better space utilization.


Validating conditions for enabling sign-up button On an authentication view, there are some inputs, like password, email, etc and a sign up button which is enabled only if theses inputs are not empty.

So what I have is this function:

func canSignUpButtonBeEnabled() -> Bool {

    let password = self.passwordTextField.text.stringByReplacingOccurrencesOfString(" ", withString: "")

    return (!self.emailTextField.text.isEmpty && !self.passwordTextField.text.isEmpty
            && !self.firstNameTextField.text.isEmpty  && !self.lastNameTextField.text.isEmpty
            && countElements(password) >= 4)

}

And after each character entered by the user, I set on the enable property of my button the result of this function.

So I was wondering if there is a cleaner solution to do it?

You bind password but then in your return logic you use self.passwordTextField.text.isEmpty which seems to be wrong - but would get caught by countElements(password) >= 4 later.

This would be arguably more readable, and perhaps modular, if you introduced intermediate predicates.  Like such:

class Whatever {

  func hasName () -> Bool {
    return !self.firstNameTextField.text.isEmpty  &&
           !self.lastNameTextField.text.isEmpty
  }

  func hasEmail () -> Bool {
    return !self.emailTextField.text.isEmpty
  }

  func hasValidPassword () -> Bool {
    let password = self.passwordTextField.text.stringByReplacingOccurrencesOfString(" ", withString: "")
    return countElements(password) >= 4
  }

  func canSignUpButtonBeEnabled() -> Bool {
    return hasEmail() && hasName() && hasValidPassword()
  }
}


Generically typed interface I came across the following code in our code base:

public interface ICloneable<T>
{
    T Clone();
}

public class MyObject : ICloneable<MyObject>
{
    public Stuff SomeStuff { get; set; }

    T Clone()
    {
        return new MyObject { SomeStuff = this.SomeStuff };
    }
}

The pattern of using a generic interface and then doing class A : Interface<A> looks pretty bizarre to me. I have a feeling it's either useless or that it could be changed into something less intricated.
Can someone explain if this is right/wrong, and how it should be changed?

You see this same pattern in the framework for IEquatable<T>.  The most frequent use case for this is writing a generic method where you constrain the generic parameter type to implement the interface.  This then allows you to write code in terms of a statically typed method Equals<T>.  Another example is IComparable<T>, which is very handy for implementing methods relying on sorting without having to use the older style of providing an external Comparator class.  For instance, the default comparison mechanism for the LINQ OrderBy method uses IComparable<T>, if it's available.  In both of these cases, it's very natural to say that an instance of a type is comparable or equatable to other instances of the same type.


Scraping different categories using different links through proxy I've written a script in python to scrape name, review_star and review_count by going through thousands of links (to different products) stored in a csv file using reverse search. As those links are of amazon site so it is very natural to get the ip address banned for a short time while making use of few links only. However, to retain the continuation it is necessary to filter this process through proxy. This is what I tried to do here and it is running smoothly. For the record: as these proxies are collected from web, they may not last long. Anyways, this scraper is supposed to make requests using each links from the csv file, collect the product name, review_star and review_count from amazon site without being blocked. Considering the space, I only used three proxies in my scraper. I tried my level best to make it flawless and it is working without leaving any complaint at this moment. Any suggestion to make this better will be highly appreciated.

This is the script I've written:

import csv
import requests
from lxml.html import fromstring

def crawl():

    proxy={
        'https': 'https://188.166.154.140:8118',
        'https': 'https://198.52.44.130:53281',
        'https': 'https://46.101.2.115:8118'
        }

    with open("amazon.csv", 'r') as input_file:
        for entry in csv.DictReader(input_file):
            url = entry['Link']
            response = requests.get(url,headers={'User-Agent':'Mozilla/5.0'},proxies=proxy)
            root = fromstring(response.text)
            name = root.cssselect("#productTitle")[0].text.strip() if root.cssselect("#productTitle") else ""
            star = root.cssselect(".AverageCustomerReviews .a-icon-alt")[0].text if root.cssselect(".AverageCustomerReviews .a-icon-alt") else ""
            count = root.cssselect(".AverageCustomerReviews .totalReviewCount")[0].text if root.cssselect(".AverageCustomerReviews .totalReviewCount") else ""
            print("Name: {}\nStar: {}\nCount: {}".format(name,star,count))

if __name__ == '__main__':
    crawl()

These are the five links out of thousands which are supposed to store in a csv file named amazon.csv containing a header Link:

Link
https://www.amazon.com/dp/B013KBZ9RY
https://www.amazon.com/dp/B004AC6PQW
https://www.amazon.com/dp/B01JJBA06Y
https://www.amazon.com/dp/B001G9G6XY
https://www.amazon.com/dp/B000UCM0P6

Assuming the proxy dictionary with duplicate keys is just a posting error, here are some of the things I think need to be addressed:

as usual, create a requests.Session() instance and re-use - this would not only allow to set headers and proxies once, but also improve performance
I would define headers as a constant or go even step further - rotate user agent strings as well - there is a fake-useragent project that would help
there is also this DRY principle violation - you are repeating cssselect() calls twice per element - in this case, it's not only the repetition but is also something that unnecessarily slows the HTML parsing down. Define a reusable function that would handle the problem:

def extract_field(root, selector, default=""):
    try:
        return root.cssselect(selector)[0].text.strip()
    except IndexError:
        return default

Here is the code with the above and other modifications applied:

import csv

import requests
from lxml.html import fromstring

def extract_field(root, selector, default=""):
    try:
        return root.cssselect(selector)[0].text.strip()
    except IndexError:
        return default

def crawl():
    proxy = {  # assuming is correct
        'https': 'https://188.166.154.140:8118',
        'https': 'https://198.52.44.130:53281',
        'https': 'https://46.101.2.115:8118'
    }

    with open("amazon.csv", 'r') as input_file, requests.Session() as session:
        session.headers = {'User-Agent': 'Mozilla/5.0'}
        session.proxies = proxy

        for entry in csv.DictReader(input_file):
            url = entry['Link']
            response = session.get(url)
            root = fromstring(response.text)

            name = extract_field(root, "#productTitle")
            star = extract_field(root, ".AverageCustomerReviews .a-icon-alt")
            count = extract_field(root, ".AverageCustomerReviews .totalReviewCount")

            print("Name: {}\nStar: {}\nCount: {}".format(name,star,count))

if __name__ == '__main__':
    crawl()


Most elegant approach to writing list renumbering function I'm writing a function with the following signature:

(Ord a) => [a] -> [Int]

It's semantics is to find an order-preserving mapping from list of objects to list of Ints.

The version of mine:

import Data.List
import Data.Map (Map, (!))
import qualified Data.Map as Map

renumber :: (Ord a) => [a] -> [Int]
renumber list = let mapping = buildmapping list Map.empty in
                map (mapping!) list
        where
            -- buildmapping :: (Ord a) => [a] -> Map a Int -> Map a Int
            buildmapping list mapping = let
                mapped = Map.keys mapping
                (rest, nextindex) = if (null mapped) then
                      (list, 1) else
                      (filter (> maximum mapped) list,
                        (1+) $ maximum $ Map.elems mapping)
                in case rest of
                  [] -> mapping -- nothing left, return
                  _  -> buildmapping rest $ Map.insert (minimum rest) nextindex mapping

I think a sorting-based approach would be simpler than using a map. The basic idea is that we sort the list, assign each item a value based on its position in the list, then undo the sorting and take the value assigned to each item. In order to undo the sorting we have to remember the position each item was at before the sort. So we get the following steps (illustrated using the example input ["a", "c", "b", "c"]):

Annotate each item with its index. So ["a","c","b","c"] becomes [(1, "a"), (2, "c"), (3, "b"), (4, "c")].
Sort the annotated items by their value. So for the example we get [(1, "a"), (3, "b"), (2, "c"), (4, "c")]
Group the sorted annotated items by their values, so that duplicate items get assigned the same number in the next step. In the example: [[(1, "a")], [(3, "b")], [(2, "c"), (4, "c")]]
Now annotate each group with its index again. In the example: [(1, [(1, "a")]), (2, [(3, "b")]), (3, [(2, "c"), (4, "c")])].
Flatten the groups, so that instead of a list of groups we have a list of items again where each item has an old index and new index. In the example: [(1, (1, "a")), (2, (3, "b")), (3, (2, "c")), (3, (4, "c"))].
Now undo the sorting by sorting the items by their old index. In the example: [(1, (1, "a")), (3, (2, "c")), (2, (3, "b")), (3, (4, "c"))].
Lastly map each item to their new index. So for the example we get the output [1,3,2,3].

This leads to the following code:

{-# LANGUAGE TupleSections #-}
import Data.List
import Data.Function (on)
import Data.Ord (comparing)

renumber :: (Ord a) => [a] -> [Int]
renumber = map fst . unsort . number . indexAndSort
    where
      indexAndSort = sortBy (comparing snd) . zip [1..]
      number = concatMap (\(i,xs) -> map (i,) xs) . zip [1..] . groupBy ((==) `on` snd)
      unsort = sortBy (comparing (fst . snd))

Note that I chose 1-based indices instead of 0-based indices because you start numbering at 1 as well and this way my code produces the same results as yours.


Search file for keyword program Just looking for some advice and I know using std namespace is bad but I didnt have the time to do all the using stuff. I want help on what here is good and what is bad practice :).

#include "stdafx.h"
#include <iostream>
#include <fstream>
#include <string>

using namespace std;

class file {
public:
    file() = default;
    file(const string &fileN) : fileName(fileN) { if (!fileName) { cout << "Invalid file closing" << endl; fileName.clear(); fileName.close(); } }

    bool setWord(const string &tWord);
    bool searchFile(const string&);
    ostream &output(ostream&) const;
private:
    ifstream fileName;
    string word;
    int occured;
};

bool file::setWord(const string &tWord) {
    if (!tWord.empty()) {
        word = tWord;
        return true;
    }
    else {
        cerr << "The word your trying to set is empty" << endl;
        return false;
    }
}

bool file::searchFile(const string &tWord) {
    if (setWord(tWord)) {
        string temp;
        occured = 0;
        while (fileName >> temp) {
            if (temp == word) {
                ++occured;
            }
        }
        return true;
    }
    return false;
}

ostream &file::output(ostream &os) const {
    os << word << " occured " << occured << " times in your file";
    return os;
}

int main()
{
    file test("stdafx.h");
    if (test.searchFile("hey"))
        test.output(cout) << endl;
    return 0;
}

If I understand correctly, the program counts the occurrences of some words in a file. I have the following observations:

Error handling

I would recommend throwing exceptions instead of/additionally to writing to cout and cerr (btw., I think if you do write to these streams, you should stick to one of them, most probably cerr).

EDIT: Note, that as the constructor is written currently, we can end up having an object in an invalid state after calling it. This would be prevented by throwing an exception.

Encapsulation, state of the class

I suggest making setWord private, or at least protected. As it is now, one could call:

file test("stdafx.h");
test.searchFile("hey");
test.setWord("fooBar");
test.output(cout) << endl;

Also note, that currently it is possible that a client calls test.output straight away:

file test("stdafx.h");
test.output(cout) << endl;

In this case, he would probably get meaningless results. I therefore suggest introducing a check at the beginning of the implementation of the  output method (if you need it at all, see below), and e.g. throw an exception if no search was performed yet.

Constructors

In this particular case, I would suggest initialising fileName only in the body of the constructor, after checking the validity of the parameter. In other words, I would first validate, and only then initialise, instead of initialising with a (possibly invalid) parameter and then cleaning up.

Also, it is not clear to me what the purpose of the default constructor is. Would an object constructed through that constructor work correctly? If not, then you can probably remove it, and keep only the parametrised one.

Destructors

I recommend adding a destructor to this class, where the file stream opened in the constructor is closed again.

Idempotency

Note, that currently it is possible to call searchFile only once. After the first call, we will be at the end of the input stream, and thus it is not possible to read from it anymore.
I therefore recommend to either reset the stream (or close/open it again), or introduce a similar check as mentioned above for the state of the object (i.e., throw an exception if searchFile is called twice).

Responsibility of the class

Instead of the method ostream &output(ostream&) const;, I would rather introduce a method int getWordCount() const, which would return the actual count, and then leave it up to the client how/if he wants to print this count (of course this would also need the above mentioned check, whether the object is in the right state).

Maybe it would be even better to have searchFile return the count directly (and throw an exception if the word we are looking for is empty, instead of communicating this fact through the return value).

Naming

I recommend changing the name of the variable fileName to something which better reflects what it is, e.g. fileStream. (fileName makes me think of a string holding the name of the file, not a stream through which the contents can be read).

Code blocks

I suggest always adding braces around code blocks, e.g. for the code above:

if (test.searchFile("hey")) {
    test.output(cout) << endl;
}

A modified version

I have modified the original code to demonstrate usage of exceptions, and include some of the other suggested changes. Note, that by removing some of the methods most of the "illegal state" problems can be fixed as well.
I did not address the "idempotency" issue.

// search.cpp

//#include "stdafx.h"
#include <iostream>
#include <fstream>
#include <string>

using namespace std;

class FileNotFoundException {};
class IllegalArgumentException {};

class file {
public:
    file(const string &fileN) : fileStream(fileN.c_str()) {
       if (!fileStream) {
          cerr << "Invalid file closing" << endl;
          throw FileNotFoundException();
       }
    }
    ~file() {
       if (fileStream) {
          fileStream.close();
       }
    }

    int searchFile(const string&);
private:
    ifstream fileStream;
};

int file::searchFile(const string &tWord) {
    if (tWord.empty()) {
       cerr << "Word is empty" << endl;
       throw IllegalArgumentException();
    }
    string temp;
    int occured = 0;
    while (fileStream >> temp) {
        if (temp == tWord) {
            ++occured;
        }
    }
    return occured;
}

int main(int argc, char * argv [])
{
    try {
        file test("stdafx.h");   // does not exist on my system
    }
    catch (const FileNotFoundException &) {
        cerr << "FileNotFoundException caught!" << endl;
    }

    file test2("search.cpp");  // it is this file
    const int foundCnt = test2.searchFile("file");

    cout << "Found " << foundCnt << " occurrences. " << endl;

    file test3("search.cpp");
    try {
        const int foundCnt2 = test3.searchFile("");
    }
    catch (IllegalArgumentException &) {
        cerr << "IllegalArgumentException caught!" << endl;
    }

    return 0;
}

Note that by having the constructor throwing an exception if the file name is invalid, we prevent the file object from being used in an invalid state. The reason is, that (in our example above), we cannot use the object test after this line:

  file test("stdafx.h");   // does not exist on my system


Find the second-largest number in an array of elements I have written this code to find second largest element in an array of random integers.  If it needs to be optimized, then do comment on it.  First of all I'm populating all the elements into the list which does not have any repeating value, then sorting, then in  list.size()-2 I have second largest element.

This is one way of doing this:

int[] randomIntegers = {1, 5, 4, 2, 8, 1, 1, 6, 7, 8, 9};
int max = Integer.MIN_VALUE;
int secMax = Integer.MIN_VALUE;
List<Integer> list = new ArrayList<>();
for(int i:randomIntegers) {
    if(!(list.contains(i))){
        list.add(i);
    }
}
Collections.sort(list);
System.out.println(list.get(list.size()-2));

If you are not allowed to sort that list then you can do the following to find the second-largest element:

int[] randomIntegers = {1, 5, 4, 2, 8, 1, 1, 6, 7, 8, 9};
int max = Integer.MIN_VALUE;
int secMax = Integer.MIN_VALUE;
List<Integer> list = new ArrayList<>();
for(int i:randomIntegers) {
    if(!(list.contains(i))){
        list.add(i);
    }
}
for(int i:list) {
    if(max < i) {
        secMax = max;
        max = i;
    }
    else if(secMax < i){
        secMax = i;
    }
}
System.out.println("Second Largest Number is :" + secMax);

ArrayList is not the right tool.  Since ArrayList makes no attempt to arrange its elements, ArrayList.contains(...) is inefficient — it has to examine every previously added element to see whether it is present.  Then, at the end, you still have to sort the entire list.  If you're adding n numbers, and adding each number takes O(n) time, then your entire solution is O(n2), which is horrible for such a simple problem.

If you want to use a Java Collections data structure, why not use SortedSet instead?  The advantages of SortedSet are:

It maintains the order of elements as it adds them
It automatically deduplicates (TreeSet does it in O(log n) time)
You don't have to sort the elements again at the end

Here's a solution (that works in O(n log n) time):

int[] randomIntegers = { 1, 5, 4, 2, 8, 1, 1, 6, 7, 8, 9 };
SortedSet<Integer> set = new TreeSet<Integer>();
for (int i: randomIntegers) {
    set.add(i);
}
// Remove the maximum value; print the largest remaining item
set.remove(set.last());
System.out.println(set.last());

A more efficient and minimalist solution wouldn't use any data structure.  You just keep track of the two largest numbers you have encountered as you walk along the input array.  See this related question.  That approach hardly uses any space and works in O(n) time, which is as good as it gets.