Checking mimetype and uploading a file I'm using this method from my Downloads() class in a back-end admin system, where these can upload some files, which must be .rar or .zip I'm not to concern about filesize, yet. Eventually, these compressed files, will be holding only .mp3 files.

Well, here's the code. As you can see, I'm checking mimetype against an array of the ones allowed, which I got them from Stack Overflow, but I'm still not sure if I'm not missing any.

Then, I'm checking against the file extension in case mimetype is faked (not sure how accurate this check is).

Are those conditionals OK? I mean, I wouldn't want the file to be uploaded, but the query not to be done, or vice-versa.

//Handle file upload process
    public function uploadFile($titulo,$file) {
        $accepted_types = array(
                        'application/zip',
                        'application/x-zip-compressed',
                        'multipart/x-zip',
                        'application/x-compressed',
                        'application/x-rar-compressed',
                );
        if (isset($file['name'])) {
            $filename = str_replace(' ', '_',$file["name"]);
            $tmp_name = $file["tmp_name"];
            $type = $file["type"];
            $name = explode(".", $filename);
            foreach($accepted_types as $mime_type) {
                if($mime_type == $type) {
                        $okay = true;
                        break;
                }
            }
            //Check mimeType
            $continue = (strtolower($name[1]) == 'zip') || (strtolower($name[1]) == 'rar') ? true : false;
            if(!$continue) {
                    return false;
            }
            $finalPath = DOWNLOAD_PATH.$filename;
            if (file_exists($finalPath)) {
                $newname=  Utility::file_newname(DWONLOAD_PATH,$filename);
                $finalPath= DWONLOAD_PATH.$newname;
            }

            if (move_uploaded_file($tmp_name, $finalPath)) {
                $fileSize = filesize($finalPath);
                $query = "INSERT INTO descargas (titulo,size,src,filename) VALUES
                    ('$titulo','$fileSize','$finalPath','$filename')";
                if ($this->mysqli->query($query)) {
                    return true;
                } else {//The file's been moved, but theere was an error in the query
                    unlink($finalPath);
                    return false;
                }
            }
        }
    }

Right, seeing as I could go on and review your code in comments, I'd thought I'd post my comments + extra review in one go instead (after all, that's what this site is for).

Starting from the top:

public function uploadFile($titulo,$file) {

A couple of niggles and suggestions

Please try to adhere to the PHP-FIG coding standards. They're not official, but there are no official standards. PHP-FIG is subscribed to by all the main players, so it's best you do, too
The $file argument is central to the entire method. Why isn't it the first argument? Suppose you want $titulo (I'm assuming it means title or something) optional, and default to a string?
What is $file meant to be? What do you expect it to be? If I am to use your class, I'd like to be able to know what I have to pass to your methods just by looking at their signature.

So, basically, I'd recommend changing your method signature to:

public function uploadFile(array $file, $titulo = 'optional default argument')
{//brace goes on new line
}

Even that wouldn't be to my liking, though. I should know I'm expected to pass a file, so why not write:

public function uploadFile(\SplFileInfo $file, $titulo = null)
{
}

In this case, you can get rid of that if (isset($file['name'])) bit, and focus on the stuff that matters.

Next:

$accepted_types = array(
                'application/zip',
                'application/x-zip-compressed',
                'multipart/x-zip',
                'application/x-compressed',
                'application/x-rar-compressed',
);

Now every time this method gets called, the array above will be constructed. After the function returns, its ref-count will (or should) be zero, and it'll get GC. If you want to call this method more than once, or if you want to re-use this class for other file manipulations, you should turn this into a property:

class Downloads
{
    private $accepted_mimes = array(
                'application/zip',
                'application/x-zip-compressed',
                'multipart/x-zip',
                'application/x-compressed',
                'application/x-rar-compressed',
    );
    public function uploadFile(\SplFileInfo $file, $titulo)
    {
        //code here
    }
}

Better yet: define an associative array (this can be a private static, for once) that groups the mime-types into "modes", which you can then use per constant:

class Downloads
{
    const MIME_MODE_FILE = 1;
    const MIME_MODE_TEXT = 2;
    const MIME_MODE_FOO = 4;//powers of 2, for easy bitwise operations

    private static $mime_modes = array(
        self::MIME_MODE_FILE => array(
                'application/zip',
                'application/x-zip-compressed',
                'multipart/x-zip',
                'application/x-compressed',
                'application/x-rar-compressed',
        ),
        self::MIME_MODE_TEXT => array(
            //mime-types for text content
        )
    );
    private $mime_mode = null;
    private $accepted = null;

    public function __construct($mode = self::MIME_MODE_FILE)
    {
        if (!isset(static::$mime_modes[$mode])) throw new InvalidArgumentException('Invalid mime-mode for '.__CLASS__);
        $this->mime_mode = $mode;
        $this->accepted = static::$mime_modes[$mode];
    }
}

And take it from there. The reason why bitwise operators can come in handy here is that, if I wanted to use your Download class to process text files, but then call a method that was intended for use on non-text files, like archive decompression, you could check the $this->mime_mode property.
However MS Office documents are, in effect zip files. So a mime-type to accept those would have to accept zip files, too. Thus, the mime-mode to set might have to be Downloads::MIME_MODE_FILE | Downloads::MIME_MODE_TEXT
Of course, you'll have to work on the constructor in that case, but that's something you can do yourself. It's not that hard.

As for the actual checking of the mime-type: I've previously linked this answer and discussed various ways to do so. You've decided to play it safe and go for the checking of the first n bytes. That's fine, but never trust the result of a single check, though: always check both the given mime-type, and the bytes, always go "Double Dutch".

Lastly: mysqli_* usage where prepared statements shine in absence.
Sure, you say this code will only be called deep in the bowels of your back-end system, and no user input will be used. Call me stupid, but an uploaded file is user input in my book. I haven't got the faintest idea as to what $titulo is, nor do I know what the filenames might look like. Personally, I'd still play it safe and use a prepared statement.
If you feel as if prepared statements will only cause overhead then don't, because prepared statements can be used more than once. You could lazy-load a statement:

class Downloads
{
    //all of the consts, methods and properties I've previously listed +:
    private $mysqli = null;
    private $stmts = array();
    //then add:
    private function getStatement($queryString)
    {
        if (!isset($stmts[$queryString]))
        {
            $this->stmts[$queryString] = $this->mysqli->prepare($queryString);
        }
        return $this->stmts[$queryString];
    }
}

The prepared statement won't exist until, from the uploadFile method you call:

$stmt = $this->getStatement(
    'INSERT INTO descargas (titulo,size,src,filename) VALUES
      (?,?,?,?)'
);

The first time this call is executed, you'll create a prepared statement, the following times, you'll just get the same prepared statement returned to you, ready to be used again. So the overhead isn't as big as you might expect it to be.
Security-wise, however, you'll be better of. And in a way, in terms of traffic between PHP and MySQL, you'll bode well, too: the query string from which the statement is prepared is sent once, for all subsequent execute calls, only the parameters are sent over to the server, via a different protocol, so if you execute this query 20 times or more, you'll possibly end up sending less data to the DB.


Submitting the current webpage to a bookmarking site I would like to hear your feedback on the link below that lets users add a link to a website. I read that javascript:void(0) is suggested instead of a # for the <a href="" attribute. Is this still the best practice?

Add Link

First of all, void is an operator, not a function. The parentheses are extraneous; it should be written as void 0.

href="#" should be avoided for the reasons given in the answer you linked; people will forget to cancel the event and the page will jump to the top when users click the link, or worse, if the page uses a base tag, the page will navigate somewhere else.

Most people will tell you to avoid putting JavaScript in tag attributes at all, instead binding events to the element later on, once the page has been loaded, or binding the event to a parent element and delegating.

If you're going to use an inline onclick attribute anyway, and there's nothing appropriate to put in the href attribute, I'd consider not making it a link at all. Make it a button or some other kind of element, possibly styled to look like a hyperlink.

As a final note, GET requests should be safe and idempotent, meaning following a link or visiting a URL should not cause changes on your server / in your database. https://www.example.com/add?... looks suspiciously like it breaks that rule; you may be in a world of trouble if spiders start crawling those links. Making a POST request, for example by submitting a form, would be the proper way to handle this.


Bucket Sort with Linked List This is my implentation of Bucket Sort, but I see myself using way too many for loops, is this necessary?

  public static void bucketSort(Student[] students) {
    final String[] courses = { "IB", "IG", "IN", "IS", "IT" };
    final int amountOfBuckets = courses.length;
    final int n = students.length;

    if (n == 0) return;

    LinkedList<LinkedList<Student>> buckets = new LinkedList<>(); //create buckets
    for (int i = 0; i < amountOfBuckets; i++) {
        buckets.add(new LinkedList<>()); //fill bucketlist with buckets
    }

    for (int i = 0; i < courses.length; i++) {
        for (Student student : students) {
            if (student.getClassNumber().startsWith(courses[i])) {
                buckets.get(i).add(student);
            }
        }
    }

    for (int i = 0; i < amountOfBuckets; i++) {
        LinkedList<Student> studentLinkedList = buckets.get(i);
        Student[] s = studentLinkedList.toArray(new Student[studentLinkedList.size()]);
        insertionSortByClass(s);
        studentLinkedList = new LinkedList<>(Arrays.asList(s));
        buckets.set(i, studentLinkedList);
    }

    LinkedList<Student> result = new LinkedList<>();
    for (LinkedList<Student> bucket : buckets) {
        result.addAll(bucket);
    }

    for (int i = 0; i < result.size(); i++) {
        students[i] = result.get(i);
    }
}

As requested:

    public static void insertionSortByClass(Student[] student) {
    Student temp;
    for (int i = 1; i < student.length; i++) {
        for (int j = i; j > 0; j--) {
            if (student[j].compareTo(student[j - 1]) < 0) {
                temp = student[j];
                student[j] = student[j - 1];
                student[j - 1] = temp;
            }
        }
    }
}

This code seems to be a specific BucketSort made for a specific program. I would instead create a more generic BucketSort:

public static <E extends Comparable<E>> void bucketSort(E[] array) {
    // ...
}

public static <E> void bucketSort(E[] array, Comparator<E> comparator) {
    // ...
}

Note there is two methods; one is for a class that implements the Comparator interface, and one to allow a Comparator to be passed as the Comparator for the class, where there is no comparator, or if you want to compare it in a different way.

Then the only problem is that you don't know the bounds for the buckets; the simple way would be to use bounds provided by the calling method:

public static <E extends Comparable<E>> void bucketSort(E[] array, E[] separators) {

}

public static <E> void bucketSort(E[] array, E[] separators, Comparator<E> comparator) {

}

The separators will be the upper limit of the bucket right before it; and the uppermost bucket will catch everything that is larger than the largest bound.

Now for the actual code:

import java.util.ArrayList;
import java.util.Collection;
import java.util.Comparator;
import java.util.LinkedList;
import java.util.List;

public class BucketSort {

    public static <E extends Comparable<E>> List<E> bucketSort(E[] array,
            E[] separators) {
        Comparator<E> comparator = new Comparator<E>() {

            @Override
            public int compare(E element1, E element2) {
                return element1.compareTo(element2);
            }

        };
        return bucketSort(array, separators, comparator);
    }

    public static <E> List<E> bucketSort(E[] array, E[] separators,
            Comparator<E> comparator) {
        final int numOfBuckets = separators.length + 1;
        // Create Buckets
        // Used ArrayList because we don't need the functions of a LinkedList
        List<Bucket<E>> buckets = new ArrayList<Bucket<E>>(numOfBuckets);
        for (int i = 0; i < numOfBuckets; i++) {
            buckets.add(new Bucket<E>());
        }
        // Sort into Buckets
        for (E element : array) {
            // Use binary search
            buckets.get(bucketNumber(element, separators, comparator)).getItems().add(element);
        }
        // Sort buckets
        for (Bucket<E> bucket : buckets) {
            bucket.getItems().sort(comparator);
        }
        // Combine Buckets and return
        for (int i = 1; i < numOfBuckets; i++) {
            buckets.get(0).getItems().addAll(buckets.get(i).getItems());
        }
        return buckets.get(0).getItems();
    }

    private static <E> int bucketNumber(E element, E[] separators,
            Comparator<E> comparator) {
        return bucketNumber(element, separators, 0, separators.length,
                comparator);
    }

    private static <E> int bucketNumber(E element, E[] separators, int low,
            int high, Comparator<E> comparator) {
        if (low + 1 == high) {
            return low;
        }
        int searchIndex = (high - low) / 2 + low;
        int compareResult = comparator
                .compare(element, separators[searchIndex - 1]);
        if (compareResult == 0) {
            return searchIndex;
        }
        return compareResult < 0 ? bucketNumber(element, separators, low,
                searchIndex, comparator) : bucketNumber(element, separators,
                searchIndex, high, comparator);
    }

}

class Bucket<E> {

    private List<E> elements;

    public Bucket() {
        this.elements = new LinkedList<E>();
    }

    public Bucket(Collection<E> elements) {
        this.elements = new LinkedList<E>(elements);
    }

    public List<E> getItems() {
        return elements;
    }

}

Yes, I know it looks 10x more complex than it need to be, but this can be used for further applications.

Since you asked for an explanation of the separators array...

The separator's array is there so that the method can know where to separate it into buckets. For example:

separator = [2, 5, 8, 11]
array = [1, 7, 2, 6, 4, 8, 12, 10, 13, 0, 5, 3, 9, 11]

Then the buckets would be:

buckets = [ [0, 1] , [2, 3, 4] , [5, 6, 7] , [8, 9, 10] , [11, 12, 13] ]


Hackerrank "Sherlock Holmes" challenge Watson gives to Sherlock an array: A1, A2, ⋯, AN. He also gives to
  Sherlock two other arrays: B1, B2, ⋯, BM and C1, C2, ⋯, CM. Then Watson asks
  Sherlock to perform the following program:

  for i = 1 to M do
   for j = 1 to N do
     if j % B[i] == 0 then
         A[j] = A[j] * C[i]
    endif
 end do
end do

  Can you help Sherlock and tell him the resulting array A? You should print all the array elements modulo (1000000007).

  Input Format

  The first line contains two integer numbers N and M. The
  next line contains N integers, the elements of array A. The next two
  lines contains M integers each, the elements of array B and C.

  Output Format

  Print N integers, the elements of array A after
  performing the program modulo (1000000007).

  Sample Input

4 3
1 2 3 4
1 2 3
13 29 71

  Sample Output

13 754 2769 1508

If we brute force, it will time out.  Please suggest ways on making this efficient.

import java.math.BigInteger;
import java.util.Scanner;
import java.util.StringTokenizer;

public class SherlockArray {

public static void main(String[] args) {
    Scanner in = new Scanner(System.in);
    String line1 = in.nextLine();
    StringTokenizer st = new StringTokenizer(line1, " ");

    int n = Integer.parseInt(st.nextToken());
    int m = Integer.parseInt(st.nextToken());
    Long[] b = new Long[m];

    String strA = in.nextLine();
    String[] stA = strA.split(" ");
    BigInteger aBI[] = new BigInteger[n];
    for (int ia = 0; ia < stA.length; ia++) {
        aBI[ia] = new BigInteger(stA[ia]);
    }

    String strB = in.nextLine();
    String[] stB = strB.split(" ");
    for (int ib = 0; ib < stB.length; ib++) {
        b[ib] = Long.parseLong(stB[ib]);
    }

    String strC = in.nextLine();
    String[] st3 = strC.split(" ");
    for (int ic = 0; ic < m; ic++) {
        for (int index = b[ic].intValue(); index <= n; index += b[ic]
                .intValue()) {
            aBI[index - 1] = aBI[index - 1].multiply(new BigInteger(st3[ic]));
        }
    }

    for (int ia = 0; ia < n; ia++) {
        aBI[ia] = aBI[ia].mod(new BigInteger("1000000007"));
        System.out.print(aBI[ia] + " ");
    }

}

}

In 1801, a guy called Carl Friedrich Gauss studied problems where the number line wrapped around, called Modular Arithmetic.

In his studies, he proved that:

$$
    (a \times b)\ \%\ n = [(a)\ \%\ n \times (b)\ \%\ n]\ \%\ n
$$

Also, 1000000007 is a prime number which means that there are other benefits...

And, it is also just less than half of Integer.MAX_VALUE.

Finally, two int values, no matter how large, when multiplied, will never be larger than Long.MAX_VALUE

Putting this all together, you can rewrite your code without the BigInteger math, something like:

for (int i = 0; i < N; i++) {
    for (int j = 0; j < M; j++) {
        if (j % B[i] == 0) {
            A[j] = (int)( ( (long) A[j] * C[i]) % 1000000007L);
        }
    }
}
System.out.println(Arrays.toString(A));

So, using some math, you can avoid the BigInteger problem entirely, and keep things as long and int values.

I imagine that this will be enough of a performance improvement to avoid the timeout.

Note, that for large numbers, BigInteger has \$O(s)\$ type complexity where s is the magnitude of the number, when doing multiplication. The bigger the number, the slower the product. Thus, your complexity for your current solution is \$O(nms)\$ where n is the size of the A array, m is the size of the B and C arrays, and s is the average size of the BigIntegers used. By reducing the problem to int-size values, we reduce the time complexity by a full order to just\$O(nm)\$.

There are likely ways for you to be able to solve the math in O(n) time as well.... I just need to think about how to restructure the if condition in the loops


RSpec/Capybara tests There are three scenarios only for when the sign up validations fail, so is there a better way of representing them rather than having 4 scenarios? I don't want to create a model folder so please don't suggest that.

Is there a better way of representing this RSpec code on Rails using Capybara?

feature 'Login' do
  before do
    FactoryGirl.create(:user)
  end

  scenario "success login", js: true do
    # set_speed(:slow)
    visit root_path
    click_link 'Login'

    fill_in 'email', :with => 'test@example.com'
    fill_in 'password', :with => 'password'
    click_button 'Login'

    expect(page).to have_content('Logged in successfully')
  end

  scenario "failed login", js: true do
    # set_speed(:slow)
    visit root_path
    click_link 'Login'

    fill_in 'email', :with => 'failed@login.com'
    fill_in 'password', :with => 'something failed'
    click_button 'Login'

    expect(page).to have_content('Invalid login/password combination')
  end
end

feature "Sign Up" do
  scenario "success sign up", js:true do
    visit root_path
    click_link 'Login'
    click_link 'Sign Up'

    fill_in 'user[email]', :with=>'signup@example.com'
    fill_in 'user[password]', :with=> 'password'
    fill_in 'user[password_confirmation]', :with=> 'password'
    click_button 'Create User'
    expect(page).to have_content('User successfully added.')
  end

  scenario "failed sign up/Wrong email format", js:true do
    visit root_path
    click_link 'Login'
    click_link 'Sign Up'
    fill_in 'user[email]', :with=>'signup.example.com'
    fill_in 'user[password]', :with=> 'password'
    fill_in 'user[password_confirmation]', :with=> 'password'
    click_button 'Create User'
    expect(page).to have_content('is invalid')
  end

  scenario "failed sign up/Short Email address", js:true  do
    visit root_path
    click_link 'Login'
    click_link 'Sign Up'
    fill_in 'user[email]', :with=>'sign'
    fill_in 'user[password]', :with=> 'password'
    fill_in 'user[password_confirmation]', :with=> 'password'
    click_button 'Create User'
    expect(page).to have_content('is too short (minimum is 5 characters)')
  end

  scenario "failed sign up/Long Email address", js:true  do
    visit root_path
    click_link 'Login'
    click_link 'Sign Up'
    fill_in 'user[email]', :with=>'fillinginwritingwronglongpasswordstogetanerror@gmail.com'
    fill_in 'user[password]', :with=> 'password'
    fill_in 'user[password_confirmation]', :with=> 'password'
    click_button 'Create User'
    expect(page).to have_content('is too long (maximum is 50 characters)')
  end

end

Some comments:

Rspec expects the concatenation of feature and `scenario to form a human readable text.
AFAIK, the orthodoox structure for specs is to perform the actions in the before block and only assertions in it blocks.
Use before blocks to keep your code DRY.
Personal opinion: in a feature spec, the less app internals you use, the better. So I'd write "/" instead of root_path.
The user/password info a in the factory, I'd prefer explicit attributes on the creation.

That's how I'd write the Login feature, the same ideas apply for Signup:

feature 'Login' do
  before do
    FactoryGirl.create(:user, :email => "test@example.com", :password => "password")
    visit root_path
    click_link 'Login'
  end

  scenario "with valid user/password", js: true do
    before do
      fill_in 'email', :with => 'test@example.com'
      fill_in 'password', :with => 'password'
      click_button 'Login'
    end

    it "shows the logged in message" do
      expect(page).to have_content('Logged in successfully')
    end
  end

  scenario "with wrong user/password", js: true do
    before do
      fill_in 'email', :with => 'failed@login.com'
      fill_in 'password', :with => 'something failed'
      click_button 'Login'
    end

    it "shows an error message" do
      expect(page).to have_content('Invalid login/password combination')
    end
  end
end


Queue implementation using linked list I have implemented a queue data structure using linked list in C#. I have also associated my implementation with unit tests. Is this implementation correct? Any suggestion?

namespace QueueDataStructure
{
    internal class Node
    {
        internal int value;
        internal Node next;
    }

    public class Queue
    {
        private Node head;
        private int size;

        public Queue(){}

        public void Enqueue(int n)
        {
            if(head == null) // queue is empty
            {
                head = new Node{
                    value = n,
                    next = null
                };
            }
            else // queue has items
            {
                var oldHead = head;
                head = new Node
                {
                    value = n,
                    next = oldHead
                };
            }
            size++;
        }

        public int? Dequeue()
        {
            if (size == 0)
                return null;

            var node = head;
            Node previous = node;
            while (node.next != null)
            {
                previous = node;
                node = node.next;
            }
            previous.next = null;
            size--;
            return node.value;
        }

        public int Count
        {
            get { return size; }
        }

        public string PrintElements()
        {
            var node = head;
            int[] elements = new int[size];
            int i = 0;
            while (node != null)
            {
                elements[i++] = node.value;
                node = node.next;
            }
            return string.Join(" ", elements);
        }
    }
}

using Microsoft.VisualStudio.TestTools.UnitTesting;
using QueueDataStructure;

namespace TestQueue
{
    [TestClass]
    public class Tests
    {
        [TestMethod]
        public void TestQueue()
        {
            var queue = new Queue();
            Assert.AreEqual(0, queue.Count);
            Assert.AreEqual(null, queue.Dequeue());

            queue.Enqueue(1);
            queue.Enqueue(2);
            queue.Enqueue(3);
            Assert.AreEqual(3, queue.Count);
            Assert.AreEqual("3 2 1", queue.PrintElements());

            Assert.AreEqual(1, queue.Dequeue());
            Assert.AreEqual(2, queue.Dequeue());
            Assert.AreEqual(3, queue.Dequeue());
            Assert.AreEqual(0, queue.Count);
        }
    }
}

Good

You are following the .NET Naming Guidelines
The variables are in the correct scope

Review
Omitting braces {} although they might be optional can lead to hidden and therfore hard to track down bugs. I would like to encourage you to always use them.

Regarding public string PrintElements() I would suggest to simply override ToString() which is more idiomatic.

If we take a closer look at the Enqueue(int n) method we notice that the difference between the if and the else part is simply the value of next. We can remove this code duplication by adding a variable Node node and assign head like so
public void Enqueue(int n)
{
    Node node = head;
    head = new Node
    {
        value = n,
        next = node
    };

    size++;
}

much better, isn't it ?

A C# developer would expect from a Queue that if the Queue is empty (in your case if size == 0) a call to Dequeue() would throw an InvalidOperationException.


Algorithm that combines same objects to single common key I wrote this algorithm but i was wondering if there was any way i can make it less 'expensive'. The algorithm needs to transform this data structure:

const deliveryHours = {
  monday: [
    {
      start: {
        hour: 3,
        minute: 0,
      },
      end: {
        hour: 6,
        minute: 0,
      },
    },
  ],
  tuesday: [
    {
      start: {
        hour: 3,
        minute: 0,
      },
      end: {
        hour: 6,
        minute: 0,
      },
    },
    {
      start: {
        hour: 5,
        minute: 0,
      },
      end: {
        hour: 3,
        minute: 0,
      },
    },
  ],
  wednesday: [
    {
      start: {
        hour: 3,
        minute: 0,
      },
      end: {
        hour: 4,
        minute: 0,
      },
    },
    {
      start: {
        hour: 5,
        minute: 0,
      },
      end: {
        hour: 1,
        minute: 0,
      },
    },
  ],
}

to:

[ { hours: { from: 3, to: 4 }, wednesday: true },
  { hours: { from: 3, to: 6 }, tuesday: true, monday: true },
  { hours: { from: 5, to: 1 }, wednesday: true },
  { hours: { from: 5, to: 3 }, tuesday: true } ]

This is my attempt, it is clearly not the best way to go about it but im having a hard time finding another way:

const deliveryHours = {
  monday: [
    {
      start: {
        hour: 3,
        minute: 0,
      },
      end: {
        hour: 6,
        minute: 0,
      },
    },
  ],
  tuesday: [
    {
      start: {
        hour: 3,
        minute: 0,
      },
      end: {
        hour: 6,
        minute: 0,
      },
    },
    {
      start: {
        hour: 5,
        minute: 0,
      },
      end: {
        hour: 3,
        minute: 0,
      },
    },
  ],
  wednesday: [
    {
      start: {
        hour: 3,
        minute: 0,
      },
      end: {
        hour: 4,
        minute: 0,
      },
    },
    {
      start: {
        hour: 5,
        minute: 0,
      },
      end: {
        hour: 1,
        minute: 0,
      },
    },
  ],
}

const mapDeliveryHours = (deliveryHours) => {
  const result = {};
  for (let i = 0; i < Object.keys(deliveryHours).length; i += 1) {
    const day = Object.keys(deliveryHours)[i];

    const ranges = deliveryHours[day];

    for (let j = 0; j < ranges.length; j++) {
      const range = ranges[j];

      result[range.start.hour.toString() + range.end.hour.toString()] = {
        hours: {
          from: range.start.hour,
          to: range.end.hour,
        },
        [day]: true,
      };

      for (let k = 0; k < Object.keys(deliveryHours).length; k++) {
        const innerDay = Object.keys(deliveryHours)[k];
        const innerRanges = deliveryHours[innerDay];

        for (let l = 0; l < innerRanges.length; l++) {
          const innerRange = innerRanges[l];
          if (day !== innerDay
            && innerRange.start.hour === range.start.hour
            && innerRange.end.hour === range.end.hour
          ) {
            result[range.start.hour.toString() + range.end.hour.toString()] = {
              ...result[range.start.hour.toString() + range.end.hour.toString()],
              [innerDay]: true,
            };
          }
        }
      }
    }
  }
  return Object.values(result);
};

console.log(mapDeliveryHours(deliveryHours))

It is not much less code but I believe it is clearer and easier to reason about:

function forEach(target, fn) {
    var keys = Object.keys(target);
    var key;
    var i = -1;
    while (++i < keys.length) {
        key = keys[i];
        fn(target[key], key);
    }
}
function transform(target, fn, accumulator) {
    if (accumulator === undefined) {
        accumulator = Object.create(target);
    }
    forEach(target, function (value, key) {
        return fn(accumulator, value, key);
    });
    return accumulator;
}
function aggregateHours(hours) {
    var agg = [];
    return transform(hours, function (result, spans, day) {
        spans.forEach(function (span) {
            var found = agg.find(function (el, i) { return el.hours.from === span.start.hour && el.hours.to === span.end.hour; });
            if (found) {
                found[day] = true;
            }
            else {
                agg.push({ hours: { from: span.start.hour, to: span.end.hour } });
                agg[agg.length - 1][day] = true;
            }
        });
    }, agg);
}

aggregateHours(deliveryHours);

Basically I have stolen the foreach and transform from my library: goodcore but written it all in javascript here so that you don't have to use it.


Finding sequential recurrences in a list of numbers I have a list like this:

seq = [-2,3,-2,3,5,3,7,1,4,-5,1,4,-5,1,4,-5]

I want to get all the sequential recurrences and where it happens, I feel the way I'm doing its too brute force and in reality I want this for several lists each with length of about 300.

dic = {}
for p1 in range(len(seq)):
    for p2 in range(p1+1,len(seq)):
        dic.setdefault(tuple(seq[p1:p2]), []).append((p1,p2))

which results in all unique sequences of numbers as keys and their positions as values, for example:

#-2,3: [(0,2),(2,4)]

But also results in a lot of entries that occur only once that don't interest me, I'm 'cleaning' these after by taking only values that have more than 1 entry:

def clean(dic):
    cleandic = {}
    for key, value in dic.items():
        if len(value) > 1:
            cleandic.setdefault(key,value)
    return cleandic

cleandic = clean(dic)

Now for the last step I'm trying to get rid of the occurrences that happens inside the bigger ones, so I sorted the dict by reverse len of keys (bigger keys comes first), for example:

#(1,4,-5,1,4,-5) : ([7,13),(10,16)]
#...
#(1,4,-5) : [(7,10),(10,13)]

The best I came up with to take out the small ones:

sorteddic = dict(sorted(cleandic.items(), key=lambda item: len(item[0]), reverse=True))

onlybigs = {}
while len(sorteddic) > 0:
    for key1, values1 in sorteddic.items():
        for key2, values2 in sorteddic.copy().items():
            if len(key2) == len(key1):
                continue
            for value1 in values1:     #ex: (7,13)
                for value2 in values2: #ex: (7,10)
                    if value2[0] >= value1[0] and value2[1] <= value1[1]:
                        sorteddic[key2].pop(sorteddic[key2].index(value2))
        onlybigs.setdefault(key1, sorteddic.pop(key1))
        break

#and a second clean in the end
readydic = clean(onlybigs)

This last step especially is taking too long because it compares each value for each key and my guess is the whole process can be done more efficiently somehow.

Any insights?

You're doing things the hard way.

dic = {}
for p1 in range(len(seq)):
    for p2 in range(p1+1,len(seq)):
        dic.setdefault(tuple(seq[p1:p2]), []).append((p1,p2))

For each range, you are calling .setdefault().  You should just use a default dictionary, which can automatically create a list for any unseen keys:

from collections import defaultdict

dic = defaultdict(list)

for p1 in range(len(seq)):
    for p2 in range(p1 + 1, len(seq)):
        dic[tuple(seq[p1:p2])].append((p1, p2))

Next, you're filtering the dictionary the hard way.

cleandic = {}
for key, value in dic.items():
    if len(value) > 1:
        cleandic.setdefault(key,value)

This can be expressed in one line, using list comprehension:

cleandic = { key: val for key, val in dic.items() if len(val) > 1 }

The next improvement comes from noting that the key portion of (-2, 3): [(0, 2), (2, 4)]} is completely recoverable from the value portion; using (0, 2) or (2, 4) as a slice of seq will return the appropriate sequence.  You do not need to maintain a dictionary; a list of lists is sufficient and less complex.  Instead of cleandic, you can have:

groups = [ val for val in dic.values() if len(val) > 1 ]

Instead of removing the second of these two dictionary values:

  #(1,4,-5,1,4,-5) : [(7,13),(10,16)]
     #(1,4,-5) : [(7,10),(10,13)]

You now just need to remove the second of these two list entries:

  [(7,13),(10,16)]
     [(7,10),(10,13)]

You can still sort this by noting that (7,13) represents a sequence of length 6.

groups = sorted(groups, key=lambda item: item[0][1] - item[0][0], reverse=True)

Any time you have to .copy() is good time to stop and think if there is another way to solve the problem.

    for key2, values2 in sorteddic.copy().items():
        ...
                  sorteddic[key2].pop(sorteddic[key2].index(value2))

Here, you are modifying sorteddic inside the loop, so you clearly can't loop on the sorteddic itself, which explains the need to copy the dictionary first.  Since the copy is itself in another loop, you are doing a lot of copies!

Moreover, this isn't even a loop!  It is just a way to extract the first item from the dictionary.

for key1, values1 in sorteddic.items():
    ...
    break

Rework the algorithm to not modify the container you're looping over:

def subset_of(long_groups, group):
    return any(lg[0] <= g[0] and g[1] <= lg[1] for long_group in long_groups
               for lg in long_group for g in group)

...

longest_groups = []
for group in groups:
    if not subset_of(longest_groups, group):
        longest_group.append(group)

Finally, you can rebuild your dictionary with the sequence tuples as keys:

readydic = { tuple(seq[group[0][0]:group[0][1]]): group for group in longest_groups }

Since all the dictionary copying has been removed, this should be significantly faster.


Proper naming for objects which have a position and bounds on a plane I have an interface which is for an object which has bounds in a 2D plane.

interface Boundable : Positionable {

    /**
     * Returns the [Bounds] (the [Position] and [Size]) of this [Boundable].
     */
    fun bounds(): Bounds

    /**
     * Returns the [Size] of this [Boundable].
     */
    fun size(): Size

    /**
     * Tells whether this [Boundable] intersects the other `boundable` or not.
     */
    fun intersects(boundable: Boundable): Boolean

    /**
     * Tells whether `position` is within this boundable's bounds.
     */
    fun containsPosition(position: Position): Boolean

    /**
     * Tells whether this boundable contains the other `boundable`.
     * A [Boundable] contains another if the other boundable's bounds
     * are within this one's. (If their bounds are the same it is considered
     * a containment).
     */
    fun containsBoundable(boundable: Boundable): Boolean
}

This is something like Rect in the Java SDK for example. Currently Boundable inherits from Positionable:

/**
 * Represents an object which is positionable within its parent.
 * Note that once positioned a [Positionable] can't be moved.
 * If you want re-positionable objects @see [Movable].
 */
interface Positionable {

    fun position(): Position = Position.zero()

}

Positionable is for objects which have a Position within a larger object on the same plane. This setup worked so far but now I need to have 3 versions of Boundable. One, which does not inherit from Positionable (to be used for detached or root objects), one which does and one which is Movable:

/**
 * A [Movable] object is a specialized [Positionable].
 * It can not only be positioned but moved as well.
 */
interface Movable : Positionable {

    fun moveTo(position: Position): Boolean

    fun moveBy(position: Position) = moveTo(position() + position)

    fun moveRightBy(delta: Int) = moveTo(position().withRelativeX(delta))

    fun moveLeftBy(delta: Int) = moveTo(position().withRelativeX(-delta))

    fun moveUpBy(delta: Int) = moveTo(position().withRelativeY(-delta))

    fun moveDownBy(delta: Int) = moveTo(position().withRelativeY(delta))
}

My problem is figured out a name for the variants, but I'm not satisfied with them. I came up with PositionableBoundable and MovableBoundable but they sound weird.

Are these names adequate or should I change them to something more descriptive?

Everything is an Entity!

If you would follow the Entity Component System approach, that is. The idea is that all objects are entities, and each Entity has several components. Size, Position, Movable - all this are different aspects of what an Entity possibly could do and are therefore each it's own Component. As for Systems, that's where most of the logic is.

Names like PositionableBoundable and MovableBoundable sound weird because it is weird, you are trying to use one name for two things. Composition over inheritance is the key here.

As for the name Boundable, I would probably have gone for just Box or Rect which are more well known.


Check if two strings are permutations of one another I am working my way through "Cracking the coding interview" problems and have finished implementing this question: "Given, two strings, write a method to decide if one is a permutation of the other". Can anyone give me any feedback on my implementation in regards of efficiency or improvements?

public static boolean containsPremutation(String firstString, String secondString){

    //Checking if either string is empty.
    if(firstString.isEmpty() || secondString.isEmpty())
        return false;

    //Checking if one string is larger than the other.
    if(firstString.length() > secondString.length() || secondString.length() > firstString.length())
        return false;

    //First convert the Strings into char[]
    char [] charFirstString =  firstString.toCharArray();
    char [] charSecondString = secondString.toCharArray();

    //Secondly, aplhabetize/sort the char arrays
    Arrays.sort(charFirstString);
    Arrays.sort(charSecondString);

    //Thirdly, create new Strings out of the sorted char arrays
    String alphaFirstString = new String(charFirstString);
    String alphaSecondString  = new String(charSecondString);

    //Now you can begin comparing each char in the Strings.
    // Begin iterating at the same char and if they are not the same char return false
    //otherwise continue iterating.
    for (int i = 0; i < alphaFirstString.length(); i++){
        if(alphaFirstString.charAt(i) != alphaSecondString.charAt(i))
            return false;
    }
    return true;

This question has used the word "permutation" for two strings, and in standard English what that's asking for is "is one the anagram of the other"? Using "anagram" as a search would help you find a lot of other information on this problem, especially on Code Review.

Your solution is significantly over-engineered. You have broken down the problem in to too many steps, and you've missed an opportunity to show how you would avoid code duplication.

Let's break down your code in to its stages:

Check each string is valid input
Check the strings are the same length
Extract the characters from each string
Sort each string's characters
Create a new String from each input's sorted characters.
Iterate over each of the sorted string's characters and compare to the other string.
Return the match status

Now, let's strip out the redundant parts... for example, you don't need to convert the sorted characters back to a string only to loop over each character again.... get rid of stage 5 and convert stage 6 to operate over the sorted array.

Also, stage 1, that's a bug. Two empty strings are anagrams of each other... agreed? Get rid of that check.

Now, what's left is two identical operations performed on two different inputs, which are then compared, so extract the common logic in to a function (Code reuse is a good thing):

private static final char[] sortedChars(String input) {
     char [] chars =  input.toCharArray();
     Arrays.sort(chars);
     return chars;
}

Then, your code looks like:

public static boolean containsPremutation(String firstString, String secondString){
    char[] firstChars = sortedChars(firstString);
    char[] secondChars = sortedChars(secondString);

    ....

}

Now what? Instead of converting them to strings, we can use the core library's Arrays class to do the comparisons for us:

return Arrays.equals(firstSorted, secondSorted);

The full code would be:

private static final char[] sortedChars(String input) {
     char [] chars =  input.toCharArray();
     Arrays.sort(chars);
     return chars;
}

public static boolean containsPremutation(String firstString, String secondString){
    char[] firstChars = sortedChars(firstString);
    char[] secondChars = sortedChars(secondString);

    return Arrays.equals(firstSorted, secondSorted);
}

Looking at the code that way, you can see other issues too...

generally, "Hungarian Notation" is frowned on in Java Code Style - don't have String appended to variable names like firstString and secondString ... we know that they are strings.
do we even need the variables in the contains... method? Could it just be:

public static boolean containsPremutation(String first, String second){
    return Arrays.equals(sortedChars(first), sortedChars(second));
}

As mentioned in a comment, it's Permutation and not Premutation.


Tweeting NASA's Astronomy Pic of the Day I want to know how I can make this old code faster, cleaner and compact. It may look like bad code because I did it when I first started with Python.

# -*- coding: utf-8 -*-

import urllib
import time
import os
from twython import Twython
from bs4 import BeautifulSoup as bs

class Alfred():

    def __init__(self):
        self.url = "http://apod.nasa.gov/apod/astropix.html"
        self.time = time.localtime()
        self.app_key = "XXX"
        self.app_sec = "XXX"
        self.token_key = "XXX"
        self.token_sec = "XXX"
        self.img_path = "static/img/"

    def start(self):

        tw = Twython(self.app_key, self.app_sec, self.token_key, self.token_sec)

        while True:

            if self.time[3] < 5:
                web = urllib.urlopen(self.url)
                soup = bs(web, "html.parser")
                img = soup.find("img")
                img_src = img.get("src")
                img_name = img_src[11:]

                if os.path.isfile(self.img_path + img_name):

                    pass

                else:

                    img_list = os.listdir(self.img_path)

                    for file_name in img_list:
                        os.remove(self.img_path + file_name)

                    img_url = "http://apod.nasa.gov/apod/" + img_src
                    urllib.urlretrieve(img_url, 'static/img/' + img_name)
                    photo = open(self.img_path + img_name, 'rb')
                    response = tw.upload_media(media=photo)
                    tw.update_status(status="Imagen astronómica del día. (vía: NASA). ~Alfred", media_ids=[response['media_id']])

            else:

                pass

a = Alfred()
a.start()

Instance and class variables.

I think tw should be an instance variable, made in the constructor.  Same for web and soup - not having to remake those every time will probably help you with your runtime issues (at least a little).

Then, I'd make all of your constants class-level variables.  This includes self.url, self.app_key, self.app_sec, self.token_key, self.token_sec, and self.img_path.  Really, the only one of the instance variables you set there that wouldn't be constant is the time.'

After that, you should probably add a few constants - "http://apod.nasa.gov/apod/" and "Imagen astronómica del día. (vía: NASA). ~Alfred" come to mind.

Then I'd go one step further and put all of the constants into a config file.  If you aren't interested in doing that, skip the rest of this section.

I assume that these may eventually change, or you might want to share/open-source your code and you probably don't want this out there publicly with all those values. Luckily, Python provides ConfigParser to handle config files.  I'd suggest the following structure for your config file

[TwitterBot]
url: http://apod.nasa.gov/apod/astropix.html
app_key: XXX
app_sec: XXX
token_key: XXX
token_sec: XXX
img_path: static/img/

Pretty standard, Windows INI file style config file.  This should either be in the same directory the file is in, or in the user's home directory.  Then you can get the parser object pretty easily - I encapsulated the behavior in a class, you can do whatever works for you.

from ConfigParser import SafeConfigParser

class Configuration(object):
    _parser = None
    _url = None
    _app_key = None
    _app_sec = None
    _token_key = None
    _token_sec = None
    _img_path = None

    def __init__(self, filepath=None):
        self.filepath = filepath

    @property
    def parser(self):
        if self._parser is None:
            self._parser = self._get_config_file(self.filepath)
        return self._parser

    @property
    def url(self):
        return self._get_config_value("url")

    @property
    def app_key(self):
        return self._get_config_value("app_key")

    @property
    def token_key(self):
        return self._get_config_value("token_key")

    @property
    def token_sec(self):
        return self._get_config_value("token_sec")

    @property
    def img_path(self):
        return self._get_config_value("img_path")

    def _get_config_value(self, value_name):
        """Gets a configuration value

        Parameters
        ==========
        value_name : str
            The name of the value you want.

        Returns
        =======
        value : str
            The value you want.
        """

        value = getattr(self, "_{}".format(value_name))
        if value is None:
            return self.parser.get("TwitterBot", value_name)
        return value

    def _get_config_file(self, filepath=None):
        """Gets the config file for the twitter bot

        Parameters
        ==========
        filepath: string, optional
            The path to the desired file.  If omitted, the function first
            looks in the local directory for a file named ".twbot.config",
            then looks in the user's home directory.

        Returns
        =======
        parser: SafeConfigParser
            A parser object with the config file loaded.
        """

        if filepath is None:
            true_filepath = ".twbot.config"
            if not os.path.isfile(true_filepath):
                true_filepath = os.path.expanduser("~/.twbot.config")
                if not os.path.isfile(true_filepath):
                    raise OSError("Couldn't find '.twbot.config' either "
                                  "locally or in the user directory")
        else:
            true_filepath = filepath

        parser = SafeConfigParser()
        with open(true_filepath, 'r') as config_file:
            parser.readfp(config_file)

        return parser

Use new style classes

Python allows you to use new-style classes (in 2.7) by inheriting from object - this is almost always something you want to be doing.

class Alfred(object):

Naming

I don't like the name start - it isn't very descriptive to me.  What are you starting?  It might be better with a more descriptive class name, but I think you can change it to something like startTweetingImages and be fine.

Break up your start function

Right now there are three distinct functions happening here - getting the image, deleting old files, and then tweeting the image.  I'd make a function for each of them.

def _get_image(self):
    img = self.soup.find("img")
    img_src = img.get("src")
    return img_src, img_src[11:]

def _remove_old_files(self):
    for file_name in os.listdir(self.img_path):
        os.remove(os.path.join(self.img_path, file_name))

def _send_tweet(self, img_src, img_name):
    img_url = self.base_nasa_url + img_src
    image_path = os.path.join(self.img_path, img_name)
    urllib.urlretrieve(img_url, image_path)
    with open(image_path, 'rb') as photo:
        response = self.tw.upload_media(media=photo)
        self.tw.update_status(
            status=self.status, media_ids=[response['media_id']])

def start(self):
    while True:
        if self.time[3] < 5:
            img_src, img_name = self._get_image()

            if not (os.path.isfile(os.path.join(self.img_path, img_name))):
                self._remove_old_files()
                self._send_tweet(img_src, img_name)

You'll notice I did a few other things - I removed the unnecessary else blocks, and then negated the condition of the if block so we could remove that else as well.  I also used a context manager, safely closing the file when I'm done with it.  Lastly, I used os.path.join to safely join together parts of a filepath so that it is operating system independent.

The above implementation assumes that you've made things constants that I mentioned should be constants, and that tw, web, and soup are each instance variables.

Sorry, I got lazy adding docstrings but you should always have them for functions. Sometimes you can omit them for private functions (prepended with a _) but I like them for the convenience of the implementor.  I've used the numpydoc style here.

Lastly, please put things that run (i.e. a = Albert(); a.start()) inside of an

if __name__ == '__main__':

block - this lets people import your module safely and do other stuff with it.


Account view with MVC and paging My Model

public class AccountListFilterModel
{
    public int page { get; set; }
    public int pagesize { get; set; }
    public string sortOrder { get; set; }
    public string packageTypeFilter { get; set; }
    public string usernameFilter { get; set; }
    public string accountStatusFilter { get; set; }
    public string nameSurnameFilter { get; set; }
    public DateTime startDate { get; set; }
    public DateTime endDate { get; set; }
    public string dateFilter { get; set; }
    public int screenCountFilter { get; set; }

}

My Controller

 public ViewResult Index(AccountListFilterModel accountListFilter){}

My View

@{
var currentPageNumber = Request["page"];
var currentPageSize = Request["pagesize"];
var currentSortOrder = Request["sortOrder"];
var currentUsernameFilter = Request["usernameFilter"];
var currentAccountStatusFilter = Request["accountStatusFilter"];
var currentPackageTypeFilter = Request["packageTypeFilter"];
var currentnameSurnameFilter = Request["nameSurnameFilter"];
var currentDateFilter = Request["dateFilter"];
var currentScreenCountFilter = Request["screenCountFilter"];
var currentScreenCountSortFilter = Request["screenCountSortFilter"];}

and

                Account Name

How can I shorten my code at View ?

You should pass your AccountListFilterModel to the view, either via model (which is the best solution as it's less error-prone) or using ViewBag \ ViewData (which is an anti-pattern actually, but fits the quick-fix niche).
Using the latter approach:

In your controller:

public ViewResult Index(AccountListFilterModel accountListFilter)
{
    ViewBag.Filter = accountListFilter;
}

In your view (you can delete the code at the top of the view, that uses Request to get parameters.

<a style="@(ViewBag.Filter.currentSortOrder == "UserName desc" ? "" : "display:none;")" ... />

It's a general rule of thumb that your view should not contain any logic at all, your controller should supply all the data that the view needs.


Convert #hashtagstylephrases to a proper list of words I'm writing a program to split the words contained in an hashtag.

For example I want to split the hashtags:

#Whatthehello #goback

into:

What the hello go back

import re,pdb
words = []
# corncob_lowercase.txt contains a list of dictionary words
with open('corncob_lowercase.txt', 'rb') as f:
    for read_word in f:
        words.append(read_word.strip())
def func_replace(each_func):
    i = 0
    wordsineach_func = []
    while len(each_func) > 0:
        i = i + 1
        word_found = longest_word(each_func)
        if len(word_found) > 0:
            wordsineach_func.append(word_found)
            each_func = each_func.replace(word_found, "")
    return ' '.join(wordsineach_func)

def longest_word(phrase):
    phrase_length = len(phrase)
    words_found = []
    index = 0
    outerstring = ""
    while index < phrase_length:
        outerstring = outerstring + phrase[index]
        index = index + 1
        if outerstring in words or outerstring.lower() in words:
            words_found.append(outerstring)
    if len(words_found) == 0:
        words_found.append(phrase)
    return max(words_found, key=len)

def check_hashtager(s):
    pdb.set_trace()
    return re.sub(r"#(\w+)", lambda m: func_replace(m.group(1)), s)

if __name__ == '__main__':
    print check_hashtager("#Whatthehello #goback")

I ran this on huge string which >10k hashtags. The function calls make it slow...

Edit 1:
There is something weird that happens when I ran it "#Whatthehello #goback #1". It doesn't stop.

At first, let me say that the code is somewhat neat and reads easily. A few nitpicks though, based upon PEP8:

each import statement should stand on its own line;
you should put blank lines before function declarations;
you should improve your naming, first by putting more _ in your variable names to separate the words, and second by choosing more meaningful names.

Debugging

You should not need to keep debugging directive in your code now. Remove the pdb module.

Global variables

And top-level code

You are aware of the if __name__ == '__main__' practice but you still keep code at top-level. Worse, you rely on your top-level code to populate a global variable… What if I want to import your_file and use your_file.check_hashtager with my own dictionary of french words?

Better let your functions accept the dictionary as parameter and let the user provide the filename:

def partition_hashtag(text, dictionary_filename):
    with open(dictionary_filename) as f:
        words = [word.strip() for word in f]
    return re.sub(r'#(\w+)', lambda m: partitioner(m.group(1), words), text)

A note on variable names: check_xxx has a meaning of validation only, leading the user to expect a boolean return value (or an error raised on validation error). I considered split_hashtag but I feel like partition_hashtag works best, since it is associated with a dictionary to partition from. partitioner is just a logical consequence.

Partitioning words

Let's talk about longest_word a bit for starter:

you check words of increasing length so there is no need to keep them all and retrieve the longest afterwards, just return the last one you found ;
you can iterate over the letters of phrase directly instead of retrieving them by indexes; it's faster.

def longest_word(phrase, words):  # accepting the dictionary of words as parameter
    word_found = None
    outer_string = ''
    for letter in phrase:
        outer_string += letter
        if outer_string in words or outer_string.lower() in words:
            word_found = outer_string
    return word_found

You’ll need to change if len(word_found) > 0 in func_replace by if word_found is not None or even if word_found which you should have used from start.

But there still exist issue with that code. Concatenating strings in a for loop is not the fastest thing in the world, you might want to consider using

for i in range(1, len(phrase)):
    outer_string = phrase[:i]

instead. But more importantly, checking existence in a list is an O(n) operation, n being the length of the list; you will have a faster execution if words were a set instead because the same operation runs in O(1).

One last thing to note is that you are checking shorter words before longer ones. If you were to try your phrase by decreasing length, the first matching word would be the one you are looking for:

def longest_word(phrase, words):
    current_try = phrase
    while current_try:
        if current_try in words or current_try.lower() in words:
            return current_try
        current_try = current_try[:-1]

    # if nothing was found, return the original phrase
    return phrase

Now let's tackle how this code is called.

First of, what is this i exactly? It is incremented but never used. Second, you made sure in longest_word to return words of at least 1 letter so you don't need to test the length. Third, you don't need to modify your input string in place but only drop its head corresponding to the word you just found.

def partitioner(hashtag, words):
    extracted_words = []
    while hashtag:
        word_found = longest_word(hashtag, words)
        extracted_words.append(word_found)
        hashtag = hashtag[len(word_found):]
    return ' '.join(extracted_words)

Introducing generators

What if partitioner produced only the list of words? We could call the join operation out of this function, like

lambda m: ' '.join(partitioner(m.group(1), words))

This let us focus on only producing words efficiently inside partioner.

Even though the proposed functions reads clearer and should execute faster, there is that list-building-out-of-a-loop thing that is discouraged in Python. Instead we try to come up with list comprehensions to build it faster (plus it reads better). Meaning we usually turn

a_list = []
for element in an_other_list:
    a_list.append(some_function(element))

into

a_list = [some_function(e) for e in an_other_list]

Here the while loop has side-effects on hashtag so we can not directly use that. But…

We are not actually interested in a list of words. We are only interested in getting words one by one to feed them into join. And Python lets you do exactly that by turning your function into a generator: use the yield keyword instead of return. Each time the control flow reach a yield keyword, a result will be returned to the caller. And if the caller ask for more, the control flow will resume after the exact same yield, remembering its old state.

Proposed improvements

import re

def partitioner(hashtag, words):
    while hashtag:
        word_found = longest_word(hashtag, words)
        yield word_found
        hashtag = hashtag[len(word_found):]

def longest_word(phrase, words):
    current_try = phrase
    while current_try:
        if current_try in words or current_try.lower() in words:
            return current_try
        current_try = current_try[:-1]
    # if nothing was found, return the original phrase
    return phrase

def partition_hashtag(text, words):
    return re.sub(r'#(\w+)', lambda m: ' '.join(partitioner(m.group(1), words)), text)

def read_dictionary_file(filename):
    with open(filename, 'rb') as f:
        return set(word.strip() for word in f)

if __name__ == '__main__':
    words = read_dictionary_file('corncob_lowercase.txt')
    print partition_hashtag("#Whatthehello #goback", words)


Check if a Binary Search Tree is valid I've written the following code in Ruby:

class Node
  attr_accessor :data, :left, :right

  def initialize(data=nil, left=nil, right=nil)
    @data  = data
    @left  = left
    @right = right
  end

  def valid?(min=nil, max=nil)
    if @left.nil? && @right.nil?
      # base case (leaf node)
      return false if min && @data < min
      return false if max && @data >= max
      true # otherwise
    else
      return false if @left && !@left.valid?(min, @data)
      return false if @right && !@right.valid?(@data, max)
      true # otherwise
    end
  end
end

The basic idea is that when you turn left (i.e. recurse into left child), the children should be less than the value of the current node (i.e. you're setting a max = @data limit for the children).

To test it, I did:

root =
  Node.new(10,
    Node.new(5,
      Node.new(2), Node.new(7)),
    Node.new(15,
      Node.new(8), Node.new(20)))

puts root.valid?.inspect # => false (because 8 isn't between 10 -> 15)

Seems to work for me, but would love any feedback (everything's fair game, including style, etc.)

Having to pass - or even being able to pass - min and max to the method doesn't seem quite right. The arguments are only there for the method's own use when calling "itself" on its child nodes - they're not really part of the class's API. So that smells a bit.

I'd take a different approach, and add an #each method:

def each(&block)
  left.each(&block) unless left.nil?
  yield data
  right.each(&block) unless right.nil?
end

This lets you say root.each { |n| ... } like you would with an array. And it lets you include Enumerable - since all that requires is that your class implements an #each method - giving you a bunch of methods for free. For instance, you might implement #valid? like so:

def valid?
  each_cons(2).all? { |a, b| a <= b }
end

And of course, thanks to Enumerable, you can also do things like root.to_a, root.reduce, root.include? and other good stuff.

In all, you get this:

class Node
  include Enumerable

  attr_accessor :data, :left, :right

  def initialize(data = nil, left = nil, right = nil)
    @data = data
    @left = left
    @right = right
  end

  def each(&block)
    left.each(&block) unless left.nil?
    yield data
    right.each(&block) unless right.nil?
  end

  def valid?
    each_cons(2).all? { |a, b| a <= b }
  end
end

Beyond that, your current code looks good, only a few comments:

Since you have attr_accessor synthesized methods, use them. Don't access instance variables directly with @... unless you have to. The point is, that if you need to customize access, you can simply swap out the synthesized accessor with a custom one, and no other code has to change, since it'll still be calling the method.
But speaking of accessors, using attr_accessor can be a bit blunt for the left and right attributes, since it lets anyone, anywhere, set them to something nonsensical, like root.left = "foobar!". That'll break things, since you suddenly have non-Node nodes in your tree. So I'd suggest using attr_readers, but adding your own assignment methods, e.g. def left=(node); ...; end. That'll let you check things before assigning.
The method in question might be named #sorted? instead. valid? could imply any number of things. Edit: You may actually want to use (a <=> b) <= 0, as that's what used to sort objects, instead of just a <= b.
Don't bother aligning the = assignment operators like you do in the initializer. Yeah, it's pretty, and I'm often tempted to do the same, but it's a bit of busywork that - if you ever have to change the code - requires you to redo it. I've read through my own commit history on projects and wondered why seemingly unchanged lines showed up in the diff, only to realise I was messing around with the whitespace and confusing future-me.

You can also take a different tack, and use an add/insert/<< or something method to add data to the tree, rather than requiring someone else to create new Nodes. E.g root << 15 would traverse the tree, and find the right place to insert the value, creating a new node to hold it, etc. etc.. That way, you won't really need a valid? method.


Similar manipulations on several numpy arrays I have n numpy arrays which all contain time series data for t periods. It is not an option to instead have one nxt matrix. I am storing all these arrays in a ResultSimulation object.

I seem to have repetitive code, which I am wondering how to shorten.

It starts with

ResultSimulation.v0 = ResultSteadyState.v0[::-1].copy()
ResultSimulation.v1 = ResultSteadyState.v1[::-1].copy()
ResultSimulation.nl = ResultSteadyState.nl[::-1].copy()
ResultSimulation.nh = ResultSteadyState.nh[::-1].copy()
ResultSimulation.ul = ResultSteadyState.ul[::-1].copy()
ResultSimulation.uh = ResultSteadyState.uh[::-1].copy()
ResultSimulation.V1 = ResultSteadyState.V1[::-1].copy()

at some place there is

# nTSteady is some number
ResultSimulation.v0 = ResultSimulation.v0[:nTSteady]
ResultSimulation.v1 = ResultSimulation.v1[:nTSteady]
ResultSimulation.nl = ResultSimulation.nl[:nTSteady]
ResultSimulation.nh = ResultSimulation.nh[:nTSteady]
ResultSimulation.ul = ResultSimulation.ul[:nTSteady]
ResultSimulation.uh = ResultSimulation.uh[:nTSteady]
ResultSimulation.V1 = ResultSimulation.V1[:nTSteady]

and finally,

addZeros = np.zeros((additionalSize,))
ResultSimulation.v0 = np.concatenate((ResultSimulation.v0, addZeros))
ResultSimulation.v1 = np.concatenate((ResultSimulation.v1, addZeros))
ResultSimulation.nl = np.concatenate((ResultSimulation.nl, addZeros))
ResultSimulation.nh = np.concatenate((ResultSimulation.nh, addZeros))
ResultSimulation.ul = np.concatenate((ResultSimulation.ul, addZeros))
ResultSimulation.uh = np.concatenate((ResultSimulation.uh, addZeros))

I guess one way to reduce this repetitive code, is to use "variable variable names", a practice that isn't encouraged in Python. Another alternative is to store them not directly in the object, but in an intermediate dict:

for series in ResultSimulation.myDict:
    ResultSimulation.myDict[series] = ResultSteadyState.myDict[series].copy()

for the first case. Say I don't like these two options+. What else can I do?

+: I dont mind creating an intermediate dictionary, but I want to keep my numpy arrays as properties of the object, and not stored in a dictionary within the object.

You can get the behavior you want using getattr and setattr, this should do the trick for your first example:

for attr in ('v0', 'v1', 'nl', 'nh', 'ul', 'uh', 'V1'):
    setattr(ResultSimulation, attr,
            getattr(ResultSimulation, attr)[::-1].copy)

It should be obvious how to modify the others to fit the same pattern.


Weapon and Hero classes For me as a complete newbie to the world of C++, getters and setters are a rather confusing topic. There seems to be a wide range of opinions on what constitutes a sound use of getters and setters. In this particular post I'm most interested in finding a good way of using getters and setters in a memory-safe way.

I hacked up a little example. Consider this epic tale of a king and his magic sword:

#include <iostream>
#include <string>

using std::string;

class Weapon {
  private:
    string name;
  public:
    Weapon(string n = "rusty sword") : name(n) {
      std::cout << "weapon '" << name << "' constructed" << std::endl;
    }
    string getName(){
      return name;
    }
    ~Weapon(){
      std::cout << "weapon '" << name << "' destroyed" << std::endl;
    }
};

class Hero {
  private:
    string name;
    Weapon weapon;
  public:
    Hero(string n) : name(n) {
      std::cout << "hero '" << name << "' constructed" << std::endl;
      describeWeapon();
    }
    void describeWeapon() {
      std::cout << name << " now swings " << weapon.getName() << std::endl;
    }
    void setWeapon(Weapon w) {
      weapon = w;
      describeWeapon();
    }
    ~Hero() {
      std::cout << "hero '" << name << "' destroyed" << std::endl;
    }
};

int main() {
  Hero h = Hero("Arthur");
  Weapon ex = Weapon("Excalibur");
  h.setWeapon(ex);
  std::cout << "Ok, now all objects will go out of scope" << std::endl;
  return 0;
}

This code yields the following output:

// our hero is given a rather modest sword to start with
weapon 'rusty sword' constructed
hero 'Arthur' constructed
Arthur now swings rusty sword

// Excalibur is created in the main scope
weapon 'Excalibur' constructed
Arthur now swings Excalibur   // since we passed the object by value, Arthur now actually owns a COPY of the original Excalibur.
weapon 'Excalibur' destroyed  // This is strange! Who got destroyed here?

Ok, now all objects will go out of scope
weapon 'Excalibur' destroyed  // Here, Arthurs copy is being destroyed.
hero 'Arthur' destroyed       // Arthur himself bites the dust.
weapon 'Excalibur' destroyed  // And finally, the original sword leaves this world.

Now, there are a few obvious problems in this piece of code:

For one, it is not ideal that we have the original Excalibur in the main-scope, while Arthur is only passed a copy. But what would be a better solution? Passing a pointer to Excalibur comes to mind, but is that the right way to go here?
It also seems like just overwriting Arthur's old weapon with the new one passed in the setter causes some erratic behaviour. Consider the last line in the second paragraph of the output: here, allegedly Excalibur gets destroyed, while I believe that it is much rather the rusty old sword that Arthur owned originally that gets demolished.

The vast majority of literature about getters and setters deals with primitives, so any help on best practices for passing objects (or pointers or references to objects) into setters would be highly appreciated!

This is a design issue, and there's no single answer.

We need to consider what's important about a Weapon.  Is it value-like, or is its identity important?  The real-world analogy is, "are copies all equivalent?".  The answer to this lies partly in the type, but also in how we intend to use it.

In the real world, Excalibur has an identity.  You can't copy it, but if you could, then each copy would have its own identity, and be owned by one person (let's ignore shared ownership, and losing it in the lake).  The same couldn't be said for the name (or the colour) of Excalibur - you can write the name as many times as you like, and they are all equal.  If you rename Excalibur to "a light-sabre", it doesn't change all the places you've written "Excalibur".

In cases where object identity is important, it's normal to delete the copy constructors and assignment operators:

class Weapon {
    // not copyable
    Weapon(const Weapon&) = delete; // also removes Weapon(Weapon&&)
    Weapon& operator=(const Weapon&) = delete;

    // instance data
    std::string name;

public:
    Weapon(std::string n = "rusty sword");
    ~Weapon();

    std::string getName();
};

Now we can't copy a Weapon.  We might be able to create another like it, but the two would be distinct.

Our hero needs to be assigned a Weapon.  Let's assume that giving Excalibur to Arthur means that he owns it, until it's destroyed or he gives it to someone else:

#include <memory>
class Hero {
    std::unique_ptr<Weapon> weapon;

public:
    void describeWeapon() {
      std::cout << name << " now swings " << weapon->getName() << std::endl;
    }
    void setWeapon(std::unique_ptr<Weapon>&& w) {
      weapon = std::move(w);
      describeWeapon();
    }
    ~Hero();
};

If the ownership is shared (perhaps it also belongs to a pool of weapons that secretly communicate with each other), then we'll want to use std::shared_ptr<Weapon> instead of a unique pointer.

One thing we need to be careful of is weapon->getName() in the code above.  If we're using pointers rather than objects or references, we'll need to be aware that we could be passed a null pointer.  We can specify that callers must not pass us null pointers, but calling code is notorious for forgetting such contracts.  An alternative is the Null Object pattern, where we create a no-effect weapon to stand in place of the null pointer.

We can arrange that like this:

class Hero {
    std::string name;
    const std::shared_ptr<Weapon> no_weapon
        = std::make_shared<Weapon>(name + "'s bare hands");
    std::shared_ptr<Weapon> weapon = no_weapon;

public:
    Hero(std::string n)
        : name(n)
    {
        std::cout << "hero '" << name << "' constructed" << std::endl;
        describeWeapon();
    }

    void describeWeapon()
    {
        std::cout << name << " now swings " << weapon->getName() << std::endl;
    }

    void setWeapon(const std::shared_ptr<Weapon>& w)
    {
        weapon = w ? w : no_weapon;
        describeWeapon();
    }
    ~Hero()
    {
        std::cout << "hero '" << name << "' destroyed" << std::endl;
    }
};

A further consideration, that's specific to C++: the constructors that take a single argument are candidates for conversions.  You could have written:

Hero h = "Arthur";
h.setWeapon("The Holy Hand Grenade of Antioch");

and those strings would be converted to new objects.  We don't want that to happen, so we should probably declare the constructors explicit.


Php/Doctrine array hydration According to the Doctrine docs, you should use Array hydration rather than record hydration when retrieving data for read-only purposes.

Unfortunately, this means I have to use array syntax (as opposed to object syntax) to reference the data.

$q = Doctrine_Query::create()
    ->from('Post p')
    ->leftJoin('p.PostComment pc')
    ->leftJoin('pc.User u')
    ->where('p.post_id = ?', $id);

$p = $q->fetchOne(array(), Doctrine_Core::HYDRATE_ARRAY);

...

foreach ($p['PostComment'] as $comment) {
    $this->Controls->Add(new CommentPanel($comment['text'],
                         $comment['User']['nickname'],
                         $comment['last_updated_ts']));
}

Maybe it's just me, but all of those string literals as array indexes are kinda scary.  Does anyone have some ideas for cleaning this up?

Scary?  In what way?  I don't really get that.

It's just syntax.  If you really care, just cast the arrays as stdClass objects

foreach ( $p['PostComment'] as $comment )
{
  $comment = (object) $comment;
  $this->Controls->Add( new CommentPanel(
      $comment->text
    , $comment->User->nickname
    , $comment->last_updated_ts
  ));
}


PHP Application layers I'm dealing with design layers of an application. Basically what I've so far are:

Business
Architecture: Client-Server
Techonologies: PHP+MySQL, HTML5, JS, 3rd parties APIs

My app

Data Sources: MySql databases, 3rd party APIs
Data Layer: Interface with CRUD operations, PDO for accessing DBs, Classes implementing CRUD interface, Classes for communication with APIs
Repository: Classes & Interfaces with bussiness methods for accessing datasource.
Business object: value objects and business rules
Application layer: controls flow between Data Layer and Presentation.
Presentation layer: html design with javascript & ajax & php code. Interacts with application layer for data retrieval

Implementation

Data Source

1x MySQL Database

1x 3rd party api

Data Layer

interface DSOps
{
    /*
    * @return array
    */
    function read($sql);

    /*
    * @return array
    */
    function readAll($sql);

    /*
    * @return boolean
    */
    function save($sql);

    /*
    * @return boolean
    */
    function delete($sql);
}

use PDO;

class DSSql implements DSOps
{
    private $bd;

    public function __construct( PDO $bd)
    {
        $this->bd =  $bd;
    }

    public function read($sql)
    {
        $stmt=$this->bd->prepare($sql);
        $stmt->execute();
        $response = $stmt->fetch(PDO::FETCH_ASSOC);
        return $response;
    }

    public function readAll($sql)
    {
        $stmt=$this->bd->prepare($sql);
        $stmt->execute();
        $response = $stmt->fetchAll(PDO::FETCH_ASSOC);
        return $response;
    }

    public function save($sql, $params)
    {
        $stmt=$this->bd->prepare($sql);
        return $stmt->execute($params);

    }

    public function borrar($sql, $params)
    {
        $stmt=$this->bd->prepare($sql);
        return $stmt->execute($params);

    }
}

Repositories:

interface QueryClients
{
    /*
    * @return Client
    */
    public function searchById($id);

    /*
    * @return array()
    */
    public function searchAll();

    /*
    * @return int
    */
    public function count();

}

class RepoClients implements QueryClients
{
    private $ds;

    public function __construct(DSOps $_ds)
    {
        $this->ds = $_ds;
    }

    /*
    * @return array
    */
    public function searchById($id)
    {
        $sql = " SELECT  * FROM clients WHERE id=$id;";
        $response=$this->ds->read($sql);
        return $response;
    }

    /*
    * @return array()
    */
    public function searchAll()
    {
        $sql = " SELECT * FROM clients; ";
        $response=$this->searchAll($sql);
        return $response;
    }

....
}

I've multiple modules that implements the same design. When implementing these design I have the following doubts:

Is correct to form queries at repository classes?
Where should I handle SQL errors? Having an SQLError
How should I handle access to APIs?
As my app has role based access for users, how does this impacts my
design/implementation? Based on logged user role I've create
different classes using same interface but implementing different
queries to database.

SQL injection

First of all, your PDO code is a straw plane that never flies. Using prepare() like that makes very little sense, leaving all your queries widely exposed to SQL injection attacks.

prepare() doesn't protect your queries by itself, making them immune by means of some magic. It helps only if you take out every single variable from your query and substitute it with a placeholder, then prepare your query and then finally execute, adding variables separately.

Therefore, all methods in DSSql should be changed in order to accept two variables - a query and an array with data:

public function read($sql, $params = [])
{
    $stmt=$this->bd->prepare($sql);
    $stmt->execute($params);
    $response = $stmt->fetch(PDO::FETCH_ASSOC);
    return $response;
}

Code duplication

As you may notice, the code in DSSql class' methods is heavily duplicated. Why not to create a basic method to run an arbitrary query, an then call this method with only different return formats?

public function query($sql, $params = [])
{
    $stmt=$this->bd->prepare($sql);
    $stmt->execute($params);
    return $stmt;
}

public function read($sql, $params = [])
{
    return $this->query($sql, $params)->fetch(PDO::FETCH_ASSOC);
}

public function readAll($sql)
{
    return $this->query($sql)->fetchAll(PDO::FETCH_ASSOC);
}

public function save($sql, $params)
{
    return $this->query($sql, $params);
}

Existing PHP classes' code reuse

As you may notice, your methods now bear very little difference. Why not to take them out at all and make use of the powerful PDOSTatement class already exists in PHP?

public function searchById($id)
{
    $sql = "SELECT  * FROM clients WHERE id=?";
    return $this->ds->query($sql, [$id])->fetch();
}

public function searchAll()
{
    $sql = "SELECT * FROM clients";
    return $this->query($sql)->fetchAll();
}

ORM

The code in the save() method is not really does the save but rater runs an arbitrary query, which could be quite confusing. If you want a real save, read and such methods, consider using an ORM, a class that constructs SQL queries for you, and in such a case it will be indeed save() or find() with no other meaning

Questions

  Is correct to form queries at repository classes?

yes, it's the place where SQL belongs

  Where should I handle SQL errors? Having an SQLError

Wherever you are handling all other errors. There is nothing special in SQL errors, handle them the same way as others.

  How should I handle access to APIs?

That is somewhat too wide question for a comment, especially without any context given. May be you should ask on Stack Overflow.

  As my app has role based access for users, how does this impacts my design/implementation? Based on logged user role I've create different classes using same interface but implementing different queries to database.

Well, this is somewhat a key question. Consider using an existing framework instead of rolling up a your own. Established frameworks already have all you need out of the box, including ORM, a template engine and of course a user management module with role based authorization. I would suggest Symfony as being the most popular PHP framework that encourages best coding practices.


Deleting all the matching items in single linked list I have the following code which I implemented to delete all the nodes for the matching value. My code handles all the 63 varieties of test case scenarios including scenarios like deleting 1 from linked list 1->1, leaving the linked list empty.

  public ListNode RemoveElements(ListNode head, int val) {
      if(head != null)
    {
        ListNode current = head;
        ListNode prevNode;
        while(current !=null && current.next != null)
        {
            prevNode = current;
            if(current.val == val)
            {
                current.val = current.next.val;
                current.next = current.next.next;
            }
            else
                current = current.next;
        }

        int count = 0;
        ListNode p = head;
        ListNode pBefore = null;
        while(p != null)
        {
            if(p.next != null)
                pBefore = p;
            p = p.next;
            count++;
                        }

        if(current.val == val)
        {
            if(count == 1)
             head = null;
             else if(count > 1)
                pBefore.next = null;
        }

    }
    return head;

}

I'm sure this can be optimized and it would be great if someone can share the best way to delete all the matching items in single linked list.

I tried searching on Google, but everywhere it is handled as part of a SingleLinked list custom class having head and tail node access. In my case, I am handling in a method and I have access only to the head of the list through the input parameter.

Several issues jump in the eyes:

very complicated logic
very strange "removal" logic: copy value from next node + delete next node
is recommended to use braces even on single statement if-else
messy indentation

This can be done a lot simpler, by using a dummy node:

ListNode dummy = new ListNode();
dummy.next = head;

ListNode runner = dummy;
while (runner.next != null)
{
    if (runner.next.val == val)
    {
        runner.next = runner.next.next;
    }
    else
    {
        runner = runner.next;
    }
}

return dummy.next;


Python 3.6+ Conventions for fetching paginated data I find myself often encountering a use case where I need to fetch paginated data, where I need all of the data. Currently I use something similar to below to handle these cases:

def fetch_paginated_data(api_base, query, good_citizen_rate):
    encoded = urllib.parse.quote(query.encode('utf-8'))
    initial = f'{api_base}?q={encoded}'
    payload = {
        'has_more': True,
        'next_page': initial
    }

    storage = []

    while payload['has_more']:
        response = requests.get(payload['next_page'])
        if response.status_code != 200:
            raise ValueError('Status not 200')
            payload['has_more'] = False
            return

        data = response.json()
        if (data and data['data']):
            storage.extend(data['data'])

        payload['has_more'] = data['has_more']
        if 'next_page' in data:
            payload['next_page'] = data['next_page']
        time.sleep(good_citizen_rate)
    return storage

My question is how could I better make this convention more reusable? Should I use async / await? yield pages? Add a callback to transform the data prior to adding it to the collection variable?

You should let the requests module do the urlencoding for you. It can take a dictionary of parameters and do it right.

Instead of having a variable around which you check in your while condition, I would prefer a while True and an explicit break here. The loop is small enough that this makes it more readable IMO.

I would indeed use yield, or rather yield from here, in order to flatten the response. This means that the next page is only fetched when you have consumed the previous data. If you need to, you could also make it asynchronously using aiohttp, have a look at this question of mine which used it or the documentation I linked.

You can also use response.raise_for_status to raise an exception if the status code is not 2XX. Note that any code after a raise is not accessible, so your payload['has_more'] = False and return will never run (unless you run it in a debugger or something).

Python has an official style-guide, PEP8, which you are following (nice!). One thing it does not recommend but I would nevertheless encourage you to do is not using unnecessary parenthesis in if statements. Python is not C.

def fetch_paginated_data(url, query, timeout=0):
    params = {"q": query}
    while True:
        response = requests.get(url, params=params)
        response.raise_for_status()
        data = response.json()
        if data and data['data']:
            yield from data['data']
        if not data['has_more']:
            break
        url = data['next_page']
        time.sleep(timeout)

Here I left the interface as is. If you are free to change it, I would consider taking params directly as an argument or even collect all keyword arguments (except for the timeout, which I also gave a defualt value) directly into a dictionary with **params. The latter would give you the very nice interface:

data = fetch_paginated_data("example.com", q="FooBar", timeout=3)

Instead of if data and data['data']: you could also just use a try..except block. This will be slightly more performant if the response usually has this key:

try:
    yield from data['data']
except (KeyError, TypeError):  # for dict, list
    pass

You should definitely add a docstring describing what this function does. Especially your naming the timeout a rate can be confusing for other people reading your code, or yourself in half a year.


PHP my way of threating static classes A few days ago I've faced an annoying problem.
Let's imagine we have 3 classes: Base, System, Handler.

The classes Systen and Handler both inherit from the class Base.
I want to access those classes this way: System::DoStuff();
I also need the Handler be available in the System, but do not want to instantiate it there, use any kind of reference injection.

I could simply design the System and the Handler class to be static. The problem would be:
They need to inherit form the Base class, which should be static too, because if it's not, it won't work with the System and Hanlder because they ARE static. If is set Base to static, it won't work either, because once a child changes some parameter in it's parent, the parent would change for ALL children.

After some testing, I've came up with this solution:

class Wire
{
    //List of classes I'll have to load
    private static $load = array("Foo", "Hello");

    //Template of the Bootstrap class
    private static $template = NULL;

    //Wire everything up
    public function Up()
    {
            //Get the bootstrap template
        self::GetTemplate();

            //Get each class
        foreach(self::$load as $class)
        {
                    //Load it...
            require_once "./class/" . $class . ".php";

                    //Build the static shell
            $bootstrap = str_replace("%CLASS%", $class, self::$template);
            eval($bootstrap);
        }
    }

    //Get the tempalte of the bootstrap class
    private function GetTemplate()
    {
        if(self::$template === NULL)
        {
                    //I've stored it in a separate file
            self::$template = file_get_contents("./class/Bootstrap.php");
        }
    }
}

Wire::Up();

I'me using the Wire class to load the classes I need. And use this Bootstrap file to create a static shell for each instatiated class.

//This is the Bootstrap class
//It offers a static shell for other instatiated classes
class %CLASS%
{
    public static $instance = NULL;
    public function __callStatic($method, $arguments)
    {
        if(self::$instance === NULL)
        {
            $class = get_called_class() . "Controller";
            self::$instance = new $class();
        }
        call_user_func_array(array(self::$instance, $method), $arguments);
    }
}

That's the Base class, for testing purpouses only

class Base
{
    public $value = 1;
}

Here are the classes Foo and Hello that I've used above

class FooController extends Base
{
    public function Bar($args)
    {
        print $args . " " . $this->value . "
";
    }
}

class HelloController extends Base
{
    public function World($args)
    {
        $this->value = 10;
        print $args . " " . $this->value . "
";
    }
}

Now, if you call

Hello::World("Hello World") //Hello World 10
Foo::Bar("Foo Bar") //Foo Bar 1
//If Foo and Base would be static, whitout the Bootstrap shell, you would see "Foo Bar 10"

What do you think about that solution? Am I hunting a mouse with a shotgun? Or maybe there are better solutions, for this scenario? I'd appreciate costructive critics!!

I don't know if I'm just not understanding your situation properly or what, but I don't think converting three classes to static for one static method is really the right thing to do here. Why would you need to call System::DoStuff() statically? Why not just $system->DoStuff()? I would refactor the one method before I refactored the three classes. At most I would convert your base class to an abstract class to prevent its instantiation and I would define the value for the $value property in each individual class.

abstract class Base {
    public $value;
}

class FooController extends Base {
    public $value = 1;

    //etc...
}

class HelloController extends Base {
    public $value = 10;

    //etc...
}

You should use doccomments instead of those normal comments. They're more useful and can be read by your IDE.

/** List of classes I'll have to load */
private static $load = array( 'Foo', 'Hello' );

"Up" is not a very descriptive method name. What is it supposed to do? From your comments and your code it looks like its instantiating the class. So normally this would be in the constructor. I would suggest renaming this to something like instantiate(), or maybe getInstance(); And then I would make this a private method, or at the very least ensure that it hasn't already been instantiated so you don't end up doing it again.

eval() is evil. No seriously, it is. It is a horrible security risk. The only saving grace in this instance is that you have not written this in such a way that user input could be used. If you need to dynamically load a class, then you should just do so like this:

foreach( $load AS $class ) {
    require "./class/$class.php";
    $instance = new $class();
}

The *_once() versions of require and include should be avoided if at all possible. With a small number of files it isn't as noticeable, but PHP has to run a special check on each request to ensure that the file hasn't already been included, this means that it will run slower. Again, this is negligible in this case, but it could become an issue later.

I hope this helps, but I really don't know if I understood your issue well enough.


Calculate area module I wrote a module that calculates the area of shapes and I wanted to get feedback. Am I on the right track?

import math

# A python program to calculate the area of shapes

# area of a square is length multiplied by width
def area_of_square(length, width):
    _check_if_values_are_strings(length = length, width = width)
    _check_if_values_are_negitive(lenth = length, width = width)

    return length * width

# area of a triangle is base multiplied by height divided by 2
def area_of_triangle(base_value, height):
    _check_if_values_are_strings(base_value = base_value, height = height)
    _check_if_values_are_negitive(base_value = base_value, height = height)

    return (base_value * height) / 2

# area of a circle is radius squared times pi
def area_of_circle(radius):
    _check_if_values_are_strings(radius = radius)
    _check_if_values_are_negitive(radius = radius)

    return math.pow(radius,2) * math.pi

def _check_if_values_are_strings(**values):
    for k,v in values.items():
        if (isinstance(v,str)):
            raise TypeError("{0} is a string".format(k))

def _check_if_values_are_negitive(**values):
    for k,v in values.items():
        if v < 0:
            raise ValueError("{0} is a negitive".format(k))

My main concern is the use of the _check_if_values_are_strings and _check_if_values_are_negitive methods. Am I using them correctly, or is there a better approach to verifying the parameters before preforming the calculation?

A few things:

Squares don't have width and height, those are rectangles
You check to see that your values are not strings, what if they are lists (or other types of objects)?
Your check functions error if their title is true, this is unintuitive. Something like ensure_value_is_positive would be a better name.


jQuery and cross browser input fields focus handling I have a form and need to be able to highlight/decorate fields as they gain focus (change their background color, border). I'm also decorating their corresponding 'labels'.

All works fine where it needs to: FF, Safari, IE 7, 8 and 9

Problem 1 cropped up with IE7 and changing background color of drop down box. There I had to resort to plain old JS to access the element and use 'onfocusin' and 'onfocusout' events otherwise it'd take 2 clicks to use the field.

Problem 2 was with Safari and how it handles 'focus' event with radio buttons. For that I'm using 'mouseenter' and 'mouseleave' events to change the color of their 'labels'.

Is there a cleaner way of doing this? The caveats are I have no control of the HTML whatsoever and I can't use anything other than jQuery 1.4.3 That's all dictated by content management system I must work with.

Here's a complete 99.9% 'production' code example: http://jsfiddle.net/HenriPablo/YvDPa/3/

Script in question:

$(document).ready( function(){

    /* PROFILE PAGE ONLY -> get rid of separator border on shipping addres block when editing any of the shipping addresses */
    if( $('#formShippingAddress').length > 0 ){
        $( 'table#block\\.crmshipping tbody tr td.block\\.crmshipping\\.text' ).css('border','none');
    }

    /* highlight input field labels when input field has focus */
    /* see if event bubble - this is critical for handling decoration of select drop down boxes in IE7 */
    if( $.support.changeBubbles){

        var partialID = 'table[id^="page"]'
        var focusable = $( partialID + ' input[type="text"], ' + partialID + ' input[type="password"], ' + partialID + ' select, ' + partialID + ' input[type="checkbox"]' );
        var clickable = $( partialID + ' input[type="radio"]' );

        focusable.focus( function( ){   decorate( $(this) ); })
        focusable.blur( function( ){ decorate( $(this) ); })

        clickable.mouseenter( function(){ decorate( $(this) ); })
        clickable.mouseleave( function(){ decorate( $(this) ); })

    } else { // bubbling support check

        var i, d = document.getElementsByTagName('select')

        for( i in d ){
            d[i].onfocusin = function() {
                this.style.backgroundColor = '#b4d5ec'
                $(this).closest("td").prev().addClass('highlightLabel');
            }
            d[i].onfocusout= function() {
                this.style.backgroundColor='#ffffff'
                $(this).closest("td").prev().removeClass('highlightLabel');
            }
        } // for

        $('table[id^="page"] input' ).focus( function( ){
            decorate( $(this) )
        })

        $('table[id^="page"] input').blur( function( ){
            decorate( $(this) );
        })
    } // handle  IE7

    function decorate( elm ){
        elm.toggleClass('highlightField');
        elm.closest("td").prev().toggleClass('highlightLabel');
    } // decorator function
});

Many thanks in advance :-)

I'm not quite sure if this will do quite what you want, but here is my rehashing of the code:

$(document).ready(function() {
    $('table[id^="block"] input, select').bind("focus blur", function() {
        var $this = $(this);
        $this.closest("td").prev().toggleClass('highlightLabel');
        $this.not("select").toggleClass('highlightField');
    });
});

You can see it in action here on jsFiddle.

As far as I can tell, the only thing I changed, behavior-wise, is that the select dropdown list does not have a background color when focused. IMO, it looks better that way, but you might prefer it with the background color.

It seems to work the same on all the browsers I tested, but let me know if it doesn't and I'll look at it and see if I can fix it.