Direction Array: reducing complexity I have an m*n matrix where every element is unique. From a given starting point I have to move to the smallest point in a relative direction (e.g. up, down, left, right) and then have to repeat the process again. When all other surrounding points have a value that is greater than the existing one I have to stop and print the position from start. Suppose I have a 5x5 matrix:

1  2  3  4  5
6  7  8  9  10
11 12 13 14 15
16 17 18 19 20
21 22 23 24 25

and starting point is (2,2) then the output will be 13,8,3,2,1.
I have solved this problem my way, but the problem is its complexity. I do not think my solution is efficient. Can anyone suggest to me a better solution?

N.B: Except scanner pkg, I am not allowed to import any other pkg. Here is my code:

import java.util.Arrays;
import java.util.Scanner;
public class DirectionArray {
public static void main(String [] args) {
        Scanner in = new Scanner(System.in);
        int row = in.nextInt();
        int col = in.nextInt();
        int[][] ara = new int[row][col];
        int[] ara2 = new int[4];
        for (int i = 0; i < row; i++) {
            for (int j = 0; j < col; j++) {
                ara[i][j] = in.nextInt();
            }
        }
        System.out.println("Give starting point(x) ");
        int x= in.nextInt();
        System.out.println("Give starting point(y) ");
        int y= in.nextInt();
        int sx=x;
        int sy =y;
        int [] fx={+1,-1,0,0};
        int [] fy={0,0,+1,-1};
        int p=0;
        int l=0;
        int v=0;
        int r=0;
        int [] result=new int[row*col] ;
        int min=ara[x][y];
        boolean swap=true;
        for(int i=0;i<(row*col)-1;i++) {
            for (int k = 0; k < 4; k++) {
                int nx = x + fx[k];
                int ny = y + fy[k];

                if (nx >= 0 && nx < row && ny >= 0 && ny < col) {
                    if (min > ara[nx][ny]) {

                        ara2[p] = ara[nx][ny];
                        p++;
                        // x = nx;
                        //y = ny;
                    }

                }

            }
            p=0;
            while(swap) {
                swap=false;
                r++;

                for (int q = 0; q < ara2.length-r; q++) {
                    if(ara2[q]>ara2[q+1]){
                    int temp = ara2[q];
                        ara2[q]=ara2[q+1];
                        ara2[q+1]=temp;
                        swap=true;
                    }
                }
            }
           // Arrays.sort(ara2);

                for(int j=0;j<ara2.length;j++) {
                    if(ara2[j]!=0)
                    {
                        v=ara2[j];
                        result[l]=v;
                        l++;
                        break;
                    }
                }
               // System.out.println(v);
                min=v;
            for(int o=0;o<ara2.length;o++) {
                ara2[o]=0;
            }
                for(int m=0;m<row;m++){
                    for(int n=0;n<col;n++){
                        if(ara[m][n]==v) {
                            x = m;
                            y = n;
                        }
                    }
                }

        }

        System.out.print(ara[sx][sy]+" ");
        for(int i=0;i<result.length;i++) {
            if(result[i]!=0) {
                System.out.print(result[i] + " ");
            }
            if(result[i]==0) {
                break;
            }
        }
    }
}

Thanks for sharing your code!
Naming
Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.
avoid single character names
Since the number of characters is quite limited in most languages you will soon run out of names. This means that you either have to choose another character which is not so obviously connected to the purpose of the variable. And/or you have to "reuse" variable names in different contexts. Both makes your code hard to read and understand for other persons. (keep in mind that you are that other person yourself if you look at your code in a few month!)
On the other hand in Java the length of identifier names is virtually unlimited. There is no penalty in any way for long identifier names. So don't be stingy with letters when choosing names.
avoid abbreviations
In your code you use some abbreviations such as the variable ara. Although this abbreviation makes sense to you (now) anyone reading your code being not familiar with the problem (like me) has a hard time finding out what this means.
If you do this to save typing work: remember that you way more often read your code than actually typing something. Also for Java you have good IDE support with code completion so that you most likely type a long identifier only once and later on select it from the IDEs code completion proposals.
OOP
Your code is a procedural approach to the problem.
There is nothing wrong with procedural approaches in general, but Java is an object oriented (OO) programming language and if you want to become a good Java programmer then you should start solving problems in an OO way.
But OOP doesn't mean to "split up" code into random classes.
The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.
Doing OOP means that you follow certain principles which are (among others):

information hiding / encapsulation
single responsibility / separation of concerns
same level of abstraction
KISS (Keep it simple (and) stupid.)
DRY (Don't repeat yourself.)
"Tell! Don't ask."
Law of Demeter ("Don't talk to strangers!")

Here is a OO-ish approach to the problem.
The idea is that a "cell" (a position in an the array) "knows" its neighbors. This the solution does not need to iterate over the whole array multiple times. My approach need to visit only 4 cells per iteration. This I have 90% less compares to do compared to your procedural approach. The ratio gets better the bigger the array gets...
I introduced two custom classes:

a Cell class that does the actual search logic recursively.
a StartPosition class as a Data Transfer Object (DTO) which is needed to enable a method to return both, x and y coordinate of the starting point.

I ignored your restriction to only use Scanner as the only allowed import because it is somewhat overstated.
import java.io.PrintStream; // implicit dependency from System.out.
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.Scanner;

public class DirectionArray {
    class Cell {
        private final int value;
        private final TreeSet<Cell> neighborsSortedByValue = new TreeSet<>((c1, c2) -> c1.value - c2.value);

        public Cell(int value) {
            this.value = value;
        }

        public void addNeighbor(Cell neighbor) {
            neighborsSortedByValue.add(neighbor);
        }

        public void findSink(Collection<Cell> pathToSink) {
            pathToSink.add(this);
            Cell minNeighbor = neighborsSortedByValue.first();
            if (minNeighbor.value < this.value)
                // recursive call if lesser value neighbor exists
                minNeighbor.findSink(pathToSink);
        }

        @Override
        public String toString() {
            return value + " ";
        }

    }

    class StartPosition {
        private int x;
        private int y;
    }

    public static void main(String[] args) {
        Scanner in = new Scanner(System.in);
        PrintStream out = System.out;
        new DirectionArray().runToSink(in, out);
    }

    void runToSink(Scanner in, PrintStream out) {
        Cell[][] cellArray = readArray(in);
        setNeighbors(cellArray);

        StartPosition startPosition = askForStartPosition(in, out);

        Collection<Cell> pathToSink = new ArrayList<>();
        cellArray[startPosition.x][startPosition.y].findSink(pathToSink);
        print(pathToSink, out);
    }

    private Cell[][] readArray(Scanner in) {
        int row = in.nextInt();
        int col = in.nextInt();
        Cell[][] cellArray = new Cell[row][col];
        for (int i = 0; i < row; i++) {
            for (int j = 0; j < col; j++) {
                cellArray[i][j] = new Cell(in.nextInt());
            }
        }
        return cellArray;
    }

    private void setNeighbors(Cell[][] cellArray) {
        for (int i = 0; i < cellArray.length; i++) {
            for (int j = 0; j < cellArray[i].length; j++) {
                setNeighborsToCell(i, j, cellArray);
            }
        }
    }

    private void setNeighborsToCell(int i, int j, Cell[][] cellArray) {
        for (int x : new int[] { -1, 1 }) { // one force, one back
            // neighbors in column
            cellArray[i][j].addNeighbor(cellArray[calculateNeighborIndex(i, x, cellArray.length)][j]);
            // neighbors in row
            cellArray[i][j].addNeighbor(cellArray[i][calculateNeighborIndex(j, cellArray[i].length, x)]);
        }
    }

    private int calculateNeighborIndex(int i, int x, int length) {
        return (length + i + x) % length;
    }

    private StartPosition askForStartPosition(Scanner in, PrintStream out) {
        StartPosition startPosition = new StartPosition();
        out.println("Give starting point(x) ");
        startPosition.x = in.nextInt();
        out.println("Give starting point(y) ");
        startPosition.y = in.nextInt();
        return startPosition;
    }

    private void print(Collection<Cell> pathToSink, PrintStream out) {
        for (Cell pathStep : pathToSink) {
            out.print(pathStep);
        }
    }
}

Finally I created a test driver a class that runs the solution with your test data. This test class is run by the JUnit framework:
import static org.hamcrest.CoreMatchers.endsWith;
import static org.junit.Assert.*;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.util.Scanner;

import org.junit.Test;

public class DirectionArrayTest {

    @Test
    public void test() {
        Scanner scanner = new Scanner(new ByteArrayInputStream(
                ("5 5\n" + // array dimensions
                 "1  2  3  4  5\n" +
                 "6  7  8  9  10\n" +
                 "11 12 13 14 15\n" +
                 "16 17 18 19 20\n" +
                 "21 22 23 24 25\n" +
                 "2 2\n" // start position index
                 ).getBytes()));

        ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
        PrintStream out = new PrintStream(byteArrayOutputStream);

        new DirectionArray().runToSink(scanner, out);

        System.out.println(byteArrayOutputStream.toString());
        assertThat(byteArrayOutputStream.toString(), endsWith("13 8 3 1 "));
    }

}


Sieve of Eratosthenes For practice, I've implemented Sieve of Eratosthenes in Java by thinking about this visual animation.

I would like to hear some suggestions for improvements, especially how to make it more efficient and readable (sometimes there are trade offs) and how to follow good Java conventions. Also, suggestions on issues from naming conventions to proper javadoc are welcome as well. I'm just trying to improve my programming skills.

Incidently, the sieveOfEratosthenes method below is big O(?).

/**
 * returns a boolean array for prime numbers
 * @param max the maximum value to search for primes
 * @return boolean array of integers with primes true
 */
public boolean[] sieveOfEratosthenes(int max){

    boolean[] primeCandidates = new boolean[max]; //defaults to false
    for(int i=2; i<max; i++ ){primeCandidates[i]=true;}

    for(int i=2; i<Math.sqrt(max);i++){
        if(primeCandidates[i] == true){
            //all multiples of i*i, except i, are not primeCandidates
            for(int j = i + i; j<max; j=j+i){
                primeCandidates[j]=false;
            }
        }

    }
    return primeCandidates;
}

/**
 * print the index number for all true in given array
 * @param a boolean array
 */
public void printTrue(boolean[] arr){
    for(int i=0; i<arr.length; i++){
        if(arr[i]==true){
            System.out.print(i + ", ");
        }
    }
}

Input checking is missing. You should throw an IllegalArgumentException if max is less than zero instead of the NegativeArraySizeException.

Result of Math.sqrt(max) doesn't change, so maybe it's would worth to extract out to a local variable but maybe the JVM already optimize this for you.

Anyway, it's fine. I made some renaming and formatting, here is the result:

public boolean[] sieveOfEratosthenes(final int max) {
    if (max < 0) {
        throw new IllegalArgumentException("max cannot be less than zero: " + max);
    }
    final boolean[] primeCandidates = new boolean[max]; // defaults to false
    for (int i = 2; i < max; i++) {
        primeCandidates[i] = true;
    }

    final double maxRoot = Math.sqrt(max);
    for (int candidate = 2; candidate < maxRoot; candidate++) {
        if (primeCandidates[candidate]) {
            for (int j = 2 * candidate; j < max; j += candidate) {
                primeCandidates[j] = false;
            }
        }

    }
    return primeCandidates;
}


Thread worker simulator Because I have been writing more multithreaded C++14 code lately, I've developed some tools that help me make sure that my threads are all working as intended.  In the real code, it's often difficult to detect race conditions or unintended sequence dependencies among threads, so I have found that temporarily substituting a work simulator (worksim in the code) for the real thread code, leaving any other locks, mutexes, etc. in place, allows me to gain some insight as to how the various threads might run.

There are three main pieces in the code below.  The first is the Logger class which is more or less a mutex-protected interface to std::cout.  It takes a std::stringstream & as input, emits the contents of it to std::cout and the clears the input stringstream.

The second piece is the worksim itself which employs a binomial random number distribution to generate delays that are typically around 25ms, emitting status to the passed Logger object until the stop flag is set.  I deliberately do not want to use a conditional variable here because I want to simulate the work that the thread would do.

The third piece is a simple main showing how these are used.  It creates 10 threads, waits five seconds and then shuts everything down.

I'm interested in comments on the worksim portion in particular.  Specifically,

is the intent clear?
is the use of a static number generator OK here?
is there a better way to handle the logging?

threadplay.cpp

#include <iostream>
#include <atomic>
#include <mutex>
#include <thread>
#include <random>
#include <chrono>
#include <vector>
#include <sstream>

class Logger {
public:
    Logger &operator<<(std::stringstream &ss) {
        std::lock_guard<std::mutex> lock(m);
        std::cout << ss.str();
        ss.str(std::string{});
        return *this;
    }
private:
    std::mutex m;
};

void worksim(Logger &mylog, std::atomic<bool> &stop, int n) {
    using namespace std::chrono_literals;
    static std::mt19937 gen(std::random_device{}());
    std::binomial_distribution<int> dist(50, 0.5);
    std::stringstream ss;
    ss << "Checking " << n << "\n";
    mylog << ss;
    for (auto delay = dist(gen); !stop; delay = dist(gen)) {
        ss << "Thread " << n << " sleeping for " << delay << "ms\n";
        mylog << ss;
        std::this_thread::sleep_for(1ms * delay);
    }
    ss << "Leaving " << n << "\n";
    mylog << ss;
}

int main()
{
    using namespace std::chrono_literals;
    std::atomic<bool> stop{false};
    std::vector<std::thread> th;
    Logger mylog;
    for (int i=0; i<10; ++i) {
        th.push_back(std::thread(worksim, std::ref(mylog), std::ref(stop), i));
    }
    std::this_thread::sleep_for(5s);
    stop = true;
    for (std::thread &thread : th) {
        thread.join();
    }
}

1. Is the intent clear?

Not completely for me. As you're saying

  for the real thread code, leaving any other locks, mutexes, etc. in place, allows me to gain some insight as to how the various threads might run.

the closest intent I can grasp is, you want to eliminate certain threads by replacing them with worksim threads, to isolate other threads that might be the source of race conditions, dead locks or other multiple threading related problems in the to be tested code.

I'm not a 100% sure if these simplified threads may be really useful for diagnosis of the above mentioned problems. Especially as they won't affect other threads, other than with a random delay and don't touch those synchronization mechanisms left in place. To some degree may be.

2. Is the use of a static number generator OK here?

The instantiation of static local variables will be thread safe. Execution of operations like dist(gen)) will not guarantee thread safety.

The reference documentation about std::binomial_distribution::operator() doesn't tell anything about thread safety, and because gen is a shared resource between threads I would doubt about appropriate results.

3. is there a better way to handle the logging?

The standard I/O output can be fairly slow, and as I mentioned in my comment "The use of a shared logger with the mutex locking may severely influence your threads' runtime behavior".

A generally better approach might be to use the shortest possible lock to pass the log message to the logger (e.g. using a protected queue of std::unique_ptr's), and let the Logger class output these from another background thread (that's what we're using in our production systems).


Socket HTTP post request I am working on a socket HTTP post request.  Here is one method that I come up with.  It looks dirty and is in C instead of C++.

bool SendBasicInfo(USER* user, SOCKET sock)
{
    char FormBuffer[1024];

    char DataType1[] = "a=";
    char DataType2[] = "&b=";
    char DataType3[] = "&c=";

    char FormAction[] = "/g/g.php";

    // get: length of the actual content
    int ContentLength = (int)strlen(user->user_id) + (int)strlen(user->pcname) + (int)strlen(user->username) + strlen(DataType1) + strlen(DataType2) + strlen(DataType3);
    char ContentLengthBuffer[sizeof(ContentLength)];
    sprintf_s(ContentLengthBuffer, "%d", ContentLength);

    // header
    strcpy_s(FormBuffer, "POST ");
    strcat_s(FormBuffer, FormAction);
    strcat_s(FormBuffer, " HTTP/1.1\n");
    strcat_s(FormBuffer, "Content-Type: application/x-www-form-urlencoded\n");
    strcat_s(FormBuffer, "Host: \n");
    strcat_s(FormBuffer, "Content-Length: ");
    strcat_s(FormBuffer, ContentLengthBuffer); // size of actual content
    strcat_s(FormBuffer, "\n\n");

    // actual content
    strcat_s(FormBuffer, DataType1);
    strcat_s(FormBuffer, user->user_id);
    strcat_s(FormBuffer, DataType2);
    strcat_s(FormBuffer, user->pcname);
    strcat_s(FormBuffer, DataType3);
    strcat_s(FormBuffer, user->username);

    std::cout << FormBuffer << std::endl;

    return ( send(sock, FormBuffer, strlen(FormBuffer), NULL) >= (int)strlen(FormBuffer) );
}

The idea here is to send a message like this one:

POST /g/g.php HTTP/1.1
Content-Type: application/x-www-form-urlencoded
Host:
Content-Length: 31

a=123&b=computername&c=username

This was working fine, but it just seems hard-coded and wrong.  How can I make the size of FormBuffer to be dynamic instead of 1024?

This was working fine, but it just seems hard-coded and wrong.

It's a monolythic monster. It works, but if you have the time, you could benefit from re-writing it (using c++ alternatives).

  How can I make the size of FormBuffer to be dynamic instead of 1024?

Like so (C++):

bool SendBasicInfo(USER* user, SOCKET sock)
{
    std::ostringstream FormBuffer; // <<< here

    char DataType1[] = "a=";
    char DataType2[] = "&b=";
    char DataType3[] = "&c=";

    char FormAction[] = "/g/g.php";

    // get: length of the actual content
    auto ContentLength = strlen(user->user_id) +
        strlen(user->pcname) +
        strlen(user->username) +
        strlen(DataType1) +
        strlen(DataType2) +
        strlen(DataType3);

    // header
    FormBuffer << "POST " << FormAction << " HTTP/1.1\n";
    FormBuffer << "Content-Type: application/x-www-form-urlencoded\n";
    FormBuffer << "Host: \n";
    FormBuffer << "Content-Length: " << std::to_string(ContentLength) << "\n\n";

    // actual content
    FormBuffer << DataType1 << user->user_id;
    FormBuffer << DataType2 << user->pcname;
    FormBuffer << DataType3 << user->username;

    const auto str = FormBuffer.str();

    std::cout << str << std::endl;

    return ( send(sock, str.data(), str.size(), NULL) >= str.size() );
}

Otherwise (if you want to write C code), you will have to iterate over the entire data once and compute it's resulting size, then allocate memory for the result, then iterate again over the data to create the buffer contents, then sent to socket and remember the result in a variable, then free the allocated memory, then deturn the stored result. If it sounds ugly, that's because it is :D.

Edit: The HTTP standard states that you should send \r\n at the end of header lines, not \n. It may not matter much in effect though, because both servers and clients are mandated to receive and parse both variants - as developers tend not to respect this rule (I may be remembering it wrong though).


Simple integer range for C++11 range-based for loops I'm really tired of having to type

for (int iSomething = rangeBegin; iSomething < rangeEnd; ++iSomething)
{
   ...
}

whenever I want to iterate over an integer range (most IDEs help with the typing, but still it looks so verbose, naming the integer 3 times!)

I wanted something like this:

for (int iSomething : LoopRange(rangeBegin, rangeEnd))
{
   ...
}

Or if rangeBegin is 0 (the majority of the cases) then a simple

for (int iSomething : LoopRange(rangeEnd))
{
   ...
}

My very simple implementation:

class LoopRangeIterator
{
public:
    LoopRangeIterator(int value_)
        : value(value_){}

    bool operator!=(LoopRangeIterator const& other) const
    {
        return value != other.value;
    }

    int const& operator*() const
    {
        return value;
    }

    LoopRangeIterator& operator++()
    {
        ++value;
        return *this;
    }

private:
    int value;
};

class LoopRange
{
public:
    LoopRange(int from_, int to_)
        : from(from_), to(to_){}

    LoopRange(int to_)
        : from(0), to(to_){}

    LoopRangeIterator begin() const
    {
        return LoopRangeIterator(from);
    }

    LoopRangeIterator end() const
    {
        return LoopRangeIterator(to);
    }

private:
    int const from;
    int const to;
};

I named it LoopRange to make it clear that it's for loops and it isn't some general integer range class that you would use for intersecting or building union etc.

Of course this class could be generalized in many ways, but I think if you need more complex functionality (e.g. custom step sizes, double values), then you are doing something special and you are better off writing the explicit for loop.

What do you think about it?
If I use such a thing throughout my project, would it confuse and disturb/distract people too much compared to just using the classic and verbose for(...; ...; ...) style?

You can make the class template with trivial changes (add template<typename T> and change int by T in your classes), then make a construction function that deduces integer types:

template<typename T>
LoopRange<T> range(T from, T to)
{
    static_assert(std::is_integral<T>::value,
                  "range only accepts integral values");

    return { from, to };
}

That will even allow you to explicitly tell what kind of integer you want to loop with if needed:

for (auto i: range<unsigned>(0, 5))
{
    std::cout << i << " ";
}

If you need to generate indices to iterate through a std::vector, this can be useful since std::vector<T>::size_type is probably bigger than int. While the static_assert avoids some potential problems with floating point values, it also inhibits the use of integer-like classes (for example, a hypothetical BigNum class).

You can simplify some of your functions thanks to list initialization. For example, used in a return statement, it sallows you not to explicitly repeat the return type (unless the return type's constructor is explicit):

LoopRangeIterator begin() const
{
    return { from };
}

LoopRangeIterator end() const
{
    return { to };
}

On a side note, such a range utility would also be interesting if it worked with floating point numbers, and maybe decimal numbers in the future (akin to Python's numpy.arange). However, you would have to special-case the class for those types if you want to avoid problems: if you repeatedly add the same floating point (say 0.01), you will accumulate rounding errors. Computing every value from the base value with a multiplication could be away to circumvent such a problem.


Printing characters in a matrix as a string I need to solve this problem in Python 3 (within 3 sec):

  A is a given (NxM) rectangle, filled with characters "a" to "z".
  Start from A[1][1] to A[N][M] collect all character which is only
  one in its row and column, print them as a string.

  Input:

  \$N\$, \$M\$ in first line (number of row and column \$1 \le N\$, \$M \le
> 1000\$). Next, \$N\$ lines contain exactly \$M\$ characters.

  Output:

A single string

  Sample input 1:

1 9
arigatodl

  Sample output 1:

rigtodl

  Sample input 2:

5 6
cabboa
kiltik
rdetra
kelrek
dmcdnc

  Sample output 2:

codermn

This is still not fast enough when \$N\$, \$M\$ = 1000. I'd like suggestions on improving the speed, or any other ways to solve the given problem, as long as the solution is in Python 3 and is faster than mine.

from operator import itemgetter
Words,Chars,answer=[],"abcdefghijklmnopqrstuvwxyz",""
N,M=[int(i) for i in input().split()]
for _ in range(N):
    Words.append(input())
                                    # got the inputs
for row,word in enumerate(Words):   # going through each words
    Doubts=[]                       # collect chars only one in its row.
    for char in Chars:
            if (word.count(char)==1):
                    Doubts.append((char,word.index(char)))
    for case in sorted(Doubts,key=itemgetter(1)):   #sorting by index
            doubtless=True                  #checking whether 1 in its column or not.
            for i in range(N):
                    if (Words[i][case[1]]==case[0] and i!=row):
                            doubtless=False
                            break
            if (doubtless):
                    answer+=case[0] #if char is one in its row and column, adds to answer.
print (answer)

I would suggest to create a 1000x1000 testcase and measure the performance before the optimization process. Please use the following test generator:

import os
import random

f = open('1000x1000.in','w')
f.write('1000 1000\n')
ALPHABET = 'abcdefghijklmnopqrstuvwxyz'
for _ in range(1000):
       f.write(''.join([ALPHABET[random.randint(0, len(ALPHABET)-1)] for _ in range(1000)]) + '\n')
f.close()

After that just run it and measure the performance of your application. I got about 47 ms on my pretty old C2D E7200.


Display AJAX response from PHP I have used jQuery AJAX for my development. Below I have added example code for AJAX request and response.

ajax.php

$queView = array();

$queView['time'] = 40;
$queView['name'] = "Level hard";
$queView ['noQuestions'] = 10;
$queView['queDetails'] .= "
Some html content
";
$queView['queDetails'] .= "";

foreach($users as $user){
   $queView['queDetails'] .= "
".$user['name']."
";
   $queView['queDetails'] .=  "
".$user['address']."
";
}

echo json_encode($queView);

JavaScript function with jQuery;

function showQuestions(){

 $.ajax({
        data:{showQues:'1',attemptedQue:queId,selAnswer:checkedAns,timeTaken:timeTaken},
        url:'include/ajaxAttempt.php',
        success: function(data){

            data = $.parseJSON(data);

            $('#time').html(data.time);
            $('#name').html(data.name);
            $('#noQuesitons').html(data.noQuestions);
            $('#queDetails').html(data.queDetails);
        },

        error:function(xhr,err){
            alert("Error");
        }
    });
}

$queView['queDetails'] has a lot of HTML content.

Is this a good practice?

If you're passing information, pass only information, not markup. Alternatively completely generate the HTML markup on the server-side, and just apply it on the client side. There's no point in combining the two, and if done wrong you could cause double escaping issues.

Second, don't use alert() as a debug function. Use console.log() and use your console.


Create release package from git tag with Powershell In my deployment I need to generate a zip file from a git repo. I have the basic work done with how to pull the latest tag and create the zip file. I ended wanting to update the files that are pulled down to have the last modified of the last commit instead of when I just checked them out.

# Set-ExecutionPolicy -ExecutionPolicy RemoteSigned -Scope CurrentUser
param(
    [string] $tag = "",
    [string] $gitURI = "https://github.com/jquery/jquery.git",
    [string] $checkForNewTagsOnly = "Y" #if yes then it will exit if the folder already exists
)
$Currentlocation = "C:\_Builds"
New-Item -ItemType Directory -Force -Path $Currentlocation

Set-Location $Currentlocation
if( $checkForNewTagsOnly -eq "Y"){
    git clone $gitURI --depth 1 master --single-branch
    Set-Location $Currentlocation\master
    git fetch --tags
    $tag = git describe --tags $(git rev-list --tags --max-count=1)
    $tag = $tag|split-path -leaf
}

if([string]::IsNullOrWhiteSpace($tag)){($tag = Read-Host "Enter the tag")}

if( $checkForNewTagsOnly -eq "Y" -and (Test-path $Currentlocation\$tag)){
    Write-Host "$tag already exists, nothing to do"
    exit
}

New-Item -ItemType Directory -Force -Path $Currentlocation
If (Test-path $Currentlocation\$tag) {Remove-item $Currentlocation\$tag}

Set-Location $Currentlocation
git clone $gitURI --recurse-submodules --single-branch --shallow-submodules --branch $tag $tag
Set-Location $Currentlocation\$tag

Write-Host "$tag checked out, running clean up on $Currentlocation"
Get-ChildItem -filter *test* -Directory -recurse -force | remove-item -force -recurse

$ToDelete = "*.gitignore","*.gitattributes","*.gitmodules"
FOREACH ($Item IN $ToDelete) {
    FOREACH ($File IN (Get-ChildItem -recurse | Where-Object {$_.name -like "$Item"})) {
        $FullName = $File.FullName
        Write-Host "Deleting $FullName" -ForegroundColor Green
        Remove-Item $File.FullName
    }
}

Write-Host "fixing the file dates, since git doesn't keep the dates, let's update it the last commit date"
FOREACH ($File IN (Get-ChildItem -recurse)) {
    $FullName = $File.FullName
    $lastModifiedDate = git log -1 --date=short --format=%cd "$FullName"
    $File.LastWriteTime = $lastModifiedDate
    $File.CreationTime = $lastModifiedDate
}

Get-ChildItem -Include .git -recurse -force | remove-item -force -recurse

Write-Host "$tag cleaned, creating zip file"
If (Test-path $Currentlocation\$tag.zip) {Remove-item $Currentlocation\$tag.zip}

Compress-Archive -Path $Currentlocation\$tag -DestinationPath $Currentlocation\$tag.zip

Write-Host "Good Job, the package is created $tag.zip"

You should put [CmdletBinding()] before the param block. This gives you some nice features.

I changed $checkForNewTagsOnly to be of type switch rather than string. You can call the script like this MyScript -checkForNewTagsOnly or MyScript -checkForNewTagsOnly:$false. I think that is better because otherwise how are people supposed to know to supply a "Y" as a parameter?

I made the tag parameter mandatory (Mandatory=$true) so that you won't need to check for it later.

By the way, I think you should start these parameters start with a capital letter. That is the convention.

[CmdletBinding()]
param(
    [Parameter(Mandatory=$true, HelpMessage="Enter the tag.")]
    [string] $tag,
    [string] $gitURI = "https://github.com/jquery/jquery.git",
    [switch] $checkForNewTagsOnly = $true #if yes then it will exit if the folder already exists
)

We use $checkForNewTagsOnly like this now that it is a boolean:

if ($checkForNewTagsOnly) {
    # ...
}

We can get rid of this line altogether now that tag is mandatory:

if([string]::IsNullOrWhiteSpace($tag)){($tag = Read-Host "Enter the tag")}

It's generally better to use Write-Output rather than Write-Host because you can't pipe Write-Host output. You couldn't for instance write it to a log.

Write-Host "$tag checked out, running clean up on $Currentlocation"

Please be consistent with your formatting. Why do you have FOREACH in caps? I find the inconsistency of your formatting in general to be extremely triggering to my OCD. It's best to stick to conventional formatting. That makes it easier for people to read.

FOREACH ($Item IN $ToDelete) {
    FOREACH ($File IN (Get-ChildItem -recurse | Where-Object {$_.name -like "$Item"})) {

-Filter is faster than -Include. I don't know if that's an issue here or not, but it's something to be aware of. You used -Filter earlier, by the way.

Get-ChildItem -Include .git -recurse -force | remove-item -force -recurse


Perform basic string compression using the counts of repeated characters I managed to implement this question from Cracking the Coding Interview:

  Implement a method to perform basic string compression using the counts
  of repeated characters. For example, the string aabcccccaaa would become
  a2b1c5a3. If the "compressed" string would not become smaller than the original string, your method should return the original string.

Can anyone offer feedback on my code?

public static String countRepeatedChars(String str){
    int initialLength = str.length();
    char[] chars = str.toCharArray();
    int occurances = 0;
    String compressedString = "";
    char currentChar = chars[0];

    for(int i = 0; i < initialLength; i++){
        char c  = chars[i];
        if(currentChar == c && i + 1 <= initialLength){
            occurances++;
        }

        else if (currentChar != c || i + 1  > initialLength){

            compressedString += currentChar + String.valueOf(occurances);
            currentChar = chars[i];
            occurances = 1;
        }

        if(i + 1 >=  initialLength){
            compressedString += currentChar + String.valueOf(occurances);

        }
    }
    if(compressedString.length() < initialLength){
         return compressedString;
    }
    return str;
}

Interviewing techniques and design decisions

If you were asked a question like this in an interview, always ask your interviewer for clarification.  Not only does it avoid misunderstandings, it also demonstrates your ability to identify corner cases.  You can score points before writing a single line of code!

In this case, the question I would ask is: how should a streak of 10 or more identical characters be encoded?  An encoded string that always alternates character, count, character, count etc. would be easy to decode.  However, a two-digit count would break the pattern and make it harder to decode.  Indeed, the interpretation could even be ambiguous.  For example, does the encoded string a101c9 mean aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaccccccccc or a0ccccccccc?  I would argue that it should be interpreted as the latter, since

otherwise there would be no good way to compress a0ccccccccc, and
we could preserve the nice character-count-character-count alternation, which is a useful property for decoding.

If we make that design decision, then, it seems to follow that a string consisting of a repeated 101 times followed by c repeated 9 times would have to be encoded as a9a9a9a9a9a9a9a9a9a9a9a2c9.

That is the design decision that I would make, and I would justify it like that to my interviewer.

Critiques

Repeated string concatenation is a no-no!  In Java, strings are immutable, so every time you do + or +=, you create a new string by allocating a chunk of memory, copying the entire contents of the old string, then copying the string to be appended.  Using + once or twice is fine, but never do it many times in a loop!  Use a StringBuilder instead.  In this case, you could also just use a char[] buffer, since you know that the buffer should never grow beyond a certain limit — the length of the input string.
Speaking of the length limit… you only compare the length of the compressed string at the end, when you are basically already done.  A smarter algorithm might be able to bail out earlier if the length is exceeded.
You have redundant conditions:

for(int i = 0; i < initialLength; i++){
    char c  = chars[i];
    if(currentChar == c && i + 1 <= initialLength){
        occurances++;
    }

    else if (currentChar != c || i + 1  > initialLength){

i + 1 <= initialLength would be more idiomatically written as i < initialLength.  But that is exactly the condition of the for loop itself!  Therefore, the i + 1 <= initialLength test is always true, and superfluous.

By the same reasoning, i + 1  > initialLength can never be true, and is also superfluous.  The remaining else if (currentChar != c) could just be written as else.
The special case for the loop termination is clumsy.  This condition

if(i + 1 >=  initialLength){

can happen just once, at the very last iteration of the loop.  You can therefore just move it out of the loop.
What happens with degenerate input?  For example, if str is an empty string, then your code would crash on char currentChar = chars[0].  By my interpretation, a zero-length input should produce an empty string as output.  In fact, if you see an input of length 0, 1, or 2, you could immediately return str.
"Occurances" should be spelled "occurrences".

Suggested solution

This code would address the concerns above.  I've used just a char[] array as scratch space.  By writing directly to the buffer, I've eliminated the need to write another compressedString += currentChar + String.valueOf(occurances) at the end.

JavaDoc would be a nice touch.  If coding on a whiteboard, a "verbal" comment might be good enough, though.

/**
 * Performs run-length encoding on a string.  If the encoded string
 * would be longer than the input, then the input string is returned.
 * To make decoding simple and unambiguous, streaks are limited to
 * 9 characters long; longer streaks are broken up.
 */
public static String compress(String in) {
    if (in.length() < 3) return in;

    int n = in.length(), i = 0, o = 0;
    char[] out = new char[n + 1];
    out[o++] = in.charAt(0);
    out[o++] = '1';
    for (i = 1; i < n && o < n; i++) {
        if (in.charAt(i) == in.charAt(i - 1) && out[o - 1] != '9') {
            // Continued streak
            out[o - 1]++;
        } else {
            // New streak
            out[o++] = in.charAt(i);
            out[o++] = '1';
        }
    }
    return (i == n && o < n) ? new String(out, 0, o) : in;
}


Nested file_get_contents / json_decode I have this code but it takes over a minute to run. I'm looking at around 400 entries being brought back for the first loop. Any ideas on how it could be improved for speed?

<?php

    $context = stream_context_create(array(
        'http' => array(
            'method' => 'GET',
            'header' => "Authorization: Basic abc123\r\nContent-Type: application/json\r\n"//,'content' => json_encode($postData)
        )
    ));

    // Send the request
    $response = file_get_contents('https://localhost', FALSE, $context);

    // Check for errors
    if($response === FALSE){
        die('Error');
    }
    // Decode the response
    $responseData = json_decode($response, TRUE);

    // Print the date from the response
    foreach($responseData as $row) {
       foreach($row as $k) {
             $response = file_get_contents('https://localhost/answers'.$k['id'], FALSE, $context);
             $responseDataAns = json_decode($response, TRUE);
                 echo $responseDataAns['summary']." / ".$responseDataAns['solution']."\r\n";
       }
    }
    ?>

This is the REST API I am using specifically. This is used to retrieve all answers then this to get an instance.

I want to use the REST API to list all questions, allow someone to go into a question and then update it.

It seems to me that you are making multiple requests to 'https://localhost/answers'.$k['id'] inside a loop and that you have one route for each answers, such as:

https://localhost/answers100
https://localhost/answers101
https://localhost/answers102

My suggestion is to abandon that approach and combine them into one route that can handle multiple ids.
For instance:

https://localhost/answers?id[]=100&id[]=101&id[]=102

Then grab the id like this:

$ids = $_GET['id'];

which is an array.
Process each id one by one and combine the result and send as JSON.
For instance:

{
    "100": {
        "summary": "",
        "solution": ""
    },
    "101": {
        "summary": "",
        "solution": ""
    },
    "102": {
        "summary": "",
        "solution": ""
    }
}

So, instead of making 400 requests, you only need to make 1 request.
Much faster.


Parsing numbers from equations into strings This is my first code in C++.  Since I'm new to the language, I'm just looking for pointers on what can be made better.  I tried to cut out unnecessary stuff, but there are some comments in there.  I know Java, so if you're trying to explain anything in Java, I can understand it.

Feel free to mention anything, whether it be code style (which I'm completely unsure what the conventions are in C++), performance, etc.

The goal of the program is to take an equation, such as 5x^2+48204x=7, and parse the 5 and 48204 into strings.

using namespace std;

void printHelp();

class Equation {
public:
    string equation1;
    string equation2;

    int matrix [3][2];

    Equation(string one, string two) {
        one.erase(remove_if(one.begin(), one.end(), isspace), one.end());
        two.erase(remove_if(two.begin(), two.end(), isspace), two.end());
        equation1 = one;
        equation2 = two;

        cout << equation1 << endl;

        init();
    }

    void init() {
        size_t firstx = equation1.find_first_of("x");
        size_t secondx = equation1.find_first_of("x", firstx + 1);
        if (secondx == string::npos) {
            cout << "Secondx == 0" << endl;
        }

        //cout << firstx << endl;
        //cout << secondx << endl;

        int startloc = firstx - 1;

        while (true) {
            //cout << "starting with startloc = " << startloc << endl;
            if (startloc == -1) {
                break;
            }
            char c = equation1.at(startloc);
            if (c == ' ' || c == '=' || c == '+' || c == '-' || c == '*' || c == '/') {
                //cout << "Found something" << endl;
                break;
            }
            startloc--;
        }

        string s;
        unsigned int i = startloc + 1;
        //cout << i << endl;
        //cout << firstx << endl;
        while (i < firstx) {
            //cout << "Character at " << i << endl;
            //cout << equation1[i] << endl;
            stringstream ss;
            string temp;
            ss << equation1[i];
            ss >> temp;
            s.append(temp);
            i++;
        }
        cout << s << endl;

        startloc = secondx - 1;

        while (true) {
            //cout << "starting with startloc = " << startloc << endl;
            if (startloc == 0) {
                cout << "Problem" << endl;
            }
            char c = equation1.at(startloc);
            if (c == ' ' || c == '=' || c == '+' || c == '-' || c == '*' || c == '/') {
                //cout << "Found something" << endl;
                break;
            }
            startloc--;
        }

        s = "";
        i = startloc + 1;
        while (i < secondx) {
            stringstream ss;
            string temp;
            ss << equation1[i];
            ss >> s;
            s.append(temp);
            i++;
        }

        cout << s << endl;
    }

};

int main(int argc, char *argv[])
{
    Equation equation(argv[1], "blank");
    //printHelp();
    string s;
    cin >> s;
    return 0;
}

void printHelp()
{
    cout << "Welcome!" << endl;
    cout << "Stuff about commands" << endl;
}

Without changing things too much...

You need some #includes.
Don't use using namespace. One of the issues is knowing where a function comes from (which one is being used) - this hides it. Using a short std:: gains you a lot in understandability. You can do using std::string which is better, but imo even that isn't worth it.
You might as well put printHelp before main rather than after, that way you don't have to declare it.  And it's not used currently anyway.
Matrix also isn't used.
Having an Equation class doesn't make sense for what you're (currently) using it for. You might as well just have a naked function.
You don't allow for the possibility that firstx may be invalid, ie there are no x in the equation. This may be a given, of course. And for that matter, that the x^2 term will be first!
If secondx is invalid, skip over processing it.
Multiple breaks in while loops should be avoided, for readability. This (first) loop probably should be split off into another function since it's used twice, together with the following section.
I'm a little nervous of int startloc = firstx - 1;, since firstx is unsigned and startloc isn't. I'd cast firstx to an int first if I were using it like this.
Consider using std::string::find_last_of and std::string::substr. They'll simplify the code somewhat.
Check that you don't lose a negative coefficient.


Simple C++ program to add two integers I started learning C++ and the first exercise was to create a small program to add two numbers. No more requirements. I did this and put a small validate the user input. I am sure that it could be much better written, so I appreciate all the critics and suggestions.

#include <iostream>

int x;
int sum = 0;
int i = 0;

int addTwoNumbers()
{
    while(i<2)
    {
        std::cin >> x;
        if(std::cin.fail())
        {
           std::cout << "Error! Invalid Input" << std::endl;
           std::cin.clear();
           std::cin.ignore(256,'\n');
        }
        else
        {
             sum += x;
             i++;
        }
    }
    return sum;
}
int main()
{
    std::cout << "Enter two integers" << std::endl;
    return sum = addTwoNumbers();
}

Good job on your first program!

Before moving on to the review, a quick note on using namespace std

As long as you do it in a cpp file (and not a header), bringing in symbols from a namespace one at a time is absolutely fine, since you are in control:

#include <iostream>

using std::cout;
using std::cin;
using std::endl;

Errors

main's return value

The main()'s return value is for program status, not program output. I suspect you meant to do:

cout << "the sum is: " << addTwoNumbers() << endl;
return 0;

Good practice improvements

Avoid globals

int x;
int sum = 0;
int i = 0;

These have no business being global variables, they belong inside of your functions, as close to their first use as possible:

for(int i=0; i<2;)
{
    int x;
    cin >> x;

istream::fail() vs operator bool()

Edit Disregard this recommendation. See the comment below.

instead of if(std::cin.fail()) you should simply do if(std::cin), it actually covers more scenarios as well.

istream::ignore()

Whenever you use an arbitrary number like 256, it's a sign that something is not how it should be. In this specific case, you want to do cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n')

From a higher level:

addTwoNumbers() does too many things

Try to keep your functions short, single-purpose and reusable.

As guidance, look at this version of your main().

int main() {
  cout << "Enter two integers" << endl;

  int first = getNumber();
  int second = getNumber();

  cout << "the sum is: " << (first + second) << endl;

  return 0;
}

getNumber() would now be be a usefull and reusable piece of code, which is what functions should be.


Python tool to reduce number of resistors in circuit I've written a tool in Python to help me with a specific problem I have with electronic circuit design. Here are the motivations:

It is common in circuit design to need to choose two resistor values to form a voltage divider. Any number of combinations of resistors can satisfy the divider. And there are a lot of available resistor values to choose from. It is not uncommon for an engineer to just pick a resistor value out of thin air and solve for the other.
It is very desirable to reduce the number of line items in the BOM for a circuit board. In other words, the more resistor values I can share elsewhere in the same circuit, the better.

Prior to running this program, I have created lists of candidate resistor pairs and their combined cost in separate files. The contents of these JSON-encoded files look like this:

[[RA1, RA2, priceA],[RB1,RB2,priceB],[RC1,RC2,priceC],...]

Every pair of resistors in the same file will satisfy some specified voltage divider ratio. I have as many files as I have unique voltage dividers in the circuit.

My program then takes the multiple JSON files as input arguments, generates every permutation of resistor combinations, and tries to find which permutation produces the least number of unique resistor values. I also have it only show me the outputs with the lowest combined resistor price (many resistors are the same cost, so this helps pare down the results).

I am very new to Python and come from a long history of coding in C. I want to learn how to make this code more Pythonic. In particular, I'm convinced the embedded for-loops in the middle of the code can be replaced with something much more efficient.

resistor_bom_reducer.py:

"""
  python3 resistor_bom_reducer.py file1.json file2.json file3.json ... fileN.json
"""

import json
import sys
import itertools

pairlists = []
totalperms = 1

# Read in JSON data from input arguements.
# pairlists is a list of lists representing every pair of resistors and their combined cost
for filename in sys.argv[1:]:
    with open(filename) as json_data:
        lst = json.load(json_data)
        totalperms *= len(lst)
        pairlists.append(lst)

print("Calculating permutations: %d" % totalperms)
perms = list(itertools.product(*pairlists))
print("Done")

# Loop through every permutation of resistors.
# combo is a list of flattened lists of every resistor combination
combo = [[0 for rows in range(len(pairlists)*2)] for cols in range(len(perms))]
price = [0 for x in range(len(perms))]
count = 0
for i in range(0,len(perms)):
    for j in range(0,len(pairlists)):
        combo[i][2*j] = perms[i][j][0]
        combo[i][2*j+1] = perms[i][j][1]
        price[i] += perms[i][j][2]
        count += 1
        if ( count % 1000000 == 0 ):
            print("%d of %d" % (count,totalperms))

print("")

# Find the combination with the least number of unique resistor values
minlen = 999
for s in combo:
    if ( len(set(s)) < len(pairlists)*2 ):
        if ( len(set(s)) < minlen ):
            minlen = len(set(s))

# Find the lowest combination price
minprice = 999.0
for s,p in zip(combo,price):
    if ( len(set(s)) == minlen ):
        if ( p < minprice ):
            minprice = p

# Print only combinations of the lowest price and least number of unique resistor values
for s,p in zip(combo,price):
    if ( len(set(s)) == minlen ):
        if ( p == minprice ):
            print(s,p)

with open(filename) as json_data:

Good!

        lst = json.load(json_data)
        totalperms *= len(lst)
        pairlists.append(lst)

lst isn't a great name, but I think it's forgiveable with such a tight scope. Maybe worth sanity-checking that the structure is what you expect?

print("Calculating permutations: %d" % totalperms)
perms = list(itertools.product(*pairlists))

Why bother with the code to calculate totalperms if you can just extract it as len(perms)?

Although, on the flip side, why coerce perms to a list and force it all into memory?

# combo is a list of flattened lists of every resistor combination
combo = [[0 for rows in range(len(pairlists)*2)] for cols in range(len(perms))]

I'm not sure what the point of this is. Perhaps the comment should explain why we need it.

for i in range(0,len(perms)):

You're right to think there's a more Pythonic way of doing this:

    for i, perm in enumerate(perms):

This doesn't require perms to be a list, because it removes the need to index into it.

    for j in range(0,len(pairlists)):
        combo[i][2*j] = perms[i][j][0]
        combo[i][2*j+1] = perms[i][j][1]
        price[i] += perms[i][j][2]

This really isn't doing much. Why not inline the calculation of the best option?

        count += 1
        if ( count % 1000000 == 0 ):
            print("%d of %d" % (count,totalperms))

That's buggy: it should be at one less level of indentation.

# Find the combination with the least number of unique resistor values

This is a pet peeve of mine: unique means that there is only one in existence. Distinct expresses precisely and unambiguously what you mean here.

minlen = 999
for s in combo:
    if ( len(set(s)) < len(pairlists)*2 ):
        if ( len(set(s)) < minlen ):
            minlen = len(set(s))

It's not standard Python style to use brackets around the condition. The conditions can be merged into one as

    if len(set(s)) < min(len(pairlists)*2, minlen):
        minlen = len(set(s))

but then that points to just initialising minlen to len(pairlists)*2 instead of the magic number 999.

Finally, this doesn't strike me as likely to be an efficient approach.

I would think it's likely to be more efficient (although still not very) to build a set of the distinct values and then look at subsets in order of increasing size, maybe guided by frequencies of the values.

The best option is probably to try to work out an encoding to a standard problem and use an optimised solver. The obvious one is SAT: you've basically got a Boolean formula already: each resistor value corresponds to a variable, and each file is an OR of a bunch of two-variable ANDs. Then you just need to add a count of the number of variables which are TRUE and require it to be less than a threshold, and search for the critical value of the threshold. For the prices you can use the same SAT formulation and loop:

while sat_instance.solve():
    yield sat_instance.solution()
    add a rule to sat_instance to exclude the solution


Euler Project Problem #1 Just starting out in Python, coming from a C/C++ background. Any comments on style, how its solved, unseen redundancies, is it 'Pythonic' enough or am I lacking in the general syntax structure?

#https://projecteuler.net/problem=1
'''
problem statement: If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.
Find the sum of all the multiples of 3 or 5 below 1000.

Created on Jan 12, 2018

@author: Anonymous3.1415
'''

#is test number divisable by divisor
def divisable_by(divisor,test_num):
    if not test_num % divisor:
        return True
    else:
        return False
'''
FUNCTION DEF: nat_nums_divisors_1_or_2(divisor1, divisor2 , range_start, range_end)
PURPOSE: Find list of natural numbers that are divisable by either divisor1 or divisor2
@ var: (nat_nums_list) list of numbers that satisfy conditon
@ var: (nat_num_iterator) used to iterate through the natural numbers withing a given range

'''
def nat_nums_divisors_1_or_2(divisor1, divisor2 , range_start, range_end):
    nat_nums_list = []
    for nat_num_iterator in range(range_start, range_end):
        if divisable_by(divisor1, nat_num_iterator) or divisable_by(divisor2, nat_num_iterator):
            nat_nums_list.append(nat_num_iterator)
        nat_num_iterator += 1
    return nat_nums_list

nat_nums_div_3_5 = [] # Natural numbers divisable by either 3 or 5
divisor_3, divisor_5 = 3, 5
range_start, range_end = 1, 1000

nat_nums_div_3_5 = nat_nums_divisors_1_or_2(divisor_3, divisor_5, range_start, range_end)

nat_nums_div_3_5_sum = sum(nat_nums_div_3_5)
print ("The sum of Natural Numbers in range: [%d, %d]; divisable by either 3 or 5 is: %d") % (range_start, range_end, nat_nums_div_3_5_sum)

Before the review, let me advise you that Project Euler problems have very little to do with programming. They are mostly about math. Your code is very ineffective because it scales (almost) linearly with the range, whereas the solution can be achieved in a (almost) constant time.

The construct

    if condition:
        return True
    else
        return False

is a long way to say

    return condition

The in range loop which modifies its loop variable, as in

for nat_num_iterator in range(range_start, range_end):
    ....
    nat_num_iterator += 1

gives me shivers. Here you increment nat_num_iterator twice per iteration. If it is an intent, use range(range_start, range_end, 2).


Cloning Windows Forms controls from a template In a recent project I needed a way to clone windows forms controls from a template which had predefined Size, Font, etc.

Since there is no Clone() method in the Control class, I created my own object that does that:

public class ControlTemplate<TSource>
    where TSource : Control
{
    private readonly List<Action<TSource>> _controlActions;

    public ControlTemplate()
    {
        _controlActions = new List<Action<TSource>>();
    }

    public ControlTemplate<TSource> WithArgument(Action<TSource> action)
    {
        if (action == null)
        {
            throw new ArgumentNullException(nameof(action));
        }
        _controlActions.Add(action);
        return this;
    }

    public TSource Clone()
    {
        TSource control = Activator.CreateInstance<TSource>();
        foreach (var action in _controlActions)
        {
            action.Invoke(control);
        }
        return control;
    }
}

It works somewhat like the Builder pattern, you can call WithArgument multiple times and all of the changes to the control will be saved in a list, to be later executed in the Clone method, on a fresh object in order to apply all of the changes.

The problem with this class is that it's not immutable and that's something really important here in order to have the same object every time you clone it.

If you have an instance of it stored somewhere that works as a template, outside classes can access the Clone method to create new object but they can also make modifications to the template using the WithArgument method, which will cause problems.

In order to fix that I made a "wrapper" class that only exposes the Clone method and operates with an instance of ControlTemplate internally which makes the template more immutable:

public class ControlTemplateCreator<TSource>
    where TSource : Control
{
    private readonly ControlTemplate<TSource> _sourceOfTemplate;

    public ControlTemplateCreator(ControlTemplate<TSource> sourceOfTemplate)
    {
        if (sourceOfTemplate == null)
        {
            throw new ArgumentNullException(nameof(sourceOfTemplate));
        }
        _sourceOfTemplate = sourceOfTemplate;
    }

    public TSource Clone()
    {
        return _sourceOfTemplate.Clone();
    }
}

Example usage would be:

public static class Settings
{
    private static readonly ControlTemplate<Label> _templateLabel;
    public static ControlTemplateCreator<Label> TemplateLabel { get; }

    static Settings()
    {
        _templateLabel = new ControlTemplate<Label>()
            .WithArgument(label => label.TextAlign = ContentAlignment.MiddleLeft)
            .WithArgument(label => label.AutoSize = false)
            .WithArgument(label => label.Size = new Size(135, 20))
            .WithArgument(label => label.Margin = new Padding(0))
            .WithArgument(label => label.Font = new Font("Aerial", 9));

        TemplateLabel = new ControlTemplateCreator<Label>(_templateLabel);
    }
}

public class Foo
{
    public void Bar()
    {
        Label clonedLabelFromTemplate = Settings.TemplateLabel.Clone();
    }
}

The code looks pretty good to me, it's extendable, you can create various templates from the same class, outside classes cant modify the original template.

Here are some of my questions:

I wonder why isn't there a Clone method in the Control class are there any bad sides to cloning controls this way?
Are there any flaws in the code?
I'm not sure if WithArgument is a good name.

I think you should remove Clone method from your builder. So you essentially would have:

interface ITemplateBuilder<TSource>
{
     ITemplateBuilder<TSource> WithArgument(...);
     ITemplate<TSource> Build();
}

where:

interface ITemplate<TSource>
{
     TSource Clone();
}

while it is similar to what you have, it conveys your intention in clearer way.

But overall I have to agree with Adriano, I think you are over-engineering. A simple factory will do the job and will probably take less code to write. However you can improve your implementation so it works more like a simplified version of WPF style, so that those actions you were saving up can be applied to any instance of Control. That, I think, would be much more useful. That you can't do with a factory.

 var style = new ControlStyle<Label>()
        .Setter(label => label.TextAlign = ContentAlignment.MiddleLeft)
        .......;
 style.Apply(_myLabel);
 style.Apply(_myCustomClassThatInheritsLabel);


Array to Tuple in Swift I am burdened with the requirement of interacting with a C based library, which has a bunch of constant sized arrays (e.g. char[17]).

When trying to assign or read those properties from swift, they are represented as a Tuple type of that size..

As far as I know, you can't access tuples with subscripts, and it is not trivial to convert an array to a tuple .. so I am left with this ugly code to interact with the library:

Note for the curious: The C library uses the eatLength to determine which values of the tuple are valid, and which shouldn't be considered. That is why I don't care about what the padded value is.

typedef struct _DMMove
{
    uint8_t steps[17];
    uint8_t eats[16];
    uint8_t eatLength;
} DMMove;

extension DMMove {

    init(steps: [Int], eats: [Int]) {

        var paddedSteps = Array(0..<17) as [UInt8]
        var paddedEats = Array(0..<16) as [UInt8]

        for (i, v) in enumerate(steps) {
            paddedSteps[i] = Int8(v)
        }

        for (i, v) in enumerate(eats) {
            paddedEats[i] = Int8(v)
        }

        var stepsTuple = (0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0) as (Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8)
        var eatsTuple = (0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0) as (Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8,Int8)

        stepsTuple.0 = paddedSteps[0]
        stepsTuple.1 = paddedSteps[1]
        stepsTuple.2 = paddedSteps[2]
        stepsTuple.3 = paddedSteps[3]
        stepsTuple.4 = paddedSteps[4]
        stepsTuple.5 = paddedSteps[5]
        stepsTuple.6 = paddedSteps[6]
        stepsTuple.7 = paddedSteps[7]
        stepsTuple.8 = paddedSteps[8]
        stepsTuple.9 = paddedSteps[9]
        stepsTuple.10 = paddedSteps[10]
        stepsTuple.11 = paddedSteps[11]
        stepsTuple.12 = paddedSteps[12]
        stepsTuple.13 = paddedSteps[13]
        stepsTuple.14 = paddedSteps[14]
        stepsTuple.15 = paddedSteps[15]
        stepsTuple.16 = paddedSteps[16]

        eatsTuple.0 = paddedEats[0]
        eatsTuple.1 = paddedEats[1]
        eatsTuple.2 = paddedEats[2]
        eatsTuple.3 = paddedEats[3]
        eatsTuple.4 = paddedEats[4]
        eatsTuple.5 = paddedEats[5]
        eatsTuple.6 = paddedEats[6]
        eatsTuple.7 = paddedEats[7]
        eatsTuple.8 = paddedEats[8]
        eatsTuple.9 = paddedEats[9]
        eatsTuple.10 = paddedEats[10]
        eatsTuple.11 = paddedEats[11]
        eatsTuple.12 = paddedEats[12]
        eatsTuple.13 = paddedEats[13]
        eatsTuple.14 = paddedEats[14]
        eatsTuple.15 = paddedEats[15]

        self.init(
            steps: stepsTuple,
            eats: eatsTuple,
            eatLength: Int16(eats.count)
        )
    }

}

More Notes

If you don't know how a C struct is exposed to Swift, well it's as simple as though there is an actual Swift struct with an init that takes all the struct's attributes as parameters.

I am sure someone will suggest using Tuple initializers ... Here ya go:

  Expression was too complex to be solved in reasonable time; consider breaking up the expression into distinct sub-expressions

Today I had to do something that looks like this, so I am posting my answer in case someone needs this.
I needed to create a C struct for passing to pulseaudio in linux, the struct is like this:

struct pa_cvolume{
    uint32 channels;
    uint32 values[32];
}

what I was trying to do was to set newVolume in as much fields as the value of channels was:

let newVolume = 23123 // some random value

var paVolume = pa_cvolume() // create struct
paVolume.channels = 2 // read channels from other place

var volumeChannels = withUnsafeMutablePointer(&paVolume.values) {
  return UnsafeMutablePointer<UInt32>($0)
}

//iterate as if it was C
for i in 0..<Int(paVolume.channels) {  // Int required to subscript UnsafePointers
  volumeChannels[i] = newVolume
}

print(paVolume)

this way you don't need padding (because creating the struct comes with memory already zeroed)


What is the best way to deal with multiple return types in Python's is_number? I have not tried decorators yet or functions within functions, but this current un-pythonic method seems a little convoluted. I don't like how I need to repeat myself when I check the return_type 3 times.

Any suggestions are welcome, especially if the repetion can be dealt with in an elegant way. Please note that I am not that interested in the numerous reference to test if an object is a number as I believe this part is incorporated within my solution. I am interested in addressing the 3x duplication to deal with the return type. Moreover, while I appreciate the locale method is the more rigorous way of dealing with internationalisation, I prefer the simplicity of allowing the caller more flexibility in choosing the characters.

Also, some people on Stack Overflow just posted trivial solutions to the smaller boolean is_number function that has been asked many times.

def is_number(obj, thousand_sep=',', decimal_sep=None, return_type='b'):
    """ determines if obj is numeric.

    if return_type = b, returns a boolean True/False
    otherwise, it returns the numeric value

    Examples
    --------
    >>> is_number(3)
    True
    >>> is_number('-4.1728')
    True
    >>> is_number('-4.1728', return_type='n')
    -4.1728
    >>> is_number(-5.43)
    True
    >>> is_number("20,000.43")
    True
    >>> is_number("20.000,43", decimal_sep=",", thousand_sep=",")
    True
    >>> is_number("20.000,43", decimal_sep=",", thousand_sep=".", return_type="n")
    20000.43
    >>> is_number('Four')
    False
    >>> is_number('Four', return_type='n')
    """
    try:
        if is_string(obj):
            if decimal_sep is None:
                value = float(obj.replace(thousand_sep, ""))
            else:
                value = float(obj.replace(thousand_sep, "").replace(decimal_sep, "."))
            if return_type.lower() == 'b':
                return True
            else:
                return value
        else:
            value = float(obj)
            if return_type.lower() == 'b':
                return True
            else:
                return value
    except ValueError:
        return False
        if return_type.lower() == 'b':
            return False
        else:
            return None

I am disregarding some questionable code (such as the fact that '3,,,4' would be handled identically to '34', and so forth), and as you mention, the fact that locale's atof is probably more fitting than write-your-own. I am doing so because, worse than those, I really don't like the return_type parameter and its usage.

I would strongly suggest separating this into two functions. For example a to_number which acts like return_type='n', or even throws an exception instead of returning None, and an is_number which calls to_number and converts the exception or None to False, and other values to True. Each function would have very simple usage thanks to very simple definitions, and their code would become simpler accordingly.


Multiple isset block going from more specific to more generic I have a data structure that allows for inheritance of properties, in such a way that if a property is not defined within a context, the second most specific context is checked until said property is found. Simplified it's basically the following:

function getCarPartColor($carData, $owner, $brand, $part) {
    $color = false;

    if(isset($carData->{$owner}->{$brand}->{$part})) {
        $color = $carData->{$owner}->{$brand}->{$part};

    } else if(isset($carData->{$owner}->{$brand}->allparts)) {
        $color = $carData->{$owner}->{$brand}->allparts;

    } else if(isset($carData->{$owner}->allbrands->allparts)) {
        $color = $carData->{$owner}->allbrands->allparts;

    } else if(isset($carData->allowners->allbrands->allparts)) {
        $color = $carData->allowners->allbrands->allparts;
    }

    return $color;
}

Where $carData is the data structure and the rest are the querying variables. If I were to call:

getCarPartColor($carData, "AnthonyNewel", "opel", "engine");

But I hadn't defined any color for $carData->AnthonyNewel->opel->engine, then it checks if a color is set on $caraData->AnthonyNewel->opel->allparts, and fetches this one instead if it exists. If it's not there, it then checks if it's defined for allbrands, and it goes on.

While the code above works, one can't help but notice the repetition. Is there a less verbose way to write this while still being maintanable (i.e. not a long ternary operator)?

Long time reader, first time poster, welcoming of critique.

You could create an array populated with each variant of the property you're looking for:

    $is_set = array (
        $carData->{$owner}->{$brand}->{$part},
        $carData->{$owner}->{$brand}->allparts,
        $carData->{$owner}->allbrands->allparts,
        $carData->allowners->allbrands->allparts
    );

And then check each value in the array using a foreach loop which ends when the condition isset evaluates to true. The first property to evaluate to true is then returned which in turn breaks you out of the function (credit to Brett for the insight).

    foreach ($is_set as $this_set) {
        if isset($this_set) {
            return $this_set;
        }
    }

Putting it all together would look like:

function getCarPartColor($carData, $owner, $brand, $part) {

    $is_set = array (
        $carData->{$owner}->{$brand}->{$part},
        $carData->{$owner}->{$brand}->allparts,
        $carData->{$owner}->allbrands->allparts,
        $carData->allowners->allbrands->allparts
    );

    foreach ($is_set as $this_set) {
        if isset($this_set) {
            return $this_set;
        }
    }
}


Generating and using a list of prime numbers Here is some code I've created to generate and use a list of prime numbers.  This is some of the first Common Lisp code I've written, and I'd be grateful for any comments regarding style and its use or misuse of common idioms.  I'm not really interested in improving the algorithm, but in making my implementation readable and efficient.

    ;;; Estimate the number of primes below a number n
(defun primes-pi (n)
  (ceiling (/ n (- (log n) 1))))

;;; (A high) estimate of the number below which there are n primes, this is the
;;; reverse of primes-pi
(defun primes-n (n)
  (* n (ceiling (+ (log n) 1))))

;;; Code to generate prime numbers
(defun plist (n)
  (let (;; An array of booleams flags for the sieve
        (arr (make-array (+ n 1) :element-type 'boolean :initial-element t))
        ;; The first prime the loop below will handle
        (p 3)
        ;; results is used to build the list of primes
        (result nil))
    ;; 0 isn't prime
    (setf (aref arr 0) nil)
    ;; 1 isn't prime
    (setf (aref arr 1) nil)
    ;; 2 is primes
    (push 2 result)

    ;; Now the special cases are out of the way start sieving for new primes
    ;; when p is nil the loop has reached the end of the arr array
    (do () ((eq nil p))
      ;; add p to the list of primes
      (push p result)
      ;; Remove all multiples of p from the array
      (loop for i from (+ p p) to n by p
         do (setf (aref arr i) nil))
      ;; search forward in the array to the next non nil entry, this will be
      ;; the next prime.
      (setf p (loop for i from (+ 2 p) to n by 2
                 when (eq t (svref arr i))
                 return i)))
    ;; reverse the list to get the primes in ascending order
    ;; not really needed if only using the list in build-is-prime?
    (nreverse result)))

;;; This function creates and returns a closure that allows testing
;;; for primeality of at least the first n prime number
(defun build-is-prime (n)
  (let ((prime-table (make-hash-table :size n)))
    ;; Populate the hash table
    (loop for i in (plist (primes-n n)) do
         (setf (gethash i prime-table) t))
    ;; Lambda to test for presence of p in the hash table
    (lambda (p)
      (gethash p prime-table)
      )))

for one thing, (eq nil p) is usually written (not p).

the variable p participating only in that do should be declared inside it, not outside it in the enclosing let. Everything should be in its proper place. Same with result - make do your last form and use the return clause of do to return the result:

(defun plist (n)
  (let (
        (arr (make-array (+ n 1) :element-type 'boolean :initial-element t))
       )
    (setf (aref arr 0) nil)
    (setf (aref arr 1) nil)

    (do ( (p 3)
          (result (list 2)))
        ( (not p)  (nreverse result))

      (push p result)   ; and what if n==2 ??
      ....
      )))

Don't mix arefs and svrefs, be consistent. Again, the test (when (eq t (svref arr i)) ...) would normally be written just as (when (svref arr i) ...).

Your do loop is a little buggy: it adds 3 unconditionally, even if n < 3. Reorganize. Or, and since you don't inspect them, no need to mark 0 and 1 as non-primes, just like we don't bother with the evens. Which means we get to declare the array inside the do's scope too:

(defun plist (n)
  (do ((arr (make-array (+ n 1) :element-type 'boolean :initial-element t))
       (result (list 2))
       (p      3        (+ p 2)))
      ((> p n)
       (nreverse result))       ; return form
    (when (aref arr p)
        (push p result)
        (if (<= p (/ n p))
          (loop for i from (* p p) to n by (* 2 p)
            do (setf (aref arr i) nil))))))

An algorithmic improvement, though you didn't want one: start eliminating multiples of p from (* p p), not from (+ p p), and increment by steps of (* 2 p) among odds only, for twice the speed.

Now that the code is short and compact, we see one more possibility for improvement: if we hit the termination condition, no need to recheck it. Finally, add the documentation string there too:

(defun plist (n)
  "return the list of primes not greater than n.
   build it by means of the sieve of Eratosthenes."
  (do ((arr (make-array (+ n 1) :element-type 'boolean :initial-element t))
       (result (list 2))
       (p      3        (+ p 2)))   ; a candidate possible prime
      ((> p (/ n p))
       (nconc (nreverse result)
              (loop for i from p to n by 2 if (aref arr i) collect i)))
    (when (aref arr p)              ; not marked as composite: p is prime
        (push p result)
        (loop for i from (* p p) to n by (* 2 p)  ; mark the multiples
            do (setf (aref arr i) nil)))))

I don't find micro-commenting adding much clarity (even the opposite), but YMMV. Variable names like arr and result are self-documenting.


Couting numbers greater than given - using binary search It's my first exercise involving the well-known binary search. The problem goes as follows: we are given a sequence of n numbers and we have to answer to t questions, each of which is: “How many numbers in the specified sequence are greater than the i-th element of the sequence?” For example, we have n = 3 and t = 3. Our sequence is 2, 1, 3. And we have to count numbers greater than the first (1), second (2) and third element of our sequence (3). The correct output is “1 2 0”.  I've already written some code… but it works only in some cases.

    #include <iostream>
#include <algorithm>

using namespace std;

template<typename T> int binSearch(const T& x, const T* array, size_t size)
{

int l = 0, p = size - 1, s;

while (l <= p)
{

s = (l + p) / 2;

if (array[s] == x)
return s;

if (array[s] > x)
p = s - 1;

else
l = s + 1;
}

return -1;
}

int main(){

    int n, t;

    cin >> n >> t;

    int a[n];

    int a_sorted[n];

    int questions[t];

    for (int i=0; i<n; i++){ cin >> a[i]; a_sorted[i]=a[i]; };

    for (int i=0; i<t; i++) cin >> questions[i];

    sort (a_sorted, &a_sorted[n]);

    for (int i=0; i<t; i++){

        int index=binSearch(a[questions[i]-1], a_sorted, n);

        int count_=n-(index+1);

        cout << count_ << endl;

    }

    return 0;

}

If my question is difficult to understand, feel free to ask for additional information – it wasn't easy to ask too ;)

First, the algorithm you're using strikes me as fairly inefficient. At least if I've understood the code, your intent is to have one array of items in original order, and another in sorted order. When you get a query, you find the number at the specified position in the original array, then do a binary search to find the same item in the sorted array.

I guess if the intent is specifically to write a binary search, that's all right, but if your intent is really to carry out such queries quickly, this doesn't seem like the best approach. Instead, I'd create a more direct mapping from input position to result.

For one possible way to do this, you could create an array of pairs of input values and the position at which that value was entered:

typedef std::pair<int, size_t> input;

std::vector<input> inputs;

int temp;
size_t i = 0;

while (std::cin >> temp)
    inputs.emplace_back(temp, i++);

Then we can sort the inputs (preferably in descending order) based on the input number:

std::sort(inputs.begin(), inputs.end(),
    [](input a, input b) {
        return a.first > b.first;
    });

Then (the interesting part) we create our lookup table to give the result for a query directly:

std::vector<size_t> query_table(inputs.size());

for (int i=0; i<inputs.size(); i++)
   query_table[inputs[i].second] = i;

Then handling a query is something like:

size_t query(size_t input_pos) {
    return query_table[pos];
}

Looking at the code in your question, the first thing that sticks out is indentation (or lack thereof). In binSearch, you seem to have no indentation at all. In main, you have a little indentation, but most of your loops and such have the controlled statement on the same line as the loop header, which most C and C++ programmers find distasteful (to put it mildly).

You're also depending on using a variable length array (your int a[n]; andint a_sorted[n];`) which isn't part of C++ (though some compilers include it as an extension).

Looking specifically at the algorithm you're using for your binary search, it appears that it'll potentially give incorrect answers whenever your input contains duplicate values. It simply looks for any instance of the correct value in the sorted array, but since you only want the number of inputs that were large, it should be looking for the position of the last entry with that value. For example: if your input were [1, 2, 1, 1, 1, 1, 1, 2], the sorted array would be: [1, 1, 1, 1, 1, 1, 2, 2]. Since that has 8 elements, and the first element it looks at is the fourth, it would find the fourth, and you'd then conclude that all the other numbers were larger, so it would say there were four larger numbers. What you really want to find is the last instance of that value, so (in this case) you'd find there are really only two larger values.


Setting header color on scroll for each post On this site this site, I have some code which you can see below which figures out the offset for each post from the top of the document and assigns a colour from each post to the header when it scrolls into view. I am having to wait for the window load event until the code works which really is not very elegant. Can anyone see how I can improve this code?

$(window).load(function() {
    var $header = $("header");
    var numberOfSections = $("section").length;
    var sectionOffsets = [];

    for(var i = 0; i < numberOfSections; i++) {
        sectionOffsets.push($("section").eq(i).offset().top);
    }

    $(window).scroll(function () {
         $("section").each(function () {
            if ($(window).scrollTop() > $(this).offset().top - 180) {
                $("header").css('color', $(this).data("colour"));
            }
        });
    }).scroll();
});

jsBin demo

Instead of storing section positions on window onload time, which can be always kind of inaccurate since your images might still load...

get dynamically  the current position / size data of an element using:

element.getBoundingClientRect()

using that JS method you don't even need to calculate the $(window).scrollTop()
 cause the returned value is the element's position respective to the client top (viewport top edge).

To retrieve only the one element that matches a top criteria you can use the jQuery's .filter() method and return the element which...

$section.filter(function(){
    var r = this.getBoundingClientRect();
    return  r.top + r.height - 100  > 0; // ...matches this
})

(100 is the header height in this demo; set as desired or calculate dynamically)

Now, since jQuery filtered more than one element that matches that criteria of (gbcr.top + gbcr.height - headeroffser) > 0 by going directly to chain another method to it like .data(), the value will be respective to the first of elements returned in the .filter() collection:

$header.css({
  color : $section.filter(function(){
    var r = this.getBoundingClientRect();
    return  r.top + r.height - 100 > 0;
  }).data().colour // is the colour data of the first of filtered elements
});

Store that colouring stuff inside a setScrollColors function and use like:

$(function() { // DOM ready shorthand

  var $header  = $("header");  // Cache selectors
  var $section = $("section");

  function setScrollColors() {
    $header.css({
      color : $section.filter(function(){
        var r = this.getBoundingClientRect();
        return  r.top + r.height - 100 > 0;
      }).data().colour
    });
  }

  setScrollColors();                            // Call inside dom ready
  $(window).on("load scroll", setScrollColors); // call also on load and scroll

});
*{margin:0;}
header{
  position:fixed;
  z-index:1;
  top:0;
  width:100%;
  background:#f9f9f9;
  height:100px;
  border-bottom:1px solid currentColor;
}
#content{
  margin-top:100px;
}
section{
  min-height: 1000px;
  min-height: calc(100vh - 100px);
  border-top:1px solid #000;
}

HEADER

black

red

blue

green

fuchsia

orange


Address book with database tables This is my first time messing with PHP and I tried to put together a simple address book that when the user fills out the form it would update a database table, then display the row data on the page after submission.

It works fine, but I just want to make sure I'm headed in the right direction.

index.php:

<?php

if ($_SERVER["REQUEST_METHOD"] == "POST") {

// connect to database
include("inc/dbconnect.php");

// assigns form data to table columns
$assign = "INSERT INTO contacts(firstName,lastName,email,phone,birthday) VALUES ('$_POST[firstName]','$_POST[lastName]','$_POST[email]','$_POST[phone]','$_POST[birthday]')";

// execute query
if (mysqli_query($database,$assign)) {
    header("Location:http://localhost/address-book/");
    exit;
} else {
    echo "Everything blew up!" . mysqli_error($database);
}

// closes database connection
mysqli_close($database);

}

?>

<!DOCTYPE html>

<title>Address Book</title>

Address Book

            Contact name
            Email address
            Phone
            Birth Date

        <?php include("inc/records.php"); ?>

Add new contact

<form method="post" action="index.php">
    <input type="text" name="firstName" id="firstName" placeholder="First name">
    <input type="text" name="lastName" id="lastName" placeholder="Last Name">

    <input type="text" name="email" id="email" placeholder="Email address">
    <input type="text" name="phone" id="phone" placeholder="Phone">

    <input type="text" name="birthday" id="birthday" placeholder="Birthday">
    <input type="submit" value="Save record" placeholder="Save">
</form>

dbconnect.php:

// database connect
$database = mysqli_connect(
            "localhost",
            "user",
            "password",
            "database"
);

records.php:

// connects to database
include("dbconnect.php");

// targets the database table contacts
$records = mysqli_query($database,"SELECT * FROM contacts");

// pull row data from database
while($record = mysqli_fetch_array($records)) {
    echo "
";
    echo "" . $record['firstName'] . " " . $record['lastName'] . "	";
    echo "" . $record['email'] . "	";
    echo "" . $record['phone'] . "	";
    echo "" . $record['email'] . "	";
    echo "
";
}

mysqli_close($database);

I think there are two major issues you should address right in the beginning when you start with PHP.

Separation of Layout and Logic

This means in general that you shouldn't mix HTML and PHP code. Later this will lead you to the MVC-Pattern.

index.php

<?php
// connect to database
include("inc/dbconnect.php");

if ($_SERVER["REQUEST_METHOD"] == "POST") {
    //...
}

$result = mysqli_query($database,"SELECT * FROM contacts");

$records=array();
while($row = mysqli_fetch_array($result)) {
    $record[]=$row;
}

mysqli_close($database);

include 'template.php';

template.php

<!DOCTYPE html>

...

        <?php foreach ($records as $record):?>

             <?= $record['firstName']?> <?= $record['lastName']?>
             <a href="mailto:<?= $record['email']?>"><?= $record['email']?>
             <?= $record['phone']?>
             <?= $record['email']?>

        <?php endforeach;?>

...

Prevent SQL Injection

You are passing user input directly to the query you send to the database. Wikipedia will elaborate some examples in detail and shows how queries can be manipulated.

The easies way for you to prevent this, is using PHP Data Objects and Prepared Statements. I think there is no need for going in detail here if you don't have a specific question, there are many good tutorials out there. The important part is, that the user is no longer able to change the query you send to the database.

Minor issues

If you only have one database connection PHP will use this connection automatically with the mysqli_ methods so you don't have to pass the $database around.
It is best practice to leave away the final '?>' in plain php files. This will prevent that you send any whitespace after ?> to the browser accidentally. (Important if you change the header later.)
echo "Everything blew up!" . mysqli_error($database); . May you should use die.


Word frequency counter I recently took a self-assessment question to assess my Python ability for an online class. The problem was to return the frequency of a word occurring, as part of a tuple.

  Implement a function count_words() in Python that takes as input a
  string word_string and a number number_of_words, and returns the n most frequently-occurring words in word_string. The return value should be a list of tuples - the top n
  words paired with their respective counts [(, ), (,
  ), ...], sorted in descending count order.

  You can assume that all input will be in lowercase and that there will
  be no punctuations or other characters (only letters and single
  separating spaces). In case of a tie (equal count), order the tied
  words alphabetically.

  E.g.: print count_words("this is an example sentence with a repeated word example",3) Output: [('example', 2), ('a', 1), ('an', 1)]

def count_words(word_string, number_of_words):
  """
  take in a word string and return a tuple of the
  most frequently counted words

  word_string = "This is an example sentence with a repeated word example",
  number_of_words = 3
  return [('example', 2), ('This', 1), ('a', 1)]
  """
  word_array = word_string.split(' ')
  word_occurence_array = []
  for word in word_array:
    if word in word_string:
      occurence_count = word_array.count(word)
      word_occurence_array.append((word, occurence_count))
    else:
      # no occurences, count = 0
      word_occurence_array.append((word, 0))

  # dedupe
  word_occurence_array = list(set(word_occurence_array))

  # reorder
  # can also pass, reverse=True, but cannot apply `-` to string
  word_occurence_array.sort(key=lambda tup: (-tup[1], tup[0]))

  # only return the Nth number of pairs
  return word_occurence_array[:number_of_words]

You can then call this function:

count_words(word_string="this is an example sentence with a repeated word example", number_of_words=3)

which returns [('example', 2), ('a', 1), ('an', 1)]

I found the process of tuple sorting, quite tricky, and achieved it using word_occurence_array.sort(key=lambda tup: (-tup[1], tup[0])). I was wondering if there are any other improvements that I can make to my overall code.

I hope this is a reasonable question - I've tweaked the description and example so that I hope it's not too easily identifiable.

This is exactly the kind of task that should be handled using collections.Counter, which offers a most_common() method to extract the results.


Tag helper to add class if the model is invalid I need to add a specific class to the input if the model is invalid

ValidationErrorClassTagHelper.cs

[HtmlTargetElement("input", Attributes = ValidationForAttributeName + "," + ValidationErrorClassName)]
public class ValidationErrorClassTagHelper : TagHelper
{
    private const string ValidationForAttributeName = "validation-for";

    private const string ValidationErrorClassName = "validationerror-class";

    [HtmlAttributeName(ValidationForAttributeName)]
    public ModelExpression For { get; set; }

    [HtmlAttributeName(ValidationErrorClassName)]
    public string ValidationErrorClass { get; set; }

    [HtmlAttributeNotBound]
    [ViewContext]
    public ViewContext ViewContext { get; set; }

    public override void Process(TagHelperContext context, TagHelperOutput output)
    {
        ViewContext.ViewData.ModelState.TryGetValue(For.Name, out ModelStateEntry entry);
        if (entry != null && entry.Errors.Any())
        {
            output.AddClass(ValidationErrorClass, HtmlEncoder.Default);
        }
    }
}

AccountViewModels.cs

using System.ComponentModel.DataAnnotations;

namespace AccountViewModels
{
    public class LoginViewModel
    {
        [Required]
        [EmailAddress]
        public string Email { get; set; }

        [Required]
        [DataType(DataType.Password)]
        public string Password { get; set; }
    }
}

Login.cshtml

@model LoginViewModel

<form method="post" asp-action="Login">
    <input asp-for="Email" validation-for="Email" validationerror-class="is-invalid"/>
    <input asp-for="Password" validation-for="Password" validationerror-class="is-invalid"/>
    <button type="submit" class="btn btn-primary">Login</button>
</form>

Generated html is the email is invalid for example

<input class="input-validation-error is-invalid" type="text" data-val="true" data-val-required="The Email field is required." id="Email" name="Email" value="">

Is the good way to do this ?
Is there a native way to do that using Asp.Net Core 2?

Have you considered adding style rules to class input-validation-error? It gets added to input element when its value is recognized as invalid.

See live example on .NET Fiddle.

However, if you really need to add your own custom class, take notice that your code only adds custom class to input elements on server side, so that specific class will stay on input elements forever (until next HTTP request). Earlier, I suggested making use of default input-validation-error class, because jQuery validation plugin takes care of removal that class when it discovers the input value as valid. Eventually, you would have to implement same client-side mechanism to remove/add your custom class probably by writing custom jQuery validation script.

Of course, none of above is a real problem if you simply do not care about dynamic client-side validation.


Search a file (simple grep clone) For part of a project, I was given this directive:

  The grep command will take an exact string and an exact filename as
  the 2 arguments and you will need to search through the filename for
  the given string and display the "line" and "line number" the string
  resides in.
  Example output would be:

# grep bob FileWithBob
12 and then bob picked up the phone and called jane
24 even though they knew that bobby was not going to show up

Testing seems to give identical results to the actual grep program for everything I've tried:

$ ./my_grep "test" grep.c
1:// test
2:// multi-line test

$ grep -n "test" grep.c
1:// test
2:// multi-line test

grep.c:

// test
// multi-line test
#define _GNU_SOURCE 1       // for getline()
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <regex.h>
#include <unistd.h>
#include <sys/types.h>

#ifndef streq
#define streq(x, y) (strcmp((x), (y)) == 0)
#endif

#define MSGBUFSIZE  512

regex_t pattern;

int compile_pattern(const char *pat)
{
    int flags = REG_NOSUB;
    char error[MSGBUFSIZE] = "";

    int ret = regcomp(&pattern, pat, flags);
    if (ret)
    {
        regerror(ret, &pattern, error, sizeof(error));
        fprintf(stderr, "pattern `%s`: %s\n", pat, error);
        return ret;
    }

    return 0;
}

// read lines of text and match against the pattern
int process(FILE *fp)
{
    char *buf = NULL;
    size_t size = 0;
    char error[MSGBUFSIZE] = "";
    int ret = 0;

    for (int i = 1; getline(&buf, &size, fp) != -1; i++)
    {
        ret = regexec(&pattern, buf, 0, NULL, 0);
        if (ret)
        {
            if (ret != REG_NOMATCH)
            {
                regerror(ret, &pattern, error, sizeof(error));
                fprintf(stderr, "%s\n", error);
                free(buf);
                return ret;
            }
        }
        else
            printf("%d:%s", i, buf);
    }

    free(buf);
    return 0;
}

void usage(char* prog_name)
{
    fprintf(stderr, "usage: %s [-i] [-E] pattern [ files ... ]\n", prog_name);
    exit(1);
}

int main(int argc, char **argv)
{
    FILE *fp = NULL;

    if (1 == argc)
        usage(argv[0]);

    int err = compile_pattern(argv[optind]);
    if (err)
        return err;
    else
        optind++;

    if (optind == argc) // no files, default to stdin
        process(stdin);
    else {
        for (int i = optind; i < argc; i++)
        {
            if (streq(argv[i], "-"))
                process(stdin);
            else if ((fp = fopen(argv[i], "r")))
            {
                process(fp);
                fclose(fp);
            }
            else
            {
                fprintf(stderr, "%s: could not open: %s\n", argv[i], strerror(errno));
                err = -1;
            }
        }
    }

    regfree(&pattern);
    return err;
}

Any suggestions?

On the whole your code seems pretty concise and clean.  I've just got a couple of side points.
Scope creep
This is a real problem in production code, where developers implement stuff that isn't really needed.  Your specification you've posted says 'take an exact string and an exact filename'.  You've allowed a regular expression to be passed.  The original requirements could have been met by just using strstr which would have made the code quite a bit smaller.  You've used extra time and added extra complexity, is there a real benefit?
The scope creep also means that I can't pass the basic string "\n" into your parser and have it find it as the '\n' is interpretted by the regex parser.  So, with grep I can do:

grep -F "\n" someFile.h

And get lines with the text \n:

printf("some text\n");

Whereas running your code:

./a.exe "\n" someFile.h

I get every line that ends in a newline (i.e. every line of the file).
-h -i and arguments
If you're going to implement programs that take in arguments, it's a good idea to give the user some way to find out what the arguments mean.  When I use grep -i, I'm trying to do a case-insensitive search.  Your program advertises a '-i' option, however it doesn't do the same thing as grep.  Instead I get told it can't open a file:

$ ./a.exe -i if someFile.h
if: could not open: No such file or directory