Optimization for histogram computation algorithm in C Next is an algorithm to calculate the histogram for a given YUV420sp image.  The YUV data is given by the hardware camera as an unsigned char*.

The algorithms works correctly, but I'm looking to optimize it for speed.

void calculateHistogram(const unsigned char* yuv420sp, const int yuvWidth, const int yuvHeight,
                        const int canvasHeight, int* histogramBins)
{
    const int BINS = 256;

    // Clear the output

    memset(histogramBins, 0, BINS * sizeof(int));

    // Get the bin values

    const unsigned int totalPixels = yuvWidth * yuvHeight;

    for (int index = 0; index < totalPixels; index++)
    {
        histogramBins[yuv420sp[index]]++;
    }

    // Resolve the maximum count in bins

    int maxBinValue = 0;

    for (int index = 0; index < BINS; index++)
    {
        if (histogramBins[index] > maxBinValue)
        {
            maxBinValue = histogramBins[index];
        }
    }

    maxBinValue = maxBinValue == 0 ? 1 : maxBinValue;

    // Normalize to fit into the histogram UI canvas height

    for (int index = 0; index < BINS; index++)
    {
        histogramBins[index] = histogramBins[index] * canvasHeight / maxBinValue;
    }
}

Style

Everything looks quite good to me.

The only thing that I find a bit disturbing is the empty line after comments : it split relevant things apart and it makes the code harder to read.

You could add a bit of documentation explaining what your code does.

Making things more simple

After the for-loop to compute the max value, you perform maxBinValue = maxBinValue == 0 ? 1 : maxBinValue;. As far as I can understand, the point is not to have a division by 0 if max is 0 which corresponds to a situation where histogramBins consist only of 0s which should happen only of totalPixels <= 0 which is quite unlikely.

In any case :

I find the usage of a ternary operator a bit inappropriate here as it might as well be a simple : if (maxBinValue == 0) maxBinValue = 1;. Alternatively, if you were to define functions (or macros) for min and max, this could be : maxBinValue = max(1, maxBinValue).
You could get rid of this is a very very simple way : if you were to assign 1 to maxBinValue in the first place (before the loop), you wouldn't have to worry about it being equal to 0.

Please note that if you were to define a max function/macro, it could be useful inside the loop too as it would become : maxBinValue = max(histogramBins[index],maxBinValue).

Getting rid of magic numbers

This 256 in const int BINS = 256; definitly sounds like a value I've seen in other places. Indeed, it is not just a random setting I can update if I ever want to : it corresponds to the maximum value yuv420sp[index] can take. Its type is unsigned char and the maximum value can be retrieved from limits.h : doing const int BINS = UCHAR_MAX would probably make things easier to understand.


Dynamically-resizable array implementation, for use in a game-dev-related library I've been working on a library that includes a list-like structure, which I use internally multiple times to store different types of structs. However, I'm not entirely confident with my ability to safely manage memory in C yet, so I'd appreciate if someone could let me know if there's anything wrong with my code (it seems to work just fine.)

This is the code for the list structure (with some bits from the associated header put at the top for simplicity.)

// Put this at the top of structs to make it possible to use them in a cg_mem_list
#define CG_ITEMIZABLE int cg_index;

// Free extra spaces after this many exist
#define CG_MEM_EXTRAS 10

typedef struct cg_mem_item {
    CG_ITEMIZABLE
} cg_mem_item;

typedef struct cg_mem_list {
    cg_mem_item** items;
    int num, capacity, itemsize;
} cg_mem_list;

void
cg_new_mem_list (cg_mem_list* list, int itemsize)
{
    list->items = 0;
    list->num = 0;
    list->capacity = 0;
    list->itemsize = itemsize;
}

cg_mem_item*
cg_mem_list_add (cg_mem_list* list)
{
    cg_mem_item* mitem = malloc (list->itemsize);

    if (list->capacity - list->num < 1) {
        list->capacity++;
        list->items = realloc (list->items, list->capacity * sizeof (cg_mem_item*));
    }

    mitem->cg_index = list->num;

    list->items[list->num++] = mitem;

    return mitem;
}

void
cg_mem_list_remove (cg_mem_list *list, cg_mem_item *item)
{
    list->num--;

    int index = item->cg_index;

    if (index < list->num) {
        list->items[index] = list->items[list->num];
        list->items[index]->cg_index = index;
    }

    if (list->capacity - list->num > CG_MEM_EXTRAS) {
        list->capacity = list->num;
        list->items = realloc (list->items, list->capacity * sizeof (cg_mem_item*));
    }

    free (item);
}

void
cg_destroy_mem_list (cg_mem_list* list)
{
    free (list->items);
}

This is how I use it:

// The struct being stored in the list
typedef struct some_data_type {
    CG_ITEMIZABLE
    int random_data;
} some_data_type;

int main (void) {
    // Declare and define the list
    cg_mem_list data_list;
    cg_new_mem_list (&data_list, sizeof(some_data_type));

    // Allocates a new item and sets it up
    some_data_type* first_item = (some_data_type*)cg_mem_list_add(&data_list);
    first_item->random_data = 12;

    // <More additions and operations>

    // This frees the item
    cg_mem_list_remove(&data_list, (cg_mem_item*)&first_item);
}

I'd also like to note that I didn't implement any checks malloc or realloc because this is in a game-dev-related library, and I figure if it can't allocate any new memory it's probably gonna crash and burn either way. If you have any suggestions for how to gracefully handle this (other than just straight-up throwing an error and aborting, which is pretty much the same as crashing anyways for this sort of application honestly) I'd appreciate it.

Use a memory checker
There's a clear bug, identified by running the test program under Valgrind:
==21190== Invalid free()
==21190==    at 0x48369AB: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==21190==    by 0x109320: cg_mem_list_remove (215759.c:64)
==21190==    by 0x109388: main (215759.c:94)
==21190==  Address 0x1fff0006a8 is on thread 1's stack
==21190==  in frame #2, created by main (215759.c:82)

This is where a local variable, not allocated with malloc() has been passed to free().
There's also a leak:
==21190== 16 (8 direct, 8 indirect) bytes in 1 blocks are definitely lost in loss record 2 of 2
==21190==    at 0x48356AF: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==21190==    by 0x4837DE7: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==21190==    by 0x1091F8: cg_mem_list_add (215759.c:36)
==21190==    by 0x109366: main (215759.c:88)

This is only partially mitigated if we add a call to cg_destroy_mem_list, because the individual items still don't get reclaimed (that's what the "8 indirect" refers to).
Other issues
We need to include <stdlib.h>, to use malloc() and family.
We're completely missing the (necessary) error checking when we call malloc() and realloc(), both of which can return null pointers.  Remember to check the result of realloc() before overwriting the old pointer (which is still valid if realloc() failed).


Golang execute several go routine in loop and then wait I'm new in Go and excited with its easy-to-use concurrency implementation. However I'm not sure if I'm doing it right in Golang way.
Consider the code where I spawn process(i int, ch chan int) every loop as separate goroutine. Then the result which comes from the channel will be consumed by consume(ch chan int, wg *sync.WaitGroup).
I know that the channel is blocking, so let's implement buffered channel. And of course I don't want the process terminated before every goroutine finished the operation, so I add WaitGroup.

func main() {
    var wg sync.WaitGroup
    n := 5
    ch := make(chan int, n)
    for i := 0; i < n; i++ {
        println("Processing ", i)
        go process(i, ch)
        go consume(ch, &wg)
    }
    println("Finished the process")
    wg.Wait()
}

func consume(ch chan int, wg *sync.WaitGroup) {
    wg.Add(1)
    defer wg.Done()
    println("Result ", <-ch)
}

func process(i int, ch chan int) {
    ch <- (i * 5)
}

Am I doing it right? Or there are better way to do this?
Many thanks!

Every time you execute a go statement it is passed to the scheduler. What if scheduling is delayed? wg.Add(1) is not executed and wg.Wait() is true. For example, run your code in the Go Playground where GOMAXPROCS is 1.

package main

import (
    "sync"
)

func main() {
    var wg sync.WaitGroup
    n := 5
    ch := make(chan int, n)
    for i := 0; i < n; i++ {
        println("Processing ", i)
        go process(i, ch)
        go consume(ch, &wg)
    }
    println("Finished the process")
    wg.Wait()
}

func consume(ch chan int, wg *sync.WaitGroup) {
    wg.Add(1)
    defer wg.Done()
    println("Result ", <-ch)
}

func process(i int, ch chan int) {
    ch <- (i * 5)
}

Playground: https://play.golang.org/p/dQ_lFRz2Y8a

Output:

Processing  0
Processing  1
Processing  2
Processing  3
Processing  4
Finished the process

Make sure that all the wg.Adds are run before wg.Wait. Move the println("Finished the process") to the correct place after the wg.Wait. For example,

waiting.go:

package main

import (
    "runtime"
    "sync"
)

func main() {
    println("GOMAXPROCS", runtime.GOMAXPROCS(0))
    var wg sync.WaitGroup
    n := 5
    ch := make(chan int, n)
    for i := 0; i < n; i++ {
        println("Processing ", i)
        go process(i, ch)
        wg.Add(1)
        go consume(ch, &wg)
    }
    wg.Wait()
    println("Finished the process")
}

func consume(ch chan int, wg *sync.WaitGroup) {
    defer wg.Done()
    println("Result ", <-ch)
}

func process(i int, ch chan int) {
    ch <- (i * 5)
}

Playground: https://play.golang.org/p/3czBixAjxdT

Output:

GOMAXPROCS 1
Processing  0
Processing  1
Processing  2
Processing  3
Processing  4
Result  0
Result  5
Result  10
Result  15
Result  20
Finished the process

Run the Go data race detector to check for data races. It finds none.

$ go run -race waiting.go
GOMAXPROCS 4
Processing  0
Processing  1
Result  0
Processing  2
Result  5
Processing  3
Result  10
Processing  4
Result  15
Result  20
Finished the process
$


Escaping the prison - finding the shortest way I have given an assignment where I have to escape a labyrinth. The goal is to find the shortest way. I have done some research and there seem to be two strategies to solve the problem: the Depth-first search and Breadth-first search, where the first starts at the root (Starting point in maze) and explores as far as possible along each branch before backtracking and the second begins at a root node and inspects all the neighboring nodes. Then for each of those neighbor nodes in turn, it inspects their neighbor nodes which were unvisited, and so on.

I have implemented both algorithms (non-recursive implementations) that will go on until they find the end:

/**
 * A non-recursive implementation of DFS
 * @param maze
 */
void solveUsingDepthFirst(IMaze maze) {
    Stack<IMazePosition> candidates = new Stack<IMazePosition>();
    //insert start position
    candidates.push(maze.getStartPosition());

    IMazePosition currentPosition;
    IMazePosition nextPosition;
    while (!candidates.empty()) {
        currentPosition = candidates.pop();

        if (maze.isMazeSolved(currentPosition)) break;

        //mark the current position
        maze.markPosition(currentPosition, maze.getPathMark());
      // maze.printMaze();
        // Check for possible ways to go
        nextPosition = currentPosition.north();
        if (maze.canMove(nextPosition)) candidates.push(nextPosition);
        nextPosition = currentPosition.east();
        if (maze.canMove(nextPosition)) candidates.push(nextPosition);
        nextPosition = currentPosition.south();
        if (maze.canMove(nextPosition)) candidates.push(nextPosition);
        nextPosition = currentPosition.west();
        if (maze.canMove(nextPosition)) candidates.push(nextPosition);
    }

    System.out.println(!candidates.empty() ? "Done" : "Sorry, could not do it");
    maze.printMaze();
}

/**
 * A non-recursive implementation of BFS
 * @param maze
 */
void solveUsingBreadthFirst(IMaze maze) {
    LinkedList<IMazePosition> candidates = new LinkedList<IMazePosition>();
    //insert start position
    candidates.add(maze.getStartPosition());

    IMazePosition currentPosition;
    IMazePosition nextPosition;
    while (!candidates.isEmpty()) {
        currentPosition = candidates.removeFirst();

        if (maze.isMazeSolved(currentPosition)) break;

        //markPosition the current position
        maze.markPosition(currentPosition, maze.getPathMark());
     //   maze.printMaze();

        // Check for possible ways to go
        nextPosition = currentPosition.north();
        if (maze.canMove(nextPosition)) candidates.add(nextPosition);
        nextPosition = currentPosition.east();
        if (maze.canMove(nextPosition)) candidates.add(nextPosition);
        nextPosition = currentPosition.south();
        if (maze.canMove(nextPosition)) candidates.add(nextPosition);
        nextPosition = currentPosition.west();
        if (maze.canMove(nextPosition)) candidates.add(nextPosition);
    }

    System.out.println(!candidates.isEmpty() ? "Done" : "Sorry, could not do it");
    maze.printMaze();
}

The map (or the maze) is represented as char[][]. The MazePosition is a representation of coordinates:

public MazePosition(int x, int y) {
    this.x = x;
    this.y = y;
}

Now with that code, I have all the possible paths from start to end that I was able to find until I found the end for the first time - is it fair to assume that the shortest path is amongst them?
Given that I have the possible paths, how would I go with finding the shortest? And, is the code for path generation any good at all? Can I optimize any of the routines I already have.

Also, as far as I know, there are no "holes" in the maze, which means I can always stick to the wall.

Reviewing Code:

Names

When naming classes in hierarchies, share the naming conventions used in the standard libraries.  Therefore, your top level interface should be Maze, not IMaze.  Likewise, IMazePosition.

Repetition

You are repeating yourself both small and large here.

Small: notice that you repeat yourself for each of the cardinal directions (north, south, east, west).  That code is screaming for a loop:

for(MazePosition nextPosition : crntPosition.getNeighbors()) {
    if (maze.canMove(nextPosition)) candidates.add(nextPosition);
}

Large: notice that solveUsingDepthFirst and solveUsingBreadthFirst are the same except that they order the points differently.  This suggests that you have overlooked an abstraction: a SearchOrder.

public interface SearchOrder {
    boolean isEmpty();
    MazePosition next();
    void add(MazePosition candidate);
}

public class DepthFirstSearchOrder implements SearchOrder {
   Stack<MazePosition> stack = new Stack<MazePosition> ();

   // implement the interface accessing this.stack
}

public class BreadthFirstSearchOrder implements SearchOrder {
   // notice that candidates is a List<>, because the interface
   // really doesn't care about the implementation.
   List<MazePosition> candidates = new LinkedList<MazePosition> ();

   // implement the interface using this.candidates
}

void solveUsingBreadthFirst(Maze maze) {
    solve(maze, new BreadthFirstSearchOrder());
}

void solve(Maze maze, SearchOrder order) {
    // ...
}

Missing Requirements

It looks like you aren't keeping track of the moves you have made; your existing example seems to tell you if a solution exists, but it doesn't actually report what the solution is.  This may or may not be a flaw - depending on the requirements.

Also, it's not fair to assume that your shortest solution in among them.  Imagine a maze that is a big ring, with the exit right next to the start - your depth first search can go the wrong way around the ring, giving you the longest solution instead of the shortest.  (The first result found in a breadth first search will be the shortest).

What you are missing is the ability to measure how far you have traveled when you reach the exit.  So you need something to count the number of moves you have made, so that you can tell if the current path you are taking has improved on your best so far.

If you had such a thing, you could (by generalizing it further) abandon search branches when they are guaranteed to be worse than the solution you have already found....


Advent of Code 2020 - Day 1: finding 2 or 3 numbers that add up to 2020 Next: Advent of Code 2020 - Day 2: validating passwords
Problem statement
I decided to take a shot at Advent of Code 2020 to exercise my Rust knowledge.  Here's the task for Day 1:

Day 1: Report Repair
[...]
Specifically, they need you to find the two entries that sum to 2020
and then multiply those two numbers together.
For example, suppose your expense report contained the following:
1721
979
366
299
675
1456

In this list, the two entries that sum to 2020 are 1721 and 299.
Multiplying them together produces 1721 * 299 = 514579, so the correct
answer is 514579.
[...]
Part Two
[...] They offer you a second one if you can find three numbers in
your expense report that meet the same criteria.
Using the above example again, the three entries that sum to 2020 are
979, 366, and 675. Multiplying them together produces the answer,
241861950.

The full story can be found on the website.
My solution
src/day_1.rs
use {anyhow::Result, std::io::prelude::*};

pub type Number = u64;

pub fn parse_data<R: BufRead>(reader: R) -> Result<Vec<Number>> {
    reader.lines().map(|line| Ok(line?.parse()?)).collect()
}

// Finds a pair of numbers in `data` that add up to `sum`
pub fn find_pair(mut data: Vec<Number>, sum: Number) -> Option<(Number, Number)> {
    data.sort_unstable();

    find_pair_in_sorted(&data, sum)
}

// Finds a triple of numbers in `data` that add up to `sum`.
pub fn find_triple(mut data: Vec<Number>, sum: Number) -> Option<(Number, Number, Number)> {
    data.sort_unstable();

    while let Some(number) = data.pop() {
        let sub_sum = match sum.checked_sub(number) {
            Some(sub_sum) => sub_sum,
            None => continue,
        };

        if let Some((a, b)) = find_pair_in_sorted(&data, sub_sum) {
            return Some((a, b, number));
        }
    }

    None
}

// Finds a pair of numbers in `data` that add up to `sum`
// by binary searching for `sum - n` for each number `n` in `data`.
// `data` must be sorted in ascending order.
pub fn find_pair_in_sorted(mut data: &[Number], sum: Number) -> Option<(Number, Number)> {
    while let Some((&number, new_slice)) = data.split_last() {
        data = new_slice;

        let target = match sum.checked_sub(number) {
            Some(target) => target,
            None => continue,
        };

        if data.binary_search(&target).is_ok() {
            return Some((target, number));
        }
    }

    None
}

src/bin/day_1_1.rs
use {
    anyhow::{anyhow, Result},
    aoc_2020::day_1::{self as lib, Number},
    std::{fs::File, io::BufReader},
};

const PATH: &str = "./data/day_1/input";
const SUM: Number = 2020;

fn main() -> Result<()> {
    let file = BufReader::new(File::open(PATH)?);
    let data = lib::parse_data(file)?;

    let (a, b) = lib::find_pair(data, SUM)
        .ok_or_else(|| anyhow!("cannot find pair that adds up to {}", SUM))?;
    println!("{} * {} = {}", a, b, a * b);

    Ok(())
}

src/bin/day_1_2.rs
use {
    anyhow::{anyhow, Result},
    aoc_2020::day_1::{self as lib, Number},
    std::{fs::File, io::BufReader},
};

const PATH: &str = "./data/day_1/input";
const SUM: Number = 2020;

fn main() -> Result<()> {
    let file = BufReader::new(File::open(PATH)?);
    let data = lib::parse_data(file)?;

    let (a, b, c) = lib::find_triple(data, SUM)
        .ok_or_else(|| anyhow!("cannot find triple that adds up to {}", SUM))?;
    println!("{} * {} * {} = {}", a, b, c, a * b * c);

    Ok(())
}

Crates used: anyhow 1.0.37
cargo fmt and cargo clippy have been applied.

Your code is well done and shows familiarity with Rust.
The best I can do is comment on some parts of the code and provide two small, concrete pieces of advice. There is very little room for improvement in the code.
I see your Vecs in function parameters are well motivated. The one in find_triple is motivated by the need to do pop, and the one in find_pair just follows the other for consistency between parts of the task.
The decision to introduce Number as a type alias is ok, and works and looks fine, but keep in mind that it adds an extra mental hop for the reader.
Return types
Acting by intuition I'd change the return types to have const-length arrays instead of tuples. This brings attention to the fact that the array/tuple is homogeneous. I would change:
-> Option<(Number, Number, Number)> and return Some((a, b, number));
to
-> Option<[Number; 3]> and return Some([a, b, number]);
Typo?
It's good to be correct and consistent in naming. The function name is find_triple - but 'triple' is not a noun, 'triplet' is.


Calculate last month, last 3 months, and last 6 months from today I am looking for some ideas on how this could be improved:

switch ($periodValue) {
    case "lastmonth":
        $until = mktime(0, 0, 0, date('n'), 1, date('Y'));
        $from = mktime(0, 0, 0, date('n', $until) - 1, 1, date('Y', $until));
        break;
    case "last3month":
        $until = mktime(0, 0, 0, date('n'), 1, date('Y'));
        $from = mktime(0, 0, 0, date('n', $until)-3, 1, date('Y', $until));
        break;
    case "last6month":
        $until = mktime(0, 0, 0, date('n'), 1, date('Y'));
        $from = mktime(0, 0, 0, date('n', $until)-6, 1, date('Y', $until));
        break;

How could I make this "smarter", so that it works out if we are near the end of the month it will show the last 30 days instead of the previous month?

Try the DateTime class. In combination with the DateInterval class you can do something like this:

$until = new DateTime();
$interval = new DateInterval('P2M');//2 months
$from = $until->sub($interval);
echo 'from' . $from->format('Y-m-d') . 'until' . $until->format('Y-m-d');

Also, you have to set the $until variable only once, before the switch statement.


Markup of a blog page and its posts Currently building my personal site using Jekyll, inuitcss and GitHub Pages. I'd like to hear your thoughts on the markup of my blog page and its post pages.

What do you think about handling logo's like this? Also I'm not sure about the headings. The page-title probably should be part of the main section.

blog markup:

            BlogPortfolioProfilImpressum

Freundliche Artikel

                    26. Januar 2014

We can't stop here

This is a post excerpt from the actual article. Usually the first paragraph.

                    15. Januar 2014

Introduction anyone?

This is a post excerpt from the actual article. Usually the first paragraph.

Author. Hosting. Used tools.

post markup:

            BlogPortfolioProfilImpressum

            26. Januar 2014

We can't stop here&hellip;

How're you doing? Eveything alright?

Another paragraph. Might be a conclusion.

Author. Hosting. Used tools.

Blog markup

The site title should be a h1.

Use an ul for the navigation links. Or, if you don’t want that, use at least a div element for each link, or a textual separator between each link.

The page title (+ the content under its scope) needs be enclosed in a sectioning element (probably section in this case). Also, don’t include the page title in the site’s header.

The main element should also contain the page title.

Each post excerpt should get its own article (as a general rule: don’t use headings in list items (e.g. li) unless they have a sectioning content parent).

You may use the time element for the dates.

Which leads to (excluding anything not related to the outline):

Freundliche Artikel

We can't stop here

Introduction anyone?

Post markup

Again, use the site heading as h1 (as a child of body, without any sectioning content element parents).

Again, use ul for the navigation links etc.

You may use the time element for the dates.

Your footer contains information about the whole page (body). So if one of the articles is written by a different author, the article should get its own footer.

Some further explanations from the comments:

(a) Why the site name should be bodys h1

Please see my answer to a different question. The last example (HTML + the corresponding outline) applies to your case.

In short: When you don’t use a heading for the site name, your main content heading will be used as the heading for the whole body. Everything else on this page will be in scope of this heading. But there are things on your page which shouldn’t be in its scope, for example your navigation.

In your current HTML, the nav (which clearly represents navigation for the whole site) would be in the scope of "Freundliche Artikel" (while your nav might be friendly, it’s clearly not an article ;-)).

(b) Why the logo can be used for (a)

(You need to agree with (a) first.)

Headings are not for text only. A logo typically represents the site name (blog name, company name, whatever). It doesn’t matter if the logo includes a textual representation or if it’s graphics only. In both cases, the alt text would be the textual site name, as this is what the logo stands for.

(c) Why you can’t use sectioning elements (for your main content) when your main content is not included in a sectioning element and you have a site heading

Let’s take this (wrong) example:

Artikelschmiede

Freundliche Artikel

We can't stop here

Introduction anyone?

The outline is:

1. Artikelschmiede
  1.1 Freundliche Artikel
  1.2 We can't stop here
  1.3 Introduction anyone?

But the outline should be:

1. Artikelschmiede
  1.1 Freundliche Artikel
    2.1 We can't stop here
    2.2 Introduction anyone?

So to get the correct outline, use a sectioning element for the whole main content (typically section or article):

Artikelschmiede

Freundliche Artikel

We can't stop here

Introduction anyone?


Object iteration and appending to form In this code, I'm getting the json from backend.  Once I find the json availability, I loop and make 5 columns.  Once the columns are made, I append after the legend of my form.

Can this be improved any further?

if(data.hasOwnProperty("allLocales")){

            var col0 = $("<fieldset />"),
                col1 = $("<fieldset />"),
                col2 = $("<fieldset />"),
                col3 = $("<fieldset />"),
                col4 = $("<fieldset />");

            $.map(data["allLocales"], function(value, i){

                var name = value.name;

                if(i % 5 === 0 ){
                    col0.append($("<label />").text(name).prepend($("<input type='checkbox' />").attr("value", name)));
                }else if(i % 5 === 1){
                    col1.append($("<label />").text(name).prepend($("<input type='checkbox' />").attr("value", name)));
                }else if(i % 5 === 2){
                    col2.append($("<label />").text(name).prepend($("<input type='checkbox' />").attr("value", name)));
                }else if(i % 5 === 3){
                    col3.append($("<label />").text(name).prepend($("<input type='checkbox' />").attr("value", name)));
                }else if(i % 5 === 4){
                    col4.append($("<label />").text(name).prepend($("<input type='checkbox' />").attr("value", name)));
                }

            })

            $("form legend").after(col0,col1,col2,col3,col4).end().find("span.submit").css({display:"block"});
        }

First you should not create on function for each column.
In place of :

var col0 = $("<fieldset />"),
    col1 = $("<fieldset />"),
    col2 = $("<fieldset />"),
    col3 = $("<fieldset />"),
    col4 = $("<fieldset />");

You should use :

var col_type  = "<fieldset />";

var cols      = {}

And then add it to your JSON :

cols["col"+ i % 5]  = $(col_type)...

So, you'll get a JSON object of this form :

{
  "col0":   <YOUR_jQuery element>,
  "col1":   <YOUR_jQuery element>,
  "col2":   <YOUR_jQuery element>,
  "col3":   <YOUR_jQuery element>,
  "col4":   <YOUR_jQuery element>
}

JSON are not support in jQuery for after() iteration. You must use an Array.

cols.push($(col_type)...)

And you won't need one variable for each new column.

Could you give a sample of your data.allLocales ?
And a sample of your HTML code ?

And no if(data.hasOwnProperty("allLocales")) is needed

Described :

for( column in data["allLocales"] ) {
  var name = data["allLocales"][column].name

  $("form legend").after(
    $("<fieldset />")
    .append(
      $("<label />")
      .text(name)
      .prepend(
        $("<input type='checkbox' />")
        .attr("value", name)
      )
    )
  )
 .end()
 .find("span.submit")
 .css({display: "block"});
}

Shorter :

for( column in data["allLocales"] ) {
  var name = data["allLocales"][column].name
  $("form legend").after($("<fieldset />").append(
    $("<label />").text(name).prepend($("<input type='checkbox' />").attr("value", name)))).end().find("span.submit").css({display: "block"}
  )
}


Merge sort implementation with vectors I'm new to algorithms. To be honest I am not really sure if my implementation is right. However, the output is sorted correctly in ascending order so perhaps it's correct. Please feel free to give critiques and suggest how to improve its overall efficiency.

How can improve its performance and readability?
Have I used bad practice?
Did I implement the algorithm correctly?

Main.cpp
#include <vector>
#include <iostream>
using namespace std;
void sort(vector<int> & bar);
void mergeSort(vector<int>&left,vector<int>& right,vector<int>& bars);

int main() {
    vector<int> nums;
    for (size_t i = 0; i < 11; i++)
        nums.push_back(rand() % 1000);

    sort(nums);

    for (size_t i = 0; i < nums.size(); i++)
        cout << nums[i] <<endl;
    system("pause");
    return 1;
}
void sort(vector<int> & bar) {
    if (bar.size() <= 1) return;

    int mid = bar.size() / 2;
    vector<int> left;
    vector<int> right;

    for (size_t j = 0; j < mid;j++)
        left.push_back(bar[j]);
    for (size_t j = 0; j < (bar.size()) - mid; j++)
        right.push_back(bar[mid + j]);

    sort(left);
    sort(right);
    mergeSort(left, right, bar);
}
void mergeSort(vector<int>&left, vector<int>& right, vector<int>& bars)
{
    int nL = left.size();
    int nR = right.size();
    int i = 0, j = 0, k = 0;

    while (j < nL && k < nR)
    {
        if (left[j] < right[k]) {
            bars[i] = left[j];
            j++;
        }
        else {
            bars[i] = right[k];
            k++;
        }
        i++;
    }
    while (j < nL) {
        bars[i] = left[j];
        j++; i++;
    }
    while (k < nR) {
        bars[i] = right[k];
        k++; i++;
    }
}

Output

Overview

Good first try. It seems like it works. But currently it only works for vectors and ints.

Some thoughts for improving the algorithm in the future.

If you look at most C++ algorithms they don't take a container as a parameter (vector<> is a container), instead they take iterators into the container. That way the algorithm can be used on any type of container (or a sub range of a container).

Also be using templates you can make the code work with any type that supports the comparison operator less than operator<.

Code Review

Vertical White Space

Nothing wrong here:

#include <vector>
#include <iostream>
using namespace std;
void sort(vector<int> & bar);
void mergeSort(vector<int>&left,vector<int>& right,vector<int>& bars);

But it seems a bit cramped all together. If you add some judicious vertical white space you can make it easier to read. I tend to separate logically distinct parts with an extra blank line. Like this:

#include <vector>
#include <iostream>

using namespace std;

void sort(vector<int> & bar);
void mergeSort(vector<int>&left,vector<int>& right,vector<int>& bars);

Using namespace

Please don't do this:

using namespace std;

Yes. All the C++ text books use it. But they are trying to save space because a book only has so much space available for printing. In real programs this is actually quite dangerous and should be avoided.

The reason the Standard namespace is called std is so that prefixing members with std:: is not a big hurdle.

Read this for details: Why is “using namespace std” considered bad practice?

Prefer prefix ++

    for (size_t i = 0; i < 11; i++)

OK. In this case (and a lot of cases) it makes no difference if you use the prefix or postfix version. But it can make a difference when the object being incremented is an object (lot of container iterators). A lot of the time when modifying C++ code, you change the type in one place and the type changes will cascade through your code, so it is best to always use the most efficient version no matter what the current type is. So in these cases prefer to use the prefix increment.

If you always use prefix ++ then you will always have the optimal increment.

    for (auto i = 0; i < 11; ++i) // Also note the use of auto

rand() is not very random

        nums.push_back(rand() % 1000);

This is the C random number generator. C++ has its own random number generator that is much better.See here for interface and example: http://en.cppreference.com/w/cpp/numeric/random

Also note that because you don't use srand() you will get the same sequence of numbers every time (which is a useful feature for testing).

Prefer Range based for:

When iterating over a container (something that supports begin/end) prefer to use the range based for.

    for (size_t i = 0; i < nums.size(); i++)
        cout << nums[i] <<endl;

    for (auto const& num: nums) {
        cout << num << "\n";
    }

Always use {} around sub blocks

I know it looks safe not to bother in places like this:

    for (size_t i = 0; i < nums.size(); i++)
        cout << nums[i] <<endl;

But it is a common type of error to add an extra indent line or use a function that happens to be multiple statement macro. To avoid these issues simply add braces.

    for (size_t i = 0; i < nums.size(); i++) {
        cout << nums[i] << "\n";
    }

Prefer '\n' over std::endl

The main difference in the two is that std::endl in addition to adding \n to the stream will flush the stream. The stream will already auto flush at the optimal times and forcing a manual flush will only result in a degradation in performance.

Don't use system dependent code

    system("pause");

This only works on windows. Easier to just ask the user for input.

    std::cout << "Hit Return to continue\n";
    std::string line;
    std::getline(std::cin, line);

Don't need return in main

    return 1;

The main() function is special. If you don't specify a return value then the compiler will auto generate one for you that returns 0.

So it has become traditional for simple applications that never fail to have no return statement as an indication that this application will never fail. If I see a return statement at the end of main I will look through main to find the other return statements that indicate a failure state.

You should also note that a return value of non zero from main() is an indication to the operating system that the application failed. So return 1; lets the OS know your application failed and can potentially cause it to throw up an error screen to the user.

The & and * are part of the type

The & and * are part of the type in C++ and usually placed syntactically with the type information.

void sort(vector<int> & bar) {

Putting a space between the type and the & just looks strange.

void sort(vector<int>& bar) {

Don't run if statement together

    if (bar.size() <= 1) return;

Prefer to use two lines for this (and the sub block brace).

    if (bar.size() <= 1) {
        return;
    }

When debugging the code and stepping through it this will make your life easier.

Use reserve

    int mid = bar.size() / 2;
    vector<int> left;
    vector<int> right;

To prevent potential re-allocation of the underlying storage you can use reserve to make sure these vectors have enough space.

    left.reserve(mid);
    right.reserve(bar.size() - mid);

Prefer to use standard algorithms rather than manual loops.

    for (size_t j = 0; j < mid;j++)
        left.push_back(bar[j]);
    for (size_t j = 0; j < (bar.size()) - mid; j++)
        right.push_back(bar[mid + j]);

There is a standard copy. You should check out the standard for a list of helpful algorithms. http://en.cppreference.com/w/cpp/algorithm

    std::copy(std::begin(bar), std::begin(bar) + mid, std::back_inserter(left));
    std::copy(std::begin(bar) + mid, std::end(bar), std::back_inserter(right));

One Line Per Variable (and use meaningful names)

    int i = 0, j = 0, k = 0;

There is a big push for self documenting code. Part of this is to make sure that you can understand what is happening by reading the names of the variables and the functions. So names like i,j,k are not meaningful (unless you happen to have done a lot of fortran programming).

    int leftLoop  = 0;
    int rightLoop = 0;
    int barLoop   = 0;

Stable Sort

An important property of sorting is to try and keep equal object in the same relative order after the sort. A sort that does this is called a stable sort. This means if two values are equal you should choose the one from the left rather than the right vector.

        // This is not stable.
        // When the objects are equal you select the one from the right.
        // To make this sort stable change to use `<=`
        if (left[j] < right[k]) {
            bars[i] = left[j];
            j++;
        }
        else {
            bars[i] = right[k];
            k++;
        }

Simplify using Ternary

This is a dangerous piece of advice. Use it judiciously as overuse can make the code harder to read. But sometimes it is useful to use ? : operator as it can make the code simpler and easier to read.

But sometimes it is useful to use the ternary operator instead of and if then else block. I think this is one of these places (others will disagree so use if you like).

        bars[i] = (left[j] <= right[k])
                    ? left[j++]
                    : right[k++];
        ++i;

Advanced Section

I would redesign this to use templates and iterators.

template<typename I>
void sort(I begin, I end);
template<typename I>
void merge(I leftBegin, I leftEnd, I rightBegin, I rightEnd, I destination);

When you start using templates and don't know what the type being sorted is things can get complicated because the underlying type may be very expensive to copy around. So you should prefer to move object rather than copy objects (for integers it makes no difference so I did not mention it above).

for(auto& val: container) {
    dest.emplace_back(std::move(val));
}

Also there is a move algorithm for moving objects from one container to another.

std::move(std::begin(container), std::end(container), std::back_inserter(dest));

You allocate a lot of temporary vectors (there is a left/right in each recursive call to sort()). You can pre-allocate these vectors and re-use them throughout your recursion (with a tiny bit of extra work).


Checking if two binary trees are identical Two binary trees are considered equal if they are structurally identical and the nodes have the same value.

Tree is defined as :

    /**
 * Definition for a binary tree node.
 * public class TreeNode {
 * int val;
 * TreeNode left;
 * TreeNode right;
 * TreeNode(int x) { val = x; }
 * }
 */

And my solution is

public boolean isSameTree(TreeNode p, TreeNode q) {

    if(null == p && null == q){
        return true;
    }

    if((null == p && null != q) || (null == q && null != p)){
        return false;
    }

    if(p.val == q.val){
       return isSameTree(p.left,q.left) ? isSameTree(p.right,q.right) :false;
    } else {
        return false;
    }

}

Please suggest any improvements.

Your code already does its job well. It can be shortened a bit:

public boolean isSameTree(TreeNode p, TreeNode q) {
    if (p == null || q == null) {
        return p == q;
    }

    return p.val == q.val
        && isSameTree(p.left, q.left)
        && isSameTree(p.right, q.right);
}

I flipped the operands of the == operators since having null in the right hand side reads more naturally.

I added spaces after the if and after the commas to follow the common formatting style.


Efficiency of a linq query on a nested structure For a survey website I've written this query to show the survey results, with a percentage for each chosen answer:

SurveyResultViewModel srvm =
    (from s in _context.Surveys
        where s.Id == id

        select new SurveyResultViewModel
        {
            Title = s.Title,
            ResponseCount = s.Results.Count,
            Questions = from q in s.Questions
                select new QuestionResultViewModel
                {
                    Text = q.Text,
                    Answers = from a in q.Answers
                        select new AnswerResultViewModel
                        {
                            Text = a.Text,
                            SelectedCount = a.Results.Count,

                            SelectedPercentage =
                            (double)a.Results.Count / q.Answers.Select(an => an.Results.Count).Sum()
                        }
                }
        }).Single();

However, I do think it can be improved, just not sure how. It runs 4 queries for every question in the survey because of the line that calculates the percentage.

It runs 4 queries for every question in the survey because of the line that calculates the percentage.

You're trying to retrieve data and make calculations at the same time. The issue stems from trying to do these two things. This is basically violating SRP.

Edit According to a coworker, this is not strictly an SRP violation, but it's very close to the spirit of what makes SRP important. I sort of see his point, but an in-depth discussion is a bit offtopic here.

Instead, try to only retrieve data at this point. There are other ways to do the calculation at a later stage:

Use an automated property
Perform the calculation on the in-memory objects

1. Only retrieve the needed information and store it in an appropriate spot.

We have to make sure that we retrieve all needed information that allows us to do the calculation at a later stage. Your percentage calculation relies on two pieces of information:

The result count of the current answer.
The combined result count of all answers that belong to the (parent) question.

(1) is already being retrieved (it's stored in SelectedCount).

(2) is not explicitly retrieved, but can already be derived by looking at the Answers property of a QuestionResultViewModel. In short, it can be found by calling myQuestionModel.Answers.Sum(x => x.SelectedCount)

So the first step is easy, simply omit the calculation:

// ...

Answers = from a in q.Answers
    select new AnswerResultViewModel
    {
        Text = a.Text,
        SelectedCount = a.Results.Count
    }

// ...

2. Option 1 - using an automated property.

I find this the cleanest solution, it forgoes the need to manually trigger a calculation. There are a few prerequisites here:

An AnswerResultViewModel needs to have a reference to its parent QuestionResultViewModel in order to access the total count. For the example, I will assume such a property exists by the name of Question.
If the calculation is expensive, you should reconsider using an automated property because it will be recalculated every time it is accessed. However, I don't consider your current example to be an expensive calculation.

The automated property, using an expression-bodied member:

public int SelectedPercentage =>
               (double)this.SelectedCount / this.Question.Answers.Sum(an => an.SelectedCount);

If you prefer a more traditional syntax:

public int SelectedPercentage
{
    get
    {
        return (double)this.SelectedCount / this.Question.Answers.Sum(an => an.SelectedCount);
    }
}

Minor changes:

Instead of .Select(lambda).Sum(), I simply used .Sum(lambda). It's slightly shorter and easier to read.
Instead of an.Results.Count, I used an.SelectedCount since it already contains the correct value.

3. Option 2 - Manual calculation.

I like this option less, but it's better in cases where the calculation is expensive and you want to avoid calculating the same value over and over.

After you've retrieved the value from the database, calculate the percentages.

If the Question navigational property exists:

foreach(var answer in srvm.Questions.SelectMany(q => q.Answers))
{
    answer.SelectedPercentage = (double)answer.SelectedCount / answer.Question.Answers.Sum(an => an.SelectedCount);
}

If there is no navigational property:

foreach(var question in srvm.Questions)
foreach(var answer in question.Answers)
{
    answer.SelectedPercentage = (double)answer.SelectedCount / question.Answers.Sum(an => an.SelectedCount);
}


Brute Force Algorithm in C This is a simple brute force algorithm that I have programmed in C.  All the program does is print out every possible combination of the given alphabet for the given length.

I would prefer suggestions on how to improve the algorithm, or decrease run-time.  Any other suggestions are acceptable though.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

static const char alphabet[] =
"abcdefghijklmnopqrstuvwxyz"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"0123456789";

static const int alphabetSize = sizeof(alphabet) - 1;

void bruteImpl(char* str, int index, int maxDepth)
{
    for (int i = 0; i < alphabetSize; ++i)
    {
        str[index] = alphabet[i];

        if (index == maxDepth - 1) printf("%s\n", str);
        else bruteImpl(str, index + 1, maxDepth);
    }
}

void bruteSequential(int maxLen)
{
    char* buf = malloc(maxLen + 1);

    for (int i = 1; i <= maxLen; ++i)
    {
        memset(buf, 0, maxLen + 1);
        bruteImpl(buf, 0, i);
    }

    free(buf);
}

int main(void)
{
    bruteSequential(3);
    return 0;
}

I'm going to skip reviewing the OP's code, which has already been reviewed quite nicely by others.  Instead I'm just going to show a much faster version and explain it.
What is the algorithm?
Basically the way the algorithm works is that a buffer is created that holds alphaLen^2 patterns, where alphaLen is the length of the alphabet.  A pattern is one combination, such as "aaaaa\n".  The reason that alphaLen^2 patterns are used is because the buffer is prepopulated with the last 2 letters already set to all possible combinations.  So for example, the buffer initially looks like (for length 5 patterns):
"aaaaa\naaaab\naaaac\n ... aaa99\n" (62*62 patterns, 23064 bytes in length)

On every iteration, the function uses write() to output the buffer, and then increments the third to last letter.  This involves writing that letter alphaLen^2 times (once per pattern).  So the first iteration goes like:
"aabaa\naabab\naabac\n ... aab99\n"
   ^      ^      ^           ^
   |      |      |           |
   +------+------+-----------+----- Characters written to buffer (62*62 changes)

Whenever the third to last character wraps around, then we also need to update the fourth to last letter.  If that wraps around, the fifth to last letter is also updated, etc.  This continues until the first letter wraps around, at which point we are done.
How fast is it?
For all testing, I outputted to /dev/null so that my hard drive speed would not be the limiting factor.  I tried the OP's program for 5 letter patterns but for me it took way too long (203 seconds).  So I'll use Edward's estimate of 18 seconds for the OP's program instead.  I also tested Edward's program on my machine, as well as a second program I wrote where I extended the algorithm to hardcode the last 3 characters instead of 2.  I am using Cygwin gcc (32-bit) with -O4 on a Windows desktop.  Here are the results:
Time (secs)  Length   Program
-----------  ------   -------
    18.0       5      syb0rg
     9.6       5      Edward
     0.4       5      JS1 (2 characters)
     0.4       5      JS1 (3 characters)

   665.0       6      Edward
    26.0       6      JS1 (2 characters)
    24.2       6      JS1 (3 characters)

  1513.7       7      JS1 (3 characters)

As you can see, this algorithm is very fast.
The code
Both programs are available here on GitHub.  I'll show the 2 character variation below:
static void generate(int maxlen)
{
    int   alphaLen = strlen(alphabet);
    int   len      = 0;
    char *buffer   = malloc((maxlen + 1) * alphaLen * alphaLen);
    int  *letters  = malloc(maxlen * sizeof(int));

    if (buffer == NULL || letters == NULL) {
        fprintf(stderr, "Not enough memory.\n");
        exit(1);
    }

    // This for loop generates all 1 letter patterns, then 2 letters, etc,
    // up to the given maxlen.
    for (len=1;len<=maxlen;len++) {
        // The stride is one larger than len because each line has a '\n'.
        int i;
        int stride = len+1;
        int bufLen = stride * alphaLen * alphaLen;

        if (len == 1) {
            // Special case.  The main algorithm hardcodes the last two
            // letters, so this case needs to be handled separately.
            int j = 0;
            bufLen = (len + 1) * alphaLen;
            for (i=0;i<alphaLen;i++) {
                buffer[j++] = alphabet[i];
                buffer[j++] = '\n';
            }
            write(STDOUT_FILENO, buffer, bufLen);
            continue;
        }

        // Initialize buffer to contain all first letters.
        memset(buffer, alphabet[0], bufLen);

        // Now in buffer, write all the last 2 letters and newlines, which
        // will after this not change during the main algorithm.
        {
            // Let0 is the 2nd to last letter.  Let1 is the last letter.
            int let0 = 0;
            int let1 = 0;
            for (i=len-2;i<bufLen;i+=stride) {
                buffer[i]   = alphabet[let0];
                buffer[i+1] = alphabet[let1++];
                buffer[i+2] = '\n';
                if (let1 == alphaLen) {
                    let1 = 0;
                    let0++;
                    if (let0 == alphaLen)
                        let0 = 0;
                }
            }
        }

        // Write the first sequence out.
        write(STDOUT_FILENO, buffer, bufLen);

        // Special case for length 2, we're already done.
        if (len == 2)
            continue;

        // Set all the letters to 0.
        for (i=0;i<len;i++)
            letters[i] = 0;

        // Now on each iteration, increment the the third to last letter.
        i = len-3;
        do {
            char c;
            int  j;

            // Increment this letter.
            letters[i]++;

            // Handle wraparound.
            if (letters[i] >= alphaLen)
                letters[i] = 0;

            // Set this letter in the proper places in the buffer.
            c = alphabet[letters[i]];
            for (j=i;j<bufLen;j+=stride)
                buffer[j] = c;

            if (letters[i] != 0) {
                // No wraparound, so we finally finished incrementing.
                // Write out this set.  Reset i back to third to last letter.
                write(STDOUT_FILENO, buffer, bufLen);
                i = len - 3;
                continue;
            }

            // The letter wrapped around ("carried").  Set up to increment
            // the next letter on the left.
            i--;
            // If we carried past last letter, we're done with this
            // whole length.
            if (i < 0)
                break;
        } while(1);
    }

    // Clean up.
    free(letters);
    free(buffer);
}


General solution to sum multiples of multiple numbers below a limit sumMultiples() is a working general solution to Project Euler's first problem.  Don't read it if you want to try it yourself.

This question is about preserving this general nature while improving performance, perhaps using math tricks like sumSkip() and sumIndetity().  Unfortunately everything I find that takes advantage of these tricks sacrifices the ability to have a general solution that works with any number of multiples and any limit.  Notice how using sumSkip() and sumIdentity() require hand tuning to work.  I'd like to use the same interface sumMultiples() uses and allow any number of base numbers with any values.  Well, short of causing overflows anyway.

In main sumSkip() and sumIdentity() are being used with the inclusion–exclusion principle in a rigid hand tuned form.  I'm suspecting there is a general way to apply this principle to this problem but frankly I can't see it.  I'll accept solutions that don't use it so long as they truly improve this solutions Big O Complexity.

I believe sumMultiples() is O(below x base) complexity.  I'm looking to improve on that.

Before linking possible duplicates or solutions please ensure they deal with the general requirement.

public static void main(String[] args) {
    int below = 1000;
    int[] base = {3, 5};

    System.out.println( sumMultiples(below, base) );

    // Making names shorter to help readability
    int b0 = base[0];
    int b1 = base[1];

    // Hand tuned to number and values.  Something I'd like to avoid.
    System.out.println( sumSkip(b0, below) + sumSkip(b1, below) - sumSkip(b0*b1, below) );
    System.out.println(  b0*sumIdentity(below/b0) +
                         b1*sumIdentity(below/b1-1) -
                         b0*b1*sumIdentity( below / (b0*b1) )  );
}

// General solution - any sized base with any values - uses brute force approach
private static int sumMultiples(int below, int... base) {
    int result = 0;
    boolean isMultiple;

    // Every integer below limit
    for(int i = 1; i < below; i++) {

        // Resetting dirty bit pattern
        isMultiple = false;

        // Every base
        for (int j = 0; j < base.length; j++) {

            // Stays false until a multiple is found. Then stays true.
            isMultiple |= i % base[j] == 0;
        }

        // Sum if i is a multiple of an integer in multiples
        if (isMultiple) {
            result += i;
        }
    }

    return result;
}

// Skip non multiples - still fairly brute force - less general
public static int sumSkip(int base, int below) {
    int result = 0;
    int multiple = base;

    while (multiple < below) {
        result += multiple;
        multiple += base;
    }

    return result;
}

// Sum using math
public static int sumIdentity(int n) {
    return n*(n+1)/2;
}

Output:

233168
233168
233168

After I provided a quick-glance alternative implementation yesterday, here comes a full review now. It's so different in approach I chose to add another answer for it. That said, let's get started:

I see in the comments to a now deleted answer that you don't want comments on the code you provided as main. I'll do it anyways, because you're doing some things that make me shiver:

System.out.println( sumMultiples(below, base) );

This is definitely nitpickish, but I personally prefer to not add additional spaces inside the braces of a method-call:

System.out.println(sumMultiples(below, base));

// Making names shorter to help readability
int b0 = base[0];
int b1 = base[1];

Don't do that. Shorter names are not readable. A readable name can be easily read out loud. That makes your comment a blatant lie in my eyes. Usually this would be the point where you'd have lost my respect as fellow programmer... I understand that you may sometimes want variable names shorter, and they are nice in the rest of your code. But the resoning is something ...

Oh and... Don't number your variables! This makes it difficult to process them in their mental context, and makes the code harder to understand.

// Hand tuned to number and values. Something I'd like to avoid
System.out.println( sumSkip(b0, below) + sumSkip(b1, below) - sumSkip(b0*b1, below) );
System.out.println( b0*sumIdentity(below/b0) +
                    b1*sumIdentity(below/b1-1) -
                    b0*b1*sumIdentity( below/ (b0*b1) )  );

Why are you doing this? There is some relatively complicated problematic and it's slammed into the reader's face in the main-method.

The main-method is supposed to be a place of high-level abstraction. I think reading a main-method should be just like reading English. A simple English sentence. Consider the following (as example):

public static void main(String[] args) {
    new Program().start();
}

Extract the complexity into methods. I figure it's fine to have this hand-tuned and doubt there is a way to avoid this when not working with an already generalized formula.

// General solution - any sited base with any values - uses brute force approach

This wants to be javadoc:

/**
 * General solution. Calculates the result using a brute-force approach
 *
 * @parameter below
 *     An integer representing the non-inclusive upper border of the problem.
 * @parameter base
 *     At least one integer representing the divisors for leftover-free division of numbers that are to be summed.
 */

For the sumMultiples, see my other answer. There's not really much else to say about that. Nice naming, simple and straightforward approach... Actually this goes for the rest of your code (aside from the non-javadoc javadocs) with one small exception:

How the **** does sumIdentity work? //Sum using math doesn't help me all that much ;)

Alas, that's it for now.


Basic Game Model in Scripting Language I've been working with Lua for a little bit, and I have finally come up with some code that can be extended to create the core of a game model.  I come from an object oriented background, so working with Lua is very different for me.

The purpose of this code would be to create the game model for a game with Lua.  Another programming language would load the scripts and then make the calls to create the Game object and to advance the turns.  Any information resulting from a game turn could be passed back to the rendering in the proper programming language.

I would love to hear about any aspect of this code, but I am new to Lua syntax so I am sure I am making rookie mistakes.

Game.lua

--This syntax loads other scripts, such as Player.lua
local player = require "Player"

local Game = {} -- the table representing the class, which will double as the metatable for the instances
Game.__index = Game -- failed table lookups on the instances should fallback to the class table, to get methods

-- syntax equivalent to "MyClass.new = function..."
function Game.new()
    local self = setmetatable({}, Game)

    self.players = {}

    self.currentTurn = 0

    return self
end

--Adding players to the game
function Game.addPlayers(self, numPlayers)
    --i has to be set to 1 for it to work properly
    for i = 1, numPlayers, 1 do
        self:addPlayer()
    end
end

function Game.addPlayer(self)
  table.insert(self.players, player.new(6))
end

--Advancing and reporting the game turns
function Game.advanceTurn(self)
  self.currentTurn = self.currentTurn + 1
end

function Game.getTurnNumber(self)
  return self.currentTurn
end

--test function to access player data
function Game.reportPlayers(self)
  for key,value in pairs(self.players) do
    print(key)
  end
end

--create the game "instance"
local game = Game.new()
game:addPlayers(2)

--these types of commands would be sent to Lua by a proper programming language
game:advanceTurn()
print(game:getTurnNumber())
game:advanceTurn()
print(game:getTurnNumber())
game:reportPlayers()

Player.lua

Player = {} -- the table representing the class, which will double as the metatable for the instances
Player.__index = Player -- failed table lookups on the instances should fallback to the class table, to get methods

-- syntax equivalent to "MyClass.new = function..."
function Player.new(startingHandSize)
    local self = setmetatable({}, Player)
    self.handSize = startingHandSize
    print(self.handSize)
    return self
end

function Player.set_handSize(self, newHandSize)
    self.handSize = newHandSize
end

function Player.get_handSize(self)
    return self.handSize
end

return Player

I don't notice any major issues with your code.

Use _name naming for private variables, since everything is global (and thus, accessible) by default in lua. You can use the normal variable name for getters and setters.
You can modify the getter (get_handSize) and setter (set_handSize) a little as functions in lua don't need the parameters:

function Player.new( startingHandSize )
    local self = setmetatable( {}, Player )
    self._handSize = startingHandSize
    return self
end
function Player:handSize( newHandSize )
    if not newHandSize then return self._handSize end
    self._handSize = newHandSize
end

You can now simply use player:handSize() or player:handSize(n) for getting and setting the private variable.
Since you are simply inserting new players to self.players table, you can iterate using ipairs to list them sequentially (depends on your game design).
You can save a little disk space by using the oop approach of defining functions instead of passing self every time.

That much was a basic approach to oop in lua. Now, extending the concept, you can use closures for proper private variables.

function Player.new( startingHandSize )
    local self = setmetatable( {}, Player )
    local h = (function()
        local _handSize = startingHandSize
        return function( self, newHandSize )
            if not newHandSize then return _handSize end
            _handSize = newHandSize
        end
    end)()
    self.handSize = h  -- self._handSize will always be nil
    return self
end

and in a similar fashion:

function Game.new()
    local self = setmetatable({}, Game)
    local c, a = (function()
        local _turn = 0
        return function( self )
            return _turn
        end, function( self )
            _turn = _turn + 1
        end
    end)()
    self.getTurnNumber, self.advanceTurn = c, a  -- self.currentTurn will always be nil
    return self
end


Loosely coupled movement I'm working on my ability to write loosely coupled applications that follow SOLID principles. This is a simple application trying to demonstrate the concept. Is it OK? Am I going along the right lines? What needs to be improved or changed?

I'd just like your overall opinion on the design structure and the code please. If you'd like a straightforward question to answer directly, then it'd be "Is this code that is appropriate to be used in production?"

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

namespace LooselyCoupled
{
public class Program
{
    static void Main(string[] args)
    {
        //Usage
        Human human = new Human();
        human.Name = "John";
        human.Age = 36;

        Cat cat = new Cat();
        cat.Name = "Molly";
        cat.Age = 8;

        MovementManager movement = new MovementManager(human);
        movement.Move();

        MovementManager movement2 = new MovementManager(cat);
        movement2.Move();
    }

    //Interface to act as a middle man (inversion) to redirect the otherwise narrow flow
    public interface iMovement
    {
        void Move();
    }

    //A manager that has no idea of any implementation, but only an interface
    public class MovementManager
    {
        private iMovement movement;

        public MovementManager(iMovement movement)
        {
            this.movement = movement;
        }

        public void Move()
        {
            movement.Move();
        }
    }

    //I've decided that every class that implements iMovement will have certain attributes
    //therefore I've decided to add an abstract class into the mix (is this OK?)
    public abstract class BaseMovementEntity : iMovement
    {
        public string Name { get; set; }
        public int Age { get; set; }

        public abstract void Move();
    }

    public class Human : BaseMovementEntity
    {
        public override void Move()
        {
            Console.WriteLine($"Name: {Name}, Age {Age} has begun moving...");
        }
    }
    public class Cat : BaseMovementEntity
    {
        public override void Move()
        {
            Console.WriteLine($"The little cat whose name is: {Name}, Age {Age} has begun moving to catch the dog...");
        }
    }
}
}

Let me share my thoughts. From the SOLID perspective your example is very simple and does not explicitly break any principles.

The only thing that you should not do in large-scale production code is to couple your application code (MovementManager, etc.) with execution host (Console app project).

What you should usually do is to put your application code into separate assembly(ies), employ some DI Container and create a Composition Root (also preferable in separate assembly). Application code/assemblies must not have any references to DI Container or Composition Root. On the other side, your execution host (Console App project) must reference Composition Root and Application Code assemblies, and resolve an instance of MovementManagerFactory via DI Container. Then you can use MovementManagerFactory to create your MovementManager with appropriate iMovement object.

You would need MovementManagerFactory because your MovementManager constructor requires iMovement object. To simplify things I'd rather remove that constructor and instead pass iMovement object to the Move() method directly:

public class MovementManager
{
    public void Move(iMovement movement)
    {
        movement.Move();
    }
}

As a side note: everywhere in your application when you use new operator to create an instance of a class, you potentially make your code tightly coupled. It is especially true when that 'new-ed' class belongs to another module or component.

Dependency Injection in .NET by Mark Seemann is an excellent book about design of loosely coupled application.


Renaming form controls and underlying code I was working on adjusting some variable naming on an Excel project and ran into an issue that the MSForm control names needed to be updated. When you change the properties of the control on the form, the underlying code for the form doesn't seem to update accordingly.

Examples:

  You rename btn_button1 to btn_populateList. Now when you click the button it still wants to call btn_button1_Click() rather than
  btn_populateList_Click().
  You rename inputbox1 to listToPopulate but when you run pre-existing code, it still refers to inputbox1 instead of the new
  name of the control.

Anyhow, I've always wanted to mess around to get VBA to do stuff in the VBE, so this was my chance!

The macro finds the target project and target form and then updates the control names and appends the string NEW to oldName such that it's oldNameNEW. This was done so I could easily revert the names back using the same procedure with left(len()-3).

Ideally, the array of new names would be populated by the user with descriptive names and would be populated outside of the For loop, obviously.

I'm sure there's some clever refactoring that can be done, but I wasn't sure if I'd break it.

Also, I'm worried I misunderstand how Forms call their macros given it's not designed the same way as controls on sheets.

Public Sub FindReplaceInEntireModule()
    Const TARGET_ADDIN As String = "bUTLAddIn"
    Const TARGET_FORM As String = "form_chtGrid"

    Dim targetProject As VBIDE.VBProject
    Dim targetModule As VBIDE.VBComponent
    Dim targetCode As VBIDE.CodeModule
    Dim controlIndex As Long
    Set targetProject = Application.VBE.VBProjects(TARGET_ADDIN)

    Set targetModule = targetProject.VBComponents(TARGET_FORM)
    Dim targetForm As Object
    Set targetForm = targetModule.Designer
    Dim numberOfControls As Long
    numberOfControls = targetForm.Controls.count

    Dim newControlNames() As String
    ReDim newControlNames(1 To numberOfControls)
    Dim oldControlNames() As String
    ReDim oldControlNames(1 To numberOfControls)

    For controlIndex = 1 To numberOfControls
        oldControlNames(controlIndex) = targetForm.Controls(controlIndex).name
        newControlNames(controlIndex) = oldControlNames(controlIndex) & "NEW"
        targetForm.Controls(controlIndex).name = newControlNames(controlIndex)
    Next

    Set targetCode = targetModule.CodeModule

    Dim findWhat As String
    Dim replaceWith As String

    Dim numberOfLines As Long
    numberOfLines = targetCode.CountOfLines
    Dim lineNumber As Long
    Dim lineText As String

    For controlIndex = 1 To numberOfControls
        findWhat = oldControlNames(controlIndex)
        replaceWith = newControlNames(controlIndex)
        For lineNumber = 1 To numberOfLines
            lineText = targetCode.Lines(lineNumber, 1)
            If InStr(1, lineText, findWhat, vbTextCompare) > 0 Then
                targetCode.ReplaceLine lineNumber, Replace(lineText, findWhat, replaceWith, , , vbTextCompare)
            End If
        Next
    Next

End Sub

You've basically implemented (parts of) a rename refactoring.

The problem is that the VBIDE API only gives you strings to work with, so you essentially have a "smart find & replace", but it's still a find & replace.

A LOT of things can go wrong when processing code without a symbol table.

Imagine a control named MyButton that you want to rename to SomeButton. Then you have this code in the module:

Dim MyButtonForeColor As Long
MyButtonForeColor = vbBlack

Your code will rename MyButtonForeColor too, because it contains MyButton - but did you mean to? In this case, probably.

UserForm controls are Public, so you can have code outside the form that does this:

With New UserForm1
    .Show vbModal
    ActiveSheet.Cells(1, 1) = .SomeTextBox.Text
End With

And that code will break when you rename SomeTextBox to anything else.

Or, you could have conflicting but unambiguous names in different scopes, that a simple find & replace will break:

UserForm1.SomeButton.Caption = UserForm2.SomeButton.Caption

Naming is hard. Renaming is even harder.

Having worked on Rubberduck for over two years, I can assure you that there is absolutely no way to implement any kind of rename refactoring without a symbol table - and that means lexing, parsing and resolving the VBA code you want to refactor.

Ctrl+H isn't a refactoring tool. Rubberduck is. I don't mean to sound like an ad, but your code needs to know exactly which code across the entire project is referencing the control you're renaming - and that simply isn't possible to implement reliably in VBA. Heck, Rubberduck has a proper parser and resolver, and still struggles with a number of edge cases.

I give you this code to play with - it's EVIL beyond words, I know... but it's 100% legal VBA code, and Rubberduck sorts it all out:

'Project Name = MyProject
'Module Name = MyModule
Option Explicit

Private MyVar As MyProject
Private MyVar1 As MyModule

Private Type MyProject
  MyModule As String
  MySub As String
End Type

Private Type MyModule
  MyVar As String
  String As MyProject
End Type

Private Type MySub
  MyVar As MyModule
  MyVar1 As MyProject
End Type

Private Type MyVar
  MyProject As MyModule
End Type

Sub MySub()

  Dim MyProject As MyProject
  Dim MySub As MySub
  Dim MyVar As MyVar

  MyVar.MyProject.MyVar = "Smith"
  MySub.MyVar1.MySub = MyProject.MySub

  'My brain hurts....

End Sub

Your strategy (I know you're only looking at controls, but the principle is the same) will fail to correctly rename MySub here (whichever you pick).

Your code looks great though - good naming, clean code, nice spacing, indentation, casing, a bit large for a single procedure though, but as I said I don't think any of it is relevant, since there's no way any identifier-renaming VBA macro can work [in all cases] simply by looking at context-less strings.


Get RSS feeds and store them into database It is my first program in Clojure. It read RSS feed's list from text file, get each feed and store  result into sqlite database.

project.clj:

(defproject cowl "0.1.0-SNAPSHOT"
  :description "FIXME: write description"
  :url "http://example.com/FIXME"
  :license {:name "Eclipse Public License"
            :url "http://www.eclipse.org/legal/epl-v10.html"}
  :dependencies [[org.clojure/clojure "1.8.0"],
                 [org.clojars.scsibug/feedparser-clj "0.4.0"]
                 [org.clojure/java.jdbc "0.3.7"]
                 [org.xerial/sqlite-jdbc "3.7.2"]]
  :main cowl.core/fetch
)

core.clj:

(ns cowl.core
  (:require [cowl.rss :as rss]
            [cowl.db :as db]))

(defn fetch []

  "Fetch information from RSS and store it to DB"

  (with-open [r (clojure.java.io/reader "settings\\feed-list.txt")]
    (doseq [line (line-seq r)
           entry (rss/process-feed line)]

      (db/insert-entity (:title entry) (:link entry) (:source entry)))))

entity.clj:

(ns cowl.entity)

(defn make-entity [title, link, source]
    {:title title, :link link, :source source})

rss.clj:

(ns cowl.rss
  (:require
    [cowl.entity :as entity]
    [cowl.db :as db]
    [feedparser-clj.core :as rss]))

(defn construct-entry [source, entry]
    (entity/make-entity (:title entry) (:link entry) source))

(defn process-feed [url]

    "Gets the feed URL and returns list of entites"
    (let [feed (rss/parse-feed url)
        construct-foo (partial construct-entry (:title feed))]

        (map construct-foo (:entries feed))))

db.clj:

(ns cowl.db
  (:require
    [clojure.java.jdbc :as sql]))

(def db {:classname "org.sqlite.JDBC", :subprotocol "sqlite", :subname "work.db"})

(defn create-db []
  (sql/execute! db ["drop table if exists entities"])
  (let [q (sql/create-table-ddl :entities
                                [:title :text]
                                [:link :text :primary :key]
                                [:source :text])]
  (sql/execute! db [q])))

(defn insert-entity [title, link, source]
    (let [result (sql/update! db :entities { :title title :source source} ["link=?" link])]
        (if (zero? (first result))
            (sql/insert! db :entities { :title title :link link :source source}))))

(defn get-records []
    (sql/query db "select id, title, link, source from entities"))

When using defn, the docstring goes before the arguments list, not after.

In order to prevent environment coupling, functions that use a db should accept it as an argument instead of accessing it via a Var. This may seem like an inconvenience, but it will yield much more maintainable code.

You should also improve the naming :) names likes construct-foo or process-feed are not very explicit.


Giving error response in Web API I'm creating a Web Api in .NET Framework that is connected to an ASMX service. No own logic, just pass through data.

Everything works as expected. But not sure if it's good. Have to repeat the same try/catch block in all controller actions.

SecurityController - send username and password and retrieve a session Id to use in subsequent calls:

public class SecurityController : ApiController
{
    public SecurityController(ILogger logger)
    {
        Logger = logger;
    }

    ILogger Logger { get; }

    [HttpPost]
    public IHttpActionResult StartSession([FromBody] StartSessionRequestModel requestModel)
    {
        var client = new SecurityFacade(); // from proxy class
        var requestMessage = new StartSessionRequest
        {
            Username = requestModel.Username,
            Password = requestModel.Password
        };

        try
        {
            var startSessionResponse = client.StartSession(requestMessage);

            return Ok(new
            {
                startSessionResponse.SessionId
            });
        }
        catch (SoapException ex)
        {
            return BadRequest(ex.Code.Name);
        }
        catch (Exception ex)
        {
            Logger.Error(ex);
            return InternalServerError(ex);
        }
    }
}

Output when entered wrong user/password

{
    "Message": "InvalidUsernameAndPasswordPair"
}

On the client (Xamarin), using RestSharp.

var request = new RestRequest("security/startsession");
request.AddParameter("Username", Username);
request.AddParameter("Password", Password);
restClient.PostAsync<StartSessionResponse>(request, (response, handle) =>
{
    if (response.IsSuccessful)
    {
        var sessionId = response.Data.SessionId;

        // todo
    }
    else
    {
        var responseMessage = JsonConvert.DeserializeAnonymousType(response.Content, new { Message = "" });

        if (responseMessage.Message == "InvalidUsernameAndPasswordPair")
        {
            // todo: show "invalid user/password" error
        }
        else
        {
            // todo: show other nicely worded error
        }
    }
});

Handling exceptions within the action is a cross-cutting concern that can be extracted and placed into one of the available extensiblity points. This will greatly reduce the repeated  exception handling in all controller actions.

Reference Global Error Handling in ASP.NET Web API 2

  Exception handlers are the solution for customizing all possible responses to unhandled exceptions caught by Web API.

With that the above controller in your example can be refactored to something as simple as

public class SecurityController : ApiController {
    private readonly ISecurityProxy client; // abstraction of proxy class

    public SecurityController(ISecurityProxy client) {
        this.client = client;
    }

    [HttpPost]
    public IHttpActionResult StartSession([FromBody] StartSessionRequestModel requestModel) {
        if (ModelState.IsValid) {
            var requestMessage = new StartSessionRequest {
                Username = requestModel.Username,
                Password = requestModel.Password
            };
            StartSessionResponse result = client.StartSession(requestMessage);
            return Ok(result);
        }
        return BadRequest(ModelState);
    }
}

Why so lean?

The SecurityFacade has been abstracted out as classes should depend on abstractions and not be tightly coupled to concretions as the original example implies that dependency injection is already being used.

The ILogger and exception handling is no longer needed in the controller as that is a cross-cutting concern that can be moved to an ExceptionHandler

public class WebApiExceptionHandler : ExceptionHandler {
    private readonly ILogger logger;

    public WebApiExceptionHandler(ILogger logger)
        : base() {
        this.logger = logger;
    }

    public override void Handle(ExceptionHandlerContext context) {
        var innerException = context.ExceptionContext.Exception;

        // Ignore HTTP errors.
        if (innerException.GetType().IsAssignableFrom(typeof(System.Web.HttpException))) {
            return;
        }

        //Handle SOAP errors as bad requests
        IHttpActionResult result = null;
        if (innerException.GetType().IsAssignableFrom(typeof(SoapException))) {
            var soapException = innerException as SoapException;
            result = new BadRequestResult(soapException.Code.Name, context.Request);
            context.Result = result;
            return;
        }

        //Handle and log general errors
        logger.Error(innerException);
        result = new InternalServerErrorResult(context.Request);
        context.Result = result;
    }

    private class BadRequestResult : IHttpActionResult {
        public BadRequestResult(string message, HttpRequestMessage request) {
            Message = message;
            Request = request;
        }

        public HttpRequestMessage Request { get; private set; }
        public string Message { get; private set; }

        public Task<HttpResponseMessage> ExecuteAsync(CancellationToken cancellationToken) {
            var model = new {
                Message = Message
            };
            var response = Request.CreateResponse(HttpStatusCode.BadRequest, model);
            return Task.FromResult(response);
        }
    }
}

  An exception handler indicates that it has handled an exception by setting the Result property to an action result (for example, an ExceptionResult, InternalServerErrorResult, StatusCodeResult, or a custom result). If the Result property is null, the exception is unhandled and the original exception will be re-thrown.

the handler can now be added as a global error handler for API calls.

The following extension method is added to simplify adding the handler when configuring the Web API at start up.

public static class WebApiConfig {

    public static void Register(HttpConfiguration config) {

        //... code removed for brevity

        config.ReplaceExceptionHandler();
    }

    /// <summary>
    /// Global Exception handling
    /// </summary>
    /// <remarks>
    /// There must be exactly one exception handler.
    /// (There is a default one that may be replaced.)
    /// </remarks>
    public static HttpConfiguration ReplaceExceptionHandler(this HttpConfiguration config) {
        var errorHandler = config.Services.GetExceptionHandler();
        if (!(errorHandler is WebApiExceptionHandler)) {
            var service = config.Services.GetService(typeof(WebApiExceptionHandler));
            config.Services.Replace(typeof(IExceptionHandler), service);
        }
        return config;
    }
}


Set-uid root program that runs a program as the user "restrict" I've written a very short program that is intended to run the program specified in its arguments (argv) as the user named "restrict" (which exists). I want to make sure that there are no subtle bugs in the program, and that the program is as simple as possible.

Is this program doing what I intend?
Can someone use this program to do anything malicious? Assume that anything running as the user "restrict" is not able to do anything malicious, but anything running as the user "root" obviously can.
Can this code be simplified, or (assuming the code is correct) can this code's correctness be made more obvious?

The code (compiled with gcc --std=c11):

#include <unistd.h>
int main (int argc, char **argv) {
    setuid(0); // Switch user to root.
    char *start[] = {
        "/usr/bin/sudo", "-u", "restrict", // Switch user to "restrict".
        "--" // End arguments.
    };
    int start_len = (sizeof start) / (sizeof start[0]);
    char *args[start_len+argc];
    char *env[] = {NULL}; // Delete all environment variables.
    int i;
    for (i = 0; i < start_len; ++i)
        args[i] = start[i];
    // Note that argv is null-terminated, so we can go past the end by one.
    // Conveniently, we also want to discard the first argument, so just loop
    // over the arguments as usual, but with an offset of one.
    for (i = 0; i < argc; ++i)
        args[start_len+i] = argv[i+1];
    execve(args[0], args, env);
}

The final binary will be owned by root, and will have the set-uid bit set, and will be executable by users in a particular group.

The ultimate goal is to allow users in a particular group to run any executable as the user "restrict".

Alternatively, is there a better way to accomplish this goal?

See Set-uid root program that runs a program as the user “restrict” (follow-up) for the next iteration of this code.

Stay away from variable length arrays. First, they are optional in C11  (in C99 they were mandatory). Second, there is no way to detect allocation failure. A malicious user may invoke the program with a command line long enough to cause stack overflow.
Why sudo? You are root already, so you may set the user and group ID directly.


Token-finding function I want to make sure find_value_from_key() is safe and efficient.  My constraints are language C++, use of standard library ok, but cannot use Boost or any new C++11 features.

Can anyone offer any comments/criticisms/feedback on my implementation of the function here?  Any flaws in my test cases would also be interesting to hear.

#include <string>
#include <iostream>

using std::cout;
using std::endl;
using std::string;

bool find_value_from_key(const string& source, const string& key, string& value, const string& EndOfValueMarker) {
   bool ret = false;
   size_t pos_start = source.find(key);
   if(pos_start != string::npos) {
      size_t pos_end = source.find_first_of(EndOfValueMarker, pos_start+key.length());
      if(pos_end == string::npos && source.length() > pos_start+key.length())
         pos_end = source.length();

      if(pos_end != string::npos) {
         value = source.substr(pos_start+key.length(), pos_end-pos_start-key.length());
         ret = true;
      }
   }
   return ret;
}

int main(int argc, char* argv[])
{
   //test strings to exercise find_value_from_key function
   const char* test_strings[] = {
      "key1 = value1",
      "   key1 = value1   ",
      "key1 = value1,key2 = value2 ,key3 = value3",
      "_key1 = value1 ,key2 = value2 ,Gkey3 = value3,",
      "key1 = value1,key2 =  value2 ,key3 = value3 x",
      "key1 = value1,key2 = value2key3 = value3\n",
      "key1 = value1_,key2 = value2 ,key3 = value3                    ",
      "key1 = value1$,key2 = value2 ,key3 =   value3",
      "key1 = value1\t,key2 = value2  ,key3 = value3\t" };

   const char* keys[] = {"key1 = ", "key2 = ", "key3 = "};

   int elements = sizeof(test_strings) / sizeof(test_strings[0]);

   for(int i = 0; i < elements; ++i) {
     cout << "processing: " << test_strings[i] << endl;
     string value;
     if(find_value_from_key(test_strings[0], keys[0], value, ", \t\n"))
        cout << value << endl;
   }

   return 0;
}

find_value_from_key
The core method find_value_from_key looks functional for the limited specification you have given. I am not a great fan of the specification though, I think the function should try to be a bit smarter about identifying values in the input strings. The two items I find most concerning are:

It will successfully find key1 =  and value1 in "ThisIsSubkey1 = value1"
it does not successfully find value1 in "key1 =  value1" (note two spaces after =)

These are both items related to the context-naive usage of the find(...) method.
Still, as far as the specification goes, your code looks like it does the job.
There is one improvement that can/should be done to make it more readable. You reuse the value pos_start+key.length() four times. Performance-wise this may be sorted out by a compiler, but it does not help the readability at all...
I would prefer the function written as:
bool find_value_from_key(const string& source, const string& key, string& value, const string& endmarkers) {
   bool ret = false;
   size_t key_start = source.find(key);
   size_t val_start = key_start + key.length();
   if(key_start != string::npos) {
      size_t val_end = source.find_first_of(endmarkers, val_start);
      if(val_end == string::npos && source.length() > val_start)
         val_end = source.length();

      if(val_end != string::npos) {
         value = source.substr(val_start, val_end - val_start);
         ret = true;
      }
   }
   return ret;
}

Note how there is no pos_start variable... it was being reused for different purposes in different places. It was confusing. Renaming the variables to self-document makes the code easier to understand. The compiler will sort out any optimizations in the same ways as before.
There is one additional item in that method... why the CamelCase parameter EndOfValueMarker? I have renamed it endmarkers.
main
The main method has a couple of issues I think must just be an artifact from testing.
You set up the loop to check each input string, but, you do not use i (the loop variable) as an index to the actual data you check... you have the code:

   for(int i = 0; i < elements; ++i) {
     cout << "processing: " << test_strings[i] << endl;
     string value;
     if(find_value_from_key(test_strings[0], keys[0], value, ", \t\n"))
        cout << value << endl;
   }

This code prints each test_strings[i] just fine, but the input to the function is always index 0... so you only actually test the first input.
The second item is that you only test for key1. The other keys are not tested.
The loop code should look like:
for(int i = 0; i < elements; ++i) {
    cout << "processing: " << test_strings[i] << endl;
    string value;
    for (int k = 0; k < 3; k++) {
        if(find_value_from_key(test_strings[i], keys[k], value, ", \t\n"))
            cout << keys[k] << " -> " << value << endl;
    }
}

I know, it has the magic number 3 for the k loop, but at least it checks each key....
Ideone
I have put my recommendations in to this ideone... take it for a spin


Implement an algorithm to determine if a string has all unique characters I'm working through Cracking the Coding Interview. The first challenge is to implement an algorithm to determine if a string has all unique characters.

I've gone through several iterations, but here's my "final" solution. I've made several assumptions:

We are considering case-insensitive strings.
We are only considering strings that contain alphabetic characters.
We are only considering ASCII strings.

Here's my algorithm and a quick driver. The logic is simple, but I wanted to explain what's going on for sake of the reader.

/*
Implement an algorithm to determine if a string has all unique characters.
What if you can not use additional data structures?
*/

#include <iostream>
#include <string>

bool is_unique(const std::string& s) {
    // seen is a 32-bit bitset. We have seen no letters yet.
    int seen = 0;
    // for every character in the string
    for(auto c: s) {
        // convert the string to lowercase (assumption 1)
        char lower_c = std::tolower(c);
        // normalize the character (assumptions 2 and 3)
        int offset = lower_c - 'a';
        // shift 1 at most 26 to the left. Fits within 32-bit bitset.
        int shifted = 1 << offset;

        if(!(seen & shifted)) {
            // if we haven't seen it, add it to seen
            seen |= shifted;
        } else {
            // otherwise return false -- we have seen it
            return false;
        }
    }
    // if we haven't returned false after the loop, it must be unique
    return true;
}

int main(int argc, char** argv) {
    std::string s{argv[1]};
    std::cout << "Is " << s << " unique? " << is_unique(s) << '\n';
}

Does anyone have recommendations?

If I'd asked an interview question, and got this as an answer (at least as the first answer) I think my immediate reaction would be to consider it excessively clever.

To make much sense, along with the conditions you gave, I'd have had to specify that minimizing memory usage was absolutely crucial.

Under ordinary circumstances, I'd expect an answer something like this:

bool all_unique(std::string const &input) {
    std::map<char> temp(input.begin(), input.end());
    return temp.size == input.size();
}

...or something like this:

bool all_unique(std::string input) {
    std::sort(input.begin(), input.end());
    auto p = std::unique(input.begin(), input.end());
    return p == input.end();
}

There are, of course, variations on these themes (e.g., perfectly reasonable and acceptable to use unordered_set instead of set).

Either of these will work with a much wider range of inputs than you've specified. They will use more storage, but the amount they use is still too small to matter under any normal circumstances.

If somebody were going to craft a special-purpose set of their own, I'd normally expect them to start from std::bitset or std::vector<bool>. I guess if they insisted on starting with a native type to represent their bits, I wouldn't object too terribly much, but I'd be a little disappointed if they chose int for that type.

If you want a bag of bits, you should usually use an unsigned type.
If you want at least 32 bits, you should use a type that guarantees at least 32 bits (int only guarantees at least 16 bits).

As such, obvious choices would be unsigned long and uint32_t.


Changing color via JRadioButtons and JButtons I've got my full program working as expected. My full program looks like this:

I'm posting a part of my code which changes the color of the circle. The user can change the color of the circle via two ways:

By selecting a JRadioButtonMenuItem from the JMenu.
By pressing one of the colors(JButtons) in the JPanel below the main panel.

Here is the relevant piece of code which works as expected:

    colorbuttons = new JButton[]{new JButton(),
                                 new JButton(),
                                 new JButton(),
                                 new JButton(),
                                 new JButton(),
                                 new JButton(),}; //Different color buttons

    colorbuttons[0].setBackground(Color.WHITE);
    colorbuttons[1].setBackground(Color.GREEN);
    colorbuttons[2].setBackground(Color.RED);
    colorbuttons[3].setBackground(Color.YELLOW);
    colorbuttons[4].setBackground(Color.BLUE);
    colorbuttons[5].setBackground(Color.BLACK); //Setting each color

    for(int i=0; i<colorbuttons.length; i++)
    {
        colorbuttons[i].addActionListener(this);
        colorPan.add(colorbuttons[i]); //Adding into a JPanel
    }

    bar = new JMenuBar();
    menu1 = new JMenu("Edit circle");
    sub1 = new JMenu("Colour");
    colors = new JRadioButtonMenuItem[]{new JRadioButtonMenuItem("White"),
                                        new JRadioButtonMenuItem("Green"),
                                        new JRadioButtonMenuItem("Red"),
                                        new JRadioButtonMenuItem("Yellow"),
                                        new JRadioButtonMenuItem("Blue"),
                                        new JRadioButtonMenuItem("Black",true)};

    ButtonGroup group = new ButtonGroup();
    for(int i=0; i<colors.length; i++)
    {
        group.add(colors[i]);
        sub1.add(colors[i]);
    }
    menu1.add(sub1);
    bar.add(menu1);

    menu1.setMnemonic(KeyEvent.VK_E);
    sub1.setMnemonic(KeyEvent.VK_C);

    ActionListener radiolisten = new ActionListener(){
        public void actionPerformed(ActionEvent e)
        {
            int getIndex = 0;
            JRadioButtonMenuItem temp = (JRadioButtonMenuItem) e.getSource();

            for(int i=0; i<colors.length; i++)
            {
                if(temp == colors[i])
                    break;
                getIndex++;
            }

            color = colorbuttons[getIndex].getBackground();
            repaint();
        }
    };

    for(int i=0; i<colors.length; i++)
        colors[i].addActionListener(radiolisten);

and the actionPerformed method of the class( which the JButtons are added to):

public void actionPerformed(ActionEvent e)
{
    JButton temp = (JButton)e.getSource();
    color = temp.getBackground();
    int getIndex=0;

    for(int i=0; i<colorbuttons.length; i++)
    {
        if(temp == colorbuttons[i])
            break;
        getIndex++;
    }

    colors[getIndex].setSelected(true);
    repaint();
}

color is a global variable that denotes the color of the circle.

I would like to shorten the above code snippet, especially both the actionPerformed methods. How can I do it?

For my first code review, I hope you find this useful!

Firstly, I like the fact you've used arrays for your buttons, this is good. The name of the array is also nice and descriptive.

colorbuttons = new JButton[]{new JButton(),

Looking at some of your other variable names, I'm not quite as happy.

bar = new JMenuBar();
menu1 = new JMenu("Edit circle");
sub1 = new JMenu("Colour");
colors = new JRadioButtonMenuItem[]{new JRadioButtonMenuItem("White"),
color

Firstly colors I'd expect a list/array of colours - but no, it's an array of radio button menu items. I think color is your worst offender here. By reading the variable name, we don't know what object it's the color of.

Now, you have buttons related to a subset of colours. So I would start with an enum that will keep a track of the Color and the name

private enum EColorSet {
    eWhite(Color.WHITE, "White"),
    eGreen(Color.GREEN, "Green"),
    eRed(Color.RED, "Red"),
    eYellow(Color.YELLOW, "Yellow"),
    eBlue(Color.BLUE, "Blue"),
    eBlack(Color.BLACK, "Black");

    Color c;
    String label;

    private EColorSet(Color c, String label)
    {
        this.c = c;
        this.label = label;
    }
}

Now that you have a set of colours to iterate against, you can set up your arrays as follows

colorbuttons = new JButton[EColorSet.values().length]
colors = new JRadioButtonMenuItem[EColorSet.values().length]

and initialise them whilst looping through the colours

for (EColorSet currentColor : EColorSet.values()) {
    colourbuttons[currentColor.ordinal()] = new JButton();
    colourbuttons[currentColor.ordinal()].setBackground(currentColor.c);
    //do the same for the colors menu items -note you use the label...
}

see the fact in colourbuttons[currentColor.ordinal()] .setBackground(currentColor.c); i'm using the colour stored in my enum

We need to map the button (or menu item click) to an index in the array, which also represents a EColorSet instance. Unfortunately, we still need the index of the button in the array, but

public void actionPerformed(ActionEvent e) {
    JButton temp = (JButton)e.getSource();

    int index = 0;
    for(JButton b : colourbuttons) {
        if(temp.equals(b)) {
            break;
        }
        index ++;
    }
    EColorSet currentColor = EColorSet.values()[index];
    setColor(currentColor);
    setMenuFromColor(currentColor);
}

Now to finish this off, when a button is clicked, we need to find the index, but we can do this as we know the color now.

private void setMenuFromColor(EColorSet colourClicked){
    colors[colorClicked.ordinal].setSelected(true);
    //set the menu to selected
}

private void setColor(EColorSet colourClicked){
    //set the colour of the circle
}