More efficient way to make a Twitter status in a string of just words I am making an application where I will be fetching tweets and storing them in a database. I will have a column for the complete text of the tweet and another where only the words of the tweet will remain (I need the words to calculate which words were most used later).

How I currently do it is by using 6 different .replaceAll() functions which some of them might be triggered twice. For example I will have a for loop to remove every "hashtag" using replaceAll().

The problem is that I will be editing as many as thousands of tweets that I fetch every few minutes and I think that the way I am doing it will not be too efficient.

What my requirements are in this order (also written in comments down below):

Delete all usernames mentioned
Delete all RT (retweets flags)
Delete all hashtags mentioned
Replace all break lines with spaces
Replace all double spaces with single spaces
Delete all special characters except spaces

Here is a short and compilable example:

public class StringTest {

    public static void main(String args[]) {

        String text = "RT @AshStewart09: Vote for Lady Gaga for \"Best Fans\""
                + " at iHeart Awards\n"
                + "\n"
                + "RT!!\n"
                + "\n"
                + "My vote for #FanArmy goes to #LittleMonsters #iHeartAwards"
                + " htt…";

        String[] hashtags = {"#FanArmy", "#LittleMonsters", "#iHeartAwards"};
        System.out.println("Before: " + text + "\n");

        // Delete all usernames mentioned (may run multiple times)
        text = text.replaceAll("@AshStewart09", "");
        System.out.println("First Phase: " + text + "\n");

        // Delete all RT (retweets flags)
        text = text.replaceAll("RT", "");
        System.out.println("Second Phase: " + text + "\n");

        // Delete all hashtags mentioned
        for (String hashtag : hashtags) {
            text = text.replaceAll(hashtag, "");
        }
        System.out.println("Third Phase: " + text + "\n");

        // Replace all break lines with spaces
        text = text.replaceAll("\n", " ");
        System.out.println("Fourth Phase: " + text + "\n");

        // Replace all double spaces with single spaces
        text = text.replaceAll(" +", " ");
        System.out.println("Fifth Phase: " + text + "\n");

        // Delete all special characters except spaces
        text = text.replaceAll("[^a-zA-Z0-9 ]+", "").trim();
        System.out.println("Finaly: " + text);
    }
}

(subject to further changes)

In your simple example, how are the hashtags and usernames actually derived from the tweet?

My suggestion is to tokenize the tweet by whitespaces first, then look at the individual words to determine if they must be stored ("Vote") or discarded ("#LittleMonsters").

    // Delete all RT (retweets flags)
    text = text.replaceAll("RT", "");

You do realize that this will turn text like "ART!" into just "A!" right? Tokenizing first should remedy this issue.

On a related note, Apache Incubator Storm's tutorials usually use tweets as an example to demonstrate the Big Data approach. I'm not suggesting that you need such a set-up in your context, but perhaps you can give those a quick read-through to pick up some tips.


Android close running apps I have developed a service to automatically detect whether an app has started. If the user forbid this app to start (could be chosen from a list of apps), a dialog will be shown.

public class AppStartReceiver extends Service {
    private static final int DELAY_IN_MS = 2000;
    private List<String> forbiddenApps = new ArrayList<String>();
    private BroadcastReceiver screenReceiver = null;
    private Timer timer;
    private static boolean screenOn = true;

    @Override
    public int onStartCommand(Intent intent, int flags, int startId) {

        //register screen on/off receiver
        registerScreenReceiver();

        if (forbiddenApps.isEmpty()) {
            DatabaseHelper db = new DatabaseHelper(this);
            forbiddenApps = db.getAllApps();
        }

        //start listening for app starts
        listenForAppStarts();

        return START_STICKY;
    }

    @Override
    public void onDestroy() {
        unregisterReceiver(screenReceiver);
        timer.cancel();
        super.onDestroy();
    }

    /**
     * start listening for apps to start
     * only foreground apps are being watched
     */
    private void listenForAppStarts() {
        final ActivityManager actMgr = (ActivityManager)getSystemService(Context.ACTIVITY_SERVICE);
        timer = new Timer();
        timer.scheduleAtFixedRate(new TimerTask() {

            public void run() {
                final String foregroundApp = actMgr.getRunningTasks(1).get(0).topActivity.getPackageName();
                if (forbiddenApps.contains(foregroundApp)) {
                    //forbidden app detected => show dialog
                    Intent dialogIntent = new Intent(getBaseContext(), AlertActivity.class);
                    dialogIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
                    startActivity(dialogIntent);
                }

                // stop if user turns screen off
                if(!screenOn) {
                    timer.cancel();
                }
            }
        }, 0, DELAY_IN_MS);
    }

    @Override
    public IBinder onBind(Intent arg0) {
        return null;
    }

    /**
     * register screen receiver
     * gets callback from it
     */
    private void registerScreenReceiver() {
        // create new receiver
        screenReceiver = new BroadcastReceiver(){
            @Override
            public void onReceive(Context context, Intent intent) {
                if (intent.getAction().equals(Intent.ACTION_SCREEN_OFF)) {
                    Log.i("ScreenReceiver", "Screen off");
                    screenOn = false;
                } else if (intent.getAction().equals(Intent.ACTION_SCREEN_ON)) {
                    Log.i("ScreenReceiver", "Screen on");
                    listenForAppStarts();
                    screenOn = true;
                }
            }
        };

        //register receiver
        IntentFilter filter = new IntentFilter(Intent.ACTION_SCREEN_ON);
        filter.addAction(Intent.ACTION_SCREEN_OFF);
        registerReceiver(screenReceiver, filter);
    }

}

I works perfectly, but I don't know how good it is (does it drain too much battery?). Also, I'd like to know if my programming style is ok.

Code-Style: Good
Functionality: no problem seen
Potential Bugs: only one I can see
Android-Expert-Level: I am not an android expert.
Android-Basic-Level: Everything looks sane

Bug (in jest):

People can play 1.9 seconds of angry birds, swap to a different app, and then swap back ;-)

Suggestions:

There is only one improvement I can suggest.

   private BroadcastReceiver screenReceiver = null;

This line is out-of-place. The only place it is used is in your registerScreenReceiver method. You may as well declare it in that method.

I would be tempted actually to pull the declaration out of the method and make it a final in-initialization anonymous instance:

 private final BroadcastReceiver screenReceiver = new BroadcastReceiver(){
        @Override
        public void onReceive(Context context, Intent intent) {
            if (intent.getAction().equals(Intent.ACTION_SCREEN_OFF)) {
                Log.i("ScreenReceiver", "Screen off");
                screenOn = false;
            } else if (intent.getAction().equals(Intent.ACTION_SCREEN_ON)) {
                Log.i("ScreenReceiver", "Screen on");
                listenForAppStarts();
                screenOn = true;
            }
        }
    };


String related problem from CodingBat I was trying this problem and I was wondering if there is a way to check for the edge cases inside the for-loop.

The problem statement:

  We'll say that a lowercase 'g' in a string is "happy" if there is another 'g' immediately to its left or right. Return true if all the g's in the given string are happy.

Here I am starting the loop from index 1 as I need to look back and I don't want an ArrayIndexOutOfBounds exception. The same applies for the end edge case where I need to look the following index.

To avoid this problem I'm checking these cases right after I've checked all the others, but it doesn't feel quite right. Is there a way to do it in a more compact way?

public boolean gHappy(String str) {

   if(str.length() == 1 && str.charAt(0) == 'g') return false;
   if(str.length() == 0) return true;

   for(int i = 1; i < str.length() - 1; i ++) {
     if(str.charAt(i) == 'g') {
       if(str.charAt(i - 1) != 'g' && str.charAt(i + 1) != 'g') return false;
     }
   }
 // edge cases (start-end)
 if(str.charAt(0) == 'g' && str.charAt(1) != 'g') return false;
 if(str.charAt(str.length() - 1) == 'g' && str.charAt(str.length() - 2) != 'g') return false;

 return true;
}

There's several 'special cases' in your code.

String of length 1
String of length 0
'g' at start of String
'g' at end of String

I'd recommend getting rid of these special cases.

To do this, you can check for special conditions inside the for-loop.

Loop through all the positions in the string, and check if there is a 'g' on that position. If it is, then check if the position is more than 0 (i.e. it has a char in front of it) or if the position is less than str.length - 1 (i.e. it has a char after it).

The details about how to write this can be done in oh so many ways, but here is what I would end up with:

for (int i = 0; i < str.length(); i++) {
    char ch = str.charAt(i);
    if (ch == 'g') {
        if (i > 0 && str.charAt(i - 1) == ch) {
            continue;
        }
        if (i < str.length() - 1 && str.charAt(i + 1) == ch) {
            continue;
        }
        return false;
    }
}

return true;


Project Euler #14: Longest Collatz sequence Problem :  source
Which starting number, under one million, produces the longest chain?
n → n/2 (n is even)
n → 3n + 1 (n is odd)

I want to receive advice on my code.
It is too slow... It takes 514 seconds

import math
from functools import reduce
import time
start_time = time.time()

target = int(math.pow(10,6))
first_list = list(map(lambda x : x , range(1,target+1)))
def chain_operate(given_list):
    given_list = [0 if x == 1 else x for x in given_list]
    new_list = list(map(lambda x : x/2 if x%2==0  else 3*x+1 , given_list))
    new_list = [0 if x == 1 else x for x in new_list]
    # replace 1 to 0 because of below condition : while max(new_list) != reduce(lambda x,y : x+y, new_list)

    while max(new_list) != reduce(lambda x,y : x+y, new_list): # when the list comes to (0,0,0,4,0,0), break
        new_list = chain_operate(new_list)[0] #recursive method

    return (new_list,new_list.index(max(new_list)))

print(chain_operate(first_list)[1]+1)
print("--- %s seconds ---" % (time.time() - start_time))

Your algorithm involves the following:

Make a list of size 1,000,000
Until done:
a. Make a new list of size 1,000,000
b. Make  another new list of size 1,000,000
c. Make another new list of size 1,000,000
d. Iterate over it twice
e. And then another time

That strikes me as extremely expensive to start out. Furthermore, consider all the extra work we're doing. The sequence in the problem example is:

13 --> 40 --> 20 --> 10 --> 5 --> 16 --> 8 --> 4 --> 2 --> 1

You are calculating all 10 of those chains independently. That means you're calculating the chain for 16 six times. That's an enormous amount of extra work. The actual longest chain is of length 525. That means that for the actual answer, we are doing 524 more iterations than necessary, and we end up having to create over 1500 lists of size one million. Even if most of the time, most of our elements will be 0! We can do way better by just memoizing, which also takes less memory to boot!

Just looking at some runtimes for small values of target (10x each via timeit):

target        OP
100        0.09s
1000       1.29s
10000     19.07s

A few code comments before I get into a simpler algorithm. Don't use map(). It's far more complicated than a list comprehension. These two:

first_list = list(map(lambda x : x , range(1,target+1)))
new_list = list(map(lambda x : x/2 if x%2==0  else 3*x+1 , given_list))

Can be replaced by:

first_list = range(1, target+1)
new_list = [x/2 if x%2 == 0 else 3*x+1 for x in given_list]

The latter could be wrapped further to avoid the extra list comprehension just to move your 1s to 0s but it's overkill anyway. Similarly, your:

reduce(lambda x,y : x+y, new_list)

Could be replaced by:

sum(new_list)

Better Algorithm

First, let's start with our handy memoize decorator:

def memoize(f):
    cache = {}
    def wrapper(*args):
        if not args in cache:
            cache[args] = f(*args)
        return cache[args]
    return wrapper

And then start separating our concerns. We will write a function that for ONE value of n finds the length of a chain:

@memoize
def collatz_chain(n):
    if n == 1:
        return 1
    elif n % 2 == 0:
        return 1 + collatz_chain(n/2)
    else:
        return 1 + collatz_chain(3*n+1)

And then we just need to wrap all of that:

return max(range(1, target+1), key=collatz_chain)

It's still linear time, but I end up doing FAR less work than you have to, since most of the time I'm doing almost nothing. And the numbers show it:

target        OP    memoize
100        0.09s     0.003s   ~30x
1000       1.29s     0.024s   ~54x
10000     19.07s     0.239s   ~80x


Brainfuck Interpreter I want to write an example for a language similar to Haskell called Frege. While the interpreter is conceptually easy, it is lengthy and looks still quite messy. Note that I don't want to use Parsec etc, as it isn't available yet in Frege. Please help me to improve the Haskell version.

import Data.Char

data Tape = Tape { left :: [Int], cell :: Int, right :: [Int] }

instance Show Tape where
   show (Tape ls c rs) = show [reverse ls,[c],rs]

data Op = Plus | Minus | GoLeft | GoRight | Output | Input | Loop [Op] deriving (Eq, Show)

removeComments :: [Char] -> [Char]
removeComments xs = filter (`elem` "+-<>.,[]") xs

parseOp :: [Char] -> Maybe (Op, [Char])
parseOp ('+':cs) = Just (Plus, cs)
parseOp ('-':cs) = Just (Minus, cs)
parseOp ('<':cs) = Just (GoLeft, cs)
parseOp ('>':cs) = Just (GoRight, cs)
parseOp ('.':cs) = Just (Output, cs)
parseOp (',':cs) = Just (Input, cs)
parseOp ('[':cs) = case parseOps cs of
  (prog, (']':cs')) -> Just (Loop prog, cs')
  _ -> Nothing
parseOp _ = Nothing

parseOps :: [Char] -> ([Op],[Char])
parseOps cs = go cs [] where
  go cs acc = case parseOp cs of
    Nothing -> (reverse acc, cs)
    Just (op, cs') -> go cs' (op:acc)

parse :: String -> [Op]
parse prog = case parseOps $ removeComments $ prog of
   (ops, []) -> ops
   (ops, rest) -> error $ "Parsed: " ++ show ops ++ ", Rest: " ++ rest

execute :: [Op] -> IO Tape
execute prog = exec prog (Tape [] 0 [])

exec :: [Op] -> Tape -> IO Tape
exec [] tape = return tape
exec (Plus:prog) (Tape ls c rs) = exec prog (Tape ls (c+1) rs)
exec (Minus:prog) (Tape ls c rs) = exec prog (Tape ls (c-1) rs)
exec (GoLeft:prog) (Tape ls c rs) =
  let (hd,tl) = uncons ls in exec prog (Tape tl hd (c:rs))
exec (GoRight:prog) (Tape ls c rs) =
  let (hd,tl) = uncons rs in exec prog (Tape (c:ls) hd tl)
exec (Output:prog) tape = do
  printAsChar (cell tape)
  exec prog tape
exec (Input:prog) (Tape ls _ rs) = do
  n <- getChar
  exec prog (Tape ls (digitToInt n) rs)
exec (Loop loop:prog) tape @ (Tape ls 0 rs) = exec prog tape
exec again@(Loop loop:prog) tape = do
  tape' <- exec loop tape
  exec (if (cell tape') == 0 then prog else again) tape'

uncons :: [Int] -> (Int,[Int])
uncons [] = (0,[])
uncons (x:xs) = (x,xs)

printAsChar :: Int -> IO ()
printAsChar i = putStr $ [chr i]

main = do
  tape <- execute $ parse helloWorld
  putStrLn $ "\n" ++ show tape ++ "\n"

helloWorld =
  ">+++++++++[<++++++++>-]<.>+++++++[<++++>-]<+.+++++++..+++.[-]>++++++++" ++
  "[<++++>-]<.>+++++++++++[<+++++>-]<.>++++++++[<+++>-]<.+++.------.--------." ++
  "[-]>++++++++[<++++>-]<+.[-]++++++++++."

[Edit]

parseOp can be simplified to:

ops = [('+', Plus),('-', Minus),('<',GoLeft),('>',GoRight),('.',Output),(',',Input)]

parseOp :: [Char] -> Maybe (Op, [Char])
parseOp ('[':cs) = case parseOps cs of
  (prog, (']':cs')) -> Just (Loop prog, cs')
  _ -> Nothing
parseOp (c:cs) = fmap (flip (,) cs) $ lookup c ops
parseOp [] = Nothing

Disclaimer: I know nothing about Frege, all comments apply to Haskell only.

1)

Running hlint on your code shows places where you can remove $ and brackets. Please do it!

2)

In exec, you always do exec prog tape after finishing current instruction. So you are iterating the list in some sense. This is a fold.

exec :: [Op] -> Tape -> IO Tape
exec prog tape = foldM f tape prog

  where f (Tape ls c rs) Plus    = return $ Tape ls (c+1) rs
        f (Tape ls c rs) Minus   = return $ Tape ls (c-1) rs
        f (Tape ls c rs) GoLeft  = let (hd, tl) = uncons ls in return $ Tape tl hd (c:rs)
        f (Tape ls c rs) GoRight = let (hd, tl) = uncons rs in return $ Tape (c:ls) hd tl
        f tape           Output  = printAsChar (cell tape) >> return tape
        f (Tape ls _ rs) Input   = do n <- getChar
                                      return $ Tape ls (digitToInt n) rs

        f tape again@(Loop loop) | cell tape == 0 -> return tape
                                 | otherwise -> do tape' <- exec loop tape
                                                   f tape' again

3)

printAsChar i = putStr $ [chr i]

hlint will tell you the $ is redundant:

printAsChar i = putStr [chr i]

You can use putChar:

printAsChar i = putChar (chr i)

and finally get:

printAsChar =  putChar . chr

You have a strange asymmetry - output uses chr, and input digitToInt. These are not inverses! digitToInt '0' is 0, but chr 0 is '\NUL', not '0'.

If you want to output numbers longer than 1 character, use

printAsString = putStr . show

4)

I would merge execute and exec:

 execute :: [Op] -> IO Tape
 execute = foldM f (Tape [] 0 [])
   where f = ...

5)

 putStrLn $ "\n" ++ show tape ++ "\n"

putStrLn already adds '\n" to the end, you might remove it.

6)

If you remove the requirement to print the tape (is it needed for debugging only?), you can use an infinite list:

 execute = foldM f (Tape (repeat 0) 0 (repeat 0))

and get rid of uncons:

       f (Tape (hd:tl) c rs) GoLeft  = return $ Tape tl hd (c:rs)
       f (Tape ls c (hd:tl)) GoRight = return $ Tape (c:ls) hd tl


Using a static variable inside a lambda Is using a static variable in a lambda function ok, or considered a bad practice? The code below works as intended (fills a vector with consecutive numbers).

#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;

int main()
{
    vector<int> vec(100);

    generate(vec.begin(), vec.end(), [] () { static int i = 0; return i++; });
}

Yes it is perfectly valid.

lambdas in C++ were designed to be functionally equivalent to functors that were used a lot in C++03.

So you can consider:

auto x = [state1, state2](Param1 param1, Param2 param2){/* Do Stuff */};

To be functionally equivalent to:

struct AnonClassX
{
    State1  state1;
    State2  state2;
    AnonClassX(State1 state1, State2 state2)
       : state1(state1)
       , state2(state2)
    {}
    returnValue operator()(Param1 param1, Param2 param2) const
    {
         /* Do Stuff */
    }
};
AnonClassX   x(state1,state2);

It is quite normal to use static variables in functions and methods. Lambda is just a shorthand for creating an anonymous class with state and an operator()() to make it act like a function. So it should be very normal to put static members inside it.


dput() concatenated vector with no quotes, separated by commas in R An oddly specific task - I have a dataframe with columns with values I need to concatenate in order to prepare a query for a website that requires very specific inputs: unquoted strings, separated by commas.  Here's a dput() of a subset of the dataframe with the columns needed:

d = structure(list(CodeSpace = c(1303L, 1303L, 1303L, 1303L, 1303L,
1303L), TagID = c(23041L, 23042L, 23044L, 23046L, 23043L, 23045L
)), class = "data.frame", row.names = c(NA, -6L))

The output I need is:

A69-1303-23041, A69-1303-23042, A69-1303-23044, A69-1303-23046, A69-1303-23043, A69-1303-23045

I can approximate it (though I end up with a trailing comma) with:

cat(paste0(noquote(paste("A69", d$CodeSpace, d$TagID, sep = "-")) , ","))

Is there a better (i.e. more principled) way to do this?


Generic Linked List Implementation in Java I am looking for feedback on my doubly linked list implementation that I have written in Java. I am looking for specific feedback on the efficiency of my code, as well as my usage of generics. I am pretty new to using generics in Java, and I'm not quite sure that I am using them correctly yet.

My node class is pretty basic, but I'd like to revisit my Equals and Hashcode. Am I implementing these correctly? See snippet below:

My node class's equals/hashcode:

@Override
public boolean equals(Object that){
    if(this == that)
        return true;
    if(!(that instanceof LLNode))
        return false;

    return this.data.equals(((LLNode)that).getData());
}

@Override
public int hashCode(){
    return (this.data == null ? 0 : this.data.hashCode());
}

The DoublyLinkedList class, where the linked list is actually implemented can be found here, as there is too much to post here I think. Am I missing any major functionality? Are generics used properly?

If possible I'd also like to have my test cases reviewed to see if they are strong enough, and at least somewhat well designed. Those are here.

I was unsure of how to test certain things - specifically cases where I am expecting exceptions to be thrown.

My answer only deals with the "testing" part of your question, not the review of the classes themselves.

If I can start with a question - are you expecting LLNode to be used by other classes?  Or is it only for use by DoublyLinkedList?  If it's the latter, then it's effectively private to DoublyLinkedList, whether you've declared it that way or not.  That is, it's not part of your library's API, and I wouldn't bother with separate tests for it.

Your tests for removeAt look like they're only checking that the size of the structure is correct at the end of the test, not that the correct nodes have been removed.  They really need some kind of assertion to make sure that the list has the right nodes in it afterwards.

The way you've tested exceptions is fine; although there are other ways of doing it.  I suggest you read up on the ExpectedException feature of JUnit, and see whether it's what you want to use.

Now for the controversial stuff - I've been shot down for this point of view before, but this is the voice of experience speaking.

It's hard, on reading your test, to tell what behaviour of the list has been tested, and what hasn't.  For example, you have a testRemoveAt; but without reading every line of the test, I can't tell which cases of the removeAt method have been tested - for example, (1) What if the argument is negative?  (2) What if the argument is greater than the number of things in the list? (3) What if the argument is valid? (4) What if the list is empty?.  I think you've tested (3) and (4), but not (1) and (2), but I'm not sure.

For this reason, I strongly recommend naming your tests after the behaviour that they test, NOT after the method that implements that behaviour.  This means you'll have more tests, and smaller ones, but you'll be able to see far more easily which behaviour has and hasn't been tested.  Also, in the event of a failure, it will be easier to see what has actually gone wrong.  So in this particular case, I would name the tests for removeAt something like -

@Test
public void removeAtThrowsException_withNegativeArgument(){
}

@Test
public void removeAtThrowsException_whenListIsEmpty(){
}

@Test
public void removeAtRemovesCorrectNodeFromList_whenArgumentLessThanSizeOfList(){
}

@Test
public void removeAtThrowsException_whenArgumentNotLessThanSizeOfList(){
}

As far as tests for equals go, you probably want quite a few cases.  Such as -

What happens if the argument is not a DoublyLinkedList?
What happens if the argument is a DoublyLinkedList with different elements
What happens if the argument is a DoublyLinkedList with the same elements in a different order?
What happens if the argument is a DoublyLinkedList with the same elements in the same order?
What happens if the argument is a DoublyLinkedList with equal elements (but not the same) in the same order?

Again, I recommend having each of these as a separate test method, with a descriptive name.  And the biggest benefit is that you can look at your test method names, and know which cases have and have not been tested, without having to read through the code.


Set with custom compare In my project I use std::set with custom Compare function for maintaining an event queue. It fulfills following criteria:

event duplication is not allowed
should maintain the insertion order

(tried std::vector first, but that performed poorly because of complicated duplicate removal)

The important code is:

template <typename T>
class EventQueue {
public:
  EventQueue() : set_(std::set<DataWrap>()) {}
  void Insert(T elem) {
    set_.insert(DataWrap(elem, set_.size()));
  }

  // ....

private:
  struct DataWrap {
    T data;
    unsigned int order;
    DataWrap(T d, unsigned int o) : data(d), order(o) {}
    bool operator<(const DataWrap& other) const {
      if (data == other.data)
        return false;
      else
        return order < other.order;
    }
  };
  std::set<DataWrap> set_;
};

Naturally I want to avoid undefined behaviour. As far as I can see my Compare fulfills the requirements of strick weak ordering (irreflexive, transitive, asymetric)

Can you confirm that this code is well-defined, or am I missing something? Thanks for your time!

As a side note, you pass the element by copy not by const reference. This might also have some real preformance effects. SO rather do

 void Insert(const T&)

Also you could use emplace_back to enable inplace construction of the elements in the vector. On the other hand std::find only takes const T& so I would guess, that move semantics do not really buy you anything here.


Create User profile with separate social media data I am creating user profile logic for a blogging platform.  I have the UserProfile model and then two separate models, UserSocialLink and SocialLinkType to control the user's various social media contacts (i.e. Facebook, Twitter, Google+, etc...).  I am looking for feed back on the execution of the methods used to Add new UserProfiles.  Is this the way you would handle it? Is there a better way? What are some of the issues you see with my methods (specifically in the service class)?

The Models

public class UserProfile
{
    public int Id { get; set; }
    public string UserName { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string ProfilePicUrl { get; set; }
    public string Location { get; set; }
    public string DateOfBirth { get; set; }
    public string Bio { get; set; }
    public DateTime Created { get; set; }
    public DateTime Updated { get; set; }
 }

public class UserSocialLink
{
    public int Id {get; set;}
    public int UserProfileId { get; set; }
    public int SocialLinkTypeID { get; set; }
    public string LinkValue { get; set; }
    public DateTime Created { get; set; }
    public DateTime Updated { get; set; }

    public virtual UserProfile UserProfile { get; set; }
    public virtual SocialLinkType SocialLinkType {get; set;}
}

public class SocialLinkType
{
    public int Id { get; set; }
    public string LinkType { get; set; }
    public string URLFormat { get; set; }
}

The Add Methods in The Service Class

public class UserProfileService : IUserProfileService
{
    public UserProfile Add(UserProfile userProfile)
    {
        var now = DateTime.UtcNow;

        if (userProfile == null)
            throw new ArgumentNullException("User Profile");

        userProfile.Created = now;
        userProfile.Updated = now;
        using (var db = new ApplicationDbContext())
        {
            db.UserProfiles.Add(userProfile);
            db.SaveChanges();
        }
        return userProfile;
    }

    public UserProfile AddSocialLink(string userName, UserSocialLink userSocialLink, string socialLinkType)
    {
        var now = DateTime.UtcNow;
        var userProfile = Get(userName);
        if (userProfile == null)
            throw new Exception("User Profile Not Found");
        using (var db = new ApplicationDbContext())
        {
            var linkType = db.SocialLinkTypes.SingleOrDefault(p => p.LinkType.ToUpper() == socialLinkType.ToUpper());
            if (linkType == null)
                throw new Exception("Social Media Type Not Found");
            userSocialLink.Created = now;
            userSocialLink.Updated = now;
            userSocialLink.SocialLinkTypeID = linkType.Id;
            db.UserSocialLinks.Add(userSocialLink);
            db.SaveChanges();
            return userProfile;
         }
    }
}

  Related GitHub Issue #14

public UserProfile AddSocialLink(string userName, UserSocialLink userSocialLink, string socialLinkType)

Shouldn't the socialLinkType be a property of the UserSocialLink class? It seems odd to me that the code has to pass a string along with the object in order to tell the method what type it is. That said, I really do like that you're checking the database to make sure the type is valid. Now that I think of it, why pass the user name as a string? Wouldn't it be a simpler API to pass a UserProfile in? At least, it would be more consistent for the dev using the UserProfileService.

This code is a great example of why we should all use proper indentation and braces around if statements.

var linkType = db.SocialLinkTypes.SingleOrDefault(p => p.LinkType.ToUpper() == socialLinkType.ToUpper());
if (linkType == null)
    throw new Exception("Social Media Type Not Found");
userSocialLink.Created = now;
userSocialLink.Updated = now;
userSocialLink.SocialLinkTypeID = linkType.Id;
db.UserSocialLinks.Add(userSocialLink);
db.SaveChanges();
return userProfile;

At a glance, it looks like the userSocialLink only gets it's properties set if (linkType == null) and after an exception is thrown. Of course, this is ridiculous and not what is actually happening, but braces make that crystal clear to Mr. Maintainer.

var linkType = db.SocialLinkTypes.SingleOrDefault(p => p.LinkType.ToUpper() == socialLinkType.ToUpper());
if (linkType == null)
{
    throw new Exception("Social Media Type Not Found");
}
userSocialLink.Created = now;
userSocialLink.Updated = now;
userSocialLink.SocialLinkTypeID = linkType.Id;
db.UserSocialLinks.Add(userSocialLink);
db.SaveChanges();
return userProfile;


Find/replace number with commas in Java I have a requirement to put a comma between each digit of a number in a Java string.

Example input:

the number is 12345 and the number is 45678

The output should be:

the number is 1,2,3,4,5 and the number is 4,5,6,7,8

I wrote the following and I am not sure if it is optimized. If someone reviews it and help me fine tune it, it will be helpful.

public static void main(String args[]) {
      String str = "the number is 12345 and the number is 45678";
      System.out.println("Original Text:\n" + str);
      System.out.println("Updated Text:\n" + formatNumber(str, ','));
   }

   public static String formatNumber(String text, char separator) {
       Pattern p = Pattern.compile("\\d+");
       Matcher m = p.matcher(text);
       StringBuffer sb = new StringBuffer();
       while(m.find()) {
           String num = m.group();
           num = num.replaceAll(".(?!$)", "$0"+separator);
           m.appendReplacement(sb,num);
       }
       return m.appendTail(sb).toString();
   }

Output:

Original Text:
    the number is 12345 and the number is 45678
    Updated Text:
    the number is 1,2,3,4,5 and the number is 4,5,6,7,8
    Process finished with exit code 0

Though it gave me the output I wanted, I have a feeling that it is not highly optimized when the string size grows larger and larger.

Two of the really nice things about regular expressions are:

they can be compiled, which makes them fast, and reusable
they can replace what's between characters, not just the characters themselves.

In your case, you can reduce it to a "simple" lookaround regular expression. What you are looking for, is the gap between two digits...

in a regular expression, the gap can have no width. So, look for a place that follows a digit, and is also followed by a digit. Then, replace that place with a separator.

Then, compile and save that expression away, and reuse it:

private static final Pattern BETWEEN_DIGITS = Pattern.compile("(?<=\\d)(?=\\d)");

public static String formatNumber(String text, char separator) {
    return BETWEEN_DIGITS.matcher(text).replaceAll("" + separator);
}

That pattern is complicated.... but read up on look-behinds, and look-aheads: http://www.regular-expressions.info/lookaround.html


ThreadPoolExecutor singleton I'm trying to build a ThreadPoolExecutor singleton to use it across my app. I'm very new to this concept and after going through Google docs I have created this:

public class MyThreadPoolExecutor {

    // Sets the amount of time an idle thread will wait for a task before terminating
    private static final int KEEP_ALIVE_TIME = 1;

    // Sets the Time Unit to seconds
    private static final TimeUnit KEEP_ALIVE_TIME_UNIT;

    // Sets the initial threadpool size to 8
    private static final int CORE_POOL_SIZE = 8;

    // Sets the maximum threadpool size to 8
    private static final int MAXIMUM_POOL_SIZE = 8;    // A static block that sets class fields

    private final BlockingQueue<Runnable> MyBlockingQueue;

    // A managed pool of background download threads
    private final ThreadPoolExecutor MyPoolExecutor;

    // A single instance of PhotoManager, used to implement the singleton pattern
    private static MyThreadPoolExecutor MyThreadPoolInstance = null;

    static {

        // The time unit for "keep alive" is in seconds
        KEEP_ALIVE_TIME_UNIT = TimeUnit.SECONDS;

        // Creates a single static instance of PhotoManager
        MyThreadPoolInstance = new MyThreadPoolExecutor();
    }

    private MyThreadPoolExecutor() {
        MyBlockingQueue = new LinkedBlockingQueue<Runnable>();

        MyPoolExecutor = new ThreadPoolExecutor(CORE_POOL_SIZE, MAXIMUM_POOL_SIZE,
                KEEP_ALIVE_TIME, KEEP_ALIVE_TIME_UNIT, MyBlockingQueue);
    }

    public static MyThreadPoolExecutor getInstance() {

        return MyThreadPoolInstance;
    }

    public void queueJob(Runnable r){

        MyPoolExecutor.execute(r);
    }

}

And I'm using it like this throughout the app:

            MyThreadPoolExecutor threadPool = MyThreadPoolExecutor.getInstance();
            threadPool.queueJob(new MyRunnable());

It looks fine to me, but I can't find good reference of how to do it. Is the above correct, or is it the right way to implement such a thing?

The good:

Well I was actually struggling a little to find places in your code, where you have made no mistakes whatsoever (when seen as whole), but actually there's a lot of good things in your code:

Correct use of the right data structures
Extraction of "magic numbers" to properly named constants
Correct implementation of the singleton pattern
Clean and simple initialization

But wherever there's light there's also shadows.

The bad:

You are overcommenting. Comments that merely restate what happens in the line right below them are useless noise. Comments like that don't explain anything new.
You can actually assume, that people reading your code can program, there's no necessity to spell things out for them. After all they are intelligent ;)

Instead your comments should shed light on complicated algorithms, that are hard to follow, even when written cleanly. Also important to comment is the why of code that's "different" to the standard. Further reading
You're violating naming conventions. Fields, no matter whether they are static or not, should be named in camelCase instead of pascal case. This was the reason Vladimir thought your code didn't compile.
Overly long names: MyThreadPoolInstance could just be instance. This also applies to MyPoolExecutor and MyBlockingQueue, which should be executor and queue respectively.
And while we're at the queue. It's never used. If you need it use it, if you don't need it: get rid of it.

The ugly:

There's two kinda ugly things about your code. The first being that you are misusing a static initializer block where it's completely unnecessary. Instead you could just do the eager initialization directly. Your executor and KEEP_ALIVE_TIME_UNIT do not depend on any ordering.

The second thing is that you're using the Singleton pattern. The Singleton pattern is often a misinformed choice. The thought of using a Singleton often comes from needing "a central ... for ..."

That is in 90% of the cases possible to solve without resorting to the Singleton pattern. This is especially true in your case. Often singletons get misused as a "Utility class". Enforcing Singleton use is problematic, because it can easily become a bottleneck in your application if done wrongly.

Additionally in your case (as in most) it's completely unnecessary. Instead you should strive to make your code just use the built-ins of java, as demonstrated in Vladimir's answer


Parsing data from XML using Ruby and Nokogiri I have a method that parses XML into an array of hashes.

Here is the original XML:

<rowset name="skillqueue" key="queuePosition" columns="queuePosition,typeID,level,startSP,endSP,startTime,endTime">
  <row queuePosition="0" typeID="20495" level="3" startSP="2829" endSP="16000" startTime="2013-03-04 16:25:50" endTime="2013-03-05 01:02:20"/>
  <row queuePosition="1" typeID="19767" level="4" startSP="40000" endSP="226275" startTime="2013-03-05 01:02:20" endTime="2013-03-07 06:40:31"/>
</rowset>

Here is the final array of hashes:

[{:queuePosition=>"0", :typeID=>"20495", :level=>"3", :startSP=>"2829", :endSP=>"16000", :startTime=>"2013-03-04 16:25:50", :endTime=>"2013-03-05 01:02:20"}, {:queuePosition=>"1", :typeID=>"19767", :level=>"4", :startSP=>"40000", :endSP=>"226275", :startTime=>"2013-03-05 01:02:20", :endTime=>"2013-03-07 06:40:31"}]

Here is the method, which uses Nokogiri for parsing:

def get_training_queue(xml_data)
  queue = []
  xml_data.xpath("//row").each do |skill_in_queue|
    skill = {}
    skill_in_queue.attributes.each do |details|
      skill[details[0].to_s.to_sym] = details[1].to_s
    end
    queue << skill
  end
  queue
end

This works great, but it feels a bit inelegant.

I'm curious if there is a better way to create the hashes within the internal loop?
I am calling to_s because I couldn't figure out a way to pull out just the key/value without doing so, and it 'feels' like I'm missing some more elegant way of doing that.

Notes:

Use functional Enumerable#map instead of imperative pattern obj = [] +  Enumerable#each + Array#<<.
Use Kernel#Hash to build a hash from its pairs. Note that this method is very ugly and some prefer a more OOP approach, in that case check Enumerable#mash from Facets.
Use nokogiri_node.text.
Arguments in blocks can be unpacked.

I'd write:

require 'nokogiri'
require 'facets'

def get_training_queue(xml_data)
  xml_data.xpath("//row").map do |skill_in_queue|
    skill_in_queue.attributes.mash do |name, attribute|
      [name.to_sym, attribute.text]
    end
  end
end

All those eachs and in-place updates show that you think in imperative terms instead of functional, check this wiki page.


Renaming the iframe src attribute to facilitate lazy loading I'm not great with javascript. I need some code to rename all iframe src attribute names to data-src, for a lazyload plugin to work. I couldn't find out how to rename the attribute names, but with some trial and error I managed something similar and it is working OK:

$( document ).ready( function() {

     $( 'iframe' ).each(function() {

        //store each iframe scr attribute value in a variable
        var attr_value = $(this).attr("src");

        //Add a data-src attribute with the src attribute value
        $(this).attr('data-src', attr_value);

        // remove the old src attribute
        $(this).removeAttr('src');
    });
});

Does this look OK? or is there a better way to achieve this. Any help appreciated?

Calling $(this) three times is a bit wasteful.  You can improve that by taking advantage of $.attr(attributeName, function) and by using chaining.

$(function() {
     $('iframe').attr('data-src', function() { return $(this).attr('src'); })
                .removeAttr('src');
});

// For demonstration...
$(function() {
     $('#printout').text($('iframe').attr('data-src'));
});

  data-src=


Return total balance from unsorted purchase items that may have specials This is in reference to the following kata:

  Item   Unit      Special
         Price     Price
  --------------------------
    A     50       3 for 130
    B     30       2 for 45
    C     20
    D     15

I am still trying to learn F#.
Thus, I have the following code:

module Checkout

(*Types*)
type Type = A | B | C | D
type Total = { UnitPrice:int ; Qty:int }
             member x.Price() = x.UnitPrice * x.Qty

type Special =
    | None
    | ThreeForOneThirty
    | TwoForFourtyFive

type Item =
    { Type:Type
      Total:Total
      Special:Special }

      member x.Price() =
        match x.Special with
        | ThreeForOneThirty ->
            if x.Total.Qty / 3 > 0
            then (x.Total.Qty / 3) * 130
            else x.Total.Price()

        | TwoForFourtyFive ->
            if x.Total.Qty / 2 > 0
            then (x.Total.Qty / 2) * 45
            else x.Total.Price()

        | None -> x.Total.Price()

(*Private Functions*)
let private getTypeQty itemsOfType =
    itemsOfType |> Seq.sumBy(fun x -> x.Total.Qty)

let private consolidate group acc =

    let first = group |> Seq.head
    { Type    =  first.Type;
      Total   = { Qty=group |> getTypeQty; UnitPrice=first.Total.UnitPrice }
      Special = first.Special; } :: acc

(*Tests*)
open FsUnit
open NUnit.Framework

[<Test>]
let ``buying (2) A units, (1) B unit, (1) A unit = $160`` () =

    // Setup
    let a2 = { Type=A; Total={UnitPrice=50; Qty=2}; Special=ThreeForOneThirty }
    let b =  { Type=B; Total={UnitPrice=30; Qty=1}; Special=TwoForFourtyFive  }
    let a =  { Type=A; Total={UnitPrice=50; Qty=1}; Special=ThreeForOneThirty }

    seq [a2; b; a] |> Seq.groupBy (fun item -> item.Type)
                   |> Seq.map snd
                   |> Seq.fold(fun consolidated group -> consolidate group consolidated) []
                   |> Seq.sumBy (fun item -> item.Price())
                   |> should equal 160

I don't like the way I coupled the Specials DU to an item. Thus, my implementation enables an item to reference any special. It just feels wrong...

Paraphrasing Kent Beck (IIRC), I'd design my types so that concerns that change together are kept together, whereas concerns that change separately should be kept as separate as possible.

With that in mind, I'd start defining shopping basket items as simply as possible:

type Good = { SKU : string; Price : int }

This also enables you to define the price table, independently of the discount rules:

let prices = [
    { SKU = "A"; Price = 50 }
    { SKU = "B"; Price = 30 }
    { SKU = "C"; Price = 20 }
    { SKU = "D"; Price = 15 } ]

// string -> Good option
let scan sku = prices |> List.tryFind (fun x -> x.SKU = sku)

This code snippet also defines a simple scan function that can be used to look up a string in the price table in order to turn it into a Good value.

I don't think modelling the product as a discriminated union (e.g. A | B | C | D) is a good idea, because that's going to make it hard if you want to add an E product, or if you want to remove the C product.

The same type of argument goes for price rules. List prices change according to business decisions, and the same goes for discounts, but at different rates. Discriminated unions define a finite set of options, but I don't think a set of discounts ought to be a finite set of hard-coded options. You should be able to add or remove discount rules according to business decisions.

Instead, I'd be inclined to define a discount with the type Good list -> int * Good list. The idea here is that you input a list of Good values, and you get back a price (int) and the remaining Good values where a price has yet to be calculated.

You can, for example, implement the rule about the A products like this:

// Good list -> int * Good list
let aRule items =
    let xs, others = List.partition (fun x -> x.SKU = "A") items
    let hits, rest =
        xs |> List.chunkBySize 3 |> List.partition (fun l -> l.Length = 3)
    let xTotal = hits.Length * 130
    xTotal, rest |> List.concat |> List.append others

This looks, perhaps, a bit complicated, but it works like this:

First, it partitions all items into A items (xs) and all other items (others).
It then divides xs into chunks of 3. This produces a list of lists, where most of the lists have the length 3. There may, however, be a residual list with one or two items, so these are partitioned into the rest value.
The total for all the hits is calculated. Each hit is a list of three A items, which has the special discount price 130. You may have three, six, nine, etc. A items, so the special price 130 is multiplied with the length of hits, which is a list of lists.
Finally, xTotal is returned, together with all the items that didn't trigger the discount. These include all non-A items, but also residual A items.

Likewise, the discount for B items can be defined in the same way:

// Good list -> int * Good list
let bRule items =
    let xs, others = List.partition (fun x -> x.SKU = "B") items
    let hits, rest =
        xs |> List.chunkBySize 2 |> List.partition (fun l -> l.Length = 2)
    let xTotal = hits.Length * 45
    xTotal, rest |> List.concat |> List.append others

This function is almost identical to aRule, so is a candidate for some refactoring. Here, however, I decided to apply the rule of three, so I left them as is.

Apart from these discount rules, you'll also need a 'default' price calculation rule:

// Good list -> int * 'a list
let defaultRule items = List.sumBy (fun x -> x.Price) items, []

Notice how Good list -> int * 'a list also fits the desired type of Good list -> int * Good list. This enables you to define a set of rules as a list:

// (Good list -> int * Good list) list
let rules = [aRule; bRule; defaultRule]

Calculating a total price is now easy:

// ('a -> int * 'a) list -> 'a -> int
let total rules items =
    let acc (tot, rest) f =
        let tot', rest' = f rest
        tot + tot', rest'
    rules |> List.fold acc (0, items) |> fst

This function performs a left fold over rules, while accumulating the total so far, and the rest of the items.

Tests

This implementation passes all the tests in the original kata formulation:

open Xunit
open Swensen.Unquote

[<Theory>]
[<InlineData("", 0)>]
[<InlineData("A", 50)>]
[<InlineData("AB", 80)>]
[<InlineData("CDBA", 115)>]

[<InlineData("AA", 100)>]
[<InlineData("AAA", 130)>]
[<InlineData("AAAA", 180)>]
[<InlineData("AAAAA", 230)>]
[<InlineData("AAAAAA", 260)>]

[<InlineData("AAAB", 160)>]
[<InlineData("AAABB", 175)>]
[<InlineData("AAABBD", 190)>]
[<InlineData("DABABA", 190)>]

// Incremental
[<InlineData("A", 50)>]
[<InlineData("AB", 80)>]
[<InlineData("ABA", 130)>]
[<InlineData("ABAA", 160)>]
[<InlineData("ABAAB", 175)>]
let ``total returns correct result`` (items : string) expected =
    let actual =
        items |> Seq.choose (string >> scan) |> Seq.toList |> total rules
    expected =! actual

Flexibility

Is this the simplest possible implementation? Most likely not, but it's fairly flexible.

Imagine, for example, that you're being asked to implement a new 'bundle' discount where you get A, B, and C for 85 if you buy them together. You can implement this without changing any of the existing code:

// f:('a -> bool) -> ('a list -> 'a option * 'a list)
let tryFindFirst f =
    let acc (found, others) x =
        match found with
        | Some hit -> Some hit, x :: others
        | None -> if f x then Some x, others else None, x :: others
    List.fold acc (None, []) >> (fun (found, others) -> found, List.rev others)

// Good list -> int * Good list
let bundleRule items =
    let aOpt, rest = tryFindFirst (fun x -> x.SKU = "A") items
    let bOpt, rest = tryFindFirst (fun x -> x.SKU = "B") rest
    let cOpt, rest = tryFindFirst (fun x -> x.SKU = "C") rest
    match aOpt, bOpt, cOpt with
    | Some a, Some b, Some c -> 85, rest
    | _ -> 0, items

As you can see, bundleRule has the desired type, so can be used together with the other pricing rules:

[<Theory>]
[<InlineData("ABC", 85)>]
[<InlineData("BAC", 85)>]
[<InlineData("ABBA", 145)>]
[<InlineData("ABCAA", 185)>]
let ``bundle rule is applied correctly when having high priority``
    (items : string)
    expected =

    let actual =
        items
        |> Seq.choose (string >> scan)
        |> Seq.toList
        |> total (bundleRule :: rules)
    expected =! actual

Notice the test case for ABCAA. This combination fits both the bundle discount, and the triple-A discount. Since bundleRule comes first, however, it wins, and removes ABC from further calculation, thereby correctly preventing the triple-A rule from being applied.

If your business experts ask you to prioritise the triple-A rule over the bundle rule, that's possible as well:

[<Theory>]
[<InlineData("ABC", 85)>]
[<InlineData("BAC", 85)>]
[<InlineData("ABBA", 145)>]
[<InlineData("ABCAA", 180)>]
let ``bundle rule is applied correctly when having low priority``
    (items : string)
    expected =

    let actual =
        items
        |> Seq.choose (string >> scan)
        |> Seq.toList
        |> total [aRule; bRule; bundleRule; defaultRule]
    expected =! actual

In this test, notice that the order of the rules is different. This causes aRule to trigger before bundleRule, and for that reason, the ABCAA test case has a different outcome.

State

Did I just come up with all of this myself? Not quite, I was leveraging my knowledge of functional programming 'design patterns'.

Functions with the type 's -> 'a * 's are part of the State monad. All the price functions have the type Good list -> int * Good list, which make them State 'instances'.

In fact, you can make use of the state monad easier by introducing the standard monadic functions like return, bind, map, and so on. In F#, you can further sprinkle on syntactic sugar by implementing a state computation expression. Here are some notes on doing that.

Sometimes, it helps to define a type alias for the monadic type in question:

type State<'a, 's> = ('s -> 'a * 's)

This isn't strictly necessary, but I think it makes is a bit easier to reason about some of the following code.

Next, you can define a module to implement the most important functions:

module State =
    // 'a -> State<'b,'a> -> 'b * 'a
    let run state (f : State<_, _>) = f state

    // 'a -> 'b -> 'a * 'b
    let lift x state = x, state

    // ('a -> 'b) -> State<'a,'c> -> 'c -> 'b * 'c
    let map f x state =
        let x', newState = run state x
        f x', newState

    // ('a -> State<'b,'s>) -> State<'a,'s> -> 's -> 'b * 's
    let bind (f : 'a -> State<'b, 's>) (x : State<'a, 's>) state =
        let x', newState = run state x
        run newState (f x')

    // 'a -> 'a * 'a
    let get state = state, state

    // 'a -> 'b -> unit * 'a
    let put newState _ = (), newState

    // ('a -> 'a) -> ('a -> unit * 'a)
    let modify f = get |> map f |> bind put

You can define more, but here, I only added the functions I'll need in the coming examples.

The most important functions are lift and bind. Often, in other languages, the lift function is called return, or pure, but both of these are reserved keywords in F#. These two functions define the monad. The rest are useful extra functions (apart from the map function, which could be used to define State as a Functor in Haskell).

Computation expression

Using the functions in the State module, you can implement a computation expression builder for the State monad. Again, I only add the absolute minimum required in order to make the examples below compile:

type StateBuilder () =
    // State<'c,'d> * ('c -> State<'e,'d>) -> ('d -> 'e * 'd)
    member this.Bind (s, f) = State.bind f s
    // 'a -> ('b -> 'a * 'b)
    member this.Return x = State.lift x

As you can see, the Bind and Return methods simply delegate to the State module's functions.

Refactored price functions

With these new tools, you can now refactor the above price functions.

First, I decided to implement a single function that you can use to define n for x discounts:

// int -> int * string -> (Good list -> int * Good list)
let forOnly price (n, sku) = state {
    let! xs = List.partition (fun x -> x.SKU = sku)
    let hits, residual =
        xs |> List.chunkBySize n |> List.partition (fun l -> l.Length = n)
    do! State.modify (residual |> List.concat |> List.append)
    return hits.Length * price }

If you keep in mind the definition of the State type, you can also view this function as having the type int -> int * string -> State<int, Good list>. It can be used like this to define a price rule:

// Good list -> int * Good list  .. or, alternatively:
// State<int, Good list>
(3, "A") |> forOnly 130

Notice that it's possible to use the existing, built-in function List.partition with a let! binding. This is possible because the type of List.partition is ('a -> bool) -> 'a list -> 'a list * 'a list. Notice how 'a list -> 'a list * 'a list can also be viewed as State<'a list, 'a list>. This means that the first let! expression binds xs to the items that satisfy the predicate passed to List.partition, while the items that don't match are being threaded through the computation expression as the underlying state.

Once you've found any residual, you'll need to put it back into the underlying state. You can do this with do! State.modify. Finally, you return the result of the price calculation.

The same sort of refactoring can be done for bundleRule:

// Good list -> int * Good list
let bundleRule = state {
    let! originalItems = State.get
    let! aOpt = tryFindFirst (fun x -> x.SKU = "A")
    let! bOpt = tryFindFirst (fun x -> x.SKU = "B")
    let! cOpt = tryFindFirst (fun x -> x.SKU = "C")
    match aOpt, bOpt, cOpt with
    | Some a, Some b, Some c -> return 85
    | _ ->
        do! State.put originalItems
        return 0 }

Compared to the previous incarnation of bundleRule, this implementation is actually a bit longer, but IMO more readable. Again, the underlying state is implicitly threaded through the computation, but as you can tell, you can retrieve it with State.get and 'write' it with State.put.

I find this version more readable because I don't have to declare intermediary state values (rest) and pass them around. In the previous version, if I forgot to pass them around in the correct manner, the calculation would be incorrect.

It's almost as though the State-based implementation looks more imperative. The same would be the case in Haskell, and there's a saying that Haskell is the best imperative language that exists. Here we see that F# is no slouch either.


Trie structure using Hash in Ruby This is a follow-up to Boggle board solver in Ruby

class Trie < Hash
  def build(string)
    string << '.' # Terminator indicating end of a complete word
    string.chars.inject(self) do |h, char|
      h[char] ||= {}
    end
  end
end

# Build the trie from all the words
@trie = Trie.new
File.foreach('wordlist.txt') { |w| @trie.build(w.chomp) }

Here I use a simplistic Trie structure based on a Hash in Ruby. It does the trick very nicely, but I am wondering whether there is a better way to implement this in Ruby that will consume less memory.
(It lacks lookup methods which is perfectly fine, because I track through the Trie along with the path through letters on a board - the '.' acts as a marker for a complete word).

When I load the file, which contains 270K+ words and is ~2.9MB in size, the memory consumption is over 188 megabytes.

Is there a way to reduce the memory consumption, perhaps, implementing this differently?

Bloom Filters

I suggest a new data structure, specifically a Bloom Filter.  A bloom filter is a boolean array of length m.  You will also need k different hashing algorithms (more on m and k in a moment). Each hash should accept complete word and return a number 0 through m.

A bloom filter is a big hashing data structure that returns one of two results when checking if an item is in the hash:
1) The item is definitely not in the data structure
2) The item is maybe in the data structure

To use the bloom filter, you will load it with every word in your list.  Specifically, for every word, load the first two letters, then first three, then so on, until the whole word is loaded.  I like the idea of using a period to mark the end of the word, so do that too.  For example, for the word 'cart', you would load your filter with ['ca', car', 'cart'].

Then, on the boggle array, you can do an exhaustive DFS search fromevery starting position. if the letter combination (with or without period) is in the list, then you keep branching and searching.  If the combination isn't in the list, you choose a different path, etc.

Simple concrete example

Here is a simple example of a bloom filter being used.  Let's set m=100 (array of 100 booleans, initialized to true) and k=2 (use two different hashing algorithms).

When hashing the word 'cat', hash1 might return 7 and hash2 might return 36, so you set indexes 7 and 36 to false.  When hashing the word 'dog', hash1 might return 7 and hash2 might return 70, so those indices are set to false.  Next, we check to see if 'fish' is in the array.  Hash1 returns 80 and hash2 returns 7.  Because all the indices are not false, we know that 'fish' is definitely not in the array.  Finally, we check if 'bird' is in the array.  Hash1 returns 70 and Hash2 returns 7.  'Bird' might be in the array and we will have to check the actual wordlist to confirm.

Some notes

1) You should be able to search using only the bloom filter until you are checking for a complete word (For example when checking for 'car.', and not checking 'car' as a substring of 'cart.' or 'carriage.').  Otherwise if the bloom filter says it's probably in the list, just assume it's true.
2) Depending on how thorough your edge cases are, it may be possible to occasionally miss a possible word depending on the hash algoirthms chosen, size of filter, etc.  In general, I would say that while you are not 100% guaranteed to find the highest scoring word, you will definitely find a high scoring word.
3) The bloom filter here is chosen to reduce the memory space of the search.  The running time may very well be longer than the trie structure.
4) When confirming a word exist in your word list, you should, of course, use a binary search.

Choosing m and k

There is some math involved in choosing m and k.  I'll spare you the finer details and suggest you look at wiki section about choosing optimal sizes.


Improve performance Javascript function I'm doing a BattleShip game in javascript with iio engine.

I'm trying to play against a computer so I have to put a random position for the ships (I hope you know the game :) ).

I have 5 ships that have to be placed in a grid (10x10). The problem is that the function is pretty slow, and sometimes the page don't get load at all.

I want to know if there are some emprovement for the speed of these function, I'm a little bit newbie :D

function posShips(size){
    // var size -> size of the ship
    var isOk = false; // flag var to check if the ship is in a right position
    var isOk2 = true; // flag var, become false if the cell is already fill with another ship
    var i;
    var j;
    var side; // horizontal or vertical
    while(!isOk){
        i = iio.getRandomInt(1,11);
        j = iio.getRandomInt(1,11);
        side = iio.getRandomInt(0,2);
        if((side ? j : i)+size-1 < 11){ // Not out of the array
            for (var k = 0;  k < size; k++) { // Size of the ship
                if(side){
                    if(gridHit[i][j+k].stat == "empty"){ //If is empty put the ship
                        gridHit[i][j+k].stat = "ship";
                        gridHit[i][j+k].setFillStyle("red")
                    }else{ // If not empty
                        isOk2 = false; //Position is not good, do all the thing again.
                        for (var a = 0;  a < size; a++) { // Reset cell
                            gridHit[i][j+a].stat = "empty";
                        }
                        k = 10;
                    }
                }else{
                    if(gridHit[i+k][j].stat == "empty"){ //If is empty put the ship
                        gridHit[i+k][j].stat = "ship";
                        gridHit[i+k][j].setFillStyle("red")
                    }else{ // If not empty
                        isOk2 = false; //Position is not good, do all the thing again.
                        for (var a = 0;  a < size; a++) { // Reset cell
                            gridHit[i+a][j].stat = "empty";
                        }
                        k = 10;
                    }
                }
            };
            if(isOk2)
                isOk = true;
        }
    }
}

This is not really a speed issue; there are a couple of problems with your code that prevent it from working properly.

The most serious problem is that, if the position is not good you set isok2 = false. The while loop then picks another position. But nothing resets isok2 to true. So in fact, if the function does not find a good position on the first attempt, it will continue to loop indefinitely, which is why the page does not load.

A second problem is that when the position is not good you set all of the cells to empty.

for (var a = 0;  a < size; a++) { // Reset cell
    gridHit[i][j+a].stat = "empty";
}

but this overwrites other ships that might have been in that space. A better approach would be to find an appropriate space first, and only once the whole space has been checked, start filling that space with 'ship' cells. Then you don't have to worry about resetting cells you have started filling before realising the space was blocked.

Some other comments:

(1) Instead of using the condition if((side ? j : i)+size-1 < 11) to check if the position is within the grid, it would be better to make sure that the random numbers chosen are within the grid in the first place:

i = iio.getRandomInt(1, side ? 11 : 11 - size);
j = iio.getRandomInt(1, side ? 11 - size : 11);

(2) Instead of setting k = 10 to break out of the loop, simply use break.

(3) Debug what is happening in your code by strategically placing console.log commands.

(4) It's often cleaner to use functions and return statements rather than flags. For instance, you could have a function like this within your main function:

function blocked() {
    for (var k = 0; k < size; k++) {
        if (side ? grid[i][j + k] : grid[i + k][j]) {
            return true;
        }
    }
}

Here is a jsfiddle demonstrating these points (in the function makeEnemyShip). It uses the same basic algorithm that you do and there is no speed problem.


Python exception-raising generator function I want my generator function to raise as soon as it can.  If I make it yield the elements directly, then the KeyError won't happen until iteration begins.  Is the following code the best I can do?

def cluster_terminals(self, key):
    """
    Deals with forwarding.
    """
    return_list = self.cluster_map[key]
    def internal_cluster_terminals(return_list):
        for x in return_list:
            if isinstance(x, ClusterTerminal):
                yield x
            elif isinstance(x, NodeTerminalMappingName):
                yield from x.node_terminal.cluster_terminals(x.mapping_name)
            else:
                assert False
    return internal_cluster_terminals(return_list)

In terms of raising the KeyError as soon cluster_terminals is called (rather than when its result is iterated), this general pattern is likely the best. There are a couple of other points that bear mentioning, though.

First, assert False is one of the worst ways to handle a detected error (marginally better than, say, sys.exit(0)). Make the assertion self-documenting so that if when it gets triggered sometime in the future, you have more immediate information in the stack trace about what is going on. At a minimum, include a message with the assertion:

assert False, "`return_list` contains unexpected object of type " + str(type(x))

An arguably better option is to put it up front with an explicit test, even if that means duplicating some of your logic:

assert all(isinstance(x, (ClusterTerminal, NodeTerminalMappingName)) for x in return_list)

And then adjust your conditional to either have a pass (and a short comment pointing to the assertion) in that branch, or omit that branch entirely. This makes it abundantly explicit to anyone reading your code that that is an 'impossible' situation, and means the invariants of this class have been broken (and that it isn't that client code has passed in bad data - for that, use a normal exception rather than an assertion).

A more minor quibble is that you don't need to pass return_list into the helper function as an arguments. You can use closures:

return_list = self.cluster_map[key]
def internal_cluster_terminals():
    ...
return internal_cluster_terminals()

This will behave identically - Python functions can see variables from the scope they are defined in, even if that scope no longer 'exists' when they are executed.

internal_cluster_terminals is a bit of an unwieldy name, and doesn't tell us anything we don't already know (it's.. internal to the function it's defined in. Wow, amazing). A more usual name for this type of function is something like helper. return_list isn't a great name either, especially since it's actually misleading (it isn't ever returned). Since it's a value from a mapping, call it after what those values conceptually are - cluster might be a good option.

Finally, consider adding a short comment to document why you need a helper function in the first place, like:

# Use an internal generator instead of yielding directly
# so we can bail out early if there's a KeyError


index.php implementation I'm writing a MVC app, I've put effort into writing index.php since it must be the entrance point (like main for C and Java) imho.
I would like to ensure if someone who were to work on this file wouldn't be confused.

require_once 'App/config.php'; //main constants are defined here
require_once 'App/autoload.php';

if(strpos(URL,'error/'))
    goto start;
try{
    Connection::set(DBMS,HOST,PORT,DB_NAME,DB_USER,DB_PWD,$PDO_OPTIONS);
    DAO::init();

}catch(Exception $e){
    if(!PROD){ //if the app is online PROD=TRUE
        throw $e;

    }else
        header('location: '.WEBROOT.'error/503/',503);

    exit;
}

session_start();

if(!isset($_SESSION['user']))
    $_SESSION["user"] = new Guest;

start:
    try{
        extract($_GET);
        unset($_GET);

        if(isset($controller, $action, $params))
            Dispatcher::dispatch($controller, $action, $params);

        else if (isset($controller, $action))
            Dispatcher::dispatch($controller, $action);

        else if (isset($controller, $params))
            Dispatcher::dispatch($controller, NULL, $params);

        else if (isset($controller))
            Dispatcher::dispatch($controller);

        else
            Dispatcher::dispatch();

        echo Dispatcher::deliver(); //output the response

    }catch(Throwable $t){
        if(!PROD)
            throw $t;

        else if($t instanceof TypeError && strpos($t->getTrace()[0],'Dispatcher.php'))
            header('location: '.WEBROOT.'error/400/',400);

        else if(strpos($t->getMessage(),'not found'))
            header('location: '.WEBROOT.'error/404/',404);

        else
            header('location: '.WEBROOT.'error/503/',503);
    }

Goto

Honestly, I didn't even know that PHP supported labels and goto. But this is how the manual for PHPs goto starts:

If you don't have a very good reason to use goto, just don't use it. It generally leads to bad code.

What you have is basically this:

if(strpos(URL,'error/'))
    goto start;

// init database

start:
    // init app

Or in other words:

if error:
    do not start database connection

start program

So you can rephrase your confusing goto statement like this:

if(!strpos(URL,'error/')) {
    initDatabase();
}

initApp();

function initDatabase() {
    // Connection::set(...)
}

function initApp() {
    // start session, call dispatcher, etc
}

Formatting

You should always use curly brackets, even for one-line statements. If you absolutely think that you need to omit them (you don't), be consistent. This for example isn't easy to read:

if(!PROD){ //if the app is online PROD=TRUE
    throw $e;

}else
    header('location: '.WEBROOT.'error/503/',503);

exit;

But something like this also isn't clear at first glance:

if(strpos(URL,'error/'))
    goto start;
try{

At the very least add a new paragraph. But really, curly brackets are the right thing here.

header redirect

In your case it isn't strictly necessary, but it's good practice to always die after a header redirect as a client doesn't has to follow it, so code that comes after it will be executed as well.

This may cause you problems in case you add further code later on, and expect the redirect to stop execution.

Extract

Extract is not a very good function. It's not secure to let a user have complete control over your variables, and it leads to bad code, as it's unclear what variables are defined.

Just assign the variables you need directly.

Misc

I wouldn't hard-code URLs such as your error URL. Ideally, URLs are customizable in some config file.


If you restricted your lines to 79 columns, as recommended by the Python style guide (PEP8), then we wouldn't have to scroll the code horizontally to read it here.

Anyway, all four loops have the same form:

max(grid[i][j]
    * grid[i+i_step][j+j_step]
    * grid[i+2*i_step][j+2*j_step]
    * grid[i+3*i_step][j+3*j_step]
    for i in range(i_min, i_limit)
    for j in range(j_limit))

given particular choices of parameters i_step, j_step, i_min, i_limit and j_limit. So we can implement the program as a data-driven loop over those choice of parameters:

print(max(max(grid[i][j]
              * grid[i+i_step][j+j_step]
              * grid[i+2*i_step][j+2*j_step]
              * grid[i+3*i_step][j+3*j_step]
              for i in range(i_min, i_limit)
              for j in range(j_limit)))
          for i_step, j_step, i_min, i_limit, j_limit
          in [( 0, 1, 0, 20, 17),  # horizontal
              ( 1, 0, 0, 17, 20),  # vertical
              ( 1, 1, 0, 17, 17),  # leading diagonal
              (-1, 1, 3, 20, 17)]) # trailing diagonal

If you want to avoid writing out the multiplication, then the best approach would be to define a helper function, for example using functools.reduce and operator.mul:

from functools import reduce
from operator import mul

def product(iterable):
    """Return the product of the numbers in the iterable."""
    return reduce(mul, iterable, 1)

and then:

print(max(max(product(grid[i+k*i_step][j+k*j_step] for k in range(4))
              for i in range(i_min, i_limit)
              for j in range(j_limit)))
          for i_step, j_step, i_min, i_limit, j_limit
          in [( 0, 1, 0, 20, 17),  # horizontal
              ( 1, 0, 0, 17, 20),  # vertical
              ( 1, 1, 0, 17, 17),  # leading diagonal
              (-1, 1, 3, 20, 17)]) # trailing diagonal


my constructor is doing work. Is this a code smell in this example? I have created a queue in javascript. This queue also has blocking take(). As you all probably know this is not really true, because javascript does not have threads. It just wait until element has been added to queue. You can view to source-code at https://gist.github.com/2467432. I try to write testable code. As you can see from my code below my constructor does real work?

var EventEmitter = require('events').EventEmitter,
    Queue = module.exports = function Queue() {
    /*  Constructor is doing work..??
    */
    var _emitter    = this._emitter     = new EventEmitter(),
        _awaiting   = this._awaiting    = [],
        _queue      = this._queue       = [];

    _emitter.on('offer', function (data) {
        _awaiting.shift()(data);
    });

};

My question is if this a code smell? I think this class(object) is a leaf of application graph(value-object), so maybe it is not such a big deal. This way I think I can keep my API clean(easy to use). I think my class is still easy to test as you can hopefully see from my test class.

  Value-objects, these tend to
  have lots of getters / setters and are very easy to construct are
  never mocked, and probably don't need an interface. (Example:
  LinkedList, Map, User, EmailAddress, Email, CreditCard, etc...). (2)
  Service-objects which do the interesting work, their constructors ask
  for lots of other objects for colaboration, are good candidates for
  mocking, tend to have an interface and tend to have multiple
  implementations (Example: MailServer, CreditCardProcessor,
  UserAthenticator, AddressValidator).

If you think it is a code smell I would like to know how you would fix this(but still keep the tests pass  without any modification if possible?)

Personally the only issue I see with the code in terms of your constructor doing real work or testability is the instantiation of an EventEmitter object. Doing this in the constructor prevents you from mocking out or injecting a different class/instance for testing. You could use Dependency Injection as a simply way to resolve this:

var EventEmitter = require('events').EventEmitter,
    Queue = module.exports = function Queue( emitter ) {
    /*  Constructor is doing work..??
    */
    var _emitter    = this._emitter     = emitter,
        _awaiting   = this._awaiting    = [],
        _queue      = this._queue       = [];

    _emitter.on('offer', function (data) {
        _awaiting.shift()(data);
    });

};


LeetCode: Symmetric Tree C# https://leetcode.com/explore/interview/card/top-interview-questions-easy/94/trees/627

  Given a binary tree, check whether it is a mirror of itself (ie,
  symmetric around its center).

  For example, this binary tree [1,2,2,3,4,4,3] is symmetric:

    1
   / \
  2   2
 / \ / \
3  4 4  3

  But the following [1,2,2,null,3,null,3] is not:

   1
   / \
  2   2
   \   \
   3    3

  Note: Bonus points if you could solve it both recursively and
  iteratively.

using System.Collections.Generic;
using GraphsQuestions;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace TreeQuestions
{

/**
 * Definition for a binary tree node.
 * public class TreeNode {
 *     public int val;
 *     public TreeNode left;
 *     public TreeNode right;
 *     public TreeNode(int x) { val = x; }
 * }
 */

    /// <summary>
    /// https://leetcode.com/explore/interview/card/top-interview-questions-easy/94/trees/627
    /// </summary>
    [TestClass]
    public class SymmetricTreeTest
    {
        //     1
        //    / \
        //   2   2
        //  / \ / \
        // 3  4 4  3
        [TestMethod]
        public void ValidSymmetricTree()
        {
            TreeNode root = new TreeNode(1);
            root.left = new TreeNode(2);
            root.right = new TreeNode(2);
            root.left.left = new TreeNode(3);
            root.left.right = new TreeNode(4);
            root.right.left = new TreeNode(4);
            root.right.right = new TreeNode(3);
            Assert.IsTrue(SymmetricTree.IsSymmetric(root));
        }

        //    1
        //   / \
        //  2   2
        //   \   \
        //   3    3

        [TestMethod]
        public void NotValidSymmetricTree()
        {
            TreeNode root = new TreeNode(1);
            root.left = new TreeNode(2);
            root.right = new TreeNode(2);
            root.left.right = new TreeNode(3);
            root.right.right = new TreeNode(3);
            Assert.IsFalse(SymmetricTree.IsSymmetric(root));
            Assert.IsFalse(SymmetricTree.IsSymmetricIterative(root));
        }
    }

    public class SymmetricTree
    {
        public static bool IsSymmetric(TreeNode root)
        {
            if (root == null)
            {
                return true;
            }
            return Helper(root.left, root.right);
        }

        public static bool Helper(TreeNode t1, TreeNode t2)
        {

            if (t1 == null && t2 == null)
            {
                return true;
            }

            if (t1 == null || t2 == null)
            {
                return false;
            }

            if (t1.val != t2.val)
            {
                return false;
            }

            //make sure the MIRRORED SIDES are sent!
            return Helper(t1.left, t2.right) && Helper(t1.right, t2.left);
        }

        public static bool IsSymmetricIterative(TreeNode root)
        {
            if (root == null)
            {
                return true;
            }
            Queue<TreeNode> Q = new Queue<TreeNode>();
            Q.Enqueue(root);
            Q.Enqueue(root);
            while (Q.Count > 0)
            {
                TreeNode t1 = Q.Dequeue();
                TreeNode t2 = Q.Dequeue();
                if (t1 == null && t2 == null)
                {
                    continue;
                }
                if (t1 == null || t2 == null)
                {
                    return false;
                }
                if (t1.val != t2.val)
                {
                    return false;
                }
                Q.Enqueue(t1.left);
                Q.Enqueue(t2.right);
                Q.Enqueue(t1.right);
                Q.Enqueue(t2.left);
            }
            return true;
        }
    }
}

Please review for performance

Performance

The recursive function is as fast as I can think of.
The iterative function should enqueue root.left and root.right instead of root and root to gain a cycle (micro-optimisation).

Review

I find it weird that the null node is considered symmetric IsSymmetric(null). I would throw an error for invalid input.
Both algorithms are not able to deal with TreeNode instances that have a cycle (stack overflow exception vs infinite loop respectively). You will lose some performance if you guard against cycles.
Helper is a public method. There is no reason for this. Make it private or local inside the public method, and perhaps rename it to IsSymmetric, which will not be in conflict with the public method because of different signature.
Parameter names t1 and t2 are best avoided. Use full and more clear names like node1 and node2.
Queue<TreeNode> Q = new Queue<TreeNode>(); should be written more concisely and clearly as var queue = new Queue<TreeNode>();.


Find the greatest number in an array The program reads a .txt file that contains lines of numbers. The first one has one number N and the second one has \$N\$ numbers as the first line says.

The \$N\$ numbers is \$1 \le N \le 1.000.000\$.

The program read the \$N\$ numbers in an array and performs the following procedure:

  Read the number one by one backwards in the array and check to find the max value. Every time a new max value is found the int total increases by one. In the end we write the total value in an output file.

Is my code well written?

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

/*---------------------*/
/* defined constants   */
/* for restriction     */
/*---------------------*/
#define MIN 1
#define MAX 1000000

/*---------------------*/
/* function prototypes */
/*---------------------*/
int main(void);
FILE *read_input(char *filename_r);
int count_children(FILE *input);
int pass_heights(FILE *input, int *children,const unsigned int size);
int chck_tall(const int *children,const unsigned int size);
void write_output(const unsigned short total,char *filename_w);

/*----------------------------------*/
/* start of program - Main Function */
/*----------------------------------*/
int main(void) {

    char *filename_r = "xxx.in";
    char *filename_w = "xxx.out";

    FILE *input = read_input(filename_r);

    unsigned int size = count_children(input);

    int *children = malloc(size * sizeof(unsigned short));
    if (children==NULL)
    exit(1); //General application error

    pass_heights(input, children, size);

    fclose(input);

    unsigned short total = chck_tall(children, size);

    free(children);

    write_output(total,filename_w);

    return 0;
}

FILE *read_input(char *filename_r) {

    FILE *input = fopen(filename_r, "r");

    if(input == NULL)
    exit(66); //EXIT_NOINPUT 'cannot open input'

    return input;
}

int count_children(FILE *input) {

    unsigned int count = 0;
    fscanf(input, "%d",&count);

    if(count > MAX || count < MIN)
    exit(1); //General application error

    return count;
}

int pass_heights(FILE *input, int *children,const unsigned int size) {

    for(register int i = 0; i < size; i++)
    fscanf(input, "%d",&children[i]);

    return *children;
}

int chck_tall(const int *children,const unsigned int size) {

    unsigned short total = 0;
    unsigned short tmp_max = 0;
    for(register int i = size - 1; i >= 0; i--)
    {
        if(children[i] > tmp_max) {
            tmp_max = children[i];
            total++;
        }
    }
    return total;
}

void write_output(const unsigned short total,char *filename_w) {

    FILE *output = fopen(filename_w, "w");

    if(output == NULL)
    exit(74); //EXIT_IOERR 'input/output error'

    fprintf(output, "%d\n", total);
    fclose(output);
}

Constant string literals should be declared as such (const):

const char *filename_r = "xxx.in";
const char *filename_w = "xxx.out";

If you don't do that then you have a char * pointing to a string literal which the compiler will probably put in a read only data segment. Should you ever try to modify it (via the non-const char pointer) you'll run into undefined behaviour (most likely it will crash your program).
Your helper methods should in general not terminate the program (you call exit() in quite a few places). Their return value should indicate that they could not perform the operation they were supposed to leaving it to the caller (in your case main) to take appropriate action (like terminating early).
You should alway places braces around blocks, even one-liners. Things like this:

if(count > MAX || count < MIN)
exit(1); //General application error

are really confusing as it reads like exit() is executed unconditionally and if you ever add another statement to it then you'll run into trouble. Braces make it crystal clear what's intended:

if (count > MAX || count < MIN)
{
    exit(1); //General application error
}

This happens in quite a few places. Readability beats brevity anytime.
You use unsigned short for the count which means it will be unsigned 16bit on most platforms. So if the input sequence is descending like 100000, 99999, 99998, ..., 1 with more than 65535 elements then your count variable will overflow.
Don't leave vowels out of names for functions or variables: chck_tall vs check_tall. If you leave them out and they do not shorten the name sufficiently then why did you leave them out. And if they shorten the name sufficiently then it's likely it will become too garbled to read properly. So in either case leaving them out is not a wise choice.
Your array holds int yet when you check for the max in chck_tall your temporary variables are short which can lead to truncation. Your compiler should have warned you about it. Do not ignore compiler warnings. In fact it is not uncommon in production code to run with an option which treats warnings as errors (most compilers do offer such an option).
Not sure why you think you need to make the loop variable register:

for (register int i = size - 1; i >= 0; i--)

register is just a hint to the compiler which is free to ignore it anyway. Typically you should only use it in specific circumstances when you know if and why it is required. Otherwise it's just adding noise.


Number-Guessing Game in Rust I don't yet see a code review for a number-guessing game in Rust. Since I am just learning the language, and since the number-guessing game is part of the official Rust tutorial, I thought it would be beneficial both for me and for this site to open a code review request.

I've started with the official tutorial and then built upon it using the basic language constructs (functions, conditionals, loops) to add some error checking and break common logic into reusable functions.

My background is mostly in C and Java development. This is the final example I've come up with after spending about 5 hours reading the book. What I'm interested to learn is whether I'm using the language's unique features in the expected ways and if I'm re-inventing the wheel in any parts.

extern crate rand;

use std::io;
use std::cmp::Ordering;
use rand::Rng;

fn read_i32() -> i32 {
    loop {
        let mut input = String::new();
        io::stdin().read_line(&mut input)
            .expect("Failed to read line.");
        let input: i32 = match input.trim().parse() {
            Ok(n) => n,
            Err(_) => {
                println!("Please input a number.");
                continue;
            },
        };
        return input;
    }
}

fn read_guess(min: i32, max: i32) -> i32 {
    let mut guess: i32;
    loop {
        guess = read_i32();
        if guess < min {
            println!("Please input a number that is at least {}.", min);
        } else if guess > max {
            println!("Please input a number that is no larger than {}.", max);
        } else {
            break;
        }
    }
    guess
}

fn main() {
    println!("Guess the number!");
    let (min, max) = (1, 10);
    let secret_number = rand::thread_rng().gen_range(min, max+1);
    loop {
        println!("Please input your guess, between {} and {}.", min, max);
        let guess = read_guess(min, max);
        match guess.cmp(&secret_number) {
            Ordering::Less    => println!("Too small!"),
            Ordering::Greater => println!("Too big!"),
            Ordering::Equal   => {
                println!("You win!");
                break;
            },
        };
    }
}

As you probably expect, there's not much that's crazy about your code. It's pretty straight-forward. There's just a little bit that belies your C and Java background. ^_^

There's no need for a mutable guess in read_guess.
There's no need for an input variable at all. This also removes the need for an explicit type as it can be inferred from the return type.
For a very performance oriented approach, move the string allocation outside of the loop. In the off chance that someone keeps entering invalid numbers, there's no reason to allocate a new string each time.
Don't end the expect string with punctuation as a colon is automatically added.
Add spaces around binary operators
Generally, I'd indicate that min and max are CONSTANTs, but doing so may be overkill here, as it would be verbose (const MIN: i32 = 1).

extern crate rand;

use std::io;
use std::io::prelude::*;
use std::cmp::Ordering;
use rand::Rng;

fn read_i32() -> i32 {
    let mut input = String::new();

    loop {
        input.clear();
        io::stdin().read_line(&mut input).expect("Failed to read line");
        match input.trim().parse() {
            Ok(n) => return n,
            Err(_) => println!("Please input a number."),
        };
    }
}

fn read_guess(min: i32, max: i32) -> i32 {
    loop {
        let guess = read_i32();
        if guess < min {
            println!("Please input a number that is at least {}.", min);
        } else if guess > max {
            println!("Please input a number that is no larger than {}.", max);
        } else {
            return guess;
        }
    }
}

fn main() {
    println!("Guess the number!");
    let (min, max) = (1, 10);
    let secret_number = rand::thread_rng().gen_range(min, max + 1);
    loop {
        println!("Please input your guess, between {} and {}.", min, max);
        let guess = read_guess(min, max);
        match guess.cmp(&secret_number) {
            Ordering::Less    => println!("Too small!"),
            Ordering::Greater => println!("Too big!"),
            Ordering::Equal   => {
                println!("You win!");
                break;
            },
        };
    }
}

For fun, here's an alternate read_i32 that didn't end up looking much better:

fn read_i32() -> i32 {
    let stdin = io::stdin();
    for line in stdin.lock().lines() {
        let line = line.expect("Failed to read line");
        match line.trim().parse() {
            Ok(n) => return n,
            Err(_) => println!("Please input a number."),
        }
    }
    unreachable!("Ran out of standard input");
}

  Since everything is immutable unless specified otherwise, what is the purpose of a constant?

A constant can exist outside of a function, while let statements must occur inside of one. Additionally, a const is likely to be inlined at compile time, while a static can have its address taken. More info is in the book.

  Can you briefly explain when and why I should use the unreachable macro?

You should use unreachable when the code is unreachable, but the compiler cannot statically verify that. In my example, the compiler cannot know that standard input will provide data forever or return an Err which will cause a panic.

Actually, thinking about it further, your original code will go into an infinite loop when /dev/null is connected to standard in: cargo run </dev/null. The version I have will panic and print a marginally helpful error message. Maybe my version is useful after all...


Kadane maximum subarray Find the maximum sum of subarray with Kadane's algorithm

//test case
int maxKanade, startKanade, endKanade;
maxKanade = Kadane(new List<int>() { 2, -4, 6, -3, 9 }, out startKanade, out endKanade);
Debug.WriteLine("{0} {1} {2}", maxKanade, startKanade, endKanade);
maxKanade = Kadane(new List<int>() { 3, 4, 6, -3, -9 }, out startKanade, out endKanade);
Debug.WriteLine("{0} {1} {2}", maxKanade, startKanade, endKanade);
//test case end

public static int Kadane(List<int> input, out int start, out int end)
{
    //from wiki https://en.wikipedia.org/wiki/Maximum_subarray_problem
    //def max_subarray(A):
    //max_ending_here = max_so_far = 0
    //for x in A:
    //    max_ending_here = max(0, max_ending_here + x)
    //    max_so_far = max(max_so_far, max_ending_here)
    //return max_so_far
    start = 0;
    int s = 0;
    end = 0;
    int count = 0;
    int max_ending_here = 0, max_so_far = 0;
    foreach(int i in input)
    {
        if (max_ending_here + i > 0)
        {
            max_ending_here += i;
        }
        else
        {
            // if go negative basically take a fresh start
            max_ending_here = 0;
            s = count + 1;
        }
        if (max_so_far < max_ending_here)
        {
            max_so_far = max_ending_here;
            end = count;
            start = s;
        }
        count++;
    }
    return max_so_far;
}

Testing
When @200_success asked if you had tested the code you seemed puzzled and pointed out that you had included two tests.  Unfortunately, writing things out to the screen and sight checking the results would not be considered good unit testing and for a reviewer they are not very useful as all we can tell is that some output was displayed. There is nothing to indicate what the expected results were or if they were achieved.

Also, two unit tests which pass is a limited subset to say that it is working.

What happens with an empty input?
What happens if they are all negative?
What happens if we have two sub-arrays which both have the same maximum value?

to a degree this is a gap in the requirements, the original Kadane only cares about the max value not the start and end.

Should we return start = 0 and end = 0 if there are no elements in the input or if all are negative. The result (0) is correct but it seems to indicate an array of length starting at 0, ending at 0.

Bottom line: Testing a function like this is a great place to use an automated unit testing framework. It may seem like doubling the code to be written but by the time one has run the code, read and checked the answers and repeated umpteen times for each tweak to the code to deal with the next thing we hadn't thought of, it does save time and gives a nice warm fuzzy feeling that we can see that it works and if any changes break it.

API
We do not use the List functionality anywhere in the function but insist that the user passes in a List.  I would push for changing the interface to IEnumerable<int>.


Cleaner or better readability in URL composition I have a URL that is hardcoded via a config which I extract into a config:

$url = 'http://www.example.com/api/v1/all/limit/{limit}/offset/{offset}'
// this is stored as a string inside a config file

then $limit = 10; $offset = 0;

I currently use the following preg_replace to replace the given URL and transform into an effective URL:

$url = preg_replace('/\{([A-Z, a-z]+)\}/e', "$$1", $url);

The effective string is now:

  http://www.example.com/api/v1/all/limit/10/offset/0

It works, however, I had to dig deep into figuring out the pattern and $$1 part so there goes readability.

My questions:

Is there anything else out there that someone can think of that can be as good as the above code?
I could use a str_replace with 2 arrays and do a replacement of the variables - it might be cleaner but it maybe slower.  Would anyone advise replacing that theory instead of this?  If so, why?
The number of variables could grow - currently doing this it will capture all $variable there is within that scope - so any suggestion would need to keep in mind that limit/offset will not be the only limited variables that will be incoming:

$url = 'http://www.example.com/api/v1/all/id/{id}/limit/{limit}/offset/{offset}'

$url = 'http://www.example.com/api/v1/all/relatedId/{relatedId}/limit/{limit}/offset/{offset}'

You could simplify to something such as:

$url = 'http://www.example.com/api/v1/all/limit/%d/offset/%d';

$newURL = sprintf($url, $limit, $offset);

We use sprintf() for this.

Update

  e (PREG_REPLACE_EVAL)

  Warning This feature has been DEPRECATED as of PHP 5.5.0. Relying on
  this feature is highly discouraged.

Docs

Now I really suggest you find an alternative.

According to your update, you say the number of variables might grow. Could you explain (example code?) this more? If it "grows", then the issue may not be this part of the function, it may be the surrounding code that needs fixing.