Censoring emails in Java I'm working on a simple method that would censor emails (GPDR stuff) so they won't show the full name of a user, with 1st character, optional dot, 1st character after the dot and whatever is left after the @ to be left uncensored. So, the intent is like this:
Email to be censored: name.lastname@email.com
Desired effect: n***.l*******@email.com
I've written this:
public static String getCensoredUsername(String username) {
    if (username.contains("@")) {
        char[] chars = username.toCharArray();
        for (int i = 1; i < chars.length; i++) {
            boolean isCurrentCharacterADot = chars[i] == '.';
            boolean isPreviousCharacterADot = chars[i - 1] == '.';

            if (chars[i] == '@') {
                break;
            }
            if (chars[i] == '.') {
                chars[i] = '.';
            } else if (!isCurrentCharacterADot && !isPreviousCharacterADot) {
                chars[i] = '*';
            }
        }
        return String.valueOf(chars);
    }
    return username;
}

It feels to me very Java 6. What would make it look better?

There is nothing wrong about being "too Java 6"!
First few notes about naming and naming convention:

username is, in fact, the email, so should be email
getCensoredUsername for the same reason should be getCensoredEmail but in Java get, by convention, is used only for getter or when you're applying very little logic; thus is better to use an "action-subject" type of name like censorEmail
Being Java a type language, calling a variable chars is not helpful, that should be emailChars or even just email if that is not colliding with other variables, the compiler will tell you the type.

Biggest problem though, is the data validation, you should check at least that email is not null.
Normally you would check that the email is "valid" but if you take a look at the email syntax you might notice that's almost impossible to do it without writing a formal parser (or using some library). So let's say we want just censor the "reasonably valid" emails.
Your approach to go char by char in the username part is not very flexible: what if tomorrow you want to have f****o.f*****i@gmail.com? You need to add other flags, and wouldn't be very enjoyable.
I suggest you to start from the censor method and build bottom up from that.
Here is my solution
  public static String hideLastChars(String name) {
    Objects.requireNonNull(name, "String should not be null");
    if (name.length() <= 1) {
      return name;
    } else {
      return name.charAt(0) + IntStream.range(1, name.length()).mapToObj(i -> "*").collect(Collectors.joining());
    }
  }

Now that we have our "logic" we need to do a bit of input validation.
  public static String censorEmail(String email) {
    Objects.requireNonNull(email, "Email should not be null");
    String[] splitEmail = email.split("@");
    if (splitEmail.length > 1 && splitEmail[0].length() > 0) {
      return censorUsername(splitEmail[0]) + "@" + splitEmail[1];
    }
    throw new IllegalArgumentException("Please provide a valid email");
  }

Now we know that the email is not null, it does contain a '@' and user is there.
We just need to apply our hideLastChars to each piece.
We have a little problem here because if you just split on each . you will end up losing trailing dots, for example in an email like john.doe....@gmail.com.
A solution is to use Lookahead regexp to split on each . but keeping it in the split result, this is the regexp: ((?<=\.)|(?=\.)), for example "john.doe...".split("((?<=\.)|(?=\.))") will return { "john", ".", "doe", ".", ".", "." }
Now we know how to map each piece and how to extract it.
  private static String censorUsername(String username) {
    return Arrays
        .stream(username.split("((?<=\\.)|(?=\\.))"))
        .map(scratch_1::hideLastChars)
        .collect(Collectors.joining(""));
  }

I suggest you to create a unit test to test the logic so every time you find a new email that creates problem you can add in the test and see what happens (What about p.g@curry.com?).
I've run few examples using your code and mine. Here's the result

INPUT
ORIGINAL
REVIEWED

name.surname@gmail.com
n***.s******@gmail.com
n***.s******@gmail.com

name.surname.p99@gmail.com
n***.s******.p**@gmail.com
n***.s******.p**@gmail.com

name.surname.p99.@gmail.com
n***.s******.p**.@gmail.com
n***.s******.p**.@gmail.com

@gmail.com
@*****.c**
Exception: Please provide a valid email

user.com
user.com
Exception: Please provide a valid email

user%example.com@example.org
u***********.c**@example.org
u***********.c**@example.org

"john..doe..."@example.org
"****..d**..."@example.org
"****..d**..."@example.org

" "@example.org
"**@example.org
"**@example.org

null
Exception: Cannot invoke "String.contains(java.lang.CharSequence)" because "username" is null
Exception: Email should not be null

...p...g...@gmail.com
...p...g...@gmail.com
...p...g...@gmail.com

p.g@curryfication.com
p.g@curryfication.com
p.g@curryfication.com


Any way to speed up this UPDATE? SQL:

CREATE FUNCTION dbo.fnRandomForeNames ()
RETURNS VARCHAR(50)
AS
BEGIN
RETURN  (
            SELECT TOP 1 [FirstName]
            FROM [tmp_ForeNames]
            ORDER BY (SELECT new_id from GetNewID)
        )
END
GO

Similar functions for dbo.fnRandomSurNames() etc.

UPDATE Table1
SET firstname = dbo.fnRandomForeNames(),
    lastname = dbo.fnRandomSurNames(),
    address1 = dbo.fnRandomAddress1(),
    address2 = dbo.fnRandomAddress2(),
    address3 = dbo.fnRandomAddress3(),
    birthdate = DATEADD(DAY, ABS(CHECKSUM(NEWID()) % 3650), '1990-01-01')

My C# Code:

    private void RunThis(string connString, StreamReader sr)
    {
        sr.BaseStream.Position = 0;
        string sqlQuery = sr.ReadToEnd();
        using (SqlConnection connection = new SqlConnection(connString))
        {
            Server server = new Server(new ServerConnection(connection));
            server.ConnectionContext.StatementTimeout = 4200;
            server.ConnectionContext.ExecuteNonQuery(sqlQuery);
        }
        sr.Close();
    }

........

 RunThis(e.Argument.ToString(), _updateClaim);

Where e.Argument.ToString() is the connection string.

The CREATE FUNCTION scripts are run earlier, take very little time to run.
Also, names are stored in tmp databases, these are entered in C# via arrays.
These also take very little time to run.

Table1 contains approx 140,000 rows and takes approx. 14 mins to complete.

I have also tried using parameterised SQL queries, skipping the tmp tables and SQL functions and instead creating the SQL query and executing it from the code, such as the following:

UPDATE Table1
SET lastname = '{0}',
    firstname = '{1}',
    birthdate = DATEADD(DAY, ABS(CHECKSUM(NEWID()) % 3650), '1990-01-01'),
    address1 = '{2}',
    address2 = '{3}',
    address3 = '{4}'
    WHERE u_id = '{6}'

And some C#:

 using (SqlConnection connection = new SqlConnection(connString))
        {
            connection.Open();
            for (int i = 0; i < arraySize; ++i)
            {
                string updateString = string.Format(updateString2, GetRandomSurname(), GetRandomForeName(), GetRandomAddress1(), GetRandomAddress2(), GetRandomAddress3(), "", ids[i]);
                SqlCommand cmd = new SqlCommand(updateString, connection);
                cmd.CommandType = CommandType.Text;
                cmd.ExecuteNonQuery();
            }
        }

The latter method also taking upwards of 14 minutes.

Any ideas on how to cut down the time it takes to update the table?

Not sure what that ORDER BY (SELECT new_id from GetNewID), but comparing the following approaches, second is much faster and spends most of the time in COUNT(*), which could be pre-calculated.

SELECT TOP 1 name FROM master.sys.all_objects ORDER BY NEWID()

DECLARE @n int
SELECT @n = RAND() * (SELECT COUNT(*) FROM master.sys.all_objects)

SELECT name FROM (
    SELECT ROW_NUMBER() OVER (ORDER BY (SELECT 1)) as n, name
    FROM master.sys.all_objects
) AS names
WHERE n = @n

I guess you could make it even faster by materializing integer sequential id inside your names tables and making a clustered index on that.


Parse a substring out of string, allocate memory accordingly I have written the following code in order to:

Find begin and end of FTP user and password from a given URL (ftp://[user[:password]@]host[:port]/url-path)
Allocate memory to hold this string
Copy the sub-string into the allocated memory

Here is the code for your review:

static char * swd_parseUsrPass(void)
{
    char *usr_pass = NULL;
    char *url_str = NULL;
    char *beginIndx = NULL;
    char *endIndx = NULL;
    int str_len = 0;

    url_str = getURLaddr();

    /* Find begin and end indexs */
    beginIndx = url_str + strlen(FTP_PREFIX);
    endIndx = strchr(beginIndx, '@');

    if ((endIndx == NULL) || endIndx <= beginIndx) {
        printf("Could not locate user and password in URL\n");
        return NULL;
    }

    str_len = endIndx - beginIndx;

    /* Allocate maximum possible string */
    usr_pass = malloc((sizeof(char) * str_len) + 1);
    if (usr_pass == NULL) {
        printf("Failed to allocated memory!\n");
        return NULL;
    }

    /* Copy user and password to new string */
    strncpy(usr_pass, beginIndx, str_len);
    *(endIndx + 1) = '\0';

    return usr_pass;
}

A 0-ary function (swd_parseUsrPass) calling another 0-ary function (swd_getURLaddr) hints that there are global variables lurking around. Using globals is not recommended; if they are unavoidable, at least limit their visibility. Consider char swd_parseUsrPass(char * url_addr), and let the caller provide the argument.
sizeof(char) is guaranteed to be 1.
*(endIndx + 1) = '\0' modifies the source string, while usr_pass remains not terminated. I presume you meant

usr_pass[str_len] = '\0';


Feynmann lifeguard riddle graphical representation Physicist Richard Feynman though of a very interesting riddle:

Imagine you are a lifeguard at the side of the beach and there is a child drowning in the water. (You must cross both sand and water to get to him)
Which route would you take to go to the child and save him?

Not a straight line1 because in the sand you move faster than in the water so the advantage of staying a larger percentage of the trip on the sand outweighs the disadvantage of having a longer trip (straight line is shortest) up to a certain point.
How to get an intuition about the shape of such route?
I wrote a simple visualization with modifiable parameters to get an intuitive feeling for different start positions and sand/sea speed ratios, here is an example (yellow is sand, blue is sea):

I am interested in all kinds of suggestions, here is the code:
from tkinter import *

LIFEGUARD_Y = 100
CHILD_Y = 350
SAND_SPEED = 3 # SEA_SPEED = 1
WIDTH = 500
HEIGHT = 500

def distance(a, b):
    return ((a[0] - b[0])**2 + (a[1]-b[1])**2)**0.5

def total_time(lifeguard_y, child_y, midpoint_y,sand_speed=SAND_SPEED):
    sand_len = distance( (lifeguard_y, 0), (midpoint_y, WIDTH//2) )
    sea_len = distance( (midpoint_y, WIDTH//2), (child_y, WIDTH) )
    sand_time = sand_len / sand_speed
    sea_time  = sea_len / 1
    return sand_time + sea_time

def find_optimal_midpoint_y(lifeguard_y, child_y,sand_speed=SAND_SPEED):
    return min(range(0, 500), key=lambda y: total_time(lifeguard_y, child_y, y, sand_speed))

def show_situation():
    master = Tk()

    master.title("Feymann lifeguard route problem")
    w = Canvas(master, width=WIDTH, height=HEIGHT)
    w.pack()

    l = Label(master, text="Lifeguard Y")
    l.pack()

    lifeguard_y_w = Scale(master, from_=0, to=HEIGHT, orient=HORIZONTAL)
    lifeguard_y_w.pack()

    l = Label(master, text="Child Y")
    l.pack()

    child_y_w = Scale(master, from_=0, to=HEIGHT, orient=HORIZONTAL)
    child_y_w.pack()

    l = Label(master, text="Sand Speed / Sea Speed ratio")
    l.pack()

    sand_speed_w = Scale(master, from_=1, to=3, resolution=0.1, orient=HORIZONTAL)
    sand_speed_w.pack()

    w.create_rectangle(0, 0, WIDTH//2, HEIGHT, fill="yellow")
    w.create_rectangle(WIDTH//2, HEIGHT, WIDTH, 0, fill="blue")

    def draw_optimal_route():
        w.create_rectangle(0, 0, WIDTH//2, HEIGHT, fill="yellow")
        w.create_rectangle(WIDTH//2, HEIGHT, WIDTH, 0, fill="blue")

        midpoint_y = find_optimal_midpoint_y(lifeguard_y_w.get(), child_y_w.get(), sand_speed=sand_speed_w.get())

        w.create_line(0, lifeguard_y_w.get(), WIDTH//2, midpoint_y)
        w.create_line(WIDTH//2, midpoint_y, WIDTH, child_y_w.get())

    b = Button(master, text="GO", command=draw_optimal_route)
    b.pack()

    mainloop()

if __name__ == "__main__":
    show_situation()

1 A straight line only in the trivial and non-realistical case where sand-speed = sea-speed such as for a bird.
Bonus Physics Note: Light behaves the same way as this perfect lifeguard because it always chooses the fastest path.

Don't use wildcard imports

Change from tkinter import * to import tkinter as tk, and then prefix all tk classes and variables with tk. (eg: tk.Frame(...)).

One reason is that PEP8 discourages wildcard imports. Also, by using the prefix your code becomes more self-documenting.

One problem I see people make is that they import * from both tkinter and ttk. Since both of them have many classes with the same name (Button, Entry, etc), it becomes impossible to know which is being used without remembering which the order of the imports. If you change the order of the imports you could break many lines of your code.

You don't use ttk here, but if you get in the habit of using wildcard imports, eventually it will cause you problems.

Name your main function main

This is largely personal preference, but I think it helps to name your main function main, and place it at the top of the file. When I was scanning your code, it wasn't until I got down to literally the last line in the file that I knew which function was called first.

Use an object-oriented architecture

Python objects are easy to use, and make writing GUI code much easier. I recommend writing all tkinter programs with classes and objects. There's a stackoverflow question that covers this: Best way to structure a tkinter application

Separate widget creation from widget layout

I personally find it easier to write, maintain, and understand code when the widget creation is separate from widget layout. That is, instead of doing "create widget; layout widget; create widget; layout widget", do "create widget; create widget; layout widget; layout widget". I usually do the grouping by parent (eg: create all children of a parent widget, then layout those children, then go to the next, and so on)

When all of your widgets are in the root it's not quite as important as when you have multiple areas in your GUI that are independent, but even with everything sharing the same parent, grouping them together makes that more apparent.

For example:

l = Label(master, text="Lifeguard Y")
lifeguard_y_w = Scale(master, from_=0, to=HEIGHT, orient=HORIZONTAL)
l = Label(master, text="Child Y")
child_y_w = Scale(master, from_=0, to=HEIGHT, orient=HORIZONTAL)
l = Label(master, text="Sand Speed / Sea Speed ratio")
sand_speed_w = Scale(master, from_=1, to=3, resolution=0.1, orient=HORIZONTAL)

l.pack()
lifeguard_y_w.pack()
l.pack()
child_y_w.pack()
l.pack()
sand_speed_w.pack()

Use explicit options to pack

You are relying on default values for all of your pack statements. While those defaults likely won't ever change, it makes your code a bit easier to read and your intent more obvious if you're explicit about where you expect the widgets to go.

l.pack(side="top")
lifeguard_y_w.pack(side="top")
l.pack(side="top")
child_y_w.pack(side="top")
l.pack(side="top")
sand_speed_w.pack(side="top")

Consider the effect of resizing the window on your design

Your program doesn't handle resizing very well. It's not terrible, but it could be better. For example, as a user I would expect the yellow/blue area to expand to fill the top part of the window when I make the window larger, but it doesn't. I also don't expect to have the buttons chopped off if I make the window smaller; instead, I expect the yellow/blue area to shrink. It's also fairly common for an "OK" button to stick to the bottom, which it doesn't.

You can use options to the geometry managers (pack, place, and grid) to control this. There's no single solution to making it work, it all depends on what you want to have happen when the window is resized. However, the first step usually involves designating a "hero" widget that will grow and shrink with the window. Most GUIs have such a thing, typically it's a text or canvas widget.


Finding the indices of the two items whose price adds up to to a value I've solved the Google Jam store credit problem. but there's some inefficiencies that I want to get rid of them to improve the algorithm and reduce the multiple iteration. What are other better ways to do so?

Problem:

  You receive a credit C at a local store and would like to buy two items. You first walk through the store and create a list L of all available items. From this list you would like to buy two items that add up to the entire value of the credit. The solution you provide will consist of the two integers indicating the positions of the items in your list (smaller number first).

  Input:

  The first line of input gives the number of cases, N. N test cases follow. For each test case there will be:

  One line containing the value C, the amount of credit you have at the
  store.
  One line containing the value I, the number of items in the
  store.
  One line containing a space separated list of I integers. Each
  integer P indicates the price of an item in the store.
  Each test case
  will have exactly one solution.

  Output:

  For each test case, output one line containing "Case #x: " followed by
  the indices of the two items whose price adds up to the store credit.
  The lower index should be output first.

  Limits

  5 ≤ C ≤ 1000 1 ≤ P ≤ 1000

  Small dataset

  N = 10 3 ≤ I ≤ 100

  Large dataset

  N = 50 3 ≤ I ≤ 2000

Solution:

#!/usr/bin/env python3.4
import sys
from itertools import combinations, count

cases = []
credits = []
items = []

with open(sys.argv[1]) as file_:
    next(file_)
    cases = (list(zip(*[file_]*3)))

for case in cases:
    credits.append(int(case[0]))
    items.append([int(item) for item in case[2].split()])

for credit, prices, i in zip(credits, items, count()):
    for tow_items in combinations(prices, 2):
        if sum(tow_items) == credit:
            print("Case #{}:".format(i+1), end=' ')
            print(' '.join([str(i+1) for i, price in enumerate(prices)
                            if price in tow_items]))

Code organisation

Instead of building cases, then use it to build credits and items then use them to fix the problem, you could do things in a more straightforward way with less temporary containers.

with open(sys.argv[1]) as file_:
    next(file_)
    for i, case in enumerate(zip(*[file_]*3)):
        credit = int(case[0])
        prices = [int(item) for item in case[2].split()]
        for tow_items in combinations(prices, 2):
            if sum(tow_items) == credit:
                print("Case #{}:".format(i+1), end=' ')
                print(' '.join([str(i+1) for i, price in enumerate(prices) if price in tow_items]))

Also, it would probably make sense to split your code into multiple functions : you could write a function handling input/ouput and a function to handle the algorithmic part of the problem.

def get_indices_with_sum(prices, credit):
    for tow_items in combinations(prices, 2):
        if sum(tow_items) == credit:
            return [i for i, price in enumerate(prices) if price in tow_items]
    assert False  # Is not suppose to happen

def run_on_file(filename):
    with open(filename) as file_:
        next(file_)
        for i, case in enumerate(zip(*[file_]*3)):
            credit = int(case[0])
            prices = [int(item) for item in case[2].split()]
            j, k = get_indices_with_sum(prices, credit)
            print("Case #{}: {} {}".format(i+1, j+1, k+1))

run_on_file(sys.argv[1])

Now, different great things are easily doable :
 - you can your call to run_on_file behind an if __name__ == "__main__": guard to have reusable code on one hand (you could need the get_indices_with_sum function to solve other problems) and code actually doing something on the other hand.
 - you can write tests to check your code automatically. The great thing is that the problem description gives you a few examples.

def unit_tests():
    j, k = get_indices_with_sum([5, 75, 25], 100)
    assert (j, k) == (1, 2)
    j, k = get_indices_with_sum([150, 24, 79, 50, 88, 345, 3], 200)
    assert (j, k) == (0, 3)
    j, k = get_indices_with_sum([2, 1, 9, 4, 4, 56, 90, 3], 8)
    assert (j, k) == (3, 4)

if __name__ == "__main__":
    unit_tests()
    # run_on_file(sys.argv[1])

At this step, your code is already better but we haven't improved performance. We've only make improvements easier to write and to test.

Algorithm

In order to write a faster algorithm, I am assuming we are interested in the case when we are given a store with a huge number of elements n (called I in the problem description).

At the moment, you (potentially) iterate over all combinations and there are n * (n-1)/2. Assuming you always find what you are looking for, we may assume that you go through n * (n-1) / 4 combinations in average. Then, when the solution is found, your iterate once more over prices so you roughly perform n + n * (n-1) / 4 operations. We usually keep only the asymptotic behavior of the program and remove the constant factor and say that your program is 'O(n^2)'.

You could try to run your program with bigger and bigger inputs and see the time it takes. Usually when you take a input twice as big, your algorithm will take 4 times as much time.

If you want to give it a try, I've tweaked the original test to add the following cases (adding elements costing 101 in the store doesn't change what you can get with 100 dollars):

nb_added_elements = 10000
j, k = get_indices_with_sum([101] * nb_added_elements + [5, 75, 25], 100)
assert (j, k) == (nb_added_elements + 1, nb_added_elements + 2)
j, k = get_indices_with_sum([5, 75, 25] + [101] * nb_added_elements, 100)
assert (j, k) == (1, 2)

The problem you are trying to solve is pretty common and has a O(n) solution on a sorted array (you'll find explanations looking for "array find pair sum" in our favorite search engine).

Your case is slightly more complicated because the array is not sorted and sorting it would cause a mess in the indices.

Many solutions are possible : either build an array with value and index before sorting it by value and/or keep a copy of the unsorted array so that you can find its initial position back at the end.

The resulting solution is likely to be O(n*log(n)) because of the sorting (other operations such as lookup/copy and obviously the proper algorithm are only O(n) and as such considered negligeable).

Other details

count and enumerate can be given a starting position (defaulting to 0) so that you don't have to add 1 yourself if you want to count from 0.


String extension method that truncates at sentence punctuation I've received a request at work and the senior developer is away on vacation so I can't ask him. The request was to truncate lengthy article titles while maintaining readability. In other words, don't simply chop a word in half and add an ellipsis.

I wrote an extension method that looks for punctuation and establishes length requirements.

Bullet Points:

truncate at sentence endings ('.','!','?')
allow for variable maxLength
make sure title is at least 30 chars long (for the case where a title starts like "wow!")
when all else fails truncate at last space and add an ellipsis

Anyway, the method works the way I intended, however, the programming feels gross.

What is a more succinct way of writing this string extension method?

I've reproduced the code in a fiddle:
.NET Fiddle

Extension method

public static class StringExtensions
{

    public static string TruncateAtFirstSentence(this string value, int maxLength)
    {

        char[] punc = new char[3];
        punc[0] = '.';
        punc[1] = '!';
        punc[2] = '?';

        // not the best programming, basically falls though when requirements
        // arent met.
        foreach (char c in value) {

            if (c == punc[2]) {

                var str = value.Substring(0, value.IndexOf("?") + 1);

                if (str.Length <= 30)
                    continue;
                else
                {
                    if (str.Length > maxLength)
                    {
                        continue;
                    }
                    else
                    {
                        return str;
                    }
                }
            }
            else if (c == punc[1]) {

                var str = value.Substring(0, value.IndexOf("!") + 1);

                if (str.Length <= 30)
                    continue;
                else
                {
                    if (str.Length > maxLength)
                    {
                        continue;
                    }
                    else
                    {
                        return str;
                    }
                }
            }
            else if (c == punc[0]) {

                var str = value.Substring(0, value.IndexOf(".") + 1);

                if (str.Length <= 30)
                    continue;
                else
                {
                    if (str.Length > maxLength)
                    {
                        continue;
                    }
                    else
                    {
                        return str;
                    }
                }
            }
        }

        var output = value.Length <= maxLength ? value : value.Substring(0, maxLength);
        output = output.Substring(0, output.LastIndexOf(' '));

        return output + "...";

    }

}

Main program

public class Program
{

    public static void Main()
    {

        string sentence = "This is a simple! question that ends with! a question mark. This is nearly the same. sentence that also ends with a period.";
        string longSentence = "This is a simple question that does not end with a question mark This is nearly the same sentence that ends with a period.";

        var output = StringExtensions.TruncateAtFirstSentence(sentence, 100);
        var output2 = StringExtensions.TruncateAtFirstSentence(longSentence, 100);

        Console.WriteLine("output 1: \n{0}\n", output);
        Console.WriteLine("output 2: \n{0}\n", output2);
    }
}

It's an interesting problem isn't it :)

Here's my extension method that does the same (stripped of some additional options)

public static string TruncateTo(
    this string input,
    int truncateLength,
    bool addEllipsis = true)
{
    // Check to see if we even need to bother truncating
    if (string.IsNullOrWhiteSpace(input) || input.Length <= truncateLength)
        return input;

    // copy the input
    var temp = input;

    // Define a list of characters that we can safely break on
    char[] anyOf = { '.', ',', ';', ':', '?', '!' };

    // LastIndexOfAny starts at the max position works backwards thru' the string
    var truncatePosition = temp.LastIndexOfAny(anyOf, truncateLength);

    // no appropriate place to truncate.
    if (truncatePosition == -1)
    {
        // Return original string but you could fall back to splitting on last space
        return temp;
    }
    temp = temp.Substring(0, truncatePosition + (addEllipsis ? 0 : 1));
    return addEllipsis
        ? temp + "..."
        : temp;
}

I've added comments but I think the code is pretty clear. I've also modified the code a fair bit as we support quite a few different ways of deciding where to split.

You'll also notice that it's greedy - I.e. it tries to keep the string as long as possible.


Canvas in Tkinter app for Conway's Game of Life updates so slowly This is Conway's Game of Life written in Python and using Tkinter for GUI.

Problems are:

The "Play" button works so slowly. I can see how the canvas is redrawing from top to bottom. But when I quickly click on the "step" button - I see that I want to get from "play" button: I don't see redrawing process. How can I make it faster and proper?
The __init__ method confuses me as it's so large. Are there a ways to reorganize methods or have I done it right?
How can I make this code more pythonic?

gui.py

# -*- coding: utf-8 -*-
from Tkinter import *
from processor import *
from functools import partial
import threading

class LifeGUI:
    def __init__(self, height, width):
        self.height = height
        self.width = width
        self.prev_x = self.prev_y = None
        self.root = Tk()
        self.field = init_field(height=self.height, width=self.width)
        self.canvas = Canvas(self.root, width=10 * len(self.field[0]), height=10 * len(self.field))
        self.clear()
        self.next_button = Button(self.root, bg="blue", text="step", command=self.step)
        self.next_button.pack()
        self.clear_button = Button(self.root, bg="yellow", text="clean", command=self.clear)
        self.clear_button.pack()
        self.clear_button = Button(self.root, bg="green", text="play", command=partial(self.step, {'repeat': True}))
        self.clear_button.pack()
        self.clear_button = Button(self.root, bg="red", text="stop", command=self.stop)
        self.clear_button.pack()
        self.canvas.bind("<Button-1>", self.switch_cell)
        self.canvas.bind("<B1-Motion>", self.switch_cell)
        self.canvas.bind("<ButtonRelease-1>", self.forget)
        self.canvas.pack()
        self.timer = threading.Timer(0.1, partial(self.step, {'repeat': True}))
        self.root.mainloop()

    def switch_cell(self, event):
        x = event.x / 10
        y = event.y / 10
        if (self.prev_x, self.prev_y) != (x, y):
            self.field[y][x] = not self.field[y][x]
            self.redraw()
        self.prev_x, self.prev_y = x, y

    def forget(self, event):
        self.prev_y = self.prev_x = None

    def step(self, repeat=False):
        self.field = next_step(self.field)
        self.redraw()
        if repeat:
            self.timer = threading.Timer(0.1, partial(self.step, {'repeat': True}))
            self.timer.start()

    def stop(self):
        self.timer.cancel()
        self.timer = None

    def clear(self):
        self.field = init_field(height=self.height, width=self.width)
        self.redraw()

    def redraw(self):
        self.canvas.delete("all")
        for i in range(len(self.field)):
            for j in range(len(self.field[0])):
                self.canvas.create_rectangle(j * 10, i * 10, (j + 1) * 10 - 2, (i + 1) * 10 - 2, fill="blue" if self.field[i][j] else "orange")

def main():

    LifeGUI(40, 40)

if __name__ == '__main__':
    main()

processor.py

# -*- coding: utf-8 -*-
def init_field(height=10, width=10, alives=()):
    fld = [[False] * width for _ in range(height)]
    for alive in alives:
        fld[alive[0]][alive[1]] = True
    return fld

def alive_neighbours(field, x, y):
    result = []
    for i in range(-int(y != 0), int(y != len(field[0]) - 1) + 1):
        for j in range(-int(x != 0), int(x != len(field) - 1) + 1):
            if (i or j) and field[x + j][y + i]:
                result.append((x + j, y + i))
    return result

def next_step(fld):
    result = [row[:] for row in fld]
    for i in range(len(fld)):
        for j in range(len(fld[1])):
            if fld[i][j] and len(alive_neighbours(fld, i, j)) not in (2, 3):
                result[i][j] = False
            elif len(alive_neighbours(fld, i, j)) == 3:
                result[i][j] = True
    return result

A few things to make your code better/faster :

You can use tuple unpacking to rewrite :

def init_field(height=10, width=10, alives=()):
    fld = [[False] * width for _ in range(height)]
    for alive in alives:
        fld[alive[0]][alive[1]] = True
    return fld

like this :

def init_field(height=10, width=10, alives=()):
    fld = [[False] * width for _ in range(height)]
    for x, y in alives:
        fld[x][y] = True
    return fld

You could compute alive_neighbours less often by storing the result :

        nb_neighbours = len(alive_neighbours(fld, i, j))
        if fld[i][j] and nb_neighbours not in (2, 3):
            result[i][j] = False
        elif nb_neighbours == 3:
            result[i][j] = True

I must confess that I find alive_neighbours really complicated.


Project Euler 1 - Multiples of 3 and 5 by Loki Multiples of 3 and 5

  Multiples of 3 and 5
  Problem 1

  If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

  Find the sum of all the multiples of 3 or 5 below 1000.

Used the command line (bash):

(seq 0 3 999; seq 0 5 999) | sort | uniq | xargs | tr ' ' '+' | bc

For the range it seemed good enough:

> time (seq 0 3 999; seq 0 5 999) | sort | uniq | xargs | tr ' ' '+' | bc
233168

real    0m0.021s
user    0m0.019s
sys     0m0.019s

You can replace sort | uniq with sort -u.
Note that (...) executes in a sub-shell.
Grouping with { ...; } would be more efficient,
and equivalent to what you did.

So without optimizing much, keeping it still simple and easy to type, you get:

{ seq 0 3 999; seq 0 5 999; } | sort -u | xargs | tr ' ' '+' | bc

Even so, sorting doesn't sound great. Probably it would be better to not sort, but add the sum of seq 0 -15 -999.

seq is not portable. And it's an additional process. You could either use {start..end..step} native Bash syntax, or native Bash counting for loops, but I bet you intentionally didn't because it's a bit longer to type.

And as Bash can easily do such simple math, you don't really need bc.

Here's a more efficient version, using fewer processes:

((x = $({ seq 0 3 999; seq 0 5 999; } | tr '\n' '+')$(seq 0 -15 -999))); echo $x

Here's another version using native Bash features only:

x=0
for ((i = 0; i < 1000; i += 3)); do ((x += i)); done
for ((i = 0; i < 1000; i += 5)); do ((x += i)); done
for ((i = 0; i < 1000; i += 15)); do ((x -= i)); done
echo $x

The most performant solution is of course using the closed form formula:

n=999; echo $((3 * (n / 3) * (n / 3 + 1) / 2 + 5 * (n / 5) * (n / 5 + 1) / 2 - 15 * (n / 15) * (n / 15 + 1) / 2))


Returning IQueryable from my repository in Repository pattern design pattern I am developing an application based on Repository design pattern. What should be the return type from the Repository?

This is my UserRepository class:

public class UserRepository : Repository<UserMaster>, IUserRepository
{
    public UserRepository(DbContext context)
        : base(context)
    {

    }
    public UserMaster GetById(int id)
    {
        return FindBy(x => x.Userid == id).FirstOrDefault();
    }

}

The base Repository class:

public abstract class Repository<T> : IRepository<T>
  where T : class
{
    protected DbContext _entities;
    protected readonly IDbSet<T> _dbset;

    public Repository(DbContext context)
    {
        _entities = context;
        _dbset = context.Set<T>();
    }

    public virtual IQueryable<T> GetAll()
    {

        return _dbset.AsQueryable<T>();
    }

    public IQueryable<T> FindBy(System.Linq.Expressions.Expression<Func<T, bool>> predicate)
    {

        IQueryable<T> query = _dbset.Where(predicate).AsQueryable();
        return query;
    }

    public virtual T Add(T entity)
    {
        return _dbset.Add(entity);
    }

    public virtual T Delete(T entity)
    {
        return _dbset.Remove(entity);
    }

    public virtual void Edit(T entity)
    {
        _entities.Entry(entity).State = System.Data.Entity.EntityState.Modified;
    }

    public virtual void Save()
    {
        _entities.SaveChanges();
    }
}

Is this the right way to return IQueryable from the repository? If I want to join two tables, where should I implement the join logic? Inside or outside the repository?

Currently my return type is IQueryable from the Repository and I am doing join on the Business layer after calling GetAll() function (returns IQueryable) of the required tables. Is this the right approach?

IQueryable is a leaky abstraction(you don't wan't to know anything about sql in the service layer).
It is also very difficult to test.

I would recommend not returning IQueryable.
I would return IEnumerable or ICollection and make my queries run in the repository(ToList on the repository).

I would create queries in my repositories to handle the joins.
You can test your queries separately for correctness and performance.


Convert dict entries to sets and lists Can you please tell me if there is a better way to do this?

people = {'123456':{'first': 'Bob', 'last':'Smith'},
          '2345343': {'first': 'Jim', 'last': 'Smith'}}

names = list()
first_names= set()
last_names = set()
for device in people:
    names.append(device)
    first_names.add(people[device]['first'])
    last_names.add(people[device]['last'])

I want to get a list of IDs, and sets of first and last Names. I know this code will work, but can I improve this?

Instead of appending the id's one by one, I would use:

ids = list(people.keys())

Note that I changed names to ids, as numbers don't seem like names to me. This is something that @jano's answer included but didn't explicitly state.

Then you can change your the rest of your code to:

first_names = set()
last_names = set()

for details in people.values():
     first_names.add(details['first'])
     last_names.add(details['last'])


Better way of checking if the current user already like a post I have three models which look like this:

class Post < ActiveRecord::Base
  belongs_to :user, :counter_cache => :count_post
  has_many :likes
end

class User < ActiveRecord::Base
  has_many :posts
  has_many :likes
end

class Like < ActiveRecord::Base
  belongs_to :user
  belongs_to :post, :counter_cache => :count_like
end

A user can only like once per Post so I have a method that checks if the current user already like a post and the dislike link is displayed.

<% if post.has_like_from? current_user %>
    <li id="like_<%= dom_id(post) %>"><%= link_to "like", create_like_path(:post_id => post.id), :remote => true %>

<% else %>
    <li id="dislike_<%= dom_id(post) %>"><%= render 'dislike', :post => post %>

<% end %>

And I have this method on model post:

 def has_like_from?(target_user)
   likes.where(:user_id => target_user.id).first == nil
 end

Is there any way to check every post if current user already like a post? I want to increase performance because load to many posts per request.

Example: startrace when load 7 records

Started GET "/" for 127.0.0.1 at 2014-06-30 23:32:51 +0700
  ←[1m←[35mUser Load (1.0ms)←[0m  SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1
Processing by HomesController#index as HTML
  ←[1m←[36mPost Load (1.0ms)←[0m  ←[1mSELECT "posts".* FROM "posts" ORDER BY created_at DESC←[0m
  ←[1m←[35mLike Load (1.0ms)←[0m  SELECT "likes".* FROM "likes" WHERE "likes"."post_id" = 7 AND "lik
es"."user_id" = 1 LIMIT 1
  ←[1m←[36mLike Load (0.0ms)←[0m  ←[1mSELECT "likes".* FROM "likes" WHERE "likes"."post_id" = 6 AND
"likes"."user_id" = 1 LIMIT 1←[0m
  ←[1m←[35mLike Load (1.0ms)←[0m  SELECT "likes".* FROM "likes" WHERE "likes"."post_id" = 5 AND "lik
es"."user_id" = 1 LIMIT 1
  ←[1m←[36mLike Load (1.0ms)←[0m  ←[1mSELECT "likes".* FROM "likes" WHERE "likes"."post_id" = 4 AND
"likes"."user_id" = 1 LIMIT 1←[0m
  ←[1m←[35mLike Load (0.0ms)←[0m  SELECT "likes".* FROM "likes" WHERE "likes"."post_id" = 3 AND "lik
es"."user_id" = 1 LIMIT 1
  ←[1m←[36mLike Load (0.0ms)←[0m  ←[1mSELECT "likes".* FROM "likes" WHERE "likes"."post_id" = 2 AND
"likes"."user_id" = 1 LIMIT 1←[0m
  ←[1m←[35mLike Load (0.0ms)←[0m  SELECT "likes".* FROM "likes" WHERE "likes"."post_id" = 1 AND "lik
es"."user_id" = 1 LIMIT 1
  Rendered homes/_posts.html.erb (29.0ms)

Have you created an index for likes.post_id? Note also that you can simplify that method by using has_many with the option through and  ActiveRecord#include?. That's what I'd write (with positive logic, I don't see why has_like_from? should perform a == nil):

class Post < ActiveRecord::Base
  belongs_to :user, :counter_cache => :count_post
  has_many :likes
  has_many :like_users, :through => :likes

  def liked_by?(user)
    like_users.include?(user)
  end
end


More elegant Sass @mixin to create pseudo-element shapes I picked up Sass last week, and I'm already diving into expressions. I have a @mixin that creates a CSS border triangle using the following call: .foo:after { ... @include triangle(bottom,8px,7px,8px,#4986c8); } producing something like:

Here's my @mixin:

@mixin triangle($dir,$tl,$size,$br,$clr) {
    $a: solid transparent;
    @if $dir == top {
        border-left: $tl $a; border-bottom: $size solid $clr; border-right: $br $a; border-top: 0;
    } @else if $dir == right {
        border-top: $tl $a; border-left: $size solid $clr; border-bottom: $br $a; border-right: 0;
    } @else if $dir == bottom {
        border-left: $tl $a; border-top: $size solid $clr; border-right: $br $a; border-bottom: 0;
    } @else if $dir == left {
        border-top: $tl $a; border-right: $size solid $clr; border-bottom: $br $a; border-left: 0;
    }
}

Works as expected; this just doesn't seem the most elegant way to write this particular mixin. How might you do this, or any improvement tips for my code?

In the same vein, I also created this (hopefully it is self-explanitory) to be used alongside:

@mixin pseudo-pos($dir) {
    content: ""; position: absolute;
    @if $dir == top { bottom: 100%; }
    @else if $dir == right { left: 100%; }
    @else if $dir == bottom { top: 100%; }
    @else if $dir == left { right: 100%; }
}

Refactoring

You can DRY this up a bit by taking advantage of lists and the zip function.

@mixin triangle($dir, $color, $size, $leg1: $size, $leg2: $leg1) {
    $sides:
        0,
        $leg1 solid transparent,
        $size solid $color,
        $leg2 solid transparent;

    $order:
        ( top: top right bottom left
        , right: right bottom left top
        , bottom: bottom right top left
        , left: left bottom right top);

    $x: zip(map-get($order, $dir), $sides);

    border: {
        @each $p, $s in $x {
            #{$p}: $s;
        }
    }
}

Misc.

Avoid using abbreviated variable names like $clr.  It might seem obvious to you that it's short for color, but I have a product sitting under my sink called CLR.  I had to read the mixin's code in order for me to discern what it was supposed to be for.

I've modified the argument order here and added some defaults so that you only have to give 1 piece of size information if you want an equilateral triangle, 2 pieces will give you an isosceles.  You probably don't really need that last argument, but I threw it in anyway.

Improving further

I don't have time at the moment to go into this, but things could further be improved by doing the following:

Specifying height/width rather than the desired border width
Using shorthand

You can see what I mean by looking over the results from a CSS triangle generator


Maximum product of word lengths in Python Problems was to calculate maximum product of word lengths:

  Given a string array words, find the maximum value of length(word[i])
  * length(word[j]) where the two words do not share common letters. You may assume that each word will contain only lower case letters. If
  no such two words exist, return 0.

Given ["abcw", "baz", "foo", "bar", "xtfn", "abcdef"]
Return 16
The two words can be "abcw", "xtfn".

Example 2:
Given ["a", "ab", "abc", "d", "cd", "bcd", "abcd"]
Return 4
The two words can be "ab", "cd".

Example 3:
Given ["a", "aa", "aaa", "aaaa"]
Return 0

My answer is correct and passed all the test cases. I am looking to get some code review on this.

class Solution(object):
    def maxProduct(self, words):
        """
        maximum value of length(word[i]) * length(word[j]) where the two words do not share common letters
        >> maxProduct(["eae", "ea", "aaf", "bda", "fcf", "dc", "ac", "ce", "cefde", "dabae"])
        >> 15
        """
        max_product = 0
        words = sorted(words, key=len, reverse=True)
        for index, j in enumerate(words):
            for i in words[index + 1:]:
                count = 0
                for k in i:
                    if k in j:
                        count = 1
                        break
                if not count:
                    m = len(j)
                    n = len(i)
                    word_product = m * n
                    if ((m == n) or (m > n and n == m - 1)) and word_product > max_product:
                        return word_product
                    if word_product > max_product:
                        max_product = word_product
        return max_product

My solution is quite different, let me explain it:

At first I define a small helper to check if two words have any letter in common, it may also be in-lined, but I like small functions.

def common(a, b):
    """
    >>> common("hello", "hi")
    {'h'}
    """
    return set(a) & set(b)

Then I define the relevant score function for the strings, as you see, after defining our criteria we are almost done.

def product_score(w1, w2):
    """
    >>> product_score("foo", "bar")
    9
    >>> product_score("foo", "fuu")
    0
    """
    return 0 if common(w1, w2) else len(w1) * len(w2)

The last step is very easy. We already know the criteria, so we can just map it over the combination of 2 of the input list and take the max:

def maxProduct(words):
    """
    Maximum value of length(word[i]) * length(word[j]) where the two words do not share common letters.

    >>> maxProduct(["eae", "ea", "aaf", "bda", "fcf", "dc", "ac", "ce", "cefde", "dabae"])
    15
    """
    return max(product_score(w1, w2)
                   for (w1, w2) in itertools.combinations(words, 2))

I think my solution is more readable, mainly because it uses smaller functions and because it uses already existing wheels (standard library modules).


SDL Initialization and Management wrappers I am using SDL2 for window management and rendering, but it can be a little verbose. And because of how SDL2 works under the hood, the SDL_Renderer is tied to image resource loading.

So, I am setting out to write a wrapper around the SDL initialization and window/renderer management.

The core of it is called SDLContext, which is responsible for initializing SDL, as well as being where the SDL_Window and SDL_Renderer will reside.

It is initialized with a helper struct, SDLOpts, which just holds any initialization values.

SDLOpts

struct SDLOpts {
  int flags = SDL_INIT_EVERYTHING;

  struct SDLWindowOpts {
    int flags = 0;
    int w = 600;
    int h = 480;
  } window_opts;

  struct SDLRendererOpts {
    int flags = 0;
  } renderer_opts;
};

SDLContext.h

struct SDL_Window;
struct SDL_Renderer;
class SDLContext {
public:
  explicit SDLContext(SDLOpts const& options) noexcept;
  ~SDLContext() noexcept;

  SDLWindow sdlWindow() const noexcept;
  SDLRenderer sdlRenderer() const noexcept;

private:
  SDLOpts options_ {};
  SDL_Window* sdlwindow_ {nullptr};
  SDL_Renderer* sdlrenderer_ {nullptr};

};

SDLContext.cpp

struct SDL_Window;
struct SDL_Renderer;
class SDLContext {
public:
  explicit SDLContext(SDLOpts const& options) noexcept;
  ~SDLContext() noexcept;

  SDLWindow sdlWindow() const noexcept;
  SDLRenderer sdlRenderer() const noexcept;

private:
  SDLOpts options_ {};
  SDL_Window* sdlwindow_ {nullptr};
  SDL_Renderer* sdlrenderer_ {nullptr};

};

One thing I am on the fence about is throwing in the constructor marked noexcept.
I don't want program execution to continue if I can't initialize SDL/create a window/create a renderer, and I also want to avoid using an Init function (trying to make it fit into RAII). But, I don't see any other alternatives.

SDLWindow and SDLRenderer are pretty bare-boned right now.
They just exist as a wrapper around all of the SDL_* functions that relate to SDL_Window or SDL_Renderer.

One decision I did make was to make their default constructor private, and mark SDLContext as a friend class. This way, the only way to create an SDLWindow or SDLRenderer is from within the SDLContext class. They should not be created outside of it, because there should only be one window and one renderer.

SDLRenderer.h

struct SDL_Renderer;
class SDLRenderer {
  friend class SDLContext;
public:
  ~SDLRenderer() = default;

  //...
  //Functions that draw or change the rendering properties (IE, color)
  //...
private:
  explicit SDLRenderer(SDL_Renderer* renderer) noexcept;

  std::experimental::observer_ptr<SDL_Renderer> sdlrenderer_ {nullptr};
};

SDLWindow.h

struct SDL_Window;
class SDLWindow {
  friend class SDLContext;
public:
  ~SDLWindow() noexcept = default;

  //...
  //Functions that modify the windows width and height
  //...
private:
  explicit SDLWindow(SDL_Window* window) noexcept;

  std::experimental::observer_ptr<SDL_Window> sdlwindow_ {nullptr};
};

Any suggestions to what I've done so far? Any alternatives to stopping program execution outside of throw in a noexcept / using an Init function? Is it sane to mark SDLContext as a friend class to SDLRenderer and SDLWindow in order to prevent them from being created wherever?

Throwing in a noexcept function

This is legal, see https://stackoverflow.com/questions/39763401/can-a-noexcept-function-still-call-a-function-that-throws-in-c17. However, you already have doubts about it, so that's telling you it's probably not a very proper thing to do. Compilers will also warn about this.

If you want your program to terminate when it can't create a window, I would make this explicit, and instead of using throw, call std::terminate() explicitly.

Define a namespace for your library

All your classes and structs have names with SDL prefixed, for good reasons. However, you can make it more explicit to the compiler that you want everything in a separate namespace. The advantage is that within that namespace, you don't have to use the prefix anymore. For example, you can write:

namespace SDL {
  class Window {
    friend class Context;
  public:
    ~Window() noexcept = default;
    ...
  };

  class Context {
  public:
    explicit Context(Opts const &options) noexcept;
    ~Context() noexcept;

    Window Window() const noexcept;
    Renderer Renderer() const noexcept;

  private:
    Opts options_ {};
    SDL_Window *window_ {};
    SDL_Renderer *renderer_ {};
  };
}

In your application, you can instantiate an Window like so:

SDL::Window myWindow;

Avoid forward declarations whenever possible

Forward declarations should only be used if really necessary. Otherwise, they are just duplicating code, with the potential to make mistakes. If you are using a SDL_Window * in your header files, just make sure you #include <SDL.h> in it.

Avoid friend classes

Why is SDLContext a friend class of SDLWindow? If it needs to access some private member variables, maybe you should add public accessor functions instead? From the code you posted I don't see any reason why a friend class is necessary. In general, it is something to be avoided, since it bypasses the usual member protection scheme.

Use of std::experimental

While we all would like to have the latest and greatest language features available, consider that your code might be compiled by others using older compilers. Using experimental C++20 features might prevent them from using your code. Furthermore, observer_ptr<> is not doing anything really, it's just there as an annotation. It's there to convey to other users that this pointer is not owned. SO it is more useful to use this for the public API than for private member variables. But, that also brings me to:

Have classes own their own pointers to SDL2 objects

It looks a bit weird that SDLWindow has a private constructor that takes a pointer to an SDL_Window. Why not have SDLWindow call SDL_CreateWindow() itself in its constructor? The same goes for SDLRenderer.

When you do this, then SDLContext should no longer store pointers to SDL_Window and SDL_Renderer, but it should instead store a SDLWindow and SDLRenderer directly:

class Context {
  Window window_;
  Renderer renderer_;

public:
  Context(...);
  ...
};


TypeScript 3 form validation object Please, correct me with anything that I say in here (The actual question is below the code).

I've been quickly prototyping a project (ASP.NET Core 2.1) and haven't found the need to structure it with Webpack/Parcel/Rollup/Browserify as it's done with simple JavaScript validations per views (I do minify the files with Gulp).

Since I'm not enclosing my files into modules, they're all open to the global scope (bad idea, right?). Therefore, I've decided to "namespace" them by running everything inside a JavaScript variable.

The code that I have looks like this:

const formValidation = {
  submitBtn : null as unknown as HTMLButtonElement,
  errors: null as unknown as string[],
  errorsDiv: null as unknown as HTMLDivElement,
  gradeSections: null as unknown as HTMLDivElement,
  form: null as unknown as HTMLFormElement,
  mapDOM() {
    this.submitBtn = document.getElementById('js-btn-submit') as HTMLButtonElement;
    this.errorsDiv = document.getElementById('js-errors') as HTMLDivElement;
    this.gradeSections = document.getElementById('js-grade-sections')  as HTMLDivElement;
    this.form = document.getElementById('js-submit-form') as HTMLFormElement;
    this.mapEventListenersFormValidation();
  },
  mapEventListenersFormValidation() {
    this.submitBtn.addEventListener('click', this.submit.bind(this));
  },
  submit(evt: Event) {
    console.log(evt);
    evt.preventDefault();
    this.resetForms();
    if (!this.checkIfSubmitIsValid()) {
      this.showErrors();
      return;
    }
    this.form.submit();
  },
  checkIfSubmitIsValid() {
    this.errors = [];
    const subjectSelected = this.isSubjectSelected();
    const sectionsSelected = this.areSectionsSelected();

    if (!subjectSelected) {
      selectSubjectDropdown.classList.add('is-invalid');
      this.errors.push('Debe de seleccionar por lo menos una materia');
    }

    if (!sectionsSelected) {
      this.gradeSections.classList.add('form-error');
      this.errors.push('Debe de seleccionar al menos una sección');
    }

    return subjectSelected && sectionsSelected;
  },
  isSubjectSelected() {
    return selectSubjectDropdown.selectedIndex !== 0;
  },
  areSectionsSelected() {
    return selectedSectionCounter > 0;
  },
  showErrors() {

    if (!this.errorsDiv || !this.errorsDiv.parentElement) {
      return;
    }
    this.errorsDiv.parentElement.removeAttribute('hidden');
    this.errorsDiv.innerHTML = '';
    this.errors.forEach((error) => {
      const li = document.createElement('li');
      li.innerHTML = error;
      this.errorsDiv.appendChild(li);
    });
  },
  resetForms() {
    selectSubjectDropdown.classList.remove('is-invalid');
    this.gradeSections.classList.remove('form-error');
  },

};

DOMReady(formValidation.mapDOM.bind(formValidation));

Check the submitBtn, errors, and errorsDiv properties. AFAIK, JavaScript doesn't have a way to declare an "empty" property. Therefore something sane would be to assign it either undefined or null.

As I want to have it statically defined (through TypeScript), I want to assign a type to those properties. The problem is that I just can't assign a non-nullable type to a null.

As I'm trying to avoid non-implicit any's in the scripts, the compiler suggested me to use unknown.

The question is, whether it's ok for me to use the unknown type from TypeScript to cast a null to an unknown so it can let me cast it to the type that I'm going after.

Edit Ended up adding the whole document into it. I was working with that function, so it's different now.

The compiler can't know beforehand if document.getElementById('js-btn-submit') wil return anything. That is why you cannot assign the result directly to a type.

One solution is to tell the compiler that you are certain that the HTML element exists, using !

let button = document.getElementById("button")! as HTMLButtonElement

If you are NOT certain that the button exists in the HTML document, you can use the suggestion given by VS Code:

If you hover the mouse over document.getElementById you can see that the returned type is null | HTMLElement

I've rewritten your object as a class, because in object notation the colon is used to pass the value, not the type: const obj = {a:3}

class FormValidation {
    submitBtn: null | HTMLButtonElement

    mapDOM() {
        this.submitBtn = document.getElementById('js-btn-submit') as HTMLButtonElement;
    }
}

const f = new FormValidation()
f.mapDOM()


How could I make this PHP $_POST more secure? -Or is it secure already? On most my $_post data inputted on my site I use the following php:

$example = $_POST['textfield'];
$example = strip_tags($example);
$example = mysql_real_escape_string($example);

And then I would interact with the MySQL database...

Is this 'secure' / 'safe'?

Any major exploits to the above code?

-How could I make it secure?

Thanks alot.

I recommend getting yourself a simple, simple function that uses PDO and parameterizing of queries by default behind the scenes.

Here are sample use cases for query functions that use PDO in the background that I use all the time:

$iterable_resultset = query('select * from users where email = :email and username ~* :username or user_id = :user_id', array(':email'=>$email, ':username'=>$username, ':user_id'=>(int)$user_id));
// Results a resultset that is able to be foreached over.

$multidimensional_array = query_array('select * from users where email = :email and username ~* :username or user_id = :user_id', array(':email'=>$email, ':username'=>$username, ':user_id'=>(int)$user_id));
// Results in a multidimensional array with each subarray corresponding to a database row, or null.

$row_of_data = query_row('select * from users limit 1');
//Results in a single dimensional array or null.

$item = query_item("select max(score) from players where username ~* :username", array(':username'=>$username));
// Returns a single piece of information, or null.

There's a lot of advantage from a refactoring viewpoint to having simple function-based parameterizing of sql queries.  PDO is often verbose, and sometimes when you're securing bad code you need simple, terse functions to fit in the place of old mysql_query() calls and the like.

In your specific case, I think that you are making the mistake of escaping before it's time, which can often introduce weird bugs.

$untrusted_input = @$_REQUEST['password'];

is all that you should do when getting the input.  Don't strip tags, don't do anything until it comes time to -use- that untrusted input in sql or in html:

In the sql, parameterize it while using the sql, filter/modify the data based on the circumstances.
When outputting to html, escape it then, e.g. via .htmlentities($example). or using a templating engine.

Here's an example of your code turning a non-existent get/post variable (that would normally result in null) into an empty string, for just the first of many subtle things that happen when you escape too early: http://ideone.com/r7bSr

In general, when you're first getting used to the security issues that php can throw your way, I really recommend using a template engine (e.g. smarty) to help you keep your business logic and your display templates separate.  A lot of people will argue about whether you need a templating engine with php because php is a templateable system already.  Engine or not doesn't matter, but a template-based approach to separate your manipulation logic from your display logic is very necessary, which most programmers would probably agree on.  And personally, I found it easier to learn how that separation benefits php when I was first starting out by just learning how a templating engine does things, before I could understand how to apply those principles to native php without a templating engine.

TL;DR; - Set up a templating engine, it'll get you using best practices faster.


Checking the balanced parenthesis as asked in interview Input:

  Input contains one string S which consists of big and small latin
  letters, digits, punctuation marks and brackets from the set []{}().

  Constraint:

  Constraints. The length of S is at least 1 and at most 105

  Output:

  If the code in S uses brackets correctly, output “Success" (without
  the quotes). Otherwise, output the 1-based index of the first
  unmatched closing bracket, and if there are no unmatched closing
  brackets, output the 1-based index of the first unmatched opening
  bracket.

Code:

console.clear();

function hashBalancedParanthesis(string) {
  // Pre: The string should have ASCII characters
  var len = string.length;
  var stack = [];

  for (var i = 0; i < len; i++) {
    var char = string[i];
    //console.log(stack + " : " + char);
    if ('([{'.indexOf(char) !== -1)             // O(3 * len)
      stack.push(char);
    else if (')]}'.indexOf(char) !== -1) {      // O(3 * len)
      // Below operations are O(n) time
      var top = stack.pop();

      if (top === '[' && char !== ']') return string.indexOf(char) + 1;
      if (top === '{' && char !== '}') return string.indexOf(char) + 1;
      if (top === '(' && char !== ')') return string.indexOf(char) + 1;
    }
  }
  // Post: When here the stack should be empty for balanced case
  if (stack.length !== 0) return string.indexOf(stack.pop()) + 1;
  return true;
};

// False
console.log(hashBalancedParanthesis('('));
console.log(hashBalancedParanthesis('{[}'));
console.log(hashBalancedParanthesis('foo(bar[i);'));

// True
console.log(hashBalancedParanthesis(''));
console.log(hashBalancedParanthesis('{}[]'));
console.log(hashBalancedParanthesis('[()]'));
console.log(hashBalancedParanthesis('(())'));
console.log(hashBalancedParanthesis('foo(bar);'));

Description:

Instead of hard work since last three years I got rejected in one of the dream companies so, now I would like to have clear understanding of running time and edge cases as well as code quality.

Naming is now second priority.

As per my knowledge the above code runs in linear time \$\Theta(n)\$ where n is the string length.

PS: I am going to write a detailed analysis of my interview experience I would like to know which place would be better for it.

Returning mixed types.

return string.indexOf(stack.pop()) + 1;
return true;

This is a dangerous design for the interface, as it requires strict type checks in the calling code to distinguish between the error case (non-zero integer) and the success case (true boolean).

Stick to a single data type for the return value at all cost. This would have been quite easy in this case, as every error case yields a non-zero integer. So just returning 0 to signal error free execution would have been acceptable.

Not fulfilling the requirements

  if there are no unmatched closing brackets, output the 1-based index of the first unmatched opening bracket.

Said ahead, the "fix" suggested by @BrunaCosta doesn't work either.

The only way to reconstruct this information, is to record it when creating the stack. You can't identify the position correctly afterwards.

This shows that you lack understanding of what data you need to compute in order to solve the problem.

Proper testing

console.log(hashBalancedParanthesis('('));

This is by no means a unit test. A test is characterized by either failing or succeeding in a replicable way. Simply sending the output to console does neither, even less so when the expected value is only denoted as a comment in your code, making it impossible to run the tests automatically.

If you don't want to use a full testing framework, the least you could have done would have been to use the builtin assert() function to assert that each of your test cases either succeeds, or your test suite fails.

Your tests should not have been placed in the main file either. Even less so, when that means that they run on every inclusion of your library. And less again when they cause log spam without any good reason at that.

As pointed out by @BrunaCosta, your unit tests are also flawed in another way, as they don't test any non-trivial case.

Separation of concern

function hashBalancedParanthesis(string);

Your approach of separating the actual algorithm from the input/output handling was almost correct.

Except that you forgot to implement the latter part. You are neither capturing any input, nor are you creating any output. Except for the log spam resulting from placing your strange test cases into main file.

Providing a user interface of any sort is usually considered part of these challenges, even if it's just a console application which reads from stdin and outputs to stdout. Or in the case of JavaScript best a small HTML application.

Your comments are a lie

// False

The value False will never appear on the console.

Whenever you comment something - and you should if something isn't self-explanatory - you must take care to update the comments as well. A comment which is out of sync with the commented code is far worse than not having a comment at all.


Randomly pick a host from a list, subject to an uptime requirement I have a POJO as shown below which will have hostname and other attributes.

public class MachineInfo {

    private String hostname;

    // getters and setters here
}

I have my below method from which I need to randomly pick a hostname which is up for more than 10 minutes from machines list. The checkHostUptime method will return true if a particular machine is up for more than 10 minutes.

public static String pickRandomMachine(List<MachineInfo> machines) {
    String hostname = null;
    Collections.shuffle(machines, new Random(System.nanoTime()));
    for (MachineInfo info : machines) {
        if (!checkHostUptime(info.getHostName())) {
            continue;
        }

        hostname = info.getHostName();
        break;
    }

    return hostname;

}

I want to keep on trying finding the hostname until I find the hostname which is up for more than 10 minutes from machines list. If I am not able to find anything, then I would simply return null.

Is there any better way of doing the same thing?

Naming

by calling pickRandomMachine() where I pass a List<MachineInfo> I would assume that I will get a MachineInfo. Better name it to getRandomMachineHostName()
if you name the parameter holding the List machines you should rename info to machine.
hostname should be hostName, because it is camelCase casing for variable names.

General

A Random should be created once and be reused.
You should place and use the String hostname inside the loop, to remove the need to accessing machine two times.
You should extract the shuffeling of the machines to a separate method.
By returning out of the loop we can reverse the if condition

Refactoring

Implementing the above will lead to

public static String getRandomMachineHostName(List<MachineInfo> machines) {

    shuffleMachines(machines);

    for (MachineInfo machine : machines) {
        String hostName = machine.getHostName();
        if (checkHostUptime(hostName)) {
            return hostName;
        }
    }

    return null;
}

private static Random random = new Random(System.nanoTime());
private static void shuffleMachines(List<MachineInfo> machines) {
    Collections.shuffle(machines, random);
}


Largest subarray with sum equal to 0 This is a typical interview question:

  Given an array that contains both positive and negative elements without 0, find the largest subarray whose sum equals 0.

I am not sure if this will satisfy all the edge case. If someone can comment on that, it would be excellent. I also want to extend this to sum equaling to any ,  not just 0. How should I go about it? And any pointers to optimize this further is also helpful.

from collections import Counter

def sub_array_sum(array,k=0):
    start_index = -1
    hash_sum = {}
    current_sum = 0
    keys = set()
    best_index_hash = Counter()
    for i in array:
        start_index += 1
        current_sum += i
        if current_sum in hash_sum:
            hash_sum[current_sum].append(start_index)
            keys.add(current_sum)
        else:
            if current_sum == 0:
                best_index_hash[start_index] = [(0,start_index)]
            else:
                hash_sum[current_sum] = [start_index]
    if keys:
        for k_1 in keys:
            best_start = hash_sum.get(k_1)[0]
            best_end_list = hash_sum.get(k_1)[1:]
            for best_end in best_end_list:
                if abs(best_start-best_end) in best_index_hash:
                    best_index_hash[abs(best_start-best_end)].append((best_start+1,best_end))
                else:
                    best_index_hash[abs(best_start-best_end)] = [(best_start+1,best_end)]

    if best_index_hash:
        (bs,be) = best_index_hash[max(best_index_hash.keys(),key=int)].pop()
        print array[bs:be+1]
    else:
        print "No sub array with sum equal to 0"

def Main():
    a = [6,-2,8,5,4,-9,8,-2,1,2]
    b = [-8,8]
    c = [7,8,-1,1]
    d = [2200,300,-6,6,5,-9]
    e = [-9,-6,8,6,-14,9,6]
    sub_array_sum(a)
    sub_array_sum(b)
    sub_array_sum(c)
    sub_array_sum(d)
    sub_array_sum(e)

if __name__ == '__main__':
    Main()

I have to admit I didn't fully understood the algorithm. Anyway it seems suboptimal, due to use of heaviweight containers and a nested loops, and is really hard to follow.

I would rather start with an observation that replacing the array with its partial sums reduces the problem to finding two equal elements as distant as possible (two partial sums being equal means the sum in between is 0). This is pretty trivial:

Calculate partial sums (\$O(n)\$)
Stable sort (sum(i),i) tuples (\$O(n\log^2n\$))
Scan for largest equal range (\$O(n)\$)


JSON data from Twitter to Edgelist? Following along another code review (Printing out JSON data from Twitter as a CSV) I would like to submit a slightly adapted code for review.

This code imports JSON data obtained from Twitter and prints out just the tweet author and if there are any users the author mentions in the tweet.  User_mentions is a field that is provided in the JSON output.  The difficult part I've encountered is that an author sometimes doesn't mention anyone, or mentions 5 other user.  So I'm not sure of the best way to account for this, besides what I've cobbled together below.

My ultimate goal:
Create an edgelist from this data to then put into a network visualization tool. I've been converting the output (example below) from this code using a UNIX command (also below) I wrote, but if anyone has a better way to do this within this code, please do let me know.

Format after code:

author1,mention1,mention2,mention3
author1,mention4
author2 author3,mention5
author4,mention3,mention6

Ultimate format in CSV:

author1,mention1
author1,mention2
author1,mention3
author1,mention4
author3,mention5
author4,mention3
author4,mention6

Python Code:

import json
import sys

tweets=[]

# import tweets from JSON
for line in open(sys.argv[1]):
try:
    tweets.append(json.loads(line))
except:
pass

# create a new variable for a single tweets
tweet=tweets[0]

# pull out various data from the tweets
tweet_author = [tweet['user']['screen_name'] for tweet in tweets]
tweet_mention1 = [(tweet['entities']['user_mentions'][0]['screen_name'] if len(tweet['entities']['user_mentions']) >= 1 else None) for tweet in tweets]
tweet_mention2 = [(tweet['entities']['user_mentions'][1]['screen_name'] if len(tweet['entities']['user_mentions']) >= 2 else None) for tweet in tweets]
tweet_mention3 = [(tweet['entities']['user_mentions'][2]['screen_name'] if len(tweet['entities']['user_mentions']) >= 3 else None) for tweet in tweets]
tweet_mention4 = [(tweet['entities']['user_mentions'][3]['screen_name'] if len(tweet['entities']['user_mentions']) >= 4 else None) for tweet in tweets]
tweet_mention5 = [(tweet['entities']['user_mentions'][4]['screen_name'] if len(tweet['entities']['user_mentions']) >= 5 else None) for tweet in tweets]
tweet_mention6 = [(tweet['entities']['user_mentions'][5]['screen_name'] if len(tweet['entities']['user_mentions']) >= 6 else None) for tweet in tweets]
tweet_mention7 = [(tweet['entities']['user_mentions'][6]['screen_name'] if len(tweet['entities']['user_mentions']) >= 7 else None) for tweet in tweets]
tweet_mention8 = [(tweet['entities']['user_mentions'][7]['screen_name'] if len(tweet['entities']['user_mentions']) >= 8 else None) for tweet in tweets]
tweet_mention9 = [(tweet['entities']['user_mentions'][8]['screen_name'] if len(tweet['entities']['user_mentions']) >= 9 else None) for tweet in tweets]
tweet_mention10 = [(tweet['entities']['user_mentions'][9]['screen_name'] if len(tweet['entities']['user_mentions']) >= 10 else None) for tweet in tweets]

#outputting to CSV
out = open(sys.argv[2], 'w')

rows = zip(tweet_author, tweet_mention1, tweet_mention2, tweet_mention3,    tweet_mention4, tweet_mention5, tweet_mention6)

from csv import writer
csv = writer(out)

for row in rows:
    values = [(value.encode('utf8') if hasattr(value, 'encode') else value) for value in row]
    csv.writerow(values)

out.close()

UNIX command:
Used to take the output of this code and format as an edgelist. If this part can be worked into the above code, it would be much appreciated!

cat [file.txt] | sed 's/,/ /g' | awk '{print $1, $2 "##" $1, $3 "##" $1, $4 "##" $1, $5 "##" $1, $6 "##" $1, $7 "##" $1, $8 "##" $1, $9 "##" $1 $10}' | sed 's/##/\n/g' | sed 's/ /,/g'

I've found another answer on https://github.com/alexhanna to this question  that I've been able to adapt and enrich to somewhat meet my needs:

import json
import sys
from csv import writer
import time
from datetime import datetime

startTime = datetime.now()

with open(sys.argv[1]) as in_file, \
    open(sys.argv[2], 'w') as out_file:
    print >> out_file
    csv = writer(out_file)
    tweet_count = 0

    for line in in_file:
        tweet_count += 1
        try:
            tweet = json.loads(line)
        except:
            pass

        if not (isinstance(tweet, dict)):
            pass
        elif 'delete' in tweet:
            pass
        elif 'user' not in tweet:
            pass
        else:
            if 'entities' in tweet and len(tweet['entities']['user_mentions']) > 0:
                user = tweet['user']
                user_mentions = tweet['entities']['user_mentions']

                for u2 in user_mentions:
                    print ",".join([
                        user['screen_name'],
                        u2['screen_name']
                        ])

#values = [(value.encode('utf8') if hasattr(value, 'encode') else value) for value in row]
#csv.writerow(values)

print "File Imported:", str(sys.argv[1])
print "# Tweets Imported:", tweet_count
print "File Exported:", str(sys.argv[2])
print "Time Elapsed:", (datetime.now()-startTime)


Threaded and asyncio API library In Python I am trying to create an API for a connected device. I want to be available for both threaded (using request) and async applications (using aiohttp).
What I've come up with is wrapping the get method of both requests and aiohttp in a decorator. This decorator is passed at init and API calls are explicitly wrapped using the passed decorator.

It works, but I'd like to know how others think of this approach ? Are there better ways or will I be running into issues later on ?

def threaded_gett(function):
    # The threaded decorator
    def wrapper(*args, **kwargs):
        url, params = function(*args)
        response = requests.get(url, params)
        _json = response.json()
        return function.__self__.process_response(_json)

    return wrapper

def async_gett(function):
    # The async decorator
    def wrapper(*args, **kwargs):
        url, params = function(*args)
        try:
            resp = yield from function.__self__.session.get(url, params=params)
        except Exception as ex:
            lgr.exception(ex)
        else:
            _json = yield from resp.json()
            yield from resp.release()
            return function.__self__.process_response(_json)

    # wrapping the decorator in the async coroutine decorator.
    wrapper = asyncio.coroutine(wrapper)
    return wrapper

class ThreadedApi(BaseApi):
    def __init__(self,threaded_gett):
        BaseApi.__init__(self,threaded_gett)

class AsyncApi(BaseApi):
    def __init__(self,async_gett):
        BaseApi.__init__(self,async_gett)

class BaseApi():
    def __init__(self,get_wrapper):
        self.status = get_wrapper(self.status)

    def status(self):
        return <status path>

Your code is not complete but perhaps I've got your idea.

self.status = get_wrapper(self.status) looks slightly strange to me (while it works, sure).

I would prefer to use class decorators:

import asyncio
import functools

import aiohttp
import requests

def public(func):
    func.public = True
    return func

def sync_api(cls):
    def wrapper(func):
        @functools.wraps(func)
        def wrapped(self, *args, **kwargs):
            url, params = func(self, *args, **kwargs)
            response = requests.get(url, params)
            _json = response.json()
            return self.process_response(_json)

    for name in dir(cls):
        func = getattr(cls, name)
        if getattr(func, 'public'):
            setattr(cls, name, wrapper(func))

def async_api(cls):
    def wrapper(func):
        @functools.wraps(func)
        @asyncio.coroutine
        def wrapped(self, *args, **kwargs):
            url, params = func(self, *args, **kwargs)
            session = self.session
            try:
                resp = yield from session.get(url, params=params)
            except Exception as ex:
                logger.exception(ex)
                raise
            try:
                _json = yield from resp.json()
                yield from resp.release()
            except Exception as ex:
                logger.exception(ex)
                resp.close()
                raise
            else:
                return self.process_response(_json)

    for name in dir(cls):
        func = getattr(cls, name)
        if getattr(func, 'public'):
            setattr(cls, name, wrapper(func))

class BaseApi:

    @public
    def status(self):
        return 'http://example.com', {}

@sync_api
class ThreadedApi(BaseApi):
    pass

@async_api
class AsyncApi(BaseApi):

    def __init__(self):
        self.session = aiohttp.Session()

P.S. Please keep in mind that yield from resp.json() may raise an exception too.

P.P.S.
If you are using Python 3.4 I strongly suggest upgrading to 3.5.

Otherwise please use famous async def/await syntax.
async with session.get() could simplify the snippet a little.


Binary Search Tree to store objects by object ID I'm trying to create a binary search tree that would be used to insert my Packet objects'. It holds information like partId, description, price, and partCount. It's a basic binary search tree. Could you take a look and tell me if there's nothing odd in the code? Because I'm trying to brush up on my BST. Please give me your comments and reviews.

I'm not doing anything complicated, just simple BST. I haven't done C++ in months, trying to review everything again.

This is my testing program:

#include <iostream>
#include "BST.h"
#include "Packet.h"

using namespace std;

int main() {
    BST test1;
    cout << "-------------------------------------------------------" << endl;
    cout << "Testing BST" << endl;
    cout << "-------------------------------------------------------" << endl;
    Packet packetTest(123, "This is a packet of cheese.", 12.95, 10);
    Packet packetTest1(321, "This is a packet of cheese.", 12.95, 10);
    Packet packetTest2(456, "This is a packet of cheese.", 12.95, 10);
    Packet packetTest3(7, "This is a packet of cheese.", 12.95, 10);
    Packet packetTest4(119, "This is a packet of cheese.", 12.95, 10);
    Packet packetTest5(456, "This is a packet of cheese.", 12.95, 10);
    test1.insert(packetTest);
    test1.insert(packetTest1);
    test1.insert(packetTest2);
    test1.insert(packetTest3);
    test1.insert(packetTest4);
    test1.insert(packetTest5);
    test1.preorderTraversal();
    system("pause");
}

Here's my BST.h:

#pragma once

#include <string>
#include <iostream>
#include <stack>
#include "Packet.h"
using namespace std;

class Node
{
    friend class BST;
public:
    Node() : packet(packet), rlink(nullptr), llink(nullptr) {}
    ~Node() {}

private:
    Packet *packet;
    Node *rlink, *llink;
};

class BST {
public:
    BST();
    void BST::insert(Packet& packet);
    void BST::insert(Node* &p, Node *newNode);
    void preorderTraversal() const;
private:
    Node *root;
    void preorderTraversal(const Node *p) const;
};

The BST.cpp Careful with this part, I might have done some things here, can you take an extra look here?:

#include "BST.h"

BST::BST() : root(nullptr) {}

void BST::insert(Packet& thisPacket) {
    Node *newNode = new Node;
    newNode->packet = &thisPacket;
    insert(root, newNode);
}

void BST::insert(Node *&p, Node *newNode)
{
    if (p == nullptr)
        p = newNode;
    else if (p->packet->getPartId() < newNode->packet->getPartId())
        insert(p->llink, newNode);
    else
        insert(p->rlink, newNode);
}

void BST::preorderTraversal() const {
    if (root == nullptr)
        cerr << "There is no tree.";
    else
    {
        preorderTraversal(root);
    }
}

void BST::preorderTraversal(const Node *p) const {
    if (p != nullptr) {
        cout << p->packet->getPartId() << " ";
        preorderTraversal(p->llink);
        preorderTraversal(p->rlink);
    }
}

And finally Packet.h:

#pragma once
#include <string>

using namespace std;

class Packet {
public:
    Packet(int partId, string description, double price, int partCount) :
        partId(partId), description(description), price(price), partCount(partCount) {}
    int getPartId() const {return partId;}

private:
    int partId;
    string description;
    double price;
    int partCount;
};

Stop using namespace std;

Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h.

Unnecessary includes

Again BST.h. There are a lot of unused includes in there. The only one that might serve a purpose would be Packet.h which could easily be replaced by a forward declaration. Most of the include should go to the corresponding .cpp file.

The BST

I'm not sure about the interface. If your intended usage is to pass packages by reference, it would be desirable to declare BST::insert(Node *&p, Node *newNode) as private, similar to the public and private versions of preorderTraversal. preorderTraversal might also be better called printPreorder or something like this since it only prints the nodes and does not allow to access them.

There is also a small memleak in insert since those Node*s created with Node *newNode = new Node; will never get deleted. valgrind confirms this. As @TobySpeight rightfully pointed out in his comment and detailed in his answer, one can argue if leaking all of the tree's nodes should really be considered as a "small" memleak. A possible solution for this is to use smart pointers from <memory>. There is a good overview in this blog post by Herb Sutter on which type of smart pointer (e.g. std::unique_ptr or std::shared_ptr) as well as the type of parameter passing (by-reference vs. by-value) should be used to express a certain "meaning".


Beginner user registration and login program I'm new to coding and C++, this is one of my first pieces of "software". Could anyone give me some constructive criticism?

#include <iostream>
#include <fstream>
#include <string>

using namespace std;

bool IsLoggedIn()
{
    string user, pass, un, pw;
    cout << "Username: " << endl;
    cin >> user;
    cout << "Password: " << endl;
    cin >> pass;

    ifstream read(user + ".txt");
    getline(read, un);
    getline(read, pw);

    if (un == user && pw == pass)
        return true;
    else
        return false;

}

inline bool does_file_exist(const string& name)
{
    ifstream filename(name +".txt");
    if(!filename)
        return false;
    else
        return true;
}

int main()
{
    system("clear");

    int input;
    cout << "(1) Create Account \n" << "(2) Log in \n"<< "(3) Change password" << endl;
    cin >> input;
    if (input == 1 || input == 2 || input == 3)
    {
        if (input == 1)
        {
            system("clear");

            string user, pass;

            cout << "Create Account \n" << endl;
            cout << "Username: " << endl;
            cin >> user;

            if (does_file_exist(user))
            {
                cout << "Username already registered \n" << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
            }

            cout << "Password: " << endl;
            cin >> pass;

            ofstream file;
            file.open(user + ".txt");
            file << user << endl << pass;
            file.close();

            main();
        }

        else if (input == 2)
        {
            system("clear");

            cout << "Log In \n" << endl;

            bool status = IsLoggedIn();

            if (!status)
            {
                cout << "Incorrect username or password!" << endl << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
                return 0;
            }
            else
            {
                cout << "Successfully logged in!" << endl;
                cin.get();
                return 1;
            }

        }

        else if (input == 3)
        {
            system("clear");

            string user, pass;

            cout << "Change password \n" << endl;
            cout << "Username: " << endl;
            cin >> user;
            if (does_file_exist(user))
            {
                cout << "New Password: " << endl;
                cin >> pass;

                ofstream file;
                file.open(user + ".txt");
                file << user << endl << pass;
                file.close();

            main();
            }
            else
            {
                cout << "Username not registered \n" << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
            }

        }
    }
    else
    {
        system("clear");
        cout << "Invalid input" << endl;
        main();
    }
}

Not a full review, but too long for a comment

I'm not a C++ developer, but there are a few common foundational programming mistakes that you've made that I saw:

using namespace std is a bad practice. This imports the entirety of the namespace into the current namespace, which can cause a multitude of problems. Here is an article that explains the possible problems, going more in-depth than I could ever. *Note: I didn't change anything that would require std:: at the beginning by removing the namespace, merely for the fact that I don't have the time right now. That can be a good exercise for you :).
Instead of return true; } else { return false; }, simply return the expression you're evaluating. For example in your code, return (un == user && pw == pass).
When a user enters an already existing username/incorrect username or password/username not registered, you call main() again. This can fill the stack pretty quickly with these recursive calls. What if a user keeps making mistakes? Then you have tens or even hundreds of main calls in the stack. Not good. You can wrap all the code in main in a while loop. If a user makes a mistake, simply do nothing and let the loop reset. If a user makes a valid choice, do what you need to do, then break. Note: Didn't implement this either because of time, this one is for you :).

Barely changed but still changed code

#include <iostream>
#include <fstream>
#include <string>

//ADD "std::" TO ALL THINGS THAT COME FROM STD NAMESPACE

bool IsLoggedIn()
{
    string user, pass, un, pw;
    cout << "Username: " << std::endl;
    cin >> user;
    cout << "Password: " << endl;
    cin >> pass;

    ifstream read(user + ".txt");
    getline(read, un);
    getline(read, pw);

    return (un == user && pw == pass);

}

inline bool does_file_exist(const string& name)
{
    ifstream filename(name +".txt");

    return filename;

}

int main()
{
    system("clear");

    int input;
    cout << "(1) Create Account \n" << "(2) Log in \n"<< "(3) Change password" << endl;
    cin >> input;
    if (input == 1 || input == 2 || input == 3)
    {
        if (input == 1)
        {
            system("clear");

            string user, pass;

            cout << "Create Account \n" << endl;
            cout << "Username: " << endl;
            cin >> user;

            if (does_file_exist(user))
            {
                cout << "Username already registered \n" << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
            }

            cout << "Password: " << endl;
            cin >> pass;

            ofstream file;
            file.open(user + ".txt");
            file << user << endl << pass;
            file.close();

            main();
        }

        else if (input == 2)
        {
            system("clear");

            cout << "Log In \n" << endl;

            bool status = IsLoggedIn();

            if (!status)
            {
                cout << "Incorrect username or password!" << endl << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
                return 0;
            }
            else
            {
                cout << "Successfully logged in!" << endl;
                cin.get();
                return 1;
            }

        }

        else if (input == 3)
        {
            system("clear");

            string user, pass;

            cout << "Change password \n" << endl;
            cout << "Username: " << endl;
            cin >> user;
            if (does_file_exist(user))
            {
                cout << "New Password: " << endl;
                cin >> pass;

                ofstream file;
                file.open(user + ".txt");
                file << user << endl << pass;
                file.close();

            main();
            }
            else
            {
                cout << "Username not registered \n" << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
            }

        }
    }
    else
    {
        system("clear");
        cout << "Invalid input" << endl;
        main();
    }
}


Simple Java web app code I wanted to work on HttpSessions and JSP.

This is the view I have:

<!DOCTYPE html>

    <meta charset="utf-8">

    <c:out value="All available products:"/>

    <c:forEach items="${applicationScope.allProducts}" var="product">
        <c:out value="${product}"/>
        &nbsp;
        <a href="${pageContext.servletContext.contextPath}/shop?addProductbyName=<c:out  value="${product}"/>">Add to basket

    </c:forEach>

    Currently your cart contains these items dude:

    <c:forEach items="${sessionScope.cart}" var="product">
        <c:out value="${product.key}"/>
        &nbsp;
        <c:out value="${product.value}"/>

    </c:forEach>

And the Servlet:

package com.tugay;

import javax.servlet.ServletException;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

/**
 * User: koraytugay
 * Date: 20/07/14
 * Time: 18:07
 */

@WebServlet(urlPatterns = "/shop", name = "shoppingServlet")
public class ShopServlet extends HttpServlet {

    @Override
    public void init() throws ServletException {
        if (getServletContext().getAttribute("allProducts") == null) {
            getServletContext().setAttribute("allProducts", Database.getAllProducts());
        }
    }

    @SuppressWarnings("unchecked")
    @Override
    protected void doGet(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse)
            throws ServletException, IOException {
        Map<String, Integer> cart = doSessionStuff(httpServletRequest, httpServletResponse);
        String addProductbyName = httpServletRequest.getParameter("addProductbyName");
        if (addProductbyName != null) {
            if (cart.get(addProductbyName) == null) {
                cart.put(addProductbyName, 0);
            }
            cart.put(addProductbyName, cart.get(addProductbyName) + 1);
        }

        httpServletResponse.sendRedirect(getServletContext().getContextPath()+"/shopping.jsp");
    }

    @SuppressWarnings("unchecked")
    private Map<String, Integer> doSessionStuff(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse) {
        HttpSession session = httpServletRequest.getSession();
        Map<String, Integer> cartMap = null;
        if (session.getAttribute("cart") == null) {
            cartMap = new HashMap<String, Integer>();
            session.setAttribute("cart", cartMap);
        }
        cartMap = (Map<String, Integer>) session.getAttribute("cart");
        return cartMap;
    }
}

And the "Mock" Db:

package com.tugay;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;

/**
 * User: koraytugay
 * Date: 20/07/14
 * Time: 17:58
 */
public class Database {

    private static HashMap<Integer,String> products = new HashMap<Integer, String>();

    static {

        products.put(1,"Table");
        products.put(2,"Sofa");
        products.put(3,"Chair");
        products.put(4,"Window");

    }

    public static List<String> getAllProducts() {
        return new ArrayList<String>(products.values());
    }

}

This is what my page looks like:

If anyone wants to review this, they are welcome.

Interfaces vs implementations

Always use interfaces in declarations, not implementations. Instead of:

 private static HashMap<Integer,String> products = new HashMap<Integer, String>();

Do like this:

private static Map<Integer,String> products = new HashMap<Integer, String>();

doSessionStuff

The httpServletResponse parameter is unused, so drop it.

No need to initialize cartMap to null, as you will assign to it anyway. Actually you don't need that local variable at all, you could simplify the method without it:

@SuppressWarnings("unchecked")
private Map<String, Integer> doSessionStuff(HttpServletRequest httpServletRequest) {
    HttpSession session = httpServletRequest.getSession();
    if (session.getAttribute("cart") == null) {
        session.setAttribute("cart", new HashMap<String, Integer>());
    }
    return (Map<String, Integer>) session.getAttribute("cart");
}

doGet

You only use the cart variable if addProductbyName != null, so it would be better to move it inside the if block.

It's a bit awkward to do cart.put(addProductbyName, 0); and then again cart.put(addProductbyName, cart.get(addProductbyName) + 1);. And you are doing cart.get(addProductbyName) multiple times.

The @SuppressWarnings("unchecked") is also unnecessary for this method, as there are no unchecked casts.

The method would be cleaner like this:

@Override
protected void doGet(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse)
        throws ServletException, IOException {
    String addProductbyName = httpServletRequest.getParameter("addProductbyName");
    if (addProductbyName != null) {
        Map<String, Integer> cart = doSessionStuff(httpServletRequest);
        Integer count = cart.get(addProductbyName);
        if (count == null) {
            cart.put(addProductbyName, 1);
        } else {
            cart.put(addProductbyName, count + 1);
        }
    }

    httpServletResponse.sendRedirect(getServletContext().getContextPath()+"/shopping.jsp");
}


Initializing a std::array from a recursive template computation I wrote this code for an answer over on SO.  I suspect it's possible to clean it up a bit, particularly the base-case specialization for FactorialArray, which is repetitive and contains odd bits (for instance: why double curly braces? because clang complains about un-braced subobject initializers otherwise...)

Any and all suggestions are appreciated.  Note that Factorial is separate from FactorialArray for pedagogical clarity; I realize this makes the compile-time calculation O(N2) unless the compiler is clever enough to memoize it.

#include <iostream>
#include <cinttypes>
#include <array>

using std::uint64_t;

// Helper template that computes the factorial of one integer
template<uint64_t I> struct Factorial
{ static constexpr uint64_t value = I * Factorial<I-1>::value; };

template<> struct Factorial<0> { static constexpr uint64_t value = 1; };

// FactorialArray recursively assembles the desired array as a variadic
// template argument pack from a series of invocations of Factorial
template<uint64_t I, uint64_t... Values> struct FactorialArray
  : FactorialArray<I-1, Factorial<I>::value, Values...>
{};

// and in the base case, initializes a std::array with that pack
template<uint64_t... Values> struct FactorialArray<uint64_t(-1), Values...>
  : std::array<uint64_t, sizeof...(Values)>
{
  constexpr FactorialArray()
    : std::array<uint64_t, sizeof...(Values)> ({{Values...}})
  {}
};

int main()
{
  static FactorialArray<10> f;
  for (std::size_t i = 0; i < f.size(); i++)
    std::cout << i << "! = " << f[i] << '\n';
  return 0;
}

I think your code is fine for the most part. I don't like the inheritance from std::array, seeing as you can do the same thing with a static array data member, which would make the code easier to maintain (less coupling).

If you're well-and-truly in C++14 land, I think the code is clearer if you use constexpr functions rather than functions-disguised-as-structs:

#include <iostream>
#include <utility>
#include <array>

constexpr size_t factorial(size_t t) {
    return t == 0 ? 1 : t * factorial(t - 1);
}

template<size_t... i>
constexpr auto factorial_array(std::index_sequence<i...>) {
    return std::array<size_t, sizeof...(i)>{{factorial(i)...}};
}

template<size_t size>
constexpr auto factorial_array() {
    return factorial_array(std::make_index_sequence<size>{});
}

int main() {
  constexpr auto f = factorial_array<15>();
  for (size_t i = 0; i < f.size(); i++) {
    std::cout << i << "! = " << f[i] << '\n';
  }
  return 0;
}