Content aggregator using bs4 and requests Standard code review, tell me what's good, what's bad, and how to improve. Critical suggestions welcomed. This is a content aggregator using bs4 and requests. I did not use any tutorials or help.
import requests
from bs4 import BeautifulSoup as bs

topic = input('Enter the topic: ').lower()
print()
def getdata(url, headers):
    r = requests.get(url, headers=headers)
    return r.text

def linesplit():
    print('-~'*16, 'COLIN IS MOOCH', '-~'*16, '\n')
headers = {
    'User-agent':
    "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36 Edge/18.19582"
}
google = getdata(f'https://www.google.com/search?q={topic}&oq={topic}&aqs=chrome..69i59j69i57j69i59j69i60l3j69i65j69i60.2666j0j7&sourceid=chrome&ie=UTF-8', headers)
soup = bs(google, 'html.parser')
links = str(soup.find_all('div', class_='TbwUpd NJjxre'))
links = links.replace('
', '')
links = links.replace('', '')
links = links.replace('
', '')
links = links.replace('
', '')
links = links.replace('
', '')
links = links.replace('', '')
links = links.replace(' › ', '/')
links = links.split(', ')
links[-1] = links[-1].replace(']', '')
links[0] = links[0].replace('[', '')
info = []
counter = 0

for x in range(len(links)):
    try:
        htmldata = getdata(links[x], headers)
        newsoup = bs(htmldata, 'html.parser')
        website = ''
        for i in newsoup.find_all('p'):
            website = website + ' ' + i.text
        info.append(links[x])
        info.append(website)
        counter += 1
    except Exception:
        continue
try:
    for x in range(0, (counter * 2) + 2, 2):
        if info[x+1] != '':
            linesplit()
            print()
            print('From ', info[x], ':')
            print()
            print(info[x+1])
            linesplit()
except IndexError:
    pass

Your linesplit is... strange? I'm going to ignore that message and pretend it makes sense.
Whenever there's an API, prefer it. In this case Google has a Custom Search API that allows you to skip past all of the scraping insanity. You'll need to get an API key and configure an engine instance to search the whole web. The front-end of such an application, without including your second "scrape all of the paragraphs" step, looks like:
from typing import Iterable
from requests import Session

# To set a custom search engine to search the entire web, read
# https://support.google.com/programmable-search/answer/4513886
API_KEY = '...'
ENGINE_ID = '...'

def api_get(session: Session, query: str) -> Iterable[str]:
    with session.get(
        'https://customsearch.googleapis.com/customsearch/v1',
        headers={'Accept': 'application/json'},
        params={
            'key': API_KEY,
            'cx': ENGINE_ID,
            'q': query,
        }
    ) as resp:
        resp.raise_for_status()
        body = resp.json()

    for item in body['items']:
        yield item['link']

def main():
    topic = input('Enter the topic: ').lower()

    with Session() as session:
        urls = api_get(session, topic)

    print('\n'.join(urls))

if __name__ == '__main__':
    main()


React component to list TV shows from a JSON result I'm trying to rewrite my old project in Reactjs. As I am a beginner and this is my first app, I need some assistance in understanding what is the best way to do this in the React way and how I can improve myself and this demo project.

Here is a working demo of it.

ListManager.js

import React, { Component } from 'react'
import List from './List'

export class ListManager extends Component {
    constructor(props, context) {
       super(props, context)
       this.state = { finaldata :[]}
}

APIres(ids){
    Promise.all(ids.map(id =>
        fetch('http://api.tvmaze.com/shows/'+id).then(resp => resp.json().then(data => data.name))
        )).then(data => {
            data.sort()
            this.setState({finaldata:data})
        })
}

componentWillMount(){
    const id = [1,2,3,4,5]
    this.APIres(id)

}

render() {

    return (

{this.props.title}

            <List data={this.state.finaldata} />

    )
  }
}

List.js

'use strict'
import React, { Component } from 'react'

const ListItem = (props) =>
{ props.text }

const List = (props) => {
    const listNodes = props.data.map((
    value,index) => {
        return(
            <ListItem text={value} key={index}></ListItem>
        )
    })

    return (
{listNodes}

    )
}

export default List

Some note: id array will come with a URL query like example.com/#/?id=1,2,3,4. I will be using react-router for this.

A few things to change. Move the api request to componentDidMount because componentWillMount is considered legacy now. Rename the method thats making the api request to be more declarative which is to be more descriptive of whats its doing. Move the list of ids into the method thats making the api request. All of those changes + the changes to the api request from your question here https://stackoverflow.com/questions/39473302/show-result-from-parallel-ajax-request-in-react-component/39486654#39486654 would look like this:

import React, { Component } from "react";
import List from "./List";

class ListManager extends Component {
  constructor(props) {
    super(props);

    this.state = {
      listData: []
    };

    this.getListData = this.getListData.bind(this);
  }

  componentDidMount() {
    this.getListData();
  }

  getListData() {
    const ids = [1, 2, 3, 4, 5];

    let listData = [];

    ids.forEach(id =>
      fetch("http://api.tvmaze.com/shows/" + id)
        .then(resp => resp.json())
        .then(show => {
          listData.push(show);
        })
    );

    this.setState({
      listData: listData.sort((a, b) => a.runtime - b.runtime)
    });
  }

  render() {
    return (

{this.props.title}

        <List data={this.state.listData} />

    );
  }
}

export default ListManager;


Singly-linked implementation in C I'm a C++ programmer and thought I'd revisit the roots, so I scribbled something really quickly. I would appreciate input on how my C code is looking!

slist.h

#ifndef SLIST_H
#define SLIST_H

#include <stddef.h>
#include <stdbool.h>

struct slist_s
{
    int key;
    struct slist_s* next;
};

typedef struct slist_s slist_t;

slist_t* slist_push_front(slist_t*, int);
slist_t* slist_push_back(slist_t*, int);

slist_t* slist_pop_front(slist_t*);
slist_t* slist_pop_back(slist_t*);

_Bool    slist_empty(const slist_t* const);
slist_t* slist_clear(slist_t*);

size_t   slist_size(const slist_t*);

void     slist_print(const slist_t*, const char*);

slist_t* slist_for_each(slist_t* const, void(*)(int*));

#endif

slist.c

#include <stdlib.h>
#include <stdio.h>
#include <stddef.h>
#include <stdbool.h>

#include "slist.h"

slist_t* slist_push_front(slist_t* root, int val)
{
    slist_t* node = malloc(sizeof(slist_t));
    if (!node) {
        return NULL;
    }
    node->key = val;
    node->next = !root ? NULL : root;

    root = node;
    return root;
}

slist_t* slist_push_back(slist_t* root, int val)
{
    slist_t* node = malloc(sizeof(slist_t));
    if (!node) {
        return NULL;
    }
    node->key = val;

    if (!root)
    {   /* list is empty */
        node->next = NULL;
        root = node;
    }
    else
    {   /* need to walk to end of list */
        slist_t* temp = root;
        while (temp->next) {
            temp = temp->next;
        }
        node->next = NULL;
        temp->next = node;
    }
    return root;
}

slist_t* slist_pop_front(slist_t* root)
{
    if (!root) {
        return NULL;
    }
    if (!root->next)
    {
        free(root);
        return NULL;
    }
    slist_t* todel = root;
    root = root->next;
    free(todel);
    return root;
}

slist_t* slist_pop_back(slist_t* root)
{
    if (!root) {
        return NULL;
    }
    if (!root->next)
    {
        free(root);
        return NULL;
    }
    slist_t* todel = root;
    while (todel->next->next) {
        todel = todel->next;
    }
    free(todel->next);
    todel->next = NULL;
    return root;
}

_Bool slist_empty(const slist_t* const root)
{
    return root;
}

slist_t* slist_clear(slist_t* root)
{
    while (root) {
        root = slist_pop_front(root);
    }
    return root;
}

size_t slist_size(const slist_t* root)
{
    size_t size = 0;
    const slist_t* temp = root;
    while (temp)
    {
        temp = temp->next;
        ++size;
    }
    return size;
}

void slist_print(const slist_t* root, const char* format)
{
    const slist_t* temp = root;
    while (temp)
    {
        printf(format, temp->key);
        temp = temp->next;
    }
}

slist_t* slist_for_each(slist_t* const root, void(*fun)(int*))
{
    slist_t* temp = root;
    while (temp)
    {
        fun(&temp->key);
        temp = temp->next;
    }
    return root;
}

Returning root

Returning the root suggests the use case

    root = slist_push_front(root, value);

However, if node allocation fails, the return value is NULL, and the whole list is lost. I recommend to change the signature to

    int slist_push_front(slist_t ** rootptr, int value);

to be used along the lines of

    if (slist_push_front(&root, value) == FAIL) {
        ....
    }

In slist_push_front, it doesn't matter whether the root is NULL or not. node->next = root will work just fine in both cases.
In slist_push_back the next pointer must become null in any case, so it seems logical to assign it once when the node is created.

I also recommend to refactor the while (temp->next) loop into a slist_get_tail function. Then slist_push_back is streamlined into

int slist_push_back(slist_t ** root, int val)
{
    slist_t * node = slist_create_node(val);
    if (node == NULL) {
        return FAILURE;
    }
    slist_t * tail = slist_get_tail(*root);
    if (tail) {
        tail->next = node;
    } else {
        *root = node;
    }


Create all combinations of length N from a given alphabet I have some (very limited) experience in Java 6 and decided to revisit the language. To do so I wrote a solution for the following challenge:

You are given a number N and a string S. Print all of the possible ways to write a string of length N from the characters in string S, comma delimited in alphabetical order.
The first argument will be the path to the input filename containing the test data. Each line in this file is a separate test case. Each line is in the format: N,S i.e. a positive integer, followed by a string (comma separated). E.g.
Print all of the possible ways to write a string of length N from the characters in string S comma delimited in alphabetical order, with no duplicates.

Input sample:
1,aa
2,ab
3,pop

Output sample:
a
aa,ab,ba,bb
ooo,oop,opo,opp,poo,pop,ppo,ppp

The code works but performance is pretty bad and it feels like this can be improved.
Please point out anything that is wrong with this. I'm especially interested to know if this could be written in an entirely different way using features of Java 8.
import java.io.BufferedReader;
import java.io.FileReader;
import java.io.File;
import java.io.IOException;
import java.util.List;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Set;
import java.util.HashSet;
import java.lang.String;
import java.lang.Integer;

public class Main
{
    public static List<String> create_combinations(List<Character> alphabet, int word_length)
    {
        List<Integer> index = new ArrayList<Integer>(word_length);
        for (int i = 0; i < word_length; ++i)
        {
            index.add(0);
        }
        List<String> results = new ArrayList<String>();

        for (;;)
        {
            StringBuilder word = new StringBuilder();
            for (int i = 0; i < index.size(); ++i)
            {
                word.append(alphabet.get(index.get(i)));
            }
            results.add(word.toString());

            for (int i = index.size() - 1; ; --i)
            {
                if (i < 0)
                {
                    return results;
                }

                index.set(i, index.get(i) + 1);

                if (index.get(i) == alphabet.size())
                {
                    index.set(i, 0);
                }
                else
                {
                    break;
                }
            }
        }
    }

    public static void main (String[] args) throws IOException
    {
        BufferedReader buffer = new BufferedReader(new FileReader(new File(args[0])));
        String line;
        while ((line = buffer.readLine()) != null)
        {
            int word_length = Integer.parseInt(line.substring(0, line.indexOf(',')));
            String letters = line.substring(line.indexOf(',') + 1);

            Set<Character> unique_letters = new HashSet<Character>();
            for (char c : letters.toCharArray())
            {
                unique_letters.add(c);
            }
            List<Character> alphabet = new ArrayList<Character>();
            alphabet.addAll(unique_letters);
            Collections.sort(alphabet);

            System.out.println(String.join(",", create_combinations(alphabet, word_length)));
        }
    }
}

Nitpicks

You're using the "wrong" bracing style :). The vast majority of Java code-conventions enforces "egyptian" bracing style with the opening brace on the same line.
The method name create_combinations is in "snake_case". The vast majority [...] "camelCase" variable naming. The method should be named createCombinations.

Approach

While you're using a pretty performant approach with StringBuffer, this can be improved a bit when working on a raw char[]. The vast majority of the costs here comes from creating the new Strings. That is something you won't be able to eliminate.

Furthermore you seem to have missed the "crucial point" of the challenge. What you're doing from a mathematical (or theoretical cs) point of view is counting. Your strings are fixed-width numbers.

Consider the following call:

createCombinations({'0', '1'}, 4);

the results are the following:

0000
0001
0010
0011
0100
0101
0110
0111
[...]
1111

You should see that this matches the binary numbers 0 to 15.

Now let's see what we can do with this information:

Know how many results there will be. (namely \$a^c\$ with \$a\$ being the size of the alphabet and \$c\$ the length of the word)
Count through the results.

This makes clear what our method signature will look like:

private String[] createCombinations(char[] orderedAlphabet, int wordLength) {

If you want to add some raw performance it would be a consideration to return a char[][] instead.

Enough talk about concepts: Here's how it looks:

public String[] createCombinations(char[] orderedAlphabet, int wordLength) {
    long resultCount = Math.pow(orderedAlphabet.length, wordLength);
    String[] result = new String[resultCount];
    char[] workingWord = new char[];
    for (int i = 0; i < workingWord.length; i++) {
        workingWord[i] = orderedAlphabet[0];
    }
    // initialization complete, algorithm starts:
    for (int i = 0; i < result.length; i++) {
        result[i] = new String(workingWord);
        int leastSignificantDigit = i % orderedAlphabet.length;
        workingWord[workingWord.length - 1] = orderedAlphabet[leastSignificantDig];
        // TODO propagate changes up the workingWord if the lsd is 0.
        // [...]
    }
    return result;
}

Propagating the changes up the working word is left as a challenge for you.

Interestingly the observation about the "crucial point" of the challenge will allow you to formulate  the challenge as a Stream. Do note that the stream will not have the state of a workingWord to ease computation (because streams must not have side-effects), which imposes a performance hit. Either way it's basically only a bit of hard thinking on how numbers work to get from IntStream.range(0, resultCount) to a Stream<String> containing all your results.


Data set with many variables in Python, many indented dictionaries? I am working with a data set that has many variables. Currently I am storing the data in many indented dictionaries in the following way:
import numpy as np

X_POSITIONS = [0,1.5,1]
Y_POSITIONS = [0,1,2]
CHANNELS = ['left pad', 'right pad', 'top pad', 'bottom pad']

data = {}
for x in X_POSITIONS:
    data[x] = {}
    for y in Y_POSITIONS:
        data[x][y] = {}
        for ch in CHANNELS:
            data[x][y][ch] = np.random.rand() # Here I would place my data.

This works fine, but it is cumbersome if by some reason I need to change the order of the keys. Consider the following function:
def do_something_with_single_channel(data_from_one_channel):
    for x in data_from_one_channel:
        for y in data_from_one_channel[x]:
            print(f'x = {x}, y = {y}, data[x][y] = {data_from_one_channel[x][y]}')

Before calling this function the whole data object has to be rearranged:
new_data = {}
for ch in CHANNELS:
    new_data[ch] = {}
    for x in X_POSITIONS:
        new_data[ch][x] = {}
        for y in Y_POSITIONS:
            new_data[ch][x][y] = data[x][y][ch]

do_something_with_single_channel(new_data['left pad'])

Since this seems to be a very common thing to do, I am sure that there must already exist something better than dictionaries for this purpose. Ideally I imagine something that can be accessed in the same way as the arguments of a function by giving names to the variables and forgetting about the order, for example something of the form
do_something_with_single_channel(data[channel='left pad'])

Does something like this exists? What's the name?


LinkedList of int nodes in C++ I'm new to C++ programming. I'm experienced in Java and its OOP paradigm.
This code works well. I just need to make sure whether it's correct in terms of C++ programming standard.

main.cpp

#include <iostream>
#include "ListNode.h"
#include "LinkedList.h"

using namespace std;

int main(void) {

    LinkedList l;
    ListNode a(10);
    ListNode b(5);
    ListNode c(3);
    l.addFirst(a);
    l.addFirst(b);
    l.addLast(c);

    bool empty = l.isEmpty();

    cout << l.listSize() << endl;

    cout << empty << endl;

    system("PAUSE");
    return 0;
}

LinkedList.h

#pragma once
#include "ListNode.h"

class LinkedList
{
public:
    LinkedList();
    ~LinkedList();
    void addFirst(ListNode &node);
    void addLast(ListNode &node);
    bool isEmpty();
    int listSize();

private:
    ListNode *head;
    ListNode *tail;
    int size;
};

LinkedList.cpp

#include "LinkedList.h"

LinkedList::LinkedList()
{
    head = new ListNode();
    tail = new ListNode();
}

LinkedList::~LinkedList()
{
}

void LinkedList::addFirst(ListNode & node)
{
    if (isEmpty())
    {
        *head = node;
        *tail = node;
        size = 1;
    }
    else {
        node.setNext(*head);
        head->setPrev(node);
        *head = node;
        size++;
    }
}

void LinkedList::addLast(ListNode & node)
{
    if (isEmpty())
    {
        *tail = node;
        *head = node;
        size = 1;
    }
    else
    {
        tail->setNext(node);
        node.setPrev(*tail);
        *tail = node;
        size++;
    }
}

bool LinkedList::isEmpty()
{
    return size == 0;
}

int LinkedList::listSize()
{
    return size;
}

ListNode.h

#pragma once
class ListNode
{
public:
    ListNode();
    ListNode(int val);
    ~ListNode();

    inline void setPrev(ListNode &node) { *prev = node; }
    inline ListNode *getPrev() { return prev; }
    inline void setNext(ListNode &node) { *next = node; }
    inline ListNode *getNext() { return next; }
    inline void setValue(int val) { value = val; }
    inline int getValue() { return value; }

private:
    ListNode *prev;
    ListNode *next;
    int value;
};

ListNode.cpp

#include "ListNode.h"

ListNode::ListNode()
{
}

ListNode::ListNode(int val)
{
    prev = new ListNode;
    next = new ListNode;
    value = val;
}

ListNode::~ListNode()
{
}

Am I do it correctly as usually done by C++ programmer? Or there is something I need to fix.
I heavily put my focus on pointer/reference usage.

Two things:

You need to use a lot of consts.

Example:

    void setNext(ListNode &node)

    void setNext(const ListNode &node)

You call to new in some places of your code but never call to delete


Print output one word per line - K&R Exercise 1-12 When writing this code I found myself getting more and more complex. When I got to the solution I trimmed the excess parts I'd thought I needed from before, but didn't actually need. The example of wc.c had some definitions for IN and OUT. Why didn't I need those for writing this example?

If there are any bugs or improvements I can make, let me know.

include <stdio.h>

/* prints its input one word per line */
main()
{
  int c;

  c = 0;
  while ((c = getchar()) != EOF) {
     if (c == ' ')
      putchar('\n');
     else
       putchar(c);
  }
}

include <stdio.h>

You're missing a # there, copy/paste issue?

main()

That's been out of style for a very long time, and is in fact no longer valid according the standard (from C99 and on). Always specify the return type (for main and all other functions).

int main()

int c;

c = 0;

Don't keep uninitialized variables around for any length of time if you can avoid it. A bit trivial here, but nonetheless prefer:

int c = 0;

In this specific case, as Jonathan Leffler pointed out, you're unconditionally initializing c in the very next line, so the initialization is not necessary.

(Starting with C99, you can declare variables as you need them. You don't have to stack them all at the top. So as far as possible, only declare variables at the point where they are needed and initialize them at the same time.)

if (c == ' ')
 putchar('\n');
else
  putchar(c);

Your indentation's out of whack here, the first putchar should be indented one with one more space.

As for "bugs", your program won't print what is usually expected if, summarizing points made in Stat1c_V01D's answer and comments by Jonathan:

The input contains empty lines. You'll output empty lines too. They should be omitted.
The input contains whitespace other than plain spaces and newlines (e.g tabs or form feeds). Your code will not split those out. Look at isspace.
The input contains consecutive whitespace characters. Your program will output consecutive new lines between words.
The output doesn't end with a \n. Your program won't output one either which is "bad form". On a Unix terminal, you'll end up with no newline between the last word output and the new prompt.

That's why extra bookkeeping is needed in the solutions you find for this exercise.


Service layer for course student registration I have this piece of code in my service layer and registers and student for a course.
As you can see if have alot of if statements and i was wondering if any could suggest a nicer way of writing this code?
    public async Task<bool> RegisterStudentForCourseAsync(RegisterStudentForCourseRequest registerStudent)
    {
        var student = await _studentRepository.GetStudentInformationAsync(registerStudent.StudentId);

        if (student == null)
        {
            throw new StudentCourseRegistrationException("Student Not Found");
        }

        var alreadyRegistered = student.Enrollments.Any(x => x.CourseId == registerStudent.CourseId);

        if (alreadyRegistered)
        {
            throw new StudentCourseRegistrationException("Student is Already Registered for Course");
        }

        //change this to a config setting
        if (student.SubjectCount >= 5)
        {
            throw new StudentCourseRegistrationException("Cannot Register Amount of Courses");
        }

        var course = _courseRepository.GetCourseInformation(registerStudent.CourseId);

        if(course == null)
        {
            throw new StudentCourseRegistrationException("Course Not Found");
        }

        if(course.Enrolled >= course.Capacity)
        {
            throw new StudentCourseRegistrationException("Course is fully Booked");
        }

        return await _courseRepository.SaveRegisterationOfStudentForCourse(course, registerStudent, student);
    }
}

you can write fluent extension methods:
 public static class Extensions {
     public static T EnsureNotNull<T>(this T t, string message = null) {
         if (t == null) throw new StudentCourseValidationException(message ?? "Value not found");
         return t;
     }
     public static Student EnsureAlreadyRegistered(this Student student, string courseId) {
          if (student.Enrollments.Any(x => x.CourseId == courseId) throw new StudentCourseRegistrationException("Student is Already Registered for Course");
          return student;
      }
      //and so on
 }

then your code would look like:
public async Task<bool> RegisterStudentForCourseAsync(RegisterStudentForCourseRequest registerStudent)
    {
        var student = await _studentRepository.GetStudentInformationAsync(registerStudent.StudentId);

        student
              .EnsureNotNull()
              .EnsureNotAlreadyRegistered(registerStudent.CourdeId)
              .EnsureCanHaveMoreCourses();

        var course = _courseRepository.GetCourseInformation(registerStudent.CourseId);

        course
             .EnsureNotNull()
             .EnsureNotFullyBooked();

        return await _courseRepository.SaveRegisterationOfStudentForCourse(course, registerStudent, student);
    }
}


Possible leak in heavy app I built my first long running AJAX heavy app and memory leaks are eating me alive.
I don't want to post too much code because it should be my job to fix it all, but I need some help.

Does this code leak? Where? Why? and if you'd be so kind as to suggest a fix.

function list_stuff(data, type, where, order) {
    var items = [];
    $.each(data, function (i, item) {
        items.push(
        //using jquery $data might make this look more readable
        '
 ' + '
' + item.name + '
' + '
' + item.description + '
' + '
' + 'Miles: ' + item.distance + '  People: ' + item.people);

        if (item.privacy != 0) {
            items.push('  (Private group) ');
        }
        if (where == '#thelist') {
            items.push('
' + '<button data-action ="2" type ="button" class ="join_button" onclick="join_group(this)">Join

');
        } else if (where == '#thelist2') {
            items.push('
' + '<button data-action= "4" type ="button" class ="join_button" onclick="join_group(this)">Leave

');
        }
    });

    if (order === 'append') {
        $(where).append(items.join(''));
    } else {
        $(where).prepend(items.join(''));
    }

    scroll.refresh();
}

My AJAX looks like this:

function ajax(send,where) {
    $.ajax({
        url: where,
        type: 'POST',
        dataType: 'json',
        data: send,
        success: function(a) {
            if (a.success===1) {
                list_stuff(a.data, a.type, '#list1','append');
            } else {
                create_group_problem(a.msg);
            }
            if(a.msg.length >1 ) {
                msg_disp(a.msg);
            }
        },
        error: function(err){
            msg_disp('error');
        }
    });
}

I understand (tenuously) that JavaScript gets garbage collected, but doesn't this form a circular reference that may prevent it from being collected? Please correct me if I'm wrong, but here is my reasoning:

list_stuff():

'<button data-action ="2" type ="button" class ="join_button" onclick="join_group(this)">Join

'

JS -> DOM
Generated LI Element:

onclick="join_group(this)"

DOM ->JS

Last question for the sake of learning/understanding: if this is not a circular reference, that will be prevented from being garbage collected, how can I change it to become one?

function join_group(block) {
    var $block = $(block);
    var pass   = 0;
    var c_id   = $block.closest("li").data("id");
    var p      = $block.closest("li").data("privacy");
    var action = $block.data("action");

    if(p !== 0 && action ==3){
        pass = prompt("This group requires a password to join.");
        if (pass === null || pass ==='') {
            return;
        }
    }

    var send ={
        group_id: c_id,
        group_action: action,
        privacy:p,
        password: pass,
    };

    ajax(send, 'group');
}

javascript is garbage collected, unless you're making huge(tens of MB of XML/JSON) data transfers every couple of seconds and keep references to that data, there should be "no memory leak" unless the browser is doing a lousy job.

I would put "items" array as global variable so that with each request, you overwrite it, in this way, old data will be discarded, i.e.

// moved globally
var items = [];
function list_stuff(data, type, where, order)
{
  // try to parse data as JSON
  try
  {
    items = jQuery.parseJSON(data);
    var index;
    var item;
    for ( index = 0; index < items.length; index++ )
    {
      item = items[index];
      // do something with "item"
    }
  }
  catch(e)
  {
    // handle error here
  }
}


To-do list app using jQuery I created a simple todo kind of app using JavaScript. It is working fine but the code which I wrote is not proper. Please review my code and improve it if it is needed.

$(function () {
    var todo = new Todo('contents');

    $('.addBtn').on('click', function() {
        var name = $(this).parent().find('input[type="text"]').val();
        todo.add(name);
    });

    $('.contents').on('click', '.remove', function() {
        var el = $(this).parent();
        todo.remove(el);
    });

    $('.contents').on('click', '.update', function() {
        var dom = $(this);
        todo.addUpdateField(dom);
    });

    $('.contents').on('click', '.updateBtn', function() {
        var el = $(this);
        todo.update(el);
    });
});

Here is my todo.js file

var Todo = function(c) {
    var contents = $('.' + c),
    name;

    add = function (name) {
        if(name != "") {
            var div = $('

');
            div.append('' + name + '');
            div.append("<button class='update' class='update'>Edit</button>");
            div.append("<button class='remove' name='remove'>Remove</button>");
            contents.prepend(div);
            $('.name').val('').focus();
        }
        return;
    },

    addUpdateField = function (dom) {
        var name = dom.parent().find('span').text(),
            field = $('<input type="text" value="' + name + '" />'),
            update = $('<button class="updateBtn">Update</button>');

        return dom.parent().html('').append(field).append(update);
    },

    update = function(el) {
        var val = el.parent().find('input').val();
        el.parent().html('' + val + '')
        .append('<button class="update" class="update">Edit</button>')
        .append('<button class="remove" class="remove">Remove</button>');
    },

    remove = function (el) {
        return el.remove();
    };

    return {
        add             : add,
        update          : update,
        remove          : remove,
        addUpdateField  : addUpdateField
    };
};

As I read it, you want:

one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow Inkbug`s advice from his comment, you should probably do this:

cache the todo root element (var todoRoot = $(...);)
rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read
change the api of your todo.js functions. For update / edit / remove, just use the dom node of the div as the argument:

function(target, action) {
    todoRoot.on('click', target, function(e) {
        e.preventDefault();
        e.stopPropagation();
        action.call(todo, $(this).parents('.names').first());
    });
}

bindClick('.remove', todo.remove);
bindClick('.edit',   todo.edit);
bindClick('div',     todo.update);

don't use that many appends. Put it all in a string (that can contain more than one DOM node!)
don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM
move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create


Finding the middle element of Singly Linked List Here is my code:

    public void findMiddlePos() {
    int pos = 1;
    Node pointerOne = first;
    Node pointerTwo = first;
    while(pointerOne.next.next != null) {
        pointerOne = pointerOne.next.next;
        pointerTwo = pointerTwo.next;
        pos++;
    }
    System.out.println("The middle element is : ");
    if((pos % 2) == 1)
        pointerTwo.printNode();
    else
        pointerTwo.next.printNode();

}

Is it an efficient way? Also, I am doubtful about this line while(pointerOne.next.next != null). Previously, I was just using this condition in while loop: pointerOne.next != null, but I was getting Null pointer exception so shifter to earlier one.

Thanks.

I'm no Java programmer, so this merely regards the algorithm.

while(pointerOne.next.next != null) {
    pointerOne = pointerOne.next.next;
    pointerTwo = pointerTwo.next;
    pos++;
}

What happens if pointerOne.next is null? I'm guessing you'd need this:

while(pointerOne != null && pointerOne.next != null) {

I don't know if there is some shorthand for this in Java.

I also fail to see the purpose of the final if. Assume that you have a list with 3 elements. Then, after the loop, you'll have

pointerOne = first.next.next; // i.e., pointerOne = last
pointerTwo = first.next;
pos = 2;

So, pointerTwo points to the second (i.e., middle) element. But then your if will printout the successor of pointerTwo, which is the third element of a three-element list, so definitely not the middle one.

Lastly, I'd suggest using more descriptive names. For example, ptrLast and ptrMiddle, instead of pointerOne and pointerTwo. It makes your code more readable to others (which also includes you, after a few months).

One comment on your previous attempt:

  Is it an efficient way? Also, I am doubtful about this line while(pointerOne.next.next != null). Previously, I was just using this condition in while loop: pointerOne.next != null, but I was getting Null pointer exception so shifter to earlier one.

If the length of your list is even, you'll get pointerOne == null at some point, so pointerOne.next will result in an error. But, similarly, if the length of your list is odd, you'll get pointerOne.next == null at some point, so pointerOne.next.next will result in an error.

Since you're jumping by two nodes at a time, you need to make sure that they are both non-null. Hence the condition I've put above.


Assistance requested to refactor a section of Typescript code for readability Code
if (source.length > 0) {
    if (
        source[0].mandatory === true &&
        this.allInputs.documentsType !== "mandatory"
    ) {
        item.mandatory = !item.mandatory;
    } else if (
        source[0].mandatory === false &&
        this.allInputs.documentsType === "mandatory"
    ) {
        item.mandatory = !item.mandatory;
    }
} else {
    if (
        target[0].mandatory === true &&
        this.allInputs.documentsType === "mandatory"
    ) {
        item.mandatory = !item.mandatory;
    } else if (
        target[0].mandatory === false &&
        this.allInputs.documentsType !== "mandatory"
    ) {
        item.mandatory = !item.mandatory;
    }
}

All is working, the problem I have is that this code is not very readable. Can someone please help me refactor this?
Thanks

To make it easier to follow the flow of the logic, I'm just going to pull out the longer parts of these conditions into helper variables. They may not stay like that in the final version, but it helps as I'm revising it.
const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';

if (source.length > 0) {
    const mandatory = source[0].mandatory;
    if (mandatory === true && isDocumentTypeMandatory) {
        item.mandatory = !item.mandatory;
    } else if (mandatory === false && this.allInputs.documentsType === "mandatory") {
        item.mandatory = !item.mandatory;
    }
} else {
    const mandatory = target[0].mandatory;
    if (mandatory === true && this.allInputs.documentsType === "mandatory") {
        item.mandatory = !item.mandatory;
    } else if (mandatory === false && isDocumentTypeMandatory) {
        item.mandatory = !item.mandatory;
    }
}

The trick to simplifying these kinds of things is to figure out what is similar, figure out how to change the code to make those parts exactly the same, then DRY it up.
For example, I'm noticing that the two branches of the outer if-else are almost exactly the same. The only differences are where it gets the mandatory variable from, and the ! that is sometimes in front of isDocumentTypeMandatory.
Lets start by extracting out the diferences from the two branches:
const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';
const mandatory = source.length > 0 ? source[0].mandatory : target[0].mandatory;
const sourceHasContent = source.length > 0;

if (source.length > 0) {
    if (mandatory === true && isDocumentTypeMandatory !== sourceHasContent) {
        item.mandatory = !item.mandatory;
    } else if (mandatory === false && isDocumentTypeMandatory === sourceHasContent) {
        item.mandatory = !item.mandatory;
    }
} else {
    if (mandatory === true && isDocumentTypeMandatory !== sourceHasContent) {
        item.mandatory = !item.mandatory;
    } else if (mandatory === false && isDocumentTypeMandatory === sourceHasContent) {
        item.mandatory = !item.mandatory;
    }
}

Now those the two branches of the outer-if are now exactly the same, which lets me just get rid of it.
const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';
const mandatory = source.length > 0 ? source[0].mandatory : target[0].mandatory;
const sourceHasContent = source.length > 0;

if (mandatory === true && isDocumentTypeMandatory !== sourceHasContent) {
    item.mandatory = !item.mandatory;
} else if (mandatory === false && isDocumentTypeMandatory === sourceHasContent) {
    item.mandatory = !item.mandatory;
}

At this point, we can just combine the two conditions into one. I personally prefer avoiding multiline if conditions and instead extract the condition into an external variable.
const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';
const mandatory = source.length > 0 ? source[0].mandatory : target[0].mandatory;
const sourceHasContent = source.length > 0;

const flipMandatoryValue = (
    (mandatory === true && isDocumentTypeMandatory !== sourceHasContent) ||
    (mandatory === false && isDocumentTypeMandatory === sourceHasContent)
);

if (flipMandatoryValue) {
    item.mandatory = !item.mandatory;
}

This is probably about as good as it'll get. Certainly, the variable naming can be a ton better (without much context, its hard to give them good names).

There is one other trick that can be applied to simplify things even further, but there's already so much going on here that it might just make the code even less readable, but I'll present it anyways.
This whole time I've been assuming that mandatory can take on other values besides "true" or "false", which is why you're explicitly checking if they're === true or === false. In the following revisions, I'll preserve this idea by doing a typeof check. I'll then modify the condition as follows:
const flipMandatoryValue = typeof mandatory === 'boolean' && (
    (mandatory && isDocumentTypeMandatory !== sourceHasContent) ||
    (!mandatory && isDocumentTypeMandatory === sourceHasContent)
);

// is the same as
const subCondition = isDocumentTypeMandatory !== sourceHasContent;
const flipMandatoryValue = typeof mandatory === 'boolean' && (
    (mandatory && subCondition) ||
    (!mandatory && !subCondition)
);

// is the same as
const subCondition = isDocumentTypeMandatory !== sourceHasContent;
const flipMandatoryValue = typeof mandatory === 'boolean' && (
    // Either both of these booleans should be true, or both should be false,
    // so we can just compare them to each other with ===.
    mandatory === subCondition
);

// is the same as
const subCondition = isDocumentTypeMandatory !== sourceHasContent;
// We can drop the typeof check - we're comparing with a boolean anyways
const flipMandatoryValue = mandatory === subCondition;

This gives us the final revision as follows:
const isDocumentTypeMandatory = this.allInputs.documentsType === 'mandatory';
const mandatory = source.length > 0 ? source[0].mandatory : target[0].mandatory;
const sourceHasContent = source.length > 0;
const subCondition = isDocumentTypeMandatory !== sourceHasContent;

if (mandatory === subCondition) {
    item.mandatory = !item.mandatory;
}

As I said before, using === to compare booleans can be a useful trick to simplify complicated if-thens, but here I feel like it adds more complexity than value, unless you can find good ways to name everything and make it more intuitive.


JavaScript events architecture I have made one architecture on my project and want to know. Is it good or bad?

events = {
      init: function() {
        for (var callback in this.events){
          this.bind(this.events[callback].selector, this.events[callback].event, this[callback], this.events[callback].params || {})
        }
      },
      events: {
        initSettings: {
          selector: ".js-settingsCollapse",
          event: "click",
          params: {
            ss: "11"
          }
        },
        settingTrigger: {
          selector: ".js-settingTrigger",
          event: "click",
          params: {
            ss: "22"
          }
        },
        themeTrigger: {
          selector: ".js-themeTrigger",
          event: "click",
          params: {
            ss: "33"
          }
        }
      },
      bind: function(selector, event, callback, params) {
        $(selector).on(event, params, callback)
      },
      initSettings: function(e) {
        ...
      },
      settingTrigger: function(e) {
        ...
      },
      themeTrigger: function(e) {
        ...
      }
    }

And in document.ready I run the events.init() function. All events in the events.events object takes and bind them using the jQuery method on.

To me the code is a little hard to read in that you have an initialization function, metadata around events to be bound, the binding function, and the callbacks all at the same level in your events object.  How is this any better than just defining all the event bindings within a typical document.ready() block?  At least there, all your code for each binding is kept all in one place, rather than spread out across the different properties in this event object.

To me, something like the following is simply clearer (and would actually be more concise in terms of total characters needed to implement the functionality):

$(document).ready(function() {
    $('.js-settingsCollapse').on('click', function() {
        ...
    });

    $('.js-settingTrigger').on('click', function() {
       ...
    });

    ...
}

I also don't see how your framework will handle event delegation so as to allow for this approach to work with dynamically added elements.

Also, since you are really not using any functionality in jQuery which is not available in regular javascript, you might want to consider dropping the jQuery dependency.


Proper use of a generic value type wrapper class I have a wrapper class:

[Serializable]
// (465 references)
public class Value<T>
{
    private T value;

    [NonSerialized]
    private List<IValueListener<T>> listeners = new List<IValueListener<T>>();

    // (232 references)
    public Value(T value)
    {
        this.value = value;
    }

    // (274 references)
    public T Get()
    {
        return value;
    }

    // (18 references)
    public void Set(T value)
    {
        if (!this.value.Equals(value))
        {
            this.value = value;

            foreach (IValueListener<T> listener in listeners)
            {
                listener.ValueUpdated(this);
            }
        }
    }

    public void AddListener(IValueListener<T> listener)
    {
        listeners.Add(listener);
    }

    public void RemoveListener(IValueListener<T> listener)
    {
        listeners.Remove(listener);
    }
}

The listener interface:

public interface IValueListener<T>
{
    void ValueUpdated(Value<T> source);
}

It's used in a program that alters save files of a game. An object is constructed using the save file data and any value types encountered are wrapped in the Value class. Object is then managed by the program and the save file is rewritten from the objects data upon saving.

The listener is here since altering some data in the save file influences other, so this gives me a way to manage that on a low level (instead of making TextBoxes notify each other).

When I save the file, I actually clone the object, process the clone for saving and save the clone. That's why the class is Serializable. I don't care about listeners when saving so the list of them isn't.

My question is whether what I have done here is a well written code or should it be substituted with something else.

I also copied some of the # of references that VS displays just to demonstrate how heavily this class is used.

It is unclear how to use this class if I want to subscribe to two different objects that implement, say, Value<double>. There is no way to tell which of the two values has changed. This looks like a pretty major design flaw, unless I am missing something. So you should probably either use regular events (so I can supply different handlers for different values) or provide "source" argument. Personally, I think first option is better. You should also consider using proper event aggregator instead of storing the list of listeners in values themselves.

Also I think you should have an actual property instead of Get() and Set() methods. And you can make listeners field read-only.


Polynomial multiplication using Karatsuba method I am trying to solve a problem, which requires me to output the xor of all coefficients in the product of 2 input polynomials. Having seen that the normal O(n^2) multiplication is not optimal, I tried to accomplish the same task by using the Karatsuba method which is equivalent to the one for numbers. It turned out that my code ran even slower in practice than the O(n^2) one, even though it gave me the correct answer.
#include <bits/stdc++.h>

using namespace std;

// In ra đa thức
// Print the polynomial coefficient
void print(vector<int> a) {
    for (int &i: a) cout << i << " ";
    cout << endl;
}

// Lũy thừa bậc 2 tiếp theo của 1 số
// The next power of 2 of a number
int nextPowerOf2(int n)
{
    n--;
    n |= n >> 1;
    n |= n >> 2;
    n |= n >> 4;
    n |= n >> 8;
    n |= n >> 16;
    n++;
    return n;
}

// Đưa 2 đa thức về cùng bậc, trả về bậc chung
// Make 2 polynomials to have the same degree
int equalize(vector<int> &a, vector<int> &b) {
    int x = a.size(), y = b.size();
    if (x > y) {
        b.resize(x);
        return x;
    }
    else if (x < y) {
        a.resize(y);
        return y;
    }
    return x;
}

// Loại bỏ các hệ số đa thức bậc cao nhất bằng 0
// Remove the highest degree coefficients which are equal
void cuttail(vector<int> &a) {
    while (a.back() == 0)
        a.pop_back();
}

// Hàm tính tổng 2 đa thức
// Sum of 2 polynomials
vector<int> sum(vector<int> a, vector<int> b) {
    int deg = equalize(a, b);
    vector<int> c(deg);
    for (int i = 0; i < deg; i++)
        c[i] = a[i] + b[i];
    return c;
}

// Hàm tính hiệu 2 đa thức
// Difference of 2 polynomials
vector<int> diff(vector<int> a, vector<int> b) {
    int deg = a.size();
    vector<int> c(deg);
    for (int i = 0; i < deg; i++)
        c[i] = a[i] - b[i];
    return c;
}

// Hàm tính tích 2 đa thức
// Product of 2 polynomials
vector<int> prod(vector<int> a, vector<int> b) {
    int deg = a.size();
    int hdeg = deg/2;
    if (deg == 1) return {a[0] * b[0]};
    vector<int> a1(hdeg), a2(hdeg), b1(hdeg), b2(hdeg), v(deg), u(deg), w(deg*2);
    for (int i = 0; i < deg/2; i++) {
        a1[i] = a[i];
        b1[i] = b[i];
        a2[i] = a[i + hdeg];
        b2[i] = b[i + hdeg];
    }
    u = prod(a1, b1);
    v = prod(a2, b2);
    w = prod(sum(a1, a2), sum(b1, b2));
    w = diff(diff(w, u), v);
    vector<int> wdeg(hdeg, 0), vdeg(deg, 0);
    w.insert(w.begin(), wdeg.begin(), wdeg.end());
    v.insert(v.begin(), vdeg.begin(), vdeg.end());
    return sum(u, sum(w, v));
};

int main() {
    ios_base::sync_with_stdio(false);
    cin.tie(NULL); cout.tie(NULL);
    int n, m;
    cin >> n;
    vector<int> a(n+1);
    for (int &i: a)
        cin >> i;
    cin >> m;
    vector<int> b(m+1);
    for (int &i: b)
        cin >> i;
    int deg = nextPowerOf2(equalize(a, b));
    a.resize(deg);
    b.resize(deg);
    vector<int> c = prod(a, b);
    cuttail(c);
    int result = 0;
    for (int i: c)
        result = result ^ i;
    cout << result;
    return 0;
}

I highly doubted that the vector allocation is behind the slow speed of multiplication, but I don't know how to fix it. I would appreciate if you can take a look at my code and give me some idea to improve it.
EDIT: As requested, I have a test case given here:
3 83 86 77 15
4 93 35 86 92 49
The 2 input polynomials are in the degree of 3 and 4 respectively: $$83 + 86x + 77x^2 + 15x^3$$ and $$93 + 35x + 86x^2 + 92x^3 + 49x^4$$
The output is 20731, which is the xor of all coefficients of the product of 2 input polynomials above (7719 xor 10903 xor 17309 xor 19122 xor 19126 xor 12588 xor 5153 xor 735 = 20731)

Algorithmic complexity is not a good indicator of real-world performance
When you have to choose between an \$\mathcal{O}(N^2)\$ or \$\mathcal{O}(N^{1.58})\$ algorithm, you would think that the latter is faster, however that is only true for sufficiently large values of \$N\$. In practice, unless you have more than a thousand digits to multiply, the simple \$\mathcal{O}(N^2)\$ algorithm is faster.
Don't make the inputs of equal length
If you want to multiply a polynomial of degree 100 with a polynomial of degree 1, then your program will expand the latter polynomial to degree 100, and then do the multiplication. But most of the work is now wasted on multiplying things by 0. Try to make your algorithm work for vectors of different lengths.
Avoid creating unnecessary intermediate results
If the goal is purely to get the XOR of the coefficients of the product, then you don't actually need to store the product before calculating the final result of the XORs. Instead, with a trivial algorithm, you can just simply do:
int result{};
for (auto i: a)
    for (auto j: b)
         result ^= i * j;

This avoids creating a temporary vector, which requires heap memory allocation, and avoids an additional pass over the temporary result to get the final answer. Again, this doesn't reduce algorithmic complexity, but it does reduce how many cycles you have to spend for each iteration of the \$\mathcal{O}(N^2)\$ algorithm even further.
Use a hybrid approach
In your implementation of Karatsuba's algorithm, you have this line:
if (deg == 1) return {a[0] * b[0]};

Knowing that for smaller vectors, a naive algorithm is actually faster, you can replace this line with:
if (deg < CUTOFF)
    return naive_product(a, b);

Where you set the constant CUTOFF to some value that you have to determine experimentally. This approach is similar to those taken by state-of-the-art sorting algorithms, which typically also use a divide-and-conquer approach and when the problem gets small enough, they will use insertion sort.
Pass vectors by const reference
Most of your functions like prod(), sum(), and so on, take parameters by value. This is very inefficient, since it means a copy of the  vectors used as input arguments will be made. Pass them by const reference instead, it's as simple as:
vector<int> sum(const vector<int> &a, const vector<int> &b) {
    ...
}


Drawing a table in Python3 I have created a module to draw text tables. The relevant parts are below and here's the full code for txtable).

class Row(Txtable):
    [...]
    def format(self, col_widths):
        """Return the row as formatted string.
        """

        columns = []
        for x, column in enumerate(self.columns):
            # skip empty rows
            if 0 == len(str(column)):
                columns.append((" " * col_widths[x]))
                continue
            columns.append(self.format_cell(x, col_widths[x]))
        return str(" " * self.col_spacing).join(columns)

class Table(Txtable):
    [...]
    def add_rows(self, rows):
        """Add multiple rows.
        """

        for columns in rows:
            self.add_row(columns)

    [...]
    def render(self):
        """Return the table as a string.
        """

        # render cells and measure column widths
        for y, row in enumerate(self.rows):
            for x, column in enumerate(row.columns):
                fmt_column = row.format_cell(x)
                width = len(fmt_column)
                try:
                    if self.col_widths[x] < width:
                        self.col_widths[x] = width
                except IndexError:
                    self.col_widths.append(width)
        # render the table with proper spacing
        output = ""
        lmarg = (" " * self.margin)
        for row in self.rows:
            [...do rest of the rendering...]

I'd like to know about ways I could optimize this please. I see I could calculate the maximum column widths in Table.add_rows() instead of Table.render(), but I'd like to keep the possibility to modify the rows before rendering the final output.

How could I cut back on the looping? Is there a better way to output tabular data?

The nested loop under # render cells and measure column widths computes maximum cell width of each column. Using itertools.zip_longest to turn columns into rows lets you use max on them:

cell_widths = [[len(row.format_cell(x)) for x in range(len(row.columns))]
                for row in self.rows]
col_widths = list(map(max, itertools.zip_longest(*cell_widths, fillvalue=0)))


Text menu and validation of the chosen item I am writing a c# console app that will take a user input from a list of selected options and do things based on the selected option. I have it working right now but just looking over it there seems that I should be able to shorten the code with other methods and make it perform a bit faster and more efficient but for some reason I just can't see it. Am I missing something or is this really the way to be doing this check?

static void Main(string[] arguments)
{
    if (arguments.Length == 0)
    {
        Console.WriteLine("No run mode entered. Usage: JobSightMaintenance <FirstRun | NightlyMaintenance>");
        Console.WriteLine("Please select one of the following options to run:");
        printOptions();

        string userInput;
        bool isValidInput = true;

        do
        {
            userInput = Console.ReadLine();
            int selectedOption;

            try
            {
                selectedOption = int.Parse(userInput);
            }
            catch (Exception ex)
            {
                Console.WriteLine("Invalid Option, please try again");
                printOptions();
                isValidInput = false;
                break;
            }

            switch (selectedOption)
            {
                case 1:
                    isValidInput = true;
                    break;

                case 2:
                    isValidInput = true;
                    break;

                default:
                    Console.WriteLine("Invalid Option, please try again");
                    printOptions();
                    isValidInput = false;
                    break;
            }
        }
        while (!isValidInput);
    }

    if (Debugger.IsAttached) { Console.ReadKey(); }
}

private static void printOptions()
{
    Console.WriteLine("1. Run Intial Data Import");
    Console.WriteLine("2. Nightly Maintenance");
}

Let’s suppose that perfectness is our goal :)

    static void Main(string[] args)
    {
        new Menu("Please select one of the following options to run:")
            .Option("Run Initial Data Import", () => Console.WriteLine("Importing..."))
            .Option("Nightly Maintenance", () => Console.WriteLine("I am the terror (that flaps in the night)!"))
            .Option("Exit")
            .Execute();
    }

Where:

public class Menu
{
    public Menu(string prompt = "Please choose:")
        : this(prompt, Enumerable.Empty<Option>())
    {
    }

    Menu(string prompt, IEnumerable<Option> options)
    {
        Prompt = prompt;
        Options = options;
    }

    string Prompt { get; }
    IEnumerable<Option> Options { get; }

    public Menu Option(string name, Action action = null) =>
        new Menu(Prompt, Options.Concat(new[] { new Option(name, action ?? (() => {})) }));

    public override string ToString() =>
        Prompt + "\n\r" +
        string.Join("\n\r", Options.Select((o, i) => $"{i+1}. {o.Name}"));

    public void Execute() => Execute(Console.Out, Console.In);

    public void Execute(TextWriter writer, TextReader reader)
    {
        writer.WriteLine(this);
        int i;
        if (int.TryParse(reader.ReadLine(), out i) && --i >= 0 && i < Options.Count())
            Options.ElementAt(i).Execute();
        else
            Execute(writer, reader);
    }
}

And

class Option
{
    public Option(string name, Action action)
    {
        Name = name;
        Execute = action;
    }

    public string Name { get; }
    public Action Execute { get; }
}


Student information demo in Swing using MVC I've followed several guides on how to implement MVC in Java, and so far I've come up with the classes shown below. From my understanding, MVC follows these guidelines.

The Model should be independent and unaware of the View and Controller.
The View should wait for interaction from the user and inform the Controller.
The Controller gets and sets information in the Model and updates the View if necessary.
It's possible for the View to reference the Model. (This is what I don't understand).

Model:

public class Student_Model {

    private String fname, lname;

    public String getFirstName(){
        return this.fname;
    }
    public String getLastName(){
        return this.lname;
    }
    public void setFirstName(String newFname){
        this.fname = newFname;
    }
    public void setLastName(String newLname){
         this.lname = newLname;
    }

}

View

public class Student_View {

    public JPanel panel;
    public JTextField student_fname, student_lname;
    public JLabel stu_fname_label, stu_lname_label;
    public JButton changeName;

    public Student_View(){

        panel = new JPanel();
        panel.setPreferredSize(new Dimension(200,200));
        panel.setBackground(Color.red);

        student_fname = new JTextField(20);
        student_lname = new JTextField(20);

        stu_fname_label = new JLabel("First Name: ");
        stu_lname_label = new JLabel("Last Name: ");

        changeName = new JButton("Change student data");

        panel.add(stu_fname_label);
        panel.add(student_fname);
        panel.add(stu_lname_label);
        panel.add(student_lname);
        panel.add(changeName);

        panel.setVisible(true);

    }
}

Controller

public class Student_Controller implements ActionListener {

    Student_Model student_model;
    Student_View student_view;

    public Student_Controller(Student_Model sm, Student_View sv){
        this.student_model = sm;
        this.student_view = sv;

        initActionListeners(this);
    }

    public void actionPerformed(ActionEvent e) {
        student_model.setFirstName("Bill");
        System.out.println(student_model.getFirstName());

    }
    public void initActionListeners(ActionListener al){
        student_view.changeName.addActionListener(al);

    }

}

And finally it's put together in the as such:

public class MVC_Design_Demo extends JFrame {

    public MVC_Design_Demo(){
        setPreferredSize(new Dimension(500,500));
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setResizable(false);
        setLocationRelativeTo(null);
        getContentPane().setBackground(Color.black);

        init();

    }
    public void init(){

        Student_Model newStudent = retrieveStudentInfo();
        Student_View student_view = new Student_View();
        add(student_view.panel);
        Student_Controller student_control = new Student_Controller(newStudent, student_view);

    }

    public static void main(String[] args){
        MVC_Design_Demo mvc = new MVC_Design_Demo();
        mvc.setVisible(true);
        mvc.pack();

    }
    public static Student_Model retrieveStudentInfo(){
        Student_Model sm = new Student_Model();
        sm.setFirstName("John");
        sm.setLastName("Smith");
        return sm;

    }
}

Does this follow the MVC design pattern and are my above assumptions about the guidelines MVC should follow correct?

First of all MVC is not a "real" design pattern as it lacks atomicity. It's something that adresses some points of the SOLID-principles, especially the single responsibility principle. Furthermore you may use design patterns to realize MVC. If you follow the SOLID-principles carefully you will certainly come up with this 3-class struture in your usecase anyway.

Secondly you apply the MVC correctly. It's good to have a starting point that is more concrete to most of the developers as the SOLID-principles. These developers will produce better code if they use this "best practise" as I would call it. This "best practise" provides a good balance between productivity and code quality even though there are many more improvements possible.

One example:
Currently two classes are technology dependent: Student_View and Student_Controller. Both know Swing as the UI technology. It's better to have less classes depending on technology. A naive way is to melt controller and ui to have only one class. But that violates the single responsibility principle (S in SOLID) as the class wil get MORE responsibilities as it has before. We would take a step backwards if we do so. So the task should be to preserve responsibilities AND decrease technology dependencies.

How to do that:

Use the listener pattern! Let the UI implement the Listener-Interface and register it in the controller so the controller does only know "Listeners". The principle applied here is "dependency inversion" (D in SOLID).

What I want to say is: Yes, you did apply MVC correctly. But is this the core? No. Is it the end? No. It's something you will come up with as a intermediate result if you use the real design patterns mentioned in this book (Design Patterns: Elements of reusable Object-Oriented software) AND apply the SOLID principles and the law of demeter correctly.

Some points to your code in detail:

You have public/package scope variables that should be private scope
You tear apart the Button construction as it should contain the ActionListener declaration as well
Use lazy getter instantiation for your UI elements and use these getter only

So you have some issues in encapsulation, locality and dependency.

I provide you an advanced version that meets these requirements.

public class MVC_Design_Demo extends JFrame {

    public MVC_Design_Demo() {

        add(new Student_View());

        setPreferredSize(new Dimension(500, 500));
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setResizable(false);
        setLocationRelativeTo(null);
        getContentPane().setBackground(Color.black);

    }

    public static void main(String[] args) {
        MVC_Design_Demo mvc = new MVC_Design_Demo();
        mvc.setVisible(true);
        mvc.pack();
    }

}

public class Student_Controller {

    private Student_Model newStudent;

    private Set<StudentListener> listeners;

    public Student_Controller() {
        this.newStudent = retrieveStudentInfo();
        this.listeners = new HashSet<>();
    }

    public static Student_Model retrieveStudentInfo() {

        Student_Model sm = new Student_Model();

        sm.setFirstName("John");
        sm.setLastName("Smith");

        return sm;
    }

    public void addListener(StudentListener listener) {
        this.listeners.add(listener);
        listener.registeredAsListener(this.newStudent);
    }

    public void setFirstName(String fName) {
        this.newStudent.setFirstName(fName);
        notifyOnFirstNameChanged();
    }

    private void notifyOnFirstNameChanged() {
        for (StudentListener listener : listeners) {
            listener.onFirstNameChanged(this.newStudent.getFirstName());
        }
    }

}

public class Student_Model {

    private String fname, lname;

    public String getFirstName() {
        return this.fname;
    }

    public String getLastName() {
        return this.lname;
    }

    public void setFirstName(String newFname) {
        // Some business logic
        if (newFname.equals("Bill")) {
            this.fname = newFname + "y";
        } else {
            this.fname = newFname;
        }
    }

    public void setLastName(String newLname) {
        this.lname = newLname;
    }
}

public class Student_View extends Panel implements StudentListener {

    private JTextField student_fname, student_lname;
    private JLabel stu_fname_label, stu_lname_label;
    private JButton changeName;

    private Student_Controller controller;

    public Student_View(){

        setPreferredSize(new Dimension(200,200));
        setBackground(Color.red);
        add(getLabelStuFName());
        add(getTextFieldStudentFName());
        add(getLabelLName());
        add(getTextFieldStudentLName());
        add(getButtonChangeName());

        setVisible(true);

        this.controller = new Student_Controller();
        this.controller.addListener(this);

    }

    private JButton getButtonChangeName() {
        if (changeName == null) {
            changeName = new JButton("Change student data");
            changeName.addActionListener(new ActionListener() {

                @Override
                public void actionPerformed(ActionEvent e) {
                    controller.setFirstName("Bill");
                }

            });
        }
        return changeName;
    }

    private JLabel getLabelLName() {
        if (stu_lname_label == null) {
            stu_lname_label = new JLabel("Last Name: ");
        }
        return stu_lname_label;
    }

    private JLabel getLabelStuFName() {
        if (stu_fname_label == null) {
            stu_fname_label = new JLabel("First Name: ");
        }
        return stu_fname_label;
    }

    private JTextField getTextFieldStudentLName() {
        if (student_lname == null) {
            student_lname = new JTextField(20);
        }
        return student_lname;
    }

    private JTextField getTextFieldStudentFName() {
        if (student_fname == null) {
            student_fname = new JTextField(20);
        }
        return student_fname;
    }

    @Override
    public void registeredAsListener(Student_Model newStudent) {
        this.getTextFieldStudentFName().setText(newStudent.getFirstName());
        this.getTextFieldStudentLName().setText(newStudent.getLastName());
    }

    @Override
    public void onFirstNameChanged(String firstName) {
        this.getTextFieldStudentFName().setText(firstName);
    }

}

public interface StudentListener {

    void registeredAsListener(Student_Model newStudent);

    void onFirstNameChanged(String firstName);

}

Some annotations:

In the model there is now a little business logic to illustrate that you may get an adapted result to present to the ui. ("Bill" will be converted to "Billy")

All former public or package scope variables are private now so you can ensure they will not be manipulated accidentially.

Lazy initialization is introduced for all ui components. The single responsibility principle was applied here. Furthermore, lazy initialization makes the code more flexible as you remove the neccessity to care about the instantiation order/timepoint.

The creation of the ActionListener is now part of the button creation. It was removed from the controller. Previously it was neccessary to have the Button public to access the "addActionListener"-method. The Button is private now. Furthermore you established a "long" dependency to the method "addActionListener". The "law of demeter" was applied, I shortend the dependency. This was the first step to make the controller independent from Swing.

The introduction of the listener pattern is the second step to make the controller independent from Swing. (dependency inversion)

The model informs the listener once when the listener is registered. So the listener can initialize itself.

The model notifies all listeners if the first name changes.

The model is created within the UI class. Now you have some less classes knowing the model. (less dependencies)


Selecting three tables with relationship I've been working on improving my website code.  I've noticed that querying from a table inside a PHP while function, where while loop is also querying another table:

while($row = mysql_fetch_assoc($table1)){
$table2 = mysql_query(select);
mysql_fetch_assoc($table2);
}

It might be slowing down the performance, depending on the fact that we're querying the second table for each row of the first table.

So I've decided to select both of the tables from the same query.  I came up with this code to accomplish what I think is more sufficient.

Now I have few questions:

Am I correct for what I've said about the way my old code is slow?
Would using PHP functions to query from each table individually be more sufficient?
Is there anything that I need to add or change in my code, that I've made?

Code:

SELECT posts.*, users.username, SUM(IF(rates.hash = posts.hash, (rates.likes - rates.dislikes), 0)) AS rate
FROM posts,users,rates WHERE posts.position = posts.submitter AND users.username = posts.submitter GROUP BY posts.hash ORDER BY posts.ID DESC LIMIT 5

I don't really understand what this join is trying to accomplish? Seems redundant since the information is the same, but perhaps with your real world data it is not always the case.

WHERE  posts.position = posts.submitter

Old-style JOINs are not recommended. Use INNER JOIN instead. This may also cause performance issues because rates is not explicitly joined so it would be trying to do a CROSS JOIN and those are very slow not to mention it can mess up your result set.

FROM    posts,
        users,
        rates
WHERE   posts.position = posts.submitter
AND     users.username = posts.submitter

Becomes:

SELECT  posts.*,
        users.username,
        SUM(
        IF(rates.hash = posts.hash, (rates.likes - rates.dislikes), 0)
        ) AS rate
FROM    posts
INNER JOIN users
    ON users.username = posts.submitter
INNER JOIN rates
    ON users.username = rates.username
WHERE   posts.position = posts.submitter
GROUP BY posts.hash
ORDER BY posts.ID DESC LIMIT 5;

I also formatted it for ease of reading. Executes in 2 ms using your data.
Try it: http://sqlfiddle.com/#!2/d198f/4

Edit: fixed a typo.


Problems with backtracking algorithm in C++ I have to implement an algorithm that generates a modified version of a Thue Sequence, being that there can't be any adjacent sub-sequence can repeat itself using the letters N,P and O.

For example, NOPNO is a valid output but NOPNPNO is not because it repeats here NOPNPNO.

I tried to approach the problem using backtracking. My idea is to add a letter and check for repetitions until I get the right length that is required. You can check for more details of the problem here.

This is the code that I came up:

#include <string>
#include <iostream>

using namespace std;

bool found;

/* This function check, for every time a letter is added, if the sequence
 * is going to repeat itself. For each pair of substrings with length i
 * they are compared to check if they are equal.
 * ex:
 *  The sequence NONPNOPN will check if N = P, PN = NO, OPN = NPN, NOPN = NONP
 *
 * The function returns True if it should reject the sequence and False otherwise.
*/
bool reject(string str){
    int i = 1, len = str.size();
    while(i <= len/2){
        if(str.substr(len-2*i,i) == str.substr(len-i,i)) return true;
        i++;
    }
    return false;
}

/* This function uses backtracking to generate the answers. It recursively adds
 * a letter to the string and checks if it is a probable candidate, printing the answer if
 * it's the right answer and returning otherwise
*/
void Thue(int n, string str){
    if(found) return;
    if(reject(str)) return;

    if(str.size() == n){
        cout << str << endl;
        found = true;
        return;
    }

    Thue(n,str+"N");
    Thue(n,str+"O");
    Thue(n,str+"P");
}

int main(){
    int n;
    while(cin >> n && n){
        found = false;
        cin.ignore();
        Thue(n,"");
    }
    return 0;
}

It is working fine but my problem is, it's too slow. I think my problem is the function to check for repetitions but I don't know how I can make it faster. How can I speed this up?

Observation 1

If you find a 5000-length NOP-sequence, an L-length prefix of it would be a valid nop-sequence. Hence computing a full-length nop-sequence once in the beginning and just outputting prefixes (or suffixes) of it will make things quicker.

Observation 2

Generating substrings and checking them will make code slow. Writing your own substring equality check will speed this up. In addition, pass the string by const reference because it is not going to be modified.

bool checkequal(const string &str, int l1, int r1, int l2) {
  for (int i = l1, j = l2; i < r1; ++i, ++j) {
    if (str[i] != str[j])
      return false;
  }
  return true;
}

bool reject(const string &str) {
  int i = 1, len = str.size();
  while (i <= len / 2) {
    if (checkequal(str, len - i, len, len - 2 * i))
      return true;
    i++;
  }
  return false;
}

These two improvements should make your code fast enough to be accepted in contest.


Hex string to Base64 in Haskell This is my take on the first challenge on the cryptopals crypto challenges list. I think that i made it quite simple but i feel like it is possible to optimize it further. Any suggestion?

The code:

import qualified Data.HashMap.Strict as HM
import qualified Data.IntMap.Strict as IM
import Data.Maybe
import Data.Char (digitToInt)
import Numeric (readInt)

type BinString = String
type HexString = String
type Base64String = String

base64 = IM.fromList (zip [0..63] (['A'..'Z'] ++ ['a'..'z'] ++ ['0'..'9'] ++ ['+','/']))

hexToBinDict = HM.fromList [('0',"0000"),('1',"0001"),('2',"0010"),('3',"0011"),('4',"0100"),('5',"0101"),
                                ('6',"0110"),('7',"0111"),('8',"1000"),('9',"1001"),('a',"1010"),('b',"1011"),
                                    ('c',"1100"),('d',"1101"),('e',"1110"),('f',"1111")]

hexToBase64 :: HexString -> Base64String
hexToBase64 xs = map fromJust [IM.lookup x base64 | x <- base10] ++ padMissingSextets sextets
    where base10 = map (fromJust) (map (binToDec) sextets)
          sextets = binToSextets (hexToBin xs)

--returns a list of the 4-digit binary encodings of the given string's hex codes
hexToQuartets :: HexString -> [BinString]
hexToQuartets [] = []
hexToQuartets hexCode = fromJust (HM.lookup (head hexCode) hexToBinDict) : hexToQuartets (tail hexCode)

--converts an hex string into a binary string
hexToBin :: HexString -> BinString
hexToBin xs = concat (hexToQuartets xs)

--pads the string with 0's on the right until its a sextet
padRight :: BinString -> BinString
padRight xs = xs ++ (replicate n '0')
    where n = 6 - length xs

--returns a list of all full sextets on the string
binToSextets :: BinString -> [BinString]
binToSextets xs = binToSextetsAux (tail xs) [] [head xs] 1

--recursiverly find sextets and pad the last one if necessary
binToSextetsAux :: BinString -> [BinString] -> BinString -> Int -> [String]
binToSextetsAux [] sextets ys _
    |length ys == 6     = sextets ++ [ys]
    |otherwise          = sextets ++ [padRight ys]
binToSextetsAux xs sextets ys i
    |i == 6     = binToSextetsAux (tail xs) (sextets ++ [ys]) [head xs] 1
    |otherwise  = binToSextetsAux (tail xs) sextets (ys ++ [head xs]) (i+1)

--binary string to decimal conversion
binToDec :: Integral a => BinString -> Maybe a
binToDec = fmap fst . listToMaybe . readInt 2 (`elem` "01") digitToInt

--pads with "=" on the right for each empty sextet
--if the length is not divisible by 4, then theres 1 or 2 empty sextets
padMissingSextets :: [BinString] -> BinString
padMissingSextets sextets
    |(length sextets) `mod` 4 /= 0      = if (length sextets) `mod` 4 == 2 then "=="
                                            else "="
    |otherwise                          = ""

```

Ok, for starters, it mentions on the first challenge:

Always operate on raw bytes, never on encoded strings. Only use hex and base64 for pretty-printing.

Since String is actually a list of encoded Chars, it would make more sense to migrate to an implementation that uses ByteString instead. This data structure is suited for handling raw data, like we will be doing here. There is also a very helpful parser library called attoparsec. It is a parser combinator library, similar to parsec, but specialized for ByteString. Using it to parse the input as a ByteString makes the most sense in this situation.

Parsing ByteStrings

A ByteString can be formed from [Word8] using pack, and it can also be broken apart again using unpack. Word8 is just an 8-bit unsigned integer, and interestingly, an 8-bit unsigned integer is also what is needed for holding the value a single hex-digit! This means that we can parse two Word8s from '0' to 'f' and get a single Word8 which holds the encoded number.

import Control.Applicative (liftA2, liftA3, many)
import Data.Attoparsec.ByteString.Char8
import qualified Data.ByteString as B
import           Data.ByteString (ByteString, pack, uncons)
import Data.Bits (shift)
import Data.Char (ord)
-- The strict version is suitable here
import Data.IntMap.Strict (IntMap, fromAscList, (!))
import Data.Word8

hexMap :: IntMap Word8
hexMap = fromAscList $ zip (map ord $ ['0'..'9'] ++ ['a'..'f']) [0..]

hex :: Parser Word8
hex = liftA2 mkWord8 hexChar hexChar
  where
    hexChar :: Parser Word8
    hexChar = choice $ map char8 $ ['0'..'9'] ++ ['a'..'f']

    mkWord8 :: Word8 -> Word8 -> Word8
    mkWord8 a b =
          (hexMap ! fromIntegral a) `shift` 4
        + (hexMap ! fromIntegral b)

hexStr :: Parser ByteString
hexStr = fmap pack (many hex)  -- Parser is a Functor

parseHex :: ByteString -> Either String ByteString
parseHex = parseOnly hexStr

Let's take a look at how the hex parser is put together. First, look at the inner functions. mkWord8 takes two Word8 values and looks both of them up in hexMap. Then, it bit shifts the first result by four bits to the left and adds it to the second result. This means that if the first and second arguments to mkWord8 are "f2", then we will have 11110010 stored in the resulting Word8.

hexChar is a Parser that matches a single character in the range of ['0'..'9'] or ['a'..'f']. This works by mapping char8 :: Char -> Parser Word8 over the list of acceptable characters and then calling choice :: [Parser Word8] -> Parser Word8 to indicate that any of the generated parsers will work.

Now, it is easy to understand how hex works. It is a parser that takes two characters and calls mkWord8 on the results, assuming both characters match. To extend hex in order to allow for arbitrary numbers of hex-digits, we just need to utilize many :: Parser Word8 -> Parser [Word8], as in many hex. To actually use hex, we can utilize parseOnly which will run a parser and return either the results or a String containing an error message if it fails.

Converting bytes to Base64

We are halfway to a working solution now that we can parse an incoming hex string into a ByteString. To make it all the way, we need to convert the ByteString into Base64 and print it. This can be done by first converting to something that we are more familiar with, such as decimals, but it may be possible to convert more directly using a little math. Since we know that Base64 uses 6 bits (64 = 2^6) and Word8 uses 8 bits, we can conclude that both formats will line up every 24 bits. When this happens, we can convert 3 bytes into 4 Base64 characters. If there aren't 3 bytes left to use, we will have to utilize as much as we can before giving up.

-- These two functions pull 3 items or 2 items from the front of the ByteString,
-- respectively. (`uncons` pulls one off)

uncons3 :: ByteString -> Maybe ((Word8,Word8,Word8), ByteString)
uncons3 b = do -- We will use the Monad instance of Maybe by using `do`
    (w1,b1) <- uncons b
    (w2,b2) <- uncons b1
    (w3,b3) <- uncons b2
    Just ((w1,w2,w3), b3)

uncons2 :: ByteString -> Maybe ((Word8,Word8), ByteString)
uncons2 b = do
    (w1,b1) <- uncons b
    (w2,b2) <- uncons b1
    Just ((w1,w2), b2)

---

type Base64 = Char -- Might as well use good 'ol Strings again...

toBase64 :: ByteString -> [Base64] -- [Base64] is equivalent to String
toBase64 b =
    case uncons3 b of
        Just ((w1,w2,w3),b') -> from3Bytes w1 w2 w3 ++ toBase64 b' -- loop
        Nothing -> -- Less than 3 bytes left...
            case uncons2 b of
                Just ((w1,w2),b') -> from2Bytes w1 w2
                Nothing -> -- Less than 2 bytes left...
                    case uncons b of
                        Just (w,b') -> from1Byte w
                        Nothing -> [] -- No more data, so end the string

base64Map :: IntMap Base64
base64Map = undefined

-- Convert 3 Word8's to 4 Chars
from3Bytes :: Word8 -> Word8 -> Word8 -> [Base64]
from3Bytes = undefined

-- Convert 2 Word8's to 2 Chars (drop the remainder)
from2Bytes :: Word8 -> Word8 -> [Base64]
from2Bytes = undefined

-- Convert 1 Word8 to 1 Char (drop the remainder)
from1Byte :: Word8 -> [Base64]
from1Byte = undefined

I'll leave it up to you to finish up by filling in the undefined portions. Keep in mind that you can do bitwise manipulations with Data.Bits. Obviously you will need to break up the 8-bit ByteStrings into smaller chunks in order to reassemble them into 6-bit pieces, and have some mapping from these pieces to their Base64 character representation. I left the stub for base64Map as a reminder of one possible solution here.

There's a lot to digest here, but I think this will give you some other strategies to consider. Good luck and happy coding!


Customized lock-free RingBuffer I have implemented my own lock-free ringbuffer for multiple producers multiple consumers using vector. Can you guys help me review to see if there are any problems and ways to improve?

Explanation:
Each producer runs on its own thread and will use Push() to enter its item into the RingBuffer.

Each consumer runs on its own thread and has its own unique number. When consumer TryRead(), it will enter its unique number.

RingBuffer will keep track of the last position each consumer read in std::vector<long> mReadIdxVec

If mReadIdxVec[consumer unique number] >= mMaxReadIdx, TryRead() will return a nullptr.

Question:
Within Push() method, will it be a problem if multiple producers on different threads call this line mVector[index] = pMsg; at the same time?

Can this code be improved further? Any comments are welcomed!

RingBuffer.h

#ifndef __RING_BUFFER_H_
#define __RING_BUFFER_H_

#include <atomic>

class RingBuffer
{
    private:
        std::vector<Foo *> mVector;
        std::atomic<long> mWriteIdx{0}; //mWriteIdx will not exceed 2,000,000,000
        std::atomic<long> mMaxReadIdx{0}; //mMaxReadIdx will not exceed 2,000,000,000
        std::vector<long> mReadIdxVec;

    public:
        RingBuffer(int pNumOfConsumers);
        void Push(Foo *);
        Foo * TryRead(const int&);
};

#endif

RingBuffer.cc

#include "RingBuffer.h"
#define MAX_SIZE 1000

RingBuffer::RingBuffer(int pNumOfConsumers)
{
    mVector.reserve(MAX_SIZE);
    mVector.assign(MAX_SIZE, nullptr);
    mReadIdxVec.assign(pNumOfConsumers, 0);
}

void RingBuffer::Push(Foo * pMsg)
{
    if(!pMsg) return;
    long writeIdx = mWriteIdx++;
    long index = writeIdx % MAX_SIZE;
    if(writeIdx >= MAX_SIZE) delete mVector[index];
    mVector[index] = pMsg;
    mMaxReadIdx++;
}

Foo * RingBuffer::TryRead(const int& pConsumer)
{
    if(mReadIdxVec[pConsumer] >= mMaxReadIdx) return nullptr;
    long index =  mReadIdxVec[pConsumer] % MAX_SIZE;
    mReadIdxVec[pConsumer]++;
    return mVector[index];
}

You have comments in the definition of RingBuffer on the value limits for some members, but there is nothing in the code that enforces that or checks to see if those limits are exceeded.

Why does TryRead take its parameter by const int&?  The reference is unnecessary.  Just pass in the int.

Why is MAX_SIZE a macro?  It should be a constexpr (if your compiler supports it) or static const int if not.

In the RingBuffer constructor, you can combine the reserve and assign calls into one, and include it in the member initializer list.

Push will happily and silently overwrite things that have been added to your ring buffer but not consumed.

The way your Push is set up, you can have multiple threads write to the same memory location (if, for instance, there are enough other calls to Push so that the value calculated for index wraps back around).

It is also possible for mMaxReadIdx to indicate a slot is available for reading when it isn't.  For example, thread A starts a Push, and gets a write index.  Thread B then starts another Push, gets a write index, stores its value and increments mMaxReaddx before thread A has stored its message.  The slot used by thread A will be readable by a TryRead but won't have data in it yet.

TryRead can return the same message to multiple consumers, since each consumer has its own idea of where the bottom of the buffer is.

RingBuffer::RingBuffer(int pNumOfConsumers): mVector(MAX_SIZE, nullptr)


Oneliner to extract External IP from IFCONFIG.CO I wrote a small function in powershell that works fine, however I feel its extremely hacky and I'm sure there must be a smarter way to do this. In the interests of learning, what can I do to improve on this?

function WhatsMyIP
{
return ((Invoke-RestMethod -Uri http://ifconfig.co).split("`n") | where {$_ -match "`"ip`""}).split("<").split(">")[4]
}

I tried this with Invoke-WebRequest but the returned data is the same, except you need to expand the .content property to get at it, making this slightly more clunky than before.

I'd like to do this natively, I realise I can just invoke CURL or what ever but the point is that I am very likely to be on systems where CURL isn't available when I need this.

I don't like the idea of using Invoke-RestMethod here. Normally it would return an object, but here it just returns html as a string, which isn't much use to us. I guess that's because the payload is not XML or JSON, so PowerShell can't turn it into an object for us.

Invoke-WebRequest in contrast will parse the HTML for us, and then we can filter out the element we want:

function WhatsMyIP
{
    $x = Invoke-WebRequest -Uri http://ifconfig.co
    $elt = $x.AllElements | ? { $_.tagName -eq 'CODE' -and $_.class -eq 'ip' }
    $elt.innerText
}

The question mark ? means "where". (It's an alias for Where-Object.) So, what we are doing is we get all the HTML elements on the page, and we filter out all of them except the ones that have a tagName of 'CODE' and a class of 'ip'. (There will only be one of them on this particular page.) We are catching this element: 1.2.3.4.

That's "screen scraping" though, which is a bit crappy. I notice on the web site that they provide JSON if you give them the right URL. Now we can use Invoke-RestMethod:

function WhatsMyIP
{
    $x = Invoke-RestMethod -Uri ifconfig.co/all.json
    $x.'X-Real-Ip'
}

Note that there's no need to use return. Any unconsumed objects will be put in the pipeline. If you don't understand the pipeline, you should read up on it, because it is key to understanding PowerShell.


Concurrent HTTP request loop I'm using the rolling curl library to fire HTTP requests for content-length headers of images (we need to know their size to weed out placeholders and low res images). The image URLs are stored in a database so I need to loop over the data in our products table (approx 1 million rows but will grow bigger, potentially much bigger).

I'm using PHP and the Laravel framework (the artisan CLI component). The operation seems to slow down as time progresses e.g. it starts processing 100 requests in less than a second and later the time to process 100 rows/requests is logged at over 20 seconds. Can anyone explain this and / or offer any performance improvement suggestions? The task is running on an Amazon EC2 micro instance so processing power / memory is limited.

public function fire()
{
    $dt = new DateTime();
    Log::info("started: ".$dt->format('Y-m-d H:i:s'));
    $counter = 1;
    Item2::where('img_size', '=', NULL)->chunk(1000, function($items) use ( &$counter)
    {
        $results = array();
        $filePath = storage_path().'/imports/new/new_img_sizes_'.$counter.'.csv';

        if (!File::exists($filePath)) {
            File::put($filePath, '');
        }
        $start = microtime(true);
        $rollingCurl = new \RollingCurl\RollingCurl();
        $rollingCurl->setOptions($this->curlOptions);

        foreach ($items as $item)
        {
            if ($item->img !== '') {

                $results[$item->id] = array('url' => $item->img, 'size' => null);
                $rollingCurl->get($item->img);
            }

        }
        //callback runs on each curl request
        $rollingCurl->setCallback(function(\RollingCurl\Request $request, \RollingCurl\RollingCurl $rollingCurl) use (&$results, $filePath) {
            $responseInfo = $request->getResponseInfo();
            //var_dump($responseInfo);exit;
            $length = $responseInfo['download_content_length'];

            foreach ($results as $key => $value) {
                if (array_search($request->getURL(),$value)) {
                    $idKey = $key;
                    $results[$idKey]['size'] = $length;
                    File::append($filePath,$idKey.','.$results[$idKey]['size']."\r\n");
                    break;
                }
            }

        })

            ->setSimultaneousLimit(10)
            ->execute();

        $counter++;
        echo 'done in...'.(microtime(true) - $start).PHP_EOL;
        Log::info('1000 records: '.(microtime(true) - $start));
        Log::info('Last url was: '.json_encode(end($results)));
        exit;
    }); // end item chunk

Some benchmarks:

done in...3.8803641796112
done in...7.4326379299164
done in...8.1860301494598
done in...8.5088090896606
done in...10.606615781784
done in...10.655412912369
done in...10.804574966431
done in...14.004528045654
done in...10.903785943985
done in...11.905344009399
done in...13.763195991516
done in...14.723680019379
done in...15.823812961578
done in...17.972007989883
done in...31.734715938568
done in...20.509822845459
done in...22.924754858017
done in...34.274693012238
done in...39.217702865601
done in...29.883662939072
done in...24.094554901123
done in...25.726534128189
done in...31.788655996323
done in...24.713880062103
done in...25.855134963989
done in...23.161122083664
done in...32.380167007446
done in...36.53077507019
done in...31.859884023666
done in...71.458341121674

I don't see what could be causing ever-decreasing performance since the only state outside the main loop is a counter, but adding calls to memory_get_usage as @jsanc623 recommends would help with that.

But you can definitely improve the performance by avoiding the loop over $results for every URL returned. Take advantage of PHP's array which is a hash table providing O(1) lookup.

First, make the URL the key for the results array.

foreach ($items as $item) {
    if ($item->img !== '') {
        $url = $item->img;
        $results[$url] = array(
            'id' => $item->id,
            'url' => $url,
            'size' => null
        );
        $rollingCurl->get($url);
    }
}

Next, use isset instead of array_search and looping.

// foreach ($results as $key => $value) { ... } becomes

$url = $request->getURL();
if (isset($results[$url]) {
    $results[$url]['size'] = $length;
    File::append($filePath, $results[$url]['id'] . ',' . $length . "\r\n");
}

Since you're placing each URL into the array before making the GET request, the if isn't even necessary.

Separately, you can delay writing to the file until the end of the inner loop so you write one big chunk rather than one thousand tiny chunks. It may be that I/O is more costly, and perhaps you're getting killed by seeking to the end of an ever-growing file? Seems unlikely but maybe after a million writes it adds up.

General Review

There are a few things that could be cleaned up here as well.

Consistent formatting: put your opening brace on the same line or the next, but do it consistently.
Blank lines before the closing brace seem misplaced to me. Use blank lines to logically group related code, though functions are preferred to whitespace.
Simplify testing if the image URL exists

if ($item->img !== '')

can be simplified to

if ($item->img)

to skip null values as well. Even if the field cannot be null, it's still easier to read.