Faster way to determine largest prime factor Project Euler problem 3 asks:

  What is the largest prime factor of the number 600851475143?

The first step I took is to determine the highest value number I should be looking at as a possibility (600851475143 / 2). Next I wanted to check each prime number as I go to see if it is a factor of 600851475143. If so, I store it in a variable denoting the highest prime factor encountered so far.

On running the program, the prime factor '6857' is the last output I am seeing for several minutes in my output. I entered this as the answer and it was correct. My problem is finding a way to streamline the algorithm to greatly reduce the processing time.

public class Euler3 {

    public static void main(String[] args) {
        long max = 600851475143L / 2;
        long lgstPrime = 1;
        boolean isPrime = true;

        for (int i=2; i<max; i++){
            for (int j=2; j<=i; j++){
                if ((i % j == 0) && (j != i))
                    isPrime = false;
            }

            if ((isPrime) && (600851475143L % i == 0))
                lgstPrime = i;

            isPrime = true;     //reset
        }

        System.out.println("\n" + "the largest prime factor of the number 600851475143 is " + lgstPrime);
    }
}

My problem is finding a way to streamline the algorithm to greatly reduce the processing time.

Well, if we take into account that we are on Code Review, than an answer to this could be a little bit off the scope, as @HosamAly has already written, too. I will investigate this a little bit further.

Problems related to prime numbers are studied very extensive. The conclusion of this is, whatever you do, you will most probably just reinvent the wheel.

So the question should not be "how to reduce the processing time", it should be "how to handle my problem" which leads to the question of the problem definition. If your problem definition is just as mentioned "What is the largest prime factor of the number 600851475143?", then take the number, put it into Wolfram Alpha, and be done in 5 seconds.

If your problem definition is more like "How can I calculate the largest prime factor of a given integer?" Then find any algorithm/library, check if it is suitable and be done.

If your problem definition is a real world requirement, then find out the time constraint. It is always about time constraints for real world problems like this. Without a time constraint, you could improve it literally forever.

If your problem definition is more like "I want to improve my knowledge about coding and optimization in Java and I will try the problem xyz" Then there is no real answer to this. You could ask probably for some solutions on stackoverflow (as far as I got it, they do not like questions for euler problems there, so hide it a little bit. I would post an answer there if you link it).

Some suggestions about your code were already done, but I do not see the point in giving advices in optimization here.

My suggestions about the code:

long lgstPrime = 1;

Please write the full name, so everyone can read and understand it. Is it longestPrime, largestPrime, logarithmstPrime? You do not save real time or space by saving some characters (assuming we are in Java and not something like fortran77).

long max = 600851475143L;
for (int i = 2; i < max; i++) {
    ...
}

I do not think that your code will work and output anything. This is an infinite loop. Why? Because i is an integer, it will overflow at Integer.MAX_VALUE (2^31 - 1 or 2147483647) starting at Integer.MIN_VALUE or -2147483648 again. This will never reach max which is greater than 2147483647. So the loop will run forever.

Example to test:

final long max = 600851475143L;
for (int i = 0; i < max; i++) {
    if (i == Integer.MAX_VALUE)
        System.out.println("will overflow now");
}

Better:

long max = 600851475143L;
for (long i = 2; i < max; i++) {
    ...
}

For readability, stick to one coding style:

for (int i=2; i<max; i++){
      for (int j=2; j<=i; j++){
          if ((i % j == 0) && (j != i))

Better:

for (int i = 2; i < max; i++) {
    for (int j = 2; j <= i; j++) {
        if ((i % j == 0) && (j != i))


Sieve of Eratosthenes - Python I've been doing a lot of Project Euler lately and just wanted to make sure my implementation was as good as it could be. Does anyone have any suggestions to speed this up?

def sieve(upperlimit):
    # mark off all multiples of 2 so we can use 2*p as the step for the inner loop
    l = [2] + [x if x % 2 != 0 else 0 for x in range(3, upperlimit + 1)]

    for p in l:
        if p ** 2 > upperlimit:
            break
        elif p:
            for i in range(p * p, upperlimit + 1, 2 * p):
                l[i - 2] = 0
    # filter out non primes from the list, not really that important i could work with a list full of zeros as well
    return [x for x in l if x]

Here's my starting point for computing the performance improvement due to the various revisions below: how long does it take to sieve for the prime numbers below \$ 10^8 \$?

>>> from timeit import timeit
>>> test = lambda f: timeit(lambda:f(10**8), number=1)
>>> t1 = test(sieve)

The exact number is going to depend on how fast your computer is, so I'm going to compute performance ratios, but for the record, here it is:

>>> t1
78.9875438772142

Your initialization of the list l takes more than half the time, so let's try a cheaper approach. Let's also give this array a better name, and make it a Boolean array while we're about it.

def sieve2(n):
    """Return a list of the primes below n."""
    prime = [True] * n
    for p in range(3, n, 2):
        if p ** 2 > n:
            break
        if prime[p]:
            for i in range(p * p, n, 2 * p):
                prime[i] = False
    return [2] + [p for p in range(3, n, 2) if prime[p]]

When optimizing a function like this, it's always worth keeping the un-optimized version around to check the correctness of the optimized version:

>>> sieve(10**6) == sieve2(10**6)
True

This already runs in less than a third of the time:

>>> test(sieve2) / t1
0.30390444573149544

We could avoid the test for p ** 2 > n by computing a tighter limit for the loop. Note that I've used n ** .5 here as this is slightly faster than math.sqrt(n).

def sieve3(n):
    """Return a list of the primes below n."""
    prime = [False, False, True] + [True, False] * (n // 2)
    for p in range(3, int(n ** .5) + 1, 2):
        if prime[p]:
            for i in range(p * p, n, 2 * p):
                prime[i] = False
    return [p for p in range(2, n) if prime[p]]

This makes little difference to the overall runtime:

>>> test(sieve3) / t1
0.2971086436068156

We can accumulate the result as we go, instead of in a separate iteration at the end. Note that I've cached result.append in a local variable to avoid looking it up each time round the loop.

def sieve4(n):
    """Return a list of the primes below n."""
    prime = [False, False, True] + [True, False] * (n // 2)
    result = [2]
    append = result.append
    sqrt_n = (int(n ** .5) + 1) | 1    # ensure it's odd
    for p in range(3, sqrt_n, 2):
        if prime[p]:
            append(p)
            for i in range(p * p, n, 2 * p):
                prime[i] = False
    for p in range(sqrt_n, n, 2):
        if prime[p]:
            append(p)
    return result

Again, this makes very little difference:

>>> test(sieve4) / t1
0.286016401170129

We can use Python's slice assignment instead of a loop when setting the sieve entries to False. This looks wasteful since we create a large list and then throw it away, but this avoids an expensive for loop and the associated Python interpreter overhead.

def sieve5(n):
    """Return a list of the primes below n."""
    prime = [True] * n
    result = [2]
    append = result.append
    sqrt_n = (int(n ** .5) + 1) | 1    # ensure it's odd
    for p in range(3, sqrt_n, 2):
        if prime[p]:
            append(p)
            prime[p*p::2*p] = [False] * ((n - p*p - 1) // (2*p) + 1)
    for p in range(sqrt_n, n, 2):
        if prime[p]:
            append(p)
    return result

This gives a small but noticeable improvement:

>>> test(sieve5) / t1
0.2617646381557855

For big improvements to the performance of numerical code, we can use NumPy.

import numpy

def sieve6(n):
    """Return an array of the primes below n."""
    prime = numpy.ones(n, dtype=numpy.bool)
    prime[:2] = False
    prime[4::2] = False
    sqrt_n = int(n ** .5) + 1
    for p in range(3, sqrt_n, 2):
        if prime[p]:
            prime[p*p::2*p] = False
    return prime.nonzero()[0]

This is more than 6 times as fast as sieve5, and more than 25 times as fast as your original code:

>>> test(sieve6) / t1
0.03726392181902129

We could avoid allocating space for the even numbers, improving memory locality:

def sieve7(n):
    """Return an array of the primes below n."""
    prime = numpy.ones(n // 2, dtype=numpy.bool)
    sqrt_n = int(n ** .5) + 1
    for p in range(3, sqrt_n, 2):
        if prime[p // 2]:
            prime[p*p // 2::p] = False
    result = 2 * prime.nonzero()[0] + 1
    result[0] = 2
    return result

>>> test(sieve7) / t1
0.029220096670965198

And finally, an implementation that sieves separately for primes of the form \$ 6i − 1 \$ and \$ 6i + 1 \$, due to Robert William Hanks:

def sieve8(n):
    """Return an array of the primes below n."""
    prime = numpy.ones(n//3 + (n%6==2), dtype=numpy.bool)
    for i in range(3, int(n**.5) + 1, 3):
        if prime[i // 3]:
            p = (i + 1) | 1
            prime[       p*p//3     ::2*p] = False
            prime[p*(p-2*(i&1)+4)//3::2*p] = False
    result = (3 * prime.nonzero()[0] + 1) | 1
    result[0] = 3
    return numpy.r_[2,result]

This is about 40 times as fast as the original sieve:

>>> test(sieve8) / t1
0.023447068662434022


Conversion helper with support for enumerations and Nullable I recently wrote this code as a more versatile stand-in for Convert.ChangeType. I have a nagging feeling that there's something I might be overlooking, or that there might be a more efficient algorithm for this.

    /// <summary>
    ///     Returns an object of type <typeparamref name="T"/> whose value is equivalent to that of the specified
    ///     object.
    /// </summary>
    /// <typeparam name="T">
    ///     The output type.
    /// </typeparam>
    /// <param name="value">
    ///     An object that implements <see cref="IConvertible"/> or is <see cref="Nullable{T}"/> where the underlying
    ///     type implements <see cref="IConvertible"/>.
    /// </param>
    /// <returns>
    ///     An object whose type is <typeparamref name="T"/> and whose value is equivalent to <paramref name="value"/>.
    /// </returns>
    /// <exception cref="System.ArgumentException">
    ///     The specified value is not defined by the enumeration (when <typeparamref name="T"/> is an enum, or Nullable{T}
    ///     where the underlying type is an enum).
    /// </exception>
    /// <exception cref="System.InvalidCastException"
    /// <remarks>
    ///     This method works similarly to <see cref="Convert.ChangeType(object, Type)"/> with the addition of support
    ///     for enumerations and <see cref="Nullable{T}"/> where the underlying type is <see cref="IConvertible"/>.
    /// </remarks>
    internal static T ChangeType<T>(object value) {

        Type type = typeof(T);
        Type underlyingNullableType = Nullable.GetUnderlyingType(type);

        if ((underlyingNullableType ?? type).IsEnum) {

            // The specified type is an enum or Nullable{T} where T is an enum.

            T convertedEnum = (T)Enum.ToObject(underlyingNullableType ?? type, value);

            if (!Enum.IsDefined(underlyingNullableType ?? type, convertedEnum)) {
                throw new ArgumentException("The specified value is not defined by the enumeration.", "value");
            }

            return convertedEnum;
        } else if (type.IsValueType && underlyingNullableType == null) {

            // The specified type is a non-nullable value type.

            if (value == null || DBNull.Value.Equals(value)) {
                throw new InvalidCastException("Cannot convert a null value to a non-nullable type.");
            }

            return (T)Convert.ChangeType(value, type);
        }

        // The specified type is a reference type or Nullable{T} where T is not an enum.
        return (value == null || DBNull.Value.Equals(value)) ? default(T) : (T)Convert.ChangeType(value, underlyingNullableType ?? type);
    }

/// <summary>
///     Returns an object of type <typeparamref name="T"/> whose value is equivalent to that of the specified
///     object.
/// </summary>

...

/// <returns>
///     An object whose type is <typeparamref name="T"/> and whose value is equivalent to <paramref name="value"/>.
/// </returns>

I don't see any reason to repeat what was said in <summary> in <returns>. I wouldn't write <returns> here at all. (Unless you're writing a library with very high demands on documentation, like the .Net framework itself.)

underlyingNullableType ?? type

You're repeating this expression several times. You should probably extract it into a variable.

You should be consistent: either use if-else if-else and ignore the returns, or acknowledge the returns and use if-if-nothing.


Find group of strings that are anagrams This question refers to this problem on lintcode. I have a working solution, but it takes too long for the huge testcase. I am wondering how can it  be improved? Maybe I can decrease the number of comparisons I make in the outer loop.

class Solution:
    # @param strs: A list of strings
    # @return: A list of strings
    def anagrams(self, strs):
        # write your code here
        ret=set()
        for i in range(0,len(strs)):
            for j in range(i+1,len(strs)):
                if i in ret and j in ret:
                    continue
                if Solution.isanagram(strs[i],strs[j]):
                    ret.add(i)
                    ret.add(j)

        return [strs[i] for i in list(ret)]

    @staticmethod
    def isanagram(s, t):
        if len(s)!=len(t):
            return False
        chars={}
        for i in s:
            if i in chars:
                chars[i]+=1
            else:
                chars[i]=1

        for i in t:
            if i not in chars:
                return False
            else:
                chars[i]-=1
                if chars[i]<0:
                    return False

        for i in chars:
            if chars[i]!=0:
                return False
        return True

Different algorithm

Instead of considering all pairs of strings which leads to a O(n^2) behavior (where n is the number of strings), you could operate differently and use a dictionnary to group words based on some kind of signature with would be the same for anagrams and different for non-anagrams.

In your case, you could use the "sorted" version of a word as a signature : only 2 anagrams will be indentical once sorted. Once you've determined how to compute this, you can easily define a dictionnary mapping sorted words to the anagrams they correspond to.

This is a typical situation where the setdefault method is useful.

You can write something like:

t1 = ["lint", "intl", "inlt", "code"] # return ["lint", "inlt", "intl"].
t2 = ["ab", "ba", "cd", "dc", "e"] # return ["ab", "ba", "cd", "dc"].

def anagrams(word_list):
    anagrams = {}
    for w in word_list:
        anagrams.setdefault(''.join(sorted(w)), []).append(w)
    # print(anagrams)
    return sorted(  # sort is to make results always the same
                [w for anagrams in anagrams.values()
                    if len(anagrams) > 1
                        for w in anagrams])

assert anagrams(t1) == ['inlt', 'intl', 'lint']
assert anagrams(t2) == ['ab', 'ba', 'cd', 'dc']


Caesar Cipher Guesser I've written a Caesar Cipher in python (a more in-depth explanation is provided in the docstring at the top of the program). At first, it was supposed to just be the cipher, but I expanded it to have the user guess the original string, given the encoded string. I would like feedback on the caesar algorithm, and any other nitpicks you can find in my program. Any and all feedback is appreciated and considered!

Link to github prompt

cipher.py

""" Import Statements """
import random

"""
Caesar cipher - Implement a Caesar cipher, both encoding and decoding.

The key is an integer from 1 to 25. This cipher rotates the letters of the alphabet
(A to Z). The encoding replaces each letter with the 1st to 25th next letter in the
alphabet (wrapping Z to A). So key 2 encrypts "HI" to "JK", but key 20 encrypts "HI"
to "BC". This simple "monoalphabetic substitution cipher" provides almost no security,
because an attacker who has the encoded message can either use frequency analysis to
guess the key, or just try all 25 keys.

Input is all lowercase, so using ord() should use 97-122 a-z
"""

WORDS = [
    "thisisarandomstring", "hellomynameisben", "whatisyourname", "thiswaswrittenonamac",
    "macsarethebestcomputer", "thisprogramisinpython", "thisonlyhasletters", "andeveryother",
    "stringinthislist", "isonlyusingletters", "becausethatshowitworks", "andifyoudontlikeit",
    "thenyoucangetyour", "naysayerselfoutofhere", "thatshowiroll"
]

CHART = {
    97: 'a', 98: 'b', 99: 'c', 100: 'd', 101: 'e', 102: 'f', 103: 'g',
    104: 'h', 105: 'i', 106: 'j', 107: 'k', 108: 'l', 109: 'm', 110: 'n',
    111: 'o', 112: 'p', 113: 'q', 114: 'r', 115: 's', 116: 't', 117: 'u',
    118: 'v', 119: 'w', 120: 'x', 121: 'y', 122: 'z'
}

def encode(text, rotations):
    """ Encodes the passed text, rotating `rotations` times """
    encoded_text = ""
    for index in range(len(text)):
        new_position = ord(text[index]) + (rotations % 26)
        if new_position > 122:
            new_position = 97 + (new_position - 122)
        encoded_text += CHART[new_position]
    return encoded_text

def decode(text, rotations):
    """ Decodes the passed text, with the passed rotations """
    decoded_text = ""
    for index in range(len(text)):
        new_position = ord(text[index]) - (rotations % 26)
        if new_position < 97:
            diff = 97 - new_position
            new_position = 122 - diff
        decoded_text += CHART[new_position]
    return decoded_text

def check_guess(guess):
    """ Checks how close the guess is to the original string """
    if len(guess) != len(DECODED):
        return "Guess must be same length to check guess!"
    response = ""
    for i in range(len(guess)):
        if guess[i] == DECODED[i]:
            response += guess[i]
        else:
            response += " "
    return response

if __name__ == '__main__':

    ROTATIONS = random.randint(3, 26)
    ENCODED = encode(random.choice(WORDS), ROTATIONS)
    DECODED = decode(ENCODED, ROTATIONS)
    print(f"""
        Encoded: {ENCODED}
        Length: {len(ENCODED)}
    """)

    print(f"What is the original string?")
    GUESS = input(">>> ")
    GUESSES = 1
    while GUESS != DECODED:
        print(f">>> {check_guess(GUESS)}")
        GUESS = input(">>> ")
        GUESSES += 1

    print("Correct!")

    print(f"""
        Encoded: {ENCODED}
        Original: {DECODED}
        Guesses: {GUESSES}
    """)

First comment - "Import Statements" - is unnecessary.

I would add few empty lines to make code more readable - ie. before return, for

Document PEP 8 -- Style Guide for Python Code suggest to use UPPER_CASE_NAMES for constant values - WORDS and CHARS you can treat as constant values but ROTATIONS, ENCODED, DECODED, GUESS,GUESSES` are not constant values.

Instead of range(len(..)) in

for index in range(len(text)):
    new_position = ord(text[index]) - (rotations % 26)

you can iterate list

for char in text:
    new_position = ord(char) - (rotations % 26)

There is similar situation in other loop.

for i in range(len(guess)):
    if guess[i] == DECODED[i]:
        response += guess[i]

But here you need i to get DECODED so you can use enumerate()

 for i, char in enumerate(guess):
    if char == DECODED[i]:
        response += char
    else:
        response += " "

You can calculate rotations % 26 only once - before for loop.


Determine whether a number and its square are both the sum of two squares Code gets timed out and needs to be simplified.
To ensure that n = sum of squares
, To determine if n^2 = sum of squares
, I need to input 4 integers : 5881, 2048, 2670, 3482
#To define the sum of squares
    def sumSquare(n) :
        i = 1
        while i * i <= n :
            j = 1
            while(j * j <= n) :
                if (i * i + j * j == n) :
                    return True
                j = j + 1
            i = i + 1

        return False

#To define that the square of n is equals to the square of i added to the square of j
    def Squared(n):
        i = 1
        while i * i <= n * n:
            j = 1
            while (j * j <= n * n):
                if(i * i + j * j == n * n):
                    return True
                j = j + 1
            i = i + 1

        return False

    T = int(input())
    while T>0:
        T-=1
        n = int(input())
        if ((sumSquare(n)) and (Squared(n))):
            print ("Yes")
        else:
            print("No")

You can speed it up by saving the squares in a set. If you wrote a function to do this, you can give it a mutable default argument for it to save all the squares instead of having to calculate them repeatedly.
# dict used because there’s no insertion ordered set
def sum_squares(n, saved_squares=dict()):
    # check for sum while saving squares for future calls
    check_end = n
    if int(n**(1/2)) > len(saved_squares):
        check_end = (len(saved_squares) + 1)**2
        found_while_saving = False
        for num_to_save in range(len(saved_squares) + 1, int(n**(1/2) + 1)):
            square = num_to_save**2
            saved_squares[square] = None
            if n - square in saved_squares:
                found_while_saving = True
        if found_while_saving == True:
            return True
    # check for sum in set of already calculated squares
    for square in saved_squares:
        if n - square in saved_squares:
            return True
        # insertion order needed so early return won’t happen
        if square >= check_end:
            return False

With the numbers 5881, 2048, 2670, and 3482, it will only have to calculate any squares for the first call with the number 5881. On the other three calls it can skip to checking the set of all the already calculated squares. But no matter what order of numbers it’s called with, it will only have to calculate the square of any number once.
This is how I tested it on repl.it:
nums_to_check = (2048, 2670, 3482, 5881)
for n in nums_to_check:
    print(sum_squares(n) and sum_squares(n**2))

It completed in under a second, so hopefully it’s fast enough for the site you’re using.


Accessing a REST API with PHP I'm currently using a scientific information management system that employs a RESTful API. I've never used this API with PHP, and I had a hard time finding best practice examples online. After some reading and experimenting, I came up with the following proof-of-concept procedure to retrieve data from the API and then upload the XML object, slightly modified, in order to change information in the system.

I'm most concerned with PHP best practices here: is my PHP efficient, robust, and secure? Is there any code I can remove or make more efficient? I'm using PHP v5.3.8

<!DOCTYPE html>

<?php
$uriPrefix = 'http://[user:pass]@[path_to_web_app:port]/api/v2/';

//Open up a connection to the API
//for the container:
$containerSuffix = 'containers/27-95';
$containerUri = $uriPrefix . $containerSuffix;
$containerCurl = curl_init($containerUri);

//for the sample:
$sampleSuffix = 'samples/DOR6A12';
$sampleUri = $uriPrefix . $sampleSuffix;
$sampleCurl = curl_init($sampleUri);

//API-specific settings
curl_setopt($containerCurl, CURLOPT_RETURNTRANSFER, true);
curl_setopt($sampleCurl, CURLOPT_RETURNTRANSFER, true);

//get the yummy data
$containerResult = curl_exec($containerCurl);
$containerXml = new SimpleXMLElement($containerResult);
$sampleResult = curl_exec($sampleCurl);
$sampleXml = new SimpleXMLElement($sampleResult);

//close the API connection
curl_close($containerCurl);
curl_close($sampleCurl);

echo '
Some XML for a LIMS Container:
';
echo htmlentities($containerXml->asXML());
echo '
Some XML for a LIMS Sample:
';
echo htmlentities($sampleXml->asXML());

//Now let's infer the project ID for the sample from the XML result
$limsId = $sampleXml->project['limsid'];
echo "
The project ID for the sample is $limsId
";

//Now let's change the sample name
$newName = 'New Sample Name';
$oldName = (string)$sampleXml->name;
$sampleXml->name = $newName;

$changeSampleCurl = curl_init($sampleUri);
curl_setopt($changeSampleCurl, CURLOPT_RETURNTRANSFER, true);
curl_setopt($changeSampleCurl, CURLOPT_CUSTOMREQUEST, "PUT");
curl_setopt($changeSampleCurl, CURLOPT_POSTFIELDS,$sampleXml->asXML());

$changeSampleResult = curl_exec($changeSampleCurl);
echo '
The submitted XML
';
echo htmlentities($changeSampleResult);

curl_close($changeSampleCurl);

echo "
The old sample name was $oldName
";
echo "
It should now be $newName
";
?>

The most obvious omission is error handling. You don't handle curl errors or xml parsing errors.

A REST api is like any other external service you might use in your application (such as a database). You might consider creating a client library for it (first check to see if one does not already exist for the service in question). Example usage with the implementation details neatly tucked away:

<?php
//read these values from your configuration and set here
$connection = array(
    'user' => '',
    'pass' => '',
    'baseUrl' => '',
    'version' => ''
);
try {
    $client = new FooClient($connection);
    $container = $client->getContainer('27-95');
    if ($container) {
        //do something with the fetched container
    }

    $sample = $client->getSample('DOR6A12');
    if ($sample) {
        $sample->setName('new name');
        $client->saveSample($sample);
    }
} catch (FooClientException $e) {
    //handle accordingly
}

I'm assuming that the client will return mapped objects for the getContainer and getSample methods. These mapped objects would be instances of php classes which are part of this client library. The xml responses returned by the service would be mapped to to these returned objects. That way, you are not directly dealing with the xml here, rather those details are dealt with in your client library (errors are handled there too with an appropriate exception being thrown if need be).


Clojure: Find specific element in HTML tree I am trying to find all "link" elements that link to RSS feeds in a data structure created by clj-tagsoup. I wrote the following code, which seems to work fine. But: I come from a Java background and I am new to functional programming and clojure, so I am not sure if this is the right way to do it. Could you please give me some hints about what I could improve in the following code:

(defn- matches-attributes [current-attributes required-attributes]
    (reduce #(and %1 (= (second %2) ((first %2) current-attributes)))
            true
            required-attributes))

(defn- find-tag [tag-name current-tag attributes]
    (if (= tag-name (tagsoup/tag current-tag))
        current-tag
        (filter #(and (= tag-name (tagsoup/tag %1))
                (matches-attributes (tagsoup/attributes %1) attributes))
                (tagsoup/children current-tag))))

(defn- find-rss-feed [page-result]
    (let [document (tagsoup/parse-string (:body page-result))
          head (first (find-tag :head document nil))
          rss-links (if (nil? head)
                        nil
                        (find-tag :link head {:type "application/rss+xml"}))]
    ... do something with rss-links...))

Edit To clarify a bit, what I am particularily interested in is if traversing the HTML tree the way I do it is the "right" was in a functional language. What I'm also interested in is if "filter" and "reduce" are a good choice as I use them, or if there is a better way. And, of course, I'd also appreciate general hints about what I could do better in this little program...

It's not bad, but we can make it a bit more idiomatic. Let's look at this function first:

(defn- matches-attributes [current-attributes required-attributes]
    (reduce #(and %1 (= (second %2) ((first %2) current-attributes)))
            true
            required-attributes))

The use of reduce is a bit odd, instead let's use every? Secondly, if current-attributes is a hash map or a set, then we can actually use it as a function. Since every? takes a predicate, we can rewrite the entire function as:

(defn- matches-attributes [current-attributes required-attributes]
  (every? (fn [tv tg]
              (= (current-attributes tg) tv))
          required-attributes))

On the last function, when-let will help you clean it up a bit more:

(defn- find-rss-feed [page-result]
    (let [document (tagsoup/parse-string (:body page-result))
          rss-links (when-let [head (first (find-tag :head document nil))]
                        (find-tag :link head {:type "application/rss+xml"}))]
    ... do something with rss-links...))

Hopefully this helps a bit. Your code doesn't really support this, but the cool thing to remember is that Clojure collections are functions that return nil if the item doesn't exist. So for example:

=> (remove #{1 2 3} [1 2 3 4 5 6 7])
(4 5 6 7)


CakePHP Ticket Component I have the following code that allows easy creation, pulling, and deletion of Tickets in a CakePHP application.

App::uses('Component', 'Controller');
App::uses('Ticket', 'Model');
App::import(array('Security'));

class TicketComponent extends Component
{
    public $name = 'Ticket';

    // Create a new ticket by providing the data to be stored in the ticket.
    function set($info = null)
    {
        $this->garbage();
        if ($info)
        {
            $ticketObj = new Ticket();
            $data['Ticket']['hash'] = md5(time());
            $data['Ticket']['data'] = $info;

            if ($ticketObj->save($data))
            {
                return $data['Ticket']['hash'];
            }
        }
        return false;
    }

    // Return the value stored or false if the ticket can not be found.
    function get($ticket = null)
    {
        $this->garbage();
        if ($ticket)
        {
            $ticketObj = new Ticket();
            $data = $ticketObj->findByHash($ticket);
            if (is_array($data) && is_array($data['Ticket']))
            {
                // optionally auto-delete the ticket -> this->del($ticket);
                return $data['Ticket']['data'];
            }
        }
        return false;
    }

    // Delete a used ticket
    function del($ticket = null)
    {
        $this->garbage();
        if ($ticket)
        {
            $ticketObj = new Ticket();
            $data = $ticketObj->findByHash($ticket);
            if ( is_array($data) && is_array($data['Ticket']) )
            {
                return $data = $ticketObj->delete($data['Ticket']['id']);
            }
        }
        return false;
    }

    // Remove old tickets
    function garbage()
    {
        $deadline = date('Y-m-d H:i:s', time() - (24 * 60 * 60)); // keep tickets for 24h.
        $ticketObj = new Ticket();
        $data = $ticketObj->query('DELETE from tickets WHERE created < \''.$deadline.'\'');
    }
}

It feels a little old now (not sure how old the code is) but wondered if certain things could be improved and if there was any parts that were considered 'bad code'.

Some observations of my own:

Do I need to keep creating the $ticketObj? And is using new Ticket() correct for CakePHP conventions?
The garbage function runs when anything ticket related is called, and makes sure that expired tickets cannot be used... But requires that it be called at the beginning of all other functions. Could this be better? Not necessarily a CRON job, but more code efficient.

In order to call each time the garbage function just create a

public function beforeFilter() {
        parent::beforeFilter();
        $this->garbage();

About the new Ticket()
i think it would be okay to change the

$ticketObj->..

to

$this->Ticket->..

Also i think setting your function name's as set is a bad idea because they are built in functions in cakephp


Kattis "Chasing Subs" custom decryption challenge I wrote a function to solve a problem in Java, but it's still too brute force, and I'd like to optimize in terms of runtime.

Link to the description of the problem. The input consists of two lines: a fragment of plaintext, preceded by the message that is encrypted using some unknown character-to-character mapping. The challenge is to find out where the plaintext fragment could have come from.

What I have tried in the code below is to go over the encrypted string char by char (for the length of my key every time), and try to make a dictionary. If I could successfully assign every character from the key to a character from the encrypted text in a deterministic manner, then I note it down in a counter, and start over from the next character in the encrypted text.

So far this solution works on every test, until it reaches huge test case where my "brute force" solution is exceeding the time limit.

Any ideas on how could I improve performance or try a different approach?

package chasingsubs;

import java.util.HashMap;
import java.util.HashSet;
import java.util.Scanner;

public class ChasingSubs {

    private void decrypt(String line, String key) {
        int counter = 0;
        int startIndex = 0;
        for (int i = 0; i < line.length() - key.length() + 1; i++) {
            HashMap<Character, Character> dictionary = new HashMap<>();
            HashSet<Character> used = new HashSet<>();

            for (int j = 0; j < key.length(); j++) {
                Character keyChar = key.charAt(j);
                Character sourceChar = line.charAt(i+j);

                if (dictionary.get(keyChar) == null) {
                    if (!used.contains(sourceChar)) {
                        dictionary.put(keyChar, sourceChar);
                        used.add(sourceChar);
                    }else{
                        break;
                    }
                } else if (!dictionary.get(keyChar).equals(sourceChar)) {
                    break;
                }

                if (j == key.length() - 1) {
                    counter++;
                    if (counter == 1) {
                        startIndex = i;
                    }
                }
            }
        }
        if (counter == 0) {
            System.out.println("0");
            return;
        }
        if (counter == 1) {
            System.out.println(line.substring(startIndex, startIndex + key.length()));
            return;
        }
        System.out.println(counter);
    }

    public static void main(String[] args) {
        ChasingSubs base = new ChasingSubs();
        Scanner scanner = new Scanner(System.in);
        base.decrypt(scanner.nextLine(), scanner.nextLine());
    }

}

Alternative approach

You are repeating a lot of work.  Each time you advance a character, you rebuild the matching.  But you don't need to do that.  Instead create an array that says where to find the next instance of that character in the string.

public static int buildNextArray(String s) {
    int[] next = new int[s.length() - 1];
    for (int i = 0; i < next.length; i++) {
        next[i] = s.indexOf(s.charAt(i), i + 1) - i;
    }

    return next;
}

This converts the string into a unique array holding the relevant information:  how far away is the next instance of that character in the string?  Now, if the array you build for the decrypted key matches a portion of the array for the encrypted line, then it is a possible cipher match.

I'll leave the exact implementation for you to develop.  Note that a negative number in the array for the key may match an unequal number in the array for the line.  You may want to think about when it can match and when it can't match.

I would expect this approach to be faster because you can quickly see if a particular sequence matches by climbing the sequence for a particular letter.  Consider the case where the input is

  abcdefghijklmnopqrstuvwxyz
  abca

In your version, you would match the first three letters to all twenty-three possible sequences and only find out on the fourth letter that it didn't work.  With my suggestion, you would find out on the second letter that it won't work.  And while there may be some inputs for which this is slower, the general result will be faster.  Because starting with three or more different letters is more common than having the first and fourth letter the same.

It also saves the overhead of the HashSet and HashMap accesses.  Instead, you can do array accesses.

Some other coding comments that won't affect performance:

Code to the interface

            HashMap<Character, Character> dictionary = new HashMap<>();
            HashSet<Character> used = new HashSet<>();

This could be

            Map<Character, Character> dictionary = new HashMap<>();
            Set<Character> used = new HashSet<>();

It won't matter much here, but in general, you want to code to the interface.  It makes it easier to switch implementations later.

Delegate

        for (int i = 0; i < line.length() - key.length() + 1; i++) {
            HashMap<Character, Character> dictionary = new HashMap<>();
            HashSet<Character> used = new HashSet<>();

            for (int j = 0; j < key.length(); j++) {
                Character keyChar = key.charAt(j);
                Character sourceChar = line.charAt(i+j);

                if (dictionary.get(keyChar) == null) {
                    if (!used.contains(sourceChar)) {
                        dictionary.put(keyChar, sourceChar);
                        used.add(sourceChar);
                    }else{
                        break;
                    }
                } else if (!dictionary.get(keyChar).equals(sourceChar)) {
                    break;
                }

                if (j == key.length() - 1) {
                    counter++;
                    if (counter == 1) {
                        startIndex = i;
                    }
                }
            }
        }

This gets a lot simpler if you make a helper method.

    public bool canMatch(String haystack, String needle) {
        Map<Character, Character> dictionary = new HashMap<>();
        Set<Character> used = new HashSet<>();

        for (int j = 0; j < needle.length(); j++) {
            Character keyChar = needle.charAt(j);
            Character sourceChar = haystack.charAt(j);

            if (dictionary.get(keyChar) == null) {
                if (used.contains(sourceChar)) {
                    return false;
                }

                dictionary.put(keyChar, sourceChar);
                used.add(sourceChar);
            } else if (!dictionary.get(keyChar).equals(sourceChar)) {
                return false;
            }
        }

        return true;
    }

I flipped around some of the internal logic.  I find an if (false)/else structure confusing.  I would rather check truth.  And in this case, that also means that we can get rid of the else, as the original else clause finishes with a return (break).  This has a side effect of reducing the indent on the other clause, which can sometimes be helpful.

Now, if the needle matches, we can return true.  Otherwise, we return false.  This allows us to change the caller

    for (int i = 0; counter == 0 && i < 1 + line.length() - key.length(); i++) {
         if (canMatch(line.substring(i), key)) {
             counter++;
             startIndex = i;
         }
    }

    if (counter == 0) {
        return "0";
    }

    for (int i = startIndex + 1; i < 1 + line.length() - key.length(); i++) {
        if (canMatch(line.substring(i), key)) {
            counter++;
        }
    }

    if (counter == 1) {
        return line.substring(startIndex, startIndex + key.length());
    }

    return Integer.toString(counter);

Now we don't have to compare j to the last index of the key on every iteration.  The helper method will only return true when that was true.

I also changed the original method to return a string rather than print.  Then the caller can do the print.  This is a good habit that makes the method more flexible.  Of course, it would be even better if we returned something other than a string, perhaps a result object.


Defaulting dependencies What are some advantages and disadvantages of providing default implementations for dependencies. I have chosen to do this because it allows the objects to be easily used in the application but also allows me to mock for unit testing.

public class RoleProvider : RoleProviderBase
{
    private IRoleDataProvider _roleDataProvider;

    private IEntitlementsDataProvider EntitlementsDataProvider
    {
        get
        {
            if (_roleDataProvider == null)
            {
                _roleDataProvider = new RoleDataProvider();
            }
            return _roleDataProvider;
        }
    }

    public RoleProvider(IRoleDataProvider roleDataProvider)
    {
        _roleDataProvider = roleDataProvider;
    }

    public RoleProvider()
    {
    }
}

I personally don't see any problem with this (maybe someone else might provide better insight) but in this situation I do tend to do it differently but instantiating it via constructor overloading and making the private field readonly.

e.g.

public class RoleProvider : RoleProviderBase
{
    private readonly IRoleDataProvider _roleDataProvider;

    public RoleProvider()
       : this(new RoleDataProvider())
    {
    }

    public RoleProvider(IRoleDataProvider roleDataProvider)
    {
        _roleDataProvider = roleDataProvider;
    }
}

Perhaps this question might be better asked on PE as well?


Insurance premium calculator exercise I attempted a coding exercise during an interview, defined & implemented here (readme for problem):

https://gitlab.com/vineet_rc/health-insurance-premium-quote-generator-2

It was initially a simple and single java class program.

public double calculatePremium(Person person) {
    // Added base premium
    double premium = 5000;

    // Increase premium depending on age
    if (person.getAge() >= 18) {
        premium *= 1.1;
    }
    if (person.getAge() >= 25) {
        premium *= 1.1;
    }
    if (person.getAge() >= 30) {
        premium *= 1.1;
    }
    if (person.getAge() >= 35) {
        premium *= 1.1;
    }
    if (person.getAge() >= 40) {
        // Add 20% per 5 years above 40
        int age = person.getAge() - 40;
        while (age >= 5) {
            premium *= 1.2;
            age -= 5;
        }
    }

    // Add gender specific adjustments
    if (person.getGender() == Gender.MALE) {
        premium *= 1.02;
    }

    // Increase 1% per health issue
    for (HealthIssues issue : person.getHealthIssues()) {
        premium *= 1.01;
    }

    Set<Habbits> habbitsList = person.getHabbits();
    // Decrease 3% per good habbit
    for (Habbits habbit : habbitsList) {
        if (Constants.goodHabbits.contains(habbit)) {
            premium *= 0.97;
        }
    }
    // Increase 3% per good habbit
    for (Habbits habbit : habbitsList) {
        if (Constants.badHabbits.contains(habbit)) {
            premium *= 1.03;
        }
    }

    return premium;
}

I only had three JUnits with different Personobjects initially. The reviewer mentioned if the JUnit failed, we wouldn't know exactly where it failed.

So I took cue & cut the various calculations & gave it its own method. Then called them in my original calculatePremium() method.

public double calculatePremium(Person person) {
    // Added base premium
    double premium = 5000;
    premium = this.applyAgeBasedCalcs(person, premium);
    premium = this.applyGenderBasedCalcs(person, premium);
    premium = this.applyHealthIssuesBasedCalcs(person, premium);
    premium = this.applyGoodHabbitsBasedCalcs(person, premium);
    premium = this.applyBadHabbitsBasedCalcs(person, premium);
    return premium;
}

And of course added JUnits for them (not an exhaustive set). So instead of testing only for the final answer, we're also testing if the individual calculating components are working fine or not.

Finally the reviewer mentioned that if he ever wanted to change the order or the number of apply* methods called (add new ones), but without changing this class every time, how would one do about it? He still may want to use this original version of the rules. I immediately explained the strategy pattern & upon request, implemented it.

I merely made an interface (PremiumCalculationStrategy) & a single class that implements it (NormalPremiumCalculationStrategy) for now.

public double calculatePremium(Person person) {
    // Added base premium
    double premium = 5000;
    premium = Rules.applyAgeBasedCalcs(person, premium);
    premium = Rules.applyGenderBasedCalcs(person, premium);
    premium = Rules.applyHealthIssuesBasedCalcs(person, premium);
    premium = Rules.applyGoodHabbitsBasedCalcs(person, premium);
    premium = Rules.applyBadHabbitsBasedCalcs(person, premium);
    return premium;
}

I made all the apply* methods static as I didn't see the need to instantiate a Rules object; there's no state & the method is functional.

Unfortunately, that wasn't well appreciated. He didn't like the static methods & said I broke all the OO principles. He suggested that I should make it more object-oriented. Additionally just because there's a class & interface, doesn't mean it's a strategy pattern.

I didn't understand the comments too well. Could someone review this & let me know of a good way to code this, or just why is my implementation so bad? What's a good way to make it more object-oriented, and what's the right usage of a strategy pattern?

A good OOP design is open for extension and closed for modification.
The single-method implementation is very far from that.

I think the purpose of the exercise is to be able to build a premium calculator from parts,
in a way that you can easily modify the parts that are likely to change.
What are the parts?

The base premium
Calculate percentage modifier based on conditions

There is a list of conditions, with associated percentage increase or decrease

A percentage modifier part is essentially a function that takes a Person and returns a percentage.
You could provide such parts to make it easy to build a calculator, for example:

interface PremiumPercentCalculator extends Function<Person, Integer> {}

PremiumPercentCalculator male(int percent) {
    return person -> person.male ? percent : 0;
}

PremiumPercentCalculator preConditions(int percent) {
    return person -> person.preConditions().size() * percent;
}

PremiumPercentCalculator goodHabits(int percent) {
    return person -> person.habits().stream().filter(Habit::isGood).mapToInt(h -> percent).sum();
}

PremiumPercentCalculator badHabits(int percent) {
    return person -> person.habits().stream().filter(h -> !h.isGood()).mapToInt(h -> percent).sum();
}

Then, using the builder pattern, you could build a calculator from parts like this:

float premium = new Calculator(person, 5000)
    .add(p -> getPremiumPercentByAge(p))
    .add(male(2))
    .add(preConditions(1))
    .add(badHabits(3))
    .add(goodHabits(-3))
    .calculate();

Imagine that there is a UI for an insurance agent,
where he can drag and drop widgets that correspond to condition types like "is male", "pre-conditions", and so on, each with an option to modify the percentage to apply.
Such a UI could be easily built from the parts we provided,
and new parts can be easily added by adding more PremiumPercentCalculator implementations.

Notice that person and the base premium 5000 are constructor parameters,
as they are absolutely essential for the calculator,
so it made sense to make them required up front,
so that the rest of the implementation can count on them to exist without further validation.

You might notice that badHabits and goodHabits iterate over the collection of habits twice.
This might very well be negligible.
But if not, it's easy enough to fix,
by adding another part:

PremiumPercentCalculator habits(int goodPercent, int badPercent) {
    return person -> person.habits().stream().mapToInt(h -> h.isGood() ? goodPercent : badPercent).sum();
}

Admittedly getPremiumPercentByAge is not so easy to make extensible,
this part can certainly be improved,
but I think that was not the most important part of the interview.

Note that although I used lambdas for compact expressions,
the concept doesn't require it,
and although the solution will be more verbose with classes fully written out,
that won't diminish its benefits.


Iterative tree traversal to turn tree into a dictionary and list I am trying to iteratively turn a tree into a list.

For example:

                   c1
              c11        c12
          c111   c112

The tree above should return:

        [
            {'value': 'c1', 'children':
                [
                    {'value': 'c12', 'children': []} ,
                    {'value': 'c11', 'children':
                        [
                            {'value': 'c112', 'children': []},
                            {'value': 'c111', 'children': []}
                        ]
                ]
            }
        ]

Below is my code and here is gist for clearer viewing:

class Node():
    def __init__(self, node_list, value):
        self.node_list = node_list
        self.value = value

def reconstruct_iteratively(root):

    stack = list()

    # A tuple that stores node to be traverse and the layer the node is in
    node_tuple = (root, 0)
    stack.append(node_tuple)

    layer = 0
    pre_layer = 0
    level = dict()

    # This is a postorder traversal
    while len(stack) != 0:
        # This case catch the event that next node is the parent layer and this node is not
        #     a termination node

        if node_tuple[1] < pre_layer - 1:
            parent_node = stack.pop()
            parent_node_dict = {'value': parent_node[0].value, 'children': level.pop(layer, None)}
            pre_layer = layer
            layer -= 1

            if layer in level:
                level[layer].append(parent_node_dict)
            else:
                level[layer] = [parent_node_dict]

            if len(stack) != 0:
                node_tuple = stack[-1]

        # This case catch the event for traversing down to the child
        elif node_tuple[0].node_list is not None:
            for child in node_tuple[0].node_list:
                stack.append((child, layer + 1))
            node_tuple = (node_tuple[0].node_list[-1], layer + 1)
            layer += 1
        # This case catch the event that we are at the termination node
        elif node_tuple[0].node_list is None:
            old_node = stack.pop()
            node_dict = {'value': old_node[0].value, 'children': []}
            node_tuple = stack[-1]

            # Two possible scenario
            # 1. The next node is in the same layer
            # 2. The next node is in the parent layer
            if node_tuple[1] == layer:
                if layer in level:
                    level[layer].append(node_dict)
                else:
                    level[layer] = [node_dict]
            else:
                if layer in level:
                    level[layer].append(node_dict)
                else:
                    level[layer] = [node_dict]

                parent_node = stack.pop()
                parent_node_dict = {'value': parent_node[0].value, 'children': level.pop(layer, None)}
                pre_layer = layer
                layer -= 1

                if layer in level:
                    level[layer].append(parent_node_dict)
                else:
                    level[layer] = [parent_node_dict]

                if len(stack) != 0:
                    node_tuple = stack[-1]

    return [parent_node_dict]

I am looking for advices to make the code clearer and more efficient. Any comment would be appreciated.

Thanks

Some suggestions:

Empty lists are False, so while len(stack) != 0 is the same as while stack.  None is also False, so you can check for empty lists and None values at the same time.

You can use dict.setdefault to get a value and set it to a default (in your case an empty list). if it isn't already there.

You can convert an append loop to extend with a generator expression, or better yet just a zip.

Your last two elif tests are mutually exclusive, so the last can be an else.

The if node_tuple[1] == layer: test does the same thing in the first line of both cases.  I  moved that out of the if test, but if they are supposed to do something different you should fix that yourself.

pre_layer always has its value subtracted by one, so it is easier to subtract one before defining it.

You always set node_tuple to stack[-1] if stack is non-empty, so you can move that out  of the if test entirely.  And if you put it at the beginning of the loop, you can avoid the if test entirely.  You can simplify this further by only getting it if you need it.

def reconstruct_iteratively(root):
    # A tuple that stores node to be traverse and the layer the node is in
    stack = [(root, 0)]

    layer = 0
    pre_layer = -1
    level = dict()

    # This is a postorder traversal
    while stack:
        node_value, node_layer = stack[-1]
        node_list = node_value.node_list
        # This case catch the event that next node is the parent layer and
        # this node is not a termination node
        if node_layer < pre_layer:
            parent_node = stack.pop()
            parent_node_dict = {'value': parent_node[0].value,
                                'children': level.pop(layer, None)}
            layer -= 1
            pre_layer = layer

            level.setdefault(layer, []).append(parent_node_dict)

        # This case catch the event for traversing down to the child
        elif node_list:
            stack.extend(zip(node_list, [layer+1]*len(node_list)))
            layer += 1

        # This case catch the event that we are at the termination node
        else:
            # Two possible scenario
            # 1. The next node is in the same layer
            # 2. The next node is in the parent layer
            level.setdefault(layer, []).append({'value': node_value.value,
                                                'children': []})
            del stack[-1]
            if stack[-1][1] != layer:
                parent_node_dict = {'value': stack.pop()[0].value,
                                    'children': level.pop(layer, None)}
                layer -= 1
                pre_layer = layer

                level.setdefault(layer, []).append(parent_node_dict)

    return [parent_node_dict]


Passing an NSManagedObjectContext I've been experimenting with how to pass my managed object context throughout my application in Core Data and I came across an approach that I'd like reviewed.

I've been creating a protocol for all of my View Controllers that need a managed object context to implement:

protocol ManagedObjectContextProperty {
    var managedObjectContext: NSManagedObjectContext! { get set }
}

Then I create an extension to that protocol to assert that the context does indeed exist:

extension ManagedObjectContextProperty {
    func checkManagedObjectContext(name: String) {
        if managedObjectContext == nil {
            assertionFailure("\(name) is missing the managed object context.")
        }
    }
}

Finally, I create an extension that fetches my context from the app delegate and add's it to my managedObjectContext variable:

extension UIViewController {
    func getManagedObjectContext<T : UIViewController where T : ManagedObjectContextProperty>(controller: T) {
        var controller = controller
        let appDelegate = UIApplication.shared().delegate as! AppDelegate
        controller.managedObjectContext = appDelegate.managedObjectContext
    }
}

Then in my View Controller I simply do this:

class ViewController: UIViewController, ManagedObjectContextProperty {

    var managedObjectContext: NSManagedObjectContext!

    override func viewDidLoad() {
        getManagedObjectContext(controller: self)
        checkManagedObjectContext(name: "ViewController")
    }
}

Is this approach is against the general convention? If so, why?

Ok, I think I've found an answer that I can appreciate. From Marcus Zarra's blog:

  A view controller typically shouldn’t retrieve the context from a
  global object such as the application delegate. This tends to make the
  application architecture rigid. Neither should a view controller
  typically create a context for its own use. This may mean that
  operations performed using the controller’s context aren’t registered
  with other contexts, so different view controllers will have different
  perspectives on the data.

source: http://www.cimgf.com/2011/01/07/passing-around-a-nsmanagedobjectcontext-on-the-iphone/

Look's like I'll go back to passing the managed object context via Segue.


Optimizing matrix multiplication algorithm I have implemented my own version of algorithm to multiply two matrices together and I need someone who know what they are doing to see if there is anything that can be done in a more optimal way. I am also keen on understanding how I can make it robust such that it doesn't crash when non-rectangular matrices are passed as argument i.e. matrix that has uneven number of elements in different Vds it holds.
I am particularly interested in function matrixDot in the code bellow, everything else is only to show how I am using it in my project).
#include "iostream"
#include <vector>

#define LOG(m) std::cout << m << std::endl

struct Vd
{
    std::vector<double> v;
};

struct Md
{
    std::vector<Vd> m;

    //fill matrix with num
    void fill(unsigned const int rows, unsigned const int cols, const double num)
    {
        m.clear();
        for (unsigned int i = 0; i < rows; i++)
        {
            Vd tempVec;
            for (unsigned int j = 0; j < cols; j++)
            {
                tempVec.v.push_back(num);
            }
            m.push_back(tempVec);
        }
    }

    friend std::ostream& operator << (std::ostream& out, const Md& mat)
    {
        out << "[" << std::endl << std::endl;
        for (unsigned int i = 0; i < mat.m.size(); i++)
        {
            out << "[";
            for (unsigned int j = 0; j < mat.m[i].v.size(); j++)
            {
                if (j % mat.m[i].v.size() == mat.m[i].v.size() - 1)
                    out << mat.m[i].v[j] << "]" << std::endl << std::endl;
                else
                    out << mat.m[i].v[j] << ", ";
            }
        }
        out << "]" << std::endl;
        return out;
    }
};

inline void matrixDot(const Md& m1, const Md& m2, Md& outm)
{
    if (m1.m[0].v.size() && m2.m.size())
        if (m1.m[0].v.size() != m2.m.size())
        {
            LOG("Shape mismatch: " << "matrix1 columns: " << m1.m[0].v.size() << ", " << "matrix2 rows: " << m2.m.size());
            throw std::exception();
        }

    unsigned int m1x = 0; unsigned int m1y = 0; unsigned int m2y = 0; //m2x = m1y

    while (outm.m.size() < m1.m.size())
    {
        Vd tempv;
        while (tempv.v.size() < m2.m[0].v.size())
        {
            double total = 0.0;
            while (m1x < m1.m[0].v.size())
            {
                total += m1.m[m1y].v[m1x] * m2.m[m1x].v[m2y];
                m1x++;
            }
            tempv.v.push_back(total);
            m1x = 0;
            m2y < m2.m[0].v.size() - 1 ? m2y++ : m2y = 0;
        }
        m1y < m1.m.size() - 1 ? m1y++ : m1y = 0;
        outm.m.push_back(tempv);
    }
}

int main()
{
    Md mat1;
    mat1.fill(5, 2, 1.0);
    Md mat2;
    mat2.fill(2, 6, 2.0);
    Md mat3;
    matrixDot(mat1, mat2, mat3);
    std::cout << mat3;
}

I have to admit I'm a bit confused, you did the hard parts and have a question on the easy parts? I might be misunderstanding your question.

I am also keen on understanding how I can make it robust such that it doesn't crash when non-rectangular matrices are passed as argument i.e. matrix that has uneven number of elements in different Vds it holds.

You could validate that you have a well formed matrix by
inline bool isValid(const Md& mat)
{
    if (mat.m.size())
    {
        int size = mat.m[0].v.size();
        for (int i = 1; i < mat.m.size(); i++) {
            if (size != mat.m[i].v.size())
            {
                return false;
            }
        }
    }

    return true;
}

and incorporating it into the matrixDot function for validation similar to the shape validation you have right now
    if (m1.m[0].v.size() && m2.m.size())
        if (m1.m[0].v.size() != m2.m.size())
        {
            LOG("Shape mismatch: " << "matrix1 columns: " << m1.m[0].v.size() << ", " << "matrix2 rows: " << m2.m.size());
            throw std::exception();
        }

    if (!isValid(m1))
    {
        LOG("Invalid matrix :: " << std::endl);
        std::cout << m1;
        throw std::exception();
    }

    if (!isValid(m2))
    {
        LOG("Invalid matrix :: " << std::endl);
        std::cout << m2;
        throw std::exception();
    }

Any optimizations I can think of are around utilizing std::array instead of std::vector as you are aware of row and column lengths at time of creation.


Get json format from sqlalchemy class I have written a small tool to generate a json file from sqlalchemy models files to automate the tedious procedure of writing all the sql raws in a json format. The script works, but there are few parts, see get_modules function, that can be improved. I would be glad to have a review of this code. Thanks

# -*- coding: utf-8 -*-

import os
import sys
import glob

PATH = sys.argv[1]
output_filename = 'en.json'

intro = ("""
{
  "sidebar": {
      "overview": "Overview",
  },
  "about": {
      "title": "title"
    },
    "welcome": {
        "title": "Welcome to the prototype app!"
    },

""")

def get_fields(input_filename):
    """It returns all fields of a model in a list"""
    fields = []
    endpoint = None
    with open(PATH+input_filename, 'r') as inputfile:
        for line in inputfile:
            if '__plural_name__' in line:
                endpoint = line.split('=')[1].strip().replace("'","")
            if '= Column' in line:
                fields.append(line.split()[0])
    return endpoint, fields

def get_modules():
    """It returns a list of all models"""
    files = []
    cwd = os.getcwd()
    os.chdir(PATH)
    files = glob.glob("*.py")
    os.chdir(cwd)
    return files

if __name__ == "__main__":

    models = get_modules()
    target = open(output_filename, 'w')
    target.write(intro)
    for m in models:
        if m != '__init__.py':
            end_name, attrs = get_fields(m)
            target.write('  "{}": {{\n'.format(end_name))
            target.write('    "title": "{}",\n'.format(end_name))
            target.write('    "fields": {\n')
            for a in attrs:
                target.write('      "{}": "{}",\n'.format(a,a))
            target.write('    }\n')
            target.write('  },\n')
    target.close()

You should take advantage of what is already available. For example, the json module. Instead of putting the string together manually, use json.dumps.  Python doesn't have a SQLAlchemy library built-in, but you can get one on PyPI, just pip install SQLAlchemy. Using this, you can browse https://stackoverflow.com/q/5022066/5827958 to see some simple ways of converting SQLAlchemy to JSON.

You define files = [] in get_modules(). That's unnecessary. Python does not need you to declare variables before using them. Just save yourself that tiny bit of processing time, and take it out.

You can also make use of os.path, a built-in pointer to an OS-specific filesystem path module. In get_modules(), you can simplify down to:

def get_modules():
    return glob.glob(os.path.join(PATH, "*.py"))

Since that now returns an absolute path instead of a relative one, you can now use merely input_filename instead of PATH+input_filename in get_fields().

I would also define get_modules as taking an argument for its path. That way, an importing module can use it for whatever path it wants, but your if __name__ == '__main__' block will give it sys.argv[1]. Since I just changed your function to using absolute paths, get_fields() doesn't need to know about PATH at all. On the side, you shouldn't be reading sys.argv unless __name__ == '__main__' anyway.

The purpose of your file is to convert, but an importing module has no access to the conversion code. I would suggest defining a function for the conversion, and then let the if __name__ == "__main__" block call that function with sys.argv[1] as an argument.

I'm glad that you use a with block in get_fields(), but I see you aren't in your main code. This could cause unexpected problems if something unexpected happened. You should open a file without a with block only in very rare cases.


I/O-TxtFile Line Distance Program I am trying to write a lab/program that loops through a .txt input file. Reads an entire line of data as a string, then splits that string into an array delimited by a space. If there are 4 items in the resulting array, and each item is a valid double, I want it to determine the distance between the points & return info to a separate output.txt file.

RLabs.java

import java.io.*;
import java.util.Scanner;

public class RLabs {

    public static void main(String[] args) throws FileNotFoundException {
        double dealtaX, deltaY, distance,x1, y1, x2, y2;
        String Line, item1, item2, item3, item4;
        String[] Sect;
        Scanner inFile;
        PrintWriter outFile;

        System.out.println("I/O File Line Distance Program Starting. Opening file...");
        System.out.println("Please ensure data is entered per line in the format of (X1 Y1 X2 Y2).");
        inFile = new Scanner(new File("input.txt"));
        outFile = new PrintWriter(new File("countresults.txt"));

        while(inFile.hasNext()){

            Line = inFile.nextLine();
            Sect = Line.split(" ");
            if(Sect.length == 4) {
                item1 = Sect[0];
                item2 = Sect[1];
                item3 = Sect[2];
                item4 = Sect[3];

                if(testDoubleLow(item1,0)&&(testDoubleLow(item2,0)&&(testDoubleLow(item3,0)&&(testDoubleLow(item4,0))))){
                    x1 = Double.parseDouble(item1);
                    y1 = Double.parseDouble(item2);
                    x2 = Double.parseDouble(item3);
                    y2 = Double.parseDouble(item4);
                    dealtaX = (x2-x1);
                    deltaY = (y2-y1);

                    distance = Math.sqrt(Math.pow(deltaY,2)+ Math.pow(dealtaX,2));
                    outFile.printf(Line);

                    outFile.printf(" The distance between (%1.1f,%1.1f) and (%1.1f,%1.1f) is %1.1f\n", x1,y1,x2,y2,distance);

                }
                else {
                    outFile.println("Line did not correct data");
                }

            }
            else {
                outFile.println("Line did not have 4 item(s)");
            }
        }

        outFile.close();
        inFile.close();

        System.out.println("Done...");
    }

        private static boolean testDoubleLow(String token, double l){

            if(testDouble(token)) {
                double num = Double.parseDouble(token);
                if(num > l ) {
                    return true;
                }
            }

            return false;
        }

        private static boolean testDouble(String s) {
            try {
                Double.parseDouble(s);
                return true;
            }
            catch(Exception e) {
                return false;

    }

    }
}

Input data I'm using

17.2 23.2 15.6 17.76

1

frog

123 frog 2 98

22 99 19 76

-13 03 92 03

l 123 03 96

1293.333333333333333 3 4 111111111111111111

2 1 1 2

21.4518345 94 21 mom

12 67 tti 30

128 30 -2 69

670 20 49 100

1230 230 304 500 60 30 60 606 59

Output from program

17.2 23.2 15.6 17.76 The distance between (17.2,23.2) and (15.6,17.8) is 5.7

Line did not have 4 item(s)

Line did not have 4 item(s)

Line did not have correct data

22 99 19 76 The distance between (22.0,99.0) and (19.0,76.0) is 23.2

Line did not have correct data

Line did not have correct data

1293.333333333333333 3 4 111111111111111111 The distance between (1293.3,3.0) and (4.0,111111111111111104.0) is 111111111111111104.0

2 1 1 2 The distance between (2.0,1.0) and (1.0,2.0) is 1.4

Line did not have correct data

Line did not have correct data

Line did not have correct data

670 20 49 100 The distance between (670.0,20.0) and (49.0,100.0) is 626.1

Line did not have 4 item(s)

Object-Orientated

your code is very procedural (honestly, it's just one long main methode) not very object-orientated... seperate your code into seperate concern(Segregation of concerns), at least a kind of DistanceParser class and a ErrorHandler class. Doing this make your ready open for the open-closed principal

DistanceParser fileParser = DistanceParser.fileInstance(inputFile);   //applying dependecy injection
fileParser.setErrorHandler(new FileErrorHandler(logFile));
fileParser.parse();

if you would apply that logic you were able to create easily any other parser (maybe DistanceParser.inputStreamInstance(inputStream) ), you could easily replace your FileErrorHandler with any other ErrorHandler and you were able to test your functionality with a proper unit test

@Test
DistanceParser fileParser = DistanceParser.fileInstance(invalidInput);
ErrorHandler errorHandlerMock = mock(ErrorHandler.class);
fileParser.setErrorHandler(errorHandlerMock );
fileParser.parse(); //errors during execution are handled by the errorHandler (handleError())
verify(errorHandlerMock , atLeastOnce()).handleError();

You know you could implement interfaces on your Parser class and your Errorhandler class, so you can apply Liskovs Substitution Principal ...

Tell, don't ask

another minor issue is naming of your methods, i'm still wondering about your testDoubleLow() methode. if you would use proper names your code would be far more readable.

misleading naming here:

testDouble(String s) should be named into isDouble(String doubleAsString)
testDoubleLow(String token, double l) should be named into isBiggerThan(String doubleAsString, double minimalValue)


Creating Original Database Algorithm--Login System For fun, I have been making a very basic login system in Python. I was going to look up an algorithm or something for user info storage and transfer, but then decided it would be more fun to come up with and implement it myself. What I would like to know is:

Is there anyway to make it secure (i.e.-encrypting the password without revealing the algorithm in the file)?
Can the algorithm I used be made any more efficient?
Is it cleanly written? I tried to take into account the comments on my last question regarding simplifying code and any comments about its readability would be appreciated--by secondary goal is beautiful code.

#This program will do a couple of things.
#The first time it is run, it will request that you add a password. Once you do, it will change the check_e$
#Every subsequent time, it will check the password in the other file and then match user input.
#If user input is correct it will display a joke.
#If user input is incorrect it will exit the program.

#Function to help the user pick a password
def pick_password():
        file = '/home/vhx/Documents/code/pydata_test/password_dbs/pswd.txt'
        print 'Please pick a password.'
        password = raw_input()
        target = open(file, 'w')
        target.write(password)
        file = '/home/vhx/Documents/code/pydata_test/password_dbs/existence_check.txt'
        target = open(file, 'w')
        target.write('YES')
#       file.close()

#Function to check the password with the password located in pswd.txt
def password_check():
        file = '/home/vhx/Documents/code/pydata_test/password_dbs/pswd.txt'
        pwd_check = open(file).read()
        userpass = raw_input('Please input a password.\n')
        if userpass == pwd_check:
                print 'Password accepted!'
                print 'Ready for the joke?'
                raw_input()
                print 'Why did the fly fly? Because the spider spied her!'
        elif userpass != pwd_check:
                print 'Sorry, wrong password.\n'
                exit()
        else:
                print 'Invalid syntax.'
                exit()

#location of password existence check file
EC = '/home/vhx/Documents/code/pydata_test/password_dbs/existence_check.txt'
PWD = '/home/vhx/Documents/code/pydata_test/password_dbs/pswd.txt' #Location of password file

pswd_exist = open(EC).read() #Checking to see if the password exists
if pswd_exist == 'YES':
        pass
else:
        pick_password() #If it doesn't, user will pick a password

#Checking for password
password_check()

Note: If you want to run it, it requires two files --pswd.txt and existence_check.txt--which you then must link (and replace all necessary file locations).

If you input the correct password, the program tells a joke, though whether its funny is debatable.

It looks like passwords are stored in plain text form in the pswd.txt file. Never ever store passwords in plain text form. Store passwords salted and cryptographically hashed. That way, if an attacker gains access to the file, he still has to crack the password, which can be extremely difficult if the password is strong enough.

To verify a password, apply the same algorithm to the user input as used when creating the salted and cryptographically hashed version. The result will only match the stored password if the user entered the correct password.

@Boris left a great comment, quoting it verbatim:

  Encrypted passwords are a bad idea as the encryption key needs to be stored in the code, then then it's just a question of looking at the code to gain the key. Hashing is irreversible and using a good, random, unique, salt per password and a purpose built cryptographic hashing algorithm is the only acceptable way of storing passwords - the best idea is to use something like bcrypt which is industry standard and rolls all this together.


Is it OK to use while ((line = r.readLine()) != null) construct? I want to refactor the following code because I don't feel comfortable about using assignment inside comparison operator. It looks like pretty idiomatic C, but do you think this is a good practice in Java?

private void demoA(BufferedReader reader) throws IOException {
    String line = null;
    while ((line = reader.readLine()) != null) {
        doSomething(line);
    }
}

Here is an alternative.

private void demoB(BufferedReader reader) throws IOException {
    String line = reader.readLine();
    while (line != null) {
        doSomething(line);
        line = reader.readLine();
    }
}

UPDATE: I've stumbled across a similar question asked couple years ago. It seems that opinions on whether it's OK or not are divided. However, both Guava and Commons IO provide alternative solutions for this issue. If I had any of these libs in the current project, I'd probably use them instead.

Assignment inside a condition is ok in this case, as the assignment is surrounded by an extra pair of parentheses – the comparison is obviously != null, there is no chance that we wanted to type line == reader.readLine().

However, a for loop might actually be more elegant here:

for (String line = reader.readLine(); line != null; line = reader.readLine()) {
    doSomething(line);
}

Alternatively, we could do this which also restricts the scope of line as with the for-loop, and additionally eliminates unnecessary repetition:

while (true) {
    final String line = reader.readLine();
    if (line == null) break;

    doSomething(line);
}

I like this solution most because it doesn't mutate any variables.


Finding difference between date now and last comment I want users to wait 9 minutes after their last comment. I have written this code. It works but do you think this is practical? Is there a simpler solution?

$now = new DateTime( date( 'Y-m-d H:i:s' ) );
$lastPost = new DateTime( '2016-01-08 13:16:59' );
$diff = $lastPost->diff( $now );

if ( $diff->format( '%Y-%m-%d %H' ) == '00-0-0 00' ) {
    if ( $diff->format( '%i' ) < 9 ) {
         echo $diff->format( '%i minuts left or ahead' );
    }
}

Current time is easily obtained simply with new DateTime(), no need to specify any formatting (default is now). Also you don't need to format dates to compare strings because DateTime itself is comparable. All together:

$now = new DateTime();
$lastPostTime = new DateTime('2016-01-08 13:16:59');
$notEnabledBefore = lastPostTime->add(new DateInterval('PT9M'));

if (now < notEnabledBefore) {
    $timeToWait = notEnabledBefore->diff(now);
    echo $timeToWait->format('%i minutes left or ahead');
}


Is there any way to shorten up this Facebook API call? Right now I am working on an AJAX call that grabs a random photo that a Facebook user is tagged in and then grabs the large source for it. Is there any way to speed this up or at least distill the call into one query?

var query1 = encodeURI("SELECT pid,xcoord,ycoord FROM photo_tag WHERE subject={{mystery.uid}} order by rand() LIMIT 1");
var query2 = encodeURI("SELECT src_big FROM photo WHERE pid=");

function get_pic() {
        var l = $.get("https://graph.facebook.com/fql?q="+query1,
                {access_token: "{{token}}"},
                function(data1) {
                        $.get("https://graph.facebook.com/fql?q="+query2+data1.data[0].pid,
                        {access_token: "{{token}}"},
                        function(data2) {
                                $("#mystery_img").html('');
                                var top = $("#mystery_img img").offset().top + Math.floor(data1.data[0].ycoord);
                                var left = $("#mystery_img img").offset().left + Math.floor(data1.data[0].xcoord);
                                var img = $(document.createElement('img')).attr({src: "/static/images/curved_arrow.gif"})
                                    .css('position', 'absolute')
                                    .css('top', top)
                                    .css('left', left);
                                    $("#mystery_img").append(img);
                        },
                        "json");
                },
                "json");
    }

I haven't worked with Facebook's API using queries like this, but I know SQL inside and out, and this screams INNER JOIN to me.

var joinedQuery = "SELECT Photo_Tag.pid,Photo_Tag.xcoord,Photo_Tag.ycoord,Photo.src_big FROM photo_tag AS Photo_Tag INNER JOIN photo AS Photo ON Photo_Tag.pid = Photo.pid WHERE Photo_Tag.subject = {{mystery.uid}} ORDER BY RAND() LIMIT 1;";

Also, the use of ORDER BY RAND() is going to be inherently slow (especially with a system as large as Facebooks), so might I recommend something more like this:

var joinedQuery = "SELECT Photo_Tag.pid,Photo_Tag.xcoord,Photo_Tag.ycoord,Photo.src_big FROM photo_tag AS Photo_Tag INNER JOIN photo AS Photo ON Photo_Tag.pid = Photo.pid INNER JOIN (SELECT (MAX(pid) * RAND()) AS pid FROM photo_tag WHERE Photo_Tag.subject = {{mystery.uid}}) AS MaxId ON Photo_Tag.pid = MaxId.pid WHERE Photo_Tag.subject =  {{mystery.uid}} ORDER BY Photo_Tag.pid ASC LIMIT 1";

Written to be more readable, that query is:

SELECT Photo_Tag.pid
    ,Photo_Tag.xcoord
    ,Photo_Tag.ycoord
    ,Photo.src_big
FROM photo_tag AS Photo_Tag
INNER JOIN photo AS Photo ON Photo_Tag.pid = Photo.pid
INNER JOIN (
    SELECT (MAX(pid) * RAND()) AS pid
    FROM photo_tag
    WHERE Photo_Tag.subject = {{mystery.uid}}
) AS MaxId ON Photo_Tag.pid = MaxId.pid
WHERE Photo_Tag.subject = {{mystery.uid}}
ORDER BY Photo_Tag.pid ASC
LIMIT 1;

Using ORDER BY RAND() requires querying every row to build a new "randomness" ID, creating and populating a temporary table with this newly randomized result set, and then querying from that table to bring back your row. For large datasets, this can be very slow.

By instead JOINing the table back onto itself with random pids generated, then ORDERing and LIMITing that dataset, it can result in a much faster query. This article explains in (enormous) detail what I mean, but also shows that a query such as the one above will be faster with a table holding as little as 100 rows, and will be no slower than the original query in any case.

Even if you don't get rid of ORDER BY RAND(), your reduction to a single query should speed up your call.


Regular expression to parse guid and other attributes I have this regular expression that extracts guid and couple of other attributes like name, type and version. Please review the regex for any optimizations and improvements.

Strings are always in the pattern of

/publication/guid/type/name;version=1234

regex

(([a-f0-9]+\-)+[a-f0-9]+)\/(.*?)\/(.*?);version=(\d*)

Test records

Extract bold pieces from string.

/publication/d40a4e4c-d6a3-45ae-98b3-924b31d8712a/collection/content1;version=1520623346833

Expected output:

d40a4e4c-d6a3-45ae-98b3-924b31d8712a
collection
content1
1520623346833

/publication/d40a4e4c-d6a3-45ae-98b3-924b31d8712a/article/testContent;version=1520623346891

Expected output

d40a4e4c-d6a3-45ae-98b3-924b31d8712a
article
testContent
1520623346891

Code

Language is F#, but the regex works in C# too.  Furthermore, I would like to use the same regex in Node.js, so I would like the regex to be language agnostic.

let matchEntity (m: Match) =
    { id= m.Groups.[1].Value; eType = m.Groups.[3].Value; name= m.Groups.[4].Value; version = m.Groups.[5].Value}

let regex = new Regex("(([a-f0-9]+\-)+[a-f0-9]+)\/(.*?)\/(.*?);version=(\d*)")
matchEntity regex.Match "/publication/d40a4e4c-d6a3-45ae-98b3-924b31d8712a/collection/content1;version=1520623346833"

TL;DR; list of adjustments:

.NET RegEx support named capture groups. make use of them.
Make use of the GUID format specification.
Simplify retrieval with non-capturing groups.
Make assumptions explicit in character classes. Prefer negated chargroups to non-greedy matching

I propose the following regular expression instead:

(?<guid>[a-f0-9]{8}(?:\-[a-f0-9]{4}){3}\-[a-f0-9]{12})\/(?<type>[^\/]+)\/(?<name>[^;]+);version=(?<version>\d*)

While this regex is somewhat longer it matches both examples in 62 steps (as opposed to 117). This may only seem like a minor improvement, but it's not all that happens:

This regex uses named capturing groups that allow a much more natural and clear pattern extraction. Instead of accessing groups by magic indices, you can access them by name and the construction of matchEntity is accomplished as follows:

let matchEntity (m: Match) =
    { id= m.Groups.["guid"].Value;
      eType = m.Groups.["type"].Value;
      name= m.Groups.["name"].Value;
      version = m.Groups.["version"].Value }

Last but not least this regex also does not match incorrect GUID specifications


Float to binary conversion I wrote a program to convert float to it's binary IEEE754 representation in C.
Any improvements to this is appreciated
#include <stdio.h>
#include <stdlib.h>
void binary(float num) {
    int n = *(int *) &num; //Evil bit hack from Quake3 Q_sqrt function
    int size = sizeof(n) * 8;
    char *s = malloc(1 + size); //A string which can hold 32 characters
    s[size] = '\0';
    int index = size - 1;
    while(index >= 0) {
        if (n & 1)
            *(s + index) = '1';
        else
            *(s + index) = '0';
        n >>= 1;
        index--;
    }
    printf("%s\n", s);
    free(s);
}

int main(void) {
    float num;
    scanf("%f", &num);
    binary(num);
}

IEEE754 conformance
Compilers aren't required to support IEEE754 decimals. Most implementations do, but regardless, it's better to check for maximum portability. If you're using C99 or later, the macro __STDC_IEC_559__ is defined whenever IEEE754 is supported.

sizeof(int)
Similarly, int isn't guaranteed to be 32 bits. The compiler only guarantees a minimum size of 16 bits. You can use the fixed width integer types defined in stdint.h i.e. uint32_t or int32_t.

Use size_t instead of int
The proper type for sizeof expression is size_t. As a general use, any variable representing size or index should be size_t.

Since you're not using s outside of the binary function, you can just allocate a fixed an array of size 33 on the stack instead of on the heap using malloc.
static const size_t num_bits = 33;
char s[num_bits];

*(s + index) is just fancy way to write s[index]. The latter is much cleaner, IMO and much more easier to understand for someone who isn't familiar with pointer arithmetic.


Get application credentials from linked array obtained from Cloud Foundry API I am new to Java and I use the following code to retrieve parameters
from a linked hash map from the Cloud Foundry API. My question is if there is a better way to do it in Java?

public HashMap<String, String> getUserData(String Url, String org, String space, String app) throws MalformedURLException {
        URL endpoint = new URL(Url);
        CloudFoundryOperations ops = sm.getCurrentUserCfClient(endpoint, org, space);

        GetApplicationEnvironmentsRequest request = GetApplicationEnvironmentsRequest.builder()
                .name(app)
                .build();

        ApplicationEnvironments environments = ops.applications()
                .getEnvironments(request)
                .block();

Map<String, Object> mapSystemProvided =  environments.getSystemProvided();

    LinkedHashMap<String, Object> SERVICES = (LinkedHashMap<String, Object>) mapSystemProvided.get("SERVICES");
    ArrayList<Object> apps = (ArrayList<Object>) SERVICES.get("apps");
    LinkedHashMap<String, Object> appsList = (LinkedHashMap<String, Object>) apps.get(0);
    LinkedHashMap<String, Object> credentials = (LinkedHashMap<String, Object>) appsList.get("credentials");

    HashMap<String, String> parameters = new HashMap<String, String>();
    parameters.put(ID, credentials.get(ID).toString());
    parameters.put(DEFAULTS, credentials.get(DEFAULTST).toString());

    return parameters;

Consider breaking this into more, reusable methods.  E.g.

public Map<String, Object> getCredentials(String url, String org, String space, String app)
        throws MalformedURLException {

    GetApplicationEnvironmentsRequest request = GetApplicationEnvironmentsRequest.builder()
            .name(app)
            .build();

    Map<String, Object> mapSystemProvided = sm
            .getCurrentUserCfClient(new URL(url), org, space)
            .applications()
            .getEnvironments(request)
            .block()
            .getSystemProvided();

    Map<String, Object> SERVICES = (LinkedHashMap<String, Object>) mapSystemProvided.get("SERVICES");
    List<Object> apps = (ArrayList<Object>) SERVICES.get("apps");
    Map<String, Object> appsList = (LinkedHashMap<String, Object>) apps.get(0);

    return (LinkedHashMap<String, Object>) appsList.get("credentials");
}

public Map<String, String> getUserData(String url, String org, String space, String app)
        throws MalformedURLException {

    Map<String, Object> credentials = getCredentials(url, org, space, app);

    Map<String, String> parameters = new HashMap<>();
    parameters.put(ID, credentials.get(ID).toString());
    parameters.put(DEFAULTS, credentials.get(DEFAULTS).toString());

    return parameters;
}

Your original method was mostly getting the credentials.  If we move that into another method, it actually leaves this method rather simple and straightforward.

I moved what had been ops into the mapSystemProvided chain, as it wasn't used for anything else.

As a general rule, it is easier to have interfaces as types rather than implementations.  In this case, it makes the lines fit in the code window without scrolling on my screen.  That may not matter in your IDE though.  Mostly I just find it a good habit.  Most all of the time, you're just using the interface methods anyway.  Using the interface as the type helps enforce that and allows you to switch implementations easily.  Even if it doesn't actually work that way here.

I think that you could change the casts to the interface as well.  I didn't try that here, as I don't work with casting enough to be sure.  And of course there isn't enough context here to actually test this code.  But if I was working on this code, I'd try that to decouple it even more from the implementations.

I'd probably change SERVICES to services as I changed Url to url.  That way they fit the standard Java naming scheme.

I would consider changing the entity model.  For example, why is credentials hidden multiple levels deep inside of the getSystemProvided() result?  Maybe there's a reason for that, but it's hard to see with just this code.  It seems overly abstracted, at least when viewed in isolation like this.

Your indentation is funky.  I fixed it to be consistent in the alternative versions.  Perhaps that was just a copy & paste error.  There was also a missing } that I added.


Exposing a simple struct using Rust FFI I am trying to expose a simple struct via FFI. Is this the best way to do it?

I am not 100% sure about the get_text function. This is taking the string out of the struct and cloning it before returning a pointer to it. I suspect this is going to leak. Whats the best idiomatic way around this? Do I need to provide a free_string function? Is there a way to just return a pointer into the Rust string?

use std::mem;
use std::os::raw::{c_char, c_void};
use std::ffi;

#[derive(Debug)]
pub struct SomeData {
    text : String,
    number : usize,
}

#[no_mangle]
/// Make the structure. Box it to stick on the heap and return a pointer to it.
/// This absolves Rust from having to deal with the memory, so it is the callers
/// responsibility to free it by calling drop_data.
pub extern "C" fn make_some_data (text: *const c_char, number: usize) -> *mut c_void {
    let cstr = unsafe {
        assert!(!text.is_null());
        ffi::CStr::from_ptr(text)
    };

    let data = cstr.to_str().ok().map(|utf8_str| {
        Box::into_raw(Box::new(SomeData {
            text: String::from(utf8_str),
            number,
        }))
    }).unwrap();

    data as *mut c_void
}

#[no_mangle]
/// Returns the text field of the struct.
pub extern "C" fn get_text(data: *mut c_void) -> *mut c_char {
    let data = unsafe {
        assert!(!data.is_null());
        Box::from_raw(data as *mut SomeData)
    };
    let text = data.text.clone();
    mem::forget(data);
    ffi::CString::new(text).unwrap().into_raw()
}

#[no_mangle]
/// Returns the number field of the struct.
pub extern "C" fn get_number(data: *mut c_void) -> usize {
    let data = unsafe {
        assert!(!data.is_null());
        Box::from_raw(data as *mut SomeData)
    };
    let number = data.number;
    mem::forget(data);
    number
}

#[no_mangle]
/// Frees the memory.
pub extern "C" fn drop_data (data: *mut c_void) {
   mem::drop(data);
}

First of all, you should always run clippy:

error: this public function dereferences a raw pointer but is not marked `unsafe`
  --> src/lib.rs:20:29
   |
20 |         ffi::CStr::from_ptr(text)
   |                             ^^^^
   |
   = note: #[deny(clippy::not_unsafe_ptr_arg_deref)] on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref

error: calls to `std::mem::drop` with a value that implements Copy. Dropping a copy leaves the original intact.
  --> src/lib.rs:60:4
   |
60 |    mem::drop(data);
   |    ^^^^^^^^^^^^^^^
   |
   = note: #[deny(clippy::drop_copy)] on by default
note: argument has type *mut std::ffi::c_void
  --> src/lib.rs:60:14
   |
60 |    mem::drop(data);
   |              ^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#drop_copy

drop(data) is indeed totally useless here pointer don't have drop implementation, you must use Box::from_raw(data as *mut SomeData) (BTW you didn't check for null here hope this is what you want). I also advise you to mark these function as unsafe.

For the style there is two sides as you seem to require the pointer is no null:

You could assume the pointer is always no null, add only a debug panic
You could panic if the pointer is null

I'm in favor of the first option but it's as you like, if you take the second option I advice to use Option<&T> this will prevent you to forget to check if the pointer is null, it's guarantee by Rust that Option<&c_char> will be None for null pointer value.

Your get_number() and get_text() use Box::from_raw(data as *mut SomeData) for nothing, just use directly the pointer or transform it to reference.

I'm unsure void pointer is necessary.

  Do I need to provide a free_string function? Is there a way to just return a pointer into the Rust string?

yes of course you must use from_raw()

Final code:

use std::ffi;
use std::os::raw::{c_char, c_void};

#[derive(Debug)]
pub struct SomeData {
    text: String,
    number: usize,
}

#[no_mangle]
pub unsafe extern "C" fn make_some_data(text: Option<*const c_char>, number: usize) -> *mut c_void {
    ffi::CStr::from_ptr(text.unwrap())
        .to_str()
        .map(|str| {
            Box::into_raw(Box::new(SomeData {
                text: str.to_string(),
                number,
            }))
        })
        .unwrap() as *mut c_void
}

#[no_mangle]
pub unsafe extern "C" fn get_text(data: Option<&c_void>) -> *mut c_char {
    let data = my_real_data(data);
    ffi::CString::new(data.text.clone()).unwrap().into_raw()
}

#[no_mangle]
pub unsafe extern "C" fn get_number(data: Option<&c_void>) -> usize {
    let data = my_real_data(data);
    data.number
}

unsafe fn my_real_data(data: Option<&c_void>) -> &SomeData {
    &*(data.unwrap() as *const std::ffi::c_void as *const SomeData)
}

#[no_mangle]
pub unsafe extern "C" fn drop_data(data: Option<*mut c_void>) {
    Box::from_raw(data.unwrap() as *mut SomeData);
}

#[no_mangle]
pub unsafe extern "C" fn drop_text(data: Option<*mut c_char>) {
    ffi::CString::from_raw(data.unwrap());
}