MP3 player for Linux I wrote a Python MP3 player for Linux using PyGame, with a curses GUI and a mouse-only interface.

It's purely for personal use.

It hasn't been polished yet, but for now I'd like to know what I've done inefficiently so I can fill in some ignorance-holes. I know a few things could be handled more directly.

import curses
import pygame

pygame.mixer.init(frequency=22050, size=-16, channels=2, buffer=4096)
player = pygame.mixer.music
song = "testshort.mp3"
player.load(song)
player.play()
screen = curses.initscr()
curses.noecho()
curses.mousemask(curses.ALL_MOUSE_EVENTS)
screen.keypad(1)
curses.curs_set(0)
screen.resize(6, 30)
screen.border(0)
screen.addstr(4, 19, "*"*5+"-"*5)
player.set_volume(0.5)
screen.addstr(4, 2, "|")
paused = 0
curses.halfdelay(1)

while True:
            busycheck = player.get_busy()
            if busycheck == 0:
                screen.addstr(4, 2, ">")
            char = screen.getch()
            if char == curses.KEY_MOUSE:
                cords = list(curses.getmouse())
                mouse_x = cords[1]
                mouse_y = cords[2]
                vol_on = mouse_x - 18
                vol_off = 10 - vol_on
                screen.border(0)
                if mouse_y == 4:
                    if 29 > mouse_x > 18:
                        vol_bar = screen.addstr(4, 19, "*"*vol_on+"-"*vol_off)
                        volchange = (mouse_x-19)/9.0
                        player.set_volume(volchange)
                    elif mouse_x == 2:
                        if paused == 0:
                            if busycheck == 1:
                                screen.addstr(4, 2, ">")
                                player.pause()
                                paused = 1
                            else:
                                player.play()
                                paused = 0
                                screen.addstr(4, 2, "|")
                        else:
                            player.unpause()
                            screen.addstr(4, 2, "|")
                            paused = 0

import curses
import pygame

pygame.mixer.init(frequency=22050, size=-16, channels=2, buffer=4096)
player = pygame.mixer.music

When giving modules a shorter name, the usual method is from pygame.mixer import music as player It makes it a little clearer what you are doing.

song = "testshort.mp3"
player.load(song)
player.play()

For anything but a quick script, its recommended that you put your code inside a main function.

screen = curses.initscr()

I recommend that you take a look at the curses.wrapper function. It'll take care of closing curses correctly in the case of an error.

curses.noecho()
curses.mousemask(curses.ALL_MOUSE_EVENTS)
screen.keypad(1)
curses.curs_set(0)
screen.resize(6, 30)
screen.border(0)
screen.addstr(4, 19, "*"*5+"-"*5)
player.set_volume(0.5)
screen.addstr(4, 2, "|")

Your code jumps between curses settings, outputing to the screen, and setting up the player. I recommend dividing it cleanly between these three different tasks.

paused = 0
curses.halfdelay(1)

while True:
            busycheck = player.get_busy()
            if busycheck == 0:

use if not player.get_busy(). Using == to check in the case of bools is considered bad practice. And there is no reason to split across two lines here

                screen.addstr(4, 2, ">")
            char = screen.getch()
            if char == curses.KEY_MOUSE:
                cords = list(curses.getmouse())

There is no reason to call list here.

                mouse_x = cords[1]
                mouse_y = cords[2]

I suggest mouse_x, mouse_y = coords[:2]

                vol_on = mouse_x - 18
                vol_off = 10 - vol_on
                screen.border(0)

Why are you calling screen.border here?

                if mouse_y == 4:
                    if 29 > mouse_x > 18:
                        vol_bar = screen.addstr(4, 19, "*"*vol_on+"-"*vol_off)

You are basically repeating what you had previously. I suggest refactoring so as to share code.

                        volchange = (mouse_x-19)/9.0
                        player.set_volume(volchange)

See how you are effectively changing the same piece of information three times? This suggests you should put all of that in a function.

                    elif mouse_x == 2:
                        if paused == 0:
                            if busycheck == 1:
                                screen.addstr(4, 2, ">")
                                player.pause()
                                paused = 1
                            else:
                                player.play()
                                paused = 0
                                screen.addstr(4, 2, "|")
                        else:
                            player.unpause()
                            screen.addstr(4, 2, "|")
                            paused = 0

Again, your code tangles together in a seeming haphazard manner the different things it does.

This is how I'd do it:

import curses
import pygame
from pygame.mixer.music import music

SONG = "testshort.mp3"

class MusicPlayer(object):
    def __init__(self):
        pygame.mixer.init(frequency=22050, size=-16, channels=2, buffer=4096)
        music.load(SONG)
        music.play()
        self.paused = False
        self.change_volume(5)

    def change_volume(self, volume):
        self.volume = volume
        music.set_volume( volume / 9.0 )

    def toggle_pause(self):
        self.paused = not self.paused:
        if self.paused:
            music.pause()
        else:
            music.unpause()

def update_screen(screen, player):
    screen.addstr(4, 19, "*" * player.volume + "-"* (10 -player.volume))

    if music.get_busy() or player.paused:
        screen.addstr(4, 2, "|")
    else:
        screen.addstr(4, 2, ">")

def main(screen):
    curses.mousemask(curses.ALL_MOUSE_EVENTS)
    screen.keypad(1)
    curses.curs_set(0)
    screen.resize(6, 30)
    screen.border(0)
    curses.halfdelay(1)

    player = MusicPlayer()

    while True:
        update_screen(screen, player)

        char = screen.getch()
        if char == curses.KEY_MOUSE:
            mouse_x, mouse_y = curses.getmouse()[:2]
            if mouse_y == 4:
                if 29 > mouse_x > 18:
                    player.change_volume(mouse_x - 18)
            elif mouse_x == 2:
                player.toggle_pause()

if __name__ == '__main__':
    curses.wrapper(main)

See how each piece of the code worries about one small thing? We don't mix controlling the music with updating the display.


Correct GenericDAO implementation? I implemented a MVC Framework with Generic DAO and I would like know if is correct. I did not have developed yet the MySQLDAO.

My controller class

class ClienteController {
  public function __construct() {  }

  public function actionSubmit(){
      $objCliente = new Cliente();
      $objCliente->setNome($_POST['nome']);
      $objCliente->setCodigo($_POST['codigo']);
      $objCliente->getDAOFactory()->insert();
  }
}

DAO Class for client, persistence.

class ClienteDAO implements IRecord {
  const TABLE = 'cliente';

  private $campos = array(
                         "codigo" => "cliCod",
                         "nome"   => "cliNom"
                         );

  private  $DAOFactory;

  public function __construct() {
      $this->DAOFactory = new DAOFactory($this);
  }

  public function getDAOFactory() {
      return $this->DAOFactory->getInstance();
  }
}

Model class

class Cliente extends ClienteDAO {
  private $codigo;
  private $nome;

  public function __construct(){  }

  public function getCodigo() {
      return $this->codigo;
  }

  public function setCodigo( $codigo ) {
      if ( $codigo > 0 ) {
          $this->codigo = $codigo;
      }
  }

  public function getNome() {
      return $this->nome;
  }

  public function setNome( $nome ) {
      $this->nome = $nome;
  }
}

Database class

class MySQLDao implements iDAO {
    public function insert() {

    }

    public function update() {

    }

    public function delete() {

    }

    public function limit() {

    }

    public function orderBy() {

    }

    public function findAll() {

    }

    public function addColumns() {

    }

    public function addGroupBy() {

    }

    public function addOrderBy() {

    }

    public function addHaving() {

    }

    public function addCustomizedHaving() {

    }

    public function addColumSubQuery() {

    }

    public function addAnd() {

    }

    public function addOr() {

    }

    public function checkExistanceColumn() {

    }
}

Factory class

class DAOFactory {
  private $objeto;
  private $factory;

  public function __construct($obj, $banco = null) {
      $this->factory = $banco;
      $this->objeto = $obj;
  }

  public function getFactory() {
      return $this->factory;
  }

  public function setFactory($factory) {
      $this->factory = $factory;
  }

  public static function getInstance() {
      if ($this->getFactory() == null) {
          return new MySQLDAO($this->getObjeto());
      }
  }

  public function getObjeto() {
      return $this->objeto;
  }
}

Interfaces

interface IDAO {
  public function insert();
  public function update();
  public function delete();
  public function limit();
  public function orderBy();
  public function findAll();
  public function addColumns();
  public function addGroupBy();
  public function addOrderBy();
  public function addHaving();
  public function addCustomizedHaving();
  public function addColumSubQuery();
  public function addAnd();
  public function addOr();
  public function checkExistanceColumn();
}

interface IRecord {
  public function getDAOFactory();
}

Some recommendations:

Instead of creating monolithic DAOs, you should split it in domain objects and data mappers. This way you would separate the entity-specific business logic from storage.
Do not ever use active record as default solution in any code. It has very specific and limited use-case, and when you exceed those limitation, your code becomes convoluted and hackish. Active records are good for two things: fast prototyping (creating temporary throw-away code) and implementation extremely simple domain entities with almost no business logic (glorified value objects with hard-coded storage).
Look into use of PDO. It should provide you with a bit more flexible storage infrastructure when dealing with SQL databases.
You currently have domain business logic from model layer leaking in the controller. This is kinda bad. Controller should only change the state of model layer. It should not know how to create and save entities. You might benefit from reading this answer.
Watch every lecture, that is listed here. Your code seems to suffer from tight and violations of Law of Demeter. You won't be able to understand everything, that is talked about, at first (at least , I didn't). If that happens, watch the listing till the end ad start again. Each time you will understand more of it.


Lazy singleton with BackboneJS I am trying to have a Backbone.Model which is a singleton and I wish to instantiate it lazily. I only need to support modern browsers so please do not worry about the use of ES5 getter.

Is this the simplest way of achieving my goal?

var LazySingleton = function(){
    var _LazySingleton = Backbone.Model.extend({
        defaults: {
            foo: 0
        },
        initialize: function(){
            console.log('I am initializing');
        }
    });

    return {
        _instance: null,
        get instance() {
            if (this._instance === null) {
                this._instance = new _LazySingleton();
            }

            return this._instance;
        }
    };
}();

console.log('LazySingleton has been declared, but not initialized');
LazySingleton.instance.set('foo', 3);
console.log('foo should be 3:', LazySingleton.instance.get('foo'));

This is not the simplest way.

There seems to be no point in instance, if there can be only 1 instance, why have it all ?
Why have initialize at all ?
Why set foo to 0, I would set it undefined
If I were to keep the instance concept, I would write this:

_instance: null,
get instance() {
    if (this._instance === null) {
        this._instance = new _LazySingleton();
    }

    return this._instance;
}

as

_instance: null,
get instance() {
    return this._instance = this._instance || new _LazySingleton();
}

I would probably go for something like this ( I might have the syntax wrong )

var LazySingleton = Backbone.Model.extend({
    defaults: {
        foo: undefined
    },
    initialize: function(){
    });


Creating website via SQL table and PHP object I wanted to try keeping the content of a website (mostly) in a SQL table and have made the basic template for the body.  Set the $pageName and include this template.

class Page {
  public $content = [];
  public $heading;
}

$page = new Page();

$conn = mysqli_connect($dbhost, $dbuser, $dbpass, $dbname);
/* check connection*/
if ($conn->connect_errno) {
  die("Failed to connect with MySQL: " . $conn->connect_errno);
};

$sql = "SELECT type, heading1, content1, style FROM $table WHERE page = '$pageName' ORDER BY ord";
$result = mysqli_query($conn, $sql);

while($row = mysqli_fetch_array($result, MYSQL_ASSOC)) {
        switch($row['type']){
          case "content":
            $page->content[] = array("content" => '
' . $row['content1'] . '
', "heading" => "
" . $row['heading1'] . "
", "style" => $row['style']);
            break;
          case "heading":
            $page->heading = "
" . $row['heading1'] . "
";

            if ($row['content1']) {
              $page->heading .= "
" . $row['content1'] . "
";
            }
        }
    }

  mysqli_close($conn);
 ?>

  <?php echo $page->heading; ?>

<?php
for ($i=0; $i < count($page->content); $i++) {
  echo "
content[$i]["style"]."'>";
  echo $page->content[$i]["heading"];
  echo $page->content[$i]["content"];
  echo "
";
}
?>

I'm planning on adding more types than content and heading as needed.  I will also add includes such as HTML head/meta, navbar, and footer.

I just wondered what people thought of this (besides "why?").

It's simple. It looks nice enough.

But please, for the love of little Bobby Tables, bind your variables: http://www.wikihow.com/Prevent-SQL-Injection-in-PHP . That would work out to:

$stmt->bind_param("s", $pageName);

once you've created a prepared statement.

Also, consider using a numeric identifier which maps to a pageName, similar to how StackExchange maps 174740 to "creating-website-via-sql-table-and-php-object".

Wrap very long lines, please. After commas in your array() would be a good place.

You didn't show your schema. I hope you have a compound unique key on (page_name, ord). Perhaps it's your PK.

Sometimes HTML tags aren't perfectly nested, and such errors might become stored in your database. I don't know what the PHP equivalent of python BeautifulSoup is, but at the DB->web interface your loop has an opportunity to ensure only well-formed HTML is sent to clients.


Structuring a HTML5 game without a framework After trying a couple different HTML5 frameworks, I've always found pure JS the most appealing. However, I am uncertain about the way I'm structuring it.

var titlescreen = new Titlescreen();
var fps = new FPSTracker();
var input = new Input();
var background = new Background();
var shops = new Shops();
var chunkhandler = new ChunkHandler();
var explosionhandler = new ExplosionHandler();
var ui = new UI();
var world = new World();
var player = new Player();
var joystick = new Joystick();

var old_time = 0;
var dt = 0;

load();

function init(){    // called once resources loaded
    // ...
    requestAnimationFrame(loop);
}

function update(){
    player.update();
}

function draw(){
    background.draw();
    world.draw();
    shops.draw();
    player.draw();
    chunkhandler.draw();
    explosionhandler.draw();
    ui.draw();
    fps.draw();
}

function loop(time){
    dt = Math.min(0.04, (time - old_time) / 1000);

    if(!game_paused){
        update();
        draw();
    }

    old_time = time;
    requestAnimationFrame(loop);
}

Some objects listed here must communicate information between each other. For example, the Player class accesses some of world's functions by simply stating world.functionINeedToUse();, using the global variable's name. Coming from a Java background, this gives me a very bad vibe as it depends on the declaration of this global variable with the specific name. Though I've found JS to be a more forgiving language, this still does not feel right.

In order to prevent objects from depending on global variables (and in JavaScript almost every identifier is actually a variable), you can use the principle of dependency injection.

When an object needs to interact with another object, it should receive that object, either when it is initialized, through a setter or as a method parameter.

When your player requires the world, you could pass it when you create it and store it in a variable in local scope:

var player = new Player(world);

However, when your game has multiple worlds and the player moves from one to the other, it would be good to be able to change the world. In that case you could have a setter:

var player = new Player();
player.setWorld(world);

When the world is only required for a single method, you can also decide to pass the object to that method as a parameter:

player.checkCollisions(world);


Remove code duplication inside of a loop preserving performance I'm coding a 2D collision engine, and I need to merge adjacent axis-aligned bounding boxes depending on a direction (left, right, top, bottom).

The four cases are very similar, except for the if condition, and the push_back argument.
Is there any way I can refactor this code without compromising performance?

vector<AABB> getMergedAABBSLeft(vector<AABB> mSource)
{
    vector<AABB> result;

    while(!mSource.empty())
    {
        bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

        for(auto& b : mSource)
            if(a.getRight() == b.getRight())
            {
                result.push_back(getMergedAABBVertically(a, b));
                eraseRemove(mSource, b); merged = true; break;
            }

        if(!merged) result.push_back(a);
    }

    return result;
}

vector<AABB> getMergedAABBSRight(vector<AABB> mSource)
{
    vector<AABB> result;

    while(!mSource.empty())
    {
        bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

        for(auto& b : mSource)
            if(a.getLeft() == b.getLeft())
            {
                result.push_back(getMergedAABBVertically(a, b));
                eraseRemove(mSource, b); merged = true; break;
            }

        if(!merged) result.push_back(a);
    }

    return result;
}

vector<AABB> getMergedAABBSTop(vector<AABB> mSource)
{
    vector<AABB> result;

    while(!mSource.empty())
    {
        bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

        for(auto& b : mSource)
            if(a.getBottom() == b.getBottom())
            {
                result.push_back(getMergedAABBHorizontally(a, b));
                eraseRemove(mSource, b); merged = true; break;
            }

        if(!merged) result.push_back(a);
    }

    return result;
}

vector<AABB> getMergedAABBSBottom(vector<AABB> mSource)
{
    vector<AABB> result;

    while(!mSource.empty())
    {
        bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

        for(auto& b : mSource)
            if(a.getTop() == b.getTop())
            {
                result.push_back(getMergedAABBHorizontally(a, b));
                eraseRemove(mSource, b); merged = true; break;
            }

        if(!merged) result.push_back(a);
    }

    return result;
}

I find this line hard to read:

bool merged{false}; AABB a{mSource.back()}; mSource.pop_back();

Please split variables up 1 per line.
Yes the new syntax allows {} for list initialization. But these are not lists so it seems confusing to me (this one is more personal bias so feel free to ignore).

bool   merged(false);
AABB   a(mSource.back());
mSource.pop_back();

Pass large parameters by const reference if you can.
It saves a copy and you don't seem to need the copy (since you are not modifying the value (Note the eraseRemove() call has no effect externally and does not effect the rest of the code)).

vector<AABB> getMergedAABBSLeft(vector<AABB> const&   mSource)
                                     //     ^^^^^^^^

Since your four functions are identical apart from one method call you could write a generic version and pass that method as a parameter:

vector<AABB> getMergedAABBSLeft(vector<AABB> mSource)
{    return getMergedAABBSGeneric(mSource, &AABB::getRight);
}


Heapsort With Full-Scale Genericity I've written a heapsort implementation. It's generic: generic comparators and sequences with elements of a generic type.

The implementation (follows closely chapter 6 from Introduction to Algorithms 3rd Edition and) consists of

heapify: establishes the heap property for an element:

template<typename BiDirIterator, typename Cmp>
void heapify(BiDirIterator begin, BiDirIterator end,
typename std::iterator_traits<BiDirIterator>::difference_type i, Cmp cmp)
{
    const auto dist = std::distance(begin, end);

    while (true) {
        auto left = 2 * i + 1;
        auto right = left + 1;
        typename std::iterator_traits<BiDirIterator>::value_type largest;

        if (left < dist && cmp(begin[left], begin[i]))
            largest = left;
        else
            largest = i;
        if (right < dist && cmp(begin[right], begin[largest]))
            largest = right;

        if (largest == i)
            break;

        std::swap(begin[i], begin[largest]);
        i = largest;
    }
}

buildHeap: creates a heap from a permutation:

template<typename BiDirIterator, typename Cmp>
void buildHeap(BiDirIterator begin, BiDirIterator end, Cmp cmp)
{
    for (auto i = std::distance(begin, end) / 2 - 1; i >= 1; --i)
        heapify(begin, end, i, cmp);
    heapify(begin, end, 0, cmp);
}

sort: applies Heapsort to retrieve an ordered sequence from the permutation:

template<typename BiDirIterator, typename Cmp>
void sort(BiDirIterator begin, BiDirIterator end, Cmp cmp)
{
    buildHeap(begin, end, cmp);
    for (auto i = std::distance(begin, end) - 1; i > 0; --i) {
        std::swap(*begin, begin[i]);
        heapify(begin, std::next(begin, i - 1), 0, cmp);
    }
}

Necessary includes are:

#include <algorithm>
#include <iterator>
#include <functional>

In action:

#include <iostream>
#include <array>

int main()
{
    std::array<int, 10> perm { 5, 6, 9, 3, 6, 4, 8, 4, 87, 4 };

    heap::sort(perm.begin(), perm.end(), heap::max<int>{});
}

where the heap utilities are put into a separate heap namespace and heap::max is simply

template<typename T> using max = std::greater<T>;

Some questions:

Is it OK to use Cmp as a template parameter? That makes it more generic, but the error messages are way more complicated and point to stuff deep down in the implementation. (Granted, you can't say my implementation is anywhere close to "deep," but this is more of a general question.) Passing a Cmp, where Cmp is some alias for a type, would also be possible but not as generic. std::sort also uses the template parameter, though, so I do so as well.
Is my use of iterators alright? I tried to keep the interface clean with iterators for interaction with standard containers, but the implementation also uses numerical indices when it's more convenient.
heapify is ugly: there's an infinite loop with a break condition and it basically finds the largest element out of three while checking that they are within range (i.e., < dist). I have a feeling this can be beautified both syntactically and semantically. Some hint on that would be appreciated.
The implementation is supposed to work with bidirectional iterators. std::list doesn't work because subscripting is not supported, it's not efficient for std::list, etc. Should I replace subscripting with std::next to make it work on std::list at the cost of performance guarantees? std::list would work, but it might be frickin' slow asymptotically.

left = 2 * i + 1; for a non-random access iterator this is an operation that is linear with regards to i so it's slow for std::list as well.

Heapify can be adust to not need to std::advance an arbitrary amount by going over the list multiple times:

template<typename BiDirIterator, typename Cmp>
void heapify(BiDirIterator begin, BiDirIterator end, Cmp cmp)
{
    bool changed = false;
    do{
        auto parent = begin;
        auto left = begin == end? end : begin+1;
        auto right = left == end? end : left+1;
        while (left != end) {
            auto largest = left
            if (right != end && cmp(*left, *right))
                largest = right;

            if (cmp(*parent, *largest)){
                std::swap(*parent, *largest);
                changed = true;
            }

            parent = ++parent;
            left = right == end? end : right+1;
            right = left == end? end : left+1;
        }
    }while(changed);
}

This will loop over the data at most O(log n) times and puts the elements in heap order.

However for pulling out the data there is no way to only look at the affected elements in O(log n) time. Which means that you can't sort a linked list with heapsort and still be in O(n log n) time complexity.

Instead to sort a linked list in O(n log n) time you would use merge sort. This requires that you are able to be able to split the list up into separate linked lists in that you can merge. std::list has this functionality with it's splice member function. (It also has a sort member function that will do that built-in)


Sudoku Solver To SAT implementation I implemented an algorithm for solving sudoku using reduction from sudoku to SAT.

Now, I am trying to refactor the code using code review principles. I find some difficulties apply the principles. Because of that I would like to share the following code snippet in order to get some hints how to approach this problem efficiently.

The following method generate boolean expressions in CNF such that for any entry puzzle_{ij} a boolean expression E in CNF will be generated such that E = true iff there is exactly one literal x_{ijk}, k in {1, 2, ..., 9} such that x_{ijk} = true

// DIGITS := {1, 2, ..., 9}
// Combinatorics.product(DIGITS) = {(1,1), (1,2), ..., (9,8), (9,9)}

static List<List<Integer>> exactlyOneDigitForEachEntry() {
    ArrayList<Pair> pairs = Combinatorics.product(DIGITS);
    List<List<Integer>> clauses = new ArrayList();
    for (Pair pair : pairs) {
        ArrayList<Integer> literals = new ArrayList();
        for (int k : DIGITS) {
            int literal = (100 * pair.getA()) + (10 * pair.getB()) + k;
            literals.add(literal);
        }
        List<List<Integer>> expression = exactlyOneOf(literals);
        extendClauses(clauses, expression);
    }

    return clauses;
}

.

/**
 *
 * @param literals A set of literals
 * @return a boolean expression E in conjunctive normal form such that E =
 * true iff there is exactly one literal that is true.
 */
private static List<List<Integer>> exactlyOneOf(ArrayList<Integer> literals) {
    // The arrayList represents a boolean formula F such that F = true iff
    // there is exaclty one literal l such that l = true
    List<List<Integer>> expression = new ArrayList();
    expression.add(literals);
    ArrayList<Pair> pairs = Combinatorics.combinations(literals);
    for (Pair pair : pairs) {
        List<Integer> clause = new ArrayList();
        clause.add(-1 * pair.getA());
        clause.add(-1 * pair.getB());
        expression.add(clause);
    }
    return expression;
}

The code is pretty short, so there's not much to comment on.

Magic numbers

int literal = (100 * pair.getA()) + (10 * pair.getB()) + k

I have no idea what that means. What is 100? What is 10? Why do you sum them up? I suggest creating a method int getLiteralId(int firstDigit, int secondDigit, int thirdDigit) and add a comment that explains what's actually going on.

clause.add(-1 * pair.getA());
clause.add(-1 * pair.getB());

Why are multiplying them by -1? At least create a method with an appropriate name (something like int negateLiteral(int literalId)).

Variable names

k and pairs isn't very descriptive. I suggest something like digit and digitPairs.

Overall design

Representing a boolean expression as a list of integers leads to confusion. I'd redesign it in the following way:

A BooleanExpression abstract base class.
A Variable subclass for representing variables.
A Negation subclass for representing negations.
A Conjunction subclass for representing  conjunctions.
A Disjunction subclass for representing disjunctions.
The signatures will be
BooleanExpression exactlyOneOf(List<BooleanExpression> expressions) and
BooleanExpression exacltyOneDigitForEachEntry()

It'll make the code much more readable. For instance,

Conjunction conjunction = new Conjunction();
...
conjunction.add(new Negation(expression));

and

Disjunction disjunction = new Disjunction();
disjunction.add(new Variable(firstDigit));
disjunction.add(new Variable(secondDigit));
disjunction.add(new Variable(thirdDigit));
conjunction.add(disjunction);

looks much better than some "magic" multiplication and addition of seemingly unrelated numbers, doesn't it?

It's safer in a sense that an arbitrary integer can't accidentally turn into a literal/expression.

It also makes more sense because a boolean expression is a boolean expression. It's not a list of integers (it might be implemented as such, but it's an implementation detail).

What if you use a third-party solver and it needs a list of integers in a specific format?  It doesn't really matter. You can still use the design shown above. Just add a toList method to the BooleanExpression and implement it in concrete subclasses according to the rules of the solver. Call it only once before passing your expression to the solver.


Parsing pipe delimited lines I am parsing the following line of code via a specific format.
Line is:

S|111111|87654321|Bar UK|BCreace UK|GBP|24/08/2010|

The Format is:

Index    Field Length
S0 -     1
S1 -     6
S2 -     34
....
...
S6 -     10

I am validating using many if statements. Could anyone suggest a better approach?

private static StatementLineResult Validate(string delimitedRecord)
{
    if (delimitedRecord == null)
        throw new ArgumentNullException("delimitedRecord");

    var items = delimitedRecord.Split('|');
    if(items.Length != 19)
        throw new Exception("Improper line format");

    var errorMessage = new Dictionary<string, string>();

    var bankIdentifierCodes = new List<string> {"ABCDGB2L", "EFGHGB2L"};

    if (items[0].Length != 1 || items[0] != "S")
        errorMessage.Add("Record Identifier","Invalid Record Identifier");

    var sortCode = 0;
    if (!int.TryParse(items[1], out sortCode) || items[1].Length > 6)
        errorMessage.Add("Sort Code", "Invalid sort code");

    if (string.IsNullOrEmpty(items[2]) || items[1].Length > 34)
        errorMessage.Add("Account Number", "Invalid account number");

    if (string.IsNullOrEmpty(items[3]) || items[1].Length > 35)
        errorMessage.Add("Account Alias", "Invalid account alias");

    if (string.IsNullOrEmpty(items[4]) || items[1].Length > 35)
        errorMessage.Add("Account Name", "Invalid account name");

    if(string.IsNullOrEmpty(items[5]) || items[5].Length != 3)
        errorMessage.Add("Account Currency", "Invalid account currency");

    if (!string.IsNullOrEmpty(items[6]) && items[6].Length > 20)
        errorMessage.Add("Account Type", "Invalid account type");

    if(string.IsNullOrEmpty(items[7]) || items[7].Length != 8 || !bankIdentifierCodes.Contains(items[7],StringComparer.OrdinalIgnoreCase))
        errorMessage.Add("Bank Identifier Code", "Invalid bank identifier code");

    if (!string.IsNullOrEmpty(items[8]) && items[8].Length > 35)
        errorMessage.Add("Bank Name", "Invalid bank name");

    if (!string.IsNullOrEmpty(items[9]) && items[9].Length > 27)
        errorMessage.Add("Bank Branch Name", "Invalid bank branch name");

    DateTime transactionDate;
    if (!DateTime.TryParse(items[10], out transactionDate))
        errorMessage.Add("Transaction Date", "Invalid date");

    if (!string.IsNullOrEmpty(items[11]) && items[11].Length > 25)
        errorMessage.Add("Narrative Line 1", "Invalid narrative line 1");
    if (!string.IsNullOrEmpty(items[12]) && items[12].Length > 25)
        errorMessage.Add("Narrative Line 2", "Invalid narrative line 2");
    if (!string.IsNullOrEmpty(items[13]) && items[13].Length > 25)
        errorMessage.Add("Narrative Line 13", "Invalid narrative line 13");
    if (!string.IsNullOrEmpty(items[14]) && items[14].Length > 25)
        errorMessage.Add("Narrative Line 14", "Invalid narrative line 14");
    if (!string.IsNullOrEmpty(items[15]) && items[15].Length > 25)
        errorMessage.Add("Narrative Line 15", "Invalid narrative line 15");

    if(_transactionTypes.First(item=>item.TransactionType==items[16]) == null)
        errorMessage.Add("Transaction Type", "Invalid transaction type");

    decimal debitValue;
    if(items[17] != "" && !Decimal.TryParse(items[17], out debitValue))
        errorMessage.Add("Debit Value", "Invalid debit amount");

    decimal creditValue;
    if (items[18] != "" && !Decimal.TryParse(items[18], out creditValue))
        errorMessage.Add("Credit Value", "Invalid credit amount");

    return new StatementLineResult()
    {
        ErrorMessages = errorMessage,
        Data = BankLineData(delimitedRecord),
        IsValid = errorMessage.Count==0
    };
}

In the object initializer syntax you're using here:

return new StatementLineResult()
{
    ErrorMessages = errorMessage,
    Data = BankLineData(delimitedRecord),
    IsValid = errorMessage.Count==0
};

The parentheses are redundant; object initializer calls the parameterless constructor by default, so you don't need to call it explicitly.

However there's a design smell here; using this syntax tells me your StatementLineResult class looks something like this:

public class StatementLineResult
{
    public Dictionary<string,string> ErrorMessages { get; set; }
    public SomeType Data { get; set; }
    public bool IsValid { get; set; }
}

Notice how everything could be set from the outside, even with invalid state? What if IsValid is set to false when there are items in ErrorMessages? Nonsensical isn't it!

The type is crying to be immutable. Make the properties get-only, and assign their values from the constructor. If the logic for IsValid is "has no error messages", then it's probably redundant. In any case, it should be get-only and return ErrorMessages.Count == 0, not an arbitrary bool value.

Don't expose a Dictionary (or a List, for that matter) like that. Expose an IEnumerable<T> - your client code needs only to iterate the values, and doesn't care about the Key and Value concepts. What's the type of T then?

How about a tiny little class?

public class FieldValidationError
{
    public FieldValidationError(string fieldName, string message)
    {
        _fieldName = fieldName;
        _message = message;
    }

    private readonly string _fieldName;
    public string FieldName { get { return _fieldName; } }

    private readonly string _message;
    public string Message { get { return _message; } }
}

Then instead of exposing a Dictionary<string,string>, you can expose a much more expressive IEnumerable<FieldValidationError>, and not worry about the possibility that client code could tamper with the results.

Random thoughts:

Don't compare strings to null or "" - use string.IsNullOrEmpty instead... which you are using.. just not consistently.
Don't throw System.Exception. Use the most appropriate existing exception type, or make one if none fits the bill appropriately. But don't throw the base exception type; it forces client code to catch (Exception), which makes it catch all kinds of nasties that you would probably rather see bubble up the call stack and crash your app. In this case, throwing an ArgumentException would probably be fine; deriving an InvalidFieldCountException from it could work too.

Spot the issue:

if (!int.TryParse(items[1], out sortCode) || items[1].Length > 6)
    errorMessage.Add("Sort Code", "Invalid sort code");

if (string.IsNullOrEmpty(items[2]) || items[1].Length > 34)
    errorMessage.Add("Account Number", "Invalid account number");

if (string.IsNullOrEmpty(items[3]) || items[1].Length > 35)
    errorMessage.Add("Account Alias", "Invalid account alias");

if (string.IsNullOrEmpty(items[4]) || items[1].Length > 35)
    errorMessage.Add("Account Name", "Invalid account name");

if(string.IsNullOrEmpty(items[5]) || items[5].Length != 3)
    errorMessage.Add("Account Currency", "Invalid account currency");

Looks like a copy-pasta bug. I think the part after the || operator is actually intended to use the same index as the part before.

This is clearly a bug:

if(_transactionTypes.First(item=>item.TransactionType==items[16]) == null)

First does not return null when there's no item: it throws an InvalidOperationException instead. You probably meant to use FirstOrDefault:

if(_transactionTypes.FirstOrDefault(item => item.TransactionType == items[16]) == null)

...but that's sub-optimal, too; if that's meant to say "if there's no _transactionTypes item for items[16]", then it can be expressed more concisely like this:

if(!_transactionTypes.Any(item => item.TransactionType == items[16]))


Python private variables in a class I was reading about private variables and functions in Python and I managed to get almost everything except one thing.

Is it recommended to write underscore before variables defined inside a function?

Here is the code so it can be more clear:

Version 1:

class House():

    def __init__(self):

        self.color = "RED"
        self._discount_coefficient = 0.8

    def get_total_charges(self, json_from_communal_company):

        _water_price        = json_from_communal_company["waterPrice"]
        _electricity_price  = json_from_communal_company["electricityPrice"]

        _total_charges = self._discount_coefficient * (_water_price + _electricity_price)

        return _total_charges

Version 2:

class House():

    def __init__(self):

        self.color = "RED"
        self._discount_coefficient = 0.8

    def get_total_charges(self, json_from_communal_company):

        water_price        = json_from_communal_company["waterPrice"]
        electricity_price  = json_from_communal_company["electricityPrice"]

        total_charges = self._discount_coefficient * (water_price + electricity_price)

        return total_charges

I know that variable _discount_coefficient needs to have underscore, but I am not sure if variables water_price, electricity_price and total_charges should have underscore or not.

Can someone tell me which version is better and why?

For Python object names, a single leading underscore denotes private status, like you said.
Unlike other languages, this is not enforced however.
It is only a (strong) suggestion for users of your code not to use these objects.

This could be internal variables, like your self._discount_coefficient.
For example, maybe you implement a @property that secrectly stores something in self._discount_coefficient, but for the calling code, you need to enforce an invariant (in this case, maybe 0 <= self._discount_coefficient <= 1).
This way, users can only set the discount_coefficient instance attribute (notice no underscore) to be within 0 and 1, otherwise an error is thrown.
Nothing keeps people from directly mingling with self._discount_coefficient, however.
Therefore, the convention of underscores; basically a contract where all guarantees you make about your code are void if private objects are modified directly.

Having said all that (which you probably heard of already), it becomes clear that users of your House class can never directly access the variables inside your method anyway.
They either access get_total_charges as a whole, or nothing.
The decision is therefore to either hide get_total_charges as a whole, if it is private, or leave it like in your case.

water_price, electricity_price and total_charges are only temporary variables and tossed the moment the function returns.
They are never relevant to the entire private/public debate because they are never seen from calling code.
If you assigned any of them as instance attributes via self, you would have to decide for or against a leading underscore.

As it is, Version 2 looks fine.
As for remaining style, House() can just be House and the equal signs before json_from_communal_company should not be aligned (looks fine now but is a maintenance nightmare).


Doubly linked list using C++ and templates Working doubly linked list using C++ and templates, I tried to make it by myself. I tried to use head and tail as buffers and make a simple implementation. Please help me understand what I have done wrong, done good and could have done worse.

#pragma once
#include <iostream>

template <typename T>
struct node
{
    T value;
    node* prev = nullptr;
    node* next = nullptr;
};

template <typename T>
class LList
{
public:
    LList<T>();
    ~LList();
    T& get(const int& pos) const;
    void insert(const T& value, const int& pos);
    T& remove(const int& pos);
private:
    int n_elements;
    node<T> *head = new node<T>;
    node<T> *tail = new node<T>;

};

template<typename T>
LList<T>::LList()
{
    n_elements = 0;
    head->next = tail;
    tail->prev = head;
}

template<typename T>
LList<T>::~LList()
{
}

template<typename T>
T& LList<T>::get(const int & pos) const
{
    node<T> *temp = head;
    for (int i = 0; i <= pos; i++)
        temp = temp->next;
    return temp->value;
}

template<typename T>
void LList<T>::insert(const T & value, const int & pos)
{
    node<T> *temp = new node<T>;
    temp->value = value;
    node<T> *prev = head;
    node<T> *next = head->next;

    for (int i = 0; i < pos; i++)
    {
        prev = prev->next;
        next = next->next;
    }
    prev->next = temp;
    next->prev = temp;
    temp->prev = prev;
    temp->next = next;
    ++n_elements;
}

template<typename T>
T & LList<T>::remove(const int & pos)
{
    node<T> *prev = head;
    node<T> *curr = head->next;
    node<T> *next = curr->next;
    for (int i = 0; i < pos; i++)
    {
        prev = prev->next;
        curr = curr->next;
        next = next->next;
    }
    prev->next = next;
    next->prev = prev;
    T value = curr->value;
    delete curr;
    --n_elements;
    return value;
}

#include <iostream>
#include "LList.h"

int main()
{
    LList <double> mylist;
    mylist.insert(20, 0);
    mylist.insert(30, 1);
    mylist.insert(40, 2);
    mylist.insert(30, 2);
    mylist.insert(10, 0);
    std::cout << "removing: " << mylist.remove(0) << std::endl;
    mylist.insert(10, 0);
    mylist.insert(10, 0);
    mylist.insert(10, 0);
    std::cout << "removing: " << mylist.remove(3) << std::endl;
    std::cout << "removing: " << mylist.remove(2) << std::endl;
    std::cout << "removing: " << mylist.remove(1) << std::endl;
    std::cout << mylist.get(0) << std::endl;
    std::cout << mylist.get(1) << std::endl;
    std::cout << mylist.get(2) << std::endl;
    std::cout << mylist.get(3) << std::endl;

    system("pause");
    return 0;
}

Includes
<iostream> isn't needed by the header, so don't include it there.
Null pointer
When I try to run the code, I get an attempt to dereference a null pointer at the first remove(0), which crashes the program.  This is because remove() returns a reference to local variable value, which is no longer in scope.  Return it by value instead:
    T remove(int pos);
 //^^^ not T&

Memory leak
If I add return 0; before the crash (or fix the bug), I do see a memory leak:
==29576== 168 (24 direct, 144 indirect) bytes in 1 blocks are definitely lost in loss record 7 of 7
==29576==    at 0x4835E2F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29576==    by 0x109362: LList<double>::LList() (205728.cpp:27)
==29576==    by 0x10917F: main (205728.cpp:92)

I think ~LList() needs to do more cleanup:
template<typename T>
LList<T>::~LList()
{
    for (auto p = head->next;  p != tail;  ) {
        auto next = p->next;
        delete p;
        p = next;
    }
    delete head;
    delete tail;
}

Copy construction
205728.cpp:11:7: warning: ‘class LList<double>’ has pointer data members [-Weffc++]
205728.cpp:11:7: warning:   but does not override ‘LList<double>(const LList<double>&)’ [-Weffc++]
205728.cpp:11:7: warning:   or ‘operator=(const LList<double>&)’ [-Weffc++]

We need to fix this, and either make the list safely copyable, or totally uncopyable.
Prefer initialization to assignment
LList<T>::LList() : n_elements{0}
                   /^^^^^^^^^^^^^

Or just provide a default initializer, as with prev and next.
Don't pass primitive types by reference
T& get(const int& pos) const;
//             ^^^

Built-in types such as int are best passed by value rather than by const reference (and prefer size type to int for counting objects).  Also, in this case, we probably want to return a reference to const T from a const list, and a reference to mutable T from a mutable list (do as the standard containers do):
T const& get(std::size_t pos) const;
T& get(std::size_t pos);

Copying
Prefer to std::move() values where possible.  It's not hard to make the list able to accept move-only types:
template <typename T>
struct node
{
    T value;
    node* prev;
    node* next;
    node(T value, node *prev = nullptr, node *next = nullptr)
        : value{std::move(value)}, prev{prev}, next{next}
    {}
};

template<typename T>
void LList<T>::insert(T value, const int pos)
{
    node<T> *temp = new node<T>(std::move(value), prev, next);

Exception safety
Constructing a linked list requires memory to be allocated for head and tail.  If the first new succeeds, but the second fails, we end up leaking memory when the std::bad_alloc is thrown.
There's no reason not to make head and tail be plain sub-objects:
private:
    int n_elements = 0;
    node<T> head = { T{}, nullptr, &tail };
    node<T> tail = { T{}, &head, nullptr };

(adjust all head-> to head. and other head to &head; the same for tail).
Remove doesn't need to track prev and next
Once we've found the target for remove, we can easily access its neighbours:
template<typename T>
T LList<T>::remove(const int pos)
{
    node<T> *curr = head.next;
    for (int i = 0; i < pos; i++) {
        curr = curr->next;
    }
    curr->prev->next = curr->next;
    curr->next->prev = curr->prev;
    T value = std::move(curr->value);
    delete curr;
    --n_elements;
    return value;
}

The insert method can be simplified in a similar way.
The node type needn't be public
Since node isn't exposed in the public interface, it can be a private type within LList:
template <typename T>
class LList
{
    struct node
    {
        T value;
        node* prev;
        node* next;
        node(T value, node *prev = nullptr, node *next = nullptr)
            : value{std::move(value)}, prev{prev}, next{next}
        {}
    };

    // ...

    node head = { T{}, nullptr, &tail };
    node tail = { T{}, &head, nullptr };

};

Modified code
I've made some other minor changes to simplify further.
#include <cstddef>
#include <utility>

template <typename T>
class LList
{
    struct node
    {
        T value;
        node* prev;
        node* next;
        node(T value, node *prev = nullptr, node *next = nullptr)
            : value{std::move(value)}, prev{prev}, next{next}
        {}
        node(const node&) = delete;
        void operator=(const node&) = delete;

        friend void swap(node& a, node& b) {
            using std::swap;
            swap(a.value, b.value);
            swap(a.prev, b.prev);
            swap(a.next, b.next);
        }
    };

public:
    LList() = default;
    LList(const LList<T>&);
    LList(const LList<T>&&);
    LList& operator=(LList<T>);
    ~LList();

    T const& get(std::size_t pos) const;
    T& get(std::size_t pos);

    void insert(T value, std::size_t pos);
    T remove(std::size_t pos);

    template<typename U>
    friend void swap(LList<U>&, LList<U>&);

private:
    std::size_t n_elements = 0;
    node head = { T{}, nullptr, &tail };
    node tail = { T{}, &head, nullptr };
};

template<typename T>
LList<T>::LList(const LList<T>& other)
{
    // work from back to front, so we always insert at position 0
    // (which is the cheapest)
    for (auto i = other.n_elements;  i > 0;  --i) {
        insert(other.get(i-1), 0);
    }
}

template<typename T>
LList<T>::LList(const LList<T>&& other)
{
    swap(*this, other);
}

template<typename T>
LList<T>& LList<T>::operator=(LList<T> other)
{
    swap(*this, other);
    return *this;
}

template<typename T>
LList<T>::~LList()
{
    for (auto p = head.next;  p != &tail;  ) {
        auto next = p->next;
        delete p;
        p = next;
    }
}

template<typename T>
void swap(LList<T>& a, LList<T>& b)
{
    using std::swap;
    swap(a.head, b.head);
    swap(a.tail, b.tail);
    swap(a.n_elements, b.n_elements);
}

template<typename T>
const T& LList<T>::get(std::size_t pos) const
{
    auto p = head.next;
    while (pos--)
        p = p->next;
    return p->value;
}

template<typename T>
T& LList<T>::get(std::size_t pos)
{
    auto p = head.next;
    while (pos--)
        p = p->next;
    return p->value;
}

template<typename T>
void LList<T>::insert(T value, std::size_t pos)
{
    auto p = &head;
    while (pos--)
        p = p->next;
    auto next = p->next;
    next->prev = p->next = new node(std::move(value), p, next);
    ++n_elements;
}

template<typename T>
T LList<T>::remove(std::size_t pos)
{
    auto p = head.next;
    while (pos--)
        p = p->next;
    p->prev->next = p->next;
    p->next->prev = p->prev;
    auto value = std::move(p->value);
    delete p;
    --n_elements;
    return value;
}

From this position, you could consider more advanced topics, such as providing iterators.  Actually, providing full iterator support is quite a lot of work - 12 methods and a pair of classes - but there are some good reviews here that may give you a metaphorical "leg-up".
Or you might consider your own choice of next steps - don't let me constrain you!


LINQ transforming hierachical data to flat list The following code is pretty ugly. I suspect that there is a more functional and elegant way of achieving this result.

var lines =
new[]{
    new{Head="A",Value="1"},
    new{Head="",Value="2"},
    new{Head="",Value="3"},
    new{Head="",Value="4"},
    new{Head="B",Value="5"},
    new{Head="B",Value="6"},
    new{Head="C",Value="7"},
    new{Head="",Value="8"},
    new{Head="D",Value="9"},
    new{Head="",Value="10"}};

string currentHead="";
string currentValue="";

lines.Select(line=>{
        string newHead;
        string newValue;
        if (String.IsNullOrEmpty(line.Head))
        {
            newHead = currentHead;
        } else
        {
            newHead = line.Head;
            currentHead = line.Head;
        }
        if (String.IsNullOrEmpty(line.Value))
        {
            newValue = currentValue;
        } else
        {
            newValue = line.Value;
            currentValue = line.Value;
        }
        return new{Head=newHead,Value=newValue};}).ToList();

The desired output is to produce

A,1

A,2

A,3

A,4

B,5

B,6

C,7

C,8

D,9

D,10

You need to look through your code with an analytical eye and discover the pattern that you are implementing. I call it default if (null or) empty.

So I created an extension method to implement that pattern.

public static string DefaultIfEmpty(this string source, string defaultValue)
{
    return string.IsNullOrEmpty(source) ? defaultValue : source;
}

(Note of caution: you may want to choose a different name for it since this method already exists in LINQ; technically, this is an overload, which could create problems if a future version of C# adds this signature.)

This makes it possible to adapt the LINQ expression:

var currentHead = string.Empty;
var currentValue = string.Empty;
var result = lines.Select(
    line =>
    {
        currentHead = line.Head.DefaultIfEmpty(currentHead);
        currentValue = line.Value.DefaultIfEmpty(currentValue);
        return new { Head = currentHead, Value = currentValue };
    }).ToList();

Sweet, isn't it?


A data structure with push(int x), pop(), min() and max() in O(1) I have written this Java code for a data structure which includes 3 stacks to supports four operations in \$O(1)\$: push(int x), pop(), min() and max().

Instead of pushing new max and min in every push, I tried to optimize code in this way to have less space.

import java.util.Stack;

public class MyDS {

    Stack<Integer> s;
    Stack<Integer> minStack;
    Stack<Integer> maxStack;

    public MyDS(){
        s = new Stack<Integer>();
        minStack = new Stack<Integer>();
        maxStack = new Stack<Integer>();
    }

    // Push Method
    public void push(int k){

        if(minStack.isEmpty()){
            minStack.push(k);
        }else if(k <= minStack.peek()){
            minStack.push(k);
        }

        if(maxStack.isEmpty()){
            maxStack.push(k);
        }else if(k >= maxStack.peek()){
            maxStack.push(k);
        }
        s.push(k);
    }

    // Pop Method
    public void pop(){

        int popped;
        if(!s.isEmpty()){
            popped = s.pop();
        }else{
            popped = -1;
        }

        if(popped == min()){
            minStack.pop();
        }

        if(popped == max()){
            maxStack.pop();
        }
    }

    // Min Method
    public int min(){
        if(!minStack.isEmpty()){
            return minStack.peek();
        }else{
            return Integer.MIN_VALUE;
        }
    }

    // Max Method
    public int max(){
        if(!maxStack.isEmpty()){
            return maxStack.peek();
        }else{
            return Integer.MAX_VALUE;
        }
    }
}

This is my earlier version of DS:

import java.util.Stack;

public class DS {
    static Stack<Integer> stack;
    static Stack<Integer> minStack;
    static Stack<Integer> maxStack;

    public DS(){
        stack = new Stack<Integer>();
        minStack = new Stack<Integer>();
        maxStack = new Stack<Integer>();
    }

    // Push Method
    public void push(int k){
        stack.push(k);
        if(!minStack.isEmpty()){
            minStack.push(Math.min(k, minStack.peek()));
        }else{
            minStack.push(k);
        }
        if(!maxStack.isEmpty()){
            maxStack.push(Math.max(k, maxStack.peek()));
        }else{
            maxStack.push(k);
        }
    }

    // Pop Method
    public void pop(){
        if(!stack.isEmpty() && !minStack.isEmpty() && !maxStack.isEmpty()){
            stack.pop();
            minStack.pop();
            maxStack.pop();
        }
    }

    // Find Min
    public int findMin(){
        if(!minStack.isEmpty()){
            return minStack.peek();
        }
        return Integer.MIN_VALUE;
    }

    // Find Max
    public int findMax(){
        if(!maxStack.isEmpty()){
            return maxStack.peek();
        }
        return Integer.MAX_VALUE;
    }

    public static void main(String[] args) {

        DS ds = new DS();

        System.out.println("Push 7, 6, 5: ");
        ds.push(7);
        ds.push(6);
        ds.push(5);

        System.out.println("S1: " + stack);
        System.out.println("S2: " + minStack);
        System.out.println("S3: " + maxStack);

        System.out.println("Min till now: " + ds.findMin());
        System.out.println("Max till now: " + ds.findMax());

        System.out.println("Push 4, 3: ");
        ds.push(4);
        ds.push(3);

        System.out.println(stack);
        System.out.println(minStack);
        System.out.println(maxStack);

        System.out.println("Min till now: " + ds.findMin());
        System.out.println("Max till now: " + ds.findMax());

        System.out.println("1 pop(): ");
        ds.pop();
        System.out.println("Min till now: " + ds.findMin());
        System.out.println("Max till now: " + ds.findMax());

        System.out.println("1 pop(): ");
        ds.pop();
        System.out.println("Min till now: " + ds.findMin());
        System.out.println("Max till now: " + ds.findMax());
    }
}

Your MyDS class has the right idea, in general.

Special values like -1, Integer.MIN_VALUE, and Integer.MAX_VALUE make me suspicious.  All of those special values denote what I consider to be error cases.  Using special cases that might also be valid data is a dangerous habit that can lead to bugs.  Instead of those special numbers, it would be better to throw exceptions — probably NoSuchElementException.  You should also offer a size() and/or an isEmpty() method so that users of your data structure can proactively avoid encountering the exception.

The three instance variables should be private.  The default access is rarely appropriate.  java.util.Stack is to be avoided, due to unfortunate historical design decisions (inappropriately extending java.util.Vector, and being thread-safe by default).  The documentation recommends ArrayDeque instead.

Of the four operations in MyDS, I think pop() could use the most work.    It's weird that pop() doesn't return a value.  The -1 is entirely avoidable: if the main stack is empty, the min and max stacks should surely be empty too.

public int pop() {
    if (s.isEmpty()) {
        throw new NoSuchElementException();
    }
    int popped = s.pop();
    if (popped == min()) {
        minStack.pop();
    }
    if (popped == max()) {
        maxStack.pop();
    }
    return popped;
}


Game of Life generation procession algorithm I've tried to make Game of Life in console and this is the algorithm I used to generate new generation:

void processGeneration() {
    population = 0;
    int neighbors;
    int tempGrid[ROWS][COLS];
    for(int i = 0; i < ROWS; i++) {
        for(int j = 0; j < COLS; j++) {
            if (grid[i][j] == -1) {
                continue;
            }
            neighbors = countNeighbors(i, j);
            if (grid[i][j] == 0) {
                if (neighbors == 3) {
                    tempGrid[i][j] = 1;
                } else {
                    tempGrid[i][j] = 0;
                }
            } else if (grid[i][j] == 1) {
                if (neighbors < 2 || neighbors > 3) {
                    tempGrid[i][j] = 0;
                } else {
                    tempGrid[i][j] = 1;
                }
            }
        }
    }
    for(int i = 0; i < ROWS; i++) {
        for(int j = 0; j < COLS; j++) {
            if (grid[i][j] == -1) {
                continue;
            }
            if (tempGrid[i][j] == 1) {
                population++;
            }
            grid[i][j] = tempGrid[i][j];
        }
    }
}

I'm a bit uncertain if what I did was right, so could you check the algorithm? Also, grid is the grid where I save the cells in and I just use tempGrid to change the values and then copy them to the grid.

int countNeighbors(int row, int col) {
    int neighbors = 0;
    for(int i = row - 1; i < row + 2; i++) {
        for(int j = col - 1; j < col + 2; j++) {
            if (i == row && j == col) {
                continue;
            }
            if (i > -1 && j > -1 && i < ROWS && j < COLS) {
                neighbors += grid[i][j];
            }
        }
    }
    return neighbors;
}

Your algorithms looks correct, I could not find any obvious mistakes.

On the other hand, I have a suggestion towards the readability of your code:
Use more functions that tell the reader what you're doing right now. For example, instead of saying

if (grid[i][j] == 0) {
    if (neighbors == 3) {
        tempGrid[i][j] = 1;
    } else {
        tempGrid[i][j] = 0;
    }
}

you could write:

int cellIsDead(int x, int y) { return grid[i][j] == 0; }
int hasEnoughNeighboursToComeToLife(int x, int y) { return countNeighbors(x, y) == 3 }
void makeCellComeAlive(int x, int y) { tempGrid[i][j] = 1; }

if (cellisDead(i, j) && hasEnoughNeighboursToComeToLife(i, j)) {
    makeCellComeAlive(i, j);
}

Yes, this takes more time and also probably some global variables (although grid already looks global, so no issue), but it is a) easier to read (see how it almost reads like the rules of the game?) and b) easier to prove to be correct using unit tests. And any decent compiler will inline these function calls at compile time, so no runtime overhead.


Generate random string to match the goal string with minimum iteration and control statements The following is my python code to find the number of iterations needed to generate random string iteratively to match the input string.
Here I have repeated a similar kind of for loop in both the functions getRandomString() and getMatchString(). I tried to avoid it but couldn't. I want this to be combined together into a single generateString() function with minimum iteration statements. Any suggestions to make it so or any other possible way to make the code better without complicating much?
The input is any string statements like "Hello, World! How Are Y@u doiNG?" and the output is the number of iterations needed to match the input string e.g 408. Your suggestions to improve the code will be helpful.
import random

goalString = input()
stringLen = len(goalString)

alphanum = '''abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ !@#&()–[{}]:;‘,'?/*1234567890'''
alphaLen = len(alphanum)

def getRandomString():
    randomString = ''
    for i in range(stringLen):
        randomString = randomString + alphanum[random.randrange(alphaLen)]

    return randomString

def getMatchString():
    betterString = getRandomString()
    newString = ''
    iteration = 1 #included the iteration of generating randomString
    while betterString != goalString:
        charIndex = [i for i in range(stringLen) if betterString[i] == goalString[i]]
        for i in range(stringLen):
            if i in charIndex:
                newString = newString + betterString[i]
            else:
                newString = newString + alphanum[random.randrange(alphaLen)]

        betterString = newString
        newString = ''
        iteration = iteration + 1

    return iteration

print(getMatchString())

Sample Input: "Hello, World! How Are Y@u doiNG?"
Sample Output: 408   ##May change depending on the random iteration##

Find my suggestions below.

goalString = input()

Let the user know what to do, input accepts a string as an argument:
goalString = input('Type a string:')

alphanum = '''abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ !@#&()–[{}]:;‘,'?/*1234567890'''

If you need all printable characters you can use string.printable:
import string

alphabet = string.printable
# alphabet contains: 0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~

Or choose a combination of other constants in the string module.

Generating a random string
def getRandomString():
    randomString = ''
    for i in range(stringLen):
        randomString = randomString + alphanum[random.randrange(alphaLen)]

    return randomString

Avoid using globals passing the length as an argument
String concatenation can be done more efficiently with join
Use random.choices to avoid the for-loop
PEP 8: function names should be lowercase, with words separated by underscores. Variable names follow the same convention as function names.

def generate_random_string(n):
    alphabet = string.printable
    return ''.join(random.choices(alphabet, k=n))

The function to match the string creates many strings using concatenation. It can be done more efficiently by using a list of booleans (as @rperezsoto suggested).
def get_matching_string(s):
    n = len(s)
    matches = [False] * n
    i = 0
    while not all(matches):
        matches = [matches[i] or s[i] == c for i, c in enumerate(generate_random_string(n))]
        i += 1
    return i

The key part is to use the operator or that "remembers" positive matches.

I want this to be combined together into a single generateString()
function with minimum iteration statements

I would suggest keeping two separate functions as in your first solution. The code will be easier to test and reuse. Additionally, you can extract the logic to request the input and print the result from the function getMatchString with a main function:
def main():
    s = input('Type a string:')
    print(get_matching_string(s))

if __name__ == "__main__":
    main()


Removing same event from listener I have a myDiv.mousedown, window.mousemove, and window.mouseup event. I'm trying to organize my code, and have it clean. So I created a function and returned the 3 event handlers.
The mousedown event adds a mousemove event, and the mouseup event removes the mousemove event.

Since I need to pass the exact same object to removeEventListener() as the one I used with addEventListener(), I have to create a variable for each event handler.

This doesn't seem to be organized and clean. Does this follow common practice? If not what is the 'correct', organized and clean code to perform that?

JSFiddle

var myDiv = document.getElementById('myDiv');

var VeryUsefullClass = (function() {

  function VeryUsefullClass(parentElem, options) {
    var _this = this;
    _this.index = 0;

    _this.mouseDownHandler = _this.mouseHandler().mouseDown;
    _this.mouseMoveHandler = _this.mouseHandler().mouseMove;
    _this.mouseUpHandler = _this.mouseHandler().mouseUp;

    myDiv.addEventListener('mousedown', _this.mouseDownHandler);
    window.addEventListener('mouseup', _this.mouseUpHandler);
  }

  VeryUsefullClass.prototype.mouseHandler = function() {
    var _this = this;
    var obj = {
      mouseDown: function(e) {
        console.log('mouseDown');
        window.addEventListener('mousemove', _this.mouseMoveHandler);
      },
      mouseMove: function(e) {
        myDiv.innerHTML = 'mouseMove ' + _this.index++;
      },
      mouseUp: function(e) {
        console.log('mouseUp');
        _this.index = 0;
        window.removeEventListener('mousemove', _this.mouseMoveHandler);
      }
    }
    return obj;
  }

  return VeryUsefullClass;
})();

console.clear();

var hello = new VeryUsefullClass();
#myDiv {
  position: absolute;
  background-color: orange;
  width: 200px;
  height: 100px;
}

Consider using dependency injection

What that means is "injecting" what you want to manipulate into your class (instead of relying on "global" variable), for example:

let a = 42;
function manipulateA () {
    return a = a + 24;
}
console.log(manipulateA(), a); // 66, 66

versus:

let a = 42;
function manipulate (a) {
    return a = a + 24;
}
console.log(manipulate(a), a); // 66, 42

This allows for much easier to test code, and you know exactly what will happen with everything instead of relying on something to magically happen in the global scope.

Consider this instead (using es6 and dependency injection):

class MoveOver {
  // dependency injection
  constructor ({element = document.body}) {
    this.element = element;
    this.index = 0;
  }

  startListening () {
    const mouseMove = () => {
      this.element.textContent = `mouseMove_${this.index++}`;
    }
    this.element.addEventListener('mousedown', () => {
      console.log('mouseDown');
      this.element.addEventListener('mousemove', mouseMove, false);
    }, false);
    window.addEventListener('mouseup', () => {
      console.log('mouseUp');
      this.element.removeEventListener('mousemove', mouseMove, false);
    }, false);
  }
}

const move1 = new MoveOver({element: document.getElementById('mouse')});
const move2 = new MoveOver({element: document.getElementById('mouse2')});

move1.startListening();
move2.startListening();
.cover {
  height: 200px; width: 200px;
  border: red solid 1px;
  float: left; margin: 15px;
}

In the above example we are able to add the same events to two (2) different elements, but yet still yield the same results for both


Creating multiple threads to process items in a listview The goal is to pick items from a listview, do some web related task with the information and then update the UI with the returned information from the web request.

The problem is with having multiple list view items processed at once, me being a newbie to C#, i came up with this code, I'm pretty sure this is not good, i just came up with it, is there a better way to get this done ?

Dictionary<String,BackgroundWorker> worker_list = new Dictionary<String, BackgroundWorker>();
int completed_count = 0;
int total_work;

public Form1()
{
    InitializeComponent();
}

public void worker(String woker_id)
{
    WebClient client = new WebClient();
    byte[] response =
    client.UploadValues("http://google.com", new NameValueCollection() { });

    worker_list[woker_id].ReportProgress(100);

    if(completed_count != total_work){
        worker(woker_id);
    }
}

// Adding the work to the que since a thread can't access background Ui thread
private void button2_Click(object sender, EventArgs e)
{
    // preparing task list
    txtOutput.Text += "Total stuff to run" + listView1.Items.Count + Environment.NewLine + Environment.NewLine;

    // check for valid input
    int value;
    if (!int.TryParse(textBox1.Text, out value) || textBox1.Text == "0"){
        MessageBox.Show("invalid number of threads");
        return;
    }

    // create threads
    for (int i = 0; i < value; i++)
    {
        String woker_id = "bg_" + i.ToString();

        worker_list[woker_id] = new BackgroundWorker();
        worker_list[woker_id].WorkerSupportsCancellation = true;
        worker_list[woker_id].WorkerReportsProgress = true;

        // Intiial message
        txtOutput.Text += "Worker " + woker_id + " Started" + Environment.NewLine;

        worker_list[woker_id].DoWork += delegate {
            worker(woker_id);
        };

        worker_list[woker_id].ProgressChanged += delegate {
            if(completed_count == total_work) {
                return;
            }else
            {
                txtOutput.Text += "Worker " + woker_id + "is running again" + Environment.NewLine;
            }

            listView1.Items[completed_count].BackColor = Color.LightGreen;
            completed_count++;
        };

        worker_list[woker_id].RunWorkerCompleted += delegate {
            worker_list[woker_id].CancelAsync();
            worker_list.Remove(woker_id);
            txtOutput.Text += "Worker " + woker_id + " Completed | Thread count = " + worker_list.Count() + Environment.NewLine;
        };

        worker_list[woker_id].RunWorkerAsync();
    }

    // Line spacing
    txtOutput.Text += Environment.NewLine + Environment.NewLine;
}

private void Form1_Load(object sender, EventArgs e)
{
    for (int i = 1; i < 11; i++)
    {
        Random rnd = new Random();
        ListViewItem item = new ListViewItem("Item " + i);
        // Sleep Time
        item.SubItems.Add((rnd.Next(100,500) * i).ToString());
        listView1.Items.Add(item);
    }

    // setting up the total work
    total_work = listView1.Items.Count;
}

Thank you.

We actually don't use the BackgroundWorker anymore. The same thing can be done much easier with the newer Task Parallel Library (TPL).

Briefly you need to (ok, you should) put the download logic into a new class that could have a Download method. It needs to be marked with async and return a Task to be awaitable. But you need a result so you can use it with a generic parameter Task<byte[]>. (Sidenote: remember to dispose the WebClient). For the new download to work you need to use the awaitable upload API.

If you want to be able to report progress or just be notified when a task is finished you can use the IProgress<T> interface as one of the parameters. You call it inside the method to report its progress. Here just an arbitrary task-id.

class Downloader
{
    public async Task<byte[]> Download(int taskId, NameValueCollection data, IProgress<int> progress)
    {
        using (var client = new WebClient())
        {
            var response = await client.UploadValuesTaskAsync("http://google.com", data);
            progress.Report(taskId);
            return response;
        }
    }
}

Your form code could then be reduced to a simple event handler that is also marked with async. It prepares a collection of download-tasks and then awaits them all or processes each one when it's finished (your choice).

When a task is finished, the form receives a progress report (that can be seen here as a notification) via the ProgressChanged event of an instance of the Progress<T> class.

class MyForm : Form
{
    private readonly Downloader _downloader = new Downloader();

    private async void button2_Click(object sender, EventArgs e)
    {
        // Initialize progress handling.
        var progress = new Progress<int>();
        progress.ProgressChanged += (downloader, taskId) =>
        {
            // update label...
        };

        var downloadCount = 2; //... get the count
        var downloadTasks =
            Enumerable
            .Range(0, downloadCount)
            // Create task specific arguments.
            .Select((x, i) => _downloader.Download(i, new NameValueCollection(), progress));

        var results = await Task.WhenAll(downloadTasks);
    }
}

You may also use a different pattern if you want to process each task result right away by doing it as soon as any task is finished with WhenAny and a loop:

for (int i = 0; i < downloadCount; i++)
{
    var firstTask = await Task.WhenAny(downloadTasks);
    var result = await firstTask;
    // process task result...
}


Implementing bounds-checking with exceptions based on an existing bitset container BitPat here is a wrapper around a bitset.

class BitPat
{
private:
    boost::dynamic_bitset<> data;
public:
    bool BitPat::operator[](size_t coord) const
    {
        if(coord < length)
            return data[coord];
        else
            throw std::out_of_range(std::string("Pattern coord out of range")
                + " (" + std::to_string(coord%_width) + ", " + std::to_string(coord/_width) + ") "
                + "[" + std::to_string(coord) + "]");
    }

    // vs:

    bool BitPat::operator[](size_t coord) const
    {
        if(coord >= _length){
            throw std::out_of_range(std::string("Pattern coord out of range")
                + " (" + std::to_string(coord%_width) + ", " + std::to_string(coord/_width) + ") "
                + "[" + std::to_string(coord) + "]");
        }
        return data[coord];
    }
};

I have additional getters and setters where I'd implement bounds-checking like this. It generates a lot of boilerplate code though.
As a first step maybe I should define some little formatting functions, but most importantly I'm curious which version of the if statements is more conventional.
Not sure if omitting return statements like in the first case is good practice either.

It doesn't really matter which direction you do the checking in - it's a matter of personal preference. Both are valid, semantically identical, and will almost certainly perform identically.

One thing you can do to avoid having to rewrite all of this stuff is to have a single function that does bounds-checking:

size_t boundsChecked(size_t coord) const {
    // whichever order of this you prefer
    if (coord >= _length) {
        throw std::out_of_range(...);
    }
    return coord;
}

And then defer to that function everywhere you want to add bounds checking:

bool operator[](size_t coord) const
{
    return data[boundsChecked(coord)];
}


Dynamic insert function for PDO (for any case) I created a dynamic insert function for PDO.

It does work, but I want to be sure that it is secure and well coded.

function insertNewRows($pdo, $table, $fields, $placeholders, $values=''){

    if($values){
        $result = $pdo->prepare("INSERT INTO $table ($fields) VALUES ($placeholders)");
        $result->execute($values);
    }else{
        $result = $pdo->query("INSERT INTO $table ($fields) VALUES ($placeholders)");
    }

}

Using positional placeholders:

$session_id = '1000000001';
$createdBy  = 12;

$fields = "session_id, username, createdBy";
$placeholders = "?, 'boss 21', ?";

$values = array($session_id,  $createdBy);

insertNewRows($pdo, 'web_statistics', $fields, $placeholders, $values);

Not using positional placeholders:

$fields = "session_id, username, createdBy";
$placeholders = "'abc', 'boss 25', 3";

insertNewRows($pdo, 'web_statistics', $fields, $placeholders);

I want to use the same tactic for UPDATE and SELECT functions, so am I good to do so with the code I have written?

I don't see any point in this function in its present form.
Given you've got a general purpose query function,
function pdo($pdo, $sql, $args = NULL)
{
    $stmt = $pdo->prepare($sql);
    $stmt->execute($args);
    return $stmt;
}

it will be cleaner, easier to use and more readable to have a conventional SQL than such random rags of a code:
$session_id = '1000000001';
$createdBy  = 12;

$sql = "INSERT INTO web_statistics (session_id, username, createdBy) VALUES (?,?,?)";
pdo($pdo, $sql, [$session_id,  'boss 21', $createdBy]);

The same goes for SELECT and UPDATE. You can find a complete set of examples by the link above.
Update.
Looks like this question got some traction, due to some mysterious ways of Stack overflow. It means it will attract people other than the OP, who may have the same question but have different code, or just curious about Dynamic insert function for PDO in general. Well then, I must admit that the answer is rather a trick and too localized, being focused on the actual code in the OP. For the present code, where different query parts are still hardcoded in the different function parameters, it is indeed makes a lot more sense to have them hardcoded in the form of a conventional SQL query instead.
But of course the common idea of a dynamic insert function for PDO is different. it's rather a function that is called like this:
prepared_insert($pdo, $table_name, $data);

where $data is an associative array like this
$data = [
    'name' => $name,
    'password' => $hashed_password,
];

Well then for this case I've got a simple INSERT helper function for PDO Mysql
function prepared_insert($pdo, $table, $data) {
    $keys = array_keys($data);
    $keys = array_map('escape_mysql_identifier', $keys);
    $fields = implode(",", $keys);
    $table = escape_mysql_identifier($table);
    $placeholders = str_repeat('?,', count($keys) - 1) . '?';
    $sql = "INSERT INTO $table ($fields) VALUES ($placeholders)";
    $pdo->prepare($sql)->execute(array_values($data));
}

Note that his function will need a helper function of its own,
function escape_mysql_identifier($field){
    return "`".str_replace("`", "``", $field)."`";
}

to protect identifiers added to the query.


Count specific array values I want to count specific strings from array values

$array = array("report=D","report=D","report=D","report=I","report=I");

I am going to count the number of D,U & I, as well as their meanings.

D = Delivered
U = Undelivered
I = Invalid

I am expecting an output of:

D = 3
U = 0
I = 2

First, I replace "report=" using str_replace(), then array_filter is used to count specific values.

<?php

$array = array(
"report=D","report=D","report=D","report=I","report=I"
);

$array = str_replace("report=","",$array);

function getting_u($n) { return $n == "U"; }

function getting_d($n) { return $n == "D"; }

function getting_i($n) { return $n == "I"; }

$result_of_u = array_filter($array, "getting_u");
$result_of_d = array_filter($array, "getting_d");
$result_of_i = array_filter($array, "getting_i");

print_r($result_of_u);
print_r($result_of_d);
print_r($result_of_i);

echo "Count of U = ".count($result_of_u);
echo "\n";
echo "Count of D = ".count($result_of_d);
echo "\n";
echo "Count of I = ".count($result_of_i);
?>

See a working code here.

The code seems to work fine for me, but I'm concerned whether this is the correct/best method of procedure.

I am also unsure if my code contains any strange behaviour or present/future problems.

As I can't comment, the philonus's answer is the clearer and best for me, if you want to display zero you can do this, initializing the stats array:

$input = array(
    "report=D","report=D","report=D","report=I","report=I"
);

$stats = array(
    'U' => 0,
    'D' => 0,
    'I' => 0,
);
foreach($input as $value) {
    $types = explode('=', $value);
    $stats[$types[1]]++;
}

print_r($stats);


Finding the nth element of generalized Fibonacci sequences I solved the Fibonacci Golf problem on checkio.org:

  The Fibonacci numbers or Fibonacci sequence are the numbers in the
  following integer sequence: 0, 1, 1, 2, 3, 5, 8, 13, 21...

  This is not the only interesting integer sequence however; there are
  many various sequences like the Fibonacci numbers, where each element
  is calculated using the previous elements. Let's take a look at a few
  of them. Described below are several integer sequences which you
  should try to implement:

fibonacci:
f(0)=0, f(1)=1, f(n)=f(n-1)+f(n-2)

tribonacci:
f(0)=0, f(1)=1, f(2)=1, f(n)=f(n-1)+f(n-2)+f(n-3)

lucas:
f(0)=2, f(1)=1, f(n)=f(n-1)+f(n-2)

jacobsthal:
f(0)=0, f(1)=1, f(n)=f(n-1)+2*f(n-2)

pell:
f(0)=0, f(1)=1, f(n)=2*f(n-1)+f(n-2)

perrin:
f(0)=3, f(1)=0, f(2)=2, f(n)=f(n-2)+f(n-3)

padovan:
f(0)=0, f(1)=1, f(2)=1, f(n)=f(n-2)+f(n-3)

  You are given the name of a sequence and a number "n". You should find
  the n-th element for that given sequence. In this mission the main
  goal is to make your code as short as possible. The system will check
  the length of your compiled code and assign a point value. The shorter
  your compiled code, the better. Your score for this mission is dynamic
  and directly related to the length of your code.

  For reference, scoring is based on the number of characters used. 1000
  bytes is the maximum allowable.

  Input: Two arguments. The name of a sequence as a string and a number
  "n" as a positive integer.

  Output: The n-th element of the sequence as an integer.

Who can make this better? My code is some bytes larger than it's supposed to be.

def fibgolf(type, n):
    if type=="fibonacci":
        a,b=0,1
        for i in range(n):
            a,b=b,a+b
        return a
    elif type == "tribonacci":
        a,b,c=0,1,1
        for i in range(n):
            a,b,c=b,c,a+b+c
        return a
    elif type == "lucas":
        a,b=2,1
        for i in range(n):
            a,b=b,a+b
        return a
    elif type == "jacobsthal":
        a,b=0,1
        for i in range(n):
            a,b=b,a*2+b
        return a
    elif type == "pell":
        a,b=0,1
        for i in range(n):
            a,b=b,b*2+a
        return a
    elif type == "perrin":
        a,b,c=3,0,2
        for i in range(n):
            a,b,c=b,c,a+b
        return a
    elif type == "padovan":
        a,b,c =0,1,1
        for i in range(n):
            a,b,c=b,c,a+b
        return a

The easiest way to simplify this is to come up with a method to represent the various sequences efficiently. For example, the following represents each as a tuple of starting values, and a function that takes one tuple and returns another (the next step):

SEQUENCES = {
    'fibonacci': ((0, 1), lambda a, b: (b, a+b)),
    'tribonacci': ((0, 1, 1), lambda a, b, c: (b, c, a+b+c)),
    ...
}

The function to apply these is then a trivial dictionary lookup and a loop over the number of steps:

def fibgolf(name, n):
    """Calculate the nth value of the named sequence."""
    vals, func = SEQUENCES[name]
    for _ in range(n):
        vals = func(*vals)
    return vals[0]


Project Euler - Problem No.5 - Smallest multiple Here's the problem at hand:

  2520 is the smallest number that can be divided by each of the numbers from 1 to 10 without any remainder.
  What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

Here's the code Ive written to solve it:

from math import sqrt

def prime_factorization(n: int) -> dict:
    '''
    Returns a dict with the prime factorization of the integer n, where the keys of the dict are the prime numbers
    and the values are the powers in the factorization for the appropriate prime number key.
    :param n: an integer we want to compute he's prime factorization
    '''
    prime_dict = {}

    # dealing with even prime numbers - e.g. 2
    while n % 2 == 0:
        if 2 not in prime_dict:
            prime_dict[2] = 0
        prime_dict[2] += 1
        n = n // 2

    # dealing with odd numbers
    upper_bound = int(sqrt(n))
    for i in range(3,upper_bound+1):
        while n % i == 0:
            if i not in prime_dict:
                prime_dict[i] = 0
            prime_dict[i] += 1
            n = n // i

    # In case n is prime by itself
    if n > 1:
        prime_dict[n] = 1

    return prime_dict

def smallest_dividable_num(n:int) -> int:
    '''
    Returns the smallest integer whose dividable by all the numbers from 1 to n.
    :param n: an integer upper bound which we want to compute the smallest divisable num from 1 to it.
    '''
    prime_dict = {}
    for i in range(1,n+1):
        tmp_dict = prime_factorization(i)
        for k,_ in tmp_dict.items():
            if k not in prime_dict or prime_dict[k] < tmp_dict[k]:
                prime_dict[k] = tmp_dict[k]
    res = 1
    for k,v in prime_dict.items():
        res = res*(k**v)
    return res

And here's a small test case Ive written for this code:

from ProjectEuler.problem5 import smallest_dividable_num

if __name__ == "__main__":
    # The prime factorization is {2**3,3**2,5,7} -> returns 2520
    print(smallest_dividable_num(10))
    # The prime factorization is {2**2,3,5} -> returns 60
    print(smallest_dividable_num(6))
    # The prime factorization is {2**4,3**2,5,7,11,13,17,19} -> returns 232792560
    print(smallest_dividable_num(20))

Let me know your thoughts about my solution. thanks :)

Handling values not in dict in prime_factorization

In 2 places in prime_factorization, we end up performing dictionnary lookup to set the default value 0 when needed.

There are various other ways to do so:

We could use a variable to store the coefficient for the current divisor:

    # dealing with even prime numbers - e.g. 2
    d = 2
    coef = 0
    while n % d == 0:
        coef += 1
        n = n // d
    if coef:
        prime_dict[d] = coef

    # dealing with odd numbers
    for d in range(3, int(sqrt(n))+1):
        coef = 0
        while n % d == 0:
            coef += 1
            n = n // d
        if coef:
            prime_dict[d] = coef

(Note that if coef is not required if we do not mind having 0s in our dictionnary).

An alternative could be to use an appropriate data structure.

import collections

def prime_factorization(n: int) -> dict:
    '''
    Returns a dict with the prime factorization of the integer n, where the keys of the dict are the prime numbers
    and the values are the powers in the factorization for the appropriate prime number key.
    :param n: an integer we want to compute he's prime factorization
    '''
    prime_dict = collections.defaultdict(int)

    # dealing with even prime numbers - e.g. 2
    d = 2
    while n % d == 0:
        prime_dict[d] += 1
        n = n // d

    # dealing with odd numbers
    for d in range(3, int(sqrt(n))+1):
        while n % d == 0:
            prime_dict[d] += 1
            n = n // d

Or a Counter:

import collections

def prime_factorization(n: int) -> dict:
    '''
    Returns a dict with the prime factorization of the integer n, where the keys of the dict are the prime numbers
    and the values are the powers in the factorization for the appropriate prime number key.
    :param n: an integer we want to compute he's prime factorization
    '''
    prime_dict = collections.Counter()

    # dealing with even prime numbers - e.g. 2
    d = 2
    while n % d == 0:
        prime_dict[d] += 1
        n = n // d

    # dealing with odd numbers
    for d in range(3, int(sqrt(n))+1):
        while n % d == 0:
            prime_dict[d] += 1
            n = n // d

However, my preference is to completely split the concerns between the logic generating the prime divisors and the one counting them. Using a generator, we can get something like:

import collections

def yield_prime_factors(n: int):
    '''Yield prime factors.'''
    d = 2
    while n % d == 0:
        yield d
        n = n // d

    # dealing with odd numbers
    for d in range(3, int(sqrt(n))+1):
        while n % d == 0:
            yield d
            n = n // d

    # In case n is prime by itself
    if n > 1:
        yield n

def prime_factorization(n: int) -> dict:
    '''
    Returns a dict with the prime factorization of the integer n, where the keys of the dict are the prime numbers
    and the values are the powers in the factorization for the appropriate prime number key.
    :param n: an integer we want to compute he's prime factorization
    '''
    return collections.Counter(yield_prime_factors(n))

Iterating over keys and values in smallest_dividable_num

You use for k,_ in tmp_dict.items() which dumps the values from the dict and use tmp_dict[k] which gets the values from the dict.

You could as well just write:

    tmp_dict = prime_factorization(i)
    for k, v in tmp_dict.items():
        if k not in prime_dict or prime_dict[k] < v:
            prime_dict[k] = v

which can also be written:

    for k, v in prime_factorization(i).items():
        if k not in prime_dict or prime_dict[k] < v:
            prime_dict[k] = v

Using max and default values

The snippet just above could be written by taking advantage of the Python builtins:

        prime_dict[k] = max(v, prime_dict.get(k, 0))

Using in place operator

You can write things such as:

        n //= d

or

    res *= (k**v)

to avoid repeating the left member of the operand.

A different algorithm

The algorithm is all about computing Least Common Multiple which can be easily computed with the Greatest Common Divisor.

When this is implemented and tests cases are rewritten to ensure benchmark gives relevant timing:

from timeit import default_timer as timer
import functools

def gcd(a, b):
    """Computes gcd for 2 numbers."""
    while b:
        a, b = b, a % b
    return a

def lcm(a, b):
    """Computes lcm for 2 numbers."""
    return a * b // gcd(a, b)

def lcmm(*args):
    """Computes lcm for numbers."""
    return functools.reduce(lcm, args)

def smallest_dividable_num(n:int) -> int:
    '''
    Returns the smallest integer whose dividable by all the numbers from 1 to n.
    :param n: an integer upper bound which we want to compute the smallest divisable num from 1 to it.
    '''
    return lcmm(*range(1, n + 1))

if __name__ == "__main__":
    start = timer()
    for i in range(20):
        assert smallest_dividable_num(10) == 2520
        assert smallest_dividable_num(6) == 60
        assert smallest_dividable_num(20) == 232792560
    for i in range(1, 40):
        smallest_dividable_num(i)
    end = timer()
    print(end - start)


Handle builtin commands I've written a small C function to handle the builtin commands of a custom shell that I'm writing in C. Is it better to use a switch instead?

int handleBuiltinCommands(char *input, int ret) {
    int built_in_command = 0;
    if (strcmp(input, "exit") == 0) {
        free(input);
        exit(0);
    }
    if (StartsWith(input, "cd")) {
        built_in_command = 1;
        runcd(input);
    }
    if (StartsWith(input, "checkEnv")) {
        built_in_command = 1;
        checkEnv(ret);
    }
    return built_in_command;
}

I compile it with gcc -pedantic -Wall -ansi -O3

(Background: Tokenizing a shell command)

It's OK to use the if construction, although if you plan to have more than a few commands, using switch leads to cleaner code imho. In addition, why not using if else instead of just if? That way not all of your if have to be evaluated (in average).


Fuzzy Humanize date using Moment.js for datatable searching In my JS project, I am using datatable plugin to show data where I have a date column which users can filter/search.

In order to help users to search using a logical string such as today,  thismonth , lastweek I am writting a fucntion which uses MomentJs.

I am looking at the most efficient way of writing this fuzzyDate function.

function fuzzyDate(mydate){
  var fuzzy1 = ''; var fuzzy2 = ''; var fuzzy3 = '';

  var todaydt = moment(moment().format('YYYY-MM-DD'));
  var daydiff = mydate.diff(todaydt,'day');
  if (daydiff==-1){fuzzy1='Yesterday'}
  if (daydiff==0){fuzzy1='Today'}
  if (daydiff==1){fuzzy1='Tomorrow'}

  var weekdiff = mydate.diff(todaydt,'week');
  if (weekdiff==-1){fuzzy2='LastWeek'}
  if (weekdiff==0){fuzzy2='ThisWeek'}
  if (weekdiff==1){fuzzy2='NextWeek'}

  var monthdiff = mydate.diff(todaydt,'month');
  if (monthdiff==-1){fuzzy3='LastMonth'}
  if (monthdiff==0){fuzzy3='ThisMonth'}
  if (monthdiff==1){fuzzy3='NextMonth'}

  return fuzzy1 + ' ' + fuzzy2 + ' ' + fuzzy3;
}

console.log( '2018-06-01 - ', fuzzyDate(moment('2018-06-01','YYYY-MM-DD')) ) ;
console.log( '2018-07-01 - ', fuzzyDate(moment('2018-07-01','YYYY-MM-DD')) ) ;
console.log( '2018-07-07 - ', fuzzyDate(moment('2018-07-07','YYYY-MM-DD')) ) ;
console.log( '2018-07-10 - ', fuzzyDate(moment('2018-07-10','YYYY-MM-DD')) ) ;
console.log( '2018-07-11 - ', fuzzyDate(moment('2018-07-11','YYYY-MM-DD')) ) ;
console.log( '2018-07-12 - ', fuzzyDate(moment('2018-07-12','YYYY-MM-DD')) ) ;
console.log( '2018-07-13 - ', fuzzyDate(moment('2018-07-13','YYYY-MM-DD')) ) ;
console.log( '2018-07-15 - ', fuzzyDate(moment('2018-07-15','YYYY-MM-DD')) ) ;
console.log( '2018-07-30 - ', fuzzyDate(moment('2018-07-30','YYYY-MM-DD')) ) ;
console.log( '2018-08-30 - ', fuzzyDate(moment('2018-08-30','YYYY-MM-DD')) ) ;
<script src="https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.22.2/moment.min.js"></script>

Output of the above function

2018-06-01 - LastMonth
2018-07-01 - LastWeek ThisMonth
2018-07-07 - ThisWeek ThisMonth
2018-07-10 - ThisWeek ThisMonth
2018-07-11 - Yesterday ThisWeek ThisMonth
2018-07-12 - Today ThisWeek ThisMonth
2018-07-13 - Tomorrow ThisWeek ThisMonth
2018-07-15 - ThisWeek ThisMonth
2018-07-30 - ThisMonth
2018-08-30 - NextMonth

What if you have three functions that each calculate the string on its own? Therefore:

function days_diff(daydiff) {
  if (daydiff==0) return 'Today'
  if (daydiff==1) return 'Tomorrow'
  if (daydiff==-1) return 'Yesterday'
  return null
}
function weeks_diff(daydiff) {
  if (daydiff > 0 && daydiff < 7) return 'Thisweek'
  if (daydiff < 7) return 'Nextweek'
  return null
  ...

Then you can have your one aggregate function that calls the three of them...

Here's the much more complicated one from Rails that might provide guidance/inspiration: https://github.com/rails/rails/blob/2c97fbf6503c9199f3fe5ed06222e7226dc6fcd9/actionview/lib/action_view/helpers/date_helper.rb#L104