Dividing the screen in two halves using LinearLayout I want to divide the screen in two halves. The layout is locked on portrait mode. The top half will have one button and the bottom half will have many buttons. That's why I choose this division, since the top half can be potentially scrollable.

This is my layout xml:

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:gravity="center"
    android:orientation="vertical" >

    <LinearLayout
        android:gravity="center"
        android:layout_width="match_parent"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:orientation="horizontal" >

        <Button
            android:id="@+id/play_button"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:text="@string/play_button" />

    </LinearLayout>

    <LinearLayout
        android:gravity="center"
        android:layout_width="match_parent"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:orientation="horizontal" >

        <Button
            android:id="@+id/button_1"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:text="button_1" />

        <Button
            android:id="@+id/button_2"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:text="button_2" />

        <Button
            android:id="@+id/button_3"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:text="button_3" />

    </LinearLayout>

</LinearLayout>

And this is how it looks like:

It looks as expected, but I was wondering if I'm doing it correctly. Is there a better, easier, more common, different, (etc) way to achieve this? Are there any mistakes or anti-patterns or bad things in there?

Your layout looks very good. android:layout_weight is the correct way to solve this.
A couple of other notes regarding your XML layout:

As your outermost LinearLayout has match_parent for both width and height, there is no need for android:gravity="center".

I would write 0dp instead of 0dip (even though it is the exact same thing). Using dp just seems more common to me. This is a very minor nitpick though :)

Don't hardcode strings in your XML. Use the strings.xml resource file and use android:text="@string/button3text"

You might know about this but either way you (and any future readers of this post) should also be aware that there exists android:layout_gravity. Although they sound similar, they are not the same thing. Read more about the differences between them in Android - gravity and layout_gravity

If, in the future you want to play around a bit more with layout_weights, you should take a look at What is android:weightSum in android, and how does it work?, especially this answer

If you would like to support landscape mode as well, I suggest learning how to use Android Fragments. If you would like to support older devices, there's a support package that can help with that. A Fragment has one onCreate method and one onCreateView method, which makes it easy to persist data between screen orientation changes (or other changes) while creating a new view for the data.

Summary:
Your layout looks excellent. At least in my eyes.


Using setInterval() to make data reactive I have a video component passing data from an async GET request to a child component:
// Inside a v-for loop
<video
    :title="video.title"
    :id="video._uid"
>
// ...
<Controls :videoData="video" />

When receiving the prop in Controls I then use that data to select the video by it's id in the DOM and set data properties according to that element:
created() {
    setTimeout(() => {
        this.video = document.getElementById(this.videoData._uid);
        this.videoDuration = this.video.duration;

        setInterval(() => {
            this.videoCurrentTime = this.video.currentTime;
            this.videoPlaying = !this.video.paused;
            this.videoVolume = this.video.volume;
        }, 1);
    }, 1000);
}

The setTimeout is to get around the data being asynchronous and the setInterval is to continuously update the data, such as the current time of the video, whether it's paused or not, etc.
It works exactly as it should, but it seems really inefficient. What if someone has slow internet and it takes longer than 1000ms to fetch the data, for example.
I would think the fact it's in a v-for loop would prevent the need for setTimeout as the component wouldn't exist before the video has been fetched. I must be missing something.
Request:
created() {
    this.fetchVersion().then(async () => {
        await Axios.get(
            `https://api.storyblok.com/v1/cdn/stories/videos?version=published&token=<STORYBLOK_TOKEN>&cv=${this.getVersion}`
        ).then(res => {
            res.data.story.content.body.forEach(i => {
                this.storyblokVideos.push(i);
            });
        });
    });
}

Does anyone have a better way of making the data reactive?
UPDATE
Thanks to @potato's answer:
I set a data property called isLoading: true then used the video's event oncanplay to conditionally load the controls: @canplay="isLoading = false". No more async errors. Then I used the different events to pass data:
@play="playing = true"
@pause="playing = false"
@timeupdate="updateCurrentTime()"
@volumechange="updateVolume()"

Much more efficient.

Use events and callbacks to react to things as they happen instead of repeatedly checking for changes.
You can use the .then function to do whatever needs to be done once the data is received.
You can learn about the <video> element events here: https://www.w3schools.com/tags/ref_av_dom.asp


java.util.Observable but with generics to avoid casts This is an Observable class similar to java.util.Observable. The difference is that it uses generics to avoid casts. The question is: Is it really worth the effort? What are the problems with this code?

import java.util.ArrayList;
import java.util.Collection;

/**
 * like java.util.Observable, But uses generics to avoid need for a cast.
 *
 * For any un-documented variable, parameter or method, see java.util.Observable
 */
public class Observable<T>  {
   private boolean changed = false;

   // Holds registered observers.
   // @TODO if class is not final can we make this protected? (should we?).
   private final Collection<Observer<? super T>> observers;

   /**
    * observersSet gives the opportunity of changing the implementing set. The
    * benefit is, final user can specify the 'order' of observers notified.
    * Generally, not a good idea at all.
    *
    * @TODO should this be private and the class final?.
    */
   protected Observable(Collection<Observer<? super T>> observersSet) {
      this.observers = observersSet; }

   public static <F> Observable<F>getInstance() {
      // return getInstance(container);
      ArrayList<Observer<? super F>> container = new ArrayList<>();
      return new Observable<>(container);
   }

   /**
    * Before this method can be actually used, this.update(T arg) must be fixed.
    * It has a hard-coded ArrayList. Generally, not a good idea, not at all.
    *
    * User would expect the call getInstance(ArrayList<SomeObserver> ar) to
    * work however it wont. It must be:
    * getInstance(ArrayList<? extends SomeObserver> ar);
    *
    * It's bad, ugly, and more importantly, violating.
    *
    * Unless of course the first example could be made to work. Maybe a cast
    * will do.
    */
   @SuppressWarnings("UnusedParameters")
   public static <F> Observable<F>
      getInstance(Collection<Observer<? super F>> observersSet) {
      throw new UnsupportedOperationException("Not yet implemented.");
      // return new Observable<>(observersSet);
   }

   // @TODO null check maybe for add?
   public synchronized boolean add(final Observer<? super T> o) {
      return !this.observers.contains(o) && observers.add(o);

   }
   public void addObserver(final Observer<? super T> o)         { this.add(o); }

   public synchronized boolean rm(final Observer<? super T> o)  { return observers.remove(o); }
   public void deleteObserver(final Observer<? super T> o)      { this.rm(o); }

   public synchronized void clear() { this.observers.clear(); }
   public void deleteObservers()    { this.clear(); }
   // @TODO all of them need synchronized?
   public synchronized void setFlag() { this.changed = true; }
   public synchronized void clrFlag() { this.changed = false; }

   // Hei! did you know if you used projectLombok.org you could remove this
   // method? didn't you?!!! :O 0_o
   public synchronized boolean getFlag() { return this.changed; }
   public boolean hasChanged() { return this.getFlag(); }
   public int countObservers() { return this.observers.size(); }

   public void update(final T args) {
      ArrayList<Observer<? super T>> copy;

      synchronized (this) {
     if(!changed) return;
     copy = new ArrayList<>(this.observers);
     this.clrFlag();
      }
      // Look at OpenJDK to see why a new Collection is used.
      for(Observer<? super T> o : copy) o.update(args);
   }
   public void notifyObservers(final T args) { this.update(args); }

   public void update() { this.update(null); }
   public void notifyObservers() { this.update(); }
}

Observable now carries type of args sent to each Observer, and Observer is also aware of it, thus no cast needed.

The follow up question is located here.

Pick a convention for your curly brackets/indentation and stick with it. You code has all of the following:

Brackets included, one-lined statement
Brackets omitted, one-lined statement
New line after opening bracket, closing bracket on same line.
New line after opening bracket, closing bracket on different line.
Un-indented code inside brackets.
Padding spaces to vertically aligning opening brackets (with multiple different vertical lines being padded to).
Single space padding opening brackets.
Blank line before closing bracket.

The Java convention is:

public synchronized boolean add(final Observer<? super T> o) {
   return !this.observers.contains(o) && observers.add(o);
}

It is best to always include curly brackets even when they could be omitted. If you start with an if that does not include brackets and then need to add a second line, forgetting to would have the second line execute unconditionally.

It is safer to synchronize on a private instance variable than on the class instance. You have no control over what someone does with the instance of your class. Maybe they think it would be a good idea to use it to synchronize some other code. Now you have two different pieces of code using the same lock instance that might be result in a deadlock because they are not coordinated properly. A private lock variable guarantees that this can't happen.

You have to synchronize all accesses to observers. countObservers() could be called while you are adding or removing an observer.

Don't create aliases for your methods. It will only lead to confusion if there are multiple ways to do the same things.

observersSet is not a Set Don't make the argument name appear as if it is expecting one. If you do want it to be a set (you might since you are checking for duplicates in add()), then change the type from Collection to Set. If you need observers to be called in the order in which they were added, LinkedHashSet satisfies this requirement. If you accept any Collection when creating the instance, there is nothing to prevent the collection from already containing a duplicate.


Linked list loop detection in Java A schoolmate is going through the interview process, and was given the question of detecting if a linked list data structure has an infinite loop in it. I wrote this example, and wanted to see what experience programmers thought of it.

/*
* detecting loops in a linked list, java
*
* This linked list doesn't have a payload, but that is irrelevant to
* this problem, although might be useful for debugging.
*
*/

import java.util.*;

class Node
{
  Node n = null;
  Node()
  {
  }
  Node(Node n)
  {
    setNext(n);
  }
  void setNext(Node n)
  {
    this.n = n;
  }
  boolean hasNext()
  {
    return n!=null;
  }
  Node next()
  {
    return n;
  }
  boolean isLoopy(Set<Node> visited)
  {
    if(visited.contains(this))
    {
      return true;
    }else
    {
      if(hasNext())
      {
        visited.add(this);
        return next().isLoopy(visited);
      }else
      {
        return false;
      }
    }
  }
  boolean isLoopy()
  {
    return isLoopy(new HashSet<Node>());
  }
  // test harness
  public static void main(String[] args)
  {
    Node n1 = new Node();
    Node n2 = new Node(n1);
    n1.setNext(n2);

    System.out.println("loopy list is loopy:");
    System.out.println(n1.isLoopy());

    Node n4 = new Node();
    Node n3 = new Node(n4);

    System.out.println("non loopy list is loopy:");
    System.out.println(n3.isLoopy());
  }
}

This is a classic interview question.  At this point, I think it measures more whether you've been to many interviews than whether you can think creatively algorithmically while on a job interview.  Anyway,

public class Node {
  public Node next = null;
  public Node() {}
  public Node(Node n) { next = n; }
  public boolean hasNext() { return next != null; }
  public boolean hasLoop() {
    Node n = this, n2 = this;
    while (n.hasNext()) {
      n = n.next;
      if (n == n2) return true;
      if (!n.hasNext()) return false;
      n = n.next;
      if (n == n2) return true;
      n2 = n2.next;
    }
    return false;
  }
}

(Not compiled, but you get the idea.  I think getters/setters for next is overkill, given that the whole point is to manipulate the field at a low level, given this data structure.)

This uses constant space; since n travels twice as fast as n2, if there is a loop, both n and n2 will get caught at it and n will overtake n2 from behind (after a maximum number of steps equal to twice (lead-in plus loop length)).  Otherwise, n will reach the end and we're done.


Map value to color rgba I wrote this function to map value to a color

I am looking for some general feedback on how I can improve the efficiency of the function. and if there is better way to do it.

function getColor($value, $values_range, $opacity = '0.7') {

       $deltaS = ($values_range['max'] - $values_range['min']) / 2;
       $ds = ($value - $values_range['min']) / $deltaS;
       $nearest_color_index = floor($ds);
       $alpha = $ds - $nearest_color_index;

      $r = $g = $b = -1;
      $colors = [['r'=>255,'g' => 60,'b' => 40],
                 ['r' => 255,'g' => 247,'b' =>40],
                 ['r' => 12,'g' => 197,'b' => 17]];

      if ($nearest_color_index != 2) {
             $r = round($colors[$nearest_color_index]['r'] + $alpha * ($colors[$nearest_color_index + 1]['r'] - $colors[$nearest_color_index]['r']));
             $g = round($colors[$nearest_color_index]['g'] + ($alpha * ($colors[$nearest_color_index + 1]['g'] - $colors[$nearest_color_index]['g'])));
             $b = round($colors[$nearest_color_index]['b'] + $alpha * ($colors[$nearest_color_index + 1]['b'] - $colors[$nearest_color_index]['b']));

             return "rgba($r,$g,$b,$opacity)";

     } else {
          $r = round($colors[$nearest_color_index]['r']);
          $g = round($colors[$nearest_color_index]['g']);
          $b = round($colors[$nearest_color_index]['b']);
          return "rgba($r,$g,$b,$opacity)";
  }

 }

Function name

The function name getColor isn't representative of what it does its better of being named createRgbaString (or something along those lines)

Return early

Your script returns in two different places, this should be changed so you exit the function early, it saves an else statement and a level of identing

Variable nameing style

Your variable naming style is not compatible with PSR2 but that isn't the end of the world.

Line length

Your line length exceeds 80 character line length, this is again not the end of the world but will make it hard to read for some people

Repeated calculation

Your repeatedly do the same calculation if the $index != 2 I would move this into a function so if it needs to be updated you only have todo it in one location

How I might write it

I have used terrible variable names, because I don't have the time do work out the complete logic of your script, so this should be unit tested if your going to use it

function getColor($value, $values_range, $opacity = '0.7')
{
    $deltaS = ($values_range['max'] - $values_range['min']) / 2;
    $ds = ($value - $values_range['min']) / $deltaS;
    $nearest_color_index = floor($ds);
    $alpha = $ds - $nearest_color_index;

    $r = $g = $b = -1;

    $colors = [
        ['r'=>255,'g' => 60,'b' => 40],
        ['r' => 255,'g' => 247,'b' =>40],
        ['r' => 12,'g' => 197,'b' => 17]
    ];

    $rNormal = getColorIndexPlusModifier($nearest_color_index, "r", 0, $colors);
    $gNormal = getColorIndexPlusModifier($nearest_color_index, "g", 0, $colors);
    $bNormal = getColorIndexPlusModifier($nearest_color_index, "b", 0, $colors);

    if ($nearest_color_index == 2) {
        return "rgba($rNormal,$gNormal,$bNormal,$opacity)";
    }

    $rPlusOne = getColorIndexPlusModifier($nearest_color_index, "r", 1, $colors);
    $gPlusOne = getColorIndexPlusModifier($nearest_color_index, "g", 1, $colors);
    $bPlusOne = getColorIndexPlusModifier($nearest_color_index, "b", 1, $colors);

    $r = doCalculation($rNormal, $alpha, $rPlusOne);
    $g = doCalculation($gNormal, $alpha, $gPlusOne);
    $b = doCalculation($bNormal, $alpha, $bPlusOne);

    return "rgba($r,$g,$b,$opacity)";
}

function getColorIndexPlusModifier($colorIndex, $colorString, $toModifyBy, $colors)
{
    return round($colors[$colorIndex + $toModifyBy][$colorString]);
}
//TODO Rename to something meaningful
function doCalculation($normalValue, $alpha, $plusOneValue)
{
    return round($normalValue + $alpha * ($plusOneValue - $normalValue));
}


Python Find All Adjacent Subsets of Set of Coins Which Have a Tails Minority Given a sequence of heads and tails I want to find how many significant subsequences are in this sequence where the number of heads is not less than the number of tails. I want to achieve this in \$O(N \log N)\$ time.

Example input:
[ 'H', 'T', 'H', 'T', 'T', 'H' ]

Example output:

11

Explanation:

{H} {H} {H}
{H, T} {T, H} {H, T} {T, H}
{H, T, H}
{H, T, H, T}
{H, T, T, H}
{H, T, H, T, T, H}

I believe my current algorithm is \$O(N^2)\$. I solve the problem recursively, iterating with the list of coins sliced on either end.

Here is my current algorithm. Am I correct that this is \$O(N^2)\$ and is not \$O(N \log N)\$?

def count_sequences( data ):
    print go(range(0,len(data)),data)
seen = set()
def go(rang,data):
    if tuple(rang) in seen: return 0
    seen.add(tuple(rang))
    h = 0
    summ = 0
    if len(rang)==0: return 0
    for i in rang:
        if data[i] == 'H': h += 1
        summ += go(rang[1:],data)
        summ += go(rang[:-1],data)
    if len(rang) == 1:
        if h ==1: return 1
        else: return 0
    if h > (len(rang)-1)/2 :
        return 1 + summ
    else: return summ

go has these costs:

                                     O(x)
def go(rang,data):
    if tuple(rang) in seen: return 0   n
    seen.add(tuple(rang))              n
    h = 0                              1
    summ = 0                           1
    if len(rang)==0: return 0          1
    for i in rang:                     n (
        if data[i] == 'H': h += 1         1
        summ += go(rang[1:],data)         n + T(n-1)
        summ += go(rang[:-1],data)        n + T(n-1)
    if len(rang) == 1:                 ) 1
        if h ==1: return 1             1
        else: return 0                 1
    if h > (len(rang)-1)/2 :           1
        return 1 + summ                1
    else: return summ                  1

You call this once for each sub-tuple of data, and there are at least this many of those:

$$
1 + 2 + 3 + ... + (n-1) + n
$$

Because there are \$1\$ for length \$n\$, \$2\$ for length \$n-1\$, etc. There's one more for length 0, but that's uninteresting.

Each should be multiplied by its cost; if you consider only those that succeed (not those which are caught by seen), they cost \$\mathcal{O}(n^2)\$ each due to

    for i in rang:                     n (
        if data[i] == 'H': h += 1         1
        summ += go(rang[1:],data)         n + T(n-1)
        summ += go(rang[:-1],data)        n + T(n-1)
                                       )

The calls are not part of this; only the looping and the slicing. This gives a cost of

$$
1(n)^2 + 2(n-1)^2 + 3(n-2)^2 + ... + (n-1)(2)^2 + n(1)^21
$$

or

$$
\sum_{k=1}^n k (n-k+1)^2 = \frac{1}{12} n (n+1) (n^2+3 n+2)
$$

which is \$\mathcal{O}(n^4)\$. So caching does improve this a lot from \$\mathcal{O}(2^n n!)\$... but not nearly enough.

Note that we could prove that it's valid to ignore failed calls by moving the if tuple(rang) in seen check into the caller and seeing that the cost is the same.

Using range objects like Python 3's range would give costs of \$\mathcal{O}(n)\$ each, not \$\mathcal{O}(n^2)\$, since slicing would be \$\mathcal{O}(1)\$. However, the check if tuple(rang) in seen prevents this from mattering. You can see this by moving the check into the caller. If you changed this to if rang in seen using an \$\mathcal{O}(1)\$ hash, this would work and reduce the cost to \$\mathcal{O}(n^3)\$ overall. Math elided for brevity.

We can test this experimentally like:

import time

def times(iters):
    for n in range(iters):
        heads = ['H'] * (2**n)
        seen.clear()

        s = time.time()
        count_sequences(heads)
        print((time.time() - s) * 10**5)

times(8)

Giving

0.691413879395
1.00135803223
3.48091125488
14.9011611938
90.2891159058
759.696960449
7593.48869324
87769.818306

We expect for \$\mathcal{O}(n^4)\$ that a doubling of size will give a factor-of-16 change; we instead see closer to a factor-of-12 change, or \$\mathcal{O}(n^{3.6})\$. This is probably partially because slices are fast relative to interpreter overhead. As \$n\$ increases, this will likely change.

When testing with Python 3 ranges, one gets

1.0251998901367188
1.1444091796875
3.5762786865234375
19.14501190185547
123.23856353759766
878.2625198364258
6614.780426025391
51359.89189147949
414633.48865509033

which is almost perfectly \$\mathcal{O}(n^3)\$.

Your code should use spacing. Note how when you posted this question you used paragraphs with line breaks. The same this helps with code. You should also stick to PEP 8.

Just changing that and using brackets on print gives

def count_sequences(data):
    print(go(range(0, len(data)), data))

seen = set()
def go(range_, data):
    if tuple(range_) in seen:
        return 0

    seen.add(tuple(range_))

    h = 0
    summ = 0
    if not range_:
        return 0

    for i in range_:
        if data[i] == 'H':
            h += 1

        summ += go(range_[1:],data)
        summ += go(range_[:-1],data)

    if len(range_) == 1:
        if h == 1:
            return 1
        else:
            return 0

    if h > (len(range_) - 1) / 2:
        return 1 + summ

    else:
        return summ

I would use a better name than rang/range_; probably indices. However, best use two integers: start/stop. count_sequences should return rather then print. Further, a little cleanup gives

def count_sequences(data):
    return go(data, 0, len(data))

seen = set()
def go(data, start, stop):
    if (start, stop) in seen:
        return 0

    seen.add((start, stop))

    h = 0
    summ = 0
    if start == stop:
        return 0

    for i in xrange(start, stop):
        if data[i] == 'H':
            h += 1

        summ += go(data, start+1, stop)
        summ += go(data, start, stop-1)

    if stop - start == 1:
        return h

    if h > (stop - start - 1) / 2:
        summ += 1

    return summ

Then consider that

summ += go(data, start+1, stop)
summ += go(data, start, stop-1)

is a loop invariant, so move it out.

And

for i in xrange(start, stop):
    if data[i] == 'H':
        h += 1

can be

h = data[start:stop].count('H')

You can make data[start:stop].count('H'), the only remaining \$\mathcal{O}(n)\$ aspect, faster by replacing data with a list of cumulative counts and doing

h = counts[stop] - counts[start]

This should reduce the cost to just \$\mathcal{O}(n^2)\$ overall; constant cost for each subsequence.

This doesn't do anything so remove it:

if stop - start == 1:
    return h

This then gives

def count_sequences(data):
    counts = [0]
    for item in data:
        counts.append(counts[-1] + (item == 'H'))

    return go(counts, 0, len(data))

seen = set()
def go(counts, start, stop):
    if start == stop:
        return 0

    if (start, stop) in seen:
        return 0
    seen.add((start, stop))

    h = counts[stop] - counts[start]

    summ = go(counts, start+1, stop) + go(counts, start, stop-1)
    if h > (stop - start - 1) / 2:
        summ += 1

    return summ

Simplifying

The first step in simplifying is to consider

h = counts[stop] - counts[start]

if h > (stop - start - 1) / 2:
    summ += 1

This can be reduced to

if counts[stop] - counts[start] >= 0:
    summ += 1

by making 'T' count as -1. Then any subsection with more heads than tails will have a positive total.

This simplifies the whole code to

from itertools import combinations

def count_sequences(data):
    counts = [0]
    for item in data:
        counts.append(counts[-1] + (1 if item == 'H' else -1))

    return sum(end >= start for start, end in combinations(counts, 2))

This is still \$\mathcal{O}(n^2)\$ but it's also much faster and has no recursion limit.

This gives us the secret for how to do this in \$\mathcal{O}(n \log n)\$. We want to do this fast:

sum(end >= start for start, end in combinations(counts, 2))

Let's say you have a list counts as so:

[1, 2, 1, 0, -1, 0, 1, 2, 3, 2, 1, 0, -1, 0, -1, 0]

You look at the list in two parts:

[1, 2, 1, 0, -1, 0, 1, 2]   [3, 2, 1, 0, -1, 0, -1, 0]

You know f(first_half) and f(second_half), so you want to find

sum(end >= start for start, end in product(first_half, second_half))

Unfortunately, the above is no faster than the naïve method. However, you can do merge sort with f(first_half) and f(second_half) in \$\mathcal{O}(n \log n)\$ time, so you can instead have

[-1, 0, 0, 1, 1, 1, 2, 2]   [-1, -1, 0, 0, 0, 1, 2, 3]

Then

sum(end >= start for start, end in product(first_half, second_half))

can be optimized by making a single pass:

def sum_across(first_half, second_half):
    total = 0
    snd_idx = 0

    for i in first_half:
        while second_half[snd_idx] < i:
            snd_idx += 1

            if snd_idx == len(second_half):
                return total

        total += len(second_half) - snd_idx

    return total

All in all, this gives:

from heapq import merge

def sum_across(first_half, second_half):
    total = 0
    snd_idx = 0

    for i in first_half:
        while second_half[snd_idx] < i:
            snd_idx += 1

            if snd_idx == len(second_half):
                return total

        total += len(second_half) - snd_idx

    return total

def count_sequences(data):
    counts = [0]
    for item in data:
        counts.append(counts[-1] + (1 if item == 'H' else -1))

    def recurse(ns):
        if len(ns) <= 1:
            return ns, 0

        mid_point = len(ns) // 2
        first_half, second_half = ns[:mid_point], ns[mid_point:]

        fst_sorted, count_fst = recurse(first_half)
        snd_sorted, count_snd = recurse(second_half)

        count = count_fst + count_snd + sum_across(fst_sorted, snd_sorted)
        ns_sorted = list(merge(fst_sorted, snd_sorted))

        return ns_sorted, count

    counts_sorted, count = recurse(counts)
    return count

count_sequences([ 'H', 'T', 'H', 'T', 'T', 'H' ])
#>>> 11


Random walk on a 2D grid The program assignment:

  A drunkard in a grid of streets randomly picks one of four directions
  and stumbles to the next intersection, then again randomly picks one
  of four directions, and so on. You might think that on average the
  drunkard doesn't move very far because the choices cancel each other
  out, but that is not the case. Represent locations as integer pairs
  (x,y). Implement the drunkard's walk over 100 intersections, starting
  at (0,0) and print the ending location.

I created this Drunkard class:

import java.util.*;

class Drunkard {
    int x, y;
    Drunkard(int x, int y) {
    this.x = x;
    this.y = y;
    }
    void moveNorth() {
    this.y -= 1;
    }
    void moveSouth() {
    this.y += 1;
    }
    void moveEast() {
    this.x += 1;
    }
    void moveWest() {
    this.x -= 1;
    }
    void report() {
    System.out.println("Location: " + x + ", " + y);
    }
}

And this DrunkardTester class to test the above class

import java.util.Random;

public class DrunkardTester {
    public static void main(String[] args) {
    Random generator = new Random();
    Drunkard drunkard = new Drunkard(0, 0);
    int direction;
    for (int i = 0; i < 100; i++) {
        direction = Math.abs(generator.nextInt()) % 4;
        if        (direction == 0) { // N
        drunkard.moveNorth();
        } else if (direction == 1) { // E
        drunkard.moveEast();
        } else if (direction == 2) { // S
        drunkard.moveSouth();
        } else if (direction == 3) { // W
        drunkard.moveWest();
        } else {
        System.out.println("Impossible!");
        }
    }
    drunkard.report();
    }

}

It complies and runs fine, and it prints a different coordinate location each time I run it, but I just wanted to get a few other pairs of eyes on it to see if there are any glaring errors.

Also, on my DrunkardTester class is there anyway to eliminate the "Impossible!" else statement? I wasn't really sure if that was necessary or what else could be put there.

There is absolutely no need to call Math.abs for generator.nextInt() since nextInt will never produce a negative number. Also, your last else-statement is impossible to happen so you can erase that one.

There is also no need to use the modulo (%) operator to fix your direction variable to the correct range when you instead can supply an integer value to the nextInt method. generator.nextInt(4); will give you a value from 0 to 3 (inclusive).

It is a Java coding convention to indent your code properly, after each { the next line should have one more indentation step, like this:

void moveSouth() {
    this.y += 1;
}

The below might be a little bit over your level, but it is a really nice solution and is very useful to learn. When you are dealing with four directions, I would create an enum to help with the different direction possibilities.

public enum Direction4 {
    NORTH(0, -1), EAST(1, 0), SOUTH(0, 1), WEST(-1, 0);

    private final int dx;
    private final int dy;

    private Direction4(int dx, int dy) {
        this.dx = dx;
        this.dy = dy;
    }
    public int getX() { return dx; }
    public int getY() { return dy; }
}

Using this enum, you can simplify several things of your other code. For starters, you can replace your move-methods with one single move method:

void move(Direction4 direction) {
    this.x += direction.getX();
    this.y += direction.getY();
}

And when generating the direction to move at, you can use the Direction4 enum:

// Get an index number and input the "size" of the enum to `nextInt`
int directionIndex = generator.nextInt(Direction4.values().length);

// Get the direction from the index generated above
Direction4 dir = Direction4.values()[directionIndex];

// Make the move:
drunkard.move(dir);

It might be subjective whether four different moveNorth, moveSouth etc. methods are preferable instead of using a single move-method that takes the direction as an argument. I myself find that using an enum redures the amount of code and it makes the code flexible and scalable (if you want to add or modify existing directions you can do so, for example adding MEGA_NORTH that moves 10 steps north just for fun).

If you don't perform the enum approach, then at least use a switch

direction = generator.nextInt(4);
switch (direction) {
    case 0:
        drunkard.moveNorth();
        break;
    case 1:
        drunkard.moveEast();
        break;
    case 2:
        drunkard.moveSouth();
        break;
    case 3:
        drunkard.moveWest();
        break;
}

Although I must say: There's not much that beat a nice little enum :)


Parsing displayed month/year for calendar I have a 6 x 7 grid for a calendar = 42 DateTimes. I'm trying to display the displayed month/year at the top of the calendar.

If there are 3 months displayed, I only want to show the name for the middle month (October 2018, for instance). If 2 months are displayed, split the name (October - November 2018). If 2 months are displayed and have different years, append with year on both sides (December 2018 - January 2019).

This is what I have, and it works, but it's in a property that may be "getted" often and I want this to be as efficient as possible. Here's the code:

public string ViewedMonthYearStr
{
    get
    {
        if (_grid != null)
        {
            string first;
            string middle = string.Empty;
            string last = string.Empty;
            HashSet<int> months = new HashSet<int> { _grid[0].Month, _grid[21].Month, _grid[41].Month };
            if (months.Count == 3) first = _grid[21].ToString("MMMM yyyy");
            else
            {
                first = _grid[0].ToString("MMMM");
                middle = " - ";
                last = _grid[41].ToString("MMMM yyyy");
            }
            if (_grid[0].Year != _grid[41].Year && months.Count != 3)
            {
                middle = $" {_grid[0].ToString("yyyy")} - ";
            }
            _viewedMonthYearStr = $"{first}{middle}{last}";
        }
        return _viewedMonthYearStr;
    }
}

Is there a more efficient way to do this? My gut tells me that if/else's aren't the most efficient way.

I wouldn't worry about this being inefficient - you're not doing anything crazy here, and a calendar header probably won't need to be refreshed millions of times per second anyway.

Still, I can think of some changes that will improve readability and maintainability:

I would use a guard clause to reduce nesting, and to make it more obvious what happens when grid is null: if (_grid == null) return null;. This also gets rid of edge-cases early on, while in the current code you have to scan down to the end to see if maybe there's still an else statement following.
Adding an empty line between the end of the else block and the next if statement will make it more obvious that they're not related.
However, the last if statement only applies when displaying two months, so why not put it inside the above else block? That also allows you to remove the months.Count != 3 check.
Putting first = _grid[21]... on the same line as that if statement, and without braces, makes the control flow more difficult to see at a glance. Omitting braces can also result in subtle bugs, so some people make a point of always using braces. Either way, consistency matters: if an else statement uses braces I'd expect the related if statement to also use them.
Why assign the result to _viewMonthYearStr instead of returning it directly? That field does not seem to serve any purpose.
0, 21 and 41 are 'magic values': the meaning of those values isn't directly clear, and they easily break the code when the size of _grid needs to be changed. Replacing _grid[0] with _grid.First(), _grid[41] with _grid.Last() and _grid[21] with _grid[_grid.Length / 2] will make the code more robust and easier to understand. You could also assign their results to local variables, such as: var middleDate = _grid[_grid.Length / 2];. That simplifies the code somewhat, and it allows you to use _grid[0] and _grid[_grid.Length - 1] instead of First and Last without sacrificing readability, if you really need that little bit of performance gain.
Instead of using the local variables first, middle and last, you can return results immediately. That will make the possible output formats easier to see.
Instead of using a hash set, you can also check if the Month of the middle date is different from that of both the first and the last date. That will put a tiny bit less pressure on the GC.
Instead of calling date.ToString(format), and then using the result in an interpolated string, you can also specify the format in the interpolated string directly: $"{date:format}".
I'd rename ViewedMonthYearStr to Header or Title - that's what it's used for, after all.

With all that, this is what I would end up with:

public string Header
{
    get
    {
        if (_grid == null)
            return null;

        var firstDate = _grid.First();
        var middleDate = _grid[_grid.Length / 2];
        var lastDate = _grid.Last();

        // If more than 2 months are displayed, focus only on the middle month:
        if (middleDate.Month != firstDate.Month && middleDate.Month != lastDate.Month)
            return $"{middleDate:MMMM yyyy}";

        if (firstDate.Year != lastDate.Year)
            return $"{firstDate:MMMM yyyy} - {lastDate:MMMM yyyy}";

        return $"{firstDate:MMMM} - {lastDate:MMMM yyyy}";
    }
}


Preview method for multiple types of panels Using Rubocop, pretty much any case statement is caught by the Cyclomatic Complexity cop, often the Assignment Branch Condition and method length cops as well. To get around this, I've been refactoring code like this:

def preview
  case object.panel_type
  when "image"
    asset_image_path(object.asset)
  when "profile_block"
    Profile.find(object.type_id).try(:full_name)
  when "slider"
    if object.options.present? && object.options["slides"].present?
      Slide.find(object.options["slides"][0]).headline
    else
      "Empty"
    end
  when "menu"
    Menu.find(object.type_id).try(:name)
  else
    clean_content.try(:truncate, (object.span * 4))
  end
end

Into this:

def preview
  if %w(image profile_block slider menu search_list).include?(object.panel_type)
    send("#{object.panel_type}_preview")
  else
    clean_content.try(:truncate, (object.span * 4))
  end
end

private

def image_preview
  asset_image_path(object.asset)
end

def profile_block_preview
  Profile.find(object.type_id).try(:full_name)
end

def slider_preview
  if object.options.present? && object.options["slides"].present?
    Slide.find(object.options["slides"][0]).headline
  else
    "Empty"
  end
end

def menu_preview
  Menu.find(object.type_id).try(:name)
end

def search_list_preview
  GridPanel::SEARCH_TYPES.invert[object.type_id]
end

In general, I think dynamic method names can be a bit problematic for maintenance. One of the problems with legacy Rails code is finding all the usage of dynamic methods in a project when you have to add or remove things. But in this case, I think the names have consistency and a limited scope, so it doesn't seem like it would be difficult to maintain or update later. Is there are more conventional way to do this?

I don't know your class but it looks like your object can have different subclasses depending on object.panel_type (maybe a single table inheritance?) and every subclass implements it's own preview method.

 class Image < BaseClass
  def preview
      asset_image_path(object.asset)
  end
end

class Slider < BaseClass
  def preview
       if object.options.present? && object.options["slides"].present?
          Slide.find(object.options["slides"][0]).headline
       else
         "Empty"
       end
  end
end


Load bitmap object data into array of byte values I'm building this as part of a larger class to rapidly search for smaller images inside larger images. This is my first serious F# project so comments especially concerning style and organization are appreciated.

module imagesearch =
    type BitmapSearchClass() =
        // Largely inspired by this article
        // http://msdn.microsoft.com/en-us/library/system.drawing.imaging.bitmapdata.aspx
        static member loadBitmapIntoArray (pBitmap:Bitmap) =
            let tBitmapRectangle = Rectangle(0, 0, pBitmap.Width, pBitmap.Height)

            let tLockMode = ImageLockMode.ReadOnly
            let tImageFormat = PixelFormat.Format24bppRgb

            let tBitmapData = pBitmap.LockBits(tBitmapRectangle, tLockMode, tImageFormat)
            let tIntPtr = tBitmapData.Scan0

            let tImageByteLength = Math.Abs(tBitmapData.Stride) * pBitmap.Height

            // Zerocreate creates the "empty" array
            let tImageRGBValues : byte array = Array.zeroCreate tImageByteLength

            do Marshal.Copy(tImageRGBValues, 0, tIntPtr, tImageByteLength)

            do pBitmap.UnlockBits(tBitmapData)

            tImageRGBValues, pBitmap.Width, pBitmap.Height

Current state of the module is below

module ImageSearch =
    type BitmapSearch() =
        // Largely inspired by this article
        // http://msdn.microsoft.com/en-us/library/system.drawing.imaging.bitmapdata.aspx
        static member LoadBitmapIntoArray (pBitmap:Bitmap) =
            let tBitmapData = pBitmap.LockBits( Rectangle(Point.Empty, pBitmap.Size),
                                                ImageLockMode.ReadOnly,
                                                PixelFormat.Format24bppRgb )

            let tImageArrayLength = Math.Abs(tBitmapData.Stride) * pBitmap.Height
            let tImageDataArray = Array.zeroCreate<byte> tImageArrayLength

            Marshal.Copy(tImageDataArray, 0, tBitmapData.Scan0, tImageArrayLength)
            pBitmap.UnlockBits(tBitmapData)

            tImageDataArray, pBitmap.Width, pBitmap.Height

module imagesearch =

All types should use PascalCase, this includes modules. So the correct name would be ImageSearch.

type BitmapSearchClass() =

That's a weird name, names of classes shouldn't end with Class. And why is it a class in the first place? If it contains only static members, it should probably be a module.

static member loadBitmapIntoArray (pBitmap:Bitmap) =

Public methods should also use PascalCase (LoadBitmapIntoArray).

pBitmap:Bitmap
let tBitmapRectangle = …

Prefixing each parameter with p and each local variable with t seems like a weird kind of Hungarian notation. Please don't do that, it doesn't serve any purpose.

Rectangle(0, 0, pBitmap.Width, pBitmap.Height)

A shorter way to write this would be:

Rectangle(Point.Empty, pBitmap.Size)

let tLockMode = ImageLockMode.ReadOnly
let tImageFormat = PixelFormat.Format24bppRgb

let tBitmapData = pBitmap.LockBits(tBitmapRectangle, tLockMode, tImageFormat)

Why are these variables? Since you never use them again, and since naming them doesn't add any useful information, you can just write them directly in the function call:

let tBitmapData = pBitmap.LockBits(tBitmapRectangle, ImageLockMode.ReadOnly, PixelFormat.Format24bppRgb)

let tImageRGBValues : byte array = Array.zeroCreate tImageByteLength

Another way to specify the type is to add type parameter to zeroCreate:

let tImageRGBValues = Array.zeroCreate<byte> tImageByteLength

do Marshal.Copy(tImageRGBValues, 0, tIntPtr, tImageByteLength)

do pBitmap.UnlockBits(tBitmapData)

You don't need to write do here. (Actually, I was surprised you can write do here.)


Simple log function in Bash I am just getting a little more comfortable with Bash.
I wanted a super simple log function to write to a file and output to the console for messages and errors. I also wanted the terminal and the file to interpret newline characters rather than printing them out.
Here is what I came up with (it works fine):
#!/bin/bash

log () {
  if [[ "$3" == '-e' || "$3" == '--error' ]]; then
    >&2 echo -e "ERROR: $1" && printf "ERROR: $1\n" >> "$2"
  else
    echo -e "$1" && printf "$1\n" >> "$2"
  fi
}

# Example implementation
log_file=snip/init.log
msg="\nthis is an error message with leading newline character"
log "$msg" $log_file -e
msg="this is success message with no newline characters"
log "$msg" $log_file

I don't need any more functionality however I have some questions:

Is an efficient and readable piece of code that won't break when passed strings with special characters?
Syntactically, could I have done this 'better' such as using pipes instead of &&. tee instead of printf etc.?
How can I refactor this code to allow for the optional -e flag to be passed in first?

There's quite a bit of repetition:

echo && print with roughly the same arguments - tee is probably better here
error and non-error paths - perhaps use one function to prepend the ERROR: tag and call the other?

It might be best to have the filename first, then any number of message words?  That would make the interface more like echo, for example.
There's also a problem with our use of printf - any % characters in the error message will be interpreted as replacement indicators.  Not what we want.  We could use printf %b to expand escape sequences instead:
# log FILE MESSAGE...
log () {
    local file="$1"; shift
    printf '%b ' "$@" '\n' | tee -a "$file"
}

# log_err FILE MESSAGE...
log_error () {
    local file="$1"; shift
    local message="ERROR: $1"; shift
    log "$file" "$message" "$@" >&2
}

If we do want to have a single function with arguments (e.g. to add more options), then we might want to use the while/case approach of option parsing that's often used at the program level:
#!/bin/bash

set -eu

# log [-e] [-f FILE] MESSAGE...
log() {
    local prefix=""
    local stream=1
    local files=()
    # handle options
    while ! ${1+false}
    do case "$1" in
        -e|--error) prefix="ERROR:"; stream=2 ;;
        -f|--file) shift; files+=("${1-}") ;;
        --) shift; break ;; # end of arguments
        -*) log -e "log: invalid option '$1'"; return 1;;
        *) break ;; # start of message
       esac
       shift
    done
    if ${1+false}
    then log -e "log: no message!"; return 1;
    fi
    # if we have a prefix, update our argument list
    if [ "$prefix" ]
    then set -- "$prefix" "$@"
    fi
    # now perform the action
    printf '%b ' "$@" '\n' | tee -a "${files[@]}" >&$stream
}

There's a non-obvious trick in there that might need explanation: I've used ${1+false} to expand to false when there's at least one argument remaining, or to an empty command when there are no arguments (empty command is considered "true" by if and while).


Program that tells if a date is valid in C I'm reading a book about C programming, at the end of each chapter it has some training exercise and one of them was to make a program that tells if a date is valid or not, the code below is what I did and I would like to know if it can be improved?

#include <stdio.h>

int main(){
int dd,mm,yy;

printf("write a date(day/month/year):");
scanf("%d/%d/%d",&dd,&mm,&yy);

int bissextile=(yy%4==0)?1:0;

if (dd>31 || dd<1 || mm>12 || mm<1)
    printf("Invalid date.\n");
    else
        if((mm==2 && dd<30 && bissextile) || (mm==2 && dd<29 && bissextile==0))
            printf("valid date.\n");
            else
                if((mm==4 || mm==6 || mm==9 || mm==11)&& dd<31)
                    printf("valid date.\n");
                    else
                        if(dd<31 && mm!=2)
                            printf("valid date.\n");
                            else
                                printf("Invalid date.\n");
}

Validation is a specific task, distinct from receiving input, so it deserves to have its own function.

The code suffers from excessive nesting, which could be improved by putting else if at the same indentation level. However, another problem is that some of the checks are affirmative and some of them are negative, which makes the code harder to follow.

Since the validation depends mostly on the month, I'd write it this way, with a switch:

int isValid(int dd, int mm, int yy) {
    if (dd <= 0) return 0;
    switch (mm) {
      case 1: case 3: case 5: case 7:
      case 8: case 10: case 12:
        return dd <= 31;
      case 4: case 6: case 9: case 11:
        return dd <= 30;
      case 2:
        return dd <= 28 + (yy % 4 == 0);
      default:
        return 0;  /* invalid month */
    }
}

Then, in main():

if ( 3 == scanf("%d/%d/%d",&dd,&mm,&yy) &&
     isValid(dd, mm, yy) ) {
    puts("Valid date");
} else {
    puts("Invalid date");
}

Note that validating the return value from scanf() is important too.

The code doesn't handle the Gregorian correction properly, such as for year 1900.


The Rust Programming Language Pig Latin Exercise This is my implementation of the Pig Latin recommended exercise in The Rust Programming Language book. I am using the unicode segmentation crate to split the string into words while also keeping the delimiters. Any pointers on making this code more idiomatic or run more optimal?
use unicode_segmentation::UnicodeSegmentation;

#[allow(overlapping_patterns)]
fn translate_word(s: &str) -> String {
  let mut it = s.chars();
  let first = it.next().unwrap();
  match first.to_ascii_lowercase() {
    'a' | 'e' | 'i' | 'o' | 'u' => format!("{}-hay", s),
    'a'..='z' => format!("{}-{}ay", it.collect::<String>(), first),
    _ => s.to_string(),
  }
}

pub fn translate(s: &str) -> String {
  s.split_word_bounds()
    .map(translate_word)
    .collect::<Vec<_>>()
    .join("")
}

The code is inside a module named pig_latin.

Be aware that Rust typically uses 4 spaces
It's fine if you consistently use 2 spaces (especially if you override it in rustfmt.toml), but just be aware that the standard is different.
Collect directly to a String
Instead of collecting to a Vec and then copying that over to a new Vec (within String), collect to a String directly:
pub fn translate(s: &str) -> String {
    s.split_word_bounds().map(translate_word).collect()
}

Use Chars::as_str
When you use str::chars, the specific iterator that it returns is called Chars. It has a handy function to get the remaining part, so you don't need to allocate a new string:
'a'..='z' => format!("{}-{}ay", it.as_str(), first),

Use Cow
This is a bit of an advanced optimization that you don't need to do—the Book doesn't even mention it once.
Currently, you allocate a new String even when the output is identical to the input. Instead, return Cow<str>: if the first character isn't a letter, you can return Cow::Borrowed(s), which points to the existing &str. If it does start with a letter, return Cow::Owned(format!(...)), which has the same overhead as it did before. Here, I'm using .into() instead of writing Cow::Owned and Cow::Borrowed explicitly. You can do either.
fn translate_word(s: &str) -> Cow<str> {
    let mut it = s.chars();
    let first = it.next().unwrap();
    match first.to_ascii_lowercase() {
        'a' | 'e' | 'i' | 'o' | 'u' => format!("{}-hay", s).into(),
        'a'..='z' => format!("{}-{}ay", it.as_str(), first).into(),
        _ => s.into(),
    }
}

Final code
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=78daca7b7adab4587436cacf35bca90e


HackerRank - Binary tree to compute the Nth power of natural numbers that sum up to X I'm trying to solve this challenge on HackerRank.
In short, given x and n, I have to determine how many ways I can pick numbers from the nth powers of the  natural numbers such that their sum is x.
So my first reasoning was to compute the list of the nth powers that I need as a first step. This list is
list = takeWhile (<= x)  [i^n | i <- [1..]]`

Now list is the bucket from which I have to pick numbers to add up to n.
I first thought about list comprehensions, because that's how I would pick a given number of numbers from the list; for instance this is how I would generate the sumList of the sums of any different triplets from list:
sumList = [x + y + z | x <- list, y <- dropWhile (<= x) list, z <- dropWhile (<= y) list]

Then I would just need to length $ filter (== x) sumList.
However, after some thinking, I concluded that list comprehensions are not the way to go, since I don't know in advance how many numbers I have to pick from list when attempting to sum them up to x.
So I thought that any possible sum corresponds to a combination of pick or not-pick while traversing the list.  This made me think of binary trees, and I eventually came up with this solution, which fails the Test Case 3 for timeout:
main :: IO()
main = do
  [x,n] <- sequence $ replicate 2 ((read :: String -> Int) <$> getLine)
  print $ length $ filter (== x) $ getLeaves $ listToSumTree $ takeWhile (<= x) [i^n | i <- [1..]]

data Tree a = Leaf a | Node a (Tree a) (Tree a) deriving (Show)

listToTreeImpl :: (Num a, Eq a) => [a] -> Tree a -> Tree a
listToTreeImpl [] tree = tree
listToTreeImpl (l:ls) (Leaf x) = Node x
                         (listToTreeImpl ls $ Leaf x)
                         (listToTreeImpl ls $ Leaf (x + l))

getLeaves :: Tree a -> [a]
getLeaves (Leaf a) = [a]
getLeaves (Node _ (Leaf _) (Node _ _ _)) = error "this should not happen"
getLeaves (Node _ (Node _ _ _) (Leaf _)) = error "this should not happen"
getLeaves (Node _ left right) = getLeaves left ++ getLeaves right

listToSumTree :: (Num a, Eq a) => [a] -> Tree a
listToSumTree ls = listToTreeImpl ls (Leaf 0)

I can only think of getLeaves $ listToSumTree as the only critical part of the program, since listToSumTree constructs a full tree, which is not needed (only the leaves are), and getLeaves traverses all the tree, only to get to the bottom layer of it.

What you're essentially doing with your binary tree is checking all possible combinations of numbers from the list [i^n | i <- [1..x]]. That's quite a lot of combinations! (Exercise: how many?)
Not all of those combinations are sensible, however. For example, if x = 100 and n = 2, and you've already chosen, say, 4 and 5 (giving 62+72=85), then adding 82=64 is not going to help, nor is adding 92=81. In your current solution, however, you're still checking those.
So how does one encode this knowledge in the search for solutions? The key thing is that we can build the solution recursively: we can use an already computed answer for a smaller case to help answer a larger case. If we're given x = 10 and n = 2, then we can only add 12, 22 or 32. If we try adding 32, we have only 1 left to fill, which we can only do by adding 12. If, instead, we start by trying 22, we have to make 6, which is impossible. Starting with 12 lets us choose 32 to fill up the remaining 9. Since only the set of numbers counts, not their order, this means there is only one solution for x=10,n=2: namely, {12,32} (as stated in the problem).
Checking whether two solutions are the same modulo ordering is annoying, however. Can we do better? Yes we can, by adding another parameter to the problem.
Think about how we could do this before reading on.

A way to change the problem to prevent us from generating multiple solutions that are really the same set of numbers, is to add as a parameter the maximum value we're allowed to choose. So a problem is now not only specified by (x=10,n=2), but also by an integer M; for example, if M=9, then we may add 12≤9, 22≤9 and 32≤9, so (x=10,n=2,M=9) has one solution: {12,32}. However, (x=10,n=2,M=2) has no solutions, because by only using 12 and 22 you can never make 10.
How is this useful? Well, consider how we could, as a lazy office worker, go about computing how many solutions there are to (x=10,n=2,M=10). (For the original problem, we just set M=x, since choosing numbers higher than x is certainly useless.) This lazy office worker realises that either x=0, in which case we have precisely 1 solution (choosing no numbers at all), or x≠0, in which case we need to choose at least one number. If we choose, say, i2, then the remainder still to fill is 10 - i2. Since our lazy office worker is also clever, they realise that if we force any solutions for 10 - i2 to only choose numbers smaller than i2 (i.e. set M = i2 - 1), then we won't ever get any duplicates: there may certainly be solutions for our (x,n,M) that choose i2 and also some larger j2, but then we'll find that one by starting with j2 and then going downward.
So our lazy office worker writes down a series of smaller problems: [(x-in,n,in-1) | i <- [1..M]], to solve for the next office worker in line. (These are recursive calls.) The total number of solutions is just the sum of the number of solutions for each of these subproblems.
An alternative, more direct way to explain the same way of solving, is to note that while all solutions are sets of distinct integers, we can represent those sets by sorted lists. If the empty solution is not possible (i.e. x≠0), then each of the possible solutions for our current problem has one largest value, so we can find all solutions for the problem going over each largest value in from 1 to M, computing the solutions for (x - i2, n, in - 1), and prepending that largest value i2 to its corresponding sub-solutions. If, instead of building the actual solution lists we just compute the number of them, we arrive at the same solution as above.

Now the above is not very formalised yet, and that is on purpose. Because for me, the next step to formalisation would be writing out the recursive function, which is 95% of the work in implementing the program. And that's your task. :)
If any of the above is unclear (probably), or you get stuck, feel free to ask further questions.
EDIT: When I coded out my first solution for the problem, I also cached recursive calls in a table, making it a dynamic programming solution. However, for the given input range that apparently doesn't help, so a standard recursive program works fine.
EDIT 2: If you want to handle 1000 1, then you do need dynamic programming :)


Simple addition, multiplication, divison, and subtraction program I made a post a couple of days ago asking if anyone noticed bad habits or thought of any suggestions: (First Hangman game)

I've made 2 math programs just to make sure I got it all down pat. The first one (the subject of this question) is a simple addition, multiplication, division, and subtraction program that would deal out random numbers and ask you to add, sub, multiply, or divide them.

#12/14/14
import random

def choose():
    num1, num2 = pick_numbers()
    types = input("What type would you like Add, Sub, Mutl, Div?\n> ")
    if types.upper() == "ADD":
        print("Creating your addition problem...")
        print("{} + {}".format(num1, num2))
        correct_answer = num1 + num2
        return correct_answer
    elif types.upper() == "SUB":
        print("Creating your subtraction problem...")
        print("{} - {}".format(num1, num2))
        correct_answer = num1 - num2
        return correct_answer
    elif types.upper() == "MULT":
        print("Creating your mutliplecation problem...")
        print("{} * {}".format(num1, num2))
        correct_answer = num1 * num2
        return correct_answer
    elif types.upper() == "div":
        print("Creating your divison problem...")
        print("{} / {}".format(num1, num2))
        correct_answer = num1 / num2
        return correct_answer
    else:
        print("Error: Please only choose, add, sub, mult, or div")

def pick_numbers():
    num1 = random.randint(1, 100)
    num2 = random.randint(1, 100)
    return num1, num2

def print_problem():
    correct_answer = choose()
    while True:
        answer = input("What is your answer?\n> ")
        if(int(answer) == correct_answer):
            print("Correct! The answer was {}".format(answer))
            return False
        else:
            print("Sorry {} isn't correct.".format(answer))

def play_again():
    try_again = input("Would you like to play again?\n> ")
    if try_again.upper() == "Y":
        return True
    elif try_again.upper() == "N":
        return False
    else:
        print("Error: please choose only 'Y' or 'N'")

while True:
    print_problem()
    if not play_again():
        break

Do you still notice any bad habits or have any suggestions so I can have better structure or code organization?

Be careful with exceptions. int(answer) may raise a ValueError if an answer cannot be converted. Same goes for unchecked division.
In case of an illegal choice, choose doesn't return a value. Not a problem here, but rather a bad habit.
if/elif/elif/... cascade is usually better expressed via a dictionary, in this case of 2-tuples (a format string and a lambda:

problems = { "ADD": ("{} + {}", lambda x,y: x + y),
             "MULT": (...),
              etc }

Then the body of choose could be reduced to

try:
    format, action = problems[types.upper()]
    print format.format(num1, num2)
    return action(num1, num2)
except KeyError:
    print "Choose only from %s" % ",".join([key for key in problem.keys()])

and you may freely add more operations to problems not ever touching the code.


Backwards stepwise regression code in R (using cross-validation as criteria) I want to write code that does backward stepwise selection using cross-validation as a criterion.

I have only started learning R a month ago and I have almost zero programming experience prior to that. Hence, I would appreciate any comments on the code.

(I understand that there are issues with the backward stepwise selection process itself.)

rm(list=ls())
library(forecast)
library(fpp)
set.seed(1)

x1<-c(1:500)*runif(20,min=0,max=200)
x2<-c(1:500)*runif(20,min=0,max=200)

library(caret)
data(credit)
score <- credit$score
log.savings <-log(credit$savings+1)
log.income <-log(credit$income+1)
log.address<- log(credit$time.address+1)
log.employed <- log(credit$time.employed+1)

##################################

#Input
full.model<- score ~ log.savings + log.income + log.address + log.employed + x1 +x2

# 1. Start with the full model
counter=1
full <- lm(full.model)
scopevars<- attr(full$terms, "predvars") # list of model predicotrs
n <- length(full$residuals);  # sample size
current_best <- full #what is the currenly the best model ? that would be our full model for now

while(T){ #process until we break the loop

  best_cv <- CV(current_best)[1] #the CV of the current_best model
  temp <- summary(current_best) #summary output of the current_best model
  p <- dim(temp$coefficients)[1]  # current model's size

  rnames <- rownames(temp$coefficients)  # list of terms in the current model

  compare <- matrix(NA,p,1) # create a matrix to store CV information
  rownames(compare) <- rnames #name the matrix

  #create a loop that drops one predictor at a time and calculates the CV of the regression when that predictor is dropped
  #put all the information into "compare"
  #drop the variable that has the lowerst CV when it is dropped.

  for (i in 2:p) {
    drop_var <- rnames[i] #variable to be deleted
    f <- formula(current_best) #current formula
    f <- as.formula(paste(f[2], "~", paste(f[3], drop_var, sep=" - "))) # modify the formula to drop the chosen variable (by subtracting it)
    new_fit <- lm(f)  # fit the modified model
    compare[i]<-CV(new_fit)[1]
  }

  remove_var <-rownames(compare)[which.min(compare)] #drop is the varibale that is associate with the lowerst CV
  update_cv <- compare[which.min(compare)]

  if(update_cv>best_cv) break #we should not continue if dropping a variable will not improve Cv

   write(paste("--- Dropping",counter, remove_var , update_cv, "\n"), file="") # output the variables we are dropping.

  f <- formula(current_best) #current formula
  f <- as.formula(paste(f[2], "~", paste(f[3], remove_var , sep=" - ")));  # modify the formula to drop the chosen variable (by subtracting it)
  current_best <- lm(f)# we now have a new best model
  counter=counter+1 #update our counter
  next
} #end the while loop
print(current_best)
CV(current_best)

Your code is good. I could not find important corrections to be made. I could find only some small cosmetic improvements - nothing serious.

For example. You do not need the caret package loaded for this task. You do not need to use next at the end of the while loop. It is good practice to keep your code in width of 80 characters. It improves readability a lot. See my code attached.

rm(list=ls())
gc()

library(forecast)
library(fpp)

set.seed(1)
x1 <- c(1:500) * runif(20, min=0, max=200)
x2 <- c(1:500) * runif(20, min=0, max=200)

data(credit)
score <- credit$score
log.savings <- log(credit$savings+1)
log.income <- log(credit$income+1)
log.address <- log(credit$time.address+1)
log.employed <- log(credit$time.employed+1)

##################################

#Input
full.model <- score ~ log.savings + log.income + log.address +
  log.employed + x1 +x2

# 1. Start with the full model
counter <- 1L
full <- lm(full.model)

scopevars <- attr(full$terms, "predvars") # list of model predicotrs
n <- length(full$residuals)  # sample size
current_best <- full #what is the currenly the best model?
 # that would be our full model for now

while(T) { #process until we break the loop

  best_cv <- CV(current_best)[1] #the CV of the current_best model
  temp <- summary(current_best) #summary output of the current_best model
  p <- dim(temp$coefficients)[1]  # current model's size

  rnames <- rownames(temp$coefficients)  # list of terms in the current model

  compare <- matrix(NA,p,1) # create a matrix to store CV information
  rownames(compare) <- rnames #name the matrix

  # create a loop that drops one predictor at a time and calculates
  # the CV of the regression when that predictor is dropped
  # put all the information into "compare"
  # drop the variable that has the lowest CV when it is dropped.

  for (i in 2:p) {
    drop_var <- rnames[i] #variable to be deleted
    f <- formula(current_best) #current formula
    f <- as.formula(paste(f[2], "~", paste(f[3], drop_var, sep=" - ")))
    # modify the formula to drop the chosen variable (by subtracting it)
    new_fit <- lm(f)  # fit the modified model
    compare[i] <- CV(new_fit)[1]
  }

  remove_var <- rownames(compare)[which.min(compare)]
  # drop is the varibale that is associate with the lowest CV
  update_cv <- compare[which.min(compare)]

  if (update_cv > best_cv) break # we should not continue
  # if dropping a variable will not improve CV

  write(paste("--- Dropping", counter, remove_var , update_cv, "\n"), file="")
  # output the variables we are dropping

  f <- formula(current_best) #current formula
  f <- as.formula(paste(f[2], "~", paste(f[3], remove_var , sep=" - ")))
  # modify the formula to drop the chosen variable (by subtracting it)
  current_best <- lm(f) # we now have a new best model
  counter <- counter + 1L # update our counter
} #end the while loop

print(current_best)
CV(current_best)


Unit test best-practices in Python I'm a Java/C coder getting started with some Python for a side-project.  I've written some unit tests for a recent side-project I'm working on, but I have a sinking suspicion I'm just writing Java-code, translated into Python.

I've posted a small sample of some unit-tests I wrote.  I'd really appreciate any advice in how to make the code more Pythonic (best-practices, conventions, etc).

import unittest
import urllib2
from connectionstate import ConnectionState

class Test(unittest.TestCase):
  """Unit tests for the ConnectionState"""

  def test_connectionstate_reports_bad_network_connection(self):
    """ConnectionState reports a failure when currently disconnected"""

    #Set up ConnectionState around mock
    always_disconnected = mock_connection_check_func_always_disconnected
    connstate_with_mock = ConnectionState(always_disconnected)

    actual_state = connstate_with_mock.is_connected()
    self.assertEqual(actual_state, False)

  def test_connectionstate_detects_good_network_connection(self):
    """ConnectionState should report success when we can connect"""
    #Set up ConnectionState around mock
    always_connected = mock_connection_check_func_always_connected
    connstate_with_mock = ConnectionState(always_connected)

    actual_state = connstate_with_mock.is_connected()
    self.assertEqual(actual_state, True)

  def test_connectionstate_remembers_disconnections(self):
    """If the internet connection drops and then comes back, ConnectionState
    can remember that the connection dropped temporarily
    """
    #Set up ConnectionState around mock
    sometimes_connected = mock_conn_every_other_conn_succeeds
    connstate_with_mock = ConnectionState(sometimes_connected)

    #Call is_connected a few times so get an unsuccessful connection
    for count in range(0, 3):
      connstate_with_mock.is_connected()

    actual_result = connstate_with_mock.has_connection_dropped()
    self.assertEqual(actual_result, True)

  def test_connectionstate_doesnt_bring_up_ancient_history(self):
    """has_connection_dropped only reports failures that have happened
    since the last call to has_connection_dropped.  Calling
    """
    #Set up ConnectionState around mock
    sometimes_connected = mock_conn_every_other_conn_succeeds
    connstate_with_mock = ConnectionState(sometimes_connected)

    #Call is connected a few times so an unsuccessful connection is reported
    for count in range(0, 3):
      connstate_with_mock.is_connected()

    #Call once to clear failures
    connstate_with_mock.has_connection_dropped()

    actual_result = connstate_with_mock.has_connection_dropped()
    self.assertEqual(actual_result, False)

#Begin mock-connection functions
def mock_connection_check_func_always_disconnected(request=None, timeout=1):
  raise urllib2.URLError("Unable to connect to network")
def mock_connection_check_func_always_connected(request=None, timeout=1):
  pass
def mock_conn_every_other_conn_succeeds(request=None, timeout=1):
  #Initialize 'static' call-counter variable
  if "call_counter" not in mock_conn_every_other_conn_succeeds.__dict__:
    mock_conn_every_other_conn_succeeds.call_counter = 0;

  mock_conn_every_other_conn_succeeds.call_counter += 1

  if mock_conn_every_other_conn_succeeds.call_counter % 2 == 0:
    mock_connection_check_func_always_disconnected(request, timeout)
  else:
    mock_connection_check_func_always_connected(request, timeout)
#End mock-connection functions

if __name__ == "__main__":
  unittest.main()

I have a few nitpicks:

If you use assertFalse and assertTrue instead of assertEquals(something, True/False) where appropriate, the output of your unit tests will make more sense when they fail. (http://docs.python.org/2/library/unittest.html#assert-methods)
The argument order of assertEquals is expected, actual. Again, if you use the correct order the error messages will work as intended.
A more common style for when you want a block of code to execute a fixed number of times (like in for count in range(0, 3):) is: for _ in xrange(3):. The _ tells the reader that the value is unimportant, and xrange uses less memory since it doesn't create a list.


Counting digits of numbers Write a program that loops prompting for positive or zero integers of
  data type long. Then the number of digits the integer consists of (in
  decimal representation) should be printed to stdout. Entering a
  negative number immediately stops the program. Output examples: 0 has
  1 digit. 999 has 3 digits. etc.

(And I think that I am not allowed to use any finished helpful function in any C library. It is just 'plain' C coding or so. I don't know how to describe it. We haven't learned arrays, strings etc. yet)

I heard the tips and have already adjusted it in the code below, so this is v2.

Questions:

I found two solutions yet to get this exercise/program 'working'. You can see it in v2 and v3, I've posted below.

I use VS 2017, i.e. when I 'Start without debugging' (CTRL+F5), then the CMD will be closed immediately, but when I go to Project-Properties/Linker/System and choose "Console (/SUBSYSTEM:CONSOLE) at SubSystem, the CMD-window stays.

Now the comparison between v2 and v3:

I.e. when I use v2 I do not need to set a specific "SubSystem" as I told you before. I think it is because of getchar() in the while() is waiting for an input.

But I can also a while without 'EOF', which some guys on SO suggested before, so I made v3 that uses the while without getchar(). But to see my program, I have to set a "SubSystem" as I described before.

What solution is 'better'? And what would you improve?

Here is my C code v2:

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

int main(void)
{
  long number = 0;
  int n = 0;
  int i = 0;

  do
  {
    printf("Enter a number: ");
    fflush(stdout);
    i = scanf_s("%ld", &number);

    if(i == 1)
    {
      if (number < 0)
      {
        exit(EXIT_FAILURE);
      }
      else if (number == 0)
      {
        n = 1;
      }
      else
      {
        while (number != 0)
        {
          number /= 10;
          n++;
        }
      }
    }
    else
    {
      printf("Error - You've probably entered nothing."); //Idk what to write here, because the return-value of scanf_s can be less then 1 in this case so it must be zero,
    }                                                     //if nothing is entered and somehow the program makes it into this else.

    printf("The number you've entered has %d digits.\n\n", n);
    n = 0;

  } while (getchar() != 'EOF');

  return 0;
}

Here is my C-Code v3:

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

int main(void)
{
  long number = 0;
  int n = 0;
  int i = 0;
  int buffer = 0;

  do
  {
    printf("Enter a number: ");
    fflush(stdout);
    i = scanf_s("%ld", &number);

    buffer = number;

    if(i == 1)
    {
      if (number < 0)
      {
        exit(EXIT_FAILURE);
      }
      else if (number == 0)
      {
        n = 1;
      }
      else
      {
        while (number != 0)
        {
          number /= 10;
          n++;
        }
      }
    }
    else
    {
      printf("Error - You've probably entered nothing."); //Idk what to write here, because the return-value of scanf_s can be less then 1 in this case so it must be zero,
    }                                                     //if nothing is entered and somehow the program makes it into this else.

    printf("The number you've entered has %d digits.\n\n", n);
    n = 0;

  } while (buffer >= 0);

  return 0;
}

There's a possibility that user may enter invalid characters at input. If that's the case, subsequent calls to scanf() fail, as these characters remain in buffer of input stream, thus printing error message infinitely.

You can use construct like the one below to avoid that. It looks like this may serve you better based on the requirement.

while (true)
{
    if (scanf("%ld",&input) != 1 || input < 0)
    {
        fprintf(stderr,"error: invalid input\n");  /* arbitrary */
        exit(EXIT_FAILURE);
    }
    else
    {
        // rest of code
    }
}


Replace strings in a file I had to create an executable to search and replace strings in a file. This is to be used in my installer for text file manipulation. I have various placeholders in configuration files that I need to replace with proper values after querying the end user system.

I have written the following code. This being my first real experience with C#, I do not expect the code to be any good. I have come up with this code based upon a couple of hours of reading MSDN and stackoverflow. I needed someone to review this. There is no one greater than the community to do this.

Please review this and let me know what modification are necessary.
I am specifically concerned about the exception handling.
I have included two try-catch blocks.

My logic:

If I have all operations under one try-catch block, the failure of one process will block the execution of all others. That is, if my

text = Regex.Replace(text, args[1], args[2]);

has an exception and if I have

WriteLog(args[0], args[1], args[2], strException, intStatus);

also in the same try block, it will not execute and I will not have any logs.

Also, if I have the

WriteLog(args[0], args[1], args[2], strException, intStatus);

in the catch block, any exception in that method cannot be caught. Yes I can have try-catch in the WriteLog method but I see no problems in my approach too.

So first, I have a try-catch for file manipulation and then another for log write.

Please ignore the log path(hardcoded as D:).

class Program
{
    static int Main(string[] args)
    {
        //args[0] = The file that needs modification
        //args[1] = The string to replace
        //args[2] = The string with which to replace args[1]
        int intStatus = 0;
        string strException = "";

        //Get out immediately if no/improper arguments are found
        if (args.Length < 3)
        {
            intStatus = 1;
            return intStatus;
        }

        try
        {
            StreamReader streamReader = new StreamReader(args[0]);
            String text = streamReader.ReadToEnd();
            streamReader.Close();
            text = Regex.Replace(text, args[1], args[2]);
            StreamWriter streamWriter = new StreamWriter(args[0]);
            streamWriter.Write(text);
            streamWriter.Close();
        }
        catch (Exception ex)
        {
            //text file manipulation failed
            strException = ex.ToString();
            intStatus = 2;
        }

        try
        {
            WriteLog(args[0], args[1], args[2], strException, intStatus);
        }
        catch
        {
            //log write failed
            if (intStatus == 0)
            {
                intStatus = 3; //if intStatus = 1 when we get here, no need to modify the value(its a complete failure)
                               //if intStatus = 0 when we get here, make intStatus = 2 so as to clearly distinguish a "log write failed"
                               //error from a "text file manipulation failed" error
            }
        }

        return intStatus;
    }

    //The new Logger for the exe
    static void WriteLog(string arg0, string arg1, string arg2, string exception, int status)
    {
            string strGrepLogFileName = string.Format("D:\\TempLogs\\Grep-{0:yyyy-MM-dd_hh-mm-ss-tt}.log", DateTime.Now);
            StreamWriter GrepLog = new StreamWriter(strGrepLogFileName, true);
            GrepLog.WriteLine("Argument 1: " + arg0);
            GrepLog.WriteLine("Argument 2: " + arg1);
            GrepLog.WriteLine("Argument 3: " + arg2);
            GrepLog.WriteLine("Status: " + status.ToString());
            GrepLog.WriteLine("Exception: " + exception);
            GrepLog.Close();
    }
}

EDIT 1:
It is an absolute pity that I cannot mark multiple answers.

Use the params keyword

//The new Logger for the exe
static void WriteLog(string exception, int status, params string[] arguments)
{
    string logFile = string.Format("D:\\TempLogs\\Grep-{0:yyyy-MM-dd_hh-mm-ss-tt}.log", DateTime.Now);
    StreamWriter grepLog = new StreamWriter(logFile, true);
    grepLog.WriteLine("Status: " + status.ToString() + " (0 for PASS, 1 for FAIL)");
    grepLog.WriteLine("Exception: " + exception);
    // TODO: Consider logging "no arguments" if that is the case.
    for(int i=0; i<arguments.Length; i++)
    {
        grepLog.WriteLine("Argument " + i + ": " + arguments[i]);
    }
    grepLog.Close();
}

Use an enum for status

Since the status has 3 states, use an enum for it. This will help you avoid mistakes.

Why do you try-catch while logging?

If you don't see any particular possibility for the log to fail, then simply assume it and don't try-catch the logging.

If you want the logging to no matter what not break the application with exceptions, and you cannot fix those exceptions (why?), then I would suggest putting the try-catch inside the logging method.

Main - exception should catch, log, and return

I would have the exception log the error and return, instead of continuing normally.

Alternatively, I would catch, log, and re-throw: try { ... } catch (Exception e) { WriteLog(...); throw; }

Use camelCase for local variables

StreamWriter grepLog;
Using a starting underscore (_grepLog) is also common for private fields.

Follow the de-facto conventions. (See e.g. https://stackoverflow.com/questions/2976627/on-c-sharp-naming-conventions-for-member-variables )

Avoid Hungarian Notation

In string strGrepLogFileName, the part "str" brings nothing new.
If you change the variable's type (e.g. to an Uri) without changing its meaning, you should not have to rename the variable, and using Hungarian Notation you do.


Mock/Stub out filesystem in F# for unit testing I'm looking to do some basic verification testing on my functions that write to the filesystem.

I took a hint from here on how to mock out the filesystem using an interface, but I'm kinda bummed on how the FileSystemOperations type needs to be passed along through every function. Any ideas how to improve this, or where I went wrong?

I'm rather new to FP, so any feedback on other design issues are welcome as well.

module FileSystem =

    open System.IO

    type IFileSystemOperations =
        abstract member copy: string -> string -> bool -> unit
        abstract member delete: string -> unit
        abstract member readAllBytes: string -> byte[]
        abstract member createDirectory: string -> DirectoryInfo
        abstract member directoryExists: string -> bool

    type FileSystemOperations () =
        interface IFileSystemOperations with
            member this.copy source destination overwrite =
                File.Copy(source, destination, overwrite)

            member this.delete path =
                File.Delete path

            member this.readAllBytes path =
                File.ReadAllBytes path

            member this.createDirectory path =
                Directory.CreateDirectory path

            member this.directoryExists path =
                let fileInfo = FileInfo path
                fileInfo.Directory.Exists

module FileMover =

    open FileSystem

    let private ensureDirectoryExists (fileSystemsOperations:IFileSystemOperations) destination =
        let directoryExists = fileSystemsOperations.directoryExists destination
        if not (directoryExists) then
            fileSystemsOperations.createDirectory destination |> ignore

    let private compareFiles (fileSystemsOperations:IFileSystemOperations) moveRequest =
        let sourceStream = fileSystemsOperations.readAllBytes moveRequest.Source
        let destinationStream = fileSystemsOperations.readAllBytes moveRequest.Destination
        sourceStream = destinationStream

    let private copyToDestination (fileSystemsOperations:IFileSystemOperations) (moveRequest:MoveRequest) =
        ensureDirectoryExists fileSystemsOperations moveRequest.Destination
        fileSystemsOperations.copy moveRequest.Source moveRequest.Destination true

    let private deleteSource (fileSystemsOperations:IFileSystemOperations) (move:Move) =
        fileSystemsOperations.delete move.Request.Source

    let private moveFile (fileSystemsOperations:IFileSystemOperations) moveRequest =
        copyToDestination fileSystemsOperations moveRequest
        let filesMatch = compareFiles fileSystemsOperations moveRequest
        let move = match filesMatch with
                   | true -> {Request = moveRequest; Result = Success}
                   | false -> {Request = moveRequest; Result = Failure BytesDidNotMatch}

        deleteSource fileSystemsOperations move

    let move (fileSystemsOperations:IFileSystemOperations) targetPath pictures =
        let moveFile = moveFile fileSystemsOperations

        pictures
        |> getMoveRequests targetPath
        |> Seq.map moveFile

Update (for reference & anyone interested)

Based on the answers below, here is how I currently refactored the code. It's a work in progress, but it's already more easily testable (main goal) and the responsibilities seem to fit better as well (eg: injecting ensureDirectoryExists into copyToDestination instead of into moveFile).

Thanks for your answers!

Even though the SOLID principles are known to be principles related to Object-Oriented Design, I'd still take a cue from the Dependency Inversion Principle. As APPP states it (ch. 11): "clients [...] own the abstract interfaces".

You can apply this principle in FP as well, in the sense that you can start by defining the overall behaviour of the function you're interested in, and then see what happens.

As an example, I'd approach the ensureDirectoryExists function like this:

let private ensureDirectoryExists destination =
    let directoryExists = dirExists destination
    if not directoryExists then
        createDir destination

Now, the above version doesn't compile, because dirExists and createDir aren't defined. However, that's easy to fix: just promote them to function arguments:

let private ensureDirectoryExists dirExists createDir destination =
    let directoryExists = dirExists destination
    if not directoryExists then
        createDir destination

Viola: now you have a higher-order function; it's type is

('a -> bool) -> ('a -> unit) -> 'a -> unit

No interfaces are required.

This is already intriguing for a couple of reasons:

It's no longer about files or directories: 'a could be string values containing file names or directory names, but it could also be DirectoryInfo or something not at all related to files.
Although it looks complex, it only involves two types: 'a and unit. That should make us pay attention as well.
You can use partial application to apply the ensureDirectoryExists with the two first arguments, and then use this partially applied function to test any number of different directories.

You can still 'implement' dirExists and createDir, and apply them like this:

open System.IO

let directoryExists path =
    let fileInfo = FileInfo path
    fileInfo.Directory.Exists

let ensureDirectoryExists' =
    ensureDirectoryExists directoryExists (Directory.CreateDirectory >> ignore)

Notice that the createDir 'implementation' is so simple that I chose to inline it. I could also have inlined the dirExists 'implementation' with (fun path -> (FileInfo path).Directory.Exists), but I chose to do this as a separate function in order to illustrate the various options available.

The partially applied ensureDirectoryExists' function has this simple function signature: (string -> unit), which is comparable to the OP version of the function.

While I think I'll leave the rest of the functions as an exercise, I'd like to return to the thought-provoking signature of the ensureDirectoryExists function:

('a -> bool) -> ('a -> unit) -> 'a -> unit

There's nothing here about files or directories, so a more generic name might be in place. It looks like the Tester-Doer idiom, so we could call it testDo instead.

One of the great benefits of making the interface more generic is that you don't need to involve the file system at all for testing. Instead, you can test this function using integers, strings, or some other 'easy' values. If the logic is unconstrained generic and works for numbers and strings, it'll also work for directories and files.


WebService async WebMethod How do I simplify this method?  Is handling an exception in this method a good idea?

[WebMethod]
public string GetMDSToken(string xuid)
{
    var tokenTask = _mapTokenService.GetMappedSSOTokenAsync(xuid);
    var tokenTimestampTask = _SSOFrontendService.GetSSOTokenTimestampAsync();

    Task.WhenAll(tokenTask, tokenTimestampTask);

    if (tokenTimestampTask.Result.Status != ResultStatus.OK)
        return _serializer.Serizalize(tokenTimestampTask.Result);

    if (tokenTask.Result == null)
    {
        var response = _handledResponseFactory.CreateErrorResponce(ResultStatus.Business, NotLogginErrorMessage);
        return _serializer.Serizalize(response);
    }

    try
    {
        var mappedToken = tokenTask.Result;
        if (mappedToken.SSOTokenTimestamp != tokenTimestampTask.Result.Content)
            UpdateSSOToken(mappedToken);

        var MDSTokenTask = _MDSFrontendService.GetToken(mappedToken.SSOToken);

        Task.WhenAll(MDSTokenTask);
        return _serializer.Serizalize(MDSTokenTask.Result);
    }
    catch (Exception ex)
    {
        var response = _handledResponseFactory.CreateExceptionResponce(ex);
        return _serializer.Serizalize(response);
    }
}

I can't speak for the language you a using specifically, but I have always been lead to believe that declaring variables within if and try statements is bad form. Can't say I've ever seen one in a catch statement, but I believe the same principle might apply.

You seem to be wrapping a lot in that try block. Do you expect that

var mappedToken = tokenTask.Result;

could actually throw an exception? If not, I would personally recommend removing it from that code block.

To be clear, how much code to put in a try block would be a matter of opinion, but are you obeying the single responsibility principle? I would posit that the code would be simplified if the reader knew what is expected to throw an exception at a glance. Your try block is responsible for 6 different tasks by my count. Should the reader assume that all of those tasks are capable of faulting, and that the resulting exception should be the same regardless of which part failed and which part was successful? If UpdateSSOToken were to execute but Task.WhenAll failed are we in the same boat we would be in if UpdateSSOToken had failed? Something to consider.

As for is it a good idea to have the exception at all-

  The method you choose depends on how often you expect the event to
  occur.

To help you determine that you should consider the following about the WhenAll method of Tasks-

  If any of the supplied tasks completes in a faulted state, the
  returned task will also complete in a Faulted state

This answer cites some useful information on exception with tasks, including

  An Async Primer by Bill Wagner

which goes into detail about the await pattern. I would suggest at least reading the primer if none of the other links, I believe it directly addresses your question.


Shift register library design I am creating a small library for controlling bars of leds and have some doubts about my design.

I have 2 basic entities in the design so far:

BarService (and implementation)

template<class T>

class BarService{
public:
  virtual void turnOnLeds(T* bar, int end) = 0;

class PinBarService : public BarService<PinBar>{
public:
  virtual void turnOnLeds(PinBar* bar, int end);
  void initPins(PinBar* bar);
};

class OpampService : public BarService<OpampBar>{
public:
  virtual void turnOnLeds(OpampBar* bar, int end);
  void initPins(OpampBar* bar);
};

Bar models:

class Bar{
private:
  int ledCount;
public:
  Bar(int ledCount);
  int getLedCount();
};

class BasicBar : public Bar{
private:
  int* pins;
public:
  BasicBar(int ledCount, int pins[]);
  int getPin(int index);
};

class OpampBar : public Bar{
private:
  int input;
public:
  OpampBar(int ledCount, int input);
};

My reasoning for separating these is that I feel the Bar models should only contain information about themselves, thus allowing me to create more classes later on which use that information rather than having the bar itself perform actions (like writing to hardware).

The reason I'm having doubts is that by using templates every implementation of the service needs to accept a different model.

My question comes down to: Am I going overboard with separating the model from its actions? and are templates a good solution here? and overall, what could be improved about this design?

Generally "template vs polymorphism" is a complex topic. A simple rule of thumb: if it is high level (e.g., service you instantiate a few times) go virtual, if it is low level (you need to create a array of million these objects) then go template.

In your case... honestly your interface makes little sense to me - it is difficult to make any advices.

Say, is there any point in Bar class? It simply holds an int and serves no other purpose. Just having a common function and variable doesn't imply that you need to have a shared basic class. If it had virtual destructor then I could understand to a certain degree.

Why OpampBar is a class? It just holds two integers. Why not make it a struct with public members? Why make it unnecessarily complicated?

What is this BasicBar and what is this pins raw pointer? Is it a pointer to an array? A pointer to data that doesn't belong to it? Why not use std::vector<int>? There are lots of well made classes and functions in STL, why not utilize them?

What is the point of BarService? It is an interface class. I know of two purposes of interface classes: (1) To allow your program to switch between different derived classes easily. (2) Hide implementation. Is this the case? (Well, it can also serve as poor man's concept substitude... but I doubt that you use any SFINAE).

You need to watch and read tutorials on C++. There are plenty for free - just google it. There are lots of CppCon videos on YouTube where they explain additions to C++ (you gotta learn what was added in C++11/14 as well as what STL already had for 20 years).


Unit testing a servlet in a meaningful way I have to write a unit test for the method processRequest in the servlet below and I'm wondering if:

It just shouldn't be done.
The class should be rewritten / refactored to allow easier unit testing. Suggestions as to how?
There is a meaningful way of getting the value of response.getWriter().print(result), which is all the method returns.

Method in question:

protected void processRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

    response.setContentType("text/plain;charset=UTF-8");
    session = request.getSession(false);
    User sessionUser = (User) session.getAttribute("user");
    int userId = sessionUser.getId();

    if (sessionUser != null) {
        switch (request.getParameter("function")) {
            // UPLOAD IMAGE FUNCTION
            case "uploadimg":
                // save image
                final String path = getServletContext().getRealPath("") + "/resources/img/profile/tmp/";
                String imagePath = imageService.uploadImage(request, path, userId);

                // return image path for use in frontend java script
                response.getWriter().print(getServletContext().getContextPath() + "/resources/img/profile/tmp/" + imagePath);
                break;

            // SAVE IMAGE FUNCTION
            case "save":
                // move image from temp folder to permanent storage
                final String pathOrigin = getServletContext().getRealPath("") + "/resources/img/profile/tmp/";
                final String pathDestination = getServletContext().getRealPath("") + "/resources/img/profile/";
                boolean result = imageService.moveImage(pathOrigin, pathDestination, userId);

                // Returns true whether or not the image was saved successfully
                response.getWriter().print(result);
                break;
        }

        response.getWriter().flush();
        response.getWriter().close();
    }
}

@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    processRequest(request, response);
}

Bug: You're verifying if sessionUser is different from null, but you're doing sessionUser.getId() before you're sure if it's null or not. If it was null, doing getId would have thrown a NullPointerException

int userId = sessionUser.getId();

if (sessionUser != null) {


Program that prints star pattern I have a program that prints the star pattern.

Starr Pattern is:

          *
          *
          *
          *
  * * * * * * * * *
          *
          *
          *
          *

My code is:

$n=5;

for($i=0; $i<$n; $i++){

  for($k=$n-$i; $k>0; $k--){
      echo ' '.' ';
  }
  for($j=0; $j< ($tot = $i+1); $j++){

      if($i==$j && $tot<$n){
        echo '* ';
      }else{
          if($tot<$n){
            echo ' '.' ';
          }else{
             echo '* ';
          }
      }

  }

  if($i == ($n-1)){
      for($l=$n-1; $l>0;$l--){
         echo '* ';
       }
     }
  echo '
';

}

for($i=0;$i<$n;$i++){
  for($k=$n-$i; $k>0; $k--){
      echo ' '.' ';
  }

  for($j=0; $j< ($tot = $i+1); $j++){

      if($i==$j && $tot<$n){
        echo '* ';
            }else{
              echo ' '.' ';
            }
    }
  echo '
';
}

My question is that how can I calculate the complexity of this code?

And also Is there a good way to reduce loops?

It looks nice. Can you please make me understand how it works

For example

 function f($n){$s='';$b=str_pad('*',($m=$n*2+1),' ',2);for($i=0;$i<$m;++$i)$s.=($i==$n)?str_repeat('*',$m):$b;return chunk_split($s, $m);}

Output

     *
     *
     *
     *
     *
***********
     *
     *
     *
     *

Sandbox

Let me write it in a way that is easier to see

$n = 5;
$row_len = $n*2+1;
$output = '';
//pad $n spaces on each side of *
$default = str_pad('*',$row_len,' ',STR_PAD_BOTH); //is = "     *     "

//loop once for each vertical row (same as width)
for($i=0;$i<$row_len;++$i){
    if($i==$n){
        //do the center line all *'s
        $output .= str_repeat('*',$row_len);//is = "***********"
    }else{
        //use the default line from above
        $output .= $default;
    }
}

//now we have a single line that is the length squared so we can chunk it
//into equal parts, to make a square
echo chunk_split($output, $row_len);  //is = "     *          *     ***********"

Sandbox

Basically we can create the "     *       " row by using string pad.  Because we can pad spaces on both sides of a * up to the length of a row  ($n*2)+1.

Then the center row, is just * so we can use string repeat to the length of the row for that line.

Last we take our big huge line of spaces and * and split it into chunks (\n) on the length of our row we want.