How can I use vector for entering numbers? I want to enter names, student numbers and student scores and display this information. I used arrays in this way:

#include "stdafx.h"
#include <iostream>
#include <string>
#define size 3
using namespace std;
class student{
public:
    void vrod();
    void dis();
    int stno,score,i,n;
};
void student::vrod(){
cout<<"name=";
cin>>name;
cout<<"stno=";
cin>>stno;
cout<<"score=";
cin>>score;
}
void student::dis(){
    cout<<"name="<<name<<"\n"<<"stno="<<stno<<"\n"<<"score="<<score<<"\n";

}

int main(){
int i, j=0,n,;
string h;

student st[size];
while (j<3){
st[j].vrod();

j++;}
j=0;
while (j<3){
st[j].dis();

j++;}
cin.get();
cin.get();
}

According to recommendation of my friends, I used vector in this way:

#include "stdafx.h"
#include<iostream>
#include<vector>
#include<sstream>
#include<string>
#define n 3
using namespace std;

int main()
{
  int input;
  string names;
  vector<int> stno,score;
  vector<string> name;
  cout<<"Enter the number of inputs: ";

  cout << "Enter your numbers to be evaluated: " << endl;
  for(int i=0;i<n;i++){
      cout<<"student No.=";
  cin >> input;
  stno.push_back(input);
  }

 for(int i=0;i<n;i++){
      cout<<"scor=";
  cin >> input;
  score.push_back(input);
  }

 for(int i=0;i<n;i++){
     cout<<"name=";
  cin >> names;
  name.push_back(names);
  }

   for(int i=0;i<stno.size();i++)
       cout<<stno[i];
    for(int i=0;i<score.size();i++)
       cout<<score[i];
    for(int i=0;i<name.size();i++)
       cout<<name[i];

  cin.get();
  cin.get();
}

Do you have any opinion to improve the second code written via vector? Can I save information from one student including student numbers and student scores in one position in vector like arrays? I mean for example if my n is 3, I enter the information of students in three positions like arrays(student st[size];)

Formatting

This is probably the worst formatted piece of code I have seen in a while. It does not matter to the compiler but you are writing in a high level language so that other humans can read it.

Indentation
White Space
one line per declaration
Use meaningful variable and function names (don't shorten them).

Examples:

void student::vrod(){
cout<<"name=";
cin>>name;
cout<<"stno=";
cin>>stno;
cout<<"score=";
cin>>score;
}

// this should be (more like)

void student::vrod()
{
    std::cout << "name=";
    std::cin >> name;

    std::cout << "stno=";
    std::cin >> stno;

    std::cout << "score=";
    std::cin >> score;
}

Declarations. The types of objets are the most important thing in C++. So give each object a name and a type and use a single line for each.

    int stno,score,i,n;

    // That's just lazy;

    int stno;   // Why not studentNumber
    int score;
    int i;
    int n;

Stop Using "using"

using namespace std;

See: Why is “using namespace std” in C++ considered bad practice?

For anything longer than a 10 line program it becomes a curse and is likely to get you into trouble (in a way that is hard to find). So avoid doing it. Also avoid using it in your 10 line programs as that can be habit forming and will make you use it accidently).

Nameing conventions.

It is common convention for user defined types to have an initial capitol letter. This is to make types stand out from objects.

MyThing();  // This is a temporary object that is created.
myThink();  // This is a function call
            //
            // Notice the initial capitol letter allows you
            // to distinguish the two.

Also member variables should be nouns (the names of things, descriptive). While methods should be verbs (actions that are applied to your object). Using longer names will not make the code larger but it will make the code more readable.

class Student
{
    public:
        // Method names are actions that can be applied to
        // the object (avoid names like get/set it breaks encapsulation

        void getInfo(std::istream& str = std::cin);
        void display(std::ostream& str = std::cout);
    private:
        // Member variables should always be private.
        // Any modification of the state of the object
        // should be controlled by a method of the class
        // so that state can be updated consistently.

        // Use meaningful names.
        // Note: These are inside the student class.
        //       So name  => is a student name
        //          score => is a student score

        std::string   name;   // You seem to have forgotten this member.
        std::size_t   number;
        int           score;
        // These seem redundant so I am removing them.
        // int           i;
        // int           n;
};

Input and Output operator.

Your functions. vrod() and dis() work fine. But they only accept input/ouput from the standard streams. It would be nice if they could accept input/output from any stream (like a string or a file or a network socket).

You should pass the stream they are going to use as a parameter. You can default it to the stanadrd stream so that if it is not specified then it uses std::cin/std::cout.

class student{
public:
    void vrod(std::istream& in = std::cin);
    void dis(std::ostream& out = std::cout);
};

Then normal input and output is done via operator<< and operator>>. Writting these for your class becomes very simple.

std::ostream& operator<<(std::ostream& s, student const& data)
{
    data.dis(s);
    return s;
}
std::istream& operator>>(std::istream& s, student& data)
{
    data.vrod(s);
    return s;
}

Now you can print your object just like other objects.

std::cin << st[2];

Overall

The first prgram is better than the second program because of the encapsulation of student. BUT arrays are not very flexable and you should be using std::vector<student> st; ratheer than student st[size];. The ability to resize, lazily construct objects, always know the its size are three definite advantages that vector has over an array.


Javascript: Mapping combinations of input and possible suffix arrays leads to repetitive if statements I am writing a program to combine the basic form of a word with suffix arrays following the choices from a form group (Angular 9) to receive the declination.

This is the flow:

The user inputs a word.
The user chooses the predefined grammar categories of the word (genus, class, animacy and others).
According to the choices of the user the input is mapped with an array of the corresponding suffixes.

Now, my problem is that I can not wrap my head around the problem to find a more dynamic solution than writing an if-statement for every possible combination of choices and the corresponding array of suffixes.

Right now I compare the values array from the user choices with every predefined combination array.
You will see in a second that this solution leads to a lot of repetitive code and I ask you to help me to find a better solution.

Two (of many possible) predefined conditions of choices plus the fitting suffix arrays

conditionOne = ['inanimate', 'm', '2', 'hard'];
conditionOneSuffixes = ['', 'а', 'у', 'а', 'ом', 'е', 'ы', 'ов', 'ам', 'ов', 'ами', 'ах'];
conditionTwo = ['animate', 'm', '2', 'hard'];
conditionTwoSuffixes = ['', 'а', 'у', 'а', 'ом', 'е', 'ы', 'ов', 'ам', 'ов', 'ами', 'ах'];

My first function to save the choices as values from angular reactive forms

setValues($event: string) {
    this.values = [];
    this.values = Object.values(this.substantiveFormGroup.value); // the user choices from the form group saved to an array
    this.setDeclination(this.values);
  }

My second function to map the fitting suffix array on the input (only two possible if statements)

setDeclination(values) {
    if (
      this.conditionOne.every(i => values // the predefinded array of choices
        .includes(i))) {
      this.declination = this.conditionOneSuffixes.map( // the corresponding suffix array
        suffixes => this.substantiveFormGroup.get("input").value + suffixes
      );
    }
    else if (
      this.conditionTwo.every(i => values // another predefinded array of choices
        .includes(i))) {
      this.declination = this.conditionTwoSuffixes.map( // the corresponding suffix array
        suffixes => this.substantiveFormGroup.get("input").value + suffixes
      );
    }
// else if (another condition) { and so on...}
    else this.declination = [];
  }

What would be a better, less repetitive solution?

Thank you for your time.

this.values = [] will never be used.  It is unnecessary.

setValues($event: string) {
    //this.values = []; //unnecessary
    this.values = Object.values(this.substantiveFormGroup.value); // the user choices from the form group saved to an array
    this.setDeclination(this.values);
  }

The code's logic will always set a value for this.declination, so it makes sense to just assign it at the top, so you know that this.declination will always be overwritten and by default should be initialized to an empty array.  It also allows the code to be shorter.
If there will be more than two conditions, you should probably have them in a list anyways.  This way of organizing still benefits, and the use of an array list and destructuring with for of loop expresses the code's use of conditions to check for and suffixes to use in an organized list in one place, as well as shortening the code significantly and making it more concise.
The result is that it can be more simply read as an iteration on a list of conditions to process.
Brevity and DRY naturally occurs when it is distilled this way.

setDeclination(values) {
    this.declination = [];
    for (const [condition, suffixes] of [
     [this.conditionOne, this.conditionOneSuffixes],
     [this.conditionTwo, this.conditionTwoSuffixes]
    ])
       if(condition.every(i => values.includes(i))) {
         const inputValue = this.substantiveFormGroup.get("input").value;
         this.declination = suffixes.map(suffix => inputValue + suffix));
         break;
       }
 }

I have made some modifications to the formatting and the declaration of variables to control line length and make the code more concise, and it should be mostly self-explanatory.


JavaScript implementation of insertion sort I just started reading Introduction to Algorithms and the first algorithm introduced is the insertion sort.

Even though the sort seems to work, I thought I'd ask for feedback right at the start since this is a really new topic to me.

This is the JS implementation I came up with based on the pseudo code:

function insertionSort(arr) {
  for (let j = 1; j < arr.length; j++) {
    let key = arr[j]
    let i = j - 1
    while (i >= 0 && key < arr[i]) {
      i--
    }
    arr.splice(i + 1, 0, key)
    arr.splice(j + 1, 1)
    console.log(arr)
  }
}

const arr = [0, 12, 55, 0, 33333, 5, 1, 7, 2, 3, 3]

insertionSort(arr)

I just started reading Introduction to Algorithms

Do you mean the popular textbook from MIT Press? In a course taught using this book, it would likely not be acceptable to use other library functions like Array.prototype.splice. Here it glosses over a significant part of insertion sort, which is making space for the next value in the already sorted half of the array. It hides what might be an essential factor in runtime analysis.

In the book pseudocode, it shifts the numbers in the while loop using the assignment A[i + 1] = A[i]. That is more standard array usage and should work just fine in JS.

Pragmatically, using JS arrays for analysis of algorithms is problematic because they can quietly increase in size more like an ArrayList in Java than a regular array. The usage of splice in this code causes the array to change size from n to n+1, then back to n, which is not necessarily trivial at runtime.

  I thought I'd ask for feedback right at the start since this is a really new topic to me.

The courses based on this book are often not very heavy on coding, focusing more on reasoning and proofs. As I recall, the book exercises do not feature programming. For hands-on experience implementing algorithms, there may be better books.

In the CS program that I attended, there was an earlier class that used a book like Data Structures and Algorithms in Java by Goodrich and Tamassia which focused heavily on implementation.


Simple way to decrement number within a repeating range I currently have a solution to increment numbers within a range:

i % x + 1

Where x is the maximum number in the range and i is the previous number.

It is nice and simple, and gives this array: 1,2,3,4,1,2...

I also need to do this in reverse, which should produce this array: 1,4,3,2,1,4,3...

This is my solution:

i == 1 ? x : (i - 1 % x)

Is there a way to do it without the ternary operator?

Some thoughts:

(i + x) % x == i //For i >= 0 && i < x

Then

(i + x - 1) % x

Gives us the range from 0 to x-1.
We want the range from 1 to x so we change this to:

(i + x - 2) % x + 1;

But I don't think it's particularly readable, so your ternary code might end up being better.


Replace HTML input file element with custom styled button I've been working on a solution to replace the HTML input file element with a custom styled button.

jsFiddle: https://jsfiddle.net/Lau1989/4jLz04pc/5/

First route the click event to the actual file input

//route click event on styled button to the actual file input
button.addEventListener('click', function() {
    input.click();
}, false);

Then replace the full/fake path of the file to display only the file name and its extension

//display the name of the selected file
input.addEventListener('change', function() {
    console.log('val', input.value);
    //Remove the full/fake path to display only the file name
    var file_name = input.value.replace(/[A-Za-z]:\\fakepath\\/i, '');
    output.innerText = file_name;
}, false);

Although this code works so far on all tested browsers, I have no idea if it really should be used in production in terms of security or cross-browser compatibility.

Is there any downside to this approach I don't see yet ? If so, what would be a better approach to do this ?

Your question

Is there any downside to this approach I don't see yet ? If so, what would be a better approach to do this ?

I don't really think there is a downside to this approach. Below I outline an alternative approach to updating the text - it may not necessarily be better but possibly more robust.
Feedback
This looks like a good approach - who needs the fakepath anyway??
I tested this code in Windows 10 Chrome, Firefox and Opera; Safari on Mac OS X, as well as FIrefox on Linux Mint. It appears to work in all except in Firefox on Linux Mint, where the output  tag text was not updated correctly. I was able to update the code to use Element.innerHTML for setting the HTML correctly.
Alternative approach
An alternate approach is to use String.split() on the file input value, splitting on \, then use Array.pop() to get the last item in the array. Expand the snippet below to see it in action.
In theory, this alternate approach should be quicker because it just uses string splitting and array access instead of a regular expression replacement... Here is a jsPerf test - so far it seems it is quicker to use the split() technique in FF and Edge on Windows but not Chrome...

var input = document.getElementById('input'),
  button = document.getElementById('button'),
  output = document.getElementById('output');

//route click event on styled button to the actual file input
button.addEventListener('click', function() {
  input.click();
}, false);

//display the name of the selected file
input.addEventListener('change', function() {
  console.log('val', input.value);
  var parts = input.value.split('\\');
  output.innerHTML = parts.pop();
}, false);
input[type="file"] {
  width: 1px;
  height: 1px;
  position: absolute;
  top: -10px;
  left: -10px;
}

button {
  width: 200px;
  height: 40px;
  cursor: pointer;
}

button,
#output {
  display: inline-block;
  vertical-align: middle;
}
<input type="file" id="input">

<button id="button">Upload image</button>


Validating a credit card number using Luhn's algorithm I implemented Luhn's algorithm explained on Wikipedia to validate a credit card number, in Bash.

Does this code follow good practices?

#!/bin/bash
getchecknumber(){ # get the last digit of the cc number
    str=$1
    checknum=$(echo ${str: -1})
}
getrest(){ # get the rest of the numbers in the sequence
    str=$(echo ${str%?})
}
doubleeveryother(){ # double every other number
    # split characters into an array
    arr=()
    i=0
    while [ "$i" -lt "${#str}" ]; do
        arr+=(${str:$i:1})
        i=$((i+1))
    done
    for ((i=1; i<=15; i++))
    do  {
        char=$(echo "$i-1" | bc)
        double=$(echo "${arr[$char]} * 2" | bc)
        if [ $(($i%2)) -eq 0 ] ; then foo=bar; else arr[$char]=$double #replace the number in the array
        fi
    }
    done
    echo -e
}
adddigitsum() { #split a two digit number into two numbers and add them
    for ((a=0; a<=14; a++))
    do {

        NUMSPLIT=()
        number=$(echo ${arr[$a]})
        if [[ ${#number} == 2 ]] ; then
            i=0
            while [[ "$i" -lt ${#number} ]]; do
                NUMSPLIT+=(${number:$i:1})
                i=$((i+1))
                done
            num=$((${NUMSPLIT[0]} + ${NUMSPLIT[1]}))
            addeddigit=$( echo $num | bc )
            arr[$a]=$addeddigit # replace the number in the array
        fi
    }
    done
}
addall(){ #add all the digits
    sum=$( IFS="+"; bc <<< "${arr[*]}" )
}
multiply(){ #multiply by 9
    result=$( echo "$sum * 9" | bc )
}
validate(){ #see if the last number of 'result' is equal to the checknum
    resultchecknum=$(echo ${result: -1})
    if [[ $resultchecknum == $checknum ]] ; then echo "CC Number is valid!" ; else echo "CC Number is not valid."
    fi
}
echo "$1 "
getchecknumber $1
getrest
doubleeveryother
adddigitsum
addall
multiply
validate

if [ $(($i%2)) -eq 0 ] ; then foo=bar; else arr[$char]=$double

Then foo=bar? In any case, an alternative way of writing this is something like (borrowing your final if statement):

[[ $resultchecknum == $checknum ]] && echo "CC Number is valid!" \
                                    || echo "CC Number is not valid."

Also, you have a mix of echo "..." | bc and bc <<< "...", I suggest keeping to the latter as it simpler to write.

A similar alternative to if-then-else is using &&:

[ ! $(($i%2)) -eq 0 ] && arr[$char]=$double


Censor words in a sentence I wrote three versions of the same code using Reduce, a functional approach and also a regex pattern approach.

I'd like to get feedback, especially from the functional one since I'm learning  functional programming and I'm sure my solution can be greatly improved.

const CHAR = 'X'
const BANNED = ['nice', 'is', 'stupid', 'potato']
const sentence = 'lorem ipsum is a nice and stupid sentence'

// VERSION 1 - REDUCE
const censor = (sentence) =>
  sentence.split(' ').reduce((acc, word) =>
    acc + ' ' + (BANNED.includes(word) ? CHAR.repeat(word.length) : word), '')

const censored = censor(sentence).trim()

console.log(censored)

// VERSION 2 - PIPE (functional?)
const pipe = (...fns) => fns.reduce((f, g) => (...args) => g(f(...args)))

const extractWords = (sentence) => sentence.split(' ')

const removeBanned = (arr) =>
  arr.reduce((acc, word) => acc + ' ' + replaceIfBanned(word), '')

const cleanUp = (arr) => arr.trim()

const replaceIfBanned = word =>
  BANNED.includes(word) ? replaceWithChar(word) : word

const replaceWithChar = (word) => CHAR.repeat(word.length)

const work = pipe(extractWords, removeBanned, cleanUp)(sentence)

console.log(work)

// VERSION 3 - REGEX
const styleHyphenFormat = (sentence) => {
  const replaceWithX = (match) => CHAR.repeat(match.length)
  const filter = new RegExp(BANNED.join('|'), 'gi')

  return sentence.replace(filter, replaceWithX)
}

console.log(styleHyphenFormat(sentence))

You can also check it out in JSFiddle.

The problem with the first two is that you are assuming separators are spaces but they could potentially be tabs, new lines or punctuation. Although it is easy to split on a non-word character joining them back in is tricky (even multiple spaces will fool the algorithm). Your regex solution has the added problem that it will convert words like kiss to kXXs.

I would stick to the regex, it is shorter and, imo, more readable. You can fix the issue by using a word boundary regex match:

const filter = new RegExp(`\\b(${BANNED.join('|')})\\b`, 'gi')

That said, I would also measure the performance of the three solutions as that is likely to be the biggest concern, especially with longer strings.

Edit 1

An alternative solution using split would be something like:

const censor = (sentence) =>
  sentence.split(/\b/).map(
    (word) => BANNED.includes(word.toLowerCase()) ? CHAR.repeat(word.length) : word
  ).join('')


A simple 3D Wrapper class around std::vector I am working on a very simple 3D vector class, which I can use to set up and access three-dimensional vectors using a wrapper around std::vector. It's not intended to be foolproof, just good enough to get by.

The main goal is to make it simple to use, explicit and easy to debug. I have often accidentally put in out-of-bounds coordinates or given invalid dimensions, so this at least can catch those cases and report back in an understandable way.

Hence all the debug prints, should it be used improperly for any reason.

The only thing I am unhappy with is resize. I would not have included it (too dangerous), but found some cases in my code where I could not set up its dimensions in advance, like if it was a member of another class. In those cases, it needed a default and resize option.

Since resize does all the checking, I just reused it for the constructor rather than copy-paste all that.

I'm still somewhat new to C++ and was taught from a C background, so I'm always looking to improve my style.

template <typename T>
class Vector3D
{
 private:
  std::vector<T> data;

 public:
  // Dimensions in each direction
  unsigned long xDim;
  unsigned long yDim;
  unsigned long zDim;

  // Constructor
  Vector3D(unsigned long _xDim, unsigned long _yDim, unsigned long _zDim)
  {
    resize(_xDim, _yDim, _zDim);
  }

  T& at(unsigned long _x, unsigned long _y, unsigned long _z)
  {
    if (_x >= xDim || _y >= yDim || _z >= zDim)
      {
        std::cerr << "Position: " << x << ", " << y << ", " << z << std::endl;

        std::cerr << "Vector width: " << xDim << ", " << yDim << ", " << zDim
                  << std::endl;

        std::cerr << "Invalid indices into 3D vector" << std::endl;

        // Throw an exception here..
      }

    unsigned position = _x + (_y * xDim) + (_z * xDim * yDim);

    return data[position];
  }

  /**
   * @brief Default constructor. Dangerous to use, should use the explicit one
   * if at all possible.
   */
  Vector3D()
  {
    xDim = 0;
    yDim = 0;
    zDim = 0;
  }

  /**
   * @brief Re-sizes a 3D vector with new dimensions.
   * @details This is a nuclear option. You're re-sizing the dimensions of a
   * three-dimensional vector. Do not expect results to be good. Only use this
   * explicitly when initialization is impossible up-front.
   * @param _xDim, _yDim, _zDim - The new dimensions of the vector
   */
  void resize(unsigned _xDim, unsigned _yDim, unsigned _zDim)
  {
    if (_xDim == 0 || _yDim == 0 || _zDim == 0)
      {
        // Throw an exception here...
      }

    xDim = _xDim;
    yDim = _yDim;
    zDim = _zDim;

    // Resize could fail, especially if the dimensions are too large. Just print
    // out the dimensions in x,y,z, number of elements and total bytes just in
    // case
    try
      {
        data.resize(xDim * yDim * zDim);
      }
    catch (std::exception& e)
      {
        std::cerr << e.what() << std::endl;

        std::cerr << "Error allocating 3D Vector. Dimensions: ";

        std::cerr << "X: " << xDim << " Y: " << yDim << " Z: " << zDim
                  << std::endl;
        std::cerr << "Number of elements: " << (xDim * yDim * zDim)
                  << std::endl;
        std::cerr << "Size in bytes: " << (xDim * yDim * zDim) * sizeof(T)
                  << std::endl;

        // Throw an exception here...
      }
  }

  unsigned long getSize() { return data.size(); }

  // Should only be used if the vector is storing pointers to some data
  void freeData()
  {
    for (unsigned long i = 0; i < data.size(); i++)
      {
        free(data[i]);
      }
  }

  // A couple methods to get the real vector, if they want it.
  std::vector<T>& getData() { return data; }

  const std::vector<T>& getData() const { return data; }
};

Quick comments:

unsigned long => size_t everywhere ... it's the same, but that's the alias for this job
check for _xDim, _yDim, _xDim < 0 ... underflow is also undefined behvaiour ;-)
implement operator[] for those who are confident about their bounds, or for faster release builds. It's not trivial for multiple dims, check here.
free data is flawed. You need to look into unique_pointer or shared pointer which will do this stuff for you.
Resize? It is actually required? Can you not auto-resize during at()? unless you want that as a code check during development.
Consider a library


Deflating a Stream using System.IO.Compression I receive messages from a MessageWebSocket connection frequently, which are compressed with zlib. The following code decompresses them to a stream which can be read with JSON.Net's JsonReader.

I'm pretty sure there are some improvements to be done, but can't quite put my finger on what:

private MemoryStream _compressed;
private DeflateStream _decompressor;
private void HandleMessage(object sender, MessageWebSocketMessageReceivedEventArgs e)
{
    using (var datastr = e.GetDataStream()?.AsStreamForRead())
        using (var ms = new MemoryStream())
            {
                datastr.CopyTo(ms);
                ms.Position = 0;
                byte[] input = new byte[ms.Length];
                ms.Read(input, 0, (int)ms.Length);
                int index = 0;
                int length = input.Length;
                using (var output = new MemoryStream())
                {
                    if (input[0] == 0x78)
                    {
                        //Remove the zlib header
                        _compressed.Write(input, index + 2, length - 2);
                        _compressed.SetLength(length - 2);
                    }
                    else
                    {
                        _compressed.Write(input, index, length);
                        _compressed.SetLength(length);
                    }

                    _compressed.Position = 0;
                    _decompressor.CopyTo(output);
                    _compressed.Position = 0;
                    output.Position = 0;

                    using (var reader = new StreamReader(output))
                    using (JsonReader jsreader = new JsonTextReader(reader))
                    {
                        //Deserialize JSON from the jsreader stream
                    }
                }
            }
}

I think you're doing too much in one function, hiding the big picture I'd want to read without going into the implementation details. I'd first start with a broad overview:

using (var reader = new StreamReader(e.GetDataStream().AsStreamForRead())
{
    SkipOptionalZlibHeader(reader);
    var uncompressedStream = ReadAndDecompressStream(reader);
    DecodeJson(uncompressedStream);
}

With:

const int ExpectedZLibHeaderMarker = 0x78;
const int ExpectedZLibHeaderSize = 2;

static void SkipOptionalZlibHeader(BinaryReader reader)
{
    if (!HasZlibHeader())
        return;

    reader.Seek(CalculateZLibHeaderSize(), SeekOrigin.Current);

    bool HasZlibHeader()
        => reader.PeekChar() == ExpectedZLibHeaderMarker;

    long CalculateZLibHeaderSize()
        => return ExpectedZLibHeaderSize;
}

Stop here for one second. Here you made some HUGE assumptions:

Compression flags are always 0x78. That's not true because if message contract simply says that it's a compressed zlib stream with zlib header then they may change it without breaking the contract. Also note that there is no reason the raw compressed stream cannot start with that magic number. You may want to try to decode the trimmed stream and if it fails then try to decode it as it had no header. Better: be sure it has (or not) the header and stick to it otherwise you're deploying code that may fail.
Header size is always two bytes. According to the flags there might be more than that.
Everything else in the stream is data. No, there is also a footer with a checksum.

Note that we dropped one useless copy because SkipOptionalZlibHeader() simply seek after the header. As exercise for the reader: if underlying stream does not support seeking (check CanSeek property) then you need to read and discard those bytes.

Note that ReadAndDecompressStream() is returning a Stream, the fact that you reuse the same MemoryStream multiple times (very good things if you have a measured and truly beneficial gain in memory footprint and/or performance) is an implementation detail and I'd love to keep its callers unaware of that. Also there is no reason to reuse DeflateStream. We may then implement it as this:

static Stream ReadAndDecompressStream(BinaryReader reader)
{
    // Go back to beginning for writing
    _output.Position = 0;

    using (var decompressor = new DeflateStream(_output, CompressionMode.Decompress, true))
    {
        reader.CopyTo(decompressor);
    }

    // Go back to beginning for reading
    _output.Position = 0;

    return _output;
}

There is something that really bothers me: we're returning an IDisposable object (MemoryStream) but caller does not have its ownership (and then it has not to dispose it). It's not intuitive and it must be documented. Also note that your actual code is working just because an implementation detail: StreamReader calls Dispose() on the provided Stream and your code is working just because disposing MemoryStream has not visible effects (but it's an implementation detail). We also have to do something for that.

Let's first change ReadAndDecompressStream() to return a StreamReader instead and change its name to something more meaningful:

static StreamReader CreateDecompressedReader(BinaryReader reader)
{
    // Go back to beginning for writing
    _output.Position = 0;

    using (var decompressor = new DeflateStream(_output, CompressionMode.Decompress, true))
    {
        reader.CopyTo(decompressor);
    }

    // Go back to beginning for reading
    _output.Position = 0;

    return new StreamReader(_output, Encoding.UTF8, false, _output.Length, true);
}

Now caller has the ownership of the disposable object:

using (var reader = new StreamReader(e.GetDataStream().AsStreamForRead())
{
    SkipOptionalZlibHeader(reader);
    using (var decompressedReader = CreateDecompressedReader(reader))
        DecodeJson(decompressedReader);
}

Note that I removed ?., in your original code if e.GetDataStream()? returned null then the first call to datastr.CopyTo() fails. If this may happen then properly handle the case.

As final step you should add some error checking. With I/O things may go wrong then you'd better be prepared (especially if you keep those assumptions in-place), just don't add catch (Exception) and be specific.


HTML generation in C# I have a method that generates some HTML, but it looks ugly to me. Maybe there are some better solutions for this?

 public static string GetReview(Quiz quiz)
    {
        StringBuilder sb = new StringBuilder();

        string questionSection = string.Format(
            "
" +
                "{0}" +
            "
", quiz.Question.GetText());

        string correctAnswer = quiz.Answer.GetText();
        string userAnswer = quiz.UserAnswer?.GetText() ?? string.Empty;

        string answerSection;
        if (correctAnswer == userAnswer) {
            answerSection = string.Format(
                "
" +
                    "Your correct answer:" +
                 "
" +
                 "
" +
                    "{0}" +
                 "
", correctAnswer);
        } else {
            answerSection = string.Format(
                @"
" +
                    "The correct answer was:" +
                 "
" +
                 "
" +
                    "{0}" +
                 "
" +
                 "
" +
                 "You had selected: " +
                 "{1}", correctAnswer, userAnswer);
        }

        string explanationSection = ForQuiz(quiz);

        sb.AppendFormat("
{0}

{1}

{2}
", questionSection, answerSection, explanationSection);

        return sb.ToString();
    }

Before we move to the generation part, let's talk about your html...

        question

The correct answer was:

        good answer

    You had selected: bad answer

Your correct answer:

qwe

 can only nest phrasing content.
 and
 both belongs to flow content and palpable content. Therefore, you can't nested these 2 under
 element. For the definition of a phrasing content consult this answer.
Personally, I would prefer using
 to separate block of text, instead of using a
 to insert a line-break.

        question

The correct answer was:

        good answer

You had selected: bad answer

Your correct answer:

        the answer

As for html generation, I believe, it is better to use a templating engine, such as RazorEngine which allows you to  write C# code within the template itself :

review.cshtml :

@{
    var correctAnswer = Model.Answer.GetText();
    var userAnswer = Model.UserAnswer == null ? Model.UserAnswer.GetText() : string.Empty;
}

    @Model.Question.GetText()

@if (userAnswer == correctAnswer)
{

The correct answer was:

        @correctAnswer

You had selected: @userAnswer

}
else
{

Your correct answer:

        @correctAnswer

}

generation code :

public string GetReview(Quiz quiz)
{
    var path = Desktop.GetFile("review.cshtml"); // TODO: fix this path
    var template = File.ReadAllText(path);

    return Engine.Razor.RunCompile(template, path, typeof(Quiz), quiz);
}

EDIT: I noticed you used the null-propagation operator from c# 6. You can also use the new string interpolation to format the html :

public static string GetReview3(Quiz quiz)
{
    var builder = new StringBuilder();
    builder.Append($@"

            {quiz.Question.GetText()}

    ");

    string correctAnswer = quiz.Answer.GetText();
    string userAnswer = quiz.UserAnswer?.GetText() ?? string.Empty;
    if (correctAnswer == userAnswer)
    {
        builder.Append($@"

The correct answer was:

                {correctAnswer}

You had selected: {userAnswer}

        ");
    }
    else
    {
        builder.Append($@"

Your correct answer:

                {correctAnswer}

        ");
    }

    builder.Append(ForQuiz(quiz));

    return builder.ToString();
}


Implementing an autocomplete system Question:
Implement an autocomplete system. That is, given a query string s and a set of all possible query strings, return all strings in the set that have s as a prefix.
For example, given the query string de and the set of strings [dog, deer, deal], return [deer, deal].
Hint: Try preprocessing the dictionary into a more efficient data structure to speed up queries.
What I tried to do:
Implement prefix tree is way to go as researched from internet. So without looking at actual implementation code, I just grabbed the idea and wrote down the code to challenge myself. Of course code is a messy, but will be really appreciated if someone could advice on how it can be improved in terms of efficiency and simplicity.
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

#define ALP_PREFIX 97 //for numeric value of alphabets

typedef struct stTree
{
    struct stTree *nLetter[26];
    bool isLastChar;
    char value;
} stTree;

stTree* createNode(void)
{
    stTree *node = malloc(sizeof *node);
    node->value = '\0';
    node->isLastChar = false;
    return node;
}

stTree* insertNode(stTree *currNode, char value, bool isLastChar)
{
    if (!currNode->nLetter[value - ALP_PREFIX])
    {
        stTree *node = malloc(sizeof *node);
        node->value = value;
        currNode->nLetter[value - ALP_PREFIX] = node;
    }

    currNode->nLetter[value - ALP_PREFIX]->isLastChar = isLastChar;
    return currNode->nLetter[value - ALP_PREFIX];
}

stTree* treeTraverse(char *chars, stTree *Node)
{
    stTree *nextNode = Node->nLetter[chars[0] - ALP_PREFIX];
    if (nextNode && chars[0] != '\0')
    {
        return treeTraverse(chars + 1, nextNode);
    }
    else
        return Node;
}

void autoComplete(stTree *Node)
{
    for (int i = 0; i < 26; i++)
    {
        if (Node->nLetter[i])
        {
            printf("%c", Node->nLetter[i]->value);
            if (Node->nLetter[i]->isLastChar)
                printf("]");
            autoComplete(Node->nLetter[i]);
        }
    }

    printf("\n");

}

int main(void)
{
    stTree *rootNode = createNode();

    insertNode(insertNode(insertNode(insertNode(rootNode, 'c', false), 'a', false), 'r', false), 't', true);
    insertNode(insertNode(insertNode(rootNode, 'c', false), 'a', false), 'r', true);
    insertNode(insertNode(insertNode(rootNode, 'c', false), 'a', false), 't', true);

    char a[] = "ca";
    printf("%c%c->\n", a[0], a[1]);

    stTree *lastNode = treeTraverse(a, rootNode);

    autoComplete(lastNode);

}

Output:
ca->

r]t]

t]

It means, autocompleted words are: car, cart, cat

I'll only look at the memory vs. speed trade-off.
Suppose that M is the key size we want to look up, and N is the number of words in the dictionary. If we start with a sorted dictionary, then the prefix can be found with binary search, so that is O(M * log N) lookup cost in terms of the dictionary size. A prefix tree has a lookup cost of O(M). But the "work" done to actually look things up is trivial, i.e. on modern CPUs there's no cost to doing the character comparisons and such, especially that you could use SIMD intrinsics to speed that up by a lot. The entire cost is in cacheline accesses.
To speak in concrete terms, let's use the alphanumeric word dictionary from https://github.com/dwyl/english-words, on x86-64 (i.e. 8-byte pointer size). Let's also assume that we're wishing to optimize for lookup efficiency (broadly defined).
First, some general statistics of this dictionary:

3,494,697 letters
370,103 words
1,027,815 nodes in a prefix tree
3 child nodes average branching factor.
9 letters median word length (also the average), 31 letters maximum word length
branching factors (for successive letters looked up):
1-26, 2-22, 3-8.6, 4-5.1, 5-2.7, 6-1.7, 7-1.1, 8-0.9, 9-0.8

So, if we stored the dictionary as a simple concatenation of null-terminated C strings, with no other optimizations, it'd take around 4MB of memory. This fits comfortably in the 6MB L3 cache of the CPU I'm trying this on (that cache can hold ~100k cachelines of 64 bytes each, in best case).
On the other hand, the question's implementation of the prefix tree takes ~220MB of RAM because the nodes are huge: 216 bytes. We'd have hoped that the prefix tree would have "compressed" our data, but instead it expands it by a factor of 55x. Each node fits in 4 cache lines (3.3 really).
Tree traversal using Node needs to pull up to two cachelines per Node (maybe 1.5 on average): one for the child node pointer, another for the value and the child pointer. Some of those cachelines will have to be misses, since the tree doesn't fit in the cache.
The L3 cache can fit around 30k nodes. Let's somewhat arbitrarily assume that about 10k of them will be "long lived hot nodes". The product of the first 5 branching factors is ~25k, so we can estimate that about 4 first characters of each word will be nodes that fit in L3. With average word being 9 letters long, we will miss 4-5 additional nodes, or 8-10 additional cachelines. That'd be the real cost of this implementation on this particular data set.
Now let's say our benchmark will be as follows: take a "lookup string" of all words in random order, concatenated into a long string of zero-terminated words. Iterating such a string is very fast, since the CPU will prefetch everything, so other than some cache pressure, the overhead of using such permuted word stream is very low. We use words from that string as lookup keys, and copy them to an output string as we perform each lookup. An exemplary benchmark takes about 120ms to look up all the 370k words in random order from the prefix tree, constructing the "output string". The function being timed is below, the assert is a no-op in release mode, and the nodes were all allocated from a contiguous memory block.
I wrote the examples below in C++, to make life a bit easier, but they could all be written in C without any loss of performance.
auto doLookup(Node *root, const std::string &lookupStream)
{
    std::string result, word;
    Node *node = root;
    result.clear();
    result.reserve(lookupStream.size());
    for (char ch : lookupStream) {
        if (ch) {
            node = node->children[ch - 'a'];
            word.push_back(ch);
        } else {
            node = root;
            result.append(word);
            result.push_back('\0');
            word.clear();
        }
    }
    assert(result == lookupStream);
    return std::make_pair(std::move(result), node);
}

Given this "standard" to compare with, let's do the "next most stupid thing": an equivalent lookup using just the dictionary, with zero-delimited words in ascending order, using a binary search, where each time we "throw a dart" in the dictionary, we have to scan back to find the beginning of the word.
The function being timed is below. It isn't particularly pretty but it does the job.
auto doLookup2(const std::string &dictionary, const std::string &lookupStream)
{
    assert(dictionary.front() == '\0');
    assert(dictionary.back() == '\0');
    assert(lookupStream.back() == '\0');
    std::string result, word;
    result.reserve(lookupStream.size());
    for (char ch : lookupStream) {
        if (ch)
            word.push_back(ch);
        else {
            const char *begin = &dictionary.front(), *end = &dictionary.back();
            const char *prevP = nullptr;
            for (;;) { // binary search
                const char *p = begin + (end - begin) / 2;
                const char *const p0 = p;
                assert(p0 != prevP); // otherwise we didn't find the word
                while (*p)
                    --p; // find start of the word
                p ++;
                int const cmp = strcmp(word.data(), p);
                if (cmp == 0) {
                    assert(word == p);
                    result.append(word);
                    result.push_back('\0');
                    word.clear();
                    break;
                }
                if (cmp < 0 /* word < p */) {
                    end = p0;
                } else { /* word > p */
                    begin = p0;
                }
                prevP = p0;
            }
        }
    }
    assert(result == lookupStream);
    return result;
}

This function takes about 170ms (vs. the tree's 120ms), and trades off 50% longer execution for 55x smaller memory use, although it doesn't find the first word with a given prefix, just some word - then one has to iterate backward. Doing that in the most rudimentary way adds some 10ms to the runtime (I'm not showing it above).
This trade-off may be acceptable as-is. But either approach could be micro-optimized. Cutting the tree size in half by replacing the node pointers with indices of nodes in the pool cuts about 10ms off the runtime - we don't expect a huge improvement, since the tree still doesn't fit in the cache.
But the tree approach seems promising, so let's try to reduce memory consumption further. Instead of allocating 26 child node pointers in each node, let's allocate only one, and double the pointer array size whenever we run out of space. There isn't a direct index-to-pointer mapping for child pointers anymore, but instead we can store a bit flag for each character, indicating whether the pointer is present or absent in the array. Furthermore, the leaf nodes can all be stored as stock nodes, since they are identical. The node looks as follows:
struct TreeNode
{
    uint32_t activeChildren;
    struct TreeNode *children[1]; // this is a dynamically sized array
} typedef TreeNode;

The activeChildren member also stores the end-of-word flag - access is provided via accessors.
This approach reduces the allocated memory size to 15MB, and the lookup benchmark takes about 140ms - only 20ms worse than the "reference" implementation. Only 750k nodes are individually allocated, the remaining 250k are all leaf nodes collected as the single stockLeafNode.
Retrieval of a child node looks as follows:
TreeNode **getChildNode(TreeNode *node, char ch)
{
    ch -= NODE_LOWEST_VALUE;
    uint32_t childrenBits = node->activeChildren;
    if (!bitIsSet(childrenBits, ch))
        return NULL;
    return node->children + numberOfSetBitsBelow(childrenBits, ch);
}

numberOfSetBitsBelow(bits, bit) returns the count of set bits ("1") in the bits paramer, but only the bits in positions lower than bit number bit. E.g. numberOfSetBitsBelow(0xFE, 3) == 2, because below bit 3 only bits 2 and 1 are set.
Using getChildNode, we have the following prefix lookup:
TreeNode *findNode(TreeNode *node, const char *word)
{
    for (char ch = *word++; ch; ch = *word++) {
        TreeNode *nextNode = *getChildNode(node, ch);
        if (!nextNode)
            break;
        node = nextNode;
    }
    return node;
}

Complete project is at https://github.com/KubaO/stackoverflown/tree/master/questions/cr-prefix-tree-252512


Compute max population given a list of people's birth and death year I recently came across a brief programming puzzle prompt (say that three times fast) which asked to return the year that population peaked.

The actual algorithm to compute it can be done several ways and is not particularly difficult, but I have recently been challenging myself to write .. ahem .. "more functional" code.

There are several tenets of FP, but a chief one is not producing what developers call "side effects". I'm hoping to get to a point where I do not rely as much on classic object-oriented patterns and feel more at ease diving right into the functional style. Of course this takes practice, so I decided to spend some extra time polishing up my code to meet my internal requirements:

val persons = Seq(
  Person(2000, 2010),
  Person(2000, 2005),
  Person(1990, 2005)
)

// Create an array of population change.
val births = persons.groupBy(_.birthYear).mapValues(_.length)
val deaths = persons.groupBy(_.deathYear).mapValues(_.length)
val populationChanges = (births.keySet ++ deaths.keySet).map { year =>
  (year, births.getOrElse(year, 0) - deaths.getOrElse(year, 0))
}.toMap

println("populationChanges", populationChanges)

val sortedYears = populationChanges.keys.toSeq.sorted
println(sortedYears)

// construct an array of total populations
val populations = populationChanges.toSeq
  .sortBy(_._1)
  .scanLeft(0) { (a, b) => a + b._2 }
  .tail
  .zip(sortedYears) // this is a bit hacky
  .map{ case(count, year) => (year, count) }

println("pop", populations)
println("max pop", populations.maxBy(_._2)) // (2000, 3)

This is a demonstration of what I created after a few minutes of iteration.

I was readily able to produce a list of population changes and from there, performed further transformations to arrive at a list of total populations for each year. From there determining the year that population peaked was trivial.

However, I'm not fully satisfied with this solution. I could be being too dogmatic, but in my mind I would not be breaking scope of a map function to reference external values, even though it proved expedient in this case.

Your approach isn't bad if you want to do multiple analyses of the population over time. For programming puzzles I often find it useful to focus in on exactly what's being requested and leave the other interesting stuff for another day.

persons.flatMap(p => Seq((p.birthYear, 1), (p.deathYear, -1)))
       .sorted
       .foldLeft((0,0,0)){case ((mxYear,mxPop,curPop), (thisYear,dif)) =>
         if (curPop+dif > mxPop) (thisYear, curPop+dif, curPop+dif)
         else (mxYear, mxPop, curPop+dif)
       }._1  //res0: Int = 2000

In this case the data is

traversed once - to attach a 1 or -1
sorted - to get all the years in order
traversed a 2nd and final time - to count the population changes and save the maxes


"What is the maximum that Player 1 can win?" This is a question that I encountered in one of the competitive coding tests. The question goes as follows:

  A 2-player game is being played. There is a single pile of stones. Every stone has an amount (positive) written on top of it. At every turn, a player can take the top 1 or 2 or 3 stones and add to his kitty. Both players want to maximize their winnings. Assuming both players play the game optimally and Player 1 starts the game, what is the maximum amount that Player 1 can win?

I have devised the following recursive approach:

class Main {
  public static void main(String[] args) {
    int a[] = {1,2,3,7,4,8,1,8,1,9,10,2,5,2,3};
    int sum = 0;
    for (int i=0;i<a.length; i++) {
      sum += a[i];
    }
    System.out.println(maxPlayer1(a, 0, sum, 0, a.length));
  }
  public static int maxPlayer1(int[] a, int currSum, int sum, int start, int len) {
    if (len-start <=3) {
      int val = 0;
      for (int i=start; i<len; i++) {
        val += a[i];
      }
      return val;
    }

    int v1 = a[start] + (sum - currSum - a[start]) -
    maxPlayer1(a, currSum + a[start], sum, start + 1, a.length);

    int v2 = a[start] + a[start+1] + (sum - currSum - a[start] - a[start+1]) -
    maxPlayer1(a, currSum + a[start] + a[start+1], sum, start + 2, a.length);

    int v3 = a[start] + a[start+1] + a[start+2] + (sum - currSum - a[start] - a[start+1] - a[start+2]) -
    maxPlayer1(a, currSum + a[start] + a[start+1] + a[start+2], sum, start + 3, a.length);

    return Math.max(v1, Math.max(v2, v3));
  }
}

I have checked my solution on a few inputs. Is my algorithm correct?

Usability

I have a remark about defining recursive functions.
You don't want consumers of your API to think about the intermediate variables and their initial value. The consumer does not care you are using recursion internally.

Make your recursive function private.

private static int maxPlayer1(int[] a, int currSum, int sum, int start, int len)

And create a public API entrypoint.

public static int maxPlayer1(int[] a) {
     final int sum = IntStream.of(a).sum();
     return maxPlayer1(a, 0, sum, 0, a.length);
}

The consumer can now call your API without getting a headache:

public static void main(String[] args) {
     final int a[] = {1,2,3,7,4,8,1,8,1,9,10,2,5,2,3};
     System.out.println(maxPlayer1(a));
}

As compared to the original code the consumer had to write.

public static void main(String[] args) {
    int a[] = {1,2,3,7,4,8,1,8,1,9,10,2,5,2,3};
    int sum = 0;
    for (int i=0;i<a.length; i++) {
      sum += a[i];
    }
    System.out.println(maxPlayer1(a, 0, sum, 0, a.length));
  }


Making a row of bricks We want to make a row of bricks that is goal inches long. We have a number of small bricks (1 inch each) and big bricks (5 inches each). Return true if it is possible to make the goal by choosing from the given bricks.

bool make_bricks(int small,int big,int goal)
{
    if(big > 0)
    {
        double test = (double)goal/((double)big*5.0);//The above if statement eliminates the possibility of division by zero
        if(test > 1.0)//This means we cannot make the row with big bricks alone
        {
            if(small >= (goal-big*5))//If we have enough small bricks after using all our big bricks
            {
                return true;
            }
            else//If we don't have enough small bricks to finish the row
            {
                return false;
            }
        }
        else if(test == 1)//If we have exactly enough big bricks to make the row
        {
            return true;
        }
        else//We have enough big bricks to make the row, but perhaps not enough small bricks
        {
            if(small >= (goal%5))//If we have enough small bricks after using as many big bricks as possible
            {
                return true;
            }
            else//We do not have enough small bricks after using as many big bricks as possible
            {
                return false;
            }
        }
  }
  else//If the big is not greater than 0, we depend solely on small bricks to make the row.
  {
      if(small >= goal)//If we have enough small bricks to make the row or more than enough
      {
          return true;
      }
      else//We do not have enough small bricks to make the row
      {
          return false;
      }
  }
}

Your function is very much more complicated than it needs to be.
As I understand it, the function should check if it is possible to build a row of exactly the goal size, that goal is not a "minimum" requirement, but the exact requirement.
Futhermore, I assume it would be OK to make a row that is of size 14, for example, completely out of 14 small bricks, and 0 large bricks, as long as the row has the size of goal.

We test for the small bricks first:

// check minimum number of small bricks required.
if (goal % 5 > small) return false; // not enough small bricks.

If the target is something like 14, then no matter what, we will absolutely need at least 4 small bricks, or the goal is impossible.

Then we test to see if there is enough length, in total, to make the goal:

if (small+big*5 < goal) return false; // not enough
else return true; // passed both tests

And if we pass both of these checks, then we know it is possible to reach the goal with the bricks that were supplied.

Here is how you could write the entire funcion:

bool make_bricks(int small,int big,int goal)
{
    if (goal % 5 > small) return false;
    if (small+big*5 < goal) return false;
    else return true;
}

As suggested by user "200_success", you could further simplify this to:

bool make_bricks(int small,int big,int goal)
{
    return (goal % 5 <= small) && (small+big*5 >= goal);
}

This version does cut down on the number of code lines, if that matters to you. Performance wise, there is no reason to think it will actually make a difference which one you use. The first, longer version, is perhaps clearer and easier to comment.

Note that, of course, none of these versions take into account what would happen if the function was called with nonsensical arguments, such as a negative number of small bricks.

Perhaps as a side-note, note that modulus % calculation is costly, though, so don't use it when you can avoid it. If you're writing x % 10, for example, then you might save performance by first checking if x < 10, and possibly avoid a few modulus calculations. That is, if you expect ´x´ to often be less than 10. If you expect the opposite, then don't make such a check.

When it comes to the design/interface choices, I would criticize the choice of the name of the function. The name seems to imply that the function "makes bricks". Good practice suggests that you name your functions in a way that makes it possible to deduce what the function does. And as  Scott Meyers explained in a talk, it's about what people who see the function/interface expect about it.
Even if you are the only one who will ever use this function, I'm talking about good practice, and I hope and assume you'll appreciate the input/feedback, since you posted it here.

Common practice for functions whose primary role is to return a bool is to have the function name be a statement about the thing in question, such as is_ready() or was_released(), or something of that style, since this fits with the idea of true or false.
I suggest you consider something like bool can_make_row(int numSmall, int numBig, int rowSize).


Calling a function when all asynchronous calls complete Below is a simplified version of some code I am using to execute a function after all asynchronous calls complete.  Is this a reasonable piece of code? It seems to work.  Nothing would break if DoStuff() was called twice, though that would be very inefficient.

var i = 0;
var done = function () {
    i++;
    if (i < 2) return;
    //Everything is done
    DoStuff();
};
elem1.executeAsync(function () {
    done();
});
elem2.executeAsync(function () {
    done();
});

The approach itself (i.e. using a counter) is reasonable, but the hard-coded 2 and DoStuff limits its use.

You could do something like this, to derive more generic implementation:

function createCounter(count, callback) {
  count || (count = 1); // default to 1
  (typeof callback === 'function') || (callback = function () {}); // default to no-op
  return function () {
    --count || callback();
  };
}

Now you have a function that returns a specialized "done" function. Use like so:

var done = createCounter(2, DoStuff);

elem1.executeAsync(function () {
  done();
});

elem2.executeAsync(function () {
  done();
});

Incidentally, this can be shortened to:

var done = createCounter(2, DoStuff);

elem1.executeAsync(done);
elem2.executeAsync(done);

(of course, your actual code is likely more complex)

However, I'd say you should also look into the Promise/Deferred patterns.
For instance, using jQuery's implementation (and assuming executeAsync returns a promise) you can do this

$.when(elem1.executeAsync(), elem2.executeAsync()).then(DoStuff);


Possible Bugs in substring program I have written a small snippet to validate a substring in a string in O(n). I have tested the code using some combinations. I require your help to let me know if there are any bugs in my code with respect to any other cases that I might have missed out:-

class StringClass( object ):
    def __init__( self, s ):
        self._s = s
    def __contains__( self, s ):
        ind = 0
        for ch in self._s:
            if ind < len( s ) and ch == s[ind]:
                ind += 1
            elif ind >= len( s ):
                return True
            elif ch == s[0]:
                ind = 1
            elif ch != s[ind]:
                ind = 0

        if ind >= len( s ):
            return True

if __name__ == '__main__':
    s = StringClass( 'I llove bangaolove' )
    print 'love' in s

PS: Here, Instead of trying to find the string 'love' in 'I llove bangalove', I'm using the other method: i.e. to find if the string 'I llove bangalove' contains 'love'.

Please let me know if there are any corrections to be made here.

One case that you missed is replication in the search string.

>>> '1213' in StringClass('121213')
False
>>> '1213' in '121213'
True

This is because your class is already past the second one before it sees a difference and has to start completely over.

Asides from that, the empty string and None cases are problems, as is mentioned in other answers.


The only thing better than being unique is being first and unique.

Challenge:

  Write a program which finds the first non-repeated character in a string.

Specifications:

  The first argument is a path to a file.
  The file contains strings, one per line.
  Print out the first non-repeated character of each string.

Solution:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.Map;
import java.util.LinkedHashMap;
import java.util.Scanner;

public class NoRepeat {
    public static void main(String[] args) throws FileNotFoundException {
        Scanner input = new Scanner(new File(args[0]));

        while (input.hasNextLine()) {
            System.out.println(
                retrieveFirstNonRepeatedLetter(input.nextLine())
            );
        }
    }

    public static char retrieveFirstNonRepeatedLetter(String input) {
        Map<Character, Boolean> letters = new LinkedHashMap<>();
        char firstNonRepeated = ' ';

        for (int i = 0; i < input.length(); i++) {
            char current = input.charAt(i);

            if (letters.containsKey(current)) {
                letters.put(current, false);
            } else {
                letters.put(current, true);
            }
        }

        for (Map.Entry<Character, Boolean> entry : letters.entrySet()) {
            if (entry.getValue() == true) {
                firstNonRepeated = entry.getKey();
                break;
            }
        }

        return firstNonRepeated;
    }
}

This solution is \$O(n)\$ but I get the feeling that it's far from the best possible way to accomplish this.

I thought maybe I should set it to account for upper and lower cases, but it wasn't specified (and 100% passed with this implementation).

Tests:

public class MyFirstUnitTest {
    @Test
    void toothyTest() {
        assertEquals('h', NoRepeat.retrieveFirstNonRepeatedLetter("tooth"));
    }

    @Test
    void smellyTest() {
        assertEquals('d', NoRepeat.retrieveFirstNonRepeatedLetter("odor"));
    }
}

This is on top of @tim's excellent review.
I think this is a very good solution,
and a clever use of LinkedHashMap.
I can't think of a better or simpler algorithm.

Simplifications

This can be written simpler:

if (letters.containsKey(current)) {
    letters.put(current, false);
} else {
    letters.put(current, true);
}

Like this:

for (int i = 0; i < input.length(); i++) {
    char current = input.charAt(i);
    letters.put(current, !letters.containsKey(current));
}

Instead of comparing boolean expressions to true like this:

if (entry.getValue() == true) {

You can use them directly:

if (entry.getValue()) {

Unit tests

It's nice that you start adding unit tests. It will pay off!

To run your test cases in an IDE,
you need to make them public (at least I had to, in IntelliJ).

As a small extra tip,
to avoid long repeated calls like NoRepeat.retrieveFirstNonRepeatedLetter(...),
sometimes it can be practical to create a helper method, for example:

void assertFirstNonRepeatedLetter(char expected, String string) {
    assertEquals(expected, NoRepeat.retrieveFirstNonRepeatedLetter(string));
}

@Test
public void toothyTest() {
    assertFirstNonRepeatedLetter('h', "tooth");
}

@Test
public void smellyTest() {
    assertFirstNonRepeatedLetter('d', "odor");
}

Why fall back to firstNonRepeated = ' ' ?

I suggest to pay close attention to the specs.
It doesn't mention what should happen if there are no unique letters in the input.
Don't assume arbitrary fallback values.
The behavior is undefined, so it's kind of an illegal state.
It's a good move to throw an IllegalStateException in this case:

public static char retrieveFirstNonRepeatedLetter(String input) {
    Map<Character, Boolean> letters = new LinkedHashMap<>();

    for (int i = 0; i < input.length(); i++) {
        char current = input.charAt(i);
        letters.put(current, !letters.containsKey(current));
    }

    for (Map.Entry<Character, Boolean> entry : letters.entrySet()) {
        if (entry.getValue()) {
            return entry.getKey();
        }
    }

    throw new IllegalStateException("No non-repeated characters");
}

And add a corresponding unit test to cover this:

@Test(expected = IllegalStateException.class)
public void nothingUnique() {
    NoRepeat.retrieveFirstNonRepeatedLetter("abab");
}


Learning PHP MVC: bootstrapping and front-controller I am creating an MVC-Framework to learn more about how PHP and software architecture works in general.
My goal is to create a simple website with it.

Right now I have trouble on what exactly is the job of the bootstrap file and the FrontController Class.

Can you have a look if what I did makes sense to you?

My Bootstrap-File looks like this:

<?php

use app\core\FrontController;
use app\core\Router;

// include configuration, database and autoloader
require_once(realpath(__DIR__ .DS.'..') . "/config/config.php");
require_once(realpath(__DIR__ .DS.'..') . "/config/db.php");
require_once(realpath(__DIR__ .DS.'..'.DS.'..') . "/vendor/autoloader.php"); // autoload for the core MVC Framework

// load composer autoloader
require_once(realpath(__DIR__ .DS.'..'.DS.'..') . "/vendor/autoload.php");

// set routes
$route = isset($_GET['route']) ? $_GET['route'] : null;
$action = isset($_GET['action']) ? $_GET['action'] : null;

// get FrontController
$fc = new FrontController(new Router, $route, $action);

echo $fc->output();

And the FrontController Class

<?php

namespace app\core;

class FrontController {
    public $pdo;
    private $route, $routeName, $model, $controller, $view, $twig;

    public function __construct(Router $router, $routeName, $action = null) {
        $this->pdo = new \PDO('mysql:host='.DB_HOST.';dbname='.DB_NAME, DB_USER, DB_PASS);

        //Fetch a route based on a name, e.g. "search" or "list" or "edit"
        $this->route = $router->getRoute($routeName);
        $this->routeName = $routeName;

        //Fetch the names of each component from the router
        $modelName = "\app\model\\".$this->route->model;
        $controllerName = "\app\controller\\".$this->route->controller;
        $viewName = "\app\\view\\".$this->route->view;

        //Instantiate each component
        $this->model = new $modelName($this->pdo);
        $this->controller = new $controllerName($this->model);
        $this->view = new $viewName($this->model);

        //Run the controller action
        if(!empty($action) && method_exists($this->controller, $action)) $this->controller->{$action}();

                // load TWIG
                $loader = new \Twig_Loader_Filesystem(realpath(__DIR__ .DS.'..'.DS.'..') . "/site/themes/".TPL_DEFAULT."/templates/"); // *!* replace TPL_DEFAULT with $theme
                $this->twig = new \Twig_Environment($loader, array(
                    'cache' => realpath(__DIR__ .DS.'..'.DS.'..') . "/cache/compilation/",
                ));
    }

    public function getRouteName() {
        return $this->routeName;
    }

    public function getModel() {
        return $this->model;
    }
    public function getController() {
        return $this->controller;
    }
    public function getView() {
        return $this->view;
    }

        public function output() {
                var_dump($this->twig);
                /* removed testing code for getting twig template on code Review SO */

        $nav = $this->view->output($this->routeName, "nav");

        var_dump($nav); // *!* test

        $page = $this->view->output($this->routeName);

        $title = $page["title"];
        $content = $page["content"];

        return $header . "<h1>" . $title . "</h1>" . $content . $footer;
    }
}

I thought maybe you could review these two files and tell me if I understood the use of them correctly.

For me it's like bootstrap is there to just import stuff like config, autoloader, FrontController.
And the FrontController does the basic handling of the Framework.

I hope this code makes at least some sense.
You can find the whole project here btw.

I would say you are generally on the right track but have some mixed responsibilities: there are some things in the front-controller that belong to the bootstrapping and some things in the front-controller that belong in the actual controller.

Also a major comment: I think as long as you are doing this you should take the time and also implement a dependency injection container (of some sort, even if a very simple one).  DI and Inversion of Control are standards in not just the PHP MVC world but also the OOP world in general.  As long as this is a learning exercise for you, you might as well learn another very critical component of such a system.  No bootstrapping/front-controller would be complete without proper use of dependency injection, if for no other reason that to enable proper unit/integration testing.

Starting at the beginning then:

A nitpick, but I would give your bootstrap file a namespace.  If it is using things from app\core, I think it makes sense for it to live in app\core itself.  Related to that: is this your index.php file, or is it included by your index.php file?  Personally I split things up a bit more even, and have the bootstrap simply initialize the app (i.e. require all necessary files, initialize error catchers, etc) and let the index.php file require the bootstrap file.  Then, I have the index.php file load and call the FrontController.  That's certainly not a requirement, but I find it to be helpful as you might have a couple different "environments" that run differently, but still require bootstrapping (i.e. command line, testing, and responding to web requests).
You have a lot of lines like this: realpath(__DIR__ .DS.'..'.DS.'..').  Many systems will define constants for these sorts of things in the bootstrap so that you don't have to repeat it so often (in particular because knowing where directories are is often needed by other parts of the system, i.e. file caching, uploads, etc...).
Because you are passing the router into the FrontController, you are already halfway to dependency injection.  However, you then have the FrontController directly build the database connection.  Especially since the database connection isn't used by the FrontController, there is no reason for the FrontController to be building it.  You make it public which implies that you intend for other things to get the database out of the FrontController, but it is not clear how they would access it, nor is this a good way of passing necessary resources back and forth.  Again, a DI container would fix all of these issues, and then you can have a class that is a simple wrapper around the PDO object, and which initializes the DB connection when it is actually needed. You just have to design your DI container so that you can make your database object a "singleton" which is only created once.
Your FrontController is initializing both the model and view.  Ignoring issues revolving around using a DI container to instantiate resources, it doesn't make sense for the FrontController to be doing anything with models or views at all.  The point of the FrontController is to act more like a directory.  It takes the route, determines what controller should be handling the route, and then defers execution to the actual controller assigned to the route.  The final controller is then responsible for instantiating models (if needed), calling views, etc.  As a for instance, what if a view isn't needed because a particular route represents an API call which returns JSON?
In essence, your biggest issue is that your front-controller is strongly coupled to the rest of your application, leaving no room for flexibility on how the application should respond to a given request.  Effectively, your system is requiring one model, view, and controller for every route.  It rarely works out that cleanly in practice, and so decoupling your FrontController from the rest of the application is a necessity.  Again, you should think of it more as a gatekeeper than as something that actually performs any actions itself.

All in all I would say that this is a good starting point.  You obviously have best-practices and design patterns in mind, and that is always a good thing.  From here on out it is all about understanding and implementing good SOLID principles, in particular a good division of labor and separation of concerns between all the various actors of your system.


Drop-down for populating a list of cities I have created a drop-down to populate a list of cities. Everything works fine, but I would like to know better ways of doing this. Also, please let me know if it is possible to create the same drop down using <Select> instead of HTML helpers.

//ViewModel

public class LocationDTO
{
    public IEnumerable<CityDTO> Cities { get; set; }
    public LocationDTO()
    {
        this.Cities = new CityDTO[] { };
    }
}

public class CityDTO
{
    public string CityId { get; set; }
    public string CityName { get; set; }
}

I've used entity framework database first approach to get the data back from database. Please address improvements that need to be done.

//Controller

Models.LocationDTO Loc = new Models.LocationDTO();
EF.LocationEntities locCtx = new EF.LocationEntities();

public Action Result Index() {
    using(locCtx) {
        var locResults    = (from q in locCtx.usp_GetAllCities()
                       Select new Models.CityDTO {
                       CityId = q.Id,
                       CityName = q.Name  });
        loc.Cities = locResults.ToList();
    }

    List<Models.CityDTO> citiesList = new List<Models.CityDTO>();
    Models.CityDTO city = new Models.CityDTO() { CityId = "-1", CityName = "Select City" };
    citiesList.Add(city);
    citiesList.AddRange(Loc.Cities.ToList());

    ViewBag.CitiesDropDown = citiesList;
    return view(loc);
}

I'd also like to know how the lamdba expression works in this scenario:

//View

@{
    List<TestApp.Models.CityDTO> citiesList = ViewBag.CitiesDropDown;
    var cityItems = new SelectList(citiesList, "CityId", "CityName");
}
<div>
    Cities: @Html.DropDownListFor(x => x.Cities.SingleOrDefault().CityID, @cityItems)
</div>

View Model

Looks fine to me, although I'd recommend that if all your DTO classes are to be appended with 'DTO' you instead put them inside a namespace named 'DTO' and leave the 'DTO' characters off the class names entirely.

Controller

Use var when the Right Hand Side (RHS) is obvious.

Also, I don't like the indentation in your LINQ statement. It makes it difficult to discern between LINQ and constructor.

There is no point in calling ToList so much. AddRange is going to enumerate the whole collection anyway, so you don't actually need any ToList calls at all.

var Loc = new Models.LocationDTO();
var locCtx = new EF.LocationEntities();

public Action Result Index() {
    using(locCtx) {
        var locResults = (from q in locCtx.usp_GetAllCities()
                             Select new Models.CityDTO
                             {
                                 CityId = q.Id,
                                 CityName = q.Name
                             });
        loc.Cities = locResults;
    }

    var citiesList = new List<Models.CityDTO>();
    var city = new Models.CityDTO()
    {
        CityId = "-1",
        CityName = "Select City"
    };
    citiesList.Add(city);
    citiesList.AddRange(Loc.Cities);

    ViewBag.CitiesDropDown = citiesList;
    return view(loc);
}

View

You make a call to Cities.SingleOrDefault(). SingleOrDefault() can return a null value if your City class' default operator returns null (which is the err default behaviour). If this happens, accessing CityId will fail.

//View

@{
    List<TestApp.Models.CityDTO> citiesList = ViewBag.CitiesDropDown;
    var cityItems = new SelectList(citiesList, "CityId", "CityName");
}
<div>
    Cities: @Html.DropDownListFor(x => x.Cities.SingleOrDefault().CityID, @cityItems)
</div>


Thread-safe wrapper around WCF ServiceModel.ClientBase subclass I have an ASP.NET application which uses a Service Reference to a (third-party, offsite) payment processor.

The service reference class is generated automatically.
Its implementation is a subclass of ServiceModel.ClientBase, which
MSDN documents as, "Any instance members are not guaranteed to be thread safe."

Is the following a good implementation of my wrapper class (to be called from my aspx pages), to guarantee that access to the service reference singleton is thread-safe?
Should I prefer to simply make the service reference an instance member of the wrapper class, without any locking? My fear was that would imply more than one concurrent service instance, and I don't know how to test whether the remote server side would handle that successfully.

class Authorize : IDisposable
{
    private static ServiceSoapClient s_serviceSoap = null;
    private static object s_locker;

    // This method is invoked from Global.Application_Start() in Global.asax.cs
    internal static void Application_Start()
    {
        // ServiceSoapClient derives from ServiceModel.ClientBase<ServiceSoap>
        s_serviceSoap = new ServiceSoapClient();
        s_locker = new object();
    }

    private bool lockWasTaken = false;
    internal Authorize()
    {
        // Emulate C# `lock` statement
        Monitor.Enter(s_locker, ref lockWasTaken);
        // http://stackoverflow.com/questions/2008382/how-to-heal-faulted-wcf-channels
        if (s_serviceSoap.InnerChannel.State ==
           System.ServiceModel.CommunicationState.Faulted)
        {
            s_serviceSoap.Abort();
            s_serviceSoap = new ServiceSoapClient();
        }
    }
    public void Dispose()
    {
        if (lockWasTaken)
            Monitor.Exit(s_locker);
    }

    internal bool IsAlive()
    {
        // delegate to the singleton
        ANetApiResponseType response = s_serviceSoap.IsAlive();
        log(response, "IsAlive");
        return response.resultCode == MessageTypeEnum.Ok;
    }
    // plus other methods which delegate to the singleton
}

The code in the OP seems thread-safe to me, but may not be the best solution.
If the channel has many concurrent users, and if using the channel takes for example 100 msec, then 100 concurrent users would queue for up to 10 seconds before being able to use the (single) channel one after the other. A 10-second delay is unacceptable (web users abandon the session).
If the channel is to a payment processor, authorizing a customer transaction might take longer than 100 msec (not 100++ msec of CPU time for your ASP.NET server, but rather a 100++ msec delay waiting for a network response from the payment processor).
Wenlong Dong's Blog article, "Performance Improvement for WCF Client Proxy Creation in .NET 3.5 and Best Practices" (2007) says,

A common pattern of using ClientBase styled proxies is to perform
creation/disposing on each call:
foreach (string msg in myList)
{
    // You may add try/catch block here ...
    HelloWorldProxy proxy = new HelloWorldProxy("MyEndpoint", new EndpointAddress(address)))
    proxy.Hello(msg);
    proxy.Close();
    // Error handling code here ...
}

This article is not ASP.NET-specific, but it states that concurrent instances of a channel are normal.
This article also warns that, creating and destroying proxies can be very expensive, gives advice on how to minimize the cost, mentions a performance improvement (implicit cacheing) made in .NET 3.5, and warns what behaviour (i.e. specifying an endpoint) will disable that cacheing.
A follow-up article titled A Sample for WCF Client Proxy Pooling gives advice on how to implement a pol of proxies.

See also here and here for warnings about how to close and dispose a WCF channel safely:

It should be closed before it's disposed
If there has been a communication error the Close could throw an exception
In an exception is thrown you should call Abort before trying to Dispose the channel


Outputting a user's name, day of birth, and test scores in a bar chart I've been asked to output the users name, what day of the week they were born on, output a line from a poem corresponding with the day they were born on, and their scores for a vark test. We were told to calculate the month of birth as a number and find the number/ month.

This is the first bit of code I've done and I'm just after some comments/feedback on it

MONTHS = 'JANFEBMARAPRMAYJUNJULAUGSEPOCTNOVDEC'; //months
document.write ('
Introduce Yourself

');
var userName = ''; //create username variable
userName = prompt('Enter your First Name','Alex'); //prompt user for name

var dateString = prompt('Enter your birth date in the form dd mmm yyyy','26 APR    1988'); //get users date of birth
var substrings = dateString.split(' ');
var day = substrings[0];
var month = substrings[1];
var year = substrings[2];

var monthNumber = month.indexOf(MONTHS);
if (monthNumber >= 0); {
}

var poem = ["Sunday's child is fair and wise", "Monday's child is fair of face",        "Tuesday's child is full of grace", "Wednesday's child is full of woe", "Thursday's child has far to go", "Friday's child is loving and giving", "Saturday's child works hard for a living"];
var daysArray = new Array ('Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday',    'Friday', 'Saturday');
var birthDate = new Date(year, monthNumber/3, day);
var dayNumber = (birthDate.getDay());
var dayName = daysArray[dayNumber];
var dayPoem = poem[dayNumber];
document.writeln ('

My name is:   '+ userName +'

 I was born on a '+ dayName +':  '+dayPoem+'

');

var varkString = prompt('Enter your VARK scores - [visual|aural|read|kinesthetic]','9|3|11|10');
var subStrings = varkString.split('|');
var visual = subStrings[0];
var aural = subStrings[1];
var read = subStrings[2];
var kinesthetic = subStrings[3];
var visualBar = 30*visual;
var auralBar = 30*aural;
var readBar = 30*read;
var kinestheticBar = 30*kinesthetic;
document.write ('
My Vark Scores
');
document.writeln ('
');
document.writeln('('+visual+')
');
document.writeln('('+aural+')
');
document.writeln('('+read+')
');
document.writeln('('+kinesthetic+')
');{
}

Judging by your code, you're a beginner in JS (blame it on document.write :P). Anyways, here's some tips to save you early on your JS journey.

MONTHS = 'JANFEBMARAPRMAYJUNJULAUGSEPOCTNOVDEC'; //months

Declare variables with a var. That's the proper practice, and when you start using functions, you don't accidentally declare globals. In addition, MONTHS is a set of month abbreviations. You probably want to use an array. Lastly, we still retain the convention of constants as call capital letters, so credits to that.

var MONTHS = ['JAN','FEB','MAR', ... ];

document.write ('
Introduce Yourself

');

There are limited reasons to use document.write. The problem with it is that once the page is fully loaded, doing it will discard the entire page and the browser will write the content from a fresh blank page. This means its not suitable for after-page-creation operations.

Suggesting you look into DOM manipulation functions like document.createElement() and document.appendChild as well as document.querySelectorAll. This is as close to a library-like functionality as you get without ever using one.

userName = prompt('Enter your First Name','Alex'); //prompt user for name

While there's nothing against prompts, I suggest you explore more on forms and input elements. They are HTML elements that allow you to input data and have JS retrieve them.

var dateString = prompt('Enter your birth date in the form dd mmm yyyy','26 APR    1988'); //get users date of birth
var substrings = dateString.split(' ');
var day = substrings[0];
var month = substrings[1];
var year = substrings[2];

This will probably break if you don't check for the values. Have some form of validation to check if the values are valid.

var poem = ["Sunday's child is fair and wise", "Monday's child is fair of face",        "Tuesday's child is full of grace", "Wednesday's child is full of woe", "Thursday's child has far to go", "Friday's child is loving and giving", "Saturday's child works hard for a living"];
var daysArray = new Array ('Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday',    'Friday', 'Saturday');

You just declared poem using the literal notation, but not daysArray? Tsk3. Best practice is use literals whenever possible. Saves you keystrokes and looks less Java-ish.


Implementing Haskell's \\ (Set Difference) Learn You a Haskell presents the \\ function.

  \\ is the list difference function. It acts like a set difference,
  basically. For every element in the right-hand list, it removes a
  matching element in the left one.

Example:

ghci> [1..10] \\ [2,5,9]
[1,3,4,6,7,8,10]

Please critique my implementation.

setDifference :: (Eq a) => [a] -> [a] -> [a]
setDifference xs excludes = filter (not . (`elem` excludes)) xs

Test:

*Main> setDifference [1,2,3] [2,4,5,6]
[1,3]

I find that to be a very succinct, readable definition. I probably would have written it too. Unfortunately it's wrong!

One of the more tedious requirements of beautiful code is that it be correct, so let's set up some really quick testing using QuickCheck. Since you've been reimplementing functions from base QuickCheck can generate a bunch of random test data and throw it at both the original function and your reimplementation and verify that the outputs match.

We'll start by defining a property that says the output from setDifference and \\ should always be the same.

prop_equal :: (Eq a) => [a] -> [a] -> Bool
prop_equal xs ys = setDifference xs ys == xs \\ ys

We can use this property ourselves in an interactive session, but that gets tedious fast. Instead, call quickCheck with the property and it will generate arbitrary data to test it with.

  GHCi

> :m +Test.QuickCheck
> quickCheck prop_equal
*** Failed! Falsifiable (after 9 tests and 4 shrinks):
[(),(),(),(),(),()]
[()]

Aha! QuickCheck has determined that our property doesn't always hold. Each line printed after "Failed!" corresponds to an argument to the property. This might appear a little confusing due to the use of () (the unit or void type). Because our property was polymorphic, GHC picked a default. You can specialize the property to get output with other types, try quickCheck (prop_equal :: String -> String -> Bool). (You may have gotten slightly different output, that is okay and expected due to the generation of arbitrary data having been random.)

By itself this might not tell us enough though, why did these two arguments produce a failure? You can get all of the cases that QuickCheck tried by using verboseCheck instead.

  GHCi

> verboseCheck (prop_equal :: String -> String -> Bool)
... pages ...

This will print a whole pile of text with various passing and failing cases until it gives up trying to shrink the test case. Run verboseCheck a couple of times and eyeball the output, you should be able to notice that it always reduces the failure to something like the following.

Passed:
"aa"
"b"
Failed:
"aa"
"a"

If you explore these examples manually in the repl or simulate them by hand (or imagination) you'll see that setDifference removes every instance of the elements from excludes from xs instead of just the first as \\ does. Now armed with the fact that the implementation is wrong, and knowing what's wrong with it we can head back to the drawing board to fix setDifference.

For a full exploration of QuickCheck check out Real World Haskell's chapter on QuickCheck.


SHA256 password cracker - brute force I wrote a SHA256 password cracker. I initially used lists to store the information in list1.txt and list2.txt but I would end up getting memory overloads. So I now use files.
I noticed that when I made this transition, the programme speed reduced hugely. Is there a way to optimise the speed of this programming without running into memory overload issues?
Thanks
import hashlib
import time
import os

def extract_password(fname):
    if fname == 0:
        filename = "list2.txt"
    else:
        filename = "list1.txt"
    for line in open(filename, "r"):
        yield line.strip()

def save_password(fname, password):
    if fname == 0:
        filename = "list1.txt"
    else:
        filename = "list2.txt"

    file = open(filename, "a")
    file.write(password)
    file.write("\n")
    file.close()

def next_password(lsta, item,next_char, secure_password):
    found = 0
    guess = item + chr(next_char)
    if hash_password(guess) == secure_password:
        print(guess)
        found = 1
    save_password(lsta, guess)
    return found

def hash_password(pwrd):
    #Generates hash of original password
    pwrd = pwrd.encode("UTF-8")
    password = hashlib.sha256()
    password.update(pwrd)
    return password.hexdigest()

def delete_file(ltsa):
    try:
        if ltsa == 1:
            os.remove("list2.txt")
        else:
            os.remove("list1.txt")
    except:
        pass

def reset_file_status():
    try:
        os.remove("list2.txt")
        os.remove("list1.txt")
    except:
        pass

def find_password(secure_password):
    #Brute force to find original password
    found = 0
    lsta = 1
    for length in range(1, 15):
        if found == 1: break
        lsta = lsta^1
        delete_file(lsta)
        for next_char in range(65, 123):
            if found == 1: break
            if length == 1:
                found = next_password(lsta, "", next_char, secure_password)
                if found == 1: break
            else:
                for item in extract_password(lsta):
                    found = next_password(lsta, item, next_char, secure_password)
                    if found == 1: break

if __name__ == "__main__":
    reset_file_status()
    start = time.time()
    secure_password = hash_password("AAAA")
    find_password(secure_password)
    print(f"{(time.time() - start)} seconds")

The optimization you are looking for is very simple:
Ask your self - what do I really need to keep?
The answer: only the password guess I am currently checking.
There is no need to store all the old guesses, so there is no need for files or huge lists.
All you really need is one string you will keep updating.
To visualize this, think of your string as a growing number, with each letter being a base 58 digit.
Now. all you really need is to do a +1 on the first digit, checking for carry and updating following digits if needed just like usual addition.
Unfortunately, Python strings don't allow assignment by index, but they do support slicing.
Here is a function that would generate sequential passwords running through all the letters and growing the length as needed (one password per call!):
def make_next_guess(guess):
    carry = 1
    next_guess = guess

    for i in range(len(guess)):
        cur_char = ord(guess[i]) + carry
        if cur_char > ord('z'):
            cur_char = ord('A')
            carry = 1
        else:
            carry = 0

        next_guess = next_guess[:i] + chr(cur_char) + guess[i + 1:]
        if carry == 0:
             break

    if carry = 1:
        next_guess += 'A'

    return next_guess

With this you can use one loop for all the possibilities up to the maximum length:
guess = 'A'

for _ in range(58 ** 14): #password maximum length 14 and there are 58 characters that can be used

    if hash_password(guess) == secure_password:
        print(guess)
        break

    guess = make_next_guess(guess)


FizzBuzz in Commodore Basic 1 REM
2 REM FIZZBUZZ IN COMMODORE BASIC
3 REM
10 FOR I = 1 TO 100
20 IF (I/3)=INT(I/3) THEN PRINT "FIZZ";
30 IF (I/5)=INT(I/5) THEN PRINT "BUZZ";
40 IF (I/3)<>INT(I/3) AND (I/5)<>INT(I/5) THEN PRINT I;
50 PRINT
60 NEXT I

I'm not happy with the tests for "multiple of 3" and "multiple of 5".  Using floating-point arithmetic to determine integer multipleness feels highly inelegant, but I don't believe Commodore Basic has "integer mod" functionality, and I can't think of another similarly-compact way to determine that one integer is a multiple of another.
When run using the VICE emulator, it takes almost nine seconds to execute.  Besides getting rid of the floating-point division, are there any obvious ways to speed things up?

As Jerry explained, all math operations treat numbers as floating point... even numbers stored as integers. This was my first attempt:

1 rem
2 rem fizzbuzz in commodore basic
3 rem
10 t = time
20 for i = 1 to 100
30 n = i / 15 :if n = int(n) then print "fizzbuzz" :goto 70
40 n = i / 3 :if n = int(n) then print "fizz" :goto 70
50 n = i / 5 :if n = int(n) then print "buzz" :goto 70
60 print i
70 next
80 print "ran for" (time - t) / 60 "seconds"

Skipping the rest of the conditions when a condition passes speeds it up significantly.
Assigning the value of X/Y to a variable and reusing it is faster than calculating it twice. Some places around the internet mention the formula X-INT(X/Y)*Y, but that was slower than N=INT(N) in my tests.
According to this article you can write your own modulo "operator" in assembly and use it in BASIC. The assembly program is available here. Seems like overkill to me.

You might think that you could replace the goto 70 with next, but it doesn't quite work. Once the loop is complete, it will go to the next line, and call the next on that line, and you'll get a "next without for" error. Technically it still does what it's supposed to do, but it doesn't exit cleanly.

You could do something like this as a compromise, but it's hackish:

1 rem
2 rem fizzbuzz in commodore basic
3 rem
10 t = time
20 for i = 1 to 100
30 n = i / 15 :if n = int(n) then print "fizzbuzz" :next
40 n = i / 3 :if n = int(n) then print "fizz" :next
50 n = i / 5 :if n = int(n) then print "buzz" :next :goto 70
60 print i: next
70 print "ran for" (time - t) / 60 "seconds"

Edward had a good idea: keep separate counters for fizz and buzz instead of doing division. Here's a variation on his approach that sacrifices speed for readability. It doesn't use goto and is about as readable as the original code in the question. It's faster than my examples above, but slower than Edward's solution (since it doesn't skip other conditions after passing one, and doesn't have a combined "fizzbuzz" case).

1 rem
2 rem fizzbuzz in commodore basic
3 rem
10 t = time
20 fizz = 0 :buzz = 0
30 for i = 1 to 100
40 fizz = fizz + 1 :buzz = buzz + 1
50 print
60 if fizz = 3 then print "fizz"; :fizz = 0
70 if buzz = 5 then print "buzz"; :buzz = 0
80 if fizz > 0 and buzz > 0 then print i;
90 next
100 print ,"ran for" (time - t) / 60 "seconds"

Incidentally, the biggest performance bottleneck seems to be printing carriage returns and moving everything up the screen. If you try the original code in the question without printing any carriage returns, it will run about three times faster.

Assuming the screen has 40 columns by 25 rows, it's just big enough to fit the fizzbuzz output neatly into columns without scrolling. This version does just that, shaving off about a second from the previous version.

1 rem
2 rem fizzbuzz in commodore basic
3 rem
10 t = time
15 print chr$(147);
20 fizz = 0 :buzz = 0 :row = 0 :col = 0
30 for i = 1 to 100
40 fizz = fizz + 1 :buzz = buzz + 1
50 row = row + 1
60 poke 211, col
70 if fizz = 3 then print "fizz"; :fizz = 0
80 if buzz = 5 then print "buzz"; :buzz = 0
90 if fizz > 0 and buzz > 0 then print i;
100 if row = 25 then print chr$(19); :col = col + 10 :row = 0
110 if row > 0 then print
120 next
130 secs = (time - t) / 60
140 poke 198, 0 :wait 198, 1
150 print chr$(147) "ran for" secs "seconds"

Notes:

chr$(147) is CLR, a form feed character which clears the screen.
poke 211, col manipulates system memory at the address where the cursor column is stored.
chr$(19) is HOME; it moves the cursor to the top row.
poke 198, 0 :wait 198, 1 waits for the user to press a key before continuing, so the timing message and READY prompt don't overwrite any of the output at the end.

Here's what the output from the final version looks like:


Expressing multiple AND conditions in ruby In ruby, is there a more concise way of expressing multiple AND conditions in an if statement?

For example this is the code I have:

    if first_name.blank? and last_name.blank? and email.blank? and phone.blank?
      #do something
    end

Is there a better way to express this in Ruby? 1.9.2 or 1.9.3

if [first_name, last_name, email, phone].all?(&:blank?)
  #do something
end

Caveat: While and/&& short-circuit the expression (that's it, only the needed operands are evaluated), an array evaluates all its items in advance. In your case it does not seem something to worry about (they seem cheap attributes to get), but if you ever need lazy evaluation and still want to use this approach, it's possible using procs, just slightly more verbose:

p = proc
if [p{first_name}, p{last_name}, p{email}, p{phone}].all? { |p| p.call.blank? }
  #do something
end