Python Hangman (guess the word) game I implemented a Hangman/guess the word clone in Python. This is my first attempt at making an actual interactive app instead of just a list of functions or a static program. It is also the first time I try to actually implement classes since I've been having a hard time wrapping my head around them. It's probably not the best or most practical application for a class, but it saved me the trouble of having to go around declaring globals and nonlocals throughout the code.

import json
import random
from os import system, name
import msvcrt
import colorama

class Hangman:
    def __init__(self):
        self.level = ""
        self.word = ""
        self.wrong_chars = []
        self.from_victory = 0
        self.from_defeat = 0
        self.game_state = ""
        self.on_repeat = True

    @staticmethod
    def cls():
        system('cls' if name == 'nt' else 'clear')

    def difficulty_scene(self):
        print("""
  DIFFICULTY LEVEL:
  ------------------

  (1) Very easy.
  Very long words (10+ characters) and 10 lives

  (2) Easy.
  Long words (8 or 9 characters) and 8 lives

  (3) Standard.
  Medium words (6 or 7 characters) and 6 lives

  (4) Hard.
  Short words (4 or 5 characters) and 4 lives

  (5) Impossible.
  Tiny words (3 characters) and 3 lives.

    """)
        self.level = input("  Select difficulty (1-5): ")

    def game_prep(self):
        self.word = word_list[-int(self.level) + 5][int(random.random()
                                                        * len(word_list))]
        self.wrong_chars = []
        self.from_victory = len(self.word)
        self.from_defeat = self.make_from_defeat()
        self.game_state = ""

    def make_from_defeat(self):
        return {
            "1": 10,
            "2": 8,
            "3": 6,
            "4": 4,
            "5": 3
        }.get(self.level, 6)

    def main_scene(self):
        colorama.init()

        print(f"\n  {'_' * len(self.word)}")

        while True:
            char = msvcrt.getch().decode('UTF-8')
            if char in self.word:
                for i in range(len(self.word)):
                    if char == self.word[i]:
                        print(f'\x1b[2;{i+3}H', end="")
                        print(char, end="")
                        self.from_victory -= 1
            else:
                self.wrong_chars.append(char)
                print(f'\x1b[4;{((len(self.wrong_chars) - 1) * 2) + 3}H', end="")
                print(char, end="")
                self.from_defeat -= 1
            if self.from_victory == 0 or self.from_defeat == 0:
                break

    def end_scene(self):
        if self.from_victory == 0:
            print("\n  YOU HAVE WON")
        elif self.from_defeat == 0:
            print("\n  YOU HAVE LOST")
            print(f"  your word was {self.word}")
        repeat = input("\n  Try again? (y/n):\n  ")
        if repeat == "y":
            self.on_repeat = True
        else:
            self.on_repeat = False

    def main(self):
        self.cls()
        self.difficulty_scene()
        self.cls()
        while self.on_repeat:
            self.game_prep()
            self.main_scene()
            self.cls()
            self.end_scene()
            self.cls()

game = Hangman()

if __name__ == '__main__':

    with open("words.json", "r") as read_file:
        word_list = json.load(read_file)

    game.main()

words.json is a 2-dimensional array of words sorted by length.

Docstrings: You should include a docstring at the beginning of every method, class, and module you write. This will help any documentation identify what your code is supposed to do.
Simplified boolean comparisons:

You have code like:

if repeat == "y":
    self.on_repeat = True
else:
    self.on_repeat = False

Instead of having an if/else and setting the boolean condition based on that, you can simply set the value of self.on_repeat based off the first check, like so:

self.on_repeat = repeat == "y"

Meaningful method names: Method names should be based off what the code inside them does. While some of your methods do (main_scene, end_scene), some of them do not. make_from_defeat for example. I had no clue what the same was trying to suggest about the method, and it doesn't seem to correlate to what the method does. I left them unchanged so you could figure out for yourself what those method names should be.
Constant Variable Naming: All constant variables in your program should be UPPERCASE, to distinct them from non-constants and make it clear to you/a reviewer that they are constants.

Updated Code

"""
Module Doctring:
A description of your program goes here
"""
import json
import random
from os import system, name
import msvcrt
import colorama

class Hangman:
    """
    Class for storing properties of the object `Hangman`
    """
    def __init__(self):
        self.level = ""
        self.word = ""
        self.wrong_chars = []
        self.from_victory = 0
        self.from_defeat = 0
        self.game_state = ""
        self.on_repeat = True

    @staticmethod
    def cls():
        """ Clears the console """
        system('cls' if name == 'nt' else 'clear')

    def difficulty_scene(self):
        """ Prints the difficulty options """
        print("""
  DIFFICULTY LEVEL:
  ------------------

  (1) Very easy.
  Very long words (10+ characters) and 10 lives

  (2) Easy.
  Long words (8 or 9 characters) and 8 lives

  (3) Standard.
  Medium words (6 or 7 characters) and 6 lives

  (4) Hard.
  Short words (4 or 5 characters) and 4 lives

  (5) Impossible.
  Tiny words (3 characters) and 3 lives.

    """)
        self.level = input("  Select difficulty (1-5): ")

    def game_prep(self):
        """ Prepares the game """
        self.word = WORD_LIST[-int(self.level) + 5][int(random.random()
                                                        * len(WORD_LIST))]
        self.wrong_chars = []
        self.from_victory = len(self.word)
        self.from_defeat = self.make_from_defeat()
        self.game_state = ""

    def make_from_defeat(self):
        """ Retuns the length of the word to guess """
        return {
            "1": 10,
            "2": 8,
            "3": 6,
            "4": 4,
            "5": 3
        }.get(self.level, 6)

    def main_scene(self):
        """ Determines if letter is inside word """
        colorama.init()

        print(f"\n  {'_' * len(self.word)}")

        while True:
            char = msvcrt.getch().decode('UTF-8')
            if char in self.word:
                for i in range(len(self.word)):
                    if char == self.word[i]:
                        print(f'\x1b[2;{i+3}H', end="")
                        print(char, end="")
                        self.from_victory -= 1
            else:
                self.wrong_chars.append(char)
                print(f'\x1b[4;{((len(self.wrong_chars) - 1) * 2) + 3}H', end="")
                print(char, end="")
                self.from_defeat -= 1
            if self.from_victory == 0 or self.from_defeat == 0:
                break

    def end_scene(self):
        """ Determines if the player has won or list """
        if self.from_victory == 0:
            print("\n  YOU HAVE WON")
        elif self.from_defeat == 0:
            print("\n  YOU HAVE LOST")
            print(f"  your word was {self.word}")
        repeat = input("\n  Try again? (y/n):\n  ")
        self.on_repeat = repeat == "y"

    def run(self):
        """ Runs the game """
        self.cls()
        self.difficulty_scene()
        self.cls()
        while self.on_repeat:
            self.game_prep()
            self.main_scene()
            self.cls()
            self.end_scene()
            self.cls()

if __name__ == '__main__':

    GAME = Hangman()

    with open("words.json", "r") as read_file:
        WORD_LIST = json.load(read_file)

    GAME.run()


Determining a digit on a given position in the A002260 sequence This is my solution to this SO question, which can formulated as

take the sequence A002260, i.e., 1, 1, 2, 1, 2, 3, ...
concatenate all its digits
find the digit at a given position (1-based)]

What bothers me is that my solution is rather complicated and fails near Long.MAX_VALUE due to overflow. I also wonder if this could be done in an error-proof way, since I ran through quite some off by one errors.
There's the code, a test and a demo.
public class Ogen {
    /** The length of the number {@code n} (in decimal representation) */
    int length(long n) {
        return LongMath.log10(n, RoundingMode.DOWN) + 1;
    }

    /**
     * The total number of digits in the sequence {@code 1, 2, ..., n}, i.e.,
     * the sum of {@code length(i)} of all positive {@code i} up to and including {@code n}.
     * */
    long cumulativeLength(long n) {
        if (n==0) return 0;
        long result = 0;
        final int maxLength = length(n);
        for (int numberLength=1; numberLength<=maxLength; ++numberLength) {
            // all numbers between min and max have the length numberLength
            final long min = LongMath.pow(10, numberLength-1); // 1, 10, 100, ... highest power of 10 not greater than n
            final long max = Math.min(10*min - 1, n); // 9, 99, 999, ..., n
            final long countOfDifferentNumbers = max - min + 1;
            result += numberLength * countOfDifferentNumbers * 1;
        }
        return result;
    }

    /**
     * The total number of digits in the sequence {@code 1, 1, 2, 1, 2, 3, ..., 1, ..., n}, i.e.,
     * the sum of {@code cumulativeLength(i)} of all positive {@code i} up to and including {@code n}.
     */
    long doublyCumulativeLength(long n) {
        if (n==0) return 0;
        long result = 0;
        final int maxLength = length(n);
        for (int numberLength=1; numberLength<=maxLength; ++numberLength) {
            // all numbers between min and max have the length numberLength
            final long min = LongMath.pow(10, numberLength-1); // 1, 10, 100, ... highest power of 10 not greater than n
            final long max = Math.min(10*min - 1, n); // 9, 99, 999, ..., n
            final double avg = 0.5 * (min+max);
            final double averageNumberOfOccurrences = n - avg + 1;
            final long countOfDifferentNumbers = max - min + 1;
            result += numberLength * countOfDifferentNumbers * averageNumberOfOccurrences;
        }
        return result;
    }

    /**
     * The biggest number {@code n} such that the sequence {@code 1, 1, 2, 1, 2, 3, ..., 1, ..., n} ends before
     * position {@code pos}, i.e., such that {@code doublyCumulativeLength(n) < pos}.
     */
    long fullSequenceBefore(long pos) {
        long lo = 0;
        // find upper bound
        long hi = (long) (Math.sqrt(pos)) / 2;
        while (doublyCumulativeLength(hi) < pos) {
            lo = hi;
            hi += 1 + (hi >> 4);
        }
        // binary search
        while (hi>lo+1) {
            final long mid = (hi+lo) / 2;
            if (doublyCumulativeLength(mid) >= pos) {
                hi = mid;
            } else {
                lo = mid;
            }
        }
        assert doublyCumulativeLength(lo) < pos;
        assert doublyCumulativeLength(lo+1) >= pos;
        return lo;
    }

    public char digitAt(long pos) {
        pos -= doublyCumulativeLength(fullSequenceBefore(pos));
        long lo = 0;
        long hi = pos;
        while (hi>lo+1) {
            final long mid = (hi+lo) / 2;
            if (cumulativeLength(mid) >= pos) {
                hi = mid;
            } else {
                lo = mid;
            }
        }
        pos -= cumulativeLength(lo);
        return String.valueOf(lo+1).charAt((int) pos - 1);
    }
}

Frankly I have been staring at the code for ... 20 minutes, and I can't figure out your algorithm. I can't even throw out a good guess as to whether the result is right, or not.

There are some style-related issues that do not help:

Cumulative is not spelled with 2 m's, and no e as you have it in doublyCummulativLength
The self-referential comments are not useful if you can't figure out where the logic/code should start from. Building up on the algorithm would be more possible if the right starting point was given. In other words, your comments assume too much about what people reaing your code will know (or otherwise I am too dumb to see the obvious stuff so the comments are not helping me).
You reference the class LongMath but that class is not included in the code.
Your comments mention things like: // ... and occur on the average count2/2 times What does that mean? How can you be accurate to an exact digit with reasoning like that?
Code like:

    final long count2 = 2*n - (min+max) + 2;
    result += i * (max-min+1) * count2 / 2;

makes little, or no sense to me. count2? What does that mean? max and min are not great either. Lots of constants too.

In general, the code is not primed for a sight-unseen review. I would need you sitting beside me so I can ask questions as I go through. That is not really suitable for Code review.

As an aside, I believe having a class that represents the sequence that is being used would make a really big difference for this code. You need to extract out the class, and iterate it to find 'the spot'.

I played around with this problem, and specifically targetted an overflow-safe solution, so that digitAt(Long.MAX_VALUE) will work. This is the code I have.

Note that I use a class, which allows me to keep a state, and then add to the state.

On my computer the test for digitAt(Long.MAX_VALUE) returns 4 in less than 2.5 seconds. I do not know if this is a good result, or not.

Also, I compared my results with a naive implementation for relatively small values.

Make of this what you will:

import static org.junit.Assert.*;
import org.junit.Test;

public class Seq2260 {

    public static char digitAt(final long index) {
        if (index < 1) {
            throw new IllegalArgumentException("No such position " + index);
        }
        Seq2260 sequence = new Seq2260();
        sequence.incrementPast(index);
        // OK, spans have been added until the current span contains the index.
        // locate the actual digit in the current span.
        return sequence.seekChar(index - sequence.nextOffset);
    }

    // SCALES indicate the number of digits in each scale, and the value where the scale changes too.
    // for example, SCALES[1] are 1-digit values, and when you hit the value 10, you are no longer
    // in SCALES[1].
    // SCALES[0] is only there to make the indexes line up.
    // The Long.MAX_VALUE is only there to make the range end.
    // Note that only Scales up to SCALES[9] will actually be used.
    // by the time you reach SCALES[10] (10,000,000,000) each number is 10 digits long
    // and the cumulative effect means that the index is now huge.
    private static final long[] SCALES = {0L, 10L, 100L,
        1_000L, 10_000L, 100_000L,
        1_000_000L, 10_000_000L, 100_000_000L,
        1_000_000_000L, 10_000_000_000L, 100_000_000_000L,
        1_000_000_000_000L, 10_000_000_000_000L, 100_000_000_000_000L,
        1_000_000_000_000_000L, 10_000_000_000_000_000L, 100_000_000_000_000_000L,
        1_000_000_000_000_000_000L, Long.MAX_VALUE
    };

    //number of digits in a number
    private int scale = 1;
    // how far along the next span will start
    // start at 1 to be 1-based
    private long nextOffset=1;
    // how long the current span is.
    private long length = 0;
    // the last value in the span
    private long span = 0;

    private void incrementPast(final long index) {
        // this is an overflow-safe compare
        // nextOffset could hypothetically be a very small negative number (wrapped). The math
        // here is safe though, because a very large number subtract a very negative number
        // will overflow again and still be less than length.
        while (index - nextOffset >= length) {
            extendSpan();
        }
    }

    private void extendSpan() {
        if (nextOffset < 0) {
            // we have previously overflowed our limit. We only support a single overflow.
            throw new IllegalStateException("Attempt to extend mapping beyond Long.MAX_VALUE");
        }
        span++;
        if (span == SCALES[scale]) {
            scale++;
            if (span == Long.MAX_VALUE) {
                throw new IllegalStateException("Overflowed Long in a span....");
            }
        }
        // for large values, this will overflow. That is the limit of where we can map to.
        // we can handle one overflow (a negative offset). After that, we fail.
        nextOffset += length;
        length += scale;
    }

    private char seekChar(long seek) {
        int sc = 1;
        long pos = 0;
        long val = 0;
        while (pos <= seek) {
            val++;
            if (val == SCALES[sc]) {
                sc++;
            }
            pos += sc;
        }
        // OK, so val includes the char, and it is....
        int charfromright = (int)(pos - seek);
        String value = String.valueOf(val);
        return value.charAt(value.length() - charfromright);
    }

    public static void main(String[] args) {
        System.out.printf("Confirm %s is %s%n", Seq2260.digitAt(1000), "" + smallGet(1000));
        System.out.printf("Confirm %s is %s%n", Seq2260.digitAt(10000), "" + smallGet(10000));

    }

    @Test
    public void testNaieveMatch() {
        for (int i = 3; i < 64 * 1024; i += 19) {
            assertTrue(smallGet(i) == Seq2260.digitAt(i));
        }
    }

    @Test
    public void testNaieveSequential() {
        for (int i = 1; i < 4 * 1024; i++) {
            assertTrue(smallGet(i) == Seq2260.digitAt(i));
        }
    }

    private static char smallGet(int i) {
        StringBuilder sb = new StringBuilder(i + 10);
        sb.append("-");

        int oldmax = 1;
        int current = 0;
        while (sb.length() <= i) {
            current++;
            sb.append(current);
            if (current == oldmax) {
                current = 0;
                oldmax++;
            }
        }
        return sb.charAt(i);
    }

    @Test
    public void testGetDigitAt() {
        assertEquals('1', Seq2260.digitAt(1));
        assertEquals('1', Seq2260.digitAt(2));
        assertEquals('2', Seq2260.digitAt(3));
        assertEquals('1', Seq2260.digitAt(4));
        assertEquals('2', Seq2260.digitAt(5));
        assertEquals('3', Seq2260.digitAt(6));
        assertEquals('1', Seq2260.digitAt(7));
        assertEquals('2', Seq2260.digitAt(8));
        assertEquals('3', Seq2260.digitAt(9));
        assertEquals('4', Seq2260.digitAt(10));

        assertEquals('5', Seq2260.digitAt(20));
        assertEquals('5', Seq2260.digitAt(33));

        assertEquals('9', Seq2260.digitAt(54));
        assertEquals('1', Seq2260.digitAt(55));
        assertEquals('0', Seq2260.digitAt(56));

        assertEquals('0', Seq2260.digitAt(67));
        assertEquals('2', Seq2260.digitAt(99));
    }

    @Test
    public void testLongMax() {
        assertEquals('4', Seq2260.digitAt(Long.MAX_VALUE));
    }

}


Django Unified Method for Deleting Instances of Any Model This code works, but I don't know if there are better and especially safer ways to do this.

For context: this is a Django view (Python) and I wanted to unify the deletion methods because there were many of them.

def delete(request, class_name, object_ID):
    obj = get_object_or_404(eval(class_name), id=object_ID)
    obj.delete()
    return redirect(dashboard)

Now I know that the eval() method should be used with caution, but would it be enough to wrap a try-catch-method around it or should I do something like a switch-case, or is it even ok to use the code as is?

It will be called from this url route (urls.py):

path('delete/<str:className>/<int:object_ID>', views.delete)

  Note: This route will only be accessible to employees and there will be a permission check, so my question is about best practice and potential problems.

The use of eval() here is less than ideal as it allows you to call the URL delete/User/3/, for instance, and it will work against all odds. Worse, you can call the URL delete/delete/9 where eval will resolve delete to this view function and lead to all kind of weird behavior.

So the first step is to restrict which symbols can be found from there. The easiest way is to retrieve the attribute directly from your models:

from . import models

...

def delete(request, type_name, object_id):
    try:
        cls = getattr(models, type_name)
    except AttributeError:
        raise Http404
    get_object_or_404(cls, id=object_id).delete()
    redirect(dashboard)

But this is still too large as anything that you imported in models can still be retrieved. So you're better off using a base class in your models.py that only your deletable models will inherit from:

class Deletable:
    def can_delete(self, user):
        return user.is_active and user.is_staff

class OneOfYourCustomModel(Deletable, models.Model):
    # whatever

And check in your view that whatever you get is Deletable:

def delete(request, type_name, object_id):
    try:
        cls = getattr(models, type_name)
    except AttributeError:
        raise Http404
    if inspect.isclass(cls) and issubclass(cls, models.Deletable):
        get_object_or_404(cls, id=object_id).delete()
    else:
        raise Http404
    redirect(dashboard)

You'll also note that it also lead to easy customization of which user can delete what: you just need to override one method and add proper logic (eg raising 403 forbidden) in your view. This can be more useful than the @staff_member_required in case your models can be associated to (owned by) a particular user:

class MyOwnedModel(Deletable, models.Model):
    owner = models.ForeignKey(django.contrib.auth.models.User, …)

    def can_delete(self, user):
        return user == self.owner or super().can_delete(user)

But of course, if only the staff can delete things, the decorator is plenty sufficient.

Lastly, it would be good to separate the "retrieve the actual model" logic from the "delete the particular instance of that model" one. The latter being the responsibility of the view, but the former would better fit as a custom URL converter.

The main advantage being that you don't need to handle the 404s for unknown models in your view, django will take care of that itself if your converter ever raises a ValueError. Going this route, you can have a converters.py:

from inspect import isclass

from django.urls.converters import StringConverter

from . import models

class DeletableConverter(StringConverter):
    def to_python(self, value):
        try:
            cls = getattr(models, value)
        except AttributeError:
            raise ValueError
        if not isclass(cls) or not issubclass(cls, models.Deletable):
            raise ValueError
        return cls

    def to_url(self, value):
        return value.__name__

Usage in your urls.py:

from django.urls import path, register_converter

from . converters import DeletableConverter

register_converter(DeletableConverter, 'deletable')

urlpatterns = [
    …,
    path('delete/<deletable:model>/<int:object_id>', views.delete),
    …,
]

And in your view:

def delete(request, model, object_id):
    instance = get_object_or_404(model, id=object_id)
    if instance.can_delete(request.user):
        # or a simple @staff_member_required decorator
        instance.delete()
    redirect(dashboard)


Java merge sort implementation efficiency The problem was to implement merge sort algorithm in Java, where I recursively split and merge the array until I have the sorted array. Even though the code is working properly, I feel it is verbose and can still be improved.

  private static int[] Split_merge(int[] a) {

    int splitindex = a.length/2;
    int[] lhs=Arrays.copyOfRange(a, 0, splitindex);
    int[] rhs= Arrays.copyOfRange(a, splitindex, a.length);
    if(lhs.length==1 || rhs.length==1){
      return a= merge(lhs, rhs);

    }else {
       lhs= Split_merge(lhs);
      rhs=  Split_merge(rhs);
        return a=merge(lhs, rhs);

    }

}

private static int[] merge(int[] lhs, int[] rhs) {
    int[] result = new int[lhs.length+rhs.length];
    int reslutindex=0;
    int lhsindex=0;
    int rhsinsex=0;
            while(reslutindex<=result.length-1){

                    if(rhsinsex>=rhs.length ||lhsindex<=lhs.length-1 && lhs[lhsindex]<=rhs[rhsinsex]){
                        result[reslutindex] = lhs[lhsindex];
                    lhsindex++;
                    reslutindex++;
                    }else {
                    if(rhsinsex<=rhs.length-1){
                          result[reslutindex] = rhs[rhsinsex];
                    rhsinsex++;
                    reslutindex++;
                    }

                }

                }

    return result;

}

First of all your indentation is really bad. This makes the code hard to read and not so fun to review.

The easiest solution to this is to get yourself a nice IDE (most popular ones are eclipse and IntelliJ) and have that IDE auto format your code.

Then to properly put the code in the textbox on this site just paste it, select it entirely and press the {} button at the top.

As for your algorith I don't fully understand why you keep creating new arrays. The usual implementation for sorting an array does so in place. So I would expect the method signature to look like this:

public static void sort(int[] array) {

Which then calls a helper method that also uses indicates which part of the array has to be sorted:

public static void sort(int[] array) {
    sort(array, 0, array.lenght-1);
}

public static void sort(int[] array, int start, int end) {

This method now doesn't physically split the array, but just looks at half the range, get's both sides sorted and then merges both halves in place into sorted order.

public static void sort(int[] array, int start, int end) {
    if(end == start) {
        return;
    }
    int middle = (end-start)/2;
    sort(array, start, middle);
    sort(array, middle+1, end);

    //TODO merge array[start:middle] with array[middle+1:end]
}

Before going into the merge implementation let's take a look at the space complexity of your current one.

Since the array get's split into 2 each time, the method recursively call's itself up to O(log N) steps (with N the size of the array and log meaning the 2-log). At each step you copy the given array into 2 new variables. And call the recursion with half the array. So the total needed memory for storing your arrays is I think O(N+(log N)²).

Now notice that the part of the algorithm I have given does not reserve any extra space for arrays. So up till now it's still running O(1) space. And this is including the recursive steps already.

As for actually merging, although possible to do in place, I would now take a temp copy of the array. This makes merging a lot easier at the cost of O(N) space. We do have to be careful at which one we actually put the result in though. Because we want our input array to remain sorted even after returning from this method.

For this reason we can never reassign the array variable. We have to do array[index]=newValue to achieve this.

Here's an idea about how to implement this:

int leftIndex = 0;
int rightIndex = middle;
int currentIndex = 0;
int[] partialSorted = Arrays.copyOf(array, array.length);
while(both parts not finished){
    if(partialSorted[leftIndex] < partialSorted[rightIndex]){
        array[currentIndex++] = partialSorted[leftIndex++];
    } else {
        //similar for right side
    }
}
//copy remainder of left side
//copy remainder of right side

The parts I left out shouldn't be too hard to figure out yourself. I didn't want to just give you the entire solution as that would take out all the fun.

EDIT 2: You can further reduce the space requirement in half if you only store the left half into a temporary copy. This also saves you from copying the remainder of the right hand side after the while loop.

EDIT: Technically this last step of copying the array to a temp and putting it back into the original in the correct place makes this implementation no longer in-place. The true in place implementation is a lot harder than I expected :)

A final optimisation you should consider is before actually merging to first see if it is already sorted: (last element of left half is smaller than first element of right half). Because then you can just return without copying anything at that iteration. This is a serious improvement in space complexity when the input is already a sorted array (turning it into O(1)).


F# wrapper to generate SHA256 signature for a file I've created the following module to allow me to generate a SHA256 signature for a file. (In real life this is used to verify an image file hasn't been amended).  The cut down code looks like this:

open System.IO
open System.Text
open System.Security.Cryptography;

// Get the SHA256 hash of a file
let SHA256 (file:FileInfo) =
    let FileSHA256Wrap (hashFile : FileStream) (sha256Hash : SHA256)  : byte[] =
        sha256Hash.ComputeHash(hashFile)
    let FileWrap (hashFile : FileStream) : byte[] =
        using (SHA256Managed.Create()) (FileSHA256Wrap hashFile)
    using (file.Open(FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) FileWrap

// Convert the byte[] version of the hash to a printable encoded HEX string
let HexEncoded (hash:byte[]) :string =
    let sb = new StringBuilder(hash.Length * 2)
    hash |> Array.map (fun c -> sb.AppendFormat("{0:X2}",c)) |> ignore
    sb.ToString()

// Get the file hash and convert it to a HEX string
let HexEncodedSHA256 (file:FileInfo) =
    file |> SHA256 |> HexEncoded

let fi = new FileInfo("somefile.tif")
HexEncodedSHA256 fi

I've two questions here, with the double using statements I find the wrap functions help me work out what is going on - but is there a neater and more succinct way to write this without making it hard to work out what is happening?

The implementation of HexEncoded uses a string builder, is there a more functional way to do this.

The use binding is usually better than the using function. The object is disposed when leaving the scope of the use binding (when leaving the SHA256 function).

let SHA256 (file:FileInfo) =
    use hashFile = file.Open(FileMode.Open, FileAccess.Read, FileShare.ReadWrite)
    use sha256Hash = SHA256Managed.Create()
    sha256Hash.ComputeHash(hashFile)

You can avoid the use of a string builder by using String.concat. You can also use the F# sprintf function, which has slightly different format specifier syntax.

let HexEncoded (hash:byte[]) :string =
    hash
    |> Array.map (sprintf "%02X")
    |> String.concat ""

This might be slightly slower than your string builder because it requires an intermediate array. (Note that your implementation could remove the intermediate array by using Array.iter instead of Array.map and moving the ignore to inside the lambda.)


Retrieving a csv header I have two seperate calls that returns a .csv file. I need to extract a couple of headers from the files depending on which one is called. The headers does not always sit in the same index, and the CourseId name changes to Asset_ID in the one call.

How can I refactor the code to make it more maintainable?

Also, I think the naming is not really good.

public static class CsvHeader
        {
            public struct Index
            {
                public string Email { get; set; }
                public string CompletionDate { get; set; }
                public string CourseId { get; set; }
            }
            public static Index GetCsvHeaderIndex(string[] headers)
            {
                string emailIndex = null;
                string completionDateIndex = null;

                var email = headers.FirstOrDefault(_ => _.Contains("Email"));
                var completionDate = headers.FirstOrDefault(_ => _.Contains("Completion"));
                var courseId = headers.FirstOrDefault(_ => _.Contains("CourseID"));

                if (!string.IsNullOrEmpty(email)) emailIndex = headers.First(_ => _.Contains("Email")).Split('(', ')')[1]; /*3*/
                if (!string.IsNullOrEmpty(completionDate)) completionDateIndex = headers.First(_ => _.Contains("Completion")).Split('(', ')')[1]; /*30*/
                var courseIdIndex = string.IsNullOrEmpty(courseId) ?
                    headers.First(_ => _.Contains("Asset_ID")).Split('(', ')')[1] :
                    headers.First(_ => _.Contains("CourseID")).Split('(', ')')[1];

                return new Index
                {
                    Email = emailIndex,
                    CompletionDate = completionDateIndex,
                    CourseId = courseIdIndex
                };
            }
        }

Kudos for creating a struct to strong type the data, but what does Index mean? It's an odd term for course information. I'd look for a better name.
Also, don't string type all the things. You have a date here, you should store it as a date.

public string CompletionDate { get; set; }

MSDN article on parsing dates from strings.
You're duplicating string literals like "Email". Extract constants for these.
You're splitting on the same character array in several places, but recreating the array for each call. Extract a local variable for ['(', ')']


Concatenating multiple strings from a JSON response I would like to concatenate a series of strings populated from an XHR response. So, I've chosen to do it like this:

$.post('/API',null,'json')
    .done(function(response){
        if (response.status === 'success') {
            location = response.data.city + ', ' + response.data.state;
        }
    })
    .fail(function(){
        location = previousLocation;
    })
    .always(function(){
        $("#location").text(location);
    });

This feels clunky. Should I be more defensive in assuming a response from the API? Would this be more elegantly handled by some kind of templating function?

I would suggest using then whenever possible. done and fail are specific to jQuery while then is a "standard" pseudo-interface for promises.

And if my jQuery isn't mistaken, the success handler will fire for 2xx and 3xx responses, while the fail handler will fire on 4xx and 5xx responses. You should take advantage of those instead of returning and checking statuses manually. The only time I remember APIs require using 2xx for errors is for Flash, for some reason.

Assuming response always has a data field and you want everything in the data field to be concatenated, you can use Object.keys, array.map and array.join.

Your code should end up looking like this. Two-space indent is a personal preference, but I go with it because it looks more compact.

$.post('/API',null,'json').then(function(response){
  location = Object.keys(response.data).map(function(key){
    return response.data[key];
  }).join(', ');
}, function(){
  location = previousLocation;
}).always(function(){
  $("#location").text(location);
});


Getting the right conditional ConfigId Many conditions need to be scanned for the first one that matches a set of tests. If there is a matching condition, that conditions ConfigId should be returned.

I have a way to do the matching of conditions, but there are too many continue statements. Is there a better way to perform these checks?

private Integer match(List<ConfitionEntity> conditions, ActualDto actualDto) {

    if (CollectionUtils.isEmpty(conditions)) {
        return null;
    }
    Integer configId = null;
    for (ConfitionEntity condition : conditions) {
        boolean match = matchState(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchStatus(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchType(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchPre(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchMobile(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchAge(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchNumber(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchPassed(condition, actualDto);
        if (!match) {
            continue;
        }
        match = matchName(condition, actualDto);
        if (match) {
            configId = condition.getConfigId();
            break;
        }

    }
    return configId;
}

With the Java functional extensions you can "easily" encode these tests as a BinaryPredicate short-circuitung and set. See the documentation for The and method on BiPredicate.

Consider the following code which you can add-to as your conditions change:

private static final BiPredicate<QueueMsgPushConditionEntity, QueueUpdatedDto> MATCH = ((c, q) -> c != null && q != null)
    .and((c, q) -> matchQueueState(c.getQueueState(), q.getState()))
    .and((c, q) -> matchFirstPush(c.getFirstPush(), q.getSerialId())
    .and((c, q) -> matchPushType(c.getPushType(), PushType.QUEUE_CALL))
    .......

Now, with that static lambda/functional expression, you can change your match function to look like:

private Integer match(List<QueueMsgPushConditionEntity> conditions,
                      QueueUpdatedDto queueUpdatedDto) {

    Optional<QueueMsgPushConditionEntity> first = conditions.stream()
        .dropWhile(c -> !MATCH.test(c, queueUpdatedDto))
        .findFirst();

    return first.map(c -> c.getQueueMsgPushConfigId()).or(null);

}

What the above stream does, is go through all the conditions, and it throws them all away until it finds one that matches all the MATCH tests, and then it stops looking. If/when it finds one, it returns it as the Optional, which is mapped to the Integer, or null if nothing was found.


Performance of Min and Max Sum of array in C# Print two space-separated long integers denoting the respective minimum and maximum values that can be calculated by summing exactly four of the five integers.

Stuck at understanding a hint given in question.

Hint: Beware of integer overflow! Use 64-bit Integer.

Code:

string[] arr_temp = Console.ReadLine().Split(' ');
int[] arr = Array.ConvertAll(arr_temp, Int32.Parse);
arr = arr.OrderBy(i => i).ToArray();
int mine = arr[0];
int maxe = arr[arr.Length - 1];
int sum = 0;
for (int i = 0; i < arr.Length; i++)
{
    if(arr[i]>=1 && arr[i]<=Math.Pow(10,9))
     sum+= arr[i];
}
Console.WriteLine((sum-maxe) + " " + (sum-mine));

Note : The motive of posting here, is to improve learning over code review, solution is already on multiple blogs.

Performance
The current solution does two things:

Sort the inputs,   O(N log(N))
Sum the inputs,    O(N)

It is possible to calculate the result in a single pass O(N) by reading through the inputs, keeping track of the min, max and sum as we progress and then subtracting the min and max from the sum as we are currently doing.

Note: For the current code

if(arr[i]>=1 && arr[i]<=Math.Pow(10,9))
     sum += arr[i];

does nothing useful, the problem definition says that the inputs are in the range [1,10^9] so the test will never fail.

Religious war trigger
I would push for always using braces around a block, even single line blocks. I have seen too many cases of

if(arr[i]>=1 && arr[i]<=Math.Pow(10,9))
     sum += arr[i];
     // do something else every time, not just when the test passes

The Hint
The 64- bit integer hint means that the sum of the values may exceed the maximum value for a int, so use a long for the sum.


Is this a decent way to create an EntityManager? I'm pretty new to C++.  I'm trying to design a EntityManager system, and so far this is what I have:

#ifndef ENTITY_MANAGER
#define ENTITY_MANAGER

#include "Entity.h"
#include "Airplane.h"
#include "Runway.h"
#include "Customer.h"
#include <unordered_map>

class EntityManager
{
private:
    typedef std::unordered_map<int, Entity*> EntityMap;

    EntityMap entityMap;
public:
    /*
    Gets an instance of the EntityManager
    */
    static EntityManager* instance();

    /*
    Creation and Deletion
    */

    /*
    Registers an Entity
    */
    void registerEntity(Entity* entity)
    {
        entityMap.insert(std::pair<int, Entity*>(entity->getId(), entity));
    }

    /*
    Removes an Entity by its ID
    */
    void removeEntity(int id)
    {
        entityMap.erase(id);
    }

    /*
    Removes an Entity by its instance
    */
    void removeEntity(Entity* entity)
    {
        removeEntity(entity->getId());
    }

    /*
    Gets an Entity by its id
    */
    Entity* getById(int id)
    {
        return entityMap.at(id);
    }

    /*
    Gets all the Airplanes
    */
    std::set<Airplane*> getAirplanes()
    {
        std::set<Airplane*> result;
        for (std::unordered_map<int, Entity*>::iterator it = entityMap.begin(); it != entityMap.end(); it++)
        {
            Entity* e = it->second;
            Airplane* plane = dynamic_cast<Airplane*>(e);
            if (plane)
            {
                result.insert(plane);
            }
        }
        return result;
    }

    /*
    Gets all the Runways
    */
    std::set<Runway*> getRunways()
    {
        std::set<Runway*> result;
        for (std::unordered_map<int, Entity*>::iterator it = entityMap.begin(); it != entityMap.end(); it++)
        {
            Entity* e = it->second;
            Runway* runway = dynamic_cast<Runway*>(e);
            if (runway)
            {
                result.insert(runway);
            }
        }
        return result;
    }

    /*
    Gets all the Customers
    */
    std::set<Customer*> getCustomers()
    {
        std::set<Customer*> result;
        for (std::unordered_map<int, Entity*>::iterator it = entityMap.begin(); it != entityMap.end(); it++)
        {
            Entity* e = it->second;
            Customer* customer = dynamic_cast<Customer*>(e);
            if (customer)
            {
                result.insert(customer);
            }
        }
        return result;
    }
};
#endif

A little background:

Entity is my base class, which handles assigning itself an id, etc.
Airplane is a subclass of Entity, which has methods that describe its size, fuel, etc
Runway is a subclass of Entity, which also has methods to describe its size
Customer is a subclass of Entity, which has methods describing its AI state, etc

I'm looking for any help I can get in things like code correction and design flaws.

Ahhhhhhhhh
static EntityManager* instance();

No ownership associated with even the manager.
If I get a manager. Who is responsible for destroying it?
// Do this.
// Use the classic singleton pattern.
static EntityManager& instance()
{
     static EntityManager   managerInstance;
     return managerInstance;
}

https://stackoverflow.com/questions/1008019/c-singleton-design-pattern/1008289#1008289
Who owns the entity?
void registerEntity(Entity* entity)

Is the manager supposed to take ownership. Which means it will take responsibility for destroying the entity? Or are you giving the manager a reference it can use but does not own (thus it will not destroy).
As it stands. This is completely the wrong way of doing it. Once you tell me how you want to define the semantics of the interface I can tell you how to proceed.

Update The manager is supposed to take ownership.
So lets continue:
void registerEntity(Entity* entity)
{
    entityMap.insert(std::pair<int, Entity*>(entity->getId(), entity));
}

From the interface you can tell that the manager takes ownership. As a result it can easily be called like this:
Airplane    airplane;
EntityManager::instance()->registerEntity(&airplane);

Now what will happen in your code. I suspect it will blow up at some point in the distant future. Try tracking that bug down.
The way to do this is to tell the manager he is supposed to take ownership. To do this your interface is supposed to take a std::unique_ptr. Also a couple of potential bugs to watch out for. Somebody may pass you a NULL in which case calling getId() is going to generate undefined behavior (by that I mean crash).
int registerEntity(std::unique_ptr<Entity>&& entity)
{
    if (!entity.get())
    {    return -1;
    }

    entityMap.insert(std::pair<int, Entity*>(entity->getId(), std::move(entity)));
    return entity->getId();
}

Notice I am moving the smart pointer into the entityMap. This shows that you are moving ownership into the map. It must also be smart enough to hold the smart pointers. So you have two options:
std::unordered_map<int, std::unique_ptr<Entity>> EntityMap;

Alternatively boost has a pointer container library, where the container takes ownership. Unfortunately they don't have unordered_map but they do have map.
boost::ptr_map<int, Entity> EntityMap;

Using the boost pointer containers has the advantage as access to the elements in the container make them look like objects (not pointers) thus using with any of the standard algorithms is trivial (Note: using algorithms on normal containers that hold pointers is a pain because of the extra de-reference involved).
Anyway either technique will remove the problem of memory leaks.
No check for NULL. Also I see no advantage of having this interface when the previous one void removeEntity(int) does exactly the same thing and does not have the disadvantage of ownership semantics.
void removeEntity(Entity* entity)
{
    removeEntity(entity->getId());
}

Returning a pointer here:
Entity* getById(int id)
{
    return entityMap.at(id);
}

Does this give you ownership of the object back?
I would think not. If you return a pointer you open yourself up to the user being confused and deleting the pointer.
Entity* plane = EntityManager::instance()->getById(12);
// WORK on place
delete plane;     // BANG

So return a reference to indicate the caller is not getting ownership back. This does open the question of how do do you handle values that would have returned NULL previously. I would handle this by having an existence test. So for ID that you are not sure about you can validate before a retrieval.
Entity& getById(int id) const
{
    return *entityMap[id];   // de-reference the smart pointer.
}
bool checkById(int id)
{
    return entityMap.find(id) != entityMap.end();
}


Return lambda expression based on parameters This function returns a lambda expression based on the parameters sent. I am sure there is a better way to do this.

    [NonAction]
    private Expression<Func<ScheduleItemDto, bool>> Where(Guid siteGuid, Guid? eventGuid, MonthOfYear month, int year, bool? closed)
    {
        Expression<Func<ScheduleItemDto, bool>> notAdminExpression = x => x.Active == true;
        Expression<Func<ScheduleItemDto, bool>> baseExpression = x => x.SiteGuid == siteGuid && x.Date.Month == (int)month + 1 && x.Date.Year == year;
        Expression<Func<ScheduleItemDto, bool>> eventExpression = x => x.EventGuid == eventGuid;
        Expression<Func<ScheduleItemDto, bool>> closedExpression = x => x.Closed != null;
        Expression<Func<ScheduleItemDto, bool>> notClosedExpression = x => x.Closed == null;
        Expression<Func<ScheduleItemDto, bool>> resultExpression = x => true;

        switch (closed)
        {
            case null:
                if (eventGuid == null)
                {
                    resultExpression = baseExpression;
                }
                else
                {
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, eventExpression.Body), baseExpression.Parameters[0]);
                }
                break;

            case true:
                if (eventGuid == null)
                {
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, closedExpression.Body), baseExpression.Parameters[0]);
                }
                else
                {
                    Expression<Func<ScheduleItemDto, bool>> exp = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, closedExpression.Body), baseExpression.Parameters[0]);
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(exp.Body, eventExpression.Body), exp.Parameters[0]);
                }
                break;

            case false:
                if (eventGuid == null)
                {
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, notClosedExpression.Body), baseExpression.Parameters[0]);
                }
                else
                {
                    Expression<Func<ScheduleItemDto, bool>> exp = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, notClosedExpression.Body), baseExpression.Parameters[0]);
                    resultExpression = Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(exp.Body, eventExpression.Body), exp.Parameters[0]);
                }
                break;
        }

        if (!AccessControl.HasAccess(Permissions.DashboardAdministrator))
        {
            return Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(resultExpression.Body, notAdminExpression.Body), resultExpression.Parameters[0]);
        }

        return resultExpression;
    }

Personally, I find your code easy enough to follow. The flow control is lengthy but is quite simple. A few notes before searching alternatives for the flow control.

Expression<Func<ScheduleItemDto, bool>> notAdminExpression = x => x.Active == true;

Could be

Expression<Func<ScheduleItemDto, bool>> notAdminExpression = x => x.Active;

Your function could be broken in two pieces, one for the switch statement and another for the permission part. It may be a matter of personal preference, but allows early return statements, removing the assignment operation and thus entirely removing any state from the function, pure functions are easier to follow.

// ...
case null:
    if (eventGuid == null)
    {
        return baseExpression;
    }
    else
    {
        return Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, eventExpression.Body), baseExpression.Parameters[0]);
    }
// ...

private Expression<Func<ScheduleItemDto, bool>> RestrictPermission(Expression<Func<ScheduleItemDto, bool>> baseExpression)
{
    Expression<Func<ScheduleItemDto, bool>> notAdminExpression = x => x.Active
    return Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, notAdminExpression.Body), baseExpression.Parameters[0]);
}
// ...
RestrictPermission(Where(...));

Now the ways I can think of rewriting the switch statement

If chain

if(closed == null && eventGuid == null)
    return ...;
else if (closed == null && eventGuid != null)
    return ...;
else if (closed && eventGuid == null)
    return ...;
...

Very easy to parse which case each return belongs, but quite verbose.

Dictionary

 var options = new Dictionary<Tuple<bool?, bool>, Expression<Func<ScheduleItemDto, bool>>> {
    { Tuple.Create(null, true), baseExpression },
    { Tuple.Create(null, false), Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, eventExpression.Body), baseExpression.Parameters[0]) },
    { Tuple.Create(true, true), Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, closedExpression.Body), baseExpression.Parameters[0]) },
    // ...
}
return options[closed, eventGuid == null];

Adds complexity and reduces readability, but is probably the most compact version.

If plus ternary

if (closed == null)
    return eventGuid == null
        ? baseExpression
        : Expression.Lambda<Func<ScheduleItemDto, bool>>(Expression.AndAlso(baseExpression.Body, closedExpression.Body), baseExpression.Parameters[0]);
else if (closed)
    ...
else
    ...

Half-way between readability and verbosity.

Honestly, I would stick to your switch or a chained-if. You can't get much elegance without making the code unnecessary complex. Or at least not with the options I know, I hope someone has a better solution.


K-combination recursive algorithm implementation Tried to implement this solution for finding n choose k.
I have used recursion and this is how it goes for k as 4 and n as 6. Zero based

The idea is to have array of size k keeping sequence of indices of elements from the input array (which are numbers from 0 to n - 1) in increasing order. On each step, algorithm looks for the closest to the end item which can be incremented, increments it and fills up items right to that item.

 def get_combinations(str_index, k, n, index, limit, increment):
     if increment < limit:
         str_index[index] = increment
         result.add(tuple(str_index[0:index]+[increment]+str_index[index+1:]))
         get_combinations(str_index[:], k, n, index, limit, increment+1)
         if index >= 0 and str_index[index-1]+1 < increment:
             get_combinations(str_index[:], k, n, index-1, increment, str_index[index-1])

 result = set()
 word = "happyco"
 k = 4
 n = len(word)
 str_index = [i for i in range(k)]
 result.add(tuple(str_index))
 get_combinations(str_index, k, n, k-1, n, k)
 for combination in result:
     print("".join([word[i] for i in combination]))

However, there are some problems with this recursive approach:

a. It will cause stack overflow if n and k is huge.

b. I am using set to fix duplicates in the result.

There're a few more problems with code: for instance, it crashes for k = 0 (which is a valid input).
You implementation is really hard to follow: it's not clear what the arguments index, limit and increment stand for. They're way too generic. index: What kind of index? Sure, it's a position in the original string or list. But what does it represent? It's not an arbitrary position, is it? It's the index of the element that should be changed, isn't it? Why not call it idx_to_change, pos_to_change or something like that? The same goes for limit: what kind of limit is it?
There's too much stuff going on in the get_combinations function (like choosing the next position, generating a new combination and so on). I suggest splitting into a few smaller function with meaningful names.
I didn't fully understand your code, but if you need to keep a set to avoid duplicated, you're doing it wrong. I don't see why you need to recursion here either.
Let's go back to the algorithm:

comb = [0..k-1]
do
    result.append(comb)
    // I assume that it returns null if there's no next combination
    comb = get_next_combination(comb)
while comb != null

Pretty simple, isn't it? Now we need to learn to generate the next combination. This part is already described in your post. So let's just create another function that does it.

Here's the code that implements this idea directly:

def get_pos_to_change(comb, n):
    """
    Finds the rightmost position in the comb list such that its value can
    can be increased. Returns -1 if there's no such position.
    """
    pos_to_change = len(comb) - 1
    max_possible_value = n - 1
    while pos_to_change >= 0 and comb[pos_to_change] == max_possible_value:
        pos_to_change -= 1
        max_possible_value -= 1
    return pos_to_change

def inc_value_at_pos(comb, pos):
    """
    Increments the value at the given position and generates the
    lexicographically smallest suffix after this position.
    """
    new_comb = comb[:]
    new_comb[pos] += 1
    for idx in range(pos + 1, len(comb)):
       new_comb[idx] = new_comb[idx - 1] + 1
    return new_comb

def get_next_combination(comb, n):
    """
    Returns the lexicographically next combination or None if it doesn't
    exist.
    """
    pos_to_change = get_pos_to_change(comb, n)
    if pos_to_change < 0:
        return None
    return inc_value_at_pos(comb, pos_to_change)

def combinations(n, k):
    """
    Generates all n choose k combinations of the n natural numbers
    """
    comb = [i for i in range(k)]
    while comb is not None:
        yield comb
        comb = get_next_combination(comb, n)

It's split into several smaller functions so that it's easier to read.

A few more technical details: I used yield here to implement a generator. It's useful if the number of combinations is huge. Not storing them in a collection helps to save a lot of memory in this case (stack overflow is not the only possible issue: you can also just run out of space to store all combinations).

Note that this implementation handles corner cases properly (like k = 0).

There're a few more things you might want to add, such as parameter validation (it makes sense to throw an exception if k > n or either of them is negative).


Console-like printing of message JavaScript I'm learning JavaScript and I've made a very simple little document-modifying script that prints a message as if it was someone typing into a console. It practices recursion and in my opinion, I feel a bit iffy about the number of if-cases in the code. My main concern is the JavaScript though I know there's still a lot I have to learn about CSS animations.

var text = document.getElementById('text');
var greeting = ['Hello. I am a console wannabe.',
                'All systems are functioning.',
                'I like pie.'];
text.innerHTML = '▮';

(function greet() {
    if (greeting.length > 0 && greeting.length < 3) {
        text.insertBefore(document.createElement('br'), text.lastChild);
    }
    var line = greeting.shift();
    if (!line) {
        return;
    }
    line = line.split('');
    (function type() {
        var character = line.shift();
        if (!character) {
            return setTimeout(greet, 2000);
        }

        text.insertBefore(document.createTextNode(character), text.lastChild);
        setTimeout(type, 300);
    }());
}());
body{
    background-color: #000000;
    color: #99ffcc;
    font-family: Courier;
}

i{
    font-style: unset;
    font-size: 1em;
    animation: blink 1100ms linear infinite;
}

@keyframes blink {
    49% {
        opacity: 1;
    }
    50% {
        opacity: 0;
    }
    89% {
        opacity: 0;
    }
    90% {
        opacity: 1;
    }
}

I would like as many opinions as you can have about my simple little script. Does it look like I have a small grasp of JavaScript?

Your code is really easy to understand, and there's not much wrong with it.

I promise I'll go easy on you.

Right on the first lines, something bothers me...

You have this:

var text = document.getElementById('text');
var greeting = ['Hello. I am a console wannabe.',
                'All systems are functioning.',
                'I like pie.'];
text.innerHTML = '▮';

What's wrong? The names!

text implies some actual text, but you have a
. greeting is alright, but it is actually text. And that  is a prompt.

I propose the following rewrite:

var screen = document.getElementById('screen');
var text = ['Hello. I am a console wannabe.',
                'All systems are functioning.',
                'I like pie.'];
screen.innerHTML = '&marker;';

It all makes sense, except for the class, which I will explain later on.
And please, use the HTML entities, to avoid problems with file encodings.
Also, don't use  for this, since it will have problems when the animation is stopped.
The best would be to move the prompt to the HTML and ignore that line.

Lets analyse the function greet().

You have the following code:

(function greet() {
    if (greeting.length > 0 && greeting.length < 3) {
        text.insertBefore(document.createElement('br'), text.lastChild);
    }
    var line = greeting.shift();
    if (!line) {
        return;
    }
    line = line.split('');
    (function type() {
        var character = line.shift();
        if (!character) {
            return setTimeout(greet, 2000);
        }

        text.insertBefore(document.createTextNode(character), text.lastChild);
        setTimeout(type, 300);
    }());
}());

Huh? The greet() function is preparing to type the text? And only works for this example? Well, let me re-write this for you.

function type(text, screen) {
    //You have to check for lines and if the screen is an element
    if(!text || !text.length || !(screen instanceof Element)) {
        return;
    }

    //if it is not a string, you will want to make it into one
    if('string' !== typeof text) {
        text = text.join('\n');
    }

    //normalize newlines, and split it to have a nice array
    text = text.replace(/\r\n?/g,'\n').split('');

    //the prompt is always the last child
    var prompt = screen.lastChild;
    prompt.className = 'typing';

    var typer = function(){
        var character = text.shift();
        screen.insertBefore(
            //newlines must be written as a `<br>`
            character === '\n'
                ? document.createElement('br')
                : document.createTextNode(character),
            prompt
        );

        //only run this again if there are letters
        if( text.length ) {
            setTimeout(typer, 300);
        } else {
            prompt.className = 'idle';
        }
    };
    setTimeout(typer, 300);
};

Wow, some fine German Overengeneering going on there!

You can pass it a string or an array of strings, and a 'screen', where the message will be added.

Now, enough of Javascript, lets talk about CSS!

It is a pretty straightforward CSS, without many complications.
But, remember that it suffered a rewrite.

Replace your i CSS with this:

#prompt{
    font-style: unset;
    font-size: 1em;
}
#prompt.idle {
    animation: blink 1100ms linear infinite;
    -webkit-animation: blink 1100ms linear infinite;
}

On your HTML, simply change the id.

Remember, we re-wrote it:

&marker;

Final result:

And now, all together:

function type(text, screen) {
 //You have to check for lines and if the screen is an element
 if(!text || !text.length || !(screen instanceof Element)) {
  return;
 }

 //if it is not a string, you will want to make it into one
 if('string' !== typeof text) {
  text = text.join('\n');
 }

 //normalize newlines, and split it to have a nice array
 text = text.replace(/\r\n?/g,'\n').split('');

 //the prompt is always the last child
 var prompt = screen.lastChild;
 prompt.className = 'typing';

 var typer = function(){
  var character = text.shift();
  screen.insertBefore(
   //newlines must be written as a `<br>`
   character === '\n'
    ? document.createElement('br')
    : document.createTextNode(character),
   prompt
  );

  //only run this again if there are letters
  if( text.length ) {
   setTimeout(typer, 300);
  } else {
   prompt.className = 'idle';
  }
 };
 setTimeout(typer, 300);
};

window.onload=function(){
 var screen = document.getElementById('screen');
 var text = [
  'Hello. I am a better console wannabe.',
  'All systems are functioning.',
  'I like pie.'
 ];

 type(text, screen);
};
body {
 background-color: #000000;
 color: #99ffcc;
 font-family: Courier;
}
#prompt {
 font-style: unset;
 font-size: 1em;
}
#prompt.idle {
 animation: blink 1100ms linear infinite;
 -webkit-animation: blink 1100ms linear infinite;
}

@keyframes blink {
 49% {opacity: 1;}
 50% {opacity: 0;}
 89% {opacity: 0;}
 90% {opacity: 1;}
}
@-webkit-keyframes blink {
 49% {opacity: 1;}
 50% {opacity: 0;}
 89% {opacity: 0;}
 90% {opacity: 1;}
}

&marker;

To ressemble a little more a console, the animation is only applyed when there's no typing.

With minor changes, it is possible to make it work with keypresses on the keyboard. And with a write queue. But that's left as an exercise for the O.P.

Also, one cool thing you can try is to use a random timeout, to create a varying effect on the writting, to simulate real typing! You can even add an extra interval to special characters, to be more human-like:

function type(text, screen) {
 //You have to check for lines and if the screen is an element
 if(!text || !text.length || !(screen instanceof Element)) {
  return;
 }

 //if it is not a string, you will want to make it into one
 if('string' !== typeof text) {
  text = text.join('\n');
 }

 //normalize newlines, and split it to have a nice array
 text = text.replace(/\r\n?/g,'\n').split('');

 //the prompt is always the last child
 var prompt = screen.lastChild;
 prompt.className = 'typing';

 var typer = function(){
  var character = text.shift();
  screen.insertBefore(
   //newlines must be written as a `<br>`
   character === '\n'
    ? document.createElement('br')
    : document.createTextNode(character),
   prompt
  );

  //only run this again if there are letters
  if( text.length ) {
   var delay, next = text[0];
   //based on a querty pt-PT keyboard, there delays are subjective
   if(next.match(/[a-z\d\t\-\.,º]/)){
    //fastest characters
    delay = 50;
   } else if(next == ' ' || next == '\n' || next.match(/[\\\|\!\"\#\$\%\&\/\(\)\=\?\'»\*ª_:;>A-Z]/)) {
    //medium-slow keys
    delay = 100;
   } else if(next.match(/[\@\€\£\§\{\[\]\}\~\´]/)) {
    //slow keys
    delay = 150;
   } else {
    //Yes, that slow!
    delay = 250;
   }
   //repeated characters are types faster
   if(next == character) {
    delay -= 25; //reduces the delay by 50
   }

   setTimeout(typer, delay + (Math.random() * 50));
  } else {
   prompt.className = 'idle';
  }
 };
 setTimeout(typer, 50 + (Math.random() * 50));
};

window.onload=function(){
 var screen = document.getElementById('screen');
 var text = [
  'Hello. I am a better console wannabe.',
  'All systems are functioning.',
  'I like pie.',
  'É só pra teste (just for testing).',
  'ASCII PARTY!"#$%&/()=?!!!!!'
 ];

 type(text, screen);
};
body {
 background-color: #000000;
 color: #99ffcc;
 font-family: Courier;
}
#prompt {
 font-style: unset;
 font-size: 1em;
}
#prompt.idle {
 animation: blink 1100ms linear infinite;
 -webkit-animation: blink 1100ms linear infinite;
}

@keyframes blink {
 49% {opacity: 1;}
 50% {opacity: 0;}
 89% {opacity: 0;}
 90% {opacity: 1;}
}
@-webkit-keyframes blink {
 49% {opacity: 1;}
 50% {opacity: 0;}
 89% {opacity: 0;}
 90% {opacity: 1;}
}

&marker;


Product matcher refactoring regarding scopes and arrays I know I have seen this before and I can't remember how to fix it or find where I saw how to fix it:

def product_matcher(item)
  product = []
  if !item.blank?
    product = self.products.where(:id => item.id).first
  else
    product = self.products.where(:id => token.id).first
  end
  product
end

It seems wrong to me to first set the variable to an array, then inside the scope of the each to set product to the result of the query and then at the end of the method have the variable return the product.  Since Ruby doesn't have explicit types my belief is that you don't even need the product = [] and since the result of a Ruby method is always to return you more than likely don't even need product at the end. However, due to scoping the output would be encapsulated inside the each block and the method would ultimately return nothing when called.  How do I improve this?

It is redundant to use self here.

def product_matcher(item)
  attribute_id = item.blank? ? token.id : item.id
  products.where(id: attribute_id).first
end


IdNumber: a string of fixed length and with character constraints In my program, I have special id numbers. They have a fixed length of 10 characters and may only contain digits and lowercase letter. I created my own data type for this. I want to be able to assign string values to it (with the implicit operator). When I do so, there should be automatically a verification if all conditions are met (the length and the constraints at the characters).

public class IdNumber
{
    private string idnumber;

    public static implicit operator IdNumber(string value)
    {
        if (value.Length == 10 && value.All(c => IsDigit(c) || IsLowercaseLetter(c)))
        {
            IdNumber m = new IdNumber();
            m.idnumber = value;
            return m;
        }
        else
        {
            throw new ArgumentException("id numer must have length 10 and must only have digits and lower case-letters");
        }
    }

    public override string ToString()
    {
        return idnumber;
    }

    public override int GetHashCode()
    {
        return idnumber.GetHashCode();
    }

    public override bool Equals(object obj)
    {
        if(obj is IdNumber other)
        {
            return this.idnumber == other.idnumber;
        }
        else
        {
            return false;
        }
    }

    public static bool operator ==(IdNumber m1, IdNumber m2)
    {
        if(m1 is null)
        {
            return m2 is null;
        }
        return m1.Equals(m2);
    }

    public static bool operator !=(IdNumber m1, IdNumber m2)
    {
        return !(m1 == m2);
    }

    private static bool IsDigit(char c)
    {
        return c >= '0' && c <= '9';
    }

    private static bool IsLowercaseLetter(char c)
    {
        return c >= 'a' && c <= 'z';
    }
}

Now I can use it via

IdNumber myId = "ab45jk23zz";
IdNumber otherId = "asfd" // throws exception

I have never used the implicit operator and I am also not sure about overriding the GetHashCode method (I want to use them as a key in a Dictionary, so it is important this works correctly). Hence I want to be sure I did not overlook anything. Of course, any other suggestions for improvement are welcome.

I will review your code from top to bottom.

private string idnumber;

this should be named using camelCase casing and because you don't change its value you should make it readonly which means you can only assign a value to it inside the constructor or by initialising like e.g private string idnumber = "somevalue";. Because you set this value only inside the implicit operator you should add a private constructor which has a string as argument. This would then look like this

public class IdNumber
{
    private readonly string idNumber;
    private IdNumber(string idNumber)
    {
        this.idNumber = idNumber;
    }

Because the implicit operator is public you should validate the argument value wether it is null. It just looks better to throw an ArgumentNullException than to receive an NullReferenceException.

Instead of using value.All(c => IsDigit(c) || IsLowercaseLetter(c)) you could use a simple Regex to do the validation for you. This will shorten your code because the two methods IsDigit() and IsLowercaseLetter() won't be needed anymore.

The implicit operator would then look like this

public static implicit operator IdNumber(string value)
{
    if (value is null) { throw new ArgumentNullException(nameof(value)); }

    if (Regex.IsMatch(value, "[0-9a-z]{10}"))
    {
        return new IdNumber(value);
    }
    throw new ArgumentException("id numer must have length 10 and must only have digits and lower case-letters");
}

ToString() and GetHashCode() are both fine but in the Equals() method you can either just remove the else like so

public override bool Equals(object obj)
{
    if(obj is IdNumber other)
    {
        return this.idNumber == other.idNumber;
    }

    return false;
}

or combine the if condition with the idNumber comparision like so

public override bool Equals(object obj)
{
    return (obj is IdNumber other)
        && this.idNumber == other.idNumber;
}


Simple C# console application to read from csv and query DB I wrote a simple console application that reads contents from a CSV file, and using that input, checks a database table to see if the contents are within it. If there is a match, then the program will output the name of the file that the input is included in to a text file.

I feel like I should break this application up into more compartmentalized functions, such as separating the DB connection from DB read.

Any input for this greatly appreciated!

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Data.SqlClient;
using System.Windows.Forms;
using System.IO;
using Microsoft.VisualBasic.FileIO;

namespace MLMSpellingReader
{
    class SpellingReader
    {
        static void Main(string[] args)
        {
            SpellingReader.readCSV();
            MessageBox.Show("Evaluation is complete, press OK to continue");
        }

        public static void readCSV(){

            string filePath = Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments);
            string outputFile = filePath + @"\outputFile.txt";
            string queryString = null;
            using (TextFieldParser parser = new TextFieldParser(@"c:\users\myuser\Documents\spellcheck.csv"))
            {
                parser.TextFieldType = FieldType.Delimited;
                parser.SetDelimiters(",");
                while(!parser.EndOfData)
                {
                    string[] fields = parser.ReadFields();
                    foreach(string field in fields)
                    {
                        string eval = field;
                        queryString = "select Name from myTable where Logic like '%" + eval + "%' and active = '1'";
                        SpellingReader.connectToDB(outputFile, queryString, eval);
                    }
                }
            }
        }

        public static void connectToDB(string outputFile, string queryString, string eval)
        {
            string connectionString = "Server=myServer;Database=myDBTable;Integrated Security=True;";
            string checkSpellingQuery = queryString;
            SqlConnection conn;
            conn = new SqlConnection(connectionString);
            try
            {
                SqlCommand command = new SqlCommand(checkSpellingQuery, conn);
                conn.Open();
                SqlDataReader reader = command.ExecuteReader();
                using(StreamWriter file = new StreamWriter(outputFile, true))
                {
                    while(reader.Read())
                    {
                        file.WriteLine("The misspelling " + eval + " was found in: " + reader["Name"]);
                    }
                }
                reader.Close();
                conn.Close();
            }
            catch(Exception ex)
            {
                MessageBox.Show("Cannot open connection: " + ex.ToString());
            }
        }

    }
}

readCSV

This function has no reason to be public, make it private. Also its name is misleading, it's not just reading CSV file but also connecting to DB and saving output...

string outputFile = filePath + @"\outputFile.txt";

Do not blindly concatenate paths, use Path.Combine(). Also filename should be moved to configuration (or if immutable to a const member).

string queryString = null;

You do not need to declare a variable at the top of your method, it's usually better to declare as late as possible (best if together with its initialization).

using (TextFieldParser parser = new TextFieldParser(@"c:\users\myuser\Documents\spellcheck.csv"))

Do not hardcode paths. Never. Read it from configuration, command line or asks for them. Here you do not even use Environment.GetFolderPath()!

string eval = field;

It serves no purpose, just drop it.

queryString = "select Name from myTable where Logic like '%" + eval + "%' and active = '1'";

Do not ever ever build SQL commands like that. Use parameters, code may be broken because of deliberate SQL injection attacks or because of unescaped input data.

SpellingReader.connectToDB(outputFile, queryString, eval);

You're connecting to DB multiple times, it's probably a waste: you'd better keep connection open until you processed all input lines.

connectToDB

public static void connectToDB(string outputFile, string queryString, string eval)

This function has no reason to be public, make it private. Also here the name is misleading, you're not just connecting to DB but also querying data and writing output...

eval parameter has not a truly meaningful name, you'd better pick something more close to your domain.

SqlConnection conn;
conn = new SqlConnection(connectionString);

There is no reason to split this code in two lines, moreover you do not dispose connection in case of errors (use using).

SqlDataReader reader = command.ExecuteReader();

You do not dispose this object in case of errors.

using(StreamWriter file...

Some LINQ may make your code shorter and more readable (if you do not have a huge number of lines to write).

Minor issues

In C# usually functions are PascalCase, also you should be consistent with spacing (if ( or while(?)

What to do?

Let's first make clear what your application is doing:

Read all words from an input file (each word is separated with comma and each line contains multiple words).
For each word check if there is a record in the database where Logic column contains given text.
Print Name column of those records.

Let's write some code (let me assume the number of input words is reasonable - it fits in memory):

static IEnumerable<string> ReadAllWords(string path)
{
    var words = new List<string>();
    using (var parser = new TextFieldParser(path))
    {
        parser.TextFieldType = FieldType.Delimited;
        parser.SetDelimiters(",");

        while (!parser.EndOfData)
            words.AddRange(parser.ReadFields());
    }
}

Now let's query DB:

static IEnumerable<string> FindRecordsByWord(SqlConnection connection, string word)
{
    using (var command = connection.CreateCommand())
    {
        command.CommandText = "SELECT Name FROM myTable WHERE Logic LIKE @word AND active = '1'";
        command.Parameters.AddWithValue("@word","%" + word+ "%");

        using (var reader = command.ExecuteReader())
            yield return reader["Name"];
    }
}

Let's put some pieces together:

var mispelledWords = ReadAllWords(InputFile); // From configuration or cmdline
using (var conn = new SqlConnection("MyConnectionString")) // From configuration!!!
{
    var recordsWithMispelledNames = mispelledWords
        .SelectMany(w => FindRecordsByWord(conn, w).Select(n => $"Found {w} in {n}"));

    File.WriteAllLines(OutputFile, recordsWithMispelledNames);
}

Note that you may now refactor out some simple logic:

static IEnumerable<string> FindRecordsByWordList(IEnumerable<string> words)
{
    using (var conn = new SqlConnection("MyConnectionString")) // From configuration!!!
    {
        return mispelledWords
            .SelectMany(w => FindRecordsByWord(conn, w).Select(n => $"Found {w} in {n}"));
    }
}

Your Main() function will then be as simple as:

File.WriteAllLines(OutputFile,
    FindRecordsByWordList(ReadAllWords(InputFile)));

Few more notes:

There is space for more abstractions (input type, output type) but I'd keep code as simple as possible until you actually need it.
You should specify input/output file encoding (don't really rely on default settings and magic guesses, you'll be surprised).
There are debates around AddWithValue() usage, decide if you like it or not.
There is not any error checking (see also Know when to retry or fail when calling SQL Server from C#?)


SQL Query to perform a "reverse exclusion" on a LEFT JOIN with a select from the left side table I have a set of tables that I want to execute a LEFT JOIN on and bring back "excluded" rows.  In addition, I would like the left table's ID included as part of the results set.  Here's some sample data:

Table1_ID

1

3

5

Table2_ID
Table2_Val

1
a

2
b

3
c

4
d

5
c

A simple left join would look like this
SELECT t1.Table1_ID, t2.Table2_Val
FROM Table2 t2
LEFT JOIN Table1 t1 ON t2.Table2_ID = t1.Table1_ID
WHERE t1.Table1_ID IS NULL

And yields the following table

Table1_ID
Table2_Val

NULL
b

NULL
d

However, my desired result is to have for each ID in Table2 that is missing in Table1, return the set of values in Table2 paired with the existing values in table 1.  Like so:

Table1_ID
Table2_Val

1
b

1
d

3
b

3
d

5
b

5
d

So far, the best I've been able to come up with is through using a CROSS JOIN to accomplish this, but I'd like to know if there's a better/more efficient way to handle this.  Here's the code I'm curious about improving:
WITH cte (T1_ID, T2_ID, T2_Value)
AS
(
SELECT t1.Table1_ID, t2.Table2_ID, t2.Table2_Val
FROM Table1 t1 CROSS JOIN Table2 t2
GROUP BY t1.Table1_ID, t2.Table2_ID, t2.Table2_Val
)
SELECT
  cte.Table1_ID,
  t2.Table2_Val
FROM Table2 t2
INNER JOIN cte ON t2.Table2_ID = cte.T2_ID
LEFT JOIN Table1 t1 ON t2.Table2_ID = t1.Table1_ID
WHERE t1.Table1_ID IS NULL

You already have the table2_vals you need.
Just cross join them with table1. No need for long nested stuff and group by:
select t1x.Table1_Id,q.Table2_Val
from
    Table1 t1x
    cross join
    (
        SELECT t2.Table2_Val
        FROM Table2 t2
        LEFT JOIN Table1 t1 ON t2.Table2_ID = t1.Table1_ID
        WHERE t1.Table1_ID IS NULL
    )q


Resize iframe & textarea in reverse directions I have a textarea and iframe whose sizes should change dynamically and in reverse directions. I'd like to achive this effect without using a frameset or jQuery plugin. Here's the result of my attempt:

<!DOCTYPE html>
<html>

<head>
    <meta charset="UTF-8">
    <title>Resize</title>
    <style>
        textarea,
        iframe {
            display: block;
            width: 300px;
            height: 200px;
            margin: 0;
            border: 0;
            padding: 0;
        }
        textarea {
            background: green;
            resize: none;
        }
        iframe {
            background: blue;
        }
    </style>
</head>

<body>
    <input id="slide" oninput="resize();" onchange="resize();" type="range" min="0" max="400" value="200">
    <textarea id="textarea"></textarea>
    <iframe id="iframe"></iframe>
    <script>
        var slide = document.getElementById("slide");
        var textarea = document.getElementById("textarea");
        var iframe = document.getElementById("iframe");

        function resize() {
            var slideValue = slide.value;
            textarea.style.height = slideValue + "px";
            iframe.style.height = 400 - slideValue + "px";
        }
    </script>
</body>

</html>

Demo

I wonder if you could review my code and let me know what you think.

Perhaps add overflow:hidden; to the CSS of the textarea and iframe, and add scrolling="no" as an attribute of the HTML iframe element: because otherwise, a scrollbar will appear when the slider is very big or very small.

Maybe a scrollbar is what you want (for usability); but a scrollbar makes a small visual artifact in an empty textbox or iframe, when the element is shorter than one line.


Checking for a palindrome in any sub-range of a range This is my attempt at answering "Palindrome from all the substrings" which asks to detect if a string contains a palindrome as a substring.

The algorithm implemented here is the one described in its accepted answer.

First, here is the main function (based on the corresponding main in the initial question):

int main()
{
    size_t n_str;
    std::cin >> n_str;

    for (size_t i = 0; i < n_str; ++i)
    {
        std::string str;
        std::cin >> str;

        std::cout << (contains_palindrome(str) ? "YES" : "NO") << std::endl;
    }

    return EXIT_SUCCESS;
}

Second, I decided to extend contains_palindrome from exclusively accepting an std::string to taking any container (e.g. std::vector<char>, std::array<int,N>, ...) as an argument:

template <class T>
bool contains_palindrome(const T& s)
{
    if (s.size() < 2) return false;

    for (auto it = s.cbegin(),
            next = std::next(it),
           nnext = std::next(next);
         nnext != s.cend();
         it = next, next = nnext, std::advance(nnext, 1))
        if (*it == *next || *it == *nnext)
            return true;

    return *std::prev(s.cend()) == *std::prev(s.cend(), 2);
}

NB: <cstdlib>, <iostream>, <iterator> and <string> are required.

QUESTION(S):

Is there any instance of bad practice in the code? In particular,

is the template correct according to the desired behaviour? Am I
right in assuming that contains_palindrome will work as expected on
any class implementing the methods size(), cbegin() and cend()?
is the use of iterators done correctly? In particular, I made sure to
call the "c versions" of begin() and end() (after declaring s
as const) and used std::prev(it), std::next(it) and
std::advance(it,1) instead of, respectively, it-1, it+1 and
it++: is this, as I was told, good practice? Are there other
instances of good practice related to iterators that I overlooked?
I used iterators because I learned that it is "the [right]
way to do it in C++" but what do I gain over the following C-like
approach?

for (size_t i = 0; i < s.size()-2; ++i)
    if (s[i] == s[i+1] || s[i] == s[i+2])
        return true;

return s[s.size()-1] == s[s.size()-2];

Well, especially with templates, the devil is in the details.

Yes, the template has the desired behavior, if it works. Your code expects a readable bidirectional range knowing its size which is not a raw array.

You really don't need to ask for the ranges size, compare iterators instead.

Asking for the previous position is inexcusable if you don't need it.
Instead of checking curr vs curr+1 and curr vs curr+2 in the loop and finally end-1 vs end-2 after, check begin vs begin+1 before the loop and curr vs curr+2 and curr+1 vs curr+2 in the loop

Until the Ranges TS is incorporated into the core standard, making a range from an iterator-pair is uncomfortable, while getting a start- and end-iterator from a range is easy. So, go with the common interface using two iterators.

Also consider adding a specialization for single-pass-ranges. Allow requesting it manually, as it should be more efficient if dereferencing an iterator is expensive.

See the answer to question 2 about .cbegin() and co.
Calling the "c versions" of the member-functions restricts your codes generality.

They aren't part of the core range-interface, being just convenience-functions for non-generic code, so not all types have them. See std::initializer_list<T> for a common one.

Even though raw arrays are perfectly fine, they lack member-functions.

In summary, always use the free functions, especially in generic code.
Using iterators instead of the indexing-operator allows you to efficiently use a type without cheap random-access, like lists, files, and so on.

The aim is to require the minimum interface you can get away with without compromising efficiency, so your algorithm is as generally useable as possible.

Some algorithms will take advantage of additional capabilities to provide more efficiency or other additional guarantees.


Script to get available IP automatically Recently I made a script to get available IP automatically. I was woundering if anyone can give me tips on how to make this code look better, preferably in a class and OOP. I'm gonna show the code to my boss, but i want it to look clean and nice before i do so, and hopefully learn a thing or two about writing better code.

Code:

import requests
from orionsdk import SwisClient
import getpass

# Brukerinfromasjon
npm_server = 'mw-solarwinds.yy.dd'
username = 'jnk'
password = getpass.getpass()

server_navn = input('Skriv inn DNS navn: ')
dns_ip = '10.96.17.4'  # 10.96.17.5 = Felles
dns_sone = 'yy.dd'

verify = False

if not verify:
    from requests.packages.urllib3.exceptions import InsecureRequestWarning

    requests.packages.urllib3.disable_warnings(InsecureRequestWarning)

swis = SwisClient(npm_server, username, password)  # Kjører mot IPAM med brukerinformasjon

subnets = {
    'ka-windows': ['10.112.12.0', '24'],
    'ka-linux': ['10.112.10.0', '24'],
    'ka-exa-mgmt': ['10.112.26.0', '28']
}

print("Tilgjengelige subnets: ")

for i in subnets:
    print(i)

print("--------------")

found = False

while not found:
    inp = input("Skriv in Subnet: ")
    if inp in subnets:
        '''
        Finner ledig IP adresse i subnet
        '''

        sub_ip = subnets[inp][0]
        sub_cdir = subnets[inp][1]
        ipaddr = swis.invoke('IPAM.SubnetManagement', 'GetFirstAvailableIp', sub_ip, sub_cdir)
        '''
        Sette DNS > IP
        '''
        dns = swis.invoke('IPAM.IPAddressManagement', 'AddDnsARecord', server_navn, ipaddr, dns_ip, dns_sone)
        print("IP: {} > DNS: {}".format(ipaddr, server_navn))
        found = True
    else:
        print("Det er ikke et subnet, velg en fra listen.")

Not sure if it's any better as a class...

#!/usr/bin/env python

import getpass
import requests

from orionsdk import SwisClient
from requests.packages.urllib3.exceptions import InsecureRequestWarning

class Subnet_Explorer(dict):
    def __init__(self, npm_server, auth, dns, verify, **kwargs):
        super(Subnet_Explorer, self).__init__(**kwargs)
        self.update(
            npm_server = npm_server,
            auth = auth,
            dns = dns,
            server_navn = input('Skriv inn DNS navn: '),
            swis = SwisClient(npm_server, auth['username'], auth['password'])
        )

        if verify == False:
            requests.packages.urllib3.disable_warnings(InsecureRequestWarning)

    def directed_exploration(self, subnets):
        """
        Yields tuple of IP and DNS addresses from select subnets
        """
        unexplored_subnets = subnets.keys()
        while True:
            print("Unexplored Subnets: {unexplored_subnets}".format(
                unexplored_subnets = unexplored_subnets))
            inp = input("Skriv in Subnet: ")

            if not unexplored_subnets or inp not in unexplored_subnets:
                print("Det er ikke et subnet, velg en fra listen.")
                break

            unexplored_subnets.remove(inp)
            ipaddr = self['swis'].invoke('IPAM.SubnetManagement',
                                      'GetFirstAvailableIp',
                                      subnets[inp][0],
                                      subnets[inp][1])
            dns = self['swis'].invoke('IPAM.IPAddressManagement',
                                   'AddDnsARecord',
                                   self['server_navn'],
                                   ipaddr,
                                   self['dns']['ip'],
                                   self['dns']['sone'])

            yield ipaddr, self['server_navn']

if __name__ == '__main__':
    """
    Running as a script within this block, eg. someone ran;

        python script_name.py --args

    to get here, usually.
    """

    auth = {
        'username': 'jnk',
        'password': getpass.getpass(),
    }

    dns = {
        'ip': '10.96.17.4',
        'sone': 'yy.dd',
    }

    subnet_explorer = Subnet_Explorer(
        npm_server = 'mw-solarwinds.yy.dd',
        auth = auth,
        dns = dns,
        verify = False)

    exploration = subnet_explorer.directed_exploration(
        subnets = {
            'ka-windows': ['10.112.12.0', '24'],
            'ka-linux': ['10.112.10.0', '24'],
            'ka-exa-mgmt': ['10.112.26.0', '28']
        })

    print("--------------")
    for ipaddr, server_navn in exploration:
        print("IP: {ipaddr} > DNS: {server_navn}".format(
            ipaddr = ipaddr,
            server_navn = server_navn))
    print("--------------")

... though perhaps ya see some things ya like. Regardless ya may want to consider is adding some argparse stuff after the if __name__ == '__main__': line. And probably best to use anything from above modifications with care as I may have made things a bit more messy by turning towards classes.

Is there a reason for setting dns = swis.invoke(...) when it's not being used for anything?


Rails UserHelper method I need help cleaning up this monstrosity. Basically this is a user helper method that generates an array of cumulative user sign ups per week (start of week == Wednesday) starting 2016-06-15.

So something like this:

[ 1, 5, 9, 22, 34 ]

Where sign ups each week were 1, 4, 4, 13, 12.

In production, this is taking 5 seconds to run (and only 550 users over 7 weeks are present).

  def get_user_array
   @total_date_array = Array.new

    # Remember that the range will go as far as the last Sunday, see next step
    @date_array  = (Date.parse("2016-06-15")..Date.parse(Time.now.strftime("%F"))).select(&:wednesday?)
      @date_array.each do |x|
      start  = x - 7
      finish = x
      @range = start...finish
      @local_user_array = Array.new
      User.all.reject{|u| !u.valid? }.map {|u| @local_user_array << "Yes" if @range === Date.parse(u.created_at.strftime("%F"))}
      @total_date_array <<  @local_user_array
    end

   # Now get the current weeks users
    @current_week_start = @date_array.last
    @current_week_end = Date.parse(Time.now.strftime("%F"))
    @current_week_range = @current_week_start..@current_week_end
    @current_local_user_array = Array.new
    User.all.reject{|u| !u.valid? }.each do |u|
       @current_week_range === Date.parse(u.created_at.strftime("%F")) ? @current_local_user_array << "Yes" : false
    end

    @total_date_array <<  @current_local_user_array

   ## Now create the array....Phew!
   sum = 0
   @total_date_array = @total_date_array.map { |a| Array(a).count("Yes") }.map{ |x| sum  += x }

end

I am fairly inexperienced. I guessing that switching a lot of the code to SQL rather than keeping it in memory is one way to begin.

Ok, so there's a lot to cover here.

get_user_array is a terrible name. What does "getting a user array" mean? Is it an array of users? A user's array? Be descriptive.
Why all the instance variables (@...)? This method is a simple helper that has no need to persist state between invocations. Don't use instance variables for this!
You appear to be laboring under a lot of misunderstandings of, well, fairly basic things to be frank.

Don't use #map unless your intention is to transform an array 1-to-1 into a new array. You're using #map a lot, but only once for an actual mapping operation.
You're doing a lot of pointless conversion back and forth. For instance, Array(a).count when a is already an array.
You also do stuff like Date.parse(u.created_at.strftime("%F")) which means you're taking a Date, making it into a String, and then back to a Date again. I'm guessing you may be doing that on purpose to drop the time-part, but doesn't it strike you as an incredibly complicated way of doing that? Why involve strings when going from Date to Date? This sort of thing should cause you to reexamine your approach, because it just doesn't smell right.
User.all.reject{|u| !u.valid? } for one, what you want is #select - not #reject. Right now you're saying "do not give me users that are not valid"; with #select, you'd be saying "give me valid users". I.e. User.all.select(&:valid?).
However, you should really be doing neither! Selecting a subset of records based on given criteria is what databases are for. Your User model should ideally have a scope that'd allow you to just get valid users in one go, e.g. User.valid.all.
However, you don't really want that either, since you're still pulling every single user for every single week that's passed since mid-June 2016. That's crazy. If you have ~500 user records and it's now been ~7 weeks since your zero-day, you're instantiating 3,500 records only to discard the vast majority with a #reject call! As time passes and user base grows, this gets worse in a hurry. Yes, Rails is smart enough to cache some stuff, but jeez! Doesn't that strike you as a fundamentally bad approach?

Luckily, you can do more stuff in the database. Most, in fact (probably all of it, with more SQL, but I'll leave that for someone better qualified to figure out).

Supposing your User model has a valid scope that restricts queries to only valid records (thus avoid the horrendous #reject stuff), you could do:

START_DATE = "2016-06-15".freeze

# ...

per_week = User
  .valid
  .where("created_at >= ?", START_DATE)
  .order("created_at")
  .group("FLOOR(DATEDIFF(created_at, '#{START_DATE}') / 7)")
  .count

Note: I couldn't get proper query interpolation to work in the group call, so it's just plain string interpolation. Only use this when you 100% control the value to be inserted, otherwise you'll leave yourself open to injection attacks.

This will produce a SQL query that

Calculates the number of weeks between the created_at timestamp, and the start date
Groups rows sharing the same week
Counts the number of rows in each group

The result you get back is something like:

{
  0 => 1,
  1 => 4,
  2 => 4,
  3 => 13,
  4 => 12
}

To then do the cumulative summation you can do:

sum = 0
cumulative_per_week = per_week.map(&:last).map { |_, count| sum += count }

Now, cumulative_per_week will be [1, 5, 9, 22, 34].

I'd also recommend splitting logic into two methods: One for sign-ups per week, and one for cumulative sign-ups per week:

START_DATE = "2016-06-15".freeze

def signups_per_week
  User
    .valid
    .where("created_at >= ?", START_DATE)
    .order("created_at")
    .group("FLOOR(DATEDIFF(created_at, '#{START_DATE}') / 7)")
    .count
    .values
end

def signups_per_week_cumulative
  sum = 0
  signups_per_week.map { |count| sum += count }
end