Creating random maze in Go /*
Create a random maze
*/

package main

import (
    "fmt"
    "math/rand"
    "time"
)

const (
    mazewidth   = 15
    mazeheight  = 15
)

type room struct {
    x, y int
}

func (r room) String() string {
    return fmt.Sprintf("(%d,%d)", r.x, r.y)
}

func (r room) id() int {
    return (r.y * mazewidth) + r.x
}

// whetwher walls are  open or not.
// There are (num_rooms * 2) walls. Some are on borders, but nevermind them ;)
type wallregister [mazewidth * mazeheight * 2]bool

var wr = wallregister{}

// rooms are visited or not
type roomregister [mazewidth * mazeheight]bool

var rr = roomregister{}

func main() {
    rand.Seed(time.Now().Unix())

    stack := make([]room, 0, mazewidth*mazeheight)
    start := room{0, 0}

    // mark start position visited
    rr[start.id()] = true

    // put start position on stack
    stack = append(stack, room{0, 0})

    for len(stack) > 0 {
        // current node is in top of the stack
        current := stack[len(stack)-1]

        // Slice of neighbors we can move
        availneighbrs := current.nonvisitedneighbors()

        // cannot move. Remove this room from stack and continue
        if len(availneighbrs) < 1 {
            stack = stack[:len(stack)-1]
            continue
        }

        // pick a random room to move.
        next := availneighbrs[rand.Intn(len(availneighbrs))]

        // mark next visited
        rr[next.id()] = true

        // open wall between current and next:
        first, second := orderrooms(current, next)

        // second is either at the right or bottom of first.
        if second.x == first.x+1 {
            wr[first.id()*2] = true
        } else if second.y == first.y+1 {
            wr[first.id()*2+1] = true
        } else { // probably impossible or maybe not...
            panic("Wot?!?")
        }
        // push next to stack
        stack = append(stack, next)
    }

    // print maze

    // print upper border
    for x := 0; x < mazewidth; x++ {
        if x == 0 {
            fmt.Printf("   ")
        } else {
            fmt.Printf("_ ")
        }
    }
    fmt.Println()

    for y := 0; y < mazeheight; y++ {
        fmt.Printf("|") // left border
        for x := 0; x < mazewidth; x++ {

            id := room{x, y}.id()
            right := "|"
            bottom := "_"
            if wr[id*2] {
                right = " "
            }
            if wr[id*2+1] {
                bottom = " "
            }
            if x == mazewidth-1 && y == mazeheight-1 {
                right = " "
            }
            fmt.Printf("%s%s", bottom, right)
        }
        fmt.Println()
    }
}

// return slice of neighbor rooms
func (r room) neighbors() []room {
    rslice := make([]room, 0, 4)
    if r.x < mazewidth-1 {
        rslice = append(rslice, room{r.x + 1, r.y})
    }
    if r.x > 0 {
        rslice = append(rslice, room{r.x - 1, r.y})
    }
    if r.y < mazeheight-1 {
        rslice = append(rslice, room{r.x, r.y + 1})
    }
    if r.y > 0 {
        rslice = append(rslice, room{r.x, r.y - 1})
    }
    return rslice
}

// return rooms that are not visited yet
func (r room) nonvisitedneighbors() []room {
    rslice := make([]room, 0, 4)
    for _, r := range r.neighbors() {
        if rr[r.id()] == false {
            rslice = append(rslice, r)
        }
    }
    return rslice
}

// order to rooms by closeness to origin (upperleft)
func orderrooms(room1, room2 room) (room, room) {
    dist1 := room1.x*room1.x + room1.y*room1.y
    dist2 := room2.x*room2.x + room2.y*room2.y
    if dist1 < dist2 {
        return room1, room2
    }
    return room2, room1
}

http://play.golang.org/p/8W_FbBfUjb (You can run it here. But since time.Now() is fixed there, you will always get same maze.)

In any aspect of it, how does it look?

General notes:

the code would benefit from splitting it into more methods,
go uses camelCaseConvention, not onlylowerletters.

Now, some particulars.

stack = append(stack, room{0, 0})

You can use start variable here.

func (r room) neighbors() []room {

In this function, it'd be more idiomatic to use switch { ... } instead of sequence of ifs. Same for the if-elses in the main().

Also it may be more efficient to have a single global re-used [4]room array and have neighbors/nonvisitedneighbors returns slices on it instead of make-ing a new one each call.


Steganography with AES encryption I created a small program that hides arbitrary files in png images using steganography - it encodes the files in the two least significant bits of every channel, so using a 4-channel png we can encode one byte per pixel.

I also wanted to hide the presence of secret data by encrypting it, and, in effect, making it indistinguishable from random noise to make it impossible to prove there is any data at all hidden in the image without a valid key.

I would like to know if the method I'm using is safe and robust, and if there is anything I should have done better. I'm using a python library called cryptography to do the encryption. It would be also nice to know what attacks (if any) my code could be vulnerable to.

The whole code of my program can be found here.

Here are the functions that do the encryption and decryption. The idea is that the password string you pass to the program is turned into a large key using a key derivation function.

def encrypt(data, password, padding=0):
    """Encrypts data using the password.
    Encrypts the data using the provided password using the cryptography module.
    The password is converted into a base64-encoded key which is then used in a
    symmetric encryption algorithm.
    """

    if padding < 0:
        print "Image too small to encode the file. \
You can store 1 byte per pixel."
        exit()

    password = bytes(password)

    #Use key stretching to generate a secure key
    kdf = PBKDF2HMAC(
        algorithm=hashes.SHA256(),
        length=32,
        salt=bytes(password),
        iterations=100000,
        backend=default_backend())

    key = kdf.derive(bytes(password))

    nonce = os.urandom(16)

    cipher = Cipher(algorithms.AES(key),\
                    modes.CTR(nonce), backend=default_backend())
    enc = cipher.encryptor()
    ct = enc.update(data) + enc.finalize()

    #Add padding if needed
    ct += os.urandom(padding-16)

    #add nonce to data to allow decryption later (nonce does not need to be kept
    #secret and is indistinguishable from random noise)
return bytes(nonce) + ct

def decrypt(data, password):
    """Decrypts data using the password.
    Decrypts the data using the provided password using the cryptography module.
    If the pasword or data is incorrect this will return None.
    """

    password = bytes(password)

    #Salt is equal to password as we want the encryption to be reversible only
    #using the password itself
    kdf = PBKDF2HMAC(algorithm=hashes.AES(),
                     length=32,
                     salt=bytes(password),
                     iterations=100000,
                     backend=default_backend())

    key = base64.urlsafe_b64encode(kdf.derive(password))
    f = Fernet(key)
    token = f.decrypt(data)
return token

I expect that those two functions are where vulnerabilities and/or otherwise faulty code could be. If anyone feels like this is not enough to determine the quality of the code, feel free to read the rest of the program (at the link at the top) and review it as well.

Cheers

For cases like this:

if padding < 0:
        print "Image too small to encode the file. \
You can store 1 byte per pixel."
        exit()

It's probably better to define a custom exception and raise that:

class SizeError(Exception):
    pass

...

if padding < 0 :
    raise SizeError("Image too small to encode the file. \
    You can store 1 byte per pixel.")

I'm no security expert, but is it ok to use the string you want to encode as salt in kdf? This looks like security through obscurity (because it means you just double the input before encrypting).


Singly linked list implementation in Scala What could be a better implementation, following the Scala way of doing it?

class Node(itemValue: Object, nextItem:Node){
  val value = itemValue
  val next = nextItem
}

class MyList() {
  var start: Node =null

  def add(value:Object){
    val cache = start
    this.start = new Node(value,cache)
  }

  def print:Unit  = {
    var n = start;
    println(n.value)
    while(n != null){
      println(n.value)
      n = n.next;
    }
  }

}

A list is just a wrapper for an element that also contains a pointer to another list, thus its definition is straightforward:

scala> abstract class MyList[+A]
defined class MyList

scala> case class MyCons[A](head: A, tail: MyList[A]) extends MyList[A]
defined class MyCons

scala> case object MyNil extends MyList[Nothing]
defined object MyNil

scala> val xs = MyCons(1, MyCons(2, MyCons(3, MyNil)))
xs: MyList[Int] = MyCons(1,MyCons(2,MyCons(3,MyNil)))

scala> xs.head
res2: Int = 1

scala> xs.tail
res3: MyList[Int] = MyCons(2,MyCons(3,MyNil))

This list is completely immutable. Because of the polymorphic type A it can take any type of element and because it is covariant +A, it can also take the subtypes of an element:

scala> trait T
defined trait T

scala> class X extends T
defined class X

scala> val xs: MyList[T] = MyCons(new X, MyNil)
xs: MyList[T] = MyCons(X@56a13ea6,MyNil)

The covariant nature of type A also allows the definition of MyNil, which would not be possible otherwise because it is parameterized with type Nothing, Scalas bottom type.

This single linked list definition is also the actual implementation of type List in the stdlib of Scala, although this implementation contains some more optimization details and MyCons is called ::.

If you also want to access the members head and tail when you only have type MyList instead of the more concrete type MyCons then you have to declare them directly in the supertype:

abstract class MyList[+A] {
  def head: A
  def tail: MyList[A]
}

case class MyCons[A](head: A, tail: MyList[A]) extends MyList[A]

case object MyNil extends MyList[Nothing] {
  def head = throw new NoSuchElementException("MyNil.head")
  def tail = throw new NoSuchElementException("MyNil.tail")
}


Find all numbers that are factors of one array and factor into the second array, then print their count I'm working on a solution to the Following problem on Hackerrank, and I seem to have found a function that works for the purpose.
However, I feel as though this is a very overdesigned solution to a problem that appears, at first glance, to be simple. This is what I've come up with so far:
This code takes in two arrays, finds every pair of numbers from 1 to 100, multiplies them together, and tests if the product is a factor of all elements in the first array. If so, it appends the product to the fact array. Then, it tests every element in the fact array and checks if the element evenly divides every element in the second array. If so, it loads that element into a new list. Finally, it returns the length of the new list
I've run the code through a few static analyzers but they became unhelpful after a point. I'm looking for ways to cut this code down and reduce its complexity and depth. Any comments or criticisms are welcome!
Apologies if I've made any mistakes in my question-asking etiquette here, feel free to let me know if this needs more context and thanks in advance!
def get_total_x(input_factors: int, test_numbers: int) -> int:
    fact = list()
    output = list()
    for mult1 in range(100):
        for mult2 in range(100):
            # find all multiples of mult1 and mult2 that can be factored
            # into all numbers in a, while ignoring zero.
            if all(v == 0 for v in [(mult1 * mult2) % factor for factor in input_factors]) and (mult1 * mult2 != 0):
                fact.append(mult1 * mult2)
    seen = set()
    seen_add = seen.add
    # remove duplicates, may be able to cut out?
    fact = [x for x in fact if not (x in seen or seen_add(x))]
    for test in fact:
        # check for all numbers from the previous loop that divide b cleanly.
        if all(w == 0 for w in [factor2 % test for factor2 in test_numbers]):
            output.append(test)

    return len(output)

if __name__ == '__main__':

    arr = (2, 4)

    brr = (16, 32, 96)

    total = get_total_x(arr, brr)

    print(total)

Redundant calculations
You calculate mult1 * mult2 three times. Instead, do this once, assign it to a variable, and use that.

Since you want to never want to calculate when a value is 0, you can set the start of range to 1.
range(1, 101) # 1 -> 100, since (inclusive, exclusive)

Now you can remove that (product != 0) check.
Type hints
At first, it looks like the function accepts two integers. In reality, it accepts two tuples of integers. Consider doing the following:
from typing import Tuple

def get_total_x(input_factors: Tuple[int], test_numbers: Tuple[int]) -> int:

This makes it clear that you want tuples of integers as parameters, instead of a single integer.
Using sets
You use a set, but you don't actually use the set. You're trying to add values to the set that aren't already there, or that you can add to the set. Instead of having that list comprehension, simply cast fact into a set. Does the exact same thing.
fact = set(fact)

Magic numbers
What is the significance of 100? I see in your question you explain, but not in the code. I would assign this to a variable to make it more clear.


Printing integers with spaces between the digits Problem:  Given an integer between 1 and 32767 print the individual digits with 2 spaces between them.

I am using C How To Program but this is not homework.  The book has not gotten to arrays or anything more complicated than looping structures and function. I try to avoid using those structures and concepts that have not been introduced yet.  I get the digits to separate and output.  I was wondering if there is a better way to do this using the tools I have.

My Code:

//-----------------------------------------------------------------------------
// Program: EX5_22.c - Separating Digits
// Programmer:  Joseph Cunningham
// Class: CsC_20 - c
// Date: 2/16/19
//
// This program will prompt the user for a number from 1 - 32767.  It will then
// output that number separated into its constituent digits with 2 spaces
// between each digit
//-----------------------------------------------------------------------------

#include<stdio.h>
#include<math.h>

int getNumLength(int number);
void separate(int number, int power); // Fucntion prototype

int main (void)
{
  // Variable declaration

  int number; // User entered number to digitize
  int power;  // Power to generate divisor

  // Prompt user for number

  printf("Please input a number (1-32767): ");
  scanf("%d", &number);

  // Get the power for the divisor

  power = getNumLength(number);

  // Output digits

  separate(number, power);

  return 0;
}

//-----------------------------------------------------------------------------
// Function: getPower(number) - counts the number of digits in the number to
// generate a power
// Input:  int number - the user entered number
// Output: none
// Return value - length of thr number
//-----------------------------------------------------------------------------

int getNumLength(int number)
{
  int length; // the length of number

  while (number != 0)
  {
    number = number / 10;
    length++;
  }

  return length;
}

//-----------------------------------------------------------------------------
// Function: separate(number) - separates and displays an integer between 1
// -32767 into its digits
// Input: int number - intger betwenn 1 - 32767, int power - power for divisor
// Output: numbers digits separated by 2 spaces
// Return value: none
//-----------------------------------------------------------------------------

void separate(int number, int power)
{
  int divisor; // the divisor from power
  int i;  // loop counter

  for(i = power - 1; i >=0; i--)
  {
    divisor = (int)pow(10, i);
    printf("%d  ", number / divisor);
    number = number % divisor;
  }
}

// Variable declaration

  int number; // User entered number to digitize
  int power;  // Power to generate divisor

So // is a C++ comment that is only available in newer versions of C.

Declaring variables at the beginning of the block is an old C standard that you don't need in newer versions of C.  The modern standard (in C and most other languages) is to put variable declarations as close to first use as possible.  For number, you're pretty much there.  For power, it could be declared with the  initialization.

    /* Power to generate divisor */
    int power = getNumLength(number);

I put the comment on a separate line so as to be more readable and to avoid side scroll (not so much here as in other examples).

I would have probably have written that

    int digit_count = count_digits(number);

That's clearer in my opinion and does not require an explanatory comment.  We're counting the number of digits and storing the result in a variable.  We'll know what the variable is later, because we call it digit_count.

I prefer snake_case, particularly in C.  It relies less on the reader (who may not be a native English speaker) being able to recognize capital letters.  That said, camelCase is quite common.  Which to use is up to you so long as you do so consistently.

I also changed to a four column indent.  That is more common than two column in code.  The only place that I'd recommend two column is in markup languages like HTML and XML.  They don't have methods, so their indent increases with their complexity.  With code, if you are indenting so much that you are running out of space on the screen, that may be a sign that you should push code into separate functions or methods.

An alternate approach

  int divisor; // the divisor from power
  int i;  // loop counter

  for(i = power - 1; i >=0; i--)
  {
    divisor = (int)pow(10, i);
    printf("%d  ", number / divisor);
    number = number % divisor;
  }

You are using the rather expensive pow function in each iteration of your for loop.

Consider

    for (int divisor = buildInitialDivisor(number); divisor > 0; divisor /= BASE) {
        printf("%01d  ", number / divisor);
        number %= divisor;
    }

This declares the loop variable as part of the loop declaration.

This uses a constant BASE instead of the magic number 10.  This improves readability and makes the program easier to modify.

I changed from %d to %01d.  Now if you wanted to display, say, two digits at a time, you could change that to %02d and change BASE to 100.  It should zero pad the number so as to always print two digits.  Of course, it would print a single leading digit as two digits too.  That might be undesirable.  You might consider how you could fix that.

The %= is just a shorter syntax.  Your original line and the revised one will do the exact same thing.  Assignment operators in C.

This gets rid of pow and replaces it with a different function and then divides on each iteration.

This gets rid of your i variable which only tracked the number of iterations.  We can do that directly.

I prefer to always put the curly brackets on the same line as the code structure.  Either form is fine so long as you are consistent.  But you'll see both as you go, so might as well start recognizing them now.

This also requires

const int BASE = 10;

and

int buildInitialDivisor(int number) {
    int divisor = 1;
    while (number / BASE >= divisor) {
        divisor *= BASE;
    }

    return divisor;
}

But it gets rid of your getNumLength function.

Recursion

You may not have gotten there yet, but when you get to recursion, you might try this problem again.  Recursion often helps when you need to reverse the natural order of output.  This is because you can display while returning from the recursion.  So rather than building the largest divisor first, you can incrementally increase the size of the divisor (or better yet, decrease the number, always using the same divisor).

You also might try this again after learning sprintf and character arrays.


Is there a simpler way to make an English-readable list from strings in a tuple? I looking over some old code, I had written the function below with a C-style for-loop iterating over a count variable. I then fixed it up with new knowledge, but still am suspicious that there is even a better way to do this.

The function should behave like so:

>>> join_list(['a'])
'a'
>>> join_list(['a', 'b'])
'a or b'
>>> join_list(['a', 'b', 'c'])
'a, b, or c'

The function I have currently:

def join_list(my_list):
    length = len(my_list)
    if length == 1:
        return my_list[0]
    elif length == 2:
        return ' or '.join(my_list)
    else:
        head = ', '.join(my_list[:-1])
        return head + ', or ' + my_list[-1]

Looks pretty good. The only problem I can see is that you're not doing a check to make sure the list is not empty: if it is, then you'll try to access my_list[-1] in the final else clause, which will raise an IndexError.

Hence, I'd simply add a check up front for length == 0 and return '' in that case.


String concatenation of name parts I have written the following function to concatenate the parts of a name to produce the full name:

/**
 * Returns the full name of the Person.
 * @param boolean $includeTitle Whether to include the Person's title.
 * @param boolean $includeMiddleNames Whether to include the Person's middle names.
 * @param string $separator Separator character.
 * @return string Full name of the Person.
 */
public function getFullName($includeTitle = false, $includeMiddleNames = false, $separator = ' ') {
  if ($includeTitle && $this->getTitle()) {
    $name = $this->title;
  } else {
    $name = '';
  }
  if ($this->forename) {
    if ($name) {
      $name .= $separator;
    }
    $name .= $this->forename;
  }
  if ($includeMiddleNames && $this->middlenames) {
    if ($name) {
      $name .= $separator;
    }
    if ($separator != ' ') {
      // multiple middle names will be separated by spaces and so will need to be converted to $separator
      $name .= str_replace(' ', $separator, $this->middlenames);
    } else {
      $name .= $this->middlenames;
    }
  }
  if ($this->surname) {
    if ($name) {
      $name .= $separator;
    }
    $name .= $this->surname;
  }
  return $name;
}

I changed it to use an array and implode the parts - which has made it less lines, but I'm not really sure if it is written better as a result:

$nameParts = array();
if ($includeTitle && $this->getTitle()) {
  $nameParts[] = $this->title;
}
if ($this->forename) {
  $nameParts[] = $this->forename;
}
if ($includeMiddleNames && $this->middlenames) {
  if ($separator != ' ') {
    $middlenames = str_replace(' ', $separator, $this->middlenames);
  } else {
    $middlenames = $this->middlenames;
  }
  $nameParts[] = $middlenames;
}
if ($this->surname) {
  $nameParts[] = $this->surname;
}
return implode($separator, $nameParts);

I'm sure there must also be a better way but I can't think what. Any suggestions?

I assume the class this method resides in represents a person's full name (and title).

Two remarks before suggesting major refactorings:

You are using a getter getTitle and direct access to $this->title: stick for one. Usually I prefer to use getters within the class as well (as usual they contain some logic beside returning). Your other variables don't have getters at all. Also, getting a value just for checking for its existence is bad practice. Introduce some existence method for this (e.g. hasTitle).
Don't store information serialized during runtime ($middlenames). As you obviously need the middle names as an array, store them accordingly.

Now looking at your class, it has two responsibilities: (1) representing a name, and (2) formating a name. Moving the name formatting into a separate class decouples representation and formating. It allows you to have different means to format at name at the same time and when you want to change the formatting you don't have to touch the name class at all. This is how such a formatter might look like (untested):

class NameFormater {
    private $seperator;
    private $includeTitle;
    private $includeMiddleNames;

    public function __construct($seperator, $includeTitle, $includeMiddleNames) {
        // argument validation here
        $this->seperator = $seperator;
        $this->includeTitle = $includeTitle;
        $this->includeMiddleName = $includeMiddleName;
    }

    public function format(Name $name) {
        $nameParts = array();

        if ($this->includeTitle && $name->hasTitle()) {
            $nameParts[] = $name->getTitle();
        }
        if ($name->hasForName()) {
            $nameParts[] = $name->getForName();
        }
        if ($this->includeMiddleNames && $name->hasMiddleNames()) {
            $nameParts = array_merge($nameParts, $name->getMiddleNames());
        }
        if ($name->hasLastName()) {
            $nameParts[] = $name->getLastName;
        }

        return implode($this->seperator, $nameParts);
    }
}

Now this method could be refined further. E.g. we first could allow $nameParts to contain null values if the respective part was not set and filter them before implodeing. This would make it more challenging to read though and I prefer a bit more verbosity over compactness. The current version is trivial to follow and understand. This is due to no complex nesting or logic and names that speak for themselves (e.g. hasTitle) - and in my opinion this is the most important point in programming :)


Mounting multiple partitions on a disk Is it possible to improve the code of this bash script? ... or should I just use another language? (If so, which one would fit the best the situation, according to you?)

Here is the code (simplified, to emphasis the problem):

#!/bin/bash
# mountDDE mount the partitions of the disk b.
# If arguments are passed, it will mount the specific partitions, a, b or c.

if [ -v "$1" ] # If no argument is passed, act on all partitions.
then a=t
    b=t
    c=t
fi

while [ -n "$1" ] ; do
    if [ $1="a" ] # We test the argument(s), to know which partition has to be mounted.
    then a=t
    elif [ $1="b"] # I hate redundunces
    then b=t
    elif [ $1="c"] # Again...
    then c=t
    fi
    shift
done

[ $a="t" ] && sudo mount /dev/sba /media/ba # I'll actually use uuids.
[ $b="t" ] && sudo mount /dev/sbb /media/bb # Not direct paths, like here.
[ $c="t" ] && sudo mount /dev/sbc /media/bc

To me, this code is redundant. Is there any more efficient/better way to do it ?
Which language should I use for this task?

Since you're using bash (I assume a recent version of bash), you can use some advanced features, like associative arrays and indirect variables

#!/bin/bash

# If no argument is passed, act on all partitions.
if (( $# == 0 )); then
    a=t b=t c=t
else
    for arg; do
        if [[ $arg == a|b|c ]]; then
            declare $arg=t
        else
            echo "ignoring argument '$arg'"
        fi
    done
fi

declare -A uuids=( [a]=1234 [b]=3456 [c]=5678 )

for dest in a b c; do
    if [[ ${!dest} == t ]]; then
        sudo mount /dev/sb$dest "${uuids[$dest]}"
    fi
done

Note that the [ command acts differently when given different numbers of arguments. With just a single argument (such as [ $a="t" ]), you'll get a success exit status if the argument is not empty (and the string "$a=t" is not empty no matter the value of $a). You have to put spaces between all the operators and operands for [ and test and [[.


Minimum Refills from A to B I am studying algorithms and I had to solve this algorithm:

Consider the following problem. You have a car such that if you fill it up to full tank, you can travel with it up to 400 kilometers without refilling it. And you need to get from point A to point B, and the distance between them is 950 kilometers. Of course, you need to refuel on your way, and luckily, there are a few gas stations on your way from A to B. These are denoted by blue circles, and the numbers above them mean the distance from A to the corresponding gas station along the way from A to B. And you need to find the minimum number of refuels to get from A to B.

I came up to a very different solution that the teacher and I would like to know your thoughts:

import java.io.*;
import java.util.*;

public class MinRefill{

 static int getMinRefill(int[] numbers, int refillAt) {
    int lastRefill=0;
    int currentRefill=0;
    int numOfRefills=0;
    int endPoint= numbers.length;

    while(currentRefill<endPoint-1){

        if(numbers[currentRefill+1]-numbers[currentRefill]>refillAt){
            return 0;
        }

        if(lastRefill+refillAt<numbers[currentRefill+1]){
            lastRefill=numbers[currentRefill];
            numOfRefills++;
        }

        currentRefill++;
    }

    return numOfRefills;
}
public static void main(String[] args) {

    Scanner in = new Scanner(System.in);

    int n = in.nextInt();
    int[] numbers = new int[n];

    for (int i = 0; i < n; i++) {
        numbers[i] = in.nextInt();
    }

    System.out.println(getMinRefill(numbers,400));

}
}

Tested input:

0
0 200 375 550 750 950

0 = A and 950 = B

Your algorithm looks fine, and efficient. Strictly speaking, since the stations are sorted, you could use binary search to find the next station to refill, by looking for the insertion point of lastRefill + 400. This could speed things up if gas stations are extremely dense. But that's unrealistic anyway, so I think no need to bother, your simple implementation is fine.

The implementation is a bit hard to read because of the names. I recommend these renames:

numbers -> stations
refillAt -> capacity
numOfRefills -> refills

The writing style is also too compact and hard to read.
It's recommended to put spaces around operators.

Lastly, the endpoint variable is redundant, stations.length is just as easy to understand.

With the renames and the formatting changed, the method becomes:

static int getMinRefill(int[] stations, int capacity) {
    int lastRefill = 0;
    int current = 0;
    int refills = 0;

    while (current < stations.length - 1) {

        if (stations[current + 1] - stations[current] > capacity) {
            return 0;
        }

        if (lastRefill + capacity < stations[current + 1]) {
            lastRefill = stations[current];
            refills++;
        }

        current++;
    }

    return refills;
}


Tick-tack-toe (Jumping into C++) The exercise is from Alex Allain's book "Jumping into C++".
From Chapter 10 Practical Problem 4:

  Write a small tic-tac-toe program that allows two players to play
  tic-tac-toe competitively. Your program should check to see if either
  player has won, or if the board is filled completely (with the game
  ending in a tie). Bonus: can you make your program detect if the game
  cannot be won by either side before the entire grid is filled?

Here's my code. My main goal was a functioning tick-tack-toe game with little concern on the user experience (e.g.: the coordinates a player enters start from 0, or (0,0) coordinate is in the upper left corner no lower left as one would intuitively think). As for coding style I wanted the code to be as DRY as possible, with small, atomic, easy to understand functions doing most of the work.

#include <iostream>

using namespace std;

bool checkWin(int board[][3], int player) {
    // horizontal
    for (int i = 0; i < 3; i++) {
        if (board[i][0] == player && board[i][1] == player && board[i][2] == player) return true;
    }

    // vertical
    for (int i = 0; i < 3; i++) {
        if (board[0][i] == player && board[1][i] == player && board[2][i] == player) return true;
    }

    // diagonal
    if ((board[0][0] == player && board[1][1] == player && board[2][2] == player) ||
        (board[0][2] == player && board[1][1] == player && board[2][0] == player))
        return true;

    return false;
}

// TODO implement function if the game is a tie before the last move

bool fillPlace(int board[][3], int x, int y, int value) {
    if (x > 2 || x < 0) {
        cout << "Invalid x coordinate. Try again.\n";
        return false;
    } else if (y > 2 || y < 0) {
        cout << "Invalid y coordinate. Try again.\n";
        return false;
    } else if (board[x][y] != 0) {
        cout << "Place is already filled. Try again.\n";
        return false;
    } else {
        board[x][y] = value;
        return true;
    }
}

void initBoard(int board[][3], int size) {
    for (int i = 0; i < size; i++) {
        for (int j = 0; j < 3; j++) {
            board[i][j] = 0;
        }
    }
}

void printBoard(int board[][3], int size) {
    cout << "-----" << endl;
    for (int i = 0; i < size; i++) {
        for (int j = 0; j < 3; j++) {
            cout << board[i][j] << " ";
        }
        cout << endl;
    }
    cout << "-----" << endl;
}

bool isBoardFilled(int board[][3]) {
    bool isFilled = true;
    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) {
            if (board[i][j] == 0) isFilled = false;
        }
    }
    return isFilled;
}

int main() {

    int board[3][3];
    initBoard(board, 3);
    printBoard(board, 3);

    int playerOnesTurn = true;
    while (true) {
        int x, y;

        if (playerOnesTurn) {
            cout << "Player 1's turn" << endl;
            do {
                cout << "Enter x coordinate: "; cin >> x;
                cout << "Enter y coordinate: "; cin >> y;
            } while (!fillPlace(board, x, y, 1));
        } else {
            cout << "Player 2's turn" << endl;
            do {
                cout << "Enter x coordinate: "; cin >> x;
                cout << "Enter y coordinate: "; cin >> y;
            } while (!fillPlace(board, x, y, 2));
        }
        playerOnesTurn = !playerOnesTurn;

        if (isBoardFilled(board)) {
            cout << "The game is a tie.\n";
            break;
        }

        printBoard(board, 3);
        if (checkWin(board, 1)) {
            cout << "Player 1 has won\n";
            break;
        } else if (checkWin(board, 2)) {
            cout << "Player 2 has won\n";
            break;
        }
    }

    return 0;
}

Avoid using namespace std

This can cause name collisions because it adds every name in the std namespace to the global namespace. For a small program like this one it's unlikely that you'll run into any problems (then again, maybe not) but it's best to get into the habit of using the std:: prefix on names in the std namespace.

Alternatively, you can introduce using declarations like using std::cout; to add specific names to the global namespace.

Avoid std::endl in favor of \n

std::endl flushes the stream, which can cause an unnecessary loss in performance. In printBoard() you use std::endl three times but you can easily replace some of those uses with \n.

Use a class for the Tic Tac Toe board

Instead of using a 3x3 array it's better to create a class to hold the state of the Tic Tac Toe board, with functions to set or get the value of a place on the board, determine whether or not the board is filled, etc. This allows you to hide implementation details which may change later on -- for example, if your compiler supports C++11 or will do so in the future you may want to change your internal representation of the board to use std::array. If you have a class for the Tic Tac Toe board the external interface would not change, only the code that implements the functionality.

Here's what that class might look like:

class Board {
    int board[3][3];
public:
    typedef int value_type;

    // More modern alternative to typedef:
    //using value_type = int;

    Board() {
        for (std::size_t i = 0; i < 3; i++) {
            for (std::size_t j = 0; j < 3; j++) {
                board[i][j] = 0;
            }
        }
    }

    // Gets the value of a board place
    int place(std::size_t x, std::size_t y) const {
        // Might want to check coordinates and throw an exception (e.g. std::out_of_range) if coordinate(s) are invalid
        return board[x][y];
    }

    // Sets the value of a board place
    // It would probably be better to throw an exception (e.g. std::out_of_range) instead of printing to std::cout
    bool place(std::size_t x, std::size_t y, int value) {
        if (x > 2 || x < 0) {
            cout << "Invalid x coordinate. Try again.\n";
            return false;
        } else if (y > 2 || y < 0) {
            cout << "Invalid y coordinate. Try again.\n";
            return false;
        } else if (board[x][y] != 0) {
            cout << "Place is already filled. Try again.\n";
            return false;
        } else {
            board[x][y] = value;
            return true;
        }
    }

    bool is_filled() const {
        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                // Return immediately once we find a non-filled place
                if (board[i][j] == 0) return false;
            }
        }

        // All places were found to be filled
        return true;
    }
};

(Note the use of std::size_t for the coordinate type and the typedef for the int value type, which mimic practices from the standard library.)

You would no longer need initBoard() since the Board constructor performs the initialization, and some of your other functions would become member functions of the class. In some cases you can improve the performance of these functions -- e.g. you can keep a count of how many places are filled by incrementing the count each time a new place is filled by Board::place(), and then Board::is_filled() simply has to check if that count equals 9 rather than checking every place in the board (the performance gain is negligible for such a tiny board but the principle is important).

Overload operator<< for printing the board

printBoard() only prints to std::cout. You can overload operator<< in order to insert the board into any std::ostream. Using the above Board class the code would look like this:

std::ostream& operator<<(std::ostream& os, Board board) {
    os << "-----\n";

    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) {
            os << board.place(i, j) << " ";
        }

        os << '\n';
    }

    os << "-----" << std::endl;

    return os;
}

(Note the replacement of some of the std::endl uses with '\n`.)

To print the board to std::cout you'd simply use std::cout << board;, and you can use the same function to print the board to a different std::ostream such as a std::fstream (for writing to a file).

Make your code DRYer

In some cases you can make your code DRYer. For example, the code in the if and else sections is very similar here:

if (playerOnesTurn) {
    cout << "Player 1's turn" << endl;
    do {
        cout << "Enter x coordinate: "; cin >> x;
        cout << "Enter y coordinate: "; cin >> y;
    } while (!fillPlace(board, x, y, 1));
 } else {
    cout << "Player 2's turn" << endl;
    do {
        cout << "Enter x coordinate: "; cin >> x;
        cout << "Enter y coordinate: "; cin >> y;
    } while (!fillPlace(board, x, y, 2));
 }

You can put that in a function like this:

// Capture Board by reference so we modify the original Board, not a copy
void player_move(Board& board, int player) {
    cout << "Player " << player << "'s turn" << endl;

    std::size_t x, y;

    do {
        cout << "Enter x coordinate: "; cin >> x;
        cout << "Enter y coordinate: "; cin >> y;
    } while (!board.place(x, y, player));
}

Here's a demo program with some of the changes I suggested:

#include <iostream>
#include <ostream>

using namespace std;

class Board {
    int board[3][3];
public:
    typedef int value_type;

    // More modern alternative to typedef:
    //using value_type = int;

    Board() {
        for (std::size_t i = 0; i < 3; i++) {
            for (std::size_t j = 0; j < 3; j++) {
                board[i][j] = 0;
            }
        }
    }

    // Gets the value of a board place
    int place(std::size_t x, std::size_t y) const {
        // Might want to check coordinates and throw an exception (e.g. std::out_of_range) if coordinate(s) are invalid
        return board[x][y];
    }

    // Sets the value of a board place
    // It would probably be better to throw an exception (e.g. std::out_of_range) instead of printing to std::cout
    bool place(std::size_t x, std::size_t y, int value) {
        if (x > 2 || x < 0) {
            cout << "Invalid x coordinate. Try again.\n";
            return false;
        } else if (y > 2 || y < 0) {
            cout << "Invalid y coordinate. Try again.\n";
            return false;
        } else if (board[x][y] != 0) {
            cout << "Place is already filled. Try again.\n";
            return false;
        } else {
            board[x][y] = value;
            return true;
        }
    }

    bool is_filled() const {
        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                // Return immediately once we find a non-filled place
                if (board[i][j] == 0) return false;
            }
        }

        // All places were found to be filled
        return true;
    }
};

bool checkWin(Board& board, int player) {
    // horizontal
    for (int i = 0; i < 3; i++) {
        if (board.place(i, 0) == player && board.place(i, 1) == player && board.place(i, 2) == player) return true;
    }

    // vertical
    for (int i = 0; i < 3; i++) {
        if (board.place(0, i) == player && board.place(1, i) == player && board.place(2, i) == player) return true;
    }

    // diagonal
    if ((board.place(0, 0) == player && board.place(1, 1) == player && board.place(2, 2) == player) ||
        (board.place(0, 2) == player && board.place(1, 1) == player && board.place(2, 0) == player))
        return true;

    return false;
}

// TODO implement function if the game is a tie before the last move

std::ostream& operator<<(std::ostream& os, Board board) {
    os << "-----\n";

    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) {
            os << board.place(i, j) << " ";
        }

        os << '\n';
    }

    os << "-----" << std::endl;

    return os;
}

// Capture Board by reference so we modify the original Board, not a copy
void player_move(Board& board, int player) {
    cout << "Player " << player << "'s turn" << endl;

    std::size_t x, y;

    do {
        cout << "Enter x coordinate: "; cin >> x;
        cout << "Enter y coordinate: "; cin >> y;
    } while (!board.place(x, y, player));
}

int main() {
    Board board;
    std::cout << board;

    // should be a bool, not an int
    // alternatively, just use an int which is 1 when it's player 1's turn and 2 when player 2's turn
    bool playerOnesTurn = true;

    while (true) {
        player_move(board, playerOnesTurn ? 1 : 2);

        playerOnesTurn = !playerOnesTurn;

        if (board.is_filled()) {
            cout << "The game is a tie.\n";
            break;
        }

        std::cout << board;

        if (checkWin(board, 1)) {
            cout << "Player 1 has won\n";
            break;
        } else if (checkWin(board, 2)) {
            cout << "Player 2 has won\n";
            break;
        }
    }

    return 0;
}

I left a few other minor tips in the comments and I didn't make all the changes I suggested (e.g. I did not remove using namespace std).


Generic DB Value Getter I'm trying to see if there is a better way to do this when I just need one column with one row. Do not mind the lack of use of a prepared statement, I will put it in.

public <T> T getValueFromDb(String col, String table, String cond, Class<T> returnValueClass) {

    T val = null;

    try {
        ResultSet rs = getSet("SELECT " + col + " FROM " + table + " WHERE " + cond);
        if(rs.next()) {
            if(returnValueClass == Integer.class) {
                val = (T) (Integer) rs.getInt(col);
            } else if(returnValueClass == Double.class) {
                val = (T) (Double) rs.getDouble(col);
            } else if(returnValueClass == String.class) {
                val = (T) rs.getString(col);
            }
        }
        closeSetQuietly(rs);
    } catch(SQLException sql) {
        Log.logException(null, sql);
    }

    return val;

}

Should I just perhaps do just:

returnValueClass.cast(rs.getInt(x));

Using polymorphism is the way to get an great solution. You are receiving the class in your method just to infer the the return type while you could abstract the idea of conversion and use it to do the inference too. Let's create an interface to convert the result and its implementations:

interface ResultTransformer<T> {
    T transform(ResultSet rs, String column) throws SQLException;
}

public class IntegerResultTransformer implements ResultTransformer<Integer> {
    public Integer transform(ResultSet rs, String column) throws SQLException {
        return rs.getInt(column);
    }
}

public class StringResultTransformer implements ResultTransformer<String> {
    public String transform(ResultSet rs, String column) throws SQLException {
        return rs.getString(column);
    }
}

public class DoubleResultTransformer implements ResultTransformer<Double> {
    public Double transform(ResultSet rs, String column) throws SQLException {
        return rs.getDouble(column);
    }
}

Now the method will receive a ResultTransformer and just call its method transform:

public <T> T getValueFromDb2(String col, String table, String cond, ResultTransformer<T> transformer) {
    // concatenate strings isn't a good idea,
    // use the PreparedStatement methods: setString, setDouble and so on
    ResultSet rs = getSet("SELECT " + col + " FROM " + table + " WHERE " + cond);
    try {
        return transformer.transform(rs, col);
    } catch (SQLException e) {
        throw new RuntimeException(e); // don't return null
    }
}

Using the method:

Integer anInteger = getValueFromDb2("col", "table", "cond", new IntegerResultTransformer());
String aString = .getValueFromDb2("col", "table", "cond", new StringResultTransformer());
Double aDouble = .getValueFromDb2("col", "table", "cond", new DoubleResultTransformer());

For now on, you just need to create a new ResultTransform implementation.


Java method to add daily interest to bank account after month is over For an assignment, I am told to write two methods: incrementDay() and incrementMonth():

  incrementDay()

  This method takes no input and returns nothing. If the previous
  month's balance was not paid in full, the credit card charges interest
  on the current balance after each day. To do this, the class will need
  two additional fields, a variable that keeps track of the interest
  charged so far and a boolean variable that is a paid-in-full flag that
  keeps track of whether the user has paid the balance in full.

  incrementDay works as follows: If the paid-in-full flag is false then
  sum the account balance and the interest charged so far, then multiply
  this sum by the interest rate divided by 365, and add this value to
  the interest charged so far.

  If the paid-in-full flag is true, then there is nothing to do.

  incrementMonth()

  This method takes no input and returns nothing. Add the interest
  charged so far to the account balance. Reset the interest charged so
  far to 0.

  If the total payment this month is greater than or equal to the
  monthly payment, set the paid-in-full flag to true, otherwise set the
  flag to false.

  If the minimum monthly payment is smaller than the monthly payment and
  the total payment this month is less than the minimum monthly payment,
  add the late payment penalty to the current balance.

  Set the total payment this month to 0.

  Set the monthly payment equal to the current balance.

I'm just asking for someone to read my code and tell me if the logic and tests work in all cases.

public void incrementDay()
  {
    if(!paidInFullFlag)
    {
      interestCharged += (this.balance + interestCharged) * (interestRate/365);
    }
  }

  /*Method adds interestCharged to balance and checks whether or not account has been paid in full for month*/
  public void incrementMonth()
  {
    this.balance += interestCharged;
    this.interestCharged = 0;
    if(this.sumPayment >= this.monthlyPayment)
    {
      this.paidInFullFlag = true;
    }
    else
    {
      this.paidInFullFlag = false;
    }

    if(minMonthly < monthlyPayment && sumPayment < minMonthly)
    {
      this.balance += latePaymentPenalty;
    }
    this.sumPayment = 0;
    this.monthlyPayment = this.balance;
  }

Test:

> CreditCardAccount c = new CreditCardAccount()
> c.setCreditLimit(100)
> c.setInterestRate(.08)
> c.setLatePaymentPenalty(3)
> c.setMinMonthlyPayment(20)
> c.charge(30)
true
> c.getBalance()
30.0
> c.payment(5) //pay 5 dollars towards balance
> c.incrementDay()
> c.incrementMonth()
> c.getBalance() //flag is initially set to true, so no late payment penalty or interest is charged, but flag is set to false for next month
25.0
> c.payment(5)
> c.charge(20)
true
> c.getBalance()
40.0
> c.incrementMonth() //late payment penalty is charged
> c.getBalance()
43.0
> c.incrementDay()
> c.getBalance()
43.0
> c.incrementMonth()
> c.getBalance() //interest AND late payment penalty is added
46.00942465753425

Conventions

Java conventions state that code is indented with 4 spaces, not 2. Java also states that the brace should be on the same line, though this is more of your choice; the important part is consistency. After formatting, your code will then look like:

public void incrementDay() {
    if (!paidInFullFlag) {
        interestCharged += (this.balance + interestCharged) * (interestRate
                / 365);
    }
}

/*
 * Method adds interestCharged to balance and checks whether or not account
 * has been paid in full for month
 */
public void incrementMonth() {
    this.balance += interestCharged;
    this.interestCharged = 0;
    if (this.sumPayment >= this.monthlyPayment) {
        this.paidInFullFlag = true;
    } else {
        this.paidInFullFlag = false;
    }

    if (minMonthly < monthlyPayment && sumPayment < minMonthly) {
        this.balance += latePaymentPenalty;
    }
    this.sumPayment = 0;
    this.monthlyPayment = this.balance;
}

Miscellaneous

Here:

    if (this.sumPayment >= this.monthlyPayment) {
        this.paidInFullFlag = true;
    } else {
        this.paidInFullFlag = false;
    }

Since the condition is a boolean already, this could be simplified to:

    this.paidInFullFlag = this.sumPayment >= this.monthlyPayment;

The comment here:

/*
 * Method adds interestCharged to balance and checks whether or not account
 * has been paid in full for month
 */

Should be a JavaDoc comment:

/**
 * Method adds interestCharged to balance and checks whether or not account
 * has been paid in full for month
 */

Very similar, but not exactly the same.


Faster way to perform function calculation in Python? I'm interested in whether there is a way to further improve a "fast" version of a function used in a homework assignment I received recently (I've already submitted the completed work).

from math import log

def func_fast(mass, density):
    return sum(map((log(mass * density)).__truediv__, range(1,10001)))

def func_slow(mass, density):
    total = 0.0
    for i in range(10000):
        masslog = log(mass * density)
        total += masslog/(i+1)

    return total

mass = 2.5
density = 12.0

The fast version times in around 2-2.5ish seconds while the slow version nets 6-7 seconds.

This continues from Jaime's post.

This can be trivially sped up to \$\mathcal{O}(1)\$ by caching sum(1/j for j in range(1, 10001)) = 9.787606036044348. Then one just does

def func_O1(mass, density):
    # 9.7876... = sum(1/j for j in range(1, 10001))
    return log(mass * density) * 9.787606036044348

This gives a speedup by a factor of 3000.

If the limit (10000) is varied, use an approximation for the harmonic series:

from math import log

def harmonic(n):
    gamma = 0.57721566490153286060651209008240243104215933593992
    return gamma + log(n) + 0.5/n - 1./(12*n**2) + 1./(120*n**4)

def func_fastest(mass, density):
    return log(mass * density) * harmonic(10000)

harmonic is inaccurate below \$n \approx 120\$ so it would make sense to cache exact values up to harmonic(120). Note that whilst this is an approximation, for \$n\$ greater than \$120\$ it's exact \$\pm 1\text{eps}\$ whereas the summation gets steadily worse.

This only speeds up the function by a factor of 500.


Solution for Parent/Child circular references: WeakReference class Sometimes in advanced OOP scenarios, a class needs to hold instances of another class which needs to hold a reference to the "parent". For example when you have a dynamic UserForm control that needs to "call back" to the parent form that created it, or when you have a ViewAdapter that talks to some UI, which in turn needs to "call back" to the adapter.

Such relationships create circular references, and if nothing is done to solve this, the objects don't get cleaned up and you're looking at what's essentially a memory leak.

With the help of Comintern I've written a class that solves this problem, and called it WeakReference - in order to make the API as simple to use as possible, I wrapped it with an IWeakReference interface:

VERSION 1.0 CLASS
BEGIN
  MultiUse = -1  'True
END
Attribute VB_Name = "IWeakReference"
Attribute VB_GlobalNameSpace = False
Attribute VB_Creatable = False
Attribute VB_PredeclaredId = False
Attribute VB_Exposed = True
Option Explicit

Public Property Get Object() As Object
End Property

Here's the WeakReference class:

VERSION 1.0 CLASS
BEGIN
  MultiUse = -1  'True
END
Attribute VB_Name = "WeakReference"
Attribute VB_GlobalNameSpace = False
Attribute VB_Creatable = False
Attribute VB_PredeclaredId = True
Attribute VB_Exposed = False
Option Explicit
Implements IWeakReference

#If Win64 Then
Private Declare PtrSafe Sub CopyMemory Lib "kernel32.dll" Alias "RtlMoveMemory" (hpvDest As Any, hpvSource As Any, ByVal cbCopy As LongPtr)
#Else
Private Declare Sub CopyMemory Lib "kernel32.dll" Alias "RtlMoveMemory" (hpvDest As Any, hpvSource As Any, ByVal cbCopy As Long)
#End If

Private Type TReference
    Address As Long
End Type

Private this As TReference

Public Function Create(ByVal instance As Object) As IWeakReference
    With New WeakReference
        .Address = ObjPtr(instance)
        Set Create = .Self
    End With
End Function

Public Property Get Self() As IWeakReference
    Set Self = Me
End Property

Public Property Get Address() As Long
    Address = this.Address
End Property

Public Property Let Address(ByVal value As Long)
    this.Address = value
End Property

Private Property Get IWeakReference_Object() As Object
    ' Bruce McKinney's code for getting an Object from the object pointer:
    Dim objT As Object
    CopyMemory objT, this.Address, 4
    Set IWeakReference_Object = objT
    CopyMemory objT, 0&, 4
End Property

Can this class be improved? Is the interface & factory method overkill?

Here's a simple example usage scenario:

Class: TheParent

Option Explicit

Private child As TheChild

Private Sub Class_Initialize()
    Set child = New TheChild
    Set child.Parent = Me
End Sub

Private Sub Class_Terminate()
    Set child = Nothing
End Sub

And the TheChild class:

Option Explicit

Private ref As IWeakReference

Public Property Get Parent() As TheParent
    Set Parent = ref.Object
End Property

Public Property Set Parent(ByVal value As TheParent)
    Set ref = WeakReference.Create(value)
End Property

Private Sub Class_Terminate()
    Stop ' expected break here when TheParent is terminated
    Set ref = Nothing
End Sub

And a little procedure to test everything:

Public Sub Test()
    Dim p As TheParent
    Set p = New TheParent
    Debug.Print ObjPtr(p)
    Set p = Nothing
End Sub

As expected, the Stop statement is hit in TheChild, and if you put a breakpoint in TheParent's Class_Terminate handler, it's also hit - whereas if you replace the IWeakReference with TheParent in TheChild, none of the two Class_Terminate handlers run.

Amazing code as always, Mathieu and Comintern; many thanks.

I'm here only two add two cents:

Probably you tested it in a 32 bits setup; to compile in a 64 bits you need to (only differences):

    #If Win64 Then
    Private Declare PtrSafe Sub CopyMemory Lib "kernel32.dll" Alias "RtlMoveMemory" (hpvDest As Any, hpvSource As Any, ByVal cbCopy As LongPtr)
    Private Type TReference
        Address As LongPtr
    End Type
    #Else
    Private Declare Sub CopyMemory Lib "kernel32.dll" Alias "RtlMoveMemory" (hpvDest As Any, hpvSource As Any, ByVal cbCopy As Long)
    Private Type TReference
        Address As Long
    End Type
    #End If
         ...
    #If Win64 Then
    Public Property Get Address() As LongPtr
        Address = this.Address
    End Property
    Public Property Let Address(ByVal value As LongPtr)
        this.Address = value
    End Property
    #Else
    Public Property Get Address() As Long
        Address = this.Address
    End Property
    Public Property Let Address(ByVal value As Long)
        this.Address = value
    End Property
    #End If

And your test works:

Public Sub Test()
    Dim p As TheParent
    Set p = New TheParent
    Debug.Print ObjPtr(p)
    Set p = Nothing
End Sub

And now things seems to be OK. But...

Now, try to do something useful with Parent property of Child class, not only assure GC:

Slightly modified Parent Class; Child class the same:

 Option Explicit
Private Type tTheParent
    Child As TheChild
    CollectionName As String
End Type
Private this As tTheParent

Private Sub Class_Initialize()
    Set this.Child = New TheChild
    Set this.Child.Parent = Me
    Debug.Print this.Child.Parent.CollectionName
End Sub
Public Property Get GetChild() As TheChild
    Set GetChild = this.Child
End Property
Private Sub Class_Terminate()
    Set this.Child = Nothing
End Sub

Public Property Get CollectionName() As String
    CollectionName = IIf(this.CollectionName = vbNullString, "COLLECTION", this.CollectionName)
End Property

New Test:

 Public Sub TestChildParent()
    Dim p As TheParent
    Set p = New TheParent
    Dim c As TheChild
    Set c = p.GetChild
    Debug.Print c.Parent.CollectionName
    Debug.Print ObjPtr(p)

    Set p = Nothing
 End Sub

And what we get? I crashed Excel, Word and the Video driver!

And now the @this's comment shines; now I got illuminated by his (this?) words:

  Please don't hard-code the size... ever. LenB is to VBA what sizeof is
  to C. Use it to size the allocation for the copy memory API. – this

I was directed here because yesterday I posted at SO an error 53 getting kernel, and until I crash windows I didn't understood that the correct Bruce McKinney's code for getting an Object from the object pointer is:

Dim objT As Object
CopyMemory objT, this.Address, LenB(this.Address) 'not 4
Set IWeakReference_Object = objT
CopyMemory objT, 0&, LenB(this.Address) 'not 4

And this answer my question at SO too, because the kernel 53 error gone...

PS: Please, don't be surprised at this enigmatic approach using this as Private Type
someone that I follow teached me ;)

PS2: And this, thanks for that! sorry, couldn't hold myself


Selecting mail messages that meet one of four search criteria This is the current very repetitive and long SQL I have. Unfortunately I'm not that good at SQL to group this and get rid of the repetition.

select * from Mail
where admin_status = 1
and admin_id = 4
and core_event_id =0
and user_id !=0
and mail_reply_status = 0
and sent_to = 0
and admin_inbox_date > DATE_SUB(CURDATE(), INTERVAL 48 HOUR)

or ( admin_status = 1
and admin_id = 4
AND sent_to = 0
and mail_reply_status = 0
and user_id != 0
and core_event_inactive = 1)
and admin_inbox_date > DATE_SUB(CURDATE(), INTERVAL 48 HOUR)

or (`subject` like 'RE:%'
AND admin_status = 1
AND `admin_id` = 4
and core_event_id =0
and user_id != 0
and mail_reply_status = 0
and sent_to = 4 )
and admin_inbox_date > DATE_SUB(CURDATE(), INTERVAL 48 HOUR)

or (`subject` like 'RE:%'
and admin_status = 1
and admin_id = 4
AND sent_to = 0
and mail_reply_status = 0
and user_id != 0
and core_event_inactive = 1)
and admin_inbox_date > DATE_SUB(CURDATE(), INTERVAL 48 HOUR)

First of all, the indentation is next to inexisting. The code is badly formatted. Are you scared of tabs? There's no excuse for a wall of unformatted SQL. There are online tools to fix it for you!

You should have all keywords in capitals. Why is that? Well, it's an old convention that comes from times where there was no syntax highlighting. Nowadays, they are everywhere! Heck, even I made one! But still, capitals help to read it.

And as such, your Mail table should be all in lowercase.

You are extremelly inconsistent with your backticks. Either you use it or you don't use it! Don't mix both, please!

And now, to remove repetition.

All messages are within an interval of 48 hours.
All have admin_status = 1
admin_id is always the same
sent_to only changes if subject like 'RE:%'
mail_reply_status is always 0
user_id is never 0
core_event_inactive shows twice with the value of 0

With this information, I've made this:

SELECT * FROM mail WHERE admin_status = 1
    AND admin_id = 4
    AND user_id = 0
    AND mail_reply_status = 0
    AND admin_inbox_date > DATE_SUB(CURDATE(), INTERVAL 48 HOUR)
    AND ( sent_to = 0 XOR ( sent_to = 4  AND subject LIKE 'RE:%' ) )

Since the LIKE is a really heavy check, we put it at the very last. Since SQL supports short-circut evaluation, this positioning may result in faster queries. The slowest checks are the last ones.

It was really hard to follow if everything is 200% correct, but I hope I've nailed it. Please, comment if you see something innacurate.


Using the $q service in AngularJS to handle promises I have a service which needs to store some settings data in a DB and at the same time maintain a reference to that in the user'slocalStorage (we want two users logged in to the same account to be able to view them differently). This method looks for an existing previewSession in the localStorage and if not found, creates one and generates a unique URL.

As you can see at the beginning, I'm using the $q service to return a promise and then resolving that promise as and when all operations have finished. There is one use of the $http service and the save call is on a $resource object.

Is this a proper way to handle such a use-case, or could I have done it better?

To summarize the requirements:

Method gets called with a settings parameter
Method must always return a URL (unless an error occurs on the db level or some such thing)

var PREVIEW_SESSION_KEY_BASE = '<a constant string>';
function getPreviewSessionUrl(settings) {
        var deferred = $q.defer();

        $http.get('/' + appCode + '/reservedDomain',
            function(appResponse) {
                $localForage
                    .getItem(PREVIEW_SESSION_KEY_BASE + themeId)
                    .then(function(data) {
                        if (data) {
                            deferred.resolve(appResponse + '?<a query key>=' + data);
                        } else {
                            previewSessionResource.save(
                                {},
                                {
                                    settings: settings
                                },
                                function(previewResponse) {
                                    $localForage
                                        .setItem(PREVIEW_SESSION_KEY_BASE + themeId, previewResponse)
                                        .then(function(data) {
                                            deferred.resolve(appResponse + '?<a query key>=' + data);
                                        });
                                },
                                function(error) {
                                    deferred.reject(error);
                                }
                            );
                        }
                    });
            },
            function(error) {
                deferred.reject(error);
            }
        );

        return deferred.promise;
    }

Things that could have been better;

This:

        },
        function(error) {
            deferred.reject(error);
        }
    );

could simply be

        }, deferred.reject );

reject is a function that takes error as a parameter after all..
I would have externalized the previewSessionResource.save(call, it makes the reading too hard. You only capture settings and appResponse which you would then provide as parameters.
If you control the signature of previewSessionResource.save, then I would simply request for settings as an object instead of requesting for { settings: settings } to keep it simple, it does not make sense to wrap a single data point into an object.
I am also idly wondering if previewSessionResource.save should not be responsible for calling $localForage.setItem(), would you ever call previewSessionResource.save without calling $localForage.setItem()?

On the whole, I think this is okay, it just needs some rejiggering to read better.


Web token authentication in Golang I am new to Go and the web token / JTW concept. In order to learn Go and learn about web tokens, I am making my own implementation in Go (never to be used, of course).

If a user gets authenticated (assume that part is secure), I generate a web token, [header].[payload].[signature]. The header contains a timestamp of when the token was created. The payload has the user id and maybe role in there too, and the signature is there to show that the payload hasn't been altered.

Are there any security concerns with this?
Is there a way to improve the performance / speed of this?

package gwt

import (
    "crypto/hmac"
    "crypto/sha512"
    "crypto/subtle"
    "encoding/base64"
    "encoding/json"
    "errors"
    "fmt"
    "os"
    "strings"
    "time"
)

var Expired error = errors.New("Token expired")
var Invalid error = errors.New("Invalid token")

var expireAfter int = 60 * 60 * 24 * 7
var refreshAfter int = 60 * 60 * 24 * 3
var secret []byte

func init() {
    gwtSecret := os.Getenv("GWT_SECRET")
    if gwtSecret == "" {
        fmt.Println("You must set GWT_SECRET environment variable: export GWT_SECRET=change_this")
        os.Exit(0)
    } else if len(gwtSecret) < 16 {
        fmt.Println("GWT_SECRET must be longer than 16 characters")
        os.Exit(0)
    }

    secret = []byte(gwtSecret)
}

func Expire(e int, r int) {
    expireAfter = e
    refreshAfter = r
}

func Make(object map[string]interface{}) (string, error) {
    exp := time.Now().Format(time.UnixDate)
    header := base64.StdEncoding.EncodeToString([]byte(exp))

    payloadBytes, err := json.Marshal(object)
    if err != nil {
        return "", err
    }

    payload := base64.StdEncoding.EncodeToString(payloadBytes)

    return header + "." + payload + "." + sign(header+payload), nil
}

func GetPayload(token string) (map[string]interface{}, string, error) {
    parts := strings.Split(token, ".")
    if len(parts) != 3 || subtle.ConstantTimeCompare([]byte(sign(parts[0]+parts[1])), []byte(parts[2])) != 1 {
        return nil, "", Invalid
    }

    headerBytes, err := base64.StdEncoding.DecodeString(parts[0])
    if err != nil {
        return nil, "", Invalid
    }

    header := string(headerBytes)

    created, err := time.Parse(time.UnixDate, header)
    if err != nil {
        return nil, "", Invalid
    }

    expired := created.Add(time.Second * time.Duration(expireAfter)).Before(time.Now())
    if expired {
        return nil, "", Expired
    }

    payloadBytes, err := base64.StdEncoding.DecodeString(parts[1])
    if err != nil {
        return nil, "", Invalid
    }

    var payload map[string]interface{}
    if err := json.Unmarshal(payloadBytes, &payload); err != nil {
        return nil, "", Invalid
    }

    var newToken string
    stale := created.Add(time.Second * time.Duration(refreshAfter)).Before(time.Now())
    if stale {
        newToken, err = Make(payload)
        if err != nil {
            return nil, "", Invalid
        }
    } else {
        newToken = ""
    }

    return payload, newToken, nil
}

func sign(s string) string {
    h := hmac.New(sha512.New, secret)
    h.Write([]byte(s))

    return base64.StdEncoding.EncodeToString(h.Sum(nil))
}

The token will be stored locally on the client, sent via http headers on the necessary requests, and everything is https.

An updated version can be found here.

Go Code Review Comments

  Don't Panic

  Don't use panic for normal error handling. Use error and multiple
  return values.

func Make(object interface{}) string {
    payloadBytes, err := json.Marshal(object)
    if err != nil {
        panic(err)
    }
    payload := base64.StdEncoding.EncodeToString(payloadBytes)
    return payload + "." + sign(payload)
}

I'm going to exploit this, an exported function that panics, to crash your application.

func sign(s string) string {
    h := hmac.New(sha256.New, []byte("secret"))
    h.Write([]byte(s))
    return base64.StdEncoding.EncodeToString(h.Sum(nil))
}

I'm going use a literal embedded in your executable, []byte("secret"), to break your authentication.


Cheapest flights with at most k stops This is the Cheapest Flights Within K Stops problem from leetcode.com:

  There are \$n\$ cities connected by \$m\$ flights. Each fight starts from city \$u\$ and arrives at \$v\$ with a price \$w\$.

  Now given all the cities and fights, together with starting city \$src\$ and the destination \$dst\$, your task is to find the cheapest price from \$src\$ to \$dst\$ with up to \$k\$ stops. If there is no such route, output \$-1\$.

I have a naive implementation that fails on efficiency. I had a feeling this would happen, but I took this approach to implement my first ever iterative deepening depth first search (IDDFS). Since it fails on runtime, I don't know if my code is 100% valid from an implementation standpoint

My algorithm:

Enumerate all paths of depth less than \$K + 1\$ using IDDFS, add to priority queue by path cost
If the queue isn't empty, a path exists that makes at most \$K\$ stops, extract min from priority queue.

Obviously step 1 grows exponentially and isn't at all efficient.

What I would like help reviewing is whether my iterative deepening depth first search is from an implementation standpoint correct.

def findCheapestPrice(self, n, flights, src, dst, K):
        """
        :type n: int
        :type flights: List[List[int]]
        :type src: int
        :type dst: int
        :type K: int, K + 1 = max search depth, K + 2 = amount of visited nodes
        :rtype: int
        """
        from collections import defaultdict
        import heapq
        adj = defaultdict(list) # u : (v, cost)

        for u,v,c in flights:
            adj[u].append((v,c))

        paths = [] # pqueue

        def enumerateAllPaths(u, goal, visited, path, pathcost, depthLimit):
            visited[u] = True
            path.append(u)

            if u == goal:
                heapq.heappush(paths, (pathcost, len(path), path))
            elif depthLimit == 0:
                path.pop()
                visited[u] = False
                return
            else:
                for v,c in adj[u]:
                    if not visited[v]:
                        enumerateAllPaths(v, goal, visited, path, pathcost + c, depthLimit - 1)
            path.pop()
            visited[u] = False

        visited = [False for s in range(len(n))]
        path = []

        enumerateAllPaths(src, dst, visited, path, 0, K + 1)

        # (length of path, pathcost, path)
        bestcost = 2**63 - 1 # best cost decreases if a better path is found

        if paths:
            pathcost, pathlen, path = heapq.heappop(paths)
            bestcost = pathcost

        if bestcost != 2**63 - 1:
            return bestcost
        else:
            return -1

1. Review

There doesn't seem to be any iterative deepening going on. The code just implements an ordinary depth-first search for all simple paths in the graph. Iterative deepening would require calling enumerateAllPaths first with depthLimit=1, then with depthLimit=2 and so on.
The docstring should explain what the parameters mean.
Imports are best done once at the top level of a module, not every time a function is called.
The comment for paths says "pqueue" which I don't understand (is it an abbreviation for "priority queue"?). It would be better to be explicit, for example I would write something like "heap of (cost, length, path) tuples for paths that reach the goal".
The code for getting the best path out of the heap is needlessly long-winded, and has a bug (if the best path has cost \$2^{63}-1\$ it will not be found). Better to write:

if paths:
    cost, _, _ = heapq.heappop(paths)
    return cost
else:
    return -1

If you need a number that's larger than any possible cost, use infinity, that is, float('inf') instead of an arbitrary number.
The goal of the problem is to find just one path to the destination (the cheapest one), so it is wasteful to maintain a collections of all paths to the destination. Better just to remember the best path found so far.
There is no need to pass the variables goal and visited and path into the enumerateAllPaths function. These do not change during the search.
These lines of code are repeated:

path.pop()
visited[u] = False

By testing depthLimit != 0 instead of depthLimit == 0 you could avoid this repetition.
Instead of implementing visited as a dictionary mapping nodes to True or False, use a set of nodes instead.
The path list is not needed: this is only used to find the length of the path, but that is redundant as len(path) is always equal to K + 1 - depthLimit. And since you don't care about the actual length of the path, only its relative length compared with other paths, you can drop the K + 1 (since this is the same for all paths) and just use -depthLimit.

2. Revised code

from collections import defaultdict

def cheapest_path_cost(edges, src, dst, k):
    """Return the cost of the cheapest path in the graph from src to dst
    visiting no more than k other nodes. The graph is specified as a
    sequence of tuples (u, v, c) meaning that there is an edge from u
    to v with cost c. If there is no path, return infinity.

    """
    graph = defaultdict(list)  # {u: (v, cost)}
    for u, v, c in edges:
        graph[u].append((v, c))
    infinity = float('inf')
    best = infinity, 0         # (cost, -depth) of best path found.
    visited = set()            # Set of nodes visited on current path.

    def enumerate_paths(u, cost, depth):
        # Enumerate simple paths of length up to depth, starting at u.
        nonlocal best
        visited.add(u)
        if u == dst:
            best = min((cost, -depth), best)
        elif depth:
            for v, c in graph[u]:
                if v not in visited:
                    enumerate_paths(v, cost + c, depth - 1)
        visited.remove(u)

    enumerate_paths(src, 0, k + 1)
    return best[0]

3. Testing

You write in the post:

  Since it fails on runtime, I don't know if my code is 100% valid from an implementation standpoint.

This suggests to me that you have not tested your code. Is that right? You say in comments that you "already solved this problem with Bellman–Ford" in which case you could use your other solution as a test oracle.

Here's the kind of test that you might quickly implement using a test oracle. Here I'm using the compressed sparse graph routines in scipy.sparse.csgraph, but any other graph library would do.

import numpy as np
from scipy.sparse.csgraph import csgraph_from_dense, dijkstra
from unittest import TestCase

class TestCheapestPathCost(TestCase):
    def test_cheapest_path_cost(self):
        # Random graph on n nodes with given density and costs up to maxcost.
        n = 10
        density = 0.3
        maxcost = 5
        graph = np.random.randint(1, maxcost + 1, (n, n))
        graph[np.random.random((n, n)) > density] = 0

        # Convert to array of edges.
        i, j = np.mgrid[:n, :n]
        edges = np.dstack((i, j, graph))
        edges = edges[edges[..., 2] != 0]

        # Find shortest paths and compare.
        shortest = np.dstack((i, j, dijkstra(graph))).reshape(-1, 3)
        for src, dst, expected in shortest:
            self.assertEqual(cheapest_path_cost(edges, src, dst, n), expected)

4. Performance

There are a couple of easy optimizations you can make in depth-first searches.

First, prune paths that are no better than the best path so far, like this:

def enumerate_paths(u, cost, depth):
    # Enumerate simple paths of length up to depth, starting at u.
    nonlocal best
    visited.add(u)
    if u == dst:
        best = cost, -depth
    elif depth:
        for v, c in graph[u]:
            if v not in visited and (cost + c, 1 - depth) < best:
                enumerate_paths(v, cost + c, depth - 1)
    visited.remove(u)

Note that we don't need the call to min in this version of the code because we already tested that the cost was an improvement, before visiting this search node.

Second, when constructing the adjacency list representation of the graph, sort the edges at each vertex by their cost. This means that lower-cost paths are visited sooner in the search, and this makes it more likely that the pruning will be effective.


What is the best approach to use in R and why? I'm starting working with R, and found some tutorials and exercises online.

I want to divide one variable in two, bigger and equal 79 and smaller 79.

Perhaps because I'm used to python, my first approach was to do something like this:

z <- numeric(length(faithful$waiting))
n = 0
for (i in faithful$waiting) {
n = 1 + n
if (i < 79) z[n] <- 1
}

But I found many tutorials that use this solution instead:

min_wait <- min(faithful$waiting)-0.1
max_wait <- max(faithful$waiting)
cutof <- c(min_wait,79,max_wait)
waiting_cat <- cut (faithful$waiting, breaks=cutof)

What is the best way to do something like this? And can someone explain why.

Thank you!

As you realize, your first approach works (it gives a result consistent with the criteria you specify), but it is not idiomatic R. Iterating over elements of a set/list/vector is idiomatic of python, and does have a place in R as well. However, what this approach misses is 2 aspects of R: inherent vectorization and the factor data type.

In R, all basic types are vectors and can hold multiple items (of the same type).  A single value is just a special case of a length 1 vector.  Since everything is a vector, all the standard functions are designed to operate on the whole vector at once.  They are implicitly vectorized over the elements of the vector rather than needing an explicit loop (iteration or for loop) to operate on each element.  So the first simplification is to eliminate the for loop over elements of faithful$waiting and just do the comparison on the whole vector.

> faithful$waiting < 79
  [1] FALSE  TRUE  TRUE  TRUE FALSE  TRUE FALSE FALSE  TRUE FALSE  TRUE FALSE
 [13]  TRUE  TRUE FALSE  TRUE  TRUE FALSE  TRUE FALSE  TRUE  TRUE  TRUE  TRUE
 [25]  TRUE FALSE  TRUE  TRUE  TRUE FALSE  TRUE  TRUE  TRUE FALSE  TRUE  TRUE
 [37]  TRUE FALSE  TRUE FALSE FALSE  TRUE FALSE  TRUE  TRUE FALSE  TRUE  TRUE
 [49] FALSE  TRUE  TRUE FALSE  TRUE FALSE  TRUE FALSE  TRUE  TRUE  TRUE FALSE
 [61]  TRUE FALSE  TRUE FALSE  TRUE FALSE  TRUE  TRUE  TRUE  TRUE FALSE  TRUE
 [73] FALSE  TRUE  TRUE  TRUE  TRUE  TRUE  TRUE FALSE  TRUE FALSE  TRUE  TRUE
 [85]  TRUE FALSE  TRUE FALSE  TRUE FALSE  TRUE FALSE  TRUE  TRUE  TRUE  TRUE
 [97] FALSE  TRUE  TRUE FALSE  TRUE FALSE  TRUE FALSE FALSE  TRUE FALSE  TRUE
[109] FALSE FALSE  TRUE  TRUE FALSE FALSE  TRUE FALSE  TRUE FALSE  TRUE FALSE
[121]  TRUE  TRUE  TRUE  TRUE FALSE FALSE  TRUE FALSE  TRUE FALSE  TRUE FALSE
[133]  TRUE FALSE  TRUE FALSE  TRUE FALSE  TRUE FALSE FALSE  TRUE FALSE  TRUE
[145]  TRUE  TRUE FALSE  TRUE FALSE  TRUE  TRUE  TRUE  TRUE FALSE  TRUE  TRUE
[157] FALSE FALSE  TRUE FALSE  TRUE FALSE  TRUE  TRUE  TRUE  TRUE  TRUE FALSE
[169]  TRUE FALSE  TRUE  TRUE  TRUE  TRUE FALSE FALSE  TRUE  TRUE FALSE  TRUE
[181]  TRUE  TRUE FALSE FALSE  TRUE  TRUE FALSE  TRUE FALSE  TRUE FALSE  TRUE
[193]  TRUE FALSE  TRUE FALSE FALSE  TRUE  TRUE  TRUE  TRUE FALSE FALSE  TRUE
[205]  TRUE  TRUE  TRUE FALSE  TRUE FALSE  TRUE FALSE  TRUE  TRUE  TRUE  TRUE
[217]  TRUE FALSE  TRUE  TRUE  TRUE FALSE  TRUE  TRUE  TRUE FALSE  TRUE  TRUE
[229]  TRUE FALSE  TRUE  TRUE FALSE  TRUE FALSE  TRUE  TRUE  TRUE FALSE  TRUE
[241]  TRUE  TRUE FALSE  TRUE FALSE FALSE  TRUE FALSE  TRUE  TRUE  TRUE FALSE
[253]  TRUE  TRUE FALSE FALSE  TRUE FALSE  TRUE FALSE  TRUE FALSE  TRUE FALSE
[265]  TRUE  TRUE  TRUE FALSE  TRUE FALSE  TRUE  TRUE

This brings up another aspect.  faithful$waiting is of length 272, 79 is length 1.  Argument recycling causes the 79 to be repeated until it is of the same length as faithful$waiting.  Then the comparison is done element-wise, returning a logical variable.  If you want it as a numeric (as in your first example), this can be converted directly: FALSE becomes 0 and TRUE becomes 1

> as.numeric(faithful$waiting < 79)
  [1] 0 1 1 1 0 1 0 0 1 0 1 0 1 1 0 1 1 0 1 0 1 1 1 1 1 0 1 1 1 0 1 1 1 0 1 1 1
 [38] 0 1 0 0 1 0 1 1 0 1 1 0 1 1 0 1 0 1 0 1 1 1 0 1 0 1 0 1 0 1 1 1 1 0 1 0 1
 [75] 1 1 1 1 1 0 1 0 1 1 1 0 1 0 1 0 1 0 1 1 1 1 0 1 1 0 1 0 1 0 0 1 0 1 0 0 1
[112] 1 0 0 1 0 1 0 1 0 1 1 1 1 0 0 1 0 1 0 1 0 1 0 1 0 1 0 1 0 0 1 0 1 1 1 0 1
[149] 0 1 1 1 1 0 1 1 0 0 1 0 1 0 1 1 1 1 1 0 1 0 1 1 1 1 0 0 1 1 0 1 1 1 0 0 1
[186] 1 0 1 0 1 0 1 1 0 1 0 0 1 1 1 1 0 0 1 1 1 1 0 1 0 1 0 1 1 1 1 1 0 1 1 1 0
[223] 1 1 1 0 1 1 1 0 1 1 0 1 0 1 1 1 0 1 1 1 0 1 0 0 1 0 1 1 1 0 1 1 0 0 1 0 1
[260] 0 1 0 1 0 1 1 1 0 1 0 1 1

The second aspect of R I mentioned was factors.  Factors are the implementation of a data type which can take on any of a predefined set of values.  In some languages, these are enumerated types.  Internally, they are stored as integer indexes into a vector of values (and this sometimes shows through).

You can create a factor from another vector by defining the levels it can take and, optionally, labels these levels should be displayed as.  Continuing the example

> factor(as.numeric(faithful$waiting < 79), levels=c(1,0), labels=c("<79", "79+"))
  [1] 79+ <79 <79 <79 79+ <79 79+ 79+ <79 79+ <79 79+ <79 <79 79+ <79 <79 79+
 [19] <79 79+ <79 <79 <79 <79 <79 79+ <79 <79 <79 79+ <79 <79 <79 79+ <79 <79
 [37] <79 79+ <79 79+ 79+ <79 79+ <79 <79 79+ <79 <79 79+ <79 <79 79+ <79 79+
 [55] <79 79+ <79 <79 <79 79+ <79 79+ <79 79+ <79 79+ <79 <79 <79 <79 79+ <79
 [73] 79+ <79 <79 <79 <79 <79 <79 79+ <79 79+ <79 <79 <79 79+ <79 79+ <79 79+
 [91] <79 79+ <79 <79 <79 <79 79+ <79 <79 79+ <79 79+ <79 79+ 79+ <79 79+ <79
[109] 79+ 79+ <79 <79 79+ 79+ <79 79+ <79 79+ <79 79+ <79 <79 <79 <79 79+ 79+
[127] <79 79+ <79 79+ <79 79+ <79 79+ <79 79+ <79 79+ <79 79+ 79+ <79 79+ <79
[145] <79 <79 79+ <79 79+ <79 <79 <79 <79 79+ <79 <79 79+ 79+ <79 79+ <79 79+
[163] <79 <79 <79 <79 <79 79+ <79 79+ <79 <79 <79 <79 79+ 79+ <79 <79 79+ <79
[181] <79 <79 79+ 79+ <79 <79 79+ <79 79+ <79 79+ <79 <79 79+ <79 79+ 79+ <79
[199] <79 <79 <79 79+ 79+ <79 <79 <79 <79 79+ <79 79+ <79 79+ <79 <79 <79 <79
[217] <79 79+ <79 <79 <79 79+ <79 <79 <79 79+ <79 <79 <79 79+ <79 <79 79+ <79
[235] 79+ <79 <79 <79 79+ <79 <79 <79 79+ <79 79+ 79+ <79 79+ <79 <79 <79 79+
[253] <79 <79 79+ 79+ <79 79+ <79 79+ <79 79+ <79 79+ <79 <79 <79 79+ <79 79+
[271] <79 <79
Levels: <79 79+

Note that in the output of the factor, the possible levels are listed as any given vector may or may not have an every possible value in it.  The set of allowed values can be larger than the ones that are present in any given vector.

For dividing into two groups based on a single cut point, this is fine, but this approach becomes unwieldy when making more than two groups.  cut is a built-in function which is designed to do just this: categorize a continuous variable based on a set of cut points.  There is always one fewer groups than cut points because the points define the edges of the groups (and anything outside the extremes is set to NA).  When including everything, I don't usually compute the extremes and use those, but use -Inf and Inf.  By default, the intervals are closed on the right which means that the upper value is included in the range but the lower value is not.  Since you wanted 79 in the upper group, you have to include right=FALSE.

> cut(faithful$waiting, breaks=c(-Inf,79,Inf), right=FALSE)
  [1] [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf)
  [8] [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79)
 [15] [79, Inf) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79)
 [22] [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79)
 [29] [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79)
 [36] [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [79, Inf) [-Inf,79)
 [43] [79, Inf) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [79, Inf)
 [50] [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf)
 [57] [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79)
 [64] [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79)
 [71] [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79)
 [78] [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79)
 [85] [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79)
 [92] [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79)
 [99] [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [79, Inf)
[106] [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [79, Inf) [-Inf,79) [-Inf,79)
[113] [79, Inf) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79)
[120] [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [79, Inf)
[127] [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79)
[134] [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf)
[141] [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf)
[148] [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf)
[155] [-Inf,79) [-Inf,79) [79, Inf) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79)
[162] [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf)
[169] [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf)
[176] [79, Inf) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79)
[183] [79, Inf) [79, Inf) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf)
[190] [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf)
[197] [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [79, Inf)
[204] [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf)
[211] [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79) [-Inf,79)
[218] [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79)
[225] [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79)
[232] [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79)
[239] [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf)
[246] [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79) [-Inf,79) [79, Inf)
[253] [-Inf,79) [-Inf,79) [79, Inf) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79)
[260] [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79)
[267] [-Inf,79) [79, Inf) [-Inf,79) [79, Inf) [-Inf,79) [-Inf,79)
Levels: [-Inf,79) [79, Inf)

This gives the idiomatic way of doing this as:

waiting_cat <- cut(faithful$waiting, breaks=c(-Inf,79,Inf), right=FALSE)

It implicitly loops over faithful$waiting and returns a factor which represents a categorical set of choices.


Coding multiples that equal 100 The objective was to see what numbers can be multiplied to equal 100.

Example:

1 x 1 = 1 //no match
5 x 20 = 100 //match

The objective (as I'm learning) was only to use an array as I wanted to practice how it functioned. There are probably other classes I could have used which would have been better, for example, an ArrayList to store a match rather than an array defined of [100].

I'm pretty new to programming and would like your opinion on my following block of code.

public class ArraySearchTest
{

    public ArraySearchTest()
    {
        doTest();
    }

    public void doTest()
    {
        int [] array = new int[100];
        int [] bbb = new int[11];
        int [] ans = new int[100];

        for(int i = 0; i < array.length; i++)
        {
            for(int p = 0; p < bbb.length; p++)
            {
                if( p * i == 100)
                {
                    ans[p] = (p + i);
                    System.out.println(p + " x " + i + " Equals 100");
                }
            }
        }
    }
}

General Style:

Use functions where you can. You have embedded all the code in to one method.

For many reasons, you should create a function which takes the 100 value as a parameter. Additionally, the purpose of the code is to return pairs of numbers that have 100 as the product, so what you want is a container to store each pair.

This is a start to turning your code in to Object Oriented code.

Classes (OOP)

A container for a result, which is two values that multiply to the target, would be called something like CoFactors (Factors which together make the target). It would look something like:

public class CoFactor {
    private final int lesser, greater;

    public CoFactor(int lesser, int greater) {
        this.lesser = lesser;
        this.greater = greater;
    }

    public int getLesser() {
        return lesser;
    }

    public int getGreater() {
        return greater;
    }
}

Algorithm

Now, with that class, we can return the factors of a number with a method like:

public static final List<CoFactor> getCoFactors(int target) {
    ....
}

The above method will return a list of factors that have the target as a product.

How would that be coded? First, some math....

each factor of a number has a partner (perhaps, if the number is a square number, the partner is itself (3 x 3 = 9 for example) The lesser factor will only be smaller when the lesser factor is less than the square-root of the target....
each small factor has a partnering large factor, and there is only one partner.
we only need to find the small ones to also be able to find the large ones.
we can tell if a number is a factor if there's no remainder after a division (use the % modulo operator)

This can be computed as:

List<CoFactor> factors = new ArrayList<>();
int root = (int)Math.sqrt(target);
for (int attempt = 1; attempt <= root; i++) {
    if (target % attempt == 0) {
        factors.add(new CoFactor(attempt, target / attempt));
    }
}
return factors;


Dice roller - python I am about to graduate with my Associates degree in Math and will soon go for my bachelors. I've decided with two years of school left its best if I start learning to program. I am starting with python.

I recently completed a beginners course on YouTube from free code camp. I completed novice programs that come with the course. (Guessing game)(mad Libs) & a(multiple choice quiz)

Now I am moving on to complete 3 programs outside of the lecture before I move to another python course. My ideas were (Dice game), (Hangman), (short game)

Below is my python code for the dice game. I planned on creating an array and looping through then selecting a random number from the loop. It's proving a little difficult. so far I just print a 3x3 matrix then select a random number. fyi the 3x3 is suppose to represent a 9 sided dice.

My question: Is this code good enough for me to move on to my next program or should I stick with it and code the dice game the way I originally planned. Thank you for the feedback.

#Create dice using matrix 9 sides
#Create function: Give user option to roll dice
#Return random dice number  1-9

from random import seed
from random import randint
dice = [

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

def diceroll():
  start = input("do you want to play dice Y/N ")
  if start == "Y" or start == "y":
   print(dice[0])
   print(dice[1])
   print(dice[2])
  x = input("Do you want to roll the dice: Y/N ")

  while x == "y" or x =="Y":
   if  x != "y" or x !="Y":
     for i in range(1):
       roll=randint(1,9)
       print(roll)
   x = input("Do you want to roll the dice again: Y/N ")
 else:
  print("GoodBye: ")

A bit of honesty

When starting to review your code, I found it unintelligible and I was unable to follow the logic. I was able to understand what you are trying to do but not your implementation so I will post a few improvements here.

Structure

There is no need for the for loop in the body. You can see this as you do not use the value you gain from the interand. Because of this, we can cut the loop out.

.lower()

We can strip checking for a lowercase or uppercase answer by using .lower(). This takes a string and returns the all lowercase equivalent. This means we can strip the user input check to: if start.lower() == "y":

Walrus operator

New in Python 3.8 is the walrus operator! It looks like this :=. We can use this in the while loop as it can handle prompting the user to quit. We can combine this with the .lower() to simplify the while loop.

Unused import

Despite importing random.seed you never use it, we can remove it from the imports.

Final code

from random import randint
dice = [
[[1],[2],[3]],
[[4],[5],[6]],
[[7],[8],[9]]
]

def diceroll():
    """Loop over dice and print the value"""
    start = input("do you want to play dice Y/N ")
    if start.lower() == "y":
        print(dice[0])
        print(dice[1])
        print(dice[2])

        while (x := input("Do you want to roll the dice?  [y/n]: ").lower()) == "y":
            roll=randint(1,9)
            print(roll)
    else:
        print("GoodBye: ")

Further improvements

The function uses a global variable which is a bad practice, it is much better to give the function the global variable as an argument. I did not know if you wanted the dice array for something else, so I left it as it is. Also I question the need for a dice array in the first place, but again, this is your code with your own spec.


Python set variables to shorter list and list length I have a simple task that I can't find an elegant solution to. Say I have two arrays and I want to find which one is shorter and create variables for both their lengths (specifying them as long and short). Here's an inelegant way to do it:

l1 = len(arr1)
l2 = len(arr2)

if l1 < l2:
    short_arr = arr1
    long_arr = arr2
    lshort = l1
    llong = l2
else:
    short_arr = arr2
    long_arr = arr1
    lshort = l2
    llong = l1

What's a better way to do it?

There is nothing inelegant about your code.  You are computing the length of each array exactly once, have one test, and assign variables in each of the two paths of the branch.  It is clean, fast, and efficient.

You could make it shorter by combining the assignments into structured assignment statement:

l1, l2 = len(arr1), len(arr2)

if l1 < l2:
    short_arr, long_arr = arr1, arr2
    lshort, llong = l1, l2
else:
    short_arr, long_arr = arr2, arr1
    lshort, llong = l2, l1

but it is debatable whether that is clearer.

I can’t claim this is more elegant, but it is significantly shorter:

short_arr, long_arr = (arr1, arr2) if len(arr1) < len(arr2) else (arr2, arr1)
lshort, llong = len(short_arr), len(long_arr)

And the absolutely wrong way to do it would be:

((lshort, short_arr), (llong, long_arr)) = sorted([(len(arr1), arr1), (len(arr2), arr2)])


Sum of factorial digits I have seen one algorithmic task and I am not sure if my solution is fully efficient. As input to the method you are given positive integer N. And you should return the sum of values of the digits in a decimal representation of N!.

For example, given N = 6 the function should return 9, because N != 720 and sum of digits 7 + 2 + 0 = 9. Result cannot exceeds 100000000. N is higher or equal than 1 and less or equal than 2000.

Is this an efficient solution and the best optimized?

public int solution(int N) {
    long factorial = 1;
    for (int i = 1; i <= N; i++)
        factorial = factorial * i;
    long res = 0;

    while(factorial>0) {
        res+=factorial%10;
        factorial/=10;
    }
    if (res > 100000000)
        return -1;
    return (int) res;
}

Your code only works for small values of N. The algorighm is effective, but unfortunately your overflows are real.

Factorial numbers get very big, very fast... I think it is around 12! that you run out of space in a 32-bit integer, and about 20! that your run out of space in a 64-bit long.

You need to convert your logic to use BigInteger instead of Long for it to work at all.