Text substitution templating function In a recent interview I was asked to solve the below problem.

Problem:

  Given a string with variables in them, e.g.
  "I am a string with {{ variable }} in them"

  Create an output where the {{ variable}} is replaced with their
  actual values.

  So if given the input:

var template = {
  "sentence": "The {{ object }} {{ verb }} ",
  "object": "{{ adjective }} {{ noun }}",
  "verb": "flew",
  "adjective": "colorful",
  "noun": "bird"
};

  the function call

fillInTheBlanks(template, 'sentence')

  must produce the output:

The colorful bird flew

I came up with the following solution (which works):

var sentence = fillInTheBlanks(template, 'sentence');

//Accomplish however you'd like, additional utility functions are fine
function fillInTheBlanks(template, outputKey) {

  var val = template[outputKey];

  while (val.search(/{{/i) >=0) {
      val = splitAndMerge(val);
      val = val.replace(/,/g, " ");
  }

 return val;
}

function splitAndMerge(val) {
   var splittedSentence = val.split(' ');
    _.forEach(splittedSentence, function(item, idx) {
    if (item === '{{') {
      splittedSentence[idx+1] = resolveString(splittedSentence[idx+1]);
      splittedSentence.splice(idx+2, 1);
      splittedSentence.splice(idx, 1);
    }
  });

  return  splittedSentence.toString();

}

function resolveString(item) {
  return template[item];
}

PLNKR: http://plnkr.co/edit/4ciGzVewZRDi4RcSDsfI?p=preview

However, this solution was not considered elegant. what would have been a better way to solve this ?

Generally, using split and treating a string as an array - only to turn it back into a string - isn't very elegant for find-and-replace task like this. If the input is a string, and the output is a string, making the intermediate step an array seems odd given that JS has some OK string manipulation features.

Your code is also buggy. Since you use toString instead of a plain join, you introduce commas in the string. And you then have to remove those again, but you do that with a global replace. Meaning the string can't have commas of its own, or they'll get eaten too.

Instead of all that, you could simply do join(" "), and get the same result as you would with toString() & replace(/,/g, " ") but without stomping on existing commas.

You're also replying on spaces in the string. But really, the only delimiters you should care about are {{ and }}. I should be able to write a template string like "{{ foo }}{{ bar }}" (no space in between) to get a string like StackOverflow. Or I might use hyphens or something else (e.g. the aforementioned commas) and I wouldn't expect the code to eat them. I would also expect to be able to use two spaces inside the braces - or no spaces at all. Neither thing is handled properly by your code, as it's totally dependent on the whitespace being just so.

Lastly, your code would fail on malformed template strings like "here are two braces {{ and here two more {{". That's not a proper template, and should just be returned unaltered. But your code only looks for the opening braces and assumes there to be two close braces later on. So given the input above, I get "here are two braces  two more " as output, because it aggressively splices away some words.

A couple of minor notes:

/{{/i - the i flag (case-insensitive matching) doesn't matter here. There is no upper- or lowercase for curly braces.
"Split" as an adjective is also "split". So "splitted sentence" isn't quite grammatically kosher.
You could combine the splice calls, since splice both removes and inserts elements in an array. I.e.

splittedSentence.splice(idx, 3, resolveString(splittedSentence[idx+1]));

The resolveString() function is problematic. For one, it's a bit much to wrap property access in a function. In fact, calling the function is longer than just writing template[item]. But more importantly the function relies on the template object being accessible in its scope. Note that fillInTheBlanks, the point of the task, does not make such an assumption; there you must pass in the object. Which means you could call fillInTheBlanks("x", { x: "hi!" }) and it should work. But in your case it would fail, because the template object won't be in resolveString()'s scope.
And as @Jonah pointed out, and which I'd missed, you're using underscore (or lodash) just to do a forEach, when forEach is built in. Sure, some really old browsers lack it, but if that's a concern there's always the plain old for-loop.

Anyway, I said you don't need to split the string as there are string manipulation functions you can use instead. Specifically, the String.replace method accepts a function as its 2nd argument. That function will be passed the matched part(s) of the string, and its return value will be used as the replacement. Add in a regular expression, and you've got a pretty neat solution.

So I imagine that your interviewers were looking for something like this perhaps:

var template = {
  "sentence": "The {{ object }} {{ verb }} ",
  "object": "{{ adjective }} {{ noun }}",
  "verb": "flew",
  "adjective": "colorful",
  "noun": "bird"
};

function interpolate(string, values) {
  return string.replace(/\{\{\s*(.*?)\s*\}\}/g, function (_, key) {
    return interpolate(values[key] || "", values);
  });
}

function fillInTheBlanks(template, key) {
  return interpolate(template[key], template);
}

// output (just for the stack snippet)
document.write( fillInTheBlanks(template, 'sentence') );

In this case, fillInTheBlanks is just there to kick things off. The actual work is done in interpolate. That function looks for instances of {{...}} in its input string, and then replaces it with the value for whatever key is between in the braces (ignoring spaces and such). And it calls itself on the resulting string, in order to achieve the recursive replacement behavior.

The name interpolate comes from "string interpolation", the general name for "filling in the blanks" in a template string.

The regular expression can be explained like so:

\{\{          // look for two opening braces "{{"
\s*           // followed by zero or more whitespace characters
(             // start a capture group
  .*?         // match anything that's matched by the following (ungreedy matching)
)             // end capture group
\s*           // match zero or more whitespace characters
\}\}          // match the close braces "}}"

You could skip the \s* parts, and instead match everything between {{ & }} and use trim to remove any leading/trailing whitespace afterward. However, trim isn't always available while the regex should work anywhere.

Anyway, that should match "{{foo}}", "{{  foo}}", "{{foo   }}", etc.. The capture group (group 1) will only match foo within that. The g flag after the regex means "global" - i.e. "replace all", not just the first match.

So when used with replace, the function passed in will receive the entire matched part of the string (e.g. "{{ foo }}") as its 1st argument. We don't care about the entire thing though, so the argument is just named _ in the above to denote we're not using it for anything (not to be confused with underscore/lodash js).

As its 2nd argument, the function receives the contents of the 1st capture group (e.g. "foo"), which is what we're interested in. (Had there been more capture groups, they would've been passed in as the 3rd, 4th, etc. arguments.) So we take that capture group's contents, and treat it as a key/property name, and look it up in the values object. Should the key not actually exist, values[key] will evaluate to undefined, so we handle that by defaulting to an empty string.

In other words, values[key] || "" reads as "get the value for key, or, if it's missing, use an empty string".

And that is then passed through interpolate again, so the string is  built recursively.

The danger with this is that the templating can itself be recursive. If, for instance, you call:

var values = {
  foo: "{{ bar }}",
  bar: "{{ foo }}"
};

interpolate("{{ foo }}", values);

it'll endlessly bounce back and forth, replace one with the other. In practical terms, JavaScript will of course just give up (i.e. raise an exception and stop dead) because the stack level - interpolate calling interpolate ad infinitum - will get too deep.

Your solution would enter an infinite loop instead, as it uses a while loop.

How to deal with such pathological input is outside the scope of this review, but would've been a good question to ask the interviewers. (Personally, I'd just let JS give up; the best you can do is write a bunch more code... and also give up, since there's no good solution. You can be more graceful about it and provide an error message, but that's about it.)

On a side note, the task seems a little weird. Why have fillInTheBlanks take a "raw" key, when the task is supposedly about templating? It'd make more sense to skip straight to interpolate("{{ sentence }}", template), as that's the core of the task. It's also why I chose to expose interpolate in the same scope as fillInTheBlanks - it could just as well be nested inside fillInTheBlanks, but it's far more useful when it's accessible.

I'd also wonder why the object holding the keys and values is called template. That makes no sense and is in fact backwards: The template is the string, and the object holds the values you use to fill in that template. Hence why I called it values in my function.


Backtracking all the words in the English alphabet that have 5 letters, where no 2 consecutive letters can be consonants or vowels I have the following problem in my algorithms textbook:

  Write a computer program that generates and counts how many words of 5 letters of the English alphabet can be formed, with the condition that no 2 consecutive letters can be consonants or vowels.

From the statement, I understand that the output should have this form cvcvc or vcvcv, where c means consonant and v means vowel.

The way I approached this problem was to write a function that generates all the words with 5 letters given a permutation of the sets (since there are 2 available sets, consonants and vowels). There are only 2 permutations of the sets available: cvcvc or vcvcv. And I call the function generate_words for every permutation of the sets.

My question is: Is it a good practice to hardcode the permutation of the sets? Would have been a better solution to write a function that generates this information?

# We use this variable just o track the number of generated solutions.
counter = 1

def _print_solution(set_master, solution):
    result = [set_master[set_index][element]
        for set_index, element in enumerate(solution)]

    global counter
    # ljust(6) because, in our case, the maximum number of words is 21 66 00
    # which has 6 digits.
    print str(counter).ljust(6), ', '.join(map(str, result))
    counter += 1

def _generate_words(set_master, solution, set_counter):
    if set_counter == len(set_master):
        _print_solution(set_master, solution)
        return

    for i in range(0, len(set_master[set_counter])):
        solution.append(i)
        _generate_words(set_master, solution, set_counter + 1)
        solution.pop()

def generate_words(set_master):
    solution = []
    _generate_words(set_master, solution, 0)

if __name__ == "__main__":

    vowels = ['a', 'e', 'i', 'o', 'u']
    consonants = ['b', 'c', 'd', 'f', 'g', 'h', 'j', 'k', 'l', 'm', 'n', 'p',
        'q', 'r', 's', 't', 'v', 'x', 'z']

    generate_words([vowels, consonants, vowels, consonants, vowels])
    generate_words([consonants, vowels, consonants, vowels, consonants])

In Python, strings support the sequence interface, so you can write the vowels and consonants as strings, which are easier to read than lists:

vowels = 'aeiou'
consonants = 'bcdfghjklmnpqrstvxz'

Writing out the consonants explicitly like this is risky: it would be easy to omit one and not notice. (What happened to w and y?) It would be more reliable to construct the consonants by subtracting the vowels from all lowercase letters:

from string import ascii_lowercase
consonants = sorted(set(ascii_lowercase) - set(vowels))

Generating the words is very simple if you use the built-in itertools.chain and itertools.product:

from itertools import chain, product
v, c = vowels, consonants
words = chain(product(v, c, v, c, v), product(c, v, c, v, c))

Counting the words is even simpler. If there are \$v\$ vowels and \$c\$ consonants, then there are \$v^3c^2\$ words of the form VCVCV and \$v^2c^3\$ words of the form CVCVC, for \$v^2c^2(v+c)\$ words altogether:

>>> v, c = len(vowels), len(consonants)
>>> v ** 2 * c ** 2 * (v + c)
286650

which is the same as the number of elements emitted by the generator:

>>> sum(1 for _ in words)
286650

You asked, "Is it a good practice to hardcode the permutation of the sets?" Well, good practice is not some magic book of lore, it's the set of techniques that best meet your requirements. In this case, the goal is to answer your algorithms question, so the requirements include things like correct, clear (and so easily marked) and short.

It's never wrong to think about generalizing your code, but you have to balance the cost of the generalization (in terms of more complex code) against the benefits. If you suspect that in the near future you are going to have to generate and count sets of words under other rules about adjacency of vowels and consonants, then generalizing the code now would be a good idea. But if not, there's no point: you can always generalize it later if you need to.


Optimizing a social network website I am developing my social network website, and I want to know if it's optimized or not.  I am good at ASP.NET 4 and WebForms, but not MVC or JavaScript.

This code is for "liking" a post.  I did not use any JavaScript to do that.  Is this "like" button optimized or not?  How many online users can my server handle (assume max online user is 500-700)?

protected void like_Click(object sender, EventArgs e)
{
    try
    {
        int ID = Convert.ToInt32(Session["ID"].ToString());
        LinkButton like = (LinkButton)sender;
        GridViewRow gvrow = (GridViewRow)like.NamingContainer;
        int ActivityTypeID = Convert.ToInt32(activity.DataKeys[gvrow.RowIndex].Values["ActivityTypeID"].ToString());
        int SourceID = Convert.ToInt32(activity.DataKeys[gvrow.RowIndex].Values["SourceID"].ToString());
        LinkButton unlike = gvrow.FindControl("unlike") as LinkButton;
        Label numberoflike = gvrow.FindControl("numberoflike") as Label;
        like.Visible = false;
        unlike.Visible = true;
        if (ActivityTypeID == 1 || ActivityTypeID == 4)
        {
              //check users liked it before or not ?
              // this code will run a stored procedure
            if (BusinessLayer.StatusLike_Table.GetByIsliked(SourceID, ID, 1) == 0)
            {
              // make an object of like table
                BusinessLayer.StatusLike_Table nl = new BusinessLayer.StatusLike_Table();
                nl.Id = Convert.ToInt32(Session["ID"].ToString());
                nl.Statusid = SourceID;
                nl.Statuslikedate = System.DateTime.Now;
                nl.Save();
                  // saves like to database
                numberoflike.Text = BusinessLayer.StatusLike_Table.GetByNumberOfLike(SourceID).ToString();
             // update number of like by counting all likes for this post
            }

                     }
        else if (ActivityTypeID == 2 || ActivityTypeID == 5)
        {
            if (BusinessLayer.StatusLike_Table.GetByIsliked(SourceID, ID, 2) == 0)
            {
                BusinessLayer.PhotoStsLike_Table nl = new BusinessLayer.PhotoStsLike_Table();
                nl.Id = Convert.ToInt32(Session["ID"].ToString());
                nl.Photostsid = SourceID;
                nl.Photostslikedate = System.DateTime.Now;
                nl.Save();
                numberoflike.Text = BusinessLayer.PhotoStsLike_Table.GetByNumberOFlike(Convert.ToInt32(SourceID)).ToString();
            }
        }
    }
    catch (Exception ee)
    {
    }
}

For the commenting system and posting system and etc, the code is the same as above.  If all the code will be server-side and does not use any JavaScript, does it cause to have a slow website or not?

"Likes" are store on a database like this:

likeID int identity specification true ! identity increment true and 1
PostID int
ID int
Date Datetime

I don't use any caching and I want to optimize user experience.

Not your primary concern, but a number of issues are jumping at me; I'll leave the performance aspects to other reviewers.

  Naming

like_Click should be renamed OnLikeButtonClicked or something similar, because it's a method, and method names in C# should be PascalCase (without underscores).

int ID should be renamed id, because it's a local variable, and local variables in C# should be camelCase.

LinkButton like should be renamed likeButton to not look like it's an action or something.

Notice how the syntax formatting gets confused with int ActivityTypeID and int SourceID? That's because PascalCase is for type and method names; these are local variables.

Label numberoflike is alllowercase and that's not helping readability. I'd call it something like likesCountLabel, indicating it's a control and that it's something about "likes count". Again, camelCase.

  Readability

Naming aside, a number of things could be done to improve readability here.

if (ActivityTypeID == 1 || ActivityTypeID == 4)

These values are known as magic numbers - nobody knows what they stand for, they just work. Consider defining an enum ActivityType instead, and refer to the values by their names.

Remove comments that state the obvious, that essentially rephrase what the code is already saying, and those that can eventually (read: will!) turn into blatant lies:

// this code will run a stored procedure

Really? It's an implementation detail of that static/nested class (ew!), that this method doesn't need to care about. Whether it calls a stored procedure, concatenates T-SQL into a string, calls a web service and parses Json or XML to get your results, has no importance whatsoever. This comment is doomed, remove it.

// make an object of like table

Not needed. var myVariable = new MyType() already says everything we need to know.

That one is confusing:

    nl.Save();
      // saves like to database
    numberoflike.Text = BusinessLayer.StatusLike_Table.GetByNumberOfLike(SourceID).ToString();

Either put comments above the line they're commenting, or at the end of it... but not below! It makes it look like numberoflike.Text = ... is going to save something to the database. Yeah, right.

Avoid disemvoweling identifiers. nl means sweet nothing to anyone reading this code, and gvrow would probably be better off as simply row - the fact that it's a GridView isn't really important.

  Gotta catch'em alltm

This is called Pokemon exception handling:

catch (Exception ee)
{
}

Why? Because regardless of what's being thrown, it wil be caught. And then shoved under the carpet without leaving any trace. Not good: you should always catch the most specific exception types, those you can reasonably expect to be thrown, and properly handle them. Exceptions you didn't see coming, or that you can't properly handle, should bubble up the stack, get logged, and perhaps crash the application.

OTOH, you're getting a compiler warning, saying ee is declared but not used. You can eliminate that warning by going all-out on Pokemon handling:

catch (Exception){ }

The BusinessLayer static class smells, but changing it to be a constructor-injected instance would probably require massive structural changes.. I'd approach the refactoring with incremental steps towards dependency-injection, so as to facilitate unit-testing.


Find common keys in at least two dicts I'd like to obtain a dict of the following form (not strict):

{65: ['Fresh', 'Frozen'],
 66: ['Fresh', 'Delicatessen'],
 75: ['Grocery', 'Detergents_Paper'],
 128: ['Fresh', 'Delicatessen'],
 154: ['Milk', 'Grocery', 'Delicatessen']}

when I have an input which is list of dicts. Their keys may be intersected and values - not. For example in our case we have the same key 65 in two dicts with value Fresh and Frozen.

This is my solution and I'd like to improve it:

outliers = [
    {65: 'Fresh', 66: 'Fresh', 81: 'Fresh', 95: 'Fresh', 96: 'Fresh',
     128: 'Fresh', 171: 'Fresh', 193: 'Fresh', 218: 'Fresh', 304: 'Fresh',
     305: 'Fresh', 338: 'Fresh', 353: 'Fresh', 355: 'Fresh', 357: 'Fresh',
     412: 'Fresh'},

    {86: 'Milk', 98: 'Milk', 154: 'Milk', 356: 'Milk'},

    {75: 'Grocery', 154: 'Grocery'},

    {38: 'Frozen', 57: 'Frozen', 65: 'Frozen', 145: 'Frozen', 175: 'Frozen',

     264: 'Frozen', 325: 'Frozen', 420: 'Frozen', 429: 'Frozen', 439: 'Frozen'},

    {75: 'Detergents_Paper', 161: 'Detergents_Paper'},

    {66: 'Delicatessen', 109: 'Delicatessen', 128: 'Delicatessen',
     137: 'Delicatessen', 142: 'Delicatessen', 154: 'Delicatessen',
     183: 'Delicatessen', 184: 'Delicatessen', 187: 'Delicatessen',
     203: 'Delicatessen', 233: 'Delicatessen', 285: 'Delicatessen',
     289: 'Delicatessen', 343: 'Delicatessen'}
]

common_outliers = {}
for outlier in outliers:
    for idx, feature in outlier.items():
        if idx not in common_outliers:
            common_outliers[idx] = [feature]
        else:
            common_outliers[idx].append(feature)
common_outliers = {idx: features for idx, features in common_outliers.items()
                   if len(features) > 1}
print(common_outliers)

This implementation is fine. It's easy to read and it works well.

A minor optimization could be to use an auxiliary dictionary to track the keys seen so far, in order to avoid an extra iteration to create the final result. But I don't think this is really important.

seen = {}
common_outliers = {}
for outlier in outliers:
    for idx, feature in outlier.items():
        if idx not in seen:
            seen[idx] = [feature]
        else:
            seen[idx].append(feature)
            common_outliers[idx] = seen[idx]
print(common_outliers)


Iterate through multiple elements based on their id in jQuery Haven't worked too much with jQuery and I had to get the values from different elements (they have the ids 0, 1 and 2) in order to compute the sum of them and update a field with that value.
The code works well but I was wondering if it is possible to write less code and use something like a for loop in this case.
Here is the code:
function total() {
  var subtotal0 = parseFloat(
    $('#0').find('.cart-subtotal').text().replace('€', '')
  );

  var subtotal1 = parseFloat(
    $('#1').find('.cart-subtotal').text().replace('€', '')
  );

  var subtotal2 = parseFloat(
    $('#2').find('.cart-subtotal').text().replace('€', '')
  );

  var total = subtotal0 + subtotal1 + subtotal2;

  $('.cart-total-price').text(total + '€');
}

Is it possible to make it better?

Instead of IDs (like #0), use classes instead. (Numeric-indexed IDs are quite a code smell anyway.) Or, if you can't remove the IDs entirely, just add classes to those elements. For example, adding the class of subtotal-container to each of those elements with IDs.
To make things clearer, I'd also recommend calling the function something like calculateTotal instead of total - it's more precise and also avoids possibly-confusing shadowing:
function calculateTotal() {
  const total = [...$('.subtotal-container .cart-subtotal')]
    .map(subtotalElm =>
  Number(subtotalElm.textContent.replace('€', '')))
    .reduce((a, b) => a + b, 0);
  $('.cart-total-price').text(total + '€');
}

Though, at this point, you may as well remove the dependency on jQuery entirely if you wished, it's not really accomplishing anything useful:
const total = [...document.querySelectorAll('.subtotal-container .cart-subtotal')]
  .map(subtotalElm => Number(subtotalElm.textContent.replace('€', '')))
  .reduce((a, b) => a + b, 0);
document.querySelector('.cart-total-price').textContent = total + '€';

With jQuery, keep in mind that
$('someSelector').find('someOtherSelector')

is equivalent to
$('someSelector someOtherSelector')

via the descendant selector.


Limited typecasting with regex I'm learning Python, and found a fun little video on YouTube called "Learn Python through public data hacking".  In essence it uses the CTA unofficial API to do some parsing of XML data.  In taking things further than inline code, I'm chunking behaviors into modules and classes to further understand how Python works.

The XML from the API looks something like this:

<buses rt="22">
<time>1:40 AM</time>
    <bus>
        <id>4194</id>
        <rt>22</rt>
        <d>East Bound</d>

Northbound

        <dn>E</dn>
        <lat>41.88327006970422</lat>
        <lon>-87.62828115689553</lon>
        <pid>5421</pid>
        <pd>Northbound</pd>
        <run>P238</run>
        <fs>Howard</fs>
        <op>49875</op>
        <dip>5314</dip>
        <bid>7323287</bid>
        <wid1>0P</wid1>
        <wid2>238</wid2>
    </bus>

</buses>

I'm using the python-requests module for the HTTP side, and once the XML is downloaded, I parse each <bus> node into a Bus class, assign each of the child nodes into a dictionary, and make that visible with a get(prop) statement, so I can just call bus.get('lat') to retrieve the latitude, etc.  However, in order to do the correct calculations on it (i.e. arithmetic) each node's value needs to be returned as the correct type.  By default, they're all read as strings.

Considering that Python doesn't have a "switch" statement like most other languages, someone at SO said to use a dictionary. Is this the/a correct way of doing something like this? Or is there some nifty builtin that I don't know of?

def dyncast(value):
    _type = type(value)
    types = {
        "FloatType": (r'^[\d]{2}\.[\d]+$', lambda f : float(f)),
        "IntType" : (r'^[\d]+$', lambda i : int(i)),
        "StrType" : (r'^[a-zA-z]+$', lambda s : str(s))
    }
    for typeval in types:
        pattern = types[typeval][0]
        fn = types[typeval][1]
        match = re.match(pattern, value)

        # if it matches a regex and has a group(), return the
        # lambda calculation (typecast)
        if match and match.group():
            return fn(value)

    # return straight up if no matches
    return value

# Called via:
for elm in node:
    self._data[elm.tag] = dyncast(elm.text)
# where node is the bus node, and elm are the child nodes

It seems to work well, as I get returns such as:

# print bus.get('fs'), type(bus.get('fs'))
Harrison <type 'str'>

# print bus.get('lat'), type(bus.get('lat'))
41.9027030677 <type 'float'>

Makes sense to me. A couple small things:

lambda f: float(f) should be equivalent to just float if I'm not mistaken. You can simplify the loop a bit too since you're not actually using the dictionary keys.

types = [
  (r'(regex)', float),
  (r'(regex)', int),
  (r'(regex)', str),
]
for pattern, fn in types:
  match = ...


Preorder traversal of binary tree to produce formatted string Given a complete binary tree returns the following format (Parent ( leftchild (leftchild, rightchild), rightchild(leftchild,rightchild) ). Looking for code review, optimizations and best practices.

public final class PreOrderList {

    private TreeNode root;

    /**
     * Constructs a binary tree in order of elements in an array.
     * The input list is treated as  BFS representation of the list.
     * Note that it is the clients responsibility to not modify input list in objects lifetime.
     *
     */
    PreOrderList(List<String> items) {
        create (items);
    }

    private static class TreeNode {
        TreeNode left;
        String item;
        TreeNode right;

        TreeNode(TreeNode left, String item, TreeNode right) {
            this.left = left;
            this.item = item;
            this.right = right;
        }
    }

    private void create (List<String> items) {
        root = new TreeNode(null, items.get(0), null);

        final Queue<TreeNode> queue = new LinkedList<TreeNode>();
        queue.add(root);

        final int half = items.size() / 2;

        for (int i = 0; i < half; i++) {
            if (items.get(i) != null) {
                final TreeNode current = queue.poll();
                final int left = 2 * i + 1;
                final int right = 2 * i + 2;  // http://stackoverflow.com/questions/21156774/unit-testing-complicated-methods-what-to-test-and-what-to-assume

                if (items.get(left) != null) {
                    current.left = new TreeNode(null, items.get(left), null);
                    queue.add(current.left);
                }
                if (right < items.size() && items.get(right) != null) {
                    current.right = new TreeNode(null, items.get(right), null);
                    queue.add(current.right);
                }
            }
        }
    }

    /**
     * Returns the string representation of the format
     * (Parent ( leftchild (leftchild, rightchild), rightchild(leftchild,rightchild) )
     * It needs a complete balanced tree, else invalid format is be returned.
     *
     * @return  the String representation.
     */
    public String getPreorderList() {
        if (root == null) {
            throw new NoSuchElementException("The element cannot be null.");
        }

        final StringBuilder sb = new StringBuilder();
        sb.append("(");
        sb.append(root.item);
        preOrder(root, sb);
        sb.append(")");

        return sb.toString();
    }

    private void preOrder(TreeNode node, StringBuilder sb) {
            if (node.left != null) {
                sb.append("(");
                sb = sb.append(node.left.item);
                preOrder(node.left, sb);
            }

            if (node.right != null) {
                sb.append(",");
                sb = sb.append(node.right.item + "");
                preOrder(node.right, sb);
                sb.append(")");
            }
    }

    public static void main(String[] args) {
        PreOrderList pol = new PreOrderList(new ArrayList<String>(Arrays.asList("1", "2", "3", "4", "5", "6", "7")));
        assertEquals("(1(2(4,5),3(6,7)))", pol.getPreorderList());
    }
}

I haven't fully parsed your code, but I would definitely add a check before creating the root. This get(0) would break on an empty list, and would break on a null list. Or you could throw an IllegalArgumentException as well.


Finding the closest pair of numbers which add to a target number This function takes two lists of same length and a target number: the number which is to be summed. The function then returns a list of pairs whose sum is closest to the target number than any other pairs. It also finds the pair whose sum is equal to the target number.

def pair_finder(list1, list2, t):
    list1.sort()
    list2.sort()
    list1.reverse()
    t_low = list1[-1] + list2[0]
    t_high = list1[0] + list2[-1]
    pairs_low = []
    pairs_high = []
    pairs_equal = []
    for i in list1:
        k = 0
        for j in list2:
            if i + j < t:
                if i + j > t_low:
                    pairs_low.clear()
                    t_low = i + j
                    pairs_low.append([i, j])
                elif (i + j == t_low):
                    pairs_low.append([i, j])
            if i + j > t:
                if i + j < t_high:
                    pairs_high.clear()
                    t_high = i + j
                    pairs_high.append([i, j])
                    list2 = list2[k:]
                    break
                elif i + j == t_high:
                    pairs_high.append([i, j])
            if i + j == t:
                pairs_equal.append([i, j])
            k += 1
    pairs = []
    for q in pairs_low:
        pairs.append(q)
    for w in pairs_high:
        pairs.append(w)
    for r in pairs_equal:
        pairs.append(r)
    return pairs

while True:
    try:
        l1 = []
        l2 = []
        li = input("enter the first sequence of numbers by giving exactly one space between numbers:")
        for i in li.split(" "):
            l1.append(int(i))
        lj = input("enter the second sequence of numbers by giving exactly one space between numbers:")
        for j in lj.split(" "):
            l2.append(int(j))
        if len(l1) == len(l2):
            target = int(input("enter the target number: "))
            break
        else:
            print("the length of both sequences should be same!")
            l1 = []
            l2 = []
    except ValueError:
        print("only integer type values are allowed!")
for pair in pair_finder(l1, l2, target):
    print(pair)

Example:
Let list 1 be 2 98 63 41 25 27 -51 48 18 54 31 28 55 11  and list 2 be 21 56 87 65 21 12 75 41 33 91 32 15 8 -35 and target number be 43 hence the pairs (15, 27),(33, 11),(41, 2),(32, 11),(15, 28),(12, 31)

Human Computer Interaction

Prompts

        li = input("enter the first sequence of numbers by giving exactly one space between numbers:")
        lj = input("enter the second sequence of numbers by giving exactly one space between numbers:")

These are awfully long prompts for the user to enter in a list of values after.  Maybe instructions, and then two short input prompts?

        print("Enter two space separated integer number sequences, of the same length.")
        li = input("First sequence:  ")
        lj = input("Second sequence: ")

White-space separators

li.split(" ") will separate a string into a number of items separated by exactly one space.  If the user wants to enter a single digit number on one line, and a two digit number on the next, and keep the columns of numbers lined up, they can't use extra spaces.

.split(), with no arguments, will split a string into a number of items separated by one or more white-space characters.

>>> "     2     3 \t   \n  4 \r  5     ".split()
['2', '3', '4', '5']

No hard requirements to use exactly one space.  And leading and trailing spaces are trimmed, too!

Unexpected Behaviour

def pair_finder(list1, list2, t):
    list1.sort()
    list2.sort()
    ...

After calling pair_finder(l1, l2, target), you'll find that l1 and l2 have been sorted!  The caller probably did not expect that.

Use:

def pair_finder(list1, list2, t):
    list1 = sorted(list1)
    list2 = sorted(list2)
    ...

The sorted(...) function doesn't modify the input list, and it returns a new list.  By assigning those to the original variables, list1 & list2 will be sorted, but the caller's l1 & l2 lists won't be touched.

Pythonic Constructs

Loop like a native

List comprehension

l1 = []
for i in li.split(" "):
    l1.append(int(i))

This is an inefficient construct.  You are creating a list, and then expanding the list one item at a time.

You could create the list all at once, using list comprehension:

l1 = [int(i) for i in li.split(" ")]

And applying the same operation to every item in a sequence is called a mapping operation, and Python has a built-in map(func, sequence) function:

l1 = list(map(int, li.split(" ")))

Or, combining with the input and using the better space handling:

print("Enter two space separated integer number sequences, of the same length.")
l1 = list(map(int, input("First sequence:  ").split()))
l2 = list(map(int, input("Second sequence: ").split()))

Enumerate

        k = 0
        for j in list2:
            ...
            k += 1

This should be replaced with:

        for k, j in enumerate(list2):
            ...

to allow Python to maintain the k index while walking through the list2 items.

Extending Lists

Adding a list to another list is a list.extend(...) operation:

pairs = []
for q in pairs_low:
    pairs.append(q)
for w in pairs_high:
    pairs.append(w)
for r in pairs_equal:
    pairs.append(r)

Could become simply:

pairs = []
pairs.extend(pairs_low)
pairs.extend(pairs_high)
pairs.extend(pairs_equal)

if ... elif

        if i + j < t:
            ...
        if i + j > t:
            ...
        if i + j == t:
            ...

If the sum is less than t, it won't be greater than t, or equal to t.  And if it is greater than t, it won't be equal to t.  And if it is not less than or greater than t, it can only be equal to t.  Why do the extra comparisons?

        if i + j < t:
            ...
        elif i + j > t:
            ...
        else:
            ...

PEP-8

Unnecessary parenthesis:

            elif (i + j == t_low):

Variable names too short to be meaningful

li, lj, l1, l1, q, w, r, t

Main guard

Mainline code should be protected with

if __name__ == '__main__':
    ...

to allow the file to be imported into another file, for unit tests, etc.

Algorithmic Improvements

pair_finder() starts off by sorting both input lists.  That is an \$O(N \log N)\$ operation.  Then it reverses one of the lists, which is an \$O(N)\$ operation.  And then ...

for i in list1:
    ...
    for j in list2:
        ...

... which is \$O(N^2)\$!  Right now, this is the time consuming part of the algorithm.  But what are we doing?  We are looking for two numbers which sum to target.  Let’s turn that around:

for i in list1:
    desired = target - i
    # find “desired” in list2

Well, list2 is sorted, so we can do a binary search to find the desired value.

for i in list1:
    desired = target - i
    pos = bisect.bisect_left(list2, desired)
    ...

The binary search is \$O(\log N)\$, so with the outer loop, the time complexity has dropped to \$O(N \log N)\$, the same as the sorting.

The desired value may or may not be in list2.  If it is, it is at list2[pos].  Assuming we don’t fall off the start of list2, then for the current i value, i+list2[pos-1] would be the largest sum less than target.

Assuming we don’t fall off the end of list2, if list2[pos] == desired, then the sum i+list2[pos+1] will be the smallest sum greater than target for the current value of i.

for i in list1:
    desired = target - i
    pos = bisect.bisect_left(list2, desired)
    if pos < len(list2):
        if list2[pos] == desired:
            # add (i, desired) to pairs_equal
            low, high = pos - 1, pos + 1
        else:
            low, high = pos - 1, pos
        if low >= 0:
            # add (i, list2[low]) to pairs_low, if >= t_low
        if high < len(list2):
            # add (i, list2[high]) to pairs_high, if <= t_high

But ... what about duplicates?  If list2 contains duplicate values, pos + 1 may not be sufficient to advanced to a value larger than desired.  You could use both bisect_left and bisect_right to find either end of a sequence of multiple desired values.  The difference in right and left would be the count of those values, and you could add [(i, desired)] * count to pairs_equal.  But you’d also need to do the same for the pairs_low and pairs_high, which means more binary searching to find those ends.  Or, you could use two collections.Counter objects to count occurrences of each value in list1 and list2, and then remove duplicates values from list1 and list2.  Any pairs added would need to be replicated by the product of the respective counts to occur the correct number of times in the result.


Usertool For Website Admins -- Follow up A while ago, I posted about creating a user tool for forum admins that allows them to add, edit, and delete users from their desktops. This still is not complete, posting for review on tool so far

I've finished with the delete_user method and also added a couple new features to it, what this code does, is take your input, and save it to a YAML file ( will be converting YAML to JSON ). It's advantages are that you don't have to go directly to your database until you actually need to uplaod the file, so you can create an entire list of users from your desktop without sending one validation e-mail, or having to validate one user.

#!/usr/bin/env ruby

require 'yaml'

def menu
  print "Welcome to User Menu. Would you like to [A]dd users, [D]elete users, [E]dit users, [V]iew all users, or [Q]uit the system?\n"
  input = gets.chomp!
  case input
  when /a/i
    add_user
  when /d/i
    delete_user
  when /e/i
    edit_user
  when /v/i
    view_all
  else
    puts "Exiting system.."
    exit
  end
end

def load_hash
  File.exist?('users.yml') ? YAML.load_file('users.yml') : {users: {}}
end

def save_hash(hash)
  File.open('users.yml', 'w') { |s| s.write(hash.to_yaml) }
end

def add_user
    hash = load_hash
    hash[:users][:"#{prompt('Enter username:')}"] =
      {email_address: prompt("Enter email:"),
       member_status: prompt("Enter member status:")}
    save_hash(hash)
    add_user if restart
end

def delete_user
    hash = load_hash
    to_delete = prompt("Enter username:").to_sym
    if hash[:users].has_key?(to_delete)
        hash[:users].delete(to_delete)
    else
        puts 'User not found'
    end
    save_hash(hash)
    delete_user if restart
end

def edit_user
  edit_user if restart
end

def view_all
  puts load_hash
end

def restart
  puts "Would you like to edit another user?"
  input = gets.chomp
  if input =~ /yes/i
    return true
  else
    puts "Exiting program.."
    exit
  end
end

def prompt ( message )
  puts message
  gets.chomp
end
menu

Overview of the YAML file:

---
:username: TEST
:email_address: TEST
:member_status: TEST

Is there anything here that I can do better? Better syntax, combine methods, edit something? I'm looking for every possible example.

Bug

Adding a user with a username that is already taken overwrites the existing user.

Adding a user with a blank username is allowed. (Maybe you should allow alphanumeric characters only, and set a minimum required length)

Unbounded recursion

def add_user
    # ...
    add_user if restart
end

The recursion you're using for repeating this function can grow arbitrarily deep, causing a stack overflow error. Use a loop instead:

def add_user
  loop do
    # ...
    break unless restart
  end
end

Then, restart doesn't need to harshly exit the whole program. It can simply return a boolean:

def restart
  puts "Would you like to edit another user?"
  input = gets.chomp
  return input =~ /yes/i
end

(You can print "Exiting program" at the end of the program, if you want)

Also, don't exit in menu. It's not needed.

Nitpicking

print "...?\n"

Use puts instead of using a newline in the string.

if hash[:users].has_key?(to_delete)
  hash[:users].delete(to_delete)
else
  puts 'User not found'
end

Hash#delete returns the deleted value, or nil if the key is not found. So you can use its return value for the condition:

user = hash[:users].delete(to_delete)
unless user
  puts 'User not found'
end

Also, rename to_delete to username.

puts "Would you like to edit another user?"

This message appears for every action, not just editing. Pass a string argument to specify the action (e.g. "add" or "edit") and use it when printing the message. Also, rename restart to again? to be more accurate.

Creating a class

Most methods operate on the same data structure: the users hash. So it makes sense to put them all inside a class which stores the hash as a field. As a bonus, you won't need to reread the users file every time you perform an action.

class UsersList
  def add(username, email_address, member_status)
    file[:users][username.to_sym] = {
      email_address: email_address,
      member_status: member_status
    }
  end

  def remove(username)
    file[:users].delete(username)
  end

  def save
    save_hash
  end

  def to_s # for viewing
    file.to_s
  end

  private

    def file
      # Return @hash, which stores the hash
      # If it's not loaded yet (@hash is nil), initialize it with load_hash first
      @hash ||= load_hash
    end

    def load_hash
      File.exist?('users.yml') ? YAML.load_file('users.yml') : {users: {}}
    end

    def save_hash
      File.open('users.yml', 'w') { |s| s.write(@hash.to_yaml) }
    end
end

Notice all methods for accessing the hash use the private method file. (I named it file because hash is an existing method of all objects). file returns the hash, but only loads it once from the file, when it's first needed.

Note the methods only deal with the logic of the operations, not with interacting with the user, in order to seperate these two concerns.

To use this class, you'll need to make an instance of it:

$users_list = UsersList.new # global variable for handling the users list

Then change add_user, delete_user and view_all to use it:

def add_user
  loop do
    username = prompt('Enter username: ')
    email_address = prompt('Enter email:')
    member_status = prompt('Enter member status:')
    $users_list.add(username, email_address, member_status)
    break unless again?('add')
  end
end

def delete_user
  loop do
    username = prompt("Enter username:").to_sym
    unless $users_list.remove(username)
      puts 'User not found'
    end
    break unless again?('delete')
  end
end

def view_all
  puts $users_list.to_s
end

Finally, at the end of your program, save the users list:

$users_list.save


Using TAGS with  on your websites This function creates a series of links at a desired destination by the usage of a premade tag system.

Usage:

createMyTAGS_DATA passes in a single line of all the links as: LinkText;LinkAddress;;
createMyTAGS_LOCATION passes the destination to a div,span,... to render the result

Result would be:

LinkText1LinkText2 [...]

Note: each of tag(LinkText;LinkAddress;;) must have more than 10 characters.

click here

From a once over;

Indent your code, it will be easier to read
Use lowerCamelCase for your names createMyTAGS -> createMyTags, though personally I would call the function createLinks

I would call createMyTAGS_DATA simply s for string or data
I would call createMyTAGS_LOCATION either id or element or elementID, you wont then feel the need to assign that parameter to the slightly better named linkTags

For your string handling, you should read up on .split(), it would make this so much easier
alert statements are terrible, this function should never go wrong. Consider using console.log instead if you must.
If you have long variables or chained variables (which you should avoid), consider +=:
document.getElementById(v1).innerHTML = document.getElementById(v1).innerHTML + linkNew;
becomes
document.getElementById(v1).innerHTML += linkNew;
Consider using addEventListener instead of assigning the event listener straight into the HTML

My counter proposal:

function createLinks(data, elementId){

    var element = document.getElementById(elementId),
        links = data.split(';;'),
        html = element.innerHTML,
        linkParts, url, text;

    links.forEach( function( link ){
        linkParts = link.split(';');
        text = linkParts[0];
        url = linkParts[1];
        html += ''+text+' ';
    });

    element.innerHTML = html;
}
click here


Finding the closest data point in a sequential list Given a List<double> and a double, it will find the closest value and return the index of the closest value in the list. The list will always be in order.

private int FindClosestPoint(double val, List<Double> list)
{
    int ret = new int();

    int max = list.Count;
    int min = 0;
    int index = max / 2;

    while (max - min > 1)
    {
        if (val < list[index])
            max = index;
        else if (val > list[index])
            min = index;
        else
            return index;

        index = (max - min) / 2 + min;
    }

    if (max != list.Count && min != 0)
    {
        if (Math.Abs(list[max] - val) < Math.Abs(list[min] - val))
            ret = max;
        else
            ret = min;
    }
    return ret;
}

Bug

Suppose list[0] = 0, list[1] = 1, and val = 2.  After doing the binary search, you are left with min = 1 and max = 2.

When we get here:

if (max != list.Count && min != 0)

the block is skipped because max == list.Count.  This is correct because you don't want to deference list[max].  However, you end up returning ret which was never set and still contains 0.  The correct index to return would be 1.

I believe you can fix your function by eliminating the ret variable and returning min instead.  In other words:

private int FindClosestPoint(double val, List<Double> list)
{
    int max = list.Count;
    int min = 0;
    int index = max / 2;

    while (max - min > 1)
    {
        if (val < list[index])
            max = index;
        else if (val > list[index])
            min = index;
        else
            return index;

        index = (max - min) / 2 + min;
    }

    if (max != list.Count &&
            Math.Abs(list[max] - val) < Math.Abs(list[min] - val))
    {
        return max;
    }
    return min;
}

Strange way to initialize an int

Also I noticed this strange line:

int ret = new int();

I'm not sure why you didn't just initialize ret the same way as you did with min:

int ret = 0;

The two statements are equivalent but since int is a value type, the former statement is odd because it makes it look like you are trying to allocate an object.


A simple program with trains and inheritance Here it is my program. I'm especially worried about the main because I know that one must be careful while instancing vectors of "inherited" objects. The code I have works fine (as far as I can see) but I saw the original program made by my teacher and his implementation of the vector in the main uses new and iterator. I'd like to have some piece of advice on how to improve!

#include <vector>
#include <iostream>
#include <cmath>

class Station {
private:
    double x_, y_;

public:
    Station(double x, double y) : x_(x), y_(y) { }
    Station(const Station & s) : x_(s.x_), y_(s.y_) { }

    // dichiarazione funzione globale
    friend double dist(const Station & s, const Station & t);
};

class Train {
private:
    double vm_;
public:
    Train(double vm) : vm_(vm) { }
    double getvm() {return vm_; }

    virtual double time_of_travel(const Station & s, const Station & t) {
        return( dist(s, t)/vm_);
    }
};

class Regionale : public Train {
public:
    Regionale() : Train(60.) { }
};

class Intercity : public Train {
private:
    Station stop_;
public:
    Intercity(const Station stop) : stop_(stop), Train(110.) { }

    double time_of_travel(const Station & s, const Station & t) {
        return( (dist(s, stop_) + dist(stop_, t))/this->getvm());
    }

};

double dist(const Station & s, const Station & t) {
    return(sqrt(pow(s.x_ - t.x_, 2) + pow(s.y_ - t.y_, 2)));
}

int main() {
    Station Milano (0. ,0.);
    Station Bergamo (35. ,25.);
    Station Piacenza (40. , -45.);

    std::cout << dist (Milano, Bergamo) << std::endl;
    std::cout << dist (Bergamo, Piacenza) << std::endl;
    std::cout << dist (Piacenza, Milano) << std::endl;

    Intercity i(Milano);
    Regionale r;
    std::vector<Train * > v = {&i, &r};

    for(auto elem : v) std::cout << elem->time_of_travel(Bergamo, Piacenza) << std::endl;

}

Coal_ already made some good points. Let's just elaborate on the one about private.
A class makes everything in it private by default that is why you don't need it if you order your interface this way. However it's recommended to order your interface from public to private so people who read it can see which methods you expose without having to read through all the private data first.
If you are using a trailing underscore because you're worried about problems when assigning arguments to members then there is no need for that. This is because it's guaranteed to work even if the names are the same. Just make sure to be careful in the constructor body if you do this!
Your variable names could be better. In general you should avoid single letter names and choose a descriptive name.
Some parts are probably easier to read if you put a few more spaces inbetween operators. Indenting could also be improved for example in your ctors:

Foo()
    : bar(0)
    , baz(1)

is a lot more readable than putting everything in a single line.
Some of your parenthese in the return statements are superfluous and can be removed.
In C++ it's generally recommended to treat & as part of the type and therefore write foo& instead of foo &.
Since you included <cmath> you should probably prefix pow and sqrt with std::.
You seem to use double solely because of your math functions. Perhaps strategic casting would be better instead of using a type you don't fully make use of.
When using the ranged-for loop it's recommended to use this form:

for (auto const& value : container) {
    // ...
}

The const makes it clear you don't intent to modify things in this loop and passing by reference avoids unecessary copies.
If you run your compiler with warnings (as you should! For example to discover some potential problems you can use an online compiler like Clang with -Weverything. For production code you want to pick your warning levels more carefully though) you will notice that there are some issues.
You have a wrong initialization order and are missing a virtual destructor. Order can be important when you depend on something in the base class.
The way you handle your objects in main is certainly interesting but this seems like a good place for smart[1] pointers[2] and their respective make_unique/shared functions.


Function fputs always return zero on success I am using the fputs() function to write a string to a file. I want to handle the case in which the string has been successfully written to the file. Here is my code:

#include <stdio.h>

main() {

  FILE *fp;
  int rets; //to keep fputs return value

   fp = fopen("test.txt", "w");
   rets = fputs("\nThis is testing for fputs...\n", fp);
   printf("Fputs return value : %d\n",rets); //Return value is always zero
   fclose(fp);

   if(rets >= 0 )
       printf("Successfully written on the file. Proceed..");

   return 0;
}

According to the documentation for fputs():

  On success, a non-negative value is returned.

When I executed this code multiple times, I noticed that the return value is always zero. According to this observation, it is enough to check if return value is zero (rather than non-negative). According to this, the last line of my code could be:

   if(rets == 0 ) //equal to zero for success
       printf("Successfully written on the file. Proceed..");

Should I trust the above observation?

Is the documentation wrong, or ambiguous?

Although your observations show that you happen to always get 0 as a (success-case) return value, implementations are permitted to return any value of 0 or more.  For example, a different implementation of the standard library may find it convenient to return the number of characters that have been printed.

You cannot assume that rets == 0 will be as good as rets >= 0, because:

You might want to run your code on a different system, with a different standard library.  This might happen if you share your code with somebody else (perhaps for them to help you fix a bug).
The different system might be your current system in a year's time, when you've upgraded your libraries.

If your assumption is no longer true, then you suddenly have a huge number of hard-to-find bugs that all need fixing at once.  That is not what we call a Good Thing.  By depending only on what the standard guarantees, you maximise your likelihood of full portability, including to future versions of your current environment.


Find the cheapest order placed out of all of the stores visited - follow-up #2 Made some adjustments to the previous code thanks to the helpful and thoughtful review by @G.Sliepen.

Problem statement: This  program should  ask  for  the  total  number  of  shops  that  will  be  visited.   At  each  shop,  ask  for  the  number of  ingredients  that  need  to  be purchases.   For  each  ingredient,  ask  for  the  price.    Keep  track  of the  total for  the  order  so  that  you  can  write  it  down  before  leaving  the  shop.   This  program should  also  track  with  order  was  the  cheapest  and  which  shop  the  cheapest  order  was  at.

Original code: Find the cheapest order placed out of all of the stores visited

1st adjustments to the original code: Adjustments based on reviews #1; Find the cheapest order placed out of all of the stores visited

Any  and all hints, tips, tricks, advice, suggestions are greatly appreciated!

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

int read_positive_int(const char* prompt)
{
   printf("%s", prompt);

    int n;
    while (true)
    {
        if (scanf("%d", &n) != 1)
        {
            fprintf(stderr, "Invalid input!\n");
            return 1;
        }
    return n;
    }
}

float read_real_positive(const char* prompt)
{
    printf("%s", prompt);

    float m;
    while (true)
    {
        if (scanf("%f", &m) != 1)
        {
            fprintf(stderr, "Invalid input!\n");
            return 1;
        }
    return m;
    }
}
int main(void)
{
    int num_shops = read_positive_int("How many shops will be visited? ");

    float total_cost[num_shops];

    float cheapest_order;
    int cheapest_shop = 1;

    for (int i = 0; i < num_shops; i++)
    {
        printf("You are at shop #%d.\n", i+1);
        int num_ingredients = read_positive_int("How many ingredients are needed? ");

        float cost_ingredient[num_ingredients];

        total_cost[i] = 0;

        for (int j = 0; j < num_ingredients; j++)
        {
            printf("What is the cost of ingredient #%d", j+1);
            cost_ingredient[j]= read_real_positive("? ");
            total_cost[i] += cost_ingredient[j];
        }
        printf("The total cost at shop #%d is $%.2f.\n", i+1, total_cost[i]);

        if (i == num_shops - 1)
        {
            cheapest_order = total_cost[0];

            for (int k = 1; k < num_shops; k++)
            {
                if (total_cost[k] < cheapest_order)
                {
                    cheapest_order = total_cost[k];
                    cheapest_shop = k + 1;
                }
            }
        }
    }
    printf("The cheapest  order  was  at  shop #%d, and the total cost of the order was $%0.2f\n", cheapest_shop, cheapest_order);
    return 0;
}![enter image description here](https://i.stack.imgur.com/GcAtS.jpg)

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

Eliminate arrays by carefully rethinking the problem

At the end of the program, all that is needed is the cheapest_order and the cheapest_shop.  All of the arrays could be eliminated by simply keeping, as each shop total is calculated, the current cheapest order.  There is no need to keep any data from a shop that is not cheapest, nor any reason to store the ingredient costs in an array.

Fix the bugs

If the scanf fails, as it does if the user inputs a letter instead of a number, the program acts as if the number 1 had been entered, and the user gets no chance to correct it.  Also, when I entered -1 for the number of shops to be visited, I got this result:

How many shops will be visited? -1
The cheapest  order  was  at  shop #1, and the total cost of the order was $81082112.00

Test your code thoroughly

One way to test the code is to simply try it multiple times.  A better way is to automate the process by writing a test driver.  There are many ways to do this; one simple way is to manually create a few files with test input and then feed them to the program.

Don't use floating point for money

There is a problem using floating point (that is, float or double types) to represent money values.  See this question for a thorough explanation for why that's the case.

An alternative is to keep a number of cents as an integer value internally.  For more depth about floating point issues, I'd recommend the excellent article "What every computer scientist should know about floating-point arithmetic" by David Goldberg.


Error Handling in Servlets and JSPs I have an HTML login form that is processed by a servlet, which then redirects the user to a result page.

login.jsp:

LOG IN

Please enter your credentials.

<form action="process-login.jsp" method="post">

        Username:
        <input type="text" name="username" size="30" required title="Please enter your username."/>

        Password:
        <input type="password" name="password" size="30" required title="Please enter your password."/>

<input type="submit" value="Login"/>
</form>

Servlet (process-login.jsp):

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    String userId = request.getParameter("username");
    String userPassword = request.getParameter("password");

    if(userId != null && userPassword != null) {
        DatabaseManipulator dm = new DatabaseManipulator();
        String[] usernameArray = dm.dbEntriesToArray("user_id");
        String registeredPassword = dm.getRegisteredPassword(userId);

        LoginModule lm = new LoginModule();
        boolean hasExistingAccount = lm.hasExistingAccount(usernameArray, userId);
        boolean isPasswordCorrect = lm.isPasswordCorrect(userPassword, registeredPassword);

        // returns new session if it does not exist
        HttpSession session = request.getSession(true);

        // bind objects to session
        session.setAttribute("hasExistingAccount", hasExistingAccount);
        session.setAttribute("isPasswordCorrect", isPasswordCorrect);

        if(!hasExistingAccount) {
            response.sendRedirect("login-result.jsp");

        } else if(!isPasswordCorrect) {
            response.sendRedirect("login-result.jsp");

        // login success
        } else {
            String currentUserId = dm.getCurrentUserId(userId);
            int currentUserType = dm.getCurrentUserType(userId);

            session.setAttribute("currentUserId", currentUserId);
            session.setAttribute("currentUserType", currentUserType);

            response.sendRedirect("index.jsp");
        }

    } else {
        // redirect on illegal access of servlet
        response.sendRedirect("index.jsp");
    }
}

login-result.jsp:

LOGIN RESULT

<%
// retrieve objects from session
Boolean hasExistingAccount = session.getAttribute("hasExistingAccount") != null ? (Boolean) session.getAttribute("hasExistingAccount") : true;
Boolean isPasswordCorrect = session.getAttribute("isPasswordCorrect") != null ? (Boolean) session.getAttribute("isPasswordCorrect") : true;

if(!hasExistingAccount) {
%>

<form action="login.jsp">

User does not exist!

<input type="submit" value="Try Again"/>
</form>

<% } else if(!isPasswordCorrect) { %>

<form action="login.jsp">

Username or password is incorrect!

<input type="submit" value="Try Again"/>
</form>

<% } else {
    // redirect on illegal access of jsp
    response.sendRedirect("index.jsp");
}
%>

(Kindly forgive the scriptlets for now, as it is what we have reached in our lessons so far, though I am now aware it is deprecated.)
I use plain if-else statements, where the result page gives the appropriate message. Is this an acceptable way to handle user input errors?

This doesn't look good:

DatabaseManipulator dm = new DatabaseManipulator();
String[] usernameArray = dm.dbEntriesToArray("user_id");
String registeredPassword = dm.getRegisteredPassword(userId);

LoginModule lm = new LoginModule();
boolean hasExistingAccount = lm.hasExistingAccount(usernameArray, userId);
boolean isPasswordCorrect = lm.isPasswordCorrect(userPassword, registeredPassword);

The interaction between the current class, DatabaseManipulator and LoginModule is not natural and inefficient.
Why get an array of usernames from a DatabaseManipulator and pass that to LoginModule?
These classes are too tightly coupled: they know too much about each other.
The current class gets something from DatabaseManipulator and passes it on to LoginModule.
A system with too many connections between its elements like this is complex and rigid.

The responsibilities are also not well separated.
The current class shouldn't need to know the details of validating users,
but delegate to a dedicated UserManager class.
Consider these methods and responsibilities:

UserManager.isValidUser(String username): check if the user exists. The class will hide how this check is performed. Maybe it loads an array of all usernames, like the original code. Hopefully it doesn't, but to outsiders it won't matter. You can extend it later: you might add the concept of disabled users. This class and method will be the single place to make that change, the rest of your program doesn't have to know how it works.
UserManager.isPasswordCorrect(String username, String password): check if the password is correct

The original code would then become:

UserManager userManager = new UserManager();
boolean hasExistingAccount = userManager.isValidUsername(userId);
boolean isPasswordCorrect = userManager.isPasswordCorrect(userId, userPassword);

This is more intuitive, the details of user validation are now correctly hidden from the current class, and it's shorter too.

Security

I'm not sure it's such a good idea to tell the user when a username doesn't exist.
One can discover the valid usernames on the site by a brute-force attack.
It might be somewhat better to give the same error message whether the username exists or not, something:

  The username or password you entered is incorrect.

Minor things

These conditions can be joined:

if(!hasExistingAccount) {
    response.sendRedirect("login-result.jsp");

} else if(!isPasswordCorrect) {
    response.sendRedirect("login-result.jsp");

like this:

if (!hasExistingAccount || !isPasswordCorrect) {
    response.sendRedirect("login-result.jsp");
}

When you have code like this:

if (userId != null && userPassword != null) {

    // many, many, many lines of code...

} else {
    // redirect on illegal access of servlet
    response.sendRedirect("index.jsp");
}

... by the time you read until the else block, it can be difficult to remember what the if was about... In such situations it's better to make the shortest block come first by reverting the condition:

if (userId == null || userPassword == null) {
    // redirect on illegal access of servlet
    response.sendRedirect("index.jsp");
} else {
    // many, many, many lines of code...
}

or, it might be even more natural to return early and drop the else block,
reducing one level of indentation:

if (userId == null || userPassword == null) {
    // redirect on illegal access of servlet
    response.sendRedirect("index.jsp");
    return;
}

// many, many, many lines of code...

This is especially useful with more complex, arrow-shaped code, with many nested conditions.


Calculating the number of primes between 3 and another number This is a simple program that gets a number from a user and calculates the number of primes between 3 and the user's number. I'm just curious if I'm using too many variables. I'm only asking since I'm used to not needing to initialize iterator variables before a loop.

#include <iostream>
using namespace std;
int isPrime(int);
int main() {
    int userValue;
    int b;
    int x;
    char c;
    cout<< "Enter a number : "<< flush;
    cin >> userValue;
    for (x = 3; x <= userValue; x= x+2){
        a = isPrime(x);
        if (a == 1){
        b +=1;
        }
    }
    cout << "There is, " << b << " number of primes from 3 to "<< userValue << endl;
    cout << "hit enter to terminate..."<< flush;
    cin.get();
    cin.get();
}

int isPrime(int number){
    int i;
    for(i=2; i<number; i++){
    if (number % i == 0 && i != number) return 0;
    }
    return 1;
}

No, I wouldn't say you're using too many variables (except for c, which doesn't appear to be used anywhere).  Not enough functions, though.  And perhaps too many spurious assignments.  Specifically, I'd suggest something like:

#include <iostream>
using namespace std;

bool isPrime(int);
int countPrimes(int);
int main() {
    int userValue;
    cout<< "Enter a number : "<< flush;
    cin >> userValue;

    cout << "There are, " << countPrimes(userValue) << " number of primes from 3 to "<< userValue << endl;
    cout << "hit enter to terminate..."<< flush;
    cin.get();
    cin.get();
}

int countPrimes(int number) {
    int result = 0;
    for (int x = 3; x <= number; x += 2){
        if (isPrime(x)){
            result++;
        }
    }
    return result;
}

bool isPrime(int number){
    for(int i=2; i<number; i++){
        if (number % i == 0 && i != number) return false;
    }
    return true;
}

That will do the same thing as your code.  Also, please consider using variable names that are not just single-letters.


Generic class for displaying UIAlertView / UIAlertController on iOS To address backwards compatibility for UIAlertView, I have come up with an interface that would seem to work for both iOS versions 8 and lower.

This is not something I would like to roll out in production hence I would appreciate few extra pair of eyes taking a look at it.

Here is a link to the GitHub gist.

//UIAlertMode.h
#import <Foundation/Foundation.h>
#import <UIKit/UIKit.h>

typedef void (^AlertViewSelectionBlock) (NSInteger);
typedef void (^AlertControllerDefaultAction ) (UIAlertAction *);
typedef void (^AlertControllerCancelAction) (UIAlertAction *);
typedef void (^GenericBlock) (void *);

@interface UIAlertMode : NSObject

@property (assign, nonatomic) AlertViewSelectionBlock alertViewBlock;
@property (assign, nonatomic) AlertControllerCancelAction cancelAlertAction;
@property (assign, nonatomic) AlertControllerDefaultAction defaultAlertAction;

- (id) initAlertWith:(NSString *) title
                    :(NSString *) message
                    :(NSString *) cancelButtonTitle
                    :(NSString *) otherButtonTitle;

- (BOOL) supportsiOS8;
- (void) showAlertFor:(AlertViewSelectionBlock) alertViewBlock;
- (void) showAlertControllerFor:(AlertControllerDefaultAction) defaultAction :(AlertControllerCancelAction) cancelAction;
- (void) showAlertWith:(GenericBlock) block;
@end

// UIAlertMode.m

#import "UIAlertMode.h"
#import <objc/objc-runtime.h>
#import "AppDelegate.h"

@interface UIAlertMode () <UIAlertViewDelegate> {
    UIAlertView       *_alertView;
    UIAlertController *_alertController;
}

@end

@implementation UIAlertMode

@synthesize alertViewBlock;
@synthesize defaultAlertAction;
@synthesize cancelAlertAction;

- (id) init {
    self = [super init];
    return self;
}

- (id) initAlertWith:(NSString *) title
                    :(NSString *) message
                    :(NSString *) cancelButtonTitle
                    :(NSString *) otherButtonTitle {
    self = [super init];
    if ([self supportsiOS8]) {
        _alertController = [UIAlertController alertControllerWithTitle:title message:message preferredStyle:UIAlertControllerStyleAlert];

        UIAlertAction *defaultAction = [UIAlertAction actionWithTitle:otherButtonTitle style:UIAlertActionStyleDefault handler:^(UIAlertAction *action) {
            if (self.defaultAlertAction) {
                self.defaultAlertAction(action);
            }
        }];

        UIAlertAction *cancelAction = [UIAlertAction actionWithTitle:cancelButtonTitle style:UIAlertActionStyleCancel handler:^(UIAlertAction *action) {
            if (self.cancelAlertAction) {
                self.cancelAlertAction(action);
            }
        }];

        [_alertController addAction:defaultAction];
        [_alertController addAction:cancelAction];

    } else {
        _alertView = [[UIAlertView alloc] initWithTitle:title message:message delegate:self cancelButtonTitle:cancelButtonTitle otherButtonTitles:otherButtonTitle, nil];
    }
    return self;
}
#pragma mark - Public API

- (BOOL) supportsiOS8 {
    Class kAlertViewClass = NSClassFromString(@"UIAlertController");
    return !kAlertViewClass ? NO : YES;
}

- (void) showAlertFor:(AlertViewSelectionBlock) alertBlock {
    self.alertViewBlock = alertBlock;
    [_alertView show];
}
- (void) showAlertControllerFor:(AlertControllerDefaultAction) defaultAction :(AlertControllerCancelAction) cancelAction {
    self.cancelAlertAction = cancelAction;
    self.defaultAlertAction = defaultAction;
    dispatch_async(dispatch_get_main_queue(), ^{
        AppDelegate *delegate = (AppDelegate *)[[UIApplication sharedApplication] delegate];
        [delegate.window.rootViewController presentViewController:_alertController animated:YES completion:nil];
    });
}

#pragma mark - UIAlertView delegate method

- (void) alertView:(UIAlertView *)alertView clickedButtonAtIndex:(NSInteger)buttonIndex {
    if (self.alertViewBlock) {
        self.alertViewBlock(buttonIndex);
    }
}

I would like to get feedback on design, scalability, and memory management.

As a start, this is confusing:

- (BOOL) supportsiOS8 {
    Class kAlertViewClass = NSClassFromString(@"UIAlertController");
    return !kAlertViewClass ? NO : YES;
}

There was a time when I was uncomfortable with ternary operators.  They're admittedly kind of weird.  I've gotten used to them, and I do enjoy them, but they should be used appropriately.

The logic here is that NSClassFromString returns nil if it can't figure out what class you've specified.

For starters, I'd change this to look more like this:

- (BOOL)supportsiOS8 {
    return NSClassFromString(@"UIAlertController") != nil;
}

Now it's slightly more readable and more clear what we're doing.  We don't actually care what's returned beyond whether or not it is nil.

However, there's still problems...

Your method is called supportsiOS8, but all your method actually checks is whether or not this class can see a class called UIAlertController.  And there's a difference.

I can use a class called UIAlertController on iOS 7 and lower.  It's simply a matter of me implementing that class myself.  If I've imported that class's file into this file, your supportsiOS8 will return YES.  And my implementation of UIAlertController may or may not match Apple's implementation.  So with that said, simply renaming the method as supportsAlertController wouldn't really work.

Moreover, it's likely that UIAlertController will continue to exist in iOS 9 and onward.  Apps that use this class may or may not actually support iOS 8 (because supporting iOS 8 isn't actually necessary).

Instead, what we actually need to do is check the version of iOS that the user is actually using and compare it to the minimum iOS version for Apple's UIAlertController (iOS 8).

For starters, we can use the following method to determine the device's version:

[[UIDevice currentDevice] systemVersion];

This will return the device's version as an NSString.  It might look something like @"8.1".

Now, we compare the return of this to the version we want.

- (BOOL)hasMinimumRequiredOS {
    NSString *deviceVersion = [[UIDevice currentDevice] systemVersion];
    return ([deviceVersion compare:@"8" options:NSNumericSearch] != NSOrderedAscending);
}


Strategy design pattern in C++ using unique_ptr 1. Introduction

This is for my ANN project.

Each building block of ANN, neuron, has activation function which determines how to get the output given the input (see formula in on repo I linked).

I have abstract class Neuron which represents a neuron in ANN. The following points have been taken into account:

Not only activation function is intended to be used but its derivative also
Activation function (and its derivative) returns double and takes double as the only argument
It should be easy and clear how to add and use new defined activation function
Activation function should be initialized at the construction step of Neuron instance

2. Possible solution

2.1. Activation class

Each activation function is represented by an abstract class Activation:

class Activation
{
    public :
        virtual double f( double x ) = 0;
        virtual double fPrime( double x ) = 0;
};

2.2. Concrete activation

Concrete activation is represented by a derived class:

class Identity : public Activation
{
    public :
        double f( double x ) { return x; }
        double fPrime( double x ) { return 1.; }
};

class Sin : public Activation
{
    public :
        double f( double x ) { return sin(x); }
        double fPrime( double x ) { return cos(x); }
};

class Sigmoid : public Activation
{
    public :
        double f( double x ) { return 1. / ( 1. + exp( -x ) ); }
        double fPrime( double x )
        {
            return ( 1. - f(x) ) / f(x);
        }
};

2.3. Activation as a class member

I decided to use std::unique_ptr to store activation function as a member of Neuron class.

class Neuron
{
    private :
        std::unique_ptr<Activation> _act;

    public :
        Neuron( std::unique_ptr<Activation> act ) :
            _act( std::move( act ) )
        { }

        double Act( double x ) { return _act->f( x ); }//get output
        double ActPrime( double x ) { return _act->fPrime( x ); }//get derivative of activation

        //some other functions including pure virtual ones
};

2.4. Adding new activation

In order to add new activation function one "just" should create a class derived from Activation class and overwrite functions f and fPrime.

2.5. Using

Neuron n( std::make_unique<Identity>() );
n.Act( 30. );
n.ActPrime( 30. );

3. Discussion

I am pretty satisfied with this solution but what I am thinking about is is any destructor needed in this case if I use unique_ptr? If so, then what exactly?

Any critic, suggestion, correction, idea, advice etc. would be appreciate.

This looks like a standard use of PIMPL.  Is that all the member functions you need?  If just 2 or 3, just manually forwarding each one is fine.

Does the Activation class do anything besides have those forwarding functions?  If not, don’t bother with the wrapper and forwarding at all!  Just make an instance of the concrete class and use it.  (If there are other functions, see if they can simply be non-virtual in the base and inherited)

Correct, unique_ptr has a destructor — that is part of its defining nature.  Don’t write one for the class, and the compiler knows it should just call the destructors of all the members.

The usage is a little clumsy:

Neuron n( std::make_unique<Identity>() );

How about adding a wrapper so you can write

auto n = Neuron_with<Identity>();

As a matter of style, leading underscores are generally criticized here.  Some compilers had and still support special keywords and extensions in this format.  You have to remember to never use a capital initial letter after the underscore.

It is perfectly OK and idiomatic to just name the constructor arguments the same as the members.


Project Euler problem #3 My code works but would take 160 days to run lol.

#prime factors - ex.3

primes = []
for possibleprime in range (2,100000,1):
    isPrime = True
    for num in range(2, possibleprime, 1):
        if possibleprime % num == 0:
            isPrime = False

    if isPrime:
        primes.append(possibleprime)

#print(primes)

def prime(n):
    prime_factors = []
    n = int(n)
    for x in primes:
        if n % x == 0:
            prime_factors.append(x)
    return prime_factors

# n = 13195
# print(prime(n))

n = 600851475143
print(prime(n))

The way you generate primes is very inefficient. There are a few quick fixes to speed it up. But first let's put the primality check into its own function:

def is_prime(n):
    prime = True
    for num in range(2, n):
        if n % num == 0:
            prime = False
    return prime

primes = [n for n in range(2, 100000) if is_prime(n)]

This uses a list comprehension, the fact that the default increment of range is 1 and follows Python's official style-guide, PEP8.

Now, if you know that a number is composite, there is no need to check for all other numbers if they divide the number:

def is_prime(n):
    for num in range(2, n):
        if n % num == 0:
            return False
    return True

All prime numbers except for 2 are odd. So just start with 2 in the list and increment by 2:

    primes = [2] + [n for n in range(3, 100000, 2) if is_prime(n)]

You could even hard-code all primes below 10, but the performance gain from that is very small.

And finally, if you know that k is a divisor of n, then so is n // k. In other words, as soon as you have checked all values smaller than sqrt(n), you have already checked all possible divisors.

from math import sqrt

def is_prime(n):
    for num in range(2, int(sqrt(n)) + 1):
        if n % num == 0:
            return False
    return True

There are even faster ways to generate all primes up to some number and they are known as sieves (since they sieve out multiples of numbers for which you know that they are prime). One possible implementation as a generator of the well-known Sieve of Eratosthenes is this:

def prime_sieve(limit):
    prime = [True] * limit
    prime[0] = prime[1] = False

    for i, is_prime in enumerate(prime):
        if is_prime:
            yield i
            for n in range(i * i, limit, i):
                prime[n] = False

primes = list(prime_sieve(100000))


FizzBuzz in SQL--no loops MS SQL Server, no loops.  ;)

WITH Numbers AS (
    SELECT 1 as Number
    UNION ALL
    SELECT Number+1 FROM Numbers WHERE Number < 100
)
SELECT CASE
    WHEN Number % 15 = 0 THEN 'FizzBuzz'
    WHEN Number % 5 = 0 THEN 'Buzz'
    WHEN Number % 3 = 0 THEN 'Fizz'
    ELSE CAST(Number as varchar)
END AS FizzBuzz
FROM Numbers;

Execution time ~5ms.

With performance being the most important concern, could this be made better?

Recursive CTEs in SQL Server execute involving a temp table. This is probably why this tiny amount of work even takes a measurable amount of time. This should take <= 1ms.

Use a numbers table. Or, use one of the many tricks to materialize a sequence of numbers without table access such as:

SELECT Num
FROM (VALUES (1), (2), ...) x(Num)

Using Number % 15 = 0 instead of Number % 3 = 0 AND Number % 5 = 0 is less clear. It requires mathematical insight to convince anyone that this is even correct. Code should be obviously correct.

The default length of varchar is unclear. (Can you tell from memory?!) Better use nvarchar(400). By default, use Unicode characters in order to just never have certain bugs and problems.

These issues aside this query is simple. Anyone can very quickly understand it and conclude that it is correct. That's good code.


Fastest image upload into SQL database I am trying to upload an image (screenshot from desktop) to an SQL database as fast as possible. I would like to optimize my current procedure in terms of speed:

bmpScreenshot = new Bitmap(Screen.PrimaryScreen.Bounds.Width,
                       Screen.PrimaryScreen.Bounds.Height,
                       System.Drawing.Imaging.PixelFormat.Format32bppArgb);
gfxScreenshot = Graphics.FromImage(bmpScreenshot);
gfxScreenshot.CopyFromScreen(Screen.PrimaryScreen.Bounds.X,
                                    Screen.PrimaryScreen.Bounds.Y,
                                    0,
                                    0,
                                    Screen.PrimaryScreen.Bounds.Size,
                                    CopyPixelOperation.SourceCopy);

byte[] imageBytes = convertImageToByteArray(bmpScreenshot);
MemoryStream mem = new MemoryStream(imageBytes);
using (Bitmap bmp1 = (Bitmap)Image.FromStream(mem))
{

     ImageCodecInfo[] codecs = ImageCodecInfo.GetImageDecoders();
     ImageCodecInfo jgpEncoder = codecs[1];

     System.Drawing.Imaging.Encoder myEncoder =
     System.Drawing.Imaging.Encoder.Quality;

}

  using (MemoryStream m = new MemoryStream())
                    {
                        EncoderParameters myEncoderParameters = new EncoderParameters(1);
                        EncoderParameter myEncoderParameter = new EncoderParameter(myEncoder, 23L);
                        myEncoderParameters.Param[0] = myEncoderParameter;
                        bmp1.Save(m, jgpEncoder, myEncoderParameters);
   using (Bitmap bmpLast = (Bitmap)Image.FromStream(m))
                        {
                    string query = "UPDATE ScreenCapture SET ScreenCapture=@Image,BroadcastTime=@BroadcastTime WHERE ID = 1";
                    SqlCommand sqlCommand = new SqlCommand(query, SQLconn);
                    ImageConverter converter = new ImageConverter();
                    byte[] beforeUpload = (byte[])converter.ConvertTo(bmpLast, typeof(byte[]));
                    sqlCommand.Parameters.AddWithValue("@Image", beforeUpload);
                    sqlCommand.Parameters.AddWithValue("@BroadcastTime", DateTime.Now);
                    SQLconn.Open();
                    sqlCommand.ExecuteNonQuery();
                    SQLconn.Close();
                }
            }
        }

Profiling - Performance and Diagnostics in Visual Studio 2013

The method I posted has following usage:

40,8% - byte[] imageBytes = convertImageToByteArray(bmpScreenshot);
10,4% - MemoryStream mem = new MemoryStream(imageBytes);
37,8% - mp1.Save(m, jgpEncoder, myEncoderParameters);

From captureUpload which takes 93.6% in System.Windows.Forms.ni.dll:

69.9% - System.Drawing.ni.dll
26.8% - convertImageToByteArray
8.8 - PresentationFramework.ni.dll

convertImageToByteArray takes 26.8% on return ms.ToArray().

It now takes me about 1 second to upload and download on client side, which is not so  efficient in my case. If anyone have idea how this procedure is possible to make faster I would be glad to know. The image has around 60 KB, which is quite a lot.

For graphic part :

I think Gdi++ of .NET framework is very slow.
Mainly the Bitmap() construtor and Save() method.

If it's possible for screenshot, I recommend to use :
Either directly gdi32.dll or (maybe, im not sure that is good idea) direct2d (benefits on hardware acceleration).

For transfert :

For your upload, run the transfert in other thread. (async or Delegate with BeginInvoke).
After it depends on your internet connection speed.

However, i don't understand why you recreate your Bitmap object twice :

using (Bitmap bmp1 = (Bitmap)Image.FromStream(mem))
using (Bitmap bmpLast = (Bitmap)Image.FromStream(m))

Don't reload with Bitmap,
create only one Bitmap for your screenshot, save it with encoding in memory stream and send memory stream to your database. With your memory stream you can get directly byte array with no conversion.

MemoryStream.Read(byte[] buffer, int offset, int count)


Navbar that adds and removes classes I've written some jQuery for my navbar, but it's super sloppy and I'm always trying to write better code.

$(function(){
    $(window).scroll(function(){
    var sT = $(this).scrollTop();
    if (sT >= 70) {
        $('.navbar').addClass('navbar-switch')
        $('.navbar .scroll-link').addClass('navbar-link-switch')
        $('.navbar .logo').addClass('logo-adjust')
        $('.navbar hr').addClass('hide')
        $('.navbar .navbar-collapse').addClass('border')
    } else {
        $('.navbar').removeClass('navbar-switch')
        $('.navbar .scroll-link').removeClass('navbar-link-switch')
        $('.navbar .logo').removeClass('logo-adjust')
        $('.navbar hr').removeClass('hide')
        $('.navbar .navbar-collapse').removeClass('border')
    }
    })
})

My goal is that, when the user scrolls pass a certain limit, my navbar removes certain classes and adds certain classes.

Is hard to evaluate without the html  but, considere introduce toggleClass.

$element.toggleClass(className, condition);

http://api.jquery.com/toggleClass/

If your jQuery version doesn´t allow it, you can introduce as a separate function:

function toggleClass($element, class, condition) {
   condition ? $element.addClass(class) : $element.removeClass(class);
}

and the full solution: http://jsfiddle.net/cbhvteon/1/


Checking if a number is Armstrong number or not here is another basic interview question that I had:

Armstrong Numbers

public class ArmstrongCheck {

  public static void main(String[] args) {

    Scanner in = new Scanner(System.in);

    if (!in.hasNextInt()) {
      System.out.println("error");
    }

    int number = in.nextInt();

    armstrong(number);
    System.out.println(armstrong(number));
  }

  private static String armstrong(int number) {
    double sum = 0.0;
    int oldNumber = number;

    while (number > 0) {

      int temp = number % 10;
      number = number / 10;
      sum = sum + Math.pow(temp, 3);
    }

    if (sum == oldNumber) {
      return "armstrong number";
    } else {
      return "not armstrong  number";
    }
  }
}

code is working but I want to simplfy/improve it, any suggestions?

In addition to the points made by justjofe, I also have a few suggestions.

You might consider making the method armstrong(int) return a boolean instead of a String, and then using the returned boolean to construct the output of your program. That way, you separate the logic of determining whether a number is an Armstrong number from the program's interface. In this case, a better name for the method might be "isArmstrong" instead of "armstrong":

public static void main(String[] args) {
    Scanner in = new Scanner(System.in);

    // ...

    if (isArmstrong(in.nextInt())) {
        System.out.println("armstrong number");
    } else {
        System.out.println("not armstrong number");
    }
}

private static boolean isArmstrong(int number) {
    // ...
    return sum == oldNumber;
}

And now about the code that checks a number for being an Armstrong number itself. I think assigning a new value to the method parameter number is confusing, because it forces you to introduce a local variable that serves no other purpose than to store the original value of the method parameter. So why not do it the other way around, using the variable declared inside the method for the loop and leaving the method parameter number assigned to the original value?

private static boolean isArmstrong(int number) {
    double sum = 0.0;
    int n = number;

    while (n > 0) {
        int temp = n % 10;
        n = n / 10;
        sum = sum + Math.pow(temp, 3);
    }

    return sum == number;
}

After this change, the while loop combined with the declaration of n (or whatever you want to call it) actually lends itself to being expressed as a for loop instead:

private static boolean isArmstrong(int number) {
    double sum = 0.0;

    for (int n = number; n > 0; n = n / 10) {
        int temp = n % 10;
        sum = sum + Math.pow(temp, 3);
    }

    return sum == number;
}

I would now even go so far as to skip the introduction of the temp variable and just invoke Math.pow(n % 10, 3), since you only need to evaluate n % 10 once per loop iteration, but that is purely a matter of taste.

Finally, some comments on points mentioned by justjofe:

First, about closing the scanner: Be aware that calling close() on the Scanner will also call System.in.close() which might not be desirable. There are several threads concerning this at stackoverflow, and one answer says this:

  Generally speaking, the code that creates a resource is also responsible for closing it. System.in was not instantiated by by your code, but by the VM. So in this case it's safe to not close the Scanner […]

However, this doesn't address the issue of garbage collection with regards to the Scanner object raised by justjofe, and I don't know enough to give specific advice here, so I'll just make you aware of this issue.
Second, a small heads-up on compound assignment operators such as += and /=: While it might appear that

sum = sum + Math.pow(temp, 3); and
sum += Math.pow(temp, 3);

are equivalent, this is actually not the case. If you make sum an int instead of a double, the second statement will compile, however, the first one will not. The reason for this is that the second statement is actually equivalent to

sum = (int) (sum + Math.pow(temp, 3));

(as stated here in the Java Language Specification). Since Math.pow(double, double) returns a double, the result needs to be cast to an int before being assigned to an int variable. The compound assignment operator does this for you, but when using an ordinary =, you have to do the casting yourself.


Is this an efficient way of mounting a file-system via SSH (SSHFS)? #!/usr/bin/python3

# Description: Python script that will mount FTP-Server via SSHFS protocol
import sys
import os
import subprocess

def mount():
#Function for mounting the networked drive
    if os.path.exists('/home/user/MNT/MEDIA/'):
        print('Network drive is mounted')
        un = input("To unmount type U/u \n")
        if un == "u" or un == "U":
            subprocess.call(['sudo fusermount -u /home/user/MNT/'], shell=True)
        else:
            pass
        exit(0)
    if not os.path.exists('/home/user/MNT/MEDIA/'):
        IP = input("Type I/i for local connection, E/e for external\n")
        if IP == "i" or IP == "I":
            subprocess.call(['sudo sshfs -o idmap=user -o uid=1000 -o gid=1000 -o allow_other -o default_permissions -p 100 user@10.0.0.100:/media/Hitachi/ /home/user/MNT'], shell=True)
        if IP == "e" or IP == "E":
            subprocess.call(['sudo sshfs -o idmap=user -o uid=1000 -o gid=1000 -o allow_other -o default_permissions -p 100 user@xxxxxxxx.com/media/Hitachi/ /home/user/MNT'], shell=True)
        elif IP == "": #returns if null value
            return mount()

def makedir():
#Function to check and see if directory is created and if not create it
    if not os.path.exists('/home/user/MNT/'):
        subprocess.call(['sudo mkdir /home/user/MNT/'], shell=True)
    if os.path.isdir('/home/user/MNT/'):
        mount()

makedir()

EDITED:

def sshfs():
#function to mount network drive
    while True:
        user = input("Type 'i' for internal, 'e' for external connection\n")
        fs = File_System.get(user.lower()) #Get value from the dict called File_System
        if fs is None:
            print("Invalid option, enter either 'i' for internal or 'e' for external\n")
        else:
            subprocess.call(["sudo sshfs -o uid=1000 -o gid=1000 -o idmap=user -o default_permissions -o allow_other -p 100 %s %s" % (fs, Local_Mount)], shell=True)
            exit(0)

Efficiency is just not an issue in this kind of task.  As "glue" code, it's not computationally intensive.  There are, however, some maintainability issues.

In mount(), the basic outline is:

def mount():
    if os.path.exists('/home/user/MNT/MEDIA/'):
        ...
        exit(0)
    if not os.path.exists('/home/user/MNT/MEDIA/'):
        ...

… which should be expressed as:

def mount_or_unmount():
    if os.path.exists('/home/user/MNT/MEDIA/'):
        ...
    else:
        ...

Using recursion for error handling is weird.  That would normally be handled using a while-loop.  It's also slightly odd that the empty string triggers a retry, but any other invalid input would exit the script.
The two sudo sshfs commands differ only slightly.  Your code should be structured accordingly.

REMOTE_FS = {
    'i': 'user@10.0.0.100:/media/Hitachi/',
    'e': 'user@xxxxxxxx.com/media/Hitachi/',
}

...
while True:
    ip = input("Type I/i for local connection, E/e for external\n")
    fs = REMOTE_FS.get(ip.lower())
    if fs is None:    # Input was not 'i' or 'e'
        continue
    subprocess.call(['sudo sshfs -o idmap=user -o uid=1000 -o gid=1000 -o allow_other -o default_permissions -p 100 %s /home/user/MNT' % (fs)], shell=True)
    break