Implementing a REST API in Go using gorilla/mux and Gorm While learning Go I decided to implement a simple REST API to get to know the language and since I am inexperienced in Go I would appreciate any feedback on the code.

Please post any thoughts you have. They don't have to be constructive, but if you have some constructive feedback that will be great.

Q: In particular, I would like to ask the next questions: Are there any bad patterns? Is there something you would do another way? What could be improved? Do I violate any code style rules (say, type names, variables)? Is it ok to place all this code in one file?

Note: I used gorilla/mux and gorm here.

type student struct {
    ID   string `gorm:"primary_key" json:"id"`
    Name string `json:"name"`
    Age  int    `json:"age"`
}

type App struct {
    DB *gorm.DB
}

func (a *App) start() {
    db, err := gorm.Open(
        "postgres",
        "user=go password=go dbname=go sslmode=disable")
    if err != nil {
        panic(err.Error())
    }
    a.DB = db
    db.AutoMigrate(&student{})
    r := mux.NewRouter()
    r.HandleFunc("/students", a.getAllStudents).Methods("GET")
    r.HandleFunc("/students", a.addStudent).Methods("POST")
    r.HandleFunc("/students/{id}", a.updateStudent).Methods("PUT")
    r.HandleFunc("/students/{id}", a.deleteStudent).Methods("DELETE")
    http.ListenAndServe(":8080", r)
}

func (a *App) getAllStudents(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    var all []student
    err := a.DB.Find(&all).Error
    if err != nil {
        sendErr(w, http.StatusInternalServerError, err.Error())
    } else {
        json.NewEncoder(w).Encode(all)
    }
}

func (a *App) addStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    s := student{}
    err := json.NewDecoder(r.Body).Decode(&s)
    if err != nil {
        sendErr(w, http.StatusBadRequest, err.Error())
    } else {
        err = a.DB.Save(&s).Error
        if err != nil {
            sendErr(w, http.StatusInternalServerError, err.Error())
        } else {
            w.WriteHeader(http.StatusCreated)
        }
    }
}

func (a *App) updateStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    s := student{}
    err := json.NewDecoder(r.Body).Decode(&s)
    if err != nil {
        sendErr(w, http.StatusBadRequest, err.Error())
    } else {
        s.ID = mux.Vars(r)["id"]
        err = a.DB.Save(&s).Error
        if err != nil {
            sendErr(w, http.StatusInternalServerError, err.Error())
        }
    }
}

func (a *App) deleteStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    s := student{}
    a.DB.First(&s, mux.Vars(r)["id"])
    err := a.DB.Delete(s).Error
    if err != nil {
        sendErr(w, http.StatusInternalServerError, err.Error())
    }
}

func sendErr(w http.ResponseWriter, code int, message string) {
    response, _ := json.Marshal(map[string]string{"error": message})
    w.Header().Set("Content-Type", "application/json")
    w.WriteHeader(code)
    w.Write(response)
}

func main() {
    app := App{}
    app.start()
}

Update: I've got some great feedback and I have an additional question related to that. In both the answers you suggest me to get rid of else blocks / use an early return. Is it a common practice in Go? Is it because of error handling in Go which makes you create more if/else blocks? I am asking because in other languages (such as Java) it is considered a bad practice. The main reason is that it hides complexity. Every time you create a return inside an if block you make a the code more flat hiding the real complexity.

ID as a string is weird. I expect to see an int here to benefit of the auto increment feature.
your delete handler is weak. Suppose the GET ID parameter is set to a non existing ID value, the read operation will fail but the value of the ID property of the student variable will be an empty string. The next delete statement will delete the row with ID=="", which might exists and be a valid. The problem also exists for an integer PK where ID=0. You must check all errors.
the update handler has a similar problem, and it is not acceptable for as long as the api provides both add and update as different operations. Usually an update operation requires to check against a couple PK+LastUpdateDate to ensure acid transactions from UI POV. So, you might try to load the entity from the db, at the cost of an extra read. Or update with clauses. The latter might be done automatically by the db framework, this is unclear without a better knowledge of its internal.
the getAllStudents handler is a ddos. Obviously that always depends of the db size, but for a production system there must not exist any handler that loads the entire db in memory. So add Limit/Offset parameters, check they are valid and apply them to the query.
overall your code has too many level of indentations. You should practice the early return pattern to improve its clarity.

Here is an updated version of the code, so the reader can evaluate the return early pattern and appreciate various modifications as explained before.

Care was taken to demonstrate a proper startup/shutdown sequence to prevent connections misshandling.
http.Server is moved out so the handler is mountable onto alternatives http server implementation (httptest.*)
the db engine is switched to sqlite for demonstration
OP was missing the pgsql driver blank import

here is the code (it is still missing proper test suite!)

package main

import (
    "context"
    "encoding/json"
    "errors"
    "fmt"
    "log"
    "net/http"
    "os"
    "os/signal"
    "strconv"
    "time"

    "github.com/gorilla/mux"
    "github.com/jinzhu/gorm"
    _ "github.com/jinzhu/gorm/dialects/sqlite"
)

type student struct {
    ID   string `gorm:"primary_key" json:"id"`
    Name string `json:"name"`
    Age  int    `json:"age"`

    // http://gorm.io/docs/conventions.html
    // gorm.Model is a basic GoLang struct which includes the following fields: ID, CreatedAt, UpdatedAt, DeletedAt.
    CreatedAt time.Time
    UpdatedAt time.Time
    DeletedAt *time.Time
}

type App struct {
    DB *gorm.DB
}

func (a *App) Handler() http.Handler {
    r := mux.NewRouter()
    r.HandleFunc("/students", a.getAllStudents).Methods("GET")
    r.HandleFunc("/students", a.addStudent).Methods("POST")
    r.HandleFunc("/students/{id}", a.updateStudent).Methods("PUT")
    r.HandleFunc("/students/{id}", a.deleteStudent).Methods("DELETE")
    return r
}

func (a *App) Open() error {
    db, err := gorm.Open("sqlite3", ":memory:")
    if err != nil {
        return err
    }
    a.DB = db
    return db.AutoMigrate(&student{}).Error
}

func (a *App) Close() error {
    return a.DB.Close()
}

func (a *App) getAllStudents(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    // see also a struct decoder to transform get params as struct.
    // https://github.com/gorilla/schema
    var offset, limit int
    {
        y := r.URL.Query().Get("Limit")
        if y != "" {
            x, err := strconv.Atoi(y)
            if err != nil {
                sendErr(w, fmt.Errorf("Limit argument, %v", err), http.StatusBadRequest)
                return
            }
            limit = x
        }
        y = r.URL.Query().Get("Offset")
        if y != "" {
            x, err := strconv.Atoi(y)
            if err != nil {
                sendErr(w, fmt.Errorf("Offset argument, %v", err), http.StatusBadRequest)
                return
            }
            offset = x
        }
    }
    if limit < 0 || limit > 30 {
        limit = 30
    }
    var all []student
    err := a.DB.Find(&all).Limit(limit).Offset(offset).Error
    if err != nil {
        sendErr(w, err, http.StatusInternalServerError)
        return
    }
    json.NewEncoder(w).Encode(all)
}

func (a *App) addStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    s := student{}
    err := json.NewDecoder(r.Body).Decode(&s)
    if err != nil {
        sendErr(w, err, http.StatusBadRequest)
        return
    }
    err = a.DB.Create(&s).Error
    if err != nil {
        sendErr(w, err, http.StatusInternalServerError)
        return
    }
    w.WriteHeader(http.StatusCreated)
}

func (a *App) updateStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    s := student{}
    err := json.NewDecoder(r.Body).Decode(&s)
    if err != nil {
        sendErr(w, err, http.StatusBadRequest)
        return
    }
    id := mux.Vars(r)["id"]
    if s.ID != id {
        sendErr(w, errors.New("invalid ID"), http.StatusBadRequest)
        return
    }
    s.ID = id                 // Why not read it from Body directly ?
    err = a.DB.Save(&s).Error //gorm does a proper job http://gorm.io/docs/update.html
    sendErr(w, err, http.StatusInternalServerError)
}

func (a *App) deleteStudent(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json")
    id := mux.Vars(r)["id"]
    s := student{}
    err := a.DB.First(&s, id).Error
    if err != nil {
        sendErr(w, err)
        return
    }
    err = a.DB.Delete(s).Error
    sendErr(w, err)
}

func sendErr(w http.ResponseWriter, err error, codes ...int) {
    if err == nil {
        return
    }
    if len(codes) < 1 {
        codes = append(codes, http.StatusInternalServerError)
    }
    response, _ := json.Marshal(map[string]string{"error": err.Error()})
    w.Header().Set("Content-Type", "application/json")
    http.Error(w, string(response), codes[0])
}

func main() {
    app := App{}
    if err := app.Open(); err != nil {
        log.Fatal("open failed", err)
    }
    defer app.Close()

    srv := &http.Server{
        Addr:    ":8080",
        Handler: app.Handler(),
    }

    err := make(chan error)
    go func() {
        err <- srv.ListenAndServe()
    }()

    select {
    case e := <-err:
        log.Fatal("listen failed", e)
    case <-time.After(time.Millisecond * 200):
        log.Println("app started at ", srv.Addr)
    }

    sigint := make(chan os.Signal, 1)
    signal.Notify(sigint, os.Interrupt)
    <-sigint
    log.Println("got signal")

    if err := srv.Shutdown(context.Background()); err != nil {
        // Error from closing listeners, or context timeout:
        log.Printf("HTTP server Shutdown: %v", err)
    }
    log.Println("properly shutdown")
}


Using Roslyn to find interfaces within a solution The intent of the below code is to find all interfaces within a solution whose fully-qualified names match the given predicate. It seems to work, but as I am new to Roslyn I'm guessing there are things that could be improved. I'd appreciate any pointers.

public async Task<IImmutableList<INamedTypeSymbol>> GetMatchingInterfacesInSolution(string solutionPath, Func<string, bool> predicate)
{
    var workspace = MSBuildWorkspace.Create();
    var solution = await workspace.OpenSolutionAsync(solutionPath);
    var compilations = await Task.WhenAll(solution.Projects.Select(x => x.GetCompilationAsync()));

    return compilations
        .SelectMany(x => x.SyntaxTrees.Select(y => new { Compilation = x, SyntaxTree = y }))
        .Select(x => x.Compilation.GetSemanticModel(x.SyntaxTree))
        .SelectMany(
            x => x
                .SyntaxTree
                .GetRoot()
                .DescendantNodes()
                .OfType<InterfaceDeclarationSyntax>()
                .Select(y => x.GetDeclaredSymbol(y)))
        .Where(x => predicate(x.ToDisplayString()))
        .ToImmutableList();
}

Here's another approach I tried. I couldn't really see any benefit to doing it this way. It's more verbose and difficult to follow:

public static async Task<IImmutableList<INamedTypeSymbol>> GetMatchingInterfacesInSolution(string solutionPath, Func<string, bool> predicate)
{
    var workspace = MSBuildWorkspace.Create();
    var solution = await workspace.OpenSolutionAsync(solutionPath);
    var compilations = await Task.WhenAll(solution.Projects.Select(x => x.GetCompilationAsync()));

    return compilations
        .Select(x => x.Assembly.GlobalNamespace)
        .SelectMany(x => FindInterfacesRecursive(x, predicate))
        .ToImmutableList();
}

private static IEnumerable<INamedTypeSymbol> FindInterfacesRecursive(INamespaceSymbol @namespace, Func<string, bool> predicate)
{
    foreach (var member in @namespace.GetMembers())
    {
        var childNamespace = member as INamespaceSymbol;

        if (childNamespace != null)
        {
            foreach (var @interface in FindInterfacesRecursive(childNamespace, predicate))
            {
                yield return @interface;
            }

            continue;
        }

        var namedType = member as INamedTypeSymbol;

        if (namedType == null || namedType.TypeKind != TypeKind.Interface)
        {
            continue;
        }

        if (predicate == null || predicate(namedType.ToDisplayString()))
        {
            yield return namedType;
        }
    }
}

Name captured variables as you would name local variables.
First SelectMany and Select can be merged into a single SelectMany.

code:

public async Task<IImmutableList<ISymbol>> GetMatchingInterfacesInSolution(string solutionPath, Func<string, bool> predicate)
{
    var workspace = MSBuildWorkspace.Create();
    var solution = await workspace.OpenSolutionAsync(solutionPath);
    var compilations = await Task.WhenAll(solution.Projects.Select(x => x.GetCompilationAsync()));

    return compilations
        .SelectMany(compilation => compilation.SyntaxTrees.Select(syntaxTree => compilation.GetSemanticModel(syntaxTree)))
        .SelectMany(
            semanticModel => semanticModel
                .SyntaxTree
                .GetRoot()
                .DescendantNodes()
                .OfType<InterfaceDeclarationSyntax>()
                .Select(interfaceDeclarationSyntax => semanticModel.GetDeclaredSymbol(interfaceDeclarationSyntax)))
        .Where(symbol => predicate(symbol.ToDisplayString()))
        .ToImmutableList();
}


VBA Function slower than Excel Formula I've replaced the following horrendous Excel formula (values and names have been replaced):

=IF(AND(SourceTable[@Field1]="Value1",SourceTable[@Field2]="Value2"),"Result1",
    IF(SourceTable[@Field2]="Value3","Result2",
        IF(AND(SourceTable[@Field1]="Value4",SourceTable[@Field3]="Value5"),"Result3",
            IF(AND(SourceTable[@Field1]="Value6",OR(SourceTable[@Field3]="Value7",SourceTable[@Field3]="Value8")),"Value9",
                VLOOKUP(SourceTable[@Field1];MatchingTable;2;0)))))

With a VBA Function so it would be easier to read and supposedly faster:

Function CUSTOMFUNCTION(oRngField1 As Range, oRngField2 As Range, oRngField3 As Range) As String

    'If imbrication
    If (oRngField1 = "Value1" And oRngField2 = "Value2") Then
        CUSTOMFUNCTION = "Result1"
        Exit Function
    ElseIf oRngField2 = "Value3" Then
        CUSTOMFUNCTION = "Result2"
        Exit Function
    ElseIf oRngField1 = "Value4" Then
        If oRngField3 = "Value5" Then
            CUSTOMFUNCTION = "Result3"
            Exit Function
        ElseIf (oRngField3 = "Value6" Or oRngField3 = "Value7") Then
            CUSTOMFUNCTION = "Result4"
            Exit Function
        End If
    End If

    'Vlookup if no match before
    Dim sTableMatchingFieldLookup As String
    Dim sTableMatchingFieldResult As String
    sTableMatchingFieldLookup = "MatchingTable[Field1]"
    sTableMatchingFieldResult = "MatchingTable[Field2]"
    CUSTOMFUNCTION = Range(sTableMatchingFieldResult)(Application.Match(oRngField1, Range(sTableMatchingFieldLookup), 0))

End Function

I get the same result as the function however it does it two times longer than the formula when working with 2k rows. Why isn't the code faster than the Excel formula? Can it be improved? The file is expected to grow so it means the code will be of no use.

Edit: Using more arguments improved the macro, see my response below.

I'm assuming that names have been changed to protect the innocent, but just in case they've not been...

These are terrible names. All of them.

Function names should have verb-noun names and be PascalCased. So instead of CUSTOMFUNCTION should be GetCustomMatch (or something like that. I can't say because you've ommitted any actual names...)
Parameters should be camelCase and tell me what kind of data
should be passed in and NEVER should they be numbered the way
this is. So, maybe this instead (again, I have to guess because I
have no clue):

Function GetCustomMatch(LookupRange As Range, lastYearsSalesRange as Range, iJustDontKnowWhatThisIsRange as Range) As String

Don't use hungarian notation. Just... please don't. It should be
killed with fire and buried out back.

Other Stuff

Some of your "Values" are used in multiple places. Store them in a
constant and use that constant anywhere they show up. (Of course,
use meaningful names for them.)
I have a feeling that sTableMatchingFieldLookup and ...Result should be actual ranges instead of strings. String typing is bad. If you insist on keeping them as strings, store the values as constants.
You should be able to pick up some performance by setting Application.Calculation = xlCalculationManual. This Stack Overflow answer has a pretty good example of what you're trying to do.

Things you did right

You exit early if you've already found a match before performing the lookup rather than nesting if statements.


Linked list in Python Below is my attempt at a doubly-linked list in Python (3):

class LinkedListNode:
    def __init__(self, value):
        self.value = value
        self.left  = None
        self.right = None

class LinkedList:
    def __init__(self):
        self.head = self.tail = None
        self.size = 0

    def get_first(self):
        if self.head:
            return self.head.value
        raise Exception('Reading from an empty list.')

    def get_last(self):
        if self.tail:
            return self.tail.value
        raise Exception('Reading from an empty list.')

    def add_first(self, value):
        node = LinkedListNode(value)
        if self.head:
            node.right = self.head
            self.head.left = node
            self.size += 1
        else:
            self.head = self.tail = LinkedListNode(value)
            self.size = 1

    def add_last(self, value):
        node = LinkedListNode(value)
        if self.tail:
            node.left = self.tail
            self.tail.right = node
            self.size += 1
        else:
            self.head = self.tail = node
            self.size = 1

    def remove_first(self):
        if self.head:
            ret = self.head.value
            self.head = self.head.right
            self.head.left = None
            self.size -= 1
            return ret
        raise Exception('Removing from an empty list.')

    def remove_last(self):
        if self.tail:
            ret = self.tail.value
            self.tail = self.tail.left
            self.tail.right = None
            self.size -= 1
            return ret
        raise Exception('Removing from an empty list.')

    def __repr__(self):
        str = '['
        if self.size > 0:
            str += self.head.value
            node = self.head.right
            while node:
                str += ', ' + node.value
                node = node.right
        return str + ']'

def main():
    list = LinkedList()

    while True:
        cmd_line = input()
        if cmd_line == 'quit':
            break
        command_tokens = cmd_line.split()
        command = command_tokens[0]
        try:
            if command == "get_first":
                print(list.get_first())
            elif command == "get_last":
                print(list.get_last())
            elif command == "add_first":
                list.add_first(command_tokens[1])
            elif command == "add_last":
                list.add_last(command_tokens[1])
            elif command == "remove_first":
                list.remove_first()
            elif command == "remove_last":
                list.remove_last()
            elif command == "print":
                print(list)
        except Exception:
            print("Unknown command")

main()

How would I rewrite it in a professional manner?

A better main

Instead of just calling main() at the bottom, wrap it in if __name__ == "__main__":. In otherwords:

if __name__ == "__main__":
    main()

This allows me to re-use your module better, see the answer I linked for more details.

if chains

This portion of code:

        if command == "get_first":
            print(list.get_first())
        elif command == "get_last":
            print(list.get_last())
        elif command == "add_first":
            list.add_first(command_tokens[1])
        elif command == "add_last":
            list.add_last(command_tokens[1])
        elif command == "remove_first":
            list.remove_first()
        elif command == "remove_last":
            list.remove_last()
        elif command == "print":
            print(list)

Is a code smell to me. I would refactor this with a dictionary:

commands = {
    "get_first": lambda: print(list.get_first()),
    "get_last": lambda: prrint(list.get_last()),
    ...
}

This can then be called with:

commands[command]()

Library Shadowing

list = LinkedList()

list is already part of the standard library. I would not over shadow the name. I would change the name to something like linked for instance.

namedtuple?

It is questionable whether you want to do this or not, but the class LinkedListNode is just an __init__ method. In this case, I would usually advocate using a namedtuple instead and using the defaults parameter added in Python 3.7 if applicable. Although, then again, defaults is only in 3.7... So, maybe not applicable. Still useful to know about.

Implement __str__ how you implemented __repr__

__str__ is for human-readable output. __repr__ is for more internal information. You have implemented __repr__ to display a human readable representation of your list. Switch to __str__ instead.


Bot that selects web reviews based on spreadsheet entries In my program, I prompt the user to enter a range of cities (in a spreadsheet), and then a range of review scores, and then make some decisions based on what the user has entered. Since the handling of the both user inputs (both ranges) is common to each, I have combined both into the single function getRanges() below. It works well, but I get the sense that can be improved (shortened, and with fewer if/else's). In the absence of a switch statement in Python, what are some neater approaches here?

# -*- coding: utf-8 -*-
from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.common.keys import Keys
from selenium.webdriver.support.ui import Select
from selenium.common.exceptions import NoSuchElementException
from selenium.common.exceptions import NoAlertPresentException
from selenium.common.exceptions import ElementNotVisibleException
from selenium.common.exceptions import WebDriverException
import unittest, time, re
from functions import *
from collections import OrderedDict
import csv
from openpyxl import load_workbook
from easygui import enterbox, msgbox
import traceback

class bot(unittest.TestCase):
    def setUp(self):
        self.wb = load_workbook("Spreadsheet.xlsx", data_only=True)
        self.cities = []
        self.getRanges('cities')
        self.getRanges('reviews')
        print 'Running...'

        try:
            self.driver = webdriver.Firefox()
        except WebDriverException, e:
            print "Unable to load profile, retrying"
            try:
                self.driver = webdriver.Firefox()
            except WebDriverException, e:
                print "Unable to load profile, retrying"
                self.driver = webdriver.Firefox()
        self.driver.implicitly_wait(30)
        self.base_url = "https://www.google.com/ncr"
        self.verificationErrors = []
        self.accept_next_alert = True
        self.reviewPattern = re.compile(ur'\d+')
        self.niche = "plumbers+"

    def getRanges(self, citiesOrReviews):
        self.LowerLimit = enterbox("Enter a lower limit for %s, or 'all'" % citiesOrReviews, 'Range')
        if self.LowerLimit.lower().strip("'") == "all":
            self.reviewRange = False
            if citiesOrReviews.lower() == 'cities':
                for row in self.wb.active.rows:
                    self.cities.append(row[0].value)
        elif self.LowerLimit.isdigit():
            if citiesOrReviews.lower() == 'cities':
                if int(self.LowerLimit) > len(self.wb.active.rows):
                    msgbox("The lower limit you've entered is higher than the number of cities in the spreadsheet. Let's retry.", 'Error')
                    self.getRanges(citiesOrReviews)
            self.UpperLimit = enterbox("Enter an upper limit for %s:" % citiesOrReviews, "Range")
            if self.UpperLimit.isdigit():
                if citiesOrReviews.lower() == 'cities':
                    if int(self.UpperLimit) > len(self.wb.active.rows):
                        msgbox("The upper limit you've entered is higher than the number of cities in the spreadsheet. Let's retry.", "Error")
                        self.getRanges(citiesOrReviews)
                    if self.UpperLimit <= self.LowerLimit:
                        msgbox("The upper and lower limits are identical, or the upper limit is less than the lower. Let's retry.", "Error")
                        self.getRanges(citiesOrReviews)
                    else:
                        if citiesOrReviews.lower() == 'cities':
                            for row in self.wb.active.rows[int(self.LowerLimit) - 1:int(self.UpperLimit)]:
                                self.cities.append(row[0].value)
                elif citiesOrReviews.lower() == 'reviews':
                    self.reviewRange = [int(self.LowerLimit) - 1, int(self.UpperLimit)]
        else:
            msgbox("The value entered must either be 'all', or a number. Let's retry.", "Error")
            self.getRanges(citiesOrReviews)

I will only comment on the "getting rid of the repeating ifs part".  A way to avoid writing the same condition in different places is to move the check entirely into a separate piece of code.  Some may refer to this as "state pattern".  Below is an example:

class StateExample(object):

    class State(object):

        def __init__(self, example):
            self.example = example

        def __str__(self):
            return self.__class__.__name__

    class Cities(State):

        def query_limits(self):
            self.example.do_one_thing(self)

    class Reviews(State):

        def query_limits(self):
            self.example.do_something_else(self)

    def __init__(self):
        self.state_factories = {
            'cities': self.Cities,
            'reviews': self.Reviews
        }

    def do_one_thing(self, state):
        print 'doing one thing with %s' % state

    def do_something_else(self, state):
        print 'doing something else with %s' % state

    def interact(self, city_or_review):
        self.state = self.state_factories[city_or_review](self)

    def interact_more(self):
        self.state.query_limits()

And an example usage, to illustrate the usage:

>>> example = StateExample()
>>> example.interact('cities')
>>> example.interact_more()
doing one thing with Cities
>>> example.interact('reviews')
>>> example.interact_more()
doing something else with Reviews
>>>

You may or may not need all of the machinery here.  The important part is to notice how this would be equivalent to testing repeatedly for the same condition inside interact and interact_more, which in the example above happens only once.

You can read more about this technique here: https://en.wikipedia.org/wiki/State_pattern

As an aside: it is not necessary to use classes and objects to achieve the same goal, but I want to keep this answer short.


Skyline problem solved by sweep line The Skyline problem from UVa Online Judge is as follows:

  You are to design a program to assist an architect in drawing the skyline of a city given the locations of the buildings in the city. A building is specified by an ordered triple \$ (L_i,H_i,R_i) \$ where \$ L_i \$ and \$ R_i \$ are left and right coordinates, respectively, of building \$ i \$ and \$ H_i \$ is the height of the building.

  The input is a sequence of building triples. All coordinates of buildings are integers less than 10,000 and there will be at least one and at most 5,000 buildings in the input file. Each building triple is on a line by itself in the input file. All integers in a triple are separated by one or more spaces. The triples will be sorted by \$ L_i \$, the left x-coordinate of the building, so the building with the smallest left x-coordinate is first in the input file.

  The output should consist of the skyline vector \$ (v_1, v_2, v_3, \ldots, v_{n−2}, v_{n−1}, v_n)\$, the \$ v_i \$ such that \$ i \$ is an even number represent a horizontal line (height). The \$ v_i \$ such that \$ i \$ is an odd number represent a vertical line (x-coordinate). The skyline vector should represent the “path” taken, for example, by a bug starting at the minimum x-coordinate and traveling horizontally and vertically over all the lines that define the skyline. Thus the last entry in all skyline vectors will be a 0.

This is my solution:

# [SortedDict](http://www.grantjenks.com/docs/sortedcontainers/sorteddict.html)
from sortedcontainers import SortedDict # pip3 install sortedcontainers
from collections import deque
import heapq

def skyline(buildings_by_L): # a deque.
    buildings_by_H = SortedDict() # a self-balancing binary tree.
    buildings_by_R = [] # a priority queue.
    skyline = []
    def add_point(x):
        h = buildings_by_H.iloc[-1] if buildings_by_H else 0
        if not skyline:
            skyline.append((x, h))
        else:
            x0, h0 = skyline[-1]
            if h != h0:
                if x == x0:
                    skyline[-1] = (x, max(h, h0))
                else:
                    skyline.append((x, h))
    def insert(b, sets=[set()]):
        heapq.heappush(buildings_by_R, (b[2], b))
        s = buildings_by_H.setdefault(b[1], sets[0])
        s.add(b)
        if s == sets[0]:
            sets[0] = set()
        add_point(b[0])
    def delete(b):
        buildings_by_H[b[1]].remove(b)
        if not buildings_by_H[b[1]]:
            del buildings_by_H[b[1]]
        add_point(b[2])
    while buildings_by_L or buildings_by_R: # another sweep line algorithm
        if (not buildings_by_R
            or buildings_by_L
               and buildings_by_L[0][0] <= buildings_by_R[0][0]):
            insert(buildings_by_L.popleft())
        else:
            delete(heapq.heappop(buildings_by_R)[1])
    return skyline

Test Cases:

buildings = (
    (1,9,3), (1,11,5), (2,6,7), (3,13,9), (12,7,16),
    (14,3,25), (19,18,22), (23,13,29), (24,4,28))
assert ([(1, 11), (3, 13), (9, 0), (12, 7), (16, 3), (19, 18), (22, 3), (23, 13), (29, 0)]
    == skyline(deque(buildings)))

Can you please suggest more elegant and eloquent ways for this problem? (Perhaps better ways to represent a graph with vertices and edges?)

1. Bug

If the city contains two identical buildings then skyline raises an exception:

>>> skyline(deque([(1, 2, 3), (1, 2, 3)]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr66776.py", line 97, in skyline
    delete(heapq.heappop(buildings_by_R)[1])
  File "cr66776.py", line 87, in delete
    buildings_by_H[b[1]].remove(b)
KeyError: 2

This is because the code represents buildings by tuples of numbers, and that means that two identical buildings get stored just once in a set, so when you come to remove the second building, it's not there. To fix this, buildings need to be represented such that each building is unique. One way to do this would be to use a Building class, as objects are always unique unless they implement an __eq__ method.

2. Review

It's not clear to the reader how the data is represented. The variable b represents a building, but what are b[0], b[1] and b[2]? These are the building's left x-coordinate, height, and right x-coordinate respectively, so it would make the code a lot easier to read if these were b.left, b.height, and b.right. It's easy to do this using a class, or collections.namedtuple.
It's not clear exactly what the helper functions add_point, insert, delete do. In particular, add_point doesn't always add a point to the skyline list: sometimes it updates the last point in the list. The insert function adds a building to the buildings_by_R and buildings_by_H collections, but also calls add_point. These functions could do with comments explaining what exactly it is that they do.
Some of the operations could do with a comment to explain their purpose. For example here:

h = buildings_by_H.iloc[-1] if buildings_by_H else 0

It would be worth noting that this finds the height of the highest building currently standing.
The insert and delete helper functions are only called once, so there's no particular advantage to making them into functions. The code might be clearer if these functions were inlined at the point of use.
The insert function contains the following lines of code that manipulate the vaguely named sets parameter:

def insert(b, sets=[set()]):
    s = buildings_by_H.setdefault(b[1], sets[0])
    s.add(b)
    if s == sets[0]:
        sets[0] = set()

It looks as if there was a concern that if the code were written in the obvious way:

def insert(b):
    buildings_by_H.setdefault(b[1], set()).add(b)

then a set() object would be created and immediately discarded each time a building was inserted at a position for which buildings_by_H already had an entry. This is a valid concern, but the following would seem to be a clearer way to handle it:

def insert(b):
    try:
        s = buildings_by_H[b[1]]
    except KeyError:
        buildings_by_H[b[1]] = s = set()
    s.add(b)

It's worth making a little effort to avoid using libraries that are not built into Python. Having to install sortedcontainers is a small but genuine barrier to running code (and might partly account for the time this question has languished without being reviewed). In this case, SortedDict is used to maintain the collection of currently standing buildings, such that (i) the height of the highest building can be found via buildings_by_H.iloc[-1]; and (ii) the set of buildings at a given height can by located via buildings_by_H[b[1]] and so a building can be removed.

However, we can implement (i) by storing the buildings in a heap, because as it says in the heapq documentation:

  The interesting property of a heap is that its smallest element is always the root, heap[0].

Python heaps are min-heaps but we want the highest building, so we have to reverse the comparison order on buildings. If buildings are represented by objects, as recommended in §1 above, then this can be done by implementing a __lt__ method on the Building class.

How do we implement (ii)? You can't easily delete elements from a heap. The insight here is that we don't have to delete the building from the heap, we can leave it in the heap but mark it as finished (if buildings are represented by objects, as recommended in §1, above then this can be done by setting an attribute). We only care about the highest standing building, which is at the root of the heap, so we can just check that one to see if it is finished and pop it if so.
There's a lot of complexity in the add_point function. This complexity is due to the fact that when add_point(x) is called, we don't know whether or not we've processed all the buildings that start or end at x, so when we construct a point (x, h) we don't know whether we're done with it, or whether we might have to revise it later.

It would simplify this code considerably if we could be sure that all the buildings that start or end at x have been processed. Then we would know that the point would not need to be revised, and we could just yield it.

3. Revised code

from collections import defaultdict, namedtuple
from heapq import heappop, heappush

class Building:
    def __init__(self, height):
        self.height = height
        self.finished = False

    def __lt__(self, other):
        # Reverse order by height, so that when we store buildings in
        # a heap, the first building is the highest.
        return other.height < self.height

# An event represents the buildings that start and end at a particular
# x-coordinate.
Event = namedtuple('Event', 'start end')

def skyline(buildings):
    """Given an iterable of buildings represented as triples (left, height,
    right), generate the co-ordinates of the skyline.

    >>> list(skyline([(1,9,3), (1,11,5), (2,6,7), (3,13,9), (12,7,16),
    ...               (14,3,25), (19,18,22), (23,13,29), (24,4,28)]))
    ... # doctest: +NORMALIZE_WHITESPACE
    [(1, 11), (3, 13), (9, 0), (12, 7), (16, 3), (19, 18), (22, 3),
     (23, 13), (29, 0)]
    >>> list(skyline([(1, 3, 2), (1, 3, 2)]))
    [(1, 3), (2, 0)]

    """
    # Map from x-coordinate to event.
    events = defaultdict(lambda:Event(start=[], end=[]))
    for left, height, right in buildings:
        b = Building(height)
        events[left].start.append(b)
        events[right].end.append(b)

    standing = []               # Heap of buildings currently standing.
    last_h = 0                  # Last emitted skyline height.

    # Process events in order by x-coordinate.
    for x, event in sorted(events.items()):
        # Update buildings currently standing.
        for b in event.start:
            heappush(standing, b)
        for b in event.end:
            b.finished = True

        # Pop any finished buildings from the top of the heap.
        while standing and standing[0].finished:
            heappop(standing)

        # Top of heap (if any) is the highest standing building, so
        # its height is the current height of the skyline.
        h = standing[0].height if standing else 0

        # Yield co-ordinates if the skyline height has changed.
        if h != last_h:
            yield x, h
            last_h = h

4. Performance comparison

Test case (this is an order of magnitude bigger than the largest case specified in the problem description):

>>> from timeit import timeit
>>> import random
>>> R = lambda n:random.randrange(n) + 1
>>> buildings = sorted((x, R(10000), x + R(1000)) for x in (R(10000) for _ in range(50000)))

Original code:

>>> queue = deque(buildings)
>>> timeit(lambda:skyline(queue), number=1)
1.46421534800902

Revised code:

>>> timeit(lambda:list(skyline(buildings)), number=1)
0.9026356039103121

The revised code is about 40% faster than the original.


Deepest node in tree I have written two classes, one to represent the tree and a Util class to get the deepest node from the tree. Can you please review the code?

Below is the class for Tree:

package org.vik.ds.tree;

public class BinaryTree<E>{

private E data;
private BinaryTree<E> leftNode;
private BinaryTree<E> rightNode;

public BinaryTree(E data, BinaryTree<E> leftNode, BinaryTree<E> rightNode)
{
    this.data = data;
    this.leftNode = leftNode;
    this.rightNode = rightNode;
}

public E getData()
{
    return data;
}

public void setData(E data)
{
    this.data = data;
}

public BinaryTree<E> getLeftNode()
{
    return leftNode;
}

public void setLeftNode(BinaryTree<E> leftNode)
{
    this.leftNode = leftNode;
}

public BinaryTree<E> getRightNode()
{
    return rightNode;
}

public void setRightNode(BinaryTree<E> rightNode)
{
    this.rightNode = rightNode;
}

public boolean isLeafNode()
{
    return (leftNode == null && rightNode == null);

}

}

Below is algo class, which finds deepest node in tree:

public class DeepestNodeInTree{

public static <T> BinaryTree<T> getDeepestNode(BinaryTree<T> rootNode)
{
    BinaryTree<T> deepestNode = rootNode;
    int maxDeeper = 0;

    Deque<BinaryTree<T>> nodeStack = new LinkedList<BinaryTree<T>>();

    BinaryTree<T> tempNode = rootNode;
    Map<BinaryTree<T>, Boolean> isNodeVisitedMap = new HashMap<>();
    while (tempNode != null)
    {

        if (tempNode.isLeafNode())
        {
            int currentMaxDeeperLen = nodeStack.size() - 1;
            if (currentMaxDeeperLen > maxDeeper)
            {
                deepestNode = tempNode;
                maxDeeper = currentMaxDeeperLen;

            }

            isNodeVisitedMap.put(tempNode, true);
            tempNode = nodeStack.isEmpty() ? nodeStack.pop() : null;
        }
        else
        {

            BinaryTree<T> notVisitedChildNode = getNonVisitedChildNode(tempNode, isNodeVisitedMap);
            if (notVisitedChildNode != null)
            {
                nodeStack.push(tempNode);
                tempNode = notVisitedChildNode;
            }
            else
            {
                isNodeVisitedMap.put(tempNode, true);
                tempNode = nodeStack.isEmpty() ? nodeStack.pop() : null;

            }
        }
    }
    return deepestNode;
}

/**
 * This will get left or right node, if both node is visited it will return
 * null
 *
 * @param tempNode
 * @param isNodeVisitedMap
 * @return
 */
private static <T> BinaryTree<T> getNonVisitedChildNode(BinaryTree<T> tempNode,
        Map<BinaryTree<T>, Boolean> isNodeVisitedMap)
{
    BinaryTree<T> childNode = tempNode.getLeftNode();

    if (isNodeVisited(isNodeVisitedMap, childNode))
    {
        childNode = isNodeVisited(isNodeVisitedMap, tempNode.getRightNode()) ? null : tempNode.getRightNode();
    }
    return childNode;
}

private static <T> boolean isNodeVisited(Map<BinaryTree<T>, Boolean> isNodeVisitedMap, BinaryTree<T> childNode)
{
    return childNode == null || isNodeVisitedMap.containsKey(childNode);
}
}

Your braces do not follow the Standard Java Conventions. The standard way is:

public BinaryTree(E data, BinaryTree<E> leftNode, BinaryTree<E> rightNode) {
    this.data = data;
    this.leftNode = leftNode;
    this.rightNode = rightNode;
}

Notice the movement of the braces.

You should have a constructor for your BinaryTree that takes no parameters and one that takes only the data:

public BinaryTree() {

}

public BinaryTree(E data) {
    this.data = data;
}

public BinaryTree(E data, BinaryTree<E> leftNode, BinaryTree<E> rightNode) {
    this.data = data;
    this.leftNode = leftNode;
    this.rightNode = rightNode;
}

This way, programmers using it has a choice of being able to give less information, and add it later through the set() methods. Also, this allows them to prevent them from doing stuff like:

BinaryTree<Integer> tree = new BinaryTree<Integer>(null, null, null);

After all, who wants to do that?

I don't see any point in making a separate class for finding the deepest node int the tree. Why not just put it under one class?

There is a better way to do it with recursion:

public BinaryTree<E> getDeepestNode(BinaryTree<E> rootNode) {
    if (rootNode.leftNode.isLeafNode() && rootNode.rightNode.isLeafNode()) {
        return rootNode;
    }
    return getDeepestNode(rootNode.getLeftNode(), 0) > getDeepestNode(
            rootNode.getRightNode(), 0) ? getDeepestNode(rootNode
            .getLeftNode()) : getDeepestNode(rootNode.getRightNode());
}

private int getDeepestNode(BinaryTree<E> rootNode, int depth) {
    if (rootNode.leftNode.isLeafNode() && rootNode.rightNode.isLeafNode()) {
        return depth + 1;
    }
    return getDeepestNode(rootNode, depth + 1);
}

I know you wanted it without recursion, but I only came up with a solution by using recursion. If you really don't want to use recursion, then your code is fine.


Parallel quicksort algorithm taking way too long Below is a Python implementation of the quicksort algorithm using parallelism. It takes about 1 second per every 10 items in the list, which is hilariously unacceptable. Why is it so slow?

from multiprocessing import *

def quicksort(lyst, connection=None):
    if len(lyst) > 1:
        pivot = lyst.pop(len(lyst)-1)
        wall = 0
        for i in range(len(lyst)):
            if lyst[i] <= pivot:
                lyst[wall], lyst[i] = lyst[i], lyst[wall]
                wall += 1
        receiveLeft, sendLeft = Pipe()
        receiveRight, sendRight = Pipe()
        Process(target=quicksort, args=(lyst[:wall], sendLeft)).start()
        Process(target=quicksort, args=(lyst[wall:], sendRight)).start()
        lyst = receiveLeft.recv() + [pivot] + receiveRight.recv()
    if connection:
        connection.send(lyst)
        connection.close()
    return lyst

if __name__ == '__main__':
    quicksort([8,4,6,9,1,3,10,2,7,5])

EDIT
Thanks for the responses. As it turns out, switching to Threads and limiting the number of them I was spawning sped things up. However, my linear version of the algorithm still performed better.

The problem seems to be the fact that you are not dealing with threads (as you should), but rather entire processes (which is more computationally demanding). Refer to this page for details on how to spawn threads.

Also, as a side node, it makes no sense to keep spawning new threads over and over again, since spawning a thread can take easily a couple of milliseconds, which would obliterate all performance gain. Instead, ask the user to pass the maximum amount of threads allowed, and do not spawn any more than that. Furthermore, it would be reasonable to cancel spawning a new thread in case the range to sort it receives is too small.

Hope that helps.


React Component Reuse with 2 similar components I have 2 variations of components that I wanted to create in React. They both use Radio buttons with the only difference is one adds an image to one side of it.

The first method will use just one component but use conditional logic to display the appropriate HTML:

    getImgDiv(iconFile) {
        return (

);
    }
    render() {
        var radioId = this.props.radioGroup + this.props.value.replace(" ", "");
        var iconFile = this.props.iconFile;
        var innerLabel = "label-inner no-img";
        var imgDiv = "";
        if (iconFile !== undefined && iconFile !== null && iconFile !== "") {
            iconFile = "/img/" + iconFile;
            innerLabel = "label-inner"; // add appropriate class when there is an image
            imgDiv = this.getImgDiv(iconFile);
        }

        return (

                <Field
                    name={this.props.radioGroup}
                    component="input"
                    type="radio"
                    value={this.props.value}
                    id={radioId}
                />

                        {imgDiv}

{this.props.text}

        );
    }

The second method, I was thinking of just creating 2 slightly different React classes to handle each case... 1 called RadioBtn the other RadioBtnIcon with the img section added:

function RadioBtn({ radioGroup, value, text }) {
    var radioId = radioGroup + value.replace(" ", "");
    return (

            <Field
                name={radioGroup}
                component="input"
                type="radio"
                value={value}
                id={radioId}
            />

{text}

    );
}

However, its bugging me that neither of these approaches seems particularly elegant...

Any suggestions guys?

Many Thanks,

Phil.

You can take advantage of conditional rendering to concisely render the iconFile if it exists, and use the conditional operator to construct the outer class name. Using two separate classes for nearly identical functionality is pretty WET, I wouldn't recommend it.

Because the iconFile variable actually contains a string (if it exists), not a File, consider naming it something else, maybe iconPath:

        {iconPath &&

}

{text}

You currently reference properties of this.props a lot. Consider destructuring them all immediately instead.

Since you're using ES6+ syntax, always use const, never use var.

It sounds like Field is using the component prop as a tag name. Because variable tag names in JSX need to be upper-case, and because Component could easily be mistaken for React.Component, consider using a different name, maybe Tag.

const Field = ({ name, Tag, type, value, id }) => (
    <Tag name={name} type={type} value={value} id={id} />
);
class RadioImage extends React.Component {
    render() {
        const { radioGroup, value, iconPath, text } = this.props;
        const radioId = radioGroup + value.replace(" ", "");
        return (

                <Field
                    name={radioGroup}
                    Tag="input"
                    type="radio"
                    value={value}
                    id={radioId}
                />

                        {iconPath &&

}

{text}

        );
    }
}

ReactDOM.render(
    // The Fragments below can be replaced with <>  </> with an up-to-date Babel
    // (Stack Snippets do not support it)
    <React.Fragment>
        <RadioImage radioGroup="radioGroup" value="value1" text="text1" />
        <RadioImage radioGroup="radioGroup" value="value2" iconPath="iconPath" text="text2 with icon" />
    </React.Fragment>
    ,
    document.getElementById('root')
);

You might consider avoiding IDs when possible - they implicitly create global variables, which aren't always the best idea when avoidable. You can move the <input> inside the  so that no htmlFor={radioId} is needed.

You can make RadioImage a stateless functional component if you want, it doesn't look to have any need to be stateful:

const Field = ({ name, Tag, type, value, id }) => (
    <Tag name={name} type={type} value={value} id={id} />
);
const RadioImage = ({ radioGroup, value, iconPath, text }) => (

            <Field
                name={radioGroup}
                Tag="input"
                type="radio"
                value={value}
            />

                {iconPath &&

}

{text}

);
ReactDOM.render(
    // The Fragments below can be replaced with <>  </> with an up-to-date Babel
    // (Stack Snippets do not support it)
    <React.Fragment>
        <RadioImage radioGroup="radioGroup" value="value1" text="text1" />
        <RadioImage radioGroup="radioGroup" value="value2" iconPath="iconPath" text="text2 with icon" />
    </React.Fragment>
    ,
    document.getElementById('root')
);
.option {
  border: 1px solid black;
}


Split words that are too big I need to optimize the below code, as much as possible in:

using less resources as possible
being as fast as possible

What the code does is simple, it splits the string by words and only if a word is equal or bigger than 40 characters it will add spaces every X characters to the word that was too big:

<?php

$text = "more text here 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.1% more text here!";

foreach (explode(" ", $text) as $word)
{
    if (strlen($word) >= 40)
    {
        $word = wordwrap($word, 8, " ", true);
    }
    $result[] = $word;
}
$data = implode(" ", $result);
echo $data;

Live DEMO

Is there anything that could be improved in this already simple code?

I would think a better approach might be to identify location of spaces in string and look for cases where there is index position > 40 between these spaces, and then to insert the space into the string as necessary.

This would certainly be a memory-optimized way of working with this as you could potentially only need to hold the subject string (with any inserted separators) in memory, along with a few helper variables.

Your current approach would hold all the following in memory:

$text - the main subject string
temporary storage of explode results
$word - a substring duplication of content already stored in $text)
$result - an array containing duplicated content from $text + inserts
$data - a duplicate copy of content in $result just joined into a string on the separator.

So, there is a lot of duplication of storage in your solution.

I am guessing there is also increased operational complexity around the explode() and implode() operations that could be avoided.

I would also consider generalizing this into a function.  I have given some sample code below for how this might look:

function chunk_substring(
    $subject_string,
    $separator = ' ',
    $max_substring_length = 40,
    $chunk_size = 8
) {
    // not shown - validate input as non-zero length strings or positive integers
    // as appropriate. May also want to check logic around chunk size
    // needing to be less than max substring length and such

    // loop through index positions of separator in target string
    $previous_index = 0;
    $continue = true;
    $separator_length = strlen($separator);
    while ($continue) {
        $next_index = strpos($subject_string, $separator, $previous_index);
        if($next_index === false) {
            // separator not found in remainder of string
            $continue = false;
            continue;
        }
        if($next_index === $previous_index) {
            // we had a separator match at beginning of string
            // or consecutive separators
            // let's increment $previous_index by separator length
            $previous_index += $separator_length ;
            continue;
        }
        if(($next_index - $previous_index) >= $max_substring_length) {
            // we have a "long" substring
            // let's start inserting separators into it
            $idx = $previous_index + $chunk_size;
            while ($idx < $next_index) {
                // insert $separator at $idx
                $subject_string = substr_replace(
                    $subject_string,
                    $separator,
                    $idx,
                    0
                );

                // update $next_index to account for length of newly added
                // segment to string
                $next_index += $separator_length;

                // increase $idx by $chunk_size + $separator_length
                $idx = $idx + $chunk_size + $separator_length;
            }
        }
        // We are done evaluating / modifying this string segment
        // Set $previous_index to $next_index value in preparation for
        // next iteration
        $previous_index = $next_index;
    }

    // return the string
    return $subject_string;
}

This approach would hold the following in memory:

$subject_string - would potentially grow as separators are inserted. You could also potentially have the function pass this by reference if you desire to have this function directly modify the external variable that is passed in
$separator - small string
Various low memory footprint integer variables for configuration and index storage.

Much less duplication in memory in this case to where overall memory utilization would scale (more or less) 1:1 with size of input string.

You might take a look at the accepted answer for the StackOverflow post I have linked below which deals with the related question of using explode() vs. string tokenization, which is, in essence, what I am proposing.  You might consider running a similar test to find out for your self what works best for your use cases.

https://stackoverflow.com/questions/5861826/exploding-an-array-within-a-foreach-loop-parameter


String joining function in C I wrote a function that joins a collection of Strings with a delimiter. It's based on Java's version of the function. I'm assuming C has some similar built-in.

Example:

int main() {
    char* coll[] = {"Hello", "World", "This", "Should", "Be", "Joined!"};
    char* joined = awfulJoinStr(", ", coll, 6);

    printf("%s\n", joined);
    // Prints "Hello, World, This, Should, Be, Joined!"
}

Basically how it works is, it alternates between iterating the collection strings and the delimiter. It writes the current word to the buffer until it hits a null terminator, then it switches to either writing the next word, or to the delimiter.

I'm new to C, so I'd like comments on anything in the code. My main concerns:

It's super inefficient. All the strings need to be iterated at the start to get a total length so I know how big of a buffer to allocate later. I can't see how else I'd do it though unless I do reallocations while writing, but that seems like it would be even more inefficient.
It requires three iteration counters (stringI, charI, and bufferI) and a flag. I need to keep track of the current word that I'm writing, my position in it, and need to track where I am in the buffer. I also need a flag to track when I'm currently writing a delimiter or word.
It requires passing in how many strings I want to join (nStrings). This seems like it's necessary as well since I can't know the length of strArr, but it's unfortunate.
It's huge. It doesn't fit in one screen at a comfortable zoom.

I also included my version of strlen for kicks, and because I used it instead of the built-in.

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

size_t myStrLen(char* str) {
    size_t length = 0;

    while (str[length] != '\0') {
        length++;
    }

    return length;
}

char* awfulJoinStr(char* delimiter, char** strArr, size_t nStrings) {
    size_t delimiterSize = myStrLen(delimiter);
    size_t totalDelimiterSize = (nStrings - 1) * delimiterSize;

    size_t totalStringSize = 0;
    for (size_t i = 0; i < nStrings; i++) {
        totalStringSize += myStrLen(strArr[i]);
    }

    size_t bufferSize = totalDelimiterSize + totalStringSize + 1;
    char* buffer = malloc(sizeof(char) * bufferSize);
    buffer[bufferSize - 1] = '\0';

    bool inDelim = false;
    size_t charI = 0;
    size_t stringI = 0;
    for (size_t bufferI = 0; bufferI < bufferSize - 1;) {
        char current = inDelim ? delimiter[charI] : strArr[stringI][charI];

        if (current == '\0') {
            // Start at the beginning of the word and toggle what we're writing
            charI = 0;

            if (!inDelim) {
                stringI++;
            }

            inDelim = !inDelim;

        } else {
            buffer[bufferI] = current;

            charI++;
            bufferI++;
        }
    }

    return buffer;
}

I see a number of things that may help you improve your code.

Use standard library functions where appropriate

Except as a learning exercise (which I recognize this probably is), there's little reason to write duplicates of existing, well-tested, well-documented and often highly optimized library functions.  For that reason, in real code, I'd expect strlen() to be used rather than myStrLen().

Don't leak memory

Unlike Java, in C it is the responsibility of the programmer to clean up memory allocations.  For that reason, I'd expect the sample code to have the line free(joined); before the end of main.

Use const where appropriate

The delimiter and strArr arguments to awfulJoinStr are not and should not be modified by that function, so it would be better to indicate that fact and change the parameter list to this:

char* awfulJoinStr(const char* delimiter, const char** strArr, size_t nStrings)

Consider using a sentinel value

Generally, there are two ways to tell a function the size of a list.  Either it can be passed explicitly as you do in the current code, or one can use a sentinel value which is simply a special "end of list" marker value.  In this case, I think it might be more convenient to use a sentinel value of NULL.  So your coll could look like this:

const char* coll[] = {"Hello", "World", "This", "Should", "Be", "Joined!", NULL};

And now the code could look for the special NULL value instead of having to pass a count.

Check for errors

The call to malloc could fail.  The only indication you would have of this would be that it would return NULL, so for that reason, you should check for a NULL return from malloc before using the returned pointer or risk having your code crash.

Also, if nStrings is zero, the calculated totalDelimiterSize is huge and the result will certainly not be what you want, even if it doesn't crash.

Simplify code by knowing the standard well

The current code includes this line:

char* buffer = malloc(sizeof(char) * bufferSize);

However, the standard defines sizeof(char) as always being equal to 1 so this could and should be written like this instead:

char* buffer = malloc(bufferSize);

Bail out early on error

If the parameters to the function are faulty, or if there are no passed strings, it would probably make most sense to bail out early, rather than doing a bunch of work that would ultimately be thrown away.

Understand the use of pointers

A fundamental (but not always easy!) concept in C programming is the pointer.  Understanding how to use pointers effectively is an important skill to learn when mastering C programming.  Examples as applied to this program are shown in the code in the next section.

Simplify your code by decomposing into functions

Here's one way to rewrite the function using most of what's suggested above:

char* awfulJoinStr(const char* delimiter, const char** strArr) {
    if (strArr == NULL || delimiter == NULL) {
        return NULL;
    }
    // first calculate the total length of the strings
    size_t bufferSize = 0;
    size_t nStrings = 0;
    for (const char **str = strArr; *str != NULL; ++str) {
        bufferSize += strlen(*str);
        ++nStrings;
    }
    if (nStrings == 0) {
        return NULL;
    }
    size_t delimiterSize = strlen(delimiter);
    bufferSize += (nStrings - 1) * delimiterSize + 1;
    char* buffer = malloc(bufferSize);
    if (buffer == NULL) {
        return NULL;
    }
    buffer[bufferSize - 1] = '\0';
    for (char *curr = scopy(buffer, *strArr++); *strArr; ++strArr) {
        curr = scopy(curr, delimiter);
        curr = scopy(curr, *strArr);
    }
    return buffer;
}

As you see, the copying is done via a custom string copy function scopy.  We use this instead ofthe standard strcpy because strcpy returns the start of the destination string, but this code requires one past the end instead.  Here's scopy:

char *scopy(char *dest, const char *src) {
    while (*src) {
        *dest++ = *src++;
    }
    return dest;
}

This short function uses a lot of specialized C knowledge.  First, the passed parameters are both pointers.  We can alter the pointers without necesarily altering what they're pointing to.  Second is the idea of post-increment versus pre-increment.  This line is a very common C idiom:

*dest++ = *src++;

What it means in English is, in effect, "get the thing that src is pointing to and copy that to wherever dest is pointing; then increment both pointers."  (That's not exactly the sequence of events, but it will suffice to understand the intent of this line.)


Function/method to get selective columns Is it possible to simplify getRows() or it is fine as it is now?

protected $container;
public function __construct(ContainerInterface $container) {
    $this->container = $container;
    $this->em = $this->getDoctrine()->getManager();
}

/*
* @param    $ntt              name of the entity to fetch
* @param    $cols             columns to fetch
* @param    $where            array of data to be matched (for example array('id'=>1))
* @param    $result_type      type of result required. (object|array)
*/
public function getRows($ntt,$cols=array(),$where=array(),$result_type=null){
    $data = 'nulllllll';
    if($ntt && $where && is_array($where)){
        $i = 1;
        $filter = '';
        foreach ($where as $key => $value) {
            if(is_array($value)){
                $filter = $filter.($filter ? ' AND ' : '')."tm.$key in (:v$i)";
            }else{
                $filter = $filter.($filter ? ' AND ' : '')."tm.$key = :v$i";
            }
            $param['v'.$i] = $value;
            $i++;
        }

        $select = 'tm';
        if($cols){
            $select = '';
            foreach ($cols as $col) {
                $select = $select.($select ? ', ' : '')."tm.$col";
            }
        }

        $ntt = 'AppBundle:'.$ntt;
        $data = $this->em->getRepository($ntt)->createQueryBuilder('tm')
        ->select($select)
        ->where($filter)
        ->setParameters($param)
        ->getQuery();
        if($result_type == 'array'){
            $data = $data->getArrayResult();
        }else{
            $data = $data->getResult();
        }
    }
    return $data;
}

Calling:

getRows('My_user',array('lName','llname'),array('id'=>array(1,4,50)),'array')

Where My_user is my entity name and lName and llname are the columns I want to fetch.

Will output:

Array
(
    [0] => Array
        (
            [lName] => Abc
            [llname] => Sha
        )

    [1] => Array
        (
            [lName] => And
            [llname] => Bainbri
        )

    [2] => Array
        (
            [lName] => Ken
            [llname] => Lee
        )

)

I'd suggest your code to be rewritten like this (please, take note of the inline comments):

<?php

// Requires PHP >= 5.4

public function getRows($ntt, $cols = array(), $where = array(), $result_type = null)
{
    $data = 'nulllllll';

    // Early return after checking arguments,
    // instead of nesting the rest of the code inside the if{}
    if (!($ntt && $where && is_array($where))) {
        return $data;
    }

    $filters = [];
    $param = [];

    // The callback function here uses outer variables: `$filters` and `$param`,
    // that are passed by reference `&`
    array_walk($where, function($value, $key) use (&$filters, &$param) {
        static $i = 0;
        $i++;

        $placeholder = "v{$i}";
        $condition = is_array($value) ? "in (:{$placeholder})" : "= :{$placeholder}";
        $filters[] = "tm.{$key} {$condition}";
        $param[$placeholder] = $value;
    });

    // Use `implode` to concatenate the `$filters` into a string
    $filter = implode(' AND ', $filters);

    $select = 'tm';
    if (!empty($cols) && is_array($cols)) {
        $select = implode(', ', array_map(function($column) {
                return "tm.{$column}";
            }, $cols));
    }

    $repository = "AppBundle:{$ntt}";
    $data = $this->em->getRepository($repository)->createQueryBuilder('tm')
        ->select($select)
        ->where($filter)
        ->setParameters($param)
        ->getQuery();

    if ($result_type === 'array') {
        return $data->getArrayResult();
    }

    return $data->getResult();
}


Keep asking the user [after performing an action] until they wish to stop I have a form that requires the user to select photos to be included in their portfolio before they can proceed to use the app for the first time.

I don't want to spend the rest of my life implementing a custom file browser, so I've decided to just use the FolderBrowserDialog to allow them to select a folder containing photos. I've also decided to let them browse as many times as they need in case they have photos scattered in other locations.

I can't think of a better way to write this, but it seems like there would be a better way. I'd appreciate any thoughts/suggestions on how to improve this piece or advice on a better way to write it:

    private void Main_Shown(object sender, EventArgs e)
    {
        /* Let them select photos to be included in the portfolio
         * and move them to the permanent Photo Collection folder,
         * and display them on the form.
         */

        bool selectingPhotos = true;

        while (selectingPhotos) {
            using (FolderBrowserDialog folderPicker = new FolderBrowserDialog())
            {
                folderPicker.Description = "Please browse to, and select the folder containing the files you'd like to add.";

                if (folderPicker.ShowDialog(this) == System.Windows.Forms.DialogResult.OK)
                {
                    // Add files to list for processing later.

                    // Ask if they want to add more files from another directory.
                    if (MessageBox.Show("Do you want to select more photos from another location?", "Confirmation", MessageBoxButtons.YesNo) == System.Windows.Forms.DialogResult.Yes)
                    {
                        selectingPhotos = true;
                    }
                    else
                    {
                        selectingPhotos = false;
                    }
                }
                else
                {
                    selectingPhotos = false;
                }
            }
        }
    }

Since you have a task you want to repeat at least once, the do-while is the most suitable. How about the following:

private void Main_Shown(object sender, EventArgs e)
{
    /* Let them select photos to be included in the portfolio
     * and move them to the permanent Photo Collection folder,
     * and display them on the form.
     */

    using (FolderBrowserDialog folderPicker = new FolderBrowserDialog())
    {
        folderPicker.Description = "Please browse to, and select the folder containing the files you'd like to add.";

        do
        {
            if (folderPicker.ShowDialog(this) == System.Windows.Forms.DialogResult.OK)
            {
                // Add files to list for processing later.
            }
            else
            {
                break;
            }

        } while (MessageBox.Show("Do you want to select more photos from another location?", "Confirmation", MessageBoxButtons.YesNo) == System.Windows.Forms.DialogResult.Yes)
    }
}


Find and process duplicates in list of lists I'm trying to merge counts for items (URLs) in the list:

[['foo',1], ['bar',3],['foo',4]]

I came up with a function, but it's slow when I run it with 50k entries. I'd appreciate if somebody could please review and suggest improvements.

def dedupe(data):
    ''' Finds duplicates in data and merges the counts '''
    result = []
    for row in data:
        url, count = row
        url_already_in_result = filter(lambda res_row: res_row[0] == url, result)
        if url_already_in_result:
            url_already_in_result[0][1] += count
        else:
            result.append(row)
    return result

def test_dedupe():
    data = [['foo',1], ['bar',3],['foo',4]]
    assert dedupe(data) == [['foo',5], ['bar',3]]

It looks like you could use collections.Counter. Although you may want to use it earlier in your code, when you create the list of pairs you pass to dedupe. As is, you could use the following in your code:

from collections import Counter

def dedupe(data):
    result = Counter()
    for row in data:
        result.update(dict([row]))
    return result.items()

>>> data = [['foo',1], ['bar',3],['foo',4]]
>>> dedupe(data)
[('foo', 5), ('bar', 3)]


Animation using function pointers So I've been thinking alot about simple/convenient ways to implement simple 2D animation using a bare-bones graphics library I'm using for school (Animation is way out of the scope of the class, I'm just experimenting.)

I thought of using a custom function passed to the class to allow the "user" of the code to just write their own code to do whatever they want to the sprite, then return control to the sprite's Update() function.  Here's what I have so far:

// SpriteStuff.h

typedef bool (AnimationStep*)(Sprite* sprite) ANIMATIONSTEP

class Sprite {
    //...
    ANIMATIONSTEP m_currentAnimation;

    void BeginAnimation(ANIMATIONSTEP step);
    void EndAnimation();

    void Update();
    //...
};

void Sprite::BeginAnimation(ANIMATIONSTEP step) {
    if (step != NULL)
        m_currentAnimation = step;
}
void Sprite::EndAnimation() {
    m_currentAnimation = NULL;
}

void Sprite::Update() {
    //...
    if (m_currentAnimation != NULL)
        if (!m_currentAnimation(this)) EndAnimation();
    //...
}

// (off in some other code...)
void DoAnimateThing() {
    mySprite->BeginAnimation(GeneralFallAnimationStep);
}

bool GeneralFallAnimationStep(Sprite* sprite){
    return sprite->y++ > SOME_LIMIT_SOMEWHERE;
}

// Could possibly end it early...
void CancelAnimationOnThing() {
    mySprite->EndAnimation();
}

It seems like a pretty simple design that will do what I need, but almost too simple.  I'm doing something terribly wrong, huh?

EDIT:  So I've gotten some good input on this, but now I'm curious... Any suggestions how controlling the speed of these animations?  Can this be done well in the Sprite class, or should that be handled strictly by the ANIMATIONSTEP?

Too simple? Hardly. That is pretty much a textbook implementation of animation using a callback-based event model. In a production-grade version there would be a few extra null checks and similar to check for programming mistakes, but the basic implemation could be very similar.
I'm assuming that elsewhere there is a main loop that periodically calls update on each relevant sprite. I'm also assuming that either update takes care of drawing the spite, or that code not shown is responsible for that.
One Weakness and Possible Solution
One thing though that may be problematic for complicated animations is that your design gives the function no scratch space in which to store any state information (complicated animations may want to know if this is the first or second time the sprite was at coordinate (457,758) for example).
An easy solution is to add an extra void * to the sprite. This would be an extra pointer the animation function can set to whatever it wants (usually an animation specific structure). Have BeginAnimation set this new pointer to NULL. Document that the pointer must either be left NULL, or allocated with new. EndAnimation would delete the pointer on the animation's behalf if it was not still NULL, since the animation is not given the opertunity to cleanup if EndAnimation is called directly.
There are cleaner solutions than the above, but the that one is easy to explain, and should work.


Dictionary file manager The dictionary class loads entries from a file and then can perform operations on them. The dictionary class then stores the entries back into the file.

I would like to know the best way to check if the arguments to my dictionary member functions are valid terms/definitions.  I have thought of two possible solutions, but I am open to any other solutions.  I could either check the arguments passed in to the functions or make a term and definition class and and have the class check instead.

The reason why I want to distinguish strings from terms/definitions is so that I can read them from a file without having to encounter cases where definitions do not end in periods or other cases caused by human error.

dictionary.cpp

#include "dictionary.h"

//*** @TODO: What constitutes a term/definition ***

bool dictionary::search_term(const std::string& term){
    for(auto& it: entries){
        if(it.first != term);
        else return true;
    }return false;
};

bool dictionary::erase_entry(const std::string& term){
    if(search_term(term)){
        entries.erase(term);
        return true;
    }else return false;
};

bool dictionary::define_term(const std::string& term, const std::string& definition){
    if(search_term(term)){
        entries[term] = definition;
        return true;
    }else return false;
};

bool dictionary::write_entry(const std::string& term, const std::string& definition){
if(!search_term(term)){
    entries[term] = definition;
        return true;
    }else return false;
};

inline bool exists (const std::string& name) {
   struct stat buffer;
   return (stat (name.c_str(), &buffer) == 0);
}

bool dictionary::ofs_entries(const std::string& path){
    std::string file = (path + title);
    std::ofstream ofs(file.c_str());
    if(!ofs) return false;
    for(auto& it: entries){
        ofs << it.first << ": " << it.second << '\n';
    }ofs.close();
};

bool dictionary::ifs_entries(const std::string& path){
    std::string file = (path + title);
    if(!exists(file)) return false;
    std::ifstream ifs(file.c_str());
    if(!ifs) return false;
    std::string entry;
    while(true){
        //read entries
        if(!ifs.eof()) break;
    }return true;
};

dictionary.h

#ifndef DICTIONARY_H
#define DICTIONARY_H

#include <algorithm>
#include <iterator>
#include <fstream>
#include <string>
#include <map>

class dictionary{
   public:
        dictionary(const std::string& title, const std::string& definition = "")
            : entries{{title, definition}}, title(title){;};

        bool write_entry(const std::string& term, const std::string& definition = "");
        bool define_term(const std::string& term, const std::string& definition);

        bool erase_entry(const std::string& term);
        bool search_term(const std::string& term);

        bool ofs_entries(const std::string& path);
        bool ifs_entries(const std::string& path);
    private:
        std::map<std::string, std::string> entries;
        std::string title;
};

#endif//DICTIONARY_H

Use the tools you have
The first thing I notice is that you're doing some work you don't need to. You're using a std::map to hold your keys and values, but you aren't utilizing the built-in method for finding an entry. (In other languages maps are actually named dictionary!) Your entire search_term() method could look like this:
bool dictionary::search_term(const std::string& term){
    std::map<std::string, std::string>::iterator it = entries.find(term);
    return it != entries.end();
};

Likewise, the erase_entry() method could just call the std::map::erase() method.
Naming is hard
For the most part your variable and method names are pretty good! There are a few that I'd change, though. The exists() function should be something like file_exists() so it's not confused with checking if an entry exists in the dictionary.
Also, ofs_entries() and ifs_entries() are odd names. I'd call them something like write_entries_to_file() and read_entries_from_file().


Java mergesort with generics The following is my implementation of mergesort with Java generics. I chose to use the iteration variables a_iter and b_iter to prevent modifying the lists while merging. Any suggestions for improvement?

public static <T extends Comparable<T>> List<T> mergesort(List<T> list){
    // base case
    int len = list.size();
    if(len<=1)
      return list;
    // recursively sort two halves
    List<T> left = mergesort(list.subList(0,len/2));
    List<T> right = mergesort(list.subList(len/2, len));
    // merge two halves
    List<T> combined = new ArrayList<>();
    // vars to keep place in iteration (used to prevent ConcurrentModificationException)
    int a_iter = 0;
    int b_iter = 0;
    while(a_iter<left.size() || b_iter<right.size()){
      // if left exhausted
      if(a_iter>=left.size()){
        combined.add(right.get(b_iter));
        b_iter++;
        continue;
      }
      // if right exhausted
      if(b_iter>=right.size()){
        combined.add(left.get(a_iter));
        a_iter++;
        continue;
      }
      T a = left.get(a_iter);
      T b = right.get(b_iter);
      if(a.compareTo(b)<0){
        combined.add(a);
        a_iter++;
      }
      else{
        combined.add(b);
        b_iter++;
      }
    }
    return combined;
  }

The first thing I would do is split up the while loop into 3 parts:
- handling both not empty
- add all remaining from left
- add all remaining from right

No wait, the first thing I did is paste the code in IntelliJ and have it autoformat to comply with the java coding conventions.
Since it's just some minor points like spacing I'm not going to list them here, overall your code looks decent already.

I also renamed a_iter and b_iter to leftIndex and rightIndex. Both because a variable shouldn't have an _ in it's name and because it better conveys what the variable is. Same for renaming a and b to leftElement and rightElement.

Now back to splitting up the while loop. I changed the || in the while condition to an &&, removed the "when xxx exhausted" parts and added the 2 new while loops after. This is the result:

int leftIndex = 0;
int rightIndex = 0;
while (leftIndex < left.size() && rightIndex < right.size()) {
    T leftElement = left.get(leftIndex);
    T rightElement = right.get(rightIndex);
    if (leftElement.compareTo(rightElement) < 0) {
        combined.add(leftElement);
        leftIndex++;
    } else {
        combined.add(rightElement);
        rightIndex++;
    }
}

// At least one of the sub lists is empty at this point.
// Just add all remaining elements from the other sub list.
while(leftIndex < left.size()){
    combined.add(left.get(leftIndex++));
}
while(rightIndex < right.size()){
    combined.add(right.get(rightIndex++));
}

Or if you want, replace the last while loops with an addAll

combined.addAll(left.subList(leftIndex, left.size()));
combined.addAll(right.subList(rightIndex, right.size()));

The biggest problem with this implementation is that it creates a lot of temporary lists. Taking a quick glance at how the built in list sort in java is implemented shows that they turn it into an array first, sort the array and then update the given list:

 public static <T extends Comparable<? super T>> void sort(List<T> list) {
     Object[] a = list.toArray();
     Arrays.sort(a);
     ListIterator<T> i = list.listIterator();
     for (int j=0; j<a.length; j++) {
         i.next();
         i.set((T)a[j]);
     }
 }

Note that this doesn't return the list. It actually sorts the list you pass into it.

If you're looking for more improvements to the merge algorithm itself you could look up how it's implemented in java. (Although recently it's changed into a Tim sort which is a combination of merge sort and insertion sort).

As a final note: if your intention was to actually use your code in a project other than studying how merge sort is implemented or practicing your java skills I would advise against it. Just use the built-in functions if available.


Producing residues of combinatorics on strings Basically, this code does multiplication of chosen operations.

public static String[] gen8String(String[] pattern1, String[] pattern2){
    String[] combinedSubset = new String[90]; //emty array for the subset of 90 strings
    String  combinedString = ""; //string holder for each combined string
    int index = 0; //used for combinedSubset array
    int present = 0; //used to check if all 6 characters are present

    for(int i = 0; i < 15; i++){

        for(int j = 0; j < 15; j++){
            combinedString = pattern1[i] + pattern2[j]; //combine both 4 letter strings into 8 char length string
            char[] parsedString = combinedString.toCharArray(); //parse into array

            //check if all 6 characters are present
            for(int k = 1; k <= 6; k++)
            {
                if(new String(parsedString).contains(k+"")) {
                    present++;
                }
                else
                    break;
                //if all 6 are present, then add it to combined subset
                if(present == 6)
                    combinedSubset[index++] = combinedString;
            }
            present = 0;
        }
    }
    return combinedSubset;
}

Let's look at an example:

Let's says I have 2 strings ABCDEF and ACDEBF. Now before I call gen8string, I will first perform a \${6 \choose 4}\$ deletion operation on each string. For instance, I will take ABCDEF delete any two characters in that string and I am left with substring of length 4. How many different such strings can I have (given all the letters are unique)? \$15 = {6 \choose 4}\$. Now, I will do the same for both strings. Now I am left with 2 string arrays, each of size 15.

In gen8String, I am passing in the 2 above mentioned arrays and I am combining the substrings under the following constraints:

I can only combine 2 strings to make one 8-length string (4+4).
I can only combine them if there are no overlapping missing characters.

What do I mean by the 2nd point? Well, let's say I one of the inputs in pattern is ABCD and in pattern2 one of the inputs is ACDE. These 2 substrings cannot be combined because there are overlapping missing characters, i.e. the F. However, if I have ABCD in pattern1 and CDEF, these can be combined because all 6 characters are present in the 8-length string at least once. So, no overlapping missing characters. This can be seen by tracing through the code.

As a concluding note, this function essentially does a

$${{6}\choose{4}} * {{4}\choose{2}} = 90$$

How can I optimize/generalize this code? Ways to improve it?

Think about alternatives

  Let's says I have 2 strings ABCDEF and ACDEBF. Now before I call gen8string, I will first perform a \$\binom 6 4\$ deletion operation on each string. For instance, I will take ABCDEF delete any two characters in that string and I am left with substring of length 4. How many different such strings can I have (given all the letters are unique)? \$15=\binom 6 4\$. Now, I will do the same for both strings. Now I am left with 2 string arrays, each of size 15.

Why?

You want to generate 90 strings.  Why generate 255 strings and then mask out 165 of them?  Why not just generate 90 strings directly?

  ABEFCDEF 001122
  ABDFCDEF 001212
  ABDECDEF 001221
  ...
  ABCFBDEF 020112
  ...
  ABEFABCD 221100

The six numbers are which string has the letter.  So 001122 means AB in the first string, CD in the second string, and EF in both.  Iterate from 001122 to 221100 or vice versa.  Then you don't need gen8string at all, as all it does is eliminate invalid strings like 2222-- and 22210-.

Of course, this would happen outside the code that you posted.

Working with what we have here

You have

public static String[] gen8String(String[] pattern1, String[] pattern2){
    String[] combinedSubset = new String[90]; //emty array for the subset of 90 strings
    String  combinedString = ""; //string holder for each combined string
    int index = 0; //used for combinedSubset array
    int present = 0; //used to check if all 6 characters are present

    for(int i = 0; i < 15; i++){

        for(int j = 0; j < 15; j++){
            combinedString = pattern1[i] + pattern2[j]; //combine both 4 letter strings into 8 char length string
            char[] parsedString = combinedString.toCharArray(); //parse into array

            //check if all 6 characters are present
            for(int k = 1; k <= 6; k++)
            {
                if(new String(parsedString).contains(k+"")) {
                    present++;
                }
                else
                    break;
                //if all 6 are present, then add it to combined subset
                if(present == 6)
                    combinedSubset[index++] = combinedString;
            }
            present = 0;
        }
    }
    return combinedSubset;
}

Consider

public static String[] gen8String(String[] patterns1, String[] patterns2) {
    String[] results = new String[90];
    int resultCount = 0;

    for (String pattern1 : patterns1) {
        for (String pattern2 : patterns2) {
            String combined = pattern1 + pattern2;

            //check if all 6 characters are present
            for (int k = 1; combined.contains(k+""); k++) {
                //if all 6 are present, then add it to the results and end
                if (k >= 6) {
                    results[resultCount++] = combined;
                    break;
                }
            }
        }
    }

    return results;
}

This changes the names of the parameters.  They are collections of patterns, not the patterns themselves.

I changed the names of combinedSubset and index to be clearer about how they relate to the results.  This allowed me to drop the comments as obsolete.

Switching from C-style for loops to range based for loops gets rid of the unnecessary i and j variables.  It also gets rid of the magic number 15.

I renamed combinedString to combined and moved the declaration to where it was set.  Since a single value is never used across iterations of the outer two loops, the scope doesn't need to be any wider.

We don't need present, as we already have k.  If we make it to the sixth iteration, we can update the results and stop the loop.

If we gate the loop on the contains check, we can just check the iterations once inside the loop.  The original code checked twice on each iteration.

Note that your example suggested inputs like ABCD and CDEF.  The patterns here are actually like 1234 and 3456.

Generalizing

Your code only works for four element subsets of a six element set.  Consider rewriting so the results are a List.  Then you wouldn't have to give a result set size or maintain your own count of the results.

The largest value for k (6 here) should probably be a parameter.  Or pass in an array or list over which we could iterate.

I already modified the for loops to be based on the size of the input parameters, so that's already generalized.

And of course the name would need to change.

Precalculate

Because the current results are so narrow, you could just set them manually.  One ninety element array.


Auto refreshing div in jQuery While implementing auto refreshing div in jQuery that fetches updates on posts periodically as can be seen on Twitter, Facebook etc.

I had to think about making the refresh as efficient as possible. I decided to send a request to the server every 30 seconds - 1 minute. And, the request will be sent only if the browser window is active.

var postUpdateIntervalID;
var interval = 30000;

$(window).load(function(){
    if (!postUpdateIntervalID) {
        postUpdateIntervalID = setInterval(function() {
            updatePostsList();
        }, interval);
    }
});

$(window).focus(function(){
    if (!postUpdateIntervalID) {
        updatePostsList();
        postUpdateIntervalID = setInterval(function() {
            updatePostsList();
        }, interval);
    }
});

$(window).blur(function() {
    clearInterval(postUpdateIntervalID);
    postUpdateIntervalID = 0;
});

function updatePostsList() {
    // Sends a request to the server to fetch new posts.
}

On load, set interval of 30 seconds to call updatePostsList.
On focus, call updatePostsList immediately. Then, set interval as above.
On blur, reset the interval.

If this is for a complex web application, I would recommend using some sort of MVC framework. Personally I prefer backbone.js. Here are some great resources that should help you develop a maintainable app, as well help with updating and refreshing views.

MVC explanation: http://msdn.microsoft.com/en-us/library/ff649643.aspx

Backbone.js: http://documentcloud.github.com/backbone/

Connection between the two: http://css.dzone.com/articles/backbonejs-mvc-javascript

As a more direct answer to your question, I believe the code you have looks fine. But if you have multiples modules in your application, running view refresh functions on timers is not a good idea.


Java Stream - Collections - remove duplicates based on string and presence/absence of local date value I have a list of objects with a string (name) and a local date (date of deletion).

I wish to implement uniqueness on these objects based on:

If two objects have the same name, then I wish the one with no date
of deletion to have priority.
If two objects have the same name, and a date of deletion, then I wish the one with the most recent date of deletion to have priority.

Based on the above criteria, I have implemented, what I think is the right solution.

Do you have any pointers as to the correctness of my approach? Do you have any suggestions for improvements?

The class:

public class Unique implements Comparable<Unique>
{
    private String name;
    private LocalDate dateOfDeletion;

    public Unique(String name, LocalDate dateOfDeletion) {
        this.name = name;
        this.dateOfDeletion = dateOfDeletion;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public LocalDate getDateOfDeletion() {
        return dateOfDeletion;
    }

    public void setDateOfDeletion(LocalDate dateOfDeletion) {
        this.dateOfDeletion = dateOfDeletion;
    }

    @Override
    public String toString() {
        return "Unique [name=" +name + ", dateOfDeletion="+dateOfDeletion+"],";
    }

    /**
     * order based on name ascending, and date descending
     */
    @Override
    public int compareTo(Unique rhs) {
        Comparator<Unique> reverseDateComparator = (a, b) -> {
            LocalDate thisDodel = a.getDateOfDeletion() == null ? LocalDate.now().plusDays(1) : a.getDateOfDeletion();
            LocalDate thatDodel = b.getDateOfDeletion() == null ? LocalDate.now().plusDays(1) : b.getDateOfDeletion();
            return thatDodel.compareTo(thisDodel);
        };
        return Comparator.comparing(Unique::getName).thenComparing(reverseDateComparator).compare(this, rhs);
    }
}

The Test:

public static void main(String[] args) {
    List<Unique> uniqueList = Arrays.asList(
                new Unique("a", null)
                ,new Unique("b", null)
                ,new Unique("b", LocalDate.of(2016, Month.SEPTEMBER, 20))
                ,new Unique("c", LocalDate.of(2016, Month.SEPTEMBER, 19))
                ,new Unique("c", LocalDate.of(2016, Month.SEPTEMBER, 20))
                );

    System.out.println("list ordered, with duplicates: ");
    uniqueList.stream().sorted().forEach(System.out::println);

    System.out.println("=================");
    System.out.println("list ordered, duplicates removed: ");
    Comparator<Unique> uniqueComp = (a, b) -> a.getName().compareTo( b.getName() );
    //SOLUTION HERE: since list is already ordered by date desc, it will ignore the records with same name already added to the list
    TreeSet<Unique> sortedFilterd = uniqueList.stream().sorted().collect(Collectors.toCollection(() -> new TreeSet<Unique>(uniqueComp)));
    sortedFilterd.forEach(System.out::println);
}

What I expect to see is the following:

list ordered, with duplicates:
Unique [name=a, dateOfDeletion=null],
Unique [name=b, dateOfDeletion=null],
Unique [name=b, dateOfDeletion=2016-09-20],
Unique [name=c, dateOfDeletion=2016-09-20],
Unique [name=c, dateOfDeletion=2016-09-19],
=================
list ordered, duplicates removed:
Unique [name=a, dateOfDeletion=null],
Unique [name=b, dateOfDeletion=null],
Unique [name=c, dateOfDeletion=2016-09-20],

Here is a link to the working sample: Link to working sample to the above code

Simplification

The code can be simplified using built-in methods from the Comparator API.

public int compareTo(Unique rhs) {
    Comparator<Unique> reverseDateComparator = (a, b) -> {
        LocalDate thisDodel = a.getDateOfDeletion() == null ? LocalDate.now().plusDays(1) : a.getDateOfDeletion();
        LocalDate thatDodel = b.getDateOfDeletion() == null ? LocalDate.now().plusDays(1) : b.getDateOfDeletion();
        return thatDodel.compareTo(thisDodel);
    };
    return Comparator.comparing(Unique::getName).thenComparing(reverseDateComparator).compare(this, rhs);
}

The reverseDateComparator is primarily used to handle null values. However, there are the built-ins comparators nullsFirst and nullsLast that exist just for this: they wrap an existing comparator and turn it null safe, by considering null values either greater or lower than non-null values.

As such, you can rewrite this method with:

@Override
public int compareTo(Unique rhs) {
    return Comparator.comparing(Unique::getName)
                .thenComparing(Unique::getDateOfDeletion, Comparator.nullsFirst(Comparator.reverseOrder()))
                .compare(this, rhs);
}

This directly passes the method-reference Unique::getDateOfDeletion to thenComparing and compares the dates by placing null values before other values and non-null in their reverse natural order (so that the most recent date is sorted before).

Using static imports makes the code a bit cleaner also. With the following imports,

import static java.util.Comparator.comparing;
import static java.util.Comparator.nullsFirst;
import static java.util.Comparator.reverseOrder;

the methods becomes very readable with regard to the initial goal (compare by name, then compare by dates reversed with first no deletion date):

@Override
public int compareTo(Unique rhs) {
    return comparing(Unique::getName)
                .thenComparing(Unique::getDateOfDeletion, nullsFirst(reverseOrder()))
                .compare(this, rhs);
}

In the same way, the following:

Comparator<Unique> uniqueComp = (a, b) -> a.getName().compareTo( b.getName() );

can be written:

Comparator<Unique> uniqueComp = Comparator.comparing(Unique::getName);

equals needs to be overriden

Since your Unique class implements Comparable, it is strongly recommended to let it override the equals method, and have a consistent implementation.

In this case, the equals should simply verify the equality of both the names of the date of deletion. To handle null values, you can make use of Objects.equals: this method returns true when the two given objects are either both null or both not-null and equal with regard to equals.

Other comments

Do you really need setters in your Unique class? Consider making that class immutable.
You have a trailing comma in

return "Unique [name=" +name + ", dateOfDeletion="+dateOfDeletion+"],";

Does it make sense to allow a null name? If not, you can validate that it is always present in the constructor with requireNonNull. Also, since the date of deletion might be null, a constructor overload without it may be simpler for the client.