Script for a multi-user webapp database connection I have the following working script which I use for my database connection for a multi-user webapp. I use a different database for the login, this script is called only after the user is successfully logged in. The login uses PDO.

I would like to know what I can do to

make this more secure
harder to fail

include('.dbinc.cred.inc.php');

try {
$dbh = new PDO('mysql:host=' . HOST_ONE . ';dbname=' . DB_ONE . '', USER_ONE, PASS_ONE);

$statement = $dbh->prepare("SELECT user_db, user_db_pass, user_db_user, user_name FROM databases WHERE user_name = :name");
$statement->execute(array('name' => $_SESSION['user_name']));

foreach ($statement as $row) {
   $user_db = $row['user_db'];
   $user_db_user = $row['user_db_user'];
   $user_db_pass = $row['user_db_pass'];
}

}
  catch (PDOException $e) {
print "Error!: " . $e->getMessage() . "<br/>";
die();
}
$dbh = null;

$sql = new mysqli (HOST_ONE,$user_db_user, $user_db_pass,$user_db);
$sql -> set_charset ( 'utf8' );
if ($sql->connect_errno)
{echo "<script language=JavaScript>location.href='#ajax/account_setup.php';</script> ";}

Is it better to do this in 2 files, one for the user check, then define HOST_TWO and so on and include the first file in the second?

Why am I using PDO and then MySQLi? I do this (weird) thing because the whole app uses mysqli_ at the moment and I'm rewriting everything to PDO atm. But actually I need the MySQLi connection, though.

Lets tackle your code point by point:

Security:

In terms of problems with security, I only see 1 problem:

There isn't any kind of encryption on your code.
From what I can see, you connect to a database that has all the password stored in plain text.
Any attacker that puts his hand on your file .dbinc.cred.inc.php will be very happy!

You can use the mcrypt list of function.
For documentation, you can check here: http://php.net/manual/en/ref.mcrypt.php

Edit:

As @EliasVanOotegem pointed out on his comment, the collation isn't being set.
Appending ';charset=latin1' to the string should do it.

If you are using a verson prior to 5.3.6, this will be ignored.
Instead, you must run $dbh->exec("set names latin1");
Source: https://stackoverflow.com/a/4361485
Luckly, PHP has a function to solve this kind of issue: version_compare().
The version can be checked like this: version_compare(PHP_VERSION, '5.3.6', '<')
Documentation: http://php.net/manual/en/function.version-compare.php

Now, you may ask: why not utf8?
The reason is that the default collation for most servers is in fact latin1_swedish_ci.
This means that a password like aç$%dKeä would be returned as scrambled data.
When you try to connect to the other database, using mysqli_*, the connection would be always rejected because the password would be different.

The code itself:

On the code itself, I see some problems too.

Lets make some sections for it.

The try{} block:

Inside the try{} block, you have the following code:

foreach ($statement as $row) {
   $user_db = $row['user_db'];
   $user_db_user = $row['user_db_user'];
   $user_db_pass = $row['user_db_pass'];
}

This code isn't wrong, but this is useful when you fetch multiple rows from the database.
Another thing is that you only use this code to assign the variables to the values from the database.
You should use the list() constructor, using also the $statement->fetch() method.
The above code can be rewritten into this:

list($user_db,$user_db_user,$user_db_pass)=$statement->fetch(PDO::FETCH_NUM);

Documentation:
list() constructor: http://php.net/manual/en/function.list.php
method $statement->fetch(): http://php.net/manual/en/pdostatement.fetch.php

The catch{} block:

Inside the catch block, you are using a print and on the next line you have an empty die().
Removing the print and placing the text on the die() will have the same effect.
This code:

  catch (PDOException $e) {
print "Error!: " . $e->getMessage() . "
";
die();
}

Becomes:

catch (PDOException $e){
    die('Error!: ' . $e->getMessage() . '
');
}

This is not a real issue, but you are concatenating an empty string on the PDO initialization.
Here is the code:

$dbh = new PDO('mysql:host=' . HOST_ONE . ';dbname=' . DB_ONE . '', USER_ONE, PASS_ONE);

Right after DB_ONE, you can remove that concatenation.

Outside the try ... catch{}`:

Outside the try ... catch{} block, you have this line:

$dbh = null;

This line, at the most, should be moved inside the try{} block, which is where you initialize the PDO object.

Other observations:

Your indentation is a little broken.
Indentation allows us to understand better where a block starts and finishes, and what is inside it.

This is just an observation, just for the paranoid people.

You fetch the data from the database but you don't free the result set.

You should run $statement->closeCursor(); after you fetch your data.
You can check the documentation here: http://php.net/manual/en/pdostatement.closecursor.php

After you don't need your variables, you should use unset just to free the memory, deleting the variables $statement and $dbh.
This way, I believe, you can skip the line $dbh = null; which is the example given in the php documentation, on the link: http://php.net/manual/en/pdo.connections.php (go to example #3).

Another thing I noticed is the absence of the php tags.

Almost at the end of the code, you have this echo:

echo " ";

The language attribute is deprecated and can be safely dropped. (Source: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-language )
This was useful in the times when VBScript was used for Internet Explorer, to diferentiate both languages.
Nowadays, you don't need this.
Another thing is that the value of the attribute isn't between quotes.
If you are paranoid about this, you can use the type attribute.
You can set the value text/javascript, being the final code like this:

echo '';

Documentation about the type attribute:  https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-type

The final code (security apart, reflecting the edit):

Applying all the changes, the final code will look like this:

<?php

    include('.dbinc.cred.inc.php');

    try {

        $dbh = new PDO('mysql:host=' . HOST_ONE . ';dbname=' . DB_ONE . ';charset=latin1', USER_ONE, PASS_ONE);

        if(version_compare(PHP_VERSION, '5.3.6', '<'))
        {
            $dbh->exec('set names latin1');
        }

        $statement = $dbh->prepare('SELECT user_db, user_db_pass, user_db_user, user_name FROM databases WHERE user_name = :name');
        $statement->execute(array('name' => $_SESSION['user_name']));

        list($user_db, $user_db_user, $user_db_pass) = $statement->fetch(PDO::FETCH_NUM);

        $statement->closeCursor();

        unset($statement, $dbh);

    }
    catch (PDOException $e){
        die('Error!: ' . $e->getMessage() . '
');
    }

    $sql = new mysqli(HOST_ONE,$user_db_user, $user_db_pass,$user_db);
    $sql -> set_charset('utf8');
    if ($sql->connect_errno)
    {
        echo '';
    }

?>

As a micro-optimization, I swapped the quotes.
In terms of real-world usage, using "" would make a negligible difference.
Use the quotes that you prefer, I simply prefer to save some CPU cycles.

Thanks to @EliasVanOotegem for spotting the safety issue regarding the lack of a specification on the used collation to connect to the database.
This is a serious issue because it changes the way that the parameters are escaped and avoids the use of special characters to 'nullify' the escapes, allowing other commands to run.
You can see how this attack is performed in here: http://hakipedia.com/index.php/SQL_Injection#Filter_Bypassing


Creating an iPad-style video frame with CSS and without images In an effort to reduce the images being used on the page, I've manipulated box-shading, etc. to make a "vector" looking monitor/ipad look.

This is great and all, gets the job done, however, seems there is still a large load draw on the box-shading rendering. Is there a better method or tweak to this method that would be a better approach?

Original Demo |
Updated Demo

HTML

CSS

.vid{
    max-width:800px;
    width:80%;
    margin:0 auto}

  .vidlink{
    margin-top:50px;
    background:url('img/vidbg.svg') no-repeat;
    background-position:50%;
    background-size:cover;
    border:1px solid rgba(0,0,0,.6);
    box-shadow:0 0 0 25px #fff,
               0 0 5px 27px #777;
    border-radius:5px 5px 0 0;
    display:block;
    margin-bottom:-5px;
    padding:25% 0}

    .vidlink:before{
      content:" ";
      position:relative;
      left:50%;
      top:50%;
      margin:-40px 0 0 -40px;
      padding:28px 40px;
      background:url('img/icon/play.svg') no-repeat;
      background-size:75px;
      background-position:50%}

      .vidlink:hover:before{
        background:url('img/icon/play-hover.svg') no-repeat;
        background-size:75px;
        background-position:50%}

Your code is perfectly valid, according to the HTML and CSS validators at W3C:

  HTML Validator
  CSS Validator

However, there are a couple things you could improve.

HTML:

I assume you already know to always specify the doctype and character encoding in your HTML files.  The character encoding is not required by the validator, but it is good to always include it:

<!DOCTYPE HTML>
<head>
    <meta charset="utf-8">
    <title>Page Title</title>
</head>

Your CSS is difficult to read because of your indentation and your use of braces.  All of your indentation should match, not have some blocks at the nested indentation level, and some at their proper level.  Also, most CSS files use braces like this:

.vid {
    max-width:800px;
    width:80%;
    margin:0 auto;
}

Not like this:

.vid{
    max-width:800px;
    width:80%;
    margin:0 auto}

Edit

After clarification in the comments, this is a valid CSS style, but I still prefer the above style.

End Edit

The CSS validator does not require that you have a semi-colon ; after the very last item, but you should to keep things all the same style.

Overall, your code is good and the UI is beautiful.  These are just a few things you could improve on and a few tips that just because they are so important, they deserve to be here.


Optimizing Project Euler Solution #12 Just to reiterate my prev. post, as I think it helps structure my weaknesses.

Coding methodology:  Is the algorithm I used feasible or mostly correct? If not, how do I correct it and why won't it work or scale? I realize in some cases there are always going to be optimal solutions that will be over my head mathematically, but generally you can yield a solution that is within a reasonable range of optimal that is passable without knowing a ton of number theory in my experience. Where possible, I hope to learn algorithms along the way (such as the Sieve of Eratosthenes for prime ranges).
Coding execution: Given some pseudocode that we determine algorithmically will work, is the code written executed without redundancies and minimizing runtime?
Being more pythonic: Is something I'm not doing pythonic or can it be done simpler with some sort of library I don't know about? I believe this is mostly semantic, though in some cases can have large effects on performance (for instance, using map() instead of a for loop to apply int()).

I submitted the following code as a solution to Project Euler's #12. It's extremely slow, running in mean 7.13 seconds over 100 trials (I think; just using other submitted solutions as a benchmark. It seems as though solutions <1 seconds are generally decent.)

def factors(n):
    return set(reduce(list.__add__,
                ([i, n//i] for i in range(1, int(n**0.5) + 1) if n % i == 0)))

def triangle_number(n):
    for i in range(n):
        n += i
    return n

n = 1
while True:
    if len(factors(triangle_number(n))) >= 500:
        print n, triangle_number(n)
        break
    else:
        n +=1

The code for the factors function was borrowed from here.

This is an addition to Caridorc's answer.

The usual reason for using itertools is that you don't want to construct lists when you don't need to. For example, in Python 2, you'd write for i in xrange(100) instead of for i in range(100) so that you don't have to construct a whole list of 100 numbers when you don't need it. If this doesn't make sense to you, familiarize yourself with the difference between generator expressions (genexprs) and list types.

In your case, however, there's a much more serious problem. You've got a Shlemiel the painter's algorithm!

When you write reduce(list.__add__, list_of_lists), you're essentially writing list_1 + ... + list_n. Python will evaluate this as follows:

Take list_1 and list_2. Create a new list object of the proper size, and copy the elements from lists 1 and 2 into this new list. Call it intermediate_1.
Take intermediate_1 and list_3. Create a new list of the proper size, and copy the elements from these two lists into the new list. Call it intermediate_2.
…
Take intermediate_(n-1) and list_n. Create a new list of the proper size, and copy the elements from these two lists into the new list. This is the final result.

Do you see the problem? This code is quadratic in both runtime and memory usage.

Here's some hard data. Consider the following function:

def fn(n):
    lists = [[0] * 10000 for i in xrange(n)]
    combined = reduce(list.__add__, lists)

I ran this for values of n of 1, 10, 100, 200, 250, 300, 400, and 500 (using IPython's %timeit fn(100), which does proper timing), and got the following results:

This definitely looks quadratic, and the coefficient of determination is R2 = 0.99655, which indicates an extremely strong correlation.

When we instead use

def fast(n):
    combined = list(itertools.chain.from_iterable(
        [0] * 10000 for i in xrange(n)))

the time for fast(500) is just 65.8 ms instead of 7.42 s. (This shows that it is indeed the Shlemiel factor and not just large lists that's causing the slowdown.)

So, tl;dr, yes, use iterators—but make sure you know why!

So when use reduce?

Just a review of what reduce does for binary operators:

reduce(int.__add__, [x1, x2, x3], x0) = (((x0 + x1) + x2) + x3)

This is extended to general functions as follows:

reduce(f, [x1, x2, x3], x0) = f(f(f(x0, x1), x2), x3)

You should use reduce when you want to write code that looks like either of the above examples. Basically, reduce just saves you a loop.

Conceptually, you use reduce when you want to collapse a bunch of things into one value.

It may help to think of reduce in comparison with its other core functional operations:

Use map when you want to transform a bunch of values to other things (like "reverse all these strings"). You use map in list comprehensions when you write [s[::-1] for s in strings].
Use filter when you want to take a bunch of values and keep just some of them (like "take just the strings that are palindromes"). You use reduce in list comprehensions when you write [s for s in strings if s == s[::-1]].
Use reduce when you want to collapse a bunch of things into one value (like "concatenate all these strings"). You can't use reduce in list comprehensions, because you're not creating a new list (just a value).

Notes:

The problem in your case is that the version with list_1 + ... + list_n is just as bad in terms of efficiency. reduce wasn't your problem; it was what reduce expanded to.
With reduce, you can omit the third argument x0, and reduce will use the first element of the sequence, or raise a TypeError if the sequence is empty.

Before noting some examples, it may be helpful to realize that some operations from __builtins__ are just reduce under the hood. For example:

sum(ints) = reduce(int.__add__, ints, 0);
all(bools) = reduce(lambda x, y: x and y, bools, True), and, similarly;
any(bools) = reduce(lambda x, y: x or y, bools, False);
min(ints) is like reduce(lambda x, y: x if x < y else y, ints) (it's actually a bit smarter), and, similarly;
max(ints) is like reduce(lambda x, y: x if x > y else y, ints).

A few quick examples:

def factorial(n):
    return reduce(int.__mul__, xrange(1, n + 1), 1)

def replay_chess_game(moves):
    return reduce(lambda board, move: board.then(move),
                  moves,
                  INITIAL_BOARD)

def perform_git_rebase(commits):
    return reduce(lambda head, commit: head.cherry_pick(commit),
                  commits,
                  git_globals.refs.HEAD)

I don't use reduce all that much in Python, but you should definitely know when to use it.


My implementation of counting sort I've made the following implementation of counting sort. I don't think that I understand the algorithm fully but this implementation seems to be working correctly.

I've used the Counterdictionary from the collections module. It seems to be the best way to handle the counting part. But I haven't seen anyone using it to implement counting sort.

Is there any way to improve the code?

from random import randrange
from collections import Counter
from copy import deepcopy

def counting_sort(aList, length):
    """
    counting sort does not work for negative numbers
    counting sort assumes that each element is SMALL INTEGER

    running time is O(maxVal - minVal) ---> Linear

    (Useful only when the difference between maxVal and minVal is not large)
    """
    list_copy = deepcopy(aList)  # a copy so that I can return it and check it against the original list applied to sorted function
    c = Counter()

    for num in list_copy:
        c[num] += 1

    index = 0

    # for i in range(len(c)):
        # while 0 < c[i]:
            # list_copy[index] = i
            # c[i] -= 1
            # index += 1
    for key in c:
        while c[key] > 0:
            list_copy[index] = key
            c[key] -= 1
            index += 1

    return list_copy

aList = [randrange(100) for _ in range(1000)]
after = counting_sort(aList, len(aList))
print(after == sorted(aList))

Your code is not reliable because Counter is unordered.  In many cases, it comes up with the right result, but do not depend on it.  I will not give an alternate solution because Code Review is for improvements on existing code, not for writing new code.  Although your code should not be used for sorting, I have a couple comments on what you did.

You aren't using length, so why require it for an argument?

Instead of creating a copy of a list and then changing each index, create an empty list and add things to it.

Your implementation of Counter does not take advantage of it.  You use it very similarly to how you would use a dictionary.  Just use Counter(aList)

You can simplify your for key in c: loop by using itertools.repeat.  Note: don't use it for mutable objects.

Your naming is inconsistent.  PEP 8 recommends snake_case, but you use both snake_case and pascalCase.  You aren't required to comply with PEP 8's recommendation, but you should at least be consistent.  You should also use more descriptive names than c, for example.

The new code:

from collections import Counter
from itertools import repeat

def counting_sort(number_list):
    """
    counting sort does not work for negative numbers
    counting sort assumes that each element is SMALL INTEGER

    running time is O(maxVal - minVal) ---> Linear

    (Useful only when the difference between maxVal and minVal is not large)
    """

    sorted_list = []
    for value, amount in Counter(number_list).items():
        sorted_list.extend(repeat(value, amount))

    return sorted_list


Trying (and failing) to implement the best conventions on OOP Swift I'm making a MacOS app that will do some analysis on the user's calendar data. For now it only supports Apple's native calendar (EventKit), but I will later add support to Google, outlook, etc.

Models/Events/Base.swift:

import Foundation

struct EventData {
    var title: String;
    var startDate: Date;
    var endDate: Date;
    var organizerName: String;
    var notes: String;
    var location: String;
}

struct CalendarData {
    var title: String;
    var isSubscribed: Bool;
}

class Events {
    init() {
        checkForAuthorization();
    }

    func checkForAuthorization() {}
    func requestAccess() {}
    func getCalendars() -> [CalendarData] {
        return [];
    }
    func getEvents(calendarName: String, from: Date, to: Date) -> [EventData] {
        return [];
    }
}

Models/Events/iCal.swift:

import Foundation
import EventKit

class iCal: Events {

    let eventStore = EKEventStore();
    var calendars: [EKCalendar]?;

    override func checkForAuthorization() {
        let status = EKEventStore.authorizationStatus(for: EKEntityType.event);

        switch (status) {
            case EKAuthorizationStatus.notDetermined:
                self.requestAccess();
                break;
            case EKAuthorizationStatus.authorized:
                break;
            case EKAuthorizationStatus.restricted, EKAuthorizationStatus.denied:
                break;
         }
    }

    override func requestAccess() {
        eventStore.requestAccess(to: EKEntityType.event, completion:{ (accessGranted: Bool, error: Error?) in
            if accessGranted == true {
                print("Granted")
            } else {
                print("Denied")
            }
        });
    }

    override func getCalendars() -> [CalendarData] {
        let _cals = self.eventStore.calendars(for: .event);
        var cals: [CalendarData] = [];

        for cal: EKCalendar in _cals {
            cals.append(CalendarData(title: cal.title, isSubscribed: cal.isSubscribed));
        }
        return cals;
    }
    override func getEvents(calendarName: String, from: Date, to: Date) -> [EventData] {
        let cals = self.eventStore.calendars(for: .event);
        var events: [EventData] = [];

        if let calIndex: Int = cals.firstIndex(where: { $0.title == calendarName }) {
            let selectedCalendar: EKCalendar = cals[calIndex];

            let predicate = eventStore.predicateForEvents(withStart: from, end: to, calendars: [selectedCalendar])
            let _events = eventStore.events(matching: predicate) as [EKEvent];

            for ev: EKEvent in _events {
                events.append(
                    EventData(
                        title: ev.title,
                        startDate: ev.startDate,
                        endDate: ev.endDate,
                        organizerName: ev.organizer?.name ?? "",
                        notes: ev.notes ?? "",
                        location: ev.location ?? ""
                    )
                );
            }

        }

        return events;
    }
}

And I use this class like this:

let calendar = iCal();
for cal in calendar.getCalendars() {
   print("****** \(cal.title) *******\n");
   print(calendar.getEvents(calendarName: cal.title, from: Calendar.current.date(byAdding: .day, value: -2, to: Date())!, to: Calendar.current.date(byAdding: .day, value: 1, to: Date())!))
}

This works, but it feels very wrong...
I know the semi-colons are not needed, but I always used it in other languages and the linting will delete them anyway after.

Questions:

How can I improve this?
Would it be better to instantiate the class
Events, by passing the calendar type as an argument (eg. iCal,
outlook, etc)
Are those structs ok? As in should they go in another
file?

Thank you

Please consider follow recommendations:

Make habit of extracting each struct/class/enum in separate files which brings more readability and clarity. Code looks less cluttered.

Please try getting used to of not using semicolons :)

You have defined Events as a class with empty implementations of functions. Are you sure you want to do that, I think it could be converted as a protocol instead
To be able to inject different type of calendar APIs, you should define your own protocol and make concrete implementations for each calendar (Adapter Pattern).

You must use dependency injection. For example even things like EKEventStore() should be injected.
final class iCal: Events {

    private let eventStore: EKEventStore

    init(with eventStore: EKEventStore = EKEventStore()) {
        self.eventStore = eventStore
    }
}

Using force unwrap is a bad practice, you should use guard let or if let to unwrap the optional value and only then use that unwrapped value for whatever purpose you want. By using forced unwrapping your app can crashes unexpectedly where it will find a nil value inside any optional variable.

Please make use of Swiftlint to make sure that your code is following standard convention. It will force you to think swift way :)

You should make class final when don't want to allow subclassing it (even if when you are not sure for now). If you do need subclassing later, you can remove final keyword.

All the best!


Merge table stored procedure I wrote a stored procedure that will do a merge of two tables:

CREATE PROCEDURE [dbo].[merge_tables]
-- Add the parameters for the stored procedure here
@SourceTable varchar(50),
@DestinationTable varchar(50),
@PrimaryKey varchar(50)
AS
BEGIN
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;
DECLARE @update_query varchar(max) =
    (select Concat('SET ', string_agg(cast(@DestinationTable + '.[' + name + '] = '+ @SourceTable +'.[' + name +']' as varchar(max)),','))
    from sys.columns
    WHERE object_id = OBJECT_ID(@DestinationTable) and name != @PrimaryKey and generated_always_type = 0 and system_type_id != 189 and is_identity = 0);

DECLARE @insert_query varchar(max) = (select Concat('([', string_agg(cast(name as varchar(max)),'],['), '])', ' VALUES ([', string_agg(cast(name as varchar(max)),'],['), '])')
    from sys.columns
    WHERE object_id = OBJECT_ID(@DestinationTable) and generated_always_type = 0  and is_identity = 0 and system_type_id != 189);

DECLARE @merge_query varchar(max) = 'MERGE ' + @DestinationTable +
' USING ' + @SourceTable +
' ON (' + @SourceTable + '.' + @PrimaryKey + ' = ' + @DestinationTable + '.' + @PrimaryKey + ')' +
' WHEN MATCHED THEN UPDATE ' + @update_query +
' WHEN NOT MATCHED BY TARGET THEN INSERT ' + @insert_query +
' WHEN NOT MATCHED BY SOURCE THEN DELETE;';
select @merge_query;
EXEC(@merge_query)
END
GO

It appears to work. Is there any way in which this code can be improved? Also, I am not sure how to make this work with compound keys, but that might be off topic for this site.

Is there any way in which this code can be improved?

The procedure parameters should reflect the underlying data types. Each component of an identifier is sysname, a synonym for nvarchar(128) NOT NULL. Using varchar(50) artificially limits the length, and characters that might be used. Likewise, Unicode should be used for the dynamic SQL itself.
The user-supplied table identifier strings should be split using PARSENAME into schema and object portions, then quoted using QUOTENAME. This is safer and more correct than manually surrounding the whole identifier with square brackets.
QUOTENAME returns nvarchar(258), so a safe type for the input is twice this, plus one for the . separator, giving nvarchar(517).
Add a @Debug bit parameter, or similar, so the user can view the generated dynamic SQL (and the associated execution plan) before deciding to execute it.
The procedure should have a basic error handling framework, for example as described by Erland Sommarskog in Error and Transaction Handling in SQL Server.
Add some basic checks for things like object existence, the presence of a primary key, and so on.
STRING_AGG should normally have a WITHIN GROUP clause to provide deterministic ordering.
Compute the distinct column lists once, then reuse this value as necessary.
Separate the code into more steps to make it clearer, and easier to debug. Try to avoid code that scrolls off to the right, and deeply nested function calls.
It is difficult to anticipate all the conditions that could cause the MERGE statement to fail, but consider adding extra checks for e.g. computed columns. Even if this is covered by another condition, being explicit makes the intent clearer.
Avoid hard-coding a numeric type id. Use the TYPE_ID function instead.
Using CONCAT_WS can result in cleaner code than CONCAT when using separators.

Also, I am not sure how to make this work with compound keys...

This can be done by consulting sys.key_constraints and sys.index_columns, see the example code below.
Example
This illustrates how the above points might be incorporated:
CREATE OR ALTER PROCEDURE dbo.MergeTables
    @Source nvarchar(517),
    @Target nvarchar(517),
    @Debug bit = 0
AS
BEGIN
    SET NOCOUNT, XACT_ABORT ON;
    BEGIN TRY
        -- Cannot specify a database or server in the input table names
        IF PARSENAME(@Source, 3) IS NOT NULL
        BEGIN
            RAISERROR(N'Three- or four-part names are not supported for the table identifer %s.', 16, 1, @Source);
        END;

        IF PARSENAME(@Target, 3) IS NOT NULL
        BEGIN
            RAISERROR(N'Three- or four-part names are not supported for the table identifer %s.', 16, 1, @Target);
        END;

        -- Break and quote input e.g. 'My Schema.My Object' - > [My Schema].[My Object]
        SET @Source = CONCAT_WS(N'.',
            QUOTENAME(PARSENAME(@Source, 2)),
            QUOTENAME(PARSENAME(@Source, 1)));

        SET @Target = CONCAT_WS(N'.',
            QUOTENAME(PARSENAME(@Target, 2)),
            QUOTENAME(PARSENAME(@Target, 1)));

        -- Check tables are accessible
        DECLARE @SourceID integer = OBJECT_ID(@Source, N'U');
        DECLARE @TargetID integer = OBJECT_ID(@Target, N'U');

        IF @SourceID IS NULL
        BEGIN
            RAISERROR(N'Table %s not found in the current database.', 16, 1, @Source);
        END;

        IF @TargetID IS NULL
        BEGIN
            RAISERROR(N'Table %s not found in the current database.', 16, 1, @Target);
        END;

        IF CONVERT(integer, OBJECTPROPERTYEX(@SourceID, 'TableHasPrimaryKey')) = 0
        BEGIN
            RAISERROR(N'Table %s does not have a primary key.', 16, 1, @Source);
        END;

        IF CONVERT(integer, OBJECTPROPERTYEX(@TargetID, 'TableHasPrimaryKey')) = 0
        BEGIN
            RAISERROR(N'Table %s does not have a primary key.', 16, 1, @Target);
        END;

        /* TODO: Check source and target schemas are compatible*/

        DECLARE
            -- Find the primary key columns for the MERGE statement's ON clause
            @PrimaryKeyColumnList nvarchar(max) =
            (
                SELECT
                    STRING_AGG(
                        CONVERT(nvarchar(max),
                            CONCAT(N'S.', QUOTENAME(C.[name]), N'=T.', QUOTENAME(C.[name]))),
                        N' AND ')
                        WITHIN GROUP (ORDER BY IC.key_ordinal)
                FROM sys.key_constraints AS KC
                JOIN sys.index_columns AS IC
                    ON IC.[object_id] = KC.parent_object_id
                    AND IC.index_id = KC.unique_index_id
                JOIN sys.columns AS C
                    ON C.[object_id] = IC.[object_id]
                    AND C.column_id = IC.column_id
                WHERE
                    KC.parent_object_id = @SourceID
                    AND KC.[type_desc] = N'PRIMARY_KEY_CONSTRAINT'
            ),
            @InsertClause nvarchar(max) = N'INSERT (column_list) VALUES (column_list)',
            -- Find the insert columns list
            @InsertColumnList nvarchar(max) =
            (
                SELECT
                    STRING_AGG(
                        CONVERT(nvarchar(max),
                            QUOTENAME(C.[name])),
                        N',')
                        WITHIN GROUP (ORDER BY C.column_id)
                FROM sys.columns AS C
                WHERE
                    C.[object_id] = @SourceID
                    AND C.is_computed = 0
                    AND C.is_identity = 0
                    AND C.is_column_set = 0
                    AND C.is_hidden = 0
                    AND C.generated_always_type = 0
                    AND C.system_type_id != TYPE_ID(N'timestamp')
            ),
            -- Generate the UPDATE SET clause
            @UpdateClause nvarchar(max) = N'UPDATE SET ' +
            (
                SELECT
                    STRING_AGG(
                        CONVERT(nvarchar(max),
                            CONCAT(N'T.', QUOTENAME(C.[name]), N'=S.', QUOTENAME(C.[name]))),
                        N',')
                        WITHIN GROUP (ORDER BY C.column_id)
                FROM sys.columns AS C
                WHERE
                    C.[object_id] = @SourceID
                    AND C.is_computed = 0
                    AND C.is_identity = 0
                    AND C.is_column_set = 0
                    AND C.is_hidden = 0
                    AND C.generated_always_type = 0
                    AND C.system_type_id != TYPE_ID(N'timestamp')
                    AND NOT EXISTS
                    (
                        -- Exclude column if it is part of the primary key
                        SELECT 1
                        FROM sys.key_constraints AS KC
                        JOIN sys.index_columns AS IC
                            ON IC.[object_id] = KC.parent_object_id
                            AND IC.index_id = KC.unique_index_id
                        WHERE
                            KC.parent_object_id = C.[object_id]
                            AND KC.[type_desc] = N'PRIMARY_KEY_CONSTRAINT'
                            AND IC.column_id = C.column_id
                    )
            ),
            -- Basic form of the merge statement
            @MergeStatement nvarchar(max) =
                CONCAT_WS(
                    N' ',
                    N'MERGE @Target AS T',
                    N'USING @Source AS S',
                    N'ON primary_key_match_list',
                    N'WHEN MATCHED THEN merge_matched',
                    N'WHEN NOT MATCHED BY TARGET THEN merge_not_matched',
                    N'WHEN NOT MATCHED BY SOURCE THEN DELETE;');

        -- Substitute the insert column list into the INSERT clause
        SET @InsertClause = REPLACE(@InsertClause, N'column_list', @InsertColumnList);

        -- Substitute other values into the MERGE template
        SET @MergeStatement = REPLACE(@MergeStatement, N'@Target', @Target);
        SET @MergeStatement = REPLACE(@MergeStatement, N'@Source', @Source);
        SET @MergeStatement = REPLACE(@MergeStatement, N'primary_key_match_list', @PrimaryKeyColumnList);
        SET @MergeStatement = REPLACE(@MergeStatement, N'merge_matched', @UpdateClause);
        SET @MergeStatement = REPLACE(@MergeStatement, N'merge_not_matched', @InsertClause);

        BEGIN TRANSACTION;

            IF @Debug = 0
            BEGIN
                -- Peform the MERGE
                EXECUTE (@MergeStatement);
            END;
            ELSE
            BEGIN
                PRINT @MergeStatement;
            END;

        COMMIT TRANSACTION;

        RETURN 0;
    END TRY
    BEGIN CATCH
        IF @@TRANCOUNT > 0 ROLLBACK TRANSACTION;
        THROW;
        RETURN -1;
    END CATCH;
END;


Best way to handle repetitive error code or return value I have a wrapper which interacts with a third party component. In Each function i am performing the operation if it the component is initialized.

I have other function to check the errorCode and component initialization check.

Is this right way to do this? I am not happy with the readability of this code. Help me improving this code.

Thanks in advance.

Void Operation1 (parameter someInput)
{
    int errorCode= 0;
    if (Initialized)
    {
        errorCode = ThirdPartyComponent.Operation1(someInput);
    }

     CheckErrorCodeAndShowError ("Operation1 with someInput failed", returnValue);
}

Void Operation2 (parameter someInput)
{
    int errorCode= 0;
    if (Initialized)
    {
        errorCode = ThirdPartyComponent.Operation2(someInput);
    }

    CheckErrorCodeAndShowError ("Operation2 with someInput failed", returnValue);
}

Void CheckErrorCodeAndShowError (String errorMessage, int errorCode)
{
   if (!Initialized)
   {
       //Show not initialized error
   }

   if (errorCode == -1)
   {
      //Show Operation failed error (errorMessage)
   }
   else if (errorCode == 0)
   {
      //Show operation success message etc
   }
   //other error code check
}

Okay, well here is what I would probably do in this situation. If the error codes you are getting are consistent this will make this type of solution even better (consistent as in "FileNotFound" error code is always -42 or something like that).

I would add a private method that throws an "Uninitialized" exception if the object has not been initialized. There is no need to be checking that twice for each function call.

private void ValidateInitialized()
{
    throw new LibraryUninitializedException(); // Just create this exception class
}

Then I would create a helper method that takes a Func<int> that you can call from each of the wrapper methods. The helper method will handle the error code stuff for you. It would look something like this (note: I have not compiled this, so I may have some minor syntax errors).

private void DoThirdPartyOperation(Func<int> operation, string message = "")
{
     ValidateInitialized();
     switch(operation())
     {
         case 0:
             // No error - do nothing
             break;
         case someErrorCode:
             throw new SomeErrorException(message);
             break;
         case someOtherErrorCode:
             throw new SomeOtherErrorException(message);
             break;

         default:
             // Choose either to throw some sort of general exception and include
             // the error code in it OR throw an UnrecognizedErrorException
     }
}

public void Operation1(someParameter)
{
    DoThirdPartyOperation(() => { ThirdParty.Operation1(someParameter); });
}

public void Operation2(someParameter)
{
    DoThirdPartyOperation(() => { ThirdParty.Operation2(someParameter); },
        "Failed performing Operation2");
}


Reduce boilerplate with RepositoryPattern I'm refactoring a WPF application that uses Repository pattern to access the web services, I've got a tons of repository wrote as

public class DiagnosticsRepository : DefaultRepositoryBase, IDiagnosticsRepository
{
    public Task<IList<MasterDiagnosticType>> GetMasterDiagnosticTypes(string username)
    {
        var client = GetServiceStackClient();

        var request = new MasterDiagnosticTypesRequest
        {
            UserName = username
        };

        return client.PostAsync(request);
    }

    public Task<IList<MasterDiagnostic>> GetMasterDiagnosticsFromType(int type)
    {
        var client = GetServiceStackClient();

        var request = new MasterDiagnosticRequest()
        { MasterType = type };

        return client.PostAsync(request);
    }

    public Task<DiagnosticoDataResponse> GetMasterDiagnostic(string storedProcedure)
    {
        var client = GetServiceStackClient();

        var request = new DiagnosticoDataRequest()

        {
            StoredProcedureName = storedProcedure,
            Data = DateTime.Today
        };

        return client.PostAsync(request);
    }
}

How can I review it so that I can avoid the repetition? the only part that changes is the request

If you must keep the code the same way, I can think of only reducing the repeated

var client = GetServiceStackClient();
return client.PostAsync(request);

by doing something like this:

public class DiagnosticsRepository : DefaultRepositoryBase, IDiagnosticsRepository
{
    public Task<IList<MasterDiagnostic>> GetMasterDiagnosticsFromType(int type)
    {
        var request = new MasterDiagnosticRequest()
        {
            MasterType = type
        };

        return Refactor<IList<MasterDiagnostic>, MasterDiagnosticRequest>.Get(request);
    }

    public Task<DiagnosticoDataResponse> GetMasterDiagnostic(string storedProcedure)
    {
        var request = new DiagnosticoDataRequest()
        {
            StoredProcedureName = storedProcedure,
            Data = DateTime.Today
        };

        return Refactor<DiagnosticoDataResponse, DiagnosticoDataRequest>.Get(request);
    }

    // get rid of the repetition...
    private class Refactor<TRespone, TRequest>
    {
        public static Task<TRespone> Get(TRequest request)
        {
            var client = GetServiceStackClient();
            return client.PostAsync(request);
        }
    }
}

Frankly, I don't think you should have all these various methods and signatures at all. You could reduce the repository to just the methods that are usually found in a repository (ex: Add, Update, GetById, Delete, Etc...

Which would mean your repository would potentially just look like this instead:

public class DiagnosticsRepository<T> : IDiagnosticsRepository
{
    public static Task<T> Get(object request = default(object))
    {
        var client = GetServiceStackClient();
        return client.PostAsync(request);
    }
}

-The below is unconfirmed for your particular library, but based on my quick read of the documentation, this should work-
With the above minimized repository,  if you like, you could go even further and swap your requests to anoymous types to reduce the amount of classes you are generating -- which is not uncommon especially for JSON requests. For example:

string storedProcedure = "...";
object request = new { StoredProcedureName = storedProcedure, Data = DateTime.Today };
var response = DiagnosticsRepository<DiagnosticoDataResponse>.Get(request);


Simple Hangman in Python I decided to code a simple Hangman in Python today and wanted some feedback on the code since I only started programming three weeks ago.

I intentionally didn't add a lot of whitespace, it just doesn't make it any more readable for me - but feel free to give me reasons to start doing so.

Any tips and suggestions are appreciated.

import os

words = ["foobar", "gnarf", "fnord"]

current_word = 0
maximum_words = len(words)
maximum_guesses = 10
solved_words = 0

for word in words:
    current_word += 1
    current_guess = 0
    dashes = []
    for letter in word:
        dashes.append("_")
    while current_guess < maximum_guesses:
        current_guess += 1
        os.system("clear")
        print("Word {}/{}".format(current_word, maximum_words))
        print("Solved {}/{}".format(solved_words, maximum_words))
        print("Guess {}/{}".format(current_guess, maximum_guesses))
        print("\n{}\n".format(" ".join(dashes)))
        guess = input("Guess: ")
        guess = guess.lower()
        if not len(guess) == 1 or not guess.isalpha():
            current_guess -= 1
        for index, letter in enumerate(word):
            if letter == guess:
                dashes[index] = letter
        if not "_" in dashes:
            solved_words += 1
            break

The output looks something like this:

Word 1/3
Solved 0/3
Guess 5/10

f o o b a _

Guess: r

Any suggestings for a better output?

I see a couple issues, though nothing too major.

Improper form for 'if not in'

While this works, it's not the proper form for this:

if not "_" in dashes:

The better form for this is:

if "_" not in dashes:

Tracking if a value is not equal to something

This works:

if not len(guess) == 1 or not guess.isalpha():

But, we can remove one 'not' though,  and use an inequality operator instead:

if len(guess) != 1 or not guess.isalpha():

Generation of dashes

You can shorten the creation of 'dashes' to this when you create it, and then get rid of the for loop that generated it previously:

dashes = ['_'] * len(word)

No tracking of guesses already made

Your code does not track guesses already made.  Therefore, people can 'waste' guesses.

Let's do two things:  first let's move around some things.  Secondly, let's add some code to capture this case and not increment the number of guesses made when this happens.

First, let's change where we increment the 'current guess' value, and define a different 'start' value:

for word in words:
    current_word += 1
    current_guess = 1
    dashes = []
    ....
    while current_guess < maximum_guesses:
        os.system("clear")
        ....
        guess = guess.lower()
        if not len(guess) == 1 or not guess.isalpha():
            current_guess += 1
            ....

Basically, we moved the current_guess incrementer to lower in the code.  And initialize the guess counter to 1 to begin with.

Now let's do some "already guessed" code:

for word in words:
    ....
    guesses = set()
    for letter in word:
        ...
    guess = guess.lower()
    if guess not in guesses:
        guesses.add(guess)
        current_guess += 1
        if not len(guess) == 1 or not guess.isalpha():
            current_guess -= 1
        for index, letter in enumerate(word):
            if letter == guess:
                dashes[index] = letter
        if "_" not in dashes:
            solved_words += 1
            break

This allows 'repeated' guesses of the same letter to not take effect.  That way, if someone enters 'o' as a guess twice, accidentally or intentionally, on the word 'foobar', they aren't penalized for something already guessed.  You can choose to not do this, but it helps protect against "What the heck, I shouldn't be penalized for something I already guessed!" complaints.

I don't change much of your program.  This is the total program with my above suggestions, plus some whitespace for readability.  I don't have any suggestions for improving the output, though, because it seems to be okay as it is, at least to me.

import os

words = ["foobar", "gnarf", "fnord"]

current_word = 0
maximum_words = len(words)
maximum_guesses = 10
solved_words = 0

for word in words:
    current_word += 1
    current_guess = 1
    dashes = ['_'] * len(word)
    guesses = set()

    while current_guess < maximum_guesses:
        os.system("clear")

        print("Word {}/{}".format(current_word, maximum_words))
        print("Solved {}/{}".format(solved_words, maximum_words))
        print("Guess {}/{}".format(current_guess, maximum_guesses))
        print("\n{}\n".format(" ".join(dashes)))

        guess = input("Guess: ")
        guess = guess.lower()

        if guess not in guesses:
            guesses.add(guess)
            current_guess += 1

            if len(guess) != 1 or not guess.isalpha():
                current_guess -= 1

            for index, letter in enumerate(word):
                if letter == guess:
                    dashes[index] = letter

            if "_" not in dashes:
                solved_words += 1
                break


Longest Palindromic Substring better approach I have encountered to this question and applied a brute force algorithm off top of my head and couldn't find a better optimized solution.

I was hoping I could get some insight to implement a better optimized code in terms of complexity. My solution has \$O(n^2)\$ time and space complexity.

class Solution:

    def longestPalindrome(self, s: 'str') -> 'str':
        if s=="":
            return ""
        else:
           ## Find all the substrings
            arr=[]
            for i in range(len(s)):
                char = s[i]
                arr.append(char)
                for j in range(i+1, len(s)-1):
                    char+=s[j]
                    arr.append(char)

            ##Find the palindrome with a longest length
            max_length = 0
            for j in range(len(arr)):
                if self.isPalindrome(arr[j]):
                    if len(arr[j]) > max_length:
                        max_length = len(arr[j])
                        index = arr[j]
            return index

    def isPalindrome(self,s:'str')->'str':
        if s == s[::-1]:
            return True

Unnecessary else after return

In the body of an if, if you end up returning something in that body, an else is not required. A return will exit the body of the function, so the code in the else won't be run. So, you can simply move the code in the else to the outside, after the if. This may have been confusing, but take a look at the updated code to see what I'm talking about.

enumerate()

Instead of using range(len()), use enumerate(). This will allow you to access both the index and the value at that position, and enumerate works for other iterables as well. It's useful here because instead of writing arr[j] over and over, you can simply write value, which is the value at arr[j].

_ for unused loop variables

Also touching on enumerate- When you only want to use one of the values (in your code, you have both cases present), use an _ in that place. This indicates to you and other readers that that variable is to be ignored, and isn't necessary.

Parameter Spacing

When using : in parameters, make sure there is exactly one space after the :.

Operator Spacing

Make sure to space out your operators when assigning and performing arithmetic on variables. It makes it much easier to read, and looks nicer.

return True

Instead of explicitly returning true if the if condition is true, simply return that condition. It will return the boolean value that is returned from that condition.

function_naming

I know you can't change the function names (assuming this is coming from a code challenge website), but remember that method names are to be in snake_case, not camelCase or PascalCase. This is in reference to PEP-8 Method Naming Guidelines. Thanks to @SᴀᴍOnᴇᴌᴀ for pointing this out to me.

Updated Code

class Solution:

    def longestPalindrome(self, s: 'str') -> 'str':
        if s == "":
            return ""
        ## Find all the substrings
        arr = []
        for i, value in enumerate(s):
            char = value
            arr.append(char)
            for j in range(i + 1, len(s) - 1):
                char += s[j]
                arr.append(char)

        ##Find the palindrome with a longest length
        max_length = 0
        for _, value  in enumerate(arr):
            if self.isPalindrome(value):
                if len(value) > max_length:
                    max_length = len(value)
                    index = value
        return index

    def isPalindrome(self, s: 'str')->'str':
        return s == s[::-1]


Kill dead instances of Excel on a server We have several legacy programs (VB6) generating Excel report. They are scheduled tasks running on a server. Yes, violate this.

As a result, we've got lots of "dead" instances of Excel on the server. I intend to write a program to clean these dead Excels. What is the correct way to kill an idle instance? I know it's not a good practice but it seems to be the easiest way for me.

Process[] procs = Process.GetProcessesByName("EXCEL");

List<ProcessInfo> beginProcessInfoList = new List<ProcessInfo>();
foreach (Process p in procs)
{
    if (p.HasExited)
        continue;
    try
    {
        ProcessInfo pi = new ProcessInfo() { PID = p.Id, TotalProcessorTime = p.TotalProcessorTime };
        beginProcessInfoList.Add(pi);
    }
    catch (Exception exp)
    {
        this.Log(exp.Message);
    }
}

Thread.Sleep(TimeSpan.FromSeconds(10));

foreach (Process p in procs)
{
    if (p.HasExited)
        continue;
    try
    {
        p.Refresh();
        if (p.TotalProcessorTime - beginProcessInfoList.Single(pi => pi.PID == p.Id).TotalProcessorTime == TimeSpan.FromSeconds(0))
        {
            p.Kill();
            p.WaitForExit();
            this.Log(string.Format("EXCEL started at {0} killed at {1}.", p.StartTime, DateTime.Now));
        }
    }
    catch (Exception exp)
    {
        this.Log(exp.Message);
    }
}

Your code appears to be correct - however, this can be dangerous because it will kill ALL instances of Excel that are running so you run the risk of killing one of your currently running legacy programs.  There are proper ways to clean up Excel resources - see my question here for a C# version.  Do you have any ability to update the legacy programs?  If so, I would recommend that over this.


Turning on/off setInterval locating - using React.js I work with the GoogleMaps API, using React.js.
Inside the GoogleMap component:

I have a component called UserLocationTimer, which activates
setInterval within useEffect and samples the user's location every 2
seconds, centering the map accordingly. In addition, returns the
UserLocation component that displays the marker of the user's
location on the map.

In case the user drags the map, I catch the event isDragged, and return only the UserLocation component that displays the
marker of the user's location on the map.

It works fine, however, how would you optimize this code?
Is there a more efficient way to catch the dragging event and stopping the timer interval from the parent, instead using conditional rendering?
  <GoogleMap
    . . .
    onDrag={(map) => {
      setIsDragged(true);
    }}
    onDragEnd={(map) => {
      setIsDragged(true);
    }}

  >
    . . .

    {!isDragged && (
      <UserLocationTimer
        userLocation={userLocation}
        setUserLocation={setUserLocation}
        panTo={panTo}
      ></UserLocationTimer>
    )}

    {isDragged && (
      <UserLocation userLocation={userLocation}></UserLocation>
    )}

  </GoogleMap>

export const UserLocationTimer = (props) => {
  const [timer, setTimer] = useState(null);

  useEffect(() => {
    const interval = setInterval(() => {
      setTimer(new Date());
      locateMe();
    }, 2000);
    return () => clearInterval(interval);
  }, []);

  function locateMe() {
    // locating using geolocation...
  }

  return (
    <>
      {timer && <UserLocation userLocation={props.userLocation}></UserLocation>}
    </>
  );
};

export const UserLocation = (props) => {

  if (props.userLocation)
    return (
      <>
        <Marker
          position={{
            lat: props.userLocation.lat,
            lng: props.userLocation.lng,
          }}
          icon={{
            url: userLocationIcon,
            scaledSize: new window.google.maps.Size(30, 30),
            origin: new window.google.maps.Point(0, 0),
            anchor: new window.google.maps.Point(15, 15),
          }}
        />
      </>
    );
  else return null;
};

Typo? I think you meant to set isDragged to false when dragging finishes:
onDragEnd={() => {
  setIsDragged(false);
}}

Note that you don't need to declare parameter you don't use (and probably shouldn't, to make the code easier to read).
Concise props When you have variables that you want to pass as props of the same name, you can make things a bit concise by using shorthand property names and spreading the object into props. For example:
<UserLocationTimer
  userLocation={userLocation}
  setUserLocation={setUserLocation}
  panTo={panTo}
></UserLocationTimer>

can be
<UserLocationTimer {...{userLocation, setUserLocation, panTo}} />

(it has no children, so it can be made self-closing)
Listing fewer identifiers while writing code means less chance of typos, and helps enforce consistent names for things.
Marker position props Rather than listing out the lat and lng properties of userLocation, you can pass the plain object itself to position:
<Marker position={props.userLocation}

Return early instead of long if/else blocks - if one sees if followed by a long block of code, followed by an else, keeping track of what condition the else is corresponding to can be harder to recognize at a glance than it needs to be. Consider returning the short branch first instead. See below.
JSX fragments are only needed when enclosing 2 or more tags - if a fragment only has one child tag, it can be removed completely, like this:
export const UserLocation = ({ userLocation }) => (
  if (!userLocation) {
    return null;
  }
  return (
    <Marker
      // ...

The same thing applies to the JSX returned by UserLocationTimer, but there's a better approach:
Consolidate UserLocation Since UserLocation is being rendered regardless, I think conditionally putting it inside UserLocationTimer isn't the right way to go. I'd remove the timer component completely, and implement its logic in the parent component instead, while unconditionally rendering UserLocation.
const intervalRef = useRef();
useEffect(() => {
  if (isDragged) {
    const intervalId = setInterval(locateMe, 2000);
    // Cleanup will run on dismount and when isDragged goes to false
    return () => clearInterval(intervalId);
  }
}, [isDragged]);

// ...

<GoogleMap
  onDrag={() => {
    setIsDragged(true);
  }}
  onDragEnd={() => {
    setIsDragged(false);
  }}
>
  . . .
  <UserLocation {...{userLocation}} />
</GoogleMap>

Naming The name UserLocation is very similar to userLocation, but one is a component, while the other holds data. Maybe rename them so that they're more distinct - perhaps UserLocationMarker and userLocationCoords?


Event handler for conditions I am trying to detect whether a condition is being returned true or false. The only problem is, I have a bunch of if/else statements. Is it possible to refactor this in less lines? Maybe a for loop or re-writing my markup?

$(document).on('click', '.cert-modal-btn', function(event) {
event.preventDefault();

        var $this  = $(this);
    certNcv        = $this.data("ncv"),
    certDealer     = $this.data("cert"),
    certExpress    = $this.data("express"),
    certOne        = $this.data("dealone"),
    certPromise    = $this.data("iscustomer"),
    certEcomm      = $this.data("isecomm"),
    $tabbedMenu    = $('.tabbed-menu').find('li'),
    $certsDescription = $('.certs-description').find('li');

if(certNcv) {
    console.log("certNcv is true");
    $tabbedMenu.eq(0).removeClass('hidden-item');
    $certsDescription.eq(0).removeClass('hidden-item');
}
else {
    $tabbedMenu.eq(0).addClass('hidden-item');
    $certsDescription.eq(0).addClass('hidden-item');
}
if (certExpress == "X") {
    console.log("certExpress is X");
    $tabbedMenu.eq(1).removeClass('hidden-item');
    $certsDescription.eq(1).removeClass('hidden-item');
}
else {
    $tabbedMenu.eq(1).addClass('hidden-item');
    $certsDescription.eq(1).addClass('hidden-item');
}
if(certEcomm) {
    console.log("certEcomm is true");
    $tabbedMenu.eq(2).removeClass('hidden-item');
    $certsDescription.eq(2).removeClass('hidden-item');
}
else {
    $tabbedMenu.eq(2).addClass('hidden-item');
    $certsDescription.eq(2).addClass('hidden-item');
}
if(certOne) {
    console.log("certOne is true");
    $tabbedMenu.eq(3).removeClass('hidden-item');
    $certsDescription.eq(3).removeClass('hidden-item');
}
else {
    $tabbedMenu.eq(3).addClass('hidden-item');
    $certsDescription.eq(3).addClass('hidden-item');
}
if(certPromise) {
    console.log("certPromise is true");
    $tabbedMenu.eq(4).removeClass('hidden-item');
    $certsDescription.eq(4).removeClass('hidden-item');
}
else {
    $tabbedMenu.eq(4).addClass('hidden-item');
    $certsDescription.eq(4).addClass('hidden-item');
}
if(certDealer) {
    console.log("certDealer is true");
    $tabbedMenu.eq(5).removeClass('hidden-item');
    $certsDescription.eq(5).removeClass('hidden-item');
}
else {
    $tabbedMenu.eq(5).addClass('hidden-item');
    $certsDescription.eq(5).addClass('hidden-item');
}
// console.log(certNcv);
// console.log(certDealer);
 });

If you start with an array:

let certs = ["ncv", "cert", "express", "dealone", "iscustomer", "isecomm"];

then it reduces down to a for loop.

for (int i = 0; i < certs.length; i++) {
    if ($this.data(certs[i])) {
        console.log(certs[i] + " is true");
        $tabbedMenu.eq(i).removeClass('hidden-item');
        $certsDescription.eq(i).removeClass('hidden-item');
    }
    else {
        $tabbedMenu.eq(i).addClass('hidden-item');
        $certsDescription.eq(i).addClass('hidden-item');
    }
}


Project Euler #14 (Longest Collatz Sequence) in Haskell You can check the problem here:
http://projecteuler.net/problem=14

My first approach in Haskell was this:

import Data.Ord
import Data.List

computeCollatzSequenceLength n = let
    compute l n
        | n == 1 = l+1
        | even n = compute (l+1) (n `div` 2)
        | otherwise = compute (l+1) (3*n+1)
    in generate 0 n

main = print $ fst $ maximumBy (comparing snd) $ zip [1..1000000] $ map computeCollatzSequenceLength [1..1000000]

Too slow it was, so I tried to use a sort of memoization by using an unboxed array:

import Data.Ord
import Data.List
import Data.Int
import Data.Array.Unboxed

computeCollatzSequenceLength :: (UArray Int64 Int64) -> Int64 -> (Int64, [Int64])
computeCollatzSequenceLength a n = let
    compute :: Int64 -> [Int64] -> Int64 -> (Int64, [Int64])
    compute l s n'
        | n' == 1 = (l+1, reverse s)
        | v > 0 = (l+v, reverse s)
        | even n' = compute (l+1) (n':s) (n' `div` 2)
        | otherwise = compute (l+1) (n':s) (3*n'+1)
        where v = if n' > (snd $ bounds a) then 0 else a!n'
    in compute 0 [] n

computeMax m = let
    compute :: (Int64,Int64) -> Int64 -> (UArray Int64 Int64) -> (Int64,Int64)
    compute candidate n a
        | n == (m+1) = candidate
        | otherwise = let
            (l, u) = computeCollatzSequenceLength a n
            a' = a//(filter (\(n',_) -> n' <= m) (zip u [l,l-1..]))
            in if (snd candidate) < l then compute (n,l) (n+1) a' else compute candidate (n+1) a'
    in compute (0,0) 1 (array (1,m) [ (i,0) | i <- [1..m] ])

main = print $ fst $ computeMax 1000000

But, this was also very slow...(Both took more than a few minutes on my machine. Actually, the latter seems to take much longer...) I don't know what I've done wrong(I'm still a novice in Haskell.) In theory, the memoized version should be faster. I want to avoid using one of existing memoization packages out there.

What are issues with the current approach and how can I optimize this while still keeping its overall structure/approach?

EDIT: I found an elegant solution here: http://www.haskell.org/haskellwiki/Euler_problems/11_to_20#Problem_14 (The last one there, I mean.) Imperativeness and assignment are so hardwired in me as an inveterate C++ programmer, I cannot think of such a solution on my own at the moment. Still, I'd like to know what were issues with my approach above.

EDIT2: My not-working try based on @Hammar's advice:

import Data.Int(Int64)
import Data.List(maximumBy)
import Data.Ord(comparing)
import Control.Monad
import Control.Monad.ST
import Data.Array.ST
import Data.Array.Unboxed

getCollatzSequenceLengthUpto :: Int64 -> (UArray Int64 Int64)
getCollatzSequenceLengthUpto n = runSTUArray $ do
    seqLengths <- newArray (1,n) 0
    forM_ [1..n] $ \i -> do
        let compute l s n' = do
            v <- if n' > n then 0 else (readArray seqLengths n')
            if n' == 1
            then
                return (l+1, reverse s)
            else
                if v > 0
                then
                    return (l+v, reverse s)
                else
                    if even n'
                    then do
                        out <- compute (l+1) (n':s) (n' `div` 2)
                        return out
                    else do
                        out <- compute (l+1) (n':s) (3*n'+1)
                        return out
        (l, u) <- compute 0 [] i
        forM_ (filter (\(n',_) -> n' <= n) $ zip u [l,l-1..]) $ \(ix, e) -> do
            writeArray seqLengths ix e
    return seqLengths

main = print $ fst $ maximumBy $ (comparing snd) $ assocs $ getCollatzSequenceLengthUpto 1000000

The main reason why this is slow is that every update to your array is making a new copy of it. The version you linked to on the wiki avoids this problem by using a boxed array instead, which allows you to define the array in one go and let lazy evaluation take care of evaluating the elements in an appropriate order and modifying the array behind the scenes.

A boxed array is less memory efficient since it's basically an array of pointers to individually-allocated integers, but for the size of this problem it's not too bad. If you want to use an unboxed array for improved space efficiency, you would probably want to use a mutable one in the ST monad instead. There's an example of how how to do that in an old answer of mine on Stack Overflow.

Regarding your second attempt:

First off, let's get this code to compile.

Since the else branch of this if is returning a statement in the ST monad, so must the then branch. A quick fix is to add return:

v <- if n' > n then return 0 else (readArray seqLengths n')
                    ^^^^^^

And there shouldn't be a $ when passing the comparison function
to maximumBy here:

main = print $ fst $ maximumBy  (comparing snd) $ assocs $ getCollatzSequenceLengthUpto 1000000
                               ^

With those small changes, it compiles and it runs in about 4 seconds on my netbook. Not too bad!

However, it's a bit messy, let's see if we can fix that. First of all, let's reduce that ball of nested if's.

We can get rid of the outermost if by using a separate equation for when n' is 1:

compute l s 1  = return (l+1, reverse s)

The two branches of the innermost if are almost the same. We can move the if inside to the only place they differ. Also do out <- compute ...; return out is the same as just compute ...:

compute l s n' = do
    v <- if n' > n then return 0 else readArray seqLengths n'
    if v > 0
        then return (l+v, reverse s)
        else compute (l+1) (n':s) (if even n' then n' `div` 2 else (3*n'+1))

Here is the full working code after these adjustments. I still think the way
you're using a list to deal with the updates is somewhat complicated, but I think fixing that in a satisfactory way would require more substantial changes to your algorithm, so I've left it the way it is.

import Control.Monad (forM_)
import Control.Monad.ST
import Data.Array.ST
import Data.Array.Unboxed
import Data.Int (Int64)
import Data.List (maximumBy)
import Data.Ord (comparing)

getCollatzSequenceLengthUpto :: Int64 -> (UArray Int64 Int64)
getCollatzSequenceLengthUpto n = runSTUArray $ do
    seqLengths <- newArray (1,n) 0
    forM_ [1..n] $ \i -> do
        let compute l s 1  = return (l+1, reverse s)
            compute l s n' = do
                v <- if n' > n then return 0 else readArray seqLengths n'
                if v > 0
                    then return (l+v, reverse s)
                    else compute (l+1) (n':s) (if even n' then n' `div` 2 else (3*n'+1))
        (l, u) <- compute 0 [] i
        forM_ (filter (\(n',_) -> n' <= n) $ zip u [l,l-1..]) $ \(ix, e) -> do
            writeArray seqLengths ix e
    return seqLengths

main = print $ fst $ maximumBy (comparing snd) $ assocs $ getCollatzSequenceLengthUpto 1000000


Pull-up sequence accumulator counter I wanted to create a function describing a sport game called "Leader". The idea is that you make as many push-ups as you can, increasing each repetition by 1 and as you reach your maximum, each next repetition is decreased by 1 until you reach 0 push-ups eventually.

I managed to do this using dictionaries, but I think this could be done in much easier way.

from typing import List, Tuple

def leader_step(max_pushups, step): # maximum pushups a person can do and a step of increment
    i = 0  # count of the repetitions
    pushups: List[Tuple[int, int]] = [(0, 0)]  # number of pushups at the beginning (at each repetition, it total)
    while pushups[i][0] <= max_pushups + abs(step): # +abs(step) in case step > 1
        if pushups[i][0] >= max_pushups:  # decrease push-ups as they reach max
            step = -step
        i += 1
        now = step + pushups[i - 1][0]
        sum = now + pushups[i - 1][1]  # counting the sum of all push-ups by adding previous sum and current pushups
        pushups.insert(i, (now, sum))
        if pushups[i][0] < 1:  # game stops when you reach 0 push-up
            break
    return pushups[1:-1]

Function should return 2 sequences:

showing the number of push-ups at each repetition
showing total sum of push-ups made at each repetition

You can indeed simplify this quite a bit using a generator and the itertools module.

I would separate out the generating of the pushups to be done from the total pushups. For this you can use two range objects and the yield from (Python 3.3+) keyword combination:

def pushups(n):
    yield from range(1, n)
    yield from range(n, 0, -1)

The accumulation can be done using itertools.accumulate and itertools.tee to duplicate the generator:

from itertools import accumulate, tee

def leader_step(n):
    gen1, gen2 = tee(pushups(n))
    return list(gen1), list(accumulate(gen2))

if __name__ == "__main__":
    print(leader_step(5))
# ([1, 2, 3, 4, 5, 4, 3, 2, 1], [1, 3, 6, 10, 15, 19, 22, 24, 25])

As noted in the comments by @Peilonrayz, it is not actually necessary to split the generator (as long as it fits into memory, which is very likely, given that presumably a human will try to do this training):

def leader_step(n):
    training = list(pushups(n))
    return training, list(accumulate(training))


Automatic C++ enum to string mapping macro I have long been looking for an elegant, one-line solution to map enum to strings automatically, for use with the << and >> stream operators.

I know that a lot of macros have been proposed to achieve that, but I never found a really simple scheme, with only one macro call.

This is my attempt, and I would be interested to discuss the advantages and limitations of this approach...

Definitions:

#include <string>
#include <iostream>
#include <stdexcept>
#include <algorithm>
#include <iterator>
#include <sstream>
#include <vector>

#define MAKE_STRING(str, ...) #str, MAKE_STRING1_(__VA_ARGS__)
#define MAKE_STRING1_(str, ...) #str, MAKE_STRING2_(__VA_ARGS__)
#define MAKE_STRING2_(str, ...) #str, MAKE_STRING3_(__VA_ARGS__)
#define MAKE_STRING3_(str, ...) #str, MAKE_STRING4_(__VA_ARGS__)
#define MAKE_STRING4_(str, ...) #str, MAKE_STRING5_(__VA_ARGS__)
#define MAKE_STRING5_(str, ...) #str, MAKE_STRING6_(__VA_ARGS__)
#define MAKE_STRING6_(str, ...) #str, MAKE_STRING7_(__VA_ARGS__)
#define MAKE_STRING7_(str, ...) #str, MAKE_STRING8_(__VA_ARGS__)
#define MAKE_STRING8_(str, ...) #str, MAKE_STRING9_(__VA_ARGS__)
#define MAKE_STRING9_(str, ...) #str, MAKE_STRING10_(__VA_ARGS__)
#define MAKE_STRING10_(str) #str

#define MAKE_ENUM(name, ...) MAKE_ENUM_(, name, __VA_ARGS__)
#define MAKE_CLASS_ENUM(name, ...) MAKE_ENUM_(friend, name, __VA_ARGS__)

#define MAKE_ENUM_(attribute, name, ...) name { __VA_ARGS__ }; \
    attribute std::istream& operator>>(std::istream& is, name& e) { \
        const char* name##Str[] = { MAKE_STRING(__VA_ARGS__) }; \
        std::string str; \
        std::istream& r = is >> str; \
        const size_t len = sizeof(name##Str)/sizeof(name##Str[0]); \
        const std::vector<std::string> enumStr(name##Str, name##Str + len); \
        const std::vector<std::string>::const_iterator it = std::find(enumStr.begin(), enumStr.end(), str); \
        if (it != enumStr.end())\
            e = name(it - enumStr.begin()); \
        else \
            throw std::runtime_error("Value \"" + str + "\" is not part of enum "#name); \
        return r; \
    }; \
    attribute std::ostream& operator<<(std::ostream& os, const name& e) { \
        const char* name##Str[] = { MAKE_STRING(__VA_ARGS__) }; \
        return (os << name##Str[e]); \
    }

Usage:

enum MAKE_ENUM(Test3, Item13, Item23, Item33, Itdsdgem43);

class Essai {
public:
    enum MAKE_CLASS_ENUM(Test, Item1, Item2, Item3, Itdsdgem4);

};

int main() {
    std::cout << Essai::Item1 << std::endl;

    Essai::Test ddd = Essai::Item1;
    std::cout << ddd << std::endl;

    std::istringstream strm("Item2");
    strm >> ddd;

    std::cout << (int) ddd << std::endl;
    std::cout << ddd << std::endl;
}

I agree with the comments: macros are a mess. Avoid them when possible.

If you're willing to make certain sacrifices (a little repetition), it's possible to avoid macros here. Doing so will also avoid some of the arbitrary limitations in your macro implementation, such as the 10 or so cap on enum values.

Using C++11, it's not hard to wire up Loki Astari's approach to operator<< and operator>> with some type traits template metaprogramming. I personally think enums are much cleaner with the strongly typed enum class ... variant introduced by C++11, so I would suggest that anyway, but the enum_serializable approach I show here works on both strongly and weakly typed enums.

#include <type_traits>
#include <iostream>
#include <https://codereview.stackexchange.com/a/14315/507 by reference>

template <typename Enum>
struct enum_serializable : std::false_type
{};

// enable operator<< and operator>> for `Essai` values
template <>
struct enum_serializable<Essai> : std::true_type
{};

template <typename Enum>
typename std::enable_if<enum_serializable<Enum>::value, std::ostream&
>::type operator<<(std::ostream& os, Enum e)
{
    return os << enumToString(e);
}

template <typename Enum>
typename std::enable_if<enum_serializable<Enum>::value, std::istream&
>::type operator>>(std::istream& is, Enum& e)
{
    return is >> enumFromString(e);
}

It's also not hard to pull Loki Astari's approach into the operator<< and operator>> functions above, removing the separate enumToString and enumFromString functions and the enumRefHolder and enumConstRefHolder structs entirely.

This general approach should be available before C++11 as well, but std::enable_if will have to be implemented instead of #included.


Getting angle between all points in array I have got some code that calculates the angle between all of the points in an array. It works, but it is quite slow. This is because it has complexity O(n^2). It has to loop through the array and apply the angle function to every combination of points.

import math
import random
def angle(pos1,pos2):
    dx=pos2[0]-pos1[0]
    dy=pos2[1]-pos1[1]
    rads=math.atan2(dy,dx)
    rads%=2*math.pi
    degs=math.degrees(rads)
    return degs
class Point(object):
    def __init__(self):
        self.pos=[random.randint(0,500) for _ in range(2)]#random x,y
        self.angles=[]
points=[Point() for _ in range(100)]
for point in points:
    point.angles=[] #so that each frame, they will be recalculated
    for otherC in points:
        if otherC is point:continue #don't check own angle
        ang=angle(point.pos,otherC.pos)
        point.angles.append(ang)

I feel like it could be greatly sped up by using numPy or some other method. I did some searching (here), but all I could find was functions to get angle between planes, or complex numbers. I just need simple arrays. How can this code be optimized for more speed?

Mathematically, if you have the angle between \$ A(x_1, y_1) \$ and \$ B(x_2, y_2) \$, given as:

$$ \theta = \tan^{-1}{\dfrac{y_2 - y_1}{x_2 - x_1}} \tag{in degrees} $$

then, the angle between \$ B \$ and \$ A \$ would be:

$$ \phi = 180° + \theta \mod {360°} $$

The \$ \mod{360} \$ is simply there if you want results in the interval \$ [0, 360) \$.

With the above, you'll only need to calculate angles between 2 points once, halving the number of calculations performed.


Determining if variables (integers) are greater or less than 0 Hobbyist here. I think my case/when logic needs to be cleaned up a bit. I'd also appreciate critique on the other parts...but I'm more interested in cleaning up the case/when statements...

puts "please assign an integer to x:"
x = gets.chomp.to_i

if (x > 0)
  value_of_x = true
else
  value_of_x = false
end

puts "now please assign an integer to y:"
y = gets.chomp.to_i

if (y > 0)
  value_of_y = true
else
  value_of_y = false
end

case
when (value_of_x == true) && (value_of_y == true)
  puts "two variables are > 0"
when ((value_of_x == true) && (value_of_y == false)) || ((value_of_x == false) && (value_of_y == true))
  puts "at least one variable is > 0"
else
  puts "no variables are > 0"
end

The following will simplify your boolean logic, remove repetition in your input processing, and better express what you're really doing:

puts "please assign an integer to x:"
x = gets.chomp.to_i

puts "now please assign an integer to y:"
y = gets.chomp.to_i

num_positive = (x > 0 ? 1 : 0) + (y > 0 ? 1 : 0)

case num_positive
when 2
  puts "two variables are > 0"
when 1
  puts "at least one variable is > 0"
else
  puts "no variables are > 0"
end


Databinding entire objects between child Vue components and the root Vue instance I have a form in which I am collecting data from two applicants. In order to minimize HTML duplication I built an applicant Vue component that generates each applicant from a template.

Each applicant has an applicant data object. My question is regarding the best practice for data binding these applicant objects inside the component back to the root Vue instance. Here is my working code:

Vue Root Instance

var app = new Vue({
   el: '#app',
   data: {
      form: {
         borrower: {
            firstname: 'John',
            middlenames: '',
            surname: 'Rambo'
         },
         coborrower: {
            firstname: 'Homer',
            middlenames: '',
            surname: 'Simpson'
         }
      }
   }
})

Template

Component

The applicant prop gives me access to the applicant objects in the root Vue instance

Vue.component('applicant', {
   template: '#applicant-container',
   props: ['applicant'],
})

Component Instance

I pass each applicant object to the component instance through the applicant prop.

<applicant :applicant="form.borrower"></applicant>
<applicant :applicant="form.coborrower"></applicant>

This code works perfectly. Any changes that are made using the inputs generated from the components is reflected when I output the applicant objects e.g

{{ form.borrower }}
{{ form.coborrower }}

The thing that I'm unsure about is this approach best practice? The Vue's docs state:

  ...the parent may need to pass data down to the child, and the child may need to inform the parent of something that happened in the child. However, it is also very important to keep the parent and the child as decoupled as possible via a clearly-defined interface

  Every component instance has its own isolated scope. This means you cannot (and should not) directly reference parent data in a child component’s template1

Since I'm passing the parent applicant object to the component and am modifying it directly through that passed in reference using the v-model directive am I not directly modifying the parent object so therefore keeping the data coupled as the docs recommended against.

1https://vuejs.org/v2/guide/components.html#Composing-Components

General feedback

The use of the

Borrower:
 in parent:{{ form.borrower }}
  <applicant v-model="form.borrower"></applicant>

Co-borrower:
 in parent:{{ form.coborrower }}
  <applicant v-model="form.coborrower"></applicant>


Finding all possible letter combinations from an inputted phone number Prompt:

  Given a string containing digits from 2-9 inclusive, return all
  possible letter combinations that the number could represent.

  A mapping of digit to letters (just like on the telephone buttons) is
  given below. Note that 1 does not map to any letters.

I'm a relatively new LeetCoder and I feel pretty good about solving this problem. What are some obvious improvements that I could be making to improve the runtime or approach towards this problem or other problems in general?

public IList<string> LetterCombinations(string digits)
{
    string[] ph = new string[] { "0", "1", "abc", "def", "ghi", "jkl", "mno", "pqrs", "tuv", "wxyz" };
    List<string> result = new List<string>();
    if (digits == null || digits.Length == 0) return result;
    int len = ph[digits[0] - '0'].Length;
    for (int i = 0; i < len; i++)
    {
        GetCombos(digits, "", i, ref result, digits.Length);
    }

    return result;
}

public void GetCombos(string inputDigits, string curVariation, int charIndex, ref List<string> resultList, int length)
{
    string[] phone = new string[] { "0", "1", "abc", "def", "ghi", "jkl", "mno", "pqrs", "tuv", "wxyz" };

    if (curVariation.Length != length)
    {
        char ch = phone[inputDigits[0] - '0'][charIndex];
        curVariation += ch;
    }
    if (curVariation.Length == length)
    {
        resultList.Add(curVariation);
        return;
    }

    string newInput = inputDigits.Substring(1, inputDigits.Length - 1);
    if (newInput == "") return;
    int numChars = phone[newInput[0] - '0'].Length;
    for (int i = 0; i < numChars; i++)
    {
        GetCombos(newInput, curVariation, i, ref resultList, length);
    }
}

There is only one thing I really don't like:

You have the same array instantiated in two different places (the phone/ph array). Here it's fairly simple and easy to maintain, but IRL you should never do that.

You have two options to resolve this:

1) Create the phone array as a class field or property.

2) In the newest versions of C# you can place the recursive GetCombos(...) as an internal/local function inside LetterCombinations(...) and then only have string[] phone = {...} defined in the outer:

IList<string> LetterCombinations(...)
{
  string[] phone = { ... };

  void GetCombos(...)
  {
    ...
    char ch = phone[inputDigits[0] - '0'][charIndex];
    ...
  }

  int len = phone[digits[0] - '0'].Length;
  ...
}

As shown the inner function can access the variables defines in the containing method.

Other things:

public void GetCombos(string inputDigits, string curVariation, int charIndex, ref List<string> resultList, int length) { ... }

You don't need the ref keyword because a list is a reference type.

I don't think newInput will ever be empty, so you can skip this check:

  if (newInput == "") return;

I would make this test

  if (digits == null || digits.Length == 0) return result;

as the first thing to do in the method:

  if (string.IsNullOrWhiteSpace(digits)) return new List<string>();

(or maybe throw an exception?)

In GetCombos(...) you could use the yield return concept instead of the resultList argument as shown below:

public IList<string> LetterCombinations(string digits)
{
  if (digits == null || digits.Length == 0) return new List<string>();

  string[] phone = new string[] { "0", "1", "abc", "def", "ghi", "jkl", "mno", "pqrs", "tuv", "wxyz" };

  List<string> result = new List<string>();

  IEnumerable<string> GetCombos(string inputDigits, string curVariation, int charIndex, int length)
  {
    if (curVariation.Length != length)
    {
      char ch = phone[inputDigits[0] - '0'][charIndex];
      curVariation += ch;
    }
    if (curVariation.Length == length)
    {
      yield return curVariation;
    }
    else
    {
      string newInput = inputDigits.Substring(1, inputDigits.Length - 1);
      int numChars = phone[newInput[0] - '0'].Length;

      for (int i = 0; i < numChars; i++)
      {
        foreach (string variation in GetCombos(newInput, curVariation, i, length))
        {
          yield return variation;
        }
      }
    }
  }

  int len = phone[digits[0] - '0'].Length;
  for (int i = 0; i < len; i++)
  {
    result.AddRange(GetCombos(digits, "", i, digits.Length));
  }

  return result;
}

The LeetCode challenge states that the return value of LetterCombinations(...) should be IList<string>, but it would be more elegant to let it return IEnumerable<string> as well and then use yield return insted of result.AddRange(GetCombos(...)) (you'll have to make it in a foreach () loop though).

Just for the exercise I made my own version:

IEnumerable<string> LetterCombinations(string digits)
{
  string[] phone = new string[] { "0", "1", "abc", "def", "ghi", "jkl", "mno", "pqrs", "tuv", "wxyz" };

  int length = digits.Length;

  IEnumerable<string> Runner(int buttonIndex, int digitIndex)
  {
    if (buttonIndex < 2 || buttonIndex > 9)
      throw new ArgumentOutOfRangeException(nameof(digits));

    if (digitIndex == length)
    {
      foreach (char ch in phone[buttonIndex])
        yield return ch.ToString();
    }
    else
    {
      int newButtonIndex = digits[digitIndex] - '0';
      digitIndex++;

      foreach (char ch in phone[buttonIndex])
      {
        foreach (string tail in Runner(newButtonIndex, digitIndex))
        {
          yield return $"{ch}{tail}";
        }
      }
    }
  }

  return Runner(digits[0] - '0', 1);
}

Not to claim it to be better than yours - in fact it seems to be slightly slower - but just to show another approach.

In fact it can be done in one line using LINQ:

IEnumerable<string> LetterCombinations(string digits)
{
  string[] phone = new string[] { "0", "1", "abc", "def", "ghi", "jkl", "mno", "pqrs", "tuv", "wxyz" };
  return digits.Skip(1).Select(d => d - '0').Aggregate(phone[digits[0] - '0'].Select(c => c.ToString()), (acc, i) => phone[i].SelectMany(c => acc.Select(a => $"{a}{c}")));
}


Search Android contacts more efficiently Is there is better way to do that more efficiently?

onCreate():

@Override
protected void onCreate(Bundle savedInstanceState)
{
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);
    layout = new Layout();
    Event event = new Event(layout);
}

Layout and Event:

class Layout
{
    public Layout()
    {
         txtName = (EditText)findViewById(R.id.txtName);
         btnSerch = (Button)findViewById(R.id.btnSerch);
        group1 = (RadioGroup)findViewById(R.id.group1);
    }
    RadioGroup group1;
    EditText txtName;
    Button btnSerch;
}

class Event
{
    public Event(Layout layout)
    {
        layout.btnSerch.setOnClickListener(new start_Serch());
    }
}

class start_Serch implements OnClickListener
{
    @Override
    public void onClick(View view)
    {
        Serch(view, layout);
    }
    Layout layout;
}

Main goal: (the main commands of the application)

void Serch(View v, Layout layout)
{
    Uri Contacts = android.provider.ContactsContract.Contacts.CONTENT_URI;

    Cursor C = getContentResolver().query(Contacts, null, null, null, null);

    if(C != null)
    {
        if(C.moveToFirst())
        {
            do
            {
                String display_ContactsName = getValue(C, android.provider.ContactsContract.Contacts.DISPLAY_NAME);

                if (Check(display_ContactsName, C) == true)
                {
                    break;
                }

            }while (C.moveToNext());
        }
    }
}

getValue() and Check():

private String getValue(Cursor cursor, String name)
{
    return cursor.getString(cursor.getColumnIndex(name));
}
private boolean Check(String Name, Cursor C)
{
    boolean Found = false;
    int id = layout.group1.getCheckedRadioButtonId();
    switch (id)
    {
        case -1:
            break;
        case R.id.fullName:
            if (layout.txtName.getText().toString().equals(Name))
            {
                AlertDialog.Builder alert = new AlertDialog.Builder(MainActivity.this);

                alert.setTitle("Hey This Contact Is in the LIST!!! yay you are the best lasy man EVER SO G***");

                alert.setMessage("The contact "+Name+" was found in this phone... oh ya");

                alert.setPositiveButton("ok",new DialogInterface.OnClickListener() {

                    @Override
                    public void onClick(DialogInterface dialogInterface, int i)
                    {
                    }
                });
                alert.show();
                Found = true;
            }
            break;
        case R.id.Contane:
            if ( Name.toLowerCase().contains ( layout.txtName.getText ().toString ().toLowerCase()))
            {
                AlertDialog.Builder alert = new AlertDialog.Builder(MainActivity.this);

                alert.setTitle("this is the first result for the name that you typed");

                alert.setMessage("The contact "+Name+" was found in this phone... oh ya");

                alert.setPositiveButton("ok",new DialogInterface.OnClickListener() {

                    @Override
                    public void onClick(DialogInterface dialogInterface, int i)
                    {
                    }
                });
                alert.show();
                Found = true;
                break;
            }
    }

     if (C.isLast() == true&&layout.txtName.getText().toString().equals(Name) == false&& Name.toLowerCase().contains ( layout.txtName.getText ().toString ().toLowerCase()) == false)
    {
        AlertDialog.Builder alert = new AlertDialog.Builder(MainActivity.this);

        alert.setTitle("Hey This Contact Is NOT in the LIST!!!");

        alert.setMessage("The contact/part of "+layout.txtName.getText().toString()+" was not found in this phone...");

        alert.setPositiveButton("ok",new DialogInterface.OnClickListener() {

            @Override
            public void onClick(DialogInterface dialogInterface, int i)
            {
            }
        });
        alert.show();
    }
    return Found;
}

Android Strings

Android provides a nice way of externalizing Strings in a strings.xml resource file. Using it right from the start of your project or as soon as possible is a very good idea, you might regret it later if you don't. Using XML-resources in Android is how to provide internationalization of your application. It is also good to keep all strings in one place, so you can get a nice overview of the strings shown in your application.

Speaking of Strings that are showing in your application, the content of your message String for your AlertDialogs is very questionable. If I as an user would encounter the message "f*ck ya" in an application, I would never ever give it a 5 star rating on Google Play. And it honestly does not give me a good impression of you as a person either.

The "ok" message is already defined as a string message in Android, and if your dialog button should only close the dialog then you can pass null as the listener. So therefore you can use alert.setPositiveButton(android.R.string.ok, null); which greatly improves code readability.

Readability

Speaking of code readability... I feel that you have an excessive amount of empty lines in your code, including but not restricted to:

if(C != null)

{

            alert.setPositiveButton("ok",new DialogInterface.OnClickListener() {

                @Override

                public void onClick(DialogInterface dialogInterface, int i)

        do

        {

            String display_ContactsName = getValue(C, android.provider.ContactsContract.Contacts.DISPLAY_NAME);

Instead consider this:

if(C != null)
{
    ...
    alert.setPositiveButton("ok",new DialogInterface.OnClickListener() {
        @Override
        public void onClick(DialogInterface dialogInterface, int i)
    ...
        do
        {
            String display_ContactsName = getValue(C, android.provider.ContactsContract.Contacts.DISPLAY_NAME);


Function for formatting a timespan with optional trailing zero I'm looking to achieve a time format that looks like this: 2d 4h remaining; or 1h 36m remaining; or 35s remaining, etc. So that it only displays the two largest values of time (this is how Clash of Clans and some other mobile games format wait times.)

I would like to format the wait time value as simply 5h or 5h 0m depending on whether I am displaying the total wait time or currently counting down. My code is below… But is there a slightly simpler way of writing it without so many returns?

(This question was originally asked here:
Is there a simpler way to format this timespan? )

public string FormatRushTime ( System.TimeSpan span, boolean removeZeros )
{
    if ( span.Days > 0 )
    {
        if ( span.Hours > 0 || !removeZeros )
            return String.Format("{0:d}d {1:d}h", span.Days, span.Hours);
        else
            return String.Format("{0:d}d", span.Days);
    }
    if ( span.Hours > 0 )
    {
        if ( span.Minutes > 0 || !removeZeros )
            return String.Format("{0:h}h {1:m}m", span.Hours, span.Minutes);
        else
            return String.Format("{0:h}h", span.Hours);
    }
    if ( span.Minutes > 0 )
    {
        if ( span.Seconds > 0 || !removeZeros )
            return String.Format("{0:m}m {1:s}s", span.Minutes, span.Seconds);
        else
            return String.Format("{0:m}m", span.Minutes);
    }
    return String.Format("{0:d}s", span.Seconds);
}

If your goal is to reduce returns, you could use a single string.Format call and not make use of additional arguments in certain cases:

// Changed removeZeroes to includeZeroes since it read kind of like a double negative.
string format = includeZeros ? "{0:d}d {1:d}h" : "{0:d}d";

// span.Hours will not get used if there's nowhere to put it in the format string:
return string.Format(format, span.Days, span.Hours);


Generating public/private RSA key pair programmatically, using it for JWTs In my app I'm using JSON Web Tokens for authentication purposes, using the pyjwt library. Instead of using static keys and/or worrying about key distribution, the server generates a public/private key pair upon startup itself and just keeps it in memory. The keys will be reset and thereby all existing tokens invalidated when the server restarts, which is fine for the intended use case. The key pair is generated using pycrypto.

The question is: am I generating and using the keys correctly, or is there any flaw which can lead to an exploit? Below is the general class which generates and validates all JWTs in my app:

from typing import NamedTuple
from datetime import datetime
import jwt
from Crypto.PublicKey import RSA

KeyPair = NamedTuple('KeyPair', [('public', str), ('private', str)])

class JWT:
    def __init__(self, keypair: KeyPair=None, algorithm: str='RS256'):
        self.keypair = keypair or self.generate_keypair()
        self.algorithm = algorithm

    @staticmethod
    def generate_keypair() -> KeyPair:
        key = RSA.generate(2048)
        return KeyPair(public=key.publickey().exportKey('PEM').decode('ascii'),
                       private=key.exportKey('PEM').decode('ascii'))

    def generate_token(self, payload: dict) -> str:
        payload.update(dict(iat=datetime.utcnow()))
        return jwt.encode(payload, key=self.keypair.private, algorithm=self.algorithm).decode('ascii')

    def decode_payload(self, token: str, **kwargs) -> dict:
        try:
            return jwt.decode(token, self.keypair.public, algorithms=[self.algorithm], **kwargs)
        except jwt.exceptions.InvalidTokenError as e:
            raise InvalidAuthenticationToken

There isn't too much to see here because the key generation simply relies on RSA.generate(2048), but I wonder why you would need this code as it is exceedingly shallow.

Regenerating key pairs for signing at startup is utter nonsense because a key pair is next to useless if the public key isn't trusted by the receiving party.

Exporting the private key as unprotected PEM is also very dangerous. The method would fail if hardware support (a HSM) is ever used.

The inclusion of  payload.update(dict(iat=datetime.utcnow())) is undocumented. Although it may be expected for webtokens, a caller will want to know that this is performed. Undocumented side effects are a sure fire way of breaking the principle of least surprise.

Passing kwargs** without any validation doesn't give me a very warm feeling inside.

Slightly less of an issue maybe, but an RSA 2048 bit key provides a key strength of only 112 bits or so. You may want to consider at least 3072 bits.

This code is clearly related only to webtokens and RSA keys, even with a specific key size. So allowing an argument algorithm: str='RS256' may just allow a user to get the object instance in an invalid state, and not much more.


Double Linked List remove method I am implementing some basic data structures to freshen up my memory and I wanted my DoubleLinkedList remove method to be reviewed. I feel like it has extra checks but I could not think of another way to implement.

My implementation holds both the head and the tail of the list making things a bit more complex.

remove method:

public void remove(T data) {
    if (isEmpty()) {
        throw new NoSuchElementException();
    }

    for (Node<T> current = this.head; current != null; current = current.getNext()) {
        if (current.getData().equals(data)) {
            Node<T> previous = current.getPrevious();
            Node<T> next = current.getNext();

            if (this.size == 1) { // At head with size == 1
                this.head = this.tail = null;
            } else if (previous == null) { // At head with size > 1
                this.head = next;
                next.setPrevious(previous);
            } else if (next == null) { // At tail
                previous.setNext(next);
                this.tail = previous;
            } else { // Rest of cases
                previous.setNext(next);
                next.setPrevious(previous);
            }
            this.size--;
            return;
        }
    }
    throw new NoSuchElementException();
}

I have couple suggestions:

Separate Logic

I would separate lookup and modify logic. Add a function to remove element and call it from this one leaving in this one only logic to lookup an item.

public void remove(Node<T> data) {
    if (!data) {
        throw new NoSuchElementException();
    }

    // Removing logic here
    ...
}

public void remove(T data) {
    return remove(search(data));
}

public Node<T> search(T data) {
    for (Node<T> current = this.head; current != null; current = current.getNext()) {
        if (current.getData().equals(data)) {
            return current;
        }
    }

    return null;
}

Simplify Checks

I think this should work and cover all your cases, no?

// If this is head of the list
if (previous == null) {
    this.head = next;
}
else {
    previous.setNext(next);
}

// If this is tail of the list
if (next == null) {
    this.tail = previous;
}
else {
    next.setPrevious(previous);
}


Could this template function be improved? I'm writing wrapper functions for some of the functions in <algorithm>.
While the following works perfectly, I'm not sure if this is the best way to approach it:

template<class ContainerType, class Predicate>
ContainerType filter(
    const ContainerType& container,
    Predicate predicate
) {
    ContainerType filteredContainer;
    std::remove_copy_if(
        container.begin(),
        container.end(),
        std::back_inserter(filteredContainer),
        predicate
    );
    return filteredContainer;

Would you change something about the wrapper? I'm especially worried about the template parameters, e.g. you'll have to read the entire function in order to know what types are valid for predicate etc.

The use of template parameters here is just fine. You can call the second one UnaryPredicate if that makes you happier, but in the end, C++ currently lacks the ability to specify further requirements on template arguments. (That's what concepts are for.)

So you really need to specify the detailed requirements in prose in the documentation, e.g.

  ContainerType must fulfill the Container requirement, and its value_type must be Copyable. Predicate must be an UnaryPredicate taking value_type as its argument.

What you could possibly improve is making this more efficient in C++11 when the argument is a temporary.