Phone keypad implementation I was wondering, beyond the standard improvements that this needs, is there a more "clever" way of solving this problem?

The problem is to translate letters into digits without changing the format of the original string. The conversion is based on the international standard letter/number mapping on phones (i.e. where ABC is 2, DEF is 3, ..., PQRS is 7, TUV is 8, and WXYZ is 9).

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

        System.out.print("Enter a string: ");
        String s = input.nextLine();

        System.out.println(getNumbers(s));
    }

    public static String getNumbers(String s) {
        String result = new String();

        //Read and append s onto result
        for (int i = 0; i < s.length(); i++) {
            if (Character.isLetter(s.charAt(i))) {
            result += getNumber(Character.toUpperCase(s.charAt(i)));
            }
            else {
                result += s.charAt(i);
            }
        }
        return result;
    }

    public static int getNumber(char upperCaseLetter) {
        int number = ((upperCaseLetter - 'A') / 3) + 2;
        if (number < 7) {
            return number;
        }
        else if (upperCaseLetter - 'A' < 20) {
            return 7;
        }
        else if (upperCaseLetter - 'A' < 23) {
            return 8;
        }
        else {
            return 9;
        }
    }
}

A much simpler solution would be to use a lookup table.  A mapping implemented using a lookup table would be easier to understand, with no magic numbers.

Furthermore, phone numbers are more numeric strings than numbers, so I'd make it a char-to-char lookup.

Repeated string concatenation is bad for performance.  To compose a string, use a StringBuilder.

public class Keypad {
    private static final char[] DIGITS = (
                 // ABC     DEF
                   "222" + "333" +
        // GHI      JKL     MNO
          "444"  + "555" + "666" +
        // PQRS     TUV     WXYZ
          "7777" + "888" + "9999").toCharArray();

    public static String getNumbers(CharSequence s) {
        StringBuilder result = new StringBuilder(s.length());
        for (int i = 0; i < s.length(); i++) {
            char c = Character.toUpperCase(s.charAt(i));
            if ('A' <= c && c <= 'Z') {
                result.append(DIGITS[c - 'A']);
            }
        }
        return result.toString();
    }
}


Automated Java fighting and battling simulation This is my second post since I've started coding/joined this site so be ready for some bad code and beginner questions!

I am just playing around with Java right now, as I believe this is a fun way to learn the language, so I decided to make a simple fight simulator. The simulator is meant to have each player start with a set amount of health (hitPoints) and they take turns hitting each other. Their damage dealt determined by a random number roll.

I just started trying to use multiple classes to make my code easier to ready and more. Correct I guess.

I have a question but also hope someone can give me pointers on how to improve my code in some ways. My questions is. I want to make it so when a player hits 0 health the program goes to the outCome() method but it currently allows 1 more hit after a player loses. So if the person who attacks first dies, they get 1 more hit in before the program ends but this is wrong and I'm not sure how to fix this. Here is my code.

This is my main class that runs everything.

package com.company;
import java.util.Scanner;

public class Battle {

    public static Scanner scan = new Scanner(System.in);

    public static void main(String[] args) {
        System.out.println("Please enter your username.");
        Players.userName = scan.nextLine();

        System.out.println(Players.userName + ", get ready for battle..");
        System.out.println("Press enter when ready.");
        scan.nextLine();

        while(Players.pHP > 0 && Players.oHP > 0) {
            Game.fight();
        }

        Game.outCome();
    }

}

This is my Player class which I believe is very unnecessary right now and would like some feedback on how to make it more useful than it is. This class feels entirely wrong to me.

package com.company;

public class Players {

    static String userName;
    static int hitPoints = 99;
    static int oHP = opponentHP();
    static int pHP = playerHP();

    public static int playerHP() {
        return pHP = hitPoints;
    }

    public static int opponentHP() {
        return oHP = hitPoints;
    }

}

And finally my Game class which kind of has the game code in it. This class looks messy to me and I believe I will improve on my organization with practice.

package com.company;

import java.util.Random;

public class Game {

    public static Random r = new Random();
    private static boolean isTurn = true;

    public static void fight() {
        int damage = r.nextInt(20);

        try {
            Thread.sleep(1000);
        } catch(InterruptedException ex) {
            Thread.currentThread().interrupt();
            System.out.println("INTERUPTED DELAY");
        }

        if(isTurn) {
            Players.oHP -= damage;
            if(Players.oHP < 0) {
                Players.oHP = 0;
            }
            System.out.println("You hit a(n) " + damage + " and now your
    opponent has " + Players.oHP + " HitPoints.");
            isTurn = false;
        } else {
            Players.pHP -= damage;
            if(Players.pHP < 0) {
                Players.pHP = 0;
            }
            System.out.println("Your opponent hit a(n) " + damage + " and now
    you have " + Players.pHP + " HitPoints.");
            isTurn = true;
        }
    }

    public static void outCome() {

        if (Players.oHP == 0 || Players.oHP < 0) {
            victory();
        } else {
            defeat();
        }
    }

    private static void victory() {
        System.out.println("You are the ultimate warrior.");
        System.out.println("Thanks for fighting..........");
    }

    private static void defeat() {
        System.out.println("You have been slain.");
        System.out.println("Thanks for fighting..........");
    }

}

Hi thanks for sharing your code!

Players class

Creating a class to represent a Player is a great idea, however your current class represents both players. If you want to have 2 players in your game, it might make more sense to have a Player class rather than a Players class.

Also try and expose the bare minimum that is possible by making as much private as possible. Don't reveal implementation details, only expose what is absolutely required.

Instantiating your players could look like this.

int hp = 100;
Player playerOne = new Player(playerName, hp);
Player playerTwo = new Player("Opponent", hp);

Where possible avoid using classes as a way of storing global / static variables. The benefit of creating classes is to provide a method of abstraction.

On top of this Player class your Game class is essentially just some static methods that you call from the main method. What if your code could look like this

Game game = new Game(player1, player2);
game.fight();

I re-wrote a version of your program, hopefully you find this useful.

Main

import java.util.Scanner;

public class Main {

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

        System.out.println("Please enter your username.");
        String userName = scan.nextLine();

        System.out.println("Please enter initial hp.");
        int hp = scan.nextInt();

        Player human = new Player(userName, hp);
        Player opponent = new Player("Opponent", hp);

        Game game = new Game(human, opponent);
        game.fight();
    }
}

Player

import java.util.Random;

public class Player {
    private final String userName;
    private int hp;

    public Player(String userName, int initialHp) {
        this.userName = userName;
        hp = initialHp;
    }

    public void attack(Player enemy) {
        Random rnd = new Random();
        int damage = rnd.nextInt(21) + 1;
        enemy.dealDamage(damage);
        System.out.println(getUserName() + " dealt " + damage + " to " + enemy.getUserName());
        System.out.println(enemy.getUserName() + " has " + enemy.getHp() + " hit points left!");
    }

    public void dealDamage(int damage) {
        hp -= damage;
        hp = hp < 0 ? 0 : hp;
    }

    public boolean isDead() {
        return hp <= 0;
    }

    public String getUserName() {
        return userName;
    }

    public int getHp() {
        return hp;
    }
}

Game

import java.util.Random;

public class Game {

    private final Player humanPlayer;
    private final Player aiPlayer;

    public Game(Player humanPlayer, Player aiPlayer) {
        this.humanPlayer = humanPlayer;
        this.aiPlayer = aiPlayer;
    }

    public void fight() {
        try {
            Thread.sleep(1000);
        } catch (InterruptedException ex) {
            Thread.currentThread().interrupt();
            System.out.println("INTERUPTED DELAY");
        }

        while (true) {
            humanPlayer.attack(aiPlayer);
            if (aiPlayer.isDead()) {
                System.out.println("Opponent is dead!");
                victory();
                return;
            }

            aiPlayer.attack(humanPlayer);
            if (aiPlayer.isDead()) {
                System.out.println("You are dead!");
                defeat();
                return;
            }
        }
    }

    private void victory() {
        System.out.println("You are the ultimate warrior.");
        System.out.println("Thanks for fighting..........");
    }

    private void defeat() {
        System.out.println("You have been slain.");
        System.out.println("Thanks for fighting..........");
    }

}

Some changes of note and general advice

Added some logic to the Player class where I thought it would be reasonable to do so. These should hopefully make the code more readable too.

The return statement in the method when either player dies will exit immediately.

Made it so each player has a different random number. Otherwise here the Player would win every time since they hit first.

I would recommend making values final if possible. E.g. the userName or the Random object.

Don't blindly make getters and setters for everything (this is a commonly encouraged practice especially when you're first learning to code), make getters only if you need to and setters if you really need to mutate the value.

Don't make member variables public. This makes it very hard to change your code. Use getters if you need to access the values.

small thing here, the line if (Players.oHP == 0 || Players.oHP < 0) could simply be replaced with if(Players.oHP <= 0)

A class should be considered a blue print for making an object, not a way of accessing all information about all the objects.

Hopefully this review was helpful for you, keep it up!


Chain of responsibility pattern class ReportFormat(object):
    PDF = 0
    TEXT = 1

class Report(object):
    """Strategy context."""

    def __init__(self, format_):
        self.title = 'Monthly report'
        self.text = ['Things are going', 'really, really well.']
        self.format_ = format_

class Handler(object):

    def __init__(self):
        self.nextHandler = None

    def handle(self, request):
        self.nextHandler.handle(request)

class PDFHandler(Handler):

    def handle(self, request):
        if request.format_ == ReportFormat.PDF:
            self.output_report(request.title, request.text)
        else:
            super(PDFHandler, self).handle(request)

    def output_report(self, title, text):
        print ''
        print '  '
        print '    <title>%s</title>' % title
        print '  '
        print '  '
        for line in text:
            print '
%s
' % line
        print '  '
        print ''

class TextHandler(Handler):

    def handle(self, request):
        if request.format_ == ReportFormat.TEXT:
            self.output_report(request.title, request.text)
        else:
            super(TextHandler, self).handle(request)

    def output_report(self, title, text):
        print 5*'*' + title + 5*'*'
        for line in text:
            print line

class ErrorHandler(Handler):

    def handle(self, request):
        print "Invalid request"

if __name__ == '__main__':
    report = Report(ReportFormat.TEXT)
    pdf_handler = PDFHandler()
    text_handler = TextHandler()

    pdf_handler.nextHandler = text_handler
    text_handler.nextHandler = ErrorHandler()

    pdf_handler.handle(report)

O/P:

*****Monthly report*****
Things are going
really, really well.

I would not use Chain of Responsibility to solve this problem. For every request, a single handler is chosen based on the same property: it's output format. For that you can use a simple dispatch table: a dict mapping each output format to its handler. There's no point asking each handler if it can handle the request.

Update: As usual, the Portland Pattern Repository has some good historical discussion of this pattern, specifically when not to use it:

  Do not use Chain of Responsibility when each request is only handled by one handler, or, when the client object knows which service object should handle the request.

Note: Report and ReportFormat are unchanged from the OP and omitted for brevity.

class ReportDispatcher(object):
    def __init__(self):
        self.reports = {}

    def add(self, report):
        self.reports[report.format] = report;

    def handle(self, request):
        report = self.reports[request.format_]
        if report:
            report.handle(request)
        else
            print "Invalid request"

class Handler(object):

    def __init__(self, format):
        self.format = format

    def handle(self, request):
        print "subclass responsibility"

class PDFHandler(Handler):

    def __init__(self):
        super(PDFHandler, self).__init__(ReportFormat.PDF)

    def handle(self, request):
        print ''
        print '  '
        print '    <title>%s</title>' % request.title
        print '  '
        print '  '
        for line in request.text:
            print '
%s
' % line
        print '  '
        print ''

class TextHandler(Handler):

    def __init__(self):
        super(TextHandler, self).__init__(ReportFormat.TEXT)

    def handle(self, request):
        print 5*'*' + request.title + 5*'*'
        for line in request.text:
            print line

if __name__ == '__main__':
    reports = ReportDispatcher()
    reports.add(PDFHandler())
    reports.add(TextHandler())

    report = Report(ReportFormat.TEXT)
    reports.handle(report)

As you can see from the main method, sending a report request is completely decoupled from the various report handlers. Instead, the request is sent to the dispatcher which knows about the handlers.


Login code probably not secure I'm new to PHP security and have made this login file:

<?php
header('Content-type: application/javascript');

$hostname_localhost = "";
$database_localhost = "";
$username_localhost = "";
$password_localhost = "";

$salt1 = "";
$salt2 = "";

$localhost = mysql_connect($hostname_localhost,$username_localhost,$password_localhost)
or
trigger_error(mysql_error(),E_USER_ERROR);

mysql_select_db($database_localhost, $localhost);

$username = clean($_POST['username']);
$password = $_POST['password'];

$sha1_password = sha1($salt1.$password.$salt2);

$query_search = "select * from users where username = '".$username."' AND     password = '".$sha1_password. "'";

$query_exec = mysql_query($query_search) or die(mysql_error());
$rows = mysql_num_rows($query_exec);

if($rows == 0) {
    echo "No Such User Found";
} else {
    $result = mysqli_query($localhost,"select * from users where username =     '".$username."'");

    while ($row = mysqli_fetch_array($result)) {
        echo $row['id'];
    }

}

function clean($string){
    if (get_magic_quotes_gpc()) $string = stripslashes($string);
    return mysql_real_escape_string($string);
}

?>

You'll probably all find a million things wrong with it but I need all the help I can get!

Warning
This is a code-review (CR) site. Though my answer/review may seem harsh at points, it is my firm belief that CR can, and should, be tough. I've explained my reasoning at length here.
It is not my intention to hurt yours or anyone else's feelings, be advised that some criticisms may be phrased badly, or bluntly. It is important, then, to keep in mind that my only goal is to help.

Due to the fact that much of CR is criticizing other peoples code, you may get the impression that I see myself as the light of the world, and only source of
"good code". Not so. Some critiques are subjective (such as which extension I recommend: PDO and not mysqli_*). TMTOWTDI and to each his own.

The first thing that is "bad" (as in: not good practice), is that you're not using the right MySQL extension. What is in fact outright wrong is that you're not consistently using a single extension throughout. Most of the time, you're using the mysql_* extension (which is really, really bad, mkay). But then out of the blue you have this, shall we say, moment of clarity, and you use the MySQL Improved extension (or mysqli_*):

//all your code using mysql_*
$result = mysqli_query($localhost,"select * from users where username =     '".$username."'");
while ($row = mysqli_fetch_array($result)) {
    echo $row['id'];
}
//reverting back to deprecated extension

The help section of this site specifically request you post actual, working code to review. If you need this code fixed/debugged: set about trying yourself and, if you can't seem to get it to work, post a question on SO.
Please clarify (by editing your posted code) which extension you're actually using. Also mention any notices, errors or warnings you get, if any.
Regardless of what extension you choose: make sure it's not deprecated, use it as was intended and be consistent.

The second thing is the use of magic quotes. Like the mysql_* extension, these have been deprecated for quite some time and have been removed since PHP 5.4.0. Do not use them. Ever. No, seriously. Stop. I'm not joking. Honest.

Third item on the agenda: at no point are you bothering to check if there is any POST data to be processed, You just blindly assume $_POST['username'] will be there. That can issue notices (undefined index E_NOTICE).
Adding an if (!isset($_POST['username']) || !isset($_POST['password'])) exit(); isn't a lot of work, and prevents all this.
I would assume you're include-ing this file as part of a bigger script, and in case that script does check for isset params, then you should be good, but if that's the case: be careful with that header business. If the headers are already sent, you'll get an error.

Which brings us neatly on to point four: header('Content-type: application/javascript');? Why? What? How? Why are you setting a header that makes no sense? As far as I can tell, you're only echoing text, a more suitable header, then, would be:

header('Content-Type: text/plain');

If you are going to add markup to the output, change that header to:

header('Content-Type: text/html');

Fifth: mysql_connect(...) or trigger_error(mysql_error(),E_USER_ERROR); vs $query_exec = mysql_query($query_search) or die(mysql_error());. Why are you using that horrible or? and why use trigger_error the first time 'round, only to then switch to that equally horrible die?
If the db connection failed, then the app failed. throw new RuntimeException('Could not connect to db'); from a function that returns a db connection is what you could do, or simply exit(1);
Whatever you do: be consistent.

Lastly, and most importantly: as a result of the things mentioned above, and the fact you're simply concatenating barely validated POST data into a query: You are vulnerable to injection attacks. This site explains what they are.
An easy, reliable and safe way to prevent this sort of attack is to use prepared statements. An example of how this is done with PDO would be:

$dsn = 'mysql:host=127.0.0.1;dbname=default_db';
$user = 'root';
$pass = 'DoNotUseRootUserIfYouDoNotNeedRootAccess';
$pdo = new PDO(
    $dsn,
    $user
    $pass,
    array(//more options here
        PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_OBJ
    )
);
//no quotes, no data, just the query as-is
$stmt = $pdo->prepare(
    'SELECT *
     FROM users
     WHERE username = :uname
       AND password = :pass'
);
$stmt->execute(
    array(
        ':uname' => $_POST['username'],
        ':pass'  => sha1($salt1.$_POST['password'].$salt2)
    )
);
if (!($row = $stmt->fetch()))
{
    echo 'User not found';
}
else
{
    do
    {
        echo $row->id, PHP_EOL;
    }while($row = $stmt->fetch());
}

Just browse through the documentation for the PDO class, or work out how to use prepared statements using the mysqli_* extension. It's not very hard, and I'd be happy to review your attempts at it, but I don't really like mysqli all too much, so I'm not posting an example here.
To help you choose between the two extensions, there's a special choose-an-api page can be found here

The reason why I prefer PDO, in case you are wondering is simply because its API is clean, and consistent: it's OO-only, whereas mysqli_* offers both a procedural and OO API. That, to my eye, makes for messy code.
mysqli_* uses a bit too much pass-by-reference in its methods:

$foo = 123;
$mysqliStmt->bindParam('i', $foo);//REFERENCE to $foo
$foo = 678;
$mysqliStmt->execute();//will use CURRENT value of $foo -> 678

It, once again, has to do with being consistent.

Last thoughts:
Given that you're still new to PHP, I'll leave it at this. Spend some time on the doc pages, and if it all gets a bit much, perhaps look at the unofficial coding standards: at PHP-FIG. All major players (Zend, Symfony, Doctrine, Drupal, Joomla... the list is on the site) subscribe to these standards. Since I've repeated it many times before here, if you want to be consistent, too: try and adopt these coding standards.


Getting the latest hash from a file I'm trying to write clean code, but I'm not so good with exception handling.
How can I improve this method? Or is it good enough already?

This method gets a hash from a file.

public String getLatestHash() {
    String hash = "none";

    URL packURL = null;
    try {
        packURL = new URL(ConfigManager.get("play_url") + "/" + HTMLParser.getPackName());
    } catch (MalformedURLException e) {
        logger.severe("invalid URL!");
        logger.severe(e.getMessage());
    }

    try (InputStream inputStream = packURL.openConnection().getInputStream()) {
        hash = DigestUtils.md5Hex(inputStream);
    } catch (IOException e) {
        e.printStackTrace();
    }

    return hash;
}

Why do you use logger.severe(...) in your MalformedURLException, but in your IOException you use e.printStackTrace()? Pick one and stick with it; this can include, for example, totally removing the printStackTrace() and replacing it with a comment that explains that you don't need to log it because [reason]. However, I'd suggest logging everything. It's a lot easier to find bugs if you log everything.

try (InputStream inputStream = packURL.openConnection().getInputStream()) {

And when you get a MalformedURLException in the try/catch before, your user is going to receive a very confusing NullPointerException instead of a more palatable (possibly custom) exception that means something in context. You can always catch (NullPointerException e) { throw /*your exception here*/; } to 'change' the exception being thrown (though be sure to use better indenting)

You define packURL right before you use it, but you define hash way at the beginning. Be consistent -- move String hash = "none"; to the line just before the relevant try.

Why is the default hash "none"? Wouldn't an empty string make more sense?

Aside from that, though, it looks about as good as it'll get. The multiple separate try/catch blocks are messy, but as far as I can tell that's the best way to do it.


OpenMP parallel for critical section and use of flush I am not sure about the place where flush should be used (if it is used at all here).

int Window_Shift :: get_window_shift()
{
    int window_shift = INFINITE;

    #pragma omp parallel for
    for(unsigned int node_id = 0;node_id< no_of_valid_nodes ;node_id++)
    {
        //for all the nodes that can be extended
        //finds the minimum possible window shift
        if(can_node_determine_window_shift(node_id) == true)
        {
            int node_window_shift = get_window_shift_for_node(node_id);
            #pragma omp flush(window_shift)
            #pragma omp critical(get_window_shift_for_node)
            {
            window_shift = min(window_shift,node_window_shift);
            #pragma omp flush(window_shift)
            }
        }
    }

Please review the code, especially the flush() commands.  Should atomic be used instead of a critical section?

With just one function provided and some return information, it's hard to tell everything about it.  Since this is a member function with "get" in its name (meaning it's an accessor), and only local variables are being modified (no data members), then this should likely be a const function.

int Window_Shift :: get_window_shift() const {}

If you are actually modifying data members somewhere, then disregard this.
The whitespace in the for loop statement is a little inconsistent:

for(unsigned int node_id = 0;node_id< no_of_valid_nodes ;node_id++)

This is how it could look:

for (unsigned int node_id = 0; node_id < no_of_valid_nodes; node_id++)

In a conditional statement, you don't need to explicitly use true or false.  It's also safer to avoid this as it'll prevent a possible mismatch of = and ==.  This will cause bugs if the compiler doesn't warn you about it (although you should have compiler warnings turned up high).

This is the same as == true:

if (someConditional)

This is the same as == false:

if (!someConditional)

INFINITE isn't defined anywhere, so either this is your own definition, or this is supposed to be INFINITY from <cmath>.  You could instead use C++'s alternatives from <limits>.

If you must use int, which is finite, you can use std::numeric_limits::max() to get its maximum value, which is normally 231 -1:

int max = std::numeric_limits<int>::max();

If you can use a floating-point type (float or double), then you can get true infinity by using std::numeric_limits::infinity():

float infinity = std::numeric_limits<float>::infinity();

double infinity = std::numeric_limits<double>::infinity();

More information regarding infinity here.
You could consider defining private() and shared() for the variables, which goes inside of the parallel for directive. You'll also need default(none) so that the compiler will allow you to set this yourself.  This can give you more control over your code, as the compiler would otherwise have to determine what should be private and what should be shared.

  Should atomic be used instead of a critical section?

You may have to test this yourself, but atomic may be faster.  Critical sections, on the other hand, may lead to more serialization, especially if you have one in a loop.  Always try to avoid unnecessary serialization in parallel code. Atomics can also cause this, but usually not as greatly since it doesn't lock an entire code section for all of the threads, unlike a critical section.


Usersystem basic class I've recently picked up OOP in Java and I'm trying to use it for my PHP user system.

I've come up with the following generic.class. On my main site, I'll simply put include('generic.class.php'); at the top of the file before my content.

<?php
include_once('connect.class.php');

class User {

    private $id;
    private $username;
    private $email;

    public function __construct(){
        $this->id = $_SESSION['id'];
        $this->username = $_SESSION['username'];
        $this->email = $_SESSION['email'];
    }

}

class Generic {

    private $maintenance = false;

    public function __construct() {
        // Start session
        session_start();

        // Check maintenance status
        if($this->maintenance){
            include('maintenance.php');
            die();
        }

        if(user_logged_in()){
            $user = new User();
        }
    }
}

function user_logged_in(){
    if (isset($_SESSION['username'], $_SESSION['id'])){
        return true;
    } else {
        return false;
    }
}

$generic = new Generic();
$conn = new Conn();
?>

Echoing $user->username doesn't seem to work. Or is there no need to use a User class at all? (most PHP user systems I see don't have a User class)

Is this the right approach to using OOP and classes with a user system? If not, how should my classes be like?

Couple of points:

Utilize Dependency Injection for your constructor to loosely couple it's dependencies.
In OOP, functions are not used that often, but methods. user_logged_in() should reside in some sort of User authentication handler (Single Responsibility Principle). Where you could do something like $usrAuthService->isLoggedIn().
Give your classes meaningful names. Generic and Conn are really bad names. What is a "Generic" class? What is "Conn"? Connection? Connection of what? In a month when you come back to visit your code, you will not understand at a glance what is happening.
A constructor's primary purpose is to initialize the object.

$user->username doesn't work because your class properties are private.

You must realize that there are many different design patterns out there that programmers may adapt for their systems. User systems that have entity classes like User are more domain oriented (DDD).


Data binding with keydown I want to create a JS MVVM framework for learning and personal project.

jsFiddle

HTML:

<input type="text" mk="value=test" value="test" />

JS (markjs):

(function () {
    Mark = {};
    var mark = {
        text: {},
        value: {}
    };

    Mark.set = function (name, value) {
        for (var key in mark) {
            if (mark[key][name]) {
                mark[key][name].value = value;
                rejuvenate();
            }
        }
    }

    function rejuvenate() {
        for (var key in mark) {
            for (var name in mark[key]) {
                if (mark[key][name]) {
                    if (key === 'text') mark[key][name].node.innerText = mark[key][name].value;
                    else if (key === 'value') mark[key][name].node.value = mark[key][name].value;
                    else if (key === 'click') mark[key][name].node
                }
            }
        }
    }

    var elems = document.getElementsByTagName('*');
    for (var ii = 0; ii < elems.length; ii++) {
        if (elems[ii].getAttribute('mk')) {
            var marks = elems[ii].getAttribute('mk').split(';');
            if (marks.length > 0) {
                for (var jj = 0; jj < marks.length; jj++) {
                    var d = marks[jj].split('=');
                    if (d[0] === 'value' || d[0] === 'text') {
                        mark[d[0]][d[1]] = {
                            node: elems[ii],
                            name: d[1],
                            value: ''
                        };
                        if (d[0] === 'value') {
                            elems[ii].onkeydown = function (e) {
                                e = e || window.event;
                                var charCode = e.charCode || e.keyCode,
                                    character = String.fromCharCode(charCode),
                                    value = this.value;

                                var cmk = this.getAttribute('mk').split(';');
                                for (var cc = 0; cc < cmk.length; cc++) {
                                    var dd = cmk[cc].split('=');
                                    Mark.set(dd[1], value);
                                }
                            }
                        }

                    }
                }
            }
        }
    }
})();

Mark.set('test', 'Yoza');

I tried to create angular-model-like or ko-observable-like. I created a model name: test. If any mark (mk) with marker value=test update will update any element innerTextwith marker text=test. It's like KnockoutJS but for now I want to understand how it works. My problem is that input update is always late, not like Angular every type will update model.

I also provide Mark.set(name, value). It will update every marker with same name to that value.

Please help me to improve this code and review my code and concept.

I want to create a js MVVM framework

Oh no... not again. No, seriously. If this is for learning purposes or personal stuff, that's fine. If you plan to use this in real projects and production, you might consider picking up an existing framework or help improve an existing one.

  my problem is input update always late, not like angular every type will update model.

That's because during a keydown event, the value of the input hasn't been changed yet. Frameworks usually use a combination of different events (which differ across elements as well as browsers) to consistently get values whenever the UI state changes. For instance, radio buttons in older IE don't fire some event on selection (I think it was change) and frameworks work around it by detecting click instead.

  angular every type will update model

The approach of each framework is different. Angular uses "dirty checking", which is a periodic check of data changes, and re-rendering once necessary. Frameworks like Ember and Backbone rely on setters which, aside from setting values, recompute dependent entities like computed props, and the DOM.

var elems = document.getElementsByTagName('*');
for (var ii = 0; ii < elems.length; ii++) {
  if (elems[ii].getAttribute('mk')) {

This is a bad idea. It's fast in small cases, terrible when your page is huge. Consider narrowing down the initial set of elements instead of getting every single one of them and filtering.

<input type="text" mk="value=test" value="test" />

Your markup is also... too much. All we needed is to know where this input binds to. You'd only need to assign the binding. Additionally, when using frameworks, you seldom actually touch the values. Values come straight from the view-model. In this case, "test" shouldn't be necessary. The following should be fine:

<input type="text" data-mk="test" />

You should also consider when your data starts to contain objects and arrays. You'd then have to deal with data contexts, key path resolution, parsing the expressions, scoping. Your example works fine when your DOM only has one model to bind to. In real life, you could have a dozen of data sources and elements nesting each other.

elems[ii].onkeydown = function (e) {
    e = e || window.event;
    var charCode = e.charCode || e.keyCode,
        character = String.fromCharCode(charCode),
        value = this.value;

    var cmk = this.getAttribute('mk').split(';');
    for (var cc = 0; cc < cmk.length; cc++) {
        var dd = cmk[cc].split('=');
        Mark.set(dd[1], value);
    }
}

Like I said earlier, listening to events require combinations of events and may differ across elements and browsers. You could move this logic out of your main framework logic (maybe in a separate function, or an entirely separate module) that you could simply pull in and call.


Incrementing letter-spacing of one line of text I was asked to create a script which will increment the letter-spacing of one line of text, until its width matches that of the first line; for use in headline/subheadline text to maintain a block effect as illustrated:

The problem is, it's very slow.  Is there a faster way of calculating this?

$(window).resize(function() {
var h1,h2;
var x = 1;
h1 = $('#header').find('h1');
h2 = $('#header').find('h2');
h2.css( { 'letter-spacing':'1px' } );

while(h1.width() !== h2.width() && x < 1000) {
    if (x == 1000) { break; }
    h2.css( 'letter-spacing','+=0.1px' );
    x++;
}
} );

As with all things related to performance, you must measure.  Here's a jsperf with your original code in it and several other iterations: http://jsperf.com/set-text-width

You can do several things to speed it up considerably:

Cache h1.width() since it doesn't change.
Make your comparison be >= because you may never get ==.
Use a larger pixel increment until you get close, then step down to a smaller increment

Per jsperf, this is about 5x faster than your original code.

function matchSize3() {
    var h2,w;
    var x = 1;
    w = $('#header').find('h1').width();
    h2 = $('#header').find('h2');
    h2.css( { 'letter-spacing':'1px' } );

    while(h2.width() < w) {
        if (x++ > 1000) { break; }
        h2.css('letter-spacing', '+=1px');
    }

    for (var i = 0; i < 10; i++) {
        if (h2.width() <= w) break;
        h2.css('letter-spacing', '-=0.1px');
    }
}

And, here's an even faster version that just attempts to calculate the desired pixel spacing and starts with that.  This is 14x faster than your original.  I don't think the last two loops are probably needed at all.

function matchSize4() {
    var h2, w;
    w = $('#header').find('h1').width();
    h2 = $('#header').find('h2');
    h2.css( { 'letter-spacing':'1px' } );

    // try to calculate the best starting point
    var l = h2.text().length;
    var diff = w - h2.width();
    h2.css("letter-spacing", ((diff/l) + 1) + "px");

    // these may not be needed at all
    if (h2.width() < w) {
        for (var i = 0; i < 10; i++) {
            if (h2.width() >= w) break;
            h2.css('letter-spacing', '+=0.1px');
        }
    } else {
        for (var i = 0; i < 10; i++) {
            if (h2.width() <= w) break;
            h2.css('letter-spacing', '-=0.1px');
        }
    }
}

And here's a version that just calculates the desired letter-spacing with no iteration.  This is 27x faster than your original version:

function matchSize5() {
    var h2, w;
    w = $('#header').find('h1').width();
    h2 = $('#header').find('h2');

    // calc the desired spacing
    var len = h2.text().length;
    var diff = w - h2.width();
    h2.css("letter-spacing", (diff/(len - 1)) + "px");
}

For speed reasons, this one assumes the letter-spacing is initially 0.  If it won't reliably be zero, then you have to obtain the initial value and use that in the calculation or set it to zero initially.

And here is a jsFiddle of this one to show that it works: http://jsfiddle.net/jfriend00/trazumeh/


Getting the max value from a list of sums This is my version of the knapsack challenge and I was wondering if there was a way to do it so it doesn't go through the foreach statement so many times. At the minute it goes through it thousands of times to get he closes value to the target number, which takes long time to do, I was wondering if there was a way to either do it without foreach or make it quick with the foreach.

private void btnGenerate_Click(object sender, RoutedEventArgs e)
{
    string input;
    string bag;
    int goal;
    int count = 0;

    int element;
    List<int> finalResult = new List<int>();
    do
    {
        bag = txtBag.Text;
    }
    while (!int.TryParse(bag, out goal));
    input = txtGenerate.Text;
    string[] elementsText = input.Split(' ');
    List<int> elementsList = new List<int>();
    foreach (string elementText in elementsText)
    {

        if (int.TryParse(elementText, out element))
        {
            elementsList.Add(element);
        }
    }
    Solver solver = new Solver();
    List<List<int>> results = solver.Solve(goal, elementsList.ToArray());
    int closestValue = 0;
    foreach (List<int> result in results)
    {
        count++;
        int totalValue = result.Sum(x => Convert.ToInt32(x));

        if (totalValue >= closestValue)
        {
            closestValue = totalValue;
            finalResult = result;
            txtSum.Text = closestValue.ToString();

        }
    }

    foreach (int value in finalResult)
    {
        txtCount.Text = count.ToString();

        cboResult.ItemsSource = finalResult;

    }
}

I also have a class with this in:

public class Solver
{
    public List<List<int>> mResults;
    public int closestValue = 0;
    public List<List<int>> Solve(int goal, int[] elements)
    {
        mResults = new List<List<int>>();
        RecursiveSolve(goal, 0, new List<int>(), new List<int>(elements), 0);
        return mResults;
    }

    private void RecursiveSolve(int goal, int currentSum,
    List<int> included, List<int> notIncluded, int startIndex)
    {
        for (int index = startIndex; index < notIncluded.Count; index++)
        {
            int nextValue = notIncluded[index];
            if (currentSum + nextValue <= goal && currentSum + nextValue >= closestValue)
            {
                List<int> newResult = new List<int>(included);
                newResult.Add(nextValue);
                mResults.Add(newResult);
                closestValue = currentSum + nextValue;
            }
            if (currentSum + nextValue < goal)
            {

                List<int> nextIncluded = new List<int>(included);
                nextIncluded.Add(nextValue);
                List<int> nextNotIncluded = new List<int>(notIncluded);
                nextNotIncluded.Remove(nextValue);
                RecursiveSolve(goal, currentSum + nextValue,
                    nextIncluded, nextNotIncluded, startIndex++);
            }
        }
    }
}

You've got logic mixed in with your view.

Move as much of the code out of your button handler as you can, and into a separate function (and preferably a separate class).

A button handler should do just that, handle button presses. It should not then perform calculations.

Your spacing could use some work. For example:

    int element;
    List<int> finalResult = new List<int>();
    do
    {
        bag = txtBag.Text;
    }
    while (!int.TryParse(bag, out goal));
    input = txtGenerate.Text;
    string[] elementsText = input.Split(' ');
    List<int> elementsList = new List<int>();
    foreach (string elementText in elementsText)
    {

        if (int.TryParse(elementText, out element))
        {
            elementsList.Add(element);
        }
    }
    Solver solver = new Solver();
    List<List<int>> results = solver.Solve(goal, elementsList.ToArray());
    int closestValue = 0;
    foreach (List<int> result in results)
    {
        count++;
        int totalValue = result.Sum(x => Convert.ToInt32(x));

        if (totalValue >= closestValue)
        {
            closestValue = totalValue;
            finalResult = result;
            txtSum.Text = closestValue.ToString();

        }
    }

Would be a lot more readable like this:

    int element;
    List<int> finalResult = new List<int>();

    do
    {
        bag = txtBag.Text;
    }
    while (!int.TryParse(bag, out goal));

    input = txtGenerate.Text;
    string[] elementsText = input.Split(' ');
    List<int> elementsList = new List<int>();

    foreach (string elementText in elementsText)
    {

        if (int.TryParse(elementText, out element))
        {
            elementsList.Add(element);
        }
    }

    Solver solver = new Solver();
    List<List<int>> results = solver.Solve(goal, elementsList.ToArray());
    int closestValue = 0;

    foreach (List<int> result in results)
    {
        count++;
        int totalValue = result.Sum(x => Convert.ToInt32(x));

        if (totalValue >= closestValue)
        {
            closestValue = totalValue;
            finalResult = result;
            txtSum.Text = closestValue.ToString();

        }
    }

Use var when it is obvious from the RHS of a declaration what the type of the variable is, and always use var in foreach loops.

e.g.

    int element;
    var finalResult = new List<int>();

    do
    {
        bag = txtBag.Text;
    }
    while (!int.TryParse(bag, out goal));

    input = txtGenerate.Text;
    var elementsText = input.Split(' ');
    var elementsList = new List<int>();

    foreach (var elementText in elementsText)
    {

        if (int.TryParse(elementText, out element))
        {
            elementsList.Add(element);
        }
    }

    var solver = new Solver();
    List<List<int>> results = solver.Solve(goal, elementsList.ToArray());
    var closestValue = 0;

    foreach (var result in results)
    {
        count++;
        int totalValue = result.Sum(x => Convert.ToInt32(x));

        if (totalValue >= closestValue)
        {
            closestValue = totalValue;
            finalResult = result;
            txtSum.Text = closestValue.ToString();
        }
    }

Avoid Hungarian notation in C#, and prefer public properties over public fields. Additionally, public collections should be readonly, and you don't need to intiialize an integer as 0. Its default value is 0.

    private readonly List<List<int>> results

    public List<List<int>> Results
    {
        get
        {
            return results;
        }
    }

    public int ClosestValue {get; set;}


Dice rolling program which also counts and stores the result Is there any room for improvement, in terms of simplicity and efficiency?
from random import randint

class Dice():

    def __init__(self):
        """Making a dice instance with 6 sides."""
        self.sides = 6

class Roller(Dice):

    def __init__(self,name,age):
        """Making a Dice Roller Instance"""
        super().__init__()
        #inhert sides attribute of a dice
        self.name = name
        self.age = str(age)

    def describe_roller(self):
        """Describe the information of the roller"""
        print('name: ' + self.name.title())
        print('age: ' + self.age)

    def roll_multiple(self,t):
        """Roll the dice multiple times and keep count in a dict"""
        count = {}
        one = 0
        two = 0
        three = 0
        four = 0
        five = 0
        six = 0
        for i in range(0,t):
            key = randint(1, self.sides)

            if key == 1:
                one += 1
                count["one"] = one
            elif key == 2:
                two += 1
                count["two"] = two
            elif key == 3:
                three += 1
                count["three"] = three
            elif key == 4:
                four += 1
                count["four"] = four
            elif key == 5:
                five += 1
                count["five"] = five
            elif key == 6:
                six += 1
                count["six"] = six
        print(count)

mike = Roller('mike',25)
mike.roll_multiple(6)

Roller should not inherit from Dice, as Roller represents a human. Humans being a sub-class of Dice is pretty unintuitive. Rather, Roller should have an attribute: self.dice = Dice()
There's no particular reason Roller.age should be of type str. I'd keep it as an int and only convert to str if you need its string-represantation.
f-Strings are a really convenient tool for creating strings containing variables / expressions. They also handle converting to str for you. Example: print(f'age: {self.age}')
You don't need the empty parantheses in class Dice()
You should put a space after the comma between arguments in a function call: mike = Roller('mike',25) becomes mike = Roller('mike', 25)
roll_multiple should be a method of Dice that can be called by the Roller. This is part of an intuitive API. Users (including yourself) will probably expect to roll the dice, not the human. If you need Roller.roll_multiple, I'd only make it a wrapper for Dice.roll_multiple.
roll_multiple(self, t): t is not a particularly expressive variable name, something like times would be better for readability.
Adding type hints (PEP 484) increases the readability of your code.
It's a convention in Python to name variables, whose values you don't actually use, _. So for i in range(0,t): becomes for _ in range(t):. Notice that you don't need to provide zero as the first argument as it's the default start for range.
Passing sides as a default argument to the constructor of Dice makes the class a whole lot more versatile.

Algorithm
Hardcoding values like
one = 0
two = 0
three = 0
...

should always raise a red flag when writing code. This is neither necessary nor recommended in most situations. You already laid the groundwork by putting Dice.sides in an attribute, now we need to use it. You also don't need to keep a seperate count of the occured values outside of the dict. I implemented it to exactly mirror your functionality (i.e. using string-representations as keys for the dict and only counting numbers that actually occured), inside of class Dice:
from random import randint
from collections import defaultdict
from num2words import num2words

...

def roll(self) -> int:
    return randint(1, self.sides)

def roll_multiple(self, times: int) -> dict:
    count = defaultdict(int)

    for _ in range(times):
        number = self.roll()
        key = num2words(number=number)
        count[key] += 1

    return dict(count)

num2words is a third-party library, that needs to be installed first.
If having keys of type int is fine for your use case, I would recommend using them as they're way more usable for further calculations. You also don't necessarily need to cast to dict, as a defaultdict provides all the same basic functionality. It does make a difference when printing though. Here is the adjusted implementation:
def roll_multiple(self, times: int) -> defaultdict:
    count = defaultdict(int)

    for _ in range(times):
        key = self.roll()
        count[key] += 1

    return count

Suggested code
from random import randint
from collections import defaultdict

class Dice:
    def __init__(self, sides: int = 6):
        """Making a dice instance with a variable number of sides."""
        self.sides = sides

    def roll(self) -> int:
        """Returns the result of a single dice roll."""
        return randint(1, self.sides)

    def roll_multiple(self, times: int) -> defaultdict:
        """Provides the results of a variable number of dice rolls in a defaultdict."""
        count = defaultdict(int)

        for _ in range(times):
            key = self.roll()
            count[key] += 1

        return count

class Roller:
    def __init__(self, name: str, age: int) -> None:
        """Making a Dice Roller Instance"""
        self.name = name
        self.age = age

        self.dice = Dice()

    def describe_roller(self) -> None:
        """Describe the information of the roller"""
        print(f'name: {self.name.title()}')
        print(f'age: {self.age}')

    def roll_multiple(self, times: int) -> None:
        """Roll the dice multiple times and print the result"""
        print(self.dice.roll_multiple(times))


SQLAlchemy - InsertFromSelect with columns support In SQL there is no way to do an "INSERT... SELECT". If you want to do it without using raw SQL in several places of your code you can create custom SQL compilation.

There is an example about how to do "INSERT...SELECT" in SA documentation. This example doesn't support column in the INSERT part of the sentence, some thing like: INSERT into table(col1, col2)...".

I've modified the example to that, this support table ("INSERT INTO table (SELECT...)") or columns ("INSERT INTO table (col1, col2) (SELECT...)".

Please, have a look an comment :)

from sqlalchemy.sql.expression import Executable, ClauseElement
from sqlalchemy.ext.compiler import compiles

class InsertFromSelect(Executable, ClauseElement):
    def __init__(self, insert_spec, select):
        self.insert_spec = insert_spec
        self.select = select

@compiles(InsertFromSelect)
def visit_insert_from_select(element, compiler, **kw):
    if type(element.insert_spec) == list:
        columns = []
        for column in element.insert_spec:
            if element.insert_spec[0].table != column.table:
                raise Exception("Insert columns must belong to the same table")
            columns.append(compiler.process(column, asfrom=True))

        table = compiler.process(element.insert_spec[0].table)
        columns = ", ".join(columns)

        sql = "INSERT INTO %s (%s) (%s)" % (
                table, columns,
                compiler.process(element.select))

    else:
        sql = "INSERT INTO %s (%s)" % (
                compiler.process(element.insert_spec, asfrom=True),
                compiler.process(element.select))

    return sql

Example of its use with columns:

InsertFromSelect([dst_table.c.col2, dst_table.c.col1], select([src_table.c.col1, src_table.c.col1]))

Example of its use only with a table:

InsertFromSelect(dst_table, select(src_table]))

This works for me, but I want to hear other opinions.

columns.append(compiler.process(column, asfrom=True))

should be simply

columns.append(column.name)

and

table = compiler.process(element.insert_spec[0].table)

should be

table = compiler.process(element.insert_spec[0].table, asfrom=True)


Project Euler 29 - Distinct Powers I have just completed problem 29 from the Project Euler website:

  Consider all integer combinations of \$a^b\$ for \$2 \le a \le 5\$ and \$2 \le b \le 5\$:

  \$2^2=4\$, \$2^3=8\$, \$2^4=16\$, \$2^5=32\$

  \$3^2=9\$, \$3^3=27\$, \$3^4=81\$, \$3^5=243\$

  \$4^2=16\$, \$4^3=64\$, \$4^4=256\$, \$4^5=1024\$

  \$5^2=25\$, \$5^3=125\$, \$5^4=625\$, \$5^5=3125\$

  If they are then placed in numerical order, with any repeats removed, we get the following sequence of 15 distinct terms:

  \$4, 8, 9, 16, 25, 27, 32, 64, 81, 125, 243, 256, 625, 1024, 3125\$

  How many distinct terms are in the sequence generated by \$a^b\$ for \$2 \le a \le 100\$ and \$2 \le b \le 100\$?

I have written the code for this problem in Java. With my knowledge of Java, I believe it is as efficient as it can be, however, I think it can be improved further.

import java.util.ArrayList;
import java.math.BigInteger;
public class problemTwentyNine {
  public static void main(String [] args) {
    long start = System.nanoTime();
    ArrayList<BigInteger> distinctTerms = new ArrayList<BigInteger>();
    BigInteger temp;
    for(int i = 2; i <= 100; i++) {
      for(int j = 2; j <= 100; j++) {
        temp = BigInteger.valueOf(i).pow(j);
        if(!distinctTerms.contains(temp)) {
          distinctTerms.add(temp);
        }
      }
    }
    System.out.println("Number of distinct terms: " + distinctTerms.size());
    long stop = System.nanoTime();
    System.out.println("Execution time: " + ((stop - start) / 1e+6) + " ms");
  }
}

Output:

  Number of distinct terms: 9183

  Execution time: 306.462625 ms

Now although this is already fairly quick, after looking through some solutions in other languages, I have found that this problem can be solved in just 6 ms in C#. Now I know that it will not be possible in Java as we don't have built in methods like SortedSet.

SortedSet set = new SortedSet();
for (int a = 2; a <= 100; a++) {
    for (int b = 2; b <= 100; b++) {
        double result = Math.Pow(a, b);
        set.Add(result);
     }
 }

C# coded sourced form here.

I'd appreciate any suggestions for ways in which I can improve the performance of the Java solution.

The code is very suboptimal. Keep in mind that Project Euler problems are not about brute force.

You only need to count the distinct powers. You don't have to actually calculate them. Instead, you need to realize that

a base \$a\$ may generate a repeat only if it is a perfect power itself.
each base which is not a perfect power contributes the same amount of unique elements (that is, there is 98 powers of 2, 98 powers of 6, etc) and all such sets are fully disjoint.
sets of elements generated by \$a\$, \$a^2\$, ... \$a^{100}\$ and \$b\$, \$b^2\$, ... \$b^{100}\$ are also equipotent and disjoint.

So, to obtain a solution you need to find out

how many numbers below \$b\$ are not perfect powers (linear complexity),
how many elements are in each set (also linear complexity; think of unique numbers in a multiplication table)

and multiply the results.


Filter Duplicate Elements in Haskell I'm working on HackerRank to try to improve my Haskell skills along side with reading Haskell Programming from first principles. I wrote a program that works, but it seems to time out on large input sets. The purpose of the program is

Given a list of n integers a = [a1, a2, ..., an], you have to find those integers which are repeated at least k times. In case no such element exists you have to print -1.
If there are multiple elements in a which are repeated at least k times, then print these elements ordered by their first occurrence in the list.

So I wrote a few different functions to help with this.
count which counts the number of occurrences of an element in a list
count :: Eq a => Integral b => a -> [a] -> b
count e [] = 0
count e (a:xs) = (count e xs +) $ if a == e then 1 else 0

uniq which removes duplicates from a list
uniq :: Eq a => [a] -> [a] -> [a]
uniq x [] = x
uniq [] (a:xs) = uniq [a] xs
uniq x (a:xs) = if a `elem` x then uniq x xs else uniq (a:x) xs

filt which filters through a list and removes elements that don't occur at least k times.
filt :: Show a => Num a => Read a => Eq a => Integral b => [a] -> b -> [a]
filt a b = reverse $ uniq [] [i | i <- a, count i a >= b]

printList which prints a list as a space separated list or prints -1 if the list is empty.
printList :: Show a => [a] -> IO ()
printList [] = putStrLn "-1"
printList a = putStrLn $ unwords [show i | i <- a]

readNumbers which takes a space separated string and returns a Num list from that string.
readNumbers :: Show a => Eq a => Num a => Read a => String -> [a]
readNumbers = map read . words

run which throws all of this together and runs this n times.
run :: (Show a, Eq a, Num a, Read a) => a -> IO ()
run 0 = putStr ""
run n = do
  a <- getLine
  b <- getLine
  printList $ filt (readNumbers b) (readNumbers a !! 1)
  run $ n - 1

main the main function. It gets a number n and then calls run n.
main :: IO ()
main = do
  a <- getLine
  run $ read a

This code works, for example, with the input
3
9 2
4 5 2 5 4 3 1 3 4
9 4
4 5 2 5 4 3 1 3 4
10 2
5 4 3 2 1 1 2 3 4 5

and gives the desired output of
4 5 3
-1
5 4 3 2 1

However, with larger datasets this code is incredibly slow. I'm guessing it's because the recursion is less than optimal, but I can't really pinpoint what is taking so long. My best guess is that uniq or count is the limiting factor, but I can't figure out how to optimize them.

If you write uniq as a right fold, you don't need to pass an accumulator through, and the list comes out in the right order:

uniq :: Eq a => [a] -> [a]
uniq [] = []
uniq (x:xs) = (if x `elem` xs then id else (x:)) $ uniq xs

filt :: Show a => Num a => Read a => Eq a => Integral b => [a] -> b -> [a]
filt k is = uniq [i | i <- is, count i is >= k]

(Edit: Actually that one throws out the first of each two equal elements, not the last. Here`s one without that problem:

uniq :: Eq a => [a] -> [a]
uniq [] = []
uniq (x:xs) = x : uniq (filter (/=x) xs)

)

You've commendably already brought count into a form that allows it to be written in terms of library combinators:

count :: Eq a => Integral b => a -> [a] -> b
count e = sum . map (\a -> if a == e then 1 else 0)

That's a bit ugly due to lambdas though, here's a nicer version:

count e = length . filter (== e)

For separation of monadic and pure code (and generally for factoring out common code from across cases), here's a showList to replace printList:

showList :: Show a => [a] -> String
showList [] = "-1"
showList a = unwords [show i | i <- a]

Calling a monadic action a given number of times doesn't need manual recursion, and thus also doesn't need to give the repeated action a name:

main :: IO ()
main = do
  a <- readLn
  replicateM_ a $ do
    [_n, k] <- map read . words <$> getLine
    numbers <- map read . words <$> getLine
    putStrLn $ showList $ filt k numbers

(I think readNumbers doesn't deserve a name.)

In case the order in which the output is given isn't important, here's a version that doesn't require quadratic time because each element is compared to every other:

filt k = map head . filter ((>=k) . length) . group . sort

which relies on Data.Lists sort being faster than quadratic time.


Automatic variable generation from data table I am adjusting Fast Report to create some reports, and I have created a loop that runs through query results stored inside a datatable and assigns each TenantName to a variable. However, the code I wrote seems bulky and would get out of hand if I needed more variables.

Would there be a more sensible way to do this?

If cmbReports.Text = "Conditions Of Tenancy" Then
        Dim ListReport = New FastReport.Report
        ListReport.Load("C:\Users\richard\Documents\BMSSouthSide\BMSPanda\Reports\CONDITIONSOFTENANCY.frx")
        ListReport.SetParameterValue("CRMConnectionString", "Data ..... ")
        Dim strSql As String = "Select  TenantForename + ' ' + TenantSurname as FullName From Tenants Where '" & lblLeaseIDValue.Text & "' = LeaseID and PropertyID =" & lblPropertyIDValue.Text
        Dim dtb As New DataTable
        Dim ten1 As String
        Dim ten2 As String
        Dim ten3 As String
        Dim ten4 As String
        Dim ten5 As String
        Dim ten6 As String
        Dim ten7 As String
        Dim ten8 As String
        Dim ten9 As String
        Dim ten10 As String
        Dim ten11 As String
        Dim IntTenantID As Integer
        Using cnn As New SqlConnection(My.Settings.BMSSouthSideConnectionString)
            cnn.Open()
            Using dad As New SqlDataAdapter(strSql, cnn)
                dad.Fill(dtb)
            End Using
            Dim Ten As Integer = dtb.Rows.Count
            Do Until Ten = 0
                For Each row As DataRow In dtb.Rows
                    If Ten = 11 Then Ten1 = row("FullName")
                    If Ten = 10 Then Ten2 = row("FullName")
                    If Ten = 9 Then Ten3 = row("FullName")
                    If Ten = 8 Then Ten4 = row("FullName")
                    If Ten = 7 Then Ten5 = row("FullName")
                    If Ten = 6 Then Ten6 = row("FullName")
                    If Ten = 5 Then Ten7 = row("FullName")
                    If Ten = 4 Then Ten8 = row("FullName")
                    If Ten = 3 Then Ten9 = row("FullName")
                    If Ten = 2 Then Ten10 = row("FullName")
                    If Ten = 1 Then Ten11 = row("FullName")
                    Ten = Ten - 1T
                Next row
            Loop
            Dim strSql2 As String = "Select Top 1 TenantID From Tenants Where '" & lblLeaseIDValue.Text & "' = LeaseID and PropertyID =" & lblPropertyIDValue.Text
            Dim datatableTenantID As New DataTable
            Using Tenant As New SqlDataAdapter(strSql2, cnn)
                Tenant.Fill(datatableTenantID)
            End Using
            For Each IntTenant As DataRow In datatableTenantID.Rows
                IntTenantID = IntTenant("TenantID")
            Next
            cnn.Close()
        End Using
        Dim TenantList As String = Ten11 & " " & Ten10 & " " & Ten9 & " " & Ten8 & " " & Ten7 & " " & Ten6 & " " & Ten5 & " " & Ten4 & " " & Ten3 & " " & Ten2 & " " & Ten1
        ListReport.SetParameterValue("PropertyID", Val(lblPropertyIDValue.Text))
        ListReport.SetParameterValue("LeaseID", Val(lblLeaseIDValue.Text))
        ListReport.SetParameterValue("TenantID", IntTenantID)
        ListReport.SetParameterValue("TenantList", TenantList)
        ListReport.Show()
    End If

Use array, this will reduce by a lot your number of variables.
Name your variable properly ("ten" should be "tenant")
Normaly, variable starts with a lower case in .net
Do not concatenate your strings for queries, this will increase your chances of injections
Your datatable is a bit useless in your case, you can write directly to your variables with a datareader
Your listReport is only needed at the end, I think you could initialize it there instead
I would separate this into multiple functions. A "GetTenants" that returns the list, a "GetTenantID" and a "ShowReport". This way these function can be out of the UI and reused if needed.
Your report takes multiple tenant names but only one ID, this seems strange.

If cmbReports.Text = "Conditions Of Tenancy" Then

    Dim tenants As New List(Of String)
    Dim tenantID As Integer

    Dim propertyId As Integer
    Dim leaseId As Integer

    If Not Int32.TryParse(lblPropertyIDValue.Text, propertyId) Then
        ' Not an integer, this is an error
    End If

    If Not Int32.TryParse(lblLeaseIDValue.Text, leaseId) Then
        ' Not an integer, this is an error
    End If

    tenants = GetTenants(propertyId, leaseId)
    tenantID = GetTenantID(propertyId, leaseId)

    ShowReport(tenants, tenantID, propertyId, leaseId)

End If

Function GetTenants(ByVal propertyId As Integer, ByVal leaseId As Integer) As List(Of String)

    Dim tenants As New List(Of String)

    Using cnn As New SqlConnection(My.Settings.BMSSouthSideConnectionString)
        cnn.Open()

        Dim command As New SqlCommand("Select  TenantForename + ' ' + TenantSurname as FullName From Tenants Where LeaseID = @LeaseID and PropertyID = @PropertyID", cnn)

        command.Parameters.Add("@LeaseID", SqlDbType.Int).Value = leaseId
        command.Parameters.Add("@PropertyID", SqlDbType.Int).Value = propertyId

        Using reader As command.ExecuteReader()
            Do While reader.Read()
                tenants.Add(reader.GetString("FullName"))
            Loop
        End Using

        cnn.Close()
    End Using

    Return tenants
End Function

Function GetTenantID(ByVal propertyId As Integer, ByVal leaseId As Integer) As Integer

    Dim tenantID As Integer

    Using cnn As New SqlConnection(My.Settings.BMSSouthSideConnectionString)
        cnn.Open()

        Dim command As New SqlCommand("Select Top 1 TenantID From Tenants Where LeaseID = @LeaseID and PropertyID = @PropertyID", cnn)

        command.Parameters.Add("@LeaseID", SqlDbType.Int).Value = leaseId
        command.Parameters.Add("@PropertyID", SqlDbType.Int).Value = propertyId

        Using reader As command.ExecuteReader()
            If reader.Read() Then
                tenantID = reader.GetInt32("TenantID")
            End If
        End Using

        cnn.Close()
    End Using

    Return tenantID
End Function

Sub ShowReport(ByVal tenants As List(Of String), ByVal tenantID As Integer, ByVal propertyId As Integer, ByVal leaseId As Integer)

    Dim listReport = New FastReport.Report

    listReport.Load("C:\Users\richard\Documents\BMSSouthSide\BMSPanda\Reports\CONDITIONSOFTENANCY.frx")
    listReport.SetParameterValue("CRMConnectionString", "Data .....

    Dim tenantList As String = String.Join(" ", tenants.ToArray())

    ListReport.SetParameterValue("PropertyID", propertyId)
    ListReport.SetParameterValue("LeaseID", tenantID)
    ListReport.SetParameterValue("TenantID", tenantID)
    ListReport.SetParameterValue("TenantList", tenantList)
    ListReport.Show()

End Sub


Command Class & CommandCollection class I have written Command Class & CommandCollection class.

Every command will have instance of Command object and it get assigned in the CommandCollection object.

Usage:

$commandCollection = new CommandCollection();

$data = ['domain' => 'www.domain.com'];

// version 4 and 5 is belong to script_one.sh file under add-site name
$commandCollection->addCommand('add-site', 'path/script_one.sh', $data)
                   ->forVersion([4, 5]);

$commandCollection->addCommand('add-site', 'path/script_two.sh', $data)
                   ->forVersion([6]);

View the shell script of version 5 (path/script_one.sh)

echo $commandCollection->viewCommand('add-site', 5);

I will be using CommandCollection object in some of Manager classes such as SitesManager class and CronJobManager class

What do you think of my code and is there anything could be improved or refactored?

CommandCollection Class

class CommandCollection
{
    private $commandFiles;

    public function __construct()
    {
        $this->commandFiles = collect([]);
    }

    public function addCommand($name, $filename, $data)
    {
        $nameFilter = $this->commandFiles->where('name', $name)->where('filename', $filename);

        if (!$nameFilter->isEmpty()) {
            throw new \Exception('Already exists!');
        }

        $command = new Command($name, $filename, $data);
        $this->commandFiles->push($command);

        return $command;
    }

    public function viewCommand($name, $version)
    {
        foreach ($this->commandFiles as $commandFile) {
            if ($commandFile->name == $name && $commandFile->hasVersion($version)) {
                return $commandFile->render();
            }
        }
    }
}

Command Class

class Command
{
    protected $name;
    protected $filename;
    protected $version;

    protected $data;

    public function __construct($name, $filename, $data)
    {
        $this->name = $name;
        $this->filename = $filename;
        $this->data = $data;

        $this->version = collect();
    }

    public function forVersion($versions)
    {
        foreach ($versions as $version) {
            $this->version->push($version);
        }
    }

    public function render()
    {
        return view($this->filename, $this->data)->render();
    }

    public function hasVersion($number)
    {
        return $this->version->contains($number);
    }

    public function __get($property)
    {
        if (property_exists($this, $property)) {
            return $this->$property;
        }
    }
}

I would suggest that you instantiate your Command object and inject into CommandCollection via your addCommand() method.

$command = new Command('add-site', 'path/script_one.sh', $data);
$command->forVersion([4, 5]);
$commandCollection->addCommand($command);

This gives you clear separation of logic such that your collection no longer needs to understand how to instantiate the Command object.  It also removes the obfuscated logic of calling forVersion() on a Command object returned from addCommand().  Why would addCommand() be expected to return a Command object instead of simply adding a Command object to the collection?

Passing the Command dependency like this would also allow you to type hint that a valid Command object is passed. Right now, you are doing absolutely nothing to validate that that the parameters being passed to this method (or really all other public methods across these two classes) contain good values to work with.

Should data passed to forVersion() be information that is actually passed to the Command constructor, as opposed to being passed in a separate method?  Does the version data really need to be mutable on the object such that there should be a setter method like this? Are you allowing your Command objects to be instantiated into a bad or incomplete state?

Should you really be using Laravel Collection object as the basis for your collection class?  In other words, do you really want to you an array for your data structure here?  Note how in your methods you will have to repeatedly iterate over the collection/array to determine if certain entries are present, or to get their values.  Your addCommand() method performs an \$O(n^2)\$ search against your underlying Collection just to be able to add an entry.  Your viewCommand() requires an \$O(n)\$ iteration over the collection in order to return a Command file rendering.

This might not be a problem if you are only ever expecting to be working with a handful of entries in the collection, but will become a problem if you expect your collection to grow to significant sizes (even 100 entries would cause 10,000 individual iterations on the collection to be performed to add a new command, 1000 entries would require 1,000,000 iterations!).

If this concerns you, you might consider a key-value lookup scheme to allow for quick \$O(1)\$ lookup of results.

For example a structure like the following would enable fast lookup against the collection.

[
    '{filename}' => [
        {version} => {Command Object},
        {anotherversion} => {Command Object},
        ...
    ],
    '{anotherFilename}' => [ ... ],
    ...
]

With a viewCommand() method like:

public function viewCommand($name, $version)
{
    if(isset($commandFiles[$name][$version])) {
        return $commandFiles[$name][$version]->render();
    }
    return false;
}

Why should the collection hold a method for rendering the command? You already have the render() command living properly on the object, so isn't this basically a find() method?

For example:

$command = $collection->find($name, $version);
$command->render();

The collection should just be a collection. With methods only for managing/accessing the collection not for calling methods on the objects contained in the collection.

Note that generalizing to just a find() method would also allow you to call this method in the addCommand() method to see if command already exists.

Note that if you decided you still want to stick with Laravel Collection as your base data structure, then you might also get to the realization that your collection class really adds little value above and beyond what the Laravel class adds.  So why not:

$collection = collect([]);
$collection->push(new Command(...));

//  and to render
$command = $collection->where('name', $name);
$command->render();

Otherwise you might find yourself eventually adding filter, sort, etc. methods to your class because you have hidden away the underlying Collection capabilities.

Is it conceivable that a command in the collection could be replaced with a new Command object?  If so, then you should not be throwing exception in this case.  I would not think a collection would necessarily need to hold the logic as to whether an item in the collection could be replaced or not, as a caller could easily check for existence of an item in the collection and make decision to overwrite or not.

public function __get($property)
{
    if (property_exists($this, $property)) {
        return $this->$property;
    }
}

What if property does not exist? Should you throw exception? This condition would likely mean caller is accessing this class in unexpected fashion and should probably fail loudly.

$commandFile->name == $name

Consider using exact comparisons as default behavior, only using loose comparisons where there truly is a use case for it.


Strip all comments and create a new file with only (JS) code This code hasn't been tested exhaustively - I would love suggestions about a testing library.
Critical, Optional and Positive feedback are more then welcome.
const fs = require('fs');

/**
 * strip all comments and create a new file with only code
 * @param {int} file name to clean..
 * @return {int} function returns 0 at the end.
 */
function clean(file) {
  const inputFileName = file;

  // create output file name. e.g: test.js -> test_CLEAN.js
  let outputFileName = inputFileName.split('.');
  outputFileName[0] += '_CLEAN';
  outputFileName = outputFileName.join('.');

  // clean output file old data
  fs.writeFile(outputFileName, ``, (err) => {
    if (err) {
      console.log(err);
    }
  });

  // comments regex defentions
  // lines starts with // where // can have spaces before for indentation
  const singleLineComment = /^\s*\/\//;
  // start skipping lines on /* can have spaces before
  const multiLineCommentStart = /^\s*\/\*/;
  // stop skipping lines on */ end the end of the line
  const multiLineCommentEnd = /\*\/$/;
  // mixed lines with code and inline comments
  const singleLineCommentAfterCode = /[;]\s*\/\//;

  // skip lines bool
  let skipLine = false;

  // load input file to memory, split it by lines, filter white space lines, then loop over each line
  fs.readFileSync(inputFileName, 'utf-8')
      .split(/\n/)
      .filter((line) => line != '')
      .forEach((line) => {
        // return in a forEach() callback is equivalent to continue in a conventional for loop.

        if (multiLineCommentStart.test(line)) {
          skipLine = true;
          return;
        }

        if (multiLineCommentEnd.test(line)) {
          skipLine = false;
          return;
        }

        if (singleLineComment.test(line)) {
          return;
        }

        if (singleLineCommentAfterCode.test(line)) {
          line = line.split(' //')[0];
        }

        // append lines to output file
        if (!skipLine) {
          // append additional \n at the end of a blocks (after '};')
          line = line === '};' ? line + '\n' : line;
          fs.appendFileSync(outputFileName, `${line}\n`, (err) => {
            if (err) {
              // append failed
              console.log(err);
            }
          });
        }
      });

  return 0;
}

To handle codes, use a parser. Build an AST from parser (maybe comments are already removed there). And then convert it to codes. A FSM implementation may work. (I'm not sure). But I believe your current RegExp approach are far from working.
Anyway,
1. Line break
JavaScript support 5 different line terminator sequence. Your code only support one or two of them.
2. Single line comment
Line end no need to have ;.
var a = 42// comment

3. Multi line comment
Start / end of multi line comment no need to take a single line. They may appear on the same line.
var a = 42; /* comment */

var a = 42; /*
*/ var b = 42;

3. Some more complex comments
After you fix above issues, you are ready to handle more complex situations:
var a = 3
/*
*// 3;
/*/ /*/
/* // */
// /*
var b = 4;
// */

4. String and RegExp
Text in strings / regular expressions are not comments.
var a = '; // ';

var a = '\
// ';

var a = `
// `;

var a = /[; //]/;

This would be more complex if mix them up.
var c = `; // ${function () {
    a =  '}';
    // Wow, SO failed to use correct syntax highlight for this too!
}}`

5. HTML like comments
This could be optional. Most browsers supports HTML like comments. But this is not required to every JavaScript engine.
var a = 42;
--> this is comment
<!-- this is comment too

6. Shebang
It is up to you if you want to handle shebang as comments. They work on some browsers.
#!/usr/bin/node

var a = 42;


Given two int values, return the one closer to 10 I did the exercise below to practice. Can someone help me to shorten or improve the code in a simple way for a beginner?

The exercise is:

  Given 2 int values, return whichever value is nearest to the value 10,
  or return 0 in the event of a tie.

public int Closer(int a, int b)
{
     Console.WriteLine(Closer(8, 13));    // output: 8
     Console.WriteLine(Closer(13, 8));    // output: 8
     Console.WriteLine(Closer(13, 7));    // output: 0
}

My solution:

namespace Closer
    {
        class Program
        {
            static void Main(string[] args)
            {
                Console.WriteLine(Closer(13,15));    // output : 13
                Console.WriteLine(Closer(13, 13));   // output : 0
                Console.WriteLine(Closer(3,5));      // output : 5
                Console.WriteLine(Closer(13, 7));    // output : 0
                Console.WriteLine(Closer(-3, -5));   // output : -3
                Console.WriteLine(Closer(-3, -1));   // output : -1
                Console.ReadLine();
            }
            public static int Closer(int a, int b)
            {
                int calcA = a - 10;
                int calcB = b - 10;
                if (Math.Abs(calcA) == Math.Abs(calcB))
                {
                    return 0;
                }
                else if ((a >= 10 || b >= 10) && a < b )
                {
                    return a;
                }
                else if ((a >= 10 || b >= 10) && b < a)
                {
                    return b;
                }
                else if (calcB > calcA && Math.Abs(a)!= Math.Abs(b))
                {
                    return b;
                }
                else if ((calcA < 0 || calcB < 0) && (calcA > calcB && Math.Abs(a) != Math.Abs(b)))
                {
                    return a;
                }
                else return Closer(a,b);
            }
        }
    }

You have a lot of redundant code in here:

public static int Closer(int a, int b)
{
    int calcA = a - 10;
    int calcB = b - 10;
    if (Math.Abs(calcA) == Math.Abs(calcB))
    {
        return 0;
    }
    else if ((a >= 10 || b >= 10) && a < b )
    {
        return a;
    }
    else if ((a >= 10 || b >= 10) && b < a)
    {
        return b;
    }
    else if (calcB > calcA && Math.Abs(a)!= Math.Abs(b))
    {
        return b;
    }
    else if ((calcA < 0 || calcB < 0) && (calcA > calcB && Math.Abs(a) != Math.Abs(b)))
    {
        return a;
    }
    else return Closer(a,b);
}

First of all, you can figure everything out if you know the distance between the values of a, b, and 10.

int calcA = Math.Abs(a - 10);
int calcB = Math.Abs(b - 10);

You have principle in this section right, but it can be simplified a bit if you set calcA and calcB to use the absolute difference:

if (calcA == calcB)
{
    return 0;
}

Now, you just need to apply the same principle to the other values:

if (calcA < calcB)
{
    return a;
}

return b;    // we already established that the values are not the same, and that "a" is farther from 10 than "b"

Here is our new method:

public static int Closer(int a, int b)
{
    int calcA = Math.Abs(a - 10);
    int calcB = Math.Abs(b - 10);

    if (calcA == calcB)
    {
        return 0;
    }

    if (calcA < calcB)
    {
        return a;
    }

    return b;
}

But, 10 is a magic number in there, and magic numbers are never good.  Why don't you create a const value to store the number that you are checking the passed values against?

public static int Closer(int a, int b)
{
    const int compareValue = 10;
    int calcA = Math.Abs(a - compareValue);
    int calcB = Math.Abs(b - compareValue);

    if (calcA == calcB)
    {
        return 0;
    }

    if (calcA < calcB)
    {
        return a;
    }

    return b;
}

Now, you can change your method as much as you want, and you only have the magic number 10 in one place, so it is easy to change your method to compare the values against a different number.

Edit:

Thanks to Eric Lippert, I have fixed two bugs.  Eric found the bug that int.MinValue != int.MaxValue, so calling Math.Abs(int.MinValue) crashes.  If you pass a value that equals int.MinValue + compareValue, the program will crash.  While fixing this, I found another bug in which the wrong value will be returned if you call the function with two values that are less than int.MinValue + compareValue because they wrap around in the call value - compareValue.  Both of these bugs are now fixed for the compare value of 10, still working on a generic patch:

public static int Closer(int a, int b)
{
    const int compareValue = 10;

    while (a <= int.MinValue + compareValue || b <= int.MinValue + compareValue)
    {
        if (a <= int.MinValue + compareValue)
        {
            b = AdjustVal(b, compareValue);
            a++;
        }
        if (b <= int.MinValue + compareValue)
        {
            a = AdjustVal(a, compareValue);
            b++;
        }
    }

    int calcA = Math.Abs(a - compareValue);
    int calcB = Math.Abs(b - compareValue);

    if (calcA == calcB)
    {
        return 0;
    }

    if (calcA < calcB)
    {
        return a;
    }

    return b;
}

Edit 2:

Eric Lippert says we should do the arithmetic in longs. Of course, this doesn't work when we take input as long too.

public static int Closer(int a, int b)
{
    const int compareValue = 10;

    long calcA = Math.Abs((long)a - compareValue);
    long calcB = Math.Abs((long)b - compareValue);

    if (calcA == calcB)
    {
        return 0;
    }

    if (calcA < calcB)
    {
        return a;
    }

    return b;
}


Fastest way to find dataframe indexes of column elements that exist as lists I asked this question here: https://stackoverflow.com/q/55640147/5202255 and was told to post on this forum. I would like to know whether my solution can be improved or if there is another approach to the problem. Any help is really appreciated!

I have a pandas dataframe in which the column values exist as lists. Each list has several elements and one element can exist in several rows. An example dataframe is:

X = pd.DataFrame([(1,['a','b','c']),(2,['a','b']),(3,['c','d'])],columns=['A','B'])

X =
 A          B
0  1  [a, b, c]
1  2  [a, b]
2  3     [c, d]

I want to find all the rows, i.e. dataframe indexes, corresponding to elements in the lists, and create a dictionary out of it. Disregard column A here, as column B is the one of interest! So element 'a' occurs in index 0,1, which gives {'a':[0,1]}. The solution for this example dataframe is:

Y = {'a':[0,1],'b':[0,1],'c':[0,2],'d':[2]}

I have written a code that works fine, and I can get a result. My problem is more to do with the speed of computation. My actual dataframe has about 350,000 rows and the lists in the column 'B' can contain up to 1,000 elements. But at present the code is running for several hours! I was wondering whether my solution is very inefficient.
Any help with a faster more efficient way will be really appreciated!
Here is my solution code:

import itertools
import pandas as pd
X = pd.DataFrame([(1,['a','b','c']),(2,['a','b']),(3,['c','d'])],columns=['A','B'])
B_dict = []
for idx,val in X.iterrows():
    B = val['B']
    B_dict.append(dict(zip(B,[[idx]]*len(B))))
    B_dict = [{k: list(itertools.chain.from_iterable(list(filter(None.__ne__, [d.get(k) for d in B_dict])))) for k in set().union(*B_dict)}]

print ('Result:',B_dict[0])

Output

Result: {'d': [2], 'c': [0, 2], 'b': [0, 1], 'a': [0, 1]}

The code for the final line in the for loop was borrowed from here https://stackoverflow.com/questions/45649141/combine-values-of-same-keys-in-a-list-of-dicts, and https://stackoverflow.com/questions/16096754/remove-none-value-from-a-list-without-removing-the-0-value

itertuples
When traversing over the rows of a DataFrame, using itertuples is generally faster than iterrows. The latter makes a new Series for each row, the former a namedtuple, which is generally faster.
defaultdict
Appending items to a list on a key is typically done with a defaultdict.
from collections import defaultdict
result = collections.defaultdict(list)
for row in X.itertuples():
    idx = row.Index
    for item in row.B:
        result[item].append(idx)

The method you use to combine the values in the dictionary is meant to include empty items as None. Here you add them, and then filter them out, so using a defaultdict will simplify this up a lot.
naming
Try to follow pep-8.

snake_case for variables, etc.
spaces around operators
...

timings
dummy data
def make_dummydata(rows, max_length, seed=0):
    letters = string.ascii_letters
    np.random.seed(seed)
    random.seed(seed)
    col1 = np.random.randint(0, 10, size=rows)
    items_per_row = np.random.randint(0, max_length, size=rows) + 1
    col2 = [random.choices(letters, k=amount) for amount in items_per_row]
    return pd.DataFrame({"A": col1, "B": col2})

benchmark method
import timeit

def benchmark(cases, functions):
    for rows, max_length in cases:
        df = make_dummydata(rows, max_length)
        for name, function in functions.items():
            result = timeit.timeit(
                stmt=f"function(df)",
                globals={"df": df, "function": function},
                number=1,
            )
            yield rows, max_length, name, result

results
cases = [(10, 2), (100, 10), (1000, 40), (10000, 200)]
functions = {
    "OP": find_op,
    "maarten": find_maarten,
    "jezrael": find_jezrael,
}
list(benchmark())

[(10, 2, 'OP', 0.001344002000003286),
 (10, 2, 'maarten', 0.0003913850000003549),
 (10, 2, 'jezrael', 0.005293956000002709),
 (100, 10, 'OP', 0.027166392000005146),
 (100, 10, 'maarten', 0.0004795910000012782),
 (100, 10, 'jezrael', 0.013824836999994261),
 (1000, 40, 'OP', 0.3434149869999956),
 (1000, 40, 'maarten', 0.0032574399999987236),
 (1000, 40, 'jezrael', 0.018533767000000978),
 (10_000, 200, 'OP', 33.48681208600001),
 (10_000, 200, 'maarten', 0.10972772499999905),
 (10_000, 200, 'jezrael', 0.7631061700000004),
 (350_000, 1000, 'maarten', 22.097186581000003),
 (350_000, 1000, 'jezrael', 516.128048978)]

The method using defaultdict is a lot faster. This only says something about the run-time, not the memory usage, where my method does not create a large intermediary DataFrame.


Applying multiple php FilterIterator instances in a cascading fashion I wrote an implementation of php FilterIterator objects that allows me to apply them in a "cascade" way, so I can finally obtain an array of objects which is the result of all those filters.

It works fine, but - being new to SPL objects - I'm wondering if there is a better way to achieve the same results with FilterIterators et similia.

My implementation

I have a Factory class holding an array of objects in a static property:

class Factory {

    /**
     * @var Reservation[]
     */
    public static $reservations;

    public function __construct(){
        self::$reservations = $this->get_all();
    }

    private function get_all(){
        // Queries the database...
        return $results; // array of Reservation objects
    }

    // other unrelated things...
}

As I need the ability to filter such array by some criteria on-demand, I defined a bunch of FilterIterator sub-classes. The following is byOrder, others (byService, byProvider etc.) are defined in a similar way:

class byOrder extends FilterIterator {

    protected $order;

    public function __construct(Iterator $iterator, $order){
        parent::__construct($iterator);
        $this->order = $order;
    }

    public function accept(){

        /** @var $current Reservation */
        $current = $this->current();

        return $current->order() === $this->order;
    }
}

In order to apply just one filter to the array (I need it to stay as array) wherever in my application I can do:

// somewhere else...
$factory = new Factory();
// later on...
$reservations = new ArrayObject(Factory::$reservations);
$iterator = $reservations->getIterator();
$filteredReservation = iterator_to_array(new byOrder($iterator, 'ORDER_ID'));

To be able to apply multiple filters in a "cascade" way, I created the following helper class:

class Select {

    /**
     * @var ArrayIterator
     */
    private $reservations;

    public function __construct(array $reservations)
    {
        $obj = new ArrayObject($reservations);
        $this->reservations = $obj->getIterator();
    }

    public function get(){
        return iterator_to_array($this->reservations);
    }

    public function byOrder($order){
        $this->reservations = new byOrder($this->reservations, $order);

        return $this;
    }

    public function byService($service){
        $this->reservations = new byService($this->reservations, $service);

        return $this;
    }

    // ... and so on
}

It is instantiated on-demand through a method inside the Factory class:

class Factory {

    // All the previously defined content, plus:
    public function select() {
        return new Select(self::$reservations);
    }
}

So I'm able to apply multiple cascading filters like this:

// somewhere else...
$factory = new Factory();
// later on...
$filtered = $factory->select()
    ->byOrder('ORDER_ID')
    ->byService('SERVICE_NAME')
    ->get();

Edit

One suggestion is to make your helper class more dynamic. As it currently stands it is heavily tied to each filter class so you could not reuse it in another project. For example, imagine a new project that deals with selling cars. Your Select helper class only knows about orders, services, providers. You'd have to rewrite it to know about cars, prices, colours etc.

To get around this, you can define a filter property which holds a list of filters. Then, when calling get(), loop over the filters and apply each one. The benefit here is that the helper class is now
unaware of any filter implementations and can be used in any project.

<?php

/*************************************************/
/******************** FILTERS ********************/
/*************************************************/
class Order extends FilterIterator
{
    private $_value = null;

    public function __construct(Iterator $iterator, $value)
    {
        $this->_value = $value;
        parent::__construct($iterator);
    }

    public function accept()
    {
        return $this->current()->order === $this->_value;
    }
}

class Future extends FilterIterator
{
    public function __construct(Iterator $iterator)
    {
        parent::__construct($iterator);
    }

    public function accept()
    {
        return $this->current()->date > new DateTime();
    }
}

class Date extends FilterIterator
{
    private $_value = null;

    public function __construct(Iterator $iterator, $value)
    {
        $this->_value = $value;
        parent::__construct($iterator);
    }

    public function accept()
    {
        return $this->current()->date === $this->_value;
    }
}

/*************************************************/
/*************** COLLECTION HELPER ***************/
/*************************************************/
class Helper
{
    private $_items = [];
    private $_filters = [];

    public function __construct(array $items)
    {
        $this->_items = (new ArrayObject($items))->getIterator();
    }

    public function filter($filter, $value = null)
    {
        $this->_filters[] = [$filter => $value];

        return $this;
    }

    public function get()
    {
        foreach ($this->_filters as $filter) {
            foreach ($filter as $name => $value) {

                if ($value) {
                    $this->_items = new $name($this->_items, $value);
                } else {
                    $this->_items = new $name($this->_items);
                }
            }
        }

        return iterator_to_array($this->_items);
    }
}

/*************************************************/
/****************** TEST CASES *******************/
/*************************************************/
class Reservation
{
    public $order = null;
    public $date = null;
    public $status = null;

    public function __construct($order, $date, $status)
    {
        $this->order = $order;
        $this->date = $date;
        $this->status = $status;
    }
}

$reservations = new Helper([
    new Reservation(123, new DateTime('+1Day'), 'Active'),
    new Reservation(512, new DateTime('+2Month'), 'Active'),
    new Reservation(456, new DateTime('-6Hour'), 'Pending'),
    new Reservation(789, new DateTime('-17Day'), 'Cancelled'),
    new Reservation(264, new DateTime('+1Hour'), 'Pending'),
    new Reservation(151, new DateTime('+1Year'), 'Active'),
]);

$filtered = $reservations
    ->filter('Future')
    ->filter('Order', 151)
    ->get();

var_dump($filtered);

Final note - I played around with the implementation for a while and found FilterIterators to be rather cumbersome because I had to write a single filter class for each permutation of a property. For example, in a reservation system you'd need the following filters: ByOrder, ByOrderNotEqualTo, ByDate, ByFuture, ByPast, By6MonthsFromNow, ByStatus, ByStatusNotEqualTo, ByCustomerId, etc.

One solution is to add a where method to your helper class which would accept a comparison operator. Again, I point you to Laravel's implementation and a document highlighting its usage. Food for thought.

EndEdit

If you're looking for a simple method, take a look at Laravel's implementation of their Collection::filter which calls Arr::filter. It provides a clean, chainable way to pass a callback to array_filter.

Below, is a basic demonstration on how to use it. I've implemented it using ArrayObject but you can use any iterable class. The filter calls will be slightly longer to type out than your implementation since you must define a callback function each time, but it makes the source much cleaner since you don't have to define a separate filter class for every property.

http://sandbox.onlinephpfunctions.com/code/cea564648d52c702e112d9775d1afe76f41fc810

<?php

class Collection extends ArrayObject
{
    public function filter(callable $callback = null)
    {
        if ($callback) {
            // This will return a new Collection object with the filtered results.
            // Late static binding -  You **could use either `static`
            // or `self` here since both will resolve to Collection.
            // Going with `static` in case you want to extend this class.
            // See - https://stackoverflow.com/q/5197300/296555
            return new static (array_filter($this->getArrayCopy(), $callback, ARRAY_FILTER_USE_BOTH));
        }

        return new static (array_filter($this->getArrayCopy()));
    }
}

class Reservation
{
    public $order = null;
    public $date = null;
    public $status = null;

    public function __construct($order, $date, $status)
    {
        $this->order = $order;
        $this->date = $date;
        $this->status = $status;
    }
}

$reservations = new Collection([
    new Reservation(123, new DateTime('+1Day'), 'Active'),
    new Reservation(512, new DateTime('+2Month'), 'Active'),
    new Reservation(456, new DateTime('-6Hour'), 'Pending'),
    new Reservation(789, new DateTime('-17Day'), 'Cancelled'),
    new Reservation(264, new DateTime('+1Hour'), 'Pending'),
    new Reservation(151, new DateTime('+1Year'), 'Active'),
]);

$filtered = $reservations
    // Active reservations
    ->filter(function ($value, $key) {
        return $value->status === 'Active';
    })
    // In the future
    ->filter(function ($value, $key) {
        return $value->date > new DateTime();
    });

var_dump($filtered);

Finally, here's a decent package that took this idea a step further and implements most array functions so you can chain them.