Toggling a taxi between busy and available states Is there a better way to do this? That switch_status method seems like it could be made a lot more compact.

class Taxi
  def initialize
    @status = :available
  end

  def switch_status
    if @status == :available
      @status = :busy
    else
      @status = :available
    end
  end
end

t = Taxi.new
t.switch_status

I voted to reopen, though I still don't consider this a good CodeReview question. Perhaps my answer here will shed some light on why I think that.

You say you want to shorten the switch_status method. Well, Ruby has ternaries, same as many other languages so:

def switch_status
  @status = (@status == :available ? :busy : :available)
end

And done. Shorter.

But the reason this question feels incomplete is because that, in and of itself, is completely equivalent to the original code. So it's not really a review, just a demonstration of an alternative if..else syntax found in many languages.

A review would require knowing more about why this method exists. What its purpose is, when you need it, why it's using symbols, and so on.

To start, just having the switch_state method is odd. You presumably have to know what state the taxi (as the class name suggests it is) is already in, in order to see if switching it makes sense. But your code contains no mechanism for determining the current state. In fact, the only way to get a Taxi instance to return something is to change its state. Calling switch_state will return the new state, and you'll have to work backwards from there or switch it back again.

E.g. suppose you have an instance, and want to know its state. Currently, you'd have to do this:

some_taxi.switch_state # flip it once
current_state = some_taxi.switch_state # flip it back again

That makes very little sense.

Also: You're switching between exactly two states - which is what booleans do already. So why aren't you using booleans? They're limited to two states, so you're certain it's either one or the other (whereas a symbol can be anything). So you could just as well do this:

def switch_status
  @busy = !@busy
end

Really, that should do the same trick, since this taxi has only two states.

And also, given the lack of context, I'd say you're entire class could, in effect, be replaced with just a boolean, since the class's only purpose right now is to store a boolean state. So cut out the middleman. A raw boolean value would even tell you its current state, which is more than your class does now.

But presumably you want the encapsulation and the symbols for something. Or do you want symbols? A more Ruby'esque way to inquire about a known set of states would be with ?-methods:

def busy?
  @busy
end

def available?
  !@busy
end

Or you can just have one of 'em and negate their value, e.g. !taxi.busy?. Either way, it's infinitely better than flipping the state twice just to find out what it currently is.

If you still want symbols, write an accessor:

def status
  @busy ? :busy : :available
end

Then again, if this is a taxi, does it really have only those two states? Maybe one is :out_of_service for instance?

Again, no idea. The question doesn't say.

Or it could be that available/busy can be - and likely should be - inferred from something else. There's no reason to manually track multiple separate state values if they're dependent on each other. In fact, it's likely harmful to try. So perhaps your code could do this instead:

def status
  @passengers.any? ? :busy : :available
end

Now status is derived from something (presumably) more meaningful.

But does this make sense? No idea. The question doesn't say.

As mentioned, just having switch_state is odd because presumably there's more to it than that: The state is connected to something. Otherwise, as I also mentioned, you could replace the whole class with a simple boolean value. Only being able to manipulate the state by flipping it seems weird. Like a light switch you can only even flip to the opposite of whatever it is - and you don't know whether that's on or off.


Refactoring coordinates for Minecraft Pi buildings written in Python I'm fairly new to coding and I wrote this script in Python for a house with a roof in Minecraft Pi. I've found different ways to write the coordinates to make the code base shorter, but I'm wondering if it is possible for me to refactor it any further so I don't have to list each coordinate line by line.

import mcpi.minecraft as minecraft
import mcpi.block as block
mc = minecraft.Minecraft.create()
p = mc.player.getTilePos()

mc.setBlocks(p.x+1, p.y, p.z+1, #Main structure starting coordinates (DO NOT ALTER)
             p.x+19, p.y+4, p.z+6, #Structure size/coordinates (Alters cube size)
             block.STONE_BRICK)

mc.setBlocks(p.x+2, p.y+1, p.z+2, #Hollowing out main structure interior
             p.x+18, p.y+3, p.z+5,
             block.AIR)

mc.setBlocks(p.x, p.y+4, p.z, #Main Structure Roof (Front Side, 1st Row)
             p.x+12, p.y+4, p.z,
             block.STAIRS_WOOD.id, 2)

mc.setBlocks(p.x+20, p.y+4, p.z,
             p.x+20, p.y+4, p.z,
             block.STAIRS_WOOD.id, 2)

mc.setBlocks(p.x, p.y+5, p.z+1, #Main Structure Roof (Front Side, 2nd Row)
             p.x+13, p.y+5, p.z+1,
             block.STAIRS_WOOD.id, 2)

mc.setBlocks(p.x+19, p.y+5, p.z+1,
             p.x+20, p.y+5, p.z+1,
             block.STAIRS_WOOD.id, 2)

mc.setBlocks(p.x, p.y+6, p.z+2, #Main Structure Roof (Front Side, 3rd Row)
             p.x+14, p.y+6, p.z+2,
             block.STAIRS_WOOD.id, 2)

mc.setBlocks(p.x+18, p.y+6, p.z+2,
             p.x+20, p.y+6, p.z+2,
             block.STAIRS_WOOD.id, 2)

mc.setBlocks(p.x, p.y+7, p.z+3, #Main Structure Roof (Front Side, 4th Row)
             p.x+20, p.y+7, p.z+3,
             block.STAIRS_WOOD.id, 2)

mc.setBlocks(p.x, p.y+4, p.z+7, #Main Structure Roof (Rear Side, 1st Row)
             p.x+20, p.y+4, p.z+7,
             block.STAIRS_WOOD.id, 3)

mc.setBlocks(p.x, p.y+5, p.z+6, #Main Structure Roof (Rear Side, 2nd Row)
             p.x+20, p.y+5, p.z+6,
             block.STAIRS_WOOD.id, 3)

mc.setBlocks(p.x, p.y+6, p.z+5, #Main Structure Roof (Rear Side, 3rd Row)
             p.x+20, p.y+6, p.z+5,
             block.STAIRS_WOOD.id, 3)

mc.setBlocks(p.x, p.y+7, p.z+4, #Main Structure Roof (Rear Side, 4th Row)
             p.x+20, p.y+7, p.z+4,
             block.STAIRS_WOOD.id, 3)

mc.setBlocks(p.x+1, p.y+5, p.z+2, #Main Structure Roof End (Right Side, 1st Row)
             p.x+1, p.y+5, p.z+5,
             block.STONE_BRICK)

mc.setBlocks(p.x+1, p.y+6, p.z+3, #Main Structure Roof End (Right Side, 2nd Row)
             p.x+1, p.y+6, p.z+4,
             block.STONE_BRICK)

mc.setBlocks(p.x+19, p.y+5, p.z+2, #Main Structure Roof End (Left Side, 1st Row)
             p.x+19, p.y+5, p.z+5,
             block.STONE_BRICK)

mc.setBlocks(p.x+19, p.y+6, p.z+3, #Main Structure Roof End (Left Side, 2nd Row)
             p.x+19, p.y+6, p.z+4,
             block.STONE_BRICK)

You can put the different parts of the building in a list and then iterate through this list, using the values in the list.

I would propose this structure:

parts = [
    # x0  y0  z0    x1  y1  z1    block id          meta data
    (( 1,  0,  1), (19,  4, 19), (block.STONE_BRICK, 0)), # Main structure
    (( 2,  1,  2), (18,  3,  5), (block.AIR        , 0)), # Hollowing out main structure interior
    # ... the other parts
]

You can then use this loop to build the parts:

for (x0, y0, z0), (x1, y1, z1), (block, meta) in parts:
    mc.setBlocks(p.x + x0, p.y + y0, p.z + z0,
                 p.x + x1, p.y + y1, p.z + z1,
                 block, meta)

I would also advice you to follow a few style suggestions from PEP8, in this case I specifically mean that you should add empty lines after the imports:

import mcpi.minecraft as minecraft
import mcpi.block as block

mc = minecraft.Minecraft.create()
p = mc.player.getTilePos()

and that you should always add spaces around operators and after #:

mc.setBlocks(p.x+1, p.y, p.z+1 #...

to:

mc.setBlocks(p.x + 1, p.y, p.z + 1 # ...

This simply eases the reading process.


CodeChef- Chef and Strange Formula For positive integer x let define function F(x) = 1 * (1! + x) + 2 * (2! + x) + .. + x * (x! + x).
"k!" means factorial: k! = 1 * 2 * .. * k
Chef wants to calculate F(p1) + F(p2) + ... + F(pn).
As answer could be large, help him, calculate value modulo m.

Input

First line contains two integers n and m.
Next line contains n space separated integers pi.

Output

Output a single line containing one integer --- calculated value modulo m.

Constraints
1 ≤ n ≤ 10^5
1 ≤ pi ≤ 10^18
1 ≤ m ≤ 10^7

Subtasks
Subtask #1: 1 ≤ pi ≤ 6 (10 points)
Subtask #2: 1 ≤ pi ≤ 7 * 10^3 (25 points)
Subtask #3: m - prime number (25 points)
Subtask #4: original constraints (40 points)

Here is my code
#include <stdio.h>
#define M 10000000

long long int factorial(long long int n)
{
    long long int i,fact=1;
    for(i=1;i<=n;i++)
        fact=((fact%M)*(i%M))%M;
    return fact;
}
long long int func(long long int n)
{
    long long int a=((n%M)*(n%M)*((n+1)%M))%M/2;
    long long int sum=(a+factorial(n+1)-1)%M;
    return sum;
}

int main()
{
    long long int n,m;
    scanf("%lld%lld",&n,&m);
    long long int i,p[n];
    for(i=0;i<n;i++)
    {
        scanf("%lld",&p[i]);
    }
    long long int sum=0;
    for(i=0;i<n;i++)
    {
        sum+=func(p[i]);
    }
    printf("%lld\n",sum%m);
    return 0;
}

My code exceeds time limit on subtasks 2,3 and 4. How do I improve efficiency?

As youself noted F(x) can be reduced to:

x*x*(x+1)/2 + (x+1)! - 1

As @Daniel Sokolov pointed, you don't need to calculate full factorial for each p[i]. If p is a sorted array, you can use previously calculated factorial in the current (x+1)! calculation.

Furthermore it would be better to get modulo after each arithmetic operation. This ensure that the current result after each operation will always < m*m < 10^14.

So the code might look like this:

/**
 *  @param m modulo
 *  @param n number of integers
 *  @param p sorted array of integers
 *  @return sum of F(pn) series modulo m
*/
function sum(unsigned long m, unsigned n, unsigned long long int *p) {
    unsigned long long int
        x, // current element of p array
        x0 = 1, // previous element of p array
        a, // sum of arithmetic progression
        fac = 1, // factorial of (x + 1)
        ret = 0; // result
    for (unsigned i = 0; i < n; i++) {
        x = p[i];
        if (x < x0) return 0;
        a = x % m * x % m * ((x + 1) / 2 % m) % m;
        for (var k = x0 + 1; k <= x + 1; k++)
            fac = fac * (k % m) % m;
        ret = ((ret + a) % m + fac - 1) % m;
        x0 = x;
    }
    return ret;
}

Note that it assumes p is a sorted array.


Simplify MySQL INSERT query Is there a way to simplify the code below?  I want to simplify it, but I don't know if it is possible.

In the table below, I insert data which will end up in a MySQL table.

HTML

        Dată / Interval orar
        Denumire
        Moderatori
        Detalii

        <input name="DataInterval1" type="text" id="DataInterval1">
        <input name="Denumire1" type="text" id="Denumire1">
        <input name="Moderatori1" type="text" id="Moderatori1">
        <input name="Detalii1" type="text" id="Detalii1">

        <input name="DataInterval2" type="text" id="DataInterval2">
        <input name="Denumire2" type="text" id="Denumire2">
        <input name="Moderatori2" type="text" id="Moderatori2">
        <input name="Detalii2" type="text" id="Detalii2">

    ...

        <input name="DataIntervaln" type="text" id="DataIntervaln">
        <input name="Denumiren" type="text" id="Denumiren">
        <input name="Moderatorin" type="text" id="Moderatorin">
        <input name="Detaliin" type="text" id="Detaliin">

Part of the PHP code that I want to simplify:

$DataInterval1=$_POST['DataInterval1'];
$Denumire1=$_POST['Denumire1'];
$Moderatori1=$_POST['Moderatori1'];
$Detalii1=$_POST['Detalii1'];
$DataInterval2=$_POST['DataInterval2'];
$Denumire2=$_POST['Denumire2'];
$Moderatori2=$_POST['Moderatori2'];
$Detalii2=$_POST['Detalii2'];
...
$DataIntervaln=$_POST['DataIntervaln'];
$Denumiren=$_POST['Denumiren'];
$Moderatorin=$_POST['Moderatorin'];
$Detaliin=$_POST['Detaliin'];

// Insert data into mysql
$sql="INSERT INTO tabel (DataInterval, Denumire, Moderatori, Detalii)VALUES
('".$DataInterval1."', '".$Denumire1."', '".$Moderatori1."', '".$Detalii1."'),
('".$DataInterval2."', '".$Denumire2."', '".$Moderatori2."', '".$Detalii2."'),
...
('".$DataIntervaln."', '".$Denumiren."', '".$Moderatorin."', '".$Detaliin."')";
$result=mysql_query($sql);

Use indexing in HTML name attributes

        Dată / Interval orar
        Denumire
        Moderatori
        Detalii

        <input name="DataInterval[0]" type="text" id="DataInterval1">
        <input name="Denumire[0]" type="text" id="Denumire1">
        <input name="Moderatori[0]" type="text" id="Moderatori1">
        <input name="Detalii[0]" type="text" id="Detalii1">

    ...

        <input name="DataInterval[n]" type="text" id="DataIntervaln">
        <input name="Denumire[n]" type="text" id="Denumiren">
        <input name="Moderatori[n]" type="text" id="Moderatorin">
        <input name="Detalii[n]" type="text" id="Detaliin">

Use the indexing on the server side

<?php
$data = array();
foreach ($_POST["DataInterval"] as $index => $value) {
    $data[] = array(
        "DataInterval" => $_POST["DataInterval"][$index],
        "Denumire" => $_POST["Denumire"][$index],
        "Moderatori" => $_POST["Moderatori"][$index],
        "Detalii" => $_POST["Detalii"][$index],
    );
}

After this you only have to iterate throught the $data array and execute paramtrized INSERT SQL commands.

do not use old mysql lib
learn about PDO and prepared statements


Easy to read palindrome checker I made a palindrome checker that's supposed to be designed to be simple and easy to read. Please let me know what you think. I believe the time complexity is \$\mathcal{O}(n)\$ but I'm not too sure about that:

Challenge: You'll need to remove all non-alphanumeric characters (punctuation, spaces and symbols) and turn everything into the same case (lower or upper case) in order to check for palindromes.

function reverseString(str)
{
  str = str.replace(/[^\w\s]|_/g, "").toLowerCase().split(" ").join("");
  var array = [];
  for(var i = str.length ; i >=0; i--)
  {
    array.push(str[i])
  }
  return(array.join(""));
}

reverseString("My age is 0, 0 si ega ym.");

function palindrome(str) {
  str = str.replace(/[^\w\s]|_/g, "").toLowerCase().split(" ").join("");
  if(str === reverseString(str))
  {
   return true;
  }
  else
  {
    return false;
  }
}

Time complexity

Your time complexity is linear but you can save a few traversals over the string and lower the constant factor as you improve readability. Checking whether a string is a palindrome can be done in one pass with two pointers at each end of the string (plus some conditionals for your special characters), but this gains speed at the expense of readability; I'd encourage a round of clean-up before going for optimizations.

Repeated code

Repeated code harms maintainability and readability. Notice that the line

str.replace(/[^\w\s]|_/g, "").toLowerCase().split(" ").join("");

appears in two places in the code. If you decide to change one to accept a different regex but forget to change the other one, you've introduced a potentially subtle bug into your program. Move this to its own function to avoid duplication.

Use accurate function names and use builtins

reverseString is a confusing function: it does more than reversing a string as advertised: it also strips whitespace and punctuation, which would be very surprising if I called this function as a user of your library without knowing its internals. All functions should operate as black boxes that perform the task they claim to, nothing more or less.

The array prototype already has a reverse() function, so there's no need to write this out by hand.

Avoid unnecessary verbosity

The code:

  if(str === reverseString(str))
  {
   return true;
  }
  else
  {
    return false;
  }

is clearer written as return str === reverseString(str);, which says "return the logical result of comparing str and its reversal".

Improve the regex to match your specification

Including spaces in your regex substitution to "" is easier than .split(" ").join(""). If you wish to remove all non-alphanumeric characters, a regex like /[^a-z\d]/gi reflects your written specification accurately (or use \W if you don't mind including underscores).

Style remarks

JS uses K&R braces instead of Allman by convention.
Add a blank line above for and if blocks to ease vertical congestion.
Add a space around keywords and operators like for( and >=0, which are clearer as for ( and >= 0.
No need for parentheses around a return value.
array.push(str[i]) is missing a semicolon.
CodeReview's snippet autoformatter will automatically do most of this for you.

Rewrite 1

const palindrome = str => {
  str = str.replace(/[^a-z\d]/gi, "").toLowerCase();
  return str === str.split("").reverse().join("");
};

console.log(palindrome("My age is 0, 0 si ega ym."));

Rewrite 2: uglier, but faster

Benchmark

const palindrome = str => {
  str = str.replace(/[^a-z\d]/gi, "").toLowerCase();
  let left = 0;
  let right = str.length;

  while (left < right) {
    if (str[left++] !== str[--right]) {
      return false;
    }
  }

  return true;
};

[
  "",
  "a",
  "aba",
  "racecar",
  "racecar ",
  " racecar",
  " race car",
  " r  r a c e   c a  rr     ",
  ".a .. r . ... . .{9f08e988-1e35-4dc6-a24a-5c7e03bce5ba}$ $!ace ca r3  a",
].forEach(test => console.log(palindrome(test)));

console.log();
[
  "ab",
  "abc",
  "racecars",
  "racescar",
  " ra scecar",
  " r   sace car",
  "a r  r a c e   c a  rr     ",
  " r  r a c e   c a  rr     a",
  ".a .. r . ... . .$$$ $!aces ca r  a",
].forEach(test => console.log(palindrome(test)));


Setting a cookie based on a selector I've got a conditional that sets a cookie based on whether a selector exists. There are other places in the code where I do something similar if the selector exists. If the selector exists I either add "nfl" or "nfl-" (notice the dash) to various attributes on the page. I'm trying to remove as much code as I can as there are about five other places I have similar code where I'm adding other attributes. I also feel like there's a better way to approach this without having to write this a ton of times:

if($(".userLocation").length > 0){

I'm banging my head on how to condense this but can't figure this out. Please include code samples; I learn best that way.

if($(".userLocation").length > 0){
    var user_cookie = {
    name: 'nfl-user-profile',
    options: {
        path: '/',
        expires: 365
    }
    };
} else {
    var user_cookie = {
    name: 'user-profile',
    options: {
        path: '/',
        expires: 365
    }
    };
}

if($(".userLocation").length > 0){
$('header').attr("href","#nfl-city");
$('header').attr("href","#nfl-state");
} else {
$('header').attr("href","#city");
$('header').attr("href","#state");
}

if($(".userLocation").length > 0){
$('footer').attr("title","#nflcity");
$('footer').attr("title","#nflstate");
} else {
$('footer').attr("title","#city");
$('footer').attr("title","#state");
}

Only thing I can think of is that wrap the condition:

var hasUserLocation = function (hasFunc, noFunc) {
    if ($(".userLocation").size() > 0) {
        hasFunc();
    } else {
        noFunc();
    }
}

then you can call like this:

hasUserLocation(function () {
    alert("has");
}, function () {
    alert("no")
})

jsfiddle


Monty Hall Optimization in Python 3.5 This is for homework but the deadline is over and I know the implementation is correct because it concurs with the math. At this point I just want to check my implementation for potential optimizations in terms of speed, memory usage, clarity, modularity, and anything else you guys might think of. (I know there is generally a trade-off between computation speed and memory usage, so if you have comments on that I appreciate any insight you might have)

The specs of the function are that it should be flexible as to how many doors and trials we want, and whether or not we're switching doors. We also assume that the guest always chooses the first door. This is the only non-variable component we're allowed.

For example, would using np.array make the code faster than using lists? I didn't think it'd make a difference because they're very small, but that could be wrong.

In general, what could I do better?

def monty_hall (num_doors, trials, switch):

    #count number of successful trials
    successes = 0

    for i in range(trials):
        #determine behind which door the prize is
        doors = [i for i in range(1, num_doors+1)]
        prize_door = np.random.choice(doors)

        #determine which door the host opens if guest switches
        host_doors = [i for i in range(2, num_doors+1)]
        if prize_door != 1:
            host_doors.remove(prize_door)
        host_choice = np.random.choice(host_doors)

        #determine which door the guest chooses if switching
        doors.remove(1); doors.remove(host_choice)
        guest_switch = np.random.choice(doors)

        #we assume the guest is always choosing the first door
        if prize_door == 1 and switch == 0:
            successes += 1

        elif prize_door != 1 and switch:
            if guest_switch == prize_door:
                successes += 1

    probability = successes / trials
    return probability

If you were using python 2.x, there would be no need to do doors = [i for i in range(1, num_doors+1)], because range just returns a list. So just do:

doors = range(1, num_doors+1)

(similar for host_doors).

Since you are using python 3.x, range will return a range object, but you can just call list on it:

doors = list(range(1, num_doors+1))

Minor point: there is no need for the intermediate variable probability, just return successes / trials.

Python has an official style-guide, PEP8, which recommends not having unnecessary blank lines (such as between your if and else block).

Also, you should add one space in front between # and the actual comment.

It is also customary to use _ as a loop variable that is unused.

You can make your code slightly more efficient by only drawing the user's switched choice when actually switching:

def monty_hall (num_doors, trials, switch):
    successes = 0
    for _ in range(trials):
        # determine behind which door the prize is
        doors = list(range(1, num_doors+1))
        prize_door = np.random.choice(doors)

        # door the guest chooses. (Fixed to 1)
        guest_choice = 1
        doors.remove(guest_choice)

        # determine which door the host opens before guest is allowed to switch
        # this will be faster for larger lists than copying the `doors` list
        host_choice = prize_door
        while host_choice == prize_door:
            host_choice = np.random.choice(doors)
        doors.remove(host_choice)

        # determine which door the guest chooses if switching
        if switch:
            guest_choice = np.random.choice(doors)

        # check win condition
        if guest_choice == prize_door:
            successes += 1

    return successes / trials

It might actually be faster (for larger number of doors it will be), not to copy the doors for the host choice, but just take a random door from doors and take another one if it is the prize_door:

host_choice = prize_door
while host_choice == prize_door:
    host_choice = np.random.choice(doors)
doors.remove(host_choice)

For small lists this will be better:

host_doors = doors[:]
host_doors.remove(prize_door)
host_choice = np.random.choice(host_doors)
doors.remove(host_choice)

Overall the most costly part of this code is probably the removes, which can be quite costly for large lists if removing near the beginning (like for guest_choice). It would be better to store the doors as a set (for which deleting is O(1)). But that makes choosing one door harder (you have to do np.random.choice(tuple(doors)) everytime you draw). You should time both approaches and see what works best for your data.


Scraping data unveiling a button from craigslist I've written some code to parse the names and phone numbers from craigslist. It starts from the link in "m_url" then goes one layer deep to parse the name and then again another layer deep to parse the phone number. Note that it goes 2 layer deep only when it sees "show contact button" on that page so that it can unveil the phone number from that link to scrape. It only prints the result when it sees the button on that page. That's because there are around 120 names on that page but it prints only those containing that specific button. Sometimes when I come across such "show contact button" link within a page from where I am supposed to harvest data, I get frightened. That's why I tried to work on it. It works smoothly now. Any improvement on this script will be very helpful.

import re
import requests
from lxml import html

m_url = "http://bangalore.craigslist.co.in/search/reb?s=120"
base = "http://bangalore.craigslist.co.in"

def get_link(url):
    page1 = requests.get(url).text
    tree = html.fromstring(page1)
    for row in tree.xpath('//li[@class="result-row"]'):
        links = base + row.xpath(".//a[contains(concat(' ', @class, ' '), ' hdrlnk ')]/@href")[0]
        process_doc(links)

def process_doc(medium_link):

    page2 = requests.get(medium_link).text
    tree = html.fromstring(page2)
    try:
        name = tree.xpath('//span[@id="titletextonly"]/text()')[0]
    except IndexError:
        name = ""
    try:
        link = base + tree.xpath('//section[@id="postingbody"]//a[@class="showcontact"]/@href')[0]
    except IndexError:
        link = ""

    parse_doc(name, link)

def parse_doc(title, target_link):

    if target_link:
        page = requests.get(target_link).text
        tel = re.findall(r'\d{10}', page)[0] if re.findall(r'\d{10}', page) else ""
        print(title, tel)

get_link(m_url)

Pics of that button:

There are some things I would do differently:

create a class - this way you may share a web-scraping session between the methods and also share the base and start urls
use more meaningful variable and method names - for instance, parse_doc() could be get_contact_info(); you can re-use page variable names in different methods etc.
you can use findtext() method to get the text
I would also return from the scraper and print out the results outside of it
you are searching for the phone number twice - instead, use the .search() method and check if you've got a match object or none
you can pre-compile and re-use the regular expression pattern for the phone number

Improved code:

import re

import requests
from lxml import html

class CraigListScraper:
    PHONE_NUMBER_PATTERN = re.compile(r'\d{10}')

    def __init__(self, start_url, base_url):
        self.session = requests.Session()
        self.base_url = base_url
        self.start_url = start_url

    def scrape(self):
        page = self.session.get(self.start_url).text

        tree = html.fromstring(page)
        for row in tree.xpath('.//li[@class="result-row"]'):
            link = self.base_url + row.xpath(".//a[contains(concat(' ', @class, ' '), ' hdrlnk ')]/@href")[0]

            yield self.process_search_result(link)

    def process_search_result(self, medium_link):
        page = self.session.get(medium_link).text
        tree = html.fromstring(page)

        name = tree.findtext('.//span[@id="titletextonly"]')

        try:
            contact_info_link = self.base_url + tree.xpath('//section[@id="postingbody"]//a[@class="showcontact"]/@href')[0]
            phone_number = self.get_contact_info(contact_info_link)
        except IndexError:
            phone_number = ""

        return name, phone_number

    def get_contact_info(self, target_link):
        page = self.session.get(target_link).text

        match = self.PHONE_NUMBER_PATTERN.search(page)
        return match.group(0) if match else ""

if __name__ == '__main__':
    start_url = "http://bangalore.craigslist.co.in/search/reb?s=120"
    base_url = "http://bangalore.craigslist.co.in"

    scraper = CraigListScraper(start_url, base_url)
    for result in scraper.scrape():
        print(result)


Filter options of select box using input box value I have a select box with some options in it and a input box. I have to filter the values of select box on the basis of the keyword entered in the text box.

JavaScript

function filter() {
    var keyword = document.getElementById("search").value;
    var select = document.getElementById("select");
    for (var i = 0; i < select.length; i++) {
        var txt = select.options[i].text;
        if (txt.substring(0, keyword.length).toLowerCase() !== keyword.toLowerCase() && keyword.trim() !== "") {
            select.options[i].style.display = 'none';
        } else {
            select.options[i].style.display = 'list-item';
        }
    }
}

HTML

        <input type="text" id="search" name="search" style="margin: 10px;width: 165px;" onkeyup="filter()">

        <select id="select" size="10" style="margin: 10px; width: 165px;height: 170px;">
            <option>Cupcake</option>
            <option>Cunut</option>
            <option>Eclair</option>
            <option>Froyo</option>
            <option>Gingerbread</option>
            <option>Honeycomb</option>
            <option>Ice Cream Sandwich</option>
            <option>Jelly Bean</option>
            <option>KitKat</option>
            <option>Lollipop</option>
            <option>Marshmallow</option>
            <option>Nougat</option>
        </select>

Is this the right way for this task?

Here is the working code for the scenario.

I would start by moving the search term into the function definition, to save having to look up the DOM element. So I would change in the HTML

 <input type="text" id="search" name="search" style="margin: 10px;width: 165px;" onkeyup="filter(this.value)">

And then have function be defined with keyword as a parameter.

function filter(keyword) {
  ...
}

The other main change I would make is to use string.prototype.startsWith rather than substring checking, this is more explicit about what is trying to be done, and saves having logic to handle the empty string.

Also, rather than an If statement and then setting style.display, the display is set with either 'list-item' or 'none' with the ternary operator.

This leaves the function as

function filter(keyword) {
    var select = document.getElementById("select");
    for (var i = 0; i < select.length; i++) {
        var txt = select.options[i].text;
        var include = txt.toLowerCase().startsWith(keyword.toLowerCase());
        select.options[i].style.display = include ? 'list-item':'none';
    }
}

A further improvement you might want to look at may include moving the select element selection outside of this function to improve re-use.


Batch retrieve formatted address along with geometry (lat/long) and output to csv I have a csv file with 3 fields, two of which are of my interest, Merchant_Name and City.
My goal was to output multiple csv files each with 6 fields, Merchant_Name, City, name, formatted_address, latitude, longitude.

For example, if one entry of the csv is Starbucks, Chicago, I want the output csv to contain all the information in the 6 fields (as mentioned above) like so-
Starbucks, Chicago, Starbucks, "200 S Michigan Ave, Chicago, IL 60604, USA", 41.8164613, -87.8127855,
 Starbucks, Chicago, Starbucks, "8 N Michigan Ave, Chicago, IL 60602, USA", 41.8164613, -87.8127855
and so on for the rest of the results.

For this, I used Text Search request of Google Maps Places API. Here is what I wrote.

import pandas as pd
# import googlemaps
import requests
# import csv
# import pprint as pp
from time import sleep
import random

def search_output(search):
    if len(data['results']) == 0:
        print('No results found for {}.'.format(search))

    else:

        # Create csv file
        filename = search + '.csv'
        f = open(filename, "w")

        size_of_json = len(data['results'])

        # Get next page token
        # if size_of_json = 20:
            # next_page = data['next_page_token']

        for i in range(size_of_json):
            name = data['results'][i]['name']
            address = data['results'][i]['formatted_address']
            latitude = data['results'][i]['geometry']['location']['lat']
            longitude = data['results'][i]['geometry']['location']['lng']

            f.write(name.replace(',', '') + ',' + address.replace(',', '') + ',' + str(latitude) + ',' + str(longitude) + '\n')

        f.close()

        print('File successfully saved for "{}".'.format(search))

        sleep(random.randint(120, 150))

API_KEY = 'your_key_here'

PLACES_URL = 'https://maps.googleapis.com/maps/api/place/textsearch/json?'

# Make dataframe
df = pd.read_csv('merchant.csv', usecols=[0, 1])

# Construct search query
search_query = df['Merchant_Name'].astype(str) + ' ' + df['City']
search_query = search_query.str.replace(' ', '+')

random.seed()

for search in search_query:
    search_req = 'query={}&key={}'.format(search, API_KEY)
    request = PLACES_URL + search_req

    # Place request and store data in 'data'
    result = requests.get(request)
    data = result.json()

    status = data['status']

    if status == 'OK':
        search_output(search)
    elif status == 'ZERO_RESULTS':
        print('Zero results for "{}". Moving on..'.format(search))
        sleep(random.randint(120, 150))
    elif status == 'OVER_QUERY_LIMIT':
        print('Hit query limit! Try after a while. Could not complete "{}".'.format(search))
        break
    else:
        print(status)
        print('^ Status not okay, try again. Failed to complete "{}".'.format(search))
        break

I want to implement next page token but cannot think of a way which woudn't make it all a mess. Another thing I wish to improve is my csv writing block. And dealing with redundancy.
I further plan to concatenate all the csv files into one(but still keeping the original separate files).

Please note that I'm new to programming, in fact this is actually one of my first programs to achieve something. So please elaborate a bit more when need be. Thanks!

Use the csv library

x.writerows(custom_tuple(row) for row in data['results'])

Grabbing mutable data from the global scope is, I'm pretty sure, poor form. You should pass data as an argument to search_output.

Just pass in data['results'] as results, not the whole data object.
Consider calling it output_search_results.
Consider using precise type hints.
The existing argument search is being used as a file-name, so call it filename.

Pass the csv-writer object in as an argument to output_search_results.

Maybe this means introducing a middle-layer function to create the csv-writer; that's fine.
This is going to let you call output_search_results multiple times after opening the file once.

Encapsulate the code that fetches data from google in its own function.

Maybe even in its own class, if you're going to try to make more intelligent handlers for the failure cases.
Now you can easily build the functionality to fetch a "next page" of results based on the next-page-token. It'll be a sibling to the above function, or maybe an option to it.

From here you can either pass data['next_page_token'] as another (optional) parameter to output_search_results, which will call itself recursively, or you can call it in a while loop as you grab each next page.
You'll probably need to add some safety measures, such as what to do if a file already exists, etc.


Displaying a letter grade based on a percentage Here is a class I made that is able to ask a teacher what percentage their student has in a class and gives them back a grade. The code is very repetitive and I am wondering what I can do to simplify the class below before inputting any more grades.

import java.util.Scanner;

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

        int x = 0;

        System.out.print("What is the percentage of the student: ");
        x = grades.nextInt();

        if (x > 100 || x < 0){
                System.out.println("Please put in a percent from 0 - 100!\n");
        }

        while (x > 100 || x < 0 ){

            System.out.print("What is the percentage of the student: ");
            x = grades.nextInt();
            if (x > 100 || x < 0){
                System.out.println("Please put in a percent from 0 - 100!\n");
            }
        }
        if (x <= 100 && x >= 96){
             System.out.print("A+");
        }
         if (x <= 95 && x >= 92){
             System.out.print("A");
        }
         if (x <= 91 && x >= 90){
             System.out.print("A-");
        }
         if (x <= 89 && x >= 50){
             System.out.print("B+");
        }
         if (x <= 49 && x >= 0){
             System.out.print("B");
        }
        // so on and so forth down the grading scale
    }
}

It would be good to split the different sub-tasks to different functions,
for example:

int promptScore(): a method to prompt for score
String getGrade(int score): a method to return the grade from the score

The promptScore method could use an infinite loop with two steps:

Ask for a number
If the number is within the range, return it

The getGrade method could use a sorted map that stores the limits and the corresponding grades.
You could iterate over the entries in this map,
and when you find a value that is below the limit, then you return the corresponding grade.

Something like this:

class Grades {

    private static final Map<Integer, String> limits;

    static {
        limits = new TreeMap<>();
        limits.put(50, "B");
        limits.put(90, "B+");
        limits.put(92, "A-");
        limits.put(96, "A");
        limits.put(101, "A+");
    }

    public static int promptScore() {
        Scanner scanner = new Scanner(System.in);

        while (true) {
            System.out.print("What is the percentage of the student: ");
            int x = scanner.nextInt();

            if (0 <= x && x <= 100) {
                return x;
            }
            System.out.println("Please put in a percent from 0 - 100!\n");
        }
    }

    public static String getGrade(int score) {
        for (Map.Entry<Integer, String> entry : limits.entrySet()) {
            int limit = entry.getKey();
            if (score < limit) {
                return entry.getValue();
            }
        }
        throw new AssertionError("Impossible case");
    }

    public static void main(String[] args) {
        System.out.println(getGrade(promptScore()));
    }
}

The benefit of splitting up the functions like this is that writing unit tests becomes easy:

@Test
public void testB() {
    assertEquals("B", Grades.getGrade(0));
    assertEquals("B", Grades.getGrade(49));
}

@Test
public void testB_Plus() {
    assertEquals("B+", Grades.getGrade(50));
    assertEquals("B+", Grades.getGrade(89));
}

@Test
public void testA_Minus() {
    assertEquals("A-", Grades.getGrade(90));
    assertEquals("A-", Grades.getGrade(91));
}

@Test
public void testA() {
    assertEquals("A", Grades.getGrade(92));
    assertEquals("A", Grades.getGrade(95));
}

@Test
public void testA_Plus() {
    assertEquals("A+", Grades.getGrade(96));
    assertEquals("A+", Grades.getGrade(100));
}


Pascal's triangle using C++ vectors This is my C++ program. I wanted to print out Pascal's triangle. I let the user decide how many rows to print out. The first row is considered as 0, and is just:

  1

I'm using vectors for this task. I think that I do not have to store the full triangle as that would take up too much extra space. I just store the previous row and the next one. I use the SetRow() function to determine the numbers in the next row based on the numbers in the previous row. Then each row is printed out. Please give me some feedback on my code. Is it an effective way to approach this problem? Does my code have any "bad programming practices"? What would you have done differently?

#include <iostream>
#include <vector>
using std::vector;
using std::cout;
using std::cin;
using std::endl;

void SetRow(vector<int> &old_row, vector<int> &new_row, int row);

int main() {
  int n_num;
  cout << "What row of Pascal's triangle do you want to go up to?" << endl;
  cin >> n_num;
  cout << endl;

  vector <int> row_1 = {1};
  vector <int> row_2;
  vector <int> curr_row;

  cout << row_1.at(0) << endl;

  // loop through all the rows
  for (int i = 1; i < (n_num + 1); i++) {
    if (i % 2 == 1) {
      SetRow(row_1, row_2, i);
      curr_row = row_2;
    } else if (i % 2 == 0) {
      SetRow(row_2, row_1, i);
      curr_row = row_1;
    }
    for (auto k: curr_row) {
      cout << k << ' ';
    }
    cout << endl;
  }

  // This ends the program
  return 0;
}

void SetRow(vector<int> &old_row, vector<int> &new_row, int row) {
  new_row.clear();
  new_row.push_back(1);
  for (int i = 0; i < (row - 1); i++) {
    new_row.push_back(old_row.at(i) + old_row.at(i + 1));
  }
  new_row.push_back(1);
}

The code seems all right; you might add the uniform variable initialization: int n_num {};. Furthermore, the way you read the number of rows is unstable, when you use std::cin to do it. Imagine you enter "b" instead of a number. Your program has no loop, that prevents that. You could use a function that only reads integers:

int readInt(std::istream& stream) {
    /*
     * Function to get integers, that are entered by the user
     */
    int input;
    stream >> input;
    return input;
}

Then int num_n {readInt(std::cin)}; would be a possible way to do it.

Second off: why do you have the line else if (i % 2 == 0) ? I suppose you could just use else instead. If i % 2 == 1 returns false, it has to be 0. So there is no need to check again. It's like checking if your number is odd, and if it's not checking again if it's even.


A Lisp version of the Schwartzian transform Randal Schwartz stole Lisp's decorate-sort-undecorate idiom when he came up with what we'd later call the Schwartzian transform. All of my Lisp knowledge disappeared through nine half lives, but I gave it a shot in SBCL. I'm aiming for an idiomatic example I can use in a presentation about the history of the Schwartzian transform.

This works (and I know I can generalize it further), but I wonder how Lisp-y it is:

(require :sb-posix)
(defun schwartzian-files-mtime ( glob-pattern )
    (map
        'list
        #'cdr
        (stable-sort
            (map
                'list (
                    lambda (x) (
                        cons
                            (sb-posix:stat-mtime (sb-posix:stat x))
                            x
                        )
                    )
                (directory glob-pattern)
            )
            #'<
            :key
            #'car
        )
    )
)

(schwartzian-files-mtime "/etc/*")

What is not enough LISP-y in your function is the style used for the parentheses (they should not be left alone as last element of a line and they should be on the last line of the enclosed form). Another minor point is that you can use the shorter mapcar instead of map 'list.

Here is a version of your function (generalized for any function applied to the data and any predicate) which follows the more conventional style used in Common Lisp.

(defun schwartzian-transform (list costly-function predicate)
"sort a list of objects over the value of a function applied to them,
by applying the Schwartzian Transform (https://en.wikipedia.org/wiki/Schwartzian_transform)
the parameters are the list, the function, and the predicate for the sort."
  (mapcar #'cdr
          (stable-sort (mapcar (lambda (x)
                                 (cons (funcall costly-function x) x))
                               list)
                       predicate
                       :key #'car)))

(require :sb-posix)
(schwartzian-transform
 (directory "/etc/*")
 (lambda (x) (sb-posix:stat-mtime (sb-posix:stat x)))
 #'<=)


Keeping UI responsive while performing long running task I recently asked for some advice on the best way to structure my code for a program I was writing. (see this question on Programmers.SE).  The solution I was given was quite elegant, however I am less happy with the code that I've written so that the UI remains responsive.

Here is my code, I will outline the reasons I am not happy with it below:

    private void buttonUpdateContacts_Click(object sender, EventArgs e)
    {
        BackgroundWorker worker = new BackgroundWorker();
        worker.WorkerReportsProgress = true;
        worker.DoWork += new DoWorkEventHandler(worker_DoWork);
        worker.ProgressChanged += new ProgressChangedEventHandler(worker_ProgressChanged);
        worker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(worker_RunWorkerCompleted);
        worker.RunWorkerAsync();
    }

    void worker_DoWork(object sender, DoWorkEventArgs e)
    {
        (sender as BackgroundWorker).ReportProgress(0, "Parsing word document...");

        WordDocumentParser wordDocumentParser;
        try
        {
            wordDocumentParser = new WordDocumentParser(Settings.Default.OohSheetLocation);
        }
        catch (FileNotFoundException exception)
        {
            e.Result = new FileNotFoundException("Could not find the word document. File path is: " + exception.FileName, exception);
            return;
        }

        List<TppContact> oohSheetContacts = wordDocumentParser.GetContacts();

        try
        {
            _contactManager = GoogleContactsManager.CreateContactsManager();
        }
        catch (InvalidCredentialsException argumentException)
        {
            e.Result = argumentException;
            return;
        }
        catch (ArgumentNullException nullArg)
        {
            e.Result = nullArg;
            return;
        }

        (sender as BackgroundWorker).ReportProgress(0, "Comparing Google contacts to the word document...");
        var comparer = new ContactComparer(oohSheetContacts, _contactManager);
        _actions = comparer.Compare();
    }

    void worker_ProgressChanged(object sender, ProgressChangedEventArgs e)
    {
        labelCurrentStatus.Text = e.UserState.ToString();
    }

    void worker_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
    {
        if (e.Result != null)
        {
            MessageBox.Show((e.Result as Exception).Message);
        }
        else
        {
            if (_actions.Count == 0)
            {
                textBoxChanges.Text = "There are no changes to be made.";
                buttonApplyChanges.Enabled = false;
            }
            else
            {
                textBoxChanges.Text = string.Empty;
                _actions.ForEach(action => textBoxChanges.Text += action.ChangeDescription + Environment.NewLine);
                buttonApplyChanges.Enabled = true;
            }
        }
    }

    private void button2_Click(object sender, EventArgs e)
    {
        labelCurrentStatus.Text = "Updating Google...";
        _contactManager.UpdateContacts(_actions);

        labelCurrentStatus.Text = "Done.";
    }

I don't like how tied up my code is to the background worker.  For example, to handle the exceptions that the various methods can throw, I have to set the result of the background worker to be the exception then handle it in the worker_RunWorkerCompleted method.  This also means that I lose the ability to give the user proper error messages such as "invalid username and password supplied".  Maybe the answer is to set that as the result rather than the exception?
It goes without saying that I probably shouldn't have all this logic being called from the button clicked method but I am unsure of a better way to do it.  This is written in WinForms at the minute but there's very little reason not to write it in WPF.
Am I using the best structure here?  I tried to use Tasks but didn't get very far and I've used background workers in the past so I know how they work.

Can you offer any pointers how to make this code "better" or am I doing it the best I can?

1) I would use the ProgressChanged event with a custom object as my e.UserState which is updated within the code.

// sample worker state class definition
internal class WorkerState
{
    public Exception ExceptionThrown { get; set; }
    public string ProgressMessage { get; set; }
    public int MaximumProgressValue { get; set; }
    public int ProgressValue { get; set; }
    public int Actions { get; set; }
}

Sample Usage

// this would likely be initialized / reinitiatlized before the RunWorkerAsync is called
private WorkerState m_WorkerState;

private void buttonUpdateContacts_Click(object sender, EventArgs e)
{
    // for question 2) this could be convered into some form of
    // factory where providing delegates for the ProgressChange and WorkCompleted
    BackgroundWorker worker = new BackgroundWorker();

    worker.UserState = new WorkerState();

    worker.WorkerReportsProgress = true;
    worker.DoWork += new DoWorkEventHandler(worker_DoWork);
    worker.ProgressChanged += new ProgressChangedEventHandler(worker_ProgressChanged);
    worker.RunWorkerCompleted += new RunWorkerCompletedEventHandler(worker_RunWorkerCompleted);
    worker.RunWorkerAsync();
}

void worker_DoWork(object sender, DoWorkEventArgs e)
{
    Worker worker = (sender as BackgroundWorker);
    WorkerState state = worker.UserState;

    state.UserState.ProgressMessage = "Parsing word document...";
    state.UserState.MaximumProgressValue = 100;
    state.UserState.ProgressValue = 0;

    worker.ReportProgress(0, state);

    // ...

    try
    {
        // do something
    }
    catch (InvalidCredentialsException argumentException)
    {
        e.ExceptionThrown = argumentException;
        return;
    }
    catch (ArgumentNullException nullArg)
    {
        e.ExceptionThrown = nullArg;
        return;
    }

    // ..
}

2) There is not too much wrong with putting the code in the button click event but you could make better use of OO concepts like factories for creating the BackgroundWorker component etc. I personally would split the bulk of the code in the DoWork method into seperate classes following SOLID principles (especially the Single Responsibility... this class is responsible for blah, blah blah).

3) Tasks are likely to be unsuitable in this case as this is exactly what the BackgroundWorker component was designed for.


Determine a list contains two elements with a given sum On interview I was asked to write method with following contract:

boolean checkList(List<Long> list, long sum){...}

for example it has to return true for arguments:
 ({1,2,3,4,5,6}, 9)  because 4+5 = 9

and it have to return false for arguments:

({0,10,30}, 11)  because there are no combination of 2 elements with sum 11

I suggested code like this:

boolean checkList(List<Long> list, long expectedSum) {
    if (list.size() < 2) {
        return false;
    }
    for (int i = 0; i < list.size() - 1; i++) {
        for (int k = i; k < list.size(); k++) {
            if ((list.get(i) + list.get(k)) == expectedSum) {
                return true;
            }
        }
    }
    return false;
}

But interviewer asked me to improve the solution. How?

As 200_success mentions, your solution is \$O(n^2)\$, due to the double for loop.

You actually have an error in your inner loop.  You start the inner loop with for (int k = i; ..., but it should be for (int k = i + 1; ....  This means you could return true for an even expected sum when half of that value appears once in the list.

If you correct the above error, the test for list.size() < 2 is unnecessary.  The outer loop will run at most once (size=1) and the inner loop will not run at all, therefore the return false; at the end of the function will be executed.

An \$O(n)\$ solution is:

start with an empty set
for each value x in the list:

if set contains expectedSum - x, return true
add x to the set.

returnfalse

It is important to check for containment before adding the value, or ({1,2,3,4}, 8) would return true, despite not containing a pair of 4’s.

Set<Long> set = new HashSet<>(list.size()*2);
for(long x: list) {
    if (set.contains(expectedSum - x)) {
        return true;
    }
    set.add(x);
}
return false;

The list does not need to be sorted.

The capacity of the HashSet is initialized to double the size of the list, to avoid re-sizings, which would slow the algorithm down.

From the comments: "Why double?" The default load factor for a HashSet is 0.75. If the number of elements in the set exceeds load_factor * capacity, the capacity is increased and the contents rehashed for the new size. If the capacity was set to the size of the list, it would still end up expanding the capacity and rehashing after adding the first 75% of the entries in the list. You need an initial capacity at least 33% larger than the final size to avoid the expansion: list.size()*4/3 + 1 might be sufficient, but is perhaps cutting things a little too fine.  Double seemed a reasonable amount.


Spawning an alien to chase the player I have just made a simple script which spawns an alien that chases the player. I want to move as much of the script into functions so as to minimize the amount of code, to make it run better when my game gets a lot bigger. I am new to functions and classes, and I want to know what can be turned into a class in another script and how to go about doing it. I do not want just code, I want to be able to understand what I'm doing with it as well.

import pygame, sys, random, time, math
from pygame.locals import *
pygame.init()

bifl = 'screeing.jpg'
milf = 'character.png'
alien = 'alien_1.png'

screen = pygame.display.set_mode((640, 480))
background = pygame.image.load(bifl).convert()
mouse_c = pygame.image.load(milf).convert_alpha()
nPc = pygame.image.load(alien).convert_alpha()

x, y = 0, 0
movex, movey = 0, 0

z, w = random.randint(10, 480), random.randint(10, 640)
movez, movew = 0, 0

while True:
    for event in pygame.event.get():
        if event.type == QUIT:
            pygame.quit()
            sys.exit()

        if event.type == KEYDOWN:
            if event.key == K_w:
                movey = - 4
            elif event.key == K_s:
                movey = + 4
            elif event.key == K_a:
                movex = - 4
            elif event.key == K_d:
                movex = + 4

        if event.type == KEYUP:
             if event.key == K_w:
                movey = 0
             elif event.key == K_s:
                movey = 0
             elif event.key == K_a:
                movex = 0
             elif event.key == K_d:
                movex = 0

    if w < x:
        movew =+ 0.4
    if w > x:
        movew =- 0.4
    if z < y:
        movez =+ 0.4
    if z > y:
        movez =- 0.4

    x += movex
    y += movey
    w += movew
    z += movez
    print('charecter pos: ' + str(x) + str(y))
    print('alien pos: ' + str(w) + str(z))

    chpos = x + y
    alpos = w + z
    print(alpos, chpos)
    if chpos == alpos:
        pygame.quit()
        sys.exit()

    screen.blit(background, (0, 0))
    screen.blit(mouse_c, (x, y))
    screen.blit(nPc, (w, z))

    pygame.display.update()

Long multiple choice decisions in python work well as dictionaries, rather than elifs.

def handle_event(event):
    OnKeyDown = {K_w:(0, 4), K_s:(0,-4), K_a:(-4, 0), K_d:(4,0)}
    OnKeyUp = {K_w:(False, True), K_s:(False, True), K_a:(True, False), K_d:(True, False)}

    if event.type == KEYUP:
         delta = OnKeyDown[event]
         movex += delta[0]
         movey += delta[1]
    elif event.type == KEYDOWN:
         delta = OnKeyUp[event]
         if delta[0]: movex = 0
         if delta[1]: movey = 0

   x += movex
   y += movey


Exponential growth calculator for COVID-19 I've been listening to some programmers and they use terms like good and bad code. I am not really sure what they mean by that. I mean I went on r/badcode and saw some stuff I knew was definitely bad code. But I don't really understand if bad code is repeatedly typing the same code instead of looping or just bad layouts.
    import java.io.IOException;
    import java.util.Scanner;
    class Covid19ExpoGrowth {
    public static void main(String[] args) throws IOException {
    // I will use the formula y = a(1 + r)^x
    // a = initial value
    // r = growth rate
    // x = time interval
    int a;
    double r;
    int x;

    // Workout exponential growth
    System.out.println("Let us see the Exponential growth of Covid-19.");
    System.in.read();
    System.out.println("We will set the initial value of Covid as 88,000 as that was the amount of cases in february");
    a = 88000;
    System.in.read();

    System.out.println("We need to find the growth rate of Covid-19, we can use the growthRate method");
    System.in.read();
    r = growthRate(14000, 88000);
    r = Math.round(r * 10.0) / 10;
    System.out.println("Using the growthRate method, we worked out the growth rate is: " + r + " rounded to no d.p");
    System.in.read();

    System.out.println("The time interval will be from February 01 2020 - April 16 2020, 45 days");
    x = 45;
    System.in.read();

    System.out.println("Now we have all the values let us use the method calculateExpoGrowth");
    System.in.read();
    double roundedExpo;
    roundedExpo = Math.round(calculateExpoGrowth(a, r, x) * 100) /10;
    System.out.println("The Exponential growth of Covid 19 is: " + roundedExpo + " rounded to 2 d.p");

    System.out.println("You can now work out your own Exponential Growth");

    // User works out chosen exponential growth
    while (true) {
        System.out.println("Please enter the initial value");
        Scanner scan = new Scanner(System.in);
        a = scan.nextInt();

        Scanner scan_gr = new Scanner(System.in);System.out.println("Enter WORKOUT GROWTH RATE if you would like to workout the growth rate," +
                "if you already know it press enter");
        String gr = scan_gr.nextLine();
        if (gr.equals("WORKOUT GROWTH RATE")) {
            int pastValue;
            int presentValue;

            System.out.println("Please enter the past value");
            pastValue = scan.nextInt();

            System.out.println("Please enter the present value");
            presentValue = scan.nextInt();

            r = growthRate(pastValue, presentValue);
        } else {
            System.out.println("Please type in the growth rate");
            r = scan.nextDouble();
        }
        System.out.println("Now finally, type in the time interval");
        x = scan.nextInt();
        double roundedExpo2;

        roundedExpo2 = Math.round(calculateExpoGrowth(a, r, x) * 100) / 10;

        System.out.println("The program has calculated the Exponential growth is: " + roundedExpo2 + " rounded to 2 d.p");
    }
}
public static double growthRate(int pastVal, int presentVal){
    // To work out growth rate you:

    double newVal = presentVal - pastVal;
    // Subtract the past value from the present value

    return newVal / pastVal;
    // Divide the new value by the past value

}
public static double calculateExpoGrowth(int a, double r, int x){
    // Exponential growth formula
    return (a * Math.pow((1 + r), x));

}
}

Your formula was assuming 14,000 cases a day, rather than 14,000 cases a month.  I removed all rounding, choosing to round for display only.

Here's a partial result of the last test run I did.  I formatted the output manually to fit in the answer.

Let us see the Exponential growth of Covid-19.
We will set the initial value of Covid as
88,000 as that was the amount of cases in
February.

We need to find the growth rate of Covid-19,
we can use the growthRate method.
We'll assume a linear growth rate of 14,000
cases a month.

Using the growthRate method, we worked out
the growth rate is 0.005303.

The time interval will be from February 1,
2020 - April 16, 2020, 45 days.
Now we have all the values let us use the
method calculateExpoGrowth

The Exponential growth of Covid 19 is
111,647 cases by April 16, 2020.

I cleaned up the code a lot.  Here it is.

import java.io.IOException;
import java.text.DecimalFormat;
import java.util.Scanner;

public class Covid19ExpoGrowth {
    public static void main(String[] args) {
        try {
            calculateGrowth();
        } catch (IOException e) {
            e.printStackTrace();
        }

        System.out.println();
        System.out.println("You can now work out your "
                + "own Exponential Growth");

        // User works out chosen exponential growth
        Scanner scan = new Scanner(System.in);
        while (getUserInput(scan));
        scan.close();
    }

    private static void calculateGrowth() throws IOException {
        // I will use the formula y = a(1 + r)^x
        // a = initial value
        // r = growth rate
        // x = time interval
        int a;
        double r;
        int x;

        // Workout exponential growth
        System.out.println("Let us see the Exponential "
                + "growth of Covid-19.");
        System.out.println(
                "We will set the initial value of Covid "
                + "as 88,000 as that was the amount of "
                + "cases in February.");
        System.out.println();
        a = 88_000;
        System.out.println("We need to find the growth "
                + "rate of Covid-19, we can use the "
                + "growthRate method.");
        System.out.println("We'll assume a linear growth rate "
                + "of 14,000 cases a month.");
        System.out.println();
        r = growthRate(a, 14_000d + a) / 30d;
        String output = String.format("%.6f", r);
        System.out.println("Using the growthRate method, "
                        + "we worked out the growth "
                        + "rate is: " + output + ".");
        System.out.println();
        System.out.println("The time interval will be "
                + "from February 1, 2020 - April 16, 2020,"
                + " 45 days.");
        x = 45;
        System.out.println("Now we have all the values "
                + "let us use the method calculateExpoGrowth");
        System.out.println();
        double roundedExpo = calculateExpoGrowth(a, r, x);
        DecimalFormat formatter = new DecimalFormat(
                "###,###,###");
        output = formatter.format(roundedExpo);
        System.out.println("The Exponential growth of "
                + "Covid 19 is " + output
                + " cases by April 16, 2020.");
    }

    private static boolean getUserInput(Scanner scan) {
        int a;
        double r;
        int x;

        System.out.println("Please enter the initial value");
        String value = scan.nextLine().trim();
        if (value.isEmpty()) {
            return false;
        }

        a = Integer.valueOf(value);

        System.out.println("Enter WORKOUT GROWTH RATE if you "
                + "would like to workout the growth rate,"
                + "if you already know it press enter");
        String gr = scan.nextLine().trim();
        if (gr.equalsIgnoreCase("WORKOUT GROWTH RATE")) {
            int pastValue;
            int presentValue;

            System.out.println("Please enter the past value");
            pastValue = Integer.valueOf(scan.nextLine().trim());

            System.out.println("Please enter the present value");
            presentValue = Integer.valueOf(scan.nextLine().trim());

            System.out.println("Please enter the days");
            int days = Integer.valueOf(scan.nextLine().trim());

            r = growthRate(pastValue, presentValue) / days;
        } else {
            System.out.println("Please type in the growth rate");
            r = Double.valueOf(scan.nextLine().trim());
        }
        System.out.println("Now finally, type in the time "
                + "interval");
        x = Integer.valueOf(scan.nextLine().trim());
        double roundedExpo2 = calculateExpoGrowth(a, r, x);
        DecimalFormat formatter = new DecimalFormat(
                "###,###,###");
        String output = formatter.format(roundedExpo2);
        System.out.println(
                "The program has calculated the Exponential "
                + "growth is: " + output + ".");
        return true;
    }

    public static double growthRate(double pastVal,
            double presentVal) {
        double newVal = presentVal - pastVal;
        return newVal / pastVal;
    }

    public static double calculateExpoGrowth(int a, double r,
            int x) {
        // Exponential growth formula
        return (a * Math.pow((1d + r), (double) x));
    }
}


Displaying random elements for a quiz app I'm a teacher and I built a one-page web app to display random question sets for quizzes and tests. The script I wrote selects one question from a set (array) of questions.

The script works fine, but I wonder if it follows best practices or if it could be made more efficient. I want to share my app with other teachers, but I don't want to spread bad practices.

Here's a fiddle of the app itself.

Clicking anywhere on the page brings up a spinner. Clicking again brings up five new random questions. There is more JS in the app, but I'm mainly concerned about the JS related to the question randomization.

// change questions here -- in quotes, comma separated
var qset1 = new Array("Set 1 Question 1","Set 1 Question 2","Set 1 Question 3","Set 1 Question 4","Set 1 Question 5");

var qset2 = new Array("Set 2 Question 1","Set 2 Question 2","Set 2 Question 3","Set 2 Question 4","Set 2 Question 5");

var qset3 = new Array("Set 3 Question 1","Set 3 Question 2","Set 3 Question 3","Set 3 Question 4","Set 3 Question 5");

var qset4 = new Array("Set 4 Question 1","Set 4 Question 2","Set 4 Question 4","Set 4 Question 4","Set 4 Question 5");

var qset5 = new Array("Set 5 Question 1","Set 5 Question 2","Set 5 Question 5","Set 5 Question 4","Set 5 Question 5");

function setUP() {
var set1=Math.floor(Math.random()*qset1.length);
var set2=Math.floor(Math.random()*qset2.length);
var set3=Math.floor(Math.random()*qset3.length);
var set4=Math.floor(Math.random()*qset4.length);
var set5=Math.floor(Math.random()*qset5.length);

document.getElementById('set_1').innerHTML = qset1[set1];
document.getElementById('set_2').innerHTML = qset2[set2];
document.getElementById('set_3').innerHTML = qset3[set3];
document.getElementById('set_4').innerHTML = qset4[set4];
document.getElementById('set_5').innerHTML = qset5[set5];

}

I assume your sets won't actually look like that, and that was just for the sake of this example.  Right now you're doing the same operation on a bunch of virtually identical things - this is exactly the use case for a for-loop.

First we need to put all of our questions into an array of arrays, and then we can loop over them and do whatever we want.  I made a few other changes as well - by using the nth-child() selector I don't have to have the ids - you can switch between the two methods by changing which lines are commented out.  You can run this code snippet and see how it works down below.

function populateQuestions() {
  var questionSets = [
    ["Set 1 Question 1", "Set 1 Question 2", "Set 1 Question 3", "Set 1 Question 4", "Set 1 Question 5"],
    ["Set 2 Question 1", "Set 2 Question 2", "Set 2 Question 3", "Set 2 Question 4", "Set 2 Question 5"],
    ["Set 3 Question 1", "Set 3 Question 2", "Set 3 Question 3", "Set 3 Question 4", "Set 3 Question 5"],
    ["Set 4 Question 1", "Set 4 Question 2", "Set 4 Question 4", "Set 4 Question 4", "Set 4 Question 5"],
    ["Set 5 Question 1", "Set 5 Question 2", "Set 5 Question 5", "Set 5 Question 4", "Set 5 Question 5"]
  ];

  for (var setIndex = 0; setIndex < questionSets.length; ++setIndex) {
    var questionSet = questionSets[setIndex];
    var questionIndex = Math.floor(Math.random() * questionSet.length);
    var question = questionSet[questionIndex];
    var selector = '#questions li:nth-child(' + (setIndex + 1).toString() + ')';
    //var setId = 'set_' + (setIndex + 1).toString();
    document.querySelector(selector).innerHTML = question;
    //document.getElementById(setId).innerHTML = question;
  }
}

First question

Second question

Third question

Fourth question

Fifth question

<button type=button onclick="populateQuestions()">Populate the questions!</button>

I do have one potential concern - right now you're still presenting the sets in the same order, but you might want to randomize what order the sets appear in. I'll leave that as an exercise to the reader, but it isn't much more complicated than this. You might also want to not present questions that have already been seen before, which is a little tricky. You either want to remove them from the sets once they've been used, or have an object to keep track of ones that have already been used.

Lastly, it might be worthwhile to write a function like getQuestions that looks like this

function getQuestions(callback) {
    // do something to get the questions
    var questions = ...;
    // once the questions are ready, use the callback
    callback(questions);
}

function populateQuestions(questionSets) {
    // same code, minus the creation of questionSets
}

getQuestions(populateQuestions);

We use the callback so this can become an asynchronous method - if you ever wanted to get this data via AJAX, or a web-service, or database, or wherever, it becomes much easier to integrate that into your workflow.


Implement a readN() from read7() Exercise:

  Using a read7() method that returns 7 characters from a file,
  implement readN(n) which reads n characters.

  For example, given a file with the content "Hello world", three
  read7() returns "Hello w", "orld" and then "".

I made a version using std::deque. It seems inefficient to me with all the loops to read/write a string, but I couldn't think of an alternative for this FIFO. Any comments on that part would be especially appreciated.

#include <iostream>
#include <sstream>
#include <deque>

inline auto read7()
{
  static std::istringstream is("1234567890123456789012345678901234567890");
  char res[8]{};
  is.get(res, 8);
  return std::string(res);
}

template <typename T1, typename T2>
auto& operator<<(std::deque<T1>& deq, const T2& seq)
{
  for (const auto& elm : seq)
  {
    deq.push_back(elm);
  }
  return deq;
}

template <typename T1, typename T2>
auto get(std::deque<T2>& deq, std::size_t n)
{
  T1 res;
  while (res.size() != n && !deq.empty())
  {
    res += deq.front();
    deq.pop_front();
  }
  return res;
}

auto readN(std::size_t n)
{
  static std::deque<char> buffer;
  while (n > buffer.size())
  {
    auto old_sz = buffer.size();
    buffer << read7();
    if (old_sz == buffer.size())
      break;
  }

  return get<std::string>(buffer, n);
}

int main()
{
  std::size_t sz;
  while(std::cin >> sz)
    std::cout << readN(sz) << '\n';

  return 0;
}

Why is read7 declared inline? You should only do this when it's needed as an optimization. (And it's particularly confusing when the function contains a static variable.)

It's not necessary for is to be static. It should really be an argument to read7, so it can work on any istream. (In real code, buffer should also be a parameter to readN, but for this toy problem it's reasonable to have it static.)

Bug: what happens if the input contains null characters?

You could avoid the queue by using a string as the queue. You can simply append the result of each read7 to it until you have enough characters, then remove and return the first n.


SPOJ GENERAL: sorting by swaps of distance k I have been trying to solve this simple problem on SPOJ for quite some time now, but I keep on getting TLE (Time limit exceeded) for some reason.

Since the problem is in Portuguese, a brief description of the problem is like this (without the story):

  You are given an array of size N, you have to arrange the array in
  ascending order, such that an element can only be swapped with
  elements which are at a distance k from it. If the array can be
  sorted then print the number of swaps required to arrange them in
  ascending order, if it cannot be sorted print impossivel.

This is my code:

#include <iostream>
#include <cstdio>

using namespace std;
int a[100005];
int main() {
    int t;
    int n, k;
    scanf("%d", &t); //number of test cases
    while(t--) {
        scanf("%d %d", &n, &k);
        bool result = true;
        int count = 0;

        for(int i = 0; i < n; i++) {
            scanf("%d", &a[i]);
        }

        for(int i = n; i > 0; i = i - k) {
            int j = 0;
            for( ; j < i - k; j++) {
                if(a[j] > a[j + k]) {
                    int temp = a[j];
                    a[j] = a[j + k];
                    a[j + k] = temp;
                    count++;
                }
            }

            for( ; j < i - 1; j++) {
                if(a[j] > a[j + 1]) {
                    result = false;
                    break;
                }
            }

            if(!result)
                break;
        }
        if(result)
            printf("%d\n", count);
        else
            printf("impossivel\n");
    }
}

My logic : I perform N/k iterations on the array. I initialize the loop variable i to N. In each iteration I check i-k elements with the element at a distance k from it, if they are to be swapped then I swap them and increment the number of swaps needed, else I do nothing. Then I check the elements from i-k to i, if they are in ascending order, if not I break the loop and print "impossivel", else I change i to i-k and again perform the loop. By my logic after every iteration the last k elements will be in ascending order, if is possible to sort them, since at every step I move the elements which are greater to the right.

Does this seem correct to you? How can optimize this further?

The above code can be optimized a little by below changes.

Replace this

        for( ; j < i - k; j++) {
            if(a[j] > a[j + k]) {
                int temp = a[j];
                a[j] = a[j + k];
                a[j + k] = temp;
                count++;
            }
        }

        for( ; j < i - 1; j++) {
            if(a[j] > a[j + 1]) {
                result = false;
                break;
            }
        }

        if(!result)
            break;

By

        int  j = i-k;
        int x = 0;
        int tmp[i/k]; // Do check for i==0 here
        for(int j = i; j >= 0; j -= k){
            tmp[x] = a[j];
            x++;
        }
        sort(tmp);
        for(int j = i; j >= 0; j -= k){
             a[j] = tmp[x]; x--;
        }

Add a check outside the main loop if its sorted or not.
But still this will be in order of \$O(N*KlogK)\$ which in worst cases \$O(N^2logN)\$. You might still get the TLE with that.

We can solve this in \$O(NlogN)\$. Trying thinking of solution in which you sort this a keeping original indexes of each element. Then calculate the number of swap by comparing the old index with new one.


Permutations of a string I wrote the following code to permute the characters of a string assuming there is no repeated character.

I also want to make sure the space complexity of this algorithm is \$O(n*n!)\$: There are \$n!\$ recursive calls, for each of these calls, I copy a string of size \$n\$. Am I right?

Time complexity: \$O(n!)\$: \$n!\$ recursive calls

Is it possible to make it more efficient?

  public static void permute(String str, String prefix){
    if (str.length() == 0) System.out.println(prefix);

    for (int i = 0; i < str.length(); i++){
      String c = Character.toString(str.charAt(i));
      String rest = str.substring(0, i) + str.substring(i+1);
      permute(rest, prefix + c);
    }

  }

The number of permutations of n choose n items is always going to be n!. The way to optimize the solution is to prune the permutations to those that might solve the problem. This requires domain knowledge.

For example if the problem involves ordinary English words all permutations  matching *qz* or *mA* can be eliminated.

Incidentally, \$O(n*n!)\$ can be shortened to \$O(n!)\$ since it is roughly \$O((n+1)!)\$


Python file modifying script with focus on OOP I am currently coding a tool for my work that modifies a textfile by removing unneeded parts of a string at specific spots in the file.
I would like to know if there are any obvious things I am missing or doing too complicated.
I know that some of the comments are considered too long for PEP8.

class vis_file_tools:

    def __init__(self):
        pass

    # Splits the input line into list (by a space), removes the last position, joins them with spaces and returns the modified line.
    def remove_jedec_line(self, input_line):
        input_line_split = input_line.split(" ")         # splits the string that was input through the function into a list
        del input_line_split[-1]                         # deletes the last line from the list (In this case it removes the jedec)
        output_line = " ".join(input_line_split)         # joins back the list to a string. Joins the list with a space
        output_line = output_line + "\n"
        return output_line                               # returns the finished string to the caller of the function

    def remove_jedecs_file(self, path_input_file, path_output_file):
        with open(path_input_file, "r") as file:        # Opens the input file read only
            temp_data = ""
            comp_line_reached = False
            for line in file:                           # "line" in this case is already a variable. Do not us file.readline() when using the "for line" loop.
                if "COMP" in line:                      # Sets the bool "comp_line_reached" to "true" when the loop reaches the "COMP" in the .vis file which marks the start of the components
                    comp_line_reached = True
                    temp_data = temp_data + line
                elif comp_line_reached is False:        # Simply writes the lines from the file into a variable until the component part is reached.
                    temp_data = temp_data + line
                else:
                    line_no_jedec = self.remove_jedec_line(line)
                    temp_data = temp_data + line_no_jedec
        with open(path_output_file, "w") as write_file:
            write_file.write(temp_data)
            temp_data = ""                              # Empties memory

PEP-8

ClassNames should be BumpyWords; snake_case is used for things like method_names and variable_names.  So you class should be:

class VisFileTools:
    ...

Unnecessary __init__ method

This method does nothing; it may safely be omitted.

    def __init__(self):
        pass

Public / Private

remove_jedec_line looks like a private method, used by the remove_jedecs_file method.  If remove_jedec_line is not supposed to be used by actors outside of the class, it should be prefixed with a leading underscore:

    def _remove_jedec_line(self, inputline):
        ...

Garbage Collection

temp_data = ""    # Empties memory

This is unnecessary.  The variable is about to go out of scope, which will naturally release the memory it is holding.

Stop Writing Classes

See the video Stop Writing Classes

Your class is unnecessary.  It has no data members.  It has one public method.  It should just be a function.

Simplified code

Here is a function version of your code (untested):

def remove_jedecs_file(path_input_file: str, path_output_file: str) -> None:
    """
    Read `path_input_file`, copying each line to `path_output_file`.
    Once the start of components is reached (indicated by a line containing "COMP"),
    the last "word" (jedec) of each line is removed.
    """

    def remove_jedec_line(input_line: str) -> str:
        return " ".join(input_line.split(" ")[:-1]) + "\n"

    with open(path_input_file) as file, open(path_output_file, "w") as write_file:
        comp_line_reached = False

        for line in file:
            if comp_line_reached:
                line = remove_jedec_line(line)
            else:
                comp_line_reached = "COMP" in line

            write_file.write(line)

Notes:

Type-hints (Python 3.6+) have been added to the function (: str and -> None)
A """docstring""" has been added to describe the function.
A nested function is used for remove_jedec_line(), making it "private".
[:-1] returns all but the last element in a list, which is simpler than using del input_line_split[-1].
Lines are not being accumulated in temp_data.  Instead, lines are written out immediately after being read in, which is a much lighter load on memory, and avoid \$O(N^2)\$ string concatenation.
"COMP" in line is a "slow" search operation, looking for that substring in the line string.  When found, it sets a flag.  Here, once the flag is set, we can skip the slow substring search.  But I'm introducing a slight change in behaviour: if subsequent lines also contain "COMP", the OP version would not do the remove_jedec_line() transformation where as mine will.  The assumption here is that "COMP" only appears once.


Changing state of a light (Uncle Bob's light switch problem) I watched Robert Martin's programming 101 video. I'll explain what problem he talks about in the video for those of you who haven't watched it.

Robert Martin is in his house and shows a light switch. If this switch goes up, a light is on. If switch goes down, the light is off. He wrote a code to handle it. This is simple but the situation is getting more complicated when he shows second, third and fourth light switch. All of them change state of the same light. Each of them is dependent on the others. He also shows how to handle it in a code.

Robert Martin explains that this code is not the best solution because there can be a situation when two people come to the same room and they want to turn the light on. He demonstrated that the code he has written doesn't work well because the light doesn't change its state. It's happening because a computer doesn't see simultaneous events. One of them occurs first. That's happening when two people want to turn on the light. It happens simultaneously only from human perspective. For a computer one person changes state of the light first so in fact the state of the light is changed twice. Fortunately Uncle Bob shows how to solve this problem. He measures time elapsed from the last light state change. If the next change is happening in 500 ms, he rejects it. Now when two people want to turn the light on the light is actually on.

Unfortunately implementing above solution breaks a case when one person quickly turns on and turns off the light. Because the second change is happening in 500 ms it is rejected. So it is not possible to quickly turn on and turn off the light. Uncle Bob didn't show how to solve this problem. He left it as a homework.

It took me long to figure it out how can I solve this but finally I've came to a working solution. I'm not sure though if my solution is good or maybe I only think it is good. Can you tell me if I solved the problem correctly or there is a better solution?

Uncle Bob's code looks like this:

void lightLogic() {
    boolean a = switchA.isUp();
    boolean b = switchB.isUp();
    boolean c = switchC.isUp();
    boolean d = switchD.isUp();
    int currentSwitchState = 0;

    int base=10;
    if (a) currentSwitchState += base*base*base;
    if (b) currentSwitchState += base*base;
    if (c) currentSwitchState += base;
    if (d) currentSwitchState += 1;

    int currentTime = millis();
    if (currentSwitchState != lastSwitchState) {
        if (currentTime - lastChangeTime > 500) {
            light.state = !light.state;
            lastChangeTime = currentTime;
        }
    }

    lastSwitchState = currentSwitchState;

}

I introduced a new variable called penultimateSwitchState and I check if the current state of the light equals to penultimate state. If yes, I know that the same switch has been used to change light state so I accept that change:

void lightLogic() {
    boolean a = switchA.isUp();
    boolean b = switchB.isUp();
    boolean c = switchC.isUp();
    boolean d = switchD.isUp();
    int currentSwitchState = 0;

    int base=10;
    if (a) currentSwitchState += base*base*base;
    if (b) currentSwitchState += base*base;
    if (c) currentSwitchState += base;
    if (d) currentSwitchState += 1;

    int currentTime = millis();
    if (currentSwitchState != lastSwitchState) {
      if ((currentTime - lastChangeTime > 500) || (penultimateSwitchState == currentSwitchState)) {
        light.state = !light.state;
        lastChangeTime = currentTime;
      }
      penultimateSwitchState = lastSwitchState;
    }

    lastSwitchState = currentSwitchState;
  }
}

Welcome to the world of programming where you never know exactly when you're done because the client doesn't know precisely what they want!

The business solution is to this problem is to first sit down with the client, write down a proposal about some precise interpretation of what should happen. Have the client agree with this interpretation and sign it. Then implement this exactly as proposed.

Then later on when the client tests your code they're either happy (yay, we're done!) or they are not. If they're not you write down what they want to change and make them pay for it again. Happy times! (for you at least, the client doesn't like paying again for something "you should have known" in the first place).

Extra note: sometimes a client asks for something that cannot work. They might only realise the contradiction after you explained what exactly happens and offer them 2 possible ways that achieve 1 of the 2 things they ask for. In such a case you let the client decide which of the 2 is more important.

To write working code we first need to know which exact result we want to get.
Here's the current problem more precisely:

1) If 2 (or more) different switches are flipped within a certain time (half a second) of each other, ignore one of them and change the state of the light.
2) If a switch is flipped twice in rapid succession the second flip should not be ignored.

Question: If (1) and (2) happen within half a second, should the light change state?

My answer would be: If a switch is flipped once within half a second, the light should change state (compared to half a second earlier) irregardless of other switches.

Solution 1:

One possible solution is to modify the implementation of the switches themselves. If a switch is first in state off, and then flipped on, it will still say it's off for half a second. Only if it has remained on for half a second will it say it's turned on. This solves the problem of flipping the same switch twice in rappid succession ... with the downside that a light state will change at the earliest half a second after a switch is flipped. This is something the client has to agree on.

If the client agrees on that change, the lightLogic can remain exactly the same as in Bob's solution since that correctly handles the remaining requirement of ignoring multiple switches flipping "simultaniously".

This kind of solution is often used to prevent flickering in oversensitive keys. Consider a switch that has a bad contact. If switched halfway, it might rappidly say ON>OFF>ON>OFF even though you only flicked it once. Looking at a change from a small delay before makes sure that it will only fire once in such cases as intended.

Solution 2:

Alternatively if you require that the light switches on immediatly after a single switch is flipped, you'll have to do it differently.

Let's say the light is switched from off to on. For the next 500 milliseconds, under which condition should the light be switched back off? -> If ALL switches are back to what they were when the light was still off.

This looks similar to your idea, but only looking at the penultimate state isn't enough. If switch A is flipped, and then switch B is flipped 4 times, the light should still be ON because of switch A.

An idea would be like this (note not tested):

int currentTime = millis();
if (currentTime - lastChangeTime <= 500) { // might be considered simultanious
    if(currentSwitchState == initialSwitchState) { //all switches back to initial state
        light.state = initialLightState;
        lastChangeTime = 0; //respond to any new flip after this as if it was new.
    } // else light stays as it is now
} else {
    if (currentSwitchState != previousSwitchState) { // start new case
        light.state = !light.state;
        lastChangeTime = currentTime;
        initialSwitchState = previousSwitchState;
    } // else nothing changed
}
previousSwitchState = currentSwitchState; //always update latest known state


Interactive dictionary with inexact lookups (step 1) I'm currently building an interactive dictionary. I'm using a sample JSON file (i.e. data.json) as a template. But that's not it. My aim is to build an app where I can store the knowledge that I acquire during my learning process.

And I'd like you to take a look at my code to see if there are areas where I can improve the performance of my program.

Code

import json
from difflib import get_close_matches

data = json.load(open('data.json', 'r'))

def newdict(word, definition):
    data.update({word: [definition]})
    with open('data.json', 'w') as file:
        json.dump(data, file)

def tr(word):
    gcm = get_close_matches(word, data, 1, 0.8)
    if word in data:
        print('· ' + word + ':')
        for index, definition in enumerate(data[word]):
            print(str(index+1) + '.', definition)
    elif word.title() in data:
        print('· ' + word.title() + ':')
        for index, definition in enumerate(data[word.title()]):
            print(str(index + 1) + '.', definition)
    elif word.upper() in data:
        print('· ' + word.upper() + ':')
        for index, definition in enumerate(data[word.upper()]):
            print(str(index + 1) + '.', definition)
    elif len(gcm) > 0:
        print('· ' + gcm[0].capitalize() + ':')
        for index, definition in enumerate(data[gcm[0]]):
                print(str(index+1) + '.', definition)
    else:
        print("'%s' definition isn't available at the moment." % word.capitalize())
        x = input('Would you like to add "%s" to the dictionary? ' % word.capitalize())
        x = x.lower()
        if x == 'yes':
            y = input('Type in the meaning of %s: ' % word)
            newdict(word, y)
        else:
            print('Ok, as you wish!')

w = input('Enter a word: ')
w = w.lower()
while w != '0':
    tr(w)
    w = input('\nEnter a word: ')
    w = w.lower()

Is there room for improvement?

The biggest thing that jumps out at me is that your project isn't in the form of a class, it's a function. This is, while not quite discouraged, certainly non-advantageous. Classes make things more portable. If I were building this, I'd structure it something like this:

class fuzzydict:
  """Fuzztdict: A dictionary with inexact lookups
       #Function definitions, etc go here.
  """
  def __init__(self):
    #Create the data file, initialize all your other variables.
  def __str__(self):
    #This one isn't necessary, but it would allow you to customize what would happen when you call print on a fuzzydict.
  def find_word(self,word):
    #Find a word in the dictionary, or report that it (probably) doesn't exist.
  def add_word(self,word,definition):
    #Add a word to the dictionary.

Then, you'd instantiate it with newDict = fuzzydict() and you'd be good to go. You can use stuff like newDict.find_word('apple'), which is a much more intuitive interface than your while loop.

A few other things about your code:

You're checking various versions of your data in the JSON. Do you intend to eventually be able to input data yourself? Then I'd suggest formatting your entries into all UPPER or all lower case, then including how they were originally inputted as an attribute for that entry. This'll eliminate the multiple checks.
Error handling. The difference between a good program and a truly epic program is how gracefully it can recover from something it didn't expect.
As stated in other answers, document. Document, document, document. Even when it's just a particularly interesting way of doing something, make your code as descriptive as possible. Future you will thank you, as well as anybody else who tries to decipher the finished project. Remember docstrings, they're a gift from whatever deities the creators of Python worship.