Checking to see if array can be sorted using only one operation I'm doing this HackerRank problem, which asks whether an array of up to 100000 integers can be sorted in ascending order by exactly one of the following:

swapping two elements
reversing a subarray
doing nothing (i.e. it is already sorted)

I've written this code and it passes all the tests, but it seems very convoluted. Is there any way I can streamline this code to make it more Pythonic?

n = int(raw_input())
ar = map(int, raw_input().strip().split())
ar.insert(0, -1)
ar.append(1000001)

ups = 0
downs = 0

for i in xrange(1, n+1):
    if ar[i] < ar[i-1] and ar[i] < ar[i+1]:
        downs += 1
        later_index = i
    elif ar[i] > ar[i-1] and ar[i] > ar[i+1]:
        ups += 1
        first_index = i

for i in xrange(n, 0, -1):
    if ar[i] > ar[i-1] and ar[i] > ar[i+1]:
        swap_i = i

if downs == ups == 2:
    ar[swap_i], ar[later_index] = ar[later_index], ar[swap_i]
    if all(ar[i] < ar[i+1] for i in xrange(1, n)):
        print 'yes'
        print 'swap %d %d' % (swap_i, later_index)
    else:
        print 'no'
elif downs == ups == 1:
    if first_index == 1 or later_index == n:
        ar[swap_i], ar[later_index] = ar[later_index], ar[swap_i]
        if all(ar[i] < ar[i+1] for i in xrange(1, n)):
            print 'yes'
            print 'swap %d %d' % (swap_i, later_index)
        else:
            print 'no'
    else:
        if first_index - later_index == -1:
            ar[first_index], ar[later_index] = ar[later_index], ar[first_index]
            if all(ar[i] < ar[i+1] for i in xrange(1, n)):
                print 'yes'
                print 'swap %d %d' % (first_index, later_index)
            else:
                print 'no'
        else:
            ar = ar[:first_index:] + ar[later_index:first_index-1:-1] + ar[later_index+1::]
            if all(ar[i] < ar[i+1] for i in xrange(1, n)):
                print 'yes'
                print 'reverse %d %d' % (first_index, later_index)
            else:
                print 'no'
elif ups ==1 and downs == 0:
    print 'yes'
    print 'reverse %d %d' % (first_index, later_index)
else:
    print 'no'

I feel there are far too many if-else blocks ruining the program structure.

You have to make some functions.

You can start off with swap, reverse and is_sorted.
These are simple one or two liners:

def swap(array, i, j):
    array[i], array[j] = array[j], array[i]
    return array

def reverse(array, i, j):
    return array[:i] + array[j:i-1:-1] + array[j+1:]

def is_sorted(array, length):
    return all(array[i] < array[i+1] for i in xrange(1, length))

This keeps things nice and DRY.
'Cause repeating your self is bad.

Next you can do with a get_indexs.
This will use the loops you have at the moment, to make the code simpler, you can make two comprehensions.
This can be to get ups and downs.
And from this you can get ups, downs, first_index, last_index and swap_i.

Since you currently create first_index in a for loop, it's not guaranteed to be created,
and so we'd have to create it before the loops too.
This can cause the following function:

def get_indexs(array, length):
    first_index = None
    later_index = None
    swap_i = None
    range_ = xrange(1, length + 1)
    up = [i for i in range_ if array[i - 1] < array[i] > array[i + 1]]
    down = [i for i in range_ if array[i - 1] > array[i] < array[i + 1]]

    if up:
        first_index = up[-1]
        swap_i = up[0]
    if down:
        later_index = down[-1]

    return (
        len(up),
        len(down),
        first_index,
        later_index,
        swap_i,
    )

Next you'd want to change your massive if-else block.
You do a check if the output of the function is sorted, and you manually print everything.
Instead if you make a function that returns the three strings that you need, and you use these to test the array,
then you can significantly reduce the complexity of your program.
Take:

def get_command(array, length):
    ups, downs, first_index, later_index, swap_i = get_indexs(array, length)
    if downs == ups == 2:
        return 'swap', swap_i, later_index
    elif downs == ups == 1:
        if first_index == 1 or later_index == length:
            return 'swap', swap_i, later_index
        else:
            command = 'swap' if first_index - later_index == -1 else 'reverse'
            return command, first_index, later_index
    elif ups == 1 and downs == 0:
        return 'reverse', first_index, later_index
    return False

The final function that you'd want to make is a main.
You'd want to move all your global declarations into it.
Such as n = int(raw_input()).
You'd then want to move the little section that you copied and pasted on all your previous if-else's.
To be able to easily use the functions that you'd made above you'd want to add a single variable to the global scope too, it's just a simple dictionary that will hold the functions.
This could result in the following simple function:

COMMANDS = {
    'swap': swap,
    'reverse': reverse
}

def main():
    n = int(raw_input())
    ar = map(int, raw_input().strip().split())
    ar.insert(0, -1)
    ar.append(1000001)

    command = get_command(ar, n)
    if command:
        c, i, j = command
        ar = COMMANDS[c](ar, i, j)
        if is_sorted(ar, n):
            print 'yes'
            print '{} {} {}'.format(*command)
            return None
    print 'no'

Your algorithm may be easy to improve, but I think that you have to improve the base of you program.
As you'll end up coping and pasting lots of code all over the place, as you already have.
This code may not be the easiest to read, as optimized code normally isn't,
and may lead to a horrible program, rather than quite a nice one.

Also you say:

  doing nothing (i.e. it is already sorted)

But it prints 'no', if I enter a sorted list.

5
1 2 3 4 5
no

Should that not just be a 'yes' rather than a 'no'?


Reading set of large log files line by line and count how many hostnames appear on each line I have around 300 log files in a directory and each log file contains around 3300000 lines. I need to read through each file line by line and count how many hostnames that appear on each line. I wrote basic code for that task, but it takes more than 1 hour to run and takes lots of memory as well. How can I improve this code to make it run faster?

import pandas as pd
import gzip

directory=os.fsdecode("/home/scratch/mdsadmin/publisher_report/2018-07-25")#folder with 300 log files
listi=os.listdir(directory)#converting the logfiles into a list

for file in listi:#taking eaching log file in the list
    tt=os.path.join(directory,file)# joining log file name along with the directory path
    with gzip.open(tt,'rt') as f: #unzipping the log file
        rows=[]#clearing the list after every loop
        for line in f: #reading each line in the file
            s=len(line.split('|'))
            a=line.split('|')[s-3]
            b=a.split('/')[0] #slicing just the hostname out of each line in the log file
            if len(b.split('.'))==None:
                ''
            else:
                b=b.split('.')[0]
            rows.append(b) # appending it to a list

    df_temp= pd.DataFrame(columns=['hostname'],data=rows) #append list to the dataframe after every file is read
    df_final=df_final.append(df_temp,ignore_index=True) #appending above dataframe to a new one to avoid overwriting
    del df_temp #deleting temp dataframe to clear memory
df_final=df_final.groupby(["hostname"]).size().reset_index(name="Topic_Count") #doing the count

Sample log lines

tx:2018-05-05T20:44:37:626 BST|rx:2018-05-05T20:44:37:626 BST|dt:0|**wokpa22**.sx.sx.com/16604/#001b0001|244/5664|2344|455
tx:2018-05-05T20:44:37:626 BST|rx:2018-05-05T20:44:37:626 BST|dt:0|**wokdd333**.sc.sc.com/16604/#001b0001|7632663/2344|342344|23244

Desired output

So I think you can improve the efficiency of your code like this.

First, as I said in one comment, you can replace:

s=len(line.split('|'))
a=line.split('|')[s-3]

by

a=line.split('|')[-3]

as no need to know the total length of a list to get the third element from the end.

Second, assigning a then b with a value take some times, you can do it in one line:

a=line.split('|')[-3]
b=a.split('/')[0]

becomes

b=line.split('|')[-3].split('/')[0]

Third, I'm not sure len can equal to None, maybe you wanted to check for 0, but if our code run like this, I would say that:

if len(b.split('.'))==None:
    ''
else:
    b=b.split('.')[0]

is not useful so you can calculate directly the final b with:

b=line.split('|')[-3].split('/')[0].split('.')[0]

Forth, because actually you don't need to assign b anymore, you can append the value into rows directly, such as:

rows=[]
for line in f:
    rows.append(line.split('|')[-3].split('/')[0].split('.')[0])

or as a list comprehension:

rows = [line.split('|')[-3].split('/')[0].split('.')[0] for line in f]

Fifth, again, you create df_temp to use it once and then delete it, you can append directly into df_final such as:

df_temp= pd.DataFrame(columns=['hostname'],data=rows)
df_final=df_final.append(df_temp,ignore_index=True)
del df_temp

is better this way:

df_final=df_final.append(pd.DataFrame(columns=['hostname'],data=rows),
                         ignore_index=True)

Ultimately, rows is not necessary anymore, so all the code from the line with ... until the line del ... can be written:

with gzip.open(tt,'rt') as f:
    df_final=df_final.append(pd.DataFrame(columns=['hostname'],
                                          data=[line.split('|')[-3].split('/')[0].split('.')[0] for line in f]),
                             ignore_index=True)

So far, I think we saved some time, but I know that appening dataframe in a loop is not the best practice, especially because you need to assign again df_final each time. It's better to add all the dataframes that you want to append together in a list, and then use pd.concat outside of the loop. Your code becomes:

list_final = []
for file in listi:
    tt=os.path.join(directory,file)
    with gzip.open(tt,'rt') as f:
        list_final.append(pd.DataFrame(columns=['hostname'],
                                       data=[line.split('|')[-3].split('/')[0].split('.')[0]
                                             for line in f]))
df_final = (pd.concat(list_final,ignore_index=True)
              .groupby(["hostname"]).size().reset_index(name="Topic_Count"))

Timing

I create one file with around 3 millions of rows, running you method was 8.9 seconds while mine was 5.8 (a gain more than 30%). I run the code on a listi containg 10 of this file, and your method gave more than 91 seconds (a bit more than stricly 10 times the method with one file) while mine was about 57 seconds (a bit less than 10 times the method for just one file).

I don't know about all the multiprocessing or serializing calculations in Python, but it may be a good option too.


Attempting a Strategy design pattern in JS I'm going through the Head First Design Patterns book and I want to check whether I'm understanding some aspects of the first chapter. Does the code below program correctly to interfaces, encapsulate changeable behavior, and employ composition in a reasonable manner?

// trying to enforce some design pattern habits

// create a duck, that prints a line to the page as text
class DuckAbilities {
    constructor() {
        this.flying = new FlyBehavior()
    }
    addToPage() {
        this.statement = document.createElement("p")
        this.statement.innerHTML=this.flying.fly()
        document.body.append(this.statement)
    }
}
//interface for behaviors
class FlyBehavior {
    fly() {
        null
    }
 }

class DoesFly{
    fly() {
        return "I'm flying"
    }
}
class DoesNotFly {
    fly() {
        return "not flying"
    }
}

class Mallard {
    constructor() {
        this.abilities = new DuckAbilities()
        this.abilities.flying = new DoesFly()
    }

}

class Rubber{
    constructor() {
        this.abilities = new DuckAbilities()
        this.abilities.flying = new DoesNotFly()
    }
}

window.onload = ()=> {
    let mallard = new Mallard()
    mallard.abilities.addToPage()
    let rubber = new Rubber()
    rubber.abilities.addToPage()
}

There are some issues with your class, but I don't think that they are so specific to implementing a strategy pattern.

addToPage() {
    this.statement = document.createElement("p")
    this.statement.innerHTML=this.flying.fly()
    document.body.append(this.statement)
}

This assumes there is a global document variable, and that statement is part of the abilities object. If such a method is present, I would expect document to be a parameter of the method and statement to be a local variable. But even that doesn't seem right because you would be mixing representation (a paragraph "p") and a data class.

//interface for behaviors
class FlyBehavior {
    fly() {
        null
    }
 }

Now that doesn't seem right. If you have JS ducktyping then you don't need this class. Furthermore, specifying null is just asking for null pointer exceptions at a later stage.

this.abilities = new DuckAbilities()

OK, so now we have DuckAbilities object but without a valid state, just null, which we then adjust in the next call. There seem to be two ways of resolving this issue:

having the fying behavior as parameter to the DuckAbilities constructor;
removing the DuckAbilities altogether and just assigning the various flybehaviors to a field.

So when we're using classes anyway, lets implement it using those.

I've created an "abstract" class Duck because we require inheritance there. I don't like to create an interface for the strategies because JavaScripts duck-typing should be sufficient.

'use strict'

class Duck { // this is the context
    constructor(flyAbility) {
        // ES2015 only, avoid instantiation of Duck directly
        // if (new.target === Abstract) {
        //  throw new TypeError("Cannot construct Abstract instances directly");
        // }

        this.flyAbility = flyAbility;
    }

    // this is the operation, returning the flyBehavior as a string
    showFlyBehavior() {
        return "This duck " + this.flyAbility.flyBehavior();
    }
}

// Strategy interface is missing due to JS ducktyping

// Strategy #1
class DoesFly {
    // with algorithm #1
    flyBehavior() {
        return "flies";
    }
}

// Strategy #2
class DoesNotFly {
    // with algorithm #2
    flyBehavior() {
        return "doesn't fly";
    }
}

// Context #1
class Mallard extends Duck {
    constructor() {
        super(new DoesFly());
    }
}

// Context #2
class Rubber extends Duck {
    constructor() {
        super(new DoesNotFly());
    }
}

let duck = new Mallard();
console.log(duck.showFlyBehavior());

duck = new Rubber();
console.log(duck.showFlyBehavior());

Sorry about using NodeJS, but in principle only console.log is NodeJS specific ... I hope.


min and max simultaneously I have two numbers: a and b. I want to put the smaller of them in one variable, and the larger in another variable (if they are equal, then any of them can be in any of the variables). Currently my code (in Javascript / Java) takes 7 lines:

if (a<=b) {
    small = a;
    large = b;
} else {
    small = b;
    large = a;
}

I could also use the Math.max and Math.min functions:

small = Math.min(a,b);
large = Math.max(a,b);

This takes only 2 lines but it is wasteful because the test of which one is larger will be done twice - once in Math.min and once in Math.max.

Is there a shorter / more elegant way to do this?

int t = a + b;
a = a > b ?  t - a : a;
b = t - a;
// a is the small one

Or

small = a;
large = b;
if (a>b) {
  small = b;
  large = a;
}

But I think it will be shorter in Python:

if a > b:
   a, b = b, a


Removing first or last character of a string with unit-testing I am trying to remove a duplicate letter of first or last letter, with an exception of containing 'a' as first letter and 'l' and 'e' as last letter. However, this code does not look efficient to me because, what if I had 20 letters of exception? Could I put this in a string or an array? I would not want to compare each letter all in one if statement. What would be a better way to rewrite this code? Example of what the code should do:

aaron should return aaron
bill should return bill
lee should return lee

All other names with duplicate first and last letters should return with the removed duplicate letter. This is a unit test, where firstName is being passed as a string. Instead of using if statement of each letter to check, is there an alternative solution?

UnitTest:

[TestCase("aaron", "aaron")]
public void QualityAssurance(string firstName, string expected)
{
    var actual = warmups.QualityAssurance(firstName);
    Assert.AreEqual(expected, actual);
}

Unit:

public string QualityAssurance(string firstName)
{
    if (firstName[0] != 'a' && firstName[firstName.Length - 1] != 'l' && firstName[firstName.Length - 1] != 'e')
    {
        if (firstName[0] == firstName[1] && firstName[firstName.Length - 1] == firstName[firstName.Length - 2])
        {
            return firstName.Remove(0, 1).Remove(firstName.Length - 2, 1);
        }
        else if (firstName[0] == firstName[1])
        {
            return firstName.Remove(0, 1);
        }
        else if (firstName[firstName.Length - 1] == firstName[firstName.Length - 2])
        {
            return firstName.Remove(firstName.Length - 1);
        }
    }
    return firstName;
}

You're checking both the start and end twice and you only modify when both start and end have duplicate letters. Is that intentionally?:

  if (firstName[0] != 'a' && firstName[firstName.Length - 1] != 'l' && firstName[firstName.Length - 1] != 'e')
  {
    if (firstName[0] == firstName[1] && firstName[firstName.Length - 1] == firstName[firstName.Length - 2])
    {
      return firstName.Remove(0, 1).Remove(firstName.Length - 2, 1);
    }
    else if (firstName[0] == firstName[1])
    {
      return firstName.Remove(0, 1);
    }
    else if (firstName[firstName.Length - 1] == firstName[firstName.Length - 2])
    {
      return firstName.Remove(firstName.Length - 1);
    }
  }

It could be simplified to:

  if (string.IsNullOrEmpty(name) || name.Length < 2) return name;

  if (name[0] != 'a' && name[name.Length - 1] != 'l' && name[name.Length - 1] != 'e')
  {
    if (name[0] == name[1])
      name = name.Substring(1);
    if (name.Length > 1 && name[name.Length - 1] == name[name.Length - 2])
      name = name.Substring(0, name.Length - 1);
  }
  return name;

But what about "aaronn" shouldn't the last "nn" be reduced to one 'n' in that?

If so, you'll have to check start and end separately:

string RemoveDuplicatesAtStartAndEnd(string name)
{
  if (string.IsNullOrEmpty(name) || name.Length < 2) return name;

  if (name[0] != 'a' && name[0] == name[1])
    name = name.Substring(1);
  if (name.Length > 1 && name[name.Length - 1] != 'l' && name[name.Length - 1] != 'e' && name[name.Length - 1] == name[name.Length - 2])
    name = name.Substring(0, name.Length - 1);

  return name;
}

and as shown, you'll also have to validate the input before handling it.

If you want to handle any number of duplicates chars at start and end you can do:

string RemoveDuplicatesAtStartAndEnd(string name)
{
  if (string.IsNullOrEmpty(name) || name.Length < 2) return name;

  while (name.Length > 1 && name[0] != 'a' && name[0] == name[1])
    name = name.Substring(1);

  while (name.Length > 1 && name[name.Length - 1] != 'l' && name[name.Length - 1] != 'e' && name[name.Length - 1] == name[name.Length - 2])
    name = name.Substring(0, name.Length - 1);

  return name;
}

Another way could be to use Regex:

string RemoveDuplicatesAtStartAndEnd(string name)
{
  name = Regex.Replace(name, @"^([^a])\1{1,}", m => m.Value[0].ToString());
  return Regex.Replace(name, @"([^el])\1{1,}$", m => m.Value[0].ToString());
}

Other regex approaches may be valid, but this is one way to do it.

What about upper-lower case?

Your purpose for this method is not clear, but I wouldn't be confident about it, taken all possible names into account. Manipulate names in this way seems as risky business IMO.


Parsing JSON and exploring relationships in Python Question:

  Given two users say user1 and usern, print True if there was a chain
  of tweets such that user1 tagged user2, user2 tagged user3 .....
  usern-1 tagged usern or viceversa.

  Input Format

  The 1st line and the 2nd line contains the two users. The 3rd line of
  the input contains the filename in which the dataset of tweets in JSON
  format.

  JSON Format

  { "TweetCount" : 1, "TwitterData" : [ { "userName" : "user1",
  "tweetid" : 12345, "tweet" : "@user2" } ] }

  Output Format

  Print "True" ( quotes are for clarity ) if the two mentioned users are
  connected, and "False" otherwise.

  Constraints:

  1 <= TweetCount <= 300000
  Username, Tweet are of UTF-8 format
  1 <= Tweetid <= 1020

  Sample Input:

user1
user2
input000.in

  input000.in

  { "TweetCount" : 2, "TwitterData" : [ { "userName" : "user1",
  "tweetid" : 1, "tweet" : "Tweet 1 @user2" }, { "userName" : "user2",
  "tweetid" : 2, "tweet" : "Tweet 2 @user4" } ] }

  Sample Output:

  True

  Explanation:

  user1 has user2 tagged in his tweet, hence True

I was wondering if you could take a look at my code and see if

there's a better way of solving the problem than the way I did it (i.e. should I construct a graph?)
I make my code more pythonic

import json

# standardize all input names to lower case
user1 = raw_input().lower()
user2 = raw_input().lower()

filename = raw_input()
user1_mentions = []
to_explore = []
reached = [] # list of users reached

with open(filename) as f:
    json = json.load(f) # JSON representation of input file

# find user1's mentions
for tweet in json['TwitterData']:
    if tweet['userName'] == user1:
        user1_mentions += ([x[1:].encode('utf-8') for x in tweet['tweet'].split() if x.startswith('@') and x[1:].isalnum()])
        to_explore = user1_mentions

while to_explore != []:
    temp_to_explore = [] # temp array for storing words to explore next
    for tweet in json['TwitterData']:
        if tweet['userName'] in to_explore:
            temp_to_explore += ([x[1:].encode('utf-8') for x in tweet['tweet'].split() if x.startswith('@') and x[1:].isalnum()])
    reached += to_explore
    to_explore = temp_to_explore

print reached # ['user2', 'user4']
print user2 in reached # True

Writing pythonic code

Python has a coding guideline called PEP 8. It contains a lot of relevant information about how to write your code. You'll find various tools to check your code againt PEP 8 to ensure you follow it properly.

Among other things :

  For sequences, (strings, lists, tuples), use the fact that empty
  sequences are false.

In your case, you should write while to_explore instead of while to_explore != [].

A bit of scafolding

In order to improve your code, it is a good thing to split it into different parts. Here I have splitted your code into functions. I took this chance to move the part calling functions behind an if main guard. Also, I have hardcoded some values to make testing easier. To make things better, the different functions should be documented but I can't be bothered.

import json

def get_input_data(from_user = False):
    if from_user:
        # standardize all input names to lower case
        user1 = raw_input().lower()
        user2 = raw_input().lower()
        with open(raw_input()) as f:
            json_data = json.load(f) # JSON representation of input file
    else:
        user1 = 'user1'
        user2 = 'user2'
        json_data = json.loads('{ "TweetCount" : 1, "TwitterData" : [ { "userName" : "user1", "tweetid" : 12345, "tweet" : "@user2" } ] }')
    data = json_data['TwitterData']
    return (user1, user2, data)

def chain_exists(user1, user2, data):
    user1_mentions = []
    to_explore = []
    reached = [] # list of users reached

    # find user1's mentions
    for tweet in data:
        if tweet['userName'] == user1:
            user1_mentions += ([x[1:].encode('utf-8') for x in tweet['tweet'].split() if x.startswith('@') and x[1:].isalnum()])
            to_explore = user1_mentions

    while to_explore != []:
        temp_to_explore = [] # temp array for storing words to explore next
        for tweet in data:
            if tweet['userName'] in to_explore:
                temp_to_explore += ([x[1:].encode('utf-8') for x in tweet['tweet'].split() if x.startswith('@') and x[1:].isalnum()])
        reached += to_explore
        to_explore = temp_to_explore

    print reached # ['user2', 'user4']
    return user2 in reached

if __name__ == '__main__':
    user1, user2, data = get_input_data()
    print(chain_exists(user1, user2, data))

It could be interesting to add more hardcoded tests.

A bit of logic

Everytime you add something to user1_mentions, you change to_explore. It probably would make sense to get the value at the end of the loop.

# find user1's mentions
for tweet in data:
    if tweet['userName'] == user1:
        user1_mentions += ([x[1:].encode('utf-8') for x in tweet['tweet'].split() if x.startswith('@') and x[1:].isalnum()])
to_explore = user1_mentions # this doesn't need to be defined before this point

The right data structure

The way you build user1_mentions, it might contain the same value multiple times. It is something we might want to avoid because it will not bring anything except performance issues. Instead of using list, you should use sets.

def chain_exists(user1, user2, data):
    user1_mentions = set()
    reached = [] # list of users reached

    # find user1's mentions
    for tweet in data:
        if tweet['userName'] == user1:
            user1_mentions.update([x[1:].encode('utf-8') for x in tweet['tweet'].split() if x.startswith('@') and x[1:].isalnum()])
    to_explore = user1_mentions

    while to_explore:
        temp_to_explore = set() # temp set for storing words to explore next
        for tweet in data:
            if tweet['userName'] in to_explore:
                temp_to_explore.update([x[1:].encode('utf-8') for x in tweet['tweet'].split() if x.startswith('@') and x[1:].isalnum()])
        reached += to_explore
        to_explore = temp_to_explore

    print reached # ['user2', 'user4']
    return user2 in reached

The very right data structure

In order to use to implement efficient algorithm, you'll need to preprocess your data into something relevant. Mapping user names to the set of users they mention is likely to be required. Here's a piece of code to do so :

def make_graph_from_data(data):
    graph = {}
    for tweet in data:
        graph.setdefault(
            tweet['userName'],
            set()).update(x[1:] for x in tweet['tweet'].split() if x.startswith('@') and x[1:].isalnum)
    return graph

Once you have the data preprocess, you won't need to worry about parsing tweets or whatever.

A simple bug

What if a user was to mention hisself ? You'd get stuck in the loop.

Conclusion

You'll find various solutions to your problem in the litterature so I'll let you have a look at this : it might be a good idea to consider solutions to the shortest path problem by considering a graph : you are in a case of Directed graphs with nonnegative weights. There might be some even more relevant algorithm because you care about the existence of a chain but not so much about it length but I don't have a particular algorithm in mind.


Find smallest prime number greater than given n I need to write a program which will find a prime number, greater than the given n.

Can it be done simpler?

#include <stdio.h>

int isPrime(int n)
{
    int i,j=0;
    for(i=1; i<=n; i++)
    {
        if(n%i == 0)
            j++;
    }
    if(j == 2)
        return 1;
    else if(j > 2)
        return 0;
}

int fun(unsigned int n)
{
    int i=n+1;
    while(1)
    {
        if(isPrime(i))
            break;
        i++;
    }
    return i;
}

int main(int argc, char **argv)
{
    printf("%d\n", fun(19));
    return 0;
}

You can implement function isPrime to test primality in a little more efficient manner:

int isPrime(int n) // assuming n > 1
{
    int i,root;

    if (n%2 == 0 || n%3 == 0)
        return 0;

    root = (int)sqrt(n);

    for (i=5; i<=root; i+=6)
    {
        if (n%i == 0)
           return 0;
    }

    for (i=7; i<=root; i+=6)
    {
        if (n%i == 0)
           return 0;
    }

    return 1;
}


Delete last node in singly linked list In a TED talk Linus Torvalds made a point about “good taste” at approximately 14:10 in the interview. I read through his examples of “bad taste” and “good taste” and wanted to implement the same principle for a function that deletes the last node in a singly linked list. I also followed his coding style.
#include <stdio.h>
#include <stdlib.h>

/* linked list structure */
struct node {
    int data;
    struct node *next;
};

/* create new node */
struct node *new_node(int data, struct node *next)
{
    struct node *new = malloc(sizeof *new);
    if (!new) {
        fprintf(stderr, "Error: memory allocation failed\n");
        exit(EXIT_FAILURE);
    }
    new->data = data;
    new->next = next;
    return new;
}

/* delete last node */
void delete_last(struct node *head)
{
    if (head == NULL) {
        fprintf(stderr, "Error: linked list underflow\n");
        exit(EXIT_FAILURE);
    }
    struct node **cursor = &head;
    while ((*cursor)->next != NULL)
        cursor = &(*cursor)->next;
    free(*cursor);
    *cursor = NULL;
}

I also followed [Torvalds'] coding style

I'm all for coding style standards, though I decline to listen to all of his suggestions simply because he's the loudest (seriously, very very loud) voice in the room. His blind obedience to 1988's K&R with no other rationale I find short-sighted as well. Anyway, enough editorializing:
Your non-parenthesized sizeof *new falls on a matter in his guide that is a little ambiguous -

The notable exceptions are sizeof, typeof, alignof, and __attribute__, which look somewhat like functions (and are usually used with parentheses in Linux, although they are not required in the language)

Whereas parens are not required after sizeof, I see them in the majority of code I encounter so I'd recommend sticking with them.
I warn against writing keywords from C++ as symbol names in C, in this case new. If ever you want this to be readily C++-compatible, this will cause you grief.


Internet Speed Calculator This program calculates up and downspeed of a given connection for as long as it is running.

#upspeed/downspeed indicator
#written by Chubak -> aurlito@riseup.net

import requests
import time
import os

def get_seconds(datetime):
    time = datetime.split(' ')[3].split(':')

    time_hours = int(time[0])
    time_minutes = int(time[1])
    time_seconds = int(time[2])

    return time_hours*360 + time_minutes*60 + time_seconds

def downspeed():
    url = "http://speedtest.ftp.otenet.gr/files/test100k.db"

    current_seconds = get_seconds(time.asctime())

    file = requests.get(url)
    headers = file.headers

    file_size = int(headers['Content-Length'])/1000

    dl_seconds = get_seconds(time.asctime())

    time_difference = dl_seconds - current_seconds

    return round(file_size / time_difference)

def upspeed():
    current_seconds = get_seconds(time.asctime())

    dummy_file = os.path.join(os.getenv('APPDATA'), 'dummy.txt')
    post_url = 'http://httpbin.org/post'
    with open(dummy_file, 'wb') as dummy:
        for i in range (1500):
            dummy.write(str.encode('This is a dummy text. Its sole propose is being uploaded to a server. '))

        dummy.close()

    files = {'file' : open(dummy_file, 'rb')}

    request = requests.post(post_url, data=files)

    headers = request.headers

    file_size = int(headers['Content-Length'])/1000

    dl_seconds = get_seconds(time.asctime())

    time_difference = dl_seconds - current_seconds

    return round(file_size / time_difference)

if __name__ == '__main__':
    while True:
        up = None
        down = None

        if up == None and down == None:
            up = upspeed()
            down = downspeed()

            print('At {0} your Downspeed is: {1}, and your Upspeed is: {2}'.format(time.asctime(), down, up))

            time.sleep(5)

            up = None
            down = None

I'd appreciate it if your dropped any hints about turning this into a desktop widget.

Your whole function get_seconds is not needed at all. You can use the function time.time(), which gives you the seconds since epoch (Jan 1, 1970 0:00:00). Subtracting two such times will yield the number of seconds passed in between. I would also do the time measurement exclusively around the actual download, otherwise you add the (small) overhead of reading the header for the file-size.

def downspeed():
    url = "http://speedtest.ftp.otenet.gr/files/test100k.db"

    start = time.time()
    file = requests.get(url)
    end = time.time()

    time_difference = end - start
    file_size = int(file.headers['Content-Length'])/1000
    return round(file_size / time_difference)

You should also report the units of your speeds measured. As it is, they will be KB/s, with the K denoting 1000 (as you divide by that). You should be aware that there is also a different unit, sometimes written KiB/s, where Ki stands for 1024. You should probably use the same definition, which speedtest uses, when it says the file is 100kb big.


Javascript form parser I am trying to make a generic form parser for Javascript. The idea being, a developer could drop the function onto a form and sumbit via AJAX, or do something with a form besides submit it. The result will be in the same format used in a query string.

What I have posted works, so I am basically wondering if this is a good design? What could be improved? Thanks for the help!

function test_submit(id){
    var results=[],form=document.getElementById(id);
    for(var i=0;i<form.elements.length;++i){
        var obj=form.elements[i];
        if(obj.name&&!obj.disabled){
            switch(obj.tagName){
            case 'SELECT':
                for(var j=0;j<obj.length;++j){
                    if(obj.options[j].selected){
                        var value=obj.options[j].value;
                        if(!value)value=obj.options[j].text;
                        results.push(obj.name+'='+escape(value));
                    }
                }
                break;
            case 'INPUT':
            case 'TEXTAREA':
                var type=obj.type,value=obj.value;
                if(type)type=type.toLowerCase();
                if(type&&(type=='radio'||type=='checkbox')){
                    if(obj.checked)results.push(obj.name+'='+(value?escape(value):'on'));
                }
                else results.push(obj.name+'='+escape(value));
                break;
            }
        }
    }
    return results.join('&');
}

I'd change two things.  I'd start by declaring your var's in one statement like so:

function test_submit(id){
    var q=[],
        p=$(id).getElementsByTagName('SELECT');

Second I'd use more descriptive variable names than i, p, r, n.  If you run this code through a minifier it'll do this for you.  When you come back to edit this code in a few months you'll want to know exactly what each variable is for, and using a descriptive name helps with that.

I'd also be leary of using $ in the global namespace as a variable name since a lot of the popular libraries use it and you don't want to have any collisions with those.


Display a dropdown based on another dropdown change I have a dropdown and when that dropdown changes I want to display/render another dropdown. Can someone help me improve it?

So, a person requests for getting referred in a company for a job. When that person selects a company name in the "Referral Form" I want to show only the cover letters for specific to that company.

My JSON Action:

public JsonResult ListOfCoverLetterByCompanyId(int companyId)
{
    var coverletters = _context.CoverLetters
        .Where(c => c.CompanyId == companyId)
        .ToList();

    var dropdown = new List<SelectListItem>();
    foreach (var cl in coverletters)
    {
        dropdown.Add(new SelectListItem { Text = cl.CoverLetterName, Value = cl.CoverLetterId.ToString() });
    }
    return Json(dropdown, JsonRequestBehavior.AllowGet);
}

My View File:

@model Bridge.ViewModels.ReferralViewModel

@using (Html.BeginForm())
{
    @Html.AntiForgeryToken()

Job Request

        @Html.ValidationSummary(true, "", new { @class = "text-danger" })

            @Html.LabelFor(model => model.CompanyId, htmlAttributes: new { @class = "control-label col-md-2" })

                @Html.DropDownListFor(m => m.CompanyId, Model.Companies, new { @class = "form-control js-change", onchange = "companyChanged()" })
                @Html.ValidationMessageFor(model => model.CompanyId, "", new { @class = "text-danger" })

        <select id="ajaxdrop" name="ajaxdrop"></select>

                <input type="submit" value="Create" class="btn btn-default" />

}

@section scripts{

}

Model:

public class ReferralViewModel
{
    public int ReferralViewModelId { get; set; }
    [Display(Name = "Resume")]
    public int ResumeId { get; set; }
    public IEnumerable<Resume> Resumes { get; set; }
    public int CoverLetterId { get; set; }
}

First to address the controller method your calling in the ajax function.

Your currently returning a serialized collection of SelectListItem which means your unnecessarily passing all the values of the other properties of SelectListItem (Selected, Group and Disabled) back to the view when you don't need or use them. You should only pass back the values you need, by either using a view model, or by creating anonymous objects. Note also that you do not need .ToList() in your query. Your code can simply be

public JsonResult ListOfCoverLetterByCompanyId(int companyId)
{
    var coverletters = _context.CoverLetters.Where(x => x.CompanyId == companyId).Select(x => new
    {
        Value = x.CoverLetterId,
        Text = x.CoverLetterName
    });
    return Json(coverletters, JsonRequestBehavior.AllowGet);
}

Your method is also a [HttpGet] meaning that users can navigate to it via the address bar, which is probably not what you want. You should mark the method [HttpPost] (in which case you do not need the JsonRequestBehavior.AllowGet argument). Alternatively you can check if the request is an ajax request. A simple way to do that is to create a FilterAttribute. A typical example is shown in MVC Set accessibility level on a method called from ajax.

The main issue with your code however, is that your not binding the 2nd dropdownlist to your model. Not only does your name attribute of the <select> not match the property you want to bind it to (CoverLetterId), but if the user submits the form and you need to return the view because ModelState is invalid, then the data the user entered in the 2nd dropdownlist is lost (in fact the 2nd dropdownlist will be empty). You also will not be able to use you code for editing an existing Referral for the same reason. To address this, first modify your model to

public class ReferralViewModel
{
    public int? Id { get; set; }
    [Display(Name = "Company")]
    [Required(ErrorMessage = "Please select a Company")]
    public int? CompanyId { get; set; }
    [Display(Name = "Cover Letter")]
    [Required(ErrorMessage = "Please select a Cover Letter")]
    public int? CoverLetterId { get; set; }

    public IEnumerable<SelectListItem> Companies { get; set; }
    public IEnumerable<SelectListItem> CoverLetters { get; set; }
}

A few things to note.

Name your PK property just Id. This means that if your using
the same view model for editing an existing Referral, that you do
not need to include a hidden input in the view for the property
(assuming your using the default route, and that your method is
public ActionResult Edit(int id)). The framework will automatically
add a route value in the forms action attribute and it will
automatically be bound in the POST method.
Assuming that both ResumeId and CoverLetterId are required in
the database, you should make your properties nullable and add a
[Required] attribute to protect against under-posting attacks.
Refer What does it mean for a property to be [Required] and
nullable?
for more detail.

Your model is also referring to ResumeId, but the view code is creating the first dropdownlist for CompanyId so I assume that was just a typo.

Now your view code for the dropdownlists will then be (<div> elements omitted for clarity)

@Html.LabelFor(m => model.CompanyId, new { @class = "control-label col-md-2" })
@Html.DropDownListFor(m => m.CompanyId, Model.Companies, "Please select", new { @class = "form-control" })
@Html.ValidationMessageFor(m => m.CompanyId, "", new { @class = "text-danger" })

@Html.LabelFor(m => model.CoverLetterId, new { @class = "control-label col-md-2" })
@Html.DropDownListFor(m => m.CoverLetterId, Model.CoverLetters, new { @class = "form-control" })
@Html.ValidationMessageFor(m => m.CompanyId, "", new { @class = "text-danger" })

Note that I have removed the additional class name, and the onchange attribute which I will discuss later. Note also the 3rd parameter in the 1st DropDownListFor() method, which adds a <option value="">Please select</option> which will be selected if a value for CompanyId has not been set, and forces the user to make a selection and trigger the ajax call. It also means that when the user submits the form, a client side validation error will be shown if they have not selected a Company.

Then you need to modify your controller code so that if the value of CompanyId is null, then you set the value of CoverLetters to be an empty SelectList (i.e when the page is first rendered in your Create() method, but if its not null, then populate it based on the value of CompanyId (i.e. in a POST method if you need to return the view, or in an Edit() method. You controller code would typically look like

public ActionResult Create()
{
    ReferralViewModel model = new ReferralViewModel();
    ConfigureViewModel(model);
    return View(model);
}
[HttpPost]
public ActionResult Create(ReferralViewModel model)
{
    if (!ModelState.IsValid)
    {
        ConfigureViewModel(model);
        return View(model);
    }
    .... // Initialize a new instance of the data model, set its properties based on the view model, save and redirect
}
private void ConfigureViewModel(ReferralViewModel model)
{
    // Populate companies always
    model.Companies = _context.Companies.Select(x => new SelectListItem
    {
        Text = x.CompanyName,
        Value = x.CompanyId.ToString()
    });
    // Populate cover letters only if a company has been selected
    // i.e. if your editing an existing Referral, or if you return the view in the POST method
    if (model.CompanyId.HasValue)
    {
        model.CoverLetters = _context.CoverLetters.Where(x => x.CompanyId == model.CompanyId).Select(x => new SelectListItem
        {
            Value = x.CoverLetterId.ToString(),
            Text = x.CoverLetterName
        });
    }
    else
    {
        model.CoverLetters  = new SelectList(Enumerable.Empty<SelectListItem>());
    }
}

Finally, your scripts.

It is better practice to use Unobtrusive Javascript rather than polluting your htlm markup with behavior, so rather that using an onchange attribute, use $('#CompanyId').change(function() { ...

You should also cache elements that you repeatedly refer to by assigning them to a javascript variable so that your functions are not searching through the DOM again. You script would look like

var coverLetterSelect = $('#CoverLetterId'); // cache it
$('#CompanyId').change(function() {
    var companyId = $(this).val(); // use $(this)
    coverLetterSelect.empty(); // clear existing options
    if (!companyId) {
        return; // there is no point making an ajax call so just exit
    }
    $.ajax({
        type: 'POST', // or 'GET' depending on how you have modified the method above
        url: "@Url.Action("ListOfCoverLetterByCompanyId", "Referral")', // use @Url.Action() to generate urls
        data: {companyId : companyId },
        success: function (response) {
            coverLetterSelect.append($('<option></option>').val('').text('Please select')); // add a default null option
            $.each(response, function (i, item) {
                coverLetterSelect.append($('<option></option>').val(item.Value).text(item.Text));
            });
        },
        ....
    });
})

Note that you need to remove the contentType option.


Update Google Sheet with Python Below is a code to update a google sheet using the database values. The script is working fine but it is rather slow because of the sleep function which I have to put because of the "Quota exceeding error" below. If I remove the sleep time it would work much faster but after a certain number of rows get updated it will throw the below error.

I have to update three columns namely A,B & C of which the value start row is from 4th row onwards. So basically I need advise/recommendations to improve this code on the below points:

To make the query execution faster without getting the below error.
Is there a way to generalise this script, means without hardcoding the spreadsheet name (DB details I can get in a generalised way and read from config, for testing purpose only hardcoded the values)

gspread.exceptions.APIError: {   "error": {
    "code": 429,
    "message": "Insufficient tokens for quota 'ReadGroup' and limit 'USER-100s' of service 'sheets.googleapis.com' for consumer 'project_number:*******'.",
    "status": "RESOURCE_EXHAUSTED",
    "details": [
      {
        "@type": "type.googleapis.com/google.rpc.Help",
        "links": [
          {
            "description": "Google developer console API key",
            "url": "https://console.developers.google.com/project/***/apiui/credential"
          }
        ]
      }
    ]   } }

Python Code:

import psycopg2
import gspread
from oauth2client.service_account import ServiceAccountCredentials
from time import sleep
import datetime

def update_sheet(sheet, table, rangeStart='A', rangeEnd='C'):
 for index, row in enumerate(table):
    range = '{start}{i}:{end}{i}'.format(start=rangeStart, end=rangeEnd, i=index+4)
    cell_list = sheet.range(range)
    for i, cell in enumerate(cell_list):
      start_time = datetime.datetime.now()
      cell.value = row[i]
      sheet.update_cells(cell_list)
      end_time = datetime.datetime.now()
      if (end_time - start_time).total_seconds() < 1:
         sleep(1.01 - (end_time - start_time).total_seconds())

cnx_psql = psycopg2.connect(host="xxx.xxx.xxx.xx", database="postgres", user="postgres",password="******", port="5432")
print('DB connected')
psql_cursor = cnx_psql.cursor()

METADATA_QUERY = '''select product_id,CAST(low_stock_date as TEXT) low_stock_date,sku from test.low_stock_date;'''
psql_cursor.execute(METADATA_QUERY)
results = psql_cursor.fetchall()
cell_values = (results)

scope =  ['https://spreadsheets.google.com/feeds', 'https://www.googleapis.com/auth/drive']
creds = ServiceAccountCredentials.from_json_keyfile_name('/Users/lins/Documents/GS_secret/secret_key.json',scope)
client = gspread.authorize(creds)
sheet = client.open_by_url('https://docs.google.com/spreadsheets/d/1****zQ/edit#gid=0').sheet1

#Function Call
update_sheet(sheet, cell_values)

psql_curso.close()
cnx_psql.close()

The update_cells call can update multiple cells at once. So you should calculate all new values locally and push the changes in one update. This should use only one token, instead of one per cell (untested code):

def update_sheet(sheet, table, start='A', end='C'):
    to_update = []
    for i, row in enumerate(table):
        cells = sheet.range(f'{start}{i+4}:{end}{i+4}')
        for cell, value in zip(cells, row):
            cell.value = value
        to_update.extend(cells)
    sheet.update_cells(to_update)

I also changed the names and indentation to follow Python's official style-guide, PEP8, and used an f-string for easier string formatting.

This still has the problem that it needs to get each row from the sheet, each of which does an API call. Instead you can get each column as a range and update one column at a time, which saves calls if your table has less columns than rows. For this we need to transpose the table first, though:

def update_sheet(sheet, table, columns="ABC", header=4):
    to_update = []
    table = list(zip(*table))    # transpose the table
    for col_name, col in zip(columns, table):   # iterating over columns now
        r = f"{col_name}{header}:{col_name}{len(col)+header}"  # changed range
        print(r)  # for debugging
        cells = sheet.range(r)
        for cell, value in zip(cells, col):
            cell.value = value
        to_update.extend(cells)
    sheet.update_cells(to_update)

With this it is no problem to use e.g. this table of size 2500 x 3:

import numpy as np

...
table = list(map(list, np.arange(2500*3).reshape(-1, 3).astype(str)))
update_sheet(sheet, table)


Checking balance of a travel card with Selenium A Leap Card is essentially a travel card for Dublin, Ireland that can be used to pay for a bus or tram fare. It has to be topped up and I often forget how much I have left on it.

To get familiar with Selenium, I wrote some code that gets the balance for me. I haven't worked with this framework before, so I'd like to know if there's room for improvement here.

import json

from selenium import webdriver

def get_credentials():
    """Get user credentials from a JSON file"""
    with open('auth.json', 'r') as f:
        auth = json.load(f)
        username = auth['username']
        password = auth['password']
    return username, password

class LeapHandler:
    """Class to handle any interactions with Leap Card website"""

    def __init__(self):
        self.driver = webdriver.Chrome()
        self.driver.get('https://www.leapcard.ie/en/login.aspx')
        self.login()

    def get_login_elements(self):
        """Find and return elements necessary to log in"""
        username_id = 'ContentPlaceHolder1_UserName'
        password_id = 'ContentPlaceHolder1_Password'
        login_id = 'ContentPlaceHolder1_btnlogin'
        username_field = self.driver.find_element_by_id(username_id)
        password_field = self.driver.find_element_by_id(password_id)
        login_button = self.driver.find_element_by_id(login_id)
        return username_field, password_field, login_button

    def login(self):
        """Login into user account"""
        username_field, password_field, login = self.get_login_elements()
        username, password = get_credentials()
        username_field.send_keys(username)
        password_field.send_keys(password)
        login.click()

    def get_balance(self):
        """Return current Leap Card Balance"""
        balance = self.driver.find_element_by_css_selector('div.pull-left').text
        return balance

if __name__ == '__main__':
    lh = LeapHandler()
    balance = lh.get_balance()
    print(f'LepaCard balance is €{balance}')

Your code is actually quite clean and readable, good job. Here are some of the things I would consider changing:

you can simplify get_credentials() a bit, using the tuple packing/unpacking:

def get_credentials():
    """Get user credentials from a JSON file."""
    with open('auth.json', 'r') as f:
        auth = json.load(f)
        return auth['username'], auth['password']

end your docstrings with a dot as per PEP8
if the page manipulation selenium code is going to continue grow, you should definitely consider switching to using Page Object pattern building abstractions around the webpages you are working with. But, for this kind of problem, it is perfectly fine to not use page objects
div.pull-left does not look like a good quality locator. It may work now, but this locator is fragile because it is design-specific (pull-left is a bootstrap class used for page layout). See if this element has an id or a data-oriented class or a data-oriented attribute.


GIF background rollover I made a cool roll-over in CSS which displays an animated GIF when the parent is hovered.

Codepen

The problem is that I will have about 40 .day like that, all playing the animated GIF behind the circle at any time. I'm not sure that any browser/computer/connection can handle that much.

I tried to figure out a solution:

$( ".day" ).mouseenter(function() {
    $( ".play" ).append( "<img src="http://media.giphy.com/media/5Vb7xQB7Z3ScE/giphy.gif">" );
});

But I'm not sure if it will be better.

If someone as any idea to enhance that code, it would be really appreciated.

(The gif is just a sample.)

First off, as far as I remember the gif is only loaded once regardless of how many there are on the page (Just opened the dev tools on that page and it does. Thank you crystal ball.) So HTTP request overhead isn't a problem.

As for rendering, slower machines and browsers will tend to crawl when rendering a lot of animated gifs. So 40 of those would considerably be a pain to do.

Now off to your code, yes it is a problem because you are you are getting all .play on every mouseenter, create and append a new image object for each of them. The overhead lies in the creating of a jQuery object for the .play set on each mouseenter as well as the generation of an image for each .play on each mouseenter.

I think the JS solution is better because it doesn't render 40 animated gifs all at once, and your code can be tweaked a bit for optimization. We can create the image just once and move it around using appendTo. Also, since the mouseover handler already has a reference to the target being hovered, we can use that as target for appendTo.

// Create an image just once
var img = $('<img src="http://media.giphy.com/media/5Vb7xQB7Z3ScE/giphy.gif">');

// Since we can't hover on .play because it's behind .img-circle, we designate
// .img-circle as our hover target, and use previousSibling to get the .play
$('.day').on('mouseover','.img-circle',function(event){

  // Here, we are not generating any new objects. img and this.previousSibling
  // are already provided for
  img.appendTo(this.previousSibling);
});

Here's a working demo


Long SQL query with much duplication between two halves of a UNION This works, but it's huge.

We need to repeat each table column on each UNION SELECT, as well as the WHERE clause for DATE comparison.

(
SELECT GROUP_CONCAT(APA_T.district), t.name
FROM tbl_activity AS t
INNER JOIN tbl_activity_package AS ap ON t.id = ap.activity_id
INNER JOIN (
SELECT DISTINCT apa.district AS district, (
SELECT s1.activity_package_id
FROM tbl_activity_package_address s1
WHERE apa.district = s1.district
ORDER BY s1.id DESC
LIMIT 1
) AS idActivityPackage
FROM
tbl_activity_package_address apa
ORDER BY apa.district
) AS APA_T
ON ap.id = APA_T.idActivityPackage
WHERE (ap.publication_date <= CURDATE())
AND (DATE_ADD(ap.publication_date, INTERVAL ap.publication_duration_in_days DAY) >= CURDATE())
GROUP BY t.name
ORDER BY APA_T.district
)
UNION DISTINCT
(
SELECT GROUP_CONCAT(DISTINCT apa2.district ORDER BY apa2.district), t2.name
FROM tbl_activity AS t2
INNER JOIN tbl_activity_package AS ap2 ON t2.id = ap2.activity_id
INNER JOIN tbl_activity_package_address AS apa2 ON ap2.id = apa2.activity_package_id
WHERE (ap2.publication_date <= CURDATE())
AND (DATE_ADD(ap2.publication_date, INTERVAL ap2.publication_duration_in_days DAY) >= CURDATE())
GROUP BY t2.name
ORDER BY apa2.district
)
LIMIT 6, 6

I have removed the WHERE CLAUSE for DATE comparisons, because I can't have it on this fiddle structure, but, the repetition is still an issue. I've been told about INTERSECT but found nothing about that on MySQL.

http://sqlfiddle.com/#!2/37dd94/51

Your SQL Fiddle and the question have inconsistent column names: the fiddle has id_activity_package, while the code in this question has activity_package_id.  I'll consider the fiddle as supplementary information, and treat the code in the question as authoritative.

The two halves of the UNION are nearly identical, with the exception of the second INNER JOIN.  Let's analyze it one step at a time.

In the first half of the UNION, you join with APA_T, which is…

SELECT DISTINCT apa.district AS district,
    (SELECT s1.activity_package_id
        FROM tbl_activity_package_address s1
        WHERE apa.district = s1.district
        ORDER BY s1.id DESC
        LIMIT 1
    ) AS idActivityPackage
    FROM tbl_activity_package_address apa
    ORDER BY apa.district

The DISTINCT there is superfluous, since the innermost SELECT is guaranteed to return at most one row per district.

As I read it, APA_T is a subquery that lists the most recently added activity_package_id for each district.  I think it's worth creating a view for clarity.

CREATE VIEW latest_activity_package_address AS
SELECT *
    FROM tbl_activity_package_address
    WHERE id IN (
        SELECT MAX(id)
            FROM tbl_activity_package_address AS latest_apa
            GROUP BY district
    );

Then, the first half of the UNION becomes:

SELECT GROUP_CONCAT(district ORDER BY district), t.name
    FROM tbl_activity AS t
        INNER JOIN tbl_activity_package AS ap
            ON t.id = ap.activity_id
        INNER JOIN latest_activity_package_address AS apa
            ON ap.id = apa.activity_package_id
    WHERE (ap.publication_date <= CURDATE())
        AND (DATE_ADD(ap.publication_date, INTERVAL ap.publication_duration_in_days DAY) >= CURDATE())
    GROUP BY t.name
    ORDER BY apa.district

I've copied the ORDER BY clause into the GROUP_CONCAT(), where I think it belongs.
Next, I'd like to point out that LIMIT 6,6 will produce non-deterministic results, since there is no ORDER BY clause on the UNION query.  You've put an ORDER BY on each half of the UNION, but that doesn't guarantee that the result will be ordered the same way.
Surprisingly, I haven't found a way to reduce duplication between the two halves of the UNION.  However, there is now a nice parallelism which may be more aesthetically pleasing.  You don't need to select new table aliases for the second half; they aren't in the same scope as the first half.

SELECT GROUP_CONCAT(district ORDER BY district), t.name
    FROM tbl_activity AS t
        INNER JOIN tbl_activity_package AS ap
            ON t.id = ap.activity_id
        INNER JOIN latest_activity_package_address AS apa
            ON ap.id = apa.activity_package_id
    WHERE (ap.publication_date <= CURDATE())
        AND (DATE_ADD(ap.publication_date, INTERVAL ap.publication_duration_in_days DAY) >= CURDATE())
    GROUP BY t.name
    ORDER BY apa.district
UNION DISTINCT
SELECT GROUP_CONCAT(district ORDER BY district), t.name
    FROM tbl_activity AS t
        INNER JOIN tbl_activity_package AS ap
            ON t.id = ap.activity_id
        INNER JOIN tbl_activity_package_address AS apa
            ON ap.id = apa.activity_package_id
    WHERE (ap.publication_date <= CURDATE())
        AND (DATE_ADD(ap.publication_date, INTERVAL ap.publication_duration_in_days DAY) >= CURDATE())
    GROUP BY t.name
    ORDER BY apa.district


Customizing errors based on HTTP status code in golang I want to collect errors and its causes by defining custom errors (HTTP status 40x & 50x). For example, if user's request was bad, I want more specific reasons for the error.

A customized error has its own code (not equal to HTTP status code, but related since the code format is $HTTP_STATUS (3-digit) + own code (2-digit). The reason it has its own error code is that I store this error data (as JSON) to Elasticsearch and query for each error's occurrences in Kibana.

I'd like to get reviews on the following codes. Best practice or any small tips for custom errors are welcome.

error.go :

package main

import (
    "encoding/json"
    "fmt"
    "net/http"
)

const (
    codeEmptyNameErr        = 40001
    codeInvalidAgeErr       = 40002
    codeMethodNotAllowedErr = 40501
    codeJSONMarshalErr      = 50001
)

// MyError contains custom code, error message, and HTTP status code.
type MyError struct {
    HTTPStatus int    `json:"-"`
    Code       int    `json:"code"`
    Message    string `json:"message"`
}

func (e *MyError) Error() string {
    return fmt.Sprintf("HTTPStatus: %v, Code: %v, Message: %q",
        e.HTTPStatus, e.Code, e.Message)
}

// WriteToResponse writes response for the error.
func (e *MyError) WriteToResponse(w http.ResponseWriter) {
    w.WriteHeader(e.HTTPStatus)
    fmt.Fprintf(w, e.ToJSON())
    // TODO: store e.ToJSON() to ElasticSearch for future analysis
}

// ToJSON returns JSON string for a MyError.
func (e *MyError) ToJSON() string {
    j, err := json.Marshal(e)
    if err != nil {
        return `{"code":50099,"message":"ScrapError.JSONStr: json.Marshal() failed"}`
    }
    return string(j)
}

// EmptyNameErr .
func EmptyNameErr(name string) *MyError {
    return &MyError{
        HTTPStatus: http.StatusBadRequest,
        Code:       codeEmptyNameErr,
        Message:    "name shoud not be empty",
    }
}

// InvalidAgeErr .
func InvalidAgeErr(age string) *MyError {
    return &MyError{
        HTTPStatus: http.StatusBadRequest,
        Code:       codeInvalidAgeErr,
        Message:    fmt.Sprintf("invalid age: %q", age),
    }
}

// MethodNotAllowedErr .
func MethodNotAllowedErr(method string) *MyError {
    return &MyError{
        HTTPStatus: http.StatusMethodNotAllowed,
        Code:       codeMethodNotAllowedErr,
        Message:    fmt.Sprintf("method %q is not allowed", method),
    }
}

// JSONMarshalErr .
func JSONMarshalErr(err error) *MyError {
    return &MyError{
        HTTPStatus: http.StatusInternalServerError,
        Code:       codeJSONMarshalErr,
        Message:    err.Error(),
    }
}

main.go :

package main

import (
    "encoding/json"
    "fmt"
    "log"
    "net/http"
    "strconv"
)

func main() {
    // request example:
    // curl -X POST -d 'name=John%20Doe&age=10' localhost:8083/persons
    http.HandleFunc("/persons", newPersonHandler)
    log.Fatal(http.ListenAndServe(":8083", nil))
}

func newPersonHandler(w http.ResponseWriter, r *http.Request) {
    w.Header().Set("Content-Type", "application/json; charset=utf-8")
    switch r.Method {
    case "POST":
        name, ageStr := r.FormValue("name"), r.FormValue("age")
        if name == "" {
            EmptyNameErr(name).WriteToResponse(w)
            return
        }
        age, err := strconv.ParseUint(ageStr, 10, 64)
        if err != nil {
            InvalidAgeErr(ageStr).WriteToResponse(w)
            return
        }

        resBody, err := json.Marshal(person{name, age})
        if err != nil {
            JSONMarshalErr(err).WriteToResponse(w)
            return
        }

        fmt.Fprintf(w, string(resBody))
    default:
        MethodNotAllowedErr(r.Method).WriteToResponse(w)
    }
}

type person struct {
    Name string `json:"name"`
    Age  uint64 `json:"age"`
}

Your code is fine, but don't use fmt.Fprintf if you don't need formatting.
It will scan your string for placeholders (like %s):
fmt.Fprintf(os.Stdout, "100%sure")
// 100%!s(MISSING)ure

Use Write directly:
os.Stdout.Write([]byte("100%sure"))
// 100%sure

And don't forget to handle returned error.
A bit more about io.Writer
Quote from documentation:

Writer is the interface that wraps the basic Write method.
Write writes len(p) bytes from p to the underlying data stream. It returns the number of bytes written from p (0 <= n <= len(p)) and any error encountered that caused the write to stop early. Write must return a non-nil error if it returns n < len(p). Write must not modify the slice data, even temporarily.

I guess that implementations of Write method are encouraged to write all data in a single call. If we digg the sources a bit we'll find, that under the hood most Write methods are implemented with loops.
On the other hand io.Reader may return with partial read and nil error, so methods like io.ReadFull and ioutil.ReadAll are quite handy.


Primitive full-text search on some properties of an object I am not a JS/TS Guru, so i wanted to ask if one could write this better.

I needed a very primitive Full-Text-Search of some properties of an Object. What do you think?

export const search = <T, K extends keyof T>(term: string, objects: T[], keys: K[]): T[] => {
  const foundObjects = objects.filter(x => {
    for (const key of keys) {
      if (x.hasOwnProperty(key) === false) {
        continue;
      }

      const val = x[key];

      if (typeof val === 'string' && val.toLowerCase().includes(term)) {
        return true;
      } else {
        continue;
      }
    }
    return false;
  });
  return foundObjects;
};

You have a lot of code that is not needed.

The two continue are not needed.
Rather than assign the variable foundObjects the filtered result just return the result directly.
The variable val that holds the property can be side stepped.
Then the hasOwnProperty can be combined with the if statement
Rather than use the for loop you can also use Array.some to find if any item matches the search

Thus you get

export const search = <T, K extends keyof T>(term: string, objects: T[], keys: K[]): T[] => {
    return objects.filter(x =>
        keys.some(key =>
            x.hasOwnProperty(key) &&
            typeof x[key] === 'string' &&
            x[key].toLowerCase().includes(term)
        )
    );
};


Counting the total value of Card instances given I am trying to write a function called countHand() that takes in an array of Card instances and counts the total value of the cards given.

The requirements are as follows:

The function returns the value of the cards in the hand as an Int.  It does not use loops or nested functions.
The card values are as follows:

Any Ace preceded by 5 of Diamonds is worth 100 points.
Any odd numeric card (3, 5, 7, 9) of any suit's worth the double of its rank value in points when immediately preceded in the hand by any card of the Hearts.

Examples:

The hand 3♥, 7♣ has total value of 14.
The hand 3♣, 7♥ has total value of 0.

Brevity/using one statement functions/closures/case statements are prefered.

Here is the current solution:

enum Suit {
    case Clubs, Diamonds, Hearts, Spades
}

enum Rank {
    case Jack, Queen, King, Ace
    case Num(Int)
}

struct Card {
    let suit: Suit
    let rank: Rank
}

/*
- Generate an array of tuples of 2 card subsequences by zipping the hand array with itself (minus the first element)
- Map over the array examining each pair for value-contributing combinations in a switch statement
- Sum all resulting values
*/
func countHand(hand: [Card]) -> Int {
    return Array(Zip2(hand, dropFirst(hand))).map{(card1: Card, card2: Card)->Int in
        switch (card1.suit, card1.rank, card2.rank) {
        case (.Hearts, _, .Num(let numRank)) where numRank % 2 == 1:
            return 2 * numRank
        case (.Diamonds, .Num(5), .Ace):
            return 100
        default:
            return 0
        }}.reduce(0, +)
}

countHand([
    Card(suit:.Hearts, rank:.Num(10)),
    Card(suit:.Hearts, rank:.Num(6)),
    Card(suit:.Diamonds, rank:.Num(5)),
    Card(suit:.Clubs, rank:.Ace),
    Card(suit:.Diamonds, rank:.Jack)
    ])// --> prints 110

I am wondering if there is room for improvement of this code, whether it would be brevity/execution speed/memory usage/CPU (extreme cases could be considered i.e a hand with a big number of cards).

First a minor note. It is less efficient to create a temporary Array from the result of Zip2. You can use the function map instead of the method map:

map(Zip2(hand, dropFirst(hand))) { ... }.reduce(0, +)

Second, I believe that having such a complex comment describing what your method does, is a sign that your code can be much more understandable. Usually a good replacement for a comment is a well named function. This is how I would implement your function:

func countHand(hand: [Card]) -> Int {
    func iterateTwoCardsAtATime(hand: [Card], block: (first: Card, second: Card) -> ()) {
        for (first, second) in Zip2(hand, dropFirst(hand)) {
            block(first: first, second: second)
        }
    }

    func scoreForConsecutiveCards(first: Card, second: Card) -> Int {
        switch(first.suit, first.rank, second.rank) {
            case (.Hearts, _, .Num(let numRank)) where numRank % 2 == 1:
                return 2 * numRank
            case (.Diamonds, .Num(5), .Ace):
                return 100
            default:
                return 0
        }
    }

    var sum = 0
    iterateTwoCardsAtATime(hand) { (first, second) in
        sum += scoreForConsecutiveCards(first, second)
    }
    return sum
}

Yes, this is definitely more verbose, but the code is much more clear and doesn't require a comment.

First, a new developer looks at the method and sees that there is a function called iterateTwoCardsAtATime. If they want to verify that it works as expected, they can, but most can assume it works and skip that implementation.

Then they get to scoreForConsecutiveCards. This contains all of the complicated scoring logic which is probably the biggest concern when evaluating this method. It is well contained and distinct from the other logic of the method. The other logic is very utilitarian while this logic is important business logic.

Finally, there is a simple understandable block of code written in English like language to sum up the results of all of the scores.

With modern IDEs with features like code folding, clarity is way more important than brevity.


Program that guesses your number using bitwise operations I got the idea for this program from this site.

functions.cpp:

#include <iostream>
#include <cstdlib>
#include <iomanip>

namespace my
{
    int getOneOrZero()
  {
    return (rand() >> 14);   // >> the bitwise operator
  }

// getArray leaves the numbers ( 0-99 ) that have in their bit position        represented by bigFlag the num ( 0 or 1 )

void printNums(int8_t bitFlag, int num)    // num is from getOneOrZero() so it's 1 or 0;
{
    for (int counter = 0; counter < 100; ++counter)  // there are quite a few implicit conversions to one byte integers
        {
            if ( (counter & bitFlag) != num*bitFlag )
                std::cout << std::setw(8) << " ";
            else
                std::cout << std::setw(8) << counter;

            if ( (counter % 9) == 0)
                std::cout << std::endl;
        }
}

char getAnswer()
{
    while (1)
    {
        std::cout << "\n Is your number shown above ?\n\n 'y' for yes , 'n' for no , 'r' for reset : ";
        char answer;
        std::cin >> answer;
        std::cin.ignore(32767,'\n');

        if (std::cin.fail())
            std::cin.clear();

        if (answer == 'y' || answer == 'n' || answer == 'r')   // I could use switch but nevermind
            return answer;
    }
}

void turnsPassed(int turns)
{
    std::cout << "\n This is turn " << turns << "\n\n";
}

int swapZeroOrOne(int num)
{
    switch (num)
    {
    case 0 :
        return 1;
    case 1 :
        return 0;
    default:
        std::cout << "\nSwapZeroOrOne ERROR !\n";
        break;
    }
}

int getUpdateForGuessNum(int8_t flag, int num, char answer)
{
    switch (answer)
    {
    case 'n' :
        { int newNum = swapZeroOrOne(num);
          return newNum*flag;
        }
    case 'y' :
        return num*flag;
    case 'r' :
        break;
    default  :
        std::cout << "\n ERROR ! In getUpdateForGuessNum\n";
        break;
    }
  }
}

main.cpp:

#include <iostream>
#include <iomanip>
#include <cstdlib>
#include <ctime>
#include "functions.hpp"
#include "constants.hpp"
#include <stdlib.h>      // for system() commands >.<
int main()
{
srand( static_cast<unsigned int>(time(0)));

Reset :      // goto !
system("cls");

int guessNum = 0;

// guessing loop !

for (int counter = 0; counter < 7; ++counter)
{
    if (counter == 0)
        std::cout << "\n Think of a number between 0 to 99\n";

    my::turnsPassed(counter + 1);  // +1 cause counter starts from 0

    int num {my::getOneOrZero()};  // this gives a randomness to the numbers shown each time you run the programm

    my::printNums(myVar::bitFlag[counter],num);

    char answer { my::getAnswer() };

    if (answer == 'r')
        goto Reset;

    guessNum |= my::getUpdateForGuessNum(myVar::bitFlag[counter],num,answer);

    system("cls");
}

std::cout << "\n Your number is \n";
std::cout << "\n" << std::setfill('-')  << std::setw(81) << "\n";
std::cout << std::setfill(' ') << std::setw(41) << guessNum << "\n";
std::cout << "\n" << std::setfill('-')  << std::setw(81) << "\n";

system("pause");  // I should fix this

return 0;
}

functions.hpp:

#pragma once

namespace my
{
    int getOneOrZero();
    void printNums(int8_t,int);
    char getAnswer();
    void turnsPassed(int);
    int getUpdateForGuessNum(int8_t,int,char);
    int swapZeroOrOne(int);

}

constants.hpp:

#pragma once

namespace myVar
{
    const int8_t bitFlag[] {0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80};
}

Please suggest ways to improve this program. Explain the suggestions you make because I need to know why I should change something.

Here are some things that may help you improve your code.

Understand type implications

The code includes a bitFlag array that is declared as const int8_t but then contains a value that is 0x80.  The problem with that is that when the compiler encounters the constant 0x80, it converts it into an int by default, and so that would be the value 128.  However, 128 is not representable in an int8_t.  That bit pattern is actually -128 as an int8_t, so I'd recommend either using a different type (such as uint8_t) for the variable or writing -0x80 for the constant which is correct, but a little strange looking.

Know the standard types

This function is in the current code:

int getOneOrZero()
{
    return (rand() >> 14);   // >> the bitwise operator
}

There are two problems with this.  The first is that it apparently assumes that an int is 16 bits.  However, on my machine, an int is 64 bits.  In general, you can't assume that the size of an int is a fixed size.  The standard only says (implicitly from the required range) that it must be 16 bits, but it may be larger.  The second problem is addressed in the next point.

Consider using a better random number generator

Because you are using a compiler that supports at least C++11, consider using a better random number generator.  In particular, instead of rand, you might want to look at std::bernoulli_distribution and friends in the <random> header.

Here's one way to rewrite it:

int getOneOrZero()
{
    static std::mt19937 gen{std::random_device{}()};
    static std::bernoulli_distribution bd;
    return bd(gen);
}

Ensure every control path returns a proper value

The swapZeroOrOne routine returns 1 or 0 under some set of conditions but then doesn't return anything at all otherwise (although it prints an error essage).  This is an error because all control paths should return a value.  Since it's only used once, and because it can be trivially rewritten, I'd probably replace this:

case 'n' :
    { int newNum = swapZeroOrOne(num);
      return newNum*flag;
    }

with this:

    return (1-num)*flag;

Avoid using goto

Having a goto statement in modern C++ is usually a sign of bad design.  Better would be to eliminate them entirely -- it makes the code easier to follow and less error-prone.  In this code, it's probable that you could use a loop instead:

for (bool playing=true; playing;  )
    // code to play the game
    // ask user if they want to play again
    playing = answer == 'r';
}

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause").  The first is that it is not portable to other operating systems which you may or may not care about now.  The second is that it's a security hole, which you absolutely must care about.  Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything.  First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system.  Then rewrite the contents of those functions to do what you want using C++.  For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    std::cout << "\x1b[2J";
}

Think of the user

If the user were to get an error message that said:

  ERROR ! In getUpdateForGuessNum

What use is that unless they also happen to be the author of the code?  Instead, for errors that are an indication of a program flaw, I'd use assert or for things that are unusual but still should be accomodated by the program, use an exception.

Use objects

You have a guessNum and counter to support the game and then separate functions printNums and getAnswer, etc. that operate on guessNum. With only a slight syntax change, you would have a real object instead of C-style code written in C++.  You could declare a GuessNum object and then printNums, getAnswer, etc. could all be member functions.

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Code style: Should prints be high or low level? Where should i put print statements describing the flow of the program? Is there any convetion or code style for this?
Should they be in higher level functions?
def create_collage():
    print('finding images ...')
    image_paths = get_image_paths()

    print('loading images ...')
    images = load_images(image_paths)

    print('assembling output image ...')
    output = assemble_output(images)

    ...

Or in lower level functions?
def get_image_paths():
    print('finding images ...')
    ...

def load_images(image_paths):
    print('loading images ...')
    ...

def assemble_images(images):
    print('assembling output image ...')
    ...

```

This is a basic but important question.
Most functions can be arranged to operate purely in the realm of computation:
they take data as input, return data as output, and have no side effects in the
"real world". Examples of side effects would include interacting with the file
system or a database, getting user input, and printing.
Basically anything that exists outside of the tidy confines of the computer's
"brain".
Functions with side effects tend to have trickier failure scenarios and they
are harder to test in an automated way (for example, with typical unit testing
frameworks) without jumping through various hoops.
That's why most people advise you to aim for a design where side effects like
printing end up in a thin outer layer of your program (for example, in main()
or a close sibling) rather than in lower-level, computation-heavy functions.
Keep as much of your code base as you can in the realm of pure computation.
Gary Burnhardt's talk on this
subject is excellent.
Of course, every best practice has its exceptions. Don't become a slave to
abstract rules. For example, programs that are designed to run persistently in
response to requests (such as a web service) might have very strong reasons to
want printing in many locations -- for debugability, auditing, and
other reasons. In those situations, you definitely want to use a logging
framework. Those frameworks have configuration options that allow them to play
nicely with automated testing and other development needs. But even in that use
case where we have decided to allow side effects deeper than the top level, the
same general principles apply: put the logging as high in the call stack as you
can, consistent with other needs. For example, in a classic web service, my
default would be to put logging in the methods directly handling incoming
requests, not in various utility/helper functions than can be focused purely on
data-oriented computation. Drawing those lines is a judgment call, of course.
And, as shown in Burnhardt's demo, sometimes initial judgements are incorrect,
and a design can be refactored without too much difficulty to narrow the
proportion of a code base that deviates from pure data-oriented computation.
The main point is to avoid casually scattering side effects across your code
base. Allow side effects only when you don't have an alternative. And if you
can wrap the side effect in a tool designed to lessen the costs (such as a
logging framework) do that as well.


Advanced multiple criteria filter VBA This code is working on the user selection when pressed a button the table will be filtered by the number of criteria the user selected:

    Sub advancedMultipleCriteriaFilter()

 Dim cellRng As Range, tableObject As Range, subSelection As Range
 Dim filterCriteria() As String, filterFields() As Integer
 Dim i As Integer

Application.ScreenUpdating = False
'Call removeSpace

If Selection.Rows.Count > 1 Then
 MsgBox "Cannot apply filter to multiple rows within the same column. Please make another selection and try again.", vbInformation, "Selection Error!"
 Exit Sub
 End If

i = 1
 ReDim filterCriteria(1 To Selection.Cells.Count) As String
 ReDim filterFields(1 To Selection.Cells.Count) As Integer

Set tableObject = Selection.CurrentRegion
 For Each subSelection In Selection.Areas
 For Each cellRng In subSelection
 filterCriteria(i) = cellRng.Text
 filterFields(i) = cellRng.Column - tableObject.Cells(1, 1).Column + 1
 i = i + 1
 Next cellRng
 Next subSelection

With tableObject
 For i = 1 To UBound(filterCriteria)
 .AutoFilter field:=filterFields(i), Criteria1:=filterCriteria(i)
 Next i
 End With

Call GetLastRow

Set tableObject = Nothing
Application.ScreenUpdating = True

End Sub

Sub resetFilters()

Dim sht As Worksheet
Dim LastRow As Range

Application.ScreenUpdating = False

   On Error Resume Next
        If ActiveSheet.FilterMode Then
    ActiveSheet.ShowAllData
  End If

'Range("A3:T3").ClearContents
Application.ScreenUpdating = True
Call GetLastRow

End Sub

Private Sub GetLastRow()

 'Step 1: Declare Your Variables.
  Dim LastRow As Long

 'Step 2: Capture the last used row number.
  LastRow = Cells(Rows.Count, 8).End(xlUp).Row

 'Step 3: Select the next row down
  Cells(LastRow, 8).Offset(1, 0).Select

 End Sub

Private Sub GetLastRow()

I like that it's Private. Other than that... a Sub doesn't "get" anything; a Sub does something - this is a side-effecting procedure with the lipstick of a Function. A pig with lipstick is still a pig though. Let's look at how it's implemented:

 'Step 1: Declare Your Variables.
  Dim LastRow As Long

 'Step 2: Capture the last used row number.
  LastRow = Cells(Rows.Count, 8).End(xlUp).Row

 'Step 3: Select the next row down
  Cells(LastRow, 8).Offset(1, 0).Select

These comments are noise. Comments that say what the code does are useless clutter, unless the code is absolutely unreadable... in which case the solution is to improve the code's readability. Good comments say why, not what. Bad comments should be removed.

There are a number of serious issues with that procedure. Cells and Rows both implicitly refer to the active worksheet, which means if the user happens to activate another sheet while the code is running, everything falls apart (I see you've addressed that elsewhere though) - it also falls apart if other code activates another sheet than the one that's expected to be active though. And given how your entire code [ab]uses Select and Activate, that's a real concern.

This would be my GetLastRow:

Private Function GetLastRow(ByVal sheet As Worksheet, Optional ByVal fromColumn As Long = 1) As Long
    GetLastRow = sheet.Cells(sheet.Rows.Count, fromColumn).End(xlUp).Row
End Function

No side effects. Returns ("gets") a value. Says what it does, does what it says: it returns the last row; a better name for a side-effecting procedure that selects the first empty row could be SelectFirstNonEmptyRowInColumnH. Too specific? It's exactly what it does though. The problem isn't the specific name - the problem is what the procedure does.

The benefits of using non-side-effecting functions are numerous: first, I can call my function off any worksheet (not just the active one), make it look at any column, and I can call it 20 times in a row on 20 different sheets, not once will my Application.Selection be affected.

It also makes code that reads exactly like what it does:

Call GetLastRow

Regardless of the Call keyword being useless here (or anywhere else - it's been obsolete since the advent of the implicit procedure call syntax... over 20 years ago), reading the above line of code has a very different feel than reading this one:

sheet.Cells(GetLastRow(sheet, 8), 8).Offset(1).Select

More complicated? Ok then, let's split it up:

Const importantColumn As Long = 8

Dim lastRow As Long
lastRow = GetLastRow(sheet, importantColumn)

sheet.Cells(lastRow, importantColumn) _
     .Offset(1) _
     .Select

By not hiding .Select operations behind a side-effecting procedure call [disguised as a function], we make it clear that we're tampering with the workbook's state.

There's only ever ONE reason to use .Select in VBA code: when you want to set the current selection for the user.

Using .Select for any other reason, is asking for your code to be as slow as Excel VBA code can get, not to mention extremely frail and bug-prone.

Working off Selection is dangerous. In fact, your code can very easily blow up on the 2nd executable statement:

If Selection.Rows.Count > 1 Then

How? You assume Selection is a Range. But Selection can be a Shape. Selection can be a Chart, or a Series. Selection can be Nothing. Each of these make execution jump to your ErrorHandler subroutine, to display a cryptic error message in the debug/immediate pane, that doesn't tell anything about what's going on, where, or why:

  Error number: 91 Object variable or With block variable not set.

Or

  Error number: 438 Object doesn't support this property or method.

But programmers making assumptions (Selection is a Range?) are a dangerous thing: you'll likely never encounter these error messages, because when you test your macro yourself, Selection will be a Range. Things get more fun when it's an actual user running your code though. Users are particularly bad at wording useful bug reports - most of the time they sound like a downvoted Stack Overflow question that's accumulating "unclear what you're asking" close votes:

  I clicked your macro button but nothing happens, it's not working.

Good luck figuring that one out!

Instead, you could separate responsibilities, increase the level of abstraction, and have smaller procedures that do one thing - and have few reasons to fail. Then you could include the procedure's name in your Debug.Print statement, and at least you'd know which step blew up.

I'd review the actual crux of the code, but the [lack of] indentation makes my head spin, so instead I'll point you to Rubberduck and its Smart Indenter tool (disclaimer: I manage this open-source project; credits to @Comintern for most of the work on the Smart Indenter port), which can help you fix this.

Please read: How to avoid using Select in Excel VBA macros.


Speed up long running Ruby code I'm running code which works fast locally, but when it's on Heroku it's slow. Heroku says this is because there is less memory and CPU power on my hosting than locally. I'm trying to rewrite my code so it's more efficient, but I'm not sure how.

This is what they said:

  ...[my] method (especially, a.each part) is taking time. I think a
  itself is pretty long, and you're transpose it with subs.keys, and do
  some process for each of them. Even though s.gsub!(*pair) is not "long
  running" code, if you run this million times (actually runs 1,161,216
  times for the word "startup"), it'll be "slow" request.

Here is my code:

def substitute(word)
    subs = Hash.new
    Section.all.to_a.each do |s|
        alternates_array = Array.new
        s.substitutions.each do |alt|
            alternates_array << alt.alternate
        end
        alternates_array << s.part
        new_hash = Hash.new
        subs[s.part] = alternates_array
    end
    a = subs.values
    a = a.first.product(*a.drop(1))
    alternates = Array.new
    a.each do |a|
        new_string = [subs.keys, a].transpose.each_with_object(word.dup){|pair, s| s.gsub!(*pair)}
    end
    return alternates
end

Section Model

class Section
  include Mongoid::Document
  field :part, type: String

  embeds_many :substitutions

  accepts_nested_attributes_for :substitutions
end

Substitution Model

class Substitution
  include Mongoid::Document
  field :alternate, type: String

  embedded_in :section
end

Read with batches
Looping over Section.all.to_a is very memory greedy (it reads everything to memory before starting to process it). You will be far better off using batches:

Section.find_each do |section|
  # ...
end

Naming
Using member names like s, a, etc. is not very readable, and makes understanding what your code does very difficult. For that reason, it is very hard for us reviewers to give you an intelligent advice on how your code can be improved - we just don't understand what it does...

Shadowing
a.each do |a| - to add insult to injury, you shadow the a member when you loop over it. This makes your code totally unreadable. I still don't understand what that loop is all about.

Loop bloat
The line a = a.first.product(*a.drop(1)) is very suspicious, since it bloats the amount of data your are looping on by several scales. Are you sure that is what you want to do?

Where does your data go?
The new_string, which holds the result for each iteration is not seemed to be saved anywhere...

Filter your data
At the bottom line, you are substituting words in your input. Not all sections exist in all inputs, but you search for them over and over again. You might save a lot of calculations if you filter irrelevant sections in your code, before looping on them:

Section.find_each do |section|
  next unless word[section.part]
  alternate_array #...
end

Doing stuff over and over again
I'm pretty sure, though I might be wrong, that at the end s.gsub!(*pair) ends up running the same "pairs" more than once on the same string (or its duplicates). It will be better strategy to cache results, or maybe pre-process your data (build a template, and only set the variables, or something).

Edit
You could use a variant of gsub! which receives a hash as the replacement:

  If the second argument is a Hash, and the matched text is one of its
  keys, the corresponding value is the replacement string.

So you could write:

replace_regexp = Regexp.union(subs.keys)

replaced_words = a.map do |alts|
  word.gsub(replace_regexp, Hash[[subs.keys, alts].transpose])
end

This does a single gsub per combination (instead of once per key), and further reduces the run complexity of your code.


HackerRank: Equal Stacks This is my solution to the Equal Stacks problem on HackerRank. I thinks it's really messy and slow. could you give me some thoughts about how to optimize given code and algorithm(i'm pretty sure problem can be solved way effectively)

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

public class Solution {

    public static void main(String[] args) {
        Scanner inputStream = new Scanner(System.in);

        int n1          = inputStream.nextInt();
        int n2          = inputStream.nextInt();
        int n3          = inputStream.nextInt();

        int n1Height    = 0;
        int n2Height    = 0;
        int n3Height    = 0;

        int h1[]        = new int[n1];
        int h2[]        = new int[n2];
        int h3[]        = new int[n3];

        for(int h1_i=0; h1_i < n1; h1_i++){
            h1[h1_i] = inputStream.nextInt();
            n1Height += h1[h1_i];
        }
        for(int h2_i=0; h2_i < n2; h2_i++){
            h2[h2_i]  = inputStream.nextInt();
            n2Height += h2[h2_i];
        }
        for(int h3_i=0; h3_i < n3; h3_i++){
            h3[h3_i]   = inputStream.nextInt();
            n3Height  += h3[h3_i];
        }
        int highestHeight = 0;
        for(int i=0; i< n3; i++){
            if(heightReachable(n3Height,h2,n2Height) && heightReachable(n3Height, h1, n1Height)){
                highestHeight = n3Height;
                break;
            }
            n3Height -= h3[i];
        }

        System.out.println(highestHeight);

        inputStream.close();
    }
    /**
     * [heightReachable - returns boolean identifying whether or not given      height is reachable by given stack]
     * @param  height      [height which we should check if is reachable or not]
     * @param  stackBlocks [stack blocks array containing height of each block]
     * @param  stackHeight [number of blocks in given stack]
     * @return             [true if height is reachable by removing zero or more elements from the stack and false otherwise]
     */
    private static boolean heightReachable(int height, int[] stackBlocks, int stackHeight){
        for(int i=0; i < stackBlocks.length; i++){
            if(stackHeight == height) return true;
            stackHeight = stackHeight - stackBlocks[i];
        }
        return false;
    }
}

Method decomposition

The main method reads and parses the input,
and does a large part of the calculation logic too.
It would be better to move the calculation outside.
That is, after the input is read and parsed,
it will be better to leave just this in main:

System.out.println(findEqualHeight(h1, h2, h3));

And all the calculation logic can be in findEqualHeight.

Simplify the algorithm

The algorithm is a bit complicated and hard to understand.
It's also inefficient.
As you knock off cylinders from the 3rd stack,
you check if you can reduce the others from the top.
This can lead to recalculating the sums multiple times.

Consider this alternative implementation:
knock off cylinders from the highest stack,
until they are all of the same height:

static int findEqualHeight(int[] h1, int[] h2, int[] h3) {
    int sum1 = sum(h1);
    int sum2 = sum(h2);
    int sum3 = sum(h3);

    int i1 = 0;
    int i2 = 0;
    int i3 = 0;

    while (true) {
        if (sum1 > sum2 || sum1 > sum3) {
            sum1 -= h1[i1++];
        } else if (sum2 > sum1 || sum2 > sum3) {
            sum2 -= h2[i2++];
        } else if (sum3 > sum1 || sum3 > sum2) {
            sum3 -= h3[i3++];
        } else {
            break;
        }
    }

    return sum1;
}

static int sum(int[] arr) {
    return IntStream.of(arr).sum();
}