Managing ZK Tabpanels Scenario:

I am working on  a Java/ZK Subscriber Management application. There are 2 tabs: Basic Info and STB-VC. The first one for creating a new subscriber and the second one for assigning STB/VC(Set-top box and viewing card).

I am trying to achieve/ensure Someone can only click/use the second tabpanel only after creating a new subscriber and not before.

I have the following code which does that. Is there a better way to achieve the task?

Subscriber.zul

    <window id="main" title="" border="normal"
        apply="com.sss.composer.SubscriberComposer" width="100%"
        height="100%">

            <image id="home" src="/img/home.png" width="30px" />
            <image id="logout" src="/img/logout.png" width="30px" />

        <tabbox id="tb">
            <tabs id="tabs">
                <tab id="A" label="Basic Info" />
                <tab id="B" label="STB-VC" />
            </tabs>
            <tabpanels>
                <tabpanel id="basic">

                    <include id="inc1" src="basic.zul"></include>
                </tabpanel>
                <tabpanel id="stbvc">
                    <include id="inc2"></include>
                </tabpanel>
                <tabpanel>

                </tabpanel>
                <tabpanel>

                <tabpanel>

                </tabpanel>
            </tabpanels>
        </tabbox>
    </window>

SubscriberComposer

public void onSelect$tb(ForwardEvent fe) {

        try {
            Object o = fe.getOrigin().getTarget();

            if (o instanceof Tab) {

                Tab t = (Tab) o;

                Tabpanel tp = t.getLinkedPanel();

                tp.invalidate();

                if ((tp.getId().equals("stbvc") && (Sessions.getCurrent()
                        .getAttribute("subscriber") == null))) {

                    Messagebox.show(
                            "Create Subscriber first,then assign STB/VC!",
                            "Warning", Messagebox.OK, Messagebox.INFORMATION);

                    tb.setSelectedPanel(basic);

                } else {

                    inc2.setSrc("stbvc.zul");

                    System.out.println("Subscriber ID set in session...");
                }
            }
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

Your code could be improved by using a guard clause which reduces horizontal spacing.

Names of variables shouldn't be shortened. Tabpanel tp could be easily refactored to a more meaningful Tabpanel tabPanel. Increasing readability helps Sam the Maintainer to the maintaining job.

You should catch specific exceptions, not just Exception. Also you should enclose only the code which can throw an exception.

Object target = fe.getOrigin().getTarget();

if (!(target instanceof Tab)) { return; }

Tab tab = (Tab) target ;

Tabpanel tabPanel = tab.getLinkedPanel();

tabPanel.invalidate();

if ((tabPanel.getId().equals("stbvc") && (Sessions.getCurrent()
        .getAttribute("subscriber") == null))) {

    Messagebox.show(
            "Create Subscriber first,then assign STB/VC!",
            "Warning", Messagebox.OK, Messagebox.INFORMATION);

    tabPanel.setSelectedPanel(basic);

} else {

    inc2.setSrc("stbvc.zul");

    System.out.println("Subscriber ID set in session...");
}


Parse a string to get a SectorId In mobile phones technologies, a SectorId is a 128-bit value broadcast by a 1xEV-DO BTS system.

That is a 16 bytes data.

I made a structure to store them:

using System;
using System.Globalization;
using System.Linq;
using System.Text;

public struct SectorId
{
    readonly byte[] id;

    SectorId(byte[] id)
    {
        this.id = new byte[16];
        Array.Copy(id, this.id, 16);
    }

    public override bool Equals(object obj)
    {
        if (obj is SectorId)
        {
            return Equals((SectorId)obj);
        }

        return false;
    }

    public bool Equals(SectorId other)
    {
        return id.SequenceEqual(other.id);
    }

    public override int GetHashCode()
    {
        int hash = 0;
        unchecked
        {
            foreach (var b in id)
            {
                hash *= 397;
                hash += b;
            }
        }
        return hash;
    }

    public static SectorId Parse(string s)
    {
        Throw.IfArgumentIsNull("s", s);

        SectorId result;
        if (TryParse(s, out result))
        {
            return result;
        }

        throw new FormatException();
    }

    public override string ToString()
    {
        var sb = new StringBuilder();
        foreach (var b in id)
        {
            sb.AppendFormat("{0:X2}", b);
        }
        return sb.ToString();
    }

    public static bool TryParse(string s, out SectorId result)
    {
        if (s == null)
        {
            goto Fail;
        }

        if (s.Length > 32)
        {
            goto Fail;
        }

        if (s.Length < 32)
        {
            s = s.PadRight(32, '0');
        }

        var bytes = new byte[16];

        for (var i = 0; i < 16; i++)
        {
            var substring = s.Substring(i * 2, 2);
            if (!byte.TryParse(substring, NumberStyles.HexNumber, CultureInfo.InvariantCulture, out bytes[i]))
            {
                goto Fail;
            }
        }

        result = new SectorId(bytes);
        return true;

        Fail:
        result = default(SectorId);
        return false;
    }
}

The TryParse part is the worse for me. I would like to avoid gotos.

Note that you can use multiple return statements in a method, so instead of using goto, you can use a return statement. I.e:

if (s == null)
{
    return false;
}


Requesting credentials from users I need to handle configuration errors and ask the user for the right credentials if something is wrong. I cannot decide between three implementations. Which style is better in Python and why?

A:

while not backend.check_config():
    click.echo('Invalid Configuration parameters!')
    for param_name, question in backend.config.check_config_requires:
        value = prompt(question, default=getattr(backend.config, param_name))
        setattr(backend.config, param_name, value)

B:

while True:
    try:
        backend.check_config()
        break
    except ConfigurationError:
        click.echo('Invalid Configuration parameters!')
        for param_name, question in backend.config.check_config_requires:
            value =prompt(question, default=getattr(backend.config, param_name))
            setattr(backend.config, param_name, value)

C:

while True:
    try:
        backend.check_config()
    except ConfigurationError:
        click.echo('Invalid Configuration parameters!')
        for param_name, question in backend.config.check_config_requires:
            value =prompt(question, default=getattr(backend.config, param_name))
            setattr(backend.config, param_name, value)
    else:
        break

I am assuming that backend.check_config() is different between your implementations.  In one case, it seems to return a boolean, but the other two make it look like it might raise an exception.  My answer is based on that assumption.

The name of the function is check_config.  If it were, parse_config, for example, I would say that it should throw an error because it is expected to do something, but can't because of a problem.  Since it is merely checking, I think a boolean fits better, so my preference goes to A.

Between B and C, it looks like the only difference is whether break is in the try block or the else block.  I am happy to see that you are aware of the else block, but my opinion is that it isn't necessary.  The else block is useful for the cases when you are trying to exclude a certain piece of code from the try block.  That makes sense for when the try is in another try, or for when you don't expect an error and want to know the error happened.  In this case, it is impossible for break to throw an error, so I believe the else block is an unnecessary complication.

I did notice that you took out the space between = and prompt(...) in B and C.  PEP 8, the Python style guide, recommends a space on each side of = in assignments. (It recommends no spaces in a function call such as dict(x=4, y=6))  That could be just a typo, but this is a review.

Based on how you use it, I would think that backend.config would be better as a dictionary.  That way, you could do backend.config[param_name] instead of the more complicated getattr(backend.config, param_name) (or settattr(...)).  If config is created externally, you might consider using vars(config) to create a dictionary of its attributes.  Without further information, I couldn't guarantee that it would be easier, but that's how it looks.


Class Object to create objects of type Object, in a dynamic way, with magic methods I have a project in my mind for quite some time, and it requires me to create dynamic objects.

Usually, casting an array to object does the trick (e.g.: (object)array('test'=>5)). But sometimes, we still want the magic methods to fire up. For that, I have developed a class.

It supports the magic methods __construct, __get, __set, __isset and __unset. This also has a method to check the keys in your object.

class Object {
    private $props = array();
    private $methods = array(
        '__construct'=>null,
        '__get'=>null,
        '__set'=>null,
        '__isset'=>null,
        '__unset'=>null
    );

    private function __run_magic_method( $method, $args=null )
    {
        $obj = (object)$this->props;
        $return = call_user_func_array( $this->methods[$method], array_merge( array(&$obj), (array)$args ) );
        $this->props = (array)$obj;
        return $return;
    }

    private function __has_magic_method( $method )
    {
        return isset( $this->methods[$method] ) && is_callable( $this->methods[$method] );
    }

    function __construct( array $props )
    {
        foreach( array_keys($this->methods) as $method )
        {
            if( isset( $props[$method] ) )
            {
                $this->methods[$method] = $props[$method];
                unset( $props[$method] );
            }
        }

        $this->props = $props;

        if( $this->__has_magic_method( '__construct' ) )
        {
            $this->__run_magic_method( '__construct' );
        }
    }

    function __get( $key )
    {
        if($this->__has_magic_method('__get' ) )
        {
            return $this->__run_magic_method( '__get', array($key) );
        }
        else
        {
            return $this->props[$key];
        }
    }

    function __set( $key, $value )
    {
        if( $this->__has_magic_method( '__set' ) )
        {
            $this->__run_magic_method( '__set', array( $key, $value ) );
        }
        else
        {
            $this->props[$key] = $value;
        }
    }

    function __isset( $key )
    {
        if( $this->__has_magic_method( '__isset' ) )
        {
            return $this->__run_magic_method( '__isset', array($key) );
        }
        else
        {
            return isset( $this->props[$key] );
        }
    }

    function __unset( $key )
    {
        if( $this->__has_magic_method( '__unset' ) )
        {
            return $this->__run_magic_method( '__unset', array($key) );
        }
        else
        {
            unset( $this->props[$key] );
        }
    }

    function __call( $key, $args )
    {
        if( isset( $this->props[$key] ) && is_callable( $this->props[$key] ) )
        {
            return call_user_func_array( $this->props[$key], $args );
        }
        else
        {
            throw new Exception( 'The key "'.addslashes($key).'" is not a callable' );
        }
    }

    private function __keys()
    {
        return array_keys( $this->props );
    }

    static function keys( Object $obj=null )
    {
        if( $obj === null )
        {
            return array();
        }
        else
        {
            return $obj->__keys();
        }
    }
}

Example of usage:

$obj = new Object(array(
    '__construct'=>function(){echo 'constructor executed', PHP_EOL;},
    '__isset'=>function($t,$k){
        echo 'checking key: ', $k, PHP_EOL;
        return isset($t->{$k});
    },
    'test'=>5
));

if( isset($obj->test) )
{
    echo $obj->test;
}

/*
Should output:

constructor executed
checking key: test
5
*/

What can I improve in this code, in terms of readability? What crimes am I committing with this code?

As pointed out before, in a comment, non-magic methods in a class shouldn't be prefixed with __.

Since they are private, they are prefixed with _.

Closures are a really nice feature that came with PHP5.3, and they weren't being used to their full potential.

Closures have the method bindTo, which generates a new Closure binded to that object.
This allows to use the variable $this inside the Closure.
It helps the fellow programmers that decide to use this to write in a more natural way.
The best part is that there are no kludges needed!

The method _run_magic_method() will look like this:

    private function _run_magic_method( $method, $args=null )
    {
        $obj = (object)$this->props;

        if( is_a( $this->methods[$method], 'Closure' ) )
        {
            $return = call_user_func_array( $this->methods[$method]->bindTo( $obj ), (array)$args );
        }
        else
        {
            $return = call_user_func_array( $this->methods[$method], array_merge( array(&$obj), (array)$args ) );
        }
        $this->props = (array)$obj;
        return $return;
    }

Since objects are passed as references in PHP5, all changes are reflected on the properties.

The final code:

<?php

    class Object {
        private $props = array();
        private $methods = array(
            '__construct'=>null,
            '__get'=>null,
            '__set'=>null,
            '__isset'=>null,
            '__unset'=>null,
            '__toString'=>null
        );
        private function _run_magic_method( $method, $args=null )
        {
            $obj = (object)$this->props;

            if( is_a( $this->methods[$method], 'Closure' ) )
            {
                //Inside the closure, the $this will be changing the parent $this->props
                $return = call_user_func_array( $this->methods[$method]->bindTo( $obj ), (array)$args );
            }
            else
            {
                $return = call_user_func_array( $this->methods[$method], array_merge( array( &$obj ), (array)$args ) );
            }

            $this->props = (array)$obj;
            return $return;
        }
        private function _has_magic_method( $method )
        {
            return isset( $this->methods[$method] ) && is_callable( $this->methods[$method] );
        }
        function __construct( array $props )
        {
            foreach( array_keys( $this->methods ) as $method )
            {
                if( isset( $props[$method] ) )
                {
                    $this->methods[$method] = $props[$method];
                    unset( $props[$method] );
                }
            }
            $this->props = $props;
            if( $this->_has_magic_method( '__construct' ) )
            {
                $this->_run_magic_method( '__construct' );
            }
        }
        function __get( $key )
        {
            if($this->_has_magic_method('__get' ) )
            {
                return $this->_run_magic_method( '__get', array($key) );
            }
            else
            {
                return $this->props[$key];
            }
        }
        function __set( $key, $value )
        {
            if( $this->_has_magic_method( '__set' ) )
            {
                $this->_run_magic_method( '__set', array( $key, $value ) );
            }
            else
            {
                $this->props[$key] = $value;
            }
        }
        function __isset( $key )
        {
            if( $this->_has_magic_method( '__isset' ) )
            {
                return $this->_run_magic_method( '__isset', array($key) );
            }
            else
            {
                return isset( $this->props[$key] );
            }
        }
        function __unset( $key )
        {
            if( $this->_has_magic_method( '__unset' ) )
            {
                return $this->_run_magic_method( '__unset', array($key) );
            }
            else
            {
                unset( $this->props[$key] );
            }
        }
        function __call( $key, $args )
        {
            if( isset( $this->props[$key] ) && is_callable( $this->props[$key] ) )
            {
                return call_user_func_array( $this->props[$key], $args );
            }
            else
            {
                trigger_error( 'The key "'.addslashes($key).'" is not a callable', E_USER_ERROR );
            }
        }
        function __toString( )
        {
            if( $this->_has_magic_method( '__toString' ) )
            {
                return $this->_run_magic_method( '__toString' ) . '';
            }
            else
            {
                trigger_error( 'There is no defined __toString method', E_USER_ERROR );
            }
        }
        private function _keys()
        {
            return array_keys( $this->props );
        }
        static function keys( Object $obj=null )
        {
            if( $obj === null )
            {
                return array();
            }
            else
            {
                return $obj->_keys();
            }
        }
    }

Example of execution:

$obj = new Object(array(
    'a'=>5,
    '__get'=>function($k){
        echo 'Getting the value from the key \'',$k,'\'', PHP_EOL;
        return $this->{$k};
    },
    '__set'=>function($k,$v){
        echo 'Setting a value on the key \'',$k,'\'', PHP_EOL;
        $this->{$k}=$v;
    }
));

/*
 *Should output:
 *
 * Getting the value from the key 'a'
 * 5
 * Setting a value on the key 'b'
 * Getting the value from the key 'b'
 * 6
 *
 */


Phone number/email regex verifier Is there anything I can do better here? I tried looking for ways to simplify this with comprehension but could not figure out how and was told it is better to explicitly use for loops.

import re

print("This is a email/phone number parser.")
print("Type in the name of the file you want to parse:\n")
filename = input("> ")

emailregex = re.compile(r'''
([a-z0-9\.]+)
(@)
([a-z0-9\.]+)
(\.)
([a-z]{2,4})
''', re.VERBOSE | re.IGNORECASE)

phoneregex = re.compile(r'''
(\+\d)?
(-)?
(\d{3}|\(\d{3}\))?
(\.|-|\s)?
(\d{3})
(\.|-|\s)
(\d{4})
''', re.VERBOSE)

with open(filename, 'r') as filename:
    filedata = filename.read()
    emailmo = emailregex.findall(filedata)
    phonemo = phoneregex.findall(filedata)

print("Emails: \n")
for email in emailmo:
    for group in email:
        print(group, end='')
    print()

print()

print("Phone numbers: \n")
for pn in phonemo:
    for group in pn:
        print(group, end='')
    print()

The regular expression for email that matches 100% accurately would be a sight to see.  One answer to this related question claims to do that.  It certainly is horrendous, so I won't complain that yours isn't completely accurate.  While a phone-number is relatively simple, the possibilities are more than you might think.  For example, not all countries require ten digits.  The ITU has defined the maximum number of digits to be 15.  You can then take into account that the country code might not have a plus sign (+) before it.  It might be something like 1-800-123-4567.  The number of possibilities grows the more one thinks about it, so I won't complain.  I'm merely pointing it out.

I would use re.finditer() instead of re.findall().  That means that instead of getting a list of tuples, you get an iterator of matches.  With that, it becomes:

for email in emailmo:
    print(email.group())

and similar for the phone numbers.

There are two ways that I see to do the list comprehension (with re.finditer()):

[print(pn.group()) for pn in phonemo]
print('\n'.join([pn.group() for pn in phonemo]))

The first is positively sickening.  It creates a list as long as the number of phone numbers or email addresses, and that list isn't even used.  The second is a little more to my liking, but I do believe that the explicit for loop is better.  For your curiosity, the old code in similar comprehensions could be horrendous:

[print(''.join(pn)) for pn in phonemo]

or ... horrendous:

print('\n'.join([''.join(pn) for pn in phonemo]))


Shell function to convert .png to .svg file I needed to convert some custom icons that were in .png format to .svg format and did not want to use an online converter for it.

I could not find an out-of-the-box solution online so I created my own with what I could find.

Out of curiosity and a willingness to learn, as well as to maybe serve the needs of others trying to accomplish the same task I am uploading it here as my first post.

My knowledge on image formats is very minimum at best. My knowledge of shell scripting is at a beginner's stage. My knowledge of coding overall is at hobby level.

I am hoping any comments will help to:

Improve on the overall code quality & readability (also to benefit future maintenance)
Improve on the quality of the actual conversion, make it less lossy.
If possible remove any unneccessary dependencies, as currently potrace is used which wasn't installed by default on my arch system.

My current working setup:

function png_to_svg() {

    ### First use convert to convert to .pnm format ###

    number_of_dots_in_filepath=$(echo "$1" | grep -o "\." | wc -l)
    pnm_filepath=""
    for i in $(seq 1 $number_of_dots_in_filepath); do
        pnm_filepath="$pnm_filepath.$(echo "$1" | cut -f $i -d '.')"
    done

    remove_leading_space=${pnm_filepath#?}
    pnm_filepath="$remove_leading_space.pnm"
    command="convert \"$1\" \"$pnm_filepath\""

    eval "$command"

    ### Second use potrace to convert .pnm to .svg file ###

    svg_filepath="$remove_leading_space.svg"
    command="potrace \"$pnm_filepath\" -s -o \"$svg_filepath\""
    eval "$final_command"

    ### Third and last: remove temporary .pnm file ###

    eval "rm \"$pnm_filepath\""

}

Edited setup after first reply:

function png_to_svg() {

    ### First use convert to convert to .pnm format ###

    pnm_filepath=${1%.*}.pnm
    convert "$1" "$pnm_filepath"

    ### Second use potrace to convert .pnm to .svg file ###

    svg_filepath=${1%.*}.svg
    potrace "$pnm_filepath" -s -o "$svg_filepath"

    ### Third and last: remove temporary .pnm file ###

    rm "$pnm_filepath"

}

You don’t need eval. For example,

eval "rm \"$pnm_filepath\""

is the same as

rm "$pnm_filepath"

but the latter is more readable.

The same is true for the other two cases where you use eval.

I’m not sure what the purpose is of looping over the dots in the file path. Note that directory names can have dots too. The best way to get a file name with a different extension is to use ${x%.*} (see here):

pnm_filepath=${1%.*}.pnm

(I’m assuming you use Bash).


Elevation to power function using class templates I attached below a small working example of a function that is able to perform x^y. As it is possible to see the base (x) takes double and the power takes an int. As last the main loop will also round up or down the result accordingly.

Not always I have these type of formats, and I could have base as int and power as double or base as long int and power as additional type. How can I improve this function to make it more general with different type format using template template <class identifier> or/and template <typename identifier>?

#include <cmath>

using namespace std;

class Power {
public:
    double raiseToPower(double x, int power)
    {
        double result;
        int i;
        result = 1.0;
        for(i = 1; i <=power; i++){
            result = result*x;
        }
        return result;
    }

    double floor0(double num)
      {
        if( (num + 0.5) >= (int(num) + 1) )
        return int(num)+1;
        else
        return int(num);
    }
};

int main()
{
    double x;
    int i;
    Power example;
    std::cout<<"please enter the number"<<std::endl;
    std::cin>>x;
    std::cout<<"please enter the integer power that you want this number raised to"<<std::endl;
    std::cin>>i;
    std::cout<<"rise to power "<<i<<" is equal to "<<example.raiseToPower(x,i)<<std::endl;
    std::cout<<"the result rounded is "<< example.floor0(example.raiseToPower(x,i))<<std::endl;
}

Thank for any insight and point to the right direction.

Headers and namespaces

Don't using namespace, especially a big and growing namespace like std that's not designed for it.

There's no need to include <cmath>.  On the other hand, there is a clear need for <iostream>, which has been omitted.

Structure

There's no need for the Power class; it maintains no state.  The functions should simply be free functions, perhaps in a namespace.

Consider full range of types

raiseToPower() only works with non-negative exponents; it should either accept an unsigned type or be modified to work correctly with negative inputs.  Perhaps like this:

    if (power < 0) {
        return raiseToPower(1/x, -power);
    }

Improve the algorithm

For large power, the loop is executed many times.  We can use binary exponentation to reduce that to log₂ power iterations of the loop.

Avoid over-complication

floor0() doesn't need that if/else; simply add 0.5 before truncating:

constexpr double floor0(double num)
{
    return int(num + 0.5);
}

It might be better to use long or long long there; in any case, you'll still suffer bugs when the value is too big for the integer type.  std::floor() doesn't have that problem.

Validate inputs

If I enter a non-number, I don't get a clear error message.  Instead, the program uses uninitialised values, which is Undefined Behaviour.  Don't do that; instead check that std::cin is still good before using x or i.

Avoid std::endl unless you need output flushing

None of the uses of std::endl here are necessary, and we can use \n instead.  (Remember that using std::cin flushes the output streams, and returning from main() also flushes outputs).

Modified code

double raiseToPower(double x, int power)
{
    if (power < 0) {
        return raiseToPower(1/x, -power);
    }

    double result = 1.0;
    double m = x;

    for (;  power;  power /= 2) {
        if (power % 2) {
            result *= m;
        }
        m *= m;
    }
    return result;
}

constexpr double floor0(double num)
{
    return int(num + 0.5);
}

#include <iostream>

int main()
{
    double x;
    int i;
    std::cout << "Please enter the number\n";
    std::cin >> x;
    std::cout << "Please enter the integer power that "
                 "you want this number raised to\n";
    std::cin >> i;
    if (!std::cin) {
        std::cerr << "Input format error\n";
        return 1;
    }
    auto const result = raiseToPower(x,i);
    std::cout << x << " raised to power " << i << " is equal to "
              << result << '\n';
    std::cout << "The result rounded is "
              << floor0(result) << '\n';
}


Formatting data in array with objects I am formatting a data result from the array of objects that looks like this:

const data = {
  periods: [{
    month: 'January',
    sickLeave: {
        differance: '-12000',
        revision: '6000',
        paid: '18000',
    },
    holidayLeave: {
        differance: '10000',
        revision: '22000',
        paid: '12000',
    },
    received: '-2000',
    taken: '2000',
    result: '0',
  },
  {
    month: 'Februar',
    sickLeave: {
        differance: '-8000',
        revision: '6000',
        paid: '18000',
    },
    holidayLeave: {
        differance: '10000',
        revision: '22000',
        paid: '12000',
    },
    received: '-2000',
    taken: '2000',
    result: '0',
  }],
};

What I want to do with this is to make a new array that looks like this:

[{
  name: "sickLeaveDifferance",
  result:  ["-12000", "-8000"]
},
{
  name: "sickLeaveRevision",
  result: ["6000", "6000"]
},
{
  name: "sickLeavePaid",
  result: ["18000", "18000"]
},
{
  name: "holidayLeaveDifferance",
  result: ["10000", "10000"]
},
{
  name: "holidayLeaveRevision",
  result: ["22000", "22000"]
},
{
  name: "holidayLeavePaid",
  result: ["12000", "12000"]
},
{
  name: "received",
  result: ["-2000", "-2000"]
},
{
  name: "taken",
  result: ["2000", "2000"]
},
{
  name: "result",
  result: ["0", "0"]
}]

I am achieving this with this function:

const formattedResult = perioder => {
  let resultArray = [];
  const pushNewValue = (key, value) => {
    const objectExists = resultArray.find(e => e.name === key);
    if (objectExists) {
      objectExists.result.push(value)
    } else {
      resultArray.push({
        name: key,
        result: [value]
      })
    }
  };

  perioder.map(el =>
   Object.entries(el).forEach(([key, value]) => {
     if (key != 'month') {
       if (key === 'sickLeave' || key === 'holidayLeave') {
         const prop = key;
         Object.entries(value).forEach(([key, value]) => {
           const name = `${prop + key[0].toUpperCase() + key.slice(1)}`;
           pushNewValue(name, value);
         })
       } else {
         pushNewValue(key, value);
       }
     }
   }))

   return resultArray;
}

But, I feel like there is a better, more elegant way of achieving this, just not sure how to do this?

Here is the fiddle.

Do you really anticipate the data model changing so much that this needs to be ultra flexible?  In order to do so the code is quite complex - for instance having a forEach inside of an if inside of a forEach inside of a map.  You already have model specific checks in the code anyways, such as the check for the 'sickLeave' and 'holidayLeave' keys.  I think a solution along these lines would be perfectly acceptable:

const initialNormalizedVals = {
  "sickLeaveDifferance": [],
  "sickLeaveRevision": [],
  "sickLeavePaid": [],
  "holidayLeaveDifferance": [],
  "holidayLeaveRevision": [],
  "holidayLeavePaid": [],
  "received": [],
  "taken": [],
  "result": [],
}

const normalizedData = data.periods.reduce((outcome, { sickLeave, holidayLeave, received, taken, result }) => {
  outcome.sickLeaveDifferance.push(sickLeave.differance)
  outcome.sickLeaveRevision.push(sickLeave.revision)
  outcome.sickLeavePaid.push(sickLeave.paid)
  outcome.holidayLeaveDifferance.push(holidayLeave.differance)
  outcome.holidayLeaveRevision.push(holidayLeave.revision)
  outcome.holidayLeavePaid.push(holidayLeave.paid)
  outcome.result.push(result)
  outcome.taken.push(taken)
  outcome.received.push(received)

  return outcome
}, initialNormalizedVals)

const transformedData = Object.keys(normalizedData).map(key => {
  return {
    name: key,
    result: normalizedData[key],
  }
})


Optimizing Beautiful Soup elements I have this function which parses Beautiful Soup elements:

def get_info():
    holder = []
    problem_dict = {}
    selection_container = []
    for moselect in base_page.find_all('div', attrs={'class': 'Moselect'}):
        if moselect.find('div', attrs={'id': 'noSize'}):
            holder.append(moselect.find_all('div')[1:])

    for held_item in holder[0]:
        if held_item.find_all('option')[1:]:
            for m in held_item.find_all('option')[1:]:

                append_dict(selection_container, m)

                if len(m.text.split(' ')) > 1:
                    stock_info = {
                        'message': reduce(lambda x, y: x + ' ' + y, m.text.split(' ')[1:])[1:-1],
                        'status': 'pre-order'
                    }
                    problem_dict.update({'stock': stock_info})

        else:
            for m in held_item.find_all('option'):

                append_dict(selection_container, m)

                if len(m.text.split(' ')) > 1:
                    stock_info = {
                        'message': reduce(lambda x, y: x + ' ' + y, m.text.split(' ')[1:])[1:-1],
                        'status': 'pre-order'
                    }
                    problem_dict.update({'stock': stock_info})

    problem_dict.update(dict(selections=selection_container))
    return problem_dict

It will return dict with:

problem_dict = {'selection': [{'key': 'value'},{'key': 'value'},]}

and sometimes it returns:

problem_dict = {'selection': [{'key': 'value'},{'key': 'value'}], 'stock': {'key': 'value', 'key': 'value'} }

How can I refactor and optimize this method?

There are a couple things in this method that can be refactored to remove a lot of duplicate code:

Firstly, you have this if statement:

if held_item.find_all('option')[1:]:
    ...
else:
    ...

What this is really checking for is if there is more than 1 option found. So, instead of having to write nearly identical code to handle the cases of 0 options or at least 1 option, we can simply store the find_all result and modify it based on a quick length check:

options = held_item.find_all('option')
if len(options) > 1:
    options.pop(0)

for option in options:
    append_dict(selection_container, option)

    # If there is a non-zero amount of spaces, there will be at least
    # 2 partitions when split.
    if option.text.count(' '):
        stock_info = {'message': reduce(lambda x, y: x + ' ' + y, option.text.split(' ')[1:])[1:-1],
                      'status': 'pre-order'}
    problem_dict.update({'stock': stock_info})

Now, take a look at this line of code:

stock_info = {
    'message': reduce(lambda x, y: x + ' ' + y, m.text.split(' ')[1:])[1:-1],
    'status': 'pre-order'
}

I have two points to make about this line of code:

As a quick disclaimer about this first point:

  The line is still compliant with PEP8, so this is just a personal preference.

With that out of the way, I try to never to put an opening {/(/[ at the end of a line unless I have to. Also, I never put the closing }/)/] on a line by itself. So I would format the line like this:

stock_info = {'message': reduce(lambda x, y: x + ' ' + y, m.text.split(' ')[1:])[1:-1],
              'status': 'pre-order'}

My second point about this line of code is about how you create message. They way you use reduce is not quite necessary. From what I can tell, you want to take everything after the first space, then everything of that section except the first and last characters. This can be easily done using the split function:

stock_info = {'message': m.split(' ', 1)[-1][1:-1] ...

Or you can use list splicing:

# Below is reduced version of:
#    m[m.index(' ') + 1:][1:-1]
stock_info = {'message': m[m.index(' ') + 2:-1] ...

Past these two points, there isn't much to improve the algorithm from what information you gave. If you wanted to reduce the number of lines, you could write the first for loop as a list comprehension:

holder = [moselect.find_all('div')[1:]
          for moselect in base_page.find_all('div', attrs={'class': 'Moselect'})
          if moselect.find('div', attrs={'id': 'noSize'})]

Style-wise, you could improve a few of your variable names (m, holder, and selection_container specifically) as some of them are either ambiguous or slightly misleading.

Here is your code with my suggestions implemented:

def get_info():
    problem_dict = {}
    selection_container = []
    holder = [moselect.find_all('div')[1:]
              for moselect in base_page.find_all('div', attrs={'class': 'Moselect'})
              if moselect.find('div', attrs={'id': 'noSize'})]

    for held_item in holder[0]:
        options = held_item.find_all('option')
        if len(options) > 1:
            options.pop(0)

        for option in options:
            append_dict(selection_container, option)

            # If there is a non-zero amount of spaces, there will be at least
            # 2 partitions when split.
            if option.text.count(' '):
                stock_info = {'message': {'message': m[m.index(' ') + 2:-1],
                              'status': 'pre-order'}
            problem_dict.update({'stock': stock_info})

problem_dict.update(dict(selections=selection_container))
return problem_dict


Compare large text File in java line by line Dear fellow developers I am doing a java program which compare two text files line by line, first text file has 99,000 lines and the other file has 1,15,000 lines. I want to read the files and compare in such a way so that if any line matches between the first file and the second file it should print the match.

I have written the code but it is taking almost 10 mins to complete, as it is printing due to the for loop. How can it be made fast, efficient and efficient in terms of memory allocation? How can it be optimized for performance? Please guide me.

public class Main {

static final String file1 = "file1.txt";
static final String file2 = "file2.txt";

static BufferedReader b1 = null;
static BufferedReader b2 = null;

static List<String> list_file1 = null;
static List<String> list_file2 = null;

public static void main(String[] args) {

    list_file1 = new ArrayList<String>();
    list_file2 = new ArrayList<String>();

    String lineText = null;

    try {
        b1 = new BufferedReader(new FileReader(file1));
        while ((lineText = b1.readLine()) != null) {
            list_file1.add(lineText);
        }
        b2 = new BufferedReader(new FileReader(file2));
        while ((lineText = b2.readLine()) != null) {
            list_file2.add(lineText);
        }
        compareFile(list_file1,list_file2);
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }

}

private static void compareFile(List<String> list_file1, List<String> list_file2) {
    for(String content1:list_file1){
        for(String content2:list_file2){
            if(content1.equals(content2)){
                System.out.println("Match Found:-"+content1);
            }
        }
    }
}
}

Your program's time complexity is \$O(n*m)\$ and space complexity is \$O(n + m)\$ where 'n' is no. of lines in the first file and 'm' is no. of lines in second file.

Here is an optimised version of the above program, with time complexity \$O(n + m)\$ and space complexity \$O( min(m,n) )\$. I have not tested this program, but it should be able to present output on the screen within few seconds :)

import java.io.*;
import java.util.*;

class Main{
   public static void main(String args[]){
      try ( BufferedReader reader1 = new BufferedReader(new FileReader("file1.txt"));
            BufferedReader reader2 = new BufferedReader(new FileReader("file2.txt")) ){

            //assuming file1.txt is smaller than file2.txt in terms of no. of lines
            HashSet<String> file1 = new HashSet<String>();

            String s = null;
            while( ( s = reader1.readLine()) != null){
               file1.add(s);
            }

            while( (s = reader2.readLine()) != null ){
               if(file1.contains(s))
                  System.out.println(s);
            }
      }
      catch(IOException e){
         System.out.println(e);
      }

   }
}

Note: Only one file is in memory at a time and HashSet<> instead of nested loops for comparison.


Rabin Karp algorithm improvement for string contains A to Z only I have made some improvement to Rabin Karp algorithm when the source and pattern only contain the characters from A to Z. The improvement is, I use base 26 for hashing (as there are total 26 characters). In this case, when the hashes match, there is no need to compare the actual string contents (since there are no hash collisions).

Below is my code in Python 2.7, any advice on code efficiency improvement (in terms of algorithm complexity perspective), bugs or code style is appreciated.

Source code in Python 2.7,

def rabin_karp_match(source, pattern):
    base = 26
    p_hash = 0
    s_hash = 0
    for i in pattern:
        p_hash = p_hash * base + ord(i)-ord('A')
    for j,v in enumerate(source):
        if j < len(pattern) - 1:
            s_hash = s_hash * base + ord(v)-ord('A')
            continue
        s_hash = s_hash * base + ord(v)-ord('A')
        if s_hash == p_hash:
            return j - len(pattern)+1
        s_hash = s_hash - (ord(source[j-len(pattern)+1]) - ord('A')) * base ** (len(pattern) - 1)
    return -1

if __name__ == "__main__":
    print rabin_karp_match('GACGCCA','CGC')
    print rabin_karp_match('GATACCCATCGAGTCGGATCGAGT', 'GAG')
    print rabin_karp_match('FOOBARWIDGET', 'WIDGETS')

The code looks good, this probably improves the runtime in some cases, but I am not sure if hash matches happen often enough for this to be effective on average.
Efficiency/Logic

It's probably a good idea to check for the pattern size at the beginning of your method, to avoid overflows. This technically won't happen in python as it will be automatically change the storage to big-int, but the performance of that will be much worse.

Minor optimization for runtime is to save the value of base ** (len(pattern) - 1) before your loop. Exponentiation is an expensive function, so it's better to do it only one time.

Readability

Try to avoid one letter variables like i and j, except for numeric loop indices. A more expressive variable name would improve readability a lot.
Look for repeated parts in your code, and see if it makes sense to extract them into functions. One example in your code is calculating the letter index which is repeated multiple times.
Might make more sense to return None instead of -1 in case of no match.

Modified code
def letter_position(letter):
  return ord(letter) - ord('A')

def rabin_karp_match(source, pattern):
    base = 26
    p_hash = 0
    s_hash = 0
    for letter in pattern:
        p_hash = p_hash * base + letter_position(letter)

    first_letter_base = base ** (len(pattern) - 1)
    for i, letter in enumerate(source):
        s_hash = s_hash * base + letter_position(letter)
        i < len(pattern) - 1:
          continue

        match_start_index = i - len(pattern) + 1
        if s_hash == p_hash:
          return match_start_indexj

        s_hash = s_hash - letter_position(source[match_start_index]) *  first_letter_base
    return None


NoSQL schema for weather data I am using NodeJS and MongoDB for a weather app that takes the weather info from a logger, saves it to a database, and relays the data to the browser in real-time.  It is hosted here with code here which has project details and explanation.

I am saving the weather data to the mongolab cloud-db like so:

// Save to cloud db on MongoLab
mongodb.MongoClient.connect(uri, function (err, cloud_db) {
  cloud_db.collection('raw_data', function(err, collection) {
    if(!err) {
        collection.insert(dbString, function(docs) {
            collection.count(function(err, count) {
                if(!err) {
                    console.log("Saved to mongolab");
                }
            });
        });
     }
  });
});

and the document in db looks like:

{
  "lat": "22.65587",
  "long": "75.82626",
  "date": "2014/08/23",
  "time": "20:10:00.000",
  "temp_c": "25.2",
  "humidity": "86.5",
  "pressure_mbar": "939.0",
  "wind_dir": "85.9",
  "wind_speed_mps": "0.8",
  "rain_mm": "0.0",
  "solar_rad_wpsqm": "-3.8",
  "_id": {
      "$oid": "53f8a7d840ecdfe006b0e3be"
  }
}

I was wondering if this was the best possible schema for storing the data in terms of speed of data retrieval and ease of use.

Another concern is handling the units. I have named the quantities with units in them. Is there any other way this can be handled better ?

You should probably store the numeric values also as numbers and not as strings. This makes data retrieval and manipulation easier. Also have a look here for the coordinates. Date and time should be one field and conform to the ISO date format so you can easily parse it with JS and all other programming languages.


Adjacency list from scratch Here is my code for implementing an adjacency list from scratch. I would like to get feedback from the experts for optimization.

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

 /*a single node of an adjacency list*/
typedef struct adjList{

    int dest;
    struct adjList *next;

} adjList;

/*Image of a graph...*/

typedef struct Image{

    int source;
    adjList *head;

} Image;

void initialize_graph(Image *graph, int vertices);

void print_graph(Image graph[], int vertices);

void add_adj_node(Image *graph, int source, int destiny, bool directed);

void free_graph(Image graph[], int vertices);

int main() {

    int vertices;
    scanf("%d", &vertices);

    Image graph[vertices];

    printf("size of graph: %d bytes\n", sizeof graph);

    initialize_graph(graph, vertices);

    printf("initialized image of graph\n");

    print_graph(graph, vertices);

    printf("                          \n");
    //is the graph directed ? ans: false

    add_adj_node(graph, 1, 7, false);
    add_adj_node(graph, 1, 3, false);
    add_adj_node(graph, 4, 6, false);
    add_adj_node(graph, 4, 1, false);
    add_adj_node(graph, 5, 2, false);
    add_adj_node(graph, 1, 5, false);
    add_adj_node(graph, 1, 2, false);

    print_graph(graph, vertices);

    free_graph(graph, vertices);

    /*if this print produces segmentation fault then the memory is fully freed*/
    printf("graph[1].head->dest%d\n", graph[1].head->dest);

    return 0;
}

void initialize_graph(Image graph[], int vertices) {

        for(int i = 1; i<= vertices; i++){
                graph[i].source = i;
                graph[i].head = NULL;
        }

        return;
}

void add_adj_node(Image *graph, int src, int dest, bool directed){

    adjList *cache = malloc(sizeof(adjList));
    /*create a single node*/
    cache->dest = dest;
    cache->next = NULL;

    if(graph[src].head == NULL){
            graph[src].head = cache;

    }

    else{
            /*put the head address on the crawler*/
            adjList *crawler = graph[src].head;

            while( crawler->next != NULL){
                crawler = crawler->next;
            }

            /*update head value and address. head will point to new adj node
             this will also link src -> dest*/

            crawler->next = cache;
    }

    if (directed == false) {
          directed = true;

          /*notice we've changed the sequence. dest and src*/
          add_adj_node( graph, dest, src, directed);
    }

    return;
}

void print_graph(Image *graph, int vertices){

    for(int i = 1; i<= vertices; i++){

                adjList *crawl = graph[i].head;
                printf("node: %d    ", graph[i].source);

                while(crawl){

                      printf("%d ", crawl->dest);
                      crawl = crawl->next;
                }

                printf("\n");
        }

    return;
}

/*just a reverse version of crawling a graph*/

void free_graph(Image *graph, int vertices){

    for(int i = 1; i<= vertices; i++){

                adjList *cache;
                printf("releasing elements of node: %d    ", graph[i].source);

                while(graph[i].head){

                      /*put the next adjacency node in the cache*/
                      cache = graph[i].head->next;
                      /*free the present adjacencey node*/
                      free(graph[i].head);
                      graph[i].head = cache;
                }

                printf("\n");
        }

    return;
}

Out-of-bounds access: an array

Image graph[vertices];

has vertices entries, starting with index 0, with the largest valid index being vertices - 1. The loop

for(int i = 1; i<= vertices; i++)

touches graph[vertices], which is illegal. A correct (and idiamatic) loop is

for(int i = 0; i < vertices; i++)

Testing for memory correctly released via inducing a segfault is, to put it mildly, unconventional. You are not even guaranteed to get one.
Crawling the list is a waste of time. It is much simpler to prepend  the newly created node at the head of the list:

void add_adj_node(Image *graph, int src, int dest, bool directed)
{
    adjList * node = create_node(dest);
    node->next = graph[src].head;
    graph[src]head = node;
}

Recursive invocation of add_adj_node is confusing. I recommend to have a helper function do_add_adj_node and invoke it like this:

void add_adj_node(Image *graph, int src, int dest) {
    do_add_adj_node(graph, src, dst);
    do_add_adj_node(graph, dst, src);
}


Python 3 two-sum performance I just submitted a Python solution to the 'Two Sum' problem on LeetCode.

The Problem

  Given an array of integers, return indices of the two numbers such
  that they add up to a specific target.

  You may assume that each input would have exactly one solution, and
  you may not use the same element twice.

  Example

  Given nums = [2, 7, 11, 15], target = 9

  Because nums[0] + nums[1] = 2 + 7 = 9

  return [0, 1].

My Solution

class Solution:
    def twoSum(self, nums, target):
        number_bonds = {}
        for index, value in enumerate(nums):
            if value in number_bonds:
                return [number_bonds[value], index]
            number_bonds[target - value] = index
        return None

It was accepted and then told me the following:

  Your runtime beats 83.12 % of python3 submissions.

This means that 16.88% of python3 submissions were more efficient than this one.

How can I get this more efficient than the solution provided above? Is there a funky way to do this with numpy that I'm not getting? Is there maybe another data structure I should use in this situation? Or a better algorithm altogether? I've explored a few alternate solutions and profiled with timeit but none were faster than this.

I think your solution is already pretty good. It is at worst \$\mathcal{O}(n)\$ in both runtime and memory. You use enumerate instead of iterating over the indicies manually.

The only four things I would comment on are:

In real life code, this being a class would of course  be completely unnecessary (but it seems to be mandatory in this case).
Explicitly returning None at the end. This is always a judgement call between being unnecessarily verbose (since it is returned implicitly anyways) and being more expressive. Since this case is explicitly excluded in the problem description (all inputs have exactly one solution), I would not add it here. If it were an explicit rule to return None in case there was no solution I would, however, include it.
You should add some docstring to the class/method. At least to get in the habit of always doing it.
Python's official style-guide, PEP8, recommends using lower_case for variable, function and method names (but this seems to also be mandatory from the problem description).


JavaScript Random Color Generator I made a page to generate a random color with JavaScript and set that color to the background of the page.

//Global Variables
const hexChars = '0123456789ABCDEF';
const container = document.getElementById('container');
const hexText = document.getElementById('hex-text');

//Function to generate random hex code

function randomColor() {
 let hexCode = '#';
 for(i=0; i<=5; i++) {
   let randomNum = Math.floor(Math.random() * 16);
    hexCode += hexChars[randomNum];
  }
  return hexCode;
}

//Function to change page color & text

function applyColor() {
  let hexCode = randomColor();
 container.style.backgroundColor = hexCode;
  hexText.innerHTML = hexCode;
}

//Make random color on page load, click, and spacebar press.
window.onload = applyColor();
container.onclick = () => applyColor();
document.body.onkeyup = e => {
  if(e.keyCode == 32) {
    applyColor();
  }
}
* {
  margin: 0;
  padding: 0;
  user-select: none;
}

#container {
  position: absolute;
  height: 100%;
  width: 100%;
  display: flex;
  align-items: center;
  justify-content: center;
  cursor: pointer;
  transition: background-color .4s;
}

#hex-text {
  font-family: sans-serif;
  font-size: 2em;
  color: white;
  background-color: rgba(150, 150, 150, .6);
  padding: .4em;
  border-radius: 6px;
}

    #000000

How can I improve the efficiency or functionality of the JavaScript?

Edit: I'd also like to know if this is a fair generator, or if it's more likely to get one shade than another. I don't see why it would, but you never know.

Looks very good. And, as far as I know, your colour generator is fair.

For efficiency

you don't need to generate a random number for every hex character. You are choosing among 256^3 (16 777 216) colours, labeled with every number from 0x0 (0 in decimal) to 0xFFFFFF (256^3-1 in decimal, or 16 777 215).

So you can:

generate a random number let randomNum = Math.floor(Math.random() * 16777216) // 256^3
use an inherent function for JavaScript Numbers, toString, to convert it to a string with an arbitrary radix:
let hextest = randomNum.toString(16)

By design, randomNum is a natural number (non-negative or 0) so you don't need code for validation).
pad it with zeros. This question was asked a million times in Stack Overflow (this is only an example).

For your example, where you don't need validation, hextest = ('00000' + hextest).substr(-6) will do. (After padding with all the zeros you'd ever need, picks only the last 6 characters.)
convert to uppercase and decorate: hextest = '#' + hextest.toUppercase(). Reference for toUpperCase()

This way you can get rid of the hexChars constant.

For functionality

I don't have any good idea on how to add functions. But I can suggest an improvement:

in the code you have to capture spacebar presses, add the line e.preventDefault(). This will prevent the default behaviour for the space key (which is scrolling a screen down).

That way, you can add all the content you want to your page without fear of accidentally scrolling down (and it won't scroll down Code Review when I try your embedded snippet, either).


Deck shuffle and drawing cards I developed a simple deck shuffle and drawing system, as a test on how to use collections and learn from it.

However, I think my code is a bit wonky, and I think here is the best place to get new tips on what to improve.

Public deck As New Collection
Public face As New Collection
Public symbols As New Collection

Sub atomic()

face.Add "A"
For i = 2 To 10
    face.Add i
Next i
face.Add "J"
face.Add "Q"
face.Add "K"
symbols.Add ChrW(9826)
symbols.Add ChrW(9828)
symbols.Add ChrW(9825)
symbols.Add ChrW(9831)

End Sub

Sub lime()

Set deck = Nothing
Set face = Nothing
Set symbols = Nothing
Dim i As Long

For i = 1 To 52
deck.Add i
Next i

atomic

End Sub

Sub versive(draw As Long)
Dim randc As Long
Dim ub As Long
Dim cardnumber As Long
Dim actualcard As String
Dim actualcard2 As String
Dim i As Long
Dim lastrow As Long
lastrow = Cells(Rows.count, 14).End(xlUp).row
For i = 1 To draw

    ub = deck.count
    If ub = 0 Then
        MsgBox "Out of cards"
    Exit Sub
    End If
    randc = Int((ub - 1 + 1) * Rnd() + 1)
    If randc > ub Then
       randc = ub
    End If
    cardnumber = deck.Item(randc)
    deck.Remove (randc)
    actualcard = face.Item(cardnumber - (WorksheetFunction.RoundUp(cardnumber / 13, 0) - 1) * 13) & symbols.Item(WorksheetFunction.RoundUp(cardnumber / 13, 0))
    lastrow = lastrow + 1
    Cells(lastrow, 14).Value = actualcard
Next i
End Sub

Private Sub CommandButton1_Click()
versive (51)
End Sub

Private Sub CommandButton2_Click()
lime
End Sub

If it matters, it is developed on Excel VBA.

The atomic sub calls for the definition of the cards names, because it was not just numbers (Like 1 is actually "A") and the definition for the symbols (Hearts, diamonds, etc).

The lime sub basically restarts everything. It runs the atomic sub, so lime actually needs to run first. It also defines that the deck will have 52 cards.

The versive sub actually draws cards. So after it gets a random number from 1 to the amount of cards left in the deck, it converts the number to the actual value and symbols of cards, doing some math to convert it in such a way that number 1 becomes Ace of Diamonds and number 52 becomes King of Clubs. Versive sub also allows multiple cards drawn at once.

Your naming is funny and all but quite counter-productive - names primary goal is to explain what the code does, not to give a lol moment (I know, boring).

  The atomic sub calls for the definition of the cards names

then call it DefineCards

  The lime sub basically restarts everything

then call it Restart

  The versive sub actually draws cards

then call it DrawCards


Trying to simplify a setTimeout sequence Here is the script:

function opening(){
    setTimeout(function () {
        var opening = $('#stage');
        opening.find('h1').fadeIn('slow', function(){
            setTimeout(function () {
                opening.find('.logo').fadeIn('slow', function(){
                    setTimeout(function () {
                        opening.find('p').fadeIn('slow');
                        opening.find('#btn').fadeIn('slow');
                    }, 400);
                });
            }, 800);
        });
    }, 400);
}

It's work fine. But I would like to try to simplify it. Is possible?

// Put everything in one function, but break it up internally
function opening() {
  // A tiny, internal helper function
  // I've switched the order of arguments compared to setTimeout
  // because it makes for nicer code (no dangling argument after
  // a function body)
  function createTimeout(delay, func) {
    return function () {
      setTimeout(func, delay);
    };
  }

  var step1, step2, step3, // Declare some vars that'll hold functions
      stage = $('#stage'); // and find the #stage element right away

  // Create the first step. When called, it'll wait 400ms
  // and then execute the function
  step1 = createTimeout(400, function () {
    stage.find('h1').fadeIn('slow', step2); // call step2, when the fade-in completes
  });

  // Same principle as above
  step2 = createTimeout(800, function () {
    stage.find('.logo').fadeIn('slow', step3); // call step3
  });

  // and again
  step3 = createTimeout(400, function () {
    stage.find('p').fadeIn('slow');
    stage.find('#btn').fadeIn('slow');
  });

  step1(); // execute step 1
}

So calling opening() will

wait 400ms
make the H1 to fade in
wait 800ms
make the logo fade in
wait 400mx
make the P and #btn elements fade in


Python command-line Tic Tac Toe for two humans I’d love some feedback on this human v. human, tic tac toe game I wrote in Python 2.7.10. I’d love to hear your thoughts on optimization and making the code simpler and more “pythonic”.

Also, I’m wondering if passing the player and turn variables around the way I have is the best approach, or if I should consider global variables (which is how I might do it in Javascript, in a closure).

# Command-line Tic Tac Toe for two humans written in Python.
# Play in a terminal by running 'python tictactoe.py'.

boxes = [ ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ]
first_player = 'X'
turn = 1
winning_combos = [  [0, 1, 2], [3, 4, 5], [6, 7, 8], [0, 3, 6],
                    [1, 4, 7], [2, 5, 8], [0, 4, 8], [2, 4, 6], ]

def print_board(initial=False):
    """ Print the game board. If this is the beginning of the game,
        print out 1-9 in the boxes to show players how to pick a
        box. Otherwise, update each box with X or 0 from boxes[].
    """
    print('''
             {} | {} | {}
            -----------
             {} | {} | {}
            -----------
             {} | {} | {}
        ''').format(*([x for x in range(1, 10)] if initial else boxes))

def take_turn(player, turn):
    """ Create a loop that keeps asking the current player for
        their input until a valid choice is made.
    """
    while True:
        box = raw_input('Player %s, type a number from 1-9 to select a box: ' % player)

        try:
            box = int(box) - 1 # subtract 1 to sync with boxes[] index numbers
            if 0 <= box <= 8:
                if boxes[box] == ' ': # initial value
                    boxes[box] = player # set to value of current player
                    break
                else:
                    print('That box is already marked, try again.\n')
                    continue
            else:
                print('That number is out of range, try again.\n')
                continue

        except ValueError:
            # Not an integer
            print('That\'s not a valid number, try again.\n')
            continue

def switch_player(turn):
    """ Switch the player based on how many moves have been made.
        X starts the game so if this turn # is even, it's 0's turn.
    """
    current_player = '0' if turn % 2 == 0 else 'X'
    return current_player

def check_for_win(player, turn):
    """ Check for a win (or a tie). For each combo in winning_combos[],
        count how many of its corresponding squares have the current
        player's mark. If a player's score count reaches 3, return a win.
        If it doesn't, and this is already turn # 9, return a tie. If
        neither, return False so the game continues.
    """
    win = False
    tie = False
    if turn > 4: # need at least 5 moves before a win is possible
        for x in range(len(winning_combos)):
            score = 0
            for y in range(len(winning_combos[x])):
                if boxes[winning_combos[x][y]] == player:
                    score += 1
                if score == 3:
                    win = True

        if turn == 9:
            tie = True

    return win, tie

def play(player, turn):
    """ Create a loop that keeps the game in play
        until it ends in a win or tie
    """
    while True:
        take_turn(player, turn)
        print_board()
        win, tie = check_for_win(player, turn)
        if win or tie:
            if win:
                print('Game over. %s wins!\n' % player)
            else:
                print('Game over. It\'s a tie.\n')
            break
        turn += 1
        player = switch_player(turn)

# Begin the game:
print('\n\nWelcome to Tic Tac Toe for two humans!')
print_board(initial=True)
play(first_player, turn)

Avoid too much code in try blocks

The try ... except ValueError in the take_turn function wraps too many statements that it shouldn't need. Only the int(...) statement may raise a ValueError there, so it would be better to wrap only that in the try. When you wrap a large block of code, it can lead to masked errors or make defect localization harder.

Magic values

Instead of referring to the player symbols by their literals like '0' and 'X', it would be better to give them names.

Iterate over elements when possible

In this code you iterate over indexes of elements:

   for x in range(len(winning_combos)):
        score = 0
        for y in range(len(winning_combos[x])):
            if boxes[winning_combos[x][y]] == player:
                score += 1
            if score == 3:
                win = True

It would be more natural, simpler and safer to iterate over elements:

for combo in winning_combos:
    score = 0
    for index in combo:
        if boxes[index] == player:
            # ...


Efficiently Sorting File Paths I'm trying to print out a list of files in a specific order, with files closer to the root directory sorted first. For example these files:

c:\users\steve\foo.txt
c:\users\bar.txt
c:\users\steve\desktop\file.txt
c:\users\john\bing.txt

Should be sorted as:

c:\users\bar.txt
c:\users\john\bing.txt
c:\users\steve\foo.txt
c:\users\steve\desktop\file.txt

Code:

namespace fs = std::experimental::filesystem;

std::vector<std::string> sortedRules;
std::sort(sortedRules.begin(), sortedRules.end(),
    [&](const string &str1, const string &str2)
{
    fs::path path1(str1);
    fs::path path2(str2);

    //if parent directories are the same, compare filenames
    if (path1.parent_path() == path2.parent_path())
        return path1 < path2;

    std::vector<std::string> path1Expnded;
    std::vector<std::string> path2Expnded;

    for (const auto &part : path1)
        path1Expnded.emplace_back(part.string());

    for (const auto &part : path2)
        path2Expnded.emplace_back(part.string());

    //if paths have same amount of stems, compare full paths
    if (path1Expnded.size() == path2Expnded.size())
        return path1 < path2;

    int minSize;
    path1Expnded.size() <= path2Expnded.size()
        ? minSize = path1Expnded.size()
        : minSize = path2Expnded.size();

    //sort by first differing stem
    for (int i = 0; i < minSize - 1; i++)
        if (path1Expnded[i] != path2Expnded[i])
            return path1Expnded[i] < path2Expnded[i];

    //if sizes differ and no stems are the same, sort by number of stems
    return path1Expnded.size() < path2Expnded.size();
});

I created a custom comparison lambda that overrides how std::sort compares, and it works most of the time. Sometimes items cannot be found after being sorted using this comparison lambda, and after being searched for using this same comparison lambda. So I'm guessing my algorithm is somehow incorrect.

Also even when the algorithm does run, it does so very slowly. It took me awhile to get the files to sort just the way I wanted, and while I tried to allow the function to return as soon as possible its performance is pretty poor. How can I fix the algorithm to ensure its correctness, and also make it more efficient?

First, don't use the conditional operator as an if statement.  Either encode the conditional as part of the initialization of minSize, or use an if statement instead of the conditional.  In this case, though, a better solution is to make use of the standard min function:

int minSize = std::min(path1Expnded.size(), path2Expnded.size());

But we can redo your code to eliminate this part.

The sort you've coded and the one you describe (how I am interpreting your description, anyway) are different, as your code only looks at the number of subfolders if one path is in a subfolder of the folder that has the file in the other path.

Your problem description ("files closer to the root directory sorted first"), sounds like all the paths with fewer folders should be before longer ones, in which case the number of stems should be the first thing compared.  Then a direct comparison on the full paths if they have the same folder count.

What you've coded up will put files in a folder before subfolders of that folder.  Your current implementation does a lot of memory allocations and frees, mainly with strings and vector resizes.  You can simplify this a lot by first comparing the two parent_paths.  If they are different, return the result of this comparison (the path to a file will be shorter than the path for a subfolder in the folder with the file, so it'll be earlier).  Otherwise the two paths are the same, and return the result of comparing the filenames.


Creating a struct-of-arrays database with encapsulating interface c++ I want to create a struct-of-arrays (SOA) database for better cache use but also have a separate interface that gives encapsulation. I am close to the design I think but would like more experienced coding eyes to have a look to critique my design. Here is what I have so far:
struct OrbitalDataBase {
  OrbitalDataBase(std::shared_ptr<FEMDVR> a_radial_grid, std::shared_ptr<AngularGrid> a_angular_grid)
      : nbas(a_radial_grid->getNbas()), number_of_lm_pairs_per_orbital(), m_orbital_coefs() {
    m_orbitals_coefs.reserve(a_radial_grid->getNbas() * a_angular_grid->getNumChannels());
}

  inline void StoreData(const uint32_t &a_ith_orbital,
                        std::shared_ptr<MOPartialWaveRepresentation> a_MO) {

    this->number_of_lm_pairs_per_orbital.push_back(a_MO->getNumChannels());

    this->m_orbital_coefs.reserve(a_ith_orbital * nbas *
                                  a_MO->getNumChannels());

    for (uint32_t i = 0; i < nbas * a_MO->getNumChannels();
         ++i) {
      this->m_orbital_coefs.push_back(a_MO->getPartialWaveRep(i));
    }
  }

  /* private: */
  uint32_t nbas;
  std::vector<uint32_t> number_of_lm_pairs_per_orbital;
  std::vector<std::complex<double>> m_orbital_coefs;
};

struct OrbitalAccessor {
  OrbitalAccessor(const OrbitalDataBase &a_orbital_data)
      : orbital_data(a_orbital_data){};

  inline std::complex<double> coef(uint32_t ith_orbital, uint32_t i) const {
    return this->orbital_data.m_orbital_coefs
        [ith_orbital * this->orbital_data.nbas *
             this->orbital_data.number_of_lm_pairs_per_orbital[ith_orbital] +
         i];
  }

private:
  const OrbitalDataBase orbital_data;
};

How this would work is construct the database with the number of orbitals and then in a for-loop, save all the ith_orbital's coefficients in one large array. Something like this
uint32_t number_of_orbitals = 5;
OrbitalDataBase orb_data_base(number_of_orbitals);
for (uint32_t ith_orbital = 0; ith_orbital < number_of_orbitals < ++ith_orbital)
  {
    Orbital();
    orb_data_base.StoreData(ith_orbital, femdvr_grid, orbital);
  }

The Orbital() function just creates the orbital I want to store. Then the OrbitalAccessor should be able to access these large arrays index with stride ith_orbital. I think I should pass the accessor the created database I want to access in a constructor like
  int which_orbital = 1, int index 4; // example looks at 2nd orbital's 5th coefficient
  OrbitalAccessor orb_accessor(orb_data_base);
  orb_accessor.coef(which_orbital, index);

I tried to have the ObitalDataBase member data private but I get

'm_orbitals_coefs' is a private member of 'OrbitalDataBase'

so I commented the private: keyword so it will compile. I think they should be private for encapsulation so I'd prefer that if I am correct.
I think a better way would be to have OrbitalAccessor derive the OrbitalDataBase rather than pass it in to a constructor.
UPDATE: I changed the code a little so now the m_orbital_coef has a base vector capacity and resizes if it becomes larger because a_MO->getNumChannels() can be larger than a_angular_grid->getNumChannels().

There is a lot missing from this question, but there is enough code to be able to tell that a lot that could be improved.  Here are some ideas for improving your code.
Use all of the required #includes
All of the #includes for this code are missing in the description.  I believe the required list is this:
#include <complex>
#include <cstdint>
#include <memory>
#include <vector>

Provide complete code to reviewers
This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.  There is a bit of that in your question, so we'll start from there.
Start with the desired interface
It's usually very useful to start by imagining how we'd like to use the code before implementing the interface.  It appears that there exists some external source of orbital data (I'm guessing TLE) which this code is supposed to read and parse (not shown) and then provide storage and access methods.  First, rather than two separate classes, OrbitalDataBase and OrbitalAccessor, it seems clear to me that there should rather be a single OrbitalDataBase and that the accessor should be a member function.  That leads to the first simplification:
std::complex<double> coef(uint32_t ith_orbital, uint32_t i) const {
    return m_orbital_coefs[ith_orbital * nbas * number_of_lm_pairs_per_orbital[ith_orbital] + i];
}

Don't write this->
Within member functions this-> is redundant. It adds visual clutter and does not usually aid in understanding.
Use more whitespace for clarity
It's very difficult to parse the OrbitalDataBase constructor because it's all run together with very little whitespace.  I'd modify it to look more like this:
struct OrbitalDataBase {
  OrbitalDataBase(std::shared_ptr<FEMDVR> a_radial_grid, std::shared_ptr<AngularGrid> a_angular_grid)
      : nbas(a_radial_grid->getNbas())
      , number_of_lm_pairs_per_orbital()
      , m_orbital_coefs()
{
    m_orbital_coefs.reserve(a_radial_grid->getNbas() * a_angular_grid->getNumChannels());
}

Don't explicitly invoke implied constructors
The constructor, reformatted as above, reveals that the default and implied constructors for number_of_lm_pairs_per_orbital and m_orbital_coefs are explicitly called.  This is both unnecessary and unhelpful.  Simply omit those two lines.
Prefer const references to smart pointers
If you are passing in data, such as the FEMDVR and AngularGrid in this code, that are used but not altered, this implies const.  Further, if we are not sharing ownership, which appears to be the case here, these should be references rather than std::shared_ptr.
OrbitalDataBase(const FEMDVR& a_radial_grid, const AngularGrid& a_angular_grid)
    : nbas(a_radial_grid.getNbas())
{
    m_orbital_coefs.reserve(nbas * a_angular_grid.getNumChannels());
}

Understand modern use of inline
Based on how you're using it, it appears that you are abusing the inline keyword to get an effect that the compiler is already going to provide without it.  That is the inline specifier for functions used to be a request to the compiler to optimize by putting the code inline rather than creating an explicit function call.  However, this was only ever a suggestion, and the changing semantics of inline for C++17 and beyond are likely to lead to trouble unless very carefully used.  The reason is that, starting with C++17, it tells the compiler that multiple defifinitions are allowed, but if they differ in any way, invoking the function is undefined behavior.  For these reasons, you're better off omitting inline in most cases.
Rethink your data structures
Right now getting a value back uses this rather complex form:
std::complex<double> coef(uint32_t ith_orbital, uint32_t i) const {
    return m_orbital_coefs[ith_orbital * nbas * number_of_lm_pairs_per_orbital[ith_orbital] + i];
}

This is very cache unfriendly because it has to look up one value from number_of_lm_pairs_per_orbital and then use that to index into another vector.  It may make things more clear and give better performance if instead you defined a struct or class that encapsulated both.  The obvious would be this:
struct Orbital {
    std::vector<std::complex<double>> m_orbital_coefs;
};

Then OrbitalDataBase could use std::vector<Orbital> as its only data member, since the number of coefficients is m_orbital_coefs.size() and nbas is now m_orbital_coefs.size().
Even simpler might be this:
using Orbital = std::vector<std::complex<double>>;


Classes for reading XML from a file Basically, all I have to do is read some xml from a file. And I have a working solution, although I am not sure that it is the best way of going about doing things, as I have only recently started working with xml. Any advice on how I could improve the following?

[XmlRoot("iosLayout")]
public class iosLayoutXml
{
    [XmlElement("transactionLayout")]
    public List<TransactionLayoutXml> transactionLayouts { get; set; } = new List<TransactionLayoutXml>();
}

[XmlRoot("transactionLayout")]
public class TransactionLayoutXml
{
    [XmlElement("field")]
    public List<FieldXml> fields { get; set; } = new List<FieldXml>();

    [XmlAttribute("type")]
    public string type { get; set; }
}

[XmlRoot("field")]
public class FieldXml
{
    [XmlAttribute("element")]
    public string element { get; set; }

    [XmlAttribute("text")]
    public string text { get; set; }

    [XmlAttribute("value")]
    public string value { get; set; }
}

It's a small code snippet but IMO you may improve few things about style:

Usually (in .NET environment) properties are TitleCase as well as classes.
Whatever your favorite naming convention is you should be consistent, if you use title case for classes then iosLayoutXml should be renamed IosLayoutXml.
Prefixes and suffixes are generally avoided, you're using Xml suffix and it has also another disadvantage: you're exposing an implementation detail. You may, in future, change storage to Json but calling code should not be aware of that.

Some notes (without more context they're almost questions) about code:

If your classes will not be derived then you should also mark them as sealed. To open a class to inheritance requires much more work.
You do not initialize properties with default values: is it null, for example, acceptable for FieldXml.text if omitted in XML file?
From caller point of view is it null equivalent to String.Empty? Do you want to move this responsability on caller?
Will anyone use these classes for setting properties? In this case you should also add proper argument checking.
You may not need it in this moment but it may be useful (think about it) to define FieldCollection and use it instead of List<Field>. Once it has been deserialized will it change in size? If not you may also consider to use FieldXml[].
Is it valid to have a null entry for TransactionLayoutXml.fields?
I don't know which kind of values will go into FieldXml.text and FieldXml.value (and if format is fixed or you have some freedom) but if they contains characters that must be escaped you may want to use node text instead of an attribute, especially if XML may edited by hand it's much more handy to use CDATA sections.


Making a list from user input I have wrote a program that lets the user open up their text file that would look something like this:

1
2
3
4
5
6

It would then let the user enter a name for the list such as "numbers" The program would then let the user choose where they want to save the list. The user would then click go and the program would turn their input into a list like this:

numbers = ['1', '2', '3', '4', '5', '6']

This is the code that I wrote:

import tkinter
from tkinter import *
from tkinter.filedialog import askopenfilename
from tkinter.filedialog import asksaveasfilename
from tkinter.messagebox import showerror
from tkinter import filedialog
import sys

def program():
    global saveName
    with open(fname) as input_file:
        longList = [line.strip() for line in input_file.readlines()]
        finListName = listName.get()
        finList = (finListName) + (" = ") + str(longList)
        txtFile = open((saveName),"w")
        txtFile.write(finList)

#Window
window = tkinter.Tk()
window.title("Python List Maker")
window.geometry("300x170")

#Label
fileSelectLBL = tkinter.Label(window, text="Please select your file:")
fileSelectLBL.pack()

#Button
def load_file():
    global fname
    fname = askopenfilename(filetypes=(("Text files", "*.txt"),
                                       ("All files", "*.*") ))
filename = tkinter.Button(window, text="Browse", command = load_file)
filename.pack()

#List Name
listNameLBL = tkinter.Label(window,text="Please enter what you want to call the list:")
listNameLBL.pack()
listName = Entry(window)
listName.pack()

#Save List
saveListLBL = tkinter.Label(window,text="Please select where you want to save the list:")
saveListLBL.pack()
def save_file():
    global saveName
    saveName = asksaveasfilename(filetypes=(("Text files", "*.txt"),
                                       ("All files", "*.*") ))
Savename = tkinter.Button(window, text="Browse", command = save_file)
Savename.pack()

#Go
goBTN = tkinter.Button(window, text="GO!", command = program)
goBTN.pack()

#Main Loop
window.mainloop()

Now although this code works, it is really messy. I am not that new to Python but my code is much longer and complex than what it could/should be. This was my first ever go at a program with a GUI, all of my other programs have just been run through the shell.

I am just looking for some general feedback on how I can improve this, and make it shorter and more efficient.

Structure

Firstly, try not to use the global keyword. This keyword should really be used only if you HAVE to. A simple solution for this is to implement a class that wraps around all of the functions:

class MyFileManager(object):
    def __init__(self, filename='', save_filename=''):
        self.filename = filename
        self.save_filename = save_filename

    # Re-named `program` function
    def save_users_list(self, list_name):
        ...

    def get_filename(self):
        self.filename = askopenfilename(filetypes=(("Text files", "*.txt"),
                                                   ("All files", "*.*")))

    def get_save_file(self):
        self.save_filename = asksaveasfilename(filetypes=(("Text files", "*.txt"),
                                                          ("All files", "*.*")))

Now your button declarations would look like this (see here for the reasoning behind using lambda for go_button):

manager = MyFileManager()
filename_button = tkinter.Button(window, text="Browse", command=manager.get_filename)
save_filename_button = tkinter.Button(window, text="Browse", command=manager.get_save_file)
go_button = tkinter.Button(window, text="GO!", command=lambda: manager.save_users_list(
                                                                   list_name.get()))

Naming

Notice how in my examples above I use_underscores instead of camelCase for variable names. This is the Python convention.
When thinking of variable names, err on the side of being too verbose rather than too brief: list_name_label is a much better name than listNameLBL.
You mostly do a good job of this, but for GUI widgets remember to append the type of widget in the variable name. You forgot to do this on your filename button widget.

program Function

Your orignal function has a few problems/tweaks that can be made:

The name is very ambiguous. Update it to better reflect what the function does.
You used the with statement to open filename. However, when you open saveName you don't. Because of that you forgot to close() that file. ALWAYS use with when accessing files as it will save you these mistakes.
I prefer to always say which mode we are opening files in. So add the mode ('r') to the with statement.
Try to make the amount of time a file is open the smallest it can be. So instead of having filename open for the duration of the function only keep it open when creating longList.
Instead of doing multiple string concatenations, use format() to create your finList.
There is no need for the parens around variables.

Here is my version of your program function. Assume it is inside the class I outlined above:

def save_users_list(self, list_name):
    with open(self.filename, 'r') as file:
        numbers_list = [line.strip() for line in file.readLines()]

    with open(self.save_filename, 'w') as file:
        file.write('{} = {}'.format(list_name, str(numbers_list)))

As a final general comment, take a look at PEP8, the official Python style guide. It will help your code look and feel more Pythonic.


Turn multiple key presses into single GameAction The InputHandler class for my game detects key presses, turns each one into a GameAction, and raises an event:

public class InputHandler
{
    public delegate void ActionListener(GameActions gameAction);

    public event ActionListener ActionRequested;

    private void ProcessKeyboard(KeyboardState keyboardState)
    {
        var pressedKeys = keyboardState.PressedKeys();

        foreach (var inputAction in pressedKeys.Select(GetInputAction))
        {
            ActionRequested(inputAction.Down);
        }

        var releasedKeys = keyboardState.ReleasedKeys(prevPressedKeys);

        foreach (var inputAction in releasedKeys.Select(GetInputAction))
        {
            ActionRequested(inputAction.Up);
        }

        prevPressedKeys = pressedKeys;
    }
}

Only one GameAction can be requested at a time. This means that if you press the key bound to GameActions.MoveUp and the key bound to GameActions.MoveLeft at the same time, two separate events are raised and it looks like your character is moving diagonally.

Now, I need to detect this as a single GameAction, e.g. GameActions.MoveUpLeft. There has to be a better way to do it than what I've come up with.

private void ProcessKeyboard(KeyboardState keyboardState)
{
    var pressedKeys = keyboardState.PressedKeys();

    var inputActions = pressedKeys.Select(GetInputAction).ToList();
    var downActions = inputActions.Select(ia => ia.Down);

    if (downActions.Contains(GameActions.MoveUp) && downActions.Contains(GameActions.MoveLeft))
    {
        ActionRequested(GameActions.MoveUpLeft);
    }
    else if (downActions.Contains(GameActions.MoveUp) && downActions.Contains(GameActions.MoveRight))
    {
        ActionRequested(GameActions.MoveUpRight);
    }
    else if ...
}

I'm sure you can see how this can get quickly out of hand. How can I make it better?

var pressedKeys = keyboardState.PressedKeys();
var inputActions = pressedKeys.Select(GetInputAction).ToList();
var downActions = inputActions.Select(ia => ia.Down);

This is what's painted you into the corner you're in. You're not showing your GameActions enum (a better name would be GameAction, or even better, MoveDirection..actually, Direction suffices, since all there is to it is a bunch of values that each represent a direction - the point is, enum type names should not be plural), but I presume it looks something like this:

public enum GameActions
{
    MoveLeft,
    MoveUpLeft,
    MoveUp,
    MoveUpRight,
    MoveRight,
    MoveDownRight,
    MoveDown,
    MoveDownLeft
}

The problem is that MoveUpLeft is a combination of MoveUp and MoveLeft, and since the enum type itself doesn't account for it, your code has to.

I'd suggest using a [Flags] attribute:

[Flags]
public enum Direction
{
    Left = 1,
    Top = 2,
    Right = 4,
    Down = 8
}

Then, instead of downActions being an IEnumerable<GameActions> and using LINQ IEnumearble<T>.Contains extension, you can add the values of all "down actions", and pass ActionRequested an enum value that represents the sum of all pressed directions, be it Left+Top, Down+Right or whatever. The [Flags] attribute merely serves as a visual cue to remind you that these values can be combined and, using bitwise AND operations or simply, the .HasFlag() method, the handler can determine whether the received value 3 contains Direction.Left:

void ActionRequested(Direction direction)
{
    if (direction.HasFlag(Direction.Left))
    {
        // direction contains Direction.Left
    }
    if (direction.HasFlag(Direction.Right))
    {
        // direction contains Direction.Right
    }
    if (direction.HasFlag(Direction.Top))
    {
        // direction contains Direction.Top
    }
    if (direction.HasFlag(Direction.Bottom))
    {
        // direction contains Direction.Bottom
    }
}

It's not clear from your post exactly what GetInputAction does and what downActions means, so it's hard to recommend a change that will fit into your code, but here's some inspiration:

class Program
{
    [Flags]
    public enum Direction
    {
        Left = 1,
        Top = 2,
        Right = 4,
        Bottom = 8
    }

    static void Main(string[] args)
    {
        var direction = (Direction)3;
        Console.WriteLine("Direction: {0}", direction);
        Console.WriteLine("Left: {0}", direction.HasFlag(Direction.Left);
        Console.WriteLine("Top: {0}", direction.HasFlag(Direction.Top);
        Console.WriteLine("Right: {0}", direction.HasFlag(Direction.Right);
        Console.WriteLine("Bottom: {0}", direction.HasFlag(Direction.Bottom);

        Console.ReadLine();
    }
}

Output:


Abstract classes with operator overloading I'm following a YouTube video and am wondering if there are any other efficient ways of doing this. Is it possible to do it with an interface too?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Learning
{
    class Program
    {
        static void Main(string[] args)
        {
            Shape Rect = new Rectangle(5, 3);
            Shape Tri = new Triangle(8, 12);
            Rectangle CombinedRectangle = new Rectangle(3, 3) + new Rectangle(6, 6);
            Console.WriteLine(CombinedRectangle.Area());
        }

        abstract class Shape
        {
             public abstract double Area();

        }

        class Triangle : Shape
        {
            private double theBase { get; set; }
            private double theHeight { get; set; }

            public Triangle(double theBase, double theHeight)
            {
                this.theBase = theBase;
                this.theHeight = theHeight;
            }

            public override double Area()
            {
                return (theBase * theHeight) / 2;
            }
        }

        class Rectangle : Shape
        {
            private double Length { get; set; }
            private double Width { get; set; }

            public Rectangle(double Length, double Width)
            {
                this.Length = Length;
                this.Width = Width;
            }

            public override double Area()
            {
                return Length * Width;
            }

            public static Rectangle operator+ (Rectangle r1, Rectangle r2)
            {
                double combinedWidth = r1.Width + r2.Width;
                double combinedLength = r1.Length + r2.Length;
                return new Rectangle(combinedLength, combinedWidth);
            }
        }

    }
}

To answer your direct question, yes an interface would be better than an abstract class.  Plus you may want to consider that Area could be a property rather than a method.

To address things you didn't ask about:

For the Triangle class, you may want to give simpler names to the base and height.  So Base instead of theBase and Height instead of theHeight.  And they should be public properties, perhaps with private setters.

public double Base { get; private set; }
public double Height { get; private set; }

For Rectangle class, the Length and Width could also be public.

public double Length { get; private set; }
public double Width { get; private set; }

Also the constructor should use lower casing for the arguments:

public Rectangle(double length, double width)

The operator could be shortened:

public static Rectangle operator+ (Rectangle r1, Rectangle r2)
{
    return new Rectangle(r1.Length + r2.Length, r2.Width+ r2.Width);
}

Or if you are using C# 6.0:

public static Rectangle operator+ (Rectangle r1, Rectangle r2) => new Rectangle(r1.Length + r2.Length, r2.Width+ r2.Width);

The last line uses an Expression-bodied function member found in C# 6.0.  A single line method body can be replaced with a 'lambda arrow' and you can omit the return since its implied.

Putting it altogether

Using C# version 6.0 syntax

interface IShape
{
    double Area { get; }
}

public class Triangle: IShape
{
    public double Base { get; private set; }
    public double Height { get; private set; }
    public Triangle(double theBase, double theHeight)
    {
        Base = theBase;
        Height = theHeight;
    }
    public double Area => (Base * Height) / 2;
}

public class Rectangle : IShape
{
    public double Length { get; private set; }
    public double Width { get; private set; }
    public Rectangle(double length, double width)
    {
        Length = length;
        Width = width;
    }
    public double Area => Length * Width;

    public static Rectangle operator+(Rectangle r1, Rectangle r2) => new Rectangle(r1.Length + r2.Length, r2.Width+ r2.Width);
}


PHP pages with multiple conditionally displayed search elements I'm tasked with building a PHP web application where the content displayed on each page depends on a number of variables & must be re-usable across multiple pages.

A simple example of this is a search page with multiple elements:

A "basic search" div, displayed to all users.
An "advanced search" div, displayed to some users.
A "previous search history" div, displayed to all users, relevant to only their search history. This element should auto-refresh every 60 seconds.

I need each element to be re-usable & have the ability to be refreshed independent of all other elements on the page, thus decoupled from the overall page view.

For example, on a "user_info.php" page, I may want to display the same "previous search history" div. This means I can't build everything into "search.php", otherwise I'd have to build or include "previous search history" twice - once in "search.php" and again in "user_info.php". Since the search history must auto-update, an "include" statement won't work if we use a single page view.

The approach I'm leaning towards is to start by drawing a "search template" containing a bunch of placeholders which will then be populated with HTML via XHR calls.

I'm hoping to get feedback on this approach. Please note that this is a VERY basic example which illustrates how it works. The full implementation will be more complex (ie using proper route mapping, authentication, interval timers for auto div refresh if needed, etc).

If no $_GET variables are set, let's say index.php draws this page template (assuming jQuery & Bootstrap are loaded):

        <?php if ($advancedSearchAccess) { ?>

        <?php } ?>

Then once the page has finished loading, we populate each placeholder:

$(".contentPlaceholder").each(function(e) {
    $(this).load("index.php?route=" + $(this).data("route"));
});

The divs will be populated as determined in index.php if $_GET["route"] is set. Example:

switch($_GET["route"]) {
    case "basicSearch":
        include ("basicSearch.php");
        break;
    case "advancedSearch":
        include ("advancedSearch.php");
        break;
    case "userSearchHistory":
        include ("userSearchHistory.php");
        break;
}

Now on any page I can simply include the .contentPlaceholder with the relevant data-route and that element will be displayed wherever I want, with the ability to have it re-loaded without redrawing the entire page.

I'm keen to hear any feedback on this approach; is there a better way to do this? Please keep in mind the client has requested any frameworks (React, Angular, Laravel, etc etc) not be used (Bootstrap + jQuery are allowed, hence used in my example). Also I'm aware of maximum simultaneous XHR requests; this application should have no more than 6 at a time anyway.

I think your general approach of moving towards reusable components is one that is very popular across a broad range of applications. Since you pretty much only have trivial code in your question, I will talk more towards the design.

You mentioned a polling feature for search history.  This doesn't make sense to me. You know exactly when a user performs a search, so I don't know why you would need to poll for this information. In initial page load you can load into initial page source, but for asynchronous AJAX searches you should be able to simply update the history in response to the search events.

Be very careful when designing your components to decide what content needs to be loaded on initial page view vs. what content should be loaded asynchronously.

I also find it to be an odd pattern to have this route/switch construct when you are doing nothing but including a separate file anyway. Why not make these different end points on your API?

What is the difference between basic search and advanced search? Should this really be treated as two separate components?


Does this I/O operation caused a memory leak My colleague did some code Optimise work.

Code below is from a public method of a public class (NOT STATIC CLASS OR METHOD)

code before:

                Bitmap btMap = new Bitmap(Convert.ToInt32(WebConfig.StatsOpenEmailImageWidth), Convert.ToInt32(WebConfig.StatsOpenEmailImageHeight), PixelFormat.Format32bppArgb);
                MemoryStream memStrm = new MemoryStream();

                btMap.Save(memStrm, ImageFormat.Png);
                memStrm.WriteTo(context.Response.OutputStream);

Code after:

                using (Bitmap btMap = new Bitmap(Convert.ToInt32(WebConfig.StatsOpenEmailImageWidth), Convert.ToInt32(WebConfig.StatsOpenEmailImageHeight), PixelFormat.Format32bppArgb))
                {
                    using (MemoryStream memStrm = new MemoryStream())
                    {
                        btMap.Save(memStrm, ImageFormat.Png);
                        memStrm.WriteTo(context.Response.OutputStream);
                    }
                }

Is it a necessary optimisation againt memory leak?

Absolutely, the 2nd example is the way to ensure disposable objects/resources are released for the GC. The using clause does this by enforcing the rule that the object being created must inherit from IDisposable. At the closing '}' of each using block, the object is disposed.

In the first example, you will have memory leaks occur if you don't close and dispose the MemoryStream.