MVVM Calling command by name from CommandBindings For example I have this property in my ViewModel.

public IStorageFolder CurrentFolder
{
    get
    {
        return _currentFolder;
    }
    set
    {
        Set(ref _currentFolder, value);
        CommandBindings[nameof(NavigateCommand)].Execute(Model);
    }
}

private IStorageFolder _currentFolder = null;

CommandBindings.cs

public class CommandBindings
{
    ...

    public ICommand this[string name]
    {
        get
        {
            return GetCommandByName(name);
        }
    }

    public ICommand GetCommandByName(string name)
    {
       ...
    }
}

Navigate.cs

public class NavigateCommand : ExplorerCommand
{
    public override bool CanExecute(IExplorerModel model)
    {
        return true;
    }

    public override async Task ExecuteAsync(IExplorerModel model)
    {
        ....
    }
}

CommandBindingsProvider.cs

public class CommandBindingsProvider : ICommandBindingsProvider
{
    public CommandBindings GetBindings()
    {
        var commandInfos = new List<CommandInfo>();
        var serviceLocator = ServiceLocator.Current;
        var explorerCommandTypes = GetExplorerCommandTypes();

        foreach (var commandType in explorerCommandTypes)
        {
            var command = serviceLocator.GetInstance(commandType) as ICommand;
            if (command == null) throw new Exception($"No instance registered for {commandType.FullName} command.");
            var commandName = commandType.Name;
            var commandInfo = new CommandInfo(commandName, command);
            commandInfos.Add(commandInfo);
        }

        var bindings = new CommandBindings(commandInfos);
        return bindings;
    }

    private static IEnumerable<Type> GetExplorerCommandTypes()
    {
        var assembly = typeof(ExplorerCommand).GetTypeInfo().Assembly;
        foreach (Type type in assembly.GetTypes())
        {
            if (type.GetTypeInfo().GetCustomAttributes(typeof(ExplorerCommandAttribute), true).Count() > 0)
            {
                yield return type;
            }
        }
    }
}

Commands are registered in CommandBindings by nameof class.
I execute them by using this syntax in code:

CommandBindings[nameof(NavigateCommand)].Execute(Model);

and by this syntax in XAML :

<ic:InvokeCommandAction Command="{Binding CommandBindings[NavigateCommand}]" />

which looks bad for me, but I can't argue it. Is there any rules that I volatile? In any case, is there anything that can improve readability of this line?

Why?

If I want to add new functionality to my control, I create command class, inherit it from ExplorerCommand, decorate it with ExplorerCommandAttribute and I am done. CommandBindings are populated by retrieving all commands decorated by this attribute. I didn't change ViewModel for that. Also UI menu items are populated from this CommandBindings and there is no need to change UI too. This is flexibility that I achieved and won't lose it.

I might not fully understand how your commands are meant to be used, but the following things strike me as rather cumbersome:

Requiring a separate class for each command.
Command (class) names must be globally unique.
Having to pass context to a command.

Alternative

An alternative is to create a single RelayCommand class that wraps an Action:

public class RelayCommand : ICommand
{
    public RelayCommand(Action action) { ... }
    ...
}

In your view models, you can expose commands as bindable properties:

public class MyViewModel
{
    private ICommand _saveDataCommand;
    public ICommand SaveDataCommand
    {
        get { return _saveDataCommand ?? (_saveDataCommand = new RelayCommand(SaveData)); }
    }

    private void SaveData()
    {
        ...
    }
}

And in your views, you can bind to these commands:

<Button Command="{Binding SaveDataCommand}" />

Now, you no longer have to create separate classes, just separate methods (and ICommand properties, but see below). Because those methods live in a specific view model, they likely already have access to the context they need, and their name only needs to be unique within that view model, not globally.

A possible further improvement

I've successfully been using the above approach in WPF, but having to create all those ICommand properties was still cumbersome. Initially I made a code snippet to make them easier to write, but eventually I settled on a different approach.

The data binding system in WPF (and perhaps also UWP, I'm not sure) not only uses reflection, it also checks if a data context implements ICustomTypeDescriptor, and if so, it'll use the GetProperties methods to detect additional 'properties'. I used that to create a view model base class that generates such a 'property' for each method that is marked with a custom [Command] attribute.

This took some work, but in the end it simplified things to just this:

public class MyViewModel : ViewModelBase
{
    [Command]
    public void SaveDataCommand()
    {
        ...
    }
}


Reach certain Wiki page through random links I just read about Wiki game on Wikipedia and wanted to implement it as a fun project for myself using python.

Task:To reach Mathematics Wikipedia page using wikipedia hyperlinks randomly.

Using Beautiful Soup documentation,I was able to create following crude code which is quite slow for my purpose.

import random
import requests
from bs4 import BeautifulSoup

r = requests.get("https://en.wikipedia.org/wiki/Main_Page")
soup = BeautifulSoup(r.content)
a = [link.get('href') for link in soup.find_all('a')]
del a[0:6]
while(soup.title.text != 'Mathematics - Wikipedia'):
    a = [link.get('href') for link in soup.find_all('a')]
    del a[0:6]

    if (a == []):
        add = "/wiki/Special:Random"
    else:
        add = random.choice(a)
        if(add == None):
            add = random.choice(a)

    url = "https://en.wikipedia.org/"+add
    r = requests.get(url)
    soup = BeautifulSoup(r.content)
    print(soup.title.text)

First,I would like to reduce the useless links that this program goes through such as About Wiki,user login,etc.
I knew I can simply use if-else statements but wanted to know if there is another simpler or faster implementation for it in beautiful soup or something else.

Secondly,Any suggestions to improve code would be welcome including additional reading resources.

Wikipedia has an API available. You can make calls to e.g. "https://en.wikipedia.org/w/api.php?action=query&prop=links&titles={title}&pllimit=500" and get back a list of titles the Wikipedia page title links to, without all the internal links (but with non-existing pages and things like categories).

Incidentally, there is even a python package available that makes using it a lot easier, Wikipedia-API. With this your code would become:

import requests
from bs4 import BeautifulSoup
import wikipediaapi
import random
from itertools import count

WIKI = wikipediaapi.Wikipedia('en')

def random_page():
    r = requests.get("https://en.wikipedia.org/wiki/Special:Random")
    soup = BeautifulSoup(r.content)
    page = WIKI.page(soup.title.text[:-12])
    assert page.exists()
    return page

BLACKLISTED = ["Wikipedia:", "Category:", "Template:", "Template talk:", "User:",
               "User talk:", "Module:", "Help:", "File:", "Portal:"]
def blacklisted(title):
    return any(title.startswith(x) for x in BLACKLISTED)

def random_walk(target_title):
    page = random_page()
    for i in count():
        print(i, page)
        if page.title == target_title:
            return i
        links = list(page.links.values())
        # print(f"{len(links)} links")
        if not links:
            return None
        page = random.choice(links)
        while blacklisted(page.title) or not page.exists():
            page = random.choice(links)

if __name__ == "__main__":
    print(random_walk('Mathematics'))

This still needs to blacklist some pages (see BLACKLISTED constant, the content of which I found by trial-and-error) and I'm not quite happy about the trial and error way of getting a random page, but filtering for existing pages needs to fetch them all, which is quite slow.

In any case, this should be a bit faster than actually getting the whole page and parsing it yourself.

I also put the code into function and guarded the calling under a if __name__ == "__main__": to allow reusing of the code. In doing this I took the liberty of adding a counter and returning it in case the target page is found (12000 pages and counting...).

Another question is if this is the best/most fun way to play this game. The way I know it, the goal is to reach a target page as fast as possible. In that case you will want to do a breadth-first search of all links instead (depth-first would fail because there are loops, meaning you could get lost forever). Although I have to admit, it is fun watching the bot and seeing where it wanders...

A good compromise might be checking if any of the links on the current page is to Mathematics, and only randomly jumping to the next page if not.

Some generic comments about your code:

You don't need parenthesis around your conditions, and they are discouraged unless needed to make the condition span multiple lines.
Empty lists are falsey, so you can replace if (a == []): with if not a:.
Your code only tries twice to get a random page, which will work most of the time but will fail eventually. Instead use a while loop that continues indefinitely.


Bare bones SNAKE game This is the bare bones of a SNAKE game that I'm working on, the code runs as is, I've taken great care to keep my code clean and readable. I would appreciate feedback on my code. I plan on creating some graphics to greatly improve the presentation and a menu and high scores.

import sys
import random
import pygame

pygame.init()

class Snake:

    RED = (200, 0, 0)
    BLUE = (0, 0, 190)
    BLACK = (0, 0, 0)
    GREEN = (0, 230, 0)

    score = 0
    xpos = 80
    ypos = 16
    direction = (1, 0)
    snake_tail = [[16, 16], [32, 16], [48, 16], [64, 16], [80, 16]]

    keys = {"GO": pygame.KEYDOWN, "UP": pygame.K_UP, "DOWN": pygame.K_DOWN,
            "LEFT": pygame.K_LEFT, "RIGHT": pygame.K_RIGHT
            }

    fps = pygame.time.Clock()

    def __init__(self, width, height):

        self.width = width
        self.height = height
        self.food_x = random.randint(1, width - 1) * 16
        self.food_y = random.randint(1, height - 1) * 16
        self.screen = pygame.display.set_mode((width * 16, height * 16))

    def main_loop(self):

        while True:

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

                if event.type == self.keys["GO"] and event.key == self.keys["RIGHT"]:
                    self.direction = (1, 0)
                if event.type == self.keys["GO"] and event.key == self.keys["LEFT"]:
                    self.direction = (-1, 0)
                if event.type == self.keys["GO"] and event.key == self.keys["UP"]:
                    self.direction = (0, -1)
                if event.type == self.keys["GO"] and event.key == self.keys["DOWN"]:
                    self.direction = (0, 1)

            self.move_snake()
            self.fps.tick(12)

    def move_snake(self):

        self.xpos += self.direction[0] * 16
        self.ypos += self.direction[1] * 16
        self.snake_tail.append([self.xpos, self.ypos])
        segment = self.snake_tail.pop(0)

        pygame.draw.circle(self.screen, self.BLACK, (segment[0], segment[1]), 8)

        for segments in self.snake_tail:
            pygame.draw.circle(self.screen, self.BLUE, (segments[0], segments[1]), 8)

        pygame.draw.circle(self.screen, self.GREEN, (self.food_x, self.food_y), 8)
        pygame.draw.circle(self.screen, self.RED, (segments[0], segments[1]), 8)
        pygame.display.update()

        self.kill_snake()

    def kill_snake(self):

        if self.snake_tail[-1] in self.snake_tail[:-1]:
            pygame.quit()
            sys.exit()
        if self.xpos <= 0 or self.xpos >= self.width * 16 or self.ypos <= 0 or self.ypos >= self.height * 16:
            pygame.quit()
            sys.exit()

        self.feed_snake()

    def feed_snake(self):

        if self.xpos == self.food_x and self.ypos == self.food_y:
            self.score += 1
            pygame.display.set_caption(str(self.score))
            self.snake_tail.insert(0, [self.xpos, self.ypos])
            self.food_x = (random.randint(1, self.width - 1)) * 16
            self.food_y = (random.randint(1, self.height - 1)) * 16

Snake(25, 25).main_loop()

There are two main issues with your code.

Comments

Explain your code and what you are trying to do. If you keep developing this game, after some time you will have problems to understand what you are doing.

Especially explain the move_snake(), kill_snake() and feed_snake() functions, but also the usage of the variables.

Constants

Rename all constants, like keys to KEYS.

Access all constants with e.g. Snake.KEYS because they are constants, no class variables that are different for every instance.

To improve readability, you can change the indention for KEYS like this:

KEYS = {"GO": pygame.KEYDOWN,
        "UP": pygame.K_UP,
        "DOWN": pygame.K_DOWN,
        "LEFT": pygame.K_LEFT,
        "RIGHT": pygame.K_RIGHT }


Print the length of words as a vertical histogram I thought I'd give it a shot. What I came up with is from a culmination of my last two submissions, incorporating most of the feedback from this community, including better naming of variables, initialization, and making my program a bit more user-friendly. I'm not sure which optimizations I can make or if there are any issues with my code.

Any feedback is welcome!

#include <stdio.h>

/* print the length of words as input to a histogram with vertical bars */
int main(void) {
  int c, i, j;
  int word_length = 0;
  int max_word_frequency = 0;
  int histogram[10] = {0};

  putchar('>');
  while ((c = getchar()) != EOF && word_length < 10) {
    if (c != ' ' && c != '\n' && c != '\t') {
      ++word_length;
    }
    else {
      ++histogram[word_length];
      word_length = 0;
    }
  }
  for (i = 0; i < 10; ++i) {
    printf("%d ", i);
    if (histogram[i] > max_word_frequency)
      max_word_frequency = histogram[i];
  }
  putchar('\n');
  for (i = 0; i < max_word_frequency; ++i) {
    for (j = 0; j < 9; ++j) {
      if (histogram[j] > 0) {
         printf("x ");
         histogram[j] = histogram[j] -1;
      }
      else {
         printf("  ");
      }
    }
  putchar('\n');
  }
}

main()

Your main() does three things: print a prompt, count the frequencies, and print the histogram.  The latter two, in particular, are well-defined complex tasks, they each deserve to be a function.

If the input is redirected from a file, rather than typed directly on the console, then the > prompt just messes up the output.  To see if standard input is a console, you should test isatty() on Unix, or _isatty() on Windows.

int main() {
    int histogram[21];
    if (isatty(fileno(stdin))) {
        putchar('>');
    }
    count_word_lengths(stdin, histogram, sizeof histogram / sizeof histogram[0]);
    print_histogram(histogram, sizeof histogram / sizeof histogram[0]);
}

Counting

If the input ends abruptly with a word followed by EOF, with no intervening whitespace, then the last word doesn't get counted.

Instead of if (c != ' ' && c != '\n' && c != '\t'), consider using tests like isspace() and ispunct().  By the way, you are treating punctuation as if they were part of words, which is probably not the right behaviour.

If the input contains any word with length 10 or greater, then the counting terminates prematurely with no indication that anything went wrong.  (Personally, I'd choose to count all excessively long words as if it were the maximum length supported by the histogram array.)

Printing

Destroying the histogram data as you print is a bad idea, especially if it will cause a print_histogram() function to wipe out its input parameter.

Once you fix that, the printf("x ") and printf("  ") would be better with a ternary expression.

for (int row = 1; row <= max_word_frequency; ++row) {
    for (int i = 0; i < 9; ++i) {
        printf(row <= histogram[i] ? "x " : "  ");
    }
    putchar('\n');
}


Export Hero information and list of his items to excel sheet/csv I created simple export tool for my project. My requirments was to create one csv file for details of my hero which contains name, level, gold etc. And second file for list of items.
Both of files contains static header and generated Data.
First, let me share my model classes:
public class Hero {
    private String name;
    private int level;
    private List<Item> items = new ArrayList();
    private String className;
    private BigDecimal gold;

//getters setters constructors
}

public class Item {

    private int id;
    private String name;
    private double weight;
//getters setters constructors
}

public class Weapon extends Item {

    private int dmg;
    private int level;
//getters setters constructors
}

public class Food extends Item {

    private int capacity;
    private int hpRegen;
//getters setters constructors
}

I wont share real classes, because they are a little too big, so I have prepared sample classes.
And here we go with example 'exporter'. This class has two public methods which return CSVWriter which we can use to save data to file.
public class HeroWriter {

    private Hero hero;

    private final static String[] HERO_INFO_HEADER = {
            "server",
            "date",
            "name",
            "class",
            "level",
            "gold"
    };

    private final static String[] ITEM_LIST_HEADER = {
            "id",
            "name",
            "weight",
            "dmg",
            "hp",
            "required level",
            "capacity"
    };

    public HeroWriter(Hero hero) {
        this.hero = hero;
    }

    public CSVWriter writeHeroInformation() {
        CSVWriter writer = new CSVWriter(null);
        writer.writeNext(HERO_INFO_HEADER);

        String[] heroInformation = {
                "Mocked Server",
                new Date().toString(),
                hero.getName(),
                hero.getClassName(),
                hero.getLevel()+"",
                hero.getGold().toString()
        };

        writer.writeNext(heroInformation);

        return writer;
    }

    public CSVWriter writeItemList() {
        CSVWriter writer = new CSVWriter(null);
        writer.writeNext(ITEM_LIST_HEADER);

        for (Item item : hero.getItems()) {
            if(item instanceof Weapon) {
                Weapon weapon = (Weapon) item;

                String[] weaponDetail = {
                        weapon.getId() + "",
                        weapon.getName(),
                        weapon.getWeight() + "",
                        weapon.getDmg() + "",
                        "-",
                        weapon.getLevel() + "",
                        "-"
                };
                writer.writeNext(weaponDetail);

            } else if (item instanceof Food) {
                Food food = (Food) item;

                String[] foodDetail = {
                        food.getId() + "",
                        food.getName(),
                        food.getWeight() + "",
                        "-",
                        food.getHpRegen() + "",
                        "-",
                        food.getCapacity() + ""
                };
                writer.writeNext(foodDetail);
            }
        }

        return writer;
    }
}

I would like to as you to review HeroWriter class, because I have problem with OOP and would like to improve in this aspect.
I was thinking that maybe I could create Exportable interface which contains method like String[] export(); and I would implement it in Hero/Weapon/Item class that would help me remove instanceof part of code. But I am not sure if it is good idea.

hero.getLevel()+""

Use an explicit cast instead of implicit one:
Integer.toString(hero.getLevel())

```java
new Date().toString(),
// ...
hero.getGold().toString()

Which might or might not be the format you want in the final file. Ideally you'd define explicit formatting rules.

There are a few approaches to serialization of data like in your case.
POJOs are Serializable
The first approach is to teach your POJOs how they are being serialized. That means that they implement some interface, like CsvSerializable which defines the method `serialize(CSVWriter csvWriter).
This has the upsides that the the objects themselves know how to serialize themselves. The downsides are that your data is now bound to CSVWriter and that you need to thing about where and how to put the header.
Adapters
The second possibility is to use "adapters", which means that you have Hero and HeroSerializer classes. The later has a method like serialize(Hero hero, CSVWriter csvWriter).
This has the upside that your data classes are fine on their own, the downside is that you need two classes per class (that's 100% more class per class) and that the logic you already have is not that much improved.
Monolithic serializer
That's basically that approach that you already have. With some minor changes (like encapsulating more logic into separate methods, for example the writing of each different item type into its own method) it's actually a very good approach. It keeps your data classes on their own, but you have an ever growing monolith.
Reflective serialization
Fields
Now comes the fun one, you could use reflection to access the fields at runtime. That means that your ITEM_LIST_HEADER actually becomes the key to serialization of your objects. That approach is more complicated, but means a little bit more flexibility. The downside is that your fields always must be the same than the headers...well, if you consider that a downside.
So what you could be doing is something like this:
private void writeLine(CSVWriter csvWriter, Object object, String[] fieldNames) {
    Map<String, Field> fields = getFields(object.getClass());
    String[] values = new String[fields.length];

    for (int index = 0; index < fieldNames.length; index++) {
        String currentFieldName = fieldNames[index];
        Field field = fields.get(currentFieldName);

        if (field != null) {
            values[index] = Objects.toString(field.getObject(object));
        } else {
            values[index] = "-";
        }
    }

    csvWriter.writeNext(values);
}

private Map<String, Field> getFields(Class<?> clazz) {
    // Could be cached for each class if performance is important.
    Map<String, Field> fields = new HashMap<>();

    for (Field field : clazz.getDeclaredFields()) {
        // Maybe add some checks, like if the field is static or not.
        field.setAccessible(true);
        fields.put(field.getName(), field);
    }

    return fields;
}

Untested and without exception handling, but I guess you see where this is going.
Getters/Setters
Like above, you can access the Getters/Setters reflectively. It is close to the same solution as above, with the difference that you don't need the set the fields accessible. However, you must be slightly more selective with what methods you expose in that case, and you need to convert the names.


Testing a chance-based class So, chance, huh? Randomness.

I'm looking into how I can test a class whose sole responsibility is to return weighted random results, that is, where there is a set probability of returning true.

The trouble is, random being what it is, sometimes the result doesn't reflect the initial probability.

Here's my attempt, which passes the specs most of the time.

chance.rb

class Chance

  include Singleton

  def self.percent(amount)
    (rand(100) + 1) <= amount
  end

  # x in y chance of being true
  def self.proportion(x, y)
    rand(y) <= x
  end

end

chance_spec.rb

describe Chance do

  context 'percent' do

    it 'should return true 50% of the time' do
      results = 100.times.collect{ Chance.percent(50) }
      results.select{|x|x}.count.should be_within(10).of(50)
    end

    it 'should return true 75% of the time' do
      results = 100.times.collect{ Chance.percent(75) }
      results.select{|x|x}.count.should be_within(10).of(75)
    end

    it 'should return true 25% of the time' do
      results = 100.times.collect{ Chance.percent(25) }
      results.select{|x|x}.count.should be_within(10).of(25)
    end

    it 'should return true 100% of the time' do
      results = 100.times.collect{ Chance.percent(100) }
      results.select{|x|x}.count.should eq(100)
    end

    it 'should return true 0% of the time' do
      results = 100.times.collect{ Chance.percent(0) }
      results.select{|x|x}.count.should eq(0)
    end

  end

  context 'proportion' do

    it 'should return true 6 in 12 times' do
      results = 12.times.collect{ Chance.proportion(6, 12) }
      results.select{|x|x}.count.should be_within(2).of(6)
    end

    it 'should return true 2 in 6 times' do
      results = 6.times.collect{ Chance.proportion(2, 6) }
      results.select{|x|x}.count.should be_within(1).of(2)
    end

  end

end

Any clues?

A few notes on your current code:

Your test case descriptions don't make a lot of sense - try reading them out loud: It should return true 50% of the time... and 75% of the time... and 25% of the time... etc.. Wait, which is it? You're omitting the argument in the description, so the descriptions become confusing and/or meaningless.
You're adding 1 in the percent method, but not in the proportion method. So a zero-in-1 chance always works out to true! And even a 0 in 1,000,000 is still a chance! Probably not what you want. For both methods I'd suggest using a range argument instead.
You're including Singleton for no reason, since you define your methods as class/static methods. So there's no point in instantiating the class - as a singleton or otherwise.
What happens if you pass floating point or negative numbers? That's an exercise for the reader.
Your results.select{|x|x}.count can be simplified to just results.count(&:itself)
Let one method call the other: percent could just call proportion(amount, 100). That way you can focus on testing proportion as that's the only one that actually doesn't anything random. And you'd more easily avoid/fix the bug mentioned above, since it'd only exist in one method, not two.

But as for testing randomness in general: In this case, I'd say don't bother. Because you can't really test it. Your current tests are fuzzy (±10 within target) just so they'll pass more reliably. But that's changing the tests to suit the output, rather than the other way around.

What you want are tests that are strictly pass/fail. This you can do for things like it "raises an ArgumentError for negative arguments" but not so much for the normal operation of the methods.

What it comes down to is whether you trust Ruby to:

do basic arithmetic and comparison
reliably generate pseudo random numbers (here's the C source for Ruby's random, in case you're interested)

If you trust that, well, testing doesn't make much sense anymore.

Instead spend the time reviewing your code throughly. As seen above, your tests didn't cover the zero-in-N chance bug, and you didn't catch it either. For 2 single-line methods, you wrote an order of magnitude more test code - that effort seems misplaced, when the tests are fudged to pass and the code is buggy.

If you still insist on writing a test, then write a single test case that takes maybe a million samples for 50% or 1-in-2, and checks whether the result is within a narrower tolerance. But I'd run this test by itself a dozen times, but disable it when running the full suite (e.g. tag it so it can be included/excluded easily). It may still fail because it is still random, but that's no reason to fail the entire suite if you've tested thoroughly beforehand. (Edit: For a small project, it's fine to run this test every time, but if your code is on a CI server or you have automation relying on the result of the tests - e.g. can't commit/push to SCM without green tests - it's a pain to have all that at the mercy of randomness.)

Meanwhile you can still do single-sample tests for outliers: 0% chance should be false, 100% should be true, and 0-in-1 should be false, 1-in-1 should be true. Those can be run with the rest of the suite everytime - and should they suddenly, randomly fail, well, something's definitely wrong so start debugging.


Efficiency of Project Euler 28 - number spiral diagonals I have written a solution for Problem 28.

  Starting with the number 1 and moving to the right in a clockwise direction a 5 by 5 spiral is formed as follows:

  $$\begin{array}{rrrrr}
\color{red}{21} &22 &23 &24 &\color{red}{25}\\
20 & \color{red}{7} & 8 & \color{red}{9} &10\\
19 & 6 & \color{red}{1} & 2 &11\\
18 & \color{red}{5} & 4 & \color{red}{3} &12\\
\color{red}{17} &16 &15 &14 &\color{red}{13}\\
\end{array}$$

  It can be verified that the sum of the numbers on the diagonals is 101.

  What is the sum of the numbers on the diagonals in a 1001 by 1001 spiral formed in the same way?

Since I am a beginner, I am looking for suggestions for improving the efficiency of my program.

from timeit import default_timer as timer
start = timer()
def find_sum(limit):
    limit*=limit
    num=1
    add=2
    result=1
    while num<limit:
        for i in range(4):
            num+=add
            result+=num
        add+=2
    return result
print (find_sum(1001))
elapsed_time = (timer() - start) * 1000 # s --> ms
print ("Found in %r ms." % (elapsed_time))

An ultimate efficiency is in math. It can be shown that the result for \$2n+1\$ by \$2n+1\$ spiral is

$$(\frac{2n}{3}) (8n^2 + 15n + 13) + 1$$

The formula can be derived by induction. Another method is to realize that it must be a cubic dependency and fit the coefficients. And you can see that it is cubic, because numbers along the diagonals grow quadratically.


Clipboard support in jQuery using revealing module pattern I have recently been getting into the habit of leveraging the revealing module pattern for all my code. I used this guide for inspiration, but my code doesn't feel as elegant.

var styleGuide = (function styleGuideHandler() {
        'use strict';

        var publicAPI,
            intervalId = null,
            clipboard = new Clipboard('.copyButton'),
            btns = document.querySelectorAll('.style-guide');

        function setTooltip(btn, message) {
            $(btn).attr('data-original-title', message);
            setTimeout(function() {
                $(btn).tooltip('show');
            }, 150);
        }

        function hideTooltip(btn) {
            if (intervalId !== null) {
                clearTimeout(intervalId);
            }
            intervalId = setTimeout(function() {
                $(btn).tooltip('hide');
                intervalId = null;
            }, 500);
        }

        publicAPI = {
            init: function() {
                clipboard.on('success', function(e) {
                    setTooltip(e.trigger, 'Copied!');
                    hideTooltip(e.trigger);
                    e.clearSelection();
                    console.log(e);
                });

                clipboard.on('error', function(e) {
                    setTooltip(e.trigger, 'Failed!');
                    hideTooltip(e.trigger);
                    console.log(e);
                });

                $('.copyButton').tooltip({
                    trigger: 'click',
                    placement: 'bottom'
                });

                $('pre code').each(function(i, block) {
                    hljs.highlightBlock(block);
                });

                /* preventDefault on buttons */
                for (var i = 0, l = btns.length; i < l; i++) {
                    btns[i].addEventListener('click', function(e) {
                        e.preventDefault();
                        e.stopPropagation();
                    });
                }
            }

        };

        return publicAPI;
    })();

    $(document).ready(styleGuide.init);

Also, would executing the ready function like this $(document).ready(function(){styleGuide.init}); encapsulate the module further? Meaning, there would be no chance the styleGuide module could overwritten?

Interesting question, your code is very readable.

However, if you only reveal an init function, then really there is not much sense in using a revealing pattern.

I would probably not self execute styleGuideHandler but pass it to the jQuery call:

$(document).ready(styleGuideHandler);

Other than that, just for giggles, I might also pass the few globals it uses:

$(document).ready(styleGuideHandler( document, Clipboard, hljs ));

Then when styleGuideHandler is executed, you run the code in init.


Shell script that copies configuration and dotfiles to repository A while ago I had the idea to store all the important configuration and dotfiles from my system in a Git repository. That saved me a few months ago: My harddrive died, but I could get back to work with Ubuntu and that repository quickly.

Only problem: I maintained that repo by manually copying the files from the various directories to the repo directory. That’s what I wrote the shell script for.

What does the script do?

Checks whether the command is executed from within the root of a git repository
Detects which OS it’s operating on (Windows, Linux or OS X) and saves that information in a variable OS
Assigns variables for certain tool groups (like Bash, Git, etc.) and creates the target directories in the repo if they don’t exist
Copies file by file to these destinations

Notes:

Currently, I separate the files on a per-OS basis. This is probably unnecessary and even a hinderance. I’d like to here your opinion here
The script currently does not use any Bash >4 features, because besides Ubuntu I mainly work on Windows. I use Git Bash there which only has version 3 bundled.
I also wrote a short article about this script: Backup configuration and dotfiles with Git and a shell script
If you’re interested in current/updated versions, check out the script in the repo: get-dotfiles.sh

#!/bin/bash

################################################################################
#
# Shell script to copy important configuration files from the current
# environment to this repository.
#
################################################################################

# Exit as soon as a command fails
set -e

# Accessing an empty variable will yield an error
set -u

# Assume the working directory is the path to the dotfiles repository
REPO_PATH="$PWD/"

# Abort mission when we’re not in a git repository
if [[ ! -d .git ]] || ! git rev-parse --git-dir > /dev/null 2>&1; then
    echo "Not a git repository."
    exit 1
fi

# Detect OS (OS X, Linux or Windows)
if [ "$(uname)" = "Darwin" ]; then
    echo "Detected system: OS X"

    OS="osx/"

    ST_DIR="$HOME/Library/Application Support/Sublime Text 3/Packages/User/"
elif [ "$(expr substr $(uname -s) 1 5)" = "Linux" ]; then
    echo "Detected system: Linux"

    OS="linux/"

    ST_DIR="$HOME/.config/sublime-text-3/Packages/User/"
elif [ "$(expr substr $(uname -s) 1 10)" = "MINGW32_NT" ]; then
    echo "Detected system: Windows"

    OS="win/"

    ST_DIR="$HOME/AppData/Roaming/Sublime Text 3/Packages/User/"
    NPM_DIR="$HOME/AppData/Roaming/npm/node_modules/npm/"
fi

# Adjust destination paths so the files are separated by OS
BASH_DEST="${REPO_PATH}${OS}bash/"
GIT_DEST="${REPO_PATH}${OS}git/"
NPM_DEST="${REPO_PATH}${OS}npm/"
RUBY_DEST="${REPO_PATH}${OS}ruby/"
ST_DEST="${REPO_PATH}${OS}sublime-text/"

declare -a destinations=($BASH_DEST $GIT_DEST $NPM_DEST $RUBY_DEST $ST_DEST)

for dest in "${destinations[@]}"; do
    mkdir -p "$dest"
done

# Bash
cp "$HOME/.bashrc" "$BASH_DEST"
cp "$HOME/.bash_aliases" "$BASH_DEST"

# Git
cp "$HOME/.gitignore_global" "$GIT_DEST"
cp "$HOME/.gitconfig" "$GIT_DEST"

# Node.js/npm
cp "${NPM_DIR}.npmrc" "$NPM_DEST"

# Ruby/RubyGems
cp "$HOME/.gemrc" "$RUBY_DEST"

# Sublime Text
cp "${ST_DIR}Preferences.sublime-settings" "$ST_DEST"
cp "${ST_DIR}Markdown.sublime-settings" "$ST_DEST"
cp "${ST_DIR}YAML.sublime-settings" "$ST_DEST"
cp "${ST_DIR}Package Control.sublime-settings" "$ST_DEST"
cp "${ST_DIR}For Loop (range).sublime-snippet" "$ST_DEST"
cp "${ST_DIR}Fraction (TeX).sublime-snippet" "$ST_DEST"

# OS specific copy operations
if [ "$OS" = "osx/" ]; then
    echo "Nothing here."
elif [ "$OS" = "linux/" ]; then
    cp "${ST_DIR}Default (Linux).sublime-keymap" "$ST_DEST"
elif [ "$OS" = "windows/" ]; then
    cp "${ST_DIR}Default (Windows).sublime-keymap" "$ST_DEST"
fi

echo "Completed."

(Shell scripts are new terrain for me, I probably made some faulty assumptions. Still, don’t have mercy.)

This is pretty nicely written. I have a couple of tips to improve it though.

Don't include trailing / in path variables

Although there's absolutely nothing wrong with this:

REPO_PATH="$PWD/"
# ...
OS="osx/"
# ...
BASH_DEST="${REPO_PATH}${OS}bash/"

In my experience, not including the trailing slash is typically more ergonomic, and sometimes even less error-prone:

You might have a function that's expecting the trailing slash, and some callers might forget to append it correctly, and there's no reasonable way to enforce at "compile time".
When I see concatenated path-like variables like $REPO_PATH${OS}bash I'm always suspicious if the caller added the slash or not.
It makes me constantly look over my shoulders and re-check all uses.

Consider this alternative:

REPO_PATH=$PWD
# ...
OS=osx
# ...
BASH_DEST="$REPO_PATH/$OS/bash"

Here, I know that "$REPO_PATH/$OS/bash" will be definitely close to what I expect,
and no need to worry about forgotten slashes anywhere.
I also know it's a path,
which is less obvious in the case of the original.

Don't execute the same thing repeatedly

When you detect the OS,
the script might execute uname 3 times.
It would be better to run it once and save in a variable.

Avoid expr

expr is archaic. Usually there are better solutions.
You could also simplify the OS detection conditions using [[ instead:

uname=$(uname -s)

# Detect OS (OS X, Linux or Windows)
if [[ $uname = Darwin ]]; then
    echo "Detected system: OS X"
    OS=osx
    ST_DIR="$HOME/Library/Application Support/Sublime Text 3/Packages/User/"
elif [[ $uname == *Linux* ]]; then
    echo "Detected system: Linux"
    OS=linux
    ST_DIR="$HOME/.config/sublime-text-3/Packages/User/"
elif [[ $uname == *MINGW32_NT* ]]; then
    echo "Detected system: Windows"
    OS=win
    ST_DIR="$HOME/AppData/Roaming/Sublime Text 3/Packages/User/"
    NPM_DIR="$HOME/AppData/Roaming/npm/node_modules/npm/"
fi

Quote paths that may contain spaces

It's not obvious that these paths will never contain spaces:

declare -a destinations=($BASH_DEST $GIT_DEST $NPM_DEST $RUBY_DEST $ST_DEST)

It's better to be safe, and put double-quotes around each.

More careful error handling

If you couldn't detect the OS, I suggest to print a warning and exit.

Use case for switches

The final OS-specific operations could be written a bit cleaner using a case statement.


Object Oriented vs Not jQuery & JS I have taken a widget/ plugin that I wrote yesterday and just now it has been re-written in an object oriented format. It is certainly more code to write it the OO way, so please let me know also how to analyze the performance of each example of the same widget below:

Please keep in mind that I use data-attr selectors because of their descriptive nature and the fact that it separates style and functionality by not binding its functionality to .class selectors. Yes, I understand ID's are faster. I use a ~ before the = when querying these selectors because I like to leave the data-function or data -widget attr's open to the addition of new functionality (E.G. $('data-function="accordion jsondataset").. in this format javascript will not pick up the jsondataset attribute unless a ~ is added in the selector scripting resembling $('data-function~="jsondataset").

Here is the finished code:

var tabs = function(){
  var tabsWidgetEnvoked = $('[data-widget~="tabs"]');
  if (tabsWidgetEnvoked){ // if data-attr present in DOM
    var settings = [
      { // widget settings as an object
        tabs : $('[data-function="tabs"]'),
        content: $('[data-function="content"]'),
        tabShowing : '1',
        animated: true,
      }
    ]
    $.each(settings, function(){
      // event handling
      tabs = this.tabs;
      content = this.content;
      show = this.tabShowing;
      animated = this.animated;
    });
     if (animated === true){
        $('body').addClass('animated');
      }
      tabs.children().each(function(i){
        i = i + 1;
        $(this).attr('data-nav-order', i);
      });
      content.children().each(function(i){
        i = i+ 1;
        $(this).attr('data-content-order', i);
      });
      tabs.children(':nth-child('+show+')').addClass('active')
      content.children(':nth-child('+show+')').addClass('active')
      tabs.children().click(function(e){
        e.preventDefault();
        tabs.children().removeClass('active')
        $(this).addClass('active');
        requestedView = $(this).attr('data-nav-order');
        content.children().removeClass('active');
        content.children('[data-content-order="'+requestedView+'"]').addClass('active')
      });
  }
}
tabs();

I did add animation and a default view to the object oriented one as well but aside from this, the functionality is the same.

Here is the code for the standard spaghetti code version.

Here is the one I rewrote with an OO approach.

First of all, maybe you should give the user a way to configure the selector - this gives most applications the chance to go for an id selector, others might want to have a more global or unobstrusive approach - so they can just use a data-selector.

Your object oriented approach is not following the standard principles for jquery plugins - from what I see there you are using jquery so why not use their plugin techniques then. As the jquery docs have a huge documentation of that, here is just a small example:

$.fn.tabs= function() {
    this.css( "color", "green" );
};

$( "a" ).tabs(); // Makes all the links green.

See http://learn.jquery.com/plugins/basic-plugin-creation/ for a better explanation. This also makes possible what I showed above - let the user decide what selector to use.

For this simple tabs script I don't see any other considerations when it comes to performance.


Find minimum peak elements in an array Question: Given an array numbers
$$a := [2, 7, 8, 5, 1, 6, 3, 9, 4]$$
Check the below conditions, both the conditions should be satisfied.
\begin{gather}
\text{If }a[i] > a[i-1]\text{ or if first element }a[i] > a[i+1]
\end{gather}
\begin{gather}
\text{If }a[i] > a[i+1]\text{ or if last element }a[LastIndex] > a[LastIndex - 1]
\end{gather}

1st Iteration - 8, 6, 9 are peak values.

Remove the smallest ele.
Remove 6.
New arr {2, 7, 8, 5, 1, 3, 9, 4}.
Output Arr - {6}

2nd Iteration - 8, 9 are peak values.

Remove the smallest ele.
Remove 8.
New arr {2, 7, 5, 1, 3, 9, 4}.
Output Arr - {6, 8}

3rd Iteration - 7, 9 are peak values.

Remove the smallest ele.
Remove 7. New arr {2, 5, 1, 3, 9, 4}.
Output Arr - {6, 7, 8}

4th Iteration - 5, 9 are peak values.

Remove the smallest ele.
Remove 5.
New arr {2, 1, 3, 9, 4}.
Output Arr - {6, 7, 8, 5}

5th Iteration - 2, 9 are peak values.

Remove the smallest ele.
Remove 2.
New arr {1, 3, 9, 4}.
Output Arr - {6, 7, 8, 5, 2}

6th Iteration - 9 are peak values.

Remove the smallest ele.
Remove 9.
New arr {1, 3, 4}.
Output Arr - {6, 7, 8, 5, 2, 9}

7th Iteration - 4 are peak values.

Remove the smallest ele.
Remove 4.
New arr {1, 3}.
Output Arr - {6, 7, 8, 5, 2, 9, 4}

8th Iteration - 3 are peak values.

Remove the smallest ele.
Remove 3.
New arr {1}.
Output Arr - {6, 7, 8, 5, 2, 9, 4, 3}

9th Iteration - 1 are peak values.

Remove the smallest ele.
Remove 1.
New arr {1}.
Output Arr - {6, 7, 8, 5, 2, 9, 4, 3, 1}

Output: {6, 8, 7, 5, 2, 9, 4, 3, 1}
My solution is working but I am looking for optimized solution. Please let me know.
Here is my code:
public int[] findMinimumPeaks(int[] arr){
        List<Integer> list1 = new ArrayList<Integer>(arr.length);
        int[] output = new int[arr.length];
        for(int i: arr)
            list1.add(i);

        for(int i =0; i<arr.length; i++){
            int minIndex = minimumPeakElement(list1);
            output[i] = list1.get(minIndex);
            list1.remove(minIndex);
        }
        return output;
    }

    public int minimumPeakElement(List<Integer> list1){
        int minIndex = 0, peakStart = Integer.MAX_VALUE, peakEnd = Integer.MAX_VALUE;
        int peak = Integer.MAX_VALUE, minPeak = Integer.MAX_VALUE;

        if(list1.size() >= 2){
            if(list1.get(0) > list1.get(1)) peakStart = list1.get(0);
            if(list1.get(list1.size() - 1) > list1.get(list1.size() - 2)) peakEnd = list1.get(list1.size() - 1);
            if(peakStart < peakEnd){
                minPeak = peakStart;
                minIndex = 0;
            }
            else if(peakEnd < peakStart){
                minPeak = peakEnd;
                minIndex = list1.size() - 1;
            }
        }

        for(int i=1; i<list1.size() - 1; i++){
            if(list1.get(i) > list1.get(i + 1) && list1.get(i) > list1.get(i-1)) peak = list1.get(i);
            if(peak < minPeak){
                minPeak = peak;
                minIndex = i;
            }
        }
        return minIndex;
    }

```

Thou shalt not bruteforce.
Your algorithm exhibits a quadratic time complexity, and cannot be salvaged. You need a better one.
First observation you should make is that when you remove the peak, other peaks stay put. That alone is enough to see that rescanning the entire array is a waste of time.
More important observation is that once you remove the peak, the new peak may appear only in the position immediately adjacent to the peak being removed. Please prove this fact (or at least convince yourself that it is so). Also notice that the newborn peak would in general be smaller than the existing peaks, if any.
I hope it is enough to get you going.

As for the proper review:

Flat is better than nested. The condition list1.size() >= 2 better be reversed:
  if (list1.size() < 2) {
      // The list consists of a single element, which is a peak,
      // and it resides at index 0
      return 0;
  }
  // Proceed with a general case here
  ....

list1 looks like a strange name. There are no other lists, are there?


Binary conversion app I'm pretty new to ruby and programming in general.

I had an idea for a fun little project that I could use to gain a little experience. I wrote a simple program that takes a 4 bit binary sequence (ie. 1001, 1110, etc) and converts it into its decimal counterpart (ie 1111 = 15). I got the program working but even I can tell it's pretty ugly and limited code. I've been racking my brain trying to figure out a way to improve and I'm thinking if I do something similar to this app (converting to an array and reversing so i can work left to right instead of right to left) but instead of hard coding the if statements for each digit I could use a loop that just doubles the last digit and add that to the sum if it's a one in that spot.

#!/usr/bin/ruby
# binary_conversion.rb

puts "
    ***************************************************
            BINARY CONVERSION TOOL
                VERSION: 0.1
            CREATED BY: Micky Scandal
    ***************************************************
"
sleep(1)
again = 0
while again != "n"
    @zero = 0
    @one = 0
    @two = 0
    @three = 0
    @bin_input = ""
    until @bin_input.length == 4                                    # && bin_input.class == Fixnum
        print "Please enter a 4 bit binary string (ex: 0110):> "
        @bin_input = gets.chomp
    end
    puts "You entered #{@bin_input}"

    @bin_array = @bin_input.to_s.split(//).map(&:to_i)
    @bin_array.reverse!
    p @bin_array
    puts "converting..."; sleep(0.5)

    if @bin_array[0] == 1
        @zero = 1
    end
    if @bin_array[1] == 1
        @one = 2
    end
    if @bin_array[2] == 1
        @two = 4
    end
    if @bin_array[3] == 1
        @three = 8
    end

    ans = @zero + @one + @two + @three
    puts "answer is #{ans}"
    puts ""
    puts "Run again? (Y/N)"
    again = gets.chomp
end

Welcome to SO and glad you are learning Ruby.
You know there are easy ways in Ruby to do convertions between numbers ?
If not take a look at this blog http://rubyquicktips.com/post/18935873770/convert-between-number-bases-easily

Answering your question: you have a lot of boiler code which you could cut by taking the exponent of each digit like here under. While I still could do this shorter by injecting an object instead of declaring the dec = 0 I won't do so for readability.

I use the method each_char on the string to split into a character array and daisychain this result to another method with_index that adds... yes an index.
Then I use the formula below to add the exponent of that char multiplied by 2 (the base) converted to an integer taking into account the position in the string.
This is not the best algorithm by far to do this, just an example how to use these methods to shorten your code and make it more 'Rubylike'.
If you search for good exercises which you can test and compare your results with others solutions taka a look at https://www.codewars.com

Happy code learning !

bin_input = "1111"
puts bin_input.to_i(2)  #=> 15 this is the normal Ruby way

# This is our educational way
dec = 0
print dec
bin_input.to_s.each_char.with_index do |char, index|
  dec += (char.to_i*2)**(bin_input.length-(index+1))
  puts "+(#{char.to_i*2})**(#{bin_input.length-(index+1)})"
end
puts dec # 15

Which gives

15
0+(2)**(3)
+(2)**(2)
+(2)**(1)
+(2)**(0)
15


Crawling pages to obtain .htm and .txt files from sec.gov I am pretty much a python newbie and was looking at this question on StackOverflow.

Whereas the OP was interested in downloading the .htm |.txt files, I was simply interested in using Beautiful Soup and requests to gather all the links, to those files, into one structure. Ideally, it would have been a single list but I have a list of lists.

The process is:

Start with the landing page and grab all the links in the leftmost column which has the header CIK.

CIK column sample with single link selected on right

Navigate to each of those links and grab the document button links in the Format column:

Format column sample with single link selected on right

Navigate to each of those document button links and grab the document links in the Document Format Files section, if the file mask is .htm | .txt.

Document Format Files sample with single link selected on right

Print the list of links (which is a list of lists)

Other info:

I use CSS selectors throughout to target the links.

What particularly concerns me, given my limited Python experience, is the fact I have a list of lists rather than a single list, whether my re-use of variable names is confusing and potentially bug prone, and I guess whether what I am doing is particularly pythonic and efficient.

In languages I am more familiar with, I might attempt to create a class and provide the class with methods; which in effect are my current functions.

Set-up info:

The version of the jupyter notebook server is: 5.5.0

The server is running on this version of Python:

Python 3.6.5 |Anaconda, Inc.| (default, Mar 29 2018, 13:32:41) [MSC v.1900 64 bit (AMD64)]

Python code:

from bs4 import BeautifulSoup
import requests
from urllib.parse import urljoin

base = 'https://www.sec.gov'
start_urls = ["https://www.sec.gov/cgi-bin/browse-edgar?company=&match=&CIK=&filenum=&State=&Country=&SIC=2834&owner=exclude&Find=Find+Companies&action=getcompany"]

def makeSoup(url):
    res = requests.get(url)
    soup = BeautifulSoup(res.content, "lxml")
    return soup

def getLinks(pattern):
    links = soup.select(pattern)
    return links

def getFullLink(links):
    outputLinks = []
    for link in links:
        outputLinks.append(urljoin(base, link.get("href")))
    return outputLinks

soup = makeSoup(start_urls[0])
links = getLinks("#seriesDiv [href*='&CIK']")
firstLinks = getFullLink(links)

penultimateLinks = []

for link in firstLinks:
    soup = makeSoup(link)
    links = getLinks('[id=documentsbutton]')
    nextLinks = getFullLink(links)
    penultimateLinks.append(nextLinks)

finalLinks = []

for link in penultimateLinks:
    for nextLink in link:
        soup = makeSoup(nextLink)
        links = getLinks("[summary='Document Format Files'] td:nth-of-type(3) [href$='.htm'],td:nth-of-type(3) [href$='.txt']")
        nextLinks = getFullLink(links)
        finalLinks.append(nextLinks)

print(finalLinks)

Since you never use getLinks without a call to getFullLink afterwards, I would merge these two functions. I would also make it a generator:

def get_links(soup, pattern):
    for link in soup.select(pattern):
        yield urljoin(base, link.get("href"))

Then your main part can become this nested for loop:

if __name__ == "__main__":
    pattern1 = "#seriesDiv [href*='&CIK']"
    pattern2 = "[id=documentsbutton]"
    pattern3 = "[summary='Document Format Files'] td:nth-of-type(3) [href$='.htm'],td:nth-of-type(3) [href$='.txt']"

    final_links = []
    for first_link in get_links(make_soup(start_urls[0]), pattern1):
        for second_link in get_links(make_soup(first_link), pattern2):
            final_links.extend(get_links(make_soup(second_link), pattern3))
    print(final_links)

I also renamed your functions according to Python's official style-guide, PEP8 and added a if __name__ == "__main__": guard.

One way to make this a bit faster (this sounds like it could be a lot of requests) is to use requests.Session to re-use the connection to the server:

session = requests.Session()

def make_soup(url):
    res = session.get(url)
    res.raise_for_status()
    return BeautifulSoup(res.content, "lxml")

Here I also added a guard so that the program stops if any site does not exists (i.e. returns 404 or similar).


Simple Tic Tac Toe game inside terminal Hello fellow programmers. I started learning C last week and today I wanted to see if I can make a working program, so I made this game:
/*
 * TESTED IN LINUX - 2020
*/

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

int player = 1, choice;
int places[10] = {'o', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

void switchPlayers();
void displayBoard();
int markBoard(char mark);
int checkForWin();

int main() {

    while (!checkForWin()){
        system("clear");
        displayBoard();
        switchPlayers();
    }
    system("clear");
    displayBoard();
    checkForWin();

    return 0;
}

void switchPlayers(){
    if (player == 1) {
        printf("     Player 1 choose: ");
        scanf("%d", &choice);
        if (markBoard('X'))
            player = 1;
        else
            player = 2;

    }
    else if (player == 2){
        printf("     Player 2 choose: ");
        scanf("%d", &choice);
        if (markBoard('O'))
            player = 2;
        else
            player = 1;

    }
}

void displayBoard(){
    printf("\n             X & O           \n Player 1 (X) - Player 2 (O)\n\n");

    printf("\t    |   |   \n"
           "\t  %c | %c | %c \n"
           "\t ___|___|___\n"
           "\t    |   |   \n"
           "\t  %c | %c | %c \n"
           "\t ___|___|___\n"
           "\t    |   |   \n"
           "\t  %c | %c | %c \n"
           "\t    |   |   \n\n"
           ,  places[1], places[2],
           places[3], places[4], places[5],
           places[6], places[7], places[8], places[9]);
}

int markBoard(char mark){
    for (int i = 1; i < 10; ++i) {
        if (choice == i && places[i]-48 == i) {
            places[i] = mark;
            return 0;
        }
    }
    return 1;
}

int checkForWin() {
    short draw = 0;

    //Horizontal check
    for (int i = 1; i < 10; i += 3) {
        if (places[i] == places[i + 1] && places[i + 1] == places[i + 2]) {
            printf("\tYou won!\n");
            return 1;
        }
    }
    //Vertical check
    for (int i = 1; i < 4; i += 1) {
        if (places[i] == places[i + 3] && places[i + 3] == places[i + 6]) {
            printf("\tYou won!\n");
            return 1;
        }
    }
    //Diagonal check
    if (places[1] == places[5] && places[5] == places[9]) {
        printf("\tYou won!\n");
        return 1;
    } else if (places[3] == places[5] && places[5] == places[7]) {
        printf("\tYou won!\n");
        return 1;
    }
    //Check for draw
    for (int j = 1; j < 10; ++j) {
        if (places[j] - 48 != j)
            draw++;
    }
    if (draw == 9){
        printf("\t  Draw!\n");
        return 1;
    }

    return 0;
}

Do you have any tips on how I could make it more efficient or fix something I did wrong?

On the whole this is nicely done for an application made during your first week of C programming. The functions are generally reasonably-sized, the code is comprehensible and the design decisions are understandable given the size and purpose of the application.
My remarks mostly center around improving the design to support new features and avoid bugs. The merit of many of my points may be negligible if you're never planning on adding to the application beyond what you have, but if you'd like to add (for example) even fairly unassuming improvements like an AI opponent or arbitrary board sizes, the current design would be pushed beyond its narrow limits.
Avoid globals
Your core state variables
int player = 1, choice;
int places[10] = {'o', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

should all be scoped to main at minimum. The problem with the existing design is that all functions operating on these global variables are non-idempotent. This means they are unsafe in a multithreaded environment and modify state outside of themselves which makes the application difficult to reason about and may lead to bugs.
The parameterless functions in your original code creates an illusion of simplicity. But in reality these functions are quite unsafe and would inhibit a larger application significantly.
Encapsulate related data
int player = 1, choice;
int places[10] = {'o', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

Are loose variables that are conceptually attributes of the same entity and as such should be grouped in a TicTacToePosition structure along with a set of functions that operate on this structure. This makes it easier to understand the purpose and relationship between these pieces of data. Taken out of context, a variable name like places has non-obvious purpose, but as a struct member, position.places or ttt_state.squares is a bit clearer.
Separate UI from game logic
If you want to generalize and expand on your code, you'll need to detach user interaction from game logic. Doing so makes the code maintainable and expandable through looser coupling.
There are many subtle manifestations of the strong UI-game logic coupling throughout the app, like:
int markBoard(char mark){
    for (int i = 1; i < 10; ++i) {
        //       ^
        if (choice == i && places[i]-48 == i) {
            //                      ^^^
            places[i] = mark;
            return 0;
        }
    }
    return 1;
}

In the above code, the 1-indexing seems to be a convenience measure to avoid having to normalize 1-indexed user input to internal logic. But this convenience leads to an awkward board design for the programmer and a confusing magic 'o' character at the 0-th index.
Additionally, -48 is a conversion between external UI and internal game logic that markBoard shouldn't be responsible for. The correct name for this function is convertFromUserInputCharAndMarkBoard, which is overburdened. Normalize/sanitize user input outside of the tic tac toe board logic. This lets you keep the interface user-centric while supporting an intuitive internal representation for the programmer.
switchPlayers does more than switch players: it also takes user input. Those are two distinct things that should be separate.
checkForWin does win checking but also does IO, a side effect. Better to just return the result and let the caller handle the IO. In fact, checkForWin is called twice in the main function, once to check for a win and the second time to display the winning player after clearing the screen.
UX
I recommend spelling out the input format more precisely and using X wins! or O wins! instead of You won!.
Instead of Player 1 and Player 2, using X and O throughout removes ambiguity and lets you avoid a layer of indirection by having to display/explain Player 1 (X) - Player 2 (O) which asks the user to mentally translate between multiple terms for the players.
Bad input into scanf spams the console and there are no error messages or handling to speak of. scanf isn't the right tool here; use fgets to pull the line in as a string and parse the number out of it.
I'm not really crazy about system("clear"). It feels invasive. If you're committed to this sort of interface, I'd go all in with curses. Or just keep it simple and keep printing without clearing.
Avoid convoluted logic
In checkForWin, the draw logic is somewhat hard to follow:
//Check for draw
for (int j = 1; j < 10; ++j) {
    if (places[j] - 48 != j)
        draw++;
}
if (draw == 9){
    printf("\t  Draw!\n");
    return 1;
}

Once again, the -48 is an artifact of user input conversion that really doesn't belong in the game engine. Instead of a player variable and this manual draw check logic, most two-player board games use a single number, ply, which counts the turn. A draw check then becomes ply >= length of board assuming is_won is called first, figuring out whose turn it is becomes ply % 2 and switching sides is ply++.
Ply can be used to avoid pointless win checks if not enough moves have been played. In tic tac toe, it seems like a minor optimization but it can speed up an AI that's running win checks thousands of times per turn, and it's a single extra line of code.
Consider breaking commented code out into functions
The checkForWin function has 4 distinct parts to it: checking horizontals, verticals, diagonals and draws. Each could be a separate function instead of delimiting the areas with comments. Otherwise, some of these loops could be merged and the logic simplified (it's debatable which is best).
Code style

Keep your braces consistent: void switchPlayers(){ should be void switchPlayers() {.

Use #include <stdbool.h>:
  if (draw == 9){
      printf("\t  Draw!\n");
      return 1;
  }

could then be
  if (draw == 9){
      printf("\t  Draw!\n");
      return true;
  }

which is easier for the programmer to understand.

Possible rewrite
While I'd prefer to use a bitboard and bitmasks to check win patterns, I think it's most instructive to keep the array format to avoid too radical of a departure from your design.
Feel free to accuse this code of being prematurely future-proof or anticipating adding AI and other features. Fair enough--it is more code. While I've gone somewhat deep into generalizing board sizes and so forth, you can cherry-pick the techniques that make sense for you and take the rest with a grain of salt.
Future steps might be adding a GUI, generalizing the board sizes with malloc or a FAM, adding an AI, adding a variant or network play.
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>

struct TicTacToePosition {
    uint8_t ply;
    uint8_t board_len;
    uint8_t side_len;
    char board[3][3];
};

struct TicTacToePosition ttt_init() {
    struct TicTacToePosition ttt_pos = {};
    ttt_pos.board_len = sizeof ttt_pos.board;
    ttt_pos.side_len = sizeof ttt_pos.board[0];
    return ttt_pos;
}

char ttt_current_player(const struct TicTacToePosition *pos) {
    return pos->ply % 2 ? 'O' : 'X';
}

char ttt_last_player(const struct TicTacToePosition *pos) {
    return pos->ply % 2 ? 'X' : 'O';
}

bool ttt_is_board_full(const struct TicTacToePosition *pos) {
    return pos->ply >= pos->board_len;
}

bool ttt_legal_move(const struct TicTacToePosition *pos, int row, int col) {
    return row >= 0 && row < pos->side_len &&
           col >= 0 && col < pos->side_len && !pos->board[row][col];
}

bool ttt_try_move(struct TicTacToePosition *pos, int row, int col) {
    if (!ttt_legal_move(pos, row, col)) {
        return false;
    }

    pos->board[row][col] = ttt_current_player(pos);
    pos->ply++;
    return true;
}

bool ttt_line_win(const unsigned int len, const char *arr) {
    for (int i = 1; i < len; i++) {
        if (!arr[0] || !arr[i] || arr[0] != arr[i]) {
            return false;
        }
    }

    return true;
}

bool ttt_is_won(const struct TicTacToePosition *pos) {
    if (pos->ply < 5) return false;

    const uint8_t len = pos->side_len;
    char left_diag[len];
    char right_diag[len];

    for (int i = 0; i < len; i++) {
        char column[len];
        left_diag[i] = pos->board[i][i];
        right_diag[i] = pos->board[i][len-i-1];

        for (int j = 0; j < len; j++) {
            column[j] = pos->board[j][i];
        }

        if (ttt_line_win(len, pos->board[i]) || ttt_line_win(len, column)) {
            return true;
        }
    }

    return ttt_line_win(len, left_diag) || ttt_line_win(len, right_diag);
}

char ttt_fmt_square(const struct TicTacToePosition *pos, int i, int j) {
    return pos->board[i][j] ? pos->board[i][j] : i * pos->side_len + j + '1';
}

void ttt_print_board(const struct TicTacToePosition *pos) {
    puts("");

    for (int i = 0; i < pos->side_len; i++) {
        for (int j = 0; j < pos->side_len - 1; j++) {
            printf("   |");
        }

        printf("\n %c ", ttt_fmt_square(pos, i, 0));

        for (int j = 1; j < pos->side_len; j++) {
            printf("| %c ", ttt_fmt_square(pos, i, j));
        }

        if (i < pos->side_len - 1) {
            printf("\n___");

            for (int j = 1; j < pos->side_len; j++) {
                printf("|___");
            }
        }

        puts("");
    }

    for (int i = 0; i < pos->side_len - 1; i++) {
        printf("   |");
    }

    puts("\n");
}

int ttt_get_num(const char *failure_prompt) {
    for (;;) {
        int result;
        char buf[128];
        fgets(buf, sizeof buf, stdin);

        if (sscanf(buf, "%d", &result)) {
            return result;
        }

        printf("%s", failure_prompt);
    }
}

void ttt_get_move(struct TicTacToePosition *ttt_pos) {
    for (;;) {
        printf("Choose a square for %c's move: ",
               ttt_current_player(ttt_pos));
        int move = ttt_get_num("Invalid input. Try again: ") - 1;
        int row = move / ttt_pos->side_len;
        int col = move % ttt_pos->side_len;

        if (ttt_try_move(ttt_pos, row, col)) {
            break;
        }

        puts("Invalid move. Pick an empty square between 1 and 9.");
    }
}

void ttt_play_game() {
    for (struct TicTacToePosition ttt_pos = ttt_init();;) {
        ttt_print_board(&ttt_pos);
        ttt_get_move(&ttt_pos);

        if (ttt_is_won(&ttt_pos)) {
            ttt_print_board(&ttt_pos);
            printf("%c won!\n", ttt_last_player(&ttt_pos));
            break;
        }
        else if (ttt_is_board_full(&ttt_pos)) {
            ttt_print_board(&ttt_pos);
            puts("The game is a draw");
            break;
        }
    }
}

int main() {
    ttt_play_game();
    return 0;
}


Finding a Protein Motif I've solved the meaty bits of this bioinformatics problem, albeit a bit clumsily I think. In particular I've messed around with manipulating the fasta info into a form Biopython's SeqIO.parse() will accept and it screams to be optimized.

So there is my question to you. How can I better save the HTTP response from Uniprot into something that is easily parsed by SeqIO.parse()?

'''
Stephen Wist
to solve this problem: http://rosalind.info/problems/mprt/
'''

import sys, re # IO and motif finding
import urllib3 # get protein fasta from uniprot
from Bio import SeqIO, Seq # fasta parsing and manipulation

if len(sys.argv)<2:
    print("need at least one uniprotID")

f = open("ros.fa", "w")
http = urllib3.PoolManager() # urllib3 takes care of stuff here

# get fasta of all given uniprotIDs
for item in sys.argv[1:]:
    url = "http://www.uniprot.org/uniprot/" + item + ".fasta"
    req = http.request("GET", url)
    byte_string = req.data
    string = byte_string.decode("utf-8")
    f.write(string) # BioPython SeqIO.parse() won't work on the
                    # decoded byte_string

print(  "\nreq.data:\n", req.data,
        "\ndecoded req.data:\n", string)

f.close()
pattern = re.compile("[N][^P][S|T][^P]")
for seq in SeqIO.parse("ros.fa", "fasta"):
# keeping this here, it might be useful
# [(m.start(0), m.end(0)) for m in re.finditer(pattern, string)]
    for match in re.finditer(pattern, str(seq.seq)):
        print(match.start(0) + 1)

When cracking argv, you have an opportunity to use argparse if you wish.

Please bury all this code under def main():, rather than creating lots of top-level globals like f.

Rather than f = open() ... f.close(), you might phrase it this way:

with open('ros.fa', 'w') as f:
    ...

There are some comments you might elide. The identifier string is accurate, but a bit vague. Might be better to elide the temp variables:

f.write(req.data.decode('utf-8'))

Overall, this code looks like it gets the job done.


Click through a table of hyperlinks My goal was to click every single hyperlink under the "Shortcuts" header on here.

The code looks like it works to me, but if you have ideas that could help me improve/change I would love to hear them! Please do not be to critical of my variable naming.

#   Author: Brandon Finley
#   Date: 5/17/2017
#   Purpose: Clicks on every link of the selenium2library shortcuts by counting how many
#            elements it has in the table.
*** Settings ***
Library     Selenium2Library
Suite Setup   Open Browser   http://robotframework.org/Selenium2Library/Selenium2Library.html   chrome

*** Variables ***
${SHORTCUTS} =  dom=document.getElementsByClassName("shortcuts")[0].childNodes

*** Test Cases ***
Testing
    #   First half of the shortcuts table has shortcuts-container xpath name
    ${Cuts-Container} =  get matching xpath count  //*[@id="shortcuts-container"]/div/a
    #   Second half of the shortcuts table has keywords-container xpath name
    ${Key-Container} =  get matching xpath count  //*[@id="keywords-container"]/table/tbody/tr
    #   Count is the addition of both xpath names integer
    ${COUNT} =  evaluate  ${Cuts-Container}+${Key-Container}

    #   Simple for loop that will only click on every odd element because every even
    #   element is a non-clickable symbol used to make the shortcuts table look pretty
    : FOR   ${INPUT}    IN RANGE    0   ${COUNT}
    \   ${ODD?} =    evaluate  ${INPUT}%2
    \   Only Click Odd  ${ODD?}   ${INPUT}
    \   sleep   .2s

*** Keywords ***
Only Click Odd
    [Arguments]  ${EvenOrOdd}    ${Counter}
    run keyword if  ${EvenOrOdd}==1  click element  ${SHORTCUTS}[${Counter}]

Given one of the main highlights of Robot Framework is readability, I'll show my suggestions to improve that first and then go into my mechanical suggestions.

First, good readability practice in Robot Framework is to line up your cells by tabs (or sets of four characters depending on your IDE) and capitalize each word of your keyword uses. So, your code as sent would look like this.

*** Settings ***
Library         Selenium2Library
Suite Setup     Open Browser    http://robotframework.org/Selenium2Library/Selenium2Library.html    chrome

*** Variables ***
${SHORTCUTS} =  dom=document.getElementsByClassName("shortcuts")[0].childNodes

*** Test Cases ***
Testing
    #   First half of the shortcuts table has shortcuts-container xpath name
    ${Cuts-Container} =     Get Matching Xpath Count    //*[@id="shortcuts-container"]/div/a
    #   Second half of the shortcuts table has keywords-container xpath name
    ${Key-Container} =      Get Matching Xpath Count    //*[@id="keywords-container"]/table/tbody/tr
    #   Count is the addition of both xpath names integer
    ${COUNT} =              Evaluate    ${Cuts-Container}+${Key-Container}

    #   Simple for loop that will only click on every odd element because every even
    #   Element is a non-clickable symbol used to make the shortcuts table look pretty
    : FOR   ${INPUT}    IN RANGE    0   ${COUNT}
    \   ${ODD?} =   Evaluate    ${INPUT}%2
    \   Only Click Odd  ${ODD?}     ${INPUT}
    \   Sleep   .2s

*** Keywords ***
Only Click Odd
    [Arguments]     ${EvenOrOdd}    ${Counter}
    Run Keyword If  ${EvenOrOdd}==1     Click Element   ${SHORTCUTS}[${Counter}]

I'm not trying to be too critical of your variable naming but I do have to be for completionism and for other future Robot Framework users looking for how to write readable code. Because (it appears) that you're using the Python version of Robot Framework, I'd say it's good practice to keep to the Python convention of naming variables, which is lower_case_with_underscores for local variables or UPPER_CASE_WITH_UNDERSCORES for global variables and index variables in loops. Also, shorter, more descriptive variable names are perfectly acceptable for this size of program. Re-written for that, your variables should be ${shortcuts}, ${keywords}, ${count}, ${odd}, ${even_or_odd}, and ${counter}.

Mechanically, I'm assuming it works fine as-written, but there's a few optimization problems. First, having a custom keyword for a single action is usually redundant (unless you want to simplify a repeated action with a lot of parameters to a single word for readability), so you could re-write your For loop as follows (using the re-formatted variables):

: FOR   ${INPUT}    IN RANGE    0   ${count}
    \   ${odd} =   Evaluate    ${INPUT}%2
    \   Run Keyword If  ${odd}==1     Click Element   ${SHORTCUTS}[${INPUT}]
    \   Sleep   .2s

That makes the keyword unnecessary, making your code easier to maintain.

Finally, as another good practice, Selenium2Library is outdated. You'll need to download it separately, but ExtendedSelenium2Library is more robust, contains more keywords, and includes code to deal with Angular applications.

The final code after formatting and optimization looks like this (I also took some liberties on comments that are either unnecessary or too wordy for what they describe):

*** Settings ***
Library         ExtendedSelenium2Library
Suite Setup     Open Browser    http://robotframework.org/Selenium2Library/Selenium2Library.html    chrome

*** Variables ***
${SHORTCUTS} =  dom=document.getElementsByClassName("shortcuts")[0].childNodes

*** Test Cases ***
Testing
    #   Count the number of shortcuts and keywords
    ${shortcuts} =  Get Matching Xpath Count    //*[@id="shortcuts-container"]/div/a
    ${keywords} =   Get Matching Xpath Count    //*[@id="keywords-container"]/table/tbody/tr
    ${count} =      Evaluate    ${shortcuts}+${keywords}

    #   Click every odd element (evens are unclickable)
    : FOR   ${INPUT}    IN RANGE    0   ${count}
        \   ${odd} =        Evaluate    ${INPUT}%2
        \   Run Keyword If  ${odd}==1   Click Element   ${SHORTCUTS}[${INPUT}]
        \   Sleep           .2s

In other notes, this is actually a very good first Robot Framework script that is very optimal (as far as I can tell) other than my suggestions.

Also, I completely missed who wrote this code originally when I was writing up this review. I know this guy IRL. Hey Fin! Hope this explained how I get my code to look so nice.


This JavaScript implementation of range is fast. What are it's downsides? This implementation of range() is very fast:

RANGE = []; for (var i=0; i<65536; ++i) RANGE.push(i-32768);
range = function(a,b){ return RANGE.slice(a+32768,b+32768); };

Are there downsides in using this approach?

relatively speaking-

1) its a bit heavy on the memory use, requiring the memory for a property + mem of a number for each integer in the possible range

2) always consumes the memory and cpu needed to intialize, even if the script never has use for it.


Filtering or repeating elements as needed I'm having trouble thinking of a good name for the following extension method:

public static IEnumerable<T> TrimOrExpand<T>(this T[] items, int desiredCount)
{
    var ratio = (double)items.Length / desiredCount;
    for (int i = 0; i < desiredCount; i++)
        yield return items[Convert.ToInt32(i * ratio)];
}

The thing I don't like about this method name is that "Trim" may communicate to the user that it's cutting off the last x records of the array, when actually it is excluding items at a regular interval throughout the array. Likewise it will also "Expand" the array by repeating items if you specify a count that is larger than items.Length.

Is there a better technical term for what I'm trying to do? Or is TrimOrExpand obvious enough to understand what it does?

I think if you look at the objects within the IEnumerable as a statistical numeric value than you could use the term Normalize. In this case you are "normalizing" your values (objects) to a desired count.

public static IEnumerable<T> NormalizeObjects<T>(this T[] items, int desiredCount)

If you dislike the term Objects you could replace it with Items or even Records depending on what your use case is.

If the only thing you dislike about your current name is "Trim" then you could replace that term with the opposite of Expand, which I would argue is Contract.

public static IEnumerable<T> ExpandOrContract<T>(this T[] items, int desiredCount)

I think either of the above are decent options based on what your method is currently doing.


Check if object contains only empty nested objects lodash.isEmpty([{}]) returns false even though the object contained in the array is empty.

I need something that considers these kind of nested objects empty too and wrote the below recursive function. It looks a bit ugly and doesn't use any JavaScript specific tricks. Any comments welcome.

export function isDeepEmpty(input) {
  // catch null or undefined object
  if (!input) return true
  if (typeof(input) === 'object') {
    for (const entry of Object.values(input)) {
      if (!isDeepEmpty(entry)) {
        return false
      }
    }
    return true
  }
  // if input is not an object return false
  return false
}

Some issues?

typeof is not a function. eg typeof(input) === 'object' is written typeof input === 'object'
null is an Object so when you test for an object using typeof you MUST make sure it is not null as well.
The test that you comment as // catch null or undefined object will catch any value that evaluates to a falsey, such as false, 0, ""

What is empty?

As it is unclear what you define as empty so I will make some assumptions, they are arbitrary and may or may not fit your needs.

An array is empty if it contains only empty items.
An object is empty if it is null or contains no own properties. If it has properties that are null or (defined as) undefined it is not empty.
A string is empty if it contains no characters
Empty items are null, undefined, {}, [], and ""

Examples

Empty

[],
{},
[undefined],
[null],
[[], [], []],
[{},,[],[[[null],[undefined]],[,,,,,]],
new Array(10),
null,
undefined,
""

Not empty

[0]
[{A:null}],
{A:undefined},
[,,,0],
[[], [], [1]],
[{},,[],[[[1],[]],[]],
(new Array(10))[1] = 0,
false,
true,
"  ",

Rewrite

With the above assumptions you can rewrite the code as a two functions.

As a non empty object mean we return false, and thus we do not need to iterate its values.

The entry point is isItemEmpty you would call it as you did isDeepEmpty

const isObjEmpty = obj => obj === null || Object.keys(obj).length === 0;
const isItemEmpty = item => item === undefined || item === "" ||
    (Array.isArray(item) && array.every(isItemEmpty)) ||
    (typeof item === "object" && isObjEmpty(item));

Usage

isItemEmpty([{},[],[[]]]);    // returns true
isItemEmpty([{A:0},[],[[]]]); // returns false
isItemEmpty("");              // returns true


A "C" shape of 7 boxes that turn green on click Company name asks you to build a website with 7 boxes shaped like a c, which should turn green on click, and then when all are clicked, should turn red in reverse clicking order.

https://jsfiddle.net/yx296/dvacLduw/

document.addEventListener('DOMContentLoaded', function() {
  var boxHolder = document.querySelector(".boxHolder");
  var count = 0;

  boxHolder.addEventListener('click', changeGreen);

  var boxes = [];

  function changeGreen(e) {
    if (e.target !== e.currentTarget) {
      var box = e.target;
      if (box.style.backgroundColor !== 'green') {
        box.style.backgroundColor = 'green';
        count++;
        boxes.push(e.target);
      }
    }

    if (count === 7) {
      count = 0;
      setTimeout(function() {
        reverseTurnRed();
      }, 1000);
    }
    e.stopPropagation();
  }

  function reverseTurnRed() {
    boxes.reverse();
    boxes.forEach(function(box, index) {
      setTimeout(function() {
        box.style.backgroundColor = 'red';
      }, index * 500)
    });
    setTimeout(function() {
      boxes = [];
    }, 0)
  }
});
div {
  box-sizing: border-box;
}

.container {
  font-size: 0;
  margin: 0 auto;
  width: 400px;
  height: 400px;
  border: 1px solid blue;
  position: relative;
}

.boxHolder {
  border: 1px solid black;
  display: inline-block;
  position: absolute;
  top: 50%;
  left: 50%;
  transform: translate(-50%, -50%);
}

.box {
  width: 50px;
  height: 50px;
  border: 1px solid black;
  margin: 2px;
}

.topRow {
  display: inline-block;
}

.bottomRow {
  display: inline-block;
}
<!DOCTYPE html>

  <meta charset="UTF-8">
  <title>vanillaJSAddeparChallenge</title>
  <link rel="stylesheet" href="styles.css">

It seems it doesn't matter whether or not I leave the e.stopPropagation in there... what is the point of it?

Willing to accept any critique on HTML, CSS or JavaScript.

This has "interview question" all over it :D Anyways.

First, I'd avoid inline styles. The most common issue with inline styles is that they are hard to override. Only !important can override them from a stylesheet, aside from changing the inline style values themselves. If you carry over the habit to building large apps, CSS maintenance becomes a hell lot worse.

Additionally, styles in JS is hard to maintain. In a large code base, looking for which JS changed some style is hard. You don't know where it happened and the debugger won't tell you immediately without putting DOM breakpoints, which is annoying when a lot of changes take place.

As an alternative, I suggest you define CSS classes corresponding to the state of the box and programmatically add/remove classes. I would suggest reading about BEM, especially modifiers (the "M"). We can use modifiers as "state".

// Base box style
.c-box__box{
  color: transparent
}

// During the "selected" state, you add this class.
.c-box__box--selected{
  color: green
}

// During the "unselected" state, add this class.
.c-box__box--unselected{
  color: red
}

In the sample CSS above, a box would initially be transparent. Clicking the box, you make the box turn to a "selected" state. Thus you add .c-box__box--selected. When you unselect, you make the box turn to an "unselected" state. You remove the .c-box__box--selected and add .c-box__box--unselected. Since styles are defined accordingly in CSS, it will color the box appropriately. Now JS has no idea of styling, only classes pertaining the state of the boxes.

e.stopPropagation() stops the bubbling process, and prevents the event from going up the ancestors and firing events that are attached there. In this case, you don't need it as you are not interested in stopping some event handler in some ancestor from firing.

In your code, you're manually counting your elements (I see a hard 7 there). An alternative would be to use querySelectorAll to get each box and attach a handler. When they get clicked, they get stored in another array. When that array's length matches the length from querySelectorAll, then  you can say all your boxes have been selected and you can start your unwinding process. A reverse operation would simply be a pop of the array of selected boxes. This way, you can add an arbitrary amount of boxes without worrying about a hard-coded limit.

Also, instead of a recursive setTimeout, you can use a more loop-like setInterval. When you reach empty, you simply stop the interval from running. Also note that timers are async. One can easily do something between the unwinding process, like say click on a red box. I suggest you put a flag to tell the handler to not proceed while it's unwinding.

Here's a demo (warning: I wrote it using SCSS and ES6): http://codepen.io/anon/pen/JGRzVe

var boxesNodeList = document.querySelectorAll(".c-box__box");
var boxesArray = Array.prototype.slice.call(boxesNodeList);
var selectedBoxes = [];
var isUnwinding = false;

boxesArray.forEach(function(box) {

  box.addEventListener('click', function() {
    if (isUnwinding) return;

    box.classList.remove('c-box__box--unselected');
    box.classList.add('c-box__box--selected');
    selectedBoxes.push(this);

    if (selectedBoxes.length === boxesArray.length) {
      isUnwinding = true;
      var timer = setInterval(function() {
        var box = selectedBoxes.pop();
        box.classList.remove('c-box__box--selected');
        box.classList.add('c-box__box--unselected');
        if (!selectedBoxes.length) {
          clearInterval(timer);
          isUnwinding = false;
        }
      }, 500);
    }
  });
});
.c-box {
  display: table;
  border: 1px solid blue;
  padding: 100px;
  overflow: hidden;
}
// Base box style
.c-box__box {
  float: left;
  border: 1px solid #000;
  width: 50px;
  height: 50px;
  margin: 2px;
  // Every 3n+1, break to a new line
  &: nth-child(3n + 1) {
    clear: both;
  }
}
// Just a transparent box
.c-box__spacer {
  display: inline-block;
  border: transparent;
  width: 50px;
  height: 50px;
  margin: 2px;
}
// Selected state style
.c-box__box--selected {
  background: green;
}
// Unselected state style
.c-box__box--unselected {
  background: red;
}


node.js + Express code critique I'm learning Node.  I'm working on a personal project that uses Node + Express + Node-MySQL + Generic-Pool (for MySQL pooling).  I am new to the idea of module.exports and I'm still grokking it.

The following code works.  However I want to organize my code in a way that is logical and optimal, and MVCish.  I am wondering if those of you who have done this before could take a look at the following and let me know if this is the "right" way to structure things.  (Yes I know this going to depend a lot on the developer.  But at least knowing if there are obvious problems in my approach would be helpful.)

My folder structure looks roughly like this:

app.js
routes/
models/
base/

In my main app.js file I have...

var express = require( "express" ),
    mysql = require( "mysql" ),
    generic_pool = require ( "generic-pool" ),
    routes = require( "./routes" );

var app = module.exports = express(),
    pool = generic_pool.Pool({
        name: "mysql",
        create: function( cb ) {
            var db = mysql.createConnection( require( "./config/db" ) );
            db.connect();
            cb( null, db );
        },
        destroy: function( db ) {
            db.end();
        },
    });

app.configure(function(){
    app.set( "pool", pool );
    ...
});
routes( app );
app.listen(80, function());

Here is an example route/ file "stuff.js"

module.exports = function( app ){
    var model = require( "../models/stuff" );
    model.init( app );
    app.get( "/stuff", function( req, res ){
        model.get( function( result ){
            res.json( result );
        });
    });
};

Here is an example model/ file "stuff.js"

var db;
module.exports.init = function( app ){
    db = require( "../base/db" );
    db.init( app );
};
module.exports.get = function( cb ){
    var sql = "SELECT * FROM stuff";
    db.do( sql, false, cb );
};

And here is the base/db file

var pool;
module.exports.init = function( app ){
    pool = app.get( "pool" );
};
module.exports.do = function( sql, params, cb ){
    pool.acquire(function( err, db ){
        if (err) {
            // ...
        } else {
            db.query( sql, function( err, result ) {
                if ( err ) {
                    // ...
                } else if ( typeof cb === "function" ) {
                    cb( result );
                }
            });
        }
        pool.release( "db" );
    });
};

Your code looks fine.

However, you are not showing us much to review, definitely come back when there is more to review.

Some minor comments:

Having your db connection config through ./config/db is good
You probably want to throw an exception if ( typeof cb !== "function" ) instead of silently ignoring that
The base/db code nests 4 levels of callback, you should look into queue management libraries for node.js


Pausing and resuming a concurrent database update queue I have a class DatabaseQueue which asynchronously runs SQLite updates that it takes from a LinkedBlockingQueue.

I implemented a pausing mechanism for this class which should ideally stop the execution of the queue as soon as possible and then wait for it to be unpaused before executing more queries.

I'm very new to concurrent programming but I managed to get this working using multiple ReentrantLock's and Condition's. This seems very verbose and clumsy though, especially with all the lock()'s and unlock()'s.

In addition to this, I also don't like how I have to execute an additional dummy query just to get the executor thread to continue its loop. What would be a better way to implement this?

import java.sql.Connection;
import java.sql.SQLException;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;

public class DatabaseQueue {

    private static final LinkedBlockingQueue<String> queue = new LinkedBlockingQueue<>();
    private static final QueueExecutor executor = new QueueExecutor();

    private static boolean locked = false;
    private static ReentrantLock lockAttempted = new ReentrantLock();
    private static ReentrantLock lockAcquired = new ReentrantLock();
    private static Condition attempted = lockAttempted.newCondition();
    private static Condition acquired = lockAcquired.newCondition();

    public static void start() {
        if (locked) {
            locked = false;
            lockAttempted.lock();
            attempted.signal(); //Signal to resume queue execution
            lockAttempted.unlock();
        } else {
            executor.start();
        }
    }

    public static void pause() {
        locked = true;
        if (queue.isEmpty()) {
            queue.add("SELECT 1");
        }
        lockAcquired.lock();
        acquired.awaitUninterruptibly(); //Wait for the lock to be acquired
        lockAcquired.unlock();
    }

    public static void queue(String update) {
        queue.add(update);
    }

    public static class QueueExecutor extends Thread {

        public QueueExecutor() {
            setName("Database Queue Executor");
            setDaemon(true);
        }

        @Override
        public void run() {
            Connection connection = Database.getConnection();
            while (true) {
                try {
                    if (locked) {
                        lockAcquired.lock();
                        acquired.signal(); //Signal that the lock has been acquired
                        lockAcquired.unlock();
                        lockAttempted.lock();
                        attempted.awaitUninterruptibly(); //Wait for the lock to be released
                        lockAttempted.unlock();
                    }
                    String update = queue.take();
                    connection.createStatement().executeUpdate(update);
                    Database.update(update);
                } catch (InterruptedException | SQLException e) {
                    e.printStackTrace();
                }
            }
        }

    }

}

Why are all your variables static? You shouldn't use them.
You don't need that many locks. Actually, you need only 1 and a AtomicBoolean (locked).
I can't understand why you add "SELECT 1" if the queue is empty. That'll  waste some precious time.
I think as long as there's only 1 QueueExecutor you don't need to make the queue thread-safe.
You could execute your statements in batches but I'm not sure of the pros and cons.
Unless you 100% trust the user you should use PreparedStatements but even if you do trust the user you still should use it as it will give you a speed (i.e. query execution) boost.
Wouldn't it be better to pass Statements to queue instead of String? It would allow you to easily allow the user to use PreparedStatements

Example implementation (not tested):

import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.util.ArrayDeque;
import java.util.Deque;
import java.util.concurrent.atomic.AtomicBoolean;

public class DatabaseQueue {

    private final Deque<PreparedStatement> queue = new ArrayDeque<>();
    private final QueueExecutor executor = new QueueExecutor();
    private final AtomicBoolean paused = new AtomicBoolean(false);
    private final Object object = new Object();

    public DatabaseQueue() {
        executor.start();
    }

    public void start() {
        if (!paused.compareAndSet(true, false)) {
            throw new IllegalStateException("DatabaseQueue wasn't paused");
        }

        executor.notify();
    }

    public void pause() {
        if (!paused.compareAndSet(false, true)) {
            throw new IllegalStateException("DatabaseQueue is already paused");
        }
    }

    public void close() {
        executor.shutdown();

        paused.set(false); // we want to execute all the statements before closing
        executor.notify();
    }

    public void queue(PreparedStatement statement) {
        queue.addLast(statement);

        if (!paused.get())
            executor.notify();
    }

    public PreparedStatement createStatement(String sql) throws SQLException {
        return Database.getConnection().prepareStatement(sql);
    }

    class QueueExecutor extends Thread {
        private boolean shutdown = false;

        QueueExecutor() {
            setName("Database Queue Executor");
            setDaemon(true);
        }

        void shutdown() {
            shutdown = true;
        }

        @Override
        public void run() {
            // even if we should shutdown we first should execute all the statements
            do {
                if (paused.get() || queue.isEmpty()) {

                    try {
                        this.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }

                try {
                    PreparedStatement statement = queue.removeFirst();

                    if (!paused.get())
                        statement.execute();
                    else
                        queue.addFirst(statement);
                } catch (SQLException e) {
                    e.printStackTrace();
                }
            } while (!shutdown || !queue.isEmpty());
        }
    }
}


Creating 28 checkboxes with a common name and different values There are 28 check boxes that are given a common name and have different values:

<input type="checkbox" name="Schedules" id="checkbox1" value="1">

In controller in post action:

public ActionResult Create(SubscriptionPlanCreateViewModel subscriptionPlanCreateViewModel, FormCollection Form)
{
    string[] AllStrings = Form["Schedules"].Split(',');
    foreach (string item in AllStrings)
    {
        int value = int.Parse(item);
        if (value == 1)
        {
            subscriptionPlanCreateViewModel.Timeschedule = "OneTime";
        }
    }
}

By using this code I need to use 28 if conditions. How can I reduce the code size?

It seems to me that what you really need to do is change the view code. For example, if you do this:

<input type="checkbox" name="Timeschedule" id="checkbox1" value="OneTime">
<input type="checkbox" name="Timeschedule" id="checkbox2" value="Weekly">

That will automatically bind to the Timeschedule property on your SubscriptionPlanCreateViewModel. However, if the user selects multiple checkboxes, the values will probably come across as comma-separated. You can either make Timeschedule an array (which would probably better represent what you're doing from a data perspective) or you can simply replace the commas with " | " to produce the string you say you want in your comments.