Two implementations of file-reading I work quite a lot with file reading and writing in C/Win32 and I started wondering if my code is completely ugly and how I can improve it. I wrote two simple examples of reading a file, and I wonder how these functions could be improved. Are these methods bad - and how they can be improved? I use MSVC so the code might contain MS-specific macros or keywords.

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    DWORD check = 0;
    BOOL success = FALSE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile != INVALID_HANDLE_VALUE)
    {
        if((*outsize = GetFileSize(hFile, NULL)) != INVALID_FILE_SIZE)
        {
            if((*outdata = malloc(*outsize)) != NULL)
            {
                if(ReadFile(hFile, *outdata, *outsize, &check, NULL) && check == *outsize)
                {
                    success = TRUE;
                }
            }
        }
    }

    if(hFile != INVALID_HANDLE_VALUE)
    {
        CloseHandle(hFile);
    }

    return success;
}

BOOL read_file2(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    DWORD check = 0;
    BOOL success = TRUE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile == INVALID_HANDLE_VALUE)
    {
        success = FALSE; goto _end;
    }

    if((*outsize = GetFileSize(hFile, NULL)) == INVALID_FILE_SIZE)
    {
        success = FALSE; goto _end;
    }

    if((*outdata = malloc(*outsize)) == NULL)
    {
        success = FALSE; goto _end;
    }

    if(!ReadFile(hFile, *outdata, *outsize, &check, NULL) || check != *outsize)
    {
        success = FALSE; goto _end;
    }

_end:

    if(hFile != INVALID_HANDLE_VALUE)
    {
        CloseHandle(hFile);
    }

    return success;
}

Questions:

Which one is better, and why?
What is bad about the above examples?
Can you show a better example or how you would do it?

You could improve your first example by returning early and moving variable definitions nearer to first use. I prefer this to the use of gotos but maybe I've just been conditioned against their use for too long:

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    HANDLE h = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
    if(h == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    BOOL success = FALSE;
    if((*outsize = GetFileSize(h, NULL)) != INVALID_FILE_SIZE)
    {
        if((*outdata = malloc(*outsize)) != NULL)
        {
            DWORD check = 0;
            if(ReadFile(h, *outdata, *outsize, &check, NULL) && check == *outsize)
            {
                success = TRUE;
            }
            else { free(*outdata); }
        }
    }
    CloseHandle(h);
    return success;
}

Another approach is to split into two functions, thereby removing the need for nesting and gotos.  The compiler can optimize away the function call (eg by inlining) if read_file_data is made static, but this inner function might possibly be useful in itself:

static BOOL read_file_data(HANDLE h, DWORD size, PBYTE *outdata)
{
    *outdata = malloc(size);
    if (*outdata == NULL) {
        return FALSE;
    }
    DWORD size_read = 0;
    if (ReadFile(h, *outdata, size, &size_read, NULL) && (size_read == size)) {
        return TRUE;
    }
    free(*outdata);
    return FALSE;
}

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    HANDLE h = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
    if (h == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    BOOL success = FALSE;
    *outsize = GetFileSize(h, NULL);
    if (size != INVALID_FILE_SIZE) {
        success = read_file_data(h, *outsize, outdata);
    }
    CloseHandle(hFile);
    return success;
}

You can also improve the goto method a little by returning early and changing the default success to FALSE (and freeing the buffer on error).

BOOL read_file2(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    BOOL success = FALSE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    if((*outsize = GetFileSize(hFile, NULL)) == INVALID_FILE_SIZE) {
        goto _end;
    }
    if((*outdata = malloc(*outsize)) == NULL) {
        goto _end;
    }
    DWORD check = 0;
    if(!ReadFile(hFile, *outdata, *outsize, &check, NULL) || (check != *outsize)) {
        free(*outdata);
    }
    else success = TRUE;

_end:
    CloseHandle(hFile);
    return success;
}


Business interfaces and packages When I last did a lot of Java programming, I'd do this:

All classes in a package would be default (package) visibility.
Each package would have a public PackageNamePack factory class, e.g. BusinessPack.
And the package would have a number of interfaces.
The package factory would have methods like newBusiness(...) which would instantiate new BusinessImplWhatever objects, cast as a Business interface.

The factory class:

package io.eaternet.adapters.framework;

public class FrameworkPack {
    public static Business newBusiness(String name, String address, String city, String zipcode, String origKey) {
        return new BusinessImpl(name, address, city, zipcode, origKey);
    }

    public static Inspection newInspection(String origKey, String businessOrigKey, String score, String date) {
        return new InspectionImpl(origKey, businessOrigKey, score, date);
    }
}

An interface:

package io.eaternet.adapters.framework;

public interface Business {
    public String getName();
    public String getAddress();
    public String getCity();
    public String getZipcode();
    public String getOrigKey();
}

A package-visibility implementation class:

package io.eaternet.adapters.framework;

class BusinessImpl implements Business {
    String name, address, city, zipcode, origKey;

    BusinessImpl(String name, String address, String city, String zipcode, String origKey) {
        this.name = name;
        this.address = address;
        this.city = city;
        this.zipcode = zipcode;
        this.origKey = origKey;
    }

    public String getName() { return name; }
    public String getAddress() { return address; }
    public String getCity() { return city; }
    public String getZipcode() { return zipcode; }
    public String getOrigKey() { return origKey; }
}

I've started a new Java library which follows this pattern, and am wondering if it still make sense. This particular code is part of a CSV parser, which is why everything is a string.

FrameworkPack is not a factory.
Business declares methods that automatically are public, remove the word public.
BusinessImpl looks like a ReadOnly-Class, declare the fields as final.
If FrameworkPack shall never be instanicated, create a private Constructor and make the class final so noone is able to create a instance.

You can use the Abstract-Factory and Singleton- pattern by ease.

public interface Framework {
    Business newBusiness(String name, String address, String city,
                         String zipcode, String origKey);
    Inspection newInspection(String origKey, String businessOrigKey,
                             String score, String date);
}
public interface Business {
    String getName();
    String getAddress();
    String getCity();
    String getZipcode();
    String getOrigKey();
}
public final class FrameworkPack implements Framework {
    private FrameworkPack(){};
    private static final FrameworkPack INSTANCE;
    public static final FrameworkPack getInstance(){
        if (INSTANCE==null) {
           ensureExistingInstance();
        }
        return INSTANCE;
    }
    private synchronized void ensureExistingInstance(){
       if(INSTANCE==null){
         INSTANCE = new FrameworkPack();
       }
    }

    public BusinessImpl newBusiness(String name, String address, String city,
                         String zipcode, String origKey){
      return new BusinessPack(name, address, city, zipcode, origKey);
    }
    ...
}
public final class BusinessPack {
    private final String name;
    private final String address;
    private final String city;
    private final String zipCode;
    private final String origKey;
    BusinessPack(String name, String address, String city,
                         String zipcode, String origKey) {
      this.name = name;
      this.address = address;
      this.city = city;
      this.zipcode = zipcode;
      this.origKey = origKey;
    }
    ...
}


Writing my first binary search by myself Is there any way in which I can improve my code?  I don't know if it is 100% right, and if it can be tweaked in order to be more efficient. That's why I'm posting here.
I'm open to any suggestions :)
#include <iostream>

using namespace std;

int binarySearch(int arr[], int low, int high, int n)
{
    if (low <= high)
    {
        int i = (high + low) / 2;
        if (arr[i] == n)
        {
            return i;
        }
        else
        {
            if (arr[i] > n)
            {
                return binarySearch(arr, low, i - 1, n);
            }
            if (arr[i] < n)
            {
                return binarySearch(arr, i + 1, high, n);
            }
        }
    }
    return -1;
}

int main()
{
    int arr[10]{ 1, 2, 3, 4, 5, 6, 8 }, n = 7;
    cout<<binarySearch(arr, 0, 6, 9);
    return 0;
}

Thanks.

Avoid deeply nested if-statements
Deeply nested if-statements are hard to read. You can reduce the amount of nesting here by inverting the first if-statement and returning early, and avoiding the else-statement by using the fact that the if part already returns:
int binarySearch(int arr[], int low, int high, int n)
{
    if (low > high)
    {
        return -1;
    }

    int i = (high + low) / 2;
    if (arr[i] == n)
    {
        return i;
    }

    if (arr[i] > n)
    {
        return binarySearch(arr, low, i - 1, n);
    }
    else
    {
        return binarySearch(arr, i + 1, high, n);
    }
}

Use size_t for array indices
When dealing with sizes, counts and indices, prefer using size_t to hold their values, as that type is guaranteed to be big enough to cover any array that can be addressed by your computer.
size_t is unsigned, so it might look like you can't return -1, but it turns out that integer promotion rules make it work regardless: you can return -1, and the caller can check if (binarySearch(...) == -1), although perhaps better would be to create a constant for this, similar to std::string::npos.
Try to make it more generic
Your binary search algorithm works for plain arrays of integers, but it doesn't work for anything else. What if you want to search in an array of floats? What if you want to search in a std::vector? The standard library provides std::binary_search() which can work on a variety of containers holding any type that can be compared. It might be good practice to try to make the interface to your binary search implementation similar to that of std::binary_search(). It is not as hard as it looks! You can start by making it a template for arrays of different types:
template<typename T>
size_t binarySearch(T arr[], size_t low, size_t high, const T &n)
{
    ...
}

Once you have done that excercise, try to have it take two iterators, and return an iterator to the element if it's found, or last if not (assuming last now points to right after the end of the range to search):
template<typename It, typename T>
It binarySearch(It first, It last, const T &n)
{
    ...
}

This is a bit more advanced; you have to work within the limitations of the iterators.
Last you might want to add another template parameter, typename Comp, so you can provide a custom comparison operator.


Speeding up 3n+1 challenge I have been trying to make this code go faster by trying to write it more efficiently and I don't know what else to do. I got it to 30 seconds but I have seen 23 and 24 and I have no idea on how to do it.

public class Main {

    public static void main(String[] args) {
        long startTime = System.currentTimeMillis();

        for(long n = 1; n <= 100000000; n+=2){
            long value = cycleLength(n);
            if(value > (n*n)){
                System.out.println(n + " " + value);
            }
        }

        long endTime = System.currentTimeMillis();

        System.out.println("Runtime = " + (endTime - startTime) + " milliseconds");

    }//main

    public static long cycleLength(long n) {
        long hi = n;

        while (n > 1) {
            if((n&1)==0)  // bitwise AND
                n = n >> 1;  // n even
            else {
                n = 3*n+1; // n odd

                if(hi < n)
                    hi = n;
                n = n >> 1;
            }
        }

        return hi;
    }

}

This code runs in 2 secs vs the original 24 secs in my box, with no additional memory needed, or 0.5 secs if you don't mind using more than 1 CPU.

Going to explain how it works with an example:

Let say that "n" is 97.
At this point maxSoFar is 9232 (which is the value of cycleLength for 27, 31, 41, ...), which is the maximun value of cycleLength found for all "n" < 97.
97 * 97 is 9409 which is bigger than 9232, which means that if inside the loop of the cycleLength, "j" becomes smaller than 97, then cycleLength will be at most 9232, which is smaller than 9409 so it is not necessary to continue doing any calculations as cycleLenght will be never bigger than n * n.

So with "n" 97, n_square is 9409. We do the first loop, which means that "j" is 292. As "j" > "n", we still have hope that it can become bigger than 9409. Two cycles latter, "j" becomes 73 (292 / 2 / 2). Now we know that 73 is never going to be bigger than 9232, which means that there is no point of searching anymore and we can exit cycleLength.

Note that with this implementation cycleLength for 97 actually return 73 (instead of 9232 of your implementation) but the results are still correct.

The code is basically the same as yours but adding a condition in the cycleLength loop.

public class Main {

private static long maxSoFar = -1;

public static void main(String[] args) {
  long startTime = System.currentTimeMillis();

for (long n = 1; n <= 100000000; n += 2) {
  long value = cycleLength(n);
  if (value > (n * n)) {
    System.out.println(n + " " + value);
    maxSoFar = Math.max(value, maxSoFar);
  }
}

long endTime = System.currentTimeMillis();

System.out.println("Runtime = " + (endTime - startTime) + " milliseconds");
}//main

public static long cycleLength(long j) {
long hi = j;
long original = j;
long n_square = j * j;
while (j > 1) {
  if (j < original && maxSoFar < n_square) {
    return hi;
  }
  if ((j & 1) == 0)  // bitwise AND
  {
    j = j >> 1;  // n even
  } else {
    j = 3 * j + 1; // n odd
    if (hi < j) {
      hi = j;
    }
    j = j >> 1;
  }
}

return hi;
}

}

The multithreaded version creates a bunch of threads each one calculating a different serie of n. So for 2 threads:
Thread 1 will do 1, 5, 9, 13 ...
Thread 2 will do 3, 7, 11, 15 ...

In my box (4 cores) the optimal value for n = 10^8 is 32 threads.

import java.util.concurrent.ConcurrentHashMap;

public class Main implements Runnable {

private long maxSoFar = -1;
private int start;
private int step;
static ConcurrentHashMap results = new ConcurrentHashMap();

public Main(final int start, final int step) {
  this.start = start;
  this.step = step;
}

public static void main(String[] args) throws InterruptedException {
long startTime = System.currentTimeMillis();

Thread[] threads = new Thread[32];

for (int i = 0; i < threads.length; i++) {
  threads[i] = new Thread(new Main((i * 2) + 1, threads.length * 2));
}

for (Thread thread : threads) {
  thread.start();
}

for (Thread thread : threads) {
  thread.join();
}

System.out.println("results = " + results);

long endTime = System.currentTimeMillis();

System.out.println("Runtime = " + (endTime - startTime) + " milliseconds");

}

public void run() {
long startTime = System.currentTimeMillis();

for (long n = start; n <= 100000000; n += step) {
  long value = cycleLength(n);
  if (value > (n * n)) {
    System.out.println(Thread.currentThread().getName() + " " + n + " " + value);
    results.put(n, value);
    maxSoFar = Math.max(value, maxSoFar);
  }
}

long endTime = System.currentTimeMillis();

System.out.println("Runtime = " + Thread.currentThread().getName() + " " + (endTime - startTime) + " milliseconds");
}//main

public long cycleLength(long j) {
long hi = j;
long original = j;
long n_square = j * j;
while (j > 1) {
  if (j < original && maxSoFar < n_square) {
    return hi;
  }
  if ((j & 1) == 0)  // bitwise AND
  {
    j = j >> 1;  // n even
  } else {
    j = 3 * j + 1; // n odd
    if (hi < j) {
      hi = j;
    }
    j = j >> 1;
  }
}

return hi;
}

}


Trying to convert imperetive code to functional -Haskell I'm very new to Haskell and I was trying to implement a DFT which is very imperative into Haskell and I wanted to get a feedback. More specially how can I avoid so many helper functions and I avoid limiting to Double everywhere. Thank you.

My DFT algorithm:

 void dft(double[] inreal , double[] inimag, double[] outreal, double[] outimag) {
     int n = inreal.length;
     for (int k = 0; k < n; k++) {      // For each output element
         double sumreal = 0;
         double sumimag = 0;
         for (int t = 0; t < n; t++) {  // For each input element
             double angle = 2 * Math.PI * t * k / n;
             sumreal +=  inreal[t] * Math.cos(angle) + inimag[t] * Math.sin(angle);
             sumimag += -inreal[t] * Math.sin(angle) + inimag[t] * Math.cos(angle);
         }
         outreal[k] = sumreal;
         outimag[k] = sumimag;
     }
 }

My Haskell code:

-- Length of the array
ownLength :: [t] -> Int
ownLength [] = 0
ownLength (_: xs) = 1 + ownLength xs

dft_resolve_nested :: [((Double, Double), Double)] -> Double -> Int -> [(Double, Double)]
dft_resolve_nested [] _ _ = []
dft_resolve_nested (((x, y), t) : xs) k n = do
  let angle = 2.0 * pi * ( t) * ( k) / (fromIntegral n)
  let sumreal = x * (cos angle) + y * (sin angle)
  let sumimag = - x * (sin angle) + y * (cos angle)
  (sumreal, sumimag) : (dft_resolve_nested xs k n)

tuples_sum :: [(Double, Double)] -> (Double, Double)
tuples_sum [] = (0, 0)
tuples_sum ((x1, y1) : xs) = do
  let (x2, y2) = tuples_sum xs
  (x1 + x2, y1 + y2)

dft_resolve ::  [((Double, Double), Double)] -> [(Double, Double)]
dft_resolve [] = []
dft_resolve ls = do
  let n = ownLength ls
  let (((x, y), k) : xs) = ls
  let (xr, yr) = tuples_sum (dft_resolve_nested ls k n)
  (xr, yr) : (dft_resolve xs)

dft :: [(Double, Double)] -> [(Double, Double)]
dft [] = []
dft ls = dft_resolve (zip ls [0..])

-- Main driver
main = do
  print (dft [(1,2), (3,4)])

Why are you reimplementing length O_o?

dft_resolve_nested :: Double -> Int -> [((Double, Double), Double)] -> [(Double, Double)]
dft_resolve_nested _ _ [] = []
dft_resolve_nested k n (((x, y), t) : xs) = do
  let angle = 2.0 * pi * t * k / fromIntegral n
  let sumreal = x * cos angle + y * sin angle
  let sumimag = - x * sin angle + y * cos angle
  (sumreal, sumimag) : dft_resolve_nested k n xs

tuples_sum :: [(Double, Double)] -> (Double, Double)
tuples_sum [] = (0, 0)
tuples_sum ((x1, y1) : xs) = do
  let (x2, y2) = tuples_sum xs
  (x1 + x2, y1 + y2)

dft_resolve ::  [((Double, Double), Double)] -> [(Double, Double)]
dft_resolve [] = []
dft_resolve ls@((_, k) : xs) = do
  let (xr, yr) = tuples_sum $ dft_resolve_nested ls k $ length ls
  (xr, yr) : dft_resolve xs

-- Main driver
main = print $ dft_resolve $ zip [(1,2), (3,4)] [0..]

The explicit recursion can be done by library functions.

dft_resolve_nested :: Double -> Int -> ((Double, Double), Double) -> (Double, Double)
dft_resolve_nested k n ((x, y), t) = do
  let angle = 2.0 * pi * t * k / fromIntegral n
      sumreal = x * cos angle + y * sin angle
      sumimag = - x * sin angle + y * cos angle
  in (sumreal, sumimag)

tuples_sum :: (Double, Double) -> (Double, Double) -> (Double, Double)
tuples_sum (x1, y1) (x2, y2) = (x1 + x2, y1 + y2)

dft_resolve ::  [((Double, Double), Double)] -> (Double, Double)
dft_resolve ls@((_, k) : _) = foldr tuples_sum (0,0) $
  map (dft_resolve_nested k $ length ls) ls

-- Main driver
main = print $ map dft_resolve $ tails $ zip [(1,2), (3,4)] [0..]

Many of these names can be removed. You were also only ever restricted to Double by your own type signatures :). (You may want to tell it what Floating instance to use somewhere, though.)

import Data.NumInstances.Tuple

main :: IO ()
main = print
  [ sum
    [ (  x * cos angle + y * sin angle
      , -x * sin angle + y * cos angle
      )
    | (t, (x, y)) <- ls
    , let angle = 2 * pi * t * k / genericLength ls
    ]
  | (k, ls) <- zip [0..] $ tails [(1,2), (3,4)]
  ]

Edit: Data.Complex specializes in this sort of math:

import Data.Complex

main :: IO ()
main = print
  [ sum [z / cis angle ** t | (t, z) <- ls]
  | (k, ls) <- zip [0..] $ tails [1:+2, 3:+4]
  , let angle = 2 * pi * k / genericLength ls
  ]


Parser for a lazyloading yellowpage I've written a script to harvest Name and Phone number from yellowpage Canada under pizza category. The web page doesn't show its full content until scrolled downmost. In my crawler's every cycle, it fetches 40 records. I twitched a little in my loop which I've learnt lately to get the full content. Hope I did it the way it should be.

import requests
from lxml import html

Base_url="https://www.yellowpages.ca/search/si/{0}/pizza/Toronto"
with requests.session() as session:
    page_num=1
    while True:
        response = session.get(Base_url.format(page_num))
        if response.status_code==404:
            break
        tree = html.fromstring(response.text)
        for titles in tree.xpath("//div[@itemprop='itemListElement']"):
            try:
                title = titles.xpath(".//h3[@itemprop='name']/a/text()")[0]
            except IndexError:
                title=""
            try:
                phone= titles.xpath(".//h4[@itemprop='telephone']/text()")[0]
            except IndexError:
                phone=""
            print(title,phone)
        page_num+=1

Applying PEP8 recommendations about naming and formatting,
as well as wrapping the main logic in a main function and calling from within a if __name__ == '__main__': guard,
the code would become easier to read and possible to import and test:

import requests
from lxml import html

url_format = "https://www.yellowpages.ca/search/si/{0}/pizza/Toronto"

def main():
    with requests.session() as session:
        page_num = 1
        while True:
            response = session.get(url_format.format(page_num))
            if response.status_code == 404:
                break
            tree = html.fromstring(response.text)
            for titles in tree.xpath("//div[@itemprop='itemListElement']"):
                try:
                    title = titles.xpath(".//h3[@itemprop='name']/a/text()")[0]
                except IndexError:
                    title = ""
                try:
                    phone = titles.xpath(".//h4[@itemprop='telephone']/text()")[0]
                except IndexError:
                    phone = ""
                print(title, phone)
            page_num += 1

if __name__ == '__main__':
    main()

I'm not a huge fan of using exceptions in situations that are not really exceptions, like here:

try:
    title = titles.xpath(".//h3[@itemprop='name']/a/text()")[0]
except IndexError:
    title = ""

What if a titles.xpath(...) call would raise an IndexError due to a bug? It would be inadvertently caught and unnoticed.
Catching IndexError here is an indirect way of the real intention,
which is getting the first title if it exists.
It would be better to write the code in a way that expresses the intention directly, for example:

for item in tree.xpath("//div[@itemprop='itemListElement']"):
    title = get_first_or_empty(item, ".//h3[@itemprop='name']/a/text()")
    phone = get_first_or_empty(item, ".//h4[@itemprop='telephone']/text()")
    print(title, phone)

Where get_first_or_empty is:

def get_first_or_empty(item, xpath):
    matches = item.xpath(xpath)
    if matches:
        return matches[0]
    return ""


Perl pie alternative Running on Cygwin, I cannot easily use perl -pi -e because it will always produce .bak files even though I don't want them. I wrote this script below to emulate perl -pi -e behavior and unlink the backup files after processing.

Also I wanted to simplify the call of the script. Instead of:

$ git ls-files | xargs perl -pi -e s/foo/bar/

I rather prefer to write

$ git ls-files | pie s/foo/bar

Although this program is not aimed to be particularly used with Git, with this alias pie="!git ls-files | pie" it becomes

$ git pie s/foo/bar

I feel this script can be simplified and the performances could be slightly improved. The question is how.

#!/usr/bin/perl
use 5.010;
use strict;
use warnings;
use File::Copy;

my $re = shift @ARGV;
@ARGV  = split(/\n/, do {local $/; <stdin>}) if @ARGV eq 0;

for my $file (@ARGV) {
    next unless -f $file;
    if (-f "$file.bak") {
        warn "[Warning] Backup file already exists and is been used by pie";
    }
    open my $fh,  "<",  $file;
    open my $tmp, ">", "$file.bak";
    while (<$fh>) {
        eval($re);
    } continue { print $tmp $_ }
    close $tmp;
    close $fh;
    move "$file.bak", $file;
}

Ok, here is a solution with no additional modules, moving eval out of the loop, and with few additional checks.

use strict;
use warnings;

$^I = '.bak';
my $re = shift @ARGV or die "Nothing to do..";
$re = eval "sub{ $re }" or die "Incorrect expression..";

chomp(@ARGV  = <STDIN>) if !@ARGV;
@ARGV = grep {
  warn "[Warning] Backup file already exists and is been used by pie"
    if -f "$_$^I";
  -f $_;
}
@ARGV;

my @bak = map "$_$^I", @ARGV;
while (<>) {
  $re->();
  print;
}
unlink(@bak);


Overly complicated parser combinator I recently started to learn Haskell in order to write some small tools for me. Currently I'm working on a program which calculates how much steam a (railcraft) boiler of a given size outputs per tick.

In the console the user inputs a string of the following format: S | L | BL-Size | BH-Size, example BH-3x3x4

I then use ParserCombinators from the Text.ParserCombinators.ReadP library to parse this string into a custom type. However I think my parser is overly complicated. Any suggestions how to simplify the following code?

module Boilers (Boiler(..), Tank(..)) where

data Boiler = Hobbyist | Locomotive | Boiler (Int, Int, Int) Tank deriving (Show)
data Tank = HighPressure | LowPressure deriving (Eq, Show)

module BoilerParser where

import Data.Char
import Text.ParserCombinators.ReadP
import Control.Applicative
import Boilers
import Data.Maybe

boiler :: ReadP Boiler
boiler = do
  b <- standard <++ hobbyist <++ locomotive
  maybe pfail return b

standard :: ReadP (Maybe Boiler)
standard = do
  satisfy (\c -> toUpper c == 'B')
  t <- fmap (\t -> case toUpper t of
    'H' -> Just HighPressure
    'L' -> Just LowPressure
    _   -> Nothing) get
  satisfy (== '-')
  s <- BoilerParser.size
  case t of
    Nothing -> pfail
    (Just a) -> return (Just (Boiler s a))

hobbyist :: ReadP (Maybe Boiler)
hobbyist = fmap (\c -> if toUpper c == 'H' then Just Hobbyist else Nothing) get

locomotive :: ReadP (Maybe Boiler)
locomotive = fmap (\c -> if toUpper c == 'L' then Just Locomotive else Nothing) get

size :: ReadP (Int, Int, Int)
size = do
  x <- size' 1
  satisfy (== 'x')
  y <- size' 1
  satisfy (== 'x')
  z <- size' 1
  return (x,y,z)

size':: Int -> ReadP Int
size' ds = fmap read (count ds (satisfy isDigit))

boiler :: ReadP Boiler
boiler
  =   Hobbyist <$ char 'H'
  <|> Locomotive <$ char 'L'
  <|> flip Boiler <$ char 'B'
    <*> (HighPressure <$ char 'H' <|> LowPressure <$ char 'L')
    <*> ((,,) <$ char '-'
      <*> (read <$> munch1 isDigit) <* char 'x'
      <*> (read <$> munch1 isDigit) <* char 'x'
      <*> (read <$> munch1 isDigit))

By changing the internal definition of Boiler you can clean up that flip.

By also changing the user interface you can use the Read instance deriver:

data Boiler = H | L | BL Int Int Int | BH Int Int Int deriving (Read, Show)

Your example would then look like BH 3 3 4.

By the way, whereever you're using the Eq instance of Tank, consider pattern matching instead.


Ray reflection with a triangle I'm trying to reflect a ray of a triangle using Processing and not sure I got the code right. I'm using two bits of pseudo code from two locations:

Intersection of a Line with a Plane from Paul Bourke's site.
Reflecting a vector from 3DKingdoms site.

Here's my code so far:

PVector[] face = new PVector[3];
float ai = TWO_PI/3;//angle increment
float r  = 300;//overall radius
float ro = 150;//random offset

PVector n;//normal
Ray r1;

void setup(){
  size(500,500,P3D);
  for(int i = 0 ; i < 3; i++) face[i] = new PVector(cos(ai * i) * r + random(ro),random(-50,50),sin(ai * i) * r + random(ro));
  r1 = new Ray(new PVector(-100,-200,-300),new PVector(100,200,300));
}
void draw(){
  background(255);
  lights();
  translate(width/2, height/2,-500);
  rotateX(map(mouseY,0,height,-PI,PI));
  rotateY(map(mouseX,0,width,-PI,PI));
  //draw plane
  beginShape(TRIANGLES);
  for(PVector p : face) vertex(p.x,p.y,p.z);
  endShape();
  //normals
  PVector c = new PVector();//centroid
  for(PVector p : face) c.add(p);
  c.div(3.0);
  PVector cb = PVector.sub(face[2],face[1]);
  PVector ab = PVector.sub(face[0],face[1]);
  n = cb.cross(ab);//compute normal
  line(c.x,c.y,c.z,n.x,n.y,n.z);//draw normal

  pushStyle();
  //http://paulbourke.net/geometry/planeline/
  //line to plane intersection u = N dot ( P3 - P1 ) / N dot (P2 - P1), P = P1 + u (P2-P1), where P1,P2 are on the line and P3 is a point on the plane
  PVector P2SubP1 = PVector.sub(r1.end,r1.start);
  PVector P3SubP1 = PVector.sub(face[0],r1.start);
  float u = n.dot(P3SubP1) / n.dot(P2SubP1);
  PVector P = PVector.add(r1.start,PVector.mult(P2SubP1,u));
  strokeWeight(5);
  point(P.x,P.y,P.z);//point of ray-plane intersection

  //vector reflecting http://www.3dkingdoms.com/weekly/weekly.php?a=2
  //R = 2*(V dot N)*N - V
  //Vnew = -2*(V dot N)*N + V
  //PVector V = PVector.sub(r1.start,r1.end);
  PVector V = PVector.sub(r1.start,P);
  PVector R = PVector.sub(PVector.mult(n,3 * (V.dot(n))),V);
  strokeWeight(1);
  stroke(0,192,0);
  line(P.x,P.y,P.z,R.x,R.y,R.z);
  stroke(192,0,0);
  line(r1.start.x,r1.start.y,r1.start.z,P.x,P.y,P.z);
  stroke(0,0,192);
  line(P.x,P.y,P.z,r1.end.x,r1.end.y,r1.end.z);
  popStyle();

}
void keyPressed(){   setup();  }//reset
class Ray{
  PVector start = new PVector(),end = new PVector();
  Ray(PVector s,PVector e){   start = s ; end = e;  }
}

I've tried to keep it simple and comment it. Still, the reflected vector (drawn in green) doesn't seem to be quite right:

I'm not sure if the reflected vector should meet the normal.  Should I normalize/scale some vectors at some point?

Turns out I needed to normalize the normal first:

n.normalize();

full code for reference:

PVector[] face = new PVector[3];
float ai = TWO_PI/3;//angle increment
float r  = 300;//overall radius
float ro = 150;//random offset

PVector n;//normal
Ray r1;

void setup() {
  size(500, 500, P3D);
  for (int i = 0 ; i < 3; i++) face[i] = new PVector(cos(ai * i) * r + random(ro), random(-50, 50), sin(ai * i) * r + random(ro));
  r1 = new Ray(new PVector(-100, -200, -300), new PVector(100, 200, 300));
}
void draw() {
  background(255);
  lights();
  translate(width/2, height/2, -500);
  rotateX(map(mouseY, 0, height, -PI, PI));
  rotateY(map(mouseX, 0, width, -PI, PI));
  //draw plane
  beginShape(TRIANGLES);
  for (PVector p : face) vertex(p.x, p.y, p.z);
  endShape();
  //normals
  PVector c = new PVector();//centroid
  for (PVector p : face) c.add(p);
  c.div(3.0);
  PVector cb = PVector.sub(face[2], face[1]);
  PVector ab = PVector.sub(face[0], face[1]);
  n = cb.cross(ab);//compute normal
  n.normalize();
  line(c.x, c.y, c.z, n.x, n.y, n.z);//draw normal

  pushStyle();
  //http://paulbourke.net/geometry/planeline/
  //line to plane intersection u = N dot ( P3 - P1 ) / N dot (P2 - P1), P = P1 + u (P2-P1), where P1,P2 are on the line and P3 is a point on the plane
  PVector P2SubP1 = PVector.sub(r1.end, r1.start);
  PVector P3SubP1 = PVector.sub(face[0], r1.start);
  float u = n.dot(P3SubP1) / n.dot(P2SubP1);
  PVector P = PVector.add(r1.start, PVector.mult(P2SubP1, u));
  strokeWeight(5);
  point(P.x, P.y, P.z);//point of ray-plane intersection

  //vector reflecting http://www.3dkingdoms.com/weekly/weekly.php?a=2
  //R = 2*(V dot N)*N - V
  //Vnew = -2*(V dot N)*N + V
  //PVector V = PVector.sub(r1.start,r1.end);
  PVector V = PVector.sub(r1.start, P);
  PVector R = PVector.sub(PVector.mult(n, 2 * (V.dot(n))), V);
  strokeWeight(1);
  stroke(0, 192, 0);
  line(P.x, P.y, P.z, R.x, R.y, R.z);
  stroke(192, 0, 0);
  line(r1.start.x, r1.start.y, r1.start.z, P.x, P.y, P.z);
  stroke(0, 0, 192);
  line(P.x, P.y, P.z, r1.end.x, r1.end.y, r1.end.z);
  popStyle();
}
void keyPressed() {
  setup();
}//reset
class Ray {
  PVector start = new PVector(), end = new PVector();
  Ray(PVector s, PVector e) {
    start = s ;
    end = e;
  }
}


Compare script using awk and bash I try to create compare script using awk and another command and running successful.
But I think the script I create it's to long.
Is there anyone can shorten my script below ?
After plan A shorten the code than, plan B is :
 1. I want eliminated a lot temp file (.txt), only need `lengkap.txt`
 2. Put command in variable if can

It's my pleasure if anyone can help me
Below is the code that I create based searching and trying.
#!/bin/bash

### Path Folder who will be compare ###
path1=/home/rio/apps1
path2=/home/rio/apps2

### Find all filename and convert to MD5 ###
find $path1 -type f | xargs md5sum > checksums.md5
find $path2 -type f | xargs md5sum > checksums2.md5

### Compare to find different folder ###
awk 'NR==FNR{c[$1]++;next};c[$1] == 0' checksums.md5 checksums2.md5 > hasil1.txt
awk 'NR==FNR{c[$1]++;next};c[$1] == 0' checksums2.md5 checksums.md5 > hasil2.txt

### Merge result of compare ###
awk '{print $0}' hasil1.txt hasil2.txt > perbedaan.txt

### Filter Just Filename Difference ###
cat perbedaan.txt | awk '{print $2}' > hasilperbedaan.txt

### File about result compare (just filename) ###
cekhasil=/home/rio/hasilperbedaan.txt

### Check if File result compare empty or not ###
if [ -s "$cekhasil" ]
then
   echo " file exists and is not empty "
    ### Find All filename and date, after that put as we want ###
    find $path1 -type f -ls | awk '{print $11" "$8" "$9" "$10 }' > filedate1.txt
    find $path2 -type f -ls | awk '{print $11" "$8" "$9" "$10 }' > filedate2.txt

    ### Compare to get the date of filename ###
    awk 'A[$1]++' hasilperbedaan.txt filedate1.txt > pre_hasil1.txt
    awk 'A[$1]++' hasilperbedaan.txt filedate2.txt > pre_hasil2.txt
    ### Merge result of compare with date ###
    awk '{print $0}' pre_hasil1.txt pre_hasil2.txt > lengkap.txt
else
   echo " file does not exist, or is empty "
fi

Well, it took time to figure out what your program is doing. So I did shorten it in two phases.
First phase: I removed all duplicate temporary files, and used a pipe when a temp file was used once.
STEP1: you just make a checksum of all files in $path1 and $path2
### Find all filename and convert to MD5 ###
find $path1 -type f | xargs md5sum > checksums.md5
find $path2 -type f | xargs md5sum > checksums2.md5

You don't need 2 temp files, as path is included in filename. So you can replace this with a single find. I use sort to be able to use uniq afterwards:
find "$path1" "$path2" -type f | xargs md5sum  | sort > cksum.md5

STEP2: you find unique checksums in both files, and get the corresponding filenames.
### Compare to find different folder ###
awk 'NR==FNR{c[$1]++;next};c[$1] == 0' checksums.md5 checksums2.md5 > hasil1.txt
awk 'NR==FNR{c[$1]++;next};c[$1] == 0' checksums2.md5 checksums.md5 > hasil2.txt

### Merge result of compare ###
awk '{print $0}' hasil1.txt hasil2.txt > perbedaan.txt

### Filter Just Filename Difference ###
cat perbedaan.txt | awk '{print $2}' > hasilperbedaan.txt

As we have a checksum-sorted file, we just filter with uniq, and get filenames.
Note: As we use a MD5 checksum (128 bits), length is 128bits / 8bits * 2hex=32
uniq -u -w32 cksum.md5 | awk '{print $2}' > "$cekhasil"

If you prefer awk to uniq, just compare previous record's $1 with current's.
STEP 3: you match found entries with a new search (find) in the two source paths.
if [ -s "$cekhasil" ]
then
    echo " file exists and is not empty "
    ### Find All filename and date, after that put as we want ###
    find $path1 -type f -ls | awk '{print $11" "$8" "$9" "$10 }' > filedate1.txt
    find $path2 -type f -ls | awk '{print $11" "$8" "$9" "$10 }' > filedate2.txt

    ### Compare to get the date of filename ###
    awk 'A[$1]++' hasilperbedaan.txt filedate1.txt > pre_hasil1.txt
    awk 'A[$1]++' hasilperbedaan.txt filedate2.txt > pre_hasil2.txt
    ### Merge result of compare with date ###
    awk '{print $0}' pre_hasil1.txt pre_hasil2.txt > lengkap.txt
else
    echo " file does not exist, or is empty "
fi

Here, it is better to avoid new full find, and just loop over filenames.
if [ -s "$cekhasil" ]
then
    echo " file exists and is not empty "
    while read -r fn ; do
        ls -dils "$fn" | awk '{print $11" "$8" "$9" "$10 }'
    done < "$cekhasil" > lengkap2.txt

else
    echo " file does not exist, or is empty "
fi

Alltogether, the script becomes:
#!/bin/bash

path1=/home/br/dev/tools/bash
path2=/home/br/dev/tools/bash2
outfile=hasilperbedaan.txt

find "$path1" "$path2" -type f | xargs md5sum  | sort > cksum.md5

uniq -u -w32 cksum.md5 | awk '{print $2}' > "$outfile"

if [ -s "$outfile" ]; then
    echo " file exists and is not empty "
    while read -r fn ; do
        ls -dils "$fn" | awk '{print $11" "$8" "$9" "$10 }'
    done < "$output" > lengkap2.txt

else
    echo " file does not exist, or is empty "
fi

At this point, we can notice the 2 remaining temp files are used only once. We just get rid of them, and just pipe all together to get the output file.
    #!/bin/bash

    path1=/home/br/dev/tools/bash
    path2=/home/br/dev/tools/bash2
    outputfile="lengkap-v2.txt"

    find "$path1" "$path2" -type f | xargs md5sum  | sort |
        uniq -u -w32 | awk '{print $2}' |
        while read -r fn ; do
            ls -dils "$fn" | awk '{print $11" "$8" "$9" "$10 }'
        done > "$outputfile"
    [[ -s "$outputfile" ]] && echo " file exists and is not empty " ||
            echo " file does not exist, or is empty "

Notes:

I don't understand your second question: "Put command in variable if
can", so I skip that one.
You should never use the output of find (except with -print0 options) or ls as input of any command. But it is out of scope for this answer.


Raise a list of integers to powers and return uniques Problem (inspired by Project Euler 29):

  Consider all integer combinations of ab for X ≤ a ≤ Y
  and X ≤ b ≤ Y

  Return a sequence of distinct terms in numerical order

Input is a list of 2 integers separated by a space (" "), representing the lower and upper bounds (X Y). Input is read from txt file.
Output is a numerically sorted list of all unique numbers generated by the function (ab) , separated by space. Output is written to txt file.

Example:

Input:  2 5
Output: 4 8 9 16 25 27 32 64 81 125 243 256 625 1024 3125

And the code:

Option Explicit On
Option Strict On
Option Infer On
Option Compare Text
Imports System
Imports System.IO
Imports System.Windows.Forms
Module ProjectEuler29

    Sub Main()
        Dim myBounds = GetInput("C:\temp\square.txt")
        Dim myResults = UseBoundsToCreateList(myBounds)
        WriteToFile("C:\temp\square-answer.txt", myResults)
    End Sub

    Private Function GetInput(ByVal inputPath As String) As Integer()
        Dim returnText As String = File.ReadAllText(inputPath)
        Dim returnArray As String() = returnText.Split(New String() {}, StringSplitOptions.None)
        Return ConvertStringToIntegerArray(returnArray)
    End Function

    Private Function ConvertStringToIntegerArray(ByVal arrayOfStrings As String()) As Integer()
        Return Array.ConvertAll(arrayOfStrings, Function(str) Int32.Parse(str))
    End Function

    Private Function UseBoundsToCreateList(ByVal myBounds As Integer()) As String
        Dim numberOfIntegers = myBounds(myBounds.Length - 1) - myBounds(0) + 1
        Dim resultList As New List(Of Double)
        Dim i As Integer
        Dim myResult As Double
        For i = myBounds(0) To myBounds(0) + numberOfIntegers - 1
            For j = myBounds(0) To myBounds(0) + numberOfIntegers - 1
                myResult = i ^ j
                If Not resultList.Contains(myResult) Then resultList.Add(myResult)
            Next
        Next
        resultList.Sort()
        Return ChangeListDblToString(resultList)
    End Function

    Private Function ChangeListDblToString(ByVal resultList As List(Of Double)) As String
        Return String.Join(" ", resultList.ToArray)
    End Function

    Private Sub WriteToFile(ByVal outputPath As String, ByVal returnData As String)
        Dim fileAuthor As New StreamWriter(outputPath)
        If Not File.Exists(outputPath) Then File.CreateText(outputPath)
        fileAuthor.WriteLine(returnData)
        fileAuthor.Close()
    End Sub

End Module

Use a Hashset

This seems to be a more intuitive data structure to use since you need all of your values to be unique.

The code would change very little, but you could eliminate checking if the value is already in the List.

Use built in functions before creating your own methods

When writing to a file, you shouldn't need to check if a file doesn't exist and explicitly create it. This should happen automatically.

Use the System.IO.WriteAllText method

This also closes the file automatically, freeing you from needing to do that manually. (It basically replaces the method you wrote, so you may be able to get rid of that entirely).

You've already done similar in your GetInput function by using File.ReadAllText

Only use a one line method if it increases readability

Private Function ConvertStringToIntegerArray(ByVal arrayOfStrings As String()) As Integer()
    Return Array.ConvertAll(arrayOfStrings, Function(str) Int32.Parse(str))
End Function

The above method doesn't really have a purpose. It doesn't increase readability much since the build in method explains pretty well what it does with the name ConvertAll. Of course if you feel that your method name makes your code more human readable you should use it.


Simple program that simulates StackOverflow post I'm doing a C# tutorial by Mosh and one of the exercises is creating a program that simulates a stack overflow post upvoting/downvoting system. Very simple and short app but still wanted to know if my solution is okay and ways to improve it! I'm trying to make it as object-oriented as I possibly can, but I'm not sure I'm getting OOP correctly.

Program.cs

using System;

namespace StackOverflowPost
{
    class Program
    {
        static void Main(string[] args)
        {
            var newPost = new Post
            {
                Title = "HELLO WORLD",
                Description = "This is some awesome hello world description."
            };

            Console.WriteLine(newPost.Title);
            Console.WriteLine(newPost.Description);
            Console.WriteLine(newPost.CreationDateTime);

            newPost.UpVote();
            newPost.UpVote();
            newPost.DownVote();

            Console.WriteLine("Number of votes: " + newPost.DisplayVotes());
        }
    }
}

Post.cs

using System;

namespace StackOverflowPost
{
    class Post
    {
        public string Title { get; set; }
        public string Description { get; set; }
        public readonly DateTime CreationDateTime = DateTime.Now;
        private int _votes;

        public int DisplayVotes()
        {
            return _votes;
        }

        public void UpVote()
        {
            _votes++;
        }

        public void DownVote()
        {
            _votes--;
        }
    }
}

Just keeping a count is fine for the example app, you don't want to go mad with your implementation. Consider that in real life, you need to know who voted and when. There are various rules about when you can change your vote and when it's locked in. So although your code is fine for the simple 'show me the score' question there are some that you can't answer:

How many votes has the post had in the last day?
What is the split of up and down votes?
Am I allowed to change my vote now?
Have I already voted?

All of those questions (and more) would need to be supported by your final design. If you just wanted to support 2 for now, you could track the up and down votes separately.

As for your code, I'd split out the up and down votes and add a get only property called Score.

Worth noting, if you plan on supporting multiple regions you have to store you dates and times unambiguously. Usually, that means storing in UTC.

so

  DateTime.Now

Should have been

DateTime.UtcNow

If you just used the first, consider what would happen around any Daylight Saving Time transitions. When the clock goes back there is an entire hour of repeated local times. You won't be able to say which instant in UTC that the time corresponded to.

Edit

The minimal change I'd make to your code is:

public class Post
{
    public string Title { get; set; }
    public string Description { get; set; }

    // C#6 syntax for readonly property
    public DateTime CreationDateTime { get; } = DateTime.UtcNow;

    public int UpVoteCount { get; private set; }
    public int DownVoteCount { get; private set; }

    // C#6 expression bodied member
    public int Score => UpVoteCount - DownVoteCount;

    public void UpVote()
    {
        UpVoteCount++;
    }

    public void DownVote()
    {
        DownVoteCount++;
    }
}


PHP OOP - Does this database reading class make sense? After a whole night of work I finally have my first useful class - for reading database.

It works like a charm, but if you got a moment, please take a glance and give me feedback. Harsh criticism welcome.

Please note that security is not addressed yet, as well as usual SQL clauses, but I'd like to know am I on the right way to writing better software.

Here it is:

class DatabaseRead {
protected $dbh;                 // Database handle connection
public $columns     = array();  // Columns to be read
public $table;                  // Table to be read from
public $resultSet   = array();  // Returned results

public function __construct( $dbh, $columns, $table ) {
    $this->dbh          = $dbh;
    $this->columns      = $columns;
    $this->column       = '';
    $this->table        = $table;
    $this->resultSet    = array();

    $read_database = $this->dbh->query('SELECT ' . implode(',' , $this->columns) . ' FROM ' . $this->table);
    while ($row = $read_database->fetch()) {
        foreach($this->columns as $this->column) {
            $this->resultSet[$this->column] = $row[$this->column];
        }
    }
    return $this->resultSet;
}

}

Have I wrote an actual class that makes sense, or just a glorified function?

Thanks.

Why don't you split the "constructor" part and the "get the values" part?

Code :

class Database {

    protected $dbh;                 // Database handle connection

    public function __construct( $dbh) {
        $this->dbh          = $dbh;
    }

    public function getResults($columns,$table)
    {
        $resultSet = array();

        $read_database = $this->dbh->query('SELECT ' . implode(',' , $columns) . ' FROM ' . $table);
        while ($row = $read_database->fetch()) {
            foreach($columns as $column) {
                $resultSet[$column] = $row[$column];
            }
        }
        return $resultSet;
    }
}

Usage :

$db = new Database($dbh);
$db->getResults($columns, $table);


Irreversibly hash email addresses while preserving format/entropy I'm looking to irreversibly hash valid email addresses which have been parsed from mail log data as part of a machine learning project. I need to ensure that the processed data has been scrubbed of personally identifiable information (specific users/client domain etc).

A quick google-fu led me to pyffx and the inbuilt secrets package.

I'm looking to scrub the email addresses while retain the formatting and beginning character sequence:

#!/usr/bin/env python
# coding: utf-8

import pyffx, secrets

def ffx_encrypt(email,secret):
    raw_user, raw_domain = email.split('@')
    #retaining first few characters to test entropy of bulk sender lists
    user_chars = raw_user[:3]
    user_rem = raw_user[3:]
    #get unique characters for each string to retain entropy
    uniq_user_chars = ''.join(set(raw_user))
    uniq_dom_chars = ''.join(set(raw_domain))

    e_user = pyffx.String(secret,alphabet=uniq_user_chars,length=len(user_rem))
    e_dom = pyffx.String(secret,alphabet=uniq_dom_chars,length=len(raw_domain))

    user_encrypt = e_user.encrypt(user_rem)
    dom_encrypt = e_dom.encrypt(raw_domain)

    return user_chars + user_encrypt + '@' + dom_encrypt;

#To be generated at runtime
secret = secrets.token_hex(32).encode()

print(ffx_encrypt('test1@gmail.com',secret))
print(ffx_encrypt('firstname_surname1@mail.net',secret))
print(ffx_encrypt('username1@mail.co.uk',secret))
print(ffx_encrypt('username1@gmail.com',secret))
print(ffx_encrypt('username1@mail.net',secret))
print(ffx_encrypt('bounce-mc.uk1147123_813.721605-sue.test=mail.net@mail555.atl123.test.net',secret))

##Sample run results
#teste@limigooac
#firms_smnnueefrna_@tnmaenmi
#userersua@k.m.auuamo
#userersua@limigooac
#userersua@tnmaenmi
#bout50um7=8s_t43n07s0.6tn5knt0e366u-7c73bl3_2iio@1.eisnss5i1l32s.3.ea..3

At the moment I'm not focused on performance, elegance or robustness, it's more about avoiding stepping on a landmine if there's an obvious flaw with my implementation which could make the post-processed email addresses/domains reversible.

Feedback would be greatly appreciated.

EDIT: Clarified that input will be valid email addresses.

Test cases are very limited.  You might want to normalise equivalent addresses before hashing; for example, these addresses are all equivalent:

user@example.org
user@Example.Org
"user"@example.org

It's probably desirable that they hash to the same value.

Simply splitting on @ is naive - it's better to split on unquoted @, or more simply, just on the last @, given that DNS names don't contain @.

It's probably worth reading I Knew How To Validate An Email Address Until I Read The RFC.  After that, start looking for an email address parsing library for Python; I haven't used it, but it appears that Flanker will handle the parsing much more robustly than this code.


Merging array entries referenced by indexes in another array I have one question list (q_arr) and the user can either skip or attempt that question. In the end of last question, I display all question with their selected answer and "N/A"  if not attempted.

Attempted question list is r_arr, where q property is the question number from q_arr list (q:1 means the first element of q_arr).

    (function() {

    let r_arr = [{q:1, a:true},{q:3, a:false}];
    let q_arr = [
                { q: 'one', o: [true, false] },
                { q: 'two', o: [true, false] },
                { q: 'three', o: [true, false] },
                { q: 'four', o: [true, false] }
            ];

    //desired output: [{q:"one", a: true}, {q:"two", a: "N/A"}, {q:"three", a: false}, {q:"four", a:"N/A"}]

    let op = q_arr.filter((s, idx, arr) => {
        // console.log("s", s, idx, arr);
        let id = idx + 1;
        r_arr.map((f) => {
            if (id === f.q) {
                f.q = s.q;
            }
        });
    });

    console.log("r_arr", r_arr);
    })();

Now I again need to filter with q_arr and change to get not attempted question list and merge with this new r_arr and this seems a lengthy way.

Kindly suggest a way to solve this problem in fewer lines in ES6.

If you index the answers to the index of the questions you can do this:

let r_arr = {
            0: true,
            3: false
};
let q_arr = [
            { q: 'some question one', o: [true, false] },
            { q: 'some question two', o: [true, false] },
            { q: 'some question three', o: [true, false] },
            { q: 'some question four', o: [true, false] }
        ];

var a = q_arr.map((q, i) => ({'q': q.q, 'o': i in r_arr ? r_arr[i]  : 'na'}) )
console.log(a)

You'll need to account for the fact that js arrays are zero indexed.


Find an order with the minimum total price We want to create an application for the analysis of the order history at an online store. Each order is made by a customer and concerns the purchase of one or more items. Orders are stored in OR, an array of integers having 4 columns. The generic line OR[i] = [o, a, q, p] denotes the fact that in the order with code "o" a quantity "q" of the article with code "a" is required, sold at a unit price equal to "p" (for which obviously the total price paid for the item a in the order "o" is equal to "p * q"). An order has as many rows in OR as there are different items contained in the order.

I must write a function "ordine_min(OR)" which returns the order code with the minimum total price. In case of a tie, any of the orders with the minimum total price must be returned.

def ordine_min(OR):
    ret = []
    for i in range(len(OR)):
        if prezzo(OR,i):
            ret.append((prezzo(OR, i),i))
    return min(ret)

def prezzo(OR, i):
    prezzo_tot = 0
    for k in range(len(OR)):
        if OR[k][0] == i:
            prezzo_tot += OR[k][2] * OR[k][3]
    return prezzo_tot

The matrix is here:

OR = [[1,1,2,2],
      [1,2,3,2],
      [2,1,1,2],
      [2,4,1,3],
      [3,3,2,1],
      [3,4,2,1],
      [4,4,1,7],
      [4,5,2,1],
      [5,1,2,4],
      [5,5,1,4],
      [6,1,2,1],
      [6,2,1,3]]

Your prezzo() function is very inefficient: for every line, you scan every line in OR to for a matching o, which makes your algorithm O(n2).  To make it worse, you call it from twice from ordine_min(), doubling the work!  Ideally, the calculations should be done in one single pass through OR.

Your code is also very hard to read and understand, due to cryptic naming — OR, i, OR[k][2] * OR[k][3].  Using destructuring assignments can help improve readability.

from collections import Counter

def ordine_min(ordini):
    prezzi_neg = Counter()
    for ordine, articolo, quantità, prezzo in ordini:
        # The Counter can efficiently find the maximum total.  We calculate
        # negative prices to trick it into finding the minimum instead.
        prezzi_neg[ordine] -= quantità * prezzo
    for ordine, prezzi_neg in prezzi_neg.most_common(1):
        return -prezzi_neg, ordine


Creating tetrominoes in game A few weeks ago, I started working with Java. To get more familiar with the language, I decided to make a Tetris clone.

As I finished the Tiles beans class, I went to the Tetromino class, which is basically made of different sets of 4 Tiles objects (I'm grouping them with ArrayLists). Each Tetromino has a different colour and shape.

Would it be better practice to identify what Tetromino I'm trying to make right in the Tetromino class, or should I make a different class for each different kind of Tetromino available, probably extending the Tetromino class?

Example

(Please note that these are just design-illustrative made to represent my question)

Instead of something like:

public class Tetromino {
    private ArrayList<Tiles> tiles = new ArrayList<Tiles>();

    public Tetromino(char shape) {
        switch(shape) {
          case 'L':
             createLShapedTetr();
             break;
          /* etcetera */
        }
    }

    private void createLShapedTetr(){
     /*
      Sets color for bricks
      Adds each brick (with their X and Y coordinates) to the arrayList
     */
    }

   /* other methods, such as spinPiece() */
}

Do something like this:

public class Tetromino {
    private ArrayList<Tiles> tiles = new ArrayList<Tiles>();

     /* Tetrominoes basic methods that every Tetr does */
}

public class TetrominoShapeJ extends Tetromino {
     /* sets colour, space and spin-method in here */
}

You should only create subclasses for the different shapes if they are going to behave differently. If I were implementing it, I don't think they would behave differently, so I would not make subclasses for them. I think in my case every method would be the same, and each instance might have a different set of points with the blocks it covered. So an example method called public boolean canMoveDown might check the shapes points against any points on the game board below it, which would be the same implementation for each shape.

I don't think you should be passing a char to the constructor, though. I'd prefer an enum, or barring that, the set of points the particular shape you are creating covers, with that set kept in a factory. That keeps all of that data out of the class and the class is just responsible for processing the set of points it takes up. When coding try to follow the single responsibility principle where each class has one thing to do. This keeps everything much simpler and much easier to test and maintain.

Studies on software development have shown that systems with a large amount of inheritance are tougher to work on, tougher to maintain, and have more bugs. It's basically hidden functionality when you are looking at any class and it has a whole set of classes above it in an inheritance chain. So using inheritance for no reason is very bad.


MySQLi-extension class (parameterized queries) I've written some code today to be able to dynamically execute prepared statements/parameterized queries.

I need a fresh pair of eyes to look through my code for eventual performance gains that can be made.

I've tested the code on up to 20 parameters without problems.

For formatting I use this: {0:i}, with position of argument and type of argument

class p_mysqli extends mysqli
{
    public function __construct($host = 'localhost', $user = null, $pass = null, $db = null, $port = null, $socket = null)
    {
        @parent::__construct($host, $user, $pass, $db, $port, $socket);
    }

    public function QueryParam($sql)
    {
        // Find all parameters to be bound
        preg_match_all("/{(\d{1,}:.)}/", $sql, $matches);

        $noStripped = $matches[0];
        $sql = str_replace($noStripped, "?", $sql);
        $params = $matches[1];

        $queryTypes = Array();
        $queryParams = Array();
        $statement = $this->stmt_init();
        $statement = $this->prepare($sql);
        if(!$statement)
            die("Error on prepare: " . $this->error);

        if(count($params) > 0)
        {
            // Get params and pop SQL-query from array
            // This is also the part I'm wondering about..
            $arguments = func_get_args();
            $arguments = array_reverse($arguments);
            array_pop($arguments);
            $arguments = array_reverse($arguments);

            for($i = 0; $i < count($params); $i++)
            {
                $lp = preg_split("/:/", $params[$i]);
                $queryTypes[] = $lp[1];
                $queryParams[] = &$arguments[$lp[0]];
            }

            $query = Array();
            $query[0] = join("", $queryTypes);
            $query = array_merge($query, $queryParams);

            $stateRet = call_user_func_array(array(&$statement, 'bind_param'), $query);

            if(!$stateRet)
                die("Error on parameterbindning: " . $statement->error);
        }
        $statement->execute() or die("Error on execute: " . $statement->errno);
        $ret = $statement->get_result();
        $statement->close();
        return $ret;
    }
}

Example usage:

$indb->queryParam("SELECT COUNT(*) counted_column_test FROM users WHERE userid != {0:i} AND disabled = {2:b} AND email = {1:s}", 1, "this@email.net", false);

You use a very complicated way to extract the first element of an array.  Just use array_shift instead.

$query = array_shift($arguments);

Use foreach instead of a for loop:

foreach ($params as $param) {
    $lp = explode(":", $param); //I changed preg_split to explode, because you use a static delimiter
    $queryTypes[] = $lp[1];
    $queryParams[] = &$arguments[$lp[0]];
}


DbGeography search query I have a situation in which I need to get the closest road to a DbGeography point.

This takes 5 - 8 seconds to run in some cases.

I have a Spatial_Index on the column and I re-organised it, so it is not fragmented.

This query is quite heavy, sometimes the CPU usage raises to 100% because of this.

This is what the table looks like:

Here is the spatial result (in case it helps):

    public string GetRoad(DbGeography point)
    {
        var name = "";
        try
        {
            using (var gisEntities = new GISSpatialEntities())
            {
                var query = String.Format(@"
                            DECLARE @point geography;
                            SET @point = geography::STPointFromText('POINT({0} {1})', 4326);

                            SELECT TOP 10 * FROM GeographyLine
                            WHERE GeoLine.STIsValid() =1 AND  @point.STBuffer(200).STIntersects(GeoLine) = 1
                            ORDER BY GeoLine.STDistance(@point)", point.Longitude, point.Latitude);
                var firstOrDefault = gisEntities.GeographyLines.SqlQuery(query).FirstOrDefault();
                if (firstOrDefault != null)
                {
                    name = firstOrDefault.LineName;
                }
            }
        }
        catch (Exception ex)
        {
            var msg = string.Format("Exception in GetRoad() - {0} - Inner [{1}] \r\n", ex.Message, ex.InnerException);
            _logger.LogMessage(MessageType.Error, msg);
        }
        return name;
    }

Instead of doing SELECT TOP 10 * FROM ... you should only retrieve the columns needed which in your case is LineName.
var name = "" doesn't need to be there, if you just return firstOrDefault.LineName and for no result/exception return string.Empty.
While we are at var firstOrDefault I would like to say that using var here is just wrong because it isn't clear from the right side of the assignment what the type is. The variable name firstOrDefault isn't descriptive enough so you should consider using maybe geographyLine or foundGeographyLine.
You are selecting the TOP 10 but using FirstOrDefault. This just doesn't seem right. Using SELECT TOP 1 would be much better and faster.


Writing to a file in Golang across concurrent go routines I've been reading around how golang writes to a file, and this stack overflow question and this reddit question highlights the fact Go doesn't gurantee atomicity when writing to a file system. Although I didn't see any interleaved writes (which I guess could be due to writev(2)), I didn't want to risk it so I built a a simple Go interface to do that.

I'm not super proficient with Go, but I'd like to understand how this code can be improved with best practices and on potential issues that may arise when using it.

package main

import (
    "fmt"
    "io"
    "os"
)

// FileLogger defines the methods to log to file
type FileLogger interface {
    Write(s string)
}

type fileLogger struct {
    stream chan string
    writer io.Writer
}

func (l *fileLogger) run() {
    for {
        select {
        case s := <-l.stream:
            _, err := l.writer.Write([]byte(s))
            if err != nil {
                fmt.Println("Error writing to file: ", err.Error())
            }
        }
    }
}

func (l *fileLogger) Write(s string) {
    l.stream <- s
}

// NewFileLogger returns a new FileLogger
func NewFileLogger() FileLogger {
    file, err := os.OpenFile(
        "logs/logs.txt", os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666,
    )

    if err != nil {
        panic(err)
    }

    f := &fileLogger{make(chan string), file}
    go f.run()
    return f
}

I could for instance make it conform to the io.Writer interface, but I'm not sure what benefits there are of that?

Would there be an advantage to mutex locks here using the sync package?

Go doesn't guarantee atomicity ... so I built a simple Go interface to do that

Okay, but is your implementation atomic? The only atomic operations guaranteed to be atomic in Go are through the sync/atomic package (or things like func (*Cond) Wait under sync).

If you need true atomic write, use the atomic package. However, using log is usually sufficient.

Your implementation looks like it's trying to be concurrent, not atomic. Rather than rolling your own concurrent writer interface, use the standard API.

You're right, fmt (& os write functions, etc.) do not provide concurrency. However, the log package does.

You can see that they use mutex locks for the Output function, which is used by almost everything else.

This should perform nearly identical to your use case, because you open the file with O_APPEND, and log appends.

So open a file, and pass it to log.New().

For example:

package main

import (
    "log"
    "os"
)

func main() {
    f, err := os.OpenFile("testfile", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)

    if err != nil {
        log.Fatal(err)
    }

    logger := log.New(f, "", 0)
    logger.Output(2, "wow")
}


Using XSLT to turn content of WYSIWYG stored in XML into HTML The CMS I use uses the Xalan XSLT processor which is a XSLT version 1.0 processor. Editors who use WYSIWYG fields within the CMS can save content that will look like either:

<story>

Story Text which may have formatting.

</story>

or:

<story>Story Text which may have formatting.</story>

or even:

<story>

Story Text which may
 have formatting.

</story>

In order to handle those varying cases I've been using:

<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
<xsl:output method="html"/>
<xsl:template match="/system-index-block">
    <xsl:for-each select="story">
        <xsl:choose>

            <xsl:when test="./p|./div">

                <xsl:copy-of select="./text()|./*"/>
            </xsl:when>

            <xsl:otherwise>

                    <xsl:copy-of select="./text()|./*"/>

            </xsl:otherwise>
        </xsl:choose>
    </xsl:for-each>
</xsl:template>
</xsl:stylesheet>

Is there a better way of producing well formed HTML from this data? Is there a way to simply this?

You should change your mindset when working with XSLT: look at it as a functional language, where you declare rules for the transformation of elements. Think about it in terms of events: when a p element is found in input, I expect this and that in the output.

First, regarding the definition of your data. You should ask for (or create) a more formal description of the data. There are different formats available to describe the structure (grammar), DTD and XML Schema being the more common. There are more simple alternatives such as RELAX NG and Schematron.

My guess, from your stylesheet and your descriptions:

there is always a <system-index-block> at the top
there is a list of <story> elements in <system-index-block>
there is mixed HTML contents in <story>, with
,
,
, and text allowed

I am not sure about the last part: is it possible to have both text and div or paragraph at the top level in a story?

<story>
  Text before a
Paragraph
 in between or
Div
 even after ?
</story>

You should break your unique template rule into several template rules, one or several for each element. Then to direct the flow from rule to rule, do not use for-each but rather apply-templates:

<xsl:template match="system-index-block">

  <xsl:apply-templates />
</xsl:template>

<xsl:template match="story[p|div]">
   <xsl:apply-templates mode="copy" />
</xsl:template>

<xsl:template match="story">

    <xsl:apply-templates mode="copy" />

</xsl:template>

Note that I used the mode "copy" above: you can now create different rules for the same nodes depending on the mode, for example:

<xsl:template match="text()" />

<xsl:template match="text()" mode="copy">
    <xsl:copy />
</xsl:template>

Also, you will probably get into trouble because you only declared a namespace for the xsl prefix. You typically need a prefix/namespace declaration for the input and output formats. You will then need to use the prefix in match/select expressions and in the tag names for the output, for example, with input prefix bound to your input namespace and html bound to the html namespace:

<xsl:template match="input:story">

    <xsl:apply-templates mode="copy" />

</xsl:template>

To go further, here are pointers that you will probably find useful:

Namespaces and XSLT Stylesheets
Identity transformation


List with no sequential repeats (where possible) Assume I have the following list (List<string>)

G
G
M
T

I'd like this to be in an order where the same letter will not occur twice, such as

G
M
G
T

(I appreciate other variations would also suffice)

Please note, if I had the following

G
G
G
G
G
M
T

Then of course some values will have to occur concurrently. The position within the List  where this concurrency occurs is not relevant. What I'm after is to stop this repetition where possible to most evenly spread the letters.

The C# console application code I share works

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace resorting
{
    class Program
    {
        static void Main(string[] args)
        {
            List<string> list = new List<string> { "G", "G", "G", "G", "M", "M", "T", "T", "T", "T", "T", "T", "T", "T", "G", "G", "G", "G", "M", "M", "T", "T", "T", "T", "T", "T", "T", "T" };

            list.UpdateList();
        }
    }

    public static class extension
    {
        public static void UpdateList(this List<string> list)
        {
            GetList(list);
            list.Reverse(); // DO I HAVE TO REVERSE
            GetList(list);  // AND THEN CALL THE METHOD AGAIN
        }

        private static void GetList(List<string> list)
        {
            int total = list.Count;

            int inc = 2;

            for (int i = 0; i < total; i++)
            {
                if (total <= i + 2)
                    break;

                if (total <= i + inc)
                    continue;

                if (list[i] == list[i + 1])
                {
                    var val = list[i];
                    list.RemoveAt(i);
                    list.Insert(i + inc, val);
                    i = i - 1;
                    inc++;
                    continue;
                }
                inc = 2;
            }
        }
    }
}

As you can see, I have to call this function twice. I reverse the order and then reapply the same logic. Whilst it works, and based upon the amount of data I will be using, I don't need the function to be more efficient  my question is still about efficiency (if worth while against the amount
of work required).

Can I update the code to not need to reverse the list to avoid calling the code twice?

Here's an alternate way to do it. It doesn't involve modifying and reversing the existing list. Instead, it first groups and counts the items, then yields the one that wasn't printed last time and still needs to be yielded the most times. I'm not sure about actual runtime for large lists, but as long as the number of distinct items in the list is small, the reorderings and searches should run very fast.

public static IEnumerable<T> WithoutDupes<T>(this IEnumerable<T> source) where T : IEquatable<T>
{
    var sourceGroups = source.GroupBy(x => x).Select(x => new GroupCount<T>(x)).ToList();
    T last = default(T);
    bool hasLast = false;
    while (sourceGroups.Count > 0)
    {
        sourceGroups = sourceGroups.OrderByDescending(x => x.Count - x.CountOutputted).ToList();
        var group = (hasLast ? sourceGroups.FirstOrDefault(x => !last.Equals(x.Value)) : null) ?? sourceGroups[0];

        yield return group.Value;
        group.CountOutputted++;
        if (group.CountOutputted == group.Count)
        {
            sourceGroups.Remove(group);
        }
        last = group.Value;
        hasLast = true;
    }
}
private class GroupCount<T>
{
    public T Value { get; private set; }
    public int Count { get; private set; }
    public int CountOutputted { get; set; }
    public GroupCount(IGrouping<T, T> grouping)
    {
        this.Value = grouping.Key;
        this.Count = grouping.Count();
    }
}

As example output, with your input, it produces this list (note how it handles the impossible case: puts the extra T's at the end)

T, G, T, G, T, G, T, G, T, G, T, M, T, M, T, G, T, G, T, M, T, M, T, G, T, T, T, T


Base adder, given base and two numbers of that base, it adds them Nothing more to add than the title. Looking for code review, optimizations and best practices.

public final class BaseAdder {

    private BaseAdder() { }

    /**
     * Given two numbers and their bases, adds the number
     * and returns the result in the same base.
     *
     * This code supports max-base 26.
     *
     *
     * @param num1  the first number to be added
     * @param num2  the second number to be added
     * @param base  the base of the numbers
     */
    public static String add (String num1, String num2, int base) {

        if (base <  2) throw new IllegalArgumentException("The base should at least be 2. Input base is: " + base);
        if (base > 26) throw new IllegalArgumentException("The base should be less than 26. Input base is: " + base);

        /*
         * http://stackoverflow.com/questions/355089/stringbuilder-and-stringbuffer-in-java
         * and StringBuilder is intended as a drop in replacement for StringBuffer where synchronisation is not required –  Joel Dec 23 '09 at 8:52
         *
         */
        final StringBuilder stringBuilder = new StringBuilder();
        int carry = 0;
        for (int i = num1.length() - 1; i >= 0; i--) {
            int x =  getIntValue(num1.charAt(i), base) +   getIntValue(num2.charAt(i), base) + carry;

            if (x >= base) {
                carry = 1;
                x = x - base;
            } else {
                carry = 0;
            }
            stringBuilder.append(getCharValue(x, base));
        }

        if (carry == 1) stringBuilder.append(1);

        return stringBuilder.reverse().toString();
    }

    public static int getIntValue(char ch, int base) {
        if (ch > getCharValue(base - 1, base)) {
            throw new IllegalArgumentException(" invalid character " + ch + " for input base " + base);
        }

        if (ch >= '0' &&  ch <= '9') {
            return ch - '0';
        }
        Character.toUpperCase(ch);
        return ch - 'A' + 10;
    }

    public static char getCharValue(int x, int base) {
        if (x >= base) {
            throw new IllegalArgumentException(" invalid number " + x + " for input base " + base);
        }

        if (x >= 0 &&  x <= 9) {
            return (char) (x + '0');
        }

        return (char)(x + 'A' - 10);
    }

public static void main(String[] args) {
    assertEquals("100", BaseAdder.add("10", "10", 2));
    assertEquals("11", BaseAdder.add("10", "01", 2));

    assertEquals("25", BaseAdder.add("10", "15", 10));

    assertEquals("4A", BaseAdder.add("1C", "2D", 15));
    assertEquals("1B9", BaseAdder.add("CC", "ED", 16));

    assertEquals("1B", BaseAdder.add("C", "D", 14));
    assertEquals("1A", BaseAdder.add("C", "D", 15));
    assertEquals("19", BaseAdder.add("C", "D", 16));
}
}

The limit of the maximum radix of 26 sounds odd, why not use the full alphabet of characters up to 10 digits, and 26 letters, giving a total of radix-36?

You have not included a reinventing-the-wheel tag, and thus I can suggest the following:

public static String add (String num1, String num2, int base) {
    if (base > 26) {
       // arbitrary limitation to 26... BigInt supports to 36.
    }
    BigInteger bione = new BigInteger(num1, base);
    BigInteger bitwo = new BigInteger(num2, base);
    return bione.add(bitwo).toString(base);
}

I believe that satisfies the review, optimization and best-practices request.


How to Avoid Conditional Check Duplication In the code, I am checking altPhoneExist and altEmailExist conditions multiple times. How can I avoid this and improve the performance and readability of the code?
Code Description:
A contact is created and added to a list if one of the conditions is true. And a different set operation executed for each one.
boolean altPhoneExist = contact.getAlternativePhoneNumber() != null;
boolean altEmailExist = contact.getAlternativeEmail() != null;

if (altPhoneExist || altEmailExist) {

    if (altPhoneExist) {
        contact.setPhoneNumber(contact.getAlternativePhoneNumber());
    }

    if (altEmailExist) {
        contact.setEmail(contact.getAlternativeEmail());
    }

    rqType.getContactInfoList().add(CraneFactory.createContactPerson(contact));
}

In my opinion, there are not other good ways that I see to avoid this kind of logic; you can attenuate the arrow code by inverting the first condition and returning early.
if (!altPhoneExist && !altEmailExist) {
   return;
}

// Here we know it's either one of them

if (altPhoneExist) {
   contact.setPhoneNumber(contact.getAlternativePhoneNumber());
}

if (altEmailExist) {
   contact.setEmail(contact.getAlternativeEmail());
}

rqType.getContactInfoList().add(CraneFactory.createContactPerson(contact));


Arrays, inheritance, generics I had a requirement where I need to create object using same interface but one derived class expects simple data and other array of type. Here is solution I came up with:

I had two questions:

As you can see I am converting IEquatable<T> into T. In my testing so far I did not see any disasters. Can you think of any from top off your head?
Is there a way I can change constraint in ArrayExpectant::CloneFromMe to ArrayWrapper instead of IEquatable?

public class ArrayWrapper<T> : IEquatable<T>
{
    public T[] Array { get; set; }

    public bool Equals(T other) { return false;  }
    public bool Equals(T[] other) { return Array == other; }
    public bool Equals(ArrayWrapper<T> other) { return Array == other.Array; }
}

public interface ICreationByCloning
{
    ICreated CloneFromMe<TClone>(IEquatable<TClone> t) where TClone : IEquatable<TClone>;
}

public interface ICreated { }

public class Simpleton<T> : ICreated, ICreationByCloning
{
    public Simpleton(T t) { }
    public ICreated CloneFromMe<TClone>(IEquatable<TClone> t) where TClone : IEquatable<TClone>
    {
        return new Simpleton<TClone>((TClone)t);
    }
}

public class ArrayExpectant<T> : ICreated, ICreationByCloning where T : IEquatable<T>
{

    T[] Data { get; set; }
    public ArrayExpectant(T[] t) { Data = t; }
    public ICreated CloneFromMe<TClone>(IEquatable<TClone> t) where TClone : IEquatable<TClone>
    {
        TClone[] array = (t as ArrayWrapper<TClone>).Array;

        return new ArrayExpectant<TClone>(array);
    }
}

I'm not completely sure what are you trying to accomplish, but I think this is bad and very confusing code.

You seem to be using the interface IEquatable<T> to mean “T, or a type that has some relation to T”. That's not what it's meant for at all!

Also, the point of interfaces is to be able to use different types in the same way. For example, if I have a IEnumerable<string>, I know I can iterate over it using a foreach and process the strings it gives me.

But that does not apply to your interface. I have to know whether I have have Simpleton<T> (I have to pass T to it) or ArrayExpectant<T> (I have to pass ArrayWrapper<T> to it).

Because of this, I would suggest you to drop the interface completely, I don't see any benefit of having it.


Create a unique four digit code from 35 characters I want to generate a four digit code based on the code generated before it, starting at 0000 and going to zzzz.

These are the available characters to be inside the code:

private static final char[] _availableChar = "0123456789abcdefghijklmnopqrstuvwxyz".toCharArray();

The first thing I do is the get the newest code or the code with the highest auto_incremented Id.

I then pass that to this function to generate the next cod:

private String getNewCodeFromOldCode(String oldCode) {
        char[] newCodeChars = oldCode.toCharArray();

        if (oldCode == null || oldCode.isEmpty()) {
            return "0000";
        } else {
            char[] oldCodeChars = oldCode.toCharArray();

            if (oldCodeChars[3] != 'z') {
                newCodeChars[3] = _availableChar[String.valueOf(_availableChar).indexOf(oldCodeChars[3]) + 1];
            } else {

                if (oldCodeChars[2] != 'z') {
                    newCodeChars[3] = '0';
                    newCodeChars[2] = _availableChar[String.valueOf(_availableChar).indexOf(oldCodeChars[2]) + 1];
                } else {

                    if (oldCodeChars[1] != 'z') {
                        newCodeChars[2] = '0';
                        newCodeChars[1] = _availableChar[String.valueOf(_availableChar).indexOf(oldCodeChars[1]) + 1];
                    } else {

                        if (oldCodeChars[0] != 'z') {
                            newCodeChars[1] = '0';
                            newCodeChars[0] = _availableChar[String.valueOf(_availableChar).indexOf(oldCodeChars[0]) + 1];
                        } else {
                            JsfUtil.addErrorMessage("Unable To Generate A Code");
                        }

                    }

                }

            }
        }

        return String.valueOf(newCodeChars);
    }

I'm not exactly sure how to calculate the permutations but I think I have upwards of million different perm. available.

By my count, 0-9 + a-z gives 36 characters rather than 35.

For four digits like that, the number of combinations is \$36^4\$, which works out to 1,679,616.

As for the code itself, I think I'd do things just a little differently. What you really have is a 4-digit number in base 36. It's probably easiest to just use a normal number, and then use toString to generate the base-36 representation when you need it:

Integer counter;
String result = toString(counter, 36);

This does differ in one minor detail: it won't fill in leading zeros, so if it's shorter than 4 characters, you'll have to fill in the leading zeros yourself (still a pretty trivial operation though).

For the sake of security, I don't install Java on my machines, but there's a rough imitation of the basic idea, but in C++:

#include <iostream>
#include <string>
#include <deque>
#include <cassert>

// This should be approximately equivalent to what you'd get from
// Java's `toString`, at least for non-negative numbers:
std::string toString(unsigned value, unsigned base) {
    static const char digits[] = "0123456789abcdefghijklmnopqrstuvwxyz";

    assert(base <= 36);

    std::deque<char> ret;

    while (value) {
        unsigned digit = value % base;
        value /= base;
        ret.push_front(digits[digit]);
    }
    return std::string(ret.begin(), ret.end());
}

// This calls toString, then pads the result out to 4 digits.
std::string paddedToString(unsigned value, unsigned base, unsigned num_digits) {
    std::string s = toString(value, base);

    // Add enough 0's to get to the desired width:
    return std::string(num_digits - s.size(), '0') + s;
}

// A quick test of the code.
int main() {
    // First we'll write the first few values:
    for (unsigned value = 0; value < 256; value++)
        std::cout << paddedToString(value, 36, 4) << "\t";

    std::cout << "\n\n";

    // Then some values from around the middle of the range:
    unsigned base = 512 * 1024;

    for (unsigned i = 0; i < 256; i++)
        std::cout << paddedToString(i + base, 36, 4) << "\t";
}


Parse proxy strings to array of objects The goal of this module is to take in a string (user input, someone copy and pasting a large list of proxies) and parse it into an array of objects.
I appreciate any and all feedback, but I'm specifically interested in improving the stringToProxy function.
Namely I'm unsure about mutating the string parameter, seems like there could be a better solution. And the way I'm declaring the proxy at the beginning of the function feels wrong to me (using As) but I couldn't think of any better solution. I also feel as though I may be overusing string.split().
Thanks for your time!
Edit:
Incorporated the url module as Nazar recommended. I'm happy with this solution but still taking feedback.
const { URL } = require("url");

interface Proxy {
    host: string;
    port: number;
    protocol: string;
    auth?: {
        username: string;
        password: string;
    };
}

function parse(string: string): Proxy[] {
    const rawArray = stringToArray(string);
    const fixedArray = rawArray.map((string) => fixProxyString(string));
    const proxyArray = fixedArray.map((string) => stringToProxy(string));
    return proxyArray;
}

function stringToArray(string: string): string[] {
    return string.trim().replace(/[ ,\n]+/g, ",").split(",");
}

function fixProxyString(string: string): string {
    return string.includes("://") ? string : `http://${string}`; //if user doesnt specify a protocol, default to http
}

function stringToProxy(string: string): Proxy {
    const url = new URL(string);
    return {
        host: url.hostname,
        port: url.port ? parseInt(url.port) : 80, //if proxy is 80 url will ignore it so i need to explicitly add it
        protocol: url.protocol.slice(0, -1), //removing the colon
        auth: url.username ? { username: url.username, password: url.password } : undefined
    };
}

export = parse;

Before edit:
interface Proxy {
    host: string;
    port: number;
    auth?: {
        username: string;
        password: string;
    };
    protocol?: string;
}

function parse(string: string): Proxy[] {
    const rawArray = stringToArray(string);
    const proxyArray = rawArray.map((string) => stringToProxy(string));
    return proxyArray;
}

function stringToArray(string: string): string[] {
    return string.trim().replace(/[ ,\n]+/g, ",").split(",");
}

function stringToProxy(string: string) {
    const proxy = {} as Proxy;

    if (string.includes("://")) {
        const [protocol, remainder] = string.split("://");
        proxy.protocol = protocol;
        string = remainder;
    }
    if (string.includes("@")) {
        const [auth, remainder] = string.split("@");
        const [username, password] = auth.split(":");
        proxy.auth = { username, password };
        string = remainder;
    }
    const [host, port] = string.split(":");
    proxy.port = parseInt(port);
    proxy.host = host;

    return proxy;
}

export = parse;

Usage:
const parse = require("parse-proxy");

parse(`1.1.1.1:80, 2.2.2.2:80, 3.3.3.3:80`)

//result
[
  { host: '1.1.1.1', port: 80 },
  { host: '2.2.2.2', port: 80 },
  { host: '3.3.3.3', port: 80 }
]

parse(`https://user:pass@104.236.55.48:8080\nhttps://user:pass@213.105.29.14:3128`)

//result
[
  {
    protocol: 'https',
    auth: { username: 'user', password: 'pass' },
    host: '104.236.55.48',
    port: 8080
  },
  {
    protocol: 'https',
    auth: { username: 'user', password: 'pass' },
    host: '213.105.29.14',
    port: 3128
  }
]

A couple of small comments:
Don't call your parameter the same thing as its type string: string. This makes the code needlessly difficult to read - don't make the reader waste time wondering whether this string a type or the variable. Use input, source or something else that makes sense.
Don't shadow outer variables/arguments in arrow functions, again this harms readability rawArray.map((string) => fixProxyString(string));. Note that the parenthesis are optional for one parameter too. Consider instead:
function parse(input: string): Proxy[] {
    const rawArray = stringToArray(input);
    const fixedArray = rawArray.map(raw => fixProxyString(raw));
    const proxyArray = fixedArray.map(fixed => stringToProxy(fixed));
    return proxyArray;
}

I think (although not tested) that you can further simplify:
function parse(input: string): Proxy[] {
    return stringToArray(input)
        .map(fixProxyString)
        .map(stringToProxy);
}

You don't need to replace and then split here return string.trim().replace(/[ ,\n]+/g, ",").split(",");. You can just split by the regex directly.


Calculate mean, median, and mode from list of integers I am trying to solve a problem put in The Rust Programming Language #2

  Given a list of integers, use a vector and return the mean (average), median (when sorted, the value in the middle position), and mode (the value that occurs most often; a hash map will be helpful here) of the list.

I decided to put the values in a simple struct and return a Result. I want to be sure I'm using all the language features within the calculations and I want to reduce the amount of casts if possible.

use std::collections::HashMap;

/// structure to represent the values after calculating the mean,
/// median, and mode of some integers.
/// the mode Vec holds each individual mode.
#[derive(Debug)]
pub struct Calculations {
    mean: f64,
    median: f64,
    mode: Vec<i32>,
}

/// calculates the mean, median, and mode of values.
/// returns a Result holding a Calculations or an error string
/// when given an empty slice
pub fn calculate_mmm(values: &[i32]) -> Result<Calculations, &str> {
    // if input is an empty slice, return an error message
    if values.is_empty() { return Err("Found empty slice") }

    // add all numbers together and divide by length
    let mean = values.iter().fold(0, |p, &q| p + q) as f64 / values.len() as f64;

    // sort all values and return middle element or average of middle 2 elements
    let median = {
        let mut sorted = values.to_vec();
        sorted.sort();

        if sorted.len() % 2 == 0 {
            (sorted[sorted.len() / 2] + sorted[(sorted.len() / 2) + 1]) as f64 / 2.0
        } else {
            sorted[(sorted.len() as f64 / 2.0) as usize] as f64
        }
    };

    // holds modes
    let mut mode: Vec<i32> = Vec::new();

    // holds each number and the number of times it occures
    let mut occurrences = HashMap::new();

    for i in values {
        let count = occurrences.entry(i).or_insert(0);
        *count += 1;
    }

    // the maximum times a value occurres
    let mut max_value = 0;

    for &value in occurrences.values() {
        if value > max_value {
            max_value = value;
        }
    }

    // find the numbers which occur the maximum number of times
    for (key, value) in occurrences {
        if value == max_value {
            mode.push(*key);
        }
    }

    // return all values
    Ok(
        Calculations { mean: mean as f64, median: median as f64, mode: mode }
    )
}

There's no main method and more importantly, there are no tests. How do you know if the code works?
It's probably an artifact of learning a new language, but the code is overly documented. There's no reason to have comments describing what the implementation of the code is. It's more important to describe why a given implementation was chosen.
I would break out smaller functions to have somewhere for names to hang off of.
Documentation aspects:

"returns a ..." in docs is not useful; the return type already says that.
It is good to say why an error can occur though. You can also create a named error type that might have a variant like Error::EmptySlice, which is more self-describing.
"structure to ..." is docs is not useful; it is a struct.
"represent" is all programs do, so is redundant.
Avoid referring to concrete implementation details ("... is a Vec ...")

You have a good use of fold, but sum exists.
Median is the mode of the middle of the sorted vector. Now that there are functions, you can reuse them.
Why convert to a floating point, divide, then back to a usize? (sorted.len() as f64 / 2.0) as usize)
Don't declare the type of mode; let the compiler's type inference handle it.
Use Iterator::filter instead of the if in the loop.
Use Iterator::map instead of the manual destructuring in the for loop for (key, value).
Use Iterator::collect to create the Vec instead of creating it and pushing to a Vec.
I'd oneline the counting with the HashMap.
Use Iterator::max and unwrap_or to find the biggest value instead of doing it by hand.
You could track the maximum value during the first loop. This would save one iteration, but it's not as pretty.
The casts to f64 wen creating the structure are redundant; the values are already f64.

use std::collections::HashMap;

/// The mean, median, and mode of some integers.
#[derive(Debug)]
pub struct Calculations {
    mean: f64,
    median: f64,
    mode: Vec<i32>,
}

fn mean(values: &[i32]) -> f64 {
    let sum: i32 = values.iter().sum();
    sum as f64 / values.len() as f64
}

/// Returns the middle element of the elements in sorted order (or the
/// average of the middle 2 elements)
fn median(values: &[i32]) -> f64 {
    let mut sorted = values.to_vec();
    sorted.sort();

    let mid = sorted.len() / 2;

    if sorted.len() % 2 == 0 {
        mean(&sorted[(mid - 1)..(mid + 1)])
    } else {
        sorted[mid] as f64
    }
}

fn mode(values: &[i32]) -> Vec<i32> {
    let mut occurrences = HashMap::new();

    for i in values {
        *occurrences.entry(i).or_insert(0) += 1;
    }

    let max_value = occurrences.values().cloned().max().unwrap_or(0);

    occurrences.into_iter()
        .filter(|&(_, v)| v == max_value)
        .map(|(&k, _)| k)
        .collect()
}

/// Calculates the mean, median, and mode of values.
/// It is an error to pass an empty slice
pub fn calculate_mmm(values: &[i32]) -> Result<Calculations, &str> {
    if values.is_empty() {
        Err("Found empty slice")
    } else {
        Ok(Calculations {
            mean: mean(values),
            median: median(values),
            mode: mode(values),
        })
    }
}

fn main() {
    println!("{:?}", calculate_mmm(&[3, 2, 1, 2]));
    println!("{:?}", calculate_mmm(&[3, 2, 1]));
}


Creating a filtered list by comparing one list to another other list - Python I have a list and I need to get certain elements out of it. The way I did it was to compare the second list to the first list and then append to a third list. Here is my code:
l1 = ["0 = 0","1 = 1","2 = 2", "And",
      "Something", "Wicked", "This", "Way", "Comes",
      f"zero.0 = 0", f"one.0 = 1", f"two.0 = 2",
      f"zero.1 = 0", f"one.1 = 1", f"two.1 = 2",
      f"zero.2 = 0", f"one.2 = 1", f"two.2 = 2",
      "purple", "monkey", "dishwasher"]
l3 = []
for element in l1:
   for i in range(3):
       l2 = [f"zero.{i} = 0", f"one.{i} = 1", f"two.{i} = 2"]
       for j in l2:
           if j in element:
               l3.append(element)

The results of this nested for loop is:
['zero.0 = 0', 'one.0 = 1', 'two.0 = 2', 'zero.1 = 0', 'one.1 = 1', 'two.1 = 2', 'zero.2 = 0', 'one.2 = 1', 'two.2 = 2']

The result is exactly what I want but I'm looking for a more Pythonic way of writing this. Any help would be greatly appreciated.

Unnecessary format strings
Unless you intend to format a string right then and there, you don't need an f before it.
Regex
This can be reduced to one line using regex:
import re

l3 = [item for item in l1 if re.match(r"\w{3,5}[.]\d\s[=]\s\d{1,}", item)]

Explanation

\w{3,5} matches any word character (equal to [a-zA-Z0-9_])

{3,5} Quantifier — Matches between 3 and 5 times, as many times as possible, giving back as needed (greedy)

Match a single character present in the list below [.]

. matches the character . literally (case sensitive)

\d matches a digit (equal to [0-9])

\s matches any whitespace character (equal to [\r\n\t\f\v ])

Match a single character present in the list below [=]

= matches the character = literally (case sensitive)

\s matches any whitespace character (equal to [\r\n\t\f\v ])

\d{1,} matches a digit (equal to [0-9])

{1,} Quantifier — Matches between one and unlimited times, as many times as possible, giving back as needed (greedy)