Calculate highest prime factor very slow Sometimes when I write a Python 3.x script with too many nested loops and some big values it just takes too long to give the answer. How can I optimize my code so that it runs faster, or where should I optimize?

def isprime(n):
    '''check if integer n is a prime'''
    # make sure n is a positive integer
    n = abs(int(n))
    # 0 and 1 are not primes
    if n < 2:
        return False
    # 2 is the only even prime number
    if n == 2:
        return True
    # all other even numbers are not primes
    if not n & 1:
        return False
    # range starts with 3 and only needs to go up the squareroot of n
    # for all odd numbers
    for x in range(3, int(n**0.5)+1, 2):
        if n % x == 0:
            return False
    return True

ans=0
for i in range(600851475143,0,-1):
if 600851475143%i==0:
    if isprime(i)==True:
        ans=i
        break;
    else:
        continue;
print(ans)

This program should calculate the highest prime factor of the number 600851475143, but it didn't give any answer for half an hour.

You want to compute the largest prime factor of a given number, but it is actually more efficient to look for the smallest prime factors first (and I am just repeating my arguments from my answer). If you test the possible factors in increasing order and divide the given number by a found factor as much as possible, then each found factor is necessarily a prime number. This means that no primality test is needed at all.

This leads to the following function (which is just a translation of the referenced
answer to Python):

def max_prime_factor(n):
    '''find the largest prime factor of integer n'''

    largest_factor = 1
    i = 2

    # i is a possible *smallest* factor of the (remaining) number n.
    # If i * i > n then n is either 1 or a prime number.
    while i * i <= n:
        if n % i == 0:
            largest_factor = i
            # Divide by the highest possible power of the found factor:
            while n % i == 0:
                n //= i
        i = 3 if i == 2 else i + 2

    if n > 1:
        # n is a prime number and therefore the largest prime factor of the
        # original number.
        largest_factor = n

    return largest_factor

Timing for the largest prime factor of 600851475143 (on a 3,5 GHz Intel Core i5 iMac):

Your method: 43 seconds
Sieve method from @wei2912: 0.2 seconds
Above method: 0.016 seconds


MVC Layout - Which way to add listeners is better? So I'm doing a basic MVC layout for a pretty basic game that I am making. The game requires the user to move up/down/left/right via buttons on the GUI. Since I'm using an MVC layout and my buttons are in a different class than the ActionListeners, I was wondering what the best way to add the action listeners are?

Method 1:

View Class ActionListener method:

public void addMovementListeners(ActionListener u, ActionListener d, ActionListener l, ActionListener r){
    moveUp.addActionListener(u);
    moveDown.addActionListener(d);
    moveLeft.addActionListener(l);
    moveRight.addActionListener(r);
}

Control Class add ActionListener method:

private void addListeners(){
    viewGUI.addMovementListeners(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPressed = 1;
        }
    },
    new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPressed = 2;
        }
    },
    new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPressed = 3;
        }
    },
    new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            buttonPressed = 4;
        }
    });
}

Method 2:

View Class ActionListener method:

public void addMovementListeners(ActionListener a){
    moveUp.addActionListener(a);
    moveDown.addActionListener(a);
    moveLeft.addActionListener(a);
    moveRight.addActionListener(a);
}

public JButton[] getButtons(){
    JButton[] temp = new JButton[4];
    temp[0] = moveUp;
    temp[1] = moveDown;
    temp[2] = moveLeft;
    temp[3] = moveRight;

    return temp;
}

Control Class add ActionListener method:

JButton[] movementButtons;

private void addListeners(){
    movementButtons = viewGUI.getButtons();
    viewGUI.addMovementListeners(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            if(e.getSource() == movementButtons[0]){
                buttonPressed = 1;
            }else if(e.getSource() == movementButtons[1]){
                buttonPressed = 2;
            }else if(e.getSource() == movementButtons[2]){
                buttonPressed = 3;
            }else if(e.getSource() == movementButtons[3]){
                buttonPressed = 4;
            }
        }
    });
}

Which method is better? Is there another method that is even better than these two? Let me know! Trying to get this MVC thing down :). (Any other suggestions welcome as well)!

Method #1 feels like the better approach, because it doesn't require your view to expose its buttons, but the four-times ActionListener method seems odd.  Both methods have some trouble with magic numbers.

Introduce an enum for directions.  It may feel like overkill at the moment, but it'll take care of your direction-to-action mapping.  It will also help shape some of your options.

Then consider supplying Action instead of ActionListener.  This will also make it easier to enable/disable the movement buttons in case something blocks your way (like going off-field), without your view having to expose its buttons.

When you combine these two, you get something like the following:

View:
    void setAction(Direction dir, Action action);

Model:
    void move(Direction dir);
    /** Returns directions possible to move to at this moment. */
    Set<Direction> getPossibleDirections();

enum Direction { UP, RIGHT, DOWN, LEFT; }

class Controller {
  Map<Direction, MoveAction> actions;

  public void setView(View view) {
    actions = new EnumMap<>(Direction.class);
    for ( final Direction direction : Direction.values() ) {
      final MoveAction action = new MoveAction(direction, direction.toString());
      actions.put(direction, action);
      view.setAction(direction, action);
    }
  }

  void refreshState() {
    final Set<Direction> dirs = model.getPossibleDirections();
    for ( final MoveAction action : actions.values() ) {
      action.setEnabled(dirs.contains(action.direction));
    }
  }

  class MoveAction extends AbstractAction {
    final Direction direction;

    MoveAction(Direction direction, String name) {
      super(name);
      this.direction = direction;
    }

    public void actionPerformed(ActionEvent evt) {
      model.move(direction);
      refreshState(); // or have model firePropertyChange
    }
  }
}


The trip programming challenge I would like some feedback on my solution for the trip programming challenge. Please go through my code understanding that I am interested in interviewing for a dev position at some point and am also interested in competitive programming (I'm just getting started):

'''

@author: Teererai
This code is intended to calculate the minimum that a number of students needs to exchange
 in order to get the ammounts that they spend to within one cent of each other. The idea
 is to calculate the average then round it up and down to the nearest cent. The next step
 is to calculate the cumulative absolute positive and negative difference for each ammount spen
 from the nearest rounded average spend and take the maximum between the cumulative positive and negative difference.
'''
import sys
from sys import stdin
import math
from decimal import Decimal

def main():
    num_students=int(stdin.readline()) #read number of students
    while num_students!=0:
        i=num_students
        total_spending=0
        student_spending=[] # stores amount spend by each student
        pos_difference=0 # Keeps track of cumulative absolute difference between average spend and ammounts higher than the average.
        neg_difference=0 #Keeps track of cumulative negative difference.
        for i in range(i):
            current_spending=float(stdin.readline())
            student_spending.append(current_spending)
            total_spending+=current_spending
        average_spending=total_spending/num_students
        #these following two variables account for the fact that we can never get to the exact average.
        lower_average_spending=math.floor(average_spending*100)/100
        upper_average_spending=math.ceil(average_spending*100)/100
        #As the challenge specifies, we are not aiming to get the ammount spend to the exact average but rather to either the nearest cent above and below it.
        for element in student_spending:
            if element>average_spending:
                pos_difference=pos_difference+element-upper_average_spending
            else:
                neg_difference=neg_difference+lower_average_spending-element
        amount_exchanged=Decimal(max(neg_difference,pos_difference))
        amount_exchanged=round(amount_exchanged,2)
        print("$",amount_exchanged,sep="")
        num_students=int(stdin.readline())

    return
main()
sys.exit(0)

p.s. the code works perfectly and has been accepted by the uva online judge. I am simply looking for advice.

Readability counts
You follow PEP8 naming conventions but fail to follow layout ones. The result is a big block of code that get harder to read than necessary. Use spaces and blank lines to give the reader some rest.
You should also get yourself familiar with the if __name__ == '__main__' idiom and remove some unnecessary stuff:

return at the end of a function has no added value;
same for sys.exit(0) at the end of a program.

Break functionnality into tinier function
As it stand, your main function does 3 things:

Compute the required value;
Parse the input for information about computing such value;
Parse the input to repeat such computation for a number of trips.

You should use 3 functions instead, each responsible of its own task. Or at least split the "parse input for a trip and compute the value for this trip" part into its own function.
Define variables when you need them
Python is a dynamic language, you don't need to define variables before using them, nor preallocate memory for them. Just declare a new variable when you need it.
You should also aim at removing the number of intermediate variables that you use only once.
Use builtins
input, max, sum and list-comprehensions can help you express your thoughts in a more straighforward way.
str.format can help you with rounding and formatting output.
Proposed improvements
import math
from decimal import Decimal

def compute_expenses_for_a_trip(students_count):
    students_spending = [float(input()) for _ in range(students_count)]
    average_spending = sum(students_spending) / students_count

    # these following two variables account for the fact that we can never get to the exact average.
    lower_average_spending = math.floor(average_spending * 100) / 100
    upper_average_spending = math.ceil(average_spending * 100) / 100

    pos_difference = sum(element - upper_average_spending for element in students_spending if element > average_spending)
    neg_difference = sum(lower_average_spending - element for element in students_spending if element < average_spending)
    return Decimal(max(pos_difference, neg_difference))

def trip_programming_challenge():
    while True:
        num_students = int(input())  # read number of students
        if not num_students:
            break
        amount_exchanged = compute_expenses_for_a_trip(num_students)
        print('${:.2f}'.format(amount_exchanged))

if __name__ == '__main__':
    trip_programming_challenge()

I didn't enforce a line length of 80 characters but you might want to.
I'm also unsure why you introduced Decimal that late into the computation. Either you use them all along or you remove them, but introducing them halfway through kind of defeat their purpose.
Also note that the two summed differences are mainly proposed as an alternative syntax but since they iterate two times over the same list you may find your for loop faster. If it matters, profile and choose the best.


How to implement a dynamic temperature conversion method based on given enums? I am making some basic temperature conversion methods, and I want the end result to look something like this:

double celsius = TemperatureConvert.convert(10, Temperature.FAHRENHEIT, Temperature.CELSIUS);
// Where 'Temperature' is an enum with three values: FAHRENHEIT, CELSIUS, and KELVIN
// This mehtod is read logically as "convert 10 degrees fahrenheit to celsius"

Originally, I thought that the best approach was to have some private methods inside TemperatureConvert to do the conversion for me. I quickly discovered I was wrong. I want to make this method dynamic, so I used my Google Fu and found this answer. If you don't want to read it, the answers basically tell OP about the ability to have an abstract method in the enum class, and then have each enum override it.

I am having some trouble figuring out the best approach. Here are the two possibilities I came up with:

Create two abstract methods, convertA() and convertB(). Have convertA() convert to the unit that is first in alphabetical order, and have convertB() convert the other unit. In the TemperatureConvert.convert() method, I can figure out what method to use by comparing the enum name. This also brings up an issue if the same unit is passed into the convert() method, as there is no method to do any conversion. However, no conversion is needed so that might not be a problem.
Create three abstract methods, convertToCelsius(), convertToFahrenheit(), and convertToKelvin(). Use a switch statement to decide which method to use.

The java.util.concurrent.TimeUnit enum class provides both a generic conversion method and specific methods for each modeled unit, e.g., toHours. You could do something similar and still provide the third-party TemperatureConvert.convert method if you want.

Here's the implementation of SECONDS:

SECONDS {
    public long toNanos(long d)   { return x(d, C3/C0, MAX/(C3/C0)); }
    public long toMicros(long d)  { return x(d, C3/C1, MAX/(C3/C1)); }
    ...
    public long toHours(long d)   { return d/(C5/C3); }
    ...
    public long convert(long d, TimeUnit u) { return u.toSeconds(d); }
},

Note that the author went for efficiency by precalculating many of the conversion constants. I wouldn't bother with that to start as it's probably premature for your needs. Your conversion methods should start simply:

CELCIUS {
    public double toFahrenheit(long t) {
        return t * 9 / 5 + 32;
    }
    public double toKelven(long t) {
        return t + 273;
    }
    public double convert(double t, TemperatureUnit u) {
        return u.toCelcius(v);
    }
}

Also, all of the conversion methods are stubs that throw an exception rather than abstract. Why? I don't know. If these weren't enums and could be extended, stub methods that fail only when called are handy for allowing optional operations--think Collection.remove(Object) which throws UnsupportedOperationException if not implemented by the concrete container.


Merge sorting linked lists This works as expected and I want to get this code reviewed so that I can improve my coding ability.

I am concerned about readability and the quality of the code

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

struct node
{
    int element;
    struct node *next;
};

struct node* mergeList(struct node *list1, struct node *list2)
{
    struct node *root = NULL, *cur = NULL;

    if(list1 == NULL)
    {
        return list2;
    }

    if(list2 == NULL)
    {
        return list1;
    }

    while( (list1 != NULL) && (list2 != NULL) )
    {
        struct node *tmp = malloc(sizeof(struct node));
        if( list1->element  < list2->element )
        {
            tmp->element = list1->element;
            tmp->next = NULL;
            list1 = list1->next;
        }
        else
        {
            tmp->element = list2->element;
            tmp->next = NULL;
            list2 = list2->next;

        }
        if(root == NULL)
        {
            root = tmp;
            cur = root;
        }
        else
        {
            cur->next = tmp;
            cur = tmp;
        }

    }

    while(list1 != NULL)
    {
        struct node *tmp = malloc(sizeof(struct node));
        tmp->element = list1->element;
        tmp->next = NULL;
        cur->next = tmp;
        cur = tmp;
        list1 = list1->next;
    }
    while(list2 != NULL)
    {
        struct node *tmp = malloc(sizeof(struct node));
        tmp->element = list2->element;
        tmp->next = NULL;
        cur->next = tmp;
        cur = tmp;
        list2 = list2->next;
    }

    return root;

}

void findMid(struct node *root, struct node **list1, struct node **list2)
{

    struct node *slow;
    struct node *fast;
    printf("\nIn Findmis\n");
    if( (root == NULL) || (root->next == NULL) )
    {
        *list1 = root;
        *list2 = NULL;
        return;
    }
    else
    {

        slow =  root;
        fast = root->next;
        while (fast != NULL)
        {
            fast = fast->next;
            if(fast!=NULL)
            {
                slow = slow->next;
                fast = fast->next;
            }

        }

        *list1 = root;
        *list2 = slow->next;
        slow->next = NULL;

    }

}

void mergeSort(struct node **root)
{
    struct node *list1, *list2;
    struct node *head = *root;
    if( ( head  == NULL) || (head->next == NULL))
    {
        return;
    }

    findMid(head, &list1, &list2);

    if(list1 != NULL)
    printf("\nList1 = %d", list1->element);
    if(list2 != NULL)
    printf("\nList2 = %d", list2->element);
    mergeSort( &list1 );
    mergeSort( &list2 );

    *root = mergeList( list1, list2 );

}

void insertNodes(struct node **root, int key)
{
    struct node *tmp = malloc(sizeof(struct node));
    tmp->element = key;
    tmp->next = *root;
    *root = tmp;

}

void printList(struct node *root)
{
    while( root!= NULL)
    {
        printf("\n%d\n", root->element);
        root = root->next;
    }

}

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

    struct node *root1 = NULL;

    struct node *root2 = NULL;
    insertNodes( &root1, 1);
    insertNodes( &root1, 2);
    insertNodes( &root1, 3);
    insertNodes( &root1, 4);
    insertNodes( &root1, 5);
    printList(root1);
    mergeSort( &root1);
    printList(root1);
    return 0;

}

Preliminaries

Your compiler should have warned you that in main(), you never use list2.  You do compile with warnings turned on, I hope?

Usually, the first node of a linked list is called the head.  "Root" usually refers to the top of a tree.

mergeList() and findMid() are helper functions, which are designed to be used exclusively to support mergeSort().  It's a good habit to declare helper functions with the static modifier, so that they will be invisible to code in other .c files.

findMid()

The function could be better named, I think.  "Find" sounds like a passive action, so you wouldn't expect that it might modify its inputs (which it does at the end: slow->next = NULL).  I suggest calling it bisectList() instead.

Calling the function will always result in setting *list1 to root, so why bother making list1 an out-parameter?  Going a step further, if there's only one output (list2), you can just return it normally.

So, the function could become static struct node *bisectList(struct node *head).  Here's a more compact implementation.

/**
 * Returns a pointer to a node near the middle of the list,
 * after having truncated the original list before that point.
 */
static struct node *bisectList(struct node *head)
{
    /* The fast pointer moves twice as fast as the slow pointer. */
    /* The prev pointer points to the node preceding the slow pointer. */
    struct node *fast = head, *slow = head, *prev = NULL;

    while (fast != NULL && fast->next != NULL)
    {
        fast = fast->next->next;
        prev = slow;
        slow = slow->next;
    }

    if (prev != NULL)
    {
        prev->next = NULL;
    }
    return slow;
}

mergeList()

The function signature looks fine.  There is altogether too much code, though.

One major problem, which @rolfl alluded to, is that you are duplicating every node that you encounter.  Rather, you should be able to merge the lists just by manipulating the next pointers of the existing nodes.

To start with, you can eliminate the two special cases before the loop.  (That meant, by the way, that your function not only leaked memory, it also had an inconsistent memory management policy: sometimes it would duplicate the list, and sometimes not.)

Once we have decided not to duplicate the nodes, the epilogue becomes trivial, and the last two loops can go away.

The code duplication within the main loop can be reduced by using a pointer to a pointer (which I call **min), referring to whichever list a head with the smaller value.  The if (root == NULL) special case can be removed by initializing a pointer to a dummy head node.

static struct node* mergeList(struct node *list1, struct node *list2)
{
    struct node dummy_head = { 0, NULL }, *tail = &dummy_head;

    while ( (list1 != NULL) && (list2 != NULL) )
    {
        struct node **min = (list1->element < list2->element) ? &list1 : &list2;
        struct node *next = (*min)->next;
        tail = tail->next = *min;
        *min = next;
    }
    tail->next = list1 ? list1 : list2;
    return dummy_head.next;
}

mergeSort()

It would be more natural to accept a struct node * and return a struct node *, I think.

You left in some printf() calls for debugging.  Why not take advantage of the existing printList() function for that?

Most of the improvements are just natural consequences of the changes suggested above.

struct node *mergeSort(struct node *head)
{
    struct node *list1 = head;
    if ( (list1 == NULL) || (list1->next == NULL) )
    {
        return list1;
    }

    struct node *list2 = bisectList(list1);

    printf("\nList1 = "); printList(list1);
    printf("\nList2 = "); printList(list2);

    return mergeList( mergeSort(list1), mergeSort(list2) );
}

Conclusion

Your understanding of the mergesort algorithm is generally sound.  However, there were some massive memory leaks, and the implementation was verbose.  Try to look for opportunities to design more natural function interfaces and reduce special cases.


Greedy adaptive dictionary (GAD) for supervised machine learning For my project in machine learning supervised, I have to simplify a training-data and I have to use this technique at page 5 of the document.

Pseudocode algorithm

My code (numbers are the steps):

import numpy as np
import math
def GAD(X):
    # 1. Initialize
    l=0
    D=[]
    I=[]
    R = np.copy(X)

    # 2. repeat l=<N (look algorithm)
    while l<X.shape[0]:
        # 3. Find residual column (l1 to l2 norm ratio)
        min_eps = float('inf') # to search min initialize to infinite
        j_min   =   -1
        for j in range(R.shape[1]):
            norma1 = norma2 = 0
            for i in range(R.shape[0]):
                norma1 += abs(R[i][j])
                norma2 += (R[i][j])**2
            norma2 = math.sqrt(norma2)
            eps = norma1/norma2   # sparsity index
            if min_eps > eps and j not in I: #excludes if already inserted
                min_eps=eps
                j_min = j
        # 4. Set the l-th atom equal to normalized
        norma2 = np.sqrt(np.sum(R[:, j_min]**2, axis=0))
        atomo = R[:, j_min]/norma2
        # 5. Add to the dictionary
        if len(D) == 0:
            D = np.asarray(atomo)
        else:
            D = np.vstack((D, atomo.T))
        I.append(j_min)
        # 6. Compute the new residual
        for j in range(R.shape[1]):
            R[:, j] = R[:, j]-atomo*(atomo.T*R[:, j])
        l = l+1
    # 7. Termination (read page 6 of the document)
    return D,I

I have some doubts:

Is it a correct implementation (observe mostly steps 5 and 6 in the pseudocode algorithm)?

I haven't read the paper so don't necessarily understand the algorithm, but this seems wrong to me:

    # 3. Find residual column (l1 to l2 norm ratio)
    min_eps = float('inf') # to search min initialize to infinite
    j_min   =   -1
    for j in range(X.shape[1]):
        norma1 = norma2 = 0
        for i in range(X.shape[0]):
            norma1 += abs(X[i][j])
            norma2 += (X[i][j])**2
        norma2 = math.sqrt(norma2)
        eps = norma1/norma2   # sparsity index
        if min_eps > eps and j not in I: #excludes if already inserted
            min_eps=eps
            j_min = j

The pseudocode says to find the column of \${\mathbf R}^l\$ with minimum ratio, but this code finds the column of \$\mathbf X\$ with minimum ratio.

Have you tested your code and made sure it returns the correct result?


Project Euler Problem 35: counting circular primes below 1 million Project Euler Problem 35 asks:

  The number, 197, is called a circular prime because all rotations of the digits: 197, 971, and 719, are themselves prime.

  How many circular primes are there below one million?

import itertools
import math
import time

prime_list = []
#num = 14
count = 0
limit = 100000
time.clock()

def get_primes(n):
    numbers = set(range(n, 1, -1))
    #primes = []
    while numbers:
        p = numbers.pop()
        prime_list.append(p)
        numbers.difference_update(set(range(p*2, n+1, p)))
    #return primes

get_primes(limit)

print "Done", time.clock()

for r in xrange(2,limit):
    #b = 1
    if r in prime_list:
        #print r,count
        num = str(r)
        if (all(int(num[i:]+num[:i]) in prime_list for i in xrange(len(num)))):
            count += 1
print count,time.clock()

I am generating all possible primes with another function and it is giving me output in 60-80 ms for limit=1000000

Still the loop is taking around 19 seconds for finding cicular prime count under limit=100000

Regarding the primes above 10. If they're prime and all of their rotations must be primes, then each digit must be in the set of 1, 3, 7 and 9. So for all 2 digits numbers,\$ 4^2 \$, for all 3 digits, \$ 4^3 \$, until 6. That's exactly 5456 numbers to check, which is very few, compared to 1 million. Now we have all the numbers possible and only those.

Also, you play with numbers with a max value of 1 million. You also only need to check if a number is prime. To do that, you only need to generate primes until 1000 and check if one of the candidates is divisible by any of those prime numbers. There are 168 primes under 1000. So you can store them in an array. It's faster than always generating the primes again and again.

This should already improve your timing by a really large factor.


Comparison of values in dictionary I have the following dictionary:

results_dict = {'Current model': {'Recall': 0.77,
  'Precision': 0.1,
  'F1_score': 0.18,
  'Frauds': 94},
 'New model': {'Recall': 0.96,
  'Precision': 0.17,
  'F1_score': 0.29,
  'Frauds': 149}}

What I want is to make a comparison for each metric between the two models, and in case the one belonging to the new model is better, adding +1 to a variable i. So far, I have made this:

recall_new = results_dict['New model']['Recall']
recall_current = results_dict['Current model']['Recall']
precision_new = results_dict['New model']['Precision']
precision_current = results_dict['Current model']['Precision']
F1_new = results_dict['New model']['F1_score']
F1_current = results_dict['Current model']['F1_score']
frauds_new = results_dict['New model']['Frauds']
frauds_current = results_dict['Current model']['Frauds']

i = 0

if recall_new > recall_current or recall_new > (recall_current-(0.1)):
    i+=1
if precision_new > precision_current or precision_new > (precision_current-(0.1)):
    i+=1
if F1_new > F1_current or F1_new > (precision_current-(0.1)):
    i+=1
if frauds_new > frauds_current or frauds_new > int(round(frauds_current*1.2,0)):
    i+=1

The code makes what is intended to do but it is very verbose and repetitive and I was wondering whether it could be simplified or not. Thank you!

Index less

Also, you're indexing a lot. Perhaps it's better to seperate the two dicts:

old, new = results_dict["Current model"], results_dict["New model"]

(And what's up with having an underscore in Current_model and a space in New model? That's asking for typos. You're not even consistent with it - in your first code they're both spaces, but in the assignment you use 3 underscores and a space for Current...)

Also, with how you make the checks, one of your conditions always implies the other. You should remove the redundant comparisons. Change:

if recall_new > recall_current or recall_new > (recall_current-(0.1)):
# Into:
if recall_new > recall_current - 0.1:

The additional braces don't do anything, and if recall_new is bigger than current-0.1, then it is also bigger than current.

Loop

If you look closely, you'll see you're doing the same thing multiple times. So just make it a loop.

Arguably you should make an outside variable for the keys to iterate over, or iterate over the keys of either new or old dict. But if hard-coding is appropriate, it could look a lot like:

new_better = 0  # i is a bad variable name. Names should have meaning.
for key in ("Recall", "Precision", "F1_score"):
    if new[key] > old[key]-0.1:
        new_better += 1
if new["Frauds"] > old["Frauds"]*1.2:
    new_better += 1

Note that I removed your rounding. Python has no issues transforming a float to the closest integer by means of int(), but neither does it have a problem with comparing ints to floats. I did notice that your adjustments make it easier to score an increment for the first three variables, but harder for the fourth. Is this intentional?


Search in a JSON structure after a key This code is supposed to search for a specific key in a object or an array or a mix between both. Is there anything I can improve?

function getAllObjectsIn(key, data, casesensitive){

    var returns = [];
    var searchForInner = function (key, data, returnParameter, casesensitive){

        if(typeof(data) === 'object'){
            for (var i in Object.keys(data)){
                if(!(casesensitive)){
                    var searchTerm = Object.keys(data)[i].toLowerCase();
                    var key = key.toLowerCase();
                };
                if(searchTerm === key){
                    returns.push(data[Object.keys(data)[i]]);
                }
                else if (typeof(data[Object.keys(data)[i]]) === 'object'){
                    searchForInner(key,data[Object.keys(data)[i]],returnParameter);
                };
            };
        };
    };

    searchForInner(key,data,returns,casesensitive);
    return(returns);
};

Your function name is not very meaningful in that;

a) there is nothing to say values returned in the array will be objects (since you are just grabbing whatever data is stored for the key), and

b) you are not getting "all" data items at all: in fact you are performing a specific key search.

Perhaps something like recursiveKeySearch() or similar will makes more sense.
I also question why you would include case insensitive search behavior against keys in the first place; that is contrary to the expected behavior of key/property names in JavaScript: the potential need to do case-insensitive key/property names evaluations might suggest bad code in other areas of the application where key/property cases are not being applied consistently.

Don't put in hacks in your code which can make your code perform in unexpected manners.

If you remove that requirement, you have no need to inspect object.keys  anymore and can just directly check if the key exists, greatly simplifying your code.
Your code does not really do anything to validate that the data being passed to it is sensible to work with.

I am guessing that you are really wanting to do is require that data be either an Array specifically, or any other type of object in the Object prototype chain; you have some edge cases you are missing here like typeof(data) === 'object' returning true if data is null.

When those edge cases happen (which could be often if you are iterating over arbitrary objects) I would think you would still need to handle them within your code.
Do you truly wish for you function to be able to search against numeric keys (i.e. for case of Array object)?

This might seem like a little bit odd of an approach based on JavaScript's inability to actually have numeric keys on an object (they will be converted to strings); this really means that if an integer key is passed, you can only meaningfully search in Arrays, whereas if a string key is passed, you can only meaningfully search in Objects: I personally would expect that this function would not be meaningful for searching numeric Array keys, as typically these keys only indicate "order" not some meaningful value for use in lookup.

Thus you might consider handling Arrays differently - just iterating and recursing into them, but not actually evaluating them for matches.

If you need to deep search for numeric key matches, perhaps go with another function altogether.
The design of your recursion is a little odd.

Why the need for an inner function?
Your current code approach stops recursion for case where there is a key match; is this really desired?

I would think expected behavior would be to deep search all values in all keys regardless as to whether the key matched the criteria.

As it stands, you do not recurse into the value for any cases where you have a key match.
You have a few errors in logic around your case sensitivity handling;

In the case sensitive case, the searchTerm variable would have never been populated, thus your key comparison will always fail.

In the case-insensitive logic path, you overwrite your key value, potentially breaking your code, as you are now potentially recursing and going through subsequent iterations with a totally new key value.
This may not actually make a difference with code as written, but I would think you want key to be consistent throughout recursion.

Also, if you truly want to support integer key values, these make no sense with cases sensitivity, and you would in fact significantly alter the key you are searching for by casting it to string when performing toLowerCase() on it.
Per my earlier comment around need for case insensitive search, here is a good example for how you have introduced three potential bugs here just because of added complexity on supporting this use case, and not really implementing in best manner.
You would be better off setting Object.keys(data) to a variable, so you don't have to keep extracting the keys every time you want to act against them.

You can obviously avoid this altogether as you should probably simply just replace

This:

for (var i in Object.keys(data)){

with this:

for (var dataKey in data) {

as there is no reason you need to know the (actually meaningless and technically unpredictable) index number around an array created from the key of the data object.

Object properties in JavaScript do not have a guaranteed order, so it seems inappropriate to try to rely on index positions based on this behavior.

Just work with the property names themselves.

Putting it together

If you looked to refactor based on the considerations mentioned above, you might end up with something like:

function recursiveKeySearch(key, data) {
    // not shown - perhaps validate key as non-zero length string

    // Handle null edge case.
    if(data === null) {
        // nothing to do here
        return [];
    }

    // handle case of non-object, which will not be searched
    if(data !== Object(data)) {
        return [];
    }

    var results = [];

    // Handle array which we just traverse and recurse.
    if(data.constructor === Array) {
        for (var i = 0, len = data.length; i < len; i++) {
            results = results.concat(recursiveKeySearch(key, data[i]));
        }
        return results;
    }

    // We know we have an general object to work with now.
    // Now we need to iterate keys
    for (var dataKey in data) {
        if (key === dataKey) {
            // we found a match
            results.push(data[key]);
        }

        // now recurse into value at key
        results = results.concat(recursiveKeySearch(key, data[dataKey]));
    }

    return results;
}

Or, if you REALLY need to add a case insensitive search:

function recursiveKeySearch(key, data, caseSensitive = true) {
    // not shown - perhaps validate key as non-zero length string
    // not shown - perhaps validate caseSensitive as boolean

    // Handle null edge case.
    if(data === null) {
        // nothing to do here
        return [];
    }

    // handle case of non-object, which will not be searched
    if(data !== Object(data)) {
        return [];
    }

    var results = [];

    // determine 'key' value to be used for comparison
    var comparisonKey = key;
    if (caseSensitive === false) {
        comparisonKey = key.toLowerCase();
    }

    // Handle array which we just traverse and recurse.
    if(data.constructor === Array) {
        for (var i = 0, len = data.length; i < len; i++) {
            results = results.concat(recursiveKeySearch(key, data[i], caseSensitive));
        }
        return results;
    }

    // We know we have an general object to work with now.
    // Now we need to iterate keys
    for (var dataKey in data) {
        var isMatch = false;
        if(caseSensitive === false) {
            isMatch = (comparisonKey === dataKey.toLowerCase());
        } else {
            isMatch = (comparisonKey === dataKey);
        }

        if(isMatch) {
            results.push(data[dataKey]);
        }

        // now recurse into value at key
        results = results.concat(recursiveKeySearch(key, data[dataKey], caseSensitive));
    }

    return results;
}


Python program for Word Search II This is a Leetcode problem -

  Given a 2D board and a list of words from the dictionary, find all
  words in the board.

  Each word must be constructed from letters of a sequentially adjacent
  cell, where "adjacent" cells are those horizontally or vertically
  neighboring. The same letter cell may not be used more than once in a
  word.

  Note:

  All inputs are consist of lowercase letters a-z.
  The values of
  words are distinct.

Here is my solution to this challenge -

class Solution:
    def __init__(self, board, words):
        self.board = board
        self.words = words

    def find_words(self, board, words):

        root = {}
        for word in words:
            node = root
            for c in word:
                node = node.setdefault(c, {})
            node[None] = True
        board = {i + 1j * j: c
                 for i, row in enumerate(board)
                 for j, c in enumerate(row)}

        found = []
        def search(node, z, word):
            if node.pop(None, None):
                found.append(word)
            c = board.get(z)
            if c in node:
                board[z] = None
                for k in range(4):
                    search(node[c], z + 1j ** k, word + c)
                board[z] = c
        for z in board:
            search(root, z, '')

        return found

Program explanation - I first build a tree of words with root root and also represent the board a different way, namely as a one-dimensional dictionary where the keys are complex numbers representing the row/column indexes. That makes further work with it easier. Looping over all board positions is just for z in board, the four neighbors of a board position z are just z + 1j ** k (for k in 0 to 3), and I don't need to check borders because board.get just returns None if I request an invalid position.

After this preparation, I just take the tree and recursively dive with it into each board position. Similar to how you'd search a single word, but with the tree instead.

Here is an example input/output -

output = Solution([
  ['o','a','a','n'],
  ['e','t','a','e'],
  ['i','h','k','r'],
  ['i','f','l','v']
], ["oath","pea","eat","rain"])

print(output.find_words([
  ['o','a','a','n'],
  ['e','t','a','e'],
  ['i','h','k','r'],
  ['i','f','l','v']
], ["oath","pea","eat","rain"]))

>>> ['oath', 'eat']

So, I would like to know whether I could make this program shorter and more efficient.

Any help would be highly appreciated.

Awkward API

It seems strange to initialise a Solution object with various info that we provide again in the call of the method find_words.

Actually, looking for places where self is used makes it pretty obvious: the instance is never used.

It is a good occasion to watch the Stop Writing Classes talk from Jack Diederich.

To be continued ?


Duplicate Line Finder I recently wrote a program that reads a plain text file, and compares every line and alerts you if any are similar, based on a float value that you pass in to a function.

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.util.*;

public class DuplicateLineFinder {

    public static void main(String[] args) throws IOException {

        final float SIMILARITY_RATE = 0.75f;

        String fileLocation = System.getProperty("user.home") + "/Desktop/";
        String fileName = "lines.txt";
        BufferedReader file = new BufferedReader(new FileReader(fileLocation + fileName));

        List<String> lines = new ArrayList<String>();
        String line;
        while((line = file.readLine()) != null) {
            lines.add(line);
        }

        for (int i = 0; i < lines.size()-2; i++) {
            for (int j = i+1; j < lines.size()-1; j++) {

                if (IsSimilar(lines.get(i), lines.get(j), SIMILARITY_RATE))
                    System.out.println("WARNING!\n " + lines.get(i) + "\n and \n" + lines.get(j) + "\n might be the same");
            }
        }

        file.close();
    }

    public static boolean IsSimilar(String line1, String line2, float percentSimilar) {
        int duplicateWordCount = 0;
        List<String> words1 = new LinkedList<String>(Arrays.asList(line1.split(" ")));
        List<String> words2 = new LinkedList<String>(Arrays.asList(line2.split(" ")));

        if (words1.size() >= words2.size()) {
            for (String word : words2) {
                if (words1.remove(word)) {
                    duplicateWordCount++;
                }
            }

            if (duplicateWordCount > (words2.size() * percentSimilar)) return true;
        }

        else if (words2.size() > words1.size()) {
            for (String word : words1) {
                if (words2.remove(word)) {
                    duplicateWordCount++;
                }
            }

            if (duplicateWordCount > (words1.size() * percentSimilar)) return true;
        }

        return false;
    }

}

It works well, and is customisable in case you want to make it more/less strict. I am wondering if there is a better way to do my if statements in the IsSimilar function, as without them it doesn't work as well.

To test case, you can use toLowerCase() and toUpperCase() methods in String to get case. Also using String, you can use the split(' ') method to get each word and compare them instead of using two lists. It would definately increase performance to some extent.

To get the contents a bit better, the new nio package was updated in java 8 and has methods like Files.readAllLines(Path p);, returning a List (a list of lines) instead of using the BufferedReader trick. You can also use readAllBytes(Path p); which will return a byte array, but it is much easier to use readAlLines for your case.

Other than that I don't think there is much to give you. You have done a good job.


jQuery plugin creation and declaration for usage with $(). or $ Simple idea: just have a plugin (more a basic function). That I can use with jQuery in different ways it offers, means:

var myNewVar = $.myNewFunction( myVar );
var myNewVar = $().myNewFunction( myVar );

I want to know if what I did is the good way to doesn't repeat the code, or if their is a better way to.

Here my plugin:

// New function to jquery to change an array (from form) to an object
$.formArrayToObject = function( array ) {
    var form = {};
    $(array).each(function(){
        if( !form[this.name] ) {
            form[this.name] = this.value;
        }
        else {
            if ( !Array.isArray(form[this.name]) ) {
                form[this.name] = [form[this.name]];
            }
            form[this.name].push(this.value);
        }
    });
    return form;
};

// Second declaration to be able to call both $. and $().
jQuery.fn.formArrayToObject = function() {
    $.formArrayToObject();
};

Since your function - as far as I can tell - doesn't rely on a jQuery collection, I'd stick to always using "1 true way" of calling it.
I.e. I assume that

$("some > selector").formArrayToObject(arr) === $.formArrayToObject(arr);

that is, the two ways of calling it give the same exact result.

If that's the case, just use $.yourFunction and only that. Having two invocations that should be semantically different but actually aren't is just confusing.

For instance, jQuery doesn't have both $.ajax() and $(...).ajax() simply because the latter doesn't really make sense; the $.ajax function isn't dependent on a certain context or selector, so why would you call it as if it was?

(besides, your second declaration in the code above is broken; it ignores arguments)


Circular buffer offsets and sorted set of offsets sorted on their index Consider the following C++03 code:

#include <algorithm>
#include <deque>
#include <boost/iterator/counting_iterator.hpp>

const unsigned int _count = 0x10000;

struct recentFunctor {
  recentFunctor(const int base, const int firstOffset)
    : _base(base),
      _firstOffset(firstOffset) {
  }

  int getIndex(const int offset) const {
    return _base + _normalize(offset - _firstOffset);
  }

  bool operator()(const int offsetA, const int offsetB) const {
    return getIndex(offsetA) < getIndex(offsetB);
  }

private:
  int _normalize(const int offset) const {
    // Make negative offsets positive like math modulo.
    // Assumes _count is 0x10000.
    return int(static_cast<uint16_t>(offset));
  }

  const int _base;
  const int _firstOffset;

};

// Insert the inclusive range [beginOffset, lastOffset] into the recentItems
// ordered deque of offsets.  The offsets are into a circular array of 65,536
// items, so the range may wrap around.  The ordering of the offsets is based
// upon the index that corresponds to each offset.  For example, a beginOffset
// of 65,000 and a lastOffset of 100 means to insert the range of offsets
// [65000, 65535] into the deque at insertionPoint followed by the range of
// offsets [0, 100] and the beginOffset of 65,000 may correspond to an index of
// 65,000 but the lastOffset of 100 could correspond to an index of 65636 for
// the first time the circual buffer to which these offsets refer wraps around.

void
insertRecentRange(const int beginOffset,
                  const int lastOffset,
                  const int currentBase,
                  const int currentFirstOffset,
                  std::deque<int>::iterator insertionPoint,
                  std::deque<int> &recentItems)
{
  // The recentItems is an ordered set of offsets that have been noted.

  if (beginOffset <= lastOffset) {
    recentItems.insert(insertionPoint,
                       boost::counting_iterator<int>(beginOffset),
                       boost::counting_iterator<int>(lastOffset + 1));
  } else {
    // Fix this in C++11, for now insert() does not return what we want so we
    // must call lower_bound() to compute it:
    recentItems.insert(insertionPoint,
                       boost::counting_iterator<int>(beginOffset),
                       boost::counting_iterator<int>(int(_count)));

    std::deque<int>::iterator firstInsert
      = std::lower_bound(recentItems.begin(), recentItems.end(), beginOffset,
                         recentFunctor(currentBase, currentFirstOffset));

    recentItems.insert(firstInsert + (int(_count) - beginOffset),
                       boost::counting_iterator<int>(0),
                       boost::counting_iterator<int>(lastOffset + 1));
  }
}

While working in C++03, is there any way to perform the insert() and not have to call lower_bound() to compute a follow on insertion point for the remaining offsets?  Can this code be made easier to understand without changing the underlying data structures?  Any other suggestions to improve this?

One way is to supply uint16_t as the third template argument to boost::counting_iterator so that it allows wrapping around, allowing the code to be greatly simplified:

#include <algorithm>
#include <deque>
#include <boost/iterator/counting_iterator.hpp>

// Assumes _count == 65536 (0x10000)
typedef boost::counting_iterator<uint16_t,
                                 boost::use_default,
                                 uint16_t> uint16_wrapping_counting_iterator;

// Insert the inclusive range [beginOffset, lastOffset] into the recentItems
// ordered deque of offsets.  The offsets are into a circular array of 65,536
// items, so the range may wrap around.  The ordering of the offsets is based
// upon the index that corresponds to each offset.  For example, a beginOffset
// of 65,000 and a lastOffset of 100 means to insert the range of offsets
// [65000, 65535] into the deque at insertionPoint followed by the range of
// offsets [0, 100] and the beginOffset of 65,000 may correspond to an index of
// 65,000 but the lastOffset of 100 could correspond to an index of 65636 for
// the first time the circual buffer to which these offsets refer wraps around.

void
insertRecentRange(const int beginOffset,
                  const int lastOffset,
                  std::deque<int>::iterator insertionPoint,
                  std::deque<int> &recentItems)
{
  const uint16_t startOffset = uint16_t(beginOffset);
  const uint16_t endOffset   = uint16_t(lastOffset + 1);

  // The recentItems is an ordered set of offsets that have been noted.
  recentItems.insert(insertionPoint,
                     uint16_wrapping_counting_iterator(startOffset),
                     uint16_wrapping_counting_iterator(endOffset));
}


Feedback on automatic form filling with reflection Context:

I need to fill a handful of .docx templates with answers that come from form.responses, which is a dynamic object that comes from a JSON and can have different properties that vary according to the type of the form that was answered.

To do the filling, we need to instantiate a DTO called BindDto that takes the template string (eg.: ${PersonName}) and the value that will replace it (eg.: form.responses.client_name = "John Doe").

In my case, I created some [X]Constants classes that have properties that look like this...

public static readonly KeyValuePair<string, string> PERSON_NAME = new KeyValuePair<string, string>("client_name", "${PersonName}");
public static readonly KeyValuePair<string, string> PERSON_AGE = new KeyValuePair<string, string>("my_age", "${PersonAge}");
public static readonly KeyValuePair<string, string> MOTHER_NAME = new KeyValuePair<string, string>("mother", "${MotherName}");

...to map which template string each field from the JSON correponds to.

The code below was written so I could fill all the templates independent from which fields they might have by searching the [X]Constants classes and using the KeyValuePairs that I have set.

It works (o' course), but it seems I little... clumsy. And I don't have enough knowledge about C# to be sure this isn't pure madness.

What do you think about this code? Is it valid? Is is overengineering? How can I improve it?

I don't think performance will ever be a concern, but I would like to be enlightened in that sense too.

Thanks in advance to anyone willing to discuss it!

public List<BindDto> GenerateBinds(IEnumerable<Form> forms)
{
    var constants = new List<FieldInfo>();
    var binds = new List<BindDto>();

    constants.AddRange(typeof(AuthConsts).GetFields());
    constants.AddRange(typeof(ClientConsts).GetFields());
    constants.AddRange(typeof(ContactConsts).GetFields());
    constants.AddRange(typeof(AddressConsts).GetFields());
    constants.AddRange(typeof(ReferenceConsts).GetFields());

    foreach (var form in forms)
    {
        var formResponses = form.response.ToObject<Dictionary<string, string>>();

        IEnumerable<string> formFields = formResponses.Keys;

        foreach (var field in formFields)
        {
            foreach (var constant in constants)
            {
                if (constant.FieldType == typeof(KeyValuePair<string, string>))
                {
                    var templateString = (KeyValuePair<string, string>)constant.GetValue(this);

                    if (templateString.Key == field)
                        binds.Add(new BindDto(templateString.Value, formResponses[field]));
                }
            }
        }
    }

    return binds;
}

Just to tell you upfront, your code looks good, is easy to read and everything is well named. But it can be improved nevertheless.

First thing, public methods should always validate its method arguments. Passing null would lead to a nasty NullReferenceException at the start of the most outer foreach loop.

I would suggest making the constants a class level variable being filled in the constructor, because the types won't usually change while your code is running.

By restricting FieldType of the constants to KeyValuePair<string, string> will eliminate the if inside the most inner loop and will limit the constants to a smaller number.

Omitting braces {} although they may be optional can be dangerous because it may lead to hidden and therfor hard to find bugs. I would like to encourage you to use them.

Implementing the mentioned points will look like this

private readonly List<FieldInfo> constants = new List<FieldInfo>();
public ctor()
{
    constants.AddRange(typeof(AuthConsts).GetFields());
    constants.AddRange(typeof(ClientConsts).GetFields());
    constants.AddRange(typeof(ContactConsts).GetFields());
    constants.AddRange(typeof(AddressConsts).GetFields());
    constants.AddRange(typeof(ReferenceConsts).GetFields());
}

public List<BindDto> GenerateBinds(IEnumerable<Form> forms)
{
    if (forms == null) { throw new ArgumentNullException(nameof(forms)); }

    var neededType = typeof(KeyValuePair<string, string>);
    var binds = new List<BindDto>();
    var keyValueConstants = constants.Where(c => c.FieldType == neededType);

    foreach (var form in forms)
    {
        var formResponses = form.response.ToObject<Dictionary<string, string>>();

        IEnumerable<string> formFields = formResponses.Keys;

        foreach (var field in formFields)
        {
            foreach (var constant in keyValueConstants)
            {
                var templateString = (KeyValuePair<string, string>)constant.GetValue(this);

                if (templateString.Key == field)
                {
                    binds.Add(new BindDto(templateString.Value, formResponses[field]));
                }
            }
        }
    }

    return binds;
}


Retrieving access token from SharedPreference Here is code that retrieves an access token from SharedPreference.

  public static AuthToken getFromPreference(SharedPreferences p) {
    try {
      String at = p.getString(ACCESS_TOKEN_KEY, "");
      String uid = p.getString(USER_ID_KEY, "");
      long t = p.getLong(EXPIRES_AT_KEY, -1);

      if (at.isEmpty() || uid.isEmpty() || t == -1)
        return null;

      return new AuthToken(at, uid, t);
    } catch (ClassCastException e) {
      Log.wtf(TAG, "Incorrect field type in shared preference.");
      return null;
    }
  }

Can I omit exception handling if I don't expect it to happen in normal workcycle? Or should I handle even this terrible situation to not let app crash?

ClassCastException is not a checked exception, and so it's not a normal practice to catch it. You can omit catching this exception. However, you should make sure your application will never store values in shared preferences that would raise such errors.


Minimize sum of array by applying specific operation on x carefully chosen elements This code works, and solves the problem, but doesn't satisfy some time requirements, especially when the 'nums' array can have a length of over 1000. How can this be optimized?
Just reference:
This function does x number of operations on any element in the nums array so that the sum of all elements in the final array is always as small as possible. As a result, you have to perform the operation on the elements with the highest values first.
function minSum(nums, x) {
    if (nums.length === 0) {
        return false;
    }
    function operation(max) {
        let redcuedMax = Math.floor(max / 10);
        return redcuedMax
    }
    let ops = x;
    while (ops > 0) {
        let max = Math.max(...nums);
        const ofNumber = (element) => element >= max ;
        let maxIndex = nums.findIndex(ofNumber)
        let operated = operation(max);
        nums[maxIndex] = operated;
        ops--
    }
    return nums.reduce((prev,next) => prev + next, 0)
}

minSum([400,209,77], 4) //out: 31
minSum([5000,5000,5000,5000,5000,], 4) //out: 7000

You can simplify the code (and maybe accelarate it) by not assigning variables for values that are used once.
spreading the nums array is slow (Math.max), see
also. Math.max with spreading or the faster alternative (Math.max.apply) for a larger array (somewhere between 120.000 - 130.000 elements) will throw a Range Error, so use a loop for it.
reduce is (a lot) slower than a regular loop, so use a
loop to determine the sum
Math.floor can be replaced by a bitwise operator
(it's slightly faster)
Side note: alway use semicolons

The first snippet shows the aforementioned optimizations

const testLongerArray = [...Array(2000)].map((v, i) => i && i*10 || 1);
const testVeryLongArray = [...Array(200000)].map((v, i) => i && i*10 || 1);
console.log(minSum(testLongerArray, 600)); // out 10812701
console.log(minSum(testVeryLongArray, 1600)); // out 197130527201
console.log(minSum([209,400,77], 4)); //out: 31
console.log(minSum([5000,5000,5000,5000,5000,], 4)); //out: 7000

function maxValue(arr) {
  let max = 0;
  let i = arr.length;
  while (i--) {
    max = arr[i] > max && arr[i] || max;
  }
  return max;
};

function minSum(nums, x) {
    if (nums.length === 0) {
        return false;
    }
    const start = performance.now();

    while (x--) {
        // Note: a loop for max is the fastest
        // and for very large arrays spreading and Math.max.apply
        // will throw, so using the loop here
        const max = maxValue(nums);
        const maxIndex = nums.indexOf(max);
        //               ^ use indexOf
        nums[maxIndex] = (max / 10) | 0;
        //               ^ Math.floor replacement
    }

    // replace reduce with a loop
    let len = nums.length;
    let sum = 0;

    while (len--) {
      sum += nums[len];
    }

    return `sum: ${sum}, time: ${(performance.now() - start).toFixed(2)} ms`;
}

Now you don't have to iterate through the whole array. If you sort the input array descending, you can take a subset of the first [x length] elements of the sorted input array and perform the operation on the elements of that subset. Adding up the subset elements and the elements of the original sorted array minus its first x elements should give the desired result.
That's worked out in the second snippet. In both snippets the performance is timed, so you can compare both snippets (especially for the longer arrays).

const testLongerArray = [...Array(2000)].map((v, i) => i && i * 10 || 1);
const testVeryLongArray = [...Array(200000)].map((v, i) => i && i * 10 || 1);
console.log(minSum(testLongerArray, 600)); // out 10812701
console.log(minSum(testVeryLongArray, 1600)); // out 197130527201
console.log(minSum([209, 400, 77], 4)); //out: 31
console.log(minSum([5000, 5000, 5000, 5000, 5000, ], 4)); //out: 7000

function minSum(nums, x) {
  if (nums.length === 0) {
    return false;
  }
  const start = performance.now();
  const operation = v => (v / 10) | 0;
  nums.sort((a, b) => b - a);
  let subset = nums.slice(0, x);
  let sum = 0;
  nums = nums.slice(x);

  while (x--) {
    const maxVal = maxValue(subset);
    subset[subset.indexOf(maxVal)] = operation(maxVal);
  }
  nums = nums.concat(subset);
  x = nums.length;

  while (x--) {
    sum += nums[x];
  }

  return `sum: ${sum}, time: ${(performance.now() - start).toFixed(2)} ms`;
}

function maxValue(arr) {
  let max = 0;
  let i = arr.length;
  while (i--) {
    max = arr[i] > max && arr[i] || max;
  }
  return max;
}


Bubble sort examination I solved bubble sort in an exam, but the examiner wasn't satisfied with the solution. Please tell me why my solution is unsatisfactory.

function bubbleSort(myArray){
  var temp = undefined,isSwap = false;

  for(var x = 0; x < myArray.length; x++){
   if(((x+1) <= (myArray.length - 1)) && myArray[x] > myArray[x+1]){
     temp = myArray[x];
     myArray[x] = myArray[x+1];
     myArray[x+1] = temp;
     isSwap = true;
   }else if(isSwap === true && x === (myArray.length - 1)){
     x = -1;
     isSwap = false;
   }else{
     continue;
   }
 }
 return myArray;
}

Just to see the performance I added a print statement i.e. console.log
in the start of the loop like below

for(var x = 0; x < myArray.length; x++){
    console.log(myArray);

This is what I am able to see for the specific input:

console.log(bubbleSort([5,1,6,2,0]));

Output:

rahul@rahul:~/myPractise/Algo$ node sorting.js
[ 5, 1, 6, 2, 0 ]
[ 1, 5, 6, 2, 0 ]
[ 1, 5, 6, 2, 0 ]
[ 1, 5, 2, 6, 0 ]
[ 1, 5, 2, 0, 6 ]
[ 1, 5, 2, 0, 6 ]
[ 1, 5, 2, 0, 6 ]
[ 1, 2, 5, 0, 6 ]
[ 1, 2, 0, 5, 6 ]
[ 1, 2, 0, 5, 6 ]
[ 1, 2, 0, 5, 6 ]
[ 1, 2, 0, 5, 6 ]
[ 1, 0, 2, 5, 6 ]
[ 1, 0, 2, 5, 6 ]
[ 1, 0, 2, 5, 6 ]
[ 1, 0, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]

As @Zeta said, only your examiner can tell you why he wasn't satisfied.
But here are a couple of suggestions anyway:

Variable names are important and myArray doesn't mean much. You can try inputArray for example.
Changing the index of a for loop is usually not a good idea, because it makes the code a bit more difficult to understand (not by a lot, but why make it harder?).

This is another go at the same algorithm, which I think is more readable:

function bubbleSort(inputArray)
{
  var index;
  var done = false;
  var temp;

  while ( !done )
  {
    done = true;
    for(var index = 0; index < inputArray.length; index++)
    {
      if ( inputArray[index] > inputArray[index+1] )
      {
        temp = inputArray[ index ];
        inputArray[ index ] = inputArray[ index+1 ];
        inputArray[ index+1 ] = temp;
        done = false;
      }
    }
  }

  return inputArray;
}


Palindrome-inize a number For example if I start with the number 146. I first reverse it, so it becomes 641. Then I add this to the original number to make 787 which is a palindrome. I would repeat the process if the answer is not a palindrome.

I made this program show how long it ran for when it finishes or is interrupted and also to write the output to a file called output.txt.

from sys import exit
from signal import SIGINT, signal
from timeit import default_timer

def n(x):

    counter = 0
    b = 0
    start = default_timer()

    def t(*args):

        end = default_timer()
        total = end - start
        print (total)

        with open('output.txt','w') as f:

            f.write('{}\n{}\n{}'.format(b, counter, total))

        exit(0)

    signal(SIGINT,t)

    while True:

        b += int(str(x)[::-1])

        if b == int(str(b)[::-1]):

            end = default_timer()
            print (b, counter)
            t()
            break

        else:

            counter += 1
            print (b,counter)
            x = b

A few notes:

A more pythonic way to determine if a given value is a palindrome:

str(n) == str(n)[::-1]

You do something similar in your code, but are converting from a string back to an int.  This is more readable to me.

However, this can slow down the code if we do this sort of casting a lot, so it would be better to abstract this functionality to a loop to reduce the number of casts and further increase readablity:

def is_palindrome(num):
    string = str(num)
    return string == string[::-1]

Use the palindrome test as a check with the while loop
In the while loop, use the logic you already have to add on a reversed int:

n += int(str(n)[::-1])

I would make it easier to input a number for the code to use, I did this with argparse.

def get_args():
    parser = argparse.ArgumentParser(description=
        'Generate palindrome from number when added to its reverse')
    parser.add_argument('num', type=int, help='number for palindrome generator')
    return parser.parse_args()

Right now you are writing just a snippet of data to a file, but it is being overwritten every time.  I'd recommend just outputting to stdout with this current method, or changing it so you append to a file instead of overwrite it.  I've gone with the former recommendation in my final code.
For profiling and timing code, it's recommended you use a Python profiler instead of writing code yourself.

Final Code

import argparse

def get_args():
    parser = argparse.ArgumentParser(description=
        'Generate palindrome from number when added to its reverse')
    parser.add_argument('num', type=int, help='number for palindrome generator')
    return parser.parse_args()

def is_palindrome(num):
    string = str(num)
    return string == string[::-1]

def main():
    args = get_args()
    while not is_palindrome(args.num):
        args.num += int(str(args.num)[::-1])

    return args.num

if __name__ == "__main__":
    print(main())

Test run:

$ python test.py 146
787


Dealing with non-required user input I'm working on a page where a user can create new records, but not all values are required. When creating the SqlCommand with potentially null values, I'm wondering what's the best way, from a code readability standpoint, to check if there's any valid/useful information in a given field. If there is, add that information to the command, if there's isn't, add a DBNull.

Let's say we have a textbox, Address, which is non-compulsory (comm is the SqlCommand):

if(String.IsNullOrEmpty(Address.Text))
{
    comm.Parameters.AddWithValue("@Address", (Object) DBNull.Value)
}
else
{
    comm.Parameters.AddWithValue("@Address", (Object) Address.Text);
}

That seems like a lot of unnecessary typing. But trying to get clever and condense it into a single statement:

comm.Parameters.AddWithValue("@Address",
     String.IsNullOrWhiteSpace(Address.Text) ?
     (Object) DBNull.Value :
     (Object) Address.Text
     );

That seems a little too condensed, harder to parse and understand. Not too difficult to figure out, but (at least to me), also not immediately apparent what's going on. So a trade-off:

Object addressParam =
    String.IsNullOrEmpty(Address.Text) ?
    (Object) DBNull.Value :
    (Object) Address.Text;

 comm.Parameters.AddWithValue("@Address", addressParam);

(Or the above in a if/else block).
Another option that just occurred as I wrote this question:

Object addressParam = (Object) DBNull.Value;
if(!String.IsNullOrWhiteSpace(Address.Text))
{
     addressParam = (Object) Address.Text;
}

Are any of these methods prefered? I am going about this completely the wrong way? Am I just way over-thinking this? It's the latter, isn't it?

I personally prefer your 3rd example

Object addressParam =
    String.IsNullOrEmpty(Address.Text) ?
    (Object) DBNull.Value :
    (Object) Address.Text;

 comm.Parameters.AddWithValue("@Address", addressParam);

It easily reads that you are evaluating and initializing your Object and directly after you're using your newly instantiated Object. It also doesn't take up as much space as your if-statements and it doesn't really slow you down when reading the code -> you see an Object being initialized and then used, and when needed you can always take an extra second to see what you're evaluating.


Psycho and ordinary numbers The problem statement can be found here. In short, here's what the problem is about:

  You're given a set of numbers and you need to find whether the the numbers are ordinary or psycho. For a number to be psycho, its number of prime factors that occur even times should be greater than the number of prime factors that occur odd number of times. Else, it's ordinary.

My solution for this is as follows:

First, I initialize the Sieve of Eratosthenes. This is the fastest method I know to get a list of prime numbers.
Next, I loop over all the test cases and loop over all it's factors that are prime to increment the even and odd counter, to finally compare them and find the answer. For this I have to loop from 0 to half of the number.

This algorithm of mine is \$O(n)\$ for one input. Since the input size is of the order of \$10^7\$ and the number of inputs of the order of \$10^6\$ my algorithm takes time of the order of \$10^{13}\$. I need help with reducing this time.

#include <cstdio>
using namespace std;

bool primes[5000000];

void erastho()
{
   for (int i = 0; i < 5000000; i++)
   {
      primes[i] = 1;
   }
   primes[0] = 0;
   primes[1] = 0;
   for (int i = 2; i < 3164; i = i + 1)
   {
      if (primes[i])
      {
         int p = i*2;
         while(p < 5000000)
         {
            primes[p] = 0;
            p = p + i;
         }
      }
      else continue;
   }
}

int main()
{
   erastho();
   int t;
   scanf("%d", &t);
   while(t--)
   {
      int n;
      scanf("%d", &n);
      int hal = n/2;
      int v, ev = 0, od = 0;
      for (int i = 2; i <= hal; i++)
      {
         if((primes[i]) && (n%i == 0))
         {
            while (n%i == 0)
            {
               n = n/i;
               v++;
            }
            if (v % 2 == 0) ev++;
            else od++;
            v = 0;
         }
      }
      if (ev > od)
      {
         printf("%s \n","Psycho Number");
      }
      else
      {
         printf("%s \n", "Ordinary Number");
      }
   }
}

Do not use namespace std:
There is multiple reviews on that here on this site already, so I'll just sum it up shortly:

using namespace std pollutes global namespace and leads to possible conflicts with other functions. For more information you can read this stackoverflow post on the matter.

Naming:
Simply because of your variable names I found it hard to understand what exactly you were doing. Characters are not costly. Anymore that is, but you should really write out what your variables are supposed to do:
od --> oddFactorOccurence
ev --> evenFactorOccurence
v --> currentFactorCounter
n --> number
t --> countOfNumbers
hal --> half / evalThreshold
erastho... no. just. NO. ;)


Finding clusters in a matrix I got asked at an interview to write a program that, given a NxM matrix with zeros and ones, prints out the list of clusters of 1s. The clusters are defined as patches of 1s connected horizontally, vertically or diagonally.

Here's the code I submitted but I'm wondering if there is a better way, e.g. a well-known algorithm to produce the solution.

namespace ConsoleApplication1
{
    /// <summary>
    /// Usage:
    /// <![CDATA[
    ///
    ///            var m = 4; // rows
    ///            var n = 5; // cols
    ///            var matrix = new[]
    ///            {
    ///                0,1,0,0,0,  // 0-4
    ///                0,0,1,1,0,  // 5-9
    ///                0,0,0,0,0,  // 10-14
    ///                0,1,1,0,0   // 15-19
    ///            };
    ///
    ///            var finder = new MatrixClusterFinder(matrix, m, n);
    ///            finder.PrintClusters();
    /// PRINTS:
    /// (0,1)(1,2)(1,3)
    /// (3,1)(3,2)
    ///
    /// ]]>
    ///
    ///
    ///
    /// </summary>
    public class MatrixClusterFinder
    {
        private int m;
        private int n;
        private int[] matrix;

        public MatrixClusterFinder(int[] matrix, int m, int n)
        {
            this.m = m;
            this.n = n;
            this.matrix = matrix;
        }

        public void PrintClusters()
        {
            var visited = new bool[m * n];

            for (var i = 0; i < m * n; i++)
            {
                var isClusterFound = Traverse(i, visited);

                if (isClusterFound)
                {
                    Console.WriteLine(); //crlf
                }
            }

        }

        private bool Traverse(int curr, bool[] visited)
        {
            if (visited[curr])
            {
                return false;
            }

            bool result = false;
            visited[curr] = true;
            if (matrix[curr] == 1)
            {
                result = true;
                var thisM = curr / n;
                var thisN = curr % n;
                Console.Write("({0},{1})", thisM, thisN);

                var adjs = GetAdjacent(curr);
                foreach (var adj in adjs)
                {
                    Traverse(adj, visited);
                }
            }
            return result;
        }

        private int[] GetAdjacent(int curr)
        {
            var thisM = curr / n;
            var thisN = curr % n;
            var result = new List<int>();
            for (var i = thisM; i <= thisM + 1; i++)
            {
                if (i < 0 || i > m - 1)
                {
                    continue;
                }
                for (var j = thisN - 1; j <= thisN + 1; j++)
                {
                    if (j < 0 || j > n - 1 || (i == thisM && j < thisN))
                    {
                        continue;
                    }
                    var res = i * n + j;
                    if (res == curr)
                    {
                        continue;
                    }
                    result.Add(res);
                }
            }
            return result.ToArray();
        }

    }
}

For a problem like this, the interviewer is not actually looking for the most efficient/well-known algorithm possible. They want you to demonstrate that you can solve simple problems, using OO principles (encapsulation, inheritance, polymorphism), C# language features (Generics, LINQ, TPL etc) as well as industry standard patterns and practices like TDD, SOLID, DRY and so on.

The solution you have written is good in that it solves the problem and correctly uses recursion (probably eliminating 90% of the competition). I had some spare time today, so I've written an alternative solution that demonstrates some of the things I just mentioned. It is by no means, "the answer", just one of many possible solutions.

class Program
{
    static void Main()
    {
        var input = new[,]
        {
            {0, 1, 0, 0, 0},
            {0, 0, 1, 1, 0},
            {0, 0, 0, 0, 0},
            {0, 1, 1, 0, 0}
        };

        var locator = new ClustersLocator<int>(1);
        var clusters = locator.LocateClusters(input);

        foreach (var cluster in clusters)
            Console.WriteLine(cluster);

        Console.ReadKey();
    }
}

Coord

A 2D coordinate (struct) used by Matrix and Cluster.

public struct Coord : IEquatable<Coord>
{
    public Coord(int n, int m) : this()
    {
        N = n;
        M = m;
    }

    public int N { get; private set; }
    public int M { get; private set; }

    #region ToString() and Equals()
}

Cluster

A list of Coords that is equal to another Cluster if it has the same Coords.

public class Cluster
{
    public Cluster(IEnumerable<Coord> coords)
    {
        if (coords == null)
            throw new ArgumentNullException("coords");

        Coords = coords;
    }

    public IEnumerable<Coord> Coords { get; private set; }

    // Override Equals() and GetHashCode() to compare Coords
    #region Equals() and ToString()
}

Matrix

A 2D matrix of generic values. Implements IEnumerable<Coord> to allow for easy enumeration of all coordinates. Contains helper methods for getting adjacent coordinates.

internal class Matrix<T> : IEnumerable<Coord>
{
    private readonly T[,] _values;

    public Matrix(T[,] values)
    {
        _values = values;
    }

    public Matrix(int rows, int columns)
    {
        _values = new T[rows, columns];
    }

    public T this[Coord coord]
    {
        get { return _values[coord.N, coord.M]; }
        set { _values[coord.N, coord.M] = value; }
    }

    public int Rows { get { return _values.GetLength(0); } }
    public int Columns { get { return _values.GetLength(1); } }

    public IEnumerator<Coord> GetEnumerator()
    {
        for (var n = 0; n < Rows; n++)
            for (var m = 0; m < Columns; m++)
                yield return new Coord(n, m);
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public IEnumerable<Coord> GetEqualAdjacentCoords(Coord coord)
    {
        return GetAdjacentCoords(coord)
            .Where(a => this[a].Equals(this[coord]));
    }

    public IEnumerable<Coord> GetAdjacentCoords(Coord coord)
    {
        var adjacent = new[]
        {
            new Coord(coord.N - 1, coord.M + 1),
            new Coord(coord.N - 1, coord.M),
            new Coord(coord.N - 1, coord.M - 1),
            new Coord(coord.N + 1, coord.M + 1),
            new Coord(coord.N + 1, coord.M),
            new Coord(coord.N + 1, coord.M - 1),
            new Coord(coord.N, coord.M + 1),
            new Coord(coord.N, coord.M - 1)
        };

        return adjacent.Where(InRange);
    }

    public bool InRange(Coord coord)
    {
        return coord.N >= 0 && coord.N < Rows
            && coord.M >= 0 && coord.M < Columns;
    }
}

ClusterLocator

The main algorithm. Also generic, and can locate clusters of any value.

public class ClustersLocator<T> : IClustersLocator<T>
{
    private readonly T _clusterValue;

    public ClustersLocator(T clusterValue)
    {
        _clusterValue = clusterValue;
    }

    public IEnumerable<Cluster> LocateClusters(T[,] matrix)
    {
        if (matrix == null)
            throw new ArgumentNullException("matrix");

        return LocateClusters(new Matrix<T>(matrix));
    }

    private IEnumerable<Cluster> LocateClusters(Matrix<T> matrix)
    {
        var clusters = new List<Cluster>();
        var visited = new Matrix<bool>(matrix.Rows, matrix.Columns);

        foreach (var coord in matrix.Where(c => !visited[c]))
        {
            visited[coord] = true;

            if (!matrix[coord].Equals(_clusterValue))
                continue;

            var cluster = new ClusterBuilder<T>(matrix, visited).Build(coord);
            clusters.Add(cluster);
        }
        return clusters;
    }
}

ClusterBuilder

The recursive part of the algorithm. Uses a Matrix and a single member of a cluster to locate the other members in the same cluster.

internal class ClusterBuilder<T>
{
    private readonly Matrix<T> _matrix;
    private readonly Matrix<bool> _visited;

    public ClusterBuilder(Matrix<T> matrix, Matrix<bool> visited)
    {
        _matrix = matrix;
        _visited = visited;
    }

    public Cluster Build(Coord seed)
    {
        var clusterCoords = new List<Coord> {seed};
        Build(clusterCoords, seed);

        return new Cluster(clusterCoords);
    }

    private void Build(List<Coord> cluster, Coord current)
    {
        _visited[current] = true;

        var nextCoords = _matrix.GetEqualAdjacentCoords(current)
            .Where(c => !_visited[c]);

        foreach (var next in nextCoords)
        {
            cluster.Add(next);
            Build(cluster, next);
        }
    }
}

And most importantly, tests for the public API!

[TestFixture]
public class ClusterLocatorTests
{
    [Test]
    public void Throws_if_matrix_is_null()
    {
        var sut = new ClustersLocator<int>(1);

        Assert.Throws<ArgumentNullException>(() => sut.LocateClusters(null));
    }

    [Test]
    public void Returns_empty_collection_if_matrix_is_empty()
    {
        var sut = new ClustersLocator<int>(1);

        var clusters = sut.LocateClusters(EmptyMatrix);

        Assert.That(clusters, Is.Empty);
    }

    [TestCaseSource("GetLocatesClustersTestCases")]
    public void Locates_clusters(int[,] matrix, IEnumerable<Cluster> expectedClusters)
    {
        var sut = new ClustersLocator<int>(1);

        var clusters = sut.LocateClusters(matrix);

        Assert.That(clusters, Is.EqualTo(expectedClusters));
    }

    private static IEnumerable<ITestCaseData> GetLocatesClustersTestCases()
    {
        var testCase1 = new TestCaseData(Matrix1, Clusters1);
        testCase1.SetName("Large matrix of mixed clusters");

        // Add more test cases as needed
        return new[] {testCase1};
    }

    #region Test data
}


Is it bad practice to have executable code in my channel state object? I have a CommunicationStateMachine object that's attached to my channels. It keeps track of who's turn it is to speak, how far along the conversation the server and client are, things like the client ID, all bytes read to date, the ByteBuffer etc.

My previous version of this server did some checks in the Selector thread (bad idea, I know), like checking if the message was complete, and checking if the client was a valid one.
To that end I had the methods that did those checks declared in the CommunicationStateMachine.class and the Selector thread called those methods on every OP_READ pass.

Now I'm removing all that work and putting the responsibility on the worker threads, and in order to keep things a bit cleaner I've decided to use the State design pattern. I've made a class called Processor.class which contains abstract methods.
Depending on what mode the server is in (reading/writing text or reading/writing hex commands) I've created classes that extend Processor.class and implement it's methods. The result of the JSON mode processor so far is this:

public class JsonMode extends Processor {

    @Override
    public void processBytes(CommunicationStateMachine commsState,
            DataDAO database) {

        boolean validHeaderFormat = commsState.isValidClientHeaderFormat();

        if (validHeaderFormat) {

            boolean messageComplete = commsState.checkJsonMessageIntegraty();
            log.debug("JsonProcessor: messageComplete: " + messageComplete);
            String deviceId = commsState.getDeviceId();
            FirmwareFile file = database
                    .checkScheduledFirmwareUpgrade(deviceId);
            if (file != null) {
                commsState.setScheduledUpdate(true);
                log.debug("This is a scheduled update for " + deviceId
                        + " for file: " + file);
            } else {
                commsState.setScheduledUpdate(false);
                log.info("Device "
                        + deviceId
                        + ": Not scheduled for an update, scheduledUpgrade returned null");
            }
        }
    }

    @Override
    public void prepareWriteBytes(CommunicationStateMachine commsState,
            DataDAO database) {
        // TODO Auto-generated method stub

    }

}

So my thinking here, is that the methods in the commsState object that are called here, such as commsState.isValidClientHeaderFormat() and commsState.checkJsonMessageIntegraty() should be removed from the CommunicationStateMachine object and placed in the JSONMode.class processor, meaning I will only call get and set methods from commsState and keep all processing outside that class.
This is the way to go right?  My thinking is otherwise I'll either start adding callable code for the other modes to the CommunicationStateMachine.class and it will get more unwieldy, or I'll have JSONMode code in the CommunicationStateMachine.class and other Modes codes outside, which would definitely be wrong.

Any other thoughts you might have on this process would be greatly appreciated too.

I agree that you should pull this functionality out of CommunicationStateMachine. Your state machine should only expose the methods necessarily to make changes in state and to query the current state. Your CommunicationStateMachine class should also be responsible for enforcing the rules of your state machine, i.e. ensuring that only valid state transitions happen. This logic is typically hidden inside the class and invoked when an attempt is made to change state.

In general, the approach you've suggested (the State design pattern) seems like a sensible choice based on your description of your requirements.

Minor note: you've got a small typo on checkJsonMessageIntegraty, which should be Integrity at the end. It may also be more intuitive if your class is called JsonProcessor to reflect its purpose.


Funny String Java Solution I'm a little rusty in Java, but I'm using it to get ready for interviews.  Here's the problem description from HackerRank:

  Problem Statement

  Suppose you have a string \$S\$ which has length \$N\$ and is indexed from \$0\$
  to \$N−1\$. String \$R\$ is the reverse of the string \$S\$. The string \$S\$ is funny
  if the condition \$|S_i−S_{i−1}|=|R_i−R_{i−1}|\$ is true for every \$i\$ from 1 to
  \$N−1\$.

  (Note: Given a string str, stri denotes the ascii value of the ith
  character (0-indexed) of str. |x| denotes the absolute value of an
  integer x)

  Input Format

  First line of the input will contain an integer T. T testcases follow.
  Each of the next T lines contains one string S.

  Constraints

  \$1 \le T \le 10\$
  \$2 \le\$ length of S \$\le 10000\$

  Output Format

  For each string, print Funny or Not Funny in separate lines.

This passing solution took me about 20 minutes, so that might be a bit long given the difficulty of the problem.  I'm open to critiques on my speed too.

import java.io.*;
import java.util.*;
import java.text.*;
import java.math.*;
import java.util.regex.*;

public class Solution {
    private static boolean isFunny (String s)
    {
        String rev = reverse(s);
        boolean stillEq = true;
        for (int i = 2; i < s.length() && stillEq; ++i)
        {
            int comp = (int)s.charAt(i) - (int)s.charAt(i-1);
            int comp2 = (int)rev.charAt(i) - (int)rev.charAt(i-1);
            stillEq = Math.abs(comp) == Math.abs(comp2);
        }

        if (stillEq)
            return true;
        else
            return false;
    }

    private static String reverse (String s)
    {
        if (s.length() > 0)
            return s.charAt(s.length()-1) + reverse(s.substring(0, s.length()-1));
        else
            return "";
    }

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        int tests = sc.nextInt();
        for (int i = 0; i < tests; ++i)
        {
            String out = isFunny(sc.next()) ? "Funny" : "Not Funny";
            System.out.println( out );
        }
    }

Bug

The main loop starts at i = 2. That is, s.charAt(1) - s.charAt(0) is never attended to. Suspicious, isn't it?
Naming

A name comp (and comp2) presumes that it carries some information about comparison. It obviously doesn't. diff and rdiff sound better.
Algorithm

Reversal of the string just wastes time. You may work the same string from both directions simultaneously:

for (int i = 1; i < s.length(); ++i) {
    diff = s.charAt(i) - s.charAt(i - 1);
    rdiff = s.charAt(s.length() - 1 - i) - s.charAt(s.length() - 1 - (i-1));
    ...

Returning the comparison result is an anti-pattern.

return stillEq;

achieves the same result as

if (stillEq)
    return true;
else
    return false;

in a much cleaner way.
If you insist on reversing a string, better come up with an iterative method. Java cannot eliminate tail recursion.


Measuring performance of code using StopWatch I have created a StopWatch class to measure the performance of any code. I use this StopWatch code in any of my multithreading projects as well.

public static class StopWatch {

    public static ConcurrentHashMap<Long, Long> histogram = new ConcurrentHashMap<Long, Long>();

    /**
     * Creates an instance of the timer and starts it running.
     */
    public static StopWatch getInstance() {
        return new StopWatch();
    }

    private long end = -1;
    private long interval = -1;
    private final long start;

    private StopWatch() {
        start = interval = currentTime();
    }

    /**
     * Returns in milliseconds the amount of time that has elapsed since the timer was created. If the
     * stop method has been invoked, then this returns instead the elapsed time between the creation of
     * the timer and the moment when stop was invoked.
     *
     * @return duration it took
     */
    public long getDuration() {
        long result = 0;

        final long startTime = start;
        final long endTime = isTimerRunning() ? currentTime() : end;

        result = nanoToMilliseconds(endTime - startTime);

        boolean done = false;
        while (!done) {
            Long oldValue = histogram.putIfAbsent(result, 1L);
            if (oldValue != null) {
                done = histogram.replace(result, oldValue, oldValue + 1);
            } else {
                done = true;
            }
        }

        return result;
    }

    /**
     * Returns in milliseconds the amount of time that has elapsed since the last invocation of this same method. If
     * this method has not previously been invoked, then it is the amount of time that has elapsed since the timer
     * was created. Note that once the stop method has been invoked this will just
     * return zero.
     *
     * @return interval period
     */
    public long getInterval() {
        long result = 0;

        final long startTime = interval;
        final long endTime;

        if (isTimerRunning()) {
            endTime = interval = currentTime();
        } else {
            endTime = end;
        }

        result = nanoToMilliseconds(endTime - startTime);

        return result;
    }

    /**
     * Stops the timer from advancing. This has an impact on the values returned by both the
     * getDuration and the getInterval methods.
     */
    public void stop() {
        if (isTimerRunning()) {
            end = currentTime();
        }
    }

    /**
     * What is the current time in nanoseconds?
     *
     * @return returns back the current time in nanoseconds
     */
    private long currentTime() {
        return System.nanoTime();
    }

    /**
     * This is used to check whether the timer is alive or not
     *
     * @return checks whether the timer is running or not
     */
    private boolean isTimerRunning() {
        return (end <= 0);
    }

    /**
     * This is used to convert NanoSeconds to Milliseconds
     *
     * @param nanoseconds
     * @return milliseconds value of nanoseconds
     */
    private long nanoToMilliseconds(final long nanoseconds) {
        return nanoseconds / 1000000L;
    }
}

I wanted to review this piece of code. Any suggestions or improvements I can make here?

Just a few little things:

A public static getInstance() method is commonly used in the singleton pattern. However, you always create a new StopWatch. At least you should name it to createNewInstance(), if you want to keep the method at all.
Many of your comments are unnecessary. For example the comment before isTimerRunning() doesn't help to understand the code. If you are generating documentation, they might be required, if not, omit such comments.
The result variable in getInterval() is not required. You can just return the result like this:

return nanoToMilliseconds(endTime - startTime);

This saves you two lines of code and makes the code clearer. The same applies to getDuration().
by using the ternary if operator, you can shorten getInterval() a bit more:

public long getInterval() {
    final long startTime = interval;
    final long endTime = isTimerRunning() ? currentTime() : end;
    interval = endTime;
    return nanoToMilliseconds(endTime - startTime);;
}

The assignment interval = endTime now also happens when the StopWatch has already been stopped but this isn't a problem.


Personal quiz with input validation The object of the code is to prompt the user with a quiz and collect the g's, y's, 0's, and 1's and if there is more than one then the user passes (else, they fail).  I am a beginner and would like to know if there is a more efficient direction for this code to go in.

from time import sleep
print(pic1)

def check(name):
    if name.lower() == "brandon":
        print("Welcome home master")
    else:
        print("Let's find some data")
        sleep(0.5)
        print("What is your favorite color?")
        color = input("fav_color>>> ")
        if color == "":
            print("No skipping!")
            print(check(input("Enter Name Here>>> ")))
        print("What is your dream car?")
        car = input("fav_car>>> ")
        if car == "":
            print("No skipping!")
            print(check(input("Enter Name Here>>> ")))
        print("What team are you on?")
        team = input("team_name>>> ")
        if team == "":
            print("No skipping!")
            print(check(input("Enter Name Here>>> ")))
        print("Who is your best friend?")
        crush = input("best_friend>>> ")
        if crush == "":
            print("No skipping!")
            print(check(input("Enter Name Here>>> ")))
        print("What color is your shirt?")
        shirt = input("shirt_color>>> ")
        if shirt == "":
            print("No skipping!")
            print(check(input("Enter Name Here>>> ")))
        print("You got clout?")
        clout = input("clout_level>>> ")
        if clout == "":
            print("No skipping!")
            print(check(input("Enter Name Here>>> ")))
        print("Do you think you passed the test?")
        passed = input("passed?>>> ")
        if passed == "":
            print("No skipping!")
            print(check(input("Enter Name Here>>> ")))
        print("Calculating...")
        sleep(2.5)

        def add():
            color_counted = color.count("g")
            car_counted = car.count("g")
            team_counted = team.count("g")
            crush_counted = crush.count("g")
            shirt_counted = shirt.count("g")
            clout_counted = clout.count("y")
            clout_counted2 = clout.count("1")
            clout_counted3 = clout.count("0")
            passed_counted = passed.count("y")
            total = color_counted + car_counted + team_counted + clout_counted3 + passed_counted
            total = crush_counted + shirt_counted + clout_counted + clout_counted2
            return total
        if add() > 1:
            print("passed")
        else:
            print("failed")
        print("Congrats on your result!")
        sleep(100)

check(input("Enter Name Here>>> "))

Welcome to CodeReview! This isn't great code, but that doesn't mean that this is a bad question. I think that you've come to the right place.

Recursion

    if color == "":
        print("No skipping!")
        print(check(input("Enter Name Here>>> ")))

This uses recursion when it shouldn't. In other words, check is calling itself needlessly, and if the user tries to "skip" enough times, you'll get a stack overflow. Instead of doing this, just loop until you have valid input.

Fake delays

    print("Calculating...")
    sleep(2.5)

Few things bother me more than when a user interface lies to me. This output suggests that it takes 2.5 seconds for a calculation to be done, but that simply isn't so. Don't lie to your users - just show the results.

sleep(100) is actively harmful - the user needs to Ctrl+C to kill the program; otherwise it sits there preventing the user from getting their shell terminal back. This should just be deleted.

Don't repeat yourself

There are many places in the program that are expressed in a repetitive manner, especially user input (the "no skipping" mechanism), the call to count, and the additions to get total. I suggest the following:

def input_without_skip(prompt: str) -> str:
    while True:
        result = input(f'{prompt}>>>')
        if result:
            return result
        print('No skipping!')

def is_master() -> bool:
    return input_without_skip('Enter Name Here') == 'Hercules'

def get_counts():
    for question, prompt, chars in (
        (     'What is your favorite color?',   'fav_color', 'g'),
        (          'What is your dream car?',     'fav_car', 'g'),
        (            'What team are you on?',   'team_name', 'g'),
        (         'Who is your best friend?', 'best_friend', 'g'),
        (        'What color is your shirt?', 'shirt_color', 'g'),
        (                   'You got clout?', 'clout_level', 'y10'),
        ('Do you think you passed the test?',     'passed?', 'y')
    ):
        print(question)
        answer = input_without_skip(prompt)
        yield sum(answer.count(c) for c in chars)

def main():
    if is_master():
        print('Welcome home, master.')
        return

    print("Let's find some data!")
    total = sum(get_counts())

    if total > 1:
        print("passed")
    else:
        print("failed")
    print("Congrats on your result!")

if __name__ == '__main__':
    main()