How to reduce code from different alert-dialogue? I have three types alert dialogue. Each alert dialogue behave differently but I don't like to write three types of alert dialogue in same class because it extends the class size drastically. Here are the three alert dialogue given below:

public boolean onJsAlert(WebView view, String url, String message,
        final android.webkit.JsResult result) {
    new AlertDialog.Builder(currentActivity)
            .setTitle("Alert !")
            .setMessage(message)
            .setPositiveButton(android.R.string.ok,
                    new AlertDialog.OnClickListener() {
                        public void onClick(DialogInterface dialog,
                                int which) {
                            result.confirm();
                        }
                    }).setCancelable(false).create().show();

    return true;
}

@Override
public boolean onJsConfirm(WebView view, String url, String message,
        final android.webkit.JsResult result) {
    new AlertDialog.Builder(currentActivity)
            .setTitle("Confirmation")
            .setMessage(message)
            .setPositiveButton(android.R.string.yes,
                    new AlertDialog.OnClickListener() {
                        public void onClick(DialogInterface dialog,
                                int which) {
                            result.confirm();
                        }
                    })
            .setNegativeButton(android.R.string.no,
                    new AlertDialog.OnClickListener() {
                        public void onClick(DialogInterface dialog,
                                int which) {
                            result.cancel();
                        }
                    }).setCancelable(false).create().show();

    return true;
}

And the last alert dialogue is:

public void onGeolocationPermissionsShowPrompt(final String origin,
        final GeolocationPermissions.Callback callback) {

    final boolean remember = true;

    AlertDialog.Builder builder = new AlertDialog.Builder(
            WebviewActivity.this);
    builder.setTitle("Locations");
    builder.setMessage(" Would like to use your Current Location")
            .setCancelable(true)
            .setPositiveButton("Allow",
                    new DialogInterface.OnClickListener() {
                        @Override
                        public void onClick(DialogInterface dialog,
                                int id) {
                            callback.invoke(origin, true, remember);

                            SharedPreferences pref = currentActivity
                                    .getPreferences(currentActivity.MODE_PRIVATE);
                            SharedPreferences.Editor editor = pref
                                    .edit();
                            editor.putBoolean("isLocationAvailable",
                                    true);
                            editor.commit();

                            webview.loadUrl(getUrl(gps.getLatitude(),
                                    gps.getLongitude(), "true"));
                        }
                    })
            .setNegativeButton("Don't Allow",
                    new DialogInterface.OnClickListener() {
                        @Override
                        public void onClick(DialogInterface dialog,
                                int id) {
                            callback.invoke(origin, false, remember);
                            webview.loadUrl(getUrl("", "", "false"));

                        }
                    });
    AlertDialog alert = builder.create();
    alert.show();
}

How can I reduce the code size. Any suggestion please?

I also like to keep classes short, but I don't think it should influence your design.  You can put the alert building code in another class file if you really want to.

I would not try to coerce the geolocation alert with onJsAlert/onJsConfirm since it is quite different.  But you could do something if you really wanted to reuse some code for those two (onJsAlert/onJsConfirm).  The positive OnClickListener could be defined outside and shared for onJsAlert/onJsConfirm.  You could instead write a common method for building those two alert dialogs, with an if-statement that checks if a negative button should be added or not.


How to make my save/load to/from file function more canonical? I  started to learn Kotlin and am now looking for common ways to save object to file and then load an object from a file. In my case, I am saving map<String, List<Int>> and have feeling that my approach is not good enough.

fun main(args: Array<String>) {
//    CreateDs.createLabeledList()
    val map = mutableMapOf<String, List<Int>>()
    map.put("first", listOf(1, 2, 3))
    map.put("second", listOf(4, 5, 6))
    val f = File("labels.txt")
    save(f, map)
    System.out.println(map)
    map.clear()
    load(f, map)
    System.out.println(map)

}

fun save(f: File, map: Map<String, List<Int>>) {
    f.printWriter().use { out ->
        map.forEach {
            out.println("${it.key}:${it.value.toString().replace("[", "").replace("]", "").replace(" ", "")}")
        }
    }
}

fun load(f: File, map: MutableMap<String, List<Int>>) {
    f.inputStream().bufferedReader().useLines { lines ->
        lines.forEach {
            val parts = it.split(":")
            map.put(parts[0], parts[1].split(",").map { it.toInt() })
        }
    }
}

Converting a list of Ints to a comma separated string

map.forEach {
    out.println("${it.key}:${it.value.toString().replace("[", "").replace("]", "").replace(" ", "")}")
}

This could be refactored as:

map.map { (key, value) ->
    key + ":" + value.joinToString(",") { it.toString() }
}.forEach(out::println)

Loading data into the map

val map = mutableMapOf<String, List<Int>>()
map.put("first", listOf(1, 2, 3))
map.put("second", listOf(4, 5, 6))

This can be rewitten to use mapOf, which increases readability

val map = mutableMapOf(
        "first" to listOf(1, 2, 3),
        "second" to listOf(4, 5, 6)
)

Destructuring declaration for splitting string

val parts = it.split(":")
map.put(parts[0], parts[1].split(",").map { it.toInt() })

Using a destructuring declaration may make the purpose of this code clearer.

val (key, values) = "a".split(":")
map.put(key, values.split(",").map { it.toInt() })


Singleton Database wrapper Before everybody yells at me about why I wrote a PDO wrapper class, it's to avoid writing try/catch, prepare(), execute(), etc. each time a query should be executed.

I have pretty big website, so instead of repeating:

$sth =  $dbh->prepare('Some query');
$dbh->BindParam('x', $x);
$dbh->BindParam('y', $y);
 //..etc
$sth->execute();

a few thousands times I can simply do:

$res = $dbh->query('Some query', $x1, $y1, //etc);
$res->fetch(\PDO::FETCH_OBJ)

And also because I want to do some operations on each query like count queries, execution time, etc.

Is this Singleton wrapper really bad or overkill?

namespace App\Database {

class Database
{

 static private $PDOObject;

 function __construct()
 {
     if(!self::$PDOObject) {

        try{

            self::$PDOObject = new \PDO('mysql:host=127.0.0.1;dbname=test', 'username', 'pentagonpassword');

        }
        catch(\PDOException $e) {
             //handle exception
             throw $e;
        }
        return self::$PDOObject;

      }
 }

 public function query($query, ...$params)
 {

   try
   {
    //start timer and other stuff (count queries etc)
    $sth = self::$PDOObject->prepare($query);
    $sth->execute($params);
    //end timer
    return $sth;
    }
    catch(\PDOException $e)
    {
       throw $e;
    }

 }

 public function lastid($name = NULL)
 {
   return self::$PDOObject->LastInsertId($name);
 }

  //Other methods that processes some complex stuff

}
}

Singleton

I wouldn't call your Database class a singleton wrapper, as you can have multiple instances of it (not of the PDOObject field, but of the Database class).

See here and here for actual singleton pattern.

I don't think that your approach is necessarily bad though.

Is this overkill?

I think that you are a bit dishonest in your example. Your not actually using BindParam in your code, and you would still have to catch the exception that is thrown. So instead of saving multiple lines per query as you suggest, you are actually only saving one, which isn't that much.

But doing it this way does provide you the ability to easily add functionality to executing a query, or change how you execute queries all together. I think that this is a big advantage and a good case for having this class.

Returning in the constructor

  return self::$PDOObject;

Why are you returning the PDO instance in the constructor? This seems a little odd.

Style

I say this a lot, but good style actually is important and leads to easier to read code.

remove all unnecessary newlines. You have a lot of newlines, which makes your code hard to read
be consistent with your curly brackets and indentation (always indent the same amount, always have curly brackets close at the same indentation as the opening statement).
also be consistent with spaces (try{ -> try {)
comments: //handle exception isn't a useful comment, especially if it is handled by just throwing the exception :)


Locate a number in an array with a special ordering Below are two examples of arrays (with a special ordering with odd numbers on one side, and even number on the other side) I have, with some given len:

9  7  5  3  1  2  4  6  8

11  9  7  5  3  1  2  4  6  8  10

For some value z, I want to find the column the value is in. MATLAB code I wrote:

if z == 1
   column = ceil(len/2);
elseif logical(mod(z,2))
   column = ceil(len/2) - (z-1)/2;
else
   column = ceil(len/2) + z/2;
end

Does this above locate the correct column?

I suspect that the better solution is to compute the column index directly, without the if-else conditions. You can do this by using integer division, and modulo. Consider the following computation breakdown:

% declare an int32 value 2
itwo = int32(2)
% compute the position of the column with the value 1
onecol = idivide(len + 1, itwo)
% compute how far away the desired value is from the 1 column
distance = idivide(z, itwo)
% compute the direction of the desired value from the 1 column (-1 is left (odd numbers), +1 is right (even numbers))
direction = 1 - 2 * mod(z, itwo)

You can string this together as a 1-liner:

column = idivide(len + 1, itwo) + idivide(z, itwo) * (1 - 2 * mod(z, itwo))

You can also put it in a function should you choose. I have put together this example here:

https://goo.gl/Oenp85 (and updated for int32 conversion here: https://goo.gl/NgJJhN

Note that it has been a long time since I played with MATLAB.

indexOfVal = @(mat, val) idivide(numel(mat) + 1, int32(2)) + idivide(val, int32(2)) * (1 - 2 * mod(val, int32(2)));

x = [9 7 5 3 1 2 4 6 8];

result1 = indexOfVal(x, 9)
result2 = indexOfVal(x, 7)
result3 = indexOfVal(x, 5)
result4 = indexOfVal(x, 3)
result5 = indexOfVal(x, 1)
result6 = indexOfVal(x, 2)
result7 = indexOfVal(x, 4)
result8 = indexOfVal(x, 6)
result9 = indexOfVal(x, 8)

Produces:

result1 =  1
result2 =  2
result3 =  3
result4 =  4
result5 =  5
result6 =  6
result7 =  7
result8 =  8
result9 =  9


Scan a list of assigns in nodejs My starting point is a file like so:

# Default environment to run the app locally.
# In production, the app will probably run on NginX,
# which will act as a reverse proxy
export NODE_ENV='development'
export APPNAME='gigsnet'
export DBHOST='192.168.1.13'
export DBNAME='gigsnet-development'
export IPADDRESS='localhost'
export PORT='8080'

This is a simple bash file that sets and exports some environment variables.
I want some nodeJs scripts to use the same file to set the same variables. The idea is to require this file, and have process.env magically enriched.

So, I wrote this:

var fs = require('fs')
var p = require('path')

fs.readFileSync(p.join(__dirname, 'localEnv.sh')).toString().split('\n').forEach(line => {
  var tokens = line.split(' ')
  if (tokens[0] === 'export') {
    var [ name, value ] = tokens[1].split('=')
    process.env[name] = value.match(/["'](.*?)["']/)[1]
  }
})

Problems I have:

That first line is way too long for my own likings
The variable token could and should have a better name

Looks clunky.

How would you improve it?

The fact that you're parsing BASH means you're probably not worried about portability, which means you probably don't need the path module since the directory separator will always be /. So, you can replace p.join(__dirname, 'localEnv.sh') with __dirname+'/localEnv.sh'
There's a module for reading stuff line by line already, no need to home-roll it.
This is a better regex because it takes escaped quotes into account: "(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*' see here.

require('readline').createInterface({
  input: require('fs').createReadStream(__dirname+'/localEnv.sh')
}).on('line', function (line) {
  var tokens = line.split(' ')
  if (tokens[0] === 'export') {
    var [ name, value ] = tokens[1].split('=')
    process.env[name] = value.match(/"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'/)[1]
  }
});


Abstract classes in Blackjack game From what I've read, I should use abstract classes when I want to define behavior for a superclass, and I don't want to instantiate the superclass. So, I'm making a Blackjack game in Java, and I want a BlackjackDealer class and a BlackjackPlayer class. Would it be okay to make these children of a BlackjackPerson  class, since some of the behavior should be the same? An example of behavior that should be the same:

abstract class BlackjackPerson {

   protected Card[] cards;
   public void hit(Card c) {
       // add c to cards
   }

   public abstract void displayHand();
   // print out hand
}

An example of a BlackjackDealer class, with its own implementation of the displayHand method (since a dealer has 1 card hidden):

class BlackjackDealer extends BlackjackPerson {
    public void displayHand() {

        for (Card c : cards) {
            System.out.println(c);
        }
    }
}

This is my first time using inheritance, I just want to know if I'm on the right track.

Look for common functionality

The real power of inheritance comes from the existence of common functionality.  In this case, the dealing of cards and the calculation of totals will be common to both players and dealers, and so having both of these in the parent class would be a good choice.

Look for differing functionality

A dealer will have rules on when they hit and when they stay.  A player will likely have an amount bet and a chip balance.  These would be candidates for inclusion in the child classes.


N-queens problem in Haskell by bruteforce I wrote a Haskell program to solve the N-queens problem by bruteforce. It works and I find it reasonably readable

But it is pretty slow:

5 seconds for one solution of 8 queens.
1 minute for one solution for 9 queens.
Crash for 10 queens. I fear that allFalseOneTrue gives memory problems as it uses permutations.

I would like to hear both improvements on readability and performance:

import Control.Monad
import Data.List

-- True indicates that there is a queen, False that there is not.
type QueenBoard = [[Bool]]

count   :: Eq a => a -> [a] -> Int
count x =  length . filter (==x)

fAnd :: (a -> Bool) -> (a -> Bool) -> (a -> Bool)
fAnd = liftM2 (&&)

rotate90 :: [[a]] -> [[a]]
rotate90 = map reverse . transpose

noQueensinSameRow :: QueenBoard -> Bool
noQueensinSameRow board = not (any (\row -> count True row > 1) board)

noQueensinSameColumn :: QueenBoard -> Bool
noQueensinSameColumn = noQueensinSameRow . rotate90

-- Attribution to http://stackoverflow.com/questions/32465776/getting-all-the-diagonals-of-a-matrix-in-haskell
diagonals :: [[a]] -> [[a]]
diagonals []       = []
diagonals ([]:xss) = xss
diagonals xss      = zipWith (++) (map ((:[]) . head) xss ++ repeat [])
                                  ([]:(diagonals (map tail xss)))

allDiagonals :: [[a]] -> [[a]]
allDiagonals xss = (diagonals xss) ++ (diagonals (rotate90 xss))

noQueensinSameDiagonal :: QueenBoard -> Bool
noQueensinSameDiagonal = noQueensinSameRow . allDiagonals

isQueenSolution :: QueenBoard -> Bool
isQueenSolution = (noQueensinSameRow `fAnd` noQueensinSameColumn `fAnd` noQueensinSameDiagonal)

allFalseOneTrue :: Int -> [[Bool]]
allFalseOneTrue length = nub $ permutations ( [True] ++ (replicate (length - 1) False) )

allProduct = sequence :: [[a]] -> [[a]]

allPossibleBoards :: Int -> [QueenBoard]
allPossibleBoards size = allProduct (replicate size (allFalseOneTrue size))

solveQueens :: Int -> [QueenBoard]
solveQueens = (filter isQueenSolution) . allPossibleBoards

main :: IO()
main = mapM_ putStrLn $ map show $ head $ solveQueens 8

The standard way to solve this is to:

Only keep track of the positions of the queens - not whether a square has a queen.
Since only each column may have one queen (and must have one queen), a solution is a [Int] indicating the row positions of the placed queens, e.g. [0,2,4] is a solution for a board having 3 columns (and at least 5 rows) where the queens are at (0,0), (1,2) and (2,4).

So build up the solution iteratively. If you have the placement [r1] how can it be extended to a valid placement [r1, r2]? Clearly r2 can't equal r1 and (0,r1) can't be on the same diagonal as (1,r2). And, of course, r2 has to be a valid row number.


Using the Return Value of a Function to Select an Element in a Struct Array I'm learning MATLAB (still trying to "think in vectors") and have the following piece of code:

stats = regionprops(bwconncomp(mask), 'all');

% find the object nearest our nucleus
smallest_dist = flintmax;
location = [cell_x, cell_y];
for j = 1:numel(stats)
    stat = stats(j);
    dist = pdist([location;[stat.Centroid]], 'euclidean');
    if dist < smallest_dist
        regions{channel} = stat;
        smallest_dist = dist;
    end
end

All that is doing is searching through a struct array for the region which has a location (centroid) closest to my known cell's X/Y.  I'm sure this is not canonical and that it could be improved.

I think this comes pretty close. The use of arrayfun() or other *fun()-Operations is very often better than for loops.

Your code was not working out of the box. Thus, I created code, which is directly executable:

function minimalExample()
    target.x = 10;
    target.y = 5;

    source(1) = struct('x', 1, 'y', 3);
    source(2) = struct('x', 5, 'y', 15);

    [~, minIndex] = min(arrayfun(@(x) (myDistance(x, target)), source));

    minSource = source(minIndex);
    disp(minSource);

    function d = myDistance(source, target)
        d = (source.x - target.x)^2 + (source.y - target.y)^2;
        d = sqrt(d);
    end
end


Trial Multiplication for Factorization I am trying a trial multiplication for factorization. This is a general example, but each sequence can be specified to pairs of sequences \${p,q}\$ for a given \$n\$. For example, if \$n\$ ended in 3, then {\$p,q\$} would be: {1,3},{3,1},{7,9} and {9,7} with sequence steps of -10 and 10 respectively.

The essence of the method is if \$pq < n\$, raise \$q\$. If \$pq > n\$, lower \$p\$ while continuing where the last \$p\$ and the last \$q\$ left off.

#Trial Multiplication
TM<- function (number){

    if (floor(sqrt(number))%%2==0)
    p_start <<- floor(sqrt(number))-1

    if (floor(sqrt(number))%%2>0)
    p_start <<- floor(sqrt(number))

    if (ceiling(sqrt(number))%%2==0)
    q_start <<- ceiling(sqrt(number))+1

    if (ceiling(sqrt(number))%%2>0)
    q_start <<- ceiling(sqrt(number))

    for (p in seq(p_start,1,-1)){
      for (q in seq(q_start,number,1)){

      #Quick primality tests for p and q...
      #if(p%%3==0 | p%%5==0 | p%%7==0 | p%%11==0 | p%%13==0 | p%%17==0 | p%%19==0) break
      #if(q%%3==0 | q%%5==0 | q%%7==0 | q%%11==0 | q%%13==0 | q%%17==0 | q%%19==0) next

      if(p*q==number)
      return(c(p,q))

      if(p*q>number) break

      q_start<<- q
      p_start<<- p

      }

  #if(q%%3==0 | q%%5==0 | q%%7==0 | q%%11==0 | q%%13==0 | q%%17==0 | q%%19==0) break
  #if(p%%3==0 | p%%5==0 | p%%7==0 | p%%11==0 | p%%13==0 | p%%17==0 | p%%19==0) next

  if(p*q==number)
  return(c(p,q))

  if(p*q<number) break

  p_start<<- p
  q_start<<- q

    }

}

What is slow is to create seq(q_start,number,1). Take the example where  number is 298,716,239: you are asking R to create a vector of nearly 300 million integers. It chokes. Instead of looping over sequences created in memory, it is easier to just keep the current value, increment and limit in memory. See for example:

TM <- function (n) {

   n <- as.integer(n)
   r <- sqrt(n)
   p <- as.integer(floor(r))
   q <- as.integer(ceiling(r))

   while (p >= 1L & q <= n) {
      if (p * q == n) return(c(p, q))
      if (p * q > n) { p <- p - 1L } else { q <- q + 1L }
   }
   stop("hmm... we should not be here...")
}

TM(1900)
TM(298716239)

Now that's a start... If you try with a large prime, e.g. 298716247, it will take a very long time to converge towards 1 * 298716247 though. So you will have to get more creative to make it converge faster. At least, you should be fixed about why your original code was so slow.

Edit: here is an idea I got for making it converge faster where you update both p and q at each iteration. I hope the math checks out and I did not leave a corner case:

TM2 <- function (n){

   n <- as.integer(n) # everything is faster with integers
   r <- sqrt(n)
   p <- as.integer(floor(r))
   q <- as.integer(ceiling(r))
   while (p >= 1L & q <= n) {
      if (p * q == n) return(c(p, q))
      if (p*q > n) {
         p <- p - 1L
         q <- as.integer(floor(n / p))
      } else {
         q <- q + 1L
         p <- as.integer(ceiling(n / q))
      }
   }
   stop("hmm...")
}

TM2(1900)
TM2(298716239)
TM2(298716247)


Working with Classes (inheriting), @ properties and Initialization I'm working on this Objective C programming assignment I found online.  I'm not sure if I have met all the requirements, especially part C. Any help or suggestion will be appreciated.

  Part 6

  a) Implement class A with properties a1, a2, and a3 (int,
  string, int).

  b) New objects are automatically initialized to 1, "hello", 1.

  c) Also provide initializer to any data and constructor (called
  without alloc) to do the same.

  d) Make sure %@ ob object of A will print all data.

  e) Then implement B inheriting from A. B adds property b (string).

  f) Make sure B works as A, that is new object is initialized to 1, "hello", 1, and 3 (the new data). The rest also must work on B.

   //classA.h file
#import <Foundation/Foundation.h>

@interface ClassA : NSObject
// Part 6a
@property int a1;
@property NSString *a2;
@property int a3;

-(NSString *) description;
-(id) initWithA1: (int) x andA2: (NSString *) s andA3: (int) y;
-(id) init;
@end

//classA.m file
#import "ClassA.h"

@implementation ClassA

-(id) initWithA1:(int)x andA2:(NSString *)s andA3:(int)y {
    self = [super init];
    if (self) {
        self.a1 = x;
        self.a2 = s;
        self.a3 = y;
    }
    return self;
}

// part 6b
- (id) init {
    return [self initWithA1:1 andA2:@"hello" andA3:1];
}

// part 6d
-(NSString *) description {
    return [NSString stringWithFormat:@"ClassA a1 = %d , a2 = %@ , a3 = %d", self.a1, self.a2, self.a3];
}

@end

//classB.h file
#import "ClassA.h"

@interface ClassB : ClassA
@property int a1;
@property NSString *a2;
@property int a3;
@property NSString * b;

-(NSString *) description;
-(id) initWithA1:(int)x andA2:(NSString *)s andA3:(int)y andB: (NSString *) z;
-(id) init;
@end

//classB.m file
#import "ClassB.h"

@implementation ClassB

-(id) initWithA1:(int)x andA2:(NSString *)s andA3:(int)y andB:(NSString *)z {
    self = [super init];
    if (self) {
        self.a1 = x;
        self.a2 = s;
        self.a3 = y;
        self.b = z;
    }
    return self;
}

-(id) init {
    return [self initWithA1:1 andA2:@"hello" andA3:1 andB:@"3"];
}

-(NSString *) description {
    return [NSString stringWithFormat:@"ClassB a1 = %d , a2 = %@ , a3 = %d , b = %@" , self.a1, self.a2, self.a3, self.b];
}

@end

//viewController.m file
#import "ViewController.h"
#import "ClassA.h"
#import "ClassB.h"
@interface ViewController ()

@end

@implementation ViewController

- (void)viewDidLoad
{
    [super viewDidLoad];
    ClassA * a = [ClassA new];
    NSLog(@"%@", a);

    ClassB * j = [ClassB new];
    NSLog (@"%@", j);
}

- (void)didReceiveMemoryWarning
{
    [super didReceiveMemoryWarning];
    // Dispose of any resources that can be recreated.
}

@end

This is my first review so bear that in mind when you are reading it.

The first thing I see is that in the initialization, you should not use the automatic setters and getters for properties.  The reason for this is that the object may not be completely initialized, and thus may fail.  So instead of doing:

self.a1 = x;
self.a2 = s;
self.a3 = y;

access the properties directly like so:

_a1 = x;
_a2 = s;
_a3 = y;

Otherwise, your init pattern is good, and bonus points for having a default -(id)init that calls your designated initializer.

For the names of all of these, I would make sure to always use a more descriptive name.  It is probably okay in such a simple example to use a1, x, y, etc, but for anything more complicated, use names that describe in more detail exactly what the variable is.

In the description method it looks like you are calling the int values with %d.  I can't think of a reason not to use %i, but maybe I am wrong here.

One last thing, in the interface the spacing inside the declaration of -(id)initWithEtc is loose.  It looks correct in the implementation though.


Getting the miliseconds from now to the next midday I need to get the total milliseconds to the next mid-day (12:00:00) to signal a timer that will run once a day.

Assuming the application can be shut down and started anytime, is this code to get the milliseconds left to midday (used to set the timer) correct? (It doesn't need to be exact; it can be off by a few seconds.)

TimeSpan now = DateTime.Now.TimeOfDay;
TimeSpan target = new DateTime(DateTime.Now.Year, DateTime.Now.Month, DateTime.Now.Day, 12, 0, 0).TimeOfDay;

double r = target.TotalMilliseconds - now.TotalMilliseconds;

if (r > 0) // It's before noon
    ;
else // It's after noon
    r = TimeSpan.FromTicks(TimeSpan.TicksPerDay).TotalMilliseconds + r;

t = new Timer(DoWork, null, r, TimeSpan.FromTicks(TimeSpan.TicksPerDay));

Can it be done more efficiently or in fewer lines of code?

if (r > 0) // It's before noon
   ;
else // It's after noon
   r = TimeSpan.FromTicks(TimeSpan.TicksPerDay).TotalMilliseconds + r;

Change to:

if (r <= 0) // It's after noon
   r = TimeSpan.FromTicks(TimeSpan.TicksPerDay).TotalMilliseconds + r;

No need to have an if that does nothing.


Determining whether a string is transformable into another by repetitively increasing its prefixes alphabetically Here's Alice and Strings, a programming challenge on hackerearth:

  Two strings \$A\$ and \$B\$ comprising of lower case English letters
  are compatible if they are equal or can be made equal by following
  this step any number of times:

  Select a prefix from the string \$A\$ (possibly empty), and increase the alphabetical value of all the characters in the prefix by the same
  valid amount. For example if the string is xyz and we select the
  prefix xy then we can convert it to yz by increasing the
  alphabetical value by 1. But if we select the prefix xyz then we
  cannot increase the alphabetical value.

  Your task is to determine if given strings \$A\$ and \$B\$ are
  compatible.

  Input format

  First line: String \$A\$

  Next line: String \$B\$

  Output format

  For each test case, print YES if string \$A\$ can be converted to
  string \$B\$, otherwise print NO.

  Constraints

  $$
  \DeclareMathOperator{\len}{len}
  1 \leq \len(A) \leq 1\,000\,000 \\
  1 \leq \len(B) \leq 1\,000\,000 \\
  $$

Can you please review my solution to it?  Please comment on any aspect to improve such as readability, comment usage, further improvements on speed/memory usage, etc.

/*
HackerEarth
https://www.hackerearth.com/practice/algorithms/string-algorithm/basics-of-string-manipulation/practice-problems/algorithm/aliceandstrings-9da62aa7/
*/

#include <iostream>

/*
Recusrsively checks if string A can be converted to B by incrementing the prefixes:

eg:
A = abaca B = cdbda
[step 1]bcbda (prefix abac incremented by 1)
[step 2]bc[b]da cd[b]da (don't take bcb as a prefix because b is already matched)
[step 3]cdbda -----> so convertible (incrementing prefix bc)
*/
bool transformable(std::string& A, std::string& B, int last_index)
{
    if (A.compare(0, last_index, B, 0, last_index) == 0)
    {
        return true;
    }
    else
    {
        if (A[last_index] == B[last_index])
        {
            return transformable(A, B, --last_index);
        }
        else
        {
            if (A[last_index] > B[last_index])
            {
                return false;
            }
            else
            {
                while(A[last_index] != B[last_index])
                {
                    for(int i = 0; i <= last_index; i++)
                    {
                        A[i] += ('b' - 'a');
                    }
                }

                return transformable(A, B, --last_index);
            }

        }

    }

}

int main()
{
    std::string A;
    std::string B;

    std::cin >> A >> B;

    if (A.length() != B.length())
    {
        std::cout << "NO" << std::endl;
        return 0;
    }

    if (transformable(A, B, A.length() - 1))
        std::cout << "YES" << std::endl;
    else
        std::cout << "NO" << std::endl;

    return 0;
}

Code review

In general, the code looks nice and follows consistent styles.  Some small observations:

Missing #include <string>.
Avoid std::endl when \n suffices.  std::endl flushes the buffer, while \n does not.  Unnecessary flushing can cause performance degradation.  See std::endl vs \n.  Use

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

instead of

std::cout << "YES" << std::endl;

unless flushing is a requirement from the website (I hope not).
Since we are depending on lowercase letters being continuously encoded anyway, 'B' - 'A' is just 1.  And A[i] += 1 becomes ++A[i].

Better algorithm

That said, the logic of the code is overly convoluted.  The many levels of nested control statements make it hard to follow the code.  There is a much simpler algorithm to do it: to determine whether A is transformable into B,

take the differences between the corresponding positions B[i] - A[i];
form a sequence out of these differences;
append the sequence by 0 (to prevent cases like bbbbb => aaaaa);
test if this sequence is sorted in non-ascending order.

For example:

for A = "abcde", B = "ccdde", the sequence of differences is 21100, so the transformation is possible (abcde => bcdde => ccdde);
for A = "abcde", B = "bbcce", the sequence of differences is 10010, so the transformation is impossible (try it).

Here's (roughly) how I would put everything together:

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

bool transformable(std::string_view a, std::string_view b)
{
    if (a.size() != b.size()) {
        return false;
    }
    std::vector<int> diff_sequence(a.size() + 1);
    std::transform(b.begin(), b.end(), a.begin(), diff_sequence.begin(), std::minus<>{});
    diff_sequence.back() = 0;
    return std::is_sorted(diff_sequence.begin(), diff_sequence.end(), std::greater<>{});
}

int main()
{
    std::string A;
    std::string B;

    std::cin >> A >> B;

    if (transformable(A, B)) {
        std::cout << "YES\n";
    } else {
        std::cout << "NO\n";
    }
}

(live demo)

(Replace std::string_view with const std::string& and remove the #include <string_view> if C++17 is not available, which IIRC is the case with competitive programming sites.)

The transformable function can be simplified with the range-v3 library:

namespace views = ranges::views;

bool transformable(std::string_view a, std::string_view b)
{
    return ranges::is_sorted(
        views::concat(
            views::transform(b, a, std::minus<>{}),
            views::single(0)
        ), std::greater<>{});
}

(live demo)


Generating 10-character passwords, with 3-6 digits and 3-6 uppercase letters, in C++ In response to a question on Information Security on brute-forcing passwords, I wrote code that helped solve the problem:

  Generate a password list of 10 character passwords containing only a combination of 3 - 6 numbers and 3 - 6 uppercase letters.

I'd like a code review done of the snippet I wrote.  I don't know really anything about optimizing software. I can write it (self-taught) but I don't have the deep insights needed to improve already working software, so I've started posting code snippets in here for you guys to give me insights. I really think this channel is great and without further ado, I'll get to it.

#include <iostream>
#include <vector>
#include <random>
#include <string>

const char charset[] = {'A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z','0','1','2','3','4','5','6','7','8','8','9'};
int main()
{

    std::cout << "Please enter the number of passwords to generate here: ";
    int num_pass;
    std::cin >> num_pass;
    std::random_device dev;
    std::mt19937_64 rng(dev());
    std::vector<std::string> passwds;
    std::uniform_int_distribution<std::mt19937_64::result_type> dist(0, sizeof(charset) - 1);

    for (int i = 0; i < num_pass; ++i) {
        std::string pass = "";
        int num_nums = 0, num_chars = 0;
        while (pass.length() < 10) {
            char c = charset[dist(rng)];
            if (isdigit(c) && num_nums < 6) {
                pass += c;
                num_nums++;
            }
            else if (isalpha(c) && num_chars < 6) {
                pass += c;
                num_chars++;
            }
        }
        passwds.push_back(pass);
        std::cout << pass << std::endl;
    }
    std::cin.get();
}
```

Your passwords will be biased, with more letters appearing towards the front of the password and more digits towards the end.

A better approach would be to determine how many digits you will have in the password. Then, for each character, determine if it should be a digit based on the number of digits you want to have and the number of characters left to fill by checking if (random(characters_left) < digits_left).  Then select either a random digit or letter for that position.

When you declare pass, you don't need to pass it an empty string.  It is default constructed as empty.

std::string pass;


Get a notification when questions get posted I like to help people on StackOverflow, matter of fact nowadays it depends on how quick you are when answering question.
With that being said, I can't always keep a tab and an eye to refresh the browser to view new questions.

I made a very basic script to send me a notification (OSX) whenever there's a new question.

import requests, os, threading, time
from bs4 import BeautifulSoup
from timeloop import Timeloop
from datetime import timedelta

tl = Timeloop()

SE_REALTIME = "https://stackexchange.com/questions?tab=realtime"
WATCHED_TAG = "swift"
QUE_LI = []

# The notifier function
def notify(title, subtitle, message, link):
    t = '-title {!r}'.format(title)
    s = '-subtitle {!r}'.format(subtitle)
    m = '-message {!r}'.format(message)
    l = '-open {!r}'.format(link)
    os.system('terminal-notifier {}'.format(' '.join([m, t, s, l])))

def fetch():
    # The SOUP
    soup = BeautifulSoup(requests.get(SE_REALTIME).content, "html.parser")

    # List of Questions with the sid: Stackoverflow.com
    li = soup.find_all('div', {'data-sid': 'stackoverflow.com'})

    # Loop Through
    for child in li:
        id = child['class'][2]
        all_tags = child.find("span", attrs={'class': 'realtime-tags'}).text.strip()
        tags = all_tags.split()
        if WATCHED_TAG in tags:
            if id not in QUE_LI:
                question = child.find("a", attrs={'class': 'realtime-question-url realtime-title'})
                QUE_LI.append(id)
                notify(title = 'New Question {}'.format(' '.join([WATCHED_TAG])),
                subtitle = 'Stackoverflow.com',
                message  = question.text.strip(),
                link = question['href'])

@tl.job(interval=timedelta(seconds=2))
def fetch_questions_every_2s():
    fetch()

if __name__ == "__main__":
    tl.start(block=True)

Ok, This code works well. But I'm looking for:

A way to listen for changes on the div tree (without making a request every 2 second)
Less code.

Dependencies:

Terminal-Notifier
BeautifulSoup
TimeLoop
Requests

Why would you use stackexchange.com and only take ones with the title of stackoverflow.com? That's a huge waste.

You could change your url from https://stackexchange.com/questions?tab=realtime to https://stackoverflow.com/questions?tab=realtime.

However, actually visiting the website the way you are is hacky and overkill. Instead of web scraping, use the Stackexchange API.

There is no way to get a notification when a new question is posted. You will have to make a call every time you want the latest results.

Edit: there may be a way to get notifications, see vogel612's comment:

  there is websockets to receive notifications when a new question is
  posted. To my knowledge these are very much undocumented and easier to
  get access to with a userscript (which would imply changing to
  javascript), but it's not like there's no way to receive notifications
  from SE directly.


Infinite House of Pancakes in Haskell In order to learn haskell, I wrote a solver for the Infinite House of Pancakes problem.

-- Solve the 'Infinite House of Pancakes' problem.
-- https://code.google.com/codejam/contest/6224486/dashboard#s=p1

import Text.Printf
import Control.Monad

-- Generic solver code

readInt :: IO Int
readInt = readLn

readIntList :: IO [Int]
readIntList = getLine >>= return . map read . words

main :: IO [()]
main = do
  n <- readInt
  forM [1..n] (\i ->
    parse >>= putStr . printf "Case #%d: %d\n" i . solve)

-- Problem-specific code

parse :: IO [Int]
parse = getLine >> readIntList

cost :: Int -> Int -> Int
cost l n = if r == 0 then q - 1 else q where (q, r) = n `quotRem` l

test :: Int -> [Int] -> Int
test l = (+l) . sum . map (cost l)

solve :: [Int] -> Int
solve s = minimum . map (\l -> test l s) $ [1 .. maximum s]

I used this python solution as a reference, which is based on the  following problem analysis. I tested  my program using those input and output files, and it seems to work fine:

$ stack ghc pancake.hs -- -O2
$ \time -f "%e s" ./pancake < B-large-practice.in | diff - B-large-practice.out
0.28 s

I'm looking for comments about the following points:

coding style
performance (currently runs in 0.28 seconds using -O2, against 5.5 seconds for python 3)
bind operators vs do syntax (I used both  here, but it was mostly for educational purposes)
tools and ecosystem (code analysis, better compilation flags, etc.)

EDIT - Thanks to @Gurkenglas answer, I've been able to improve this program quite a lot. Here's a link to the corresponding gist.

I'll refactor so every line tells the reader something about the program.

{-# LANGUAGE ScopedTypeVariables #-}
import Text.Printf
import Control.Monad
import Data.List.Split (splitPlaces)
import Safe (headNote)

main :: IO ()
main = interact                                                                -- no side effects
  $ (\f -> unlines . zipWith f [1..] . subList [2,4..] . lines)                -- embarassingly parallel
  $ (\solve (i :: Int) -> printf "Case #%d: %d" i . solve . map read . words)  -- mere math
  $ (\cost (s :: [Int]) -> minimum [cost l s | l <- [1 .. maximum s]])         -- brute optimization
  $ \l -> (+l) . sum . map ((`div` l) . subtract 1)

-- This gets a name because we ought to be importing it from a library.
subList :: [Int] -> [a] -> [a]
subList is = map (headNote "subList: index list not strictly ascending")
  . drop 1 . splitPlaces (zipWith (-) is $ 0:is)


Usage of TPL vs Parallel.ForEach() on file processing i've been working on an implementation of a my own from scratch set of library Astron and I wanted to get my TPL usage reviewed because i'm not confident with this technology :/

My app is currently unpacking .d2p files, which is a custom file format from a french game called Dofus, it's simply an archive of others inflated archives of another custom file format .dlm. Here is the current output of my app :

The progress bars came from here.

So everything seems to works as expected, files are processes concurrently, 308mo of .dlm files are parsed deflated in 10sec, that is exactly what i wants it to do but i may have misused the TPL. The full project can be found at this address, but the portion of code I want to get reviewed is from src/Astron.Unpacker/Managers/D2PManager.cs :

public class D2PManager : BaseFileManager
{
    private readonly ILogger _logger;
    private readonly string _dlmFilesFolder;
    public D2PManager(IContainer container) : base(container)
    {
        _logger = ServiceLocator.Logger;
        _dlmFilesFolder = container.GetInstance<Settings>().DlmOutputFolder;
    }

    public async Task UnpackAll(string[] filesPath)
    {
        _logger.Log<D2PManager>(LogLevel.Info, $"Attempting to unpack {filesPath.Length} d2p files...");

        var tasks = new List<Task>(filesPath.Length);
        tasks.AddRange(filesPath.Select(d2PFilePath => UnpackD2PFile(d2PFilePath)));

        await Task.WhenAll(tasks).ConfigureAwait(false);
    }

    public async Task UnpackD2PFile(string path)
    {
        var d2PFile      = new FileAccessor(path);
        var metaUnpacker = new D2PFileMetadataUnpacker(_binaryFactory, _serDes);
        metaUnpacker.Unpack(d2PFile);

        var archiveUnpacker = new DlmArchivesUnpacker(_binaryFactory, _serDes, metaUnpacker.Value);
        archiveUnpacker.Unpack(d2PFile);

        var progressCount = 1;
        var progressBar   = new ProgressBar(PbStyle.SingleLine, archiveUnpacker.Values.Count);
        progressBar.Refresh(0, Path.GetFileName(d2PFile.FullPath));
        await Task.Delay(10); // doesn't print all either way
        foreach (var archive in archiveUnpacker.Values)
        {
            var filePath      = (_dlmFilesFolder + archive.RelativePath).Replace('/', '\\');
            var fileDirectory = Path.GetDirectoryName(filePath);
            using var decompressedData = new MemoryStream();
            using var deflatedStream = new DeflateStream(new MemoryStream(archive.CompressedData),
                CompressionMode.Decompress);

            await deflatedStream.CopyToAsync(decompressedData);
            if (!Directory.Exists(fileDirectory)) Directory.CreateDirectory(fileDirectory);

            File.WriteAllBytes(filePath, decompressedData.GetBuffer());
            progressBar.Refresh(progressCount, filePath);
            progressCount++;
        }
    }
}

Here are my questions :

If I don't add the Task.Delay() right after the processbar initialization, the files seems to be processed synchronously (the progress bar show up when the last completed), why does it happen ?
Is it right to use .ConfigureAwait(false) on Task.WhenAll() ?
Am I starting every tasks the right way ? Shouldn't I use Task.Run() instead with Task.WaitAll() ?

As seen with @t3chb0t and @dfhwze I have been misusing the TPL. I also don't see any point on .ConfigureAwait(false) a Task.WhenAll() call, also with the async all-the-way rule, using an awaitable task for this case would have me to convert many of my APIs to async one.

So they provided me these solutions :

Modifiy my task creation logic this way :  tasks.AddRange(filesPath.Select(async d2PFilePath => await Task.Run(() => UnpackD2PFile(d2PFilePath)))); and removing the Task.Delay(10); on my UnpackD2PFile(string path) task
Use the Parallel.Foreach() loop to let the framework create and handle the concurrency and also allow me to keep a synchronous API

Both of these solutions completed the work in the same time, and provided this output :

As you can see now the behavior is quite different than before, not every task start at the same moment, also now there is a delay before every progress bars shown up. But we gained almost 2sec on the total execution time :)

I highly recommend to use the Parallel approach as it is the easiest one to implement.


Calculating money made per hour I am new to coding and made myself my first little program, which lets me calculate the amount of money I make per hour doing a transcription project. While my code works (yay!) and I am proud of my first attempt at creating a program from scratch, I'm not sure I went about it the most efficient way. Could someone please look this over and let me know where I could improve?

#include <stdio.h>
#include <cs50.h>

int main(void)

{
    float clip_time;
    float real_time;
    float pay_rate;
    float pay_amt;
    float hours;

do{

printf("Transcription Rates\n");
printf("Clip Time: ");
clip_time = get_float();
}
while(clip_time <= 0);

printf("%.2f min clip pays: ", clip_time);
    pay_rate = get_float();

printf("So, $%.2f for the project \n", pay_rate * clip_time);
    pay_amt = pay_rate*clip_time;
    {

do{
    printf("Time to Complete Clip (in minutes): ");
    real_time = get_float();

   printf("Equals %.2f hours \n", real_time/60);
   hours = real_time/60;

}
while(real_time <= 0);

    {
        printf("Real time spent per audio minute: %.2f \n", real_time / clip_time);
}

{
        printf("Hourly pay $%.2f div by %.2f hours equals $%.2f ph \n", pay_amt, hours, pay_amt / hours);
    }

    }
}

I think you have written your code well! Here are some steps you can have in mind when coding:

Always comment the code, even though it is self-explanatory, it might not be in 1-5 years later. Comment so that you and others trying to help you understand what your goal was.
You can always improve your code, one way is follow DRY mindset, which is Don't Repeat Yourself. You can use functions to wrap your code, instead of using the same code in multiple places.
Learn as you go, watch others how they organize your code, and implement their structure into your code.


Command-line utility to capitalize long words in a file I have been developing a capitalizer in Python. This is actually a script that an end user will type at their terminal/command prompt. I have made this script to:

Taking the first argument as a file name and process it.
Don't process words that have 3 or less alphabet.

Here's some code:
#!/usr/bin/env python
#-*- coding:utf-8 -*-

from os import linesep
from string import punctuation
from sys import argv

script, givenfile = argv

with open(givenfile) as file:
    # List to store the capitalised lines.
    lines = []
    for line in file:
        # Split words by spaces.
        words = line.split()
        for i, word in enumerate(words):
            if len(word.strip(punctuation)) > 3:
                # Capitalise and replace words longer than 3 letters (counts
                # without punctuation).
                if word != word.upper():
                    words[i] = word.capitalize()
        # Join the capitalised words with spaces.
        lines.append(' '.join(words))
    # Join the capitalised lines by the line separator.
    capitalised = linesep.join(lines)

print(capitalised)

A lot more features still have to be added but this script does the stuff it is made for.
What I want (from you):
I want this script to be emerge as a command line utility. So,

How can I make this a better command line utility?
Could I have written in a better way?
Can this script be faster?
Are there any flaws?
Other than GitHub, how can I make it available to the world (so that it is easier to install for end users)? because GitHub is for developers.

Please also post links to make my script and me even more mature.

As a Command Line Utility

The Argparse module is designed to facilitate the creation of command line utilities. It makes processing command line arguments much easier, and will even automatically display usage information if the user incorrectly enters arguments.

Potential Flaws

Your program does not preserve whitespace. This could be by design but I suspect it is not.
Example:

the      quick      brown  fox

will become

the Quick Brown Fox


Can this method be changed to remove all objects in one call? I am converting a VB.NET app over to C#. I won't subject you to the old code but the following is done in a little over 100 lines using nested loops. I am using Entity Framework 6 as my ORM. I have cleaned that up some to this:

public void Remove(int key)
{
    var myObject = _context.MyObjects.FirstOrDefault(x => x.QueryKey == key);
    var myObjectInputs = _context.MyObjectInputs.Where(x => x.QueryKey == key);
    var myObjectInputProperties = _context.MyObjectInputProperties.Where(x => myObjectInputs.Select(y => y.WQI_Key).Contains(x.WQI_Key));
    var myObjectOutputs = _context.MyObjectOutPuts.Where(x => x.QueryKey == key);
    var myObjectOutputProperties = _context.MyObjectOutputProperties.Where(x => myObjectOutputs.Select(y => y.WQO_Key).Contains(x.WQO_Key));
    var myObjectGroups = _context.MyObjectGroups.Where(x => x.QueryKey == key);

    _context.MyObjects.Remove(myObject);
    _context.MyObjectInputs.RemoveRange(myObjectInputs);
    _context.MyObjectInputProperties.RemoveRange(myObjectInputProperties);
    _context.MyObjectOutPuts.RemoveRange(myObjectOutputs);
    _context.MyObjectOutputProperties.RemoveRange(myObjectOutputProperties);
    _context.MyObjectGroups.RemoveRange(myObjectGroups);
    _context.SaveChanges();
}

I still don't like it. The method is huge and doesn't follow the single responsibility principle. I considered refactoring
to this:

public void Remove(int key)
{
    var myObject = _context.MyObjects.FirstOrDefault(x => x.QueryKey == key);
    _context.MyObjects.Remove(myObject);
    RemoveChildren(key);
    _context.SaveChanges();
}

private void RemoveChildren(int key)
{
    RemoveInputs(key);
    RemoveOutputs(key);
    RemoveGroups(key);
}

private void RemoveGroups(int key)
{
    var myObjectGroups = _context.MyObjectGroups.Where(x => x.QueryKey == key);
    _context.MyObjectGroups.RemoveRange(myObjectGroups);
}

private void RemoveOutputs(int key)
{
    var myObjectOutputs = _context.MyObjectOutPuts.Where(x => x.QueryKey == key);
    var myObjectOutputProperties =
        _context.MyObjectOutputProperties.Where(x => myObjectOutputs.Select(y => y.WQO_Key).Contains(x.WQO_Key));
    _context.MyObjectOutPuts.RemoveRange(myObjectOutputs);
    _context.MyObjectOutputProperties.RemoveRange(myObjectOutputProperties);
}

private void RemoveInputs(int key)
{
    var myObjectInputs = _context.MyObjectInputs.Where(x => x.QueryKey == key);
    var myObjectInputProperties =
        _context.MyObjectInputProperties.Where(x => myObjectInputs.Select(y => y.WQI_Key).Contains(x.WQI_Key));
    _context.MyObjectInputs.RemoveRange(myObjectInputs);
    _context.MyObjectInputProperties.RemoveRange(myObjectInputProperties);
}

This seems like an okay option, but I am wondering if there is a way to remove all of these items in one fell swoop.

Obviously in a perfect world I would associate these objects in the database and have them cascade delete; however, I cannot edit our database structure at this time.

Can this method be changed to remove all objects in one call?

There is no way to remove multiple objects from multiple tables in one statement and there shouldn't be (cascading foreign key constraints notwithstanding).

The problem is that Remove(int key) is a poor name for that function. Something like RemoveObjectInputsOutputsPropertiesAndGroups(int key) would be better. Similarly, I would have on idea what a RemoveChildren() function should do in this context.


Thread-safe Bloom Filter in Java I have tried to implement a Bloom Filter in Java here.

https://github.com/srirammanoj/skynet/tree/master/bloomfilter

I just wanted to know if my implementation can be called  'thread-safe' , and if there is an improvement to be made on the locking I have used here. I opted for locks instead of 'sychronized', as I wanted multiple simultaneous reads. I tried to minimize the section of the code inside the lock.

The optimistic read lock of Stamped Lock seems fragile and will give me erroneous reads and I would rather block a read than fail it and retry.

I wanted to know if there is an obvious improvement to be made here.

The actual methods in the code containing the lock. ----

private void addElement(byte[] element) {
    if (null == element) {
        throw new NullPointerException();
    }

    long hash1 = Integer.toUnsignedLong(Murmur3Hash.murmurhash3_x86_32(element, 0, element.length, filterParams.seed1));
    long hash2 = Integer.toUnsignedLong(Murmur3Hash.murmurhash3_x86_32(element, 0, element.length, filterParams.seed2));

    writeLock.lock();
    try {
        for (int i = 1; i <= filterParams.k; i++) {
            long hash = hash1 + i * hash2;

            int mod = ( int ) ( hash % filterParams.bucketLength );

            int offset = (i - 1) * filterParams.bucketLength;

            int index = offset + mod;
            bitSet.set(index);
        }

        elementsAdded++;
    } finally {
        writeLock.unlock();
    }

}

public boolean containsElement(byte[] element) {
    if (null == element) {
        throw new NullPointerException();
    }

    long hash1 = Integer.toUnsignedLong(Murmur3Hash.murmurhash3_x86_32(element, 0, element.length, filterParams.seed1));
    long hash2 = Integer.toUnsignedLong(Murmur3Hash.murmurhash3_x86_32(element, 0, element.length, filterParams.seed2));

    readLock.lock();
    try {
        for (int i = 1; i <= filterParams.k; i++) {
            long hash = hash1 + i * hash2;

            int mod = ( int ) ( hash % filterParams.bucketLength );
            int offset = (i - 1) * filterParams.bucketLength;
            int index = offset + mod;
            if (index < 0)
                index = -index;

            if (!bitSet.get(index)) {
                return false;
            }
        }
    } finally {
        readLock.unlock();
    }

    return true;
}

Thanks.

The locking looks fine, but there are a couple of things which look fishy in the code.

Firstly,

            if (index < 0)
                index = -index;

This should surely be present in both methods or in neither? But it's only in the second one.

Secondly,

            int offset = (i - 1) * filterParams.bucketLength;

It seems that each derived hash is stored in a non-overlapping bucket. By my understanding of Bloom filters, what you have here isn't a Bloom filter.

Finally, I suggest for your consideration unrolling the multiplications:

        long hash = hash1;
        for (int i = 1, offset = 0; i <= filterParams.k; i++) {
            hash += hash2;

            int mod = ( int ) ( hash % filterParams.bucketLength );
            int index = offset + mod;

            bitSet.set(index);

            offset += filterParams.bucketLength;
        }

This saves a bit of thought as to "why does it multiply by i to calculate hash but by i-1 to calculate offset?".


Binary search: first/last/random occurrence I've written some code to "binary search" a list, and return the first occurrence of the target:

def bsearch(a, left, right, target, first_or_last = 'first'):
    """
    Use binary search to find the first, last, or random index
    of target.

    >>> a = [1,1,1,1,1,1]
    >>> bsearch(a, 0, len(a)-1, 1, 'first')
    0
    """

    if left > right:
        return -1

    mid = (left + right)//2
    val = a[mid]

    if val == target:
        first_occur = mid
        if first_or_last == 'first'
            later_occur = bsearch(a, left, mid - 1, target)  # check if there's an earlier occurrence in the left sub-list
            if later_occur != -1:
                return later_occur
            else:
                return first_occur

        elif first_or_last == 'last':
            later_occur = bsearch(a, mid + 1, right, target)
            if later_occur != -1:
                return later_occur
            else:
                return first_occur
        else:
            return first_occur

    if target < val:
        return bsearch(a, left, mid - 1,  target)
    if target > val:
        return bsearch(a, mid + 1, right, target)

Is there a more elegant/cleaner way to give this function the ability to find the first, last, or random occurrence of the target, without use of a string to represent the cases (as I've done)? Suggestions would be great.

You want to have the ability to choose between finding the first, last and a random occurrence of an element in a list. Since you have three options, you cannot use true/false, and numbers aren't very intuitive (and they usually end up magic numbers). The remaining options are strings, functions or some other data structure. Since you don't like strings, and other data structures wouldn't make much sense (why use a complex object when a simple one will suffice?), then let's stick to functions.

But wait, you don't want any code duplication. That's perfectly ok. We note that all 3 of these options involve variations of calling the basic binary search.

Let's create a basic binary search that outputs a few extras:

def _binary_search(array, element, low, high):
    """Binary search that returns (index, low, high) of the final search step"""
    while low <= high:
        mid = (low + high) // 2
        current_element = array[mid]
        if current_element == element:
            return (mid, low, high)
        elif current_element < element:
            low = mid + 1
        elif current_element > element:
            high = mid - 1

    return (-1, 0, 0)

Then, let's create the new functions:

def lower_bound(array, element):
    """Returns the index of the first occurrence of element in array"""
    index = -1
    first_index, low, high = _binary_search(array, element, 0, len(array)-1)
    index = first_index
    while first_index != -1:
        index = first_index
        first_index, low, high = _binary_search(array, element, low, first_index-1)
    return index

def upper_bound(array, element):
     """Returns the index of the last occurence of element in array"""
     index = -1
     first_index, low, high = _binary_search(array, element, 0, len(array)-1)
     index = first_index
     while first_index != -1:
         index = first_index
         first_index, low, high = _binary_search(array, element, first_index+1, high)
     return index

def binary_search(array, element):
    """Basic binary search"""
    lower_bound = 0
    upper_bound = len(array) - 1
    return _binary_search(array, element, lower_bound, upper_bound)[0]

This has the advantage of being readable, not susceptible to spelling errors ("First" vs "first" vs "fist") and not having much in the way of duplicated code.

As a fun fact: this entire problem can be easily solved by making wrappers around bisect_left and bisect_right from the bisect module like this very convenient SortedCollection Python recipe has done here.

Update:

Renamed functions to comply with standards as per comments
Changed algorithms to not create new arrays and instead do an in-place binary search
Changed algorithms to produce the correct results
Added reference to an easy to use Python recipe that implements this, and other features.


Locker Manager app c++ good afternoon.
I'm learning c++ and I made this app to "manage" lockers.
I would like you to review it please, you can find the source code at my github: https://github.com/VitalZero/lockermanager

The app is made up by 3 main clases:

CDocument: Takes care of file input/output
CLockers: The lockers itself, stores it's info
CLockManager: Takes care of the lockers and the document

Also, there's a "helper" class I made to show the functionality, SimpleMenu, that is not part of the main "program"

To use it as is, just create an instance, feed the file name and thats it:

#include "CLockManager.h"
#include "SimpleMenu.h"

int main()
{
  CLockManager manager("database.txt");
  SimpleMenu menu(manager);

  menu.ShowMenu();

  return 0;
}

File CDocument.h:

#ifndef CDOCUMENT_H_
#define CDOCUMENT_H_
/*
 * CDocument.h
 * It is used by CLockManager.
 * Just used to load and save data from a file using a vector of CLockers
 * CLockers overloads stream operators >> and <<
 * Just feed a vector of CLockers and it's all good!
 */
#include <fstream>
#include <string>
#include <vector>
#include "CLockers.h"

class CDocument{
public:
    CDocument() = delete;
    CDocument(const std::string& path_in)
        :
        path(path_in)
    {
    }
    ~CDocument()
    {
        if(inFile.is_open())
            inFile.close();
    }
    bool SaveData(std::vector<CLockers>& lockers);
    bool LoadData(std::vector<CLockers>& lockers);

private:
    std::fstream inFile;
    std::string path;
};

#endif /* CDOCUMENT_H_ */

File CDocument.cpp

#include "CDocument.h"

bool CDocument::SaveData(std::vector<CLockers>& lockers)
{
    inFile.open(path.c_str(), std::ios_base::out | std::ios_base::trunc);

    if(inFile.is_open())
    {
        for(unsigned int i = 0; i < lockers.size(); ++i)
        {
            inFile << lockers.at(i);
        }
        inFile.close();
        return true;
    }

    return false;
}

bool CDocument::LoadData(std::vector<CLockers>& lockers)
{
    inFile.open(path.c_str(), std::ios_base::in);

    if(inFile.is_open())
    {
        CLockers tmp;
        lockers.clear();

        while( (inFile.good()) && (inFile >> tmp) ) // here is the problem from CLockers, even though is being checked in here
        {                                           // it gives another extra loop after the fail bit is set
            lockers.push_back(tmp);
        }

        inFile.close();

        return true;
    }
    else
    {
        // Probably move this out to menu.cpp
        // interaction only should occur outside the class
        std::cout << "\tThe file "
                << path << " doesn't exist or can't be opened.\n";
        std::cout << "Do you want to create it? y/n: ";
        std::string stringAnswer;
        std::getline(std::cin, stringAnswer);

        if( (stringAnswer == "y") || (stringAnswer == "Y") )
        {
            inFile.open(path, std::ios_base::out | std::ios_base::trunc);
            inFile.close();
            return true;
        }
    }
    return false;
}

Thank you in advance! and any suggestion/correction is highly appreciated!. Greetings.

Close on destruction

You shouldn't need to explicitly close your file member on destruction. Since it's a safe variable and not an unsafe pointer, fstream's own destructor will be called and the file will be closed.

Const members

You should make path const, since it doesn't change over the course of the object's lifetime.

File object scope

inFile is not opened in the constructor, and it's always used in open/close pairs in your methods. That means that it shouldn't be a member at all; just use local variables. One benefit is that you won't need to explicitly close those files; when the variable goes out of scope the file will be closed.

Error handling

If you want to drink more of the OOP kool-aid, you shouldn't be using boolean return values, which is a pattern inherited from C code. Instead, your methods would be void-typed, and you'd throw an exception if something goes wrong.


Build a combinatorial matrix from two vectors I want to define a matrix of values with a very simple pattern. I've achieved it with the code below but it's ugly - very difficult to read (I find) and surely not as optimal, in terms of performance, as it could be. The former is really what I'm trying to address here. I feel this can be done much more elegantly and would love some guidance from the community.

Here is an example of what I am hoping to achieve, from x and y I want to get points:

y =

    10    30    50    70

x =

    10    30    50

points =

    10    10
    30    10
    50    10
    10    30
    30    30
    50    30
    10    50
    30    50
    50    50
    10    70
    30    70
    50    70

x and y are, of course, no always those exact vectors. They are, however, very simply created. Something like so:

scale = 20;
max_x = 85;
max_y = 62;

y_count = floor(max_x / scale);
x_count = floor(max_y / scale);

y = ((1:y_count) * scale) - scale / 2;
x = ((1:x_count) * scale) - scale / 2;

Here's my brute force approach:

y = repmat(y, x_count, 1);
y = reshape(y, 1, size(y, 1) * size(y, 2));
y = y';

x = repmat(x, y_count, 1);
x = x';
x = reshape(x, 1, size(x, 1) * size(x, 2));
x = x';

points = [x y];

It works, but it isn't very elegant.


Basic API wrapper around a Restful service I'm writing a basic wrapper around a restful service that returns a list of Stores. I'm using RestSharp which I'm injecting into the class.

Naturally, I want to make it completely testable (that's a major requirement!).

I'm also thinking that it could be more generic i.e. GetStores but I'm struggling how to do as I need to specify things like the RootElement.

public class TopManClient : ITopManClient
{
    private readonly IRestClient _restClient;
    private const string BaseUrl = "https://TopmanStores.Api.com/";
    private const string AcceptTenant = "uk";
    private const string AcceptLanguage = "en-GB";

    public TopManClient(IRestClient restClient)
    {
        _restClient = restClient;

        _restClient.BaseUrl = new Uri(BaseUrl);
        _restClient.AddDefaultHeader("Accept-Tenant", AcceptTenant);
        _restClient.AddDefaultHeader("Accept-Language", AcceptLanguage);
    }

    public IRestRequest GetRequest(string url)
    {
        return new RestRequest(url, Method.GET);
    }

    public List<Store> GetStores(string postCode)
    {
        var request = GetRequest("stores");
        request.RootElement = "Stores";
        request.AddQueryParameter("q", postCode);

        var response = _restClient.Execute<List<Store>>(request);

        if (response.ErrorException != null)
        {
            const string message = "Error retrieving response. Check inner details for more info.";
            throw new ApplicationException(message, response.ErrorException);
        }

        return response.Data;
    }
}

I would personally prefer to use HttpClient directly as it provides full control and perfect testability - see here.

We can define our REST API as:

// see https://jsonplaceholder.typicode.com/
// this site exposes some REST API example to automate
public interface ITypicode
{
    Task<BlogPost[]> GetPostsAsync();
    Task<BlogPost> GetPostAsync(int id);
}

public class Typicode : JsonRestApi, ITypicode
{
    public Typicode(HttpMessageHandler handler = null)
        : base("https://jsonplaceholder.typicode.com", handler)
    {
    }

    public Task<BlogPost[]> GetPostsAsync() => GetAsync<BlogPost[]>("posts");
    public Task<BlogPost> GetPostAsync(int id) => GetAsync<BlogPost>($"posts/{id}");
}

public class BlogPost
{
    public int UserId { get; set; }
    public int Id { get; set; }
    public string Title { get; set; }
    public string Body { get; set; }
}

Where base class to simplify API definition is:

public abstract class JsonRestApi
{
    static readonly JsonSerializerSettings Settings = new JsonSerializerSettings()
    {
        ContractResolver = new CamelCasePropertyNamesContractResolver(),
        Converters = new [] { new StringEnumConverter() }
    };

    protected JsonRestApi(string baseAddress, HttpMessageHandler handler = null)
    {
        Client = handler == null ? new HttpClient() : new HttpClient(handler);
        Client.BaseAddress = new Uri(baseAddress);
        Client.DefaultRequestHeaders
            .Accept
            .Add(new MediaTypeWithQualityHeaderValue("application/json"));
    }

    protected async Task<T> GetAsync<T>(string uri) =>
        await ParseJson<T>(await Client.GetAsync(uri));

    protected async Task<T> PostAsync<T>(string uri, object data = null) =>
        await ParseJson<T>(await Client.PostAsync(uri, JsonContent.From(data)));

    protected HttpClient Client { get; }

    async Task<T> ParseJson<T>(HttpResponseMessage response) =>
        JsonConvert.DeserializeObject<T>(await response.Content.ReadAsStringAsync(), Settings);

    class JsonContent : StringContent
    {
        public static JsonContent From(object data) =>
            data == null ? null : new JsonContent(data);

        public JsonContent(object data)
            : base(JsonConvert.SerializeObject(data), Encoding.UTF8, "application/json")
        {
        }
    }
}

This code is fully testable.


Haskell CodeEval FizzBuzz CodeEval FizzBuzz

  Quick Description:

  Take 1 command line argument of a file-path. This file will contain
  test cases formatted as such 3 space separated numbers per line. For
  numbers X, Y and Z you must print out all numbers between 1 and
  Z, space separated, replacing all numbers divisible by X with 'F'
  and all numbers divisible by Y with 'B'.

I am entirely new to Functional programming, and my Haskell knowledge is from a single Pluralsight class, so I am looking for any amount of feedback, particularly for best practices. I also feel like the actual fizzBuzzSingle function can be solved in a list comprehension instead, but I'm still uncomfortable with the syntax so I didn't use one.

If my code is reading too object-oriented please let me know. I also did look up the other FizzBuzz questions on this site, but they all seem to not being handling file input, which seemed the hardest part to me, so it seemed worth posting this.

import System.Environment

fizzBuzzSingle :: Int -> Int -> Int -> String
fizzBuzzSingle f b n
    | n `mod` f == 0 && n `mod` b == 0  = "FB"
    | n `mod` f == 0                    = "F"
    | n `mod` b == 0                    = "B"
    | otherwise                         = show n

fizzBuzz :: (Int, Int, Int) -> [String]
fizzBuzz (f,b,end) = map (fizzBuzzSingle f b) [1..end]

convertInputLine :: String -> (Int, Int, Int)
convertInputLine x = packageInputs ((map read . words) x :: [Int])

packageInputs :: [Int] -> (Int, Int, Int)
packageInputs [f,b,end] = (f, b, end)

concatOutput :: [String] -> String
concatOutput (x : xs) =
    foldl (\x y -> x ++ " " ++ y) x xs

handleFizzBuzz :: [String] -> [String]
handleFizzBuzz [] = []
handleFizzBuzz (x : xs) =
    (concatOutput . fizzBuzz . convertInputLine $ x) : handleFizzBuzz xs

main :: IO ()
main = do
    args <- getArgs
    let path = args !! 0
    file <- readFile path
    putStrLn . unlines . handleFizzBuzz . lines $ file

Some functions in the Prelude can be of help. One is map, which allows you to write a simpler handleFizzBuzz.  Another is unwords, which is equivalent to your concatOutput.

handleFizzBuzz :: String -> String
handleFizzBuzz =
    map $ unwords . fizzBuzz . convertInputLine

main :: IO ()
main = do
    args <- getArgs
    let path = args !! 0
    file <- readFile path
    putStrLn . unlines . map handleFizzBuzz . lines $ file

Another is <>, the simple monoidal concat. As both String is a Monoid and (Monoid a) => Maybe a is a Monoid, so is Maybe String. It lets us eliminate two cases in fizzBuzzSingle if we also return a Maybe:

fizzBuzzSingle :: Int -> Int -> Int -> Maybe String
fizzBuzzSingle f b n =
    ("F" <$ guard (mod n f == 0)) <> ("B" <$ guard (mod n b == 0))

The <$ is a useful operator from Data.Functor. Its implementation is something like b <$ fa = fmap (\_ -> b) fa. guard returns a Nothing when our divisibility checks return False, which is perfect for us since that means "F" <$ Nothing evaluates to Nothing.

But, as we're now in Maybe-land, we need to discharge it. In the case where a number is divisible by either divisor, we just want to print the number. So we can turn to fromMaybe from Data.Maybe:

fizzBuzz :: [Int] -> [String]
fizzBuzz [f, b, end] =
    map (\n -> fromMaybe (show n) (fizzBuzzSingle f b n)) [1 .. end]

But we notice that the inner closure can be rewritten as \n -> (fromMaybe . show $ n) (fizzBuzzSingle f b $ n). That seems useless, but it's exactly like the implementation of the applicative sequencing operator for (->) r.

fizzBuzz :: [Int] -> [String]
fizzBuzz [f, b, end] =
    map (fromMaybe . show <*> fizzBuzzSingle f b) [1 .. end]

This is a pretty esoteric trick. Applicative is already somewhat of a stumper, but I think there's something about (->) r that makes its Applicative instance especially difficult to understand.

But, if we elect to use this trick and combine it with everything else, the program boils down to:

import Data.Monoid
import Data.Maybe
import Control.Monad
import System.Environment

fizzBuzzSingle :: Int -> Int -> Int -> Maybe String
fizzBuzzSingle f b n =
    ("F" <$ guard (mod n f == 0)) <> ("B" <$ guard (mod n b == 0))

fizzBuzz :: [Int] -> [String]
fizzBuzz [f, b, end] =
    map (fromMaybe . show <*> fizzBuzzSingle f b) [1 .. end]

main :: IO ()
main = do
    args <- getArgs
    file <- readFile (head args)
    (putStrLn . unlines . map (unwords . fizzBuzz . map read . words) . lines) file

I think there's something to be said for simplicity. I think your pattern matching makes for a more readable fizzBuzzSingle at the end of the day, though this implementation easily extends if you want to test three or four or five divisors. Anyway, monoids and functors and applicatives, oh my!

Credit is due: I saw this originally on /r/haskell.


String join of distinct string I have a class like this one:

class EmailClass
{
    public string MailAdresse { get; set; }
    public string MailAdresseCC { get; set; }
}

Through a JSON deserialization I obtain a List<EmailClass> and two other strings anotherMailAddress and justAnotherMailAddress.

I need to join all those adresses in one big string (the separator is ";"). For this purpose I wrote this code:

// this one is populated in another section but it
// is put there for the sake of knowing the  variable name
List<EmailClass> splittedList

List<string> listOfAdresses = new List<string>();
string joinedAdresses = String.Empty;

listOfAdresses.AddRange(splittedList.Select(x => x.MailAdresse));
listOfAdresses.AddRange(splittedList.Select(x => x.MailAdresseCC));
listOfAdresses.Add(anotherMailAddress);
listOfAdresses.Add(justAnotherMailAdresses);

joinedAdresses = String.Join(";", listOfAdresses.Distinct().ToArray());

Now, is there a more polished way to obtain this result?
How much will Distinct() affect the performance on medium result set (~100.000 adresses)? I've read that the complexity of the function should be O(n), is this true?

Use a HashSet then it will only contain unique values, duplicates will be discarded when calling Add based upon the hashcode of the string. This should be far more efficient than calling Distinct.

List<EmailClass> splittedList

HashSet<string> listOfAdresses = new HashSet<string>();

// HashSet does not contain an AddRange method.
foreach (var emailClass in splittedList)
{
    listOfAdresses.Add(emailClass.MailAdresse);
    listOfAdresses.Add(emailClass.MailAdresseCC);
}

listOfAdresses.Add(anotherMailAddress);
listOfAdresses.Add(justAnotherMailAdresses);

string joinedAdresses = String.Join(";", listOfAdresses.ToArray());

--

Following the "feedback" from Almaz, here's a basic benchmark to show the performance difference (using unique values for each address):

private static void Main(string[] args)
{
    List<EmailClass> splittedList = Enumerable.Range(1, 100000).Select(i => new EmailClass
    {
        MailAdresse = i.ToString() + "@email.com",
        MailAdresseCC = i.ToString() + "cc@email.com"
    }).ToList();

    OriginalMethod(splittedList);
    HashSetMethod(splittedList);
    LinqMethod(splittedList);

    Console.ReadLine();
}

private static void OriginalMethod(List<EmailClass> splittedList)
{
    var sw = new Stopwatch();
    sw.Start();
    List<string> listOfAdresses = new List<string>();

    listOfAdresses.AddRange(splittedList.Select(x => x.MailAdresse));
    listOfAdresses.AddRange(splittedList.Select(x => x.MailAdresseCC));
    listOfAdresses.Add("someone@email.com");
    listOfAdresses.Add("someone.else@email.com");
    var joinedAdresses = String.Join(";", listOfAdresses.Distinct().ToArray());
    sw.Stop();
    Console.WriteLine("OriginalMethod");
    Console.WriteLine(sw.Elapsed);
}

private static void HashSetMethod(List<EmailClass> splittedList)
{
    var sw = new Stopwatch();
    sw.Start();
    HashSet<string> listOfAdresses = new HashSet<string>();

    foreach (var emailClass in splittedList)
    {
        listOfAdresses.Add(emailClass.MailAdresse);
        listOfAdresses.Add(emailClass.MailAdresseCC);
    }

    listOfAdresses.Add("someone@email.com");
    listOfAdresses.Add("someone.else@email.com");

    string joinedAdresses = String.Join(";", listOfAdresses.ToArray());
    sw.Stop();
    Console.WriteLine("HashSetMethod");
    Console.WriteLine(sw.Elapsed);
}

private static void LinqMethod(List<EmailClass> splittedList)
{
    var sw = new Stopwatch();
    sw.Start();
    var emails = splittedList.SelectMany(emailClass => new[] { emailClass.MailAdresse, emailClass.MailAdresseCC })
        .Concat(new[] { "someone@email.com", "someone.else@email.com" })
        .Distinct();

    var joinedAdresses = String.Join(";", emails);
    sw.Stop();
    Console.WriteLine("LinqMethod");
    Console.WriteLine(sw.Elapsed);
}

Results run in release mode without debugger attached:

OriginalMethod: 00:00:00.0789540
HashSetMethod:  00:00:00.0488568
LinqMethod:     00:00:00.0668056

Ramping up to 1,000,000 items

OriginalMethod: 00:00:00.8189667
HashSetMethod:  00:00:00.6891028
LinqMethod:     00:00:01.0157357

As you can see, the HashSet approach is substantially faster than the OP and significantly faster than the Linq approach.


Made my own simple calculator So I made this calculator in c# console, I'm pretty nooby-ish in programming. I want to know if I can make it better, because my coding skills suck. Pretty sure my logic sucks too.

Any advice and tips?

   public static void Main() {
        bool returnToStart;

        do {
            returnToStart = Calc();

        } while(returnToStart == true);

        Console.ReadLine();

    }
    public static bool Calc() {
        ExitProgram();
        Console.Clear();

        float firstNumber; float secondNumber; float result;
        string operation;

        Console.Write("First number: ");
        firstNumber = float.Parse(Console.ReadLine());

        Console.Write("Choose your operator: addition(+), subtraction(-), " +
                     "multiplication(*) or division(/): ");
        operation = Console.ReadLine();

        Console.Write("Second number: ");
        secondNumber = float.Parse(Console.ReadLine());

        Console.Clear();

        switch(operation) {
            case "+":
                result = (firstNumber + secondNumber);
                Console.WriteLine("Calculation: {0}", result);
                break;
            case "-":
                result = (firstNumber - secondNumber);
                Console.WriteLine("Calculation: {0}", result);
                break;
            case "*":
                result = (firstNumber * secondNumber);
                Console.WriteLine("Calculation: {0}", result);
                break;
            case "/":
                if(firstNumber == 0) {
                    Console.WriteLine("Cant divide by zero!");
                } else {
                    result = (firstNumber / secondNumber);
                    Console.WriteLine(result);
                }
                break;

            default:
                Console.WriteLine("Error! Try again.");
                break;

        }
        return true;

    }
    public static bool ExitProgram() {
        string exit = "Exit";

        Console.WriteLine("If you're done with your calculations,\nthen you can exit this program by simply typing in 'Exit' ");
        Console.Write("Want to quit the program?: ");
        exit = Console.ReadLine();

        if(exit == "Exit") {
           Environment.Exit(0);

        }return true;

    }

}

}

Keep It Simple For the User
Rather than making the user type the word Exit it might be better to ask the user
Are you done yet(y/n)? and accept a simple yes/no answer. It also might be better if the answer was not case sensitive so accept Y, y, N and n.

Exit From Main() When Possible
The Main() function may contain clean up code so exiting from the ExitProgram() function may not be the best idea. A second problem I see with the ExitProgram() function is that it is called before the user ever enters a calculation. There are two possible ways to handle this, one would be to move the call to ExitProgram() to after the calculation is performed, the other would be to have a function that contains a do while loop that calls Calc within the loop and tests ExitProgram() in the while condition.

Function Complexity
The function Calc() is overly complex (it does too much) and should be multiple functions. One of the functions should get the user input, and a second function should do the calculation. This would be applying two programming principles, the Single Responsibility Principle and the KISS Principle.

The Single Responsibility Principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

The Keep It Simple (KISS) Principle is an engineering principle that predates computers, basically it is keep the implementation as simple as possible.


Light weight C++ Signals/Slots or Event System This assumes there is no need to aggregate results from the listeners of a broadcast signal.

Technically I guess the slots are whatever your std::function holds, so they aren't special. So I guess this is more of a Signals/Connections design.

The "connection" or Signal<T...>::Listener is the owner of the callback. It's ideal to have this owned by the object that has the callback function or on the object your lambda captures. That way when that object goes out of scope so does the callback, which prevents it from being called anymore.

You could dump listeners into a container on the object and forget about them. Or store specifically named instances of them which is very useful for switching the object you're listening to or disconnecting.

template <typename... FuncArgs>
class Signal
{
    using fp = std::function<void(FuncArgs...)>;
    std::forward_list<std::weak_ptr<fp> > registeredListeners;
public:
    using Listener = std::shared_ptr<fp>;

    Listener add(const std::function<void(FuncArgs...)> &cb) {
        // passing by address, until copy is made in the Listener as owner.
        Listener result(std::make_shared<fp>(cb));
        registeredListeners.push_front(result);
        return result;
    }

    void raise(FuncArgs... args) {
        registeredListeners.remove_if([&args...](std::weak_ptr<fp> e) -> bool {
            if (auto f = e.lock()) {
                (*f)(args...);
                return false;
            }
            return true;
        });
    }
};

usage

Signal<int> bloopChanged;

// ...

Signal<int>::Listener bloopResponse = bloopChanged.add([](int i) { ... });
// or
decltype(bloopChanged)::Listener bloopResponse = ...

// let bloopResponse go out of scope.
// or re-assign it
// or reset the shared_ptr to disconnect it
bloopResponse.reset();

Sink arguments (objects we want to make a copy of and store internally) are generally passed by value, then moved into place, so the add function can take its parameter by value rather than const&.
We could perhaps make fp public with a name like Function, and use it for the add function parameter.
raise will make unnecessary copies of the arguments. We can avoid that with perfect forwarding.
We don't need to copy the weak_ptr in the raise lambda, we can use a const&. We also don't need to specify the return value.

.

#include <forward_list>
#include <functional>
#include <memory>

template <class... FuncArgs>
class Signal
{
public:

    using Function = std::function<void(FuncArgs...)>;
    using Listener = std::shared_ptr<Function>;

    Listener add(Function cb) {
        auto const result = std::make_shared<Function>(std::move(cb));
        listeners.push_front(result);
        return result;
    }

    template<class... Args>
    void raise(Args&&... args) {
        listeners.remove_if([&args...](std::weak_ptr<Function> const& e) {
            if (auto f = e.lock()) {
                (*f)(std::forward<Args>(args)...);
                return false;
            }
            return true;
        });
    }

private:

    std::forward_list<std::weak_ptr<Function>> listeners;
};

#include <iostream>

struct Noisy
{
    Noisy() = default;

    Noisy(Noisy const&) { std::cout << "copy\n"; }
    Noisy& operator=(Noisy const&) { std::cout << "copy assign\n"; return *this; }

    Noisy(Noisy&&) { std::cout << "move\n"; }
    Noisy& operator=(Noisy&&) { std::cout << "move assign\n"; return *this; }
};

int main()
{
    {
        auto s = Signal<int>();
        auto const l = s.add([] (int v) { std::cout << "value: " << v << std::endl; });
        s.raise(5);
    }
    {
        auto s = Signal<Noisy const&>();
        auto const l = s.add([] (Noisy const&) { });
        auto const n = Noisy();
        s.raise(n);
    }
}


Two times same code in methode except one little line In Service.Insert(), I call DeviceRepo.Insert() and in Service.Update(), I call DeviceRepo.Update(). All my other code is exactly the same.

I'm thinking I can make one method that do the same thing update. I want to update and insert when I will insert a device.

The methods respectively insert a new or update an existing (IoT) device into my database with entity framework 6.

How could I refactor the code? You can see I have the same methods twice where only another method is called.

public class DeviceService : IDeviceService
{
    internal IGenericRepo<Device> _deviceRepo;

    public DeviceService()
    {
        _deviceRepo = new DeviceRepo(); // P.S.: DeviceRepo overrides GenericRepo<Device> who implements IGenericRepo<Device>
    }

    public void Insert(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
    {
        device.Framework = GiveObjects<Framework>(selectedFrameworks, _frameworkRepo);
        device.OS = GiveObjects<OS>(selectedOSs, _osRepo);

        SetStateUnchanged<Framework>(device.Framework);
        SetStateUnchanged<OS>(device.OS);

        _deviceRepo.Insert(device);
        _deviceRepo.SaveChanges();
    }

    public void Update(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
    {
        device.Framework = GiveObjects<Framework>(selectedFrameworks, _frameworkRepo);
        device.OS = GiveObjects<OS>(selectedOSs, _osRepo);

        SetStateUnchanged<Framework>(device.Framework);
        SetStateUnchanged<OS>(device.OS);

        _deviceRepo.Update(device);
        _deviceRepo.SaveChanges();
    }
}

GenericRepo<Device>:

public class GenericRepo<TEntity> : IGenericRepo<TEntity> where TEntity : class
{
    internal ApplicationDbContext context;
    internal DbSet<TEntity> dbSet;

    public GenericRepo()
    {
        context = new ApplicationDbContext();
        dbSet = context.Set<TEntity>();
    }

    public virtual TEntity Insert(TEntity entity)
    {
        return dbSet.Add(entity);
    }

    public virtual void Update(TEntity entityToUpdate)
    {
        context.Entry(entityToUpdate).State = EntityState.Modified;
        dbSet.Attach(entityToUpdate);
    }

    public virtual void SaveChanges()
    {
        context.SaveChanges();
    }
}

public class DeviceRepo : GenericRepo<Device>
{ /* not important methods I override for this question */ }

I think in this case I would simply make a private method to capture the duplicated code.  I probably wouldn't mind about the extra call to the SaveChanges() as it's only one line and removes any need for further complexity on the code.

public void Insert(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
{
    UpdateDeviceProperties(device, selectedFrameworks, selectedOSs);

    _deviceRepo.Insert(device);
    _deviceRepo.SaveChanges();
}

public void Update(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
{
    UpdateDeviceProperties(device, selectedFrameworks, selectedOSs);

    _deviceRepo.Update(device);
    _deviceRepo.SaveChanges();
}

private void UpdateDeviceProperties(Device device, IEnumerable<int> selectedFrameworks, IEnumerable<int> selectedOSs)
{
    device.Framework = GiveObjects<Framework>(selectedFrameworks, _frameworkRepo);
    device.OS = GiveObjects<OS>(selectedOSs, _osRepo);

    SetStateUnchanged<Framework>(device.Framework);
    SetStateUnchanged<OS>(device.OS);
}

I might also consider returning either a boolean (or some other error result) from the methods to raise awareness of whether the db action actually succeeded to help with raising failures back to the caller of the method.

In the case of Insert it could be as simple as (assuming device has an Id of course) return device.Id > 0;


Exponential backoff generator Exponential backoff in the context of various networking protocols looks something like this:

  When a collision first occurs, send a “Jamming signal” to prevent further data being sent.
  Resend a frame after either 0 seconds or 51.2μs, chosen at random.
  If that fails, resend the frame after either 0s, 51.2μs, 102.4μs, or 153.6μs.
  If that still doesn't work, resend the frame after k · 51.2μs, where k is a random integer between 0 and 23 − 1.
  In general, after the cth failed attempt, resend the frame after k · 51.2μs, where k is a random integer between 0 and 2c − 1.

I've written a generator that handles this:

def exponential_backoff(k):
    num_failed = 0
    while True:
        suceeded = yield k*random.randint(0, 2**num_failed-1)
        num_failed = (num_failed + 1) if not suceeded else 0

Usage:

backoff_generator = exponential_backoff(TIME_FRAME)
try:
    send_message("super cool message")
except MessageSendFailed:
    time.sleep(backoff_generator.send(False))
else:
    backoff_generator.send(True)

Does this seem like a reasonable way to handle things? The goal was to have a simple method of getting the amount of time to wait, without having to maintain too much state in the application itself, without adding an unreasonable amount of extra processing time, and without too much kruft.

A few suggestions:

Possibly I’m using the given code incorrectly, but it doesn’t seem to work for me. I defined a send_message() function that would always fail:

class MessageSendFailed(Exception):
    pass

def send_message(msg):
    raise MessageSendFailed

but when I run it, it immediately fails with the following error:

$ python expofailed.py
Traceback (most recent call last):
  File "expofailed.py", line 29, in <module>
    time.sleep(backoff_generator.send(False))
TypeError: can't send non-None value to a just-started generator

Alternatively, if I have a copy of send_message() that never throws an error, I get the same error.
I’m not a big fan of the foo = bar if condition else baz style of Python ternary operator, because it tends towards unreadability by cramming everything onto a single line. I prefer splitting it into an explicit if block like so:

if succeeded:
    num_failed = 0
else:
    num_failed += 1

And now you extend those branches more easily, and write a comment about why each branch behaves it does (because it’s not entirely obvious to me).
Use a better variable name than k as the argument to your function – perhaps interval? Don’t skimp on variable names – characters are cheap.
Your generator should have a docstring and a comment.
You’ve misspelt “succeeded”.


Is there a simpler way to write a row from one table to another? I have the following code:

const String sqlSelect = "SELECT * FROM UserPasswords WHERE username='System Administrator';";
const String sqlInsert = "INSERT INTO UserPasswords VALUES (@username,@Password,@startDate,@Expired,@UserPasswordsID)";
using (var con1 = new SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings["dataConnectionString"].ConnectionString))
using (var con2 = new SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings["utilConnectionString"].ConnectionString))
{
  con1.Open();
  con2.Open();
  using (var selectCommand = new SqlCommand(sqlSelect, con1))
  {
    using (var reader = selectCommand.ExecuteReader())
    {
      if (reader.Read())
      {
        using (var insertCommand = new SqlCommand(sqlInsert, con2))
        {
          insertCommand.Parameters.Add(new SqlParameter("@username", reader.GetString(0)));
          insertCommand.Parameters.Add(new SqlParameter("@Password", reader.GetString(1)));
          insertCommand.Parameters.Add(new SqlParameter("@startDate", reader.GetDateTime(2)));
          insertCommand.Parameters.Add(new SqlParameter("@Expired", reader.GetBoolean(3)));
          insertCommand.Parameters.Add(new SqlParameter("@UserPasswordsID", reader.GetInt32(4)));
          insertCommand.ExecuteNonQuery();
        }
      }
    }
  }

All I need to do is copy the row with the 'System Administrator' info into another table as a backup. Though this code works is there someway to do this more elegantly?

Transferring Data Using SqlBulkCopy

private static void PerformBulkCopy()
{
    string connectionString =
            @"Server=localhost;Database=Northwind;Trusted_Connection=true";
    // get the source data
    using (SqlConnection sourceConnection =
            new SqlConnection(connectionString))
    {
        SqlCommand myCommand =
            new SqlCommand("SELECT * FROM Products_Archive", sourceConnection);
        sourceConnection.Open();
        SqlDataReader reader = myCommand.ExecuteReader();

        // open the destination data
        using (SqlConnection destinationConnection =
                    new SqlConnection(connectionString))
        {
            // open the connection
            destinationConnection.Open();

            using (SqlBulkCopy bulkCopy =
            new SqlBulkCopy(destinationConnection.ConnectionString))
            {
                bulkCopy.BatchSize = 500;
                bulkCopy.NotifyAfter = 1000;
                bulkCopy.SqlRowsCopied +=
                    new SqlRowsCopiedEventHandler(bulkCopy_SqlRowsCopied);
                bulkCopy.DestinationTableName = "Products_Latest";
                bulkCopy.WriteToServer(reader);
            }
        }
        reader.Close();
    }
}

Solution #2: stored procedure


Displaying data from JSON in a table What do you think of my code?  I know it is a little rough around the edges but as of two days ago I had no idea how to use jQuery or JavaScript.

$(document).ready(function () {

var json = {

    "205100": {
        "success": true,
            "data": {
            "type": "game",
                "name": "MasterKin",
                "steam_appid": 9000,
                "required_age": 40,
                "is_free": false,
                "controller_support": "full",
                "dlc": [212894, 212893, 208575, 208570]
        }
    }

};

var game_name = [];
for (var key in json) {
    if (json.hasOwnProperty(key)) {
        var item = json[key];
        game_name.push({
            ItemName: item.data.name //Changing the .name to .dlc or .type will then display that result

        });
    }
}
console.log(game_name);
var tr;
for (var i = 0; i < game_name.length; i++) {
    tr = $('
');
    tr.append("" + game_name[i].ItemName + "	");

    $('table').append(tr);
}
});

        Name

I don't know what the possible rest of your code looks like, so my opinion might be wrong. What I don't understand is why a lot of people use jQuery for the slightest bits nowadays. The display of the array could be done like this:

var table = document.getElementById("displayTable"); //give this ID to your table

for (var i = 0; i < game_name.length; i++) {
    var row = table.insertRow(i);
    var cell = row.insertCell(0);
    cell.innerHTML = game_name[i].ItemName;
}

Update:

If you insist on using jQuery, cache the selector in a variable and re-use the variable. Overusing selectors can result in poor performance and since you're using the selector inside a loop this can easily occur.

var table = $('#displayTable');

for (var i = 0; i < game_name.length; i++) {
    table.append("
" + game_name[i].ItemName + "
");
}

Furthermore, give useful names to variables. Names like tr or game_name are not meaningful, try tableRow and gameNames instead. I updated the JSFiddle working example.