Electrical engine calculations The ElectricalEngine class responds to the horsepower message. Because efficiency is calculated in percent a programmer can mistakenly initialize it with an integer instead of a float.

class ElectricalEngine
  attr_reader :volts, :current, :efficiency

  def initialize(volts, current, efficiency)
    @volts, @current, @efficiency = volts, current, efficiency
  end

  HP_IN_WATTS = 746
  def horsepower
    (volts * current * efficiency) / HP_IN_WATTS
  end
end

puts ElectricalEngine.new(240, 90, .6).horsepower # correct
puts ElectricalEngine.new(240, 90, 60).horsepower # buggy

How would you handle this scenario?

Do nothing. It's the programmers responsibility to know the right datatype.
Rename efficiency to efficiency_as_float to make it clearer.
Rename efficiency to efficiency_as_percent and adjust horsepower's calculation.
Write a custom efficiency method to check the datatype and convert it accordingly.
Check efficiency type and raise an error if it's not a float.
Other

Solution four might look like this. Of course this conversion can happen in the initialize method too, but I think this is cleaner.

def horsepower
  (volts * current * efficiency_as_float) / HP_IN_WATTS
end

def efficiency_as_float
  if efficiency.is_a?(Integer) # what if 1 is passed in instead of 1.0?
    efficiency / 100.to_f
  else
    efficiency
  end
end

Solution 5 would look something like this:

def initialize(volts, current, efficiency)
  raise "Efficiency must be a float" unless efficiency.is_a?(Float)
  @volts, @current, @efficiency = volts, current, efficiency
end

Should ElectricalEngine own the responsibility of converting incorrect datatypes?

my two cents : use duck-typing.

@efficiency = efficiency.to_f

... if it quacks like a float, then it is a float. This allows to leverage ruby's awesomeness with things like :

class EfficiencyProfile
  def initialize(some_data, value)
    @data = some_data
    @efficiency_value = some_value
  end

  def to_f
    @efficiency_value.to_f
  end
end

e = EfficiencyProfile.new(some_big_chunk_of_data_about_engine_performance, 42)
ElectricalEngine.new(1,2,e) # would work without a complain

Dynamic typing sure is dangerous, but you have to embrace it if you want to benefit from it.

Checking the type explicitly is an half-arsed solution, because :

it is cumbersome, and you will never be as good at it than, say, a C compiler is
it will completely remove the sole benefit from dynamic typing : flexibility.


Printing fizzy lines Challenge
Given a test case print out a FizzBuzz Series.
Specifications

The first argument is a path to a file.
Each line includes a test case.
Each test case is comprised of three spaced delimited integers.

The first two integers are the dividers X and Y.
The third integer, N is how far to count.

Print out the series 1 through N, replacing numbers divisible by X with F, numbers divisible by Y with B and numbers divisible by both with FB.

Constraints

The input file is formatted correctly.
The numbers are valid positive integers.
X is in range [1, 20]
Y is in range [1, 20]
N is in range [21, 100]
Output should be one line per set with no trailing or empty spaces.

Input Sample

3 5 10
2 7 15

Output Sample

1 2 F 4 B F 7 8 F B
1 F 3 F 5 F B F 9 F 11 F 13 FB 15

Source
My Solution:
#include <stdio.h>

void print_buzzified(int fizz, int buzz, int count) {
    for (int i = 1; i <= count; i++) {
        if (i % fizz == 0 && i % buzz == 0) {
            printf("%s", "FB");
        } else if (i % fizz == 0) {
            printf("%s", "F");
        } else if (i % buzz == 0) {
            printf("%s", "B");
        } else {
            printf("%d", i);
        }

        printf(i < count ? " " : "\n");
    }
}

int main(int argc, const char * argv[]) {
    FILE *file;
    if (argc < 2 || !(file = fopen(argv[1], "r"))) {
        puts("No argument provided / File not found.");
        return 1;
    }

    file = fopen(argv[1], "r");
    int fizz;
    int buzz;
    int count;

    while (!feof(file)) {
        fscanf(file, "%d %d %d", &fizz, &buzz, &count);
        print_buzzified(fizz, buzz, count);
    }

    fclose(file);
}

Design:

Good job on setting up everything in main(), then passing off control to another function.
Declare and initialize file after checking the command line arguments.
Initialize fizz, buzz, and count.
Don't use !feof(file) to control your loop.  See this answer for more details.
Check the return value of fscanf() to make sure we're reading good data.
Good job remembering to close the file.

Readability & Maintainability/Performance

I'm grouping these two categories together for this review, since they happen to go hand in hand.

Your for loop can be refactored down a bit:

if (i % fizz == 0) fputc('F', stdout);
if (i % buzz == 0) fputc('B', stdout);
if (i % fizz && i % buzz)) fprintf(stdout, "%d", i);

Some people may find issue with the duplicated check on i with fizz and buzz, but we cut out some conditional branches doing this and the more branches we cut, the less susceptible to branch prediction we are.  There may be a better way to write this, but I can't think of it right now.
Note how I used fputs() and fputc() in the code above instead of printf().  This is because those functions are \$ O(1) \$ operations instead of \$ O(n) \$ to loop over the string and format it.  This may speed up your program by a hair, if that, but the main reason I recommend this change is to make clear we aren't modifying any strings.
Since the range of \$ X \$, \$ Y \$, and \$ N \$ is 1-20, it would be good to change the type representing these variables to something a bit more compact in memory.

Depending on what we want to optimize for (memory or performance), I would recommend either uint_least8_t or uint_fast8_t.

uint_least8_t: give me the smallest type of unsigned int which has at least 8 bits. Optimize for memory consumption.
uint_fast8_t: give me an unsigned int of at least 8 bits. Pick a larger type if it will make my program faster, because of alignment considerations. Optimize for speed.


Suggestions for optimising this front end code I've stumbled across some UI (React.js) code that I've worked out to have a time complexity of O(n^4) (I hope that's right, if not please do correct me). I was interested in how one might go about optimising something like this.

I know that for() loops do perform slightly better than array.forEach() and I'd assume const uses a bit less memory than let due to not needing to support re-allocation.

Would using functional operators like map and filter improve performance?

I'd be really interested to see some suggestions to this, and maybe the ideal optimisation for it?

The code:

animate () {
  let movingScreens = {}
  if (this.animationsDisabled()) {
    movingScreens = {}
  } else {
    for (let i in scrollAnimationDefs.movingScreens) {
      if (i !== 'appsTop' && i !== 'appsBottom') movingScreens[i] = scrollAnimationDefs.movingScreens[i]
    }
  }

  for (let shape in movingScreens) {
    if (movingScreens[shape].scrolls) {
      for (let scroll of movingScreens[shape].scrolls) {
        const from = scroll.from * this.scrollMax
        const to = scroll.to * this.scrollMax
        if (from <= this.scrollTop && to >= this.scrollTop) {
          const styles = scroll.styles(
            (this.scrollTop - from) / (to - from)
          )

          for (let style in styles) {
            let newStyle = styles[style]

            const els = scrollAnimationDefs.movingScreens[shape].els.map(
              ref => {
                if (ref === 'screen1IfContainer') return this.animations.screen1IfContainer.current.container.current
                return this.animations[ref].current
              }
            )

            for (const i in els) {
              if (els[i]) {
                els[i].style[style] = newStyle
              }
            }
          }
        }
      }
    }
  }
}

Style and code

There are several places where you should use const rather than let. Mainly inside the for loops.
The second movingScreens = {} is redundant.
Avoid nesting loops too deeply. Use a function to avoid this.
Don't use for...in as you need to be sure you are not getting properties higher up the prototype chain. Use Object.keys to get the keys and iterate them with a for...of
Avoid long lines by using alias to reference object paths or long names.
The naming is confusing, you have shapes, screens, scrollScreens that seam to be interchangeable
Is this a typo or a really bad name and reference this.animations.screen1IfContainer.current.container.current

Avoid iterating again

You build the object movingScreens then you iterate it again. Would be better to process the items instead of building the object movingScreens.

You use Array.map to create an array of elements, then you iterate that array again. Process the elements as you iterate the first time.

Rewrite

Removed the object movingScreens processing the shapes in the first loop.
Added function scrollScreen to process a (screen, shape, or scroll I can not be sure).
Used aliases to reference long names and paths. currentCont, and screens
All variables are now const
Used Object.keys for safer key iteration.

As there is no way for me to test this code, nor do I have a context (what is what), it may contain typos or logic errors.

animate() {
    const currentCont = this.animations.screen1IfContainer.current.container.current;
    const scrollScreen = shape => {
        for (const scroll of shape.scrolls) {
            const from = scroll.from * this.scrollMax;
            const to = scroll.to * this.scrollMax;
            if (from <= this.scrollTop && to >= this.scrollTop) {
                const styles = scroll.styles((this.scrollTop - from) / (to - from));
                for (const style of Object.keys(styles)) {
                    for(const ref of shape.els) {
                        const el = ref === 'screen1IfContainer' ?
                            currentCont : this.animations[ref].current;
                        if (el) { el.style[style] = styles[style] }
                    }
                }
            }
        }
    }
    if (!this.animationsDisabled()) {
        const screens = scrollAnimationDefs.movingScreens;
        for (const key of Object.keys(screens)) {
            if (key !== 'appsTop' && key !== 'appsBottom' && screens[key].scrolls) {
                scrollScreen(screens[key]);
            }
        }
    }
}

Questions

  \$O(n^4)\$ (I hope that's right, if not please do correct me)

You have not defined n thus its is meaningless to define the complexity.

  Would using functional operators like map and filter improve performance?
  I'd be really interested to see some suggestions to this, and maybe the ideal optimisation for it?

As this is animation these points can not be answered. The slow point is the moving of pixels, this is highly dependent on the device it is running on. The JS code is but a fraction of the workload, you can optimize it till the cows come home and it may not make a spec of difference on the performance of the animation.

Any optimization would require the full animation to see when and where the many GPU  state changes, compositing, and what not can be optimized.

Update

I forgot this line in your question.

  I'd assume const uses a bit less memory than let due to not needing to support re-allocation

If const uses less memory that would be up to the JS engine. I would not use it as a criteria as when to use const or not.

We use const to protect against accidental mutation.
We use let and const to avoid accidentally redefining a variable.
We use const (as with let) to scope the variable to a block. A block is code between { and } The exception is when a variable is defined in a for, do,  or while loop as a const or let it is scoped to the loop block, not to the current block. eg { /*current block a is not in scope */ for(const a of b) { /* a is in scope  loop block*/ } }
We use const to give meaning to constants that are otherwise just numbers or strings in your source code. eg const unit = 1;
Currently chrome, FF, edge give const, let, and var have a slight performance benefit over literals.  eg 1 + 1 is a little slower than one + one with one defined as const one = 1;

Do not rely on this to remain true as engines are constantly changing. The benefit is very small and only an advantage under very specific conditions.


Pick a random file from a directory tree I'm working on a python module to randomly choose a wallpaper for my desktop background among thousands of pictures in my photo library.

I though I'd make a general function:

get_random_file(ext, top)

  Return the name of a random file in a directory tree with top as root.

Since I'm dealing with a very large directory tree, listing all possible files would be very time consuming (edit: see comments on C. Harley answer). So I went for heuristic method, making sure all files are covered and adding an upper bound in case there are no files with given extension. There are occasionally false negatives, which is not a too big deal, but most importantly, it runs nearly instantly.

Algorithm is simple:

Get a list of directories, including .
Pick a random directory from that list.
If the picked directory is . (either randomly, or only available option), then retrieve a list of files in that directory.
If the list of files is empty, go back to 1.
Return a random file from that list.

Note that this algorithm can go many levels deep into sub-directories.

So far I'm happy with performance and returned values. It is able to quickly pick up a file in a very large tree.

However, I'm after general comments in terms of performance and also code style.

What I'm not particularly proud of in my code is the use of next(os.walk(top))[1] to get the list of sub-directories in the current directory.

There are also false negatives, when asking for a file extension with not many occurrences in a tree with many directories. However, I don't want to increase the limit too much to avoid waiting when really there are not such files in the tree.

The code:

# -*- coding: utf-8 -*-
"""
Created on Wed Aug 29 14:05:27 2018
@author: raf
"""
import os
import sys
import glob
import random

def get_random_files(ext, top):
    '''Return the name of a random file within a top path.

    Works recursivelly in subdirectories.

    Note that a full list of files could be got with
        glob(top + '/**/*.' + ext, recursive=True)

    However that would be extremely slow for large directories.
    '''
    _top = top
    ct, limit = 0, 50000
    while True:

        if ct > limit:
            return 'No file found after %d iterations.' % limit
        ct += 1

        try:
            dirs = next(os.walk(top))[1]
        except StopIteration:  # access denied and other exceptions
            top = _top
            continue

        i = random.randint(0, len(dirs))
        if i == 0:  # use .
            files = glob.glob(top + '/*.' + ext)
            if not files:
                top = _top
                continue
            i = random.randint(0, len(files)-1)
            return files[i]

        top += '/' + dirs[i-1]

if __name__ == '__main__':
    if len(sys.argv) > 1:
        print(get_random_files(sys.argv[1], os.getcwd()))
    else:
        print(get_random_files('*', os.getcwd()))

If we rewrite your code to take advantage of Pathlib, we get the following:

import os
import sys
import random
from pathlib import Path

def get_random_files2(ext, top=os.getcwd()):
    file_list = list(Path(top).glob(f"**/*.{ext}"))
    if not len(file_list):
        return f"No files matched that extension: {ext}"
    rand = random.randint(0, len(file_list) - 1)
    return file_list[rand]

if __name__ == '__main__':
    extension = sys.argv[1] if len(sys.argv) > 1 else "*"
    print(get_random_files2(extension))

I appended a "2" to the function name in my example just to make it easier to copy/paste if you want.
The benefits of using Pathlib is that we can glob directly from the path object, and it will recursively get a list of files throughout the tree.

I noticed your code only works with os.getcwd() so there was no point in injecting two parameters into get_random_files. However, I left the function signature the same in case you want to enhance it later.

The looping seemed out of place when (a) you want to check for files (b) you want a random file. So, the code gets the files or returns "No files found", and if it finds some files, picks one of them from the list of files.
Please take some time to understand the approach I used. The biggest take-aways you can have are:

Infinite loops are bad (the while True:) unless you really, really need them (because you don't want to write event-based code or you just want to block),
Looping is always slow,
Modifying variables inside loops is bad (your construction/approach is faulty when you modify variable content when those variables control or affect the functionality or processing of what your code is actually doing),
if you're going to use random values, it's best to understand why you're doing that. If it's to pick from a list, ensure you populate the list first, don't rely on the random value to dictate your actions.
Look at keeping functions to 4 or so lines - because this will ensure you adhere to the Single Responsibility principal, which makes your code better in the long run (less support and maintenance).

Otherwise, a good attempt at automation. Keep it up and post more of your code when you have it.


Writing defaultdict to CSV file I have a defaultdict being written to file like this:

writer = csv.writer(open(locationToSaveFile, 'wb+'))
for k,v in dict.iteritems():
    writer.writerow([k ,v])

I then have this horribly convoluted text to read it back in:

myDict = defaultdict(list)
with open(fileLocation, 'rb') as test:
    reader = csv.DictReader(test, ['processedText', 'date'])

    for line in reader:
        myDict[line['processedText']].append(line['date'])

textDict = defaultdict(list)

for processedText, date in myDict.items():
    actual_list = ast.literal_eval(date[0])
    for a in actual_list:
        textDict[processedText].append(a)

for processedText, date in textDict.items():

Obviously this looks very wasteful (and looks horrible!) but I don't know how else to import the defaultdict.

When opening it with the reader = and for line in reader lines it creates a list within a list. The only way out of this is to use ast to convert it.

Could somebody advise me how to tidy this up?

Note: I'm very unfamiliar with Python

csv is meant to store rows of data, not a simple dictionary. You can use json for that:

import json

# writing
json.dump(yourdict, open(filename, 'w'))

# reading
yourdict = json.load(open(filename))


AVL tree implementation in C I have an assignment for which I need to write an AVL tree. This is what I have written so far. It works on all of my tests, but suddenly fails in checking system with TL (time limit exceeded).

Personally I think there could be a bug with input data in test (although I have already solved this problem with Cartesian tree).

struct node
{
    int key;
    int data;

    int height;

    struct node* left;
    struct node* right;
};

typedef struct node node;

node* new_node(int key, int data)
{
    node* p = malloc(sizeof(*p));

    p -> key    = key;
    p -> data   = data;
    p -> height = 1;
    p -> left   = NULL;
    p -> right  = NULL;

    return p;
}

int max(int a, int b)
{
    return a > b ? a : b;
}

int height(node* p)
{
    return p ? p -> height : 0;
}

void recalc(node* p)
{
    p -> height = 1 + max(height(p -> left), height(p -> right));
}

node* rotate_right(node* p)
{
    node* q = p -> left;

    p -> left = q -> right;
    q -> right = p;

    recalc(p);
    recalc(q);

    return q;
}

node* rotate_left(node* p)
{
    node* q = p -> right;
    p -> right = q -> left;
    q -> left = p;

    recalc(p);
    recalc(q);

    return q;
}

node* balance(node* p)
{
    recalc(p);

    if ( height(p -> left) - height(p -> right) == 2 )
    {
        if ( height(p -> left -> right) > height(p -> left -> left) )
            p -> left = rotate_left(p -> left);
        return rotate_right(p);
    }
    else if ( height(p -> right) - height(p -> left) == 2 )
    {
        if ( height(p -> right -> left) > height(p -> right -> right) )
            p -> right = rotate_right(p -> right);
        return rotate_left(p);
    }

    return p;
}

node* search(node* p, int key)
{
    if ( !p )
        return NULL;

    if ( key < p -> key )
        return search(p -> left, key);
    else if ( key > p -> key )
        return search(p -> right, key);
    else
        return p;
}

node* insert(node* p, int key, int data)
{
    if ( !p )
        return new_node(key, data);

    if ( key < p -> key )
        p -> left = insert(p -> left, key, data);
    else if ( key > p -> key )
        p -> right = insert(p -> right, key, data);
    else
        p -> data = data;

    return balance(p);
}

node* find_min(node* p)
{
    if ( p -> left != NULL )
        return find_min(p -> left);
    else
        return p;
}

node* remove_min(node* p)
{
    if ( p -> left == NULL )
        return p -> right;

    p -> left = remove_min(p -> left);
    return balance(p);
}

node* remove_item(node* p, int key)
{
    if ( !p )
        return NULL;

    if ( key < p -> key )
        p -> left = remove_item(p -> left, key);
    else if ( key > p -> key )
        p -> right = remove_item(p -> right, key);
    else
    {
        node* l = p -> left;
        node* r = p -> right;
        free(p);

        if ( r == NULL )
            return l;

        node* m = find_min(r);
        m -> left = l;
        m -> right = remove_min(r);

        return balance(m);
    }

    return balance(p);
}

void free_tree(node* p)
{
    if ( !p )
        return;

    free_tree(p -> left);
    free_tree(p -> right);
    free(p);
}

int main(void)
{
    node* root = NULL;

    char c;
    int k, d;
    while ( scanf("%c", &c) && c != 'F' )
    {
        if ( c == 'A' )
        {
            scanf("%d %d", &k, &d);
            root = insert(root, k, d);
        }
        else if ( c == 'S' )
        {
            scanf("%d", &k);
            node* n = search(root, k);
            if ( n )
                printf("%d %d\n", n -> key, n -> data);
        }
        else if ( c == 'D' )
        {
            scanf("%d", &k);
            root = remove_item(root, k);
        }
    }

    free_tree(root);

    return 0;
}

Overall, this seems to be some well-crafted code that is clear, concise and doesn't leak memory.  Here are some things that may help you improve your code.

Use the appropriate #includes

In order to compile and link, this code requires the following two lines:

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

For the program to be complete, these should be listed, too.

Fix node deletion

There is a problem with the remove_item code.  In particular, if we construct a tree with just three items and then attempt to remove the root, the current code causes a crash because the left subtree is added before the call to remove_min(r). To fix that, simply swap the lines in remove_item so that this part of the code looks like this:

    node* m = find_min(r);
    m -> right = remove_min(r);
    m -> left = l;

This is likely to be the problem that causes the time limit to be exceeded.


C++ Fixed-size general-purpose thread pool I've been reading about thread pools in C++ for a few days now and decided to roll out my own. I mainly intend to use it to learn how to implement parallel algorithms at some point in the future but before that I have to know if there's something I can do to make it more efficient.

These are all the variables I'm using. I decided to put everything inside its own namespace and make the std::condition_variable (responsible for pausing the main thread) static because there's really no need for every thread_pool object to have a copy of it.

namespace async {

    static std::condition_variable main_thread_cv;

    template <size_t Size>
    class thread_pool {

        private:
            std::mutex mutex_m;
            std::atomic<int> busy_m;
            std::condition_variable pool_cv_m;
            std::array<std::thread, Size> workers_m;
            std::queue<std::function<void()>> task_queue_m;
            bool should_terminate_m;
            void thread_loop();

        public:
            thread_pool();
            ~thread_pool();
            thread_pool(const thread_pool& other) = delete;
            thread_pool(const thread_pool&& other) = delete;
            thread_pool& operator=(const thread_pool& other) = delete;
            thread_pool& operator=(const thread_pool&& other) = delete;
            void wait();
            template <typename Fn, typename... Args> void enqueue(Fn&& function, Args&&... args);

    };
...
}

This is the thread loop executed by all of the worker threads.

template<size_t Size>
void thread_pool<Size>::thread_loop() {

    thread_local std::function<void()> task;

    for (;;) {
        { std::unique_lock<std::mutex> lock(mutex_m);
            pool_cv_m.wait(lock, [this]() { return !task_queue_m.empty() || should_terminate_m; });
            if (should_terminate_m) {
                break;
            }
            task = task_queue_m.front();
            task_queue_m.pop();
        }
        busy_m++;
        task();
        busy_m--;
        main_thread_cv.notify_one();
    }

}

ctor and dtor:

template<size_t Size>
thread_pool<Size>::thread_pool() {

    busy_m = 0;
    should_terminate_m = false;

    for (auto& thread : workers_m) {
        thread = std::thread([this]() { thread_loop(); });
    }

}

template<size_t Size>
thread_pool<Size>::~thread_pool() {

    busy_m = 0;
    should_terminate_m = true;

    pool_cv_m.notify_all();

    for (auto& thread : workers_m) {
        thread.join();
    }

}

The wait and enqueue functions:

template<size_t Size>
void thread_pool<Size>::wait() {

    { std::unique_lock<std::mutex> lock(mutex_m);
        main_thread_cv.wait(lock, [this]() { return busy_m == 0 && task_queue_m.empty(); });
    }

}

template<size_t Size>
template<typename Fn, typename ...Args>
void thread_pool<Size>::enqueue(Fn&& function, Args&& ...args) {

    { std::scoped_lock<std::mutex> lock(mutex_m);
        task_queue_m.push(std::bind(std::forward<Fn>(function), std::forward<Args>(args)...));
        pool_cv_m.notify_one();
    }

}

Ensure the mutex is locked when changing should_terminate_m

In the destructor, you should hold the mutex locked while changing should_terminate_m. It may work as it is in this particular case, but in general, if you hold a mutex while accessing a variable anywhere in the code (it's held in thread_loop() while reading it), you should hold the mutex everywhere you access it. This avoids unexpected behavior.

Stick with either std::unique_lock or std::scoped_lock

You use both types of locks in the code. While functionally it should be perfectly fine, it's weird to mix these two. Be consistent and use only one of them. I recommend sticking with std::scoped_lock if you don't mind not being compatible with pre-C++17 compilers.

Issues with wait()

One issue I see is that in order to support the wait() member function, you have a reference counter busy_m, and you have to notify the condition vairable pool_cv_m every time a task finishes. This adds overhead even if nothing is actually calling wait(), but more importantly, because you use main_thread_cv.notify_one(), if multiple threads are calling wait(), then the notification might go to the wrong thread.

Another issue is that if jobs are enqueued often enough that the queue is never empty, wait() never returns, even though all jobs that were enqueued before wait() was called did finish.

You could indeed make it explicit that only the main thread is allowed to enqueue jobs and call wait(), but that might limit the usefulness of this thread pool somewhat.


Simple number guessing game in python The problem is to ask the user to guess a number between 1 to 100 and compare it with a random number of that range. If user guessed number is lesser/greater than the random number print "too low" or "too high" accordingly. Take new input asking the user to guess the correct number again. When the guessed number matches with the random number print "you win" and the number of attempts the user required to guess it.
import random
u_num = int(input("guess a number between 1 to 100: "))
w_num = random.randint(1,100)
i = 1
while u_num != w_num:
    if u_num < w_num:
        print("too low")
    else:
        print("too high")
    u_num = int(input("guess again: "))
    i += 1
print(f"you win, and you guessed this number in {i} times!")

This looks to do the job. I would only change the variable naming to be more meaningful in this context to guess for user guess and answer for the generated number.
I would also try to handle erroneous input such as strings instead of numbers being passed.
But besides this is still functionally good :D


How to format embedded if statements in JavaScript What would be the best way to format this code? It looks a bit messy to me in regards to indentation etc.

 if (typeof _srwc != 'undefined') {
    for (var i=0; i < _srwc.length; i++){
         var currentArg = _srwc[i];;
          if (typeof window[currentArg[0]] == 'function') {
             window[currentArg[0]](currentArg[1], currentArg[2], currentArg[3]);
             } else {
             console.log('The Setter method: "' + currentArg[0] + '" is undefined');
             }
    }
 }

How to format code is not really how CodeReview works ;)

You are indenting with 2 and with 4 spaces, pick one, I advocate 2
It is more common to compare _srwc to undefined itself
_srwc is an unfortunate name, it conveys no meaning
window[currentArg[0]](currentArg[1], currentArg[2], currentArg[3]); could use a comment block
Do not use console.log in production code
Also, read about apply, it allows you to call a function and pass an array of arguments so you don't have to be limited to 3 arguments.

if (_srwc !== undefined) {
  for (var i=0; i < _srwc.length; i++){
    var currentArg = _srwc[i];
    if (typeof window[currentArg[0]] == 'function') {
      window[currentArg[0]](currentArg[1], currentArg[2], currentArg[3]);
    } else {
      console.log('The Setter method: "' + currentArg[0] + '" is undefined');
    }
  }
}


Grouping counts from tables by account ID I'm working with this query to get counts from three different tables, and to group the results by account ID:

SELECT accountId , SUM(ApplicantsCount) as ApplicantsCount, SUM(ApprovedCount) as ApprovedCount, SUM(ScreenedCount) as ScreenedCount
FROM (
    SELECT      application.accountId, COUNT(*) ApplicantCount, 0 ApprovedCount, 0 ScreenedCount
    FROM        application
    [accountIdCondition]
    GROUP BY    application.accountId

    UNION

    SELECT      application.accountId, 0, COUNT(*), 0
    FROM        application
    JOIN        termsofapproval ON application.accountId = termsofapproval.accountId
    JOIN        approvaluserjoin ON termsofapproval.id = approvaluserjoin.termsofapprovalId
[accountIdCondition]
    GROUP BY    application.accountId

    UNION

    SELECT      application.accountId, 0, 0, COUNT(*)
    FROM        application
    JOIN        screened
    ON application.id = screened.applicationId
    [accountIdCondition]
    GROUP BY    application.accountId
  ) CountsTable
GROUP BY CountsTable.accountId

However, it runs too slowly, and times out the task handler I'm calling it in. Is there a way I can write this to run faster?

I took the liberty of throwing this in to an SQLFiddle here

If I play with the query, and run the screened and approved queries on the data that I chose, I see you have a condition which may or may not be a bug. If you consider my data, where I have multiple accountId values per application, then, your subquery:

SELECT      application.accountId, 0, COUNT(*), 0
FROM        application
JOIN        termsofapproval ON application.accountId = termsofapproval.accountId
JOIN        approvaluserjoin ON termsofapproval.id = approvaluserjoin.termsofapprovalId
[accountIdCondition]
GROUP BY    application.accountId

That query will duplicate the count of approved users if there are multiple applications for the same accountId. In the SQLFiddle I have, it returns a count of 4 for only 2 distinct approvaluserjoin records.

It is likely that in your data it is not possible to get that condition though... right?

Regardless. I believe the more logical representation of your query is as follows (which I have in this SQLFiddle here

SELECT application.accountId ,
       count(distinct application.id) as ApplicantsCount,
       count(distinct approvaluserjoin.id) as ApprovedCount,
       count(distinct screened.id) as ScreenedCount
FROM application
left join termsofapproval on application.accountId = termsofapproval.accountId
left join approvaluserjoin on approvaluserjoin.termsofapprovalId = termsofapproval.id
left join screened on screened.applicationId = application.id
where application.accountId <= 2
group by application.accountId

Note how there is only one join, using left outer joins. Also note that a count of a null value is 0, so the null values in the outer-join results do not contribute to the sum. The coutn(distinct ...) construct allows you to count the things you are interested in, even if the query returns them in multiple contexts.

You will need to carefully understand the query, the implications are different to yours, and it may be more accurate than what you have (or less accurate).


How to increase the efficiency of DLA algorithm? The algorithm:

Place seed at the center of the canvas.
Release a random walker from the edge.
Random walker sticks to the neighboring sites of the seed/previous points.
Repeat N(particles) times.

For a 500x500 matrix with 50k iterations, it would easily take one full day to compute. How to reduce the computation time drastically?

Why 50k particles, cause it's a part of the assignment!

Logically program is correct.

I tried profiling, it didn't tell me a whole lot. The while loop is causing the issue? Any suggestions to improve the efficiency of my code?

import time
import numpy as np
import random
import matplotlib.pyplot as plt
from numba import jit, cuda, prange

#from numba import roc
starttime = time.time()
arr = [h,w] = [500,500] #track of height and width of canvas
particles = 50000
sticking_coeff = 0.5
canvas = np.zeros((h, w)).astype(int) #the blank n * n matrix
canvas[h//2,w//2] = 1 #make center element is 1
#// to get the floor value
stick = []
stick.append([h//2 + 1, w//2])#below 1
stick.append([[h//2 - 1, w//2]])#above 1
stick.append([h//2, w//2 + 1])#right of 1
stick.append([h//2, w//2 - 1])#left of 1

#@jit(nopython=True, parallel=True)
#@roc.jit(device=True)
#@numba.jit()
@jit(cache=True)
def walk(A, B, canvas):# A: row B: Column
    while True:
        #print('while')
        x = np.random.randint(4)#0: row 1: column
        if x == 0:#forward option
            A += 1
            #print('A+')
        elif x == 1:
            B += 1
            #print('B+')
        elif x == 2:
            A -= 1 #reverse option
            #print('A-')
        else:
            B -= 1
            #print('B-')

        #take care of overflow
        if A < 0:
            A = 0
            #print('A=0')
        elif B < 0:
            B = 0
            #print('B=0')
        if A >= h:
            A -= 1
            #print('A-1')
        elif B >= w:
            B -= 1
            #print('B-1')

        positi = [A , B]
        if positi in stick:
            if np.random.rand() < sticking_coeff:
                for site in [[positi[0] + 1, positi[1]],
                            [positi[0] - 1, positi[1]],
                            [positi[0], positi[1] + 1],
                            [positi[0], positi[1] - 1]]:
                    if site not in stick:
                        stick.append(site)
                canvas[positi[0] , positi[1]] = 1
                break  # <-- Also, this is require, I think
            else:
                continue

    return canvas

for i in prange(particles):

        print('particle ',i+1)
        selec = random.sample(set(['A','B','C','D']),1)
        #pos1 = randrange(0, len(arr))
        pos = np.random.randint(0, len(arr))
        print(selec)

        if selec == ['A']:
            #arr[:,0]#first column
            #arr[pos,0] = 1
            walk(pos,0,canvas)

        elif selec == ['B']:
            #arr[:,-1]#last column
            #arr[pos,-1] = 1
            walk(pos,-1,canvas)

        elif selec == ['C']:
            #arr[0,:]#first row
            #arr[0,pos] =1
            walk(0,pos,canvas)

        else:
            #arr[-1,:]#last row
            #arr[-1,pos] = 1
            walk(-1,pos,canvas)
plt.matshow(canvas)
plt.savefig("rand_walk.png",dpi=2000)
print('That took {} seconds'.format(time.time() - starttime))

You might consider to use a tuple of directions, something like this:

directions = ((1,0), (0,1), (-1,0), (0,-1))

In your while loop, you can do:

direction = directions[np.random.randint(4)]
A += direction[0]
B += direction[1]

If you switch from tuple to numpy arrays (direction and your current position), you could also use numpy.add which might be faster. If this really improve performence you have to measure. To do this, you might use a seed for your random generator, to get reproducible code.

The border check might be faster with NumPy too, using abslolut - if its ok, to change the behavior that way (your praticles wont "clue" at the border, but bounce back).

Precalculation

You could go a step further, by precalculating all posible paths for several steps.

# each direction has same propability - making things easier
# you could simply create a list with an entry for each path of a tree diagram,
# as each path has the same probability
def add_one_step(steps: list):
    random_step = [(1,0), (0,1), (-1,0), (0,-1)]
    if steps == []:
        return random_step
    result = []
    for i, steps_entry in enumerate(steps):
        for step in random_step:
            a = steps_entry[0]+step[0]
            b = steps_entry[1]+step[1]
            result.append((a, b))
    return result

def get_multiple_steps(n=5):
    final_directions = []
    while n > 0:
        final_directions = add_one_step(final_directions)
        n -= 1
    return final_directions

# be careful about how many steps to precalculate! The list lengths go with 4**n
precalculated_steps = []
for i in range(12):
    precalculated_steps.append(get_multiple_steps(i))

You could use such precalculated values, to do several steps in one go.

n = 10
direction = precalculated_steps[n][np.random.randint(4**n)]
A += direction[0]
B += direction[1]

Thats the most simple aproach. If you want to go to higher n values, you have to think about how to reduce precalculation time (this is just a simple brute force calculation of all paths), and how to safe each result only once (and how many times it occurs).

The tricky part is your border and the stick. You have to chose a fitting matrix size depending on your current distance to border and stick.

Outlook - binomial distribution and minimal distance matrix

If you understood the above and really want to speed up your code, I would suggest to look at:

Binomial distribution

you can split stepping in two seperate movements (x and y) - than your radom walk over n steps is simply a binominal distribution
with this, you could precalc a quadratic matrix for each n = 1 .. 249 with the probability to reach a  position

precalc minimal distance from sticking and border for each position; you have to update this only when a particle finally sticks - and you only need to check for each position, if this stick-position is closer than the previous value; This matrix is used to get the max number of steps you can do in one go.

By using the minimal distance matrix to determine the maximal number of steps allowed to do, and then using the fitting precalculated stepping matrix the solution should be ready in minutes or seconds. If you can be more lax with the border condition, it would speed up the algorithm even more.

Further disscussion and code example can be found in another question.

Errors

After understanding what your code is intended to do, I think your for-loop over all paricles is broken.

First:

pos = np.random.randint(0, len(arr))  # will set pos to 0 or 1
# should be:
pos = np.random.randint(0, arr[0])
# or better
random_pos_x = np.random.randint(0, arr[0])
random_pos_y = np.random.randint(0, arr[1])

Also, you only start particles at the upper and left border?

walk(pos,-1,canvas)  # why -1?
...
walk(-1,pos,canvas)  # why -1?

# should be:
walk(random_pos_x ,arr[1]-1, canvas)
walk(arr[0]-1, random_pos_y, canvas)

The name arr is an example for a bad name, because it hints to "array" (thus it was hard to spot the mistake above). Better would be samething like area. Or use h and w (personally I would prefer to name thus as height and width; thats easier to read in most cases)


A tool to help you memorize text I wrote a snippet of code to help me memorize text out of files and was hoping to get some feedback on the code, since Python isn't my strongest language.

You can see the code on GitHub, but I've pasted it below as well.

#!/usr/bin/python
# coding=utf-8

import argparse
import codecs
import random
import sys

parser = argparse.ArgumentParser(description="""A tool to help memorize some text! When provided with a file, this program will remove random words from each line of text and ask you to provide the words that were removed.""")
parser.add_argument('--no-color', action='store_true', help='hide colorful underlining')
parser.add_argument('--a', dest='attempts', default=3, help='number of attempts to allow per word (0 for unlimited attempts, default: 3)')
parser.add_argument('filename', metavar='filename', type=str, help='the text file')

args = parser.parse_args()

try:
    with codecs.open(args.filename, 'r', 'utf-8') as f:
        current_line = ''
        for line in f:
            missing_words = [u'']
            idx = 0
            for word in line.split(' '):
                show_word = True
                if random.randrange(10) < 5:
                    show_word = False
                for char_idx in range(0,len(word)):
                    if (word[char_idx].isalpha() and show_word) or not word[char_idx].isalpha():
                        current_line += word[char_idx]
                    else:
                        missing_words[idx] += word[char_idx]
                        if not args.no_color:
                            current_line += '\033[91m'
                        current_line += '_'
                        if not args.no_color:
                            current_line += '\033[0m'
                if word[-1] != '\n':
                    current_line += ' '
                if len(missing_words[idx]):
                    missing_words.append(u'')
                    idx += 1
            missing_words.pop()
            attempts = 0
            while len(missing_words):
                print(current_line)
                try:
                    attempt = raw_input('Enter the next missing word: ')
                    if unicode(attempt,'utf-8') == missing_words[0]:
                        print("Correct! The word was '" + missing_words[0] + "'.")
                        if args.no_color:
                            current_line = current_line.replace(''.join('_' for char in missing_words[0]),missing_words[0],1)
                        else:
                            current_line = current_line.replace(''.join('\033[91m_\033[0m' for char in missing_words[0]),missing_words[0],1)
                        missing_words.pop(0)
                        attempts = 0
                    elif args.attempts > 0 and attempts == args.attempts:
                        print("Too many attempts. The word was '" + missing_words[0] + "'.")
                        if args.no_color:
                            current_line = current_line.replace(''.join('_' for char in missing_words[0]),missing_words[0],1)
                        else:
                            current_line = current_line.replace(''.join('\033[91m_\033[0m' for char in missing_words[0]),missing_words[0],1)
                        missing_words.pop(0)
                        attempts = 0
                    else:
                        print("Incorrect. Please try again.")
                        attempts += 1
                except:
                    sys.exit(0)
except IOError:
    print("File not found: '" + args.filename + "'")

Python's style guide (PEP0008) says that lines should aim to be 79 characters long. This prevents people having to scroll across as they read a line. You've broken that multiple times. I'd suggest using ways of breaking lines to avoid it. Like this:

parser = argparse.ArgumentParser(description="""A tool to help memorize some text!
When provided with a file, this program will remove random words from each line
of text and ask you to provide the words that were removed.""")
parser.add_argument('--no-color', action='store_true',
                    help='hide colorful underlining')
parser.add_argument('--a', dest='attempts', default=3,
                    help=('number of attempts to allow per word '
                          '(0 for unlimited attempts, default: 3)') )

To explain that last one. You can spread what would be one line over multiple lines by putting brackets around it. You can then break strings up without even using + because they'll be implicitly concatenated together, turning it back into one long string.

You can assign the result of a boolean to a variable in Python, so you could turn these three lines:

show_word = True
if random.randrange(10) < 5:
    show_word = False

Into just one:

show_word = random.randrange(10) > 4

You don't need to use a for loop and refer to the character by index, you can directly get Python to loop over the individual elements of a list. So instead of

for char_idx in range(0,len(word)):
    if (word[char_idx].isalpha() and show_word) or not word[char_idx].isalpha():

you can use

for char in word:
    if (char.isalpha() and show_word) or not char.isalpha():

Much cleaner and easier to read.

What is this part doing? It's quite unclear and I had to read back up and down to understand it. A comment would give very useful context here.

# If there was a word added, append a new empty string to missing words.
if len(missing_words[idx]):
    missing_words.append(u'')
    idx += 1

That comment could be improved, but I think even better would be a refactoring of this code. You should make a temporary empty string, then at the end check if it contains anything and append that. Performing operations on the list item only to append a new item one is kind of backwards. Also, for the record, you could eliminate idx anyway because with Python you can use -1 as an index value to get the last item in the list. So you could just do missing_words[-1] and get the same result.

I assume you have this bare try except so that people don't get errors spat at them for using Ctrl+C to exit the program. But in that case you should still use a specific error like KeyboardInterrupt otherwise you're just hiding your errors and making debugging harder. Also it's important not to put too many lines inside a try except when it can be avoided. If my presumption is right and this is to catch KeyboardInterrupts nicely, then it only needs to be around the raw_input line.

try:
    attempt = raw_input('Enter the next missing word: ')
except KeyboardInterrupt:
    sys.exit(0)

If you have other errors you want to catch, you can catch multiple ones. Just make sure they're in a comma separated list and then any of those exceptions will run your exit.

except (KeyboardInterrupt, SystemExit):

Don't have an integer called attempts and a string called attempt. Either switch the int to being tries or switch the string to being guess. It's just confusing the other way and liable to cause typos. Also it could be a good idea to start the loop with next_word = missing_words[0]. It would save you some typing throughout and is more clear about what the variable is.

''.join('_' for char in missing_words[0]) is long and confusing. You can actually just use the multiplication operators on a string, so '_' * len(missing_words[0]) would work fine.

Python has a concept called truthiness where many types of data can be evaluated as a boolean easily. So if you just go elif args.attempts, that will return True for integers that aren't 0, and False for 0 itself.

elif args.attempts and attempts == args.attempts:

Also you have duplicate code here. It could be avoided with a little rearranging.

if unicode(attempt,'utf-8') == missing_words[0]:
    print("Correct! The word was '" + missing_words[0] + "'.")
elif args.attempts > 0 and attempts == args.attempts:
    print("Too many attempts. The word was '" + missing_words[0] + "'.")
else:
    print("Incorrect. Please try again.")
    attempts += 1
    continue

if args.no_color:
    current_line = current_line.replace('_' * len(nextWord), nextWord, 1)
else:
    current_line = current_line.replace('\033[91m_\033[0m' * len(nextWord),
                                        nextWord, 1)
missing_words.pop(0)
attempts = 0

The main thing that might look confusing to you here is the continue keyword. It's used in loops to tell Python to ignore the rest of the code in the loop and go to the next iteration. This way, if the answer is incorrect but the user has attempts left, Python will continue onto the next iteration.

This is a good error message, but you should use str.format to make nicer looking string formatting.

print("File not found: '{}'".format(args.filename))

It's helpful both for coercing variables into strings but also for making the string one coherent string of characters you can read rather than breaking it up.


A genric extension method to filter Linq-EF queries I have various types of EF entities, all of them have a navigation property called "Employee". When generating reports the user will have the option to filter the report according to the different employee properties (Cost Center, gender, etc.).

Something like this:

var courses = context.Courses
              .Where(c => c.Employee.CostCenterID == ccID
                     && c.Employee.Rank == rankID
                     ....
                    )
                    .ToList();

The real filter code is much more longer but this was just a hint. Anyway, I figured out an easier way of doing this filtering with an extension method using an interface:

public interface IFilterable
{
    Employee Employee
    {
        get;
        set;
    }
}

Then, I added partial classes to inherit the previous interface for different entities that has the Employee navigation property, for example:

public partial class Course: IFilterable
{
}

Then created the following generic method:

public static IQueryable<T> Filter<T>(this IQueryable<T> source, SearchCriteria sc)
    where T : class, IFilterable
{
    var filtered = source.Where(e => e.Employee.CostCenterID == sc.CostCenterID
        && e.Employee.Gender == sc.Gender
        .....
        );

     return filtered;
}

then simply I can use it like this on any class that inherits IFilterable:

var list = context.Courses.Filter(sc).ToList();

Note: the SearchCriteria is just a simple class that holds different employee properties.

Q: Is this the way to do it? or is there a more elegant way?

I think you might run into some design ugliness with the in the future because you are now coupling the knowledge that an entity could be filtered by a certain property to each entity.

Imagine a requirement comes in to filter by Course - what would that look like in your design?

Are you going to add a Course property to your IFilterable - but then what if an entity currently implementing IFilterable doesn't have a Course? So you then might need to add another IFilterable interface like an ICourseFilterable and another Filter method to go with it. And then if you want to filter on another entity ... seems quite cumbersome and involved to me.

I think the simplest and most flexible way forward is to actually move the matching responsibility into the SearchCriteria. So instead of having a SearchCriteria you rename it to EmployeeSearchCriteria and give it a method to match a particular employee:

public class EmployeeSearchCriteria
{
    private int _CostCenterId;
    private Gender _Gender;
    ... // other fields to match

    public EmployeeSearchCriteria(int costCenterId, Gender gender, ...)
    {
        _CostCenterId = costCenterId;
        _Gender = gender;
        ...
    }

    public bool Matches(Employee e)
    {
        return e.CostCenterID == _CostCenterId && e.Gender == _Gender && ... ;
    }
}

Then you can do your filtering via:

var list = context.Courses.Where(c => sc.Matches(c.Employee)).ToList();

And in the future if you want to filter entities by Course you create a CourseSearchCriteria with a method bool Matches(Course c) which you then can use as

var list = context.Employees.Where(e => sc.Matches(e.Course)).ToList();

The advantage is that the filtering by specific entities is encapsulated into single classes with single responsibilities and you do not have to litter your code with all these filter interfaces.

The concern that an entity can be filtered is now removed from the entity and kept separate which is always a good thing. The more an entity has to know what might happen to it the more complex your code structure will become.


Using state transitions to filter C comments This is my second attempt at K&R 1-23,

  Write a program to remove all comments from a C program. Don't forget
  to handle quoted strings and character constants properly. C comments
  don't nest.

I was previously packing characters into various io buffers, and someone suggested this was not a good choice. Thought I'd try something more along the lines of a state machine:

#include <stdio.h>

#define NORMAL          0
#define SINGLE_QUOTE    1
#define DOUBLE_QUOTE    2
#define SLASH           3
#define MULTI_COMMENT   4
#define INLINE_COMMENT  5
#define STAR            6

int state_from_normal(char prev_symbol, char symbol)
{
    int state = NORMAL;

    if (symbol == '\'' && prev_symbol != '\\') {
        state = SINGLE_QUOTE;
    } else if (symbol == '"') {
        state = DOUBLE_QUOTE;
    } else if (symbol == '/') {
        state = SLASH;
    }
    return state;
}

int state_from_single_quote(char prev_symbol, char symbol)
{
    int state = SINGLE_QUOTE;

    if (symbol == '\'' && prev_symbol != '\\') {
        state = NORMAL;
    }
    return state;
}

int state_from_double_quote(char prev_symbol, char symbol)
{
    int state = DOUBLE_QUOTE;

    if (symbol == '"' && prev_symbol != '\\') {
        state = NORMAL;
    }
    return state;
}

int state_from_slash(char symbol)
{
    int state = SLASH;

    if (symbol == '*') {
        state = MULTI_COMMENT;
    } else if (symbol == '/') {
        state = INLINE_COMMENT;
    } else {
        state = NORMAL;
    }
    return state;
}

int state_from_multi_comment(char symbol)
{
    int state = MULTI_COMMENT;

    if (symbol == '*') {
        state = STAR;
    }
    return state;
}

int state_from_star(char symbol)
{
    int state = STAR;

    if (symbol == '/') {
        state = NORMAL;
    } else {
        state = MULTI_COMMENT;
    }
    return state;
}

int state_from_inline_comment(char symbol)
{
    int state = INLINE_COMMENT;

    if (symbol == '\n') {
        state = NORMAL;
    }
    return state;
}

int state_from(int prev_state, char prev_symbol, char symbol)
{
    switch(prev_state) {
        case NORMAL :
            return state_from_normal(prev_symbol, symbol);
        case SINGLE_QUOTE :
            return state_from_single_quote(prev_symbol, symbol);
        case DOUBLE_QUOTE :
            return state_from_double_quote(prev_symbol, symbol);
        case SLASH :
            return state_from_slash(symbol);
        case MULTI_COMMENT :
            return state_from_multi_comment(symbol);
        case INLINE_COMMENT :
            return state_from_inline_comment(symbol);
        case STAR :
            return state_from_star(symbol);
        default :
            return -1;
    }
}

int main(void)
{
    char input;
    char symbol = '\0';
    char prev_symbol;

    int state = NORMAL;
    int prev_state;

    while ((input = getchar()) != EOF) {
        prev_symbol = symbol;
        prev_state  = state;
        symbol = input;
        state  = state_from(prev_state, prev_symbol, symbol);

        if (prev_state == SLASH && state == NORMAL) {
            putchar(prev_symbol);
        }
        if (prev_state != STAR && state < SLASH) {
            putchar(symbol);
        }
    }
}

In all, this looks like pretty solid code.  I have just a few suggestions that may help you improve your code.

Use an enum for related constants

The states are all related and not just standalone constants.  For that reason, I'd recommend encapsulating them all in an enum:

enum { NORMAL, SINGLE_QUOTE, DOUBLE_QUOTE, SLASH, MULTI_COMMENT, INLINE_COMMENT, STAR } state_e;

Restructure or comment to make the code easier to read

The most significant feature in the code is a state machine.  In order to understand a state machine, I typically need to know what's being processed (an input C program), what the states are (which are enumerated) and how the code transitions from state to state.  The last bit is the part that is a little tough to decipher.  It's probably mostly right, but it's hard to decode.  For instance, see how long it takes you to answer the questions, "How does the code enter the SLASH state?", "How does it leave the SLASH state?"

Don't forget about line continuation

The \ character is a line continuation character in C.  One effect it can have that affects this program is to continue a single-line comment:

// BAD is never defined \
#define BAD 1

Don't forget about trigraphs

Many people either don't use or don't know about trigraphs but they exist and, for better or worse, are still part of the language. This affects this particular program because the ??/ is the trigraph for \ which is the line continuation character.  Related to the preceding comment, BAD is never defined in this code fragment:

// are you surprised??/
#define BAD 1


Audio player in Angular 2 I just built an audio player in Angular 2 using a player component and a player service. It's all working fine, I just feel like there is a much better way to do this.

Should the audio object be in the service or the component? I'm skeptical because I'm using three different observables and I don't think that is the best way to do it.

player.component.ts:

export class PlayerComponent implements OnInit, OnDestroy {

  // General variables
  private song: Song;
  private currentTime: string;
  private fullTime: string;
  private isPlaying: boolean;
  // Subscription variables
  private songSubscription: any;
  private currentTimeSubscription: any;
  private fullTimeSubscription: any;

  constructor(private _playerService: PlayerService) {
  }

  ngOnInit() {
    this.songSubscription = this._playerService.song.subscribe(data => this.song = data);
    this.currentTimeSubscription = this._playerService.currentTime.subscribe(data => this.currentTime = data);
    this.fullTimeSubscription = this._playerService.fullTime.subscribe(data => this.fullTime = data);
    console.log("Player subscription initialized");
  }

  toggleAudio() {
    this.isPlaying = this._playerService.toggleAudio();
  }

  ngOnDestroy() {
    this.songSubscription.unsubscribe();
    this.currentTimeSubscription.unsubscribe();
    this.fullTimeSubscription.unsubscribe();
    console.log("Player subscription destroyed");
  }

}

player.service.ts:

export class PlayerService {

  private audio: any;
  public song: Subject<Song> = new Subject<Song>();
  public currentTime: Subject<string> = new Subject<string>();
  public fullTime: Subject<string> = new Subject<string>();

  constructor(private _utilityService: UtilityService) {
    this.audio = new Audio();
  }

  setPlayer(song: Song) {
    this.song.next(song);
    this.audio.src = song.audio;
    this.audio.oncanplaythrough = () => {
      this.audio.play();
      this.fullTime.next(
        this._utilityService.getFormatedTime(this.audio.duration)
      );
    };
    this.audio.ontimeupdate = () => {
      this.currentTime.next(
        this._utilityService.getFormatedTime(this.audio.currentTime)
      );
    };
  }

  toggleAudio() {
    if (this.audio.paused) {
      this.audio.play();
    } else {
      this.audio.pause();
    }

    return this.audio.paused;
  }

}

player.component.html:

    {{currentTime}}

    {{song.title}} by {{song.artist}}

    {{fullTime}}

It's a pretty primitive player right now. I want to make sure I'm implementing it correctly before I add more features.

There is one way I can think of improving this. In angular2, components should only handle view logic.
The problem isn't there at the moment but could be there when you add more functionalities. Let's say you want to add a recorder for the song that saves the song from time 0 to time 1.
You'd have to add that to this component and keep cluttering it.

I would treat this component as a "master" songPlayer. It operates on a song (i.e. Your song model) so that is okay. It could also operate on the "Player" model. Player has a state. It has its time, knows if it is playing and where the current time is.

Perhaps the best way to structure this would be to start with models:
PlayerModel(time, isPlaying, currentTime, song). Song is as it is. What this does is that it creates only one subscription on your playerComponent. To the Player.

I hope I'm not keeping it too abstract with this. It was just an idea.


Inserting new Node in a Binary Search Tree I wrote this method to recursively insert a new node at the proper position in a BST. Is it good enough? Can it be improved? It went through a couple of revisions and I still am not very confident in this version.
Making the new node root when no nodes are present is taken care of in a separate wrapper function.

public void insertNode(Node<Integer> currentParent, Node<Integer> newNode) {
    if (newNode.getNodeData() < currentParent.getNodeData()) {
        if(currentParent.getLeftChild() == null)
            currentParent.setLeftChild(newNode);
        else
            insertNode(currentParent.getLeftChild(), newNode);

    } else {
        if(currentParent.getRightChild() == null)
            currentParent.setRightChild(newNode);
        else
            insertNode(currentParent.getRightChild(), newNode);
    }
}

A BST must not contain duplicate values.
Your implementation allows inserting a duplicate value as the right child.
It should be something like this:

if (newNode.getNodeData() < currentParent.getNodeData()) {
    if (currentParent.getLeftChild() == null) {
        currentParent.setLeftChild(newNode);
    } else {
        insertNode(currentParent.getLeftChild(), newNode);
    }
} else if (newNode.getNodeData() > currentParent.getNodeData()) {
    if (currentParent.getRightChild() == null) {
        currentParent.setRightChild(newNode);
    } else {
        insertNode(currentParent.getRightChild(), newNode);
    }
} else {
    // duplicate item
    // option 1: do nothing: ignore input, add nothing to the tree
    // option 2: throw new IllegalArgumentException("no no: dupe")
}

It's also recommended to use braces always,
so I modified the original code accordingly.


Generic comparitive array selection sort My code works, but my IDE is giving me warnings for unchecked method invocation.  I have found useful information about Comparable arrays here.  I have found useful information about unchecked method invocation here.  Despite this, I just can't figure out how to please my IDE.  My intention is to have the method selectionSort sort an array of either Integers or Doubles using compareTo.

import java.util.Arrays;

class Main{
    public static void main(String[] args){

        Comparable[] data0 = {2, 8, -39, 904, 8, 0};
        Comparable[] data1 = {1.1, -5.68, 4.39, -0.01, 5.59};

        System.out.printf("Unsorted array: %s%n%n", Arrays.toString(data0));
        selectionSort(data0);
        System.out.printf("%nSorted array: %s%n", Arrays.toString(data0));

        System.out.printf("Unsorted array: %s%n%n", Arrays.toString(data1));
        selectionSort(data1);
        System.out.printf("%nSorted array: %s%n", Arrays.toString(data1));
    }

    private static <T extends Comparable<T>> void selectionSort(T[] data){
        for(int i = 0; i < data.length - 1; ++i){
            int smallest = i;

            for(int index = i + 1; index < data.length; ++index){
                if (data[smallest].compareTo(data[index]) > 0){
                    smallest = index;
                }
            }

            swap(data, i, smallest);
            printPass(data, i + 1, smallest);
        }
    }

    private static <T> void swap(T[] data, int first, int second){
        T temporary = data[first];
        data[first] = data[second];
        data[second] = temporary;
    }

    private static <T> void printPass(T[] data, int pass, int index){
        System.out.printf("after pass %2d: ", pass);

        for(int i = 0; i < index; ++i){
            System.out.printf("%s ", data[i]);
        }

        System.out.printf("%s* ", data[index]);

        for(int i = index + 1; i < data.length; ++i){
            System.out.printf("%s ", data[i]);
        }

        System.out.printf("%n               ");

        for (int j = 0; j < pass; j++){
            System.out.printf("-- ");
        }
        System.out.println();
    }
}

When I put this into IntelliJ with my default settings I already get a warning on the Comparable[] data0 declaration.

  Raw use of parameterized class

Changing the array declarations to these:

Integer[] data0 = {2, 8, -39, 904, 8, 0};
Double[] data1 = {1.1, -5.68, 4.39, -0.01, 5.59};

solved both warnings.


Filename sequencer with outdated C++ My path to learning C++ has been a rough one. Currently, I'm writing a project and my knowledge is very outdated. While I can write code that accomplishes the tasks required, I'd like to improve and learn new aspects of the language.

So, I've written a class called Sequencer. It works. It's not great. It's not terrible. But it's based on old code. How would I improve the class with modern code?

#pragma once
#include <regex>
#include <string>
#include <experimental/filesystem>

class Sequencer
{
public:

    //////////////////////
    // CTors / DTor
    Sequencer(std::string const first_frame_in): first_frame_name(first_frame_in)
    {
        bool result = std::regex_search(first_frame_name, first_frame_regex_results, match_pattern);
        prefix = first_frame_regex_results[1];
        std::string frame = first_frame_regex_results[2];
        ext = first_frame_regex_results[3];
        first_frame = std::stoi(frame);
        digit_padding = frame.length();
        current_frame = first_frame;
        out_prefix = "_signature";
        file_delim = ".";

        get_last_frame();
    }

    ~Sequencer() {}

    Sequencer(Sequencer const&) = delete;
    void operator=(Sequencer const&) = delete;

    bool get_next_frame(std::string &next_frame, std::string &name_out)
    {
        if (current_frame <= last_frame)
        {
            tmp_num = add_padding(current_frame);
            next_frame = prefix + file_delim + tmp_num + file_delim + ext;
            name_out = prefix + out_prefix + file_delim + tmp_num + file_delim + "ppm";
            current_frame++;
            return true;
        }
        else
        {
            name_out = "";
            next_frame = "";
            return false;
        }
    }

private:

    void get_last_frame()
    {
        bool test = true;
        std::string tmpname;
        unsigned int i = current_frame + 1;
        do
        {
            tmpname = "C:\\ProgramData\\NVIDIA Corporation\\CUDA Samples\\v9.0\\3_Imaging\\MovieHasher\\data\\" + prefix + file_delim + add_padding(i) + file_delim + ext;
            test = std::experimental::filesystem::exists(tmpname);
            if (test)
                i++;
            else
                i--;
        } while (test);
        last_frame = i;
    }

    std::string add_padding(int frame_num_in)
    {
        std::string outval = std::to_string(frame_num_in);

        int length = outval.length();
        while (length < digit_padding)
        {
            outval = "0" + outval;
            length = outval.length();
        }

        return outval;
    }

    std::string first_frame_name;
    std::regex match_pattern = std::regex("([a-zA-Z]*).([0-9]*).(\\w{3})");
    std::smatch first_frame_regex_results;
    std::string prefix, ext, out_prefix, tmp_num, file_delim;
    int first_frame;
    int last_frame;
    int digit_padding;
    int current_frame;

};

A sample main file to run the Sequencer class

////////////////////////
// INCLUDES

//// System
#include <iostream>
#include <string>

//// User Defined
#include "../../Sequencer.h"

int main(int argc, char **argv)
{
    std::string image_filename = "prefix.00000.ext";
    std::string out_filename = "prefix_signature.00000.ppm";
    Sequencer sequencer(image_filename);

    bool tmp = true;
    do
    {
        tmp = sequencer.get_next_frame(image_filename, out_filename);
        if (tmp)
            std::cout << image_filename << ", " << out_filename << std::endl;
    } while (tmp);

    return 0;
}

Code:

// CTors / DTor: This comment doesn't add anything that we can't see from the code.

Sometimes comments like this denote sections or regions of code in a file, but without consistently doing this for all the code, we don't know where the "constructor / destructor" section ends. I'd suggest just removing the comment.
Sequencer(std::string const first_frame_in): There's little benefit to making function arguments that are passed by value const. It doesn't affect the caller, and makes the function signature harder to read.

Perhaps this should be a const& though?
When we have lots of member variables in a class, adding an m_ prefix on the names (e.g. m_current_frame) helps to distinguish them from local variables or function arguments.
first_frame_name, match_pattern, and first_frame_regex_results are all unused after the constructor, so they could just be local variables rather than members.
tmp_num and tmpname are not informative names. They should also be local variables since they are each used in only one function.
Presumably negative frame numbers can't exist. If this is the case we should be using an unsigned integer type, not a signed one.
We should check the result of the regex search and handle an error (since presumably this is going to be a user specified string).
The regex captures "zero or more" digits, but we don't check that we got a match before trying to convert the frame number. Perhaps we should require "one or more" digits / letters when matching.
Perhaps the regex pattern should use the specified file_delim too.
Using a chain of + operators for std::strings tends to create a lot of temporary strings. Using std::string::append is likely to be faster, especially if we reserve() the memory we need first.
We can add the padding digits in one operation by using std::string::insert.

Design:

I'd suggest grouping the information extracted from the first frame filename into a struct. We can then put the regex search and data extraction into a separate function, and more easily use the constructor initializer list.
Unless there's a particular reason for it, I wouldn't try finding the last frame inside the Sequencer class. It's probably simpler to restrict the "sequencer" to only generating filenames, and leave checking if the file exists for code that does actual processing.
Perhaps something like FrameFilenameGenerator might be a more accurate name than Sequencer?
We can effectively use the same code to generate the input and output filenames.
If we let the user keep track of the current frame, our generator can generate the filename for any requested frame, and doesn't have to keep track of the frame itself.

So applying the above points, we might get something like:

#include <regex>
#include <string>
#include <experimental/filesystem>

class FrameFilenameGenerator
{
public:

    // note:
    // `first_frame_filename` isn't copied, so we only need a const&
    // `file_delim` is a "sink argument" (i.e. we store a copy in the class), so we take the argument by value and move it into place.
    FrameFilenameGenerator(std::string const& first_frame_filename, std::string file_delim = "."):
        m_file_delim(std::move(file_delim)),
        m_info(get_first_filename_info(first_frame_filename))
    {

    }

    // note: we should probably be more careful with the frame offset!
    // first_frame_number + frame_offset could overflow!
    std::string make_filename(std::uint32_t frame_offset, std::string const& output_suffix = "") const
    {
        auto result_size =
            m_info.prefix.size() +
            output_suffix.size() +
            m_file_delim.size() +
            m_info.digit_padding +
            m_file_delim.size() +
            m_info.extension.size();

        auto result = std::string();
        result.reserve(result_size);

        result.append(m_info.prefix);
        result.append(output_suffix);
        result.append(m_file_delim);
        result.append(frame_number_to_string(m_info.first_frame_number + frame_offset, m_info.digit_padding));
        result.append(m_file_delim);
        result.append(m_info.extension);

        return result;
    }

private:

    struct FirstFilenameInfo
    {
        std::string prefix;
        std::uint32_t first_frame_number;
        std::size_t digit_padding;
        std::string extension;
    };

    static FirstFilenameInfo get_first_filename_info(std::string const& first_frame_filename)
    {
        auto result = FirstFilenameInfo();

        const auto match_pattern = std::regex("([a-zA-Z]*).([0-9]*).(\\w{3})");
        std::smatch regex_results;

        if (!std::regex_search(first_frame_filename, regex_results, match_pattern))
            throw std::runtime_error("First frame filename does not meet the required format!");

        result.prefix = regex_results[1];

        if (regex_results[2].length())
        {
            result.first_frame_number = std::uint32_t{ std::stoul(regex_results[2]) };
            result.digit_padding = regex_results[2].length();
        }

        result.extension = regex_results[3];

        return result;
    }

    static std::string frame_number_to_string(std::uint32_t frame_number, std::size_t digit_padding)
    {
        auto result = std::to_string(frame_number);

        if (result.size() >= digit_padding)
            return result;

        auto const zeros_needed = (digit_padding - result.size());
        result.insert(result.begin(), zeros_needed, '0');

        return result;
    }

    std::string m_file_delim;
    FirstFilenameInfo m_info;
};

#include <iostream>

int main()
{
    std::string image_filename = "prefix.00030.ext";
    auto generator = FrameFilenameGenerator(image_filename);

    for (auto i = 0u; i != 20u; ++i)
    {
        auto in = generator.make_filename(i);
        auto out = generator.make_filename(i, "_signature");

        //if (!std::experimental::filesystem::exists(in))
        //  break;

        std::cout << in << " -> " << out << "\n";
    }

    return EXIT_SUCCESS;
}


Enforcing Paired Statements Using ScriptBlock I really like c#'s Using statement as it allows you to pair Open statements with their corresponding Close/Dispose statements in an elegant way which matches C#'s syntax.
I realised that something similar can be achieved in PowerShell; e.g. Push-Location should be paired with a Pop-Location, and this can be enforced by creating a function which allows you to use a USING-like syntax.

e.g.

function PushDPopD {
    [CmdletBinding()]
    param (
        [string]$Path
        ,
        [scriptblock]$Code
    )
    process {
        if (Push-Location $Path -PassThru) { #only proceed with this function's logic if our pushd were successful
            try {
                $Code.Invoke()
            } finally { #ensure that whatever happens in user code, we always popd after our pushd
                Pop-Location
            }
        }
    }
}
function ShowCurrentLocation {
    ("You are in: {0}" -f (Get-Location).Path)
}

Clear-Host
[string]$localVariable = "We keep variables even in scriptblock context"

write-host "`n`n==Healthy Demo==`n`n" -ForegroundColor cyan
ShowCurrentLocation
PushDPopD 'c:\Users' {
    "hello"
    ShowCurrentLocation
    $localVariable
    "bye"
}
ShowCurrentLocation

write-host "`n`n==Exception Demo==`n`n" -ForegroundColor cyan
ShowCurrentLocation
PushDPopD 'c:\ThisPathDoesNotExist' {
    "hello"
    ShowCurrentLocation
    $localVariable
    "bye"
}
ShowCurrentLocation

To me this seems like a logical approach / looks neater than manually checking that all pushes are correctly paired with pops.
The only potential issue is if someone includes a popd in the scriptblock itself; but that's no worse than the original scenario, so isn't a big drawback.
This also gives us the advantage that code won't blindly run on thinking it's in a different directory to the one specified if the pushd fails.

As a concept does this seem logical / have I missed anything.

NB: I'm aware it's not currently following verb-noun convention; that's because it's just proof of concept code, so I've not yet put any thought into suitable naming (suggestions welcome though).

To be honest I feel this might be overcomplicating things.

I use Push-Location/Pop-Location in try/finally blocks when needed (like when loading the SQLPS module), but that's almost never.

For the most part I think one should avoid changing the current location and instead reference the desired path (either relative or absolute). So something like:

$p = $BasePath | Join-Path -ChildPath 'subfolder'
Get-ChildItem -Path $p

rather than:

$p = $BasePath | Join-Path -ChildPath 'subfolder'
Push-Location $p
try {
    Get-ChildItem
} finally {
    Pop-Location
}

As you've seen, there are also some issues with using scriptblocks in that way. You might have scope issues with variables, and call stack/execution context will be a bit different. This might get complicated further when you consider pipelines.

So in summary I'd rather avoid changing the working directory and then not obfuscate the process when I do so it's clear what's going on.


Animation that gets triggered by two buttons In order for me to get better and create more desired code, I would like to find out if the following situation is bad or acceptable.

I am creating an animation that gets triggered by two buttons, one on the left of a JPanel and the other on the right. Depending on which button is pressed, the animation starts from the button position.

Now it can be quite some work working out the different calculations with a lot of internal if and else statements (to know which button is clicked) on the Method that handles the animation (I have done so).

If I clone the method and modify it to be suitable for one button (so each button calls a different method), which saves a lot of hassle, is that good or bad practise?

private void moveImage(){

    if(xpos < imagewidth && ypos < imageheight) {

           if(buttonA){

              image.setX(image.getX() - xmove);

           } else {

              image.setX(image.getX() + xmove);
           }

           image.setY(image.getY() + ymove);
    }

     //other codes here
}

Or I could do this for button A and the same for button B:

private void moveImageA(){

        if(xpos < imagewidth && ypos < imageheight) {

               image.setX(image.getX() - xmove);

               image.setY(image.getY() + ymove);
        }

          //other codes here
    }

Depends on application design. What is "more important" - image, or button?

It could be something like move(Image image) method in the buttons (decalred in interface of the buttons), or it may be reactOn(Button button) in Image.

It is hard to recommend something with the presented information. Generally, code reuse is always a good move.


Two sum algorithm variant I need to compute the number of target values \$t\$ in the range: \$-10000 \le t \le 10000\$ (inclusive) such that there are distinct numbers \$x, y\$ in the input file that satisfy \$t= x+y\$. I have the code which does work and would like to optimize it to run it faster.

Also, we'll count the sum \$t\$ only once. If there are two \$x,y\$ pairs that add to 55, you will only increment the result once.

public static int Calculate(int start, int finish, HashSet<long> numbers)
{
    int result = 0;
    for (int sum = start; sum <= finish; sum++)
    {
        foreach (long n in numbers)
        {
            if (numbers.Contains(sum - n) && n != (sum - n))
            {
                result++;
                break;
            }
        }
    }

    return result;
}

This is an assignment and I completed it with full marks. My code is taking like 30 minutes to run against the data set of 1 million numbers. I tried to think a way to optimize my code, but couldn't get to the right thought and would appreciate some help.

There is no need to iterate over the range. Consider a pseudocode:

sort numbers from the input file into an array A
N = size of collection
result = 0
for i in [0..N)
    find largest j (j > i) such that A[i] + A[j] < start
    find smallest k (k > i) such that A[i] + A[k] > finish
    result += k - j

Each find is \$O(\log N)\$ at worst. Overall complexity is \$O(N \log N)\$ regardless of the target range.


Drawing a regression line with a timeseries My code is using seaborn to draw a regression line with a timeseries data as the x axis, and whatever you want the y-axis to be. Since seaborn does not support this function directly, a dummy column has to be made first based on the timeseries (i.e. instead of using 2016-05-01, use 1,2,3,4,5... to represent the date. Then, plot the regression line using 1,2,3... as the x-axis and replace the 1,2,3 label with 2016-05-01, 2016-05-02...

def regplot_timeseries(df:pd.DataFrame, time_col:str, data_col:str, figsize=(14, 6), xlabel = '', ylabel = ''):
    # make a copy of the incoming data (think of the incoming data as an excel sheet visually)
    dfc = df.copy()

    # if it is a index, treat it slightly differently
    # add a column to the copy of the excel sheet for plotting purpose
    if time_col == 'index':
        dfc = dfc.sort_index()
        dfc['date_f'] = pd.factorize(dfc.index)[0] + 1
        mapping = dict(zip(dfc['date_f'], dfc.index.date))
    else:
        dfc = dfc.sort_values(time_col_str)
        dfc['date_f'] = pd.factorize(dfc[time_col])[0] + 1
        mapping = dict(zip(dfc['date_f'], dfc[time_col].dt.date))

    # plotting
    fig, ax = plt.subplots()
    fig.set_size_inches(figsize[0], figsize[1])
    sns.regplot('date_f', data_col, data=dfc, ax=ax)
    labels = pd.Series(ax.get_xticks()).map(mapping).fillna('')
    ax.set_xticklabels(labels)
    ax.set_xlabel(xlabel)
    ax.set_ylabel(ylabel)

    # delete the copy to free up memory
    del dfc

I am new to functional programming, and from what I read, I shouldn't alter the data being passed in. In this case, I shouldn't alter df, so I made a copy instead and deleted the copy at the end of the function. Is this the right to do it?

You don't need dfc, dfc.sort_index() and dfc.sort_values(time_col_str) both make a copy of the data. They don't perform an in-place sort.
del dfc isn't needed, Python will perform that action at the end of the function anyway. Also I don't think it's doing what you think it's doing. I may be wrong, but since variables in Python are like names, when you're using del, you're just removing one name tag to the object.

  Deletion of a name removes the binding of that name from the local or global namespace, depending on whether the name occurs in a global statement in the same code block.

In my personal opinion, Python doesn't lend itself well to functional programming. Yes you can do FP in it, but it tends to start looking very ugly very fast. Don't care about whether something is FP or not, just make it work.
I don't really know what your code is doing, but the if-else and the 'plotting' should probably be in two separate functions.


Login page with Selenium webdriver I am automating a login page. I am using the page object design pattern Selenium with Java. What should I improve on in my code?

package Pages;

import org.openqa.selenium.WebDriver;
import org.openqa.selenium.WebElement;
import org.openqa.selenium.support.FindBy;
import org.openqa.selenium.support.PageFactory;

public class LoginPage
{
    private WebDriver driver;

       @FindBy(id="userName")
       WebElement username;
       @FindBy(id="password")
       WebElement password;
       @FindBy(className="btn-info")
       WebElement button;
       public LoginPage(WebDriver driver)
       {
        //initialize elements
          PageFactory.initElements(driver, this);

       }
       public void set_username(String usern)
       {
        username.clear();
        username.sendKeys(usern);
       }
       public void set_password(String userp)
       {
        password.clear();
        password.sendKeys(userp);
       }
       public void click_button()
       {
        button.submit();
       }
}

package Test;
import org.testng.annotations.Test;

import Pages.LoginPage;
import org.testng.Assert;
import org.openqa.selenium.By;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.firefox.FirefoxDriver;
import org.testng.annotations.AfterTest;
import org.testng.annotations.BeforeTest;

public class LoginTest
{
WebDriver driver;
@BeforeTest
public void setup()
{
    System.setProperty("webdriver.firefox.marionette","pathToGeckodriver");
    driver=new FirefoxDriver();
    driver.manage().window().maximize();
    driver.get("https://twhyderabad.github.io/demo_site/");
}
@Test(priority=5)
public void verify1()
{
    LoginPage login=new LoginPage(driver);
    login.set_username("admin");
    login.set_password("admin");
    login.click_button();
    Assert.assertTrue(driver.getPageSource().contains("Blog"));
}
@Test(priority=1)
public void verify2()
{
    LoginPage login=new LoginPage(driver);
    login.set_username("adm");
    login.set_password("admin");
    login.click_button();
    Assert.assertEquals(driver.findElement(By.xpath("//*[@id='errorMessage']")).getText(),"Wrong username or password!");
}
@Test(priority=2)
public void verify3()
{
    LoginPage login=new LoginPage(driver);
    login.set_username("admin");
    login.set_password("adm");
    login.click_button();
    Assert.assertEquals(driver.findElement(By.xpath("//*[@id='errorMessage']")).getText(),"Wrong username or password!");
}
@Test(priority=3)
public void verify4()
{
    LoginPage login=new LoginPage(driver);
    login.set_username("adm");
    login.set_password("adm");
    login.click_button();
    Assert.assertEquals(driver.findElement(By.xpath("//*[@id='errorMessage']")).getText(),"Wrong username or password!");
}
@Test(priority=4)
public void verify5()
{
    LoginPage login=new LoginPage(driver);
    login.set_username("");
    login.set_password("");
    login.click_button();
    Assert.assertEquals(driver.findElement(By.xpath("//*[@id='errorMessage']")).getText(),"Wrong username or password!");
}
@AfterTest
public void close()
{
    driver.close();
}
}

The method names are consistent, but usually camelcase is advocated.
Indentation is inconsistent, but maybe that's from pasting it here?
Variables like usern and userp are bad, why not name and
password or something that's readable?
Why is the window maximised?  The site doesn't look that's necessary
for anything.
Consider having a login method instead of repeating the same code
five times, e.g. login.login("admin", "admin").
Extract common code to a new method.  That is, for four of the five
tests it could just be checkFailedLogin("foo", "bar").
IDs are/should be unique to elements in HTML, so the XPath is way too
complicated, consider just By.id (and By.className and By.name
for that matter).
Also consider returning the expected page from that method so that
it's clear from the code, MainPage main = login.login(...); and have
it raise an exception if the new page doesn't match what was expected
(e.g. we usually have an invisible value unique to each page to
identify it in tests, but there are probably other ways to do that).


Find the largest product of an array This task is taken from www.interviewbit.com

  Given an array of integers, return the highest product possible by
  multiplying 3 numbers from the array

  Input:

array of integers e.g {1, 2, 3}

  Example:

[0, -1, 3, 100, 70, 50]

=> 70*50*100 = 350000

  NOTE: Solution will fit in a 32-bit signed integer

My approach: First sort the array, then return the maximum the following numbers: The product of the first two smallest numbers (because they could be negative) and the last number (largest) or the product of the three largest numbers.

My solution has a runtime of \$O(n \log n)\$ due to the sort and a space complexity of \$O(1)\$. I wonder whether there is a faster solution than \$O(n\log n)\$.

function highesProd(a) {
  a.sort((a,b) => a - b);
  return Math.max(a[0] * a[1] * a[a.length - 1], a[a.length - 3] * a[a.length - 2] * a[a.length - 1]);
}

Yes you can do this in O(n) time.
You just need to scan through the array 1 time and track the 3 largest numbers and 2 smallest numbers. At the end compare the product of the 2 smallest * the largest vs the product of the 3 largest. One of those will be the max product of three numbers.

You only have to track the 2 smallest because the product of 3 negative will be negative. I put in checks to ensure the neg1 an neg2 are negative numbers, but this isn't strictly necessary for just finding the max product. Just storing the 2 smallest numbers will still return the correct answer for this problem.

I think the purpose of this question in an interview setting may be to show that sometimes the a longer block of code may run faster that a one liner.

let arr = [-2, 7, 3, 4,4,4,2,-11];
console.log(maxProduct(arr));

function maxProduct(arr) {

    //special cases
    if (arr.length<3) return null;
    if (arr.length===3) return arr.reduce( (a,b) => a*b);
    if (arr.length===4) return arr.sort((a, b) => a - b).slice(1,4).reduce( (a,b) => a*b);

    //otherwise
    let seeds = arr.slice(0,5).sort((a, b) => a - b);
    let [min1, min2, max3, max2, max1] = seeds;

    for (let i = 5; i < arr.length; i++) {
        let cur = arr[i];
        if (cur >= max1) {
            max3 = max2;
            max2 = max1;
            max1 = cur;
        } else if (cur >= max2) {
            max3 = max2;
            max2 = cur;
        } else if (cur >= max3 ) {
            max3 = cur;
        } else if (cur <= min1) {
            min2 = min1;
            min1 = cur;
        } else if (cur <= min2) {
            min2 = cur
        }
    }

    return Math.max(min1 * min2 * max1, max1 * max2 * max3);
}


More efficient way of removing line indices? After profiling my current script I've found that nearly 100% of the run time (and I'm not surprised) comes from the following function:

def rem_asc_by_pos(xmlFile, fileOut,removeIndices):
fileIn = xmlFile.replace('.xml', '.asc')
with open(fileIn, 'r') as fin:
    with open(fileOut, 'w') as fout:
        #remove indices from each line and write to new file
        if len(removeIndices) > 0:
            for line in fin:
                lineList = list(line)
                checkLine = ''
                newLine = []
                for ind,char in enumerate(lineList):
                    if ind not in removeIndices:
                        newLine.append(char)
                lineOut = ''.join(newLine)
                fout.write(lineOut)

The files themselves can have anywhere from a 15 thousand lines to 2.5 million at the most, each line is usually from 300-600 characters.

I was thinking that the write operation may be causing an increase in run-time so I could write multiple lines at a time but wouldn't know a good way to determine when it should write the lines (so as to not run out of memory).

Is there anything obvious that I might be able to change to decrease the overall run-time? To go through all the files currently it takes about 20 hours and any decrease from that is appreciated!

edit: Adding an example of input and expected output!

Input:
I have a bunch of data files that look similar to this:

511111 12  1.000000  1.000000  1.000000   .750000
511112 12  1.000000   .666667   .500000   .555556

The removeIndicies list contains the indicies to remove from each line, for this example we want to remove the first three and indicies 19-28.

Expected output:

111 12  1.000000  1.000000   .750000
112 12  1.000000   .500000   .555556

1. Test case

You can't fix what you don't measure. So let's make a test case containing a million lines similar to the example input in your question:

>>> with open('in.txt', 'w') as f:
...     for _ in range(10**6):
...         _ = f.write('511111 12  1.000000  1.000000  1.000000   .750000\n')

How long does your code take?

>>> from timeit import timeit
>>> indices = list(range(3)) + list(range(18, 29))
>>> timeit(lambda:rem_asc_by_pos('in.txt', 'out.txt', indices), number=1)
32.99110442500023

2. Rewrite

Here's a quick rewrite to simplify your code:

Follow the Python style guide (PEP8).
Write a docstring explaining what the code does.
Pick a better name for the function.
Combine the two with clauses into one to keep the indentation sensible.
No need to pass mode 'r' to open: that's the default.

And some changes to speed it up:

Turn the list of removed indices into a set, so that we can look up indices in constant time.
There's no need to split line into a list: you can iterate over a string just as easily.
Drop the checkLine and newList and lineOut intermediate variables.
Cache fout.write in a local variable to avoid having to look it up for every line.

def remove_indices(file_in, file_out, indices):
    """Copy file_in to file_out, removing the given indices."""
    index_set = set(indices)
    with open(file_in) as fin, open(file_out, 'w') as fout:
        write = fout.write
        for line in fin:
            write(''.join(c for i, c in enumerate(line) if i not in index_set))

This is about twice as fast (in my test case):

>>> timeit(lambda:remove_indices('in.txt', 'out.txt', indices), number=1)
17.147783935997722

3. Use slices instead of indices

The next improvement is to represent the parts of the line to be kept (rather than the parts to be removed) in the form of Python slice objects. A slice object represents a range of indices in the same form as an array slice, that is, as start and stop indices together with a step.

So if you are going to remove indices 0–2 and 18–28, then you are going to keep indices 3–17 and 29–. In slice notation that would be:

>>> slices = [slice(3, 18), slice(29, None)]

Note that the stop for the first slice is 18, because ranges of integers in Python are always exclusive at the top.

Here's a version of your function that takes slices to keep instead of indices to remove:

def keep_slices(file_in, file_out, slices):
    """Copy file_in to file_out, keeping the given slices."""
    with open(file_in) as fin, open(file_out, 'w') as fout:
        write = fout.write
        for line in fin:
            for s in slices:
                write(line[s])

This is substantially faster:

>>> timeit(lambda:keep_slices('in.txt', 'out.txt', slices), number=1)
1.877711878001719

That's about an 18 times speedup (on this test case; it might be different on other test cases). Is that good enough for you? It ought to reduce your 20 hour runtime to an hour or so.