Basic PHP template loader For a project, I need to write a basic template manager who will be responsible to load the requested HTML or PHP template when the user clicks on a menù link. It's like a page loader since that all the loaded templates are static HTML pages. I want to improve it if is it possible. Here is the code.

on the main index of the project I have a jQuery code like this:

<script>
$(document).ready(function(){

$('.contents').load('templates/home.php');

$(document).on('click','a',function(e){
    e.preventDefault();
    $('#nav-modal').modal('hide');
    var url = $(this).attr('href');
    if(url == 'booking/'){
        window.location.href= 'booking/';
    } else {

    $.ajax({
        type: 'GET',
        url: 'templates/TemplateController.php'+url,
        cache: false,
        dataType: 'html',
        success: function(response){
             $('.contents').empty()
             .html(response);
        }
        });
    }

    });

});
</script>

on the PHP 'controller' side I have this code who will only call a class to obtain the needed files. The URL passed from AJAX to the controller is something like this: ?tpl=atemplate

<?php
require_once 'Autoloader.php';

if(isset($_GET['tpl'])){
    $tpl = new TemplateLoader;
    echo $tpl->render($_GET['tpl']);
}

?>

And then in the simple class, I have only a switch control that will select what is the requested template to load on the index

<?php

class TemplateLoader{

    public function __construct(){
        #$this->path = $path;
    }

    public function render($tpl){

        switch($tpl){
            case 'home':
                echo file_get_contents('home.php');
                break;
            case 'about':
                echo file_get_contents('about.php');
                break;
            case 'services':
                echo file_get_contents('services.php');
                break;
            case 'contacts':
                echo file_get_contents('contacts.php');
                break;
            case 'prices':
                echo file_get_contents('prices.php');
                break;
        }
    }
}

?>

Technical mistakes

echo void

echo $tpl->render($_GET['tpl']);

This echo will display empty string (null), because you've already output a string with echo within method and have void return type (implicit return null). Make render() method return a string, because you want to defer output as much as you can. You won't be able to do anything with it after that (including headers management). Eventually you might want to use psr-7 response object with stream and plug yourself into some framework - class with method that returns string can be left untouched (Open-closed Principle) and wrapped with an object that changes string into stream.

closing tag

?>

Simply don't use it at the end of file. It causes hard to debug problems with undesired output - more here.

Algorithm

Render method works on hardcoded strings, which rarely is a good idea. You can make it more generic, as the logical pattern is quite distinct. Append path & file extension to given name and get file contents if it exists. The only thing you need to be careful about is path traversal attack - if you allow only lowercase letters it won't be possible. Also some error handling will be required.

OOP

Api

First of all your class api is perfect. Giving only template name, without implying that file system is involved makes it encapsulated abstraction and that's what OOP is about - don't be fooled by magic tricks and utilities. It is really a hard part, that often goes into area of so called "unknown unknowns" if you get it wrong, and it is really easy to do so.

Naming

You can write a book about that, so I'll point a few things:

Avoid naming your classes with names ending with -or or -er. It fits interfaces pretty well - they're focused on delivering something specific (from client's perspective) in unspecified way.
On the other hand avoid mentioning IO gateway names in data focused interfaces. It fits class names - they're interface implementations delivering something by talking to concrete IO mechanism.
Your TemplateLoader is concrete kind of loader, but you should be able to switch it with implementation that delivers strings from database or memory (when testing the class that uses it). You won't see the point when global namespace is the client using it, but when additional layers wrap it up it won't require messing with code beside the place where object composition happens.
render() method usually implies that some data processing is involved - you are simply returning template contents.

Refactored

Since this class is small (as it should) I've written refactored version based on the things I've pointed out and type hints for php 7:

interface TemplateLoader
{
    /**
     * @param string $templateName
     *
     * @throws InvalidArgumentException|TemplateNotFoundException
     *
     * @return string
     */
    public function contents(string $templateName): string;
}

class TemplateFiles implements TemplateLoader
{
    private $templatesDirectory;

    public function __construct(string $templatesDirectory = '') {
        $this->templatesDirectory = $templatesDirectory;
    }

    public function contents(string $templateName): string
    {
        $filename = $this->templateFilename($templateName);
        return file_get_contents($this->existingFile($filename));
    }

    private function templateFilename(string $templateName): string
    {
        $file = $this->validName($templateName) . '.php';
        return rtrim($this->templatesDirectory, '/') . '/' . $file;
    }

    private function validName(string $template): string
    {
        if (preg_match('/[^a-z]/', $template)) {
            throw new InvalidArgumentException('...');
        }

        return $template;
    }

    private function existingFile(string $filename): string
    {
        if (!file_exists($filename)) {
            throw new TemplateNotFoundException('...');
        }

        return $filename;
    }
}

You would have to implement two exceptions and put method call into try-catch block. There are two of them, because one may come from potentially malicious request (you might want to log these), while the other may be dev error or bypassed UI by user (it's not control flow exception which should be avoided).


Count occurrences of a given number in a list I have written a solution to find the occurrence of a common number in a n lists. I am just concerned if this is the best solution? Please suggest the best way to do it.

import java.util.Arrays;
import java.util.Collections;
import java.util.List;

public class CommonNumberFinder
{

    final static List<Number> list1 = Arrays.asList(new Number[] { 1, 2, 3, 4, 5, 6, 7, 8, 9 });
    final static List<Number> list2 = Arrays
            .asList(new Number[] { 3, 5, 1, 6, 2, 6, 3, 3, 6, 1, 5, 7, 2, 6, 2, 5, 2, 6, 8, 2 });

    public static void main(String[] args)
    {
        findCountOfNumber(Arrays.asList(new List[] { list1, list2 }), 6);
    }

    private static void findCountOfNumber(final List<List<Number>> lists, final Number num)
    {
        int count = 0;
        for (List<Number> list : lists)
        {
            if (!list.contains(num))
            {
                System.err.printf("Number %d is not common", num);
                return;
            }
        }
        for (List<Number> list : lists)
        {
            for (Number number : list)
            {
                if (number == num)
                    count++;
            }
            System.out.println("List have-" + count);
            count = 0;
        }

    }
}

There are a decent number of reservations I take with this code.    Most IDEs would pick this up, but there are some nuances which you may not have fully grasped.

First, your import of java.util.Collections is superfluous and isn't used anywhere.

Next, your bound on List<Number> seems a bit...overly generic.  Since these lists support Number, they could support BigDecimal and BigInteger, or more conventionally, Float and Double - which poses problems later on.

There's no reason to declare this:

Arrays.asList(new List[]{list1, list2});

Arrays.asList takes a variadic argument, so you can simply list the arguments after it without the need for an explicit array.

Arrays.asList(list1, list2);

You should look to do this for your other lists as well.

The biggest issue as far as a broken implementation that I can see is this:

if (number == num)

This is wrong for a few reasons:

You're using == on objects.
The objects you're performing this == on are not guaranteed to unbox.
Strict equivalence in the face of floating-point values is not entirely guaranteed.

If you wanted to fix this, you'd want to fix the bound on what your method accepts to incorporate Comparable...

private static <T extends Number & Comparable<T>> void findCountOfNumber(final List<List<T>> lists, final T num)

...and simply compare the values together.

for (T number : list) {
    if (number.compareTo(num) == 0) {
        count++;
    }
}

You'll also need to ensure that the types you use for your array and the types that you use for num are homogeneous, since it really wouldn't make sense to try to find a floating-point value in a list of integers.  This is easily accomplished with the new type bounds by simply changing your lists to List<Integer>.

Lastly, let's talk about the output.  This is incredibly cryptic:

List have-1
List have-5

What you're saying is, the first list has one occurrence of the number 6, and the second list has five occurrences of the number 6.  But, what you're searching for and which list you're talking about aren't clear.

You should look to express that concisely.  Perhaps instead of using an enhanced-for you should look to use a conventional for, which states clearly what list has what occurrences of what number.


jQuery menu: hide and show on click I am new to jQuery and currently practicing. I wrote a little piece of code to show and hide a navigation on click and change the text of the button when the navigation is opened and closed.

I am fresh in this area, therefore I would very much value getting some feedback about the code.  For example, are there any ways to do it better, with less code or any other area of the matter? I would like a code review to learn and practice (the second if is especially bothering me).

// Function Hide nav
function hideNav() {
$('nav').hide();
}

// Hide Nav
hideNav();

// Function Show Nav

function showNav() {
$('nav').show();
}

// Clickevent on Button
$('button').click(function() {
if($('nav').is(':hidden')){
// Show Nav on Clickevent
showNav();
// Change Text when Nav is showing
$(this).text('Close me');

}
else
hideNav();
// Change Text back when Nav is hiding
if($('nav').is(':hidden')){
$('button').text('Open me');
}
});

Some general style points:

Indentation, it's important. Please indent one level for each set of braces moving forward.
Frivolous comments. You have one comment for each function. These comments simply repeat what the names of the functions already tell us. They're noise. Remove them. I find it's a good practice to write pseudocode comments and then "fill in the blanks", but they shouldn't stay once you've implemented it. Comments should always say why, rarely should they say what.

Now we can see what's really going on a little better.

function hideNav() {
    $('nav').hide();
}

hideNav();

function showNav() {
    $('nav').show();
}

$('button').click(function() {
    if($('nav').is(':hidden')){
        showNav();
        $(this).text('Close me');
    }
    else
        hideNav();

    if($('nav').is(':hidden')){
        $('button').text('Open me');
    }
});

Personally, I don't see a point to the showNav() and hideNav() functions. It seems they don't do much except save a few keystrokes, but you've already written them and they're not actually hurting anything. I guess they're ok.

This else statement really needs some braces.

    if($('nav').is(':hidden')){
        showNav();
        $(this).text('Close me');
    }
    else
        hideNav();

    if($('nav').is(':hidden')){
        $('button').text('Open me');
    }

It's really not clear when the second if($('nav').is(':hidden') is supposed to execute. I think it should look like this.

    if($('nav').is(':hidden')){
        showNav();
        $(this).text('Close me');
    } else {
        hideNav();
    }

    if($('nav').is(':hidden')){
        $.('button').text('Open me');
    }

Which brings to mind some code duplication. I would create a isNavHidden() function to simplify all this a bit.

function isNavHidden() {
    return $('nav').is(':hidden');
}

Then you could write the above snippet like this.

    if(isNavHidden()){
        showNav();
        $(this).text('Close me');
    } else {
        hideNav();
    }

    if(isNavHidden){
        $.('button').text('Open me');
    }


Naive implementation of sort inplace I had the following assignment for the Python 101 course I'm taking in my university.

'''
Modify the naive implementation (see below) of a sorting algorithm so that
the list to be sorted is modified *in place*.

Here is the idea:

1. Iterate over all indices i = 0, 1, 2, ... of the input list

2. Find the index j of the smallest element in the rest of the list (from i on)

3. Replace the elements at indices i and j

If we want to sort the list [5, 3, 1, 4, 8, 2, 7, 6], for instance, a run of
the algorithm would be:

i=0, j=2, list=[5, 3, 1, 4, 8, 2, 7, 6]
i=1, j=5, list=[1, 3, 5, 4, 8, 2, 7, 6]
i=2, j=5, list=[1, 2, 5, 4, 8, 3, 7, 6]
i=3, j=3, list=[1, 2, 3, 4, 8, 5, 7, 6]
i=4, j=5, list=[1, 2, 3, 4, 8, 5, 7, 6]
i=5, j=7, list=[1, 2, 3, 4, 5, 8, 7, 6]
i=6, j=6, list=[1, 2, 3, 4, 5, 6, 7, 8]
'''

In principle, the only two funcions I have to modify are "index_of_smallest_element" and "naive_sort_inplace".

def index_of_smallest_element(some_list):
    '''Returns the index of the smallest element in some_list'''
    guess=some_list[0]
    for element in some_list:
        if element < guess:
            guess=element
    return some_list.index(guess)

def naive_sort(some_list):
    sorted_list = []
    while len(some_list) > 0:
        # remove smallest element from some_list
        smallest_element = some_list.pop(index_of_smallest_element(some_list))
        # ... and append it to sorted_list
        sorted_list.append(smallest_element)
    return sorted_list

def naive_sort_inplace(some_list):
    def exchange(some_list,x,y):
        some_list[x],some_list[y]=some_list[y],some_list[x]

    for i in range(0,len(some_list)):
        remaining_list=some_list[i:(len(some_list)+1)]
        i_rest=index_of_smallest_element(remaining_list)
        j=i+i_rest
        print('i=',i,'j=',j,'list=',some_list)
        exchange(some_list,i,j)
    return some_list

if __name__ == '__main__':
    my_list = [5, 3, 1, 4, 8, 2, 7, 6]
    naive_sort_inplace(my_list)
    print('Test: ', my_list == [1, 2, 3, 4, 5, 6, 7, 8])

Although the code is fully functional, I'm wondering is there would be a better way to do it, especially the "naive_sort_inplace" function.

Problems

First of all, your current implementation is not truly in-place, since the slice operation creates a new list of that size, which breaches the required max constant extra space usage.

In addition you perform double work in the index_of_smallest_element when you first find the value you want, and then have to run through the entire list again to find the index (the index call has linear time cost)

Advise

When working in-place you usually have to work with indexes or pointers, and this is also such a case. Most of the work can be put into 3 categories, index work, swapping work, and control flow, if you are doing work outside of those you should think through an extra time on whether you are off in a problematic direction.

This means that you should manually keep track of relevant indexes, and when calling functions the way to work on a subset is to pass the indexes describing this to it.

Solution

there are 2 main changes to be done, both connected to index_of_smallest_element. We first change thats implementation to the following:

def index_of_smallest_element(some_list, start=0):
    smallest = some_list[start]
    for i in range(start+1, len(some_list)):
       value = some_list[I]
       if value < smallest:
           smallest = value
           start = i
    return start

With this it becomes easy to write the naive (and really bad) sorting in-place:

def naive_sort_inplace(some_list, out=None):
    def exchange(some_list,x,y):
        some_list[x],some_list[y]=some_list[y],some_list[x]

    for i in range(0,len(some_list)):
        j = index_of_smallest_element(remaining_list, i)
        if out is not None:
            out('i=',i,'j=',j,'list=',some_list)
        exchange(some_list,i,j)
    return some_list

I am not quite sure why you want to do all the printing, but I assume it is for testing or proving correctness. For reuse I have changed it so it by default does not print, but if you pass in print (or some other output function such as open(filename,'w').write) then it write those log updates to that output function.


Handling singularities in squircle parametric equations In order to plot a squircle using its parametric equations:

The equations can be found in page 9 of this
this article:

Fong, Chamberlain. "Squircular Calculations." arXiv preprint arXiv:1604.02174 (2016).

Unfortunately the following singularities are not handled by those equations:

For t = 0, t = pi/2, t = pi, t = 3pi/2, and t=2*pi

Here is the code to plot these equations in Python
"""Plot squircle shape using its parametric equations"""
import numpy as np
from numpy import pi, sqrt, cos, sin, sign
import matplotlib.pyplot as plt

# FG stands for Fernandez-Guasti who introduced the algebric equations.
def FG_param(r, s, t):
    x = np.ones_like(t)
    y = np.ones_like(t)
    for i, _t in enumerate(t):
        if np.isclose(_t, 0.0):
            x[i] = r
            y[i] = 0.0
        elif np.isclose(_t, pi/2.0):
            x[i] = 0.0
            y[i] = r
        elif np.isclose(_t, pi):
            x[i] = -r
            y[i] = 0.0
        elif np.isclose(_t, 3*pi/2.0):
            x[i] = 0.0
            y[i] = -r
        elif np.isclose(_t, 2*pi):
            x[i] = r
            y[i] = 0.0
        else:
            x[i] = r*sign(cos(t[i]))/(s*sqrt(2)*np.abs(sin(t[i])))*sqrt(1 - sqrt(1-s**2*sin(2*t[i])**2))
            y[i] = r*sign(sin(t[i]))/(s*sqrt(2)*np.abs(cos(t[i])))*sqrt(1 - sqrt(1-s**2*sin(2*t[i])**2))
    return x, y

s = 0.7
r = 1.0
NTHETA = 90
t = np.linspace(0, 2*pi, NTHETA)

x, y = FG_param(r, s, t)

plt.gca().set_aspect('equal')
plt.scatter(x, y, s=5)
plt.show()

The function FG_param looks very clumsy. I appreciate any help to make it more compact and neat.
Thank you

Here's a different solution.  The singularities occur when sin(_t) or cos(_t) is zero.  So check for those conditions.

def FG_param(r, s, t):
    x = np.ones_like(t)
    y = np.ones_like(t)

    for i, _t in enumerate(t):
        sin_t = sin(_t)
        cos_t = cos(_t)

        if np.isclose(sin_t, 0.0):
            x[i] = -r if np.isclose(_t, pi) else r
            y[i] = 0.0

        elif np.isclose(cos_t, 0.0):
            x[i] = 0.0
            y[i] = r if _t < pi else -r

        else:
            radical = sqrt(1 - sqrt(1 - s**2 * sin(2*_t)**2))

            x[i] = r*sign(cos_t) / (s*sqrt(2)*np.abs(sin_t)) * radical
            y[i] = r*sign(sin_t) / (s*sqrt(2)*np.abs(cos_t)) * radical

    return x, y


PHP find cheapest permutation I have multiple products that all have sizes and I need to find the cheapest configuration that meets the minimum required size.

For example, John needs a minimum of 10 litres of storage - it can be more, but not less.

There are 2L, 3L, 5L, 8L and 10L options (but this can change).

As an example, it might be cheaper to get:

1x10L container OR
2x5L containers OR
1x2L, 1x3L and 1x5L OR
4x3L (this one is over 10 L, but it is still possible that it will be cheaper)

So far I've tried looping over and over up to 4 times (because typically the maximum requirement will be 40 L), but in some cases I am running out of memory, and it doesn't seem like the most efficient way of doing it.

// Size is in mL

$available_containers = [
[
  'id' => 22700,
  'price' => 1190,
  'size' => 2000,
],
[
  'id' => 22701,
  'price' => 1245,
  'size' => 3000,
],
[
  'id' => 22702,
  'price' => 1415,
  'size' => 4000,
],
[
  'id' => 22715,
  'price' => 12300,
  'size' => 5000,
],
[
  'id' => 22706,
  'price' => 1740,
  'size' => 5000,
],
[
  'id' => 22703,
  'price' => 1510,
  'size' => 5000,
],
[
  'id' => 22707,
  'price' => 1790,
  'size' => 6000,
],
[
  'id' => 22704,
  'price' => 1770,
  'size' => 6000,
],
[
  'id' => 22708,
  'price' => 2215,
  'size' => 7000,
],
[
  'id' => 22705,
  'price' => 2195,
  'size' => 8200,
],
[
  'id' => 22709,
  'price' => 2660,
  'size' => 8200,
],
[
  'id' => 22710,
  'price' => 2799,
  'size' => 10000,
],
[
  'id' => 22711,
  'price' => 2910,
  'size' => 12500,
],
[
  'id' => 22712,
  'price' => 3260,
  'size' => 15000,
],
[
  'id' => 22713,
  'price' => 4130,
  'size' => 20000,
],
[
  'id' => 22714,
  'price' => 3770,
  'size' => 27000,
]
];

$required_size = 8; // Can change.
$container_install = 5;

foreach ( $available_containers as $v ){
  foreach ( $available_containers as $v2 ){
    foreach ($available_containers as $v3 ) {
      foreach ( $available_containers as $v4 ){

        $all_configs = [
          [
            'size' => $v['size'],
            'configuration' => [ $v['size'] ],
            'price' => $v['price'],
          ],
          [
            'size' => $v['size'] + $v2['size'],
            'configuration' => [ $v['size'], $v2['size'] ],
            'price' => $v['price'] + $v2['price'] + $container_install,
          ],
          [
            'size' => $v['size'] + $v2['size'] + $v3['size'],
            'configuration' => [ $v['size'], $v2['size'], $v3['size'] ],
            'price' => $v['price'] + $v2['price'] + $v3['price'] + $container_install + $container_install,
          ],
          [
            'size' => $v['size'] + $v2['size'] + $v3['size'] + $v4['size'],
            'configuration' => [ $v['size'], $v2['size'], $v3['size'], $v4['size'] ],
            'price' => $v['price'] + $v2['price'] + $v3['price'] + $v4['price'] + $container_install + $container_install + $container_install,
          ],
        ];

        foreach ( $all_configs as $c ){
          if ( $c['size'] >= $required_size ){
            $configuration[] = array(
              'configuration' => $c['configuration'],
              'size' => $c['size'],
              'price' => $c['price'],
            );
          }
        }
      }
    }
  }
}

// Sort by price.
$sorted_configs = array_sort($configuration, 'price', SORT_ASC); // This function simply sorts all permutations by price
```

This problem is an instance of Integer Linear Programming. ILP is NP-hard, so an algorithm to find the optimal solution will not be much faster than brute-force. However, a common technique to find an approximate optimum is to solve it as a Linear Programming problem without the integer restrictions, then round the results up or down as necessary. Fortunately, many libraries exist to solve non-integer LP quite efficiently.


SQL server database access helper class using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Configuration;
using System.Data.SqlClient;
using System.Data;

namespace ExamManagement.DLL
{
    public class SQLDataAccessHelper
    {
        string connectionString=string.Empty;

        public SQLDataAccessHelper()
        {
            try
            {
                connectionString = ConfigurationManager.ConnectionStrings["DefaultConnectionString"].ConnectionString;

            }
            catch (Exception)
            {
                //todo error handling  mechanism
                throw;
            }
        }

        public void ExecuteNonQuery(string commandText,CommandType commandType, params SqlParameter[] commandParameters)
        {
            using(var connection= new SqlConnection(connectionString))
            using (var command = new SqlCommand(commandText, connection))
            {
                command.CommandType = commandType;
                command.Parameters.AddRange(commandParameters);
                connection.Open();
                command.ExecuteNonQuery();
            }
        }

        public DataSet ExecuteQuery(string commandText,CommandType commandType,params SqlParameter[] parameters)
        {
            using(var connection=new SqlConnection(connectionString))
            using(var command= new SqlCommand(commandText,connection))
            {
                DataSet ds=new DataSet();
                command.CommandType=commandType;
                command.Parameters.AddRange(parameters);
                SqlDataAdapter da = new SqlDataAdapter(command);
                da.Fill(ds);
                connection.Close();
                return ds;
            }
        }
    }
}

This is helper class I have written to retrieve data and execute CRUD operation on sql server database either using queries or stored procedures.

Currently it is used from DAL classes as follows :

I create instance of it
I call method "ExecuteQuery" and it returns DataSet

Details as follows:

namespace ExamManagement.DLL.Implementations
{
    public class StudentDAO : IStudentDAO
    {
        SQLDataAccessHelper sqlHelper;
        public StudentDAO()
        {
            sqlHelper = new SQLDataAccessHelper();
        }

        public List<Entities.Student> AllStudents()
        {
            List<Entities.Student> studentsList = new List<Entities.Student>();

            SqlParameter[] parameterList = { new SqlParameter("@Id",DBNull.Value),
                                               new SqlParameter("@name",DBNull.Value),
                                               new SqlParameter("@address",DBNull.Value)};

            var table = sqlHelper.ExecuteQuery("GetStudents", System.Data.CommandType.StoredProcedure,parameterList).Tables[0];
            studentsList = table.AsEnumerable().Select(row => new Entities.Student { Id = row.Field<int>("Id"), Name = row.Field<string>("Name"), Address = row.Field<string>("Address") }).ToList();
            return studentsList;
        }

I have following questions with regards to it

Helper class should be static class or the methods in it should be static methods?
Connection string is not supplied by class user, instead its is auto calculated from Web.config using ConfigurationManager. Is this approach ok?
I'm creating connection in "Using" statement, so can I ignore calling connection.Close() or its required even then ?
How can I improve it.
Possible to write unit tests for this class?

Everything looks good over here, however, I would like to mention some points which cover some of your questions.

I can see your methods are dependent on the global variable connectionString.

Global variables creates confusion when code grows.
It harder to maintain.
The code which uses global variables gets tightly coupled.

I suggest you to create a class to handle all database connections. This will make your code more flexible:

It allows to use same connection in other classes.
It allows to use different database connections.

public class ConnectionManager
{
    public static SqlConnection GetSqlConnection()
    {
        string connectionString = ConfigurationManager.ConnectionStrings["DefaultConnectionString"].ConnectionString;
        var connection = new SqlConnection(connectionString);
        connection.Open();
        return connection;
    }

    public static OleDbConnection GetOleDbConnection()
    {
        string connectionString = ConfigurationManager.ConnectionStrings["OleDbConnectionString"].ConnectionString;
        var connection = new OleDbConnection(connectionString);
        connection.Open();
        return connection;
    }
}

Above method returns open connection so you don't have to open it.
It force to implement using, try-catch-finally or IDisposable to make sure you always close connection which is a good thing.
No need to have constructor for each class which use database.

It is not a good practise to ignore return value. Your method ExecuteNonQuery has return type void which ignores the return value of command.ExecuteNonQuery();
You should use {} to make code more readable.

Before:

public void ExecuteNonQuery(string commandText, CommandType commandType, params SqlParameter[] commandParameters)
{
    using (var connection = new SqlConnection(connectionString))
    using (var command = new SqlCommand(commandText, connection))
    {
        command.CommandType = commandType;
        command.Parameters.AddRange(commandParameters);
        connection.Open();
        command.ExecuteNonQuery();
    }
}

After:

public int ExecuteNonQuery(string commandText, CommandType commandType, params SqlParameter[] commandParameters)
{
    int affectedRows = 0;
    using (var connection = ConnectionManager.GetSqlConnection())
    {
        using (var command = new SqlCommand(commandText, connection))
        {
            command.CommandType = commandType;
            command.Parameters.AddRange(commandParameters);
            affectedRows = command.ExecuteNonQuery();
        }
    }
    return affectedRows;
}

Hope this helps.


Apply a sepia filter to an image I'm writing a simple function that applies a Sepia filter to an image. My function works, but I'm a beginner gopher. So I would like to know if there is a way to improve the code. I usually code in Javascript, so I have troubles with a strong typed language such as go.

// Sepia apply a sepia filter to Image.
func Sepia(pic image.Image) (output *image.RGBA) {

    bounds := pic.Bounds()
    w, h := bounds.Max.X, bounds.Max.Y
    rect := image.Rect(0, 0, w, h)
    output = image.NewRGBA(rect)

    for x := bounds.Min.X; x <= w; x++ {
        for y := bounds.Min.Y; y <= h; y++ {
            r, g, b, a := pic.At(x, y).RGBA()
// Part to improve from HERE
            r >>= 8
            g >>= 8
            b >>= 8
            a >>= 8

            r = uint32(math.Min(.393*float64(r)+.769*float64(g)+.189*float64(b), 255))
            g = uint32(math.Min(.349*float64(r)+.686*float64(g)+.168*float64(b), 255))
            b = uint32(math.Min(.272*float64(r)+.534*float64(g)+.131*float64(b), 255))

            newColor := color.RGBA{
                uint8(r),
                uint8(g),
                uint8(b),
                uint8(a),
            }
// to HERE
            output.Set(x, y, newColor)
        }
    }
    return
}

I would like to know if there is a way to improve the code.

// Part to improve from HERE
r >>= 8
g >>= 8
b >>= 8
a >>= 8

r = uint32(math.Min(.393*float64(r)+.769*float64(g)+.189*float64(b), 255))
g = uint32(math.Min(.349*float64(r)+.686*float64(g)+.168*float64(b), 255))
b = uint32(math.Min(.272*float64(r)+.534*float64(g)+.131*float64(b), 255))

newColor := color.RGBA{
  uint8(r),
  uint8(g),
  uint8(b),
  uint8(a),
}
// to HERE

In Go, readability is paramount. I would write:

// Part to improve from HERE

rr := float64(r >> 8)
gg := float64(g >> 8)
bb := float64(b >> 8)
aa := a >> 8

newColor := color.RGBA{
    R: uint8(math.Min(.393*rr+.769*gg+.189*bb, 255)),
    G: uint8(math.Min(.349*rr+.686*gg+.168*bb, 255)),
    B: uint8(math.Min(.272*rr+.534*gg+.131*bb, 255)),
    A: uint8(aa),
}

// to HERE


Re-arranging a list into columns In short, what this script aims to do is take the following data:

1
Alpha
Beta
Delta
2
Beta
Echo
Foxtrot
Kilo
3
Alpha
Kilo
4
Beta
Echo
Kilo
Zulu

And re-arrange like this:

1       2        3      4
Alpha   Beta     Alpha  Beta
Beta    Echo     Kilo   Echo
Delta   Foxtrot         Kilo
        Kilo            Zulu

The code I have is as follows:

      Private Sub PrepareLibrary()

Application.ScreenUpdating = False

Dim Rng As Range
Dim i As Long
Dim n As Range
Dim x As Long
Dim Library As Worksheet
Dim LastRow As Long
Set Library = Sheets("Library")
i = 1
x = 0
LastRow = Library.Range("A1").SpecialCells(xlCellTypeLastCell).row

Do While i <= LastRow
Set Rng = Library.Range("A" & i)
Set n = Library.Range("A1")

If IsNumeric(Rng) = True Then
x = x + 1
Rng.Copy
n.Offset(0, x).Insert Shift:=xlDown
i = i + 1
ElseIf IsNumeric(Rng) = False Then
Rng.Copy
n.Offset(1, x).Insert Shift:=xlDown
i = i + 1
ElseIf Application.WorksheetFunction.CountA(Rng) = 0 Then
i = i + 1

End If
Loop
End Sub

Essentially, this runs through Column A one at time, copying each cell across individually, pasting into a new row whenever it comes across a number. It works well on small databases, but when expanding out to over 20,000 rows of data, I get a "not enough resources" message. Willing to attempt an entirely different approach if it seems possible. I'm new to VBA, so that may explain why this code is quite inefficient.

Your naming tells me nothing and doesn't follow Standard VBA naming conventions.

Dim i - this is a counter? why not rowNumber
Dim n - this is also a counter? why not labelNumber
Dim x - is this a counter? why not nextRowNumber
Dim Library should be library
Dim LastRow should be lastRow

With library in fact Worksheets have a CodeName property - View Properties window (F4) and the (Name) field can be used as the worksheet name. This way you can avoid Sheets("mySheet") and instead just use mySheet.

A good way to speed up your macro is to use Application.Screenupdating = False and Application.Calculation = xlManual and Application.EnableEvents = False. Just be sure to return them to True and xlAutomatic and True before exiting the sub.

Additionally, you're doing everything on the sheet, which is slow by nature. Even just reading into an array should be quicker -

Option Explicit

Private Sub PrepareLibrary()
    Application.Screenupdating = False
    Dim lastRow As Long
    Dim results As Variant
    Dim resultsIndex As Long
    Dim currentRow As Long
    Dim currentColumn As Long
    currentColumn = 0
    lastRow = Library.Cells(Rows.Count, "A").End(xlUp).Row
    ReDim results(1 To lastRow)
    results = Library.Range("A1:A" & lastRow).Value

    For resultsIndex = 1 To lastRow
        If IsNumeric(results(resultsIndex, 1)) Then
            currentColumn = currentColumn + 1
            Sheets("Sheet2").Cells(1, currentColumn) = results(resultsIndex, 1)
            currentRow = 2
        Else:
        Sheet2.Cells(currentRow, currentColumn) = results(resultsIndex, 1)
        currentRow = currentRow + 1
        End If
     Next
     Application.Screenupdating = True
End Sub

That's not a very good example of using the array to the maximum benefit, but it's something.


String pattern riddle I've tried this problem on Codeforces.  Given a set of string patterns, all of which are the same length, and which consist of literal lowercase ASCII characters and ? wildcards, find a pattern that satisfies them all.

I want to optimize and simplify it further and make it a more elegant solution, if possible.

import sys

def main():
    n = int(sys.stdin.readline())
    t = sys.stdin.readlines()
    l = len(t[0])
    result = ''
    if n==1:
        result = t[0].replace('?', 'x')
    else:
        for y in range(0,l-1):
            let = t[0][y]
            for x in range(1,n):
                if let == '?' and t[x][y]!= '?':
                    let = t[x][y]
                if t[x][y] != let and t[x][y] != '?':
                    result += '?'
                    break
                elif x == n-1:
                    if let == '?':
                        result += 'x'
                    else:
                        result += let
    print result

main()

Use more meaningful variable names, for example:

lines instead of t
length instead of l
Since y is a character position, I would rename to pos
let to letter, because it's not much longer and a bit more readable (since let is keyword in some other programming languages, I find it confusing to read)

Since lines[0] is kind of special, and you make many references to it, it would be good to give it a name:

first = lines[0]

Your input lines end with a trailing newline character, but in the main logic of pattern matching this is just noise, not an important detail. I recommend this:

first = lines[0].strip()

This will get rid of the trailing newline character, and in the main logic you don't have to worry about checking letters until first[-1], you can consider the letters in first, which is simpler.

If you make changes to the script, you have to repeat all the tests to see if it's still working. That's very troublesome. To make that easier, add some unit tests, and refactor the main method to make it more testable, for example:

import unittest
from test.test_support import run_unittest

def main():
    n = int(sys.stdin.readline())
    lines = sys.stdin.readlines()[0:n]
    print find_common_pattern(lines)

def find_common_pattern(lines):
    # the rest of the code from the old version of `main`

class FindPatterns(unittest.TestCase):
    def test_given_examples(self):
        self.assertEqual('xab', find_common_pattern(['?ab\n', '??b\n']))
        self.assertEqual('?', find_common_pattern(['a\n', 'b\n']))
        self.assertEqual('xaxb', find_common_pattern(['?a?b\n']))

run_unittest(FindPatterns)

After this, it will be easy to follow my suggestions below and verify that the script is still working correctly, passing all test cases. (Feel free to add more!)

Instead of range(0, length-1), you can write simply range(length-1).

if let == '?' and t[x][y]!= '?':
    let = t[x][y]
if t[x][y] != let and t[x][y] != '?':
    result += '?'

If the first if was true, the second will not be true, and it's a waste to evaluate it again. You could make the second if an elif instead.

The last elif in your for loop is a bit dirty: it's for checking if you're on the last line. The condition will be evaluated for every line where the current position is ?, which is a bit wasteful. It would be better to refactor that loop to perform that check only once at the end, for example:

for pos in range(length):
    letter = first[pos]
    newletter = letter
    for x in range(1, len(lines)):
        if letter == '?' and lines[x][pos] != '?':
            letter = lines[x][pos]
        elif lines[x][pos] != letter and lines[x][pos] != '?':
            newletter = '?'
            break
    if letter == '?':
        result += 'x'
    else:
        result += newletter

After this refactoring, you don't need the value of x inside the loop anymore, so you could rewrite more intuitively as:

for line in lines[1:]:
    if letter == '?' and line[pos] != '?':
        letter = line[pos]
    elif line[pos] != letter and line[pos] != '?':
        newletter = '?'
        break

Since the characters ? and x appear several times in the code, I would treat them as constants and name them for example:

ANYCHAR = '?'
SAMPLECHAR = 'x'

Instead of running main, the common way is like this:

if __name__ == '__main__':
    main()

This way, your script will still run as before when you do python script.py, and at the same time it will be possible to import it from another python script as a library without automatically running it. It's a good practice to get used to, even if you don't intend to make this a library.

Putting it all together, and some other minor improvements, this would be more Pythonic, and slightly more readable and efficient:

ANYCHAR = '?'
SAMPLECHAR = 'x'

def find_common_pattern(lines):
    first = lines[0].strip()
    lines = lines[1:]
    if not lines:
        return first.replace(ANYCHAR, SAMPLECHAR)
    result = ''
    for pos in range(len(first)):
        letter = first[pos]
        newletter = letter
        for line in lines:
            if line[pos] != ANYCHAR:
                if letter == ANYCHAR:
                    letter = line[pos]
                elif line[pos] != letter:
                    newletter = ANYCHAR
                    break
        if letter == ANYCHAR:
            result += SAMPLECHAR
        else:
            result += newletter
    return result


TopCoder problem "Substitute" - SRM 160 (Division II Level One) I have solved the problem "Substitute" at Topcoder.

  A simple, easy to remember system for encoding integer amounts can be very useful. For example, dealers at flea markets put the information about an item on a card that they let potential buyers see. They find it advantageous to encode the amount they originally paid for the item on the card.
  A good system is to use a substitution code, in which each digit is encoded by a letter. An easy to remember 10-letter word or phrase, the key, is chosen. Every '1' in the value is replaced by the first letter of the key, every '2' is replaced by the second letter of the key, and so on. Every '0' is replaced by the last letter of the key. Letters that do not appear in the key can be inserted anywhere without affecting the value represented by the code.. This helps to make the resulting code much harder to break (without knowing the key).
  Create a class Substitute that contains the method getValue that is given the Strings key and code as input and that returns the decoded value.

I started studying Big O notation and I think the complexity of my program is \$O(n^2)\$. How can I make my program more efficient and clean? Can I make it \$O(n)\$?

     public class Substitute {

    public int getValue(String key, String code) {
        String s = "";
        for (int i = 0; i < code.length(); i++) {
            for (int j = 0; j < key.length(); j++) {
                int x = key.indexOf(code.charAt(i)) + 1;
                if (code.charAt(i) == key.charAt(j)) {
                    if (x >= 10) {
                        x = 0;
                    }
                    s = s + x;
                }
            }
        }
        return Integer.parseInt(s);
    }
}

Analysis

You've picked an interesting problem about which to ask about Big-O complexity.  The problem is, you're being sloppy about what you call "n".  This function has two inputs: a key of length 10 and a code of variable length.  Let's say that m is the length of key (which is 10) and n is the length of code.

The outer i loop executes n times.  For each of those, outer loops, the inner j loop executes m times.  For each inner loop, key.indexOf() — an O(m) operation — is called; then we might do s = s + x — an O(n) operation.  Finally, Integer.parseInt(s) is an operation that I would expect to be O(n).

The complexity, therefore, is O(n ∙ m (m + n) + n).  If we consider m to be constant, that simplifies to O(n2).

However, the "silliest" part of the code is s = s + x.  That O(n) operation could be O(1) if you had used a StringBuilder instead of repeated string concatenation.  With a StringBuilder, your function would become O(n).

Perspective

It's important to keep a sense of perspective, though.  Big-O complexity is useful for getting a feel for how well an algorithm scales to handle large input.  For example, it can help you estimate whether an algorithm that analyzes a million DNA bases will finish in seconds or hours.

On the other hand, Big-O analysis is nearly useless for predicting the performance of "small" problems like this one.  The challenge states that n is at most 9 — and in any case, a result larger than 9 digits would overflow an int.  It is very possible that an algorithm that Big-O analysis predicts will scale well could actually perform worse than a simpler algorithm that in theory should scale poorly.  For example, the time it takes to initialize an "efficient" data structure could exceed the time it takes to do a naïve String.indexOf() search.

Suggested solution

Building a string and then parsing it as an integer is inefficient.  Why not just compute the integer as you go?

public int getValue(String key, String code) {
    int value = 0;
    for (int i = 0; i < code.length(); i++) {
        int pos = 1 + key.indexOf(code.charAt(i));
        if (pos == 10) {
            value *= 10;
        } else if (pos > 0) {
            value = 10 * value + pos;
        }
    }
    return value;
}


A* path finding getting the node neighbors Is there any way I can minimise this code.The code will get the neighbours of the current node and point the arrow to the current node

private boolean isDiagonal(int x, int y, Node node) {
    if (x == -1 && y == 1) {
        node.arrow.setDrawable("bottom-right");
        return true;
    } else if (x == 1 && y == 1) {
          node.arrow.setDrawable("bottom-left");
        return true;
    } else if (x == -1 && y == -1) {
          node.arrow.setDrawable("top-right");
        return true;
    } else if (y == -1 && x == 1) {
         node.arrow.setDrawable("top-left");
        return true;
    }
    // Determine here if it's cross
    else if (x == -1 && y == 0) {
        node.arrow.setDrawable("right");
        return true;
    } else if (x == 1 && y == 0) {
        node.arrow.setDrawable("left");
        return true;
    } else if (x == 0 && y == -1) {
        node.arrow.setDrawable("up");
        return true;
    } else if (y == 1 && x == 0) {
        node.arrow.setDrawable("down");
        return true;
    }
    return false;
}

output

Just as I answered yesterday in BFS in a grid with wall breaking saldo in Java : define a direction and use it:

enum Direction {
    // define directions for each pair of offsets
    NORTH(-1, 0, "up"),
    NORTHEAST(-1, 1, "top-right"),
    SOUTH(1, 0, "...."),
    ...
    ;

    public final int offsetX;
    public final int offsetY;
    public final String drawableValue;

    private Direction(int offsetX, int offsetY, String drawableValue) {
        this.offsetX = offsetX;
        this.offsetY = offsetY;
        this.drawableValue = drawableValue;
    }
};

private boolean isDiagonal(int x, int y, Node node) {
    for(Direction dir : Direction.values())
        if(x == dir.offsetX && y == dir.offsetY) {
           node.arrow.setDrawable(dir.drawableValue);
           return true;
        }
    return false;
}

Apart from that, the method-name is a lie: it does NOT check for diagonals only, so you should find a name which describes the actual action.

(And just because this is code-review and someone will be tempted to say: add braces to the for loop: no. This is the style I prefer, I know that other people like it in other ways. ;-))


Configuring toString via a public static variable Sometimes I need toString() to be quite verbose, normally I don't. It can't be nicely solved by using other methods as toString() is what gets shown in debugger (and also in the logs unless I call some method explicitly). As nobody should ever rely on its behavior, I wonder if the following is acceptable

public static boolean volatile verboseToString;

public String toString() {
    return toString(verboseToString);
}

public String toString(boolean verbose) {
    if (verbose) {
        return longDescription();
    } else {
        return shortDescription();
    }
}

Is there any better solution?

You could try to use Logger level to control verbose mode.

public String toString() {
    return toString(logger.isDebugEnabled());
}

public String toString(boolean verbose) {
    if (verbose) {
        return longDescription();
    } else {
        return shortDescription();
    }
}

Advantage of this approach is that it's easier to control toString() verbosity level - simply by config file for you logging framework, JMX extension, ...

This could also give you more flexibility - based on log level (INFO, DEBUG, TRACE), your could control how much information you put into toString().


Bootstrap dynamic dropdown items I'm trying to create a function for a dynamic dropdown. I have done it this way and it works, but I want to make my code cleaner.

 function renderRowCounter() {

    var rowCount = [25, 50, 100];
    var first = rowCount[0];
    var items = '';

    $.each(rowCount, function(i,v) {
        items += "
" + v + "
";
    });

    return  "
" +
            "<button type=\"button\" data-toggle=\"dropdown\" class=\"btn btn-default dropdown-toggle\">" + first + " </button>" +
            "
" + items +
            "
" +
            "
 ";
}

Looks pretty clean for me.

2 things:

I'm not an English expert, but check the name of the function and the action.
I understand "render" as to "display on the screen", or "write on the screen," but the function doesn't really write to the screen. It returns some template to be injected later.
Instead of using a concatenated string, you can use a template engine to avoid concatenating strings. It separates the logic from the template and makes both more maintainable. The most popular ones are Mustache, Handlebars, and the Backbone engine.


Inventory check with too many else-ifs I am using if else statements:

public ActionResult Process(int? inventoryCheck, int? POCheck, int? PromotionCheck)
{
    //var result = db.ValidInventory(2);

    //1. When inventory is low:
    if (inventoryCheck != null && POCheck == null && PromotionCheck == null)
    {
        db.ValidInventory(inventoryCheck);
    }
    //2. When PO is not generated
    else if (POCheck != null && inventoryCheck == null && PromotionCheck == null)
    {
        db.ValidInventory(POCheck);
    }
    //3. For Promotion
    else if (POCheck == null && inventoryCheck == null && PromotionCheck != null)
    {
        db.ValidInventory(PromotionCheck);
    }
    else if (inventoryCheck != null && POCheck != null && PromotionCheck == null)
    {
        //4. When Inventory is low and PO is not generated
        inventoryCheck = 4;
        db.ValidInventory(inventoryCheck);
    }
    else if (inventoryCheck != null && POCheck == null && PromotionCheck != null)
    {
        //5.When inventory is low and Promotion Check
        inventoryCheck = 5;
        db.ValidInventory(inventoryCheck);
    }
    else if (inventoryCheck == null && POCheck != null && PromotionCheck != null)
    {
        //6. When PO is not generated and Promotion Check
        inventoryCheck = 6;
        db.ValidInventory(inventoryCheck);
    }
    else if (inventoryCheck != null && POCheck != null && PromotionCheck != null)
    {
        //7. When PO is not generated and Promotion Check   and inventory is low
        inventoryCheck = 6;
        db.ValidInventory(inventoryCheck);
    }

    return RedirectToAction("BackAnalysis");
}

Here more conditions are present, then I have to add more if else blocks.

How can I optimize it?

You should have to split your function absolutely.
You should have to group commons conditions statements and instructions.

However, I think there are deeper problem in your architecture which cause this piece of code. You have to rethink your problem at higher level.

For the three top conditions you can do this, for example :

    public static void Test(int? inventoryCheck, int? POCheck, int? PromotionCheck)
    {
        bool bInventoryCheck = (inventoryCheck == null);
        bool bPOCheck = (POCheck == null);
        bool bPromotionCheck = (PromotionCheck == null);

        ValidInventory (!bInventoryCheck && bPOCheck && bPromotionCheck, inventoryCheck);
        ValidInventory (bInventoryCheck && !bPOCheck && bPromotionCheck, PromotionCheck);
        ValidInventory (bInventoryCheck && bPOCheck && !bPromotionCheck, POCheck);

    }

    public static void ValidInventory(bool validate, int? toValidate)
    {
        if (validate)
            //db.ValidInventory(toValidate);
    }

But I don't think it's a good solution because as you mention here, you have a lot of other cases. We can reduce your code by many way but it's bury the real problem.

In order to reduce we have to understand the concept behind the Process.

PS : Here the "else if" statement seems to be unnecessary, because you are testing every parameters in each case.


File upload in AngularJS causes browser out of memory error I have an AngularJS code to perform upload of files. At first I thought it works OK but when I've tried to upload bigger files (e.g. 5 files, 10 MB each) then I saw that this code has very poor performance regarding memory handling.

For example: when I attach 5 files (10 MB each = 50 MB in total) then peak browser process memory demand reaches 2 GB!
I've tested it in Firefox, Chrome and IE. Additionally, in Chrome when browser process reaches 3 GB of memory committed then the page crashes.
In Firefox I can see an error in console when trying to upload more than 80 MB at once:
out of memory

I've tried to modify the parts where the actual request is made and removed data: JSON.stringify(formattedData) which was the first bottleneck. And it turned out that stringifying is redundant there.

But the second bottleneck still exists which is the function binArrayToJson which takes the ArrayBuffer and reads the data into the byte array.

var binArrayToJson = function (binArray) {
   var str = [binArray.length];
   for (var i = 0, binLength = binArray.length; i < binLength; i++) {
   str[i] = binArray[i];
   }
   return str;
}

var applyAttachments = function (data1) {
   angular.forEach(data1,
       function (value, key) {
           if (key === "Attachments") {
               for (var i = 0; i < value.length; i++) {
                   if (value[i].file) { //already saved item don't contain 'file' property
                       var reader = new FileReader();

                       reader.onloadend = (function (f) {
                           return function (result) {

                               var arrayBuffer = result.target.result;
                               value[f].fileContent = binArrayToJson(new Uint8Array(arrayBuffer));
                               value[f].isUploaded = true;
                           };
                       })(i);

                       reader.readAsArrayBuffer(value[i].file);
                   }
               }
           }
       });
};

var createMVCModel = function (output) {
   var defaultStringValue = "";
   return {
       id: output.id,
       Name: output.name || defaultStringValue,
       Status: output.status || "Draft",
       Date: output.date || null
       Attachments: output.attachments || []
   };
};

var saveModel = function (data, url) {
   var formattedData = createMVCModel(data);
   var deferred = $q.defer();

   applyAttachments(formattedData);

   var check = function () {
       if (allNewFilesUploaded(formattedData) === true) {
           $http({
                   url: url,
                   method: "POST",
                   data: formattedData,
                   headers: { 'Content-Type': undefined }
               })
               .then(function (result) {
                       deferred.resolve(result);
                   },
                   function (result) {
                       deferred.reject(result);
                   });
       } else {
           setTimeout(check, 1000);
       }

   }
   check();
   return deferred.promise;
};

I omitted the parts where the following requirements are being met:

checking number of files (limit is 10)
checking size of each file (limit is 10 MB each)
checking permitted file extensions
sending rest of the (text) data along with files to the server (ASP.NET MVC method)

Marshaling the file contents as a byte array is obviously problematic.

Browsers can easily upload files through HTML forms, with

<form method="POST" enctype="multipart/form-data">
  <input type="file">
</form>

Adapt the server to handle file uploads using that mechanism, then develop a replacement AJAX client.


Moving a rover and receiving current coordinates I made a little text-based game where you move a rover and it gives you the current coordinates. You can essentially move around a 1000x1000 grid, and get output into stdout on what your coordinates are. I'm simply wondering if I could improve anything.

# Rover moving program
from random import randint

# Starting position for rover
r_pos = {'x': randint(1, 1000), 'y': randint(1, 1000)}

def move_rover(x_pos, y_pos):
    if x_pos <= 1000 and y_pos <= 1000:
        r_pos['x'] = x_pos
        r_pos['y'] = y_pos
        print('x{} y{}'.format(
            r_pos['x'], r_pos['y']))
    elif x_pos > 1000 and y_pos > 1000:
        print('Invalid position: x{} y{}'.format(
            x_pos, y_pos))

You run it like this.

>>> import rover
>>> rover.move_rover(234, 789)
x234 y789

Validation

Your validation is weird, and probably not what you intended.

move_rover(-1, -1) appears to succeed, even though it's not within your 1000 × 1000 grid.
move_rover(-1, 1001) is a no-op.  move_rover(1001, 500) is a no-op.

move_rover(2000, 2000) prints a message.  However, it would be more idiomatic to raise some kind of ValueError.  Printing the error message directly limits the reusability of your code.

Output

Similarly, I advise against having your move_rover() function also print the new coordinates.  Each function should do one thing only.  Printing should be a separate operation.

There is repetition in the code to format the position in the success and error cases.  The formatting code should be factored out to a common routine.

Representation

Using a dictionary with keys named 'x' and 'y' seems cumbersome.  You could use a tuple, a namedtuple` or class.  A class probably make sense, since your rover should act as an object that responds to messages.

Suggested implementation

from random import randint

MIN_COORD, MAX_COORD = 1, 1000

class Rover(object):
    def __init__(self):
        """Places the rover randomly in the coordinate range with a uniform
        distribution"""
        self.x = randint(MIN_COORD, MAX_COORD)
        self.y = randint(MIN_COORD, MAX_COORD)

    def move(self, x, y):
        if MIN_COORD <= x <= MAX_COORD and MIN_COORD <= y <= MAX_COORD:
            self.x, self.y = x, y
        else:
            raise ValueError('Invalid position: %s' % (self))

    def __str__(self):
        """Reports the position of the rover"""
        return 'x{} y{}'.format(self.x, self.y)

You run it like this:

>>> from rover import Rover
>>> rover = Rover()
>>> print(rover)
x944 y556
>>> rover.move(234, 789)
>>> print(rover)
x234 y789


Java method to check if a binary tree is a valid BST I wrote a Java method (along with a private class) to check if a binary tree is also a binary search tree (BST). I would like some feedback on the design of my solution. Here is a brief description and the code is provided below it :

Do a depth-first traversal of the tree and test if each node satisfies the binary search tree property. The binary search tree property states that a node is valid if its key is greater than every ancestral node in whose right-subtree it resides and less than every ancestral node in whose left-subtree it resides. Instead of checking the node against all of its ancestors, simply check the largest number it should be greater than (its lower bound) and the smallest number it should be less than (its upper bound).

private class Bounds {

    Node node;
    int min_bound;
    int max_bound;

}

public boolean check_if_valid_binary_search_tree(Node node) {

    if (node == null) return true;

    Deque<Bounds> stack = new ArrayDeque<Bounds>();

    Bounds bounds = new Bounds();

    bounds.node = node;
    bounds.max_bound = Integer.MAX_VALUE;
    bounds.min_bound = Integer.MIN_VALUE;

    stack.push(bounds);

    while (!stack.isEmpty()) {

        bounds = stack.pop();

        Node curr_node = bounds.node;
        int lower_bound = bounds.min_bound;
        int upper_bound = bounds.max_bound;

        if (curr_node.val < lower_bound || curr_node.val > upper_bound) return false;

        if (curr_node.left != null) {

            Bounds left = new Bounds();

            left.node = curr_node.left;
            left.max_bound = curr_node.val;
            left.min_bound = lower_bound;

            stack.push(left);

        }

        if (curr_node.right != null) {

            Bounds right = new Bounds();

            right.node = curr_node.right;
            right.min_bound = curr_node.val;
            right.max_bound = upper_bound;

            stack.push(right);

        }

    }

    return true;

}

Note : The approach I use is modelled after a solution written in python by Parker Phinney (of Interview Cake) to the problem of checking if a binary tree is a valid BST.

What comes to the actual algorithm, you could traverse the tree in-order and check that node keys are strictly growing. This takes \$\Theta(n)\$ time.

public class Main {

    private static final class TreeNode {
        int key;
        TreeNode left;
        TreeNode right;
        TreeNode parent;

        TreeNode(int key) {
            this.key = key;
        }
    }

    private static TreeNode minimum(TreeNode node) {
        if (node == null) {
            return null;
        }

        while (node.left != null) {
            node = node.left;
        }

        return node;
    }

    private static TreeNode successor(TreeNode node) {
        if (node == null) {
            return null;
        }

        if (node.right != null) {
            return minimum(node.right);
        }

        TreeNode parent = node.parent;

        while (parent != null && parent.right == node) {
            node = parent;
            parent = parent.parent;
        }

        return parent;
    }

    public static boolean isBinarySearchTree(TreeNode root) {
        if (root == null) {
            return true;
        }

        TreeNode node = minimum(root);
        int previousKey = node.key;

        while (true) {
            node = successor(node);

            if (node == null) {
                return true;
            }

            if (previousKey >= node.key) {
                return false;
            }

            previousKey = node.key;
        }
    }

    public static void main(final String... args) {
        // Build a valid BST.
        TreeNode root = new TreeNode(0);
        TreeNode node1 = new TreeNode(-2);
        TreeNode node2 = new TreeNode(2);
        TreeNode nodeBad = new TreeNode(-2);

        root.left = node1;
        node1.parent = root;

        root.right = node2;
        node2.parent = root;

        node1.right = nodeBad;
        nodeBad.parent = node1;

        System.out.println(isBinarySearchTree(root));

        nodeBad.key = -3;

        System.out.println(isBinarySearchTree(root));

        nodeBad.key = -1;

        // Now should print 'true'.
        System.out.println(isBinarySearchTree(root));
    }
}

Naming

Please fix check_if_valid_binary_search_tree to camel case: checkIfValidBinarySearchTree.

Also, not curr_node, but rather currentNode would more appropriate for Java.

Coding conventions

Fix

if (curr_node.val < lower_bound || curr_node.val > upper_bound) return false;

to

if (curr_node.val < lower_bound || curr_node.val > upper_bound) {
    return false;
}

Use the diamond inference:

Deque<Bounds> stack = new ArrayDeque<>();

instead of

Deque<Bounds> stack = new ArrayDeque<Bounds>();


Using Node.js, Express, and Express Router to implement a very simple REST API To learn the ropes of Node.js and Express, I'm trying to implement a very simple REST API. The API says what it should do instead of doing it, so I can focus on the framework's structure. The requirement for the API is the following:

Mock API for a question and answer game.
/qanda endpoint: Repository of all question-answer pairs.
/qanda/questionId endpoint: A specific question-answer pair.
Should handle HTTP verbs GET, POST, PUT, DELETE.
Assume HTTP request body is JSON.

All this is arbitrary. I'm just trying to get the hang of dealing with and parsing of HTTP requests and responses. To keep things organized I'm also using Express Router and Node.js modules.
Here is index.js
'use strict';
const express = require('express');

const qanda = require('./routes/qanda.js');

const port = 9999;
const app = express();

app.use('/qanda', qanda);

app
  .all('/', (req, res, nxt) => {
  res
    .status(200)
    .send('Welcome!');
  })
  .listen(port, () => {
    console.log(`Listening to localhost:${port}`);
  });

And here is ./routes/qanda.js
'use strict';
const express = require('express');

const qandaRouter = express.Router();
qandaRouter.use(express.json());

qandaRouter
  .route('/')
  .get((req, res, nxt) => {
    res
      .status(200)
      .send('Sending all the qandas!');
  })
  .post((req, res, nxt) => {
    res
      .status(200)
      .send(`Adding ${req.body.q} = ${req.body.a}`);
  })
  .put((req, res, nxt) => {
    res
      .status(405)
      .send('PUT not supported.');
  })
  .delete((req, res, nxt) => {
    res
      .status(200)
      .send('Deleting all qands!');
  });

qandaRouter
  .route('/:qId')
  .get((req, res, nxt) => {
    res
      .status(200)
      .send(`Sending qanda ${req.params.qId}`);
  })
  .post((req, res, nxt) => {
    res
      .status(405)
      .send('POST not supported.');
  })
  .put((req, res, nxt) => {
    res
      .status(200)
      .send(`Updating ${req.params.qId} with ${req.body.q} = ${req.body.a}`);
  })
  .delete((req, res, nxt) => {
    res
      .status(200)
      .send(`Deleting qanda ${req.params.qId}`);
  });

module.exports = qandaRouter;

I'm looking forward to all kind of feedback! From style, to best practices, anti-patterns, whatever you can think of is very much appreciated! I do have some specific questions:

Is method chaining the usual way to handle something like this in Express?
Is the method chaining style readable and maintainable?
Is there a more concise way to achieve this?
What would you do differently?

After I examine the codes you have written, I could see some things that are not rightly done. Below are the few things I highlighted:

The structuring of the codebase is not okay hence it can be maintainable and scalable if the codebase grows.
The RESTful API naming convention is not an acceptable one. Endpoints of any REST API should contain only resources (nouns) and use HTTP methods (verbs) for actions. You named your endpoint as /qanda instead of /qandas. I recommend you read this RESTful API Designing guidelines by Mahesh Haldar posted at Hackernoon. It's a great resource for developers.
Thirdly, you did not separate your server from express; they are both coupled together. It's not best practice to do so.
When API is being consumed by other applications, upgrading the APIs with some changes would also lead to breaking the existing services contract - this has been noticed by a lot of applications that provide API services. It is a best practice to always prefix all your URLs with /api/v1/users . If there is any major breaking update, we can name the new set of APIs as v2, v3, or v1.X.X. respectively.

From my experience, this is how I would approach it:

Firstly, I will create a directory called, qanda-api, inside it, I'll create other directories called src and two files app.js and server.js that reside inside the root directory with src
Secondly, inside src I will create two sub-directories called routes and controllers
Thirdly, I will initialize an npm registry in the root of the project directory that keeps track of all npm installed packages by typing the command below via the terminal:

npm init

After that, I will install the three dependencies that I will be using via the same terminal, but this time you must have internet access:

npm install express dotenv morgan

If the above process executes successfully, you will see the installed packages inside a file called package.json

Now let's do the coding by getting our hands dirty

Let's first create a file named controllers/qandaController.js in other to write our qanda logic that will be exported to its route file.

exports.getAllQandas = (req, res) => {
    res.status(200).json({
        status: 'success',
        message: 'Documments retrieved successfully',
        data: 'The data goes here from the data!'
    });
};

exports.getQanda = (req, res) => {
    res.status(200).json({
        status: 'success',
        message: 'Doc retrieved successfully',
        data: 'The data goes here from the data!'
    });
};

exports.createQanda = (req, res) => {
    res.status(201).json({
        status: 'success',
        message: 'Qanda created successfully',
        data: 'Return the created data here!'
    });
};

exports.updateQanda = (req, res) => {
    res.status(200).json({
        status: 'success',
        message: 'Doc updated successfully',
        data: 'The updated data goes here!'
    });
};

exports.deleteQanda = (req, res) => {
    res.status(200).json({
        status: 'success',
        message: 'Doc deleted!',
        data: 'Return the Deleted data'
    });
};

Now, it's time for us to create our qanda route in routes/qandaRoutes.js. We will then import the controller logic in its route handler below.

const express = require('express');
/**
 * import the qanda controller
 */
const qandaController = require('./../controllers/qandaController');

const router = express.Router();

router
    .route('/')
    .get(qandaController.getAllQandas)
    .post(qandaController.createQanda);

router
    .route('/:id')
    .get(qandaController.getQanda)
    .patch(qandaController.updateQanda)
    .delete(qandaController.deleteQanda);

module.exports = router;

Let's open the file we created earlier on called app.js in other to make use of the express we had installed. The content of this file is only for express which is then exported to its server:

const express = require('express');
const morgan = require('morgan');

const qandaRouter = require('./routes/qandaRoutes');

const app = express();

if (process.env.NODE_ENV === 'development') app.use(morgan('dev'));

// ROUTES
app.use('/api/v1/qandas', qandaRouter);

/*
 ** HANDLING UNHANDLED ROUTES
 */
app.all('*', (req, res, next) => {
    res.status(404).json({
        status: 'fail',
            message: `Can't find ${req.originalUrl} on this Server!`
    });
});

module.exports = app;

Let's create our server which becomes our entry point to the application. This where the actual execution happens whereby the exported app.js get used by inserting it as an argument in the createServer() function.

require('dotenv').config({ path: '.env' });
const http = require('http');

const app = require('./app');

const port = process.env.PORT || 5000;

const server = http.createServer(app);

server.listen(port, () => console.log(`App running on port ${port}`));

Conclusion
It's highly recommended to always have separation of concerns while writing codes and to aptly have a clean codebase that is maintainable and scalable over time. There's more to code organization when it comes to API design, one of it is the DRY principle (Don't Repeat Yourself). If you want to learn more about API design in Node.JS I recommend you take this course by Jonas. He demystifies the abstract concepts and taught best practices to the core. I've wrapped this answer with API design projects in Github: API and Tourism API. Feel free to clone or modify as you need.


Parsing indented text and inserting its data into a tree My goal is to parse indented text in the style of python and YAML.

This only find the parent of each line.

This bit of code seems to do the trick, but I'm not really satisfied and I wanted to know if you would do this another way.

raw = """animal
    carnivorous
        tiger
        lion
    vegetarian
        cow
        sheep
plant
    algea
    tree
        leaf
        pine
    fungus
        good
        bad
            evil
            mean
    cactus
        big
        small"""

lines = raw.split('\n')
indents = [(0,0,'root')]
for a in raw.split('\n'):
    indent = 0
    while a[indent] == ' ': indent+=1
    if indent % 4:
        print("not multiple of 4")
        break
    indents.append((len(indents), int(indent/4)+1,a.replace('  ','')))
for a in indents: print(a)

stack=[indents[0]]
entries =[indents[0]]

prev_indent = 0
for item in indents[1:]:
    print("#########################")
    id, indent, name = item
    diff = indent - prev_indent
    print(item)
    print("diff",diff, [a[2] for a in stack])

    if diff>0:
        entries.append(item+(stack[-1][2],))
    elif diff<0:
        # entries.append(item+(stack[-diff][2],))
        count = -diff
        while count>-1: stack.pop();count-=1
        entries.append(item+(stack[-1][2],))
    elif diff==0:
        stack.pop()
        entries.append(item+(stack[-1][2],))
    stack.append(item)
    prev_indent = entries[-1][1]

    print("result", entries[-1])
print("########################")

for a in entries:
    if len (a) == 3: continue
    ident, level, name, parent = a
    print(level*' '*4, name, '(', parent, ')')

This results in this (the name in parenthesis is the parent):

animal ( root )
    carnivorous ( animal )
        tiger ( carnivorous )
        lion ( carnivorous )
    vegetarian ( animal )
        cow ( vegetarian )
        sheep ( vegetarian )
plant ( root )
    algea ( plant )
    tree ( plant )
        leaf ( tree )
        pine ( tree )
    fungus ( plant )
        good ( fungus )
        bad ( fungus )
            evil ( bad )
            mean ( bad )
    cactus ( plant )
        big ( cactus )
        small ( cactus )

You should be able to accomplish this task by making one linear pass through the lines, instead of making one pass to build indents and a second pass to build entries.

It's good practice to package your code into functions, and to write docstrings for them.  In particular, if you have code that follows the pattern

outputs = []
for item in inputs:
    outputs.append(…)

… then consider writing a generator instead.

When you detect indentation that is not a multiple of 4 spaces, you print a message and stop building indents, but other than that, you still allow the program to proceed normally.  The program should probably abort at that point, and I suggest doing so by raising an exception.  Furthermore, I consider indentation that is suddenly excessively deep (e.g. going from 1 level to 3 levels of indentation) to be another kind of error that should be detected.

I don't like the way you handle the special case of the root node.  In particular, having a non-uniform tuple length is asking for trouble — it is basically data that is not of the same type.  I would avoid making the root node part of the data structure altogether, so that you don't have to write an exclusion for this special case:

for a in entries:
    if len (a) == 3: continue

Analyzing the text one character at a time (using while a[indent] == ' ': indent+=1) feels tedious.  I suggest using regular expressions to describe what kind of text you are expecting.  For example,

re.compile(r'^(?P<indent>(?: {4})*)(?P<name>\S.*)')

… says that you are looking for indentation at the beginning of the line that is a multiple of four spaces, followed by a name that starts with a non-space character.

Suggested solution

import re

def parse_tree(lines):
    """
    Parse an indented outline into (level, name, parent) tuples.  Each level
    of indentation is 4 spaces.
    """
    regex = re.compile(r'^(?P<indent>(?: {4})*)(?P<name>\S.*)')
    stack = []
    for line in lines:
        match = regex.match(line)
        if not match:
            raise ValueError(
                'Indentation not a multiple of 4 spaces: "{0}"'.format(line)
            )
        level = len(match.group('indent')) // 4
        if level > len(stack):
            raise ValueError('Indentation too deep: "{0}"'.format(line))
        stack[level:] = [match.group('name')]
        yield level, match.group('name'), (stack[level - 1] if level else None)

raw = """…"""
for level, name, parent in parse_tree(raw.split('\n')):
    print('{0}{1} ( {2} )'.format(' ' * (4 * level), name, parent or 'root'))


Custom array like implementation with components x, y, z I want to implement a custom vector-like in Python that stores its components in a list (or another container). And I want to access the components as x, y and z in order to get and set them. What is the best way to do it?

I implemented it like this:

import numpy as np

class Vector3d:
    components = ['x', 'y', 'z']

    def __init__(self):
        self._data = np.array([0.0, 0.0, 0.0])

    def __getattr__(self, key):
        if key in self.components:
            index = self.components.index(key)
            return self._data[index]
        else:
            return super().__getattr__(key)

    def __setattr__(self, key, value):
        if key in self.components:
            index = self.components.index(key)
            self._data[index] = value
        else:
            return super().__setattr__(key, value)

    def __repr__(self):
        return repr(self._data)

    def norm(self):
        return np.linalg.norm(self._data)

a = Vector3d()
a.x = 1.2
a.y = 2.3
a.z = 3.4
print(a.x, a.y, a.z)
print(a)
print(a.norm())

Here are the aspects that I dislike about it:

First, I duplicated the code if key in self.components: index = self.components.index(key).
Second, searching for the index every time seems to be non-optimal towards the consuming time. I believe there's a better way to implement it.

Please, suggest different approaches to me.

There may be many causes of doing it. For example, I may want to store the components as (ctypes.c_double * 3) array and to pass them to a DLL to gain performance of some calculations. Or to store them as numpy.array having the access to all numpy array methods. But I still want to keep the access to the components through x, y, z from the outside.

I suggest you to use a solution that relies on properties notion. This is simpler and more efficient to implement. Here is what you can do:

class CustomArrayLike:

    def __init__(self):
       self._x = None
       self._y = None
       self._z = None

    @property
    def x(self):
       return self._x

    @x.setter
    def x(self, value):
       self._x = value

    @property
    def y(self):
       return self._y

    @y.setter
    def y(self, value):
       self._y = value

    @property
    def z(self):
       return self._z

    @x.setter
    def z(self, value):
       self._z = value

    def __repr__(self):
       return repr([self._x, self._y, self._z])

if __name__ == '__main__':
   a = CustomArrayLike()
   a.x = 1.2
   a.y = 2.3
   a.z = 3.4
   print(a.x, a.y, a.z)
   print(a)


Birthday validity-checking I have written code to check any birthday input's validity. As I am new in programming, and after going through several debugging steps, the code became very ugly.

month_dict = {'jan':'January',
          'feb':'February',
          'mar':'March',
          'may':'May',
          'jul':'July',
          'sep':'September',
          'oct':'October',
          'dec':'December',
          'apr':'April',
          'jun':'June',
          'aug':'August',
          'nov':'November'}

day = int(raw_input ('Enter your birth day: '))
month = raw_input ("Enter your birth month: ")
year_input = int (raw_input ('Enter your birth year: '))

days_31 = ['jan', 'mar', 'may', 'jul', 'aug', 'oct', 'dec']
days_30 = ['apr', 'jun', 'sep', 'nov']
days_28 = ['feb']

def valid_day_finding ():
    global valid_day
    if month_name in days_31:
        if day > 0 and day < 32:
            valid_day = day
        else:
            valid_day = 'invalid'
    elif month_name in days_30:
        if day >= 1 and day <= 30:
            valid_day = day
        else:
            valid_day = 'invalid'
    elif month_name in days_28:
        if year != 'invalid':
            if (year % 4 == 0 and year % 100 != 0) or (year % 400 == 0):
                if day >= 1 and day <= 29:
                    valid_day = day
                else:
                    valid_day = 'invalid'
            else:
                if day >= 1 and day <= 28:
                    valid_day = day
                else:
                    valid_day = 'invalid'
        else:
            valid_day = 'invalid'
    else:
        valid_day = 'invalid'
def valid_month_finding():
    global month_name
    if month in month_dict.keys():
        month_name = month
    else:
        month_name = 'invalid'
def valid_year_finding():
    global year
    if year_input > 1900 and year_input <2020:
        year = year_input
    else:
        year = 'invalid'
def birthday_checking():
    if valid_day != 'invalid' and month_name != 'invalid' and year != 'invalid':
        print 'your birthdate is %d - %s - %d' % (valid_day, month_dict[month_name], year)
    else:
        print 'Your Birthday is invalid'
valid_year_finding()
valid_month_finding()
valid_day_finding()
birthday_checking()

This code is very inefficient. How can I improve this code?

I know that there are built-in functions to check this, but I am trying to learn coding, which is why I am giving this a try.

Naming and using functions

These are all very bad function names:

valid_year_finding()
valid_month_finding()
valid_day_finding()
birthday_checking()

Think of functions as actions. Typically their names should include a verb,
take an input, and either return an output, or modify state.
Consider this rephrasing and usage:

try:
    year = get_valid_year(year_input)
    month = get_valid_month(month_input)
    day = get_valid_day(year, month, day_input)
    validate_birthday(year, month, day)

    print('Your birthday is {} - {} - {}'.format(year, month, day))

except ValueError as e:
    print('Invalid birthday: ' + e.message)

The get_* methods should return valid values,
or else raise an exception if the input is invalid.
This way if a problem is found with year,
there's no need to process month.

Notice that above I pass year and month as parameters to get_valid_day.
This usage makes sense,
as you cannot validate a day without knowing the year and the month.

Example implementation:

def get_valid_year(year_input):
    year = int(year_input)
    if 1900 < year < 2020:
        return year
    raise ValueError('year is not within 1900 and 2020')

Formatting

There are serious formatting issues with this code.
Please read and follow the Python style guide.

A variable should have one type

For example after these statements,
year might end up as either integer or string:

if year_input > 1900 and year_input <2020:
    year = year_input
else:
    year = 'invalid'

Avoid the global keyword, and global variables

It's too hard to avoid using global. For example instead of this:

def valid_year_finding():
    global year
    if year_input > 1900 and year_input <2020:
        year = year_input
    else:
        year = 'invalid'

You could reorganize the code like this:

def valid_year_finding():
    if year_input > 1900 and year_input < 2020:
        return year_input
    else:
        return 'invalid'

year = valid_year_finding()

We got rid of the global keyword,
but the year_input global variable still remains.
Let's make that a parameter of the function instead:

def valid_year_finding(year_input):
    if year_input > 1900 and year_input < 2020:
        return year_input
    else:
        return 'invalid'

year = valid_year_finding(year_input)

Avoid free variables

If a variable is used in a code block but not defined there, it is a free variable.
In this function, year_input is a free variable:

def valid_year_finding():
    if year_input > 1900 and year_input < 2020:
        return year_input
    else:
        return 'invalid'

Free variables are acceptable when their scope is small and obvious.
In the posted code, year_input is defined in the global scope,
making it a global variable, which should be avoided.

Use the chained comparison operator

This condition can be simplified:

   if year_input > 1900 and year_input < 2020:

Using the chain comparison operator:

    if 1900 < year_input < 2020:

Use the right data structures

Instead of lists:

days_31 = ['jan', 'mar', 'may', 'jul', 'aug', 'oct', 'dec']
days_30 = ['apr', 'jun', 'sep', 'nov']
days_28 = ['feb']

These should be sets:

days_31 = {'jan', 'mar', 'may', 'jul', 'aug', 'oct', 'dec'}
days_30 = {'apr', 'jun', 'sep', 'nov'}
days_28 = {'feb'}  # granted, for this one, list or set doesn't matter

Use doc tests

Doc tests can greatly help you test your implementations,
here's an example:

def get_valid_year(year_input):
    """
    >>> get_valid_year('1901')
    1901
    >>> get_valid_year('1900')
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
    ValueError: year is not within 1900 and 2020
    >>> get_valid_year('2020')
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
    ValueError: year is not within 1900 and 2020
    >>> get_valid_year('x')
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
    ValueError: invalid literal for int() with base 10: 'x'
    """
    year = int(year_input)
    if 1900 < year < 2020:
        return year
    raise ValueError('year is not within 1900 and 2020')

If you have this code in a script called bday.py,
you could run the doc tests with:

python -m doctest bday.py

If the tests pass, it outputs nothing.
Otherwise it prints an informative report of what went wrong.


Merge k sorted link lists in python I got asked this following interview for the following problem.
Wondering if you have any thought and feedback for 3 solutions below:

Merge k sorted linked lists and return it as one sorted list. Analyze and describe its complexity. Link to the problem:
https://leetcode.com/problems/merge-k-sorted-lists/description/

# Time:  O(nlogk)
# Space: O(1)

# Merge k sorted linked lists and return it as one sorted list.
# Analyze and describe its complexity.

# Definition for singly-linked list.
class ListNode(object):
    def __init__(self, x):
        self.val = x
        self.next = None

    def __repr__(self):
        if self:
            return "{} -> {}".format(self.val, self.next)

# Merge two by two solution.
class Solution(object):
    def mergeKLists(self, lists):
        """
        :type lists: List[ListNode]
        :rtype: ListNode
        """
        def mergeTwoLists(l1, l2):
            curr = dummy = ListNode(0)
            while l1 and l2:
                if l1.val < l2.val:
                    curr.next = l1
                    l1 = l1.next
                else:
                    curr.next = l2
                    l2 = l2.next
                curr = curr.next
            curr.next = l1 or l2
            return dummy.next

        if not lists:
            return None
        left, right = 0, len(lists) - 1;
        while right > 0:
            if left >= right:
                left = 0
            else:
                lists[left] = mergeTwoLists(lists[left], lists[right])
                left += 1
                right -= 1
        return lists[0]

# Time:  O(nlogk)
# Space: O(logk)
# Divide and Conquer solution.
class Solution2:
    # @param a list of ListNode
    # @return a ListNode
    def mergeKLists(self, lists):
        def mergeTwoLists(l1, l2):
            curr = dummy = ListNode(0)
            while l1 and l2:
                if l1.val < l2.val:
                    curr.next = l1
                    l1 = l1.next
                else:
                    curr.next = l2
                    l2 = l2.next
                curr = curr.next
            curr.next = l1 or l2
            return dummy.next

        def mergeKListsHelper(lists, begin, end):
            if begin > end:
                return None
            if begin == end:
                return lists[begin]
            return mergeTwoLists(mergeKListsHelper(lists, begin, (begin + end) / 2), \
                                 mergeKListsHelper(lists, (begin + end) / 2 + 1, end))

        return mergeKListsHelper(lists, 0, len(lists) - 1)

# Time:  O(nlogk)
# Space: O(k)
# Heap solution.
import heapq
class Solution3:
    # @param a list of ListNode
    # @return a ListNode
    def mergeKLists(self, lists):
        dummy = ListNode(0)
        current = dummy

        heap = []
        for sorted_list in lists:
            if sorted_list:
                heapq.heappush(heap, (sorted_list.val, sorted_list))

        while heap:
            smallest = heapq.heappop(heap)[1]
            current.next = smallest
            current = current.next
            if smallest.next:
                heapq.heappush(heap, (smallest.next.val, smallest.next))

        return dummy.next

if __name__ == "__main__":
    list1 = ListNode(1)
    list1.next = ListNode(3)
    list2 = ListNode(2)
    list2.next = ListNode(4)

    print Solution().mergeKLists([list1, list2])

Bug in Solution3  (using heapq)

The items being put on the heap need to be fully sortable.  Tuples are compared element by element. If the first element of two tuples compare equal, then the next element of the two tuples are compared.  In your solution, the second element of the tuple is a ListNode.  But no methods for comparing ListNodes have been defined.  So if the first element of the tuples are equal a TypeError will be raised when the ListNodes are compared.  The solution is to use 3-tuples: (value, seq_no, ListNode), in which the seq_nos are unique.

class Solution3:
    # @param a list of ListNode
    # @return a ListNode
    def mergeKLists(self, lists):
        dummy = ListNode(0)
        current = dummy

        heap = []
        for seq_no, sorted_list in enumerate(lists):
            if sorted_list:
                heapq.heappush(heap, (sorted_list.val, seq_no, sorted_list))

        while heap:
            _, seq_no, smallest = heapq.heappop(heap)
            current.next = smallest
            current = current.next
            if smallest.next:
                heapq.heappush(heap, (smallest.next.val, seq_no, smallest.next))

        return dummy.next

See discussion in heapq documentation: https://docs.python.org/3.7/library/heapq.html#priority-queue-implementation-notes


Retrieve recognised text from JSON returned by Microsoft OCR The Microsoft OCR API returns json, and if I want to extact the text data from this json:

response = \
{
  "language": "en",
  "textAngle": -2.0000000000000338,
  "orientation": "Up",
  "regions": [
    {
      "boundingBox": "462,379,497,258",
      "lines": [
        {
          "boundingBox": "462,379,497,74",
          "words": [
            {
              "boundingBox": "462,379,41,73",
              "text": "A"
            },
            {
              "boundingBox": "523,379,153,73",
              "text": "GOAL"
            },
            {
              "boundingBox": "694,379,265,74",
              "text": "WITHOUT"
            }
          ]
        },
        {
          "boundingBox": "565,471,289,74",
          "words": [
            {
              "boundingBox": "565,471,41,73",
              "text": "A"
            },
            {
              "boundingBox": "626,471,150,73",
              "text": "PLAN"
            },
            {
              "boundingBox": "801,472,53,73",
              "text": "IS"
            }
          ]
        },
        {
          "boundingBox": "519,563,375,74",
          "words": [
            {
              "boundingBox": "519,563,149,74",
              "text": "JUST"
            },
            {
              "boundingBox": "683,564,41,72",
              "text": "A"
            },
            {
              "boundingBox": "741,564,153,73",
              "text": "WISH"
            }
          ]
        }
      ]
    }
  ]
}

def check_for_word(ocr):
    # Initialise our subject to None
    print("OCR: {}".format(ocr))
    subject = None
    for region in ocr["regions"]:
        if "lines" in region:
          for lines in region["lines"]:
            if "words" in lines:
              for word in lines["words"]:
                if "text" in word:
                  subject = word["text"].lower()
                  break

    print("OCR word is {}".format(subject))
    return subject

print(response["regions"][0]["lines"][0]["words"][0]["text"]) # Should return this

print(check_for_word(response))

Each dictionary has arrays and we are unsure if the array contains any element
Also not sure if the dictionary has key

Let's say we just wish to return the first text it matched from the image file.

This code works but it has a deep nested structure that has bad smell. Is there a better practice to write this in a cleaner way?

One way to almost halve the number of lines (and levels of indentation) needed is to use dict.get with [] as the optional default option:

def check_for_word(ocr):
    for region in ocr["regions"]:
        for lines in region.get("lines", []):
            for word in lines.get("words", []):
                if "text" in word:
                    return word["text"].lower()
    else:
        raise KeyError("OCR word not found")

I would also move the printing outside the function, so you can immediately return and add a else clause to catch the case that it is not present (this part could also be done outside with your code by checking for None).


Fizzbuzz function in PHP Most implementations of FizzBuzz that I've seen rely on 3 modulus operations. Mine does two and a Boolean. (Not implying one is better, just different)

I understand that FizzBuzz is supposedly not to identify great coders, but rather to weed out those that would fail, but I'd like to know if this solution would likely be viewed in positive terms by a potential employer.

<?php
function fizzbuzz($fizz,$buzz,$max=100){
    $output=array();

    for ($i=1; $i <= $max; $i++){
        $output[$i]=''; //If I ignore warnings, this isn't needed.
        if(!($i % $fizz)){
        $output[$i]="Fizz";
        }
        if(!($i % $buzz)){
        $output[$i].="Buzz";
        }
        if(!$output[$i]){
            $output[$i]=$i;
        }
    }
    return $output;
}

foreach(fizzbuzz(3,5) as $out){
    echo $out . "\n"; //or echo $out.'<br>';
}

See it work: Fizzbuzz

Some things I would look for on an interview question such as this:

Is the coder handling bad input cases?  This is something you are not really doing now. What if non-integer values are passed for parameters?  What if values are not > 0?  Oftentimes in interview scenarios like this, I at least want to see the coder acknowledge (even if with a "todo" kind of comment) that validations need to take place.  If I was the one writing this code in the interview, I would probably ask the interviewer if they want you to write the validation code or just get to the heart of the algorithm.  Regardless of their answer, at least they understand that you acknowledge these edge cases.  If you are doing this as an off-line coding test for an interview than I would for sure add in the validation so they don't think you missed this.
I would personally like to see more exact and meaningful comparisons rather than just vaguely truthy/falsey comparison.

Example:

// instead of
if(!($i % $fizz)){
// something like this
if($i % $fizz === 0){

// instead of
if(!$output[$i]){
// do this
if(empty($output[$i])) {

To me, the suggested replacement examples are more clear in intent.  Also, I would be worried about developers that make a habit of loose or unclear comparisons as this could be indicative of someone who is writing error-prone code.

The comment //If I ignore warnings, this isn't needed. is really bothersome.  Why would you ignore warnings?  I think your initialization of this array key to an empty string makes sense, specifically in light of you wanting to check it after the moduli checks to see if it is still an empty string.  Don't apologize for writing code that behaves properly.


Printing chessboard with *(asterisk) and .(dot) I have written a program in which user enters total number of conditions (cases) and then that many number of inputs to print chessboard.

For example:

3
21
*
.
12
*.
35
*.*.*
.*.*.
*.*.*

Here is my code:

def main():
    no_of_case = input()
    for i in range(no_of_case):
        user_input = input()                    # take input

        row = int(str(user_input)[0])
        column = int(str(user_input)[1])

        count = 0

        a = "*"
        b = "."

        while True:
            count = count +1

            if row == 1 and column == 1:
                print "*."
                break

            if row == 1 and column%2 == 0:
                print (a + b) * (column/2)
                break

            if row == 1 and column%2 != 0:
                print (a + b) * (column/2) + "*"
                break

            if column == 1 and row%2 == 0:
                print a
                print b
                if count == row/2:
                    break

            if column == 1 and row%2 != 0:
                print a
                print b

                if count == row/2:
                    print "*"
                    break

            if row != 1 and column !=1:

                if (row%2 == 0) and (column%2== 0):
                    print (a + b) * (column/2)
                    print (b + a) * (column/2)

                elif (row%2 != 0) and (column%2 == 0):
                    print (a + b) * (column/2)
                    print (b + a) * (column/2)

                elif (row%2 == 0) and (column%2 != 0):
                    print (a + b) * (column/2) + "*"
                    print (b + a) * (column/2) + "."

                elif (row%2 != 0) and (column%2 != 0):
                    print (a + b) * (column/2) + "*"
                    print (b + a) * (column/2) + "."

                if row%2 == 0 and count == row/2:
                    break

                elif row%2 != 0 and count == row/2:
                    if column%2 == 0:
                        print "*." * (column/2)
                    elif column%2 != 0:
                        print "*." * (column/2) + "*"
                    break

main()

Any suggestion in what way I can improve it?

Your current code limits the number of rows/columns to a single integer. Is this desired behavior?

For clarity sake, I would probably generate the even/odd rows and then print each until you have printed enough rows

Function to specify the input

def get_int_digits(l):
    try:
        inp = input()
    except SyntaxError:
        print "You did not enter anything"
        return 0
    if type(inp) != int:
        print "You did not enter an integer."
        return 0
    if l is not None and len(str(inp)) != l:
        print "You did not enter the correct number of digits."
        print "{} digits expected.".format(l)
        return 0
    return inp

Function to output the "board"

def print_board(row, column):
    even_string = ("*." * (column // 2 + 1)) [0:column]
    odd_string = (".*" * (column // 2 + 1)) [0:column]

    for i in xrange(row):
        print odd_string if i % 2 else even_string

Driver

if __name__ == __main__:
    cases = get_int_digits(None)
    for i in xrange(cases):
        row_col = get_int_digits(2)
        print_board(row_col // 10, row_col % 10)