Really fast Knight's Tour using concurrency and Goroutines In less that 5 seconds on a normal PC, I found the answer in simple brute-force. Before I met Go, I didn't believe that it will be so easy to use all the power of the PC by using Go Goroutines for concurrency and multithreading.

I tried exactly the same simple algorithm, without concurrency in Haskell and JavaScript. In Go it works more than 10x faster. Feedback is welcome.

package main

import (
    "fmt"
)

const (
    a         = 8
    b         = 8
    maxLevels = 64
)

func main() {
    c := make(chan [a][b]int)
    for x1 := 0; x1 < a; x1++ {
        for y1 := 0; y1 < b; y1++ {
            go func(x1 int, y1 int) {
                var chess [a][b]int
                var current int
                success := tryToMove(&chess, current, x1, y1)
                if success {
                    c <- chess
                }
            }(x1, y1)
        }
    }
    printChess(<-c)
}

func tryToMove(chess *[a][b]int, current int, x int, y int) bool {
    if x >= a || y >= b || x < 0 || y < 0 {
        return false
    }
    if chess[x][y] > 0 {
        return false
    }
    current++
    chess[x][y] = current
    if current == maxLevels {
        return true
    }
    if tryToMove(chess, current, x+2, y+1) {
        return true
    }
    if tryToMove(chess, current, x+2, y-1) {
        return true
    }
    if tryToMove(chess, current, x-2, y+1) {
        return true
    }
    if tryToMove(chess, current, x-2, y-1) {
        return true
    }
    if tryToMove(chess, current, x+1, y+2) {
        return true
    }
    if tryToMove(chess, current, x+1, y-2) {
        return true
    }
    if tryToMove(chess, current, x-1, y+2) {
        return true
    }
    if tryToMove(chess, current, x-1, y-2) {
        return true
    }
    chess[x][y] = 0
    current--
    return false
}

func printChess(chess [a][b]int) {
    for i := 0; i < a; i++ {
        fmt.Println(chess[i])
    }
}

Answer:

[64 53 20 43 22 55 32 29
[41 44 63 54 33 30 23 6]
[52 19 42 21 56 7 28 31]
[45 40 51 62 1 34 5 24]
[18 61 16 57 14 25 8 27]
[39 46 37 50 35 2 11 4]
[60 17 48 15 58 13 26 9]
[47 38 59 36 49 10 3 12]

Your code does the task at hand, and that's a good thing. It's pretty quick too, an that's great.

What your code lacks though, is a sense of readability, and maintainability. There are also a number of potential bugs, and some variable shadowing.

Slices

My initial beefs with the code are that it depends heavily on constants, and on fixed arrays, instead of slices. In general Go favors slices over arrays. Consider this method:

func printChess(chess [a][b]int) {

it requires a 2D array with very specific dimensions, you can't use that function to print different size boards in the same program. It would be better to have a slice:

func printChess(chess [][]int) {

That can print any 2D slices, not just arrays of a very specific dimension. It makes it reusable.

Changing to slice from array would be quite a change in your code, though.

Constants

Your use of constants is impacting the code in odd ways. I mentioned the array-to-slice above, but it also impacts things like passing pointers in to arrays instead of slices, etc.

The names of your constants are also "horrible", a, and b are ugly.

your maxLevels constant is a better name, but really it is a computed value: maxLevels = a * b. I would not use it at all as a separate constant.

Constants in general are a bad idea, unless they really are constants. The dimensions of the board should be passed around, or inferred, as parameters.

Channels

Your use of the result channel appears to be a good idea, but it runs the risk of failing when no result is found. The fact that the 8x8 board has at least one valid result means that you are lucky, and that the channel completes OK. If there was no solution, though, your code would hang indefinitely.

You would be better by having a combination of a waitgroup and closing the channel if/when the waitgroup completes.

it would be best to solve that problem in it's own method too. It makes the process easier to read.

Dangling go-routines

your code does not clean up any go-routines that are still working when the first solution is found. You should find a way to terminate these routines cleanly - this would make your code usable in an application that does not immediately exit on the first solution.

You can use a select-with-default on the channels to implement a killer.

select {
case <-killer:
    // we were killed
    ....
default:
    //not killed, so keep going
    .....
}

Closure Shadowing.

This code here is complicated and made worse by bad variable naming:

for x1 := 0; x1 < a; x1++ {
    for y1 := 0; y1 < b; y1++ {
        go func(x1 int, y1 int) {
            var chess [a][b]int
            var current int
            success := tryToMove(&chess, current, x1, y1)
            if success {
                c <- chess
            }
        }(x1, y1)
    }
}

The x1 and y1 variables are duplicated in different scopes. The closure pattern is the right pattern, but give your variables different, non-shadowing names. Also, why x1 and y1 instead of just x and y?

for x := 0; x < a; x++ {
    for y := 0; y < b; y++ {
        go func(startx int, starty int) {
            var chess [a][b]int
            var current int
            success := tryToMove(&chess, current, startx, starty)
            if success {
                c <- chess
            }
        }(x, y)
    }
}

The above makes it clear that startx and x are different variables.

Additionally, if you create a parameter-based closure for the x and y variables, you should probably be consistent and create it for the c channel too.

Rows vs. Colums

It's a good thing your board is square - you mix up the use of the a and b constants, and the use of the x and y coordinates. If you were to have a rectangular board, say 6x8, your would cause index-out-of-bounds exceptions. Hmmm, no, actually, it will work, but your logic of using x and y implies that x is a horizontal position, but really, x in your code is the vertical position, and y is the horizontal position.

a is the height of the board, and b is the width (at least, that's the way that the printChess will print it out.

If you use variable names that imply a secondary meaning (like x), then you should ensure that the secondary meaning is consistent with the code's implementation.

Verbose Knight "Moves"

Your code embeds the knight-move logic as a series of if statements like:

if tryToMove(chess, current, x+2, y+1) {
    return true
}
if tryToMove(chess, current, x+2, y-1) {
    return true
}
if tryToMove(chess, current, x-2, y+1) {
    return true
}
if tryToMove(chess, current, x-2, y-1) {
    return true
}
if tryToMove(chess, current, x+1, y+2) {
    return true
}
if tryToMove(chess, current, x+1, y-2) {
    return true
}
if tryToMove(chess, current, x-1, y+2) {
    return true
}
if tryToMove(chess, current, x-1, y-2) {
    return true
}

it is often neater to encode it as a set of values in a slice:

var (
    knightMoves = []struct{ drow, dcol int }{
        {-2, -1}, {-2, +1},
        {-1, -2}, {-1, +2},
        {+1, -2}, {+1, +2},
        {+2, -1}, {+2, +1},
    }
)

and then iterate that slice to get the moves:

for _, move := range knightMoves {
    if tryToMove(width, height, board, current+1, col+move.dcol, row+move.drow) {
        return true
    }
}

Conclusion

Putting all the above together, I came up with a main method that inputs the solution constraints and handles a non-solution too:

func main() {
    if soln := solveBoard(8, 8); soln != nil {
        printChess(soln)
    } else {
        fmt.Println("No solution")
    }
}

Additionally, the actual solving method handles all cases:

func solveBoard(width, height int) [][]int {
    attempts := width * height
    results := make(chan [][]int)

    var wg sync.WaitGroup
    wg.Add(attempts)
    killer := make(chan bool)

    defer close(killer)
    for x := 0; x < width; x++ {
        for y := 0; y < height; y++ {
            startx := x
            starty := y
            go func() {
                if ok, soln := solveBoardFrom(killer, width, height, startx, starty); ok {
                    results <- soln
                }
                wg.Done()
            }()
        }
    }
    go func() {
        wg.Wait()
        close(results)
    }()
    return <-results
}

Note the creation of the WaitGroup and the killer channel that allows you to capture the no-solution condition, and the early solution killer.

Also note the different mechanism for creating the closure. I am not recommending one way vs. the other, but creating the variables in the inside scope like startx := x allows you to access startx inside the goroutine without worrying about the scope, just like a parameter-based closure function.

The recursive function is simplified with the loop, but more complicated by the killer process:

func tryToMove(killer chan bool, width, height int, board [][]int, current int, col int, row int) bool {
    // check to see whether we have been killed
    select {
    case <-killer:
        // we were killed
        return false
    default:
        //not killed, so keep going
    }

    if col < 0 || row < 0 || col >= width || row >= height {
        return false
    }
    if board[row][col] > 0 {
        return false
    }
    board[row][col] = current
    if current == width*height {
        // solved
        return true
    }
    for _, move := range knightMoves {
        if tryToMove(killer, width, height, board, current+1, col+move.dcol, row+move.drow) {
            return true
        }
    }

    board[row][col] = 0
    return false
}

It has a lot of parameters. It is past the point where a structure may be a better solution, but I left it as a lot of parameters for you to correlate things back to your code better.

Playground

You can see this running in the playground: https://play.golang.org/p/5zxmj8WV5u

The full code I am playing with is:

package main

import (
    "fmt"
    "sync"
)

var (
    knightMoves = []struct{ drow, dcol int }{
        {-2, -1}, {-2, +1},
        {-1, -2}, {-1, +2},
        {+1, -2}, {+1, +2},
        {+2, -1}, {+2, +1},
    }
)

func main() {
    if soln := solveBoard(8, 8); soln != nil {
        printChess(soln)
    } else {
        fmt.Println("No solution")
    }

    fmt.Println("-------------------")

    if soln := solveBoard(7, 9); soln != nil {
        printChess(soln)
    } else {
        fmt.Println("No solution")
    }

    fmt.Println("-------------------")

    if soln := solveBoard(2, 2); soln != nil {
        printChess(soln)
    } else {
        fmt.Println("No solution")
    }

}

func solveBoard(width, height int) [][]int {
    attempts := width * height
    results := make(chan [][]int)

    var wg sync.WaitGroup
    wg.Add(attempts)
    killer := make(chan bool)

    defer close(killer)
    for x := 0; x < width; x++ {
        for y := 0; y < height; y++ {
            startx := x
            starty := y
            go func() {
                if ok, soln := solveBoardFrom(killer, width, height, startx, starty); ok {
                    results <- soln
                }
                wg.Done()
            }()
        }
    }
    go func() {
        wg.Wait()
        close(results)
    }()
    return <-results
}

func tryToMove(killer chan bool, width, height int, board [][]int, current int, col int, row int) bool {
    // check to see whether we have been killed
    select {
    case <-killer:
        // we were killed
        return false
    default:
        //not killed, so keep going
    }

    if col < 0 || row < 0 || col >= width || row >= height {
        return false
    }
    if board[row][col] > 0 {
        return false
    }
    board[row][col] = current
    if current == width*height {
        // solved
        return true
    }
    for _, move := range knightMoves {
        if tryToMove(killer, width, height, board, current+1, col+move.dcol, row+move.drow) {
            return true
        }
    }

    board[row][col] = 0
    return false
}

func solveBoardFrom(killer chan bool, width, height, col, row int) (bool, [][]int) {
    board := make([][]int, height)
    for i := range board {
        board[i] = make([]int, width)
    }
    if tryToMove(killer, width, height, board, 1, col, row) {
        return true, board
    }
    return false, nil
}

func printChess(board [][]int) {
    for r := 0; r < len(board); r++ {
        fmt.Println(board[r])
    }
}


Split the array so that the sum of the numbers on one side of the split equals the sum of the numbers on the other side I would really appreciate if you can help me to find a better way to do it.

public class SplitArray
{

    public SplitArray() { }

    public bool CanArrayBeSplit(int[] intArray)
    {
        foreach(var pair in GeneratePairs(intArray))
        {
            if (pair.Item1 == pair.Item2)
                return true;
        }

        return false;
    }

    private List<Tuple<int, int>> GeneratePairs(int[] integerArray)
    {
        var output = new List<Tuple<int, int>>();
        for (int i = 0; i < integerArray.Length -1; i++)
        {
            output.Add(Tuple.Create(GetSum(0, i+1, integerArray), GetSum(i + 1, integerArray.Length ,integerArray)));
        }
        return output;
    }

    private int GetSum(int startIndex, int endIndex , int[] array)
    {
        int sum = 0;

        for (int i = startIndex; i < endIndex; i++)
        {
            sum += array[i];
        }

        return sum;
    }
}

The solution to this problem is easy, once you know the trick.

The trick, is to calculate the sum of all values then do add/subtract until they match...

This is very different to what you have done, so it does not really make sense to review your algorithm, and then say 'you did it wrong'.

What I will do, is say your code is neat enough, and the logic you have done is visible enough, to make the algorithm clear. Even though it is not the best algorithm, you have implemented it well.

The problems with your algorithm are that you are doing the following:

you are calculating the sums of each side of the split each time
you are calculating all possible splits, even if you could identify a successful split sooner
The Pairs and other classes are overkill for the solution you could have....

So, the following code (on ideone too) makes the 'simple' solution 'obvious'....

public static bool CanArrayBeSplit(int[] intArray) {
    int sum = 0;
    foreach (int v in intArray) {
        sum += v;
    }
    int left = 0;
    int right = sum;
    for (int i = 0; left != right && i < intArray.Length; i++) {
        left += intArray[i];
        right -= intArray[i];
    }
    return left == right;
}
public static void Main()
{
    Console.WriteLine("Testing: " + CanArrayBeSplit(new int[]{1,2,3,4,5,6,7,8,9}));
    Console.WriteLine("Testing: " + CanArrayBeSplit(new int[]{1,2,3,4,5,6,7,8,9,45}));

}


Simple Python Console Game With Inefficient Code Flow I made a simple program as an exercise in improving my code flow in Python, which does the following:

asks user to input their name
presents them with a choice to either print their name in reverse, or print the number of characters
after the output is printed, the user is asked if they want to "play again".  if not, exits

Have I done anything especially goofy?  Any low hanging fruit?  Best practice I'm unaware of?
from sys import exit

def userinput():
    print('Enter your name:')
    n = input()
    print('Hello, ' + n + ', would you rather (R)everse or (C)ount the letters of your name?')
    a = input()
    return (a,n)

def choosef(choice,myname):
    while choice not in ['R','C']:
        print("Only 'R' or 'C' are valid. Try again:")
        choice = input()
    if choice == 'R':
        print(''.join(reversed(myname)))
    else:
        spaces = 0
        for c in list(myname):
            if c == ' ':
                spaces = spaces + 1
        print('Total # Characters: ' + str(len(myname) - spaces))
    playagain()

def playagain():
    print("Would you like to play again, (Y)es or any other character to exit?")
    yn = input()
    if yn == 'Y':
        ui = userinput()
        choosef(ui[0],ui[1])
    else:
        exit()

ui = userinput()
choosef(ui[0],ui[1])

Try to forget everything that you know about the code, and assume that you aren't the author. Imagine yourself to be a third person reading the code, he/she knows nothing about it and just reading it for the first time.
ui = userinput()
choosef(ui[0],ui[1])

Alright, userinput() must take some input and put it into ui. Makes sense, but what is choosef()?.
Naming convention
Use meaningful names for functions and variables. take this example
x = 3.14
y = 24

what is x and y to the reader? Is that the price of something?
pi_value = 3.14
number_of_hours = 24

Ahhh okay, now I know exactly what 3.14 means. So If I use it somewhere the reader will also know what I exactly mean. This is an important part of writing clean code.
Another aspect is the style. There are a few ways I can write user input*
user_input()
USERINPUT()
UserInput()
userinput()

Which one should I follow?
To have a consistent naming convention, python code follows the PEP-8 naming convention. When I say follows, I mean that it is recommended that you should follow it too as other python libraries also use this. It makes the code look cleaner.
Things like functions follow : lower_snake_case
Classes follow: CamelCase
You can read the link for more information.
Taking input in Python
print('Enter your name:')
n = input()

Clearly, you want to display a message to the user before he enters something. This is why the input() function has something called input prompt.
name = input("Enter your name: ")
You can display the message between the (). This removes the additional line. Also note that I changed n to name for the reasons I mentioned above.
Formatting strings
From your code, I can see that you concatenated strings with + to form meaningful sentences. That works but there is a huge problem when you want to use variables of different types.
name = "Eric"
age = 14
job = "Comedian"
print("Hello " + name  + "You are " + age + " years old and you are a " + comedian)

TypeError: can only concatenate str (not "int") to str

Simply use Python 3's f-strings. Place the letter 'f' before "
print(f"Hello {name}, you are {age} years old ")

Cleaner.
return value from userinput()
What you are currently doing is returning a tuple of choice and name. It's better that you keep them as they are because later on, you slice from it just to get them again.
This means you directly do
return name, choice

# Calling the function

name, choice = userinput()

Code structure
Couple of points

Do not ask "Do you want to play again? " in the play_again() function. The reason you called that function should be because the user wants to play again. Move that to a playgame() function which will call user_input() every time the user wants to play the game, and break from the loop only if the user enters 'n' or "no"

spaces = spaces + 1 can be simplified into spaces += 1

Move the part where you count the number of characters into a separate function which will return an integer. So your play_game() function doesn't do anything other than play the game. When you need the characters , number_of_char = character_len( name ).

Use enum for clarity.

An improved version of the code
from enum import Enum

class PrintChoices(Enum):
    number_of_char = 'c'
    reversed_name = 'r'
    exit_choice = 'e'

def find_num_of_char(name):
    return len(name) - name.count(' ')

def reverse_name(name):
    return name[::-1]

def user_input():
    name = input("Enter your name: " )
    choice = input("Would you like to (r)everse your name\n or would you like to print the number of (c)haracters or (e)xit?: ")
    return name, choice

def clear_screen():
    print(chr(27) + "[2J")

def play_game():
    while True:
        clear_screen()
        name, choice = user_input()
        if choice == PrintChoices.reversed_name.value:
            print(reverse_name(name))
            input("Press any key to continue...")
        elif choice == PrintChoices.number_of_char.value:
            print(find_num_of_char(name))
            input("Press any key to continue...")
        elif choice == PrintChoices.exit_choice.value:
            break
        else:
            input("Invalid input, Press any key to continue...")

play_game()

Note: I have also added print(chr(27) + "[2J") to clear the screen.


Algebraic Effect Monad to separate pure and impure code I want to write my C# code in a way which makes effects (state, I/O, etc) explicit in the type signature. I have started to understand monads, and have some acquaintance with algebraic effects, though not deeply so. I have attempted to implement a monad for working with algebraic effects in C#. The purpose is to allow you to provide an impure function which is an interpreter over an algebraic representation of your effects at the time that you run the monad, but not before. In theory this allows you to write all of your code in a pure way, and even pass a pure function in for the effects for the sake of unit testing etc. The monad is intended to be generic with respect to what types you use to represent your effects, and it is built with a struct in order to not be nullable.

public struct Eff<TOut, TIn, TReturn>
{
    private readonly Func<Func<TIn, TOut>,TReturn> _wrappedParameter;

    private Eff(Func<Func<TIn, TOut>, TReturn> wrappedParameter)
    {
        _wrappedParameter = wrappedParameter;
    }

    public static Eff<TOut,TIn,TReturn> Return(TReturn obj)
    {
        return new Eff<TOut,TIn,TReturn>(_ => obj);
    }

    public static Eff<TOut,TIn,TReturn> Do(TIn input, Func<TOut,TReturn> continuation)
    {
        return new Eff<TOut, TIn,TReturn>(f => continuation(f(input)));
    }

    public TReturn Run(Func<TIn, TOut> f)
    {
        return _wrappedParameter(f);
    }

    public Eff<TOut, TIn, TReturn2> Map<TReturn2>(Func<TReturn, TReturn2> g)
    {
        var wrappedParameter = _wrappedParameter;
        return new Eff<TOut, TIn, TReturn2>(f => g(wrappedParameter(f)));
    }

    public Eff<TOut,TIn,TReturn2> Bind<TReturn2>(Func<TReturn, Eff<TOut,TIn,TReturn2>> continuation)
    {
        var wrappedParameter = _wrappedParameter;
        return new Eff<TOut, TIn, TReturn2>(f => continuation(wrappedParameter(f)).Run(f));
    }
}

There are several things that could be brought up here (he concepts are so new with me that I have named things poorly; I haven't yet figured out how it would look to have multiple effect types playing together in your program; etc), but there's one thing in particular I am concerned about, which is that I am nervous that if an entire program were written inside this monad, it would have the potential to overflow the .NET stack. This is where I am most interested in getting a review. I believe that if you implement a computation expression in F# it ends up executing in a trampoline monad to keep the stack from overflowing. I am particularly concerned about this in a somewhat resource-constrained environment, say an old Windows CE device running Compact Framework 3.5 (which is actually a target platform for me).

This is not a direct and full answer, but whereas I did not end up getting the opportunity I was hoping for to try this out on a C# project (yet), I did hit a perfect case for one in an F# project. I rewrote this monad in F#, implemented some concrete operations with it, and used it. I have it running in production with no problems so far, but it isn't using the constrained environment that I was describing in the question, so it hasn't given me full confidence in that yet.

After having done that, there are two main criticisms I have of the version above:

It's not very flexible. It really only allows you to have effects that conform to one type (Func<TIn,TOut>). Moreover it really only allows you to have one effect of this type.
The three generic types are overkill and not necessary. The alternative will be demonstrated below.

What I came up with was this generic effect monad:

module Effect

type Eff<'E,'a> =
     private EffReq of ('E -> 'a)

let map (f : 'a -> 'b) (EffReq p : Eff<'E,'a>) : Eff<'E,'b> =
    EffReq (f << p)

// Argh F# blocks us from using unit, pure, or return...!
let init (x : 'a) : Eff<'E,'a> =
    EffReq (fun _ -> x)

let run (io : 'E) (EffReq p : Eff<'E,'a>) : 'a =
    p io

let join (e : Eff<'E,Eff<'E,'a>>) : Eff<'E,'a> =
    EffReq (fun performEff -> run performEff e |> run performEff)

let bind (f : 'a -> Eff<'E,'b>) (e : Eff<'E,'a>) : Eff<'E,'b> =
    join <| map f e

let request = EffReq

Specific operations can be implemented using request, e.g.:

module FileIO

open Effect

type FileIO = {
    ReadAllText : string -> string
}

type IOEff<'a> = Eff<FileIO,'a>

let readAllText (path : string) : IOEff<string> =
    Effect.request (fun io -> io.ReadAllText path)

I know this is in a different language than the original question but the conceptual aspect is the most important part.

I don't consider this implementation fully battle-hardened but it should allow me to evolve the C# version in a good direction.

Note that this is really a Reader monad: it reads from an impure environment (and because that environment is impure, technically it is able to write to it as well).


Security of authentication check for a blog I am currently making a very big blogging system for my Website where I want really neat code! So, I was wondering: is this method safe/does it have any problems with security? Yes, I'll be encrypting the password anyway. I also want to know if there is a better way to achieve the same idea. Also any tips on implementing high security and high performance?

<?php
class Users {
    protected $db;
    public $id;
    public $username;
    public $password;
    public $lastlogin;
    public $email;
    public $rank;
    public $creationdate;
    public $country;
    public $status;

    public function __construct() {
        $this->db = new mysqli('localhost', 'root', 'php123', 'ht');
    }

    public function login($username, $password) {
        $query = $this->db->prepare("SELECT username, password FROM blog_users WHERE username = ? AND password = ? LIMIT 1");
        $query->bind_param('ss', $username, $password);
        $query->execute();
        $query->bind_result($username, $password);
        $query->store_result();
        if($query->num_rows == 1) {
            while($query->fetch()) {
                $userData = getData($username);

                $this->id             =   $this->userData['id'];
                $this->username       =   $username;
                $this->password       =   $password;
                $this->lastlogin      =   time();
                $this->email          =   $this->userData['email'];
                $this->rank           =   $this->userData['rank'];
                $this->creationdate   =   $this->userData['creationdate'];
                $this->country        =   $this->userData['country'];
                $this->status         =   $this->userData['status'];

                $_SESSION['loggedin'] = 1;
                return true;
            }
        } else {
            return false;
        }
        $query->close();
        $query->free_result();
    }

    public function getData($username) {
        $query = $this->db->prepare("SELECT id, email, rank, creationdate, country, status FROM blog_users WHERE username = ?");
        $query->bind_param('s', $username);
        $query->execute();
        $query->bind_result($id, $email, $rank, $creationdate, $country, $status);
        $query->store_result();
        $userData = array(
            'id'           => $id,
            'email'        => $email,
            'rank'         => $rank,
            'creationdate' => $creationdate,
            'country'      => $country,
            'status'       => $status
        );
        return $userData;
        $query->close();
        $query->free_result;
    }

}
?>

Ok, just a few quick thoughts, will be adding to this answer later today (ie after work):

One class, One task
Chances are, you've already heard of the SOLID principles. Well, here they are again: your code is in violation of the SRP (Single Responsability principle). A class has one task. No more, no less. Your class does 4 things: it stores data (all those public properties), messes with sessions, connects to the DB and performs queries.
The Database connection and queries are all things that should be dealt with in another class, that simply connects to the DB and executes queries.

This other class could then have a method like this:

public function login(User $user)
{
    $query = $this->db->prepare("SELECT username, password FROM blog_users WHERE username = ? AND password = ? LIMIT 1");
    $query->bind_param('ss', $user->username, $user->password);
    $query->execute();
    $query->bind_result($user->username, $user->password);
    $query->store_result();
    if($query->num_rows == 1)
    {
        $query->fetch();
        $query->close();
        return $this->getUserData($user);//changed name of method
    }
    $query->close();
    throw new RuntimeException('User can not login');//error
}

Even this isn't close to efficient code, because it still calls a second method to, basically, repeat the same query. Why would you do so, why not simply write:

public function login(User $user)
{
    $query = $this->db->prepare(
        "SELECT id, email, rank, creationdate,
            country, status, username, password
        FROM blog_users
        WHERE username = ? AND password = ? LIMIT 1"
    );
    $query->bind_param('ss', $user->username, $user->password);
    $query->execute();
    $query->bind_result(
        $user->id,
        $user->email,
        $user->rank,
        $user->creationdate,
        $user->country,
        $user->status,
        $user->username,
        $user->password
    );
    $query->store_result();
    if($query->num_rows == 1)
    {
        $query->fetch();
        $query->free_result();//call free_result...
        $query->close();//and close here, too: your code doesn't do this
        return $user;
    }
    $query->free_result();
    $query->close();
    throw new RuntimeException('User can not login');//error
}

Isn't that a lot shorter, clearer and doesn't this also do exactly what you are doing? K.I.S.S!

Classes with a dependency, should be open to injection
Your current code is hard to test: the constructor connects to the DB, there is no way for me (the user), or you (when testing) to mock the db connection, or use a bogus testing table.
The best way to make your class easier to test it to allow the user to pass a specific connection to the constructor, and then he can use your class/functionality to work on the db of his choice:

public function __construct(mysqli $connection = null)
{//default null means the argument is optional
    if ($connection === null)
        $connection = new mysqli();//connect to default
    $this->db = $connection;//use connection we've created, or the one the user passed through to the constructor
}

And of course: a DB connection that is opened, should be closed: when your class dies (is destructed), the DB connection should die, too:

public function __destruct()
{
    $this->db->close();
    $this->db = null;
}

That should be added to your code, too. If you don't then the connection will be kept open until your script has terminated, which could be longer than you want. If performance and cleanliness matter to you, then this should be a habit.

Standards, coding style
Yes, I complain/nag a lot about coding style and standards but if you are, like you say, creating a sizeable code-base, then you should know that coding style and standards are insanely important.
The unofficial PHP standards can be found here. Though unofficial, all major players (ie Zend, Symfony, Cake, Doctrine, ...) all subscribe to them.

Other niggles:
Your class has tons of properties, and it's those properties that you are assigning in the methods. Why, then aren't you assigning $username as soon as you can, so you don't have to pass it to the getData method as an argument?

Another remark I have is simply this:
If a script contains only PHP code, then that script should not have a closing ?> tag. Get rid of it.


Watchdog to monitor a folder I am creating a watchdog in python to monitor a folder and then create a backup with the new version when a file is modified. I extend the FileSystsemEventHandler class to add 7 extra variables. However not every variable is used on every event so I'm not sure where to initialize the variables and where to put the logic for initializing certain variables based on the event. My current program works however I am new to coding and want to make sure I am using proper coding convention.
import os
import getpass
import re
import glob
import sys
import time
import pathlib
import logging
import shutil
import tkinter as tk
from tkinter import filedialog

from watchdog.observers import Observer
from watchdog.events import LoggingEventHandler, FileSystemEventHandler

SLEEP_TIME = 30

class OnMyWatch:

    def __init__(self, watch_src, watch_dest):
        self.observer = Observer()
        self.watch_src = watch_src
        self.watch_dest = watch_dest

    def run(self):
        event_handler = Handler(watch_src, watch_dest)
        self.observer.schedule(event_handler, watch_src, recursive = True)
        self.observer.start()
        try:
            while True:
                time.sleep(SLEEP_TIME)
        except:
            self.observer.stop()
            print("Observer Stopped")

        self.observer.join()

class Handler(FileSystemEventHandler):
    def __init__(self, watch_src, watch_dest):
        self.watch_src = watch_src
        self.watch_dest = watch_dest

        self.src_dir = ''
        self.src_name = ''
        self.dest_dir = ''
        self.file_extension = ''

    # METHOD DONE
    def on_any_event(self, event):
        if event.event_type == 'moved':
            self.src_dir = os.path.join(self.watch_dest, os.path.relpath(event.src_path, self.watch_src))
            self.dest_dir = os.path.join(self.watch_dest, os.path.relpath(event.dest_path, self.watch_src))

        else:
            if event.is_directory:
                self.src_dir = event.src_path
                self.dest_dir = os.path.join(self.watch_dest, os.path.relpath(event.src_path, self.watch_src))

            else:
                src_path, self.file_extension = os.path.splitext(event.src_path)
                self.src_name = os.path.basename(src_path)
                self.src_dir = os.path.dirname(src_path)
                self.dest_dir = os.path.join(self.watch_dest, os.path.relpath(src_path, self.watch_src))

    def on_created(self, event):
        # CREATED EVENT

    def on_modified(self, event):
        # MODIFIED EVENT

    def on_moved(self, event):
        # MOVED EVENT

    def on_deleted(self, event):
        # DELETED EVENT

if __name__ == "__main__":
    root = tk.Tk()
    root.withdraw()

    watch_src = filedialog.askdirectory()
    watch_dest = filedialog.askdirectory()

    watch = OnMyWatch(watch_src, watch_dest)
    watch.run()

Wait loops
I don't think that this:
    try:
        while True:
            time.sleep(SLEEP_TIME)
    except:
        self.observer.stop()
        print("Observer Stopped")

should be necessary. I would expect that if you simply join, it would do what you want - block until the observer is done.
Even if you retained the above structure, you're not using your try correctly. That except should be a finally. In other words, you don't want to stop when something goes wrong; you want to stop whether something goes wrong or not.
Pathlib
on_any_event contains a lot of os.path calls. You'll find that replacing these with pathlib.Path equivalents is better-structured. It's a more object-oriented and "syntactically sugary" way of manipulating paths.
Overall
I'm not convinced that writing these generic watchdog wrappers (Handler, OnMyWatch) is useful. It's subtracting from the utility of watchdog. What if you want code of your own to run once Handler.on_any_event changes some of its attributes? You could subclass Handler and override on_any_event, calling super().on_any_event to set those attributes, but that's a fairly poor data-passing pattern.
You'd be better off writing a purpose-built FileSystemEventHandler that does not have those file-component attributes, does not look at event.event_type, and handles specifically the event you care about.
In other words, this is abstraction that hurts rather than helps you, and introduces functions with side-effects that set attributes that don't make sense as attributes.


The console prints a list of items for the user to scroll through using the up and down arrow keys I am somewhat new to python (yes I know, a cliché statement) and I wrote this code to scroll through a tuple of strings using the arrow keys. I know that the code is sloppy but I don't know how. Any feedback would be greatly appreciated.
import keyboard
from os import system
from time import sleep

class Menu:
    def __init__(self, *items):
        self.items = items
        self.selections = 6
        self.start = 0
        self.stop = self.start + self.selections
        self.slice = items[self.start:self.stop]
        self.selected = 0
        self.beg = '>'
        self.end = '<'

    def get_input(self):
        if keyboard.is_pressed('up'):
            if self.selected >= 0:
                self.selected += -1
            self.show()
        elif keyboard.is_pressed('down'):
            if self.selected < self.selections:
                self.selected += +1
            self.show()

    def show(self):
        system('cls')

        # shift slice up if the bottom of list is reached
        if self.selected == self.selections:
            if self.stop < len(self.items):
                self.start += 1
                self.stop += 1
                self.slice = self.items[self.start:self.stop]
            self.selected = self.selections-1

        # shift slice down if top of list is reached
        elif self.selected < 0:
            if self.start > 0:
                self.start += -1
                self.stop += -1
                self.slice = self.items[self.start:self.stop]
            self.selected = 0

        # print the slice with the selected item printed like >this<
        for i in range(len(self.slice)):
            if i == self.selected:
                print(self.beg + self.slice[i] + self.end)
            else:
                print(self.slice[i])

test_menu = Menu('item 0',
                 'item 1',
                 'item 2',
                 'item 3',
                 'item 4',
                 'item 5',
                 'last item')
test_menu.show()

while 1:
    test_menu.get_input()
    sleep(0.1)

get_input should be called handle_input. get_input suggests that it returns input values, whereas really, it's a "step" function used to handle what's going on at a slice in time.
Also in that function, self.selected += -1 is confusing. Why add a negative number instead of subtracting? I would change that and the other similar instances to self.selected -= 1. Personally, I'd also get rid of the prefix + in +1. I don't find it adds anything.

Sorry, I wrote another review before this and my brain's tired. Hopefully someone else can give better suggestions.


Is a unit test which uses LINQ to check multiple values acceptable? I'm writing unit tests for a library which parses colors from user input (example: the user input “#f00” gives a red color; the user input “60,100%,100%” gives a yellow color, etc.)

One of the conditions is that the method Parse returns null when there is actually nothing to parse, i.e. when the input is either an empty string or its length is inferior to four characters. Those are the two unit tests:

[TestMethod]
public void SubmitEmptyText()
{
    var actual = new ColorParser().Parse(string.Empty);
    Assert.IsNull(actual);
}

[TestMethod]
public void SubmitShortText()
{
    Assert.IsTrue(new[]{"#", "#f", "#ff"}.All(c => !new ColorParser().Parse(c).HasValue));
}

Is the second method doing too much?
Am I expected to create three method? Seriously?!
Is the usage of functional programming here justified, or should I instead stick to the most basic way to write the tests?

What you need is passing parameters to common test method and supplying it with informative error message.

    [TestMethod]
    public void SubmitShortText()
    {
        var colorCodes = new[] { "#", "#f", "#ff" };
        DoAssertion(colorCodes);
    }

    private void DoAssertion(string[] colorCodes)
    {
        var parser = new ColorParser();
        foreach (var code in colorCodes)
        {
            var hasValue = parser.Parse(code).HasValue;
            Assert.IsTrue(hasValue, string.Format("Failed to parse color code '{0}'.", code));
        }
    }

Parametrized tests are also supported in XUnit:

    [Theory]
    [InlineData("#")]
    [InlineData("#f")]
    [InlineData("#ff")]
    public void SubmitShortText(string code)
    {
        var parser = new ColorParser();
        var hasValue = parser.Parse(code).HasValue;
        Assert.True(hasValue, string.Format("Failed to parse color code '{0}'.", code));
    }

Tests will be executed one by one. Execution will be stopped on the first failed test.


Computing mean and median I have written a program to compute mean and median. Please review my code and give suggestions.

I have a confusion about this temps[temps.size()/2] when temps.size() is an odd number. How does the "vector::operator[]" handle floating point numbers? From the behavior in my code, I guessed it rounds to the nearest integer.

Note: I am self learning programming by reading "Programming: Principles, and Practice" by Bjarne Stroustrup. Currently, I am on chapter 4.

#include <iostream>
#include <vector>
#include <algorithm>

int main()
{
    std::vector<double> temps;          // temperatures
    for (double temp; std::cin>>temp; ) // read into temp
        temps.push_back(temp);      // put temp into vector

    // Compute Mean temperature:
    double sum = 0;
    for (int x : temps) sum += x;
    std::cout << "Average temperature: " << sum/temps.size() << '\n';

    // sort temperatures
    std::sort(temps.begin(),temps.end());

    // Compute Median temperature
    if(temps.size()%2==1) //Number of elements are odd
    {
        std::cout << "Median temperature: " << temps[temps.size()/2] << '\n';
    }
    else // Number of elements are even
    {
        int index = temps.size()/2;
        std::cout<< "Median temperature: " << (temps[index-1] + temps[index])/2;
    }

}

I have a confusion about this temps[temps.size()/2] when temps.size() is an odd number. How does the "vector::operator[]" handle floating point numbers?

There are well defined rules to implicit conversions.  temps.size() returns an unsigned integral type that represents the total count of elements in the container.  Dividing an unsigned integral by 2 (a signed integral), the implicit conversion rules for mixed expressions promotes the signed 2 to be unsigned. Division between two unsigned integrals results in an unsigned integral and any remaining fractional part is truncated.  So \$3/2 = 1\$ and the remaining \$.5\$ is truncated and discarded.

    std::vector<double> temps;
    for (double temp; std::cin>>temp; )
        temps.push_back(temp);
    // temps = ???

Be aware of the possible states a type may represent.  What happens if someone runs the program and inputs no data?  Boom.  The mean cannot be calculated because division-by-zero is undefined behavior.  The median cannot be calculated as an empty set is consider an even-sized set.  temps[0-1] accesses results in an out-of-bound access, which is also undefined behavior.  Explicitly assert/validate your pre- and post-conditions.  Do not rely on assumptions.

    for (int x : temps) sum += x;

Use auto to avoid redundant repetition of type names.  temps represents a container of doubles.  You define x to be of type int, which results in a narrowing conversion that's likely to be a source of a bug (Test! Test! Test!).  The compiler already knows what type x should be.  Use auto and let the compiler do its job.

Don't state in comments what can be clearly stated through code.  When programming, the code states what is being done.  If you need to provide/document any reasoning regarding your intent, use comments then.

Write functions. Lots of functions.  You've already split your program into single logical operations via comments.  Break up that monolithic main into parameterized functions.

int main() {
    auto temperatures = read_temperatures(std::cin);
    const auto mean_temperature = mean(temperatures);
    const auto median_temperature = median(temperatures);

    report(std::cout, mean_temperature, median_temperature);
}

Now you can write the short and simple functions that are easy to read, test, and maintain.

Get familiar with the standard algorithms found in <algorithm> and <numeric>.  Do you really need to sort the entire data set?  Would partial sorting (std::nth_element) suffice?  For even-sized data sets, you could pair up std::nth_element with std::min_element to find the two values. As Vnp mentioned, std::accumulate is a candidate for summing up a range.

Use strongly defined types to represent your temperatures.  A raw double representing temperatures is not safe.  Numeric types that are conceptually incompatible may cause catastrophic mathematical errors.  The Mars Climate Orbiter failure is a famous catastrophe caused by taking an impulse result from the US system and using that result in SI calculations without any conversions.  So if you want to store temperatures, are you representing Celsius, Fahrenheit, Kelvin, or Rankine?  Note - You'll need an understanding of programming with objects to accomplish this.  If you haven't learned about objects yet, keep it in mind for when you do encounter them.


Encrypt texts, with saved password using pycrypto I have the code bellow, which is supposed to be used inside a larger program.
Please see notes about the requirements below the code.

from __future__ import print_function
from Crypto.Cipher import AES
from Crypto.Protocol.KDF import PBKDF2
import base64
import os
import sys
import binascii

if sys.version_info.major > 2:
    raw_input = input

EncodeAES = lambda c, s: base64.b64encode(c.encrypt(s))
DecodeAES = lambda c, e: c.decrypt(base64.b64decode(e)).rstrip()

def get_digest(password, salt):
    """
    Get a digest based on clear text password
    """
    iterations = 5000
    return PBKDF2(password, salt, dkLen=32, count=iterations)

def authenticate(password, salt, digest):
    """
    salt and digest are stored in a file or a database
    """
    dig = get_digest(password, salt)
    return binascii.hexlify(dig) == digest

def write_password():
    """
    Write a secret password as a hash and the salt used for this hash
    to a file
    """
    salt = base64.b64encode(os.urandom(32))
    passwd = raw_input("Please type in the secret key:")
    key = get_digest(passwd, salt)
    f = open('passwords.txt', 'wt')
    hpk = salt+'$6$'.encode('utf8')+binascii.hexlify(key)
    f.write(hpk.decode('utf-8'))
    f.close()

def get_digest_from_file(filename):
    """
    Read a digested password and salt from the file
    """
    f = open(filename, 'rt')
    sf = f.readline()
    f.seek(0)
    salt, digest = f.readline().split('$6$')
    return salt.encode('utf-8'), digest.encode('utf-8')

def get_cipher(password, salt):
    """
    Create a chiper object from a hashed password
    """
    iv = os.urandom(AES.block_size)
    dig = get_digest(password, salt)
    chiper = AES.new(dig, AES.MODE_ECB, iv)
    return chiper

def cli_auth():
    """
    Read password from the user, if the password is correct,
    finish the execution an return the password and salt which
    are read from the file.
    """
    salt, digest = get_digest_from_file('passwords.txt')
    while True:
        password = raw_input("Please type in your password:").encode('utf-8')
        if authenticate(password, salt, digest):
            return password, salt

def prepare_data(text, block_size):
    """
    prepare data before encryption so the lenght matches the expected
    lenght by the algorithm.
    """
    num_blocks = len(text)//block_size + 1
    newdatasize = block_size*num_blocks
    return text.ljust(newdatasize)

def save_a_secret_message():
    """
    PoC to show we can encrypt a message
    """
    secret_msg = """This is a very important message! Learn Cryptography!!!"""
    # the secret message will be encrypted with the secret password found
    # in the file
    passwd, salt = cli_auth()
    cipher = get_cipher(passwd, salt)
    # explictly destroy password, so now there is no clear text reference
    # to the input given by the user
    del(passwd)
    msg = EncodeAES(cipher, prepare_data(secret_msg, AES.block_size))
    with open('secret.enc','wt') as s:
        s.write(msg.decode())
    print("The cipher message is:", msg.decode())

def read_a_secret_message():
    """
    PoC to show we can decrypt a message
    """
    passwd, salt = cli_auth()
    cipher = get_cipher(passwd, salt)
    del(passwd)
    with open('secret.enc') as s:
        msg = s.readline()
        print("The decrypted secret message is:")
        decoded = DecodeAES(cipher, msg)
        print(decoded)

if __name__ == '__main__':
    if '-i' in sys.argv:
        write_password()
    save_a_secret_message()
    read_a_secret_message()

The user is supposed to enter is password only once.
but decryption and  encryption take place many times while the program is running.
There should be a mechanism to save the password (or a digest) on the disk (not in clear text).
Should be Python2 and Python3 compatible.

So far, it seems to me, it all works OK. I tested python2 and python3. The cipher text can be decrypted with different Python versions too.

However, I am not a Crypto expert, and I would definitely like to here some criticism about this code.

Be careful naming your functions
You declare a lambda function named EncodeAES, but this function itself doesn't know anything about AES. It accepts an object with an encrypt() member function which it calls, but that object could be anything.
The name should reflect what the function does. This one does two things: it base64-encodes, and it calls encrypt(). I think any descriptive name would be really awkward, maybe it is best not to use these lambda's at all, especially since they are only used once.
Don't modify the message before/after encryption
Your DecodeAES lambda calls rstrip() on the decrypted message. But what if the original text has whitespace of its own at the end of the message that is significant? Or what if the message is not a piece of text but a binary file? Then the output after decryption will be different from the message before encryption. It also allows for length extension attacks.
Don't use ECB mode
You should never use ECB mode to encrypt things, it has many issues such as revealing patterns in the plaintext, and allowing an attacker to flip bits in the plaintext at will.
Almost any other mode would be suitable to use, if you add a unique initialization vector (IV) to the ciphertext. Note that this IV should be different each time you write an encrypted file, otherwise it might make it easy for someone to decrypt parts of the message by comparing two ciphertexts.
Even better would be to use an authenticated encryption mode to ensure that if an attacker has tampered with the ciphertext, this is detected when trying to decrypt the message. If you don't provide any form of authentication of the ciphertext, it allows an attacker to modify it, and then the output after decryption will be modified as well, sometimes in ways that are not easy to spot.
Don't use home-grown crypto in production code
While it is perfectly fine to try to implement your own encryption protocols (it's a good way to start learning about it), it is very easy to make mistakes, and mistakes will be costly (since there probably is a good reason why you want to keep the data private).
Go to  https://crypto.stackexchange.com/ and search there for how to safely encrypt files. You'll likely find many answers like this one that tell you to not try it yourself and rather use existing, well-established software to do encryption for you. If you don't want to become a crypto expert, then follow that advice. If you really do want to become better at crypto, then have a look at the well-established software and protocols out there, and try to learn how they work.
Use a higher-level cryptography library
The Crypto package is quite low-level. It provides you the basics of encryption, but already for properly encrypting a file you need to combine several techniques. It is better to use a library that provides higher level functions that take care of all the details for you. There are many of them out there. A popular one is PyNacl.


Angular 2 Component conditionally displaying a list of elements from an array This Angular component displays a list of polls that are fetched from the server and has a slide toggle giving the authenticated user the option to display all polls or just the ones that the user has authored. I use Angular Material and Angular Flex Layout.

I'm specifically looking for feedback regarding best practices and how I handled displaying polls conditionally, but welcome all feedback. I chose to keep most of the conditional logic in the component instead of in the template but I'm not sure that is in accordance with Angular best practices.

poll-list.component.html

  <md-slide-toggle *ngIf="isLoggedIn()" (change)="displayMyPolls()" fxFlexAlign="center">Show only my polls</md-slide-toggle>
  <md-nav-list fxFlexAlign="center">

      <md-icon md-list-icon>poll</md-icon>

{{poll?.question}}

{{poll?.getTotalVotes()}} votes

  </md-nav-list>

poll-list.component.ts

import { Component, OnInit } from '@angular/core';

import { PollService } from './poll.service';
import { AuthService } from '../auth/auth.service';
import { Poll } from './poll.model';

@Component({
  selector: 'app-poll-list',
  templateUrl: './poll-list.component.html',
  styleUrls: ['./poll-list.component.css']
})
export class PollListComponent implements OnInit {
  polls: Poll[] = [];
  pollsToDisplay: Poll[] = [];
  showOnlyMyPolls = false;

  constructor(private pollService: PollService, private authService: AuthService) { }

  ngOnInit() {
    this.pollService.getPolls()
      .subscribe(
                (polls: Poll[]) => {
                    this.polls = polls;
                    this.pollsToDisplay = polls;
                }
            );
  }

  displayMyPolls() {
    this.showOnlyMyPolls = !this.showOnlyMyPolls;
    if (this.showOnlyMyPolls) {
      this.pollsToDisplay = this.polls.filter((poll) => {
        return this.authService.isLoggedInUser(poll.userId);
      });
    } else {
      this.pollsToDisplay = this.polls;
    }
  }

  isLoggedIn() {
    return this.authService.isLoggedIn();
  }

}

Naming: A better name for the change event handler is toggleShowOnlyMyPolls().
TypeScript is about types: I'm trying not to forget specifying return types of the methods -- this makes it easy to inspect/scan over the signatures, and gives compile time support.
pollsToDisplay can be relatively cheaply calculated on the fly. That will simplify code significantly at the cost of performance (which is not an issue as long as you don't deal with at least several hundred of Poll objects).

Propose:

export class PollListComponent implements OnInit {
    showOnlyMyPolls = false;
    polls: Poll[] = [];

    get pollsToDisplay(): Poll[] {
        if (!this.showOnlyMyPolls)
            return this.polls;

        const loggedInUserId = this.authService.getLoggedInUserId();
        return this.polls.filter(poll => poll.userId === loggedInUserId);
    };

    constructor(private pollService: PollService, private authService: AuthService) { }

    ngOnInit(): void {
        this.pollService
            .getPolls()
            .subscribe((polls: Poll[]) => this.polls = polls);
    }

    toggleShowOnlyMyPolls(): void {
        this.showOnlyMyPolls = !this.showOnlyMyPolls;
    }

    isLoggedIn(): boolean {
        return this.authService.isLoggedIn();
    }
}

How is this code simpler?

pollsToDisplay used to be a field that is assigned in three different places: one in subscribe(...) and two in displayMyPolls(). Each of these places could have a bug, and this code was scattered. Now it's calculated on the fly:

no assignments at all;
everything happens in one place -- if there is a bug with poll list construction, you know where to look for it, easier to unit-test, etc.;
and there is no unnecessary field since get pollsToDisplay() is an accessor.

toggleShowOnlyMyPolls() became trivial, it really does only what it is responsible for, which is mode switching.


Finding the predominant number in an array My program has to read an array and say whether there is a predominant number or not in the array. A predominant number is the one with frequency bigger than number of elements of the array divided by 2. I did it, but the site I am learning on is dissatisfied with the program's speed so I do not have the maximum score. Not for that score I am asking this.

int main()
{
    int number, i, j, currentElAp = 1, maxAp = 0, maxApNumber = -1, remainingElements;
    scanf("%d", &number);
    int ar[number];
    for(i = 0; i < number; i++)
        scanf("%d", &ar[i]);

    for(i = 0; i < number / 2; i++){
        remainingElements = number - i - 1;
        if(ar[i] != maxApNumber){
            for(j = i + 1; j < number && (remainingElements + currentElAp > number / 2); j++){
                if(ar[i] == ar[j])
                    currentElAp++;
            }
        }
        if(currentElAp > maxAp){
            maxAp = currentElAp;
            maxApNumber = ar[i];
        }
        currentElAp = 1;
    }
    if(maxAp > number / 2)
        printf("Yes %d %d", maxApNumber, maxAp);
    else
        printf("No");
    return 0;
}

Restrictions:

1 ≤ number ≤ 100.000
Each element of the array is smaller than 1.000.000.000

How could my code be improved to be faster or how would you write a faster solution?

This is a very straightforward implementation and fairly easy to read. Nice work! Here are some ways it could be improved:
Performance
Since you asked about performance, let's start there. Looking at your inner loop, it looks like you are going through the entire array for every element of the array. (Actually, I think there's a mistake here. It looks like you intended to write for (j = i + 1... on the inner loop, but instead wrote for j = 1 + 1....)
Rather than doing it that way, what if we had the numbers already in order? Then it would be really easy to do, wouldn't it? You could just make 1 pass through the array, counting up the number of occurrences of each number as you go. Once you find the predominant one, you can skip the rest.
Furthermore, if at any point you have fewer than 1/2 + 1 numbers left and you switch to a new number, you automatically know that there is no predominant number. So you never have to traverse the entire array, unless the entire second half of the array contains the same number. So in the vast majority of cases you won't traverse the entire array.
So I would add a call to qsort() to sort the numbers after reading in the array. Then your remaining code would be significantly simplified, and would probably be significantly faster.
Naming
Your variable names could use some help. You're on the right track, but I would go further. Instead of number which is quite generic (everything's a number to the computer!), why not numEntries or numElements? i and j are fine for loop indexes.
What does Ap stand for? I see currentElAp, maxAp, etc. But I don't understand what it means. As such, I'd expand it to whatever it stands for.
Functions
I would break the program into separate functions with good names that describe what it does. For example, you could have a readEntries() function that reads in the array, a findPredominantNumber() function that finds the numbers you're looking for, and a displayResults() function that prints everything out. It would make the code more readable and easier to understand at a glance.
Stylistic Stuff
I would suggest a few stylistic improvements. In general it's best to put one variable declaration per line. It's easier to read and can avoid some common errors, like thinking that you're declaring a pointer when you're not.
Use more whitespace! I see only a single blank line in your function. It would be nice to see some more between the last for and the final if.
I would also recommend using brackets around single-line statements in an if or for body because this can also reduce the possibility of errors. This one is especially important on code that will be maintained over years.


Too many dependencies in a class I'm trying to work around a rule processor so I created the following classes (the interfaces would be straightforward to guess).

public class OrderProcessor : IOrderProcessor
{
    private readonly IRuleEvaluator _ruleEvaluator;

    public OrderProcessor(IRuleEvaluator ruleEvaluator)
    {
        _ruleEvaluator = ruleEvaluator;
    }

    public void Process(Command command)
    {
        var rules = _ruleEvaluator.GetRules(command);
        foreach (var rule in rules)
        {
            rule.Apply(command);
        }
    }
}

public class RuleEvaluator : IRuleEvaluator
{
    private readonly IRuleSet _ruleSet;

    public RuleEvaluator(IRuleSet ruleSet)
    {
        _ruleSet = ruleSet;
    }

    public List<IRule> GetRules(Command command)
    {
        var ruleSet = _ruleSet.GetRules();

        if (!ruleSet.Any())
        {
            return new List<IRule>();
        }

        var rules = from pair in _ruleSet.GetRules()
                    orderby pair.Value descending
                    select pair.Key;

        return rules.Where(r => r.IsApplicable(command)).ToList();
    }
}

public class RuleSet : IRuleSet
{
    private readonly Dictionary<IRule, int> _rules = new Dictionary<IRule, int>();

    public RuleSet(IDependency1 dependency1,
        IDependency2 dependency2)
    {
        _rules.Add(new Rule1(dependency1), 1000);
        _rules.Add(new Rule2(dependency2), 1);
    }

    public Dictionary<IRule, int> GetRules()
    {
        return _rules;
    }
}

I was trying to abstract it in a way that would be easy to add new Rules to the RuleSet, but my issue comes where I see that if the Rules take any dependency I have to add them to RuleSet class. Not sure if this is actually a good practice as I would say it will easily bloat.

but my issue comes where I see that if the Rules take any dependency
  I have to add them to RuleSet class

You managed to find the issue yourself. Your RuleSet class shouldn't be implemented in such way that the rules are always the same (I believe?). What this mean is that when you create a new RuleSet any rules should be able to go into that set.

I don't know what is the purpose of it but it seems your kind of associating custom data to your rules (in this case integers).

So one approach to consider is to include that data in the rules themselves, per instance:

public interface IRule{
    bool IsApplicable(Command command);
    int Data{get;}
}

Another approach to consider is to provide a method on RuleSet that allows you to add a rule to the set, after this have been created:

public class RuleSet : IRuleSet
{
    private readonly Dictionary<IRule, int> _rules = new Dictionary<IRule, int>();

    public RuleSet()
    {
    }

    public Dictionary<IRule, int> GetRules()
    {
        return _rules;
    }

    public void AddRule(IRule rule, int data){
        _rules.Add(rule, data);
    }
}

I don't know what your implementation of IRule here is but you better make sure that it implements GetHashCode and Equals properly because you are using them as a dictionary key. This is true not only the existing IRule classes but all future possible implementations of IRule that can possibly lie on this dictionary.

Another approach as @Adriano suggested, would be to rely on insertion order, or using a ordered collection such as a SortedSet (presuming the rules will not repeat this will do)


Counting nonexisting records with foreign key I have two tables Room and RoomDate. Here's the structure for Room

Room
roomID
checkInDate
checkOutDate
roomType

Here's the structure for RoomDate. They have a one to many relationship.

RoomDateID
checkInDate
checkOutDate
RoomID // foreign key

Now I want to find how many rooms per roomType are available in a specific date range. That is, if a date range with a specified roomID does not yet exist in the RoomDate table, it can be added. This is how I do it.

Count all records in the Room table per type. RoomTypes are single, double and triple.
Count all records in the RoomDate that already have the inputed date range per type (E.G. if exist in single, cannot be added in single but can still be added in other room types).
Subtract the RoomDate results from Room to get all rooms that are still available per type. So if there are no records in RoomDate, all rooms are available.

Sample input:

number of rooms in the database (RoomDate does not have records yet):
single rooms: 5
doubleRooms: 6
triple rooms: 7

User: 2015-11-11 - 2015-11-21 - 5 single rooms

Sample output:

five single rooms with the date range of 2015-11-11 - 2015-11-21 is added to the RoomDate table.
number of rooms in the database without the range 2015-11-11 - 2015-11-21:
single rooms: 0
doubleRooms: 6
triple rooms: 7

Next time the user enters a date in the range of 2015-11-11 - 2015-11-21 and again chose any number of single rooms, he cannot insert it in the database because there aren't any single rooms left. But he can still insert it with other room types double and triple as long as they are still available.

I used inner join before, but it does not count null records in RoomDate, so I mad individual queries instead.

Please review my code. I think this is too slow (6 HQL select statements), how can I improve this code and make it more efficient?

public long[] countRooms() {
    Session session = getSessionFactory().getCurrentSession();
    long[] count = new long[3];
    String query = "select count (r) from Room r where r.roomType=";
    count[0] = (long)session.createQuery(query+"0").uniqueResult();
    count[1] = (long)session.createQuery(query+"1").uniqueResult();
    count[2] = (long)session.createQuery(query+"2").uniqueResult();
    return count;
}

public long[] countAvailableRooms(LocalDate checkInDate, LocalDate checkOutDate) {
    Session session = getSessionFactory().getCurrentSession();
    String query = "select count(d) from Room r join r.roomDates d where d.checkInDate=:checkInDate and d.checkOutDate=:checkOutDate and r.roomType=";
    long[] count = countRooms();
    count[0] -= (long)session.createQuery(query+"0").setParameter("checkInDate", checkInDate).setParameter("checkOutDate", checkOutDate).uniqueResult();
    count[1] -= (long)session.createQuery(query+"1").setParameter("checkInDate", checkInDate).setParameter("checkOutDate", checkOutDate).uniqueResult();
    count[2] -= (long)session.createQuery(query+"2").setParameter("checkInDate", checkInDate).setParameter("checkOutDate", checkOutDate).uniqueResult();
    return count;
}

Database modeling

Is the presence of check-in/check-out dates on both tables intentional?

If RoomDate is more like a reservations table, then having the check-in and check-out dates in it makes sense. How is the pair of date columns in the Room table used then?

Determining occupancy

Getting occupancy by using an = check is quite simplistic, as that does not take into account later check-ins, or earlier check-outs. If you want the check to be more robust, it can be something along the lines of:

-- please check if BETWEEN includes the values or not
d.checkInDate BETWEEN :checkInDate AND :checkOutDate OR
d.checkOutDate BETWEEN :checkInDate AND :checkOutDate OR
d.checkInDate <= :checkInDate AND d.checkOutDate >= :checkOutDate

This counts existing reservations with check-in or check-out dates that either fall within or span over the requested date range.

Efficient SELECT

  Could you not use a GROUP BY SQL statement (or the HQL equivalent) so that you make one query instead of three? - myself

I'm not very familiar with Hibernate, and we don't even know your choice of DBMS, so the following is borderline pseudo-query:

SELECT r.roomType, r.total - b.booked AS remaining FROM
    ((SELECT roomType, COUNT(1) AS total FROM Room GROUP BY roomType) AS r JOIN
        (SELECT roomType, COUNT(1) AS booked FROM RoomDate d JOIN Room USING roomID
            WHERE d.checkInDate BETWEEN :checkInDate AND :checkOutDate OR
                d.checkOutDate BETWEEN :checkInDate AND :checkOutDate OR
                d.checkInDate <= :checkInDate AND d.checkOutDate >= :checkOutDate
            GROUP BY roomType) AS b USING roomType
    )

The idea here is not to fire 6 separate SQL statements, as you have found to be 'unsatisfactory' and 'inefficient'. What you need is one representation from your Room table that shows the total rooms:

$$\begin{array} {|r|r|}
\hline
roomType & total \\
\hline
0 & 5 \\
\hline
1 & 6 \\
\hline
2 & 7 \\
\hline
\end{array}$$

A representation from your RoomDate table that shows the booked, i.e. unavailable rooms (e.g. after the five-single-rooms booking is done):

$$\begin{array} {|r|r|}
\hline
roomType & booked \\
\hline
0 & 5 \\
\hline
1 & 0 \\
\hline
2 & 0 \\
\hline
\end{array}$$

Joining both on the roomType column:

$$\begin{array} {|r|r|r|}
\hline
roomType & total & booked \\
\hline
0 &5 &5 \\
\hline
1 &6 &0 \\
\hline
2 &7 &0\\
\hline
\end{array}$$

And finally subtract the two columns:

$$\begin{array} {|r|r|}
\hline
roomType & remaining \\
\hline
0 & 0 \\
\hline
1 & 6 \\
\hline
2 & 7 \\
\hline
\end{array}$$

You may want to consider if the modeling can be further improved to remove the inner JOIN. This is just something to get you started with.

edit This might be how you can do it in Hibernate/HQL... Again, I'm not familiar with Hibernate, and the casting may be done incorrectly... please test and experiment carefully.

// Assuming you now have a RoomType `enum`
Map<RoomType, Integer> map = new EnumMap<>(RoomType.class);
for (Object[] row : session.createQuery(query).list()) {
    map.put(RoomType.values()[(int)row[0]], row[1]);
}
System.out.println("Room type and free rooms:\n" + map);

Java code review

As you can see, the main solution to your problem lies in doing a more efficient SELECT once. With that out of the way, you may want to consider using a model class to represent the results, instead of a simply long[] array. It is not evident from the array itself how the indices are being mapped... if you decide to use a non-contiguous values to represent new room types, you may encounter issues mapping those.

You can also consider modeling your room types as a Java enum type, for starters.


Object-oriented design implementation of an Elevator I am a newbie when it comes to Object Oriented Programming and I have read a lot of online articles and lessons so I want to try it out on my own. I've written a console app in C# and tried implementing Object Oriented Design and SOLID Principles.

The console app checks if a person has an access to a certain floor. Also, The app is in its initial stage so right now the Elevator accomodates a single person.

Although the app works. any feedback,comments, and suggestions will be greatly appreciated especially in terms of SOLID principles or OOP Design.

here is the code below

class Program
{
    static void Main(string[] args)
    {
        Person maintenanceEmployee = new MaintenanceEmployee();
        Elevator elev = new Elevator(maintenanceEmployee);
        elev.GoToFloor(Floors.FourthFloor);

        Person guest = new Guest();
        Elevator elev2 = new Elevator(guest);
        elev2.GoToFloor(Floors.SecondFloor);

        Person empfromthirdfloor = new EmployeeFromThirdFloor();
        Elevator elev3 = new Elevator(empfromthirdfloor);
        elev2.GoToFloor(Floors.ThirdFloor);

        Console.ReadLine();
    }
}

public enum Floors
{
    FirstFloor = 1,
    SecondFloor = 2,
    ThirdFloor = 3,
    FourthFloor = 4,
    FifthFloor = 5
}

public class Elevator
{
    Person person;
    public Elevator(Person _person)
    {
        this.person = _person;
    }

    public void GoToFloor(Floors _floor)
    {
        if (person.HasAccess(_floor))
        {
            Console.WriteLine("Elevating to " + _floor.ToString());
        }
        else
        {
            Console.WriteLine("No Access to " + _floor.ToString());
        }
    }
}

public abstract class Person
{
    public Floors[] accessibleFloors;

    public bool HasAccess(Floors _floor)
    {
        foreach (Floors i in accessibleFloors)
        {
            if (_floor == i)
            {
                return true;
            }
        }
        return false;
    }

}

public class MaintenanceEmployee : Person
{
    public Floors[] _accessibleFloors = { Floors.FirstFloor, Floors.SecondFloor, Floors.ThirdFloor, Floors.FourthFloor, Floors.FifthFloor };

    public MaintenanceEmployee()
    {
        //Not sure if this is the best practice
        //Reason why I did it like this is because
        //I need to change the arrays of accessibleFloors
        //in the abstract class "Person" in order to run successfully
        base.accessibleFloors = _accessibleFloors;
    }
}

public class EmployeeFromThirdFloor : Person
{
    public Floors[] _accessibleFloors = { Floors.ThirdFloor };

    public EmployeeFromThirdFloor()
    {
        //Not sure if this is the best practice
        //Reason why I did it like this is because
        //I need to change the arrays of accessibleFloors
        //of the abstract class "Person" in order to run successfully
        base.accessibleFloors = _accessibleFloors;
    }
}

public class Guest : Person
{
    public Floors[] _accessibleFloors = { Floors.SecondFloor };

    public Guest()
    {
        //Not sure if this is the best practice
        //Reason why I did it like this is because
        //I need to change the arrays of accessibleFloors
        //of the abstract class "Person" in order to run successfully
        base.accessibleFloors = _accessibleFloors;
    }
}

Person + Derived Classes

accessibleFloors is a breach of Open–closed principle. Anyone is able to modiy this collection.
accessibleFloors could be considered a breach of Single responsibility principle. Consider having a Role class and Person storing his Roles set. This way, you can also mitigate redundant code and remove the need of derived classes as EmployeeFromThirdFloor, Guest and MaintenanceEmployee (in this trivial example these classes are not required).
accessibleFloors can be null. The code is not able to handle this situation.
HasAccess is way to convoluted: accessibleFloors.Contains(_floor); would do it.

public abstract class Person
{
    public Floors[] accessibleFloors;

    public bool HasAccess(Floors _floor)
    {
        foreach (Floors i in accessibleFloors)
        {
            if (_floor == i)
            {
                return true;
            }
        }
        return false;
    }
}

We can create a class Role.
public class Role
{
    public HashSet<Floors> AccessibleFloors { get; }

    public Role (HashSet<Floors> accessibleFloors)
    {
        AccessibleFloors = accessibleFloors
            ?? throw new ArgumentNullException(nameof(accessibleFloors));
    }

    public bool HasAccess(Floor floor)
    {
        return AccessibleFloors.Contains(floor);
    }

    // override equals and gethashcode ..
}

We can refactor Person.
public class Person
{
    public HashSet<Role> Roles { get; }

    public Person(HashSet roles) {
        Roles = roles ?? throw new ArgumentNullException(nameof(roles));
    }

    public bool HasAccess(Floors floor)
    {
        return Roles.Any(r => r.HasAccess(floor));
    }
}

Because of Inheritance, the derived classes do not require to create their own instance variable _accessibleFloors.

base.accessibleFloors = _accessibleFloors;

Elevator

There is tight-coupling between an Elevator and a Person. You force the elevator to exist only with a person. What about an empty elevator.
GoToFloor is not usable code. Consider returning a Boolean and putting the Console.WriteLine in the calling code.
The code cannot handle when Person is null.

public class Elevator
{
    Person person;
    public Elevator(Person _person)
    {
        this.person = _person;
    }

    public void GoToFloor(Floors _floor)
    {
        if (person.HasAccess(_floor))
        {
            Console.WriteLine("Elevating to " + _floor.ToString());
        }
        else
        {
            Console.WriteLine("No Access to " + _floor.ToString());
        }
    }
}

Elevator could be refactored as follows.
public class Elevator
{
    public Person Person { get; private set; }

    public bool IsFull => Person != null;
    public bool IsEmpty => Person == null;

    public bool Enter(Person person)
    {
        if (person == null) throw new ArgumentNullException(nameof(person));
        if (IsFull) return false;
        Person = person;
        return true;
    }

    public bool Exit(Floors floor)
    {
        if (IsEmpty) return false;
        if (!Person.HasAccess(floor)) return false;
        Person = null;
        return true;
    }
}

Program
class Program
{
    static void Main(string[] args)
    {
        var guestRoles = new HashSet<Role> {
            new Role(new HashSet<Floors> { Floors.FirstFloor })
        };
        var guest = new Person(guestRoles);
        var elevator = new Elevator();

        if (elevator.Enter(guest))
        {
            Console.WriteLine("guest has entered elevator");
        }
        else
        {
            Console.WriteLine("guest cannot enter elevator: elevator is full");
        }

        if (elevator.Exit(Floors.FirstFloor))
        {
            Console.WriteLine("guest has exited elevator at FirstFloor");
        }
        else
        {
            Console.WriteLine("guest cannot exit elevator at FirstFloor: access is denied");
        }

        Console.ReadLine();
    }
}


Modified TextBox for forms generating SQL statements An overview of what I've done:

I've inherited System.Windows.Forms.TextBox and added a few properties to help me out in creating forms that generate SQL statements.

I use to create a large function that would check for changes in the TextBox compared to a string. Then it would take text and concatenate it to a SQL statement. Now it's simple and easier to use.

I've added four properties:

A String property to hold the default string that the textbox is initially set to and will reset back if the text is left empty.
A String property to hold the text's associated SQL statement.
A Boolean to check if the text has changed from the default.
Finally, an Integer to hold an ID if there is a reason to need one, such as saving and loading text from a file by the ID.

I have not fully implemented the new textbox in my code, but I am working on it.

It may only be slightly modified, but it should cut down my code from 500+ lines of code down to less than +-30 if ran in a for-loop.

SQLTextBox.vb

Public Class ModifiedTextBox
    Inherits System.Windows.Forms.TextBox

    Private _strDefaultText As String
    Private _strSqlText As String
    Private _nID As Integer
    Private _bModified As Boolean = False

    Property ID()
        Set(nID)
            _nID = nID
        End Set
        Get
            Return _nID
        End Get
    End Property

    Property TextModified()
        Set(bModified)
            _bModified = bModified
        End Set
        Get
            Return _bModified
        End Get
    End Property

    Property SqlText()
        Set(strSqlText)
            _strSqlText = strSqlText
        End Set
        Get
            Return _strSqlText
        End Get
    End Property

    Property DefaultText()
        Set(strDefaultText)
            _strDefaultText = strDefaultText
            Me.Text = _strDefaultText
        End Set
        Get
            Return _strDefaultText
        End Get
    End Property

End Class

I've pulled an excerpt from my code. It's not much, but here is the new textbox vs the old one.

In the SQL text, I have something like ""Material Type"" in('INSERTTEXT').
SQLTextBox.Modified gets set when the user leaves the textbox (set to false b default).

'This should run every new textbox (untested)
For i As Integer = 0 To Me._icControls.txtMain.Length - 1
    If Me._icControls.txtMain(i).Modified = True Then
        sqlWhere += " AND " + Me._icControls.txtMain(i).SqlText
        Replace(sqlWhere, "INSERTTEXT", Me._icControls.txtMain(i).Text)
    End If
Next

'Old, long way around where I had split up
'the textboxes into 7 groups they belonged too.
If Me._icControls.txtMaterial(0).Text.ToString() <> DefaultStrings.Material(0) Then
    sqlWhere += " AND ""Material Type"" in('" + Me._icControls.txtMaterial(0).Text.ToString() + "')"
End If

If Me._icControls.txtMaterial(4).Text.ToString() <> DefaultStrings.Material(4) Then
    sqlWhere += " AND ""Grade"" in('" + Me._icControls.txtMaterial(4).Text.ToString() + "')"
End If

If Me._icControls.txtMaterial(5).Text.ToString() <> DefaultStrings.Material(5) Then
    sqlWhere += " AND ""PIW"" in('" + Me._icControls.txtMaterial(5).Text.ToString() + "')"
End If

I went from 200 lines of code to 6, with a minor change. That does not include the lines spent setting up the default text.

Let me know what you think of this. I'm not sure if it's really anything someone could use, but it's been very useful to me.

There's not much to review here really. I'd be more interested in reviewing the code that's actually doing the work, but here goes.

Lose the Systems Hungarian notation. The IDE/Code tells me what the data type is. That is if you...
Declare the data types of the properties
Use auto properties. There's way too much code here for how simple this is.

Public Class ModifiedTextBox
    Inherits System.Windows.Forms.TextBox

    Property ID() As Integer
    Property TextModified() As Boolean
    Property SqlText() As String

    Property DefaultText() As String
        Set(strDefaultText)
            _strDefaultText = strDefaultText
            Me.Text = _strDefaultText
        End Set
        Get
            Return _strDefaultText
        End Get
    End Property

End Class


Update SQL database with using statement and SqlParameters I'm just starting out to learn all about sql databases. For now based on previous suggestions that was given to me online, I've incorporated a using statement to avoid problems in sql connection and then I applied parameterized queries to avoid sql injection. I was also been given an advice to use sqlparameter arrays to supplement the parameterized queries that i implemented. I just want to know if there are other techniques that you would recommend to improve my code, which i will indicate below. Feel free to suggest better solutions.

using (SqlConnection connection = new SqlConnection(ConfigurationManager.ConnectionStrings["brandsConnection"].ToString())) {
    string query = "UPDATE [guitarBrands] SET type = @type, name = @name, image = @image WHERE id = @id";
    SqlParameter[] p = new SqlParameter[4];
    p[0] = new SqlParameter("@type", newType.Text);
    p[1] = new SqlParameter("@name", newName.Text);
    p[2] = new SqlParameter("@image", newImage.Text);
    p[3] = new SqlParameter("@id", id);

    connection.Open();
    using (SqlCommand command = new SqlCommand(query, connection))
    {
        GetExample(command, p);
        command.ExecuteNonQuery();
        connection.Close();
        command.Parameters.Clear();
    }
}

public void GetExample(SqlCommand command, params SqlParameter[] p)
{
  if (p != null && p.Any()){
        command.Parameters.AddRange(p);
   }

}

Stacking the using statements

I am particularly difficult to please with code that has nesting, and I highly prefer to stack the usings when it is possible.

using (SqlConnection connection = ...)
{
    using (SqlCommand command = ...)
    {
        ...
    }
}

versus

using (SqlConnection connection = ...)
using (SqlCommand command = ...)
{
   ...
}

Exception Handling

You should include exception handling into your code, as it will likely throw an exception sooner or later (such as getting disconnected, network issues, etc.)

using (SqlConnection connection = ...)
using (SqlCommand command = ...)
{
    try
    {
        // open the connection, execute, etc
    }
    catch
    {
        // log and handle exception(s)
    }
}

Using Lists not Arrays

It would likely do you more harm than good to be using an array, especially in the context of maintainable code. I would recommend you switch to using Lists, like this:

List<SqlParameter> p = new List<SqlParameter>();
p.Add(new SqlParameter("@type", newType.Text));
p.Add(new SqlParameter("@name", newName.Text));
p.Add(new SqlParameter("@image", newImage.Text));
p.Add(new SqlParameter("@id", id));

This will let you introduce more items into your parameters without having to do deal with the headaches of explicitly indexing.

Note: If you want to keep the same method signature for your GetExample() method, then don't forget to pass the parameters as an array --- you could use ToArray() to accomplish that.

Here it all is together...

string query = "UPDATE [guitarBrands] SET type = @type, name = @name, image = @image WHERE id = @id";

using (SqlConnection connection = new SqlConnection(ConfigurationManager.ConnectionStrings["brandsConnection"].ToString()))
using (SqlCommand command = new SqlCommand(query, connection)
{
    try
    {
        // open the connection, execute, etc
        List<SqlParameter> p = new List<SqlParameter>();
        p.Add(new SqlParameter("@type", newType.Text));
        p.Add(new SqlParameter("@name", newName.Text));
        p.Add(new SqlParameter("@image", newImage.Text));
        p.Add(new SqlParameter("@id", id));

        connection.Open();
        GetExample(command, p.ToArray());
        command.ExecuteNonQuery();
        command.Parameters.Clear();
    }
    catch
    {
        // log and handle exception(s)
    }
}


Collection with Generic Index I've just done something that works, but I have the feeling it's not correct. I made a class derived from list. I use a generic type to search the list using one of the properties of the list type as the index. Does this look right to you?

public class IndexableCollection<TCollection, TIndex> : List<TCollection>
{
    public IndexableCollection() : base() {}

    public TCollection this[TIndex index]
    {
        get
        {
            return base.Find(t => t.GetType().GetProperty(index.GetType().Name).GetValue(t).Equals(index));
        }
    }

    public List<TCollection> GetList()
    {
        return this as List<TCollection>;
    }

}

The intention is to have a collection where I can do this:

public class MyClass
{
    public int MyIntProperty { get; set; }
    public string MyStringProperty { get; set; }
    public MyEnumProperty MyEnumProperty { get; set; }
}

public void Main()
{
    var myCollection = new IndexableCollection<MyClass, MyEnumProperty>();

    var element = myCollection[MyEnumProperty.Option1];
}

The problem

There is a massive hole in your logic here. You are relying on the fact that there exists a property whose type is equal to the name of the property.

public MyEnumProperty MyEnumProperty { get; set; }

Your class would become unusable if I changed this line to:

public MyEnumProperty MyProperty { get; set; }

To explain the issue in detail, the rest of my answer will assume that you're using this second case where the type and name do not match.

The problem is here:

t => t.GetType()
      .GetProperty(index.GetType().Name)  // <== HERE
      .GetValue(t)
      .Equals(index)

GetProperty retrieves a property based on its name (MyProperty). So, what name are you using?

index.GetType().Name

The name of the type (MyEnumProperty). Which is not the same as the MyProperty name that you're looking for.

Let's look at another implementation of your class, where we will use MyIntProperty as the index:

var myCollection = new IndexableCollection<MyClass, int>();

var element = myCollection[5];

Going by the intention of your code, this should be possible. After all, you have made no specific requirements for the given TIndex, which means that your class needs to work for all possible types.

But it doesn't, because when you try to access myCollection[5], you get:

t => t.GetType()
      .GetProperty("Int32")
      .GetValue(t)
      .Equals(index)

There is no property by the name of Int32, so it does not work.

The solution

In essence, you are trying to define the indexed property by its type alone, which is a dangerous thing to do.

We could rewrite the code to search MyClass for the first property of the given type TIndex. However, what will you do if MyClass has multiple properties of that same type?

You'd be stuck guessing at the intended index, and that's not a good solution either.

What we're missing is the definition of the exact property you want to use as an index. You're trying to find it by its type alone, and that is not enough. Instead, you need a way to select the correct property.

My solution: the lambda.

You need a mapping between the element (TElement) and its intended index (TIndex). This is the perfect situation to use a lambda method of type Func<TElement, TIndex>.

I updated your code to a working example with lambdas. I slightly tweaked your code for the sake of example:

public enum MyEnum { One, Two, Three }

public class MyClass
{
    public int MyIntProperty { get; set; }
    public string MyStringProperty { get; set; }
    public MyEnum MyEnumProperty { get; set; }
}

The collection class:

public class MyCollection<TElement, TIndex> : List<TElement>
{
    private Func<TElement, TIndex> _indexMapping { get; set; }

    public MyCollection(Func<TElement, TIndex> indexMapping)
    {
        _indexMapping = indexMapping;
    }

    public TElement this[TIndex index]
    {
        get
        {
            return base.Find(t => _indexMapping.Invoke(t).Equals(index));
        }
    }
}

The only real difference is that we now store a Func<TElement, TIndex> _indexMapping. This gives us the mapping between the element and its index.
This has the interesting side effect that you can't use a TIndex that doesn't exist as a property in TElement (unless you supply a constant value unrelated to the element, but then you're being silly).

base.Find(t => _indexMapping.Invoke(t).Equals(index));

When iterating over the list, we simply invoke the mapping (on the element t), which returns the value of its intended index. All we have to do then is check whether the value matches the index that was currently given as a parameter, which is easily done by calling .Equals().

Example usages:

//Enum as index

var myCollection = new MyCollection<MyClass, MyEnum>(o => o.MyEnumProperty);
var element = myCollection[MyEnum.One];

//Int as index

var myCollection = new MyCollection<MyClass, int>(o => o.MyIntProperty);
var element = myCollection[12345];

//String as index

var myCollection = new MyCollection<MyClass, String>(o => o.MyStringProperty);
var element = myCollection["Hello"];

Tangential comments

If your index is a custom class, make sure to implement the correct equality comparison! The default behavior (checking if it's the same object in memory) may or may not be what you want.
There's still an issue when your collection contains multiple items with the same index. You'll always receive the first matching item, the others will essentially be invisible (until you remove the first item). Again, depending on your requirements, that might be intentional behavior, or it might not be.

If you want to ensure that you only have unique items in the list, and no duplicate indexes exist, you will need additional logic to validate new entries to the list.

As an aside, I would use TElement instead of TCollection, as the generic type refers to the elements of the collection (MyClass), not the collection itself (which is a List). Generic type names, e.g. TFoo, should be read as "type of the Foo". This applies to your TIndex (type of the index), and TElement would be similarly correct.


Speeding up script for processing three huge files I have three huge files I need to process in order to rearrange the data contained within.

The first file is a list of English example sentences (468,785 lines long).
A typical line from this file looks like this:

120033#eng#Creativity is an important aspect for the development of human.

The second file is a list which tells me the ID of the equivalent sentences (7,243,419 lines) in the third file. A typical line from the second file looks like this:

1#77

This tells me that the English sentence with ID "1" in the first file matches a translation with ID "77" in the third file.

The third file contains a list of the translated sentences (2,945,676 lines). A typical line from this file looks like this:

1#cmn#我們試試看！

Here is the script I am using to get each line from file one, find which sentences it links to in file 2 and then get the matching sentence from file 3:

with open("eng_lines.txt") as f:
    eng_lines = f.readlines()
with open("all_lines.txt") as f:
    all_lines = f.readlines()
with open("all_links.txt") as f:
    all_links = f.readlines()
for ln,line in enumerate(eng_lines):
    print ln,len(eng_lines)
    with open("matches.txt", "a") as matches:
        matches.write(line+"\n")
        hash = line.index("#")
        sentence_idA = line[:hash]
        for line2 in all_links:
            hash = line2.index("#")
            link_a = line2[:hash]
            link_b = line2[hash+1:].strip()
            if (link_a==sentence_idA):
                for line3 in all_lines:
                    hash = line3.index("#")
                    sentence_idB = line3[:hash]
                    if (sentence_idB==link_b):
                        matches.write(line3+"\n")

This process is going to take a LONG time (about a year given that each iteration is currently taking about a minute to process on my i7 PC).

A major problem with your code is that you're doing very long linear searches to find matches-- worse, you're doing those in a nested fashion.

Instead, you should take advantage of Python dictionaries, which have very fast lookups.  Something like this:

def read_sentences(filename):
    with open(filename) as fp:
        data = {line[:line.index("#")]: line for line in fp}
    return data

eng_lines = read_sentences("eng_lines.txt")
all_lines = read_sentences("all_lines.txt")

with open("all_links.txt") as fp:
    all_links = dict(line.rstrip().split("#") for line in fp)

with open("matches2.txt", "w") as fp_matches:
    for idcode in sorted(eng_lines, key=int):
        print "working on", idcode
        if idcode in all_links:
            fp_matches.write(eng_lines[idcode] + "\n")
            fp_matches.write(all_lines[all_links[idcode]] + "\n")
        else:
            print "warning: could not match eng_line", idcode

builds dictionaries for both eng_lines and all_lines, using the id as the key and the line as the value.  It also builds a dictionary all_links to store the connection between the id values in eng_lines and all_lines.   On some test data I mocked up, it produces a file looking like yours:

1#eng#This is example sentence number 1

995072#cmn#This should really be in Mandarin 995072

2#eng#This is example sentence number 2

1732074#cmn#This should really be in Mandarin 1732074

in about ten to twenty seconds on a dataset of your size.