Waiting for goroutines to finish I have a program that starts a bunch of goroutines, feeds them data and when there is no more data, waits for the goroutines to finish. I use sync.WaitGroup. I am unsure if I am using correctly, in an idiomatic way or if there is a better solution.
Background
My real code reads a log file of disk-space and numbers of files backed up etc for several servers. The records are intermingled. From this data it produces a web-page report per server with graphs etc. It works but I believe I can more cleanly separate the functions if I use concurrency. This is a test program before I go ahead and refactor.
Code (72 lines including data)
package main

import (
    "fmt"
    "strconv"
    "strings"
    "sync"
)

func main() {
    p := newPool()
    defer p.Close()

    for _, line := range strings.Split(logData, "\n") {
        f := strings.Fields(line)
        yr, _ := strconv.Atoi(f[0])
        sub := f[1]
        amt, _ := strconv.Atoi(f[2])
        r := logRec{yr, sub, amt}
        p.Report(r)
    }
}

type logRec struct {
    when    int
    subject string
    amount  int
}

type pool struct {
    reporters map[string]chan logRec
    wg        *sync.WaitGroup
}

func newPool() pool {
    rs := make(map[string]chan logRec)
    var wg sync.WaitGroup
    return pool{reporters: rs, wg: &wg}
}

func (p *pool) Report(rec logRec) {
    if _, ok := p.reporters[rec.subject]; !ok {
        c := make(chan logRec)
        p.wg.Add(1)
        go func() {
            defer p.wg.Done()
            tot := 0
            for r := range c {
                tot += r.amount
            }
            fmt.Printf("Total for %s is %d\n", rec.subject, tot)
        }()
        p.reporters[rec.subject] = c
    }
    p.reporters[rec.subject] <- rec
}

func (p pool) Close() {
    for _, c := range p.reporters {
        close(c)
    }

    p.wg.Wait()
}

const logData = `2018 apples 9
2018 oranges 5
2019 apples 27
2019 lemons 13
2019 oranges 2
2020 oranges 1
2020 apples 16
2020 lemons 3`

Output
Total for oranges is 8
Total for lemons is 16
Total for apples is 52

Question:
Can I improve this? Is there a better way to wait or a more idiomatic use of WaitGroups?

Yes, since you are here for code review:

Never ever ignore errors: yr, _ := strconv.Atoi(f[0])

        yr, err := strconv.Atoi(f[0])
        if err != nil {
            log.Fatal(err)
        }

You may use just one strings.Split/strings.Fields this way:

    f := strings.Fields(logData)
    for i := 3; i <= len(f); i += 3 {
        yr, err := strconv.Atoi(f[i-3])
        if err != nil {
            log.Fatal(err)
        }
        amt, err := strconv.Atoi(f[i-1])
        if err != nil {
            log.Fatal(err)
        }
        r := logRec{yr, f[i-2], amt}
        p.Report(r)
    }

You are over complicating a simple sum function, You don't need channels here, just a map does the job: It is this simple:

package main

import (
    "fmt"
    "log"
    "strconv"
    "strings"
)

func main() {
    m := map[string]int{}
    f := strings.Fields(logData)
    for i := 3; i <= len(f); i += 3 {
        amt, err := strconv.Atoi(f[i-1])
        if err != nil {
            log.Fatal(err)
        }
        m[f[i-2]] += amt
    }
    fmt.Println(m)
}

const logData = `2018 apples 9
2018 oranges 5
2019 apples 27
2019 lemons 13
2019 oranges 2
2020 oranges 1
2020 apples 16
2020 lemons 3`

For concurrency sake: You only need concurrent map, nothing more:

package main

import (
    "fmt"
    "log"
    "strconv"
    "strings"
    "sync"
)

type concurrentMap struct {
    sync.Mutex
    m map[string]int
}

func (p *concurrentMap) sum(item string, amount int) {
    p.Lock()
    p.m[item] += amount
    p.Unlock()
}

var data = &concurrentMap{m: map[string]int{}}

func main() {
    f := strings.Fields(logData)
    for i := 3; i <= len(f); i += 3 {
        amt, err := strconv.Atoi(f[i-1])
        if err != nil {
            log.Fatal(err)
        }
        data.sum(f[i-2], amt)
    }
    fmt.Println(data.m)
}

const logData = `2018 apples 9
2018 oranges 5
2019 apples 27
2019 lemons 13
2019 oranges 2
2020 oranges 1
2020 apples 16
2020 lemons 3`

Long running task example:

package main

import (
    "fmt"
    "log"
    "strconv"
    "strings"
    "sync"
    "time"
)

type concurrentMap struct {
    m map[string]int
    sync.Mutex
}

func (p *concurrentMap) sum(item string, amount int) {
    p.Lock()
    p.m[item] += amount
    p.Unlock()
}
func (p *concurrentMap) show() {
    p.Lock()
    for k, v := range p.m {
        fmt.Printf("Total for %s is %d\n", k, v)
    }
    p.Unlock()
}

var data = &concurrentMap{m: map[string]int{}}

func main() {
    go data.sum("oranges", 100)
    go data.sum("apples", 100)
    go data.sum("lemons", 100)

    go func() {
        f := strings.Fields(logData)
        for i := 3; i <= len(f); i += 3 {
            amt, err := strconv.Atoi(f[i-1])
            if err != nil {
                log.Fatal(err)
            }
            data.sum(f[i-2], amt)
            time.Sleep(1000 * time.Millisecond) // e.g. slow hard disk
        }
    }()

    t := time.NewTicker(1000 * time.Millisecond)
    defer t.Stop()
    for range t.C {
        data.show()
        fmt.Println()
    }
}

const logData = `2018 apples 9
2018 oranges 5
2019 apples 27
2019 lemons 13
2019 oranges 2
2020 oranges 1
2020 apples 16
2020 lemons 3`

And finally if your tasks will finish in time, you may use sync.WaitGroup:
package main

import (
    "fmt"
    "log"
    "strconv"
    "strings"
    "sync"
    "time"
)

type concurrentMap struct {
    m map[string]int
    sync.Mutex
}

func (p *concurrentMap) sum(item string, amount int) {
    p.Lock()
    p.m[item] += amount
    p.Unlock()
}
func (p *concurrentMap) show() {
    p.Lock()
    for k, v := range p.m {
        fmt.Printf("Total for %s is %d\n", k, v)
    }
    p.Unlock()
}

var data = &concurrentMap{m: map[string]int{}}

func main() {
    finished := &sync.WaitGroup{}
    finished.Add(1)
    go func() {
        defer finished.Done()
        data.sum("oranges", 100)
        data.sum("apples", 100)
        data.sum("lemons", 100)
    }()

    finished.Add(1)
    go func() {
        defer finished.Done()
        f := strings.Fields(logData)
        for i := 3; i <= len(f); i += 3 {
            amt, err := strconv.Atoi(f[i-1])
            if err != nil {
                log.Fatal(err)
            }
            data.sum(f[i-2], amt)
            time.Sleep(100 * time.Millisecond) // e.g. slow hard disk
        }
    }()

    finished.Wait()
    data.show()
}

const logData = `2018 apples 9
2018 oranges 5
2019 apples 27
2019 lemons 13
2019 oranges 2
2020 oranges 1
2020 apples 16
2020 lemons 3`

That is all.
I hope this helps.


Taking a list and using it to build columns Say I have [1, 2, 3, 4, 5, 6, 7, 8, 9] and I want to spread it across - at most - 4 columns. The end result would be: [[1, 2, 3], [4, 5], [6, 7], [8, 9]]. So I made this:

item_list = [1, 2, 3, 4, 5, 6, 7, 8, 9]
item_count = len(item_list)
column_count = item_count if item_count <= 4 else 4
items_per_column = int(item_count / 4)
extra_items = item_count % 4

item_columns = []

for group in range(column_count):
    count = items_per_column
    if group < extra_items:
        count += 1

    item_columns.append(item_list[:count])
    item_list = item_list[count:]

Is there a nicer way of accomplishing this?

You could do something like this (elegant, but not the fastest approach)

item_list = [1, 2, 3, 4, 5, 6, 7, 8, 9]
min_height = len(item_list) // 4
mod = len(item_list) % 4

heights = [min_height + 1] * mod + [min_height] * (4 - mod)
indices = [sum(heights[:i]) for i in range(4)]
columns = [item_list[i:i+h] for i, h in zip(indices, heights)]

Or a compressed version:

columns = [item_list[
    min_height * i + (i >= mod) * mod :
    min_height * (i+1) + (i+1 >= mod) * mod]
    for i in range(4)]

I guess the fastest way would be similar to what you already have:

index = 0
columns = []
for i in xrange(4):
    height = min_height + (i < mod)
    columns.append(item_list[index:index+height])
    index += height

(With both of these, if there are less than 4 items you get empty columns at the end.)


Nand2Tetris Code Module I'm working through the Assembler assignment in the Nand2Tetris course (chapter 6). The suggested implementation contains 4 modules: Main, Parser, Code, SyntaxTree. Today I wanted to get some feedback on my implementation of the Code module.

The book specifies a general API in order to allow students to use whichever programming language they want; I choose Python 2.7. The Code module takes a line of a simplified dialect of Assembly code called Hack and produces binary.

The Code module only deals with a single class of instruction in Hack language: The C-Instruction. This instruction takes the form dest=comp;jump, where either dest= OR ;jump may be optionally omitted. comp is one of 28 valid expressions. dest is the destination to store the result of the computation: it can be any permutation of the three registers available (A, D, M). jump is a condition which, if true, moves control flow the address in instruction memory currently stored in register A.

The resulting opcode takes the form: 111a cccc ccdd djjj, where a and cccc cc correspond to the comp, ddd corresponds to the dest, and jjj corresponds to the jump. For more details, see Chapters 4 and 6 of the textbook.

Please review on style, implementation (especially those terribly ugly lookup tables), etc.

#!/usr/bin/python
# hack code translator

# Sample conversion:
# HACK Instruction: M = 1
# 111
# a = 0
# cccccc = 1111 11
# ddd = 001
# jjj = 000
# BINARY Opcode:    1110 1111 1100 1000

jumpTable = { 'null': '000',
              'JGT' : '001',
              'JEQ' : '010',
              'JEQ' : '010',
              'JGE' : '011',
              'JLT' : '100',
              'JNE' : '101',
              'JLE' : '110',
              'JMP' : '111'  }

compTable = { '0'   : '101010',
              '1'   : '111111',
              '-1'  : '111010',
              'D'   : '001100',
              'A'   : '110000',
              '!D'  : '001101',
              '!A'  : '110001',
              '-D'  : '001111',
              '-A'  : '110011',
              'D+1' : '011111',
              'A+1' : '110111',
              'D-1' : '001110',
              'A-1' : '110010',
              'D+A' : '000010',
              'D-A' : '010011',
              'A-D' : '000111',
              'D&A' : '000000',
              'D|A' : '010101' }

def dest(srcStr):
    # Of the three portions of the opcodes,
    # this is the only one that makes obvious logical sense
    # No point in a lookup table

    # Any combination of A, D, M
    # in that order
    # TODO: bitops this
    bits = ['0', '0', '0']  # 0 by default
    if 'A' in srcStr:
        bits[0] = '1'
    if 'D' in srcStr:
        bits[1] = '1'
    if 'M' in srcStr:
        bits[2] = '1'

    return ''.join(bits)

def comp(srcStr):
    aBit = '0'

    if 'M' in srcStr:
        if 'A' in srcStr:
            raise SyntaxError
        else:
            aBit = '1'
            nStr = srcStr.replace('M', 'A')  # this way we use only one lookup table

    else:
        nStr = srcStr[:]  # non-destructive copy

    return aBit + compTable[nStr]

def jump(srcStr):
    return jumpTable[srcStr]

The values in your lookup tables are strings of binary digits, for example '111010'. It would be closer to the way that real assemblers work if you represented these as numbers in binary, for example 0b111010. I presume you use string concatenation to assemble the instructions at present:

instruction = '111' + comp + dest + jump

With numeric values for the instruction fields, you'll need to use bitwise operations instead:

instruction = 0b111 << 13 | comp << 6 | dest << 3 | jump

There's no error handling. It's likely that assembly programs will contain mistakes, for example:

M=-1;JGR   // oops: typo for JGT

The user would like to get an error message, preferably one that gives the file and line number and an explanation of what went wrong:

prog.asm(17): bad jump field "JGR"

But as far I can see from the code you posted, what will happen here is that Python will raise a KeyError in jump, which is much less useful.

(Possibly there's error handling in the rest of the code that you didn't show us, but I can only comment on what I see here.)
It's even worse for the dest field: this doesn't seem to produce any errors at all, so if I write:

BAD=1;JEQ     // Oops: typo for MAD

then the assembler goes ahead as if I had written

AD=1;JEQ

and I never get told about my mistake.
The logic for lookup of the comp field is needlessly complex. It would be simplest just to make a 7-bit lookup table (including the a bit). Trying to modify the instruction wastes time, and it makes it harder to produce good error messages (outputting the modified instruction will confuse the user, so you have to hang on to the unmodified instruction for use in error messages).

If you don't want to write out the whole 7-bit lookup table, then write code to generate it:

fullCompTable = compTable.copy()
for k, v in compTable.items():
    if 'A' in k:
        fullCompTable[k.replace('A', 'M')] = 1 << 6 | v

and run this once, when the assembler starts up.
The comment for dest says "No point in a lookup table" but that's not true. A lookup table will be faster than the computation in dest. And it only needs 15 entries. Again, if you don't want to write it out by hand, you could compute it:

from itertools import permutations

destTable = {
    ''.join('MDA'[i] for i in p): sum(1<<i for i in p)
    for n in range(1, 4)
    for p in permutations(range(3), n)
}

but this is longer than writing it out in full:

destTable = {
    'A': 4, 'AD': 6, 'ADM': 7, 'AM': 5, 'AMD': 7, 'D': 2, 'DA': 6, 'DAM': 7,
    'DM': 3, 'DMA': 7, 'M': 1, 'MA': 5, 'MAD': 7, 'MD': 3, 'MDA': 7
}

There's no need to take a copy here:

nStr = srcStr[:]  # non-destructive copy

Just write:

nStr = srcStr


2 player CLI Tic-Tac-Toe game in python This is a fully functional code that i have just completed today. I just want to know how efficient is this. and how could i improve it or if theres any bugs that i have miss out.
import os
from AskUser import ask_user as As
from random import randint

#initialising the board
class Board():
    def __init__(self):
        self.boxs = [' '] * 10

    def display(self):
        print(f' {self.boxs[1]}  |  {self.boxs[2]}  |  {self.boxs[3]} \n---------------')
        print(f' {self.boxs[4]}  |  {self.boxs[5]}  |  {self.boxs[6]} \n---------------')
        print(f' {self.boxs[7]}  |  {self.boxs[8]}  |  {self.boxs[9]} ')

    def update_box(self, box_num, key):
        def valid_position(box_num, key):
            return 0 < box_num < 10 and self.boxs[box_num] == ' '
        while not valid_position(box_num, key):
            box_num = As("Please enter a valid position: ", int)
        self.boxs[box_num] = key

    def win_solution(self,key):
        if (self.boxs[1] == key and self.boxs[2] == key and self.boxs[3] == key) or\
            (self.boxs[4] == key and self.boxs[5] == key and self.boxs[6] == key) or\
            (self.boxs[7] == key and self.boxs[8] == key and self.boxs[9] == key) or\
            (self.boxs[1] == key and self.boxs[4] == key and self.boxs[7] == key) or\
            (self.boxs[2] == key and self.boxs[5] == key and self.boxs[8] == key) or\
            (self.boxs[3] == key and self.boxs[6] == key and self.boxs[9] == key) or\
            (self.boxs[1] == key and self.boxs[5] == key and self.boxs[9] == key) or\
            (self.boxs[3] == key and self.boxs[5] == key and self.boxs[7] == key):
            return True
        return False

    def tie_game(self):
        count = 0
        for i in self.boxs:
            if i != ' ':
                count += 1
        return count == 9

    def reset_board(self):
        self.boxs = [' '] * 10

board = Board()
Turn = randint(0,1)

#Introduction
def clear_screen():
    os.system('clear')
    print('Welcome to the TicTacToe simulator')
    #Displays the board
    board.display()

while True:
    if Turn == 1:
        #Updates the board after input
        clear_screen()

        #For X Player
        x_move = As('X, Choose a position between 1 - 9 : ',int)
        #Put x input into the board
        board.update_box(x_move, "X")
        Turn = 0
        #Updates the board after input
        clear_screen()
    #Check for win for X or tie
        if board.win_solution("X"):
            print('Congrats, X you have won')
            Turn = 3
        elif board.tie_game():
            print('This is a tie game')
            Turn = 3
        else:
            pass

    elif Turn == 0:
        clear_screen()
        #For O Player
        o_move = As('O, Choose a position between 1 - 9 : ',int)
        #Put o input into the board
        board.update_box(o_move, "O")
        Turn = 1
        #Updates the board after input
        clear_screen()

        #Check for win for O or tie
        if board.win_solution("O"):
            print('Congrats, O you have won')
            Turn = 3
        elif board.tie_game():
            print('This is a tie game')
            Turn = 3
        else:
            pass

    else:
        p_again = As('Do you want to play again? (Y/N) : ').upper()
        if p_again == "Y":
            board.reset_board()
            Turn = randint(0,1)
            continue
        else:
            exit('Game is over')

This is the AskUser function
Function to check for the user input
def ask_user(message, type_= str, valid=lambda x: True, invalid_message="Invalid"):
    while True:
        try:
            user_input = type_(input(message))
        except (ValueError, TypeError):
            print("Invalid input")
            continue
        if valid(user_input):
            return user_input
        else:
            print(invalid_message)

Matrix indexing

The display function is a handful. It's often better to use a matrix library like numpy to allow better indexing features. For instance,

import numpy as np
one_thru_nine = list(range(1,10))
print(one_thru_nine)
matrix = np.array(one_thru_nine).reshape(3,3)
print(matrix)
print(matrix[0, 1])
print(matrix[1, 1])
print(matrix[2, 2])

[1, 2, 3, 4, 5, 6, 7, 8, 9]
[[1 2 3]
 [4 5 6]
 [7 8 9]]
2
5
9

(Disclaimer: np.array(one_thru_nine).reshape(3,3) is not really an ideal way of initializing numpy arrays, it's just for demonstration purposes.)

Of course, do beware of zero-indexing.

Magic numbers

As a rule of thumb, avoid magic numbers. Try not to put raw integers into code.

return 0 < box_num < 10

will be more readable as

min_boxes, max_boxes = 0, 10
return min_boxes < box_num < max_boxes

Similarly, I can understand what Turn=3 does, but it's unintuitive.

If-Else statements

else:
  pass

is redundant. If you don't have anything to do when if and elif conditions fail, you don't have to add the else statement altogether -- unlike some other languages.

That is, you can just do:

while True:
  if condition_one:
    do_something()
  elif condition_two:
    do_something_else()

Class declaration

You don't need to put parentheses next to class declarations if you are not inheriting from a super class.

class Board():
   def __init__(self):

can just be

class Board:
   def __init__(self):

Casing

As a general rule of thumb, in classes are in PascalCase and everything else (modules, variables, functions, methods) are in snake_case. As such, from AskUser import ask_user as As should be reformatted as from ask_user import ask_user as as_as or something similar.

Similarly, things like Turn should be turn.

Wrap your module with main

In general, if you are not declaring globals, and you declare functions that you will actually call as main (perhaps you're familiar with C or Java?) you should wrap them in a statement of

if __name__ == "__main__":
  board = Board()
  turn = randint(0,1) ...

If you don't get into the habit of this, you'll end up executing code you don't want to when you're just importing code from other modules.

Others

I don't see the use of valid and invalid_message anywhere?


Parsing XML and prepend HTML every 3 elements I'm working on a 3 columns photo website (responsive), and I'm parsing the img src from XML. I need to prepend HTML code every 3 elements so my photo wall respects my responsive class.

I just found a cheap solution, repeating the XML loop (for). Do you have an advice to give me, to do it right, to improve my code?

$(document).ready(function(){
 xmldata=new Array();      //initialise array for XML data
 $.ajax({                      //get the XML data
  url: "xml/gallery.xml",             //URL to get the data from
 success: function(data) {         //if we succeed, set everything up. We don't expect    failure.
  $(data).find("image").each(function()
 {
  xmldata.push($(this));        //add item into array
 });

 totalNum = xmldata.length;

 gallery_images();

}

 });

});

 function gallery_images(){

 for (var i = 0; i <= 2; i++) {

 data=xmldata[i];

 img_src=$(data).attr("src");
 href=$(data).attr("href");
 title1=$(data).attr("title1");
 client=$(data).attr("client");

  var video_gallery_html = '';
  video_gallery_html += '

';
  video_gallery_html += '  ';
  video_gallery_html += '
DENIS ROVIRA
';
  video_gallery_html += '
See all projects

';
  video_gallery_html += '    ';
  video_gallery_html += '
Nescafe Gold

';

   $(".videos_container").prepend(video_gallery_html);

 };

 for (var i = 3; i <= 5; i++) {

 data=xmldata[i];

 img_src=$(data).attr("src");
 href=$(data).attr("href");
 title1=$(data).attr("title1");
 client=$(data).attr("client");

 var video_gallery_html2 = '';
 video_gallery_html2 += '

';
 video_gallery_html2 += '  ';
 video_gallery_html2 += '
DENIS ROVIRA
';
 video_gallery_html2 += '
See all projects

';
 video_gallery_html2 += '    ';
 video_gallery_html2 += '
Nescafe Gold

';

  $(".videos_container2").prepend(video_gallery_html2);

 };

 }

Perhaps using a template system can help you, check http://handlebarsjs.com/ you can integrate with jquery and reuse partial html code as a templates


Shuffle the elements of the array randomly I have written the code to shuffle the elements of the array. Please let me know of any loopholes in the written code.

package dataStructureInJava;

import java.awt.SystemTray;
import java.util.Random;
import java.util.Scanner;

public class ShuffleArr {
    public static void main(String[] args){
        System.out.println("Main Method Started");
        int [] arr = initArr();
        printArr(arr);
        arr = swapArr(arr);
        printArr(arr);
        System.out.println("Main MEthod Ended");
    }
    public static int[] initArr(){
        Scanner in = new Scanner(System.in);
        Random rand = new Random();
        System.out.println("Enter the limit of the arr");
        int limit = in.nextInt();
        System.out.println("Enter the maximum value of the arr");
        int maxValue = in.nextInt();
        int[] arr = new int[limit];
        for(int ii=0;ii<limit;ii++){
            arr[ii] =  rand.nextInt(maxValue);
        }
        return arr;
    }
    public static int[] swapArr(int[] arr){
        Random rand = new Random();
        for(int ii=0;ii<arr.length;ii++){
            arr = swap(arr,ii,rand.nextInt(arr.length));
        }
        return arr;
    }
    public static int[] swap(int[] arr,int ii,int jj){
        int temp=arr[ii];
        arr[ii]=arr[jj];
        arr[jj]=temp;
        return arr;
    }
    public static void printArr(int[] arr){
        System.out.println("Element of the arr are");
        System.out.print("{");
        for(int ii=0;ii<arr.length;ii++){
            System.out.print(arr[ii] + " ");
        }
        System.out.print("}");
        System.out.println();
    }
}

I got his question as an assignment and want to know if it is correct or not.

Naming

I would write array out instead of shortening it to arr (it's just two more letters), because it's more readable (but I do like that you are very consistent with your shortening).
is there a reason that you are using ii/jj instead of the more commonly used i/j?

Spacing

You should use a lot more spaces, because they make your code easier to read.

I would at least add spaces after , and ;, before {, and around all = and <.

You should also try to be more consistent with your spacing. For example, you have spaces around some =, but not all, and sometimes you write int [], other times int[].

Randomness

It seems that you implemented a variation of the fisher yates algorithm, which is actually not truly random, but biased:

  Similarly, always selecting j [rand.nextInt(arr.length) in your code] from the entire range of valid array indices on every iteration also produces a result which is biased, albeit less obviously so.

You can go to Wikipedia to read the explanation. Implemented correctly, the swapArr method should probably look like this:

public static int[] swapArr(int[] arr) {
    Random random = new Random();
    for (int i = arr.length - 1; i >= 0; i--) {
        int j = random.nextInt(i + 1);
        arr = swap(arr, i, j);
    }
    return arr;
}

Of course, Random only uses pseudorandom numbers anyways, so the slight bias might not be all that important.

Pass by Value

Strictly speaking, Java is pass-by-value, but you could still to this for example (because references of objects are passed, and thus changes to an object affect the object outside of the scope of a method):

public static void swap(int[] arr, int ii, int jj) {
    int temp = arr[ii];
    arr[ii] = arr[jj];
    arr[jj] = temp;
}

And then just call it like this:

swap(arr, i, j);

The same is true for swapArr. I think writing it this way is clearer (and also shorter), but it's probably a matter of taste.

Misc

I would split initArr into two methods: one that gets the user input, and one that creates the array (that way, it's easier to get the input from different places, for example hardcoded for testing).


Project Euler 7: 10001 Prime in Functional Programming (FP) I wanted to practice functional programming (fp) without using any library but using vanilla JS only. So I took a problem from Project Euler:

  By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can
  see that the 6th prime is 13.

  What is the 10 001st prime number?

This is my solution:

(function () {
  'use strict';

  function* checkForPrime(n) {
    let m = Math.floor(Math.sqrt(n) + 1);
    while (true) {
      if (n < 2) {
        yield false;
        return;
      }
      if (m < 2 || n === 2 || n === 3) {
        yield true;
        return;
      }
      if (n % m === 0) {
        yield false;
        return;
      }
      m--;
    }
  }

  const isPrime = n => checkForPrime(n)
    .next();

  function* searchForPrimeNumbers(max) {
    let primeCounter = 0;
    let newPrimeNumber = 1;
    let runningNumber = 1;
    while (true) {
      if (primeCounter === max) {
        yield newPrimeNumber;
        return newPrimeNumber;
      }
      if (isPrime(runningNumber)
        .value) {
        newPrimeNumber = runningNumber;
        primeCounter = primeCounter + 1;
      }
      runningNumber++;
    }
  }

  const finalsolution = searchForPrimeNumbers(1e4 + 1);

  const findPrimeTill10001 = () => {
    return finalsolution.next()
      .value ? finalsolution.next()
      .value : findPrimeTill10001();
  };

  const solution = findPrimeTill10001();
  console.log(solution);
})();

As you can see, it is not consistent with the idea of FP. First I wanted to write it using recursion but I hit the stack limit. Therefore I used generators (and loops). That was the only solution I could come up with that resembles FP.

Any suggestions how to write in FP without any additional FP-Library (i.e. in pure JS only) is much appreciated.

You shouldn't be doing things in a loop that don't depend on things changing in the loop. n < 2, n === 2, and  n === 3 all depend on n, which is constant throughout your loop. Also, if you store your primes, you just have to loop through the primes until you get to sqrt(n), rather than checking every number. There are further tricks to speed up the algorithm, such as checking n only if n mod 60 is in [7,11,13,17,23,29,31,37,41,43,47,49,53,59]; that reduces the number of n to check by more than 3/4.

Is tail recursion an option?

How large is your stack size? You can break recursion into subrecursion. For instance, suppose you have a function that recursively finds the nth to n+100th primes. Then you can have a recursive function than, when asked to find k primes, calls itself on k-101, then appends the k-100 to kth primes. You can then add another layer, and have something that does 10 primes at a time. Do this in binary rather than base 10, and you'll need 14 layers to get 10001 primes.


Word Squares Generator I've solved the Word Squares problem:

  Given a set of words (without duplicates), find all word squares you can build from them.

  A sequence of words forms a valid word square if the kth row and column read the exact same string, where 0 ≤ k < max(numRows, numColumns).

  For example, the word sequence ["ball","area","lead","lady"] forms a word square because each word reads the same both horizontally and vertically.

 b a l l
 a r e a
 l e a d
 l a d y

but the following code, had a runtime so slow that I ended up in the bottom 10% of submissions speed wise. I was wondering if there's anything I'm doing in the code that's a performance killer / any way I could improve my algorithm to speed it up.

from collections import defaultdict

class Solution(object):
    def make_defaultdict(self):
        return defaultdict(self.make_defaultdict)

    def make_trie(self, words):
        trie = self.make_defaultdict()

        for word in words:
            cur = trie
            for l in word:
                cur = cur[l]
            cur['end'] = True

        return trie

    def search_trie(self, trie, search_str):
        matches = []
        sub_trie = trie

        for l in search_str:
            if l not in sub_trie:
                return []
            sub_trie = sub_trie[l]

        q = [(search_str + k, sub_trie[k]) for k in sub_trie.keys()]
        while q:
            fragment, sub_trie = q.pop(0)

            if 'end' in sub_trie:
                matches.append(fragment)
            else:
                q.extend([(fragment + k, sub_trie[k]) for k in sub_trie.keys()])

        return matches

    def make_square(self, pos, square, size, trie):
        if pos == size:
            return [square]

        for i in range(pos, size):
            search_str = ''.join([w[i] for w in square])
            candidates = self.search_trie(trie, search_str)

            if not candidates:
                return []

            result = []
            for candidate in candidates:
                result.extend(self.make_square(pos + 1, square + [candidate], size, trie))

            return result

    def wordSquares(self, words):
        all_squares = []
        trie = self.make_trie(words)

        for word in words:
            all_squares.extend(self.make_square(1, [word], len(word), trie) )

        return all_squares

Any thoughts on how to speed it up?

1. Review

There are no docstrings. What do any of these functions do? How do I call them? What do they return?
Just because leetcode.com insists on a Solution class, doesn't mean that all functions need to be members of that class. Write plain functions and avoid the unnecessary use of self.
I like the implementation of make_defaultdict, but this doesn't just make a defaultdict, it makes an empty prefix tree (trie), so a name like make_empty_prefix_tree would be better. (I prefer the name "prefix tree" because it's clear what it is from the name, whereas the name "trie" is a bit mysterious.)
Distinguishing the end of the prefix_tree by a special key like 'end' ought not to be necessary in this application (constructing word squares) because all of the words are the same length.
When you need to iterate over the keys and values of dictionary simultaneously, use dict.items. For example, this line:

q = [(search_str + k, sub_trie[k]) for k in sub_trie.keys()]

becomes

q = [(search_str + k, v) for k, v in sub_trie.items()]

avoiding the lookup of sub_trie[k].

2. Performance

In order to understand what the code is doing, it is handy to trace its execution on an example. This can be done using the Python debugger, for example we might print the value of square on each call to make_square, like this:

>>> pdb.run("Solution().wordSquares('tide idea dear yarn tidy'.split())")
> <string>(1)<module>()
(Pdb) break Solution.make_square
Breakpoint 1 at cr176612.py:38
(Pdb) commands 1
(com) silent
(com) p ' '.join(square)
(com) continue
(Pdb) continue
'tide'
'tide idea'
'tide idea dear'
'idea'
'idea dear'
'dear'
'yarn'
'tidy'
'tidy idea'
'tidy idea dear'
'tidy idea dear yarn'

(You could get a similar result by editing the code to add a print at the start of make_square but it is a good idea to practice using the debugger on easy examples like this, so that when you have a tough bug to investigate you are proficient with it.)

Looking at the output carefully you can see the problem: after choosing tide for the first row, the search continued by choosing idea for the second row. But this should have been ruled out because then the fourth column would have to start ea and nothing in the dictionary will fit there.

So work is wasted following search paths that could have been rejected earlier if all constraints had been applied. The problem is that only one constraint is applied at a time — on the \$i\$th loop in make_square we use search_trie to apply the constraint for the \$i\$th column only, failing to apply the constraints from columns \$j > i\$.

3. Organizing the work

It takes some thought to figure out how to organize the work so that all constraints are applied at the earliest possible stage. This is the kind of algorithm where it makes sense to work out the details on paper for a simple example, before starting to write code.

Suppose our dictionary consists of all, led, and lee, making the following prefix tree:

The algorithm is going to need the idea of "current position in the prefix tree". I'm going to draw this with a red triangle. For example, if l and e have been matched, then the current position looks like this:

In this position, d and e are the possible matches for the next letter.

Now, in a 3×3 word square there are nine squares to be filled:

In the real problem the result is required to be symmetric, so that choice 2 is the same as choice 4 and so on, but it will be clearer, I think, to solve the general problem first and then figure out how to take advantage of symmetry later.

When we come to make a choice for a square there are two constraints we have to satisfy, a horizontal constraint for the row and a vertical constraint for the column. The constraints correspond to positions in the prefix tree. For choice number 1, the constraints in both directions correspond to the root of the prefix tree, like this:

The vertical constraint says that we can choose a or l, and so does the horizontal constraint. Let's suppose that we choose a. Then we can update the position in the prefix tree to make constraints for the next cells, like this:

Now we can consider the constraints bearing on choice number 2, bringing in a fresh vertical constraint:

Here, the vertical constraint says that we can choose a or l, and the horizontal constraint says that we can only choose l, so this is our only way to satisfy both constraints. Once again we can propagate the constraints:

The process continues through the choices in order, until all constraints have been applied and a solution has been found (or until a dead end has been reached where the constraints cannot be satisfied, in which case we have to backtrack). If a solution is found, the data structures will look something like this:

4. Data structures

Given the approach outlined in §3 above, how can we represent the state of the search in a data structure? Well, we will need choices (the black squares in the diagram below) which we will consider in order. For each choice we have constraints bearing on that choice (the blue diamonds connected to the choice by blue lines), and there are rules specifying how to propagate the constraints after a choice has been made (the red arrows which join two blue diamonds by passing through a black square).

We can represent constraints by objects with a node attribute:

class Constraint:
    node = None     # Node in the prefix tree, or None if not initialized.

(These need to be objects rather than, say, named tuples, because the node gets modified when a constraint is propagated.)

We can represent choices as objects with lists of constraints and propagation rules:

class Choice:
    def __init__(self, constraints, rules):
        """Create a choice.

        Arguments:
        constraints: List of Constraint objects.
        rules: List of propagation rules as pairs (Constraint, Constraint).

        """
        self.constraints = constraints
        self.rules = rules

    def options(self):
        "Return set of available options, assuming initialized constraints."
        # Intersect all the keys of the constraint dictionaries.
        constraints = iter(self.constraints)
        options = next(constraints).node.keys()
        for c in constraints:
            options &= c.node.keys()
        return options

    def propagate(self, choice):
        "Propagate a choice from source to destination constraints."
        for source, dest in self.rules:
            dest.node = source.node[choice]

Now finding all assignments that meet the constraints is a straightforward depth-first search, and a convenient way to implement this in Python is to use the stack of iterators pattern, like this:

def assignments(choices):
    """Generate satisfying assignments to a sequence of choices."""
    n = len(choices)
    assignment = [None]
    stack = [iter(choices[0].options())]

    # Cache methods in local variables to avoid lookups in the loop
    assignment_append = assignment.append
    assignment_pop = assignment.pop
    stack_append = stack.append
    stack_pop = stack.pop

    while stack:
        for option in stack[-1]:
            i = len(assignment)
            if i < n:
                assignment_append(option)
                choices[i - 1].propagate(option)
                stack_append(iter(choices[i].options()))
                break
            yield tuple(assignment[1:]) + (option,)
        else:
            assignment_pop()
            stack_pop()

Notice that this generates the assignments one by one instead of collecting them all and returning a list. Generating results is more flexible than returning a list: for example, if you only want one assignment then you can use next(assignments(choices)), or if you want to see whether there is a unique assignment or not then you can use islice(assignments(choices), 2), without having to wait for the search to run all the way to completion.

It's a bit fiddly to build the data structure, but we can do it like this:

from itertools import product

def word_squares(words):
    """Generate word squares from a sequence of words."""
    n = len(words[0])
    assert all(len(word) == n for word in words)
    prefix_tree = make_prefix_tree(words)

    # Horizontal and vertical constraints for each choice.
    N = range(n)
    horiz, vert = [[[Constraint() for _ in N] for _ in N] for _ in range(2)]
    for k in N:
        horiz[k][0].node = vert[0][k].node = prefix_tree

    # Create choices.
    choices = []
    for i, j in product(range(n), repeat=2):
        h, v = horiz[i][j], vert[i][j]
        rules = []
        if j < n - 1:
            rules.append((h, horiz[i][j + 1]))
        if i < n - 1:
            rules.append((v, vert[i + 1][j]))
        choices.append(Choice([h, v], rules))

    # Find assignments and reformat them.
    for assignment in assignments(choices):
        yield [''.join(word) for word in zip(*[iter(assignment)] * n)]

5. Adding symmetry

Now it ought to be clear how to implement symmetry. We drop the squares that we don't need to fill, and reroute the constraint propagation like this:

It's still fiddly to build the choices structure, but it can be done like this:

from itertools import combinations_with_replacement

def symmetric_word_squares(words):
    """Generate symmetric word squares from a sequence of words."""
    n = len(words[0])
    assert all(len(word) == n for word in words)
    prefix_tree = make_prefix_tree(words)

    # Horizontal and vertical constraints for each choice (some we
    # won't use, but it's simpler like this).
    N = range(n)
    horiz, vert = [[[Constraint() for _ in N] for _ in N] for _ in range(2)]
    for k in N:
        horiz[k][0].node = vert[0][k].node = prefix_tree

    # Create choices.
    choices = []
    for i, j in combinations_with_replacement(N, 2):
        h, v = horiz[i][j], vert[i][j]
        rules = []
        if i == j:
            constraints = [v]
            if j < n - 1:
                rules.append((v, horiz[i][j + 1]))
        else:
            constraints = [h, v]
            if i != j < n - 1:
                rules.append((h, horiz[i][j + 1]))
            if j != i < n - 1:
                rules.append((v, vert[i + 1][j]))
        choices.append(Choice(constraints, rules))

    # Find assignments.
    grid = [[None] * n for _ in N]
    for assignment in assignments(choices):
        for a, (i, j) in zip(assignment, combinations_with_replacement(N, 2)):
            grid[i][j] = grid[j][i] = a
        yield [''.join(word) for word in grid]

Note the use of itertools.combinations_with_replacement here — this is useful when you would otherwise need two nested loops to loop over all \$0 \le i \le j < n\$:

    for i in range(N):
        for j in range(i, N):

Here combinations_with_replacement combines the two loops into one, saving a level of indentation.

6. Timing comparisons

I got kind of carried away there with data structures and algorithms, so I'm not sure if the revised code will really be any faster. Let's try an example, using all the 4-letter words in my system dictionary, which can make 4.3 million symmetric word squares:

>>> words = [w for w in map(str.strip, open('/usr/share/dict/words'))
...          if len(w) == 4 and w == w.lower()]
>>> len(words)
4347
>>> sum(1 for _ in symmetric_word_squares(words))
4264868
>>> timeit(lambda:sum(1 for _ in symmetric_word_squares(words)), number=1)
49.30328282299888
>>> timeit(lambda:Solution().wordSquares(words), number=1)
66.61988531300449

So the revised code takes about 75% as long as the original.


The next prime number in Python I have written a solution to a DM::OJ exercise to find the next Prime Number after a given number (input).  I'm seeking to optimize it so that it will run for any number up to 1000000000 in 2 seconds or less.

Here is my code:

from functools import reduce

num = int(input())

def factors(n):
  return set(reduce(list.__add__,
  ([i, n//i] for i in range(1, int(n**0.5) + 1) if n % i == 0)))

f = (factors(num))

if (len(f) != 2):
  while (len(f) != 2):
    num += 1
    f = factors(num)
  print (num)
else:
  print (num)

Just as a side note, I also found another way to execute this exact same thing, but that one is slower, so I wouldn't mind bothering with that way. However, just for reference though, I'm still adding the slow one anyways:

num = int(input())

def factors(x):
  a = []
  for i in range(1, x + 1):
      if x % i == 0:
          a.append(i)
  return a

f = (factors(num))

if (len(f) != 2):
  while (len(f) != 2):
    num += 1
    f = factors(num)
  print (num)
else:
  print (num)

if (len(f) != 2):
  while (len(f) != 2):
    num += 1
    f = factors(num)
  print (num)
else:
  print (num)

In places where you use both if and while, they can usually be collapsed into a single while. You can think of an if as a single use while. In your first if block, the final print(num) would only get executed once len(f) becomes 2, so it can just be left outside of the while loop. Which will make something like this:

while (len(f) != 2):
  num += 1
  f = factors(num)
print (num)

As for ways of speeding up, you could consider keeping a list of previous primes and only dividing by those. That will cut down on the comparisons in the factor search. It might require some prior calculation, but the next calculations will be faster.

Another thing that could help is taking steps bigger than +1 while looking for the next prime. We know that all even numbers bigger than 2 are not prime, for example, and also that all numbers divisible by 3 that are bigger than 3 are not prime. So out of every 6 numbers, only two are possibly prime. If we use a counter variable n, then 6n, 6n+2, 6n+3, and 6n+4 are definitely not prime, so you could just check 6n+1 and 6n+5. Although that would require rearranging some of your program.


adding properties together from list of objects using LINQ Looking to see if there is a simpler way to achieve this. I would love if I could keep the return as a one-line LINQ expression. I want to grab the total number of hints used, which can be more than one.

public int hintsUsed
{
    get
    {
        int i = 0;
        questions.ForEach(q => i += q.hintsUsed);
        return i;
    }
}

You can use the Sum function

public int HintsUsed => questions.Sum(q => q.hintsUsed);

By using an expression bodied member (since C# 6.0), you can even get rid of the get and the return keywords plus a few braces.

The code above is equivalent to

public int HintsUsed
{
    get
    {
        return questions.Sum(q => q.hintsUsed);
    }
}

Most of the LINQ-to-Objects functionality is provided by the Enumerable Class.


How could I remove repetition from this linq-to-entities query? This linq query works, and it does what I want it to do, but is there any other way I could improve the query so I'm not repeating a.AnswerRevisions.OrderByDescending(r => r.DateCreated).FirstOrDefault()?

IQueryable<Article> query = from a in _db.Articles
                            orderby a.DateCreated descending
                            where a.AnswerRevisions.Count > 0 &&
                            (a.AnswerRevisions.OrderByDescending(r => r.DateCreated).FirstOrDefault().UpVotes -
                             a.AnswerRevisions.OrderByDescending(r => r.DateCreated).FirstOrDefault().DownVotes < 0)
                            select a;

Use the let statement and also do the final orderby after the where clause, this will increase efficiency, as less entries will have to be sorted

IQueryable<Article> query = from a in _db.Articles
    let  rev = a.AnswerRevisions.OrderByDescending(r => r.DateCreated).FirstOrDefault()
    where a.AnswerRevisions.Count > 0 &&
          (rev.UpVotes - rev.DownVotes < 0)
    orderby a.DateCreated descending
    select a;


How can I reduce this .less call to one line per image How can I simplify this .less code to require less repetition (i.e., so that I can have many buttons with normal, rollover, and pressed states without using 3 lines for each button):

.SBtnM(@urlprefix, @width, @height, @urlsuffix) {
        width: @width;
        height: @height;
        background-image: url(%("%s_%s.png", @urlprefix,@urlsuffix));
}

@url1prefix : 'chrome://xtoolbar/skin/img1';
toolbarbutton > .btnCSS1 {.SBtnM(@url1prefix, 42px, 24px,'normal');}
toolbarbutton:hover > .btnCSS1 {.SBtnM(@url1prefix, 42px, 24px, 'rollover');}
toolbarbutton[checked="true"] > .btnCSS1 {.SBtnM(@url1prefix, 42px, 24px, 'pressed');}

Ideally, I would want to be able to use a one-liner to handle all 3 states.  E.g., something like this:

.SBtnM(@urlprefix, @width, @height) {
    //Magic Happens.  This part may become more complicated
}

.btnCSS1 {.SBtnM('chrome://xtoolbar/skin/img1', 42px, 24px);}

Output, for reference:

toolbarbutton > .btnCSS1 {
  width: 42px;
  height: 24px;
  background-image: url("chrome://xtoolbar/skin/img1_normal.png");
}
toolbarbutton:hover > .btnCSS1 {
  width: 42px;
  height: 24px;
  background-image: url("chrome://xtoolbar/skin/img1_rollover.png");
}
toolbarbutton[checked="true"] > .btnCSS1 {
  width: 42px;
  height: 24px;
  background-image: url("chrome://xtoolbar/skin/img1_pressed.png");
}

As far as I can tell, .less doesn't have any sort of parent selector for mixins.

I don't see anything wrong with what you are doing but. it can be written shorter just as you asked.

.SBtnM(@selector, @urlprefix, @width, @height)
{
   .@{selector}
   {
       .toolbarbutton > &
       {
            @url: %("%s_normal.png", @urlprefix);
            width: @width;
            height: @height;
            background-image:url(@url);
       }
       .toolbarbutton:hover > &
       {
            @url:%("%s_rollover.png", @urlprefix);
            background-image:url(@url);
       }
       .toolbarbutton[checked="true"] > &
       {
             @url:%("%s_pressed.png", @urlprefix);
            background-image:url(@url);
       }
   }
}

.SBtnM(btnCSS1, 'chrome://xtoolbar/skin/img1', 42px, 24px);

For me this produces:

.toolbarbutton > .btnCSS1 {
  width: 42px;
  height: 24px;
  background-image: url("chrome://xtoolbar/skin/img1_normal.png");
}
.toolbarbutton > .btnCSS1 {
  background-image: url("chrome://xtoolbar/skin/img1_rollover.png");
}
.toolbarbutton[checked="true"] > .btnCSS1 {
  background-image: url("chrome://xtoolbar/skin/img1_pressed.png");
}

Works for LESS 1.3.1+ ONLY. For previous versions the syntax is slightly different.


Toggling between radio buttons I have several buttons and I want to toggle between them like radio buttons.

The way I have it set up now is that each button have this same code but the buttonValue varies depending on which button they click on.

ToggleButton one;
one = (ToggleButton) view.findViewById(R.id.number_one);
one.setOnClickListener(new View.OnClickListener()  {
    public void onClick(View view) {
        buttonValue = 1;
        ToggleButton tb = (ToggleButton) view;
        if(tb.isChecked()) {
            RadioGroup gridLayout = (RadioGroup) tb.getParent();
            for(int i=0; i<(gridLayout).getChildCount(); ++i) {
                View nextChild = (gridLayout).getChildAt(i);
                if(nextChild instanceof ToggleButton && nextChild.getId()==tb.getId()){

                }else if (nextChild instanceof ToggleButton && nextChild.getId()!=tb.getId() ){
                    ToggleButton tb2=(ToggleButton) nextChild;
                    tb2.setChecked(false);
                }
            }
        } else{
            RadioGroup gridLayout = (RadioGroup) tb.getParent();
            for(int i=0; i<(gridLayout).getChildCount(); ++i) {
                View nextChild = (gridLayout).getChildAt(i);
                if(nextChild instanceof ToggleButton && nextChild.getId()==tb.getId() ){
                    ToggleButton tb2=(ToggleButton) nextChild;
                    tb2.setChecked(false);
                }else if (nextChild instanceof ToggleButton && nextChild.getId()!=tb.getId() ){
                    ToggleButton tb2=(ToggleButton) nextChild;
                    tb2.setChecked(false);
                }
            }
        }
    }
});

I basically have the same code for 10 buttons! While the code works and the buttons toggle between each other, I don't think is the best approach (especially because it's so many lines of code for 10 buttons where the main isChecked() function is exactly the same in every onClick button listener.

I was wondering if there was a way so that I only have one button listener that increments the buttonValue based on what button was clicked and keeps the main isChecked() function the same.

The trick here is to create an inner class.
Something I've started doing recently is to create a method that returns the interface you want!
private View.OnClickListener createClickListener(final int value) {
    return new View.OnClickListener()  {
        buttonValue = value;
        // the rest of your code goes here
    };
}

Thanks to marking the int value as final in the method declaration, it is accessible also from the inner class.
You can now use this like:
one.setOnClickListener(createClickListener(1));

Your actual code
While reducing the code duplication for the individual buttons is important, let's take a look at the code that is actually performed for each button!
Step 1. Adjust spacing to adhere to the Java coding standards and remove unnecessary parenthesis.
Step 2. gridLayout is used in both if and else, so put it outside.
Step 3. In the else part, you're doing the same thing no matter if nextChild.getId() and tb.getId() is the same or not, so reduce code duplication by simplifying that logic.
Step 4. Remove empty if statement
After these steps, the code is:
ToggleButton tb = (ToggleButton) view;
RadioGroup gridLayout = (RadioGroup) tb.getParent();
if (tb.isChecked()) {
    for (int i = 0; i < gridLayout.getChildCount(); ++i) {
        View nextChild = (gridLayout).getChildAt(i);
        if (nextChild instanceof ToggleButton && nextChild.getId() != tb.getId()) {
            ToggleButton tb2 = (ToggleButton) nextChild;
            tb2.setChecked(false);
        }
    }
} else {
    for (int i = 0; i < gridLayout.getChildCount(); ++i) {
        View nextChild = gridLayout.getChildAt(i);
        if (nextChild instanceof ToggleButton) {
            ToggleButton tb2 = (ToggleButton) nextChild;
            tb2.setChecked(false);
        }
    }
}

Next steps:

Put the for loop outside the if-statement
continue early if it is not an instance of ToggleButton
Adjust the if condition to take into consideration the tb.isChecked() from outside, and use an || (OR) operation for the logic.
Rename tb to clickedButton which more describes what the button is used for.

I believe this code will have the exact same effect as your original:
ToggleButton clickedButton = (ToggleButton) view;
RadioGroup gridLayout = (RadioGroup) clickedButton.getParent();
for (int i = 0; i < gridLayout.getChildCount(); ++i) {
    View nextChild = gridLayout.getChildAt(i);
    if (!(nextChild instanceof ToggleButton)) {
        continue;
    }
    if (nextChild.getId() != clickedButton.getId() || !clickedButton.isChecked()) {
        ToggleButton tb2 = (ToggleButton) nextChild;
        tb2.setChecked(false);
    }
}


Parsing length of work experience using Composite design pattern I want to get a number from given text (if it exists) which is about work experience in months. I assumed that number is in months when in the text one writes for example "27 miesięcy" or "27 miesiecy" or "27miesięcy" or "27miesiecy, and in years if one writes "3 lat" or "3lat".

Class ExperienceAlg0 is mainly responsible for that. I wanted to make it flexible and open for adding another rules/algorithms "how to find the number of months in the text". I tried to use composite design pattern.

The algorithm works well, but is the design OK according to OOP and Uncle Bob?

import java.util.ArrayList;
import java.util.List;

import org.junit.Test;

import junit.framework.TestCase;

public class TestExperienceAlg0 extends TestCase {

/*
**
** this test class unit test show how app works
**
*/
    @Test
    public void testCalc1() {
        // Given:
        String text = "programuję od 5 lat.";

        // When:
        List<ExperiencePattern> patternList = new ArrayList<>();
        patternList.add(new ExperiencePatternMonths());
        patternList.add(new ExperiencePatternYears());
        ExperienceAlg0 alg = new ExperienceAlg0(patternList);
        int result = alg.calc(text);

        // Then:
        assertEquals(0, result);
    }
}

public interface ExperiencePattern {

    public int getPattern(String text);

}

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class ExperiencePatternMonths implements ExperiencePattern{

    @Override
    public int getPattern(String text) {
        int result = 0;
        Pattern p = Pattern.compile("\\d+miesi[e|ę]cy|\\d+ miesi[e|ę]cy");
        Matcher m = p.matcher(text);

        while (m.find()) {
            String tmp = m.group();
            tmp = tmp.replaceAll("\\D", "");
            int x = Integer.parseInt(tmp);
            if (x < (40 * 12)) { // assuming that the max experience is 40
                                    // years.
                result = x;
                ;
            }
        }
        return result;
    }

}

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class ExperiencePatternYears implements ExperiencePattern{

    @Override
    public int getPattern(String text) {
        int result = 0;
        Pattern p = Pattern.compile("\\d+lat|\\d+ lat");
        Matcher m = p.matcher(text);

        while (m.find()) {
            String tmp = m.group();
            tmp = tmp.replaceAll("\\D", "");
            int x = Integer.parseInt(tmp);
            if (x < 40) { // assuming that the max experience is 40 years.
                result = x * 12;
            }
        }
        return result;
    }

}

public interface Algorithm <T> {

    public T calc(String textToLower);

}

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class ExperienceAlg0 implements Algorithm<Integer> {

    List<ExperiencePattern> listPatterns;

    public ExperienceAlg0(List<ExperiencePattern> list){
        listPatterns = list;
    }

    @Override
    public Integer calc(String text) {
        List<Integer> result = new ArrayList<>();

        for(ExperiencePattern ep : listPatterns){
            result.add(ep.getPattern(text));
        }

        Collections.sort(result, new DescendingComparator());
        return result.get(0);
    }

}

import java.util.Comparator;
public class DescendingComparator implements Comparator<Integer>{

    @Override
    public int compare(Integer x1, Integer x2) {

        return x2 - x1;
    }

}

You wrote this:

    Pattern p = Pattern.compile("\\d+miesi[e|ę]cy|\\d+ miesi[e|ę]cy");

but it would be more natural to express it without alternation:

    Pattern p = Pattern.compile("\\d+\\s*miesi[e|ę]cy");

I wonder if you maybe wanted to throw in a CASE_INSENSITIVE | UNICODE_CASE flag.

public interface ExperiencePattern {

Many projects would choose to spell this IExperiencePattern.

DRY - don't repeat yourself. It seems odd that you have a pair of copy-n-pasted identical classes with different Pattern constants, rather than one class that accepts a Pattern parameter.

        tmp = tmp.replaceAll("\\D", "");

Thank you for teaching me about the "non-digit" pattern. But the more natural way to express your intent would be to delete this, and put a capture group in your regex:

    Pattern p = Pattern.compile("(\\d+)\\s*miesi[e|ę]cy");

Then Integer.parseInt(m.group(1)) gives you exactly what you want.

        if (x < 40) {
            result = x * 12;

Ok, I guess in a straight refactor you would need to pass in two parameters, a Pattern and also maxAcceptableValue. But it's probably better to stick with a single parameter and let the caller do range checking.

public interface Algorithm <T> {

    public T calc(String textToLower);

Both of those identifiers are hopelessly vague. Give the Gentle Reader a clue, please. Also, adding some /** javadoc */ would help.

public ExperienceAlg0(List<ExperiencePattern> list){
    listPatterns = list;
}

It is customary in a ctor to explicitly mention "this": this.listPatterns = list;

    for(ExperiencePattern ep : listPatterns) {
        result.add(ep.getPattern(text));
    }

This is perfectly lovely. But generalizing to exactly 2 cases is maybe on the small side. Rather than this loop, I encourage you to hoist the range check test up to this location, which will feature a pair of nearly identical calls using each of your patterns.

    Collections.sort(result, new DescendingComparator());
    return result.get(0);

That's cool. I like it. Very clean way to discard chaff.

You asked if the design is OK according to OOP. Well, that would be easier to evaluate if you added one-sentence javadocs at top of each class, explaining the single responsibility of that class, especially for any interface classes. That would e.g. help a reader pin down which layer the range-checking belongs in.


Finding prime divisors with wheel factorization I am just starting to learn C++, and I wrote was the following implementation of the trial division algorithm to find the prime factors of a positive integer. I'm hoping the community here can suggest changes to both the algorithm and to the implementation. I barely know any language-specific features of C++, so I'm sure there is a lot of room for improvement.

In particular, I would like to be able to the following things:

Work with larger integers than what int allows.
Catch errors arising from command-line arguments that cannot be parsed as ints.
Remove the hard-coding of the wheel and the cases of the first primes 2, 3, and 5.
Improve general style/use more features of C++.

#include <iostream>

/*
 * Print out p as many times as it divides n.
 * Return the quotient of n by the highest power of p dividing n.
 */
int checkdivisor(int n, int p) {
  while (n % p == 0) {
    std::cout << p << std::endl;
    n /= p;
  }
  return n;
}

// Wheel for eliminating more composite numbers.
const int PERIOD = 7;
const int WHEEL235[PERIOD] = {4, 2, 4, 2, 4, 6, 2};

/*
 * Print the prime divisors of n.
 * The algorithm is trial division with 2-3-5 wheel factorization.
 */
void primedivisors(int n) {
  if (n > 1) {
    // Check 2, 3, and 5 individually
    n = checkdivisor(n, 2);
    n = checkdivisor(n, 3);
    n = checkdivisor(n, 5);
    // Start at the next potential prime divisor, 7.
    int p = 7;
    int i = 0;
    while (n > 1) {
      // If p^2 > n, then n is the last remaining prime divisor.
      if (p * p > n) {
        std::cout << n << std::endl;
        return;
      }
      // Check if p is a prime divisor.
      n = checkdivisor(n, p);
      // Increment p based on the wheel.
      p += WHEEL235[i++ % PERIOD];
    }
  }
}

/*
 * Parse command-line arguments as ints (if possible), and print their prime
 * factors.
 */
int main(int argc, char* argv[]) {
  for (int i = 1; i < argc; i++) {
    primedivisors(std::stoi(argv[i]));
  }
}

Work with larger integers than what int allows

Here, you seem to be asking for templates, so that your code works with any integer (and actually, any type which properly acts like an integer). I will convert the checkdivisor function for you so that you can analyze it, then templating the rest of the code will be left as an exercice:

template<typename Integer>
Integer checkdivisor(Integer n, Integer p) {
  while (n % p == 0) {
    std::cout << p << std::endl;
    n /= p;
  }
  return n;
}

Actually, it should even work with any type for which the operations %, / and == have the desired semantics.

Catch errors arising from command-line arguments that cannot be parsed as ints

That one is easy if you look at the documentation of std::stoi: it says that std::stoi throws an std::invalid_argument exception if the string does not represent an integer and an std::out_of_range exception if the string to parse represents an integer too big for the type to hold. Knowing that, you can set up a simple try/catch to handle the errors:

int main(int argc, char* argv[]) {
  for (int i = 1; i < argc; i++) {
    try {
      primedivisors(std::stoi(argv[i]));
    }
    catch (const std::invalid_argument& err) {
      std::cerr << "the input is not an integer\n";
    }
    catch (const std::out_of_range& err) {
      std::cerr << "the input is too big\n";
    }
  }
}


Thread safe cache implementation Please review this implementation of thread-safe chache.

public class StaticCacheImpl<T extends Entity> implements StaticCache<T>
{

    private boolean set = false;

    private List<T> cachedObjects = Collections.synchronizedList(new ArrayList<T>());

    @Override
    public synchronized boolean isSet() {
        return set;
    }

    @Override
    public synchronized void put(List<T> cachedList) {
        if (!set) {
            cachedObjects.addAll(cachedList);
            set = true;
        }
    }

    @Override
    public synchronized List<T> get() {
        return cachedObjects;
    }

    @Override
    public synchronized boolean contains(UUID uuid) {
        if(set) {
            for (T t : cachedObjects) {
                if (ObjectUtils.equals(uuid, t.getId())) {
                    return true;
                }
            }
        }

        return false;
    }

    @Override
    public synchronized T getById(UUID uuid) {
        if(set) {
            for (T t : cachedObjects) {
                if (ObjectUtils.equals(uuid, t.getUuid())) {
                    return t;
                }
            }
        }
        return null;
    }
}

Your cache can only be initialised once with the put method (hence your set flag): why not use a constructor instead and get rid of the flag?
If 2 items with the same UUID are identical and you only need to store one of them, using a map would simplify your design a lot
The map gives you an O(1) algorithm for contains and getById instead of O(n) in your code
Since the class is now immutable, you don't need to synchronize the methods any more which should result in much better result in case of heavy concurrency (no lock contention)

Here is what I would propose:

public class StaticCacheImpl<T extends Entity> implements StaticCache<T>
{
    //Assumes that 2 items with the same UUID only need to be stored once
    private final Map<UUID, T> cache = new HashMap<UUID, T>();

    public StaticCacheImpl(List<T> cachedList) {
        for (T t : cachedList) {
            cache.put(t.getUuid(), t);
        }
    }

    @Override
    public boolean isSet() {
        return true;
    }

    @Override
    public void put(List<T> cachedList) {
        throw new IllegalStateException("Cache has already been set");
    }

    @Override
    public List<T> get() {
        return new ArrayList<T> (cache.values()); //defensive copy
    }

    @Override
    public boolean contains(UUID uuid) {
        return cache.containsKey(uuid);
    }

    @Override
    public T getById(UUID uuid){
        return cache.get(uuid);
    }
}

If you can't use a constructor and need the put method to work as in your original design, then the only change I would make is to use a map, but you would still need to synchronize the methods and use the set flag.


Reading environment variables of various types I need to extract some data from the environment variables using Go. This data can be string, boolean or integer, so I ended up writing three functions.

package main
import (
    "fmt"
    "os"
    "strconv"
)

func getStrEnv(key string) string {
    val := os.Getenv(key)
    if val == "" {
        panic(fmt.Sprintf("some error msg"))
    }
    return val
}

func getIntEnv(key string) int {
    val := getStrEnv(key)
    ret, err := strconv.Atoi(val)
    if err != nil {
        panic(fmt.Sprintf("some error"))
    }
    return ret
}

func getBoolEnv(key string) bool {
    val := getStrEnv(key)
    ret, err := strconv.ParseBool(val)
    if err != nil {
        panic(fmt.Sprintf("some error"))
    }
    return ret
}

Which works, but in a language like Python, I would just create one function getEnv(key, type_var), which would go through various passes depending on the type_var provided. Is there a way to produce similar result with Go?

Go is statically typed. Every variable has a static type, that is, exactly one type known and fixed at compile time. You are not in dynamically typed Python anymore.

You could use reflection, BUT DON'T!

  Common difficulties with
  reflection

  If people are new to Go, they shouldn't be using reflection at all.

  -rob

Your functions are specializations of os.Getenv. Revise the function names to getenvStr, getenvInt, and getenvBool.

  os.Getenv retrieves the value of the environment variable named
  by the key. It returns the value, which will be empty if the variable
  is not present.

There is often nothing wrong when an environment variable is empty. Instead of panicing, return a named error.

For example,

package main

import (
    "errors"
    "os"
    "strconv"
)

var ErrEnvVarEmpty = errors.New("getenv: environment variable empty")

func getenvStr(key string) (string, error) {
    v := os.Getenv(key)
    if v == "" {
        return v, ErrEnvVarEmpty
    }
    return v, nil
}

func getenvInt(key string) (int, error) {
    s, err := getenvStr(key)
    if err != nil {
        return 0, err
    }
    v, err := strconv.Atoi(s)
    if err != nil {
        return 0, err
    }
    return v, nil
}

func getenvBool(key string) (bool, error) {
    s, err := getenvStr(key)
    if err != nil {
        return false, err
    }
    v, err := strconv.ParseBool(s)
    if err != nil {
        return false, err
    }
    return v, nil
}

func main() {}

References:

Package reflect

The Laws of Reflection


Improving performance hacker rank Median I'm trying to compete on HackerRank and my answer got accepted, but the times are not so good. I have a friend who sent the answer in C# too but somehow made it a lot faster. I'm wondering what can I do to improve it.

using System;
using System.Collections.Generic;
using System.IO;
using System.Text;
class Solution {

   static void Main(String[] args) {
      int N;
      StringBuilder st= new StringBuilder();

      N = Convert.ToInt32(Console.ReadLine());
      int[] x = new int[N];
       List<double> a= new List<double>();
      string[] s = new string[N];

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

         string tmp = Console.ReadLine();
         string[] split = tmp.Split(new Char[] {' ', '\t', '\n'});

         s[i] = split[0].Trim();
         x[i] = Convert.ToInt32(split[1].Trim());
          bool r=true;

          if(s[i]=="r"){
              int index= a.BinarySearch(x[i]);
              if(index>=0){
                  a.RemoveAt(a.LastIndexOf(x[i]));
              }
              else{
              r=false;
              }

          }else{
              var index = a.BinarySearch(x[i]);
                if (index < 0) index = ~index;
                  a.Insert(index,x[i]);
          }
          if(!r || a.Count==0){
              st.AppendLine("Wrong!");

          }
          else{
          st.AppendLine(calcularModa(a).ToString());
          }
      }
       Console.WriteLine(st.ToString());

   }
   static double calcularModa(List<double> a){

   int i= a.Count/2;
       if(a.Count % 2 !=0){
           return a[i];
       }else{
           return ((a[i - 1] + a[i]))/2;
       }

    }
}

There are a lot of stylistic changes I'd normally suggest, but I'm going to forgo that to mention three very simple optimizations:

remove new Char[] { ' ', '\t', '\n' } from within the loop. No need to recreate this object every time as it's an invariant.
you know the maximum size of the a list, so create it with that capacity in mind to prevent resizing as it's being accessed: List<double> a= new List<double>(N);
Repeated code in both the if and else branches pulled out (no performance optimization, but basic tenet of DRY (Don't Repeat Yourself)): int index= a.BinarySearch(x[i]);

Resulting code looks like this:

using System;
using System.Collections.Generic;
using System.IO;
using System.Text;
class Solution {

   static void Main(String[] args) {
      int N;
      StringBuilder st= new StringBuilder();

      N = Convert.ToInt32(Console.ReadLine());
      int[] x = new int[N];
       List<double> a= new List<double>(N);
      string[] s = new string[N];

      Char[] splitChars = new Char[] { ' ', '\t', '\n' };

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

         string tmp = Console.ReadLine();
         string[] split = tmp.Split(splitChars);

         s[i] = split[0].Trim();
         x[i] = Convert.ToInt32(split[1].Trim());
          bool r=true;

          int index= a.BinarySearch(x[i]);
          if(s[i]=="r"){
              if(index>=0){
                  a.RemoveAt(a.LastIndexOf(x[i]));
              }
              else{
              r=false;
              }

          }else{
                if (index < 0) index = ~index;
                  a.Insert(index,x[i]);
          }
          if(!r || a.Count==0){
              st.AppendLine("Wrong!");

          }
          else{
          st.AppendLine(calcularModa(a).ToString());
          }
      }
       Console.WriteLine(st.ToString());

   }
   static double calcularModa(List<double> a){

   int i= a.Count/2;
       if(a.Count % 2 !=0){
           return a[i];
       }else{
           return ((a[i - 1] + a[i]))/2;
       }

    }
}


Deviation and Variance This assignment requires obtaining the deviation and variance of \$n\$ numbers stored in one single array. Is there any way to make this code more efficient?

#include <iostream>
#include <cmath>

using namespace std;
int main()

{
    const int arrSize = 14;
    double average , sum = 0 , dev = 0;
    double deviation[arrSize];

    int grades[arrSize] = { 89, 95, 72, 83, 99, 54, 86, 75, 92, 73, 79, 75, 82, 73 };

    // Calculating the average
    for ( int i = 0; i < arrSize; i++ )
    {
        sum += grades[i];
        average = ( sum / arrSize );
    }
    cout << "average is : " << average << endl;
    cout << endl;

    //Calculating the deviation and variance

    double sumVar = 0 , totalVar = 0;
    for ( int i = 0; i < arrSize; i++ )
    {
        deviation[i] = ( grades[i] - average );

        cout << "Grades = " << grades[i] << " and deviation is = " << deviation[i] << endl;

        double variance = pow( deviation[i] , 2 );
        sumVar = sumVar + variance;
        totalVar = ( sumVar / arrSize );

    }
    cout << endl;
    cout << "variance = " << totalVar << endl;

    system( "pause" );
    return 0;
}

efficiency
Yes, this can be improved.
One obvious problem is that you're re-computing your average and your standard deviation on every iteration of a loop, even though only the last result (after the last iteration) is ever actually used.
For example:
for (int i = 0; i < arrSize; i++)
{
    sum += grades[i];
    average = (sum / arrSize);
}

You're computing average on every iteration, but only need or use the last value you compute. You can compute it once with code like:
for (int i = 0; i < arrSize; i++)
{
    sum += grades[i];
}
average = (sum / arrSize);

Your computation of the standard deviation is much the same way.
Use of pow
I'd avoid using pow to compute a square. It often imposes quite a bit of overhead, so pow(deviation[i], 2) will often be substantially slower than deviation[i]*deviation[i].
Formatting
Looking at the code more generally, you really need to fix your indentation.
std::endl
I would advise against using std::end. Normally, you just want '\n', which also gives you a new line, but will nearly always be (much) faster. In the case above, it won't make much difference, but if you're writing a lot of data to a file (for example) the difference can get very large, very quickly (e.g., a slowdown of 8:1 or 10:1 is fairly typical).


Toggling an array of filters What follows is a piece of code that essentially toggles an array of filters (if the filter doesnt exist it adds it, if it does, it removes it).

What would you suggest is the best way to write the following imperative approach declaratively?

var selectedFilters = [ {name:"SomeName"} , ... ]
var inputFilter = {name:"OtherName"};

var indexFound = -1;
for (let i = 0; i < selectedFilters.length; i++) {
    if (selectedFilters[i].name === inputFilter.name) {
        indexFound = i;
    }
}

if (indexFound != -1) {
    selectedFilters.splice(indexFound, 1);
} else {
    selectedFilters.push(inputFilter);
}

An idea would be to use filter first to weed out the item if it exists by name, then if the resulting array is equal to the original, push. But it still doesnt feel right.

Pure V State

There are two ways you can do this.

Pure

The first functional pure method first copies the array, then checks if the item to toggle exists then depending on that result adds or removes the item. Making sure that the added item is a copy, not a reference. It has no side effects but requires additional memory and CPU cycles.

const toggleItem = (itemDesc, items, prop = "name") => {
    items = [...items];
    const index = items.findIndex(item => itemDesc[prop] === item[prop]);
    index > -1 ? items.splice(index, 1) : items.push({...itemDesc});
    return items;
}

State

The second does not create a new array and keeps all references It is "functionally" impure and ensures that the changed state is available to all references to the original. It is considerably quicker and uses less memory.

const toggleItem = (itemDesc, items, prop = "name") => {
    const index = items.findIndex(item => itemDesc[prop] === item[prop]);
    index > -1 ? items.splice(index, 1) : items.push(itemDesc);
    return items;
}


Maximum consecutive subarray sum for a variable sliding window size The Problem:

Given an array of integers L find the largest sum of a consecutive subarray of size k or less.
Constraints:

2 <= len(L) <= 10000

3 <= k <= len(L)

each element in the array will have an absolute value no more than 200

there will be at least one positive integer in the array L

Samples:
L=[-200, 91, 82, 43], k=3, the result should be 216
L=[41, 90, -62, -16, 25, -61, -33, -32, -33], k=5, the result should be 131

The Implementation:
Initially, I started with brute-forcing the problem, probing all sizes of the sliding window starting from k to 1. This immediately got me into "time limit exceeded" situation.
The idea implemented below is based on picking out only positive integers initially. For every positive integer, we are looking if the following integers in a window are contributing to the current sum or not, cutting off situations when current sum drops below zero:
def maximum_consecutive_subarray(L, k):
    global_max = 0

    for index in range(len(L) - 1, -1, -1):
        if L[index] < 0:  # skipping all negative values
            continue

        sliding_index = index
        positive_count = positive_sum = 0

        while sliding_index >= 0 and positive_count < k:
            if L[sliding_index] >= 0:
                positive_count += 1
                positive_sum += L[sliding_index]

                global_max = max(global_max, positive_sum)
            else:
                negative_count = 1
                negative_sum = L[sliding_index]

                while sliding_index - 1 >= 0 > L[sliding_index - 1]:  # iterating over all consecutive negative values
                    negative_count += 1
                    negative_sum += L[sliding_index - 1]
                    sliding_index -= 1

                    if positive_count + negative_count == k:  # break if sliding window size limit reached
                        break

                if positive_sum + negative_sum > 0:  # if we still contribute to the maximum value
                    positive_count += negative_count
                    positive_sum += negative_sum
                else:
                    break  # exit this window if nothing was to contribute

            sliding_index -= 1

    return global_max

I would like to know if we can further improve the solution time complexity wise and would appreciate any other feedback.

While I'm not one who really promotes pasting solutions, a lot of the answers seem to be answering in a way that a C programmer would answer the question. This is more a problem that you would want to solve in a pipeline in python due to that being how python does things fast and keeps a majority of the operations in C. Look into David Beasley's work if you want to minimize the amount of code you have to write by making everything into pipelines and checkout the collections library to get to know the tools that make things like windowed operations easy and a lot faster in python.

from collections import deque
from itertools import chain

def window(iterable,size):
    ''' yields wondows of a given size '''
    d = deque(maxlen=size)
    # normalize iterable into a generator
    iterable = (i for i in iterable)
    # fill d until it only needs one more
    for i in iterable:
        d.append(i)
        if len(d) == size-1:
            break
    # yield the windows
    for i in iterable:
        d.append(i)
        yield d

def max_sequential_subarray_sum(iterable, max_size):
    ''' returns the max sum of sequential slices of an iterable '''
    g = chain(*(window(iterable,i) for i in range(2,max_size+1)))
    return max(sum(i) for i in g if all(
        a<b for a,b in window(i,2)
    ) or all(
        a>b for a,b in window(i,2)
    ))

L=[-200, 91, 82, 43]
k=3
print(max_sequential_subarray_sum(L,k))

L=[41, 90, -62, -16, 25, -61, -33, -32, -33]
k=5
print(max_sequential_subarray_sum(L,k))


Initializing a variable with a function reference (PHP) I'm working on an old PHP website, and NetBeans is complaining about an uninitialized variable. I'm seeing some code like this:

$app->soapAPIVersion($apiVersion);
$_SESSION['apiVersion'] = $apiVersion;

The function looks something like this:

function soapAPIVersion(&$apiVersion)
{
    $apiVersion = '';
    $result = false;
    $soapResult = $client->call('getAPIVersion', array('sessionKey' => $sessionKey), $url);

    if (is_string($soapResult))
    {
        $apiVersion = $soapResult;
        $result = true;
    }

    return $result;
}

I believe it's using this line to initialize the $apiVersion variable:

$app->soapAPIVersion($apiVersion);

For better coding practice, should that really be this:

$apiVersion = '';
$app->soapAPIVersion($apiVersion);

Or is it a valid trick?

Clean Code, Chapter 3. Functions has some guidance:

  Have No Side Effects

  Side effects are lies. Your function promises to do one thing, but it also does other hidden
  things.

  [...]

  Anything that forces you to check the function signature is equivalent to a double-take. It’s
  a cognitive break and should be avoided.

The side effect here is that the function changes the argument. A more readable approach would be returning the API version (instead of true/false) or throwing an exception in case of errors.

$app->soapAPIVersion($apiVersion);
$_SESSION['apiVersion'] = $apiVersion;

Note that the code should check the return value of soapAPIVersion and handle false somehow.
The soapAPIVersion name is unclear. What does it do with the API version? From Clean Code, Chapter 2: Meaningful Names:

  Method Names

  Methods should have verb or verb phrase names like postPayment, deletePage, or save.
  Accessors, mutators, and predicates should be named for their value and prefixed with get,
  set, and is according to the javabean standard.

I'd rename it to getApiVersion or fetchApiVersion.

Note the camelCase too which is usually easier to read. From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

  While uppercase may be more common,
  a strong argument can made in favor of capitalizing only the first
  letter: even if multiple acronyms occur back-to-back, you can still
  tell where one word starts and the next word ends.
  Which class name would you rather see, HTTPURL or HttpUrl?


SQL double nested query This query produces the results I want.

Is there a less wordy or more effective way to write this query?

Criteria:
1. Vendor in detail record must have a record in TRANSEFFECTIVE
2. Get the most recent (begin_frame) record from TRANSDETAIL
3. Tells me if the vendor has any other categories in the TRANSMODAL table.

In this case,

Since DDAN has a detail record and has record in TRANSEFFECTIVE, report that DDAN has an AIR,GROUND,and SEA modal record.

Since POKE has a detail record and has record in TRANSEFFECTIVE, report that POKE has only AIR and GROUND modal records.

select vend_code,
       vend_numb,
       description,
       freight_mode
from (
select vend_code,
       vend_numb,
       begin_frame,
       freight_mode,
       freight_count,
       description,
      row_number() over (partition by vend_code,
                                      vend_numb,
                          (case modal_class_code
                           when 'A1' then 1
                          when 'A2' then 1
                           when 'G1' then 2
                           when 'S1' then 3
                          end)
                         order by vend_code,
                                  vend_numb) as modal_class_count
from
(
select td.modal_vend_code as vend_code,
       td.modal_vend_numb as vend_numb,
       td.modal_begin_frame as begin_frame,
        td.modal_description as description,
       te.modal_vend_begin_frame as te_begin,
       te.modal_vend_close_frame as te_close,
       tm.modal_class_code       as modal_class_code,
       case tm.modal_class_code
           when 'A1' then 'AIRFREIGHT'
           when 'A2' then 'AIRFREIGHT'
           when 'G1' then 'GROUND'
           when 'S1' then 'SEABOUND'
           else 'UNKNOWN'
        end as freight_mode,
      rank() over (partition by td.modal_vend_code,
                                td.modal_vend_numb
                       order by td.modal_begin_frame desc) as freight_choice,
       count(*) over (partition by td.modal_vend_code,
                                   td.modal_vend_numb) freight_count
from  transdetail td
inner join transeffective te on td.modal_vend_code = te.modal_vend_code
                             and td.modal_vend_numb = te.modal_vend_numb
inner join transmodal  tm  on td.modal_vend_code = tm.modal_vend_code
                          and td.modal_vend_numb = tm.modal_vend_numb
 )
   where freight_choice=1
)
where modal_class_count=1
;

Well, I think I broke SQLFiddle...  Just for clarity's sake, I'd refactor the subqueries... that's the best way I know to do step-wise development and refinement.  I know my subquery names suck - they should be descriptive.  I would also strip out un-necessary column names -- or in my case, comment them out.

with qry1 as
    (
        select td.modal_vend_code as vend_code,
               td.modal_vend_numb as vend_numb,
               td.modal_begin_frame as begin_frame,
               td.modal_description as description,
               te.modal_vend_begin_frame as te_begin,
               te.modal_vend_close_frame as te_close,
               tm.modal_class_code       as modal_class_code,
               case tm.modal_class_code
                   when 'A1' then 'AIRFREIGHT'
                   when 'A2' then 'AIRFREIGHT'
                   when 'G1' then 'GROUND'
                   when 'S1' then 'SEABOUND'
                   else 'UNKNOWN'
                end as freight_mode,
               case tm.modal_class_code
                   when 'A1' then 1
                   when 'A2' then 1
                   when 'G1' then 2
                   when 'S1' then 3
                   else NULL
                end as modal_class,
              rank() over (partition by td.modal_vend_code,
                                        td.modal_vend_numb
                               order by td.modal_begin_frame desc) as freight_choice,
               count(*) over (partition by td.modal_vend_code,
                                           td.modal_vend_numb) freight_count
        from  transdetail td
        inner join transeffective te on td.modal_vend_code = te.modal_vend_code
                                     and td.modal_vend_numb = te.modal_vend_numb
        inner join transmodal  tm  on td.modal_vend_code = tm.modal_vend_code
                                  and td.modal_vend_numb = tm.modal_vend_numb
     )
    ,
    qry2 as
    (
        select vend_code,
               vend_numb,
               -- begin_frame,
               freight_mode,
               -- freight_count,
               description,
              row_number() over (partition by vend_code,
                                              vend_numb,
                                              modal_class
                                 order by vend_code,
                                          vend_numb) as modal_class_count
        from
         qry1
           where freight_choice=1
    )

    select vend_code,
           vend_numb,
           description,
           freight_mode
    from qry2
    where modal_class_count=1
    ;

Then again, seeing as when I tried to run this in SQLFiddle, it didn't return for over a minute, and when I tried to refresh, I got a 404 -- my attempt may be a problem.


Find sum of neighbors in a 2D array Write a function that takes 2 inputs:

a matrix, for example 2-dimensional array, and
indices, for example [row, col]

This function should return the sum of all the second input's neighbors (up, down, left, right, diagonals).

Below is my solution. Just wanted to know if there is a better way to solve this problem. And also wanted to know if I am covering all the edge cases.

function findingNeibors(myArray, i,j) {

let rowLimit = myArray.length-1;
let columnLimit= myArray[0].length-1;
let sum = 0;

if(i<0 || j< 0) {
    console.log("invalid Index")
    return
  };

if(i>rowLimit || j> columnLimit){
    console.log("You are Out Of Bound");
    return;
}

  for(let x = Math.max(0,i-1); x<=Math.min(i+1,rowLimit); x++){
  for(let y = Math.max(0,j-1); y<=Math.min(j+1,columnLimit); y++){
    if(x!==i || y!==j){
        console.log(myArray[x][y]);
        sum+=myArray[x][y];
    }
    }
  }
  return sum;
}

let input = [
  [0,1,2,3,4],
  [1,2,3,4,5],
  [2,3,4,5,6]
]

findingNeibors(input,0,0);
findingNeibors(input,2,2,0);
findingNeibors(input,-1,3);
findingNeibors(input,3,4);

```

Don't return if there is an error, throw the error.

if(i<0 || j< 0) {
    throw new Error("Invalid Index");
}

if(i>rowLimit || j> columnLimit){
    throw new Error("Index out of bounds");
}

Don't assume an array has elements

(i.e.: let columnLimit= myArray[0].length-1; will throw a TypeError if there are no elements in the list)

Play it safe.

//get first element in list,
//if it doesn't exist set it to a default empty array
const [column = []] = myArray;
const columnLimit = column.length;

const before let and let before var

When declaring variables, always define them with const by default. You only use let when you expect that variable in question is likely to mutate. And never use var unless you have a good reason to.

Note: const arr = []; arr.push(1) mutates the array but not the variable

  am I covering all the edge cases ?

Yes, you are.

Assuming that the two dimensional array has columns of equal length

  Just wanted to know if there is a better way to solve this problem

There are only 8 elements max possible around a single element, and yet for every element you check to see if (i,j) != (x,y).

To solve this "problem", simply add all values (even with (i,j))

Then before returning subtract the element at (i,j)

for(let x = Math.max(0,i-1); x<=Math.min(i+1,rowLimit); x++){
  for(let y = Math.max(0,j-1); y<=Math.min(j+1,columnLimit); y++){
        sum+=myArray[x][y];
   }
 }

return sum - myArray[i][j]

Is this better? Well, it's 9 additional conditions VS an extra addition and subtraction.