Opening social media links in a new window I'm rather new to jQuery but managed to get some social links to open in a new controlled window instead of new tab. I have a few functions that do the same thing, and this works, but there is always a better method.

I did review the question and answer here and couldn't figure out which would apply to this case.

jQuery(document).ready(function ($) {
  $("#ssba_facebook_share").attr('title','opens in a new pop-up');
  $("#ssba_facebook_share").click(function(){
    w = parseInt((screen.width - 600)/2); h = parseInt((screen.height - 400)/2);
    cwin = window.open($(this).attr('href'), 'closewin',
   'status=0,toolbar=0,location=0,menubar=0,directories=0,resizable=0,scrollbars=0,height=400,width=600');
    cwin.moveTo(w,h);
    return false;
  });
  $("#ssba_twitter_share").attr('title','opens in a new pop-up');
  $("#ssba_twitter_share").click(function(){
    w = parseInt((screen.width - 600)/2); h = parseInt((screen.height - 400)/2);
    cwin = window.open($(this).attr('href'), 'closewin',
   'status=0,toolbar=0,location=0,menubar=0,directories=0,resizable=0,scrollbars=0,height=400,width=600');
    cwin.moveTo(w,h);
    return false;
  });
  $("#ssba_google_share").attr('title','opens in a new pop-up');
  $("#ssba_google_share").click(function(){
    w = parseInt((screen.width - 600)/2); h = parseInt((screen.height - 400)/2);
    cwin = window.open($(this).attr('href'), 'closewin',
   'status=0,toolbar=0,location=0,menubar=0,directories=0,resizable=0,scrollbars=0,height=400,width=600');
    cwin.moveTo(w,h);
    return false;
  });
});

Now, I've attempted a few things to no avail, and one method was this:

jQuery(document).ready(function ($) {
  $("#ssba_facebook_share","#ssba_twitter_share","#ssba_google_share").attr('title','opens in a new pop-up');
    $(this).click(function(){
      w = parseInt((screen.width - 600)/2); h = parseInt((screen.height - 400)/2);
      cwin = window.open($(this).attr('href'), 'closewin',
      'status=0,toolbar=0,location=0,menubar=0,directories=0,resizable=0,scrollbars=0,height=400,width=600');
      cwin.moveTo(w,h);
      return false;
    });
});

Does anyone have any pointers in this, if possible, with a description of where I went wrong? Or even post a link to a question that goes over this in laymen terms? (I've been looking around and found similar posts, which is how I arrived at where I am now, but still having trouble making complete sense of it.)

First of all, make sure you declare your variables using var:

  assigning a value to an undeclared variable implicitly declares it as
  a global variable (it is now a property of the global object)

Personally, I'd probably add a class to the links in question like shareLink then you could simply do:

$(".shareLink") to select all three links

Alternately, you could try selecting by id ending in _share assuming that quality is unique to these three links:

$("[id$='_share']")

I'd also suggest chaining so that your whole code would be something like this:

$("[id$='_share']").click(function(){
    var w = parseInt((screen.width - 600)/2)
    ,   h = parseInt((screen.height - 400)/2)
    ,   cwin = window.open($(this).attr('href'), 'closewin',
               'status=0,toolbar=0,location=0,menubar=0,directories=0,
               resizable=0,scrollbars=0,height=400,width=600');
    cwin.moveTo(w,h);
    return false;
  }).attr('title','opens in a new pop-up');

Finally, I'd review the documentation on Window.open to be sure you don't want to provide left or top parameters, instead of immediately moving the window, or even a different solution. Make sure you at least read the section on Usability issues


Brainfuck interpreter in F# I have some concerns, like the updateValue function. I was trying to follow the functional paradigm, but I wonder if I could use another approach or something.

My parsing function seems overly complicated, particularly the '[' case seems complicated. Any other comments are appreciated too!

namespace Brainfuck

module Interpreter =
    type public ProgramState =
        { Pointer : int
          Memory : Map<int, int> }

    let private currentValue state = Map.find state.Pointer state.Memory

    let private updateValue state op =
        let updateMemory =
            match Map.tryFind state.Pointer state.Memory with
            | Some(x) ->
                let m = Map.remove state.Pointer state.Memory
                Map.add state.Pointer (op x) m
            | None -> Map.add state.Pointer (op 0) state.Memory
        { state with Memory = updateMemory }

    type private Command =
        | SimpleCommand of char
        | Loop of Command list

    let private parse code =
        let literals = [ '+'; '-'; '>'; '<'; '.'; ',' ] |> Set.ofList

        let rec parsing =
            function
            | x :: xs when x = '[' ->
                let innerLoop, innerRemaining = parsing xs
                let commands, remaining = parsing innerRemaining
                let remainingCommands, after = parsing remaining
                Loop(innerLoop) :: commands @ remainingCommands, after
            | x :: xs when x = ']' -> [], xs
            | x :: xs when Set.contains x literals ->
                let commands, remaining = parsing xs
                SimpleCommand(x) :: commands, remaining
            | x :: xs -> parsing xs
            | [] -> [], []
        code
        |> Seq.toList
        |> parsing
        |> fst

    let private interpretSimpleCommand state command =
        match command with
        | '>' -> { state with Pointer = (state.Pointer + 1) }
        | '<' -> { state with Pointer = (state.Pointer - 1) }
        | '+' -> updateValue state (fun x -> x + 1)
        | '-' -> updateValue state (fun x -> x - 1)
        | '.' ->
            printf "%c" <| (char) (currentValue state)
            state
        | ',' -> updateValue state (fun x -> System.Console.Read())
        | _ -> failwith "invalid command"

    let private compute bytecode =
        let rec cmp state cmd =
            match cmd with
            | SimpleCommand(x) -> interpretSimpleCommand state x
            | Loop(xs) ->
                let nextState = computeProgram state xs
                if currentValue (nextState) = 0 then nextState
                else cmp nextState cmd

        and computeProgram = List.fold cmp
        computeProgram { Pointer = 0
                         Memory = Map.empty<int, int> } bytecode

    let public eval code =
        code
        |> parse
        |> compute

let private currentValue state = Map.find state.Pointer state.Memory

This won't work correctly if you try to access memory that hasn't been set yet. Doing that should return 0, but will instead throw an exception. You can fix that by using Map.tryFind together with defaultArg:

let private currentValue state = defaultArg (Map.tryFind state.Pointer state.Memory) 0

let m = Map.remove state.Pointer state.Memory
Map.add state.Pointer (op x) m

You don't need to remove before adding the key back in, because add will overwrite the old value. (The documentation isn't clear on that.)

This also means you can make your code more DRY by using currentValue:

let updateMemory =
    Map.add state.Pointer (op (currentValue state)) state.Memory

let rec parsing =

I think that parsing is a weird name for a function, function names should be verbs. Since you're already using parse, if you can't figure out any better name, you could use something like parseInternal or parse'.

| x :: xs when x = '[' ->
    let innerLoop, innerRemaining = parsing xs
    let commands, remaining = parsing innerRemaining
    let remainingCommands, after = parsing remaining
    Loop(innerLoop) :: commands @ remainingCommands, after

This code doesn't make any sense to me. And testing it on Wikipedia's Hello World program gives me incorrect results. Instead, I think it should look like this:

| x :: xs when x = '[' ->
    let innerLoop, innerRemaining = parsing xs
    let commands, remaining = parsing innerRemaining
    Loop(innerLoop) :: commands, remaining

code
|> Seq.toList
|> parsing
|> fst

This means that if you encounter unmatched ], you're going to treat it as the end of the file. Is that the correct behavior?

And thinking about unmatched brackets some more, unmatched [ are automatically closed at the end of the input. Is that correct?

(fun x -> x + 1)

You can shorten this to: ((+) 1).

let private compute bytecode =

I think that bytecode is a misleading name here, since it's not a code composed of bytes, it's an AST.

| Loop(xs) ->
    let nextState = computeProgram state xs
    if currentValue (nextState) = 0 then nextState
    else cmp nextState cmd

This is not correct. Brainfuck loops are equivalent to while loops, not do while loops. The correct code would be something like:

| Loop(xs) ->
    if currentValue state = 0
    then state
    else
        let nextState = computeProgram state xs
        cmp nextState cmd


Looking for Interstitial Array Join that works like String Join I'm looking for an array join that will return an array with an element inserted between consecutive elements of an input array.

string[] input = { "red", "white", "blue" };
string[] result = InterstitialJoin("or", input);
// { "red", "or", "white", "or", "blue" }

If I use string.Join, then the output is a string, but I want the output to be an array.

Not quite what I want:

string.Join("or", input); // redorwhiteorblue

Does this already exist?  Is there a more straightforward or built-in way of doing it?

I could hand-roll a function to do this, but it seems like it would require you to write all the code to create an enumerator and yield elements interleaving them with the interstitial element.

e.g.

static IEnumerable<T> InterstitialJoin<T>(T interstitial, IEnumerable<T> elements)
{
    var e = elements.GetEnumerator();
    if (!e.MoveNext()) yield break;
    yield return e.Current;
    while (e.MoveNext()) {
        yield return interstitial;
        yield return e.Current;
    }
}

or directly with an array like this:

static T[] InterstitialArrayJoin<T>(T interstitial, T[] elements) {
    T[] results = new T[elements.Length + elements.Length - 1];
    int iResults = 0;
    int iElements = 0;
    while (iElements < elements.Length) {
        if (iElements != 0) results[iResults++] = interstitial;
        results[iResults++] = elements[iElements++];
    }
    return results;
}

Maybe this is the simplest code to insert an element between each consecutive elements of an input array using System.Linq query:

string[] input = { "red", "white", "blue" };
string[] result = input.SelectMany(x => new [] { "or", x }).Skip(1).ToArray();
// { "red", "or", "white", "or", "blue" }

Steps:

.SelectMany(...): "Projects each element of a sequence to an IEnumerable and flattens the resulting sequences into one sequence." - see Documentation
.Skip(1): We have to remove first "or" delimiter, because we added it before each element and we do not want it before a first element.
.ToArray(): Convert IEnumerable to string[]. If you want, you can call .ToList() method instead to receive List<string>

I do not know about any built-in method or a more straightforward way to achieve it.

See working example on dotnetfiddle.net

I hope it will help you.


C - Improving (really) big number addition I was lucky enough to stumble into this task during a job interview.  I was gently guided to use string reversals and a few other things to speed up my process.  I kept thinking that I wouldn't have done it the way I was coached.   It bothered me enough that I went home and coded up my version.

How could I improve this code?

* EDIT * added a size_t for carry.  Reduced the number of calles of strlen().

Rules:

Program must be in C
Can't use atoi, and only basic types
Must be able to add arbitrarily large numbers
Output must be a string

Example usage:

addbig 12345678901234567891234567891234567890123456789123456789  9876543210987654321098765432198765432109876543210987654321

Here's the code I came up with:

#include <stdio.h>   // printf
#include <stdlib.h>  // malloc
#include <string.h>  // strlen

char * addStrings(char* string1, char* string2) {
  char *maxString = string1;
  char *minString = string2;

  // Override our larger if we were wrong.
  if (strlen(string1) < strlen(string2)) {
    maxString = string2;
    minString = string1;
  }

  int maxSize =  strlen(maxString);

  // Allocate enough storage to include our carry and the null char at the end.
  char * myResult = malloc(maxSize + 2);

  // Null terminate the string
  myResult[maxSize + 1] = 0;

  size_t carry = 0;
  int mindex = strlen(minString) - 1;

  char x = 0;

  // One loop.  Avoid -std=c99 flag
  int i = maxSize - 1;
  for (; i >= 0; i--) {
    // Default our char
    x = maxString[i] - '0';
    // We still have something left to add.
    if (mindex >= 0){
      x = x + (minString[mindex] - '0');
      mindex--;
    }

    if (carry) x++;

    if (x >= 10) {
      x -= 10;
      carry = 1;
    } else {
      carry = 0;
    }

    myResult[i + 1] = x + '0';
  }
  // Take care of any leftover carry.
  if (carry) myResult[0] = '1';

  return myResult;
}  // END addStrings()

int main(int argc, char **argv) {
  if (argc < 2) {
    printf("Usage: %s num1 num2\n", argv[0]);
    printf("Utility for big numbers.");
    printf("Adds num1 to num2, for any length of numbers.\n");
    return 1;
  }

  char * result = addStrings(argv[1], argv[2]);

  // Removing the initial space allocated by storage if we didn't overflow
  printf("%s\n", (result[0] == ' ') ? result + 1 : result);

  if (result) {
    free(result);
    return 0;
  }

  // Returned an error state, memory not freed.
  return -1;
}

Function Error

What is myResult[0] when carry == 0?

if (carry) myResult[0] = '1';

Improvements

As code is not changing the the contents of the string, use const for potential compiler optimization and clarity to reviewers that code is in fact not changing the characters.

// char * addStrings(char* string1, char* string2)
char * addStrings(const char* string1, const char* string2)

Code that allocates memory should clearly say that as a comment and/or function name.

// Returns an allocated string representing the sum of ....
char * addStrings(
// or
char * addStrings_alloc(

Really big strings can exceed length INT_MAX.  Use size_t for indexing arrays and string lengths.  size_t is the return type of strlen().

// int maxSize =  strlen(maxString);
size_t maxSize =  strlen(maxString);

// int i = maxSize - 1;
// for (; i >= 0; i--) {
size_t i;
for (i = maxSize; i > 0; ) {
  i--;

Check the return value of malloc()

char * myResult = malloc(maxSize + 2);
if (myResult == NULL) return NULL;

No need for carry to besize_t`.

// size_t carry = 0;
int carry = 0;

No provision for - numbers. (or ones with a leading '+')
No provision for trimming leads '0' in answer with input like addStrings("007", "007")
No provision for non-digits as in addStrings("x", "007")
Simplification

   // x = maxString[i] - '0';
   // if (carry) x++;
   x = maxString[i] - '0' + carry;

Below processing should have happened in the function.

  // Removing the initial space allocated by storage if we didn't overflow
  ... (result[0] == ' ') ? result + 1 : result)

Minor

Nomenclature.  size is the size need for the array.  length is the length of the string (not counting the null character).  Suggest maxLength.

// int maxSize =  strlen(maxString);
int maxLength =  strlen(maxString);

Why the comment? Seems unneeded  // Avoid -std=c99 flag
No provision for NULL input  addStrings(NULL, "007") - but then NULL is not a valid string.  For an interview question, I'd at least suggest that inputs may need sanitizing before use.
Style: Even single if() are easier to debug/maintain with {}

// if (carry) myResult[0] = '1';
if (carry)  {
  myResult[0] = '1';
}

Test driver simplification.  free(NULL) is OK.

// if (result) {
//  free(result);
//  return 0;
//}
free(result);

Test driver function error - off by 1

// if (argc < 2) {
if (argc < 3) {
// or better
if (argc != 3) {

I'd expect code to be tested and give a reasonable (or a least defined) result with addStrings("", "7") and addStrings("", "")


Is this a sensible way of using an IoC container? I have a Factory class for a queuing system I'm playing around with.

Consuming classes only care that they get a particular interface (Job) as a result of calling the factory's load method.

Some Jobs may require a database connection, so I want to inject a PDO instance. Others may require something else, and I don't want to give all Jobs a PDO instance regardless of whether they need it.

This is what I've ended up with:

IoC class:

use Queue\Job\MeterData;
use PDO;

class IoC
{

    const DATABASE = 'db';

    const METER_DATA_JOB = 'job_meterData';

    public function get($item)
    {
        switch ($item) {

            case self::DATABASE:
                return new PDO('');

            case self::METER_DATA_JOB:
                return new MeterData($this->get(self::DATABASE));

        }
    }

}

Resulting Queue Factory class:

namespace Queue\Job;

use IoC;

class Factory
{

    const METER_DATA = 'meter-data';

    private $ioc;

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

    public function load($job)
    {
        switch ($job) {

            case self::METER_DATA:
                return $this->ioc->get(IoC::METER_DATA_JOB);

            default:
                throw new \Exception('No job class found for ' . $job);

        }
    }

}

In my mind having the Factory isn't strictly necessary but is advantageous, as I can doc-comment the load method and it will ensure that consuming code knows what type of object it is dealing with. It'll also keep all queue jobs together neatly, rather than having the consuming code access the IoC container directly instead of the more specific Queue Factory.

The problem is, if we assume that IoC container is going to be one class for a full application, isn't it going to get very messy very quickly, with a ton of import statements and such?

Is that really such an issue, because at least it will be limited to just the IoC container and it will keep the rest of the application clear?

No. What you are doing is called The Service Locator Antipattern. And it's a bad thing.

The basic rule of thumb: Don't inject the container!.

The IoC container is supposed to be some sort of overseer, wiring your objects together at the bootstrap and configuration level. It isn't an object that's supposed to be injected into other objects, to create objects there.


My own algorithm to sort integers I'm studying the C language with a Harvey Deitel book.

Trying to solve an exercise that asked to write a good sorting algorithm, I did this:

void sort(int v[], int d){

int max = 0;
for(int i = 0; i < d; i++){
    if(v[i] > max)max = v[i];
}

int v2[max+1];
int v3[max+1];
for(int i = 0; i < max+1; i++){
    v2[i] = -1;
    v3[i] = 0;
}

for(int i = 0; i < d; i++){
    if(v3[v[i]] == 0){
        v2[v[i]] = v[i];
        v3[v[i]]++;
    }
    else {
        v3[v[i]]++;
    }
}

int j = 0;
for(int i = 0; i < max+1; i++){
    if(v2[i] != -1){
        for(int k = 0; k < v3[i]; k++){
            v[j] = v2[i];
            j++;
        }
    }
}
}

I would do six changes:

Better variable names, especially for the arguments
Remove unnecessary temporary array
Handle negative numbers
Initialize temporary array with memset instead of a loop.
Comments with pre and post conditions
Dynamically allocate the temporary array to avoid problems with the stack for large arrays.

The code looks like this:

/* Preconditions:
   array is a pointer to the array that should be sorted
   length is the number of elements in the array

   Postconditions:
   array is sorted
*/
void sort(int *array, size_t length) {
    if(!array || length<1) return;

    int max = array[0];
    int min = max;

    for (int i = 0; i < length; i++) {
        if(max < array[i]) max = array[i];
        if(min > array[i]) min = array[i];
    }

    const size_t range = max - min + 1;
    const size_t size = range * sizeof *array;
    int *tmp = malloc(size);
    if(!tmp) { /* Handle allocation error */ }
    memset(tmp, 0, size);

    for (int i = 0; i < length; i++) tmp[array[i] - min]++;

    int index = 0;
    for (int i = 0; i < range; i++) {
        for (int j = 0; j < tmp[i]; j++) {
                array[index] = i + min;
                index++;
        }
    }
    free(tmp);
}


UITableViewDataSource with different sections and SearchController I have a UIViewController holding one UITableView with 3 sections. Also this UIViewController implements UISearchResultsUpdating for searching in that UITableView. The UIViewController looks like this:

class MyViewController: UIViewController, UITableViewDataSource, UITableViewDelegate, UISearchResultsUpdating {

    var searchController = UISearchController(searchResultsController: nil)

    @IBOutlet weak var tableView: UITableView!

    private struct TableViewSections {
        static let sectionOne = 0
        static let sectionTwo = 1
        static let sectionThree = 2
    }

    override func viewDidLoad() {
        super.viewDidLoad()

        self.setupTableView()
        self.setupSearchController()
    }

    private func setupSearchController() {
        self.searchController.searchResultsUpdater = self
        self.searchController.dimsBackgroundDuringPresentation = false
        self.searchController.searchBar.placeholder = "Search"
        self.searchController.searchBar.autocapitalizationType = .sentences
        self.searchController.hidesNavigationBarDuringPresentation = false
        if #available(iOS 11.0, *) {
            self.navigationItem.searchController = self.searchController
        } else {
            self.tableView.tableHeaderView = self.searchController.searchBar
        }
    }

    private func setupTableView() {
        self.tableView.delegate = self
        self.tableView.dataSource = self
    }

    func numberOfSections(in tableView: UITableView) -> Int {
        return 3
    }

    func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {

        // In each of these cases i will do the work that is needed to return the correct number of rows
        // Just for demonstration i return a random int in every case

        if self.searchController.isActive {
            switch section {
            case TableViewSections.sectionOne:
                return 10

            case TableViewSections.sectionTwo:
                return 20

            case TableViewSections.sectionThree:
                return 30

            default:
                return 0
            }
        } else {
            switch section {
            case TableViewSections.sectionOne:
                return 15

            case TableViewSections.sectionTwo:
                return 25

            case TableViewSections.sectionThree:
                return 35

            default:
                return 0
            }
        }
    }

    func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {

        // In each of these cases i will do the work that is needed to show the correct content
        // Just for demonstration i return UITableViewCell() in every case

        switch indexPath.section {
        case TableViewSections.sectionOne:

            if self.searchController.isActive {
                return UITableViewCell()
            } else {
                return UITableViewCell()
            }

        case TableViewSections.sectionTwo:

            if self.searchController.isActive {
                return UITableViewCell()
            } else {
                return UITableViewCell()
            }

        case TableViewSections.sectionThree:

            if self.searchController.isActive {
                return UITableViewCell()
            } else {
                return UITableViewCell()
            }

        default:
            return UITableViewCell()
        }
    }

    // MARK: UISearchResultsUpdating

    func updateSearchResults(for searchController: UISearchController) {
        // do the search work here...
    }
}

You can see the difference between tableView(_:numberOfRowsInSection:) and cellForRow(at:).

My Question now is: What is recommended way to do so? Is the better solution tableView(_:numberOfRowsInSection:), cellForRow(at:) or is there even a better solution?

The numberOfRowsInSection: function looks good to me, changing this function to something better will depend on the possible logic you can apply in combination with the section parameter and your data. For example, if you have an array of dictionaries like this:

let data = [ 1: ["hola", "hey"], 2: ["adiós"] ]

The implementation of the method could be reduced to:

func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
    if self.searchController.isActive {
        let array = searchData[section]
        return array.count
    }
    let array = data[section]
    return array.count
}

That indeed is valid for five sections and for one millions sections.

For the cellForRowAt indexPath: function, I would put first the check for the searching status:

if self.searchController.isActive {
    // handle sections for searching
} else {
    // handle sections normaly
}

Doing it like this will allows you to, for example, group functionality shared across multiple sections. An implementation taking the example I put could be like:

func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
    let text = ""
    if self.searchController.isActive {
        let array = searchData[indexPath.section]
        text = array[indexPath.row]
    } else {
        let array = data[indexPath.section]
        text = array[indexPath.row]
    }

    configureCell(with: text, at: indexPath)
}

I did this because I prefer to not overload so much the implementation of the delegates. In configureCell:at: you must implement the logic needed for the creation of the cells based in sections and the data you received.

As I wrote before, the implementation of those methods will depends on the structure of your data, it could reduce or increase the amount of code you need to show the data in the intended way.


Random password generator I just wrote my first Java class, and I wanted to share it with you and maybe get some quick check from more experiences coders than I am.

I want to learn OOP, and Java is just the tool I thought was the best to start with (it's very similar to C in syntax, which I'm used to, and it's spreading more and more, day by day).

You can obviously comment on Java bad habits and ways to improve the class, but consider the class structure and the abstraction of the problem I wanted to solve, the separation between in data methods, etc.. as my main point of concern.

Here's the class. The aim is to create a random password generator.

import java.util.Random;

public final class PasswordGenerator {

  // DATAS
    // characters with which the password will be composed
    private static final int charactersSize = 100;
    private static char [] characters = new char [charactersSize];

    // keep the counts of used characters
    private static int charactersCount = 0;

    // size of the password to generate
    private int passwordSize;

  // CONSTRUCTOR
    public PasswordGenerator( int passwordSize ) {

      // set the password size
      this.passwordSize = passwordSize;

      // set the characters that will be used to generate the password
      initCharacters();
    }

  // METHODS
    // fill the array of characters that will be used to generate the password
    private static char [] initCharacters() {
      int i = 0;

      // add 0-9
      for ( int j = 48; j < 58; ++i, ++j, ++charactersCount ) {
        characters[i] = (char) j;
      }

      // add @ + a-z
      for ( int j = 64; j < 91; ++i, ++j, ++charactersCount ) {
        characters[i] = (char) j;
      }

      // add A-Z
      for ( int j = 97; j < 123; ++i, ++j, ++charactersCount ) {
        characters[i] = (char) j;
      }

      return characters;
    }

    // generate a random password
    public char [] get() {

      // initialize the random number generator
      Random rnd = new Random();

      char [] password = new char [passwordSize];

      // choose a random character from the array
      for ( int i = 0; i < passwordSize; ++i ) {
        password[i] = characters[ rnd.nextInt(charactersCount) ];
      }

      return password;
    }

  // DEBUG METHODS
    // show the characters the will be used to compose the pass
    public void showCharacters() {
      for ( int i = 0; i < charactersCount && characters[i] != 0; ++i ) {
        System.out.println(characters[i]);
      }
    }

  // MAIN - testing code
    public static void main(String[] args) {
      int passwordSize = 10;
      PasswordGenerator password = new PasswordGenerator( passwordSize );
      System.out.println( password.get() );
    }

}

Your code does have its share of C-isms, but we will be able to fix that.

But first, if you are learning Java in order to understand Object Oriented Programming, pick a better language. Java's OO primitives are classes, abstract classes, interfaces and single inheritance. Since Java was designed, the world has moved on, and trait-based OO (instead of interfaces) is all the rage. The Scala language isn't very C-like, but has a wonderful object system. Another interesting language for OO is Ruby with it's metaclasses.

I assume you already know about the difference of static and non-static fields and methods. Whatever is marked as static belongs to the class and is shared among all objects of that class. Your fillCharacters static method only operates on the class level, yet you call it in an object constructor. Instead of refilling the characters each time you make a new generator, initialize that array once.

A horrible C-ism is that you make your characters array fixed size, in the hope that it will be large enough. But you only fill it with 63 characters! This is the way bugs are born. Java has multiple collections (like LinkedLists or ArrayLists) that resize themselves if necessary. I would do something like this:

private static char[] characters = initCharacters();

private static char[] initCharacters() {
  final int initialCapacity = 63;
  // a vector is a variable-size array
  final List<Character> chars = new Vector<Character>(initialCapacity);

  // add digits 0–9
  for (char c = '0'; c <= '9'; c++) {
    chars.add(c);
  }

  // add uppercase A–Z and '@'
  for (char c = '@'; c <= 'Z'; c++) {
    chars.add(c);
  }

  // add lowercase a–z
  for (char c = 'a'; c <= 'z'; c++) {
    chars.add(c);
  }

  // Copy the chars over to a simple array, now that we know
  // the length. The .toArray method could have been used here,
  // but its usage is a pain.

  final char[] charArray = new char[chars.size()];

  for (int i = 0; i < chars.size(); i++) {
    charArray[i] = chars.get(i).charValue();
  }

  return charArray;
}

private static void showCharacters() {
  System.err.println("The following " + characters.length + " characters are available: " + new String(characters));
}

What was that characters.length? Yes, Java keeps book about the size of your arrays, so you don't have to! Keeping extra variables around for the size is error prone. Also note that I allocate various objects in the initCharacters method – the garbage collector will clean up after me.

Because our character array has no unused fields, we don't need the charactersSize and charactersCount variables any longer.

Something that C doesn't have is generics, i.e. parameterized polymorphism. The arguments in angled brackets are type names, which can only be classes. Java has primitive types like char and int. These have corresponding object types like Character and Integer.

Your get method is mostly allright. I'd call it nextPassword, and would rename rnd to random. The algorithm is fine.

One thing I'd do differently would be not using much object orientation for this problem. Creating a new PasswordGenerator for different sizes is silly. I'd rather use that functionality like

System.out.println(PasswordGenerator.nextPassword(passwordSize));

i.e. use the generation as a static method, and pass the requested size as a parameter.


SQL query to order by the difference between FIRST and BEST scores Can anyone help me optimise this query?  I have the following table:

cdu_user_progress:
--------------------------------------------------------------
|id    |uid     |lesson_id    |game_id    |date    |score    |
--------------------------------------------------------------

For each user, I'm trying to obtain the difference between the best and first scores for a particular game_id for a particular lesson_id, and order the results by that difference ('progress' in my query):

SELECT ms.uid AS id, ms.max_score - fs.first_score AS progress
FROM (
    SELECT up.uid, MAX(CASE WHEN game_id = 3 THEN score ELSE NULL END) AS max_score
    FROM cdu_user_progress up
    WHERE  (up.uid IN  ('1671', '1672', '1673', '1674', '1675', '1676', '1679', '1716', '1725',         '1726', '1937', '1964', '1996', '2062', '2065', '2066', '2085', '2086')) AND (up.lesson_id = '65') AND (up.score > '-1')
GROUP BY up.uid
) ms
LEFT JOIN (
    SELECT up.uid, up.score AS first_score
    FROM cdu_user_progress up
    INNER JOIN (
        SELECT up.uid, MIN(CASE WHEN game_id = 3 THEN date ELSE NULL END) AS first_date
        FROM cdu_user_progress up
        WHERE  (up.uid IN  ('1671', '1672', '1673', '1674', '1675', '1676', '1679', '1716', '1725', '1726', '1937', '1964', '1996', '2062', '2065', '2066', '2085', '2086')) AND (up.lesson_id = '65') AND (up.score > '-1')
        GROUP BY up.uid
    ) fd ON fd.uid = up.uid AND fd.first_date = up.date
) fs ON fs.uid = ms.uid
ORDER BY progress DESC

Any help would be most appreciated!

SELECT ms.uid AS id

No effect on performance, but this seems weird.  You have a column in that table named id, but you are actually returning uid as id.  Why not just keep it as uid?

LEFT JOIN (

You know that the user has a score, because you found it in the first query.  Therefore, you don't need a LEFT JOIN.  A regular INNER JOIN is fine and will be considerably faster.

CASE WHEN game_id = 3 THEN date ELSE NULL END

It's not clear to me what this buys you.  Why not just put up.game_id = 3 in the WHERE clause?  This will seriously confuse the optimizer.  Same thing for the max_score query.

Given this query, do you have an index on lesson_id, game_id, uid, score, date?  This query could hit only the index.  It's possible that date should be before score.  I'd try making it both ways and then running EXPLAIN to see which it hits.  Then you can delete the other.  You should put whichever of lesson_id and game_id has more potential values first.  It's possible that uid should go before either, but I'd have to test to see.

You put score and date last in the index because they have inequalities.  This keeps the query from being able to optimize later columns, so they go last.  In regards to the uid IN portion, I believe that it may be able to hold its place on the game_id and lesson_id pair, so I put uid after them.

SELECT ms.uid AS id, ms.max_score - fs.first_score AS progress
FROM (
    SELECT up.uid, MAX(score) AS max_score
    FROM cdu_user_progress up
    WHERE  up.uid IN  ('1671', '1672', '1673', '1674', '1675', '1676', '1679', '1716', '1725',         '1726', '1937', '1964', '1996', '2062', '2065', '2066', '2085', '2086') AND up.lesson_id = '65' AND up.score > '-1' AND up.game_id = 3
    GROUP BY up.uid
) ms
INNER JOIN (
    SELECT up.uid, up.score AS first_score
    FROM cdu_user_progress up
    INNER JOIN (
        SELECT up.uid, MIN(date) AS first_date
        FROM cdu_user_progress up
        WHERE up.uid IN  ('1671', '1672', '1673', '1674', '1675', '1676', '1679', '1716', '1725', '1726', '1937', '1964', '1996', '2062', '2065', '2066', '2085', '2086') AND up.lesson_id = '65' AND up.score > '-1' AND up.game_id = 3
        GROUP BY up.uid
    ) fd ON fd.uid = up.uid AND fd.first_date = up.date
    WHERE up.game_id = 3 AND up.lesson_id = '65'
) fs ON fs.uid = ms.uid
ORDER BY progress DESC

Note that I added WHERE up.game_id = 3 AND up.lesson_id = '65' because I think it will help the optimizer.  You should check the EXPLAIN plan to be sure.


Reading a properties file using design patterns I am trying to read properties file using a singleton class. I would like to know if there are any best practices and design patterns to read the properties file. I found dependency injection is one way to read properties file. But, I did not find any examples on the DI design pattern. Any sample code and links would be helpful.

package com.cisco.propertiesreader;

import java.io.IOException;
import java.io.InputStream;
import java.util.Properties;
import java.util.Set;

import org.apache.log4j.Logger;

public class ReadProperties {

    private static final Logger LOGGER = Logger.getLogger(ReadProperties.class);

    private final Properties props = new Properties();

    private ReadProperties() {
        InputStream in = this.getClass().getClassLoader().getResourceAsStream("config.properties");
        try {
            props.load(in);
        } catch (IOException e) {
            LOGGER.error(e.getMessage());
        }
    }

    private static class PropHolder {
        private static final ReadProperties INSTANCE = new ReadProperties();
    }

    public static ReadProperties getInstance() {
        return PropHolder.INSTANCE;
    }

    public String getProperty(String key) {
        return props.getProperty(key);
    }

    public Set<String> getAllPropertyNames() {
        return props.stringPropertyNames();
    }

    public boolean containsKey(String key) {
        return props.containsKey(key);
    }
}

DI means injecting dependencies in  and not creating them directly in a class. Your properties file contains the String value of the keys. You need not compile your java classes if the string value are changed. Thus your string literals and your classes are de-coupled from each other.

Dependency Injection is a Design principle. (I suggest you to study SOLID design Principles). Along with it also we should pay due respects to principles like KISS and YAGNI. These say we should keep the code simple and avoid over-engineering.

You should have different Properties object for each file and pass the object from which you wish to read in your PropHolder.


Number guessing game in Clojure I have recently started learning Clojure, and decided to write a number-guessing-game as an exercise.

The game works as follows:

User enters minimum and maximum values
Computer generates a number between those two values (inclusive)
User has to guess the number (the computer always tells him, whether his guess is bigger or smaller then the secret number)

(ns numguess.core)

(defn parse-int [v]
   (try
     (Integer/parseInt (re-find #"^-?\d+$" (.toString v)))
     (catch NumberFormatException e nil)
     (catch NullPointerException e nil)))

(defn get-input
  ([] (get-input nil))
  ([default]
     (let [input (clojure.string/trim (read-line))]
       (if (empty? input)
         default
         (clojure.string/lower-case input)))))

(defn get-number-input
  []
  (let [n (parse-int (get-input))]
      (if (nil? n) (recur) n)))

(defn get-input-prompt
  ([prompt default] (println (str prompt " ")) (get-input default))
  ([prompt] (get-input-prompt prompt nil)))

(defn get-number-input-prompt
  [prompt]
  (println (str prompt " ")) (get-number-input))

(defn gameplay
  [my-num]
  (let [n (get-number-input-prompt "What is your guess?")]
    (if (< n my-num)
        (do (println "My Number is bigger")
              (recur my-num))
        (if (> n my-num)
            (do (println "My Number is smaller")
                 (recur my-num))
            (println "You have guessed it: congratulations :)")))))

(defn is-yes
  [prompt]
  (let [answer (get-input-prompt prompt "y")]
       (not= "n" answer)))

(defn generate-my-number
  [min-val max-val]
  (let [my-max (max min-val max-val) my-min (min min-val max-val)]
  (+ (rand-int (- (inc my-max) my-min)) my-min)))

(defn mainloop []
  (let [n (generate-my-number
            (get-number-input-prompt "Please enter minimal value")
            (get-number-input-prompt "Please enter maximal value"))]
    (gameplay n)
    (if (is-yes "Would you like to play again?") (recur))))

(mainloop)

I am interested in any kind of improvement suggestion, no matter whether it is something as important as a key concept, or just indentation.

Styling

All forms should be separated by a single newline.
All let bindings should be on their own line. So

(let [my-max (max min-val max-val) my-min (min min-val max-val)] ... )

Should be:

(let [my-max (max min-val max-val)
      my-min (min min-val max-val)]

Instead of fully qualifying other namespaces, you should require them in your ns delclaration:

(ns numguess.core
  (:require [clojure.string :as string]))

(string/trim ...)
(string/lower-case ...)

Structure

Before I even read the requirements, I could tell this was some sort of number game. So that's good.

However, I found it very difficult to figure out how the user input was being read in. get-input is buried
three layers deep for every codepath it's in. Input is parsed ad-hoc in various places, and occasionally the input is altered by default.
I would suggest trying to clearly separate your concerns here. Your objectives are:

Prompt the user for input
Parse that input into usable data (this would include parsing "" to a default value)

If you can separate those things out, you can easily expand Number 2 for any data type you need to parse (e.g. boolean for the "y/n" prompt).

Also, gameplay can be simplified by using a cond rather than nested ifs.

All of that being said, the structure of mainloop gives a pretty clear overview of the entire program. So bravo there.

General Notes

I've said this in another Code Review answer, but I think it applies here too.

When I moved from Java to Clojure, I tended to like very small functions (e.g. is-yes, get-input-prompt, and get-number-input).
Over time, I've found it's a) more idiomatic and b) easier to understand if you start with a rather large function and break it up as needed to
increase understanding. My reasoning for this preference is that Clojure already gives you really good, simple building blocks.
This allows you to define the simple functions that directly enrich your domain without worrying as much about length.
If I want added documentation, I will let a variable with a good name.

In the solution I wrote while preparing this code review, I didn't even include a gameplay or a generate-my-number function. Those pieces
ended up being small enough to fit in mainloop without making it too complex. And, as I said, if you can do that, it actually ends up being
easier to understand because you're not creating artificial abstractions just to hide complexity.

Update in response to OP's comment

Just to clarify my statement about function size, the goal isn't to have small functions or large functions. The goal is to have functions that do one thing, and do it in a clear, concise way.

That goal is really what programming is all about, no matter the language.

The only difference I've found with Clojure is it's easier to express higher concepts clearly. So you're not looking for arbitrary seams just to make your function smaller. You look for concepts that you can cleanly abstract out.

All that to say, if you've found the right places to break up your program, testing should be easier, not harder. Because you're working with a particular domain concept instead of an arbitrary seam.

Additionally, I find testing less necessary overall. I spend more of my time trying to find those simple concepts and less of it building guard rails for myself. Take, for example target-num in my solution below. I could abstract that out (it is a single concept, so it wouldn't be a bad idea) and write a test for it, but a) it's random, so bleh, and b) I can see everything it's doing right there. So there is no real benefit to writing a test around it.

YMMV on the testing thing. Many people find it valuable as a feedback mechanism rather than a guardrail. Just my personal take. :)

Here's my solution:

(defn prompt [parser prompt-text]
  (println prompt-text)
  (if-some [value (try
                    (parser
                      (string/trim
                        (read-line)))
                    (catch Throwable _))]
    value
    (do
      (println "I'm sorry. I didn't understand you.")
      (recur parser prompt-text))))

(defn parse-int [s]
  (Integer/parseInt s))

(defn parse-bool [s]
  ({"y" true
    "yes" true
    "true" true
    "n" false
    "no" false
    "false" false} (string/lower-case s)))

(def prompt-int (partial prompt parse-int))
(def prompt-bool (partial prompt parse-bool))

(defn my-mainloop []
  (let [min-val (prompt-int "Please enter minimal value")
        max-val (prompt-int "Please enter maximal value")
        target-num (+ (rand-int
                        (- (inc max-val) min-val))
                      min-val)]
    (loop []
      (let [guess (prompt-int "What is your guess?")]
        (cond
          (< guess target-num) (do
                                 (println "My Number is bigger")
                                 (recur))
          (> guess target-num) (do
                                 (println "My Number is smaller")
                                 (recur))
          :else
          (println "You have guessed it: congratulations :)"))))
    (when (prompt-bool "Would you like to play again?")
      (recur))))


method isInteger(BigDecimal number) isn't compliant with sonar rules public boolean isInteger(BigDecimal number) {
    boolean ret;
    try {
        number.toBigIntegerExact();
        ret = true;
    } catch (ArithmeticException ex) {
        ret = false;
    }
    return ret;
}

This methods is correctly unit tested, but my sonar tells me "Either log or rethrow this exception.". My first reaction is to ignore the sonar rule (they're not mandatory), but I'm not sure if there's a better way to do it without catching an arithmetic exception.

You should ignore the warning. However this warning is not unfounded as exceptions are costly (stack unwinding) and in this case the exception might even happen in maybe 90% of the cases.

So in this case preferably an alternative should be sought:

public boolean isInteger(BigDecimal number) {
    return number.stripTrailingZeros().scale() <= 0;
}

Here stringTrailingZeros would turn 600.00 into 6E2, hence with scale -2, and 0.0600 would become 6E-2 with scale 2.

Less code, hence even Wally points on the Dilbert scale.

P.S. there exist SuppressWarning annotations for SonarLint, though neat programmer that I am, I do not know the warning key.


Generate every unique combination using all elements from array I am trying to make a script that generates every possible combination from an array of elements. I have found a script that does it, but it is too intensive and takes extremely long to generate anything that has more than 7 elements inside the array.

I am trying to achieve, where from an array of:

array("Quick", "Brown", "Fox");

I would get a result of:

Quick Brown Fox
Quick Fox Brown
Brown Quick Fox
Brown Fox Quick
Fox Quick Brown
Fox Brown Quick

As you can see, that a single word never repeats itself, and there are only unique generations, as well as it always uses all elements of the array, so it never returns less elements than the length of array.

Here is the code that I am currently using:

function f_($n) {
    if($n<2) { return 1; }
    for($x = 2;$n-1>1;$x*=$n--);
    return $x;
}

function array_restore($r) {
    $clean = array();
    if(is_array($r)) {
        foreach($r as $k) {
            $clean[] = $k;
        }
    }

    return $clean;
}

function connect($arr){
    $back = "";
    foreach($arr as $a){
        if($back != ""){
            $back .= " ";
        }

        $back .= $a;
    }
    return $back;
}

function cmb($v) {
    $str = count($v);
    $tot = f_($str);

    $combo = array();
    for($i=0;$i<$tot*8;$i++) {
        shuffle($v);
        $sf = connect($v);
        if(!in_array($sf, $combo)){
            $combo[] = $sf;
        }
    }

    $x = array_unique($combo);
    return array_restore($x);
}

var_dump(cmb(array("Quick", "Brown", "Fox")));

EDIT: I found a bit better algorithm, seems like its impossible to make anything even quicker, so I will have to find a different way to approach my problem.

function pc_permute($items, $perms = array( )) {
    $back = array();
    if (empty($items)) {
        $back[] = join(' ', $perms);
    } else {
        for ($i = count($items) - 1; $i >= 0; --$i) {
             $newitems = $items;
             $newperms = $perms;
             list($foo) = array_splice($newitems, $i, 1);
             array_unshift($newperms, $foo);
             $back = array_merge($back, pc_permute($newitems, $newperms));
         }
    }
    return $back;
}

Your method chooses a word combination at random, then checks if that combination has been discovered previously. The first few combinations are discovered quickly, but last few take much longer. Once you have found 99.9% of the unique rows, it will take on average 1000 tries to find the next unique row. You also give up too soon, leaving some combinations undiscovered.

For n different words, there are n! (n factorial) unique combinations. So for your 8-word example:

    array("Quick", "Brown", "Fox", "Jumped", "Over", "The", "Lazy", "Dog")

there are 8! or exactly 40,320 combinations. When I ran your code, I got only 40,305 rows, in random order, with 15 combinations missing. (and it took 4 minutes!) My version (below) runs in 400ms and produces the rows in the order you desire, i.e. all the "Quick" rows first etc.

My algorithm doesn't make use of random guesses; it uses some loops and recursion to generate all the combinations deterministically.

    // For an array of n words, return an array of n! strings, each containing the words in a different order.
    function wordcombos ($words) {
        if ( count($words) <= 1 ) {
            $result = $words;
        } else {
            $result = array();
            for ( $i = 0; $i < count($words); ++$i ) {
                $firstword = $words[$i];
                $remainingwords = array();
                for ( $j = 0; $j < count($words); ++$j ) {
                    if ( $i <> $j ) $remainingwords[] = $words[$j];
                }
                $combos = wordcombos($remainingwords);
                for ( $j = 0; $j < count($combos); ++$j ) {
                    $result[] = $firstword . ' ' . $combos[$j];
                }
            }
        }
        return $result;
    }


Eloquent Javascript exercise: find average age from ancestors array per century I solved an exercise from the book "Eloquent Javascript". I want to share with you guys the solution. The exercise is from chapter 5. Summing up, I have to find the average age from the ancestors array per century. Here is my solution:

var ancestry = JSON.parse(require('../resources/ancestry'))

//Top-down approach, functions are defined after their use
//Necessary side-effect in function to print the output
function printAverageAgePerCentury(ancestry){
    var averageAgePerCentury = getAverageAgePerCentury(ancestry);
    Object.keys(averageAgePerCentury).forEach(function(century){
        console.log(century.concat(": ").concat(averageAgePerCentury[century]))
    })
}

function getAverageAgePerCentury(ancestry){
    var agesPerCentury = getAgesPerCentury(ancestry);
    return agesPerCentury.reduce(function(averageAgePerCentury, agePerCentury){
        if(averageAgePerCentury[agePerCentury.century]){
            averageAgePerCentury[agePerCentury.century] = [average(averageAgePerCentury[agePerCentury.century].concat(agePerCentury.age))];
            return averageAgePerCentury;
        } else {
            averageAgePerCentury[agePerCentury.century] = [agePerCentury.age];
            return averageAgePerCentury;
        }
    }, {})
}

function getAgesPerCentury(ancestry){
    return ancestry.map(function(person){
        return {
            century : whichCentury(person),
            age : person.died - person.born
        }
    })
}

function average(array){
    function plus(a,b){ return a + b}
    return array.reduce(plus) / array.length
}

function whichCentury(person){
    return Math.ceil(person.died / 100)
}

//--------------------------------//

printAverageAgePerCentury(ancestry);

What do you think about the top-down approach? Do you prefer the opposite? Where functions are defined before their use.
Is possible to make this solution more readable, more elegant?
What could you do to make it better?

Incorrect calculation of average

average([1, 2, 3, 4]) is \$\dfrac{1+2+3+4}{4}=2.5\$.

However, due to the way you call reduce() in getAverageAgePerCentury():

averageAgePerCentury[agePerCentury.century] = [average(averageAgePerCentury[agePerCentury.century].concat(agePerCentury.age))];

… you would actually calculate average([average([average([average([1]), 2]), 3]), 4]), which would produce a result of 3.125 — weighted in favour of the later entries.

Doing too much

A root cause of the bug, I think, is that getAverageAgePerCentury() is doing a lot of work.  It creates a map of centuries to the age of people who died in that century, and calculates the average of those values.

I would define two utility functions, inspired by Lodash's _.mapValues() and _.groupBy().

/**
 * Creates a new object in which all of the original obj's values are
 * transformed by callback(value).
 */
function mapValues(obj, callback) {
    return Object.keys(obj).reduce(function(result, key) {
        result[key] = callback(obj[key]);
        return result;
    }, {});
}

/**
 * Given an array of objects, creates a new object in which all objects where
 * attrExtractor(obj) having the same value are grouped together in an array.
 */
function groupBy(objs, attrExtractor) {
    return objs.reduce(function(result, obj) {
        var attr = attrExtractor(obj);
        if (attr in result) {
            result[attr].push(obj);
        } else {
            result[attr] = [obj];
        }
        return result;
    }, {});
}

Then, the rest of the solution looks simpler.

function average(array) {
    return array.reduce(function(a, b) { return a + b }) / array.length;
}

function averageAgePerCentury(ancestry) {
    function deathCentury(person) { return Math.ceil(person.died / 100) }
    function deathAge(person) { return person.died - person.born }

    return  mapValues(
                mapValues(
                    groupBy(ancestry, deathCentury),
                    function(persons) { return persons.map(deathAge) }
                ),
                average
            );
}

Note that I've renamed getAverageAgePerCentury().  A "getter" function should retrieve something that already exists.  Here, you're doing a calculation.  You call Math.sin(x), not Math.getSin(x), so the same naming principle applies here.


Find easter on any given year To find the days off for our custom accounting software there is this method used to find when is Easter in any given year.

private static DateTime Easter(int year) {
    int a = year%19;
    int b = year/100;
    int c = (b - (b/4) - ((8*b + 13)/25) + (19*a) + 15)%30;
    int d = c - (c/28)*(1 - (c/28)*(29/(c + 1))*((21 - a)/11));
    int e = d - ((year + (year/4) + d + 2 - b + (b/4))%7);
    int month = 3 + ((e + 40)/44);
    int day = e + 28 - (31*(month/4));
    return new DateTime(year, month , day);
}

The usual advice is "make meaningful variables". However, we know this algorithm (essentially a modification of that published by a young Gauß) and those variables don't have a simple meaning. (See Decoding Gauss Easter algorithm). We know b represents centuries, but even if you did find a word to describe what the variables represent, would using epact or even ageOfMoonOnJanFirst help to understand the algorithm? The variable represent stages of the algorithm. As Gauss wrote "It is not possible to present the full analysis that led to the algorithm".

So the usual cure of incomprehensible code - clear variables names - won't help much here.  What can we do for the poor person who has to maintain this code 6 months, or 6 years, later?  We can let them know where this algorithm comes from. This is easy enough. A comment referencing the source would allow someone seeing this to check that it is still doing what it is supposed to do.

Beyond adding this comment, the algorithm for calculating Easter is unlikely to be a bottleneck in any piece of software, so assuming that this algorithm is correct (proven and tested) I would leave it as is.


Check if variable is sent I have this piece of code in my Node.js API and I feel like it could (should) be optimized.

var apiRouter = express.Router();

apiRouter.route('/user/:username')
.put(function(req, res) {
    User.findOne({
        username: req.params.username
    }).exec(function(err, user) {
    if (err) {
        res.send(err);
    }
    if (!user) {
        res.json({
            success: false,
            message: 'User not found'
        });
    } else {
        // This is the part to optimize
        if (req.body.firstname) {
            user.firstname = req.body.firstname;
        }
        if (req.body.lastname) {
            user.lastname = req.body.lastname;
        }
        if (req.body.email) {
            user.email = req.body.email;
        }
        if (req.body.username) {
            user.username = req.body.username;
        }
        if (req.body.password) {
            // no worries about this, I hash the password before it is saved
            user.password = req.body.password;
        }

        user.save(function(err) {
            if (err) {
                res.send(err);
            }
            res.json({
                success: true,
                message: 'User updated'
            });
        });
    }
});

I use this to update my user. In the future I could (will) have way more parameters than these.

Is there a scalable solution?

I'm thinking about something like this:

var form = req.body;
user.firstname = form.firstname ? form.firstname : user.firstname;
user.lastname = form.lastname ? form.lastname : user.lastname;
// ...

Problem is that I'm not sure it is more scalable. I feel like it is just another way to write if statements.

I do not believe the current answer solves your optimization need, I believe the correct answer is to usefindOneAndUpdate.  This will cut down on the steps need to achieve this same goal.  The update function will handle all of the if checks, it will only update fields supplied in the req.body.

User.findOneAndUpdate({ username: req.params.username }, req.body, function (error, User) {
      if (User) {
        res.json({
            success: true,
            message: 'User updated'
        });
      } else {
        // ALWAYS RETURN res.send please
        return res.send(err);
      }
    });

http://mongoosejs.com/docs/api.html#query_Query-findOneAndUpdate for additional docs.

WARNING If res.body has properties that are set as undefined, it will overwrite existing values.  You should validate inputs coming in to ensure this is not the case.


Filter an array of objects, checking only specified properties and child objects The function filterList recieves an array of objects and filters the list, checking only the properties of the object specified in a 'filterFields' array.
It compares the values of those specified properties to a string 'filterTerm' using a comparison function.
If the property specified by 'filterFields' is an array or object, it also checks through those and returns the entire top-level object if the specified fields of any child objects are matched with the comparison function.
I would like to check to see if this is normal/sensible javascript and whether there is a better way of writing this functionality, performance-wise.
    //if filterTerm or filterFields are empty, return original list
    function filterList(list, filterTerm, filterFields) {
        if(filterTerm && filterFields.length > 0) {
            return list.filter(filterCallback(filterFields, filterTerm));
        }else{
            return list;
        }
    }
    //use closure to pass filterFields and filterTerm to callback
    function filterCallback(filterFields, filterTerm) {
        return function(element) {
            return filterFields.some((filterField)=>{
                if(Array.isArray(element[filterField])){
                    return element[filterField].some(function(childElement){
                        return filterCallback(childElement);
                    })
                }else if(typeof element[filterField] === 'object'){
                    return filterCallback(element[filterField]);
                }
                return comparisonFunction(element[filterField], filterTerm);
            })
        }
    }
    //comparisonFunction - field value is populated, and either filterterm contains the fieldValue or visa-versa
    function comparisonFunction(fieldValue, filterTerm) {
        return fieldValue &&
        (String(fieldValue).toLowerCase().indexOf(filterTerm.toLowerCase()) >= 0
        || String(filterTerm).toLowerCase().indexOf(fieldValue.toLowerCase()) >= 0);
    }

//EXAMPLE USE:
    arr = [{name: "a", job: "b"}, {name:"c", job:"d"}];
    filterList(arr, "a", ["name"]); // => [{name: "a", job:"d"}]
    arr2 = [{name: "a", job: "b", employees: [{name: "d", job:"e" }]}, {name: "f", job: "g"}];
    filterList(arr2, "d", ["name", "employees"]); //=> [{name: "a", job:"b", employees: [{name: "d", job: "e"}]}]

```

DRY your code You reference element[filterField] quite a few times in filterCallback. It would be nicer to save the value in a standalone variable first - then you can reference that variable instead of doing object property lookup each time.
Nested object bug filterCallback returns a function, but here, if an array or object is found, you're returning a call of filterCallback, which will always be truthy, so the .some will pass, even when it shouldn't:
return filterFields.some((filterField) => {
  if (Array.isArray(element[filterField])) {
    return element[filterField].some(function(childElement) {
      return filterCallback(childElement); // <--- always truthy
    })
  } else if (typeof element[filterField] === 'object') {
    return filterCallback(element[filterField]); // <--- always truthy
  }
  return comparisonFunction(element[filterField], filterTerm); // <--- OK
})

// No items should pass the test, but they do:

//if filterTerm or filterFields are empty, return original list
function filterList(list, filterTerm, filterFields) {
  if (filterTerm && filterFields.length > 0) {
    return list.filter(filterCallback(filterFields, filterTerm));
  } else {
    return list;
  }
}

//use closure to pass filterFields and filterTerm to callback
function filterCallback(filterFields, filterTerm) {
  return function(element) {
    return filterFields.some((filterField) => {
      if (Array.isArray(element[filterField])) {
        return element[filterField].some(function(childElement) {
          return filterCallback(childElement);
        })
      } else if (typeof element[filterField] === 'object') {
        return filterCallback(element[filterField]);
      }
      return comparisonFunction(element[filterField], filterTerm);
    })
  }
}
//comparisonFunction - field value is populated, and either filterterm contains the fieldValue or visa-versa
function comparisonFunction(fieldValue, filterTerm) {
  return fieldValue &&
    (String(fieldValue).toLowerCase().indexOf(filterTerm.toLowerCase()) >= 0 ||
      String(filterTerm).toLowerCase().indexOf(fieldValue.toLowerCase()) >= 0);
}

arr2 = [{
  name: "a",
  job: "b",
  employees: [{
    name: "z",
    job: "e"
  }]
}, {
  name: "f",
  job: "g"
}];
console.log(filterList(arr2, "d", ["name", "employees"])); //=> [{name: "a", job:"b", employees: [{name: "d", job: "e"}]}]

You need to call filterCallback(filterFields, filterTerm) first to get the filter function, then call the filter function with the value to check, eg:
return filterFields.some((filterField) => {
    const value = element[filterField];
    if (Array.isArray(value)) {
        return value.some(function (childElement) {
            return filterCallback(filterFields, filterTerm)(childElement);
        });
    } else if (typeof value === 'object') {
        return filterCallback(filterFields, filterTerm)(value);
    }
    return comparisonFunction(value, filterTerm); // <--- OK
});

//if filterTerm or filterFields are empty, return original list
function filterList(list, filterTerm, filterFields) {
  if (filterTerm && filterFields.length > 0) {
    return list.filter(filterCallback(filterFields, filterTerm));
  } else {
    return list;
  }
}

//use closure to pass filterFields and filterTerm to callback
function filterCallback(filterFields, filterTerm) {
  return function(element) {
    return filterFields.some((filterField) => {
        const value = element[filterField];
        if (Array.isArray(value)) {
            return value.some(function (childElement) {
                return filterCallback(filterFields, filterTerm)(childElement);
            });
        } else if (typeof value === 'object') {
            return filterCallback(filterFields, filterTerm)(value);
        }
        return comparisonFunction(value, filterTerm); // <--- OK
    });
  }
}
//comparisonFunction - field value is populated, and either filterterm contains the fieldValue or visa-versa
function comparisonFunction(fieldValue, filterTerm) {
  return fieldValue &&
    (String(fieldValue).toLowerCase().indexOf(filterTerm.toLowerCase()) >= 0 ||
      String(filterTerm).toLowerCase().indexOf(fieldValue.toLowerCase()) >= 0);
}

//EXAMPLE USE:

arr = [{
  name: "a",
  job: "b"
}, {
  name: "c",
  job: "d"
}];
console.log(filterList(arr, "a", ["name"])); // => [{name: "a", job:"d"}]

arr2 = [{
  name: "a",
  job: "b",
  employees: [{
    name: "z",
    job: "e"
  }]
}, {
  name: "f",
  job: "g"
}];
console.log(filterList(arr2, "d", ["name", "employees"])); //=> [{name: "a", job:"b", employees: [{name: "d", job: "e"}]}]

Precise variable names To reduce bugs, best to name variables as precisely as possible - filterCallback isn't actually the callback used in filter or some, but it's a function which creates such a callback. Maybe call it createFilterCallback instead - that probably would've made the problem clearer at a glance.
Implicit return Since you're using ES6, you could consider using arrow functions too - their implicit return can make code much more concise. Less code means less surface area for bugs, and can sometimes make it easier to read.
comparisonFunction If you want to check whether a string includes another, in ES6, better to use .includes(...) than .indexOf(...) >= 0. You also have a potential bug - you do String(fieldValue).toLowerCase() and String(filterTerm).toLowerCase(), implying that you expect that the value or term may not be a string - but then you do .indexOf(filterTerm.toLowerCase()) and .indexOf(fieldValue.toLowerCase()), which will throw if they aren't strings. Make sure to convert them to strings first:
const comparisonFunction = (fieldValue, filterTerm) => {
    const value = String(fieldValue).toLowerCase();
    const term = String(filterTerm).toLowerCase();
    return value && value.includes(term) || term.includes(value);
};

null is an object too, unfortunately, so:
}else if(typeof element[filterField] === 'object'){
    return filterCallback(element[filterField]);
}

will result in a recursive call, even when such a call won't make sense. The code works, but it's confusing flow. Better to only recurse if the value is an object and is not null.
filterCallback The whole filterCallback looks a bit ugly IMO. I'd simplify it a bit by not calling the whole function again recursively, but by declaring the function to filter elements by inside the function before returning it. Then, instead of recursing on filterCallback, recurse on that declared function:
const filterCallback = (filterFields, filterTerm) => {
  const testElement = element => filterFields.some((filterField) => {
    const value = element[filterField];
    if (Array.isArray(value)) {
      return value.some(testElement);
    } else if (value && typeof value === 'object') {
      return testElement(value);
    } else {
      return value && comparisonFunction(value, filterTerm);
    }
  });
  return testElement;
}

In all:

//if filterTerm or filterFields are empty, return original list
function filterList(list, filterTerm, filterFields) {
  if (filterTerm && filterFields.length > 0) {
    return list.filter(filterCallback(filterFields, filterTerm));
  } else {
    return list;
  }
}
//use closure to pass filterFields and filterTerm to callback
const filterCallback = (filterFields, filterTerm) => {
  const testElement = element => filterFields.some((filterField) => {
    const value = element[filterField];
    if (Array.isArray(value)) {
      return value.some(testElement);
    } else if (value && typeof value === 'object') {
      return testElement(value);
    } else {
      return value && comparisonFunction(value, filterTerm);
    }
  });
  return testElement;
}
//comparisonFunction - field value is populated, and either filterterm contains the fieldValue or visa-versa
const comparisonFunction = (fieldValue, filterTerm) => {
  const value = String(fieldValue).toLowerCase();
  const term = String(filterTerm).toLowerCase();
  return value.includes(term) || term.includes(value);
};

//EXAMPLE USE:
arr = [{name: "a", job: "b"}, {name:"c", job:"d"}];
console.log(filterList(arr, "a", ["name"])); // => [{name: "a", job:"d"}]
arr2 = [{name: "a", job: "b", employees: [{name: "d", job:"e" }]}, {name: "f", job: "g"}];
console.log(filterList(arr2, "d", ["name", "employees"])); //=> [{name: "a", job:"b", employees: [{name: "d", job: "e"}]}]


Is there a better way to translate a string into methods? So I create 3D tree like structures using L-systems. Basically this means I generate a string like this:

 "FFFFFFFF-[[FFFF-[[FF-[[F-[[X]+X]+F[+FX]-X]+F]]]]]"

This string is then interpreted by a turtle performing certain actions for each character in the string. The problem is how to map characters to turtle actions in an elegant way in C++.

Right now I use a enum to specify the actions and performAction calls methods based on the action. Is there a better way of doing it?

The turtle code:

enum TurtleAction {
    MoveForward,
    TurnLeft,
    TurnRight,
    PitchDown,
    PitchUp,
    RollLeft,
    RollRight,
    TurnAround,
    PushState,
    PopState
};

struct TurtleState {
    Vec3f pos;

    Vec3f H;
    Vec3f L;
    Vec3f U;

    double width;
};

class Turtle {
    TurtleState m_currentState;
    stack<TurtleState> m_states;

    void(*m_drawFunc)(Vec3f,double); ///< Drawing callback

public:
    double alpha; ///< Turning angle

    Turtle(TurtleState startState,
           void(*drawFunc)(Vec3f,double));
    void performAction(TurtleAction action);

private:
    void move(double dist);
    void turn(double deg);
    void pitch(double deg);
    void roll(double deg);

    void push();
    void pop();

    void rotate(const Mat44f & R);
};

void
Turtle::performAction(TurtleAction action){
    switch (action) {
        case MoveForward:
            move(1.0f);
            break;

        case TurnLeft:
            turn(-alpha);

        case TurnRight:
            turn(+alpha);

        case PitchDown:
            pitch(-alpha);

        case PitchUp:
            pitch(+alpha);

        case RollLeft:
            roll(-alpha);

        case RollRight:
            roll(+alpha);

        case TurnAround:
            turn(180.0f);

        case PushState:
            push();

        case PopState:
            pop();

        default:
            break;
    }
}

The string parsing:

class LSystem {

    map<char,TurtleAction> m_actionRules;
    TurtleState m_start;

public:

    void draw(string & str,void(*drawFunc)(Vec3f,double));
    void computeString(string & axiom, int generations);

    LSystem();
    ~LSystem();

};

void
LSystem::draw(string &str, void (*drawFunc)(Vec3f, double))
{
    Turtle turtle(m_start,drawFunc);
    turtle.alpha = 25.0f;

    for (int s = 0; s < str.size(); s++) {
        char c = str[s];
        map<char,TurtleAction>::iterator it = m_actionRules.find(c);
        if (it != m_actionRules.end()) {
            turtle.performAction(it->second);
        }
    }
}

The design flaw would have been the use of this switch case with 11 entries. That's already a lot. By the way I warn you, you have forgotten 9 break out of 11 cases.

However, in your case, each case relates to a single action. I mean, there is not a lot new to understand while reading: for each case you define a single behaviour action, inside a single word function call. (You don't put multi line, multi code, with multi conditions behaviour that is complex inside your switch case... : this is the thing to avoid and you have avoided it)

So honestly speaking it's totally acceptable.

Only for your culture I am mentionning some ways to get rid of switch case. I repeat myself, you shouldn't apply them in your case right now.

However, the first thing you may notice in such design, maybe there are common behaviours that are dupplicated between all your function call inside the switch case.

Imagine, if in roll() and turn() you would first have to

Turn head to the right to see if nothing tries to cross my path
Turn head to the left to see if nothing is moving straight right on me

and then only to roll() or to turn()

Then you could start thinking about using the Strategy pattern instead of simple function calls. This way, the strategy object which contain the action to do, can share common behaviour in a mother class.

If you started to have really many Strategies, that should be tune and tweaked precisely, then you would think about implementing the Interpreter Pattern so as to map between the precise language in your entry and the Strategies to apply.

And if you start to have a big Interpreter object and you think you need a grammar to express all the possibilities, then no doubt you should think about a Domain Specific Language. ( with you how parser etc... ).

In your case the switch case is entirely sufficient ;-)


Coloring a string in a terminal I have this code which makes it easy to color a string in terminal but it is looking really repetitive. Is there a more idiomatic way of writing this? Also is there a functional way of doing this more effectively?

It seems really trivial, something like strings blue,red etc should point to a generic function, and when you invoke it, you should get the output depending on the caller's name!

But apparently such a thing doesn't exist unless I call it as arguments. But then I lose the flexibility of writing blue(bold(italics("Foo"))) which is really the best way to do this from an end user perspective.

Can I have a single function object in the memory and do this more effectively?

bcolors = {"BLUE": '\033[94m',
           "HIGH": '\033[93m',
           "OKAY": '\033[92m',
           "FAIL": '\033[91m',
           "BOLD": '\033[1m',
           "LINE": '\033[4m',
           "ENDC": '\033[0m'
           }

def blue(string):
    return bcolors["BLUE"] + string + bcolors["ENDC"]

def yellow(string):
    return bcolors["HIGH"] + string + bcolors["ENDC"]

def green(string):
    return bcolors["OKAY"] + string + bcolors["ENDC"]

def red(string):
    return bcolors["FAIL"] + string + bcolors["ENDC"]

def bold(string):
    return bcolors["BOLD"] + string + bcolors["ENDC"]

def line(string):
    return bcolors["LINE"] + string + bcolors["ENDC"]

I came up with this:

for key in bcolors:
    locals().update({key: lambda string: bcolors[key] + string + bcolors["ENDC"]})

which is almost equivalent to your code (except the function names are uppercase). There is still a function being created for every colour but it's more concise in writing.

It works for me but apparently changing locals() is a bad idea:

  Others have suggested assigning to locals(). This won't work inside a function,  where locals are accessed using the LOAD_FAST opcode, unless you have an exec statement somewhere in the function.

https://stackoverflow.com/questions/8028708/dynamically-set-local-variable-in-python

  Note: The contents of this dictionary should not be modified; changes may not affect the values of local and free variables used by the interpreter.

https://docs.python.org/2/library/functions.html#locals

Another way I see is to write a function like so:

def _(color):
    return lambda string: bcolors[color] + string + bcolors[color]

but you'd have to call it like this:

_("BLUE")("hello")

That's the closest I can come up with.


Open/Closed Principle with calculating the total cost of items in a shopping cart I came across the below example in a tutorial for the open-closed OOP principle. This example demonstrates function of calculating the total cost of items in a shopping cart. I have provided code for both before and after refactoring to comply with the open-closed principle. Both versions are from the tutorial I followed.

The drawback with the old code is that we need to add additional if/else to support new SKUs thus violating "closed for modifications" part in the open-closed principle. But even in the new design, we need to add additional IPriceRule to support new SKUs. So in what way is the new design better than old design? Is this because with the new design it is little more flexible to add complex calculation algorithm?

I have left comments in relevant sections of the code where it violates the principle.

Before:

class OrderItem {
    string sku;
    int quantity;
}

class Cart {
    List<OrderItem> items;

    public Cart() {
        items = new ArrayList<>();
    }

    public void addItem(OrderItem item) {
        items.add(item);
    }

    int totalPrice() {
        int price = 0;

        for(OrderItem item : items) {
            if( item.sku == "foo") {
                price += item.quantity * 10;
            }
            else if (item.sku == "bar") {
                price += item.quantity * 5;
            }
            // This is the problematic part. We need to add more if/else
            // conditions to support different SKUs. Hence this violates
            // closed for modifications part of open-closed.
        }

        return price;
    }
}

After refactoring to comply with the Open-Closed principle:

interface IPriceRule {
    boolean isMatch(OrderItem item);
    int calculatePrice(OrderItem item);
}

class FooPriceRule implements IPriceRule {
    @Override
    boolean isMatch(OrderItem item) {
        return item.sku.equals("foo");
    }

    @Override
    int calculatePrice(OrderItem item) {
        return item.quantity * 10;
    }
}

class BarPriceRule implements IPriceRule {
    @Override
    boolean isMatch(OrderItem item) {
        return item.sku.equals("bar");
    }

    @Override
    int calculatePrice(OrderItem item) {
        return item.quantity * 5;
    }
}

interface IPaymentCalculator {
    int calculatePrice(OrderItem item);
}

class PaymentCalculator implements IPaymentCalculator {
    List<IPriceRule> priceRules;

    public PaymentCalculator() {
        priceRules = new ArrayList<>();
        priceRules.add(new FooPriceRule());
        priceRules.add(new BarPriceRule());
        // Add new rules here to support new SKUs. But why is this better
        // than adding if/else in the old design? Because even this class
        // changes as we add more requirements right?
    }

    @Override
    int calculatePrice(OrderItem item) {
        for(IPriceRule priceRule : priceRules ) {
            if (priceRule.isMatch(item)) {
                return priceRule.calculatePrice(item);
            }
        }

        return 0;
    }
}

class Cart {
    List<OrderItem> items;
    IPaymentCalculator paymentCalculator;

    public Cart() {
        items = new ArrayList<>();
        paymentCalculator = new PaymentCalculator();
    }

    public void addItem(OrderItem item) {
        items.add(item);
    }

    int totalPrice() {
        int price = 0;

        for(OrderItem item : items) {
            price += paymentCalculator.calculatePrice(item);
        }

        return price;
    }
}

Basics

String comparison shouldn't be done with ==, unless you can carefully ensure you're using String.intern() everywhere so that the reference comparison will always match appropriately (not to forget the documentation too).
Keep your fields private!
Should PaymentCalculator.calculatePrice(OrderItem) really return 0 if there are no matching rules? That's like saying I can get oranges for free for a payment calculator that only (accidentally?) handle apples...

Rule-based vs if statements

// Add new rules here to support new SKUs. But why is this better
// than adding if/else in the old design? Because even this class
// changes as we add more requirements right?

You can think of it that the calculation logic is not baked into the payment calculator, and that the logic can be maintained separately. For example, a more complex IPriceRule implementation might be retrieving prices from a database, and the calculator does not need to deal with any database connections know-how.


Stack implementation using linked list in C++ I'd like some feedback for the code I wrote. I haven't programmed in C++ for two years.

#include <string>

#pragma once
template <class T>
class StackLinkedList
{
public:
    StackLinkedList() : m_List(nullptr), m_Size(0) {}
    ~StackLinkedList();
    void push(const T& val);
    void pop();
    T& top();
    const T& top() const;
    size_t size() const;
    bool empty() const;
private:
    struct Node;
    Node *m_List;
    size_t m_Size;
};

template <class T>
struct StackLinkedList<T>::Node
{
    T m_Val;
    Node* m_Next;
    Node(const T& val, Node* nextPtr) : m_Val(val), m_Next(nextPtr) {}
};

template<class T>
StackLinkedList<T>::~StackLinkedList()
{
    typename Node* nextNodePtr;
    for (typename Node* curNodePtr = m_List; curNodePtr != nullptr; )
    {
        nextNodePtr = curNodePtr->m_Next;
        delete curNodePtr;
        curNodePtr = nextNodePtr;
    }
}

template<class T>
void StackLinkedList<T>::push(const T& val)
{
    typename Node* newNode = new Node(val, m_List);
    m_List = newNode;
    m_Size++;
}

template<class T>
void StackLinkedList<T>::pop()
{
    if (m_List != nullptr)
    {
        typename Node* curNodePtr = m_List;
        m_List = m_List->m_Next;
        delete curNodePtr;
        m_Size--;
    }
}

template<class T>
T& StackLinkedList<T>::top()
{
    if (empty())
    {
        throw new std::string("Stack is empty");
    }
    return m_List->m_Val;
}

template<class T>
const T& StackLinkedList<T>::top() const
{
    if (empty())
    {
        throw new std::string("Stack is empty")
    }
    return m_List->m_Val;
}

template<class T>
size_t StackLinkedList<T>::size() const
{
    return m_Size;
}

template<class T>
bool StackLinkedList<T>::empty() const
{
    return m_Size == 0;
}

This is quite good, actually. I have one big comment, and a few relatively minor comments.

RULE OF THREE. Anytime you define a destructor, you want to make sure you define your copy constructor and copy assignment operators. The reason for that is the following:

StackLinkedList<T> stack;
stack.push(...);
stack.push(...);
{
    StackLinkedList<T> stack2 = stack;
    stack2.push(...);
    // <== stack2 deleted here
}
// <== uh-oh, stack2 deleted all the nodes, but stack still thinks
// it has valid objects that it's holding onto that it needs to delete too!

Your class is sorely missing:

StackLinkedList(const StackLinkedList& );

friend void swap(StackLinkedList&, StackLinkedList& );
StackLinkedList& operator=(StackLinkedList other) {
    swap(*this, other);
    return *this;
}

For more on why I suggest this way specifically, see what is the copy-and-swap idiom.

You didn't mention if you were using C++11 or not. If you are, you want the Rule of Five: you will also need to implement the move constructor and move assignment operator. For this class in particular, those two should be very short.

Other comments are minor.

Use of typename. There are places where you need to use typename and there are places where you don't. Just don't use it in those latter cases - it's just unnecessary. In your code, literally every usage of typename is redundant and can be omitted.

top(). The typical implementation of containers is to make operations as fast as possible. So for top(), that would mean not checking empty() - that's up to the user. Just simply return m_List->m_Val and note that it's just undefined behavior to call top() on an empty stack. If you disagree and want to throw an exception, that is of course perfectly reasonable as well - but this not the way to do it:

throw new std::string("Stack is empty");

You're throwing a std::string*. Anybody that catches it would have to delete it, otherwise you're leaking memory - and std::string* doesn't really convey information. If you look in <stdexcept>, there's a better choice of exception type: std::out_of_range. Standard practice is to throw by value, catch by reference. So that line would be:

throw std::out_of_range("Stack is empty");

#pragma once should be the first line in your file, you have it after a different #include.


Thread Safe Server Querier I wanted to make a class that connects to my server and returns a value, a URL in this case. As Android 4.0 doesn't let me run network task on the main UI thread (rightly so) I have made this.

Please give me your feedback as to if it is thread safe or not and ways to improve it. I have never used ExecutorService or Future before so I would be grateful to know if I am using it correctly. I would also like to know if this is blocking the thread that called getDownloadURL.

public class DownloadURLServerGetter implements DownloadURLGetter {

    private ServerAdapter serverAdapter;
    private String id;

    @Inject
    public DownloadURLServerGetter(ServerAdapter serverAdapter) {
        this.serverAdapter = serverAdapter;
    }

    @Override
    public synchronized String getDownloadURL(String id) throws IOException {
        this.id = id;
        final ExecutorService service;
        final Future<String> task;

        service = Executors.newFixedThreadPool(1);
        task = service.submit(new MyCallable());
        try {
            return task.get();
        } catch (InterruptedException e) {
            throw new IOException(e);
        } catch (ExecutionException e) {
            throw new IOException(e);
        }
    }

    public class MyCallable implements Callable<String> {

        @Override
        public String call() throws Exception {

            HttpResponse resp = serverAdapter.get("blobs/" + id);
            String url = EntityUtils.toString(resp.getEntity());
            return Parser.parseURL(url);
        }
    }

}

Your method seems thread safe, but you are blocking getDownloadURL() because task.get() will only return once the underlying job finishes. Try reading up on Java's  future and see how you can use them more efficient and correct.

Your executor service should most likely only be created once for your application.

By the way: if you pass ServerAdapter and id to MyCallable (as constructor arguments) you no longer need to keep getDownloadURL synchronized or MyCallable non-static.


Using/reusing the SQL Command for several Inserts in the SQL Transaction I'm using the code below to save a record in my database. I was thinking that I am creating new instances of the SQL Command object and fear that I might be bloating the system memory and not doing proper garbage collection for unused objects. Can anyone point out which parts of this code I can improve?

By the way, this is the first time I've used SQL Transactions.

Private Sub SaveRecord()
    Dim type As Integer = 0

    Try
        Using dbConn As New SqlConnection(strConnStr)
            Try
                dbConn.Open()

                _params = New List(Of SqlParameter)

                ' Register record
                If formMode = 1 Then
                    dbCmd = dbConn.CreateCommand
                    dbTran = dbConn.BeginTransaction(IsolationLevel.ReadCommitted, "RBT")

                    If OrganismID = -1 Then
                        dbCmd = New SqlCommand("spNewZEntity", dbConn, dbTran)
                        dbCmd.CommandType = CommandType.StoredProcedure
                        AssignZEntityParam()

                        ZEntityID = dbCmd.ExecuteScalar
                        If ZEntityID = Nothing Then Throw New Exception("Failed to Generate ZEntityID")
                    End If

                    dbCmd = New SqlCommand("spNewXEntity", dbConn, dbTran)
                    dbCmd.CommandType = CommandType.StoredProcedure

                    AssignXEntityParam()
                    XEntityID = dbCmd.ExecuteScalar()

                    If XEntityID = Nothing Then Throw New Exception("Failed to Generate XEntityID")

                    For Each AZ In _AZItems
                        If AZ.IDAZ = -1 Then
                            dbCmd = New SqlCommand("spNewAZ", dbConn, dbTran)
                            dbCmd.CommandType = CommandType.StoredProcedure

                            _params.Clear()
                            dbCmd.Parameters.AddWithValue("@AZName", AZ.nameAZ)
                            AZID = dbCmd.ExecuteScalar()

                            If AZID = Nothing Then Throw New Exception("Failed to Generate AZID")
                            type = 1
                        Else
                            type = 2
                        End If

                        dbCmd = New SqlCommand("spNewISO", dbConn, dbTran)
                        dbCmd.CommandType = CommandType.StoredProcedure

                        AssignCreateISOParam(type, AZ)
                        dbCmd.ExecuteNonQuery()
                    Next

                    dbTran.Commit()
                    dbConn.Close()

                    MsgBox("Done.", MsgBoxStyle.Information)
                End If

                flagSaved = True
                Dispose()
            Catch ex As Exception
                CreateLog("SaveRecord", ex.Message, ex)

                Try
                    MsgBox("Attempting to rollback changes to database...", MsgBoxStyle.Information, emParsing)

                    If Not dbTran.Connection Is Nothing Then
                        dbTran.Rollback("RBT")
                        dbTran.Dispose()

                        MsgBox("Rollback successful.", MsgBoxStyle.Information, emParsing)
                    Else
                        MsgBox("Failed to rollback changes. Please contact your system administrator.",
                               MsgBoxStyle.Exclamation, emParsing)
                    End If
                Catch ex2 As SqlException
                    CreateLog("SaveRecord/Rollback Failed", ex2.Message, ex2)
                    MsgBox("Failed to rollback changes. Please contact your system administrator.", MsgBoxStyle.Exclamation, "Rollback Failed")
                End Try
            Finally
                dbCmd.Dispose()
            End Try
        End Using
    Catch ex As Exception
        CreateLog("SaveRecord", ex.Message, ex)
    End Try
End Sub

Not clear what AssignCreateISOParam(type, AZ) is and why it is above dbCmd.ExecuteNonQuery()

Create the command and parameter(s) outside the loop.  In the loop just assign the value and execute.

dbCmd = New SqlCommand("spNewAZ", dbConn, dbTran)


How do I compare two integer value hashes? I created a function that compares two hashes and returns a report on whether the subtraction of their values is negative or not.

The problem comes down to having a cost hash for a building and a current resources hash like:

cost = {:wood => 300, :stone => 200, :gold => 100}
reqs = {:wood => 200, :stone => 220, :gold => 90}

This one should return a report hash like:

report = { :has_resources => false, :has_wood =>  true, :has_stone => false, :has_gold => true }

Now, in the cost hash, :gold, :stone or :wood can be nil, i.e. non-existent.

My first attempt is definitely not the Ruby way and I don't like the function. It works, but I want to find a way to write it in a better manner:

def has_resources?(cost)
  report = { :has_resources => true, :has_wood =>  true, :has_stone => true, :has_gold => true }
  if not cost[:wood].nil?
    if self.wood < cost[:wood]
      report[:has_wood] = false
      report[:has_resources] = false
    end
  end
  if not cost[:stone].nil?
    if self.stone < cost[:stone]
      report[:has_stone] = false
      report[:has_resources] = false
    end
  end
  if not cost[:gold].nil?
    if self.gold < cost[:gold]
      report[:has_gold] = false
      report[:has_resources] = false
    end
  end

end

How should I rewrite this? I don't like the .nil? checks here, but I have to include them since the < operator does not work on nil objects. I also don't like having so many ifs.

Here's a version that is a bit more DRY. It also makes use of ruby's default value for hashes, and I threw in a dummy class in order to make the tests actually run.

This could probably be done even better, but one thing you really should try to achieve when writing ruby code (or any code at all, for that matter) is to try not to repeat yourself too much. That's where DRY come in.

class CostCalculation
  attr_accessor :wood, :stone, :gold

  def initialize(reqs)
    self.wood = reqs[:wood] || 0
    self.stone = reqs[:stone] || 0
    self.gold = reqs[:gold] || 0
  end

  def has_resources?(cost)
    report = Hash.new(true)
    cost.default = 0

    attributes.each do |key, value|
      report[:"has_#{key}"] = cost[key] <= value
    end

    if report.values.include?(false)
      report[:has_resources] = false
    end

    report
  end

  def attributes
    { wood: self.wood, stone: self.stone, gold: self.gold }
  end
end

...and here are some specs for it as well:

describe CostCalculation do
  it "has enough resources for something that's free" do
    report = CostCalculation.new({}).has_resources?({})
    report[:has_resources].should == true
  end

  %w[wood stone gold].each do |resource|
    it "does not have enough resources if #{resource} is missing" do
      cost = {resource.to_sym => 1}
      report = CostCalculation.new({wood: 0, stone: 0, gold: 0}).has_resources?(cost)

      report[:has_resources].should == false
      report[:"has_#{resource}"].should == false
    end
  end
end


Solve simple derivative questions Built my first script and its function is to solve simple derivative questions. How can I improve this code? This is my first time posting.

#This is Simple Derivative Solver
 def derivative():
     print("This program will give you the first derivative.")
     input("Press ENTER to continue...")
     cf=int(input("Enter the coefficient of the term..."))
     exp=int(input("Enter the exponent of the term..."))
     if exp==1 and cf==1:
        print("The answer is", 1)
     elif exp<0 and cf<0:
        print("The answer is:", abs(exp*cf), "x^", (exp-1))
     elif exp>0:
        print("The answer is:", exp*cf, "x^", (exp-1))
     elif exp==0:
        print("The answer is:", 0)
     elif exp<0:
        print("The answer is:", abs(exp*cf), "x^", (exp-1))
     elif cf==0:
        print("The answer is:", 0)
     elif cf<0:
        print("THe answer is:", cf*exp, "x^", (exp-1))
derivative()

Trust the math. If exp<0 and cf<0, their product will be positive, so abs is redundant. BTW, in case of elif exp < 0, call to abs leads to an incorrect answer.
In any case, \$(ax^n)'\$ always equals to \$anx^{n-1}\$. Mathematically speaking there are no special cases. There are special cases when it comes to printing:

\$n = 0\$: you (correctly) print 0
\$n = 1\$: you print x^0, but it is better to omit it altogether
\$n = 2\$: you print x^1, but it is better to print just x.
\$n < 0\$: consider printing parenthesis around the exponent.

Separate concerns. The derivative shall only compute the derivative. Input shall be handled separately.
The call to derivative should be conditionalized:

if __name__ == `__main__`:
    derivative()

This enables you to import your code in another project.


A small custom encryption / decryption script I have made a small script for encrypting data. Now I'm wondering how efficient it is for normal users who aren't programmers and for people who are programmers.

I mean if you don't have this script there is no way to find out what the text is right?

function cipher(message, action) {
  var text = message;
  var encrypted = "";

  for(var i = 0; i < text.length; i++) {
    var ASCII = text[i].charCodeAt(0);
    var n = null;

    if(i % 2 == 0) {
      n = action == 'encrypt' ? ASCII + 4 : ASCII - 4;
    }

    else if(i % 2 == 1) {
      n = action == 'encrypt' ? ASCII + 7 : ASCII - 7;
    }

    var s = String.fromCharCode(n);

        encrypted += s;;
  }
   return encrypted;
}

console.log(cipher("Hello World", 'encrypt'))

If you were to make it more secure, what would you add? I was thinking of a complete custom random mapping for each letter but retrieving it after decryption is something I can't figure out yet.

This is a simple variant of the caesar-cipher, namely a vigenere-cipher with the fixed key 'EH'.  Any variant of these ciphers is considered a toy-grade cipher that is trivially crackable if the attacker has gathered enough ciphertext to analyze the letter frequencies.  There is no way to make this class of ciphers secure, except to allow an insanely long and random key to be used, in which case it becomes a one-time pad.

If, despite acknowledging that it cannot be made secure, you still wanted to try to make some improvements to this code as a learning exercise, you could…

Allow the key to be specified as a parameter rather than hard-coded as alternating between +4 and +7
Eliminate the text variable
Write var ASCII = text[i].charCodeAt(0); as var unicode = message.charCodeAt(i);
Avoid repeated string concatenation with encrypted += s;;.  Since strings in JavaScript are immutable, you are actually creating a new string and copying the entire old string each time you want to append one character.  Instead, you should use Array.join():

var encrypted = new Array(message.length);
for (var i = 0; i < message.length; i++) {
    …
    encrypted[i] = …;
}
return encrypted.join('');