Checking for balanced brackets in Python I'm solving HackerRank "Stacks: Balanced Brackets" in Python.

  A bracket is considered to be any one of the following characters: (,
  ), {, }, [, or ].

  Two brackets are considered to be a matched pair if the an opening
  bracket (i.e., (, [, or {) occurs to the left of a closing bracket
  (i.e., ), ], or }) of the exact same type. There are three types of
  matched pairs of brackets: [], {}, and ().

  A matching pair of brackets is not balanced if the set of brackets it
  encloses are not matched. For example, {[(])} is not balanced because
  the contents in between { and } are not balanced. The pair of square
  brackets encloses a single, unbalanced opening bracket, (, and the
  pair of parentheses encloses a single, unbalanced closing square
  bracket, ].

  By this logic, we say a sequence of brackets is considered to be
  balanced if the following conditions are met:

  It contains no unmatched brackets. The subset of brackets enclosed
  within the confines of a matched pair of brackets is also a matched
  pair of brackets. Given  strings of brackets, determine whether each
  sequence of brackets is balanced. If a string is balanced, print YES
  on a new line; otherwise, print NO on a new line.

My code:

def is_matched(expression):
    if len(expression) % 2 != 0:
        return False

    opening = ("(", "[", "{")
    closing = (")", "]", "}")
    mapping = {opening[0]:closing[0],
               opening[1]:closing[1],
               opening[2]:closing[2]}

    if expression[0] in closing:
        return False

    if expression[-1] in opening:
        return False

    closing_queue = []
    for letter in expression:
        if letter in opening:
            closing_queue.append(mapping[letter])
        elif letter in closing:
            if not closing_queue:
                return False

            if closing_queue[-1] == letter:
                closing_queue.pop()
            else:
                return False

    return True

t = int(input().strip())
for a0 in range(t):
    expression = input().strip()
    if is_matched(expression) == True:
        print("YES")
    else:
        print("NO")

What could be improved here? What can be done to make my code more idiomatic (pythonic code)?

Is my implementation bad? Why is it?

Review

Your implementation is quite good, but ofcourse there are always some things that can be improved upon.

Use a if __name__ == '__main__' guard, see what they do.
opening closing and mapping could be done a bit simpler

You may use list over the values, to create a list,
And you may zip those in a dictionary with the zip keyword.

If you need to add alot of corner cases, your algorithm is likely off somewhere else.
Consider adding Testcases, Docstrings, or both in the form of Doctest. To make it easier to Test your algorithm.

Alternative code

def is_matched(expression):
    """
    Finds out how balanced an expression is.
    With a string containing only brackets.

    >>> is_matched('[]()()(((([])))')
    False
    >>> is_matched('[](){{{[]}}}')
    True
    """
    opening = tuple('({[')
    closing = tuple(')}]')
    mapping = dict(zip(opening, closing))
    queue = []

    for letter in expression:
        if letter in opening:
            queue.append(mapping[letter])
        elif letter in closing:
            if not queue or letter != queue.pop():
                return False
    return not queue

if __name__ == '__main__':
    import doctest
    doctest.testmod()


Database connection in constructor and destructor I am playing with different ways to do database interaction in PHP, and one of the ideas I have been playing with is connecting to the DB in the constructor and disconnecting in the destructor. This is the code from my Database class.

function __construct()
{
  $this->link = mysql_connect($this->server.':'.$this->port, $this->username);
  if(!$this->link)
    die('Could not connect: '.mysql_error());

  if(!mysql_select_db($this->database, $this->link))
    die('Could not select database: '.mysql_error());
}

function __destruct()
{
  if(mysql_close($this->link))
    $this->link = null;
}

This works well, my only reservation is that if I need to connect several to hit the database several times it will do multiple connections and disconnects. If I do that a lot I can see, maybe, potential problems. Is this a concern or is there a better way to do this? And is my code even up to snuff in general?

You could use MySQLi (PHP extension) which is class based by default instead of MySQL. It
is very easy to set up multiple connections. You are, however, required to know the connection you are querying always.

Congrats with the first question.


Are these MySQL Abstraction Classes safe from SQL Injection?  Can they be made safe? I was looking at this website:

http://www.devshed.com/c/a/MySQL/Creating-User-Models-in-PHP-and-MySQL/1/
and
http://www.devshed.com/c/a/MySQL/Creating-User-Models-in-PHP-and-MySQL/2/

In the first one they have methods such as:

/**
     * Perform a SELECT statement
     */
    public function select($table, $where = '', $fields = '*', $order = '', $limit = null, $offset = null)
    {
        $query = 'SELECT ' . $fields . ' FROM ' . $table
               . (($where) ? ' WHERE ' . $where : '')
               . (($limit) ? ' LIMIT ' . $limit : '')
               . (($offset && $limit) ? ' OFFSET ' . $offset : '')
               . (($order) ? ' ORDER BY ' . $order : '');
        $this->query($query);
        return $this->countRows();
    }

    /**
     * Perform an INSERT statement
     */
    public function insert($table, array $data)
    {
        $fields = implode(',', array_keys($data));
        $values = implode(',', array_map(array($this, 'quoteValue'), array_values($data)));
        $query = 'INSERT INTO ' . $table . '(' . $fields . ')' . ' VALUES (' . $values . ')';
        $this->query($query);
        return $this->getInsertId();
    }

then in the second page:

/**
     * Fetch all users
     */
    public function fetchAll()
    {
        // try to fetch users data from the cache system
        if ($users = $this->_cache->get('all')) {
            return $users;
        }
        // otherwise fecth users data from the database
        $this->_adapter->select('users');
        $users = array();
        while ($user = $this->_adapter->fetch()) {
            $users[] = $user;
        }
        if (count($users) !== 0) {
           $this->_cache->set('all', $users);
        }
        return $users;
    }

    /**
     * Insert new user data (the cache is cleared also)
     */
    public function insert(array $data)
    {
        if ($insertId = $this->_adapter->insert('users', $data)) {
            $this->_cache->clear();
            return $insertId;
        }
        return false;
    }

    /**
     * Delete the table row corresponding to the specified user (the cache is cleared also)
     */
    public function delete($id)
    {
        if ($affectedRow = $this->_adapter->delete('users', "id = $id")) {
            $this->_cache->clear();
            return $affectedRow;
        }
        return false;
    }

Is this a bad way of doing things?  Is this opening me up to sql injection just like anything else where I construct the sql on the fly would?  I want to build my models "right."  Does that mean I need to have all stored procedures or prepared statements?

Active record instances are not "models". They are anti-patterns which RoR likes call that way. And what you are implementing there is an active record.

And no, those queries are not safe. They are wide open for SQL injection, because, instead of using proper SQL prepared statements, you opted for concatenation.

As for build building model, you have to understand that model is a layer. One of two in MVC. The other being - presentation layer (made of controllers, views and templates). You might read this answer, but I am no sure how much of it you will be able to assimilate.

I think your first priority should be to read the hashphp PDO tutorial and then watching the list if lecture that you can find a the bottom of this post. And only then try another stab at the whole "mvc thing".


Querying list of servers with PS to report on Scheduled Tasks I work in a Windows environment where Scheduled Tasks have never been documented or kept up with, leaving us with about 150 servers that have a random smattering of tasks. I am attempting to construct something that will query specific information about these tasks and output them in a pretty format that I can sort through.

Here is what I have currently cobbled together:

Import-Module ActiveDirectory
$VerbosePreference = "continue"
$list = (Get-ADComputer -LDAPFilter "(&(objectcategory=computer)(OperatingSystem=*server*))").Name
$logfilepath = "$home\Desktop\TasksLog.txt"
$ErrorActionPreference = "SilentlyContinue"

invoke-command -ComputerName $list -ScriptBlock { $sched = New-Object -Com "Schedule.Service"
$sched.Connect()
$out = @()
$sched.GetFolder("\").GetTasks(0) | % {
    $xml = [xml]$_.xml
    $out += New-Object psobject -Property @{
        "Name" = $_.Name
        "Status" = switch($_.State) {0 {"Unknown"} 1 {"Disabled"} 2 {"Queued"} 3 {"Ready"} 4 {"Running"}}
        "NextRunTime" = $_.NextRunTime
        "LastRunTime" = $_.LastRunTime
        "Author" = $xml.Task.Principals.Principal.UserId
    }
}

$out | fl Name,Status,NextRunTime,LastRunTime,Author, } | Out-File $logfilepath

The "pretty" part is a work in progress. However, if I am correct, this should query a list of server names from AD and then invoke the command on all of them, then output the results to the designated file. I have tested with one server...

$list = "Server1"

...but am hesitant to attempt to invoke these all at once. I am not even sure if this is the correct/most effective way of passing a list of servers to the invoke-command cmdlet.

Fan-out remoting

Calling Invoke-Command with an array for -ComputerName is perfectly fine. Note that it's going to call them one by one. If it can't reach some, there will be errors, but it's fine. It's sort of like doing:

$list | ForEach-Object {
    Invoke-Command -ComputerName $_ -ScriptBlock { ... }
}

The above can be useful for helping you with progress, like if you wanted to do something like:

$list | ForEach-Object {
    Write-Verbose $_ -Verbose
    Invoke-Command -ComputerName $_ -ScriptBlock { ... }
}

Or you could turn it into a for loop instead so you know the index, then you can calculate percentage and use Write-Progress to get some really nice progress info.

As for how effective it is, well, very, as long as you don't have a lot of servers and/or don't care how long it takes, since it's not going to run in parallel on all the machines.

Other options for parallel remoting

Workflows

You could use a PowerShell workflow, which has capability to process things remotely and in parallel, but to be honest it's a little weird at first, and probably not necessary.

Jobs

Probably the easiest way to implement parallel jobs is to use PSJobs. You would basically add the -AsJob parameter to Invoke-Command, but then its output is the job object. You have to retool things a bit to then wait on and receive the jobs and the data they return.

Jobs are a bit heavy though; they're technically entirely separate PowerShell processes, so if you have 2,000 servers, it might not be a good idea to suddenly spawn 2,000 powershell processes on your workstation to host the jobs, so be weary of that. Jobs would mostly be useful if you have ~100 or fewer machines, a good workstation, and maybe a lot of the servers timeout (so they take longer than the remote process needs to even run, and choke the whole thing).

Runspaces

PowerShell MVP Boe Prox has a module called PoshRSJob that use runspaces as a more efficient way of multithreading in PowerShell, and it supports throttling too.

Just don't

Since you're just trying to generate a report that you'll look at later, I'd say keep it simple and just don't do it in parallel.


Performing substitutions according to a word list in a database A bit of history

I had about 40,000 words in the database. I need words to correct the texts entered incorrectly by users. User enter text in any non-legal format and I use my words in the database to correct them. Everything was great before when when I had less than 50,000 words on my database. Now I have more than 100,000 words on the database.

What method do I use to correct words?

To start, I add all the words to a single database by query to the database.
Example array with correct words:

$correctWords = array{
   "тоҷик",
   "ӯзбек",
   "қазоқ",
   "қирғиз"
   .......,
   "ғайратҷон"
}

With this array, I create another array by replacing some letters(ҷ,ӯ,қ,ғ to ч,у,к,г) with ordinary letters. In a result will be created an associative array from first array with correct words:

$wordBase = array{
   "точик"=>"тоҷик",
   "узбек"=>"ӯзбек",
   "казок"=>"қазоқ",
   "киргиз"=>"қирғиз"
   ................,
   "гайратчон"=>"ғайратҷон"
}

As you can see in the key of the array the wrong variants of words were transferred and the correct version of words was passed to the value of the array. From the text entered by users, I will search for words using words from the array key and replace them with a value to correct the text.

Usage wordbase array to correct incorrect typed words in the text:

$text = "Точик УЗБЕК казок КирГИЗ Гайратчон";

$dictionary = array{
   "точик"=>"тоҷик",
   "узбек"=>"ӯзбек",
   "казок"=>"қазоқ",
   "киргиз"=>"қирғиз"
   ................,
   "гайратчон"=>"ғайратҷон"
}

$output = preg_replace_callback("/\pL+/u", function ($m) use ($dictionary) {
$word = mb_strtolower($m[0]);
if (isset($dictionary[$word])) {
    $repl = $dictionary[$word];
    // Check for some common ways of upper/lower case
    // 1. all lower case
    if ($word === $m[0]) return $repl;
    // 2. all upper case
    if (mb_strtoupper($word) === $m[0]) return mb_strtoupper($repl);
    // 3. Only first letters are upper case
    if (mb_convert_case($word,  MB_CASE_TITLE) === $m[0]) return mb_convert_case($repl,  MB_CASE_TITLE);
    // Otherwise: check each character whether it should be upper or lower case
    for ($i = 0, $len = mb_strlen($word); $i < $len; ++$i) {
        $mixed[] = mb_substr($word, $i, 1) === mb_substr($m[0], $i, 1)
            ? mb_substr($repl, $i, 1)
            : mb_strtoupper(mb_substr($repl, $i, 1));
    }
    return implode("", $mixed);
}
return $m[0]; // Nothing changes
}, $text);

echo $output;

Result

Тоҷик ӮЗБЕК қазоқ ҚирҒИЗ Ғайратҷон

When I have 100,000 words in the database, the function is to fix this text within 13-17 seconds. It takes a lot of time. What can I do to optimize the process of correcting the text with a large number of words? How not to use the whole word from the database to fix a few words? Now my code for fixing 2 words also uses the weight of an array of words and for 1000 words too I use the same. Can I solve the problem with queries on the database itself? If you need a large number of words for testing then please contact me. I will send them to you by reference.

This whole approach seems flawed to me. It's hard to predict how people will misspell words and you therefore end up with a very big list of possible misspellings.

Why not do it the traditional way: Start with a list of correct words ($correctWords), and use that to find the words not in that list. If you convert the text ($text), you want to correct, to an array of words you can use:

$textWords       = explode(' ',mb_strtolower($text));
$misspelledWords = array_diff($textWords,$correctWords);

I would remove punctuation before starting this process. When you have the possibly misspelled words, you can decide what to do with them.

You could, for instance, compare the misspelled words with the list of correct words, using one of these functions:

http://php.net/manual/en/function.levenshtein.php

http://php.net/manual/en/function.similar-text.php

The closest one is probably the correct one. This comparison might be slow, but there are probably only a few misspelled words, so it won't take long.

You could also still use your list of misspellings, if you want to.

After you've decided, what the misspelled words should be replace by, you can do:

$text = str_replace($misspelledWord,$replacementWord,$text);

Note that str_replace() will accept arrays, like this:

$text = str_replace($misspelledWords,$replacementWords,$text);

and after that you're done.

This only shows you the basic idea. It needs a lot of refinement to be of any practical use.


Code redundancy issue while feeding dropdownlists I have a dashboard to manage a learning center system. There are scenarios like admin create learning center and define a classroom in it and then assign some courses in it and assign students to those courses and assign a teacher for each courses.

In order to achieve this, I have to create lot of dropdownlists such as for creating a course you have to select first a learning center then an appropriate classroom.

Below is the code for the dropdownlists in assigning a student into a course:

    //Retrieve all learning centers
    studentAssignmentViewModel.LearningCenters = _learningCenterService.LearningCenters.ToList();
    studentAssignmentViewModel.StudentSelectListItem = studentAssignmentViewModel.LearningCenters
        .Where(y => y.Status != Status.Removed).OrderBy(x => x.Name)
        .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });

    //Retrieve All Students
    studentAssignmentViewModel.Students = _studentAssignmentService.Students.ToList();
    studentAssignmentViewModel.StudentSelectListItem = studentAssignmentViewModel.Students
        .Where(y => y.Status != Status.Removed).OrderBy(x => x.Name)
        .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });

    //If the user is not admin, it retrieves the courses belong to the teacher otherwise all courses
    var userId = _dashboardUserAuthenticationService.AuthenticatedDashboardUser.Id;
    var userRole = _dashboardUserService.Get(userId).UserRole;
    if (userRole != "1")
    {
        studentAssignmentViewModel.Courses = _studentAssignmentService.Courses.ToList();
        studentAssignmentViewModel.CourseSelectListItem = studentAssignmentViewModel.Courses
            .Where(t => (t.Status != Status.Removed) && t.TeacherId == userId).OrderBy(x => x.Name)
            .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });
    }
    else
    {
        studentAssignmentViewModel.Courses = _studentAssignmentService.Courses.ToList();
        studentAssignmentViewModel.CourseSelectListItem = studentAssignmentViewModel.Courses
            .Where(t => (t.Status != Status.Removed)).OrderBy(x => x.Name)
            .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });
    }

And I am repeating those similar code pieces in edit and create actions in mvc for all scenarios. So I don't know if there is enough information to make a statement but what would be your suggestions for such cases, what would be the best practice to get rid of those ugly code pieces? Since situations vary, turning them into classes would lead me into so many specific classes which cannot be used for any other situation.

It's a small thing, but you could move the bulk of the code for course selection outside of the if statement by initializing the list and then filtering it further only if the user is not an admin.

    studentAssignmentViewModel.Courses = _studentAssignmentService.Courses.ToList();
    studentAssignmentViewModel.CourseSelectListItem = studentAssignmentViewModel.Courses
        .Where(t => (t.Status != Status.Removed)).OrderBy(x => x.Name)
        .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });

    //If the user is not admin, it filter the course list further
    var userId = _dashboardUserAuthenticationService.AuthenticatedDashboardUser.Id;
    var userRole = _dashboardUserService.Get(userId).UserRole;
    if (userRole != "1")
    {
        studentAssignmentViewModel.CourseSelectListItem = studentAssignmentViewModel.CourseSelectListItem
            .Where(t => (t.TeacherId == userId).OrderBy(x => x.Name)
            .Select(x => new SelectListItem { Text = x.Name, Value = x.Id.ToString() });
    }

That may perform slightly worse because now it has to iterate twice if the user isn't an admin, but you're no longer repeating yourself. Any change to the logic would only have to occur in one place.

Now there's this...

  if (userRole != "1")

If you changed this to use an enum instead, that comment becomes obsolete and can be removed. Always try to let the code document itself. I would much rather see this.

if (userRole != UserRole.Admin)

Also, you say you're repeating this logic elsewhere in your code. You've not shown it, but I highly recommend extracting a new class that is responsible for this logic. You can then inject it into these other classes via their constructor and any changes to this logic truly only has to occur in one place.


querySingle function for PDO I have been refactoring a lot of my old code lately, and with that I have been switching from sqlite3 to mysql using PDO - sqlite3 has something called "querysingle" that allows you to return single responses (as a row or a single value) from a database table.

Anyways, I really liked the simplicity of this and that it all fits in 1 line of code and (IMO) doesn't harm the readability of the code.

I wrote a function in PHP that does a similar thing, but I wanted to make sure that I wasn't missing any edge cases, or see if I could improve the function in any way.

Here is my function:

function pdoQuerySingle($con, $query, $values = array(), $single = false) {
    if($values) {
        $stmt = $con->prepare($query);
        $stmt->execute($values);
    } else {
        $stmt = $con->query($query);
    }
    if($single) return $stmt->fetch()[$single];
    return $stmt->fetch();
}

You use it as follows:

$variable = pdoQuerySingle($pdo, "SELECT * FROM table WHERE id=?", [$id]);

As you can see this query works with prepared statements for security against SQL injection, though you can leave the 3rd param empty if you aren't passing any values to the query. The 4th parameter can be used to return only a specific entry from the query return as a string so if you wanted $variable to be set to a string from a column in the database, let's say, "name" you would do

$variable = pdoQuerySingle($pdo, "SELECT name FROM table WHERE id=?", [$id], "name");

There is a better way.

That's a very good function, but you can make it 100 times more useful by returning the statement.

function pdoQuery($con, $query, $values = array()) {
    if($values) {
        $stmt = $con->prepare($query);
        $stmt->execute($values);
    } else {
        $stmt = $con->query($query);
    }
    return $stmt;
}

Given there are two excellent things,

"method chaining" (if some statement returns an object, you can call this object's method directly by adding -> )
different fetch methods supported by PDOStatement

So in the end your current code will be changed a little, but you will get a lot of other neat ways for getting your data.

To get a row

$row = pdoQuery($pdo, "SELECT name FROM table WHERE id=?", [$id])->fetch();

to get a name

$name = pdoQuery($pdo, "SELECT name FROM table WHERE id=?", [$id])->fetchColumn();

But beside these two use cases there are many others!

you can run a DML query as well:

pdoQuery($pdo, "DELETE FROM table WHERE id=?", [$id]);

fetch a list of records

$data = pdoQuery($pdo, "SELECT * FROM table")->fetchAll();

use all the formats supported by fetchAll()
even get a row count

$deleted = pdoQuery($pdo, "DELETE FROM table WHERE id=?", [$id])->rowCount();

As you can see, this new function is much more flexible and can be used to ease a lot more tasks


Swapping the diagonals of a matrix I am required to write a simple matrix program and swap the diagonal.

Given the input

1 2 3
4 5 6
7 8 9

After swapping the diagonal it should be

3 2 1
4 5 6
9 8 7

Here is my code:

public static void main(String[] args) {
    int i,j = 0,m,k;
    int Number[][] = new int [3][3];
    Scanner scanner = new Scanner(System.in);
    System.out.println("Enter the elements of matrix");
    for(i=0;i<=2;i++)
    {
        for(j=0;j<=2;j++)
        {
            m = scanner.nextInt();
            Number[i][j] = m;
        }
    }
    System.out.println("This is the original matrix");
    for(i=0;i<=2;i++)
    {
        for(j=0;j<=2;j++)
        {
            System.out.print(Number[i][j]+"\t");
        }
        System.out.println();
    }
    //Swapping
    for(i=0;i<=3;i++)
    {
        k=Number[i][j];
        Number[i][j] = Number[i][3-i-1];
        Number[i][3-i-1] = k;
    }
    for(i=0;i<=2;i++)
    {
        for(j=0;j<=2;j++)
        {
            System.out.print(Number[i][j]+"\t");
        }
        System.out.println();
    }
}

I assume that your code produces the desired output, so I'll comment on the code style only here.

While doing everything in the main method may work for small programs, it's not a good idea in general. Ideally, one method should do one thing. Moreover, exactly the same code that prints the matrix is used twice in your solution. It's a very good reason to create a separate method for it. In general, I would split your program into at least three separate methods: one of them would read the matrix, the other one would perform a required transformation and the last one would print the matrix to the standard output. It looks roughly like this:

int[][] readMatrix() {
    ...
}

void swapDiagonal(int[][] matrix) {
    ...
}

void printMatrix(int[][] matrix) {
    ...
}

The comments inside the method (like //Swapping) are an indicator that a piece of code this comments corresponds to should be probably factored out to a separate method (of course, it's not always the case. It is the case here, though). Once or code calls the swapDiagonal method, there is no need for such comments anymore. The code becomes self-documented, which is a great thing as comments can become obsolete if you change the code in the future. There's no way to ensure that the piece of code followed by the //Swapping comment actually swaps anything after changes are made (again, it's not a big deal for a such a small program, but it can become a serious issue if you implement a larger program using the same practices).
You can get rid of magic constants (like 3 and 2) in your code by creating a constant variable for them (for instance, you can set final static int MATRIX_SIZE = 3 and use it everywhere in your code. It communicates the intent more clearly and makes possible future changes easier (you need to change just one constant instead of looking for all 3's and 2's around your code).
Keeping the scope of each variable as small as possible is a good practice. The declaration of loop counters outside the loop is not just unnecessary, it's also sort of misleading (as it's normally done only when the value of the counter is used outside the loop, which is not the case here). That is, the only variable you need to declare in the begging of the method is the array to hold the matrix. The loops are usually implemented this way:

for (int i = 0; i < MATRIX_SIZE; i++) {
    for (int j = 0; j < MATRIX_SIZE; j++) {
        // do something with the (i, j) element here
    }
}

Variable naming. It's conventional for the names of variables to start with a lowercase letter. Moreover, a Number doesn't look like a good name for a matrix to me. It's confusing. You could call matrix, for instance.
Code formatting. It's conventional to surround binary operators with whitespaces. The opening curling bracket shouldn't go to the next line according to the Java coding conventions. It is also common to use int[][] array, not C-style int array[][] in Java (it has the same semantics, though).
You could also get rid of redundant variables and assignments (I'm not talking about performance, I' talking about communicating your intent as clearly as possible):

m = scanner.nextInt();
Number[i][j] = m;

looks quite strange. Why not use:

Number[i][j] = scanner.nextInt();

instead?


Class which updates an object I have a class which updates an object. The class takes a String id in its constructor and returns the appropriate class based on the id. I think these two methods should be separated into their own classes as returning an object based on a String id will probably have uses elsewhere in the code base.

I've considered adding the functionality which returns the object to update into its own static method:

ObjectToUpdate = Utils.getObjectToUpdate(id)

Is there a better way or a design pattern?

UpdateContestantObj c = new UpdateContestantObj(childWithParentRatingsObject.getCoupleId());
c.getContestantObj().setSortVal(singleScoringFM.getScore());
c.persistContestant();

public class UpdateContestantObj {

    private String id;
    private ContestantObj contestantToSave = null;

    public ContestantObj getContestantObj(){
        return this.contestantToSave;
    }

    public UpdateContestantObj(String id){
        this.id = id;

        SimpleSortingVector simpleSortingVector = (SimpleSortingVector)FutureContent.future.getContent(Constants.CONTESTANTS_DATA);
        Enumeration contestantsEnumeration = simpleSortingVector.elements();

        while(contestantsEnumeration.hasMoreElements()){
            final ContestantButtonField contestantButtonField = (ContestantButtonField)contestantsEnumeration.nextElement();
            if(contestantButtonField.getContestant().getId().equalsIgnoreCase(this.id)){
                contestantToSave = contestantButtonField.getContestant();
            }
        }
    }

    public void persistContestant(){

        contestantsStore.setContents(contestantToSave);
        contestantsStore.commit();

    }
}

There's no super-design-pattern that would be beneficial here, but you could definitely benefit from some basic OO concepts like encapsulation and single responsibility.

For starters, rather than making a bunch of utility methods, it's better design to allow each class a static method like:

// inside the ContestantObj class
public static ContestantObj fromID(string ID) {
   // code to retrieve ContestantObj
}

This way, each class is responsible for creating itself, which it should be (outside of cases where a builder or abstract factory is required).

Your UpdateContestantObj constructor should take in a ContestantObj. It shouldn't care about how the ContestantObj is created. A "Contestant Updater" should be a machine that takes in Contestants and outputs Contestants that have been updated.

Also, the naming convention of ContestantObj should simply be Contestant. We know it's an object.


Manipulating 4 inputs I have the following assignment:

  A program that takes four (4) inputs then using a conditional
  statement does the following:

  Display the values that are below or equal to 25.
  Multiply the values that are below 20 with each other.
  Add the values that are above 25 with each other.

I've successfully solved all of them as they're pretty easy, however I want to ask a couple of things along with getting feedback from you.

I know that using less variables is always a better choice if you can do it, it might not matter on such small scale projects but I believe it's useful to get in the habit head-on. So in this specific example code, can I use a single var instead of 4 different ones? I know I can for the second and third tasks, however what about the third? Can I still list the inputs that are less than or equal to 25?
What tips do you have to make the code readable and even more easy to understand?

Also keep in mind that using loops, arrays, functions ... is not allowed so I would be glad if we keep our discussion limited to conditionals only.

#include "stdafx.h"
#include "iostream"
using namespace std;

int _tmain(int argc, _TCHAR* argv[])
{
    float val1, val2, val3, val4;
    float sum = 0, multiply = 1;

    cout << "Enter the first number: ";
    cin >> val1;
    if(val1 < 20) {
        multiply *= val1;
    } else if(val1 > 25) {
        sum += val1;
    }

    cout << "Enter the second number: ";
    cin >> val2;
    if(val2 < 20) {
        multiply *= val2;
    } else if(val2 > 25) {
        sum += val2;
    }

    cout << "Enter the third number: ";
    cin >> val3;
    if(val3 < 20) {
        multiply *= val3;
    } else if(val1 > 25) {
        sum += val3;
    }

    cout << "Enter the fourth number: ";
    cin >> val4;
    if(val4 < 20) {
        multiply *= val4;
    } else if(val4 > 25) {
        sum += val4;
    }

    if(val1 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val1 << endl;
    if(val2 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val2 << endl;
    if(val3 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val3 << endl;
    if(val4 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val4 << endl;

    cout << "When we multiply the numbers below 20 we get: " << multiply << endl;
    cout << "When we add the numbers above 25 we get: " << sum << endl;

    return 0;
}

Regarding the number of variables:

For displaying all "small" values, you could introduce a variable of type string in which you save all small numbers. But since you haven't yet learned about functions and arrays, this would make your code much more complicated than it is now.

So for now, just keep the program as-is, using one variable per number.

In the rest of the review, I'm going straight from the top to the bottom of your code.

#include "stdafx.h"
#include "iostream"

The #include "stdafx.h" is fine. When an include file comes from your own project (which stdafx.h does), you should surround it with double quotes.

But all other include files (especially system headers like iostream) should not be enclosed in "double quotes" but in <angle brackets>. So it should be #include <iostream>.

using namespace std;

As soon as you start writing serious code, you should omit the using namespace std. This line imports several hundred names from the std namespace, such as min, max, sort, depending on the system headers you included. This can lead to confusion when you make typos or just forget to declare a function that is also defined in that namespace.

int _tmain(int argc, _TCHAR* argv[])
{
    float val1, val2, val3, val4;
    float sum = 0, multiply = 1;

Whenever possible, use the data type double instead of float, since it is more precise (15 digits instead of 7).

    cout << "Enter the first number: ";
    cin >> val1;

Imagine what happens when the user of the program doesn't enter a number here but simply types hello program. how are you today?. In that case val1 will not get any value, and whatever value was stored there before will be used by the program. This leads to unpredictable behavior.

To fix this, write the following code:

    if (!(std::cin >> val1)) {
        std::cerr << "Error: could not read the first number\n";
        return 1; // Any nonzero value means failure.
    }

Continuing with your code …

    if(val1 < 20) {
        multiply *= val1;
    } else if(val1 > 25) {
        sum += val1;
    }

Very nice. Short and readable. I would just change the variable name multiply into product, since variables usually get passive names. Later, when you write functions, these will get the active names, like multiply two numbers.

    cout << "Enter the second number: ";
    cin >> val2;
    if(val2 < 20) {
        multiply *= val2;
    } else if(val2 > 25) {
        sum += val2;
    }

    cout << "Enter the third number: ";
    cin >> val3;
    if(val3 < 20) {
        multiply *= val3;
    } else if(val1 > 25) {
        sum += val3;
    }

    cout << "Enter the fourth number: ";
    cin >> val4;
    if(val4 < 20) {
        multiply *= val4;
    } else if(val4 > 25) {
        sum += val4;
    }

    if(val1 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val1 << endl;
    if(val2 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val2 << endl;
    if(val3 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val3 << endl;
    if(val4 <= 25) cout << "The number(s) below or equal to 25 are/is: " << val4 << endl;

The are/is doesn't look convincing. To make the output nice, you could count the numbers that are less than or equal to 25. Then, add the following code:

    if (lessOrEqual25Count == 0) {
        std::cout << "No numbers are below or equal to 25.\n";

    } else {
        if (lessOrEqual25Count == 1) {
            std::cout << "One number is below or equal to 25:";
        } else {
            std::cout << lessOrEqual25Count << " numbers are below or equal to 25:";
        }
        if (val1 <= 25) std::cout << " " << val1;
        if (val2 <= 25) std::cout << " " << val2;
        if (val3 <= 25) std::cout << " " << val3;
        if (val4 <= 25) std::cout << " " << val4;
        std::cout << "\n";
    }

Continuing with your code …

    cout << "When we multiply the numbers below 20 we get: " << multiply << endl;
    cout << "When we add the numbers above 25 we get: " << sum << endl;

    return 0;
}

All in all, this is a nice program. It works fine, and it's good that you will learn about arrays and functions soon. They will really make programming easier, since currently you have to repeat large sections of your code over and over.


Speed up a program to find the minimum Hamming distance I have a solution to the puzzle at https://kattis.csc.kth.se/problem?id=codes.
What can I do to make my algorithm much much faster? What the algorithm does is take an N×K matrix and search for the minimum distance — that is, the least number of 1 for all the products of the matrix and a vector that ranges from 00001 to 11111 (where the top is 2K - 1).

#include <iostream>

using namespace std;

void DataEntry(int &a, int &b, int** &matrix) {

    cin >> a >> b;

    matrix = new int*[a];

    for (int i = 0; i < a; i++) {
        matrix[i] = new int[b];
    }

    for (int i = 0; i < a; i++) {
        for (int j = 0; j < b; j++) {
            matrix[i][j] = -1;
            cin >> matrix[i][j];
        }
    }

}

int* BinaryAdd(int* a, int* b, int tam) {
    int* c;
    int ac = 0;
    c = new int[tam];

    for (int i = tam - 1; i > -1; i--) {
        c[i] = a[i] + b[i] + ac;
        if (c[i] == 2) {
            c[i] = 0;
            ac = 1;
        } else
            ac = 0;
    }

    return c;
}

int* MultiplyMatrices(int** a, int* b, int tamF, int tamC) {
    int * c;
    c = new int[tamF];
    for (int i = 0; i < tamF; i++) {
        c[i] = 0;
        for (int j = 0; j < tamC; j++) {
            c[i] = c[i] + (a[i][j] * b[j]);
            c[i] = c[i] % 2;
        }
    }
    return c;
}

int MinimumDistance(int* a, int tam) {
    int MD = 0;

    for (int i = 0; i < tam; i++) {
        if (a[i] == 1)
            MD++;
    }
    return MD;
}

int pot(int a, int b) {
    int c = 1;
    for (int i = 1; i < b + 1; i++) {
        c = a * c;
    }
    return c;
}

int main() {
    int t = 0, n = 0, k = 0, d = -1, max = -1;
    int ** lecc;
    int * pro;
    int * sal;
    int * sol;
    int * zero;
    int * uno;
    int * pro1;

    cin >> t;

    for (int p = 0; p < t; p++) {

        DataEntry(n, k, lecc);

        pro = new int[k];
        uno = new int[k];
        sal = new int[n];
        zero = new int[n];

        for (int i = 0; i < k; i++) {
            pro[i] = 0;
        }
        for (int i = 0; i < k - 1; i++) {
            uno[i] = 0;
        }
        uno[k - 1] = 1;

        for (int i = 0; i < n; i++) {
            zero[i] = 0;
        }

        pro1 = BinaryAdd(pro, uno, k);

        pro = pro1;

        max = pot(2, k);

        sal = MultiplyMatrices(lecc, pro, n, k);

        d = MinimumDistance(sal, n);

        for (int i = 0; i < max - 1; i++) {

            sal = MultiplyMatrices(lecc, pro, n, k);

            pro1 = BinaryAdd(pro, uno, k);

            pro = pro1;

            int ad = MinimumDistance(sal, n);

            if (ad < d)
                d = ad;
        }
        cout << d << endl;
    }

}

Edit: I give you a sample input and output

Sample input     Sample output
2
7 4
1 0 0 0
0 1 0 0
0 0 1 0           3
0 0 0 1           0
0 1 1 1
1 0 1 1
1 1 0 1
3 2
1 1
0 0
1 1

Considering that this is meant to be a puzzle, I'll write my review as a sequence of escalating hints.

Everybody who is suggesting the use of threads or std::vector or matrix libraries is on the wrong track! How do you think error correction is implemented in your CD player or hard disk firmware? No threads or matrix libraries there, I assure you.
Don't simulate the computer; use the computer.
All operations are modulo 2.  The puzzle guarantees k ≤ 16. Under these conditions, you can massively parallelize vector dot products. (Think SIMD or AltiVec, except you don't need SIMD or AltiVec.)
I've added the bitwise tag to this question.

I'll conclude with a remark that you've basically written C code.  The only thing that makes it C++ is your use of iostream.  C++ can be much more expressive than that.


Arranging Buttons on a Process Enumerating Application I had the following piece of code for populating a list of currently running processes in an IT Self-Help application :

var proclist = ProcessHelper.GetProcList();

var i = 0;
var j = 0;

foreach (var process in proclist)
{
    if (process.ButtonText == "") continue;
    var button = new Button();

    if (i < 7)
    {
        button.Location = new Point(10, 30 * i + 65);
    }
    else if ((i > 6) & (i < 14))
    {
        button.Location = new Point(150, 30 * j + 65);
        j++;
    }
    else
    {
        button.Location = new Point(290, 30 * j + 65);
        j++;
    }

    i++;

    button.Click += (x, y) => button.Visible = false;
    button.Click += (x, y) => process.Kill();

    button.Text = process.ButtonText;

    button.Image = process.ThumbNail;
    button.ImageAlign = ContentAlignment.MiddleLeft;

    processTab.Controls.Add(button);

    button.Width = 130;
    button.Height = 25;
    button.TextAlign = ContentAlignment.MiddleLeft;
}

And today I realised, that if a user has more than 14 applications running, my logic falls apart on the third column and displays the items like this :

I managed to fix this with the following amendment (added a new variable for the third column) :

var proclist = ProcessHelper.GetProcList();

// For placing each button - need a var for each column
var i = 0;
var j = 0;
var k = 0;

foreach (var process in proclist)
{
    if (process.ButtonText == "") continue;
    var button = new Button();

    if (i < 7)
    {
        button.Location = new Point(10, 30 * i + 65);
    }
    else if ((i > 6) & (i < 14))
    {
        button.Location = new Point(150, 30 * j + 65);
        j++;
    }
    else
    {
        button.Location = new Point(290, 30 * k + 65);
        k++;
    }

    i++;

    button.Click += (x, y) => button.Visible = false;
    button.Click += (x, y) => process.Kill();

    button.Text = process.ButtonText;

    button.Image = process.ThumbNail;
    button.ImageAlign = ContentAlignment.MiddleLeft;

    processTab.Controls.Add(button);

    button.Width = 130;
    button.Height = 25;
    button.TextAlign = ContentAlignment.MiddleLeft;
}

And now the items appear correctly :

However, I couldn't help but think that this is not a very clean fix, because I just hard-coded in another column, and if a user every has 21 + applications open, the issue will occur again!

finally, I came up with this :

var i = 0;
var j = 0;
var column = 10;

foreach (var process in proclist)
{
    if (process.ButtonText == "") continue;
    var button = new Button();

    if ((i > 0) & ((i % 7) == 0))
    {
        j = 0;
        column = column + 140;
    }

    button.Location = new Point(column, 30 * j + 65);

    j++;
    i++;

    button.Click += (x, y) => button.Visible = false;
    button.Click += (x, y) => process.Kill();

    button.Text = process.ButtonText;

    button.Image = process.ThumbNail;
    button.ImageAlign = ContentAlignment.MiddleLeft;

    processTab.Controls.Add(button);

    button.Width = 130;
    button.Height = 25;
    button.TextAlign = ContentAlignment.MiddleLeft;
}

Which (as far as I can tell), will arrange the buttons correctly as long as there aren't so many that they go out of the window (which is highly unlikely in this environment). Here is a screenshot of the new version in action :

Hopefully this is the right place to post this - any feedback is extremely welcome!

What will happen when there are more processes than you can fit buttons for on your form?

You're doing the hard work by hand, reinventing a wheel that's already there for you... and as you saw, it's not exactly scaling very well.

The wheel in question is called a FlowLayoutPanel, a winforms layout control that automatically handles control positioning for you, taking overflowing controls to a new row or column, depending on how you configured it - it even handles scrollbars for you, if you have more items than can fit into the panel.

That said, your wheel has a number of squeaky parts:

if (process.ButtonText == "") continue;

Comparing a string against an empty string, would usually be written like this:

if (string.IsNullOrWhiteSpace(process.ButtonText)
{
    continue;
}

In all versions/attempts of your layout code, you hard-coded the key values.

140 / the column width, should be computed from the width of a button
7 is arbitrary and should be allowed to change if/when the form is maximized or resized. Your screenshots crop the top part, but if you haven't removed the min/maximize buttons and allow resizing of the form, the user can easily wreck your layout; it doesn't take the button height into consideration either.

BUG?

It's a little beyond the scope of this review, but this line can potentially blow everything up:

button.Click += (x, y) => process.Kill();

There's a chance your application will not-so-graciously crash and burn if you bring up your app, then start task manager and kill a process, and then click the button for that process in your app.

The process to kill won't exist anymore, and if ProcessHelper.Kill() doesn't handle that situation, boom! Make sure your wrapper wraps the operation in a try..catch block.


Concordance index calculation I am trying to calculate a customized concordance index for survival analysis. Below is my code. It runs well for small input dataframe but extremely slow on a dataframe with one million rows (>30min).

import pandas as pd

def c_index1(y_pred, events, times):
    df = pd.DataFrame(data={'proba':y_pred, 'event':events, 'time':times})
    n_total_correct = 0
    n_total_comparable = 0
    df = df.sort_values(by=['time'])
    for i, row in df.iterrows():
        if row['event'] == 1:
            comparable_rows = df[(df['event'] == 0) & (df['time'] > row['time'])]
            n_correct_rows = len(comparable_rows[comparable_rows['proba'] < row['proba']])
            n_total_correct += n_correct_rows
            n_total_comparable += len(comparable_rows)

    return n_total_correct / n_total_comparable if n_total_comparable else None

c = c_index([0.1, 0.3, 0.67, 0.45, 0.56], [1.0,0.0,1.0,0.0,1.0], [3.1,4.5,6.7,5.2,3.4])
print(c) # print 0.5

For each row （in case it matters...）:

If the event of the row is 1: retrieve all comparable rows whose

index is larger (avoid duplicate calculation),
event is 0, and
time is larger than the time of the current row. Out of the comparable rows, the rows whose probability is less than the current row are correct predictions.

I guess it is slow because of the for loop. How should I speed up it?

You will not get dramatic speedups untill you can vectorize the operations, but here are some tips already

indexing before iterating

instead of

for i, row in df.iterrows():
    if row['event'] == 1:

If you do

for i, row in df[df['event'] == 1].rows():

you will iterate over less rows.

itertuples

generally, itertuples is faster than iterrows

comparable_rows

for comparable_rows you are only interested in the proba and the length, so you might as well make this into a Series, or even better, a numpy array.

The test (df['event'] == 0) doesn't change during the iteration, so you can define a df2 = df[df['event'] == 0] outside of the loop

n_correct_rows

instead of len(comparable_rows[comparable_rows['proba'] < row['proba']]), you can use the fact that True == 1 do (comparable_rows['proba'] < row.proba).sum()

result

def c_index3(y_pred, events, times):
    df = pd.DataFrame(data={'proba':y_pred, 'event':events, 'time':times})
    n_total_correct = 0
    n_total_comparable = 0
    df = df.sort_values(by=['time'])
    df2 = df.loc[df['event'] == 0]
    for row in df[df['event'] == 1].itertuples():
        comparable_rows = df2.loc[(df2['time'] > row.time), 'proba'].values
        n_correct_rows = (comparable_rows < row.proba).sum()
        n_total_correct += n_correct_rows
        n_total_comparable += len(comparable_rows)

    return n_total_correct / n_total_comparable if n_total_comparable else N

timings

data = ([0.1, 0.3, 0.67, 0.45, 0.56], [1.0,0.0,1.0,0.0,1.0], [3.1,4.5,6.7,5.2,3.4])
%timeit c_index1(*data)

5.17 ms ± 33.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

%timeit c_index3(*data)

3.77 ms ± 160 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)


Join ArcGIS tables in Python of plant greenness in response to prior month's rainfall I am using Python 3.8 with ArcGIS Pro 2.5. This code creates a joined table where I match values for plant greenness in the current month with rainfall X number of months into the past. Each loop iterates through about 600 files (the number of files in ws1 and ws2). The code works perfectly but slowly, taking approximately four hours to run. For my work (a grad student in plant biology) I will need to run this code multiple times.

I am interested in executing the first loop in parallel, however, I've been stumped about how to do so. Any advice about how to optimize this code to run in parallel (using the multiprocessing or other package) would be appreciated. I am using an 8-core 16-thread CPU.

Here is what the code does:

The code first iterates through ws1, which contains tables of plant greenness. The date is isolated from the file name, and then I create a new variable X months before this date. The second loop iterates through ws2, which contains tables of precipitation. Similarly, I isolate the date from the filename, and then create a new variable X months into the past. The final loop iterates through ws2 again and finds the historic filename that matches the one created in the second loop.

r from the first loop is associated with plant greenness at the current month, r2 is associated with precipitation at the current month, while r3 from the last loop is associated with historic precipitation. When the dates from all three loops match then I do the add join.

import arcpy, os, glob, datetime
arcpy.env.addOutputsToMap = 0
from datetime import datetime

ws1=glob.glob(r'D:\GIS_Files\NEW_LA_FULL_SERIES\ALL_DATA_2000_2020\CITY_POLYs\*.shp')
ws2 = glob.glob(r'D:\GIS_Files\NEW_LA_FULL_SERIES\ALL_DATA_2000_2020\PRECIP_preprocessed_2000_2020\CITY_ZONAL_TABLES\*.dbf')

for r in ws1:
    basename_BOUND = os.path.basename(r)[35:41]
    basename_BOUND1=str(basename_BOUND)
    D_POLY=datetime.strptime(basename_BOUND1,'%Y%m').date()
    from datetime import datetime
    import dateutil.relativedelta
    EARLIER_POLY=D_POLY-dateutil.relativedelta.relativedelta(months=2)
    STR_PREV_POLY="{:%Y%m}".format(EARLIER_POLY)
    print(STR_PREV_POLY)
    for r2 in ws2:
        basename_ZONAL=os.path.basename(r2)[12:18]
        basename_ZONAL1=str(basename_ZONAL)
        D_PRECIP= datetime.strptime(basename_ZONAL1,'%Y%m').date()
        from datetime import datetime
        import dateutil.relativedelta
        EARLIER_PRECIP=D_PRECIP-dateutil.relativedelta.relativedelta(months=2)
        STR_PREV_PRECIP="{:%Y%m}".format(EARLIER_PRECIP)
        for r3 in ws2:
            basename_FINAL=os.path.basename(r3)[12:18]
            print(basename_FINAL)
            if STR_PREV_POLY==STR_PREV_PRECIP:
                if STR_PREV_PRECIP==basename_FINAL:
                    result = arcpy.JoinField_management(in_data=r, in_field="GEO_id",join_table=r3, join_field="GEO_id", fields=["MEAN","STD"])[0]

You should do a maintainability pass before you start optimizing. In that spirit:

Imports

from datetime import datetime
import dateutil.relativedelta

should be at the top of the file, and should not be repeated. Also, this:

arcpy.env.addOutputsToMap = 0

should be moved past the imports section.

Pathlib

These:

ws1=glob.glob(r'D:\GIS_Files\NEW_LA_FULL_SERIES\ALL_DATA_2000_2020\CITY_POLYs\*.shp')
ws2 = glob.glob(r'D:\GIS_Files\NEW_LA_FULL_SERIES\ALL_DATA_2000_2020\PRECIP_preprocessed_2000_2020\CITY_ZONAL_TABLES\*.dbf')

should get a little love from pathlib:

Factor out the common directory -

data_dir = Path(r'D:\GIS_Files\NEW_LA_FULL_SERIES\ALL_DATA_2000_2020')
ws1 = (data_dir / 'CITY_POLYs').glob('*.shp')
ws2 = (data_dir / 'PRECIP_preprocessed_2000_2020' / 'CITY_ZONAL_TABLES').glob('*.dbf')

Hard-coded string slices

These:

[35:41]
[12:18]

are a nightmare. I don't know what they're actually selecting, but I can nearly guarantee that there's a better way. It seems like a slice out of a path, and paths are structured - you should not have to do indexing like this.

Nomenclature

You, currently, understand what these variables mean:

ws1
ws2
r
r2
r3
result

But no one else does, and you might not in six months. Do yourself a favour and give these meaningful names.

Logical combination

        if STR_PREV_POLY==STR_PREV_PRECIP:
            if STR_PREV_PRECIP==basename_FINAL:

should simply be

if STR_PREV_POLY == basename_FINAL:

That intermediate variable has no effect.


Traversing two trees until a common value has been found I am traversing two objects that may share a common ancestor. To make the code small enough to show here, I have changed it into two dictionaries with values. As you can see, a_d and b_d are the inverse of each other. The idea is to step into each dictionary step by step until we find an overlapping value for the two. At each step the 'distance' is increased.

For the example case get_distance('b', 'y'), the output will print (comments added):

# init
start= b
end= y

# both a and b changed value (dist +1 for both) but no overlap found
start= c
end= x

# both a and b changed value (dist +1 for both) and an overlap was found!
start= d
end= d

Found overlap d
connected True

I hope that it is clear what this code does and what I am trying to do. Please keep in mind that the actual code works with elaborate objects and their properties, but the idea is similar. The issue that I am having is that the code is very repetitive and probably not as efficient as it could be. But I am not sure how I can improve on it. Ideas welcome!

def get_distance(start, end):
    start_d = {
        'a': 'b',
        'b': 'c',
        'c': 'd',
        'd': 'x',
        'x': 'y',
        'y': 'z',
        'z': 'z'
    }

    end_d = {
        'z': 'y',
        'y': 'x',
        'x': 'd',
        'd': 'c',
        'c': 'b',
        'b': 'a',
        'a': 'a'
    }

    connected = False
    start_dist = 0
    end_dist = 0
    while True:
        print('start=', start)
        print('end=', end)
        print()

        # Check whether a and b are the same, if so break
        if start == end:
            print(f"Found overlap {end}")
            connected = True
            break
        # Check whether the next a value is the same as b, if so break and increase a_dist
        elif start_d[start] == end:
            print(f"Found overlap {end}")
            start_dist += 1
            connected = True
            break
        # Check whether the next b value is the same as a, if so break and increase b_dist
        elif end_d[end] == start:
            print(f"Found overlap {start}")
            end_dist += 1
            connected = True
            break

        # If for a and b all options are exhausted, break
        if start == start_d[start] and end == end_d[end]:
            break

        # If a is not exhausted, get its dict value and increase distance
        if start != start_d[start]:
            start_dist += 1
            start = start_d[start]

        # If b is not exhausted, get its dict value and increase distance
        if end != end_d[end]:
            end_dist += 1
            end = end_d[end]

    dist = start_dist + end_dist
    print('connected', connected)
    return dist

if __name__ == '__main__':
    dist = get_distance('b', 'y')

Docstrings: You should include a docstring at the beginning of every method, class, and module you write. This will help documentation identify what your methods/classes/modules are supposed to accomplish.
Simplicity: Rather than dist = start_dist + end_dist then returning dist, you can simply return start_dist + end_dist. You can return the addition of the two values, rather than creating a variable to only use it once to return. This is a personal preference, but it helps reduce the number of variables in the program, which helps me; and may also help you.
Constants: Any constants in your program should be UPPERCASE. You only have one in your code, but you should keep in practice for when you write bigger programs.
Printing: You use print(..., ...) and print(f"...") interchangeably. You should stick to using only one for consistency sake. As my preference, I changed all of them to print(f"..."), but you change choose your preference.

Updated Code

"""
Traverses two objects that may share a common ancestor
"""

def get_distance(start, end):
    """
    Gets the distance from `start` to `end` in the trees
    """
    start_d = {
        'a': 'b',
        'b': 'c',
        'c': 'd',
        'd': 'x',
        'x': 'y',
        'y': 'z',
        'z': 'z'
    }

    end_d = {
        'z': 'y',
        'y': 'x',
        'x': 'd',
        'd': 'c',
        'c': 'b',
        'b': 'a',
        'a': 'a'
    }

    connected = False
    start_dist = 0
    end_dist = 0

    while True:
        print(f"Start: {start}")
        print(f"End: {end} \n")

        # Check whether a and b are the same, if so break
        if start == end:
            print(f"Found overlap: {end}")
            connected = True
            break

        # Check whether the next a value is the same as b, if so break and increase a_dist
        elif start_d[start] == end:
            print(f"Found overlap: {end}")
            start_dist += 1
            connected = True
            break

        # Check whether the next b value is the same as a, if so break and increase b_dist
        elif end_d[end] == start:
            print(f"Found overlap: {start}")
            end_dist += 1
            connected = True
            break

        # If for a and b all options are exhausted, break
        if start == start_d[start] and end == end_d[end]:
            break

        # If a is not exhausted, get its dict value and increase distance
        if start != start_d[start]:
            start_dist += 1
            start = start_d[start]

        # If b is not exhausted, get its dict value and increase distance
        if end != end_d[end]:
            end_dist += 1
            end = end_d[end]

    print(f"Connected: {connected}")
    return start_dist + end_dist

if __name__ == '__main__':
    DISTANCE = get_distance('b', 'y')
    print(f"Distance: {DISTANCE}")


Implementing a binary tree in C++ using "std::unique_ptr" I have implemented a simple binary search tree class in C++ using std::unique_ptr objects to hold the pointers to each node. In doing this I have come across a situation that is somewhat questionable in the Delete method. Since I can't copy the unique pointers, but I need a way to traverse the tree with a "temporary" pointer, I had to resort to using a raw pointer to do the traversal.

This is the section I am talking about (In the Delete method):

// Need to use a raw pointer because I can't assign to a std::unique_ptr
Node* n = node.get();
while (n->right) {

  // Using a reference to a unique_ptr instead of a raw pointer will cause this line to fail to compile
  n = n->right.get();
}

Is this an acceptable thing to do?

It compiles and runs. And since the underlying pointer is still managed by a unique_ptr object it will be deleted properly. Everything about this works fine, but it just doesn't feel right for some reason.

The full source code is shown below (Feel free to comment on anything else that may be improved as well):

#include <iostream>
#include <memory>

using std::make_unique;
using std::unique_ptr;

template <typename T>
class Tree {

  struct Node {
    Node(const T& value)
      : value(value), left(nullptr), right(nullptr) {}

    T value;
    unique_ptr<Node> left;
    unique_ptr<Node> right;
  };

public:
  Tree() : root_(nullptr) {}

  // Insert a value into the tree
  void Insert(const T& value) {
    Insert(root_, value);
  }

  // Delete a value from the tree
  bool Delete(const T& value) {
    return Delete(root_, value);
  }

  // Search the tree for a node and return true if it is found
  bool Contains(const T& value) const {
    return Contains(root_, value);
  }

private:
  void Insert(unique_ptr<Node>& node, const T& value) {
    if (not node) {
      node = make_unique<Node>(value);
    }
    else {
      value < node->value
        ? Insert(node->left, value)
        : Insert(node->right, value);
    }
  }

  bool Delete(unique_ptr<Node>& node, const T& value) {
    if (not node) {
      return false;
    }
    else if (value == node->value) {
      if (node->left) {
        unique_ptr<Node>& right = node->right;
        node = move(node->left);
        if (right) {

          // Need to use a raw pointer because I can't assign to a std::unique_ptr
          Node* n = node.get();
          while (n->right) {

            // Using a reference to a unique_ptr instead of a raw pointer will cause this line to fail to compile
            n = n->right.get();
          }
          n->right = move(right);
        }
      }
      else {
        node = move(node->right);
      }
      return true;
    }
    else {
      return value < node->value
        ? Delete(node->left, value)
        : Delete(node->right, value);
    }
  }

  bool Contains(const unique_ptr<Node>& node, const T& value) const {
    if (not node) {
      return false;
    }
    else if (node->value == value) {
      return true;
    }
    else {
      return value < node->value
        ? Contains(node->left, value)
        : Contains(node->right, value);
    }
  }

  unique_ptr<Node> root_;
};

On raw and smart pointers

First, I'll address your specific question, about using a raw pointer to refer to nodes in the tree.  I believe you're doing exactly the right thing here, because smart pointers convey information about ownership of the resource.

We are looking at nodes that are owned by the tree (actually, each node is owned by its parent), and it's entirely appropriate that that ownership is encoded as a std::unique_ptr; that says that this is the only place that's responsible for deleting the node when it's no longer required.

Our Delete method walks nodes from the root downwards; it needs to refer to the nodes, but only during the execution of the method (it doesn't store pointers anywhere).  A raw pointer is perfectly suitable here.

General review

Is <iostream> used anywhere?  I think it can be omitted.

If this is a header file, avoid bringing std::make_unique and std::unique_ptr into the global namespace, as that will affect every file that includes the header.  It's reasonably to do so inside the function bodies, but at that point it's likely better to simply use the qualified names.  Including them within the scope of the type is a middle ground where opinions will differ.

We should include <utility> for std::move (and the same comments apply to importing that name into ::).

Consider turning tests around to test a positive first, which slightly reduces cognitive load.  Instead of:

if (not node) {
  node = make_unique<Node>(value);
}
else {
  value < node->value
    ? Insert(node->left, value)
    : Insert(node->right, value);
}

This is easier to reason about:

if (node) {
  value < node->value
    ? Insert(node->left, value)
    : Insert(node->right, value);
}
else {
  node = make_unique<Node>(value);
}

Actually, that ternary can be narrowed to within the argument list (you can add more parens according to taste):

if (node) {
  Insert(value < node->value ? node->left : node->right, value);
}
else {
  node = make_unique<Node>(value);
}

On a related note, if we return early from within if, we can just flow into the next code without else:

if (not node) {
  return false;
}
else if (node->value == value) {
  return true;
}
else {
  return value < node->value
    ? Contains(node->left, value)
    : Contains(node->right, value);
}

That can become

if (not node) {
  return false;
}
if (node->value == value) {
  return true;
}

return Contains(value < node->value ? node->left : node->right, value);

We might be able to remove some repetition by adding a method to find the insertion point for a value - Contains() can return true if that insertion point has a value that compares equal, and Delete() can start from that point in removing the value.

If the tree could be large, then perhaps iteration may be better than recursion for walking the tree.  That means we'll need pointers in some places where we currently have references, and some things may have to lose const qualification, but may give a performance improvement and save on stack usage (or may not, depending on how your compiler manages to optimise the recursive call - always check these things!).

Further directions

Try to evolve the public interface to supporting standard container operations.  This probably begins with creating iterators and the member functions that return them.

Consider maintaining a balanced tree - you'll want to read up on the theory if you're not already familiar with the techniques to do this.

You haven't shown your unit tests here, but it's worthwhile occasionally running them with a memory checker such as Valgrind.  As well as ensuring we don't leak memory, that will also help identify any uses of dangling pointers or uninitialized values, which an ordinary run might not.


GCD calculation - A cross of the Euclidean and the Binary algorithm I was reading Knuth's The Art of Computer Programming - Volume 2 (Seminumerical Algorithms) Third Edition. I saw the two algorithms (A and B). A method (due to V. C. Harris) was mentioned (in page 341), which I quote:

  V. C. Harris [...] has suggested an interesting cross between Euclid's algorithm and the binary algorithm. If \$u\$ and \$v\$ are odd, with \$u\geq v>0\$, we can always write
  $$u=qv\pm r$$ where \$0\leq r<v\$ and \$r\$ is even; if \$r\neq0\$ we set \$r\leftarrow r/2\$ until \$r\$ is odd, then set \$u \leftarrow v, v \leftarrow r\$ and repeat the process. In subsequent iterations, \$q\geq3\$

Now, I've noted that often \$r\$ turns out to be grater than its original value i.e. if we would have used the simple equation, shown below: $$u=qv+r$$just like in the ordinary Euclidean algorithm and then I've tried to halve \$r\$ (if \$r\$ is even). Now, I've implemented this approach in Java as follows (I am just providing the method):

public static long gcd(long a, long b) {
    int a0 = Long.numberOfTrailingZeros(a);
    int b0 = Long.numberOfTrailingZeros(b);
    // Make both of them odd.
    a >>>= a0;
    b >>>= b0;
    // t is the common power of 2 (may be 0)
    int t = a0 < b0 ? a0 : b0;
    while ((a & b) != 0) {  // i.e. both are non-zero
        if (a > b) {
            a %= b;
            a >>>= Long.numberOfTrailingZeros(a);
        } else {
            b %= a;
            b >>>= Long.numberOfTrailingZeros(b);
        }
    }
    return (a | b) << t;    // {the non-zero value of the two} * 2^t
}

Now, I'd like to ask:

Does my approach have any significant advantages?
Can there be (I'm sure there will be) any improvements?
Would you use it in your library? :-)

PS. I am using the if to remove the need of a third temporary variable.

Introduction

Rabindranath Tagore said:

  "যদি তোর ডাক শুনে কেউ না আসে তবে একলা চলো রে..."
  Translated: "If no one responds to your call, then go your own way alone..." 1

After waiting for two whole days in vain, I've decided to answer my own question. In these two days, I've tried several methods and made certain interesting observations. Here, I shall present them in a logical order.

First, the Euclidean algorithm

Here is the implementation of the "Modern Euclidean Algorithm" (Algorithm A, TAOCP Vol - 2, Pg - 337, Section - 4.5.2) in Java:

/**
 * Returns the GCD (Greatest Common Divisor, also known as the GCF -
 * Greatest Common Factor, or the HCF - Highest Common Factor) of the two
 * (signed, long) integers given.
 *

 * It calculates the GCD using the Euclidean GCD algorithm.
 *
 * @param u The first integer (preferably the larger one).
 * @param v The second integer (preferably the smaller one).
 * @return The GCD (or GCF or HCF) of {@code u} and {@code v}.
 */
public static long euclideanGCD(long u, long v) {
    // Corner cases
    if (u < 0) u = -u;
    if (v < 0) v = -v;
    if (u == 0) return v;
    if (v == 0) return u;
    // Correction
    if (u < v) {
        long t = u;
        u = v;
        v = t;
    }
    // A1
    while (v != 0) {
        // A2
        long r = u % v;
        u = v;
        v = r;
    }
    return u;
}

In order to test it, I wrote this class:

class Test {
    public static void main(String[] args) {
        // Warm-up
        int g = 0;
        while (++g > 0)
            Math.sqrt(g);
        // Timing starts
        long time = System.nanoTime();
        long sum = 0;
        for (int i = 0; i < 20000; i++) {
            for (int j = 0; j < i; j++) {
                sum += GCD.euclideanGCD(i, j);  // Over here
            }
        }
        time = System.nanoTime() - time;
        System.out.println("sum  = " + sum);
        System.out.println("time = " + time);
    }
}

Here is its output:

sum  = 1352820356
time = 35306053968

So, it took 35.3 seconds on my Windows 7 Laptop with a 2.1 Gz Intel Pentium processor. (If we run the test again, the time will of course be different, but it will be in the range: 34-36 seconds). We shall use this time as the reference point (and the sum to test the correctness of the other methods). Now, let's try and improve upon this. Here are a few points to note:

This method is horribly slow because of the divisions (modulo operations) that it has to perform. We can improve upon this by making every effort to make the divisors smaller.
It also has so many (rather unnecessary) assignment operations. This is also a factor that reduces the speed.

Keeping these points in mind, I wrote the method that I had provided in the question. For convenience, here it is again (now well documented):

The "Better" Euclidean algorithm

/**
 * Returns the GCD (Greatest Common Divisor, also known as the GCF -
 * Greatest Common Factor, or the HCF - Highest Common Factor) of the two
 * (signed, long) integers given.
 *

 * It calculates the GCD using a modified form of the Euclidean GCD algorithm.
 *
 * @param u The first integer.
 * @param v The second integer.
 * @return The GCD (or GCF or HCF) of {@code u} and {@code v}.
 */
public static long euclideanGCD2(long u, long v) {
    // Corner cases
    if (u < 0) u = -u;
    if (v < 0) v = -v;
    int a0 = Long.numberOfTrailingZeros(u);
    int b0 = Long.numberOfTrailingZeros(v);
    // Make both of them odd.
    u >>>= a0;
    v >>>= b0;
    // t is the common power of 2 (may be 0)
    int t = a0 < b0 ? a0 : b0;
    while ((u & v) != 0) {  // i.e. both are non-zero
        if (u > v) {
            u %= v;
            u >>>= Long.numberOfTrailingZeros(u);
        } else {
            v %= u;
            v >>>= Long.numberOfTrailingZeros(v);
        }
    }
    return (u | v) << t;    // {the non-zero value of the two} * 2^t
}

We test again by changing the line marked withe the comment to sum += GCD.euclideanGCD2(i, j);

Here is the output:

sum  = 1352820356
time = 26400006940

This is a definite improvement (of around 25.2%), and this is about as far as we can go without any other help. But can go further if we use the Binary GCD algorithm. So here it is:

The binary GCD algorithm

/**
 * Returns the GCD (Greatest Common Divisor, also known as the GCF -
 * Greatest Common Factor, or the HCF - Highest Common Factor) of the two
 * (signed, long) integers given.
 *

 * This uses the Binary GCD algorithm.
 *
 * @param a The first integer.
 * @param b The second integer.
 * @return The GCD (or GCF or HCF) of {@code a} and {@code b}.
 */
public static long binaryGCD(long a, long b) {
    // Corner cases
    if (a < 0) a = -a;
    if (b < 0) b = -b;
    if (a == 0) return b;
    if (b == 0) return a;
    int a0 = Long.numberOfTrailingZeros(a);
    int b0 = Long.numberOfTrailingZeros(b);
    int t = a0 < b0 ? a0 : b0;
    a >>>= a0;
    b >>>= b0;
    while (a != b) {
        if (a > b) {
            a -= b;
            a >>>= Long.numberOfTrailingZeros(a);
        } else {
            b -= a;
            b >>>= Long.numberOfTrailingZeros(b);
        }
    }
    return a << t;
}

The output is:

sum  = 1352820356
time = 13381525510

So, the improvement is quite dramatic (around 60% compared to the original). Now, we know that this algorithm slows down when \$u\$ and \$v\$ have very different bit lengths. So, let's try and improve this:

The "Hybrid" GCD algorithm

/**
 * Returns the GCD (Greatest Common Divisor, also known as the GCF -
 * Greatest Common Factor, or the HCF - Highest Common Factor) of the two
 * (signed, long) integers given.
 *

 * It uses a hybrid of Euclidean and the binary GCD algorithm.
 *
 * @param a The first integer.
 * @param b The second integer.
 * @return The GCD (or GCF or HCF) of {@code a} and {@code b}.
 */
public static long hybridGCD(long a, long b) {
    // Corner cases
    if (a < 0) a = -a;
    if (b < 0) b = -b;
    if (a == 0) return b;
    if (b == 0) return a;
    int a0 = Long.numberOfTrailingZeros(a);
    int b0 = Long.numberOfTrailingZeros(b);
    int t = a0 < b0 ? a0 : b0;
    a >>>= a0;
    b >>>= b0;
    while ((a & b) != 0) {
        // if 'a' and 'b' have almost same bit length
        if (Math.abs(Long.numberOfLeadingZeros(a)
                - Long.numberOfLeadingZeros(b)) < 2) {
            return binaryGCD(a, b) << t;
        }
        // use Euclidean algorithm
        if (a > b) {
            a %= b;
            a >>>= Long.numberOfTrailingZeros(a);
        } else {
            b %= a;
            b >>>= Long.numberOfTrailingZeros(b);
        }
    }
    return (a | b) << t;
}

The output is:

sum  = 1352820356
time = 19178926693

Well, that's interesting! :-) It seems that our "optimized" algorithm is actually slower than the un-optimized version of the binary algorithm. This occurs because the difference in bit-length is not such a big factor in case of the binary algorithm(i.e. 64 bits max in case of long). Sure enough, if we change the length-checking value from 2 to 4, we see that the running time is ~16 seconds.

Conclusion

Here, we have seen that:

Division-based algorithms should generally be avoided.
Premature optimization seldom pays off.
Shifts, additions and subtractions are the way to go in a binary environment.

Hence, the answers are:

Yes, but there can be more.
Many, many improvements... For starters, try reducing the absolute values of the remainders.
If the library supports integers which can have huge differences in bit-length. Otherwise, I'd use the Binary GCD algorithm for long.

1 See this article for more info on the song.


Calculate the sum of 2 answers This is the code for a calculator function. It takes the first and second number, combines them and asks for an input whether the calculation was correct. If the input is "Yes", a statement is printed. If the input is "No", a different statement is printed.

def calculator(number1,number2):
    answer = ['Yes', 'yes']
    print("The first number is %d." % number1,
          "The second number is %d." % number2)
    print("The sum is %d." % (number1 + number2))
    if input("Am I right? \nANSWER : ") in answer:
        print("Knew it!")
    else:
        print("Oh, well. Blame my programmer!")

Is there anything I can do to optimize the code and make it shorter and / or simpler?

Well, this program can be reduced a lot in pythonic way in terms of number for lines for sure!!

def calculator(number1,number2):
    try:
        print("The first number is {}\nThe second number is {}".format(number1, number2))
        print("The sum is {}".format(number1 + number2))
        print("Knew it!" if str(input("Am I right? \nANSWER : ")).lower() == "yes" else "Oh, well. Blame my programmer!")
    except Exception as e:
        print(e)

You have not taken care of handling input types, what if 2 different data types are passed?


Split URL into segments I try to split URLs into segments. These segments got two information, their name and their own URL.

The last segment in the segments list should not have the URL property because it's the current URL from the browser. There is no need for it.

I created this code which works really fine

const directoryPath = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md';

const segments = directoryPath.split('/');

const info = segments.map((segment, index) => {

  const item = {
    name: segment
  };

  if (index < segments.length - 1) {
    let route = '';

    for (let i = 0; i <= index; i++) {
      route += `/${segments[i]}`;
    }

    item.url = route;
  }

  return item;
});

console.log(info);

I don't know if this is the only way doing this, maybe there is a more optimized way. I don't know if I really need the for-loop because at the top of the code I split the URL into segments and after that I create smaller URLs out of it.

Is there a better / easier way?

Defect per line of code.

Code defects AKA bugs have a strong correlation to the number of lines of code (*1). Across all languages, coding styles, and domains there is a consistent defect rate. The industry average is about 20 per 1000 lines of code.

The metric is simple.

  "... a count of lines in the text of the program's source code excluding comment lines." (*2)

One of the simplest ways to reduce defects is to reduce the number of lines. This dose not mean putting everything in one line is a good idea, nor does it mean that sprawling out what would be a single line, to many lines, does not add to the line count.

Reducing your code

So with that in mind your code is a little low density, meaning that there are more lines than are needed to do what you want done.

Reducing your code while still using the same logic yields the following

const path = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md';

const info = path.split('/').map((name, index, dirs) => {
    const item = {name};
    if (index < dirs.length - 1) {
        item.url = '';
        for (let i = 0; i <= index; i++) {
            item.url += `/${dirs[i]}`;
        }
    }
    return item;
});

console.log(info);

Ignoring the top and bottom lines we now have 10 lines.

This is still a little long as the inner loop is just a Array.join but limited to the current index.

If we look for ways to reduce the size we can see that the need to exclude the URL from the file name is adding complexity. Unless there is a good reason to not have the URL we can just treat all items equally.

const path = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md';

const dirs = path.split('/');
const info = [];
while (dirs.length) {
    info.unshift({ url : "/" + dirs.join("/"), name : dirs.pop() });
}

console.log(info);

We are now down to 5 lines. And personally info can share the declaration with dirs

const path = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md';

const info = [], dirs = path.split('/');
while (dirs.length) {
    info.unshift({ url : "/" + dirs.join("/"), name : dirs.pop() });
}

console.log(info);

We have reduced the number of lines from 12 to 4, reducing the chance of defect by over half.

- (*1) Steve McConnell, Code Complete

- (*2) wiki, Source lines of code


Appending multiple file loads in Python I am fairly new to python, been coding in it for about a year or so. My company is switching from using SAS and a Netezza database for some of our data management. In order to access certain information, I have to load .dat.gz files now using Python3 in unix. In order to access similar information I used to access using basic SQL queries, I now have to import a list of files containing similar information and sort it using python. While this is not an issue for most of the data, there is an instance where I am running into performance issues. In order to access certain data I need, I must load over 300 files to python and try to append them together. This is an issue as the program takes a long time just to load the data. I am currently using pandas read_csv using usecols option to limit the data to the three columns I need. I first used df.append(df2), but it was slow. I then changed the dataframe to list, but that still did not seem to improve performance that much. All together the data contains over 50 Million rows. While I do not expect this program to run in seconds, I would like some help to improve performance where I can.

Just to note it is important to keep the integrity of the original .dat.gz files. The files I read in very in size from 2 rows to 50k rows or more. Any help would be greatly appreciated!

I have tried df.append(df2) and concatenating lists. I also tried appending the data to a new CSV in my working directory so the data would not all be stored while the program was running. That did not improve performance either.

import pandas as pd
import sys

#function to read in directories list from agrv[1]
def import_file_list(file_name):
    file_list = []
    with open(file_name) as f:
        for row in f:
            row = row.strip()
            file_list.append(row)
    return file_list

#main loop to read in all files and append the data
def import_data(list_of_files)
    for count, item in enumerate(list_of_files):
        exposure_data = pd.read_csv(item, sep='|', usecols=['A', 'B', 'C'], compression='gzip')
            if count == 0:
                    data_dict = exposure_data.to_dict('split')
                    values_list = list(data_dict['data'])
            else:
                    temp_dict = exposure_data.to_dict('split')
                    temp_list = list(temp_dict['data'])
                    values_list = values_list + temp_list

def main():
    import_data(import_file_list(sys.argv[1]))

if __name__ == '__main__':
    main()

Expected results would be to loop through all the files I need and combine them to one dataframe, list of lists, dictionary of all the data, or whatever object is most efficient with the three columns I need to do my calculations.

A suggestion that may help is to make generators of loops in import_file_list and import_data.
This is more memory efficient on the intermediate steps and defers building the dataframe until the end.

def import_file_list(file_name):
    with open(file_name) as f:
        for row in f:
            yield row.strip()

def import_data_to_dfs_iter(list_of_files)
    for item in list_of_files:
        yield pd.read_csv(
            item, sep='|', usecols=['A', 'B', 'C'], compression='gzip')

def main():
    dfs_iter = import_data_to_dfs(import_file_list(sys.argv[1]))
    df = pd.concat(dfs_iter)


Balanced parenthesis string checker Please review my balanced parenthesis string checker code below.

I would love to know how to improve my algorithm and if you have any other feedback.

// comments inline

function validateParenthesisString(inputString){

  if(inputString.length === 0){ // empty string is as good as a valid string
    console.log('String is valid.');
    return;
  }

  var element,
      parenthesisStack = [];

 // iterate through the string
  for(var index = 0, length = inputString.length; index < length; index++){

    element = inputString.charAt(index); // cache the character

    switch(element){
      case '(':
        parenthesisStack.push('('); // openeing bracket, so just push it
        break;

      case ')':
        if(parenthesisStack[parenthesisStack.length-1] === '('){ // check if corresponding counterpart pushed already
          parenthesisStack.pop(); // remove
        } else {
          parenthesisStack.push(')');
        }
        break;

      case '[':
        parenthesisStack.push('[');
        break;

      case ']':
        if(parenthesisStack[parenthesisStack.length-1] === '['){
          parenthesisStack.pop();
        } else {
          parenthesisStack.push(']');
        }
        break;

      case '{':
        parenthesisStack.push('{');
        break;

      case '}':
        if(parenthesisStack[parenthesisStack.length-1] === '{'){
          parenthesisStack.pop();
        } else {
          parenthesisStack.push('}');
        }
        break;

      default: // garbage character
        console.log('Invalid character encountered: ' + inputString.charAt(index));
        console.log('Please enter a valid string.');
        console.log('parenthesisStack:', parenthesisStack);
        console.log('***********************************');
        return;
    };

  }

  console.log('parenthesisStack:', parenthesisStack); // just for debugging

 // check parenthesisStack
  if(parenthesisStack.length === 0){
    console.log('String is valid!');
  } else {
    console.log('String is invalid!');
  }

  console.log('***********************************');

};

validateParenthesisString('test'); // invalid
validateParenthesisString('({)}[()]{'); // invalid
validateParenthesisString('[({})]{}'); // valid

Equivalent jsFiddle.

Major

Overall, it seems good.

I would not check for a base case as your code can deal with empty strings. It adds complexity and clutter.

Additionally,

if(parenthesisStack[parenthesisStack.length-1] === '['){
    parenthesisStack.pop();
} else {
    parenthesisStack.push(']');
}

is repeated for each closing option. This violates the Don't Repeat Yourself principle strongly. Encapsulating it into a function would be a great idea.

The same goes to the opening values, they are always handled the same way, why not check if the input is one of them and give that the same treatment. You would only simplify code and ease future maintenance this way. Be aware that I am not really a fan of switches anywhere, except for enumerated types. But my advice is unbiased, as far as I feel it. You'd be better without a switch here, in my opinion.

Minor

You have a // remove comment. It is quite pointless but it is too small to be an issue.

Lastly, it seems that the inline comment before the loop is misaligned. It would be nice to fix that.

Algorithm-wise

Your algorithm only fails on the end. So if I start with } and follow it by () repeated a million times it will take a reasonable amount of time to terminate even though the string cannot possibly be valid.


Longest 'balanced' substring I was given a task where given a string of '0's and '1's, return the longest substring where the count of '0's and '1's are equal. I was told also that the code should be O(n) and space O(1).

Below is what I came up with:

def max_equal_sub(s):
    '''
    I get the substrings by iterating over the starts and the ends for each start. For each
    of these, check if the counts are equal.
    '''
    return max((s[b:e] for b in range(len(s)) for e in range(b, len(s)) if s[b:e].count('1') == s[b:e].count('0')), key=len)

def main():
    TESTS = ('000', '1111', '001010101', '0001010101', '01010101', '000000111010101',
             '0000001110101011', '00000011101010111111111111111')

    for binstr in TESTS:
        print(max_equal_sub(binstr))

if __name__ == '__main__':
    main()

This code is all working fine, but I was told that this runs at O(n2), not O(n).

Following that I have some questions:

What makes it O(n2), and how can I make it so that it is O(n).
I was told that it needed to be 'space O(1)'. Does it run at that? What would make it run at space O(1) and why?
In the function max_equal_sub, would it be more efficient to put len(s) into a variable to avoid calling it again and again?
In the function max_equal_sub, I do s[b:e] three times. Surely there is a more efficient way?

(Optional, but favoured):

Are there any other things I could do to make it faster?
You could make it PEP8-compliant along the way if you wish.
Any other recommendations, perhaps regarding program logic.

Taking this from an algorithmic perspective, rather than a python perspective (I don't know python well enough to write something sensible in it).

Your algorithm can be expressed as the logic:

Create a virtual list of balanced strings
Starting from every position in the input string...

find every substring from that point.
check to see whether that substring is balanced
if it is balanced, add it to the virtual list.

from the virtual list, pull the longest substring.

Assuming the list remains virtual (yielded instead of real), then your algorithm is essentially space-complexity O(1). If the virtual list is really generated, then the space complexity is O(n2)

The time complexity is something more than O(n2). Probably closer to O(n3). For each b value ( O(n) ) you find all e values (also O(n) ), and then after doing that, you count each 1 and each 0, which is another two O(n) operations.

So, your code runs at O(n3) not O(n2)

Code Review is not really about helping you rewrite the code, but, there are some changes I can recommend...

Firstly, in complexity analysis, doing two O(n) operations one after the other, is still time-complexity of O(n)  ( time complexity of O(n) means that if you double the size of the input data, the time to execute will double.... and that is true if you have 1 O(n) operation or 100 O(n) operations one after the other).

Use this to your advantage.

I can't spend the time to do it all for you, and I am not even certain how to arrive at a fully O(n) solution, but the process you should probably follow is:

scan all the characters once, and count the 1's and count the 0's - O(n)
find out which one is more common (difference between overall counts) - O(1)

call the common character C
call the uncommon character U

scan all characters again, and calculate - O(n)

where the longest sequence of C's is, and how long it is

scan all the characters again, and count the number of U characters before, and after the longest C sequence. O(n)
Decide whether it is possible to contain the complete longest C sequence in the result.
..... from here it gets fuzzy, and I m not sure you can keep it to O(n), but you get the idea.


DRY-ing up some rspec I have an rspec spec test that has two sets of the same tests (from 24 Pull Requests). I thought I'd refactor the spec whilst I fixed something else.

Normally I just do a basic each ... do array to DRY things up:

require 'spec_helper'

describe ReminderMailer, type: :mailer do

  # Half the code yay! :D
  ['daily','weekly'].each do |time_format|
    describe time_format do
      let(:time_format) { time_format }
      let(:user) do
        mock_model(User, nickname:           'David',
                         email:              'david@example.com',
                         languages:          ['Ruby'],
                         skills:             [],
                         pull_requests:      double(:pull_request, year: []),
                         suggested_projects: [])
      end

      # How do I do this bit better?
      let(:mail) {
        if time_format == 'daily'
          ReminderMailer.daily(user)
        else
          ReminderMailer.weekly(user)
        end
      }

end

I couldn't figure out how to use the time_format with the send command, so I could do something like:

let(:mail) { ReminderMailer.send(timeformat(user) }

How could I do this?

The snippet you asked to re-write:

let(:mail) { ReminderMailer.send(time_format, user) }

Now, IMO using each to write specs is a bad practice. Why? Because rspec already provides idiomatic infrastructure to keep specs DRYed: use before and shared_examples. I'd write:

describe ReminderMailer, type: :mailer do
  let(:user) { ... }

  shared_examples "a reminder mailer" do |subject:, body:|
    it 'renders the subject' do
      expect(mail.subject).to eq(subject)
    end

    it 'renders the receiver email' do
      expect(mail.to).to eq([user.email])
    end

    it 'renders the sender email' do
      expect(mail['From'].to_s).to eq('24 Pull Requests <info@24pullrequests.com>')
    end

    it 'uses nickname' do
      expect(mail.body.encoded).to match(user.nickname)
    end

    it 'contains periodicity in body' do
      expect(mail.body.encoded).to match(body)
    end
  end

  describe 'daily' do
    let(:mail) { ReminderMailer.daily(user) }

    it_behaves_like "a reminder mailer",
      subject: '[24 Pull Requests] Daily Reminder',
      body: 'today'
  end

  describe 'weekly' do
    let(:mail) { ReminderMailer.weekly(user) }

    it_behaves_like "a reminder mailer",
      subject: '[24 Pull Requests] Weekly Reminder',
      body: 'week'
  end
end


Dealing with a command line argument (path) with spaces I created a key in Windows registry to have an option on every folder which allows me to simply right click, choose the right option and have a PDF made, either from JPGS, or PNGs, or TIFs.

My registry key is as follows:

  '"C:\strawberry\perl\bin\perl.exe" "C:\Users\michal\Documents\createpdf\790024.pl" "%1"'.

The code, thanks to suggestions and some major modifications by Matt Jacob works. Dealing with paths with spaces is done by:

my $folder = $ARGV[0];
$folder =~ s|\\|/|g;
$folder =~ s|/$||;

To read files I have came up with this: my @files = <"$folder/*">;

As I am a Perl newbie, I would like to see if this is correct. It works on my system.

The whole script is as follows:

#!/usr/bin/perl

use PDF::API2;
use strict;
use warnings;

my $folder = $ARGV[0];
$folder =~ s|\\|/|g;
$folder =~ s|/$||;

my $pdf_file = $folder . '.pdf';

die "Not a folder!\n" unless -d $folder;
die "There's already a pdf of that name!\n" if -f $pdf_file;

my $pdf = PDF::API2->new;

my %dispatch = (
    jpg => sub { $pdf->image_jpeg(@_) },
    png => sub { $pdf->image_png(@_) },
    tif => sub { $pdf->image_tiff(@_) },
);

my @files = <"$folder/*">;

for my $ext (sort(keys(%dispatch))) {
    my $count;

    for my $file (@files) {
        next unless $file =~ /\.$ext$/;

        my $image = $dispatch{$ext}->($file);
        my $page = $pdf->page;
        $page->mediabox(0, 0, $image->width, $image->height);
        $page->trimbox(0, 0, $image->width, $image->height);
        my $gfx = $page->gfx;
        $gfx->image($image, 0, 0);

        $count++;
    }

    last if $count;
}

if ($pdf->pages) {
    $pdf->saveas($pdf_file);
}

I think it's fine, although my preference would be readdir over glob. (I know someone made the exact opposite suggestion on your original post, and that's alright.) I can't really articulate why, but I've never liked glob very much---I guess it just seems too magical to me. Some people would argue "why write three lines of code when you can write one?" or "readdir isn't good because you have to filter out dots" or "readdir isn't good because you have to remember to prepend the directory". OK, so put a tiny wrapper around it in your standard library or use read_dir from File::Slurp. Anyway, moving on...

I did want to comment on your comment:

  Dealing with paths with spaces is done by:

my $folder = $ARGV[0];
$folder =~ s|\\|/|g;
$folder =~ s|/$||;

That code doesn't actually do anything related to handling paths with spaces. All it does is take the first command line argument to your script, replacing backslashes with forward slashes and stripping off the trailing slash. It's also unnecessary, since Perl on Windows can deal with either forward or backward slashes in paths. I would also assume that Windows is not giving you a directory path with a trailing slash on it, but I don't have access to a Windows box to confirm that.

Since you are on Windows, the shebang/hashbang line is redundant and can be removed:

#!/usr/bin/perl

Other than that, I think the rest of the code looks pretty good, but I guess it would be strange if I didn't think so, considering that I wrote some of it. When it comes to glob vs. readdir, it's mostly a matter of personal preference, although there's a good thread on Stack Overflow that examines the differences. Read it for yourself and see what you think.