Counting letters program with ASCII-art graph I am writing a program in C running on UNIX which counts the number of each letters in a input text file. For a file like this:

The cat sat on the green mat

The output would be like this:

   The letter ’a’ occurs 3 times.
   The letter ’c’ occurs 1 times.
   The letter ’e’ occurs 4 times.
   The letter ’g’ occurs 1 times.
   The letter ’h’ occurs 2 times.
   The letter ’m’ occurs 1 times.
   The letter ’n’ occurs 2 times.
   The letter ’o’ occurs 1 times.
   The letter ’r’ occurs 1 times.
   The letter ’s’ occurs 1 times.
   The letter ’t’ occurs 5 times.

  5                    *
  4     *              *
  4     *              *
  3 *   *              *
  3 *   *              *
  2 *   *  *     *     *
  2 *   *  *     *     *
  1 * * * **    ***  ***
  1 * * * **    ***  ***
  0 **************************
  0 **************************
... abcdefghijklmnopqrstuvwxyz

Where the graph represents the amount of times a letter appears. (If it is more than 10, i simply put a '+' after the 10th row). The code I've currently written to achieve this is as follows: (Haven't found a good way to test for capital letters as well as lowercase yet).

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

void drawGraph(int letters[26], char alpha[26]);
void printLetters(int letters[26], char alpha[26]);
void getLetters(FILE *fp, int letters[26], char alpha[26]);

int main(int argc, char *argv[]) {
  FILE *fp;
  int letters[26] = { 0 };
  char alpha[26] = { 'a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z' };
  int indexedAlpha[256] = { 0 };
  int j = 1;

  for (i = 97; i <= 127; i++)
    {
      indexedAlpha[i] = j;
      j++;
    }

  //open file
  if ((fp = fopen(argv[1], "r")) == NULL)
    {
      perror("Cannot open file");
      exit(EXIT_FAILURE);
    }

  getLetters(fp, letters, alpha);
  printLetters(letters, alpha);
  printf("\n");
  drawGraph(letters, alpha);
  printf("\n");

  return EXIT_SUCCESS;
}

void getLetters(FILE *fp, int letters[26], char alpha[26]) {
  int c;
  for (int i = 0; (c = fgetc(fp)) != EOF; i++)
    {
      c = fgetc(fp);
      if ( isalpha(c) )
    {
      for ( int j = 0; j < 26; j++ ) //find which letter it is
        {
          if( c == alpha[j] )
        {
          letters[j]++;
          break;
        }
        }
    }
    }
}

void printLetters(int letters[26], char alpha[26]) {
  for( int i = 0; i < 26; i++ )
    {
      if(letters[i] != 0){
    printf("The letter '%c' occurs %d times.\n", alpha[i], letters[i]);
      }
    }
}

void drawGraph(int letters[26], char alpha[26]) {
  int x = 11;
  int y;
  while(x >= 0)
    {
      y = 0;
      while (y < 2)
    {
      if (x == 10)
        {
          printf(" %d ", x);
        }
      else if (x == 11)
        {
          printf("    ");
        }
      else
        {
          printf("  %d ", x);
        }

      for( int i = 0; i < 26; i++ )
        {
          if(letters[i] > 10)
        {
          printf("+");
          letters[i] = 10;
          y++; // Break out of while loop
        }
          else if(letters[i] == x)
        {
          printf("*");
        }
          else
        {
          printf(" ");
        }
          if (letters[i] == x && y == 1)
        {
          letters[i] = letters[i] - 1;
        }
        }
      printf("\n");
      y++;
    }
      x--;
    }
  printf("... ");

  for( int i = 0; i < 26; i++ )
    {
      printf("%c", alpha[i]);
    }
}

What I'm looking for is advice and tips on notation, efficiency(In amount of code written and memory usage) and any other good tips/Best practices or better methods i could use to complete this task.

Thanks!

Let just say when I first read the question I was very impressed by the graph.
Well done.

Small mistake in reading the file:

char c;
for (int i = 0; !feof(fp); i++)
{
    c = fgetc(fp);

This is wrong in all languages. The eof is never set until you read past the eof. The last successful read reads up-to (but not past) the eof. So even if there are no more characters in the file the eof flag is not set (until you try and read the character after eof).

As a result you are suffering a one off error. The loop is executed one time to many. The value of 'c' on the last iteration is EOF truncated to fit into a char.

So the standard patter is to read from the file and see if it works. If it works then you enter the loop:

int c;
for (int i = 0; (c = fgetc(fp)) != EOF; i++)
{

Note we need to change the c from a char into an int to make sure that EOF is not truncated out of the value.

Not an error: But you are implementing an O(1) operation is O(n).

  for ( int j = 0; j < 26; j++ ) //find which letter it is
    {
      if( c == alpha[j] )
    {
      letters[j]++;
      break;
    }

With a little though you can invert the array. So you use the letter to look up its ID.

char alpha[256] =
{  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,   //  0->15 ignore
   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,   // 16->31 ignore
   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,   // 32->47 ignore
   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,   // 48->63 ignore
   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,   // 64->79 ignore
   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,   // 80->95 ignore
   0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,   // 96->111 a -
   16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 0, 0, 0, 0, 0,   // 112->127 -z
   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,   // 128->143 ignore

   .. etc

   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};  // 240->255 ignore

   // Now the lookup becomes:
   if (alpha[c] != 0)
   {
       letters[alpha[c] - 1]++;
   }

But do you really care if you count all the characters. I would not (unless there are some serious space constraints). You can just count all the characters. When you print them out you just print the ones you want.

   int letters[256];

   ....
   letters[c]++;   // or maybe letters[tolower(c)]++;
   .....

   // Now we just need to de reference the count of the letters we are interested in.
   for( int i = 0; i < 26; i++ )
   {
       int count = letters[alpha[i]];
       if(count != 0){
           printf("The letter '%c' occurs %d times.\n", alpha[i], count);
       }
   }

Some small tidy up (you seem to have embeded tabs that mess up spacing on this site).


Reddit style comment chains in React This isn't meant to be a fully-featured anything, isn't meant to involve any sort of api interactions. It's just meant to be a proof-of-concept of how I could implement nested reddit-style comments.

Every comment is either a top level comment or a reply to another comment. Each level of nesting is indented and is color coded. The color-coding wraps back in on itself (eg if there are 5 defined files, then the 6th layer of nesting should have the same color code as the 1st layer of nesting).

import * as React from 'react';

export class Test extends React.Component {
    constructor(props) {
        super(props);
        this.state = {
            comments : [
                {
                    'user': 'user1',
                    'text': 'text1',
                    comments: [{
                        'user': 'user3',
                        'text': 'responding to text1',
                        comments: [
                            {
                                'user': 'user1',
                                'text': 'responding to user3'
                            }
                        ]
                    }]
                },
                {
                    'user': 'user2',
                    'text': 'text2'
                }
            ]
        }
    }
    render() {
        return

            {
                this.state.comments.map(comment => (
                    <Comment comment={comment}/>
                ))
            }

;
    }
}

class Comment extends React.Component {
    renderSidebar(level) {
        const colors = ['green','orange','red'];
        const index = level % colors.length;
        return {'border-left':'solid 3px ' + colors[index]};
    }

    render() {
        const comment = this.props.comment;
        const subComments = comment.comments || [];
        const level = this.props.level || 0;
        return (

{comment.user}

{comment.text}

                    {subComments.map(comment => (
                        <Comment comment={comment} level={level + 1}/>
                        ))}

        )
    }
}

jsfiddle here: https://jsfiddle.net/z318wsq6/

Test class

In the scope of code you provided, the div is useless and may be omitted:

render() {
  return this.state.comments.map((comment, index) => (
    <Comment key={index} comment={comment}/>
  ))
}

However, as React warns you in the console, don't forget to pass a key prop to your array items (added in the snippet above).

Comment class

The method name renderSidebar suggest to me that it will return something that React can render, but it returns an object of CSS styles instead. For that reason, I'd rename it to getSidebarStyle.
Since the method renderSidebar doesn't depend on anything except its parameters and doesn't alter any instance variables (it's a pure function), you can move it outside the class.
The array of colors has nothing to do with the logic of selecting a color depending on the intendation level. Depending on your coding style preferences, you could move it to outside the method and capitalize it.
When rendering the subcomments, set a key prop on your array items as above.
JSX is camel-cased and that's why React warns you in the console to change your CSS property names (e.g. border-left to borderLeft), also allowing you to omit the quotes.

Generally, your code is in good shape & all of the comments above are more or less only improvements. To help you move faster, I suggest always checking the console for helpful warnings and using tools such as Prettier and a linter for your JS dialect to help you while programming.


Game combo press model (substring search) I'm trying to make a model for checking combo presses in a game. Given a list of Button (enum) presses, it returns the list of possible combos completed. I tried using a suffix tree, but I'm not sure if this is the best solution. It's also my first time writing a suffix tree, and I don't know if this is the correct usage.

Also, could someone help me analyze the runtime of registering combos and searching combos? I think registering combos is O(n), but what about searching for combos?

Button enum:

public enum Button {
        Left, Right, Down, Up, A, B, X, Y;
}

Registering combos and for searching for the completed combos:

public class SuffixTree {
    private class SuffixTreeNode {
        Map<Button, SuffixTreeNode> children = new HashMap<Button, SuffixTreeNode>();
        List<Button> indices = new ArrayList<Button>();

        public void insert(List<Button> lob) {
            if (lob != null && lob.size() > 0) {
                indices.add(lob.get(0));
                Button value = lob.get(0);
                SuffixTreeNode child = null;
                if (children.containsKey(value)) {
                    child = children.get(value);
                } else {
                    child = new SuffixTreeNode();
                    children.put(value, child);
                }
                List<Button> remainder = lob.subList(1, lob.size());
                child.insert(remainder);
            }
        }

        public List<Button> search(List<Button> lob, List<Button> parents) {
            if (lob == null || lob.isEmpty()) {
                return parents;
            } else {
                Button first = lob.get(0);
                if (children.containsKey(first)) {
                    parents.add(first);
                    List<Button> remainder = lob.subList(1, lob.size());
                    return children.get(first).search(remainder, parents);
                }
            }
            return null;
        }
    }

    SuffixTreeNode root = new SuffixTreeNode();

    public void registerCombos(List<List<Button>> combos) {
        for (List<Button> combo : combos) {
            root.insert(combo);
        }
    }

    public List<List<Button>> search(List<Button> buttonPresses) {
        List<List<Button>> res = new ArrayList<List<Button>>();
        int len = buttonPresses.size();
        for (int i = 0; i < len; i++) {
            List<Button> partial = root.search(buttonPresses.subList(i, len), new ArrayList<Button>());
            if (partial != null && !partial.isEmpty()) {
                res.add(partial);
            }
        }
        return res;
    }
}

Bug

Since you never mark your suffix tree nodes as end nodes, your search will find combos that don't exist.  For example, if ABXY is a registered combo, and you search for AB, it will return both A and AB as combos.  You should have a boolean per node telling you whether the node is the end of a combo or not.

Efficiency

You appear to search for all sublists that end with the end of the original list.  For example, if the original search list were ABXY,you would search for these sublists: ABXY BXY XY Y.

Given this behavior, you could greatly improve your suffix tree by storing the combos in reverse order.  Then, when you want to search for a given list and all of its sublists, you search the suffix tree once in reverse order.  For example, given ABXY, you search the suffix tree for YXBA and return up to four combos that match (Y YX YXB YXBA).  This would reduce your search from \$O(n^2)\$ to \$O(n)\$.

How to mark end nodes

This is in response to the comment.  Given a tree with combos A B X Y ABXY, the tree would look like this:

                          Root
                      /  |   |  \
                     A*  B*  X*  Y*
                    B
                   X
                  Y*

In the tree above, the end nodes are marked with a *.  So if you searched for combo AB, it would go down the left branch from the root but it would not find a combo because the node B on the left branch does not have a *.

Why searching in reverse order is more efficient

Suppose you are given combos A B X Y ABXY.  If you reversed each combo and added to a tree, your tree would look like:

                          Root
                      /  |   |  \
                     A*  B*  X*  Y*
                                  X
                                   B
                                    A*

Now suppose you were given ABXY as your search list, meaning you actually want to search for the four sublists ABXY BXY XY Y.  You could do this in one search by searching for YXBA and returning a positive match every time you find an end node.  So in the tree above, you would follow the rightmost path four levels, passing by the lists Y YX YXB YXBA.  Since Y and A are marked as end nodes, you would return Y and YXBA as matches.  After reversing, these would correspond to Y and ABXY as matches.  Notice that you only have to traverse the tree once instead of 4 times.


JavaScript Iteration In my current project I have objects of the following structure:

CanvasBundle - Object

Some elements...
gridCanvas
axesCanvas
functions[] (contains unknown number of functionCanvases)
overlayCanvas

I'd like to write an iterator which would iterative over all the canvases (grid/axes/func/func1/func2/.../overlay).

I've currently implemented it like this:

function makeCanvasBundleIterator(canvasBundle) {
  var nextIndex = 0;
  var functionIndex = 0;

  return {
    next: function() {
      var value;

      switch (nextIndex) {
      case 0:
        value = canvasBundle.grid;
        break;
      case 1:
        value = canvasBundle.axes;
        break;
      case 2:
        value = canvasBundle.functions[functionIndex];
        break;
      case 3:
        value = canvasBundle.overlay;
        break;
      default:
        break;
      }

      if (nextIndex != 2 || functionIndex == canvasBundle.functions.length -1) {
        nextIndex++;
      } else {
        functionIndex++
      }

      return nextIndex <= 4 ?
        {value: value, done: false} :
        {done: true};
    }
  };
}

This allows me to do the following:

var it = makeCanvasBundleIterator(canvasBundle);

var next;
while (!(next = it.next()).done) {
  console.log(next.value);
}

Coming from C, this code does not look clean at all. Since I'm a real JS beginner, I'd like a review with suggestions/advice how I can clean it up. Maybe there even is a completely different/better approach.

The switch is awkward, and the nextIndex != 2 special case is cumbersome.  It would be a bit simpler if you regularized everything:

function makeCanvasBundleIterator(canvasBundle) {
  var i = 0, j = 0;
  var items = [
    [canvasBundle.grid],
    [canvasBundle.axes],
    canvasBundle.functions,
    [canvasBundle.overlay],
  };
  return {
    next: function() {
      if (i >= items.length) {
        return {done: true};
      }
      if (j >= items[i].length) {
        i++;
        j = 0;
      }
      return {done: false, value: items[i][j++]};
  };
}

But that could be simplified further by linearizing the list up front using Array.concat():

function makeCanvasBundleIterator(canvasBundle) {
  var i = 0;
  var items = [].concat(
    canvasBundle.grid,
    canvasBundle.axes,
    canvasBundle.functions,
    canvasBundle.overlay
  );
  return {
    next: function() {
      return (i >= items.length) ? {done: true}
                                 : {done: false, value: items[i++]};
  };
}

For that matter, why bother making an iterator at all?  Just return the linearized array.  The caller can iterate through it conventionally using a for loop or Array.forEach().

Note that the behaviour of these alternate solutions is different from the original if the canvasBundle is modified during iteration.  (You wouldn't want to do that, I hope?)


Server client for distributed system This is the code I am using for a distributed system. The code will be used to allow the server to send parts of a (really big) document which will be processed by the connected clients. Please tell me what things would make it more efficient and if there are any security issues that I can correct without addition of an auth library.

#include<stdio.h>
#include<string.h> //strlen
#include<stdlib.h> //strlen
#include<sys/socket.h>
#include<arpa/inet.h> //inet_addr
#include<unistd.h> //write
#include<pthread.h> //for threading , link with lpthread

void *connection_handler(void *);

int main(int argc , char *argv[]) {
    int socket_desc , client_sock , c;
    struct sockaddr_in server , client;
    //Create socket
    socket_desc = socket(AF_INET , SOCK_STREAM , 0);
    if (socket_desc == -1) {
        printf("Could not create socket");
    }
    puts("Socket created");
    //Prepare the sockaddr_in structure
    server.sin_family = AF_INET;
    server.sin_addr.s_addr = INADDR_ANY;
    server.sin_port = htons( 8888 );
    //Bind
    if( bind(socket_desc,(struct sockaddr *)&server , sizeof(server)) < 0) {
        //print the error message
        perror("bind failed. Error");
        return 1;
    }
    puts("bind done");
    //Listen
    listen(socket_desc , 3);
    //Accept and incoming connection
    puts("Waiting for incoming connections...");
    c = sizeof(struct sockaddr_in);

    pthread_t thread_id;

    while( (client_sock = accept(socket_desc, (struct sockaddr *)&client, (socklen_t*)&c)) ) {
        puts("Connection accepted");
        if (pthread_create( &thread_id , NULL , connection_handler , (void*) &client_sock) < 0) {
            perror("could not create thread");
            return 1;
        }
        //Now join the thread , so that we dont terminate before the thread
        //pthread_join( thread_id , NULL);
        puts("Handler assigned");
    }

    if (client_sock < 0) {
        perror("accept failed");
        return 1;
    }
    return 0;
}
/*
* This will handle connection for each client
* */
void *connection_handler(void *socket_desc) {
    //Get the socket descriptor
    int sock = *(int*)socket_desc;
    int read_size;
    char *message , client_message[2000];
    //Send some messages to the client
    message = "Greetings! I am your connection handler\n";
    write(sock , message , strlen(message));
    message = "Now type something and i shall repeat what you type \n";

    write(sock , message , strlen(message));
    //Receive a message from client
    while( (read_size = recv(sock , client_message , 2000 , 0)) > 0 ) {
        //end of string marker
        client_message[read_size] = '\0';
        puts (client_message);
        //Send the message back to client
        write(sock , client_message , strlen(client_message));
        //clear the message buffer
        memset(client_message, 0, 2000);
    }

    // system ("cd; chmod +x script.sh;./script");

    if(read_size == 0) {
        puts("Client disconnected");
        fflush(stdout);
    }
    else if(read_size == -1) {
        perror("recv failed");
    }
    return 0;
}

I'm not really into networking, but I can say a little about your coding style:

Missing return

if (socket_desc == -1) {
    printf("Could not create socket");
}
puts("Socket created");

If socket_desc is equal to -1. Could not create socket will be printed as well as Socket created, which is misleading. I think:

if (socket_desc == -1) {
    printf("Could not create socket");
    return -1;                           /* or some other error code */
}
puts("Socket created");

is appropriate and the code doesn't proceeds.

Outsource
I'm also not sure if it would be better to outsource this part (as void prepareServer(sockaddr_in *server):

server.sin_family = AF_INET;
server.sin_addr.s_addr = INADDR_ANY;
server.sin_port = htons( 8888 );

Unnecessary comments
I don't know who else will read this code but comments like //print the error message followed by this perror("bind failed. Error");

will just make the code more obscure, since the goal of the error message is pretty obvious.

Calling strlen for no reason
Also it's not really clear to me, why you need to call strlen(client_message) here

write(sock , client_message , strlen(client_message));

when you store assign read_size a few lines above in the while-condition. strlen will just search for a '\0', which you assign by yourself with read_size...

Besides these points I can't see any major mistakes. You should just think about your comments a little more (Do I really need this information? Is this self explaining? Is a single line comment enough here?).
As I already said: I can't say much about the networking part, maybe someone else will jump here.


Grow-only set (CRDT) I made up a quick and dirty implementation of the G-Set in C#.  I was hoping for some feedback on my approach.  I'm not totally happy with having the payload public but I can't see that I'm gaining much by making it private either.

public class GrowOnlySet<T>
{
    public HashSet<T> payload {get; private set;}

    public GrowOnlySet()
    {
        payload = new HashSet<T>();
    }

    public GrowOnlySet(HashSet<T> newstore)
    {
        payload = newstore;
    }

    public void Add(T element)
    {
        payload.Add(element);
    }

    public bool Lookup(T element)
    {
        return payload.Contains(element);
    }

    public bool Compare(GrowOnlySet<T> R)
    {
        return R.payload.IsSubsetOf(this.payload);
    }

    public void Merge(GrowOnlySet<T> R)
    {
        this.payload.UnionWith(R.payload);
    }
}

I can't see that I'm gaining much by making it private either.

The clients of the GrowOnlySet might use this object in a inappropriate way. For example they can remove an element.

So I can do this, the compiler lets me do it.

var gset = new GrowOnlySet<string>();
gset.payload.Remove("element");

Now there is no grow only set anymore. The encapsulation is important.

Also as a client I really don't need to know the internal representation, which is the way how it was chose to represent the grow only set.

And because the payload property is public now the GrowOnlySet interface has a lot of other methods that I can use (via .payload... ), thus there is no abstraction. But I needed an interface made of only Lookup, Compare, Merge and Add methods.

There is not check for null newstore.

 public GrowOnlySet(HashSet<T> newstore)
    {
        payload = newstore;
    }

Thus the methods that are using the payload will throw a NullReferenceException which is not good. I prefer to throw a ArgumentNullException so I can create a consistent GrowOnlySet and to let the clients to be aware that they need to provide a new store.

 public bool Compare(GrowOnlySet<T> R)
    {
        return R.payload.IsSubsetOf(this.payload);
    }

Naming convention here. Suggested name is r. Or maybe find a better name like other (inspired from the HashSet itself)

this. is redundant, so it can be removed to make the code cleaner.

Compare and Merge accept null arguments but again this will throw NullReferenceException. Better is to inform the clients of this class that it really needs an object to be compared or to be merged with.

So the class is refactored to something like this

 public class GrowOnlySet<T>
    {
        private readonly HashSet<T> _payload;

        public GrowOnlySet()
        {
            _payload = new HashSet<T>();
        }

        public GrowOnlySet(HashSet<T> newstore)
        {
            _payload = newstore ?? throw new ArgumentNullException(nameof(newstore));
        }

        public void Add(T element)
        {
            _payload.Add(element);
        }

        public bool Lookup(T element)
        {
            return _payload.Contains(element);
        }

        public bool Compare(GrowOnlySet<T> other)
        {
            if (other == null) throw new ArgumentNullException(nameof(other));

            return other._payload.IsSubsetOf(_payload);
        }

        public void Merge(GrowOnlySet<T> other)
        {
            if (other == null) throw new ArgumentNullException(nameof(other));

            _payload.UnionWith(other._payload);
        }
    }


"Hell Difficulty" Haskell Fast & Hard I'm a complete neophyte to Haskell with only a smidge of FP experience – I did some Racket programming academically and have written some semi-FP JavaScript professionally, but now I'm trying to learn Haskell (and FP better). So I read Haskell Fast & Hard with no problems up until the last chapter.

Monads.

  Make a program that sums all of its arguments. Hint: use the function getArgs.

I eventually wrote a program that nominally achieves this requirement, but it felt very hacky. For reference, here is a "correct" program by the author that I think I was supposed to model after. It sums the numbers in a comma-delimited string.

import Data.Maybe

maybeRead :: Read a => String -> Maybe a
maybeRead s = case reads s of
                  [(x,"")]    -> Just x
                  _           -> Nothing
getListFromString :: String -> Maybe [Integer]
getListFromString str = maybeRead $ "[" ++ str ++ "]"
askUser :: IO [Integer]
askUser = do
  putStrLn "Enter a list of numbers (separated by comma):"
  input <- getLine
  let maybeList = getListFromString input in
      case maybeList of
          Just l  -> return l
          Nothing -> askUser
-- show
main :: IO ()
main = do
  list <- askUser
  print $ sum list

Based on the above approach, I wrote a function to convert the list of argument-strings back into a comma-delimited string so that most of the other functions could work unchanged.

import Data.Maybe
import System.Environment (getArgs)

maybeRead :: Read a => String -> Maybe a
maybeRead s = case reads s of
                  [(x,"")]    -> Just x
                  _           -> Nothing
getListFromString :: String -> Maybe [Integer]
getListFromString str = maybeRead $ "[" ++ str ++ "]"
getArgsAsString :: [String] -> String
getArgsAsString [] = "0"
getArgsAsString (x:xs) = x ++ "," ++ getArgsAsString xs
askUser :: [String] -> [Integer]
askUser input = case (getListFromString (getArgsAsString input)) of
            Just l  -> l
            Nothing -> error "failsauce"
main :: IO ()
main = do
  input <- getArgs
  print $ sum (askUser input)

I am trying to stick to the spirit of the exercise and not go too far into library functions that solve the problem for me without teaching me anything (in particular, the author says not to try too hard to understand the syntax of maybeRead), but it feels very wrong to convert a list of strings to a comma-separated string only to split it out again into a maybe-list of integers.

All advice is appreciated! (But educational guidance is appreciated more than "just use xyz lib function".)

There's very little I would salvage from the model code.

Converting the arguments list into comma-separated string is a little hackish and not very efficient (nevertheless it works, and I'd be totally OK with it for a quick throw away script). Therefore I would not use getListFromString.
askUser is not needed since we're not interacting with the user.
There's already an alternative to maybeRead, in Text.Read called readMaybe.

This is how I would implement it:

import System.Environment (getArgs)
import Text.Read (readMaybe)

main :: IO ()
main = do
  values <- fmap (map readMaybe) getArgs
  putStrLn $ maybe "Arguments invalid" (show . sum) $ sequence values


My Stack implementation in Java I have started learning data structures and have implemented my own stack implementation:

package com.algo.stack;

import java.io.Serializable;
import java.lang.reflect.Array;

public class MyStack<T extends Serializable> {

    private T[] myArray = null; // the array is the stack content
    private int arraySize = 0; // holds the size counter

    /*
     * Initialize stack for its size and generic type
     */
    public MyStack(Class<T> c,int stackSize){

        @SuppressWarnings("unchecked")
        final T[] a = (T[]) Array.newInstance(c , stackSize);
        this.myArray = a;
    }

    /**
     * The push method, which adds
     * numbers in internal array
     * @param obj
     * @throws Exception
     */
    public void push(T obj) throws Exception{
        if(this.arraySize < this.myArray.length){
            this.myArray[this.arraySize++] = obj;
        }else{
            throw new Exception("MyStack is Full, need to pop");
        }

    }

    /**
     * pops or removes elements from
     * the internal array in LIFO fashion
     * @return
     * @throws Exception
     */
    public T pop() throws Exception{
        if(this.arraySize <= 0){
            throw new Exception("Nothing to pop, stack is empty");
        }
        return this.myArray[--this.arraySize];
    }

    /**
     * Returns the total size
     * @return
     */
    public int size(){
         return this.arraySize;
    }

    public static void main(String[] args){
        MyStack<Integer> stack1 = null;
        MyStack<String> stack2 = null;
        try{
            stack1 = new MyStack<Integer>(Integer.class, 3);
            stack1.push(12);
            stack1.push(24);
            stack1.push(36);

            while(stack1.size() > 0){
                System.out.println(stack1.pop());
            }

            System.out.println("\n");

            stack2 = new MyStack<String>(String.class, 4);

            stack2.push("Leo");
            stack2.push("Tom");
            stack2.push("Nancy");
            stack2.push("Julean");

            while(stack2.size() > 0){
                System.out.println(stack2.pop());
            }
        }catch(Exception e){
            e.printStackTrace();
        }
    }
}

It's working fine and the output is:

36
24
12

Julean
Nancy
Tom
Leo

Please review my rough implementation and let me know of any flaws and how I can improve.

T extends Serializable

Why? There doesn't seem to be a good reason to do this.

throw new Exception("MyStack is Full, need to pop");

Your professor probably told you to do it like this, but it'd at the very least be an interesting exercise to see if you can resize the array to hold more, rather than letting it be full. It's not terribly difficult, either.

@SuppressWarnings("unchecked")

These are almost always unnecessary; you theoretically should be able to rewrite your code such that they're unnecessary.

However, if I'm remembering my Java correctly, that's not possible in this case, so it's better to eliminate the warning, so that the "real" ones are more visible.

this.myArray[this.arraySize++]

This code, though its behavior is well-defined, is not what you should be writing. Split it into two lines. The two statements -- the set and the increment -- already connected by being very nearly the sole contents of a method. Putting the increment in the brackets just makes it more likely that the increment won't be seen. I, for one, didn't see it until I took a closer look, because it appeared to be incorrect.

You don't get an award for fewer lines. Every second you don't need to spend re-reading your code later is a second you can use to implement new, cool features, and every second people reading your code don't spend on unclear syntax is a second they can use to find real bugs.

It's unfortunate that you can't remove the Class<T> passed in to the constructor, but that's the way it is, so good job for minimizing its impact.

Space and indent things consistently. This:

new MyStack<Integer>(Integer.class, 3);

is not consistent with this:

Array.newInstance(c , stackSize);

Now, your response to any of these could be "meh, it's a project for school, so I don't really care about that", to which my response is this:

School is supposed to teach you how to code well, in part by drilling how to do that into your head through practice. If you don't practice writing code well, you'll find yourself stuck in habits of writing code that works, but nobody can read after three months. If you practice writing code well -- as in, readable, clean code, not necessarily bug-free -- starting as soon as possible, you'll save yourself a lot of headache debugging later.


Security of API Keygen for a cryptocurrency trading platform Here is an API key gen script for a cryptocurrency trading platform I am building.

First it checks to see if a key exists in the db for the user ID. If it does exist, it displays the key. If it doesn't, it creates one.

Next it checks to make sure the key is unique, by polling the db for identical keys. If one is found to be identical, the page is refreshed via meta refresh. If no key collision is found, it inserts the key in the database and refreshes the page via meta refresh. When the page is reloaded, there is now a key in the database for the user ID, and the key is displayed.

Any tips or feedback is greatly appreciated. I am a fairly inexperienced programmer and this is my first serious project.

require_once("models/config.php");
$id = $loggedInUser->user_id;
$api_select = mysql_query("SELECT * FROM userCake_Users WHERE `User_Id`='$id'");
while($row = mysql_fetch_assoc($api_select)) {
if($row["api_key"] !== null) {
    echo '
Your Api Key is:

';
    echo $row["api_key"];
}else{
    $user = $row["Username"];
    $pass = $row["Password"];
    $length = 128;
    $time = date("F j, Y, g:i a");
    $salt1 = $time . hash('sha512', (sha1 .$time));
    $salt2 = substr(md5(uniqid(rand(), true)), 0, 25);
    $salt3 = substr(md5(uniqid(rand(), true)), 0, 25);
    $salt4 = hash('sha256', (md5 .$time));
    $user_hash = hash('sha512', ($salt2 . $user . $salt1));
    $pass_hash = hash('sha512', ($salt1 . $pass . $salt2));
    $keyhash_a = hash('sha512', ($user_hash . $salt3));
    $keyhash_b = hash('sha512', ($pass_hash . $salt4));
    $hash_a = str_split($keyhash_a);
    $hash_b = str_split($keyhash_b);
    foreach($hash_a as $key => $value) {
        $hashed_a[] = $salt2 . hash('sha512', ($salt3 . $value)) . $salt1 . hash('sha256', ($salt4 . $key));
    }
    foreach($hash_a as $key => $value) {
        $hashed_b[] = $salt2 . hash('sha512', ($salt3 . $value)) . $salt1 . hash('sha256', ($salt4 . $key));
    }
    $hash_merge = array_merge($hashed_b, $hashed_a);
    $from_merge = implode($hash_merge);
    $exploded_2 = str_split($from_merge);
    $key_hash_last = implode($exploded_2);
    $key0 = str_shuffle($key_hash_last);
    $key1 = str_split($key0);
    $key2 = array_unique($key1);
    $key3 = implode($key2);
    $key4 = str_shuffle($key3);
    $key5 = str_shuffle($key4);
    $api_key0 = str_shuffle($key3.$key4.$key5.$key2);
    $api_key_prepped = mysql_real_escape_string($api_key0);
    $api_check_no_collision = mysql_query("SELECT * FROM userCake_Users WHERE `api_key` = '$api_key_prepped'"); //check to see if an identical key exists
    if(mysql_num_rows($api_check_no_collision) > 0) {
        echo '<meta http-equiv="refresh" content="0; URL=index.php?page=api">';//an identical key exists in the database, refresh the page and generate a new key.
    }else{
    $api_insert = mysql_query("UPDATE  `testing`.`userCake_Users` SET  `api_key` =  '$api_key_prepped' WHERE  `userCake_Users`.`User_ID` ='$id';"); //the key is unique, submit it to the database.
    echo '<meta http-equiv="refresh" content="0; URL=index.php?page=api">';
    }
}
}

It feels like you are combining lots of hashing and string transformation in hopes that complexity will make your scheme more secure.  That's not engineering or computer science, though — it's Cargo Cult Programming.

There are two ways that an API key scheme could work:

To generate the API key, concatenate the username with a salt and site-wide secret, then hash it.  When it is later presented, verify it by concatenating the username, salt, and secret, and see if the hashes match.

Advantages:

It may be possible to verify the API key without a database lookup.
You can immediately revoke all API keys ever issued by simply changing the site-wide secret.

Disadvantages:

It's tricky to revoke the API key for a particular user without a database lookup.

Generate any distinct, unguessable string.  Store it in the database, associated with the user.  When it is later presented, verify it by looking it up in the database.

Advantages:

It's easy to understand how it works.

Disadvantages:

Verification requires a database lookup.

What you have done is a combination of the two strategies.  You store the generated API key in the database, associated with the user.  However, while you could have used just any random string, you used a convoluted process involving lots of randomizing, hashing, concatenating, and shuffling, all without much justification, and in a way that offers no additional security benefits.

Consider, for example:

$hash_merge = array_merge($hashed_b, $hashed_a);
$from_merge = implode($hash_merge);
$exploded_2 = str_split($from_merge);
$key_hash_last = implode($exploded_2);

That just converts an array to a string, back into an array, and reconstitutes the same string again.

If you consider the properties of cryptographic hash functions, you will notice that much of the manipulations are unnecessary.  In particular, cryptographic hash functions are designed such that if inputs a and b differ by as little as one bit, hash(a) and hash(b) will not resemble each other at all.

Therefore, if you choose option (2), you can generate an API key securely using something as simple as

$api_key = hash('sha256', (time() . $id . $some_sitewide_secret . rand()));

The result will be secure because:

Cryptographic hashes are one-way (irreversible) functions, so knowing the hash reveals nothing about the input that was hashed. (Unguessable)
The input varies by time and includes randomness, so it is not repeatable. (Distinct)
The input includes the user ID, so no two users should be able to generate the same key. (Distinct)
The input includes some site-wide secret string (which you can set in your configuration file), so even if an attacker tried to generate a lot of keys using the same technique in an attempt to brute-force a match, knowing the approximate time at which the key was generated and even cracking the pseudorandom number generator would not help.  Pick a very long random string for the site-wide secret (a SHA-256 of any word processing document would do). (Unguessable)
The output has 256 bits of entropy, which is impossible to crack by exhaustive enumeration. (Unguessable)

Furthermore, the chance that the same API key would be generated twice is about 2-250.  It's more likely that your server would be struck by an asteroid tomorrow than that it would generate the same key twice using this technique, so don't worry about accidental collisions.


A class for dynamic inputs This class creates dynamic input labels.  You give it a pair of elements and it sets up the event listeners and CSS toggles to accomplish this.

Looking for feedback on code structure and correctness.

Toggle classes do what they say, they toggle one class on and another class off.

/***************************************************************************************************
**SDynInput - provides dynamic input for labels and borders
*/
    var SDynInput = $A.Class.create({
        Name: 'SDynInput',
        A: {

            // CSS classes used to toggle the label
            toggle_label_hide:       'toggle_label_hide',
            toggle_label_obscure:    'toggle_label_obscure',
            toggle_label_show:       'toggle_label_show',

            // CSS classes used to toggle the border
            toggle_border_hide:      'toggle_border_hide',
            toggle_border_obscure:   'toggle_border_obscure',
            toggle_border_show:      'toggle_border_show',

            // speed of label expanding
            speed:                   200
        },

        // elements are passed in as input/label pairs
        set: function (pair_array) {
            var index;
            for (index = 0; index < pair_array.length; index += 2) {
                this.applyEL(pair_array[index], pair_array[index + 1]);
            }
        },

        // apply event listeners to the input elements
        applyEL: function (input_element, label_element) {
            var self = this;
            input_element.addEventListener("blur", function () {
                if (input_element.value === '') {
                    $A.addClass(input_element, self.A.toggle_border_show);
                    $A.addClass(label_element, self.A.toggle_label_show);
                    $A.expandFont(label_element, 'up', self.A.speed);
                }
            }, false);
            input_element.addEventListener("focus", function () {
                if (input_element.value === '') {
                    $A.addClass(input_element, self.A.toggle_border_obscure);
                    $A.addClass(label_element, self.A.toggle_label_obscure);
                }
            }, false);
            input_element.addEventListener("paste", function () {
                $A.addClass(label_element, self.A.toggle_label_hide);
                $A.addClass(input_element, self.A.toggle_border_hide);
            }, false);
            input_element.addEventListener("keypress", function () {
                $A.addClass(label_element, self.A.toggle_label_hide);
                $A.addClass(input_element, self.A.toggle_border_hide);
            }, false);
            input_element.addEventListener("drop", function () {
                $A.setTimeout(function () {
                    $A.addClass(label_element, self.A.toggle_label_hide);
                    $A.addClass(input_element, self.A.toggle_border_hide);
                }, 0);
            }, false);
        }
    }, true);

The code is not bad;

Quite configurable
Helper functions

However,

There is quite a bit of copy paste code
Placing the style classes in self.A. makes for verbose code
The css classes themselves are lengthy as well
onBlur keeps making the font larger, but focus does not make the font smaller, which could cause comically large labels
applyEL -> applyEventListeners reads/parses better

This could be done instead:

/*****************************************************************************************
 **SDynInput - provides dynamic input for labels and borders
 */
var SDynInput = $A.Class.create({
    Name: 'SDynInput',
    // elements are passed in as input/label pairs
    set: function (inputLabelPairs) {
        var index;
        for (index = 0; index < inputLabelPairs.length; index += 2) {
            this.applyEventListeners(inputLabelPairs[index], inputLabelPairs[index + 1]);
        }
    },
    // apply event listeners to the input elements
    applyEventListeners: function (input, label) {
        var self = this,
            labelSpeed = 200,
            // CSS classes used to toggle the label
            hideLabel = 'toggle_label_hide',
            obscureLabel = 'toggle_label_obscure',
            showLabel = 'toggle_label_show',
            // CSS classes used to toggle the input border
            hideBorder = 'toggle_border_hide',
            obscureBorder = 'toggle_border_obscure',
            showBorder = 'toggle_border_show',
            applyClasses = function (inputClass, labelClass) {
                $A.addClass(input, inputClass);
                $A.addClass(label, labelClass);
            },
            applyHidingClasses = function () {
                applyClasses(hideBorder, hideLabel);
            };
        input.addEventListener("blur", function () {
            if (input.value === '') {
                applyClasses(borderShow, labelShow);
                $A.expandFont(label, 'up', labelSpeed);
            }
        }, false);
        input.addEventListener("focus", function () {
            if (input.value === '') {
                applyClasses(borderObscure, labelObscure);
            }
        }, false);
        input.addEventListener("paste", applyHidingClasses false);
        input.addEventListener("keypress", applyHidingClasses, false);
        input.addEventListener("drop", function () {
            $A.setTimeout(applyHidingClasses, 0);
        }, false);
    }
}, true);


Is this jQuery Plugin for TreeView correct? Looking for a review of my first published jQuery Plugin. It's for TreeViews, very basic example demo can be seen at: Demo

Everything works fine, just looking to see if I can get some feedback on things that I may be doing wrong best practices wise and/or things I could be doing better.

(function ( $ ) {

  var methods = {
    init : function(options) {

      // Default Settings
      var settings = $.extend({
          'expandIconClass' : 'closed',
          'contractIconClass' : 'open',
          'setFocus' : undefined,
          'classPrefix' : 'goodtree_',
        }, options);

      return this.each(function() {

        // Hide all of the children Elements
        $(this).find('ul').hide();

        // Add the plus minus buttons
        $(this).find('li').each(function() {
          if($(this).children('ul').length > 0)
            $(this).prepend($('<div />', {'class': settings.classPrefix + "toggle " + settings.expandIconClass}));
        });

        // Events
        $('.' + settings.classPrefix + 'toggle').click(function() {
          $(this).parent().children('ul').toggle();
          $(this).hasClass('open')
            ? $(this).removeClass(settings.contractIconClass).addClass(settings.expandIconClass)
            : $(this).removeClass(settings.expandIconClass).addClass(settings.contractIconClass);
        });

        if(undefined !== settings.setFocus)
        {
          $(this).goodtree('setFocus', settings.setFocus);
        }

      });
    },

    setFocus : function(obj) {
      return this.each(function() {
        var tree_parent = this;
        $(obj).parents('ul').each(function() {
          if($(this) === this)
            return;
          else
            $(this).show();
        });
      });
    }
  }

  $.fn.goodtree = function( method ) {
    if ( methods[method] ) {
        return methods[ method ].apply( this, Array.prototype.slice.call( arguments, 1 ));
      } else if ( typeof method === 'object' || ! method ) {
        return methods.init.apply( this, arguments );
      } else {
        $.error( 'Method ' +  method + ' does not exist on jQuery.goodtree' );
      }
  };
}) ( jQuery );

First off it'd be good to run the code through jslint or jshint. It identifies a few issues, such as an extra comma in the default options list; that'll break the code in older IE versions. Also, it flags some things like the block-less if, which - while allowed - it's best to avoid for the sake of consistency and maintainability.

As for the code itself, here are the things I noticed:

jQuery objects should be cached whenever possible
The code only handles unordered lists (ULs), not ordered lists (OL) although it might as well
The event handling is added document-wide every time the code is run. If you use the plugin twice in a document, the first tree gets double event handlers. That spells trouble.
The setFocus function might misbehave if the tree list is itself inside a list element, as it loops through all list-type parents up to the document root.
The classPrefix option is confusing: It's not used for the expand/contract classes, only for the toggle button's "main" class. But since that's the only one, why not just let the user define that as well? Basically, if a prefix is used only once, it's not necessary.
Comparing against undefined is a no-no. undefined is not a reserved word, and can (in some runtimes) be defined. In this case, a simple boolean works just as well.

Here's what I arrived at

(function ($) {
  var methods = {
    init: function(options) {
      // Default Settings
      var settings = $.extend({
        expandIconClass:   'closed',
        contractIconClass: 'open',
        toggleButtonClass: 'toggle',
        setFocus:          false      // just use a boolean here
      }, options);

      return this.each(function() {
        var target = $(this);

        // walk the tree
        target.find('li').each(function() {
          var node = $(this),
              branches = node.children('ul, ol'),
              button;

          if(branches.length > 0) {
            branches.hide();
            button = $('<div />', {
              'class': settings.toggleButtonClass + " " + settings.expandIconClass,
              on: {
                click: function (event) {
                  // we already have the correct elements here
                  branches.toggle();
                  button.toggleClass(settings.expandIconClass + " " + settings.contractIconClass);
                }
              }
            });

            node.prepend(button);
          }
        });

        if(settings.setFocus === true) {
          target.goodtree('setFocus');
        }
      });
    },

    setFocus: function(element) {
      return this.each(function() {
        $(element).parents('ul, ol').each(function() {
          var ancestor = $(this);
          if( this.is(ancestor) ) { // better check
            return false; // stop the each-loop
          }
          ancestor.show();
        });
      });
    }
  };

  $.fn.goodtree = function(method) {
    if(typeof methods[method] === 'function') { // stronger conditional
      return methods[method].apply(this, Array.prototype.slice.call(arguments, 1));
    } else if(typeof method === 'object' || !method) {
      return methods.init.apply(this, arguments);
    } else {
      $.error('Method ' +  method + ' does not exist on jQuery.goodtree');
    }
  };
}(jQuery));

Here's a demo (I reused your demo markup and styling, minus the open/close icons)


Having two abstract (static inner) classes within the interface they implement Is it wrong? Should I have these 2 classes in separate files?

public interface InternetDependentFlow {
    void onInternetIsAvailable();
    void onInternetIsNotAvailable();

    public static abstract class InternetIsAvailable implements InternetDependentFlow {
        @Override
        public final void onInternetIsNotAvailable() {
            // do nothing
        }
    }

    public static abstract class InternetIsNotAvailable implements InternetDependentFlow {
        @Override
        public final void onInternetIsAvailable() {
            // do nothing
        }
    }
}

Where I use this interface and the classes:

public class InternetDependentFlowTask extends AsyncTask<InternetDependentFlow,Void,Boolean> {

    private final Context context;
    private InternetDependentFlow[] internetDependentFlows;

    public static void run(Context context, InternetDependentFlow... internetDependentFlows) {
        InternetDependentFlowTask internetDependentFlowTask = new InternetDependentFlowTask(context);
        internetDependentFlowTask.execute(internetDependentFlows);
    }

    public InternetDependentFlowTask(Context context) {
        this.context = context;
    }

    @Override
    protected Boolean doInBackground(InternetDependentFlow... internetDependentFlows) {
        this.internetDependentFlows = internetDependentFlows;
        return NetworkUtils.isInternetAvailable(context);
    }

    @Override
    protected void onPostExecute(Boolean internetIsAvailable) {
        super.onPostExecute(internetIsAvailable);
        InternetDependentFlowChooser flowChooser = (internetIsAvailable == Boolean.TRUE)
                ? INTERNET_IS_AVAILABLE_FLOW_CHOOSER : INTERNET_IS_NOT_AVAILABLE_FLOW_CHOOSER;
        for (InternetDependentFlow internetDependentFlow : internetDependentFlows) {
            if (internetDependentFlow != null) {
                flowChooser.choose(internetDependentFlow);
            }
        }
    }

    private static interface InternetDependentFlowChooser {
        void choose(InternetDependentFlow internetDependentFlow);
    }

    private static final InternetDependentFlowChooser INTERNET_IS_AVAILABLE_FLOW_CHOOSER = new InternetDependentFlowChooser() {
        @Override
        public void choose(InternetDependentFlow internetDependentFlow) {
            internetDependentFlow.onInternetIsAvailable();
        }
    };

    private static final InternetDependentFlowChooser INTERNET_IS_NOT_AVAILABLE_FLOW_CHOOSER = new InternetDependentFlowChooser() {
        @Override
        public void choose(InternetDependentFlow internetDependentFlow) {
            internetDependentFlow.onInternetIsNotAvailable();
        }
    };
}

And how InternetDependentFlowTask is using:

playButton.setOnClickListener(new View.OnClickListener() {
    public void onClick(View v) {
        MusicManager.playClickBig();
        InternetDependentFlowTask.run(WelcomeActivity.this, new InternetDependentFlow() {
            @Override
            public void onInternetIsAvailable() {
                startGameActivity();
                finish();
            }
            @Override
            public void onInternetIsNotAvailable() {
                notifyUserInternetIsNotAvailable();
            }
        });
    }
});

As a matter of principle,
an interface definition should not include implementation.
These abstract classes are by definition implementations,
even though they are mostly empty and seemingly harmless.
As such,
yes, you should move the inner abstract class definitions outside.

There are different kinds of "outside":

Outside of the class but in the same file
Outside of the class in other file(s)

If they are in the same file, they cannot be public.

If they are in different files, they can be public.

Either way, to achieve the cleanest separation of interface and implementations,
I recommend to move them to different file(s).


Substituting Hebrew letter variants I have two functions that replace letters. In one function, I'm replacing a regular Hebrew letter with its final form, and the other function undoes this operation. (Consider it like lowercase/uppercase, except only a few letters have both forms.)

I created an object where the keys are the regular forms and the values are the final forms. In each function, I iterate over the object and run a regex - in one function it's map[i] -> i; the other is i -> map[i].

Would it be worth it / even possible to abstract this for loop into a function?

var map = {
    כ: 'ך',
    מ: 'ם',
    נ: 'ן',
    פ: 'ף',
    צ: 'ץ'
};

exports.toFinal = function(text) {
    for (var i in map) {
        text = text.replace(new RegExp(i,'g'), map[i]);
    }
    return text;
}

exports.toRegular = function(text) {
    for (var i in map) {
        text = text.replace(new RegExp(map[i],'g'), i);
    }
    return text;
}

(exports is Node.js's exports object, and the code is implicitly running in a closure.)

I would opt for another implementation.

You iterate over your map and for each value, you replace a character in the source string. So for n characters in your map, you run n replacement operations for a length of m. Given, that n = m, that takes n^2 steps.

I would make two maps; one for each direction:
(for simplicity reasons, I chose latin letters)

var form1 = {
    'a':'A',
    'b':'B'
};

var form2 = {
    'A':'a',
    'B':'b'
};

If I need to know, whether there is a translation, I could do a simple operation form1['a'] or form1['x'].

A translation - independent of the direction - could be defined as:

function translate(x){
  return (map[x])?map[x]:x;
}

Is a letter in the map, translate it, else return the original letter.

To specify the direction, you could define a closure, passing in the appropriate map:

 function makeTranslation(map){
    return function translate(x){
        return (map[x])?map[x]:x;
    }
}

If you want to translate more than one character, you have to split the string and translate each character. After that join the result:

function translate(expression, direction){
    return expression.split("").map(direction).join("");
}

Which results in two functions:

function toFinal(expression){
    return translate(expression, makeTranslation(form1));
}

function toRegular(expression){
    return translate(expression, makeTranslation(form2));
}

The resulting code looks like this:

var form1 = {
        'a':'A',
        'b':'B'
};

var form2 = {
    'A':'a',
    'B':'b'
};

function makeTranslation(map){
    return function translate(x){
        return (map[x])?map[x]:x;
    }
}

function translate(expression, direction){
    return expression.split("").map(direction).join("");
}

function toFinal(expression){
    return translate(expression, makeTranslation(form1));
}

function toRegular(expression){
    return translate(expression, makeTranslation(form2));
}

console.log(toRegular(toFinal("aabbcbb"))==='aabbcbb');
console.log(toFinal(toRegular("AABBcBB"))==='AABBcBB');


Find primes using "naive" algorithm I know I can use Sieves to make this faster, but I wondered if there's anything I'm missing with this implementation of the "naive" algorithm.

On my PC running the command (first million primes):

  g++ -std=c++11 -Wall -Wextra -Ofast primes.cpp -o primes && time ./primes 15485863

this takes approximately 9.3 secs.

#include <climits>
#include <cstdio>
#include <string>
#include <vector>

typedef unsigned long long ull;

int main(int argc, char *argv[])
{
    ull limit = ULLONG_MAX;
    if (argc > 1) limit = std::stoi(argv[1]);

    std::vector<ull> primes;
    primes.push_back(2);
    printf("2 is prime\n");
    primes.push_back(3);
    printf("3 is prime\n");
    primes.push_back(5);
    printf("5 is prime\n");
    int inc = 2;
    for (ull i = 7; i <= limit; i += inc) {
        bool isprime = true;
        for (ull j = 0; primes[j] * primes[j] <= i && j < primes.size(); j++) {
            if (i % primes[j] == 0) {
                isprime = false;
                break;
            }
        }

        if (isprime) {
            printf("%llu is prime\n", i);
            primes.push_back(i);
        }
        inc == 2 ? inc = 4 : inc = 2;
    }
}

A quick detail

You've written inc == 2 ? inc = 4 : inc = 2; : this is not the ""right"" way to use the "ternary" operator. Don't get me wrong, it will do what you expect it to do. However, the whole point of the operator is to return a value. In your case, it is no different that : if (inc == 2) inc = 4; else inc = 2;.

This calls for a ternary operator but to write it in a simple way : inc = (inc == 2) ? 4 : 2;


Utility method to view submatrices I implemented an utility method that is able to view an Array[n, m] in another smaller dimension [k, l], where n%k = 0 and m%l = 0.

Example:

              -- --     -- --
---   ---     |1 2|     |3 4|
|1 2 3 4|     |5 6|     |7 8|
|5 6 7 8|  => -- --(a0) -- --(a1)
|4 3 2 1|     -- --     -- --
|8 7 6 5|     |4 3|     |2 1|
---   ---     |8 7|     |6 5|
              -- --(a2) -- --(a3)

public static IEnumerable<T[,]> ViewAsNByM<T>(this T[,] values, int n, int m)
{
    var count = values.GetLength(0)*values.GetLength(1);
    for (int i = 0; i < count/(n*m); ++i)
    {
        var matrix = new T[n, m];
        for (int j = 0; j < n; ++j)
        {
            for (int k = 0; k < m; ++k)
            {
                matrix[j, k] = values[j + (i / m * n), (k + i*m)% values.GetLength(0)];
            }
        }
        yield return matrix;
    }
}

Nunit test:

[Test]
public void TestViewAsNbyM()
{
    var numbers = new[,]
    {
        {1, 2, 3, 4},
        {5, 6, 7, 8},
        {4, 3, 2, 1},
        {8, 7, 6, 5}
    };

    var aux = numbers.ViewAsNByM(2, 2).ToList();
    Assert.AreEqual(4, aux.Count);

    var a0 = new[,] {{1, 2}, {5, 6}};
    var a1 = new[,] {{3, 4}, {7, 8}};
    var a2 = new[,] { { 4, 3 }, { 8, 7 } };
    var a3 = new[,] { { 2, 1 }, { 6, 5 } };

    Assert.That(aux, Is.EquivalentTo(new[] {a0,a1,a2,a3}));
}

Any thoughts about the implementation?

Your solution seems to work fine fine for square matrix(where n = m). However, when presented with non-square matrix, the result becomes purely sporadic. This may or may not be problematic, depending on which kind of matrix you have to deal with.
Modulo % is quite expensive.
You are copying the array item by item. It is not the best way to it, especially when you are copying a continuous section within an array (where m > 1)
You should cache the result of values.GetLength(0), and more importantly values[0].GetLength(0), as they are expensive.

public static IEnumerable<T[,]> ViewAsNByM2<T>(this T[,] values, int n, int m)
{
    int height = values.GetLength(0), width = values.GetLength(1);
    int rows = height / n, columns = width / m;

    for (int row = 0; row < rows; row++)
    {
        for (int column = 0; column < columns; column++)
        {
            var matrix = new T[n, m];
            for (int i = 0; i < n; i++)
            {
                Array.ConstrainedCopy(
                    values, (row * n + i) * width + column * m,
                    matrix, i * m,
                    m);
            }
            yield return matrix;
        }
    }
}


How do I make this C# code more efficient, in terms of FPS and time complexity? Image<Bgr, Byte> frame = _capture.QueryFrame();
Image<Gray, Byte> grayFrame = frame.Convert<Gray, Byte>();
grayFrame._EqualizeHist();
MCvAvgComp[][] facesDetected = grayFrame.DetectHaarCascade(_faces, 1.1, 1,
    Emgu.CV.CvEnum.HAAR_DETECTION_TYPE.FIND_BIGGEST_OBJECT, new Size(20, 20));

if (facesDetected[0].Length == 1)
{
    MCvAvgComp face = facesDetected[0][0];

    #region Search ROI based on Face Metric Estimation -

    Int32 yCoordStartSearchEyes = face.rect.Top + (face.rect.Height * 3 / 11);
    Point startingPointSearchEyes = new Point(face.rect.X, yCoordStartSearchEyes);
    Size searchEyesAreaSize = new Size(face.rect.Width, (face.rect.Height * 3 / 11));
    Rectangle possibleROI_eyes = new Rectangle(startingPointSearchEyes, searchEyesAreaSize);

    #endregion

    int widthNav = (frame.Width / 10 * 2);
    int heightNav = (frame.Height / 10 * 2);
    Rectangle nav = new Rectangle(new Point(frame.Width / 2 - widthNav / 2, frame.Height / 2 - heightNav / 2),
        new Size(widthNav, heightNav));
    frame.Draw(nav, new Bgr(Color.Lavender), 3);
    Point cursor = new Point(face.rect.X + searchEyesAreaSize.Width / 2,
        yCoordStartSearchEyes + searchEyesAreaSize.Height / 2);

    grayFrame.ROI = possibleROI_eyes;
    MCvAvgComp[][] eyesDetected = grayFrame.DetectHaarCascade(_eyes, 1.15, 3,
        Emgu.CV.CvEnum.HAAR_DETECTION_TYPE.DO_CANNY_PRUNING, new Size(20, 20));
    grayFrame.ROI = Rectangle.Empty;

    if (eyesDetected[0].Length != 0)
    {
        frame.Draw(face.rect, new Bgr(Color.Yellow), 1);

        foreach (MCvAvgComp eye in eyesDetected[0])
        {
            Rectangle eyeRect = eye.rect;
            eyeRect.Offset(possibleROI_eyes.X, possibleROI_eyes.Y);
            grayFrame.ROI = eyeRect;
            frame.Draw(eyeRect, new Bgr(Color.DarkSeaGreen), 2);

            frame.Draw(possibleROI_eyes, new Bgr(Color.DeepPink), 2);

            if (nav.Left < cursor.X && cursor.X < (nav.Left + nav.Width)
                && nav.Top < cursor.Y && cursor.Y < nav.Top + nav.Height)
            {
                LineSegment2D CursorDraw = new LineSegment2D(cursor, new Point(cursor.X, cursor.Y + 1));
                frame.Draw(CursorDraw, new Bgr(Color.White), 3);

                int xCoord = (frame.Width * (cursor.X - nav.Left)) / nav.Width;
                int yCoord = (frame.Height * (cursor.Y - nav.Top)) / nav.Height;

                Cursor.Position = new Point(xCoord, yCoord);
            }
        }
    }
    imageBoxFrame.Image = frame;
}

I think your code is already quite good. My guess is that the calls to DetectHaarCascade are the majority of the time. Can you run a profiler on it? The Equatec profiler is free. As for the lower loop you could preallocate your colors (although I assume they're structs). Also, I don't see where possibleROI_eyes changes, so do you really need to draw that every time through the loop? Or is it drawing at the cursor position?


Merge sort C++14 implementation Here is my merge sort c++ implementation. Looking for code improvement and optimization opinion. Just one question which is better *k++ = *i++; or, *k = *i; ++k; ++i;.

/*
 *    Merge Sort
 */

#include <iostream>
#include <vector>

void mergeSort(std::vector<int> &v, std::vector<int>::iterator p, std::vector<int>::iterator r);
void merge(std::vector<int> &v, std::vector<int>::iterator p, std::vector<int>::iterator q, std::vector<int>::iterator r);
void print(const std::vector<int> &v);

int main()
{
    std::ios_base::sync_with_stdio(false);

    std::vector<int> v;

    int temp = 0;
    while (std::cin >> temp) {
        v.push_back(temp);
    }

    // Sort
    mergeSort(v, v.begin(), v.end() - 1);
    // Print the vector after sorting.
    print(v);

    return 0;
}

void mergeSort(std::vector<int> &v, std::vector<int>::iterator p, std::vector<int>::iterator r) {
    // Base case: if (p >= r) then return
    if (p < r) {
        auto q = p + (r - p) / 2;

        mergeSort(v, p, q);
        mergeSort(v, q+1, r);
        merge(v, p, q, r);
    }
}

void merge(std::vector<int> &v, std::vector<int>::iterator p, std::vector<int>::iterator q, std::vector<int>::iterator r) {
    std::vector<int>::size_type leftVectorSize = q - p + 1;
    std::vector<int>::size_type rightVectorSize = r - q;

    std::vector<int> vLeft(leftVectorSize);
    std::vector<int> vRight(rightVectorSize);

    // Copy elements p to q
    auto it = p;
    for (auto &i : vLeft) {
        i = *it++;
    }

    // Copy elements q + 1 to r
    for (auto &i : vRight) {
        i = *it++;
    }

    auto i = vLeft.cbegin(), j = vRight.cbegin();
    auto k = p;

    // Compare and merge
    while (i != vLeft.end() && j != vRight.end()) {
        if (*i <= *j) {
            *k++ = *i++;
        } else {
            *k++ = *j++;
        }
    }

    // Copy rest of the elements if any
    while (i != vLeft.end()) {
        *k++ = *i++;
    }

    // Copy rest of the elements if any
    while (j != vRight.end()) {
        *k++ = *j++;
    }
}

void print(const std::vector<int> &v) {
    for (const auto &i : v) {
        std::cout << i << "\n";
    }
}

Advice 1

mergeSort(v, v.begin(), v.end() - 1);

So your implementation requires the actual vector + two iterators; this is an anti-pattern, you should strive for the following API:

mergeSort(v.begin(), v.end());

Note v.end() instead of v.end() - 1, since end() points to the one element past the last element; this is the STL approach.

Advice 2

Same applies to the merge.

Summa summarum

If you want to improve your coding, you could start by investigating the following:

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

void mergeSort(std::vector<int>::iterator begin,
               std::vector<int>::iterator end);

void merge(std::vector<int>::iterator begin,
           std::vector<int>::iterator middle,
           std::vector<int>::iterator end);

void print(const std::vector<int> &v);

int main()
{
    std::ios_base::sync_with_stdio(false);

    std::vector<int> v;

    int temp = 0;
    while (std::cin >> temp) {
        v.push_back(temp);
    }

    // Sort
    mergeSort(v.begin(), v.end());
    // Print the vector after sorting.
    print(v);

    return 0;
}

void mergeSort(std::vector<int>::iterator begin, std::vector<int>::iterator end) {
    auto distance = std::distance(begin, end);

    if (distance < 2) {
        return;
    }

    auto middle = begin;
    std::advance(middle, distance / 2);
    mergeSort(begin, middle);
    mergeSort(middle, end);
    merge(begin, middle, end);
}

void merge(std::vector<int>::iterator begin,
           std::vector<int>::iterator middle,
           std::vector<int>::iterator end) {
    auto distance = std::distance(begin, end);
    std::vector<int> aux(distance);

    auto left = begin;
    auto left_bound = middle;
    auto right = middle;
    auto right_bound = end;
    size_t aux_index = 0;

    while (left != left_bound && right != right_bound) {
        if (*right < *left) {
            aux[aux_index++] = *right;
            ++right;
        } else {
            aux[aux_index++] = *left;
            ++left;
        }
    }

    std::copy(left, left_bound, &aux[aux_index]);
    std::copy(right, right_bound, &aux[aux_index]);
    std::copy(aux.begin(), aux.end(), begin);
}

void print(const std::vector<int> &v) {
    for (const auto &i : v) {
        std::cout << i << " ";
    }

    std::cout << std::endl;
}

Advice 3

Making the sort more idiomatic (a template) and efficient (double buffer strategy) is not too hard:

template<typename RandomIter1, typename RandomIter2>
void merge(RandomIter1 begin,
           RandomIter1 middle,
           RandomIter1 end,
           RandomIter2 aux) {
    RandomIter1 left = begin;
    RandomIter1 right = middle;
    RandomIter1 left_bound = middle;
    RandomIter1 right_bound = end;

    while (left != left_bound and right != right_bound) {
        if (*right < *left) {
            *aux = *right;
            ++right;
        } else {
            *aux = *left;
            ++left;
        }

        ++aux;
    }

    std::copy(left, left_bound, aux);
    std::copy(right, right_bound, aux);
}

template<typename RandomIter1, typename RandomIter2>
void merge_sort(RandomIter1 source_begin,
                RandomIter1 source_end,
                RandomIter2 target_begin,
                RandomIter2 target_end) {
    auto distance = std::distance(source_begin, source_end);

    if (distance < 2) {
        return;
    }

    RandomIter1 source_middle = source_begin;
    RandomIter2 target_middle = target_begin;
    std::advance(source_middle, distance >> 1); // distance >> 1 is the same as
                                         // distance / 2
    std::advance(target_middle, distance >> 1);

    merge_sort(target_begin,
               target_middle,
               source_begin,
               source_middle);

    merge_sort(target_middle,
               target_end,
               source_middle,
               source_end);

    merge(source_begin,
          source_middle,
          source_end,
          target_begin);
}

template<typename RandomIter>
void merge_sort(RandomIter begin, RandomIter end) {
    auto distance = std::distance(begin, end);

    if (distance < 2) {
        return;
    }

    using value_type = typename std::iterator_traits<RandomIter>::value_type;
    value_type* aux = new value_type[distance];
    std::copy(begin, end, aux);
    merge_sort(aux, aux + distance, begin, end);
    delete[] aux;
}

When I compare the above to your version (demo here), I get the following figures when using -O3 optimization flag:

OP time: 377 milliseconds.
coderodde time: 144 milliseconds.
Algorithms agree: true

Hope that helps.


Getting client timezone name to use with PHP date_default_timezone_set() I would like to get the current user's timezone name so it can be used with the PHP date_default_timezone_set(). After looking up a few things I've finally managed to come up with a solution for this. I'm not great with code though so any code checks/help will be appreciated.

As long as it is a fair guess of the user's timezone, it is good enough for me. It seems to work OK. Here it is:

/**
 * JS/jQuery - Set cookie of current user's UTC offset (format example: -06:00)
 *
 */
$(document).ready( function() {

        // Get UTC offset
        var offset = new Date().getTimezoneOffset(), o = Math.abs(offset);
        var offset = (offset < 0 ? "+" : "-") + ("00" + Math.floor(o / 60)).slice(-2) + ":" + ("00" + (o % 60)).slice(-2);

        if (document.cookie.indexOf("hp_time_offset") >= 0) {} else {

            // Set cookie
            $.cookie( 'hp_time_offset', offset, { expires : null, path:'/' } );
        }

});

/**
 * PHP - Get the UTC offset cookie and convert it to a timezone name
 *
 */
function hp_get_timezone() {

    if( isset( $_COOKIE['hp_time_offset'] ) ) {

        $time_offset = $_COOKIE['hp_time_offset'];

        // validate - exit if offset cookie contains anything other than "0-9 + - :",
        if ( preg_match('/[^0-9:\+\-]/', $time_offset) )
            return;

        // validate - exit if offset cookie is not in correct range
        if ( ! in_array($time_offset, range(-12, 14)))
            return;

        // Calculate seconds from offset
        list($hours, $minutes) = explode(':', $time_offset);
        $seconds = $hours * 60 * 60 + $minutes * 60;

        // Get timezone name from seconds
        $tz = timezone_name_from_abbr('', $seconds, 1);

        // bug fix
        if($tz === false)
            $tz = timezone_name_from_abbr('', $seconds, 0);

        // if still false, set a default timezone name.
        if($tz === false)
            $tz = 'America/New_York';

    } else {

        // If cookie doesn't exist, set a default timezone name.
        $tz = 'America/New_York';
    }

    return $tz;
}

//set timezone
date_default_timezone_set( hp_get_timezone() );

Any help appreciated.

On the javascript side:

I am not seeing any value in creating the string format for the offset that you are creating, as you just end up spending more code in PHP trying to get to a usable format. This is all more work than necessary. Consider keeping offset in minute-based javascript format or converting to second-based format.
You seem to use your cookie management library inconsistently here.  You write using a jQuery extension like $.cookie() while reading the actual cookie string from document. Why not use your library for both (or does it not support that?)  Also, not sure what you what your library does with expires: null. Hopefully this set to session-length cookie?

On PHP side:

I would strongly consider using PHP's DateTime and related classes (in this case specifically, DateTimeZone) rather than the procedural date_* functions.  Over time I think you will find them much more powerful and flexible in usage.  This also can help move you away from the concept of resetting your default system timezone, which I don't think you want to overwrite here, but rather use as fallback.  There may be other parts of your code that do things like insert records into database datetime fields or something like that where you want to use a consistent system time, not the end users timezone.  I don't think you have an appropriate use case for changing this setting (at least from the context shown).

Let's show an example of how this might look using DateTimeZone:

$system_tz = date_default_timezone_get();

// assume $_COOKIE['hp_time_offset'] contains a minute based offset like
// output by javascript Date().getTimezoneOffset()
if (!empty($_COOKIE('hp_time_offset') &&
    is_numeric($_COOKIE['hp_time_offset'])) {

    $offset_min = (int)$_COOKIE['hp_time_offset'];
    $offset_sec = $offset_min * 60;
    // is current system in daylight savings?
    $is_dst = (int)date('I');
    // get timezone name
    $tz = timezone_name_from_abbr('', $offset_sec, $is_dst);
    try {
        $date_timezone = new DateTimeZone($tz);
    } catch (Exception $e) {
        error_log('Invalid timezone string of "' . $tz . '" passed from cookie.');
        $tz = $system_tz;
    }
}

if(empty($timezone)) {
    // I am not wrapping this in try-catch as if there is a failure here
    // I don't know what you want to do to recover (as this should not happen).
    // Feel free to catch if needed for your implementation
    $date_timezone = new DateTimeZone($tz);
}

return $date_timezone;
// or if you just want to return the timezone string
// return $date_timezone->getName();

This proposed functionality gives you a valid DateTimeZone object representing either the cookie-derived timezone or, as a fallback, the system timezone.  You can use this object to instantiate any DateTime object you may need on your system with those objects automatically taking on the expected timezone behavior.


Rock Climbing Simulation Game I am a rock climber so I made a rock climbing game. I have two conceptual objects; the climber and the route. I ended up putting them both into a single Climber class, and made route an instance method on the class.

I feel like they should be separate, but I was having problems with inheritance, which is why I merged them.

I've also removed some other code to make this more succinct. My code works but I can't help but think it could be better - any and all feedback is appreciated.

import sys

print ("""We are playing a game today at Ship Rock. All the bros are
here. Most points wins.""")

class Climber(object):
    def __init__(self, climber_name, ability):
    self.climber_name = climber_name
    self.ability = ability
    self.routes_climbed = []
    self.energy_level = 100
    self.pump_level = 0
    self.total_score = 0
    self.pitch_count = 0

    def engine(self):
        next_action = input("What next? ")
        if next_action == "climb":
            self.climb_route((str(input("Enter route name: "))), (float(input("Enter route grade: " ))))
        elif next_action == "finish":
            self.grand_finale()

    def climb_route(self, route_name, grade):
        print ("Climbing...", route_name, " Grade...", grade)
        if grade < (.9 * self.ability):
            self.pump_level += (grade * .8)
        elif grade >= (.9 * self.ability):
            self.pump_level += (grade * 1.5)
        self.energy_level -= (grade * 1.7)
        self.total_score += grade
        self.routes_climbed.append(route_name)
        self.pitch_count += 1
        if self.energy_level <= self.pump_level:
            print ("Your day is over.")
            self.grand_finale()
        self.engine()

    def grand_finale(self):
        sys.exit() #quits program

climber1 = Climber("Erich Purpur", 13.0)
climber1.engine()

I don't know anything about climbing. But I'll try this by way of analogy.

I imagine that climbing routes might be something like ski runs. You can have a resort with multiple mountains. Each mountain can have multiple runs. The runs don't necessarily start at the same place, nor end at the same place. The runs can have different levels of challenge.

So you have:

A Resort has m Mountains
A Mountain has

s starting points (where lifts take you)
e ending points (where runs take end, and you can go home)
p points

A Point has r runs leaving it
A Run has a difficulty rating (green circle, blue square, black diamond)

How to approach this in a game? Well, you have some of it already. The pump_level and energy_level are fine.

I would say that the top few items (resort, mountain, starting point) are a series of text prompts. What you really have is a graph. When the player starts, it is at a central node on the graph. This is the "big decision" that will decide the rest of the decisions: "What resort do you want to visit?"

From there, you can change resorts (which is slow and time consuming and expensive, but might be necessary for game play, or might be a "level up" feature- you can only change resorts as you get better). Or you can head for different starting points by taking different ski lifts. Or you could head for a different mountain by taking a van over, then a ski lift. Etc.

Finally, you get to a starting point, pick from a small set of runs. You reach a point at the end if your run, pick from a different small set of runs, reach a different point at the end of that run, etc. Eventually, you reach an ending point - the bottom of the hill. You can call it a day, or go back up for more runs!

I would reverse this for climbing. There are some places to start, maybe some "trails" that take you to the bottoms of a few routes. When you climb up, you reach a point that you can change routes. Then eventually you reach the top, or some other ending point, etc.

Update:

So you have a climber traversing this graph of starting/middle/ending points by various routes. So the nodes (points) have various weighted edges (weights = difficulty, cost, etc.) and they all have names.

Your climber will need some kind of "current node" or "current location" indicator. Your graph of routes should have a common "node" class, with various "routes" connecting the nodes.

class PathNode:
    def __init__(self, ...):
        self.routes = []

class Route:
    def __init__(self, name: string, grade: float, start: PathNode, end: PathNode):
        ...

Whatever your "bottom of the mountain" type is (I called it "Resort" in the ski example) would have a set of standard starting nodes.

class Resort:
    def __init__(self, ...):
        self.start_nodes = []

Your climber would have a current node:

class Climber:
    def __init__(self, ...):
        self.location = None

You might consider making a Resort inherit from PathNode and use a bunch of zero-cost routes to connect to the starting points. That would make everything a graph traversal.

If you put a "message" into each route, describing the struggle, then the Resort -> Start messages could play into that:

  You get in a van with a couple of hipsters, and head for the bottom of El Diablo. I wonder if those guys have what it takes to make the climb?"

You might consider adding in some "trap" nodes, that can only be reached by random chance. Thus, if your energy is low and you try to climb a difficult route, maybe you fall off and have to wait for a rescue chopper...


Finding the longest sequence of increasing numbers in an array A continuation of  this queston.
The task's description can also be found there.

After reformatting the code and getting rid of the ArrayIndexOutOfBounds check,it now looks like this:

public static List<Integer> findLongestIncreasingSequence(int[] numbersToBeProcessed) {

        if (numbersToBeProcessed.length == 0) {
            return null;
        }

        List<Integer> longestIncreasingSequence = new ArrayList<Integer>();
        List<Integer> currentNumbersSequence = new ArrayList<Integer>();

        // the first number will be added always,no matter what
        currentNumbersSequence.add(numbersToBeProcessed[0]);

        for (int i = 1; i < numbersToBeProcessed.length; i++) {

            int currentNumber = numbersToBeProcessed[i];
            int previousNumber = numbersToBeProcessed[i - 1];

            if (currentNumber > previousNumber) {
                currentNumbersSequence.add(currentNumber);
            } else {
                // checks if the current sequence is bigger
                if (currentNumbersSequence.size() > longestIncreasingSequence.size()) {
                    longestIncreasingSequence.clear();
                    longestIncreasingSequence.addAll(currentNumbersSequence);
                }
                // clear the current sequence so it can start all over again
                currentNumbersSequence.clear();
                // after clearing add the current number as the first of the new
                // sequence
                currentNumbersSequence.add(currentNumber);
            }
        }
        // at the end of the loop always compare the two sequences.
        if (currentNumbersSequence.size() > longestIncreasingSequence.size()) {
            longestIncreasingSequence.clear();
            longestIncreasingSequence.addAll(currentNumbersSequence);
        }
        return longestIncreasingSequence;
    }

There are couple of things that bother me about the algorithm above:

We never actually check the n-1 number at the start of the algorithm - instead we directly check if the n number and we process it. This is why I have added the  currentNumbersSequence.add(numbersToBeProcessed[0]); code line before we go in the for loop.
What if the last n number is bigger than n-1? That means that we do not go into the else statement and the current sequence will never be compared against the largest one at this moment.
This is why I had to add an extra check after the for loop. However it leads to a very obvious code repetition.

No need for temporary lists
In your last review, it was mentioned that you could just use two integers to keep track of the sequences instead of two whole lists.  So instead of currentNumbersSequence and longestIncreasingSequence, you could track just the start and length of those two sequences.  The code then becomes:
public static List<Integer> findLongestIncreasingSequence(int[] numbersToBeProcessed) {

    if (numbersToBeProcessed.length == 0) {
        return null;
    }

    int bestStart  = 0;
    int curStart   = 0;
    int bestLength = 1;
    int curLength  = 1;

    for (int i = 1; i < numbersToBeProcessed.length; i++) {
        if (numbersToBeProcessed[i] > numbersToBeProcessed[i-1]) {
            curLength++;
            if (curLength > bestLength) {
                bestStart  = curStart;
                bestLength = curLength;
            }
        } else {
            curStart  = i;
            curLength = 1;
        }
    }
    ArrayList<Integer> ret = new ArrayList<>(bestLength);
    for (int i = 0; i < bestLength; i++) {
        ret.add(numbersToBeProcessed[bestStart+i]);
    }
    return ret;
}


PDF processing by PDFBox app jar I want to convert a large PDF file into individual text files with PDFBox using Python. This code takes lot of time to convert a PDF containing 1000 pages.

Is there anything I can do to improve it?

I cannot use any other library. My other code (Python - parsing of obtained text) works best with output of PDFBox.

import PyPDF2
import sys
import os
import subprocess

pdfFileObj = open('cpdf.pdf', 'rb')
pdfReader = PyPDF2.PdfFileReader(pdfFileObj)
tot = pdfReader.numPages
for i in range(1, tot+1):
    i = str(i)
    ffi = "out\\" + i + "-extracted.txt"
    command = "java -jar" + " " + "pdfbox-app-2.0.2.jar" + " " + "ExtractText" + " " + "cpdf.pdf" + " " + ffi + " " + "-startPage" + " " + i + " " + "-endPage" + " " + i
    subprocess.check_output(command, shell=True)
    print('Completed ' + str(i))

On the other hand, when I do this, it takes 10 seconds:

import subprocess

command = "java -jar pdfbox-app-2.0.2.jar ExtractText cpdf.pdf  out.txt -startPage 1 -endPage 3158"

subprocess.check_output(command, shell=True)

You should use with..as to open a file to make sure it is closed again, even if your program is interrupted:

with open('cpdf.pdf', 'rb') as pdfFileObj:
    ...

If your mode was only r, it could be omitted, as it is the default, however since the file is binary, we need 'rb' here.

Building your command can be greatly simplified using format, leaving out the slow string additions:

command = "java -jar pdfbox-app-2.0.2.jar ExtractText cpdf.pdf {0} -startPage {1} -endPage {1}".format(ffi, i)

Pull the command variable out of the loop and make it a constant.

Get rid of the intermediate variable ffi (which is also a very bad variable name).

Choose better variable names, like page, instead of i.

Follow PEP8 for variable names, use lower_case, not camelCase.

Make the file name a variable (allows passing it in via sys.argv or similar at some point).

Use format's awesome features of replacing named place-holders and locals() to get a dictionary of locally defined variables. Use dictionary unpacking (**dict) to connect the two.

If you are using python 2.x (which does not seem to be the case, judging from your print()), use xrange instead of range to avoid building a list with 1000 elements before iterating and build a generator instead.

Final code:

import sys
import PyPDF2
import subprocess

COMMAND = "java -jar pdfbox-app-2.0.2.jar ExtractText {file_name} out\\{page}-extracted.txt -startPage {page} -endPage {page}"

file_name = sys.argv[1] if len(sys.argv) == 2 else "cpdf.pdf"
with open(file_name, "rb") as pdf_file:
    pages = PyPDF2.PdfFileReader(pdf_file).numPages

for page in range(1, pages + 1):
    subprocess.check_output(COMMAND.format(**locals()), shell=True)
    print('Completed {}'.format(page))

Alternatively, saving the pdf_reader.numPage calls to locals():

subprocess.check_output(COMMAND.format(page=page, file_name=file_name), shell=True)

If the code were longer I would put it into a function. In that case COMMAND should be defined inside that function, because local variable lookup is faster than global variable lookup.

With functions:

import sys
import PyPDF2
import subprocess

def run(file_name, page):
    command = "java -jar pdfbox-app-2.0.2.jar ExtractText {file_name} out\\{page}-extracted.txt -startPage {page} -endPage {page}"
    return subprocess.check_output(command.format(page=page, file_name=file_name), shell=True)

def number_of_pages(file_name):
    with open(file_name, "rb") as pdf_file:
        return PyPDF2.PdfFileReader(pdf_file).numPages

if __name__ == "__main__":
    file_name = sys.argv[1] if len(sys.argv) == 2 else "cpdf.pdf"
    pages = number_of_pages(file_name)

    for page in range(1, pages + 1):
        run(file_name, page)
        print('Completed {}'.format(page))

Regarding your timing: It is probably slow because you are repeatedly starting the java program, accumulating initialization times. Maybe you can split the text obtained from the combined command somehow?


Sorting a list into different bags according to predicate I've just started with F# (I'm coming from a mostly OO background) and I'm looking for

feedback on the code, particularly: is this the way F# code should be written like?
have I overlooked an existing library function which would make this easier or could replace it completely?

The goal is:

Take an input list and sort it according to certain conditions into several bags, keeping also those items not fitting any condition. (Which is why groupBy doesn't do the trick, unless I'm overlooking something).

The function:

let sortIntoBags<'T, 'TBag> (predicate: 'TBag*'T->bool) (bags: 'TBag list)
(lst: 'T list)=
    let take (lst: 'T list) (bag: 'TBag)=
        let isInBag e = predicate (bag, e)
        let (inBag, remaining) = lst |> List.partition isInBag
        ((bag, inBag), remaining)
    let (bagSets, leftOver) = bags |> List.mapFold take lst
    (bagSets, leftOver)

A simple example for usage is:

> let l= [1..25];;

val l : int list =
[1; 2; 3; 4; 5; 6; 7; 8; 9; 10; 11; 12; 13; 14; 15; 16; 17; 18; 19; 20; 21;
 22; 23; 24; 25]

> let bags= [2; 3; 5; 7];;
val bags : int list = [2; 3; 5; 7]

> let isDivisorFor (x, y) = 0=y%x ;;
val isDivisorFor : x:int * y:int -> bool

> l |> sortIntoBags isDivisorFor bags;;
val it : (int * int list) list * int list =
([(2, [2; 4; 6; 8; 10; 12; 14; 16; 18; 20; 22; 24]); (3, [3; 9; 15; 21]);
(5, [5; 25]); (7, [7])], [1; 11; 13; 17; 19; 23])

Here's another (Subjectively better or worse) way to write this, however it does require comparison unlike yours:

let sortIntoBags<'T,'TKey when 'TKey : comparison > (predicate:'TKey*'T -> bool) (bagKeys:'TKey list) (items:'T list) : ('TKey*'T list) list * 'T list =
    let bagMap:Map<'TKey,'T list> = bagKeys |> Seq.map(fun k -> k,List.empty) |> Map.ofSeq
    items
    |> List.rev
    |> List.fold(fun (bagMap,unmatched:'T list) (item:'T) ->
        match bagMap |> Map.tryFindKey(fun k _ -> predicate(k,item)) with
        | Some k -> bagMap |> Map.add k (item::bagMap.[k]), unmatched
        | None -> bagMap,item::unmatched

    ) (bagMap,List.empty)
    |> fun (m,x)->
        m |> Map.toList, x

and the xunit tests I used to make sure it matched the output

open global.Xunit

[<Fact>]
let originalQuestion2 () =
   let l= [1..25]
   let bags= [2; 3; 5; 7]
   let isDivisorFor (x, y) = 0=y%x
   let expected = [(2, [2; 4; 6; 8; 10; 12; 14; 16; 18; 20; 22; 24]); (3, [3; 9; 15; 21]);(5, [5; 25]); (7, [7])], [1; 11; 13; 17; 19; 23]
   let actual = l |> BagSort.sortIntoBags isDivisorFor bags
   printfn "%A" actual
   Console.Error.WriteLine(sprintf "%A" actual)
   Trace.WriteLine(sprintf "%A" actual)
   Assert.Equal(expected, actual)

[<Fact>]
let originalQuestionUnfolded2 () =
   let l= [1..25]
   let bags= [2; 3; 5; 7]
   let isDivisorFor (x, y) = 0=y%x
   let expected =
        let a =
            [   2, [2; 4; 6; 8; 10; 12; 14; 16; 18; 20; 22; 24]
                3, [3; 9; 15; 21]
                5, [5; 25]
                7, [7]
            ]
        let b = [1; 11; 13; 17; 19; 23]
        a,b
   let actual = l |> BagSort.sortIntoBags isDivisorFor bags
   Assert.Equal(expected, actual)
   Assert.Equal((fst expected |> fun x -> x.[0]), fst actual |> fun x -> x.[0])