Project Euler #12 in Scala — Highly divisible triangular numbers I generally work in Python for programming competitions, but I'm teaching myself Scala. As an exercise, I've attempted Project Euler problem #12 in Scala. I hope that somebody familiar with Scala can point out anything I'm doing which is not idiomatic in "functional style" Scala. My code produces the correct result, and is reasonably fast, so I'm mostly looking for coding style advice.

object HighlyDivisibleTriangularNumber {
  val triangleCache = collection.mutable.Map[Int, Int]()
  val triangle: Int => Int = {
    case 1 => 1
    case n => triangle(n - 1) + n
  }
  def cachedTriangle(n: Int) = triangleCache.getOrElseUpdate(n, triangle(n))

  def nFactors(x: Int): Int = {
    val maxFactor: Int = math.round(math.sqrt(x.floatValue).floatValue)
    val smallFactors: Set[Int] = (1 to maxFactor).filter(f => x % f == 0).toSet
    val allFactors: Set[Int] = smallFactors ++ smallFactors.map(f => x / f)
    allFactors.size
  }

  def main(args: Array[String]): Unit = {
    val triangleNumbers = Iterator.from(1).map(cachedTriangle)
    val firstWith500Factors = triangleNumbers.dropWhile(nFactors(_) < 500).next
    println(firstWith500Factors)
  }
}

I realize that a mutable collection is not a great starting point for functional Scala, but struggled to implement a memoized version of triangle without the mutable map.

Your triangular number "cache" isn't an idiomatic technique in functional programming.  Since triangular numbers constitute an infinite sequence, it would be more elegant to model them in Scala as an infinite lazy stream, especially since you only require sequential access.  I would define it this way:

val triangleNumbers = Stream.from(1).scan(0)(_ + _)

Or, if you prefer to have the sequence start with 1 rather than 0:

val triangleNumbers = Stream.from(2).scan(1)(_ + _)

nFactors could be optimized a bit.

I would call the parameter n, which has the connotation of being an integer, instead of x, which has the connotation of being floating point.
The .floatValue casts are superfluous, and it is safe to round the square root down using .intValue rather than sometimes rounding up.
Personally, I would prefer the name middleFactor instead of maxFactor.
The lambda in .filter(f => x % f == 0) could be more idiomatically written as .filter(x % _ == 0).
You don't need to use sets.  Just a count of factors will do, along with a check of whether middleFactor is being double-counted.

def nFactors(n: Int): Int = {
  val middleFactor = math.sqrt(n).intValue
  val smallFactors = (1 to middleFactor).count(n % _ == 0)
  2 * smallFactors - (if (middleFactor * middleFactor == n) 1 else 0)
}

In main, the .dropWhile(…).next would be more clearly written as .find(…).

def main(args: Array[String]): Unit = {
  triangleNumbers.find(nFactors(_) > 500).foreach(println)
}


Find a nested property in an object This works fine and returns an object from a nested "structure" by Id.

I would like to know if you could suggest a better approach, possible a faster one.

<script>
var data = {
    item: [
        {
            itemNested: [
                {
                    itemNested2: [{
                        id: "2"
                    }
                    ]
                }
            ]
        }
    ]
};

function findById(obj, id) {
    var result;
    for (var p in obj) {
        if (obj.id === id) {
            return obj;
        } else {
            if (typeof obj[p] === 'object') {
                result = findById(obj[p], id);
                if (result) {
                    return result;
                }
            }
        }
    }
    return result;
}

var result = findById(data, "2");
console.log(result);
</script>

From a once over:

Be careful for js structure with cycles (like nodes in the DOM), you will get into an infinite loop.
This will also not find dupes, that may or may not be a problem
You also want to think about whether you want to prevent finding id in the inherited values with o.hasOwnProperty(p)
Dont check for if (obj.id === id) { in your loop, it's a waste of cycles. Check prior
Personally I tend to go either for the Spartan o or the full object, obj seems meh

I would use this version:

function findById(o, id) {
    //Early return
    if( o.id === id ){
      return o;
    }
    var result, p;
    for (p in o) {
        if( o.hasOwnProperty(p) && typeof o[p] === 'object' ) {
            result = findById(o[p], id);
            if(result){
                return result;
            }
        }
    }
    return result;
}


Is device mounted? Both UUID and device names accepted I am trying to write is_device_mounted script, which in turn will serve a greater purpose in my home Linux system.

It does not even have an error reporting function included and as you can see, I have made it clean for readers. My intention here is to review the code for general Linux. But if you are on a *BSD, I would appreciate your feedback too!

The first version of the script follows:

#!/bin/sh

set -eu

is_empty_string() { [ -z "${1}" ]; }
sanitize_device_string() { printf '%s' "${1}" | grep '^/dev/' | head -n 1; }
is_block_device() { [ -b "${1}" ]; }
is_device_uuid_identified() { printf '%s' "${1}" | grep -F '/by-uuid/'; }
translate_uuid_to_device_name() { readlink -f -n /dev/disk/by-uuid/"${1}"; }

is_device_mounted()
{
    if [ -n "${device_name}" ]; then
        # 1. basic regex should be working across platfotms
        #    tested on FreeBSD, OpenBSD, NetBSD with success
        #    I prefer the starting with (^) rather than filtering throung all text
        # 2. /proc/mounts is not available on all *BSDs, needs revision
        proc_mounts=$( grep "^${device_name} " /proc/mounts )
        [ -n "${proc_mounts}" ]
    fi
}

[ "${#}" -ne 1 ] && { echo "Invalid number of arguments."; exit 1; }
readonly raw_input_string=${1}

is_empty_string "${raw_input_string}" && { echo "The given argument is empty."; exit 1; }
readonly device_string=$( sanitize_device_string "${raw_input_string}" )

is_empty_string "${device_string}" && { echo "The given argument is not a device path."; exit 1; }
! is_block_device "${device_string}" && { echo "The given argument is not a block device."; exit 1; }
readonly block_device=${device_string}

if is_device_uuid_identified "${block_device}"
then
    readonly device_name=$( translate_uuid_to_device_name "${block_device}" )
else
    readonly device_name=${block_device}
fi

if is_device_mounted "${device_name}"
then
    echo "The device: ${block_device} IS mounted."
else
    echo "The device: ${block_device} IS NOT mounted."
fi

Nice work.  I approve of set -eu, and the script pleases Shellcheck.

Here's the things I'd consider changing.

I think error messages should go to the error stream.  Example:

[ "${#}" -ne 1 ] && { echo "Invalid number of arguments." >&2; exit 1; }
#                                                         ^^^ here

Instead of using the negation operator, I'd replace the form ! test && error with plain test || error like this:

is_block_device "${device_string}" || { echo "The given argument is not a block device." >&2; exit 1; }

The script doesn't work when I use other links to block devices, such as those in /dev/disk/by-label.  I'd fix that by abandoning the /by-uuid/ test, and instead following symlinks until a real file or dangling link is found:

resolve_symlink() {
    f="$1"
    while [ -h "$f" ]
    do f=$(readlink -f -n "$f")
    done
    printf '%s' "$f"
}

is_empty_string "${device_string}" && { echo "The given argument is not a device path." >&2; exit 1; }
is_block_device "${device_string}" || { echo "The given argument is not a block device." >&2; exit 1; }

readonly device_name=$(resolve_symlink "$device_string")

if is_device_mounted "$device_name"
then

Why does is_device_mounted ignore its argument and use $device_name instead?

Minor issue: we assume that the block device name contains no regex metacharacters here:

grep "^${device_name} "

That's probably a fair assumption on a non-weird Linux system; I normally use Awk for robust versions such tests ($1 = $device_name, with a suitable -v option) but I don't know how well that meets your portability goals.

If using grep (without the non-standard -q option), then it's usual to discard the output, and use grep's exit status directly, rather than capturing the output and testing it's non-empty.

Minor/style: I'm not a big fan of using braces for every variable expansion.  I prefer to reserve them for when they are really needed, and that seems to be the usual idiom.


Cython functions for generating NumPy date ranges I have rewritten my python code in cython in order to speed up it quite a bit. However after checking the code in jupyter it seems that part of it is still compiled as python code, therefore its not being sped up enough.

My function is pretty basic, it gets start date, end date and creates array of additional dates based on that and some other conditions.

The thing is I’m a bit clueless how could I change the highlighted yellow part of code of the date_range_np function as seen on the attached imaged. Because I’m already using numpy arrays so I thought it would be compiled as cython code. There is also one loop that I think is slowing down the function but so far I wasn’t able to come up with any replacement that sginificatly sped up the function. Actually speed is very important as those functions are executed thousands of times.
Any ideas how could I refactor it to speed it up?

Here is my code:

%%cython -a
import numpy as np
cimport numpy as np
from datetime import datetime

cpdef np.int64_t get_days(np.int64_t year, np.int64_t month):
    cdef np.ndarray months=np.array([31,28,31,30,31,30,31,31,30,31,30,31])
    if month==2:
        if (year%4==0 and year%100!=0) or (year%400==0):
            return 29
    return months[month-1]

cpdef np.ndarray[np.int64_t] date_range_np(np.int64_t start, np.int64_t end, char* freq):
    cdef np.ndarray res
    cdef np.int64_t m_start
    cdef np.int64_t m_end
    if freq.decode("utf-8")[len(freq)-1] == "M":
        m_start = np.int64(start).astype('M8[D]').astype('M8[M]').view("int64")
        m_end = np.int64(end).astype('M8[D]').astype('M8[M]').view("int64")
        res = np.arange(m_start, m_end-2, np.int64(freq[:(len(freq)-1)])).view("M8[M]").astype("M8[D]").view("int64")
        return np.array([np.min([x+datetime.fromtimestamp(start*24*60*60).day-1, x+get_days(datetime.fromtimestamp(start*24*60*60).year,datetime.fromtimestamp(start*24*60*60).month)]) for x in res])
    elif freq.decode("utf-8")[len(freq)-1] == "D":
        return np.arange(start, end-2, np.int64(freq[:(len(freq)-1)]))

cpdef np.ndarray[np.int64_t] loanDates(np.int64_t startDate,np.int64_t endDate,np.int64_t freq):
    # if frequency indicates repayment every n months
    if int(12 / freq) == 12 / freq:
        # Generate date range and add offset depending on starting day
        #print(date_range_np(start=startDate, end=endDate, freq=(str(-int(12 / freq)) + "M").encode("utf8")))
        ts = date_range_np(start=startDate, end=endDate, freq=(str(-int(12 / freq)) + "M").encode("utf8"))
    else:
        ts = date_range_np(start=startDate, end=endDate, freq=(str(-int(365 / freq)) + "D").enocode("utf8"))
    #print(ts)

    if ts.shape[0] == 0:
        return np.array([])
    elif ts.shape[0] >= 1 and ts[0] > startDate:
        ts = np.delete(arr=ts, obj=0)
    if ts[ts.shape[0]-1] < endDate:
        ts = np.delete(arr=ts, obj=-1)
    if ts[0] != startDate:
        ts = np.insert(ts,0,startDate)

    # If no dates generated (start date>end date)
    ts = ts

    # If last date generated is not end date add it
    return ts.astype('int64')

And the function highlighted:

If you want to test the functions you can try:

%timeit date_range_np(20809,17986, b"-1M")

I get about 1ms for that function.

I'm sorry, but this code is really hard to read. I must admit I don't know Cython too well, so I won't be able to comment too much on that part. But anyways, here are a few comments, in random order.

While Cython does not fully support docstrings (they do not show up in the interactive help), this should not prevent you from adding some to explain what the different functions do and what arguments they take.
You seem to be doing np.int64(start).astype('M8[D]').astype('M8[M]').view("int64") quite a lot. As far as I can tell, this extracts the month from a date, which was given as an integer(?). There is quite possibly a better way to do that (using the functions in datetime), but they might be slower. Nevertheless, you should put this into its own function.
You do freq.decode("utf-8")[len(freq)-1] twice. Do it once and save it to a variable. Also, freq[len(freq)-1] should be the same as freq[-1] and freq[:len(freq)-1] the same as freq[:-1]. This is especially costly as len(freq) is \$\mathcal{O}(n)\$ for char *, in Cython.
You create datetime.fromtimestamp(start*24*60*60) three times, once each to get the day, month and year. Save it to a variable and reuse it.
The last two comments in loanDates seem not to be true anymore:

# If no dates generated (start date>end date)
ts = ts

# If last date generated is not end date add it
return ts.astype('int64')

The documentation seems to recommend against using C strings, unless you really need them. If I read the documentation correctly you could just make the type of freq str and get rid of all your encode("utf-8") and decode("utf-8") code.
The definition of the months array is done every time the function get_days is called. In normal Python I would recommend making it a global constant, here you would have to try and see if it makes the runtime worse.
Python has an official style-guide, PEP8. Since Cython is only an extension, it presumably also applies here. It recommends surrounding operators with whitespace (freq[len(freq) - 1]), using lower_case for all function and variable names and limiting your linelength (to 80 characters by default, but 120 is also an acceptable choice).

In the end, taking 1ms to create a date range is already quite fast. As you said this is already faster than pandas.daterange (which does a lot of parsing of the input first, which you avoid by passing in numbers directly). You might be able to push it down to microseconds, but you should ask yourself if and why you need this many dateranges per second.


Find median of two sorted arrays, with complexity of min(log(m),log(n)) I wrote a program which gets two sorted arrays a and b (with sizes m and n respectively), and calculates the median value of these arrays without merging them.

Here is my code:

#include <iostream>
using namespace std;

int findMedian(int a[], int b[], int m, int n);
int max(int a, int b);
int min(int a, int b);

void main()
{

    int m, n, *a, *b, i;

    cout << " enter the size of the first array: ";
    cin >> m;
    a = new int[m];
    cout << " \nenter the size of the second array:";
    cin >> n;
    b = new int[n];

    cout << " \nthe numbers of the first array in sorted way: ";
    for (i = 0; i < m; i++)
        cin >> a[i];

    cout << "\nplease enter your numbers of the second array in sorted way: ";
    for (i = 0; i < n; i++)
        cin >> b[i];

    cout << "\nthe median of the merged array is : " << findMedian(a, b,  m,  n)<< '\n';

    free(b);
    free(a);
}

int findMedian(int a[], int b[], int m, int n)
{

    int imin = 0, imax = m, half_len = (m + n + 1) / 2;
    int i, j, max_of_left, min_of_right;

    while (imin <= imax)
    {
        i = (imin + imax) / 2;
        j = half_len - i;

        if (i < m && b[j - 1] > a[i])
            imin = i + 1;

        else if (i > 0 && a[i - 1] > b[j])
            imax = i - 1;

        else
        {

            if (i == 0)
                max_of_left = b[j - 1];

            else if (j == 0)
                max_of_left = a[i - 1];

            else
                 max_of_left = max(a[i - 1], b[j - 1]);

            if ((m + n) % 2 == 1)
                return max_of_left;

            if (i == m)
                min_of_right = b[j];
            else if (j == n)
                min_of_right = a[i];
            else
                min_of_right = min(a[i], b[j]);

                return min(max_of_left,min_of_right);
        }
    }

}

int max(int a, int b)
{
    return a > b ? a : b;
}

int min(int a, int b)
{
    return a < b ? a : b;
}

One particular thing I'd like to improve is that although the program seems to run correctly, I get this warning; I can't understand why it happens and don't know how it should be fixed:

warning C4715: 'findMedian': not all control paths return a value

Consistent whitespace

    cout << "\nthe median of the merged array is : " << findMedian(a, b,  m,  n)<< '\n';

isn't even consistent within the same line about whether << should have spaces on both sides or not. I'm surprised your editor didn't auto-format it.

There is also a wide variety of whitespace in the strings which are used to prompt for input.

Also in this category,

                return min(max_of_left,min_of_right);

appears to be indented too far. Well, either that or there are some missing {} to include it with the previous statement in a single block.

Memory management

void main()
{
    ...
    a = new int[m];
    ...
    b = new int[n];
    ...
    free(b);
    free(a);
}

Are you sure? This looks fishy to me. There's no call to malloc or friends, so why is there a call to free?.

Warning

Let's simplify the code a bit...

int findMedian(int a[], int b[], int m, int n)
{
    int imin = 0, imax = m, half_len = (m + n + 1) / 2;
    int i, j, max_of_left, min_of_right;

    while (imin <= imax)
    {
        ...
    }
}

Now can you see why "not all control paths return a value"?

Hint: what's the loop invariant?

Performance claims

The title of this question is "Find median of two sorted arrays, with complexity of min(log(m),log(n))". That's impossible: if I guarantee that the second array is of length at most 1, you're promising to find the median of the other array with complexity bounded above by 0.

I think you can claim \$\log(m+n)\$, but some comments to justify correctness and performance would be nice.


(Codewars Kata) Memoised Log Cutting (I am not sure of the official CS name for the problem in question, so I just gave it the name of the kata).

Problem

  CLEAR CUTTER'S NEEDS YOUR HELP!

  The logging company Clear Cutter's makes its money by optimizing the price-to-length of each log they cut before selling them. An example of one of their price tables is included:

# So a price table p
p = [ 0,  1,  5,  8,  9, 10]

# Can be imagined as:
# length i | 0  1  2  3  4  5 *in feet*
# price pi | 0  1  5  8  9 10 *in money*

  They hired an intern last summer to create a recursive function for them to easily calculate the most profitable price for a log of length n using price table p as follows:

def cut_log(p, n):
   if (n == 0):
      return 0
   q = -1
   for i in range(1, n+1)
      q = max(q, p[i] + cut_log(p, n-i))
   return q

  An example of the working code:

# 5ft = $10, BUT 2ft + 3ft = 5ft -> $5 + $8 = $13 which is greater in value

  However, their senior software engineer realized that the number of recursive calls in the function gives it an awful running time of 2^n (as this function iterates through ALL 2^n-1 possibilities for a log of length n).

  Having discovered this problem just as you've arrived for your internship, he responsibly delegates the issue to you.

  Using the power of Stack Overflow and Google, he wants you to create a solution that runs in Θ(n^2) time so that it doesn't take 5 hours to calculate the optimal price for a log of size 50. (He also suggests to research the problem using the keywords in the tags)

  (Be aware that if your algorithm is not efficient, it will attempt to look at 2^49 = 562949953421312 nodes instead of 49^2 = 2401... The solution will automatically fail if it takes longer than 6 seconds... which occurs at right around Log 23)

Solution

def cut_log(p, n):
    for i in range(2, len(p)):
        p[i] = max(p[i-k] + p[k] for k in range((i//2)+1))
    return p[n]

It passes all the tests. I'm putting it up for review because I'm pretty proud of it and experience as shown me that there's usually some improvement to be made.

I see three problems with this function:

Why calculate the values up to len(p), when you are actually interested in p[n] (which may be larger or smaller than len(p))? In the case of n > len(p) your code actually raises an exception (and it seems not to be excluded by the rules, but also not to be checked by the online judge).
This is not really using memoization, since you calculate everything again whenever the function is called.
You mutate p, which might lead to unexpected results (in general), although here it happens to work out fine.

The second (and third) problem are not so easy to fix (probably only by memoizing all different p ever seen and for each of them the maximum n used). Also the online judge may or may not use the same p again.

The first one however is an easy fix:

def cut_log(p, n):
    for i in range(2, n + 1):
        p[i] = max(p[i-k] + p[k] for k in range((i//2)+1))
    return p[n]


C Split function review I started learning C a week ago, and here is my implementation for the split function:

char** my_split(const char* str, char delim, int* size)
{
    int index = 0, start;
    char** results = NULL;
    *size = 0;

    do
    {
        if ((*size) % 10 == 0) //top floor reached
        {
            results = realloc(results, ((*size) + 10) * sizeof(char*)); //extend the space for another 10 pointers
            if (results == NULL) //if failed, return NULL
                return NULL;
        }
        start = index;
        while (str[index] != delim && str[index]) //find the delimeter or end of string
        {
            index++;
        }

        int length = index - start; //length of matching string
        results[*size] = malloc((length + 1) * sizeof(char)); //allocate memory in the size of the string, and another char for '\0'
        if (results[*size] == NULL) //if failed, return NULL
            return NULL;

        strncpy(results[*size], &str[start], length); //copy the matching string to the array
        results[*size][length] = 0; //end the string
        (*size)++; //add 1 to the total size
    } while (str[index++]);

    results = realloc(results, (*size) * sizeof(char*)); //trim unused bytes
    return results;
}

Is there anything I could do better (memory, performance and error handling)? Maybe in the logic of the function?

Also, feel free to criticize my writing style, as I want it to be more readable and "correct".

Note: I would take everything I say with a grain of salt.  It's been a few years since I used C on a regular basis, and even at my peak with C, I was never very familiar with the standard or certain best practices.

Your function is responsible for cleaning up memory, yet when you bail in your NULL returns, you don't do that.

In particular, if a realloc fails, the memory already allocated stays, and if allocating the second dimension-ed array fails, you still need to free what you have already allocated before returning.

A review of a few code points:

I would highly consider abstracting away the memory stuff, but if you don't, I would consider doubling the array size instead of adding ten.

Consider a fairly long string being split on space.  Assume it ends up being 1000 split strings.  This means 100 allocations, and allocating is fairly slow.  If you were to double, it would be 10 allocations.

Doubling wastes, at most, sizeof(type) * n/2 bytes, but has at most, lg n allocations (note: very rough math)
Doubling using more memory but is typically faster

size should be an unsigned type (I would use size_t)
As previously mentioned, you have potential memory leaks
As your code exists, there's really no advantage over strtok

strtok doesn't very well allow storing into an array, but your function requires fairly manual memory management anyway

The malloc/strncpy combination could be changed to use strndup and be a lot simpler (I think strndup is standard?)
You could actually use strtok inside of your function to avoid doing the index calculations

Part of your code is recreating strtok
The other part of your code is taking that strtok functionality and using it to push things into an array
What you have now will (probably) perform better than strtok

I'm not familiar enough with standard practices of C to know what the expected way would be, but something about the the memory handling feels odd to me:

const char[] str = "Hello World";
int size = 0;
char** str_parts = my_split(str, ' ', &size);
if (str_parts != NULL) {
    //Let's do something with the parts
    for (int i = 0; i < size; ++i) {
        printf("Part %d: '%s'\n", size, str_parts[i]);
    }
    //Now we have to clean up (could technically be in the printing loop)
    for (int i = 0; i < size; ++i) {
        free(str_parts[i]);
    }
    free(str_parts);
}

I might consider abstracting the memory away to a generalized vector or a specialized string vector.

That could bring your API to something like:

const char[] str = "Hello World";
cvector parts;
//A vector of char*'s that will call free() on each element before destructing
cvector_init(&parts, sizeof(char*), free);
if (my_split(&parts, ' ')) {
    for (size_t i = 0; i < parts.size; ++i) {
        //I'm a bit rusty on C, so I'm not sure if the char* cast is needed or not
        printf("Part %d: '%s'\n", size, (char*) cvector_get(&parts, i));
    }
}
cvector_destory(&parts);

The downside to this would be that you'd be tying your other code to a specific API (though bastardizing the vector into a char** would actually be fairly easy if you wrote the cvector API in a particular way -- though then you'd be depending on implementation details, which is usually a bad idea), and this vector would perform worse than a plain 2d array.  (Though if you wrote a string specialized vector, the performance could be very similar and, depending on design, you could even use it as a char** without worrying much about the implications.)


Non-testable class wrapping Java Properties with basic logic I have written a simple java.util.Properties wrapping class. Unfortunately, in my opinion, it isn't testable at the moment and I don't have any idea how to improve it. I could use PowerMockito to change properties after loading, or add a setter for them, or inject some class that provides properties, but I don't like any of that. I would like to avoid exposing properties by making it public or by getter/setter.

My application details:

Spring boot
my class is injected by @Bean mechanic
CONF_FILE_NAME contains key=value formatted properies with names same as in Keys subclass.

I would like to unit-test logic in this class (both public method) without relying on values in file.

PropertiesManager.java:

package pl.propertiesdemo;

import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.Properties;

public class PropertiesManager
{
    private final String secretToken = "SECRET";

    public final class Keys
    {
        public final static String KEY1 = "KEY1";
        public final static String KEY2 = "KEY2";
    }

    private final String CONF_FILE_NAME = "conf_file.conf";

    private Properties properties;

    public PropertiesManager() throws IOException
    {
        properties = new Properties();
        InputStream in = new FileInputStream(CONF_FILE_NAME);
        properties.load(in);
    }

    public String getProperty(String name)
    {
        String property = properties.getProperty(name);
        if (property == null) {
            System.out.println("Property " + name + " not found in properties file");
        }
        return property;
    }

    public HashMap<String, String> getAllPublicProperties()
    {
        HashMap<String, String> map = new HashMap<>();

        properties.keySet().stream().map(f -> (String)f).filter(f -> !f.contains(secretToken)).forEach(f -> map.put(f, properties.getProperty(f)));

        return map;
    }
}

PropertiesDemo.java: (main method and usage of both public methods)

package pl.propertiesdemo;

import java.io.IOException;

public class PropertiesDemo
{
    public static void main(String[] args) throws IOException
    {
        PropertiesManager manager = new PropertiesManager();
        System.out.println(manager.getProperty(PropertiesManager.Keys.KEY1));
        System.out.println(manager.getAllPublicProperties());
    }
}

conf_file_conf: (file loaded by PropertyManager)

KEY1=value_of_key1
KEY2=value_of_key2

PropertiesManager.java: (JUnit test I have so far)

package pl.propertiesdemo;

import java.io.IOException;
import java.util.HashMap;

import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;

public class PropertiesManagerTest extends TestCase
{
    public PropertiesManagerTest(String testName)
    {
        super(testName);
    }

    public static Test suite()
    {
        return new TestSuite(PropertiesManagerTest.class);
    }

    private PropertiesManager getPropertiesManager()
    {
        PropertiesManager prop = null;
        try {
            prop = new PropertiesManager();
        } catch (IOException ioe) {
            fail();
        }
        return prop;
    }

    public void testGetProperty()
    {
        PropertiesManager prop = getPropertiesManager();
        String value = prop.getProperty(PropertiesManager.Keys.KEY1);
        assertEquals("value_of_key1", value);
    }

    public void testGetAllProperties()
    {
        PropertiesManager prop = getPropertiesManager();
        HashMap<String,String> values = prop.getAllPublicProperties();

        assertTrue(values.size() > 0);
        assertEquals(0, values.keySet().stream().filter(k -> k.contains("SECRET")).count());
    }
}

I don't like my tests, because right now they are dependant of value in real configuration file.

Your class is non-testable mainly because :

it reads datas from disk/network
it doesn't use any injection

Firstly, please note that Spring has some utility to read property file and fills java beans automaticly. You should check this if you don't want to reinvent the wheel. ;)
Also, Properties is a fairly old school class in Java and isn't used that much recently.

If you do want to have your own implementation, the let's see what we can do line-by-line, shall we ?

private final String secretToken = "SECRET";

Either you plan on making this variable (and then it should be injected in the constructor) or this is a constant (so it should be private static final).

public final class Keys
{
    public final static String KEY1 = "KEY1";
    public final static String KEY2 = "KEY2";
}

I don't really get what you were trying to accomplish here. Why didn't you put those constants in the main classes ? I'd remove this code altogether IMHO.

private Properties properties;

This is set in the constructor so this is a good candidate for final ;)

public PropertiesManager() throws IOException
{
    properties = new Properties();
    InputStream in = new FileInputStream(CONF_FILE_NAME);
    properties.load(in);
}

Maybe some people will disagree but I'm no big fan of constructor that do I/O.
Also this is non-configurable : what if you want to change the file name ? what if you want to read not from a file but from a string or from the console ?
You should read about dependency injection if you've never heard of it before.
For a quick introduction, it's a principle that some codes delegates the configuration of the object to the calling code allowing for more and easier code reuse.

The InputStream is not closed which may lead to ressource-leak. See https://docs.oracle.com/javase/7/docs/api/java/lang/AutoCloseable.html#close() ;)

IMO I'd have a constructor that takes a Properties object. However, this may pose others problems (such as the Properties object still being accessed by another object and be modified directly).
So, I'd consider using a static factory method that takes a Reader and returns the PropertiesManager. The calling code will be the one responsible to close the stream.
This allows you to inject any time of Reader making your code more reusable.

public String getProperty(String name)
{
    //...

This is fine. As a side-note : you shouldn't use System.out.println for log in production code.

public HashMap<String, String> getAllPublicProperties()

Your return type shouldn't be HashMap ; when possible you should code against the interface that most suits your need, in this case : Map.

HashMap<String, String> map = new HashMap<>();

properties.keySet().stream().map(f -> (String)f).filter(f -> !f.contains(secretToken)).forEach(f -> map.put(f, properties.getProperty(f)));

This code may be dangerous as it is not thread-safe due to the forEach that modify a non thread-safe object. From a FP pov, you should try to delegate to the collect method (check https://docs.oracle.com/javase/9/docs/api/java/util/stream/Collectors.html).

This leads us to the following code :

package pl.propertiesdemo;

import static java.util.stream.Collectors.toMap;

import java.io.IOException;
import java.io.Reader;
import java.util.Map;
import java.util.Properties;
import java.util.function.Function;

public class PropertiesManager {
    public static final String DEFAULT_CONF_FILE_NAME = "conf_file.conf";

    private static final String SECRET_TOKEN = "SECRET";

    private final Properties properties;

    public static PropertiesManager createFrom(final Reader r) throws IOException {
        var properties = new Properties();
        properties.load(r);
        return new PropertiesManager(properties);
    }

    protected PropertiesManager(final Properties properties) {
        this.properties = properties;
    }

    public String getProperty(final String name) {
        String property = properties.getProperty(name);
        if (property == null) {
            System.out.println("Property " + name + " not found in properties file");
        }
        return property;
    }

    public Map<String, String> getAllPublicProperties() {
        return properties.keySet()
                .stream()
                .map(f -> (String) f)
                .filter(f -> !f.contains(SECRET_TOKEN))
                .collect(toMap(Function.identity(), f -> properties.getProperty(f)));
    }
}

Now that your object don't read from disk and can be easily injected, the object instantiation in the test class may look like this :

PropertiesManager prop = null;
try {
    prop = PropertiesManager.createFrom(new StringReader("someKey=someValue"));
} catch (IOException ioe) {
    fail();
}


Rock, Paper, Scissors - Python game This is my attempt at making a 'rock, paper, scissors' game in python. The game takes the user's input and the computer's random selection and calculates who has won. All feedback appreciated.

from random import randint

def getUser():
    print('Rock, Paper, Scissors: ')
    userIn = input().lower()
    if userIn == 'r':
        userIn = 0
    elif userIn == 'p':
        userIn = 1
    elif userIn == 's':
        userIn = 2
    else:
        getUser()
    return(userIn)

def getComp():
    compIn = randint(0,2)
    return(compIn)

def calc(user, comp):
    if user == comp:
        print("Draw!")
    elif user == 0:
        if comp == 1:
            print("You lose - ",  comp, "smothers", user)
        else:
            print("You win -", user, "smashes", comp)
    elif user == 1:
        if comp == 2:
            print("You lose - ", comp, "slices", user)
        else:
            print("You win -", user, "smothers", comp)
    elif user == 2:
        if comp == 0:
            print("You lose - ", comp, "smashes", user)
        else:
            print("You win -", user, "slices", comp)

again = 'yes'
while again != 'no':
    calc(getUser(), getComp())
    again = input('Would you like to play again? ')
print('Thank you for playing.')

Like @wb9688 mentioned in the comments, the way you currently determine the winner is fine and working, but it does not scale very well. If you wanted to add two more possible moves (for example lizard and spock), your if..else tree will become very large and hard to maintain.

It would therefore be better to just save which move beats which other move (for example in a dictionary):

beats = {'paper': 'scissors',
         'rock': 'paper',
         'scissors': 'rock'}

Then you can just do:

computer_choice = getComp()
user_choice = getUser()

if user_choice == beats[computer_choice]:
    print "{} beats {}, you win!".format(user_choice, computer_choice)
elif computer_choice == beats[user_choice]:
    print "{} beats {}, computer wins!".format(computer_choice, user_choice)
else:
    print "Tie."

If you also want to add the verbs, just add a dictionary for that as well:

verb = {'rock': 'smashes',
        'paper': 'smothers',
        'scissors': 'slices'}

And then do:

if user_choice == beats[computer_choice]:
        print "{} {} {}, you win!".format(user_choice, verb[user_choice], computer_choice)
...

You can have a look at my answer to a similar question on how easy it is to extend this framework to more moves: Stone, Paper, Scissors in Python

Note that you would have to change the structure of verb to a nested dictionary, because "Paper disproves Spock" instead of "Paper smothers Spock".

Your function getUser should be modified, as well. Right now, you call it recursively in case the user enters an illegal move. However, you never return the result of that recursive call!

In python, using a while loop is usually preferred in this case, because Python does not do tail recursion and so will hit a maximum recursion depth at some point:

def getUser():
    while True:
        user_input = raw_input("Make a choice: {} ".format(beats.keys()))
        if user_input in beats:
            return user_input
        print "Invalid choice."

Note: In Python-3.x, use input instead of raw_input and print(x) instead of print x.

Finally, in your function getComp, you can just return the result right away. However, it needs to be modified to use the keys of beats, instead of integers:

import random

def getComp():
    return random.choice(beats.keys())


Trigonometric Subplots with Matplotlib The code consists of 4 functions that result in a figure with 3 subplots:

a sine wave
a cosine wave
and the superposition of both (a bichromatic wave).

I have shown in 4 ways, the first two are basically plot the same way, but the generation of the waves are different.  I also show different methods, by adding subplots directly using plt.subplots(3, 1), also by adding one at a time using plt.subplot(3,1,i), and the last function uses the sine and cosine plot data to create the bichromatic wave.

I would like to know what to improve for the code, and also what to improve for the methods in using matplotlib ..?

import matplotlib.pyplot as plt
import math
pi = math.pi

ex_1 = "1. Using lists (comprehension) to contain A*sin(a*x), B*cos(b*x), and the superposition. \
Using plt.subplots to created the plots."

def trig_subplots_1(x=[], amplitude=(1, 1), frequency=(1, 1)):

    sine = [amplitude[0]*math.sin(frequency[0]*i) for i in x]
    cosine = [amplitude[1]*math.cos(frequency[1]*i) for i in x]
    superposition = [s+c for s,c in zip(sine, cosine)]

    fig, ax = plt.subplots(3,1)

    ax[0].plot(x, sine)
    ax[0].set_title('Sine wave')

    ax[1].plot(x, cosine)
    ax[1].set_title('Cosine wave')

    ax[2].plot(x, superposition)
    ax[2].set_title('Bichromatic wave')

    plt.tight_layout()
    plt.show()

ex_2 = "2. Generate A*sin(a*x), B*cos(b*x), and the superposition by list.append in a for-loop. \
Using plt.subplots to created the plots."

def trig_subplots_2(x=[], amplitude=(1, 1), frequency=(1, 1)):

    sine, cosine, superposition = [], [], []
    for i in x:
        sine.append( amplitude[0]*math.sin(frequency[0]*i) )
        cosine.append( amplitude[1]*math.cos(frequency[1]*i) )
        superposition.append( sine[-1] + cosine[-1] )

    fig, ax = plt.subplots(3,1)

    ax[0].plot(x, sine)
    ax[0].set_title('Sine wave')

    ax[1].plot(x, cosine)
    ax[1].set_title('Cosine wave')

    ax[2].plot(x, superposition)
    ax[2].set_title('Bichromatic wave')

    plt.tight_layout()
    plt.show()

ex_3 = "3. Similar as no.1, but creating each subplot axes by using plt.subplot(nrows, ncols, index)."

def trig_subplots_3(x=[], amplitude=(1, 1), frequency=(1, 1)):

    sine = [amplitude[0]*math.sin(frequency[0]*i) for i in x]
    cosine = [amplitude[1]*math.cos(frequency[1]*i) for i in x]
    superposition = [s+c for s,c in zip(sine, cosine)]

    plt.subplot(3,1,1)
    plt.title('Sine wave')
    plt.plot(x, sine)

    plt.subplot(3,1,2)
    plt.title('Cosine wave')
    plt.plot(x, cosine)

    plt.subplot(3,1,3)
    plt.title('Bichromatic wave')
    plt.plot(x, superposition)

    plt.tight_layout()
    plt.show()

ex_4 = "4. Similar as no.1, but the superposition wave is generated after plotting the sine and cosine then and \
get the y values from each plot using .get_ydata()."

def trig_subplots_4(x=[], amplitude=(1, 1), frequency=(1, 1)):

    sine = [amplitude[0]*math.sin(frequency[0]*i) for i in x]
    cosine = [amplitude[1]*math.cos(frequency[1]*i) for i in x]

    fig, ax = plt.subplots(3,1)

    sine_plot = ax[0].plot(x, sine)
    ax[0].set_title('Sine wave')

    cosine_plot = ax[1].plot(x, cosine)
    ax[1].set_title('Cosine wave')

    superposition = sine_plot[0].get_ydata() + cosine_plot[0].get_ydata()
    ax[2].plot(x, superposition)
    ax[2].set_title('Bichromatic wave')

    plt.tight_layout()
    plt.show()

x = [i*0.01*pi for i in range(1000)]
amplitude = (1, 1)
frequency = (1, 0.75)

print(ex_1)
trig_subplots_1(x, amplitude, frequency)
print(ex_2)
trig_subplots_2(x, amplitude, frequency)
print(ex_3)
trig_subplots_3(x, amplitude, frequency)
print(ex_4)
trig_subplots_4(x, amplitude, frequency)

Your code is repeating a little bit. For example,

ax[0].plot(x, sine)
ax[0].set_title('Sine wave')

ax[1].plot(x, cosine)
ax[1].set_title('Cosine wave')

ax[2].plot(x, superposition)
ax[2].set_title('Bichromatic wave')

This can be rewritten using a helper function (or alternatively a for-loop), which may look something like this:

def plot_subroutine(axis, xdata, ydata, title):
    axis.plot(xdata, ydata)
    axis.set_title(title)

y1 = ... # sine
y2 = ... # cosine
y3 = ... # bichromatic
yn = (y1, y2, y3)

title1 = 'Sine Wave'
title2 = 'Cosine Wave'
title3 = 'Bichromatic Wave'
titles = (title1, title2, title3)

fig, ax = plt.subplots(3,1)

Then instead of calling each plot subroutine individually (as below)

plot_subroutine(ax[0], x, yn[0])
plot_subroutine(ax[1], x, yn[1])
plot_subroutine(ax[2], x, yn[2])

You can instead do something like

if len(ax) == len(yn):
    for i in range(len(ax)):
        plot_subroutine(ax[i], x, yn[i])
plt.show()

The for-loop has the advantage that you can later change the number of output plots without changing too much of your original code (if say, you wanted to output 2 or 4 plots, or perhaps change the ydata to something entirely different).

This may or may not be relevant, but it may be nice to associate each individual plot with a specific legend label, marker style, and/or color, all of which can be handled using matplotlib. This can be especially useful for overlayed plots (especially considering that your data consists of sin(x) and cos(x) over the same x-interval.


Tic Tac Toe in JavaScript (+ jQuery) Here is my first project that I made with JavaScript and jQuery with a little bit of HTML and CSS:

var currentTurn = "X"; //X or O
var turnNumber = 0;

function generateFields() {
  var y = 1;
  var top = 50;
  for (var i = 0; i < 9; i++) {
    var x = $("#board").append(
      "

"
    );
    var fieldI = $("#nr" + i);

    if (i == 0 || i == 3 || i == 6) {
      fieldI.css("margin-left", "5px");
    }
    if (i >= 3 && i <= 8) {
      fieldI.css("margin-top", "15px");
    }

    fieldI.click(chooseField);
  }
}

$(generateFields());

/*
How table looks like:
    var fieldsOnBoard =
    ["nr0", "nr1", "nr2",
    "nr3", "nr4", "nr5",
    "nr6", "nr7", "nr8",];
 */

function chooseField() {
  if (currentTurn == "X") {
    $(this).html(
      ""
    );
    currentTurn = "O";
  } else if (currentTurn == "O") {
    $(this).html(
      ""
    );
    currentTurn = "X";
  }

  $(this).css({ opacity: 0, scale: 0 });
  $(this).removeClass("hover-field");
  $(this).transition({ opacity: 1, scale: 1 }, "in-out");

  turnNumber++;

  $(".flipper").toggleClass("flipped");

  $(this).off("click");
  if (checkIfWin() != null) {
    endOfGame(checkIfWin());
  }
}

function checkIfWin() {
  var winConditions = [
    ["nr0", "nr1", "nr2"],
    ["nr3", "nr4", "nr5"],
    ["nr6", "nr7", "nr8"],
    ["nr0", "nr3", "nr6"],
    ["nr1", "nr4", "nr7"],
    ["nr2", "nr5", "nr8"],
    ["nr0", "nr4", "nr8"],
    ["nr2", "nr4", "nr6"]
  ];
  for (var x in winConditions) {
    // $("#" + winConditions[x][0] + "> img").attr("class");
    //console.log($("#" + winConditions[x][0] + "> img").attr("class") + " " + $("#" + winConditions[x][0] + "> img").attr("class") + " " + $("#" + winConditions[x][0] + "> img").attr("class"));

    var field1 = $("#" + winConditions[x][0] + "> img").attr("class");
    var field2 = $("#" + winConditions[x][1] + "> img").attr("class");
    var field3 = $("#" + winConditions[x][2] + "> img").attr("class");
    //console.log(`pierwsza = ${field1} druga = ${field2} trzecia = ${field3}`);
    if (field1 != undefined && field2 != undefined && field3 != undefined) {
      if (field1 === field2 && field2 === field3 && field1 === field3) {
        return field1;
      }
    }
  }

  var fieldsOnBoard = [
    "nr0",
    "nr1",
    "nr2",
    "nr3",
    "nr4",
    "nr5",
    "nr6",
    "nr7",
    "nr8"
  ];
  for (var z in fieldsOnBoard) {
    var full = false;

    if ($("#" + fieldsOnBoard[z] + "> img").attr("class") == undefined) {
      break;
    }
    full = true;
  }
  if (full) {
    return "draw";
  }

  return null;
}

function endOfGame(x) {
  console.log(x);

  $("." + x)
    .first()
    .clone()
    .removeClass(".field, " + x)
    .attr("id", "winned")
    .appendTo("#board");
  $(".field, #lines").hide();
  $("#winned").transition({ height: "100px", width: "100px" });
  $("#winned").transition({
    height: "200px",
    width: "200px",
    color: "#e74c3c"
  });
  if (x != "draw") {
    $("#board").append("
HAS WON!
");
  } else {
    $("#board").append("
DRAW
");
    setTimeout(() => resetBoard(), 2000);
  }
  $("#winned").click(() => {
    resetBoard();
  });

  function resetBoard() {
    $(".field, #lines").show();
    $("#board").transition({ scale: 0, rotate: "-90deg" });
    $("#board")
      .children("[id != 'lines']")
      .remove();
    generateFields();
    $("#board").transition({ scale: 1, rotate: "0deg" });
  }
}

Codepen (with HTML and CSS)

Congrats on building this game, it's pretty cool!
I just have a few suggestions for you:

Don't use jQuery. You really don't need it for anything. The native versions of finding elements in your DOM are much faster than jQuery's methods, and you don't have to load a library.
Don't append HTML as strings. Instead, create dom elements. Then you can add id's, classes and event handlers without querying for them in the DOM.
Put CSS rules in a CSS file, and use javascript only to add and remove CSS classes from elements.

Let me try to rewrite this function following my own rules:

function generateFields() {
  var y = 1;
  var top = 50;
  for (var i = 0; i < 9; i++) {
    var x = $("#board").append(
      "

"
    );
    var fieldI = $("#nr" + i);

    if (i == 0 || i == 3 || i == 6) {
      fieldI.css("margin-left", "5px");
    }
    if (i >= 3 && i <= 8) {
      fieldI.css("margin-top", "15px");
    }

    fieldI.click(chooseField);
  }
}

Could become:

function generateFields() {
  let y = 1;
  let top = 50;
  let board = document.querySelector("board");
  for (var i = 0; i < 9; i++) {
    let field = document.createElement("div");
    board.appendChild(field);
    field.classList.add("field hover-field");
    field.id = "nr"+i;
    if (i == 0 || i == 3 || i == 6) {
      field.addClass("marginLeft");
    }
    if (i >= 3 && i <= 8) {
      field.addClass("marginTop");
    }
    field.addEventListener("click", chooseField);
  }
}

CSS

   .marginLeft {
      margin-left: 5px;
   }
   .marginTop {
      margin-top: 15px;
   }

There's more to improve, but hopefully these tips are helpful!


Checking for Duplicates, Except I have a query that returns records that have duplicate names within a given DBSTATUS. I used to do this with a direct connection to the remote oracle database server with this query. (Names changed to protect the innocent, but otherwise, this is my code.)

SELECT  SUBSTR(NAME, 1, 3) AS CATID,
    TABLE1.NAME,
    TABLE1.DESC1 AS DESCRIPTION,
    TABLE1.DBKEY,
    TABLE1.DBSTATUS,
    ISSUES.NAME_CT
FROM  (SELECT  NAME, DBSTATUS, COUNT(NAME) AS NAME_CT
       FROM  TABLE1
       WHERE  DBSTATUS IN (1,2)
       GROUP BY NAME, DBSTATUS
       HAVING COUNT(NAME) > 1) ISSUES,
       TABLE1
WHERE ISSUES.NAME = TABLE1.NAME
    AND ISSUES.DBSTATUS = TABLE1.DBSTATUS
ORDER BY CATID, NAME, DBKEY

The requirements have changed and now I need to exclude a list of CATIDs from this report, but only when they have a DBSTATUS of 2. This is what I came up with, but I don't much like the duplicated select in the WHERE statement. Since the exclusion table resides on the local server, I have switched to an OPENQUERY instead of a direct connection to the remote server. How can I improve this?

SELECT *
FROM OPENQUERY(REMOTE_ORACLE_SERVER,
'SELECT  SUBSTR(NAME, 1, 3) AS CATID,
    TABLE1.NAME,
    TABLE1.DESC1 AS DESCRIPTION,
    TABLE1.DBKEY,
    TABLE1.DBSTATUS,
    ISSUES.NAME_CT
FROM  (SELECT  NAME, DBSTATUS, COUNT(NAME) AS NAME_CT
       FROM  TABLE1
       WHERE  DBSTATUS IN (1,2)
       GROUP BY NAME, DBSTATUS
       HAVING COUNT(NAME) > 1) ISSUES,
       TABLE1
WHERE ISSUES.NAME = TABLE1.NAME
    AND ISSUES.DBSTATUS = TABLE1.DBSTATUS
ORDER BY CATID, NAME, DBKEY
')drv
WHERE (CATID IN(SELECT CATID FROM RACK_CATIDS) AND DBSTATUS = 1)
OR (CATID NOT IN(SELECT CATID FROM RACK_CATIDS))

Let me point out some things that I know can be improved so that other answers can focus on my problem.

SELECT *

I shouldn't be selecting all like this. Even if there's not really a performance hit because open query needs to get all of the meta data anyway, this is a bad habit to get into. Selects should be explicit in what we're selecting.

'SELECT  SUBSTR(NAME, 1, 3) AS CATID,

CATID isn't a very good alias. CategoryId would be much better and clearer to someone following me someday.

   WHERE  DBSTATUS IN (1,2)

This needs a comment saying what these DBSTATUSes actually represent. Mr. Maintainer would have to go find the lookup table in order to find out. (And this query gives no hint that the lookup table even exists.)


Remove all values duplicates from list I'm new to programming in general, started on boxing day. I'm trying to
learn python.
The list remove method only removes the first instance of the value found.
In the code below I attempted to remove all the instances.
Are there better ways of accomplishing this? Is this efficient? Any and all feedback is wanted.

wordlist = ["dog", "cat", "mouse", "giraffe", "cat"]
remove_queue = []
userinput = input("remove: ")
for i in range(len(wordlist)):
    if wordlist[i] == userinput:
        remove_queue.append(i)

""""reversed the list, so when deleting a value, the position of the other
values don't change"""
remove_queue.sort(reverse=True)
for i in range(len(remove_queue)):

"""uses the remove_queue list to find the position of value that needs to be
removed, and removes it. """
    del wordlist[remove_queue[i]]
print(wordlist)

Iterating backwards was smart, but, yes, there is a much simpler way to do this using a list comprehension – this is how one typically filters a list in Python.

filtered_words = [word for word in word_list if word != user_input]

Note that this creates a new list object rather than altering the original word list in place.

Your current code isn't terribly inefficient – it iterates over the list twice rather than once, but both are asymptotically linear.

If you were to keep that structure, there are a number of small things that could be done to make the code more Pythonic. Compare the following loops to your own code:

for i, word in enumerate(word_list):
    if word == user_input:
        remove_queue.append(i)

for i in remove_queue[::-1]:
    del word_list[i]

These do the same thing, but are much more readable once one knows that enumerate can be used to iterate over both the index and element at once and that list[::-1] is a common method to iterate over a list backwards (albeit, slightly inefficient).

A final note is that I have updated all two-word names to use snake-case.


How can I make this code safer against XSS attacks? I'm not sure what I'm doing wrong, but I run the code using a Web Vulnerability software and I get a lot of XSS attacks against the code. Is there something I could do better?

// connect to database
include("dbconnect.php");

$firstName = $_POST['firstName'];
$lastName = $_POST['lastName'];
$email = $_POST['email'];
$phone = $_POST['phone'];
$category = $_POST['category'];

$add = $db->prepare("INSERT INTO contacts SET firstName=(?), lastName=(?), email=(?), phone=(?), category=(?)");
$add->bind_param('sssss', $firstName, $lastName, $email, $phone, $category);

$firstName = $db->real_escape_string($firstName);
$lastName = $db->real_escape_string($lastName);
$email = $db->real_escape_string($email);
$phone = $db->real_escape_string($phone);
$category = $db->real_escape_string($category);

$add->execute();
$add->close();
header("Location:http://localhost/address-book/contact.php");
exit;

?>

Everything works great, but I'm wonder if there's a safer way to go about it.

I wrote it as prepared statements and ran each variable through mysqli_real_escape_string hoping to strip most of the nasty things. But, no lucky.

I'm new to PHP, so this is all a bit alien to me.

XSS attacks affect the whole web, regardless of the stack you're using, you always have to be careful. Based on the redirect url you provided and the queries you execute, I'm assuming you're building an on-line address book. If a client were to fill in the form, saying his name were:

">Gotcha

Your code would simply insert that in the database, properly escaped, of course. The escaping is what mysqli_real_escape_string does. It doesn't sanitize, it protects the Database, not the site. I can't, for example, post something like "bobby'; -- DROP TABLE contacts;", which is a basic example of an injection attack. But storing markup in a DB isn't wrong or dangerous as far as MySQL is concerned, so the data above can be safely inserted, and queried later on. However, when you request a list of contact names, you would get an alert, showing you your cookies. That's annoying, but not that bad. However, Check these examples to get a basic idea of how XSS attacks work, and what they can be used for.
They're used to bypass login systems, send client data to a third party or even inject/alter your page links to link to another, even more mallicious site:

All your links have now changed to link to another site. Your entire site navigation has been compromised.
The first step to prevent this is to sanitize your data:

$email = $_POST['email'];
if (!filter_var($email, FILTER_VALIDATE_EMAIL))
{
    //don't insert, redirect back, with a GENERIC error message
    die('Not all input fields contained the expected data');
}

Why, you might ask, not point out to the client which bit of data failed the checks? Simply because if you were to say "Please fill in valid emai", the attacker knows to not to use that input field, and fill in something like bill.gates@microsoft.com, and try his luck with the other fields.

Another useful thing to do is to avoid actual script tags from being send to the client when using user-supplied data:

echo htmlspecialchars(strip_tags($someUserData));
//or if some markup is allowed:
echo strip_tags($someUserData, '

');

In case some markup is allowed, though, best go about that like wiki does: using custom markup like [paragraph]text[/paragraph], to avoid having to parse the supplied DOM and filter-out all attributes that contain what might be JS.

Here's another site with an array of attack examples, and a how to defend against them.
Here's a list of XXS weak-spots, introduced by HTML5


T-test Independence for a Pandas DataFrame I want to calculate the scipy.stats.ttest_ind() for numeric columns in a pandas DataFrame with the binary target variable.
import pandas as pd
from scipy import stats

def calculate_tStatistic(df, target, numeric_cols):

    """
    Calculate the t-test on TWO RELATED samples of scores, a and b.

    This is a two-sided test for the null hypothesis that 2 related or
    repeated samples have identical average (expected) values.

    Params
    =====================================
    df               : dataframe
    target           : target column name(str)
    numeric_cols     : list of numeric columns

    Return
    =======================================
    results_df       : A dataframe with features and t-statistic p-val
    """

    # create an empty dictionary
    t_test_results = {}
    # loop over column_list and execute code
    for column in numeric_cols:
        group1 = df.where(df[target] == 0).dropna()[column]
        group2 = df.where(df[target] == 1).dropna()[column]
        # add the output to the dictionary
        t_test_results[column] = stats.ttest_ind(group1,group2)
    results_df = pd.DataFrame.from_dict(t_test_results,orient='Index')
    results_df.columns = ['t-statistic','t_stat_pval']

    results_df.reset_index(inplace=True)
    results_df.rename(columns = {"index":"x"}, inplace=True)
    return results_df

Sample
import seaborn as sns
titanic = sns.load_dataset("titanic")

calculate_tStatistic(titanic, "survived", ["age", "fare"])

Results
      x       t-statistic     t_stat_pval
0    age        3.479558       0.000630
1    fare      -1.767759       0.078795

Can someone confirm that I'm actually doing it the right way?
Is there a better / more elegant / more accurate way to do this??

There may be a bug in your code. Look at these two lines:
        group1 = df.where(df[target] == 0).dropna()[column]
        group2 = df.where(df[target] == 1).dropna()[column]

What is happening here? First, the where method sets all rows in which your condition (df[target] == 0) is not true to NaN. Then, you drop the NaN values by using dropna(). The problem: The dropna() method removes all rows where at least one element is NaN, but this not only applies to the rows not meeting your condition, but also to other rows. For example, check out the deck column in the dataset, it has lots of rows with NaN values and all of them get completely removed (that is the entire row). This is not what you want, I guess.
My suggestion:
        group1 = df.loc[df[target]==0, column].dropna()
        group2 = df.loc[df[target]==1, column].dropna()

This code first extracts the column of interest and then removes the NaN values. Not the other way around. In this way, you avoid removing too many lines. In addition, I use loc to select the values, as this is the preferred pandas method, when you need to index rows and columns simultaneously, as opposed to chained indexing like df[idx1][idx2]. Fore more details see here:
https://pandas.pydata.org/pandas-docs/stable/user_guide/indexing.html#returning-a-view-versus-a-copy

A thing I just learned: As an alternative to using the dropna() method, you can set the nan_policy to 'omit' when calling the stats.ttest_ind function:
stats.ttest_ind(group1, group2, nan_policy='omit')


2D matrices addition I have a 2 dimension matrix :

public struct Matrix2D
{
    private double[,] matrix;

    public Matrix2D(double[,] matrix)
    {
        if (matrix == null) throw new ArgumentNullException(nameof(matrix));

        this.matrix = matrix;
    }

    private double[,] Matrix => matrix ?? (matrix = new double[0, 0]);
    public int RowLength => Matrix.GetLength(1);
    public int ColumnLength => Matrix.GetLength(0);
    public double this[int row, int column] => Matrix[row, column];

    public double[,] AsArray() => (double[,])Matrix.Clone();
}

And a method to sum them :

/// <summary>
/// Sums multiple matrices togheter
/// </summary>
/// <param name="matrices">Matrices to add</param>
/// <returns>New matrix with the summated values</returns>
public static Matrix2D Sum(params Matrix2D[] matrices)
{
    if (matrices == null) throw new ArgumentNullException(nameof(matrices));
    if (matrices.Length == 0) return new Matrix2D();
    if (matrices.Length == 1) return matrices[0];

    int rowLength = matrices[0].RowLength;
    int columnLength = matrices[0].ColumnLength;
    double[,] matrix = matrices[0].AsArray();

    for (int matrixIndex = 1; matrixIndex < matrices.Length; matrixIndex++)
    {
        var matrix2D = matrices[matrixIndex];

        if (matrix2D.RowLength != rowLength || matrix2D.ColumnLength != columnLength)
            throw new ArgumentException("Matrixes must have the same size");

        for (int row = 0; row < rowLength; row++)
        {
            for (int column = 0; row < columnLength; row++)
            {
                matrix[row, column] += matrix2D[row, column];
            }
        }
    }

    return new Matrix2D(matrix);
}

At the moment, the complexity is of \$O(mnp)\$, \$m\$ being the number of matrices and \$n\$ being the row length of matrices and \$p\$ the column length.

I'm wondering if there are edge cases that might crash my method, is it a good decision to make my Matrix2D a struct and is there a way to make this more performant? Obviously any other comments are welcomed.

A couple of changes I would suggest.

To Struct or not to Struct:

Structs are useful for value equality situations. This is objects which require constant value equality checks or intend to be used as keys in Dictionaries. However what I see in use is more of an immutable class. While structs are considered to be best practice as immutable objects you can do the same with classes, but you get to sidestep the inevitable equality issue.

You see, if you were to use equality on your current Matrix object you will find it is slow to check. In fact structs can even end up using reflection to check equality in the worst case, which is seriously inefficient. see C# - Value Type Equals method - why does the compiler use reflection?

Because of this you should always overload equality when using structs, if only for speed reasons. See What needs to be overriden in a struct to ensure equality operates properly?  or one of the many other articles online about this.

Your use case doesn't seem to use or need equality, therefore I would suggest a class over a struct. As a general rule if you don't see yourself using the object as a dictionary key or for equality I wouldn't use a struct.

Var:

On a less important note I am always a strong advocate of using var in your code to make refactoring easier. In the following lines you are declaring the type twice:

int rowLength = matrices[0].RowLength;
int columnLength = matrices[0].ColumnLength;
double[,] matrix = matrices[0].AsArray();
for (int row = 0; row < rowLength; row++){...}

There are actually quite a few more places on top of this. This seems trivial but the usage of var over say double[,] or int (for the case of row) means that if you have to refactor the class into a float[,] or to return a long for a size, that your current code would still function with no effort on your part. Var tends to save on time typing long types and is indispensable for refactoring in looping. It can be a bit debatable, as each team has there own standards, but I can tell you on my team we have an "all var all the time" rule which has been a godsend for refactoring.

{'s:

You swap between style of whether you use a trailing set of {'s between lines. For your for loop you use them, but your if statement does not. While I would advocate always using them, even if you only have a single line in the statement, I would say it's more important to be consistent. If you are going to use the style without {'s for single line statements, do so everywhere. If not, which I generally suggest, do not. This is more of a taste issue, and certainly not a law of the land, but worth noting.

Otherwise this looks good. Hope this helps.


Finding all points on the line where distance between each of them is dividable by X recently I received ( during job interview) a challange to write a method for finding a largest subset of points where distance between them is dividable by a set number.

example points array is A =[-3,-2,1,0,8,7,1]

example number we will be dividing by is 3

so for array above  the biggest subset is [-2,1,7,1]
because:

distance between points is counted as follows abs(A[1] - A[2]) = 3

distance between points 7 and -2 is 9 --> 9 % 3 = 0 so its ok.

I wrote code:

var A = [-3, -2, 1, 0, 8, 7, 1];
    var biggestArray = [];
    (A).forEach(element => {
        Z=[];
        Z.push(element);
        for (i = 0; i < A.length; i++) {
            if (element !== A[i]) {
                var distance = Math.abs(element - A[i]);
                if (distance % 3 === 0) {
                    Z.push(A[i]);
                }
            }

        }
        if(Z.length>biggestArray.length){
            biggestArray=Z;
        }
    });
    console.log("biggest Array length:"+biggestArray.length);
    console.log("points: "+biggestArray);

For each element from entry array i check its distance with any other element. If it is %3 then I add this point do result array. After each outer loop iteration i check if current result is bigger than last. If yes then I store new one. I receive correct data at least for array given to me.

This task seems to be quite easy ( am I not seeing something?)
Also it will do n^2 operations. Is there any nice way to tone it down?

I would think that on each iteration you could remove the elements that you include in Z, i.e. your first iteration will return [-3, 0] so you could remove them from A, they will never match anything else. My solution would be:

let input      = [-3, -2, 1, 0, 8, 7, 1];
let result     = [];
let workingSet = input;

while (workingSet.length) {
  let first    = workingSet[0];
  let elements = [];
  let unused   = [];

  workingSet.forEach( value => {
    if ((value - first) % 3 == 0) {
      elements.push(value);
    } else {
      unused.push(value);
    }
  });

  console.log('elements', elements);
  if (elements.length > result.length)
    result = elements;
  workingSet = unused;
}

console.log('result', result);

One note is that you mix old style JS code like var with ES6 features like =>. I would expect code to either stick to using the older JS style for maximum browser compatibility or to stick to the newer JS style consistently.


Comparator based on Double values in Java I have class point that represents point in 2D and I want to sort these points once based on their x-coordinate and second based on their y coordinate. Since x and y are double values my code looks like this:

public static class Point{
    int id;
    public double x;
    public double y;
    Point(int id, double x, double y){
        this.id = id;
        this.x = x;
        this.y = y;
    }
    public static Comparator<Point> xComparator = new Comparator<Point>(){
        @Override
        public int compare(Point t1, Point t2) {
            // TODO Auto-generated method stub
            if (t1.x > t2.x){
                return 1;
            }
            if (t1.x < t2.x){
                return -1;
            }
            else{
                return 0;
            }
        }
    };
    public static Comparator<Point> yComparator = new Comparator<Point>(){
            public int compare(Point t1, Point t2) {
                // TODO Auto-generated method stub
                if (t1.x > t2.x){
                    return 1;
                }
                if (t1.x < t2.x){
                    return -1;
                }
                else{
                    return 0;
                }
            }
        };
}

Is this a good way for defining Comparator? Is there better solution for sorting based on x and on y coordinate? Is this the only way to deal with compare method based on two double values?

Your question does not show how you call/use the Comparators you have defined. Despite that, there are a number of issues I see with your code, and feel I should point out....

When you have data that is sorted, the data tends to last around for a while, and sorted-data is often a very important assumption for complex processing (i.e. if the data was not sorted correctly the computations will fail).

Whenever you have sorted data, and Comparable data, the data should almost always be immutable (cannot possibly change). If the data can change after the sort happens, then the sorted data is no longer sorted.

So, your Point class should probably be an immutable class. At the moment, it has a number of problems with that:

The member fields should be final.
There should be 'getters' for the fields, and the fields themselves should be private (not public like you have).

Your class should look more like:

public static final class Point{
    private final int id;
    private final double x;
    private final double y;

    Point(int id, double x, double y){
        this.id = id;
        this.x = x;
        this.y = y;
    }

    public int getID() {
        return id;
    }

    public double getX() {
        return x;
    }

    public double getY() {
        return y;
    }

    .....

ahhh, I see skiwi just answered a bunch... I'll cover some other stuff from here on....

Reiterating his comments:

the equals and hashCode method he suggests are very important (I was about to add that too).

Right, when you have value-based items like this, it is often convenient to make the Point have a 'natural' ordering. This means that the Point class should implement Comparable<Point>, and then implement the compareTo(Point other) method.

Having two comparators is a problem, you only want one, that does both coordinates.

Consider a compareTo() method like:

public static class Point implements Comparable<Point> {

    ....

    public int compareTo(final Point other) {

        if (other == null) {
            // non null always comes after null
            return 1;
        }

        final int xcomp = Double.compare(this.x, other.x);

        return xcomp == 0 ? Double.compare(this.y, other.y) : xcomp;
    }

This will be one comparator that does both axes.

Then, you can use this Point as a natural sort order, and use it as a key in a TreeMap, and or things like Arrays.sort(pointarray), etc.

Note, I reused the comparison function Double.compare() to do the heavy lifting of the compare.


How to improve my game? I'm building a game.  It is going well at the moment, but I'm not quite happy with my code because is not working the way I want to, due my low knowledge of C#.  I was wondering if you guys could help me out to improve it by giving me some ideas and tips.

This is how I'm checking if the user has completed the levels.

public partial class MainMenu : PhoneApplicationPage
{
public  MainMenu()
{
    InitializeComponent();
    ReadStar1();
    ReadStar2();

}

private void btnbatland_Click(object sender, RoutedEventArgs e)
{
    NavigationService.Navigate(new Uri("/1star/MainPage.xaml", UriKind.Relative));
}

private async  Task ReadStar1()
{
    StorageFolder local = Windows.Storage.ApplicationData.Current.LocalFolder;

    if (local != null)
    {
        var dataFolder = await local.GetFolderAsync("level");
        var file = await dataFolder.OpenStreamForReadAsync("star1.txt");
        string readtxt;
        using (StreamReader streamReader = new StreamReader(file))
        {
            readtxt = streamReader.ReadToEnd();
        }

        if (readtxt == "star1")
            star2_lock.Visibility = Visibility.Collapsed;
           star1_1.Visibility = Visibility.Collapsed;
    }
}

private async Task ReadStar2()
{
    StorageFolder local = Windows.Storage.ApplicationData.Current.LocalFolder;

    if (local != null)
    {
        var dataFolder = await local.GetFolderAsync("level");
        var file = await dataFolder.OpenStreamForReadAsync("star2.txt");
        string readtxt;
        using (StreamReader streamReader = new StreamReader(file))
        {
            readtxt = streamReader.ReadToEnd();
        }

        if (readtxt == "star2")
            star3_lock.Visibility = Visibility.Collapsed;

    }
}

Sometimes it takes 1-4 seconds to load the MainMenu, then after 1-3 to load the function ReadStar1() and ReadStar2().  What could I do to improve its speed, so that the users don't even notice it has changed?

In your ReadStar1 method I assume your forgot the brackets around the if (readtxt == "star1") block? Right now you always collapse visibility of star1_1.
Your ReadStar1 and ReadStar2 methods share lot of code and should be refactored into one method. The only real difference is which file you read and what you do if you read a specific string. Pass in what you want to read and remove the concern of what should happen if you do read it by returning a bool indicating whether the string was found (the purpose of the method should be to determine if a certain file contains a certain string):

private async Task<bool> StarExists(string star)
{
    StorageFolder local = Windows.Storage.ApplicationData.Current.LocalFolder;

    if (local != null)
    {
        var dataFolder = await local.GetFolderAsync("level");
        var file = await dataFolder.OpenStreamForReadAsync(star + ".txt");
        string readtxt;
        using (StreamReader streamReader = new StreamReader(file))
        {
            readtxt = streamReader.ReadToEnd();
        }

        return readtxt == star;
    }
    return false;
}

private async void CheckLevels()
{
    var hasStar1 = await StarExists("star1");
    var hasStar2 = await StarExists("star2");

    if (hasStar1)
    {
        star2_lock.Visibility = Visibility.Collapsed;
        star1_1.Visibility = Visibility.Collapsed;
    }

    if (hasStar2)
    {
        star3_lock.Visibility = Visibility.Collapsed;
    }
}

Then your constructor becomes:

public MainMenu()
{
    InitializeComponent();
    CheckLevels();
}

If you use strings like "star1" and "star2" in multiple places then consider making them public const fields of a class rather than sprinkling string literals around everywhere. If you ever decide to change them you will have to change a lot less code.


Creation of a Select in a View I have the following code in a VIEW:

<select name="selected_budget_mode" class="" onchange="OnBudgetModeChanged(this)">
<option value="0"
        <% if params[:current_budget_mode].to_i == 0 %>selected
        <% end %>><%= show_budget_mode(0) %></option>
<option value="1"
        <% if params[:current_budget_mode].to_i == 1 %>selected
        <% end %>><%= show_budget_mode(1) %></option>
<option value="2"
        <% if params[:current_budget_mode].to_i == 2 %>selected
        <% end %>><%= show_budget_mode(2) %></option>
</select>

I am aware of the select_tag and the options_for_select form helpers, but that implies that I need to create a list with the options and then pass it to the options_for_select.

The budgets_mode are not dynamic. They don't come from calling a service or other function, there are only 3 modes.

What would be the best approach to simplify this code and make it as readable as possible?

You could still use options_for_select without much hassle:

<%= options_for_select (0..2).map { |i| [show_budget_mode(i), i] }, params[:current_budget_mode].to_i %>

Of course, I'd recommend creating the array in the controller or a helper, rather than the view. The view should ideally do as little logic as possible.

Alternatively, you could use content_tag just the keep the code cleaner:

<% (0..2).each do |i| %>
  <%= content_tag :option, show_budget_mode(i), selected: params[:current_budget_mode].to_i == i %>
<% end %>

Again, I'd recommend moving stuff to the controller. Even a current_budget_mode helper method would help clean things up:

class SomeController < ActionController
  helper_method :current_budget_mode, :budget_modes

  def current_budget_mode
    params[:current_budget_mode].to_i
  end

  def budget_modes
    (0..2).to_a
  end
end

which would clean up the view:

<% budget_modes.each do |i| %>
  <%= content_tag :option, show_budget_mode(i), selected: current_budget_mode == i %>
<% end %>

or

<%= options_for_select budget_modes.map { |i| [show_budget_mode(i), i] }, current_budget_mode %>


ASCII value converter I have written VERY simple code that converts a string to a character array and then displays the ASCII value of each character. Let me know if this is the most effective/safe way of doing what I described above.

#include <iostream>

using namespace std;

int main(int argc, char *argv[]){
    //Declare Variables.
    string input;
    int size;

    //Display message to user asking them to enter a string or character.
    cout << "What character/string do you need the ascii value(s) of? " << endl;

    //Assign user input to string input.
    cin >> input;

    //Get string length of user input.
    size = input.length();

    //Copy the string into a character array.
    const char * chars = input.c_str();

    //Itterate through the character array using the string size.
    for(int i = 0; i < size; i++){
            //Return Ascii values of each character.
            cout << "The value of character " << chars[i] << " is " << (int)chars[i] << endl;
    }

    return 0;
}

int main(int argc, char *argv[]){
    //Declare Variables.
    string input;
    int size;

Don't declare variables until you actually need them, and don't declare variables you don't need. In this case, you're not using the arguments passed into main, either don't name them, or use the version of main that takes no arguments:

int main() { /* space or newline between paren and curly is usual */

cin >> input;

I'm not sure this is what you want. If the user enters a "sentence" with spaces in it, you'll only get what they typed up to the first space (not included). If you want the whole line, use std::getline.

std::string input;
std::getline(std::cin, input);

//Copy the string into a character array.
const char * chars = input.c_str();

This comment is dangerously wrong. You don't get a copy at all, you get a pointer to std::string's internals. The string data is not copied, and the pointer returned will become invalid as soon as input's lifetime ends.

std::string's length() member returns an unsigned quantity. While there's no problem in this case assigning it to an int, the correct type to use is:

std::string::size_type size = input.length();

or more conveniently:

auto size = input.length();

(And you'll need to use an unsigned quantity for the loop counter too, to avoid warnings.)

A different way of doing your loop would be to use the range-based variant. It works for std::string.

for (auto c: input) {
  std::cout << "Char \"" << c << "\" has value " << static_cast<int>(c)
            << std::endl;
}

This avoid getting the size altogether.


Add and remove classes on resize I'm just a bit confused and can not write or simplify a jQuery code, that adds or removes classes based on the viewport size. I'm pretty sure there's a simpler and more efficient way to write that trivial code. Unfortunately I can not get it.

Here is the current status:

    // initial state
    if ($(window).width() <= 900) {
        $('.readmoresection span.handler').show();
        $('.readmoresection').addClass('reduced');
    }else{
        $('.readmoresection span.handler').hide();
        $('.readmoresection').removeClass('reduced');
    }

    // on resize
    $(window).on('resize', function(){
        if ($(window).width() <= 900) {
            $('.readmoresection span.handler').show();
            $('.readmoresection').addClass('reduced');
            if($('.readmoresection').hasClass('reduced')){
                $('.readmoresection span.handler').html(' » Weiterlesen');
            }else{
                $('.readmoresection span.handler').html(' » Schließen');
                $('.readmoresection').removeClass('reduced');
            }
        }
        else{
            $('.readmoresection span.handler').hide();
            $('.readmoresection').removeClass('reduced');
        }
    });

Does anyone know a cleaner way?

I've used matchMedia() quite a bit in the past, it works quite well:
https://github.com/paulirish/matchMedia.js/

Example:

if (matchMedia('only screen and (max-width: 480px)').matches) {
  // smartphone/iphone... maybe run some small-screen related dom scripting?
}

Also instead of individually doing .addClass('class') and .removeClass('class') you could use .toggleClass('class').


Finding users who liked a post I have a method liked_by and it doesn't seem like appropriate as per Ruby style. I need to do it as less as possible.

  def liked_by
    if !object.likes.nil?
      like = Like.where(bonus_id: bonus.id)
      users = []
      like.each do |likes|
        liked_user = User.find_by(id: likes.user_id)
        users.push(liked_user.email)
      end
    end
    return users
  end

The main logic is:

I check if any likes to post available (likes.nil?)
I create users array (users = [])
For each like, I find a user with like.user_id and push this user to the users array

In the end, I return users.

Use joined tables to make more performant database calculations and avoid unnecessary models instantiations.

def liked_by
  return [] unless object.likes.present?
  User.distinct.joins(:likes).where(likes: { bonus_id: bonus.id }).pluck(:email)
end


Can only one loop be used to calculate percentage? I've a requirement to find sum of an attribute of each JSON row and then add another attribute to show percentage of corresponding attribute.
The JSON is as follows -

[
    {"value":150, ...},
    {"value":125, ...},
    {"value":100, ...},
    {"value":60, ...},
    {"value":30, ...}
]

Now, I've written following code.

var totalVal = 0;
$.each(data, function(index, value) {
    totalVal += value.value;
});
$.each(data, function(index, value) {
    value.label = Math.round((value.value / totalVal) * 10000) / 100 + "%";
});

I've to use 2 loops to achieve the result. Is there any way to do this in single loop?

EDIT: The code I've written works perfectly. Since I'm contributing back to publicly available GitHub project, I really want to know if the method is most efficient.

No, totatVal won't be a totalVal until you loop through all the rows to find it.
It might be solved by a single loop with some fancy standard functions but at the end it will come down to 2 loops.

It's pure math - you need to count your total value first in order to find a correct % of each of your values.


Int overflow check in Java I have a piece of code that takes a couple of integers and check if performing an addition on the inputs would result in an overflow.

I was wondering if this code is SOLID:

public static boolean CanAdd(int me, int... args) {
    int total = me;
    for (int arg : args) {
        if (total >= 0) {
            if (java.lang.Integer.MAX_VALUE - total >= arg) { // since total is positive, (MaxValue - total) will never overflow
                total += arg;
            } else {
                return false;
            }
        } else {
            if (java.lang.Integer.MIN_VALUE- total <= arg) { // same logic as above
                total += arg;
            } else {
                return false;
            }
        }
    }
    return true;
}

Does anyone have a better (faster) way to achieve the same thing?

I haven't found any input which isn't handled well by your code. Here are some tests:

assertTrue(CanAdd(0, Integer.MAX_VALUE));
assertTrue(CanAdd(0, Integer.MIN_VALUE));
assertTrue(CanAdd(Integer.MIN_VALUE, 0));
assertTrue(CanAdd(-1, Integer.MAX_VALUE));
assertFalse(CanAdd(1, Integer.MAX_VALUE));
assertFalse(CanAdd(-1, Integer.MIN_VALUE));

So, it works but it isn't an easy task to read it. If this isn't a bottleneck in an application I would use a long:

public static boolean canAdd(int... values) {
    long sum = 0;
    for (final int value: values) {
        sum += value;
        if (sum > Integer.MAX_VALUE) {
            return false;
        }
        if (sum < Integer.MIN_VALUE) {
            return false;
        }
    }
    return true;
}

I think it's easier to read and maintain.

Finally, a note: according to Code Conventions for the Java Programming Language the name of your method should be canAdd (with lowercase first letter).

  Methods should be verbs, in mixed case with the first letter
  lowercase, with the first letter of each internal word capitalized.

Edit:

Apache Commons Math also uses long conversion:

public static int addAndCheck(int x, int y)
        throws MathArithmeticException {
    long s = (long)x + (long)y;
    if (s < Integer.MIN_VALUE || s > Integer.MAX_VALUE) {
        throw new MathArithmeticException(LocalizedFormats.OVERFLOW_IN_ADDITION, x, y);
    }
    return (int)s;
}

As well as Guava:

public static int checkedAdd(int a, int b) {
    long result = (long) a + b;
    checkNoOverflow(result == (int) result);
    return (int) result;
}


Find all strings in list which have keywords in it from another list I a list of strings A and a list of keywords B. Now I want to return all strings from list A which have all keywords from B in it as a new list.

def string_search(strings, keys):
    res = []
    for s in strings:
        full = True
        for k in keys:
            if not(k in s):
                full = False
                break
        if full == True:
            res.append(s)
    return res

In which way could I improve this code? I ask purely out of curiosity, since it works perfectly fine and it is not the bottleneck of my processing chain.

Your general coding style is very readable.
You stick to PEP8 and your function and the variables are named reasonably.
You could optimize your code by using the built-in functions filter and all:

def string_search(strings, keys):
    return filter(lambda string: all(key in string for key in keys), strings)

Also do not use if var == True: but if var: instead.


Finding Matches in a Range / Formatting Call to Sub The code below cycles through a number of columns/pairs of columns looking for a match and doing something with it. I have this structure in three different subs with the difference being what I have labeled ' Unique Code '. My two questions are is there an obvious way to combine the two softwareCol loops and how could I structure this better to isolate the identical code.

Sub filterFile()
    ActiveWorkbook.Worksheets("Current File").ListObjects("SoftwareTable").DataBodyRange.Rows.Hidden = True
    Dim currentFile As Worksheet: Set currentFile = ActiveWorkbook.Sheets("Current File")
    Dim cell As Object
    Dim softwareCol As Integer
    Dim targetSoftware As String
    targetSoftware = UserForm1.TextBox1.Value & " " & UserForm1.TextBox2.Value
    With currentFile
        For softwareCol = 0 To 10
            For Each cell In .Range("J2:J" & lastRow(currentFile)).Offset(0, softwareCol * 2)

                ' Unique Code '
                If targetSoftware = cell.Value & " " & cell.Offset(0, 1).Value Then
                    cell.Rows.Hidden = False
                End If

            Next
        Next softwareCol
        For softwareCol = 0 To 5
            If Not (softwareCol = 2 Or softwareCol = 4) Then
                For Each cell In .Range("AF2:AF" & lastRow(currentFile)).Offset(0, softwareCol)

                    ' Unique Code '
                    If targetSoftware = cell.Value & " " Then
                        cell.Rows.Hidden = False
                    End If

                Next
            End If
        Next softwareCol

        ' Unique Code '
        .Activate
        .Range("A1").Select

    End With
End Sub

What you want to do is called refactoring - combining similar methods into a single function.

You have two loops-

 For softwareCol = 0 To 10
 Next softwareCol

 For softwareCol = 0 To 5
      If Not (softwareCol = 2 Or softwareCol = 4)
 Next softwareCol

I don't see an easy way to eliminate those loops.

Let's focus on what you want to combine

If targetSoftware = cell.Value & " " & cell.Offset(0, 1).Value Then
   cell.Rows.Hidden = False
End If
If targetSoftware = cell.Value & " " Then
   cell.Rows.Hidden = False
End If

You can do this in a boolean function -

Private Function NeedsToHide(ByVal targetSoftware As String, ByVal firstValue As String, Optional ByVal secondValue As String = vbNullString) As Boolean
    If targetSoftware = firstValue & " " & secondValue Then
        NeedsToHide = False
    Else
        NeedsToHide = True
    End If
End Function

So now it's just

 For softwareCol = 0 To 10
    'etc
    cell.Rows.Hidden = NeedsToHide(targetSoftware, cell.Value, cell.Offset(0, 1).Value)
 Next softwareCol

 For softwareCol = 0 To 5
    'etc
     cell.Rows.Hidden = NeedsToHide(targetSoftware, cell.Value)
 Next softwareCol


Pay calculator for employee at Foo Corp I completed Assignment 2 for MIT OpenCourseWare 6.092, titled "Foo Corp". The code I wrote below calculates a an employee's total pay:

package fooCorp;

class Employee{

    private double payPerHour; //regular hourly wage
    private double overTimePayPerHour; //regular hourly wage * 1.5
    private double basePayHours; //hours worked under regular hourly wage
    private double overTimeHours;//hours worked under overtime pay per hour
    private double totalHours; //adds basePayHours and overTimeHours
    private double totalPay;//base pay + overtime pay

    public void setPayPerHour(double x) //takes parameter 'x' a d sets that equal to this objects payPerHour variable
    {
        this.payPerHour = x;
    }
    public double getpayPerHour() //returns this variables payPerHour variable
    {
        return this.payPerHour;
    }

    public void setOverTimePayPerHour() //calculates and sets overtime payPerHour rate
    {
            this.overTimePayPerHour = this.payPerHour * 1.5;
    }
    public double getOverTimePayPerHour() //returns overTimePayPerHour rate
    {
        return this.overTimePayPerHour;
    }

    public void setHours(int x) //sets hours worked into three of this objects variables, basePayHours, overTimeHours and totalHours
    {
        if(x > 40){
            this.basePayHours = 40;
        }
        else{
            this.basePayHours = x;
        }
        if (x > 40){
            this.overTimeHours = x - 40;
        }
        else{
            this.overTimeHours = 0;
        }

        this.totalHours = this.basePayHours + this.overTimeHours;

    }

    public double getBasePayHours()//returns this object's basePayHours value
    {
        return this.basePayHours;
    }
    public double getOverTimeHours()//returns this object's overTimeHours value
    {
        return this.overTimeHours;
    }
    public double getTotalHours()  //returns this object's totalHours value
    {
        return this.totalHours;
    }

    public void calculateTotalPay(){ //calculates this Employee objects total pay and sets it's totalPlay variable to that value
        double basePay = this.getBasePayHours() * this.getpayPerHour();  //calculates basePay
        double overTimePay = this.getOverTimeHours() * this.getOverTimePayPerHour(); //calculates overTimePay
        this.totalPay = basePay + overTimePay;//sets value of this object's totalPay to the sum of basePay and overTimePay
    }
    public double getTotalPay()
    {
        return this.totalPay; //returns this object's totalPay value
    }

}

public class FooCorp {
    public static void main(String[] args)
    {
        Employee employee1 = new Employee();
        employee1.setPayPerHour(8.20);
        employee1.setOverTimePayPerHour();
        employee1.setHours(47);
        employee1.calculateTotalPay();
        System.out.println("You worked  " + employee1.getBasePayHours() + " regular hours."); //should be 40
        System.out.println("Plus "+  employee1.getOverTimeHours() +" hours of overtime."); // should be 7.00
        System.out.println("For a total of " + employee1.getTotalHours() + " hours."); //should be 47
        System.out.println("You get paid $" + employee1.getpayPerHour() + " per hour."); //should be 8.20
        System.out.println("You get paid $" + employee1.getOverTimePayPerHour() + "  per hour for overtime."); // should be 12.3
        System.out.println("Your total pay is: $"+employee1.getTotalPay()); //should be 414.10
    }

}

I feel that this is not proper encapsulation because this seems like a lot of code to do so little? Any advice on something i could do to improve this program would be much appreciated!

Department of Redundancy Department

There's a lot of unnecessary variables here, so cleaning that up is going to be a good way to go. Your comments indicate this pretty clearly:

private double payPerHour; //regular hourly wage
private double overTimePayPerHour; //regular hourly wage * 1.5

If overtime pay is always regular pay times 1.5, why do we need it as a separate variable? Just drop the overtime setter and have the getter do the right thing:

public double getOverTimePayPerHour()
{
    return this.payPerHour * 1.5;
}

Similarly, basePayHours and overTimeHours can be derived from totalHours:

public double getBasePayHours()
{
    return Math.min(this.totalHours, 40);
}

public double getOvertimeHours()
{
    return Math.max(0, this.totalHours - 40);
}

totalPay can be calculated on the fly and doesn't need to be a member variable.

public double getTotalPay()
{
    return getBasePayHours() * this.payPerHour + getOvertimeHours() * this.payPerHour * 1.5;
}

Invalid states

In addition to removing redundancies, the above changes also get rid of another problem in your code - invalid state. getTotalPay() is only valid after you call calculateTotalPay(), which itself is only valid after you call setOverTimePayPerHour(). That's a lot that the user has to keep track of to use your code correctly, and is extremely error-prone.

Excessive commenting

Comments are for making otherwise difficult code clear. Have a complicated algorithm, or doing something that seems unobvious? Comment!

But when your code speaks for itself (which you should always strive for), the comments are, at best, annoying and could easily deteriorate the quality of the code over time if you update the code but forget to the update the comments.

In each of these examples, the comment is literally restating the code:

return this.totalPay; //returns this object's totalPay value
public double getTotalHours()  //returns this object's totalHours value
public double getOverTimePayPerHour() //returns overTimePayPerHour rate
// etc.

On the other hand, the following comment is incorrect:

public void setPayPerHour(double x) //takes parameter 'x' a d sets that equal to this objects payPerHour variable

It doesn't set the parameter, it sets the object's member. Although the comment itself is unnecessary. If setPayPerHour() doesn't set something related to the hourly pay, it's the function's name that's the problem, not the lack of comment.