jQuery expansion menu I have a menu done in jQuery that I'd like some feedback on:

$(function() {
  // main expansion element
  $(".expander").click(function() {
    var subShown = $("ul > li", this).hasClass("show");
    if (!subShown) {
      $(".indented", this).slideDown('100').addClass("show");
      $(".caret", this).addClass("reversedCaret");
    } else {
      $(".indented", this).slideUp('100').removeClass("show");
      $(".caret", this).removeClass("reversedCaret");
      $(".indented--sub").slideUp('100').removeClass("show");
      $(".sub-caret").removeClass("reversedCaret");
      $(".more-or-less").text("More");
    }
  });

  // sub expansion element
  $(".sub-expander").click(function() {
    var subSelectText = $(".more-or-less").text();
    if (subSelectText != "More") {
      $(".indented--sub").slideUp('100').removeClass("show");
      $(".sub-caret").removeClass("reversedCaret");
      $(".more-or-less").text("More");
    } else {
      $(".indented--sub").slideDown('100').addClass("show");
      $(".sub-caret").addClass("reversedCaret");
      $(".more-or-less").text("Show Less");
    }
  });

  // stop propagation on the link element within .expander class
  $(".indented").click(function(event) {
    event.stopPropagation();
  });
});
.expander:hover {
  cursor: pointer;
}

.sub-expander--indented {
  padding: 0 0 0 23px;
}

.sub-caret {
  margin-right: 75px;
}

.indented,
.indented--sub {
  display: none;
}

.show {
  display: block;
}

  +

Item 1

Item 2

Item 3

Chapter 5

Chapter 6

Chapter 7

        +

More

This just seems like a really over the top way to do it, but the one thing I'm having a hard time with is differentiating the sub menu from the main expander without it's own class/ID.

Do you have any tips on how to improve this?

Feedback
The code makes good use of the jQuery slide functions, as well as basic CSS class addition/removal.
One could utilize event delegation to use a single event listener on a parent container element, along with .is() and tree traversal methods like .parent().
Tips to improve this
Utilize .toggle() and .toggleClass(), along with dynamic function call using a variable (e.g. $(".indented", this)[slideFunction]('100') where slidefunction is a variable containing the function to call) to condense the code.
Also, as mentioned above, use event delegation to register a single click handler. Then check if the target is a child of various elements (e.g. with class indented, with class sub-expander, etc.), and handle actions accordingly.

$(function() {
  // main expansion element
  $(".expander").click(function(event) {
    var target = $(event.target);
    // stop propagation on the link element within .expander class
    if (!target.parent('.indented').length) {
      // sub expansion element
      if (target.parent('.sub-expander').length) {
        var subSelectTextIsMore = $(".more-or-less").text() === "More";
        var slideFunction = subSelectTextIsMore ? 'slideDown' : 'slideUp';

        $(".indented--sub")[slideFunction]('100').toggleClass("show", subSelectTextIsMore);
        $(".sub-caret").toggleClass("reversedCaret", subSelectTextIsMore);
        $(".more-or-less").text(subSelectTextIsMore ? "Show Less" : "More");
      } else { //expander
        var subShown = $("ul > li", this).hasClass("show");
        var slideFunction = subShown ? 'slideUp' : 'slideDown';
        $(".indented", this)[slideFunction]('100').toggleClass("show", !subShown);
        $(".caret", this).toggleClass("reversedCaret", !subShown);
        if (subShown) {
          $(".indented--sub").slideUp('100').removeClass("show");
          $(".sub-caret").removeClass("reversedCaret");
          $(".more-or-less").text("More");
        }
      }
    }
  });
});
.expander:hover {
  cursor: pointer;
}

.sub-expander--indented {
  padding: 0 0 0 23px;
}

.sub-caret {
  margin-right: 75px;
}

.indented,
.indented--sub {
  display: none;
}

.show {
  display: block;
}

  +

Item 1

Item 2

Item 3

Chapter 5

Chapter 6

Chapter 7

        +

More


Memory-Test for Soldiers I am solving the following problem on code chef:

  \$N\$ Soldiers are lined up for a memory test. They are numbered from 0 to N-1 from left to right.
  In the test, there are \$M\$ rounds. In each round, Captain selects one position. Soldier at that position will be numbered 0. All the soldiers to the right of selected position will be numbered one greater than the soldier to his left. All the soldiers to the left of selected position will be numbered one greater than the soldier to his right.
  eg. if \$N = 6\$ and selected position is 3, then the numbering will be \$[3, 2, 1, 0, 1, 2]\$.

  After \$M\$ rounds, Captain asked each soldier to shout out the greatest number he was assigned during the \$M\$ rounds. In order to check the correctness, Captain asked you to produce the correct values for each soldier (That is the correct value each soldier should shout out).

  Input
  - The first line of the input contains an integer \$T\$ denoting the number of test cases.
  - First line of each test case contains two integers, \$N\$ and \$M\$
  - Second line of each test case contains \$M\$ integers, the positions selected by Captain, in that order.

  Output
  For each test case, output one line with \$N\$ space separated integers.

  Constraints:
  \$1 ≤ T ≤ 10^4\$
  \$1 ≤ N ≤ 10^5\$
  \$1 ≤ M ≤ 10^5\$
  \$1 ≤ \sum{N} ≤ 10^5\$
  \$1 ≤ \sum{M} ≤ 10^5\$
  \$0 ≤ {Positions\ selected\ by\ captain} ≤ N-1\$

  Example

  Input

2
4 1
1
6 2
2 3

  Output

1 0 1 2
3 2 1 1 2 3

I have solved the problem using Java and my code runs perfectly fine in my PC but unfortunately I am getting Time Limit Exceeded for this code:

import java.io.*;
class ANUARM{
  public static void main(String[] s) throws Exception{
  BufferedReader input = new BufferedReader(new InputStreamReader(System.in));
  int testCases = Integer.parseInt(input.readLine());
  for(int i = 0 ; i < testCases ; ++i){
     String[] num = input.readLine().split(" ");
     int n = Integer.parseInt(num[0]);
     int m = Integer.parseInt(num[1]);
     int[] array = new int[n];
     String[] pos = input.readLine().split(" ");
     for(int j = 0 ; j<m ; ++j){
      int temp = Integer.parseInt(pos[j]);
      for(int k = 0 ; k<n ; ++k){
       int posAquired = temp - k;
        if(k > temp)
         posAquired = k-temp;
         if(posAquired > array[k])
          array[k] = posAquired;
      }
     }
     for(int a : array){
      System.out.print(a+" ");
     }
     System.out.println();
    }
  }
}

First I was using Scanner but I saw in FAQ that Scanner is slow, so I used BufferedReader BUT still I am getting TLE. Can anyone tell me where I can optimize my code as well as the do's-and-don'ts of optimization?

Scanner is slow compared to BufferedReader but that's not the main bottleneck of this code.
(The speed of parsing the input is hardly ever the bottleneck in programming contests.)

The main bottleneck is that for every position shouted out, you iterate over every soldier. The more positions are shouted out, the more wasteful this gets.

Realize that the captain could shout out a million positions, but all that will matter is the minimum and maximum positions. Let's say there are 7 soldiers,
and the captain shouts out a 1000 numbers, but always between 3 or 4:

  0 1 2 { 3 4 } 5 6

Regardless of how many numbers he shouted, the greatest numbers assigned to soldiers will be:

  4 3 2 { 1 1 } 2 3

This logic will simplify the algorithm:

Find the min and max number shouted by the captain
Iterate over the soldiers (just one pass!), and assign the largest of \$| i - max |\$ and \$|i - min|\$

Simple implementation in Java, given the number of soldiers, and the positions shouted out by the captain:

private int[] solution(int num, int... positions) {
    int[] soldiers = new int[num];
    int max = 0;
    int min = num - 1;
    for (int pos : positions) {
        min = Math.min(min, pos);
        max = Math.max(max, pos);
    }
    for (int i = 0; i < soldiers.length; ++i) {
        soldiers[i] = Math.max(Math.abs(i - max), Math.abs(i - min));
    }
    return soldiers;
}

Unit tests:

@Test
public void testBasic1() {
    assertArrayEquals(new int[]{1, 0, 1, 2}, solution(4, 1));
}

@Test
public void testBasic2() {
    assertArrayEquals(new int[]{3, 2, 1, 1, 2, 3}, solution(6, 2, 3));
}

Last but not least, the original code was poorly formatted and not well decomposed to methods. It's best to separate the parsing and the main logic, to see clearer, so you can focus better on the main implementation. Separating the parsing and the main logic sometimes sacrifices a bit of performance or requires more memory, but in my experience never so much to run out of time / memory. By writing in a clean way you effectively gain more coding time to focus on the real problem, and find the trick to optimize the algorithm.


JUnit Assert.assertSame I am using JUnit, Hibernate and Spring Framework. I wrote a simple test to test whether entity was updated correctly or not.

Is this correct way to test if entity was updated? Could it be done better?

@Test
public void testUpdated(){
    ConfirmationEntity confirmationEntity = confirmationDao.find(1L);
    confirmationEntity.setConfirmationString("0123456789");
    confirmationEntity.setConfirmationStringValidUntil(new LocalDateTime());

    confirmationDao.update(confirmationEntity);

    ConfirmationEntity confirmationEntityUpdated = confirmationDao.find(1L);

    Assert.assertSame(
        "Expected values to be the same.",
        confirmationEntity,
        confirmationEntityUpdated
    );
}

This test, on its own, is not enough. Additionally, the test is called testUpdated, but it does not actually test that the value is updated.

What it does is:

find an object based on a key value
it modifies some non-key content
it 'persists' the changes
it finds an object for the same key as before
it tests that the object it found the second time is the identical object to the first.

At no point in this process have you actually tested that the values you think you changed on the object, actually did change.

Additionally, you never tested whether those changes were persisted to the database/store.

What I would expect a test like this to do, is to:

get the object using the key
make the changes
assert that the values have changed to the new values.
persist the changes
assert that a persist happened (some timestamp in the database maybe)?
use an independent mechanism to check that the values in the database match the values in the object (a different database connection, a different DAO instance, something).
ensure that finding the same key value finds the same instance as before.

That is a lot of things to change, which means the test, on it's own, is not enough, and should be included with a bunch of other tests, each one testing different parts of the process.


Given an array of integers, return all pairs that add up to 100 I was recently given this programming challenge on an interview, and I decided to use javascript to solve it. I did, but I'm not happy with my implementation. I can't help thinking there must be a better way of doing this.

The exercise goes like this:

  Given an array of integers, write a function that returns an array of
  each pair of integers that add up to 100. The input is

    [0, 1, 100, 99, 0, 10, 90, 30, 55, 33, 55, 75, 50, 51, 49, 50, 51, 49, 51]

  and the function should return something like this (the order is not
  important).

    [ [0,100], [1,99], [10,90], [50,50], [49,51] ]

My implementation looks like this, is there a different approach out there?

var sample_data = [0, 1, 100, 99, 0, 10, 90, 30, 55, 33, 55, 75, 50, 51, 49, 50, 51, 49, 51]

function process(data){

    var result = []
    var a;
    var b;

    for (var i=0; i < data.length; i++) {

        a = data[i];

        for (var j=0; j < data.length; j++) {

            b = data[j]

            if ( (parseInt(a) + parseInt(b)) === 100 && result.indexOf(a+","+b) == -1 && result.indexOf(b+","+a ) == -1 ) {
               result.push( a+","+b )
            }

        }
    }

    for (var i=0; i < result.length; i++) {
        result[i] = result[i].split(',')
    }

    return result
}

process(sample_data);

okay, my javascript is not great but this should find all pairs in a single pass.

this assumes all values are between 0 and 100, like in the question.

Here is a working JSFiddle that shows the code in action.

var sample_data = [0, 1, 100, 99, 0, 10, 90, 30, 55, 33, 55, 75, 50, 51,
                      49, 50, 51, 49, 51];

var found = {};
var results = [];
for(var i of sample_data) {
    if (found[100 - i] === true) {
        // pair found
        results.push({
            a: i,
            b: 100 - i
        });
    }

    found[i] = true;
}

essentially, found is used as a hashset to see if the required reciprocal has already occurred in the array.

This makes the code more concise while being as easy to understand, therefore  more maintainable. The code is also likely to have superior performance to code with nested loops.


Random Dice Simulation I extended the example in 04-random.elm to support N dice and to draw the results using SVG.

import Html exposing (..)
import Html.Events exposing (..)
import Random
import List exposing (length, repeat, range)
import Svg exposing (Svg, svg, circle)
import Svg.Attributes exposing (..)

main =
  Html.program {
    init = init,
    view = view,
    update = update,
    subscriptions = subscriptions
  }

-- MODEL

type alias Model = {
  dieFaces : List Int
}

init : (Model, Cmd Msg)
init =
  let numDice = 5
  in
    (Model (repeat numDice 1), Cmd.none)

-- UPDATE

type Msg =
  Roll |
  NewFaces (List Int)

update : Msg -> Model -> (Model, Cmd Msg)
update msg model =
  case msg of
    Roll -> (
        model,
        Random.generate NewFaces (
          Random.list (length model.dieFaces) (Random.int 1 6)
        )
      )

    NewFaces newFaces ->
      (Model newFaces, Cmd.none)

-- SUBSCRIPTIONS

subscriptions : Model -> Sub Msg
subscriptions model =
  Sub.none

-- VIEW

view : Model -> Html Msg
view model =
  div [] (
    List.map drawFace model.dieFaces
    ++ [ button [ onClick Roll ] [ text "Roll" ] ]
  )

drawFace : Int -> Html Msg
drawFace numDots =
  div [ style "width: 66px; height: 66px; padding: 10px" ] [
    svg
      [ version "1.1", width "66", height "66" ]
      (drawDice ::
      (List.map (\i -> drawDot i numDots) (range 0 (numDots - 1))))
  ]

drawDice: Svg Msg
drawDice =
  circle [ fill "gold", stroke "orange", strokeWidth "3", cx "33", cy "33", r "30" ] []

drawDot: Int -> Int -> Svg Msg
drawDot i n =
  circle [ fill "black", cx (toString (posX i n)), cy (toString (posY i n)), r "5" ] []

posX: Int -> Int -> Int
posX i n =
  round (33 + (15 * cos (6.28 * (toFloat(i) / toFloat(n)))))

posY: Int -> Int -> Int
posY i n =
  round (33 + (15 * sin (6.28 * (toFloat(i) / toFloat(n)))))

This review comes with the caveat that I've spent a few days in total on Elm so far... Take anything I say with a pinch of salt.

Firstly, some style things.

posX: Int -> Int -> Int
posX i n =
  round (33 + (15 * cos (6.28 * (toFloat(i) / toFloat(n)))))

Why so many brackets? You can cut it down to this very easily

posX i n =
  round (33 + 15 * cos (6.28 * toFloat(i) / toFloat(n)))

But you can make it even clearer by using a |>

posX i n =
  33 + 15 * cos (6.28 * toFloat(i) / toFloat(n))
    |> round

I like this more because it also promotes the calculation to its own line where nothing else happens. You could also have written:

posX i n =
  round <| 33 + 15 * cos (6.28 * toFloat(i) / toFloat(n))

I think it's up to you which one you prefer. I like to think about getting the value first and then rounding it.

There is a similar thing here:

drawFace : Int -> Html Msg
drawFace numDots =
  div [ style "width: 66px; height: 66px; padding: 10px" ] [
    svg
      [ version "1.1", width "66", height "66" ]
      (drawDice ::
      (List.map (\i -> drawDot i numDots) (range 0 (numDots - 1))))
  ]

I would instead have:

drawFace : Int -> Html Msg
drawFace numDots =
  div [ style "width: 66px; height: 66px; padding: 10px" ] [
    svg
      [ version "1.1", width "66", height "66" ]
      (range 0 (numDots - 1)
        |> List.map (\i -> drawDot i numDots)
        |> (::) drawDice
      )
  ]

It may look more complex but it's easier to follow in my opinion because each line does one thing.

Create your range
Map each to an Svg Msg (your dots)
Cons the face to the list of dots

Now, if you change the order of your arguments to drawDot you can simplify it further with currying.

drawDot: Int -> Int -> Svg Msg
drawDot n i =
  circle [ fill "black", cx (toString (posX i n)), cy (toString (posY i n)), r "5" ] []

(range 0 (numDots - 1)
    |> List.map (drawDot numDots)
    |> (::) drawDice
)

You'll find that you need to think about the usage of the function before writing it so you can get the parameter order right for currying.

In my limited understanding/knowledge of Elm, I'd say that your code is pretty good :)


Calculating Pi in Clojure Obviously there are better ways of obtaining Pi, but for educational purposes, how is the code below?

(defn leibniz-numerator [x] (* (- (rem x 4) 2) -4.0))

(defn calc-pi-leibniz
    "Calculate pi with Leibniz formula
    4/1 - 4/3 + 4/5 - 4/7 + 4/9 etc
    Very slow convergence"
    [terms]
    (reduce + (map #(/ (leibniz-numerator %) %) (range 1 terms 2))))

(print (calc-pi-leibniz 10000000))

I'd say your solution is pretty good! Since you probably won't re-use your leibniz-numerator function, you might want to make it private by declaring it with defn-, or you could declare it within the body of your main function using let or letfn.

Also, you might notice that your function actually computes pi with half the number of terms given has parameters (might not be of importance).

I can share some alternatives I came up with.

We could start by generating the sequence (4 -4 4 -4...). To do that, we can use the method iterate:

(take 4 (iterate - 4))
;;=> (4 -4 4 -4)

Then we want the sequence (1 3 5 7). You already found method rangefor that:

(take 4 (range 1 10 2))
;;=> (1 3 5 7)

If you divide those to element wise, you will get (4 -4/3 4/5 -4/7...). You can do that with map

(take 4 (map / (iterate - 4) (range 1 10 2)))
;;=> (4 -4/3 4/5 -4/7)

In our final computation, this can get really slow because we're using decimals. Let's use floats instead, and put everything together:

(defn calc-pi-leibniz [terms]
  (reduce + (map / (iterate - 4.0) (range 1.0 terms 2.0))))

But for some reason, this is 5 times slower than your solution. If someone could tell me why, I'd be grateful. I suspect that is because map has to go through two lists instead of one, but since they are both lazy this should not matter?

For a faster solution (but less elegant), we can see that there are two subsequences in the leibniz formula. One that is (4 4/5 4/9) and one that is (-4/3 -4/7 -4/11). With that in mind, you can get the following:

(defn calc-pi-leibniz [terms]
  (* 4 (- (reduce + (map / (range 1.0 terms 4.0)))
          (reduce + (map / (range 3.0 terms 4.0))))))

Which runs as fast as your solution.


Varying Parameter Names in Method Overloads Probably a dumb question but just can't seem to make a decision here.

I'm building a C# class library that I intend for other developers to use. Let's say one of these classes contains a method that accesses a remote resource and maps it to an object:

public class FooClient
{
    public async Task<Foo> ShowAsync(string name, ...)
    {
        return await DoStuffAsync(name);
    }
}

Where name identifies this remote resource. Let's also say that this resource can be identified by a numerical id, and that I can pass id.ToString() as name to this method and retrieve the same resource. So, I want an overload:

public class FooClient
{
    public async Task<Foo> ShowAsync(string name, ...)
    {
        return await DoStuffAsync(name);
    }
    public Task<Foo> ShowAsync(long id, ...)
    {
        return ShowAsync(id.ToString(), ...);
    }
}

According to the Microsoft docs, I should avoid arbitrarily varying parameter names, so I'm tempted to do something like this:

public class FooClient
{
    public async Task<Foo> ShowAsync(string foo, ...)
    {
        return await DoStuffAsync(foo);
    }
    public Task<Foo> ShowAsync(long foo, ...)
    {
        return ShowAsync(foo.ToString(), ...);
    }
}

But I'm afraid that, in this case, it would be more descriptive to have different names for the parameters, even though they identify the same resource. What should I do?

To answer your direct question - always name your parameters to match the intent of the type. If they are passing in an Id, call it an Id. What Microsoft is saying is if you had the following:

    public async Task<Foo> ShowAsync(string name, ...)
    {
        return await DoStuffAsync(foo);
    }
    public Task<Foo> ShowAsync(string resourceName, int id, ...)
    {
        return ShowAsync(resourceName + foo.ToString(), ...);
    }

In that case, name and resourceName represent the exact same concept: they are both the name of the resource. If you are using the same type and concept within overloads, always keep the name the same. For this, you are changing the type and concepts so change the name to match the concept.

With that said...

In this particular instance, I personally probably wouldn't use overloading and would, instead, say ShowBynameAsync and ShowByIdAsync.

Downside - there's two distinct methods.

A potential upside is that no matter how the developer uses your API, it's always clear (to future readers) what they are passing in. Also, it's extremely clear what's being requested. You could still delegate the "showing" to ShowByNameAsync.

It's not "wrong" to use overloading here and not "right" to use descriptive method names. Just a preference.


Jump game to find minimum hops from source to destination Given an array start from the first element and reach the last by jumping. The jump length can be at most the value at the current position in the array. Optimum result is when u reach the goal in minimum number of jumps.

For example:

  Given array A = {2,3,1,1,4}
  possible ways to reach the end (index list)
  i) 0,2,3,4 (jump 2 to index 2, then jump 1 to index 3 then 1 to index 4)
  ii) 0,1,4 (jump 1 to index 1, then jump 3 to index 4)

Since second solution has only 2 jumps it is the optimum result.

A lot of input has been derived from previous review here.

public final class JumpGame {

    private JumpGame() {}

    /**
     * Returns the shortest jump path from the source to destination
     *
     * @param jump     The jump array
     * @return         Returns one of the shortest paths to the destination.
     */
    public static List<Integer> getShortestJumps(int[] jump) {
        final List<Integer> list = new ArrayList<Integer>();
        list.add(0);

        for (int i = 0; i < jump.length - 1; ) {
            int iSteps = Math.min(jump.length - 1, i + jump[i]); // iSteps is all the consecutive steps reachable by jumping from i.
            int maxStep = Integer.MIN_VALUE; // max step is one of the step in iSteps, which has the max potential to take us forward.
            /*  trying each step of iSteps */
            for (int j = i + 1; j <= iSteps; j++) {

                /* being greedy and picking up the best step */
                if (maxStep < jump[j]) {
                    maxStep = j;
                }
            }
            list.add(maxStep);
            i = maxStep; // jump to the maxStep.
        }

        return list;
    }

    public static void main(String[] args) {
        int[] a1 = {2,3,1,1,4};
        List<Integer> expected = new ArrayList<Integer>();
        expected.add(0);
        expected.add(1);
        expected.add(4);
        Assert.assertEquals(expected, getShortestJumps (a1));

        int[] a2 = {3, 1, 10, 1, 4};
        expected = new ArrayList<Integer>();
        expected.add(0);
        expected.add(2);
        expected.add(4);
        Assert.assertEquals(expected, getShortestJumps (a2));
    }

}

Code-Style
For the most part, this is good:

return types are good.
Collections are well typed
method names are good
apart from list, a1, and a2, variable names are good

The list is a bad name for a List. The fact that it is a list is obvious... but, what is the list...? I prefer result to list.
a1 and a2 are not bad, but they are not good either. They are just 'OK'.
Algorithm
I have worked the code through, and figure there is a plane-jane O(n) solution to the problem. Your algorithm is something larger than O(n), It is closer to O(n * m) where m is the longest jump.
The alternate algorithm I cam up with sets up a 'range' for the next jump.... It seaches all the values from the current cursor to the current range, and it looks for the jump within that range that would extend the range the furthest.
The method looks like:
public static List<Integer> getShortestJumps(int[] jump) {
    final List<Integer> list = new ArrayList<Integer>();
    if (jump.length == 0) {
        return list;
    }
    int cursor = 0;
    int best = 0;
    int range = 0;
    int remaining = 1;
    while (cursor + 1 < jump.length) {
        if (cursor + jump[cursor] > range) {
            // jumping from here would extend us further than other alternatives so far
            range = cursor + jump[cursor];
            best = cursor;
            if (range >= (jump.length - 1)) {
                // in fact, this jump would take us to the last member, we have a solution
                list.add(best);
                break;
            }
        }
        if (--remaining == 0) {
            // got to the end of our previous jump, move ahead by our best.
            list.add(best);
            remaining = range - cursor;
        }

        cursor++;
    }
    // always add the last member of the array
    list.add(jump.length - 1);
    return list;
}


Simple Example of an Iterable and an Iterator in Java I was a TA for a first-year Java programming course this year. As a very simple example of iterables/iterators, I wrote the following code for the students. I am curious if any styling or other improvements can be made.

import java.util.NoSuchElementException;
import java.util.Iterator;

public class Range implements Iterable<Integer> {
    private int start, end;

    public Range(int start, int end) {
        this.start = start;
        this.end = end;
    }

    public Iterator<Integer> iterator() {
        return new RangeIterator();
    }

    // Inner class example
    private class RangeIterator implements
                    Iterator<Integer> {
        private int cursor;

        public RangeIterator() {
            this.cursor = Range.this.start;
        }

        public boolean hasNext() {
            return this.cursor < Range.this.end;
        }

        public Integer next() {
            if(this.hasNext()) {
                int current = cursor;
                cursor ++;
                return current;
            }
            throw new NoSuchElementException();
        }

        public void remove() {
            throw new UnsupportedOperationException();
        }
    }

    public static void main(String[] args) {
        Range range = new Range(1, 10);

        // Long way
        Iterator<Integer> it = range.iterator();
        while(it.hasNext()) {
            int cur = it.next();
            System.out.println(cur);
        }

        // Shorter, nicer way:
        // Read ":" as "in"
        for(Integer cur : range) {
            System.out.println(cur);
        }
    }
}

Variables

I understand why you have the Range.this.end and Range.this.start to avoid confusion about where those variables come from... If you need the Range.this as part of the teaching exercise, then sure. Otherwise, I would recommend three things....

add range as a prefix, even though it is slightly redundant
Make them final...
one variable per line... (it makes revision-control diffs/patches easier to read)

The code would look like:

private final int rangeStart;
private final int rangeEnd;

Then, all the Range.this.start would be just rangeStart, etc.

Nested classes

Your iterator class is a non-static class, so it can reference the outer class's range start/end.

In this case, the nested class can be changed to a static class very easily. This has the potential of simplifying memory management because the iterator does not need a reference to the enclosing Range.

Consider a private-static Iterator instance:

// Inner class example
private static final class RangeIterator implements
                Iterator<Integer> {
    private int cursor;
    private final int end;

    public RangeIterator(int start, int end) {
        this.cursor = start;
        this.end = end;
    }

    public boolean hasNext() {
        return this.cursor < end;
    }

    public Integer next() {
        if(this.hasNext()) {
            int current = cursor;
            cursor ++;
            return current;
        }
        throw new NoSuchElementException();
    }

    public void remove() {
        throw new UnsupportedOperationException();
    }
}

This static class removes the need for the back-references to Range.this entirely....

The new iterator is called simply with:

public Iterator<Integer> iterator() {
    return new RangeIterator(start, end);
}

Pre-Validate

It is better to pre-validate state, than to fall-through to an error... This code:

    public Integer next() {
        if(this.hasNext()) {
            int current = cursor;
            cursor ++;
            return current;
        }
        throw new NoSuchElementException();
    }

would be better as:

    public Integer next() {
        if(!this.hasNext()) {
            throw new NoSuchElementException();
        }
        int current = cursor;
        cursor ++;
        return current;
    }

Post-Increment

This block can be simplified:

        int current = cursor;
        cursor ++;
        return current;

to just:

return cursor++;

although I imagine this is done as an education ploy.

Integer as the example

Because of the int auto-bocxing I worry that Integer may not be the right choice for data type. You may want to consider a non-primitive as the data.

Autoboxing is the sort of thing that will confuse.

Conclusion

Otherwise, I don't see much in the way of problems.


Generic equality checker I use this method to check if two reference types are equal

  public static bool AreEquals<T>(T source, object obj) where T : class
  {
     if (ReferenceEquals(source, obj))
        return true;

     var convertedTarget = obj as T;

     if (ReferenceEquals(convertedTarget, null))
        return false;

     List<object> equalityMembers = typeof (T).GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).ToList<object>();
     equalityMembers.AddRange(typeof(T).GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Where(p => p.GetCustomAttribute<EqualityMemberAttribute>() != null).ToList<object>());

     if (equalityMembers.Count == 0)
        return false; //The references were not equals and there is nothing to compare..

     var enumerator = equalityMembers.GetEnumerator();
     bool areEquals = true;

     while (enumerator.MoveNext() && areEquals)
     {
        var current = enumerator.Current;

        //On sait que FieldInfo et PropertyInfo ont la méthode GetValue
        var methodInfo = current.GetType().GetMethod("GetValue", new [] {typeof (object)});

        object valueSource = methodInfo.Invoke(current, new object[] {source});
        object valueObj = methodInfo.Invoke(current, new object[] {convertedTarget});

        areEquals = valueSource.Equals(valueObj);
     }
     return areEquals;
  }

I use attributes on properties/fields to find which of them are used to decide if the two objects are equals ex :

public class MyClass
{
    [EqualityMember]
    public int ID{get;set;}

    public override Equals(object obj)
    {
        return MyStaticClass.AreEquals(this,obj);
    }
}

It works very well but I am bothered with how I get the properties and fields in a list and use reflection to invoke the GetValue method, would you have any other alternatives? Or is there anything that seems flawed in my method?

I took a shot at this as well. I think @Hangy had a few good ideas but ended up with a rather complex solution. I haven't added any caching so you might want to add that yourself afterwards.

What I changed:

Different check for null which is clearer in my opinion.
Braces for one-line statements.
Extracted member parsing to a separate method.
Instead of holding all data of PropertyInfo and FieldInfo, I simply use a Dictionary<string, object> which translates to <FieldName, FieldValue>.
Unused properties and fields data is eligible for GC as soon as you extracted the needed information.
Got rid of the Enumerator and added some juicy LINQ.

The end result:

public static class EqualityAttribute
{
    public static bool AreEquals<T>(T source, object obj) where T : class
    {
        if (ReferenceEquals(source, obj))
        {
            return true;
        }

        var convertedTarget = obj as T;
        if (convertedTarget == null)
        {
            return false;
        }

        var obj1Members = GetFieldEqualityMembers(source);
        var obj2Members = GetFieldEqualityMembers(obj);

        var noMembersToCompare = obj1Members.Values.Count == 0 && obj2Members.Values.Count == 0;
        if (noMembersToCompare)
        {
            return false;
        }

        if (obj1Members.Keys.Any(key => !obj1Members[key].Equals(obj2Members[key])))
        {
            return false;
        }

        return true;
    }

    private static Dictionary<string, object> GetFieldEqualityMembers<T>(T obj)
    {
        var properties = obj.GetType().GetProperties(BindingFlags.Public |
                                                     BindingFlags.NonPublic |
                                                     BindingFlags.Instance)
            .Where(x => x.GetCustomAttribute<EqualityMemberAttribute>() != null)
            .ToDictionary(x => x.Name, x => x.GetValue(obj));

        var fields = obj.GetType().GetFields(BindingFlags.Public |
                                             BindingFlags.NonPublic |
                                             BindingFlags.Instance)
            .Where(x => x.GetCustomAttribute<EqualityMemberAttribute>() != null)
            .ToDictionary(x => x.Name, x => x.GetValue(obj));

        properties.ToList().ForEach(x => fields.Add(x.Key, x.Value));
        return fields;
    }
}


Parsing prospero parameters Prospero is a URI scheme. There you have fields and values.

Could you check this code and give common suggestions or even test cases for JUnit tests?

(The documentation is in German.)

/**
 * Gibt einen Prospero-Parameter aus der URL zurück. Dabei wird nur der Prospero-Part geprüft.
 *
 * @param url Die URL, darf nicht null sein.
 * @param name Der Name des Parameters, darf kein '=' enthalten, darf nicht null
 *            sein.
 * @return Den Wert des Parameters oder
 *

 *
null

 *
wenn der Parameter ohne = angegeben wurde.

 *
error-Parameter

 *
wenn kein solcher Parameter gefunden wurde

 *

 */
public static String getProsperoParam(URL url, String name, String error) {
    String path = url.getPath();
    String[] split = path.split(";", 2);
    if (split.length == 2) {
        String params = split[1];
        for (String param : params.split("&")) {
            String[] paramParts = param.split("=", -1);
            if (paramParts.length == 1) {
                if (name.equals(paramParts[0])) {
                    return null;
                }
            }
            if (paramParts.length == 2) {
                if (name.equals(paramParts[0]))
                    return paramParts[1];
            }
            if (paramParts.length > 2) {
                if (name.equals(paramParts[0])) {
                    return param.substring(name.length() + 1);
                }
            }
        }
    }
    return error;
}

I successfully tested these cases:

assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a?a=a#a"), "a", null) == null;
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=4?a=a#a"), "a", null).equals("4");
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=4=a?a=a#a"), "a", null).equals("4=a");
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=4=a&m=3?a=a#a"), "m", null).equals("3");
String missingString = "Missing!";
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=4=a&m=3?a=a#a"), "e", missingString) == missingString;
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;e?a=a#a"), "e", "asdf") == null;
assert URLTools.getProsperoParam(new URL("http://a:a@a.a.a/a.a;e=?a=a#a"), "e", null).equals("");

Check input

Your comments mention that url shouldn't be null, so it's best to make sure that it really isn't:

    if (url == null) {
        throw new IllegalArgumentException("url cannot be null");
    }

Nested if statements

        if (paramParts.length == 1) {
            if (name.equals(paramParts[0])) {
                return null;
            }
        }

You have this principle three times. It can be written as:

        if (paramParts.length == 1 && name.equals(paramParts[0])) {
            return null;
        }

Although I would rewritten your if statements like this:

        if (paramParts.length > 0 &&  name.equals(paramParts[0])) {
            switch(paramParts.length) {
                case 1:
                    return null;
                case 2:
                    return paramParts[1];
                default:
                    return param.substring(name.length() + 1);
            }
        }

And are you sure that paramParts.length == 2 (now case 2) is really needed? I think the other cases cover it.

Use early return

If you return early, you can reduce nesting:

if (split.length != 2) {
    return error;
}

Tests

The specification for prosperos says:

  each field/value pair is separated from each other and from the rest of the URL by a ";" (semicolon).

You are never testing if this would work, and it wouldn't:

String prosperoParam = prospero.getProsperoParam(new URL("http://a:a@a.a.a/a.a;a=a;b=b"), "b", null);
assertEquals(prosperoParam, "b");

If you want to change the spec, I would comment on it in the Java doc (something like: See RFC 4157, but note that this implementation uses '&' instead of ';' for the separation of field/value pairs (it still does use ';' for the separation between the first field/value pair and the rest of the URL).

Also, sometimes you use == to compare strings in your tests, I would replace it with equals.


Loop optimization for image processing I have this piece of code that is running too slow. I was wondering if anyone can help me optimize it as I can't seem to find any more shortcuts. I'm not sure if using List<> is going to help me but I need complex operation such as Union and Overlap.

Also, List is desirable because I don't know how many unique partitions an Image Region is going to be before running.

u1.length = 13254 which is the number of distinct elements in RevisedListMeanH
RevisedListMeanH.Count = 90000

The purpose of the first piece of code is to group together pixels via horizontal comparison and vertical comparison. This runs for about 70 seconds.

The second portion combines both vertical and horizontal pixel blocks into a 2D-block. This section runs for about 120 seconds. My goal is to have both of these loops complete under 10 seconds.

These numbers are from a 300x300 pixel region comparisons of a 4000x3000 image.

watch.Start();
for (int s = 0; s < u1.Length; s++ )//iterate through uniquelist
{
    List<int> ConnectedBlocksH = new List<int>();
    List<int> ConnectedBlocksV = new List<int>();

    float[] RH = RevisedListMeanH.ToArray();
    float[] RV = RevisedListMeanV.ToArray();

    for (int a = 0; a < RevisedListMeanH.Count; a++)//iterate through bitmap with res
    {
        if (u1[s] == RH[a])
        {
            ConnectedBlocksH.Add(a);//add the index

        }
    }
    for (int a = 0; a < RevisedListMeanV.Count; a++)//iterate through bitmap with res
    {
        if (u1[s] == RV[a])
        {
            ConnectedBlocksV.Add(a);//add the index

        }
    }
    ArrayOfConnectedBlocksH[s] = ConnectedBlocksH;//where the data IS
    ArrayOfConnectedBlocksV[s] = ConnectedBlocksV;//where the data IS
}
watch.Stop();
long asasasdda = watch.ElapsedMilliseconds; //71 sec
watch.Reset();

 watch.Start();
//if intersects then union the two lists
//iterate through both list
List<List<int>> ListOfConnectedBlocks = new List<List<int>>();
for (int a = 0; a < ArrayOfConnectedBlocksH.Length; a++ )
{
    HashSet<int> i = new HashSet<int>(ArrayOfConnectedBlocksH[a]);
    //trigger means it scanned and there was no overlap to add to the group
    while (true)
    {

        bool trigger = true;
        for (int b = 0; b < ArrayOfConnectedBlocksV.Length; b++)
        {
            if (i.Overlaps(ArrayOfConnectedBlocksV[b]))
            {
                i.UnionWith(ArrayOfConnectedBlocksV[b]);//combines all overlaps into one group
                ArrayOfConnectedBlocksV[b].Clear();//merged so just remove
                trigger = false;
            }

        }
        if (trigger)
        {
            break;
        }
        trigger = true;
        for (int c = 0; c < ArrayOfConnectedBlocksH.Length; c++)//now cycle through horizontal
        {
            if (i.Overlaps(ArrayOfConnectedBlocksH[c]))
            {
                i.UnionWith(ArrayOfConnectedBlocksH[c]);//combines all overlaps into one group
                ArrayOfConnectedBlocksH[c].Clear();//merged so just remove
                trigger = false;
            }

        }//first cycle to get T0

        if(trigger)
        {
            break;
        }
    }

    if (i.Count != 0)
    {
        ListOfConnectedBlocks.Add(i.ToList<int>());
    }
}

watch.Stop();
long asasasda = watch.ElapsedMilliseconds;//122 seconds

First of, I'm gonna give you a basic idea...

Instead of doing 2 loops over two different arrays, check their sizes first. Create a loop for the smaller of both array sizes. In that loop you will process that Array, and then create another loop for the remainder of unprocessed data.

This can be done for your two double fors you have in there. This way you could reduce the complexity from O(n+m) to O(n+(m-n))

The pseudo code is this...

if(Arr1 > Arr2)
{
   process(Arr1, Arr2);
}
else if (Arr2 > Arr1)
{
   process(Arr2, Arr1);
}
else //They're the same size!
{
  for(idx = 0; idx != count(any will do); ++idx)
  {
     //process Arr1
     //process Arr2
   }
}


Group key-value pairs in chunks no longer than specified limit Sometime ago I saw a code which packed key,value pairs into several GET messages. That code had a bug (that is why it was shown to me: as the example of beautiful code which doesn't do what it should).

Today I decided to write this code out of curiosity and for training.

This is what I have written:

def group(url,D,limit):
    if len(D) == 0:
        if len(url) <= limit:
            yield url
            return
        else:
            raise ValueError("url is longer than limit")

    if len(url) > limit - 2:
        raise ValueError("url is too long to be able to hold any key-value pairs")

    for i,(key,value) in enumerate(D.iteritems()):
        if i == 0:
            old = url
            new = '{url}?{key}={value}'.format(**locals())
            continue
        new, old = '{new}&{key}={value}'.format(**locals()), new

        if len(new) > limit:
            if len(old) > limit:
                raise ValueError("String '{old}' is longer than GET query limit.".format(**locals()))
            yield old
            old = url
            new = '{url}?{key}={value}'.format(**locals())
    if len(new) != len(url):
        if len(new) > limit:
            raise ValueError("String '{new}' is longer than GET query limit.".format(**locals()))
        yield new

I have written some tests and the code seems to work correctly.

Still, I don't like it.

If we leave alone corner case checking at the beginning, we will only have one working cycle.
Inside that cycle I had to keep two strings: old and new. If new is longer than the limit, I yield the old one, and start a new string with blank url plus key,value pair added which caused the previous new string to grow beyond the limit.

To make it work I had to write the same statements in different places of that loop.

For example,

old = url
new = '{url}?{key}={value}'.format(**locals())

is written it two places.

raise ValueError("String '{new}' is longer than GET query limit.".format(**locals()))

is also in two places.

Can it be refactored to have statements to be written only once without duplication?

Are there any other things that I should pay attention to?

def group(url,D,limit):

D is a bad parameter name because it doesn't follow the python convention: lowercase_with_underscores, and because it a pointless abbreviation.

    if len(D) == 0:
        if len(url) <= limit:
            yield url
            return
        else:
            raise ValueError("url is longer than limit")

    if len(url) > limit - 2:
        raise ValueError("url is too long to be able to hold any key-value pairs")

Is this really a useful check? Two characters is probably not enough to actually hold any of the pairs. You probably don't catch anything here you wouldn't already catch

    for i,(key,value) in enumerate(D.iteritems()):
        if i == 0:

enumerating for the sole purpose of special casing the first entry is rather gross.

            old = url
            new = '{url}?{key}={value}'.format(**locals())
            continue

I discourage the use of continue. It's almost always better to put things in an else block

        new, old = '{new}&{key}={value}'.format(**locals()), new

Use of locals() is discouraged. It encourages sloppy coding and forces python into a slower compatibility mode for that function.

        if len(new) > limit:
            if len(old) > limit:
                raise ValueError("String '{old}' is longer than GET query limit.".format(**locals()))
            yield old
            old = url
            new = '{url}?{key}={value}'.format(**locals())
    if len(new) != len(url):

In which case would this be false?

        if len(new) > limit:

Why is this check inside the other check?

            raise ValueError("String '{new}' is longer than GET query limit.".format(**locals()))
        yield new

Of course, you've already noted the duplicated code here.

Here's my take on reworking it:

def blocked(items, limit, seperator):
    """
    items is a list of strings, to be joined with seperator.
    Produces a generator of strings, each with a length <= the limit
    """
    current = []
    current_length = 0
    for item in items:
        length = len(item) + len(seperator)
        if length > limit:
            raise ValueError('{} cannot fit'.format(item))
        elif length + current_length > limit:
            yield seperator.join(current)
            current = [item]
            current_length = length
        else:
            current.append(item)
            current_length += length
    yield seperator.join(current)

def group(url,data,limit):
    if len(url) > limit:
        raise ValueError('{} is longer than the limit of {}'.format(url, limit))

    pairs = ["{}={}".format(k,v) for k, v in data.iteritems()]
    for block in blocked(pairs, limit - len(url), "&"):
        if block:
            yield '{}?{}'.format(url, block)
        else:
            yield url


Save XML tags and information to another file This was a coding challenge I had done recently. I was given a text file and in it thee was couple of XML tags. The challenge was to get all the XML tags and the information and save it to another file.

The string I am given:

"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin nec arcu non nisl dapibus scelerisque. Phasellus sagittis ligula vel convallis porttitor. Maecenas sed velit arcu. Proin efficitur ipsum vitae augue semper, bibendum sollicitudin neque egestas".

<OpeningTag><AnotherOpeningTag>Information</AnotherClosingTag></ClosingTag>

"Etiam venenatis feugiat erat, in egestas tellus lacinia eu. Phasellus volutpat lectus nec tristique volutpat. Sed tempus sollicitudin mauris eu blandit.<OpeningTag><AnotherOpeningTag>Information</AnotherClosingTag></ClosingTag> Vestibulum consectetur euismod dui, sit amet blandit ex ultrices ac".

This is the code I came up with:

StringReader reader = new StringReader(text);
while ((text = reader.ReadLine()) != null)
{
if (text.StartsWith("<") && text.EndsWith(">"))
{
    txtXml += text + "\n";

}
else
{
    string regexPattern = @"(<.*>)(.*)(<\/.*>)";
    Regex regex = new Regex(regexPattern, RegexOptions.Singleline);

    MatchCollection collection = regex.Matches(text);

    var list = collection.Cast<Match>().Select(match => match.Value).ToList();

    for (int i = 0; i < list.Count; i++)
    {
        txtXml += list.ElementAt(i);
    }
}
}

This code works and I am getting all the tags and the information and stuff. I just wanted to know if I could achieve this same task in smaller code or code that executes faster than the current method.

There are some good points raised in comments about the requirements, and you really need to think about them in order to write code that solves the problem.  I can get you started with some feedback though.

Organize your code into a function, so that it's clear what the inputs and outputs are.

StringReader reader = new StringReader(text);
while ((text = reader.ReadLine()

This is a really odd reuse of the variable text.  Declare another string, maybe called line.  But also reconsider if a line-centric approach is even appropriate.  XML is pretty free about where whitespace (including newlines) can appear - what if there is a break between the tag name and some attributes?  How will this affect your solution?

For the Regex @"(<.*>)(.*)(<\/.*>)", note that all of these quantifiers are greedy, which may result in you capturing more than you want.  Of course, it may turn out with some clarified specs that a regex isn't even the appropriate tool to use here.


Organizing semaphores into a pyramid My homework was to organize semaphores into a pyramid. The task was accomplished, but I ended up with this long ugly conditional and was wondering if anyone can see a better way/algorithm to accomplish the same thing.

if (me==0) {
    wait_sem(semaphore[1]);
    wait_sem(semaphore[2]);
    signal_sem(semaphore[0]);

}

if (me==1) {
    wait_sem(semaphore[3]);
    wait_sem(semaphore[4]);

}
if (me==2) {
    wait_sem(semaphore[4]);
    wait_sem(semaphore[5]);

}

if (me==3) {
    wait_sem(semaphore[6]);
    wait_sem(semaphore[7]);

}
if (me==4) {
    wait_sem(semaphore[7]);
    wait_sem(semaphore[8]);

}
if (me==5) {
    wait_sem(semaphore[8]);
    wait_sem(semaphore[9]);

}
if (me==6) {
    wait_sem(semaphore[10]);
    wait_sem(semaphore[11]);

}
if (me==7) {
    wait_sem(semaphore[11]);
    wait_sem(semaphore[12]);

}
if (me==8) {
    wait_sem(semaphore[12]);
    wait_sem(semaphore[13]);

}

if (me==9) {
    wait_sem(semaphore[13]);
    wait_sem(semaphore[14]);

}

I wouldn't even use a switch here:

wait_sem(semaphore[me*2+1]);
wait_sem(semaphore[me*2+2]);
if (me==0) {
    signal_sem(semaphore[0]);
}

Edit: I see the sequence is not quite linear. If you can come up with an easy mapping between me and the indices, this will work (but with a different formula). Otherwise, a switch statement may be the easiest way.

You can also bury the switch inside a map by mapping the input ints to the two indices (std::map<int, std::pair<int, int> >). It essentially does the same thing, but you can hide the ugly in a header file.


VS 2015 std::char_traits operations At my workplace, we changed string type (which holds internationalized characters) for from std::wstring to std::u16string after VS 2015(Update 3) compiler upgrade.

Due to this, we are seeing loads of performance regressions such as this.

The profiler analysis reveals that std::u16string's std::char_traits<char16_t> operations such as copy, compare, find and assign are the most hit and are taking longer than std::wstring's std::char_traits<wchar_t> counterparts.

These std::char_traits<wchar_t> operations are written in terms of std::wmem* and std::char_traits<char16_t> operations are written in terms of for loops.

If we change these traits operations for char16_t type (or std::u16string) to use our own customized traits, we are seeing performance improvements with performance comparable to std::wstring.

We are planning to write our own custom traits (until MS fixes it for next version of VS) as follows

struct string_custom_traits : public std::char_traits<char16_t>
{
    static const char16_t * copy(char16_t* dest, const char16_t* src, size_t count)
    {
        return (count == 0 ? src : (char16_t*)std::memcpy(dest, src, count * sizeof(char16_t)));
    }

 };

Would that be OK? Are there any problems with this approach ?

Nothing in std::char_traits::copy says to return src when count is zero.  Further, we must return a pointer to modifiable char_type, which also precludes returning src.

So I think we should write:

#include <cstring>
#include <string>

struct string_custom_traits : public std::char_traits<char16_t>
{
    static char_type * copy(char_type* dest, const char_type* src, size_t count)
    {
        return (char_type*)std::memcpy(dest, src, count * sizeof *src);
    }

    static char_type *move(char_type* dest, const char_type* src, size_t count)
    {
        return (char_type*)std::memmove(dest, src, count * sizeof *src);
    }
};

I've used the template-provided alias char_type for consistency and in case you want to turn this traits class into a template itself.

Make sure you understand and accept the differences between UCS-2 and UTF-16 if you handle characters outside the BMP.


Improving my code of Margrabe Formula in Python I am new to Python and I want to see how can I improve my code to make it faster and cleaner. Below you can see the code of Margrabe's Formula for pricing Exchange Options.

I think the part where I check if the argument is None and if not take predetermined values, can be improved but i don't know how to do it. Is using scipy.stats.norm for calculating the pdf and cdf of the normal distribution faster than manually coding it myself?

    from __future__ import division
    from math import log, sqrt, pi
    from scipy.stats import norm

    s1 = 10
    s2 = 20
    sigma1 = 1.25
    sigma2 = 1.45
    t = 0.5
    rho = 0.85

    def sigma(sigma1, sigma2, rho):
        return sqrt(sigma1**2 + sigma2**2 - 2*rho*sigma1*sigma2)

    def d1(s1, s2, t, sigma1, sigma2, rho):
        return (log(s1/s2)+ 1/2 * sigma(sigma1, sigma2, rho)**2 * t)/(sigma(sigma1, sigma2, rho) * sqrt(t))

    def d2(s1, s2, t, sigma1, sigma2, rho):
        return d1(s1,s2,t, sigma1, sigma2, rho) - sigma(sigma1, sigma2, rho) * sqrt(t)

    def Margrabe(stock1=None, stock2=None, sig1=None, sig2=None, time=None, corr=None):

        if stock1 == None:
            stock1 = s1
        if stock2 == None:
            stock2 = s2
        if time == None:
            time = t
        if sig1 == None:
            sig1 = sigma1
        if sig2 == None:
            sig2 = sigma2
        if corr==None:
            corr = rho

        dd1 = d1(stock1, stock2, time, sig1, sig2, corr)
        dd2 = d2(stock1, stock2, time, sig1, sig2, corr)
        return stock1*norm.cdf(dd1) - stock2*norm.cdf(dd2)

    print "Margrabe = " + str(Margrabe())

First of all, when you compare to None, you are better off using if stock1 is None: - an object can define equality, so potentially using == could return True even when stock1 is not None. Using is checks identity, so it will only return True in the case it really is None.

As to simplifying your code, you can simply place the default values directly into the function definition:

def margrabe(stock1=s1, stock2=s2, sig1=t, sig2=sigma1, time=sigma2, corr=rho):
    dd1 = d1(stock1, stock2, time, sig1, sig2, corr)
    dd2 = d2(stock1, stock2, time, sig1, sig2, corr)
    return stock1*norm.cdf(dd1) - stock2*norm.cdf(dd2)

This makes your code significantly more readable, and much shorter.

It is also worth a note that CapWords is reserved for classes in Python - for functions, use lowercase_with_underscores - this is defined in PEP-8 (which is a great read for making your Python more readable), and helps code highlighters work better, and your code more readable.


"CommandBus" with Handlers & Observers I'm looking for a very simple (at first sight) CommandBus, which will handle some ICommand publication. The CommandBus implementation will find the appropriate IHandler to Execute the Command and then Notify some possible IObservers.

My first step is to make it work, I've chosen to use a synchronous dependency injection pattern which I may extends in the future, maybe using a real ServiceBus with some asynchronous capability.

Here is the abstract definition:

public interface ICommandBus
{
    TResult Publish<TCommand, TResult>(TCommand command)
        where TResult : ICommandResult
        where TCommand : ICommand<TResult>;
}

public interface ICommand<T>
    where T : ICommandResult
{
    int Id_User { get; }
}

public interface ICommandResult
{
    bool Success { get; }

    Exception Error { get; }

    ICollection<ValidationRule> BrokenRules { get; }
}

public interface ICommandHandler<TCommand, TResult>
    where TCommand : ICommand<TResult>
    where TResult : ICommandResult
{
    TResult Execute(TCommand command);
}

public interface ICommandObserver<TCommand, TResult>
    where TCommand : ICommand<TResult>
    where TResult : ICommandResult
{
    void Trigger(TCommand command, TResult result);
}

And here is the CommandBus implementation (using Structuremap as the DI)

public class CommandBus : ICommandBus
{
    private readonly IContainer m_container;

    public CommandBus(IContainer container)
    {
        m_container = container;
    }

    public TResult Publish<TCommand, TResult>(TCommand command)
        where TResult : ICommandResult
        where TCommand : ICommand<TResult>
    {
        using (MiniProfiler.Current.Step("CommandBus.Publish"))
        {
            var handler = m_container.GetInstance<ICommandHandler<TCommand, TResult>>();
            var observers = m_container.GetAllInstances<ICommandObserver<TCommand, TResult>>();

            var result = handler.Execute(command);

            foreach (var observer in observers)
                observer.Trigger(command, result);

            return result;
        }
    }
}

How to use:

public class CreateItemCommand: ICommand<CreateItemResult>
{
    public string Name { get; private set; }
}
public class CreateItemResult: ICommandResult
{
    public bool Success { get; private set; }
}

var command = new CreateItemCommand();
var result = commandBus.Publish<CreateItemCommand, CreateItemResult>(command);

What do you think about this pattern? Do you think it will be easy to upgrade? I'm afraid it can be an obvious bottleneck in my application...

Edit: I just want to past a link to my question about this pattern, in order to simplify all the Generics...

This is insanely nit-picky, but this is really just driving me nuts.

TResult Publish<TCommand, TResult>(TCommand command)

We read left to right from the return type to the name to the parameters, but you're ordered your generic types from param to result. I think this would be much easier to read as

TResult Publish<TResult, TCommand>(TCommand command)

It keeps the declaration closer to its use. Which is, admittedly, trivial in this case, but I think it makes the signature read more like a sentence.

On another note though, I really like that you're filtering the acceptable types. It reminds me that I need to do this more often myself. It's better than just trusting that you (or someone else most likely) knows what kind of types are okay to use generically.


Shorter, better way to write a "query" involving virtual attributes I have this code in a model for an invoicing app. This virtual attribute current_invoice gets the only non-rescinded invoice associated with a contract. The code works but it seems really verbose. Can this be done in a prettier way?

  def current_invoice
    result = []
    invoices.each do |i|
      if i.current_status != "rescinded"
        result.push(i)
      else
        false
      end
    end
    if result.length > 1
      raise "Only one non-rescinded invoice association per contract"
    else
      result[0]
    end
  end

You might be wondering why not just do this...

invoices.where.not(current_status: "rescinded").first

...but current_status is a virtual attribute on invoice.

Some notes:

result = []: Try not to use generic names like result. Also, use plural names for collections.
init empty array + each + conditional push = select/reject. More on functional programming.
else false. In an each block the value is ignored, so this does nothing.
result[0]. People usually write result.first, but [0] is also ok.
if result.length > 1. There is a serious conceptual problem here, validations should be enforced somewhere else (this is Rails, so using a validation callback).

I'd write:

def current_invoice
  non_rescinded_invoices = invoices.select { |i| i.current_status != "rescinded" }
  if non_rescinded_invoices.size > 1 # but this should be probably an AR validation
    raise("Only one non-rescinded invoice association per contract")
  else
    non_rescinded_invoices.first
  end
end


Defer pattern for constructors in C++ In some specific scenarios, I want to run code after the constructor for a class has run (namely: access std::enable_shared_from_this::shared_from_this()).

To solve this without introducing an error prone init method for all classes with this behaviour I have built a generic pattern that allows constructors to "defer" lambdas to run after the constructor has run (example in end of snippet):

// If derived from this subclasses may call enable_defer::defer in their
// constructor to run code directly _after_ their constructor has completed
// This is useful when for example the constructor wants to access the weak_ptr
// from std::enable_shared_from_this in the constructor.
// Note naming convention matching std::enable_shared_from_this & underscores to avoid collisions in subclasses
class enable_defer
{
    // Allowed factories
    template <typename T>
    friend std::shared_ptr<T> make_shared_deferrable();

    template <typename T, typename... Args>
    friend std::shared_ptr<T> make_shared_deferrable(Args&&... args);

    // Allowed implementations
    template <typename T>
    friend class __impl__;

private:
    std::vector<std::function<void()>> defered;
    bool constructed = false;

    // Only friend classes may have access to this type
    // this is because we only want friend classes to be able to implement the interface
    class __Tag__ {};

    virtual void __constructed__(__Tag__ = __Tag__()) = 0;

    // Implementation of enable_defer kept private
    // to make sure only friend factories above may
    // construct implementations
    template <typename T>
    class __impl__ : public T
    {
        static_assert(std::is_base_of<enable_defer, T>::value, "Must be enable_defer");

        // Forward base class constructors
        using T::T;

        virtual void __constructed__(__Tag__) override
        {
            constructed = true;
            for (auto fn : defered)
                fn();
        }
    };

protected:

    void defer(std::function<void()> fn)
    {
        // Make sure defer is only called in constructor
        assert(!constructed);
        defered.push_back(fn);
    }
};

// Create std::shared_ptr from enable_defer
template <typename T>
std::shared_ptr<T> make_shared_deferrable()
{
    auto shared(std::shared_ptr<T>(new enable_defer::__impl__<T>()));
    shared->__constructed__();
    return shared;
}

// Create std::shared_ptr from enable_defer
template <typename T, typename... Args>
std::shared_ptr<T> make_shared_deferrable(Args&&... args)
{
    auto shared(std::shared_ptr<T>(new enable_defer::__impl__<T>(std::forward<Args>(args)...)));
    shared->__constructed__();
    return shared;
}

class Example : public enable_defer, public std::enable_shared_from_this<Example>
{
public:

    Example()
    {
        defer([this]() {
            shared_from_this(); // Works!
        });
    }

    // Factory
    static std::shared_ptr<Example> create()
    {
        return make_shared_deferrable<Example>();
    }
};

Questions:

Are there better ways to accomplish the enforcements of the implementation class and factories?
General improvements
Bugs?
Naming?
General thoughts on this pattern.

Identifiers with leading underscore(s) followed by a capital letter are reserved for the implementation and should not be used in your own code. It's easiest to just avoid leading underscores.
When using enable_shared_from_this it's important to use a static factory function and make the other constructors private. This prevents calling the Example constructor directly (where calling shared_from_this will be undefined behaviour, since we don't have a shared_ptr).

Do we really need enable_shared_from_this? The whole thing could be more easily written as:

    class Example
    {
    private:

        Example() { }

    public:

        static std::shared_ptr<Example> Create()
        {
            auto example = std::make_shared<Example>();
            example->do_the_thing();

            return example;
        }
    };

It really depends on what do_the_thing() is...

If we have to register the class somewhere after creation (or consistently perform some action), perhaps that logic doesn't belong in the Example class at all.

struct Example {};

struct Registry
{
    template<class T, class... Args>
    std::shared_ptr<T> CreateAndRegister(Args&&... args)
    {
        auto object = std::make_shared<T>(std::forward<Args>(args)...);
        Register(object);
        return object;
    }
};

auto registry = Registry();
auto e = registry.CreateAndRegister<Example>();

While an object has control over it's own copy / move semantics, it generally shouldn't care who / what owns it.

So the question is: why does Example need to "break the 4th wall" and know that it exists inside a shared_ptr?

Large inheritance hierarchies are out of fashion nowadays (with good reason), but with a general solution, you may need to cope with:

struct ExampleA : enable_defer, enable_shared_from_this<ExampleA> {};
struct ExampleB : ExampleA {}; // what if B wants to defer something?

and

struct ExampleA : enable_defer, enable_shared_from_this<ExampleA> {};
struct ExampleB : enable_defer, enable_shared_from_this<ExampleB> {};
struct ExampleC : ExampleA, ExampleB {}; // uh oh... ?

shared_from_this is quite complicated to use as it is with a class hierarchy.


Short Eratosthenes Sieve I've written a short implementation of the Eratosthenes' sieve.

def sieve(n):
    l = list(range(2, n + 1))
    out = []
    while len(l) > 0:
        out.append(l[0])
        l = list(filter(lambda n: n % l[0], l))
    return out

Is it lacking in efficiency? Are there any improvements or changes you would make?

Yes, the implementation is short.  However, its efficiency is suspect.  That's due to:

Reassembling list every iteration
Testing every remaining member of list using % operator

The beauty of Eratosthenes' sieve is that it shouldn't need any division (including %).  With an array of values, we simply march along it in steps of n striking out members - that requires only addition, which is one of the cheapest operations in a digital computer.  (There's an optimisation that also requires multiplying a number by itself; that's only used because it's faster than the equivalent additions).

Removing the empty cells at every iteration prevents us from accessing every nth element in this way, as well as imposing its own overheads (for a start, we'll need lots of extra memory when building the new list from the old).


Converting a string to various number formats I'm working on a set of PostgreSQL helper classes, and am representing each PostgreSQL data type as objects. Instead of creating a class for each number format, I've created a template so the user can create a Number<int>, Number<double> etc as necessary, which is awesome as it saves me lots of coding!

The problem I'm facing is I have the need to convert a string representing the value of the number (which is returned from libpq) to its relevant format (the fromString method).

It's kinda messy.  Is there a neater solution than a lot of if / else's?
I also have the problem that if the string doesn't contain type T, it causes an exception, so, to be safe, I'd have to wrap everything inside a try/catch making it even more messier! So, is there a way around doing this?

template <typename T>
class Number : public spg::datatypes::BaseDataType
{
public:
    Number() : BaseDataType(false) { checkTIsNumeric(); }
    Number(T val) : BaseDataType(false), val(val) { checkTIsNumeric(); }
    Number(const std::string& s) : BaseDataType(false) { checkTIsNumeric(); fromString(s); }

    std::string toString() const { return std::to_string(val); }
    int getParamCount() { return 1; }

    T val;

protected:
    void parseString(std::vector<std::string>& splitted)
    {
        // see http://www.cplusplus.com/reference/string/

        size_t hc = typeid(T).hash_code();

        // TODO maybe these should be wrapped inside try/catch because if string doesn't contain T, it will throw
        if (hc == typeid(int).hash_code())
            val = std::stoi(splitted[0]);
        else if (hc == typeid(double).hash_code())
            val = std::stod(splitted[0]);
        else if (hc == typeid(long double).hash_code())
            val = std::stold(splitted[0]);
        else if (hc == typeid(float).hash_code())
            val = std::stof(splitted[0]);
        else if (hc == typeid(long).hash_code())
            val = std::stol(splitted[0]);
        else if (hc == typeid(unsigned long).hash_code())
            val = std::stoul(splitted[0]);
        else if (hc == typeid(long long).hash_code())
            val = std::stoll(splitted[0]);
        else if (hc == typeid(unsigned long long).hash_code())
            val = std::stoull(splitted[0]);
    }

private:
    void checkTIsNumeric()
    {
        if (!std::is_arithmetic<T>::value || std::is_class<T>::value)
            throw "T is not numeric";
    }

};

I'm not sure if the checkTIsNumeric is necessary, as compilation will fail if a non numeric type is passed as T because std::to_string doesn't have a suitable overload.

I did a few tests using the giant if / else above, and using a std::stringstream. The latter was noticeably slower. E.g. in one benchmark, 14.0781s for the above code vs 19.6901s using std::stringstream.

First things first: Export all the things which are different for different types into a policy-class, like std::basic_string and many standard-library classes do.
The main advantage is being actually able to have types with different interfaces without running into compilation-errors or at least warnings:

template<class T>
struct standard_number_traits {
    static T parse(const std::string& in) {
        T out;
        std::stringstream{in} >> out;
        return out;
    }
    using std::to_string;
};

Add specializations as needed for efficiency / versatility / correctness / fun.
Eliminate runtime-checks for having selected an acceptable type, powered by the prior change.
That also allows you to use the same basic C++ type with completely different IO-behavior:

template <class T, class traits_type = standard_number_traits<T>>
struct Number : spg::datatypes::BaseDataType
{
    Number(T val = T{}) : val(val) {}
    // Or is BaseDataType(false) not default-init?
    Number(const std::string& s) : Number(traits_type::parse(s)) {}

    std::string toString() const { return traits_type::to_string(val); }
    int getParamCount() { return 1; }

    T val; // Shall that really be public? Well, might be ok.
};

Profit?


Dynamic Programming for printing additive numbers up to digits n Edit: I am hoping to get some review / make sure I am understanding dynamic programming correctly.

I am trying to print out all additive numbers up to digits n using dynamic programming.  Additive numbers are those like 123, 1235, etc., where the sum of every 2 digits from left to right is equal to the third digit.  In this definition, non-trivial additive numbers must necessarily be at least 3 digits long, though for numbers of 2 digits or less one could trivially print out all digits 0-99.  Furthermore, this definition implies the set of additive numbers is finite and relatively tiny.

If there is a better definition of an additive number or I have misunderstood it, feel free to point out.

I believe dynamic programming is a good approach to this problem, because the solutions of n - 1 need to be re-used to compute the solutions to n.  A brute force algorithm is possible, but I think far more inefficient.

Here is my solution in Python.  It will print them all out and also return the trellis that contains the solutions for each digit.  Note that for n >= 9, there are no more additive numbers.

# -*- coding: utf-8 -*-

"""Dynamic programming
"""

BASE_CASE = 3

def print_additive_numbers(n=BASE_CASE):
    """Prints all additive numbers up to n digits

    Additive numbers are numbers of the form 123, 1235, etc.
    where the sum of every 2 digits is equal to the third digit
    in the digit expansion of the number.

    We use dynamic programming to iteratively generate
    additive numbers for increasing digits, as the solutions to
    n = 3 are re-used for n = 4, n = 5, etc.

    Args:
        n: the maximum number of digits for each representation

    Returns:
        A dictionary mapping each number of digits to all
        possible additive numbers.

    """
    if n < BASE_CASE:
        raise ValueError, "additive numbers always have 3 or more digits"
    #build the initial trellis
    trellis = {}
    trellis[BASE_CASE] = {}
    for i in xrange(1, 9):
        row = []
        for j in xrange(0, 9):
            if i + j <= 9:
                print str(i) + str(j) + str(i + j)
                row.append(str(j) + str(i + j))
        trellis[BASE_CASE][i] = row
    for m in xrange(BASE_CASE + 1, n + 1):
        trellis[m] = {}
        for key in trellis[m - 1].keys():
            row = []
            for digits in trellis[m - 1][key]:
                first_digit = int(digits[-2])
                second_digit = int(digits[-1])
                new_digit = first_digit + second_digit
                if new_digit <= 9:
                    row.append(digits + str(new_digit))
                    print str(key) + digits + str(new_digit)
            trellis[m][key] = row
    return trellis

t = print_additive_numbers(9)

1. Review

I like that you've written a docstring, but it is not quite correct. It claims that the function returns:

A dictionary mapping each number of digits to all
possible additive numbers.

but when I run it I get a more complicated data structure:

>>> from pprint import pprint
>>> pprint(additive_numbers(3))
{3: {1: ['01', '12', '23', '34', '45', '56', '67', '78', '89'],
     2: ['02', '13', '24', '35', '46', '57', '68', '79'],
     3: ['03', '14', '25', '36', '47', '58', '69'],
     4: ['04', '15', '26', '37', '48', '59'],
     5: ['05', '16', '27', '38', '49'],
     6: ['06', '17', '28', '39'],
     7: ['07', '18', '29'],
     8: ['08', '19']}}

Either the docstring or the result should be changed so that they match.
The code is not portable to Python 3 due to the use of the raise and print statements and the xrange function. It would be easy to make it portable, using range instead of xrange, the print function instead of the statement, and the modern form of raise:

raise ValueError("additive numbers always have 3 or more digits")

The function print_additive_numbers does three things: (i) it computes the numbers, (ii) it prints them, and (iii) it returns an internal data structure used in the computation. It's generally good practice for a function to have a single purpose, and then compose or combine functions to make a program. So I would redesign the function so that it does task (i); we can call print on the result if we need (ii). I don't think (iii) is necessary at all.
The code stores the last two digits of each additive number in the trellis structure in the form of a string. But this means that when it gets the digits out again it has to call int. This conversion back and forth is wasteful: it would be better to store the tuple (j, i + j) instead of the string str(j) + str(i + j).
The initial loop looks like this:

for i in xrange(1, 9):

but Python ranges are exclusive at the top, so this never generates the digit 9 and thus fails to find the additive numbers 909 and 9099. The initial loop needs to say range(1, 10).
The initial two loops look like this after correcting the problems noted above:

trellis[BASE_CASE] = {}
for i in range(1, 10):
    row = []
    for j in range(0, 10):
        if i + j <= 9:
            row.append((j, i + j))
    trellis[BASE_CASE][i] = row

but the if could be omitted if we changed the bounds on j, like this:

trellis[BASE_CASE] = {}
for i in range(1, 10):
    row = []
    for j in range(10 - i):
        row.append((j, i + j))
    trellis[BASE_CASE][i] = row

or, using a list comprehension:

trellis[BASE_CASE] = {}
for i in range(1, 10):
    trellis[BASE_CASE][i] = [(j, i + j) for j in range(10 - i)]

2. A better algorithm

This is a problem for which dynamic programming is unnecessary! That's because dynamic programming is a technique that you use when you have to consult the answers to multiple overlapping subproblems in order to answer the current problem. But when generating additive numbers, there isn't really a problem to solve. Once you've chosen the first two digits, all the other digits follow without any choice.

So I would implement it like this:

def additive_numbers(n):
    """Generate additive numbers up to n digits, in numerical order.

    Additive numbers are numbers of the form 123, 1235, etc. where the
    sum of every 2 digits is equal to the third digit in the digit
    expansion of the number.

    >>> list(additive_numbers(3))
    ... # doctest: +NORMALIZE_WHITESPACE
    [101, 112, 123, 134, 145, 156, 167, 178, 189, 202, 213, 224, 235,
     246, 257, 268, 279, 303, 314, 325, 336, 347, 358, 369, 404, 415,
     426, 437, 448, 459, 505, 516, 527, 538, 549, 606, 617, 628, 639,
     707, 718, 729, 808, 819, 909]

    The longest additive number is 8 digits long:

    >>> list(additive_numbers(8)) == list(additive_numbers(9))
    True
    >>> max(additive_numbers(8))
    10112358

    """
    for digits in range(3, n + 1):
        # Choose the first two digits.
        for i in range(1, 10):
            for j in range(10 - i):
                # m is the number generated so far, z is its last
                # digit, and y the penultimate digit.
                m, y, z = i * 10 + j, i, j
                for _ in range(digits - 2):
                    y, z = z, y + z
                    if z > 9:
                        break
                    m = m * 10 + z
                else:
                    yield m

The examples in the docstring can be executed using the doctest module.


Production code to be good for unit testing I would like to advise if the following production code is good to be unit tested. The segment of  the production code gets data from a backend service and displays it on the page. It is a class:

...
// Extract this code into a separate method because it is called in more than 1 place
Keywords.prototype.addKeyword = function(keyword) {
    $("ul").append("<li>" + keyword + "</li>");
}

// This is the method that I want to test
Keywords.prototype.getKeywords = function(forGroup) {
    // AppDao contains a bunch of methods that get data
    var keywords = AppDao.getKeywords(forGroup),
        that = this;

    $.each(keywords, function(i, keyword) {
        that.addKeyword(keyword);
    };
}
...

I'm planning to test it in the following way:

Stub Keywords.prototype.addKeyword() and make sure that it will be called with 3 arguments: "KW A", "KW B", and "KW C"
Stub AppDao.getKeywords() and make sure that it will be called with "Test Group" and will return 3 keywords: "KW A", "KW B", and "KW C"
Invoke Keywords.prototype.getKeywords() and verify the stubs

I didn't start the actual implementation of the test yet because I would like to advise with you guys if you think that something can be better done. Thanks.

You're off to a bad start.  You don't have a definition of what your getKeywords() method is supposed to do, which means you can't even begin to test it.  On top of that, it seems to be named backwards - getKeywords() calls addKeyword().


Ruby command to edit a field in a SAM file Inspired by a bioinformatics question on Stack Overflow, I wanted to brush up on my Ruby knowledge, since I’ve been meaning to replace my command-line use of Perl by Ruby.

Briefly, the problem description is as follows:

  Given is a SAM file. This is a whitespace-delimited file with at least 11 columns. After the 11th column, there can be further optional columns in a specific format1. If there is an optional column starting with RX:Z: then remove all dashes in that column.

Here’s my Ruby “one-liner”:

ruby -ane '
  $F[11..].grep(/^RX:Z:/).map! {|x| x.gsub!(/-/, "")}
  puts $F.join("\t")
' file.sam

This works but I’m wondering if there’s a way of making this more idiomatic and/or more concise. For instance, I’m puzzled by the apparent necessity to manually join the modified $F back together. There doesn’t seem to be a way of using the -p flag instead of -n, and getting the in-place changes to $F to propagate to $_. perl -ae has the same limitation as far as I know.

1 This isn’t relevant to the question but each optional column is roughly described by the regular expression [a-zA-Z][a-zA-Z0-9]:[AifZHB]:\S*. The actual description is more complicated (for instance, if the character between the two colons is Z then the field after the second colon is a zero-terminated C-string of printable characters, and notably can include space).

There isn't much to say here, it is clear what your solution is doing, and attempts to make it more concise would just make it harder to read.

I recommend replacing map! with each - map! indicates that you want the values returned by grep to be changed in place. Since that result is then discarded, it makes the code more difficult to understand.

Personally, I avoid ! methods whenever possible. The hidden mutation which can occur makes code harder to reason about. It is fine in this small example (especially once map! is replaced with each), but in a larger program it will hide bugs.

Avoiding mutation in this case makes the code a bit longer, but it has the benefit of not needing to stop and think about what has changed and if it matters. I spent several minutes trying to figure out why your code didn't fail when a field didn't include a - since gsub! returns nil when no replacements are made.

Though less concise, I prefer the following version:

ruby -ane '
$F[11..] = $F[11..].map { |x| x.start_with?("RX:Z:") ? x.gsub(/-/, "") : x }
puts $F.join "\t"
' file.sam

Not automatically joining $F back together to create $_ makes sense, what should ruby do if you use (or modify) $F but then set $_ to something else? Setting $_ whenever $F is modified would be rather confusing and bad for performance.


Fetch Keywords from comment and assign keyword id to comment I have a small callback function which receives a comment in array like below:

                        $data[$count] = array(
                            'comment_id' => $comment['id'],
                            'comment_body' => $comment['message'],
                            'comment_author_id' => $comment['from']['id'],
                            'comment_author_name' => $comment['from']['name'],
                            'active' => 1,
                            'keyword_id' => null,
                            'live_video_id' => $liveVideo->live_vidoe_id
                        );

the I send this comment to a function AssignKeywords as below:

$data = array_map([$this , 'assignKeywords'], $data);
Below is the definition of AssignKeywords

public function assignKeywords($comment)
{
    # code...
    $keywords = Keyword::where('live_video_id' , $comment['live_video_id'])->get();
    $commentBody = strtolower($comment['comment_body']);
    preg_match_all('/(?<!\w)#\w+/',$commentBody,$matches); //it returns an associative array with just 0 index and array of matches on this 0 index

    foreach($matches as $match) {   //only 1
        foreach ($match as $item) {    //max 10 if a user is mad :\
            foreach ($keywords as $keyword) {   //Max 4
                $keywordBody = strtolower($keyword['keyword_name']);
                if ($item === $keywordBody) {
                    if ($comment['keyword_id'] == NULL) { //to check if it already contains a value, if it contains we don't want to assign again
                        $comment['keyword_id'] = $keyword['id'];
                    }
                }
            }
        }
    }

    return $comment;
}<br>

I am looking for a better way to Assign Keywords and minimize foreach loops and maybe solve it by using functions? Any thoughts?

From performance point of view, 40 iterations is nothing.

However, given such a condition that your keywords are like the Highlanders, as there could be only one, the number of repetitions could be reduced as we won't need anymore after we get one.

foreach($matches as $match) {   //only 1
    foreach ($match as $item) {    //max 10 if a user is mad :\
        foreach ($keywords as $keyword) {   //Max 4
            $keywordBody = strtolower($keyword['keyword_name']);
            if ($item === $keywordBody) {
                    $comment['keyword_id'] = $keyword['id'];
                    break 3; // stop all loops
                }
            }
        }
    }
}


Any ideas how do I prettify this SQL query? This question is abut coding style. I'd like to see how would you improve this code look considering readability as well. Feel free to upvote others solutions you find good.
#1 Initial version:
SELECT greatest(
  0,
  least(
    500,
    (SELECT g.c_daily_limit * (
        SELECT count(*)
        FROM rivals.t_rival_group_auth a
        WHERE a.id_group = g.id) - (
        SELECT count(*)
        FROM rivals.t_order_results rs
        JOIN rivals.t_rivals r ON r.id = rs.id_rival
        WHERE r.id_group = g.id
          AND rs.c_date_order::date = utils.getdate()::date
          AND (rs.c_price IS NOT NULL OR rs.c_failed IS NOT NULL))
      FROM rivals.t_rival_group g
      WHERE g.c_code = _code)
  )
);

#2 An alternative version, should improve readability of nested parameters:
SELECT greatest(
  0,
  least(
    500,
    (
      SELECT g.c_daily_limit * (
        SELECT count(*)
        FROM rivals.t_rival_group_auth a
        WHERE a.id_group = g.id
      ) - (
        SELECT count(*)
        FROM rivals.t_order_results rs
        JOIN rivals.t_rivals r ON r.id = rs.id_rival
        WHERE r.id_group = g.id
          AND rs.c_date_order::date = utils.getDate()::date
          AND (rs.c_price IS NOT NULL OR rs.c_failed IS NOT NULL)
      )
      FROM rivals.t_rival_group g
      WHERE g.c_code = _code
    )
  )
);

The following are a few suggestions on how I'd write the SQL.
Source Control
If you don't already have a database project, create one in Visual Studio. Then check it in to source control. Microsoft Azure DevOps Services is free & private for teams of 5 or less (this is per project, so 5 developers per project). Then you'll be able to track changes you make to your stored procedures, views, tables, etc.
Formatting
I use the following tools for SSMS and Visual Studio, ApexSQL Refactor and Poor Man's T-Sql Formatter. I use it when I have to edit other developer's code. It's a great way to standardize your SQL. I find it does most of the formatting for me, but I'll still make a few changes after.
Copy & Paste
If you find yourself copying and pasting the same string or number over and over in your query, then you should define it as a variable. Copy and paste is a design error ~ David Parnas
Commas
I would put the commas in front to clearly define new columns. Versus code wrapped in multiple lines. It also makes trouble-shooting code easier.
Where Clause
If you put 1=1 at the top of a WHERE condition, it enables you to freely change the rest of the conditions when debugging a query. The SQL query engine will end up ignoring the 1=1 so it should have no performance impact. Reference
Common Table Expressions (CTE)
CTE's in your SQL help with documentation. The expression name can then let other developers know why you used that expression e.g. number_of_group_auth or number_of_order_results.
Schema Names
Always reference the schema when selecting an object e.g. [dbo].[Sales].

Also check out the book Clean Code. It will change the way you think about naming conventions.

Revised SQL
Without table definitions and sample records I was unable to test this, but it should give you a good start.
WITH
number_of_group_auth -- table expression names should tell you what they're doing so you don't need to use comments
AS
(
   SELECT
        [group_count] = COUNT(*)
      , [a].[id_group]
   FROM
      [rivals].[t_rival_group_auth] AS [a]
)
,
number_of_order_results
AS
(
   SELECT
        [group_count] = COUNT(*)
      , [r].[id_group]
   FROM
      [rivals].[t_order_results] AS [rs]
      INNER JOIN [rivals].[t_rivals] AS [r] ON [r].[id] = [rs].[id_rival]
   WHERE
      1=1
      AND [rs].[c_date_order]::date = utils.getdate() ::date
      AND
         (
            [rs].[c_price] IS NOT NULL
            OR [rs].[c_failed] IS NOT NULL
         )
)
SELECT
   greatest
   (0, least
      (500,
         (
            SELECT
                [g].[c_daily_limit] * [number_of_group_auth].[group_count] - [number_of_order_results].[group_count]
            FROM
                [rivals].[t_rival_group] AS [g]
                LEFT JOIN [number_of_group_auth] AS [a] ON [a].[id_group] = [g].[id]
                LEFT JOIN [number_of_order_results] AS [r] ON [r].[id_group] = [g].[id]
            WHERE
                [g].[c_code] = [_code]
         )
      )
   );


Too many objects, too "deep" class structure? I have a pretty large model library I'm working on and it's turning out quite complex, or so I believe at least and I could use a second opinion.

The basic idea is that the user should only have to create an instance of a big base class and then build on that with the other object types.

These are some of the properties in the base class:

public class ConfigurationSheet
{
    public List<MainStep> MainSteps { get; set; }
    public List<SequenceStep> SequenceSteps { get; set; }
    public List<SubSequenceStep> SubSequenceSteps { get; set; }
    public List<ConfigurationRow> ConfigurationRows { get; set; }

    public List<Tag> PLCTags
    {
        get { return ConfigurationRows.Select(x => x.PLCTag).ToList(); }
    }
}

These types are all mine and comes with the library, for example the  ConfigurationRow-object looks like this:

public class ConfigurationRow
{
    public Tag PLCTag { get; set; }
    public Dictionary<int, StepArguments> Arguments { get; set; }
}

And it continues like this.  Here's the StepArguments object:

public class StepArguments
{
    public Configurations Configurations { get; set; }
    public Calculations Calculations { get; set; }
}

The Configurations object actually goes another two steps deeper.

This could, potentially, result in code like this:

if(sheet.SequenceStep[0].DefaultConfigurations.Tolerances.SetPoint > 0)
    sheet.ConfigurationRows[1].Arguments[120].Configurations.Tolerances.HighLimit = 200;

And this doesn't seem right to me.  Should I perhaps break some of my objects and put the properties in bigger classes, or should I create shortcut properties/methods/indexers or am I maybe just overreacting?

I'm thinking maybe something like this:

public class StepArguments
{
    public Configurations Configurations { get; set; }
    public Calculations Calculations { get; set; }
    public double HighLimit { get { return Configurations.Tolerances.HighLimit; } }
}

Which would allow this:

if(sheet.SequenceStep[0].SetPoint > 0)
    sheet.ConfigurationRows[1].Arguments[120].HighLimit = 200;

While this would make clearer code it would also ruin the purpose of having the Configurations class in the first place.

I think you should definitely keep class structure as close as possible to actual data structure. So, for example, if your MainStep "contains" SequenceStep, you should reflect that in your code by using aggregation:

class MainStep
{
    public List<SequenceStep> SequenceSteps { get; set; }
    .....
}

etc...

If you want an easy access to some parts of your data, you should implement additional methods on highest level. For example, to iterate through SequenceSteps, you can implement following extension method:

public static IEnumerable<SequenceStep> GetSequenceSteps(this ConfigurationSheet sheet)
{
    foreach(var main in sheet.MainSteps)
    {
        foreach(var sub in main.SequenceSteps)
        {
            yield return sub;
        }
    }
}

(or you can use Linq).

I think the line sheet.ConfigurationRows[1].Arguments[120].Configurations.Tolerances.HighLimit is fine, as long as it reflects the actual data structure. Its definetely not the most complex thing in the world, far from it (mappings for complex databases often have way more layers of aggregation :) ). There is not much that can be done.

If you have control over your data, you can try to change its format to simplier one.
You can also try to move the logic to lower level, where possible. So it will look like row.Arguments[120].Configurations.Tolerances.HighLimit instead. Its hard to tell an exact refactoring without seeing the bigger picture. Using "shortcuts" for commonly accessed fields is fine as well, as long as you don't overdo it. Use extension methods for that where possible, so that class structure remains clear.
You can use shorter naming. Do you really need to have ConfigurationSheet.ConfigurationRow, for example? Isn't ConfigurationSheet.Row just as readable?
Also if Arguments is an equivalent of individual "cells", then using indexers makes sense: sheet[1][120]


Function with excessively complex "where" clause I'm writing a program to differentiate functions (perform basic calculus) in Haskell. The program works, but there's one function that looks very ugly to me:

--some prerequisite definitions

--a mathematical value is either...
data Value =
    Literal Double                     --a number, such as 4, -17.5, or 2.33e27
    | Variable Symbol (Maybe Double)   --a variable, which may or may not have a known value
    | FCall                            --a function call, consisting of a function and two arguments
      {
          func :: Function,
          arg1 :: Value,
          arg2 :: Value
      }
      deriving (Eq)

--simplifies a mathematical value into a simpler form.

reduce :: Value -> Value
reduce val@(Literal _)          = val               --literals cannot be simplified
reduce (Variable _ (Just val))  = Literal val       --bound variables can be simplified to literals
reduce var@(Variable _ Nothing) = var               --unbound variables cannot be simplified
reduce fc@(FCall f a b)         =                   --simplifying functions is messy...
   if isDefined f ma mb
        then matchLiterals ma mb
        else retval
    where ra = reduce a  --reduce simplifies a Value to a Value
          rb = reduce b
          ma = eval ra   --eval converts known values to Doubles and unknown Values to Nothing
          mb = eval rb
          retval = FCall f ra rb  --returned in case of failure
          matchLiterals (Just da) (Just db) = Literal $ (impl f) da db
          matchLiterals _ _ = matchCase $ findCase (cases f) ra rb
          matchCase (Just sc) = evalCase sc ra rb
          matchCase Nothing = retval

The multitude of local functions defined in the where clause seems excessive, but I can't find a better way to handle the flow of execution. I've tried to break this up into several smaller functions, but since most of these can fail (evaluate to Nothing), I still have to use pattern matching to handle the results of these functions.

The flow of logic goes like this:

if the function is defined for the given arguments:
    if the arguments have definite known values:
        evaluate and return the function call
    else:
        look for special cases (adding 0, multiplying/diving by 1, etc.)
        if a special case was found:
            evaluate and return the result of applying the special case to the arguments
        else:
            return another FCall with possibly simplified arguments

else:
    return another FCall with possibly simplified arguments

It's possible I'm fretting over nothing (hah) and this code is perfectly fine, but it seems that this is not the optimal way to handle these conditions. Can anyone offer any advice about how I might better structure this code?

It is a matter of taste but I think reduce will be a bit more cleaner with case.

reduce exp = case exp of
  Literal{}             -> exp
  Variable _ (Just var) -> Literal var
  Variable _ Nothing    -> exp
  FCall f a b
    | isDefined f ma mb -> matchLiterals ma mb
    | otherwise         -> FCall f ra rb
    where ra = reduce a
          rb = reduce b
          ma = eval ra
          mb = eval rb

Literal{} is special pattern syntax that will still work even if you add more arguments to Literal.

To reduce duplication (just a bit) in where, you can use on function from Data.Function

on :: (b -> b -> c) -> (a -> b) -> a -> a -> c

it is like (.) but allows to apply function to both arguments

(ra,rb) = on (,) reduce a b
(ma,mb) = on (,) eval ra rb