Custom Error Classes in OOP I have a class which parses a .csv file and converts it into successful rows and errors. Currently I have 12 different kind of errors (mail is not valid, missing column x, row is duplicate etc.).

Each error has a custom message. I also need to know in other classes what kind of error a row was, so a simple string is not enough. So far I am using this error class:

<?php

namespace App\Registration\GroupUploads;

/**
 * Represents all possible errors that may occur during a group upload.
 */
class Error
{

  const USEREXISTS  = 7;
  const MAILINVALID = 1;
  const SEALUSED    = 2;
  const MIXEDUP     = 3;
  // ..

  protected $id;
  protected $options;

  /**
   * Each error has a unique ID
   * @param int $id
   * @param array $options optional
   */
  protected function __construct($id, $options = null)
  {
    $this->id      = $id;
    $this->options = $options;
  }

  /**
   * Get Message of current object
   * @return string
   */
  public function getMessage()
  {
    switch ($this->id) {
      case static::USEREXISTS:
        return 'User already exists';
        break;

      case static::MAILINVALID:
        return 'Mail is not valid';
        break;

      case static::SEALUSED:
        return 'Sealnr is already used by other user.';
        break;

      case static::SEALUSED:
        return 'They messed up and mixed the seals inbetween orgas.';
        break;

      // ...

      default:
        return 'No message provided for error code';
        break;
    }
  }

  /**
   * Create error for an existing user
   * @return bool
   */
  public static function getDuplicateUserError()
  {
    return new static(static::USEREXISTS);
  }

  /**
   * Check if class is duplicate user error
   * @return bool
   */
  public function isDuplicateUserError()
  {
    return $this->id == static::USEREXISTS;
  }

  // ...

}

There are 12 constants, each representing the id of a specific error. There is one method getMessage() which uses a switch with 12 cases. Then I have 12 messages of the kind getErrorMessageX and another 12 methods isThisErrorNameX.

I feel that this is kind of messy and redundant. Also their might be me more error cases in the future, which will bloat up the class even more.

I thought about creating 12 separate classes, each class named by the error name. Something like that:

<?php

namespace App\Registration\GroupUploads;

/**
 * Error for existing user
 */
class UserExistsAlreadyError
{

  public function getMessage()
  {
    return 'User already exists';
  }

}

So instead of if($error->isDuplicateUserError()) I would write if($error instanceof UserExistsAlreadyError) .

However, I think creating 12 different classes, where each consists only of 12 lines, is a bit too much. Is there a better solution then those two extremes?

This is how I ended up doing it now:

<?php

namespace App\Registration\GroupUploads\Errors;

/**
 * Error that appear during parsing csv file
 * @var [type]
 */
abstract class Error
{
  const USEREXISTS  = 7;
  const MAILINVALID = 1;
  const SEALUSED    = 2;
  const MIXEDUP     = 3;
  const QUALMISSING = 20;

  public $id;
  protected $msg;

  /**
   * Get error message
   * @return [type] [description]
   */
  public function getMsg()
  {
    return $this->msg;
  }

  /**
   * Error if user existed before registration
   * @param  [type] $name [description]
   * @return [type]       [description]
   */
  public static function userAlreadyExists($name)
  {
    $error = new Error();
    $error->id   = static::USEREXISTS;
    $error->msg  = "User " . $name .  " already exists";
    return $error;
  }

  /**
   * Mail is not valid
   * @param  [type] $mail [description]
   * @return [type]       [description]
   */
  public static function mailInvalid($mail)
  {
    $error = new Error();
    $error->id   = static::MAILINVALID;
    $error->msg  = "The mail " . $mail .  " is not valid";
    return $error;
  }

  //..
}

This way the getMsg method is short. Also adding a new error only requires to add one method instead of two (and no modification of the getMsg function).

To identify an error I call if($error->id == Error::USEREXISTS){...}.

Having the id was quite important, because I wanted to store the errors in DB.

Its better to use factory pattern for creating custom errors. I'm working on Java and following Java code might be helpful:

class CsvException extends Exception {
    public static final int USEREXISTS = 7;
    public static final int MAILINVALID = 1;
    public static final int SEALUSED = 2;
    public static final int MIXEDUP = 3;

    public static final CsvException createException(int type) {
        CsvException exc = null;
        switch (type) {
        case CsvException.USEREXISTS:
            exc = new UserExistException();
            break;

        case CsvException.MAILINVALID:
            exc = new MailInvalidException();
            break;

        case CsvException.SEALUSED:
            exc = new SealUsedException();
            break;

        case CsvException.MIXEDUP:
            exc = new MixedUpException();
            break;

        default:
            exc = new CsvException();
            break;
        }
        return exc;
    }

    @Override
    public String getMessage() {
        return "A CSV Exception occurred!";
    }
}

class UserExistException extends CsvException {

    @Override
    public String getMessage() {
        return "User already exists!";
    }

}

class MailInvalidException extends CsvException {
    @Override
    public String getMessage() {
        return "Invalid email ID!";
    }
}

class SealUsedException extends CsvException {
    @Override
    public String getMessage() {
        return "Seal is already used!"; //I'm assuming message and this class by exception type
    }
}

class MixedUpException extends CsvException {
    @Override
    public String getMessage() {
        return "Mixed up!";
    }
}

And you can use above exception factory like below:

    ...

    public void checkMail() throws MailInvalidException {
        //If invalid email ID
        throw (MailInvalidException)CsvException.createException(CsvException.MAILINVALID);
    }

    public void checkUser() throws UserExistException {
        //If user already exists
        throw (UserExistException)CsvException.createException(CsvException.MAILINVALID);
    }

    ...


Beginning C character array explanation I have been following along the book 'The C Programming Language' 2nd edition by Kernighan and Ritchie. The below code is an exercise from the book. What I am confused about in this code is the Character array 'line'. It is a local variable so I don't understand how the value is changing when it is used as an argument in the 'get_line' function.

/*******************************
 * Author:                     *
 * Date: 4/22/2019             *
 * Purpose: Book exercise      *
 *******************************/

#include <stdio.h>
#include <stdlib.h>

#define MAX_LINE 1000

int get_line(char line[], int maxline);
void copy(char to[], char from[]);

/*Print longest input line*/
int main()
{
    int len;    //Current line length
    int max;    //Maximum length seen so far

    char line[MAX_LINE];        //Current input line
    char longest[MAX_LINE];     //Longest line saved here

    max = 0;

    //While input length is greater than 0
    while((len = get_line(line, MAX_LINE)) > 0)
    {
        //If current string is longer than max
        if (len > max)
        {
            max = len;              //Assign new max value
            copy(longest, line);    //Replace longest string value with current string
        }
    }

    if (max > 0)
    {
        printf("%s", longest);
    }
    return 0;
}

int get_line(char s[], int lim)
{
    int c, i;

    /*i = 0; i less than limit - 1 *and* input is not EOF *and* input is not enter key */
    for (i=0; i<lim - 1 && (c=getchar())!=EOF && c!='\n'; i++)
    {
        s[i] = c; // input array position i is = input
    }

    //Once enter key is hit
    if (c == '\n')
    {
        s[i] = c;   //Add enter key '\n' onto array
        i++;        //Incr i so that last char can be '\0' later
    }

    s[i] = '\0';    //The last spot on char array is '\0'
    return i;
}

void copy(char to[], char from[])
{
    int i;

    i = 0;

    //Assign from[i] to to[i] until null terminator
    while((to[i] = from[i]) != '\0')
    {
        i++;
    }
}

For example if I had a variable in the main routine

int add_num(int num1);
main()
{
    int x = 5;
    add_num(x);
    printf("%d", x);
}

int add_num(int num1)
{
    num1 += 5;
    return num1;
}

x would still remain equal to 5 unless I did:

x = add_num(x);

then x would equal 10.

So how does the line character array change when the function that it's used in returns 'i'?
Where I is the length of the string entered.

Sorry if this was a little long, but I wanted to explain myself clearly.

From the end of the chapter 1.8:

  "When the name of an array is used as an argument, the value passed to
  the function is the location or address of the beginning of the array
  - there is no copying of array elements. By subscripting this value, the function can access and alter any element of the array".

So, when you pass array as an argument to a function, you don't create a local copy of array. Instead, you create a pointer (Chapter 5) and work with it as with an array.


function swap 2 nodes in doubly linked list Helo everyone, I need to review on function swapNode in doubly linked list. It works but I want to make it clearer and better. Are there any errors like memory leak in my code?

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

struct node;
struct list;

typedef struct node node;
typedef struct list list;

struct node
{
    int point;
    char name[30];
    node *next;
    node *prev;
};

struct list
{
    node *head;
    node *tail;
    int count;
};

node *createNewNode(int point, char name[30], node *prev, node *next);
list *createList();
void insertHead(list *listNode, int point, char name[30]);
bool compareName(char a[30], char b[30]);
void swapNode(list *listNode, char nameA[30], char nameB[30]);

int main()
{
    list *listNode = createList();

    insertHead(listNode, 10, "abc def");
    insertHead(listNode, 9, "qwe rty");
    insertHead(listNode, 8, "ui op");
    insertHead(listNode, 30, "fgh jkl");
    insertHead(listNode, 1234, "akaka");

    swapNode(listNode, "fgh jkl", "akaka");

    node *temp = listNode->head;
    while (temp != NULL)
    {
        printf("%-20s%d\n", temp->name, temp->point);
        temp = temp->next;
    }

    return 0;
}

node *createNewNode(int point, char name[30], node *prev, node *next)
{
    node *newNode = (node *)malloc(sizeof(node));
    newNode->point = point;
    strcpy(newNode->name, name);
    newNode->next = next;
    newNode->prev = prev;
    return newNode;
}

list *createList()
{
    list *listNode = (list *)malloc(sizeof(list));
    listNode->count = 0;
    listNode->head = NULL;
    listNode->tail = NULL;
    return listNode;
}

void insertHead(list *listNode, int point, char name[30])
{
    node *newNode = allocateNewNode(point, name, NULL, listNode->head);
    if (listNode->head)
        listNode->head->prev = newNode;
    listNode->head = newNode;
    if (listNode->tail == NULL)
        listNode->tail = newNode;
    ++listNode->count;
}

bool compareName(char a[30], char b[30])
{
    for (int i = 0; i < 31; i++)
    {
        if (a[i] != b[i])
            return false;
        if (a[i] == '\0')
            break;
    }

    return true;
}

void swapNode(list *listNode, char nameA[30], char nameB[30])
{
    node *A = NULL, *B = NULL;
    node *temp = listNode->head;

    for (int i = 0; i < listNode->count; i++)
    {
        if (compareName(temp->name, nameA))
            A = temp;
        else if (compareName(temp->name, nameB))
            B = temp;
        temp = temp->next;
        if (A && B)
            break;
    }
    if (!A || !B)
        return false;
    else if (A == B)
        return false;

    node p=*A;
    *A=*B;
    *B=p;

    B->next = A->next;
    B->prev = A->prev;

    A->next = p.next;
    A->prev = p.prev;
}

Don't Return Values from void Functions

    if (!A || !B)
        return false;
    else if (A == B)
        return false;

The function void swapNode(List *listNode, char nameA[30], char nameB[30]) is declared void, which means it doesn't return a value, yet it attempts to return false in two places. Some compilers actually report this as an error. Rather than return false; it should just be return;.

The two if statements above could be rewritten as one if statement

    if ((!A || !B) || (A == B))
    {
        return;
    }

Missing Error Checking

The C programming memory allocation functions malloc(size_t size), calloc(size_t count, size_t size) and realloc( void *ptr, size_t new_size) may fail. If they do fail then they return NULL. Any time one of these functions are called, the result should be tested to see if it is NULL. Referencing fields through a NULL pointer yields unknown behavior and is generally a bug.

Node *safeMalloc(size_t size)
{
    Node* newNode = malloc(size);
    if (newNode == NULL)
    {
        fprintf(stderr, "Memory allocation failed in safeMalloc\n");
        exit(EXIT_FAILURE);
    }

    return  newNode;
}

Node *createNewNode(int point, char name[30], Node *prev, Node *next)
{
    Node *newNode = safeMalloc(sizeof(*newNode));
    newNode->point = point;
    strcpy(newNode->name, name);
    newNode->next = next;
    newNode->prev = prev;

    return newNode;
}

Declarations of Node Structs

It might have been easier to write the struct declarations as

typedef struct node
{
    int point;
    char name[30];
    struct node *next;
    struct node *prev;
} Node;

typedef struct list
{
    Node *head;
    Node *tail;
    int count;
} List;

Complexity

The function void swapNode(List *listNode, char nameA[30], char nameB[30]) can be simplified by breaking it into 2 functions, one that does the comparisons and then calls a swaping function as necessary:

void doSwap(Node *A, Node*B)
{
    Node p=*A;
    *A=*B;
    *B=p;

    B->next = A->next;
    B->prev = A->prev;

    A->next = p.next;
    A->prev = p.prev;
}

void swapNode(List *listNode, char nameA[30], char nameB[30])
{
    Node *A = NULL, *B = NULL;
    Node *temp = listNode->head;

    for (int i = 0; i < listNode->count; i++)
    {
        if (compareName(temp->name, nameA))
        {
            A = temp;
        }
        else if (compareName(temp->name, nameB))
        {
            B = temp;
        }
        temp = temp->next;
        if (A && B)
        {
            break;
        }
    }

    if ((A && B) && (A != B))
    {
        doSwap(A,B);
    }
}

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

A Good Habit for Programming in C and C++

For readability and maintainability a good good habit (best practice) to get into is to always put the actions in if statements and loops into braces ({ and }) as shown in the previous example. One of the major causes of bugs is to add a single line to the contents of an iff statement and to forget to add the necessary the necessary braces. This type of problem is very hard to track down when it doesn't result in a compiler error.

Leaks

  Are there any errors like memory leak in my code?

If the code was part of a larger project there would be memory leaks, the function free(void *ToBeFreed) is never called. It might be better if some linked list operations such as deleteNode() were added to the code.

Swapping Nodes without changing the data
node* swapnodes(node* head,int x,int y)
{
node* currx=NULL,*curry=NULL;
node* temp=head;
while(temp)
{
    if(temp->data==x||temp->data==y)
    {
        if(!currx)
        currx=temp;
        else
        {
            curry=temp;
            break;
        }
    }
    temp=temp->next;
}
if(currx->prev)
currx->prev->next=curry;
else
head=curry;
if(curry->next)
curry->next->prev=currx;

if(currx->next==curry)//Adjacent Nodes
{
    curry->prev=currx->prev;
    currx->next=curry->next;
    currx->prev=curry;
    curry->next=currx;
    return head;
}
curry->prev->next=currx;
currx->next->prev=curry;
swap(currx->next,curry->next);
swap(currx->prev,curry->prev);
return head;
}


HackerRank "Nested Lists" Code I completed the "Nested Lists" challenge on HackerRank, and would love any feedback on my code.

My program is fed text with the number of students in a classroom, the name of a student, and their grade. For example:

4
Shadab
8
Varun
8.9
Sarvesh
9.5
Harsh
10

The code should return the student with the second lowest grade. Here's the output using the above input:

Varun

Here is my code:

def secondlow(students):
    grades = []
    for student in students:
        grades.append(student[1])
    sort_grades = sorted(grades)
    seclow_grade = sort_grades[0]
    for grade in sort_grades:
        if grade != seclow_grade:
            seclow_grade = grade
            break
    seclow_stud = []
    for student in students:
        if student[1] == seclow_grade:
            seclow_stud.append(student[0])
    for name in sorted(seclow_stud):
        print(name)

students = []
for pupil in range(int(input())):
    new_stud = [input(), float(input())]
    students.append(new_stud)
secondlow(students)

The code mostly follows usual coding style and reads good. I would just change secondlow to second_low and avoid using abreviations in variable names.

Now for the improvements:

Print vs return

Do not code too much behaviour into your functions. Let them compute their stuff and return the result so the caller can do whatever they want with it. It doesn't add much here, but it is a good habit to have.

Getting the value you are looking for

seclow_stud = []
for student in students:
    if student[1] == seclow_grade:
        seclow_stud.append(student[0])

You do not need to store the only value you are looking for into a list. Especialy when using a for loop after that to extract the element out of the list:

for student in students:
    if student[1] == seclow_grade:
        return student[0]

Sorted

sort_grades = sorted(grades)
seclow_grade = sort_grades[0]
for grade in sort_grades:
    if grade != seclow_grade:
        seclow_grade = grade
        break

The sorted builtin returns a list of values in increasing order. What is important to note here is that sort_grades is a list. So sort_grade[0] is the lowest grade and sort_grade[1] is the second lowest. No need to search for the value yourself, sorted already did the work for you.

seclow_grade = sorted(grades)[1]

List-comprehensions

grades = []
for student in students:
    grades.append(student[1])

Python have these construct to easily build lists out of other iterables:

grades = [student[1] for student in students]

You can also use the same construct when building lists out of the input. Overall code would look like:

def secondlow(students):
    grades = [student[1] for student in students]
    seclow_grade = sorted(grades)[1]
    for student in students:
        if student[1] == seclow_grade:
            return student[0]

students = [[input(), float(input())] for _ in range(int(input()))]
print(secondlow(students))

Dictionaries

I know that the challenge explicitly mentionned lists of lists, but Python have dictionaries that can map a value to another. You can use them as a replacement:

def secondlow(students):
    seclow_grade = sorted(students.values())[1]
    for student in students.items():
        if student[1] == seclow_grade:
            return student[0]

students = {}
for _ in range(int(input())):
    name = input()
    grade = float(input())
    students[name] = value
print(secondlow(students))

Or you can take advantage of the way you can retrieve items associated to others by switching the way you store your data:

def secondlow(students):
    second_low = sorted(students)[1]
    return students[second_low]

students = {}
for _ in range(int(input())):
    name = input()
    grade = float(input())
    students[value] = name  # Note the switch here
print(secondlow(students))

Edit

After reading the challenge description again, I realised that there can be multiple students with the second lowest grade. And in fact, there might be cases where there are multiple student with the lowest grade. Making my point about having the second lowest grade as sorted(grades)[1] irrelevant… Unless you remove duplicates using a set:

def second_low(students):
    second_low_grade = sorted(set(students.values()))[1]
    for student in students.items():
        if student[1] == second_low_grade:
            yield student[0]

students = {}
for _ in range(int(input())):
    name = input()
    grade = float(input())
    students[name] = value

for second_lowest_student in second_low(students):
    print(second_lowest_student)

Here I used the yield keyword to avoid building a list in second_low. What it does is allowing the function to return several results: one each time there is a match (if student[1] == second_low_grade). Here is the version using list of lists:

def second_low(students):
    grades = set(student[1] for student in students)
    seclow_grade = sorted(grades)[1]
    for student in students:
        if student[1] == seclow_grade:
            yield student[0]

students = [[input(), float(input())] for _ in range(int(input()))]
for second_lowest_student in second_low(students):
    print(second_lowest_student)

Unfortunately, such requirements prevent you from using dictionaries the other way around.

This is my answer without using sets, dictionaries or any other than nested lists.

nestedlist = []
for _ in range(int(input())):
    name = input()
    score = float(input())
    nestedlist.append([name,score])

lowestscore = min(b for a,b in nestedlist)
for a,b in nestedlist:
    nestedlist.sort()
    for a,b in nestedlist:
        if (b == lowestscore):
            nestedlist.remove([a,b])

secondlowest = min(b for a,b in nestedlist)
for a,b in nestedlist:
    if b == secondlowest:
        print(a)


Print out all possible sizes of subsequences of string in C For example, given a string "abcdefghijk", I want my function to print out:

a, b, c, d, e, f, g, h, i, j, k.

ab, bc, cd, de, ef, fg, gh, hi, ij, jk

abc, bcd, cde, def, efg, fgh, ghi, hij, ijk

abcd, bcde

I'm sure there's a better way to do this than what I currently have. What are some optimizations I could do? Mostly in terms of speed performance, but any kind of optimization would be great; time, space, design, etc...

The commas don't matter much, it's the order that I worry about at this point, so that a, b, b, c, c, d, d, e, e, f, f, g, g, h, h, i, i, j, j, k is fine for the second line.

int main(int argc, char const *argv[])
{

    char* s = "abcdefghijk";
    int n = 11;

    for (int k = 0; k < n; k++)
    {
        for (int i = 0; i < n-k; i++)
        {
            for (int j = i; j <= i+k; j++)
                printf("%c, ",s[j]);
        }
        printf("\n");
    }

    return 0;
}

Here's a way that (at least IMO) at least marginally cleaner:

#include <stdio.h>

int main(int argc, char const *argv[])
{

    char s[] = "abcdefghijk";
    int n = sizeof(s);

    for (int k = 1; k < n; k++)
    {
        for (int i = 0; i < n - k; i++)
        {
            printf("%*.*s, ", k, k, s + i);
        }
        printf("\n");
    }

    return 0;
}

As to things about the code that can be improved, the most obvious would be using sizeof (or strlen, if necessary) to find the length of the input array, instead of hard-coding it.

Realistically, there's probably not a lot you can do to improve speed/performance--it's virtually guaranteed to be heavily I/O bound (though Tom Ernack's suggestion of using fwrite instead of printf might help--then again, depending on the standard library you're using, it might not).

One thing that I think is very important but not yet addressed in the other answers is: naming and abstraction.

Albeit your code is short, I have to look at it and think about what these loops are doing to understand what your code is supposed to do. It's much better if that's directly apparent from the identifiers (variable and function names). Speaking of functions, there's at least one functionality that I'd put into a separate function: printing a substring.

void print_substring (char const * from, char const * to) {
  fwrite(from, 1, to - from, stdout);
  // One could check return value to ensure
  // that all characters have been written,
  // but then you also need to decide what
  // to do in the case of an error.
}

Further, i, j and k are good names for generic loop counters, but here we can be way more specific:

// Note: prints all except the empty substring
void print_all_substrings(char const * string) {
  size_t const string_length = strlen(string);
  for (size_t substring_length = 1;
       substring_length <= string_length;
       ++substring_length) {
    char const * const last_begin = string + (string_length - substring_length);
    char const * begin = string;
    for (; begin != last_begin; ++begin) {
      print_substring(begin, begin + substring_length);
      printf(", ");
    }
    print_substring(last_begin, last_begin + substring_length);
    printf("\n");
  }
}

(Ideone test code)


Multithreaded web scraper with proxy and user agent switching I am trying to improve the performance of my scraper and plug up any possible security leaks (identifying information being revealed).

Ideally, I would like to achieve a performance of 10 pages per second. What would I need to do to achieve the biggest performance boost besides get a faster connection / dedicated server? What could be improved?

PS: I am only using eBay.com as an example here. The production version of the scraper will obey robot.txt requests, avoid peak traffic hours, and be throttled so I am not effectively DDoSing the site.

import requests
from bs4 import BeautifulSoup
from multiprocessing.dummy import Pool as ThreadPool

import logging
logging.basicConfig(level=logging.DEBUG, format='%(asctime)s - %(levelname)s - %(message)s')
logging.getLogger("requests").setLevel(logging.WARNING)

# NOTE: The next two sections are for demo purposes only, they will be imported from modules
# this will be stored in proxies.py module
from random import choice
proxies = [
    {'host': '1.2.3.4', 'port': '1234', 'username': 'myuser', 'password': 'pw'},
    {'host': '2.3.4.5', 'port': '1234', 'username': 'myuser', 'password': 'pw'},
]
def check_proxy(session, proxy_host):
    response = session.get('http://canihazip.com/s')
    returned_ip = response.text
    if returned_ip != proxy_host:
        raise StandardError('Proxy check failed: {} not used while requesting'.format(proxy_host))
def random_proxy():
    return choice(proxies)
# / end of proxies.py

# this will be stored in user_agents.py module
from random import choice
user_agents = [
    'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/600.8.9 (KHTML, like Gecko) Version/8.0.8 Safari/600.8.9',
    'Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36',
]
def random_user_agent():
    return choice(user_agents)
# / end of user_agents.py

def scrape_results_page(url):
    proxy = random_proxy()  # will be proxies.random_proxy()
    session = requests.Session()
    session.headers = random_user_agent()  # will imported with "from user_agents import random_user_agent"
    session.proxies = {'http': "http://{username}:{password}@{host}:{port}/".format(**proxy)}
    check_proxy(session, proxy['host'])  # will be proxies.check_proxy(session, proxy['host'])

    response = session.get(url)
    soup = BeautifulSoup(response.text, 'lxml')
    try:
        first_item_title = soup.find('h3', class_="lvtitle").text.strip()
        return first_item_title
    except Exception as e:
        print url
        logging.error(e, exc_info=True)
        return None

page_numbers = range(1, 10)
search_url = 'http://www.ebay.com/sch/Gardening-Supplies-/2032/i.html?_pgn='
url_list = [search_url + str(i) for i in page_numbers]

# Make the Pool of workers
pool = ThreadPool(8)
# Open the urls in their own threads and return the results
results = pool.map(scrape_results_page, url_list)
# Close the pool and wait for the work to finish
pool.close()
pool.join()

To get it working you will need to enter valid proxies or comment out the session.proxies = ... and check_proxy(session, proxy['host']) lines.

Performance

Each call to scrape_results_page will also call check_proxy.
As such, the check_proxy will get called for the same proxies multiple times, and I'm wondering if there's a reason for re-checking the proxies.
If not, then you can save time by checking the list of proxies once at the beginning of the program,
and remove the check from scrape_results_page.

Once you have a set of verified proxies,
instead of selecting them randomly,
it would be better to use them round-robin style,
to balance the load.
Admittedly this tip may not make any visible difference whatsoever.

Other improvements

There are a lot of parameters buried in the implementation,
for example the ebay url, canihazip.com, the number of threads and page numbers, and possibly others.
It would be better to define such values at the top,
in variables with descriptive names, all upper-cased to follow the convention for "constants".

Have you considered using the Proxicity.io APIs (https://www.proxicity.io/api)? They would allow you to ditch the proxy list, and would relieve a lot of issues with trying to rotate the proxies yourself. Not to mention, proxy lists get stale pretty fast. Getting the proxy is as easy as:

proxy = requests.get('https://www.proxicity.io/api/v1/APIKEY/proxy').json()

Result:

{
  "cookiesSupport": true,
  "country": "US",
  "curl": "http://107.151.136.205:80",
  "getSupport": true,
  "httpsSupport": false,
  "ip": "107.151.136.205",
  "ipPort": "107.151.136.205:80",
  "isAnonymous": true,
  "lastChecked": "Tue, 31 May 2016 12:36:45 GMT",
  "port": "80",
  "postSupport": true,
  "protocol": "http",
  "refererSupport": true,
  "userAgentSupport": true
}

Disclaimer: I am affiliated with Proxicity.io.


Haskell Graphing Module (Gloss) I've been slowly learning Haskell over the past few months. I made a small module to graph functions using Gloss, and I would like feedback on how idiomatic it is (looking for ways to simplify using standard functions and to make it more pointfree).

Graph.hs

module Graph (graph) where

import Graphics.Gloss

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) dx = pictures $ map Line visible
    where
        points :: [(Float, Float)]
        points = [(x, f x) | x <- [l,l+dx..r]]

        pairs :: [[(Float, Float)]]
        pairs = zipWith (\x y -> [x,y]) points $ tail points

        visible :: [[(Float, Float)]]
        visible = filter (all (\(_,y) -> b <= y && t >= y)) $ pairs

Main.hs

import Graphics.Gloss

import Graph

main :: IO ()
main = display FullScreen white . scale 20 20 . pictures $ [
        color blue $ graph f (-10, 10, -10, 10) 0.001,
        color black $ Line [(0, -10), (0, 10)],
        color black $ Line [(-10, 0), (10, 0)]
    ]
    where
        f :: Float -> Float
        f x = 1 / (x - 1)

Output

This looks great. Honestly, there isn't anything I'd change. You could make it more pointfree in some parts, but that's not going be more readable or maintainable.

Magic numbers

The only part I'd change are the static dimensions in main. Those are fine in a toy program, but a proper one will need some kind of configuration, so make sure that you use proper values from the beginning:

main :: IO ()
main = display FullScreen white . scale width height . pictures $ [
        color blue $ graph f (l, r, b, t) 0.001,
        color black $ Line [(origin, b), (origin, t)],
        color black $ Line [(l, origin), (r, origin)]
    ]
    where
        f :: Float -> Float
        f x = 1 / (x - 1)

        -- easy configurable:
        (l, r, b, t) = (-10, 10, -10, 10)
        width  = 20
        height = 20
        origin =  0

That way you can also exchange the values with proper command line interpretation

main :: IO ()
main = do
    (l, r, b, t) <- getDimensions
    let width = r - l
    let height = t - b
    let origin = ...
    display FullScreen white . scale width height . pictures $ [
...

After all, no magic numbers is a good practice in both imperative and functional languages.

Next, I'd introduce GraphBound as a type synonym, just to make graph's type signature easier on the eye:

-- | Graph boundaries, given in (left, right, bottom, top) order
type GraphBound = (Float, Float, Float, Float)

graph :: (Float -> Float) -> GraphBound -> Float -> Picture
graph f (l, r, b, t) dx = pictures $ map Line visible
    ...

You might even exchange GraphBound with a proper data type later which checks does not export its constructor to make sure that you don't end up with left = 20 and right = -10:

makeGraph :: Float -> Float -> Float -> Float -> Maybe GraphBound

However, that's an overkill, so let's not focus on that for too long.

List comprehensions vs. point-free

Now let's get back to your original query. Is it possible to make graph more point-free?

Sure:

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) = pictures . map Line
                     . filter (all (\(_,y) -> b <= y && t >= y))
                     . (tail >>= flip (zipWith (\x y -> [x, y])))
                     . map (\x -> (x, f x))
                     . flip (enumFromThenTo l) r . (l+)

The point dx is completely gone from graph. However, the function is now unreadable. We went from a perfectly understandable function to a highly complex one. It gets a lot more readable if we use some helpers, but at that point we're almost back to your original function:

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) = pictures . map Line . filter inGraph
                     . segments . points . ranged
   where
     inGraph (_,y) = fall (\(_,y) -> b <= y && t >= y)
     segments ps   = zipWith (\x y -> [x, y]) ps $ tail ps
     ...

That's not better than your original version, because your original version is already very good to begin with. The only change I could envision is a list comprehension in visible, but that's a matter of preference:

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) dx = pictures lines
    where
        points = [(x, f x) | x <- [l,l+dx..r]]
        pairs  = zipWith (\x y -> [x,y]) points $ tail points

        lines = [Line segment | segment <- pairs,  all (\(_,y) -> b <= y && t >= y)) segment]

But you're the judge on which variant you want to use.

Other remarks

Thank you for using type signatures. Keep in mind that it's uncommon to use them in local bindings (where), as the outer function's signature should fix all types already. Inner type signatures can be a hassle if you change your outer type signature later, but they're sometimes necessary.

You should simplify using standard functions and make it more pointfree.

import Data.List.Split

graph :: (Float -> Float) -> (Float, Float, Float, Float) -> Float -> Picture
graph f (l, r, b, t) dx = pictures $ map Line
  $ wordsBy (\(_,y) -> b > y || t < y) [(x, f x) | x <- [l,l+dx..r]]


Adjusting integer based on multiple elements in int array I'm working with the following code which adjusts an int based on multiple elements of an integer array. I am wondering if there's a cleaner, easier-to-read construct for this:

int Blue = 0;
int[] Adjustments = new int[24];
// ... populate Adjustments

if (Adjustments[0] == 255)
    Blue = 128;
else if (Adjustments[0] == 0)
    Blue = 0;

if (Adjustments[1] == 255)
    Blue += 64;
else if (Adjustments[1] == 0)
    Blue += 0;

if (Adjustments[2] == 255)
    Blue += 32;
else if (Adjustments[2] == 0)
    Blue += 0;

if (Adjustments[3] == 255)
    Blue += 16;
else if (Adjustments[3] == 0)
    Blue += 0;

if (Adjustments[4] == 255)
    Blue += 8;
else if (Adjustments[4] == 0)
    Blue += 0;

if (Adjustments[5] == 255)
    Blue += 4;
else if (Adjustments[5] == 0)
    Blue += 0;

if (Adjustments[6] == 255)
    Blue += 2;
else if (Adjustments[6] == 0)
    Blue += 0;

if (Adjustments[7] == 255)
    Blue += 1;
else if (Adjustments[7] == 0)
    Blue += 0;

Any suggestions?

Edit

Here is the revised version based on suggestions provided:

Blue = 0;
Green = 0;
Red = 0;
int[] Adjustments = new int[24];
// ... populate Adjustments

int[] AdjustmentStops = new[] { 128, 64, 32, 16, 8, 4, 2, 1 };
for (int i = 0; i < 8; i++)
{
    Blue += Adjustments[i] == 255 ? AdjustmentStops[i] : 0;
    Green += Adjustments[i + 8] == 255 ? AdjustmentStops[i] : 0;
    Red += Adjustments[i + 12] == 255 ? AdjustmentStops[i] : 0;
}

At the risk of being 'clever':

int Blue = 0;
int[] Adjustments = new int[24];
// ... populate Adjustments

for (int blueIndex=0; blueIndex<8; blueIndex++)
{
   if (Adjustments[blueIndex] == 255)
   {
      Blue |= (1 << (7 - blueIndex));
   }
}

Since in effect you're setting bits anyway I think using bitwise operations is idiomatic here.

Although perhaps less efficient, any time I see an array being used as a half-assed object, I'm inclined to make an object.

struct ColorAdjustment {
    public int Red;
    public int Blue;
    public int Green;
}

Then make an array of 8 of those.

But since you're only checking two values (255 or 0) a set of bools would be even better.
But since you're just doing bit math, @jon_darkstar's answer is even better still because it just does that math directly.

But best of all would probably be to use a BitArray or to do bit boolean logic on a 24-bit value (or an Int32, if you don't mind wasted space) to indicate the state of adjustments, as:

Int24 adjustments = 0xBEAF;
for(int i = 0; i < 8; i++) {
    if((adjustments && (1 << i)) != 0)
        ...do blue stuff...
    if((adjustments && (1 << (i + 8))) != 0)
        ...do red stuff...
...

Edit:
Hah, and I just realized I ran the full gamut from clear, object based code to fiddly bit-based code.


Function for minimum non-zero with multiple additional conditions This function works, but seems unnecessarily complicated.  I would like to simplify but performance is vital as it is called many times.

The conditions are:

  1) If either value is negative, then result is -1

  2) If both values are zero, then result is zero

  3) If NeedMax is true, result is largest value

  4) If NeedMax is false, result is smallest non-zero value

CODE:

Public Function SelValue(ValOne As Long, ValTwo As Long, _
                    Optional NeedMax As Boolean = True) As Long

    If ValOne < 0 Or ValTwo < 0 Then
        SelValue = -1
    ElseIf ValOne = 0 And ValTwo = 0 Then
        SelValue = 0
    ElseIf NeedMax Then
        If ValOne > ValTwo _
            Then SelValue = ValOne _
            Else SelValue = ValTwo
    Else
        If ValOne = 0 Then
            SelValue = ValTwo
        ElseIf ValOne > ValTwo And ValTwo <> 0 Then
            SelValue = ValTwo
        Else
            SelValue = ValOne
        End If
    End If

End Function

Can anyone suggest a more direct, and hopefully faster, approach?

Try to use meaningful names. What's names (SelValue, ValOne,...) stands for? It don't give info on their purpose or usage.

Furthermore, you can simplify a lot your conditional branchments, using only this:

If ValOne < 0 Or ValTwo < 0 Then
    SelValue = -1
' remove this following condition to discard case 4
Else If ValOne = 0 Or ValTwo = 0 Then
    SelValue = ValOne + ValTwo
ElseIf ValOne > ValTwo <> NeedMax Then
    SelValue = ValTwo
Else
    SelValue = ValOne
End If

not fully tested, but should do the trick

Edit : fixed case (4), where NeedMax is false and one of values is 0.

This is Excel, so some Excel built-in perks can be used.

Return a Variant, not a defined type because you can then return an error quite easily. Yes this does apply to general VBA as well.

Return early from the function if you can to save further checks - although in this simple example it is not really necessary.

Every time I see an ElseIf I think that there is something in here that can be improved.

If you are looking to expand this later, then write it for such extension. The first example below just cleans up the code.

Public Function SelValue(ValOne As Long, ValTwo As Long, _
                    Optional NeedMax As Boolean = True) As Variant
    If ValOne < 0 Or ValTwo < 0 Then
        SelValue = CVErr(xlErrValue)
        Exit Function
    End If

    If ValOne = 0 And ValTwo = 0 Then
        SelValue = CLng(0)
        Exit Function
    End If

    If NeedMax Then
        SelValue = IIf(ValOne > ValTwo, CLng(ValOne), CLng(ValTwo)) ' But note comments about a minor performance hit
        Exit Function
    End If

    If ValOne = 0 Then
        SelValue = CLng(ValTwo)
        Exit Function
    End If

    ' Now that we have cleaned out the special cases above. NeedMax is False
    If ValOne > ValTwo And ValTwo <> 0 Then
        SelValue = CLng(ValTwo)
    Else
        SelValue = CLng(ValOne)
    End If

End Function

The above code is not the final way I would leave it - personally I would refactor it again because I prefer only one exit/return from a function. But the above iteration highlights something important. It isolates the logic. And you don't handle the case where ValTwo = 0

Addendum: SelValue = CVErr(xlErrValue) is only useful if the function is a UDF. Raising error #5 (Err.Raise 5) here and still returning a Long is a valid approach for a non-UDF. Thanks to Mathieu Guindon.


Generate sample coordinates inside a Polygon I have a Polygon named as poly. I attempted to randomly select 5 coordinate points that lies inside the polygon.

import numpy as np
from shapely.geometry import Polygon, Point

poly = Polygon([(141.4378366,-25.95915986), (165.4279876,-29.43400298), (163.1382942,-47.65345814), (133.1675418,-42.99807751)])

minx, miny, maxx, maxy = poly.bounds

longs = np.arange(minx, maxx, 0.002); lats = np.arange(miny, maxy, 0.002)
longs = np.tile(longs,3).ravel(); lats = np.repeat(lats,3).ravel()
coords = np.array([(x,y) for x,y in zip(longs,lats)])

points = [Point(xy) for xy in coords]

check = [xy.within(poly) for xy in points]
pointsInside = coords[check]

ranIdx = np.random.choice(len(pointsInside),5,replace=False)
result = pointsInside[ranIdx]

print result

I think my code is ineffective. Are there any ideas for a straight and elegant implementation?

Style Nitpicks

Put spaces after commas / Keep Lines within 120 Chars

poly = Polygon([(141.4378366,-25.95915986), (165.4279876,-29.43400298), (163.1382942,-47.65345814), (133.1675418,-42.99807751)])

becomes

poly = Polygon([(141.4378366, -25.95915986), (165.4279876, -29.43400298), (163.1382942, -47.65345814),
                (133.1675418, -42.99807751)])

Use Pythonic Underscores

minx, miny, maxx, maxy = poly.bounds

becomes

min_x, min_y, max_x, max_y = poly.bounds

Eschew Semicolons (Personal Opinion)

You can use semicolons like this:

longs = np.arange(minx, maxx, 0.002); lats = np.arange(miny, maxy, 0.002)

But personally, I just wouldn't in Python. Eww.

An Alternative Algorithm

EDIT: Having read @MrE's and @MartinR's comments, I now propose this rejection sampling method. Although, this could miss frequently in a polygon with a large bounding box relative to its area; .e.g. an 8-point Christmas star with a small inner circle.

def random_points_within(poly, num_points):
    min_x, min_y, max_x, max_y = poly.bounds

    points = []

    while len(points) < num_points:
        random_point = Point([random.uniform(min_x, max_x), random.uniform(min_y, max_y)])
        if (random_point.within(poly)):
            points.append(random_point)

    return points

Another alternative that never misses, but may not be well distributed.

This was my original idea, but it didn't look so good in the morning.

Firstly, work out how to select one point within a polygon.

def random_point_within(poly):
    min_x, min_y, max_x, max_y = poly.bounds

    x = random.uniform(min_x, max_x)
    x_line = LineString([(x, min_y), (x, max_y)])
    x_line_intercept_min, x_line_intercept_max = x_line.intersection(poly).xy[1].tolist()
    y = random.uniform(x_line_intercept_min, x_line_intercept_max)

    return Point([x, y])

Then simply call that in a list comprehension to generate however many points you desire.

points = [random_point_within(poly) for i in range(5)]
checks = [point.within(poly) for point in points]

My approach is to select x randomly within the polygon, then constrain y.

This approach doesn't require NumPy and always produces a point within the polygon.

used this function with raster based 10m spacing

may be useful:

def random_points_within_poygon_and_10m_sbuffer(poly, num_points):

min_x, min_y, max_x, max_y = poly.bounds
min_lon = min_x
max_lon = max_x
min_lat = min_y
max_lat = max_y
# obtain utm bounds and dimentions
utmZone = int((np.floor((min_lon+ 180)/6)% 60) + 1)
fieldProj = Proj("+proj=utm +zone="+str(utmZone)+", +north +ellps=WGS84 +datum=WGS84 +units=m +no_defs")
UTMx_min, UTMy_max = fieldProj(min_lon, max_lat)
UTMx_max, UTMy_max = fieldProj(max_lon, max_lat)
UTMx_min, UTMy_min = fieldProj(min_lon, min_lat)
UTMx_min = math.floor(UTMx_min*0.2)*5.0 + 2.5 # make it standard grid to match img
UTMy_max = math.ceil(UTMy_max*0.2)*5.0 - 2.5  # make it standard grid to match img

utm_npix_x = int(0.1*(math.ceil((UTMx_max - UTMx_min)*0.1)*10))
utm_npix_y = int(0.1*(math.ceil((UTMy_max - UTMy_min)*0.1)*10))
#set spacing raster
spacing_utm_grid = np.arange(utm_npix_y*utm_npix_x).reshape((utm_npix_y, utm_npix_x))
spacing_utm_grid[:,:] = 0

points = []
while len(points) < num_points:
    pair_coordinates = [random.uniform(min_x, max_x), random.uniform(min_y, max_y)]
    random_point = Point(pair_coordinates)
    if (random_point.within(poly)):
        utm_x, utm_y = fieldProj(pair_coordinates[0], pair_coordinates[1])
        utm_x_pix = math.floor((utm_x - UTMx_min)/10)
        utm_y_pix = math.floor((UTMy_max -utm_y)/10)
        if(spacing_utm_grid[utm_y_pix, utm_x_pix]==0):
            points.append(random_point)
            spacing_utm_grid[utm_y_pix,utm_x_pix]=1

return points


Tkinter application with multiple windows I'm creating an application that requires multiple tkinter windows. The code below works the way how I want it to work, but I'm not sure if I'm doing it in the proper way.

The first window (and in this case, the main window as well) contains three buttons as of now, 1 label which acts as a title, and a treeview.

The "Add Member" button is the only button that has functionality as of now. When clicked, it will pop a new window. That said window contains label.

To sum it up, I would like to humbly ask if I'm doing it properly, or there is a better and more accurate way of writing this code.

class Application(tk.Frame):
    def __init__(self,parent,*args,**kwargs):
        tk.Frame.__init__(self,parent)
        self.parent = parent
        self.parent.title("Senior Citizen Association")

        ttk.Label(text="SENIOR CITIZEN ASSOCIATION").grid(row=0,column=0,columnspan=5,sticky="n")
        ttk.Button(text="Add",command=self.add_member).grid(row=1,column=0,sticky="w")
        ttk.Button(text="Delete").grid(row=2,column=0,sticky="w")
        ttk.Button(text="Exit",command=lambda:self.parent.destroy()).grid(row=3,column=2,sticky="se")

        self.tree = ttk.Treeview(height=15,column=("First Name","Middle Initial","Last Name"))
        self.tree.heading("#0",text="First Name",anchor="w")
        self.tree.heading("#1",text="Middle Initial",anchor="w")
        self.tree.heading("#2",text="Last Name",anchor="w")
        self.tree.heading("#3",text="Status",anchor="w")
        self.tree.grid(row=1,column=1,rowspan=2,columnspan=2,sticky="nsew")

    def add_member(self):
        self.addMemberWindow = tk.Toplevel(self.parent)
        self.addClass = NewMember(self.parent,self.addMemberWindow)

class NewMember(tk.Frame):
    def __init__(self,parent,child,*args,**kwargs):
        tk.Frame.__init__(self,child)
        self.parent = parent
        self.child = child
        self.child.title("Add Member")

        ttk.Label(self.child,text="Add Member").grid(row=0,column=0,sticky="nsew")

if __name__ == "__main__":
    app = tk.Tk()
    Application(app)
    app.mainloop()

For me, it is a pretty piece of code. I learned much from it. Also, I'm not sure if it is a proper way. But, it works fine and is easy to read :)


(Codewars Kata) Memoised Log Cutting (I am not sure of the official CS name for the problem in question, so I just gave it the name of the kata).

Problem

  CLEAR CUTTER'S NEEDS YOUR HELP!

  The logging company Clear Cutter's makes its money by optimizing the price-to-length of each log they cut before selling them. An example of one of their price tables is included:

# So a price table p
p = [ 0,  1,  5,  8,  9, 10]

# Can be imagined as:
# length i | 0  1  2  3  4  5 *in feet*
# price pi | 0  1  5  8  9 10 *in money*

  They hired an intern last summer to create a recursive function for them to easily calculate the most profitable price for a log of length n using price table p as follows:

def cut_log(p, n):
   if (n == 0):
      return 0
   q = -1
   for i in range(1, n+1)
      q = max(q, p[i] + cut_log(p, n-i))
   return q

  An example of the working code:

# 5ft = $10, BUT 2ft + 3ft = 5ft -> $5 + $8 = $13 which is greater in value

  However, their senior software engineer realized that the number of recursive calls in the function gives it an awful running time of 2^n (as this function iterates through ALL 2^n-1 possibilities for a log of length n).

  Having discovered this problem just as you've arrived for your internship, he responsibly delegates the issue to you.

  Using the power of Stack Overflow and Google, he wants you to create a solution that runs in Θ(n^2) time so that it doesn't take 5 hours to calculate the optimal price for a log of size 50. (He also suggests to research the problem using the keywords in the tags)

  (Be aware that if your algorithm is not efficient, it will attempt to look at 2^49 = 562949953421312 nodes instead of 49^2 = 2401... The solution will automatically fail if it takes longer than 6 seconds... which occurs at right around Log 23)

Solution

def cut_log(p, n):
    for i in range(2, len(p)):
        p[i] = max(p[i-k] + p[k] for k in range((i//2)+1))
    return p[n]

It passes all the tests. I'm putting it up for review because I'm pretty proud of it and experience as shown me that there's usually some improvement to be made.

I see three problems with this function:

Why calculate the values up to len(p), when you are actually interested in p[n] (which may be larger or smaller than len(p))? In the case of n > len(p) your code actually raises an exception (and it seems not to be excluded by the rules, but also not to be checked by the online judge).
This is not really using memoization, since you calculate everything again whenever the function is called.
You mutate p, which might lead to unexpected results (in general), although here it happens to work out fine.

The second (and third) problem are not so easy to fix (probably only by memoizing all different p ever seen and for each of them the maximum n used). Also the online judge may or may not use the same p again.

The first one however is an easy fix:

def cut_log(p, n):
    for i in range(2, n + 1):
        p[i] = max(p[i-k] + p[k] for k in range((i//2)+1))
    return p[n]

I incorporated suggestions that @Graipher made in his answer. This is the improved function that resulted from it.
Solution
history = {}

def cut_log(p, n):
    global history
    ln = len(p)
    if id(p) not in history:
        p = p + [0]*max(n - ln, 0)
        for i in range(2, max(n+1, ln)):
            p[i] = max(p[i-k] + p[k] for k in range((i//2)+1))
        history[id(p)] = p
    else:
        p = history[id(p)]
        ln = len(p)
        if n+1 > ln:
            p = p + [0]*max(n - ln, 0)
            for i in range(ln, n+1):
                p[i] = max(p[i-k] + p[k] for k in range((i//2)+1))
    return p[n]

I wasn't memoising the function, and I added that for increased performance.


Overriding default animation parameters with custom values Using JavaScript ES5, I would like to know if you could suggest me a more "elegant" and succinct way to overriding defaults variable when User pass custom values.

In the following example, an animation is executed using _defaultOptions, a developer can change the default behavior passing only some properties to a function, example:

animate(dom, {duration: 2000}); // overriding only duration, rest as default

Notes:

I need also to support IE 11.
optionsArg is optional, so animate(dom) is allowed

var dom = document.getElementById('logo');

var _defaultOptions = {
  duration: 1000,
  delay: 0,
  iterations: 1,
  direction: 'normal',
  fill: 'both'
};

var animate = function(dom, optionsArg) {
  // code to be improved here
  var options = {
    duration: optionsArg && 'duration' in optionsArg ? optionsArg.duration : _defaultOptions.duration,
    delay: optionsArg && 'delay' in optionsArg ? optionsArg.delay : _defaultOptions.delay,
    iterations: optionsArg && 'iterations' in optionsArg ? optionsArg.iterations : _defaultOptions.iterations,
    direction: optionsArg && 'direction' in optionsArg ? optionsArg.direction : _defaultOptions.direction,
    fill: optionsArg && 'fill' in optionsArg ? optionsArg.fill : _defaultOptions.fill,
  };
  dom.animate([{
    opacity: 1
  }, {
    opacity: 0
  }], options);
};

animate(dom, {duration: 2000});
<img id="logo" src="https://upload.wikimedia.org/wikipedia/commons/e/e5/NASA_logo.svg">

Object.assign(_defaultOptions, optionsArg )

But this will have side effects as it changes _defaultOptions object itself.


Finding missing numbers in an array Is it possible to make this faster? Any suggestions are more than welcome
I have used JavaScript to write this code
PROBLEM

You will get an array of numbers.

Every preceding number is smaller than the one following it.

Some numbers will be missing, for instance:
  [-3,-2,1,5] // missing numbers are: -1,0,2,3,4

Your task is to return an array of those missing numbers:
[-1,0,2,3,4]

SOLUTION
const findTheMissing = (target) => {

    // final result list
    let result = [];

    // array will go from min to max value present in the array
    const min = target[0];
    const max = target[target.length - 1];

    // will maintain the track of index of target array
    // will start from 2nd element of array because we need a no. to subsctract from
    let pivotIndex = 1;

    for (let index = min; index < max; index++) {

        // value to the index
        let pivotValue = target[pivotIndex];

        // dif of the value
        let diff = pivotValue - index;

        // diff means its time to move the pivot to next :P
        if (diff > 0) {
            // not going to current index at exists in the target array
            if (index === target[pivotIndex - 1])
                index++;

            // YO!! WE FOUND HE MISSING
            result.push(index);
        }
        else {
            pivotIndex++;
        }
    }
    return result; // got all missing numbers
}

RESULT
let source = [-5, 0, 2, 5, 7];
console.log(findTheMissing(source));
// [ -4, -3, -2, -1, 2, 3, 4, 6 ]

I think, there is an error:

If I enter the values:

[-3,-2,1,5]

I get the result:

[-2,0,2,3,4]

but it should be:

[-1,0,2,3,4]

The error occurs when there are two adjacent numbers in the array (here -3 and -2).

Naming:

Index is not an index, but the actual value in the valid sequence (from min to max). I would call it value or curValue or something like that.

I don't like the name target either, because the target is actually the sequence of numbers from min to max. input would be better IMO.

Analysis

Basically you want to compare two sequences of numbers index by index. If they differ then save the target value, if not then increment to the next value of the input array. The index of the target sequence (here from min to max) is always incremented. Instead of the target sequence you can just increment a value starting from input[0] and ending on input.[input.length - 1].

All in all it could be done something like this:

function findMissing(input) {

  var result = [];

  for (var inputIndex = 0, targetValue = input[0]; targetValue <= input[input.length - 1]; targetValue++) {
    if (input[inputIndex] != targetValue) {
      result.push(targetValue);
    }
    else {
      inputIndex++;
    }
  }

  return result;
}

No rocket science:

Do Lesser operations and your program is faster
No Loop inside Loops and Time complexity is less
Don't initialize variables in a loop instead assign them in variables out of loop
Enhance readability using KISS
Use Bit-wise operators and gain more speed, I haven't used

CODE

function findMissingNumbers(array) {
    const arraySize = array.length;
    const arr = [];
    let i = 0;
    let j = array[0];
    let jSize = array[arraySize - 1];
    while (j < jSize) {
        (array[i] === j) ? (i += 1) : (arr.push(j));
        j++;
    }
    return arr;
}


Conversion of hexadecimal string to string Given a C# string which is a set of hexadecimal numbers such as:

string HexString = "202048656c6c6f20576f726c64313233212020";

Where those hexadecimal numbers represent the ASCII text:

"  Hello World123!  "

I need to convert the HexString to a String and I have the following code which works, but looks so excessive!

string HexStringToString(string HexString) {
    string stringValue = "";
    for (int i = 0; i < HexString.Length / 2; i++) {
        string hexChar = HexString.Substring(i * 2, 2);
        int hexValue = Convert.ToInt32(hexChar, 16);
        stringValue += Char.ConvertFromUtf32(hexValue);
    }
    return stringValue;
}

Am I missing some elegant method?

I don't think there is a built-in method. Yours is pretty good but we could make some improvements:

Parameters should be camelCase => hexString.
You should favour StringBuilder when building up strings.
You should step through the string in increments of 2 to cut down on the maths.
You should validate the argument.
You should prefer var when the type is obvious.

Result of those points:

string HexStringToString(string hexString)
{
    if (hexString == null || (hexString.Length & 1) == 1)
    {
        throw new ArgumentException();
    }
    var sb = new StringBuilder();
    for (var i = 0; i < hexString.Length; i += 2) {
        var hexChar = hexString.Substring(i, 2);
        sb.Append((char)Convert.ToByte(hexChar, 16));
    }
    return sb.ToString();
}

I like the original code best.  I don't know of a better way than iterating through the string and the original code is actually pretty readable and likely no slower than any of the other options.


Convert Roman Numeral to Arabic I've been learning Haskell recently and I decided I needed to work on a (somewhat) realistic problem.

What I'm most interested in getting feedback on is how well I've used the tools in Haskell to accomplish the goal of converting Roman numerals to Arabic values. I suspect that my approach is more verbose than it needs to be, so thoughts on how I could remove unneeded code would be quite welcome.

Lastly, thoughts on how to address Haskell bad practices or similar errors would be appreciated.

-- file: roman.hs
import Data.Char

-- Application startup.
main :: IO ()
main = do
    putStrLn "Enter a Roman numeral. The numerical value will be returned."
    putStrLn "For example MMI -> 2001."
    putStrLn "Enter 'finis' to exit."
    process
    putStrLn "vale! :-)"

-- Main loop.
process :: IO ()
process = do
    putStrLn "Numeral: "
    line <- getLine
    if line == "finis"
        then return ()
        else do
            let nums = readNumerals(line)
                in if errorNum `elem` nums
                    then putStrLn (line ++ "is not a valid Roman numeral.")
                    else putStrLn ("   The Roman numeral " ++ line ++ " is equal to " ++ show(getValue(nums)))
            process

data Numeral = Numeral {
    numeralValue :: Int,
    numeralSymbol :: Char,
    subRule :: [Char]}
    deriving (Show, Eq)

-- This is the Numeral error type. used to check if the input was valid.
errorNum = Numeral 0 'E' []

-- Turn a string into a list of numerals.
readNumerals :: String -> [Numeral]
readNumerals (x:xs) = readNumeral (toUpper(x)) : [] ++ readNumerals xs -- First cons the Char to an empty list, then recur.
readNumerals "" = []

-- Parse each char into the corresponding numeral.
readNumeral :: Char -> Numeral
readNumeral 'I' = Numeral 1 'I' ['V', 'X']
readNumeral 'V' = Numeral 5 'V' []
readNumeral 'X' = Numeral 10 'X' ['L', 'C']
readNumeral 'L' = Numeral 50 'L' []
readNumeral 'C' = Numeral 100 'C' ['D', 'M']
readNumeral 'D' = Numeral 500 'D' []
readNumeral 'M' = Numeral 1000 'M' []
readNumeral _ = errorNum

-- Turn a list of Roman numerals into their corresponding Arabic number.
getValue :: [Numeral] -> Int
getValue [] = 0
getValue (x:xs) =
                if subRuler (subRule x) xs
                then negate (numeralValue x) + getValue xs
                else numeralValue x + getValue xs
                -- if x has a non-empty subRule and the next x is in the list, then flip the sign for x

-- Take the subRule from x and xs, if x of xs is in subRule, return true, otherwise false.
-- What this really means is see if the next element in the list of Numerals is one of the elements in subRule.
subRuler :: [Char] -> [Numeral] -> Bool
subRuler [] _ = False
subRuler _ [] = False
subRuler a b = numeralSymbol (head b) `elem` a

{-
Subtractive Rules, from Wikipedia
I placed before V or X indicates one less, so four is IV (one less than five) and nine is IX (one less than ten)
X placed before L or C indicates ten less, so forty is XL (ten less than fifty) and ninety is XC (ten less than a hundred)
C placed before D or M indicates a hundred less, so four hundred is CD (a hundred less than five hundred) and nine hundred is CM (a hundred less than a thousand)
-}

Parens

When you call a function you do not need to place the argument in parens:

Instead of:  readNumerals(line)
       Use:  readNumerals line

Error Handling

In Haskell we use types like Either or Maybe to indicate errors. Instead of:

let nums = readNumerals(line)
if errorNum `elem` mums
  then ...some error...
  else ...

You should define readNumerals to have this type:

readNumerals :: String -> Maybe [Numeral]

and write:

case readNumerals line of
  Nothing -> ... some error ...
  Just ns -> ... parse was valid, numerals are in `ns` ...

data Numeral

The data structure Numeral has many fields which are redundant. For instance, if n is a Numeral and numeralValue n is 1, then it will always be the case that numeralSymbol n will be I and subRules n will be ['V','X']. So there's no point storing these in the record - you can just implement these as functions:

subRules :: Numeral -> [Char]
subRules n = case numeralValue n of
                1  -> ['V','X']
                5  -> []
                10 -> ['L', 'C']
                ...

This allows you to eliminate the subRules field from the record.

Make Numeral an ADT

In fact, Numeral doesn't even need any fields. Here's a simpler way to implement Numeral:

data Numeral = I | V | X | L | C deriving (Show, Eq)

numeralValue :: Numeral -> Int
numeralValue I = 1
numeralValue V = 5
...

subRule :: Numeral -> [Numeral]
subRule I = [V, X]
subRule V = []
...

The nice thing is that the code for getValue doesn't change at all. The function subRuler does change a little (and also becomes simpler):

subRuler :: [Numeral] -> [Numeral] -> Bool
subRuler [] _ = False
subRuler _ [] = False
subRules as bs = (head bs) `elem` as

Avoid using head

head is a partial function - i.e. it could throw an error. Here's how to write subRuler without using head:

subRules [] _ = False
subRuler _ [] = False
subRuler as (b:_) = b `elem` as

We've replaced head with pattern matching, and it does the same thing. tail is also a partial function which you should replace with pattern matching.

There is a monoid form that can handle arbitrary length Roman numerals. Hackish Ruby code.

It is actually more abstract because it handles not just "IV"=4 and "IX"=9 but also strings like "IM"=999. That allows you to get rid of the subRule :: [Char].


Finding the first duplicate in an array My code returns the correct answer the first duplicate is 3 (represented as int f) . I am struggling finding a more efficient way to find the first duplicate?

My constraints are \$ 1 ≤ a.length ≤ 10^5, 1 ≤ a[i] ≤ a.length \$ .

class Program
{
  static void Main(string[] args)
  {
    int[] a = {2, 3, 3, 1, 5, 2};

    int f = FirstDuplicate(a);

    Console.ReadLine();
   }
public static int FirstDuplicate(int[] a)
{

    int[] answer = new int[2];
    answer[0] = -1;
    answer[1] = a.Length;
    for (int i = 0; i < a.Length - 1; i++)
        for (int j = i; j < a.Length; j++)
            if (a[i] == a[j] && i != j)
            {
                if (i < answer[1])
                {
                    answer[0] = a[i];
                    answer[1] = j;
                    break;

                }
            }

    return answer[0];
  }
}

For a small array or list of a handful of items, this should be very fast.  But is it efficient as you would scale out?  No.

Before getting on to a better way to achieve the it for a larger number of items, let's look at your current code.  You have some checks that aren't needed.

if (a[i] == a[j] && i != j)

The check of i != j is not needed if you were to simply initialize j to be i + 1 as in:

for (int j = i + 1; j < a.Length; j++)
    if (a[i] == a[j])

The check of if (i < answer[1]) is totally unnecessary.  In fact, answer as an array is not needed at all as you do not really do anything meaningful with answer[1].

But having 2 loops is not efficient as the input array could be larger.  As mentioned in a comment, the use of a HashSet could be used.  I am changing a few things.

The method signature is now IEnumerable<int> to allow for arrays,
lists, or any collection of integers.
I use a HashSet to keep track of items I have already
encountered.
The return type is a Nullable<int>, also written as int?, so that
if there are no duplicates found a null is returned.

The resulting method also looks cleaner.  Someone looking at this understands more readily what the code is trying to achieve:

public static int? FirstDuplicate(IEnumerable<int> items)
{

    HashSet<int> set = new HashSet<int>();
    foreach (int item in items)
    {
        if (set.Contains(item))
        {
            return item;
        }
        set.Add(item);
    }
    return null;
}

As shown in @Denis's answer, and echoed by @Corak, is the method could be made slightly faster and shorter by reducing the logic inside the foreach loop:

if (!set.Add(item))
{
    return item;
}

While this again is slightly faster, slighter cleaner, and recommended in practice, it still lags well behind @vnp 's array solution.

UPDATED Worst Case Array

I composed a worst case array to test of 100000 items where the very last item is the first duplicate of the 2nd to last item.  Your code and mine returned the correct value.

public static int[] WorstCaseArray
{
    get
    {
        int[] items = new int[(int)Math.Pow(10, 5)];
        for (int i = 0; i < items.Length; i++)
        {
            items[i] = i + 1;
        }
        items[items.Length - 1] = items[items.Length - 2];
        return items;
    }
}

UPDATE #2 - Worst Case

Obviously we should be working with the largest array allowed.

As @PeterCordes states in a comment to @vnp's answer, the list should be randomized.

As @Denis states, the worst case for a HashSet would be if the only duplicate was the first and last item.  An extremely close second would be if the last and 2nd to last item were the only duplicate.  Note this second worst case for HashSet would be the worst case for @vnp's array solution, so I will use it as my worst case.

public static int[] WorstCaseArray
{
    get
    {
        // Worst case is largest allowed array ...
        int max = (int)Math.Pow(10, 5);
        int[] items = new int[max];
        for (int i = 0; i < items.Length; i++)
        {
            items[i] = i + 1;
        }
        // ... that is randomly shuffled ...
        Shuffle(items);
        // ... with the only duplicate being the last item matching the 2nd to last!
        items[items.Length - 1] = items[items.Length - 2];
        return items;
    }
}

private static Random random = new Random();
public static void Shuffle(int[] items)
{
    //Fisher-Yates method to shuffle
    for (int i = items.Length - 1; i > 0; --i)
    {
        int randomIndex = random.Next(i + 1);
        int temp = items[i];
        items[i] = items[randomIndex];
        items[randomIndex] = temp;
    }
}

UPDATED Worst Case Timings:

Yours: 14.5105877 seconds
Mine : 00.0065640 seconds
vnp's: 00.0012733 seconds

Tiny array with many duplicates

However, as I said for only a handful of items yours is quite fast.  The problem is - it produces the wrong answer!  Using this array:

int[] array =  new[] { 1, 2, 3, 4, 5, 6, 3, 4, 5, 6 };

Where the expected answer would be 3, yours returns 6.

Yours: took 0.0006141 seconds but returned 6 (wrong)
Mine : took 0.0006162 seconds
vnp's : 0.0003843 seconds

Large, Random Array with No Duplicates

To round out the answer, let's look at the largest allowed array, randomized, but without any duplicates.  Using the shuffle routine from above:

public static int[] NoDuplicates()
{
    int[] items = Enumerable.Range(1, (int)Math.Pow(10, 5)).ToArray();
    Shuffle(items);
    return items;
}

Timings:

Yours: 15.1978462 seconds
Mine  : 00.0055364 seconds
vnp's : 00.0014876 seconds

BEST ANSWER

Also, since you have tagged this question with [time-limit-exceeded], vnp's answer is not just clever, but also the fastest in all tested scendarios, which makes it the best answer (regarding performance).

Your code it O(n^2).  This can be done in O(n).

int j = i can be int j = i + 1 and you can skip the if (a[i] == a[j] && i != j)

Looks like you are returning the last duplicate - not the first.

Array lookup is very fast but you should capture the value in the outer loop.  See DoubleLoop below.

Array based is much faster than HashSet but if you did not have the condition 1≤a[i]≤a.length you could not use array.  In the latest version of .NET I think you can pre-allocate the HashSet capacity but Array would probably still be faster.

HashSet     time    434  value 1334
Array       time     46  value 1334
Double loop time 15,321  value 1334

.

public static void TestDup()
{
    Random rand = new Random();
    int max = 2000;
    int[] array = new int[max];
    for(int i = 0; i < max; i++)
    {
        //array[i] = rand.Next(1, max);
        array[i] = i + 1;
    }
    //array[max - 1] = max - 1; //not fair to double loop
    array[max / 3] = array[max * 2 / 3];
    Stopwatch sw = new Stopwatch();
    sw.Start();
    int? dup = null;
    for (int i = 0; i < max; i++)
    {
        dup = FindFirstDupicateHashSet(array);
    }
    sw.Stop();
    Debug.WriteLine($"HashSet time {sw.ElapsedMilliseconds.ToString("N0")}  value {dup}");
    sw.Restart();
    dup = null;
    for (int i = 0; i < max; i++)
    {
        dup = FindFirstDupicateArray(array);
    }
    sw.Stop();
    Debug.WriteLine($"Array time {sw.ElapsedMilliseconds.ToString("N0")}  value {dup}");
    sw.Restart();
    dup = null;
    for (int i = 0; i < max; i++)
    {
        dup = FindFirstDupicateDoubleLoop(array);
    }
    sw.Stop();
    Debug.WriteLine($"Double loop time {sw.ElapsedMilliseconds.ToString("N0")}  value {dup}");
    Debug.WriteLine("done");
}
public static int? FindFirstDupicateHashSet(int[] array)
{
    int len = array.Length;
    HashSet<int> hs = new HashSet<int>();
    foreach(int i in array)
    {
        if(!hs.Add(i))
        {
            return i;
        }
    }
    return null;
}
public static int? FindFirstDupicateArray(int[] array)
{
    int[] compute = new int[array.Length];
    foreach(int i in array)
    {
        //Debug.WriteLine($"{i} {compute[i-1]}");
        if(compute[i - 1] > 0)  // 0 is default value
        {
            return i;
        }
        compute[i - 1] = 1;
    }
    return null;
}
public static int? FindFirstDupicateDoubleLoop(int[] array)
{
    int len = array.Length;
    int iVal;
    for (int i = 0; i < len - 1; i++)
    {
        iVal = array[i];
        for (int j = i + 1; j < len; j++)
        {
            if (iVal == array[j])
            {
                return iVal;
            }
        }
    }
    return null;
}


Generate all permutations of a list in Python This is my solution to the "Permutations" problem from Leetcode:

  Given a collection of distinct numbers, return all possible permutations.

I know this is a common routine that can be done much faster using itertools.permutations but I want to write it to learn the algorithm itself.

I would like any general feedback on the implementation of the recursive algorithm. But I am also curious whether I should use an instance helper method as it is now, or make the helper method a class or static method. If someone can show me how to re-write the helper method as a class or static method that would be appreciated.

class Solution(object):
    def permute(self, nums):
        res = []
        self._permuteHelper( nums, 0, res)
        return res

    def _permuteHelper(self, nums, start, results):  #helper method
            if start >= len(nums):
                results.append(nums[:])
            else:
                for i in range(start, len(nums)):
                    nums[i], nums[start] = nums[start], nums[i]
                    self._permuteHelper(nums, start +1, results)
                    nums[start], nums[i] = nums[i], nums[start]

s = Solution()
nums = [1, 2, 3]
print s.permute(nums)

There are a lot of suggestions in the comments and answers about using library functions. For programming challenge, I am not interested in copying and pasting library functions. Yes, this is re-inventing the wheel if you want to call it that!

Instead of calling a recursive function you could do it in place in an iterative way.

Like this:

l = [1, 2, 3, 4, 5]

def permute(l):
    n = len(l)
    result = []
    c = n * [0]

    result.append(l)

    i = 0;
    while i < n:
        if c[i] < i:
            if i % 2 == 0:
                tmp = l[0]
                l[0] = l[i]
                l[i] = tmp

            else:

                tmp = l[c[i]]
                l[c[i]] = l[i]
                l[i] = tmp

            result.append(l)
            c[i] += 1
            i = 0
        else:
            c[i] = 0
            i += 1

    return result

print(permute(l))

This is not my algorithm, it is called the Heap algorithm

Naming a class Solution because it is part of the solution of a coding excercise seems like a very bad practice to me since it does not convey its purpose or the problem it's trying to solve.

In fact, using a class for your problem, may be overkill.
It has no own __init__ implementation and only one exposed method.

Both methods of Solution namely permute and _permuteHelper could be functions, since they do not use any other properties from their class.

Furthermore, as already said in the comments, itertools.permutations already implements your problem's solution.

Also _permuteHelper should be named _permute_helper to comply with PEP8.

If you really require an object-oriented solution, I'd suggest inheriting list and giving it a permutations method:

from itertools import permutations

class PermutableList(list):
    """A list with a permutations method."""

    def permutations(self, len=None):
        """Yields permutations of the list."""
        return permutations(self, len)

if __name__ == '__main__':
    pl = PermutableList(range(1, 4))
    print(list(pl.permutations()))


Comparing Equals() method from MSDN I've implemented the Equals() support for my class as follows:

public override bool Equals(object obj)
{
  return Equals(obj as TwoDPoint);
}

public bool Equals(TwoDPoint p)
{
  return ((object)p != null) && (x == p.x) && (y == p.y);
}

This obviously doesn't match the reference implementation on MSDN, here, as they add explicit checking for null before and after converting to a TwoDPoint, and they implement the actual equality test in each method (i.e. no call from one to the other).

I like my implementation as it's much more succinct. But I'm wondering - what have I missed? Am I losing (significant) performance with my version? Is there actually a bug in this approach?

I like my implementation as it's much more succinct

Which isn't necessarily a good thing: we care about readable code, not about oneliners. It means we don't use 5 lines to write what can be written in 1 line but it also means we don't cram 5 lines in 1 line just for the sake of it.

That being said: your implementation does the same as the example. I would also prefer your example since it keeps the equality logic in one method Equals(T) rather than duplicating it.

Some comments however:

There's no point in doing this cast: (object)p != null
You might as well implement IEquatable<T> MSDN
There should be no performance difference unless you have an unusual amount of null arguments to Equals(object) that will now go through the as statement before being caught by the null check
Equals(T) will probably be inlined anyway

Warning

void Main()
{
    bool testOutcome;
    var testObject1 = new RecursiveEquals();
    var testObject2 = new RecursiveEquals();

    //// These will work
    testOutcome = (object)testObject1 == testObject2;
    testOutcome = (object)testObject1 != testObject2;

    //// These all crash
    //  testOutcome = testObject1 == testObject2;
    //  testOutcome = testObject1 != testObject2;
    //  testOutcome = testObject1.Equals(testObject2);
    //  testOutcome = ((object)testObject1).Equals(testObject2);
}

class RecursiveEquals
{
    public override bool Equals(object other)
    {
        // this would typically be less obvious
        return other.Equals(this);
    }

    public static bool operator ==(RecursiveEquals left, object right)
    {
        return ((object)left).Equals(right);
    }

    public static bool operator !=(RecursiveEquals left, object right)
    {
        return !((object)left).Equals(right);
    }
}

Casting p as a less derived object will NOT invoke the less derived implementation of the method.

I've tried to make the recursive .Equals call obvious enough to illustrate what is a more subtle common problem when people override it. In this answer, I am focusing on the point that casting the object does not change the method implementation which is used.

Unless casting to an interface type which is implemented explicitly in the Class's hierarchy, this will invoke the most derived definition. To not work this way would violate polymorphism principles.

This will work for operator overloads in .NET as you are doing. Until I started researching this answer, I was not aware that operator overloads do NOT actually use the most derived implementation. I personally find this result surprising.


Check if a Binary Tree  is aBinary Search Tree I'm understanding the question, but I want to know whether or not my implementation is correct.

The question is:

  Write a method that accepts as its argument a BinaryTree object and
  returns true if the argument tree is a binary search tree. Examine
  each node in the given tree only once.

This is my implementation:

public boolean isBST (BinaryTree<String> tree)
{
    BinaryNode Node = new BinaryNode (tree.getRootData);
    if(tree == null || Node.isLeaf() )
        return true;

    if(Node.getData > Node.getLeftChild && Node.getData < Node.getRightChild)
    {
        boolean leftTree = isBST(new BinaryTree(Node.getLeftChild));
        boolean rightTree = isBST(new BinaryTree(Node.getRightChild));
        return leftTree && rightTree ;
    }
    else return false ;
}

There are some serious problems with this function.

Issues of correctness

Code won't compile: If BinaryNode indeed has fields named getData, getLeftChild, and getRightChild that are of type String, then you can't use operators < and > on them.  For strings, as with any object, you have to use a.compareTo(b).
No consideration for unbalanced or incomplete trees: If a node has only one child, what happens?
Check for tree == null after tree.getRootData: There's no point in checking for tree == null, since it would have crashed on tree.getRootData with a NullPointerException already if that were the case.
Recursive check is insufficient: As @user2668926 points out, you have to verify that all nodes within a subtree fall within a range of values.

Serious violations of style conventions

Using an uppercase variable name: In Java, variable names should be lowerCase, except constants, which should be ALL_CAPS.  By naming a variable Node, you hurt readability tremendously by making it look like the name of a class.
Using getSomething as field names: The convention is that getSomething() is the name of a getter method (i.e., a member function), usually corresponding to an instance variable named something.  Instead, you seem to have instance variables named getRootData, getData, getLeftChild, and getRightChild in your BinaryTree and BinaryNode classes, which is exceedingly weird.

I consider these two issues to be extremely serious, even if the compiler accepts the code and the algorithm runs.  These are not "merely" conventions — they are major traps for anyone else who works with your code, enough for every programmer to go facepalm.

Less serious issues of style

Distinction between BinaryTree and BinaryNode: Wrapping nodes as trees and unwrapping the tree's root node is cumbersome.  Do you really have to make a distinction between trees and nodes?  Could you not just treat any node as a tree rooted there?
Use of generics: The BinaryTree class is genericized, so your code should also aim to be generic.  The method signature should look something like

public class BSTVerifier {
    public static <T extends Comparable> boolean isBST(BinaryTree<T> tree) {
        ...
    }
}

Alternatively,

public class BSTVerifier<T extends Comparable> {
    public boolean isBST(BinaryTree<T> tree) {
        ...
    }
}

As mariosangiorgio already said, the concept looks good, with some small improvements possible.

You should write some tests to test it. You would easily find out the mentioned problems with some good tests.

To get you an idea of a fixed version, a typical implementation could look like:

public boolean isBinarySearchTree(BinaryTree tree)
{
    if (tree == null)
        throw new IllegalArgumentException("Tree is null");
    return isBinarySearchTree(tree.getRootNode());
}

private boolean isBinarySearchTree(Node node) // note: the name is not completely correct, because we investigate a node now. But for private and together with the other method, it would be fine
{
    if(node == null || node.isLeaf() ) //hint: node == null can only happen in one case, we could remove it here, if we change the other method
        return true;

    if (!(node.getLeftChild().getData().compareTo(node.getData()) <= 0 &&
                         node.getData().compareTo(node.getRightChild().getData()) <= 0))
        return false; // not sorted

    return isBinarySearchTree(node.getLeftChild()) && isBinarySearchTree(node.getRightChild());
}

A final suggestion: Avoid abbreviations.


Counting numbers whose digits all go up or down I have created a program which takes a value x and uses this value to create a power of ten value like so 10 ** x.

This has then been used as the maximum number of a range 1..(10 ** x), which has been expanded using the splat * method to create an array of numbers, and then assigned to the variable num_array.

Using the values in this array, I want to find the total number of increasing and decreasing integers within this range. These are integers containing digits that either ascend or descend with each proceeding digit.

For example, if

x = 3

then

num_array = [*1..(10**3)] #=> num_array = [1, 2, 3, 4, .. 998, 999, 1000]

Then numbers like 234 which is ascending and 987 which is descending should be counted. Numbers like 555 fall into both categories so also should be counted but numbers like 482 should not because it doesn't follow either rule.
The each method is used to evaluate which integers follow this rule in the range.

My problem is as the powers of ten increase with each method call at the end of the program, it gets slower and slower to return the end result until it calls total_nums(6) which then takes too long.

Is there a general rule of thumb to help process really large arrays like this so it's much faster for your program to return the end result?
What would you say are your preferred methods for such an approach like this?
And would something like Memoization work here?

def total_nums(x)
   return 0 if x == nil
   return 1 if x == 0

   num = 10 ** x
   num_array = [*1..num]
   total = 0

   num_array.each do |i|
         if i == i.to_s.split("").sort.join.to_i || i == i.to_s.split("").sort.reverse.join.to_i
         total +=1
         end
   end

   return total

end

total_nums(0) #=> 1
total_nums(1) #=> 10
total_nums(2) #=> 100
total_nums(3) #=> 475
total_nums(4) #=> 1675
total_nums(5) #=> 4954
total_nums(6) #=> 12952

To make Ruby internally use a loop instead of an Array you may want to use 1.upto(10 ** x) that is an Enumerator.

But I doubt the "HUGE array" is the performance issue here.

First of all Ruby says to do total = .count{ ... } instead of total = 0; .each{ total += 1 if ... }
Then do this thing only once and store in a variable t = i.to_s.split("").sort -- x2 faster
Then use .chars instead of .split("") -- x4 faster

Then conversions between Integer and String seem to be unnecessary:

def total_nums x
  return 0 if x == nil
  return 1 if x == 0

  1.upto(10 ** x).count do |i|
    s = i.to_s.chars
    t = s.sort
    s == t || s == t.reverse
  end
end

Note: probably you want either return 0 if x == 0 or start the loop from 0, not 1.

I found quite a concise answer online that seems to be faster than the above.

def total_nums(x)
    xf, nf, tf = (1..x).inject(1,:*), (1..9).inject(1,:*), (1..x+9).inject(1,:*)
    (x+10)*tf/xf/nf/10 + tf/xf/nf - 10*x - 1
end

When doing a Benchmark comparison test, it certainly shows the difference in processing speed for these two results. If I tried a greater method call than total_nums(6) for the 1st method it would take too long. Here's the result for those interested.

require 'benchmark'

def total_nums x
  return 0 if x == nil
  return 1 if x == 0

  1.upto(10 ** x).count do |i|
    s = i.to_s.chars
    t = s.sort
    s == t || s == t.reverse
  end
end

def total_nums2(x)
        xf, nf, tf = (1..x).inject(1, :*), (1..9).inject(1, :*), (1..(x + 9)).inject(1, :*)
        ( ((x + 10) * tf) /xf/nf/10) + (tf/xf/nf) - (10 * x) - 1
end

Benchmark.bm do |x|
  x.report { puts total_nums(6) }

        #=>  user     system      total        real
        #=> 12952
        #=>  2.720000   0.010000   2.730000 (  2.760713)

  x.report { puts total_nums2(6) }

        #=>  user     system      total        real
        #=> 12952
        #=>  0.000000   0.000000   0.000000 (  0.000049)
end


Syllable-counting function I was working on a syllable counting function for a text editor(this function is accurate enough). However, I would like to know if it is possible to optimize it, it already separated into another thread, but I would like to know if there is any kind of optimization that I can do with it to make it more efficient.

private static int SyllableCount(string word)
{
    word = word.ToLower().Trim();
    int count = System.Text.RegularExpressions.Regex.Matches(word, "[aeiouy]+").Count;
    if ((word.EndsWith("e") || (word.EndsWith("es") || word.EndsWith("ed"))) && !word.EndsWith("le"))
        count--;
    return count;
}

It uses regular expressions, something that this source mentions having poor performance in .net applications, is that the case? And if not, are there any other optimizations that I can perform on it?

Anyway, it does not lag much, but my application does use around four threads already, just for keeping up with various text entry statistics, so now I'm just trying to shave the fat off so to speak.

Well, Regex is going to be slow, by virtue of it being a very powerful, flexible engine that can't assume you'll never want to do something that a regex can achieve. This particular regex pattern is pretty simple (no lookbehinds, etc) but there will be some overhead inherent in Regex use which you can trim. You can iterate through the string and count occurrences of groups of vowels in linear time with very little overhead.

private static int SyllableCount(string word)
{
    word = word.ToLower().Trim();
    bool lastWasVowel;
    var vowels = new []{'a','e','i','o','u','y'};
    int count;

    //a string is an IEnumerable<char>; convenient.
    foreach(var c in word)
    {
       if(vowels.Contains(c))
       {
          if(!lastWasVowel)
             count++;
          lastWasVowel = true;
       }
       else
          lastWasVowel = false;
    }

    if ((word.EndsWith("e") || (word.EndsWith("es") || word.EndsWith("ed")))
          && !word.EndsWith("le"))
        count--;

    return count;
}

I'd A/B the above algorithm against the one you already have; you should see at least some performance increase. Notice that although this may well be faster as it does exactly what you want and doesn't see if you want to do anything else, it uses more LOC to achieve the same result. This is Regex's real power; powerful string analysis with very concise code.

If you're worried about regexp being slow, you could try a simple Split():

int count = word.Split(new char[] { 'a','e','i','o','u','y' },
                       StringSplitOptions.RemoveEmptyEntries).Length ;

It would be easy to benchmark the difference.