Given a shape, return all triangles that can be formed Given a shape, get all triangles that can be possible by connecting the points.

  Example: given 3 points, only 1 triangle is possible, but given a pentagon, 10 are possible.

Also the combination formula is \$\dfrac{n!}{r!(n-r)!}\$.  Does it mean \$O(n!)\$ is the complexity?

Looking for:

Code review optimization and best practices.
Verifying complexity:

Time:  \$O\left(\dfrac{n!}{r!  (n-r)!}\right)\$ - which I guess is the same as \$O(n!)\$.

Space:  \$O\left(\dfrac{n!}{r! (n-r)!} \cdot r\right)\$ - space occupied by the list to be returned.

public final class GetTriangles {

    private GetTriangles() {}

    /**
     * Returns the list of all the triangles possible, given the input shape.
     * If the less than three points are returned it causes illegal argument exception
     */
    public static List<int[]> combinate (int n) {
        if (n < 3) throw new IllegalArgumentException("The input shape has less than 3 points:  " + n);
        final List<int[]> list = new ArrayList<int[]>();
        for (int i = 1; i < n - 1; i++) {
            compute(i, new int[3], 0, n, list);
        }
        return list;
    }

    private static void compute(int startFrom, int[] a, int arrPosition, int n, List<int[]> list) {
        if (arrPosition == a.length - 1) {
            a[arrPosition] = startFrom;
            list.add(Arrays.copyOf(a, a.length));
            return;
        }

        a[arrPosition] = startFrom;

        for (int i = startFrom + 1; i <= n; i++) {
            compute(i, a, arrPosition + 1, n, list);
        }
    }

    public static void main(String[] args) {
       final List<int[]> list = combinate(5);

       int[] a1  = {1, 2, 3};
       int[] a2  = {1, 2, 4};
       int[] a3  = {1, 2, 5};
       int[] a4  = {1, 3, 4};
       int[] a5  = {1, 3, 5};
       int[] a6  = {1, 4, 5};
       int[] a7  = {2, 3, 4};
       int[] a8  = {2, 3, 5};
       int[] a9  = {2, 4, 5};
       int[] a10 = {3, 4, 5};

       final List<int[]> list1 = Arrays.asList(a1, a2, a3, a4, a5, a6, a7, a8, a9, a10);

       for (int i = 0; i < list.size(); i++) {
           assertArrayEquals(list.get(i), list1.get(i));
       }
    }
}

No, it doesn't mean \$O(n!)\$ complexity.  The \$(n-r)\$ in the denominator cancels most of the terms in the numerator.  For choosing three points to make a triangle, \$r\$ = 3, and so your actual expression is \$\frac{(n)(n-1)(n-2)}{3!}\$ => \$\frac{(n^3-3n^2+2n)}{3!}\$ => \$O(n^3)\$.

So you should expect \$O(n^3)\$ time, and \$O(3*n^3)\$ => \$O(n^3)\$ space.

Next thought - if you are going to be working with triangles, you need to know where the points are (so that you can check whether or not the points are co-linear).  What your code currently does is seek out every 3-tuple combination of vertices; so you are (possibly) missing part of the problem.

That said, breaking the triangle problem into two distinct pieces is a good idea; because you do need to enumerate the vertices anyway, and treating them as ordered tuples gives you a good way to avoid extra work.

But trying to enumerate the tuples using recursion... that's a bad idea.  Better would be to simply enumerate the combinations using squashed ordering.  Thomas Andrews has a nice illustration of that technique on a more complicated problem.


Function to test if the current time is within a time window I have written the following code that tests if the current time is within a time window. It seems to work ok with my test cases, but I would be interested if anyone can see any possible problems or has any ideas for improvements?

Public Function IsCurrentTimeBetween(ByRef dteStartTime As DateTime, ByRef dteEndTime As DateTime) As Boolean

    Dim dteStart As DateTime, dteEnd As DateTime, dteCurrent As DateTime

    'strip off any date portion of the dates
    dteStart = Format(dteStartTime, "HH:mm:ss")
    dteEnd = Format(dteEndTime, "HH:mm:ss")
    dteCurrent = Format(DateTime.Now, "HH:mm:ss")

    If dteEnd < dteStart Then
        'the times span midnight
        If dteCurrent < dteStart And dteCurrent < dteEnd Then
            dteCurrent = dteCurrent.AddDays(1)
        End If
        dteEnd = dteEnd.AddDays(1)
    End If

    Return (dteStart <= dteCurrent AndAlso dteCurrent <= dteEnd)

End Function

Usage:

'Work out if we are between 5 to midnight and quarter past midnight
Debug.WriteLine("Is in midnight window? " + IsCurrentTimeBetween("23:55", "00:15").ToString)

You are taking a DateTime, formatting it to a string, then assigning that to another datetime, just to get the time of day. That won't compile with Option Strict On. What you want is provided by DateTime.TimeOfDay.

I would also change ByRef to ByVal in the parameter list.

Once you are comparing timespan instead of date, you don't have to do date addition. You can just detect the wrap-around case and you only have to check endTime.

 Public Function IsCurrentTimeBetween(ByVal dteStartTime As DateTime, ByVal dteEndTime As DateTime) As Boolean

            Dim startTime, endTime, currentTime As TimeSpan

            startTime = dteStartTime.TimeOfDay
            endTime = dteEndTime.TimeOfDay
            currentTime = Now.TimeOfDay

            If endTime < startTime Then
                'the times span midnight
                Return (currentTime <= endTime)
            Else
                'the times do not span midnight
                Return (startTime <= currentTime AndAlso currentTime <= endTime)
            End If

End Function


Advent of Code 2017 Day 6 (part 1) in Functional Programming (FP) I wanted to practice functional programming (FP) without using any library but using vanilla JS only. So I took a problem from Advent of Code (the 1st part of Day 6):

http://adventofcode.com/2017/day/6

  A debugger program here is having an issue: it is trying to repair a
  memory reallocation routine, but it keeps getting stuck in an infinite
  loop.

  In this area, there are sixteen memory banks; each memory bank can
  hold any number of blocks. The goal of the reallocation routine is to
  balance the blocks between the memory banks.

  The reallocation routine operates in cycles. In each cycle, it finds
  the memory bank with the most blocks (ties won by the lowest-numbered
  memory bank) and redistributes those blocks among the banks. To do
  this, it removes all of the blocks from the selected bank, then moves
  to the next (by index) memory bank and inserts one of the blocks. It
  continues doing this until it runs out of blocks; if it reaches the
  last memory bank, it wraps around to the first one.

  The debugger would like to know how many redistributions can be done
  before a blocks-in-banks configuration is produced that has been seen
  before.

  For example, imagine a scenario with only four memory banks:

  The banks start with 0, 2, 7, and 0 blocks. The third bank has the most blocks, so it is chosen for redistribution.
  Starting with the next bank (the fourth bank) and then continuing to the first bank, the second bank, and so on, the 7 blocks are spread
  out over the memory banks. The fourth, first, and second banks get two
  blocks each, and the third bank gets one back. The final result looks
  like this: 2 4 1 2.
  Next, the second bank is chosen because it contains the most blocks (four). Because there are four memory banks, each gets one block. The
  result is: 3 1 2 3.
  Now, there is a tie between the first and fourth memory banks, both of which have three blocks. The first bank wins the tie, and its three
  blocks are distributed evenly over the other three banks, leaving it
  with none: 0 2 3 4.
  The fourth bank is chosen, and its four blocks are distributed such that each of the four banks receives one: 1 3 4 1.
  The third bank is chosen, and the same thing happens: 2 4 1 2.

  At this point, we've reached a state we've seen before: 2 4 1 2 was
  already seen. The infinite loop is detected after the fifth block
  redistribution cycle, and so the answer in this example is 5.

  Given the initial block counts in your puzzle input, how many
  redistribution cycles must be completed before a configuration is
  produced that has been seen before?

  Your Input: 2    8   8   5   4   2   3   1   5   5   1   2   15  13  5   14

My solution in FP:

const banks = `2\t8\t8\t5\t4\t2\t3\t1\t5\t5\t1\t2\t15\t13\t5\t14`;

const parse = input => input
  .split('\t')
  .map(c => parseInt(c));

const copy = input => input.slice();

const isUnique = toCheck => state => toCheck.toString() !== state.toString();

const INPUT = parse(banks);

const redistribute = (input, index, toBeDistributed) => {
  if (!toBeDistributed) return input;
  const nextIndex = index + 1;
  const nextInput = input;
  ++nextInput[nextIndex % input.length];
  return redistribute(nextInput, nextIndex, --toBeDistributed);
};

const solveDaySix = input => {
  let banks = copy(input);
  const states = [input];
  let cycle = 0;
  while (true) {
    ++cycle;
    const max = Math.max(...banks);
    const index = banks.indexOf(max);
    banks[index] = 0;
    banks = copy(redistribute(banks, index, max));
    const stateIsUnique = isUnique(banks);

    if (!states.every(stateIsUnique)) break;
    states.push(copy(banks));
  }
  return cycle;
};

console.log("solution ", solveDaySix(INPUT));

Do you think my solution is consistent with the idea of FP (I used loops and mutate variables inside my functions)? Is there a better way to write the solution in FP? Any other improvement suggestions are welcomed.

Explanation

Algorithm

First we have to break down witch steps we have to do:

split bank string to an array
change all string values to a number
reallocate

reallocationRoutine

The algorithm above would look like this in JavaScript:

const reallocationRoutine = pipe (
    split (' '),                    // split number by space
    map (Number),                   // cast String to a number
    reallocation ([]) (0)           // reallocate with initial values
)

reallocationRoutine ('0 2 7 0')     // will return 5

Pipe is a kind of function composition.

reallocation

  I used loops and mutate variables inside my functions

You can avoid muting loops and variables by writing recursive functions. The cool thing about recursive functions is that the logic can be broken down into small steps:

we need the first index of the biggest number
we need the biggest number itself
if our store (memoryBanks) includes the current state (input)

we return the current cycleCount

else we call reallocation again, but there for we need to:

update the memoryBanks with the current input
increment the cycleCount
and redistribute the input

So it can looks like:

const reallocation = memoryBanks => cycleCount => input => {
    const firstIndexOfMax = input.indexOf(Math.max(...input))
    const max = input[firstIndexOfMax]
    return contains(memoryBanks, input)
        ? cycleCount
        : reallocation
            ( memoryBanks.concat ([input]) )
            ( increment (cycleCount) )
            ( redistribute (input) (firstIndexOfMax) (max) )
}

I used the ternary operator for conditions. The reason why is much more than only because it is shorter.
A if should always have an else and the ternary operator forces you to.

redistribute

Instead of putting the entire logic in redistribute, I wrote a function calledloop that traverses an array for a certain number of steps (steps) and a function (fn) on the current index of the passed array (xs) for each step. So we can create a more generic solution - one of the main ideas of FP.

const loop = fn => xs => index => steps =>
    steps <= 0
        ? xs
        : loop
            ( fn )
            ( changeArrayValueOnIndex(index, fn(xs[index]), xs) )
            ( getNextIndex(xs, index) )
            ( steps - 1 )

redistribute is a special form of loop where we increment each index we reaches and modify the passed input xs so, that the start value gets set to 0

const redistribute = xs => index =>
    loop (increment) (changeArrayValueOnIndex(index, 0, xs)) (getNextIndex(xs, index))

Working Example

Overall, the code is longer than the imperative solution, but only because I've encapsulated a lot of logic into its own function like increment and loop. However, these are now functions that we could reuse in our next project and, in addition, we can test everything much easier :)

// helper functions
const pipe = (...fns) => fns.reduce((f, g) => (...args) => g(f(...args)))

const map = fn => arr =>
    arr.map(fn)

const split = delimiter => str =>
    str.split(delimiter)

const arrayIsEqual = ys => xs =>
    xs.length === ys.length
        ? xs.every((x, index ) => ys[index] === x)
        : false

const contains = (xss, ys) =>
    xss
      .map(arrayIsEqual(ys))
      .some(bool => bool)

const getNextIndex = (arr, index) =>
    arr[index + 1] != undefined ? index + 1 : 0

const increment = x =>
    x + 1

const changeArrayValueOnIndex = (index, value, array) =>
    [].concat(array.slice(0, index), value, array.slice(index + 1))

// functions for businesslogic
const loop = fn => xs => index => steps =>
    steps <= 0
        ? xs
        : loop
            ( fn )
            ( changeArrayValueOnIndex(index, fn(xs[index]), xs) )
            ( getNextIndex(xs, index) )
            ( steps - 1 )

const redistribute = xs => index =>
    loop (increment) (changeArrayValueOnIndex(index, 0, xs)) (getNextIndex(xs, index))

const reallocation = memoryBanks => cycleCount => input => {
  const firstIndexOfMax = input.indexOf(Math.max(...input))
  const max = input[firstIndexOfMax]
  return contains(memoryBanks, input)
    ? cycleCount
    : reallocation
        ( memoryBanks.concat([input]) )
        ( increment(cycleCount) )
        ( redistribute (input)(firstIndexOfMax)(max) )
}

const reallocationRoutine = pipe(
    split (' '),
    map (Number),
    reallocation ([]) (0)
)

console.log('0 2 7 0:', reallocationRoutine('0 2 7 0'))
console.log('2 8 8 5 4 2 3 1 5 5 1 2 15 13 5 14:', reallocationRoutine('2 8 8 5 4 2 3 1 5 5 1 2 15 13 5 14'))


CSS/JS slider using just data-attributes and transform:translate I needed to make a multi-page form, with slides, but without using too much js code like slick.js or the likes. So I tried to figure out how to make it really simple, but still cross-browser and responsive. I ended up with a small test using transform transitions. My question is: do you maybe see any relevant drawbacks on this technique?

The whole thing is based on 3 concepts:

If I use transforms, I'll not have scrollbars.
I can use data attributes to both track the current slide and style (move) the main strip with css
For this specific project, I can safely assume I won't never ever have more than 10 slides or so.

Notice that the demo slider does not loop. It's intentional, since for this specific project I don't want it to loop. Though, it would be fairly simple to make it loop, just change the current var accordingly.

Here is a link to the demo.

The code:

HTML

1

2

3

4

5

6

7

    <button type="button" class="prev-button">prev</button>
    <button type="button" class="next-button">next</button>

CSS (scss)

.main-box{
  max-width: 600px;
  margin: 0 auto;
}

.slide-box{
  overflow:hidden;
}

.slides{
  background-color: #656565;
  display: flex;
  flex-wrap: nowrap;
  transition: transform 300ms;

  @for $i from 1 through 20{ // assuming slides won't never be > 20
    &[data-current="#{$i}"]{ transform: translateX(-100% * ($i - 1)); }
  }

  .slide{
    height: 300px;
    min-width: 100%;
    background-color: #ffb0b0;

    &:nth-child(even){
      background-color: #ff8181;
    }
  }
}//slides

JS (jquery)

var slide_strip = $('.slides');
var slides = slide_strip.find('> *');

var next_button = $('.next-button');
var prev_button = $('.prev-button');

next_button.on('click', function(e){ goto('next'); });
prev_button.on('click', function(e){ goto('prev'); });

function goto(direction){
  var current = parseInt(slide_strip.attr('data-current'), 10);

  if(direction == 'next'){
    if(current >= slides.length) return;
    current++;
  } else {
    if(current <= 1) return;
    current--;
  }

  slide_strip.attr('data-current', current);
}

My question is: do you maybe see any relevant drawbacks on this technique?

I don't see any drawbacks with the technique.

I do however see an improvement with the functions bound to the click handlers:

next_button.on('click', function(e){ goto('next'); });
prev_button.on('click', function(e){ goto('prev'); });

These can be simplified using partially applied functions:

next_button.on('click', goto.bind(null, 'next'));
prev_button.on('click', goto.bind(null, 'prev'));

And since the else condition doesn't really check the value of direction you could cheat and just use goto as the bound function on the previous button click hander:

prev_button.on('click', goto);

Additionally, while it may utilize a few more function calls (thus being slightly slower) the code within goto() could be simplified using Math.min() and Math.max():

if(direction == 'next'){
    current = Math.min(slides.length, current + 1);
} else {
    current = Math.max(current - 1, 0); // zero-based
}

var slide_strip = $('.slides');
var slides = slide_strip.find('> *');

var next_button = $('.next-button');
var prev_button = $('.prev-button');

next_button.on('click', goto.bind(null, 'next'));
prev_button.on('click',goto);

function goto(direction){
  var current = parseInt(slide_strip.attr('data-current'), 10);

  if(direction == 'next'){
    current = Math.min(slides.length, current + 1);
  } else {
    current = Math.max(current - 1, 0);
  }

  slide_strip.attr('data-current', current);
}
.main-box {
  max-width: 600px;
  margin: 0 auto;
}

.slide-box {
  overflow: hidden;
}

.slides {
  background-color: #656565;
  display: flex;
  flex-wrap: nowrap;
  transition: -webkit-transform 300ms;
  transition: transform 300ms;
  transition: transform 300ms, -webkit-transform 300ms;
}
.slides[data-current="1"] {
  -webkit-transform: translateX(0%);
          transform: translateX(0%);
}
.slides[data-current="2"] {
  -webkit-transform: translateX(-100%);
          transform: translateX(-100%);
}
.slides[data-current="3"] {
  -webkit-transform: translateX(-200%);
          transform: translateX(-200%);
}
.slides[data-current="4"] {
  -webkit-transform: translateX(-300%);
          transform: translateX(-300%);
}
.slides[data-current="5"] {
  -webkit-transform: translateX(-400%);
          transform: translateX(-400%);
}
.slides[data-current="6"] {
  -webkit-transform: translateX(-500%);
          transform: translateX(-500%);
}
.slides[data-current="7"] {
  -webkit-transform: translateX(-600%);
          transform: translateX(-600%);
}
.slides[data-current="8"] {
  -webkit-transform: translateX(-700%);
          transform: translateX(-700%);
}
.slides[data-current="9"] {
  -webkit-transform: translateX(-800%);
          transform: translateX(-800%);
}
.slides[data-current="10"] {
  -webkit-transform: translateX(-900%);
          transform: translateX(-900%);
}
.slides[data-current="11"] {
  -webkit-transform: translateX(-1000%);
          transform: translateX(-1000%);
}
.slides[data-current="12"] {
  -webkit-transform: translateX(-1100%);
          transform: translateX(-1100%);
}
.slides[data-current="13"] {
  -webkit-transform: translateX(-1200%);
          transform: translateX(-1200%);
}
.slides[data-current="14"] {
  -webkit-transform: translateX(-1300%);
          transform: translateX(-1300%);
}
.slides[data-current="15"] {
  -webkit-transform: translateX(-1400%);
          transform: translateX(-1400%);
}
.slides[data-current="16"] {
  -webkit-transform: translateX(-1500%);
          transform: translateX(-1500%);
}
.slides[data-current="17"] {
  -webkit-transform: translateX(-1600%);
          transform: translateX(-1600%);
}
.slides[data-current="18"] {
  -webkit-transform: translateX(-1700%);
          transform: translateX(-1700%);
}
.slides[data-current="19"] {
  -webkit-transform: translateX(-1800%);
          transform: translateX(-1800%);
}
.slides[data-current="20"] {
  -webkit-transform: translateX(-1900%);
          transform: translateX(-1900%);
}
.slides .slide {
  height: 300px;
  min-width: 100%;
  background-color: #ffb0b0;
}
.slides .slide:nth-child(even) {
  background-color: #ff8181;
}

1

2

3

4

5

6

7

        <button type="button" class="prev-button">prev</button>
        <button type="button" class="next-button">next</button>


Unit tests for a simple composite I do a lot of unit testing(tdd) and would like some comments about my style. Here is a simple example from a composite.

Is there anything I could improve?

public class ChangedClaimsServiceComposite : IChangedClaimsService
{
    private readonly IChangedClaimsService[] changedClaimsServices;

    public ChangedClaimsServiceComposite(params IChangedClaimsService[] changedClaimsServices)
    {
        if (changedClaimsServices == null) throw new ArgumentNullException("changedClaimsServices");
        this.changedClaimsServices = changedClaimsServices;
    }

    public List<IChangedClaim> GetChangedClaims()
    {
        return changedClaimsServices.SelectMany(x => x.GetChangedClaims()).ToList();
    }

    public void SetClaimUpdated(IChangedClaim changedClaim)
    {
        foreach (var changedClaimsService in changedClaimsServices)
        {
            changedClaimsService.SetClaimUpdated(changedClaim);
        }
    }
}

[TestFixture]
public class ChangedClaimsServiceCompositeTests
{
    [Test]
    public void GetChangedClaimsSouldCombineResultsFromDependedServices()
    {
        //Arrange
        var changedClaimService1 = new Mock<IChangedClaimsService>();
        var changedClaimService2 = new Mock<IChangedClaimsService>();

        var changedClaim1 = CreateAnyChangedClaim();
        var changedClaim2 = CreateAnyChangedClaim();
        var changedClaim3 = CreateAnyChangedClaim();
        var changedClaim4 = CreateAnyChangedClaim();

        changedClaimService1.Setup(x => x.GetChangedClaims())
            .Returns(new List<IChangedClaim>() {changedClaim1, changedClaim2});
        changedClaimService2.Setup(x => x.GetChangedClaims())
            .Returns(new List<IChangedClaim>() { changedClaim3, changedClaim4 });

        var sut = new ChangedClaimsServiceComposite(
            changedClaimService1.Object,
            changedClaimService2.Object);

        //Act
        var result = sut.GetChangedClaims();

        //Assert
        var expectedResult = new List<IChangedClaim>()
        {
            changedClaim1,
            changedClaim2,
            changedClaim3,
            changedClaim4
        };
        Assert.That(result, Is.EqualTo(expectedResult), "result");
    }

    [Test]
    public void SetClaimUpdatedShouldCallSameMethodOnDependedServices()
    {
        //Arrange
        var changedClaimService1 = new Mock<IChangedClaimsService>();
        var changedClaimService2 = new Mock<IChangedClaimsService>();

        var changedClaim = CreateAnyChangedClaim();

        var sut = new ChangedClaimsServiceComposite(
            changedClaimService1.Object,
            changedClaimService2.Object);

        //Act
        sut.SetClaimUpdated(changedClaim);

        //Assert
        changedClaimService1.Verify(x => x.SetClaimUpdated(changedClaim), "changedClaimService1");
        changedClaimService2.Verify(x => x.SetClaimUpdated(changedClaim), "changedClaimService2");
    }

    private IChangedClaim CreateAnyChangedClaim()
    {
        return new Mock<IChangedClaim>().Object;
    }
}

If you rename these:

    var changedClaim1 = CreateAnyChangedClaim();
    var changedClaim2 = CreateAnyChangedClaim();
    var changedClaim3 = CreateAnyChangedClaim();
    var changedClaim4 = CreateAnyChangedClaim();

Like this:

    var service1changedClaim1 = CreateAnyChangedClaim();
    var service1changedClaim2 = CreateAnyChangedClaim();
    var service2changedClaim1 = CreateAnyChangedClaim();
    var service2changedClaim1 = CreateAnyChangedClaim();

Then it will be more obvious that these will be used in two different services. These names will carry more meaning than just an arbitrary list of IChangedClaim objects.

What is "sut" ? A more meaningful name would be better.

In this assert:

    Assert.That(result, Is.EqualTo(expectedResult), "result");

The comment message is not very useful.
Either make it something more meaningful, or delete it.


Possible fake testing with Rhino mock I'm new to Rhino mock and I would like to know if the test I did is not fake testing, as I've read a lot about it. Are there any improvements that need to be done?

[Test]
public void GenerateDateTable_Returns_DataTableOfGroup()
{
    //Arrange
    var groupStub = MockRepository.GenerateStub<Group>();
    var groupEnumerable = new[] { groupStub };
    var mockgroup = MockRepository.GenerateMock<FillTableRow<Group>>();
    mockgroup.Replay();
    //Act
    mockgroup.GenerateDateTable(groupEnumerable);

    //Assert
    Assert.IsTrue(mockgroup.DataTableBatch.Columns.Count == 3);
}

public class FillTableRow<T>
{
    public DataTable DataTableBatch { get; set; }

    public DataTable GenerateDateTable(IEnumerable<T> enumerable)
    {
        IDbObjectFactory<T> tableFactory = new TableFactory<T>();
        DataTableBatch = (DataTable)tableFactory.GetDbObject();

        foreach (var row in enumerable)
        {
            AddRow(row);
        }
        return DataTableBatch;
    }
}

As far as I can understand (I use Moq as an isolation framework and not rhino mocks) in your test you are testing not an implementation of your class FillTableRow, but a mock, generated by isolation framework. You should always use your real class as systam under test (obviously). You use stubs to provide context in which your system under tests works, just to emulate the system's environment. And you use mocks if the state of your system under test is not changing during your test and you need to be sure that correct methods of external classes has been called.
I recomend you to read "The Art of Unit Testing" book by Roy Osherove. It gives very good explanation of how and when you should use mocks and stubs.


Computing bezier curves of degree n with recursive functions I have been looking at homemade CNC and have been wondering how curves are drawn, so I looked into it and found this cool article. I then decided to try coming up with a bezier curve algorithm in C. It seems to work ok, and while I haven't tried plotting points with this exact implementation, it seems to match up with results from a previous implementation that I did plot points with.

#include <stdint.h>

static long double bbezier(long double t, long double p0, long double p1){
    return ((p1 - p0) * t) + p0;
}

long double bezier(long double t, uint64_t *points, uint64_t n){
    long double p0 = points[0], p1 = points[1];
    if(n == 1) return bbezier(t, p0, p1);

    long double q0 = bezier(t, points, n - 1),
        q1 = bezier(t, points + 1, n - 1);
    return bbezier(t, q0, q1);
}

I then quickly wrote this test program in C++.

#include <iostream>

extern "C" long double bezier(long double, uint64_t *, uint64_t);

uint64_t pointsx[] = {
    0, 40, 100, 200
};
uint64_t pointsy[] = {
    0, 150, 60, 100
};

int main(){
    for(uint64_t i = 0; i <= 10000; ++i){
        long double ii = i;
        long double j = ii/10000;

        long double x = bezier(j, pointsx, 3);
        long double y = bezier(j, pointsy, 3);

        std::cout << "X: " << x << ", Y: " << y << '\n';
    }
    return 0;
}

I wrote an implementation running in javascript from a lightly modified w3 schools canvas tutorial here to understand how bezier curves work but it only supports 3rd degree curves. It does plot the points though, and that's what I based the above implementation on.

It doesn't make any checks to ensure t is between 0 and 1 and n != 0 but I'm not too worried. The only thing I'm worried about is segfaults in cases where n is so high that you get a stack overflow but that will be a pretty crazy curve. Anyway, how does it look?

Recursion

[Edit]
In bezier(), the 2 recursive calls to bezier() is inefficient as it exponential grows with O(2n) and only O(n2) operations are needed.   I suspect  better efficiency (linear) can be had  with a pre-computed weighing of the d[] terms below.

The concern about seg faulting due to excessive recursion would be mitigated with the above improvement.

I also change function bbezier() to code.

long double bezier_alt1(long double t, const uint64_t *points, size_t n) {
  assert(n);
  long double omt = 1.0 - t;
  long double d[n];  // Save in between calculations.

  for (size_t i = 0; i < n; i++) {
    d[i] = omt * points[i] + t * points[i + 1];
  }
  while (n > 1) {
    n--;
    for (size_t i = 0; i < n; i++) {
      d[i] = omt * d[i] + t * d[i + 1];
    }
  }
  return d[0];
}

[Edit2]

A linear  solution O(n) is possible with O(1) additional memory.

long double bezier_alt2(long double t, const uint64_t *points, size_t n) {
  assert(n);
  long double omt = 1.0 - t;
  long double power_t = 1.0;
  long double power_omt = powl(omt,n);
  long double omt_div = omt != 0.0 ? 1.0/omt : 0.0;

  long double sum = 0.0;
  unsigned long term_n = 1;
  unsigned long term_d = 1;
  for (size_t i = 0; i < n; i++) {
    long double y = power_omt*power_t*points[i]*term_n/term_d;
    sum += y;
    power_t *= t;
    power_omt *= omt_div;
    term_n *= (n-i);
    term_d *= (i+1);
  }
  power_omt = 1.0;
  long double y = power_omt*power_t*points[n]*term_n/term_d;
  sum += y;
  return sum;
}

Additional linear simplifications possible - perhaps another day.

Minor stuff

Use const

A const in the referenced data allows for some optimizations, wider application and better conveys code's intent.

// long double bezier(long double t, uint64_t *points, uint64_t n){
long double bezier(long double t, const uint64_t *points, uint64_t n){

Excessive wide type

With uint64_t n, there is no reasonable expectation that such an iteration will finish for large n.

Fortunately, n indicates the size of the array.  For array indexing and sizing, using size_t.  It is the right size - not too narrow, nor too wide a type.

// long double bezier(long double t, uint64_t *points, uint64_t n){
long double bezier(long double t, uint64_t *points, size_t n){

For this application, certainly unsigned would always suffice.

static

Good use of static in static long double bbezier() to keep that function local.

Missing "bezier.h"

I'd expect a bezier() declaration in a .h file and implementation in the .c file instead of extern "C" long double bezier(long double, uint64_t *, uint64_t); in main.c

// extern "C" long double bezier(long double, uint64_t *, uint64_t);
#include "bezier.h".

n range check

Perhaps in a debug build, test n.

long double bezier(long double t, const uint64_t *points, uint64_t n){
  assert( n > 0);    // Low bound
  assert( n < 1000); // Maybe a sane upper limit too


Add WeekDays to Date Will the below code work for adding Weekdays in all possible scenarios?
eg, If I add 4 days to a "Thursday", the result should be next "Wednesday". Adding 1 day to a "Friday", "Saturday" or "Sunday" should be the next "Monday".

I am trying to get the nth working day (weekday) after a given day.

var addOneDay=function(date) {
    var result = new Date(date.getTime());
    result.setDate(result.getDate() + 1);
    return result;
};

var addWeekDays = function(date,days) {
    var result = new Date(date.getTime());
    for (var i = 0; i < days; i++) {
        result = addOneDay(result);
        if (result.getDay()=== 6 || result.getDay()=== 0) i--;
    };
    return result;
};

var date = new Date(2016,1,18);
console.log(addWeekDays(date,4));

What is the expected behaviour if you add one weekday to a Saturday? Possible answers are: throw an exception, behavior is undefined, Monday, or Tuesday. Make a decision, and document it.

The use of var funcName = function(…) { … } is a bit unconventional compared to function funcName(…) { … }, but it's not wrong.

Other than that, I think that the code looks correct.  For a more efficient way to add large intervals, you could add a number of integral weeks (7 * Math.floor(days / 5)) and advance the remainder (days % 5) the slow way.


Number Theory Inhibitor Calculator I've been working on generating primes and prime products quickly to aid me in my research on prime numbers, their density, etc. The answer to my Large Number Limit Extravaganza question proved to be extremely useful in writing my programs. I have now expanded on this program to look at the difference in the amount of primes up to a certain point according to a lower bound, and the amount of "straight inhibitors" up to a certain point. A straight inhibitor is a figment of my imagination that I have used in my math and is extremely important for it.

import math
grandtotal = 1000
def prime_sieve(limit):
    a = [True] * limit
    a[0] = a[1] = False

    for i, isprime in enumerate(a):
        if isprime:
            yield i
            for n in xrange(i * i, limit, i):
                a[n] = False

def getDotArrayLog():
    e=55
    data = []
    while e<grandtotal:
        e+=1
        data.append((2 * e / (math.log(2.0 * e, 10) + 2)) - e / (math.log(e, 10) - 4))
    return data

def getDotArrayProduct():
    e=55
    data = []
    product = 1.0
    while True:
        if e==grandtotal: break
        e+=1
        for p in prime_sieve(e):
            product *= (1.0 - 1.0 / p)
        data.append(product)
        product = 1.0
    return data

if __name__ == "__main__":
    logs = getDotArrayLog()
    producta = getDotArrayProduct()
    print "Tested all the way up to", grandtotal
    counter = 0
    for e in logs:
        a = (1-producta[counter])*(counter+55)
        print "Amount of primes:", e
        print "Amount of inhibitors:", a
        print e-a
        counter+=1

I'm looking for some optimizations, condensations, and probably a great lecture on proper conventions.

2 small enhancements that I can see:

First, you could avoid to compute log(e,10) twice in each iteration by noticing that log(a*b) = log(a)+log(b). Since one term is fixed, you save 1 computation:

log2 = math.log(2.0,10)

def getDotArrayLog():
    e=55
    data = []

    while e<grandtotal:
        e+=1
        loge = math.log(e, 10)  # compute once only
        data.append((2 * e / (loge + log2 + 2)) - e / (loge - 4))
    return data

then, in your final loop, you could zip both arrays instead of using a counter and index access, and use enumerate to do the index counting for you, since you still need it for your computations:

for counter,(e,p) in enumerate(zip(logs,producta)):
    a = (1-p)*(counter+55)
    print "Amount of primes:", e
    print "Amount of inhibitors:", a
    print e-a


Count records and pull DISTINCT I have a table that reads every minute, but the data on one meter only changes once every 60 minutes. It will then repeat that value for 60 minutes before changing again.

I need to take the sum of these values and add them up for the day. But if I grab it by the hour, I will get duplicates. So what I decided to do was count the number of records, and if is over 30 minutes use that record.

This is currently working good and is not a big issue, but I am curious if there is an easier way to do this.

WITH A AS
(SELECT DISTINCT
  CONVERT(CHAR(12), TIME, 101) AS [ DAY ],
  DatePart (HOUR, TIME) AS [ HOUR ],
  [ B100 Pulse LAST HOUR ],
  [ RESET Counter ],
  [ B700 Pulse LAST HOUR ],
  [ B800 Pulse LAST HOUR ],
  [ B1300 Pulse LAST HOUR ],
  [ B900 Pulse LAST HOUR ],
  COUNT([ RESET Counter ]) AS Records
FROM
  (SELECT
    TOP (100) PERCENT KPI $.[ B100 Pulse LAST HOUR ],
    Main $.Time,
    KPI $.[ RESET Counter ],
    KPI $.[ B700 Pulse LAST HOUR ],
    KPI $.[ B800 Pulse LAST HOUR ],
    KPI $.[ B900 Pulse LAST HOUR ],
    KPI $.[ B1300 Pulse LAST HOUR ]
  FROM
    Main $
    INNER JOIN KPI $
      ON Main $.[LOOP INDEX ] = KPI $.[LOOP INDEX ]
  ORDER BY Main $.Time DESC) AS t1
GROUP BY CONVERT(CHAR(12), TIME, 101),
  DatePart (HOUR, TIME),
  [ B100 Pulse LAST HOUR ],
  [ RESET Counter ],
  [ B700 Pulse LAST HOUR ],
  [ B800 Pulse LAST HOUR ],
  [ B1300 Pulse LAST HOUR ],
  [ B900 Pulse LAST HOUR ])
SELECT
  Top 120 [ DAY ],
  COUNT(*) AS [ # of Records], SUM([B100 Pulse Last Hour]) * 100 AS [Building 100 Gal Discharged], SUM([B700 Pulse Last Hour]) * 100 AS [Building 700 Gal Discharged], (SUM([B800 Pulse Last Hour])
  - SUM([ B1300 Pulse LAST HOUR ])) * 100 AS [ Building 800 Gal Discharged ],
  SUM([ B900 Pulse LAST HOUR ]) * 100 AS [ Building 900 Gal Discharged ],
  SUM([ B1300 Pulse LAST HOUR ]) * 100 AS [ Building 1300 Gal Discharged ],
  SUM(
    (
      [ B100 Pulse LAST HOUR ] + [ B700 Pulse LAST HOUR ] + [ B800 Pulse LAST HOUR ] + [ B900 Pulse LAST HOUR ]
    ) * 100
  ) AS [ Daily Total ],
  [ STARTING LEVEL ] AS [ LEVEL IN B1800 AT Midnight ],
  [ Difference ]
FROM
  (SELECT
    *,
    ROW_NUMBER () Over (
      PARTITION BY Records
  ORDER BY Records ASC
  ) AS ROW
  FROM
    A
    INNER JOIN B1800_Totals
      ON A.[ DAY ] = B1800_Totals.[ DATE ]) AS A2
WHERE Records >= 30
  AND ([ DAY ] = @date)
GROUP BY DAY,
  [ STARTING LEVEL ],
  [ Difference ]
ORDER BY DAY ASC

Well done on getting a query that works: that is the hardest part!

I do agree that you have overcomplicated this query: I had to spend quite a long time playing around with it to figure out what it was intended to do.

I also suspect it might not be the best way to achieve what you want. So well done also for seeking feedback here - I'm sorry mine is coming quite late. I'm going to dump a tonne of feedback in this answer: it might seem fairly blunt but don't take it personally, we all learn from experience. :)

I struggled to decide what order to present my feedback in, so it might not be in the most logical order for you. I apologise if so.

How to tackle complex queries

There are two ways one can go about a complex query:

Dive in and try to get something that works by examination of your results, then refine it subsequently, or
Spend time up-front designing the query without actually writing it, and go straight to a simpler (perhaps simplest) solution. Usually this means you can "prove" the query to be correct by reasoning about the design logic, without having to examine the results so much.

I'm a big fan of (2) and believe that it is generally viewed as the "best" option across all schools of programming - but of course it's not always practical.

It seems to me that the biggest problem with this query is that you took approach (1). The query screams "unclear thinking" to me; it is messy and hard to read, with quite a lot of unnecessary bits. Learning to design code well before you write it is a skill of its own, but one well worth practising if you are able to.

High-level algorithm choice

Without any sample data and clear information on the source table structures, it is hard for me to critique your overall approach.

It sounds like what you have is a fairly typical scenario, that the data warehousing world at least refers to as "semi-additive facts". That is, data that are numeric and can in principle be added to each other, but for which adding them only makes sense in certain limited scenarios. Semi-additive facts aren't that uncommon and you might find that a useful search term if you want to read more about dealing with them.

Certainly, a common approach to semi-additive facts is "two pass" cubing (or grouping/aggregation), which is essentially all this query is doing (but it took me far too long to understand that). So your general approach might be just fine.

However I have a niggling feeling that you don't need to bother with the "count the number of records and if it's over 30, use that record" bit. I can't say for sure without seeing some of your data but it feels like you could just DISTINCT your core data set to reduce it to one row per meaningful set of measurements, and then do only one GROUPing operation.

I suspect that if you work on simplifying and clarifying your query, you will be able to see for yourself if this second grouping is necessary or not. When a query is as simple as it can be, wrong logic is usually easy to spot.

Comments, magic numbers and code that won't run

I'm also totally unclear where the magic number 30 comes into things. If you get a measurement per minute, which changes once per hour, I'd have expected the number 60 to feature, not 30.

Your code would benefit from clear comments explaining why constant numbers like this are what they are - more generally, your code would benefit from any comments at all.

The Top 120 bit falls into the same category. Is that 120 days (~4 months)? I'm not sure it is, because of the mysterious @date variable hidden uncommented in your code.

This isn't declared anywhere so as far as I'm concerned, your code is broken and does not run. I presume you forgot to include the declaration, or this code exists within a stored procedure - but you haven't mentioned. If a single date is being passed in, I can't understand how your query will return more than one day of results, so the Top 120 doesn't make sense. Neither does the final ORDER BY DAY ASC - it seems to me that a result set will only ever contain one value of DAY.

CTEs vs sub-queries and incremental problem solving

This bit is both SQL-specific stylistic advice and very general programming advice.

Break any programming problem down into a series of steps, then solve those steps one-by-one. The clearer it is to follow in your code which steps are which, and what order they happen in, the better your code will be.

T-SQL provides a really handy construct for step-by-step problem solving, the common table expression (CTE); you use one of them, named A. The big problem is that you only use one of them. Apart from A you use a variety of subqueries (SELECT query results enclosed in brackets, and treated like tables in their own right).

A bit of history: in the international definition of the SQL language, subqueries have been around a long time, and CTEs are a more recent addition. CTEs were introduced mainly as an improvement on subqueries: they are clearer, more flexible, and reusable (within one query).

In the context of "production" SQL code (code to be written once and used again and again - as against ad-hoc one-off analysis) I do not personally believe there is any situation in which it is better to use a subquery. CTEs are not always the best choice for performance, but in that case other constructs such as temporary tables and table variables are the better choice.

Conclusion: rework your query to use a linear series of CTEs, each of which builds on the one before towards the end result.

Use as few constructs / steps as necessary

Once you do a CTE-focussed overhaul of your code, you will see that multiple steps can be combined into one to simplify (rather than complicate) the overall query.

You have used far more steps than you need to but it's very hard to identify that currently, because you have to skip around the query to follow it.

In a general sense, you need to know/remember that what SQL does best is join tables together. Unless there's a good reason not to (for example to do with cardinality, the number of records in the relationship), do all your joins in one query set: this gives SQL Server more options for optimising how it does them.

You also need to know that GROUP BY clauses work fine in the same query as multiple joins. There's no need to do joins in one set (CTE or subquery) then group the results in a separate set.

Style and layout

Consistency is crucial in any programming task I've come across. Much of the following relates to consistency.

I can't see any patterns of when you start new lines, why/how much you indent your code, where you put commas, and so on. This reduces readability a lot. Choose a style then follow it consistently.
In general, use more indentation and spacing, not less. Bytes are not precious in this day and age. Judicious use of blank lines and reasonably deep indentation make code much easier on the eye.
Capitalise consistently. This applies to table and column names wherever you have control over them (see below for more on that), and to function names and SQL keywords. If you are capitalising keywords like SELECT, Top should also be capitalised.
Unnecessary brackets () in expressions are distracting. Instead of AND    ([ DAY ] = @date), just AND   [ DAY ] = @date.
Your code doesn't convince me that you understand the basic rules of arithmetic as they apply to SQL: for example, that sum(column*100) is the same as sum(column)*100, or that sum(column1) - sum(column2) is the same as sum(column1-column2). If you aren't rock solid on these (which is OK!) I recommend brushing up - it will really help over time. If you are, use one form or the other consistently; it will give any reader of the code additional trust in your ability.
In a production-quality SQL query, never list columns that don't need to be there. It is distracting and can (sometimes) affect performance. For example [ RESET Counter ] is never used directly (only within a COUNT()) so shouldn't be listed. You also generate a ROW_NUMBER() but never refer to or show it, so it shouldn't be there - and removing it reduces the number of sets/steps you need overall.
The SQL * is a useful shorthand when developing queries but it has no place in a final query that will be used in production. It is far better to list explicitly the columns you want to see. This makes the query more robust against changes to underlying table definitions.
Where you list the same/similar set of columns in multiple places (for example in a SELECT then again in GROUP BY, or in a CTE then again in the main SELECT), list them in the same order each time. It's much easier to see if columns have been omitted, deliberately or accidentally.

Object naming

You should qualify table names with which schema they belong to. This makes your code more robust if multiple schemas are used in the future.
You need to familiarise yourself with the rules of regular database identifiers in SQL Server. As you're clearly aware, non-regular identifiers can be used if enclosed in square brackets, but this is bad practice. Wherever possible use regular identifiers, which means no spaces. Use underscores or CamelCase instead. This (and all subsequent advice in this section) applies to aliases as well as actual object names.
While # and $ are valid in general, you shouldn't be using # at the start of anything (e.g. a column alias) because it denotes a temporary table. I would recommend avoiding # and $ altogether if possible: there really isn't any need for them that I'm aware of.
Your underlying table names Main $ and KPI $ (I think that's what they are?) are not enclosed in square brackets: I'm surprised this hasn't caused an error.
You should also be avoiding using SQL Server reserved words for object names. That link is a good one to familiarise yourself with.
If you cannot control the presence of spaces, $ signs, reserved words etc in your underlying table/column names, consider creating some simple views that mask this by renaming all columns to SQL-compliant values that won't need square brackets. This would make your query (and all other queries on the same dataset) much, much more readable.
Always alias every table in a multi-table query and use the aliases consistently to qualify every column reference. This is clearer than having to type out the full table name to qualify columns. By qualifying every column with its table name, it's really obvious which columns come from which tables (without having to refer to an ERD or specification).
Use sensible, meaningful aliases. A is almost always a terrible name for a column, table, CTE or anything else, because it doesn't tell you anything about what it does or what it is for. I always prefix CTE names with cte so I know that's what they are when I refer to them later.
CTE names can afford to be reasonably long (same kind of length as tables) to describe their contents.
Table aliases are best short - I stick to 2 or 3 characters. One is usually too short to be clear. I'd alias Main as MN and KPI as KPI (but still alias it so it's clear!).

A few points related specifically to spacing and choice of column names and aliases:

If at all possible avoid the fancy spaced column aliases in your final results (and throughout).  I think B100_Pulse_Last_Hour is as clear as [ B100 Pulse LAST HOUR ] in a result set - give your data users some credit.
Or, it's possible you're using the wrong technology here - if you really need "pretty" column names perhaps you should be using SSRS or some other technology downstream of the query?
If you really, really need pretty column names at the end of this query, consider the Alias = Expression syntax rather than Expression AS Alias. This makes it easier to align your aliases at the start of the query and let the differing-length expressions run off to the right.

For example (if you've not seen it before)

select [ Building 100 Gal Discharged ] = 100*SUM(B100_Pulse_LAST_HOUR)
from    table

Potential performance / behaviour effects

If I've understood the purpose of your @date variable, you should use it to limit the number of records as early as possible in the query, not as late as possible. Cutting out unnecessary rows early on will speed up later parts of the query.
In CTE A: the DISTINCT keyword is useless because you're also grouping by all the non-aggregate columns.
TOP (100) PERCENT is a useless construct: that means all rows, which is the default behaviour if you remove that bit altogether.
Where you use CONVERT to get [ DAY ]: format 101 outputs 10-character results (mm/dd/yyyy) but you use a CHAR(12) which will cause trailing spaces.
If you're on 2008+ you can use the date data type instead: CAST(Time as date), confusing though that reads, will do essentially the same thing as your CONVERT, but keep it in actual date format.
Although I've said above your ROW_NUMBER() is useless, you should understand that if you do need it, it is pointless to ORDER BY the same expression you PARTITION BY. The partition means that the numbering will restart from 1 for every distinct value of Records. Within a partition, all rows will have the same value of Records, so you cannot useful order on that column. This means the ordering is essentially random - not a good thing.
Instead of getting COUNT([ RESET Counter ]) in one row set and filtering on it in a later row set using WHERE, you can filter directly on the result of a COUNT() function using the HAVING clause. It works the same as WHERE but is intended for working on aggregate functions like COUNT(), SUM() etc. You put it after the GROUP BY clause and before an ORDER BY if you have one. Again, this reduces the number of sets you need.


Extracting components of a filename I have a string str ="/Users/user/Desktop/task/U6342_Account_20150112.txt"

I need return only U6342_Account_20150112 as an array ['U6342','Account','20150112']

I did

str.split('/')[-1].gsub('.txt','').split('_')

which gives me ['U6342','Account','20150112']

Is there any more convenient solution to do it?

The string looks like a filename, so you should use the library functions for handling file paths instead of rolling your own.

File.basename(str, '.txt').split('_')


Is this breadth-first search correct? I wrote a program that is supposed to be a breadth-first search algorithm, but I'm very new to search algorithm so I don't know if my method is very effective or if there is a simpler way to do this. So I'm asking if there is a way to improve my code.

In this case, I have made the program search for the closest 1 in a 2d list where you can enter the start position and it will then find the shortest path between the start position and the 1. Of course, this can be changed in a variety of ways.

I tried to make the variables and function names as clear as possible, so I don't think I need to explain everything about my code, but that's easy for me to say so please comment if you need clarification.

m = [[1, 0, 0, 0, 0, 0],
     [0, 0, 0, 0, 0, 0],
     [0, 0, 0, 0, 0, 0],
     [0, 0, 0, 0, 0, 0],
     [0, 0, 0, 0, 1, 0],
     [0, 0, 1, 0, 0, 0],
     [0, 0, 0, 0, 0, 1],
     [0, 0, 0, 0, 1, 0],
     [1, 0, 1, 0, 0, 0],
     [1, 0, 0, 0, 0, 1],
     [0, 0, 0, 0, 1, 0],
     [1, 0, 1, 0, 0, 0]]

visited = []
queue = []

parent = {}
num = 0

def breadth_first(maze, x, y):
    queue.append((x, y))
    visited.append((x, y))

    while queue:
        pos = queue[0]
        x = pos[0]
        y = pos[1]

        # remove from queue
        queue.remove(pos)

        # find neighbor
        for dir_x, dir_y in ((-1, 0), (1, 0), (0, -1), (0, 1)):
            newx = x + dir_x
            newy = y + dir_y

            neighbor = (newx, newy)

            # add to queue
            if len(maze[0]) > newx >= 0 and len(maze) > newy >= 0 and neighbor not in visited and neighbor not in queue:
                if m[newy][newx] == 1:
                    parent[neighbor] = pos
                    return neighbor

                queue.append(neighbor)
                visited.append(neighbor)
                parent[neighbor] = pos

closest = breadth_first(m, 0, 0)

path = [closest]

def search(traceback):
    while traceback != (0, 0):
        for key, value in parent.items():
            if traceback == key:
                path.append(value)
                traceback = value

    return path

def solved(maze, input_path):
    for pos in input_path[1:-1]:
        maze[pos[1]][pos[0]] = '+'

    return maze

print(solved(m, search(closest)))

The code uses pos=queue[0] and queue.remove(pos). Instead, you could use pos=queue.pop(0). Be aware that popping from the front isn't really efficient, it's O(n), which is bad for big lists.
You could use the fact that you can assign to two variables at once: x, y = queue.pop(0). This removes pos completely, fewer variables are easier to understand.
You pass a maze and the start position to the BFS. However, the queue and others are global. That's very bad design. Keep things in as small a scope as possible. You can always return a tuple from a function if a single value is not sufficient, if that was the reason.
What are len(maze[0]) and len(maze)? Call those width and height and compute them at the start of the function to answer that question.
The if-clause in the loop has too many conditions at once, which is really hard to read. There's nothing wrong with a continue when the computed position was already visited, just to illustrate one check that could be extracted.
parent[neighbor] = pos is done at two places. Do that exactly once when you first visit a new place.
visited seems to be the list of places that were already visited. The type is a Python list, but a set would be a more suitable datatype for that, unless the order of the elements matters.


Changing the state of another entity into the constructor method I have a Clock class who inherites from pyglet.clock.Clock and behaves like a Singleton.

Into the constructor method I have the following line:

pyglet.clock.set_default(self)

Who changes the state of the default clock of pyglet, setting my Clock object as the default clock.

The constructor method leaves the Clock object into a valid state (without this line in the constructor the clock don't tick!).

Changing another entity state in the constructor:

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import pyglet

import singleton

class Clock(pyglet.clock.Clock):
    __metaclass__ = singleton.Singleton

    def __init__(self):
        super(Clock, self).__init__()

        pyglet.clock.set_default(self)

class Test(object):
    def update(self, dt):
        print dt

w = pyglet.window.Window()
@w.event
def on_draw():
    w.clear()

t = Test()

c = Clock()
c.schedule_interval(t.update, 1/60.0)

pyglet.app.run()

On the other hand I can do:

Calling pyglet.clock.set_default outside of the constructor:

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import pyglet

import singleton

class Clock(pyglet.clock.Clock):
    __metaclass__ = singleton.Singleton

    def __init__(self):
        super(Clock, self).__init__()

class Test(object):
    def update(self, dt):
        print dt

w = pyglet.window.Window()
@w.event
def on_draw():
    w.clear()

t = Test()

c = Clock()
pyglet.clock.set_default(self)
c.schedule_interval(t.update, 1/60.0)

pyglet.app.run()

In this case the call:

pyglet.clock.set_default(self)

Is outside of the constructor method and then the constructor method don't change the state of another entity.

The questions:

What is the solution more elegant or pythonic?
Is any of these solutions a best practice?
How do you resolve this issue?

In Python, a singleton should be a module, not a class. There's no good reason to define a class for that. That's what's going on in pyglet's case with pyglet.clock. If you want to define your own version you should do something like:

# myclock.py

from pyglet.clock import *

Then you can add functions/etc to that module to get additional behavior. This way you don't even need to call the set_default function.


Best way to locate Find on List with reflection I am locating the find method of List in C# with the single Expression type  parameter

I am using the following ( where T is a List, in my case a SubSonic IActiveRecord )

MethodInfo info = typeof(T).GetMethods(BindingFlags.Static | BindingFlags.Public)
                             .FirstOrDefault(m => m.Name == "Find" && m.GetParameters().Count() == 1);

This works perfectly well, but feels clunky (especially the parameter count part) and I'm not sure how future proof it is.

When searching for methods using reflection I found that the most future-proof way is by searching exactly for the method, so not only the Count() is correct but I would also have checked both what is this parameter type and what is the return type of the function.


A package for the DOM - Ajax Provides 3 types of normal ajax - get, post, and post for form data.

Also provides serialized ajax (get style) using a queue.

Please review the correctness of this code.

/**************************************************************************************************
AJAX
*/

// ... snip
    Pub.ajax = function (config_ajax) {
        var xhr;

        // get
        if (config_ajax.type === 'get') {
            xhr = new win.XMLHttpRequest();
            xhr.open('GET', config_ajax.url, true);
            xhr.onload = function () {
                if (this.status === 200) {
                    config_ajax.callback(xhr.responseText);
                }
            };
            xhr.send(null);
        }

        // post
        if (config_ajax.type === 'post') {
            xhr = new win.XMLHttpRequest();
            xhr.open("POST", config_ajax.url, true);
            xhr.setRequestHeader("Content-type",
                    "application/x-www-form-urlencoded");
            xhr.onload = function () {
                if (this.status === 200) {
                    config_ajax.callback(xhr.responseText);
                }
            };
            xhr.send(config_ajax.data);
        }

        // post for form_data
        if (config_ajax.type === 'multi') {
            xhr = new win.XMLHttpRequest();
            xhr.open("POST", config_ajax.url, true);
            xhr.onload = function () {
                if (this.status === 200) {
                    config_ajax.callback(xhr.responseText);
                }
            };
            xhr.send(config_ajax.data);
        }
    };

    // queue used for serial ajax
    Priv.Queue = (function () {
        var queue = [],
            publik = {};
        function getIndexFromToken(callback) {
            var hold;
            $A.someIndex(queue, function (val, index) {
                if (val.callback === callback) {
                    hold = index;
                    return index;
                }
            });
            return hold;
        }
        function getBlockedProperty(item) {
            var blocked;
            if (item) {
                blocked = item.blocked;
            } else {
                blocked = false;
            }
            return blocked;
        }
        publik.addItem = function (callback) {
            var temp = {};
            temp.blocked = false;
            temp.callback = callback;
            temp.response_text = null;
            queue.push(temp);
        };
        publik.itemCompleted = function (response_text, callback) {
            var index,
                item,
                blocked;
            index = getIndexFromToken(callback);
            if (index !== 0) {
                queue[index].blocked = true;
                queue[index].response_text = response_text;
            } else {
                item = queue.shift();
                item.callback(response_text);
                blocked = getBlockedProperty(queue[0]);
                while (blocked) {
                    item = queue.shift();
                    item.callback(item.response_text);
                    blocked = getBlockedProperty(queue[0]);
                }
            }
        };
        return publik;
    }());

    // serial ajax
    // unlike normal ajax the ordering is guaranteed
    Pub.serialAjax = function (source, callback) {
        Priv.Queue.addItem(callback);
        Pub.ajax({
            type:       'get',
            url:        source,
            callback:   function (response_text) {
                Priv.Queue.itemCompleted(response_text, callback);
            }
        });
    };
// ... snip

From a once-over:

You keep creating the same function in Pub.ajax:

    xhr.onload = function () {
        if (this.status === 200) {
            config_ajax.callback(xhr.responseText);
        }
    };

You could consider assigning this function once to a var onloadFunction which you then assign like this : xhr.onload = onloadFunction. On the whole, Pub.ajax needs to be reviewed by you with DRY in mind.
getBlockedProperty could be written in a much shorter fashion:

function getBlockedProperty(item) {
  return item ? item.blocked : false;
}

getIndexFromToken is odd, I have no idea what it really does, not a single variable or parameter is called token and there is not a single line of comment.
publik.addItem could use Object Literal Notation to make it more succint:

publik.addItem = function (callback) {
    var temp = {
      blocked : false;
      callback : callback;
      response_text : null;
    };
    queue.push(temp);
};

lowerCamelCasing -> You are not using it everywhere
A minor observation, would it not be much easier to just send synchronous requests instead of building a custom queue ?


Python Word Jumble Game Can the following complete program be improved? I have been using the Python For Absolute Beginners book and recently made my own version of the Python Word Jumble game.

The Areas I am particularly interested in improving:

The while word Loop Block
Python Standards
Simplifying the overall program
Commenting

# Word Jumble Game. Using Mac0S (Latest). Python V 3.4.3

import random

# Initialize Variables
WORD = ('python', 'programming', 'mug', 'glass','phone') # Constant Variable
word = random.choice(WORD)
correct = word
jumble = ''
guess = ''

print('\t\tWelcome to Word Jumble!')
print('\n')
print('\tUnscramble the letters to make a word.')

while word:
    position = random.randrange(len(word))
    jumble += word[position] # Jumble process 1
    word = word[:position] + word[(position + 1):] #Jumble process 2

print('\n')
print('The Jumble is:', jumble)

guess = input('Your Guess: ')
while guess != correct:
    print('Sorry, that\'s not it')
    guess = input('Your Guess: ')

if guess == correct:
    print('\n')
    print('That\'s it! You guessed it!')
    print('\n')
    print('Thanks for playing')

exit() # User has the correct answer

In general this codes looks pretty good.  A couple of nits.
The Python Library is Huge:
I would suggest replacing the while word: loop block:
while word:
    position = random.randrange(len(word))
    jumble += word[position] # Jumble process 1
    word = word[:position] + word[(position + 1):] #Jumble process 2

With a library call like:
jumble = ''.join(random.sample(word, k=len(word)))

Main loop:
By using a break inside the main loop, it can be restructured to remove some extra code like:
while True:
    print('\nThe Jumble is:', jumble)
    guess = input('Your Guess: ')
    if guess == word:
        print("\nThat's it! You guessed it!\n")
        print('Thanks for playing')
        break

    print("Sorry, that's not it")

This removes an extra comparison and input()
Comments:
I am not a fan of right hand style comments.  I generally prefer a needed comment to proceed the line/block it is commenting.  You used reasonably descriptive variable names, I don't know that anything other than the header comment is providing much value.


Rock–paper–scissors console game I wrote this console rock-paper-scissors game. I'm new to C++. I'd appreciate any suggestions to improve the code below. Thanks.

RPSCore.h

#ifndef RPS_CORE_H
#define RPS_CORE_H

class RPSCore
{
public:
    static int rpsWinner(char& playerChoice, char& computerChoice);
    static char getComputerChoice();
};

#endif // !RPS_CORE_H

RPSCore.cc

#include <random>
#include <chrono>
#include <cstdint>
#include <map>
#include <sstream>
#include <string>
#include <algorithm>

#include "RPSCore.h"

// private helper function
char getValueIfKeyExists(std::map<std::string, char>& dict, std::string& key)
{
    char ret = '\0';
    for (std::map<std::string, char>::iterator it = dict.begin(); it != dict.end(); it++)
    {
        if (it->first.compare(key) == 0)
        {
            ret = it->second;
            break;
        }
    }
    return ret;
}

int RPSCore::rpsWinner(char& playerChoice, char& computerChoice)
{
    std::map<std::string, char> rules = { {"rs", 'r'}, {"rp", 'p'}, {"sp", 's'} };
    std::stringstream ss;
    ss << playerChoice << computerChoice;
    std::string combination = ss.str();
    std::string combinationCopy{ combination };
    std::reverse(combinationCopy.begin(), combinationCopy.end());
    char p = getValueIfKeyExists(rules, combination) == '\0' ? getValueIfKeyExists(rules, combinationCopy) : getValueIfKeyExists(rules, combination);
    return p == playerChoice && p != computerChoice ? 1 : p == computerChoice && p != playerChoice ? 2 : -1; // 1-> player wins, 2-> computer wins, -1-> draw
}

char RPSCore::getComputerChoice()
{
    std::string options = "rps";
    uint32_t seed = static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count());
    std::minstd_rand0 range(seed);
    std::uniform_int_distribution<std::minstd_rand0::result_type> dist(0, 2);
    return options[dist(range)];
}

main.cc

#include <iostream>
#include <string>
#include <cctype>

#include "RPSCore.h"

std::string input(std::string prompt="")
{
    std::string temp;
    std::cout << prompt;
    std::getline(std::cin, temp);
    return temp;
}

int main(int argc, char* argv[])
{
    char playerChoice = []() {
        std::string ret = "";
        do
        {
            ret = input("Your weapon >>> ");
            if (std::tolower(ret[0]) == 'r' || std::tolower(ret[0]) == 'p' || std::tolower(ret[0]) == 's') break;
        } while (1);
        return ret[0];
    }();
    char computerChoice = RPSCore::getComputerChoice();
    int winner = RPSCore::rpsWinner(playerChoice, computerChoice);
    std::cout << "Your choice: " << (playerChoice == 'r' ? "Rock" : playerChoice == 'p' ? "Paper" : "Scissors")  << " Computer's choice: " << (computerChoice == 'r' ? "Rock" : computerChoice == 'p' ? "Paper" : "Scissors") << " And " << (winner == 1 ? "You WIN!!!" : winner == 2 ? "Computer Wins :(" : "It's a draw :)");
    std::cin.get();
    return 0;
}

I'm thinking you named ret as in short for return.
Don't shorten variable names. Also try to give varaible names that are meaningful. Naming it return because you return it is silly.

char ret = '\0';

Same here:

std::string temp;

This line looks very odd and is a sign of poorly named variables:

ret = it->second;

r, p s these are a good example of when you should be using an ENUM.

Try to avoid 1 letter variable names.

You can use const when variables should never change. For example: rules.

I suggest putting the condition inside the while instead.
For example:

} while (std::tolower(ret[0]) != 'r' || std::tolower(ret[0]) != 'p' || std::tolower(ret[0]) != 's');

If you had an ENUM, you could check the entered value against all the values in the ENUM. This would look much better, and be easier to maintain. (E.g adding Lizard Spock)

The ENUM would also help for printing the values. (No more if r print rock etc)


JavaScript Boilerplate - Review comments required I am in between to create JavaScript Boilerplate (collection of best practices around) for low/medium complex project and will host on GitHub in sometime once finalized it.

Have divided the JavaScript modules in 3 parts

project.config.js - Would have all the required configuration
project.main.js - the JS Boilerplate
project.helper.js - would have most commonly used functions, user may or may not want it

project.config.js

/* MODULE (our namespace name) and undefined are passed here
 * to ensure 1. namespace can be modified locally and isn't
 * overwritten outside of our function context
 * 2. the value of undefined is guaranteed as being truly
 * undefined. This is to avoid issues with undefined being
 * mutable pre-ES5.
*/
(function (MODULE, undefined) {
    MODULE.config = {
        language: 'english',
        debug: true,
        etc...
    }
}(window.MODULE = window.MODULE || {}))

project.main.js:

/* MODULE (our namespace name) and undefined are passed here
 * to ensure 1. namespace can be modified locally and isn't
 * overwritten outside of our function context
 * 2. the value of undefined is guaranteed as being truly
 * undefined. This is to avoid issues with undefined being
 * mutable pre-ES5.
*/

(function( MODULE, $, undefined ){
/**
 * Logging function, for debugging mode
 */
jQuery.log = function(message) {
    if( (typeof window.console != 'undefined' && typeof window.console.log != 'undefined') && console.debug ) {
        console.debug(message);
    } else {
        //alert(message);
    }
};

/**
 * Private properties
 */
var foo = "foo",
    bar = "bar";

/**
 * Public methods and properties
 */
MODULE.foobar = "foobar";
MODULE.sayHello = function () {
    speak("hello world");
};

/**
 * Private method
 */

/* Benefits:
 * 1. Makes it easier to understand "functions as an object".
 * 2. It enforces good semicolon habits.
 * 3. Doesn't have much of the baggage traditionally associated with functions and scope.
 */
var getData = function () {
};

/* Singleton with Privileged Methods */
MODULE.subModule = (function () {
    function _subModule() {
        /**
         * Init call
         */
        var _this = this;   /* Store this to avoid scope conflicts */

        /**
         * facebookLogin - FB.login prompts the user to authorize your application
         */
        this.facebookLogin = function () {
        }

        /**
         * private method
         */
        var privateMethod = function() {
        };

        this.init = function () {
            _this.facebookLogin();
            return this;
        };

        return this.init();
    }

    return new _subModule();
}());
})(window.MODULE = window.MODULE || {}, jQuery)

Though it's pretty straight forward with comments and all but anyway let me elaborate it further, have been using Global Abatement using namespace, singleton inside the SIAF (to protect the scope and follow encapsulation) and using privileged private methods inside it and executing the privileged one in init().

And essentially returning the submodule in the last to available to the window level using namespace (MODULE).

Looks pretty neat but still looking to make it better and robust.

Would appreciate code review comments here.

Thanks for your time.

Interesting work Arif. I really loved to see different as well as interesting JS patterns you have used. It is looking to me like, it is going in a good and right direction.

Few things that you might consider to do is

I) Create some example of different modules and ensure they are in different files (it is not very clear with the current example but I assume you have already thought about it).

II) Might be a very personal choice, I don’t like _ in variable and method names. The way I know what is public and what is private is by looking at what is being returned at the bottom, as I use a pattern like below in most occasions

    (function(MODULE, $)  {
       MODULE.charting = function(options) {
          options = $.extend({
              defaultParam1: “a”,
              defaultParam2: “b”
          }, options);

          var xAxis, yAxis;

          function render() {

          }

          function destroy() {

          }

          (function init() {
             //Anything that is suppose to be called when a object of the
             //module is created. Acts like a psuedo constructor.
          })();

          return {
             render: render,
             destroy: destroy
          };
       };
     }(MODULE || {}, jQuery));

What do you think? In above example you can clearly see what is public and they can be exposed with a different name if one wants. So in refactoring it will not break other components as the way it is exposed outside can be different. If you see what I mean.

III) If you are using jQuery, you can use extend like above to define defaults and then override if passed by the user.

IV) Generally I don’t tend to put URLs in JavaScript and prefer to put them on jsp, asp, json response, etc. as then front-end doesn’t need to worry about context/domain/port.

Will send you more comments if I think of any. For the time being have a think about it and let me know your views. Also keep on sharing the progress, would love to see this growing into a very fruitful thing.


File's encryption tool in terminal I've created a wrapper for the SimpleCrypt library that allows to easily encrypt/decrypt a string.

The application is used this :

//To encrypt
python encryptor.py file_name -e

//To decrypt
python encryptor.py file_name -d

Basically, to encrypt, I read the file, encrypt it and create a new file with the extension .enc, so that it's obvious (to me) it's been encrypted.

I'm not looking for a super secure application, I'm tutoring and when I leave my laptop alone I wouldn't want students to check their future exams or stuff like that. So it's more of a small project for my own fun, that protects files from non IT people.

I think I've covered every cases (Permission denied, file doesn't exist, wrong password etc).

I think the code could look better, but I'm mostly interested in knowing if I missed some bug I'm not aware of (As I said, I tested the application to the best of my knowledge) or if there are some Python basics that I missed.

  encryptor.py

import sys
import os.path
import getpass
import simplecrypt

ReadMode = "r"
WriteMode = "w"
EncryptArg = "-e"
DecryptArg = "-d"
PermissionDenied = 13
FileDoesNotExist = 2

def main():
    if not len(sys.argv) == 3:
        print "Incorrect number of arguments."
        print "Expected file-path encrypt/decrypt"
        exit(1)

    filepath = sys.argv[1]
    encrypt_or_decrypt = sys.argv[2]

    if not (encrypt_or_decrypt == EncryptArg or encrypt_or_decrypt == DecryptArg):
        print "2nd argument provided is invalid, expected -e or -d, received : " + encrypt_or_decrypt
        exit(1)

    requires_encryption = encrypt_or_decrypt == EncryptArg

    try:
        stream = open(filepath, ReadMode)
        file_content = stream.read()
        stream.close()

        if requires_encryption:
            password = getpass.getpass("Password to decrypt : ")
            encrypted_content = simplecrypt.encrypt(password, file_content)

            write_stream = open(filepath + ".enc", WriteMode)
            write_stream.write(encrypted_content)
            write_stream.close()
        else:
            if not filepath[-4:0] == ".enc":
                print "File was not encrypted using this application. Expected .enc format."
                exit(1)

            decrypted_content = None
            try:
                password = getpass.getpass("Password : ")
                decrypted_content = simplecrypt.decrypt(password, file_content)
            except simplecrypt.DecryptionException:
                print "Password is invalid."
                exit(1)

            write_stream = open(filepath[:-4], WriteMode)
            write_stream.write(decrypted_content)
            stream.close()

        os.remove(filepath)

    except IOError, ioError:
        if ioError.errno == PermissionDenied:
            print "You do not have permission to access this file. Consider using sudo"
        elif ioError.errno == FileDoesNotExist:
            print "File does not exist."
        else:
            raise ioError

        exit(1)

if __name__ == '__main__':
    main()

If you crack argv using argparse you'll enjoy a couple of benefits, including you get --help for free.

Defining read & WRITE_MODE is maybe a little over the top, as open(f, 'w') is a standard idiom. Anyway, you have an opportunity to let with close files for you, if you like. For example:

    with open(filepath + ".enc", "wb") as write_stream:
        write_stream.write(encrypted_content)

You probably want binary mode: "wb" rather than "w".

Consider throwing an exception in place of calling exit(1).

For the filepath[-4:0] == ".enc" enforcement test you might write: filepath.endswith(".enc").

Here is a magic number, corresponding to len(".enc"): filepath[:-4]. Consider changing the enforcement test so it uses a regex, re.compile(r'^(.+)\.enc$'). Then you can use group(1) of the match object when specifying a file path.

The ioError identifier (or io_error according to PEP8) is accurate, but perhaps a bit long -- err would suffice. Better to use the "as" keyword:

except IOError as err:


Counting significant figures in a number I decided to make something to count the significant figures in a number to see how easily it could be done, but I think there may have been a better or simpler way. Any advice even just for conventional purposes is appreciated!

def count_sig_figs(answer):
    '''This fucntion will count the sigfigs used in the answer of a user'''
    sig_fig_count = 0
    num_list = list(answer)

    for index in range(len(num_list)):
        try:
            fig = int(num_list[index])
            if fig != 0:
                sig_fig_count +=1
            elif check_zero_sig(index, num_list, sig_fig_count):
                sig_fig_count += 1
        except:
            continue
    return sig_fig_count

def check_zero_sig(index, num_list, sig_fig_count):
    '''Checks for significance in a zero from a list'''
    try:
        decimal = num_list.index('.')
        if index > decimal and sig_fig_count > 0:
            return True
    except:
        if index == 0:
            return False
        elif index == len(num_list):
            return False
        new_index = index+1

        if num_list[new_index] == '.' and sig_fig_count > 0:
            return True
        elif num_list[new_index] == '.' and sig_fig_count == 0:
            return False
        elif num_list[new_index] != '.' and sig_fig_count > 0:
            fig = int(num_list[new_index])
            if fig != 0:
                return True
            else:
                return check_zero_sig(new_index, num_list, sig_fig_count)
        elif num_list[new_index] != '.' and sig_fig_count == 0:
            fig = int(num_list[new_index])
            if fig != 0:
                return True
            else:
                return check_zero_sig(new_index, num_list, sig_fig_count)
        else:
            return False

def test():
    print(count_sig_figs('1')) # 1 sig fig
    print(count_sig_figs('10')) # 1 sig fig
    print(count_sig_figs('100')) # 1 sig fig
    print(count_sig_figs('1004')) # 4 sig figs
    print(count_sig_figs('10004')) # 5 sig figs
    print(count_sig_figs('105')) # 3 sig figs
    print(count_sig_figs('01')) # 1 sig fig
    print(count_sig_figs('1.2035')) # 5 sig figs
    print(count_sig_figs('001.09508')) # 6 sig figs
    print(count_sig_figs('0.00110')) # 3 sig figs

if __name__ == "__main__":
    test()

num_list is better named digits; don't describe the type, describe the value.

test's output is unhelpful; the result should be much more obvious, especially when it fails. Keeping along the given lines, I might do something like

def test():
    def should_be(string, count):
        assert count_sig_figs(string) == count
        print("{!r} has {} sf.".format(string, count))

    should_be('1', 1)
    should_be('10', 1)
    should_be('100', 1)
    should_be('1004', 4)
    should_be('10004', 5)
    should_be('105', 3)
    should_be('01', 1)
    should_be('1.2035', 5)
    should_be('001.09508', 6)
    should_be('0.00110', 3)

Of course, a proper test suite is probably better.

This isn't idiomatic:

for index in range(len(digits)):
    try:
        fig = int(digits[index])
        if fig != 0:
            sig_fig_count +=1
        elif check_zero_sig(index, digits, sig_fig_count):
            sig_fig_count += 1
    except:
        continue
return sig_fig_count

Bare try...excepts are bad. Even

except (ValueError, IndexError):

is bad. Primarily this is because you're spreading out the checks where they don't belong and it should instead be

try:
    fig = int(digits[index])
except ValueError:
    continue

if fig != 0:
    sig_fig_count +=1
else:
    try:
        zero_sig = check_zero_sig(index, digits, sig_fig_count)
    except IndexError:
        continue
    if zero_sig:
        sig_fig_count += 1

But then because... why is check_zero_sig throwing an IndexError? Surely that's a bug.

Before we get there, quickly change this to

for index, digit in enumerate(digits):
    if digit == ".":
        continue
    if digit != '0':
        sig_fig_count += 1
    else:
        sig_fig_count += check_zero_sig(index, digits, sig_fig_count)

That's pretty much what we want expressed more simply.

check_zero_sig's except also needs updating:

try:
    decimal = digits.index('.')
except ValueError:
    ...
else:
    return index > decimal and sig_fig_count > 0

I don't get why you don't just mutate index instead of using new_index.

This can be simplified:

if digits[index] == '.' and sig_fig_count > 0:
    return True
elif digits[index] == '.' and sig_fig_count == 0:
    return False

to

if digits[index] == '.':
    return bool(sig_fig_count)

The

elif digits[index] != '.' and sig_fig_count > 0:

and

elif digits[index] != '.' and sig_fig_count == 0:

branches are the same.

This then simplifies to

if digits[index] == '.':
    return bool(sig_fig_count)
elif digits[index] != '.':
    digit = int(digits[index])
    return bool(digit) or check_zero_sig(index, digits, sig_fig_count)
else:
    return False

which is just

if digits[index] == '.':
    return bool(sig_fig_count)
else:
    digit = int(digits[index])
    return bool(digit) or check_zero_sig(index, digits, sig_fig_count)

Then we see

elif index == len(digits):
    return False

cannot fire until after index += 1. This lets us fix our IndexError.

Back to count_sig_figs. The documentation is not written idiomatically. Documentation should be to the point, direct and not talk in third person.

def count_sig_figs(digits):
    '''Return the number of significant figures of the input digit string'''
    sig_fig_count = 0
    for index, digit in enumerate(digits):
        if digit == ".":
            continue
        if digit != '0':
            sig_fig_count += 1
        else:
            sig_fig_count += check_zero_sig(index, digits, sig_fig_count)
    return sig_fig_count

Back to check_zero_sig, which is now

def check_zero_sig(index, digits, sig_fig_count):
    '''Checks for significance in a zero from a list'''
    try:
        decimal = digits.index('.')
    except ValueError:
        if index == 0:
            return False
        index += 1
        if index == len(digits):
            return False
        if digits[index] == '.':
            return bool(sig_fig_count)
        else:
            digit = int(digits[index])
            return bool(digit) or check_zero_sig(index, digits, sig_fig_count)
    else:
        return index > decimal and sig_fig_count > 0

The if digits[index] == '.' check can't fire; we're in the except. Then

digit = int(digits[index])
return bool(digit) or check_zero_sig(index, digits, sig_fig_count)

can be

return digit != '0' or check_zero_sig(index, digits, sig_fig_count)

and the recursion, which would end up re-calling .index, can just be

if index == 0:
    return False
return any(digit != '0' for digit in digits[index+1:])

index == 0 is a very strange special case; you deal with 01 but not 001 or 00001. I suggest removing the special-case and using sig_fig_count.

Now we have

def check_zero_sig(index, digits, sig_fig_count):
    '''
    Returns if a zero digit at a given position is significant,
    given a count of significant digits preceding it.
    '''
    if not sig_fig_count:
        return False
    try:
        decimal = digits.index('.')
    except ValueError:
        return any(digit != '0' for digit in digits[index+1:])
    else:
        return index > decimal

Most of this complexity is avoidable by using string operations and splitting cases:

def count_sig_figs(digits):
    '''Return the number of significant figures of the input digit string'''

    integral, _, fractional = digits.partition(".")

    if fractional:
        return len((integral + fractional).lstrip('0'))
    else:
        return len(integral.strip('0'))

That's the entirety of the logic!

Another test case you should consider is 100.. Does that have 1 significant figure or 3?


Simplifying logic of overlapping predicates I have 3 simple predicates and 3 simple actions to be taken based on those predicates. In my actual application they are not based on integer arithmetic, and in fact are rather expensive to compute (in comparison to the actions taken), but their dependency chain is the same.

#include <iostream>
#include <tuple>

bool pred1(int x) { return x % 30 == 0; }
bool pred2(int x) { return x % 15 == 0; }
bool pred3(int x) { return x %  5 == 0; }

void fun1(int x) { std::cout << "divisible by 30 \n"; }
void fun2(int x) { std::cout << "divisible by 15 \n"; }
void fun3(int x) { std::cout << "divisible by  5 \n"; }

void fun(int x)
{
    if (pred1(x)) {
        fun1(x);
        fun2(x);
        fun3(x);
        return;
    }
    if (pred2(x)) {
        fun2(x);
        fun3(x);
        return;
    }
    if (pred3(x))
        fun3(x);
}

Note that pred1 requires all 3 actions, pred2 the last 2 actions, and pred3 only the 3rd action. I was not happy with this code: while efficient in avoiding extra work, it seems overly repetitive.

However, the following straightforward try at refactoring is a lot more concise but also less efficient as it computes all 3 predicates for all inputs:

// exposition only: will compute pred1, pred2 and pred3 for all inputs
void hun(int x)
{
    if (pred1(x)) fun1(x);
    if (pred2(x)) fun2(x);
    if (pred3(x)) fun3(x);
}

So I came up with the idea of caching the predicate values in a std::tuple and dispatch the various actions based on that:

using Triple = std::tuple<bool, bool, bool>;

auto preds(int x) -> Triple
{
    if (pred1(x)) return Triple{ true,  true, true };
    if (pred2(x)) return Triple{ false, true, true };
    return Triple{ false, false, pred3(x) };
}

void gun(int x)
{
    auto const p = preds(x);
    if (std::get<0>(p)) fun1(x);
    if (std::get<1>(p)) fun2(x);
    if (std::get<2>(p)) fun3(x);
}

Live Example.

So the repetitive logic is hidden inside the repeated values of true in the std::tuple, and the duplicate predicate computation is now reduced to simple lookups into the std::tuple.

Looking at the assembler from gcc.godbolt.org, it seems that the compiler even optimizes away all unnecessary calls to std::get in gun() (e.g. it sees that the remaining fields of the std::tuple will be true if the first one is), essentially reducing the code to that of fun().

Questions:

are there other approaches to simplying the logic of overlapping predicates?
is the gun() version considered more readable and maintainable than the fun() original?
how smart are compilers in eliminating unnecessary lookups when the underying predicates and actions are not so trivial as the ones above?

It seems to me that the primary role of the programmer is to write clear and correct code and that it's largely the job of the compiler to turn that into efficient machine code.  With that said, it seems to me that the first fun() version is both clear and effective in that it avoids the needless overhead of computing conditions that don't need to be computed, and is sufficiently clear about the results of each predicate.

Your gun() version separates the predicates from the actions using an intermediary Triple and while it produces the same result, it makes the job of interpreting the code (for a human) a little bit harder. So I would advocate not doing it that way.

If you'd like to reduce the repetition, the way to do that would be to create subfunctions such as this:

void do123(int x) {
    fun1(x);
    fun2(x);
    fun3(x);
}

Once this is done, the multiple if statements become a bit shorter and easier to read, although at some possible expense in clarity.

If the predicates and functions are dealing solely with integers it doesn't much matter, but if they are dealing with larger items, and particularly objects with constructors and destructors, it would be helpful to use references and const as much as possible to avoid useless object copies.


Modeling a decision tree-like form A picture says more than a thousand words, so here's a mock-up. The actual choices are different, this is just a sample.

A user first has to choose between Red, Blue and Green. Upon selecting an option, additional form elements are displayed, depending on the choice. For Blue, a Date  and Font are required.

Currently, there's no need for a third level, but who knows what the future brings. In order to represent this data, I've come up with the following model (after many attempts).

public class SampleWizard
{
    public enum ColourChoice
    {
        Red,
        Blue,
        Green
    }

    public class RedModel
    {
        public IEnumerable<SelectListItem> AvailableShapes { get; set; }

        public int Shape { get; set; }

        public DateTime Date { get; set; }
    }

    public class BlueModel
    {
        public enum FontChoice
        {
            Arial,
            Helvetica,
            Other,
        }

        public DateTime Date { get; set; }

        public FontChoice Font { get; set; }

        public IEnumerable<SelectListItem> AvailableOtherFonts { get; set; }

        public int OtherFont { get; set; }
    }

    public ColourChoice Colour { get; set; }

    public RedModel Red { get; set; }

    public BlueModel Blue { get; set; }
}

I'm using Enums for hard-coded choices, so I can make decisions based on the value. ints come from the database and do not matter in the programmed decision flows.

This works, but I can't help but feel like this is an ugly solution and I'm worried about future modifications. I've never had to make something like this before so I can't make decisions based on past experiences.

The UI design is more or less set in stone, unless strong arguments can be made against it. (I'd need to bring them up to the designer and project manager.)

Well, i think, you should first ask yourself do you really want to complicate things? Is this something that you will re-use in other places? Because the feeling "i need to create something reusable and extendable" if familiar to every one of us, but the amount of code you'll need to write and debug is not always worth it in the end.

If you answer is "no", then you can simply do a little refactoring: move classes to separate files, replace individual viewmodel properties with Dictionary<ColourChoice, ViewModelBase>, stuff like that. It will end up looking fine.

If your answer is "yes", then i would probably create a tree-like structure from my view models by implementing something like:

interface ITreeViewModel
{
    IEnumerable<ITreeItem> Children { get; }
    bool IsSelected { get; }
    ITreeItem Parent { get; }
}

Then i would create a custom user control wich will display such tree using this interface and specidied DataTemplates (after i will fail to make wpf TreeView work as i want it too yet again -_-). This is a tricky task with plenty of pitfalls, but i think its possible. At least that would be an approach I'd try first.


Using view for repeated subquery on a large table I have a large table with which I need to do some analysis. The table is a log of IPs:

+-------------+---------------------+------+-----+---------+-------+
| Field       | Type                | Null | Key | Default | Extra |
+-------------+---------------------+------+-----+---------+-------+
| src         | int(10) unsigned    | NO   | PRI | 0       |       |
| dst         | int(10) unsigned    | NO   | PRI | 0       |       |
| packets     | int(10) unsigned    | YES  |     | NULL    |       |
| bytes       | int(16) unsigned    | YES  |     | NULL    |       |
| capturedate | int(10) unsigned    | NO   | PRI | 0       |       |
+-------------+---------------------+------+-----+---------+-------+

Every hour the past hours, IP activity is collected. The time this happens is capturedate. At that time each IP pairing (unique source to destination) is stored along with the number of packets and bytes sent from source to destination.

I've written some code for a webpage that queries the database to get a "vertex cover". It collects all the IP pairings and displays each IP and how many other unique IPs it has communicated with.

ip | verticies
123.234.12.34 | 567
234.56.78.91 | 234

So, IP '123.234.12.34' has communicated with 567 other unique IPs (as either source or destination). With millions of records in this table efficiency is an issue.

My solution is this:

First I added a view for a subquery I would reuse:

CREATE VIEW pair AS SELECT src, dst FROM iplogs GROUP BY src, dst;

Then the actual query is...

SELECT INET_NTOA(ip) AS ip, SUM(verts) as verticies
FROM (SELECT src AS ip, COUNT(src) AS verts FROM pair GROUP BY src
UNION ALL
SELECT dst AS ip, COUNT(dst) AS verts FROM pair GROUP BY dst) B
GROUP BY ip;

Is this the most efficient way to get the results? Will the "pair" view update every time the query is made? Will "pair" be queried once or twice?

Since your primary key contains both src and dst you might be able to get away with something like the following:

SELECT IF (src<dst,src,dst), COUNT(*)
FROM   iplogs
GROUP BY 1
UNION
SELECT IF (src<dst,dst,src), COUNT(*)
FROM iplogs
GROUP by 1

An explain suggests the query above can use the primary index (assuming the captured field is not first or that a suitable expression using the captured date is added to the where clause if it is first).

#id, select_type, table,  type,  possible_keys, key,     key_len, ref, rows, Extra
1,   PRIMARY,     iplogs, index, PRIMARY,       PRIMARY, 105,        , 6,    Using index; Using temporary; Using filesort
2,   UNION,       iplogs, index, PRIMARY,       PRIMARY, 105,        , 6,    Using index; Using temporary; Using filesort
 ,   UNION RESULT, <union1,2>,   ALL, , , , , , Using temporary

If you put EXPLAIN in front of your query you'll get details on how it will execute.

Getting down to two index scans should be as efficient as you can get and a bit simpler than using a view.

However, beware, I've only done rudimentary testing on a small data set at this point.

EDIT: With more details provided I've been able to test this and provide an improved response.

The union query given below returns two distinct portions of results. Adding these two together appears to provide the proper total.  Here is a bit of PHP/MySQL code taken from some testing efforts against the supplied data.

$sql  = "SELECT IF (src<dst,src,dst) src\n";
$sql .= "      ,COUNT(DISTINCT(IF (src<dst,concat(src,'|',dst),concat(dst,'|',src)))) answer\n";
$sql .= "      ,COUNT(*) raw\n";
$sql .= "FROM   iplogs\n";
$sql .= "GROUP BY 1\n";
$sql .= "UNION\n";
$sql .= "SELECT IF (src<dst,dst,src)\n";
$sql .= "      ,COUNT(DISTINCT(IF (src<dst,concat(src,'|',dst),concat(dst,'|',src))))\n";
$sql .= "      ,COUNT(*)\n";
$sql .= "FROM iplogs\n";
$sql .= "GROUP by 1\n";
if ( !$res = mysqli_query($link,$sql) )
  die("ERROR: Unable to query vertex counts from database!\n");

However, as you can imagine, you have to post-process the query in order to add the values together to get what should be the right answer.  This could be done by loading the returned records into a vertex table as is if consuming processes group by IP and sum the response.

Alternately, here is some code that does the summing and would produce records suitable for storing in a vertex table.

$qryqty = 0;
$qrylist = array();
while ( $row = mysqli_fetch_assoc($res) )
{
  $src = $row['src'];
  if ( !isset($qrylist[$src]) )
    $qrylist[$src] = 0;
  $qrylist[$src] += $row['answer'];

  $qryqty++;
}
mysqli_free_result($res);
echo "$qryqty records loaded via query<br>\n";

So, I guess there is a trade off between SQL efficiency and having to do some extra processing to sum the results either before or after storing them.

NOTE: There are no IP translations as they were loaded as text for this effort.