Unit testing Roman-Arabic-Roman numeric converter As you can see here, I wrote the methods public static int romanToArabic(String number) and public static String arabicToRoman(int number) to convert roman numbers to arabic numbers and vice versa.

I now wrote a unit-testing-class to test these methods. It is my first attempt to write a (real) unit test, so I would like to know your opinion.

Here's the code:

import org.junit.Test;
import static org.junit.Assert.*;

public class TestCases {

    @Test
    public void testRomanToArabic() {
        String[] input = {null, "", "I", "V", "XXXIII", "DCCXLVII", "CMXXIX", "MCCXXXII", "MMMCMXCIX", "MMMMXI", "KMXI",
                "VX", "VL", "VC", "VD", "VM", "LC", "LD", "LM", "DM", "IL", "IC", "ID", "IM", "XD", "XM", "CXLV", "MIXI", "IXI", "MXIII", "MMMM", "IIII"};
        int[] expectedOutput = {-1, -1, 1, 5, 33, 747, 929, 1232, 3999, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 145, -1, -1, 1013, -1, -1};
        for(int i = 0; i < input.length; i++) {
            int k = RomanNumbers.romanToArabic(input[i]);
            try {
                assertEquals(k, expectedOutput[i]);
            }
            catch(AssertionError assertFailed) {
                System.out.println("Test failed!");
                System.out.println("Input: " + input[i]);
                System.out.println("Output: " + k + "| Expected: " + expectedOutput[i]);
            }
        }
    }

    @Test
    public void testArabicToRoman() {
        int[] input = {-1, 1, 5, 33, 747, 929, 1232, 3999, 4000};
        String[] expectedOutput = {"Error", "I", "V", "XXXIII", "DCCXLVII", "CMXXIX", "MCCXXXII", "MMMCMXCIX", "Error"};
        for(int i = 0; i < input.length; i++) {
            String k = RomanNumbers.arabicToRoman(input[i]);
            try {
                assertEquals(k, expectedOutput[i]);
            }
            catch(AssertionError assertFailed) {
                System.out.println("Test failed!");
                System.out.println("Input: " + input[i]);
                System.out.println("Output: " + k + "| Expected: " + expectedOutput[i]);
            }
        }
    }
}

Is this the way to go, or is it considered bad practice to put more than one test in a method?
Do you have any other suggestions on improving the code?

I have two suggestions:

I like tests to be very easily human-readable. Instead of separate arrays of input and output, which are not aligned with each other, find a way to put the input and the expected output on one line.

The easiest way in this case would be to add a method

private void testRomanToArabic(String input, int expectedOutput) {
    assertEquals(RomanNumbers.romanToArabic(input), expectedOutput);
}

and then use this method repeatedly like

testRomanToArabic("XXXIII", 33);

etc. There are other ways to put input and expected output next to each other (e.g. create an object holding both), but this is the simplest.

Don't catch the AssertionError and print the result yourself. The test should be run by a test runner which will handle this part for you. I always do this from the Eclipse IDE by selecting "Run As JUnit Test", but there is for sure a way to do the same thing from the command line.

By catching the AssertionError, you hide the error from external tools, meaning you have to read the output yourself. If the AssertionError is thrown, your testing framework is aware of the error, which becomes very important when you have hundreds of tests: it can highlight for you the handful that are failing.


Sample size, P-values (its relationship), and data visualization with plots. ggplot and T test We know that P-values (within t-test context as an example..) is highly sensitive to sample size. A larger sample will yield a smaller p-value remaining everything else constant. On the other hand, Cohen´s d effect size remains the same.

I'm inspired in this code here, but I´ve changed some parts to make the difference between means constant, instead of creating a random variable based on a normal distribution.

Although everything is working, I do imagine that some of the experts in this community could improve my syntax.

library(tidyverse)

ctrl_mean <- 8
ctrl_sd <- 1

treated_mean <- 7.9
treated_sd <- 1.2

sample <- numeric() #criar vetor para grupar resultados
nsim <- 1000 #criar variavel
t_result <- numeric()

for (i in 1:nsim) {
  set.seed(123)
  t_result[i] <- (mean(ctrl_mean)-mean(treated_mean))/sqrt((ctrl_sd^2/(i))+(treated_sd^2/(i))) #manual t test
  sample[i] <- i # number of participants
}
ds <- data.frame(
  sample = sample, #assign the sample size
  t_result = round(t_result,3), #get the t test result
  degrees = sample*2-2) #compute the degrees of freedom

ds %>%
  filter(sample>1) %>%
  mutate(P_Value = 2*pt(abs(t_result), df=degrees,lower.tail=FALSE)) %>%
  left_join(ds,.) -> ds

#plot
ggplot(ds, aes(x=sample, y=P_Value)) +
  geom_line() +
  annotate("segment", x = 1, xend=sample, y = 0.05, yend = 0.05, colour = "purple", linetype = "dashed") +
  annotate("segment", x = 1, xend=sample, y = 0.01, yend = 0.01, colour = "red", linetype = "dashed") +
  annotate("text", x = c(1,1), y=c(.035,.001), label = c("p < 0.05", "p < 0.01"))

In this code you do not need the loop:

sample <- 1:nsim
t_result <- (mean(ctrl_mean)-mean(treated_mean)) /
  sqrt((ctrl_sd^2/(sample))+(treated_sd^2/(sample)))
# OR:
t_result <- (mean(ctrl_mean) - mean(treated_mean)) /
  sqrt((ctrl_sd^2 + treated_sd^2) / sample)

why the set seed?


Powershell script to create folders on remote servers This is pretty much the first Powershell script I've ever done and I am here looking for a quick review of it. Is there something I could have done better? Is the format okay? I don't know much about expected standards, and this was cobbled together as a result of me wanting to learn.

$Company = Read-Host 'Enter Company Name:'
$Credentials = Get-Credential

Write-Host 'Thank you! Creating client FTP folder on ServerA.' -ForegroundColor Magenta -BackgroundColor White

Invoke-Command ServerA {mkdir ("e:\ftp\users\" + $args[0] + "\INCOMING")} -ArgumentList $Company -Credential $Credentials
Invoke-Command ServerA {mkdir ("e:\ftp\users\" + $args[0] + "\OUTGOING")} -ArgumentList $Company -Credential $Credentials

Write-Host 'Done. Now creating duplicate folder on ServerB.' -ForegroundColor Magenta -BackgroundColor White

Invoke-Command ServerB {mkdir ("e:\ftp\users\" + $args[0] + "\INCOMING")} -ArgumentList $Company -Credential $Credentials
Invoke-Command ServerB {mkdir ("e:\ftp\users\" + $args[0] + "\OUTGOING")} -ArgumentList $Company -Credential $Credentials

Write-Host 'Done.' -ForegroundColor Magenta -BackgroundColor White

Whenever we first set up a user for FTP, we have to set up a folder for them. The folder needs to be named after the company, and include "Incoming" and "Outgoing" subfolders. This needs to be created on ServerA, and the exact same thing created on ServerB. This script finds out what the company name is, checks credentials, and then creates the folders in the appropriate places.

Thoughts? Things I could research to improve it?

Congratulations, you managed to get a PS script to run, that's already something!

Jokes aside, you should look into parameterizing the script, so you could invoke it with command-line arguments instead of prompting for them, if that's possible. This would also reduce the maintenance burden on the script, because if the "server A" and "server B" aren't hard-coded in the script, there's nothing to change but how the script is invoked when either server needs to be changed.

Something like this at the start of the script:

param (
    $Company = "Contoso Inc."
    $ServerA = "PathA"
    $ServerB = "PathB"
)

There's no need for Write-Host, if you want to output a string, just output a string:

"Some output"

Will output the string... Some output. Yup. That easy.


utility class for reading in JSON I'm reasonably happy with the execution:

thufir@dur:~/eclipse-workspace/jsr374$
thufir@dur:~/eclipse-workspace/jsr374$ gradle runShadow

> Task :runShadow
Oct 22, 2017 11:59:02 AM net.bounceme.dur.json.JsonOperations tryJsonFromUrl
INFO: https://my-json-server.typicode.com/typicode/demo/db
Oct 22, 2017 11:59:03 AM net.bounceme.dur.json.Main json
INFO: {"posts":[{"id":1,"title":"Post 1"},{"id":2,"title":"Post 2"},{"id":3,"title":"Post 3"}],"comments":[{"id":1,"body":"some comment","postId":1},{"id":2,"body":"some comment","postId":1}],"profile":{"name":"typicode"}}

BUILD SUCCESSFUL in 2s
6 actionable tasks: 1 executed, 5 up-to-date
thufir@dur:~/eclipse-workspace/jsr374$

am less than thrilled with the variable names, error handling, and, specifically, the convertStreamToString method.

package net.bounceme.dur.json;

import java.io.IOException;
import java.io.InputStream;
import java.io.StringReader;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.Scanner;
import java.util.logging.Logger;

import javax.json.Json;
import javax.json.JsonObject;
import javax.json.JsonReader;

public class JsonOperations {

    private static final Logger LOG = Logger.getLogger(JsonOperations.class.getName());

    private JsonObject readJsonFromString(String jsonString) {
        JsonReader jsonReader = Json.createReader(new StringReader(jsonString));
        JsonObject jsonObject = jsonReader.readObject();
        return jsonObject;
    }

    private String convertStreamToString(InputStream is) {
        Scanner scanner = null;
        scanner = new java.util.Scanner(is);
        scanner = scanner.useDelimiter("\\A");
        return scanner.hasNext() ? scanner.next() : "";
    }

    public JsonObject tryJsonFromUrl(String urlString) {
        LOG.info(urlString);
        JsonObject jsonObject = null;
        try {
            jsonObject = jsonFromUrl(urlString);
        } catch (IOException e) {
        //  e.printStackTrace();
            LOG.severe(e.toString());
        }
        return jsonObject;
    }

    private JsonObject jsonFromUrl(String urlString) throws MalformedURLException, IOException {
        LOG.fine(urlString);
        InputStream inputStream = new URL(urlString).openStream();
        String jsonString = convertStreamToString(inputStream);
        inputStream.close();
        LOG.fine(jsonString);
        JsonObject jsonObject = readJsonFromString(jsonString);
        return jsonObject;
    }

}

Open, really, to any input.  It's meant as a generic sort of utility.  Perhaps static methods?  Wasn't sure.

Finally, it's perhaps an erroneous assumption that the JSON will come in via URL...

Basically, just want to make it a bit more robust.  For example, if there's a problem, pretty much anywhere down the line...perhaps throw a custom exception?  Or return a default empty JSON object?  Both?

Finally, if there's a library which handles URL -> JSON seamlessly then all the better.

To be honest: I fail to see how this class is particularly useful (at least yet).

You can immediately create a JsonReader from a URL, making your whole problematic convertStreamToString method (as well as readJsonFromString) superfluous. So far your class is equivalent to:

private JsonObject tryJsonFromUrl(String url) {
    try (InputStream is = new URL(url).openStream();
            JsonReader reader = Json.createReader(is);) {
        return reader.readObject();
    } catch (IOException e) {
        LOG.log(Level.SEVERE, e.toString(), e);
        return null;
    }
}

That being said, let me cover your specific questions and a few unspecific observations nonetheless, since you wanted all kinds of input.

Variable names

There is nothing wrong with your variable names IMHO.

Error handling

Your code may fail to close inputStream in case of an exception, leading to a potential resource leak. To prevent this, either put inputStream.close(); in a finally block or - even better - use try-with-resources as seen above.

Your code also throws a NullPointerException when urlString is null. You can add an explicit check or document that behaviour, but you should do one of them.

Error handling (2): Logging

When encountering an exception you usually want to log the stack trace as well to help with debugging. Your current logging will print something like that:

SEVERE: java.net.UnknownHostException: some-nonexisting.host.com

This may be enough for you now since you know that there is only one possible cause within your code. But think of someone else using your class or more complex scenarios, where it might be relevant to see from where your method was called: Your current logging output won't tell you anything about that.

On the other hand

LOG.log(Level.SEVERE, e.toString(), e);

will include the whole stacktrace, making it easier to trace back the error from the logging output alone (you won't be able to debug running production servers):

SEVERE: java.net.UnknownHostException: some-nonexisting.host.com
java.net.UnknownHostException: some-nonexisting.host.com
    at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:184)
    at java.net.PlainSocketImpl.connect(PlainSocketImpl.java:172)
    at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
    at java.net.Socket.connect(Socket.java:589)
    at sun.security.ssl.SSLSocketImpl.connect(SSLSocketImpl.java:668)
    at sun.security.ssl.BaseSSLSocketImpl.connect(BaseSSLSocketImpl.java:173)
    at sun.net.NetworkClient.doConnect(NetworkClient.java:180)
    at sun.net.www.http.HttpClient.openServer(HttpClient.java:432)
    at sun.net.www.http.HttpClient.openServer(HttpClient.java:527)
    at sun.net.www.protocol.https.HttpsClient.<init>(HttpsClient.java:264)
    at sun.net.www.protocol.https.HttpsClient.New(HttpsClient.java:367)
    at sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.getNewHttpClient(AbstractDelegateHttpsURLConnection.java:191)
    at sun.net.www.protocol.http.HttpURLConnection.plainConnect0(HttpURLConnection.java:1104)
    at sun.net.www.protocol.http.HttpURLConnection.plainConnect(HttpURLConnection.java:998)
    at sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:177)
    at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1512)
    at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1440)
    at sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:254)
    at java.net.URL.openStream(URL.java:1038)
    at net.bounceme.dur.json.JsonOperations.tryJsonFromUrl(JsonOperations.java:32)
    at net.bounceme.dur.json.JsonOperations.main(JsonOperations.java:25)

Here you would see that the exception was thrown in line 32 of the JsonOperations class within the tryJsonFromUrl method and that this method was called from line 25 in the main method. That gives you already a first idea of what might have led to the exception.

convertStreamToString method

Scanner scanner = null;
scanner = new java.util.Scanner(is);

Can be shortened to

Scanner scanner = new Scanner(is);

(you are already importing java.util.Scanner)

Otherwise it doesn't look bad (see also this Stack Overflow answer).

Perhaps static methods?

If it's intended to be a pure utility class, I'd go with static methods. See this Stack Overflow question for a few details.

Assumption that the JSON will come in via URL

For a general purpose utility class that is almost certainly an erroneous assumption. You can either offer several entry methods, e.g.

public JsonObject tryJsonFromUrl(String urlString);
public JsonObject tryJsonFromUrl(URL url);
public JsonObject tryJsonFromString(String jsonString);

or a generic method, letting the caller handle the details:

public JsonObject tryJson(InputStream is);

The latter may be a bit more work for the caller, but in return offers more flexibility (it's also the option that has been chosen for javax.json.Json).

Custom exception? Or return a default empty JSON object?

Whether you throw an exception, return null or return an "empty" object is to some extent a matter of style. In your case I'd probably go without an exception as the method name ("try...") suggests that reading may fail (thus making it a non-exceptional case). And since I can't think of any general use for a default object, null would be fine as well.

If you decide to throw an exception, I'd prefer a custom one to a) make your api consistent and b) abstract from implementation details (e.g. reading JSON from a String won't throw an IOException).

General observations

"I find your lack of javadoc disturbing".

Especially when writing code for others, your class should describe what it does. In detail. See it as a contract: Your javadoc describes what behaviour a caller can (and cannot) expect. Oracles document on javadoc has a lot more details on that.

A library which handles URL -> JSON seamlessly

The default implementation, see above ;-)


Counting number of inversions This is a HackerEarth Challenge Problem.

  Question:
  Puchi hates to carry luggage, but unfortunately he got a job to carry
  the luggage of his N friends in office. Each day, one of his N
  friends, gives him the luggage of a particular weight to carry. You
  will be given the weight of luggage of each friend in the array
  Weight, where Weighti is the weight of luggage of ith friend carried
  by Puchi on ith day. It is given that all the luggages carried by
  Puchi are distinct in their weights. As Prateek assigned this job to
  Puchi, so for each day, he wants to know the number of days in future
  when Puchi will have to carry the luggage , having weight less than
  the weight of luggage of current day. Please help Prateek for the
  same.

  Input:
   The first line contains a single integer T, denoting the number of test cases. In each test case, the following input will be
  present: First line contains an integer N, where N represents the
  number of friends. Next N line contains N integers, where ith line
  contains ith integer, which represents Weighti.

  Output:
     Output exactly T lines. Each line contains N integer separated by a space, where ith integer represents the number of
  luggage of future, which are less than the weight of luggage of the
  current day.

  Constraints:

  Subtask 1:   1 <= T <= 30 1<= N <= 104 1<= Weighti <= 106

  Subtask 2:   1 <= T <= 10 1<= N <= 105 1<= Weighti <= 106

I have used merge sort to calculate the inversions:

public class Puchi_and_Luggage {
static long invfreq[]=new long[1000001];

    public static void main(String[] args) throws NumberFormatException, IOException {

        BufferedReader br=new BufferedReader(new InputStreamReader(System.in));
        int T=Integer.parseInt(br.readLine().trim());
        while(T-->0)
        {
          int N=Integer.parseInt(br.readLine().trim());
          StringBuilder ans=new StringBuilder();

          int arr[]=new int[N];
          int copy[]=new int[N];
          for(int i=0;i<N;i++)
          {
              arr[i]=Integer.parseInt(br.readLine().trim());
              copy[i]=arr[i];
              invfreq[arr[i]]=0l;
          }

          mergesort(arr,0,N-1);
          for(int i=0;i<N;i++)
          {
            ans.append(invfreq[copy[i]]+" ");
          }
          System.out.println(ans);
        }

    }

    private static void mergesort(int[] arr, int i, int j) {

        int mid=0;

        if(i<j)
        {
            mid=(i+j)/2;
            mergesort(arr,i,mid);
            mergesort(arr,mid+1,j);
            merge(arr,i,mid,j);

        }

    }

    private static void merge(int[] arr, int i, int mid, int j) {

        int temp[]=new int[arr.length];
        int l=i;
        int r=j;
        int m=mid+1;
        int k=l;
        long inv=0l;
        while(l<=mid && m<=r)
        {
            if(arr[l]<=arr[m])
            {
                temp[k]=arr[l];
                invfreq[temp[k]]+=inv;
                k++;
                l++;
            }
            else
            {
                temp[k]=arr[m];
                k++;
                m++;
                inv++;

            }

        }
        while(l<=mid){
            temp[k]=arr[l];
            invfreq[temp[k]]+=inv;
            k++;
            l++;
        }
        while(m<=r){
            temp[k]=arr[m];
            k++;
            m++;
        }

        for(int i1=i;i1<=j;i1++){
            arr[i1]=temp[i1];
            //System.out.print(arr[i1]+" ");
        }

    }

}

But my code is giving Time Limit Exceeded for test cases with large input files.

Short answer: creating - and garbage collecting - N arrays of size N takes some time, even if operator new[] has been tuned for exceptional performance (as is the case for Java and C#).

int temp[]=new int[arr.length];

Long answer: a detailed treatment of all the coding/performance sins inherent in the code offered for review would fill many pages. However, I'm leaving that for someone else and instead I'll focus only on the algorithmics, in a language-agnostic way.

Throwing needlessly complicated technology at a practice problem can be educational, if only to drive the lesson home that the technology in question is in fact not a good match for the problem and that other solutions require less effort. Hence, if a problem listed under ‘merge sort’ then one ought to also solve it via merge sorting, but the first call should always be the simplest solution that promises to do the job.

In this case, that simplest solution is using a Fenwick tree to count inversions. No fuss, no muss, solution (C#) accepted with 1.19 s combined time for all test files.

const int MAX_VALUE = 1000000;

for (int t = int.Parse(Console.ReadLine().Trim()); t-- > 0; )
{
    int n = int.Parse(Console.ReadLine().Trim());
    var a = new int[n];

    for (int i = 0; i < n; ++i)
        a[i] = int.Parse(Console.ReadLine().Trim());

    var fenwick = new int[1 + MAX_VALUE];
    var result = new int[n];

    for (int i = n - 1; i >= 0; --i)
    {
        int a_i = a[i], inversions = 0;

        for (int j = a_i; j > 0; j -= j & -j)
            inversions += fenwick[j];

        for (int j = a_i; j <= MAX_VALUE; j += j & -j)
            ++fenwick[j];

        result[i] = inversions;
    }

    Console.WriteLine(string.Join(" ", result));
}

Compare this to the reams of code necessary for the merge sorting approach.

According to the problem constraints, values can go ten times as high as indices (limit 10^6 instead of 10^5). This makes the Fenwick ten times as big as the biggest input array, increasing the average path lengths for the Fenwicking from not quite 9.5 to not quite 11.5. However, for small ranges such as these this inefficiency is negligible - the code is more than fast enough already.

For details on Fenwick trees see the Wikipedia article on Fenwicks and the related practice section on HackerEarth (which includes a tutorial; it's in the Data Structures section which you're supposed to work through before starting with Algorithms). As regards determining the average iteration counts for the Fenwick loops, I'd recommend doing a bit of Fenwicking with pen and paper (for a Fenwick of size 2^k it's k / 2 + 1).

As regards merge sorting: unlike Fenwicks, merge sorting does not give a flying meow about what the actual ranges of the input values are - all it does with the inputs is compare them. This makes the algorithm especially attractive when the value range diverges wildly from the value count. For example, Fenwicking 100000 longs would require sorting the inputs in order to ‘compress’ them to the range 1 to 100000, which requires O(N log N) of extra effort.

Merge sorting requires no such preprocessing, but since it destroys the original order of the values it requires extra effort for correlating the inversion counts thus found to the original inputs (e.g. the array copy[] in the original code). Also, it makes the code quite a bit more complicated than that using a Fenwick tree, and it would get hugely more complicated if the input values weren't guaranteed to be unique whereas the Fenwick couldn't care less about uniqueness.

Coding a top-down merge sort with recursive function calls - as opposed an iterative bottom-up version with nested loops - makes it more difficult to control certain aspects (e.g. optimal splitting instead of simple halving) and it makes it even more difficult to make sorted ranges end up precisely where they're needed, instead of resorting to redundant copying of values.

However, the recursive version has somewhat better cache locality and in any case it is more than fast enough for this problem, regardless of the redundant copying. In my benchmarks it took little more than twice as long as the simpler Fenwick and was only marginally slower than an iterative version (124 ms for 10^6 values recursive vs 110 ms iterative and 44 ms Fenwick). In a coding challenge it might pay to keep the differences in mind, but not for this simple practice problem.

Hence the only algorithmic fix required here is using a pre-allocated temp array large enough to hold the largest input. That's all.


Sum of Even Fibonacci numbers in functional programming I want to solve this problem in functional programming (fp) way only.

  Each new term in the Fibonacci sequence is generated by adding the
  previous two terms. By starting with 1 and 2, the first 10 terms will
  be:

  1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...

  By considering the terms in the Fibonacci sequence whose values do not
  exceed four million, find the sum of the even-valued terms.

Source: https://projecteuler.net/problem=2

This is my fp approach:

const range = num => Array.from(new Array(num), (_, i) => i);
const divisibleBy = divisor => num => num % divisor === 0;
const isEven = i => divisibleBy(2)(i);
const sum = (acc, val) => acc + val;
const fibonacciOf = n => {
    if (n === 0) {
        return 1;
    }
    if (n === 1) {
        return 2;
    }
    return fibonacciOf(n - 1) + fibonacciOf(n - 2);
};
const doUntil = maxVal => (fiboResult, i) => {
    const next = i + 1;
    return fiboResult >= maxVal ? i : doUntil(maxVal)(fibonacciOf(next), next);
};
const until4Mil = doUntil(4000000);
const maxRange = until4Mil(0, 0);

const solution = range(maxRange)
    .map(fibonacciOf)
    .filter(isEven)
    .reduce(sum);

console.log("solution ", solution);

1) I was struggling with the recursion, because that was the only way I could think of how to emulate the do...while loop. Do you know any alternative to the do...while loop (aka. a loop where you don't know when it ends) in fp?

2) Also, I'm not sure whether the part const maxRange = until4Mil(0, 0); is clean code or not.

Any other improvement suggestions are welcomed as well.

General Notes

The use of map(f1).filter(f2).reduce(f3) is a solid approach to functional programming.
The names of the functions and variables are reasonable.

Things to consider

Scope: The lexical scope of the the const = x => ...; is the global namespace. The values are only used locally. In Javascript this is suboptimal practice.
Lambdas: Javascript lambda notation facilitates passing anonymous functions. One advantage of passing anonymous functions is that it is done locally. const sum... is four lines from the top of the file. .reduce(sum) is only two lines from the bottom.

.reduce((x,y) => x + y)

is almost certainly easier to understand...though it may be harder to write without refactoring.
Intent: The use of const is good to express your intent to program in a functional manner, but it does not really make the code easier to read. One of the 'supposed' rationales for using functional programming is that it is easier to reason about. This is a case where: /* This code is written in a functional style. It does not use mutuation */ at the top of the file might be another way to state the your intent while letting the Javascript look more familiar.

Computer Science

In terms of functional programming, the use of recursion is a step in the right direction. However the recursive code calculating the Fibonacci values:

const fibonacciOf = n => {
    if (n === 0) {
        return 1;
    }
    if (n === 1) {
        return 2;
    }
    return fibonacciOf(n - 1) + fibonacciOf(n - 2);
};

Is "tree recursive" and therefore does a lot of extra work. Tree Recursion is described in The Structure and Interpretation of Computer Programs. Section 1.2.2 has a very helpful illustration that shows how tree recursive algorithms wind up performing substantially more work than is necessary.

Alternative Functional Approach

Recursive Design: Structurally recursive algorithms have two part: a base case and an inductive step. The naive approach to Fibonacci treats n == 0 as the base case and n-1 as the induction. The alternative code treats n as the base case and n + 1 as the induction. This means n - 1 has always been computed previously.

function sumOfEvenFibsLessThan (n) {

    /* Returns the array of Fibonacci numbers less than n */
    var fibsLessThan = function (n) {

        /* Takes an Array, returns an Array */
        var generateFibs = function(fibs) {
            var nextFib = fibs[fibs.length - 1] + fibs[fibs.length - 2];
            // test against max
            if (nextFib >= n) {
                return fibs;
            }
            else {
                // concatinate next number to array
                return generateFibs(fibs.concat([nextFib]));
            }
        };
        // check input validity
        if (n <= 1) {
            return [];
        };
        // check base case
        if (n <= 2) {
            return [1,1];
        };
        // otherwise generate array recursively
        return generateFibs([1,1]);
    };

    return fibsLessThan(n).filter(x => x % 2 === 0).reduce((x,y) => x + y);
}

Lexical Scope: Only sumOfEvenFibsLessThan is in the global namespace.
The map in the map(f1).filter(f2).reduce(f3) pattern can be considered as f1 = x => x (aka "An identity function").
A trampoline is used to start generateFibs. Trampolines are a common functional programming technique to invoke a recursive function with a local lexical scope.


Efficient data migration I'm trying to make a market depth chart with data from a exchange and a Google JS chart.

The good news is I got it working, but the bad news is the code to prepare the data to be used by google charts is just bad. I know there should be far better ways to do this but I can't come to a proper solution.

The original data is as following:

{bids: [[price, amount], [price, amount], ...],
 asks: [[price, amount], [price, amount], ...]}

Google chart needs it like this:

[[price, bid, ask], [price, bid, ask], ...]

I have the following (ugly) code:

var newData = [['price', 'bid', 'ask']];
var t = 0;
var tt = [];
var p = [];
for (var i = data.bids.length -1; i > -1; i--) {
    t += data.bids[i][1];
    tt.push(t);
    p.push(data.bids[i][0])
}
var ii = 0;
for (var i = tt.length - 1; i >= 0; i--) {
    newdData.push([p[ii], tt[i],null])
    ii++;
};
t = 0;
for (var i = data.asks.length -1; i > -1; i--) {
    t += data.asks[i][1];
    newData.push([data.asks[i][0], null, t]);
}

If I create a google chart with the newData it works as expected. How can I make the code better or more efficient?

I took some time to look at your code and rewrite it a little bit.

My first advice is, try to make your code a little bit more verbose. It will be only written once and read over and over. It's very important that the code communicates what it's supposed to do, either through variable naming or comments.
The Art Of Readable Code is a great book on this topic, thoroughly covered by "Implementation Patterns" as well.

So...you can try this out:

function newAlgorithm(data){
    // Intial row needs data for headers
    var newData = [['price', 'bid', 'ask']],
    // Optimizes for loops
    entriesCount = data.bids.length,
    // Will be calculcated later on
    bidAmountSum = 0,
    askAmountSum = 0,
    // Will contain all entries that should be pushed into the final array
    bidEntries = [],
    askEntries = [];

    // Calculate bid and ask sums
    for(var i = 0; i < entriesCount; i++){
        bidAmountSum += data.bids[i][1];
        askAmountSum += data.asks[i][1];
    }

    // Create bid and ask entries for each one in one pass, since their count is the same
    for(var i = 0; i < entriesCount; i++){
        // Pid price is iterated in reverse
        // Bid amount sum is cumulative sum of previous bids for each point in iteration
        bidEntries.push([data.bids[data.bids.length-1 - i][0], bidAmountSum, null]);
        bidAmountSum -= data.bids[i][1];

        // Price is iterated in forward order
        // Ask amount sum is cumulative sum of previous asks for each point in iteration
        askEntries.push([data.asks[i][0], null, askAmountSum]);
        askAmountSum -= data.asks[i][1];
    }

    return newData.concat(bidEntries).concat(askEntries.reverse());
}

I tested how it performs when tested with NodeJS runtime. I increased the dataset from 100 entries through 2,000,000.

I also made in-browser test here: http://jsperf.com/chart-data-repacking

I hope this helps.


Git pie - Perl oneliner for Git I often make changes on my code-base and manually using sed, awk or perl -pi -e is not always as efficient as having a simple alias: git pie.

Furthermore, because I am working on Cygwin, I cannot do real in-place replacement without a temporary file that I have to manually suppress afterwards.

Last but not least, sed, awk and perl -pi -e touch all the files that make them new from the point of view of make. I rather want to touch only the file with real changes so I wrote this Git alias:

[alias]

    # Replace
    pie = "!f() {                                            \
        git ls-files -z | xargs -0 perl -e '                 \
            my $eval = shift @ARGV;                          \
            foreach my $file (@ARGV) {                       \
                next unless -f $file;                        \
                open my $fd, \"<\", $file;                   \
                my $content = do { local $/; <$fd>};         \
                close $fd;                                   \
                my $new = do { $_ = $content; eval $eval; }; \
                if ($content ne $new) {                      \
                    open my $fd, \">\", $file;               \
                    print $fd $_;                            \
                    close $fd;                               \
                }                                            \
            }                                                \
        ' \"$1\"; }; f"

I am sure some simplifications can be made on this piece of code, so I ask for your opinion.

The usage is:

$ git pie s/foo/bar/g

I decided to slurp the whole file in memory in order to do multiline regexes which is very convenient sometime despite the performance.

There is perhaps a missing feature: the ability to exclude or include some file extensions only or simply allow this call:

$ git ls-files | grep -P '[.][ch]$' | xargs git pie s/foo/bar/

I still didn't figure out how I want to do it. One possible way is:

$ git pie 's/foo/bar/ if $file =~ /\.(ch)$/'

As written, $new will contain the return value of s///, which is 1 (true) or the empty string (false).  This will (almost) never match $content and you'll be rewriting every file unconditionally.

You can inject $1 directly without the pass-and-eval.  This will stop the Perl script altogether in the event of an error, which is probably desirable.

While my and lexical open etc. are good habits, they're at odds with the design constraints of a one-liner that needs to survive multiple levels of quoting.  For example, print FD prints $_ to handle FD, while print $fd needs an explicit $_ argument, else it prints the string representation of that handle to stdout.  Just switching to a bareword-open saves you five characters.

Perl has a mechanism to read whole files specified on command-line and you can shorten your script further by employing it.

Tying all this together, your one-liner can fit in one line with some whitespace to spare.  In 128 columns (plus four spaces of indent):

[alias]
    pie = "!f(){ git ls-files -z | xargs -0 perl -0777ne'$old=$_; '\"$1\"'; $_ ne $old and open FD, q|>|, $ARGV and print FD';} ;f"


Graph degree as solution for undirected graph paint The problem is:

  Given an undirected graph represented as an adjacency matrix and an
  integer k, write a function to determine whether each vertex in the
  graph can be coloured such that no two adjacent vertices share the same
  colour using at most k colours.
  Source: https://www.dailycodingproblem.com/

The proposed solution uses a backtracking algorithm (https://www.dailycodingproblem.com/blog/graph-coloring), but  I would like to know if just evaluating the vertice degree is enough (source: https://youtu.be/LUDNz2bIjWI?t=169).

    boolean canBeColored(int[][] adjacencyMatrix, int colors) {

    for (int row = 0; row < adjacencyMatrix.length; row++) {
        int degree = 0;
        for (int column = 0; column < adjacencyMatrix[row].length; column++) {
            if (adjacencyMatrix[row][column] == 1) {
                degree++;
            }
        }

        if (degree > colors) {
            return false;
        }
    }

    return true;
}

It is not. Consider a triangle graph which has tree nodes and three edges. All vertices has degree two but three colors are required to color it. Your function would fail for that input.

In fact, the problem is NP-complete meaning that it is not known if an efficient algorithm can be constructed for solving it. So if you can do it, you'll win fame and fortune and also a very large monetary prize.


Permutations in Python Question 2 of Homework 11 from Berkeley's CS6 1A says:

  Implement permutations, a generator function that takes in a lst and outputs all permutations of lst, each as a list (see doctest for an example). The order in which you generate permutations is irrelevant.

This is my solution:

def permutations(lst):

    if not lst:
        yield []
        return
    "*** YOUR CODE HERE ***"
    if type(lst)==tuple:
        t=lst
        lst=[]
        for elem in t:
            lst+=[elem]
    if type(lst)==str:
        lst=list(lst)

    for elem in lst:
        if elem:
            l_temp=[lst[0]]
            lst=lst[1:]
            lst.extend(l_temp)
            temp=list(lst[1:])
            r_list=[]
            lst_of_lst=[]
            r_list+=[lst[0]]
            while len(lst_of_lst)!=len(lst)-1:
                holder=temp[1:]
                t_holder=[temp[0]]
                temp=[]
                temp.extend(holder)
                temp.extend(t_holder)
                if len(r_list)==1:
                    r_list.extend([e for e in temp])
                    lst_of_lst+=[r_list,]
                    r_list=[]
                    r_list+=[lst[0]]
            yield lst_of_lst

Test cases:

>>> sorted(permutations([1, 2, 3]))
[[1, 2, 3], [1, 3, 2], [2, 1, 3], [2, 3, 1], [3, 1, 2], [3, 2, 1]]
>>> type(permutations([1, 2, 3]))
<class 'generator'>
>>> sorted(permutations((10, 20, 30)))
[[10, 20, 30], [10, 30, 20], [20, 10, 30], [20, 30, 10], [30, 10, 20], [30, 20, 10]]
>>> sorted(permutations("ab"))
[['a', 'b'], ['b', 'a']]

I need some suggestions in order to improve my code.

if type(lst)==tuple:
        t=lst
        lst=[]
        for elem in t:
            lst+=[elem]
    if type(lst)==str:
        lst=list(lst)

It's worth knowing that list works on tuples too. (Online demo).

    for elem in lst:
        if elem:

What purpose does this if serve? It seems to me to introduce a bug, because the spec does not ask for falsy elements of lst to be ignored.

            l_temp=[lst[0]]
            lst=lst[1:]

[lst[0]] would be more Pythonic as lst[:1]. Spot the symmetry with the second line.

The reuse of names is not particularly helpful, especially given that lst is a bad name. How about this?

            first, rest = lst[:1], lst[1:]

            lst.extend(l_temp)

Wait, that was just to rotate the list? Why? At this point I'm seriously in need of some comments to explain what you're trying to do.

            temp=list(lst[1:])
            r_list=[]
            lst_of_lst=[]
            r_list+=[lst[0]]

lst_of_lst? That sounds like a bad name for a 2D array, but the spec doesn't ask for a 2D array and I can't see why you'd need one.

r_list is again just lst[:1].

                holder=temp[1:]
                t_holder=[temp[0]]
                temp=[]
                temp.extend(holder)
                temp.extend(t_holder)

Or in other words temp = temp[1:] + temp[:1]. You're rotating lists often enough that maybe it's worth factoring out a function to make the flow of logic clearer.

                if len(r_list)==1:
                    r_list.extend([e for e in temp])
                    lst_of_lst+=[r_list,]
                    r_list=[]
                    r_list+=[lst[0]]
            yield lst_of_lst

There is no way this algorithm can be correct. Have you tested your code?

print len(list(permutations([1, 2, 3])))
print len(list(permutations([0, 1, 2, 3])))

should output

6
24

Once your code works, given that it's Python you should run it through an automated PEP8 checker. It should be easy to find one online.


Print all nodes at k distance from the root in a binary tree iteratively I am solving this interesting coding challenge of

  printing all nodes at K distance from the root of a binary tree in a iterative fashion

Will be great to get review regarding how this code can be improved in a better way.

public class NodesAtKDistanceFromRoot {

    private final static Node EMPTY_MARKER = new Node(-1, null, null);

    public static void print(Node root, int level) {

        if (root == null) {
            return;
        }

        Deque<Node> queue = new ArrayDeque<>();
        int d = 1;
        queue.add(root);
        queue.add(EMPTY_MARKER);
        boolean found = false;

        while (!queue.isEmpty()) {

            Node current = queue.remove();

            if (current.equals(EMPTY_MARKER)) {
                // encountered empty marker which means all the nodes of a given level is visited
                d++;
                if (d == level) {
                    found = true;
                    break;
                } else {
                    queue.add(EMPTY_MARKER);
                }
            } else {
                if (current.left != null)
                    queue.add(current.left);

                if (current.right != null)
                    queue.add(current.right);
            }
        }

        // print the nodes for the given level
        if (found) {
            while (!queue.isEmpty()) {
                Node current = queue.remove();
                System.out.println(current.val);
            }
        }
    }

}

Code in Github: https://github.com/Ramblers-Code/CodeKata/blob/master/src/main/java/kata/tree/NodesAtKDistanceFromRoot.java

I think it's a decent implementation. It is readable and understandable. Nonetheless, I have some tips:

Try to avoid single letter variable names, like d (unless in very specific cases like certain loop variables or mathematical constructs).
Personally not a fan of new lines after each opening brace, it breaks the flow for me.
Try to always use braces with if\while\... statements. It helps to prevent potential bugs.
You could instead of silently returning, throw an (IllegalArgument\Nullpointer)Exception when passed node is null. Fail fast! Same with negative depths passed.
Trivia: how would you print the root with your implementation?

I also do not think you need the marker. If you instead each loop execution only take all nodes present in the queue at the start of that loop, you will process all nodes level by level (but only use it if you have constant time size operations). As an example:

public static void print(Node root, int depth) {
    if (root == null) {
        return;
    }

    Deque<Node> queue = new ArrayDeque<>();
    queue.offer(root);

    while (!queue.isEmpty()) {
        boolean atRequiredDepth = --depth == 0;
        int nodeCount = queue.size();

        if (atRequiredDepth) {
            for (int i = 0; i < nodeCount; i++) {
                Node node = queue.poll();
                System.out.println(node.val);
            }
            return;
        } else {
            for (int i = 0; i < nodeCount; i++) {
                Node node = queue.poll();
                if (node.left != null) {
                    queue.offer(node.left);
                }
                if (node.right != null) {
                    queue.offer(node.right);
                }
            }
        }
    }
}


Checking for an undefined nested variable in a JS object Is there a better or shorthand way to perform a check on whether or not a variable within (possibly several levels of) an object is null or undefined?

What I would currently do to check is:

if (typeof json !== 'undefined' || typeof json.data !== 'undefined' || typeof json.data.formkeys !== 'undefined') {}

Is there a shorthand way to check for whether or not "json.data.formkeys" is undefined without the concern that one or all of the higher level properties or the object itself might not currently exist?

There's a lot of ways to do this. Here's an exotic (and costly) one:

if(((json || {}).data || {}).formkeys !== void 0){}

As commented, a try-catch will work

// JS has function scope. foo starts its life as declared but undefined
// If the try-catch fails, foo will remain undefined.
try{ var foo = json.data.formkeys; } catch(){}

if(foo !== undefined){}

A longer and possibly better way to do it is to create a resolver function. Most frameworks I have encountered use some form of this in their getter functions so that getting a value from a half-complete path will just return undefined instead of throwing midway.

function resolve(path, root){
  root = root || window;
  return path.split('.').reduce(function(parent, child){
    // A quick check for brevity. You can replace this with a better check
    // and return logic.
    return parent && parent[child];
  }, root);
}

var foo = resolve('json.data.formkeys');

if(foo !== undefined){}


Config file to struct in C I have a struct OPTS which contains a few chars and char[]s. I also have a file opts.cnf which can be edited in a text editor like vim. I want to read the opts.cnf and assign the values from that file to my struct.

opts.h

typedef struct OPTS {
  char opt1;
  char opt2[6];
  char opt3;
}

opts.cnf

OPT1=Y
OPT2=1234
OPT3=D

opts.c

while (fgets (work, sizeof (work), file) ) {
   if (!strncmp (work, "OPTS1=", 6) ) {
      optptr->opt1 = topper(work[6]);
   }
   if (!strncmp (work, "OPTS2=", 6) ) {
      sprintf(optptr->opt2, "%-4.4s", &work[6]);
   }
   if (!strncmp (work, "OPTS3=", 6) ) {
      optptr->opt3 = topper(work[6]);
   }
}

Is there a more efficient way to go about this? I need to be able to edit opts.cnf inside of a text editor but different formats are welcome if they make it more efficient.

This may fit perfectly depending on the purposes, or you could use another approach to be more efficient, clean or consistent.

If you just need to set a few variables of a project, and you don't mind running opts.c every time you change opts.cnf
Well, your existing solution fits perfectly.

If you are using ops.cnf to set some variables or constants at compilation time.
In this case, you may have to use #include and #define

If you are planning to use configuration file(s) for different sort of things in your project.
In this case I would recommend libconfig. Maybe LUA  or even Javascript.

Depending on your needs.


Finding all integers in an array with odd occurrence I am doing an interview exercise problem from here.

The problem is to find "all numbers that occurred an odd-number of times in an array".

Here is my high level psuedo code thinking:

Construct a map that will map integers to their frequencies.
Iterate through the array, use the map to record the frequency count.
Iterate through all the keys in the map and add the keys that have an odd value to another collection.
Convert that collection to an array and return it to the user.

Here is the my implementation of this high level psuedo code in Java:

public static int[] allOdd(int[] array) {
    Map<Integer, Integer> hm = new HashMap<Integer, Integer>();
    for(int c=0;c<array.length;c++) {
        if(hm.containsKey(array[c])) {
            hm.put(array[c], hm.get(array[c]) + 1);
        } else {
            hm.put(array[c], 1);
        }
    }
    List<Integer> allOdds = new ArrayList<Integer>();
    for(int key: hm.keySet()) {
        if(hm.get(key) % 2 == 1) {
            allOdds.add(key);
        }
    }
    int[] allOs = new int[allOdds.size()];
    for(int c=0;c<allOdds.size();c ++){
        allOs[c] = allOdds.get(c);
    }
    return allOs;
}

I tested this for two of my test cases and in both test cases ( {2,3,2,4,4,4,6,8,8} and {90,91,91,93,93,93,95,98,98,97} ), this function produced the right results([3, 4, 6] and [97, 93, 95, 90] respectively).

Is there anything I can do to improve the time and space complexity of this method. One of the worries I had was that I created too many collections (too much space) But to me each collection had its own purpose. The map was to map values to frequencies. The ArrayList was a dynamic structure to hold the odd occurrences. You needed to initialize the last array because ArrayList is a collection that holds objects so you need to manually iterate through the ArrayList and store those ints (Auto box) in the new array. To Array

Is there anything you can point out that can improve time and space efficiency in this function?

You don't really need to keep track of how many times you've seen each integer, only whether you've seen it an odd number of times. So start with your data structure (map, hash table, dictionary, whatever) being empty, which correctly reflects the state that you have seen no numbers an odd number of times. Now for each number x in the array, if x is in the map then remove it (now you've seen it an even number of times) otherwise add it.


exercise #40 in A Tour of Go My quick searching did not reveal anyone sharing their solution to this exercise, so am not sure if there are other (and better) approaches to mine:

package main

import (
    "code.google.com/p/go-tour/wc"
    "strings"
)

func WordCount(s string) map[string]int {
    foo := strings.Fields(s)
    m := make(map[string]int)
    for i := 0; i < len(foo); i++ {
        v, ok := m[foo[i]]
        if ok {
            m[foo[i]] = v + 1
        }
        m[foo[i]] = v + 1
    }
    return m
}

func main() {
    wc.Test(WordCount)
}

You can be more concise and readable using range and the fact that the default value of the int is 0 :

func WordCount(s string) map[string]int {
    m := make(map[string]int)
    for _, w := range(strings.Fields(s)) {
        m[w]++
    }
    return m
}


Sieve prime generator C++ I had some issues with the behavior of the iterator in my prime generator:

#include <vector>
#include <set>
#include <iostream>

using namespace std;

int main() {
    int a,b;
    int nbr;
    vector<int> t;
    set<int> s;

    // prime generated to b
    b = 10;
    // set 2 -> b.
    for (int i = 2; i != b; i++)
    {
        s.insert(i);
    }
    int i = 0;
    while (s.size() != 0)
    {
        i++;
        cout << "Iteration = " << i << endl;
        t.push_back(*s.begin());
        for (set<int>::iterator it = s.begin() ; it != s.end(); it++)
        {
            cout << "       s  = " << *it << endl;
            if (*it % t.back() == 0) {s.erase(it++);}
            if (it == s.end()) break;
        }
    }
    cout << "++++++++++++++++++++++++++++" << endl;
    cout << "affichage des nombres premiers stockés dans le vecteur t" << endl;
    cout << "taille t = " << t.size() << endl;
    for (vector<int>::iterator it = t.begin() ; it != t.end(); ++it)
    {
        cout << "t = " << *it << endl;
    }

I think erasing an element of s in the for loop can be a problem, but the compiler doesn't complain and it seems to work. Is it a problem? I am also happy for a general review.

Here are some things that may help you improve your code.

Eliminate unused variables

Unused variables are a sign of poor quality code, and you don't want to write poor quality code. In this code, a and nbr are unused.  Your compiler is smart enough to tell you about this if you ask it nicely.

Avoid single-line if constructs

The compiler can handle it just fine, but readers of your code are more likely to be able to correctly interpret the code if you avoid single-line if.  So instead of

if (*it % t.back() == 0) {s.erase(it++);}

use this

if (*it % t.back() == 0) {
   s.erase(it++);
}

Be careful when you erase an iterator

The standard, in section 23.2.4 says that for an associative container, when you use erase(it) where it is an iterator:

  erases the element pointed to by q. Returns an iterator pointing to the element immediately following q prior to the element being erased. If no such element exists, returns a.end().

This means that to be safe, your loop should look like this instead:

for (auto it = s.begin() ; it != s.end(); )
{
    cout << "       s  = " << *it << '\n';
    if (*it % t.back() == 0)
        it = s.erase(it);
    else
        ++it;
}

As pointed out by @LokiAstari, this works just fine for associative containers such as std::set but a more generic approach would be to use this for the line containing erase:

        s.erase(it++);

This works with set but also with other non-associative standard containers, including vector.

Think carefully about signed and unsigned

The code currently uses int types for both the vector and the set but are you looking for negative prime numbers?  If not, it could be that unsigned would be a better choice for data type.

Reconsider container choices

A std::set is a sorted associative container.  Since it's usually implemented internally as a tree, it's a relatively computationally expensive container to use consider the fact that all you really need is a list.

Consider using range-for syntax

The code currently prints the resulting vector with this code:

for (vector<int>::iterator it = t.begin() ; it != t.end(); ++it)
{
    cout << "t = " << *it << endl;
}

It can be written much more simply with a range-for:

for (const auto i : t) {
    cout << "t = " << i << '\n';
}

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream.  Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.


Is this a good way to implement let-with? I've implemented a let-with macro that takes an association list, keyed by symbols, that enables code to bind their corresponding values to local variables.

It's designed to have a similar effect to JavaScript's with, but without with's attendant problems (by making you explicitly list the variables to bind).

(define-syntax let-with
  (syntax-rules ()
    ((let-with alist (key ...) expr ...)
     (let ((key (cond ((assq 'key alist) => cdr))) ...) expr ...))))

I'd love to hear of any improvements to let-with's interface and/or implementation.

Example usage:

(define alist '((first . "Jane")
                (last . "Doe")))
(let-with alist (first last)
  (string-append first " " last))    ; "Jane Doe"

Keys that don't exist in the incoming alist receive an undefined value; they do not fail to get bound, unlike JavaScript's with.

JavaScript's with statement allows one to modify object members.  This may not be possible for certain types of values (such as strings) using the let-with macro as defined above.

One could generalize let-with to bind values from several association lists.  It could have the form:

(let-with
    ((alist0 (key00 key01 key02...))
     (alist1 (key10 key11 key12...)))
  expr0
  expr1
  ...)

Aside from being more general, the above form closely resembles the let form.  I prefer this over the simpler form, though I would understand if your taste and needs differ.

Here's my implementation of the general form--it uses two auxiliary macros, one to generate two lists (alists and their corresponding keys) and the other to emit the actual code (which looks almost exactly like your definition above):

(define-syntax let-with
  (syntax-rules ()
    ((let-with bindings . body)
     (gen-let-with bindings () () body))))

(define-syntax gen-let-with
  (syntax-rules ()
    ((gen-let-with () alists keys body)
     (emit-let-with-code alists keys body))
    ((gen-let-with ((alist (key)) . rest-bindings) alists keys body)
     (gen-let-with
      rest-bindings
      (alist . alists)
      (key . keys)
      body))
    ((gen-let-with ((alist (key . rest-keys)) . rest-bindings) alists keys body)
     (gen-let-with
      ((alist rest-keys) . rest-bindings)
      (alist . alists)
      (key . keys)
      body))))

(define-syntax emit-let-with-code
  (syntax-rules ()
    ((emit-let-with-code (alist ...) (key ...) (expr ...))
     (let ((key (cond ((assq 'key alist) => cdr))) ...) expr ...))))

Perhaps there is a simpler definition.  I haven't played with macros in a while. =)

Here are some examples:

(define alist '((first . "Jane") (middle . "Q") (last . "Doe")))
(define blist '((first . "John") (middle . "R") (last . "Lee")))
(define clist '((first . "Jose") (middle . "S") (last . "Paz")))
(define first '((title . "Ms") (suffix . "Esq")))

(let-with
    ()
  "Hello, world!")    ;    => "Hello, world!"

(let-with
    ((alist (first middle last)))
  (string-append first " " middle ". " last))    ;    => "Jane Q. Doe"

(let-with
    ((alist (first))
     (blist (middle last)))
  (string-append first " " middle ". " last))    ;    => "Jane R. Lee"

(let-with
    ((alist (first))
     (blist (middle))
     (clist (last)))
  (string-append first " " middle ". " last))    ;    => "Jane R. Paz"

(let-with
    ((first (title))
     (alist (first))
     (blist (middle))
     (clist (last))
     (first (suffix)))
  (string-append first " " middle ". " last))    ;    => "Jane R. Paz"

(let-with
    ((alist (first middle last)))
  (set! first "Rose")
  (string-append first " " middle ". " last))    ;    => "Rose Q. Doe"

(let-with
    ((alist (first middle last)))
  (string-append first " " middle ". " last))    ;    => "Jane Q. Doe"

Since let-with expands into a single let, the third-to-last example works.

The last two examples demonstrate the inability to modify a value in its corresponding association list.


Python 2 Stack and LinkedList implementation I'm currently implementing data structures and algorithms I studed in my college course. This is my implementation of a singly linked list and a stack. I would appreciate some feedback in terms of the code quality.

My LinkedList:

from random import randint

class Node:

    def __init__(self, x):
        self.current = x
        self.next = None

class LinkedList:

    def __init__(self, x):
        self.head = Node(x)

    def insert(self, x):
        new_entry = Node(x)
        new_entry.next = self.head
        self.head = new_entry

    def search(self, x):
        node = self.head

        while node.current is not x:
            if node.next is None:
                break
            node = node.next

        if node.current is x:
            return x
        else:
            return None

    def contains(self, x):
        retrieved = self.search(x)
        return retrieved is x

    def delete(self, x):
        try:
            node = self.head

            while node.next.current is not x:
                node = node.next

            if node.next.current is x:
                node.next = node.next.next
        except AttributeError:
            print str(x) + " is not in the linked list"

    def traverse(self):
        node = self.head
        text = ""  # type: str

        while node is not None:
            text += str(node.current) + "->"
            node = node.next

        return text

My stack:

class Stack:

    def __init__(self):
        self.data = []

    def push(self, e):
        self.data.append(e)

    def pop(self):
        return self.data.pop()

    def prnt(self):
        print self.data

def is_integer(val):
    try:
        int(val)
        return True
    except ValueError:
        return False

def is_binary_operator(val):
    return val is '+' or val is '-' or val is '*' or val is '/'

def postfixEval(stack, expression):
    symbols = expression.split()

    for symbol in symbols:

        if is_integer(symbol):
            s = int(symbol)
            stack.push(s)
        elif is_binary_operator(symbol):
            s2 = stack.pop()
            s1 = stack.pop()

            if symbol is '+':
                a = s1 + s2
            elif symbol is '-':
                a = s1 - s2
            elif symbol is '*':
                a = s1 * s2
            elif symbol is '/':
                a = s1 / s2

            stack.push(a)

    return stack.pop()

Notes on LinkedList:

The import is unused.
It should allow creation of an empty list.
Normally a linked list inserts items after the last item. Maybe I'm confused by the way insert works, but it looks like in your case head is always the last inserted entry in the list, and the list is traversed from head backwards through history using .next.
delete should use search.

Notes on Stack:

self.data should be self.entries or something else descriptive.
prnt should be as_string or even __str__.
int(val) does not check whether something is an integer, it just tries to convert a value to an integer.
+, -, * and / are arithmetic, not binary, operators.
In Python 3 mathematical operators are modeled as functions.
is_integer, is_binary_operator and postfixEval should not be part of Stack - they are not fundamental to the stack in any way.


Creating nested objects from string I implemented a method, to create an object based on a series of strings that are passed to it, of any depth or length. The way I implemented the solution was to map over the arguments. Assuming that strings are passed in the form x.a, it then splits the string by the '.', returning just a single object with that key if the array only had one element otherwise, it was passed through a reducer to produce an array of objects which would then be merged into one object.

I'd like to make this better/ are there any concerns with using a reducer in a map function, since it's on the order of O(n2)?

type TKeys = string[]
type TTarget = {
  [key: string]: TTarget | string,
}
type TTemplate = {
  target: TTarget,
  props: ReadonlyArray<string>
}
interface ITemplate {
  target: {
    [key: string]: string | TTemplate["target"]
  },
  props: ReadonlyArray<string>
}
const createObject = <T>(...keys: TKeys): T => (
  keys.map(key => {
    const props = key.split('.');
    const reducerTemplate: ITemplate = {
      target: {},
      props: ['']
    }

    return props.length < 2
      ? {
          [key]: ''
        }
      : props.reduce((obj: ITemplate, prop: string) => {
          const target: ITemplate["target"] = obj.target
          if (obj.props.length === 0) {
            console.log(obj, prop);
            obj.props = [prop];
            target[prop] = {};
            return obj;
          }
          const object: ITemplate["target"] = obj
            .props
            .reduce((prev: TTemplate["target"] , next: string) => (target[next] || prev[next]), {})
          const atEnd: boolean = props.indexOf(prop) === props.length - 1
          object[prop] = atEnd
            ? ''
            : {}
          obj.props = [
            ...obj.props,
            prop
          ];

          return atEnd
            ? target
            : obj;
        }, reducerTemplate)
  }).reduce((main: {} | TTarget, obj: TTarget ): TTarget => ({
    ...main,
    ...obj
}), {}))

Example Usage:

createObject('a.b.c.d', 'hello.world', 'test')
//outputs
    {
    a: {
        b: {
            c: {
                d: '',
            },
        },
    },
    hello: {
        world: '',
    },
    test: '',
}
createObject('hello')
//outputs
{ hello: '' }
createObject('a.b')
//outputs
{
    a: {
        b: '',
    },
};

In general case this always maps to O(n2). You may introduce some complex optimisations but this yields to long hardly readable code. Are you sure you will benefit from this?

I am a huge fan of CoffeeScript language and both CoffeeScript and TypeScript do a lot of hidden work so it is crucial to see the compiled JavaScript. Also, if your primary concearn is perfomance you will benefit greatly from plain JavaScript solution.

So here is my attempt on it. It is simular to Máté Safranka solution (which is quite nice and readable). It uses Array.prototype.reduce to remove all local variables in favor of function arguments:

function createObject(...pathes) {
  return pathes.reduce(function (obj, path) {
    path.split('.').reduce((obj, key) => obj[key] = obj[key] || {}, obj);
    return obj;
  }, {});
}

// Some tests
console.log(createObject('a.b.c.d', 'hello.world', 'test'));
console.log(createObject('a', 'a.b', 'a.b.c'));
console.log(createObject());
console.log(createObject('')); // note: not handled properly

The last one is not handled properly, but to me it looks more like an invalid input.

Tests

I've added provided solutions to jsperf test and compiled your TypeScript at Playground but for some reason it doesn't work as intended.

So, here is the benchmark. FYI check results in different browsers.


Python implementation of atoi I made an implementation of atoi (ascii to integer) in Python a while ago for fun, and I'd like to know what I could do to improve it.

class CannotConvertToInteger(Exception):
  """A non-numeric character was present in the string passed to atoi"""
  pass

def atoi(string : str) -> int:
  sign = multiplier = 1
  val = 0
  if string[0] == '-':
    string = string[1:]
    sign = -1
  elif string[0] == '+':
    string = string[1:]
  for i in string[::-1]:
    code = ord(i)
    try:
      if ((code > 57) or (code < 48)):
        raise CannotConvertToInteger
      else:
        val += (code - 48) * multiplier
        multiplier *= 10
    except CannotConvertToInteger:
      return print('Cannot convert string to an integer!')
  return (val * sign)

test_string = input('Enter an optionally signed integer: ')
result = atoi(test_string)
if result:
  print('It was a valid int! atoi() returned:', result)
else:
  print('It was an invalid int! atoi() returned:', result)

input()

One specific question I have is if it's bad practice to return a print call, as a way to print an error and simultaneously return from the function? I did that so that I could print the error and return None on the same line.

Python already has an exception that denotes that the value you passed is inappropriate somehow. It is ValueError, which is what the built-in int also raises if a wrong string is passed.

In addition, defining a nice readable error which you can raise, only to catch it directly within the function and to return None (the output of print) and print to the terminal is not ideal. Just let the exception rise to the caller of the function, it should be their problem if the function is used in a bad way (which you are telling them all about with that exception).
You should avoid magic constants. What are 57  and 48? Either give them names and use compound comparisons:

zero = ord("0")
nine = ord("9")

if not zero <= code <= nine:
    ...

Or, maybe even better, write a isdigit function:

def isdigit(s):
    return s in set("0123456789")

Which can be slightly sped up by using the standard library string module:

from string import digits

DIGITS = set(digits)

def isdigit(s):
    return s in DIGITS

Incidentally, don't shadow the standard library string module, just call the input s or x, as int does.

Note that there are also str.isdigit, but this unfortunately also returns true for unicode digits, such as all of ¹²³⁴⁵⁶⁷⁸⁹⁰. Only with a whitelist can you fully control what counts as a digit in your case.
Instead of iterating over the string and directly calling ord, you can use map (and reversed):

for code in map(ord, reversed(string)):
    ...

You could also iterate over multiplier (or rather its exponent) at the same time using enumerate:

for exponent, code in enumerate(map(ord, reversed(string))):
    ...
    value += (code - zero) * 10 ** exponent

Actually directly manipulating ASCII values is not the most robust (although it works). Instead you could just make a dictionary that maps strings to integer values:

VALUES = {c: d for d, c in enumerate(DIGITS)}

Using string[0] to check for a sign character can fail if the empty string is passed. Instead you can use str.startswith("+") or even str.startswith(("+", "-")). This will just return False for an empty string.
With all this done, your function can actually easily be extended to arbitrary bases (but let's stick to maximum base 36, like int, i.e. all digits and lowercase letters):

DIGITS = string.digits + string.ascii_lowercase
VALUES = {c: d for d, c in enumerate(DIGITS)}

def isdigit(s, base=10):
    return s in DIGITS[:base]

def atoi(x : str, base : int = 10):
    if not 2 <= base <= 36:
        raise ValueError("Only 2 <= base <= 36 currently supported")
    sign = 1
    if x.startswith(("+", "-")):
        if x[0] == "-":
            sign = -1
        x = x[1:]
    value = 0
    for exp, c in enumerate(reversed(x)):
        if c not in VALUES or VALUES[c] >= base:
            raise ValueError(f"{c} is not a valid digit in base {base}")
        value += VALUES[c] * base ** exp
    return sign * value

This works, as demonstrated below:

atoi("12345")
# 12345
atoi("+12345")
# 12345
atoi("-12345")
# -12345
atoi("12345", base=6)
# 1865
atoi("12345", base=5)
# ValueError: 5 is not a valid digit in base 5
atoi("101010", base=2)
# 42
atoi("1234567890abcdef", base=16)
# 1311768467294899695
atoi("1234567890abcdefghijklmnopqrstuvwxyz", base=36)
# 3126485650002806059265235559620383787531710118313327355
atoi("")
# 0
atoi("111", base=1)
# ValueError: Only 2 <= base <= 36 currently supported
atoi("Az", base=62)
# ValueError: Only 2 <= base <= 36 currently supported

You should surround your calling code with a if __name__ == "__main__": guard to allow importing from this module from another script without the user input/output being run:

if __name__ == "__main__":
    x = input('Enter an optionally signed integer: ')
    try:
        print('It was a valid int! atoi() returned:', atoi(x))
    except ValueError:
        print('It was an invalid int!)


Counting unival subtrees - Follow Up This code is a revised version of the implementation which asked for an advice on improvement. Original post here: Counting unival subtrees
Credits to: [Deduplicator], [Reinderien], [Toby Speight], [chux].
The revised version was applied following changes:

Merge insertRight(), insertLeft() function into createNode function
free resources after use
used const if referenced memory address value is not altered.
fix isTreeUniv() into recursive version
get rid of globals

Revised code:
#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

typedef struct stTree
{
    struct stTree *left;
    struct stTree *right;
    int value;
} stTree;

stTree* createNode(int value, stTree *leftNode, stTree *rightNode)
{
    stTree *node = malloc(sizeof *node);
    if (!node) abort();

    node->left = leftNode;
    node->right = rightNode;
    node->value = value;

    return node;
}

bool isUnivSubTimpl(const stTree *node, const stTree *parent, size_t *count)
{
    if (!node) return 1;    //node without child count as unival subT

    bool r = isUnivSubTimpl(node->left, node, count) &isUnivSubTimpl(node->right, node, count);
    *count += r;

    return (r &node->value == parent->value);
}

size_t countUnivSubT(const stTree *node)
{
    size_t count = 0;
    isUnivSubTimpl(node, node, &count);
    return count;
}

static stTree* findBottomLeft(stTree *node)
{
    while (node->left)
        node = node->left;
    return node;
}

bool freeTree(stTree *node)
{
    if (!node) return true;
    stTree *bottomLeft = findBottomLeft(node);

    while (node)
    {
        if (node->right)
        {
            bottomLeft->left = node->right;
            bottomLeft = findBottomLeft(bottomLeft);
        }

        stTree *old = node;
        node = node->left;
        free(old);
    }
        return true;
}

int main(void)
{
    //build a tree
    stTree *rootNode =
        createNode(0,
            createNode(1, NULL, NULL),
            createNode(0,
                createNode(1,
                    createNode(1, NULL, NULL),
                    createNode(1, NULL, NULL)
               ),
                createNode(0, NULL, NULL)));

    printf("total universal subree: %u\n", countUnivSubT(rootNode));
    if (freeTree(rootNode))
        printf("memory released\n");

}

Good job - it's nice to see improvements from the reviews you've received.
I would recommend using logical && rather than bitwise & in the computation r &node->value == parent->value.  Results should be the same, but programmers expect to see logical operators with boolean values.
The computation of r cannot use && as is, because the right-hand side does need to be evaluated for its side-effects, even when the left-hand side is false.  I'd consider rewriting as separate expressions, so a future maintainer doesn't "correct" the & to &&:
bool l_un = isUnivSubTimpl(node->left, node, count);  /* updates *count */
bool r_un = isUnivSubTimpl(node->right, node, count); /* updates *count */
bool r = l_un && r_un;

Identifiers beginning with is are reserved for Standard Library extension, so I'd advise changing the name isUnivSubTimpl, particularly as it has external linkage.
It isn't clear to me why freeTree() returns a bool, as it only ever returns true.
Building the tree could be simplified a little with
stTree *createLeafNode(int value)
{
    return createNode(value, NULL, NULL);
}


Tkinter program that displays 28 numbers on the screen I created a simple program that displays 28 numbers on the screen.

from Tkinter import *
import random

class Game(Frame):

    def __init__(self, parent):
        Frame.__init__(self, parent, background="white")
        self.parent = parent
        self.parent.title("Centered window")
        self.pack(fill=BOTH, expand=1)
        self.centerWindow()

    def centerWindow(self):

        w = 850
        h = 530

        sw = self.parent.winfo_screenwidth()
        sh = self.parent.winfo_screenheight()

        x = (sw - w)/2
        y = (sh - h)/2
        self.parent.geometry('%dx%d+%d+%d' % (w, h, x, y))

def main():

    global tile_frame_column  # the labels that go on the screen of individual numbers
    tile_frame_column = {}

    global tiles_make_number  # dictionary of all the individual numbers
    tiles_make_number = {}

    global tile_frame_column_counter
    tile_frame_column_counter=0

    global tiles_make_number_counter
    tiles_make_number_counter=0

    global xpos
    xpos=35

    global ypos
    ypos=50

    tile_numbers = ['1', '2', '3', '4', '5', '6', '1', '2', '1', '2', '1', 'l', '1', '3', '4', '2', '1', '2', '1', '1', '2', '1', '1', '6', '4', '3', '2', '2']

    root = Tk()
    ex = Game(root)

    def add_number_to_screen():
        global tile_frame_column_counter
        global tiles_make_number_counter
        global tile_frame_column
        global xpos
        global ypos

        if not tile_numbers:
            return

        rand = random.choice(tile_numbers)
        tile_frame_column[tile_frame_column_counter] = Button(root, text=rand, font="Helvetica 16 bold")

        tile_frame_column[tile_frame_column_counter].place(x=xpos, y=ypos)
        tile_numbers.remove(rand)  # remove that tile from list of tiles
        xpos += 80
        if (len(tiles_make_number) % 7 == 0) & (len(tiles_make_number) > 0):
            xpos = 35
            ypos += 80
            tile_frame_column[tile_frame_column_counter].place(x=xpos, y=ypos)
            xpos += 80
        tiles_make_number[tiles_make_number_counter] = rand
        tile_frame_column_counter += 1
        tiles_make_number_counter += 1
        root.after(10, add_number_to_screen)

    root.after(10, add_number_to_screen)
    root.mainloop()

if __name__ == '__main__':
    main()

I want the main() function to be initialized, though. Whenever a game object is created I want it to automatically add the main function -- meaning add numbers to the screen. I think the way my code is now, when a game object is created it only creates the screen.

Any ideas?

For most of your question you will need to ask stack overflow specific question on each thing you want to do.

For example: You could ask a question about how to change the display of a button when said button is clicked. This kind of question is best suited for stack overflow.

However I can help you with making your existing code a bit better.

Lets take everything in your program and place it into a class. This will serve to make working with the code a much less painful process as we can work with class attributes and we do not need to worry about global variables. (Globals are bad Mmm Kaaa!)

Here is your code reworked into a class. For the rest you should ask a series of question on stack overflow :)

A few notes: Try to be more consistent with naming conventions. ClassName, functionName, variable_name. This will make the code easier to read and maintain. You might also want to change self.parent.geometry('%dx%d+%d+%d' % (w, h, x, y)) to use the .format() method instead as it is the correct method going forward and % is being fazed out.

from Tkinter import *
import random

class Game(Frame):

    def __init__(self, parent):
        Frame.__init__(self, parent, background="white")

        self.parent = parent
        self.parent.title("Centered window")
        self.pack(fill=BOTH, expand=1)
        self.centerWindow()

        self.tile_frame_column = {}
        self.tiles_make_number = {}
        self.tile_frame_column_counter=0
        self.tiles_make_number_counter=0
        self.xpos=35
        self.ypos=50

        self.tile_numbers = ['1', '2', '3', '4', '5', '6', '1', '2', '1', '2', '1', 'l', '1', '3', '4', '2', '1', '2', '1', '1', '2', '1', '1', '6', '4', '3', '2', '2']

        self.addNumbers()

    def centerWindow(self):
        w = 850
        h = 530
        sw = self.parent.winfo_screenwidth()
        sh = self.parent.winfo_screenheight()
        x = (sw - w) / 2
        y = (sh - h) / 2
        self.parent.geometry('%dx%d+%d+%d'%(w, h, x, y))

    def addNumbers(self):
        if not self.tile_numbers:
            return
        rand = random.choice(self.tile_numbers)
        self.tile_frame_column[self.tile_frame_column_counter] = Button(root, text=rand, font="Helvetica 16 bold")

        self.tile_frame_column[self.tile_frame_column_counter].place(x=self.xpos, y=self.ypos)
        self.tile_numbers.remove(rand)  # remove that tile from list of tiles
        self.xpos += 80
        if (len(self.tiles_make_number) % 7 == 0) & (len(self.tiles_make_number) > 0):
            self.xpos = 35
            self.ypos += 80
            self.tile_frame_column[self.tile_frame_column_counter].place(x=self.xpos, y=self.ypos)
            self.xpos += 80
        self.tiles_make_number[self.tiles_make_number_counter] = rand
        self.tile_frame_column_counter += 1
        self.tiles_make_number_counter += 1
        root.after(10, self.addNumbers)

if __name__ == '__main__':
    root = Tk()
    ex = Game(root)
    root.mainloop()


Is a filter a good way to accomplish a dynamic class in angular? I want to apply a different badge color with bootstrap depending on a number.

I wrote a filter to accomplish this...

angular.module('Demo')
    .filter('reserveStatus', function() {
        return function(num) {
            var klass = "";

            num = parseInt(num, 10);

            if (num > 2 && num < 5) {
                klass = "alert-success";
            }
            else if (num >= 5 && num < 10) {
                klass = "alert-warning";
            }
            else if (num >= 10) {
                klass = "alert-danger";
            }

            return klass;
        }
    });

And it's used with interpolation like this...

Is this a good way to accomplish this?

Fun question, I think this is okay.

I would name klass -> class and I would consider building a map for the values:

var classMap = [
  { from : 3 , to : 5       , class : 'alert-success' },
  { from : 5 , to : 10      , class : 'alert-warning' },
  { from : 10, to : Infinity, class : 'alert-danger'  }
];


Capitals and countries game This is my simple game that gives you a country and has you enter the corresponding capital. What do you think? (Before running it you need to do score.txt.)

package com.company;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.PrintWriter;
import java.util.Random;
import java.util.Scanner;

public class Main {

    public static void main(String[] args) throws FileNotFoundException{

        Random r = new Random();

        Scanner input = new Scanner(System.in);

        String[] capital = {"Lisbon", "Madrid", "Paris", "Berlin", "Warsaw", "Kiev", "Moscow", "Prague", "Rome"};
        String[] country = {"Portugal", "Spain", "France", "Germany", "Poland", "Ukraine", "Russia", "Czech Republic", "Italy"};
        Boolean[] was = new Boolean[9];

        int score = 0;
        int good = 0;
        int bad = 0;

        File file = new File("score.txt");
        Scanner lastscoreopener = new Scanner(file);
        String lastscore = lastscoreopener.nextLine();
        System.out.print("Your last score was: " +lastscore +"\n");

        for (int d = 1; d<4; d++) {
            int randomint = r.nextInt(9);
            while (was[randomint] == Boolean.FALSE){
                randomint = r.nextInt(9);
            }
            was[randomint] = false;

            while (was[randomint] == true);
            for (int x = 0; x < 3; x++) {
                System.out.print("What's capital of " + country[randomint] + "\n");
                String answer = input.nextLine();
                if (answer.toLowerCase().equals(capital[randomint].toLowerCase())) {
                    if (x==0){
                        score = score+3;
                        good++;
                    }
                    else if(x==1){
                        score = score+2;
                        good++;
                    }
                    else if (x==2){
                        score = score+1;
                        good++;
                    }
                    System.out.print("Good. Your score is" +score +". \n");
                    break;
                } else {
                    if (x != 2){
                        System.out.print("Bad. You have " +(2-x) +" chances. ");}
                    if (x == 2){
                        System.out.print("Bad. Good answer is" +capital[randomint] +"\n");
                    }
                    bad++;
                }
            }
        }
        System.out.print("End of game. Good answers:" +good +". Bad answers: " +bad +". Score: " +score);

        PrintWriter saver = new PrintWriter("score.txt");
        saver.println(score);
        saver.close();

    }
}

String[] capital = {"Lisbon", "Madrid", "Paris", "Berlin", "Warsaw", "Kiev", "Moscow", "Prague", "Rome"};
String[] country = {"Portugal", "Spain", "France", "Germany", "Poland", "Ukraine", "Russia", "Czech Republic", "Italy"};

Two string arrays with members that are supposed to match by index?  Extremely difficult to maintain, as I learned from my own experience.  Use a map instead, so you can do something like:

String capital = countryCapitalMap.get(countryName);

This will help ensure there is an entry for each country/capital and make it so you only have to update things in one place.

Don't hardcode the length of the array:

int randomint = r.nextInt(9);

Use the length of the array instead, or for a map, use the size() method.

Your naming isn't always clear.  How do I know what r stands for without seeing the definition?  How did you determine to use d as your loop variable?

Just use the ordinary false keyword instead of the explicit Boolean.FALSE field here:

while (was[randomint] == Boolean.FALSE)


Matrix Exponentiation in Python I recently wrote a python code for matrix exponentiation.

Here's the code:

from typing import List
Matrix = List[List[int]]

MOD = 10 ** 9 + 7

def identity(n: int) -> Matrix:
    matrix = [[0] * n for _ in range(n)]

    for i in range(n):
        matrix[i][i] = 1

    return matrix

def multiply(mat1: Matrix, mat2: Matrix, copy: Matrix) -> None:
    r1, r2 = len(mat1), len(mat2)
    c1, c2 = len(mat1[0]), len(mat2[0])

    result = [[0] * c2 for _ in range(r1)]

    for i in range(r1):
        for j in range(c2):
            for k in range(r2):
                result[i][j] = (result[i][j] + mat1[i][k] * mat2[k][j]) % MOD

    for i in range(r1):
        for j in range(c2):
            copy[i][j] = result[i][j]

def power(mat: Matrix, n: int) -> Matrix:
    res = identity(len(mat))

    while n:
        if n & 1:
            multiply(res, mat, res)

        multiply(mat, mat, mat)

        n >>= 1

    return res

def fib(n: int) -> int:
    if n == 0:
        return 0

    magic = [[1, 1],
             [1, 0]]

    mat = power(magic, n - 1)

    return mat[0][0]

if __name__ == '__main__':
    print(fib(10 ** 18))

I would mainly like to improve this code performance wise, but any comments on style is also much appreciated.

Thanks!

Using the numpy module for numerical computations is often better as the code is generally simpler and much faster. Here is a version adapted to use numpy:

from typing import List
import numpy as np
Matrix = np.matrix

MOD = 10 ** 9 + 7

def power(mat: Matrix, n: int) -> Matrix:
    res = np.identity(len(mat), dtype=np.int64)

    while n:
        if n & 1:
            np.matmul(res, mat, out=res)
            res %= MOD

        np.matmul(mat, mat, out=mat)
        mat %= MOD # Required for numpy if you want correct results
        n >>= 1

    return res

def fib(n: int) -> int:
    if n == 0:
        return 0

    magic = np.matrix([[1, 1], [1, 0]], dtype=np.int64)
    mat = power(magic, n - 1)
    return mat[0,0]

if __name__ == '__main__':
    print(fib(10 ** 18))

As you can see, using numpy reduce significantly the size of the code since it provide primitive for creating and computing matrices (eg. matrix multiplication).
Numpy is also faster since it uses fast native code to perform the computations. However, here, your matrices are too small so that numpy can provide any speed-up.
Also note that numpy as a downside: it does not support large integers as python does. So, the code above works well as long as you do not increase too much the value of MOD. You can use dtype=object to force numpy to support large integers but it will be slower (especially on bigger matrices).

Besides using numpy, you can also specialize your code to compute 2x2 matrices much faster in this specific case. Here is the result:

from typing import List
Matrix = List[List[int]]

MOD = 10 ** 9 + 7

def identity_2x2() -> Matrix:
    return [1, 0, 0, 1]

def multiply_2x2(mat1: Matrix, mat2: Matrix, copy: Matrix) -> None:
    a00, a01, a10, a11 = mat1
    b00, b01, b10, b11 = mat2

    copy[0] = (a00 * b00 + a01 * b10) % MOD
    copy[1] = (a00 * b01 + a01 * b11) % MOD
    copy[2] = (a10 * b00 + a11 * b10) % MOD
    copy[3] = (a10 * b01 + a11 * b11) % MOD

def power_2x2(mat: Matrix, n: int) -> Matrix:
    res = identity_2x2()

    while n:
        if n & 1:
            multiply_2x2(res, mat, res)

        multiply_2x2(mat, mat, mat)

        n >>= 1

    return res

def fib(n: int) -> int:
    if n == 0:
        return 0

    magic = [1, 1, 1, 0]
    mat = power_2x2(magic, n - 1)
    return mat[0]

if __name__ == '__main__':
    print(fib(10 ** 18))

This is faster because the default interpreter (CPython) executes loops very slowly, so it is better to avoid them as much as possible. It is also faster because no additional list is created.

Please note that if you want your code to run faster, you could use the PyPy interpreter.


Creating a Ruby gem that talks to another gem I am working on creating an interface gem that works with the Jira-Ruby gem.

I have several questions and would like a general review for both my code and my attempt at creating a gem (I've never written one before). Any advice on where to go next would also be appreciated.

File structure:

jira_interface
   lib
      jira_interface
          config.rb
          version.rb
          app_interface.rb
      jira_interface.rb
   jira_interface.gemspec
   #...etc files, I used bundle's gem command to set up

My main module (jira_interface.rb) looks like this:

require "jira_interface/version"
require "jira"
require "jira_interface/config"
require "jira_interface/app_interface"

module JiraInterface

  def get_jira_client
      @client = Jira::Client.new(USERINFORMATION)
  end

  def get_project(project_name)
     @client.Project.find(project_name)
  end

end

And my app_interface.rb looks like this:

class AppInterface < JiraInterface
  before_filter :get_jira_client

  def create_issue(issue_desc, project)
    issue = @client.Issue.build
    issue.save({"fields"=>{"summary"=> issue_desc, "project"=> {"id" => project.id}, "issuetype"=> {"id"=> "1"}}})
  end

  def update_issue(issue_id)
    issue = @client.Issue.find(issue_id)

    comment = issue.comments.build
    comment.save({"body"=> "This happened again at #{Date.time}"})
  end

  def get_issues(issue_desc, project_name)
    project = get_project(project_name)

    if project.issues.detect { |issue| issue.fields['summary'].include?(issue_desc) }
      update_issue(issue.id)
    else
      create_issue(issue_desc, project)
    end
  end

end

The goal is pretty simple: call get_issues() from the rails app when an error happens. If the error is already documented, post a comment saying that it happened again. If it's not, then create a new issue.

To the best of my knowledge, this should work because I've been following the examples found online.  However, I've been having a heck of a time just testing it. If anyone could suggest a good way of testing this, that would be much appreciated.

I suggest you take a close look at how the gem you are using is tested. Those tests include mocks for all responses of the API (I assume) and is a good example for testing gems in general.

Notice:

helper methods in the support folder including shared examples
mock data to simulate responses from the source API
custom matchers to make code better readable and reduce repetition of code (DRY-principle)

Those to me are the three most important you should embrace for now.


Basic Collatz program with input validation in Python In chapter 3 of Automate the Boring Stuff with Python by Al Sweigart, one exercise asks to write a program which performs the Collatz sequence.
I wrote a basic version first, then added and restructured some of it to account for an invalid input. Any thoughts or criticisms to make it more concise, organized or legible would be appreciated.
def collatz(number): # This evaluates and prints the sequence of numbers.
        if number % 2 == 0: # If it's an even number.
            print(number // 2)
            return number // 2
        else: # If it's an odd number.
            print(3 * number + 1)
            return 3 * number + 1

def main(): # The main loop inside a function so the ValueError message can appear as many times as needed.
    try:
        user_input = int(input())
        while user_input > 1:
            user_input = collatz(user_input)
        if user_input <= 0: # If a negative integer is entered.
            print('Please enter a positive integer.')
        else:
            print('Enter another integer.')

    except ValueError: # If something other than an integer is entered.
        print('Please enter a valid integer.')

print('Type any positive integer.') # Main prompt.

while True: # This calls the main loop so you can repeat the equation as many times as needed.
    main()

Most of your code deals with obtaining and validating user input. That's
tedious code to write and maintain. In addition, programs premised on
user-interactivity are difficult to execute during development, testing, and
debugging: every execution requires a manual dialogue between the user (you,
the programmer) and the computer. That get annoying very fast. Moreover, your
program provides no clean way to quit. A more powerful approach based on
command-line arguments and the built-in argparse library is shown below.
The second issue is that your collatz() function intermixes algorithmic
computation and reporting (i.e. printing). That's almost
always a bad idea. A more flexible approach is to restrict collatz() purely to
the algorithm and do the printing at a higher level in the program -- either in
main(), if the reporting remains extremely simple, or in a function called by it.
The essence of most of these suggestions is to organize your code more
carefully to keep different tasks more separated: user input collection; input
validation; computation; and reporting.
import argparse
import sys

def main(args):
    # Get user arguments.
    opts = parse_args(args)

    # Make computations.
    sequences = [list(collatz(val)) for val in opts.initial_values]

    # Report to user. Enhance as needed.
    for seq in sequences:
        print(seq)

def parse_args(args):
    # You want the user to provide one or more integers
    # on the command line. The argparse library will handle that
    # automatically for many use cases (e.g, any integer). In your situation,
    # you do need to write a type validation function, but that process
    # is a bit simpler and more focused than your previous validation
    # code, which was mixed with the rest of the program's logic.
    ap = argparse.ArgumentParser()
    ap.add_argument('initial_values', nargs = '+', type = positive_int)
    return ap.parse_args(args)

def positive_int(val):
    # You can make this more robust if needed (eg to prevent values like
    # '12.34' from being accepted), but here's the basic technique.
    try:
        n = int(val)
        1 / n
        return n
    except Exception:
        raise argparse.ArgumentTypeError(f'Invalid positive integer: {val}')

def collatz(n):
    # Relieved of printing, this function becomes simpler, more testable,
    # and more general purpose.
    while n > 1:
        yield n
        if n % 2 == 0:
            n = n // 2
        else:
            n = 3 * n + 1
    yield n

# Invoke main() if the program was executed rather than imported.
# This is important for testability, because testing tools need
# to import your code without executing the script in its entirety.
if __name__ == '__main__':
    main(sys.argv[1:])