ObservingCache - scheduled task executor that returns a result I wrote a utility class called ObservingCache, which receives a task in the form of Supplier<T> and an interval, computes the task every [interval] millis and allows its result to be queried via the getItems() method.
I'm using for making sure that outside resources (such as configuration files or DB tables) are always in sync with the app and can be modified without having to restart the application.

The code is as follows:

public class ObservingCache<T> {
  private static final int DEFAULT_CACHE_REFRESH_INTERVAL = 10 * 60 * 1000; // 10 minutes
  private static final int DEFAULT_THREAD_POOL_SIZE = 10;

  private static volatile ScheduledExecutorService executor;

  protected T items;

  public ObservingCache(Supplier<? extends T> syncFunc) {
      this(syncFunc, DEFAULT_CACHE_REFRESH_INTERVAL, true);
  }

  public ObservingCache(Supplier<? extends T> syncFunc, boolean firstRunBlocking) {
      this(syncFunc, DEFAULT_CACHE_REFRESH_INTERVAL, firstRunBlocking);
  }

  public ObservingCache(Supplier<? extends T> syncFunc, int intervalMillis) {
      this(syncFunc, intervalMillis, true);
  }

  public ObservingCache(Supplier<? extends T> syncFunc, int intervalMillis, boolean firstRunBlocking) {
      initExecutor();
      Runnable task = () -> {
          T result = syncFunc.get();
          if (result != null) {
              items = result;
          }
      };
      if (firstRunBlocking) {
          task.run(); // First run is blocking (saves a lot of trouble later).
      }
      executor.scheduleAtFixedRate(task, firstRunBlocking ? intervalMillis : 0, intervalMillis, TimeUnit.MILLISECONDS);
  }

  private void initExecutor() {
      if (executor == null || executor.isShutdown()) {
          synchronized (this) {
              if (executor == null || executor.isShutdown()) {
                  executor = Executors.newScheduledThreadPool(DEFAULT_THREAD_POOL_SIZE);
              }
          }
      }
  }

  public T getItems() {
      return items;
  }
}

I'd really like to hear your opinion about this piece of code, especially regarding the subject of potential memory leaks caused by either a bug of mine or any known issue with Java 8's ScheduledExecutorService class.

Class is open to modification

ObservingCache should be closed for modification, this is the OCP principle. If it's not, its behaviour can be altered (by inheritance for example) and can lead to an unpredictable behaviour which make it rather difficult to test and maintain.

How to achieve that ? Mark ObservableCache final.

Executor never shutdowned

Once started, the executor can never get shutdown, which causes the task to be run indefinitly.

How to achieve that ? Create a method:

/**
 * Once called, the cached value won't be updated anymore.
 */
public void stopObserving() {
    executor.shutdownNow();
}

firstRunBlocking

This parameter is the thing that bothers me the most in your class, and also the one who gave me the most trouble when trying to remove it.

Why does it bothers me ? Everytime you have a boolean in a method (or constructor), it's a sign of poor design because the class/method should do 2 things now (one for each boolean's value), therefore violating the single responsibility principle. It makes your code hardest to test and to maintain because of the conditionnal logic flow.

If I understand correctly, the purpose of this flag is to avoid client code retrieving a null when calling getItem because the cache has not been updated at least once. I would rather resolve this problem by blocking in getItem as long as the cache has not been computed once. In Java, one have the CountDownLatch who is a thread-safe class able to fulfill this responsibility by blocking only until the cache is computed the first time.

I came up with the following wrapper to achieve that:

public class BlockingHolder<T> {
    private T value;
    private final CountDownLatch barrier = new CountDownLatch(1);

    public T get() {
        try {
            barrier.await();
            return value;
        } catch (InterruptedException e) {
            throw new RuntimeException(e);
        }
    }

    public void set(T value) {
        this.value = value;
        barrier.countDown();
    }
}

Final solution

All these remarks put together, I came up with the following solution. Note that BlockingHolder is an inner class because it doesn't have to be known in the outside world.

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;

public final class ObservingCache<T> {
    private final BlockingHolder<T> holder;
    private final ScheduledExecutorService executor;

    /**
     * The cache will be refreshed every 10 minutes
     */
    public ObservingCache(Supplier<? extends T> syncFunc) {
        this(syncFunc, 10 * 60 * 1000);
    }

    public ObservingCache(Supplier<? extends T> syncFunc, int refreshIntervalMillis) {
        this.holder = new BlockingHolder<>();
        this.executor = Executors.newScheduledThreadPool(1);
        executor.scheduleAtFixedRate(() -> holder.set(syncFunc.get()), 0, refreshIntervalMillis, TimeUnit.MILLISECONDS);
    }

    /**
     * Blocks until the cached value has been computed at least once
     */
    public T getItem() {
        return holder.get();
    }

    /**
     * Once called, the cached value won't be updated anymore.
     */
    public void stopObserving() {
        executor.shutdownNow();
    }

    private static class BlockingHolder<T> {
        private T value;
        private final CountDownLatch barrier = new CountDownLatch(1);

        public T get() {
            try {
                barrier.await();
                return value;
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
        }

        public void set(T value) {
            this.value = value;
            barrier.countDown();
        }
    }
}

Update: taking into account firstRunBlocking

This is how I would fully replace a boolean into a business class. It looks like overkill but in reality it's not. Each class has a single responsibility, is testable and maintainable.

ObservingCache is now abstract because the "blocking" behaviour can't be defined here but only in suclasses.

import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;

public abstract class ObservingCache<T> {
    /**
     * 10 minutes
     */
    public static final int DEFAULT_CACHE_REFRESH_INTERVAL = 10 * 60 * 1000;

    private final ScheduledExecutorService executor;

    public ObservingCache(Supplier<? extends T> syncFunc, int refreshIntervalMillis) {
        this.executor = Executors.newScheduledThreadPool(1);
        executor.scheduleAtFixedRate(() -> setItem(syncFunc.get()), 0, refreshIntervalMillis, TimeUnit.MILLISECONDS);
    }

    public abstract T getItem();

    protected abstract void setItem(T value);

    /**
     * Once called, the cached value won't be updated anymore.
     */
    public final void stopObserving() {
        executor.shutdownNow();
    }
}

BlockingCache contains only notion of blocking until the value is not computed. How the value is computed is totally abstracted here.

import java.util.concurrent.CountDownLatch;
import java.util.function.Supplier;

public final class BlockingCache<T> extends ObservingCache<T> {
    private final BlockingHolder<T> holder;

    public BlockingCache(Supplier<? extends T> syncFunc, int refreshIntervalMillis) {
        super(syncFunc, refreshIntervalMillis);
        this.holder = new BlockingHolder<>();
    }

    /**
     * Blocks until the cached value has been computed at least once
     */
    @Override
    public T getItem() {
        return holder.get();
    }

    @Override
    protected void setItem(T value) {
        holder.set(value);
    }

    private static class BlockingHolder<T> {
        private T value;
        private final CountDownLatch barrier = new CountDownLatch(1);

        public T get() {
            try {
                barrier.await();
                return value;
            } catch (InterruptedException e) {
                throw new RuntimeException(e);
            }
        }

        public void set(T value) {
            this.value = value;
            barrier.countDown();
        }
    }
}

And finally, the cache that doesn't block but can return null (that I personnaly would never use):

import java.util.function.Supplier;

public final class NonBlockingCache<T> extends ObservingCache<T> {
    private volatile T value;

    public NonBlockingCache(Supplier<? extends T> syncFunc, int refreshIntervalMillis) {
        super(syncFunc, refreshIntervalMillis);
    }

    /**
     * Never blocks. Returns null if the value has not been computed !
     * Otherwise, returns the cached value.
     */
    @Override
    public T getItem() {
        return value;
    }

    @Override
    protected void setItem(T value) {
        this.value = value;
    }
}


PHP e-mail form with ReCAPTCHA I want to ask if you can review the code for a simple contact form and the PHP code which sends me an email once a visitor fills the form and is validated through Recaptcha

On my index.php file here are the form and php code

HTML:

<form id="contactform" action="index.php#contact" method="post" class="form" role="form">

      <input class="form-control required" id="vname" name="vname" placeholder="Your Name" type="text" required />

      <input class="form-control required" id="vemail" name="vemail" placeholder="Your Email" type="email" required />

      <textarea class="form-control required" id="msg" name="msg" placeholder="Your Message" rows="7" required></textarea>

      <input type="submit" class="btn btn-success form-send" value="Send">

</form>

PHP:

<?php
  $captcha;
    if (isset($_POST['g-recaptcha-response'])) { $captcha = $_POST['g-recaptcha-response']; }
  // Check for correct reCAPTCHA
    $response = file_get_contents("https://www.google.com/recaptcha/api/siteverify?secret=GOOGLE SECRET KEY&response=" . $captcha . "&remoteip=" . $_SERVER['REMOTE_ADDR']);
    if (!$captcha || $response.success == false) {
        echo "Your CAPTCHA response was wrong";
        exit ;
      } else {
        // Check for Blank Fields..
        if ($_POST["vname"] == "" || $_POST["vemail"] == "" || $_POST["msg"] == "") {
    echo "Please fill all required fields";
} else {
    // Check if the "Sender's Email" input field is filled out
    $email = $_POST['vemail'];
    // Sanitize E-mail Address
    $email = filter_var($email, FILTER_SANITIZE_EMAIL);
    // Validate E-mail Address
    $email = filter_var($email, FILTER_VALIDATE_EMAIL);
    if (!$email) {
        echo "Invalid Sender's Email";
    } else {
        $to = 'myemail@mail.com';
        $subject = 'New Form Entry';
        $message = "New message was submitted from
 " . "" . $_POST['vname'] . "" . "

The message is:
" . "" . $_POST['msg'] . "";
        $headers = "From:" . $_POST['vname'] . "<" . $email . ">";
        $headers .= "MIME-Version: 1.0" . "\r\n";
        $headers .= "Content-type:text/html;charset=UTF-8" . "\r\n";
        // Sender's Email
        // Message lines should not exceed 70 characters (PHP rule), so wrap it
        $message = wordwrap($message, 70, "\r\n");
        // Send Mail By PHP Mail Function
        if (mail($to, $subject, $message, $headers)) {
            echo "Your mail has been sent successfully!";
        } else {
            echo "Failed to send email, try again.";
            exit ;
        }
    }
}
}
?>

The only function of this is to send me an email and it works as I want it, but I am not aware if this form is secure?

Is it possible that someone can exploit this code and upload a shell, or do any other sort of attack against my site.

Since $_POST['vname'] is not escaped, your mail script is vulnerable to a header-splitting attack like this.  A cleverly crafted name could be used to make your script CC any arbitrary recipient and thus send spam.


Simple Hangman for Python Assignment Criteria
Create another version of the Hangman game, this time using Lists. The program should ask for the word to guess and the number of chances to be given.
It should then split the characters in the word into individual items in a list. The other player should then be allowed to guess characters in the word. The program should display correctly guessed characters and unknown characters in the same way as the previous Hangman game.

This a simple Hangman game in Python and I was just wondering if there was any code improvements that could be done with this. I can definitely see a few, but they're very minor changes (e.g. spaces can be removed as it's redundant and never really gets used.)
# ========== Variables ============ #
letter_bank = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

guessed = False  # Word has been guessed. (boolean)

spaces = ""      # Blanks
word_guess = []  # Placeholder
word_chars = []  # Characters in Word.

word = ""      # Word being guessed.
max_guess = None # Maximum amount of Guess.
cur_guess = 0 # Current count of Guesses.
# ================================= #
def findOccurences(s, ch):
    return [i for i, letter in enumerate(s) if letter == ch]

print('''
    Welcome to Hangman! Simplified version of the original Hangman.
''')

word = input("Please enter a word: ").lower()
max_guess = int(input("Maximum Guess: "))

for character in word:
    word_chars.append(character)
    if (character != " "):
        spaces += "_ "
        word_guess.append("-")
    else:
        spaces += ("  ")
        word_guess.append("  ")

print("\n" * 50)

print('''
    You are now ready to play.
''')

print("Word: %s. " % (spaces))
while (cur_guess < max_guess) and guessed == False:
    if (word.split() == (''.join(word_guess)).split()):
        guessed = True
        print('''
            You have successfully guessed the word!
        ''')
        break;

    letter_guess = input("Please enter your guess: ").lower()
    if (len(letter_guess) == 1):
        if (letter_guess not in letter_bank):
            print("You have already used that letter!") # Prints if the character is invalid or already used.
        else:
            if (letter_guess in word_chars):
                print("%s is in the word!" % letter_guess)
                letter_bank.remove(letter_guess)
                for each in findOccurences(word, letter_guess):
                    word_guess[each] = letter_guess
                print(''.join(word_guess))
            else:
                print(''.join(word_guess))
                print("%s is not in the word!" % letter_guess)
                letter_bank.remove(letter_guess)
                cur_guess += 1
    else:
        if (letter_guess == word):
            print('''
                You have successfully guessed the word!
            ''')
        else:
            print('''
                You have lost the game! :(
            ''')
            break;

Python is not C. You don't need to declare variables at the beginning and you don't need to set them to empty values if the first thing you do in your code is set them to sensible values.

Python is also unlike C in the sense that you don't need () around statements. while expression is perfectly valid (no need for while (expression) and so is if expression.

Python is unlike C in that it does not need ; at the end of lines. While it is not a SyntaxError, it is only needed when you absolutely have to put multiple statements on the same line (e.g. sometimes when using the commandline). This is basically never the case when writing a script in a file.

Python is like C in the sense that it has sensible operator preferences. Instead of while (cur_guess < max_guess) and guessed == False you can write while cur_guess < max_guess and not guessed and the < expression will be evaluated before the and.

A second thing I did there was removing the direct comparison with False. This is almost never needed (same as direct comparison with True), because the value itself is already a bool (and if not, most types have truthy and falsy values which behave just like bools, e.g. for strings the empty string "" is falsy, every other string is thruthy).

Your letter_bank would be better suited for a set, which has fast membership tests (since you do a lot of if letter_guess not in letter_bank. And since you are using the whole lowercase alphabet, why not use the built-in module string to save yourself some typing (next time):

import string
letter_bank = set(string.ascii_lowercase)

You don't even need to change letter_bank.remove(letter_guess), because it is the same for a set.

The same is true for word_chars.

Python has an official style-guide, PEP8, that recommends using lower_case_with_underscores for variables and functions. So your findOccurences would become find_occurences.

To increase readability, I would put more of your functionality into functions. At least have a setup() and a game() function to separate these two parts of logic.

With these (rudimentary) recommendations, your code would become:

import string

def find_occurences(s, ch):
    return [i for i, letter in enumerate(s) if letter == ch]

def game(word, max_guess):
    letter_bank = set(string.ascii_lowercase)
    word_chars = set(word)
    if word_chars - letter_bank:
        print("Words can only contain ascii letters!")
        return False
    word_guess = list(" ".join("-" * len(w) for w in word.split()))
    cur_guess = 0  # Current count of Guesses.

    while cur_guess < max_guess:
        print(''.join(word_guess))
        if word == ''.join(word_guess):
            return True

        letter_guess = input(
            "Please enter your guess (%i remaining):" % (max_guess - cur_guess)).lower()
        if len(letter_guess) != 1:
            return letter_guess == word
        elif letter_guess not in letter_bank:
            print("You have already used that letter or it is not a valid letter!")
        elif letter_guess in word_chars:
            print("%s is in the word!" % letter_guess)
            letter_bank.remove(letter_guess)
            for each in find_occurences(word, letter_guess):
                word_guess[each] = letter_guess
        else:
            print("%s is not in the word!" % letter_guess)
            letter_bank.remove(letter_guess)
            cur_guess += 1
        print()
    return False

def main():
    print('Welcome to Hangman! Simplified version of the original Hangman.')

    word = input("Please enter a word: ").lower()
    max_guess = int(input("Maximum Guess: "))

    print("\n" * 50)
    print('You are now ready to play.')

    if game(word, max_guess):
        print('You have successfully guessed the word!')
    else:
        print('You have lost the game! :(\nThe word was %s' % word)

if __name__ == "__main__":
    main()


Correct practice for Python properties that depend on other attributes When a property is essentially a boolean value for whether or not an attribute exists or is set a certain way, is it more appropriate to create an object that does not list the option attribute in its __init__() call, or is it better to simply create the object with the 'falsiest' version of that attribute?

Consider the following class:

class RectLikeObj(object):
    def __init__(self, x, y, w, h, pinned=None):
        self.x = x
        self.y = y
        self.w = w
        self.h = h
        self.pinned = pinned or ()

    ###series of property getter/setters for attributes that are
    ###derived from the above
    @property
    def topleft(self):
        return (self.x, self.y)

    @topleft.setter
    def topleft(self, value):
        self.x, self.y = value

    ###and so on for all corners and center, etc

    @property
    def is_pinned(self):
        if not self.pinned:
            return False
        return True

    def pin_corner(self, corner, xy):
        self.pinned = (corner, xy)

Alternatively the object may not ever have the self.pinned attribute and the property is instead looking for the attribute.

class RectLikeObj(object):
    def __init__(self, x, y, h, w):
        self.x = x
        self.y = y
        self.w = w
        self.h = h

    ###same getters/setters as above

    @property
    def is_pinned(self):
        if getattr(self, 'pinned', False):
            return True
        return False

    def pin_corner(self, corner, xy):
        self.pinned = (corner, xy)

Now it could be done to use a try/except, only there's nothing exceptional about this behavior; it's quite binary, either the Rect object is pinned to a particular corner and xy coordinates or it isn't. So I don't know that using eafp is the most correct choice here.

Is it better to use a 'falsy' attribute such as self.pinned = () in the __init__() method, or is it better to leave it to be set later by the self.pin_corner() method and then use the property to check for its existence?

I vote the first method (with the change of having () as the default argument instead of None, because it's simpler).

If you don't absolutely need code introspection, don't use it. In my mind it's more cluttered and risks confusing editors such as PyCharm, as well as human beings, about the composition of a class. To take this idea a little further, there's no need for "pinned" to accept a tuple in the constructor. Give it the same contract as pin_corner; that is, accept both arguments and default them both to None. There's less validation required - when accepting a tuple you would need to check its length as well as the validity of its members.


Automatically turn on Computer from Raspberry Pi (based on user presence in home) I know the code is full of useless data and stylistic and analysis errors and I'd really love and appreciate to have a pro's idea about it so I can learn to code (and think) better.

import os, time
from ConfigParser import SafeConfigParser
configfilename = "imhome.ini"
scriptpath = os.path.abspath(os.path.dirname(__file__))
configfile = os.path.join(scriptpath, configfilename)
parser = SafeConfigParser()
parser.read(configfile)
ip_smartphone = parser.get('imhome', 'ip_smartphone')
mac_address_pc = parser.get('imhome', 'mac_address_pc')
hometime_path = parser.get('imhome', 'hometime_path')
in_path = parser.get('imhome', 'in_path')
mp3_path = parser.get('imhome', 'mp3_path')
maximum_time_before_logout = parser.get('imhome', 'maximum_time_before_logout')
if os.path.exists(hometime_path) == False:
        os.system ("touch " + str(hometime_path))
if os.path.exists(in_path) == True:
        os.system ("rm " + str(in_path))
finished = 1
while finished == 1:
    check_presence = os.system ("ping -w 1 " + str (ip_smartphone) + " >/dev/null")
    file_modification_time = (os.path.getmtime(hometime_path))
    file_delta_time = time.time() - file_modification_time
    if check_presence == 0 :
            os.system ("rm " + str(hometime_path))
            change_arrived=("echo " + str(check_presence) + " > hometime")
            os.system (change_arrived)

            if os.path.exists(in_path) == False:
            # You can add any custom commands to execute when home below this row

                os.system ("etherwake " + str(mac_address_pc))
                os.system ("mpg123 " + str(mp3_path) + " >/dev/null")
                os.system ("touch " + str(in_path))
            # stop adding commands

    elif check_presence == 256 and file_delta_time > int(maximum_time_before_logout):
        if os.path.exists(in_path) == True:
                    os.system ("rm " + str(in_path))

1. Comments on your code

You make quite a lot of use here of os.system, which runs a command in a subshell. This can be a risky function to use, because the shell can do anything. For example, you write

os.system ("rm " + str(hometime_path))

but what if hometime_path were -rf *?

You're probably safe in this particular program because it's only for your own use and because the strings come from your own configuration file. But it's worth getting into the habit of doing this kind of thing properly, so that in situations which really are risky, you'll know what to do.

In the case of rm you can use os.unlink:

os.unlink(hometime_path)

But in the general case, use subprocess.call and use the -- argument (if necessary) to make sure that the arguments you pass cannot be misinterpreted as options. For example,

subprocess.call(['rm', '--', hometime_path])

In a case where you want to redirect standard output, like this:

os.system ("ping -w 1 " + str (ip_smartphone) + " >/dev/null")

you can use the stdout keyword argument to subprocess.call:

subprocess.call(['ping', '-w', '1', '--', ip_smartphone],
                stdout = open('/dev/null', 'w'))

Most of your calls to str are unnecessary: these configuration parameters ought to be strings already. (If they are not, I think it you'd prefer to get an exception so that you could fix the problem, rather than silently coercing the incorrect value to a string.)
You don't need to compare booleans explicitly to True and False. So instead of:

if os.path.exists(in_path) == True:

just write:

if os.path.exists(in_path):

and instead of:

if os.path.exists(hometime_path) == False:

just write:

if not os.path.exists(hometime_path):

In this code you remove a file only if it exists:

if os.path.exists(in_path) == True:
    os.system ("rm " + str(in_path))

But the rm command already has a option for this, so you could write instead:

subprocess.call(['rm', '-f', '--', in_path])

Or do it in pure Python:

try:
    os.unlink(in_path)
except OSError: # No such file or directory
    pass

(This is an example of how it is often better to ask for forgiveness than permission.)
Assuming that " > hometime" is a mistake for " > " + hometime_path (as suggested in another answer), then in this bit of code:

os.system ("rm " + str(hometime_path))
change_arrived=("echo " + str(check_presence) + " > hometime")
os.system (change_arrived)

you remove a file and then overwrite it with the value of check_presence. There's no need to remove the file first: the second command will overwrite it if it exists.

And in any case this is easy to do in pure Python:

with open(hometime_path, 'w') as f:
    f.write('{}\n'.format(check_presence))

You use the modification time of the file hometime_path to remember the last time that your ping command succeeded. Why not remember this time in a variable in Python and avoid having to create and delete and examine this file?
You use the existence of the file in_path to remember whether your smartphone is connected. Why not remember this fact in a variable in Python and avoid having to create and delete and examine this file?

2. Rewrite

Making the above improvements results in something like this (but this is untested, so beware):

import os, subprocess, time
from ConfigParser import SafeConfigParser
from datetime import datetime, timedelta

configfilename = "imhome.ini"
scriptpath = os.path.abspath(os.path.dirname(__file__))
configfile = os.path.join(scriptpath, configfilename)
parser = SafeConfigParser()
parser.read(configfile)
ip_smartphone = parser.get('imhome', 'ip_smartphone')
mac_address_pc = parser.get('imhome', 'mac_address_pc')
mp3_path = parser.get('imhome', 'mp3_path')
timeout = timedelta(seconds = int(parser.get('imhome', 'maximum_time_before_logout')))

devnull = open('/dev/null', 'wb')
connected = False
disconnect_time = None

while True:
    ping = subprocess.call(['ping', '-w', '1', '--', ip_smartphone],
                           stdout = devnull)
    if ping == 0:
        disconnect_time = datetime.now() + timeout
        if not connected:
            connected = True
            subprocess.call(['etherwake', mac_address_pc])
            subprocess.call(['mpg123', mp3_path], stdout = devnull)
    elif ping == 256 and connected and datetime.now() > disconnect_time:
        connected = False
    time.sleep(1)


Is there any way to condense my code so it does not take up so many lines? I'm writing my program in python and have it working properly, but it's over 500 lines of code. I think there is a way I can condense my code, but I'm not sure how I should do it
Here's an example of some of my code:
def click():
    if clicked1.get() == "app1":
        os.startfile(r"C:\Users\red\Desktop\app1")
        i = 0
        while i < 20:
            try:
                app.connect(title_re=".*app1")
                break
            except pywinauto.findwindows.ElementNotFoundError:
                time.sleep(1)
                i += 1
        app1 = app.top_window()
        app1.move_window(x1, y1)
        time.sleep(.5)
        app1.maximize()
    if clicked1.get() == "app2":
        os.startfile(r"C:\Program Files (x86)\app2")
        i = 0
        while i < 20:
            try:
                app.connect(title_re=".*app2")
                break
            except pywinauto.findwindows.ElementNotFoundError:
                time.sleep(1)
                i += 1
        app1 = app.top_window()
        app1.move_window(x1, y1)
        time.sleep(.5)
        app1.maximize()

I basically have about 20 of those if statements for each different
application and for each different clicked (1-4). Is there any way I can shrink my code so it doesn't take up so many lines? Perhaps using variables?

When refactoring code that is not DRY then try to identify the parts that actually do change and put those parts into some kind of data structure like a list or dictionary. Then wrap everything into a function.
apps = {'app1': r"C:\Users\red\Desktop\app1",
        'app2': r"C:\Program Files (x86)\app2",
        ...
       }

selected_app = clicked1.get()

def click():
    os.startfile(apps.get(selected_app))
    i = 0
    while i < 20:
        try:
            app.connect(title_re=".*" + selected_app)
            break
        except pywinauto.findwindows.ElementNotFoundError:
            time.sleep(1)
            i += 1
    app1 = app.top_window()
    app1.move_window(x1, y1)
    time.sleep(.5)
    app1.maximize()


Interpret English as Math and Perform Operations The idea is to accept an English string of math operations and evaluate it from left to right.

Regarding Order of Operations: when speaking operations aloud, it is often with implied parenthesis that denotes a left-to-right chain.

For example:

  4 plus 3 times 2 squared
   === (((4 plus 3) times 2) squared)

The rhythm which it was spoken dictates whether or not to square the 2 before multiplying — but in this case the goal is to go strictly left-to-right.

The starting point to my review here was my answer to a StackOverflow question which was direct, correct, but unsatisfying in design and style.

import math
import re
import operator

# try "3 add 4 times 5"
inputs = input(">").lower()

# Using RE to find all numbers in string.
numInString = [int(d) for d in re.findall(r'-?\d+', inputs)]
inputs = inputs.split()

print('Numbers found: ', numInString)
print('Inputs found: ', inputs)

# Define all of the operators
def multiplication():
    multAns = operator.mul(numInString.pop(0) , numInString.pop(0))
    numInString.insert(0, multAns)
    print(multAns)

def division():
    divAns = operator.truediv(numInString.pop(0) , numInString.pop(0))
    numInString.insert(0, divAns)
    print(divAns)

def addition():
    addAns = operator.add(numInString.pop(0) , numInString.pop(0))
    numInString.insert(0, addAns)
    print(addAns)

def subtraction():
    subAns = operator.sub(numInString.pop(0) , numInString.pop(0))
    numInString.insert(0, subAns)
    print(subAns)

def squareRoot():
    SqrtAns = math.sqrt(numInString.pop(0))
    numInString.insert(0, SqrtAns)
    print(SqrtAns)

def squared():
    SquareAns = math.pow(numInString.pop(0), 2)
    numInString.insert(0, SquareAns)
    print(SquareAns)

def cubed():
    CubedAns = math.pow(numInString.pop(0), 3)
    numInString.insert(0, CubedAns)
    print(CubedAns)

def power():
    PowerAns = math.pow(numInString.pop(0), numInString[1])
    numInString.insert(0, PowerAns)
    print(PowerAns)

# Dictionary mapping search words to operator function
Operation = {
    'multiply': multiplication,
    'times' : multiplication,
    'multiplied': multiplication,
    'divide': division,
    'divided': division,
    'into': division,
    'add': addition,
    'sum': addition,
    'added': addition,
    'subtract': subtraction,
    'minus': subtraction,
    'take': subtraction,
    'subtracted': subtraction,
    }

for words in inputs:
    if words in Operation:
        print(Operation[words]())

How should I be naming variables? Are Operation and numInString good names?
Should I print the result of a function operator?
Should I be storing the numbers in a global list (e.g. numInString) instead of passing them to operator functions?
Should I print errors like this?:

print('NO OPERATION GIVEN')

Is it okay to assume all numbers will be of type int?
Should I leave unguarded script code such as input('>')?

How should I be naming variables? Operation and numInString good names?

Great question, Python has an extensive but approachable style guide call PEP8. Specifically local variables should be lowercase, Global Constants should be UPPERCASE, and Global variables should be avoided.

  Should I print the result of a function operator?

In order to use it later you should return the value:

Instead of: print(multAns) do return multAns

  Should I be storing the numbers in a global list, numInString, instead of passing them to operator functions?

No, pass the numbers to the operator and save the returned result

result = oper(a=1, b=3)

instead of

a = 1
b = 3
oper()

  Should I print errors like this?:

print('NO OPERATION GIVEN')

No, raise errors when you come to this sort of situation.

ValueError("Cannot match an operator to \"{}\"".format(inputstring))

  Is it ok to assume all numbers will be int?

It would be better to handle the case of decimal number, especially if you want to be able to do true division (which was in your code)

My revision with plenty of comments, incorporating my above suggestions

import re
from decimal import Decimal

Dictionary mapping search words to operator function

operations = {
    '<a> (times|multiplied by) <b>':
        lambda a, b: a * b,
    '<a> divided by <b>':
        lambda a, b: a / b,
    '(add|sum)? <a> (and|plus) <b>':
        lambda a, b: a + b,
    'subtract <a> and <b>':
        lambda a, b: a - b,
    '<a> (minus|take) <b>':
        lambda a, b: a - b,
    '<a> squared':
        lambda a: a**2,
    '<a> cubed':
        lambda a: a**3,
    '<a> raised to the power <b>':
        lambda a, b: a**b,
    }

think of lambda as "make_function"

def plus(a, b):
    return a + b

is the same as

plus = lambda a, b: a + b

also you can use the actual operator:
    eg. * instead of operator.mul
        + instead of add

Match any number eg. 8, 8.8, -3.14 https://stackoverflow.com/questions/15814592/how-do-i-include-negative-decimal-numbers-in-this-regular-expression this is defined as a Global Constant

NUM_RE = "-?\d+(\.\d+)?"

Match that number and give it a name that can be
access later via m.groupdict()

def named_number_regex(name):
    return "(?P<{name}>{num})".format(name=name, num=NUM_RE)

variable_names = ['a', 'b']
for name in variable_names:
    # replace all of the name placeholders with
    # the actual regex for matching a named number
    #     eg. "<a>" ---> "(?P<a>-?\d+(\.\d+)?)"
    # placeholder were used just to make the above patterns easier to read
    old = '<{}>'.format(name)
    new = named_number_regex(name)
    operations = {k.replace(old, new): v for k, v in operations.items()}

# allow for varying amount of whitespace
operations = {'\s*' + k.replace(' ', '\s*'): v for k, v in operations.items()}

Now the patterns look like this:

#
# \s*(?P<a>-?\d+(\.\d+)?)\s*squared
#
# let's break it down:
#
# \s*                                 ignore leading whitespace
#    (?P<a>-?\d+(\.\d+)?)             match any number, name it 'a'
#                        \s*          ignore other whitespace
#                           squared   only match if you find " squared"

Function to take a string and and recursively simplify it until we either get a number or encounter an error.

def simplify(inputstring):
    print(inputstring) # see what is going on internally

    # if you get down to a single number return its value
    if re.fullmatch(NUM_RE, inputstring) is not None:
        return Decimal(inputstring)

    for matcher, oper in operations.items():
        # iterate over all key: value pairs in the dict "operations" eg:
        #    matcher = '(?P<a>\d+) (times|multiplied by) (?P<b>\d+)'
        #    oper = lambda a, b: operator.mul(a, b)
        m = re.match(matcher, inputstring)
        if m is None:
            continue

        # dict of named matches eg {'a': '4', 'b': '8'}
        numbers = m.groupdict()

        # convert text eg. '4' strings into integers eg. 4
        # this is like your [int(d) for d in re.findall(r'-?\d+', inputs)]
        # Decimal handles ints and floats, with floating point rounding errors
        numbers = {k:Decimal(v) for k, v in numbers.items()}

        # now numbers = {'a': 4, 'b': 8}

        # get the result of the matched operation
        # Note: ** means call function with keyword args from a dict
        #     eg. oper(a=4, b=8)
        result = oper(**numbers)

        # substitute the result of oper with the matched string
        # Note we can lose precision here when casting Decimal to str
        simplified = re.sub(matcher, str(result), inputstring)

        # now simplify it further if possible
        # this is call recursion, it will continue to simplify until we get down to just a number
        print('= ', sep='', end='')
        return simplify(simplified)

    raise ValueError("Cannot match an operator to \"{}\"".format(inputstring))

Test Cases

def test_a_few_cases():
    print("\nSimple Addition:")
    result = simplify("4 plus 8")

    print("\nDemonstrate the recursion:")
    result = simplify("2 squared squared squared squared")

    print("\nMore elaborate:")
    result2 = simplify("2 plus 1.11 times -7 raised to the power 6")

    print("\nDemonstrate an Error:")
    result = simplify("2 squared squared squared squard")

def main():
    print("\nYour input:")
    yourresult = simplify(input(">").lower())

if __name__ == '__main__':
    test_a_few_cases()
    main()


Inserting into database columns I am trying to insert into a database using JDBC and each thread will be inserting into the database. I need to insert into around 30-35 columns. I wrote a stored procedure that will UPSERT into those columns.

The problem I am facing is, if you look at my run method, I have around 30 columns written over there for insertion. Is there any way I can simplify my run method so that it doesn't looks so messy which is looking right now for me? And I have few more columns as well. So if I keep on adding new columns there, it will be looking so messy at one point in my run method.

Are there any way to make this look cleaner, keeping in mind thread safety issues?

class Task implements Runnable {

    private Connection dbConnection = null;
    private CallableStatement callableStatement = null;

    public Task() {

    }

    @Override
    public void run() {

    dbConnection = getDbConnection();
    callableStatement = dbConnection.prepareCall(Constants.UPSERT_SQL);

    callableStatement.setString(1, String.valueOf(userId));
    callableStatement.setString(2, Constants.getaAccount(userId));
    callableStatement.setString(3, Constants.getaAdvertising(userId));
    callableStatement.setString(4, Constants.getaAvgSellingPriceMainCats(userId));
    callableStatement.setString(5, Constants.getaCatAndKeywordRules(userId));
    callableStatement.setString(6, Constants.getaClvBehavior(userId));
    callableStatement.setString(7, Constants.getaClvChurn(userId));
    callableStatement.setString(8, Constants.getaClvInfo(userId));
    callableStatement.setString(9, Constants.getaClvSegmentation(userId));
    callableStatement.setString(10, Constants.getaCsaCategoriesPurchased(userId));
    callableStatement.setString(11, Constants.getaCustomerService(userId));
    callableStatement.setString(12, Constants.getaDemographic(userId));
    callableStatement.setString(13, Constants.getaFinancial(userId));
    callableStatement.setString(14, Constants.getaGeolocation(userId));
    callableStatement.setString(15, Constants.getaInterests(userId));
    callableStatement.setString(16, Constants.getaLastContributorsPurchased(userId));
    callableStatement.setString(17, Constants.getaLastItemsLost(userId));
    callableStatement.setString(18, Constants.getaLastItemsPurchased(userId));
    callableStatement.setString(19, Constants.getaLastProductsPurchased(userId));
    callableStatement.setString(20, Constants.getaLastSellersPurchasedFrom(userId));
    callableStatement.setString(21, Constants.getaMainCategories(userId));
    callableStatement.setString(22, Constants.getaMessaging(userId));
    callableStatement.setString(23, Constants.getaPositiveSellers(userId));
    callableStatement.setString(24, Constants.getaPromo(userId));
    callableStatement.setString(25, Constants.getaScores(userId));
    callableStatement.setString(26, Constants.getaSegmentation(userId));
    callableStatement.setString(27, Constants.getaSellers(userId));
    callableStatement.setString(28, Constants.getaSrpBuyerUpiCount(userId));
    }
  }

    private Connection getDBConnection() {

    Connection dbConnection = null;

          Class.forName(Constants.DRIVER_NAME);
      dbConnection = DriverManager.getConnection(url,user,password);

      return dbConnection;
    }

This is my main thread code from which I am creating threads:

//create thread pool with given size
ExecutorService service = Executors.newFixedThreadPool(noOfThreads);

try {
    // queue some tasks
    for (int i = 0; i < noOfTasks * noOfThreads; i++) {
        service.submit(new Task());
    }
    service.shutdown();
    service.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);

    while (!service.isTerminated()) {

    }
} catch (Exception e) {

}

You can use the following utility method:

public static void setStrings (CallableStatement stmt, Object ... values)
{
    for (int count = values.length, i = 0; i < count; i++)
        stmt.setString (i + 1, String.valueOf (values [i]));
}

in your run() like this:

setStrings (
    callableStatement,
    userId,
    Constants.getaAccount (userId),
    Constants.getaAdvertising (userId),
    ...
    Constants.getaSrpBuyerUpiCount (userId));

This is shorter and much more readable for me.


Convert Integer to Roman Numeral I've created a function that takes an integer greater than zero as an input and converts it into its corresponding Roman numeral.  If you're not familiar with Roman numerals, you can read about it on Wikipedia.  I've tested this code with some examples, so it should be working correctly.

I'd be interested in hearing about any general improvements I could make on the code.  Nitpicky details are fine as well; whatever you have to say I'm willing to hear, but I hope your answer is more substantial than that.  I suppose my main concern is whether this is sufficiently efficient or if there is a way I could improve on that.  Also, do you think I've used the correct data structure in this situation?  I can't think of a better data structure for what I wanted to do, so that is why I chose arrays.

function arabicToRoman(num) {
    var roman = ["I", "IV", "V", "IX", "X", "XL", "L", "XC", "C", "CD", "D", "CM", "M"];
    var arabic = [1, 4, 5, 9, 10, 40, 50, 90, 100, 400, 500, 900, 1000];
    var result = "";

    while(num >= 4000) {
        result += "M";
        num -= 1000;
    }

    for(var i = arabic.length - 1; num > 0; i--) {
        while(num >= arabic[i]) {
            result += roman[i];
            num -= arabic[i];
        }
    }

    return result;
}

First, you can easily throw away semicolons ";".

Second, if you want shorter code, you can do like this:

function arabicToRoman(num) {
  var roman = {M:1000,CM:900,D:500,CD:400,C:100,XC:90,L:50,XL:40,X:10,IX:9,V:5,IV:4,I:1}
  var result = ''

  for (var i in roman) {
    while (num >= roman[i]) {
      result += i
      num -= roman[i]
    }
  }
  return result
}

But, it's also almost ten times slower, so it's up to you - do you need compact code, or performance?


Writing embedded list to CSV in Python I have a method in a Python app to write a list of records to a CSV file.  Each record is a "football player", which is really just a dictionary.  But one of the dictionary items contains a list of weekly scores.  I want to write out to CSV a record for each player, and then flatten the list of scores and write that out, too.

My code:

def write_csv_file(filename, year, list_of_players):
    print str(datetime.now()) + ' Writing file {0}'.format(OUTPUT_CSV_PATH + filename + '.csv')
    print list_of_players
    with open(OUTPUT_CSV_PATH + filename + '.csv', 'w+') as f:

        writer = csv.writer(f, quotechar='"', quoting=csv.QUOTE_MINIMAL, lineterminator = '\n')

        writer.writerow(['YEAR', 'FILENAME', 'PLAYER_KEY', 'PLAYER_NAME', 'SEASON_TOTAL', 'CALCULATED_SEASON_TOTAL', 'MEAN', 'MEDIAN', 'STD_DEVIATION', 'CV',
            'WK1', 'WK2', 'WK3', 'WK4', 'WK5', 'WK6', 'WK7', 'WK8', 'WK9', 'WK10', 'WK11', 'WK12', 'WK13', 'WK14', 'WK15', 'WK16'])
        for item in list_of_players:
            writer.writerow([year, filename, item['player_key'], item['player_name'], item['season_total'],
                item['calculated_season_total'], item['mean'], item['median'], item['std_deviation'], item['coefficient_of_variation'],
                #item['performance_score'],
                item['scores'][0],
                item['scores'][1],
                item['scores'][2],
                item['scores'][3],
                item['scores'][4],
                item['scores'][5],
                item['scores'][6],
                item['scores'][7],
                item['scores'][8],
                item['scores'][9],
                item['scores'][10],
                item['scores'][11],
                item['scores'][12],
                item['scores'][13],
                item['scores'][14],
                item['scores'][15]
                ])

It's the handling of item['scores'] that I'm sure there's a better way.  I tried getting a list comprehension to work but never could get it going.

How can I do this better?

One quick win for readability would be to rename item to player.  Another improvement would be to put each field on its own line, since the long lines vastly exceed the 79-character limit suggested by PEP 8.

For the 16 scores, you can write…

writer.writerow([
    'YEAR',
    'FILENAME',
    'PLAYER_KEY',
    'PLAYER_NAME',
    'SEASON_TOTAL',
    'CALCULATED_SEASON_TOTAL',
    'MEAN',
    'MEDIAN',
    'STD_DEVIATION',
    'CV'
] + ['WK{0}'.format(i + 1) for i in range(16)])

for player in list_of_players:
    writer.writerow([
        year,
        filename,
        player['player_key'],
        player['player_name'],
        player['season_total'],
        player['calculated_season_total'],
        player['mean'],
        player['median'],
        player['std_deviation'],
        player['coefficient_of_variation']
    ] + player['scores'])


Choosing a Base Descriptor This code is very large and seems quite confusing. I tried to refactor it to make it more readable and make the code seem less...(scroll below). However, I am not sure if this can be refactored more.

 TableTypeDescriptor baseDecriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

        if (this.IsObjectBeingValidated)
            return baseDecriptor;

        if (base.CustomTypeDescriptor != null)
        {
            if (baseDecriptor.OldTableDescriptor != null && this.IsNewInCorrection && !this.IsReadOnly)
            {
                return base.CustomTypeDescriptor;
            }
            else if (this.Locked)
            {
                return base.CustomTypeDescriptor;
            }
            else if (this.ImportedDataMD5 != null && !this.IsReadOnly)
            {
                return base.CustomTypeDescriptor;
            }
            else
            {
                if (baseDecriptor.OldTableDescriptor != null)
                {
                    return baseDecriptor.OldTableDescriptor;
                }
                else
                    return base.CustomTypeDescriptor;
            }
        }

        return base.CustomTypeDescriptor;

I then refactored it as follows. Is it possible to refactor it more?

TableTypeDescriptor baseDescriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

                if (base.CustomTypeDescriptor != null || !this.IsObjectBeingValidated)
                {
                    if ( (!this.IsReadOnly && (this.IsNewInCorrection || this.ImportedDataMD5 != null))  ||
                         this.Locked
                       )
                        baseDescriptor = base.CustomTypeDescriptor;
                    else
                    {
                        if (baseDescriptor.OldTableDescriptor != null)
                            baseDescriptor = baseDecriptor.OldTableDescriptor;
                        else
                            baseDescriptor = base.CustomTypeDescriptor;
                    }
                }

                return baseDescriptor;

You should use braces {} for single if statements too. This makes your code less errorprone.

If you are refactoring some code you should be sure to have good tests at your hand to validate that the refactored code behaves like the old code.

By extracting the check for all these this.xx() properties to a separate method like

private static bool ShouldReturnCurrentDescriptor()
{
    return (this.IsObjectBeingValidated) ||
           (this.Locked) ||
           (!this.IsReadOnly && (this.IsNewInCorrection || this.ImportedDataMD5 != null));
}

The former code can be refactored to

TableTypeDescriptor baseDecriptor = base.CustomTypeDescriptor as TableTypeDescriptor;

if (ShouldReturnCurrentDescriptor() ||
    (baseDecriptor.OldTableDescriptor == null))
{
    return baseDecriptor;
}
return baseDecriptor.OldTableDescriptor;


A few sorting algorithms implemented in C I was reading about the elementary sorting algorithms like insertion Sort, selection sort and bubble Sort in Introduction to Algorithms by Cormen and I decided to try and implement it in C.

#include<stdio.h>
#define MAXSIZE 100  //  Maximum number of elements permissible in array

void swap(int*,int*);
void print(int*,int);

int main(void) {
    int arr[MAXSIZE],n,i,j,ch,ord;
    void insertion(int*,int,int),selection(int*,int,int),bubble(int*,int,int);
    printf("\nEnter number of elements for array = ");
    scanf("%d",&n);
    printf("\n\nEnter the array elements one by one : \n\n");
    for(i=0;i<n;i++){
        scanf("%d",&arr[i]);
    }
    printf("\n\nWhat sorting you want to use ?\n\n Insertion (1) , Selection (2) or Bubble (3)\n\nEnter choice = ");
    scanf("%d",&ch);
    printf("\n\nSorting order ? \n\n Ascending (1) or Descending (2)\n\nEnter choice = ");
    scanf("%d",&ord);
    printf("\n\n");
    switch(ch){
        case 1:
            insertion(arr,n,ord);
            break;
        case 2:
            selection(arr,n,ord);
            break;
        case 3:
            bubble(arr,n,ord);
    }
    print(arr,n);
printf("\n\n");
return 0;
}

void swap(int *x,int *y){
    int t=*x;
    *x=*y;
    *y=t;
}

void insertion(int *a,int l,int ord){
    int i,j;
    for(i=1;i<l;i++){
        for(j=i;j>0;j--){
            if((ord==1 && a[j]<a[j-1]) || (ord==2 && a[j]>a[j-1]))
                swap(&a[j],&a[j-1]);
            else
                break;
        }
    }
}

void selection(int *a,int l,int ord){
    int index,i,j;
    for(i=0;i<l;i++){
        index=i;
        for(j=i;j<l;j++){
            if((ord==1 && a[j]<a[index]) || (ord==2 && a[j]>a[index]))
                index=j;
        }
    swap(&a[i],&a[index]);
    }
}

void bubble(int *a,int l,int ord){
    int i,j;
    for(i=l;i>0;i--){
        for(j=0;j<i-1;j++){
            if((ord==1 && a[j]>a[j+1]) || (ord==2 && a[j]<a[j+1]))
                swap(&a[j],&a[j+1]);
        }
    }
}

void print(int *a,int l){
    int i;
    for(i=0;i<l;i++)
        printf("%d\t",a[i]);
}

I'm looking for feedback / improvements on my code and whether the implementation of the sorting algorithms in the respective function definitions are proper or not.

This is a pretty good straightforward implementation of these algorithms. It's really nice that they're all implemented in-place so no extra memory is needed. It looks pretty good, but I have a few suggestions.

Better Naming

Your function names are pretty good. (I might add "sort" to the various sorting routine names, but it's not a big deal.) However, the variable names are a bit too terse. Normally, I'd object to using arr as the name for an array and name it after what's stored in the array. (Something like grades or names or whatever was being sorted.) In this example case, it's acceptable, but keep that in mind for the future.

It's tradition for i and j to be used for loop control variables, and I don't mind those. But I would change n to numElements. (Or if you have a better name for the array, name it num<whatever's in the array> such as numGrades, numNames, etc.)

The function parameters a and l should be named with more readable names, like arr and length. ord is reasonable, but I'd prefer order.

Use enum when appropriate

ch is presumably supposed to hold a character, which would be a reasonable name, except that it holds an int. Likewise, ord is supposed to hold the order.

Both of those should be an enumerated type. I'd make one for the sort type and one for the sort order. Something like this:

typedef enum {
    SO_Ascending = 1,
    SO_Descending
} SortOrder;

typedef enum {
    ST_Insertion = 1,
    ST_Selection,
    ST_Bubble
} SortType;

You can then create variables of those types and read the values into them.

Error handling

You may have noticed that the user can enter invalid values, too. You should verify that the values they entered are reasonable and prompt them again if not. To do that, it would probably make sense to break out some of the functionality in main() into separate functions. I suggest something like this:

void promptForArray(int arr [ MAXSIZE ], int* numElements)
{
    do {
        printf("\nEnter number of elements for array = ");
        scanf("%d", numElements);
        if ((*numElements <= 0) || (*numElements > MAXSIZE))
        {
            printf("\nYou entered an invalid value for the number of elements. Please choose a value between 1 and %d\n", MAXSIZE);
        }
    } while ((*numElements <= 0) || (*numElements > MAXSIZE));

    printf("\n\nEnter the array elements one by one : \n\n");
    for(int i = 0; i < *numElements; i++){
        scanf("%d",&arr [ i ]);
    }
}

You could then do a similar thing for getting the sort function and sort order. Something like this:

void promptForSortType(SortType* sortType)
{
    do {
        printf("\n\nWhat sorting you want to use ?\n\n Insertion (1) , Selection (2) or Bubble (3)\n\nEnter choice = ");
        scanf("%d", sortType);
        if ((*sortType < ST_Insertion) || (*sortType > ST_Bubble))
        {
            printf("\nYou entered an invalid sort type. Please enter a value between 1 and 3.\n");
        }
    } while ((*sortType < ST_Insertion) || (*sortType > ST_Bubble));
}

Readability

Do the above things should improve the readability of your code, but you can do more. I recommend leaving spaces around operators (+, -, =, etc.).

This method of introducing function prototypes within main() is very odd:

void insertion(int*,int,int),selection(int*,int,int),bubble(int*,int,int);

Instead, you should put them above main and put them all on separate lines, and you should name the parameters so someone reading them knows what the parameters mean. Something like this:

void insertion(int *arr, const int length, const SortOrder ord);
void selection(int *arr, const int length, const SortOrder ord);
void bubble(int *arr, const int length, const SortOrder ord);

Use const where appropriate

You may have noticed that I made length and ord const in the above examples. They don't change, so it's best to mark them as unchanging. Then the reader and the compiler know more about the intent of the function. You could also make arr const in the print() function.


Getting the first line frame count I'm trying to improve my functional skills and seeing where it fits and where it might not. Please review the code and see where functional practices might be applied. I'm specifically looking at trying to get rid of the state variables.

private int GetFirstLineFrameCount(XDocument doc)
{
    var subElement = (from res in doc.Descendants(_xmlns + "subelements")
                      where res.Element(_xmlns + "object")
                                 .Element(_xmlns + "metadata")
                                 .Element(_xmlns + "identifier").Value == "1"
                      select res).FirstOrDefault();

    var offsetElement = subElement.Descendants(_xmlnsEc + "start").Select(x => x.Element(_xmlnsEc + "editUnitNumber")).FirstOrDefault();

    if (offsetElement == null)
    {
        Logger.Error<SubtitleNormalizer>("no offset found");
        return 0;
    }

    return Int32.Parse(offsetElement.Value);
}

A refactored functional programming that is evil.

public static class ObjectExtension
{
    public static Tout IfNotNull<Tin, Tout>(this Tin source, Func<Tin, Tout> func)
        where Tout : class
        where Tin : class
    {
        if (source == null)
        {
            return null;
        }
        else
        {
            return func(source);
        }
    }
}

public static class stringExtension
{
    public static int ToIntDefault(this string str, Func<int> def)
    {
        if (!string.IsNullOrEmpty(str))
        {
            return int.Parse(str);
        }
        return def();
    }
}

Consumer:

private int GetFirstLineFrameCount(XDocument doc, string _xmlns, string _xmlnsEc)
{
    int result = 0;
    var subElement = (from res in doc.Descendants(_xmlns + "subelements")
                      where res.Element(_xmlns + "object")
                                 .Element(_xmlns + "metadata")
                                 .Element(_xmlns + "identifier").Value == "1"
                      select res)
                      .FirstOrDefault()
                      .IfNotNull(k =>
                          k.Descendants(_xmlnsEc + "start").Select(x => x.Element(_xmlnsEc + "editUnitNumber")).FirstOrDefault()
                      )
                      .IfNotNull(k =>
                          {
                              result = k.Value.ToIntDefault(
                                  () => {
                                          Logger.Error<SubtitleNormalizer>("no offset found");
                                          return 0;
                                      }
                                  );
                              return (object)null;
                          }
                      );
    return result;
}

No, don't follow this. It is evil.

I just demonstrate something that can be done with functions. What you can do is as dreza's suggest to accept _xmlns and _xmlnsEc as parameter. Otherwise, I found that both of the variable is legal to exists, just make them readonly and do constructor injection. Otherwise, create a configuration for both variable so you can use myXmlConfiguration.Xmlns and myXmlConfiguration.XmlnsEc.


Redirect incoming traffic based on referrer or random number This working script directs incoming traffic to different locations depending on the value present in the session for referring URI. If there is a present value, it sends the user back to their original location. If not, then a random number is generated, checked against and used to direct the visitor to one of two possible locations, each with a likelihood of 50%.

It satisfies the basic requirement but I'm wondering if there might not be a better approach.

We have two sub directories, each one with a file within it, and one file at the root. Traffic generally hits the files in the sub dirs, but I wanted to create a root file to handle an event where a user might strip out everything in the URL but the domain.

Each of the files in the sub dirs use this logic to set the referrer:

function strleft($s1, $s2) {
    return substr($s1, 0, strpos($s1, $s2));
}
function selfURL() {
    if(!isset($_SERVER['REQUEST_URI'])) {
       $serverrequri = $_SERVER['PHP_SELF'];
    } else {
       $serverrequri = $_SERVER['REQUEST_URI'];
    }
    $s = empty($_SERVER["HTTPS"]) ? '' : ($_SERVER["HTTPS"] == "on") ? "s" : "";
    $protocol = strleft(strtolower($_SERVER["SERVER_PROTOCOL"]), "/").$s;
    $port = ($_SERVER["SERVER_PORT"] == "80") ? "" : (":".$_SERVER["SERVER_PORT"]);
    $_SESSION['ref'] = $protocol."://".$_SERVER['SERVER_NAME'].$port.$serverrequri;
}
selfURL();

The file at the root uses this logic to direct traffic:

$refurl = $_SESSION['ref'];
if (isset($refurl)) {
    header("Location: " . $refurl);
} else {
    $loc1 = "dir1/file.php";
    $loc2 = "dir2/file.php";
    $num = mt_rand(1, 100);
    if ($num > 50) {
        header("Location: " . $loc1);
    } else {
        header("Location: " . $loc2);
    }
}

null coalesce does exist in php according to the google (per: https://stackoverflow.com/questions/1013493/coalesce-function-for-php )

$s = $_SERVER["HTTPS"] ?: "";
$protocol = strleft(strtolower($_SERVER["SERVER_PROTOCOL"]), "/").$s;

I'm not super familiar with php, but I think this should work from what I understand you're doing here.


Simple SQL query to get phrase pairs from a translations table This is probably the noobiest SQL question ever, and I'm almost positive there's a JOIN or something in PostgreSQL for this, but I can't find it so I'm using a naive method. Here's a bit about my schema:

phrases:

CREATE TABLE phrases
(
  phrase_id bigserial NOT NULL,
  language character varying(4) NOT NULL,
  text text NOT NULL,
  CONSTRAINT phrases_pk PRIMARY KEY (phrase_id)
)

translations:

CREATE TABLE translations
(
  source_id bigint NOT NULL,
  destination_id bigint NOT NULL,
  CONSTRAINT translations_pk PRIMARY KEY (source_id, destination_id)
)

I use the following query to get 100 phrases (for two language pairs). This requires limiting the results to the language pairs, then finding their translations in the translations table:

SELECT P1.phrase_id, P1.language, P1.text, P2.phrase_id, P2.language, P2.text
FROM phrases P1, phrases P2, translations T
WHERE (P1.language = 'eng' AND P2.language = 'fra'
    AND P1.phrase_id = T.source_id
    AND P2.phrase_id = T.destination_id)
LIMIT 100

For what it's worth, the query runs in 34 ms with ~7.5m rows in the translations table, and 1m rows in the phrases table. I'll end up doing full text searches on the phrases table though, so I want to make the translation lookup as efficient as possible (as the translations table will grow ~5x faster than phrases). Lookup in translations is also the most common operation in this database.

Are there any fancy methods of accomplishing this more elegantly/efficiently in PostgreSQL?

P.S. - There is no foreign key constraint in the translations table because I am still in the process of sanitizing the database and removing old records. Importing into Postgres was impossible with the FK due to entries missing from phrases.

I recommend using more vertical space when you write a query to make it easier to read. That said, this should work:

SELECT
    P1.phrase_id,
    P1.language,
    P1.text,
    P2.phrase_id,
    P2.language,
    P2.text
FROM    phrases P1
INNER JOIN translations T
    ON P1.phrase_id = T.source_id
INNER JOIN phrases P2
    ON P2.phrase_id = T.destination_id
WHERE   P1.language = 'eng'
AND     P2.language = 'fra'
LIMIT 100;


Processing a tab-delimited file with vertexes and ages The following block of code needs to be as fast as possible as it likely to be called many thousands of times in a run.

I am also conscious that my thinking style is leaning towards a more procedural style from time to time and hence I may not be taking full advantage of all the benefits that functional programming brings.

Any tips for

making the code run faster, and
making it more functional

let qarray (tableName : string) (startAge : int) (startYear : int) =

let tableData = File.ReadAllLines tableName

// find which row the vertex is in
let vrow = Array.findIndex (fun (s:string) -> s.StartsWith vertexLiteral) tableData
let firstYear = int(tableData.[vrow].Split('\t').[1])

// filter out all the row prior to the column headers e.g. table description and comments
let filteredArray = Array.sub tableData (vrow+1) (tableData.Length-vrow-1)

// use the vertex info to read all lines beyond that, converting to doubles
let f (s:string) = s.Split('\t') |> Array.map double

let fullArray = Array.map f filteredArray

[| for i in 0 .. (120 - startAge - 1) -> fullArray.[startAge + i - 1].[System.Math.Min(startYear - firstYear + i + 1, fullArray.[0].Length)] |]

You should indent the body of the qarray function to make it more obvious where it starts and where it's end (I mean I know it ends at the end of the file in this case, but that's not immediately apparent at first glance).

You should also rename your function f into something more descriptive.

let tableData = File.ReadAllLines tableName

If you're using .net 4.0, you can (and should) use ReadLines instead of ReadAllLines as ReadAllLines reads the whole file into memory before you can start to iterate, while ReadLines loads the file lazily which will be faster for large files.

Note that since ReadLines returns a sequence, not an array, you can not access it using indices any longer. However this is a good thing as getting rid of the indices will lead to more functional code.

// find which row the vertex is in
let vrow = Array.findIndex (fun (s:string) -> s.StartsWith vertexLiteral) tableData
let firstYear = int(tableData.[vrow].Split('\t').[1])

// filter out all the row prior to the column headers e.g. table description and comments
let filteredArray = Array.sub tableData (vrow+1) (tableData.Length-vrow-1)

As I said, you can no longer use an index based approach here. The idiomatic way to get rid of certain elements at the beginning of a sequence is to use skipWhile. Since the first element we want is the one that starts with vertexLiteral, we skip elements while they do not start with vertexLiteral:

// Skip all rows up to the one the vertex is in
let relevantRows = tableData |> Seq.skipWhile (fun s -> not s.StartsWith vertexLiteral)

(Note that by using |> to write tableData first I allowed F# to infer the type of s and thus didn't need a type annotation.)

Now we can use Seq.head to get the first row of relevantRows (the one with the vertex) and Seq.skip 1 to get the remaining rows. So the following lines become:

let firstYear = int((Seq.head relevantRows).Split('\t').[1])

let filteredRows = Seq.skip 1 relevantRows

// use the vertex info to read all lines beyond that, converting to doubles
let f (s:string) = s.Split('\t') |> Array.map double

let fullArray = Array.map f filteredArray

Those lines are fine except that you now need to use Seq.map instead of Array.map (at least on the first line, the second one may stay Array.map as Split still returns an array, but there's no harm in using Seq instead), f needs a better name and fullArray needs to be renamed because it's not an array anymore.

[| for i in 0 .. (120 - startAge - 1) -> fullArray.[startAge + i - 1].[System.Math.Min(startYear - firstYear + i + 1, fullArray.[0].Length)] |]

Ok, here 120 is a magic number, which you did not explain (which you should fix by documenting its meaning), so I'm not sure whether you know that the table will have exactly 120 elements and you have the number in there to avoid a bounds violation or whether the table can contain more than 120 elements and you only want to take the first 120. I'm going to assume the latter is the case.

Further it is not clear to me why you use fullArray.[0].Length instead of fullArray.[startAge + i - 1]. I'm going to assume that all rows have the same length and you chose 0 over startAge + i - 1 for simplicity's sake.

So what you're doing here is basically to skip the first startAge - 2 elements then indexing into each remaining element using the minimum of its length and startYear - firstYear + i + 1 as the index. This can be achieved nicely without an index based loop by using Seq.skip followed by mapi (map with an index), like this:

fullSequence |> Seq.skip (startAge - 2) |>
  Seq.mapi (fun i cols -> cols.[System.Math.Min(startYear - firstYear + i + 1, cols.Length)])

Since this is still a bit long, it might be worthwhile to factor out fun i cols -> cols.[System.Math.Min(startYear - firstYear + i + 1, cols.Length)] into a named function.


Sort dictionary by increasing length of its values Given a dictionary where the values are iterables, print the dictionary sorted by the length of its values.

def sort_dict_by_value_len(lst):
    while lst:
        min_key = min([(len(v), k) for (k, v) in lst.items()])
        print(min_key[1], lst.pop(min_key[1]))

sort_dict_by_value_len({1: 'one', 2: 'two', 3: 'three', 4: 'four', 5: 'five', 6: 'six', 7: 'seven', 8: 'eight', 9: 'nine', 10: 'ten'})

1 one
2 two
6 six
10 ten
4 four
5 five
9 nine
3 three
7 seven
8 eight

I feel like I'm doing too much computations, and worst of all, I'm emptying the input list. Is there any better way to achieve this, with or without using sort or sorted, and optionally keeping the dictionary intact?

Calling a dictionary a list is very confusing
Rather than using min, you could use sorted.

This'll change the time complexity of your code from \$O(n^2)\$ to \$O(n \log n)\$, as Python uses the Timsort. And since sorted is written in C, it'll be super fast too.

Sorted also has the keyword argument key, which lets you sort by another value, rather than the value you get.

This also keeps the original dictionary intact.
You can use dict and enumerate to build your input list. And to build a list to pass to enumerate you could use str.split.

And so I'd change your code to:

def sort_dict_by_value_len(dict_):
    return sorted(dict_.items(), key=lambda kv: (len(kv[1]), kv[0]))

def sort_dict_by_value_len_without_key(dict_):
    return [(k, dict_[k]) for _, k in sorted((len(v), k) for (k, v) in dict_.items())]

dict_ = dict(enumerate('one two three four five six seven eight nine ten'.split(), start=1))
for key, value in sort_dict_by_value_len(dict_):
    print(key, value)

If you only want \$O(1)\$ memory usage, then you could use an insertion sort. But keeps the \$O(n^2)\$ time complexity. I also cheated a bit.

I used a generator comprehension, rather than a list comprehension to ensure \$O(1)\$ space. Alternately, you could invert the algorithm, and pop from one array to another.
I used bisect.insort to remove most of the logic.

def sort_dict_by_value_len(dict_):
    output = []
    while dict_:
        key, value = dict_.popitem()
        bisect.insort(output, ((len(value), key), (key, value)))
    return (i[1] for i in output)


Implementation of Linux cp in modern C++ This is my simple implementation of the Unix cp command.  It is a C++ program but makes use of some of the lower level C library I/O functions.

Any suggestions for style or performance improvements would be appreciated.

#include<iostream>
#include<fstream>
#include<string>
#include<vector>
#include<fcntl.h>
#include<unistd.h>
int main(int argc,char* argv[]) {
    // cmd : cp file1 file2

    if ( argv[1] == nullptr || argv[2] == nullptr ){
        std::cout << "missing arguments: moderncp file1 file2 ";
        return 0;
    }
    const std::string input{argv[1]};
    const std::string output{argv[2]};

    // create a vector with 1024 byte allocated
    std::vector<char> buf(1024);
    // input file descriptor
    int infd = open(input.c_str(),O_RDONLY);
    // output file descriptor
    int outfd = open(output.c_str(),O_WRONLY|O_CREAT|O_EXCL,0664);

    while(true){
        auto count = read(infd,buf.data(),buf.size());
        if (count == 0) break;

        auto num_written = 0;
        auto num_to_write = count;
        while(num_written < num_to_write){
            num_written += write(outfd,buf.data()+num_written,count-num_written);

        }
    }
    close(infd);
    close(outfd);

    std::cout << input << '\t' << output ;
}

I see some things that may help you improve your code.

Fix the bug

The program loops infinitely (actually, as @CrisLuengo correctly points out in a comment, it's not actually infinite.  However, it loops \$2^{54}\$ times for each 1024 bytes in the input file on a 64-bit machine, so it will certainly feel infinite for any human being waiting for the copy to complete!) if the output file already exists or can't be created.  That's a bug that should be fixed.  Generally, when a call can fail (as with open) it is important to test the result for failure before acting on the result.

Use std::fstream for portability

This could be a completely portable program using only standard C++ libraries.  To do that, you'd omit <fcntl.h> and <unistd.h> and just use fstream.  I'm guessing you may have chosen to use the low-level functions for speed, but it's important to measure rather than assume.  The results may surprise you!

Delete file before writing for speed

On several Linux platforms, overwriting an existing program is slower than creating one.  For that reason, one way to make sure that the program is as fast as possible is to call std::remove before doing the copy.

Return valid error codes

A value of 0 as an error indicates no error on Linux and many other platforms.  If you execute cp with no arguments, it returns with a value of 1.  If your program is intended to be an exact replacement, I'd recommend returning 1 instead of 0 if there are missing arguments.

Check for arguments using argc

We can determine if there are the correct number of arguments by examining argc.  The way you're doing it has a problem:

if ( argv[1] == nullptr || argv[2] == nullptr ){

The problem is that if there are zero arguments, argv[1] and argv[2] might not be pointing to anything, so dereferencing them is undefined behavior and therefore not good programming practice.  In particular, you should be aware that argc might be zero which would make testing argv[1] undefined behavior.

Don't use std::string if what you need is a char *

In this case, both of the command line arguments are turned into std::strings but then used exclusively with c_str() to turn them back int char *.  Better would be to simply use char * without conversions to and from std::string.

Prefer std::array to std::vector

If the bounds of your std::vector are known at compile-time (as in this case), then it's generally better to use std::array rather than std::vector.  The std::vector has provisions for resizing that this program doesn't use and doesn't need.  In fact, for this program, there's actually not much difference between a plain C-style array and a std::array.

Name your constant

The buffer size is 1024 bytes, which is probably a bit on the small size.  I think it would be a slight improvement to increase the size and also to name that constant.

Think about non-file files

Under Linux, we have not just files but also devices and named pipes.  Think about whether your program does what you intend if the input is a named pipe.  What happens if we try to copy to /dev/null ?

An alternative version

Using most of those suggestions, here's an alternative:

#include <iostream>
#include <fstream>
#include <cstdio>

int main(int argc, char *argv[]) {
    if (argc != 3) {
        std::cout << "Usage: moderncp srcfile destfile\n";
        return 1;
    }
    std::ifstream in{argv[1]};
    std::remove(argv[2]);
    std::ofstream out{argv[2]};
    if (!out) {
        std::cerr << "Could not create output file " << argv[2] << '\n';
        return 1;
    }
    static constexpr std::size_t buffsize{1024};
    char buffer[buffsize];
    while (in.read(buffer, buffsize)) {
        out.write(buffer, buffsize);
    }
    out.write(buffer, in.gcount());
}

Note that because we're using fstreams for input and output, we don't have to explicitly close the files because the destructor will do so automatically as well as flushing the output buffer.

Results

On my 64-bit Linux machine, copying a 5.3GiB file took 1:29 with the original program, 1:25 with this rewritten version and 1:21 with cp.  Changing the rewritten version to use an 8K buffer allowed it to complete in 0:59, so that's clearly an improvement.


Markov text generator This is a Markov text generator I've created in JavaScript. I'm pretty sure the term "Markov" applies, anyway.

The way the generator works is like this: it first divides the source text into unique "chunks" of length n (specified via the order parameter, greater = more coherent result), and, for each chunk, determines which characters follow it how often. When generating a new text, it looks at the last n characters of what it's generated so far, then randomly selects one of its possible followers and appends it to the text.

(The last bit is apparently slightly different from the Dissociated press algorithm, which would output each follower with the same probability and not account for how often they occur.)

Long story short, here's my code. I'd mainly like to know if I'm following the common best practices as far as object orientation goes, I've had little experience with it before, but any kind of feedback is appreciated.

function Markov(sourceText, order) {

    this.sourceText = sourceText;
    this.order = order;

    this._setupFrequencies();

}

Markov.prototype = {

    _setupFrequencies: function() {

        this.frequencies = {};

        // for each substring of length <order>,
        // create an array of characters that can follow it
        for (var i = 0; i < this.sourceText.length - (this.order - 1); i++) {
            var chunk = this.sourceText.substr(i, this.order);
            if (!this.frequencies.hasOwnProperty(chunk)) {

                var followers = [];
                for (var k = 0; k < this.sourceText.length - (this.order - 1); k++) {
                    if (this.sourceText.substr(k, this.order) == chunk) {
                        follower = this.sourceText.substr(k + this.order, 1);
                        followers.push(follower);
                    }
                }

                this.frequencies[chunk] = followers;

            }
        }

    },

    _getRandomChar: function(chunk) {

        if (!this.frequencies.hasOwnProperty(chunk)) {
            return '';
        }
        var followers = this.frequencies[chunk];
        var randIndex = Math.floor(Math.random() * followers.length);
        return followers[randIndex];

    },

    _getRandomChunk: function() {

        var randIndex = Math.floor(Math.random() * this.sourceText.length);
        return this.sourceText.substr(randIndex, this.order);

    },

    generateText: function(length) {

        if (this.sourceText.length <= this.order) {
            return '';
        }

        var text = this._getRandomChunk();

        // take the last <order> characters from the generated string,
        // select one of its possible followers, and append it
        for (var i = this.order; i < length; i++) {
            var currentChunk = text.substr(text.length - this.order);
            var newChar = this._getRandomChar(currentChunk);

            if (newChar != '') { // the last chunk of the source has no follower
                text += newChar;
            } else {
                text += this._getRandomChunk();
            }
        }

        return text;

    }

}

It would then be used like so:

var text = "Call me Ishmael.  Some years ago—never mind how long precisely—having little or no money in my purse, and nothing particular to interest me on shore, I thought I would sail about a little and see the watery part of the world.";
var markov = new Markov(text, 3);
console.log(markov.generateText(100));

// results:
// " no money interest me Ishmael.  Some or no money in my particular to in my purse, and sail about a l"
// "y in my purse, I the would sail about a little on shore, and see thought I would sail about a little"
// "  Some Ishmael.  Some Ishmael.  Some Ishmael.  Some Ishmael.  Some years ago—never mind see the worl"

I think answering my own question is justified here. (?)

While I have no design pattern advice for myself, I've just found a way to greatly improve the performance of setupFrequencies. The code as posted in my question iterates over the entire source text every time a new chunk is found, but it just occurred to me that such an inner loop isn't necessary at all.
Eliminating it not only shortens the code, but also reduces the runtime complexity from O(n²) to O(n) - a huge improvement, especially for longer source texts.

_setupFrequencies: function() {

    this.frequencies = {};

    // for each substring of length <order>,
    // create an array of characters that can follow it
    for (var i = 0; i < this.sourceText.length - (this.order - 1); i++) {
        var chunk = this.sourceText.substr(i, this.order);
        if (!this.frequencies.hasOwnProperty(chunk)) {
            this.frequencies[chunk] = [];
        }
        var follower = this.sourceText.substr(i + this.order, 1);
        this.frequencies[chunk].push(follower);
    }

},


Create an Array of Objects for Patch Notes in React I was looking to get some reviews of my code, so I can improve my writing. Functionally it works the exact way I want it to based on my patch notes .md file. I'm open to all suggestions for improving the code, especially in making it more performant.

 state = {
  open: false,
  notes: null
 }

 componentDidMount() {
  this.getData();
 }

 getData = () => {
  axios.get(patchnotes_md_urlpath)
   .then((response) => {
    const updates = response.data.replace(/(\r\n|\n|\r)/g, '').split('### ');
    let i;
    let j;
    let notes = [];
    for ( i = 1; i < updates.length; i++ ) {
     let update = updates[i].split('* ');
     const newObj = Object.assign({});
     newObj.version = update[0];
     let list = [];
     for ( j = 1; j < update.length; j++ ) {
      list.push(update[j]);
     }
     newObj.notes = list;
     notes.push(newObj);
    }
    this.setState({
     notes
    });
   }).catch((error) => {
    console.error(error);
   });
 }

### 0.1.2

* Patch notes now implemented when the modal starts.
* Added top lists for organizations

### 0.0.1

* Patch Notes Added
* Modal to include Patch Notes

You got the first step right by removing every white space and line breaks from your string. Next you will have to remove the first occurence of ### by splitting your string and then slicing out the first element.

When done, you will have to map the result of your split. Map will create a new array the exact same size of your input and apply the same function on every attribute of it.

You will then have to split each update by the *. When done, you can get the first value of your array with shift. This function will give out the first element of your array and delete it from the array, only leaving the notes that are going to be put in the notes variable :

const data = '### 0.1.2\n* Patch notes now implemented when the modal starts.\n* Added top lists for organizations\n### 0.0.1\n* Patch Notes Added\n* Modal to include Patch Notes'

const updates = data.replace(/(\r\n|\n|\r)/g, '').split('### ').slice(1).map(update => {
 updateData = update.split('* ')
 return {
  version: updateData.shift(),
  notes: updateData
 }
})

console.log(updates)

You can then put this function in the .then of your axios call


Locate a number in an array with ascending even and descending odd entries Below are two examples of arrays I have, with some given length:

For some value z, I want to find the column the value is in. MATLAB code I wrote:

if z == 1
   column = ceil(length/2);
elseif logical(mod(z,2))
   column = length - (z-3)/2;
else
   column = z/2;
end

It is correct I think. But ugly and possibly slow. Is there a one-liner in MATLAB that can do this?

Let's call the array x:

x = 2 4 6 8 1 9 7 5 3

I'm assuming you don't have the array stored in a variable (in which case this question boils down do: find(x==z)), and that you don't need the array either (in which case this question is really: "How do I create x?").

First off:

Don't use length as a variable name. length is a builtin function, and overloading it is a bad idea. Although it's better to use the builtin numel instead of length, overloading length is not a good idea. It might cause all sorts of strange bugs. Choose another variable name (I'll use len, you might use something else).

x has a fairly straight forward setup, and can be created quite simply using concatenation and the colon operator.

x = [2:2:len, 1, fliplr(3:2:len)]

If you can guarantee that len is odd then it's simply:

x = [2:2:len, 1, len:-2:3]

Now, you have x, and you know z. The only thing remaining is a simple call to find:

col = find(x==z);


Merge dataframes on timestamps and time intervals using data.table in R I have two dataframes (x and y) that I need to join, conditional on the timestamp in x falling within the time interval of two columns in y.  I've accomplished this using data.table::foverlaps() by adapting some of the code in this stackexchange question), but in order to get it to work on my data I had to set the key of data.table x, which according to the documentation of foverlaps(), isn't supposed to be necessary.

Am I using this foverlaps()incorrectly? If so, new ideas for how to accomplish this data merging task are welcome.

library(data.table)

Data snippets

x <- structure(list(TagID = c(20161L, 23055L, 45428L, 2627L), DateTimePST = structure(c(1415481096,
1380768444, 1474560076, 1511384035), class = c("POSIXct", "POSIXt"
), tzone = "Pacific/Pitcairn"), Receiver = c(102140L, 112568L,
112568L, 121907L)), class = "data.frame", row.names = c(NA, -4L
))

y <- structure(list(Station = c("YBAAG4", "YBAAG4", "YBCSSW", "YBCSSW",
"YBBCD", "YBAWW"), Receiver = c(102140L, 102140L, 112568L, 112568L,
121907L, 121907L), Start = structure(c(1464979020, 1409256300,
1369945920, 1470761034, 1505494980, 1409246700), class = c("POSIXct",
"POSIXt"), tzone = "Pacific/Pitcairn"), End = structure(c(1473357300,
1421878500, 1382638020, 1479838293, 1513282440, 1421871360), class = c("POSIXct",
"POSIXt"), tzone = "Pacific/Pitcairn")), class = "data.frame", row.names = c(NA,
-6L))

# preview data
> x
  TagID         DateTimePST Receiver
1 20161 2014-11-08 13:11:36   102140
2 23055 2013-10-02 18:47:24   112568
3 45428 2016-09-22 08:01:16   112568
4  2627 2017-11-22 12:53:55   121907

> y
  Station Receiver               Start                 End
1  YBAAG4   102140 2016-06-03 10:37:00 2016-09-08 09:55:00
2  YBAAG4   102140 2014-08-28 12:05:00 2015-01-21 14:15:00
3  YBCSSW   112568 2013-05-30 12:32:00 2013-10-24 10:07:00
4  YBCSSW   112568 2016-08-09 08:43:54 2016-11-22 10:11:33
5   YBBCD   121907 2017-09-15 09:03:00 2017-12-14 12:14:00
6   YBAWW   121907 2014-08-28 09:25:00 2015-01-21 12:16:00

Because some Receiver numbers are associated with more than one Station, it is important to merge these two datasets on the timestamp (DateTimePST), not on the receiver number.

Prep data

x <- as.data.table(x); x$Start = x$DateTimePST; x$End = x$DateTimePST # needs these start and end columns, otherwise foverlaps throws an error
y <- as.data.table(y)
# set keys: if I omit setting the key on x, forverlap() throws an error
setkey(y, Start, End); setkey(x, Start, End)

foverlaps() join, then discard incorrect Receiver pairs

result <- data.frame(foverlaps(x, y, type = "within"))
result <- result[result$Receiver == result$i.Receiver, ] # this filters down to the correct receiver matches

Clean up results

rm_col <- c("i.Receiver", "i.Start", "i.End")
result <- result[ , !(colnames(result) %in% rm_col)]
result

Station Receiver               Start                 End TagID         DateTimePST
1  YBCSSW   112568 2013-05-30 12:32:00 2013-10-24 10:07:00 23055 2013-10-02 18:47:24
3  YBAAG4   102140 2014-08-28 12:05:00 2015-01-21 14:15:00 20161 2014-11-08 13:11:36
4  YBCSSW   112568 2016-08-09 08:43:54 2016-11-22 10:11:33 45428 2016-09-22 08:01:16
5   YBBCD   121907 2017-09-15 09:03:00 2017-12-14 12:14:00  2627 2017-11-22 12:53:55

Wrap in a function:

getStation <- function(x, y) {
  x <- as.data.table(x); x$Start = x$DateTimePST; x$End = x$DateTimePST
  y <- as.data.table(y)
  setkey(y, Start, End); setkey(x, Start, End)
  result <- data.frame(foverlaps(x, y, type = "within"))
  result <- result[result$Receiver == result$i.Receiver, ]
  result <- result[ , !(colnames(result) %in% c("i.Receiver", "i.Start", "i.End"))]
}

res <- getStation(x, y)

You'll need to use by.x to get around setting x's key.

Also, note that foverlaps can merge on any number of keys, and then do the overlaps only on the final two. What that means here is that you can do the x.Receiver == i.Receiver filter up-front, which also simplifies the clean-up step.

x = as.data.table(x)
x[ , end := DateTimePST]
y = as.data.table(y)

setkey(y, Receiver, Start, End)

result = foverlaps(x, y, by.x = c('Receiver', 'DateTimePST', 'end'), type = 'within')
result[ , end := NULL]

setDF(result)

I do find it a bit awkward/strange that you've got to define end when by.x = c('Receiver', 'DateTimePST', 'DateTimePST') would appear to be fine, and have filed a feature request.

If you don't need DateTimePST in your output, you might find the non-equi-join version more compact / easier to read:

y[x, on = .(Receiver == Receiver, Start < DateTimePST, End > DateTimePST)]
#    Station Receiver               Start                 End
# 1:  YBAAG4   102140 2014-11-08 13:11:36 2014-11-08 13:11:36
# 2:  YBCSSW   112568 2013-10-02 18:47:24 2013-10-02 18:47:24
# 3:  YBCSSW   112568 2016-09-22 08:01:16 2016-09-22 08:01:16
# 4:   YBBCD   121907 2017-11-22 12:53:55 2017-11-22 12:53:55
#    TagID
# 1: 20161
# 2: 23055
# 3: 45428
# 4:  2627

Note the order y[x -- we're using x to "look up" rows of y.

If you do need DateTimePST, it's just a bit of an ugly extension:

y[x, c(.SD, list(DateTimePST=DateTimePST)),
  on = .(Receiver == Receiver, Start < DateTimePST, End > DateTimePST)]

Finally, we can think of this as adding columns to x by a non-equi-join with y by reversing the order:

x[y, `:=`(Start = i.Start, End = i.End, Station = i.Station),
  on = .(Receiver == Receiver, DateTimePST > Start, DateTimePST < End)]


Displaying a wait cursor while we're waiting I come from VB6 where everything is single threaded, so I've never written a lick of multi-threaded code before. I just added a wait cursor to one of our GUI's by stumbling through the docs, but I'm unsure I've done it the best way. Did I?

Control is a Winform view and the code below is located in the presenter class. Full file can be found on GitHub.

    private void RefreshToDoList(object sender, EventArgs e)
    {
        try
        {
            Cursor.Current = Cursors.WaitCursor;
            var getItems = new Task<IOrderedEnumerable<ToDoItem>>(() => GetItems());
            getItems.Start();

            Control.TodoItems = getItems.Result;
        }
        finally
        {
            Cursor.Current = Cursors.Default;
        }
    }

private IOrderedEnumerable<ToDoItem> GetItems()
{
    var items = new ConcurrentBag<ToDoItem>();
    var projects = VBE.VBProjects.Cast<VBProject>();
    Parallel.ForEach(projects,
        project =>
        {
            var modules = _parser.Parse(project);
            foreach (var module in modules)
            {
                var markers = module.Comments.AsParallel().SelectMany(GetToDoMarkers);
                foreach (var marker in markers)
                {
                    items.Add(marker);
                }
            }
        });

    var sortedItems = items.OrderBy(item => item.ProjectName)
                            .ThenBy(item => item.ModuleName)
                            .ThenByDescending(item => item.Priority)
                            .ThenBy(item => item.LineNumber);

    return sortedItems;
}

Your intuition was correct, you're not doing asynchronicity the right way.

Take a look at this bit of code:

var getItems = new Task<IOrderedEnumerable<ToDoItem>>(() => GetItems());
getItems.Start();
Control.TodoItems = getItems.Result;

You're creating a new task, starting it and then you synchronously block the current thread by calling .Result. A new thread isn't created just by creating a Task object, that's what async/await is for.

Instead, change your method as such (omitting cursor and try/catch for clarity):

private async void RefreshToDoList(object sender, EventArgs e)
{
    Control.TodoItems = await Task.Factory.StartNew(() => GetItems());
}

private IOrderedEnumerable<ToDoItem> GetItems()
{
    // Everything else is the same here
}

This incorporates a few changes. First of all: I've made the event handler async void. This is the only situation where it is advised to do this, simply because there's no alternative: event handlers have the requirement to have a void return type whereas asynchronous methods work with Task.

In essence this means that you won't be able to await the event handler so if the caller finishes while the event's effects are still running, your program will exit without a regard of this still running code.

Another change is that I used Task.Factory.StartNew(delegate);. You can choose between this and Task.Run(delegate) but both are more fluent than creating a new Task object and then starting it.

You should always avoid calling .Result since it synchronously blocks the thread to wait for the result (which is the opposite of what you typically want in an asynchronous environment).

As shown in the docs:

  Accessing the property's get accessor blocks the calling thread until the asynchronous operation is complete; it is equivalent to calling the Wait method.

The only reason I'm creating a new task from the caller is because GetItems() doesn't return one. Typically you want asynchronicity throughout the entire hierarchy but I didn't see where it would've been used in GetItems().
However, if you ever call an asynchronous method then it returns Task or Task<T> and you can simply use await MyMethodAsync().


Hackerrank Sparse Arrays Solution in Java I've just solved this problem and I hope you guys give me any feedback to make my code be better.

  Problem: There are N strings. Each string's length is no more than 20 characters. There are also Q queries. For each query, you are given a string, and you need to find out how many times this string occurred previously.

  Input Format

  The first line contains N, the number of strings. The next N lines each
  contain a string. The N + 2nd line contains Q, the number of queries. The
  following Q lines each contain a query string.

import java.io.*;
import java.util.*;

public class Solution {

    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        int n = scan.nextInt();
        String[] stringArr = new String[n];

        for (int i = 0; i < n; i++){
                stringArr[i] = scan.next();
            }

        int q = scan.nextInt();
        for (int i = 0; i < q; i++){
                String stringQue = scan.next();

                int occNum = 0;
                for (int j = 0; j < n; j++){
                    if (stringQue.equals(stringArr[j])) occNum++;
                }
             System.out.println(occNum);
        }
    }
}

String[] stringArr = new String[n];

        for (int i = 0; i < n; i++){
                stringArr[i] = scan.next();
            }

Why use an array?  Consider

        Map<String, Integer> inputCounts = new HashMap<>(n);

        for (int i = 0; i < n; i++) {
            String input = scan.next();

            Integer count = inputCounts.get(input);
            if (count == null) {
                count = 0;
            }

            count++;
            inputCounts.put(input, count);
        }

Now you know the count for each input string.  So

                String stringQue = scan.next();

                int occNum = 0;
                for (int j = 0; j < n; j++){
                    if (stringQue.equals(stringArr[j])) occNum++;
                }
             System.out.println(occNum);

could become just

                String query = scan.next();

                Integer count = inputCounts.get(query);
                if (count == null) {
                    count = 0;
                }

                System.out.println(count);

If the overhead of the hash accesses is less than the overhead of the for loop, this might be faster.

A HashMap converts the string key into a location in the Map.  This is more expensive than a numeric array selection but is probably similar to the cost of the equals method.

I think that we can safely say that accessing the HashMap is going to be faster than iterating over every member of the array unless the array is tiny.  The larger question is if the cost of building the HashMap is going to be greater than the savings in processing the queries.  For a large number of strings and a small number of queries, the cost of building the HashMap can outweigh the savings in processing the queries.  That's trivially true if there are zero queries.  And may still be true for one query.  There is some point where it stops being true.  You'd have to benchmark to be sure.

I prefer names like query and count to stringQue and occNum.