Insertion Sort in C++ I have created an Insertion Sort code that sorts numbers (obviously).

#include <iostream>
#include <cstdlib>

void insertion_sort(int arr[], int length);
void print_array(int array[],int size);

int main()
{
    int array[] = {4,6,3,7,5,9,2,8,1,10};
    insertion_sort(array,10);
    print_array(array,10);
    return 0;
}

void insertion_sort(int arr[], int length)
{
    int i,j,tmp;
    for (i = 1; i < length; i++) {
        j = i;
        while (j > 0 && arr[j - 1] > arr[j]) {
            tmp = arr[j];
            arr[j] = arr[j - 1];
            arr[j - 1] = tmp;
            j--;
        }
    }
}

void print_array(int array[], int size)
{
    int j;
    for (j = 0; j < size; j++)
    {
        std::cout << array[j] << std::endl;
    }
}

Is there a way to make it shorter or better?

This is not C++, yet. Just using std::cout does not C++ make. But do not fret, we'll get there.

Safe interface

Whenever designing an API (or interface), you should strive to make it as safe as possible.

Your interface here is not safe: having two un-linked variables to correctly pass is asking for troubles; it is just too easy to mix the variables up and pass a size that does not correspond to the array to be sorted (or printed).

Obviously, in your toy example it does not show up, however as soon as you manipulate multiple arrays in the same function you'll be very likely to make this mistake. And even with a single array, passing the length before re-allocating (for example) is also likely.

So, you need to aim for an interface that conveys both pieces of information in a single argument. An obvious choice is std::vector, another would be some instance of array_view or a random_access_range but those are not Standard so let's not bother for now.

Thus, the interface becomes:

void insertion_sort(std::vector<int>& array);
void print(std::vector<int> const& array);

Do note the consistency in the naming; both times the argument is named array instead of being once arr and once array. Consistency may be the hobgobelin of little minds, but it does help readers.

Algorithmic

An insertion sort is generally described simply as: read each element in turn, and insert it in the right position among the previous read (and sorted) elements.

The cost of the algorithm is thus:

for each of the N elements
the cost of inserting it in the i-1 (where i is the index of the element being read) first elements

The outer loop we cannot do anything about, but what of the insertion?

In your code, the insertion is done by:

    while (j > 0 && arr[j - 1] > arr[j]) {
        tmp = arr[j];
        arr[j] = arr[j - 1];
        arr[j - 1] = tmp;
        j--;
    }

That is, the value is bubbling up toward the front until it finds its spot.

This is suboptimal in terms of comparisons: the optimal algorithm to find the position to insert is using a binary search rather than a linear search, for O(log N) complexity rather than O(N) one.

In terms of number of moves, well, you may have to move all elements, so it could only be improved by a constant factor anyway.

Standing on the shoulders of giants, we can improve the code by separating the search and the move, and use pre-existing algorithms to do so:

std::upper_bound yields a pointer to the first element in the range that is greater than the value we search with
std::rotate rotates the elements of the range (and middle point) passed to it, such that the range delimited by the last two elements come to precede the range delimited by the first two

We can use them keeping to indices, although it is slightly verbose:

void insertion_sort(std::vector<int>& array) {
    typedef std::vector<int>::iterator It;

    for (size_t i = 1, length = array.size(); i < length; ++i) {
        // 1. Search
        It const end_sorted = array.begin() + i;
        It const insertion_point =
            std::upper_bound(array.begin(), end_sorted, array[i]);

        // 2. Insert
        std::rotate(insertion_point, end_sorted, end_sorted + 1);
    }
}

Thus I advise jumping the gun and just switch to iterators since that it what the interfaces of the algorithms we use consume:

void insertion_sort(std::vector<int>& array) {
    typedef std::vector<int>::iterator It;

    for (It it = array.begin(), end = array.end(); it != end; ++it) {
        // 1. Search
        It const insertion_point =
            std::upper_bound(array.begin(), it, *it);

        // 2. Insert
        std::rotate(insertion_point, it, it + 1);
    }
}

Which in modern C++ would use auto rather than explicitly naming the iterator type:

void insertion_sort(std::vector<int>& array) {
    for (auto it = array.begin(), end = array.end(); it != end; ++it) {
        // 1. Search
        auto const insertion_point =
            std::upper_bound(array.begin(), it, *it);

        // 2. Insert
        std::rotate(insertion_point, it, it + 1);
    }
}

Note that abbreviating further is not a good idea; while one-liners may give the writer a fuzzy warm feeling, they just end up being head scratchers for maintainers:

// Puzzle time!
void insertion_sort(std::vector<int>& array) {
    for (auto it = array.begin(), end = array.end(); it != end; ++it) {
        std::rotate(std::upper_bound(array.begin(), it, *it), it, it + 1);
    }
}

What does endl do?

Finally, a trick question: what do think std::endl is and does?

std::endl is a so called stream manipulator, it is a function whose signature is:

template< class CharT, class Traits >
std::basic_ostream<CharT, Traits>& endl( std::basic_ostream<CharT, Traits>& os );

So, when used on a stream, it does an operation on said stream and returns it (to allow further chaining).

Specifically, ostream << std::endl is equivalent to:

ostream << '\n';
ostream.flush();

Where flush makes sure that the data is not buffered in the process but fully sent to the Operating System (it does not guarantee that the data is on disk, sent over the network, or anything else).

There is very little reason to call flush in general:

in the absence of guarantee that the data arrived on disk (or wherever), it serves not practical purpose
the buffer was introduced for performance reasons, bypassing it is asking to go slow

So, not only should you generally avoid flush, std::endl is a mouthful. Just write '\n' to get an end-of-line character and be done with it.

Thus, we rewrite print:

void print(std::vector<int> const& array) {
    for (auto it = array.begin(), end = array.end(); it != end; ++i) {
        std::cout << *it << '\n';
    }
}

Or, using the range-for notation of modern C++:

void print(std::vector<int> const& array) {
    for (int e: array) { std::cout << e << '\n'; }
}

Who said C++ could not be nifty?


Get character occurence count along with character in a string I want to find character sequence count in a given string.

Sample Input: aaaabbbbaaacccbbb

Output: a4b4a3c3b3

My below function is working great and giving me the same result. but can this be optimized?

function getCharCount(str) {
    var result = str.charAt(0);
    var count = 1;
    if (str.length == 1) {
        result += count;
        return result;
    } else {
        for(var i=1;i<str.length;i++) {
            if(str.charAt(i) != str.charAt(i-1)) {
                result += count + str.charAt(i);
                count = 1;
            } else {
                count++;
            }
            if (i == str.length - 1) {
                result += count;
            }
        }
        return result;
    }
}

I don't see any problem in your code except using of equality and inequality operators. Use strict equality(===) & inequality(!==) operators.

I'll suggest to use RegEx.

.replace(/(.)\1*/g, function(m, $1) {
    return $1 + m.length;
})

The RegEx (.)\1* will match a single non-line-break character and check if that is followed by the same character any number of times. m here is the complete match and $1 is the first chaptured group value i.e. the character.

var res = 'aaaabbbbaaacccbbb'
    .replace(/(.)\1*/g, function(m, $1) {
        return $1 + m.length;
    });
console.log(res);


Displaying an order status form Below is code i used to call Api and display the "Status" results in web page.

my senior told me that code is completely mess and need to improve so that it should be easy to understand for others. but i am not getting how i can refactor below code ? is mixing php and html code is bad ? i really need to mix those html and php to get the results that i needed .please help me for this....

<button type= "button" class="call">Show Status</button>

               ORDERID
               Status

<?php
$tabindex = 1;

if (!empty($orderrecords))
    {
    foreach($orderrecords as $k => $v)
        { ?>

        <?php
        $hide = '';
        $data['username'] = 'admin';
        $data['password'] = 'admin123';

        $url = 'https://plapi.ecomexpress.in/track_me/api/mawbd/?awb=awbnumber&order=' . $orderrecords[$k]["order_id"] . '&username=admin&password=admin123';
        $ch = curl_init();
        // some curl code
        $res = explode("\n", $output);
        if (!isset($res[13]))
            {
            $res[13] = null;
            }

        $status = $res[13];
?>

<tr class="table-row" id="table-row-<?php echo $orderrecords[$k]["id"]; ?>" tabindex="<?php echo $tabindex; ?>">

<input onclick="assignorderids('<?php echo $orderrecords[$k]["order_id"]; ?>')" type="checkbox" name="assigneeid" id="assigneeid-<?php echo $orderrecords[$k]["order_id"]; ?>" value="<?php echo $orderrecords[$k]["order_id"]; ?>">

<?php echo $orderrecords[$k]["order_id"]; ?>
<?php echo $status; ?>

 <?php
 $tabindex++;
 }
    } ?>

I can totally understand the frustration of your colleague that will need to maintain this code.

But would have been more constructive to tell that this code is considered a "mess" mainly because:

Logic of fetching order status is mixed in the middle of table rendering;
"Dirty" formatting: inconsistent indentation and trailing spaces;

The exact same thing could be better expressed like that:

<?php
function getOrderTrackingStatus($orderId) {
    $url = 'https://plapi.ecomexpress.in/track_me/api/mawbd/';
    $url.= "?awb=awbnumber&order=$orderId&username=admin&password=admin123";

    $ch = curl_init();
    // some curl code
    $res = explode("\n", $output);

    $status = isset($res[13]) ? $res[13] : null;
    return $status;
}
?>

    <button type="button" class="call">Show Status</button>

            ORDERID
            Status

    <?php $tabindex = 1; ?>
    <?php foreach($orderrecords as $record): ?>

        <tr class="table-row"
            id="table-row-<?php echo $record['id']; ?>"
            tabindex="<?php echo $tabindex++; ?>">

                <input type="checkbox"
                       name="assigneeid"
                       id="assigneeid-<?php echo $record['order_id']; ?>"
                       value="<?php echo $record['order_id']; ?>"
                       onclick="assignorderids('<?php echo $record['order_id']; ?>')">

            <?php echo $orderId; ?>
            <?php echo getOrderTrackingStatus($orderId); ?>

    <?php endforeach; ?>

Ideally this getOrderTrackingStatus function would be moved somewhere else, instead of living inside the same file that handles data visualisation.

Hope it helps you, and depending on your project you should consider using a templating engine as a way to force yourself to move logic code away from the views.


Circular Bounded Queue using C# As an exercise, I have implemented a circular bounded queue using arrays as the base data structure. I'd appreciate it if someone can review it for me.

    class BoundedQueue<T> {

    T[] que;
    int head;       // remove from head
    int tail;       // insert at tail

    public BoundedQueue(int quesize)
    {
        head = tail = -1;
        que = new T[quesize];
    }

    public void enqueue(T elem)  // if next index to tail == head => Q is FULL
    {
        int newIndex = nextIndex(tail);
        if ( newIndex == head)
            throw new BoundedQueueException(Full);

        tail = newIndex;
        que[newIndex] = elem;
        if (head == -1)
            head = 0;
    }

    public T dequeue()  // After removing from head, if that was the only element in Q
    // Mark Q to be empty by setting head and tail to -1
    {
        if (head == -1)
            throw new BoundedQueueException(Empty);

        T elem = que[head];
        que[head] = default(T);

        if (head == tail)
        {
            head = tail = -1;
        }
        else
        {
            head = nextIndex(head);
        }

        return elem;
    }

    private int nextIndex(int index)
    {
        return (index + 1) % que.Length;
    }

}

Exception handling should be added
to the constructor, handling a negative
quesize. Or take a look at Code
Contracts.
Instead of initializing head and
tail to -1, you could use
nullable ints, and adjust your
logic so it doesn't rely on the
magic number -1.
Implement some missing features.
(might have been left out
intentionally): implement ICollection and IEnumerable, isFull().
A minor point would be naming conventions. In C# method names normally start with a capital letter.
Be aware that this is not a thread safe class.
Add some comments, this code isn't that self-documenting. Or, where possible, make it self-documenting, e.g. if ( head == tail ) could be if ( Count == 0 ).


Javascript - Find a deepest node in a binary tree Problem Statement

Given the root of a binary tree, return a deepest node.
For example, in the following tree, return d.
    a
   / \
  b   c
 /
d

My Implementation

module.exports = {
    deepestNode: function deepestNode(node, level) {
        if (!level)
            level = 0;

        node.depth = level;

        if (!node.left && !node.right)
            return node;

        let deepestLeft, deepestRight = node;

        if (node.left)
            deepestLeft = deepestNode(node.left, level + 1);

        if (node.right)
            deepestRight = deepestNode(node.right, level + 1);

        if (deepestLeft.depth > deepestRight.depth)
            return deepestLeft;
        else
            return deepestRight;
    },
    TreeNode: (val) => {
        return {
            val: val,
            left: null,
            right: null
        }
    }
};

Test Class

const expect = require('chai').expect;
const deepestNode = require('../../src/google/deepestNode');

let a = deepestNode.TreeNode('a');
let b = deepestNode.TreeNode('b');
let c = deepestNode.TreeNode('c');
let d = deepestNode.TreeNode('d');

a.left = b;
b.left = d;
a.right = c;

expect(deepestNode.deepestNode(a).val).eq('d');

a = deepestNode.TreeNode('a');
expect(deepestNode.deepestNode(a).val).eq('a');

I am a Javascript newbie so any comments regarding either the algorithm itself or the use of Javascript is welcome.

Here are my thoughts:

Don't mutate function parameters unless there is good reason to do so.

node.depth = level;

The above statement basically breaks the contract of the function. The function claims to locate the deepestNode, but in fact it does findDeepestNodeAndSetDeepestPropOnAllNodes (silly, but you get the idea). This could lead to confusing, subtle bugs for the client of your module and is a serious issue.

If you want to return the level in addition to the node itself, that's easily done by returning a {node, level} object pair which doesn't corrupt the reference to node. However, since the test suite disregards the level, I'd omit it since the function name makes no mention of this and it's not typically valuable information.
Always use braces for blocks of code:

if (deepestLeft.depth > deepestRight.depth)
    return deepestLeft;
else
    return deepestRight;

is clearer and less error-prone as:

if (deepestLeft.depth > deepestRight.depth) {
  return deepestLeft;
}

return deepestRight;

Use default parameters:

function deepestNode(node, level) {
    if (!level)
        level = 0;
...

can be

function deepestNode(node, level=0) {
...

Avoid excessive conditionals; it's possible to simplify 6 branches to 2, reducing the cognitive load required to understand the code. Re-write negative conditionals to be positive when possible.

For example,

if (!node.left && !node.right)
    return node;

can be eliminated. As a rule of thumb, I try to operate only on the current node when writing recursive functions unless I'm forced to do otherwise.
Avoid excessive references and intermediate variables. Consider:

let deepestLeft, deepestRight = node;
/* ... various conditionals that may or may not change where
   `deepestLeft`, `deepestRight` and `node` points to ... */

// later on, unclear state

As with conditionals, when you begin relying on aliases for objects, it becomes difficult to keep track of what's pointing where. Code like this should only be written if there's no way around it, which isn't the case here.

In fact, this line causes crashes when node.right is the only child of a node. You may have meant:

let deepestLeft = deepestRight = node;

Adding more thorough tests to your suite would help detect bugs like this.

As an aside, prefer const instead of let unless you have to reassign the variable.
Don't crash on undefined/null parameters if it's trivial to prevent.

node.depth = level; // boom if `node` is undefined

Here's a re-write. I wrote this as browser code, but you can dump it into module.exports:

const deepestNode = (node, deepest={level: -1}, level=0) => {
  if (node) {
    if (level > deepest.level) {
      deepest.node = node;
      deepest.level = level;
    }

    deepestNode(node.left, deepest, level + 1);
    deepestNode(node.right, deepest, level + 1);
  }

  return deepest.node;
};

class TreeNode {
  constructor(val, left=null, right=null) {
    this.val = val;
    this.left = left;
    this.right = right;
  }
}

const root = new TreeNode(1,
  new TreeNode(2,
    new TreeNode(4)
  ),
  new TreeNode(3)
);

console.log(deepestNode(root));

This function can also be written iteratively in a clean way, avoiding call stack overflow errors and function call overhead:

const deepestNode = node => {
  let deepest = {level: -1};
  const stack = [[node, 0]];

  while (stack.length) {
    const [curr, level] = stack.pop();

    if (curr) {
      if (level > deepest.level) {
        deepest = {node: curr, level: level};
      }

      stack.push([curr.left, level + 1], [curr.right, level + 1]);
    }
  }

  return deepest.node;
};


Registration and Login: Best way to separate responsibilities Based on lots of read books, sometimes I get quite confused when things get bigger than the simple examples. Martin Fowler's clean code is good but not enough.

I want some feedback on how to improve my current coding workflow, especially regarding to methods doing one thing or what is exactly one thing, does it depends on context. I'll show you an example:

Let's say I have these simple features:

SIGNOUT Whenever a visitor signs out:

If the visitor is already logged in, clear user session and cookie data.

LOGIN Whenever a visitor logs in:

Signout
If there's session data assign current session data (like current cart to the logged in user)

REGISTER When a visitor submits the register form:

Signout
If the email already exist try to login with the password from registration form.
If login failed return email in use error.
If email does not exist create user, create profile info and send welcome email
Login

Controller would have something like:

[HttpPost, ValidateModelState, ExportModelStateToTempData, ValidateAntiForgeryToken]
public RedirectResult Register(AccountRegisterBindingModel binding, string returnUrl) {
    var status = customerService.Register(binding);
    if (!status.Succeded) {
        ModelState.AddModelErrors(status.Errors);
        return Redirect(Url.Action("Register", "Account", new { returnUrl }));
    }
    return Redirect(returnUrl ?? DefaultUrl);
}

As you can see, the controller is really thin, but I'm worried about the customerService.Register().

First: name suggests to be: SingoutCreateUserSendWelcomeEmailLoginAndMergeSessionData()
Second: Does this method do too many things? It looks like it.

The Register method have something like:

public SignInStatus Register(UserRegistration data) {
    authService.SignOut();
    var result = userService.CreateUser(data); // User and profile....
    if (result.Succedded) {
        communicationService.SendWelcomeEmail(data);
        customerService.MergeSessionCustomerWithUserCustomer();
    }
    return authService.Login(data)
}

Why I'm asking for a code review:

While this may seem ok, the register method does too many things.
I know I can put the logic pieces on the controller, but I have multiple endpoints for registration and I don't want to have duplicated code on controllers. Also everybody goes against it.
This method is used on multiple websites (not just one) so change the code in the future can impact a lot. that smell like something is not quite good.
If one consumer of this method changes I might have to use that in all or create a new one.
This codebase does changes a lot, constant development of features, etc.

I hope it's clear enough. In reality, the code is more complex and nested, even if I try to keep it clean I sometimes don't know where to put the code pieces to keep it as clean as possible. While this is about a simple feature, I would love to get suggestions that will work for other parts of the software.

As I read the code, it seems that what the Register method does is control the business flow as defined in the specifications.

Since all it does (at least as it is written here) is delegate the actual actions to other methods, I don't think 'it does too much' - its single responsibility is to control the flow of the registration process.

To answer your concerns about future maintenance, I see two possible kinds of new features - features that should affect all the service consumers, and features that affect only a single (or part) of the consumers, but not all.

In the first case - there is no problem - do your change in the code, and it will affect all consumers (if you move the code to the controllers that won't be true, since you'd need to change all the controllers...)

In the second case, it should be easy enough to create a new register method for that use case. Since all it does is delegate, the new method itself will be just as simple as the old one, with maybe a delegation to a new method, something like this:

public SignInStatus RegisterAsAdmin(UserRegistration data) {
    authService.SignOut();
    var result = userService.CreateUser(data); // User and profile....
    if (result.Succeeded) {
        result = userService.MakeAdmin();
    }
    if (result.Succeeded) {
        communicationService.SendWelcomeEmail(data);
        customerService.MergeSessionCustomerWithUserCustomer();
    }
    return authService.Login(data);
}

Then connect the relevant consumers to the new service.


Foreach statement for two OfType How would I write a foreach statement that includes both of these statement together

//clear textboxes
foreach (Control c in panel1.Controls.OfType<TextBox>())
        {
            if (!string.IsNullOrEmpty(c.Text))
            {
                c.Text = "";
            }
        }
        //clear price label text
        foreach (Control c in panel1.Controls.OfType<Label>())
        {
            if ((string)c.Tag == "Clearable")
            {
                c.Text = "";
            }
        }

You could write it in 1 loop but you'll still need separation of logic for each type of Control:

foreach(Control c in panel1.Controls)
{
    if(c is TextBox)
    {
        var tb = c as TextBox;
        if(!String.IsNullOrEmpty(tb.Text))
            tb.Text = "";
    }

    if(c is Label)
    {
        var l = c as Label;
        if(l.Tag != null && l.Tag.ToString() == "Clearable")
            l.Text = "";
    }
}


Implementation of KNN in R I have implemented the K-Nearest Neighbor algorithm with Euclidean distance in R. It works fine but takes tremendously huge time than the library function (get.knn). Please point out the possibility of improvement.

knn<-function(list,k){
  n=nrow(list)
  if (n<=k) stop("k can not be more than n-1")
  neigh<- matrix(0,nrow=n,ncol=k)
  for(i in 1:n){
    dist<-matrix(0,ncol=2,nrow=n)
    for (j in 1:n){
      dist[j,1]<-j
      dist[j,2]<-sum((list[i,]-list[j,])^2)
      #dist[j,2]<-dtw(list[i,],list[j,])$distance
    }
    sorted<-dist[order(dist[,2]),]
    neigh[i,]<-sorted[2:(k+1),1]
  }
  return(neigh)
}

You can improve your code by using vectorization to speed up the computation of Euclidean distances in the inner loop. The code would be:

knn <- function(mat, k){
   n <- nrow(mat)
   if (n <= k) stop("k can not be more than n-1")
   neigh <- matrix(0, nrow = n, ncol = k)
   for(i in 1:n) {
      euc.dist <- colSums((mat[i, ] - t(mat)) ^ 2)
      neigh[i, ] <- order(euc.dist)[2:(k + 1)]
   }
   return(neigh)
}

Notice that I also made a few changes:

list is a function in R so calling your object list is a pretty bad idea. Also, in the R language, a "list" refers to a very specific data structure, while your code seems to be using a matrix. So calling that input mat seemed more appropriate.
Similarly, there is a dist function in R so it is a bad idea to name your variable this way. I chose euc.dist since you were computing an euclidean distance.
order already returns an index, so the whole idea of column-biding your indices and distances was not necessary: you just need a vector of distances.

The code above might still be a bit too slow because of the computations inside the for loop. If the dimensions of your matrix are not astronomical, you could compute the matrix of distances at first using a compiled function. I once tested about a dozen of packages and found that fields::rdist (written in Fortran) was the fastest. If you do not wish to install it, you can use the base::dist function. The code would look like this:

knn <- function(mat, k){
   n <- nrow(mat)
   if (n <= k) stop("k can not be more than n-1")
   neigh <- matrix(0, nrow = n, ncol = k)
   library(fields)
   dist.mat <- rdist(mat, mat)
   for(i in 1:n) {
      euc.dist <- dist.mat[i, ]
      neigh[i, ] <- order(euc.dist)[2:(k + 1)]
   }
   return(neigh)
}

If my first suggestion is still too slow and your matrix is so large that my second solution cannot be used, you might want to use a mix between the two approaches, where you loop on chunks of rows, i.e. rely on rdist(chunk_of_rows_from_mat, mat). But I'll leave that to you as an exercise :-)

Finally, I will point out that if you are interested, you could search CRAN or the internet for a package that does exactly what you are after. KNN is a  very common tool and there must be packages (compiled from C) that already do it much faster than this code will do. But not much to be learnt there...


Improving isPrime method I've been doing some programming challenges and problems lately and it seems like many involve primes. I concluded that I would benefit from optimizing how I'm checking for them. Currently I'm using the following method:

private static boolean isPrime(int num) {
    if (num < 1) {
        throw new IllegalArgumentException(
            "Argument must be a positive integer." +
            "\n Argument was: " + num + "."
        );
    }
    if (num <= 3){ return num > 1; }
    if ((num & 1) == 0) { return false; }

    for (int i = 5; i * i <= num; i += 2) {
        if (num % i == 0) {
            return false;
        }
    }
    return true;
}

Sometimes altering longs.

Excluding bitwise hacks that would likely come at the cost of readability is this as good as it gets or is there some enhancement that would raise performance?

When misused, private access can frequently result in reduced opportinutues for code reuse, such as in this case. Self-contained static methods in particular should almost never be private, but rather should be moved into a separate utilities class and marked public.

At the same time, you should add javadoc to the method, to further facilitate code reuse:

/**
 * Tests whether a number is prime or not.
 * @param num The number to test.
 * @returns {@code true} if {@code num} is prime, {@code false} otherwise.
 * @throws IllegalArgumentException if {@code num} is zero or negative.
 */

As for readability, I think that (num & 1) == 0 is perfectly clear, however the previous statement, if (num <= 3) return num > 1;, is a little bit unclear. You should replace it with a simple check for one, if (num == 1) return false; and another if (num <= 3) return true; after it.

Note that if you start the loop at 5, you missclassify 9, 15, and 21, which you can see in this ideone sketch: https://ideone.com/Jnn6QU. They are missed because they are less than 25 but not divisible by two. You should start the loop at 3 in order to catch those numbers.

You may wish to consider using BigInteger.isProbablePrime instead of this method, however, as it is much faster for large values. While a true return from that method does not guarantee that the number is prime, you can tune its certainty to your needs.


if statements or eval? I'm wondering about the following code:

def ValidateGoto(placeToGo):
    conditions = {}
    conditions["not placeToGo"] = "Enter a positive integer in the text box"
    conditions[
              "int(placeToGo) <= 0"] = "I need a positive integer in the text box please"
    conditions[
           "not DataStore.DBFILENAME"] = "There is no data available in the table"
    for condition, errormessage in conditions.iteritems():
        try:
            if eval(condition):
                print errormessage
                return False
        except ValueError:
                return False
    return True

Should the eval statement be avoided?
Is this approach justified to reduce the number of if statements?

You have the right basic idea - pass a list of conditions - but the implementation is wrong.

Yes, you should avoid eval; it is a dangerous source of potential error (and performs poorly as a non-bonus).  You might put anything in there.  You might not even notice a typo which caused nasty side effects, since Python has a very tolerant definition of truth and will be happy as long as the expression evaluates to something.  Instead, why not pass a list of lambda functions? Then you just call each function in turn.  Much safer.

For example, you could build a list of tuples (condition, error message) like this:

conditions = [(lambda: not placeToGo, "Enter a positive integer in the text box"),
(lambda: int(placeToGo) <= 0, "I need a positive integer in the text box please"),
(lambda: not DataStore.DBFILENAME, "There is no data available in the table")]

And then iterate through the list, calling each function in turn.

If you are not familiar with functional programming (in Python or any other language), take a look at this short tutorial


Down with FizzBuzz... LOL This post is the result of reading through and following the LOLCODE Specification 1.2 ("smoking the manual", right?), and writing and executing my code on compileonline.com.

My "hello world" was going to be a fizzbuzz. I like it because it nicely illustrates the basics of a language - variables, operators, looping, conditionals, and the likes.

As far as naming goes, the specification allows for a wide variety, so I decided to go PascalCase all the way, so as to ease up the reading a bit - otherwise the language is a bit harsh on the eyes, to say the least.

I'm curious about how idiomatic my code is - this is my very first and probably last lolcode program. Did I overlook language constructs that would help readability? How's naming? Indentation? Any issues?

HAI 1.2

    I HAS A Fizz ITZ 3
    I HAS A Buzz ITZ 5

    IM IN YR FrootLoopz UPPIN YR Frootz TIL BOTH SAEM Frootz AN 100
      BTW Frootz runs 0 through 99

      I HAS A Froot ITZ SUM OF Frootz AN 1
        BTW Froot will run 1 through 100

      BOTH SAEM 0 AN MOD OF Froot AN PRODUKT OF Fizz AN Buzz
      O RLY?
        YA RLY
          Froot R "FizzBuzz"
        NO WAI
          BOTH SAEM 0 AN MOD OF Froot AN Fizz
          O RLY?
            YA RLY
              Froot R "Fizz"
            NO WAI
              BOTH SAEM 0 AN MOD OF Froot AN Buzz
              O RLY?
                YA RLY
                  Froot R "Buzz"
              OIC
          OIC
      OIC

      VISIBLE Froot

    IM OUTTA YR FrootLoopz

    VISIBLE "DOWN WITH FIZZBUZZ LOL"

KTHXBYE

I like how you have declared the Fizz and Buzz 'constants'. It makes the code more readable.

The comments are useful too, though, for consistency, the second comment should be changed from:

BTW Froot will run 1 through 100

to

BTW Froot runs 1 through 100

The idea of generating Froot from Frootz is also good, because it reduces the computations later in the code.

I only wish you used a better name than Frootz because it conflicts with Froot. The similarity is.... uncanny.

I would suggest the name Loopz:

I HAS A Loopz ITZ SUM OF Frootz AN 1

This will reduce the ambiguity, and improve the readability and maintainability.

Finally, it is good that you put the Fizz modulo check before the Buzz check because that improves performance. Many people put buzz first, but that means the modulo-5 check happens, and that fails more often than the modulo-3 check, so people who do the Buzz-loop first tend to do about 20% more conditional checks.

All in all, for a first go, I am very impressed with your result. I can't wait until you implement a fim++ interpreter in lolcode. Till then!


AJAX Done Callback to prevent requests from interrupting each other I am trying to make sure my client-side code does not send another request before it receives a reply. To the best of my knowledge the code I will post is supposed to do this but I am still getting a -1 for $return.

The AJAX request repeatedly gets the echoed value of a function which is using a batch file to get the size of a file that is being uploaded via FTP. The application using this shifted storage and is now in UNC path and now the AJAX gets nothing in return.

If I run the bat file manually I can get a response, but it might take 1 second or it might take a minute, for some reason there doesn't seem to be a lot in the middle. If I use the PHP script to call it I get $out equal to an empty array and $return equal to -1. My guess is that it's taking so long to process the request the Ajax requests are interrupting each other.

Ajax:

I am using ajaxTime to try and see how long the requests are taking. They are varying from usually around 550-650 response time:

var ajaxTime = new Date().getTime();
getdSize(f, ajaxTime);

function getSize(f, ajaxTime){
        $.ajax({
            url: "php file handling the ajax request",
            type: "POST",
            data: {
                file : encodeURIComponent(f),
                }
            })
        .done(function(data){
            var totalTime = new Date().getTime()-ajaxTime;
            console.log("Ajax Time: " + totalTime);
            console.log("Data: " + data);
        })
        .fail(function(data){
            if ( data.responseCode ){
              console.log( data.responseCode );
            }
            console.log("Ajax failure");
        });
    }

PHP:

    $file = $_POST['file'];
    echo bat_getSize($file);
[this calls bat_getSize function]

function bat_getSize($file){
    exec("getSize \"$file\"",$out, $ret);
    return $out[0];
}

Which calls this batch code:

@echo off
echo %~z1

To be sure that nothing happens before the AJAX request is achieved, you can simply add the async: false property to your $.ajax({...}).

As pointed by @Tom Stickel, doing so fires a warning about future deprecation of async: false.

I agree, and this is already known from times but, as stated in paragraph 4.5.1 of the page he cited:

  This is a long process that takes many years

It's why I suggest this solution (and also am using it somewhat frequently), but:

only in the case of this kind of situation, where the "detrimental effects to the end user's experience" is clearly not involved (at the opposite, here, it's precisely what is expected)
only for now, considering it's worth applying a so simple solution as long as it works, and change it only when deprecation will effectively happen (note that the involved piece of code might be obsolete first!)


Add item at the beginning of each inner array in jagged array So, there is a big jagged string array (~ [120] [1 000 000]) here that represents data from an excel worksheet ( columns / rows).

Task: We have to append items at the beginning of each inner array.

I have a solution, but I am not sure about performance.

private string[][] AddField(string[][] exportDataFieldValues, string linkDisplayFieldValue)
    {
        var newExportDataFieldValues = new List<string[]>();
        foreach (var row in exportDataFieldValues)
        {
            var cellsRange = row.ToList();
            cellsRange.Insert(0, linkDisplayFieldValue);

            newExportDataFieldValues.Add(cellsRange.ToArray());
        }

        return newExportDataFieldValues.ToArray();
    }

Q: As you can see, my solution contains a lot of type conversions, I believe there is a more effective solution. Сan you provide a more effective solution?

I guess your code will behave very bad performance wise.
I would suggest to create the cellsRange object first as a List<whatever> and add 0,linkDisplayFieldValue first and then call AddRange() on cellsRange passing row.ToList() as parameter.


Showing an item in a shopping cart I'm updating and old project from jQuery 1.4 to jQuery 1.7.

Can this be simplified, thus reducing the amount of code, and perhaps in such reduction, have it improved?

Having the following code:

var cartItemDel   = '<input type="hidden" class="lid" name="lineID'+ id +'" value="'+ id +'">'+token+'	',
    cartItemName  = '<input class="CartItemId" type="hidden" name="id_'+ id +'" value="'+ id +'">'+ name +'	',
    cartItemPrice = '<input class="CartItemRef" type="hidden" name="ref_'+ ref +'" value="'+ ref +'">'+ price +'&euro;	';

$(".cart-table tbody")
    .append('
'+cartItemDel+cartItemName+cartItemPrice+'
');

I've updated it to:

/*
 * ADD NEW LINE - First TD
 */
var $newLineIcon = $("", {
        src    : "components/themes/default/img/icons/cross-small.png",
        width  : "16",
        height : "16",
        alt    : "X"
    });

var $newLineLID = $("<input/>", {
        type  : "hidden",
        class : "lid",
        name  : "lineID"+ lid,
        value : lid
    });

var $newLineToken = $("", {
        class : "hidden token",
        text  : token
    });

var $newLine_firstTD = $("", { class : "del"})
    .append($newLineIcon)
    .append($newLineLID)
    .append($newLineToken);

/*
 * ADD NEW LINE - Second TD
 */
var $newLinePID = $("<input/>", {
        type  : "hidden",
        class : "CartItemId",
        name  : "id_"+ id,
        value : id
    });

var $newLinePNAME = $("", {
        class : "CartItemName",
        text  : name
    });

var $newLine_secondTD = $("")
    .append($newLinePID)
    .append($newLinePNAME);

/*
 * ADD NEW LINE - Third TD
 */
var $newLinePREF = $("<input/>", {
        type  : "hidden",
        class : "CartItemRef",
        name  : "ref_"+ ref,
        value : ref
    });

var $newLinePPRICE = $("", {
        class : "CartItemPrice",
        html  : "&euro;"
    })
    .prepend($("", {class : "lineSum", text : price}));

var $newLine_thirdTD = $("")
    .append($newLinePREF)
    .append($newLinePPRICE);

$("
")
    .append($newLine_firstTD)
    .append($newLine_secondTD)
    .append($newLine_thirdTD)
    .appendTo($(".cart-table tbody"));

Your code looks OK to me, very clear and organised.

It's more important to minify your code to decrease load time. There's a whole bunch of minification tools on the Internet.


An extension to the StringBuilder This is another pretty basic class I wrote for a library as I hate the way the default StringBuilder in .NET works.

Essentially, I wanted to have the + operator, as well as implicit conversions to strings. (Rather than needing .ToString() all the time.)

It's pretty small and simple, so there may not be a lot to critique.

Also, before you say "just inherit StringBuilder and extend it", it's sealed.

///
/// This wraps the .NET StringBuilder in a slightly more easy-to-use format.
///
public class ExtendedStringBuilder
{
    private StringBuilder _stringBuilder;

    public string CurrentString => _stringBuilder.ToString();

    public int Length => _stringBuilder.Length;

    public ExtendedStringBuilder()
    {
        _stringBuilder = new StringBuilder();
    }

    public ExtendedStringBuilder(int capacity)
    {
        _stringBuilder = new StringBuilder(capacity);
    }

    public ExtendedStringBuilder Append(string s)
    {
        _stringBuilder.Append(s);

        return this;
    }

    public ExtendedStringBuilder Append(char c)
    {
        _stringBuilder.Append(c);

        return this;
    }

    public ExtendedStringBuilder Append(object o)
    {
        _stringBuilder.Append(o);

        return this;
    }

    public static ExtendedStringBuilder operator +(ExtendedStringBuilder sb, string s) => sb.Append(s);

    public static ExtendedStringBuilder operator +(ExtendedStringBuilder sb, char c) => sb.Append(c);

    public static ExtendedStringBuilder operator +(ExtendedStringBuilder sb, object o) => sb.Append(o);

    public static implicit operator string(ExtendedStringBuilder sb) => sb.CurrentString;

    public override string ToString() => CurrentString;

    public string ToString(int startIndex, int length) => _stringBuilder.ToString(startIndex, length);
}

I didn't implement all the overloads of the .Append method (yet) or the + variants of them.

This can literally be used in the exact same manner as the .NET StringBuilder, or you can use += or + instead of .Append, and you can implicitly convert it to a string.

A couple of quick comments:

You can use the see tag's cref attribute. If you generate documentation, some tools will generate hyperlinks for you.

/// <summary>
/// This wraps the .NET <see cref="StringBuilder"/> in a slightly easier to use format.
/// </summary>

The length property of a StringBuilder is read and writable. It's also really useful for it to be so:

var sb = new StringBuilder();
foreach (var i in Enumerable.Range(0,10))
{
    sb.AppendFormat("{0},", i);
}
sb.Length--; // removes the trailing comma.

That's a contrived example which is trivially served with string.Join but setting the length can be useful!

The _stringBuilder field should be readonly.

I'd say CurrentString is superflous. Just call _stringBuilder.ToString()

I must admit, personally I think the StringBuilder api is really good, I've never needed an implicit conversion to a string or felt the need to + rather than append to them.


Get only intended input from user through the console I finally worked out all of the bugs in this bit of code. I'm pretty proud of it and I wanted to give it to all of you so that you could have it if you wanted it for any console programs that you make.

At the same time, this is ridiculous. C++ is double my age and there should seriously be a better way of getting valid, error free, inputs from the user. If anyone knows, please tell me.

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

void gValid_Input(std::string& var, std::string question) {
    using namespace std;

    do {
        cin.clear();
        cin.sync();
        cout << question;

        // Code will not work as intended on Linux without the following
        // but will work on windows (tested with g++)
        // this must also be done after clearing, syncing cin
        // and asking the question string otherwise the code will loop forever
        if (cin.peek() == '\n'){
            cin.ignore(1,'\n');
        }
    } while (!(getline(cin, var)));
}

template <typename t>
void gValid_Input(t& var, std::string question) {
    using namespace std;

    do {
        cin.clear();
        cin.sync();
        cout << question;
    } while (!(cin >> var));
}

void gValid_Option(char& response, std::vector<char> valid_Responses = {'y','n'}){
    using namespace std;
    const char diff = 'a' - 'A';

    do{
        cin.clear();
        cin.sync();
        cin >> response;

        // Comment the following out if you care about getting an upper case answer
        if (response >= 'A' && response <= 'Z'){
            response += diff;
        }

    } while (find(valid_Responses.begin(), valid_Responses.end(), response) == valid_Responses.end());
}

void gValid_Option(char& response, std::string question, std::vector<char> valid_Responses = {'y','n'}){
    using namespace std;
    const char diff = 'a' - 'A';

    do{
        cin.clear();
        cin.sync();
        cout << question;
        cin >> response;

        // Comment the following out if you care about getting an upper case answer
        if (response >= 'A' && response <= 'Z'){
            response += diff;
        }

    } while (find(valid_Responses.begin(), valid_Responses.end(), response) == valid_Responses.end());
}

I see some things that may help you improve your program.

Use const references where practical

The code currently declares its first function like so:

void gValid_Input(std::string& var, std::string question);

However the passed question is not and should not be altered by this code, so better would be to pass a const std::string& question or std::string_view question if you have a C++17 compiler.

Use return values to return values

Each of the functions is defined as returning void but each is actually returning a single value which is the response.  So for the first function, including the advise listed above, one could instead make the signature of the first function this:

std::string gValid_Input(const std::string& question);

Fix the bug

When the user inputs the end of file marker (Ctrl+D in Linux), the prompt just repeats endlessly in a loop.

Consolidate the code

All four of the functions could be represented with a single templated function:

template <typename T>
T gValid_Input(const std::string& prompt, bool (*isValid)(const T&) = [](const T&){ return true; }) {
    T var;
    while (std::cout << prompt && !((std::cin >> var) && isValid(var))) {
        if (std::cin) {
            std::string line;
            std::getline(std::cin, line);
            std::cout << var << line << " is not a valid answer\n";
        } else {
            std::cin.clear();
            std::string line;
            std::getline(std::cin, line);
            std::cout << line << " is not a valid answer\n";
        }
    }
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    return var;
}

Here's are some example of its use:

#include <complex>
#include <array>

int main() {
    auto answer{gValid_Input<std::string>("What is your name? ")};
    std::cout << "Your answer: \"" << answer << "\"\n";
    auto age{gValid_Input<int>("What is your age? ")};
    std::cout << "Your answer: \"" << age << "\"\n";
    auto cpx{gValid_Input<std::complex<double>>("What is your favorite complex number? ")};
    std::cout << "Your answer: \"" << cpx << "\"\n";
    auto weekday{gValid_Input<std::string>("What is your favorite day of the week? ",
        [](const std::string &day){ const std::array<std::string, 7> weekdays{
            "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday"
            };
            return std::find(weekdays.begin(), weekdays.end(), day) != std::end(weekdays);
        }
    )};
    std::cout << "Your answer: \"" << weekday << "\"\n";
    auto fun{gValid_Input<char>("Are you having fun? ",
        [](const char &ch){ return ch=='y' || ch=='n'; }
    )};
    std::cout << "Your answer: \"" << fun << "\"\n";
}

I'm using a lambda function as the validator, but it could also be any function with the appropriate signature.

Reconsider the interface

It would be nice to be able to use streams other than std::cin and std::cout.  I'd recommend passing the input and output streams as parameters.


Excel to serializable object I've got an Excel file from which I have to read out data to an object to serialize. So far I came up with this solution, and I'm curious if there are any clearer solutions.

public myClassFromXsd excelToMyClassFromXsd(String excelFile)
            {
                Excel.Application excelApp = new Excel.Application();
                Excel.Workbook workBook = excelApp.Workbooks.Open(excelFile);
                Excel.Worksheet workSheet = (Excel.Worksheet)workBook.Sheets[1];

                myClassFromXsd spec = new myClassFromXsd();
                UserHeader user = new UserHeader();
                ResultHeader header = new ResultHeader();
                anotherClassFromXsd specOp = new anotherClassFromXsd();
                List<anotherClassFromXsd> specOpList = new List<anotherClassFromXsd>();
                thisIsAClassToo tcd = new thisIsAClassToo();

                excelApp.Visible = false;

                tcd.orderNumber = Convert.ToString(workSheet.get_Range("B3").Value);
                switch (((String)workSheet.get_Range("B4").Value).ToUpper())
                {
                    case "I":
                        tcd.typeOfThis = EnumFromXsd.I;
                        break;
                    case "E":
                        tcd.typeOfThis = EnumFromXsd.E;
                        break;
                    case "D":
                        tcd.typeOfThis = EnumFromXsd.D;
                        break;
                    default:
                        break;
                }
                switch (((String)workSheet.get_Range("B5").Value).ToUpper())
                {
                    case "I":
                        tcd.othersCanAlter = true;
                        break;
                    case "N":
                        tcd.othersCanAlter = false;
                        break;
                    default:
                        break;
                }
                tcd.oneProperty = Convert.ToString(workSheet.get_Range("B6").Value);
                tcd.onePropertyText = Convert.ToString(workSheet.get_Range("B7").Value);
                tcd.Name = Convert.ToString(workSheet.get_Range("B10").Value);
                tcd.vatNum = Convert.ToString(workSheet.get_Range("B11").Value);
                tcd.city = Convert.ToString(workSheet.get_Range("B12").Value);

                specOp.index = 1;
                specOp.operation = Operation.create;
                specOp.Item = tcd;
                specOpList.Add(specOp);

                header.requestId = Guid.NewGuid().ToString();
                header.timestamp = DateTime.Now.ToString("yyyy-mm-ddTH:mm:sszzz");

                user.user = "TestUser";
                user.passwordHash = "BA32521232AED23798735C78739273GDHS238238723";

                spec.header = header;
                spec.user = user;
                spec.specOps = specOpList;

                excelApp.Quit();
                return spec;
            }

And the serializer and deserializer methods:

public void serializeXml(myClassFromXsd mcfx)
        {
            using (StreamWriter writer = new StreamWriter("e:\\test.xml", false))
            {

                XmlSerializer serializer = new XmlSerializer(typeof(myClassFromXsd));
                serializer.Serialize(writer, mcfx);
            }
        }

        public myClassFromXsd deserializeXml(String xmlFile)
        {
            myClassFromXsd mcfx = new myClassFromXsd();
            XmlSerializer serializer = new XmlSerializer(typeof(myClassFromXsd));
            StreamReader reader = new StreamReader(xmlFile);
            mcfx = (myClassFromXsd)serializer.Deserialize(reader);
            reader.Close();
            return mcfx;
        }

The indendation looks off - assuming it's not a paste glitch with tabs vs spaces, the scope-opening brace should line up with the method's signature:

public myClassFromXsd excelToMyClassFromXsd(String excelFile)
            {

Like this:

public myClassFromXsd excelToMyClassFromXsd(String excelFile)
{

Also, C# type names should be PascalCase, as well as any public members:

myClassFromXsd should be MyClassFromXsd
serializeXml should be SerializeXml
deserializeXml should be DeserializeXml

The get_Xxxx methods in the COM Interop interface aren't following this convention, but they're COM Interop methods, with their own conventions - they're not an example to follow for typical C# code.

I like that you're wrapping your StreamWriter in a using block. I don't understand why you're not doing the same with the StreamReader in the deserializeXml method - the reader.Close() call could then be removed:

    public myClassFromXsd deserializeXml(String xmlFile)
    {
        myClassFromXsd mcfx = new myClassFromXsd();
        XmlSerializer serializer = new XmlSerializer(typeof(myClassFromXsd));
        using (StreamReader reader = new StreamReader(xmlFile))
        {
            mcfx = (myClassFromXsd)serializer.Deserialize(reader);
        }
        return mcfx;
    }

I would give mcfx a meaningful name: as it stands it looks like it's just a shortened version of the return type's name - I like having a result for these kinds of things. Identifiers should have a pronounceable name that carries their meaning.


C - approximate pi (Monte Carlo) This is my first C program that is not "Hello World" level; I would welcome ideas to make it more efficient and readable.
#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <time.h>
#include <stdbool.h>
#define FALSE 0
#define TRUE  1

bool incircle(double x, double y);

int main (int argc, char *argv[]) {
    unsigned long long loops, cpoints, i;
    double             approx, cpu_time_used;
    int                seed;
    clock_t start, end;
    cpoints  = 0;
    loops = atoll(argv[1]);
    seed = time(NULL);
    srand(seed);
    start = clock();
    for (i = 0; i < loops; ++i)
    {
        double x = (rand() + 1.0) / (RAND_MAX+2.0);
        double y = (rand() + 1.0) / (RAND_MAX+2.0);
        if ( incircle(x,y) !=FALSE )
            cpoints+=1;
    }
    end = clock();
    cpu_time_used = ((double) (end - start)) / CLOCKS_PER_SEC;
    approx=4*(cpoints/(double)i);
    printf ("%llu" "%s" "%llu"   "%s" "%0.9f" "%s" "%0.3f\n",
            i,     " ", cpoints, " ", approx, " ", cpu_time_used);
    return 0;
}

bool incircle(double x, double y) {
        return ( x * x + y * y <= 1 );
    }

General Observations
There is a real lack of communications with the user of the program. There is no error checking performed on the possible input. While the code really doesn't do that much it is too complex and very hard to maintain.
Communications With the User
Unless the user knows exactly how to call the program, the program will crash with no indication of what went wrong. This is because the code uses argv[1] for input and doesn't check that argv[1] actually exists. The following code would be better for the user of the program because it check that an argv[1] actually exists first before using it and reports an error if there is no argv[1]:
    if (argc > 1)
    {
        loops = atoll(argv[1]);
    }
    else
    {
        fprintf(stderr, "USAGE MESSAGE");
        return EXIT_FAILURE;
    }

It is also unclear what is being printed in the following statement:
    printf ("%llu" "%s" "%llu"   "%s" "%0.9f" "%s" "%0.3f\n",
        i, " ", cpoints, " ", approx, " ", cpu_time_used);

It might be better if the the previous statement was something like this:
    printf ("i = %llu cpoints = %llu approx = %0.9f cpu_time_used = %0.3f\n",
        i, cpoints, approx, cpu_time_used);

Note: Use the formatting capabilities of the printf statement rather than using "%s" to print spaces. Rather than using the variable i in the statement it would be more understandable to use the variable loops, i and loops should be the same at this point.
Declare and Initialize the Variables as Needed
C is not the nicest language to program in because there is no default value assigned to local variables so it is always best to initialize the variable when it is declared, each variable should be declared and initialized on its own line to make the code easier to read, write, maintain and debug.
    unsigned long long loops = 0;
    unsigned long long cpoints = 0;
    unsigned long long i = 0;

If the variable loops is used for the calculations and printing then the variable i can actually be declared in the for loop.
    for (unsigned long long i = 0; i < loops; ++i)
    {
        double x = (rand() + 1.0) / (RAND_MAX+2.0);
        double y = (rand() + 1.0) / (RAND_MAX+2.0);
        if ( incircle(x,y) !=FALSE )
            cpoints+=1;
    }

Unnecessary Code
Since stdbool.h is included the #defines for TRUE and FALSE are unnecessary because stdbool.h defines true and false. In the if statement where FALSE is used you have a double negative, which is as bad in programming as it is in English. The if statement can be simplified to
        if (incircle(x,y))
            cpoints += 1;

Code Complexity
##Complexity
The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.
There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

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

The main() function can be broken up into the following functions

Get the user input from the command line and report any errors
Set up the random number generator.
The loop that is currently doing the calculations should be in its own function.

Compiler Warnings
Make sure you enable the compiler warnings when you compile some of the casts in the code are questionable and my compiler is reporting warnings. It might be best to compile with the -wall switch (enable all warnings).


Request and callback design I am just trying to improve my coding and designing skills in C++ and for that
I am trying to solve same age old problem of mapping a http request to method.

Could you guys please look at my program and point me to mistakes I am doing with design and coding style?

resource.hpp

#include <functional>
#include <string>

typedef std::function<void(void)> Handler;

enum class RequestMethod {
  GET,
  PUT,
  POST,
  DELETE
};

class Resource {

  public:
    Resource() {}
    void SetMethodHandler(const RequestMethod method, const Handler& callback);
    Handler GetMethodHandler();

    void SetPath(const std::string& path);
    std::string& GetPath();

  private:
    Resource(const Resource& other) = delete;
    Resource& operator=(Resource& rhs) = delete;

    std::string m_path;
    RequestMethod m_method;
    Handler m_callback;
};

resource.cpp

#include "resource.hpp"

void Resource::SetPath(const std::string& path) {
  m_path = path;
}

std::string& Resource :: GetPath() {
  return m_path;
}

void Resource :: SetMethodHandler(RequestMethod method, const Handler& callback) {
  m_method = method;
  m_callback = callback;
}

Handler Resource :: GetMethodHandler() {
  return m_callback;
}

main.cpp

#include "resource.hpp"
#include <memory>
#include <iostream>
#include <map>

void LoginMethodHandler() {
  std::cout << "Code here to handle a login request." << std::endl;
}

int main() {

  using ResourceSharedPtr = std::shared_ptr<Resource>;

  ResourceSharedPtr resource_ptr = std::make_shared<Resource>();
  resource_ptr->SetPath("/login");
  resource_ptr->SetMethodHandler(RequestMethod::GET, LoginMethodHandler);

  /* call to method handler */
  /* ideally in full blown application, logic will be search for path and
     method and call approriate handler */
  resource_ptr->GetMethodHandler()();

  auto path_map = std::make_pair(resource_ptr->GetPath(),resource_ptr);
  return 0;
}

I think the idea to create an abstraction class for request handling is a good one..

A few remarks and/or changes to consider:

This code is flagged as C++14 but there is only C++11 specific code.
I like the idea of using scoped enumerations (enum class ...) instead of the old-style enumerations. Scoped enumerations are more strictly typed.
There are no include guards in resource.hpp. They can protect against (potential) double inclusions.
Copy construction and copy assignment have been deleted, but by convention, deleted functions are declared public, not private..
As for the (deleted) copy assignment, the argument should be a const reference.
In main.cpp, it is good pratice to keep the application specific include file (resource.hpp) under the system include files.
get and set methods are defined in resource.cpp, but these functions are so limited,
why not include them directly in the header file, which would essentially eliminate resource.cpp.
For a default, compiler generated, constructor, it is common practice to use:

Resource() = default;

However, since an uninitialized object is created and populated with values after that, why not set those during construction ? - the constructor then becomes:

Resource(const std::string &path, RequestMethod method, const Handler& callback)
    : m_path{path}, m_method{method}, m_callback{callback} { }

The object can still be modified by calling set functions later.
Resource::GetPath() returns a reference to an lvalue which gives callers direct access to Resource::m_path, which is private.
Many consider this a violation of data encapsulation. Consider changing the return type to a plain std::string.
A std::map is not used yet, but the intention seems to be to store a std::pair in a std::map called path_map. You can call std::make_pair directly on the map inserter:

std::map< std::string, ResourceSharedPtr> path_map;
...
path_map.insert(std::make_pair(resource_ptr->GetPath(),resource_ptr));

If you are planning to use only the ResourceSharedPtr in combination with the std::map pair, you could possibly eliminate m_path inside the Resource class and use the std::map key. Otherwise they are redundant.

With this, the updated source code becomes:

resource.hpp

#ifndef RESOURCE_H
#define RESOURCE_H

#include <functional>
#include <string>

typedef std::function<void(void)> Handler;

enum class RequestMethod {
  GET,
  PUT,
  POST,
  DELETE
};

class Resource {

  public:
    Resource(const std::string &path, RequestMethod method, const Handler& callback) :
        m_path{path}, m_method{method}, m_callback{callback} { }
    void SetMethodHandler(const RequestMethod method, const Handler& callback)
    {
      m_method = method;
      m_callback = callback;
    }

    Handler GetMethodHandler() { return m_callback; }

    void SetPath(const std::string& path) { m_path = path; }
    std::string GetPath() { return m_path; }

    Resource(const Resource& other) = delete;
    Resource& operator=(const Resource& rhs) = delete;

private:
    std::string m_path;
    RequestMethod m_method;
    Handler m_callback;
};

#endif // RESOURCE_H

main.cpp

#include <memory>
#include <iostream>
#include <map>

#include "resource.hpp"

void LoginMethodHandler()
{
  std::cout << "Code here to handle a login request." << std::endl;
}

using ResourceSharedPtr = std::shared_ptr<Resource>;

std::map<std::string, ResourceSharedPtr> path_map;

int main() {

  ResourceSharedPtr resource_ptr = std::make_shared<Resource>("/login",
                                                              RequestMethod::GET,
                                                              LoginMethodHandler);

  /* call to method handler */
  /* ideally in full blown application, logic will be search for path and
     method and call approriate handler */
  resource_ptr->GetMethodHandler()();

  path_map.insert(std::make_pair(resource_ptr->GetPath(),resource_ptr));

  return 0;
}


Passing an array created in function to other functions I'm beginning my programming adventure in C++. I want to write a small program, which has to create an array in function create (size has to be defined in create function) and transfer them to function display which will print entered values.

#include <iostream>

using namespace std;

int *create(int & n);
void display(int * arr, int n);

int main()
{
    int n;
    int * arr = create(n);
    display(arr, n);

    delete[] arr;

    return 0;
}

int *create(int & n) {
    cout << "How many elements? ";
    cin >> n;
    int * arr = new int[n];
    int element;
    for(int i = 0; i < n; i++) {
        cout << endl << "Enter " << i << " array element: ";
        cin >> element;
        arr[i] = element;
    }

    return arr;
}

void display(int * arr, int n) {
    for(int i = 0; i < n; i++) {
        cout << endl << i << " element: " << arr[i];
    }
}

I'm looking for every criticism, optimization suggest and everything which will help me be a better programmer and not to learn bad habits from the beginning.

First, the usual criticisms:

Don't use using namespace std;
Don't use raw new and delete
Include the headers your code uses (e.g. <iostream>)
Prefer prefix ++i over postfix i++ as a general habit
Prefer to omit the implied return 0; from main (unless you're trying to emphasize that the return value is different via different codepaths, which doesn't apply in this case)

Then, the point specific to your API design:

Your function create has two outputs: the pointer to a new array and the size of the new array, n. The former is returned as the return value; the latter is passed in by reference (the out-parameter pattern). Instead of returning these two pieces of information by two different routes, you should unify them: Either pass in two out-parameters (yuck!) or pass out two values by return.

Taking the latter approach (two values by return) and applying all our rules except "no raw new and delete", we end up with this:

#include <iostream>
#include <utility>

std::pair<int *, int> create() {
    int n;
    std::cout << "How many elements? ";
    std::cin >> n;
    int *arr = new int[n];
    for (int i = 0; i < n; ++i) {
        int element;
        std::cout << std::endl << "Enter " << i << " array element: ";
        std::cin >> element;
        arr[i] = element;
    }
    return {arr, n};
}

void display(int *arr, int n) {
    for (int i = 0; i < n; ++i) {
        std::cout << std::endl << i << " element: " << arr[i];
    }
}

int main() {
    auto [arr, n] = create(n);
    display(arr, n);
    delete [] arr;
}

The JavaScript-looking [arr, n] is C++17 syntax, but don't worry, we're about to get rid of it in our next refactor.

Applying "no raw new and delete" allows us to simplify much further:

#include <iostream>
#include <vector>

std::vector<int> create() {
    int n;
    std::cout << "How many elements? ";
    std::cin >> n;
    std::vector<int> result;
    for (int i = 0; i < n; ++i) {
        int element;
        std::cout << std::endl << "Enter " << i << " array element: ";
        std::cin >> element;
        result.push_back(element);
    }
    return result;
}

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

int main() {
    auto vec = create(n);
    display(vec);
}

The last thing I'd consider doing is eliminating mutation from the code. You've got these variables int n and int element that aren't initialized, and then you use std::cin >> ... to mutate their values. It might be more straightforward to write an "int-getting function" and use it consistently. Plus, this gives us a place to hang the missing std::flush:

#include <iostream>
#include <vector>

template<class T>
T input(const char *prompt) {
    T result;
    // Print out everything in `prompt` (C++17 syntax)
    std::cout << prompt << " " << std::flush;
    std::cin >> result;  // Should we add some error-checking?
                 // What if the user types "hello world" here?
    return result;
}

std::vector<int> create() {
    std::vector<int> result;
    int n = input<int>("How many elements?");
    for (int i = 0; i < n; ++i) {
        result.push_back(input<int>("Enter the next array element:"));
    }
    return result;
}

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

int main() {
    display(create(n));
}

Notice that I changed your prompt from "Enter the 42 array element:" to simply "Enter the next array element:". This was because I didn't want to bother explaining how to write a variadic-template version of input that would accept input<int>("Enter the ", i, " array element:"). It's possible — even trivial, in C++17 where we can write (std::cout << ... << prompt_pieces) << " " << std::flush — but I want to keep the simple code simple.

Now we've localized all the icky "state mutation" into the innards of input<int>; we don't have to think about mutation while we're reading create.


Sequential Thread Execution using wait() and notifyAll() Problem Statement:

  T1, T2 to Tn threads prints numbers up to N such that each threads
  prints number in circular sequential fashion. T1 prints 1, T2 prints
  2, T3 prints 3 and T1 prints 4 again following strict ordering. This
  implementation would allow us to control any number of threads to do
  sequential ordered execution. It uses wait() and notifyAll() methods
  to signal threads. I am using HashMap to store sequence I would like
  to achieve and this can allow me to add N threads as long as I can
  control turns for each threads.

Please suggest any improvements.

class SignalThread implements Runnable {

    volatile Integer i = 1;

    volatile String turn = "1";

    Map<String, String> sequence = new HashMap<String, String>();

    WaitNotifySignal waitNotifySignal;

    public SignalThread(WaitNotifySignal waitNotifySignal) {
        this.waitNotifySignal = waitNotifySignal;
    }

    @Override
    public void run() {
        while (i.intValue() <= 10) {
            if (Thread.currentThread().getName().equalsIgnoreCase(turn)) {
                System.out.println("Thread: "
                        + Thread.currentThread().getName() + " --- " + i);
                i++;
                turn = getNextTurn(turn);
                waitNotifySignal.doNotifyAll();
                waitNotifySignal.doWait();
            }
        }
    }

    public void setTurnSequence(Map<String, String> sequence) {
        this.sequence = sequence;
    }

    private String getNextTurn(String currentTurn) {
        return sequence.get(currentTurn);
    }
}

And:

/**
 * Common Wait Notify Signal class
 *
 * @author manishdevraj
 *
 */

 class WaitNotifySignal {

    Object monitorObject = new Object();

    boolean wasSignalled = false;

    public void doWait() {
        synchronized (monitorObject) {
            while (!wasSignalled) {
                try {
                    monitorObject.wait();
                } catch (InterruptedException e) {
                }
            }
            // clear signal and continue running.
            wasSignalled = false;
        }
    }

    public void doNotifyAll() {
        synchronized (monitorObject) {
            wasSignalled = true;
            monitorObject.notify();
        }
    }
}

public class LinearCircularSignalT {

    public static void main(String[] args) {

        SignalThread signalThread = new SignalThread(new WaitNotifySignal());
        Thread t1 = new Thread(signalThread);
        t1.setName("1");
        Thread t2 = new Thread(signalThread);
        t2.setName("2");
        Thread t3 = new Thread(signalThread);
        t3.setName("3");
        Map<String, String> sequence = new HashMap<String, String>();
        sequence.put("1", "2");
        sequence.put("2", "3");
        sequence.put("3", "1");
        signalThread.setTurnSequence(sequence);
        t1.start();
        t2.start();
        t3.start();

    }
}

the first issue I see is that i++; is not thread safe. Instead use an AtomicInteger

second is that the thread gets into a spin lock when it is awake and it's not its turn. the solution to this is to do:

while(!Thread.currentThread().getName().equalsIgnoreCase(turn)) {
    waitNotifySignal.doWait();
}

but this has a race condition (turn changed after the test but before the wait->the changing thread has already called doNotifyAll() and deadlock occurs. (This is the reason why wait and notify have to be in the synchronized blocks)

This means that you need to synchronize the testing and the changing of turn, but if we do this then we don't need a special WaitNotifySignal class but a normal Object will suffice:

class SignalThread implements Runnable {

    final AtomicInteger index = new AtomicInteger(1);

    String turn = "1"; // synchronization will take care of visibility

    Map<String, String> sequence = new HashMap<String, String>();

    Object waitNotifySignal;

    public SignalThread(Object waitNotifySignal) {
        this.waitNotifySignal = waitNotifySignal;
    }

    @Override
    public void run() {
        int i = index.get();
        while (i <= 10) {
            synchronized(waitNotifySignal){
                while(!Thread.currentThread().getName().equalsIgnoreCase(turn)) {
                    try{
                        waitNotifySignal.wait();
                    }catch(InterruptedException e){
                        return;//interrupted means that the thread should stop
                    }
                }
            }

            System.out.println("Thread: "
                    + Thread.currentThread().getName() + " --- " + i);
            i = index.incrementAndGet();

            synchronized(waitNotifySignal){
                turn = getNextTurn(turn);
                waitNotifySignal.notifyAll();
            }
        }
    }

    //...
}

besides that your doNotifyAll() didn't. only one thread ever got out of the synchonized block in doWait, which means that deadlock would occur if more than one thread was waiting and the wrong thread got notified.


How could this constructor code be improved? How could this constructor code be improved? I feel like I'm not doing something optimally here...

@interface JGProduct : NSObject {
    NSString *identifier;
}

+(JGProduct *)productWithIdentifier:(NSString *)identifier_;
-(JGProduct *)initWithIdentifier:(NSString *)identifier_;

@end

@implementation JGProduct

+(JGProduct *)productWithIdentifier:(NSString *)identifier_ {
    return [[[[self class] alloc] initWithIdentifier:identifier_] autorelease];
}

-(JGProduct *)initWithIdentifier:(NSString *)identifier_ {
    self = [super init];
    if (self) {
        _identifier = [identifier_ retain];
    }

    return self;
}

@end

id or JGProduct return type?

I see a lot of people writing

-(id)initWithIdentifier:(NSString *)identifier_ {

instead - why is this?

Surely any subclass of JGProduct would be exchangeable for the superclass... Or should I be returning something that is id but conforms to a protocol (if present)?

JGProduct or [self class] in initialiser?

I also see this:

+(JGProduct *)productWithIdentifier:(NSString *)identifier_ {
    return [[[JGProduct alloc] initWithIdentifier:identifier_] autorelease];
}

or

+(JGProduct *)productWithIdentifier:(NSString *)identifier_ {
    return [[[self alloc] initWithIdentifier:identifier_] autorelease];
}

Why would someone write either of these? The top one surely would not make the right kind of object if ran by a subclass. And what's the difference between self and [self class] if you're in a class method?

Thanks for any feedback!

Why -init... should be declared to return id

Imagine you have class A and its subclass B sharing the same designated initializer, initWithString:.

@interface A : NSObject
-(A *) initWithString: (NSString *) string;
@end

@interface B : A
-(B *) initWithString: (NSString *) string;
@end

As you can see, the declarations have a conflicting return type, which unavoidably causes a compiler warning. You can trivially fix it by changing the methods to return id, which, given Objective-C dynamic typing, doesn't change anything about how these methods work. So it has become a well-established, but not as well-understood, tradition.

Why class initializers should use self rather than the class name

As you correctly noticed, using JGProduct instead of self will allocate an instance of JGProduct for any subclass, which is not what you want in 99% cases.

Why write +[JGProduct productWithIdentifier:]?

Most of the time, it's just a shortcut to alloc, init and autorelease to make calling code more readable. However, these initializers may be much more complicated and more efficient when implemented in class clusters. When you write a class cluster, you have one public superclass and several private subclasses. In such cases the superclass's alloc has to return a placeholder object which will allocate and instantiate an object of the right subclass in its init... implementation. The class initializer, on the other hand, can immediately return an already inited object of the right subclass.


Using 'dynamic' to save lines I am pushing an Excel file to the server and then reading it's contents. XLS and XLSX files need to be read differently, but the DLL that I'm using has the EXACT same function calls for both types. If I use a 'dynamic' type, I can save a lot of lines of code and avoid redundancy. However, I've been told many times before to avoid using dynamic if possible. Is this a good place to use it?

dynamic excelFile; // is this a good use of dynamic?
switch (fileExtension)
{
    case "xls":
        excelFile = new Excel2007();
        break;
    case "xlsx":
        excelFile = new Excel();
        break;
    default:
        results.Message = string.Format("{0} is of an unrecognized format.", uploadFile.FileName);
        return RedirectToAction("ImportWizard");
}

excelFile.OpenDatabase(targetPath, new FieldData());
excelFile.CurrentTable = excelFile.Tables[0];

var excelRecords = excelFile.ReadRecords(100);
excelFile.CloseDatabase();

The prevalent answer is: No. (Funny enough, working with Office files seems to be a valid exception to the rule).

The problem is that you don't have type validation anymore, and any issues will reveal themselves at run time.

I'd say, if it's a small thing and it won't be touched or changed by people who might not know how it works, go for it (Even then, make sure you document this).

Otherwise, an Interface approach sounds like a better idea, like ChrisWue's comment said.


Brute force searching I'm currently working on a natural language processing project and I have two ArrayLists<String> where each ArrayList contains a verb at index 0, a noun at index 1 and a noun at index 2 (repeat). I'm trying to find any two nouns in the first list that is present consecutively in the second by using:

 for(int i = 1; i<finalKnowledgeGraph.size(); i+=3) {
      for(int j = 1; j<finalKnowledgeGraph.size(); j+=3) {
           for(int k=1; k<storeAsserions.size(); k+=3) {
               if(finalKnowledgeGraph.get(i).equals(storeAsserions.get(k)) && finalKnowledgeGraph.get(j+1).equals(storeAsserions.get(k+1))){
                   System.out.println("Found one");
               } else if(finalKnowledgeGraph.get(i+1).equals(storeAsserions.get(k)) && finalKnowledgeGraph.get(j).equals(storeAsserions.get(k+1))) {
                   System.out.println("Found another");
               }
            }
        }
    }

However, this code has cubic complexity and both ArrayLists are thousands of lines long. I'm wondering if anyone has any tips on how I could go about speeding up this process. Also, I know virtually nothing about optimization so if you do have some help please break it down relatively simply.
One of my friends recently suggested a HashMap and searching through that but in my head that's simply pushing the search problem from one data-structure to another

Your current approach would benefit from sorted inputs.

But an even simpler approach would be to ignore the irrelevant verbs and rephrase the problem like this:

  A noun phrase is a pair of nouns. We are given two input sets of noun phrases, and must compute set intersection, using a problem specific notion of "phrase equality".

This implies a linear O(n) input pre-processing pass, where you populate a HashSet with each input phrase, X+" "+Y.

With that in hand, your task is trivial. Scan the other collection of phrases, and for each phrase make two set membership queries: is X+" "+Y or Y+" "+X present in the set?

BTW, your identifier storeAsserions seems to be a typo for storeAssertions. And please include a space after keywords like if & for.


What value the rear and front counter of an Array implementation of Queue start with? -1 or 0 or 1? Is there anything wrong with this implementation of Queue with array?

I have started front = rear = -1.

Some references tend to start front = 0 like this:enter link description here

I think if front starts with 0, the source code becomes cumbersome.

#include <stdio.h>
#include<ctype.h>
#define SIZE 5

int queue[SIZE] = {0};
int front = -1;
int rear = -1;

void PrintQueue()
{
    int i=0;

    if(!IsEmpty())
    {
        for(i=front+1 ; i<=rear ; i++)
        {
            printf("%d, ", queue[i]);
        }
        printf("\n");
    }
    else
    {
        printf("Queue is empty.\n");
    }
}

int IsEmpty()
{
    if(front==rear)
    {
        front = rear = -1;
        return 1;
    }
    else
    {
        return 0;
    }
}

void Enque(int value)
{
    if(rear<SIZE-1)
    {
        ++rear;
        queue[rear] = value;
    }
    else
    {
        printf("Queue overflow!\n");
    }
}

int Dequeue()
{
    int ret = 0;
    if(!IsEmpty())
    {
        ++front;
        ret = queue[front];
    }
    else
    {
        printf("Queue underflow!\n");
        ret = -99;
    }

    return ret;
}

main()
{
    int value = 0;
    int menu = 0;

    while(1)
    {
        printf("Menu\n");
        printf("--------\n");
        printf("1. Enque\n");
        printf("2. Deque\n");
        printf("3. IsEmpty\n");
        printf("4. Print\n");
        printf("5. Clrscr\n");
        printf("6. Exit\n");
        scanf(" %d", &menu);

        switch(menu)
        {
        case 1:
            printf("Enter an element : ");
            scanf(" %d", &value);
            Enque(value);
            break;

        case 2:
            printf("Dequed, %d.\n", Dequeue());
            break;

        case 3:
            if(IsEmpty())printf("Queue is empty.\n");
            else printf("Que has some values.\n");
            break;

        case 4:
            PrintQueue();
            break;

        case 5:
            system("cls");
            break;

        case 6:
            exit(0);
            break;
        }
    }
}

Most implementations of a Queue use front to point to the head element of the queue and rear to point to the first empty element at the rear of the queue:

front       rear
v            v
[1][2][3][4][ ][ ]

Your implementation has them pointing one element previous.

To change your code to work like this would be a matter of change front/rear to start at 0 and then reversing your increment/read:

void Enqueue(int value)
{
    if(rear<SIZE-1)
    {
        queue[rear] = value;
        ++rear;
    }
    else
    {
        printf("Queue overflow!\n");
    }
}

Review:

Your IsEmpty function has the side effect of resetting the queue. The name does not reflect this; it sounds like it should be merely checking whether the queue is empty. There's not really anything wrong with that. Pragmatically, such an operation should be transparent. However, I'd put a comment in there describing the side effect.

You could also implement a circularly linked queue in which you insert elements in a (conceptual) ring. Basically, when you get to the end of the array, you start inserting at the begining again. However, this means that when front = back the buffer could be full or empty. Just check rear + 1 to see if it's empty.

Oh, and please spell "queue" and its varients correctly...it's really bugging me :)


Code review for python password generator The following code is a simple random password generator that spits out a randomized password based on some input flags.

import string
import random
import argparse

def gen_char(lower, upper, digit, special):

    _lower_letters = string.ascii_lowercase
    _upper_letters = string.ascii_uppercase
    _digits = string.digits
    _special = "!@#$%^&*()"

    _rand = random.SystemRandom()

    _case = ""

    if lower:
        _case += _lower_letters

    if upper:
        _case += _upper_letters

    if digit:
        _case += _digits

    if special:
        _case += _special

    if _case:
        return _rand.choice(_case)

    else:
        return _rand.choice(_lower_letters+_upper_letters+_digits+_special)

def main():

    """

    The following lines of code setup the command line flags that the
    program can accept.

    """

    parser = argparse.ArgumentParser()

    parser.add_argument("length", type=int, help="length of password")
    parser.add_argument("--lower", "-l",
                       help="Generates passwords with lower case letters",
                       action="store_true")
    parser.add_argument("--upper", "-u",
                       help="Generates passwords with upper case letters",
                       action="store_true")
    parser.add_argument("--digit", "-d",
                       help="Generates passwords with digits",
                       action="store_true")
    parser.add_argument("--special", "-s",
                       help="Generates passwords with special characters",
                       action="store_true")

    args = parser.parse_args()

    """

    The following lines of code calls upon the gen_char() function to
    generate the password.

    """
    _pass = ""

    for x in range(args.length):

        _pass += gen_char(args.lower, args.upper, args.digit, args.special)

    print _pass

main()

The code is in working condition. I'm looking for tips in terms of coding styles, readability and perhaps optimizations if any.

If anyone is interested, the code can be found on github.

import string
import random
import argparse

def gen_char(lower, upper, digit, special):

    _lower_letters = string.ascii_lowercase
    _upper_letters = string.ascii_uppercase
    _digits = string.digits
    _special = "!@#$%^&*()"

Its against python convention to prefix local variables with _. Just don't do it. All of these categories would be better in a dictionary. That is create a global dict like:

CHARACTER_CATEGORIES = {
   'lower' : string.ascii_lowercase,
   'upper' : string.ascii_uppercase,
   'digits' : string.digits,
   'special' : "!@#$%^&*()"
}

Then take a list of categories as your parameters to this function instead.

    _rand = random.SystemRandom()

You should create a random number generator once in your program, not once per random choice.

    _case = ""

case? why case?

    if lower:
        _case += _lower_letters

    if upper:
        _case += _upper_letters

    if digit:
        _case += _digits

    if special:
        _case += _special

    if _case:
        return _rand.choice(_case)

    else:
        return _rand.choice(_lower_letters+_upper_letters+_digits+_special)

this is really the wrong place to do this. the decision to include all categories if non are specifically selected is part of interpretating the user's input. So you should do it when you are parsing that, not here.

def main():

    """

    The following lines of code setup the command line flags that the
    program can accept.

    """

Not a very useful comment, because its pretty obvious that's whats happening here.

    parser = argparse.ArgumentParser()

    parser.add_argument("length", type=int, help="length of password")
    parser.add_argument("--lower", "-l",
                       help="Generates passwords with lower case letters",
                       action="store_true")
    parser.add_argument("--upper", "-u",
                       help="Generates passwords with upper case letters",
                       action="store_true")
    parser.add_argument("--digit", "-d",
                       help="Generates passwords with digits",
                       action="store_true")
    parser.add_argument("--special", "-s",
                       help="Generates passwords with special characters",
                       action="store_true")

    args = parser.parse_args()

    """

    The following lines of code calls upon the gen_char() function to
    generate the password.

    """
    _pass = ""

    for x in range(args.length):

        _pass += gen_char(args.lower, args.upper, args.digit, args.special)

Its not very efficient to do it this way as other have pointed out. But its also bad because adding strings in python is expensive, better to put all the letters in a list and join them.

    print _pass

main()

Better to do

if __name__ == '__main__':
   main()

That way you could import this module elsewhere, and use its features.


News-reading application using the Observer pattern I have found this question online from an Amazon interview:

  Design a kind of Kindle Fire application where we can subscribe to a news
  channel and read the news from all publishers as a digital format.

I thought that using the Observer design pattern would be a good solution here.

Here is my implementation:

UnitTest class

[TestClass]
    public class ObserverPatternTest
    {
        [TestMethod]
        public void TestMethod1()
        {
            NewsChannel pub1 = new NewsChannel();
            NewsChannel pub2 = new NewsChannel();

            KindleApplication app = new KindleApplication();
            pub1.Subscribe(app);
            pub2.Subscribe(app);
            string tempStory = "story start";

            pub1.NewStory= tempStory;
            Assert.AreEqual(tempStory, app.Message);
            tempStory = " story b";
            pub1.NewStory = tempStory;
            Assert.AreEqual(tempStory, app.Message);
            tempStory = "story pub2";
            pub2.NewStory = tempStory;
            Assert.AreEqual(tempStory, app.Message);
        }
    }

KindleApplication class (Observer)

  public class KindleApplication : ObserverBase
    {
        public string Message { get; set; }
        public override void Update(string message)
        {
            Message = message;
        }
    }

   public abstract class ObserverBase
    {
       public abstract void Update(string message);
    }

NewsChannel class

 public class NewsChannel : publisher
    {
        private string _newStory;

        // Gets or sets subject state
        public string NewStory
        {
            get { return _newStory; }
            set {
                _newStory = value;
                Notify(_newStory);
            }
        }
    }

Publisher class

 public abstract class publisher
    {
        private List<ObserverBase> _observerList;

        public publisher()
        {
            _observerList = new List<ObserverBase>();
        }
        public void Subscribe(ObserverBase app)
        {
            _observerList.Add(app);
        }
        public void Unsubscribe(ObserverBase app)
        {
            _observerList.Remove(app);
        }
        public void Notify(string message)
        {
            foreach (var obesever in _observerList)
            {
                obesever.Update(message);
            }
        }
    }

I think in general your implementation is fine. You could however use the intefaces that are already provided by .NET:

IObservable<T>

  Defines a provider for push-based notification.

IObserver<T>

  Provides a mechanism for receiving push-based notifications.

You can read more about them in the article about the Observer Design Pattern

In your your application you could use them to replace the Publisher and the ObserverBase.

The KindleApplication would become a IObserver<T>:

public partial class KindleApplication<T>
{
    public T Message { get; set; }

    public void Subscribe(IObservable<T> news)
    {
        _newsUnsubscribers[news] = news.Subscribe(this);
    }

    public void Unsubscribe(IObservable<T> news)
    {
        if (_newsUnsubscribers.TryGetValue(news, out var newsUnsubscriber))
        {
            newsUnsubscriber.Dispose();
            _newsUnsubscribers.Remove(news);
        }
    }
}

and it would implement this interface by storing the observables in a dictionary so that you can unsubscribe again:

public partial class KindleApplication<T> : IObserver<T>
{
    private readonly IDictionary<IObservable<T>, IDisposable> _newsUnsubscribers = new Dictionary<IObservable<T>, IDisposable>();

    public void OnNext(T value)
    {
        Message = value;
    }

    public void OnError(Exception error)
    {
        // ...
    }

    public void OnCompleted()
    {
        // ...
    }
}

The NewsChannel is now an IObservable<T> that notifies all observers about the changes:

public partial class NewsChannel<T>
{
    private T _newStory;

    public T NewStory
    {
        get { return _newStory; }
        set
        {
            _newStory = value;
            Notify();
        }
    }

    private void Notify()
    {
        foreach (var observer in _observers)
        {
            observer.OnNext(_newStory);
        }
    }
}

The new implementation could use an ISet<IObserver<T>> so that you don't subscribe the same channel multiple times and consequently get multiple updates for the same change.

It uses a private class that implements the logic for unsubscribing via the IDisposable interface.

public partial class NewsChannel<T> : IObservable<T>
{
    private ISet<IObserver<T>> _observers = new HashSet<IObserver<T>>();

    public IDisposable Subscribe(IObserver<T> observer)
    {
        _observers.Add(observer);
        return new Unsubscriber(_observers, observer);
    }

    private class Unsubscriber : IDisposable
    {
        private ISet<IObserver<T>> _observers = new HashSet<IObserver<T>>();

        private IObserver<T> _observer;

        public Unsubscriber(ISet<IObserver<T>> observers, IObserver<T> observer)
        {
            _observers = observers;
            _observer = observer;
        }

        public void Dispose()
        {
            _observers.Remove(_observer);
        }
    }
}

If you like you can split each class into multipe partial files to better separate the implementation of the intefaces from the actual data.

As far as the test is concerned it's really hard to follow it. You should split it into multiple tests that possibly verify only a single case. This means you could have such test as:

OnNext_NotSubscribed_DoesNotReciveMessage
OnNext_SubscribedToOneFeed_RecivesSingleMessage
OnNext_SubscribedToTwoFeeds_RecivesMultipleMessages
OnNext_SubscribedAndUnsubscribed_DoesNotReciveMessages
etc.