Merge sort using sentinels This is my implementation of recursive merge sort, using sentinels:

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

void merge(int a[], int p, int q, int r)
{
    int n1,n2;
    int i,j,k;
    int *l,*m;

    n1 = q - p + 1;
    n2 = r - q;
    l = (int*)malloc(sizeof(int)*(n1+1));
    m = (int*)malloc(sizeof(int)*(n2+1));

    for(i=0; i<n1; i++)
        l[i] = a[p+i];
    for(j=0; j<n2; j++)
            m[j] = a[q+j+1];
    l[i] = INT_MAX;
    m[j] = INT_MAX;

    i = j = 0;
    for(k=p; k<r+1; k++){

        if(l[i] <= m[j]){
            a[k] = l[i];
            i++;
        }

        else{
            a[k] = m[j];
            j++;
        }

    }
}

void merge_sort(int a[],int p, int r)
{
    int q;

    if(p<r){
        q = (p+r)/2;
        merge_sort(a,p,q);
        merge_sort(a,q+1,r);
        merge(a,p,q,r);
    }
}
int main()
{
    int *num,n;
    int i;

    printf("Enter number of digits:");
    scanf("%d",&n);
    num = (int*)malloc(sizeof(int)*n);
    printf("Enter numbers:");
    for(i=0 ; i<n; i++){
        scanf("%d",&num[i]);
    }
    merge_sort(num,0,n-1);
    printf("Sorted array:\n");
    for(i=0; i<n; i++)
        printf("%d ",num[i]);
    free(num);
    return 0;
}

I'm looking for reviews, suggestions, and improvements.

In merge(), you call malloc() twice with no corresponding calls to free().  That can't be good for your memory consumption!

When computing the average of two values, don't do q = (p+r)/2, because that is vulnerable to overflow.  Instead, write it as q = p + (r - p) / 2.


Faster finding of all cubic numbers I want to improve the performance of this program.

count = 0
store = []
n = 9238899039
for i in range(1,n):
    if i*i*i <= n:
        count += 1
        store.append(i)
print count

At the moment it is very slow. I need to find all cubic numbers up to n.

There's no reason to use brute force like that.  I couldn't even get the program to complete without crashing.

To find the count, just take the cube root of n, and round down.  Then, it's a simple matter to construct the list you need.

n = 9238899039
count = int(n ** (1.0/3))
store = range(1, count + 1)


Inserting data into a file So, given a file with the following format:

  [header]
  line of data
  line of data
  line of data
  line of data

  [header 2]
  line of data
  line of data
  line of data

  ...

I add a line of data to a specific header so the result would be:

  ...
  [target header]
  line of data
  ...
  new line inserted

The program currently only adds lines but is written so it accepts an input file that lists instructions with the format instruction|[header]|line to add e.g.  A|[Passions]|Code Review would add Code Review to the Passions section.

import java.io.File;
import java.io.FileNotFoundException;
import java.io.PrintWriter;
import java.io.UnsupportedEncodingException;
import java.util.Scanner;

public class AdjustList {
    public static void main(String[] args) {
        if (args.length != 1) {
            System.err.println("No arguments provided.");
            System.exit(1);
        }

        try (Scanner fileScanner = new Scanner(new File(args[0]))) {
            while (fileScanner.hasNextLine()) {
                String[] commandParameters = fileScanner.nextLine().split("\\|");
                if (commandParameters.length != 3) {
                    throw new IllegalArgumentException("Invalid parameter format");
                }

                File targetFile = new File("ImportantList.md");
                processCommand(targetFile, commandParameters[0], commandParameters[1], commandParameters[2]);
            }
        } catch (FileNotFoundException fnfe) {
            System.err.println("File doesn't exist / wrong directory");
        }
    }

    private static void processCommand(File file, String instruction, String targetSection, String inputLine) {
        boolean taskComplete = false;
        StringBuilder fileBuilder = new StringBuilder();

        try (Scanner fileScanner = new Scanner(file)) {
            if (instruction.equals("A")) { // add
                System.out.println("Adding " + inputLine + " to " + targetSection);
                while (fileScanner.hasNextLine()) {
                    String line = fileScanner.nextLine();
                    if (line.equals(targetSection)) {
                        fileBuilder.append(line).append('\n');
                        while (!taskComplete) {
                            line = fileScanner.nextLine();
                            if (line.isEmpty()) {
                                fileBuilder.append(inputLine).append('\n');
                                taskComplete = true;
                            } else {
                                fileBuilder.append(line).append('\n');
                            }
                        }
                    } else {
                        fileBuilder.append(line).append('\n');
                    }
                }
            }
        } catch(FileNotFoundException fnfe) {
            System.err.println("File doesn't exist / wrong directory");
            System.exit(1);
        }

        writeOutput(file, fileBuilder.toString());
    }

    private static void writeOutput(File file, String output) {
        try (PrintWriter writer = new PrintWriter(file, "UTF-8")) {
            writer.write(output);
        } catch (FileNotFoundException | UnsupportedEncodingException ex) {
            ex.printStackTrace();
        }
    }
}

The file is read, line by line, storing the input into a StringBuilder with the additional line added based on a flag. This works but I get the feeling it's not the cleanest way.

Misleading Message?

    if (args.length != 1) {
        System.err.println("No arguments provided.");
        System.exit(1);
    }

So if I have 2 arguments, I get the message No arguments provided. Umm... I provided 2 arguments!
Suggested fix:
Change the above code to:
    if (args.length == 0) {
        System.err.println("No arguments provided.");
        System.exit(1);
    }

And you can possible warn the user about only requiring one argument if they provide too much. Something like:
    if (args.length > 1) {
        System.out.println("Warning! Too many arguments; only the first one will be considered.");
    }


Automatically Sending an Email based on a Google Sheet's cell value Would love some feedback on how to improve my code. It might not be as readable as I'd like in places, but it currently works. Note that this code will be adapted to work on a different spreadsheet, so making this extensible would be amazing. Also, is there any reason to put this code into a class? I come from a scripting background, and OOP concepts are new to me.

This is the data that it looks at:

The code gets a row which it uses its index to turn FALSE to TRUE after it sends an email. The email isn't implemented yet, but will be soon. Any suggestions for if there's something other then smtplib on python would be appreciated, as I might want to just send the email without hooking up an account (like mail command on Linux).

import gspread
import pprint
from oauth2client.service_account import ServiceAccountCredentials

def setup_connection(spreadsheet):
    scope = ['https://spreadsheets.google.com/feeds']
    creds = ServiceAccountCredentials.from_json_keyfile_name('client_secret.json', scope)
    client = gspread.authorize(creds)
    worksheet = client.open(spreadsheet).sheet1
    return worksheet

def send_email(is_sent, is_passing):
    """
    Takes in two parameters to determine if
    1. the email is sent
    2. did the quiz-taker pass?

    Returns true if the email was sent and false if not. Will probably need to be updated, too!
    """
    if not is_sent:
        print("sending email...")
        if is_passing:
            print("you passed! congratulations :)")
        else:
            print("sorry, but you didn't pass yet! We'd really love to see you at the MakerSpace, so make sure to try again!")
            return True
    else:
        print("email already sent.")
        return False

def main():
    worksheet = setup_connection('CoMotion Quiz Test Automation')
    rows = worksheet.get_all_values()
    index = 0
    for row in rows:
        index = index + 1
        if (row[4] == "FALSE"):
            is_passing = (float(row[0])/30 > 0.8)
            is_sent = False
            if (send_email(is_sent, is_passing)):
                worksheet.update_acell('E' + str(index), "TRUE") # Updates the cell to reflect true

            else:
                print("these are not the droids you\'re looking for...")

main()

Few minor things to improve:

you may unpack row in the for row in rows loop; moreover, you can use extended Python-3.x iterable unpacking
use enumerate() to handle the indexing
remove unused pprint import
remove extra parenthesis around row[4] == "FALSE" and float(row[0])/30 > 0.8 and send_email(is_sent, is_passing)
respect PEP8 guidelines - specifically, watch for spaces around the operators
you can use an f-string to define the cell name

The code of the main() function with the above mentioned improvements applied:

for index, (score, *_, is_email_sent) in enumerate(rows):
    if is_email_sent != "FALSE":
        continue

    is_passing, is_sent = float(score) / 30 > 0.8, False
    if send_email(is_sent, is_passing):
        worksheet.update_acell(f'E{index}', "TRUE")  # Updates the cell to reflect true
    else:
        print("these are not the droids you\'re looking for...")

I am, also, not really sure about that is_sent variable and send_email() having this argument. I think send_email should simply be responsible for sending an email and nothing - do one thing (Single Responsibility Principle). Printing out a message if an email was already sent should be done outside of the function.


Template for PHP service (/etc/init.d) script I wanted a template for a service script that is versatile and easily configurable.

Along with producing similar outputs to the SSH and Samba daemons, I request the code to be as clean, simple and self explanatory as possible.  Hence I removed all the comments.

Sample usage:

$ service mydaemon restart
mydaemon stop/waiting
mydaemon start/running, process 30549

Sample of similar command with the SSH daemon:

$ service ssh restart
ssh stop/waiting
ssh start/running, process 30577

Here is my service.php script:

#!/usr/bin/php
<?php

    define('NAME',      basename($argv[0]));
    define('RUNAS',     'root');
    define('DIR',       __DIR__);
    define('CMD',       __DIR__.'/thread.php');
    define('SUDO',      'sudo -u '.RUNAS.' ');
    define('LOG_FILE',  '/dev/null');
    define('PID_FILE',  '/var/run/'.NAME.'.pid');

    function writeln($string)
    {
        echo $string.PHP_EOL;
    }

    class service
    {
        public static function unset_pid()
        {
            if (is_file(PID_FILE)) unlink(PID_FILE);
        }

        public static function get_pid()
        {
            if (!is_file(PID_FILE)) return null;

            $pid=file_get_contents(PID_FILE);

            $process_status=null;
            if (!empty($pid) && ctype_digit($pid)) $process_status=shell_exec('ps '.$pid);

            if (empty($process_status) || strpos($process_status, CMD, 0)===false)
            {
                service::unset_pid();
                return null;
            }

            return $pid;
        }

        public static function is_running()
        {
            return (!empty(service::get_pid()));
        }

        public static function start()
        {
            if (service::is_running())
            {
                writeln('start: Job is already running: '.NAME);
                return true;
            }

            $new_pid=shell_exec('cd '.DIR.';'.SUDO.CMD.' >> '.LOG_FILE.' 2>&1 & printf $!;');
            file_put_contents(PID_FILE, $new_pid);
            service::status();

            return service::is_running();
        }

        public static function stop()
        {
            if (!service::is_running())
            {
                writeln('stop: Unknown instance:');
                return true;
            }

            shell_exec('kill '.service::get_pid());

            for ($seconds=0; $seconds<60; $seconds++)
            {
                if (!service::is_running()) break;
                sleep(1);
            }

            if (service::is_running())
            {
                writeln('Not stopped; may still be shutting down or shutdown may have failed.');
                return false;
            }

            service::status();

            return !service::is_running();
        }

        public static function restart()
        {
            if (!service::stop())
            {
                writeln('Aborting restart.');
                return false;
            }

            return service::start();
        }

        public static function status()
        {
            $running=service::is_running();
            if ($running) writeln(NAME.' start/running, process '.service::get_pid()); else writeln(NAME.' stop/waiting');
            return $running;
        }

    };

    if (count($argv)!=2)
    {
        if (count($argv)>2) writeln('Too many arguments given.');
        $argv=array($argv[0], '');
    }

    switch (strtolower($argv[1]))
    {
        case 'start':

            service::start();
            break;

        case 'stop':

            service::stop();
            break;

        case 'restart':

            service::restart();
            break;

        case 'status':

            service::status();
            break;

        default:

            if (!empty($argv[1])) writeln('Unknown option: '.$argv[1]);
            writeln('Usage: '.$argv[0].' {start|stop|restart|status}');
            break;
    }

?>

Any suggestions for improvements will be appreciated.

from stop:

        if (service::is_running())
        {
            writeln('Not stopped; may still be shutting down or shutdown may have failed.');
            return false;
        }

        service::status();

        return !service::is_running();

from start:

        service::status();

        return service::is_running();

Hmm, looks like a similar pattern... Can we combine this in a separate function?

What does service::status do?

    public static function status()
    {
        $running=service::is_running();
        if ($running) writeln(NAME.' start/running, process '.service::get_pid()); else writeln(NAME.' stop/waiting');
        return $running;
    }

... Wait, it prints a few things and then returns whether it's running.

So why the double is_running call?

I'd just return the result of service::status in these cases. It'd be more correct too; what if a service stops between your logging and the return statement? You'd log that it's running and then return that it's ... not running. Good way to have logs showing "everything is fine" whilst things aren't doing what they're supposed to do.


Generating a performance report for clans in Clash Royale The following code generates a report showing performance of clan members in a game called Clash Royale.
Everything is pretty self-explanatory, we are working with a lot of https requests, arrays and sorting.
const https = require("https");
const clanTag = "L2P8GRJR";
const token = "TOKEN_HERE"

let rawData = '';
let topDonators = [];
let worstDonators = [];
let bestPlayers = [];
let counter = 0;
const mergeByProperty = (target, source, prop) => {
    source.forEach(sourceElement => {
        let targetElement = target.find(targetElement => {
            return sourceElement[prop] === targetElement[prop];
        })
        targetElement ? Object.assign(targetElement, sourceElement) : null;
    })
}

const finalise = () => {
    if (counter !== 2) return;

    let result = `CR Clan Youtube Weekly Report
°•°•°•°•°•°•°•°•°•°•°•°•

— Top Donators ——
~~~~~~~~~~~~~~~~~~
${topDonators.map((x, i) => `${i + 1}. ${x.name} - ${x.donations}`).join('\n')}
--------------------

— Back Donators ( < 100 ) ——
~~~~~~~~~~~~~~~~~~~
${worstDonators.map((x, i) => `${50 - i}. ${x.name} - ${x.donations}`).join('\n')}
--------------------

— Net Best Players ——
(Donations × Fame earned ÷ (100 × 1000))
===================
${bestPlayers.map((x, i) => `${i + 1}. ${x.name} - ${x.netScore}`).join('\n')}
===================

{ ${bestPlayers[0].name} } is the player of the week, congrats!

Data automatically fetched and compiled at ${new Date().toUTCString()}.

Earn fame in river race and donate cards to improve your net score which will be published on Sunday. Player of The Week will be chosen based on net score. Better net score may also result in promotion.

CR may not show full report to some players, hence this report is also published to the Discord server (link in the description).`
    console.log(result)
}

https.get(`https://proxy.royaleapi.dev/v1/clans/%23${clanTag}/members`, {
    headers: {
        authorization: token
    }
}, (res) => {
    res.on("data", (data) => {
        rawData += data
    })
    res.on("end", () => {
        rawData = JSON.parse(rawData).items

        topDonators = [...rawData].sort((a, b) => b.donations - a.donations).slice(0, 10)

        rawData.forEach(elm => {
            if (elm.donations < 100) worstDonators.push(elm)
        })
        worstDonators.sort((a, b) => a.donations - b.donations)
        counter++;
        finalise()
    })
})

https.get(`https://proxy.royaleapi.dev/v1/clans/%23${clanTag}/currentriverrace`, {
    headers: {
        authorization: token
    }
}, res => {
    let warData = [];
    res.on("data", data => {
        warData += data
    })

    res.on("end", () => {
        warData = JSON.parse(warData).clan.participants;
        mergeByProperty(rawData, warData, "tag")

        bestPlayers = [...rawData].map(elm => {elm.netScore = elm.fame * elm.donations / 100000; return elm;}).sort((a,b) => b.netScore - a.netScore).slice(0, 10)
        counter++;
        finalise()
    })
})

A proxy is being used but just know that that's a necessity.
Performance is the main point here but I would love review on any and every point.

Cool project!
Good work so far.
Clean code is important and that's one of the things I struggle with. A lot.
From your code, I can see that you would benefit a lot from that too.
const token = "TOKEN_HERE";

Tokens and sensitive information you should try to store in .env files. Especially if your code might be accessible on github or similar sites.
const mergeByProperty...

This function seems too complicated for what you want to do. I really don't think you need it at all.
Same for this:
const finalise...

Try to extract logic into functions, especially if you need to use it in other places.
Try to identify repeating code, if you see that, then you might be able to create a function for it.
Do not repeat yourself. Code you repeat is hard to maintain because if you make a small change you need to make that change everywhere.
Naming your functions and variables is equally important and it's harder than it seems.
This is what I would do differently (and even this can be improved a lot):
const baseUrl = 'https://proxy.royaleapi.dev/v1/clans';
const clanUrl =  `${baseUrl}/#${clanTag}/`;
const requestOptions = {
    headers: {
        authorization: token
        }
    }

// wrap your requests into a promise
const GET = (url, options = {}) => {
    return new Promise((resolve, reject) => {
        const request = https.get(url, options, (response) => {
            if (response.statusCode < 200 || response.statusCode > 299) {
                reject(new Error(response.statusCode));
            }
            let data = '';
            response.on('data', (chunk) => data += chunk);
            response.on('end', () => resolve(JSON.parse(data)));
        });
        request.on('error', (err) => reject(err))
    })
};

// do all the data manipulation inside these helper functions
// this transforms the data you get from the server into the format
// you want

const getMembers = (members) => {
    const donors = {
        top: [...members].sort((a, b) => b.donations - a.donations).slice(0, 10),
        worst: [...members].filter(x => x.donations < 100).sort((a, b) => a.donations - b.donations)
    }
    return donors;
}

// transform the data from server into the format you want

const getCurrentRiverRace= (clanParticipants) => {
    const clan = {
        participants: clanParticipants,
        bestPlayers: [...clanParticipants].map(x => {
            return {
                ...x,
                netScore: x.fame * x.donations / 100000
            };
        }).sort((a,b) => b.netScore - a.netScore).slice(0, 10)
    }
    return clan;
}

// this one makes both requests to the server at the same time
// Notice the async. Since your requests return promises, this is how you
// handle the promises

const requests = async () => {
    const membersCall = GET(`${clanUrl}/members`, requestOptions);
    const riverRaceCall = GET(`${clanUrl}/currentriverrace`, requestOptions);
    const [members, riverRace] = await Promise.all([membersCall, riverRaceCall]);

    return {
        members: getMembers(members),
        riverRace: getCurrentRiverRace(riverRace)
    }
}

// here you just 'finalise' or output whatever you want
// you should handle the errors so maybe wrap it in a try/catch.

const output = async () => {
    try {
        //
        const r = await requests();
        const stats = `
        CR Clan Youtube Weekly Report
        °•°•°•°•°•°•°•°•°•°•°•°•

        — Top Donators ——
        ${stats.members.top}
        ....
        `;

        console.log(stats);
        return stats;
   } catch (err) {
       // here you handle the error in case the promise rejects
       // (returns error)
       // console.log is NOT error handling but it gives you an idea

       console.log(err);
   }
}

Try to puzzle it together. I'm not sure if it's 100% functional but it should be pretty close. Consider it a homework :)


Find nearest k points from the central point The task:

  Given a list of points, a central point, and an integer k, find the
  nearest k points from the central point.

  For example, given the list of points [(0, 0), (5, 4), (3, 1)], the
  central point (1, 2), and k = 2, return [(0, 0), (3, 1)].

My solution:

const lst = [[0, 0], [5, 4], [3, 1]];
const center = [1, 2];
const k = 2;

const findNearestPoints = ({lst, center, k}) => {
  // I assume the data are valid; no error checks
  const calcHypo = x => Math.sqrt((x[0] - center[0])**2
                                + (x[1] - center[1])**2);
  const sortPoints = (a,b) => calcHypo(a) - calcHypo(b);
  return lst
   .sort(sortPoints)
   .slice(0,k);
};

console.log(findNearestPoints({lst, center, k}));

Don't sqrt the distance.

It is a common mistake when filtering distances to use the complete distance calculation.

Given 2 values a and b, if a < b then it is also true that sqrt(a) < sqrt(b). Hence you don't need the expensive sqrt calculation to know the if a point is closer than another.

To find the closest the following does not use the sqrt of the distance.

function closestPoint(points, point, dist){
    var x, y, found, min = dist * dist; // sqr distance
    for(const p of points) {
        x = p[0] - point[0];
        y = p[1] - point[1];
        x *= x;
        y *= y;
        if(x + y < min){
            min = x + y;
            if (min === 0) { return p } // early exit
            found = p;
        }
    }
    return found;

}

Not in the sort!!!

You are doing the distance calculation in the sort DON'T!!!, that means you repeat the same calculations over and over.

To improve you throughput the following will reduce the over all time. The improvement is linear and does not change the complexity.

Note that in JS a ** 2 is slightly slower than a * a

A more efficient version of your solution

function findNearestPoints({list, center, k}) {
    const res = [];
    const cx = center[0], cy = center[1]; // alias and reduce indexing overhead
    const distSqr = (x, y) => (x -= cx) * x + (y -= cy) * y;
    const sort = (a, b) => a[1] - b[1];

    for (const p of list) { res.push([p, distSqr(p[0], p[1])]) }
    res.sort(sort).length = k;
    return res.map(p => p[0]);
}

The ** operator for roots

Note that JS has the ** operator. That you can use it to get roots by making the right side the inverse, 1 over the power. Thus the sqrt is **(1/2) the cube root is **(1/3)

eg

if 2 ** 2 === 4 then 4 ** (1/2) === 2
if 2 ** 3 === 8 then 8 ** (1/3) === 2  Don't approximate 8 ** 0.33 !== 2
if 2 ** 4 === 16 then 16 ** (1/4) === 2

Better sort

The sort is the bottle neck in this problem.

You can use a binary tree sort as it is the least complex for real numbers (every coder should learn how to implement a binary tree sort)

Do you need the sort?

However I think (think means might be, I am going by instinct) that there is a faster solution that does not involve a sort and that is at most \$O(n)\$

Remember that the order of the points is not important, that you need only separate the points in two. It may take a few passes to do, but as long as the number of passes is not related to the number of points or 'k' you will have a \$O(n)\$ solution.

I am not going to give you the solution this time (if there is one) as there is no problem solving experienced gained coping code.


Functional Sundaram's sieve I wrote this Sundaram's sieve in Coffeescript to quickly generate prime numbers up to limit:

sieve_sundaram = (limit) ->
    numbers = (n for n in [3...limit+1] by 2)
    half = limit / 2
    start = 4

    for step in [3...limit+1] by 2
        for i in [start...half] by step
            numbers[i-1] = 0
        start += 2 * (step + 1)

        if start > half
            result = (n for n in numbers when n > 0)
            result.shift 2
            return result

See equivalent Javascript code.

Is it possible to rewrite it in functional paradigm? I'm concerned mainly with the need to change the numbers array and increment start variable. If external library (like underscore.js) is needed, it's acceptable. JavaScript solutions are also acceptable.

result.shift 2 probably doesn't do what you think it does. You meant unshift.

shift removes the first element of an array (and doesn't take any arguments).
unshift adds new items at the beginning of the array.

As a result, your final array isn't [2,3,5,...] but [5,...]

Also, I'll point out the bug in the code you have in your "equivalent Javascript code" link - it has >= in the  result = (n for n in numbers when n >= 0) at the bottom instead of n > 0 (which is correct).

You probably guessed that since your code here has that fixed. n >= 0 doesn't filter the 0 elements from the array, n > 0 does.

And now for the functional ways. (Disclaimer: I don't think my functional rewrite of your code is any prettier - but it's more functional. Maybe someone can make it look more elegant?)

I'm no expert on functional programming, but from what I've learned, you generally try to use things like map, reduce, filter.

[].filter() takes predicate (test function returning boolean) that gets called with all the elements one by one and returns array with values that pass the test. You kind of already used it in the (n for n in numbers when n > 0) - it's the when part.
[].map() takes a "transforming" function that gets called with all the elements one by one and returns array with the transformed values. With CoffeeScript we can cheat again and use list comprehensions: (my_function(x) for x in xs).
[].reduce() is probably the least intuitive one - but we'll use it in your sieve :) You give it an initial value (accumulator) and a transforming function and it reduces the array left to right to a single value. The canonical example is summation: the_sum = [1,2,3].reduce(((acc, val) -> acc + val), 0) # the_sum is 6 now.

Now, when I hear "functional paradigm," I hear "loopless way" and "immutability." Functional programming is about more things, of course.

EDIT: oh man, I had limit = 100 in the code. Stupid mistake :) Fixed now.

sieve_sundaram = (limit) ->

  start = 4
  half = limit/2

  numbers = (n for n in [3...limit+1] by 2)
  steps = numbers[..] # copy of numbers

As you can see, numbers equals steps, at least at the first go. What we'll change will be the loops. Instead of thinking in terms of  for ... do ..., we'll think more like result = (... for ...) - transforming the arrays with our map, filter, reduce functions or their CoffeeScript counterparts.

Now, we have to define the transforming function for the reduce(), because we want to do something like steps.reduce(callback,numbers). You see, with a little bit of imagination you had for step in steps do something in your code.

Now we'll do exactly that - our function for the reduce will get a step at every call, just like your loop did. So that's that.

Next problem we have is rewriting the "global" changes you make inside the loop. So we need to do something about the numbers[i-1] = 0 and start += 2 * (step + 1) lines. We'll make the changes propagate through the result values.

But how can we make two things propagate through one value? We'll use an array. Remember, numbers is an array and start is a number. Essentially we'll insert the start at the beginning (or the end, wouldn't matter) of the numbers array and at each call of our reduce callback extract it from there - and of course, after the reduce finishes.

  callback = (start_and_nums,step) ->

    start = start_and_nums[0]
    nums = start_and_nums[1..]

So that's the extraction part. Pretty trivial with CoffeeScript.

Next part is little bit trickier. If you're familiar with recursion, you know the base case is usually written before the other parts of recursive function. We'll do something similar - check for your if start > half condition before we do anything else.

If it's true, it will simply mean that we don't need to do any of that sieving mumbo jumbo, that we're done and just need to wait till we get through all the elements, because we can't stop the reduce function midway.

    if start > half
      result = (n for n in nums when n > 0)
      result.unshift start
      return result

      # We can't stop iterating inside the reduce() although we'd like to right now.
      # Also, we slice the first element every time our function gets called,
      # so we'll put it back here for now. We'll deal with it after reduce() finishes.
      # Also, we mustn't forget about the 2!

Next portion of the code is reached only if we still have work to do (we didn't get to the return inside the if). It's outdented, but maybe that's hard to see with my comments between the lines.

Now, look at your for i... line. What you really want is i-1, because you use it in the next line (numbers[i-1]). So we'll use map. It should be pretty straightforward:

    idxs = (idx-1 for idx in [start...half] by step)

OK, we have the is, now we have to deal with the sieving. If you look at your code and think about it in terms of transforming one array to another, you realize you either change an element to 0 if it was unlucky and its index was in the [start...half] by step, or don't change anything if the index wasn't there. Look how it translates to code:

    new_nums = ((if i in idxs then 0 else num) for num,i in nums)

We dealt with the sieving, now we need to change the start variable. That's trivial:

    new_start = start + 2 * (step + 1)

And we need to prepare these new values for the next "iteration" - and return them. You can see that instead of deciding whether to finish or loop again right after changing the start, we delegate that to the beginning of the callback.

If we had it here instead of at the beginning, we'd do unnecessary (and potentially harmful) changes to the numbers array, because we can't stop the reduce from getting through all the steps. That's a minor disadvantage of this approach - we do some unnecessary work. But we can limit it to just finding out if we actually need to do something or can just return immediately.

    new_start_and_nums = new_nums[..]
    new_start_and_nums.unshift new_start
    return new_start_and_nums

That's the end of our callback function. Now remember, the function works with start_and_nums, but so far we have only the numbers. Let's fix that:

  start_and_nums = numbers[..]
  start_and_nums.unshift start

And let's finally run the reduce function!

  result = (steps.reduce(callback,start_and_nums))

We don't really need the start value at the beginning.

  result.shift()

But we need to put in the 2! The point of doing it here and not in the callback is again that we can't stop reduce from stopping. If we inserted it in there, it would get inserted multiple times.

  result.unshift 2

Aaaand we're done.

  return result

By the way, thanks for your question - I see the Sundaram's sieve for the first time. Seems very elegant :)

The final code: either copypaste the snippets above or use this gist :) But I sincerely hope you at least read through this and not just use the final thing, because of the time I put into writing the explanations... :D


What is the best way to refactor out “temporal dependence” of instance methods? For example, suppose in a view class we need to instantiate some UI components and then populate the fields. Something like:

var View = function() {
  this._initUI();
  this._populateFields();
};

View.prototype = {
  _initUI: function() {
    this.textField = new TextField();
    this.comboBox = new Combobox();
  },

  _populateFields: function() {
    this.textField.setText('foo');
    this.comboBox.setVal('bar');
  }
};

This doesn't quite smell right, as you must call _initUI before _populateFields, and it's pretty hidden that _initUI will be creating some instance variables that will later be used by _populateFields.

What's your opinion of a better way of doing this?

You could have a flag that is set when _initUI is called.  The first line of _initUI would check the flag and exit if it's already set; the second line would set it.  Then any method which needs _initUI to have been called can simply, safely, call it without fear of double-initializing things.


Using 'switch to choose different rates to calculate net pay My task is ask this from the user:

Enter the number corresponding to the desired pay rate or action:

1) $8.75/hr           2) $9.33/hr

3) $10.00/hr          4) $11.20/hr

5) quit

If choices 1 through 4 are selected, the program should request the hours
worked. The program should recycle until 5 is entered. If something other than
choices 1 through 5 is entered, the program should remind the user what the
proper choices are and then recycle. And i have to use switch

My code:

#include <stdio.h>
#define RATE1 8.75
#define RATE2 9.33
#define RATE3 10.00
#define RATE4 11.20

int main()

{
    int rateSelected;
    int weeklyHours;
    double totalPayed, rateToCalculate;

    printf("Enter the number corresponding to the desired pay rate or action:\n");
    printf("1) %.2lf$/hr                               2) %.2lf$/hr\n", RATE1, RATE2);
    printf("3) %.2lf$/hr                              4) %.2lf$/hr\n", RATE3, RATE4);
    printf("5) Quit\n");

    while ((scanf("%d", &rateSelected)) != EOF && rateSelected != 5)
    {

        if (rateSelected > 4)
        {
            printf("please enter a valid number:\n");
            continue;
        }

            switch (rateSelected)
        {
            case 1:
                rateToCalculate = RATE1;
                break;
            case 2:
                rateToCalculate = RATE2;
                break;
            case 3:
                rateToCalculate = RATE3;
                break;
            case 4:
                rateToCalculate = RATE4;
                break;

        };

        printf("please enter you weekly hours:\n");
        scanf("%d", &weeklyHours);
        totalPayed = weeklyHours * rateToCalculate;
        printf("your paychack for this week is: %.2lf\n\n",totalPayed);

        printf("Enter the number corresponding to the desired pay rate or action:\n");
        printf("1) %.2lf$/hr                               2) %.2lf$/hr\n", RATE1, RATE2);
        printf("3) %.2lf$/hr                              4) %.2lf$/hr\n", RATE3, RATE4);
        printf("5) Quit\n");

    }

    return 0;
}

How bad is it?
And one thing didn't work for me is if a user put a letter its not cycling..(any suggestion for that too?)

Thank you,

It's not bad. A couple of issues regarding validating input, which you know about, but a few 'style' things, some of which you may not know about yet.

In addition to Ryan's answer, a couple more suggestions:

Only declare one variable per line. Otherwise it becomes harder to see where it comes from.

double totalPayed;
double rateToCalculate;

In general, try to declare variables in the inner-most block in which they're used. In this case, all but rateSelected should be declared inside the while loop. It doesn't matter for this, but in more complex programs restricting where the variables can be used will become more important.
Don't have 'magic numbers', in this case the options themselves. I would suggest setting up an enum (if you know about them?, or a series of #defines), something like:

enum { OPTION_BEGIN = 1, OPTION_RATE_ENTRY = OPTION_BEGIN, OPTION_RATE_MID, OPTION_RATE_SENIOR, OPTION_RATE_MANAGER, OPTION_QUIT, OPTION_END };

Use these in getRate instead of the appropriate numbers.
Separate out the question into a separate function, it's an identifiable single task. And it becomes easier to change if it's one place. I've added another function here for readability:

int is_valid_option( int option )
{
    return ( option >= OPTION_BEGIN ) && ( option < OPTION_END );
}

int ask_for_pay_rate( void )
{
    int option = OPTION_END;
    while ( option == OPTION_END )
    {
        int answer;

        printf("Enter the number corresponding to the desired pay rate or action:\n");
        printf("1) %.2lf$/hr                               2) %.2lf$/hr\n", RATE1, RATE2);
        printf("3) %.2lf$/hr                              4) %.2lf$/hr\n", RATE3, RATE4);
        printf("5) Quit\n");

        /* scanf returns the number of successful inputs, ie 1 number.
           Test for this, rather than eof */
        if ( ( scanf( "%d", &answer ) == 1 ) && is_valid_option( answer ) )
        {
            option = answer;
        }
        else
        {
            printf("please enter a valid number:\n");
        }
    }
    return option;
}

Note by the way, that I've set up a condition at the start of the while loop which forces it to execute at least once. Doing it the way you did means that you have to duplicate code, allowing the possibility that it'll be subtly different.
You may want to validate the value for weeklyHours, should be a number >= 0


Numerate every item in dict I have a .json file with a complex structure: dict in a dict, it has no constant structure and is dynamically changed.

The goal is to make new dict where the keys are numbers of hierarchy and values must be strings that are composed of all previous steps like

2 drinks
2.1 drinks coffee
2.1.1 drinks coffee instant
2.1.2 drinks coffee real
2.2 drinks tea
2.3 drinks water

I wrote a script, and it works. But as I am new to Python and in programming, please check it out and correct me in order to code may look and perform more elegantly and in a pythonic way.

Also I would like to decrease time of execution because the input .json file is going to be quite big.

Sample cat.json file:

{
"communication":
    {"mobile":{
        "vodafone":{"subscr":"","txt":"","mms":"","internet":"","calls":
    {"in":{"home":"","roaming":""},"out":{"home":"","roaming":""}}},
        "verizon":{"subscr":"","txt":"","mms":"","internet":"1Gb","calls":
    {"in":{"home":"","roaming":""},"out":{"home":"500 min","roaming":"Other country"}}}},
    "internet":"SomeProviderName"
    },
"food":{"dairy":{"cheese":"Gauda","milk":{"brand":"name","origin":"place"}}},
"drinks":
        {
        "water":"",
        "tea":"",
        "coffee":
                {
                "instant":"",
                "real":""
                }}}

Script:

#!/usr/bin/env python -tt
# -*- coding: utf-8 -*-
import json
filetoread='cat.json'
def load_existed(filetoread):

try:
    data=json.loads(open(filetoread).read())
    return data

except ValueError:
    print 'data  loading error'

cat_data=load_existed(filetoread)

def walk_dict(d,mess,ln,new_dict,crumbs):
    inter=1
    lc=list(mess)
    last_crumb=crumbs.split( )

    for k,v in sorted(d.items(),key=lambda x: x[0]):
        if mess=='':
            mess=str(inter)
            lc=list(mess)
            last_crumb=crumbs.split( )
        if isinstance(v, dict) :
            ln=len(v)
            lc[len(lc)-1]=str(inter)
            mess="".join(lc)
            crumbs=" ".join(last_crumb)
            if len(crumbs.split( ))>0:
                crumbs=crumbs+" "+k
            else:
                crumbs=k
            #print mess,'-->',k,">"
            new_dict[mess]=crumbs
            mess=mess+'.1'
            walk_dict(v,mess,ln,new_dict,crumbs)
        else:
            if ln>0:
                ln=ln-1
                lc[len(lc)-1]=str(inter)
                mess="".join(lc)
                crumbs=" ".join(last_crumb)
                crumbs=crumbs+" "+k+" "+v
            #print  mess,'-->',"%s -> %s" % (k, v)
            new_dict[mess]=crumbs
        inter=inter+1
    return new_dict

new_data=walk_dict(cat_data,"",0,{},'')
print "-"*30
#print new_data
for v in sorted(new_data):
  print v, new_data[v]

Some notes about your code:

for readability, you should add spaces around operators
you should use with to open files; also, instead of first reading the file to a string and then using json.loads, just use json.load on the file itself
instead of running your own counter with inter, use enumerate
using key=lambda x: x[0] is pointless, it does not affect the sort order at all
you are constantly converting between strings and lists; stick to one representation, e.g. here: crumbs = " ".join(last_crumb) and right in the next line: if len(crumbs.split()) > 0:; instead, just test if last_crumb: or if crumbs:, whichever you prefer
lc[len(lc)-1] is the same as lc[-1]
there's some code duplication in the if/else; try to move that outside
your function seems to miss some of the items, e.g. you never output 1.2.2.4 communication mobile vodafone subscr; this is because you overwrite the ln parameter with ln = len(v); not sure why you need that ln parameter anyway...

Taking those into account, your walk_dict function can be simplified significantly:

def walk_dict(d, mess, new_dict, crumbs):
    lc = list(mess) if mess else ["1"]
    for inter, (k, v) in enumerate(sorted(d.items()), 1):
        lc[-1] = str(inter)
        mess = "".join(lc)
        if isinstance(v, dict) :
            crumbs2 = (crumbs + " " + k) if crumbs else k
            walk_dict(v, mess + '.1', new_dict, crumbs2)
        else:
            crumbs2 = crumbs + " " + k + " " + v
        new_dict[mess] = crumbs2
    return new_dict

But the way you convert mess into a list lc and then replace the last element is still -- quite fitting -- a "mess". Same with the way you are mixing "output parameters" and return values.

Here's how I would do it:

def walk_dict(d, key=None, parent=None):
    res = {}
    for i, e in enumerate(sorted(d), 1):
        k = (key    + "." + str(i)) if key    else str(i)
        p = (parent + " " +     e ) if parent else     e
        if isinstance(d[e], dict):
            res.update(walk_dict(d[e], k, p))
            res[k] = p
        else:
            res[k] = p + " " + str(d[e])
    return res

with open('data.json') as f:
    cat_data = json.load(f)
    new_data = walk_dict(cat_data)
    for v in sorted(new_data):
        print v, new_data[v]


Returning elements in a list also found in another list I'm trying to write a short function which takes a list (a) and returns another list which only contains the elements (x) such that -x is also in a. I've done that, and it works, but it runs really slowly. I know why it does, and I know the second for loop is unnecessary, but I don't know how to fix it without breaking the function.

def negated(a):
    mark = set()
    add_mark = mark.add
    b = []
    c = []
    for i in a:
        if i not in mark and not add_mark(i):
            b.append(i)
    for i in b:
        if -i in b:
            c.append(i)
    return c

First, let's get this out of the way:

The reason the OP version is slow is that, to decide whether to insert elements into c, it checks each one of them against b—a list, rather than a set. That means it's quadratic when it could be linear.

You could fix that by making another set, call it mark2 = set(b), then using if -i in mark2 instead of if -i in b.

That means your code is no longer quadratic, it's linear. Which is probably all you cared about.

However, it's looping twice as much as it needs to, and it's doing all of the looping in Python rather than finding ways to push it into C. That add_mark = mark.add optimization implies that you might be looking for more such tricks, and you should be able to cut the time to about a third.

But just fixing that isn't good enough once you get the community going, so a number of variations have been suggested both here and on a previous SO question. Which one is actually
fastest?

Instead of just guessing, let's actually write and test some implementations and see. Of course for real answers we need your real data, but I'll make up some data, and that should be enough to show you how to do it yourself.

# my answer on the earlier question, and my comment above
def negated_0(a):
    a = set(a)
    return [i for i in a if -i in a]

# Peter DeGlopper's comment, and hcwhsa's on the earlier question
def negated_1(a):
    s = set(a)
    return [i for i in a if -i in s]

# one suggestion I made on the earlier question
def negated_2(a):
    return set(a) & {-i for i in a}

# another suggestion I thought my be a little faster but not worth it
def negated_3(a):
    return {-i for i in a}.intersection(a)

# a possibly-improved version of #3 that I just thought of
def negated_4(a):
    a = set(a)
    return a & {-i for i in a}

# kojiro's posted answer
def negated_kojiro(a):
    '''Take a list a and returns another list which only contains the elements x such that -x is also in a'''
    neg, pos = set(), set()
    for i in a:
        if i < 0:
            neg.add(-1*i)
        else:
            pos.add(i)
    return [i for i in a if abs(i) in pos & neg]

import timeit
import random

a = [random.randint(-10000,10000) for _ in range(1000)]
for func in dir(sys.modules('__main__')):
    if func.startswith('negated_'):
        f = getattr(sys.modules('__main__'), func)
        print('{}: {}'.format(func, timeit.timeit(lambda: f(a), number=10000))

Here's the output on one laptop with Apple's Python 2.7.2:

negated_0: 1.50614500046
negated_1: 1.45001101494
negated_2: 1.79172492027
negated_3: 1.29876303673
negated_4: 1.92844605446
negated_kojiro: 84.5585548878

… and with a default-configured local build of Python 3.4 trunk:

negated_0: 1.5246370710083283
negated_1: 1.420855167991249
negated_2: 1.7558801580162253
negated_3: 1.297387560014613
negated_4: 1.8665565319824964
negated_kojiro: 72.18082603899529

… and with PyPy 2.1.0/2.7.3:

negated_0: 0.595048904419
negated_1: 0.405268907547
negated_2: 0.815263032913
negated_3: 0.878368139267
negated_4: 0.910092115402
negated_kojiro: 49.3969540596

Comparing the fixed version of the OP's implementation:

negated_op: 4.76454496384
negated_op: 3.3227077620103955
negated_op: 0.87509393692

(The original, unfixed version took over 79 seconds before I killed it in 2.7.2.)

We've improved it by 3x in CPython 2.7.2, but only 2x in the other implementations. Not quite what I'd hoped, but not bad.

It looks like negated_3 actually is a decent-sized win over the obvious implementation, but nothing else is. (And #4 actually makes things worse.)

Wrapping #2-#4 in list(…) so they return the same type as the others shows virtually no difference:

negated_0: 1.54194092751
negated_1: 1.44428801537
negated_2: 1.78255009651
negated_3: 1.29559803009
negated_4: 1.90534591675

Fixing the most serious problem with kojiro's answer, by evaluating pos & neg once outside the listcomp instead of for each element, gives me:

negated_kojiro_fix: 2.11841907501
negated_kojiro_fix: 3.84713697433
negated_kojiro_1: 0.739408969879

So, it's still significantly slower than negated_1 (which, like it, preserves duplicates and order) everywhere. Why?

It could be that it's doing twice as much setup work and not getting a comparable amount of savings in the actual comprehension. It may be simpler work, but still, adding O(N) loops in Python to speed up an O(N) list comprehension might be costly.

But there's also the fact that the listcomp filter is more complicated: abs(i) in mark instead of the -i in mark. So, if we could make it even simpler, would that make the listcomp even faster? Just keep the negated set around, and do i in mark. Which means you can replace the listcomp with a filter by using mark.__contains__ as the argument:

def negated_3b(a):
    mark = {-i for i in a}
    return filter(mark.__contains__, a)

(For Python 3, you have to make that list(filter(…)) of course, or it'll return almost instantly, having built an iterable but not iterated it.)

The results with the same three Python implementations are:

negated_3b: 1.39607014656
negated_3b: 1.8215457699843682
negated_3b: 0.51006603241

So… it beats all of the other order-and-dups-preserving methods in 2.x, CPython or PyPy, but not the set-returning negated_3 (which is obvious, when you think about it).

However, it's slower in 3.x. Maybe that's just because of the necessity of calling list? If you don't actually need a list, let's see how long it takes to iterate it, by just feeding the iterator into deque(maxlen=0):

negated_3b: 1.6808519120211713

Better, but still not as good as negated_1 in Python 3. Maybe it's just that 3.x's filter hasn't been improved since the original version was written as 2.x's itertools.ifilter, but listcomps have been improved multiple times over the years?

So, conclusions—if your data are very similar to mine (which is very unlikely!):

Unless this is such a bottleneck in your code that a few microseconds one way or the other actually make a difference, use whichever one is most readable.
If you don't need to preserve order and duplicates, use negated_3.
If you're using Python 2.x, use negated_3b.
Otherwise, use negated_1.


Fast algorithm for any(M==2) I want to find a quick way to see if a matrix M has at least one value that is, say, 2. In R, I would use any(M==2). However, this computes first M==2 for all values in M, then use any(). any() will stop at the first time a TRUE value is found, but that still means we computed way too many M==2 conditions.

I thought one could find a more efficient way, computing M==2 only as long as it is not satisfied. I tried to write a function to do this (either column-wise check, or on each element of M, check_2), but it is so far much slower. Any idea on how to improve this?

Results of benchmark, where the value Val is rather at the end of the matrix:

|expr               |mean time |
|:------------------|---------:|
|any(M == Val)      |  14.13623|
|is.element(Val, M) |  17.71230|
|check(M, Val)      |  18.20764|
|check_2(M, Val)    | 486.65347|

Code:

x <- 1:10^6
M <- matrix(x, ncol = 10, byrow=TRUE)

Val <- 50000

check <- function(x, Val) {
  i <- 1
  cond <- FALSE
  while(!cond & i <= ncol(x)) {
    cond <- any(M[,i]==Val)
    i <- i +1
  }
  cond
}

check_2 <- function(x, Val) {
  x_c <- c(x)
  i <- 1
  cond <- FALSE
  while(!cond & i <= length(x_c)) {
    cond <- x_c[i]==Val
    i <- i +1
  }
  cond
}

check_2(x=M, Val)
check(M, Val)

library(microbenchmark)

comp <- microbenchmark(any(M == Val),
               is.element(Val, M),
               check(M, Val),
               check_2(M, Val),
               times = 20)

comp

any is a primitive, it doesn't loop in R but in C, which is much much faster.

loops in R are quite slow, that's why it's important that you use said vectorized functions if you care about speed (apply functions are still loops however).

A way to speed things up is to use package Rcpp to write code in C++ through R, when you have a slow R function that uses simple loops it's the way to go, it's still not as fast as C but in our case maybe that'll be enough given we don't need to go through all the vector ?

Let's check:

# defines anyx_cpp
cppFunction(
  'bool anyx_cpp(const NumericVector x,const double y) {
     const double n = x.size();
     for (double i = 1; i < n; i++) {
       if (x(i) == y) {
         return(true);
       }
     }
  return false;
  }')

anyx_r <- function(x,y){
  for(x_ in x) if(x_ == y) return(TRUE)
  FALSE
}

vec <- 1:1e7
x <- 5e6
microbenchmark::microbenchmark(
  rloop  = anyx_r(vec,x),
  cpp    = anyx_cpp(vec,x),
  native = any(vec==x)
)

# Unit: milliseconds
#    expr      min        lq      mean   median       uq      max neval
#   rloop 166.5758 171.34355 203.15277 179.9776 198.8560 990.1650   100
#     cpp  39.5462  40.60585  57.84617  41.4594  46.1232 690.1746   100
#  native  36.9900  37.86090  51.80317  38.9640  43.6510 888.3059   100

Almost but not quite ;).

So bottom line, in general you can trust vectorized R functions, even if it might seem they're working too much at first sight.


Avoiding the use of ordinal() in an already-deployed enum While reviewing a PR, I found an enum being used like this:

class Entity {
    public enum STATUS { NEW, SENT, FAILED, OK }

    @Column(name = "STATUS") // a NUMBER(1) column in DB
    private int status; // + get/set
    //...
}

// Inside methods...
Entity e = new Entity();
e.setStatus(STATUS.SENT.ordinal());
EntityDAO.save(e);

Entity e = EntityDAO.findByStatus(STATUS.FAILED.ordinal());
logger.info("Found entity with status: " + STATUS.values()[e.getStatus()]);

Assume the necessary imports etc.

I find this use of ordinal() to be an antipattern, and consider that it should be avoided.
Instead, an inner field with the equivalent DB representation (e.g. NEW(1) or NEW("N")) should be used, in order to decouple the semantically meaningful representation of the enum value in the code (NEW) from the DB-optimized representation of such value when persisted (1 or 'N' or whatever).

Now, I don't have this POV for nothing: we've already had bugs stemming from altering the order of enum elements that were being used based on its ordinal() value (some times because of adding new values in between, some times because someone decided the values looked better in alphabetical order ¯\_(ツ)_/¯).

That's the case I made in the PR comments -- but in the end the PR got approved anyway, reasoning that my proposed correction would add too much clutter to the enum for little gain, and that we should all just be careful with enums.
And so the enum made its way to production.

Time has passed, and now I've got the chance to change that enum so its usage is not tied to the ordering of its elements.
However, since this has been in production for quite some time, I have to make it so the current DB values (0, 1, 2...) are kept and still interpreted correctly (as changing the DB values or column type would be too costly now).

This is the first version I came up with:

public enum STATUS {
    NEW (0),
    SENT (1),
    FAILED (2),
    OK (3),
    ;

    private final int val;

    STATUS (int val) { this.val = val; }

    public int val() { return val; }

    public static STATUS get(final int val) {
        switch (val) {
            case 0: return NEW;
            case 1: return SENT;
            case 2: return FAILED;
            case 3: return OK;
            default: throw new IllegalArgumentException();
        }
    }
}

I think this is as concise as it can get.
However, if a dev wanted to add a new enum value, they'd have to modify both the enum list and the switch inside the get(int) method. I find this to be inconvenient and, in a way, ammunition for my peers in favor of just using ordinal().
So I came up with this second version:

public static enum STATUS {
    NEW (0),
    SENT (1),
    FAILED (2),
    OK (3),
    ;

    private static final STATUS[] values;
    static { // like caching values() but sorted by dbValue
        STATUS[] st = values();
        values = new STATUS[st.length];
        for (STATUS s : st) {
            values[s.dbValue] = s;
        }
    }
    public static STATUS fromDB(final int dbValue) {
        return values[dbValue];
    }

    private final int dbValue;
    public int toDBValue() {
        return dbValue;
    }

    STATUS(int dbValue) {
        this.dbValue = dbValue;
    }
}

In this version, adding a new enum element is as simple as adding it to the list, just like a normal enum. And getting an element from its DB value is O(1) and rather efficient (compared to, say, using Streams to filter values() by dbValue every time). But maybe it is too complex for an enum? Also, this relies on DB values being consecutive numbers, as adding e.g. CANCELLED(99) would throw an AIOOB exception upon execution.

So my question is: how can I enhance this enum so adding elements to it is as simple as possible, getting elements by value is as efficient as possible, and it doesn't depend on ordinal()?

A negative parameter value or a parameter value >= values.length passed to fromDB() will throw an IndexOutOfBoundsException, which is less meaningful than the original IllegalArgumentException since the IndexOutOfBoundsException is complaining about an implementation detail rather than about bad input.

To avoid the IndexOutOfBoundsException, use a Map<Integer, STATUS> instead of an array.

However, if there are only a few defined values as in the question, a Map is probably not needed either.  fromDB() can simply iterate over values() and return the match when found, otherwise throw IllegalArgumentException.  Add the Map when performance profiling indicates fromDB() is a bottleneck.

Also, consider changing STATUS to Status, to follow standard naming conventions.


Countdown Timer for Tests I created this countdown timer as a tool to count down the remaining time on a test a few months ago. However, I looked at the code today and it looks like it needs some improvement, but I'm not sure what to fix.

Some problems I've already identified:

It makes a call to a macOS-specific command: say.
It uses the bell character to make noise (which generally seems like a nuisance).

import os
import time
import sys
class cl: # Thank you SO for these colors
    HEADER = '\033[95m'
    OKBLUE = '\033[94m'
    OKGREEN = '\033[92m'
    WARNING = '\033[93m'
    FAIL = '\033[91m'
    ENDC = '\033[0m'
    BOLD = '\033[1m'
    UNDERLINE = '\033[4m'
def countdown(secs, graceperiod):
    os.system("clear")
    for i in range (secs, -graceperiod, -1):
        if (i > 0):
            print cl.BOLD + cl.UNDERLINE + "%s:%02d" % (str((i)/60),(i)%60) + cl.ENDC
        elif (i == 0):
            print "STARTING GRACE PERIOD"
        else:
            print "EXTRA TIME: %s:%02d" % (str((-i)/60),(-i)%60)
        time.sleep(1)
        os.system("clear")
    print "\a"
    time.sleep(0.25)
    print "\a"
    time.sleep(0.25)
    print "\a"
    time.sleep(0.25)
    print "\a"
    time.sleep(0.25)
    os.system("say \"Time up! Put down your pencil!\"")
    print "\aTime up!"

raw_input("Ready? Press [Enter] to start. ")
timemins = 2
gracemins = 0
try:
    countdown(timemins,gracemins)
except KeyboardInterrupt:
    print cl.BOLD + cl.FAIL + "\nExiting Countdown" + cl.ENDC
    sys.exit(0)

Your code has a massive bug.

What do you expect the output of the following to be?

import time
clock = time.clock # Reduce errors introduced by me
start = clock()
for i in range(100):
    print(clock() - start)
    time.sleep(0.1)

So 100 * 0.1 = 10, so it should be about 10 seconds.
I ran this and the last print was 10.616061702852994.
This means that you get 6% more time using your program than you would otherwise.

This is a problem.
To fix it you should change the amount to sleep by subtracting from it the time it took to run the print.
And so you may get:

start = next_t = clock()
for _ in r:
    yield clock() - start
    next_t += d
    delay_t = next_t - clock()
    if delay_t < 0:
        continue
    sleep(delay_t)

This is better, as the print shouldn't exceed the delay it's good enough.
But it's not as accurate as I'd like.
While 1.101270229875297 may seem pretty accurate you can change it so that it is instead 1.100000042666685 accurate.
By changing sleep to only do say 90% of the delay, and you create a while loop to do the rest you can get much more accuracy.
This may not be needed, but knowing that the code is pretty accurate is nice.

And so I'd use:

def delay(d, total_time):
    r = range(int(total_time * (1 / d)))
    clock = time.clock
    sleep = time.sleep

    t = start = next_t = clock()
    for _ in r:
        yield t - start
        next_t += d
        t = clock()
        delay_t = (next_t - t) * 0.9
        if delay_t < 0:
            continue

        sleep(delay_t)
        while True:
            if t >= next_t:
                break
            t = clock()

This means that you'll have to change your for loop slightly.
You will need to remove the time.sleep,
and the output is the equivalent of using range(secs + graceperiod).
And so if you put i = int(round(sec - i)) at the beginning of your for loop, it's almost identical.

And so to improve your code further I'd:

Remove the brackets around your if statements.
Use str.format, and change i = -i in your else.
Stop using os.system('clear'), and instead use \r and print '...',.
Change your four print '\a''s to be in a for loop.
Change cl to Color.
Add a main. And use the if __name__ == '__main__': guard.

resulting in:

import os
import time

class Color:
    HEADER = '\033[95m'
    OKBLUE = '\033[94m'
    OKGREEN = '\033[92m'
    WARNING = '\033[93m'
    FAIL = '\033[91m'
    ENDC = '\033[0m'
    BOLD = '\033[1m'
    UNDERLINE = '\033[4m'

def delay(d, total_time):
    r = range(int(total_time * (1 / d)))
    clock = time.clock
    sleep = time.sleep

    t = start = next_t = clock()
    for _ in r:
        yield next_t - start
        next_t += d
        t = clock()
        delay_t = (next_t - t) * 0.9
        if delay_t < 0:
            continue

        sleep(delay_t)
        while True:
            if t >= next_t:
                break
            t = clock()

def countdown(secs, grace):
    prev = ''
    for diff in delay(1, secs + grace):
        i = int(round(secs - diff))
        if i > 0:
            message = '{0.BOLD}{0.UNDERLINE}{1}:{2:>02}{0.ENDC}'
        elif i == 0:
            message = 'STARTING GRACE PERIOD'
        else:
            message = 'EXTRA TIME: {1}:{2:>02}'
            i = -i
        this = message.format(Color, i / 60, i % 60)
        print prev + '\r' + this,
        prev = '\r' + ' ' * len(this)

    for diff in delay(0.25, 1):
        print '\a',
    print '\a\nTime up!'

    os.system('say "Time up! Put down your pencil!"')

def main(secs, grace):
    try:
        raw_input('Ready? Press [Enter] to start. ')
        countdown(secs, grace)
    except EOFError:
        pass
    except KeyboardInterrupt:
        print Color.BOLD + Color.FAIL + '\nExiting Countdown' + Color.ENDC

if __name__ == '__main__':
    main(10, 10)


Randomly generate a Hex Colour code and its contrasting colour using JavaScript I am attempting to generate a new colour and its contrasting colour with this javascript code.

I am making a web app that is different every time it is loaded (i.e. background and text colour)

I would love some critique on the quality and efficiency of my code.

How can it be done better?

Here is my code:

function startcolour() {
 var values = Array('1','2','3','4','5','6','7','8','9','A','B','C','D','E','F');
 var color = "#";
 var item;

 for (i = 0; i < 6; i++) {
   item = values[Math.floor(Math.random()*values.length)];
   color = color + item;
 }

 return color;
}

function contrastcolour(colour) {
 var r;
 var g;
 var b;
 var comp = [];
 var contrast = '#';
 var arrayLength;

 if (colour.charAt(0) == '#') {
  colour = colour.slice(1);
 }

 r = (255 - parseInt(colour.substring(0,2), 16)).toString(16);
 g = (255 - parseInt(colour.substring(2,4), 16)).toString(16);
 b = (255 - parseInt(colour.substring(4,6), 16)).toString(16);

 contrast = contrast + padZero(r) + padZero(g) + padZero(b);
 return contrast
}

function padZero(str) {
  if (str.length < 2) {
    return '0' + str;
  }
  else {
    return str;
  }
}

function colourRun() {
  var colours = [];
  var base = startcolour();

  colours.push(base);
  var contrast = contrastcolour(base);
  colours.push(contrast);
  return colours;
}

Sample output (aka colours):

['#123abc','#edc543']

I then do the following with jQuery to set CSS properties of HTML components:

  $(document).ready(function() {
    var colours = colourRun();
    $("body").css({"background-color": colours[0], "color": colours[1]});
    $("nav").css({"background-color": colours[1], "color": colours[0]});
  });

All constructive criticism is appreciated.

There is a similar question that you may want to take a look at for some extra insight, even thought it doesn't have the contrasting color feature:

JavaScript Random Color Generator

Now lets get to the review.

Naming

The common naming convection in JS for methods is camelCase which you didn't follow in general.
Most of the variable and function names aren't clear or descriptive of what they do/hold. This is a lot more important than it may seem at first, and not naming properly makes your code difficult to read and understand.

As a side note Uncle Bob has a full chapter on this topic on his Clean Code book.
Be consistent in the names. If you look closely you'll see that in some places you used color:

var color = "#";

While in others colour:

function contrastcolour(colour) {

This is at best confusing if not mysterious. Leaves one wondering if it is a different thing or if it has a different meaning.

For this review i went with colour which was the one you used more often.

Functions

Now an overall review of the functions you presented:

startcolour

Better than using an array of elements is using a string with all possible characters:

var values = '123456789ABCDEF';

You can still use the index operator to fetch a character as you do in a normal array.

There is no need to separate the '#' in a single variable and then add only
the color part to a different one, and the += operator is also more idiomatic
and still easy to read.

Don't use variables without declaring them, as you have in the for:

for (i = 0; i < 6; i++) {
//---^

i wasn't declared at all which makes it global, and may be the source of some hard to find bugs.

Taking all this into consideration the function could be rewritten to:

function randomColour(){
  const chars = '123456789ABCDEF';
  let colour = '#';

  for (let i = 0; i < 6; i++) {
    const randomIndex = Math.floor(Math.random() * chars.length);
    colour += chars[randomIndex];
  }
  return colour;
}

Or you can take a totally different approach, the one mentioned in the related question:

function randomColour(){
  const randNum = Math.floor(Math.random() * 16777216); //from 0 to ffffff
  return '#' + randNum.toString(16).padStart(6,"0");
}

This generates a number between 0 and 16777215, or ffffff in hexa, and outputs it with hexadecimal formatting, by calling toString(16). It is then padded with zeros to a length of 6.
padZero

There is no need to create a padding function, because there is one already, called padStart or padEnd. It even has a polyfill if you need to support some old browsers.

Using it in your code would look like so:

r.toString().padStart(2,"0")

Note that it's a method on string therefore if i have a number i must first transform it to string.
colourRun

The name on this one is also not very obvious to what it does. If the function returns a random color and it's contrast a better name would be baseContrastColours.

Its important to mention that push supports any number of elements, so there is no need to make two separate pushes. Also in this specific case its easier to just return the array literal with the respective elements instead of creating the array, pushing the elements and returning at the end.

Like this:

function baseContrastColours() {
  const base = randomColour();
  const contrast = contrastColour(base);
  return [base, contrast];
}

While it has the same functionality, its simpler and easier to read.
contrastcolour

All the r, g, b variables are created up top and only set below:

function contrastcolour(colour) {
  var r;
  var g;
  var b;
  ...

  r = (255 - parseInt(colour.substring(0,2), 16)).toString(16);
  g = (255 - parseInt(colour.substring(2,4), 16)).toString(16);
  b = (255 - parseInt(colour.substring(4,6), 16)).toString(16);
  ...
}

You want to create variables and set the values directly if there is nothing else to do in between both actions:

var r = (255 - parseInt(colour.substring(0,2), 16)).toString(16);

Considering the variables were declared with var they are even hoisted. Also in general you want to declare the variables as close as possible to were you use them.

The arrayLength and comp variables aren't used. Always remove unused variables to keep the code as clean as possible.

Also important is that you are repeating the inverse logic 3 times, one for each color channel. So while simple you may want to consider abstracting that logic to a separate function.

So this function could be rewritten as follows:

function inverseChannelColour(channelColour){
  return (255 - parseInt(channelColour, 16)).toString(16);
}

function contrastColour(colour) {
  if (colour.charAt(0) == '#') {
    colour = colour.slice(1);
  }

  const r = inverseChannelColour(colour.substring(0,2));
  const g = inverseChannelColour(colour.substring(2,4));
  const b = inverseChannelColour(colour.substring(4,6));
  const contrast = '#' + r.toString().padStart(2,"0")
                       + g.toString().padStart(2,"0")
                       + b.toString().padStart(2,"0");
  return contrast;
}

As pointed out by @Zeta in the comments if the color to be inverted is a gray very close to the middle, the inversion will generate a similar color which may not be easy to read.

You can try to avoid this by manually checking the difference of all channels to their inverted versions, and if all of them are bellow a certain threshold generate a different color. A simple approach to this would be to generate black if all channels are closer to white and white otherwise.

There are also some libraries/micro-libraries that generate colors and their complementary versions with other approaches, such as using HSL. These may be interesting for you to take a look.

Here are some of them for reference:

invert-color
Colors.js
complementary-colors


Is this a good isNaN method? public boolean isNaN(String string){

@SuppressWarnings("unused")
BigDecimal bg = null;

try{

    bg = new BigDecimal(string);

}catch(Exception e){

    return true;
}

return false;
}

There is no native isNaN method for BigDecimal.  The thing I don't like about this method is the suppressed warning. I never use the variable but only the constructor to see if it doesn't throw an exception.  If it does, I return the corresponding boolean.

What do you think?

You said :

  I never use the variable but only the constructor to see if it doesn't throw an exception

I would ask you, then why did you declare a variable at all ? You could have done :

public boolean isNaN(final String string) {
    try {
       new BigDecimal(string);
    } catch (final Exception e) {
        return true;
    }
    return false;
}

When you don't need a variable, don't declare it.

You should not catch Exception because it's too generic. What do you really want ? The constructor throw NumberFormatException if the argument is not valid, the better option would be to catch that one instead.


Longest palindromic subsequence by memoization I've written some code to solve the longest palindromic subsequence problem.

def memoize(f):
    """ Function decorator used to cache results """
    d = dict()
    def inner(*args):
        if args not in d:
            d[args] = f(*args)
        return d[args]
    return inner

def max_palindrome(s):
    """ Returns the longest length palindrome that can
        be generated from s

    >>> max_palindrome("BBABCBCAB")
    7
    """

    @memoize
    def inner(s):
        if len(s) == 1:
            return 1
        if s[0] == s[-1]:
            return 2 + inner(s[1:-1])
        return max(inner(s[1:]), inner(s[:-1]))
    return inner(s)

My solution uses memoization to improve performance. I'm wondering how my code can be improved to return the longest subsequence rather than just the longest length achievable from the input characters.

The simplistic answer, possibly not the most efficient answer, is simply to build a tuple result of the length, and the string, and then memoize that one.

When trying with longer strings, it turns out that your version of the recursive version from the link is not complete. You didn't handle the len(s) == 2 case which led to index errors. With that fixed, and the tuple returns, the code now looks like:

def max_palindrome(s):
    """ Returns the longest length palindrome that can
        be generated from s

    >>> max_palindrome("BBABCBCAB")
    (7, "BACBCAB")[1] -> BACBCAB
    """

    @memoize
    def inner(s):

        if len(s) == 1:
            return 1, s

        if s[0] != s[-1]:
            _, recursed = max(inner(s[1:]), inner(s[:-1]))
            return len(recursed), recursed

        elif s[0] == s[-1]:

            if len(s) == 2:
                return 2, s
            else:
                new_text = '{0}{1}{0}'.format(s[0], inner(s[1:-1])[1])
                return len(new_text), new_text

    return inner(s)[1]

You could also improve your code by commenting a little more, and giving some proper names instead of single character variables, which are usually reserverd for loops.

Addendum: Faulty algorithm

The suggested algorithm from your link, doesn't explore all possibilities available. Neither lengthwise, nor is all combinations covered. The following two test cases prove they are faulty:

print max_palindrome("abcABCBAabc")  # (7, 'cABCBAc')
print max_palindrome("abcABCBAcba")  # (11, 'abcABCBAcba')

Both of these should return the latter, if all combinations had been considered. This simple test run was done without memoization, which could introduce other faults. Did also test with the same strings in the online code generator linked from your proposed link.

Not that your link also states that you can rebuild the characters any way you like, whilst a more normal version is that the palindrome string must be displayed within the string itself. I.e. your links says that in the string "BBABCBCAB", all of the following are palindromes: "BACBCAB", "BBCBB", and "BBBBB". Neither of these are substrings.

Wikipedia has the more normal definition of longest palindromic substring and here is a Stack Overflow answer with links to two implementations of Manacher's algorithm.


Copy files mentioned in an Excel sheet Please help me in reducing the time complexity of this code as it is taking a long time to read its Excel input.

# -*- coding: utf-8 -*-
"""
Created on Wed Mar  9 14:26:22 2016

@author: DalalNS
"""
from openpyxl import load_workbook
import os
import shutil
filename = 'test.xlsx'
wb = load_workbook(filename, use_iterators=True)
sheetlist=(wb.get_sheet_names())
searclist=[]
for i in range(len(sheetlist)):
    ws=wb[sheetlist[i]]
    for row in ws.iter_rows():
        for cell in row:
            if(cell[1]=='B'):
                searclist.append((cell.internal_value))
searclist = [os.path.splitext(each)[0] for each in searclist]

def search(searclist):
    for root, dirs, files in os.walk('C://Users/DalalNS/Desktop/Mapping/a/b'):
        for _file in files:
            try:
                print(_file[:-4])

                if _file[:-4] in searclist:
            # If we find it, notify us about it and copy it it to C:\NewPath\
                    print('Found file in: ' + str(root))
                    shutil.copy(os.path.abspath(root + '/' + _file), 'C://Users/DalalNS/Desktop/Mapping/output')
                elif _file[:-5] in searclist:
            # If we find it, notify us about it and copy it it to C:\NewPath\
                    print('Found file in: ' + str(root))
                    shutil.copy(os.path.abspath(root + '/' + _file), 'C://Users/DalalNS/Desktop/Mapping/output')

                else:
                    print("not found")
            except:
                print("runtime exception")
print(search(searclist))

Building a list using append in a for loop is not only slow, it is an anti-pattern. Use list comprehensions instead:

searclist = [cell.internal_value for i in range(len(sheetlist)) for row in wb[sheetlist[i]].iter_rows() for cell in row if cell[1] == 'B']

This is not as readable as the previous form, lets try to fix that by iterating over sheet names directly instead of using an index, it will also speed things up.

searclist = [cell.internal_value for sheet in sheetlist for row in wb[sheet].iter_rows() for cell in row if cell[1] == 'B']

Not entirely better. In such cases you can try using a generator: simply wraps your loops into a function that yield results instead of appending them in a list:

def files_in_xl(filename):
    workbook = load_workbook(filename, use_iterators=True)
    for sheet in workbook.get_sheet_names():
        worksheet = workbook[sheet]
        for row in worksheet.iter_rows():
            for cell in row:
                if cell[1] == 'B':
                    yield cell.internal_value

You can then easily create searclist using:

searclist = list(files_in_xl('test.xlsx'))

But we’re not done. First off, you iterate over that list again to remove file extensions; why not do that when getting the file name:

yield os.path.splitext(cell.internal_value)[0]

Second, openpyxl documentation suggest using read only mode for reduced memory footprint:

def files_in_xl(filename):
    workbook = load_workbook(filename, read_only=True)
    for sheet in workbook.get_sheet_names():
        worksheet = workbook[sheet]
        for row in worksheet.rows:
            for cell in row:
                if cell[1] == 'B':
                    yield os.path.splitext(cell.value)[0]

Third, you will use searclist only to check if values are in there. Lists are terrible for that purpose since the in operation is \$O(n)\$. Use sets instead, this operation is \$O(1)\$:

patterns = set(files_in_xl('test.xlsx'))  # Renamed searclist to patterns

Now on to your search function; which is poorly named since it both search and copy:

You used os.path.splitext previously to separate the filename from its extension and here you try to do it manualy: it is both inefficient and lead to duplicated code.
You swallow meaningful information by using a bare except: if something goes wrong, the user is only left with your "there was an error" message and nothing to get a hint at what the error was. Never use bare except, always specify the kind of exception you’re expecting; and only expect exceptions you are ready to handle (printing a message is useless here since the traceback has so much more informations).
Use os.path.join to build paths instead of manually concatenating a '/'.
You should also allow the user to pass the "in" and "out" path as parameters.

Proposed improvements

# -*- coding: utf-8 -*-
"""
Created on Wed Mar  9 14:26:22 2016

@author: DalalNS
"""

from openpyxl import load_workbook
import os
import shutil
import traceback

def files_in_xl(filename):
    workbook = load_workbook(filename, read_only=True)
    for sheet in workbook.get_sheet_names():
        worksheet = workbook[sheet]
        for row in worksheet.rows:
            for cell in row:
                if cell[1] == 'B':
                    yield os.path.splitext(cell.value)[0]

def filter_and_copy(in_dir, out_dir, search_patterns):
    for root, dirs, files in os.walk(in_dir):
        absolute_root_path = os.path.abspath(root)
        for _file in files:
            filename = os.path.splitext(_file)[0]
            if filename in search_patterns:
                src_path = os.path.join(absolute_root_path, _file)
                dest_path = os.path.join(out_dir, _file)
                try:
                    shutil.copyfile(src_path, dest_path)
                except (OSError, SameFileError) as e:
                    # Alert the user if we couldn't copy and continue with other files (you may want something better than that).
                    print('Error while copying', src_path, 'to', dest_path)
                    traceback.print_exc()
                else:
                    print(src_path, 'copied to', dest_path)
            else:
                print(filename, "not found")

if __name__ == '__main__':
    patterns = set(files_in_xl('test.xlsx'))
    filter_and_copy(
        'C://Users/DalalNS/Desktop/Mapping/a/b',
        'C://Users/DalalNS/Desktop/Mapping/output',
        patterns)

I also changed shutils.copy to shutils.copyfile as it might be a little bit faster. Check the documentation for your version of Python to change the exceptions you want to handle accordingly.


Countdown Timer in Python I am a beginner in Python and I am yet to move on from the basics to things like OOP.I wrote all of this code (Python 3) in an hour or so for a simple countdown timer.Although some parts of it are seemingly excessive,is the core of the code pythonic enough or are there better ways of structuring it?I would also like to know a better way for writing comments and variables effectively.More specifically I think the IOTimer() function could be cleaner.

#modules
import datetime
import time
#TIMER
#Enter Preferences

supported_formats = ["hrs","min","sec"]

print("\n")

def get_input():
    global unit_input
    unit_input = input("Enter the format in which you want to time yourself:{}:".format(supported_formats))
    unit_input.lower()

#Check if the format entered is valid.
get_input()

if unit_input in supported_formats:
    pass
else:
    print("Invalid Input.Please re-enter you desired format.\nThe input is case insenseitive")
    get_input()

def countdown_timer(x):
    while x >= 0 :
        x -= 1
        print("{} remaining".format(str(datetime.timedelta(seconds=x))))
        print("\n")
        time.sleep(1)

#Check for input and assign variable to determine the total amount of seconds
def IOTimer():
    while True:
        try:
            if unit_input == "hrs":
                hours  = int(input("Enter the number of hours: "))
                print("\n")
                minutes = int(input("Enter the number of minutes: "))
                print("\n")
                seconds = int(input("Enter the number of seconds: "))
                print("\n")
                break
            elif unit_input == "min":
                hours = 0
                minutes = int(input("Enter the number of minutes: "))
                print("\n")
                seconds = int(input("Enter the number of seconds: "))
                print("\n")
                break
            elif unit_input == "sec":
                hours = 0
                minutes = 0
                seconds = int(input("Enter the number of seconds: "))
                print("\n")
                break
        except:
            print("Invalid Input.Re-enter the values")
            print("\n")
            IOTimer()
    hours_in_sec = hours*3600
    minutes_in_sec = minutes*60
    total_seconds = hours_in_sec + minutes_in_sec + seconds
    print("Starting countdown for {}".format(str(datetime.timedelta(seconds=total_seconds))))
    print("\n")
    time.sleep(2)
    countdown_timer(total_seconds)

IOTimer()

Before entering into the meat of the answer, let's talk about time accuracy for a bit.

For starter, time.sleep is not extremely accurate as it can be (a little bit) longer than the requested sleeping time. And even if it was, you’re performing work inbetween the repeated sleep calls, which take time and add up. So each cycle in the while loop of countdown_timer take more than a single second. Let's evaluate how much by using the following file:

import time
import datetime
import timeit

def countdown_timer(x):
    while x >= 0 :
        x -= 1
        print("{} remaining".format(str(datetime.timedelta(seconds=x))))
        print("\n")
        time.sleep(1)

if __name__ == '__main__':
    print(timeit.timeit(lambda:countdown_timer(120), number=1))

And the output is:

0:01:59 remaining

0:01:58 remaining

[Snip for readability]

0:00:01 remaining

0:00:00 remaining

-1 day, 23:59:59 remaining

121.14761522800109

Wow… That remaining time at the end… That's unexpected. Thing is that the way your while loop is setup, you are performing x + 1 iterations; which explains the 121 seconds your countdown take as well as that last remaining time. Just change the condition to while x > 0: and you’re good. But do you see that your counter drift of about 150ms over the course of two minutes? That will be 1 second and a half after 20 minutes, between 4 and 5 seconds on a 1 hour timer and several minutes for a 1 day timer…

You need to account for that drift. And the easiest way to do it is to maintain an ideal time to sleep up to which is 1 second in the future, and then compute the difference between the current time and this ideal time to try and sleep this exact amount of time. Luckily, this difference is easily computed using timedelta.total_seconds:

import time
import datetime
import timeit

def countdown_timer(x, now=datetime.datetime.now):
    target = now()
    one_second_later = datetime.timedelta(seconds=1)
    for remaining in range(x, 0, -1):
        target += one_second_later
        print(datetime.timedelta(seconds=remaining), 'remaining', end='\r')
        time.sleep((target - now()).total_seconds())
    print('\nTIMER ended')

if __name__ == '__main__':
    print(timeit.timeit(lambda:countdown_timer(120), number=1))

And the timing is way better:

0:00:00 remaining
TIMER ended
120.00124583899742

Just a single millisecond of drift. In fact, this drift is always compensated for and never accumulate. A run for 1200 seconds outputs:

0:00:00 remaining
TIMER ended
1200.0010585399978

Now for the rest of the code. Did you note how the calling code was under an if __name__ == '__main__': clause? You should get into this habit as it allows to more easily import and test the code you are developping. This will also hopefully force you to gather your code into functions and to make better usage of parameters and return values.

A coarse rewrite could look like:

import time
import datetime

def countdown_timer(x, now=datetime.datetime.now):
    target = now()
    one_second_later = datetime.timedelta(seconds=1)
    for remaining in range(x, 0, -1):
        target += one_second_later
        print(datetime.timedelta(seconds=remaining), 'remaining', end='\r')
        time.sleep((target - now()).total_seconds())
    print('\nTIMER ended')

def get_input_format(supported_formats=('hrs', 'min', 'sec')):
    while True:
        units = input('Enter the format in which you want to time yourself {}: '.format(supported_formats))
        if units in supported_formats:
            return units
        print('Invalid input. Please re-enter your desired format')
        print('The input is case sensitive')

def IOTimer(unit_input):
    while True:
        try:
            if unit_input == "hrs":
                hours = int(input("Enter the number of hours: "))
                print("\n")
                minutes = int(input("Enter the number of minutes: "))
                print("\n")
                seconds = int(input("Enter the number of seconds: "))
                print("\n")
                break
            elif unit_input == "min":
                hours = 0
                minutes = int(input("Enter the number of minutes: "))
                print("\n")
                seconds = int(input("Enter the number of seconds: "))
                print("\n")
                break
            elif unit_input == "sec":
                hours = 0
                minutes = 0
                seconds = int(input("Enter the number of seconds: "))
                print("\n")
                break
        except:
            print("Invalid Input. Re-enter the values")
            print("\n")
    hours_in_sec = hours*3600
    minutes_in_sec = minutes*60
    total_seconds = hours_in_sec + minutes_in_sec + seconds
    print("Starting countdown for {}".format(str(datetime.timedelta(seconds=total_seconds))))
    print("\n")
    time.sleep(2)
    countdown_timer(total_seconds)

if __name__ == '__main__':
    IOTimer(get_input_format())

But there is still improvement for the IOTimer function:

the name looks like it is a class as per PEP8, the official Python style guide, function names should be lower_snake_case;
I already removed the recursive call as it is already handled by the while True: loop;
there is a lot of repetitions in the various if statements;
you should not use a bare except statement, always specify the exceptions you’re expecting, otherwise it may create hard-to-diagnose bugs;
I hardly see a valid reason to sleep 2 seconds before starting the timer.

Revised version look like:

import time
import datetime

def countdown_timer(x, now=datetime.datetime.now):
    target = now()
    one_second_later = datetime.timedelta(seconds=1)
    for remaining in range(x, 0, -1):
        target += one_second_later
        print(datetime.timedelta(seconds=remaining), 'remaining', end='\r')
        time.sleep((target - now()).total_seconds())
    print('\nTIMER ended')

def get_input_format(supported_formats=('hrs', 'min', 'sec')):
    while True:
        units = input('Enter the format in which you want to time yourself {}: '.format(supported_formats))
        if units in supported_formats:
            return units
        print('Invalid input. Please re-enter your desired format')
        print('The input is case sensitive')

def main(unit_input):
    hours = minutes = seconds = 0
    while True:
        try:
            if unit_input == 'hrs':
                hours = int(input('Enter the number of hours: '))
            if unit_input != 'sec':
                minutes = int(input('Enter the number of minutes: '))
            seconds = int(input('Enter the number of seconds: '))
        except ValueError:
            print('Invalid Input. Re-enter the values')
        else:
            break
    delay = datetime.timedelta(hours=hours, minutes=minutes, seconds=seconds)
    print('Starting countdown for {}'.format(delay))
    countdown_timer(int(delay.total_seconds()))

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

But the whole point of these two extra functions is to only gather parameters for the script from the user. Let specialized modules do the work for you. Let me introduce you to argparse:

import time
import datetime
import argparse

def countdown_timer(x, now=datetime.datetime.now):
    target = now()
    one_second_later = datetime.timedelta(seconds=1)
    for remaining in range(x, 0, -1):
        target += one_second_later
        print(datetime.timedelta(seconds=remaining), 'remaining', end='\r')
        time.sleep((target - now()).total_seconds())
    print('\nTIMER ended')

def command_line_parser():
    parser = argparse.ArgumentParser(description='Simple countdown timer', conflict_handler='resolve')
    parser.add_argument('seconds', type=int, help='amount of seconds to wait for')
    parser.add_argument('-m', '--minutes', '--min', type=int, help='additional amount of minutes to wait for')
    parser.add_argument('-h', '--hours', type=int, help='additional amount of hours to wait for')
    return parser

if __name__ == '__main__':
    args = command_line_parser().parse_args()
    delay = datetime.timedelta(**vars(args))
    print('Starting countdown for', delay)
    countdown_timer(int(delay.total_seconds()))

Usage being:

$ python simple_timer.py -h 1 -m 23 45


String Trie in OCaml I'm pretty new to OCaml, so any feedback is appreciated. The goal is to implement a trie that efficiently stores and sorts strings.

module CharMap = Map.Make(Char)

(* count of members of the set that end at this node * mapping from
   next char => children *)
type trie = Node of int * trie CharMap.t

let empty = Node (0, CharMap.empty)

(* Add a string to the trie *)
let rec add (Node (count, children) : trie) = function
  | "" -> Node (count + 1, children)
  | str ->
    let firstChar = String.get str 0 in
    let lastChars = String.sub str 1 ((String.length str) - 1) in
    let newTrie = if CharMap.mem firstChar children
      then add (CharMap.find firstChar children) lastChars
      else add empty lastChars in
    let newChildren = CharMap.add firstChar newTrie children in
    Node (count, newChildren)

let addAll (trie : trie) (strs : string list) : trie =
  List.fold_left (fun trieAcc str -> add trieAcc str) empty strs

(* Get all the strings of a trie *)
let traverse (trie : trie) : string list =
  let rec helper (Node (count, children) : trie) (prevChar : char option)
      : string list =
    let string_of_char chr =
      match chr with
        | None -> ""
        | Some ch -> String.make 1 ch in
    let perChild = List.map (fun (chr, child) ->
      (helper child (Some chr))) (CharMap.bindings children) in
      let fromChildren = List.flatten perChild in
    let withPrev =
      List.map (fun str -> (string_of_char prevChar) ^ str) fromChildren in
    let rec clone str count =
      if count = 0 then [] else str::(clone str (count - 1))
    in (clone (string_of_char prevChar) count) @ withPrev in
  helper trie None

let sort (strs: string list): string list =
  traverse (addAll empty strs)

What do you think?

You're pretty new to OCaml? You're using OCaml 3.12 features and probably did a lot of functional programming before. Let's see if I can show what idioms I've seen before. I'm not an OCaml expert though, far from it.

Empty trie Consider using something like Empty to denote the empty trie. type trie = Empty | Node of int * trie CharMap.t This is better than your empty hack, and will allow you to take advantage of OCaml's pattern matching.

string_of_char is a nice OCaml name, but this is not really string_of_char, but string_of_char_option. Since it's not important, I'd tend to make it look smaller in the code.

let string_of_char_option = function None -> "" | Some ch -> String.make 1 ch

Auxiliary function name You don't need to use the name helper: traverse is fine. It's not ambiguous since traverse is not recursive. This applies to all functions with auxiliary functions.

Indentation I'm not sure I understand how you choose to put your ins. Also, fromChildren has a wrong indentation.


Updating field in Temporary table I have a more massive sp, in which I use some temporary tables, below is a part of the sp where I am updating a field in a temporary table two times.

Update #BalSheet
set OBCr = OBDr + isnull(trialDeb.amount,0)
from (
    Select v.AccountId,
        v.CompanyId,
        Sum(isnull(v.Amount,0)) Amount
    From #vwJournalDetails v , #TempJournalHeader h
    where v.JournalCredit = 1 and // 1
        h.JournalId = v.JournalId and
        h.CompanyId = v.CompanyId and
        h.JournalDate >= @dtPrevFinUnclosedSDate and
        h.JournalDate <= @dtLastFinUnclosedEDate
    group by v.CompanyId, v.AccountId
) trialDeb
Where trialDeb.Accountid = #BalSheet.AccId and
    trialDeb.CompanyId = #BalSheet.CompanyID and
    trialDeb.CompanyId = @intCompanyId and
    #BalSheet.AcType ='L'

Update #BalSheet
set OBCr = OBDr - isnull(trialDeb.amount,0)
from (
    Select v.AccountId,
        v.CompanyId,
        Sum(isnull(v.Amount,0)) Amount
    From #vwJournalDetails v , #TempJournalHeader h
    where v.JournalCredit = 0 and // 2
        h.JournalId = v.JournalId and
        h.CompanyId = v.CompanyId and
        h.JournalDate >= @dtPrevFinUnclosedSDate and
        h.JournalDate <= @dtLastFinUnclosedEDate
    group by v.CompanyId, v.AccountId
) trialDeb
Where trialDeb.Accountid = #BalSheet.AccId and
    trialDeb.CompanyId = #BalSheet.CompanyID and
    trialDeb.CompanyId = @intCompanyId and
    #BalSheet.AcType ='L'

As you can see, the two update query looks similar, and the only difference is in the where condition checking v.JournalCredit value.

If the v.JournalCredit = 1 I add the amount to the current value in the table, and when the v.JournalCredit = 0 I subtract the amount from the current value.

it is working correctly, but there are other update queries similar to this in the sp, So I was wondering if there is any way to mix these update query into one?

You can use a CASE statement in your SET to choose the operation to perform.  Not only will this make maintenance easier, it will improve performance as well since you'll only be touching the target table once.

UPDATE  #BalSheet
SET OBCr = CASE
             WHEN trialDeb.JournalCredit = 1 THEN OBDr + ISNULL(trialDeb.Amount, 0)
             WHEN trialDeb.JournalCredit = 0 THEN OBDr - ISNULL(trialDeb.Amount, 0)
           END
FROM  ( SELECT  v.AccountId,
                v.CompanyId,
                v.JournalCredit,
                SUM(ISNULL(v.Amount, 0)) AS Amount
        FROM  #vwJournalDetails AS v,
              #TempJournalHeader AS h
        WHERE v.JournalCredit IN ( 0, 1 )
          AND h.JournalId = v.JournalId
          AND h.CompanyId = v.CompanyId
          AND h.JournalDate >= @dtPrevFinUnclosedSDate
          AND h.JournalDate <= @dtLastFinUnclosedEDate
        GROUP BY v.CompanyId,
                 v.AccountId,
                 v.JournalCredit) AS trialDeb
WHERE trialDeb.Accountid = #BalSheet.AccId
  AND trialDeb.CompanyId = #BalSheet.CompanyID
  AND trialDeb.CompanyId = @intCompanyId
  AND #BalSheet.AcType = 'L' ;

Note that I don't know if you still need the v.JournalCredit filter so I just changed it to an IN clause to ensure only 0 & 1 were touched.  If those are the only two possible values, you can remove that predicate to improve the performance as well.