File uploads with image previews and remove link using Array and readAsDataURL I got my code working with vanilla JavaScript, however I'm new to JS, so I don't know if the code is robust or not. Any advice to improve the code is welcome.
Note: I just noticed a bug. The code removes the image from the DOM (is it the right term?) but it still submits the images that I deleted. Any idea how can I fix that?

 let count = 0

 function previewFiles(input) {
   const preview = document.getElementById('preview')
   const {
     files
   } = input

   Array.from(files).forEach(file => {
     const reader = new FileReader()

     reader.onload = e => {
       const div = document.createElement('div')
       const img = document.createElement('img')
       const button = document.createElement('button')

       img.src = e.target.result
       img.width = 200
       img.height = 200
       img.alt = `file-${++count}`

       button.textContent = "Delete"
       button.addEventListener('click', () => {
         preview.removeChild(div)
       })

       div.appendChild(img)
       div.appendChild(button)

       preview.appendChild(div)
     }

     reader.readAsDataURL(file)
   })
 }
    <input type="file" multiple onchange="previewFiles(this);" />

Welcome to Code Review! Good on you for opening up to constructive criticism.
Buckle up, this will be a long one ;)

 The Question
Let us start with your question:

[...] I don't know if the code is robust or not. Any advice to improve the code is welcome.

I hope you would agree with me when I say that this is not really a proper question. Code "robustness" is a rather subjective matter, so one couldn't objectively answer that. If you would like my opinion: any code that does what it needs to do, everytime you run it, is robust.
I could direct you to the guide on posting a proper question, but I find that is often unnecessary, since I understand what you would want from me so you can improve this code.
Another problem with your question is that you have a bug in your code. Fixing bugs is not really something we do at Code Review, but the way I see it, we can both improve your code and fix the bug in one swoop!
I'm going to review your code nonetheless, because it's perfectly possible, it's fun and people who come here to get their code reviewed can get easily discouraged with all these rules.
The way I measure code improvement in my day-to-day reviews, is by how much smaller you can make a file or function, while maintaining full functionality and  maintaining or improving readability. I'm not really interested in code speed, so you'd have to look elsewhere for tips on that. Let's get to work.

 The problem
The (in my opinion) most valuable advice one could give to a beginning programmer is the following:
Before you start on any given programming problem, you must first understand the problem, and the sub-problems that make up the larger problem, ad infinitum.
In your case it would require you to state and disect the problem as follows:

Create a File upload with image previews and removal functionality

How does one upload a file?

What does it mean to "upload" a file?

How does one remove an uploaded file?

What does it mean to "remove" an uploaded file?

How does one show a thumbnail for an uploaded file?

What is a thumbnail?
How do you get a thumbnail from an uploaded file?
How do you link the remove functionality to a thumbnail?

Thence we can figure out how to write code that solves these sub-problems, and combine those solutions into one big solution.

 How does one upload a file?
This is a part of the problem you've understood. To upload a file with HTML, you use the input tag with type=file. The interesting part here is the next step: what does it mean to "upload" a file?
According to the html specification for <input type="file"/>, an input tag keeps track of what items have been uploaded, known as "the selected files", under a property called files. So for a file to "be uploaded", it must be in the files property of the input. It would seem your mental model does not match the actual implementation of the input tag! That's where the bug comes from.
When you upload a file in your snippet, it overwrites the last selection of uploaded files. You probably thought you fixed it using the multiple attribute, but that attribute only specifically allows multiple files to be uploaded at once. This means that everytime you select any file(s), you lose reference to the previously uploaded files.
Now that we know what we want and understand the problems, we can define a bit of code to do what we want.

<input type="file" multiple onchange="upload(this)"/>

There we go. That's all we needed: an array to store our uploaded files and a function to add to that array any file we upload.
A couple of quick notes about this solution:

input.files is a FileList, not an array -- you knew and understood this. One could transform it to an array and use array methods on it, but I chose to use the ... spread operator. In this context, to spread a FileList means to take every File in the FileList, and provide it as an argument to files.push(...). Other solutions like for (const file of input.files) would also work, with the benefit that these methods don't create an array which we never directly use for anything but looping over.

❌ How does one "remove" an uploaded file?
The way you remove a file in your current code is not actually removing the file. You are just removing the thumbnail from the DOM (that is indeed the right term).
But then what does it mean to "remove" an uploaded file? Well, simply put: if "uploading" means that we add a File to an array, "removing" should just mean to take that File out of the array again:

<input type="file" multiple onchange="upload(this)"/>

A couple of quick notes about this solution:

To remove an element from an array in JavaScript you need to use Array.splice, which requires an index from where to start removing elements, and the number of items to delete from there. files.splice(index, 1) means to remove 1 item at index index.
To know which file we want to delete from the list, we need a way to differentiate between files. The File API defines a property "name" which we can use.
To know the index of the file we differentiated, we need to use Array.findIndex, which takes a function as a parameter and returns either the index of that file, or -1 (meaning the file was not found in the array).
In case we try to remove a non-existing file, we return. This should in theory never happen, but you should always cover your non-happy flows. That's just good software etiquette.

 How does one show a thumbnail for an uploaded file?
This is not a simple question. To show a thumbnail, we must first understand what a thumbnail is.
 What is a thumbnail?
Simply put, if a given file is an image, we want to show a small representation of that image. There's already a problem: your input accepts files of any type. If you upload a pdf or an illustrator file, your code will not work. Again, if you think about the problem and define what you do and do not want, you can create a solution.
The simplest approach is to allow any image, and nothing else. One could make custom rendering logic for all sorts of files; a fun exercise for the reader ;).
HTML has got us covered: you can specify what type of file your input will accept. You can read the entire page (which is quite interesting so why not), but the main takeaway here is that you can use the accept attribute with a value of "image/*" to accept only files that are images:

<input type="file" multiple accept="image/*" onchange="upload(this)"/>

A couple of quick notes about this solution:

An added benefit is that the HTML specification allows user agents to use this constraint to display only images in the file picker dialog.
As mentioned in the spec, there is still no guarantee that uploaded files will really, really be images. We will keep this in mind for the last step.

➡ How do you get a thumbnail from an uploaded file?
Now that we allow only images, we need a way to display uploaded files as thumbnails. You did a good job discovering that you can use the FileReader API to transform a File to an img in the DOM. The method itself is solid, but the approach could be a bit more tactical. This will be the largest code chunk of this answer, so I'm going to annotate it using comments.

<input type="file" multiple accept="image/*" onchange="upload(this)"/>

It works!
A couple of quick notes about this solution:

Like I said in the first comment, you're going to need a solid understanding of Promises. If I were to go into detail here this answer would become far too large.
A thumbnail typically is a version of an image with a much smaller resolution to save data. This solution just loads the entire image into memory, which could be troublesome in case of many uploaded files.

❌ How do you link the remove functionality to a thumbnail?
Now that we have thumbnails and know what it means to remove a file, we can link these together. This is now an easy step.

<input id="files" type="file" multiple accept="image/*" onchange="upload(this)"/>

A couple of quick notes about this solution:

Since some files may share the same name, upon removing an image, it will remove the first image from the array with that name. We can't do much about that using the native API, but you could assign some id to every file upon "uploading" them. That shouldn't be too hard.
All this work really only gives us a DOM representation of our input, but if you would like to upload these files using a <form> you're out of luck. You would need to define custom logic in order to upload the const files array upon form submission -- but since we're already heavy into JavaScript-land here, you might just want to do that. In that case, have a look at the Fetch API which allows you to post blob data (Files implement the Blob interface).

 Conclusion
We've done it -- through blood, sweat and tears perhaps.
Hopefully you learned a thing or two, otherwise this was probably really boring... :)
So how did we do according to my metric of

how much smaller you can make a file or function, while maintaining full functionality and maintaining or improving readability

Well we increased the size of the code... We went from 34 lines to 50. However, there's an important reason: we only care for less code while maintaining full functionality and maintaining or improving readability. We increased functionality, and also made sure the code is actually always correct in a happy flow.
To link that to your inquiry about "robustness": we defined what it means for your code to work, and we made sure it works in every case we want it to. That's robust code in my opinion.
I would also argue that, since we've neatly packed our functionality into reasonably small functions, it's crystal clear what each part of the code does. Your snippet was clear to me as well, so it's not an improvement per se, but that's quite the achievement when you change a large part of the code.
The main takeaways are:

Define the problem, then define the problem, then define the problem...
Think about what the code should do, but also about what it shouldn't do (and make sure it doesn't).
Whenever asynchronous code will run, such as events, you could probably use a Promise to streamline the interface.
I would personally like it better if you would use only one way of defining functions. There are subtle differences in implementation between function and () => {} but you're not leveraging those differences. You could just use either notation to be consistent and reduce the amount of "WTF?!"'s coming from the review room.

You're already quite proficient with JavaScript; you're using fancy syntax such as const and () => body, you have researched and understood intricate concepts of the browser environment and you're using as much vanilla JS and HTML as possible. Very nice!
Here's the final snippet:

<!DOCTYPE html>

  <meta charset="UTF-8">
  <meta http-equiv="X-UA-Compatible" content="IE=edge">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Document</title>

  <input type="file" id="files" multiple accept="image/*" onchange="upload(this)"/>


CaesarsCipher Code Quality Concern I have been following the FreeCodeCamp, and I am done with the basic algorithm challenges. The last one (CaesarsCipher) troubled me a bit, though I had done a similar one without much problem in C, for CS50 course.

Finally I made it work, but at the moment I am self-critical about "How?" I made it work and the quality of my code. I found the basic solution provided quite hard to understand and it looks way advanced than my code. I am posting my code here with the hope of receiving some feedback on it and finding out if am I doing something wrong/should be concerned about improving quality before moving on to new topics and challenges. (such as writing style or the functions i use/ the way I handle the problem)

function rot13(str) { // LBH QVQ VG!

  //defining an isAlpha function to skip the non alpha chars

  var isAlpha = function(char){
    return typeof char==="string" && char.length == 1 && char <= "Z" && char >="A";
   };

  //two empty arrays two use later

  var shifted = [];
  var decoded= [];

  /*transform the encoded chars to ascii numbers then shift by 13
  and copy the decoded char to another array
  */

  for(var i = 0; i < str.length; i++){

    if(isAlpha(str[i]) && str[i].charCodeAt() < 78) {

      shifted[i] = str[i].charCodeAt()+13;

      decoded.push(String.fromCharCode(shifted[i]));
    }

    else if(isAlpha(str[i]) && str[i].charCodeAt() >= 78){

      shifted[i] = str[i].charCodeAt() - 13;

      decoded.push(String.fromCharCode(shifted[i]));
    }

    else{

      decoded.push(str[i]);
    }
  }

  //lastly merge the decoded chars into a string

  var last = decoded.join("");

  return last;

}

// Change the inputs below to test
rot13("SERR PBQR PNZC");

var isAlpha = function(char){
    return typeof char==="string" && char.length == 1 && char <= "Z" && char >="A";
};

I like this. I like the careful type and length checking, and the comparison with strings rather than weird numeric codes. I would suggest first that it could do with handling lower case letters as well. Second, it seems like a moderately useful general purpose function, and not something that can only be used within the scope of a rot13 call. I'd be inclined to move it out.

Your main case analysis works (Except for not handling lower case numbers) but it could probably be clearer.
I would suggest

if(isAlpha(str[i])) {
    if (str[i].charCodeAt() < 78) {
        ...
    } else {
        ...
    }
} else {
    ...
}

This makes it clear that you have handled every possible case. In my opinion it also makes it clearer what the structure of the conditional is.

It would be even clearer what you're testing if 78 wasn't sitting there as a magic number. Using "N".charCodeAt() or even better "A".charCodeAt() + 13 would help. If concerned about the performance penatly of doing the extra function call in the check, make it a constant outside the loop.

var shifted = [];
var decoded= [];

I would suggest that you preallocate the length of your decoded array to be the length of your input string. This improves efficiency because Javascript knows to pack it tightly, and knows exactly how big it is going to get. It also improves readability by providing contextual clues. (i.e. that it's associated with str

I would completely delete shifted. You don't need an array to hold on to intermediate values that are immediately finished and placed in decoded. If you don't want to put the whole thing into one line, use a single scalar variable instead of an array.

// Change the inputs below to test
rot13("SERR PBQR PNZC");

It is good to test code! And it's nice when CodeReview questions actually come with evidence of testing too. I would suggest that a slightly more pedantic test case might be suitable. I'd want to use something with all classes of characters: upper & lower case, numbers, spaces, and assorted symbols. In particular I'd make sure that the test case contains both "A" and "Z" as well as  "@" and "[" These are just outside of the letters range in ASCII, so good for checking for Off By One errors. It would also be quite nice to express the target output so that we know whether it's worked!

(In a proper testing framework that would be in code, but a comment probably suffices here.)

In terms of naming, it broadly seems fine. I would probably choose a different name for str, possibly cipherText.

In terms of formatting things are a bit all over the place. I don't have strong opinions on JavaScript formatting style, but whatever style you pick it's worth sticking to consistently. Personally there are too many blank lines for my liking; in particular a blank line between the end of an if clause and its associated else clause is really jarring!


Need an algorithm to optimize grouping python dictionaries in hierarchical form Previously I asked a question on how to group dictionaries in a list in a hierarchical structure.

Initially I wanted to group a list of dictionaries that looks like the following, using multiple keys:

[{'dept':1, 'age':10, 'name':'Sam'},
{'dept':1, 'age':12, 'name':'John'},
.
.
.
{'dept':2,'age':20, 'name':'Mary'},
{'dept':2,'age':11, 'name':'Mark'},
{'dept':2,'age':11, 'name':'Tom'}]

And the output would be:

[
    {
        2: {
            20: [
                {
                    'age': 20,
                    'dept': 2,
                    'name': 'Mary'
                }
            ]
        },
        {
            11: [
                {
                    'age': 11,
                    'dept': 2,
                    'name': 'Mark'
                },
                {
                    'age': 11,
                    'dept': 2,
                    'name': 'Tom'
                }
            ]
        }
    },
    {
        1: {
            10: [
                {
                    'age': 10,
                    'dept': 1,
                    'name': 'Sam'
                }
            ]
        },
        {
            12: [
                {
                    'age': 12,
                    'dept': 1,
                    'name': 'John'
                }
            ]
        }
    }
]

Using this code:

import itertools, operator

l = [{'dept':1, 'age':10, 'name':'Sam'},
        {'dept':1, 'age':12, 'name':'John'},
        {'dept':2,'age':20, 'name':'Mary'},
        {'dept':2,'age':11, 'name':'Mark'},
        {'dept':2,'age':11, 'name':'Tom'}]

groups = ['dept', 'age', 'name']

groups.reverse()
def hierachical_data(data, groups):
    g = groups[-1]
    g_list = []
    for key, items in itertools.groupby(data, operator.itemgetter(g)):
        g_list.append({key:list(items)})
    groups = groups[0:-1]
    if(len(groups) != 0):
        for e in g_list:
            for k,v in e.items():
                e[k] = hierachical_data(v, groups)

    return g_list

print hierachical_data(l, groups)

This method works fine, but now I need to optimize. The dictionaries has a big memory overhead and this grouping gets pretty slow when we are dealing with huge number of records.

I was wondering if there is any algorithm I could use to reduce the time needed to do the grouping.

P.S: I wouldn't mind changing the data-structure as long as it gives me the same hierarchical format, or any better suggestion of course.

Thanks.

Try doing a recursive sequence that iterates through each group, by passing a group index to each call, and find and build the tree as it receives inputs:

def generate_hierarchical_hash(entries, groups):
  """Returns hierarchical hash of entries, grouped at each level by the groups"""
  hierarchy = {}
  for to_insert in entries:
    insert_into_hierarchy(to_insert, hierarchy, groups, 0)
  return hierarchy

def insert_into_hierarchy(to_insert, hierarchy, groupings, current_grouping_level):
  if current_grouping_level == len(groupings):
    # if at final level, do not do anything - protection case
    pass
  else:
    grouping_at_level = groupings[current_grouping_level]
    grouping_key = to_insert[grouping_at_level]
    is_final_group = current_grouping_level == len(groupings) - 1
    if is_final_group:
      # if at last grouping, nodes will contain lists, so create or retrieve list
      # and append value to it
      if grouping_key in hierarchy:
        list_for_group = hierarchy[grouping_key]
      else:
        # create new list and insert it into hierarchy
        list_for_group = list()
        hierarchy[grouping_key] = list_for_group

      # insert to insert into end of list
      list_for_group.append(to_insert)
    else:
      # otherwise, find or create hash for this grouping,
      if grouping_key in hierarchy:
        hierarchy_for_group = hierarchy[grouping_key]
      else:
        # create hash and insert into parent hash
        hierarchy_for_group = {}
        hierarchy[grouping_key] = hierarchy_for_group
      # recursive call, go down a level and group
      insert_into_hierarchy(to_insert, hierarchy_for_group, groupings, current_grouping_level + 1)

This could be called like:

input =[{'dept':1, 'age':10, 'name':'Sam'},
        {'dept':1, 'age':12, 'name':'John'},
        {'dept':2,'age':20, 'name':'Mary'},
        {'dept':2,'age':11, 'name':'Mark'},
        {'dept':2,'age':11, 'name':'Tom'}]

groups = ['dept', 'age', 'name']

hierarchical_hash = generate_hierarchical_hash(input, groups)


Architecture for manager that gets shared among different components It started with a simple log manager. Then I wanted to implement more features, for example printing the name of the component that send a message. Later on, I can think of giving each component that prints log messages it's own text color in the terminal and options to mute or solo single modules.

Therefore, the manager needs the identity of the component that called its methods. So I decided to wrap the manager with an instance for every component. It tunnels function calls to the manager together with the name of the component.

#include <iostream>
#include <string>
#include <memory>
#include <unordered_map>
using namespace std;

////////////////////////////////////////////////////////////////
// Manager declaration
////////////////////////////////////////////////////////////////

class manager {
public:
    class instance;

    instance &get_instance(string name);
    void print(string sender, string message);

private:
    unordered_map<string, unique_ptr<instance>> m_instances;
};

////////////////////////////////////////////////////////////////
// Instance declaration
////////////////////////////////////////////////////////////////

class manager::instance {
public:
    instance(string name, manager &manager);
    void print(string message);

private:
    string m_name;
    manager &m_manager;
};

////////////////////////////////////////////////////////////////
// Manager implementation
////////////////////////////////////////////////////////////////

manager::instance &manager::get_instance(string name) {
    if (m_instances.find(name) == m_instances.end())
        m_instances[name] = unique_ptr<instance>(new instance(name, *this));
    return *m_instances[name].get();
}

void manager::print(string sender, string message) {
    cout << sender << ": " << message << "." << endl;
}

////////////////////////////////////////////////////////////////
// Instance implementation
////////////////////////////////////////////////////////////////

manager::instance::instance(string name, manager &manager) : m_name(name),
    m_manager(manager) {}

void manager::instance::print(string message) {
    m_manager.print(m_name, message);
}

////////////////////////////////////////////////////////////////
// Main function
////////////////////////////////////////////////////////////////

int main() {
    manager m;
    m.get_instance("Window").print("Open up");
    m.get_instance("Asset").print("Load textures");
    return 0;
}

Here is a working example of my code. What do you think about the class structure and architecture? Is there an easier or more expressive way? I'd like to get feedback and improve the code structure before implementing other managers the same way.

About the architecture and thinking exclusively on the given example application (logging), I believe manager does not need to track all manager::instance instances. Maybe in other applications this might be needed, but not in this one. Thus, I would just turn get_instance into a convenience method (make_instance) to create new manager::instances:

class manager {
public:
    class instance;

    std::unique_ptr<instance> make_instance(const std::string& name);
    void print(const std::string& sender, const std::string& message);
};

std::unique_ptr<manager::instance> manager::make_instance(const std::string& name) {
    return std::unique_ptr<instance>(new instance(name, *this));
}

If you plan using the manager like a singleton (having only one instance and everyone can access it), maybe the manager and manager::instance classes could be merge into a single one, where manager members would become static and manager::instance would become an actual instance of manager.

Please note that both simplifications above may not apply to other applications.

Now, the general code review comments (some of which were already applied on the snippet above):

The code is very well organized, very easy to read and understand. The coding style is consistent. I have only 3 suggestions for improvement:

Always pass std::string to a function as const std::string& instead of just std::string to avoid an unnecessary copy.

Avoid using namespace std;. If, for instance, you really want to use some std symbols unqualified, import them explicitly (like using std::string;). Also avoid using declarations on headers where they may affect user code (in global or namespace scope), otherwise it may break other's code when they include your header.

Also, use braces after if, while, do and for. Those kind of bugs (adding another indented line thinking it inside a block) are hard to spot.


Code that checks if two words are anagrams I've completed a program that tests whether two words are anagrams. The program seems to work, but I was just wondering if there is anything I could be doing to make the code better. Everything is below, there is a main program and one class.

Main:

import java.util.Scanner;

public class StringTesting
{
    public static void main(String[] args)
    {
        Scanner input = new Scanner(System.in);
        StringUtil object = new StringUtil();

        String word1;
        String word2;

        System.out.print("Enter word one: ");
        word1 = input.next();

        System.out.print("Enter word two: ");
        word2 = input.next();

        if(object.isAnagram(word1, word2)){
            System.out.println("It's an anagram!");
        } else {
            System.out.println("It's not an anagram.");
        }
    }
}

StringUtil Class:

public class StringUtil
{
    public int countCharacters(String str, char ch)
    {
        int count = 0;

        for(byte i = 0; i < str.length(); i++){
            if(str.toLowerCase().charAt(i) == Character.toLowerCase(ch)){
                count++;
            }
        }

        return count;
    }

    public boolean isAnagram(String str1, String str2)
    {
        int count = 0;

        if(str1.length() != str2.length()){
            return false;
        }

        for(byte i = 0; i < str1.length(); i++){
            char ch = str1.toLowerCase().charAt(i);
            for(byte j = 0; j < str1.length(); j++){
                char ch2 = str2.toLowerCase().charAt(j);
                if(ch == ch2){
                    if(countCharacters(str1, ch) != countCharacters(str2, ch2)){
                        return false;
                    } else {
                        count++;
                    }
                }
            }
        }

        if(count == str1.length()){
            return true;
        } else {
            return false;
        }
    }
}

The algorithm in isAnagram is far from being efficient.

Consider two strings: each is a letter "a" repeated 1000 times. We will count the occurrences of 'a' in both strings 1000 times - each time getting the same answer. In other words, the worst case complexity is O(len(input)^2).

It's possible to cache the number of occurrences of a given character in a Map while retaining the rest of your current algorithm. This would speed up most cases, but the worst case complexity will still be O(len(input)^2) (consider an anagram where no letter occurs twice, and is a reverse of the first string).

What we could do is something like this:

Iterate once through each string to create a HashMap with the count of each character's occurrences.
If the two maps are equal, we have an anagram. Otherwise, we don't.

The complexity would be O(len(input1) + len(input2)).


Assigning browser cookies for A-B testing The following code creates a cookie on the client browser such that 25% of the browsers will have a value 0, and 75% a value of 1. Can anyone review the following code and point out any problems in it?

// AB testing code starts;
$abTestValues = array(1,1,0,1);
$abTestValue = 0;
$abTestCurrentValue = $abTestValues[$abTestValue];
$cookieName = 'ab-ranking-20160301';
$cookieLifeTime = time() + (86400 * 30);
$cookiePath = "/";
$abTestCookieValue = $_COOKIE[$cookieName];
if(trim($abTestCookieValue) != '' && $abTestCookieValue != null){
$abTestCurrentValue = $abTestCookieValue;
}else{
$abTestFile = 'abranking.txt';
if(is_file($abTestFile)){
$abTestLastValue = file_get_contents($abTestFile);
if( ($abTestLastValue + 1) == sizeof($abTestValues) ){
$abTestValue = 0;
}else{
$abTestValue = $abTestLastValue+1;
}
$abTestCurrentValue = $abTestValues[$abTestValue];
}
file_put_contents($abTestFile , $abTestValue);
$ranking_cookieval = $abTestCurrentValue;

setcookie($cookieName,$abTestCurrentValue,$cookieLifeTime,$cookiePath); //cookie set for 30 day
}
// AB testing code ends;

That code, without indentation, is a chore to read.  The same code, with just indentation and whitespace changes, is easier to follow.

// AB testing code starts;
$abTestValues = array(1,1,0,1);
$abTestValue = 0;
$abTestCurrentValue = $abTestValues[$abTestValue];
$cookieName = 'ab-ranking-20160301';
$cookieLifeTime = time() + (86400 * 30);
$cookiePath = "/";
$abTestCookieValue = $_COOKIE[$cookieName];
if (trim($abTestCookieValue) != '' && $abTestCookieValue != null) {
    $abTestCurrentValue = $abTestCookieValue;
} else {
    $abTestFile = 'abranking.txt';
    if (is_file($abTestFile)) {
        $abTestLastValue = file_get_contents($abTestFile);
        if (($abTestLastValue + 1) == sizeof($abTestValues)) {
            $abTestValue = 0;
        } else {
            $abTestValue = $abTestLastValue + 1;
        }
        $abTestCurrentValue = $abTestValues[$abTestValue];
    }
    file_put_contents($abTestFile, $abTestValue);
    $ranking_cookieval = $abTestCurrentValue;

    setcookie($cookieName, $abTestCurrentValue, $cookieLifeTime, $cookiePath); //cookie set for 30 day
}
// AB testing code ends;


Creating orders for gift transactions Can this be improved?

foreach (Gift gift in usedGifts)
{
    foreach (GiftTransaction GiftTransaction in gift.GiftTransactions)
    {
        if (!string.IsNullOrEmpty(giftTransaction.GiftId) &&
            !orderList.Where(b => b.GiftId == giftTransaction.GiftId).Any())
        {
            Order order = OrderHelper.PopulateSingleOrder(orderRepository, sessionRepository, giftTransaction.GiftId);
            if (order != null)
                orderList.Add(order);
        }
    }
    giftReferenceList.Add(gift.GiftReference);
}

I'm not sure if it will make your query faster but here is my take on it:

If I read the code correctly you want to create orders for all gift transactions related to gift ids which are not covered by an order yet.

usedGifts.SelectMany(g => g.GiftTransactions)
         .Select(gt => gt.GiftId)
         .Where(gi => !string.IsNullOrEmpty(gi))
         .Except(orderList.Select(o => o.GiftId)) // This will yield all gift ids which are not covered by an order
         .Select(gi => OrderHelper.PopulateSingleOrder(orderRepository, sessionRepository, gi))  // generate the orders
         .Where(o => o != null)
         .ToList();

Unfortunately this requires an extra pass over the usedGifts list to add all the references

giftReferenceList.AddRange(usedGifts.Select(g => g.GiftReference));


Check for balanced parentheses I was given an assignment to check for balanced parentheses from an arbitrary string where parentheses are defined as (, [ or { and their respective "closing" parentheses. The other requirement for the assignment was that I had to use either Stack<T> , Queue<T> or List<T>.

Here is my implementation:

private static void CheckParentheses()
{
    Console.Write("Please enter the string you wist to check formity of: ");
    var input = Console.ReadLine();
    var stack = new Stack<char>();

    char[] allowedChars = { '(', '[', '{', ')', ']', '}' };

    foreach (char chr in input)
        if (allowedChars.Any(x => x == chr))
            stack.Push(chr);

    var reverseStack = stack.Reverse();
    var sequencedBalanced = reverseStack.SequenceEqual(stack, BalancedParanthesisComparer.Instance);
    Console.WriteLine("The input string was {0}well formed", sequencedBalanced ? string.Empty : "NOT ");
}

public sealed class BalancedParanthesisComparer : EqualityComparer<char>
{
    private static readonly BalancedParanthesisComparer _instance = new BalancedParanthesisComparer();

    private BalancedParanthesisComparer() { }

    public static BalancedParanthesisComparer Instance { get { return _instance; } }

    public override bool Equals(char x, char y)
    {
        if ((x == '(' && y == ')') || (y == '(' && x == ')'))
            return true;
        if ((x == '[' && y == ']') || (y == '[' && x == ']'))
            return true;
        if (x == '{' && y == '}' || (y == '{' && x == '}'))
            return true;

        return false;
    }
}

I would like general feedback and would also like to know: regarding the use of an EqualityComparer to do something that isn't really equality-checking, is it very bad practice or is it fine?

Aside from stylistic concerns (as a Java/Clojure guy, I'll let you C#'ers decide on that stuff), I don't see how this a correct algorithm, unless additional constraints are given in the problem statement. Consider this input:

([] ())

Now, clearly that is balanced. And if you reverse it, you get:

))( ][(

But if you compare this reversed input with the original input, you don't find that each open bracket matches up with its closing bracket in the reversed string: the first character and the last are fine, but the inner pairs don't balance at all!

So, unless you are given the further guarantee that the input consists of N opening brackets followed by N closing brackets (ie, no opens after the first close), you can't just compare the reversed string.


C++ simple event system I posted an earlier version of this code a couple of days ago, so it's the updated version of the code posted here.

#include <iostream>
#include <functional>
#include <conio.h>
#include <unordered_map>
#include <string>

class EventListenerHandle
{
        template <typename U>
        friend class EventChannel;
    private:
        std::function<void()> removeFunction;
        bool removed;
        EventListenerHandle() = delete;
        inline EventListenerHandle(std::function<void()> remove_function) :removed(false)
        {
            this->removeFunction = remove_function;
        }
    public:
        inline void remove()
        {
            this->removed = true;
            removeFunction();
        }
};

template <typename T>
class EventChannel
{
private:
    using idType = unsigned int;
    static const idType& getNextId()
    {
        static idType id = 0;
        return id++;
    }

    static std::unordered_map<idType, std::function<bool(const T&)>>& getHandlers()
    {
        static std::unordered_map<idType, std::function<bool(const T&)>> handlers;
        return handlers;
    }
    static void removeListener(idType listener_id)
    {
        getHandlers().erase(listener_id);
    }
public:
    static EventListenerHandle registerListener(std::function<bool(const T&)> listener)
    {
        auto& id = getNextId();
        getHandlers()[id] = listener;
        return EventListenerHandle(std::bind(EventChannel<T>::removeListener, id));

    }
    static void tringgerEvent(T e)
    {
        for (const auto& h : getHandlers())
        {
            h.second(e);
        }
    }

};
/*to force the caller to  pass the bare type (not pointer or reference) as the templapte parameter
if these where not here one could (and possibly would) write some code like:
EventChannel<TouchEvent&>::registerListener([](const TouchEvent& ev)->bool
{
});
TouchEvent t;
DISPATCH_EVENT(t);
and expect the listener to be called. so it's better to prevent them from
using pointer and reference types as template parameter for EventChannel class.
also i decided not to care for volatile types.

*/

template<typename T>
class EventChannel<T*>{};

template<typename T>
class EventChannel<T&>{};

template <typename T>
class EventChannel<const T>{};

//just for convenience
template <typename T>
void DISPATCH_EVENT(const T& t)
{
    EventChannel<T>::tringgerEvent(t);
    //std::cout << t.eventType();
}
//to let the caller pass pointers as the event object
template <typename T>
void DISPATCH_EVENT(T* t)
{
    DISPATCH_EVENT<T>(*t);
}
#define EVET_TYPE_FUNC(T) static std::string eventType(){return #T;}
#define KDFA_DEFINE_VOID_EVENT(__NAME__)    struct __NAME__{EVET_TYPE_FUNC(__NAME__)};
struct TouchEvent
{
    int t_id;
    float x;
    float y;
    EVET_TYPE_FUNC(TouchEvent);
};

KDFA_DEFINE_VOID_EVENT(CAMERA_MOVED_OR_ZOOMED);
void main()
{
    auto lid = EventChannel<TouchEvent>::registerListener([](const TouchEvent& ev)->bool
    {
        std::cout << "got touch " << ev.t_id << "\n";
        return false;
    });

    TouchEvent t;
    t.t_id = 10;

    DISPATCH_EVENT(t);//dispatch with object
    DISPATCH_EVENT(&t);// dispatch with pointer

    //it's best to use RAII to correctly call remove on event listener handles
    lid.remove();//remove listener
    lid.remove();//calling remove multiple times would not cause any problems. this is a mere design decision

    DISPATCH_EVENT(t);

    _getch();
}

consider reading the main function first, it's pretty simple to understand. As I lost the ability to think about this design out of the box, any comment is welcomed. Thanks

Fix getNextId

static const idType& getNextId()
{
   static idType id = 0;
   return id++;
}

is not right.

There is no need to return a const idType&. It can be just idType.
If you want to return a const idType&, you need to change the return statement to:

return ++id;

Using

return id++;

has couple of problems.

The value of the first ID will be 0, not 1. Is that your intention?
id++ is an rvalue, not an lvalue. It is like:

int temp = id;
++id;
return temp;

I get the following compiler warning with your code using g++ 4.8.4:

 warning: returning reference to temporary [-Wreturn-local-addr]
  return id++;

In conclusion, I suggest:

static idType getNextId()
{
   static idType id = 0;
   return ++id;
}

Fix return type of main

You have

void main() { ... }

The standard requires the return type of main to be int. Change it to:

int main() { ... }

Change physical layout of the classes

The public section of a class are most important to users. Hence, I recommend the pubilc section listed first. If we could, we could completely hide the private section from the users. Hence, I recommend the private section to be listed last.

class EventListenerHandle
{
   public:

      // Add the public members

   private:

      // Add the public members
};

Of course, I recommend this change for Channel too.

Unused macro

You have

#define KDFA_DEFINE_VOID_EVENT(__NAME__)    struct __NAME__{EVET_TYPE_FUNC(__NAME__)};

It's not used anywhere in your posted code. It's not clear what the purpose of the macro is.

Unclear purpose of macro

You also have

#define EVET_TYPE_FUNC(T) static std::string eventType(){return #T;}

which is used in:

struct TouchEvent
{
   int t_id;
   float x;
   float y;
   EVET_TYPE_FUNC(TouchEvent);
};

However, removing the line

   EVET_TYPE_FUNC(TouchEvent);

from TouchEvent does not create any compiler or linker errors. It's not clear what the purpose of creating eventType() is.

Don't use ALL CAPS for functions

You have

template <typename T>
void DISPATCH_EVENT(const T& t)
{
   EventChannel<T>::tringgerEvent(t);
}

template <typename T>
void DISPATCH_EVENT(T* t)
{
   DISPATCH_EVENT<T>(*t);
}

All caps are usually reserved for macros, not for functions. Change them to:

template <typename T>
void dispatchEvent(const T& t)
{
   EventChannel<T>::tringgerEvent(t);
}

template <typename T>
void dispatchEvent(T* t)
{
   // No need for explicit use of T here.
   // That should be correctly deduced.
   // dispatchEvent<T>(*t);
   dispatchEvent(*t);
}

Registering and removing listeners is not symmetric

You have:

EventChannel<T>::registerListener() { ... }

for registering listeners but removing listeners is accomplished using

EventListenerHandle::remove()

I recommend adding a function removeListener in EventChannel.

EventChannel<T>::removeListener(EventListenerHandle handle) { ... }

If you do that, the only member data needed in EventListenerHandle will be just the ID of the event.

Suggested event handling code

#include <iostream>
#include <functional>
#include <unordered_map>
#include <string>

struct EventListenerHandle
{
   unsigned int id;
};

template <typename T>
class EventChannel
{
   public:

      using idType = unsigned int;

      static EventListenerHandle registerListener(std::function<bool(const T&)> listener)
      {
         auto id = getNextId();
         getHandlers()[id] = listener;
         return EventListenerHandle{id};
      }

      static void removeListener(EventListenerHandle handle)
      {
         getHandlers().erase(handle.id);
      }

      static void tringgerEvent(T e)
      {
         for (const auto& h : getHandlers())
         {
            h.second(e);
         }
      }

   private:
      static idType getNextId()
      {
         static idType id = 0;
         return ++id;
      }

      static std::unordered_map<idType, std::function<bool(const T&)>>& getHandlers()
      {
         static std::unordered_map<idType, std::function<bool(const T&)>> handlers;
         return handlers;
      }
};

template<typename T>
class EventChannel<T*>{};

template<typename T>
class EventChannel<T&>{};

template <typename T>
class EventChannel<const T>{};

template <typename T>
void dispatchEvent(const T& t)
{
   EventChannel<T>::tringgerEvent(t);
}

template <typename T>
void dispatchEvent(T* t)
{
   dispatchEvent(*t);
}

Test code

struct TouchEvent
{
   int t_id;
   float x;
   float y;
};

int main()
{
   auto lid = EventChannel<TouchEvent>::registerListener([](const TouchEvent& ev)->bool
                                                         {
                                                         std::cout << "got touch " << ev.t_id << "\n";
                                                         return false;
                                                         });

   TouchEvent t;
   t.t_id = 10;

   dispatchEvent(t);  // dispatch with object
   dispatchEvent(&t); // dispatch with pointer

   EventChannel<TouchEvent>::removeListener(lid);

   dispatchEvent(t);
}


DuplicateDictionary - A dictionary-like class which allows duplicates Since I couldn't find any good answers I made my own class called DuplicateDictionary for personal usage. I would like some tips to improve it.

public class DuplicateDictionary<TKey, TValue>: List<KeyValuePair<TKey, TValue>>
{
    public DuplicateDictionary()
    {

    }

    public DuplicateDictionary(List<KeyValuePair<TKey, TValue>> list)
    {
        foreach(KeyValuePair<TKey, TValue> kvp in list)
        {
            this.Add(kvp);
        }
    }

    public DuplicateDictionary(Dictionary<TKey, TValue> dictionary)
    {
        foreach(KeyValuePair<TKey, TValue> kvp in dictionary)
        {
            this.Add(kvp);
        }
    }

    public TValue this[TKey index]
    {
        get
        {
            this.ContainsKey(index);
            return this[index];
        }
        set
        {
            this[index] = value;
        }
    }

    public void Add(TKey key, TValue value)
    {
        this.Add(new KeyValuePair<TKey, TValue>(key, value));
    }

    public bool ContainsKey(TKey key)
    {
        foreach(KeyValuePair<TKey, TValue> kvp in this)
            if(kvp.Key.Equals(key)) return true;

        return false;
    }
}

This class is useful for log things. I use it to log a calculator's past functions and numbers, and once equals is pressed, adds all the past numbers with the functions.

Such a data structure already exists, and is called ILookup<TKey, TElement>. It can be created using the ToLookup extension.

  The ToLookup<TSource, TKey>(IEnumerable<TSource>, Func<TSource, TKey>) method returns a Lookup<TKey, TElement>, a one-to-many dictionary that maps keys to collections of values. A Lookup<TKey, TElement> differs from a Dictionary<TKey, TValue>, which performs a one-to-one mapping of keys to single values.

var list = new List<Tuple<string, int>>
{
    Tuple.Create("a", 1),
    Tuple.Create("a", 2),
    Tuple.Create("a", 3),
    Tuple.Create("b", 4),
    Tuple.Create("c", 5)
};

var lookup = list.ToLookup(t => t.Item1, t => t.Item2);

foreach(var kv in lookup)
{
    Console.Write(kv.Key);
    Console.WriteLine(" - " + string.Join(", ", kv));
}

// prints
// a - 1, 2, 3
// b - 4
// c - 5

As Vince pointed out, your DuplicateDictionary will suffer from slow access. Lookup won't.


Date Class, implemented I'm implementing this date class based on <ctime>
I'll appreciate your comments, critics and advice.

Please review, thank you!.

Date.h

/*
 * Date.h
 *
 * Usage:
 * Create object, and then call Set(day, month, year)
 * Throws invalid argument exception if Teb 29th is set with the wrong year (is not leap year)
 * TODO: throw invalid argument exception if date is invalid
 * Use GetDate() to get date formatted as "dd/mm/yyy"
 * Use GetAscTime() to get date formatted as "ddd mmm yy hh:mm:ss yyyy"
 * Substraction can be done between dates, it will return the differences in days
 *  i.e. Date today - Date yesterday = 1
 * TODO: overload + operator as Date + int
 * TODO: overload - operator as Date - int
 */
#include <ctime>
#include <string>

#ifndef DATE_H_
#define DATE_H_

class Date
{
public:
    Date();
    void Set(int day, int month, int year);
    int operator - (const Date& rhs);
    const char* GetDate() const;
    const char* GetAscTime() const;
    time_t GetDateValue() const;

private:
    std::time_t ttTime;
    std::tm tmTime;
    mutable char szTimeBuffer[255];
    static constexpr int nSecPerDay = 60 * 60 * 24;
};

#endif /* DATE_H_ */

Date.cpp

/*
 * Date.cpp
 */
#include "Date.h"
#include <stdexcept>

Date::Date()
    : ttTime(0), tmTime({0}), szTimeBuffer("")
{
    std::time(&ttTime);
    localtime_s(&tmTime, &ttTime);
}

void Date::Set(int day, int month, int year)
{
    tmTime.tm_mday = day;
    tmTime.tm_mon = month - 1;
    tmTime.tm_year = year - 1900;

    ttTime = std::mktime(&tmTime);

    // Temp date constructed to check if Feb 29th for the specified year exists
    // checks if the value of 29/Feb/YYYY is the same as 01/Mar/YYYY
    // if true, means that year is not leap year and should not have Feb 29th
    // throws invalid argument exception

    std::tm tmTemp = {0};
    std::time_t ttTemp = 0;

    // Specify march 1st, use same year as input
    tmTemp.tm_mday = 1;
    tmTemp.tm_mon = month;
    tmTemp.tm_year = tmTime.tm_year;

    ttTemp = std::mktime(&tmTemp);

    if(ttTemp == ttTime)
    {
        std::string errMsg("Error!, Feb 29th doesn't exist in year ");
        errMsg.append(std::to_string(tmTime.tm_year + 1900));
        errMsg.append("!!");
        throw( std::invalid_argument(errMsg) );
    }

}

int Date::operator -(const Date& rhs)
{
    return (std::difftime(ttTime, rhs.ttTime) / nSecPerDay);
}

const char* Date::GetDate() const
{
    std::strftime(szTimeBuffer, 255, "%d/%m/%y", &tmTime);
    return szTimeBuffer;
}

const char* Date::GetAscTime() const
{
    asctime_s(szTimeBuffer, 255, &tmTime);
    return szTimeBuffer;
}

time_t Date::GetDateValue() const
{
    return ttTime;
}

A few things I see:

The subtraction(-) operator would make more sense to return a Date object instead of an int.  This would also fit in with standard practice.  If such a function is needed, I would suggest naming a specific function for that,DiffDays perhaps.  This has the advantage being more intuitive for the user.

Instead of requiring each instance of the Date object to require the user calling Set, it would be more intuitive to overload the constructor.

It seems to me that returning a string instead of a char* would be more in fitting with a c++ program.


Reversing a word or sentence Was just wanting some general feedback on the below code regarding
1. Readability
2. Structure
3. Efficiency

import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Scanner;

/**
 * Created on 5/27/2016.
 *
 * Simple functions used to reverse either a sentence or a word.
 */

public class Reverse {

    /**
     * The main function is used to retrieve user input and apply it to one of the
     * two reverse functions.
     * @param args Not used in this application.
     */
    public static void main(String[] args){

        Scanner userInput = new Scanner(System.in);

        // Gets user input.
        System.out.print(">>> ");
        String input = userInput.nextLine();

        if(input.contains(" ")) // If contains space its a sentence
            System.out.println(reverse(Arrays.asList(input.split(" "))));
        else
            System.out.println(reverse(input));
    }

    /**
     * Used to reverse the order of given list.
     * @param words The list of words that will be reversed then returned.
     * @return The result of the reversed parameter.
     */
    private static String reverse(List<String> words){
        Collections.reverse(words); // Reverses order of words.
        return String.join(" ", words);
    }

    /**
     * Used to reverse the order of a single word.
     * @param word The string word that will be loaded into a list one character at a time.
     * @return The result of the reversed parameter.
     */
    private static String reverse(String word){
        List<String> letters = Arrays.asList(word.split("")); // Stores each letter of word into list.
        Collections.reverse(letters); // Reverses the order of the list.
        return String.join("", letters);
    }
}

It's unfortunate that there are two reverse() functions that do similar things.  What is even more disconcerting is that they are inconsistent: main() is responsible for splitting a sentence into words, but reverse(List<String> words) joins them back up.  On the other hand, reverse(String word) doesn't work that way: it does both the splitting and joining.

Why not solve both problems consistently with one function?

import java.util.*;
import java.util.regex.Pattern;

public class Reverse {
    private Reverse() {}  // Suppress the default constructor

    /**
     * Splits the input on the delimiter, reverses the list, then joins the
     * results with the delimiter.
     */
    public static String reverse(String s, String delimiter) {
        List<String> list = Arrays.asList(s.split(Pattern.quote(delimiter)));
        Collections.reverse(list);
        return String.join(delimiter, list);
    }

    public static void main(String[] args) {
        System.out.print(">>> ");
        String input = new Scanner(System.in).nextLine();

        if (input.contains(" ")) {
            // Reverse words in a sentence
            System.out.println(reverse(input, " "));
        } else {
            // Reverse characters in a word
            System.out.println(reverse(input, ""));
        }
    }
}

Note that there is a technicality, if reverse() is exposed as a public function as I have done.  In that case, you would expect it to work for any delimiter, so the delimiter must be quoted such that String.split() treats it as a literal string rather than a regular expression.


JWT authentication between Django and ReactJS I am currently using Django (2.1) to build an API, and I have added djangorestframework-jwt to manage JWT.

Here is the configuration:

JWT_AUTH = {
    'JWT_SECRET_KEY': SECRET_KEY,
    'JWT_VERIFY': True,
    'JWT_VERIFY_EXPIRATION': True,
    'JWT_EXPIRATION_DELTA': datetime.timedelta(days=14),
    'JWT_ALLOW_REFRESH': True,
    'JWT_REFRESH_EXPIRATION_DELTA': datetime.timedelta(days=7),
    'JWT_AUTH_HEADER_PREFIX': 'Bearer',
}

and the endpoints:

urlpatterns = [
    path('auth/get-token/', obtain_jwt_token),
    path('auth/refresh-token/', refresh_jwt_token),
]

The client is built with ReactJS. I use an axios instance as a client to communicate with the API. This instance is created that way :

import axios from 'axios'
import jwt_decode from 'jwt-decode' // eslint-disable-line
import { signOut } from '../actions/authActions'

const signOutOn401 = (statusCode) => {
  if (statusCode === 401) {
    signOut()
    window.location = '/signin'
  }
}

const client = axios.create({
  baseURL: process.env.API_URL,
  headers: {'Authorization': ''}
})

/*
 * This interceptor is used for:
 * - adding Authorization header if JWT available
 * - refreshing JWT to keep user authenticated
 */
client.interceptors.request.use((config) => {
  if (window.localStorage.getItem('token')) {
    let token = window.localStorage.getItem('token')
    // Calculate time difference in days
    // between now and token expiration date
    const t = ((jwt_decode(token).exp * 1000) - Date.now()) / 1000 / 60 / 60 / 24
    // Refresh the token if the time difference is
    // smaller than 13 days (original token is valid 14 days)
    if (t < 13) {
      axios.post(`${process.env.API_URL}/auth/refresh-token/`, {
        token: token
      })
        .then(({data}) => {
          token = data.token
        })
        .catch((error) => {
          signOutOn401(error.response.status)
          return error
        })
    }
    config.headers['Authorization'] = `Bearer ${token}`
  }

  return config
})

/*
 * This interceptor is used for:
 * - disconnect user if JWT is expired or revoked
 */
client.interceptors.response.use(
  (response) => {
    return response
  },
  (error) => {
    signOutOn401(error.response.status)
    return error
  }
)

export default client

The signout action only clear the session and clean the store :

export const signOut = () => {
  window.localStorage.clear()

  return ({
   type: SIGN_OUT,
    payload: {
      authenticated: false,
      user: {},
      errorMessage: ''
    }
  })
}

Everything looks working fine, I would just like to know if that implementation is correct, and if there is no security flaw :)

UX concern: refresh period

I wonder if you are confusing the access token expiration setting (JWT_EXPIRATION_DELTA) with the refresh token expiration (JWT_REFRESH_EXPIRATION_DELTA). In either case, your t < 13 check should be related to the refresh token expiration, not the access token expiration. 13 seems to be chosen because it is almost 14; hence my comment. At the very least these refreshes should never go beyond your refresh expiration (7 days), or they will fail, which defeats the purpose of providing a refresh token (because the user would always have to authenticate after the expiration).

Security concern: access token expiration

On a slightly similar note, but this one is a security concern -- you typically want the access token to have a much shorter life than the refresh token. Yours is reversed, as the access token (JWT_EXPIRATION_DELTA) is 14 days vs. the refresh token (JWT_REFRESH_EXPIRATION_DELTA) at 7 days. Note the default JWT_EXPIRATION_DELTA for the djangorestframework-jwt library is a much more conservative and standard 5 minutes:

  Default is datetime.timedelta(seconds=300)(5 minutes).

Per OAuth 2 spec:

  access tokens may have a shorter lifetime and fewer permissions than
  authorized by the resource owner

Per Auth0 guidance:

  Access tokens carry the necessary information to access a resource directly.

  ...

  Access tokens usually have an expiration date and are short-lived.

  ...

  Common implementations allow for direct authorization checks against
  an access token. That is, when an access token is passed to a server
  managing a resource, the server can read the information contained in
  the token and decide itself whether the user is authorized or not (no
  checks against an authorization server are needed). This is one of the
  reasons tokens must be signed (using JWS, for instance). On the other
  hand, refresh tokens usually require a check against the authorization
  server. This split way of handling authorization checks allows for
  three things:

  Improved access patterns against the authorization server (lower load, faster checks)
  Shorter windows of access for leaked access tokens (these expire quickly, reducing the chance of a leaked token allowing access to a
  protected resource)

  ...

Security concern: refresh token revocation

It is difficult for me to tell whether that djangorestframework-jwt library will revoke all previous refresh tokens for a given session once a new refresh token is issued for that session. Ideally it should, especially if you are using unauthenticated clients, which it looks like you are (assuming that React app is just a public web app; if it's instead packaged in a native client you may and probably should be authenticating that client). You may want to verify that behavior of the library with your own testing.

If the library does not revoke previous refresh tokens, then you should probably mitigate the risk of having so many outstanding valid refresh tokens per session (~= expiration time divided by refresh period, or currently 7 / 1 = 7 valid tokens!!!) by making the refresh period much closer to the refresh token expiration. So if you keep the refresh expiration at 7 days, then only refreshing at 6 days would make sense. Then you would typically have 1 or at most 2 outstanding valid refresh tokens for a session. However, if given a choice, it is much more secure to have automatically revoked refresh tokens and shorter refresh periods; that way a compromised refresh token is much more likely to be invalid -- and you still wouldn't have to lose the benefit of the authorized user having a long refresh expiration. (The whole point of the long expiration is for situations such as an unattended laptop or mobile browser.)

Per OAuth 2 spec:

  The authorization server MUST verify the binding between the refresh
  token and client identity whenever the client identity can be
  authenticated.  When client authentication is not possible, the
  authorization server SHOULD deploy other means to detect refresh
  token abuse.

  For example, the authorization server could employ refresh token
  rotation in which a new refresh token is issued with every access
  token refresh response.  The previous refresh token is invalidated
  but retained by the authorization server.  If a refresh token is
  compromised and subsequently used by both the attacker and the
  legitimate client, one of them will present an invalidated refresh
  token, which will inform the authorization server of the breach.

  The authorization server MUST ensure that refresh tokens cannot be
  generated, modified, or guessed to produce valid refresh tokens by
  unauthorized parties.


Shader class implementation I'm trying to design a Shader class.  For now, I'm just trying to get basic outline for handling uniforms. Some design goals I aimed for is to avoid passing a string to a function as this is error prone and has no type checking. There's really only one place this can go wrong:  if you mistype the name of the uniform when defining it.

There is a lot of potential for code bloat.  I could potentially move the Name out of the template parameters for Parameter and store it as a member variable, which would get assigned in the default constructor.

template<typename ParamType>
class Shader
{
public:
    template<typename Type, const char* (*Name)()>
    class Parameter
    {
        static const char* name;
        static GLuint location;
        static GLuint program;

        template<typename> friend class Shader;
    };

    template<typename Type, const char* (*Name)()>
    void SetParameter(const Parameter<Type,Name>&, const Type& value )
    {
        // static_assert(!std::is_same<Type,Type>::value, "Unsupported type, needs specialization.");
        typedef Parameter<Type,Name> Param;

        if(program != -1 && Param::program != program)
        {
            // set Param::location
            Param::program = program;
        }

        std::cout << Param::name << std::endl;
    }

private:

    GLuint program;

};

template<typename ParamType>
template<typename Type, const char* (*Name)()>
const char* Shader<ParamType>::Parameter<Type, Name>::name = Name();

template<typename ParamType>
template<typename Type, const char* (*Name)()>
GLuint Shader<ParamType>::Parameter<Type, Name>::program = -1;

#include "shaderparameters.inl"

shaderparameters.inl:

#define MY_PARAMETER(shader, type, name) \
    private: static const char* ShaderParameterStr##shader##type##name##() { return #name ; }  \
    public:  typedef Shader< shader >::Parameter< type, &ShaderParameterStr##shader##type##name > name;

struct ShaderParam
{
    struct Generic
    {
        MY_PARAMETER( Generic, Vec4, position )
        MY_PARAMETER( Generic, Mat4, projection )
    };

};
#undef MY_PARAMETER

Usage:

Shader<ShaderParam::Generic> s;

s.SetParameter(ShaderParam::Generic::projection(), Mat4());
s.SetParameter(ShaderParam::Generic::position(), Vec4());

What are you thoughts on this implementation? Do you have suggestions for improvements or perhaps an entirely different system?

I know I've seen people use hash maps and such, but there's still the problem of continuously using a string literal to set the uniforms and such (I don't really like the thought of using a Macro to define the string, either).

What are you thoughts on this implementation?

It is simply awful. Besides code bloat, just look how you declare such objects, and how you call methods on such objects.

  Do you have suggestions for improvements or perhaps an entirely different system?

So, to fix your implementation :

add an interface for your Shader class. Like that, you can have different shader classes for vertex, geometric and pixel shaders
remove all templates
pass the shader's program id to constructor, and remove it from the Parameter class
move 'class Parameter' out of the 'class Shader', and make it a structure. It's member variables do not need to be static. By the way, name should be std::string - not const char*


Efficient Road Network Intro

I've decided to learn haskell, because I've always enjoyed the functional aspect of Python and want to explore other languages that use this concept. I'm still stumbling in the dark most of the time, so would greatly appreciate any improvements.

Description

Challenge source

A road system is considered efficient if it is possible to travel from any city to any other city by traversing at most 2 roads.

Example

For n = 6 and roads = [[3, 0], [0, 4], [5, 0], [2, 1], [1, 4], [2, 3], [5, 2]]

the output should be True

Here's how the road system can be represented:

Code

middlePoints k n roads  = [i | i <- [0..n-1], elem [k,i] roads || elem [i,k] roads]

thoughMiddlePoints k x n roads = or [(elem [points !! i,x] roads || elem [x, points !! i] roads) | i <- [0..(length points - 1)]]
    where
    points = middlePoints k n roads

reachable k x n roads = elem [k,x] roads || elem [x,k] roads || (thoughMiddlePoints k x n roads)

efficientRoadNetwork n roads = all (==True) [ all (==True)[(reachable i j n roads) | j <- [0..n-1], i /= j] | i <- [0..n-1]]

I find !! and once-used definitions smelly.

A passed around environment like roads might as well open a scope that spans all uses of it.

efficientRoadNetwork roads n = and
  [ edge i j || any (\k -> edge i k && edge k j) [0..n-1]
  | i <- [0..n-1], j <- [0..i-1]
  ] where edge i j = elem [i,j] roads || elem [j,i] roads


Correct way to delete elements from a ConcurrentDictionary with a predicate I have written a caching wrapper class for ConcurrentDictionary. Basically it has a timer that checks if items are expired and removes them. Since ConcurrentDictionary does not have RemoveAll method that takes a predicate as a parameter, it has to decide which ones are expired first, then remove them. The problem is an item can be unexpired (it may have sliding expiration and it is accessed) before it is removed. I have to do something to make sure my code is correct.

Here's the current implementation:

private void CheckExpiredItems(object sender, ElapsedEventArgs e)
{
    _timer.Stop();
    IEnumerable<TKey> expiredItemKeys = _cachedItems.Where(item => item.Value.IsExpired)
                                                    .Select(item => item.Key)
                                                    .ToList();

    foreach (TKey expiredItemKey in expiredItemKeys)
    {
        ICacheItem<TValue> expiredItem;
        if (_cachedItems.TryRemove(expiredItemKey, out expiredItem))
        {
            if (expiredItem.IsExpired)
            {
                expiredItem.Expire();
            }
            else
            {
                _cachedItems.TryAdd(expiredItemKey, expiredItem);
            }
        }
    }
    _timer.Start();
}

It removes the expired items one by one while checking they are still expired. If it is not expired, it is added back only if there isn't a new item with the same key. However I'm still not sure if this code is correct. I would appreciate a few suggestions. If you want to take a look at the rest, it is on GitHub.

I know it's not a direct answer to your question, but I'm trying to get to the roots of the problem rather than giving correct answer to issue caused by potential misuse...

In your code what you are actually trying to do is to write a cache with time-based expiration of items. Even though ConcurrentDictionary is thread-safe, it's not quite appropriate structure for frequent element scans like you do. Moreover, in your code you remove items, then add them back in case when they have been updated in the middle. It causes side effects for other threads that may try to read the value in-between.

Correct solution would be to use a proper data structure. If by any chance you are using .NET 4.0 or later .NET has already provided you with proper solution - MemoryCache class, otherwise I would recommend creating a lock-based class that maintains 2 indexes for entries - a Dictionary for storing key-value pairs, and a SortedList for storing expiration timestamp-key mapping. In this case you'll always know upfront when the next expiration will happen so timer can be set to specific TimeSpan, and you don't have to scan through all cache entries to find expired ones.


Loading settings from a file to a dictionary Today I coded a class to get settings from a file and to load them in to a dictionary.

Please post and feedback, improvements and changes, no matter how big or small, as long as they are actual improvement and not disadvantages.

using log4net;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;

namespace Sahara.Application.Base.Core.Settings
{
    class Settings
    {
        /// <summary>
        /// Holds a list of settings.
        /// </summary>
        private Dictionary<string, string> allSettings;

        /// <summary>
        /// log4net instance
        /// </summary>
        private ILog myLogger;

        /// <summary>
        /// syncRoot object
        /// </summary>
        private object syncRoot;

        /// <summary>
        /// Creates a new instance of Settings
        /// </summary>
        public Settings()
        {
            allSettings = new Dictionary<string, string>();
            myLogger = LogManager.GetLogger(typeof(Settings));
            syncRoot = new object();

            if (!loadSettings("settings.ini"))
                throw new Exception("Failed to load settings.");
        }

        /// <summary>
        /// Loads the settings from the settings file
        /// </summary>
        /// <param name="file">Settings file path</param>
        /// <returns>bool loadSettings</returns>
        private bool loadSettings(string file)
        {
            try
            {
                allSettings.Clear();

                lock (syncRoot)
                {
                    allSettings = File.ReadLines(file)
                    .Where(isValidLine)
                    .Select(line => line.Split('='))
                    .ToDictionary(line => line[0], line => line[1]);
                }

                return true;
            }
            catch (Exception exception)
            {
                myLogger.Error(exception);
                return false;
            }
        }

        /// <summary>
        /// Checks if the line is a comment.
        /// </summary>
        /// <param name="line">Line to check</param>
        /// <returns>bool isValidLine</returns>
        private bool isValidLine(string line)
        {
            return !line.StartsWith("#") && line.Contains("=");
        }

        /// <summary>
        /// Returns a setting with the specified key.
        /// </summary>
        /// <param name="key">Key to search for</param>
        /// <returns>Setting</returns>
        public string getSetting(string key)
        {
            if (allSettings == null)
                throw new NotImplementedException("Config data has not get been implemented.");

            lock (syncRoot)
                return this.allSettings[key];
        }
    }
}

Pointless locking

You lock in two places:

When loading the configuration file, during construction. This lock will never be contested: no two threads will ever wait on this lock, because every object has its own, different, private lock
When getting a value from the dictionary. This lock may be contested in theory by multiple threads trying to get values, but there is no reason to lock for that. It would simply slow down high throughput of requests, with no benefits

You can safely remove the locking completely.

Exception handling

loadSettings catches exceptions during file loading, logs and returns a boolean. It would be simpler to not catch exceptions here, let them be thrown, and return void.

Since an instance of this class is unusable if an error happens during file loading, consider not catching any exceptions at all, let them be thrown, and let the caller handle the situation.

That will also allow you to remove the null check in the getSetting method, as the dictionary will never be null. Which is nice, as it's always easier to understand a class when you know it can never be in an invalid state.

Further improving loadSettings

Clearing the settings dictionary in this method is pointless. It would be better to create the dictionary here and return it.


Simple QGraphicsScene program At the documentation for QGraphicsScene, there is a simple example that does not work as written. I've written a fix that does work, and am curious if it could be improved.

From the docs (does not work as is):

scene = QGraphicsScene()
scene.addText("Hello, world!")
view = QGraphicsView(&scene)
view.show()

Fixed version that does work:

from PySide import QtGui
import sys

class MyFirstScene(QtGui.QWidget):
    def __init__(self):
        QtGui.QWidget.__init__(self)
        self.scene=QtGui.QGraphicsScene(self)
        self.scene.addText("Hello, world!")
        self.view = QtGui.QGraphicsView(self.scene, self)
        self.show()

if __name__=="__main__":
    app=QtGui.QApplication(sys.argv)
    firstScene = MyFirstScene()
    sys.exit(app.exec_())

My question is whether I can simplify this more, making it more like the example in the documentation. I tried doing it without making the scene an attribute of QWidget, which breaks it. But perhaps I'm missing a simpler working implementation of the documentation's skeleton?

This complete script works for me with PyQt on Windows, displays a small window and waits for me to close it.

from PyQt4 import QtGui

app = QtGui.QApplication([])

scene = QtGui.QGraphicsScene()
scene.addText("Hello, world!")
view = QtGui.QGraphicsView(scene)
view.show()

app.exec_()

A QApplication object must be created before any other GUI objects.


Simple Java calculator I am a beginner in Java programming. Here is my simple calculator. I hope you could scan through my coding and let me know if there is any error in my coding or how I could simplify the code.

import java.util.Scanner;

public class Decimal {
    /**
     * @param args
     */
    public static void main(String[] args) {
        double n1, n2;
        String operation;
        Scanner scannerObject = new Scanner(System.in);

        System.out.println("Enter first number");
        n1 = scannerObject. nextDouble();

        System.out.println("Enter second number");
        n2 = scannerObject. nextDouble();

        Scanner op = new Scanner(System.in);
        System.out.println("Enter your operation");
        operation = op.next();

        switch (operation)  {
        case "+":
            System.out.println("Your answer is " + (n1 + n2));
            break;

        case "-":
            System.out.println("Your answer is " + (n1 - n2));
            break;

        case "/":
            System.out.println("Your answer is " + (n1 / n2));
            break;

        case "*":
            System.out.println("Your asnwer is " + (n1 * n2));
            break;

        default:
            System.out.println("Je ne sais pas");

        }
    }
}

Looks good for a beginner.

Couple tweaks if you want to make it look nicer. The name of the class should be somewhat describing the purpose so in your case Calculator would fit better. If you are familiar with enum you could parse your +, -, / into an enum and do switch on that. Then next tweak could be to do the System.printout at the end and in the switch case to do just the operation.

Another suggestion would be to make 4 classes implementing binary operations with just one method taking two arguments and returning the result. Then you can have a map with a key being the operation (in enum or string) and the value would be the appropriate class. Then you end up with more classes but this main method gets shorter.

Generally it is better to split the code in more methods and classes and keep the motto "one class one responsibility", or Single Responsibility Principle. So as an example each of your arithmetic classes would be responsible for a single arithmetic operation. If you want to follow that completely, then you would have to create a class that gets the input from the user as well. The Calculator class then would be just like a coordinator saying: give me numbers, give me an operator, perform the operation and print out the result at the end.

Note: If you want some sample code leave me a comment here and I can add some.


Ensure every element in array passes asynchronous test I've written a function which ensures all objects in the items array belong to the user. This involves a database check, which results in the code being asynchronous. This code works, but feels sloppy. I'd appreciate a review and suggestions for how to improve it.

The main challenge I had with writing this was to use items.every() with Promises and I don't think the solution I came up with was very elegant. I spent quite a bit of time on it and couldn't think of a better way to do it.

Intuitively async/await feels like a good fit for this type of problem but I had difficulty applying it here.

function belongTo (user, items) {
  return new Promise(resolve => {
    resolve(items.every((item) => {
      return new Promise((resolve, reject) => {
        Item.findOne({ owner: user,
          assetID: item.assetid,
          appid: item.appid,
          contextid: item.contextid,
          marketHashName: item.marketHashName
        }).then((item) => {
          if (Object.keys(item).length !== 0) resolve(true)
          else resolve(false)
        }).catch(reject)
      })
    }))
  })
}

I'm using the JavaScript Standard style guide.

First of all, it appears that Item.findOne already returns a promise. You don't need to wrap it again in a promise. Simply use that same promise as the return value for your array.every.

Now as far as I understand your problem, you just need to hit the db for every element in item and resolve the promise belongTo returns if every element belongs to user. I don't think your code will work that way. What will happen is that belongTo return a promise will resolve immediately with the value returned by array.every.

Next, you don't need to pass true or false as promise resolution. The fact that it resolved/rejected is enough to tell you that. Also, you don't need to wrap an operation with a promise to create a promise. There's Promise.resolve() and Promise.reject() that create already resolved/rejected promises.

function belongTo (user, items) {
  const pingPromises = items.map(item => Item.findOne({ owner: user,
    assetID: item.assetid,
    appid: item.appid,
    contextid: item.contextid,
    marketHashName: item.marketHashName
  }).then((item) => {
    // Validate result if we should resolve/reject this promise.
    return Object.keys(item).length ? Promise.resolve() : Promise.reject();
  }));

  // Do not catch. If Item.findOne rejects, let's reuse that rejection.

  // Accepts an array of promises. Returns a promsie that resolves when all
  // promises resolve, rejects when at least one rejects.
  return Promise.all(pingPromises);
}

Now even with that code, the operation itself is very inefficient. You might want to consider creating some form of compound query instead and only hit the DB once. Not very familiar with MongoDB, but in MySQL this can easily be done by using multiple conditions.


How can I make my Merge Sort Code In C more secure, effective and efficient I have been implementing the merge-sort algorithm for sorting the data for a long time, each time I use the following source code for implementation.
By secure, I mean, taking care of memory management, loopholes etc.

#include<stdio.h>
#include<stdlib.h>
#include<assert.h>
void mergesort(int*,int,int);
void mergeSequence(int*,int,int,int); //Merge the two arrays
void displaySequence(int*);
int n;
int main()
{
    int t,test;
    int*num=NULL;
    int i;
    printf("Enter the number of test cases\n", test);
    scanf("%d", &test);
    assert(test>0);
    for(t=0;t<test;t++)
    {
        printf("Enter the size of the sequence\n", n);
        scanf("%d", &n);
        assert(n>0);
        num=calloc(sizeof(int),n);
        assert(num);
        printf("Enter the sequence\n");
        for(i=0;i<n;i++)
            scanf("%d", (num+i));
        printf("Sequence before sorting\n");
        displaySequence(num);
        mergeSort(num,0,(n-1));
        printf("Sequence after sorting\n");
        display(num);
        free(num);
    }
    return 0;
}
void displaySequence(int*num)
{
    int i;
    for(i=0;i<n;i++)
        printf("%d ", num[i]);
    printf("\n");
}
void mergeSort(int*num,int start,int end)
{
    int mid;
    mid=start+((end-start)/2);
    printf("Mid->%d\n", mid);
    if(start<end)
    {
        mergeSort(num,start,mid);
        mergeSort(num,(mid+1),end);
        mergeSequence(num,start,mid,end);
    }

}
void mergeSequence(int*num,int start,int mid,int end)
{
    int n1,n2;
    int i,j,k;
    n1=(mid-start)+1;
    n2=(end-mid);
    printf("N1->%d\nN2->%d\n", n1,n2);
    int left[n1];
    int right[n2];
    for(i=0;i<n1;i++)
        left[i]=num[start+i];
    for(i=0;i<n1;i++)
        printf("%d ", left[i]);
    printf("\n");
    for(j=0;j<n2;j++)
        right[j]=num[(mid+1)+j];
    for(j=0;j<n2;j++)
        printf("%d ", right[j]);
    printf("\n");
    //Now I have two arrays, and I can now merge them
    k=start;
    i=0;
    j=0;
    while((k<=end))
    {
        if(i==n1)
        {
            num[k]=right[j];
            j++;
        }
        else if(j==n2)
        {
            num[k]=left[i];
            i++;
        }
        else if(left[i]<=right[j])
        {
            num[k]=right[j];
            j=j+1;
        }
        else
        {
            num[k]=left[i];
            i=i+1;
        }
        k++;
    }
    displaySequence(num);
}

make the code more compact and secure

Separate sort code from test code

To well address security issues, OP needs to present a clear distinction between what is the sort code and what is not.  Put all sort code in a separate .c file.

int vs. size_t

C employs size_t as the type that is neither to narrow nor too wide for array indexing and sizing.

Although int is commonly sufficient, to make more secure and work with large data, use size_t.

Aside from changing int --> size_t and "%d" --> "%zu", a singular challenge is to remember size_t is some unsigned type and code needs to consider that.  (size_t)x < 0 is never true.

Beware large VLAs

Security risk: int left[n1]; is subject to stack overflow.  Consider *alloc() instead.  Insufficient memory can be detected that way.

Avoid exposing helper functions

mergeSequence() is not meant to be used outside of mergesort().  To prevent other code from using this functions, make it static.  Further, put it and mergesort() in their own .c file.

This increases security as mergeSequence()  then not need to handle all sorts of arguments, just the ones mergesort() generates.

All warnings enabled?

Save time, enable all warnings.

I'd expect mergeSort(num,0,(n-1)); (upper case S) to warn about function usage before declaration.

void mergesort(int*,int,int); (lower case S) is declared but not used.

Avoid generic names when a specific type is required

mergeSort() sorts int.  With a name like mergeSort, I expected something like qsort() with its size-of-element parameter and compare function.

Of re-architect your merge sort code to take a generic object type.

void SS_mergeSort(void *base, size_t nmemb, size_t size,
    int (*compar)(const void *, const void *));`

Compact by eliminating print out

It is unexpected that a sort routine would do any printing.  Remove printf() and displaySequence() from merge...() functions.

Compact left,right code

int left[n1]; int right[n2]; can be int left_right[n1+n2]; and then the copying facilitated with the fast lib function memcpy().

memcpy(left_right, num, sizeof left_right);
int *left = left_right;
int *right = left_right + n1;
...


Implementation of Prim's minimum spanning tree Feel free to tell about every possible issue (style, errors, inffective solution etc.) you found. Here is the main part of what I want to be reviewed, through there is some related code on GitHub.

Header

#ifndef PRIM_GRAPH_MST_H
#define PRIM_GRAPH_MST_H

#include <list>
#include "graph.h"

class PrimGraphMstImplementation;

class PrimGraphMst
// class that should find minimum spanning tree (MST) for the given graph
// and then store obtained result as a list of edges and summary weight
{
public:
    PrimGraphMst(const Graph&);
    // all work is actualy done in constructor
    // then we hold obtained result in some immutable fields

    const std::list<Graph::EdgeKey>& edges();
    // returns a list of edges from the given graph that forms MST

    double weight() const;
    // returns MST summary weight

   bool valid() const;
   // checks if obtained mst contains same number of vertices as the given graph
   // it could be wrong if the given graph was not connected

   Graph* makeTreeGraph();
   // construct new graph that have only edges from our MST

   private:
   std::shared_ptr<PrimGraphMstImplementation> impl;
};

#endif // PRIM_GRAPH_MST_H

C++ Source

#include <queue>
#include <cassert>
#include "prim_graph_mst.h"

class EdgeWithPriority
// helper class to use graph eges in priority queue
{
public:
double weight;
Graph::EdgeKey key;

EdgeWithPriority(int x, int y, const Graph& graph)
{
    weight = graph.distance(x, y);
    assert(weight >= 0);
    key = Graph::makeEdgeKey(x, y);
}

bool operator<(const EdgeWithPriority& other) const
// such behavior is needed to use this type in std::priority queue
// where top element is always max element and wee need to retrieve
// edge with the lowest weight
{
    return weight >= other.weight;
}
};

class PrimGraphMstImplementation
{
//actual implementation of Prim's algorithm
public:
PrimGraphMstImplementation(const Graph& graph)  :
    graph(graph),
    summaryWeight(0)
  // we do all the work here in constructor
  // we assume that graph is connected
{
    assert(graph.verticesCount() > 0);
    visitVertex(0);

    while(!pq.empty() && visited.size() < graph.verticesCount())
    {
        EdgeWithPriority element = pq.top();
        pq.pop();

        if(visited.find(element.key.first) == visited.end())
        {
            edges.push_back(element.key);
            summaryWeight += element.weight;
            visitVertex(element.key.first);
        }
        else if (visited.find(element.key.second) == visited.end())
        {
            edges.push_back(element.key);
            summaryWeight += element.weight;
            visitVertex(element.key.second);
        }
        else
        {
            continue;
        }
    }
}

void visitVertex(size_t index)
// helper method marks vertex as visited and adds all it's
// edges to priority queue
{
    visited.insert(index);
    for(size_t other : graph.getVertexNeighbors(index))
    {
        pq.push(EdgeWithPriority(index, other, graph));
    }
}

bool valid() const
// it could be wrong if given graph was not connected
{
    return visited.size() == graph.verticesCount();
}

const Graph& graph;
double summaryWeight;
std::list<Graph::EdgeKey> edges;

private:
std::priority_queue<EdgeWithPriority> pq;
std::set<size_t> visited;
};

PrimGraphMst::PrimGraphMst(const Graph& graph)
{
impl = std::make_shared<PrimGraphMstImplementation>(graph);
}

const std::list<Graph::EdgeKey>& PrimGraphMst::edges()
{
return impl->edges;
}

bool PrimGraphMst::valid() const
{
return impl->valid();
}

double PrimGraphMst::weight() const
{
return impl->summaryWeight;
}

Graph* PrimGraphMst::makeTreeGraph()
{
if(!valid())
    return nullptr;

Graph* result = new Graph(impl->graph.verticesCount());
for(Graph::EdgeKey element : impl->edges)
{
    double weight = impl->graph.distance(element.first, element.second);
    result->connect(element.first, element.second, weight);
}

return result;
}

Generally speaking, it seems to be coded pretty well. I can just find one or two things that could be improved:

else
{
    continue;
}

This piece of code at the end of a while seems fairly useless. I guess it would be the same without it.

void visitVertex(size_t index)
// helper method marks vertex as visited and adds all it's
// edges to priority queue
{
    visited.insert(index);
    for(size_t other : graph.getVertexNeighbors(index))
    {
        pq.push(EdgeWithPriority(index, other, graph));
    }
}

Here, you can replace pq.push(EdgeWithPriority(index, other, graph)); by pq.emplace(index, other, graph);. The function emplace will construct the object directly in the priority_queue with the given parameters.

You could also probably replace visited.insert(index); by visited.emplace(index); but I don't think it will change anything.

Graph* result = new Graph(impl->graph.verticesCount());
for(Graph::EdgeKey element : impl->edges)
{
    double weight = impl->graph.distance(element.first, element.second);
    result->connect(element.first, element.second, weight);
}

I don't know what the methods distance and connect do, but depending on what they really do, it could be better to for your for loop to take const references instead of values. But, well... it depends on distance and connect.

You should use std::unique_ptr instead of std::shared_ptr to hold the pointer to the implementation. Since std::unique_ptr cannot be copied, that means that PrimGraphMst cannot be copied either, but that's probably fine since std::shared_ptr was not copying your data either but merely doing an alias to the implementation.

You can also improve the constructor of PrimGraphMst by using a constructor initiaization list instead of a late memory allocation:

PrimGraphMst::PrimGraphMst(const Graph& graph):
    impl{ new PrimGraphMstImplementation{graph} }
{}

You may have noted it: I used curly braces in the initialization list of the constructor in the previous section of this review. Contrary to plain parenthesis, curly braces prevent any implicit narrowing type conversion. Using them can help to catch the small errors that are sometimes linked to narrowing conversions.

Style: your indentation often seems off and makes the code quite difficult to review. You should indent your code evrytime you introduce a new scope.

You are placing the comments of your function right after their point of declaration. Generally speaking, comments about a function are placed right before the function declaration.


Exception handling for timing CouchBase fetch override def get(key: String): Option[Any]  = {
  val timer = couchbaseMetricsTimer.time()
  try { // Any way (is it more elegant?) to avoid the try/catch/finally?
    Option(couchBaseClient.get(key))
  } catch {
    case e: Exception => couchbaseFailureMetricCounter.inc(); throw e
  } finally {
    timer.stop()
  }
}

Note the clients of this code are expecting exceptions in case of failures.

You could use Try utility.

But the main concern is that it doesn't support finally statement out-of-the-box.

This can be achieved with some Try enhancement's value class (by Viktor Klang):

implicit class TryOps[T](val t: Try[T]) extends AnyVal {
  def eventually[Ignore](effect: => Ignore): Try[T] = {
    val ignoring = (_: Any) => { effect; t }
    t transform (ignoring, ignoring)
  }
}

Consider this code:

val timer = couchbaseMetricsTimer.time()
val tryClient =
  Try(couchBaseClient.get(key)) map { //convert success result
    Option(_)
  } recover { //process failure result
    case e: Exception => couchbaseFailureMetricCounter.inc(); throw e
  } eventually { //perform "finally" block
    timer.stop()
  }

//return actual result
tryClient.get

It's not less, by more idiomatic, so it's up to you which way to choose.

Here are more references to discover.

Note: you can put TryOps into package object of some of your root package f.e. com.mycompany, and then you can use it everywhere in your project.


Changes quantities that are displayed in a shopping cart page I'm fairly new to PHP and am following e-commerce tutorials. When exploring a way to resolve a problem I had with a PHP code block, I came upon a post another person had with the same code block. The post suggested a way to simplify the code.

Original code block:

<?php
if (isset($_POST ['pid'])) {
    $pid = $_POST ['pid'];
    $wasFound = false;
    $i = 0;
    if (!isset ($_SESSION["cart_array"]) || count($_SESSION ["cart_array"])<1){
        $_SESSION["cart_array"] = array (0=> array("item_id"=> $pid, "quantity" => 1));
    }  else {
        foreach ($_SESSION["cart_array"] as $each_item) {
            $i++;
            while(list($key,$value)=each($each_item)){
            if ($key== "item_id" && $value == $pid) {
                array_splice ($_SESSION["cart_array"], $i-1,1, array(array("item_id"=>$pid,"quantity"=> $each_item['quantity']+1)));
                $wasFound = true;
            }
        }
    }
    if($wasFound==false) {
        array_push ($_SESSION["cart_array"], array("item_id"=> $pid, "quantity" => 1));
        }
    }
    header("Location: cart.php");
    exit();
}
?>

This was the suggested simplified code block and an explanation of what to do after changing code. I am unsure how to implement the the part suggested after the code block.

if (isset($_POST['pid'])) {
    // add (+1) item to cart
    $pid = (int)$_POST['pid']; // cast as integer
    // valid pids are > 0
    if($pid > 0){
    if(!isset($_SESSION['cart_array'][$pid])){
    // item is not in the cart, add it with quantity = 1
    $_SESSION['cart_array'][$pid] = array("item_id" => $pid, "quantity" => 1); // I left the array in the cart the same, but it
could also be simplified so that it is only the quantity, since the
item_id is now the cart array index
    } else {
    // item is in the cart, increment quantity
    $_SESSION['cart_array'][$pid]['quantity']++;
    }
    }
    header("location: cart.php");
    exit();
    }

  To get the details for the cart items, you need to run ONE query that
  gets all of them at the same time (putting a query inside of a loop is
  a resource killer). For the definition of the cart that I have
  suggested, you can use array_keys to get all the item id's. You
  would then implode those into a comma separated list and put them into
  an IN() comparison in the WHERE clause in a query to get all the
  matching rows at once.

This is what I am considering adding to the code block:

if  ($result = print_r(array_keys('cart_array',$pid))) {
$comma_seperated = implode("," $result);
// then use use $comma_seperated in query where needed later in annother code block?
}

Am I on the right path?

You don't do a good job of validating $_POST['pid']. You just assume the value passed will be good (a bad mistake when working with user input).

May I suggest something like:

$filterOptions = array('options' => array('min_range' => 1));
$pid = filter_input(INPUT_POST, 'pid', FILTER_VALIDATE_INT, $filter_options);
if(is_null($pid) || $pid === false) {
     // parameter was either not set (null) or failed filter (false)
     // you should handle this somehow
     exit();
}

The two different code examples are not the same in that the cart array is build differently in each.  The first would build a numerically-indexed structure at $_SESSION['cart_array'] like this:

[
    0 => [
        'item_id' => {{pid}},
        'quantity' => {{quantity}}
    ],
    1 => [
        ...
    ],
    ...
]

Whereas the second would build an array index by item id like this:

[
    {{pid}} => [
        'item_id' => {{pid}},
        'quantity' => {{quantity
    ],
    {{pid2}} => [
        ...
    ],
    ...
]

Did you really intend to change this structure? If so, are there other areas of code that might need to change to accommodate this changed structure?

$result = print_r(array_keys('cart_array',$pid))

This makes no sense. cart_array is not an array suitable for use with array_keys().  Why would you use print_r() (an output function) here at all?

If you want to use the second form of the cart data shown above, this is how you would extract pid values from it:

$pidsInCart = array_keys($_SESSION['cart_array']);

With the numerically-indexed cart array, it might be:

$pidsInCart = array_map(
    function($item) {
        return $item['item_id'];
    },
    $_SESSION['cart_array']
);

I would also would be careful in evaluating the result directly, as you could get unexpected truthy/falsey results there I might use:

$pidsInCart = ...;
if(!empty($pidsInCart)) {
     ...
}

Also, use meaningful variable names. $result here is a good example of a not very meaningful name in this context.

Indent your code properly.


Replace one-liner sed/awk with python I have some files that I want to process, and I know how to do it in sed/awk (for each one):

awk '{if (index($0,"#")!=1) {line++; if (line%3==1) {print $2,$3}}}' q.post  > q

or

grep -v "#" q.post | awk '{if (NR%3==1) {print $2,$3}}'

It's one line, and rather beautiful and clear.

Now, my main program is in python (2.7). Calling sed/awk from python is a bit tedious—I get some error—and I'd rather use a nice pythonic way to do it.

So far I have:

    pp_files = glob.glob("*gauss.post")
    for pp in pp_files:
        ppf = open(pp)
        with open(pp[:pp.rfind(".post")] + "_clean.post", "w") as outfile:
            counter = 0
            temp = []
            for line in ppf.readlines():
                if not line.startswith("#"):
                    temp.append(line)
            for line in temp:
                if counter % 3 == 0:
                    outfile.write(" ".join(line.split()[1:3]) + '\n')
                counter += 1
        ppf.close()

Meh.

It works, but it's not beautiful. Is there a pythonic way, preferentially a clear one liner (not 10 imbricated list comprehension, to replace awk and sed ?

Thanks

First you should add open(pp) to your with.
Always use with with open.
This is as it will always close the file, even if there is an error.

But onto your code. You seem to dislike comprehensions. I don't really get why.
Take your code:

for line in ppf.readlines():
    if not line.startswith("#"):
        temp.append(line)

This can instead be:

[line for line in ppf if not line.startswith("#")]

I know which I find easier to read. But if you don't like it fair dues.
After this I'd then slice the list, you want every third line.
To do this we can use the slice operator, say you have the string abcdefghijk, but you only want every third character.
You'd do 'abcdefghijk'[::3]. This gets adgj.
This removes the need for counter, and so can simplify your code to:

for pp in pp_files:
    with open(pp) as ppf, open(pp[:pp.rfind(".post")] + "_clean.post", "w") as outfile:
        for line in [line for line in ppf if not line.startswith("#")][::3]:
            outfile.write(" ".join(line.split()[1:3]) + '\n')

But if your file is large it'll read all of it into a list, then take a third of it put it in another list.
That's bad, instead if you use a generator comprehension and itertools.islice then you can achieve the same as above.
But the program will use less memory.

for pp in pp_files:
    with open(pp) as ppf, open(pp[:pp.rfind(".post")] + "_clean.post", "w") as outfile:
        for line in islice((line for line in ppf if not line.startswith("#")), 0, None, 3):
            outfile.write(" ".join(line.split()[1:3]) + '\n')