Are there ways to speed up this string fuzzy matching in Golang? I have a piece of python code doing fuzzy matching which works very well and is pretty fast. For reference, it uses the following files:

https://raw.githubusercontent.com/datasets/s-and-p-500-companies/master/data/constituents.csv
https://raw.githubusercontent.com/nlintz/pyBros/master/sp500/sp500.csv

import csv
from fuzzywuzzy import process

def csv_reader(file, key):
    with open(file) as csvfile:
        reader = csv.DictReader(csvfile)
        return [row[key] for row in reader]

const_data = csv_reader("constituents.csv", "Name")
target_data = csv_reader("sp500.csv", "company")

with open('results_python.csv', 'w', newline='') as csvfile:
    fieldnames = ["name", "match_name", "match_score"]
    writer = csv.DictWriter(csvfile, fieldnames=fieldnames)
    writer.writeheader()
    for row in const_data:
        match = process.extractOne(row, target_data)
        writer.writerow({'name': row, 'match_name': match[0],    'match_score': match[1]})

This is how it performs:

$ time ./fuzzy.py
./fuzzy.py  19,90s user 0,13s system 97% cpu 20,444 total

However, my surprise came when I did a pretty similar code in Golang using a port of the fuzzywuzzy library. I want to understand if the slowness of it is related to the way I structure my code, or some deeper reason within the matching library.

package main

import (
    "encoding/csv"
    "os"
    "strconv"
    "github.com/paul-mannino/go-fuzzywuzzy"
)

func readCsv(file string) [][]string {
    csvFile, _ := os.Open(file)
    defer csvFile.Close()
    r := csv.NewReader(csvFile)
    records, _ := r.ReadAll()
    return records
}

func main() {

    original:= readCsv("constituents.csv")
    target:= readCsv("sp500.csv")
    csvOut, _ := os.Create("results_go.csv")
    csvwriter := csv.NewWriter(csvOut)

    var targetData []string

    for _, line := range target[1:] {
        targetData = append(targetData, line[1])
    }
    csvwriter.Write([]string{"name", "match_name", "match_score"})
    for _, line := range original[1:] {
        match, _ := fuzzy.ExtractOne(line[1], targetData)
        csvwriter.Write([]string{line[1], match.Match, strconv.Itoa(match.Score)})
    }
}

This is how the Go version performs:

$ time ./fuzzy
./fuzzy 75,04s user 1,28s system 98% cpu 1:17,51 total

Are there any possible bad steps that I took in my Go code that made this comparison slow? Are there any improvements you could suggest to make it perform at the very least to a similar speed as the python code?

Review I - package go-fuzzywuzzy

After a quick read of your Go code, the Go code for package go-fuzzywuzzy, and the Go documentation, it is reasonable to hope for at least a 60% to 95% improvement in Go performance.

For example,

$ go build fuzzy.go && time ./fuzzy
real    0m55.183s
user    0m58.858s
sys     0m0.944s
$

After moving one line in package go-fuzzywuzzy,

$ go build fuzzy.go && time ./fuzzy
real    0m6.321s
user    0m7.211s
sys     0m0.188s
$

The line to move:

r := regexp.MustCompile("[^\\p{L}\\p{N}]")

A diff of package go-fuzzywuzzy:

diff --git a/stringutility.go b/stringutility.go
index 935f6e1..95441cc 100644
--- a/stringutility.go
+++ b/stringutility.go
@@ -5,6 +5,8 @@ import (
        "strings"
 )

+var rxCleanse = regexp.MustCompile("[^\\p{L}\\p{N}]")
+
 func Cleanse(s string, forceAscii bool) string {
        s = strings.TrimSpace(s)
        s = strings.ToLower(s)
@@ -12,8 +14,7 @@ func Cleanse(s string, forceAscii bool) string {
                s = ASCIIOnly(s)
        }

-       r := regexp.MustCompile("[^\\p{L}\\p{N}]")
-       s = r.ReplaceAllString(s, " ")
+       s = rxCleanse.ReplaceAllString(s, " ")
        return s
 }

  Package regexp

 import "regexp"

  func MustCompile

func MustCompile(str string) *Regexp

  MustCompile is like Compile but panics if the expression cannot be
  parsed. It simplifies safe initialization of global variables holding
  compiled regular expressions.

The documentation clearly says global variables.

If I spent more time reading the code, I would expect further performance improvements.

After more reading, package go-fuzzywuzzy is written in "PyGo." Writing the package in Go yields efficiencies.

For example, completely rewriting the go-fuzzywuzzy file stringutility.go in Go.

Before:

$ go build fuzzy.go && time ./fuzzy
real    0m55.183s
user    0m58.858s
sys     0m0.944s
$

After:

$ go build fuzzy.go && time ./fuzzy
real    0m5.735s
user    0m6.601s
sys     0m0.193s
$

stringutility.go:

package fuzzy

import (
    "strings"
    "unicode"
)

func Cleanse(s string, forceAscii bool) string {
    if forceAscii {
        s = ASCIIOnly(s)
    }
    s = strings.TrimSpace(s)
    rs := make([]rune, 0, len(s))
    for _, r := range s {
        if !unicode.IsLetter(r) && !unicode.IsNumber(r) {
            r = ' '
        }
        rs = append(rs, r)
    }
    return strings.ToLower(string(rs))
}

func ASCIIOnly(s string) string {
    b := make([]byte, 0, len(s))
    for _, r := range s {
        if r <= unicode.MaxASCII {
            b = append(b, byte(r))
        }
    }
    return string(b)
}

UPDATE:

The package go-fuzzywuzzy author adopted the suggested stringutility.go changes:

[Fixes #4] Stop reinitializing costly regex in stringutility
https://github.com/paul-mannino/go-fuzzywuzzy/commit/f14294bf5858c8a7fa51b026a9ee9a2802c816bf

Was using regexp.MustCompile within a frequently invoked
method Cleanse. Since go does not cache these calls, it was
incurring a costly regex compilation each time Cleanse was
called. The other changes made in the stackoverflow post that
caught this issue seem to further improve performance by
about 10% on top of the massive gains from fixing this issue,
so incorporating those as well.


Java: Find the smallest integer not in the list - Memory Efficiency This is a fairly popular interview question that I conceptually understand but have never really attempted to implement. In other similar implementations of this in Java I have seen they typically operate on an array of boolean values which kind of irks me.  It turns out that the way the JVM works however is that it will allocate 8 bits for a single boolean primitive.  Granted I believe that an array of booleans actually does allocate a single bit for each value but just as a personal challenge I decided to use an array of byte and through bitwise operations utilizing each bit for a boolean value of N.

This means that the byte array will really be N / 8 or a single bit representing each possible random number that COULD occur in a list of numbers that just happens to be of N length as well.

The strategy is to construct my bytes by scanning each randomly generated number, determining which byte and bit that number correlates to in my byte array, and flipping the appropriate bit when that number is encountered.  When the scan is finished I simply inspect each bit and the first 0 bit I encounter means that bit location in my array represents the smallest integer that is not in the list of randomly generated numbers.

It should consume roughly 1/8th of memory from if I were to use a List and is 1/4 faster than sorting the list of random numbers.

import junit.framework.TestCase;

public class TestSmallestNumberNotInList extends TestCase {

    static final int N = 30000000;
    static int[] numbers = null;
    static {
        numbers = new int[N];
        for (int i = 0; i < numbers.length; i++)
            numbers[i] = Math.abs((int)(Math.round(Math.random() * N)));
    }

    public void testFindSmallestNumberNotInList() throws Exception {

        long startTime = System.currentTimeMillis();

        int byteArrayLength = (int) Math.ceil(N / 8);
        byte[] bitset = new byte[byteArrayLength];

        for (int number : numbers) {
            int byteIndex = (int)Math.floor(number / 8);
            byte b = bitset[byteIndex];
            int bitMod = (int)(number % 8);

            // The bit value will be 1 if we encountered this number before
            int bitVal = b>>>(32-bitMod) & 0x00000001;
            if (bitVal == 1)
                continue;

            //Get a byte mask to OR against the existing byte, flipping just the one bit
            //we are concerned with.
            byte byteMask = (byte)(1<<bitMod);
            b = (byte)(b | byteMask);

            //assign the new byte value for the byte array
            bitset[byteIndex] = b;
        }

        //Scan the byte array for the first 0 bit, this ((index * 8) + bitmodulus) is the smallest
        //integer not in the list.
        int location = 0;
        for (int i = 0; i < bitset.length; i++) {
            byte b = bitset[i];
            if (b == 0xFF) {
                location += 8;
                continue;
            } else {
                for (int j=8; j>=1; j--) {
                    if ((b>>>(j) & 0x00000001) == 1)
                        location++;
                }
                break;
            }
        }

        long duration = System.currentTimeMillis() - startTime;

        System.out.println("Total Memory: " + Runtime.getRuntime().totalMemory());
        System.out.println("Free Memory: " + Runtime.getRuntime().freeMemory());
        System.out.println("Smallest number not in the list is " + location);
        System.out.println("Time to execute in milliseconds: " + duration);
    }

    private static String getBitString(int x) {
        StringBuffer buf = new StringBuffer();
        for (int i=1; i<=32; i++) buf.append(x>>>(32-i) & 0x00000001);
        return buf.toString();
    }
}

I believe the output is correct although I really haven't unit tested the results.  It certainly seems correct.  I was wondering if I am making any glaring mistakes in my arithmetic or code or if you have any other possibilities or suggestions for improvement?

So just a few things that I am seeing here.  You are using Math.floor, ceil on integers which is totally unnecessary, integers by definition don't have a decimal portion.  You also don't need to cast an int as an int.

To make this run faster and take up less stack space, in the for loop where you set the bits you don't need to check anything, just do the bit twiddling you want.

int bitMod = 0x1 << (number % 8);
byte b = number / 8;
bitset[b] |= bitMod;

Getting the position of the bit you want to change and then just or ing it won't change any other bits.

Finally, I don't know why you did it the way you did, but your final for loop doesn't work correctly.  It starts with the Most Significant Bit and moves down, instead of the least moving up.  Here is a version that I have tested and found to work.

for (int j=0; j<8; j--) {
    if ((b & (1 << j)) != 0){
        location++;
    } else {
        break;
    }
}

Finally, just to see what kind of memory this might be using, I called the free memory before and after to compare the difference.  It looks like this program uses 56 MB, and takes under 100 ms after my few changes.

actual lowest is 1
(Initial Implementation)
Free Memory: 563760
Smallest number not in the list is 2
Time to execute in milliseconds: 156
2 is in the list true

(Optimized and cleaned up)
Free Memory: 563760
Smallest number not in the list is 1
Time to execute in milliseconds: 93
1 is in the list false

Here is the class I used to test.

public class TestSmallestNumberNotInList {

static final int N = 3000000;
static int[] numbers = null;
static {
    numbers = new int[N];
    for (int i = 0; i < numbers.length; i++) {
        numbers[i] = Math.abs((int)(Math.round(Math.random() * N)));
    }
}

public void testFindSmallestNumberNotInList() {

    Runtime.getRuntime().gc();
    Runtime.getRuntime().gc();
    Runtime.getRuntime().gc();
    long total = Runtime.getRuntime().totalMemory();
    long free = Runtime.getRuntime().freeMemory();
    long startTime = System.currentTimeMillis();

    int byteArrayLength = (int) Math.ceil(N / 8);
    byte[] bitset = new byte[byteArrayLength];

    for (int number : numbers) {
        int byteIndex = (int)Math.floor(number / 8);
        byte b = bitset[byteIndex];
        int bitMod = (int)(number % 8);

        // The bit value will be 1 if we encountered this number before
        int bitVal = b>>>(32-bitMod) & 0x00000001;
        if (bitVal == 1)
            continue;

        //Get a byte mask to OR against the existing byte, flipping just the one bit
        //we are concerned with.
        byte byteMask = (byte)(1<<bitMod);
        b = (byte)(b | byteMask);

        //assign the new byte value for the byte array
        bitset[byteIndex] = b;
    }

    //Scan the byte array for the first 0 bit, this ((index * 8) + bitmodulus) is the smallest
    //integer not in the list.
    int location = 0;
    for (int i = 0; i < bitset.length; i++) {
        byte b = bitset[i];
        if (b == 0xFF) {
            location += 8;
            continue;
        } else {
            for (int j=8; j>=1; j--) {
                if ((b>>>(j) & 0x00000001) == 1)
                    location++;
            }
            break;
        }
    }

    long duration = System.currentTimeMillis() - startTime;

    System.out.println("Total Memory: " + (total - Runtime.getRuntime().totalMemory()));
    System.out.println("Free Memory: " + (free - Runtime.getRuntime().freeMemory()));
    System.out.println("Smallest number not in the list is " + location);
    System.out.println("Time to execute in milliseconds: " + duration);
    boolean isInList = false;
    for (int next : numbers){
        if (location == next){
            isInList = true;
            break;
        }
    }
    System.out.println(location + " is in the list " + isInList);
}

public void testMine() {
    Runtime.getRuntime().gc();
    Runtime.getRuntime().gc();
    Runtime.getRuntime().gc();
    long total = Runtime.getRuntime().totalMemory();
    long free = Runtime.getRuntime().freeMemory();
    long startTime = System.currentTimeMillis();

    int byteArrayLength = numbers.length / 8;
    byte[] bitset = new byte[byteArrayLength];

    for (int number : numbers) {
        int byteIndex = number / 8;
        int bitMod = (1 << (number % 8)) & 0xFF;
        bitset[byteIndex] |= bitMod;
    }

    //Scan the byte array for the first 0 bit, this ((index * 8) + bitmodulus) is the smallest
    //integer not in the list.
    int location = 0;
    for (int i = 0; i < bitset.length; i++) {
        byte b = bitset[i];
        if (b == 0xFF) {
            location += 8;
            continue;
        } else {
            for (int j=0; j<8; j++) {
                if ((b & (1 << j)) != 0){
                    location++;
                } else {
                    break;
                }
            }
            break;
        }
    }

    long duration = System.currentTimeMillis() - startTime;

    System.out.println("Total Memory: " + (total - Runtime.getRuntime().totalMemory()));
    System.out.println("Free Memory: " + (free - Runtime.getRuntime().freeMemory()));
    System.out.println("Smallest number not in the list is " + location);
    System.out.println("Time to execute in milliseconds: " + duration);

    boolean isInList = false;
    for (int next : numbers){
        if (location == next){
            isInList = true;
            break;
        }
    }
    System.out.println(location + " is in the list " + isInList);
}

public void getSmallestNotInList() {
     int lowest = 0;
     boolean notLowest = true;
     while (notLowest){
         int last = lowest;
         for (int i=0; i<numbers.length; i++){
             if (numbers[i] == lowest){
                 lowest++;
                 break;
             }
         }
         if (last == lowest){
             break;
         }
     }
     System.out.println("actual lowest is " + lowest);
}
}


Find all files in directory and subdirectories while ignoring noncritical exceptions I need to get a list of all files supported by some libary (images) in a specified directory and all subderictories.
Root directory of drive might be chosen and in this case I want to skip system folders like "Windows" and "AppData" (some other?).

For such task I can use Directory.GetFiles() but problem is - it will stop after getting any exception. In this case I need to just ignore noncritical exceptions and skip folders that cause this exception.

This code looks reasonable to me. But I want to make sure that it is done right.

Please tell me if it should be changed somehow.

public static List<string> FindSupportedFiles(string root)
{
    // class from library
    var optimizer = new ImageOptimizer();

    Queue<string> folders = new Queue<string>();
    List<string> files = new List<string>();

    folders.Enqueue(root);
    while (folders.Count != 0)
    {
        string currentFolder = folders.Dequeue();

        if (currentFolder.Contains(":\\Windows") ||
             (currentFolder.Contains(":\\Users") && currentFolder.Contains("\\AppData")))
        {
            if (folders.Count != 0) currentFolder = folders.Dequeue();
            continue;
        }

        string[] filesInCurrent = null;
        try
        {
            filesInCurrent = Directory.GetFiles(currentFolder, "*.*");
        }
        // Ignore this exceptions
        catch (UnauthorizedAccessException) { }
        catch (PathTooLongException) { }
        catch (DirectoryNotFoundException) { }

        foreach (string f in filesInCurrent)
            if (optimizer.IsSupported(f)) files.Add(f);

        string[] foldersInCurrent = null;
        try
        {
            foldersInCurrent = Directory.GetDirectories(currentFolder);
        }
        // Ignore this exceptions
        catch (UnauthorizedAccessException) { }
        catch (PathTooLongException) { }
        catch (DirectoryNotFoundException) { }

        foreach (string _current in foldersInCurrent)
            folders.Enqueue(_current);
    }
    return files;
}

You have multiple problems here:

For the case that an exception is thrown and swallowed you get a NullReferenceException at iterating over the filesInCurrent.
If the method would be called using C:/Windows/ (which is a valid path although it uses a slash instead of a backslash) the if condition would never result in true.
The if (folders.Count != 0) currentFolder = folders.Dequeue(); in addition with the continue can lead to problems. Assume the next folder in the queue would be a valid folder like C:\XFiles by calling Dequeue() and continue the folder would silently be swallowed.

Fixing the NullReferenceException is the easiest task. Just put the iterating over the filesInCurrent and foldersInCurrent inside the try block. By doing this you could place the declaration into the try as well.
Instead of checking if the currentFolder.Contains() a not wanted path, you should just refuse to add the paths while iterating over foldersInCurrent.
Instead of having the optimizer.IsSupported(f) which had former been a SomeLib.IsSupported() you should pass a Action<string, bool> which will just handle this. This will make your method future proof if e.g. the decision will be made to use a different lib.
To overcome the / vs \ problem you should create a DirectoryInfo() object using the root as parameter. By enqueueing the FullName property of the DirectoryInfo object any found subdirectory will have the \ as path separator.
The code should take advantage of the List<T>.AddRange() method instead of iterating over the files and adding them one by one.
If you later decide to exclude different paths it would be good to extract checking of the validness of the folders to a separate method which could be passed as Action<string, bool> parameter as well.
Parameters of public methods should always be ckecked for validness, hence a check for null and for emptiness of the root should be implemented.
One could use the var type if the right hand side of the assignment makes the type obvious.
Ommiting braces {} although tehy might be optional can lead to hidden and therfor hard to find bugs. I would like to encourage you to always use them.
Underscore prefixed variablenames are usually only for class-level variables.
Passing a root which doesn't exists should be checked at the beginning of the method as well. By returning early you won't need to catch a DirectoryNotFoundException.

Implementing the mentioned points will lead to

public static List<string> FindSupportedFiles(string root, Func<string, bool> isFileValid,Func<string,bool> isFolderValid)
{
    if (root == null) { throw new ArgumentNullException("root"); }
    if (string.IsNullOrWhiteSpace(root)) { throw new ArgumentException("The passed value may not be empty or whithespace", "root"); }

    var files = new List<string>();

    var rootDirectory = new DirectoryInfo(root);
    if (rootDirectory.Exists() == false) { return files; }

    //overcome problem about slash and backslash with using Contains()
    root = rootDirectory.FullName;
    if (isFolderValid(root) == false) { return files; }

    var folders = new Queue<string>();
    folders.Enqueue(root);
    while (folders.Count != 0)
    {
        string currentFolder = folders.Dequeue();

        try
        {
            var currentFiles = Directory.EnumerateFiles(currentFolder, "*.*").Where(f => isFileValid(f));
            files.AddRange(currentFiles);
        }
        // Ignore this exceptions
        catch (UnauthorizedAccessException) { }
        catch (PathTooLongException) { }

        try
        {
            var currentSubFolders = Directory.GetDirectories(currentFolder).Where(f => isFolderValid(f));
            foreach (string current in currentSubFolders)
            {
                folders.Enqueue(current);
            }
        }
        // Ignore this exceptions
        catch (UnauthorizedAccessException) { }
        catch (PathTooLongException) { }

    }
    return files;
}

private static bool IsFolderValid(string path)
{

    return (path.Contains(":\\Windows") ||
             (path.Contains(":\\Users") &&
              path.Contains("\\AppData"))) == false;
}

and could be called like so

var optimizer = new ImageOptimizer();
var supportedFiles = FindSupportedFiles(path, optimizer.IsSupported, IsFolderValid);


Interface and implementation of this replacement for Rebol/Red REJOIN This is a draft implementation of a function called COMBINE.  It's not intended to be a complex formatting dialect, just a replacement for REJOIN that has a better name and more useful behavior.  Its purpose is laid out pretty thoroughly in this blog entry:

http://blog.hostilefork.com/combine-alternative-rebol-red-rejoin/

But to make a long story short, it is supposed to let you flatten expressions into a string.  If an expression returns a block, then that block will be recursively evaluated using the same rules as if it had been handed to COMBINE directly:

combine: function [
    {Combine evaluated expressions and substitute block variables to produce a string}
    block [block!] "Specification to process"
    /with "Add delimiter between values (will be combined if a block)"
        delimiter [block! any-string! char!]
    /into "Insert output into an existing string instead of making a new one"
        out [any-string!]
    /local
        value
] [
    ;-- No good heuristic for string size yet
    unless into [
        out: make string! 10
    ]

    if block? delimiter [
        delimiter: combine delimiter
    ]

    needs-delimiter: false
    pre-delimit: does [
        either needs-delimiter [
            out: append out delimiter
        ] [
            needs-delimiter: true? with
        ]
    ]

    ;-- Do evaluation of the block until a non-none evaluation result
    ;-- is found... or the end of the input is reached.
    while [not tail? block] [
        set/any 'value do/next block 'block

        ;-- Blocks are substituted in evaluation, like the recursive nature
        ;-- of parse rules.

        case [
            unset? :value [
                ;-- Ignore unset? (precedent: any, all, compose)
            ]

            none? :value [
                ;-- Skip all nones
            ]

            any [
                function? :value
                closure? :value
            ] [
                throw make error! "Evaluation in COMBINE gave function/closure"
            ]

            block? value [
                pre-delimit
                out: combine/into value out
            ]

            any-block? value [
                ;-- all other block types as *results* of evaluations throw
                ;-- errors for the moment.  (It's legal to use PAREN! in the
                ;-- COMBINE, but a function invocation that returns a PAREN!
                ;-- will not recursively iterate the way BLOCK! does)
                throw make error! "Evaluation in COMBINE gave non-block! block"
            ]

            any-word? value [
                ;-- currently we throw errors on words if that's what an
                ;-- evaluation produces.  Theoretically these could be
                ;-- given behaviors in the dialect, but the potential for
                ;-- bugs probably outweighs the value (of converting implicitly
                ;-- to a string or trying to run an evaluation of a non-block)
                throw make error! "Evaluation in COMBINE gave symbolic word"
            ]

            true [
                pre-delimit
                out: append out (form :value)
            ]
        ]
    ]
    either into [out] [head out]
]

Pretty much anything here is up for review; both the interface and the implementation.  A few things to wonder about:

What would be a good heuristic for the output size?
Right now combine [if (1 > 2) {something}] returns an empty string.  Initially I thought that if it only received NONE, then it should return NONE.  But the existence of the /INTO refinement makes that not possible to distinguish, as it's supposed to return the insertion position.  Is this distinction more important than offering /INTO to the caller (rejoin didn't...).
How should other string types be returned?  Currently the only way to get a non-string is to use /into, in order to avoid the sensitivity of the type of the first element that REJOIN has.  But should there be an /AS refinement for passing the target type?

Some thoughts:

Initialize String

unless into [
    out: make string! 10
]

Don't know how else to discern the initial string allocation, perhaps just start with zero?

OUT might be a strange word for a value that is both input and output. I'd also allow a NONE! value here.

/input out [any-string! none!]

Then you can initialize like:

out: any [out make string! 0]

Delimiter

if block? delimiter [
    delimiter: combine delimiter
]

Cool, a delimiter can be combined on the fly!

needs-delimiter: false
pre-delimit: does [
    either needs-delimiter [
        out: append out delimiter
    ] [
        needs-delimiter: true? with
    ]
]

Rather than an extra function and an extra value for this, why not just an extra value:

active-delimiter: none

Then set active-delimiter to delimiter after the first step through BLOCK.

Iteration

while [not tail? block] [
    set/any 'value do/next block 'block
    ...
]

I'm not certain about this, but I believe FOREACH is faster than WHILE. This seems like an inefficient way of expressing:

foreach value reduce block [
    ...
]

Functions

function? :value
closure? :value

Are you just testing for these two types of function? Otherwise ANY-FUNCTION? would catch both.

Throwing Errors

throw make error! "Evaluation in COMBINE gave function/closure"

I don't think this works in Rebol 3, at least not without wrapping the function call in a CATCH block. As I understand it, you need to DO an error to trigger it.

Coerce Output Type

An /AS refinement might be a good way to do this, so long as the type is with the ANY-STRING! typeset. It'd be a close call as to whether /INTO or /AS would take precedence for output type:

type: any [type string!]
out: any [out make type 0]

None/Empty

My opinion here is:

combine []
combine [none]

Should return ""

combine/with [none none] ","

Should return ","


Displaying each number of an integer in a sequence One of the books I'm currently reading to learn C# asked me to write a method that takes an integer between 1 and 99999 and displays each number in that integer in a sequence, separating each digit by 2 spaces.

For example: the int 87564 would be written to the console as 8 7 5 6 4.

This is simple enough to do using the input as a string, converting it to a char array, or looping over the string with a foreach and printing out a formatted string for each character.

For fun though and mostly to challenge myself, I like to work out the problems as they are intended for someone just learning the concepts for the first time. The chapter was about methods and briefly introduced recursion. It's clearly the author's intent that you solve this using division and modulus operations to pick off each digit and then write them out.

So there really were limited options in terms of solving this with the material you have learned to this point in the book. You could pick off each digit and store it as it's own variable, then later write them out in order since you know the range of integers.

I decided to make the method more useful by really allowing any non-negative integer and my approach involved recursion. I'm not really experienced using recursion so I'd like to get some feedback on my implementation to see what I could have done better.

public class Program
{
    static void Main()
    {
        // Get User Input From Console
        // Validate and parse as int input
        DisplayDigits(input);
    }

    static void DisplayDigits(int value)
    {
        if (value < 10)
        {
            Console.Write("{0}  ", value);
            return;
        }
        DisplayDigits(value / 10);
        Console.Write("{0}  ", value % 10);
    }
}

Obviously, this is not meant to be production level code, I'm solving the problem in the textbook and then throwing the code away.
It appears to be working for all non-negative numbers I've tried. I even wrote an overload that takes a ulong and passed it UInt64.MaxValue and it printed everything fine. I can't help but feel like maybe it could be better in some way. Any criticism, suggestions, links to more reading material would be appreciated.

Your method is lying. Not only it's displaying the digits, performing every Console.Write operation that needs to happen, it's also performing the "digit-splitting" logic.

It's more work than what its name says. For a simple coding exercise it's without consequences, but in larger projects if this is a coding habit you have, it can mean much bigger problems.

Separation of Concerns

Let's see what needs to happen here.

Get a valid user input.
Determine what the digits are.
Come up with a string to output.

You haven't included how you're addressing the first concern, so I won't cover it here.

Ideally, the result of determining what the digits are should be just that: some IEnumerable<int> that contains all the digits you want to display. I would make it so that the recursive function that contains the logic for that, works with an IList<int> and adds each digit it finds (instead of outputting it directly to the console).

Then, when your recursive logic completes, you have an IEnumerable<int> to iterate through. Right? Maybe not. string has methods, such as Join() that can make your "digit separator" string very explicit - if you're using .net 4+ you can use the Join<T>(string, IEnumerable<T>) overload:

var input = GetValidUserInput();
if (string.IsNullOrEmpty(input))
{
    return;
}

var digits = SeparateTheDigits(input);

var separator = "  ";
var result = string.Join(separator, digits);

Console.WriteLine(result);

As a result of separating the concerns, you now only write to the console once... and you haven't hard-coded your digit separator into a format string.


Get locations that are x km away using Latitude/Longitude So I have a solution for this which uses a for loop and calculates distance between the main point and other points. I found the code for distance between two points online, changed it to my needs and wrote the rest of the code. While this code works pretty well, I'm just curious if there's a better and more efficient way of doing this. This fits really well with my current code as it's just a small scale project. But I always like looking for stuff I can optimise and improve.
function findDistanceInKm(location1, location2) {
    const lat1 = location1.lat;
    const lon1 = location1.lon;
    const lat2 = location2.lat;
    const lon2 = location2.lon;
    const earthRadius = 6371;
    const dLat = convertDegToRad(lat2 - lat1);
    const dLon = convertDegToRad(lon2 - lon1);
    const squarehalfChordLength =
      Math.sin(dLat / 2) * Math.sin(dLat / 2) +
      Math.cos(convertDegToRad(lat1)) * Math.cos(convertDegToRad(lat2)) *
      Math.sin(dLon / 2) * Math.sin(dLon / 2);

    const angularDistance = 2 * Math.atan2(Math.sqrt(squarehalfChordLength), Math.sqrt(1 - squarehalfChordLength));
    const distance = earthRadius * angularDistance;
    return distance;
}

function convertDegToRad(deg) {
    return deg * (Math.PI/180)
}

let nearLocations = [];
let newLocation = {
    lat: 65.33714,
    lon: -82.11508
}

let locationList = [
    {
        lat: 65.33714,
        lon: -82.11503
    },
    {
        lat: -1.38370,
        lon: 10.32156
    },
    {
        lat: -69.98601,
        lon: 18.80178
    },
    {
        lat: -29.37968,
        lon: -156.32350
    },
]

for(let i =0; i < locationList.length; i++){
    let distance = findDistanceInKm(newLocation, locationList[i]);
    console.log(distance)
    if(distance < 1){
        nearLocations.push(locationList[i])
    }
}

There are a number of performance optimization tricks that you can take advantage of when trying to find efficiency. One mechanism commonly used is to reduce complexity of the code by algorithmic change..... but in your case, you can't really improve the "Big O" complexity that way, you are running at O(n) already, and there's nothing you can do to improve that.
So, it then comes down to nickel-and-diming the current algorithm to shave time off of the work you're already doing. This requires looking at the expensive computations, and reducing the number of operations to improve them.
In your example, you're converting degrees to radians often. In the case of newLocation you're doing it repeatedly for each comparison. It would make sense for you to convert all locations to radian form, and then not have any conversions inside the computation loop. This will be especially useful if your real-world code has to do multiple comparisons on the locations.
So, simply doing something like (include the radian values as part of the location):
func locationToRadians(degrees) {
  return {
    lat: degrees.lat,
    lon: degrees.lon,
    rlat: convertDegToRad(degrees.lat),
    rlon: convertDegToRad(degrees.long)
  }
}

const locationListRadians = locationList.map(locationToRadians);

Do the same with the fromLocation.
Now, in your distance function, you're repeating complex computations (trig functions are non-trivial at a computational level, so let's avoid repeats - I realize that premature optimization is not always great, but this also improves the readability). Your distance computation is significantly simpler:
function findDistanceInKm(location1, location2) {
    const sinLat = Math.sin((location2.rlat - location1.rlat) / 2);
    const sinLon = Math.sin((location2.rlon - location1.rlon) / 2);
    const earthRadius = 6371;
    const squarehalfChordLength =
      sinLat * sinLat +
      sinLon * sinLon *
      Math.cos(location1.rlat) * Math.cos(location2.rlat);

    const angularDistance = 2 * Math.atan2(Math.sqrt(squarehalfChordLength), Math.sqrt(1 - squarehalfChordLength));
    const distance = earthRadius * angularDistance;
    return distance;
}

Now, you can add in the distances that way.
The savings from doing the above will only be noticeable if you reuse computations often, but in a busy system where lots of locations are being repeatedly compared to each other I can easily see a 50% CPU saving.
As a final comment, the last loop does not need the indexing mechanism, a filter is better:
const neadLocations = locationList.filter(location => {
    const distance = findDistanceInKm(newLocation, location);
    console.log(distance);
    return distance < 1
});

(it would be better without the console logging in there...)


you should move the states list outside of view, i.e. to model class.

class User < ActiveRecord::Base
  STATES = ['AL', 'AK', 'ID']
end

then in view:

<%= f.label(:state, "State:*") %>

<%= f.select(:state, User::STATES) %>

This will allow you to reuse states collection anywhere across the application and add easily validate if user hasn't modified the form manually.

Keeping states in User model is not necessary the best option but it's outside of this question scope


JavaScript string manipulation with certain logic in it How would I make this code look less ugly?

// we are looping over an array of images
// width is a parameter passed to the current function
//
// the resulting string looks like this: "30px 0"
// (it's a tool that returns coordinates for a CSS sprite)
// when iterating over the first item, we don't need
// to add "-" and "px" to X coordinate
result.push(
    ( i === 0 ? "0" : "-" + i * ( ( is2PixelRatio ? width / 2 : width ) + 10 ) + "px" ) + " 0"
);

Minor upgrades, nice idea for the prepop result Raynos

result = ["0 0"]; // Outside of loop

// ...

if (is2PixelRatio) width /= 2;
width += 10; // [Insert magik number explanation here]

result.push(  -(i * width) + "px 0"  ); // Negative numbers will convert to "-30"


Creating a device configuration During the test process of a device, there is a procedure that sets a device in a state in which it is ready to be tested (typically by writing default measure parameters, 3G connection parameters and any other stuff that is requested to be configurable by the devices R&D team).

The code below works but I don't like how it looks and I'd like to tidy it up.

public void TestSetupDefault()
{
    nrg.WriteFlagTest();

    NRG96.SetupConfiguration setupConfiguration = new NRG96.SetupConfiguration((int)Configuracion.GetDouble("PRIMARY_VOLTAGE", 1, ParamUnidad.SinUnidad),
        (ushort)Configuracion.GetDouble("SECONDARY_VOLTAGE", 1, ParamUnidad.SinUnidad),
        (ushort)Configuracion.GetDouble("PRIMARY_CURRENT", 1, ParamUnidad.SinUnidad),
        Configuracion.GetString("VOLTAGE_TYPES", "SIMPLES", ParamUnidad.SinUnidad),
        (NRG96.SetupConfiguration.MainDefaultScreens)Configuracion.GetDouble("MAIN_DEFAULT_SCREEN", 1, ParamUnidad.SinUnidad),
        (NRG96.SetupConfiguration.EnergyDefaultScreens)Configuracion.GetDouble("ENERGY_DEFAULT_SCREEN", 1, ParamUnidad.SinUnidad),
        Configuracion.GetString("HARMONIC_CALCULUS", "THD", ParamUnidad.SinUnidad).ToUpper() == "THD",
        (byte)Configuracion.GetDouble("BACKLIGHT_SHUTDOWN_TIME", 1, ParamUnidad.SinUnidad));

    nrg.WriteSetupConfiguration(setupConfiguration);

    NRG96.MaximumDemandConfiguration maximumDemandConfiguration = new NRG96.MaximumDemandConfiguration()
    {
        VariabletoCalculate = Configuracion.GetString("MAXIMUM_DEMAND_VARIABLE", "NO_PD", ParamUnidad.SinUnidad),
        RegisterTime = (ushort)Configuracion.GetDouble("MAXIMUM_DEMAND_REGISTER_TIME", 15)
    };

    nrg.WriteMaximumDemandConfiguration(maximumDemandConfiguration);

    NRG96.TriCalibrationFactors compoundFactors = new NRG96.TriCalibrationFactors((ushort)Configuracion.GetDouble("COMPOUND_VOLTAGE_CALIBRATION_FACTOR", 8790, ParamUnidad.SinUnidad));

    nrg.WriteCompoundvoltageFactors(compoundFactors);

}

By the way I have created the objects in 2 ways, using a constructor first, and setting properties (object initializer). Which one is more correct?

To conclude, the question I am asking is:

  How can I tidy up this code to make it more readable?

I feel like developing separate functions for each structure is like hiding this code with regions and I don't quite like that.

Keep in mind this is not about performance but about readability, I feel like there is no more code needed since what the struct does with the data or how I send it is not what I am asking to be reviewed, just how to properly display the code since that is a field I have much to work on.

There are several possible steps you can take to increase readability to your code, depending on how much control of the code you have and how much implementation details you which to hide.

Separate default values from database loading logic

By initializing setupConfiguration with default values by named properties and moving the database loading logic into a LoadFromDatabase() method you increase readability of the defaults:

var setupConfiguration = Configuracion.LoadFromDatabase(new NRG96.SetupConfiguration
{
    PRIMARY_VOLTAGE = 1,
    SECONDARY_VOLTAGE = 1,
    PRIMARY_CURRENT = 1,
    VOLTAGE_TYPES = "SIMPLES",
    MAIN_DEFAULT_SCREEN = SetupConfiguration.MainDefaultScreens.One,
    ENERGY_DEFAULT_SCREEN = SetupConfiguration.EnergyDefaultScreens.One,
    HARMONIC_CALCULUS = "THD",
    BACKLIGHT_SHUTDOWN_TIME = 1
});

I'm assuming SetupConfiguration can be defined by something like:

public class SetupConfiguration
{
    public int PRIMARY_VOLTAGE { get; set; }
    public ushort SECONDARY_VOLTAGE { get; set; }
    public ushort PRIMARY_CURRENT { get; set; }
    public string VOLTAGE_TYPES { get; set; }
    public MainDefaultScreens MAIN_DEFAULT_SCREEN { get; set; }
    public EnergyDefaultScreens ENERGY_DEFAULT_SCREEN { get; set; }

    public string HARMONIC_CALCULUS { get; set; }
    public byte BACKLIGHT_SHUTDOWN_TIME { get; set; }

    // Doesn't get saved to database because no public setter
    public bool HARMONIC_CALCULUS_IS_THD { get { return (HARMONIC_CALCULUS ?? "").ToUpper() == "THD"; } }

    public enum MainDefaultScreens
    {
        One = 1,
        Two
    }
    public enum EnergyDefaultScreens
    {
        One = 1,
        Second
    }
}

LoadFromDatabase() could then be implemented by:

public static SetupConfiguration LoadFromDatabase(SetupConfiguration defaultValues)
{
    return new SetupConfiguration
    {
        PRIMARY_VOLTAGE = (int)GetDouble("PRIMARY_VOLTAGE", defaultValues.PRIMARY_VOLTAGE, ParamUnidad.SinUnidad),
        SECONDARY_VOLTAGE = (ushort)GetDouble("SECONDARY_VOLTAGE", defaultValues.SECONDARY_VOLTAGE, ParamUnidad.SinUnidad),
        PRIMARY_CURRENT = (ushort)GetDouble("PRIMARY_CURRENT", defaultValues.PRIMARY_CURRENT, ParamUnidad.SinUnidad),
        VOLTAGE_TYPES = GetString("VOLTAGE_TYPES", defaultValues.VOLTAGE_TYPES, ParamUnidad.SinUnidad),
        MAIN_DEFAULT_SCREEN = (SetupConfiguration.MainDefaultScreens)GetDouble("MAIN_DEFAULT_SCREEN", (int)defaultValues.MAIN_DEFAULT_SCREEN, ParamUnidad.SinUnidad),
        ENERGY_DEFAULT_SCREEN = (SetupConfiguration.EnergyDefaultScreens)GetDouble("ENERGY_DEFAULT_SCREEN ", (int)defaultValues.ENERGY_DEFAULT_SCREEN, ParamUnidad.SinUnidad),
        HARMONIC_CALCULUS = GetString("HARMONIC_CALCULUS", defaultValues.HARMONIC_CALCULUS, ParamUnidad.SinUnidad),
        BACKLIGHT_SHUTDOWN_TIME = (byte)GetDouble("BACKLIGHT_SHUTDOWN_TIME", defaultValues.BACKLIGHT_SHUTDOWN_TIME, ParamUnidad.SinUnidad)
    };
}

Cleaning up database loading logic with helper methods

LoadFromDatabase() could be furter cleaned up by creating a couple of helper methods to do the actual loading from database:

static int Get(string name, int defaultValue) { return (int)GetDouble(name, defaultValue, ParamUnidad.SinUnidad); }
static byte Get(string name, byte defaultValue) { return (byte)GetDouble(name, defaultValue, ParamUnidad.SinUnidad); }
static ushort Get(string name, ushort defaultValue) { return (ushort)GetDouble(name, defaultValue, ParamUnidad.SinUnidad); }
static string Get(string name, string defaultValue) { return GetString(name, defaultValue, ParamUnidad.SinUnidad); }

public static SetupConfiguration LoadFromDatabase(SetupConfiguration defaultValues)
{
    return new SetupConfiguration
    {
        PRIMARY_VOLTAGE = Get("PRIMARY_VOLTAGE", defaultValues.PRIMARY_VOLTAGE),
        SECONDARY_VOLTAGE = Get("SECONDARY_VOLTAGE", defaultValues.SECONDARY_VOLTAGE),
        PRIMARY_CURRENT = Get("PRIMARY_CURRENT", defaultValues.PRIMARY_CURRENT),
        VOLTAGE_TYPES = Get("VOLTAGE_TYPES", defaultValues.VOLTAGE_TYPES),
        MAIN_DEFAULT_SCREEN = (SetupConfiguration.MainDefaultScreens)Get("MAIN_DEFAULT_SCREEN", (int)defaultValues.MAIN_DEFAULT_SCREEN),
        ENERGY_DEFAULT_SCREEN = (SetupConfiguration.EnergyDefaultScreens)Get("ENERGY_DEFAULT_SCREEN ", (int)defaultValues.ENERGY_DEFAULT_SCREEN),
        HARMONIC_CALCULUS = Get("HARMONIC_CALCULUS", defaultValues.HARMONIC_CALCULUS),
        BACKLIGHT_SHUTDOWN_TIME = Get("BACKLIGHT_SHUTDOWN_TIME", defaultValues.BACKLIGHT_SHUTDOWN_TIME)
    };
}

Automating database loading logic by reflecting on the configuration objects

Finally, if you want true magic and can make sure the configuration classes properties are named as in the database, you can automate LoadFromDatabase() with something like:

public static T LoadFromDatabase<T>(T defaultValues) where T : class, new()
{
    var result = new T();

    var getters = typeof(T).GetProperties(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance);
    foreach (var property in getters)
    {
        if (property.GetMethod != null && property.GetMethod.IsPublic
            && property.SetMethod != null && property.SetMethod.IsPublic)
        {
            var defaultValue = property.GetValue(defaultValues);
            if (property.PropertyType.Equals(typeof(int)))
            {
                var value = (int)GetDouble(property.Name, (int)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
            else if (property.PropertyType.Equals(typeof(byte)))
            {
                var value = (byte)GetDouble(property.Name, (byte)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
            else if (property.PropertyType.Equals(typeof(ushort)))
            {
                var value = (ushort)GetDouble(property.Name, (ushort)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
            else if (property.PropertyType.Equals(typeof(string)))
            {
                var value = GetString(property.Name, (string)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
            else if (property.PropertyType.BaseType.Name == "Enum")
            {
                var value = (int)GetDouble(property.Name, (int)defaultValue, ParamUnidad.SinUnidad);
                property.SetValue(result, value);
            }
        }
    }

    return result;
}

... and call the method with:

var setupConfiguration = NRG96.Configuracion.LoadFromDatabase<NRG96.SetupConfiguration>(new NRG96.SetupConfiguration
{
    PRIMARY_VOLTAGE = 1,
    SECONDARY_VOLTAGE = 1,
    PRIMARY_CURRENT = 1,
    VOLTAGE_TYPES = "SIMPLES",
    MAIN_DEFAULT_SCREEN = SetupConfiguration.MainDefaultScreens.One,
    ENERGY_DEFAULT_SCREEN = SetupConfiguration.EnergyDefaultScreens.One,
    HARMONIC_CALCULUS = "THD",
    BACKLIGHT_SHUTDOWN_TIME = 1
});

Loading all different configuration objects from the database could then be achieved without changing LoadFromDatabase<>().


Scraping webpage elements using Python I have been trying to improve my knowledge with Python and I think the code is pretty forward. However I do dislike abit the coding style I have done where I use too much try except in a content there it might not needed to be at first place.
My goal is basically to have a ready payload before scraping as you will see at the top of the code. Those should be always declared before scraping. What im trying to do basically is to try to scrape those different data. If we don't find the data, then it should skip or set the value to [], None or False (Depending on what we are trying to do).
I have read abit regarding getattr and isinstance functions but im not sure if there might be a better way than using lots of Try except as a cover if it doesn't find the element on the webpage.
I would appreciate all kind of helps!
import requests
from bs4 import BeautifulSoup

payload = {
    "name": "Untitled",
    "view": None,
    "image": None,
    "hyperlinks": []
}

site_url = "https://stackoverflow.com/questions/743806/how-to-split-a-string-into-a-list"

response = requests.get(site_url)

bs4 = BeautifulSoup(response.text, "html.parser")

try:
    payload['name'] = "{} {}".format(
        bs4.find('meta', {'property': 'og:site_name'})["content"],
        bs4.find('meta', {'name': 'twitter:domain'})["content"]
    )
except Exception:  # noqa
    pass

try:
    payload['view'] = "{} in total".format(
        bs4.find('div', {'class': 'grid--cell ws-nowrap mb8'}).text.strip().replace("\r\n", "").replace(" ", ""))
except Exception:
    pass

try:
    payload['image'] = bs4.find('meta', {'itemprop': 'image primaryImageOfPage'})["content"]
except Exception:
    pass

try:
    payload['hyperlinks'] = [hyperlinks['href'] for hyperlinks in bs4.find_all('a', {'class': 'question-hyperlink'})]

except Exception:  # noqa
    pass

print(payload)

My take, in short:

in programming there is nothing worse than silently swallowing exceptions :) And the fact that you are catching any exception, and not just those related to those specific operations, means than all kinds of errors will go unnoticed.
At the very least, the exceptions should be logged somewhere
be more specific and catch relevant exceptions instead, usually it will be stuff like HTMLParser.HTMLParseError and also requests.exceptions - experiment a bit
Note the difference in behavior between find and find_all since you are using both functions:

If find_all() can’t find anything, it returns an empty list. If find()
can’t find anything, it returns None

Armed with that knowledge you can simply test your expressions for None before you try to fetch the value with find - it's better to avoid an exception than having to handle it.
Likewise, if bs4.find_all('a', {'class': 'question-hyperlink'}) returns an empty list, you don't proceed with the list comprehension but you assign some default value, possibly: payload['hyperlinks'] = None

So the key here is to test the result of every call to find or find_all and act accordingly. Then you can get rid of those try catch blocks. Only one all-purpose exception handler for the whole procedure shall suffice.


Acquiring a data key I recently started using the builder pattern in one of my projects:

public final class DataKey {

    private final long userId;
    private final String uuid;
    private final String deviceId;
    private final int clientId;
    private final long timeout;
    private final FlowEnum flow;

    private DataKey(Builder builder) {
        this.userId = builder.userId;
        this.uuid = builder.uuid;
        this.deviceId = builder.deviceId;
        this.clientId = builder.clientId;
        this.timeout = builder.timeout;

        if (userId == 0 && uuid != null && uuid.isEmpty() && deviceId != null
                && deviceId.isEmpty()) {
            throw new IllegalStateException("You have to pass at least one"
                    + " of the following: userId, uuid or deviceId");
        }
        if (userId != 0) {
            this.flow = FlowEnum.USERFLOW;
        }
        else {
            this.flow = FlowEnum.DEVICEFLOW;
        }
    }

    public static class Builder {
        protected long userId;
        protected String uuid;
        protected String deviceId;
        protected final int clientId;
        protected long timeout = 200L;

        public Builder(int clientId) {
            this.clientId = clientId;
        }

        public Builder setUserId(long userId) {
            this.userId = userId;
            return this;
        }

        public Builder setUuid(String uuid) {
            this.uuid = uuid;
            return this;
        }

        public Builder setDeviceId(String deviceId) {
            this.deviceId = deviceId;
            return this;
        }

        public DataKey build() {
            return new DataKey(this);
        }
    }

    public long getUserId() {
        return userId;
    }

    public String getUuid() {
        return uuid;
    }

    public String getdeviceId() {
        return deviceId;
    }

    public FlowEnum getFlow() {
        return flow;
    }

    public int getclientId() {
        return clientId;
    }

    public long getTimeout() {
        return timeout;
    }
}

I also have an enum which determines whether userId or uuid/deviceId is being used. If userId is present, then the flow type is USERFLOW, otherwise it's DEVICEFLOW.

public enum FlowEnum {
    USERFLOW, DEVICEFLOW
}

While making keys using the Builder class, I will always pass clientId  will pass at least one ID key from these - userId, uuid and deviceId. If by any chance, all the ID keys are missing, then I need to throw an illegal state exception.

I need to prioritize the IDs as well if we are passing multiple IDs instead of one then userId takes the priority first, then uuid and then deviceId.

Can a validation check be done in the build() method? Also, is it thread-safe?

DataKey keys = new DataKey.Builder(100).setUserId(1234L).setUuid("aaaa").setDeviceId("sssss").addTimeout(100L).build();

System.out.println(keys.getFlow());

DataKey

State- vs. Input validation:

In your constructor you do something not quite intelligent. You first set all the fields and then check them for integrity.

IMO you should check the input data for validity before using it with something. This allows you to extract a so-called "leading guard clause". Some condition before a block of code makes it fail.

In fact you could extract the validation from the DataKey to the Builder while you're at it. Compare:

private DataKey(Builder builder) {
    if(!builder.isValid()){
         throw new IllegalStateException(INVALID_BUILDER_MESSAGE);
    }
    //keep doing the stuff you do ;)
}

Or even sooner:

public DataKey build(){
    if(!this.isValid()){
        throw new IllegalStateException(INVALID_BUILDER_MESSAGE);
    }
    return DataKey(this);
}

then you could remove the validation "clutter" from your DataKey constructor. And your isValid() method would look like:

private boolean isValid() {
    return !(userId == 0 && uuid != null
      && uuid.isEmpty() && deviceId != null
      && deviceId.isEmpty());
}

If-Statement vs Ternary operator:

    if (userId != 0) {
        this.flow = FlowEnum.USERFLOW;
    }
    else {
        this.flow = FlowEnum.DEVICEFLOW;
    }

You can shortcut this drastically by using a ternary operator. IMO this also makes your intent way more clear:

this.flow = (userId == 0) ? FlowEnum.DEVICEFLOW : FlowEnum.USERFLOW;

You can shorten this even further by adding a static import to your class:

import static fully.qualified.FlowEnum.*;

and then you get:

 this.flow = (userId == 0) ? DEVICEFLOW : USERFLOW;

But that one is personal preference, some people even see this as light obfuscation.

While we're at your enum. Currently it's no more than a glorified boolean. I will assume there could be more Flows, because if not you could just do:

this.isUserFlow = userId != 0;


Eratosthenes sieve and MPI I wrote this sieve of Eratosthenes with MPI, but I'm not sure if it's good enough. Should I use MPI_Scatter and MPI_Gather instead of collecting arrays of separate processes in the root process?

Also, I'm not sure if I call

printArray(myArray, (N/size)-1);

right, where myArray is an array of pointers. GCC gives a warning at this point.

#include "mpi.h"
#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <time.h>

#define N 60

int main(int argc, char *argv[])
{
long timeCPU;
int rank, size, k, i, v, n, scatterSize;
int **tmpArray, *myArray, firstElement, lastElement;

MPI_Init(&argc,&argv);

MPI_Comm_rank(MPI_COMM_WORLD, &rank);
MPI_Comm_size(MPI_COMM_WORLD, &size);

if(N%size != 0)
{
    MPI_Finalize();
    printf("ERROR!");
    exit(0);
}

firstElement = (rank*N)/size;
lastElement = floor(((rank+1)*N)/size);
myArray = (int*) (malloc(((N/size)-1)*sizeof(int)));

for(i=0, v=firstElement+2; i<=(N/size)-1; i++, v++)
{
    myArray[i] = v;
}
    printArray(myArray, (N/size)-1);
/* All processes have to execute above code first */
MPI_Barrier(MPI_COMM_WORLD);

k = 2;

do
{
    markMultiples(k, myArray, lastElement-firstElement);
    k = nextNumber(k, myArray, lastElement-firstElement);
    MPI_Bcast(&k, 1, MPI_INT, 0, MPI_COMM_WORLD);
}
while(k*k <=N);

MPI_Send(myArray, (N/size)-1, MPI_INT, 0, 50, MPI_COMM_WORLD);

MPI_Barrier(MPI_COMM_WORLD);

if (rank == 0)
{
    // Initialize 2D array
    tmpArray = (int**) (malloc(size*sizeof(int)));
    for(i=0;i<size;i++)
    {
        tmpArray[i] = (int*) (malloc(((N/size)-1)*sizeof(int)));
    }

    // Gather from every process his own array
    for (i=0; i<size;i++)
    {
        MPI_Recv(tmpArray[i], (N/size)-1, MPI_INT, i, 50, MPI_COMM_WORLD, MPI_STATUS_IGNORE);
        printArray(tmpArray[i], (N/size)-1);
        printf("-----------------------------------------\n");
    }

    // Build 1d array with primes - TODO

}

//printArray(array, lastElement-firstElement);
free(myArray);
MPI_Finalize();
return 0;
}

int nextNumber(int k, int *array, int n)
{
int i;

for(i=0; i<=n; i++)
{
    if(array[i] > k)
        return array[i];
}
return -1;
}

void markMultiples(int k, int *array, int n)
{
int i;

for (i=0; i<=n; i++)
{
    if(array[i] % k == 0 && array[i] != k)
    {
        array[i] = -1;
    }
}
}

void printArray(int *array, int n)
{
int i;

for(i=0; i<=n; i++)
{
    printf("array[%d] = %d\n", i, array[i]);
}
}

Since you tagged this question c++ I assume you would like a C++ approach also.

I would suggest using std::vector<int> rather than an int* and malloc. Using the vector will give you a number of useful advantages over a normal pointer, for example, memory management is greatly simplified, and there is no need to worry about how big an int is. With vector, the order of all elements is guaranteed to be contiguous.

#include <algorithm>
#include <functional>
#include <iostream>
#include <vector>

int nextNumber(int k, const std::vector<int> &array, int n)
{
    std::vector<int>::iterator i = std::find_if(array.begin(), array.begin() + n, std::bind2nd(std::greater<int>(), k));

    return (i == array.end() ? -1 : *i);
}

bool isMultiple(int a, int b)
{
    return (a % k == 0) && (a != k);
}

void markMultiples(int k, std::vector<int> &array, int n)
{
    std::replace_if(array.begin(), array.begin() + n, std::bind2nd(std::ptr_fun(isMultiple), k), -1);
}

void printArray(const std::vector<int> &array)
{
    for(int i = 0; i < array.size(); i++)
        std::cout << "array[" << i << "] = " << array[i] << std::endl;
}

Disclaimer:

I am not very good at C++, so you may want to take this advice with a teaspoon of salt.


Documenting and commenting code using xml tags I am going to rewrite my AddIn I made for the Autodesk Inventor software. It is supposed to do specific task that are not available by default in the program.

I am not a programmer but a draftsman that is interested in programming, so bare with me.

The reason for me to rewrite my program is to make it better documented and commented. Plus there is alot of code that is duplicated and hardcoded lines that must be removed.

Now I made a small sample for a class that I am doing, and how I think it should be commented and xml documented. Please feel free to comment on ALL the code not only the xml tags.

Imports Inventor

''' <summary>
''' The document object represents the Inventor Document,
''' it allows the you to rapidly access certain values and perform specific tasks
''' </summary>
Public Class Document

    ''' <summary>
    ''' Read-only property that returns the <c>Inventor.Document</c> that is associative with the XT document.
    ''' </summary>
    ''' <example>Document.ReferencedDocument() As Inventor.Document</example>
    Public Overridable ReadOnly Property ReferencedDocument As Inventor.Document

    ''' <summary>
    ''' Creates a new XT document object
    ''' </summary>
    ''' <param name="InventorDocument">The Inventor document where the new XT document will be associated with.</param>
    ''' <example>Dim XTdocument as New Document(InventorDocument As Inventor.Document)</example>
    Public Sub New(ByVal InventorDocument As Inventor.Document)

        ' Store the InventorDocument as ReferencedDocument
        ReferencedDocument = InventorDocument
    End Sub

    ''' <summary>
    ''' Gets the description of the current document
    ''' </summary>
    ''' <returns>string</returns>
    Public Property Description As String
        Get

            ' Set reference to the document property sets
            Dim oPropertySets As PropertySets =
                ReferencedDocument.PropertySets

            ' Set reference to the Design Tracking Property set
            Dim oDesignTrackingPropertySet As PropertySet =
                oPropertySets.Item("Design Tracking Properties")

            ' Set reference to the Description property
            Dim oDescriptionProperty As [Property] =
                oDesignTrackingPropertySet.Item("Description")

            ' Return the description value (as string)
            Description = oDescriptionProperty.Value

        End Get
        Set(value As String)

            ' Set reference to the document property sets
            Dim oPropertySets As PropertySets =
                ReferencedDocument.PropertySets

            ' Set reference to the Design Tracking Property set
            Dim oDesignTrackingPropertySet As PropertySet =
                oPropertySets.Item("Design Tracking Properties")

            ' Set reference to the Description property
            Dim oDescriptionProperty As [Property] =
                oDesignTrackingPropertySet.Item("Description")

            ' Return the description value (as string)
            oDescriptionProperty.Value = value

        End Set
    End Property

End Class

Only focusing on the code...no, not really, I will say something regarding the xml doc as well.

xml documentation should tell the truth, because this is what another developer reads. Seeing e.g ''' Gets the description of the current document one wouldn't expect that this property has a setter as well.
''' Creates a new XT document object isn't true either, because it is used to create a new Document object. If you have another class named XTDocument inheriting from the Document object then the doc would be ok.
Although VB.NET is case insensitive one should stick to the .NET Naming Guidelines. This means method parameters should be named using camelCase casing.
While we are at naming things, please don't use hungarian notation. Prefixing a variable with its type or with what one thinks is its type is frowned upon nowadays. What value does the o in oPropertySets add ? Having intellisense and stuff just makes it superflous. In the time of VB 6 one could say that it is easier to see the type of the variable, but nowadays one just hoovers with his mouse over the variables name and see its type.
Comments shouldn't tell what the code is doing. This should be done by the code itself by using meaningful names. Comments should always tell why the code is written like it is. As an example, please take a look at my answer here.
Nowadays (sorry I need to use this term often, because the code looks like it is ported from VB 6) we use Return someValue instead of assigning the returning values to the properties name.
Not only because it is more clear but in VB.NET this Description = oDescriptionProperty.Value will call the setter of the Description property. Hence calling the getter will call the setter as well which can lead to dangerous, because hidden, sideeffects.


Simple palindrome function for single words I have created a simple palindrome function that works only on single words. It is working as intended, but I feel that it definitely needs improvement.

def is_palindrome(w):

    if not isinstance(w, str):
        return "ERROR: input is not a string."

    elif len(w) > 2:
        if w[0] == w[-1]:
            while len(w) > 2:
                w = w[1:-1]
                is_palindrome(w)

        if len(w) == 1:
            return True

        else:
            return False

    else:
        return "ERROR: input contains two or less characters."

print is_palindrome('aibohphobia')
print is_palindrome('definitely')

Can you please review my function and post your suggestions?

Also, is it better to do w[:] = w[1:-1] instead of w = w[1:-1]?

Firstly, your testing is insufficient. This code thinks that "defad" is a palindrome and that "deed" is not. You algorithm is completely broken.

def is_palindrome(w):

    if not isinstance(w, str):
        return "ERROR: input is not a string."

Don't check types. Just treat it like a string, and trust the caller to pass the right kind of object. Also don't return strings as errors, throw exceptions.

    elif len(w) > 2:
        if w[0] == w[-1]:
            while len(w) > 2:
                w = w[1:-1]
                is_palindrome(w)

You call the function but you don't do anything with the result.

        if len(w) == 1:
            return True

        else:
            return False

This is equivalent to: return len(w) == 1

    else:
        return "ERROR: input contains two or less characters."

Why can't palindromes be zero or one characters? Also, throw exceptions, don't return strings.


Fizz having an argument with Buzz For my current project I need to validate responses. The requests will be send from multiple different shell scripts and should be controlled from another script. I'd never targeted a .sh with a .sh before, so let's try some FizzBuzz first.

Fizz.sh

#!/bin/bash

if [ $# -eq 0 ]
then
    echo "This FizzBuzz is interactive. Please provide the upper limit."
    echo "Usage : $0 limit"
    exit 1
fi

for i in `seq $1`
do
   echo `source ./buzz.sh $i`
done

Buzz.sh

#!/bin/bash

([ $(($1%15)) -eq 0 ] && echo 'FizzBuzz') ||
([ $(($1%5)) -eq 0 ] && echo 'Buzz') ||
([ $(($1%3)) -eq 0 ] && echo 'Fizz') ||
echo $1;

Is it idiomatic?
Is this the best way to hand all output from Buzz to Fizz?

The goal is to learn decent BASH. The FizzBuzz has no need for optimization, it's mostly about style, how data should be passed from script to script and whether I followed BASH-practices or not.

Two points:

echo $(something that already prints) is just redundant. This is simpler

source ./buzz.sh $i

There does not appear to be any need for two separate files. Put buzz in a function. Also, you are using a lot of subshells that are unnecessary (a). I'd stick with the more verbose but clearer if

buzz() {
    if (( $1 % 15 == 0 )); then
        echo 'FizzBuzz'
    elif (( $1 % 5 == 0 )); then
        echo 'Buzz'
    elif (( $1 % 3 == 0 )); then
        echo 'Fizz'
    fi
    echo $1
}

then

for i in $(seq $1); do buzz $i; done

a) if you want a grouping construct without the overhead of spawning a new shell, use braces:

{ echo foo; echo bar; echo baz; } | tac


Converting a binary string to an ASCII string, the longer way I had an interview recently and discovered that I'd forgotten some of the basics.  I've been playing around again and have written a function that will take a binary string (there is no validation yet) and returns the ASCII representation of said string.

I'm looking for advice or tips on how it could be improved. I don't want to use any of the API functions, this is more of a playground scenario in which I might be able to learn something.

public static String convertBinaryStringToString(String string){
    StringBuilder sb = new StringBuilder();
    char[] chars = string.replaceAll("\\s", "").toCharArray();
    int [] mapping = {1,2,4,8,16,32,64,128};

    for (int j = 0; j < chars.length; j+=8) {
        int idx = 0;
        int sum = 0;
        for (int i = 7; i>= 0; i--) {
            if (chars[i+j] == '1') {
                sum += mapping[idx];
            }
            idx++;
        }
        System.out.println(sum);//debug
        sb.append(Character.toChars(sum));
    }
    return sb.toString();
}

Sample output:

01101000 01100101 01101100 01101100 01101111
104
101
108
108
111
hello

int [] mapping = {1,2,4,8,16,32,64,128};

This seems avoidable - array access could be replaced with a bitshift. It'd also clear up what you're doing...

for (int j = 0; j < chars.length; j+=8) {
    int idx = 0;
    int sum = 0;
    for (int i = 7; i>= 0; i--) {
        if (chars[i+j] == '1') {
            sum += 1 << idx;
        }
        idx++;
    }
    System.out.println(sum);//debug
    sb.append(Character.toChars(sum));
}

It's only after making this change that I understand what you're doing! You're splitting the input string into segments that represent a single character (I got that far already), but after that, you start at the back of the represented character, and check for each bit representing character that it is '1'. If it's '1' (and thus not '0'), you add the value that the bit would represent to sum. Once you've gone through a single character representation, you let Character.toChars cast the int-value of the character back to a character. Then you add it to the result buffer, which will form the full string.

Phew.

You need more comments, so that other developers can read and understand directly how this method works.

Specifically, 1 comment to explain what your replaceAll does (regex are not obvious to me and I have to look them up), and 1 comment to say that you're parsing bits in reverse per character.

Maybe like so:

//for each character
for (int j = 0; j < chars.length; j+=8) {
    int idx = 0;
    int sum = 0;
    //for each bit in reverse
    for (int i = 7; i>= 0; i--) {
        if (chars[i+j] == '1') {
            sum += 1 << idx;
        }
        idx++;
    }
    System.out.println(sum);//debug
    sb.append(Character.toChars(sum));
}

Getting rid of all the built-in functions (for fun only)

So you want to do everything yourself?

This is bad style, since everything in java.lang (and in generally, java.util as well) can be relied on without turning this into a puzzle for your fellow programmers, but I'll humor you.

First, you can get rid of the String.split via Thomas Junk's answer, incrementing by 9 each time instead.

public static String convertBinaryStringToString(String string){
    StringBuilder sb = new StringBuilder();
    char[] chars = string.toCharArray();

    //for each character
    for (int j = 0; j < chars.length; j+=9) {
        int idx = 0;
        int sum = 0;
        //for each bit in reverse
        for (int i = 7; i>= 0; i--) {
            if (chars[i+j] == '1') {
                sum += 1 << idx;
            }
            idx++;
        }
        System.out.println(sum);//debug
        sb.append(Character.toChars(sum));
    }
    return sb.toString();
}

Leaves us with this bit of code.

We can get rid of the StringBuilder by writing to a char[]. We can get rid of Character.toChars(sum) with (char) sum. We do have to make a new string out of the character array, though.

public static String convertBinaryStringToString(String string){
    char[] chars = string.toCharArray();
    char[] transcoded = new char[(chars.length / 9)+1];

    //for each character (plus one for spacing)
    for (int j = 0; j < chars.length; j+=9) {
        int idx = 0;
        int sum = 0;

        //for each bit in reverse
        for (int i = 7; i>= 0; i--) {
            if (chars[i+j] == '1') {
                sum += 1 << idx;
            }
            idx++;
        }
        transcoded[j/9] = (char) sum;
    }
    return new String(transcoded);
}

And voila, we're rid of most of the built-ins. There's no way to get the contents of a String without use of built-ins, however, so string.toCharArray or string.charAt is required.


Merge Intervals(Golang) Solving the merge intervals problem in golang(link: https://leetcode.com/problems/merge-intervals/).
The Problem
Given an array of intervals where intervals[i] = [starti, endi], merge all overlapping intervals, and return an array of the non-overlapping intervals that cover all the intervals in the input.

Example 1:

Input: intervals = [[1,3],[2,6],[8,10],[15,18]]
Output: [[1,6],[8,10],[15,18]]
Explanation: Since intervals [1,3] and [2,6] overlaps, merge them into [1,6].
Example 2:

Input: intervals = [[1,4],[4,5]]
Output: [[1,5]]
Explanation: Intervals [1,4] and [4,5] are considered overlapping.

Code
package main

import (
    "fmt"
    "sort"
)

func main() {

    var intervals [][]int
    intervals = append(intervals, []int{7, 10})
    intervals = append(intervals, []int{3, 4})
    intervals = append(intervals, []int{2, 5})

    mergeIntervals(intervals)

}

func mergeIntervals(intervals [][]int) [][]int {

    if len(intervals) < 2{
        return intervals
    }
    var mergedIntervals [][]int
    fmt.Println("intervals :", intervals)
    sort.Slice(intervals, func(i, j int) bool {
        return intervals[i][0] < intervals[j][0]
    })

    previousEndTime := intervals[0][1]
    for index, _ := range intervals {

        if index >= len(intervals)-1 {

            if index == len(intervals)-1 {
                mergedIntervals = append(mergedIntervals, []int{intervals[index][0], intervals[index][1]})
            }
            break
        }
        if index != 0 {
            //skip current interval if previous interval end time is higher
            if previousEndTime > intervals[index][1] {
                continue
            }
        }
        fmt.Println("comparing intervals ", intervals[index], "and ", intervals[index+1])
        if intervals[index][1] > intervals[index+1][1] {
            //check if current end time greater than end time of next interval
            mergedIntervals = append(mergedIntervals, []int{intervals[index][0], intervals[index][1]})
            previousEndTime = intervals[index][1]
        } else if intervals[index][1] >= intervals[index+1][0] {
            //the actual merge for overlapping  values
            mergedIntervals = append(mergedIntervals, []int{intervals[index][0], intervals[index+1][1]})
            previousEndTime = intervals[index+1][1]
        } else {
            //just include current interval with no overlap
            mergedIntervals = append(mergedIntervals, []int{intervals[index][0], intervals[index][1]})
            previousEndTime = intervals[index][1]
        }
    }

    fmt.Println("merged intervals ", mergedIntervals)
    return mergedIntervals
}

Looking for

Code Bugs.
Feel this code is too verbose.Any suggestions on how to    optimize
the code further.

Your code appears to fail on:

Example 1:

Input: intervals = [[1,3],[2,6],[8,10],[15,18]]
Output: [[1,6],[8,10],[15,18]]
Explanation: Since intervals [1,3] and [2,6] overlaps, merge them into [1,6].

Output:
intervals : [[1 3] [2 6] [8 10] [15 18]]
comparing intervals  [1 3] and  [2 6]
comparing intervals  [2 6] and  [8 10]
comparing intervals  [8 10] and  [15 18]
merged intervals  [[1 6] [2 6] [8 10] [15 18]]

Try to simplify your code to make it more readable. For example,
package main

import (
    "fmt"
    "sort"
)

func merge(intervals [][]int) [][]int {
    const start, end = 0, 1

    var merged [][]int

    if len(intervals) > 1 {
        sort.Slice(intervals, func(i, j int) bool {
            return intervals[i][start] < intervals[j][start]
        })
    }

    for _, interval := range intervals {
        last := len(merged) - 1
        if last < 0 || interval[start] > merged[last][end] {
            merged = append(merged,
                []int{start: interval[start], end: interval[end]},
            )
        } else if interval[end] > merged[last][end] {
            merged[last][end] = interval[end]
        }
    }

    return merged[:len(merged):len(merged)]
}

func main() {
    tests := []struct {
        intervals [][]int
    }{
        {[][]int{{1, 3}, {2, 6}, {8, 10}, {15, 18}}},
        {[][]int{{1, 4}, {4, 5}}},
        {[][]int{{1, 2}}},
        {[][]int{}},
        {[][]int{{7, 10}, {3, 4}, {2, 5}}},
    }

    for _, tt := range tests {
        fmt.Print(tt.intervals)
        fmt.Println(" ->", merge(tt.intervals))
    }
}

Playground: https://play.golang.org/p/Akgsws42JfX
Output:
[[1 3] [2 6] [8 10] [15 18]] -> [[1 6] [8 10] [15 18]]
[[1 4] [4 5]] -> [[1 5]]
[[1 2]] -> [[1 2]]
[] -> []
[[7 10] [3 4] [2 5]] -> [[2 5] [7 10]]


Scanning a directory and listing contents in an HTML file My program simply accepts the name of a directory from the user, scans said directory and generates an HTML file that lists the contents.

Is there any way that this can be improved?

/*
    Basically, take the path of a directory, get a list of all files in the directory
    and output that list to an html document
*/
import java.util.Scanner;
import java.util.ArrayList;
import java.util.Arrays;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.BufferedWriter;
import java.nio.file.FileSystemException;

public class HTMLFileListGenerator{
    static void writeDirectoryFilesToHTML(File directory) throws IOException{
        ArrayList<String> contents = new ArrayList<String>(Arrays.asList(directory.list()));

        File output = new File("output.html");
        try(BufferedWriter writer = new BufferedWriter(new FileWriter(output))){
            writer.write("\n\t\n\t\t
Contents of " + directory.getCanonicalPath() + ":
\n\t\t
\n\t\t\t");

            for(int counter = 0; counter < contents.size(); counter++){
                System.out.println(contents.get(counter));
                writer.write("\t\t\t
 " + contents.get(counter) + "
\n");
            }
            writer.write("\t\t
\n\t\n");
        }
    }
    public static void main(String[] args){
        try(Scanner userInput = new Scanner(System.in)){
            File testInput;
            do{
                System.out.print("Please enter the name of the directory: ");
                String input = userInput.next();
                testInput = new File(input);
            }
            while(!testInput.isDirectory());
            writeDirectoryFilesToHTML(testInput);
        }
        catch(IOException io_exc){
            System.out.println("An error occurred: " + io_exc.getMessage());
        }
    }
}

Files and Paths

Since Java 7, the recommended way of traversing a file system is to rely on the new Path class and associated Files and Paths helper classes.

Furthermore, since you are on Java 8, there is the Files.list(Path) method that lets you iterate through a given directory as a form of Stream<Path>, so that you can leverage on the stream-processing features.

For example, just to pad each entry in the given directory with
...
, the following snippet does something similar to your for-loop:

List<String> contents;
try {
    // assuming directoryPath is validated as a directory already
    contents = Files.list(directoryPath)
                    .map(v -> "
" + v + "
")
                    .collect(Collectors.toList());
} catch (IOException e) {
    // handle accordingly
}

To write contents out to a file, there's also Files.write(Path, Iterable, OpenOption):

try {
    Files.write(outputPath, contents);
} catch (IOException e) {
    // handle accordingly
}

However, since you need to write the header and 'close' the end of the HTML document, you can rely on the forEach() terminal operation of a Stream:

private static void writeDirectoryFilesToHTML(Path directoryPath,
                                                Path outputPath) throws IOException {
    try (FileWriter target = new FileWriter(outputPath.toFile());
            BufferedWriter buffered = new BufferedWriter(target);
            PrintWriter writer = new PrintWriter(buffered)) {
        writer.write("\n\t\n\t\t
Contents of "
                        + directoryPath.toRealPath()
                        + ":
\n\t\t
\n");
        Files.list(directoryPath)
                .map(v -> "\t\t\t
 " + v + "
")
                .forEach(writer::println);
        writer.write("\t\t
\n\t\n");
    }
}

Here, writer::println is used as a method reference.

HTML output and OS-dependent newlines

You can also consider using some sort of a HTML output library so that you do not have to manually specify "...
...
..." (for example).

You are currently hard-coding your newlines to be \n, so you may want to create a helper method that writes the OS-dependent newlines for you, after every call to write().

Other observations

As you can see from above, it's recommended that your method takes in the output path as well, instead of assuming "output.html".


python3 utility: convert safely to int I made this utility function to check for parameters.
All it does is checking whether a given variable is an int, or a float/str that represent exactly an int (ie. no decimal part or NaN), and casting to that int (raising a ValueError exception on failure).
I'm not very experienced with Python (especially Python 3+), so I'm still learning and trying to improve my programming skills.
I'm open to any critics, improvements, and bad/good practices tips.
Also I'm trying to learn how to properly document Python code (my IDE is PyCharm and hints me to use reStructuredText docblocks, and default Python3 typing hints).
I'm using Python3.6 for the following code:
util.py
from typing import Union

def int_exact_cast(n: Union[int, float, str]) -> int:
    """
    Tries to cast parameter to int without losing decimal part or raise an exception
    :param Union[int, float] n: The value to be converted
    :type n: Union[int, float]
    :return: The converted int value
    :rtype: int
    :raises ValueError: raises ValueError exception if the parameter can't be casted exactly to int
    """

    t = type(n)

    if t is int:
        return n  # return immediately if is already int (optimization)

    if not (t is str or t is float):
        raise ValueError('Parameter must be an int, str or float')

    f = float(n)

    if not f.is_integer():  # not checking for math.nan because in that case is_integer() returns False
        raise ValueError('Parameter contains a decimal part')

    return int(f)

I know this question/post is old, but...

Use isinstance rather than directly storing and then checking the type() output.

We can get rid of t and two of your if statements to check type by using isinstance instead, and check against all three acceptable types:

if not (isinstance(n, (float, str, int))):
    raise ValueError('Parameter must be an int, str or float')

Just a thought on this, though: If something provided is not an int, str, or float, then it's an invalid type, and should raise a TypeError, rather than a ValueError, because TypeError is more generally used when you're passing an invalid object type into a function that can't handle it.  Therefore, your raise ValueError("...") for cases where the parameter is not an int, str, or float should probably be a TypeError with the same error message.

As was mentioned, isinstance will not behave right if you are using inherited classes which are subclasses of other primary classes.  However, consider that subclasses building upon other immutable classes are rare and as long as you aren't testing for instances of a bare object, this should suit your purposes.

The final code is this:

from typing import Union

def int_exact_cast(n: Union[int, float, str]) -> int:
    """
    Tries to cast parameter to int without losing decimal part or raise an exception
    :param Union[int, float] n: The value to be converted
    :type n: Union[int, float]
    :return: The converted int value
    :rtype: int
    :raises ValueError: raises ValueError exception if the parameter can't be casted exactly to int
    """

    if not (isinstance(n, (float, str, int))):
        raise ValueError('Parameter must be an int, str or float')

    f = float(n)

    if not f.is_integer():
        raise ValueError('Parameter contains a decimal part')

    return int(f)

Just keep in mind that if the string can't be converted to a number (such as if the string is 'I Am A Teapot'), then you get a different ValueError from the conversion system, because it's Not a Number.  (Which is why you don't need the NaN check with this method, but you have to be careful with what you're working with)


Rails method to match users according to language fluency level This code works fine, but I'm still learning, and feel like there is a more Railsy way to write this. Is there a good rails ActiveRecord method I should learn more about? Please let me know if you know a shorter, more direct way to run this same code.

Scenario: A user can have many languages, and level is an attribute of languages. I want to iterate through languages_users and find all the languages of a user, and then find where another user has 2 of the same languages, but at opposite levels.

user.rb

class User < ApplicationRecord
  has_many :languages_users
  has_many :languages, :through => :languages_users
end

controller

#Fluent languages and nonfluent languages can change depending on this situation, but here is good example:
@fluent_languages = LanguagesUser.where(user: current_user.id).where("level > 4")
@nonfluent_language = LanguagesUser.where(user: current_user.id).where("level < 5")

email_matches(@fluent_languages, @nonfluent_languages)

def email_matches(fluent_languages, nonfluent_languages)

    @fluent_languages = fluent_languages
    @nonfluent_languages = nonfluent_languages

    #array of user's fluent language_ids
    @fluent_langs = []
    @fluent_langs << @fluent_languages

    #array of user's nonfluent language_ids
    @nonfluent_langs = []
    @nonfluent_languages.each do |lang|
        @nonfluent_langs << lang.language_id
    end

    @fluent_les = LanguagesUser.select(:user_id).where(language_id: @fluent_langs, level: 1..4)
    @fluent_users_ids = []
    @fluent_les.each do |le|
        @fluent_users_ids << le.user_id
    end

    @nonfluent_les = LanguagesUser.select(:user_id).where(language_id: @nonfluent_langs, level: 5)
    @nonfluent_user_ids = []
    @nonfluent_les.each do |le|
        @nonfluent_user_ids << le.user_id
    end

    @matches = @fluent_users_ids & @nonfluent_user_ids

    @new_matches = User.where(id: @matches, status: "Active").order('last_seen DESC').limit(10)
    @new_matches.each do |user|
        UserMailer.new_match(user, current_user).deliver_later
    end
end

Let me know if I can provide anything else.

This is a somewhat shorter more concise version of your code. Taking advantages of some or Rails's magic and conventions.

def email_matches(current_user)
  fluent_ids = current_user.languages.where("level > 4").pluck(:language_id)
  nonfluent_ids = current_user.languages.where("level < 5").pluck(:language_id)

  fluent_user_ids = LanguagesUser.where(language_id: fluent_ids, level: 1..4)
    .pluck(:user_id)
  nonfluent_user_ids = LanguagesUser.where(language_id: nonfluent_ids, level: 5)
    .pluck(:user_id)

  matches = fluent_user_ids & nonfluent_user_ids

  new_matches = User.where(id: matches, status: "Active").order(last_seen: :desc).limit(10)
  new_matches.each do |user|
      UserMailer.new_match(user, current_user).deliver_later
  end
end

This takes advantage of the has_many :languages in your User model. Also uses the pluck method which returns an array of language_ids

current_user.languages.where.where("level > 4").pluck(:language_id)

There is however a way to do this all in maybe 1-2 lines of SQL, maybe someone else will provide that magic here.


Was this use of  tags in this HTML over background images better? I saw this UI for a mobile app, but I reverse engineered it for the web. This is how it looks in my CodePen

This would be either a favorites or featured list locations to travel to. I decided to go with a
 with class of "locations"

HTML

      New York

      San Francisco

      Paris

      London

In Production, I will be building this with Meteor which uses MustacheJS templates. So this is likely how I will structure the list.

MustacheJS template (proposed)

<template name="featuredLocations">

        {{city}}

</template>

CSS

.locations {
  list-style-type: none;
  padding-left: 0;
  margin: 0;
}
.locations > li {
  border-bottom: 1px #dd4977 solid;
  height: 136px;
  width: 100%;
  position: relative;
  overflow:hidden;
}
.locations li a {
  background-color: #777;
  display: block;
  height: 100%;
  postion: relative;
  text-decoration: none;
  z-index: 1;
}

.locations > li > img {
  width: 100%;
  height: 135px;
  position: absolute; top:0; left: 0; right: 0;
  z-index: 10;
}

.locations > li > a > span {
  background-color: #dd4977;
  color: #fff;
  font-size: 21px;
  letter-spacing: 1px;
  padding: 3px 5px 0 5px;
  position: absolute; bottom: 0; left: 6px;
  z-index: 20;
}

The actual images in production will be bigger and will scale down according to the device. There will be media queries that are not shown here.

The use of ul seems appropriate. My concern with using <img>s would be with the image dimensions. From what I can see above, unless you create your images specifically in those dimensions, it will become a pain to ensure they scale correctly.

I've found that if you want to preserve a predetermined width and height for your images without ruining the aspect ratio it's almost always easier to use background-image rather than <img> tags.

Using background-size: cover you can ensure that the image covers the available space, without losing the ratio:

  The cover value specifies that the background image should be sized so
  that it is as small as possible while ensuring that both dimensions
  are greater than or equal to the corresponding size of the container


reorder a list in java I have three ways of reordering a list (think baseball order) and I am sure you can come up with more.  What is a the best way?

For example if input is list is 1,2,3,4 and current is 3 then the output should be 3,4,1,2.
Another example list: 1,2,3,4 and current 4 output is 4,1,2,3.

public void reOrder(List<String> list, String current) {

        if (list.size() > 1) {
            for (int i = 0; i < list.size(); i++) {
                String host = list.remove(0);
                if (host.equals(current)) {
                    list.add(0, host);
                    break;
                }
                list.add(host);

            }
        }
    }

   public void reOrder2(List<String> list, String current) {

        if (list.size() > 1) {
            int indexOfWorkingHost = list.indexOf(current);
            if (indexOfWorkingHost != -1) {
                for (int i = 0; i < indexOfWorkingHost; i++) {
                    list.add(list.remove(0));
                }
            }
        }
    }

public void reOrder3(List<String> list, String current) {

    if (list.size() > 1) {
        List<String> tmpServers = new ArrayList<String>();
        tmpServers.addAll(list);
        for (String host : tmpServers) {
            if (host.equals(current)) {
                break;
            }
            String first = list.remove(0);
            list.add(first);
        }
    }
}

The first thing you need to do is define what you mean with "best". Fastest to execute? Most readable? Best Java-ness?

In any case neither solution is very "nice". The first one unecessarily removes the "current" item und re-adds it, the second one uses indexOf, thus indirectly has two loops, and finally the third one makes a copy of the list, which is even slower.

When using "low level" removing/adding and modification of the original list, I guess I'd do it like this (untested):

public void reOrder(List<String> list, String current) {
   int i = list.length;
   while (i-- > 0 && list.get(0) != current)
      list.add(list.remove(0));
}

(BTW, consider making use of the generics: public <T> void reOrder(List<T> list, T current) {...)

There is also a Java API function that does this (Collections.rotate), but it requires the index of the "current" element. If you have a "better" or more optimized why to get the index, other than the "left to right" search of indexOf (such as a binary search, or a map), then that would be a better solution.

In any case, using the current item's index instead of the item itself is something you should consider (depending on the the architecture of your project), because it generally not a good idea to have to search for the index of an item every time you do something with the list.

If speed is really a practical problem for you, then another thing you should check, is if you really need to reorder the actual list. There are ways just to create a "view" of the list in the desired order which would basically take virtually no additional time (especially it would be independently from the length of the list) to create and use.


Nested Deck class design and implementation Previous review of this project:

Deck and Card classes and member-accessing with one header

I'm nearly finished with my deck of cards project, and this time I made changes to hide the Card class by nesting it in the Deck class.  I'm still getting correct results when displaying the deck.  However, I do now realize that I can no longer maintain Card objects outside the class.  Instead, I have to call Card deal() with a Deck object in order to retrieve a Card, and I cannot store that Card anywhere.

Does this render my design ineffective for this purpose?  If so, how else can I maintain Cards outside the class while still preventing the user from directly constructing a Card?  Would a private constructor (with Card un-nested from Deck) be a better option?

Deck.h

#ifndef DECK_H
#define DECK_H

#include <iostream>
#include <array>

class Deck
{
private:
    class Card
    {
    private:
        char rank;
        char suit;
        unsigned value;

    public:
        Card::Card() : value(0), rank(' '), suit(' ') {}
        Card::Card(char r, char s, unsigned v) : rank(r), suit(s), value(v) {}
        bool operator<(const Card &rhs) const {return (value < rhs.value);}
        bool operator>(const Card &rhs) const {return (value > rhs.value);}
        bool operator==(const Card &rhs) const {return (suit == rhs.suit);}
        bool operator!=(const Card &rhs) const {return (suit != rhs.suit);}
        friend std::ostream& operator<<(std::ostream &out, const Card &aCard);
        friend std::ostream& operator<<(std::ostream &out, const Card &aCard)
            {return out << '[' << aCard.rank << aCard.suit << ']';}
    };

    static const unsigned VALUES[13];
    static const char RANKS[13];
    static const char SUITS[4];
    static const unsigned MAX_SIZE = 52;
    std::array<Card, MAX_SIZE> cards;
    int topCardPos;

public:
    Deck();
    void shuffle();
    Card deal();
    unsigned size() const {return topCardPos+1;}
    bool empty() const {return topCardPos == -1;}
    friend std::ostream& operator<<(std::ostream &out, const Deck &aDeck);
};

#endif

Deck.cpp

#include <algorithm>
#include "Deck.h"

const unsigned Deck::VALUES[13] = {1,2,3,4,5,6,7,8,9,10,10,10,10};
const char Deck::RANKS[13] = {'A','2','3','4','5','6','7','8','9','T','J','Q','K'};
const char Deck::SUITS[4] = {'H','D','C','S'};

Deck::Deck() : topCardPos(-1)
{
    for (unsigned rank = 0, value = 0; rank, value < 13; ++rank, ++value)
    {
        for (unsigned suit = 0; suit < 4; ++suit)
        {
            topCardPos++;
            cards[topCardPos] = Card(RANKS[rank], SUITS[suit], VALUES[value]);
        }
    }

    shuffle();
}

void Deck::shuffle()
{
    topCardPos = MAX_SIZE-1;
    std::random_shuffle(&cards.front(), &cards.back()+1);
}

Deck::Card Deck::deal()
{
    if (empty())
    {
        std::cerr << "\nDECK IS EMPTY\n";
        Card blankCard;
        return blankCard;
    }

    topCardPos--;
    return cards[topCardPos+1];
}

std::ostream& operator<<(std::ostream &out, const Deck &aDeck)
{
    for (unsigned iter = aDeck.size(); iter--> 0;)
    {
        out << "\n" << aDeck.cards[iter];
    }

    return out;
}

The way I understand your question, you want Card to be publicly available, yet only Deck should be able to construct a Card. One way to achieve this is to make the Card()  constructor private and a friend of Deck:

class Deck;

class Card {
    Card() {}
    friend class Deck;
public:
    // ...
};

class Deck {
    Card card;                     // ok
public:
    Card createCard() {            // ok
        return Card();
    }
};

int main()
{
    Deck deck;
    Card card = deck.createCard();

    Card doesntWork;               // error! Attempt to access private member
}

A somewhat cleaner solution would be to have only createCard() as friend of Card. In that case, however, you will have to dynamically allocate the Card (and return a pointer to it), because of mutual dependencies.

(The reason for this is that Card needs the definition, and not just declaration, of Deck to declare one of the Deck functions a friend. At the same time, Deck::createCard() needs the definition of Card to be able to return a Card by value. If you return a pointer, however, you only need the declaration.)

Other comments:

1: Consider giving ranks, suits and values distinct and unique types. For example using enum. Make use of the type system to enlist the compiler's aid -- by doing this you will be notified if you accidentally switch types somewhere.

2: I would change

for (unsigned iter = aDeck.size(); iter--> 0;)

to the more traditional

for (int i = aDeck.size() - 1; i > -1; --i)

The latter is more readable and clear about what is going on. The former is vulnerable to be incorrected, i.e. refactored to the (incorrect)

for (unsigned iter = aDeck.size(); iter > 0; --iter)

3: Don't call a variable iter when it's a counter and not an iterator.

4: Initializer list ordering should be the same as the variables are declared in the class. By doing this, you avoid ever risking dependency problems. (Variables are always initialized in the order they are declared in the class, not the order they appear in initializer lists.)

5: Reorder your #includes in the implementation file.

Update

6: Your card generation/constructor is error-prone. Apart from introducing type safety, I would recommend generating value based on rank, for example through a function and an std::map.