Loop through all virtualhost log files and run goaccess on each file So I have multiple websites running under apache2 virtualhost - and I wanted to use GoAccess to process the access.log for each site.

The directory structure is like so:

/home/www/site1/html
/home/www/site1/log
/home/www/site1/stats

/home/www/site2/html
/home/www/site2/log
/home/www/site2/stats

Some sites contain two different access.log files -

ssl.access.log for SSL

access.log for non-SSL

These are located in the /log directory of each site

I wanted a cronjob to run every night to process the stats with GoAccess, but I didn't want to write multiple lines of nearly duplicate commands.

I've never written a bash script before, and so I do not know if this is the most efficient way of doing things.

Each report that is generated, needs the month/year in it, so each night it gets overwritten with that months latest stats.

the reports are outputted in the stats directory of each site, in the following format

yyyy-mm.html
sslyyyy-mm.html

The Script

#!/bin/bash

# find all log files which match ess.log
LOG_FILES="/home/www/*/log/*ess.log"

# set the date format
NOW=$(date +"%Y-%m")

# loop through each log file
for f in $LOG_FILES
do

  # drop back from /home/www/site/log to /home/www/site
  path=`dirname $f`
  path=`dirname $path`

  # get the current log filename
  filename=`basename $f`

  # if /home/www/site/stats does not exist - create it
  if [ ! -d "$path/stats" ]; then
    mkdir "$path/stats"
  fi

 # get the first part of the log filename
 prefix=(${filename//./ })

 # if its equal to access, then it's not ssl log, so remove the prefix
 if [ $prefix == 'access' ]; then
   prefix=''
 fi

  # run the goaccess process
  goaccess -f $f --date-format=%d/%b/%Y --log-format='%h %^[%d:%^] "%r" %s %b "%R" "%u"' -a > "$path/stats/$prefix$NOW.html"

done

I know this is a fairly simple task, but as I have not any experience specifically in this it would be great to know where I could improve this.

The script has problems with quoting and wildcard expansion.  The script could break in unexpected ways if there are spaces or shell metacharacters in the paths.  In general, when writing shell scripts, anytime you want to write $variable, you should probably be writing "$variable" instead, expanding the variable in double-quoted context.  The only unquoted variable in this script should be $LOG_FILES, because you do want wildcard expansion to occur there.

I think you are manipulating variables too much.  For each $f, you are interested in the directory that will contain the output and the filename of the output.  I think that the following script would be shorter and more obvious.

LOG_FILES=/home/www/*/log/*access.log
NOW=$(date +%Y-%m)

for f in $LOG_FILES ; do
  path=`dirname "$f"`/../stats
  mkdir -p "$path"                                   # Creates directory as necessary

  case `basename "$f"` in
    ssl.access.log) output="$path/ssl$NOW.html" ; ;;
        access.log) output="$path/$NOW.html"    ; ;;
                 *) exit 1                           # Shouldn't be possible
  esac

  goaccess -f "$f" --date-format=%d/%b/%Y \
           --log-format='%h %^[%d:%^] "%r" %s %b "%R" "%u"' -a > "$output"
done


Add timeline link to questions and answers on Stack Exchange The following script is meant to be a user script, but it should work on the console. It takes the id of the question and adds the corresponding /posts/id/timeline link to the post-menu element, just besides others. It's meant to not have problem with other scripts. The same as above for the answers, just that in this case it should loop through all the answers and add its corresponding timeline link.

let question = document.getElementById("question");
let qid = question.dataset.questionid;
if (qid) {
    let span = document.createElement("span");
    let alink = document.createElement("a");
    alink.class = "userscript-timeline";
    alink.href = "/posts/" + Number(qid) + "/timeline";
    alink.title = "timeline for this question";
    alink.textContent = "timeline";
    span.appendChild(alink);
    question.getElementsByClassName("post-menu")[0].appendChild(span);
}
let answers = document.getElementsByClassName("answer");
if (answers) {
    for (let i = 0; i < answers.length; i += 1) {
        let aid = answers[i].dataset.answerid;
        let answer = document.getElementById("answer-" + aid);
        let span = document.createElement("span");
        let alink = document.createElement("a");
        alink.class = "userscript-timeline";
        alink.href = "/posts/" + Number(aid) + "/timeline";
        alink.title = "timeline for this answer";
        alink.textContent = "timeline";
        span.appendChild(alink);
        answer.getElementsByClassName("post-menu")[0].appendChild(span);
    }
}

I'm not sure if it's convenient to abstract the logic that create and adds the link, as I'm afraid it could introduce needless complexity. My naming sense could be better.

Direct per-attribute DOM building belongs to the '00s but, since you're using ES2015's let, let's use multiline template strings and make the final result of our code instantly readable:

let question = document.getElementById("question");
if (question) {
    question.querySelector(".post-menu").insertAdjacentHTML('beforeend', `

            timeline

    `);
}

for (let answer of document.getElementsByClassName("answer")) {
    answer.querySelector(".post-menu").insertAdjacentHTML('beforeend', `

            timeline

    `);
}

Notes:

.getElementsByClassName("abc")[0] ———> .querySelector(".abc")
Of course the former is faster but it doesn't matter in our case.
getElementsByClassName can never be null, it's a live HTMLCollection, so we can directly use it in a loop.
insertAdjacentHTML speed is about the same or faster than manual node building.


Sort a stack using Java This is my attempted solution to cracking the coding interview exercise 3.5.  Looking for any feedback on coding style or the algorithm anywhere I can improve really.

The problem specification is as follows.

  Write a program to sort a stack such that the smallest items are on the top.  You can use an additional temp stack, but you may not copy the elements into any other data structure (such as an array).  The stack supports push, pop, peek and isEmpty.

MyStack.java

package problem_2_8;

import java.util.ArrayList;
import java.util.EmptyStackException;

public class MyStack<T extends Comparable<T>> {

    private static class StackNode<T extends Comparable<T>> {
        T data;
        StackNode<T> next;

        StackNode(T data){
            this.data = data;
        }
    }

    private StackNode<T> top = null;
    private int size;

    public MyStack(){}

    public int size(){
        return size;
    }

    T pop(){
        if(top == null) throw new EmptyStackException();
        T item = top.data;
        top = top.next;
        size--;
        return item;
    }

    void push(T data){
        StackNode<T> node = new StackNode<T>(data);
        if(top != null){
            node.next = top;
        }
        top = node;
        size++;
    }

    T peek(){
        if(top == null) throw new EmptyStackException();
        return top.data;

    }

    boolean isEmpty(){
        return size == 0;

    }

    void sortStack(){
        MyStack<T> tempStack = new MyStack<T>();

        while(!isEmpty()){
            tempStack.insertToSortedStack(this, pop());
        }

        while(!tempStack.isEmpty()){
            push(tempStack.pop());
        }
    }

    void insertToSortedStack(MyStack<T> auxStack, T item){
        int countPops = 0;
        while(!isEmpty() && item.compareTo(peek()) < 0){
            auxStack.push(pop());
            countPops++;
        }
        push(item);

        for(int i = 0; i < countPops; i++){
            push(auxStack.pop());
        }

    }

    void printStack(){
        StackNode<T> current = top;
        ArrayList<String> stackAsString = new ArrayList<String>();
        while(current != null){
            stackAsString.add(current.data.toString());
            current = current.next;
        }
        System.out.println("Top -> " + String.join(", ", stackAsString));
    }

}

I ran the code as follows to test that the sorting works as expected.

Main.java

package problem_2_8;

public class Main {

    public static void main(String[] args) {
        MyStack<Integer> myStack = new MyStack<Integer>();
        myStack.push(5);
        myStack.push(7);
        myStack.push(11);
        myStack.push(5);
        System.out.println("Stack before sorting: ");
        myStack.printStack();
        myStack.sortStack();
        System.out.println("Stack after sorting: ");
        myStack.printStack();

        MyStack<String> myStackOfStrings = new MyStack<String>();
        myStackOfStrings.push("x");
        myStackOfStrings.push("c");
        myStackOfStrings.push("y");
        myStackOfStrings.printStack();
        myStackOfStrings.sortStack();
        myStackOfStrings.printStack();
    }
}

The first thing you should do is assess the visibility of your methods.  Some should be public, and others private.  I don't see any that should stay package-protected as you have them now.

Next, instead of having a printStack() method, you should consider overriding the toString() method.  Just a Java standard.

Now, I don't know if you're as far as multi-threading in your modules, but I can see some cases where your Stack is not thread-safe (particularly around the sorting).  I'll leave the problem of how to solve this as an exercise to the questioner.


Copy table using Selenium and Python I have some Python code that copies a table from a website using Selenium,to create a csv, but as I haven't used Selenium much, I have that irksome feeling in the back of my mind that there must be a better way. It's also quite a bit slower than I would like. Here's the relevant code:

# ...Navigate to proper page...

table = self.browser.find_element_by_id('data_table')
head = table.find_element_by_tag_name('thead')
body = table.find_element_by_tag_name('tbody')

file_data = []

file_header = []
head_line = head.find_element_by_tag_name('tr')
headers = head_line.find_elements_by_tag_name('th')
for header in headers:
    header_text = header.text.encode('utf8')
    file_header.append(header_text)
file_data.append(",".join(file_header))

body_rows = body.find_elements_by_tag_name('tr')
for row in body_rows:
    data = row.find_elements_by_tag_name('td')
    file_row = []
    for datum in data:
        datum_text = datum.text.encode('utf8')
        file_row.append(datum_text)
    file_data.append(",".join(file_row))

with open(srcFile, "w") as f:
    f.write("\n".join(file_data))

First off, I see a couple of things that can be shortened to generator expressions, rather than full-blown for loops. For example, this section:

file_header = []
head_line = head.find_element_by_tag_name('tr')
headers = head_line.find_elements_by_tag_name('th')
for header in headers:
    header_text = header.text.encode('utf8')
    file_header.append(header_text)
file_data.append(",".join(file_header))

Can be shortened immensely  to the following:

head_line = head.find_element_by_tag_name("tr")
file_header = [header.text.encode("utf8") for header in head_line.find_elements_by_tag_name('th')]
file_data.append(",".join(file_header))

Finally, your other for loop can be shortened to a generator expression as well. For more on generator expressions, see PEP0289.


Calling functions with variable functions I have some PHP code where I use variable functions to call the right function. I need to build a chart array (for example), and the chart array that comes out has a fixed format. But the data that goes into the chart or table comes from varying places in the underlying objects.

An example:

private function buildChartGroups(PropelCollection $groups, $propertyForDisplay) {
    $groupDisplayFunction = "getSum$propertyForDisplay";

    $chart = array();
    foreach ($groups as $group) {
            $chart[] = array('name' => $group->getName(),
                             'y' => $group->$groupDisplayFunction()
                       );
        }
    }
    return $chart;
}

and then the function gets called something like:

$result = $this->buildChartGroups($groups, "Total");
...elsewhere...
$result = $this->buildChartGroups($groups, "Count");

where $groups is an array of Group objects:

class Group {
   private $name;
   private $sumTotal;
   private $sumCount;

   public function getName() {
       return $this->name;
   }

   public function getSumTotal() {
        return $this->sumTotal;
   }

   public function getSumCount() {
        return $this->sumCount;
   }
}

This works fine and lets me re-use buildChartGroups() which I would otherwise not be able to do. But using variable functions and passing function names as strings always seems a little dirty, even though I'm sure it has its place.

Is there a way to refactor this or is there some pattern I can use to avoid them?

I agree, it does feel dirty to compute the function name like that.

A typical way to do dynamic dispatching is using polymorphism.  You could formalize the mechanism for determining which function to call by making some property-extracting objects:

abstract class PropertyExtractor {
    public abstract function getProperty($group);
}

class SumTotalExtractor extends PropertyExtractor {
    public function getProperty($group) {
        return $group->getSumTotal();
    }
}

class SumCountExtractor extends PropertyExtractor {
    public function getProperty($group) {
        return $group->getSumCount();
    }
}

Your buildChartGroups() would take one of those extractors as the second parameter:

private function buildChartGroups(PropelCollection $groups, $propertyExtractor) {
    $chart = array();
    foreach ($groups as $group) {
        $chart[] = array('name' => $group->getName(),
                         'y' => $propertyExtractor->getProperty($group));
    }
    return $chart;
}

Here it is in use:

$result = $this->buildChartGroups($groups, new SumTotalExtractor());


Registration form validation in jQuery I have created a registration form using jQuery. I am validating username and password using an AJAX request. The "submit" button is originally disabled and is only enabled when all three fields of username, email and password are filled and validated.

How and what can I improve in this code?

$(document).ready(function() {
    var usernameFlag = 1, emailFlag = 1, passwordFlag = 1;

    //event handler on input for username
    document.getElementById("reg_username").addEventListener("input", function() {
        var username = $("#reg_username").val();
        if(username !== null && username !== "") {
            usernameCheck(username).done(usernameCallback);
        }
        else {
            usernameFlag = 1;
            $("#tick_username").hide();
            $("#cross_username").hide();
            enableButton();
        }
    });

    //event handler on input for email
    document.getElementById("reg_email").addEventListener("input", function () {
        var email = $("#reg_email").val();
        if(email !== null && email !== "") {
            emailCheck(email).done(emailCallback);
        }
        else {
            emailFlag = 1;
            $("#tick_email").hide();
            $("#cross_email").hide();
            enableButton();
        }
    });

    //event handler on input for password
    document.getElementById("reg_password").addEventListener("input", function () {
        var password = $("#reg_password").val();
        if (password !== null && password !== "") {
            passwordFlag = 0;
            enableButton();
        }
        else {
            passwordFlag = 1;
            enableButton();
        }
    });

    function usernameCallback(response) {
        var usernameResponse = parseInt(response);
        usernameFlag = usernameResponse;
        if(usernameResponse === 1) {
            $("#tick_username").hide();
            $("#cross_username").fadeIn();
        }
        else {
            $("#cross_username").hide();
            $("#tick_username").fadeIn();
        }
        enableButton();
    }

    function emailCallback(response) {
        var emailResponse = parseInt(response);
        emailFlag = emailResponse;
        if(emailResponse === 1) {
            $("#tick_email").hide();
            $("#cross_email").fadeIn();
        }
        else {
            $("#cross_email").hide();
            $("#tick_email").fadeIn();
        }
        enableButton();
    }

    //ajax call to validate username
    function usernameCheck (username) {
        return $.ajax({
            type: "POST",
            url: "check_availability.php",
            data: "username="+username
        });
    }

    //ajax call to validate email
    function emailCheck (email) {
        return $.ajax({
            type: "POST",
            url: "check_availability.php",
            data: "email="+email
        });
    }

    //function to enable submit button
    function enableButton () {
        if (usernameFlag === 0 && emailFlag === 0 && passwordFlag === 0) {
            $("#register_btn").prop("disabled", false);
        }
        else {
            $("#register_btn").prop("disabled", true);
        }
    }

});

I am using three extra variables to keep track of the fields. The flags are updated if the user changes a value. After every change in input field, enableButton() checks the flags and enables the button if all of them have a value of 0.

Here is my take, dry some stuff.

You can glue each piece of code together to have a working script (I guess).

Keep track of your validations

var flags = {
    username: 1,
    password: 1,
    email: 1
};

jQuery seems to be included, so let's use it.
The code of the reg_username and reg_email seems to be similair, so let's DRY it into a function named validates

$('#reg_username').on('input', function () {
    validates('username', $(this).val());
});

$('#reg_email').on('input', function () {
    validates('email', $(this).val());
});

The code of reg_password can be shorten a little.

$('#reg_password').on('input', function () {
    flags.password = !isBlank($(this).val())
    handleButton();
});

Here is the validates function

function validates (type, value) {
    if (!isBlank(value)) {
        remoteCheck(type, value).done(remoteCallback);
    } else {
        flags[type] = 1;
        hideFeedback(type);
        handleButton();
    }
}

You can dryify emailCheck and usernameCheck in a single function named remoteCheck by passing email or username in arguments.

function remoteCheck (type, value) {
    return $.ajax({
        type: 'POST',
        url: 'check_availability.php',
        data: type + '=' + value
    });
}

And here is the dryed callback for remoteCheck

function remoteCallback (type, value) {
    flags[type] = parseInt(response);
    handleFeedback(type, flags[type])
    handleButton();
}

The feedback can also be handled with a single function

function handleFeedback (type, flag) {
    hideFeedback(type);

    if (flag) {
        $('#cross_' + type).fadeIn();
    } else {
        $('#tick_' + type).fadeIn();
    }
}

function hideFeedback (type) {
    $('#tick_' + type).hide();
    $('#cross_' + type).hide();
}

To improve readability we put the flags checker in a separated function (and it now can be used elsewhere)

function handleButton () {
    $('#register_btn').prop('disabled', !isValid());
}

function isValid() {
    return flags.username === 0 && flags.email === 0 && flags.password === 0
}

You can create an utility function to check if a string is empty or null which is used many times in this script.

function isBlank (string) {
    return string === null || string === ''
}


increase speed & lower memory consumption of text normalization function I'm trying to preprocess a large text document. I've written a text normalization function which takes a disproprtionate amount of time and memory. How can I format the function to lower these two?

The time result below was for this example.

t = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. Curabitur pretium tincidunt lacus. Nulla gravida orci a odio. Nullam varius, turpis et commodo pharetra, est eros bibendum elit, nec luctus magna felis sollicitudin mauris. Integer in mauris eu nibh euismod gravida. Duis ac tellus et risus vulputate vehicula. Donec lobortis risus a elit. Etiam tempor. Ut ullamcorper, ligula eu tempor congue, eros est euismod turpis, id tincidunt sapien risus a quam. Maecenas fermentum consequat mi. Donec fermentum. Pellentesque malesuada nulla a mi. Duis sapien sem, aliquet nec, commodo eget, consequat quis, neque. Aliquam faucibus, elit ut dictum aliquet, felis nisl adipiscing sapien, sed malesuada diam lacus eget erat. Cras mollis scelerisque nunc. Nullam arcu. Aliquam consequat. Curabitur augue lorem, dapibus quis, laoreet et, pretium ac, nisi. Aenean magna nisl, mollis quis, molestie eu, feugiat in, orci. In hac habitasse platea dictumst."

This is the script.

import time
import string
import nltk
import numpy as np

# text preprocessing module, use boolean flags to customize function
def normalize_text(text, lowercase=True, punctuation=True):

    # Step 1: Tokenize
    output = nltk.word_tokenize(text)

    # Step 2: Convert to lowercase (optional)
    if lowercase:
        output = [word.lower() for word in output]

    # Step 3: Remove punctuation:
    if punctuation:
        output = [str(token).translate(str.maketrans('', '', string.punctuation)) for token in output]
        output = [token for token in output if token != '']

    return(output)

Removing all filters and the corresponding if-statements sped up processing by a mere 0.7%.

def normalize_text2(text):

    # Step 1: Tokenize
    output = nltk.word_tokenize(text)

    # Step 2: Convert to lowercase (optional)
    output = [word.lower() for word in output]

    # Step 3: Remove punctuation:
    output = [str(token).translate(str.maketrans('', '', string.punctuation)) for token in output]
    output = [token for token in output if token != '']

    return(output)

Here is the bagged comparison.

times1 = []

for i in range(1000):
    start = time.time()
    tokens = normalize_text(t)
    end = time.time()
    times1.append(end - start)

time1 = np.mean(times1)
print(time1)

times2 = []

for i in range(1000):
    start = time.time()
    tokens = normalize_text2(t)
    end = time.time()
    times2.append(end - start)

time2 = np.mean(times2)
print(time2)

print(time2/time1)

Here are the results:

0.0021646411418914796
0.0021491129398345946
0.9928264312470212

Any advice on how to improve further? For example, how could I reduce the number of different list comprehensions, so that the same sequence of text does not need to crunched anew this many times?

You can save a little bit of time by not re-running str.maketrans for each token, since it's always going to produce the same result:

import nltk
from statistics import mean
import string
import time
from typing import List

def normalize_text3(text: str) -> List[str]:
    output: List[str] = []
    punctuation_filter = str.maketrans('', '', string.punctuation)
    for token in nltk.word_tokenize(text):
        token = token.translate(punctuation_filter)
        if not token:
            continue
        output.append(token.lower())
    return output

tested with:

for func in [normalize_text, normalize_text2, normalize_text3]:
    times = []
    for _ in range(1000):
        start = time.time()
        tokens = normalize_text(t)
        end = time.time()
        times.append(end - start)
    print(f"{func.__name__.rjust(15)}: {mean(times)}")

gets me:

dog runs
 normalize_text: 0.003226396322250366
normalize_text2: 0.0032752704620361327
normalize_text3: 0.0030987038612365725

If you want to lower memory consumption, you might consider having this function return a generator rather than a list...


Mean shift image processing algorithm for color segmentation I'm implementing a version of the mean shift image processing algorithm for color segmentation in Python/NumPy.

I've written a pure NumPy version of the actual mean shifting per pixel (which I imagine is where the majority of time is taking). It slices an array of RGB values to work on out of the parent image, then creates lower bound and higher bound RGB reference arrays and generates a boolean masking array for the pixels to use for averaging then averages.

Any further optimizations? I suspect vectorizing the x/y for loops might give a speed up but for the life of me, but I haven't figured out how. (Some how generating an array of each pixel grid to work on and then generalizing the mean shift to take array input?) gL is grid length. gS is gL squared - the number of pixels in grid.

for itr in xrange(itrs):
    if itr != 0:
        img = imgNew
    for x in xrange(gL,height-gL):
        for y in xrange(gL,width-gL):
            cGrid = img[x-gSmp:(x+gSmp+1),y-gSmp:(y+gSmp+1)]
            cLow,cUp = np.empty((gL,gL,3)),np.empty((gL,gL,3))
            cLow[:] = [img[x,y][0]-tol,img[x,y][1]-tol,img[x,y][2]-tol]
            cUp[:] = [img[x,y][0]+tol,img[x,y][1]+tol,img[x,y][2]+tol]
            cBool = np.any(((cLow < cGrid) & (cUp > cGrid)),axis=2)
            imgNew[x,y] =  np.sum(cGrid[cBool],axis=0)/cBool.sum()

The following code is a first shot and it is still not vectorized. The major points here are the extraction of the creation of cLow and cUp (don't create arrays in loops, always 'preallocate' memory), the calculation of the tolerance levels can be done in one operation (under the assumption that broadcasting is possible at this point) and at last I removed the conditional case for copying the imgNew to img (I also doubt that you do not want to copy the last iteration back into img. If so you have to remove the copy line before the loop and move the copy at the beginning of the loop to its end.).

diff_height_gL = height - gL
diff_width_gL = width - gL
sum_gSmp_one = gSmp + 1

cLow, cUp = np.empty((gL, gL, 3)), np.empty((gL, gL, 3))

imgNew = img.copy()

for itr in xrange(itrs):

    img[:] = imgNew

    for x in xrange(gL, diff_height_gL):
        for y in xrange(gL, diff_width_gL):

            cGrid = img[x-gSmp:(x + sum_gSmp_one), y-gSmp:(y + sum_gSmp_one)]

            cLow[:] = img[x, y, :] - tol
            cUp[:] = img[x, y, :] + tol

            cBool = np.any(((cLow < cGrid) & (cUp > cGrid)), axis=2)

            imgNew[x, y] =  np.sum(cGrid[cBool], axis=0) / cBool.sum()

This problems seems to be perfectly shaped to do multiprocessing. This could be an alternative/extension to vectorization. If I have time I will try the vectorization...

Kind regeards


Sorting parallel ArrayList I have a model class that contains 3 ArrayList which are in order by parallel of the same size. <Object><Calendar><Long> I want to sort it by the <Long> Is this the most clean? is there a better way? This doesn't seem memory efficient.

    public class ResultModel{
        private ArrayList<Object> sets = new ArrayList<Object>();
        private ArrayList<Calendar> dates=new ArrayList<Calendar>();
        private ArrayList<Long> unixtimes=new ArrayList<Long>();
    private class WinningSet implements Comparable<WinningSet>{
            private long unixtime;
            private Object set;
            private Calendar date;
            WinningSet(Object set,Calendar date,long unixtime){
                this.unixtime=unixtime;
                this.set=set;
                this.date=date;
            }
            @Override
            public int compareTo(WinningSet another) {
                return (int) (this.unixtime-another.getUnixtime());
            }
            public long getUnixtime() {
                return unixtime;
            }
            public Object getSet() {
                return set;
            }
            public Calendar getDate() {
                return date;
            }
        }
        public void sortSelf(){
            ArrayList<WinningSet> list=new ArrayList<WinningSet>();
            for(int i=0;i<this.unixtimes.size();i++){
                list.add(new WinningSet(this.sets.get(i),dates.get(i),unixtimes.get(i)));
            }
            Collections.sort(list,Collections.reverseOrder());
            demap(list);
        }
        private void demap(ArrayList<WinningSet> list) {
             ArrayList<Object> tSets = new ArrayList<Object>();
             ArrayList<Calendar> tDates=new ArrayList<Calendar>();
             ArrayList<Long> tUnixtimes=new ArrayList<Long>();
            for(WinningSet temp:list){
                tDates.add(temp.getDate());
                tSets.add(temp.getSet());
                tUnixtimes.add(temp.getUnixtime());
            }
            this.sets=tSets;
            this.dates=tDates;
            this.unixtimes=tUnixtimes;
        }
}

1. Correctnes.

compareTo() method of the WinningSet class is not correct. As of now Integer.MAX_VALUE < System.currentTimeMillis(). Consider the following example:

    long time1 = System.currentTimeMillis();
    System.out.println(time1 > 3*Integer.MAX_VALUE);
    System.out.println(time1 - 3*Integer.MAX_VALUE);
    System.out.println((int)(time1 - 3*Integer.MAX_VALUE));

It could be the case that your events are generated around the same time and it will not result in any error but in general - e.g. in future, if you restore some serialized objects, it could lead to a possible problem. You can rewrite it int the following way:

        @Override
        public int compareTo(WinningSet another) {
            if (this.unixtime > another.getUnixtime()) return 1;
            if (this.unixtime < another.getUnixTime()) return -1;
            return 0;
        }

2. Performance

Copy of the 3 lists is indeed a bad thing. You can use set() method from ListIterator to traverse through the lists and replace values in place.

private void demap(ArrayList<WinningSet> list) {
            Iterator li = list.iterator();
            Iterator si = sets.iterator();
            Iterator di = dates.iterator();
            Iterator ui = unixtimes.iterator();
            //you can add iterators for other lists here.
            while (li.hasNext()) {// we assume number is the same
              WinningSet temp = li.next();
              si.next(); //need to advance to the right element;
              di.next(); //need to advance to the right element;
              ci.next(); //need to advance to the right element;
              si.set(temp.getSet());
              di.set(temp.getDate());
              ui.set(temp.getUnixTime());
            }
        }

3. Design

Are you sure that instead of a 3 lists you can't consider TreeMap<Long, SomeClass> where SomeClass is pair or Calendar and Object ? In this case you will always have the right order of items to work with - there is no need to sort lists on demand and do an extra work.


Rails controller to manage EC2 instances I did a small sample application for an Interview. Since I didn't got accepted I would like your opinion on how to improve it.

The aux methods are private.
I have a usage_common to remove duplication in the usage and usage_instance methods
add_item_hash is used because the format written in the database is different from the used in the API response

The start stop and status actions are used to simple operations in EC2 instances

Maybe I should've broken it in services? one for usage, one for processes and one for EC2.

Should send the params already in the format for database and thus eliminate the methods build_usage_hash and build_process_hash

https://github.com/mabranches/monitor_api/blob/master/app/controllers/instances_controller.rb

class InstancesController < ApplicationController
  before_action :set_instance, only: [:show, :update, :destroy]

  def usage
    usage_common(:get, (Time.now - 24.hours).utc.iso8601, Time.now.utc.iso8601)
  end

  def usage_instance
    usage_common(:get_instance, params[:id], (Time.now - 24.hours).utc.iso8601, Time.now.utc.iso8601)
  end

  def processes_instance
    items = ProcessList.get_instance(params[:id])
    process_items = {}
    items.each do |item|
      process_items[item['instance_id']] = item
    end
    render json: process_items
  end

  def create
    usage = build_usage_hash params
    process = build_process_hash params

    Usage.create(usage)
    ProcessList.create(process)

    render json: [usage, process], status: :created
  rescue => e
    Rails.logger.error("Was not possible to update instance data: #{e.message}")
    render json: { error: e.message }, status: :unprocessable_entity
  end

  def processes
    items = ProcessList.get
    process_items = {}
    items.each do |item|
      process_items[item['instance_id']] = item
    end
    render json: process_items
  end

  def stop
    @params = params
    ec2_action(:stop, 'stopped')
  end

  def start
    @params = params
    ec2_action(:start, 'started')
  end

  def status
    @params = params
    ec2_instance do |ec2|
      @status = ec2.state.name
    end
  end

  private

  def usage_common(method, *args)
    items = Usage.send(method, *args)

    result = {}
    items.each do |item|
      add_item_hash(result, item)
    end
    render json: result
  end

  def ec2_instance
    instance_id = @params[:id]

    return render json: {}, status: :unprocessable_entity unless instance_id
    ec2 = Aws::EC2::Resource.new(region: 'us-west-2').instance(instance_id)
    @status = 'unknown'
    begin
      yield ec2
    rescue => e
      Rails.logger.error("Was not possible to execute action #{@params[:action]}: #{e.message}")
    end
    render json: { instance: instance_id, status: @status }
  end

  def ec2_action(method, status)
    ec2_instance do |ec2|
      ec2.send(method)
      @status = status
    end
  end

  def add_item_hash(result, item)
    result[item['instance_id']] ||= Hash.new { |h, k| h[k] = [] }
    result[item['instance_id']]['mem'] << item['mem']
    result[item['instance_id']]['disk'] << item['disk']
    result[item['instance_id']]['cpu'] << item['cpu']
    result[item['instance_id']]['usage_time'] << item['usage_time']
  end

  def build_usage_hash(params)
    instance = params[:instance]
    {
      instance_id: params[:id],
      cpu: instance[:cpu],
      mem: instance[:mem],
      disk: instance[:disk],
      usage_time: Time.now.utc.iso8601
    }
  end

  def build_process_hash(params)
    instance = params[:instance]
    {
      instance_id: params[:id],
      process: instance[:process].try(:values) || []
    }
  end
end

I see two things wrong with this controller.

It's too 'fat'. You're doing way too much in one controller.
It's not RESTful.

In this case, those two problems are much the same. Without reading too much into the code, it looks like you could break that controller up into 3 controllers, UsageController, InstanceController, Instance::PowerController. I'm not certain about the last one, you may find the Instance's power state not to be a RESTful resource.

Here's an example: https://stackoverflow.com/documentation/ruby-on-rails/4373/method-on-a-controller-that-s-not-part-of-the-default-five#t=201608110000585779419

As well though, I'd say the building of the hashes should be encapsulated by a model.


Using object oriented PHP to display users in a database I'm new to object oriented PHP, so I'm sure of the good or bad practices. The below code is working perfectly, I just don't know if it is a bad or good approach to use most of the methods as static and declare class properties as public.

Could someone provide me some advise?

In users.php

<?php

class Users{

  public $user_id, $user_name, $user_email, $user_fname, $user_lname, $user_role;

  public static function display_users(){
      $display_users[] = self::find_all_query("SELECT * FROM users");
      return (!empty($display_users)) ? array_shift($display_users) : false;
  }

  public static function find_all_query($sql){
    global $database;
    $result = $database->query($sql);
    $db_result = array();
    while($row = mysqli_fetch_assoc($result)){
      $db_result[] = self::instantiation($row);
    }

    return $db_result;

  }

  public static function instantiation($row){

    $the_users_object = new Users();

    foreach ($row as $attribute => $data) {
      if($the_users_object->has_the_attribute($attribute)){
        $the_users_object->$attribute = $data;
      }
    }
    return $the_users_object;
  }

  private function has_the_attribute($attribute){
     $users_properties = get_object_vars($this);
     return array_key_exists($attribute , $users_properties);
  }

}
?>

In admin-users.php

<?php
$display_users = Users::display_users();

foreach ($display_users as $users) {
  echo "
";
    echo " $users->user_id    	";
    echo " $users->user_name   	";
    echo " $users->user_email 	";
    echo " $users->user_fname 	";
    echo " $users->user_lname 	";
    echo " $users->user_role 	";
  echo "
";
}
?>

In database.php

<?php
// require_once("init.php");

class Database{

  private $connection;

      function __construct(){
        $this->open_db_connection();

      }

      public function open_db_connection(){
        $this->connection = new mysqli(DB_HOST,DB_USER,DB_PASS,DB_NAME);

          if($this->connection -> connect_error){
            die('
Database Connection Failed!!!!
.'. $this->connection -> connect_error);
          }

      }

      private function checkQuery($query){
        if(!$query){
          die("
Query Failed!!!!
 ". $this->connection -> error);
        }
      }

      public function query($sql){
        $result = mysqli_query($this->connection , $sql);
        $this->checkQuery($result);
        return $result;

      }

}

      $database = new Database();

?>

Some tips which might help you:

Make as few static methods as possible. Those functions should not
appear in any other function as they get in a way in unit tests. I usually use them to spit out html in views.
Don't use global varibales.
Hide as implementation as you can. Make public only those things that are vital for functionality.
I consider new operator harmful as it cannot be mocked. Use dependency injection instead to provide object with all necessary data.

Last but not least I highly recommend book Clean Code by Robert C. Martin if you care about keeping your code clean.


Scraping columns from SEDE results I use the following script to download the result of a SEDE query and scrape a specific column from it using BeautifulSoup:

import json

def extract_column(soup, colname):
    """
    Returns a generator of cell values in selected column.

    For simple columns like timestamp, a cell value can be simple,
    for example: 1414433013197

    For more complex columns like Post Link, a cell value can be an object,
    for example:

      {
        "id": 68102,
        "title": "Bash Script - File Comment out & Notate"
      }

    :param soup: a bs4 (BeautifulSoup) object
    :param colname: name of the SEDE column to extract
    :return: generator of cell values in selected column
    """
    def get_column_index():
        for index, info in enumerate(columns):
            if info['name'] == colname:
                return index
        return -1

    for script in soup.findAll('script'):
        result_sets_col = 'resultSets'
        if result_sets_col in script.text:
            start = script.text.rindex('{', 0, script.text.index(result_sets_col))
            end = script.text.index('}', script.text.index('querySetId')) + 1
            data = json.loads(script.text[start:end])

            results = data[result_sets_col][0]
            columns = results['columns']
            rows = results['rows']

            column_index = get_column_index()
            if column_index > -1:
                for row in rows:
                    yield row[column_index]

Unit tests:

import os
import unittest

from bs4 import BeautifulSoup
from robosanta.stackexchange.sede import extract_column

BASE_DIR = os.path.dirname(__file__)

SEDE_OUTPUT_HTML = os.path.join(BASE_DIR, 'sede-output.html')
POST_ID_COLUMN = 'Post Link'
DATE_COLUMN = 'CreationDate'
ROW_COUNT = 49

def new_soup():
    with open(SEDE_OUTPUT_HTML) as fh:
        return BeautifulSoup(fh)

class TestGetColumn(unittest.TestCase):
    def extract_column(self, colname):
        return extract_column(new_soup(), colname)

    def extract_post_link(self):
        return self.extract_column(POST_ID_COLUMN)

    def extract_date(self):
        return self.extract_column(DATE_COLUMN)

    def test_row_count_of_post_link(self):
        self.assertEqual(ROW_COUNT, len(list(self.extract_post_link())))

    def test_first_post_link(self):
        expected = {
            'title': 'TinyMVC Model / Plugin how to implement?',
            'id': 12144,
        }
        self.assertEqual(expected, next(self.extract_post_link()))

    def test_row_count_of_date(self):
        self.assertEqual(ROW_COUNT, len(list(self.extract_date())))

    def test_first_date(self):
        self.assertEqual(1338304381360, next(self.extract_date()))

    def test_nonexistent_column(self):
        with self.assertRaises(StopIteration):
            next(self.extract_column('nonexistent'))

if __name__ == '__main__':
    unittest.main()

You can find the content of sede-output.html (the test data) here.

I'm looking for any and all suggestions, especially concerning:

Improving the main implementation (the scraping)
Improving the way to test this

I'd test if not result_sets_col in script.text and continue in that case, to remove a level of nesting:

for script in soup.findAll('script'):
    result_sets_col = 'resultSets'
    if not result_sets_col in script.text:
        continue

    start = script.text.rindex('{', 0, script.text.index(result_sets_col))


Is there a way to shorten a set of conditionals like this? I have a long set of IF statements that basically set a null object Value to 0, I feel that because i am doing the same action each time their has to be a simple way to make this allot shorter. It just looks like something I wouldn't normally see in code.

My code looks like this:

if ($data->sl1 === NULL){ $data->sl1 = "0";}
if ($data->sl2 === NULL){ $data->sl2 = "0";}
if ($data->sl3 === NULL){ $data->sl3 = "0";}
if ($data->sl4 === NULL){ $data->sl4 = "0";}
if ($data->sl5 === NULL){ $data->sl5 = "0";}
if ($data->sl6 === NULL){ $data->sl6 = "0";}
if ($data->sl7 === NULL){ $data->sl7 = "0";}
if ($data->sl8 === NULL){ $data->sl8 = "0";}
if ($data->sl9 === NULL){ $data->sl9 = "0";}
if ($data->sl10 === NULL){ $data->sl10 = "0";}
if ($data->sl11 === NULL){ $data->sl11 = "0";}
if ($data->sl12 === NULL){ $data->sl12 = "0";}

if ($data->sn1 === NULL){ $data->sn1 = "0";}
if ($data->sn2 === NULL){ $data->sn2 = "0";}
if ($data->sn3 === NULL){ $data->sn3 = "0";}
if ($data->sn4 === NULL){ $data->sn4 = "0";}
if ($data->sn5 === NULL){ $data->sn5 = "0";}
if ($data->sn6 === NULL){ $data->sn6 = "0";}
if ($data->sn7 === NULL){ $data->sn7 = "0";}
if ($data->sn8 === NULL){ $data->sn8 = "0";}
if ($data->sn9 === NULL){ $data->sn9 = "0";}
if ($data->sn10 === NULL){ $data->sn10 = "0";}
if ($data->sn11 === NULL){ $data->sn11 = "0";}
if ($data->sn12 === NULL){ $data->sn12 = "0";}

if ($data->sr1 === NULL){ $data->sr1 = "0";}
if ($data->sr2 === NULL){ $data->sr2 = "0";}
if ($data->sr3 === NULL){ $data->sr3 = "0";}
if ($data->sr4 === NULL){ $data->sr4 = "0";}
if ($data->sr5 === NULL){ $data->sr5 = "0";}
if ($data->sr6 === NULL){ $data->sr6 = "0";}
if ($data->sr7 === NULL){ $data->sr7 = "0";}
if ($data->sr8 === NULL){ $data->sr8 = "0";}
if ($data->sr9 === NULL){ $data->sr9 = "0";}
if ($data->sr10 === NULL){ $data->sr10 = "0";}
if ($data->sr11 === NULL){ $data->sr11 = "0";}
if ($data->sr12 === NULL){ $data->sr12 = "0";}

One way I know could work would simply be to iterate through the entire object with a simple foreach like this:

foreach ($data as $item) {
    if ($item === NULL){ $item = "0";}
}

But let's say I don't want to change every single null object to 0.  Do I have any other option?

I am using PHP in my example, but I am curious to hear answers for other languages as well.

In short, the CSV parser is using the wrong data types. Whenever you have contiguous variable names, it screams "use an array!" If you at all, possibly can, use an array. That allows you to operate on it much, much more naturally, it's more flexible, and it's way less prone to human error.

If you used an array, this would be much, much simpler:

foreach ($this->sl as $k => $sl) {
    $this->sl = ($sl === null) ? "0" : $sl;
}

Since you seemingly can't use an array, your best bet is probably to either process it into an array, or use some nasty variable-variable hackiness to pretend it's an array.

$sl = array();
for ($i = 1; isset($this->{'sl' . $i}; ++$i) {
    $sl[] = $this->{'sl' . $i};
}

Or if (for some reason) you don't want to use an array:

for ($i = 1; isset($this->{'sl' . $i}; ++$i) {
    $this->{'sl' . $i} = ($this->{'sl' . $i} === null) ? "0" : $this->{'sl' . $i};
}


Model for math facts I've written a model for an app that generates math facts kids have to solve by evaluating the operation and the difficulty you pass to it at initialization.

I don't like how I am repeating the code that performs the operations. For instance, performAddition() and performMultiplication() both perform the same thing. How can I make this model better?

import UIKit

class QuestionGenerator: NSObject {

    enum BinaryOperation {
        case Addition
        case Subtraction
        case Multiplication
        case Division
    }

    enum Difficulty {
        case Easy
        case Intermediate
        case Difficult
    }

    struct Settings {
        var binaryOperation: BinaryOperation
        var difficulty: Difficulty
    }

    private var brain: Settings
    var number1: Int = 0
    var number2: Int = 0
    var answer: Int = 0

    init(operation: BinaryOperation, difficulty: Difficulty) {
        brain = Settings(binaryOperation: operation, difficulty: difficulty)
        super.init()
    }

    func newQuestion() {
        switch brain.binaryOperation {
        case .Addition:
            performAddition()
            break
        case .Multiplication:
            performMultiplication()
            break
        default: break
            // operation not implemented yet
        }
    }

    private func performAddition() {
        switch brain.difficulty {
        case .Easy:
            number1 = Int.random(1...10)
            number2 = Int.random(1...10)
        case .Intermediate:
            number1 = Int.random(10...100)
            number2 = Int.random(10...100)
        case .Difficult:
            number1 = Int.random(109...999)
            number2 = Int.random(109...999)
        }
        answer = number1 + number2
    }

    private func performMultiplication() {
        switch brain.difficulty {
        case .Easy:
            number1 = Int.random(1...10)
            number2 = Int.random(2...4)
        case .Intermediate:
            number1 = Int.random(1...50)
            number2 = Int.random(3...7)
        case .Difficult:
            number1 = Int.random(10...100)
            number2 = Int.random(4...15)
        }
        answer = number1 * number2
    }
}

Disclaimer, I'm a beginner level Swift programmer, so take my feedback with a grain portion of salt.

First off, a minor improvement for your newQuestion function, you can leave out the breaks, in Swift, cases don't fall through by default:

func newQuestion() {
    switch brain.binaryOperation {
    case .Addition:
        performAddition()
    case .Multiplication:
        performMultiplication()
    default: break
        // operation not implemented yet
    }
}

Secondly, I see no reason you need to put binaryOperation and difficulty into a Settings struct. Furthermore, from what I can see, they should be final. I'd suggest you rewrite thing to:

private let binaryOperation: BinaryOperation
private let difficulty: Difficulty

...

init(operation: BinaryOperation, difficulty: Difficulty) {
    self.binaryOperation = operation
    self.difficulty = difficulty
    super.init()
}

Then, to generalise your perform functions, I'd add two functions to your BinaryOperation enum:

enum BinaryOperation {
    case Addition
    case Subtraction
    case Multiplication
    case Division

    func rangesForDifficulty(difficulty: Difficulty) -> (Range<Int>, Range<Int>) {
        switch self {
        case .Addition:
            switch difficulty {
            case .Easy: return (1...10, 1...10)
            case .Intermediate: return (10...100, 1...100)
            case .Difficult: return (109...999, 109...999)
            }
        case .Subtraction:
            switch difficulty {
            case .Easy: return (1...10, 1...10)
            case .Intermediate: return (10...100, 1...100)
            case .Difficult:  return (109...999, 109...999)
            }
        case .Multiplication:
            switch difficulty {
            case .Easy: return (1...10, 2...4)
            case .Intermediate: return (1...50, 3...7)
            case .Difficult: return (10...100, 4...15)
            }
        case .Division:
            switch difficulty {
            case .Easy: return (1...10, 2...4)
            case .Intermediate: return (1...50, 3...7)
            case .Difficult: return (10...100, 4...15)
            }
        }
    }

    func apply(number1: Int, _ number2: Int) -> Int {
        switch self {
        case .Addition:
            return number1 + number2
        case .Subtraction:
            return number1 - number2
        case .Multiplication:
            return number1 * number2
        case .Division:
            return number1 / number2
        }
    }
}

This allows you to write a single the newQuestion function much more elegantly:

func newQuestion() {
    let (range1, range2) = binaryOperation.rangesForDifficulty(difficulty)
    number1 = Int.random(range1)
    number2 = Int.random(range2)
    answer = binaryOperation.apply(number1, number2)
}

Some people might argue, that the BinaryOperation enum has gotten unwieldy now and they might be right. You could consider putting the rangesForDifficulty function somewhere else to alleviate this, I do think that the apply function should stay where it is though.


Check if strings are substrings of another string in Python My problem starts with a list A (length around n = 100) of big strings (around 10000 characters each). I also have another q = 10000 strings of length 100. I want to check if each string is a substring of any element of the list A.

I've tried to do this using inor any but it is still taking too much time as there are 10000 iterations and in each iteration I'm checking if s of length 100 is in str of length 10000.

n,q=[int(item) for item in input().split()]
desc=[]
for i in range(n):
    desc.append(input())
desc="\t".join(desc)
for j in range(q):
    quest=input().strip()
    if quest in desc:
        print("It's in !")
    else:
        print("It's not in ..")

Is there any better way to do this much faster?

Note: The numbers I'm expliciting are upper bounds not exact values of the lengths.

The problem of finding matches for multiple fixed search strings in a corpus is solved by the Aho–Corasick algorithm in time proportional to the length of the corpus plus the number of matches.

Python doesn't come with an implementation of the Aho–Corasick algorithm (as far as I know), but the Python Package Index has the pyahocorasick package. Or you could write your own.

Alternatively, if you are on a Unix system, you can use the -F (fixed strings) option to grep and avoid Python altogether.


Find maximum area of island in matrix I recently solved the problem below from leetcode:

  Given a non-empty 2D array grid of 0's and 1's, an island is a group of 1's (representing land) connected 4-directionally (horizontal or vertical).  Find the maximum area of an island in the given 2D array.

This is how I normally write python code. I am thinking this might not be really Pythonic. Please help review the code.

from operator import add

def max_area_of_island(grid):
    """
    :type grid: List[List[int]]
    :rtype: int
    """
    rlen = len(grid)
    clen = len(grid[0])

    visited = [[0] * clen for _ in range(rlen)]
    max_island = 0
    dirs = [(-1, 0), (1, 0), (0, -1), (0, 1)]

    def add_dir(cur_loc, d):
        cur_i, cur_j = cur_loc
        new_i, new_j = tuple(map(add, cur_loc, d))
        if new_i >= 0 and new_i < rlen and new_j >= 0 and new_j < clen:
            #print("all good")
            return new_i, new_j
        #print("error")
        return -1, -1

    max_area = 0
    for i in range(rlen):
        for j in range(clen):
            if grid[i][j] == 0 or visited[i][j]:
                continue
            area = 1
            q = [(i,j)]
            visited[i][j] = True
            #print("before qsize", q.qsize())
            while q:
                #print("during qsize", q.qsize())
                cur = q.pop()
                for _,d in enumerate(dirs):
                    new_i, new_j = add_dir(cur, d)
                    if new_i < 0 or visited[new_i][new_j]: continue
                    if new_i >= 0 and grid[new_i][new_j]:
                        new_loc = (new_i, new_j)
                        q.append(new_loc)
                        visited[new_i][new_j] = True
                        area += 1
            max_area = max(area, max_area)

    return max_area

Some suggestions:

Nested functions are unusual; typically they would be neighbour functions instead. This ensures that all the context that each function needs is passed to it, making it easier to grasp the entire context involved in the processing in each function.
Python 3 supports type annotations, which are a more explicit way of declaring input and output types. You can check that your code is properly annotated using mypy, for example with this rather strict configuration:

[mypy]
check_untyped_defs = true
disallow_untyped_defs = true
ignore_missing_imports = true
no_implicit_optional = true
warn_redundant_casts = true
warn_return_any = true
warn_unused_ignores = true

Longer variable names can make your code more readable. For example, I can't tell what q is without reading most of the code, and even then it might be unclear, especially since it's an "intermediate" variable (neither a parameter nor a return value).
You could use a set of constants to define the directions in a more human readable form such as DIRECTION_WEST = (-1, 0).
Inlining such as if foo: continue is generally frowned upon, since it makes it harder to skim the code vertically.
Your docstring could include a definition of the problem, possibly by simply copying from the LeetCode website.
Python has exceptions, and it's recommended to use them rather than special return values to signal a problem.


Testable and self-contained bash script I wrote a short bash script for a completion check of downloading images. If you have any good ideas, please tell me them. Thank you.
#!/bin/sh

# if a directory name is assigned

if [ $# -eq 1 ]; then

  a=`ls -1 $1/*.jpg | wc -l`
  b=`grep jpg $1/tmp.sh | wc -l`
  if [ $a = $b ]; then
    echo $1
  fi
  exit
fi

# if some directory names are assigned

if [ $# -gt 1 ]; then
  for dir1 in "$@"; do
    ./done.sh $dir1
  done
fi

# if directory names are not assigned

if [ $# -eq 0 ]; then
  ls -d */ | xargs ./done.sh
fi

Good

good indentation
somewhat helpful comments
good to do the ; then bit on the same line as the if

Suggestions

use double brackets for conditionals
try shellcheck (It will tell you to put quotes around your variable substitutions.)
put your arguments into named variables so you don't keep have to referring to $1
put the 3 sections into if...elif...else structure to make clear that you're only going to go into one of them
check for the existance of ./done.sh at the beginning
explain what the goal of the script is somewhere.


NSLog-like logging framework for iOS I am writing a logging framework for iOS. The idea is to create a behavior similar to NSLog to print the log message and some metadata to the console.

The following struct shall hold the data of a log entry:

public enum LogLevel: Int {

    case highlight = 4
    case debug = 3
    case info = 2
    case warning = 1
    case error = 0

}

public struct LogEntry {

    let date: Date
    let file: String
    let function: String
    let line: Int
    let logLevel: LogLevel
    let text: String

    var metaText: String {
        let dateFormatter = DateFormatter()
        dateFormatter.dateFormat = "HH:mm:ss"
        return "\(dateFormatter.string(from: self.date)): \((self.file as NSString).lastPathComponent):\(self.line) \(self.function): "
    }

}

The struct can then be printed with the following function (simplified):

public class QLog {

    static func log(_ logEntry: LogEntry) {
        print("\(logEntry.metaText)\(logEntry.text)")
    }

}

Of course I want to log from any point in my code like QLogDebug("Test"). So, I added the following global functions (I got the idea from https://github.com/goktugyil/QorumLogs):

public func QLogHighlight<T>(date: Date = Date(), file: String = #file, function: String = #function, line: Int = #line, _ object: T) {
    QLog.log(LogEntry(date: date, file: file, function: function, line: line, logLevel: LogLevel.highlight, text: "\(object)"))
}

public func QLogDebug<T>(date: Date = Date(), file: String = #file, function: String = #function, line: Int = #line, _ object: T) {
    QLog.log(LogEntry(date: date, file: file, function: function, line: line, logLevel: LogLevel.debug, text: "\(object)"))
}

public func QLogInfo<T>(date: Date = Date(), file: String = #file, function: String = #function, line: Int = #line, _ object: T) {
    QLog.log(LogEntry(date: date, file: file, function: function, line: line, logLevel: LogLevel.info, text: "\(object)"))
}

public func QLogWarning<T>(date: Date = Date(), file: String = #file, function: String = #function, line: Int = #line, _ object: T) {
    QLog.log(LogEntry(date: date, file: file, function: function, line: line, logLevel: LogLevel.warning, text: "\(object)"))
}

public func QLogError<T>(date: Date = Date(), file: String = #file, function: String = #function, line: Int = #line, _ object: T) {
    QLog.log(LogEntry(date: date, file: file, function: function, line: line, logLevel: LogLevel.error, text: "\(object)"))
}

Now, this is a bit ugly because the function signatures are very long as well as the initialization of the LogEntry object. Also, all five functions are the same except the function name and the log level.

So, are there any possibilities to shorten / simplify this code? Can I somehow shorten the signatures? Can I define the functions without copy-pasting them?

All your logging function take a Date as the first argument, with a default
value of the current date. Unless there is a special reason why a caller
should pass a custom date, you can remove that argument and compute the
date inside the logging function:

public func qLogHighlight<T>(file: String = #file, function: String = #function,
                             line: Int = #line, _ object: T) {
    QLog.log(LogEntry(date: Date(), file: file, function: function, line: line,
                      logLevel: .highlight, text: "\(object)"))
}

or even give the LogEntry constructor a default value for the date
parameter, so that you can omit it here:

public func qLogHighlight<T>(file: String = #file, function: String = #function,
                             line: Int = #line, _ object: T) {
    QLog.log(LogEntry(file: file, function: function, line: line,
                      logLevel: .highlight, text: "\(object)"))
}

Note also that

function names should start with a lowercase letter (compare General Conventions in the Swift API Design Guidelines),
it suffices to pass .highlight to the QLog.log() call instead
of LogLevel.hightlight, the type is automatically inferred from the
context.

If you want five separate logging functions, one for each level, then
I do not see more room for shortening. An alternative would be to
define just one logging function which takes the log level as an
argument:

public func qLog<T>(level: LogLevel, file: String = #file, function: String = #function,
                    line: Int = #line, _ object: T) {
    QLog.log(LogEntry(date: Date(), file: file, function: function, line: line,
                      logLevel: level, text: "\(object)"))
}

This would remove to code duplication completely.

Some more thoughts:

Creating a date formatter is “expensive,” it is better to create it once
and reuse it (compare e.g. the “Re-Using Formatter Instances” section in NSFormatter on NSHipster.

This can be done with a static property (which is computed lazily and
only once):

public struct LogEntry {

    static var formatter: DateFormatter = {
        let dateFormatter = DateFormatter()
        dateFormatter.dateFormat = "HH:mm:ss"
        return dateFormatter
    }()

    ...

}

Instead of bridging to NSString one can use

URL(fileURLWithPath: file).lastPathComponent

to get the last path component of a file path.

Apart from special cases (such as escaping closures), properties can
be accessed without using self:

var metaText: String {
    return "\(LogEntry.formatter.string(from: date)): \(URL(fileURLWithPath: file).lastPathComponent):\(line) \(function): "
}


Function that adds two items from a list in search of those that will match some given sum I'm doing a Daily Challenges to get better with Python.
I completed the first challenges with a working code but I want to make sure if there's anyway to improve or optimize it.

I have a function that adds 2 items from a list in search of those that will match the already defined result.

def main():

  listA = [10,11,3,7]
  k = 21

  for i in listA:
    for e in listA:
      result = i + e
      if result == k:
        print(i, '+', e, ' = ', k)
        return
main()

I was thinking of reducing it to:

def main():
  k = 21
  listA=[10,11,3,7]
  print (k,'=',[(i and e) for i in listA for e in listA if k==(i+e)])
main()

… but that would print 21 = [10,11] instead of 21 = 10 + 11.

def main():
    k = 21
    listA=[10,11,3,7]
    print (k,' = '," + ".join([str(i and e) for i in listA for e in listA if k==(i+e)]))

main()

This minor change to your code will do what you're after.  Put a " + ".join( ) around your list creator and cast the (i and e) items to str, that will then join all string elements in the list together into a string with + as the separator (there are spaces in there, yes, and this is necessary to get the output you're after).

Proof of Concept (via tio.run) that shows you how this will execute.

Note I also made a change to add spaces around the equals sign, indentation, and extra spacing for readability, as well as indentation standardization (4 spaces for a single indentation level).


is_a? vs respond_to? I am working on some code which checks whether or not to unlink a file on the basis of the class:

File.unlink(file.path) if file.is_a? File

I think it would be more idiomatic to check whether the object responds to the unlink message:

File.unlink(file.path) if file.respond_to? :unlink

Is there any good reason to prefer the is_a? approach?

Given that on some file systems a directory is simply a File with special attributes (or to put it another way a specialized file), does it really matter?  Unlink it if it can be unlinked if that is what you want to do.  If it can't be unlinked, why would you bother?  And why would you care what it is?  If you care what it is, then use is_a?  If the inode can be unlinked, unlink it.


Speeding up naive backtracking Sudoku Solver in Haskell After watching a recent computerphile video on building a very simple sudoku solver I tried to implement the same in Haskell. From this CR question I learned that it is probably a better idea to use Vectors instead of just lists to represent a grid of numbers that is gonna be mutated. (But it is supposedly still worse than using a sparse representation.) And from this one (and another question of mine) I learned about Control.Lens, but I decided against using it to avoid using many different packages that I'm not familiar with.

Now the program I wrote is close to the original in python, but very slow. So I would like to get some feedback on how to speed it up without deviating form this very simple aproach too much.

The Program

The code defines a Board that represents a (solved or unsolved) state, with zeros for the entries that area yet to be determined. It can be indexed using Coordinates. Then there are a few setters and getters that probably could be replaced by using Control.Lens - but I'd like to avoid that for now as I just want to focus on the performance. Then there is a possible function which takes a Board, Coordinates and a candidate number an just reports whether it is possible to put the candidate at some given coordinates. Finally there is solve that does the backtracking.

So far I tried to add a take 1 $ or take 1 $! to speed it up (but only returning at most a single solution), but without success.

--https://www.youtube.com/watch?v=G_UYXzGuqvM
import qualified Data.Vector as V

data Board = Board (V.Vector (V.Vector Integer))
type Coordinates = (Int, Int)

instance Show Board where
 show (Board b)=unlines . V.toList $ V.map show b

fromList :: [[Integer]] -> Board
fromList l = Board $ V.fromList $ V.fromList <$> l

-- a few setters and getters
(!):: Board -> Coordinates -> Integer
(Board b) ! (i,j) = (b  V.! j)V.! i

getColumn :: Board -> Coordinates -> [Integer]
getColumn b (i, _) = [b ! (i, j) | j<-[0..8]]

getRow :: Board -> (Int, Int) -> [Integer]
getRow b (_, j) = [b ! (i, j) | i<-[0..8]]

getSquare :: Board -> Coordinates -> [Integer]
getSquare b (i, j) = [b ! (i'*3 + u, j'*3 + v) | u<-[0..2],v<-[0..2]]
 where i' = i `div` 3
       j' = j `div` 3

insert :: Board -> Coordinates -> Integer -> Board
insert (Board b) (i, j) k = Board b'
 where v = b V.! j
       v' = v V.// [(i, k)]
       b' = b V.// [(j, v')]

-- check whether it is possible to insert candidate at given position
possible :: Board -> Coordinates -> Integer -> Bool
possible b coords@(i, j) k
 |i < 0 || i >= 9 || j < 0 || j >= 9 || k < 0 || k > 9 = undefined
 |b ! coords > 0 = False
 |k `elem` getRow b coords = False
 |k `elem` getColumn b coords = False
 |k `elem` getSquare b coords = False
 |otherwise = True

-- check whether board is already full
full :: Board -> Bool
full b = 0 `notElem` [b ! (i,j) | i<-[0..8], j<-[0..8]]

-- recursion to find all solutions to a given board
solve :: Board -> [Board]
solve b
 |full b = [b]
 |otherwise =  concat[ solve $! (insert b (x, y) n)|
                       x<-[0..8],
                       y<-[0..8],
                       n<-[1..9],
                       possible b (x,y) n]

main = print $ solve b

-- normal sudoku
b :: Board
b = fromList [
 [5,3,0,0,7,0,0,0,0],
 [6,0,0,1,9,5,0,0,0],
 [0,9,8,0,0,0,0,6,0],
 [8,0,0,0,6,0,0,0,3],
 [4,0,0,8,0,3,0,0,1],
 [7,0,0,0,2,0,0,0,6],
 [0,6,0,0,0,0,2,8,0],
 [0,0,0,4,1,9,0,0,5],
 [0,0,0,0,8,0,0,7,9]
 ]

-- only one a few entries are missing
c :: Board
c = fromList [
 [0,0,0,0,9,4,5,6,1],
 [9,2,1,5,3,6,8,7,4],
 [4,5,6,7,8,1,9,2,3],
 [1,4,7,3,5,9,2,8,6],
 [2,8,3,6,1,7,4,5,9],
 [5,6,9,8,4,2,3,1,7],
 [6,7,4,9,2,5,1,3,8],
 [8,9,2,1,7,3,6,4,5],
 [3,1,5,4,6,8,7,9,2]]

Try it online!

It turns out I made some crucial mistakes: The iteration in solve can stop as soon we have tried all possibilities for one coordinate (x,y) that is not set yet. If we iterate further over (x,y) we get duplicate solutions which makes things blow up, so with following little modification we get results immediately:

isEmpty :: Board -> Coordinates -> Bool
isEmpty b coords = b ! coords == 0

-- check whether board is already full
full :: Board -> Bool
full b = 0 `notElem` [b ! (i,j) | i<-[0..8], j<-[0..8]]

-- recursion to find all solutions to a given board
solve :: Board -> [Board]
solve b
 |full b = [b]
 |otherwise =  concat[ solve $! (insert b (x, y) n)|
                       (x,y) <- take 1 empty,
                       n <- [1..9],
                       possible b (x,y) n]
   where empty = [(x,y) | x<-[0..8],y<-[0..8], isEmpty b (x, y)]

Try it online!


Finding the first 10 million primes I'm trying to find the first 100 million primes and am trying to find the most efficient way to do it. Can I get some advice on how I could improve this?

public class Main {

    public static void main(String[] args) {

        int counter = 0;
        int num = 3;
        int index = 1;
        int[] primes = new int[100000000];
        primes[0] = 2;

        while(counter < 99999999){
            if(isPrime(num)) {
                primes[index] = num;
                index++;
                counter++;
            }
            num+=2;
        }
        index = 1;
        for (int i : primes) {
            System.out.println(String.format("%d: %d", index, i));
            index++;
        }
    }

    private static boolean isPrime(int num){
        if (num < 2) return false;
        else if (num == 2) return true;
        for (int i = 2; i < num; i++) {
            if (num % i == 0) return false;
        }
        return true;
    }
}

Readability

        int counter = 0;
        int num = 3;
        int index = 1;
        int[] primes = new int[100000000];
        primes[0] = 2;

I find this order confusing.  Consider

        // 2 is the first prime and the only one that is even, so process it separately
        int[] primes = new int[100000000];
        primes[0] = 2;
        int index = 1;

        // 3 is the next prime after 2; i.e. the first odd prime
        int candidate = 3;

Oops, did I forget something?  What happened to counter?  I took it out entirely as unnecessary.  As it was only used in the loop, if I had included it, it would have looked like

        for (int counter = 1; counter < primes.length; ) {

And I would have replaced the while loop as well.  But I actually think that it's easier to just remove counter and use index instead.  More on that later.

By breaking the code up into clumps and adding comments, it is easier to see how the lines of code relate to each other.  It was not obvious to me on first read that index indexed primes.  Now that is much clearer.  And it is clearer why it starts at 1 rather than 0.

I prefer the name candidate to num.  It is clearer about what is being tracked there.

Robustness

        while(counter < 99999999){

Why 99999999?  After staring at it for a bit, I realized that this is because counter is only counting the numbers after 2.  Why?  I would find it easier to say and understand

    while (index < primes.length) {

First I tried it with counter set to 1 initially.  But then I realized that that made counter an exact duplicate of index.  So we can ditch counter entirely.

For either, it is more robust to limit by primes.length.  In the original code, if you changed the length of primes, you had to manually change the loop definition to match.  This way, it will match automatically.

I also find it clearer to limit based on index, as that is what is actually problematic.

Delegate

Consider replacing the entire body of the main method with two method calls.

        int[] primes = findPrimes(100000000);
        displayPrimes(primes);

Now the main method itself does just one thing, call two methods.  Each method can do just one thing:

Set the primes array.
Display the primes.

Even better might be to add a Primes class.

        Primes primes = new Primes(100000000);
        primes.initialize();
        primes.display();

This also offers some efficiency optimizations that static methods don't.  More on that later.

Optimization

But your real question was optimization, so let's think about that.

All your candidates are going to be odd numbers.  You already took advantage of the fact that 2 is the only even prime.  You can take advantage of that again by rewriting isPrime to

    public static boolean isPrime(int candidate) {
        // 2 is the only even prime
        if (candidate % 2 == 0) {
            return candidate == 2;
        }

        if (candidate < 2) {
            return false;
        }

        return isOddPrime(candidate);
    }

And then

    // only returns useful results for odd numbers equal to 3 or more.
    private static boolean isOddPrime(int candidate) {
        for (int factor = 3; factor <= candidate / factor; factor += 2) {
            if (candidate % factor == 0) {
                return false;
            }
        }

        return true;
    }

How does this help?  Notice that the situations checked in isPrime will never return true for your candidates.  The smallest one is 3, so never less than or equal to 2.  You increment by 2, so never even.  So it will always drop through to the isOddPrime check.  Therefore, we can replace the isPrime call with an isOddPrime with no change in results.

And isOddPrime is more efficient.

It saves a modulus operation and two comparisons.

This should be obvious.  Those are in isPrime prior to the call to isOddPrime.  There's a third comparison that never runs on these candidates anyway, so we won't count that.
It only checks every other number, as it can skip all the even factors.

It uses the same start at 3 and increment by 2 logic as the main method.
It only checks factors up to the square root of the candidate.

This is probably the least obvious.  But factor <= candidate / factor is the equivalent of factor * factor <= candidate or factor <= sqrt(candidate) for all positive values of factor.

We can do this safely because at least one of any pair of factors must be less than or equal to the square root.  Otherwise their product will be greater than the number.

This won't matter much at the beginning, but there is a big difference between checking every number up to a thousand and every number up to a million.

Alternative

Instead of creating a new candidate factors list each time, use your primes instead.  This works best if you have a separate primes' class with something like

    int[] primes;

    public PrimesList(int size) {
        primes = new int[size];
    }

Now primes is available to every non-static method in the class.  So your isOddPrime could become

    private boolean isOddPrime(int candidate) {
        for (int prime : primes) {
            if (prime > candidate / prime) {
                return true;
            }

            if (candidate % prime == 0) {
                return false;
            }
        }

        return true;
    }

This saves checking non-prime odd numbers like 9, 15, 21, etc.  Those will never work because you already checked their factors.  E.g. for 35, you already checked that it's not divisible by 5 and 7, so you already know that 35 won't work.

Further

You optimized out even numbers.  It's also possible to optimize out candidates divisible by three.  Instead of incrementing by 2 every time, you can increment by 2 and then 4 and repeat that cycle.  So you'd check 5, 7, skip 9, 11, 13, skip 15, etc.   I won't go into this here, as it probably doesn't add much performance.  But here is an example that uses this optimization.

Sieve

The Sieve of Eratosthenes is another possibility, but sieves don't do so well with limits on the number of primes.  They are designed to be limited by the candidate pool.  So you might find yourself needing to implement a sieve with a rolling segment.  That's extra work, so you might prefer to avoid it.

An example of a windowed sieve that you could modify to work by segments.


Calculate number of co-occurrences for two vectors I would like to understand how to get rid of all the loops I am using in this code.

I have a dataframe of 20 rows. I am simply counting the number of occurrences that the variables rs and occ appear together.

Here is the data:

 d = structure(list(householdid.x = c("101366", "101366", "102481",
                             "102481", "103755", "103755", "103788", "103788", "103799", "103799",
                             "10422", "10422", "10429", "10429", "10433", "10433", "10499",
                             "10499", "105280", "105280"), nhouse = c(4L, 4L, 4L, 4L, 4L,
                                                                      4L, 4L, 4L, 4L, 4L, 4L, 4L, 4L, 4L, 4L, 4L, 4L, 4L, 4L, 4L),
           idno = c(1013661, 1013662, 1024812, 1024811, 1037552, 1037551,
                    1037881, 1037882, 1037991, 1037992, 104222, 104221, 104291,
                    104292, 104331, 104332, 104992, 104991, 1052802, 1052801),
           rs = c(0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0,
                  0, 0, 1), occ = c(0, 0, 0, 0, 0, 0, 1, 0, 1, 1, 0, 0, 0,
                                    0, 0, 0, 0, 0, 1, 0)), class = "data.frame", .Names = c("householdid.x",
                                                                                "nhouse", "idno", "rs", "occ"), row.names = c(NA, -20L))

Here are my loops - slow and cumbersome:

countcouples = list()
countcouples$both <- 0
countcouples$unique <- 0
countcouples$zero <- 0

for(i in 1:nrow(d)){
  if(d$rs[i] == 1 & d$occ[i] == 1)
  {countcouples$both <- countcouples$both+1}

  if((d$rs[i] != 1 & d$occ[i] == 1) | (d$rs[i] == 1 & d$occ[i] != 1))
  {countcouples$unique <- countcouples$unique+1}

  if(d$rs[i] == 0 & d$occ[i] == 0)
  {countcouples$zero <- countcouples$zero+1}

}

Here are the results:

  both unique zero
    2      4   14

Any advice?

sum(d$rs & d$occ)
#[1] 2

The goal is "...simply counting the number of occurrences that the variables rs and occ appear together." If you feel that your code is long and cumbersome, the vectorization process of R is uniquely capable of speeding up the task without for loops and nested conditionals.

As mentioned in the previous answer, R coerces certain classes onto vectors when specific functions are called on them. The two columns in question are both a series of 1's and 0's. They are numeric values. I would like them to be thought of as TRUE's and FALSE's to evaluate my logical condition. Thanks to the brevity of R code I do not have to literally ask R to do that. As soon as I use the & operator, R will immediately treat all 0's as FALSE and other numbers as TRUE logical values.

Finally, sum does a similar action but in the opposite direction. It coerces (forces the class) to numeric in order to calculate the sum. It treats TRUE as 1 and 0 as FALSE. In the case of this code, Many complicated relationships are being handled by the R computing engine and, thankfully, not by us.

If you would like the other two values for unique and zero use:

sum(xor(d$rs, d$occ))
[1] 4
sum(!d$rs & !d$occ)
[1] 14

The function xor tests what is called the "exclusive or". So the code read aloud would be "How many pairs have a one in rs or occ, but not in both." The operator ! negates what comes after it. That code would read, "How many pairs have a value that is not 1 in rs and not 1 in occ."


Ping and write the status of a timer on a GUI thread I am running a timer for updating some of my operations, such as buffers. But I also want ping to be added.

It is added, but I think it can be improved.  This is because I think it's running on the GUI thread, which is not preferred as it will freeze the GUI if something goes wrong with it.  I think such a thing has occurred, and I haven't been able to locate the problem until now.

But, I am not asking for the solution for that problem, as there are other stuff.  I am basically asking for ways to improve this piece of code as it doesn't seem to be optimal in any way.

private void timer1_Tick(object sender, EventArgs e)
{
    try
    {
        if (waveProvider.BufferedDuration.Milliseconds > 40 && AudioDevices.SelectedItem.ToString() != "Wasapi Loopback")
        {
            waveProvider.ClearBuffer();
            TimesBufferClear++;
        }
        currping.Text = "Current Buffer: " + waveProvider.BufferedDuration.Milliseconds.ToString() + " Clear: " + TimesBufferClear.ToString() + " Ping: " + pingSender.Send(otherPartyIP.Address).RoundtripTime.ToString() + " Buffer: " + SendStream.BufferMilliseconds;
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message + "Timer");
    }
}

As you can see, I have a label which contains all the things I want to update.  It is fine, and I'd rather have one than many in this case.

But is this a good way to do it?  It doesn't seem that good when I look at it; it's just a mess, even if it works.

Also, the PingSender is created and disposed of outside of the thread.  I suppose that is better than just "using" it every timer run.

Are there any ideas of what can be done to improve it?

As it is not really much code, let us just focus on the label part and a little naming.

TimesBufferClear++;

If this is a variable, we should use camelCase casing for the name. If it is a property it follows the naming convention.

currping

Can be renamed to currentPingStatus, which is more meaningful.

Now let us refactor the assignment to the Text property of this label:

String formatPattern = "Current Buffer: {0} Clear: {1} Ping: {2} Buffer: {3}";

currentPingStatus.Text = String.Format(formatPattern,
         waveProvider.BufferedDuration.Milliseconds.ToString(),
         TimesBufferClear.ToString(),
         pingSender.Send(otherPartyIP.Address).RoundtripTime.ToString(),
         SendStream.BufferMilliseconds);


Sum Arithmetic Progression in Haskell I am learning Haskell and as exercise I am doing Project Euler Problems. In this case PE1.

-- Sum any given finite Airthmetic Progression
sumAP :: Integer -> Integer -> Integer -> Integer
sumAP first limit step = do
  let last  = limit - (limit `rem` step)
  let terms = (last - first) `quot` step + 1
  let avg   = first + last
  terms * avg `quot` 2

Now, off the bat I am shadowing the last function. How could I rename this variable along with first?

Is there any way I can improve readability? Any edge case or fail case I haven't covered? Any performance fail?

My first comment would be to comment your code.

You need documentation so I know what your function is meant to do. "Sum any given finite Arithmetic Progression" would suffice if you passed in an ArithmeticProgression, but instead you pass in some bounds. Writing an arithmetic progression type would be simple and might even help a little, but you still need to document that.

I don't know what algorithm you're trying to use. You've evidently taken some maths for sums of arithmetic progressions and applied it to the problem, but there's no way for me to verify that the maths is correct or that the code implements it correctly without at least rudimentary explanation of what you did. I could reimplement it myself, but code should be verifiable much more easily than that.


Gaining better performance when screen scraping is required I have a website on which for every request, the scraping of other websites is done to get accurate data. While it works well, it has a slight impact on the performance.

public ActionResult Index(int id)
{
    Product product = pe.Products.Where(p => p.Id == id).First();
    foreach (var pricing in product.Retailer_Product_Prices)
    {
        switch (pricing.RetailerId)
        {
            case 1:
                pricing.Price = PriceCompareStatic.GetFlipKart(pricing.Url);
                if (pricing.Url.Contains("?"))
                    pricing.Url = pricing.Url + "&affid=pankajupad";
                else
                    pricing.Url = pricing.Url + "?affid=pankajupad";
                break;
            case 3:
                pricing.Price = PriceCompareStatic.GetHomeShop(pricing.Url, "//span[@class='pdp_details_hs18Price']");
                break;
            .
            .
            .  // Removed for brevity
            default:
                break;
        }
        product.BasePrice = product.Retailer_Product_Prices.Min(p => p.Price);
    }
    return View(product);
}

PriceCompareStatic

public static decimal GetFlipKart(string url)
{
    var baseUrl = new Uri(url);
    HtmlAgilityPack.HtmlDocument document = new HtmlDocument();
    try
    {
        WebClient client = new WebClient();
        document.Load(client.OpenRead(baseUrl));
        var div = document.DocumentNode.SelectNodes("//meta[@itemprop='price']").FirstOrDefault();
        HtmlAttribute att = div.Attributes["content"];
        string modPrice = att.Value.Replace("Rs. ", "");
        return Convert.ToDecimal(modPrice);
    }
    catch (Exception)
    {
        return 123456789;
    }
}

public static decimal GetSpanFromWebSite(string url, string identification)
{
    var baseUrl = new Uri(url);
    HtmlAgilityPack.HtmlDocument document = new HtmlDocument();
    try
    {
        WebClient client = new WebClient();
        document.Load(client.OpenRead(baseUrl));
        var div = document.DocumentNode.SelectNodes(identification).FirstOrDefault();
        return Convert.ToDecimal(div.InnerHtml);
    }
    catch (Exception)
    {
        return 123456789;
    }
}
.
.
. // Removed for brevity

Do you think shifting from shared hosting to ones own server will help with such requirements?

It's hard to tell exactly how it's being utilized since your code sample is an example.  However, a few notes.

Due to the design of your use case above most of the time any improvements are going to pale in comparison to pulling a web page live (especially multiple).  If you want real improvement, cache the information from the other websites.

Regarding the Code sample above, the only item I would recommend looking into is the parallel.foreach statement.  If you utilize it, make sure you've already pulled all the data from the database to loop through (not waiting on EF lazy loading).  Otherwise you will run into some EF threading issues.

I would also recommend creating static delegate methods for each website provider and when your application loads put the methods into a concurrent dictionary.  It will help to clean up you code to make it more readable.


Controller with TemplateService I am working on my template web api project.

I just wanna hear any suggestion how to improve this code. I heard that repository pattern is anti pattern so I want to make code without it. Any opinion will be great :D

Service

public class TemplateService : ITemplateService
{
    private readonly ContextTemplate context;

    public TemplateService(ContextTemplate context)
    {
        this.context = context;
    }

    public int AddTemplate(TemplateInput input)
    {
        var template = new TemplateModel()
        {
            Name = input.Name,
            CreatedDate = DateTime.UtcNow
        };

        context.TemplateModel.Add(template);
        context.SaveChanges();
        return template.Id;
    }

    public int UpdateTemplate(TemplateInput input)
    {
        var template = GetTemplate(input.Id);
        if (template != null)  {
            template.Name = input.Name;
            template.ModifiedDate = DateTime.UtcNow;
            context.Update(template);
            context.SaveChanges();
        }
        return (int)template?.Id;
    }

    public bool ArchiveTemplate(int templateId)
    {
        var isArchiveSuccessfull = false;
        var template = GetTemplate(templateId);
        if (template != null)
        {
            template.IsArchived = true;
            isArchiveSuccessfull = true;
            context.Update(template);
            context.SaveChanges();
        }
        return isArchiveSuccessfull;
    }

    public IEnumerable<TemplateModelDto> GetTemplates()
    {
        return Mapper.Map<List<TemplateModel>, List<TemplateModelDto>>(this.context.TemplateModel.Where(x=>!x.IsArchived).ToList());
    }

    public TemplateModel GetTemplate(int templateId)
    {
        var template = context.TemplateModel.FirstOrDefault(x => x.Id == templateId);
        return template;
    }

Controller

 [Route("api/[controller]")]
 public class TemplatesController : Controller
 {
    private readonly ITemplateService templateService;
    private readonly ILogger logger;

    public TemplatesController(ITemplateService templateService, ILogger<TemplatesController> logger)
    {
        this.templateService = templateService;
        this.logger = logger;
    }
    // GET api/templates
    [HttpGet]
    public IEnumerable<TemplateModelDto> Get() => templateService.GetTemplates();

    // GET api/templates/5
    [HttpGet("{templateId}")]
    public TemplateModel GetTemplate(int templateId) =>  templateService.GetTemplate(templateId);

    // POST api/templates
    [HttpPost]
    public int Post([FromBody]TemplateInput input) => templateService.AddTemplate(input);

    // PUT api/templates/5
    [HttpPut("{templateId}")]
    public int Put(int templateId, [FromBody]TemplateInput input) => templateService.UpdateTemplate(input);

    // DELETE api/templates/5
    [HttpDelete("{templateId}")]
    public Boolean ArchiveTemplate(int templateId) => templateService.ArchiveTemplate(templateId);
}

You have abstracted away any concrete dependencies from the controller. I  say that is well done for decoupling the controller and would allow better testability of the controller.

ContextTemplate is assumed to be DbContext derived, which already implements Unit of Work and Repository pattern so there is really no use in re-implementing the pattern.

IMO I see no immediate problems with the above code apart from the non use of the explicitly injected ILogger in the controller, which can be removed if there is no use for it in the controller. Logging can be considered a cross-cutting concern in asp.net-core and handled external to the controller.


Read from csv file, transform to dictionnary First of all, here's the code I'm working on:

def get_data(filename, cols):
    with codecs.open(filename, 'r') as csvFile:
        for row in csv.reader(csvFile):
            # build data dictionnary
            data = {}
            for i in range(len(cols)):
                key = cols[i]

                # update
                value = row[i].strip()

                # because some columns are splited in the csv file
                if key in data:
                    data[key] += value
                else:
                    data[key] = value

            # yield data, for each row
            yield data

So, depending of the size of the file, the time to process can be very long.

I tried testing csv.reader with custom dictionnary construction VS basic csv. DictReader, and for some reason, the first one is faster...

I have some long csv files with rows like this one:

"152Q694     ","892-000357          ",       0,       0,"        "

In some files, I have this (see the product name split in two columns?):

"A","COMPANY NAME         ","1234","987654321     ","I AM A PRODUCT NAME     ","WITH SOME EXTRA INFO          ","AB    ","12345               ",0000000000000001.23,0000000000000003.45,"A","            ","Z","01234567891234058","1234","EN",000000.01,"ABC","D","        ","        ",000000,000000,"            "

All in all, there will always be n rows and x columns, so the time of the process is a multiple of n*x.  Am I wrong?

What can I do to speed things up?

I have found a great piece of code to decode/encode on the fly:
https://stackoverflow.com/a/9177937/2659902

So, I wrote this class, based on that:

class CSVRecoder(object):
    def __init__(self, filename, decoder, encoder='utf-8', eol='\r\n',
                 dialect='excel', **fmtparams):
        """
            filename: to open the file object
            decoder, encoder, eol: to instanciate the recoder object
            dialect, **fmtparams: are passed to the csv.reader function
        """
        self.filename = filename
        self.decoder = decoder
        self.encoder = encoder
        self.eol = eol
        self.dialect = dialect
        self.fmtparams = fmtparams

    def __enter__(self):
        self.f = open(self.filename, 'rb')
        sr = Recoder(self.f, self.decoder, self.encoder, self.eol)
        return csv.reader(sr, self.dialect, **self.fmtparams)

    def __exit__(self, type, value, traceback):
        self.f.close()

def reader(*args, **kwargs):
    with CSVRecoder(*args, **kwargs) as csv_recoder:
        for row in csv_recoder:
            yield row

Now, I can do things like:

def get_data(fname, cols, encoding):
    def foo(row):
        data = {}
        get = data.get
        for i in xrange(len(cols)):
            key = cols[i]
            data[key] = get(key, '') + row[i].strip()
        yield data
    res = list(map(foo, csv_recoder.reader(fname, encoding)))

Perhaps I can optimise the dictionnary construction a bit more, but I doubt it would make a big difference (performance wise). Thanks anyway. Feel free to comment if you think I missed something somewhere, I'm always happy to learn ! :)