Implement "Upgrades" Feature for Game I am building a simple adventure game that has "upgrades" feature.
In this game, user controls one player, that can be upgraded to increase things such as:

Amount of gold earned
Maximum health
Attack power
(more of this would be added later, for example: defense upgrade, experience, etc.)

An "upgrade" has a max level cap, effects on each level of upgrade, and the cost for each upgrade.

Here's my current approach.

This is my Upgrades class, containing all possible upgrades:

public class Upgrades {

    public static final int GOLD_LEVEL_CAP = 10;
    public static final float GOLD_EFFECT_PER_LEVEL = 0.01f;
    public static final int[] GOLD_UPGRADE_COSTS = {
        100, 200, 300, 400, 500,
        600, 700, 800, 900, 1000
    };

    public static final int HEALTH_LEVEL_CAP = 10;
    public static final int HEALTH_EFFECTS_PER_LEVEL = 100;
    public static final int[] HEALTH_UPGRADE_COSTS = {
        100, 200, 300, 400, 500,
        600, 700, 800, 900, 1000
    };

    public static final int POWER_LEVEL_CAP = 5;
    public static final float POWER_EFFECT_PER_LEVEL = 0.05f;
    public static final int[] POWER_UPGRADE_COSTS = {
        1000, 2000, 3000, 4000, 5000
    };

}

And this is the class that stores the current upgrades: (owned by Player class)

public class UpgradeProgress {

    private int goldUpgradeLevel = 0;
    private int healthUpgradeLevel = 0;
    private int powerUpgradeLevel = 0;

    public void upgradeGold() {
        if (goldUpgradeLevel < Upgrades.GOLD_LEVEL_CAP) {
            goldUpgradeLevel++;
        }
    }

    // Getter
    public int getGoldUpgradeLevel() {
        return goldUpgradeLevel;
    }

    // and so on..
    // same for Health and Power
}

However, if I want to add a new upgrade, for example, I wanted to add a "Defense Upgrade". I should modify both Upgrades and UpgradeProgress classes.
Is this efficient and acceptable?
Or is there any better approach?

I don't know any Java, but I think I can give you some ideas that may help you refine this code and make it easier to extend.  Take a look at my question about skill upgrades on this site, because I got some very good feedback on this and I think it is relevant to your situation.

Hard coding all of those values into the Upgrades class limits you in many ways.  It is nice that everything is in in one place for the sake of easily editing it all at once, but as you note it is forcing you to change data in multiple places if you want to extend the class at all, or even if you want to add new kinds of Upgrades to the game.

What I suggest (which is also the suggestion made to me in the question I linked above) is to make a base Upgrade class (though possibly with a different name), and then make one for each of the Upgrades that you want in your game.  Give it all the values that you are assigning in the Upgrades class, and give it the logic that you have in the UpgradeProgress class.  This way, all of your Upgrades will have the same underlying properties and functionality, and you can create a limitless number of them without having to change anything.  Just instantiate them with the values you want.

You may need a different base class for DefenseUpgrade or any other Upgrade that has different logic inside.  It may or may not be a good idea to have it inherit from the original class, it just depends on how many of the properties and methods are the same. In general though, I would recommend composition over inheritance whenever possible.

This code may be wrong because I don't know Java but here is the basic idea I am suggesting (syntax improved by @Larethian):

public abstract class Upgrade {

    private int LEVEL_CAP;
    private float EFFECT_PER_LEVEL;
    private int[] UPGRADE_COSTS;

    private int currentUpgradeLevel = 0;

    protected Upgrade(int levelCap, float effectPerLevel, int[] upgradeCosts){
        LEVEL_CAP = levelCap;
        EFFECT_PER_LEVEL = effectPerLevel;
        UPGRADE_COSTS = upgradeCosts;
    }

    public void increaseUpgradeLevel() {
        if (currentUpgradeLevel < LEVEL_CAP) {
            currentUpgradeLevel++;
        }
    }

    // Getter
    public int getUpgradeLevel() {
        return currentUpgradeLevel;
    }

    public int getTotalEffect() {
        return effectPerLevel*currentUpgradeLevel;
    }

    //could have getters or setters for any properties necessary
}

The Player would either have a few of these upgrades and would call the necessary logic to increase their levels when appropriate, or the Player could have another class such as UpgradeCollection that could contain all of the Upgrades inside it.


Python script to calculate the Collatz conjecture Here is the code.

# Collatz.py - The Collatz conjecture
initiate = True
while True:
    if initiate == True:
        initiate = False
        try:
            number = float(input('Enter a number: '))
        except:
            print('Error! Please try again.')
            number = 1
    if number % 2 == 0:
        number /= 2
    elif number == 1:
        if input('Quit? (y/n)') == 'y':
            break
        else:
            initiate = True
    else:
        number *= 3
        number += 1
    print(number,end='\n----\n')

The output does not start with the number I enter, which I find a bit surprising.  Rather, the output starts with the following number in the Collatz sequence.

The Collatz sequence deals with integers, so why convert the input to a float rather than an int?  To catch invalid input, you should catch just ValueError rather than every kind of error.  Otherwise, you wouldn't even be able to exit the program using CtrlC.  Also, since you are using Python 3, you should use the // operator for integer division instead of the / operator, which does floating-point division.

The initiate flag variable is awkward.  Also, the code for calculation, number input, and restarting are all mingled together.  I recommend splitting the Collatz calculation code into a generator function:

def collatz_seq(n):
    yield n
    while n != 1:
        n = (3 * n + 1) if (n % 2) else (n // 2)
        yield n

while True:
    try:
        start_num = int(input('Enter a number: '))
        for n in collatz_seq(start_num):
            print(n, end='\n====\n')
    except ValueError:
        print('Error! Please try again.')
    if 'y' == input('Quit (y/n)? '):
        break


Ricochet Challenge: Robust IO and working with different types of numbers in Haskell Background

I've tried to solve /r/DailyProgrammer Challenge #303 [Easy]: Ricochet (summarised below) in Haskell. The code below is now the second Haskell script I've ever written, so I'd consider myself quite the beginner. It works (to a bare minimum), but I'm not particularly happy with it.

My knowledge in Haskell at this point spans as far as the first two chapters of the fantastic Haskell Wikibook. I've not reached functors, monads, or friends, so if the solutions to my queries lie there then it might be wisest for me to settle until I learn more.

My Queries

Whilst I'm sure there are many problems, my main concerns in the code below regard the manner in which I'm dealing with the numerical arguments to ricochet, and the fragile nature of IO.

More particularly, regarding ricochet:

I wanted the 5 arguments (namely h, w, m, n and v) all to be any kind of integer (not minding Int or Integer, etc., so long as it's a whole number, mathematically speaking), barring v which can be any number at all (i.e. 1.05).
Whilst need for integers is important (as modulo (and thus the algorithm) wouldn't make sense without them), I also want to be able to perform arbitrary arithmetic, including any kind of division.
I took me much time and frustration to come up with anything that'd even compile, for what's considerably basic arithmetic, and at the end I still find that function signature and spam fromIntegral nasty. Any advice on dealing with this kind of situation in future?

And regarding IO:

What's there now is the bare minimum to work with the challenge. I'm not happy with it because it's so fragile. If anything at all is wrong with the input, the whole program explodes.
I'd rather just output a blank line if the input is invalid, but I've no idea how to get that done.

However, I'd of course appreciate any advice at all, even around spacing/indentation!

#!/usr/bin/env stack
-- stack --resolver=lts-8.2 --install-ghc runghc

{-
Ricochet
========

- https://redd.it/5vb1wf
-}

module Main
    ( Corner(..)
    , ricochet
    , main
    ) where

data Corner = UL | UR | LR | LL deriving (Show, Eq)

ricochet :: (Integral a, Fractional b) => a -> a -> a -> a -> b -> (Corner, a, b)
ricochet h w m n v
    = (c,b,d / v)

    where
    -- What's a more idiomatic way of doing this?
    h', w', d :: Num a => a
    h' = fromIntegral (h - m)
    w' = fromIntegral (w - n)
    d  = fromIntegral $ lcm w' h'

    b  = (d * (h' + w')) `div` (h' * w') - 2
    c  =
        case (
            ((d `div` h' - 1) `mod` 2),
            ((d `div` w' - 1) `mod` 2)
        ) of
            (0,0) -> LR
            (0,1) -> LL
            (1,0) -> UR
            (1,1) -> UL

main :: IO ()
main = interact ( unlines
                . map (output . input . words)
                . lines
                )

    where
    -- How to make these less fragile and more idiomatic?
    input (h:w:m:n:v:[]) = ricochet (read h) (read w) (read m) (read n) (read v)
    output (c,b,t) = show c ++ " " ++ show b ++ " " ++ show t

Challenge Summary

Each line input describes a bounding box with dimensions h and w, and an inner box with dimensions m and n (for height and width respectively).

The inner box starts in the top-right corner (0,0) and is travelling diagonally, towards (1,1). You're told the inner box is travelling at a velocity of v units per second.

I've interpreted all of the parameters as whole numbers, other than v, which is a real number.

An example line of input: 2 4 1 2 0.5.

The inner box can ricochet off of the bounding walls as it travels.

For each line of input, calculate the first corner that the inner box reaches (UR (upper right), LR (lower right) or LL (lower left)), along with the number of times the box ricocheted off of a wall before it reached that corner, and the time taken to get there.

You can reduce the fromIntegral spam like this:

ricochet :: (Integral a, Fractional b) => a -> a -> a -> a -> b -> (Corner, a, b)
ricochet h w m n v
    = (c,b,fromIntegral d / v)

    where
    h' = h - m
    w' = w - n
    d  = lcm w' h'

The return value of functions like - and lcm is of the same type as their arguments, so the types of h' and the like can be inferred. You do need to write fromIntegral d / v because the compiler doesn't perform automatically the conversion required for the division operator. It's frequent to have these conversions functions at "type cast" boundaries.

But what if you want to keep the explicit signatures, for clarity? You can use "explicit forall" along with the ScopedTypeVariables extension, which allow you to say "this type here is the exact same type as the type in the top-level signature".

{-# LANGUAGE ScopedTypeVariables #-}
module Main
    ( Corner(..)
    , ricochet
    , main
    ) where

data Corner = UL | UR | LR | LL deriving (Show, Eq)

ricochet :: forall a b. (Integral a, Fractional b)
         => a -> a -> a -> a -> b -> (Corner, a, b)
ricochet h w m n v
    = (c,b,fromIntegral d / v)

    where
    h', w', d :: a
    h' = h - m
    w' = w - n
    d  = lcm w' h'


Unit test for Unique Position Point Here is one of my msTest Unit Tests and possibly one of the worst "looking" tests.

[TestMethod]
public void UniquePositionPoint_CreateUPP_UPPExistsInPPField()
{
    //Setup the values to be used by this test.
    CIM_Network network = new CIM_Network();
    string x = "5";
    string y = "10";
    string expectedMRID = String.Format("_UPP{0}", (x + y).GetHashCode());
    //Carry out the test
    CIM_PositionPoint pp = new CIM_PositionPoint(network, mRID: "pp", fields: new Dictionary<string, object>()
        {
                    {"cim:PositionPoint.CoordinateSystem",new CIM_CoordinateSystem(network, mRID: "coordSys")},
                    {"cim:PositionPoint.xPosition", x},
                    {"cim:PositionPoint.yPosition", y},
        }, addBackReferences:true);
    //Checks if the test was carried out correctly.
    Assert.AreEqual(2, pp.Fields.Count(),
        String.Format("ERROR: There should only be 2 fields in the position point as the x and y values should of been moved out to the UPP. Currently the fields in the pp are {0}",
        string.Join(":", pp.Fields.Select(kvp => (kvp.Key + ":" + kvp.Value.ToString())))));
    Assert.IsNotNull(pp.Fields.ContainsKey("uniquePositionPoint"), "ERROR: The field 'uniquePositionPoint:' does not exist in the position point fields.");
    Assert.IsInstanceOfType(pp.getField("uniquePositionPoint"), typeof(CIM_UniquePositionPoint),
        String.Format("ERROR: Expected the object at field 'uniquePositionPoint' to be of type CIM_UniquePositionPoint however it was of type {0}",
        pp.getField("uniquePositionPoint").GetType().Name));
    CIM_UniquePositionPoint uPP = (CIM_UniquePositionPoint)pp.getField("uniquePositionPoint");
    Assert.AreEqual(expectedMRID, uPP.MRID,
        String.Format("ERROR: Expected the mRID {0} to be the UPP's mRID but instead it was {1}", expectedMRID, uPP.MRID));
    Assert.IsTrue(uPP.BackRefs.Contains(pp), "ERROR: The position point was not found within the upp.");
}

What the code does is add a "position point" into a network. When this happens a few things also happen as a result:
1. A UniquePositionPoint (upp for short) is created.
2. That upp will contain all of the coordinates of the position point.
3. The coordinates will be removed from the position point.

What I mainly want to know is if this (a bunch of asserts) is the correct way to test this functionality or should it be broken down at all?

You listed three distinct things that happen when you perform this operation. That means you should have three distinct test cases.
Clumping them all together means:

If one of the assertions about the first thing fails, none of the insertions about the other results occur. If 1 and 3 are both failing, but 2 is succeeding, that is important information to help determine the root cause of the problem.
With the way the code is now, the following can happen. The test case fails indicating that 1 is not happening correctly. You edit the code so that 1 happens correctly. When you re-run the test case, it fails because 3 isn't happening. Was 3 never happening? Is 3 not happening now because of the changes you made to fix 1? You don't know. By using multiple test cases, you will always know what is happening and what is not.

The test case is hard to read. There are 10 lines dedicated to checking if the proper result was achieved. A person who hasn't seen the code before, should have to dig through all of that just to see what the the test case is checking for.

Ways to improve:

One test case for one distinct result. You can have multiple assertions in a test case, but they should be verifying the same concept.

Clear and descriptive test names. Ideally, the method name should tell you 1) What is being tested, 2) What are the inputs (context of the test case)? 3 What the expected result is? This way, you could already have an idea of what might be wrong before even seeing the code. Your test case is following this naming structure, but it is too generic. This is another indication that you might be checking two different concepts in the same test case.

Extract more complicated assertions into a method. Just like with real code, extracting functionality into a method is a great way to improve readability. All of your assertions have very descriptive error messaged. In general, this is not a bad thing, but it might be an indication that just looking at the assertion might not clearly indicate the intent to someone reading the code. Creating a specific method for the assertion can hide away to long string.Format() call and keep the test case clean.

Other comments:

Starting your error messages with "Error:" is redundant. The fact that this message is being shown indicates that there is an error.

Use constants to clean up your test case setup. Since you will likely have many test cases with similar inputs, constructing expectedMRID for a specific point will get repetitive.

Use variable names to indicate if input values have meaning. Are you explicitly testing 5,10 or are those just some value to represent a generic input. This could help someone else know the real intent of a test case.

Not related to the test case, but the API you are testing seems like a real pain to work with. You have to craft this specifically formated dictionary just to create an point with a specified x and y coordinates. Then to get a value, who id happens to be related to the coordinates, you have to pass in a string key to get the value, which then has to be cast to the proper type.


Merge sort a given list in Python Problem: Implement merge sort in python. Don’t use Python’s built in sort or sorted. Assume your inputs will be sufficient for the memory you have.

How should I make this code better?

def merge_sort(list_sort):
    """splits the list in two parts until each part is left with one member"""

    if len(list_sort) ==  1:
        return list_sort

    if len(list_sort)>= 2:
        x= len(list_sort) / 2
        part_a = list_sort[:x]
        part_b = list_sort[x:]
        sorted_part_a = merge_sort(part_a)
        sorted_part_b = merge_sort(part_b)
        return merge(sorted_part_a, sorted_part_b)

def merge(left , right):
    """merges the two parts of list after sorting them"""
    sorted_list = []
    i = 0
    while left[:] and right[:] :
        if  left [i] > right [i]:
            sorted_list.append(right[i])
            right.remove(right[i])

        else :
            sorted_list.append(left[i])
            left.remove(left[i])

    if left[:]:
            sorted_list.extend(left[:])
    elif right[:] :
            sorted_list.extend(right[:])
    return sorted_list

details = [1, 127, 56, 2, 1, 5, 7, 9, 11, 65, 12, 24, 76, 87, 123, 65, 8,32, 86, 123, 67, 1, 67, 92, 72, 39, 49, 12, 98, 52, 45, 19, 37, 22, 1, 66, 943, 415, 21, 785, 12, 698, 26, 36, 18, 97, 0, 63, 25, 85, 24, 94, 150 ]
print "List to be sorted  = ", details
print "Sorted List = ", merge_sort(details)

Sort algorithm

instead of if len(list_sort)>= 2:, just use else:
I would get rid of all the steps inbetween.
list_sort sounds too much like merge_sort, a method, not like a variable
to prevent surprises when using this algorithm in Python 3, use // for the floor division
you can use the fact 1 // 2 evaluates to False boolean-wise

so it'd end up like:

def merge_sort_list(lost_list):
    """splits the list in two parts until each part is left with one member"""
    x = len(lost_list) // 2
    if not x: # length is 1
        return lost_list
    else:
        return merge(merge_sort(lost_list[:x]), merge_sort(lost_list[x:]))

Merge algorithm

For reuse later, I would use a more generic algorithm using generators, which doesn't alter the items passed into it. This will also help to make it easier to adapt the sorting algorithm for other containers

def merge(left, right):
    try:
        left = iter(left)
        right = iter(right)
        l = next(left)
        r = next(right)
    except StopIteration as exc:
        raise ValueError('left and right should be iterables with at least length 1', exc)
    try:
        while True:
            if l <= r:
                yield l
                l = None
                l = next(left)
            else:
                yield r
                r = next(right)
    except StopIteration:
        yield r if l is None else l  # l is None if left is exhausted
        yield from left
        yield from right
        return

thanks to PeylonRayz for noticing the bug

Alternative version

def merge(left, right):
    try:
        left = iter(left)
        right = iter(right)
        iterators = (
            (next(left), 'left', left),
            (next(right), 'right', right),
        )
    except StopIteration as exc:
        raise ValueError('left and right should be iterables with at least length 1', exc)
    try:
        while True:
            (value, key, iterator), other = min(iterators), max(iterators)
            yield value
            iterators = other, (next(iterator), key, iterator)
    except StopIteration:
        value, key, iterator = other
        yield value
        yield from iterator

I find this alternative version is slightly more elegant, and in case of ex aequo, it keeps left before right. It can be easily adapted to support descending merging

sorting dicts

def merge_sort_dict_keys(items):
    import collections
    sorted_keys = merge_sort_list(list(items.items()))
    return collections.OrderedDict((key, value) for key, value in sorted_keys)

def merge_sort_dict_values(items):
    import collections
    sorted_keys = merge_sort_list(list((value, key) for key, value in items.items()))
    return collections.OrderedDict((key, value) for value, key in sorted_keys)


Constructing a query string using StringBuilder I speedily wrote a bunch of crappy checks, to check whether strings are empty, in order to build a query string.

StringBuilder SB = new StringBuilder();

SB.AppendFormat("{0}guide/search.aspx?", root);

root is a const, containing the website address.
It's like this because we have test servers.

if (!String.IsNullOrEmpty(item)) {
    SB.AppendFormat("&i={0}", Server.UrlEncode(item));
}
if (!String.IsNullOrEmpty(found)) {
      SB.AppendFormat("&f={0}", Server.UrlEncode(found));
}
if (!String.IsNullOrEmpty(what)) {
      SB.AppendFormat("&wt={0}", Server.UrlEncode(what));
}
if (!String.IsNullOrEmpty(where)) {
      SB.AppendFormat("&wr={0}", Server.UrlEncode(where));
}

I tried replacing these with ternaries, but I couldn't get my head around how to do that with the sb.AppendFormat in place.

Is there a better way to check empty strings than -String.IsNullOrEmpty?
Is my method of naming variables correct with C# standards?
Is Server.UrlEncode the best way to format strings into query strings?

Although there's nothing wrong with your code, it is generally easier to fall back on framework classes for this kind of thing. Namely UriBuilder and HttpValueCollection.

public static Uri BuildUri(string root, NameValueCollection query)
{
    // omitted argument checking

    // sneaky way to get a HttpValueCollection (which is internal)
    var collection = HttpUtility.ParseQueryString(string.Empty);

    foreach (var key in query.Cast<string>().Where(key => !string.IsNullOrEmpty(query[key])))
    {
       collection[key] = query[key];
    }

    var builder = new UriBuilder(root) { Query = collection.ToString() };
    return builder.Uri;
}

Then you can simply use with a name value collection:

var values = new NameValueCollection
{
    {"i", "item" },
    { "f", "found" },
    { "wt", "what%" },
    { "wr", "where" }
};
BuildUri("http://example.com/search.aspx", values);
// http://example.com/search.aspx?i=item&f=found&wt=what%25&wr=where

The HttpValueCollection handles all of the encoding for you so you don't need to worry about doing it the right way. You just create the name value collection and any null/empty values will just be skipped in the BuildUri method.

Update

As svick has pointed out in the comments it might not be ideal to use an internal class in this way. I think the risk of the implementation changing is quite low but it is a risk and should be thought about.


Is the input number even? I just wrote this C++ code in Visual Studio. The code is working fine but I wanted to ask whether my code is written in a safe way or not.

#include "stdafx.h"

#include <iostream>

int input()

{

    int x;
    std::cin >> x;

    return x % 2;

}

int main()

{

    using namespace std;

    cout << "Enter a digit" << endl;

    int x = input();

    if (input == 0)

        cout << "Even";

    else

        cout << "Odd";

    return 0;

}

Don't import stdafx

It is Windows only and you don't need it.

Remove all those spaces

The code is hard to follow when you must scroll continuously:

#include <iostream>

int input() {
    int x;
    std::cin >> x;
    return x % 2;
}

int main() {
    using namespace std;
    cout << "Enter a digit" << endl;
    int x = input();
    if (input == 0)
        cout << "Even";
    else
        cout << "Odd";
    return 0;
}

Always use braces

Ugly bugs may hide behind not using parenthesis:

if (input == 0) {
    cout << "Even";
} else {
    cout << "Odd";
}

In 2015 avoid return 0

The main() function is different; the compiler writes that for you.

Separate input and computation

If you care about having separate functions (not really necessary in this trivial case) you should define pure functions with no effect on the outside world:

bool is_even(int x) {
    return x % 2 == 0;
}

Avoid verbosity

A middle level language such as C++ must be pretty verbose, anyway for such trivial cases I propose inlining a ternary:

cout << ((is_even(x)) ? "Even\n" : "Odd\n");

Don't underestimate yourself

The program works for full numbers, ask `"Enter a number:"

The icing on the cake

using namespace std; is considered bad practice (read about that on StackOverflow)

The final reviewed code

#include <iostream>

bool is_even(int x) {
    return x % 2 == 0;
}

int main() {
    std::cout << "Enter a number:" << std::endl;
    int x; std::cin >> x;

    std::cout << is_even(x) ? "Even\n" : "Odd\n");
}


Configuring a currency converter to get exchange rate feeds for for multiple currencies I'm trying to make my function more versatile by removing hard coded data.

function get_currency_rate($currencySource) {

if (isset($currencySource)) {
        $feed = $currencySource;
    } else {
        echo 'Feed not found.  Check URL';
    }

    if (!$feed) {
        echo('Feed not found');
    }

    $xml = new SimpleXmlElement($feed);

    $rate = get_rate($xml, 15); //EUR
    if ($feed == 'http://themoneyconverter.com/rss-feed/USD/rss.xml') {
        $rate = get_rate($xml, 16); //GBP
    } else {
        $rate = get_rate($xml, 56);  //USD
    }
}

I would prefer to pass in the values 15, 16, 56 and the URL 'http://themoneyconverter.com/rss-feed/USD/rss.xml' rather than hard coding them into the function.

Below is the get_rate function.

// Get and return currency rate
// Perform regular expression to extrat numeric data
// Split title string to extract currency title
function get_rate(SimpleXMLElement $xml, $xmlTagPosition) {

    $currency['rate'] = $xml->channel->item[$xmlTagPosition]->description;
    preg_match('/([0-9]+\.[0-9]+)/', $currency['rate'], $matches);
    $rate = $matches[0];

    $title['rate'] = $xml->channel->item[$xmlTagPosition]->title;
    $title = explode('/', $title['rate']);
    $title = $title[0];

    echo $rate . ' ' . $title . '
';
}

The feed URLs are set in my cityConfig.php script as below:

// Feed URLs //
$theMoneyConverter = 'http://themoneyconverter.com/rss-feed/';

// Define arrays //
$cities = array('London', 'New York', 'Paris');
$currencyHeadings = array('London - 1 British Pound Sterling', 'New York - 1 US Dollar', 'Paris - 1 Euro');
$currencySource = array($theMoneyConverter . 'GBP/rss.xml', $theMoneyConverter . 'USD/rss.xml', $theMoneyConverter . 'EUR/rss.xml');
$currencyId = array(15, 16, 56);
?>

Notice the $currencyId array values are the same as those hard coded into the get_currency_rate function. I tried passing in a second parameter for the $currencyId to no avail. Any ideas where I'm going wrong?

I have tried the following:

Added an argument onto the end of the feed URL's e.g.

?x=15 // for EURO feed
?x=16 // for GBP feed
?x=56 // for USD feed

These numbers correspond to the position of the  XML tag as an array from the feed URL's.

$currencySource = array($theMoneyConverter . 'GBP/rss.xml?x=15', $theMoneyConverter . 'USD/rss.xml?x=16', $theMoneyConverter . 'EUR/rss.xml?x=56');

I have edited the get_rate function to parse in the URL and the PHP query.

// Get and return currency rate
// Perform regular expression to extrat numeric data
// Split title string to extract currency title
function get_rate(SimpleXMLElement $xml) {

    $vars = parse_url($xml, PHP_URL_QUERY);
    parse_str($vars);
    echo "x = " . $x;
    //$x = 15;

    $currency['rate'] = $xml->channel->item[$x]->description;
    preg_match('/([0-9]+\.[0-9]+)/', $currency['rate'], $matches);
    $rate = $matches[0];

    $title['rate'] = $xml->channel->item[$x]->title;
    $title = explode('/', $title['rate']);
    $title = $title[0];

    echo $rate . ' ' . $title . '
';
}

Well the first thing I notice is that your get_currency_rate() function requires a $currencySource to be added as a parameter. Which makes the first if statement a little redundant. The other thing is that get_currency_rate() is setting the variable $rate from get_rate(), but get_rate() doen't return anything.

I think you should ommit the get_currency_rate() function and change get_rate() to handle the XML feed request and return the information you want rather than using two separate functions.

It would look like this.

function get_rate($feed_url, $xmlTagPosition) {
  $xml = new SimpleXmlElement($feed_url);
  $xml_item = $xml->channel->item[$xmlTagPosition];

  // make sure the $xmlTagPosition was valid
  if( empty($xml_item) )
    return false;

  $currency['rate'] = $xml_item->description;
  preg_match('/([0-9]+\.[0-9]+)/', $currency['rate'], $matches);
  $rate = $matches[0];

  $title['rate'] = $xml_item->title;
  $title = explode('/', $title['rate']);
  $title = $title[0];

  return $rate . ' ' . $title;
}

And now your cityConfig.php file can work more like this.

$theMoneyConverter = 'http://themoneyconverter.com/rss-feed/USD/rss.xml';

$cities = array(
  'London' => array(
    'title' => '1 British Pound Sterling',
    'currency_id' => 15
  ),
  'New York' => array(
    'title' => '1 US Dollar',
    'currency_id' => 16
  ),
  'Paris' => array(
    'title' => '1 Euro',
    'currency_id' => 56
  )
);

// now you can easily find and loop through each of your cities
foreach( $cities as $city => $info ) {
  echo '<p>';
  echo "$city - {$info['title']}";
  echo get_rate($theMoneyConverter, $info['currency_id']);
  echo '</p>';
}


Android sharing selected text to other apps I am somewhat new to Java.  I want to know if this is the proper way to share text with other Android applications, and if I am properly determining if the text is selected.  Is there a better way to share the selected text?  Is my decision statement proper?  The app works just fine.

package com.kylelk.sharedemo;

import android.app.*;
import android.content.*;
import android.os.*;
import android.view.*;
import android.view.View.*;
import android.widget.*;

public class MainActivity extends Activity
{
    private Button shareBtn;
    private EditText textToShare;
    /** Called when the activity is first created. */
    @Override
    public void onCreate(Bundle savedInstanceState)
    {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.main);

        shareBtn = (Button) findViewById(R.id.shareText);
        shareBtn.setOnClickListener(new OnClickListener() {

            @Override
            public void onClick(View v){

                //Get the selected text
                EditText et=(EditText)findViewById(R.id.textToShare);
                int startSelection=et.getSelectionStart();
                int endSelection=et.getSelectionEnd();
                String selectedText = et.getText().toString().substring(startSelection, endSelection);

                //if no text is selected share the entire text area.
                if(selectedText.length() == 0){
                    textToShare = (EditText) findViewById(R.id.textToShare);
                    String dataToShare = textToShare.getText().toString();
                    selectedText = dataToShare;
                }

                //Share the text
                Intent sendIntent = new Intent();
                sendIntent.setAction(Intent.ACTION_SEND);
                sendIntent.putExtra(Intent.EXTRA_TEXT, selectedText);
                sendIntent.setType("text/plain");
                startActivity(sendIntent);

            }
        });
    }
}

Here is the complete project: https://github.com/kylelk/ShareDemo

Honestly I believe you have answered this question yourself: The app works just fine.

From what I can see, you are using the intent exactly as you are supposed to. You are also using EditText properly. But, because you have come all the way here I guess you want to hear some suggestions/criticism as well so I have tried really hard to come up with something:

You're inconsistent in adding spaces to your code. The best practice is to use VariableType variable = something; but in three lines you are writing VariableType variable=something; which makes the code a little bit harder to read.
I don't think you need to temporarily store the selectionStart and selectionEnd variables. (If you think it looks cleaner to store them in temporary variables, then by all means continue to do so) This code would be enough:

String selectedText = et.getText().toString().substring(et.getSelectionStart(), et.getSelectionEnd());

Or if you think that line is too long,

String text = et.getText().toString();
String selectedText = text.substring(et.getSelectionStart(), et.getSelectionEnd());

Consider what would happen if there is nothing at all stored in the EditText, do you still want to start an intent even if the string is empty?


Trying to make more simple VBA I have few models like this below for diferent tools i manage. I am trying to clean/improve the code... Could you help me?

All the informations comes inside columns B to N, and the column P concatenate it with ";". There are 6 sheets i do the same thing.

Any ideas to have a better performance and cleaner code?

Private Sub bov_mobile()
Sheets("Mobile").Select
Columns("p").Clear
fim = Cells(Rows.count, 1).End(xlUp).Row

For i = 2 To fim
Range("A" & i).Select

    If ActiveCell.Offset(0, 14).Value = "BOV" Or ActiveCell.Offset(0, 14).Value = "BOV BMF" Then
        Range("P" & i).FormulaR1C1 = "=RC[-14]&"";""&RC[-13]&"";""&RC[-12]&"";""&RC[-11]&"";""&RC[-10]&"";""&RC[-9]&"";""&RC[-8]&"";""&RC[-7]&"";""&RC[-6]&"";""&RC[-5]&"";""&RC[-4]&"";""&RC[-3]&"";""&RC[-2]&"";"""
        Range("P" & i).Copy
        Selection.PasteSpecial Paste:=xlPasteValues
        Application.CutCopyMode = False
    End If
Next i

    ' ###############  ORDER BY
    Range("p2", Cells(Rows.count, 16).End(xlUp)).Select
    Selection.Sort Key1:=Range("P2"), Order1:=xlAscending, Header:=xlGuess, _
    OrderCustom:=1, MatchCase:=False, Orientation:=xlTopToBottom, _
    DataOption1:=xlSortNormal

Range("P2", Cells(Rows.count, 16).End(xlUp)).Copy

Sheets("Mod_Bov").Select
Cells(Rows.count, 1).End(xlUp).Offset(1, 0).PasteSpecial xlPasteValues

End Sub

The quickest fix you can make to improve speed is removing all of the Select statements, and instead just referencing those selected ranges with variables. (Select slows down any VBA code significantly).  You can also toggle Application.ScreenUpdating to False before the code and True after to gain run-time speed.


Count Point in an area I created this to count points in an area.

$(document).ready(function () {
    var area = [
        [4, 4],
        [4, -4],
        [-4, -4],
        [-2, -2],
        [-2, 2],
        [-4, 4],
        [4, 4]
    ];
    var points = [
        [2, 1],
        [1, 2],
        [5, 3]
    ];
    var res = countPoints(area, points);
    $('#result').text("count : " + res);
});

function countPoints(area, points) {
    var count = 0;
    var xptsOfArea = [];
    var yptsOfArea = [];
    for (var ii = 0; ii < area.length; ii++) {
        xptsOfArea.push(area[ii][0]);
        yptsOfArea.push(area[ii][1]);
    }
    var xptsToCount = [];
    var yptsToCount = [];
    for (var ii = 0; ii < points.length; ii++) {
        xptsToCount.push(points[ii][0]);
        yptsToCount.push(points[ii][1]);
    }

    for (var ii = 0; ii < points.length; ii++) {
        if (checkFor(xptsOfArea, xptsToCount[ii], 0) && checkFor(yptsOfArea, yptsToCount[ii], 0)) {
            count += 1;
            console.log('[' + xptsToCount[ii] + ',' + yptsToCount[ii] + ']');
        }
    }
    return count;
}

function checkFor(area, point, position) {
    if (area.length == position) return true;
    else {
        if (area[position] > 0) {
            if (area[position] >= point) {
                return checkFor(area, point, ++position);
            } else {
                return false;
            }
        } else {
            if (area[position] <= point) {
                return checkFor(area, point, ++position);
            } else {
                return false;
            }
        }
    }
}

I want to make this open source, so I have three main concerns:

Naming
Complexity
Idiomaticness

As mentioned in the comments your code doesn't seem to do what you intend; however, if you take a look at the Jordan Curve theorem then one aspect of the proof demonstrates that if you consider a point and a continuous loop (or polygon) and take a ray emanating from that point then: if it intersects the loop (polygon) an odd number of times then the point is inside the loop (polygon); and if it intersects an even number of times then the point is outside the loop (polygon).

We can use this to implement a test for this property but it is useful to define some data structures first.

Starting with a namespace:

Geometery = {};

Then defining a point "class":

Geometery.Point2D = function(x,y){
    if ( isNaN( x ) || !isFinite( x ) )
    {
        throw "Point2D - x co-ordinate is not a number.";
    }
    if ( isNaN( y ) || !isFinite( y ) )
    {
        throw "Point2D - y co-ordinate is not a number.";
    }
    this.x = x;
    this.y = y;
}

Geometery.Point2D.prototype.equals = function( point ){
    return point === this
        || ( point instanceof Geometery.Point2D &&
             point.x === this.x &&
             point.y === this.y
           );
}

Geometery.Point2D.prototype.toString = function(){
    return '(' + this.x + ',' + this.y + ')';
}

Geometery.Point2D.prototype.DRAW_RADIUS = 2;

Geometery.Point2D.prototype.draw = function( context ){
    context.beginPath();
    context.arc( this.x, this.y, this.DRAW_RADIUS, 0, 2 * Math.PI, false );
    context.fill();
    context.closePath();
    context.stroke();
}

Next it is useful to have some common functionality for a set of points:

Geometery.Point2DSet = function(){
    this.points = [];

    var index;
    var point;
    for ( index = 0; index < arguments.length; index++ )
    {
        point = arguments[index];
        if ( !( point instanceof Geometery.Point2D ) )
        {
            throw "Point2DSet - Constructor argument is not a Point2D.";
        }
        this.points.push( point );
    }
};

(function(){
    var calcBoundaryCoord = function(coord,fnct){
        return function(){
            var index;
            var val = this.points.length ? this.points[0][coord] : undefined;
            for ( index = 1; index < this.points.length; ++index )
            {
                val = Math[fnct]( val, this.points[index][coord] );
            }
            return val;
        }
    };

    Geometery.Point2DSet.prototype.minX = (calcBoundaryCoord('x','min'));
    Geometery.Point2DSet.prototype.minY = (calcBoundaryCoord('y','min'));
    Geometery.Point2DSet.prototype.maxX = (calcBoundaryCoord('x','max'));
    Geometery.Point2DSet.prototype.maxY = (calcBoundaryCoord('y','max'));
})();

Geometery.Point2DSet.prototype.draw = function( context ){
    for ( var index = 0; index < this.points.length; ++index )
    {
        this.points[index].draw( context );
    }
}

Geometery.Point2DSet.prototype.toString = function(){
    return '<' + this.points.join(',') + '>';
}

This can then be extended to define a line "class":

Geometery.Line2D = function(){
    if ( arguments.length != 2 )
    {
        throw "Line2D - A line must have exactly two end-points.";
    }
    Geometery.Point2DSet.apply( this, arguments );
    if ( this.points[0].equals( this.points[1] ) )
    {
        throw "Line2D - A line must have two distinct end-points.";
    }
}

Geometery.Line2D.prototype = Object.create( Geometery.Point2DSet.prototype );
Geometery.Line2D.prototype.constructor = Geometery.Line2D;

Geometery.Line2D.prototype.getGradient = function(){
    var p1 = this.points[0];
    var p2 = this.points[1];
    if ( p1.x == p2.x )
    {
        if ( p1.y < p2.y )
        {
            return Number.POSITIVE_INFINITY;
        }
        else
        {
            return Number.NEGATIVE_INFINITY;
        }
    }
    return ( p2.y - p1.y ) / ( p2.x - p1.x );
}

Geometery.Line2D.prototype.draw = function( context, startPath, endPath ){
    if ( startPath !== false )
    {
        context.beginPath();
        context.moveTo(this.points[0].x, this.points[0].y );
    }
    context.lineTo(this.points[1].x, this.points[1].y );
    if ( endPath !== false )
    {
        context.closePath();
        context.stroke();
    }
}

We can also determine how many times a ray emanating vertically upwards from a point intersects a line:

Geometery.Line2D.prototype.countIntersectionsOfVerticalRayAbovePoint = function( point ){
    // If it is to the left/right of the bounding box then return null.
    if ( point.x < this.minX() || point.x > this.maxX() )
    {
        return 0;
    }
    // If it is above the bounding box then return ABOVE.
    if ( point.y > this.maxY() )
    {
        return 0;
    }
    // If it is below the bounding box then return BELOW.
    if ( point.y < this.minY() )
    {
        return 1;
    }
    var p1 = this.points[0];
    var p2 = this.points[1];
    // Check for the special case of a vertical line.
    if ( p1.x == p2.x )
    {
        return null;
    }
    var deltaY = point.y - this.getGradient()*(point.x - p1.x) - p1.y;
    if ( deltaY == 0 )
    {
        return null;
    }
    else if ( deltaY > 0 )
    {
        return 0;
    }
    else
    {
        return 1;
    }
}

We can then extend the concept of a set of points to be a polygon:

Geometery.Polygon2D = function(){
    if ( arguments.length < 3 )
    {
        throw "Polygon2D - A polygon must have at least 3 vertices."
    }
    Geometery.Point2DSet.apply( this, arguments );
    this.lines = [];
    var p1 = this.points[ this.points.length - 1 ];
    var p2;
    for ( var index = 0; index < this.points.length; ++index )
    {
        p2 = this.points[ index ];
        this.lines.push( new Geometery.Line2D( p1, p2 ) );
        p1 = p2;
    }
}

Geometery.Polygon2D.prototype = Object.create( Geometery.Point2DSet.prototype );
Geometery.Polygon2D.prototype.constructor = Geometery.Polygon2D;

Geometery.Polygon2D.prototype.draw = function( context ){
    for ( var index = 0; index < this.lines.length; ++index )
    {
        this.lines[index].draw( context, index == 0, false );
    }
    context.closePath();
    context.fill();
    context.stroke();
    Geometery.Point2DSet.prototype.draw.apply( this, arguments );
}

And finally we can check if a point is contained within a polygon by checking the number of intersections with lines in the polygon (and if the point is on any lines):

Geometery.Polygon2D.INSIDE   = 1;
Geometery.Polygon2D.OUTSIDE  = -1;
Geometery.Polygon2D.ONBORDER = 0;

Geometery.Polygon2D.prototype.contains = function( point ){
    var numRaysCrossing = 0;
    for ( var index = 0; index < this.lines.length; ++index )
    {
        var count = this.lines[index].countIntersectionsOfVerticalRayAbovePoint( point );
        if ( count === null )
        {
            return Geometery.Polygon2D.ONBORDER;
        }
        numRaysCrossing += count;
    }
    if ( numRaysCrossing % 2 )
    {
        return Geometery.Polygon2D.INSIDE;
    }
    else
    {
        return Geometery.Polygon2D.OUTSIDE;
    }
}

You can write your own function to then iterate over multiple points and count if they are inside/outside a polygon.

Here is a basic example of using the above:

var polygon = new Geometery.Polygon2D(
    new Geometery.Point2D( 20, 140 ),
    new Geometery.Point2D( 20, 60 ),
    new Geometery.Point2D( 60, 20 ),
    new Geometery.Point2D( 100, 60 ),

    new Geometery.Point2D( 100, 84 ),
    new Geometery.Point2D( 95, 84 ),
    new Geometery.Point2D( 95, 70 ),
    new Geometery.Point2D( 75, 70 ),
    new Geometery.Point2D( 75, 85 ),
    new Geometery.Point2D( 100, 85 ),

    new Geometery.Point2D( 100, 124 ),
    new Geometery.Point2D( 95, 124 ),
    new Geometery.Point2D( 95, 110 ),
    new Geometery.Point2D( 75, 110 ),
    new Geometery.Point2D( 75, 125 ),
    new Geometery.Point2D( 100, 125 ),

    new Geometery.Point2D( 100, 140 ),
    new Geometery.Point2D( 65, 140 ),
    new Geometery.Point2D( 65, 110 ),
    new Geometery.Point2D( 55, 110 ),
    new Geometery.Point2D( 55, 140 )
);

var canvas = document.getElementById('myCanvas');
var context = canvas.getContext('2d');
context.fillStyle   = 'red';
context.lineWidth   = 1;
context.strokeStyle = 'black';

polygon.draw( context );

var points = new Geometery.Point2DSet(
    new Geometery.Point2D( 85, 117 ),
    new Geometery.Point2D( 85, 100 ),
    new Geometery.Point2D( 85, 85 ),
    new Geometery.Point2D( 85, 78 ),
    new Geometery.Point2D( 85, 60 ),
    new Geometery.Point2D( 85, 45 ),
    new Geometery.Point2D( 85, 40 ),
    new Geometery.Point2D( 100, 100 ),
    new Geometery.Point2D( 60, 100 ),
    new Geometery.Point2D( 60, 130 ),
    new Geometery.Point2D( 10, 10 )
);

for ( var i = 0; i < points.points.length; ++i )
{
    var point = points.points[i];
    var contains = polygon.contains( point );
    if ( contains == Geometery.Polygon2D.OUTSIDE )
    {
        context.fillStyle   = 'green';
    }
    else if ( contains == Geometery.Polygon2D.ONBORDER )
    {
        context.fillStyle   = 'yellow';
    }
    else
    {
        context.fillStyle   = 'blue';
    }
    point.draw( context );
}


Is this a good way to create an article in HTML5? Is this the good way to create a portfolio article in HTML5 or do you have any suggestions?

Not sure about the section but I need it as overlay over the image. Is it better to use a DIV instead?

Ski Trail

       <time datetime="2010-03-20">20th March, 2010</time>

       Graphic

A good reference for element semantics would be HTML5 Doctor. You can read all about the tags and their intended purposes. From their overview for <section>, <article> and <div>:

Section
Represents a generic document or application section. In this context, a section is a thematic grouping of content, typically with a header, possibly with a footer. Examples include chapters in a book, the various tabbed pages in a tabbed dialog box, or the numbered sections of a thesis. A web site's home page could be split into sections for an introduction, news items, contact information.
Article
Represents a section of a page that consists of a composition that forms an independent part of a document, page, or site. This could be a forum post, a magazine or newspaper article, a Web log entry, a user-submitted comment, or any other independent item of content.
Div
The div element has no special meaning at all. It represents its children. It can be used with the class, lang, and title attributes to mark up semantics common to a group of consecutive elements.

Their analogy of <section> is a chapter, an <article> is a section of the page. Chapters contain articles, not the other way around. A <div> might be more appropriate, since it has no meaning.


Writing a class for common block of code used in many other classes In this I have written the setAnalyticsInfo() method.  The code inside this is common to almost all other (30) classes, so I have created GoogleAnalyticsCode and defined one common method and calling this method from all other classes.

import com.google.analytics.tracking.android.EasyTracker;
import com.google.analytics.tracking.android.MapBuilder;

import android.app.Activity;

public class GoogleAnalyticsCode {
    private String caption;
    private Activity activity;
    public GoogleAnalyticsCode(String caption, Activity activity) {
        super();
        this.caption = caption;
        this.activity = activity;
    }

    public void setAnalyticsInfo(){
        EasyTracker easyTracker = EasyTracker.getInstance(activity);

        if(easyTracker!=null)
        {
            easyTracker.send(MapBuilder
                    .createEvent(caption,     // Event category (required)
                            caption,  // Event action (required)
                            caption,   // Event label
                            null)            // Event value
                            .build());
        }
    }
}

I am calling setAnalyticsInfo() method like this in other classes in NavDrawer.java:

      public class NavDrawer {
    public void navigate_to_rating() {
        GoogleAnalyticsCode gac = new GoogleAnalyticsCode("Rate Us", activity);
        gac.setAnalyticsInfo();

        final String appPackageName =activity.getPackageName(); // getPackageName() from Context or Activity object
        try {
            activity.startActivity(new Intent(Intent.ACTION_VIEW, Uri.parse("market://details?id=" + appPackageName)));
        } catch (android.content.ActivityNotFoundException anfe) {
            activity.startActivity(new Intent(Intent.ACTION_VIEW, Uri.parse("http://play.google.com/store/apps/details?id=" + appPackageName)));
        }
    }
    }

My question is:

Is it good programming standard to write separate classes for common blocks of code used in many other classes?

Is it good programming standard to write separate classes for common blocks of code used in many other classes?

Yes, it is. Not repeating code over and over again is an important principle of programming.

The alternative to creating a separate class would be to create a Base activity which contains this method (in cases that you feel a separate class would be overkill). You could even call this method in the onCreate method of the Base activity, thus automatically using it each time.

In this case it's really a matter of taste. I would probably put it in a method in the Base activity, because your current GoogleAnalyticsCode class doesn't really contain any logic. It just calls EasyTracker:send().

Coding Style

Just a quick side-point concerning your coding style: You should try to be consistent with the position of your opening curly bracket, your indentation, and your spacing.


Log in to Rails app using Facebook SDK I have built a Rails app where users can login using Facebook. They have an account on the Rails app but do any authentication in it other than creating a session if they have logged into Facebook successfully.

The first part is using the SDK to get the access_token if they have logged in:

FB.getLoginStatus(function (response) {
    if(response.authResponse) {
        $.post('/auth', {access_token: response.authResponse.access_token}, function(response) {
            window.location.href = '/';
        });
    }
});

And then I pass this to my create method in my controller:

def create
    user = User.from_facebook(params[:access_token])
    session[:user_id] = user.id
end

And the model has the following methods:

def self.from_facebook(access_token)
    # get the user from the FB API
    fb_user = get_user(access_token)
    # either return or create a user from the ID
    where(id: fb_user.id).first_or_create do |user|
        user.id = fb_user.id
        user.name = fb_user.name
    end
end

private

def get_user(access_token)
    response = RestClient get "https://graph.facebook.com/me?access_token=#{access_token}"
end

So I basically either return an existing user or create a new user with the user id that comes from using the access token. This should prevent any unauthorized access and only log in the correct user as it means that the user is logged in using the access token to get the user from Facebook.

Are there any flaws in this method? I know I don't handle if the access_token is invalid, etc. I'm more interested in the security aspect of logging the user in by getting there ID from the token, but as this is all done server side, it seems pretty secure. I also don't want to use anything like Omniauth or other gems.

I've implemented a few social apps before.  What you've done is pretty standard practice when it comes to oauth authentication and is secure.

The one thing you could change:

 where(id: fb_user.id).first_or_create do |user|
   user.name = fb_user.name
 end

When you use first_or_create when it creates it'll set the where query as attributes.  So in your case the id will already be set as fb_user.id.


This snippet of scheme calculates a value in pascal's triangle I'm working through SICP and have implemented exercise 1.11 (Pascal's Triangle). What I'm curious about here is performance considerations by defining functions within the main function. I would assume they get reassigned with each invocation of the function. In saying that, this style appeals to me, as the functionality is scoped to the main function, and lexical scoping is used to effect. Obviously I'm a scheme noob so please go nuts in tearing this code apart.

(define (pascal-value row elem)

  (define (out-of-range?)
    (or (> elem row) (< elem 1)))

  (define (edge?)
    (or (= row elem) (= elem 1)))

  (cond ((edge?) 1)
        ((out-of-range?) 0)
        (else (+ (pascal-value (- row 1) (- elem 1))
                 (pascal-value (- row 1) elem)))))

Yes, those inner functions do get reassigned at each invocation, but that's cheap. In sane implementations, the inner function bodies are compiled just once, with a new lexical environment attached each time.

(In other sane implementations, the inner functions could be inlined into the outer function, but still, it's only compiled once, not for every invocation.)

So don't worry about the performance, it's not as bad as you may think. :-)


The right place to place angular modal In my app I have a BaseCtrl, which is attached to the body element and is available everywhere.
In the mentioned controller I have the following code for opening modal:

    $scope.addFolder = function($event) {
        $event.preventDefault();

        // Opens an add folder modal
        var modal = ModalService.custom({
            templateUrl: '/partials/modals/create-folder.html',
            controller: ['$scope',
                function($innerScope) {
                    $innerScope.modal = modal;

                    $innerScope.create = function() {
                        var result = NodesService.createFolder($innerScope.new_folder, $scope.location);
                        result.then(function(resp) {
                            modal.close();
                            ModalService.flash('CREATED_SUCCESSFULLY');
                            $rootScope.$emit('update_nodes');
                        }, function(err) {
                            $innerScope.create_folder_error = Locales.get(err.data.message);
                        });
                    };

                }
            ]
        });
    };

I thought it's the most appropriate place to put this snippet, since I need to be able to access it app-wide.
My question is should I separate this method in different controller or should I place it in a factory/service? What is the right place to put such code in angular?

This depends on how the rest of your app feels about the function you're publishing and how modular you want your app to be. If you want absolute modularity, you'll put the functionality in a service and inject & publish it anywhere you want to use it. The same goes for functionality that is reused just in some very specific areas, not really by the whole application.

Sharing functionality using scope hierarchy (especially by putting it on the outer-most scope) is just a way to simplify your code by sacrificing transparency (of dependencies), so in the end, it's up to you to evaluate whether the gain is worth the price in your particular situation.


Java program to detect the Java class File Format magic number, in a given file Description:

I wrote a Java program that checks whether the content of the given file begins with the Java class File Format magic number 0xCAFEBABE, in big-endian byte order.

It prints a success message if:

The first 4 bytes match the magic number

It prints an error message if:

The file doesn't exist or cannot be read
The file contains less than 4 bytes
The first 4 bytes don't match the magic number

It prints a usage message if:

A file is not given as input

Code:

import java.io.*;

public class Magic {

    public static void main(String[] args) {
        if (args.length < 1) {
            System.out.println("Usage: java Magic <FILE>");
            return;
        }

        try {
            if (isMagicPresent(args[0])) {
                System.out.println(String.format("'%s' contains the magic number.", args[0]));
            } else {
                System.out.println(String.format("'%s' doesn't contain the magic number.", args[0]));
            }
        } catch (IOException ex) {
            System.err.println(ex.getMessage());
        }
    }

    private static boolean isMagicPresent(String file) throws IOException {
        try (InputStream instream = new FileInputStream(file)) {
            byte[] bytes = new byte[4];
            int readCount = instream.read(bytes);

            if (readCount == 4) {
                return Byte.toUnsignedInt(bytes[0]) == 0xCA &&
                        Byte.toUnsignedInt(bytes[1]) == 0xFE &&
                        Byte.toUnsignedInt(bytes[2]) == 0xBA &&
                        Byte.toUnsignedInt(bytes[3]) == 0xBE;
            }
        }
        return false;
    }

}

Question:

How can I improve the above code in terms of:

Readability
Performance
Boundary case and error handling

Interface

For clarity, isMagicPresent() should accept a java.io.File (or java.nio.file.Path) to emphasize that the argument is a path and not the contents of a file.

The default constructor should be suppressed.

Implementation

To read a big-endian int, use DataInputStream.readInt().  Then you can compare the magic number to 0xCAFEBABE rather than four bytes.

Driver

I recommend returning a non-zero exit code to indicate that an error has occurred.

When no argument is given, the error message should go to System.err.  (If you supported a -h or --help option, then its output should go to System.out, because that would be the requested output.)

Instead of System.out.println(String.format(…)), you should just call System.out.format(…).  Here, I've chosen to combine both cases into one print statement, but you don't have to.

Suggested implementation

import java.io.*;

public class Magic {
    private Magic() {}

    public static boolean isMagicPresent(File f) throws IOException {
        try (InputStream inStream = new FileInputStream(f);
             DataInputStream dataInStream = new DataInputStream(inStream)) {
            return dataInStream.readInt() == 0xCAFEBABE;
        }
    }

    public static void main(String[] args) {
        if (args.length < 1) {
            System.err.println("Usage: java Magic <FILE>");
            System.exit(1);
        }

        File f = new File(args[0]);
        try {
            System.out.format(
                "'%s' %s the magic number.%n",
                f,
                isMagicPresent(f) ? "contains" : "doesn't contain"
            );
        } catch (IOException ex) {
            System.err.println(ex.getMessage());
            System.exit(1);
        }
    }
}


Go Web API for Sending Emails via Contact Form I've been playing around with Go and finally found a use case for a little web service I could build.
It takes JSON data via an HTTP POST request and sends me an email via Mailgun's API (it's used for a contact form on my website).
I'm new to Go and would love some feedback on what I could do better, anything I missed and where I could do better following the "Go way" of doing things. :)
main.go
package main

import (
    "fmt"
    "log"
    "net/http"
    "os"

    "github.com/gorilla/handlers"
    "github.com/gorilla/mux"
)

var mailgunDomain string = os.Getenv("MAILGUN_DOMAIN")
var mailgunAPIKey string = os.Getenv("MAILGUN_API_KEY")
var allowedOrigin string = os.Getenv("ALLOWED_ORIGIN")
var port string = os.Getenv("PORT")

func main() {
    if port == "" {
        panic("Missing environment variable PORT")
    }
    if mailgunDomain == "" {
        panic("Missing environment variable MAILGUN_DOMAIN")
    }
    if mailgunAPIKey == "" {
        panic("Missing environment variable MAILGUN_API_KEY")
    }
    if allowedOrigin == "" {
        panic("Missing environment variable ALLOWED_ORIGIN")
    }

    r := mux.NewRouter()

    r.Use(handlers.CORS(
        handlers.AllowedHeaders([]string{"Content-Type"}),
        handlers.AllowedOrigins([]string{allowedOrigin}),
    ))

    r.HandleFunc("/", homeHandler).Methods("GET")
    r.HandleFunc("/contact", HandleContact(mailgunDomain, mailgunAPIKey)).Methods("POST", "OPTIONS")

    log.Fatal(http.ListenAndServe(":"+port, r))
}

func homeHandler(w http.ResponseWriter, r *http.Request) {
    w.WriteHeader(http.StatusOK)
    fmt.Fprintln(w, "API for maxschmitt.me")
}

contact.go
package main

import (
    "context"
    "encoding/json"
    "fmt"
    "net/http"
    "os"
    "time"

    "github.com/mailgun/mailgun-go/v4"
)

var mailgunSender string = "email@sender.com"
var mailgunReceiver string = "email@receiver.com"

// HandleContact sends emails to me via the contact form
func HandleContact(mailgunDomain string, mailgunAPIKey string) http.HandlerFunc {
    type body struct {
        Name    string `json:"name"`
        Email   string `json:"email"`
        Message string `json:"message"`
    }

    mg := mailgun.NewMailgun(mailgunDomain, mailgunAPIKey)
    mg.SetAPIBase(mailgun.APIBaseEU)

    return func(w http.ResponseWriter, r *http.Request) {
        var b body

        err := json.NewDecoder(r.Body).Decode(&b)
        if err != nil {
            http.Error(w, err.Error(), http.StatusBadRequest)
            return
        }

        message := "From: " + b.Name + "<" + b.Email + ">\n\n"
        message += b.Message

        fmt.Fprintln(os.Stdout, "## New message!")
        fmt.Fprintln(os.Stdout, message+"\n")

        mgMsg := mg.NewMessage(mailgunSender, "New contact form submission", message, mailgunReceiver)
        mgMsg.SetReplyTo(b.Email)

        // Create a 10s timeout for sending the message
        ctx, cancel := context.WithTimeout(context.Background(), time.Second*10)
        defer cancel()

        resp, id, err := mg.Send(ctx, mgMsg)
        if err != nil {
            fmt.Println("Error sending contact form submission:")
            fmt.Println(err)
            http.Error(w, "Something went wrong", http.StatusInternalServerError)
            return
        }

        fmt.Fprintln(os.Stdout, "Mailgun response:", resp)
        fmt.Fprintln(os.Stdout, "Mailgun ID:", id)

        json.NewEncoder(w).Encode(b)
    }
}

I appreciate any feedback! Thanks!

A couple thoughts on contact.go.
Global Variables
In general, it's advised to not use global variables like
var mailgunSender string = "email@sender.com"
var mailgunReceiver string = "email@receiver.com"

Instead, there are a few options. You already have a struct definition inside the HandleContact closure, so you could just move them there. You could also pass them as arguments to HandleContact().
The other alternative could be to define a Mailgun struct with those as fields (and could probably include the mailgunDomain and mailgunAPIKey as well). Then define HandleContact as a method on the Mailgun struct, and use the handler by first creating a new Mailgun with the appropriate values like so:
main.go
mg := Mailgun{
    Sender:   "email@sender.com",
    Receiver: "email@receiver.com",
    Domain:   mailgunDomain,
    ApiKey:   mailgunAPIKey,
}
r.HandleFunc("/contact", mg.HandleContact()).Methods("POST", "OPTIONS")

contact.go
type Mailgun struct {
    Sender:   string
    Receiver: string
    Domain:   string
    ApiKey:   string
}
func (m *Mailgun) HandleContact() http.HandlerFunc {
    type body struct {
        Name    string `json:"name"`
        Email   string `json:"email"`
        Message string `json:"message"`
    }

    mg := mailgun.NewMailgun(m.Domain, m.APIKey)
    //etc...

I think the biggest advantage to setting it up this way is that it becomes very testable. You can inject whatever settings are appropriate for testing.
Printing
fmt.Printf() outputs to Stdout by default, so the lines
fmt.Fprintln(os.Stdout, "## New message!")
fmt.Fprintln(os.Stdout, message+"\n")
// ...
fmt.Fprintln(os.Stdout, "Mailgun response:", resp)
fmt.Fprintln(os.Stdout, "Mailgun ID:", id)

are functionally the same as
// Note 2 newlines to keep output identical to your implementation
fmt.Printf("## New message!\n %s\n\n", message)
// ...
fmt.Printf("Mailgun response: %s\n", resp)
fmt.Printf("Mailgun ID: %s\n", id)

Errors
Don't forget to handle the error from json.NewEncoder(w).Encode(b) at the end of contact.go. As the code is written I don't see any reason why it should ever return an error, you're just re-encoding the body you just decoded and echoing it back to the client. But if you ever change your code and the body is modified in some way it could fail silently and lead to debugging headaches.
Also wondering what the purpose of re-encoding is, you could just store and re-use the original value of r.Body. Did you mean to encode and return the response from Mailgun?


Find anagrams in a list Write a program that, given a word and a list of possible anagrams,
  selects the correct sublist.

  Given "listen" and a list of candidates like "enlists" "google" "inlets" "banana" the program should return a list containing "inlets".

Here is my solution:

class Anagram(word: String) {
  val sorted_word = word.toLowerCase.sorted
  val origin_word = word.toLowerCase

  def matches(words: Seq[String]) = {
    words.filter(w => w.toLowerCase.sorted == sorted_word && w.toLowerCase != origin_word)
  }

}

Here is the gist with specs.

Looks good! Just one little thing that I, personally would do to remove one .toLowerCase call per item: map it first. So, instead of calling .toLowerCase in the filter, you can do this:

words.map(w => w.toLowerCase)
     .filter(w => w.sorted == sorted_word && w != origin_word)

You've also got a random newline before the end of the class.

Aside from that little niggle, it looks good! There's... really not much I can say. It's short code.


Transform String a into b You can perform the following operation on some string a:

  Capitalize zero or more of a's lowercase letters at some index i (i.e., make them uppercase).
  Delete all of the remaining lowercase letters in a.

  Given string a, print YES if it can be transformed to string
  b.

  String a has only capital and small letter alphabets.

  String b has only capital letters.

  THIS is the link
  to the problem.

  Example:

  a: daBcd

  b: ABC

  OUTPUT: YES

This passed all the test-cases. But I wanted to know if any further optimizations can be made or not. Maybe I am using redundant conditions or something like this.

 //n is the length of the string a
 //m is the length of the string b
 public static void func(char[] a,int n, char[] b, int m)
    {
        int[][] mat = new int[n + 1][m + 1];
        for(int i = 0 ; i < n + 1 ; i++)
            mat[i][0] = 0;
        for(int i = 0 ; i < m + 1 ; i++)
            mat[0][i] = 0;
        char capStart = 65;
        char capEnd = 90;
        char smallStart = 97;
        char smallEnd = 122;
        char diff = 32;
        for(int i = 1 ; i < n + 1 ; i++)
        {
            for(int j = 1 ; j < m + 1 ; j++)
            {
                if(a[i - 1] >= smallStart)
                {
                    if(a[i - 1] - diff == b[j - 1])
                    {
                        if(mat[i - 1][j] == j)
                            mat[i][j] = mat[i - 1][j];
                        else
                            mat[i][j] = 1 + mat[i - 1][j - 1];
                    }
                    else
                    {
                        if(mat[i - 1][j] == j || mat[i][j - 1] == j)
                            mat[i][j] = j;
                        else
                            mat[i][j] = Math.max(mat[i - 1][j],mat[i][j - 1]);
                    }
                }
                else
                {
                    if(a[i - 1] == b[j - 1])
                    {
                        mat[i][j] = 1 + mat[i - 1][j - 1];
                    }
                    else
                    {
                        mat[i][j] = mat[i][j - 1];
                    }
                }
            }
        }
        if(mat[n][m] == m)
            System.out.println("YES");
        else
            System.out.println("NO");
    }

If you want, you can check your solution for 1 of the sample inputs:

Input format:

First line contains an integer q showing the number of pairs of a and b. Next 2q lines for a and b.

10
Pi
P
AfPZN
APZNC
LDJAN
LJJM
UMKFW
UMKFW
KXzQ
K
LIT
LIT
QYCH
QYCH
DFIQG
DFIQG
sYOCa
YOCN
JHMWY
HUVPW

Output:

YES
NO
NO
YES
NO
YES
YES
YES
NO
NO

int[][] mat = new int[n + 1][m + 1];
        for(int i = 0 ; i < n + 1 ; i++)
            mat[i][0] = 0;
        for(int i = 0 ; i < m + 1 ; i++)
            mat[0][i] = 0;

Consider

        int[][] mat = new int[n + 1][m + 1];
        Arrays.fill(mat[0], 0);
        for (int i = 1; i < mat.length; i++) mat[i][0] = 0;

Now we no longer initialize mat[0][0] twice.  We initialize it only once because we exclude it from the for loop.

We use the built-in to initialize the first row.

We initialize the rest of the first column with a for loop, as the built-in doesn't work with columns.

Since we are using the single statement version of the for loop, we put the single statement on the same line as the for loop.  Now it's obvious that it's just a single statement.  Before it was less obvious.

I would actually prefer to always use the block form, but that's your choice.

I changed from n + 1 to mat.length because it is more robust in the face of future changes.  Consider what happens if you were to change the n + 1 in the first line to n + 2.  In the original code, you would have to change the second line as well.  In this version, you don't.

One might argue that you would never change the 1 to a 2 in this code.  But even if that is so here, that might not be true in other code.  It is a good habit to try to avoid having code parallel other code.  If possible, make the code follow the other as it does here.  The mat.length will always be the right value here while the n + 1 is dependent on the logic used now.

I'm not crazy about the name mat.  I'm guessing it's short for matrix, but matrix of what?  It seems like there could be a more descriptive name.

Note that given the way that Java defaults integer values, the initialization is actually unnecessary.  Java will initialize the whole thing to 0.  You don't have to do any explicit initialization.  Java will do what you want automatically.

        char capStart = 65;
        char capEnd = 90;
        char smallStart = 97;
        char smallEnd = 122;
        char diff = 32;

You only use two of these

        char smallStart = 'a';
        char diff = 'A' - 'a';

I find this more readable.  By substituting 'a' for the numeric value, it is clearer about what we seek.  Although I'd probably just change

                if(a[i - 1] >= smallStart)

to

                if (a[i - 1] >= 'a')

or

                if (Character.isLowerCase(a[i - 1]))

That last may be longer, but it is also more robust against other alphabets than the latin1 alphabet.  And it may be better optimized as well.

        for(int i = 1 ; i < n + 1 ; i++)
        {
            for(int j = 1 ; j < m + 1 ; j++)

Given the frequency with which you say i - 1 and j - 1, I'd consider saying

        for (int i = 0; i < a.length; i++)
        {
            for (int j = 0; j < b.length; j++)

I believe that this reads more naturally.  For each letter in a and for each letter in b, do...

Don't forget to change all the i - 1 to i and all the i to i + 1 if you do this.


Histogram of length of words in text This my first real program in python. I welcome harsh criticism.  I tried to make it as efficient as possible. Trying to balance speed and readability as well.  My program takes a file of text and parses and counts the frequency of the length of each word.
An important specification I left out is I'm using 2.7 or earlier and not 3.x.

This example below isn't perfect for my code b/c I scale my y axis by 20's so if the frequency isn't higher then it won't be printed out. I wanted to demonstrate quickly what it was doing. Otherwise copy and paste this into a file and load it as an argument. http://www.archives.gov/exhibits/charters/declaration_transcript.html

For example:

  If the this was the text that I used to parse into my file as an argument.

This would be the dictionary:

  1:1, 2:5, 3:3, 4:6, 5:1, 8:1

I then take this data make a histogram. I count "&" as a word of zero length. I used a time function to determine the speed, just to give me idea of what was faster.

  #!/usr/bin/python
"""
The program reads all the input, splitting it up into words, and computes the length of each word. Punctuation marks should not be included as a part of the word, so "it's" should be counted as a three-character word, and "final." should be counted as a five-character word.
The example text includes a "word" of zero length (the "tr(x) + " | ")
"); your solution should handle this.
When all input has been processed ,the program should print a table(Histogram) showing the word count for each of the word lengths that has been encountered.
"""

import sys
import time
from string import punctuation as punc
from string import maketrans as trans
import operator
lengthDict = {}

def insert_Dict(word):
    lengthDict[word] = lengthDict.get(word, 0) + 1

def timing(f):
    def wrap(*args):
        time1 = time.time()
        ret = f(*args)
        time2 = time.time()
        print '%s function took %0.3f ms' % (f.func_name, (time2-time1)*1000.0)
        return ret
    return wrap

def roundup(x):
    return x if x % 100 == 0 else x + 100 - x % 100

@timing
def main():
    fileName = sys.argv[1]

    openFile = open(str(sys.argv[1]))

    readString = openFile.read()

    out = readString.translate(trans("",""), punc)

    for word in readString.split():
       if word in punc:
           #print(word)
           insert_Dict(0)

    for word in out.split():
        #print(word)
        insert_Dict(len(word))

    maxDomain = max(lengthDict.keys())
    maxRange = max(lengthDict.values())
    roundedMaxRange = roundup(maxRange)

    for x in range(roundedMaxRange,0, -20):
        newStr = ""
        if x % 100 == 0:
            newStr = newStr + str(x) + " -|"
        elif x % 20 == 0:
            newStr = newStr + "     |"
        else:
            newStr = newStr + "      "

        for domain in range(maxDomain + 1):
            if lengthDict.get(domain) >= x and lengthDict.has_key(domain) :
                    newStr = newStr + ('***')
            else:
                newStr = newStr + '   '

        print(newStr)

    print('     -|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|-')
    print '      0',
    for x in xrange(1, 16):
        print "",
        print x,

    print

if __name__ == '__main__':
    print('This program is being run by itself')
    main()
else:
    print('I am being imported from another module')

Reinderien has covered the PEP-8 style issues, so here's some input on the logic.

Running the program (with the half of your question that isn't the code as an input) gives the following result:

This program is being run by itself
100 -|
     |
     |
     |
     |      ***   ***
     -|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|-
      0  1  2  3  4  5  6  7  8  9  10  11  12  13  14  15
main function took 0.185 ms

So two obvious issues are that the X-axis should be one character further right and the numbers lose alignment.

Per the specification you should be printing a table, not a histogram.

Also, when adjacent columns are both filled it could get a bit confusing, perhaps a single-width column would be better?

100 -|
     |
     |
     |
     |       *     *
      -|--|--|--|--|--|--|--|--|--|--|--|--|--|--|--|-
       0  1  2  3  4  5  6  7  8  9  10 11 12 13 14 15

Short texts don't appear on the chart at all, and long texts could have more than 100 words of a given length - consider normalising your counts to the proportion of the input.

You are replicating functionality that already exists in Python: collections.Counter would save you a lot of work. Keys that don't have counts will automatically return zero, for example, and you don't need insert_Dict:

out = read_string.translate(None, punc)
length_dict = Counter(map(len, out.split(" ")))

Note that explicitly splitting on a single space means you don't have to handle the single punctuation characters separately:

>>> "foo & ba'r".translate(None, punc).split(" ")
['foo', '', 'bar']

Your main should look more like:

def main():
    with open(sys.argv[1]) as f:
        data = f.read()
    counts = create_counter(data)
    proportions = normalise_counts(counts) # if required
    draw_histogram(proportions)

Splitting the other functionality out into separate functions.

Your histogram-drawing code uses a lot of explicit string concatenation. It is better to .format and .join strings (see string methods), for example:

print '       {0}'.format("".join(map("{0:<3}".format, range(16))))

Which produces:

       0  1  2  3  4  5  6  7  8  9  10 11 12 13 14 15


Finding the max sequence finder Problem Statement

  Find the max sequence finder.

  findMaxSequence([3, 2, 3, 4, 2, 2, 4]);

  findMaxSequence([3, 5, 4, 6, 1, 2, 3, 6, 10, 32]);

  findMaxSequence([3, 2, 1]);

  Expected Output

  [ 2 , 3 , 4 ]

  [1, 2, 3, 6, 10, 32]

  no

Solution

            function findMaxSequence(array, start = -1, end = -1, i = 0, results=[]){
                if((array.length == 1) || (array.length == 0)){
                        return "[ " + array + " ]";
                }
                else if(i >= array.length-1){
                       if(start != -1){
                            results.push(array.slice(start, end+1));
                       }
                       if(results.length > 0){
                         results.sort(
                            function(a, b){
                                return a.length < b.length;
                            }
                         );
                         return "[ " + results[0].toString() + " ]";
                       }else{
                         return "no";
                       }
                }else if(array[i] < array[i+1]){
                       if (start == -1){
                            start = i;
                       }
                       return findMaxSequence(array, start, i+1, ++i, results);
                }else if(array[i] >= array[i+1]){
                      if(start != end){
                        results.push(array.slice(start, end+1))
                      }
                      return findMaxSequence(array, -1, -1, ++i, results);
                }

            }
            console.log(findMaxSequence([3,2,3,4,2,2,4]));
            console.log(findMaxSequence([3, 5, 4, 6, 1, 2, 3, 6, 10, 32]));
            console.log(findMaxSequence([3,2,1]));
            console.log(findMaxSequence([1]));
            console.log(findMaxSequence([2, 2, 2, 2]));
            console.log(findMaxSequence([1, 2, 2, 3, 3, 4]));

Actual output

[ 2,3,4 ]
[ 1,2,3,6,10,32 ]
no
[ 1 ]
no
[ 1,2 ]

A per the above solution, Did I understand the problem statement? Wrt two elements being equal in an array? Did I handle two equal size sequences correctly?
Can we make the code more elegant? I see lot of nested conditions, which makes it less readable.
Is it an elegant approach of using results array? Please let me know, if there is any better approach.

Note: Above problem is pulled from here. Tested on firefox browser, Chrome could not support default argument syntax

Tested on firefox browser, Chrome could not support default argument syntax

I think it's good to write code that runs anywhere.
You can make this code run anywhere with a simple modification:

Rename findMaxSequence to findMaxSequenceInner and remove the default values
Create a new findMaxSequence function

Like this:

function findMaxSequence(array) {
    return findMaxSequenceInner(array, -1, -1, 0, []);
}

function findMaxSequenceInner(array, start, end, i, results){
    // ...
}

Instead of this:

if((array.length == 1) || (array.length == 0)){

It would be better to combine those conditions:

if (array.length <= 1) {

When all branches of if-else-if-else conditions return, you can simplify to multiple ifs, like this:

function findMaxSequenceInner(array, start, end, i, results){
    if (array.length <= 1) {
        return "[ " + array + " ]";
    }
    if (i >= array.length - 1) {
        if (start != -1) {
            results.push(array.slice(start, end+1));
        }
        if (results.length > 0) {
            results.sort(
                function(a, b) {
                    return a.length < b.length;
                }
            );
            return "[ " + results[0].toString() + " ]";
        } else {
            return "no";
        }
    }
    if (array[i] < array[i+1]) {
        if (start == -1) {
            start = i;
        }
        return findMaxSequenceInner(array, start, i+1, ++i, results);
    }
    if (array[i] >= array[i+1]) {
        if (start != end) {
            results.push(array.slice(start, end+1))
        }
        return findMaxSequenceInner(array, -1, -1, ++i, results);
    }
}

Looking at this part:

if (array[i] < array[i+1]) {
    if (start == -1) {
        start = i;
    }
    return findMaxSequenceInner(array, start, i+1, ++i, results);
}
if (array[i] >= array[i+1]) {
    if (start != end) {
        results.push(array.slice(start, end+1))
    }
    return findMaxSequenceInner(array, -1, -1, ++i, results);
}

Notice that the two conditions perfectly complement each other.
So you can remove the second condition completely, as it will always be true if the first one was false:

if (array[i] < array[i+1]) {
    if (start == -1) {
        start = i;
    }
    return findMaxSequenceInner(array, start, i+1, ++i, results);
}
if (start != end) {
    results.push(array.slice(start, end+1))
}
return findMaxSequenceInner(array, -1, -1, ++i, results);

Once again, nesting level is reduced, which is easier to read.

Lastly, there are some formatting issues:

The indentation is inconsistent
The spacing is too tight, for example }else if(array[i] >= array[i+1]){ is easier to read as } else if (array[i] >= array[i+1]) {, that is, spaces inserted around braces

In the above examples I already adjusted the indenting and spacing, I hope you see the difference from your original.


Finding the smallest positive number evenly divisible by a range of numbers This method finds the smallest positive number that is evenly divisible by all numbers from 1 to 20.

I am wondering if there is a better way to test if all iterations of a for loop completed successfully. When I put the found boolean outside the for loop, it was terminating my while loop prematurely since the break in the for loop goes to the end and executes whatever is below the for loop inside the while loop. It also seems a bit sloppy to subtract the number that is given and then add 1 to it inside the while loop. But if I put the number++ below the for loop after found is set true and I don't need to increment the number anymore. It increments one last time.

For example, for the number 20, the answer is 232792560. But due to the last increment it would return 232792561, which is an incorrect answer.

public int getLowestDiv(int n) {
    //initialize found variable, as we havent found the number yet
    boolean found = false;
    /* initilize the number as what was passed to this method minus 1, since we will increment below*/
    int number = n - 1;

    while (found != true) {
        number++;

        for (int i = 1; i <= n; i++) {
            //for 1 to int that was passed divide the current number and see if its divisible
            if (number % i != 0) {
                break;
            } else if (i == n)
                found = true;
        }
    }
    return number;
}

I have looked through the specs for Java's loops and didn't find anything about handling these type of situations. Specifically, I will loop for a certain number of iterations, testing if something is true, and if not, break, but only execute a particular statement if the for loop passed the condition on all iterations. I would like to see what the status-quo is when it comes to this as I've run into it before.

There are two parts to my initial assessment of your code.

Firstly, often the 'best' way to solve a problem where you are tempted to have a found = true type situation is to call a seperate method for that part of the work, and to simply 'return value' when you find the value... for example, your code:

     for (int i = 1; i <= n; i++) {
         //for 1 to int that was passed divide the current number and see if its divisible
            if (number % i != 0) {
             break;

         } else if (i == n)
             found = true;

     }

would be easily written as:

private static final boolean checkValue(final int value, int n) {
    while (n > 1) {
        if (value % n != 0) {
            return false;
        }
        n--;
    }
    return true;
}

then your main loop becomes:

int number = n;

while (!checkValue(number, n)) {
     number++;
}
System.out.printf("Number %d is divisible by all positive values up to %d\n", number, n);

The second issue I have is for performance..... you should not be using number++, you will save a lot of time if you only check the multiples of the largest number, so:

number += n; // instead of number++;

EDIT

If you look at the math, you will see that, we can do much better than number += n, and can determine that if we find a value that is a multiple of both n and n - 1 that the final result will be a multiple of that value. If we can use that value as the 'step' then we will go faster than just using n. Similarly, if we can find a multiple of all n, n - 1, and n - 2 then that would be a good step too.

So, consider the following:

private static final int checkValue(final long value, int n) {
    while (n > 1) {
        if (value % n != 0) {
            return n;
        }
        n--;
    }
    return 1;
}

This loop now returns the value at which the expression fails....

and we can use that in our main loop as follows:

    final int n = 23;

    long number = n;

    long step = n;
    int fact = n;
    int bestfact = fac;
    while (number > 0 && (fact = checkValue(number, n)) > 1) {
        if (bestfac > fact) {
            bestfac = fact;
            step = number;
        }
        number += step;
    }

    if (number < 0) {
        System.out.println("No values were found (overflow) divisible by all " + n + " factors");
    }

On my computer, the difference between this optimization and a naieve step, is 0.022ms vs 4961.271ms  (or 225000 times faster!!!!!!!)


Binary tree with C++11 smart pointers I'm trying to replace the use of raw pointers with smart pointers in my C++ code. The following bit is from my first attempt at a self-balancing binary tree, though there is nothing self-balancing at the moment. I am worried about some fundamental problems regarding the use of unique pointers.

template <typename K, typename D>
class rbtree {
    struct rbnode;
    typedef std::unique_ptr<rbnode> node_ptr;
    node_ptr root_;

    struct rbnode {
        node_ptr left, right;
        rbnode *parent;
        enum { Red, Black } color;
        D data;
        K key;
        rbnode():
            left(nullptr), right(nullptr), parent(nullptr),
            color(Red), data(0), key(0) {}
        rbnode(K k, D d):
            left(nullptr), right(nullptr), parent(nullptr),
            color(Red), data(d), key(k) {}
        rbnode(K k, D d, rbnode *p):
            left(nullptr), right(nullptr), parent(p),
            color(Red), data(d), key(k) {}
    };

    node_ptr insert_(node_ptr& node, node_ptr& parent, K& key, D& data);
    public:

        bool insert(K& key, D& data);
        void dfs(std::function<void (K& key, D& data)> visitor);
};

template <typename K, typename D>
void rbtree<K, D>::dfs(std::function<void (K& key, D& data)> visitor) {
}

template <typename K, typename D>
typename rbtree<K, D>::node_ptr rbtree<K, D>::insert_(
        node_ptr& node, node_ptr& parent, K& key, D& data) {
    if (node == 0) { node = node_ptr(new rbnode(key, data, parent.get())); }
    if (key == node->key) { throw "Key exists!"; }
    if (key < node->key) {
        node->left = std::move(insert_(node->left, node, key, data));
    } else {
        node->right = std::move(insert_(node->right, node, key, data));
    }
    return std::move(node);
}

template <typename K, typename D>
bool rbtree<K, D>::insert(K& key, D& data) {
    root_ = std::move(insert_(root_, root_, key, data));
    root_->color = rbnode::Black;
    return true;
}

Unless I missed something, it appears that bool rbtree<K, D>::insert(K& key, D& data) always returns true. It is quite misleading. If there is no way for this function to ever return anything but true, you could as well get rid of the bool return type.

Also, I propose a different approach than Loki's one for the default constructors of rbnode: you can use in-class members initializers for the default values:

struct rbnode
{
    node_ptr left { nullptr };
    node_ptr right { nullptr };
    rbnode *parent { nullptr };
    enum { Red, Black } color = Red;
    D data;
    K key;

    // Etc...
};

This way, your default constructor can be reduced to its simplest form:

rbnode() = default;

And the "default" values from the in-class member initializers will also be used to automatically initialize non-initialized members in the other constructors:

rbnode(K k, D d):
    data{d},
    key{k}
{}

rbnode(K k, D d, rbnode* p):
    parent{p},
    data{d},
    key{k}
{}

Note that I also used list initialization for some of the in-class members initializers and for the constructor initialization list. List initialization can help to avoid some problems related to implicit narrowing conversions of values.


Get Image From Stream, TCP Stream without converting to MemoryStream first IS it possible to read directly from the TCPStream to get an image if I know the size and all that, without first saving it to a memorystream?

My code currently works with putting the first 4 byte in the TCP stream as a length for the entire data, so first I send the length. And then I know the length so I recieve it.

But I am saving it to a MemoryStream first, and it would save time to get it directly from the Stream if possible.

  tempBytes = new byte[length];
  ms = new MemoryStream(tempBytes);

     int currentPosition = 0;
     while (currentPosition < length)
     {
     currentPosition += tt1.GetStream().Read(tempBytes, currentPosition, length - currentPosition);
     }
     newImage = Image.FromStream(ms);

  currentPosition = 0;
  while (currentPosition < intsize)
   {
    currentPosition += tt1.GetStream().Read(lenArray, currentPosition, intsize - currentPosition);
   }

    length = BitConverter.ToInt32(lenArray, 0);

So this is in a while loop, so you can see, I get the length (well I get it at the end, but I have already got it before the while loop for the first run). Then I read the length of data into a memorystream, then get the image.

But as a NetworkStream is a Stream already, there should be possible to just read it directly.

So is there a way to do this to improve the performance?

EDIT:

Pseudo code of what I mean:

Read length from Stream:

tt1.GetStream().Read(lenArray,0,4);
length = BitConverter.ToInt32(lenArray, 0);

Read and get the Image

Image = tt1.GetStream().Read(data,0,length)

This of course doesn´t work, but it´s what I am trying to do.
I want to read directly from the stream, and as I know the length and all that, it should be possible I think, without having to save to a byte array inside a memorystream.

my though is like this, though i didn't really test it

You must keep the stream open for the lifetime of the Image.  because the Image.FromStream  wont read the data until it needs it, for performance purpose;
you must make sure the all the data for that image is received before you call Image.FromStream. TCP could split your data into multiple packages when transfer.  since you transfer 4 bytes as the length of that image, you can read the length first, then you can keep checking the length of that NetworkStream, but do not read any data.  or you can peek the data but make sure the data is still in the stream;
the last condition, do not transfer more data after the image until you are done with the image. otherwise, the data after the image could be treated as the image data.  if it is hard for your current application's protocol, you might just use a separated tcp connection for image data only.

Update:

Or I think you can derived a class from System.Stream and override the Read method and some other methods as well.  Now you can manage how you read the data.  This way, you can even work on just the same TCp connection because you know the length.   Just pass that derived Stream to the Image.FromStream, that should/might work.