Removing nodes with a given value from a linked list Is this a good way of removing all the nodes that contains a value x?
This function takes as input the head of a list and has to delete all the nodes that contain a given value taken.  If there is any better way of doing this can you please show them to me?

node_t *funzione(node_t *head){

    int x;
    node_t *temp = head;
    node_t *curr = head;

    temp = (node_t *)malloc(sizeof(node_t));
    curr = (node_t *)malloc(sizeof(node_t));

    printf("Inserisci il valore che vuoi eliminare: \n");
    scanf("%d", &x);

    if (head == NULL)
        exit(0);

    while(head->val == x){
        head = head->next;
        free(temp);
        temp = head;
    }

    curr = head;
    temp = curr->next;

    while (temp != NULL){
        if (temp->val == x){
            if(temp->next != NULL){
                curr->next = temp->next;
                free (temp);
                temp = curr->next;
            }
            else {
                curr->next = NULL;
            }
        }
        else {
            curr = temp;
            temp = temp->next;
        }
    }

    return head;
}

The input asking for which value to delete can/should be placed in a separate function.  Then the function to remove the nodes can take that value as a parameter.

The two malloc calls for temp and curr near the top are completely unnecessary and just leak memory.  You want to remove nodes, so you shouldn't have to allocate any memory.

If your list consists of a single node where val == x (or, more generally, all nodes in the list have that value), your program will crash.

If the last node in the list matches the value (and there are other nodes that don't), you'd be leaking that node except that you will be stuck in an infinite loop.  You shouldn't need to handle the last node any differently than removing any other node.


Simple substitution encryption with Scala I've just started teaching myself Scala to get my feet wet with functional programming, and I'm messing with around with some classical cryptography. The following code produces monoalphabetic simple substitution encryption, but I'm still trying to wrap my head around both Scala itself and functional style, so I'm sure this could be more concise and FP-oriented. For example, I'd love to get rid of var accumText and use a val instead, but I haven't been able to figure out how to make that work. Any advice on getting this more Scala-esque and FP-oriented would be greatly appreciated!

object SimpSub {

  // alphabet array
  private val alphabet = ('A' to 'Z').toList

  def main(args: Array[String]) {
    // split text into char list
    val text = args(0).toUpperCase.toList
    val key = args(1).toUpperCase.toList

    def buildCipherAlphabet(): List[Char] = {
      // build the cipher alphabet by concatenating the key to
      // existing alphabet and removing duplicate letters
      return (key ::: alphabet).distinct
    }

    def encipher(): List[Char] = {
      // get the cipher alphabet
      val newAlph = buildCipherAlphabet()
      var accumText: String = ""
      for (c <- text) {
        // add in whitespace
        if (c == ' ')
            accumText += c
        else
          // add the letter of the cipher alphabet corresponding to the current character
          // in the plaintext
          accumText += newAlph(alphabet.indexOf(c))
      }
      val cipherText = accumText.toList
      return cipherText
    }

    val cipherText = encipher()
    println("Plaintext: " + args(0))
    println("Key: " + args(1))
    print("Ciphertext: ")
    for (c <- cipherText)
      print(c)
    print("\n")
  }
}

encipher() deserves to be a full function with two parameters rather than just a closure within main().  It should take a key parameter first, because you can think of enciphering with a key as a transformation to be applied to the text.  (For that matter, I suggest swapping your args(0) and args(1) as well.)  The function would be more usable if it accepts and returns strings instead of lists.

Instead of writing args(0) twice and args(1) twice, you should assign text and key more appropriately, by not munging them prematurely with .toUpperCase.toList.  I also suggest using destructuring to assign both values at once.

With the changes above, main() can look very clean.

As for the encipher() function, you can simplify it by using

for-comprehensions instead of repeated concatenation to an empty string
pattern matching instead of if-else.

object SimpSub {
  private val alphabet = ('A' to 'Z').toList

  def encipher(key: String, text: String): String = {
    // Build the cipher alphabet by concatenating the key to
    // the alphabet and removing duplicate letters
    val cipherAlphabet = (key.toUpperCase.toList ::: alphabet).distinct

    val cipherText = for (c <- text.toUpperCase) yield c match {
      case ' ' => ' '
      case _   => cipherAlphabet(alphabet.indexOf(c))
    }
    return cipherText.mkString
  }

  def main(args: Array[String]) {
    val Array(text, key) = args

    println("Plaintext: " + text)
    println("Key: " + key)
    println("Ciphertext: " + encipher(key, text))
  }
}


A little C++ program to have timed rounds for a game I know it could be better, but I’m new to C++, so I don’t know many of the tricks, along with some of the includes that are already obsolete.

This program asks for names as input, adds a time for rounds, and then times each player’s turn (i.e. for use in charades). Proven to compile with MinGW cpp.

Precompiled version: https://github.com/ProjectArrow78/EasyGame

#include <iostream>
#include <cmath>
#include <chrono>
#include <ctime>
#include <stdlib.h>
#include <windows.h>
#include <cwchar>
#include <vector>

using namespace std;

vector<string> playerlist;
string currentplayer;
string previousplayer;
bool setprevious = true;
int playercount;
int playerprogression = 1;
int roundtime;
int timescale = 1000;
// Regular timescale: 1000

void collectnames() {
    string playercollected;
    bool continuenameloop;
    bool firstloop = true;
    int playerscollected = 1;

        cout << "How many players will there be?" << endl;
    cin >> playercount;
    playerlist.resize (playercount);
    while(continuenameloop) {
        cout << "Who is player " << playerscollected << "?" << endl;
        if(firstloop == true) {
            cin.ignore();
            firstloop = false;
        }
        getline(cin, playercollected);
        playerlist.insert (playerlist.begin() + playerscollected, playercollected);
        playerscollected++;

        if(playerscollected == playercount+1) {
            continuenameloop = false;
        }
    }
}

void introduction() {
    cout << "Welcome to EasyGame" << endl;
    collectnames();
    cout << "How many minutes per turn?" << endl;
    cin >> roundtime;
}

void playerlogic() {
    playerprogression++;
    previousplayer = currentplayer;
    if(playerprogression > playercount) {
        playerprogression = 1;
    }
    currentplayer = playerlist [playerprogression];
    if(setprevious == true) {
        previousplayer = playerlist [1];
        setprevious = false;
    }
}

void skipturn() {
    char playturn;

    cout << "It is " << playerlist [playerprogression] << "'s turn!" << endl;
    cout << "Play turn? (Y/N)" << endl;
    cin >> playturn;
    if(playturn == 'n' or playturn == 'N') {
        playerlogic();
        skipturn();

    }
}

void playertime() {
    bool runtimer = true;
    int timeremaining = roundtime;

    while(runtimer) {
        cout << string( 100, '\n' );
        cout << "You have " << timeremaining << " minutes remaining." << endl;
        Sleep(roundtime * timescale * 12);
        timeremaining--;
        if(timeremaining == 0) {
            cout << string( 100, '\n' );
            cout << previousplayer << "'s time is up! \n" << endl;
            runtimer = false;
        }
    }
}

int main() {
    system("color 71");
    SetConsoleTitle("EasyGame");
    introduction();
    while(true) {
        playerlogic();
        playertime();
        skipturn();
    }
}

First, it's really not too bad for a beginner, there are functions with local variables and the function names are descriptive. I'm impressed with the fact that all if statements and loops use the braces ({ and }) to create compound statements even though it isn't strictly necessary, it does make maintenance much easier.

Initialize Local Variables
In the function collectnames() the variable continuenameloop is uninitialized. In some program languages this isn't a problem local variables are initialized with a null value which might make continuenameloop false (not what is desired in this case). In C++ all local variable are unitialized which means they are in an unknown state. This may have been a typo since the other local variables are initialized.

Prefer std::NAME over using namespace std;
If you are coding professionally you probably should get out of the habit of using the "using namespace std;" statement. The code will more clearly define where cout and other functions are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The function cout you may override within your own classes. This stack overflow question discusses this in more detail.

Global Variables
Generally global variables are considered a bad practice (2 references from stackexchange sites, stackoverflow and software engineering). In larger programs with multiple source files it is very difficult to write correct code, debug and maintain code using global variables. Finding where a variable value is altered becomes a huge task. A second problem with global variables in a multi-file program is that global variables are global in scope and can lead to duplicate definitions at link time. In an object oriented program all of these variables could have been global within the object but the scope would still be limited to the object itself.

In non-object oriented programming it might be better to declare the variables in functions or procedures and pass the values into sub functions.

Error Checking
There doesn't seem to be any error checking, users have been known to enter the wrong information. What happens if a negative value for roundtime is entered?

Use Objects
As the game gets more complex it might be a good idea to create classes to implement it. Most of the this current program might fit into a class called game. It might be beneficial if users were a separate class with scores, and possibly  elapsed times. Using objects will allow the code to be independent from other portions of the code, and details of the implementation can be hidden.


Project Euler #10 (sum of primes less than two million) in Clojure I'm new to Clojure and using some Project Euler problems to familiarize myself with the language's capabilities. Problem #10 asks to find the sum of all primes less than two million.

I wanted to solve this problem by building a generalized Sieve of Eratosthenes, then adding up all the primes:

(defn sieve [size]
  (-> (reduce (fn [prev i]
                (if (prev i)
                  (vec (reduce #(assoc %1 %2 false)
                               prev
                               (range (+ i i) size i)))
                  prev))
              (vec (repeat size true))
              (range 2 (-> size Math/sqrt int inc)))
      vec
      (assoc 0 false)
      (assoc 1 false)))

(time
 (let [limit 2000000
       s (sieve limit)]
   (reduce #(+ %1 (if (s %2) %2 0))
           0 (range limit))))

That works, but takes over 14 seconds when run from an already "hot" REPL. For comparison, I did a simple Java implementation:

import java.util.Arrays;
import java.lang.Math;

public class P10 {
  public static void main(String[] args) {
    int size = 2000000;

    boolean[] sieve = new boolean[size];

    Arrays.fill(sieve, true);
    sieve[0] = false;
    sieve[1] = false;

    for (int i = 2; i < (1 + Math.sqrt(size)); i++) {
      if (sieve[i]) {
        for (int j = i + i; j < size; j += i) {
          sieve[j] = false;
        }
      }
    }

    long sum = 0;
    for (int i = 0; i < size; i++) {
      if (sieve[i]) sum += (long) i;
    }

    System.out.println(sum);
  }
}

Which runs in about 0.7 seconds from the command line. (Incidentally, my Java experience isn't very deep either--does that include JVM startup?)

So I thought maybe the problem with the "pure" (?) Clojure version is the creation/reliance on Clojure's vectors. So my idea was to rework the Clojure version to use Java's mutable array:

(defn sieve [size]
  (-> (let [ar (to-array (repeat size true))]
        (dorun (map (fn [i]
                      (when (aget ar i)
                        (dorun (map (fn [j] (aset ar j false))
                                    (range (+ i i) size i)))))
                    (range 2 (-> size Math/sqrt int inc))))
        ar)
      vec
      (assoc 0 false)
      (assoc 1 false)))

But that takes more than 3.5 seconds.

At this point, I'm not so interested in implementing fancy things like bit-packing, I'm just trying to reach performance parity with the "traditional" Java implementation. What am i doing "wrong" in my Clojure that makes it "non-competitive" with the Java version?

Also, any other tips/hints/criticisms regarding Clojure style?

I won't comment on the Java bit because I don't write Java very often. I agree with the other answer though that sqrt should only be called once. Computing the sqrt of a number is relatively expensive.

First, for Clojure, a baseline on my computer:

(defn sieve [size]
    (-> (reduce (fn [prev i]
                  (if (prev i)
                    (vec (reduce #(assoc %1 %2 false)
                                 prev
                                 (range (+ i i) size i)))
                    prev))
                (vec (repeat size true))
                (range 2 (-> size Math/sqrt int inc)))
        vec
        (assoc 0 false)
        (assoc 1 false)))

(defn main []
  (time
    (let [limit 2000000
          s (sieve limit)]
      (reduce #(+ %1 (if (s %2) %2 0))
              0 (range limit)))))

; From REPL
(main)
"Elapsed time: 2274.584779 msecs"
=> 142913828922

I should note that I'm on a crappy M3 Surface Pro 4; not a power machine by a stretch of any definition. If this is taking 14 seconds, you must have something else going on. That's far too long.

I actually prefer wrapping threading calls in parenthesis, even if they aren't required. I'd write lines such as (-> size Math/sqrt int inc) as (-> size (Math/sqrt) (int) (inc)). I find it helps differentiating between the form being a plain function call, and the threading of a value. The only time I omit them is when I'm golfing.

Your nested reduction is a lot going on in only a few lines. I would definitely break the inner reduction off into its own function, give it a descriptive name, and call it in the outer reduction.

(defn calc [size i prev] ; Although give it a better name
  (vec (reduce #(assoc %1 %2 false)
               prev
               (range (+ i i) size i))))

(defn sieve [size]
    (-> (reduce (fn [prev i]
                  (if (prev i)
                    (calc size i prev)
                    prev))
                (vec (repeat size true))
                (range 2 (-> size Math/sqrt int inc)))
        vec
        (assoc 0 false)
        (assoc 1 false)))

I like breaking functions up. Having giant functions forces you to consider a large amount of data and interactions at once. If you split a part off into its own function and ensure that it's pure, you know that no code in that function can manipulate any data that you haven't explicitly given it as a parameter. That's one less bit of the code to need to look at when debugging.

For your second bit, interestingly, it performed the same for me:

(main) ; Using your arrays version
"Elapsed time: 2106.530431 msecs"
=> 142913828922

You're making a critical error though when dealing with arrays in Clojure: don't use aset unless you have to. aset works on Objects, not primitives, as its documentation notes:

  Sets the value at the index/indices. Works on Java arrays of
    reference types. Returns val.

Using aset forces your bools to be boxed into a Boolean, which is pricey when done over and over. aset has multiple overloads for primitive types that should be used instead. I'd switch to aset-boolean, and create the array using boolean-array instead:

(defn sieve2 [size]
  (-> (let [ar (boolean-array (repeat size true))]
        (dorun (map (fn [i]
                      (when (aget ar i)
                        (dorun (map (fn [j] (aset-boolean ar j false))
                                    (range (+ i i) size i)))))
                    (range 2 (-> size Math/sqrt int inc))))
        ar)
      vec
      (assoc 0 false)
      (assoc 1 false)))

(defn main []
  (time
    (let [limit 2000000
          s (sieve2 limit)]
      (reduce #(+ %1 (if (s %2) %2 0))
              0 (range limit)))))

(main)
"Elapsed time: 759.412393 msecs"
=> 142913828922

A little better. I'll also note though that use of (dorun (map is usually a code smell. You're better off just using mapv which is strict. Using laziness when you don't need it can be costly. maping isn't the right tool here for the job anyways. Use map when you want to transform a list. Use doseq when you want to carry out side effects over a list. Think of it as Java's enhanced for-loop. I'm also using doseq's :when clause to replace the use of the when macro:

(defn sieve2 [size]
  (let [ar (boolean-array (repeat size true))]
    (doseq [i (range 2 (-> size (Math/sqrt) (int) (inc)))
            :when (aget ar i)] ; Replacing the call to when
      (doseq [j (range (+ i i) size i)]
        (aset-boolean ar j false)))

    (-> ar
        (vec)
        (assoc 0 false)
        (assoc 1 false))))

(main)
"Elapsed time: 577.607879 msecs"
=> 142913828922

Still not great, but better.

Just for a laugh comparison, here's a implementation of Eratosthenes' Sieve I wrote back in April:

(defn sieve-primes [n]
  (loop [p 2 ; First prime
         marked #{} ; Found composites
         primes []]
    (let [mults (->> n
                     (range p)
                     (map #(* p %))
                     (take-while #(< % n)))

          next-p (->> p
                      (inc)
                      (iterate inc)
                      (remove marked)
                      (first))

          new-primes (conj primes p)]

      (if (>= next-p n)
        new-primes
        (recur next-p (into marked mults) new-primes)))))

(time
  (apply + (sieve-primes 2000000)))
"Elapsed time: 8719.010487 msecs"
=> 142913828922

Ouch! I don't think I tried very hard to get it lower though. That was the first time I had ever written the sieve in any language, and was happy with just having a working implementation at the time. It is pretty though, even if it's slow.

Clojure is really not a performance-oriented language though. Yes, you can add all sorts of optimizations to make good use of all cores and limit intermediate lists when doing transformations, but it generally won't perform as well as other languages. It's generally even slower than Java of all languages in my experience. I write Clojure because it's fun to write and produces nice looking code, not because it produces fast code.


Compare sequence & maps headers in fasta file This is the perl code which compares the sequence in fasta file & maps the header. Though the code is working well, I still would like to make it more efficient. Since the files I compare has >100000 sequences it is taking huge time despite using hashes. Can you please suggest more efficient way ?

Example:

File1:
>Seq1
ABCDEFGH
>Seq2
RNADIMSEQ
>Seq6
XYZ

File2:
>Seq3
ABCDEFGH
>Seq4
RNADIMSEQ

Output:
>Seq1  >Seq3
>Seq2  >Seq4
>Seq6  Not found

The code:
 my $start_run = time();

%hash=();
open(out, ">Output.txt");
open(sbjt, "File1.fasta") or die "File not found"; #Bigger file
$count =0;
while(<sbjt>)
    {
    chomp;
    if($_ =~ m/^\w+/)
        {
            $hash{$previous} = $_  ;
            #print "$previous\n";
        }
    else
        {
        $previous = $_;
        }
    $count++ ;
    }
    close sbjt;
    #print "$hash{$previous}";

%dash=();
$previous = undef;
open(query, "File2.fasta") or die "File not found"; #smaller
while(<query>)
    {
    chomp;
    if($_ =~ m/^\w+/)
        {
            $dash{$previous} = $_  ;
            #print "$previous\n";
        }
    else
        {
        $previous = $_;
        }
    }
    close query;

foreach $key (keys %dash)
{
         foreach $temp (keys %hash)
         {
            if($hash{$temp} eq $dash{$key})
            {
                push(@new_array, $temp);
            }
            next; #added
        }
        if(scalar @new_array>0)
        {
            print out "$key\t@new_array\n";
        }
        else
        {
        print  out "$key\tNot found\n";
        }
        @new_array=();
 }

 my $end_run = time();
my $run_time = $end_run - $start_run;
print "Job took $run_time seconds\n";

Obvious things: use strict and warnings. Use three argument form of open, use lexical file handles, check the success of open or use autodie.

When I run the program, Seq6 is not reported in the output. That's because the outer loop exhausts File2, not File1.

#!/usr/bin/perl
use warnings;
use strict;

my $start_run = time;
my %hash;
my $previous;

open my $SBJ, '<', 'File1.fasta' or die 'File not found';
while (<$SBJ>) {
    chomp;
    if (/^\w+/) {
        $hash{$previous} = $_;
    } else {
        $previous = $_;
    }
}
close $SBJ;

my %dash;
undef $previous;
open my $QUERY, '<', 'File2.fasta' or die 'File not found';
while (<$QUERY>) {
    chomp;
    if (/^\w+/) {
        $dash{$previous} = $_;
    } else {
        $previous = $_;
    }
}
close $QUERY;

open my $OUT, '>', 'Output.txt' or die $!;
my @new_array;
for my $temp (keys %hash) {
    for my $key (keys %dash) {
        if($hash{$temp} eq $dash{$key}) {
            push @new_array, $key;
        }
    }
    if (@new_array > 0) {
        print {$OUT} "$temp\t@new_array\n";
    } else {
        print {$OUT} "$temp\tNot found\n";
    }
    @new_array = ();
}
close $OUT or die $!;

my $end_run = time;
my $run_time = $end_run - $start_run;
print "Job took $run_time seconds\n";

If performance is a problem, you can try trading memory for speed: Hash by the sequences, not by their names:

#!/usr/bin/perl
use warnings;
use strict;

my $start_run = time;
my %hash;

open my $SBJ, '<', 'File1.fasta' or die 'File not found';
my $id;
while (<$SBJ>) {
    chomp;
    if (/^>/) {
        $id = $_;
    } else {
        $hash{$_} = $id;
    }
}
close $SBJ;

my %dash;
undef $id;
open my $QUERY, '<', 'File2.fasta' or die 'File not found';
while (<$QUERY>) {
    chomp;
    if (/^>/) {
        $id = $_;
    } else {
        $dash{$_} = $id;
    }
}
close $QUERY;

open my $OUT, '>', 'Output.txt' or die $!;
for my $seq1 (keys %hash) {
    if (exists $dash{$seq1}) {
        print {$OUT} "$hash{$seq1}\t$dash{$seq1}\n";
    } else {
        print {$OUT} "$hash{$seq1}\tNot found\n";
    }
}
close $OUT or die $!;

my $end_run = time;
my $run_time = $end_run - $start_run;
print "Job took $run_time seconds\n";

Reading the files is exactly the same code with just the hash name different. I'd refactor it into a subroutine to keep the code DRY (Don't Repeat Yourself):

sub read_fasta {
    my $filename = shift;
    my ($id, %hash);
    open my $FH, '<', $filename or die "Can't open $filename: $!";
    while (<$FH>) {
        chomp;
        if (/^>/) {
            $id = $_;
        } else {
            $hash{$_} = $id;
        }
    }
    return %hash
}

my %hash = read_fasta('File1.fasta');
my %dash = read_fasta('File2.fasta');


Suffix Array Construction I'm trying to implement a suffix array for use in programming competitions. I'm using the CodeForces example and trying to "make it my own" so I can make sure I understand it and feel better about using it.

Here's what I have so far:

struct SuffixArray {
    vector<int> sa, pos, tmp;
    int n, gap;

    SuffixArray(string s) {
        n = s.length();
        sa = pos = tmp = vector<int>(n);
        for (int i=0; i<n; i++) sa[i] = i, pos[i] = s[i], tmp[i] = 0;
        for (gap = 1;; gap *= 2) {
            quickSort(sa, n, 0, n - 1);
            for (int i=0; i<n-1; i++) tmp[i+1] = tmp[i] + sufCmp(sa[i], sa[i+1]);
            for (int i=0; i<n; i++) pos[sa[i]] = tmp[i];
            if (tmp[n-1] == n - 1) break;
        }
    }

    bool sufCmp(int i, int j) {
        if (pos[i] != pos[j]) return pos[i] < pos[j];
        i += gap;
        j += gap;
        return (i < n && j < n) ? pos[i] < pos[j] : i > j;
    }

    int partition(vector<int> &arr, int lo, int hi) {
        int p = arr[hi];
        int pi = lo;
        for (int i=lo; i<hi; i++) {
            if (sufCmp(arr[i], p)) {
                swap(arr[i], arr[pi]);
                pi++;
            }
        }
        swap(arr[hi], arr[pi]);
        return pi;
    }

    void quickSort(vector<int> &arr, int sz, int lo, int hi) {
        if (lo < hi) {
            int p = partition(arr, lo, hi);
            quickSort(arr, sz, lo, p-1);
            quickSort(arr, sz, p+1, hi);
        }
    }
};

This works, but it is hopelessly slower than the original. On this problem from SPOJ, it scores in the range expected by a naive solution which is \$O(n^2 \log n)\$.

My question is, why? My guess is it either has to do with the way I am storing and accessing data (wrapping everything in a struct, using vectors instead of pre-allocated arrays), or it could be an issue with my quickSort implementation (since the suffix array will already be 'mostly' sorted on all but the first iteration, using the last element as pivot could be bad), but I don't understand enough about what's going on under the hood to be sure.

I tested the pivot theory by using the middle element instead of the last element as the pivot for the quicksort... still way too slow.

vector<int> sa, pos, tmp;
int n, gap;

Surely you could come up with better variable names than these? Also, tmp and sa are only used inside the constructor, so they should be local variables, not struct members.

SuffixArray(string s)

This should be const string & s to avoid unnecessary copying of the string.

for (int i=0; i<n; i++) sa[i] = i, pos[i] = s[i], tmp[i] = 0;

The comma operator has a few legitimate uses, but this is not one of them. Coding it this way will lead to problems if you need to modify the loop body at all. The right way to do this is to make a multi-statement loop the usual way: curly braces surrounding multiple lines with one statement on each line.

 for (int i=0; i<n; i++)
 {
   sa[i] = i;
   pos[i] = s[i];
   tmp[i] = 0;
 }

     for (gap = 1;; gap *= 2) {

Why do you have a for loop with an empty termination condition?


Generate a Json from Strings using Jackson library I am pretty new to Java and I got a requirement of converting Strings to a json structure. My below codes works fine but I have to create multiple objects and my code looks bit ugly. I want to know the better approach.

I want the below Json Structure from three Strings SourceApplication, messagType and payload

Required Json Structure

{
    "request": {
        "header": {
            "sourceApplication": "SomeString"
            "messageType": "SomeString"
        },
        "body": {
            "payload": "SomeString"
        }
    }
}

Below is my code.

POJO Class (For mapping)

package com.test.transformer;

import java.io.Serializable;

public class JsonMapper implements Serializable {

    private Request request;

    public Request getRequest ()
    {
        return request;
    }

    public void setRequest (Request request)
    {
        this.request = request;
    }

    public static class Request implements Serializable{

        private Body body;

        private Header header;

        public Body getBody ()
        {
            return body;
        }

        public void setBody (Body body)
        {
            this.body = body;
        }

        public Header getHeader ()
        {
            return header;
        }

        public void setHeader (Header header)
        {
            this.header = header;
        }

    }

    public static class Header implements Serializable{

        private String sourceApplication;
        private String messageType;

        public String getSourceApplication ()
        {
            return sourceApplication;
        }

        public void setSourceApplication (String sourceApplication)
        {
            this.sourceApplication = sourceApplication;
        }

        public String getMessageType ()
        {
            return messageType;
        }

        public void setMessageType (String messageType)
        {
            this.messageType = messageType;
        }

    }

    public static class Body implements Serializable{

        private String payload;

        public String getPayload ()
        {
            return payload;
        }

        public void setPayload (String payload)
        {
            this.payload = payload;
        }
    }

}

My Main Class

package com.test.transformer;

import org.codehaus.jackson.map.ObjectMapper;

import java.io.IOException;

public class JsonTest {

    public static  void main(String[] args) {

        ObjectMapper mapper = new ObjectMapper();

        String srcApp = "TestSrc";
        String msgType = "msgtype";
        String payload = "mainMsg";

        JsonMapper.Header h = new JsonMapper.Header();
        JsonMapper.Body b = new JsonMapper.Body();
        JsonMapper.Request r = new JsonMapper.Request();

        h.setSourceApplication(srcApp);
        h.setMessageType(msgType);
        b.setPayload(payload);

        r.setHeader(h);
        r.setBody(b);

        JsonMapper j = new JsonMapper();

        j.setRequest(r);

        String jsonInString = null;
        try {
            jsonInString = mapper.writerWithDefaultPrettyPrinter().writeValueAsString(j);
        } catch (IOException e) {
            e.printStackTrace();
        }
        System.out.println(jsonInString);
    }
}

A few things:

Use descriptive variable names. A common Java convention is a camelCase version of the class name such as JsonMapper jsonMapper = new JsonMapper();
Unwrap your classes into separate files. This makes the code much easier to read and reuse.
test in your package name is very unusual, and probably not intended.
Test classes typically have the same name as the class they test suffixed with Test, such as JsonMapperTest.
Tests are not written in main methods.
Having getters and setters for everything is a code smell - not necessarily bad, but often misused. Your application doesn't do anything yet, so it's difficult to tell whether any of them will be necessary, but in general I would not add getters and setters until I know I need them.


Loading details of a list of cars I have a tree view with a list of cars and a webservice to get the details for each car.

On page load, there are no cars selected so the details footer is loaded with all cars.

$.ajax({
        type: 'POST',
        url: '@Url.Content("/GTracker/Cars/CarTreeDetails")',
        data: { "id": [] }, // not cars filter
        traditional: true,
        success: function (data) {
            $('#footer-details').html(data);
        }
    });

When the user select cars from the treeview, the details are then updated with only those cars.

This is the changed.jstree event:

    var ajaxDetails;

    $('#carTree').on('changed.jstree', function (e, data) {
        var i, j, p = [];

        // create the id[] array
        for (i = 0, j = data.selected.length; i < j; i++) {
            node = data.instance.get_node(data.selected[i]);
            if (node.data.type != 'root') {
                p.push(parseInt(data.instance.get_node(data.selected[i]).id));
            }
        }

        // cancel previous request
        if ((ajaxDetails && ajaxDetails.readyState != 4)) {
            ajaxDetails.abort()
        }

        // get only selected cars info.
        ajaxDetails = $.ajax({
            type: 'POST',
            url: '@Url.Content("/GTracker/Cars/CarTreeDetails")',
            data: { "id": p },  //cars list
            traditional: true,
            success: function (data) {
                $('#footer-details').html(data);
            }
        });
    });

But I feel I'm repeating myself. How may I merge those two functions together?

one for the on_load.
the other for the js_tree change event.

I think this is a good case for refactoring the ajax call out into its own method.  I have given some suggested changes in the code below.  My comments are inside /* ... */ comments.

/*
 * use more meaningful and specific name
var ajaxDetails;
*/
var carTreeXHR;

$('#carTree').on('changed.jstree', function (e, data) {
    /*
     * why initialize to i and j here?
     * use more meaningful name for selected ID's array
    var i, j, p = [];
    */
    var selectedIds = [];

    // create the id[] array
    /*
     * initialize i and j here.
     * simplify call to get node id from existing 'node' object
    for (i = 0, j = data.selected.length; i < j; i++) {
        node = data.instance.get_node(data.selected[i]);
        if (node.data.type != 'root') {
            p.push(parseInt(data.instance.get_node(data.selected[i]).id));
        }
    }
    */
    for (var i = 0, j = data.selected.length; i < j; i++) {
        var node = data.instance.get_node(data.selected[i]);
        if (node.data.type != 'root') {
            selectedIds.push(parseInt(node.id));
        }
    }
    /*
     * remove from this location and put inside  new function
    // cancel previous request
    if ((ajaxDetails && ajaxDetails.readyState != 4)) {
        ajaxDetails.abort()
    }
    */

    // get only selected cars info.
    /* call new function */
    updateCarTreeDetails(selectedIds);

   /*
   * remove to function
   ajaxDetails = $.ajax({
        type: 'POST',
        url: '@Url.Content("/GTracker/Cars/CarTreeDetails")',
        data: { "id": p },  //cars list
        traditional: true,
        success: function (data) {
            $('#footer-details').html(data);
        }
    });
    */
});

/* New function. Include in appropriate scope */
function updateCarTreeDetails(selectedIds) {
    // abort existing request if needed
    if ((carTreeXHR && carTreeXHR.readyState != 4)) {
        carTreeXHR.abort()
    }
    // execute new request
    // not shown - consider handling ajax failure case as well
    carTreeXHR = $.ajax({
        type: 'POST',
        url: '@Url.Content("/GTracker/Cars/CarTreeDetails")',
        data: { "id": selectedIds },  //cars list
        traditional: true,
        success: function (data) {
            $('#footer-details').html(data);
        }
    });
}

/* In your document.ready handler, you can call this function
 * just like you did inside the change handler */
updateCarTreeDetails([]);

One additional note - it seems odd to be executing a 'POST' method if a case where you are just reading data from the remote data source (i.e. you are not performing a create, update, or delete operation).  If you have control over this API, you might consider implementing as GET.


Retrieving MAC addresses based on PCI interface connections and SQL queries I am currently using this C# method to retrieve MAC addresses from a user's computer as a sort of unique identification. It retrieves the MAC address based on the fact that the physical card is connected to the PCI interface.

public static string returnMAC()
{
    ManagementObjectSearcher searcher = new ManagementObjectSearcher("Select MACAddress, PNPDeviceID FROM Win32_NetworkAdapter WHERE MACAddress IS NOT NULL AND PNPDEVICEID IS NOT NULL");
    ManagementObjectCollection mObject = searcher.Get();

    foreach (ManagementObject obj in mObject)
    {
        string pnp = obj["PNPDeviceID"].ToString();
        if (pnp.Contains("PCI\\"))
        {
            string mac = obj["MACAddress"].ToString();
            mac = mac.Replace(":", string.Empty);
            return mac;
        }
    }
    return "Nothing happened...";
}

A method I used previously was pulled straight from MSDN, but due to some issues it inherently had (like pulling the wireless' MAC instead of the LAN line), I switched it out for this one.

I have tested this method on Windows 7 and 8.1 systems, and it works fine in retrieving the physical MAC address. With that said - it uses SQL queries to search for strings, instead of a more typical method that uses System.Net.XXX - a more "built-in" and "native" method. Correct me if I am wrong on this.

Is there any way to make this better? I can't think of what would make it better, but on the top of my head - LINQ queries? Native methods? I'm not sure.

public static string returnMAC()

The C# coding conventions state that functions should be UpperCamelCase. Additionally that function name is not optimal, returnMAC, return it from where? More appropriate would be GetMAC or RetrieveMAC.

You should add XML documentation to this method.

ManagementObjectSearcher searcher = new ManagementObjectSearcher("Select MACAddress, PNPDeviceID FROM Win32_NetworkAdapter WHERE MACAddress IS NOT NULL AND PNPDEVICEID IS NOT NULL");

This could benefit from using a literal/verbatim string:

ManagementObjectSearcher searcher = new ManagementObjectSearcher(@"
SELECT
    MACAddress,
    PNPDeviceID
FROM
    Win32_NetworkAdapter
WHERE
    MACAddress IS NOT NULL AND
    PNPDEVICEID IS NOT NULL");

ManagementObjectCollection mObject = searcher.Get();

foreach (ManagementObject obj in mObject)

Both variable names are not great. You should avoid naming something obj if it is not an Object. Also you have the variable mObject which is a ManagementObjectCollection but can easily be confused to be a ManagementObject. Despite not being a great alternative either, collection and item might be better...though, only slightly.

ManagementObjectCollection collection = searcher.Get();

foreach (ManagementObject item in collection)

string pnp = obj["PNPDeviceID"].ToString();

Again, I would have rather named this variable pnpId or deviceId.

string pnp = obj["PNPDeviceID"].ToString();
if (pnp.Contains("PCI\\"))
{
    string mac = obj["MACAddress"].ToString();
    mac = mac.Replace(":", string.Empty);
    return mac;
}

This whole block can be shortened without losing much readability:

    if (item["PNPDeviceID"].ToString().Contains("PCI\\"))
    {
        return item["MACAddress"].ToString().Replace(":", string.Empty);
    }

Though, it seems a little bit messy at first sight, I think it's within an acceptable level.

return "Nothing happened...";

A more appropriate return value would be "" or "No PCI device found.".

Of course, the most appropriate action would be to throw an exception, as that should not happen during normal operation.


Shell script that verifies checksums when moving between filesystems I coded this because it seemed like a fun project, would be awesome if someone reviewed this.

Thanks!

#! /bin/sh

#function that checks if dependencies are installed
check_software() {
    #check if md5sum is installed

    if [ $(command -v md5sum) > /dev/null 2>&1 ]; then
        echo 'md5sum'

    #check if md5 is installed
    elif [ $(command -v md5) > /dev/null 2>&1 ]; then
        echo 'md5'

    #if neither are installed, quit
    else
        echo "Neither md5sum nor md5 are installed. Quitting." 1>&2
        exit 1
    fi
}

#function to check if we're moving to same filesystem type
check_filesystem() {
    #df gives us detailed information about the file, use awk to get only the disk
    DISK1=$(df "$SOURCE" | tail -1 | head -1 | awk '{print $1}')

    #run dirname to remove the last bit of the path as the file does not exist yet
    DISK2=$(df $(dirname "$DESTINATION") | tail -1 | head -1 | awk '{print $1}')

    if [ "$DISK1" == "$DISK2" ]; then
        echo 1
    else
        echo 0
    fi
}

main() {
     CHECKSOFT=$(check_software)

     #if file is being moved on the same filesystem
     if [ "$(check_filesystem)" == "1" ]; then
        #move normally
        if [ "$FLAGS" != '' ]; then
            mv "-""$FLAGS" "$SOURCE" "$DESTINATION"
        else
            mv "$SOURCE" "$DESTINATION"
        fi
     else
        echo "Starting bettermv"
        #copy the file
        cp -p "$SOURCE" "$DESTINATION"

        #this bit of code extracts only the hashes from the output produced
        if [ "$CHECKSOFT" = "md5sum" ]; then
            CHECKSUMSOURCE="$($CHECKSOFT "$SOURCE" | awk '{print $1}')"
            CHECKSUMDEST="$($CHECKSOFT "$DESTINATION" | awk '{print $1}')"
        else
            CHECKSUMSOURCE="$($CHECKSOFT "$SOURCE" | awk '{print $4}')"
            CHECKSUMDEST="$($CHECKSOFT "$DEST" | awk '{print $4}')"
        fi

        #compare checksums and if they match up
        if [ "$CHECKSUMSOURCE" == "$CHECKSUMDEST" ]; then
            #remove original and exit
            rm "$SOURCE"
            echo "Move completed successfuly"
            return 0
        #if they don't match up
        else
            #display error message and quit
            echo "Checksums did not match, please try again" 1>&2
            rm "$DESTINATION"
            exit 1
        fi
    fi
}

FLAGS=''
#get all the move flags and store them in a variable
while getopts "finv" opt; do
    case $opt in
        f)
        FLAGS="$FLAGS""f"
        ;;
        i)
        FLAGS="$FLAGS""i"
        ;;
        n)
        FLAGS="$FLAGS""n"
        ;;
        v)
        FLAGS="$FLAGS""v"
        ;;
        \?)
        echo "Invalid argument $OPTARG"
        ;;
    esac
done

#shift optindex past the flags
shift $(( OPTIND-1 ))

#iterate through files to move and move them to the destination (last argument supplied)
for i in $@; do
    if [ $i != ${BASH_ARGV[0]} ]; then
        SOURCE=$i
        DESTINATION="${BASH_ARGV[0]}""$(basename $SOURCE)"
        main

    fi
done
exit 0

I'll start with your entry point and will go through you code step by step.
The last code block will contain an updated version.

The key elements in my changes are split your functions and only do one specific task, e.g. extra function for getting a checksum of a file and another one comparing two checksums etc. A function should also get all necessary information by PARAMETER not by an environment variable set outside. This simplifies testing a lot.

The getopts part is fine but you should just store the flag with the additional "-" as it will simplify things later in your code. You may also want to exit if an unknown parameter was provided. I would also rename FLAGS to MV_FLAGS as it describes better what the flags are for. You would normally also print a usage if an invalid parameter was provided. After parsing the options you should also check if enough parameters were provided and print a message otherwise but this is just a UX thing.

MV_FLAGS=''
#get all the move flags and store them in a variable
while getopts "finv" opt; do
    case $opt in
        f) MV_FLAGS="$MV_FLAGS"f ;;
        i) MV_FLAGS="$MV_FLAGS"i ;;
        n) MV_FLAGS="$MV_FLAGS"n ;;
        v) MV_FLAGS="$MV_FLAGS"v ;;
        ?)
        echo "Invalid argument $1" >&2
        exit 1
        ;;
    esac
done
[ -n "$MV_FLAGS" ] && MV_FLAGS="-$MV_FLAGS"

#shift optindex past the flags
shift $(( OPTIND-1 ))

if [ $# -lt 1 ] ; then
   echo "You must specify additional filenames" >&2
   exit 1
fi

For the next part you should definitely quote $@ as otherwise you won't handle parameters with spaces correctly. There is also no need to compare $i with ${BASH_ARGV[0]} as $@ contains the parameter starting at 1. Instead of specifying environment variables to call your main function you should specify them as a parameter instead. BASH_ARGV is according to man bash also only set if you have debugging enabled, so it is not portable. I also don't really understand what DESTINATION should be, but you should change it as well. You should also either quote your parameters with " or use ${ I wouldn't mix them and there is no need to use " as well as ${}. You should also check if the source file is a valid file before pushing calling your function. The function name could also be improved, e.g move_file

for i in "$@" ; do
    SOURCE="$i"
    DESTINATION="$0""$(basename "$SOURCE")"
    if [ ! -f "$SOURCE" ] ; then
       echo "$SOURCE does not exist, ignoring." >&2
       continue
    fi
     move_file "$SOURCE" "$DESTINATION"
done

In your main and my move_file function you always call check_software but there is no need for it. It is enough to call it once  at the beginning. You should also create a function handling verifying the filenames so your code doesn't get clustered with the different program parameters etc:

#function that checks if dependencies are installed
check_software() {
    for name in md5sum md5 openssl ; do
        if [ $(type "$name") > /dev/null 2>&1 ] ; then
           return 0
        fi
    done
    return 1
}

if ! check_software ; then
    echo "Neither md5sum, md5 nor openssl are installed. Quitting." >&2
    exit 1
fi

Your check_filesystem can also be simplified a lot if you use a device number instead of parsing df, I'd also rename it to same_filesystem

same_filesystem() {
    if [ $# -ne 2 ]  ; then
       echo "Usage: $0 source target" >&2
       return 1
    fi
    FS1=$(stat -c "%d" "$1")
    FS2=$(stat -c "%d" "$(dirname "$2")" )
    [ "$FS1" -eq "$FS2" ]
    return $?
}

You can also simplify your main/move_file function a bit, e.g:

move_file() {
     # we already checked if the necessary programs are available no
     # need to check again
     if [ $# -ne 2 ] ; then
         echo "Usage: $0 source target" >&2
         return 1
     fi

     SOURCE="$1"
     DESTINATION="$2"

     if [ -f "$DESTINATION" ] ; then
       # do something if file already exists?!
       :
     fi

     #if file is being moved on the same filesystem
     if same_filesystem "$SOURCE" "$DESTINATION" ; then
        mv $MV_FLAGS -- "$SOURCE" "$DESTINATION"
        return 0
     fi

     echo "Starting bettermv" >&2
     #copy the file
     cp -p "$SOURCE" "$DESTINATION"

     if verify_files "$SOURCE" "$DESTINATION" ; then
        rm "$SOURCE"
        return 0
    else
        echo "Error moving $SOURCE to $DESTINATION, abort." >&2
        rm "$DESTINATION"
        return 1
     fi
}

get_checksum() {
     if [ $# -ne 1 ] ; then
         echo "Usage: $0 source" >&2
         return 1
     fi
     if type openssl >/dev/null 2>&1 ; then
         echo $(openssl md5 "$1" | awk '{print $2}')
     elif type md5sum >/dev/null 2>&1 ; then
         echo $(md5sum "$1" | awk '{print $1}')
     elif type md5 >/dev/null 2>&1 ; then
         echo $(md5 "$1" | awk '{print $4}')
     fi
     return 1
}

verify_files() {
    if [ $# -ne 2 ] ; then
         echo "Usage: $0 source target" >&2
         return 1
     fi

    CHECKSUM1=$(get_checksum "$1")
    CHECKSUM2=$(get_checksum "$2")

    [ $CHECKSUM1 = $CHECKSUM2 ]
    return $?
}

The complete script with all changes (i tested it):

#!/bin/sh

#function that checks if dependencies are installed
check_software() {
  for name in md5sum md5 openssl ; do
  if type "$name" 1>/dev/null 2>&1 ; then
    return 0
  fi
  done
  return 1
}

if ! check_software ; then
  echo "Neither md5sum, md5 nor openssl are installed. Quitting." >&2
  exit 1
fi

move_file() {
  # we already checked if the necessary programs are available no
  # need to check again
  if [ $# -ne 2 ] ; then
    echo "Usage: $0 source target" >&2
    return 1
  fi

  SOURCE="$1"
  DESTINATION="$2"

  if [ -f "$DESTINATION" ] ; then
    # do something if file already exists?!
    :
  fi

  #if file is being moved on the same filesystem
  if same_filesystem "$SOURCE" "$DESTINATION" ; then
    mv $MV_FLAGS -- "$SOURCE" "$DESTINATION"
    return 0
  fi

  echo "Starting bettermv" >&2

  #copy the file
  cp -p "$SOURCE" "$DESTINATION"

  if verify_files "$SOURCE" "$DESTINATION" ; then
    rm "$SOURCE"
    return 0
  else
    echo "Error moving $SOURCE to $DESTINATION, abort." >&2
    rm "$DESTINATION"
    return 1
  fi
}

same_filesystem() {
  if [ $# -ne 2 ]  ; then
    echo "Usage: $0 source target" >&2
    return 1
  fi

  FS1=$(stat -c "%d" "$1")
  FS2=$(stat -c "%d" "$(dirname "$2")" )

  [ "$FS1" -eq "$FS2" ]
  return $?
}

get_checksum() {
  if [ $# -ne 1 ] ; then
    echo "Usage: $0 source" >&2
    return 1
  fi

  if type md5sum >/dev/null 2>&1 ; then
    echo $(md5sum -- "$1" | awk '{print $1}')
  elif type md5 >/dev/null 2>&1 ; then
    echo $(md5 "$1" | awk '{print $4}')
  elif type openssl >/dev/null 2>&1 ; then
    echo $(openssl md5 "$1" | awk '{print $2}')
  else
    return 1
  fi
}

verify_files() {
  if [ $# -ne 2 ] ; then
    echo "Usage: $0 source target" >&2
    return 1
  fi

  CHECKSUM1=$(get_checksum "$1")
  CHECKSUM2=$(get_checksum "$2")

  [ $CHECKSUM1 = $CHECKSUM2 ]
  return $?
}

MV_FLAGS=''
#get all the move flags and store them in a variable
while getopts "finv" opt; do
  case $opt in
  f) MV_FLAGS="$MV_FLAGS"f ;;
  i) MV_FLAGS="$MV_FLAGS"i ;;
  n) MV_FLAGS="$MV_FLAGS"n ;;
  v) MV_FLAGS="$MV_FLAGS"v ;;
  ?)
  echo "Invalid argument $1" >&2
  exit 1
  ;;
  esac
done

[ -n "$MV_FLAGS" ] && MV_FLAGS="-$MV_FLAGS"

#shift optindex past the flags
shift $(( OPTIND-1 ))

if [ $# -lt 1 ] ; then
  echo "You must specify additional filenames" >&2
  exit 1
fi

for i in "$@" ; do
  SOURCE="$i"
  DESTINATION="$0""$(basename "$SOURCE")"

  if [ ! -f "$SOURCE" ] ; then
    echo "$SOURCE does not exist, ignoring." >&2
    continue
  fi

  move_file "$SOURCE" "$DESTINATION"
done


Caesar-Cipher Implementation Background

I am a total beginner in Haskell, so after reading the Starting out-chapter of "Learn you a Haskell", I wanted to create my first program that actually does something.

I decided to do the famous Caesar-Cipher.

Code

import Data.Char

encryptChar :: Char -> Int -> Int
encryptChar char shift = if ord char > 64 && ord char < 91    --Only encrypt A...Z
                           then (if ord char + shift > 90     --"Z" with shift 3 becomes "C" and not "]"
                                  then ord char + shift - 26
                                  else ord char + shift)
                         else ord char

decryptChar :: Char -> Int -> Int
decryptChar char shift = if ord char > 64 && ord char < 91
                            then (if ord char - shift < 65
                                   then ord char - shift + 26
                                   else ord char - shift)
                         else ord char

encrypt :: String -> Int -> String
encrypt string shift = [chr (encryptChar (toUpper x) shift) | x <- string]    --"Loop" through string to encrypt char by char

decrypt :: String -> Int -> String
decrypt string shift = [chr (decryptChar (toUpper x) shift) | x <- string]

main = print(decrypt "KHOOR, ZRUOG!" 3)

(The code does work as intended.)

Question(s)

How can this code be improved in general?
Do I follow the style guide of Haskell (indentation, etc.)?
I have a background in imperative languages. Did I do something that is untypical for functional programming languages?

I would appreciate any suggestions.

I'd start by avoiding directly encoding ASCII character codes into the logic. For example, instead of:

if ord char > 64 && ord char < 91

I'd probably use:

if char >= 'A' && char <= 'Z'

I think this shows the intent enough more clearly to be worthwhile.
Given that you also do this a couple of different places, I'd probably write a small isUpper function to return a Bool indicating whether a character is an upper-case letter:

isUpper :: Char -> Bool
isUpper char = char >= 'A' && char <= 'Z'

Then the rest of the code can use that:

encryptChar char shift = if isUpper char     -- ...

decryptChar char shift = if isUpper char     -- ...

[Note: the standard library already has an isUpper, but it may not fit your needs, since it's Unicode-aware, and here you apparently only want to deal with English letters.]


Using C++ to check if string s is subsequence of string t The problem I want to solve is very simple:

  Given a string s and a string t, check if s is subsequence of t. there is only lower case English letters in both s and t. t is potentially a very long (length ~= 500,000) string, and s is a short string (<=100).

  Example 1:
  s = "abc", t = "ahbgdc"
  Return true.

  Example 2:
  s = "axc", t = "ahbgdc"
  Return false.

I only learnt C++ for about 30 hours, so I am not sure whether my below solution is correct C++ style or not, especially when to free the pointer allocated space. I assigned two char pointer to each string and I do know I need to free them somewhere else. Since the return value depends on pointer, I don't know where to free the pointer allocated memory, which should be a big deal.

What I thought about this question is that there must be two pointers, each pointing one string. For these two strings, using pointers to compare character one by one.

#include <iostream>
#include <string.h>

using namespace std;

class Solution {
public:
bool isSubsequence(string s, string t) {
    char *sPtr = new char[s.size() + 1];
    char *tPtr = new char[t.size() + 1];

    bool returnVal = false;

    strcpy(sPtr, s.c_str());
    strcpy(tPtr, t.c_str());

    while (*sPtr && *tPtr){
        if (*sPtr == *tPtr++) {
            sPtr++;
        }
    }

    return !*sPtr;

   }
};

int main() {

    string s = "abc";
    string t = "ahbgdc";

    Solution sol = Solution();
    cout << sol.isSubsequence(s, t)<<endl;

    return 0;
}

After I talked to one of classmates, he suggests that to include  and use c++ string class to do this question. Following is my solution followed his suggestion, but for my above solution, he just said "I don't know dude, I just use c++ string class, that's simple. Your solution is just weird to me but I don't know why I feel weird."

class Solution {
public:
    bool isSubsequence(string s, string t) {
    int sIndex = 0;
    int tIndex = 0;

    while (sIndex < s.size() && tIndex < t.size()) {
        if (s[sIndex] == t[tIndex++]) {
            sIndex++;
        }
    }

    return sIndex >= s.size();
    }

};

using namespace std is considered bad practice.  Also, in C++, you should #include <string> rather than #include <string.h>.

The Solution class is an unnecessary complication.  You would be better off with just an isSubsequence() function.

In your first function, returnVal is never used.  Compiling with warnings enabled should alert you to the dead code.

The memory management rule is simple: every new must be paired with exactly one delete, and every new something[] must be paired with exactly one delete[].  Since the first solution calls new[] twice with no delete[], you have a memory leak.  Your friend is right: you should take advantage of the string class, because the whole point of the string class is to take care of memory management for you.  The second solution is definitely better.

The function signature is not ideal.  Having string parameters will cause each string to be needlessly copied when the function is called.  You should write instead:

bool isSubsequence(const std::string &s, const std::string &t) {
    …
}

std::string::size() returns std::string::size_type, so strictly speaking, you should use std::string::size_type instead of int.  (Or, if using c++11, just use auto.)


Opening a list of URLs and splitting the queries into different files I recently made a program in Python to open a list of URLs and split the queries into different files. I want to make this code more generic and simple. I am open to suggestions.

def parse_file():
    # Open the file for reading
    infile = open("URLlist.txt", 'r')
    # Read every single line of the file into an array of lines
    lines = infile.readlines()
    # For every line in the array of lines, do something with that line
    for line in lines: # for every line in lines.
        print line # print the URL
        # parse the URL query string list
        # if the URL has a query component print out the the first
        # argument(k) and if the URL has multiple arguments, print out
        # the other arguments. (v)
        for k,v in urlparse.parse_qsl(urlparse.urlparse(line).query):
            print k, v

            if k == "blog": # if the printed out arguements have blog in it
                           # write it out to the apropriate file and do this
                           # to all the others.
                with open('blog_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'p':
                with open('p_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'attachment_id':
                with open('attachment_id_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'lang':
                with open('lang_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'portfolio':
                with open('portfolio_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'page_id':
                with open('page_id_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'comments_popup':
                with open('comments_popup_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'iframe':
                with open('iframe_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'width':
                with open('width_file.txt'.format(),'a') as f:
                    f.write(v)
            elif k == 'height':
                with open('height_file.txt'.format(),'a') as f:
                    f.write(v)
                print "--------------------"
    infile.close()
    f.close()
parse_file()

Before We Get Started

It would be good if you could provide some examples of the URLs that you are looking to parse, I did a cursory search and I couldn't coerce a query result from any that I tried with:

urlparse.parse_qsl(urlparse.urlparse("some_url").query

In the interest of brevity I'll assume you have some set of URLs that this is working for.

Code Review

Step one in making your code more generic will be to remove the hard-coded URL text file from your function body. I would suggest it be a parameter to the parse_file function.

Also, You don't need to do this:

lines = infile.readlines()
for line in lines:

.readlines() isn't necessary in how it is being used here and the file can be read without it. You obviously know about the With Statement and how to use it as a context manager - but you're still opening and closing your URL list file manually!

infile = open("URLlist.txt", 'r')
...
infile.close()

Instead, you can alias the opened file in your With Statement and it will be iterable without .readlines():

with open(input_file) as input:
    for line in input:

Also, if you find yourself using multiple If/Elif clauses you might consider it time to look into some of Python's more interesting data structures, below I'm using a dictionary.
You can replace your predicate:

if k == 'blog':

and instead use a key in a dictionary with the value corresponding to the the subsequent procedure.

{'blog' : "blog_file.txt"}

Revision

Below is a possible revision to your code:

import urlparse
def parse_file(input_file):
    tags = {'blog' : "blog_file.txt",
            'p' : "p_file.txt",
            'attachment_id' : "attachment_id_file.txt",
            'lang' : "lang_file.txt",
            'portfolio' : "portfolio_file.txt",
            'page_id' : "page_id_file.txt",
            'comments_popup' : "comments_popup_file.txt",
            'iframe' : "iframe_file.txt",
            'width' : "width_file.txt",
            'height' : "height_file.txt"}
    with open(input_file) as input:
        for line in input:
            parsed_url = urlparse.parse_qsl(urlparse.urlparse(line).query)
            if len(parsed_url) > 0:
                for key, value in parsed_url:
                    if key in tags:
                        with open(tags[key], 'a') as output_file:
                            output_file.write(value)
                    else:
                        print key + " not in tags."
            else:
                print(line + " does not yield query.")

I think this should work as you describe, but without a set of example URLs it's difficult to say for sure. The line:

if len(parsed_url) > 0:

is used here because I couldn't coerce .query to return anything but an empty list on those URLs I attempted.

Final Thought

It might be a good idea to change the functionality for those keys not found in your tags dictionary. You might consider creating a miscellaneous, or catch-all file that unmatched values get written to. That way you don't totally lose work on those unmatched URLs.

This is all based on the assumption that you have some domain that your code is currently working for.


IEnumerable Extensions Linq AllOrDefault() Each() I recently decided to try and write my own implementation of Linq, which then lead me on to trying to solve some of the problems we have in our code base at work. Our code is littered with the following (obviously a simplified example):

        var items = new List<string> { "apple", "dog", "chair" };

        if (items != null && items.Any())
        {
            foreach (var item in items)
            {
                Console.WriteLine(item);
            }
        }

Essentially each time we are iterating over a collection we check to make sure the collection is not null and it actually contains at least one item.

I decided to write the following extension methods to alleviate this mess (the ML suffix on the methods was to differentiate from Linq [mostly for testing]):

        public static class EnumerableExtensions
        {
            public static IEnumerable<T> AllOrDefaultML<T>(this IEnumerable<T> items)
            {
                if (items.IsNullML()) return new List<T>();

                return items;
            }

            public static IEnumerable<T> AllOrDefaultML<T>(this IEnumerable<T> items, Func<T, bool> predicate)
            {
                items = items.AllOrDefaultML();

                if (!items.AnyML()) return new List<T>();

                return items.Where(predicate);
            }

            public static IEnumerable<T> EachML<T>(this IEnumerable<T> items, Action<T> fn)
            {
                var e = items.GetEnumerator();
                while (e.MoveNext())
                {
                    fn(e.Current);
                }
                return items;
            }

            public static bool IsNullML(this object item)
            {
                return item == null;
            }

            public static bool IsNullOrEmptyML<T>(this IEnumerable<T> items)
            {
                return !items.AnyML();
            }

            public static bool AnyML<T>(this IEnumerable<T> items)
            {
                if (items.IsNullML()) return false;

                return items.Count() > 0;
            }

            public static bool AnyML<T>(this IEnumerable<T> items, Func<T, bool> predicate)
            {
                if (items == null) return false;

                items = items.Where(predicate);

                return items.Count() > 0;
            }
        }

Essentially with these methods I can write the above code as the following:

        var items = new List<string> { "apple", "dog", "chair" };

        foreach (var item in items.AllOrDefaultML())
        {
            Console.WriteLine(item);
        }

Or even integrate the Each() method:

        var items = new List<string> { "apple", "dog", "chair" };

        items.AllOrDefaultML().EachML(i => Console.WriteLine(i));

I'm not sure how I feel about the above, but you get the idea.

I've also added a predicate to AllOrDefaultML() which works if the sequence is null:

        foreach (var item in items.AllOrDefaultML(i => i.Length == 5))
        {
            Console.WriteLine(item);
        }

Linq has a FirstOrDefault() method, why not have an AllOrDefault() which returns you an empty sequence if it's null and doesn't have any items?

I'm hoping to get some insight in to the question above, and also wondering if anyone else has implemented anything like this before? Has it cleaned up your code much? Is it easier to read? Is it confusing for colleagues (particularly new start) who aren't totally aware of what's happening? Is there anything fundamentally wrong with what I have here?

In those places you use new List<T>(), I would recommend using Enumerable.Empty<T>() in its place. It's a little more idiomatic and, as those empty collections are cached by the runtime, repeated calls don't new up more stuff to be GC'd later.

items.Count() > 0 can be replaced with items.Any(), which is more descriptive of what you're trying to test for, and will likely be cheaper computationally. More specifically,

public static bool AnyML<T>(this IEnumerable<T> items, Func<T, bool> predicate)
{
    if (items == null) return false;

    items = items.Where(predicate);

    return items.Count() > 0;
}

can be replaced with

public static bool AnyML<T>(this IEnumerable<T> items, Func<T, bool> predicate)
{
    if (items == null) return false;

    return items.Any(predicate);
}

** EDITED TO ADD **

public static IEnumerable<T> EachML<T>(this IEnumerable<T> items, Action<T> fn)
{
    var e = items.GetEnumerator();
    while (e.MoveNext())
    {
        fn(e.Current);
    }
    return items;
}

has a small bug - items.GetEnumerator() returns an IEnumerator<T>, which implements IDisposable. This means you should Dispose() of it when you're done using it (see https://stackoverflow.com/a/232616/3312 for good info). Write it like this to do so:

    using (var e = items.GetEnumerator())
    {
        while (e.MoveNext())
        {
            fn(e.Current);
        }
    }
    return items;
}


Define if post extract from a bilingual Facebook page are in English using Python I am currently extracting post on a biligual page on Facebook. Therefore, I have the problem of splitting the post in French and english before starting analysing them. I have construct a function which permit me to filter the sentence in english from all my sentence.

However it is quite complex and not accurate..

### import enchant
import nltk
### nltk.download()
### from nltk.corpus import words
from nltk.corpus import brown
import pandas as pd
import numpy as np
import re
  ### pop is an example of posts...

pop = ['hi','say','bonjour mister','sauf','tu am      belle','I suis cool','Worst trip ever je deteste AirTransat! ce sont les pires']

### Defining my function

def Extract_sentence_english(list_of_sentence):

    ### Removing punctuation, numeric and whitespace at the same time
    list_of_sentence = [re.sub(r'[^A-Za-z]+', ' ', x) for x in list_of_sentence]
    ### To lower
    list_of_sentence = map(lambda x: x.lower(), list_of_sentence)

    ### Split the list into words (splitting on space)
    split_list = [y for x in list_of_sentence for y in x.split(' ')]
    ### Return a logical value (True if English, False if other)
    log = []
    for i in range(0,len(split_list)):
        k = split_list[i] in words.words()
        log.append(k)
    ### log contain the logical value for the words
    ### Number of words per sentence
    df=[]
    for entry in list_of_sentence:
        df.append(len(entry.split()))

    df = pd.DataFrame(df)
    df.columns = ['number_of_words']

    df['index'] = range(0,len(df))

    dat = pd.DataFrame(df.loc[np.repeat(df.index.values,df['number_of_words'])])
    dat['logical'] = log
    dat['log_num'] = dat['logical'].astype(int)

    df['English_words_freq'] = dat.groupby(['index'], as_index = False).sum()['log_num']/df['number_of_words']
    final = pd.DataFrame(list_of_sentence)
    final['logical'] = df['English_words_freq'] > 0.75
    return(final)

Extract_sentence_english(pop)

Does somebody know any better way of doing this ?

Here are some comments on your code. First some stylistic comments, and afterwards some related to the logic of your code.

Method names should follow snake_case – According to the PEP8 standard, your method should be named extract_sentence_english, and slightly better worded, extract_english_sentences
Naming is hard – Most of the names seems to be slightly random. Why log, split_list, df, dat... They are not conveying anything sensible, or indicate how you've processed the original sentences.
Add vertical space – You have some, but could have more vertical space.
Better comments – For example, related to the split_list (renamed to all_words and log (renamed to is_english_words), I would rather write something like this:

# Make a list of all single words based upon the sentences
all_words = [word for sentence in list_of_sentence
                     for word in sentence.split(' ')]

# Make a corresponding list with True if the word is English,
# or False if not
is_english_words = [word in words.words() for word in all_words]

Now the comments explain the contents of the list, and what is happening on the next code lines.  I've also used a list comprehension to create the alternative to log, and with better variable names it is easier to see, and should not as expensive as the original for loop. By the way, range defaults to starting at 0 so it could simply have been range(len(split_list)).

Same kind of logic (and list comprehension) could apply to the creation of df, which could be renamed to sentence_lengths.
Why all the DataFrame mumble jumble? – I don't really see the need for all the usage of DataFrame (and don't understand what is happening besides the reconnection of words and sentences). More on this in alternate solution below.
Please add space after commas, , – In some of the parameter listings you don't add space after the comma, which makes the code harder to read. Be consistent, and always add space after comma.

Alternative logic

If I've understood your request properly you are getting a whole lot of sentences, and want to return all the English sentences (and loose the French sentences?). You go about this task by removing everything which isn't a-z, splitting into words and checking against a dictionary, and if the English word frequency is above 0.75 you'll return that sentence (without punctuation).

Doing it that way eliminates on a good indicator of French sentences. French sentences uses loads of accented characters. English doesn't. That is a very good indicator (if the Facebook guys care to write those characters), which you are missing out on.

Your solution also checks against an entire dictionary, which is kind of expensive. A simpler solution would be to keep two smaller list of typical English words and French words, and make sure to remove non-French or non-English words from these lists. I know very little French, but here is an attempt at some trigger word lists:

english_words = ["you", "are", "not", "the", "is", "what", "who", "that"]
french_words = ["le", "ce", "un", "la", "et", "au", "à", "toute"]

The idea is to have enough specific typical words in either list, so that it eases detection of the language.

So how could this look in an alternate version? Well, here is some code for you to work with:

import re
import random

sample_sentences = ['hi there, how are you mister', 'some none triggering english sentence', 'And another long english statement', 'This is not plain English', 'say','bonjour mister','sauf','tu am      belle','I suis cool', 'Hey YOU', u'Worst trip.ever je, à deteste AirTransat! ce sont les pires', u"Toute personne a droit à l´éducation"]

ENGLISH_WORDS = ["you", "are", "not", "the", "is", "what", "who", "that"]
FRENCH_WORDS = ["le", "ce", "un", "la", "et", "au", u"à", u"é", "toute"]

REQUIRED_NUMBER_OF_WORDS = 1
REQUIRED_ENGLISH_PERCENTAGE = 0.75

def extract_english_sentences(sentences):

  # Compile a pattern of all non-alphanumerics in Unicode
  pattern = re.compile('\W+', re.UNICODE)

  result = []
  print sentences

  # Loop on each sentence, and determine if it English or not
  for sentence in sentences:
    # Get a simplified sentence with only unicode alphanumerics,
    # which are lowercased
    no_punctuation = pattern.sub(' ', sentence).lower()

    # Split into single words
    words_in_sentence = no_punctuation.split()
    number_words = len(words_in_sentence)

    print ("\n>{}<".format(no_punctuation.encode('utf8', 'replace')))

    is_english = None

    # Check whether word is in either trigger list
    for word in words_in_sentence:
      if word in FRENCH_WORDS:
        print "in french words"
        is_english = False
        break

      if word in ENGLISH_WORDS:
        print "in english words"
        is_english = True
        break

    if is_english is not None and is_english:
      result.append(sentence)
      continue

    # If it didn't match anything in trigger list,
    # and we have more than 5 words, lets check the
    # percentage of English words
    if is_english is None and number_words > REQUIRED_NUMBER_OF_WORDS:
      # Count occurences of each word in the dictionary
      # give by words.words()
      #sum_english_words = sum(word in words.words()
      #                          for word in words_in_sentence)

      # Don't have dictionary, guess... :-)
      sum_english_words = random.randint(1, number_words)
      print "{} vs {}".format(sum_english_words, number_words)

      if sum_english_words / number_words > REQUIRED_ENGLISH_PERCENTAGE:
        print "Dictionary says English"
        result.append(sentence)
        continue

    if is_english is not None and not is_english:
      print "French!"
    else:
      print "Couldn't decide"

  return result

print extract_english_sentences(sample_sentences)

As I didn't have any words.words() available, I've commented out that segment, and use a random generator instead. This in combination a lot of print statements should show that in most cases it would continue with next sentence in a lot of the cases, and hopefully not fall through to the dictionary test or "Couldn't decide" very often.

When deployed to production all of the print statements should be removed. Do also note that this code is written in Python 2.7, so no parentheses are needed for the print statements, and some strings needs to be declared as unicode strings. But you didn't state which python version you are using, so I made this in the older variant. Hope this helps you along the way.

Alternate trigger word list tests

At the cost of looping twice through the words_in_sentence list, one could remove the inner for loop, and all other code related to is_english:

# Check whether word is in either trigger list
if any(word in FRENCH_WORDS for word in words_in_sentence):
  print "in french words"
  continue

if any(word in ENGLISH_WORDS for word in words_in_sentence):
  print "in english words"
  result.append(sentence)

Possible major speedup

I just realized that you could get a major speedup related to the dictionary lookup. It is very expensive to get the complete word list, words.words(), for every sentence. This should be done once, and created into a set, which then is used to check for existence within dictionary.

It's late in the night locally for me, so the following code is untested:

WORDS_SET = set(words.words())

...

sum_english_words = sum(word in WORDS_SET)

This should avoid the repeated loading of the words for every word of every sentence... Will try to test this tomorrow.


Checking for existing key in multi-dimensional array Say I have a data structure like this:

array(3) {
  ["id"]=>
  int(1)
  ["name"]=>
  string(3) "foo"
  ["message"]=>
  array(1) {
    ["action"]=>
    string(3) "PUT"
  }
}

Where 'message' and 'action' are both optional.  To check if they're present my first attempt would be to write something like this:

if (array_key_exists('message', $array) && array_key_exists('action', $array['message'])){
}

Is there a cleaner implementation?

You can just do this!

if (isset($array["message"]["action"])) { /*...*/ }

Proof:

<?php

error_reporting(E_ALL);
ini_set('error_reporting', E_ALL);
ini_set('display_errors', 1);

$array = [
    "id" => 1,
    "name" => "foo",
    "message" => [
        "action" => "PUT",
    ],
];

var_dump(isset($array["message"]["action"])); // true

$array = [
    "id" => 1,
    "name" => "foo"
];

var_dump(isset($array["message"]["action"])); // false

No log entries.

Do note that if your value is null, then isset will also return false. You probably shouldn't be creating values with null.


Move a folder to another in C# In C# I wrote a method to move a folder into another existing folder:

public void MoveFolder(string folderToMove, string destination)
{
    String destinationFolder = Path.Combine(
        destination, Path.GetFileName(folderToMove));

    Directory.Move(folderToMove, destinationFolder);
}

Usage:

MoveFolder(@"D:\myfolder\mysubfolder", @"C:\");

It works, but I am sure there is a more readable and shorter way to do that? In particular, I feel bad using GetFileName.

No tricky things to worry about: The folders exist, and destination is not inside folderToMove nor the same.
It must work on .NET 4.0 without additional libraries.

This method is public but you don't do any validation about the passed in parameters which is a bad idea because your code is giving implementationdetails to a user of that method which he/she doesn't need to know. Assume folderToMove == null  then PathCombine() will throw an ArgumentNullException which is the correct exception, but this is telling a user that you are using System.Path.Combine() which he/she doesn't need to know.

You should better do the validation yourself and throw the expected exception.

Instead of Path.GetFileName() you should use a DirectoryInfo object and read its Name property which you could extract to a separate method to construct the destination foldername like so

    public void MoveFolder(string folderToMove, string destination)
    {
        if (folderToMove == null)
        {
            throw new ArgumentNullException("folderToMove");
        }
        if (destination == null)
        {
            throw new ArgumentNullException("destination");
        }
        if (string.IsNullOrEmpty(folderToMove))
        {
            throw new ArgumentException("The parameter may not be empty", "folderToMove");
        }
        if (string.IsNullOrEmpty(destination))
        {
            throw new ArgumentException("The parameter may not be empty", "destination");
        }

        String destinationFolder = CreateDestinationFolderName(folderToMove, destination);
        Directory.Move(folderToMove, destinationFolder);
    }
    private string CreateDestinationFolderName(string folderToMove, string destination)
    {
        var directoryInfo = new DirectoryInfo(folderToMove);

        return Path.Combine(destination, directoryInfo.Name);
    }


Repeating code fragments in XML editor I'm trying to make an XML editor. So far, I got the results I wanted, but a lot of code is repeated. How can I reuse more effectively?

import java.io.File;
import java.io.IOException;

import javafx.collections.FXCollections;
import javafx.collections.ObservableList;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.transform.OutputKeys;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerException;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.stream.StreamResult;

import org.w3c.dom.Document;
import org.w3c.dom.Element;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;
import org.xml.sax.SAXException;

public class XmlUtil {
    private static File fXmlFile;
    private static DocumentBuilderFactory dbFactory = DocumentBuilderFactory
            .newInstance();
    private static DocumentBuilder dBuilder;
    private static Document doc;

    public static ObservableList<String> getAllTagText(String fileName)
            throws ParserConfigurationException, SAXException, IOException {
        ObservableList<String> tagList = FXCollections.observableArrayList();

        dBuilder = dbFactory.newDocumentBuilder();
        fXmlFile = new File(fileName);
        doc = dBuilder.parse(fXmlFile);

        doc.getDocumentElement().normalize();
        NodeList nodeList = doc.getElementsByTagName("Tag");

        for (int i = 0; i < nodeList.getLength(); i++) {
            String tag = nodeList.item(i).getTextContent();
            tagList.add(tag);
        }

        return tagList;
    }

    public static String getLanguageText(String fileName, String tag,
            String language) throws ParserConfigurationException, SAXException,
            IOException {
        dBuilder = dbFactory.newDocumentBuilder();
        fXmlFile = new File(fileName);
        doc = dBuilder.parse(fXmlFile);

        doc.getDocumentElement().normalize();
        NodeList nodeList = doc.getElementsByTagName("TEXT");

        for (int i = 0; i < nodeList.getLength(); i++) {
            Node node = nodeList.item(i);
            if (node.getNodeType() == Node.ELEMENT_NODE) {
                Element element = (Element) node;
                if (element.getElementsByTagName("Tag").item(0)
                        .getTextContent().equals(tag)) {
                    return element.getElementsByTagName(language).item(0)
                            .getTextContent();

                }
            }

        }
        return null;
    }

    public static void setText(String fileName, String tag, String textContent)
            throws SAXException, IOException, ParserConfigurationException,
            TransformerException {
        dBuilder = dbFactory.newDocumentBuilder();
        fXmlFile = new File(fileName);
        doc = dBuilder.parse(fXmlFile);

        doc.getDocumentElement().normalize();
        NodeList nodeList = doc.getElementsByTagName("TEXT");

        for (int i = 0; i < nodeList.getLength(); i++) {
            Node node = nodeList.item(i);
            if (node.getNodeType() == Node.ELEMENT_NODE) {
                Element element = (Element) node;
                if (element.getElementsByTagName("Tag").item(0)
                        .getTextContent().equals(tag)) {
                    element.getElementsByTagName("Hungarian").item(0)
                            .setTextContent(textContent);
                }

            }
            TransformerFactory transformerFactory = TransformerFactory
                    .newInstance();

            Transformer transformer = transformerFactory.newTransformer();
            transformer.setOutputProperty(OutputKeys.ENCODING, "ISO-8859-1");
            DOMSource domSource = new DOMSource(doc);

            StreamResult streamResult = new StreamResult(fXmlFile);
            transformer.transform(domSource, streamResult);
        }

    }
}

XPath does away with much of the very low-level w3c classes. And is within java SE: javax.xml.xpath.XPath.


Students with second lowest grade I solved the Nested List Python challenge on HackerRank.com passing all test cases. Although this problem can likely be solved with a short script with clever one-liners, my approach was instead to solve it in a clean, understandable way in functional style.

Here is a summary of the challenge, see link above for full description:

  Print the name(s) of any student(s) having the second lowest grade; if there are multiple students, order their names alphabetically and print each one on a new line.

  The first line contains an integer, \$N\$, the number of students.
  The \$2N\$ subsequent lines describe each student over \$2\$ lines; the first line contains a student's name, and the second line contains their grade.

I haven't used nested lists much until now and I found myself having to search the internet a good bit for parts of the functions (in particular the get_lowest_grade function) and I would like to improve this code as much as possible.

def get_students(num_students: int) -> list:
    """Returns a list of names and grades of N students read from stdin
    where each name and respective grade is separated by a new line."""
    students = []
    for s in range(0, num_students):
        name = input()
        grade = float(input())
        students.append([name, grade])
    return students

def get_lowest_grade(students: list) -> float:
    """Returns the lowest grade from a list of students[name, grade]."""
    lowest_grade_student = min(students, key = lambda x: x[1])
    return lowest_grade_student[1]

def get_lowest_grade_students(students: list) -> list:
    """Returns the students with the lowest grade
    from a list of students[name, grade]."""
    return [s for s in students if s[1] == get_lowest_grade(students)]

def exclude_lowest_grade_students(students: list) -> list:
    """Returns a list of students with the lowest graded students excluded
    from a list of students[name, grade]."""
    return [s for s in students if s[1] != get_lowest_grade(students)]

def get_student_names_sorted_alpha(students: list) -> list:
    """Returns a list of names sorted alphabetically from a list of students[name, grade]"""
    names = [s[0] for s in students]
    return sorted(names)

def main():
    num_students = int(input())
    students = get_students(num_students)
    lowest_excluded = exclude_lowest_grade_students(students)
    second_lowest = get_lowest_grade_students(lowest_excluded)
    for name in get_student_names_sorted_alpha(second_lowest):
        print(name)

if __name__ == '__main__':
    main()

Your code looks nice and is well documented.

Many details can be improved though.

num_students = int(input()) could be moved in get_students so that your code deals with input in a single place.
even though the problem talks about lists, it may be more idiomatic to use a list of tuples instead of a list of list. I'd highly recommend Ned Batchelder's excellent article "Lists vs. Tuples" to know more about this.
0 as a first argument of range is not needed as it is the default value. So you could rewrite: for s in range(0, num_students): as for s in range(num_students):.
Underscore (_) is pretty conventionnal for variable with an used value so you could write: for _ in range(num_students):.
Adding a simple print statement in get_lowest_grade shows that the function is called more often that it needs. This is a real issue when you have many students because your algorithm with go through all student for all students. Your algorithm is said to be quadratic or O(n^2). You could retrieve the lowest grade once and for all .

Your code would look like this:

def get_students() -> list:
    """Returns a list of names and grades of N students read from stdin
    where each name and respective grade is separated by a new line."""
    return [('Harry', 37.21), ('Berry', 37.21), ('Tina', 37.2), ('Akriti', 41), ('Harsh', 39),
            ('Harry', 37.21), ('Berry', 37.21), ('Tina', 37.2), ('Akriti', 41), ('Harsh', 39),
            ('Harry', 37.21), ('Berry', 37.21), ('Tina', 37.2), ('Akriti', 41), ('Harsh', 39)]
    num_students = int(input())
    students = []
    for s in range(num_students):
        name = input()
        grade = float(input())
        students.append((name, grade))
    return students

def get_lowest_grade(students: list) -> float:
    """Returns the lowest grade from a list of students[name, grade]."""
    lowest_grade_student = min(students, key = lambda x: x[1])
    return lowest_grade_student[1]

def get_students_with_grade(students: list, grade: float) -> list:
    """Returns the students with the lowest grade
    from a list of students(name, grade)."""
    return [s for s in students if s[1] == grade]

def get_students_without_grade(students: list, grade: float) -> list:
    """Returns a list of students with the lowest graded students excluded
    from a list of students(name, grade)."""
    return [s for s in students if s[1] != grade]

def get_student_names_sorted_alpha(students: list) -> list:
    """Returns a list of names sorted alphabetically from a list of students[name, grade]"""
    names = [s[0] for s in students]
    return sorted(names)

def main():
    students = get_students()
    lowest_grade = get_lowest_grade(students)
    students2 = get_students_without_grade(students, lowest_grade)
    lowest_grade2 = get_lowest_grade(students2)
    second_lowest = get_students_with_grade(students2, lowest_grade2)
    for name in get_student_names_sorted_alpha(second_lowest):
        print(name)

if __name__ == '__main__':
    main()

A different algorithm could be written using a different data structure like a dictionnary or a counter.


Optimizing code for Project-Euler Problem #23 I'm working on Project Euler's problem #23, which is

  Find the sum of all the positive integers which cannot be written as the sum of two abundant numbers

I came up with this algorithm. Find all abundant numbers under 28123 (defined Numeric#abundant?), this is slow and could be faster if skipped primes, but is fairly fast (less than 4 secs):

abundant_numbers = (12..28123).select(&:abundant?)

Find all numbers that can be expressed as the sum of 2 perfect numbers:

inverse_set = abundant_numbers.each.with_index.inject([]) do |a,(n,index)|
  a.concat(
    abundant_numbers[index..abundant_numbers.size-1]
    .take_while { |i| (n+i) <= 28123 }.map { |i| n+i }
  )
end.to_set

The rest them from all the integers under 28123 and sum them all:

solution_set = (1..28123).set - inverse_set
solution_set.reduce(:+)

Benchmarked:

▸ time ruby 0023.rb
real  0m20.036s
user  0m19.593s
 sys  0m0.352s
▸ rvm use 2.0.0
▸ time ruby 0023.rb
Solution: 4*****1

real  0m7.478s
user  0m7.348s
 sys  0m0.108s

It works, but it's a little bit slow, takes about 20secs to solve, and I hear people around saying it can be solved within miliseconds. I'm sure many of you will have a quick insight on what have I missed.

Your idea is perfectly fine (subtracting all sum of pairs from the candidates range), but I would write it differently:

xs = (1..28123)
abundants = xs.select(&:abundant?)
solution = (xs.to_set - abundants.repeated_combination(2).to_set { |x, y| x + y }).sum

With a similar idea, this is probably faster (but also a bit less declarative):

xs = (1..28123)
abundants = xs.select(&:abundant?).to_set
solution = xs.select { |x| abundants.none? { |a| abundants.include?(x - a) } }.sum


A menu option that receives input from users and then performs actions Please review my FindMovie and AddMovie methods.

public static boolean FindMovie(String movietofind, String[] ArrayMovie) {
    movie Movieobject = new movie();

    for (String ArrayMovie1 : ArrayMovie) {
        if (ArrayMovie1.equals(movietofind)) {
            return true;
        }
    }
    return false;
}

public static void AddMovie(int i, String[] ArrayMovie, String[] UserRating, int[] UserReview) {
    int addmovie, userrating;
    boolean Movie1;
    String MPAA;
    Scanner console = new Scanner(System.in);

    System.out.println("Please give me the name of the movie you want to add.");
    String Movie = console.next();

    Movie1 = FindMovie(Movie, ArrayMovie);
    if (Movie1 = false) {
        System.out.println("Do you want to add this movie to the collection? Please select 1 for Yes or 2 for No");
        addmovie = console.nextInt();
        if (addmovie == 1) {
            ArrayMovie[i] = Movie;

            System.out.println("What is the type of MPAA rating of the movie?");
            System.out.println("Please enter G, PG, PG13, or R");
            MPAA = console.next();
            UserRating[i] = MPAA;

            System.out.println("What is your rating for movie?");
            System.out.print("please choose 1, 2, 3, 4, or 5");
            userrating = console.nextInt();
            UserReview[i] = userrating;

        } else if (addmovie == 2) {
        }
    }
}

Technical

Your naming conventions are inconsistent - you mix PascalCase with alllowercase (don't know if that convention has it's own name). In Java types (like classes and interfaces) are PascalCase and method and variable names are camelCase.
Declare variables where they are used and not all at the top of the function.
Movie1 one is not a good name for a variable. The variable name should express the meaning of it. In this case it should be movieExists.
FindMovie won't compile due to the first line in the function (there is no movie type in your source code and nothing actually references a movie type except that one line of code). It should be removed (Movieobject is never used)
There is no need to prefix the variable name with the type of the variable it just adds clutter and no real value. So ArrayMovie should be movies as that's what the collection represents (a collection of movies).
If you use ArrayList instead of arrays you get automatic resizing when adding new movies and a whole bunch of nice utility functions like contains which reduce a lot of code clutter and does not impose the limitation of fixed sized arrays.
AddMovie requires the caller to pass in the index in the array where the movie should be stored. This is bad as it requires the caller to keep information about the structure of the list. It should just add a new movie to the end of a list (see previous point about ArrayList).
You can reduce nesting by breaking early when hitting the negative condition, for example:

bool movieExists = FindMovie(Movie, ArrayMovie);
if (movieExists)
{
    return;
}

System.out.println("Do you want to add this movie to the collection? Please select 1 for Yes or 2 for No");
bool userAction = console.nextInt();
if (userAction != 1)
{
    return;
}

// add the movie

Design

It seems like you had the right idea at some point about having a movie class (although it should be named Movie) encapsulating the properties of a movie like name, rating and reviews. Right now you seem to pass them around as 3 independent lists and the only connection is that the index in each list refers to the same movie. This is easy to get out of sync and you always have to pass around 3 lists - not to mention what happens when you want to add a 4th or 5th property.

AddMovie should just accept an ArrayList<Movie> as input to which you can add a new movie after you have gathered all the user input.