Framework to drive a small personal site I have been writing a very slim MVC framework to drive a small personal website. It's nothing intricate, just mostly for fun and a learning experience.

Specifically, this is the initial part of the MVC and just the entry point. I don't want to develop all this logic for the models and views if my dispatcher is not going to work right in a production site. Again, this is just the entry of the project and handles requests to load the controllers via the URL. This is not the entire MVC project.

When I am looking at other frameworks out there, they seem quite complex in their handling of the routing and controller dispatching, and my final code is just a fraction of the size.

I have the server redirect all requests back to index.php using an .htaccess file or via a Nginx config file. I have tried both.

I use the index.php file as the entry point (this is the relevant part):

/* using "Example Implementation" of the PSR-0 standards supporting both '\' and '_' namespace seperations
 * https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md
 * using file_exists to deal with otherwise unhandled errors
 */
function autoload($className) {
    $className = ltrim($className, '\\');
    $fileName  = '';
    $namespace = '';

    if($lastNsPos = strripos($className, '\\')) {
        $namespace = substr($className, 0, $lastNsPos);
        $className = substr($className, $lastNsPos + 1);
        $fileName  = str_replace('\\', DS, $namespace) . DS;
    }

    $fileName .= str_replace('_', DS, $className) . '.php';

    $fileName = ROOT . DS . $fileName;
    if(file_exists($fileName)) {
        require($fileName);
    }
}

$test = new framework\core\Router();

Router.php:

<?php

namespace framework\core {

class Router {

    function __construct() {
        //get the raw url
        $urlRaw = $_SERVER['REQUEST_URI'];

        //do the route :)
        $this->route($urlRaw);
    }

    private function route($urlRaw) {
        //do our best to split the url at the most obvious parts (/, ?, $, =)
        $urlRawParts = preg_split('/[\/?&=]+/', $urlRaw, -1, PREG_SPLIT_NO_EMPTY);

        //do a simple sanitization on the parts (allow alphanumeric and dash while stripping common web extensions)
        $urlRawParts = preg_replace('/[^A-Za-z0-9\-]|\bhtm\b|\bphp\b|\bhtml\b/', '', $urlRawParts);

        //set our controller and action or use defaults
        $controller =   ((!empty($urlRawParts[0])) ? $urlRawParts[0] : 'Index');
        $action =       ((!empty($urlRawParts[1])) ? $urlRawParts[1] : 'index');
        $arguments =    ((count($urlRawParts) > 2) ? array_slice($urlRawParts, 2) : array());

        $this->dispatch($controller, $action, $arguments);
    }

    private function dispatch($controller, $action, $arguments) {
        //format the call to our class
        $controller = 'application\\controllers\\' . ucfirst($controller) . 'Controller';

        //load our class or show the appropriate error page
        $controller = ((class_exists($controller) ? new $controller() : new Error('404')));

        //use the default class method unless a valid one exists
        $method = ((method_exists($controller, $action)) ? $action : 'index');

        //call our class method
        call_user_func_array(array($controller, $method), $arguments);
    }
}
}

?>

This works for any URL that I throw at it and will always default to a known file (Error.php which is just a simplified controller and view in one).

Am I missing anything huge here? Is my logic way off or not complete? Is this utilization slow or wasteful on resources?  I'm just looking for a general idea on if I am on the right track.

All the basic parts are here, but there's still room for improvement.

First of all, I wouldn't have the constructor call route and route then call dispatch because it makes the second method less discoverable when the code is being read. Contrast this with:

function __construct() {
    //get the raw url
    $urlRaw = $_SERVER['REQUEST_URI'];

    $route = $this->route($urlRaw); // don't care what $route is exactly
    $this->dispatch($route);
}

Here it is much more obvious what you method you should be looking for. It's true that you, as creator of the code, do not need this -- but others might, and it's a totally free lunch.

Second: this line bakes an application configuration decision inside the router. You don't want to do this, as it's totally reasonable to want to change this configuration most of the time while the router would stay mostly untouched.

$controller =   ((!empty($urlRawParts[0])) ? $urlRawParts[0] : 'Index');

It's much better to make the default controller name part of the application configuration and have the router take it from there, because it's reasonable to want your "home" page to be equivalent to the "/something" url instead of "/index".

The same goes for

$action =       ((!empty($urlRawParts[1])) ? $urlRawParts[1] : 'index');

It would be better if you allowed the controller writer to pick which action is the default:

$action = ((!empty($urlRawParts[1])) ? 'action'.$urlRawParts[1] : 'defaultAction');

But why also rename index to defaultAction and add the "action" prefix to method names? Consider also your dispatch code:

$method = ((method_exists($controller, $action)) ? $action : 'index');

This check with method_exists would allow any url that maps to a public function to be successfully dispatched, even if that function does not map to a "real" action (it can simply be a helper method). That's not really a security issue (you can easily make sensitive functions private) but it's not very consistent: targeting nonExistentAction will get you to the index page, while targeting e.g. helperMethod will show a blank page as helperMethod most likely does not directly produce content.

By demanding that actions are implemented in functions with names starting with "action" you can make sure that this never happens. The default action is a special exception to this rule, as it allows the controller writer to effectively specify which url routes to the default action and what the default action should do at the same time.

Third: It's not consistent to show a 404 for a missing controller but not do that for a missing action and use the default one instead. You should do the same in both cases, and the right choice would be the 404.

Fourth: Parameter handling needs immediate attention.

To begin with, you are sanitizing parameters to an unreasonably restricted set. What if there's a search action somewhere and the user wants to type in a character like * or a non-alphanum string? Clearly parameter sanitization needs to simply be removed.

Apart from that, you are passing GET parameters to the controller action positionally. The positional part is not a dealbreaker by itself (although it would be a dealbreaker in a framework, or if the URL format were configurable), but the GET restriction is a bit ugly. However, this cannot be fixed easily because there's no good way to know where parameters from $_POST should be inserted, or what their order relative to one another should be.

To address this, you need to make the code reflect into the controller method and look for argument names and default values, pull these out from your list of GET parameters and $_POST, place them in an array ordered by the position of each named parameter in the function signature and call the function with that. You should also probably return an HTTP 400 if the action turns out to have a non-optional parameter the value of which was not provided.

All in all not the end of the world, but a speed bump and a non-trivial amount of code.


Cleaning up code for Conway's Game of Life I've implemented a Ruby version of Conway's Game of Life that functions correctly and passes all my tests.  I was hoping to get some advice on how I could clean up the code for readability and efficiency.

For anyone who doesn't know the rule to the Game of life, they are as follows:

  Any live cell with fewer than two live neighbours dies, as if by needs caused by underpopulation.
  Any live cell with more than three live neighbours dies, as if by overcrowding.
  Any live cell with two or three live neighbours lives, unchanged, to the next generation.
  Any dead cell with exactly three live neighbours becomes a live cell.

(In my implementation, 1 is an alive cell and 0 is a dead cell).

class Game_of_life
  def initialize(hash)
    @test = nil
    str = hash[:string]
    size = hash[:size]
    @length = nil
    @board = []
    if str != nil
      str.each_char {|c| @board << c.to_i}
      @test = true
      @length = Math.sqrt(str.length)
    else
      (size*size).times do
        @board << [0,1].sample
      end
      @length = size
    end
  end

  def check_board

    @board.each_with_index do |organism, index|
      neighbours = {alive: 0, dead: 0}
      neighbour_positions = choose_neighbour_set(index)
      neighbour_positions.each do |position|
        neighbours[:alive] += 1 if @board[index+position] == 1
        neighbours[:dead] += 1 if @board[index+position] == 0
      end
      @board[index] = 0 if organism == 1 && (neighbours[:alive] < 2 || neighbours[:alive] > 3)
      @board[index] = 1 if organism == 0 && neighbours[:alive] == 3
    end
    if @test
      return @board.join("")
    else
      sleep 0.5
      puts "\e[H\e[2J"
      @board.each_slice(@length) {|row| p row }
      check_board
    end
  end

  def choose_neighbour_set(index)
    neighbour_set = [-(@length), -(@length - 1), 1, (@length+1), @length, (@length-1), -1, -(@length + 1)]
    neighbour_set.delete_if{|position| [-(@length + 1), -(@length),-(@length - 1)].include?(position)} if index < @length
    neighbour_set.delete_if{|position| [(@length-1),@length,(@length+1)].include?(position)} if index > ((@length*(@length-1)) -1)
    neighbour_set.delete_if{|position| [-(@length + 1),-1,(@length-1)].include?(position)} if index % @length == 0 || index == 0
    neighbour_set.delete_if{|position| [(@length+1),1,-(@length - 1)].include?(position)} if (index + 1) % @length == 0 && index != 0
    return neighbour_set
  end
end

# Uncomment to watch the game of life unfold!
# Game_of_life.new({size: 25}).check_board

# >>>>>>>>>>>>>>>>>>>>> Driver code

# Test string one
arr = "100100111110100011110110001100110010000010111101100010010111110110111100110000011"
game = Game_of_life.new({string: arr})
p game.check_board == "110000101000111101110000101111110001100011101101000000000000110111111001101011111"

# test string two
arr2 = "110111000101110010011000100110100100000101110111001110111011011101110010010110110"
game2 = Game_of_life.new({string: arr2})
p game2.check_board == "110011000100110110001010100001010010000100011101001000101001011100010001000111110"

# test string three
arr3 ='100110010'
game3 = Game_of_life.new({string: arr3})
p game3.check_board == "110011011"

# test string four

arr4 = "0010010111001001"
game4 = Game_of_life.new({string: arr4})
p game4.check_board == "0010101110100000"

Ruby convention is to use CamelCase for Classnames (GameOfLife)
Since "size" and "str" are mutably exclusive parameters, using a hash parameter here makes the API confusing
Using flags like "@test" to define object behavior is bad style (antipattern)
Ruby doesn't have tail recursion, so you shouldn't do recursive calls without end conditions

To make this more OOP like and self documenting you can define a base class for the basic initialization:

class GameOfLife
  def initialize(length)
    @length = length # Why not call this "size"?
    @board = []
  end

  def check_board
    # Implemented as above without the "if" statement
  end

  def choose_neighbour_set(index)
    # As above
  end
end

Then you can use subclasses for specific implementations

class RandomGameOfLife < GameOfLife
  def initialize(size)
    super(size)
    (size*size).times do
      @board << [0,1].sample
    end
  end

  def iterate_and_output_board
    while true # Iterate till ^C?
      check_board
      sleep 0.5
      puts "\e[H\e[2J"
      @board.each_slice(@length) {|row| p row }
    end
  end
end

class PredefinedGameOfLife < GameOfLife # Or use a better name here
  def initialize(str)
    super(Math.sqrt(str.length))
    str.each_char {|c| @board << c.to_i}
  end

  def check_board
    # This is just an example how to overwrite method behavior
    # Defining another method using "check_board" might be better here
    super
    @board.join("")
  end
end

Somebody else might cover the efficiency aspect. I don't want to intermix the aspects here and its better to focus on readability first.


Python code to retry function I am trying to write a python function which retries a given function until given time or function returns True with given delay.

I have written the following function, but was thinking if there is a better way to do it?

Example function I want to retry for

def is_user_exist(username):

    try:
        pwd.getpwnam(username)
        log.info("User %s exist" % username)
        return True
    except KeyError:
        log.exception("User %s does not exist." % username)
        return False

My retry function

def retry(func, *func_args, **kwargs):
    retry_count = kwargs.get("retry_count", 5)
    delay = kwargs.get("delay", 5)
    while retry_count > 1:
        if func(*func_args):
            return
        log.debug("waiting for %s seconds before retyring again")
        sleep(delay)
        retry_count = retry_count - 1

    return func(*func_args)

I like all of Ev. Kounis' answer, so I'll add some higher level details.

Let it be truth-y

Right now you aren't strictly requiring func to return True or False, just something truth-y or false-y. This is fine, and Pythonic. I think you would benefit from pointing that out in a docstring (which you should also write). Additionally, it may be worthwhile to return the actual value of func(*func_args) instead of True or False; this way you can actually get a value from the function if you wanted to take advantage of the truth-yness of the value.

Better kwargs support

You also don't allow any keyword arguments to be passed to the function - the function might support them, so you should to. I would promote the two keyword arguments you pull out of kwargs to explicit keyword arguments, with appropriate defaults (5 in your case).

Exceptions

It's weird to me that this function does have any concept of retrying after exceptions. I wouldn't want you to do something like this, for what I hope are obvious reasons

for _ in range(retry_count):
    try:
        if func(*func_args):
            return True
    except:
        pass
    log.debug("wating for %s seconds before retrying again")
    sleep delay)

However, in many cases I suspect you would know what exceptions you might expect (for example, a network timeout or connectivity blip) and that you might want to be handled by the retrying framework. To this end, I think something like this could be nice:

def retry(func, *args, retry_count=5, delay=5, allowed_exceptions=(), **kwargs):
    for _ in range(retry_count):
        try:
            result = func(*args, **kwargs)
            if result: return result
        except allowed_exceptions as e:
            pass

This obviously isn't a complete implementation; I left out some of the other pieces you have, and it behaves oddly if it fails on the last iteration, but it should be enough to start.

Fancy stuff

I think we could also get more value from this if it was a decorator. Then consumers of a function don't even need to know if they want it to retry or not; they just call their function and see that it works, and whether or not it was retried becomes irrelevant. Don't forget to use functools.wraps to preserve metadata.

import functools

def retry(retry_count=5, delay=5, allowed_exceptions=()):
    def decorator(f):
        @functools.wraps(f)
        def wrapper(*args, **kwargs):
            for _ in range(retry_count):
                # everything in here would be the same

        return wrapper
    return decorator

Then you can enable retrying for everyone, like so:

@retry(retry_count=5, delay=5)
def is_user_exist(username):
    try:
        pwd.getpwnam(username)
        log.info("User %s exist" % username)
        return True
    except KeyError:
        log.exception("User %s does not exist." % username)
        return False

Really fancy stuff

Why block when you're waiting? You could be doing so much more (this is for Python 3.5 and above) using asyncio.  There isn't built-in support for this before that, but I know there are asynchronous frameworks that should be able to accomplish the same task.

By awaiting an asynchronous function that just runs for n seconds, you achieve the same goal but allow other asynchronous functions to do work while you're just waiting. Note that depending on the event loop you might end up waiting for slightly more or less time.

I also cleaned up the issues I mentioned about handling exceptions; it now always returns the result of the function if it has one, and it'll re-raise the exception without losing any traceback if there was one. That also uses a Python 3 only feature; I've left a comment for how to do it in Python 2.

Note, I'm not as familiar with asyncio as I never got to do any serious dev there, so I might not have this piece of code exactly correct; the theory should be sound though.

import functools
import asyncio

def retry(retry_count=5, delay=5, allowed_exceptions=()):
    def decorator(f):
        @functools.wraps(f)
        async def wrapper(*args, **kwargs):
            result = None
            last_exception = None
            for _ in range(retry_count):
                try:
                    result = func(*func_args, **kwargs)
                    if result: return result
                except allowed_exceptions as e:
                    last_exception = e
                log.debug("Waiting for %s seconds before retrying again")
                await asyncio.sleep(delay)

            if last_exception is not None:
                raise type(last_exception) from last_exception
                # Python 2
                # import sys
                # raise type(last_exception), type(last_exception)(last_exception), sys.exc_info()[2]

            return result

        return wrapper
    return decorator


How should I format my code to make it easier to read and understand in the future I am using this query to generate a conversation stream between user 184 and 192-

SELECT events.event_time, messages . *
FROM   events , messages
WHERE  events.global_id=messages.global_ref_id  AND
       (messages.to =184 AND messages.from =192) OR
       events.global_id=messages.global_ref_id AND
       (messages.to =192 AND messages.from =184) AND
       messages.global_ref_id < 495
ORDER BY `messages`.`global_ref_id` ASC

This query is working good for generating conversation between two users.
Can you help me with converting this query to something more readable. Because I believe it is just a workaround and will cause some problems in future.
Schema of messages table

Field         Type
global_ref_id int(12)
to            int(12)
from          int(12)
message       text
status        int(1)
viewed        int(1)

where global_ref_id is foreign key.

I prefer using JOIN...ON, clarifying an intention of priority of OR and AND phrase.

SELECT
  events.event_time,
  messages . *
FROM
  events
  JOIN messages ON events.global_id = messages.global_ref_id
WHERE
    ((messages.to =184 AND messages.from =192)
      OR (messages.to =192 AND messages.from =184))
    AND messages.global_ref_id < 495
ORDER BY
  messages.global_ref_id ASC


Everyone loves Fibonacci I was bored and burnt out on more serious projects.... until I saw this Computerphile video on YouTube. Now I'm inspired again. Now I feel like spending some time implementing that recursive algorithm in a little console program. Fortunately, recursion is unbearably slow in this case. So, I ended up writing a small class that implements a loop algorithm to find any \$F_n\$.

I'm just learning C#, so I'm most concerned with best practices and style. I want to get a good foundation though, so all feedback is welcome.

Particular Concerns

Is my indentation correct?
Have I named my variables horribly? (I think they smell a bit.)
Which implementation is better? I'm feeling like it's six of one or a half doz. of the other.
Should I be concerned about hitting the upper boundary of the ulong type? Fibonacci numbers grow very quickly. Is there a better representation of very large numbers?

Fibonacci

class Fibonacci
{
    private int _n;
    private ulong _value;
    private ulong _FnMinus1;
    private ulong _FnMinus2;

    public int n
    {
        get { return _n; }
        set { _n = value; }
    }

    public ulong Value
    {
        get {return _value;}
        //set { _value = value; }
    }

    public void Calculate()
    {
        if (n <= 0)
        {
            throw new ArgumentOutOfRangeException("n","Can't calculate Fn when n is less than or equal to zero.");
        }
        else if (n == 1 || n == 2)
        {
            _value = 1;
        }
        else
        {
            //initialize previous results
            _FnMinus1 = 1;
            _FnMinus2 = 1;

            for (int i = 0; i <= (n - 2) ; i++)
            {
                _value = _FnMinus1 + _FnMinus2;
                _FnMinus2 = _FnMinus1;
                _FnMinus1 = _value;
            }
        }
    }

    //constructors
    public Fibonacci(int n)
    {
        _n = n;
        Calculate();
    }

    public Fibonacci() { }

}

Implementation1

class Program
{
     static void Main(string[] args)
    {

        Fibonacci fib = new Fibonacci();
        for (int i = 1; i <= 50; i++)
        {
            fib.n = i;
            fib.Calculate();
            Console.WriteLine(fib.Value);
        }

        Console.WriteLine("Press enter to close...");
        Console.ReadLine();
    }
}

Implementation2

class Program
{
     static void Main(string[] args)
    {

        for (int i = 1; i <= 50; i++)
        {
            Fibonacci fib = new Fibonacci(i);
            Console.WriteLine(fib.Value);
        }

        Console.WriteLine("Press enter to close...");
        Console.ReadLine();
    }
}

Update: Follow up question here.

Indentation and formatting are fine, as is the algorithm itself.

Your naming is inconsistent, compare n and Value; _value and _FnMinus1.  There are a variety of conventions to follow, but consistency is the most important thing.

_FnMinus1 and _FnMinus2 are only used in one method, so they can be made local to that method.

The API is a little odd:

fib.n = i;
fib.Calculate();
int result = fib.Value;

It would be more convenient if I could do this with one line, like int result = fib.Calculate(i), which would return the value.  Then it seems that n and Value are unnecessary.

Furthermore, right now there is nothing that keeps n and Value synchronized.  The class should maintain the invariant that Value is always the nth Fibanacci number.  You could do the work in the n assignment or mark Value as dirty when n changes.  Either way you essentially get rid of Calculate, so that's another approach.  I prefer getting rid of n and Value, I just mention this in case you think it makes sense to keep them.

Between Implementation 1 and Implementation 2, it's pretty much a wash.  1 has the benefit of only making one allocation.  Since the class is (going to be) stateless, it's perfectly reusable.  On the other hand 2 declares fib inside the loop, which minimizes its scope (which is good).


Class inheritance testing I'm just messing around with class inheritance. I originally had a separate goal in mind but I ended up here, and my new goal was to have a parent that was completely ignorant of it's self, i.e. no default attributes. This would seem to make it easier to subclass, the MeleClass could set; for instance canRange to True during instantiation without modifying the parent's parameters.

Questions:

Are there any other ways to do this?
How could this be bad?
Are there more efficient ways?

class Character:
    def __init__(self, **kwargs):
        for i in kwargs:
            setattr(self, i, kwargs[i])

class MeleClass(Character):
    def __init__(self, name="default", atk=1, deff=1):
        self.canRange = False
        Character.__init__(self, name=name, attack=atk, defence=deff)

class SpellClass(Character):
    def __init__(self, name="default", atk=1, deff=1):
        self.canRange = True
        Character.__init__(self, name=name, attack=atk, defence=deff)

p = MeleClass(name='TheLazyScripter')
print p.name
print p.attack
print p.canRange

I would change your Character.__init__ to this:

def __init__(self, **kwargs):
    for key, value in kwargs.iteritems():
        setattr(self, key, value)

It isn't any shorter but it's more clear what's happening.
For one thing, i is not a descriptive name by any means.  For another thing, you might as well iterate through key-value pairs1 rather than just keys.  You use both; why not iterate through both?

I would also change your other __init__ methods.  If Character.__init__ sets attributes, why not take advantage of it?  Instead of self.canRange = ..., add canRange=... to the arguments to Character.__init__.

That brings me to something else.  PEP 8, the Python style guide, says this about instance variables (such as canRange):

  Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.

1 You could also do self.__dict__.update(kwargs) but that isn't recommended and seems hackish to me anyway.


Xml to Csv Python Code too long The code takes a very simple xml and converts it into a dataframe in csv file.

This is the Xml file

<Data xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
  <Phones>
    <Date />
    <Prog />
    <Box />
    <Feature />
    <WIN>MAFWDS</WIN>
    <Set>234234</Set>
    <Pr>23423</Pr>
    <Number>afasfhrtv</Number>
    <Simple>dfasd</Simple>
    <Nr />
    <gt>6070106091</gt>
    <Reno>1233</Reno>
    <QW>3234</QW>
    <ER />
    <VR />
    <Use />
    <Dar>sdfsd</Dar>
    <age />
    <name1>sdfsfdfs</name1>
    <Sys>IVECO E.A.SY.</Sys>
    <aac>2019-05-29</aac>
    <time>02:00</time>
    <nuk name="This is some text" text_g="asadsdas" text_h="2">fsdfsfd3432fdf</nuk>
  </Phones>

</Data>

This is my code.
It works, it just gets way too long when I want to include all the children of the father phones.
Looking for some help on how to shorter the code but it performing the same purpose.

This is the code

import xml.etree.cElementTree as et
import pandas as pd

df = pd.DataFrame()

def getvalueofnode(node):
    """ return node text or None """
    return node.text if node is not None else None

def main():
    """ main """
    parsed_xml = et.parse("short.xml")
    dfcols = ['Date', 'Prog', 'Box', 'Feature', 'WIN']
    df_xml = pd.DataFrame(columns=dfcols)

    for node in parsed_xml.findall('Phones'):
        Date = node.find("Date")
        Prog = node.find('Prog')
        Box = node.find('Box')
        Feature = node.find('Feature')
        WIN = node.find('WIN')

        df = df_xml.append(
            pd.Series([getvalueofnode(Date), getvalueofnode(Prog), getvalueofnode(Box),
                       getvalueofnode(Feature), getvalueofnode(WIN)],  index=dfcols),
            ignore_index=True)

    df.to_csv("./export.csv")

main()

This is all the columns declared fro dfcols

dfcols = ['Date', 'Prog', 'Box', 'Feature', 'WIN',
          'Set', 'Pr', 'Number','Simple','Nr',
          'gt', 'Reno','QW', 'ER','VR', 'Use',
          'Dar','age','name1', 'Sys' 'aac',
          'time', 'nuk']

Additional

The element nuk has multiple attributes so each attributes gets its own column
Example

nuk_name, nuk_text_g, nuk_text_h

Optimization and restructuring

Redundant and inefficient things
The getvalueofnode function is unneeded: text attribute of a node will be None or string value. All intermediate dataframes are also unnecessary.Avoid using pd.Dataframe.append method within any loop as it will create a copy of accumulated dataframe of each loop iteration.

Namings
Use a meaningful names for your functions and if __name__ == '__main__': guard.I would suggest phones_to_df function name.

Assuming that the goal is to convert all Phones tags child nodes into dataframe records - all you need is collect a dictionaries of child nodes as <el.tag: el.text> mappings at once (+ special case for nuk tag, see below).Note, that some Phones tags could have their children in different order. So a list of dictionaries provides a robust way to arrange columns to respective values.

See the concise optimized approach below:

import xml.etree.cElementTree as cET
import pandas as pd

def phones_to_df(fname):
    tree = cET.parse(fname)
    pd.DataFrame([{**{el.tag: el.text for el in list(phone)},
                   **{f'nuk_{k}': v for k, v in phone.find('nuk').items()}}
                  for phone in tree.iterfind('Phones')]).to_csv('export.csv', index=False)

if __name__ == '__main__':
    phones_to_df(fname="test.xml")

After generating the expected export.csv file the resulting sample dataframe may look as below (I've tested on xml file with 2 Phones tags):

print(pd.read_csv('export.csv').to_string())

   Date  Prog  Box  Feature     WIN     Set     Pr     Number Simple  Nr          gt  Reno    QW  ER  VR  Use    Dar  age     name1            Sys         aac   time             nuk           nuk_name nuk_text_g  nuk_text_h
0   NaN   NaN  NaN      NaN  MAFWDS  234234  23423  afasfhrtv  dfasd NaN  6070106091  1233  3234 NaN NaN  NaN  sdfsd  NaN  sdfsfdfs  IVECO E.A.SY.  2019-05-29  02:00  fsdfsfd3432fdf  This is some text   asadsdas           2
1   NaN   NaN  NaN      NaN    ABCD     123    456        aaa    bbb NaN        1111   222   333 NaN NaN  NaN    rrr  NaN      wwww  TIMES E.A.SY.  2019-12-29  03:00        text ...  This is some text   asadsdas           2


Python program to know who wins a tic-tac-toe game Given an input where we have two players: "X" and "O", and empty cells marked with a "." like:

check_winner([
        "X.O",
        "XX.",
        "XOO"])

the program outputs if there are three "X" or "O" in horizontal, vertical or diagonal. If there is a draw it outputs "D".

def check_winner(game_result: List[str]) -> str:

    for i, row in enumerate(game_result):
        if game_result[i][0] == game_result[i][1] == game_result[i][2]:
            if game_result[i][0] != ".":
                return game_result[i][0]
        if game_result[0][i] == game_result[1][i] == game_result[2][i]:
            if game_result[0][i] != ".":
                return game_result[0][i]

    if game_result[0][0] == game_result[1][1] == game_result[2][2]:
        if game_result[1][1] != ".":
            return game_result[1][1]

    if game_result[0][2] == game_result[1][1] == game_result[2][0]:
        if game_result[1][1] != ".":
            return game_result[1][1]

    return "D"

How could we improve this code?

To avoid repetitious code, look for commonality. Here, there's a repeated task of checking whether a group of 3 characters are all the same (and not .). This could be done more generally using a set. Sets don't carry duplicates, so if the set of a group has length 1, that means all characters were the same.

The example below shows this idea, by first extracting the different groups and then iterating over them all. This probably means it's less efficient than it could be - it would be faster to create as few groups as possible - but I think the code is somewhat legible at least.

def check_winner(lst):

    # collect "triplets": rows, columns and diagonals
    rows = lst[:]
    cols = list(zip(*lst))
    diag1 = [row[i] for i, row in enumerate(rows)]
    diag2 = [row[2 - i] for i, row in enumerate(rows)]
    triplets = rows + cols + [diag1, diag2]

    # if all characters in a triplet are the same, and that character
    # is not ".", then that character marks the winner
    for triplet in triplets:
        if len(set(triplet)) == 1 and triplet[0] != '.':
            return triplet[0]
    return 'D'

print(check_winner([ "X.O", "XX.", "XOO"]))


Temperature conversion in C I have started to learn C and I have tried my hand at temperature conversion. The idea is to convert a temperature from Fahrenheit to Celsius and vice/versa. Can somebody review my code and let me know how I might be able to improve it?  My program is as follows:

#include <stdio.h>

void fahconv()
{
    double fah;
    puts("Enter value to convert from Fahrenheit to Celsius");
    scanf("%lf",&fah);
    printf("Converted temperature from F to C: %lf",((fah - 32.0)*5.0)/9.0);
}

void celsconv()
{
    double cels;
    puts("Enter value to convert from Celsius to Fahrenheit");
    scanf("%lf",&cels);
    printf("Converted temperature from C to F: %lf",(cels*9/5)+32);
}

void invalidchoice()
{
    printf("Invalid choice..exiting");
}

int main()
{
    char choice;
    puts("Enter (F or f) to convert from Fahrenheit to Celsius");
    puts("Enter (C or c) to convert from Celsius to Fahrenheit \n");
    scanf("%c",&choice);
    switch(choice)
    {
        case 'c':
        case 'C': celsconv();
              break;
        case 'f':
        case 'F': fahconv();
              break;
        default: invalidchoice();
    }
    return 0;
}

You don't need invalidchoice() as it just prints a statement and nothing else.  Just put the printf() under default.
Consider receiving all user input in main() and having the functions only do the calculations.  It's best to have a function do one (useful) thing.

In this form, the functions could also return the values to main() and displayed.  With the user input eliminated, they should now take the original temperature as an argument.
Optionally, you can return EXIT_SUCCESS or EXIT_FAILURE instead of 0 or 1, respectively.  It's especially important to return a failure value if the conversion was not successful.

You could then have something like this:

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

double fahconv(double fah)
{
    return (fah - 32) * 5 / 9;
}

double celsconv(double cels)
{
    return (cels * 9 / 5) + 32;
}

int main()
{
    double originalTemp;
    double convertedTemp;
    char choice;

    scanf("%lf", originalTemp);

    puts("Enter (F or f) to convert from Fahrenheit to Celsius\n");
    puts("Enter (C or c) to convert from Celsius to Fahrenheit\n");
    scanf("%c", &choice);

    switch (choice)
    {
        case 'c':
        case 'C':
              convertedTemp = celsconv(originalTemp);
              printf("Converted temperature from F to C: %lf", convertedTemp);
              break;
        case 'f':
        case 'F':
              convertedTemp = fahconv(originalTemp);
              printf("Converted temperature from F to C: %lf", convertedTemp);
              break;
        default:
              printf("Invalid choice..exiting");
              return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}


Combining json responses of recursive web service calls with PHP and curl I'm implementing the Hotel Price List API from Skyscanner. This requires a first call to the service to implement a session followed by 1 or more calls to the service to get the actual results.  I've used curl to implement this and got a working solution, I just wonder if this solution can be improved.

What I have:

public static function CallAPI($method, $url, $data = false)
  {
    $curl = curl_init();

    switch ($method)
    {
        case "POST":
            curl_setopt($curl, CURLOPT_POST, 1);

            if ($data)
                curl_setopt($curl, CURLOPT_POSTFIELDS, $data);
            break;
        case "PUT":
            curl_setopt($curl, CURLOPT_PUT, 1);
            break;
        default:
            if ($data)
                $url = sprintf("%s?%s", $url, http_build_query($data));
    }

    curl_setopt($curl, CURLOPT_URL, $url);
    curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($curl, CURLOPT_VERBOSE, 1);
    curl_setopt($curl, CURLOPT_HEADER, 1);

    // Request json
    curl_setopt($curl, CURLOPT_HTTPHEADER, array(
    'Accept: application/json'));

    $result = curl_exec($curl);

    $header_size = curl_getinfo($curl, CURLINFO_HEADER_SIZE);
    $header = substr($result, 0, $header_size);
    $body = substr($result, $header_size);

    $APIResult['error'] = curl_error($curl);
    $APIResult['info'] = curl_getinfo($curl);
    $APIResult['header'] = $header;
    $APIResult['body'] = $body;

    curl_close($curl);

    return $APIResult;
}

  public function GetHotels($nodeId) {

    $node = Node::find($nodeId);

    $apiKey = 'prtl6749387986743898559646983194'; // This is the test key

    $currency = $this->request->session()->get('currency');
    $locale = Lang::locale();
    $market = $this->request->session()->get('countryCode');
    $entityid = $node->lat . ',' . $node->lng . '-latlong';
    $checkindate = '2016-12-17';
    $checkoutdate = '2016-12-18';
    $guests = 2;
    $rooms = 1;

    $url = 'http://partners.api.skyscanner.net/apiservices/hotels/liveprices/v3/' . $market . '/'. $currency . '/'. $locale . '/'. $entityid . '/'. $checkindate . '/'. $checkoutdate . '/'. $guests . '/'. $rooms;

    // First call to initiate session, no actual results retrieved
    $APIResult = APIServices::CallAPI('GET', $url, array("apiKey" => $apiKey));

    $debugResult = $APIResult['body'];

    if (!empty($APIResult['info']['redirect_url']) ) {
        $APIResult =  APIServices::CallAPI('GET', $APIResult['info']['redirect_url']);
        $json = json_decode($APIResult['body'],true);

        //Concatenate the json results
        $result = $APIResult['body'];

        while ($json['status'] <> 'COMPLETE') {
            $APIResult =  APIServices::CallAPI('GET', $APIResult['info']['redirect_url']);
            $json = json_decode($APIResult['result'],true);
           //Concatenate the json results
           $result = $result . $APIResult['body'];
        }
    }

  }

I know curl is able to automatically follow redirect URL's but I don't know if this would be better?
The current solution to concat the json results in the loop works but is it the proper way to combine multiple JSON results in one?
Any other thoughts, concerns?  This is the first time for me to call a RESTful API.

public static function CallAPI($method, $url, $data = false)

Use lowercase first letter for method and function names. callAPI.
Use null rather than false as default value for $data as this more truly represents what is happening (no value has been passed).
Once in your method, you are not doing anything to validate that the parameters passed are as expected. For example, $method and $url should, at a minimum be validated as non-zero length strings.  Perhaps URL also needs to be validated as valid URL and method to be one of set of values you are expecting (i.e. POST,PUT,GET,DELETE,etc.). $data should be validated as null or non-zero length string. If these validations do not pass, you should fail out of this method (perhaps with InvalidArgumentException or similar) and not ever attempt to make a curl call.

$curl = curl_init();

Here, you just assume your curl_init() works.  You should should check for failure and handle appropriately (log error, throw Exception, etc.).
Consider moving your curl initialization sequence into its own static method which would initiate and store the curl handle on the class, making it available to the callAPI method.  There is no reason for you to continually set up and tear down curl handles for each API call.  You probably only need to do this once (changing settings on handle as appropriate.
This centrally stored handle will also be something you will need if you want to be able to optimize your code to making API calls in parallel (more on this below).

switch ($method)
{
    case "POST":
        curl_setopt($curl, CURLOPT_POST, 1);

        if ($data)
            curl_setopt($curl, CURLOPT_POSTFIELDS, $data);
        break;
    case "PUT":
        curl_setopt($curl, CURLOPT_PUT, 1);
        break;
    default:
        if ($data)
            $url = sprintf("%s?%s", $url, http_build_query($data));
}

Rather than having logic here in switch consider actually having distinct get(), post(), put(), delete(), etc. methods on your class.  Each method can make necessary config settings on curl handle and then call the central method which makes the API call and assembles the response data structure.

An incomplete implementation for this might be structured as follows:

public static function get($url, $data) {
    // make setting changes to curl handle here
    // then call call API method, which at this point is just a
    // request/repsonse handler
    return self::callAPI($url, $data);
}

curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);
curl_setopt($curl, CURLOPT_VERBOSE, 1);
curl_setopt($curl, CURLOPT_HEADER, 1);

Consider moving these option settings to common initialization method as noted previously.

// Request json
curl_setopt($curl, CURLOPT_HTTPHEADER, array(
'Accept: application/json'));

Again, probably best set during handle initialization

$APIResult['error'] = curl_error($curl);
$APIResult['info'] = curl_getinfo($curl);
$APIResult['header'] = $header;
$APIResult['body'] = $body;

Consider building this as object rather than associative array, as this really seems to be more of a set of object data rather that a hashmap type of data structure.  You might over time also find a need to add behaviors around these properties such that having your own APIResult class or similar makes sense.  Passing around an object dependency with known properties and behaviors might be preferable to just passing around a data structure.

public function GetHotels($nodeId) {

Again consider naming getHotels() and adding validation for parameter.
This method doesn't return anything!  What is the point of having this public method if it doesn't do anything for the caller?

$node = Node::find($nodeId);

Is $node a meaningful variable name here? $coordinates perhaps?

$apiKey = 'prtl6749387986743898559646983194'; // This is the test key

This should not be defined here in this method.  At a minimum it should be a constant on this class, but would be better still if this was stored in application configuration and not in this code at all.

$checkindate = '2016-12-17';
$checkoutdate = '2016-12-18';
$guests = 2;
$rooms = 1;

Why are these hard-coded?

$url =
'http://partners.api.skyscanner.net/apiservices/hotels/liveprices/v3/'
. $market . '/'. $currency . '/'. $locale . '/'. $entityid . '/'.
$checkindate . '/'. $checkoutdate . '/'. $guests . '/'. $rooms;

Consider having URL base as class constant and having a method to build the full URL

// First call to initiate session, no actual results retrieved
$APIResult = APIServices::CallAPI('GET', $url, array("apiKey" => $apiKey));

If you incorporate other suggestions, this might look like:

$APIResult = APIServices::get($url, array("apiKey" => $apiKey));

$debugResult = $APIResult['body'];

What is purpose of this?

if (!empty($APIResult['info']['redirect_url']) ) {
    $APIResult =  APIServices::CallAPI('GET', $APIResult['info']['redirect_url']);

If you know your API is going to give you redirect, just set curl to follow. Don't write a bunch of nested code to handle these redirects.  What if there are two redirects here?  Your code fails.

    $json = json_decode($APIResult['body'],true);

Why would you call this variable $json when it does not contain JSON, but rather a data structure resulting from JSON deserialization? Call the variable something that is meaningful to what it is actually storing.
What if json_decode() fails (i.e. server sent you something that is improperly formatted)?

    //Concatenate the json results
    $result = $APIResult['body'];

Why are you continuing to work with the raw JSON string here?  You already have a meaningful data structure from the previous line of code.  Concatenating JSON together DOES NOT WORK, as you will get an invalid JSON string.  JSON is ONLY used for data transport, not as a mechanism to build a data structure.  Focus on building an appropriate data structure and then serialize to JSON for delivery.

    while ($json['status'] <> 'COMPLETE') {
        $APIResult =  APIServices::CallAPI('GET', $APIResult['info']['redirect_url']);
        $json = json_decode($APIResult['result'],true);
       //Concatenate the json results
       $result = $result . $APIResult['body'];
    }

And last but not least.  I think you might strongly consider making these API calls in parallel, as that will be the fastest way for this script to operate (though it would consume more memory as a trade-off).  Take a look at curl_multi_* functions (or a library which can better handle your curl operations rather than you coding them manually).  This relates back to earlier point of needing to store curl handle on the class such that each individual call to callAPI method (or the wrapper methods).  To implement parallel calls you would likely need to store curl_multi handle on the class, with each iteration of this loop adding a new curl handle to the multi-handle and returning data from each API call to be aggregated by calling getHotels() method.

I will link you to a lightweight class I had written at one point to manage multi_curl handles geared towards a RESTful use case.  Feel free to use in your application or for inspiration.

https://github.com/mikecbrant/php-rest-client

In general, and without seeing your full class implementation, I am worried that your class is doing too much.  Why would one class need to know all of the following?

How to initiate can configure cURL calls. Ideally this could take a valid cURL wrapper object as a dependency and not have to do anything other than formulating the request and handling the response.
The low-level details of formulating requests against this specific API (i.e. URL naming, redirection handling, etc.)
The logic for deriving localization, geography, etc. based on the request/requestor.
The specifics of performing a hotel search.
The specifics of getting details for an individual hotel


Pascal triangle produced for a particular integer K Please be brutal and let me know what you think of the code below, if written at an interview.

Problem:

  Given an index k, return the kth row of the Pascal's triangle.

  For example, given k = 3, return [1,3,3,1].

  Time it took:
  25 minutes (perfectly working solution)

  Space Complexity:
  O(K)

  Time Complexity:
  O(N2)?

public ArrayList<Integer> getRow(int rowIndex) {
    List<ArrayList<Integer>> allList = new ArrayList<ArrayList<Integer>>();
    ArrayList<Integer> toAdd = new ArrayList<Integer>();
    if(rowIndex==0){
        toAdd.add(1);
        return toAdd;
    }

    if(rowIndex>=1){
        toAdd = new ArrayList<Integer>();
        toAdd.add(1);
        toAdd.add(1);
        allList.add(toAdd);
        if(rowIndex==1){
            return toAdd;
        }
    }
    for(int i=1; i<rowIndex; i++){
        ArrayList<Integer> temp = new ArrayList<Integer>();
        temp.add(1);
        for(int j=1; j<allList.get(i-1).size(); j++){
            temp.add(allList.get(i-1).get(j-1) + allList.get(i-1).get(j));
        }
        temp.add(1);
        allList.add(temp);
    }
    return allList.get(rowIndex-1);
}

Types
Given the description, I would assume that the return value is supposed to be int[] instead of List<Integer>....
... but, you are not using the more general List<Integer> value, but instead the ArrayList<Integer>. Always use the most general class type for your interfaces.
Also, by keeping the data as Integer values, you are doing a lot of boxing, and unboxing in the loops. Really, you should keep the calculations as Java primitives (int), and then box the results if needed.
Conditions
Your code has a special-case for row 0, where it returns [1]. By preference, I recommend not having special cases, although it is a rule I bend often.
Still, after the rowIndex == 0 special case, you then check to see whether it is rowIndex >= 1. This does not make sense because the rowIndex == 0 case returned from the function, so the other condition is useless. Well, not quite useless, it avoids an error condition for negative values. But, the negative-value condition should have been checked at the method start. Basically, it is a useless check. Consider this restructure:
if (rowIndex < 0) {
    throw new IllegalArgumentException("Nevative row");
}
if(rowIndex==0){
    toAdd.add(1);
    return toAdd;
}

toAdd = new ArrayList<Integer>();
toAdd.add(1);
toAdd.add(1);
allList.add(toAdd);
if(rowIndex==1){
    return toAdd;
}

Conclusion
I agree that the complexity is about O(n2), but I know it must be psosible to do it faster. The data types are a problem, but the result looks accurate.
Alternative...
So, I cheated, and looked at wiki, and it has a relatively easy function for calculating the row values for a function. I adapted it here. This is the way I would have done it, if I was able to google the algorithm. I would have used a similar approach to you, but as arrays-of-int instead, if I could not search the algorithm.
public static final int[] pascalRow(final int row) {
    // using same names as wikipedia:
    // http://en.wikipedia.org/wiki/Pascal%27s_triangle#Calculating_a_row_or_diagonal_by_itself
    int n = row + 1;
    int[] ret = new int[n];
    int val = 1;
    final int mid = (n)/2;
    for (int k = 0; k <= mid; k++) {
        ret[k] = val;
        ret[n - 1 - k] = val;
        val = (int)(val * ((n - k - 1) / (double)(k + 1)));
    }
    return ret;
}


Is there a better way to convert two-dimentional array to string? For logging purposes, I need to output a double[,] to the log file. So I need to represent the array as a string.

The following code gets the job done using basic C# 1 features, but I was wondering if there is a more elegant solution using Linq:

private static string OneRowPerLine(double[,] numbers)
{
    var ret = new StringBuilder();
    for(var i=numbers.GetLowerBound(0);i<=numbers.GetUpperBound(0);i++)
    {
        for (var j = numbers.GetLowerBound(1); j <= numbers.GetUpperBound(1); j++)
        {
            if(j>numbers.GetLowerBound(1))
            {
                ret.Append(",");
            }
            ret.Append(numbers[i, j]);
        }
        ret.Append(Environment.NewLine);
    }
    return ret.ToString();
}

First off, I think there may be bugs in your code. Specifically in the dimension parameters of some of the GetLowerBound() parameters. Based on a quick glance, it should be:

private static string OneRowPerLine(double[,] numbers)
{
    var ret = new StringBuilder();
    for(var i=numbers.GetLowerBound(0);i<=numbers.GetUpperBound(0);i++)
    {
        for (var j = numbers.GetLowerBound(1); j <= numbers.GetUpperBound(1); j++)
        {
            if(j>numbers.GetLowerBound(1))
            {
                ret.Append(",");
            }
            ret.Append(numbers[i, j]);
        }
        ret.Append(Environment.NewLine);
    }
    return ret.ToString();
}

I'll update this answer in a moment with some enhancements, but wanted to get this bit out of the way.

EDIT: here's a pair of methods which will do it a little more generically and without LINQ. It does employ generics (<T>) and extension methods (this), one of which is an iterator (yield return):

    private static string OneRowPerLine<T>(this T[,] numbers)
    {
        return string.Join(Environment.NewLine, numbers.FormatOneRow());
    }

    private static IEnumerable<string> FormatOneRow<T>(this T[,] numbers)
    {
        for (var i = numbers.GetLowerBound(0); i <= numbers.GetUpperBound(0); i++)
        {
            var row = new T[numbers.GetLength(1)];

            for (var j = numbers.GetLowerBound(1); j <= numbers.GetUpperBound(1); j++)
            {
                row[j] = numbers[i, j];
            }

            yield return string.Join(",", row);
        }
    }

call like this:

    double[,] stuff = {{1,2}, {3,4}, {5,6}};
    Console.WriteLine(stuff.OneRowPerLine());


Project Euler #1: Sum of Multiples of 3 and 5 below 1000 def prob_1():
    sum_mult=[]  #Create an empty list which will take sum of multiples of 3 and 5
    check_sum=0
    for i in range(1,1000): #Take numbers till 1000
    #if(i)
        if( (i%3)==0 or (i%5)==0 ): #divisor condition
            sum_mult.append(i)

    return sum(sum_mult)    #return sum of list

I am just starting out my journey as a programmer, here is my code and I would love to see any critical feedback and other alternative solutions maybe using some clever hack of using lambda function's etc.

I expect you made a typo.  You don't want (i%35)==0, you want (i%5)==0.

The PEP-8 style guide for Python requires 1 space before and after operators, and after commas.  Use PyLint or equivalent tool to ensure you follow all of the PEP-8 guidelines.

check_sum is unused, and can be omitted.

The brackets around the if( ... ): condition are unnecessary.  This is Python, not C, C++ or Java:

    if (i % 3) == 0 or (i % 5) == 0: #divisor condition

There is no need to create a list just to add up all the numbers after the fact.  You are only using each value once, so you could simply add the numbers up as you find them:

def prob_1():

    sum_of_multiples = 0

    for i in range(1, 1000):  # Take numbers up to but not including 1000
        if (i % 3) == 0 or (i % 5) == 0: #divisor condition
            sum_of_multiples += i

    return sum_of_multiples

You should add """doc_strings""" to your functions:

def prob_1():
    """
    Compute the sum of all the multiples of 3 or 5 below 1000.

    Returns:
        The sum of the multiples of 3 or 5, below 1000.
    """

    sum_of_multiples = 0

    for i in range(1, 1000):  # Take numbers up to but not including 1000
        if (i % 3) == 0 or (i % 5) == 0: #divisor condition
            sum_of_multiples += i

    return sum_of_multiples

You can use list comprehension a generator expression (thanks @Graipher) and the sum(...) function to compute the result, without ever creating the list in memory:

def prob_1():
    """
    Compute the sum of all the multiples of 3 or 5 below 1000.

    Returns:
        The sum of the multiples of 3 or 5, below 1000.
    """

    return sum(i for i in range(1000) if i % 3 == 0 or i % 5 == 0)

You can also solve this problem by hand with a pen, a sheet of paper, a calculator and about 1 minute of your time.  A program is entirely unnecessary.


Locating a PID based on an IP address and port I wrote a simple Python app that is given an IP address and port and the app then finds the PID listening on that IP:PORT.

I would like to hear constructive criticism regarding the bellow code.

import argparse
import sys
import socket
import subprocess
import os

def is_legal_ip(address):
    ip = address.split(":")
    pieces = ip[0].split('.')
    if len(pieces) != 4:
        return False
    try:
        return all(0<=int(p)<256 for p in pieces)
    except ValueError:
        return False

def parse_arguments():
    if len(sys.argv) > 2 or len(sys.argv) == 1:
        print("Please provide a single argument.")
        exit(1)

    parser = argparse.ArgumentParser("Returns a PID listenning on IP:PORT")
    parser.add_argument('ip_port', metavar='IP:PORT', type=str, nargs=1, help='local address')
    args = parser.parse_args()

    if (is_legal_ip(args.ip_port[0]) == False):
        print("Invalid IP address.")
        exit(1)

    return(args.ip_port[0])

def address_to_pid(ip_port):
    devnull = open(os.devnull, 'w')
    output = subprocess.check_output("netstat -nptl".split(), stderr = devnull).splitlines()
    devnull.close()

    for line in output[2::]:
        raw_line = line.split()
        if raw_line[3] == ip_port:
            if raw_line[6] == "-":
                return "No PID listening on address"
            else:
                return raw_line[6].split(r"/")[0]

def main():
    ip_port = parse_arguments()

    print address_to_pid(ip_port)

if __name__ == "__main__": main()

First off, a couple of quick tips:

Follow the PEP8 style guide
Instead of x == False, use not x, for example in if (is_legal_ip(args.ip_port[0]) == False)
Omit unnecessary imports (import socket in this example)
Use spaces around operators, for example all(0 <= int(p) < 256 for p in pieces)
Break lines after colons, for example in if __name__ == "__main__": main()

Leave the job of argument parsing to argparse

No need to work with sys.args yourself.
For example you can simply remove these lines:

if len(sys.argv) > 2 or len(sys.argv) == 1:
    print("Please provide a single argument.")
    exit(1)

The way you specified the ip-port arguments with nargs=1,
argparse will take care of validating the number of command line arguments.

Simplify the argument parsing

You can omit nargs=1.
But in that case,
the value of args.ip_port will not be a list,
but a simple string.

You can also omit the type=str, as that's the default.

Your usage would become simpler:

parser.add_argument('ip_port', metavar='IP:PORT', help='local address')
args = parser.parse_args()

if not is_legal_ip(args.ip_port):
    parser.error("Invalid IP address.")

return args.ip_port

Use two arguments instead of one

It's awkward that use ip:port,
the splitting at multiple places is just noise.
It will be simpler and cleaner to use two arguments instead of one:

parser.add_argument('ip', help='local address')
parser.add_argument('port', type=int, help='local port')
args = parser.parse_args()

if not is_legal_ip(args.ip):
    parser.error("Invalid IP address.")

return args.ip, args.port

Notice that this way argparse can take care of validating the port number.

Then in your main function, you can get the ip address and port pair like this:

ip, port = parse_arguments()

Benefit from the error reporting features of argparse

Instead of this:

if not is_legal_ip(args.ip_port):
    print("Invalid IP address.")
    exit(1)

I suggest to use this way:

if not is_legal_ip(args.ip_port):
    parser.error("Invalid IP address.")

argparse will print a usage help along with the error message,
and also exit.

A subtlety here is that argparse will exit with code 2 in this case,
which is common in case of invalid parameters of UNIX commands.

Working with file handles

When working with files, use with:

with open(os.devnull, 'w') as devnull:
    output = subprocess.check_output("netstat -nl".split(), stderr=devnull).splitlines()

This makes devnull.close() unnecessary, Python will call it when exiting the block.


Getting object when is ready I have a module, that fetches some object. Then other modules can use this object. Here is a little example of this task:

var Structure = (function(){

    var tree = null,

        df = $.ajax({
            'url'       : '/some/path',
            'type'      : 'POST',
            'data'      : { some : 'data' },
            'dataType'  : 'json'
        }),

        ready = function(cb){
            if (tree) {
                cb(tree);
                return
            }

            $.when(df).then(function(data){
                tree = TreeHandler.create(data);
                cb(tree);
            });
        };

    return {
        df      : df,
        ready   : ready
    };

})();

// Usage:
Structure.ready(function(data){
     console.log (data);
     // some actions with data
});

Am I right on doing this? How can this be made better?

I'm repeating @RoToRa for the sake putting it in answer form for beta-stats:

Your syntax is fine. Without knowing more of the problem domain, it does seem a little unnecessary.  Why not use callbacks on your $.ajax() call?


Finding the multiple of 3 and 5 Write a program that prints the numbers from 1 to 100.
But for multiples of three print “Fizz” instead of the number
and for the multiples of five print “Buzz”. For numbers which are
multiples of both three and five print “FizzBuzz”.

Is this the best way of doing this?

def fizz_buzz(num):
    if num%3==0 and num%5==0:
        return 'FizzBuzz'

    elif num % 3 == 0:
        return 'Fizz'

    elif num % 5==0:
        return 'Buzz'
    else:
        return num

for n in range(1,100):
    print(fizz_buzz(n))

General feedback

I will try to go over some points that can be useful to note. Firstly there are several things I like about your code. Firstly it is very readable. Secondly I like that you split your logic. You also split finding the string and printing it. This is good. With this being said there are always things which could and should be improved

Semantics

You should use the if __name__ == "__main__": module in your answer.

def fizz_buzz(num):
    if num%3==0 and num%5==0:
        return 'FizzBuzz'

    elif num % 3 == 0:
        return 'Fizz'

    elif num % 5==0:
        return 'Buzz'
    else:
        return num

if __name__ == "__main__":

    for n in range(1,100):
        print(fizz_buzz(n))

Which makes your code reusable for later. Eg you can call functions from your file in other programs.
Your else clause at the end of the code is useless. You could have written

    elif num % 5==0:
        return 'Buzz'
    return num

Alternatives

One problem with your code is that you have multiple exit points. Now this is not something to sweat too hard over, and it is not a goal to always try have a single exit. However it can be easier to debug a code with fewer exit points. This is of course much more relevant in longer and more complex code. Though it is a good thing to always have in mind. One way to do this is to define a new variable string

def fizz_buzz(num):
    string = ''
    if num%3==0 and num%5==0:
        string = 'FizzBuzz'

    elif num % 3 == 0:
        string = 'Fizz'

    elif num % 5==0:
        string = 'Buzz'

    if string:
       return string
    return num

The code now only has two exit points however it can still be improved. One key point is that if a number is divisible by 3 and 5, it is divisible by 15. So we can gradually build the string, like shown below

def fizz_buzz(num):
    string = ''

    if num % 3 == 0:
        string += 'Fizz'

    if num % 5==0:
        string += 'Buzz'

    if string:
       return string
    return num

As a last point the return statement could be written using a terniary conditional operator

return string if string else n

Which combines the two exit points into a single one. To sumarize

def fizz_buzz(num):
    string = ''
    if num % 3==0: string +='Fizz'
    if num % 5==0: string +='Buzz'
    return string if string else num

if __name__ == "__main__":

    for n in range(1, 100):
        print(fizz_buzz(n))

Closing comments

Python has a style guide PEP 8 which explains in excruciating detail how to structure your code. I whole heartily recommend skimming through it and follow it.

The problem FizzBuzz is very, very simple. It can be solved in a number of ways using just a simple line. Syb0rg, showed one way to write this code

for i in range(1,101): print("Fizz"*(i%3==0) + "Buzz"*(i%5==0) or i)

You can even shorten this into

i=0;exec"print i%3/2*'Fizz'+i%5/4*'Buzz'or-~i;i+=1;"*100

Using some cryptic pythonic voodoo. However as I said in the introductory I like your code, because it is easy to understand. Almost always it is better to have clear, readable code than cryptic code which is a few lines shorter. This of course disregards any speed improvements and such


Country Guessing Game I'm learning React and I made this country guessing game. What improvements/suggestions do you have for it?
Preview it here
import React, {Component} from 'react';
import './Country.css';

class Country extends Component {
    constructor(props) {
        super(props);
        this.state = {
            countries: [],
            randomCountry: {},
            randomOptions: [],
            userIsWin: '',
            disableFieldset: false,
            goodGuess: 0,
            bgColor: {backgroundColor: 'white'}
        }
        this.getRandomCountry = this.getRandomCountry.bind(this);
        this.checkWin = this.checkWin.bind(this);
    }

componentDidMount() {
    const apiUrl = "https://restcountries.eu/rest/v2/all";
    fetch(apiUrl)
    .then(data => data.json())
    .then(countries => this.setState({countries}))
    .then(this.getRandomCountry)
}

getRandomCountry() {
    const random = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    const randomOpt1 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    const randomOpt2 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    const randomOpt3 = this.state.countries[Math.floor(Math.random()*this.state.countries.length)];
    const randomOptions = [random.name, randomOpt1.name, randomOpt2.name, randomOpt3.name];
    randomOptions.sort(() => { return 0.5 - Math.random() });
    this.setState({
        randomCountry: random,
        randomOptions: randomOptions,
        userIsWin: '',
        disableFieldset: false
    })
}

checkWin(e) {
    this.setState({
        disableFieldset: true
    })
    const winCountry = this.state.randomCountry.name;
    const userGuess = e.target.value;
    if (winCountry === userGuess) {
        this.setState({
            userIsWin: 'Win',
            goodGuess: this.state.goodGuess + 1,
            bgColor: {backgroundColor: '#81C784'}
        })
    } else {
        this.setState({
            userIsWin: 'Lose',
            bgColor: {backgroundColor: '#FF8A65'}
        })
    }
    setTimeout(()=>{
        this.getRandomCountry();
        this.setState({
            userIsWin: '',
            disableFieldset: false,
            bgColor: {backgroundColor: 'white'}
        })
        console.log(e.target)
    }, 2000)

}

render() {
    return (

Country Guessing Game

                <button className="rnd mui-btn mui-btn--raised" onClick={this.getRandomCountry}>Random</button>

{this.state.userIsWin === 'Win' ? 'You guess right! ' : ''}
                    {this.state.userIsWin === 'Lose' ? 'You guess wrong. ' : ''}
                    Score:{this.state.goodGuess}

            <fieldset disabled={this.state.disableFieldset}>
                <form onClick={e => this.checkWin(e)}>
                    <button className="mui-btn mui-btn--raised" value={this.state.randomOptions[0]}>{this.state.randomOptions[0]}</button>
                    <button className="mui-btn mui-btn--raised" value={this.state.randomOptions[1]}>{this.state.randomOptions[1]}</button>
                    <button className="mui-btn mui-btn--raised" value={this.state.randomOptions[2]}>{this.state.randomOptions[2]}</button>
                    <button className="mui-btn mui-btn--raised" value={this.state.randomOptions[3]}>{this.state.randomOptions[3]}</button>
                </form>
            </fieldset>

    )
}
}

export default Country;

The screen design looks very nice. It is minimalistic, yet pleasing. It almost perfectly fits on the small screen of my phone (only sometimes, I have to scroll down to see all the answer buttons). The flags are nicely big to explore the fine graphical details. To appreciate the shape of the Nepal flag, there should be no border around it, and the background color could be set to something other than white, since that is used in some of the flags.

Bug: never sort using a random comparator. Instead, shuffle the array. This has been answered often already, just search for the above terms.

When reading the code aloud, "a country is a component" doesn't make sense. A country is something having a name and a flag, so the class should better be named CountryComponent.

Most JavaScript code can be rewritten to not use the this keyword or the bind function, which leads to clearer and simpler code. See this code review for an example.

In the setTimeout call, I prefer to extract the first argument into a named function, so that the setTimeout appears closer to the 2000:

function action() {}

setTimeout(action, 2000);

There should be a space between the label "Score:" and the score number.

When I play the quiz, my total score always stays at 0, so there must be something wrong. After each click on one of the answer buttons, the page becomes green or orange for about a second and is then reloaded, which explains that the score is reset and that the screen flickers. On the other hand, the Random button works fine, so there must be something that differs between those. (Tested with Firefox on Android.)


Checking if a booking can be cancelled or not based on three conditions I have 2 if-else statements that checks whether a booking(object mb) can be cancelled or not. If the current user is an admin or department admin, the booking can be cancelled. However, if the booking has been cancelled, of course, it absolutely cannot be cancelled by anyone.

What I am worried about if the if-else statements can be improved further. Any help would be useful.

foreach (var ub in usersBookings)
{
    MyBookingModel mb = new MyBookingModel();
    mb.bookId = ub.bookingId.ToString() + "+" + ub.startDateTime.ToString();
    mb.bookDate = ub.startDateTime.ToString("dd/MM/yyyy");
    mb.bookSlot = ub.startDateTime.ToString("HH:mm") + " - " + ub.endDateTime.ToString("HH:mm");
    mb.facilityName = ub.facilityName;
    mb.status = ub.bookingStatus.Trim();
    mb.startDateTime = ub.startDateTime.ToString("yyyy-MM-dd HH:mm:00");

    mb.canCancel = false;

    if (mb.status == "Cancelled")
    {
        //absolute. if mb.status == "Cancelled", mb.canCancel is "false"
        mb.canCancel = false;
        goto add_object;
    }

    if (deptAdmin == true || isAdmin == true)
    {
        mb.canCancel = true;
    }

add_object:
    myBookings.Add(mb);
}

You can simply assign the if conditions to the canCancel property of the MyBookingModel like so

mb.canCancel = mb.status != "Cancelled" && ( deptAdmin || isAdmin );

While we are at the properties of the MyBookingModel: Properties should be named using PascalCase casing.

canCancel -> CanCancel  and so on.

and while we are at naming things, why is it MyBookingModel and not just BookingModel ? What does the My buy you ?


Custom Kit Form Validation I have an HTML form where I need to do the following

when value of Select Box changes to initial "Not Selected" value (where values of the option is 0), I am to highlight the select box, when it's non-0, restore the original coloring of the select
when I submit the form, and the Select box value is 0, do not submit the form, and highlight the box

Doing all of this left me with code duplication.  See highlight and restore

How do I get rid of the duplication?  Can I?

highlight("kit_model");
$('#kit_submit').prop("disabled", true);

$("#kit_model").change(function() {

    if ($("#kit_model").val() == 0)
    {
        highlight("kit_model");
        $('#kit_submit').prop("disabled", true);
    }
    else
    {
        restore("kit_model");
        $('#kit_submit').prop("disabled", false);
    }

});
$("#custom_kit_form").submit(function() {
    if ($("#kit_model").val() == 0)
    {
        highlight("kit_model");
        return false;
    }

    $.post(
            "web.php
            $("#custom_kit_form").serialize(),
            $("#main").load('main.php')
         );

    return false;
});

Form

<form action="" id="custom_kit_form" method="post" name="custom_kit_form">

            FS Kit

            <select id="kit_model" name="kit_model">
                    <option value="0">-- SELECT --</option>

                    <option value="573">TUBING</option>
            </select>

            <input id="kit_submit" name="kit_submit" type="submit" value="Save">

</form>

Way too many calls to $

This pops up way too much in your code (or something like this):

$("#kit_model")

Normally, document.getElementById itself can be slow. However, this jQuery function is even slower because, not only does it have to traverse the DOM in search for a single element, but it also has to wrap it sugar another object for access to more methods/properties.

It'd be better to reduce your calls to this function. Instead, store the return from a single call in a variable and use that:

var kitModel = $("#kit_model");

Do this for the other elements that you grab more than once, too.

If you are in control of the highlight and restore functions, change them so they can accept the element itself through parameters. This will make your function more flexible, and also allows you to just continue to use this variable you have.

Tables for layout

If I understand correctly, you are using tables in your HTML code to correctly position your DOM elements. This is bad/old practice. You should instead be using CSS rules to position elements.

Tables are for data that belongs in tables.

Initialize attributes in HTML

Here, you are initializing your submit button to be disabled in your JavaScript code:

$('#kit_submit').prop("disabled", true);

Instead, you should simply just slap a disabled onto the element itself.

Unnecessary condition

Will this condition ever pass?

$("#custom_kit_form").submit(function() {
    if ($("#kit_model").val() == 0)

The submit button can't be clicked in the first place if the value of the kit model is 0 because the change listener will disable the button.

You can safely remove this part of the submit listener.


Converting units of volume I am creating a volume converter. It allows users to enter a value and select the unit to convert from and the one it will be converted into. It uses a <select> tag for user input selection.

The <select> looks like this:

<select name="from">
  <option value=0>--select--</option>
  <option value=1>cm3</option>
  <option value=2>m3</option>
  <option value=3>Feet3</option>
  <option value=4>Gallons(UK)</option>
  <option value=5>Gallons(US)</option>
  <option value=6>Inches3</option>
  <option value=7>Litres</option>
  <option value=8>Yards3</option>
  <option value=9>Quart(UK)</option>
</select>

And there is another one similar to this with the name "to".

After that I did the following in PHP:

$from=$_POST['from'];
$to=$_POST['to'];

function assign($from,$val)
      {
          switch ($from)
          {
              case 1:
                  $fromu="cm3";
                  $cm=1;
                  $me=0.000001;
                  $ft=0.000035315;
                  $gk=0.000219969;
                  $gs=0.000264172;
                  $in=0.061024;
                  $li=0.001;
                  $ya=0.000001308;
                  $qtk=0.000879877;
                  break;
               case 2:
                    $fromu="m3";
                    $cm=1000000;
                    $me=1;
                    $ft=35.32;
                    $gk=220;
                    $gs=264;
                    $in=61024;
                    $li=1000;
                    $ya=1.308;
                    $qtk=879.877;
                    break;

There are 7 more cases based on the html select "from" and each holds a value of conversion for each unit relative to the selection made.

It ends with:

        echo "

 ",$val," ",$fromu," equivalent is

 ",(double)($val*$cm)," CentiMeter3

 ",(double)($val*$me)," Meter3

 ",(double)($val*$ft)," Feet3

 ",(double)($val*$gk)," Gallons(UK)

 ",(double)($val*$gs)," Gallons(US)

 ",(double)($val*$in)," Inches3

 ",(double)($val*$li)," Litres

 ",(double)($val*$ya)," Yards3

 ",(double)($val*$qtk)," Quart(UK)

";
   }
   assign($from,$val);
}

How do I optimize this in a way that I can save all the conversion values (I'm thinking possibly in an associative array) and still be able to display one conversion based on user selection in the "to" select instead of outputting all of the converted units as seen above?

Convert the from to a single unit, and convert that single unit to to. You don't have to compute conversions for every single pair.


Creating a secure and efficient DB connection and queries (using one class) Currently, I'm working with a PHP database connection class that is in one single file with a construct. Every time I need this connection, I instantiate the class and the construct creates the connection with only having to modify the user, password, DBname and host.

It works fine and I haven't faced any problems with this class. Although I want to start taking security seriously in my web applications, I've been looking for a way to implement security (SQL Inj, XSS, DoS, etc.) and use updated functions, prepared statements, escaping, etc. My problem is that I haven't found a single answer about this exactly because I don't call the mysqli_connect function in index.php. I just create the object for the Connection class and use it.

<?php
    class Connection {
     private $connection;

     private $host='localhost';
     private $user='user';
     private $pass='pass';
     private $database='dbname';
     private $n=0;

    function __construct() {
        $this->connection = mysqli_connect($this->host, $this->user, $this->pass, $this->database);

        mysqli_query($this->connection,"SET NAMES 'utf8'");
    }

    function sql($sql) {
        if (is_array($sql)) {
            foreach ($sql as $s) {
                $res = mysql_query($s);
                if (mysql_errno()) echo mysql_error().'
';
                $this->n++;
            }
        } else {
            $res = mysqli_query($this->connection,$sql);
            if (mysql_errno()) echo mysql_error().'
';
            $this->n++;
        }
        return $res;
    }

    function __destruct() {
        if (is_resource($this->connection) )
        mysql_close($this->connection);
    }

    function getLink() {
        return $this->connection;
    }
}

So if I want to Create, Read, Update, Delete Something I do this in another file: User.php (Which i want to use the new method I'm asking for and use prepared statements for the Queries here and escape functions)

<?php
  // Declare a variable
     private $user;

  // Setter
     function setUser($val){
        $this->user = $val;
     }

  // Create Function to CreateUser
     function CreateUser(){
        $con = new Connection();
        $sql = "INSERT INTO user (DATA) VALUES(DATA)";
        $con->sql($sql);
     }

  // Same for all CRUD i just change the Query of course.

?>

<?php
  // At the index.php i do this:
  include_once 'User.php';

  $user = new User();

  if(isset($_POST['createUserButton'])){
     // $The variable i set in my User Class
     $user = $_POST['user'];

     // Then the Setter
     $user->setUser($user);

     // Then execute the Function
     $user->CreateUser();
  }
?>

I'm trying to translate this class using:

New versions of functions
Using try/catch
Security

I'm just trying to have as many points of view from more experienced developers. I'm a few months old at this and I'm trying to look at different methods to code so I can create my own.

You say that you use prepared statements, but your code doesn't actually reflect that.

sql just executes any query, without binding or escaping anything. You really need to use prepared statements though. Your current sql function isn't usable in practice and it's pure existence will likely lead to vulnerabilities (other programmers will likely use it as it exists, and it can't be used securely).

Apart from that:

don't mix different APIs. Currently, you use mysqli and mysql.
don't use mysql, it's been deprecated for years and has finally been removed in PHP7.
your formatting is off. Use correct indentation, always use curly brackets, use newlines for statements (eg after an if), and use spaces around =. Any IDE can fix these things for you.
be consistent with your naming. Your functions should all start with a lower-case character.
Also, same things should be named the same. So if you have a field called connection, the method retrieving it should be getConnection, not getLink.
Also try to be more explicit with your variable names and don't shorten names. val should be value (but really, it should be user or username), res should be result, n should be queryCount, and so on.
It's very unclear to me what n is used for. I would remove it, or document it.
sql isn't really useful. It's just a wrapper around mysql_query which automatically echoes errors (meaning that the calling code has no possibility to recover or show a different error message; throw an exception instead), and provides the possibility to execute multiple queries, which doesn't actually seem needed.
You don't want to create a new database connection for each method (for performance reasons), so instead, create it once and then pass the db object to methods that need it.


Efficient date-based lookup function in R Right now this function takes an excessively long time to run, please let me know how to make it more efficient.

lunarCal is 144x7 and the dput for it is here.

Here's the code:

if(!require(pacman)) install.packages("pacman")
pacman::p_load(lubridate)
cat("Please make sure that birthday inputs are in YYYY-MM-DD format")

lunarCal             <- readRDS("data/lunarCal.RDS")

eastern.profile <- function(birthday){
    good                 <- is.na.POSIXlt(birthday)
    if(!(good)){birthday <- strptime(birthday, format = "%Y-%m-%d", tz = "Asia/Chongqing")}
    ind                  <- (lunarCal$START_DATE <= birthday) & (lunarCal$END_DATE >= birthday)
    temp                 <- subset(lunarCal, ind)
    return(temp)
}

Here's an example of its non-vectorized usage:

eastern.profile("1975-01-01")
   YEAR START_DATE   END_DATE ANIMAL ELEMENT ASPECT                                   PERSONALITY
75 1974 1974-01-23 1975-02-10  Tiger    Wood      + Imagination, creativity, idealism, compassion

When I run it I'm using sapply but it's still slow. I've got a data set with 250,000 dates I'm applying it to and it took over 15 minutes.
We can simulate the data set to run this function on with

dat <- as.data.frame(seq(as.Date("1900/1/1"), as.Date("2040/1/1"), "days"))
dat <- rbind(dat,dat,dat,dat)

Have a look at the function findInterval. It is vectorized so it can treat all your dates in a single (fast) call. Your code should look something like this:

eastern.profile <- function(birthday) {
   bad <- !is.na.POSIXlt(birthday)
   birthday[bad] <- strptime(birthday[bad], format = "%Y-%m-%d", tz = "Asia/Chongqing")
   ind <- findInterval(birthday, lunarCal$START_DATE)
   cbind(birthday = birthday, lunarCal[ind, ])
}

Note that I was not able to test because of some issues I had with your data. So it may need some tweaks.


Function to combine arrays of associative arrays I recently create a function to combine some arrays of associative arrays in a single array in response to a stackoverflow question, where you can found more details about this.

The function works as expected, but I think their description is very obscure. In addition, i choose constant T_OBJECT_CAST as option to convert each rows as object: someone can suggest a better predefined constants?

Goal:

As requested from another user (edited):

  I've these two arrays:

  1) [0=>['title1'=>'Title 1'],  1=>['title2'=>'Title 2']];

  2) [0=>['contn1'=>'Content 1'],1=>['contn2'=>'Content 2']];

  How can I combine/merge them that they look like this?

  [

  0=>['title1'=>'Title 1','contn1'=>'Content 1'],

  1=>['title2'=>'Title 2','contn2'=>'Content 2']

  ]

Explanation:

(Globish here, i'm sorry)

To allow not predetermined arguments, I don't format function as multiArrayCombine( $arg1, $arg2, ... ), I use instead the func_get_args() function, that "allow user-defined functions to accept variable-length argument lists".

First of all, I check if the last argument is the predefined constant T_OBJECT_CAST: if it is, I set $asObject to True, then I pop-it off the end of arguments array; now in the $args variable I have an array with each passed arrays.

Next step: I retrieve the max key value of all passed arrays; i choose this way instead of more comfortable foreach( $array1 as $row ) to avoid to omit values if one of the other arrays have more rows than the first. Eventually not numeric keys are omitted.

Then, the main loop: I process each row of originals arrays and I add their keys and values to row that will added to returned array. If there are duplicated keys, only the last is returned.

After processing each array, i add the obtained row (converted to object if this option is passed) to returning array.

The function:

/*   Groups passed arrays in an array of associative arrays with same keys and values
 *
 *   @example          $array1 = array( array('a'=>'val1'),array('a'=>'val2') );
 *                     $array2 = array( array('b'=>'val3'),array('b'=>'val4') );
 *                     $array3 = array( array('c'=>'val5'),array(),array('c'=>'val6') );
 *                     multiArrayCombine( $array1, $array2, $array3 );
 *                     return: array
 *                     (
 *                        0 => array('a'=>'val1','b'=>'val3','c'=>'val5'),
 *                        1 => array('a'=>'val2','b'=>'val4'),
 *                        2 => array('c'=>'val6')
 *                     )
 *
 *   @param   array    $array1[, $array2[, $array3...]]
 *
 *   @option  const    T_OBJECT_CAST cast returned assoc arrays as stdObject
 *
 *   @return  array
 */
function multiArrayCombine()
{
    /* Get all passed parameters and T_OBJECT_CAST option: */
    $args     = func_get_args();
    $asObject = ( T_OBJECT_CAST == $args[count($args)-1] );
    if( $asObject ) array_pop( $args );

    $retval = array();          # Init array to be returned

    /* Retrieve highest passed arrays key: */
    $max = 0;
    foreach( $args as $array ) $max = max( $max, max( array_keys($array) ) );

    /* Loop for each arrays key: */
    for( $i=0; $i<=$max; $i++ )
    {
        /* Init associative array to add:  */
        $add = array();

        /* Process actual key ($i) of each passed array:  */
        foreach( $args as $array )
        {
            /* If the key ($i) exists, add  each passed array:  */
            if( is_array($array[$i]) )
            {
                foreach( $array[$i] as $key => $val )
                { $add[$key] = $val; }
            }
        }

        /* Add the obtained associative array to return array */
        if( $asObject ) $retval[] = (object) $add;
        else            $retval[] = $add;
    }

    return $retval;
}

I know that the question is already answered with cFreed answer but I want show another approach.

While I was finding if there are some function that do that I found this on php.net

function array_merge_recursive_distinct ( array &$array1, array &$array2 )
{
  $merged = $array1;

  foreach ( $array2 as $key => &$value )
  {
    if ( is_array ( $value ) && isset ( $merged [$key] ) && is_array ( $merged [$key] ) )
    {
      $merged [$key] = array_merge_recursive_distinct ( $merged [$key], $value );
    }
    else
    {
      $merged [$key] = $value;
    }
  }

  return $merged;
}

basically is the same that you want but only for two args so...
I tried to do this
pick args and send to that function in pairs

so, I write this. I tried to keep the soul of your code intact so you will see  $asObject = (T_OBJECT_CAST == $args[count($args)-1]);  and also     if($asObject) array_pop($args);

function array_merge_recursive_unique_keys(){
    $args     = func_get_args();
    $asObject = (T_OBJECT_CAST == $args[count($args)-1]);
    if($asObject) array_pop($args);

    $ret = $args;

    if(count($args)>0){
        $partial = current($args);

        for($i = 1; $i< count($args); $i++){
            if(is_array($args[$i]) && is_array($partial)){
                $partial = array_merge_recursive_distinct($partial, $args[$i]);
            }
        }

        if($asObject) {
            $ret = json_decode (json_encode ($partial), FALSE);
        }else{
            $ret = $partial;
        }

    }

    return $ret;
}


Snakes and Ladders using a magic die This is a problem from here where you're given a magic die, and the objective is to find the minimum number of dice throws (hops) to go from the start point to the end of the board, and win the game.

I am representing the board as a matrix as follows. If the value at a cell is 0, you could go to any of the next 6 places using your magic die. If the value at a cell is non-zero, you compulsorily have to go to the cell that is represented by the value (think there is a snake or ladder at that cell which is taking you to the cell represented by the value). For example, if you land at (0,1), you are forced to go to 11, which is (2,1).

int[][] a = {
                     {0,11,0,0,0},
                     {0,0,0,0,0},
                     {0,0,1,23,0},
                     {0,0,4,0,0},
                     {10,0,0,0,0}
        };

Background

I have implemented this using Depth First Search / Recursion. Someone who reviewed my code said this solution is incorrect, and this problem is not a good candidate for Depth First Search / Recursion, and can only be done by using Breadth First Search, the reason being that, there are cells which are interdependent on each other. For example, this is a valid path, (0,0) -> (0,1) -> (2,1) -> (2,2) -> (0,1). However, you are trying to find the minimum number of hops from each cell, and here, (0,1) depends on (2,1) which in turn depends on (0,1).

I am not completely sure this is a valid argument, since if you go from (0,1) to (2,1), there is no reason, to go back to (0,1) since it anyways cannot be the shortest path, since if you go back, you're basically going back to a point you previously were in except that you have made two more hops. So, this is not useful, and I have handled this by using a visited[][] state to capture that.

public class SnakesAndLadders {

    /**
     * @param args
     */
    public static void main( String[] args ) {
        // TODO Auto-generated method stub
        int[][] a = {
                     {0,11,0,0,0},
                     {0,0,0,0,0},
                     {0,0,1,23,0},
                     {0,0,4,0,0},
                     {10,0,0,0,0}
        };
        boolean[][] visited = new boolean[a.length][a[0].length];

        System.out.println( shortestPath( a, 0, 0, visited ) );
    }

    static int shortestPath(int[][] a, int i, int j, boolean[][] visited)
    {
        if(i >= a.length || j >= a[0].length)
            return Integer.MAX_VALUE-100;

        if( i == a.length-1 && j == a[0].length-1)
            return 0;

        if(visited[i][j] == true )
            return Integer.MAX_VALUE-100;

        visited[i][j] = true;
        int min = Integer.MAX_VALUE-100;
        if(a[i][j] == 0)
        {
            int pos = i*a[0].length + j;
            for(int k = pos+1;k<=pos+6;k++)
            {
                min = Math.min( min, shortestPath( a, k/a[0].length, k%a[0].length, visited ) + 1) ;
            }

        }
        else
            return shortestPath( a, a[i][j]/a[0].length, a[i][j]%a[0].length, visited ) + 1;

        visited[i][j] = false;
        return min;
    }
}

Please review my code, and give me your thoughts on whether this would work, or if depth first search cannot be used for this class of problems where you could go back to a previous recursion state.

I don't believe your depth-first approach is incorrect, as far as I can tell your visited variable does its job. However, there is room for improvement.

General improvents to readability
Firstly, this game's board can be represented by a linear tape of cells, rather than a two-dimensional grid. Using a one-dimensional array greatly simplifies the code. Only one position variable will be passed around instead of two. Additionally, the translation steps between coordinate systems that you're currently making (e.g.: int pos = i*a[0].length + j;) are eliminated.

Likely bugs
The aim is to count the minimum number of dice rolls, but you're also incrementing min when you use a snake or ladder. I think you should remove the + 1 in the else branch.

You're currently using the magic number 100, here: Integer.MAX_VALUE-100. There is no explanation as to where this 100 comes from, why it needs to be exactly that number. I assume it's to prevent interger wrapping when taking a sum. However, this exposes you to bugs. What if you use this program on a larger board that has paths longer than a 100 rolls? Your program now thinks the most optimum route is done in negative two billion rolls.

The current implementation assigns a special meaning to the value zero (no snake or ladder). However, position zero is a valid cell. This means that there is no way for a snake to point towards the cell at zero! One workaround would be to change the default value to -1.

Nitpicks
I recommend always using brackets with conditional blocks, even if there's just one line. Read this for more information.


CodeChef Fusing Weapons in a circular list I am currently trying to solve this problem on Codechef:

  Before the start of each stage, N weapons appear on the screen in circular order. Each weapon has an integer associated with it, which represents its level. The chef can choose two adjacent weapons of the same level and fuse them into a single weapon of level A+1, where A is the level of the weapons before fusing. Both the old weapons will disappear and the new weapon will be placed in the place of the old weapons, shrinking the circle.

  Chef can fuse as many times as he wants, and in each stage, he wants to make a weapon with as high a level as possible. Each stage is independent of other stages.

  Please help Chef by figuring out the maximum level of a weapon that he can get in each stage.

However, my code seems to exceed the time limit. Can someone please tell me how to optimize this code to prevent it from exceeding the time limit?

#include <iostream>

using namespace std;

class Set
{
public:
    int data[200000];
    int length;
};

int findMax(int data[], int size)
{
    int max = data[0];
    for (int i = 1; i < size; i++)
    {
        if (max < data[i])
        {
            max = data[i];
        }
    }

    return max;
}

void mergeData(int data[], int &size)
{
    for (int i = 0; i < size - 1; i++)
    {
        for (int j = i + 1; j < size; j++)
        {
            if (data[i] == data[j])
            {
                data[i]++;

                for (int k = j; k < size - 1; k++)
                {
                    data[k] = data[k + 1];
                }
                size--;

                mergeData(data, size);
            }
        }
    }
}

//Main function
int main()
{
    int numSets;
    cin >> numSets;

    Set* sets = new Set[100];
    for (int i = 0; i < numSets; i++)
    {
        cin >> sets[i].length;
        for (int j = 0; j < sets[i].length; j++)
        {
            cin >> sets[i].data[j];
        }
    }

    for (int i = 0; i < numSets; i++)
    {
        mergeData(sets[i].data, sets[i].length);
        cout << findMax(sets[i].data, sets[i].length) << endl;
    }

    return 0;
}

Looking at your merge_data() function, you'll notice that as soon as it finds 2 matching values, it combines them and recurses. What happens during and after the recursion? Let's take a look.

Suppose we have this set:

3,8,4,2,2,7,14

We first get 3. Next we start walking the rest of the list. We compare against 8. No match. We compare against 4. No match. We do this until we run out of things to compare. Then we increment i. We check the rest of the array. Etc. Eventually we come to the 2. We compare to the next value and there is a match! We combine them and recurse. What happens now? We start by comparing 3 to 8, which we've already done. Then again with 4, which we've also already done. So we're doing a bunch of work over and over again.

Then, eventually we return from our recursion. Now we go on to process the rest of the array. There's 2 problems here: 1) At this point, the recursion we're returning from has already processed the rest of the array, and 2) the array is no longer the same size, but we're going to process to the end of the original array anyway!

I haven't worked out a full solution, but hopefully the above is enough to help you see what's wrong with your current solution.


Project Euler LCM of 1..20, with some guesswork I'm doing a Euler challenge that is stating to find the smallest positive number that is divisible (without a remainder) by all numbers from 1 to 20. So I did it however, I don't like the fact that I have to guess up to which number the while loop have to go (var s.... s < 303000000) and I guess there are too many variables. I'm sure I could get rid of 's'.

var s = 0;
var n = 1;

while (s < 303000000) {
        for (var i=1;i<21;i++) {
                if (n % i != 0) {
                        break;
                }
                if (i == 20) {
                        console.log(n);
                }
        }
        n += 1;
        s += 1;
}

The below formula can be used to get the LCM of two numbers. See Reduction by the greatest common divisor section in Wikipedia

               a * b
lcm(a, b) = ―――――――――――
             gcd(a, b)

GCD can be calculated by using the Euclidean AlgorithmSee Using Euclid's algorithm section in Wikipedia.

gcd(a, b) = gcd(b, a % b)

Using these two formulas, the LCM can be calculated as follow

console.time('new');

function gcd(a, b) {
    while (b !== 0) {
        var temp = a;
        a = b;
        b = temp % b;

        // Can be written in ES6 as
        // [a, b] = [b, a % b];
    }

    return a;
}

function lcm(a, b) {
    return a * b / gcd(a, b);
}
var range = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20];
console.log(range.reduce(lcm, 1));
console.timeEnd('new');

This is very efficient than using while and for, see this JsFiddle demo.

Note: This answer is inspired by this and this posts.