Check existence of user identification number I was tasked with writing a python script to check if a passed ID number exists within a file. The file I was give contains roughly 25,000 unique ID numbers. An ID number is a unique 9 digit number.

I have a couple specific things for you to comment on if you decide to review this:

As it stands, I convert the user_id passed in the id_exists to a str. I do this because I need the ID number to be the same type when I read the ID's from the file. Is there an easier way to accomplish this?
This is my first time using a PEP 484 type hint, Generator. Am I using it correctly?
This is also my first time utilizing a generator in this fashion. I used a generator here because instead of loading the entire file into memory, storing in a list, then iterating over the list, I can read each line and can exit early if an ID is found. Is it good that I'm using it in this way, or is there an easier way to do this?
Using a helper function is a new concept to me. Am I utilizing this helper function in the most pythonic way?

Of course, any constructive criticism beyond these points is welcome. Thanks in advance.

"""
Ensures id number matches any within the ID file.

An ID number is a unique 9 digit number.
"""

import random
import time

from typing import Union, Generator

def id_exists(user_id: Union[int, str]) -> bool:
    """
    Determines if the passed ID exists within the ID file. The
    ID passed must be of length 9 exactly, and only contain numbers.

    :param Union[int, str] user_id: User ID to check for existence

    :return bool: True if ID exists in file, False otherwise
    """

    # Generator[yield_type, send_type, return_type] #
    def get_ids() -> Generator[str, None, None]:
        """
        Helper function for `id_exists`.

        Yields each line in the ID file.

        :return Generator[str, None, None]: Lines in ID file.
        """

        with open("ids.txt", "r") as file:
            for line in file:
                yield line.strip()

    # Convert user_id to <class 'str'> if not already #
    if type(user_id) == int:
        user_id = str(user_id)

    # Ensure user_id matches specifications #
    if len(user_id) != 9 or not user_id.isdigit():
        raise ValueError("ID should be 9 characters long and all digits.")

    # Check ID against file and return result #
    return any(user_id == stored_id for stored_id in get_ids())

One of:

Only take a string or a number.
Use str either way.
Work against a bespoke class.

Yes. But I find it better to just use Iterator[str], unless you're using the coroutine aspects of it.
Yeah that's fine. But:

You can just use in rather than any(...).
You don't need the function get_ids you can just use any, like you are now.

Don't use type(...) == int, use isinstance.
Validating user_id should probably happen somewhere else.

I'll make a class as an example to show this.

class UserId:
    __slots__ = ('id',)
    id: int

    def __init__(self, id: int) -> None:
        if not self.valid_id(id):
            raise ValueError("ID should be 9 characters long and all digits.")
        self.id = id

    @staticmethod
    def valid_id(id: int) -> bool:
        if not isinstance(id, int):
            return False
        str_id = str(id)
        return (
            len(str_id) == 9
            and str_id.isdigit()
        )

    def exists(self) -> bool:
        user_id = str(self.id)
        with open('ids.txt') as f:
            return any(user_id == line.strip() for line in f)

print(UserId(123456789).exists())


Properties and on change events So I made a socket communication library. And a part of it is IConnection

public enum ConnectionState
{
    NotConnected, Connecting, Connected, Authenticated, Disconnecting, Disconnected
}

public interface IConnection
{
    ConnectionState State { get; }

    event Action Connected;
    event Action Disconnected;
    event Action Authenticated;
    event Action AuthenticationFailed;

    // this two methods are the core of my question
    void OnAuthenticated();
    void OnAuthenticationFailed();

    bool Send(byte[] data);
    void Connect();
    void Close();
}

Of course IConnection provides information about connection state and is able to fire Connected/Disconnected events as it holds Socket. There is no doubt.

Now, users of IConnection would also like to know when it becomes authenticated. For example, server might listen to that events, and once connection is authenticated - send client's initial configuration data. Or client might listen to that events and decide to start communication or retry authentication process.

But. The problem is, authentication process exists in the protocol layer. IConnection has no idea such layer even exists. Protocol layer actually uses IConnection to send serialized to byte[] message to the other party. And for this one particular message kind, protocol layer is able to send data upon connection in ConnectionState.Connected state.

So, for IConnection to be able to change it's state and inform subscribers on auth process i had to implement this two methods

void OnAuthenticated();
void OnAuthenticationFailed();

Which are, obviously called from protocol layer authentication process code.

I feel like I'm doing something wrong here. And since i work alone, any thoughts will be much appreciated.

You're not quite correct saying that IConnection has no ideas about protocol layer - in your defininition IConnection provides events about authentication process. So your code should have some connections between IConnection and protocol layer. I have no clue about entire relationships between layers in your application, but I can suggest some method:

provide function for calling events (like you suggested)
implement IConnection in protocol layer
realization of IConnection listens what happens in protocol layer (i.e. AssignAuthentificationService function)
implement IAuthentifiedConnection that knows about protocol layer


The Term-inator: Pi edition The fourth project, continuing my C++ saga with terrible post names. :P

  An approximate value of pi can be calculated using the series given
  below:

  $$ \pi \approx 4 \left[ 1 - \dfrac{1}{3} + \dfrac{1}{5} - \dfrac{1}{7} +
 \dfrac{1}{9} - \cdots + \dfrac{\left( -1\right)^n}{2n + 1} \right] $$

  Write a C++ program to calculate the approximate value of pi using
  this series.  The program takes an input \$ n \$ that determines the
  number of terms in the approximation of the value of pi and outputs
  the approximation.  Include a loop that allows the user to repeat this
  calculation for new values \$ n \$ until the user says she or he wants
  to end the program.

Also, I am required to use a for loop at least once in my code.

pi.cpp:

/**
 * @file pi.cpp
 * @brief Calculates pi for the given number of terms
 * @author syb0rg
 * @date 10/3/14
 */

#include <iostream>
#include <limits>
#include <cctype>
#include <cmath>

/**
 * Makes sure data isn't malicious, and signals user to re-enter proper data if invalid
 */
void getSanitizedDouble(long double &input)
{
    while (!(input = std::cin.peek()) && input != '\n')
    {
        if (std::isalpha(input) || std::isspace(input)) std::cin.ignore(); // ignore alphabetic and space characters from input
    }
    while(!(std::cin >> input) || input < 0)
    {
        std::cin.clear(); // clear the error flag that was set so that future I/O operations will work correctly
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); // skips to the next newline
        std::cout << "Invalid input.  Please enter a positive number: ";
    }
}

int main()
{
    long double num = 0.;
    char again = '\0';
    do
    {
        long double pi = 0.;

        // get input for height, re-read input if not a positive number
        std::cout << "Enter the number of terms to approximate π: ";
        getSanitizedDouble(num);

        for(auto i = 0; i < num; i++)
        {
            pi += std::pow(-1, i) / (2 * i + 1);
        }
        pi *= 4;

        std::fprintf(stdout, "Approximated value of π for %Lg terms: %Lg\n", num, pi);

        std::cout << "Run the program again (y/N): ";  // signify n as default with capital letter
        std::cin.get();  // absorb newline character from previous input
        std::cin.get(again);
        again = std::tolower(again);
    } while (again == 'y');
}

Out parameters in C++

In C++, you should almost never use out parameters (variables taken by reference and used to return a value from a function), you can read this excellent article by Eric Niebler. There are few cases where out parameters can make sense:

When you want your return to be fast. And even then, return value optimization and move semantics may still be faster.
When you have several output values. For example, you want to assign a value to a function and return whether it succeeded:

bool assign(int from, int& to)
{
    if (from != 0)
    {
        to = from;
        return true;
    }
    return false;
}

But even for this situation, there are better solutions such as boost:optional to return both a value and whether it succeeded or not. And if you need several error values, use exceptions. And if you need to actually return several values, you general want to pack them into a dedicated struct or a std::tuple.
Input-output parameters: sometimes, you want to take a parameter, read from it, and then write to it again. That is still a valid use case (but these are not strict out parameters anymore).

In your case, it seems that getSanitizedDouble could simply return the read long double instead of having an out parameter.

Type correctness

You could improve your type correctness:

That's also a naming issue: I don't expect a function named getSanaitzeDouble to return a long double, but to return a double.
The literal used in the expression long double pi = 0.; is incorrect: 0. is a double. The correct long double literal should be 0.0L. We could also use 0.0l, but the l suffix is really too close to 1 not to be dangerous.

Order your includes

Alphabetical order is something you should always use to order your headers (at least in a logical group of headers). That allows a faster search to check whether some headers has already been included:

#include <cctype>
#include <cmath>
#include <iostream>
#include <limits>


Modal component in Angular 2 I've recently started to check out Angular 2 and TypeScript that comes with it, and I guess I'm kinda into it. However I'm not too sure about the whole "TypeScript way" of doing things, specifically the typing of variables and such.

I wrote this simple modal component which displays a modal when the ModalApiService has its open function called from anywhere in the application. There can be multiple modals on the same page that's why I had to also include the passing of a modalId to the modal so I could identify them correctly.

Can this code be optimized in any way?
Do I set the types in a correct way? Are there more things I should set a type for?
Does TypeScript have any features that I'm not taking full advantage of here?

Component:

export class ModalComponent {
  public isOpen: boolean = false;

  constructor(private _modalApi: ModalApiService, private _elementRef: ElementRef) {

    let overlay = <HTMLScriptElement>document.getElementsByClassName('modal-overlay')[0];

    if (overlay) {

      overlay.addEventListener('click', (e: any) => {

        // Close the modal if a click is made on the modal overlay itself, not its children
        if ((e.target.getAttribute('class') || []).indexOf('modal-overlay') !== -1) {
          _modalApi.close(this.modalId);
        }
      });
    }

    _modalApi.isOpen$.subscribe(
      modal => {
        this.isOpen = modal.id === this.modalId ? modal.isOpen : false;
      }
    )
  }
}

Component API service:

@Injectable()

export class ModalApiService {
  private _isOpen = new Subject<boolean>();

  isOpen$ = this._isOpen.asObservable();

  open(id: string) {
    this._isOpen.next({isOpen: true, id: id});
  }

  close(id: string) {
    this._isOpen.next({isOpen: false, id: id});
  }
}

Cool stuff, though instead of declaring _isOpen property as a <boolean> in the service class, you should provide a new interface (as you added modalId).
Something like:

interface IModalElementStatus {
    isOpen: boolean;
    id: string;
}


My take at ProjectEuler #17 - counting counting characters I tried to tackle #17 problem on Project Euler:

  If the numbers 1 to 5 are written out in words:

one, two, three, four, five,

  then there are \$3 + 3 + 5 + 4 + 4 = 19\$ letters used in total.

  If all the numbers from 1 to 1000 (one thousand) inclusive were written out in words, how many letters would be used?

I tried to solve it using dictionary. The code is working fine for n = 99, but the solution for 999 is off by 36. Any ideas on what might be going wrong? Any ideas on how I could make it more efficient?

def number_let_count(n):
    sum_let = 0
    numbers = {0: '', 1: 'one', 2: 'two', 3: 'three', 4: 'four', 5: 'five', 6: 'six', 7: 'seven', 8: 'eight', 9: 'nine',
               10: 'ten', 11: 'eleven', 12: 'twelve', 13: 'thirteen', 14: 'fourteen', 15: 'fifteen', 16: 'sixteen', 17: 'seventeen', 18: 'eighteen', 19: 'nineteen'
               ,20: 'twenty', 30: 'thirty', 40: 'forty', 50: 'fifty', 60: 'sixty', 70: 'seventy', 80: 'eighty', 90: 'ninety', 100: 'hundred'}
    for int in range(1,n+1):
        if int in numbers and int != 100:
            sum_let += len(numbers[int])
        else:
            if int in range(21,100):
                sum_let += len(numbers[(int//10) * 10]) + len(numbers[int % 10])
            elif int in range(100,1000):
                if int % 100 == 0:
                    sum_let += len(numbers[(int//100)]) + len(numbers[100])
                else:
                    sum_let += len(numbers[(int//100)]) + len(numbers[100]) + len('and') + len(numbers[((int % 100)//10) * 10]) + len(numbers[(int % 100) % 10])
    return sum_let

A matter of priorities

Make it work, make it right, make it fast : at the moment, your code does not work in that sense that it does not give the solution to the project euler problem.

Nevertheless, I'll try to give you hints to rewrite your code in such a way that it returns the same results as it does currently but in a better way.

In order to do so, I've written a few assertions to be more confident when I perform changes.

assert number_let_count(5) == 19  # given by project euler
assert number_let_count(100) == 864  # found by running your code
assert number_let_count(345) == 6178  # idem
assert number_let_count(1000) == 21077  # idem and we know this result is wrong

Style

You code looks mostly good. Something that I find really confusing (and so does my text editor) is the fact that you named your variable int : i would probably be better.

Then, the logic can be rewritten by removing a level of nesting in the ifs using elif.

Here is what I have at this point :

def number_let_count(n):
    sum_let = 0
    numbers = {0: '', 1: 'one', 2: 'two', 3: 'three', 4: 'four', 5: 'five', 6: 'six', 7: 'seven', 8: 'eight', 9: 'nine',
               10: 'ten', 11: 'eleven', 12: 'twelve', 13: 'thirteen', 14: 'fourteen', 15: 'fifteen', 16: 'sixteen', 17: 'seventeen', 18: 'eighteen', 19: 'nineteen'
               ,20: 'twenty', 30: 'thirty', 40: 'forty', 50: 'fifty', 60: 'sixty', 70: 'seventy', 80: 'eighty', 90: 'ninety', 100: 'hundred'}
    for i in range(1,n+1):
        if i in numbers and i != 100:
            sum_let += len(numbers[i])
        elif i in range(21,100):
            sum_let += len(numbers[(i//10) * 10]) + len(numbers[i % 10])
        elif i in range(100,1000):
            if i % 100 == 0:
                sum_let += len(numbers[(i//100)]) + len(numbers[100])
            else:
                sum_let += len(numbers[(i//100)]) + len(numbers[100]) + len('and') + len(numbers[((i % 100)//10) * 10]) + len(numbers[(i % 100) % 10])
    return sum_let

Making debugging easier

Now that you know that your code is wrong, you might as well try to make it easier to debug. My suggestion is to work with strings until the very last moment so that you can print it whenever you need. At the end, you just need to get the length of the string and you are done.

def number_let_count(n):
    numbers = {0: '', 1: 'one', 2: 'two', 3: 'three', 4: 'four', 5: 'five', 6: 'six', 7: 'seven', 8: 'eight', 9: 'nine',
               10: 'ten', 11: 'eleven', 12: 'twelve', 13: 'thirteen', 14: 'fourteen', 15: 'fifteen', 16: 'sixteen', 17: 'seventeen', 18: 'eighteen', 19: 'nineteen'
               ,20: 'twenty', 30: 'thirty', 40: 'forty', 50: 'fifty', 60: 'sixty', 70: 'seventy', 80: 'eighty', 90: 'ninety', 100: 'hundred'}
    ret = ""
    and_ = "and"
    for i in range(1,n+1):
        if i in numbers and i != 100:
            ret += numbers[i]
        elif i in range(21,100):
            ret += numbers[(i//10) * 10] + numbers[i % 10]
        elif i in range(100,1000):
            if i % 100 == 0:
                ret += numbers[(i//100)] + numbers[100]
            else:
                ret += numbers[(i//100)] + numbers[100] + and_ + numbers[((i % 100)//10) * 10] + numbers[(i % 100) % 10]
    return len(ret)

This is making your code slower (performing string concatenations instead of additions) but you shouldn't be able to notice this because we are working with pretty small values. Also, this is a first step for an actual optimisations to be performed once it works : you just need to replace whatever involves string with their length variant :

def number_let_count(n):
    numbers = {0: '', 1: 'one', 2: 'two', 3: 'three', 4: 'four', 5: 'five', 6: 'six', 7: 'seven', 8: 'eight', 9: 'nine',
               10: 'ten', 11: 'eleven', 12: 'twelve', 13: 'thirteen', 14: 'fourteen', 15: 'fifteen', 16: 'sixteen', 17: 'seventeen', 18: 'eighteen', 19: 'nineteen'
               ,20: 'twenty', 30: 'thirty', 40: 'forty', 50: 'fifty', 60: 'sixty', 70: 'seventy', 80: 'eighty', 90: 'ninety', 100: 'hundred'}
    numbers = {i:len(s) for i,s in numbers.items()}
    ret = 0 # ""
    and_ = len("and")
    for i in range(1,n+1):
        if i in numbers and i != 100:
            ret += numbers[i]
        elif i in range(21,100):
            ret += numbers[(i//10) * 10] + numbers[i % 10]
        elif i in range(100,1000):
            if i % 100 == 0:
                ret += numbers[(i//100)] + numbers[100]
            else:
                ret += numbers[(i//100)] + numbers[100] + and_ + numbers[((i % 100)//10) * 10] + numbers[(i % 100) % 10]
    return ret

Here, 'len' is called only a few times.

I'll keep on working on the "slow" versions because it is a better starting point for your investigations but you know what you can do when it works.

Rewrite interval checks

Computing if i in range(100, 1000) is slow because we'll compare i to many values. What you want to know if just if i is bigger than 100 and (stricly) smaller than 1000. Python has a cool way to write this (you won't find this in all programming languages) : if 100 <= i < 1000.

for i in range(1,n+1):
    if i in numbers and i != 100:
        ret += numbers[i]
    elif 21 <= i < 100:
        ret += numbers[(i//10) * 10] + numbers[i % 10]
    elif 100 <= i < 1000:
        if i % 100 == 0:
            ret += numbers[(i//100)] + numbers[100]
        else:
            ret += numbers[(i//100)] + numbers[100] + and_ + numbers[((i % 100)//10) * 10] + numbers[(i % 100) % 10]

Now, the first check begs to be rewritten as a range check too. A cool thing to notice is that cases such as forty as properly handled by ret += numbers[(i//10) * 10] + numbers[i % 10] because the second term will correspond to "". For that reason, we can simply write : if 0 < i <= 20.

Then successive range checks do not make that much sense : it is probably not interesting checking if i is bigger than 21 if we know it is stricly bigger than 20. Similarly for 100.

Then, the whole logic can be rewritten (adding some logs you might find useful):

for i in range(1,n+1):
    if i < 1:
        print("Oops, did not handle", i)
    elif i <= 20:
        ret += numbers[i]
    elif i < 100:
        ret += numbers[(i//10) * 10] + numbers[i % 10]
    elif i < 1000:
        if i % 100 == 0:
            ret += numbers[(i//100)] + numbers[100]
        else:
            ret += numbers[(i//100)] + numbers[100] + and_ + numbers[((i % 100)//10) * 10] + numbers[(i % 100) % 10]
    else:
        print("Oops, did not handle", i)
return len(ret)

Divisions and modulos

Isn't it a pain to have to perform to operations to have the quotient and the remainder of a division ? It is not anymore with the pretty cool divmod

for i in range(1,n+1):
    if i < 1:
        print("Oops, did not handle", i)
    elif i <= 20:
        ret += numbers[i]
    elif i < 100:
        tens, units = divmod(i, 10)
        ret += numbers[10 * tens] + numbers[units]
    elif i < 1000:
        hundreds, units = divmod(i, 100)
        if units:
            tens, units2 = divmod(units, 10)
            ret += numbers[hundreds] + numbers[100] + and_ + numbers[10 * tens] + numbers[units2]
        else:
            ret += numbers[hundreds] + numbers[100]
    else:
        print("Oops, did not handle", i)

Another hint for one of the bugs

Try to see how "119" gets converted to a string.


Storing recordset data I have managed to write some pretty terrible code in VBA and I know there has to be a better way. I have made all of these variables to store some data which I eventually reference in the application.  I'm fairly certain I need to loop over the recordset or fields and store the variables that way but I can't figure out how to do it in a way that actually simplifies this code.  It's torturing me because it seems like a painfully simple concept.

The columns of the table that it is gathering data from looks similar to this:

[LANGUAGE_ID]-[NEXT]-[PREVIOUS]-[RESUME] -- and so on

sqlstmt = "SELECT *" & _
" FROM cf_Language" & _
" WHERE LANGUAGE_ID = " & parm_language & ";"

Set rst_t = dbs.OpenRecordset(sqlstmt, dbOpenSnapshot)

If (parm_language <> 1) Then
    parm_surveyname = UnicodeToAscii(parm_surveyname, True)
    aNext = UnicodeToAscii(rst_t![Next], True)
    aPrevious = UnicodeToAscii(rst_t![previous], True)
    aResume = UnicodeToAscii(rst_t![resume], True)
    aFinish = UnicodeToAscii(rst_t![finish], True)
    aSave = UnicodeToAscii(rst_t![Save], True)
    aLogIn = UnicodeToAscii(rst_t![log_in], True)
    aComplete = UnicodeToAscii(rst_t![complete], True)
    aHeader = UnicodeToAscii(rst_t![header], True)
    aThanks = UnicodeToAscii(rst_t![thanks], True)
    aSavedMsg = UnicodeToAscii(rst_t![saved_msg], True)
    aBlankCode = UnicodeToAscii(rst_t![blank_code], True)
    aInvalidCode = UnicodeToAscii(rst_t![invalid_code], True)
    aTimeOut = UnicodeToAscii(rst_t![timeout], True)
    aClosed = UnicodeToAscii(rst_t![closed], True)
    aAccessCode = UnicodeToAscii(rst_t![access_code], True)
    aConFirstTime = UnicodeToAscii(rst_t![con_first_time], True)
    aAnonFirstTime = UnicodeToAscii(rst_t![anon_first_time], True)
    aAnonResume = UnicodeToAscii(rst_t![anon_resume], True)
Else
    aNext = rst_t![Next]
    aPrevious = rst_t![previous]
    aResume = rst_t![resume]
    aFinish = rst_t![finish]
    aSave = rst_t![Save]
    aLogIn = rst_t![log_in]
    aComplete = rst_t![complete]
    aHeader = rst_t![header]
    aThanks = rst_t![thanks]
    aSavedMsg = rst_t![saved_msg]
    aBlankCode = rst_t![blank_code]
    aInvalidCode = rst_t![invalid_code]
    aTimeOut = rst_t![timeout]
    aClosed = rst_t![closed]
    aAccessCode = rst_t![access_code]
    aConFirstTime = rst_t![con_first_time]
    aAnonFirstTime = rst_t![anon_first_time]
    aAnonResume = rst_t![anon_resume]
End If

Key Points

I would not use local variables to hold recordset values, because you'd need one for every field of every returned row... which is just wrong.
As already pointed out, good variable names are readable variable names (which can be pronounced!).
You're not showing it, but make sure you close that connection once you're done.

"Entities"

Here's an [perhaps less appealing] alternative to Dictionary. Since I learned about C# and started using Entity Framework, I like wrapping each record in an object - given a class called MyEntity (which does nothing more than exposing getters and setters for all properties):

Private Type ThisEntity
    SomeProperty As String
    AnotherProperty As String
End Type

Private this As ThisEntity
Option Explicit

Public Property Get SomeProperty() As String
    SomeProperty = this.SomeProperty
End Property

Public Property Let SomeProperty(value As String)
    this.SomeProperty = value
End Property

Public Property Get AnotherProperty() As String
    AnotherProperty = this.AnotherProperty
End Property

Public Property Let AnotherProperty(value As String)
    this.AnotherProperty = value
End Property

You define this object only once, and then you instantiate it as many times as you have records in your recordset. With a dictionary you get easier loading (each "entity" is a dictionary where you identify each field by their names to retrieve the corresponding value), but with this object you completely avoid referring to your properties using strings - the only hard-coded strings involved are the field names.

Reading the recordset

All you need to do is exactly as you said, loop through the recordset:

Dim entity As MyEntity
Dim entities As New Collection
While Not rs.EOF And Not rs.BOF 'BOF check if result has 0 rows

    Set entity = New MyEntity
    With entity
        .SomeProperty = rs.Fields("SomeField").Value 'or rs!SomeField
        .AnotherProperty = rs.Fields("AnotherField").Value 'or rs!AnotherField
    End With

    entities.Add entity ', entity.Id /PK property if you want to use a key here
    rs.MoveNext 'don't forget this, or your loop will be endless!
Wend

rs.Close
Set rs = Nothing
'at this point you have your entire recordset's content in the "entities" collection

About rs!FieldName notation (little "Gotcha!")

For what it's worth, I've recently fixed a bug in our codebase that was caused by the rs!FieldName notation in a specific circumstance: rs!FieldName was being added to a collection, and the recordset was left open. When I found that out, I added rs.Close and all of a sudden the collection, although non-empty, could no longer be read. This is because adding rs!FieldName to the collection (or, if you take the dictionary road) was actually adding a reference to the Field object, and the original programmer was left with the impression it worked fine because he could retrieve any entry in the collection and see the field's value - when he was actually retrieving a Field object and through the default property was fetching the value from the recordset: that stopped working when I added code to close the recordset after all records were read.

That bug was fixed by explicitly adding rs.Fields("FieldName").Value to the collection instead.


Expose encrypted serial ID in Elixir I'm working on a Phoenix/Absinthe application and I thought to expose encrypted sequential IDs instead of UUIDs since these are a bit shorter.
Encryption on Elixir/Erlang seems very hard, so I think I'll use UUIDs eventually.

Anyway I'd like to know how bad, from security perspective, is the solution I came up with:

defmodule MyAppWeb.GraphQL.Types.EncId do
  use Absinthe.Schema.Notation

  defp secret_key(len \\ 32) do
    Application.get_env(:my_app, MyAppWeb.Endpoint)
    |> Keyword.get(:secret_key_base, "")
    |> String.slice(0, len)
  end

  defp pad_bytes(binary, block \\ 16) do
    padding_bits =
      case rem(byte_size(binary), block) do
        0 -> 0
        r -> (block - r) * 8
      end

    <<0::size(padding_bits)>> <> binary
  end

  defp unpad_bytes(<<0, tail::bitstring>>), do: unpad_bytes(tail)
  defp unpad_bytes(binary), do: binary

  defp encrypt(raw_binary) do
    padded_binary = pad_bytes(raw_binary)

    :crypto.crypto_one_time(:aes_256_ecb, secret_key(), padded_binary, true)
  end

  defp decrypt(raw_enc) do
    :crypto.crypto_one_time(:aes_256_ecb, secret_key(), raw_enc, false)
    |> unpad_bytes()
    |> :erlang.binary_to_term()
  end

  def serialize(id) do
    id
    |> :erlang.term_to_binary()
    |> encrypt()
    |> Base.url_encode64(padding: false)
  end

  def parse(%{value: enc_id}) do
    try do
      {:ok, raw_enc} = Base.url_decode64(enc_id, padding: false)
      {:ok, decrypt(raw_enc)}
    rescue
      _ -> :error
    end
  end

  scalar :enc_id, name: "EncId" do
    serialize(&__MODULE__.serialize/1)

    parse(&__MODULE__.parse/1)
  end
end

I don't understand why encryption would be "hard" - the code above looks straightforward enough although I'm not sure you need the padding (never tried the new crypto APIs).

Do note that if you want to secure your UUIDs (why?), you probably want to add a random IV to your UUIDs otherwise you'll probably expose too much information - UUIDs have some bits very static. Generally speaking, rolling your own crypto protocol is fraught with error; I would use an existing library like Hashids (https://hexdocs.pm/hashids/Hashids.html) because that protocol has been analyzed and you're probably better off with the mild known weaknesses in such a library than some huge unknown weakness lurking in roll-your-own :).

But for all practical purposes, generating v4 UUIDs using a cryptographically strong RNG should be all you need.


Data Stream as Disjoint intervals I was asked the following question:

  Given a data stream input of non-negative integers a1, a2, ..., an,
  ..., summarize the numbers seen so far as a list of disjoint
  intervals.

  For example, suppose the integers from the data stream are 1, 3, 7, 2,
  6, ..., then the summary will be:

[1, 1]
[1, 1], [3, 3]
[1, 1], [3, 3], [7, 7]
[1, 3], [7, 7]
[1, 3], [6, 7]

My approach to this problem was to keep adding the numbers to a structure such as a TreeSet which will help with the in-order iteration. While iterating I can check whether the values are consecutive and if they are, I add them to the existing interval, otherwise I create a new interval. Here's the code I have to accomplish it:

public class SummaryRanges {
    private TreeSet<Integer> streamNums;
    /** Initialize your data structure here. */
    public SummaryRanges() {
        streamNums= new TreeSet<Integer>();
    }

    public void addNum(int val) {
        if(!streamNums.contains(val)) {
            streamNums.add(val);
        }
    }

    public List<Interval> getIntervals() {
        ArrayList<Interval> buffer = new ArrayList<Interval>();
        if(!streamNums.isEmpty()) {
            int start = streamNums.first();
            int end = start;
            Iterator<Integer> streamIter = streamNums.iterator();
            if(streamIter.hasNext()) streamIter.next();
            while(streamIter.hasNext()) {
                int i = streamIter.next();
                if(i == end + 1) {
                    end = i;
                } else {
                    buffer.add(new Interval(start,end));
                    start = i;
                    end = i;
                }
            }
          buffer.add(new Interval(start,end));
        }
        return buffer;
    }
}

It will be great to get some pointers on how to do this better.

Using a TreeSet effectively

Since you are already using a TreeSet as the underlying data structure, you need not check if it contains the incoming number val or not in addNum(int) as any Set implementation will not store duplicate values. Also, to skip the first element in the TreeSet, you can do:

int start = streamNums.first();
Iterator<Integer> streamIter = streamNums.tailSet(start, false).iterator();

This avoids having to next() on the Iterator manually.

Inverting ifs

public List<Interval> getIntervals() {
    ArrayList<Interval> buffer = new ArrayList<Interval>();
    if(!streamNums.isEmpty()) {
        // ...
    }
    return buffer;
}

You can reduce one level of nesting if you simply returned a new ArrayList first:

public List<Interval> getIntervals() {
    if(streamNums.isEmpty()) {
        return new ArrayList<>();
    }
    List<Interval> buffer = new ArrayList<>();
    // ...
    return buffer;
}

Interfaces over implementations and type inference

As illustrated above, buffer can be declared as a List instead of ArrayList, and at least you are not making the same mistake in the method return type declaration. Also, since Java 7, you can rely on type inference to eliminate mentioning the generic type a second time, by using the 'diamond operator' <>.

Java 8 stream-based processing

Since you are on Java 8, you can look towards its stream-based processing capabilities as the appropriate way of solving your problem. It's relatively simple to adapt your solution to reduce an integer stream to a List<Interval>, or more specifically to create a Collector out of your solution:

public class TreeInterval implements Collector<Integer, TreeInterval, List<Interval>> {

    private final TreeSet<Integer> cache = new TreeSet<>();

    @Override
    public BiConsumer<TreeInterval, Integer> accumulator() {
        return (ti, integer) -> ti.cache.add(integer);
    }

    @Override
    public Set<Characteristics> characteristics() {
        return Collections.singleton(Characteristics.UNORDERED);
    }

    @Override
    public BinaryOperator<TreeInterval> combiner() {
        return (a, b) -> { a.cache.addAll(b.cache); return a; };
    }

    private List<Interval> toResult() {
        if (cache.isEmpty()) {
            return new ArrayList<>();
        }
        List<Interval> buffer = new ArrayList<>();
        int start = cache.first();
        int end = start;
        Iterator<Integer> iterator = cache.tailSet(start, false).iterator();
        while (iterator.hasNext()) {
            int i = iterator.next();
            if (i == end + 1) {
                end = i;
            } else {
                buffer.add(new Interval(start, end));
                start = i;
                end = i;
            }
        }
        buffer.add(new Interval(start, end));
        return buffer;
    }

    @Override
    public Function<TreeInterval, List<Interval>> finisher() {
        return TreeInterval::toResult;
    }

    @Override
    public Supplier<TreeInterval> supplier() {
        return TreeInterval::new;
    }
}

In summary, a Collector needs to know how to:

supply an accumulator: represented by the supplier() method.
accumulate an Integer: represented by the accumulator() method.
combine the accumulators into one: represented by the combiner() method.
extract the results of the last accumulator to the required type: represented by the finisher() method. In turn, your solution is used as a method reference within this.
provide a set of characteristics that describes itself, for potential optimization: represented by the characteristics() method.

For illustration, this is how it can be used:

public static void main(String[] args) {
    IntStream.of(1, 3, 7, 2, 6)
        .boxed()
        .collect(new TreeInterval())
        .forEach(System.out::println);
}

// Output
[1,3]
[6,7]

Construct an IntStream with the values 1, 3, 7, 2, 6.
Box it into a Stream<Integer>.
Collect the values using an instance of TreeInterval.
For each Interval, print it using System.out.println(Object).


Vector calculation in Rust Hi I am trying to get into Rust by implementing a small library for vector calculations.

I am mainly asking to point out whether I chose a valid approach. I also am interested in comments on the overall details I might have missed or got the wrong way.

So for my Vec3 implementation I basically declared a custom type which is an array of length 3. Afterwards I am simply implementing Traits for this type. For now I only implemented it for f32.

pub type Vec3<T = f32> = [T; 3];

pub trait Vector3<T> {
    /// Creates a new Vec3 with its values initialized to `[0.0, 0.0, 0.0]`.
    fn new() -> Self;

    /// Creates a new `Vec3` based on the given `x`, `y` and `z` values.
    fn from_values(x: f32, y: f32, z: f32) -> Vec3<T>;

    /// Calculates the scalar dot product of two `Vec3`'s.
    fn dot(a: Vec3<T>, b: Vec3<T>) -> f32;

    /// Performs multiplication between two `Vec3`.
    fn multiply(self, a: Vec3) -> Vec3;

    /// Calculates the sum of two `Vec3` components.Vec3
    fn add(self, a: Vec3) -> Vec3;

    /// Scales a `Vec3` by a scalar value.
    fn scale(self, x: f32) -> Vec3;
}

impl Vector3<f32> for Vec3<f32> {
    /// Creates a new Vec3 with its values initialized to `[0.0, 0.0, 0.0]`.
    ///
    /// ```
    /// use glMatrix_rs::vec3::*;
    ///
    /// let result = Vec3::new();
    /// assert_eq!([0.0, 0.0, 0.0], result);
    /// ```
    fn new() -> Vec3<f32> {
        [0.0, 0.0, 0.0]
    }

    /// Creates a new `Vec3` based on the given `x`, `y` and `z` values.
    ///
    /// ### Arguments
    ///
    /// * `x` - The first vector component.
    /// * `y` - The second vector component.
    /// * `z` - The third vector component.
    ///
    /// ```
    /// use glMatrix_rs::vec3::*;
    ///
    /// let result = Vec3::from_values(0.0, 1.0, 2.0);
    /// assert_eq!([0.0, 1.0, 2.0], result);
    /// ```
    fn from_values(x: f32, y: f32, z: f32) -> Vec3 {
        [x, y, z]
    }

    /// Calculates the scalar dot product of two `Vec3`'s.
    ///
    /// ### Arguments
    ///
    /// * `a` - The first vector for `dot` calculation.
    /// * `b` - The second  vector for `dot` calculation.
    ///
    /// ```
    /// use glMatrix_rs::vec3::*;
    /// let a = Vec3::from_values(2.0, 2.0, 2.0);
    /// let b = Vec3::from_values(2.0, 2.0, 2.0);
    /// assert_eq!(12.0, Vec3::dot(a, b));
    /// ```
    fn dot(a: Vec3, b: Vec3) -> f32 {
        a[0] * b[0] + a[1] * b[1] + a[2] * b[2]
    }

    /// Performs multiplication between two `Vec3`.
    ///
    /// ### Arguments
    ///
    /// * `a` - Vector by which `self` will be multiplied.
    ///
    /// ```
    /// use glMatrix_rs::vec3::*;
    /// let mut out = Vec3::new();
    /// let a = Vec3::from_values(1.0, 2.0, 3.0);
    /// let b = Vec3::from_values(2.0, 2.0, 2.0);
    /// assert_eq!([2.0, 4.0, 6.0], a.multiply(b));
    /// ```
    fn multiply(self, a: Vec3) -> Vec3 {
        [self[0] * a[0], self[1] * a[1], self[2] * a[2]]
    }

    /// Calculates the sum of two `Vec3` components.Vec3
    ///
    /// ### Arguments
    ///
    /// * `a` - Vector which will be added to `self`.
    ///
    /// ```
    /// use glMatrix_rs::vec3::*;
    /// let x = Vec3::from_values(1.0, 2.0, 3.0);
    /// let a = Vec3::from_values(3.0, 2.0, 1.0);
    /// assert_eq!([4.0, 4.0, 4.0], x.add(a));
    /// ```
    fn add(self, a: Vec3) -> Vec3 {
        [self[0] + a[0], self[1] + a[1], self[2] + a[2]]
    }

    /// Scales a `Vec3` by a scalar value.
    ///
    /// ### Arguments
    ///
    /// * `x` - Scalar value by which the vector will be scaled.
    ///
    /// ```
    /// use glMatrix_rs::vec3::*;
    /// let a = Vec3::from_values(1.0, 2.0, 3.0);
    /// assert_eq!([2.0, 4.0, 6.0], a.scale(2.0));
    /// ```
    fn scale(self, x: f32) -> Vec3 {
        [self[0] * x, self[1] * x, self[2] * x]
    }
}

You can clone, build and test it any time from my GitHub repository.

My first recommendation is to use std::ops. Try writing implementations of ops::Add for addition,  ops::Mul<Vec3<T>> for cross product, and ops::Mul<T> for scaler-vector multiplication. Not only does implementing these standard traits make your code more interoperable with other Rust code, but it also gives you operator overloading, so you can use + and * on your vectors.

Second, I would question the user of the trait Vector3<f32>. Why do you need a trait? I would just put your operations in the implementation of Vec3<T> itself

impl <T> Vec3<T> {

// ... methods ...

}

Looking forward to seeing your completed code. This will even more interesting once you get generics working.


LeetCode: Divide Array in Sets of K Consecutive Numbers C# https://leetcode.com/problems/divide-array-in-sets-of-k-consecutive-numbers/

  Given an array of integers nums and a positive integer k, find whether
  it's possible to divide this array into sets of k consecutive numbers
  Return True if its possible otherwise return False.

Example 1:

Input: nums = [1,2,3,3,4,4,5,6], k = 4
Output: true
Explanation: Array can be divided into [1,2,3,4] and [3,4,5,6].

Example 2:

Input: nums = [3,2,1,2,3,4,3,4,5,9,10,11], k = 3
Output: true
Explanation: Array can be divided into [1,2,3] , [2,3,4] , [3,4,5] and [9,10,11].

Example 3:

Input: nums = [3,3,2,2,1,1], k = 3
Output: true

Example 4:

Input: nums = [1,2,3,4], k = 3
Output: false
Explanation: Each array should be divided in subarrays of size 3.

Constraints:

1 <= nums.length <= 10^5
1 <= nums[i] <= 10^9
1 <= k <= nums.length

Please review for performance.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/divide-array-in-sets-of-k-consecutive-numbers/
    /// </summary>
    [TestClass]
    public class DivideArrayinSetsofKConsecutiveNumbersTest
    {
        [TestMethod]
        public void TestExample()
        {
            int[] nums = {1, 2, 3, 3, 4, 4, 5, 6};
            int k = 4;
            Assert.IsTrue(DivideArrayinSetsofKConsecutiveNumbers.IsPossibleDivide(nums, k));
        }

        [TestMethod]
        public void TestFailed()
        {
            int[] nums = { 1, 2, 3,4};
            int k = 3;
            Assert.IsFalse(DivideArrayinSetsofKConsecutiveNumbers.IsPossibleDivide(nums, k));
        }
    }

    public class DivideArrayinSetsofKConsecutiveNumbers
    {
        public static bool IsPossibleDivide(int[] nums, int k)
        {
            if (nums.Length % k != 0)
            {
                return false;
            }

            var dict = new SortedDictionary<int, int>();
            foreach (var num in nums)
            {
                if (!dict.TryGetValue(num, out var value))
                {
                    value = dict[num] = 0;
                }

                dict[num] = value+1;
            }

            for (int i = 0; i < nums.Length / k; i++)
            {
                int start = dict.FirstOrDefault(x=>x.Value >0).Key;
                dict[start]--;
                for (int j = 1; j < k; j++)
                {
                    if (!dict.ContainsKey(start + j) || dict[start + j] < 1)
                    {
                        return false;
                    }

                    dict[start + j]--;
                }
            }

            return true;
        }
    }
}

Here's some C# in accordance with my first suggestion of sorting the array and looping.

public bool IsPossibleDivide(int[] nums, int k)
{
    if (nums.Length % k != 0)
    {
        return false;
    }

    var dict = new Dictionary<int, int>();
    foreach (var num in nums)
    {
        if (!dict.TryGetValue(num, out var value))
        {
            value = dict[num] = 0;
        }

        dict[num] = value+1;
    }
    Array.Sort(nums);

    for (int i = 0; i < nums.Length; i++)
    {
        var currVal = nums[i];
        if(dict[currVal] > 0)
        {
            for(int j = 0; j < k; j++)
            {
                if(!dict.ContainsKey(currVal + j))
                {
                    return false;
                }
                dict[currVal + j]--;
            }
        }
    }

    return true;
}

This is significantly faster, and is O(nlogn), as I say I think there's a O(n) solution but I'm not really sure of the exact details and I don't know much C#, probably I could write it in JS if you like.

As I say, this

int start = dict.FirstOrDefault(x=>x.Value >0).Key;

looks bad to me because once you've done a good number of the consecutive numbers, it might start having to look through a lot of the dict.


Integer to Roman Numeral conversion As a little standard exercise, I wrote an integer to roman numerals converter in F03. The result looks already neat enough, for my standards, but I wonder whether it could be made snappier without sacrificing clarity. For folks not familiar with the simple syntax of modern Fortran, I suggest the following quick modern Fortran tutorial.
module mRomanStrings

  implicit none
  private
  character(len=*), parameter :: RM_CHARS  = "IVXLCDMvxlcdm_"
  integer         , parameter :: MAX_ORDER = len(RM_CHARS)/2-1
  character(len=4)            :: ROMAN_STRING(0:9,0:MAX_ORDER)
  logical                     :: mRomanStringsIsNotInitialized = .TRUE.
  public :: IntegerToRoman

contains

  function IntegerToRoman(i) result(rm)
    integer         , intent(in)  :: i
    character(len=:), allocatable :: rm
    integer :: n
    if( mRomanStringsIsNotInitialized ) call mRomanSring_init()
    rm = " "
    do n = min(MAX_ORDER,int(log(dble(i))/log(10.d0)+1.d-10)), 0, -1
       rm = trim(rm)//trim(ROMAN_STRING(mod(i/10**n,10),n))
    enddo
  end function IntegerToRoman

  subroutine mRomanSring_init()
    character :: a0, a1, a2, a3
    integer :: n
    do n = 0, MAX_ORDER
       a0 = " "
       a1 = RM_CHARS(2*n+1:2*n+1)
       a2 = RM_CHARS(2*n+2:2*n+2)
       a3 = RM_CHARS(2*n+3:2*n+3)
       ROMAN_STRING(0:9,n) = [&
            a0//a0//a0//a0, a1//a0//a0//a0, a1//a1//a0//a0, a1//a1//a1//a0, a1//a2//a0//a0, &
            a2//a0//a0//a0, a2//a1//a0//a0, a2//a1//a1//a0, a2//a1//a1//a1, a1//a3//a0//a0 ]
    enddo
    mRomanStringsIsNotInitialized = .FALSE.
  end subroutine mRomanSring_init

end module mRomanStrings

program ConvertIntegersToRoman
  use, intrinsic :: iso_fortran_env
  use mRomanStrings
  implicit none
  integer :: i
  do i=1,10000
     write(OUTPUT_UNIT,"(i6,x,a)") i, IntegerToRoman(i)
  enddo
end program ConvertIntegersToRoman

It is overall good code, but there are of course some points to improve.
Objectively wrong and severe:

Always compile with as much debug options as possible with your compiler. It detects errors such as
2 * MAX_ORDER + 3 == 2 * (size(RM_CHARS) / 2 - 1) + 3 > size(RM_CHARS)

in your initialization loop.

The function expects a number that is larger than zero and smaller than 10^6. This is nowhere documented nor checked.

Small things that could be made better:

There are unnecessary many trim calls when you build up the string. If you only append trimmed strings, you know that the result is trimmed.

There is log10 for the decadic logarithm.

It helps sometimes to introduce additional variable names for self documentation.

I would recommend to use the words magnitude, exponent, and mantissa in the code. This is more self-explaining than "order".

It is not necessary to start from Zero for the first dimension of ROMAN_STRING

I would never name a logical variable with its negated form. You can always use .not. and it quickly becomes hard to read if you doubly negate it. The name could be shorter in addition.
mRomanStringsIsNotInitialized == .not. mRomanStringInitialized
.not. mRomanStringsIsNotInitialized == mRomanStringInitialized

I would use floor instead of int. It is a common error to use int instead of nint so it is nice to explicitly say floor.

Architecture:
At the moment you have runtime checks in your function to ensure initialization of ROMAN_STRING. This is not necessary since ROMAN_STRING could be made a compile time constant (with parameter).
If you do this then your function does not depend anymore on a global variable that can be mutated at run time, can be made pure and is probably a tiny bit faster.
Opinion based:
I would add more whitespace around your operators and after commata and use an indentation of at least 4 spaces.
You can format Fortran code pretty much according to the PEP8 guidelines of python, to achieve convincing results. (Especially since a lot of people in scientific computing use both languages.)
Everything together leads to:
  module mRomanStrings

    implicit none
    private
    integer         , parameter :: MAX_MAGNITUDE = 5
    character(len=4), parameter :: ROMAN_STRING(9, 0 : MAX_MAGNITUDE) = &
          reshape([character(4) :: &
          'I', 'II', 'III', 'IV', 'V', 'VI', 'VII', 'VIII', 'IX', &
          'X', 'XX', 'XXX', 'XL', 'L', 'LX', 'LXX', 'LXXX', 'XC', &
          'C', 'CC', 'CCC', 'CD', 'D', 'DC', 'DCC', 'DCCC', 'CM', &
          'M', 'MM', 'MMM', 'Mv', 'v', 'vM', 'vMM', 'vMMM', 'Mx', &
          'x', 'xx', 'xxx', 'xl', 'l', 'lx', 'lxx', 'lxxx', 'xc', &
          'c', 'cc', 'ccc', 'cd', 'd', 'dc', 'dcc', 'dccc', 'cm'], &
          [9, MAX_MAGNITUDE + 1])
    public :: IntegerToRoman

  contains

      !> Expects an integer 1 <= n <= (10^6 - 1) and returns
      !>  the Roman number literal as string.
      pure function IntegerToRoman(n) result(rm)
          integer, intent(in) :: n
          character(len=:), allocatable :: rm
          integer :: largest_magnitude, mag, mantissa
          if (n <= 0) error stop

          largest_magnitude = floor(log10(dble(n)))
          if (largest_magnitude > MAX_MAGNITUDE) error stop

          rm = ""
          do mag = largest_magnitude, 0, -1
              mantissa = mod(n / 10**mag, 10)
              if (mantissa > 0) then
                  rm = rm // trim(ROMAN_STRING(mantissa, mag))
              end if
          end do
      end function IntegerToRoman

  end module mRomanStrings

  program ConvertIntegersToRoman
      use, intrinsic :: iso_fortran_env, only: OUTPUT_UNIT
      use mRomanStrings
      implicit none
      integer :: i
      do i = 1, 99999
          write(OUTPUT_UNIT,"(i6,x,a)") I, IntegerToRoman(i)
      end do
  end program ConvertIntegersToRoman


Identifying numeric substrings of an alphanumeric string, and summing them I am going through the CodingBat exercises for Java. Here is the task I have just completed:

  Given a string, return the sum of the numbers appearing in the string, ignoring all other characters. A number is a series of 1 or more digit chars in a row.

And here is my code:

public int sumNumbers(String str){
    int total = 0;

    for (int i = 0; i < str.length(); i++) {
        if (Character.isDigit(str.charAt(i))) {
            StringBuilder appendNums = new StringBuilder();
            appendNums.append(str.charAt(i));
            for (int j = i+1; j < str.length(); j++) {
                if (Character.isDigit(str.charAt(j))) {
                    appendNums.append(str.charAt(j));
                } else {
                    break;
                }
            }
            String appendNums2String = appendNums.toString();
            total += Integer.parseInt(appendNums2String);
            i += appendNums2String.length()-1;
        }
    }
    return total;
}

The questions I have are:

Are there any parts of this code you find to be poor practise or
poor quality?
Is it acceptable to append one digit before the (conditional) digits that proceed it, or should I be identifying the entire block
first, and then append?
What is a more concise way of solving this?
For this question I relied heavily on Eclipse's debugger until I got it right. Is this good or bad practise?
Is it a better idea to use a while loop instead of the second for loop, because the characters tested are relative to i rather than the string's length? (I played around with this but couldn't figure it out.)

Learning to use, and using the debugger are fantastic ideas. It is a valuable skill that will serve you well. Understanding how your code works (and breaks) is very valuable, and the debugger helps you through there.

The remainder of your specific questions are somewhat linked to your actual implementation, and I am going to suggest that your implementation would be simpler with a String split operation. The specification says:

  Given a string, return the sum of the numbers appearing in the string,
  ignoring all other characters. A number is a series of 1 or more digit
  chars in a row.

Taking this literally, I would code this up as a Regular expresison system matching digits. Something like:

private static final Pattern NUMBERS = Pattern.compile("[0-9]+");

private static int digitSum(String s) {
    Matcher matcher = NUMBERS.matcher(s);
    int sum = 0;
    while (matcher.find()) {
        sum += Integer.parseInt(matcher.group());
    }
    return sum;
}

The Pattern says "Look in the string for a sequence of digits".

The method loops through all digit sequences, converts them to integers, and totals them.


Minimising the triangle A triangle needs a good foundation. Every row in the triangle is derived from the sum of the two values below it. However, there can be no repeated values, if a value shows up more than once the triangle crumbles. Find the base which minimises the value in the top of the triangle satisfying the condition of no duplicates.

Source

Example:

      20
    8   12
 [3]   5   7
1   2  [3]  4

Here 3 occurs twice, so the triangle is invalid

The base \$n\$ can be of any size \$n > 0\$ with

solve(1) = 1, solve(2) = [1, 2] = 3, solve(3) = [2, 1, 4] = 8

I would like suggestions on how I can improve:

The code style or best practices, so the code is easy to read.
The algorithm/constraints, I'm pretty sure that the loops can be made tighter, so not as many numbers have to be checked, but I can't figure out what the cut off is.
Is there a better way to implement the base size without hard-coding extra nested loops, as this doesn't seem very scalable?

import java.util.*;

public class spaceship {
    public static void main(String[] args) {
        long start = System.nanoTime();
        spaceship kling = new spaceship();
        kling.solveAll();
        System.out.println("Took: " + (System.nanoTime() - start) / 1000000 + "ms");
    }

    public void solveAll() {
        int len = 6;
        int bestFound = 400; //definitely big enough from previous tests
        int res = bestFound;
        int[] best = new int[len];
        int[] x = new int[len];
        int[] co = new int[] { 1, 1, 1, 1, 1, 1 };

        for (int a = 1; co[0] * a < bestFound; a++) {
            long beg = System.nanoTime();
            for (int b = 1; co[1] * b < bestFound; b++) {

                for (int c = 1; co[2] * c < bestFound; c++) {
                    for (int d = 1; co[3] * d < bestFound; d++) {
                        //for (int e = 1; co[4] * e < bestFound; e++) {
                            //for (int f = 1; co[5] * f < bestFound; f++) {

                                x = new int[] { a, b, c, d,/* e, f */};
                                res = solve(x);

                                if (res != -1 && res < bestFound) {
                                    System.out.print(Arrays.toString(x));
                                    System.out.println(":\t" + res);
                                    bestFound = res;
                                    best = x;
                                }
                            //}
                        //}
                    }
                }
            }
            System.out.println((System.nanoTime() - beg) / 1000000 + "ms");
        }

        System.out.println("The best result possible is:");
        System.out.println(bestFound);
        System.out.println(Arrays.toString(best));

    }

    public int solve(int[] in) {
        Set<Integer> data = new HashSet<>();
        // check input for duplicates
        // could be done in loops
        for (int a : in) {
            if (data.contains(a))
                return -1;
            data.add(a);
        }

        int size = in.length;
        int[][] arr = new int[size][size];
        arr[0] = in;
        // first row is the input
        // every row after is the sum of the two elements below it
        // ie pascal's triangle with outermost values ignored

        for (int i = 1; i < size; i++) {
            for (int j = 0; j < size - i; j++) {
                int a = arr[i - 1][j] + arr[i - 1][j + 1];
                if (data.contains(a))
                    return -1;
                data.add(a);
                arr[i][j] = a;
            }
        }

        // return final value, our result if no duplicates occur during the
        // process
        return arr[size - 1][0];
    }
}

Style

In Java it is common practice to have class names to use PascalCase (a.k.a. UpperCamelCase). Also the name spaceship doesn't make sense to me and is confusing. Also, why is the instance called kling?

Readability

I prefer to prefix my formal parameters with a for argument, for example aInputArray. I believe it would help readability of your code.

Please prefer descriptive names. Currently I have no idea what x, co and res are.

Nested for loops

You are basically implementing a multi-byte counter. You can do something like this (not tested, but you get the idea):

boolean incr(int[] co, int max, int min, int pos){
     if(co[pos] + 1 > max){
         co[pos] = min;
         if(pos == 0)
              return false;
         return incr(co, max, min, pos - 1);
     }
     else{
         co[pos]++;
         return true;
     }
}

void solveAll(){

    int[] co = new int[len];
    while(incr(co, bestFound, 1, co.length - 1)){
       res = solve(co);
    }
}

Performance

Lets look at solve(int[] in). You're using HashSet which is fast as long as you don't get excessive collisions. Seeing as you are dealing with many numbers that are possibly small, you could be having many hash collisions. If profiling indicates this is the case, you can try a better hash function (for example multiply by a large prime number).

Here:

int[][] arr = new int[size][size];

you allocate space for the whole triangle but you only ever use the last two rows. So you only need to store two rows and alternate between them. This has the added benefit that both rows will be hot in cache all the time, avoiding cache misses as you go from line to line. It will also avoid some extra indirections in your inner most loop.

Better algorithm

The first thing we need to realize is that the problem is kind of recursive (I can't remember the proper term off the top of my head).

What I mean by that is that if a base {a,b,c} for n=3 crumbles, then the all bases {a,b,c,x} for any x will also crumble. This means that by solving the easier problem n=3 we get information about a lot of combinations for n=4 that will crumble.  (1)

The second thing to realize about this problem is that if {a,b,c} crumbles, then {c,b,a} will also crumble. And if {a,b,c} doesn't crumble and has top value x then {c,b,a} will also not crumble and have the same top value x. (2)

The third thing is that for the base {a,b,c} the top value will be strictly larger than a+b+c. This is a useful stopping condition. (3)

Knowing the above, you have to unrestrainedly search for the combination of n base elements that produces the smallest top value as result. Use a greedy search for the smallest top value and terminate when all the nodes remaining fulfil (3).

The difficult part is to generate the nodes to be searched. This can be done effectively by starting with small n and then combinatorially incrementing the base elements and adding an additional base element to create triangles with n+1 base for any triangle that doesn't crumble. This exploits (1) to prune large sets of crumbling triangles from the start.

One should also take care to not generate symmetry pairs to triangles that have already been tried to exploit (2).

It is useful to store the base and right edge of the triangle in addition to a  HashSet of all the values in the triangle.

I.e. for the triangle:

      20
    8   12
  3   5   7
1   2   3   4

store:

1   2   3   4 (base row)

and

4   7   12   20 (right edge, bottom -> up).

Using the right edge, we can quickly calculate any expansion of this triangle. For example:

1   2   3   4   10

is easily calculated as:

       53
      20 33
        12 21
          7  14
            4  10

if we have the right edge.

I hope this helps. It's an interesting problem.


Repeater for blocks of Ruby I've written a module for repeating blocks of code, generally to cover issues related to eventual consistency and testing screen elements which may take some time to fully load. It looks like this:

class Repeater
  def initialize(&block)
    @repeat_block = block
  end

  def repeat(times: 25, delay: 0.2)
    result = nil
    times.times do
      result = @repeat_block.call
      break if @until_block.present? && @until_block.call
      sleep(delay)
    end
    result
  end

  def until(&block)
    @until_block = block
    self
  end
end

The Repeater takes two blocks of code, one to be run and the second to check for an exit criteria. Optionally the number of repeats and delays can be overwritten after the .repeat method.

This is implemented in many places throughout the codebase, an example of such is:

  result = false
  Repeater.new do
    result = event_present_in_database
  end.until do
    result
  end.repeat(times: 10, delay: 0.1)
  result

In this case the event_present_in_database method is being repeated called until it returns true instead of false, which indicates that the event has been created as expected, or else times out indicating there's an issue.

This certainly works for what I want it to do, however it doesn't seem very neat and I'm sure it could be tidied up. I know Ruby has all sorts of tricks that I've yet to come across, is there a way to leverage these to make this nicer to use/easier to read.

There's not much weak points I can see.

The biggest one is the way to use it. One needs to declare a variable to be caught in the block (closure), and I think it's unnecessary. You should be able to do this:

  Repeater.new do
    event_present_in_database
  end.until do |result|
    result
  end.repeat(times: 10, delay: 0.1)

If you changed the break like like this:

  break if @until_block.present? && @until_block.call(result)

The second, tiny point would be: make it clearer to understand the extraction to method conditionals:

  # ...
  break if exit_criteria_satisfied?(result)
  # ...

  private
  def exit_criteria_satisfied?(result)
    @until_block.present? && @until_block.call
  end

That's all I got. The rest of the code seems pretty clear to me.

  I know Ruby has all sorts of tricks

Yeah, but I don't know a single one that could help with the readability here.


Displaying "tooltips" in Highcharts I have the following piece of code:

for (var i = 0, len = legend.allItems.length; i < len; i++) {
    (function (i) {
        var item = legend.allItems[i].legendItem;

        item.on('mouseover', function (e) {
            var posLeft = 0,
            posTop = 0,
            parentWidth = chart.chartWidth;

            var html = '' +
                MetricsTooltips.get(chart.series[i].options.tooltipKey) +
            '';

            labelElement = chart.renderer.label(html, posLeft, posTop, 'callout', null, null, true, false, 'legend-tooltip').attr({zIndex: 10}).add();

            if(legend.group.alignOptions.align == 'center') {
                // _legendItemPos[0] returns the offset (left) for current legend item
                posLeft = legend.group.translateX + legend.allItems[i]._legendItemPos[0];
                // Subtract some "offset" as we don't want tooltips to overlap the legend
                posTop = legend.group.translateY - 30;

                // Make sure that tooltip won't overflow its parent
                if ((legend.allItems[i]._legendItemPos[0] + labelElement.width) > parentWidth) {
                    posLeft = parentWidth - labelElement.width;
                }

                labelElement.destroy();
            } else if (legend.group.alignOptions.align == 'right') {
                posTop = (legend.group.translateY + legend.allItems[i]._legendItemPos[1]);
                posLeft = (parentWidth - legend.group.alignOptions.width) - labelElement.width;

                if(legend.scrollGroup.translateY && legend.currentPage > 1) {
                    // Handle the case when legend group have too many metrics and paging appears in
                    posTop -= Math.abs(legend.scrollGroup.translateY);
                }

                labelElement.destroy();
            } else {
                // handle this if needed
            }

            labelElement = chart.renderer.label(html, posLeft, posTop, 'callout', null, null, true, false, 'legend-tooltip').css({zIndex: 10}).add();

        }).on('mouseout', function (e) {
            labelElement.destroy();
        });
    })(i);
}

Basically, it attach event listeners on all legend items when chart is loaded (the for is located inside onload callback), and later, when mouseenter event occurs, we display simple "tooltip" to the user.

It's working very well, but I'm looking for some refactoring hints.

My observations thus far:

You are not actually using mouseenter in your code, you should as that would be the better approach
The function inside a loop with the double var assignment is awkward, if you want to go functional just use .forEach()
The way you have // handle this if needed, the reader is not sure if that can happen. If it can happen, then the code will create 2 times labelElement since you did not call labelElement.destroy(); in that else branch. If it cannot happen, remove the code. Also if it cannot happen, place labelElement.destroy(); after the if statement
labelElement seems to be a global, I hope it is created with a var statement elsewhere in your code
From a naming perspective (not sure this is your naming), I have no idea what scrollGroup would return, and even less of an idea now that I see that that object has a method called translateY. You might want to re-consider carefully some of the naming.


Using a thread to update a table automatically in Django For my portfolio app using Django I made a thread that pulls out the usd exchange rates for various currencies every 6 hours and updates the database. The program kicks in as soon as the module module_stock.py is being imported.

Other than to make  the thread to start in a function call, should I be aware of any disadvantages if this runs in production environment for days, weeks on end? Or in other words is it ok to have this thread running at all time ?

Relevant pieces of code below.

module_stock.py:

import threading

UPDATE_INTERVAL = 21600

class WorldTradingData:
    ''' methods to handle trading data
        website: https://www.worldtradingdata.com
    '''
    @classmethod
    def setup(cls,):
        cls.api_token = config('API_token')
        cls.stock_url = 'https://api.worldtradingdata.com/api/v1/stock'
        cls.intraday_url = 'https://intraday.worldtradingdata.com/api/v1/intraday'
        cls.history_url = 'https://api.worldtradingdata.com/api/v1/history'
        cls.forex_url = 'https://api.worldtradingdata.com/api/v1/forex'

    @classmethod
    def update_currencies(cls):
        base_currency = 'USD'
        url = ''.join([cls.forex_url,
                       '?base=' + base_currency,
                       '&api_token=' + cls.api_token,
                       ])
        try:
            res = requests.get(url)
            forex_dict = json.loads(res.content).get('data', {})

        except requests.exceptions.ConnectionError:
            forex_dict = {}
            logger.info(f'connection error: {url}')

        for cur in Currency.objects.all().order_by('currency'):
            currency_key = cur.currency
            currency_object = Currency.objects.get(currency=currency_key)
            usd_exchange_rate = forex_dict.get(currency_key, '')

            if usd_exchange_rate:
                currency_object.usd_exchange_rate = usd_exchange_rate
                currency_object.save()

def update_currencies_at_interval(interval=UPDATE_INTERVAL):
    '''  update the currency depending on interval in seconds
         default value is 6 hours (21600 seconds)
    '''
    assert isinstance(interval, int), f'check interval setting {interval} must be an integer'

    wtd = WorldTradingData()
    wtd.setup()
    start_time = time.time()
    current_time = start_time
    elapsed_time = int(current_time - start_time)

    while True:
        if elapsed_time % interval == 0:
            wtd.update_currencies()

        current_time = time.time()
        elapsed_time = int(current_time - start_time)

thread_update_currencies = threading.Thread(
    target=update_currencies_at_interval, kwargs={'interval': UPDATE_INTERVAL})
thread_update_currencies.start()

models.py:

from django.db import models

class Currency(models.Model):
    currency = models.CharField(max_length=3, unique=True)
    usd_exchange_rate = models.CharField(max_length=20, default='1.0')

    def __str__(self):
        return self.currency
```

Keep a base URL

'https://api.worldtradingdata.com/api/v1/' should be factored out of your URL variables. You can use the Python libraries in urllib to safely construct the URLs after, which is preferable to using join as you do.

Don't manually construct a URL

    url = ''.join([cls.forex_url,
                   '?base=' + base_currency,
                   '&api_token=' + cls.api_token,
                   ])

This is more work than you have to do. Just pass a dict to requests.get's params kwarg.

Don't call json.loads()

        res = requests.get(url)
        forex_dict = json.loads(res.content).get('data', {})

Just call res.json().

Check your HTTP result

Call res.raise_for_status(), or at the absolute least check the status of the result. Currently, there are some failure modes that will not throw from your code at all.


Simple code that finds number of factors crashes my computer. Is there a workaround for a large number? This is my attempt at problem 12 from Project Euler. However, my code freezes my old, linux computer when I run it. I've only got 512MB of RAM and an old Intel Core 2 Duo processor on my machine which, I assume, is why it can't run the code.

Is there a way to optimize this code without using up too much resources?

#!/bin/python

def nth_triangle_number(nth):
    """ Sequence of triangle numbers is generated by adding the natural numbers.
        So the 7th triangle number would be 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28. """
    return sum([i for i in range(nth+1)])

def get_number_of_factors(num):
    quantity = 0
    for i in range(1, int(num/2)+1):     # I think the problem lies here in range()
        if not num % i:                  # BTW, using math.sqrt() produces the wrong
            quantity += 1                # answer for smaller numbers. I don't know why.
    return quantity + 1     # Added 1 to count for num (as in 1 * num)

def get_triangle_number(divisors):
    """ Find the triangle number with the given amount of divisors """
    i = 1
    while get_number_of_factors(nth_triangle_number(i)) <= divisors:
        i += 1

    return nth_triangle_number(i)

if __name__ == '__main__':
    number_of_divisors = 500
    print "Answer found: ", get_triangle_number(number_of_divisors)

I've tried initializing get_triangle_number(), i, at higher values, but it didn't seem to help. I believe the problem is with get_number_of_factors(). I had originally stored all the factors of a given integer into a list then compared its length to number_of_divisors. I figured counting each integer that satisfied the condition would save some resources.

The code does run properly and produces the correct answer when I tested the example provided on Project Euler and the following values:

    number_of_divisors = 5      # Runs properly and produces correct answer
    number_of_divisors = 50     # Runs properly and         "    "
    number_of_divisors = 150    # Runs properly and         "    "
    number_of_divisors = 200    # Crash

First, you can easily find the bottleneck in your code by running it with the profiler (ran with number_of_divisors = 100):

~> python -m cProfile t.py
Answer found:  73920
         1926 function calls in 0.522 seconds

   Ordered by: standard name

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.522    0.522 t.py:1(<module>)
      385    0.006    0.000    0.008    0.000 t.py:1(nth_triangle_number)
        1    0.000    0.000    0.522    0.522 t.py:13(get_triangle_number)
      384    0.472    0.001    0.514    0.001 t.py:6(get_number_of_factors)
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
      769    0.043    0.000    0.043    0.000 {range}
      385    0.001    0.000    0.001    0.000 {sum}

You can immediately see that:

Indeed, get_number_of_factors() slows everything down;
You can slightly optimize things by replacing range with xrange (which does not create a list).

In your case get_number_of_factors() is a good example of a function that requires algorithmic optimization. Currently it has O(N) complexity. As you correctly guessed, iterating only till sqrt(num) will make it faster:

def get_number_of_factors(num):

    limit = math.sqrt(num)
    int_sqrt = int(round(limit))

    if int_sqrt ** 2 == num:
        int_limit = int_sqrt
        quantity = 1
    else:
        int_limit = int(limit) + 1
        quantity = 0

    for i in xrange(1, int_limit):
        if not num % i:
            quantity += 2

    return quantity

Some highlights:

math.sqrt returns a float, so if you want to be extra-safe, you need to convert it to integer properly (if sqrt(4) gives you 3.9999999, just int() will produce 3).
When you are iterating to the square root, each factor you found stands for two factors: i and num / i.
But you do not want to include the root itself, because if num is a perfect root, you only need to count sqrt(num) once.

This makes your algorithm run in O(sqrt(N)).
Edit: if you want even more performance, you can try to implement number field sieve.

To add more performance, you can replace the direct sum in nth_triangle_number() with a known formula for arithmetic progression.
The result is:

import math

def nth_triangle_number(nth):
    """ Sequence of triangle numbers is generated by adding the natural numbers.
        So the 7th triangle number would be 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28.
        Using the fact that 1 + ... + n = n * (n + 1) / 2
        """
    return nth * (nth + 1) / 2

def get_number_of_factors(num):

    limit = math.sqrt(num)
    int_sqrt = int(round(limit))

    if int_sqrt ** 2 == num:
        int_limit = int_sqrt
        quantity = 1
    else:
        int_limit = int(limit) + 1
        quantity = 0

    for i in xrange(1, int_limit):
        if not num % i:
            quantity += 2

    return quantity

def get_triangle_number(divisors):
    """ Find the triangle number with the given amount of divisors """
    i = 1
    while get_number_of_factors(nth_triangle_number(i)) <= divisors:
        i += 1

    return nth_triangle_number(i)

if __name__ == '__main__':
    number_of_divisors = 100
    print "Answer found: ", get_triangle_number(number_of_divisors)

And it runs much faster now. For 100:

~> python -m cProfile t.py
Answer found:  73920
         1540 function calls in 0.007 seconds

   Ordered by: standard name

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.007    0.007 t.py:1(<module>)
      384    0.006    0.000    0.006    0.000 t.py:10(get_number_of_factors)
        1    0.000    0.000    0.006    0.006 t.py:28(get_triangle_number)
      385    0.000    0.000    0.000    0.000 t.py:3(nth_triangle_number)
      384    0.000    0.000    0.000    0.000 {math.sqrt}
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
      384    0.000    0.000    0.000    0.000 {round}

And for 500:

~> python -m cProfile t.py
Answer found:  76576500
         49504 function calls in 5.364 seconds

   Ordered by: standard name

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    5.364    5.364 t.py:1(<module>)
    12375    5.342    0.000    5.349    0.000 t.py:10(get_number_of_factors)
        1    0.010    0.010    5.363    5.363 t.py:28(get_triangle_number)
    12376    0.005    0.000    0.005    0.000 t.py:3(nth_triangle_number)
    12375    0.003    0.000    0.003    0.000 {math.sqrt}
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
    12375    0.004    0.000    0.004    0.000 {round}


Camel Case strings in XSLT I have a situation where I am receiving data from a web service and the data is in all caps.  I have to convert to HTML and format for display.  In this case, we need the string to be normalized.  The XML we receive has the possibility, and most of the time is, large, sometimes being over a MB in size.  Performance is an issue.

I have the below templates that I use for normalizing the string.  I'm limited to XSLT 1.0.

<xsl:template name="CamelCase">
    <xsl:param name="text"/>

    <xsl:variable name="checkNBSP" select="translate($text, '&nbsp;', ' ')"/>
    <xsl:variable name="check160" select="translate($checkNBSP, '&#160;', ' ')"/>

    <xsl:choose>
        <xsl:when test="contains($check160,' ')">
            <xsl:call-template name="CamelCaseWord">
                <xsl:with-param name="text" select="substring-before($check160,' ')"/>
            </xsl:call-template>
            <xsl:text> </xsl:text>
            <xsl:call-template name="CamelCase">
                <xsl:with-param name="text" select="substring-after($check160,' ')"/>
            </xsl:call-template>
        </xsl:when>
        <xsl:otherwise>
            <xsl:call-template name="CamelCaseWord">
                <xsl:with-param name="text" select="$check160"/>
            </xsl:call-template>
        </xsl:otherwise>
    </xsl:choose>
</xsl:template>

<xsl:template name="CamelCaseWord">
    <xsl:param name="text"/>
    <xsl:value-of select="translate(substring($text,1,1),'abcdefghijklmnopqrstuvwxyz','ABCDEFGHIJKLMNOPQRSTUVWXYZ')" />
    <xsl:value-of select="translate(substring($text,2,string-length($text)-1),'ABCDEFGHIJKLMNOPQRSTUVWXYZ','abcdefghijklmnopqrstuvwxyz')" />
</xsl:template>

I initially found the below template on Stack Overflow and made some slight modifications to it to meet my purposes.  Is this way the optimal way to format the string, or can this be cleaned up?

The solution looks near optimal to me. If performance is an issue I would suggest the two following minor modifications:

The cleaning of the string regarding the special characters (&nbsp; and &#160;) is done at every calling level. You could introduce a third template called CamelCaseRecursive which would look like CamelCase but it would not contain the cleaning methods. So, CamelCase would be official external interface which would call CamelCaseRecursive which would call itself recursively. This change may save the more time the more words a text has on the average.
In CamelCaseWord you could contract the two <xsl:value-of> tags into one by using the concat() function:

<xsl:value-of
    select="concat(translate(substring($text,1,1),
                             'abcdefghijklmnopqrstuvwxyz',
                             'ABCDEFGHIJKLMNOPQRSTUVWXYZ'),
                   ' ',
                   translate(substring($text,2,string-length($text)-1),
                             'ABCDEFGHIJKLMNOPQRSTUVWXYZ',
                             'abcdefghijklmnopqrstuvwxyz')
                   )" />

I would assume that the performance gains will be minimal with today's highly optimized XSLT processors. However, if this actually helps it would be great to learn about it!


Ruby script on-all-nodes-run not only for teaching I wrote the following Ruby script several years ago and have been using it often ever since on a Bioinformatics computer cluster.

It pulls out a list of hosts from the Torque queuing system qnodes. It ssh'es and runs a command on all the nodes. Then it prints the output and/or errors in a defined order (alphabetical sort of the hostnames).

A nice feature: results are printed immediately for the host that is next in the order.

I would like to use it as an example for a Ruby workshop. Could you please suggest best-practice and design pattern improvements?

#!/usr/bin/ruby

EXCLUDE = [/girkelab/, /biocluster/, /parrot/, /owl/]

require "open3"

# Non-interactive, no password asking, and seasonable timeouts
SSH_OPTIONS = ["-o PreferredAuthentications=publickey,hostbased,gssapi,gssapi-with-mic",
               "-o ForwardX11=no",
               "-o BatchMode=yes",
               "-o SetupTimeOut=5",
               "-o ServerAliveInterval=5",
               "-o ServerAliveCountMax=2"
              ].join(" ")

SSH    = "/usr/bin/ssh #{SSH_OPTIONS}"
MKDIR  = "/bin/mkdir"

raise "Pleae give this command at least one argument" if ARGV.size < 1
COMMAND = ARGV[0..-1].join(' ')

output_o = {}
output_e = {}

IO_CONNECTIONS_TO_REMOTE_PROCESSES = {}

def on_all_nodes(&block)
  nodes = []
  Kernel.open('|qnodes | grep -v "^ " | grep -v "^$"') do |f|
    while line = f.gets
      i = line.split(' ').first
      nodes.push(i) if EXCLUDE.select{|x| i =~ x}.empty?
    end
  end
  nodes.sort.each {|n| block.call(n)}
end

# Create processes
on_all_nodes do |node|
  stdin, stdout, stderr = Open3.popen3("#{SSH} #{node} \"#{COMMAND}\"")
  IO_CONNECTIONS_TO_REMOTE_PROCESSES[node] = [stdin, stdout, stderr]
end

has_remote_errors = false

# Collect results
on_all_nodes do |node|
  stdin, stdout, stderr = IO_CONNECTIONS_TO_REMOTE_PROCESSES[node]

  stdin.close

  e_thread = Thread.new do
    while line = stderr.gets
      line.chomp!
      STDERR.puts "#{node} ERROR: #{line}"
      has_remote_errors = true
    end
  end

  o_thread = Thread.new do
    while line = stdout.gets
      line.chomp!
      puts "#{node}      : #{line}"
    end
  end

  # Let the threads finish
  t1 = nil
  t2 = nil
  while [t1, t2].include? nil
    if t1.nil?
      t1 = e_thread.join(0.1) # Gives 1/10 of a second to STDERR
    end
    if t2.nil?
      t2 = o_thread.join(0.1) # Give 1/10 of a second to STDOUT
    end
  end

end

exit(1) if has_remote_errors

First of all, take a look at the Net::SSH library. I don't have much experience with it, so I don't know whether it supports all the options you need. But if it does, using it might be more robust than using the command line utility (you wouldn't have to worry about whether ssh is installed in the place you expect (or at all) and you wouldn't have to worry about escaping the arguments).

Assuming you can't (or won't) use Net::SSH, you should at least replace /usr/bin/ssh with just ssh, so at least it still works if ssh is installed in another location in the PATH.

nodes = []
Kernel.open('|qnodes | grep -v "^ " | grep -v "^$"') do |f|
  while line = f.gets
    i = line.split(' ').first
    nodes.push(i) if EXCLUDE.select{|x| i =~ x}.empty?
  end
end

When you initialize an empty array and then append to it in a loop, that is often a good sign you want to use map and/or select instead.
line = f.gets is a bit of an anti-pattern in ruby. The IO class already has methods to iterate a file line-wise.
To find out whether none of the elements in an array meet a condition, negating any? seems more idiomatic than building an array with select and check whether it's empty.

nodes = Kernel.open('|qnodes | grep -v "^ " | grep -v "^$"') do |f|
  f.lines.map do |line|
    line.split(' ').first
  end.reject do |i|
    EXCLUDE.any? {|x| i =~ x}
  end
end

nodes.sort.each {|n| block.call(n)}

I would recommend that instead of taking a block and yielding each element, you just return nodes.sort and rename the function to all_nodes. This way you can use all_nodes.each to execute code on all nodes, but you could also use all_nodes.map or all_nodes.select when it makes sense.

Open3.popen3("#{SSH} #{node} \"#{COMMAND}\"")

Note that this will break if COMMAND contains double quotes itself. Generally trying to escape command line arguments by surrounding them with quotes is a bad idea. system and open3 accept multiple arguments exactly to avoid this.

If you make SSH an array (with one element per argument) instead of a string, you can use the multiple-arguments version of popen3 and can thus avoid the fickle solution of adding quote around COMMAND to escape spaces, i.e.:

Open3.popen3(*(SSH + [node, COMMAND]))

IO_CONNECTIONS_TO_REMOTE_PROCESSES = {}
# ...
on_all_nodes do |node|
  stdin, stdout, stderr = Open3.popen3("#{SSH} #{node} \"#{COMMAND}\"")
  IO_CONNECTIONS_TO_REMOTE_PROCESSES[node] = [stdin, stdout, stderr]
end

If you heeded my above advice about all_nodes you can simplify this using map. I also would suggest not using a Hash here. If you use an array instead, the nodes will stay in the order in which you inserted them, which will mean that you can iterate over that array instead of invoking all_nodes again.

has_remote_errors = false

all_nodes.map do |node|
  [node, Open3.popen3(*(SSH + [node, COMMAND]))]
end.each do |node, (stdin, stdout, stderr)|
  stdin.close

  ethread = # ...
  # ...
end

This way you removed the complexity of first putting everything into a hash and then getting it out again.

while line = stderr.gets

Again, this can be written more idiomatically as stderr.each_line do |line|. Same with stdout.

first = true

This is never used. I can only assume that it's a left over of previous iterations of the code, which is no longer necessary. Obviously it should be removed.

# Let the threads finish
t1 = nil
t2 = nil
while [t1, t2].include? nil
  if t1.nil?
    t1 = e_thread.join(0.1) # Gives 1/10 of a second to STDERR
  end
  if t2.nil?
    t2 = o_thread.join(0.1) # Give 1/10 of a second to STDOUT
  end
end

I don't see any benefit of doing it this way. Just do:

e_thread.join
o_thread.join

Note that joining on one thread does not mean that the other threads stop running - only the main thread does, but that's perfectly okay as you want that anyway.


Replacing files with symlinks to other files I wrote a little Python script to replace some files in usr/bin with symlinks to files in a different location.

I wouldn't mind anyone telling me where I could do better.

#!/usr/bin/env python -tt

"""Xcode4 installs and expects to find a git installation at /usr/bin.
This is a pain if you want to control your own installation of git that
might be installed elsewhere. This script replaces the git files in
/usr/bin with symlinks to the preferred installation.

Update the 'src_directory' to the location of your preferred git installation.
"""
import sys
import os

#- Configuration  -----------------------------------------------------------------
src_directory = '/usr/local/git/bin/' # preferred installation
#----------------------------------------------------------------------------------

dest_directory = '/usr/bin/'
files = ('git','git-cvsserver','git-receive-pack','git-shell','git-upload-archive','git-upload-pack','gitk')

def main():
    if os.getuid():
        print "This script needs to be run as 'sudo python update_git.py'"
        sys.exit(1)

    for a_file in files:
        src_file = os.path.join(src_directory, a_file)
        dest_file = os.path.join(dest_directory, a_file)

        if os.path.exists(dest_file):
            os.remove(dest_file)

        os.symlink(src_file, dest_file)

if __name__ == '__main__':
    main()

#!/usr/bin/env python -tt

"""Xcode4 installs and expects to find a git installation at /usr/bin.
This is a pain if you want to control your own installation of git that
might be installed elsewhere. This script replaces the git files in
/usr/bin with symlinks to the preferred installation.

Update the 'src_directory' to the location of your preferred git installation.
"""
import sys
import os

#- Configuration  -----------------------------------------------------------------
src_directory = '/usr/local/git/bin/' # preferred installation
#----------------------------------------------------------------------------------

dest_directory = '/usr/bin/'
files = ('git','git-cvsserver','git-receive-pack','git-shell','git-upload-archive','git-upload-pack','gitk')

The official python style guide recommends using ALL_CAPS to name global constants. Additionally, some of these constants might be better as command line arguments to the script. That way you don't need to modify the script to install to a different location.

def main():
    if os.getuid():

I suggest using if os.getuid() != 0 because I think its better to be explicit. The code will run the same, but this way I think its clearer that you are checking for zero rather then an actual boolean value.

        print "This script needs to be run as 'sudo python update_git.py'"
        sys.exit(1)

    for a_file in files:

a_file is kinda ugly. I'm guessing you are using it to avoid replace the builtin file. I suggest filename.

        src_file = os.path.join(src_directory, a_file)
        dest_file = os.path.join(dest_directory, a_file)

        if os.path.exists(dest_file):
            os.remove(dest_file)

What if someone deletes the file between your script checking if it exists and deleting it? Also checking if the file exists duplicates effort that remove will have to do. You could rewrite the code to try remove, and then catching the doesn't exist exception. I wouldn't bother here though.

        os.symlink(src_file, dest_file)

if __name__ == '__main__':
    main()

You don't catch any exceptions. In a script this simple that might be okay. But it might be a good idea to try/catch IOError/OSError and print them out for the user hopefully with enough detail that the user can tell whats going wrong. If you don't the exception will be dumped, but the user will also see a stack trace which might be scary.


Efficient Pandas to MySQL "UPDATE... WHERE" I have a pandas DataFrame and a (MySQL) database with the same columns. The database is not managed by me.

I want to update the values in the database in an "UPDATE... WHERE" style, updating only some columns wherever some other columns match.

Here's my code:

import sqlalchemy as sqla

def save_to_db(final_df, passwd):

    engine_str = 'mysql+mysqldb://username:{}@localhost/mydb'.format(passwd)

    engine = sqla.create_engine(engine_str)

    sm = sessionmaker(bind=engine)
    session = sm()

    metadata = sqla.MetaData(bind=engine)

    datatable = sqla.Table('AdcsLogForProduct', metadata, autoload=True)

    for ind, row in final_df.iterrows():
        u = sqla.sql.update(datatable) \
            .values({"q_ECI_B_x": row.q_ECI_B_x,
                     "q_ECI_B_y": row.q_ECI_B_y,
                     "q_ECI_B_z": row.q_ECI_B_z,
                     "q_ECI_B_s": row.q_ECI_B_s}) \
            .where(sqla.and_(datatable.c.year == row.year,
                             datatable.c.month == row.month,
                             datatable.c.day == row.day,
                             datatable.c.hours == row.hours,
                             datatable.c.minutes == row.minutes,
                             datatable.c.seconds == row.seconds,
                             datatable.c.milliseconds == row.milliseconds,
                             datatable.c.microseconds == row.microseconds))

        session.execute(u)

    session.flush()
    session.commit()

I'm doing this with plain sqlalchemy because apparently pandas' built-in SQL functions can't handle "UPDATE... WHERE" scenarios. However, this is really slow.

Isn't there a more efficient way to do this?

You have eight conditions to match for every UPDATE.  A typical solution would store timestamps using a DATETIME or TIMESTAMP column, so that there is only one value to match.

For reasonable performance, ensure that the timestamp field is indexed.


Read database from IO context and update UI from Main context in Kotlin Still a bit new to Kotlin and coroutines, so I want to learn best practices. The following code seems to work as expected in it's original context, though the naming has been tweaked here. The list of widgets is retrieved from the database, and the UI is updated correctly.
My main question is, in updateList is it approprate to use withContext (Dispatchers.Main) to access the UI from a job on the IO context? Feels a bit kludgy at first blush, but it does seem to work as expected.
class WidgetListActivity : AppCompatActivity() {
    private lateinit var db: AppDatabase
    private lateinit var getListJob: Job
    private lateinit var widgetList: List<WidgetRecord>

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_widget_list)

        runBlocking  {
            db = Room.databaseBuilder(applicationContext, AppDatabase::class.java, "Widget.db")
                .createFromAsset("database/prepop_db.sqlite").build()
        }
        val recyclerView = findViewById<RecyclerView>(R.id.widget_recycler_view)
        val linearLayoutManager = LinearLayoutManager(this.applicationContext)
        recyclerView.layoutManager = linearLayoutManager

        updateList()
    }

    private fun updateList() {
        getListJob = GlobalScope.launch (Dispatchers.IO) {
            widgetList = db.widgetDao().getAllWidgets()

            // HERE'S THE SECTION IN QUESTION
            withContext(Dispatchers.Main) {
                val recyclerView = findViewById<RecyclerView>(R.id.deck_recycler_view)
                var widgetListAdapter = WidgetListAdapter(widgetList)
                recyclerView.adapter = widgetListAdapter
                widgetListAdapter.notifyDataSetChanged()
            }
        }

    }

    override fun onDestroy() {
        runBlocking {
            getListJob.join()
        }

        // PLEASE NOTE: I know you wouldn't normally destroy the database
        // like this. The whole point of this Activity is simply to test
        // loading and displaying the contents of a prepopulated database.
        this.applicationContext.deleteDatabase("Widgets.db")
        super.onDestroy()
    }
}

One thing that makes it kludgy is using GlobalScope, which is discouraged by the coroutines design lead. In this case, if this were a very long-running job, you would be leaking your UI elements because they are captured by the coroutine.
Also, since you are using Room, you can make your DAO's getAllWidgets() a suspend function, in which case, there's no reason to use a specific dispatcher to call it. A properly composed suspend function doesn't block the dispatcher it is called from, so you never should have to specify a dispatcher to call a suspend function. The proper way to do this would be:
private fun updateList() {
    getListJob = lifecycleScope.launch {
        widgetList = db.widgetDao().getAllWidgets()
        val recyclerView = findViewById<RecyclerView>(R.id.deck_recycler_view)
        var widgetListAdapter = WidgetListAdapter(widgetList)
        recyclerView.adapter = widgetListAdapter
        widgetListAdapter.notifyDataSetChanged()
    }
}

No Dispatcher specifying needed at all, because the coroutine doesn't call any blocking code and lifecycleScope defaults to Dispatchers.Main.


Evaluate parenthesis expression for correct nesting and matching parens Please review this solution for matching parens for correctness and nesting.

def matchParens(pstring):
    plist = [l for l in pstring]
    newlist = []
    if plist[0] == ')' or plist.count('(') != plist.count(')') or plist[-1::] == '(':
        return False
    while len(plist) > 0:
        head, *tail = plist
        if head == '(':
            newlist.append(plist.pop(plist.index(head)))
            right = tail.index(')')
            if isinstance(right, int):
                newlist.append(plist.pop(right))
            else:
                return False
        else:
            return False
    return True

Use snake_case (matchParens)
You are checking a property, so I would name your function has_correct_parens to make it clearer that this function return True or False
[i for i in iterable] is a longer way of saying list(iterable) so plist = [l for l in pstring] should become plist = list(pstring)
The preventive check should be at the start for clarity and efficiency
What if you will want to use square / curly / triangular parens in the future? I would use constants:

OPEN_PAREN = '('
CLOSE_PAREN = ')'