Imperative and functional-programming solutions for Jumping on the Cloud challenge Problem Statement

  Emma is playing a new mobile game that starts with consecutively
  numbered clouds. Some of the clouds are thunderheads and others are
  cumulus. She can jump on any cumulus cloud having a number that is
  equal to the number of the current cloud plus 1 or 2. She must
  avoid the thunderheads. Determine the minimum number of jumps it will
  take Emma to jump from her starting postion to the last cloud. It is
  always possible to win the game.

  For each game, Emma will get an array of clouds numbered 0 if they
  are safe or 1 if they must be avoided. For example,
  c=[0,1,0,0,0,1,0] indexed from 0...6. The number on each cloud is its index in the list so she must avoid the clouds at indexes 1
  and 5 . She could follow the following two paths:
  0 -> 2 -> 4 -> 6  or 0 -> 2 -> 3 -> 3 -> 4 -> 6 . The first path takes
  3 jumps while the second takes 4.

  Function Description

  Complete the jumpingOnClouds function in the editor below. It should
  return the minimum number of jumps required, as an integer.

  jumpingOnClouds has the following parameter(s):

  c: an array of binary integers

  Input Format

  The first line contains an integer n, the total number of clouds.
  The second line contains n space-separated binary integers
  describing clouds c[i] where 0 <= i < n.

  Output Format

  Print the minimum number of jumps needed to win the game.

  Sample Input 0

7
0 0 1 0 0 1 0

  Sample Output 0

4

  Explanation 0:  Emma must avoid c[2] and c[5]. She can win the
  game with a minimum of 4 jumps. Sample Input 1

6
0 0 0 0 1 0

  Sample Output 1

3

  Explanation 1:  The only thundercloud to avoid is c[4]. Emma can win
  the game in 3 jumps.

Imperative style Solution:

// Complete the jumpingOnClouds function below.
def jumpingOnClouds(c: Array[Int]): Int = {
    var i=0
    var length = c.length
    var jumps = 0
    while(i < length -1) {
        if(i < length-2 && c(i+2) == 0 ) i+=2
        else i+=1
        jumps +=1
    }
    jumps
}

Functional-programming style Solution using Recursion:

 def jumpingOnClouds(c: Array[Int]): Int = {
     val limit = c.length -2
     def rec(jumps: Int, index: Int): Int = {
         if (index > limit) jumps
         else {
             val jumpingOffset: Int = if(index < limit && c(index + 2) == 0 ) 2 else 1
             rec(jumps+1, index + jumpingOffset)
         }
     }
     rec(0,0)
 }

The thing about these "story" problems is that the story is often distracting, if not outright misleading, from the underlying problem to be solved.

In this case your code solves the challenge by calculating and counting the number of jumps it takes to get from the beginning to the end. More or less the way the story is layed out.  But it's worth noting:

The minimum number of jumps for a sequence of N zeros is always N/2.
At the end of every sequence of zeros is a 1 to be jumped over, except at the very end.

def jumpingOnClouds(c :Array[Int]) :Int =
  c.mkString.split("1").foldLeft(0)(_ + _.length/2 + 1) - 1

This isn't the most efficient solution possible but since the input Array is limited to no more than 100 elements I decided to go for brevity over efficiency.


Simplifying Dr.Racket alien code I am using Dr.Racket, Intermediate Student with Lambda. I was wondering if there was any way I can simplify this code using any sort of method like lambda, abstraction, map, filter, etc.

 ; alien-at-edge?: alien --> boolean
(define (alien-at-edge? an-alien)
(or (alien-at-right-edge? an-alien)
(alien-at-left-edge? an-alien)))

; any-alien-at-edge?: loa --> boolean
(define (any-alien-at-edge? a-loa)
(cond [(empty? a-loa) false]
[else (or (alien-at-edge? (first a-loa))
      (any-alien-at-edge? (rest a-loa)))]))

;alien-at-left-edge?: alien --> boolean
(define (alien-at-left-edge? an-alien)
 (<= (- (posn-x an-alien) ALIEN-DELTA-X) alien-half))

;alien-at-right-edge?: alien --> boolean
(define (alien-at-right-edge? an-alien)
(>= (+ (posn-x an-alien) ALIEN-DELTA-X) (- WIDTH alien-half)))

;any-alien-at-right-edge?: loa --> boolean
(define (any-alien-at-right-edge? a-loa)
(cond [(empty? a-loa) false]
[else (or (alien-at-right-edge? (first a-loa))
      (any-alien-at-right-edge? (rest a-loa)))]))

;any-alien-at-left-edge?: loa --> boolean
(define (any-alien-at-left-edge? a-loa)
(cond [(empty? a-loa) false]
[else (or (alien-at-left-edge? (first a-loa))
      (any-alien-at-left-edge? (rest a-loa)))]))

; alien-at-edge?: alien --> boolean
(define (alien-at-edge? an-alien)
(or (alien-at-right-edge? an-alien)
(alien-at-left-edge? an-alien)))

Not much to do here but don't forget about indentation to make your code readable.

; any-alien-at-edge?: loa --> boolean
(define (any-alien-at-edge? a-loa)
(cond [(empty? a-loa) false]
[else (or (alien-at-edge? (first a-loa))
      (any-alien-at-edge? (rest a-loa)))]))

This pattern occurs a lot in functional programming: produce a value (a boolean here) by applying a function (or here) to your list. You can rewrite your function as:

; any-alien-at-edge?: loa --> boolean
(define (any-alien-at-edge? a-loa)
  (foldl or false a-loa)

You can also use ormap if foldl is hard to understand.

;alien-at-left-edge?: alien --> boolean
(define (alien-at-left-edge? an-alien)
 (<= (- (posn-x an-alien) ALIEN-DELTA-X) alien-half))

;alien-at-right-edge?: alien --> boolean
(define (alien-at-right-edge? an-alien)
(>= (+ (posn-x an-alien) ALIEN-DELTA-X) (- WIDTH alien-half)))

Watch out for indentation and trailing spaces.

;any-alien-at-right-edge?: loa --> boolean
(define (any-alien-at-right-edge? a-loa)
(cond [(empty? a-loa) false]
[else (or (alien-at-right-edge? (first a-loa))
      (any-alien-at-right-edge? (rest a-loa)))]))

;any-alien-at-left-edge?: loa --> boolean
(define (any-alien-at-left-edge? a-loa)
(cond [(empty? a-loa) false]
[else (or (alien-at-left-edge? (first a-loa))
      (any-alien-at-left-edge? (rest a-loa)))]))

Those ones can also be implemented using foldl. Try it out!


CAPTCHA reader and maker I've made some CAPTCHA reader and maker in Python using Tkinter. Do you know if it's possible to keep image and output files in "memory" not writing them on disc? Because right now everything is 'stiffly' made. And what would you do differently?

gui.py

import tkinter as tk
from PIL import Image, ImageTk
import captcha_maker as captcha
import captcha_reader as cap

class MainApplication(tk.Frame):
    """Main window"""
    def __init__(self, parent):
        tk.Frame.__init__(self, parent)
        self.create_widgets()

    def create_widgets(self):
        """Creating widgets"""
        self.input_label = tk.Label(self, text="Text: ")

        self.input_text = tk.Entry(self)

        self.input_button = tk.Button(self, text="Generate captcha",
                                        command=self.make_captcha)
        self.img = ImageTk.PhotoImage(Image.open('bin/blank'))
        self.image_label = tk.Label(self, image= self.img)
        self.image_label.image = self.img

        self.strings = tk.StringVar()
        self.read_button = tk.Button(self, text="Read captcha",
                                        command=self.read_captcha)
        self.output_text = tk.Label(self, textvariable=self.strings)

        self.input_label.grid(row=0, column=0)
        self.input_text.grid(row=0, column=1)
        self.input_button.grid(row=1, column=0)
        self.image_label.grid(row=1, column=1)
        self.read_button.grid(row=2, column=0)
        self.output_text.grid(row=2, column=1)

    def make_captcha(self):
        captcha.make(self.input_text.get())
        self.img2 = ImageTk.PhotoImage(Image.open('bin/result'))
        self.image_label.configure(image=self.img2)
        self.image_label.image = self.img

    def read_captcha(self):
        cap.read()
        with open('bin/output.txt', 'r') as f:
            read_data = f.read().rstrip()
        self.strings.set(read_data)

if __name__ == "__main__":
    root = tk.Tk()
    MainApplication(root).grid()
    root.mainloop()

captcha_maker.py

from PIL import Image, ImageDraw, ImageFont, ImageTk
import io

def make(text):
    """Making simple and regular captcha"""
    im = Image.new("RGB", (len(text) * 13 , 27), "white")
    im.save('bin/blank', "png")
    base = Image.open('bin/blank').convert('RGBA')
    txt = Image.new('RGBA', base.size, (255,255,255,0))
    fnt = ImageFont.truetype('bin/Ubuntu-B.ttf', 24)
    d = ImageDraw.Draw(txt)
    d.text((0,0), text, font=fnt, fill=(0,0,0,255))
    out = Image.alpha_composite(base, txt)
    out.save("bin/result", "png")

if __name__ == ("__main__"):
    make("one")

captcha_reader.py

import subprocess
def read():
    """Just calling tesseract command from python"""
    subprocess.call(["tesseract", "bin/result", "bin/output"])

if __name__ == "__main__":
    read()

You can combine these 3 tricks to resolve your problem:

Create your images as string buffers using StringIO. In Python 2 you can import it by: import StringIO; while in Python 3 you can import it by running: import io then use io.StringIO.
Read your images from a string: im = Image.open(io.StringIO.StringIO(buffer))
You can share the images you are interested in across your three modules by making them global variables (How do I share global variables across modules?).


Return index of array where sum first passes n I have an array of integers and a number n.  I want to sum the elements of the array and return the index where the sum passes n, as efficiently as possible.

For example, assume:

theString = "6-7-7-10-7-6"
theNumber = 33

expected result: 5

This does the trick but not very efficiently: (and error prone; will check for errors when I've found the best solution)

Public Function getIndexFromSum(theString As String, theNumber As Integer) As Integer
    Dim theSides As Variant
    Dim sum As Integer
    Dim index As Integer

    theSides = Split(theString, "-")

    While sum <= theNumber
        sum = sum + theSides(index)
        index = index + 1
    Wend

    getIndexFromSum = index
End Function

I haven't been able to describe this to google succinctly enough to get any meaningful results, and searching for any combination of the keywords involved produces links to tons of unrelated stuff.  I'm sure this problem will have been encountered before (and likely has a smart solution out there somewhere), but I haven't been able to find any sites that can help.

Any help/advice would be very much appreciated!

EDIT - more info

1) theString will always be stored in the format "x-x-x".

2) theNumber will always be smaller than or equal to the sum of the numbers in theString.

3) The length of theString will be short - usually between 2 and 10 numbers, each less than 100.

4) The function will be called with small inputs, but many times.  Low overhead is top priority.

I'm not sure what you mean by "This does the trick but not very efficiently". I don't really see a way to make the algorithm more efficient, as it is just making a single pass over the array and exits early when it gets the answer. If the input is really long, you could convert it to a byte array instead of splitting it to cut down on the casts, but then you would have to nest the loops, add a subtraction to convert the ascii codes to numbers, add multiplications to keep track of multiples of 10, etc. This would likely be a lot slower than the current method unless you were working with strings on the order of kilobytes.

That said, a couple of observations about the code.  I'd personally use a For loop instead of a While loop to prevent you from running outside of the array bounds for inputs where the target number you are looking for is higher than the sum of the complete array.  For example, with the input string of "6-7-7-10-7-6", a target higher than 43 will overrun the array bound and throw an error.  You'll get a similar error if it is passed an empty string (Split will happily give you an array with an upper bound of -1 in that case).

Also note that you are not returning the zero based index, you are returning the one based position of the token in the string.  If this is the desired behavior, I'd rename the function to make it obvious that you can't use the return value to index into the array.

About the only thing that I would do to make this a tiny bit more efficient would be to use a String array instead of a Variant and use explicit casting so the run-time doesn't have to resolve the Variants for you more than once. Even this improvement will likely be unnoticeable on any reasonable sized input (you'll overflow an Integer long before you'd notice the performance increase anyway).

Maybe something like this:

Public Function getPositionFromSum(source As String, target As Integer) As Integer

    Dim sides() As String
    Dim sum As Integer
    Dim index As Integer

    getPositionFromSum = -1    'Or whatever the "not found" condition is.
    sides = Split(source, "-")

    For index = 0 To UBound(sides)
        sum = sum + CInt(sides(index))
        If sum > target Then
            getPositionFromSum = index + 1
            Exit For
        End If
    Next

End Function


A function determining intervals of values greater than threshold I wonder if there exists a shorter/more elegant functional programming way than listing all the possible cases. Here, a function that determines positions of beginning/end of subintervals greater than threshold is coded. The idea behind the listed code is to mark and retain the beginning of such an interval, then to push a tuple of (beginning,ending) as soon as the interval ends. Feel free to choose any other approach if needed.

-- | Determines the intervals greater than threshold.
--
-- Examples:
-- >>> intervals 0.5 [0, 0, 0, 1, 1, 0, 0, 0, 1, 1, 1, 0]
-- [(3,4),(8,10)]
-- >>> intervals 0.5 [1,0,0,0,1,1,0,0,0,1,1,1,0]
-- [(0,0),(4,5),(9,11)]
-- >>> intervals 0.5 [1,0,0,0,1,1,0,0,0,1,1,1,0,1,1,1]
-- [(0,0),(4,5),(9,11),(13,15)]
intervals :: Ord a => a -> [a] -> [(Int, Int)]
intervals threshold ys = f False 0 p
  where p = zip [0..] . map (> threshold) $ ys
        f :: Bool -> Int -> [(Int, Bool)] -> [(Int, Int)]
        f _ _ [] = []
        f True startPos ((bPos,b):[]) | b = [(startPos, bPos)]
                                      | otherwise = [(startPos, startPos)]
        f False _ ((bPos,b):[]) | b = [(bPos, bPos)]
                                | otherwise = []
        f True startPos ((aPos,a):(bPos,b):as) | a && b = f True startPos ((bPos,b):as)
                                               | a && (not b) = ((startPos, aPos)) : (f False 0 as)
                                               | otherwise = (startPos, startPos) : (f False 0 ((bPos,b):as))
        f False _ ((aPos,a):as) | a = f True aPos as
                                | otherwise = f False 0 as

(You can skip right to TL;DR for a simpler approach)

Your function actually determines the indices of list elements that are above a threshold. In Haskell, when you have a list, an index is not the idiomatic way to represent its items. What do you want with those indices?

Agreed, your version is hard to read. For another approach, I start with

intervalsT :: [Bool] -> [(Int, Int]

and notice that the group function might come in handy to collect subsequent equal elements.

*Main> group [True,True,False,False,True]
[[True,True],[False,False],[True]]

mapping length will result in [2,2,1], which is a step closer to the indices. To turn [a,b,c] into [0,a ,a+b, a+b+c], the function scanl' is perfect:

*Main> scanl' (+) 0 [2,2,1]
[0,2,4,5]

which we can zip with its own tail. But wait! We lost information whether something is above or below threshold.
zip it again with the grouped Bools, filter based on the bools, throw away the bools. This yields:

TL;DR

intervals p = intervalsT . map (>p)
intervalsT :: [Bool] -> [(Int,Int)]
intervalsT xs = let grouped = group xs
                    idx = scanl' (+) 0 . map length $ grouped
                    ivs = zip idx (map (subtract 1) $ tail idx)
                 in map snd $ filter fst $ zip (map head grouped) ivs


Serializing user data to send to another API I'm trying to improve my OOP skills in PHP. One of my projects requires me to get user info from one API and send it to another API with a different structure.

I will use a simplified situation, but basically I have a big user array from one API. I need to break this into sub objects (customer and address) and serialize the data ready to be sent as a string over SOAP to another API.

I have the following user classes:

class User_customer{

    public $data;
    public $new_user_obj;

    public function __construct($data){
        $this->data = $data;
        $this->set_data();
    }

    public function set_data(){
        $this->new_user_obj->ID = $this->data->id;
        $this->new_user_obj->FirstName = $this->data->first_name;
        $this->new_user_obj->LastName = $this->data->last_name;
    }

    public function serialize_data(){
        $serializer = new XMLSerializer();

       // Notice I send a second param below - just indicating that sometimes
       // Although very similar, these params could be different per object
        return $serializer->serializeObj($this->new_user_obj, true);
    }
}

class User_address{

    public $data;
    public $new_user_obj;

    public function __construct($data){
        $this->data = $data;
        $this->set_data();
    }

    public function set_data(){
        $this->new_user_obj->ID = $this->data->id;
        $this->new_user_obj->Address = $this->data->home_address;
        $this->new_user_obj->Postcode = $this->data->postal_code;
    }

    public function serialize_data(){
        $serializer = new XMLSerializer();

        // Notice no second param below
        return $serializer->serializeObj($this->new_user_obj);
    }
}

And then in my main handling class, I'd call each section similar to:

$data = get_from_api();
$User_customer = new User_customer($data);
$User_address = new User_address($data);

send_to_other_api($User_customer->serialize_data(), $User_address->serialize_data());

Obvious alarm bells are ringing looking at this. I know that:

These classes are very similar, and there is a lot of code duplication.
I'm sending the same big data array with a lot of 'waste' to each class
I'm calling 'new XMLSerializer' so that means there is some 'coupling'
And no doubt a ton of other things

My instinct tells me I should have a parent class that these two inherit from, but it also looks like I should use an interface as both classes use the same methods. Also, maybe I should send an XMLSerializer object into the constructor when initiating each class (dependency injection?)

Any suggestions for 'the correct' way to write the above code would be helpful.

Below I have written how I tackled your code. I hope the approach is clear from the explanations and code example. Feel free to use this approach yourself.

When I took a closer look at your code I began looking for similarities which could easily be extracted into its own class. This in turn made some of the harder things to extract easier. I have tried (I am writing from a tablet :D) to write down my implementation of your problem. It has been a little hard though because I properly lack some context, so feel free to modify the code example if you decide to use it.

The first thing I changed was your dependency of the XMLSerializer into a dependency for a common interface. This will help you if you some time in the future decide to change the serialization format into JSON instead of XML. With a common interface you can swap the actual implementation during the method call as long as the new implementation also adhere to the common interface. This will also help mitigate your tight coupling concerns.

interface Serializer {

    /*
     * I could not see the use of the second argument from your
     * code, so I added an extra one just in case:D
     */
    public function serializeObj($object, $someUnknownFlag);

}

I then extracted the serialization logic and generation of the value object into its own class. This helped mitigate your code duplication concerns. When you read through the code example you might find that specifying which parameters are required is too much duplication, but I could not find a more flexible solution without making this too 'magical'.

I also think I corrected a possible bug in your code since the property new_user_obj is not instantiated anywhere.

How the class works are documented with comments throughout the code. I feel this gives you some freedom by having the serialization logic be free of how the actual data is fetched and let the extending class worry about that instead. This also makes it easier to add or remove required data parameters and/or change the behavior of the concrete class (UserCustomer, UserAddress etc.) without altering the serialization logic.

abstract class UserDataSerializer {

    private $data     = [];
    private $required = [];

    /**
     * @param array $data     An associative array of data that can be serialized.
     * @param array $required A sequential array of required data parameters.
     */
    public function __construct(array $data, array $required)
    {
        $this->data     = $data;
        $this->required = $required;
    }

    /**
     * Serializes the passed user data into the appropriate format.
     *
     * @param Serializer $serializer Here I have used an interface instead of a concrete implementation.
     *                               This will allow you to change to way the objects are serialized from
     *                               XML to maybe JSON by substituting to serializer implementation with
     *                               another serializer implementing the same interface.
     *
     * @return string Returns the data serialized into a string.
     */
    final public function serialize(Serializer $serializer)
    {

        /*
         * This method is made FINAL to ensure child classes doesn't
         * overwrite the implementation.
         */

        /*
         * By first executing the setData method now, we ensure
         * we only use instantiate a new object when it is actually needed.
         */
        $obj = $this->setData($this->data);

        return $serializer->serializeObj($obj, true);

    }

    private function setData(array $data) {

        /*
         * Check if all required parameters are present in the
         * associative array of data.
         *
         * First we calculate the differences from the required parameters
         * and the array keys (which are our names) from the data array.
         */
        $difference = array_diff($this->required, array_keys($data));

        /*
         * If the difference array is bigger than zero we are missing
         * one ore more required parameters.
         */
        if(count($difference) > 0) {
            throw new InvalidArgumentException('Insufficient data parameters provided. Missing: ' . implode(', ', $difference));
        }

        /*
         * We create a value object by using the ArrayObject and declaring
         * the access method for properties as class properties. This is done with
         * the STD_PROP_LIST flag.
         */
        return new ArrayObject($data, ArrayObject::STD_PROP_LIST);

    }

}

I have then extracted the specific logic regarding the required data and how the data is fetched to the class extending the UserDataSerializer.

class UserCustomer extends UserDataSerializer {

    public function __construct(array $data)
    {
        /*
         * We make a sequential array of required parameters for the
         * customer user type.
         */
        $required = ['id', 'firstname', 'lastname'];

        /*
         * Configure the parent class by declaring required data parameters
         * and providing the corresponding values.
         */
        parent::__construct($data, $required);

    }

    /*
     * Here you can implement specific methods for the
     * customer user type.
     */

}

class UserAddress extends UserDataSerializer {

    /*
     * Here the constructor take individual arguments and you can
     * decide which should be serialized and which of them should be
     * used for other purposes related to the class.
     */
    public function __construct($id, $address, $postcode, $otherInto)
    {
        /*
         * We make a sequential array of required parameters for the
         * address user type.
         */
        $required = ['id', 'address', 'postcode'];

        /*
         * We generate a data array of with the required parameters as keys
         * and the provided arguments as values.
         */
        $data = array_combine($required, [$id, $address, $postcode]);

        /*
         * Configure the parent class by declaring required data parameters
         * and providing the corresponding values.
         */
        parent::__construct($data, $required);

    }

    /*
     * Here you can implement specific methods for the
     * address user type.
     */

}

The usage is now a little different. In your code the passed data is fetched from an object, where my example uses an associative array. This is just because it is more convenient, when checking if all required parameters are present. Usage is as follows:

$serializer = new XMLSerializer(); // This implements the common Serializer interface.

$customer = new UserCustomer([
  'id'        => 123,
  'firstname' => 'awesomeness',
  'lastname'  => 'himself'
]);

$customer->serialize($serializer);

$serializer = new JSONSerializer(); // New serializer implementation.

$address = new UserAddress([
    'id'       => 123,
    'address'  => 'Cool Street 1',
    'postcode' => 'over 9000'
]);

$address->serialize($serializer);

Regarding your concern for using the same big data array.

  I'm sending the same big data array with a lot of 'waste' to each class

The first and properly the best way to avoid this, is to only pass the necessary information to the constructor like I have done in the UserAddress class. Usage would be:

$address = new UserAddress(
        $data->id,
        $data->address,
        $data->postcode,
        $data->otherInto
    );

This can also be changed by only extracting the required information for each class like you do. But as the data is set in the constructor we should take care not doing too much work inside it (it may already be too much for some). We could extract the gathering of only the necessary information into its own private method. I have named it prepare. I am also assuming the big data array is an object of type User, but it could be anything.

private function prepare(User $user)
{

    return [
        'id'       => $user->getId(),
        'address'  => $user->getAddress(),
        'postcode' => $user->getPostcode()
    ];

}

You must then alter your constructor. Here the User value object is passed as an argument.

public function __construct(User $user)
{
    /*
     * We make a sequential array of required parameters for the
     * address user type.
     */
    $required = ['id', 'address', 'postcode'];

    /*
     * We prepare the required data by extracting only the
     * necessary information from the user object.
     */
    $data = $this->prepare($user);

    parent::__construct($data, $required);

}

Other things

I have changed your naming convention to CamelCase.

I have changed the way data is fetched from the big data array to individual methods. This help ensure encapsulation of the data, by only providing setters for the properties which CAN be altered.

I have a bit of trouble understanding the reason to why the class UserAddress exist. To me an address consists of a string which could be a property inside a class. If you want more detail it could be an associative array where each element is given a value during instantiation of the class.

private $address = [
    'number'   => null,
    'street'   => null,
    'city'     => null,
    'postcode' => null,
    'country'  => null
];

I hope this can help guide you, happy coding!


Speech Recognition Part 3: Training the Neural Network The last part of my speech recognition series: finally training my network.  Here's the dataset I did it with (self-generated, small I know), and the code I used.

After running this code (takes about an hour on my Mac), I get a validation accuracy of roughly 30%... not spectacular.  Any ideas on how to improve the training speed, or the neural network's accuracy?  Any other suggestions in general?

import os
os.environ['TF_CPP_MIN_LOG_LEVEL']='2'
import numpy as np
import tflearn

def main():
    LABELED_DIR = 'labeled_data'
    width = 512
    height = 512
    classes = 26  # characters
    learning_rate = 0.0001
    batch_size = 25

    # load data
    print('Loading data')
    X, Y = tflearn.data_utils.image_preloader(LABELED_DIR, image_shape=(width, height), mode='folder', normalize=True, grayscale=True, categorical_labels=True, files_extension=None, filter_channel=False)
    X_shaped = np.squeeze(X)
    trainX, trainY = X_shaped, Y

    # Network building
    print('Building network')
    net = tflearn.input_data(shape=[None, width, height])
    net = tflearn.lstm(net, 128, dropout=0.8)
    net = tflearn.fully_connected(net, classes, activation='softmax')
    net = tflearn.regression(net, optimizer='adam', learning_rate=learning_rate, loss='categorical_crossentropy')

    model = tflearn.DNN(net, tensorboard_verbose=3)
    print('Training network')
    model.fit(trainX, trainY, validation_set=0.15, n_epoch=100, show_metric=True, batch_size=batch_size)
    model.save("tflearn.lstm.model")

if __name__ == '__main__':
    main()

First things first: you can get far better results by fine-tuning the arguments.

Yes, it goes over 52% at times and I'm sure it can go even higher. Let's take a look at what colours are caused by what.

  ORE1X4
  default settings
  EAPX5J
  Epoch 200 instead of 100
  6DHKQJ
  Dropout 0.9 instead of 0.8
  53O25D
  learning_rate 1e-3 instead of 1e-4
  0TWRK8
  256 instead of 128
  QCAZN8
  Epoch 300, dropout 0.9, learning_rate 1e-3, 256 instead of 128
  H57Z4I
  learning_rate 1e-3, epoch 400

I've rewritten 0.0001 as 1e-4, easier on the eyes. And you made a good start by putting it in a variable (which, according to the PEP8, should be CAPITAL_CASED since they're pseudo-constants). So why didn't you put the others in variables as well? Look at how the functions you call have named their arguments and use this as inspiration for your variable names.

Keep in mind you're using spectograms as input. Spectrograms have some downsides, since they only measure intensity of frequencies and not the phase. You may have heard this problem described as the phase problem. This means every spectrogram has broadband noise, impacting the overall effectiveness of your output. The measured effectiveness might not even be the real effectiveness, since it probably assumes you actually like the noise.

So, not only could you use more data to achieve a higher accuracy, you may eventually need more complete data. As in, less noise and with phase information.

As for performance, there's not much you can do. Your code runs significantly faster on my laptop than on your Mac (original set-up in under 15 minutes), even without using a GPU as acceleration. Tensorflow is pretty well optimized to use multiple cores.

Keep in mind the X-axis displays steps. The time it takes to reach a certain amount of steps can vary wildly depending on the arguments you provide. 0TWRK8 took 3 times as long to reach step 500 than H57Z4I, while the latter appears to be scoring better. Figure out which arguments are 'worth their weight' and which simply slow you down for little to no gain.

My advice? Experiment! After a couple hundred epochs the data will just about flatline, so going above 200 isn't particularly useful when going for sample runs.

Fidgeting with the input reminded me of a game I played a long while back: foldit

There's the early game, the mid game and the end game. In the early game, you're looking for the big changes. The later you get, the more your focus shifts to different aspects to fine-tune your approach. However, if you had an inefficient start, you couldn't fine-tune enough to reach the score you wanted. The score would flat-line.

Consider developing this machine in the same manner. Don't rush the development to make it go fast if that will hurt it's accuracy in the end. After all, nothing is as annoying as speech recognition that only works half the time. If you need certain functions to keep your output in good quality, don't optimize it away only to regret it later.

Something else your dataset doesn't take into account, is combinations of characters. ch isn't exactly pronounced as a combination of c  and h. The same goes for ph, th and other combinations. Keep this in mind when you start field testing your network.


Dynamically call lambda based on stream input: Try 3 Based on Dynamically call lambda based on stream input: Try 2

This fixes most of the points pointed out by @Morwenn.

It has been generalized so anybody can call it.
The only extra part needed by the code is a function to retrieve values from a stream. This is passed as a template argument to  callFunctor() see the example below.

An example of how to use it:
Here I use std::stringstream as the input to be used for this example. In my own version I have a DB socket streaming rows from a MySQL DB and the lambda is called for each row. The point is the current version of callFunctor() can be used in any general environment (I hope somebody finds it useful).

main.cpp

#include "detail_caller.h"
#include <iostream>
#include <sstream>

struct GetStreamValue
{
    // The input type to this method must match
    // the type of the first argument to `callFunctor`
    // As you will be passed the stream and asked to pull
    // a specific type from it.
    template<typename R>
    static R get(std::istream& stream)
    {
        R   result;
        stream >> result;
        return result;
    }
};

int main()
{
    std::stringstream       data("10 13 Loki 4.5");

    callFunctor<GetStreamValue>(
        data,
        [](int val1, int val2, std::string const& val3, float val4)
        {
            std::cout << "Got(" << val1 << ", "
                                << val2 << ", "
                                << val3 << ", "
                                << val4 << ")\n";
        }
    );
}

detail_caller.h

#ifndef THORSANVIL_DETAIL_CALLER_H
#define THORSANVIL_DETAIL_CALLER_H

#include <utility>

template <typename T>
struct CallerTraits
    : public CallerTraits<decltype(&T::operator())>
{};

template<typename C, typename ...Args>
struct CallerTraits<void (C::*)(Args...) const>
{
    static constexpr int                                        size = sizeof...(Args);
    typedef std::make_integer_sequence<int, size>               Sequence;
    typedef std::tuple<std::decay_t<Args>...>                   AllArgs;
};

template<typename StreamValueGetter, typename Action, typename Stream, int... S>
void expandArgsAndCallFunctor(Stream& stream, Action action, std::integer_sequence<int, S...>)
{
    typedef CallerTraits<decltype(action)>  Trait;
    typedef typename Trait::AllArgs         ArgumentTuple;

    // ArgumentTuple  arguments(StreamValueGetter::template get<typename std::tuple_element<S, ArgumentTuple>::type>(stream)...);
    // action(std::get<S>(arguments)...);

    // The above two commented out lines
    // Represent the logic of the following line which retrieves
    // and calls the arguments in a single line thus avoiding temporaries.
    action(StreamValueGetter::template get<typename std::tuple_element<S, ArgumentTuple>::type>(stream)...);
}

template<typename StreamValueGetter, typename Action, typename Stream>
void callFunctor(Stream& stream, Action action)
{
    typedef CallerTraits<decltype(action)>  Trait;
    typedef typename Trait::Sequence        Sequence;

    expandArgsAndCallFunctor<StreamValueGetter>(stream, action, Sequence());
}

#endif

I was about to add more things to my answer on your previous post and noticed meanwhile that your already asked a following question (that's almost too fast! :o). I have done some additional reading about C++14 and integer sequences. And I have found some things in the standard that could help to improve your code again.

I realized at some point that you were using int as the value_type of std::integer_sequence. However, you initialize your static constexpr int size with the result of sizeof... and sizeof... returns a std::size_t, not an int.

Then I remembered that, along with std::integer_sequence, there is a typedef for std::integer_sequence<std::size_t, N> which is called std::index_sequence and which has been made to solve the kind of problem that your are trying to solve. Therefore, you should every occurrence of int by std::size_t and of std::integer_sequence<int, N> by std::index_sequence<N>.

Moreover, you don't need to create a typedef for Sequence in CallerTraits: you can deduce this class directly in callFunctor thanks to CallerTraits<>::size:

template<typename StreamValueGetter, typename Action, typename Stream>
void callFunctor(Stream& stream, Action action)
{
    using Sequence = std::make_index_sequence<CallerTraits<Action>::size>;
    expandArgsAndCallFunctor<StreamValueGetter>(stream, action, Sequence());
}

Note that I also replaced decltype(action) by Action since it is the same type.

Also, you can get rid of the public inheritance specifier for CallerTraits since you are using a struct. That does not change anything whatsoever.

Here is your code once refactored. I don't provide a link to an online compiler since none of the ones available has a good enough C++14 support (you just need to add some light library components though):

template <typename T>
struct CallerTraits
    : CallerTraits<decltype(&T::operator())>
{};

template<typename C, typename ...Args>
struct CallerTraits<void (C::*)(Args...) const>
{
    static constexpr std::size_t                size = sizeof...(Args);
    typedef std::tuple<std::decay_t<Args>...>   AllArgs;
};

template<typename StreamValueGetter, typename Action, typename Stream, std::size_t... S>
void expandArgsAndCallFunctor(Stream& stream, Action action, std::index_sequence<S...>)
{
    typedef CallerTraits<Action>        Trait;
    typedef typename Trait::AllArgs     ArgumentTuple;

    // ArgumentTuple  arguments(StreamValueGetter::template get<typename std::tuple_element<S, ArgumentTuple>::type>(stream)...);
    // action(std::get<S>(arguments)...);

    // The above two commented out lines
    // Represent the logic of the following line which retrieves
    // and calls the arguments in a single line thus avoiding temporaries.
    action(StreamValueGetter::template get<std::tuple_element_t<S, ArgumentTuple>>(stream)...);
}

template<typename StreamValueGetter, typename Action, typename Stream>
void callFunctor(Stream& stream, Action action)
{
    using Sequence = std::make_index_sequence<CallerTraits<Action>::size>;
    expandArgsAndCallFunctor<StreamValueGetter>(stream, action, Sequence());
}


Removing whitespace in self-closing node and keeping UTF-8 hex entity values intact? I work with some xml files which have UTF-8 characters like &#x00A9;, &#x00EB; but they are converted to their image counterparts i.e. ©, ë, whenever I use XDocument to modify the file and also the self-closing nodes e.x.  become  i.e. an extra whitespace is added to the node, but I want to keep the entities like ©, ë etc as it is and also remove the extra whitespace in the self-closing nodes.

Currently I'm doing it in below fashion (sample example code)

var path = @"C:\Users\Temp\sample.xml";
var content=File.ReadAllText(path);
//replace all "&" with "&amp;" to prevent conversion of entity
var newcontent=content.Replace("&", "&amp;");
File.WriteAllText(path,newcontent);
XDocument doc = XDocument.Load(path, LoadOptions.PreserveWhitespace);
string _count = doc.Descendants("fig").Count().ToString();
doc.Descendants("fig-count").First().Value = _count;
doc.Save(path, SaveOptions.DisableFormatting);
//replace all "&amp" with "&" to go back to the original state and also replace all whitespace
//in the self-closing nodes after the XDocument process complets
var x=File.ReadAllText(path);
var y=x.Replace(" />", "/>");
File.WriteAllText(path,y.Replace("&amp;", "&"));

Can anyone show me a more efficient way of doing this?

You can do something like this (using your previously posted question code as demo)

Regex regex = new Regex(@"(?<=\\tag\s?\{)[^\}]+(?=\})");
string xml = escape_string(File.ReadAllText(@"C:\Users\Temp\Desktop\test.xml"), 0);
var xdoc = XDocument.Parse(xml, LoadOptions.PreserveWhitespace);
xdoc.DocumentType.InternalSubset = null;
foreach (var formulaNode in xdoc.Descendants("disp-formula"))
{
    var matches = regex.Matches(formulaNode.Value);
    if (matches.Count == 0)
    continue;

    var id = "deqn" + matches[0].Value;
    if (matches.Count > 1)
    id += "-" + matches[matches.Count - 1].Value;

    formulaNode.Attribute("id").Value = id;
}
xdoc.Save(@"C:\Users\Temp\Desktop\test.xml", SaveOptions.DisableFormatting);
File.WriteAllText(@"C:\Users\Temp\Desktop\test.xml", escape_string(xdoc.ToString(), 1));
}

private static string escape_string(string input_string, int option)
{
    switch (option)
    {
        case 0:
        return input_string.Replace("&", "&amp;");
        case 1:
        var x=input_string.Replace(" />", "/>");
        var y=x.Replace("&amp;", "&");
        var z=y.Replace("\r\n<!DOCTYPE","<?xml version=\"1.0\" encoding=\"utf-8\"?>\r\n<!DOCTYPE");
        return z;

        default:
        return null;

    }
}

The code xdoc.DocumentType.InternalSubset = null; is to stop the Parse() method to create a extra [] in the dtd declaration and also the xml declaration was getting deleted so I added that to a replace in the method escape_string.

Not sure whether it is faster than your posted method though :)

Code for operating on multiple files in a given path (assuming the xml files inside sub-folder 'xml' are only to be processed)

using System;
using System.IO;
using System.Text.RegularExpressions;
using System.Linq;
using System.Windows.Forms;
using System.Xml.Linq;

namespace test
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            string path = textBox1.Text;
            var files = Directory.GetDirectories(path, "xml", SearchOption.AllDirectories)
                .SelectMany(d => Directory.GetFiles(d, "*.xml")).ToArray();
            foreach (var file in files)
            {
                try
                {
                    Regex regex = new Regex(@"(?<=\\tag\s?\{)[^\}]+(?=\})");
                    string xml = escape_string(File.ReadAllText(file), 0);
                    var xdoc = XDocument.Parse(xml, LoadOptions.PreserveWhitespace);
                    xdoc.DocumentType.InternalSubset = null;
                    foreach (var formulaNode in xdoc.Descendants("disp-formula"))
                    {
                        var matches = regex.Matches(formulaNode.Value);
                        if (matches.Count == 0)
                            continue;

                        var id = "deqn" + matches[0].Value;
                        if (matches.Count > 1)
                            id += "-" + matches[matches.Count - 1].Value;

                        if (formulaNode.Attribute("id") != null)
                        {
                            formulaNode.Attribute("id").Value = id;
                        }
                        formulaNode.SetAttributeValue("id", id);

                    }
                    xdoc.Save(file, SaveOptions.DisableFormatting);
                    File.WriteAllText(file, escape_string(xdoc.ToString(), 1));
                }
                catch (Exception ex)
                {
                    string filePath = path + @"\Error.log";
                    using (StreamWriter writer = new StreamWriter(filePath, true))
                    {
                        writer.WriteLine(file + ":\r\n---------------------------------------------------\r\n" + ex.Message);
                    }
                }
            }

            MessageBox.Show("Process Complete!");

        }

        private static string escape_string(string input_string, int option)
        {
            switch (option)
            {
                case 0:
                    return input_string.Replace("&", "&amp;");
                case 1:
                    var x = input_string.Replace(" />", "/>");
                    var y = x.Replace("&amp;", "&");
                    var z = y.Replace("\r\n<!DOCTYPE", "<?xml version=\"1.0\" encoding=\"utf-8\"?>\r\n<!DOCTYPE");
                    return z;

                default:
                    return null;

            }
        }
    }
}


Longest substring with at most 2 distinct characters I think I solved the following problem. However, I was wondering if there was a faster or more efficient way to solve this.

I believe the the runtime is O(n) and space is O(1)  (O(256) assuming ASCII)

# Longest substring with at most 2 distinct characters
#
# Given a string S, find the length of the longest substring T that contains at most 2 distinct characters
#
# For example,
#     Given S = "eceba"
#     T is "ece" which it's length is 3

def longest_substring(string):
    if string is None or len(string) == 1 or len(string) == 0:
        return 0
    distinct_set = set()
    longest = 0
    current = 0
    for i in string:
        set_len = len(distinct_set)
        if set_len < 2:
            if i in distinct_set:
                current += 1
            if i not in distinct_set:
                distinct_set.add(i)
                current += 1
        if set_len == 2:
            if i in distinct_set:
                current += 1
            if i not in distinct_set:
                distinct_set.clear()
                distinct_set.add(i)
                if current > longest:
                    longest = current
                current = 1
        if current > longest:
            longest = current
    return longest

Input:

test_cases = [None, "", "a", "ab", "aab", "bab", "babc", "bbbbcccac"]

Output:

0, 0, 0, 2, 3, 3, 3, 7

Three small things:

Firstly, docstrings exist. Use them. Put the explanation for how to use your method in them. That means that instead of this:

# Longest substring with at most 2 distinct characters
#
# Given a string S, find the length of the longest substring T that contains at most 2 distinct characters
#
# For example,
#     Given S = "eceba"
#     T is "ece" which it's length is 3

def longest_substring(string):

you have this:

def longest_substring(string):
    """ Longest substring with at most 2 distinct characters
    Given a string S, find the length of the longest substring T that contains t most 2 distinct characters
    For example,
        Given S = "eceba"
        T is "ece" which it's length is 3"""

At least, if I've understood their purpose correctly.

Secondly, if someone passes in None, that should probably be an error, not a return 0 case. Same if someone passes in a number, Duck, or any other invalid type.

Thirdly, according to PEP8, the official style guide, none of your lines should be longer than 80 characters. The (new) second line violates this; it even did before. Just split it in half and you're good.


Simplifying Rails Validation I have four fields a, b, c, and d.  All of them must be populated if any of them have a value.

So what I want to do is validate that they are all populated, if any of them have a value.  I'm trying to determine the best way to do this.  I think I could do this:

  class User < ActiveRecord::Base
    validates_presence_of :a, :b, :c, :if => lambda { self.d.present? }
    validates_presence_of :b, :c, :d, :if => lambda { self.a.present? }
    validates_presence_of :a, :c, :d, :if => lambda { self.b.present? }
    validates_presence_of :a, :b, :d, :if => lambda { self.c.present? }
  end

I noticed that you use 'hard' and repeated parts of code. My way to solve this task is to write a custom validation (I try to avoid them but sometimes there no other choice).

I try to place custom validations in a separate file (in config\initializers\ directory).

ActiveRecord::Base.class_eval do
  def self.validates_dependent_fields(*attr_names)
    fields_count = 0
    attr_names.each{ |attribute| fields_count += 1 if record[attribute] }

    validates_each(attr_names) do |record, attribute, value|
      if value && fields_count < attr_names.length
        record.errors[attribute] << "Dependent fields can not be empty."
      end
    end

  end
end

Algorithm:

Get the number of not nil fields;
Validate each field (and add error message if the attribute value is not nil and one or more dependent fields are not present).

Then, in the model:

class SomeModel < ActiveRecord::Base
  validates_dependent_fields :a, :b, :c, :d
end

Note that you can reuse this validation in any other model.

P.S.: I've not tested this code yet.


Median Calculation of List of Integers without using heap References

Using those references:

rolling median implementations benchmark
FAST RUNNING MEDIAN USING AN INDEXABLE SKIPLIST
Calculate the median of a billion numbers
Find running median from a stream of integers

Code

A code was written to calculate the Median with the SortedContainers library:

from itertools import islice
from sortedcontainers import SortedList
import random
import time

start_time = time.time()

class Median(object):

    def __init__(self, iterable):
        self._iterable = islice(iterable, None)
        self._sortedlist = SortedList(self._iterable)

    def __iter__(self):
        self_sortedlist = self._sortedlist
        # print(self_sortedlist)
        length = len(self_sortedlist)
        half = length // 2
        if length % 2 == 0:
            yield (self_sortedlist[half] + self_sortedlist[half - 1]) // 2
        elif length % 2 == 1:
            yield self_sortedlist[half]

def main():
    m, n = 1000, 1500000
    data = [random.randrange(m) for i in range(n)]
    # print("Random Data: ", data)
    result = list(Median(data))
    print("Result: ", result)

if __name__ == "__main__":

    main()
    print("--- %s seconds ---" % (time.time() - start_time))

Explanation

Random Number Generator

The following code generates data within the Range m and quantity n.

m, n = 1000, 15000000
data = [random.randrange(m) for i in range(n)]

Median

The Median class sorts the list of numbers and if n is odd, returns the middle item with yield self_sortedlist[half]. Or if n is even, returns the mean of two middle itens of the list with yield (self_sortedlist[half] + self_sortedlist[half - 1]) // 2

Question

How do I improve the code performance? Because for a large list (100 milion), it takes --- 186.7168517112732 seconds--- on my computer.

Performance
I instrumented your code differently:
def main():
    m, n = 1000, 15000000
    start_time = time.time()
    data = [random.randrange(m) for i in range(n)]
    print("--- %s seconds to generate" % (time.time() - start_time))
    # print("Random Data: ", data)
    start_time = time.time()
    result = SortedList(data)
    print("--- %s seconds to sort" % (time.time() - start_time))
    start_time = time.time()
    result = list(Median(data))
    print("Result: ", result)
    print("--- %s seconds ---" % (time.time() - start_time))

For me, the results were:
--- 7.407598257064819 seconds to generate
--- 4.535749673843384 seconds to sort
Result:  [500]
--- 5.01109504699707 seconds ---

This shows that the generation of the random input takes longer than finding the median, and that 90% of Median() is spent sorting (probably most of the rest is caused by converting between lists and iterators).  You're unlikely to get large gains through modifications to your own part of the code.
I got better results by using Python's built-in sorted().  We don't need any of the extra functionality of SortedList (maintaining the invariant through adds and deletes), and a single sort (mostly in native code) gives these results:
def median(values):
    sortedlist = sorted(values)
    length = len(sortedlist)
    half = length // 2
    # return not yield; see "General review"
    if length % 2 == 0:
        return (sortedlist[half] + sortedlist[half - 1]) // 2
    else:
        return sortedlist[half]

def main():
    m, n = 1000, 15000000
    start_time = time.time()
    data = [random.randrange(m) for i in range(n)]
    print("--- %s seconds to generate" % (time.time() - start_time))
    # print("Random Data: ", data)
    start_time = time.time()
    result = sorted(data)
    print("--- %s seconds to sort" % (time.time() - start_time))
    start_time = time.time()
    result = median(data)
    print("Result: ", result)
    print("--- %s seconds ---" % (time.time() - start_time))

--- 7.638948202133179 seconds to generate
--- 3.118924617767334 seconds to sort
Result:  500
--- 3.3397886753082275 seconds ---

General review
I don't see why you yield a single value, rather than simply returning.  Is there a need for Median() to be iterable?
Given that length is an integer, if length % 2 is not 0, it must be 1 - so the elif could easily be else.


Calculating pi to 7 significant figures without using Math.PI I need to calculate pi to 7 significant figures in Java—without using Math.PI.

Here is the code I came up with to do that:

public class ComputePI
{
    public static void main(String[] args)
    {
        double sum = 0.0;
        double sumOne = 0.0;
        double delta;
        int counter = 0;
        final double DENOMINATOR_CANCEL = 4.0;
        final int LARGE_NUMBER = 5000000;
        final double SMALLEST_DELTA = 0.0000004535899;
        boolean closeEnough = false;

        for (int j = 1; j < LARGE_NUMBER; j += 2) // Computes number close to pi
        {
            double firstFrac = (1.0 / (j * 2.0 - 1.0));
            double secondFrac = (1.0 / (j * 2.0 + 1.0));
            sumOne += firstFrac - secondFrac;
        }

        for (int i = 1; (!closeEnough); i += 2)   //"My computed value of pi"
        {
            double firstNum = (1.0 / (i * 2.0 - 1.0));
            double secondNum = (1.0 / (i * 2.0 + 1.0));
            sum += firstNum - secondNum;
            delta = sumOne * DENOMINATOR_CANCEL - sum * DENOMINATOR_CANCEL;
            if (delta < SMALLEST_DELTA)     // If delta reaches 7-sig accuracy
            {
                closeEnough = true;         // End loop
            }
            counter = i / 2;        // Counts iterations
        }

                        /*  output results      */
        System.out.println("My computed value of pi is: " + sum * DENOMINATOR_CANCEL);
        System.out.println("The library constant value of pi is: " + Math.PI);
        System.out.println("The number of iterations needed to reach " + "seven-significant digit accuracy is: " + counter);
    }
}

Is this a good approach, or is it horribly inefficient? How might I improve it?

Perhaps having a for loop inside a for loop would work better. One with n+1 iterations, the other with n iterations. The loop would terminate if the two sums were subtracted, and produced a difference of 0.0000009 or less (7 significant figure accuracy).

Your implementation is fine. You could add some tweaks (i += 4 instead of i += 2) to avoid multiplying i by 2.0, but this won't help much to make your program fast.

This is because you chose a simple formula, but that formula converges very slowly towards \$\pi\$.

Read on Wikipedia about efficient algorithms for computing \$\pi\$, there are several.

Or, if you want to cheat a bit, just return 4 * Math.atan(1.0).

You don't need a delta in the whole program, so only declare it where you really need it. You already did that with firstFrac, for example.

Be consistent with your chosen names. What's the difference between firstFrac and firstNum? — There is none, therefore you should use the same name in both places.

What does sumOne mean? Think of a better name for it.


Why is my Python web scraper taking so long? I am trying to create a program to download a long list of websites using Python 3.7. By using multiprocessing for each request, my code runs much faster than when it's run synchronously.

I've added a timer to my script so I can know the execution time for the script. The one I wrote below takes on average approximately 3.5 seconds to run. However, when I use this script for my long list of URL's, it takes approximately 8 minutes to execute.

I am wondering: is it taking this long because my internet is slow? Or is my script just bad?

My internet speed is pretty low (3 mb/s download, 1 mb/s upload). If anyone has faster internet, could you post your execution time for this script along with your internet speed? This way, I can see if the bottleneck is internet speed or not.

If the bottleneck isn't internet speed, but simply that my script is bad, can anyone offer any suggestions for improving the script so that the execution time is better? Thanks coders!

from urllib.request import Request, urlopen
from multiprocessing.dummy import Pool as ThreadPool
import datetime
import time
import requests

start_time = time.time()

urls = [
  'https://en.wikipedia.org/wiki/Python_(programming_language)',
  'https://www.python.org/',
  'https://stackoverflow.com/questions/tagged/python',
  'https://github.com/python',
  'https://realpython.com/python-beginner-tips/',
  'https://pythonprogramming.net/introduction-to-python-programming/',

  ]

mylist = []

def my_function(url_to_parse):
  req = Request(url_to_parse, headers={'User-Agent': 'Mozilla/5.0'})
  resp = urlopen(req).read()
  mylist.append(resp)

#------------------------------------------------------------------------------------------

pool = ThreadPool(8)
results = pool.map(my_function, urls)
pool.close()
pool.join()

print("--- %s seconds --- REQUESTS" % (time.time() - start_time))
```

serializing
You are serializing all HTML text and sending it back to the parent.
That can be time consuming.
Consider having the child (my_function()) write HTML text to a file in /tmp,
and pass back the temporary filename to the parent.
Better, also pass back some timestamps: (start, end, filename)
stragglers
If you can predict that a page will be "big" or a site will be "slow",
sort those to the front of your list of 500 URLs.
That way you won't have a handful of sluggish stragglers
impacting your overall time,
while most cores are idle and waiting for them to complete.


Jinja2 template loader I am playing around with building my own back end with python 2.7 and jinja2/cgi. Originally I was defining some of my environment and loader variables at the top but figured this was not the best practice. I then defined functions for each, but they'd only get called once or twice depending on which route it decided to execute. I came up with this class idea instead that I pass into my dispatch function, which in turn passes it into whatever rendering function happens to be being executed. Is this the proper 'pythonic' way to approach having a template loader, and if not would anyone have any suggestions?

class Template(object):
    def __init__(self):
        self._environment = None

    def __call__(self, temp_name):
        return self.get_env.get_template(temp_name)

    @property
    def get_env(self):
        return self._environment

    @get_env.getter
    def get_env(self):
        if not self._environment:
            self._loader = jinja2.FileSystemLoader(searchpath=os.path.join(
              os.path.dirname(os.path.abspath(__file__)), '../templates/madlib/'))
            self._environment = jinja2.Environment(loader = self._loader)

        return self._environment

This class then allows me to just call template('templateFileName.j2') and be returned that jinja template object. If the environment is not created, the getter will create one for me.

For starter, I'd rewrite get_env a bit and introduce intermediate variables for readability:

def get_env(self):
    if not self._environment:
        cwd = os.path.dirname(os.path.abspath(__file__))
        madlib_dir = os.path.join(cwd, '..', 'templates', 'madlib')
        loader = jinja2.FileSystemLoader(searchpath=madlib_dir)
        self._environment = jinja2.Environment(loader=loader)
    return self._environment

Next I would cleanup your @property usage; since the raw decorator already defines the getter property, you can get rid of the first method:

class Template(object):
    def __init__(self):
        self._environment = None

    def __call__(self, temp_name):
        return self.get_env.get_template(temp_name)

    @property
    def get_env(self):
        if not self._environment:
            cwd = os.path.dirname(os.path.abspath(__file__))
            madlib_dir = os.path.join(cwd, '..', 'templates', 'madlib')
            loader = jinja2.FileSystemLoader(searchpath=madlib_dir)
            self._environment = jinja2.Environment(loader=loader)
        return self._environment

Lastly, since the initialization of _environment is meant to be done (and thus checked) only once, why not take advantage of attribute access that already offer us this kind of flexibility:

class Template(object):
    def __call__(self, temp_name):
        return self.environment.get_template(temp_name)

    def __getattr__(self, attribute):
        if attribute == 'environment':
            cwd = os.path.dirname(os.path.abspath(__file__))
            madlib_dir = os.path.join(cwd, '..', 'templates', 'madlib')
            loader = jinja2.FileSystemLoader(searchpath=madlib_dir)
            self.environment = env = jinja2.Environment(loader=loader)
            return env
        return super(Template, self).__getattr__(attribute)

This way, __getattr__ is only called the first time self.environment is accessed and never again. But... Hold on... The only usefull method of this class is __call__... Better write a function instead; this will bring the check back at each call but will make the intent clearer:

def load_template(template_name):
    if not hasattr(load_template, 'environment'):
        cwd = os.path.dirname(os.path.abspath(__file__))
        madlib_dir = os.path.join(cwd, '..', 'templates', 'madlib')
        loader = jinja2.FileSystemLoader(searchpath=madlib_dir)
        load_template.environment = jinja2.Environment(loader=loader)
   return load_template.environment.get_template(template_name)

So now, no need to pass an object around everywhere, just call the function when you need to.

And if you want to, EAFP can also help reduce the overhead of the hasattr test:

def load_template(template_name):
    try:
        environment = load_template.environment
    except AttributeError:
        cwd = os.path.dirname(os.path.abspath(__file__))
        madlib_dir = os.path.join(cwd, '..', 'templates', 'madlib')
        loader = jinja2.FileSystemLoader(searchpath=madlib_dir)
        load_template.environment = environment = jinja2.Environment(loader=loader)
   return environment.get_template(template_name)


Recursive merge sort in C++ Any suggestions on how to improve this code (other than arbitrary call-stack depth)?

#include <iostream>
#include <vector>

template <typename T>
void merge(const std::vector<T>& left, const std::vector<T>& right, std::vector<T>& merged)
{
    auto i = left.begin();
    auto j = right.begin();
    auto k = merged.begin();

    while (i != left.end() && j != right.end())
    {
        *k = (*i < *j) ? *i : *j;
        if (*i < *j) ++i;
        else ++j;
        ++k;
    }
    while (j != right.end())
    {
        *k = *j; ++k; ++j;
    }
    while (i != left.end())
    {
        *k = *i; ++k; ++i;
    }
}

template <typename T>
void merge_sort(std::vector<T>& A)
{
    if (A.size() <= 1) return;
    size_t mid = A.size() / 2;
    std::vector<T> left(A.begin(), A.begin() + mid);
    std::vector<T> right(A.begin() + mid, A.end());
    merge_sort(left);
    merge_sort(right);
    merge(left,right, A);
}

int main()
{
    std::vector<int> input = {19, 14, 17, 16, 12, 9, 15, 1, 2, 11, 7, 3, 10, 14};
    std::vector<int> sorted = {1, 2, 3, 7, 9, 10, 11, 12, 14, 14, 15, 16, 17, 19};
    bool success = true;
    merge_sort(input);
    for (size_t i = 0; i < input.size(); ++i) if ( input[i] != sorted[i] ) success = false;
    std::cout << "Merge sort " << (success ? "passed\n" : "failed\n");
}

There is more efficient and idiomatic ways of implementing merge sort, yet I will assume your style. I have embedded my comments directly in your code whenever I have something to say:

#include <iostream>
#include <vector>
#include <algorithm>

template <typename T>
void merge(const std::vector<T>& left_vector,
           const std::vector<T>& right_vector,
           std::vector<T>& merged_vector)
{
    /*
    auto i = left.begin();
    auto j = right.begin();
    auto k = merged.begin();
    */
    // Better names + names for end iterators in order not to call end() on each
    // iteration:
    auto left      = left_vector.begin();
    auto right     = right_vector.begin();
    auto merged    = merged_vector.begin();
    auto left_end  = left_vector.end();
    auto right_end = right_vector.end();

    /*
    while (i != left.end() && j != right.end())
    {
        *k = (*i < *j) ? *i : *j;
        if (*i < *j) ++i;
        else ++j;
        ++k;
    }*/
    // Would be more efficient since you compare only once per iteration:
    while (left != left_end && right != right_end)
    {
        if (*right < *left)
        {
            *merged = *right;
            ++right;
        }
        else
        {
            *merged = *left;
            ++left;
        }

        ++merged;
    }

    /*
    while (j != right.end())
    {
        *k = *j; ++k; ++j;
    }
    while (i != left.end())
    {
        *k = *i; ++k; ++i;
    }*/
    // Here you could use std::copy. Only one of these two calls will have an
    // effect, since either left < left_end or right < right_end:
    std::copy(left, left_end, merged);
    std::copy(right, right_end, merged);
}

template <typename T>
void merge_sort(std::vector<T>& A)
{
    if (A.size() <= 1) return;
    size_t mid = A.size() / 2;
    std::vector<T> left(A.begin(), A.begin() + mid);
    std::vector<T> right(A.begin() + mid, A.end());
    merge_sort(left);
    merge_sort(right);
    merge(left,right, A);
}

int main()
{
    std::vector<int> input = {19, 14, 17, 16, 12, 9, 15, 1, 2, 11, 7, 3, 10, 14};
    std::vector<int> sorted = {1, 2, 3, 7, 9, 10, 11, 12, 14, 14, 15, 16, 17, 19};
    /*
    merge_sort(input);
    bool success = true;
    merge_sort(input);
    for (size_t i = 0; i < input.size(); ++i) if ( input[i] != sorted[i] ) success = false;
    std::cout << "Merge sort " << (success ? "passed\n" : "failed\n");
     */
    // You can write better:
    merge_sort(input);
    std::cout << "Merge sort passed: "
              << std::boolalpha
              << std::equal(input.begin(),
                            input.end(),
                            sorted.begin(),
                            sorted.end())
              << std::endl;
}

Hope that helps.


Get bibtex entries from metadata of PDF files When I have to write a report or an article, I usually have my bibliography as PDFs in a specific folder, so I wrote this script to automate the generation of bibtex entries.

It works so far, but I'd like to have suggestions about what I could improve

#!/usr/bin/env bash

FOLDER=$1
BIBTEX_FILE=$2

cd $FOLDER
for f in *; do
    echo "Examining $f"
    TITLE=`pdfinfo $f | grep Title | cut -c17-`

    echo "Title found in PDF metadata is : $TITLE"

    case "$TITLE" in
        doi*)
            echo "This is a DOI, nice!"
            DOI=`echo $TITLE | cut -c5-`;;
        *)
            case "$TITLE" in
                "untitled")
                    echo "Let's see what we can find in the text"
                    DOI=`pdftotext $f /tmp/pdf | cat /tmp/pdf | grep -i 'doi\|Digital Object Identifier' | awk 'NF>1{print $NF}'`;;
                *)
                    echo "Let's find a DOI from this title"
                    TITLE=`echo $TITLE | sed 's/ /%20/g'`
                    URL="http://api.crossref.org/works?query=$TITLE&rows=1"
                    DOI=`curl -s $URL | jq '.message.items[].DOI' | sed 's/"//g'`;;
            esac
    esac

    echo "The DOI is $DOI"
    echo "----------------------------------"
    URL="http://api.crossref.org/works/$DOI/transform/application/x-bibtex"
    curl -s $URL >> $BIBTEX_FILE
    echo >> $BIBTEX_FILE
    echo >> $BIBTEX_FILE
done

Always double-quote variables if they might contain spaces

These commands will fail if the variables contain spaces:

cd $FOLDER

# ...

echo >> $BIBTEX_FILE

Even if you never intend to use your script with paths containing spaces,
it's good to make it a habit to double-quote variables that might contain spaces:

cd "$FOLDER"

# ...

echo >> "$BIBTEX_FILE"

Error checking

What will happen if this command fails?

cd $FOLDER

If this fails, the program will happily continue,
download a likely invalid url with curl,
and create a bogus $BIBTEX_FILE,
essentially putting garbage in the current directory.

Look for possible points of failure where the program should abort.
If something goes wrong with this cd command you definitely don't want execution to continue.
Here's a simple way to abort:

cd "$FOLDER" || exit 1

Here's a more user-friendly way:

if ! cd "$FOLDER"; then
    echo "error: could not cd into $FOLDER"
    exit 1
fi

Use the modern $(...) instead of the obsolete `...`

For example:

TITLE=$(pdfinfo "$f" | grep Title | cut -c17-)

Notice that I double-quoted $f, as it should be.

Bogus pipeline

This is strange:

pdftotext $f /tmp/pdf | cat /tmp/pdf | ...

In a pipeline, usually the output of one command is passed as input to the next through stdout and stdin file handles. That's not what happens here, data is passed through a file. This is unusual and confusing.
Also confusing is to name a text file /tmp/pdf.

You don't need a temporary file here,
pdftotext can produce output on stdout,
and then you don't need cat either:

pdftotext $f - | ...

Removing double-quotes from the output of jq

Instead of this:

curl -s $URL | jq '.message.items[].DOI' | sed 's/"//g'

You can remove the double-quotes by using the --raw-output flag:

curl -s "$URL" | jq -r '.message.items[].DOI'

-r is a shortcut for --raw-output.
Notice also, once again, that I added the necessary double-quotes around $URL.

Redirecting multiple statements

This kind of repetition quickly becomes annoying:

curl -s $URL >> $BIBTEX_FILE
echo >> $BIBTEX_FILE
echo >> $BIBTEX_FILE

You can dispense with that using grouping:

{
    curl -s "$URL"
    echo
    echo
} >> "$BIBTEX_FILE"


Paginated method to get all items I'm using slim php and i have this simple paginated end point to get all clients, i feel like i could do some things here better, so i would appreciate all suggestions, if possible please give me code example with suggested improvements. Thank you in advance.

$app->get('/api/clients', function ($request, $response, $args) {
    require_once 'dbConnect.php';
    require_once 'shared/securityService.php';
    $queryParams = $request->getQueryParams();

    $total = getTotalCount($mysqli, $queryParams["group"]);
    $pageSize = $queryParams["pageSize"];

    $totalPages = ceil($total / $pageSize);
    $offset = ($queryParams["page"] - 1)  * $pageSize;

    if(is_numeric($queryParams["group"])) {
        $stmt = $mysqli->prepare("SELECT id, firstName, lastName, birthDate, phoneNumber, address,
            contactPerson, contactPersonPhoneNumber FROM client WHERE groupId = ? ORDER BY
            id LIMIT $pageSize OFFSET $offset");

        $stmt->bind_param("i", $queryParams["group"]);
    } else {
        $stmt = $mysqli->prepare("SELECT id, firstName, lastName, birthDate, phoneNumber, address,
            contactPerson, contactPersonPhoneNumber FROM client
            ORDER BY id LIMIT $pageSize OFFSET $offset");
    }

    $stmt->execute();
    $stmt->bind_result($id, $firstName, $lastName, $birthDate, $phoneNumber,
        $address, $contactPerson, $contactPersonPhoneNumber);

    $data = null;

    while ($stmt->fetch()) {
        $data['items'][] = array(
            'id' => $id,
            'firstName' => $firstName,
            'lastName' => $lastName,
            'birthDate' => $birthDate,
            'phoneNumber' => $phoneNumber,
            'address' => $address,
            'contactPerson' => $contactPerson,
            'contactPersonPhoneNumber' => $contactPersonPhoneNumber
        );
    }

    $data['totalItems'] = $total;

    $stmt->close();
    $mysqli->close();
    echo json_encode($data);
});

function getTotalCount($mysqli, $groupId) {
    if(is_numeric($groupId)) {
        $totalStmt = $mysqli->prepare("SELECT COUNT(id) FROM client WHERE groupId = ?");
        $totalStmt->bind_param("i", $groupId);
    } else {
        $totalStmt = $mysqli->prepare("SELECT COUNT(id) FROM client");
    }

    $totalStmt->execute();
    $totalStmt->bind_result($total);
    $totalStmt->fetch();
    $totalStmt->close();
    return $total;
}

SQL Injection

You are vulnerable to SQL injection via pageSize.

You need to use prepared statements for all values, not just some of them.

Naming

total and getTotalCount could be more descriptive. Currently, I don't really know what "total" means without looking at the code of the function. You also don't need "total" at all (it's assumed when talking about a count). $clientCount = getClientCountForGroup($mysqli, $id) would be easier to understand.

Duplication

Your two queries are basically duplicates apart from the WHERE clause. You may want to remove that duplication.


Loading html from file using JavaScript Today I was trying to load navigation bar from another HTML page. Here is code, that I am using. What do you think about it? Is there any other way to do it?

var x = document.getElementsByClassName("PageLoader");
var url = "";

for (var i = 0;i<x.length;i++){
    url = x[i].dataset.url;
    var xmlHttp = new XMLHttpRequest();
    xmlHttp.open( "GET", url, false );
    xmlHttp.send( null );
    x[i].innerHTML = xmlHttp.responseText;
}

I have some issues with your code and possible alternatives for implementation.

Usage of var

In Javascript using var instead of let or const are somewhat discouraged (in the new era of ES6). There is some value in block scope for loops using var but you can certainly always use ES6' let instead. Since you are not reassigning xmlHttp you might as well const that (same goes for x and let for url).

Use fetch instead of XMLHttpRequest

Fetch comes with a promise-ready easier API for handling requests. CanIUse reports that it's almost supported on every new Browser as well. A polyfill should be enough to support the rest of the browsers out there. It also helps in readability and simplicity.

In your case, getting HTML out of a requests is a simple as:

fetch(...)
  .then(function (response) {
    // Status 200? Type text/html? Might aswell check that here.
    return response.text();
  })
  .then(function (body) {
    // Insert into dom here...
    console.debug(body);
  })

Error handling thanks to fetch

What happens if your URL is faulty, temporarily unavailable and some other error? If you need error handling then just add a catch below then:

.catch(e){
  // Display message, console silenty
  console.debug(e);
}

Using external libraries is usually not necessary as ES6 brings the most bang for the buck anyway.


Image downloader for a website This code takes a website and downloads all .jpg images in the webpage. It supports only websites that have the <img> element and src contains a .jpg link.

(Tested here)

import random
import urllib.request
import requests
from bs4 import BeautifulSoup

def Download_Image_from_Web(url):
    source_code = requests.get(url)
    plain_text = source_code.text
    soup = BeautifulSoup(plain_text, "html.parser")
    raw_text = r'links.txt'
    with open(raw_text, 'w') as fw:
        for link in soup.findAll('img'):
            image_links = link.get('src')
            if '.jpg' in image_links:
                for i in image_links.split("\\n"):
                    fw.write(i + '\n')
    num_lines = sum(1 for line in open('links.txt'))
    if num_lines == 0:
        print("There is 0 photo in this web page.")
    elif num_lines == 1:
        print("There is", num_lines, "photo in this web page:")
    else:
        print("There are", num_lines, "photos in this web page:")
    k = 0
    while k <= (num_lines-1):
        name = random.randrange(1, 1000)
        fullName = str(name) + ".jpg"
        with open('links.txt', 'r') as f:
            lines = f.readlines()[k]
            urllib.request.urlretrieve(lines, fullName)
            print(lines+fullName+'\n')
        k += 1

Download_Image_from_Web("https://pixabay.com")

Unnecessary file operations

This is horribly inefficient:

k = 0
while k <= (num_lines-1):
    name = random.randrange(1, 1000)
    fullName = str(name) + ".jpg"
    with open('links.txt', 'r') as f:
        lines = f.readlines()[k]
        urllib.request.urlretrieve(lines, fullName)
        print(lines+fullName+'\n')
    k += 1

Re-reading the same file num_lines times, to download the k-th!

Btw, do you really need to write the list of urls to a file?
Why not just keep them in a list?
Even if you want the urls in a file,
you could keep them in a list in memory and never read that file, only write.

Code organization

Instead of having all the code in a single function that does multiple things,
it would be better to organize your program into smaller functions,
each with a single responsibility.

Python conventions

Python has a well-defined set of coding conventions in PEP8,
many of which are violated here.
I suggest to read through that document,
and follow as much as possible.


Christmas Tree in Kotlin Please review my code for printing a Christmas Tree.
The idea was to merge some functional programming ideas (immutability, streams) and OOP (every thing is a small object).

I'm looking for a review focused on readability of the code.

Here is a output from the code:

    *
   ***
  *****
 *******
*********

And the code:

fun main(a: Array<String>) {
    print(Tree(height = 5))
}

class Tree(val height: Int) {
    override fun toString(): String {
        return (0..height - 1)
                .map { i ->
                    Stage(i, height)
                }
                .joinToString(
                        separator = "\n",
                        postfix = "\n"
                )
    }
}

class Stage(val idx: Int, val height: Int) {
    override fun toString(): String {
        val trunkWidth = TrunkWidth(idx)
        val margin = Margin(MarginWidth(TreeWidth(height), trunkWidth).value())
        return listOf(
                margin,
                Trunk(trunkWidth.value()),
                margin
        ).joinToString(separator = "")
    }

}

class MarginWidth(val tree: TreeWidth, val trunk: TrunkWidth) {
    fun value(): Int {
        return (tree.value() - trunk.value()) / 2
    }
}

class TreeWidth(val height: Int) {
    fun value(): Int {
        return height * 2 - 1;
    }
}

class TrunkWidth(val idx: Int) {
    fun value(): Int {
        return 1 + 2 * idx
    }
}

class Trunk(val size: Int) {
    override fun toString(): String {
        return UniformString(size, "*").toString()
    }
}

class Margin(val size: Int) {
    override fun toString(): String {
        return UniformString(size, " ").toString()
    }
}

class UniformString(val size: Int, val char: String) {
    override fun toString(): String {
        return (0..size - 1)
                .map { char }
                .joinToString(separator = "")
    }
}

"args" is more common and descriptive than "a" in a main function signature
"\n" is system-dependent; I recommend using System.lineSeparator() instead
joinToString can be used with a transform instead of first mapping an Iterable using a transform and then calling joinToString. e.g.:

(0..height - 1).map { i -> Stage(i, height) }.joinToString()

can be written as

(0..height - 1).joinToString { i -> "${Stage(i, height)}" }

which will avoid creating an intermediary list (small optimization in this case).
Perhaps a minor issue but I personally prefer using "index" instead of "i" or "idx" to avoid any ambiguity. "idx" actually has several meanings.
I find buildString used with repeat to often be more readable than Iterable combined with joinToString:

buildString { repeat(height) { index -> appendln(Stage(index, height)) } }

I also find buildString to be more suitable than joinToString(separator = ""):

return buildString {
    append(margin)
    append(Trunk(trunkWidth.value()))
    append(margin)
}

If you are going to have classes for computed numbers than I find toInt (like AtomicInteger.toInt) much more readable than value for MarginWidth, TreeWidth, etc.
Naming an instance of TreeWidth as simply "tree" is rather confusing, I recommend using type names (at least the end of them) at the end of instance names (e.g. "stageTreeWidth", "stageWidth", "treeWidth", or simply "width").
Kotlin has a neat String.repeat function to simplify building strings with a repeated char sequence. e.g.:

"*".repeat(size)

Technically a "trunk" does not include the branches, etc. so "TrunkWidth" and "Trunk" are kind of confusing names for this. You might consider "LayerWidth" and "Layer".
I see very little point to all these classes as they do not serve much purpose. A class with a single method to define its single behavior might as well be a method. If Stage, MarginWidth, Margin, etc. will be used by methods other than Tree.toString then it might make sense to have these as classes. If they are simply to build a string representation then I would drop them in favor of inline calculations (OOP does not mean that "every thing is a small object"; e.g. "height" and "width" may be attributes of objects but are not necessarily objects themselves).

This entire collection of classes can be reduced to the following (assuming no other code is using Stage, Margin, etc.):

class Tree(val height: Int) {
    override fun toString(): String {
        return buildString {
            val treeWidth = height * 2 - 1
            repeat(height) { index ->
                val layerWidth = 1 + 2 * index
                val marginWidth = (treeWidth - layerWidth) / 2
                val marginString = " ".repeat(marginWidth)
                val layerString = "*".repeat(layerWidth)
                append(marginString)
                append(layerString)
                append(marginString)
                appendln()
            }
        }
    }
}

If Tree.height is never used then even Tree itself can be eliminated and the tree string building logic can be placed in a single function:

fun buildTreeString(height: Int) = buildString {
    repeat(height) { layerIndex ->
        val layerWidth = 1 + 2 * layerIndex
        val marginWidth = height - layerIndex - 1
        repeat(marginWidth) { append(' ') }
        repeat(layerWidth) { append('*') }
        repeat(marginWidth) { append(' ') }
        appendln()
    }
}

And you can even pre-compute the required capacity to avoid unnecessary array copying while building the string:

fun buildTreeString(height: Int): String {
    return buildString(height * (2 * height - 1 + System.lineSeparator().length)) {
        repeat(height) { layerIndex ->
            val layerWidth = 1 + 2 * layerIndex
            val marginWidth = height - layerIndex - 1
            repeat(marginWidth) { append(' ') }
            repeat(layerWidth) { append('*') }
            repeat(marginWidth) { append(' ') }
            appendln()
        }
    }
}

inline fun buildString(capacity: Int, builderAction: StringBuilder.() -> Unit)
        = StringBuilder(capacity).apply(builderAction).toString()

If however you actually need/want classes for tree components then I recommend making those components (or collections of those components) members of a Tree class (e.g. a Tree has layers: List<Tree.Layer>).


Add string to array based on flag Is there any way to improve this kind of code? I have converted this from production code for privacy reasons, so there might be some mistakes.

public class AddNewUserDto
{

    public bool IsAdmin { get; set; }
    public bool IsRead { get; set; }
    public bool IsWrite { get; set; }

    public string[] GetRolesByDto()
    {
        List<string> roles = new List<string>();

        if (this.IsAdmin)
        {
            roles.Add(USER_ROLES.ADMIN);
        }
        if (this.IsRead)
        {
            roles.Add(USER_ROLES.IsRead);
        }
        if (this.IsWrite)
        {
            roles.Add(USER_ROLES.IsWrite);
        }
        if (this.IsEntityUser)
        {
            roles.Add(USER_ROLES.IsEntityUser);
        }

        return roles.ToArray();
    }
}

public static class USER_ROLES
{
    public const string IsAdmin= "Admin";
    public const string IsRead= "Read";
    public const string IsWrite= "Write";
    public const string IsEntityUser = "EntityUser";
}

strings are probably the wrong choice to represent roles. You lose the power of the type system -- the compiler won't pick up on typos, e.g. "Amdin". Consider using an enum instead.

Be consistent with your naming. Why is it USER_ROLES.IsRead and USER_ROLES.Write (not IsWrite)? Stick to the standard naming conventions, e.g. write UserRole.IsRead instead of USER_ROLES.IsRead.

Does the calling code really need an array returned? It would probably be fine with an IEnumerable.

If we change the return type to IEnumerable<UserRole>, we can write

public IEnumerable<UserRole> GetRoles()
{
    if (this.IsAdmin)
    {
        yield return UserRole.Admin;
    }

    if (this.IsRead)
    {
        yield return UserRole.Read;
    }

    if (this.IsWrite)
    {
        yield return UserRole.Write;
    }

    if (this.IsEntityUser)
    {
        yield return UserRole.EntityUser;
    }
}

But I'm guessing you're asking more about the repetition. Here's one way to get around it -- create an association between the booleans and the values. Here I've used a dictionary; it's possibly a bit of overkill but it does give us the nice initialiser syntax:

public IEnumerable<UserRole> GetRoles()
{
    var roles = new Dictionary<UserRole, bool>
    {
        { UserRole.Admin, this.IsAdmin },
        { UserRole.Read, this.IsRead },
        { UserRole.Write, this.IsWrite },
        { UserRole.EntityUser, this.IsEntityUser }
    };
    return roles.Where(role => role.Value)
                .Select(role => role.Key);
}

Other options would be an array of KeyValuePairs, an array of Tuples, etc.


var quarterHours = ["00", "15", "30", "45"];
var times = [];
for(var i = 0; i < 24; i++){
  for(var j = 0; j < 4; j++){
    times.push(i + ":" + quarterHours[j]);
  }
}

That's the really quick way without thinking. There might be better ways, but this is what I come up with in 2 minutes.

After testing, you'll find out that it doesn't have a 0 prefix for hours 1 through 9. They show up as "9:45". You could either add a check for that, like so:

var quarterHours = ["00", "15", "30", "45"];
var times = [];
for(var i = 0; i < 24; i++){
  for(var j = 0; j < 4; j++){
    if(i < 10){
      times.push("0" + i + ":" + quarterHours[j]);
    } else {
      times.push(i + ":" + quarterHours[j]);
    }
  }
}

Some more minor cleanup...

var quarterHours = ["00", "15", "30", "45"];
var times = [];
for(var i = 0; i < 24; i++){
  for(var j = 0; j < 4; j++){
    var time = i + ":" + quarterHours[j];
    if(i < 10){
      time = "0" + time;
    }
    times.push(time);
  }
}

Or declare another array with "00", "01", "02"... "23".

Basically what you end up doing is creating every combination of hours and 15 minutes to recreate that big array of yours.


Check if a number is prime I'm looking for feedback on the following code which checks if a number is prime using Swift 2.

 import UIKit

 var number:Int = 1123

 var isPrime:Bool = true

 switch number {

 case 1:
     isPrime = false

 case 2:
     isPrime = true

 case 3:
     isPrime = true

 default:
     primeCheck:for i in 2...Int(sqrt(Double(number))) {

         if number % i == 0 {

             isPrime = false
             break primeCheck

         }
   }

}

if isPrime {

    print("The number \(number) is prime!")

} else {

    print("The number \(number) is composite!")
}

I won't comment on the algorithm you're using to find out if the number is prime or not. Though the way you're doing it is quite inefficient. Some people in the comments have already pointed out some possible improvements and I'd suggest looking into the sieve of Eratosthenes to get better performance. You can find a Swift implementation of it here.

First off, there is a lot of unnecessary whitespace, you should get rid of that and only use a blank line too separate logical parts of your code.

Secondly, let's look at the following part:

var number:Int = 1123
var isPrime:Bool = true

There are three issues with it. number is mutable, but you never actually change it, it is customary to have exactly one space after a colon in Swift (instead of none) and while we're at it, in Swift you don't typically explicitly state the the of a variable unless it significantly improves readability (which is almost never) or type inference gets the wrong type. (which also doesn't happen often.) So you can rewrite those two lines as:

let number = 1123
var isPrime = true

As for the meat of this answer, it would be preferable to extract the actual prime checking into a function:

func isPrime(number: UInt32) -> Bool {
    switch number {
        case 0, 1: // you can put multiple cases on one line
            return false
        case 2, 3:
            return true
        default:
            for i in 2...Int(sqrt(Double(number))) {
                if number % i == 0 {
                    return false
                }
            }
            return true
        }
    }
}

This allows you to write:

let number = 1123
if isPrime(number) {
    print("The number \(number) is prime!")
} else {
    print("The number \(number) is composite!")
}

or even:

print("The number \(number) is \(isPrime(number) ? "prime" : "composite")!")

This allows you to get rid of the mutable isPrime, which is something you should strive for in Swift. And allows you to nicely ecapsule the actrual logice and separate it from the rest.


Solved the problem. Might some cleanup and/or advise I wanted to display 1 outcome of 1 single custom field or attribute in the category overview, using a third party customfield extension in my J! and VM website. I got it working using the code below. As this is the first time I actually write PHP and MySQL stuff myself, I was pretty proud, but also thinking 'this can probably be done better..'

<?php
//first get the customfieldid for the product where the custom id is 10
$db2 = JFactory::getDBO();
$db2->setQuery("SELECT virtuemart_customfield_id FROM #__virtuemart_product_customfields WHERE virtuemart_product_id=".$product->virtuemart_product_id." and virtuemart_custom_id=10 ;");
$db2->query();
$customFieldId = $db2->loadResult();

//make a database connection and find the field record that contains the customfield string
$db = JFactory::getDBO();
$db->setQuery("SELECT customsforall_value_id FROM #__virtuemart_product_custom_plg_customsforall WHERE virtuemart_product_id=".$product->virtuemart_product_id." and customfield_id=".$customFieldId." ;");
$db->query();
$staatValueId = $db->loadResult();

//knowing the customsforall id with the product, saved in a $ we get the attached string
$customStaat = JFactory::getDBO();
$customStaat->setQuery("SELECT customsforall_value_name FROM #__virtuemart_custom_plg_customsforall_values WHERE virtuemart_custom_id=10 and customsforall_value_id=".$staatValueId." ;");
$customStaat->query();
$resultStaat = $customStaat->loadResult();

//display the state
echo "".$resultStaat."";
?>

Can I do this better, or clean things up?

Security

You should use prepared statements for all variable data to protect against SQL injection.

The thing is, Joomla actually doesn't seem to support prepared statements. For me, that would be a reason to think about using a different CMS, but you should at the very least follow their guidelines to prevent SQL injection (checking data type and escaping. which - when done correctly - does offer complete protection against injections).

When echoing data, you should also protect against XSS.

Misc

all your SQL keywords should be uppercase (eg and -> AND) to increase readability.
your table and column names are too long, which makes the queries hard to read. For example, to you really need customsforall or virtuemart in them?
you get a new DBO for each query, this doesn't seem necessary.
you should be able to use JOIN to reduce the amount of queries you have.