JQuery Selector in Vanilla JS I've decided to reproduce the JQuery selector - specifically for finding elements by class name, ID, and tag

So for example the tests could be written like

$('.box') === domSelector('.box') // Class
$('#box') === domSelector('#box') // ID
$('input') === domSelector('input') // Tag

Here is my approach to the problem. It works good, and I'd like to hear the ideas to simplify it or improve it in any way.

var domSelector = function(selectors) {
  if (selectors.startsWith('.')) {
    return Array.from(document.getElementsByClassName(selectors.substring(1)));
  }
  else if (selectors.startsWith('#')) {
    return [document.getElementById(selectors.substring(1))];
  }
  else if (!/^[a-zA-Z0-9!@#\$%\^\&*\)\(+=._-]+$/g.test(selectors)) {
    throw Error('Invalid Selector');
  }
  else {
    return Array.from(document.getElementsByTagName(selectors));
  }
};

1st If - Check if passed value is class. (if it starts with a dot, it is a class)
2nd If - Check if a passed value is an ID (if it starts with #, it is an ID)
3rd If - Now since I already checked for both characters # and dot , I check if passed value starts with special character (Remember that it cannot start with dot or # as I already did that check). If it starts with a special character - I throw an exception that identifier is not in a valid format
4th If - The only left option is that element is tag. No further check needs to be done as if selector doesn't match any tags it will return an empty array.

Duplicated Ids!!!

I notice in the comments there is mention of more than 1 element with the same Id and that getElementById will only return one element, which is correct for the majority of browsers depending on the version.

Even if getElementById acted the same across browsers As a developer intentionally using duplicated Id is VERY BAD! as it will force the browser into Quirks Mode
 effecting performance, layout and can effect how search engine bots crawl your site.

Though Google does say...

  “Although we do recommend using valid HTML, it’s not likely to be a factor in how Google crawls and indexes your site.”

... from SEJ 6 Reasons Why Google Says Valid HTML Matters that links to Google Browser Compatibility support page

This is not a guarantee and not something you should trust your sites SEO on.

The simple and safe rule is. NEVER duplicate Id's!!!

Your code

Ignoring the above and looking at your code without regard to the DOM and that getElementById can return an array you can improve the code (readability and maintainability [1.]) as follows

function queryDOM(selector) {
    const VALIDATOR = /[a-z0-9!@#$%^&*)(+=._-]/i, NS = "http://www.w3.org/1999/xhtml";
    const type = selector[0], query = selector.slice(1);
    if (type === ".") { return [...document.getElementsByClassName(query)] }
    if (type === "#") { return [document.getElementById(query)] }
    if (! VALIDATOR.test(selector)) { throw new RangeError('Invalid Selector') }
    return [...document.getElementsByTagNameNS(NS, selector)];
}

or for the code newbies that need the spacing

function queryDOM(selector) {
    const VALIDATOR = /[a-z0-9!@#$%^&*)(+=._-]/i;
    const NS = "http://www.w3.org/1999/xhtml";
    const type = selector[0];
    const query = selector.slice(1);

    if (type === ".") {
        return [...document.getElementsByClassName(query)];
    }

    if (type === "#") {
        return [document.getElementById(query)];
    }

    if (! VALIDATOR.test(selector)) {
        throw new RangeError("Invalid Selector");
    }

    return [...document.getElementsByTagNameNS(NS, selector)];
}

Reasoning

As a function declaration rather than a function expression so that the function is accessible at any time after parsing.
Extract the first character and remaining query in one spot to avoid logic/code duplication.
Remove the redundant else and supporting code to reduce noise.
Use ES6+ spread operator ... (AKA spread syntax) to create arrays from iteratable objects.
Use the simpler bracket access for single character in string rather than the more verbose String.startsWith which is meant for sub strings longer than one character.
Use String.slice rather than String.substring
Renamed function to hold to camel-case convention of upperCase acronyms and rearranging to stop clash with JS convention of PascalCase only functions used with the new token (Note your function will work as new DOMSelector())
Renamed argument to not imply its an array (plurals only for arrays or array like objects)
Throw a range error more suited to the error type (Really should not be throwing for this type of issue)
Remove the global flag from the RegExp
Removed redundant ^, $, + and back slashes inside the [] from RegExp
Added case insensitive flag to RegExp
Declared RegExp as a constant to take out of the statement to make it more readable.
As you include capitals in the validation of the selector I assume you are unaware that getElementsByTagName first converts the query string to lowercase. Thus I changed the function to use getElementsByTagNameNS

The getElementsByTagNameNS which namespace (NS) to use is not known from your code and thus the example assumes "http://www.w3.org/1999/xhtml". Warning quirks mode will effect the namespace.

Additionally

The function has a failure if the selector argument is not a string. As the function does throw I assume you only use it inside a try catch block and you handle the error when it occurs.

Why the complications!

As your code relies on some assumptions that are flawed and do not compile with valid HTML I can not approve the above as worthy of anything but as an example of what not to do.

The code should use querySelectorAll and covert its iteratable return to an array. I would also drop the "DOM" in the name (what else could you be querying?) and include a node to focus the query.

Also removing the validation as query string can be used to search attributes that may conflict with the validation.

function query(str, node = document) { return [...node.querySelectorAll(str)] }

Or if you control code placement and use the "use strict" directive you can use an arrow function expression to assign a named constant to prevent accidental overwriting of the function.

"use strict"; // << Must be at top of code.
const query = (str, node = document) => [...node.querySelectorAll(str)];

This is not at all replacement of jQuery

BTW

A reminder. NEVER duplicate Id's!!!

See I added 3 exclamation points and made it bold to indicate how important this warning is.

Notes

[1.] Readability and maintainability are highly subjective quantities and as such reflex my view only. I have 40Years of experience (35 as a professional) if that counts for anything.


Setting label in Java button listener How can I avoid so many if statements?

//Radio Button toggle

if (five.isSelected() == true) {
            one.setSelected(false);

    private class JButtonListner implements ActionListener {

//As my code has 4 options (1,5,+,-) to change the label, i have done 4 if else statments. is it possible to avoid such mess?

        public void actionPerformed(ActionEvent e) {
            if (e.getSource().equals(plus) && five.isSelected() == true) {
                count += 5;
                label.setText(Integer.toString(count));

            } else if (e.getSource().equals(minus) && five.isSelected() == true) {
                count -= 5;
                label.setText(Integer.toString(count));

            } else if (e.getSource().equals(minus) && one.isSelected() == true) {
                count --;
                label.setText(Integer.toString(count));

            } else if (e.getSource().equals(plus) && one.isSelected() == true) {
                count++;
                label.setText(Integer.toString(count));
            }
        }

It would be nice to see more of the code since one, five, plus and minus give NullPointerException

public void actionPerformed(ActionEvent event) {
      int value = five.isSelected() ? 5 : 1;
      if (event.getSource().equals(plus)) {
                count += value;
      } else if (event.getSource().equals(minus)) {
                count -= value;
      }
      label.setText(Integer.toString(count));
}

Since you only have 2 RadioButton, that would mean that the value could only be two values, either 5 or 1. This can be done by line two, you are asking if five.isSelected(), if(five.isSelected()) { return 5; } else { return 1; }. Also there is no need to say five.isSelected() == true, because the if already asks if it is true so five.isSelected() is sufficient.

Now you dont need to plus/minus with 5 or 1, but only check plus or minus.

And you set the text each time count has changed, so this can just be done after the if.


Pythagorean triplet adding up to 1000 Project Euler problem 9 says:

  A Pythagorean triplet is a set of three natural numbers, \$a < b < c\$, for which, $$a^2 + b^2 = c^2$$
  For example, \$3^2 + 4^2 = 9 + 16 = 25 = 5^2\$.

  There exists exactly one Pythagorean triplet for which \$a + b + c = 1000\$.
  Find the product \$abc\$.

My code finds the correct solution to the problem. The problem is it takes 47.6 seconds to get it, so I'd like to optimize it, but don't know exactly how to do so further. I've already reduced the range of the for loops.

import time
s=time.time()
a=0
b=0
c=0

for i in range(1,499): #max value for any side of the triangle must be less      than the semiperimeter
    for j in range(i,499):
        for k in range(j,499):
            if(i+j+k==1000 and i**2+j**2==k**2):
                a=i
                b=j
                c=k
                break #the solution is unique, so when it finds a solution, it doesnt need to go through any other loops.
print(a*b*c,time.time()-s)

for i in range(1,499): #max value for any side of the triangle must be less      than the semiperimeter
    for j in range(i,499):
        for k in range(j,499):
            if(i+j+k==1000 and i**2+j**2==k**2):

There are a couple quick optimizations.

for i in range(1,333):

You can restrict this beyond 499.  We know that \$ a < b < c\$ and \$a + b + c = 1000\$, so we can say that \$3*a < 1000\$ or \$a \leq 333\$.

    for j in range(i,499): #max value for any side of the triangle must be less than the semiperimeter
        k = 1000 - i - j

We don't need to iterate to find k.  We can calculate it from i and j since we know that the three sum to 1000.

        if (i**2+j**2==k**2):

So we can just check that it's a Pythagorean triple.  The sum will always be correct, as that's how we select k.

Just calculating k instead of iterating to find it should reduce the time to less than a second.


Pascal's triangle in C I am relearning C so if you could point obvious faults in this solution to this problem I'd greatly appreciate such comments. Please note that I'm using GCC extensions to use larger than 32 bit numbers. Most importantly I want to be aware of the quality of memory management in particular.

http://news.ycombinator.com/item?id=3429466

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

int64_t* next_pascal_row(int64_t* previous_row, int64_t previous_size)
{
  int64_t i;
  int64_t* retval;
  retval = (int64_t*)malloc(sizeof(int64_t)*(previous_size+1));
  for (i=0;i<previous_size+1;i++) {
    if (i==0 || i == previous_size) {
      retval[i] = 1;
    } else {
      retval[i] = previous_row[i-1] + previous_row[i];
    }
  }
  return retval;
}

void print_row(int64_t* row, int64_t size)
{
  int64_t i;
  for(i = 0; i < size; i++) {
    printf("%lld ",row[i]);
  }
  printf("\n");
}

int main()
{
  int64_t* first_row;
  int64_t* previous_row;
  int64_t* next_row;
  int64_t i,size;
  first_row = (int64_t*)malloc(sizeof(int64_t));
  first_row[0] = 1;
  size = 1;
  print_row(first_row,size);
  previous_row = first_row;
  for(i = 0; i<31;i++) {
    next_row = next_pascal_row(previous_row,size);
    size++;
    print_row(next_row,size);
    free(previous_row);
    previous_row = next_row;
  }
  free(next_row);
  return 0;
}

Looks pretty good to me. If I were doing this for an interview question, I would take on the challenge of implementing it recursively. Here are some minor changes I would make:

You are hard-coding your for loop to 31 iterations. That ought to be a const uint32_t num_iterations = 31; at the top of your code. That makes it more clear to future users what to change if they want a different number of iterations.
You are using int64_t. These are all going to be positive numbers, so uint64_t is more appropriate.
Your variables i and size are int64_t. But your values are going to overflow 64-bits well before the line they are on gets that big. I'm guessing that uint32_t is enough.
In regards to the memory management, all of the allocing and freeing is expensive and slow. If you wanted to minimize that, you should alloc 2 rows of max length up front and then ping-pong them as previous/next rows.
In your next_pascal_row routine, you're using an array pointer called retval. I see that name way overused. It would be better called this_row or next_row.
There is no need for a first_row pointer. Just use the previous_row pointer in initialization.


LeetCode Problem 66 : Plus One I have started practicing problems in leetcode and I solved the following problem on LeetCode.

The challenge is :

  Given a non-empty array of digits representing a non-negative integer, plus one to the integer.

  The digits are stored such that the most significant digit is at the head of the list, and each element in the array contain a single digit.

  You may assume the integer does not contain any leading zero, except the number 0 itself.

 Example 1:
 Input: [1,2,3]
 Output: [1,2,4]
 Explanation: The array represents the integer 123.

 Example 2:
 Input: [4,3,2,1]
 Output: [4,3,2,2]
 Explanation: The array represents the integer 4321.

I am trying to get better at writing good code for my solutions. Please give me your suggestions

class Solution {
    public int[] plusOne(int[] a) {

        int length = a.length-1;

        while(length>=0)
        {
            a[length]=a[length]+1;
            int carry = a[length]/10;
            if(carry==1)
            {
                a[length]=0;
                length=length-1;
            }
            else
                break;
        }

        if(a[0]==0)
        {
            int [] array = new int[a.length+1];
            array[0]=1;
            for(int i=1;i<array.length-1;i++)
            {
                array[i]=0;
            }
            return array;
        }
        return a;

    }
}

I can't agree with tinstaafl's idea of creating a new array for every input number that starts with a 9. It's not needed in roughly 10% of the possible inputs.

I do however agree that using a for loop is better in this case. Anyone even mildly familiar with for loops can instantly recognise you're looping over each of the digits from right to left which isn't as clear in your while loop.

Instead of breaking the for loop and doing the special case testing afterwards you could also return early instead. With a little bit of rearanging this also simplifies the loop itself slightly.

The final for loop to set the result to all 0's is not needed. Creating a new int[] array will already set them all to 0 for you.

Putting this all together gives us the following implementation:

static int[] plusOne(int[] digits){
    for(int i = digits.length-1; i>=0; i--){
        if(digits[i]<9){
            digits[i]++;
            return digits;
        }
        digits[i]=0; //carry handled by next iteration in for loop
    }
    //didn't return yet so digits were all 9's
    int[] result = new int[digits.length+1];
    result[0] = 1;
    return result;
}


Confidence score calculation I was looking for a way to calculate a score 'x' for each pair of elements in two separate arrays.  The goal of the code is to return an output array containing the score for each entry in the input arrays.

from math import sqrt
import numpy as np

downs = np.genfromtxt('path\file.csv', dtype=long, delimiter=',', skiprows=1, usecols=(1,))
ups = np.genfromtxt('path\file.csv', dtype=long, delimiter=',', skiprows=1, usecols=(2,))

n = np.add(ups,downs)

def _confidence(n):

   for i, j in zip[n, ups]:

    z = 1.0 #1.0 = 85%, 1.6 = 95%

    if n == 0:
        return 0

    phat = float(ups) / n

    x = ((phat + z*z/(2*n) - z * sqrt((phat*(1-phat)+z*z/(4*n))/n))/(1+z*z/n))

    print [x]

It looks like you are trying to calculate the Wilson Score lower confidence bound for ranking as described here: http://www.evanmiller.org/how-not-to-sort-by-average-rating.html

There are a couple issues with the code. The main one that jumps out is that only some of the needed variables (n) are passed into the function and the other simply read from the module namespace (ups, downs). Encapsulating into functions can help. Another issue is where i and j are not actually used within the loop and are bad variable names anyway.

The following code also generates synthetic data so I could test the full code.

from math import sqrt
import numpy as np

def generate_data(filename):
    """ Generate synthetic data for StackExchange example """
    ratings_per_example = 10
    ratings = np.random.binomial(ratings_per_example,0.4,100)
    fake_ups = ratings
    fake_downs = ratings_per_example-ratings
    fake_ids = np.arange(len(ratings))
    merged = np.asarray(zip(fake_ids, fake_downs, fake_ups))
    np.savetxt(filename, merged, fmt='%d', delimiter=",", header='Fake Header')

def confidence(filename):
    """ Returns an array of lower confidence bounds from up/down rankings in file """
    downs = np.genfromtxt(filename, dtype=long, delimiter=',', skip_header=1, usecols=(1,))
    ups = np.genfromtxt(filename, dtype=long, delimiter=',', skip_header=1, usecols=(2,))

    lower_bound_ranks = [wilson_lower_bound(up, down) for up, down in zip(ups, downs)]

    return lower_bound_ranks

def wilson_lower_bound(up, down, z=1.0):
    """ http://www.evanmiller.org/how-not-to-sort-by-average-rating.html """
    n = up + down
    if n == 0:
        return 0.0
    else:
        phat = float(up) / n
        return ((phat + z*z/(2*n) - z * sqrt((phat*(1-phat)+z*z/(4*n))/n))/(1+z*z/n))

generate_data('foo.csv')
confidence('foo.csv')


Python unittest.mock.patch.object context manager I want to test my core of handler, so I wrote this:

@unittest_run_loop
async def test_post_response(self):
    async def coro(a):
        return a
    validator = Mock()
    validator.validate = Mock(return_value=True)
    model = Mock()
    model.create_instance = Mock(side_effect=coro)
    normalizer = Mock
    normalizer.normalized = Mock(side_effect=lambda a: a)
    with patch.object(CreateMixin, 'validator', new=validator):
        with patch.object(CreateMixin, 'model', new=model):
            with patch.object(CreateMixin, 'normalizer', new=normalizer):
                response = await self.client.post('/', data='{"body": "ok"}')
                text = await response.text()
                self.assertTrue(text == '{"body": "ok"}' or text == '{\n    "body": "ok"\n}')

How can I improve this using decorators instead?

First, note that this is all untested, but the ideas should hold up. I tried to abstract a lot of the logic by distilling the Mock object creation to a dictionary containing the data, but it turned into a bit of a mess. To start with, consider the following structure for your Mock object data:

async def coro(a):
    return a

mock_object_data = {
    'validator': {
        'subattrs': [
            {'validate':
                {'return_value': True}
            }
        ]
    },
    'model': {
        'subattrs': [
            {'create_instance':
                {'side_effect': coro}
            }
        ]
    },
    'normalizer': {
        'subattrs': [
            {'normalized':
                'side_effect': lambda a: a
            }
        ]
    }
}

This contains the name of each Mock object, any keyword arguments that are necessary, and any attributes that also need to be created (as Mock objects themselves). These can be created from this structure using the following functions:

def _generate_mock_object(attr_dict):
    if 'subattrs' in attr_dict:
        subattrs = attr_dict.pop('subattrs')
    else:
        subattrs = []

    mock_object = Mock(**attr_dict)

    for subattr_dict in subattrs:
        for subname, kwargs in subattr_dict.items():
            setattr(mock_object, subname, _generate_mock_object(kwargs))

    return mock_object

def generate_mock_objects(object_data):
    return {n: _generate_mock_object(d) for n, d in object_data.items()}

This allows for a arbitrary number of Mock objects to be created. However, this runs into some problems when we want to use the context manager since it is not natural to have an arbitrary number of required contexts. This can be achieved using a recursive function. Here, we will supply a sequence of callables that are to be opened with the with statement, and a callback function and arguments to be executed once all of the contexts are opened.

def recursive_context_manager(callable_seq, callback, *args, **kwargs):
    if not callable_seq:
        return callback(*args, **kwargs)
    else:
        current, *rest = callable_seq
        with current():
            return recursive_context_manager(rest, callback, *args, **kwargs)

Finally, we want to create a decorator which will do the following things: (a) create all of the Mock objects, (b) generate and enter the required contexts, and (c) execute the supplied function.

def bind_mock_object_data(data):
    def decorator(f):
        factory = lambda name, obj: lambda: patch.object(CreateMixin, name, new=obj)
        mock_callables = [factory(n, o) for n, o in generate_mock_objects(data).items()]
        def wrapper(*args, **kwargs):
            return recursive_context_manager(mock_callables, f, *args, **kwargs)
        return wrapper

After all of this overhead, your final test code comes out nice and clean.

@unittest_run_loop
@bind_mock_object_data(mock_object_data)
async def test_post_response(self):
    response = await self.client.post('/', data='{"body": "ok"}')
    text = await response.text()
    self.assertTrue(text == '{"body": "ok"}' or text == '{\n    "body": "ok"\n}')


Parsing function for a server, using the Jansson library I'm making a server in C that needs to parse JSON that follows oBIX protocol and I'm using the Jansson library for this. I've been programming in high level languages such as Java and C# for my entire career and the last time I touched C was way back in 7-8 years ago, so I'm mostly looking for guidance on writing C-style code.

Here's a sample JSON input from client to server:

{"obix":"obj","is":"obix:Request","rid":"1","children":[{"obix":"op","name":"add","is":"obix:Watch","children":[{"obix":"obj","is":"obix:WatchIn","children":[{"obix":"list","names":"hrefs","children":[{"obix":"uri","val":"/device/"}]}]}]}]}

So a function I have just needs to validate what kind of request the client is making. First, I write the following defines:

#define OBIX_REQUEST_INVALID -1
#define OBIX_REQUEST_WATCH 0
#define OBIX_REQUEST_UPDATE 1
....

Then in my callback function. I first validate whether it's a valid JSON input.

int callback_web_socket(struct libwebsocket_context * this,
                                   struct libwebsocket *wsi,
                                   enum libwebsocket_callback_reasons reason,
                                   void *user, void *in, size_t len)
{
    ....

    json_t *root;
    json_error_t error;

    root = json_loads(buf,0,&error);
    if (!root)
    {
         printf("\nInvalid JSON data.\n");
         break;
    }
    ....

If it's valid JSON, I call my parse function to determine request type

int request_type = obix_parse_request(root);
printf("\nRequest Type: %d\n", request_type);

And here's how my parse function looks like:

int obix_parse_request(const json_t *obix_root_obj) {
    json_t *obix_obj = json_object_get(obix_root_obj,"obix");

    if (!json_is_string(obix_obj) || strcmp(json_string_value(obix_obj),"obj") != 0) {
        return OBIX_REQUEST_INVALID;
    }

    json_t *first_child = json_object_get(obix_root_obj, "children");
    if (!first_child || !json_is_array(first_child)) {
        return OBIX_REQUEST_INVALID;
    }

    json_t *first_child_obj = json_array_get(first_child, 0);
    if (!first_child_obj || !json_is_object(first_child_obj)) {
        return OBIX_REQUEST_INVALID;
    }

    json_t *first_child_is_str = json_object_get(first_child_obj, "is");
    if (!first_child_is_str || !json_is_string(first_child_is_str)) {
        return OBIX_REQUEST_INVALID;
    }

    const unsigned char *request_type = json_string_value(first_child_is_str);
    if (strcmp(request_type, "obix:Watch") == 0) {
        return OBIX_REQUEST_WATCH;
    }

    ...
}

If I were coding in C#, this function would raise a huge red flag for lots of code duplication. But this is C, and I'm readily assuming the code duplication is part of my incompetency in C language.

How would you elegantly write this function? Aside from cleaning up the function, I'd like to hear some comments on my coding style.

A few notes:

My first concern with your program is the use of the Jansson library.  Looking at some benchmarks shows me that it is less than stellar. (The Y-axis is parses-per-second. Thus, higher is faster.)

So a new library choice may be in order.  I would highly recommend looking a jsmn. I have yet to find a faster (and more portable!) option out there.  Here is a benchmark for jsmn compared to Jansson (jsmn is 19.28 times faster).

\$
\newcommand\T{\Rule{0pt}{1em}{.5em}}
\begin{array}{|c|c|c|c|c|c|}
\hline \textbf{Project} & \textbf{License} & \textbf{API} & \textbf{Neg. tests} & \textbf{Followers} & \textbf{Benchmark} \T \\\hline
  \text{Jansson} \T & \text{MIT} & \text{DOM} & \text{yes} & \dfrac{234}{59} & 25 \text{MB/s}, 52 \text{s} \\\hline
  \text{jsmn} \T & \text{MIT} & \text{custom} & \text{yes} & \dfrac{65}{4} & 482 \text{MB/s}, 2.3 \text{s} \\\hline
\end{array}
\$
You have used the preprocessor to define some oBIX request values.

#define OBIX_REQUEST_INVALID -1
#define OBIX_REQUEST_WATCH 0
#define OBIX_REQUEST_UPDATE 1

You could use an enum here instead.

typedef enum
{
    INVALID = -1,
    WATCH = 0,
    UPDATE = 1
} ObixRequest;

You can remove any != 0 tests from your if conditions for maximum C-ness.
You sometimes use printf() when you don't have to.

printf("\nInvalid JSON data.\n");

Use puts() for statements like this when you aren't formatting the string.  As a plus, you don't have to include the \n characters.

puts("Invalid JSON data.");


Parsing and data presentation in Django For some time now, I've been using a pattern which I think could be done more elegantly.
I have a lot of XML files that need to be represented in a Django views. My current approach is like this:

I get a XML response from an API which gives me several documents.

First I define a mock class like this:

class MockDocument:
    pass

Then I write an XPath expression that gives me a list of the document nodes:

from lxml import etree
with open("xmlapiresponse.xml", "r") as doc:
    xml_response = doc.read()
tree = etree.fromstring(xml_response)
document_nodes = tree.xpath("//documents/item")

Now I iterate over the different documents and their fields using nested for-loops:

# list for storing the objects
document_objects = []
for document in document_nodes:
    # create a MockDocument for each item returned by the API
    document_object = MockDocument()
    for field in document:
        # store the corresponding fields as object attributes
        if field.tag == "something":
            document.something = field.text
    document_objects.append(document)

Now I can pass the list of mock objects to a Django view and represent them in the template in whichever way I want. Could this be done in a simpler fashion, though?

Using a generator function gives you some flexibility.  You don't have to create a list.

def generate_documents( document_nodes ):

    for document in document_nodes:
        # create a MockDocument for each item returned by the API
        document_object = MockDocument()
        for field in document:
            # store the corresponding fields as object attributes
            if field.tag == "something":
                document.something = field.text
        yield  document_object

You can do this of the list object is somehow important.

document_objects = list( generate_documents( document_nodes ) )

Or, more usefully, this

for d in generate_documents( document_nodes ):
    ... process the document ...

Which never actually creates a list object.


Underscore and general app design review I'm working on my larger project to date, and on top of that I'm using underscore for the first time.
All is working perfectly but I'd like some criticism on the use of underscore templating and underscore after function.

The goal of the app is to generate on the dom a messaging interface starting from an empty  div. It gets initialized by passing the jQuery selector of that empty div.

I took out most of the code and left what I'd like to be reviewed the most, hope the flow of the app is quite clear anyway.

(function($) {
$(document).ready(function() {

var mssg_tmpl = {
        msg_container : ' \
    <div class="row-fluid msg-container well" id="msg-<% id %>"> \
        some html \

',

        msg_reply : ' \

 \
        some html \

'

};

mssg = {
    init : function(selector) {
        this.$container = $(selector);
        this.tmpl = {};         // List of compiled templates functions
        this.compileTemplates(mssg_tmpl);
        this.messages = [{      // THIS IS HARDCODE TO ILLUSTRATE BASIC STRUCTURE
            id: 1,              // Thread opener
            children: [2,3,4]   // Replies
        }, {
            id: 5,
            children: [6,7,8]
        }];

        for ( var i in this.messages) {
            this.$container.append(this.tmpl.msg_container(this.messages[i])); // fn msg_container is a compiled template
            this.messages[i].$container = $('#msg-' + this.messages[i].id);
            this.fetchChildren(this.messages[i]);
        }

        this.bindEvents();
        this.collapseMessages();
        return this;
    },

    compileTemplates : function(templates) {
      for ( var k in templates) {
        this.tmpl[k] = _.template(templates[k]);
      }
    },

    fetchChildren : function(message) {

        var renderChildren = _.after(message.children.length, function() {
            children = _.sortBy(children, 'id'); // Sort replies (oldest first)
            for ( var i in children) {
                message.$container.find('div.msg-reply-container').before(mssg.tmpl.msg_reply(children[i])); // fn msg_reply is a compiled template
            }
        });

        var children = [];

        for ( var i in message.children) {
            $.ajax({
                url : 'getmessage.htm?messageId=' + message.children[i]
            }).done(function(response) {
                children.push(response);
                renderChildren();
            });
        }

    },
};

mssg = mssg.init('#messaging');

});

})(jQuery);

Since you asked to review only the underscore part,

Just had one remark that I would suggest to move the templates to a script tag "text/template" rather than defining them as a variable.

eg:

Compile and use the same like below,

var msg_container = _.template($('#msg_container').html());
var msg_reply = _.template($('#msg_reply').html());

Though you asked to review only the '_' part but couldn't resist asking this question, From your example snippet I see 6 potential AJAX calls based on the loops. Wouldn't there be a performance problem since it completely relies on the number of messages and replies and what if the messages and replies are more or I could be missing the overall picture :)


Find the greatest common divisor of n numbers Just had this as a practice test problem and I'm curious how to optimize for performance. Thanks!

The greatest common divisor (GCD), also called the highest common factor (HCF) of N numbers is the largest positive integer that divides all numbers without giving a remainder.

Write an algorithm to determin the GCD of N positive integers.

function generalizedGCD(num, arr)
{
    // find the factors of lowest member of arr and then check if every other number is divisible
    arr.sort( function( a, b ) { return a-b; });
    const lowest = arr[0];
    const factors = [];

    for ( let i = 1; i <= lowest; i++) {
        if ( lowest % i === 0 ) {
            factors.push(i);
        }
    }

    // now check to see if each member of the factors array divides into each member of the original numbers array. If not, remove it from the array.
    for ( let j = 1; j < num; j++) {
        for ( let k = 0;  k < factors.length; k++) {
            if ( arr[j]  % factors[k] != 0 ) {
                factors.splice( k, 1);
            }
        }
    }

    return factors[factors.length-1];
}

As I am not a native in Javascript, I'll try to review your algorithm:

start from the lowest value and its factor is a good idea to improve speed
for your second loop I suggest you to start not from the lowest factor but start from the biggest factor. GCD is trying to find the biggest factor, So if you start from the biggest number, after you find a number that can divide all number in arr you can break your loop, it's more efficient.
I've tested it with generalizedGCD(2,[27,8]) and it returns 4 instead of 1, why? the problem is in your second loop, the size of factors changed after you delete its element and you accidentally "skip" some elements. If we try to simulate this with the example above:

    generalizedGCD(2,[8,27])
    ...
    k=0 factors=[1,2,4,8]  arr[j]=27 {27 is divisible with factors[k]=1}
    k=1 factors=[1,2,4,8]  arr[1]=27 {27 is not divisible with factors[k]=2}
    k=2 factors=[1,4,8]    arr[1]=27 {27 is not divisible with factors[k]=8} // you skipped 4
    ...
    factors=[1,4]

The solution that I can think is subtract k by 1 so it doesn't "move" when you are deleting an element in factors:

    if ( arr[j]  % factors[k] != 0 ) {
        factors.splice( k, 1);
        k--;
    }

Suggestion for another algorithm

I think it'll be better if you use faster algorithm: Euclid Algorithm. You can find GCD of every number in array by find GCD each two of them.

GCD( GCD( GCD(a[0], a[1]), a[2]), ...)

It's simple recursion:

function gcd(a, b) {
    if(b === 0) {
        return a;
    }

    return gcd(b, a%b);
}

function generalizedGCD(num, arr)
{
    factors = arr[0];

    for ( let i = 1; i < num; i++) {
        factors = gcd(factors, arr[i])
    }

    return factors;
}


Syncing Redux state with the URI in 2020 I have a React/Redux/Router/Reselect app in which I am syncing some parts of the redux state with the URL. As an interim, hacky measure (I've only been learning how to write wepapps for about 6 months so I'm very much a n00b right now) I’m currently implementing that sync as follows:

A selector provides the parts of the state which deviate from the default (e.g. the tab has changed from default and the filter bar is open and one of the filters has been changed from the default):

export const getStateConfiguration = createSelector(
  [getAllActiveFilters, getFilterBarVisible, getSelectedChart, getCurrentTab],
  (filters, filterBarVisible, selectedChart, currentTab) => {
    // inspect filters to create for deviations to default and return a config object with the stuff i need to write to the URI
     return config;
   }
);

A useEffect() in App.js is listening to that selector and when it changes calls a function in which JSON stringifies the selector's result to history. (I get that history object from router, because if I use uesHistory() from, say, BrowserRouter, I can't seem to escape infinite loop dependencies in useEffect().)

useEffect(() => {
  // some lines omitted for brevity
  writeToHistory(currentJsonConfig, stateConfiguration);
}, [dispatch, dbStatus, currentJsonConfig, decodedPathname, stateConfiguration]);

At app startup, another useEffect() in App.js calls a function which inspects the location and passes that to initialise the Redux reducers.

useEffect(() => {
    // some lines omitted for brevity
    dispatch(initialise(config));
  }
}, [dispatch, dbStatus, currentJsonConfig, location]);

As you can probably already see, all of this performs horrifically. Every time a single thing in state changes, the selector has to examine the whole state model, reparse it, provide that to useEffect(), which stringifies the whole thing again and writes it to history.

Due to performance and the hacky nature of my bespoke solution, I now plan to refactor it all to use a 3rd party library(s) which does all this for me. I’ve looked at things like use-query-params (https://github.com/pbeshai/use-query-params) and connected-router (https://www.npmjs.com/package/connected-react-router) but I was wondering if anyone has any experience, guidance, or opinions they can offer on the subject, for the most efficient, straightforward and current solution to the problem.

If it helps, you can see my source code here.  The most relevant files which contain my hacky solutions are probably /App.js, /state/selectors/StateConfigurationSelector.js and /HistoryWriter.js.  More generaly, if you do take a look and see any other n00b issues, do feel free to tell me!

Thanks :)

I ended up choosing the use-query-params library and writing a class which has several useEffects to sync state.  One of the useEffects fires once at load, to update the redux state with the query params in the URL, and the others each hang off one aspect of the state and update the query params when that item changes.  It works on the general rule that if the state element is not the default state, it should not add (or remove) the related query param, but if it is not default, then add the query param.

I couldn't just replace the relevant redux state with state in the query params for various reasons: 1) It's useful to keep state elements using the same pattern throughout, for modularity, and so that they benefit from redux dev tools; 2) the query params don't remember the state when the url changes; 3) the state does not care whether or not the item is default - it will always write it.

For those interested, my implementation is here: https://github.com/adybaby/taskmaster/blob/master/src/HistoryWriter.js and a demo here: https://master.d29jyoh6h48gjm.amplifyapp.com/


Database class using PDO The point of this question

I'm actually using it while developing a simple application and it seems to cover all my needs. Also it uses PDO so that we don't really have to worry about SQL Injection. I know I usually code strange, but I hope you could give me suggestions and feedback in order to improve it.

Code: Database Class

/* Operate on the database using our super-safe PDO system */
class db
{
    /* PDO istance */
    private $db = NULL;
    /* Number of the errors occurred */
    private $errorNO = 0;

    /* Connect to the database, no db? no party */
    public function __construct()
    {
        try
        {
            $this->db = new PDO(
                'mysql:dbname='.reg::get('db-name').';host='.reg::get('db-host'), reg::get('db-username'), reg::get('db-password')
            );
        }
        catch (Exception $e)
        {
            exit('App shoutdown');
        }
    }

    /* Have you seen any errors recently? */
    public function getErrors() { return ($this->errorNO > 0) ? $this->errorNO : false; }

    /* Perform a full-control query */
    public function smartQuery($array)
    {
        # Managing passed vars
        $sql = $array['sql'];
        $par = (isset($array['par'])) ? $array['par'] : array();
        $ret = (isset($array['ret'])) ? $array['ret'] : 'res';

        # Executing our query
        $obj = $this->db->prepare($sql);
        $result = $obj->execute($par);

            # Error occurred...
            if (!$result) { ++$this->errorNO; }

        # What do you want me to return?
        switch ($ret)
        {
            case 'obj':
            case 'object':
                return $obj;
            break;

            case 'ass':
            case 'assoc':
            case 'fetch-assoc':
                return $obj->fetch(PDO::FETCH_ASSOC);
            break;

            case 'all':
            case 'fetch-all':
                return $obj->fetchAll();
            break;

            case 'res':
            case 'result':
                return $result;
            break;

            default:
                return $result;
            break;
        }
    }

    /* Get PDO istance to use it outside this class */
    public function getPdo() { return $this->db; }

    /* Disconnect from the database */
    public function __destruct() { $this->db = NULL; }
}

Use

$db = new db;
$user = $db->smartQuery(array(
    'sql' => "SELECT UserName FROM `post` WHERE UserUID = :uid",
    'par' => array('uid' => $uid),
    'ret' => 'fetch-assoc'
));
echo $user['Username'];

What I think is wrong

Well, I have encountered these 2 points while revisiting this code, and I'd like to get some feedback about them particularly:

The error system (which, let's face it, sucks right now)
The first try-catch code, which is actually working, but I never use that so, please, look at it.

Also if my application cannot connect to the database, most (if not all) features cannot be activated (such-as the PHP error log trough a database record, so that every times an error occurred, the admin is warned trough the application itself).

A few things.

When you connect, don't hard code mysql, you are already fetching all the db info from a registry/config ... make the the dbtype configurable as well. That is after all, the point of PDO.

When you catch errors in connection or when executing a query, use the PDOException class

use exceptions for your prepare and execute calls instead of counting the errors that happens...as that's not really useful (it will be obvious an error happened, what I care about is what that error actually is).

personally, I would break down this class into specific methods. instead of connecting to the db in the constructor, I would have a connect() method to do this. I would also have a query, prepare, and bind method and then have interaction method: delete, update, insert and then a set of fetch methods, fetchOne, fetchAll, fetchCol, etc.

Also the connect method would check for a set conn, and only attempt to connect if you didn't already have an existing connection. The query method would call connect(). The reason for this is that you may include your db class throughout your code, even if you dont fetch anything in particular requests...with your code if this is the case a connection will be made regardless. What you want is lazy loading...meaning a db connection will only be made if you actually attempt to use the db with a fetch/update/delete/insert/etc.

Here is some sample pseudo code

$db = new MyPdoClass();
$db->connect();
$stmt = $db->query($sql, $params);
$results = $stmt->fetchAll(PDO::FETCH_ASSOC);
// or call your MyPdoClass::fetchAll, which would do that so you would just call
$results = $db->fetchAll($sql, $params);


Euler's totient exactly k times Given n and k, I need to find Euler's totient exactly k times where n and k are very large numbers. Can anyone help with an optimized solution for a large number?

#include <iostream>
using namespace std;

 int gcd(int a, int b)
{
    if (a == 0)
        return b;
    return gcd(b%a, a);
}

int phi(unsigned int n)
{
    unsigned int result = 1;
    for (int i=2; i < n; i++)
        if (gcd(i, n) == 1)
            result++;
    return result;
}

int main()
{
    long n,i,k;

    cin >>n>>k;
    long a[n];

    for (i=1; i<=k; i++)
        n = phi(n);

    cout << n<<endl;

    return 0;
    }

Do prime factorization

Your current phi() function is correct but slow.  You would do better by finding the prime factorization of n and using the prime factors to calculate phi.  Your current phi() takes \$O(n)\$ time, but a prime factorization takes \$O(\sqrt n)\$ time.

Stop at one

At some point, if k is large, you will reach n of 1.  At that point you can stop because phi(1) = 1.  Actually if at any point k >= n, the answer should be 1 because phi(n) reduces n by at least 1.


Feedback on a programming practice problem in C This is a programming practice that our teacher gave us, and I would appreciate if someone can look over my program and tell me if I did a good job.

Basically, the context is I joined a company and am supposed to write a program in C that takes three inputs (ft of steel, number of ball bearings, and lbs of bronze), and returns how many bells and whistles can be made the next day.

Making a whistle requires 0.5 ft of steel and 1 ball bearing. Making a bell requires 1.10 lbs of bronze (.75 lbs of bronze and 1 clapper (.35 lbs of bronze)).

This is the program I have written.  I apologize if it seems messy.

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

void PrintLine();
int CalculateWhistles(int steel, int bearings);
int CalculateBells(int bronze);

int main() {
    bool runProgram = true;
    int runAgain = 0;

    while (runProgram == true) {
        int ftSteelIn = 0;
        int numBallBearingIn = 0;
        int ibsBronzeIn = 0;

        printf("\n\n"); // Formatting...
        PrintLine();

        printf("    How many feet of steel was received today:               "); // Steel received
        scanf("%d", &ftSteelIn);
        printf("    How many ball bearings were received today:              "); // Ball bearings received
        scanf("%d", &numBallBearingIn);
        printf("    How many pounds of bronze was received today:            "); // Bronze received
        scanf("%d", &ibsBronzeIn);

        PrintLine(); // Print line

        PrintLine(); // Print line

        // Print company logo and program title
        printf("                                Acme Corporation\n");
        printf("                                Product Report\n");

        PrintLine();// Print line

        // Print number of bells and whistles
        printf("    Number of Bells                                                      %d\n", CalculateBells(ibsBronzeIn));
        printf("    Number of Whistles                                                   %d\n", CalculateWhistles(ftSteelIn, numBallBearingIn));

        PrintLine(); // Print line
        PrintLine();

        printf("\n\n");
        PrintLine();

        // Ask the user if they would like to run the program again
        printf("    Would you like to run the program again? ('1' for yes, '2' for no): ");
        scanf("%d", &runAgain);
        PrintLine();

        // If user does not say 'yes', end the program and say goodbye
        if (runAgain != 1) {
            printf("\n    Exitting the program. Have a good day.\n\n");
            PrintLine();
            runProgram = false;
        }
    }

    return 0;
}

void PrintLine() {
    int i = 0;
    printf("    ");
    while (i <= 70) {
        printf("-");
        i++;
    }
    printf("\n");
}

int CalculateWhistles(int steel, int bearings) {
    steel = steel / 0.5;
    if (steel < bearings) {
        return steel;
    }
    else if (bearings < steel) {
        return bearings;
    }
}

int CalculateBells(int bronze) {
    int bells = bronze / 1.1;
    return bells;
}

It runs fine with no errors. There is a second part where I calculate the remainder, but I want some feedback before I go on to that part.

Any advice would be appreciated, how to make it more efficient or anything like that (I'm just learning C).

Roughly 50% of all programming is about dealing with errors. You say your code works, but it doesn't handle errors and therefore doesn't work.

Example 1:

How many feet of steel was received today:  four

Program doesn't say "Error, try again". Instead it displays all prompts without waiting for any user input and exits.

Example 2:

How many feet of steel was received today:  3.7

Program doesn't say "Error, try again". Instead it displays all prompts without waiting for any user input and exits.

Example 3:

How many feet of steel was received today:  3 4

Program doesn't say "Error, try again". Instead it assumes 3 feet of steel; displays "How many ball bearings" and doesn't wait for user input and assumes 4 ball bearings.

Example 4:

Would you like to run the program again? ('1' for yes, '2' for no): yes

Program doesn't say "Error, try again". Instead it exits.

Example 5:

How many feet of steel was received today:               -99
How many ball bearings were received today:              -99
How many pounds of bronze was received today:            -99

Program decides you can make -89 bells and -198 whistles.

Example 6:

How many feet of steel was received today:               999999999999999999
How many ball bearings were received today:              999999999999999999
How many pounds of bronze was received today:            999999999999999999

Program decides you can make -1351471477 bells and -2147483648 whistles.

Example 7:

How many feet of steel was received today:               200
How many ball bearings were received today:              100

Program decides you can make 0 whistles. Note: unlike the previous examples, this is not a "failed to check for invalid input" problem.


Confirm regex for validating password policy I have the following password policy:

At least 8 characters in length
At least one digit
At least one special* character

*I consider any character that is not a letter, digit or space to be a special character.

I've come up with the following regex for enforcing this policy, and it seems to be working.

^(?=.*?[0-9])(?=.*?[^a-zA-Z0-9 ]).{8,}$

Are there any drawbacks or pitfalls with my regex?

I find the problem with regular expressions is how hard they are to read, and how quickly they become very complicated.

Instead of using a single regex to check for all your conditions I would define a class for a the password that you can then set a series of simple rules for.

This does use static methods, which is not to everyone's taste.

interface Rule
{

    public function check(string $value): bool;
}

class LengthRule implements Rule
{
    protected $length;

    /**
     * LengthRule constructor.
     *
     * @param $length
     */
    public function __construct($length)
    {
        $this->length = $length;
    }

    public function check(string $value): bool
    {
        return mb_strlen($value) >= 8;
    }
}

class DigitRule implements Rule
{
    public function check(string $value): bool
    {
        return preg_match('/\d/', $value) === 1;
    }
}

class SpecialCharacterRule implements Rule
{
    public function check(string $value): bool
    {
        return preg_match('/[^a-zA-Z\d ]/', $value) === 1;
    }
}

class Password
{
    /** @var Rules[] */
    protected static $rules = [];

    protected $value;

    /**
     * @param string $value
     */
    public function __construct(string $value)
    {
        foreach (static::$rules as $rule) {
            if (false === $rule->check($value)) {
                throw new InvalidArgumentException("The value doesn't" .
                    " match all rules.");
            }
        }

        $this->value = $value;
    }

    /**
     * @param Rule $rule
     */
    public static function registerRule(Rule $rule)
    {
        self::$rules[] = $rule;
    }

}

Password::registerRule(new LengthRule(8));
Password::registerRule(new DigitRule());
Password::registerRule(new SpecialCharacterRule());

new Password('12343.4654j');


Throttled execution of an enumeration of Tasks Example of use:
var users = await usernames.SelectTaskResults(GetUserDetails, 4);

where GetUserDetails(string username) is a method that calls HttpClient to access an API and returns a User object. The SelectTaskResults returns the awaited tasks. At most, there will be 4 concurrent threads calling the API at the same time for this example.
Concerns:
I'm worried .Wait() could cause a deadlock.
Code:
public static async Task<IEnumerable<TResult>> SelectTaskResults<TInput, TResult>(
        this IEnumerable<TInput> source,
        Func<TInput, Task<TResult>> taskFunc,
        int degreesOfParallelism = 1,
        bool throwFaulted = false,
        CancellationToken cancellationToken = default(CancellationToken))
    {
        // Task.Run creates the task but it doesn't start executing immediately - for debugging
        var tasks = source
            .Select(input => Task.Run(() => taskFunc(input), cancellationToken))
            .ToArray();

        Parallel.ForEach(tasks,
            new ParallelOptions { MaxDegreeOfParallelism = degreesOfParallelism }
            , task =>
            {
                try
                {
                    task.Wait(cancellationToken); // .Start() doesn't work for promise-like tasks
                }
                catch (Exception)
                {
                    if (throwFaulted)
                    {
                        throw;
                    }
                }
            }
        );

        var output = await Task.WhenAll(tasks.Where(x=> !x.IsFaulted));

        return output.AsEnumerable();
    }

A TPL inside async implementation: mixing of implementaion concepts makes the code complicated or even buggy. Finally Parallel.ForEach is blocking the current thread which isn't allowed in async programming.
Also avoid blocking operations on Task e.g. .Wait() or .Result for not completed Task.
But I suggest totally different approach. Consider the simplified example.
public class ThrottledApiClient : IDisposable
{
    private readonly HttpClient _client;
    private readonly SemaphoreSlim _semaphore;

    public ThrottledApiClient(string baseUrl, int concurrentDegree = 0)
    {
        _client = new HttpClient();
        _client.BaseAddress = new Uri(baseUrl);
        _semaphore = new SemaphoreSlim(concurrentDegree == 0 ? Environment.ProcessorCount * 2 : concurrentDegree);
    }

    public async Task<string[]> PostJsonRequestsAsync(IEnumerable<(string, string)> requests, CancellationToken token)
    {
        List<Task<string>> tasks = new List<Task<string>>();
        // wrap the loop with try-catch (OperationCanceledException) to handle Cancellation right here if necessary
        foreach ((string path, string json) in requests)
        {
            await _semaphore.WaitAsync(token);
            tasks.Add(PostJsonAsync(path, json, token));
        }

        return await Task.WhenAll(tasks);

        // or with filter
        //await Task.WhenAll(tasks);
        //return tasks.Where(t => t.IsCompletedSucessfully).Select(t => t.Result).ToArray();
    }

    private async Task<string> PostJsonAsync(string url, string json, CancellationToken token)
    {
        try
        {
            using HttpContent content = new StringContent(json, Encoding.UTF8, "application/json");
            using HttpResponseMessage response = await _client.PostAsync(url, content, token);
            // response.EnsureSuccessStatusCode(); // to throw on not success instead of returning result
            return await response.Content.ReadAsStringAsync(token);
        }
        // catch (HttpRequestException)
        // catch (OperationCanceledException) { return null; } // to swallow cancellation
        finally
        {
            _semaphore.Release();
        }
    }

    public void Dispose()
    {
        _client.Dispose();
        _semaphore.Dispose();
    }
}

And the demo.
(string, string)[] requests = new (string, string)[]
{
    ("/getAccounts", "{...}"),
    ("/getMediaPath", "{...}")
};

using (var api = new ThrottledApiClient("https://api.example.com", 4))
using (var cts = new CancellationTokenSource())
{
    var results = await api.PostJsonRequestsAsync(requests, cts.Token);
    //...
}

You can invoke PostJsonRequestsAsync even concurrently multiple times, even from different threads but only 4 requests will be active simoultaneously.
SemaphoreSlim is default synchronization tool in async programming for now.


Settings class for web app I needed to write a settings class for my web app; ideally this class should read its settings from a file (and be able to save them back to the file), and have only 1 instance per application - all threads read the same object, so that the settings only need to be loaded from the file when the app runs for the first time.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.ComponentModel;
using System.Security.Cryptography;
using System.Configuration;
using System.Xml.Serialization;
using System.Web.Caching;
using System.IO;

namespace Secure_Password_Repository.Settings
{
    [Serializable]
    public sealed partial class AppSettings
    {
    private static readonly XmlSerializer serial = new XmlSerializer(typeof(AppSettings));
    private static AppSettings instance;
    private static Object thisLock = new Object();

    private static void serializeToDisk(string key, CacheItemUpdateReason reason, out Object expensiveObject, out CacheDependency dependency, out DateTime absoluteExpiration, out TimeSpan slidingExpiration)
    {

        string filename = HttpContext.Current.Server.MapPath("settings.xml");

        expensiveObject = instance;
        dependency = new CacheDependency(filename);
        absoluteExpiration = Cache.NoAbsoluteExpiration;
        slidingExpiration = Cache.NoSlidingExpiration;

        using (StreamWriter sw = new StreamWriter(filename))
        {
        serial.Serialize(sw, instance);
        }
    }

    public static void Save()
    {
        string filename = HttpContext.Current.Server.MapPath("settings.xml");
        using (StreamWriter sw = new StreamWriter(filename))
        {
        serial.Serialize(sw, instance);
        }
    }

    public static AppSettings Default
    {
        get
        {

        Cache cache = HttpRuntime.Cache;
        string filename = HttpContext.Current.Server.MapPath("settings.xml");

        if (cache[filename] != null) {
            return (AppSettings)cache[filename];
        }

        lock (thisLock)
        {

        if (cache[filename] != null) {
            return (AppSettings)cache[filename];
        }

            using (StreamReader sr = new StreamReader(filename))
            {
            instance = (AppSettings)serial.Deserialize(sr);
            cache.Insert(filename, instance, null, Cache.NoAbsoluteExpiration, Cache.NoSlidingExpiration);

            if (instance.EncryptionKeySalt == "")
            {
                //random number generator
                RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider();

                //create 20 random bytes for the salt
                byte[] CryptoBytes = new byte[40];
                rngCsp.GetBytes(CryptoBytes);

                rngCsp.Dispose();

                instance.EncryptionKeySalt = Convert.ToBase64String(CryptoBytes);
            }

            return (AppSettings)cache[filename];
            }
        }

        }
    }

    public string LogoImage{ get; set; }

    public string DefaultAccountRole{ get; set; }

    public string SMTPServerAddress{ get; set; }

    public string SMTPServerUsername{ get; set; }

    public string SMTPServerPassword{ get; set; }

    public string EncryptionKeySalt{ get; set; }

    }
}

I'm just curious: is this "good code"? Are there any potential pitfalls I'm not seeing? is there anything I could be doing better?

Small issues:

Personally I'd indent things in between by curly braces
I'd use string.Empty instead of ""
I'd probably put my default public properties before my custom ones

As a higher level question, do you need to use an XML settings file with an explicit lock? Could you not use a custom section in .config file of some kind?


C++ smart pointers and the Service Locator (anti-?)pattern So I'm working on a game framework and one of the things I need are access to stateful "services" that can be accessed from all over. I was initially going to use the Singleton pattern but decided to try and come up with alternatives to avoid the short comings of that pattern.

So here is what I've got so far.

Main.cpp

#include "Engine.h"
#include "Logger.h"

int main(int argc, char* argv[]) {
    Engine engine;

    // old
    {
        auto logger = new Logger();

        engine.provide(logger);
        engine.getLoggerOld().log("old TEST");

        engine.provide(nullptr);
        delete logger;
    }
    engine.getLoggerOld().log("old TEST");//wont print, just calls empty funcs
    // old

    // new
    {
        auto splogger = std::make_shared<Logger>();

        engine.provide(splogger);
        engine.getLoggerNew()->log("new TEST");
    }
    engine.getLoggerNew()->log("new TEST");//wont print, just calls empty funcs
    // new

    return 0;
}

Engine.h

#pragma once

#include <memory>

#include "ILogger.h"
#include "NullLogger.h"

class Engine {
public:
    Engine() {
        //old
        serviceLogger = &nullLogger;

        //new
        spLoggerService = std::make_shared<NullLogger>();
    }

///////// old
public:
    void provide(ILogger* service) {
        if (service) {
            serviceLogger = service;
        } else {
            serviceLogger = &nullLogger;
        }
    }

    ILogger& getLoggerOld() const {
        return *serviceLogger;
    }
private:
    ILogger* serviceLogger;
    NullLogger nullLogger;

///////// old

///////// new
public:
    void provide(std::shared_ptr<ILogger> loggerService) {
        wpLoggerService = loggerService;
    }

    std::shared_ptr<ILogger> getLoggerNew() {
        if (wpLoggerService.expired()) {
            wpLoggerService = spLoggerService;
        }
        return wpLoggerService.lock();
    }
private:
    std::shared_ptr<ILogger> spLoggerService;
    std::weak_ptr<ILogger> wpLoggerService;

///////// new
};

I'm a bit unsure on how I should implement the services, in the above example(s) "Engine"(The Service Locator) essentially is designed to never really "own" the services, and instead it only holds a reference to the services, it's then the externally determined if/how/when the services become invalid.

In the example sections labeled "old", these are basically how I saw this pattern implemented elsewhere. I don't like that its left up to how its used to determine if the pointers will be valid or not.

In the example sections labeled "new" I tried to implement the more modern smart pointers, I like the automatic cleanup of the expired references, etc. But I'm still not sure if I like the way the references are handled.

So I guess my questions are:

Does it make sense to give total ownership of the service to the Engine(locator) and allow its life cycle to manage cleanup?
Should I stay with the raw pointer "old" way I found in online examples?
Should I keep going with the "new" smart pointer way?(one thing I'm not a fan of here is needing the if (wpLoggerService.expired()) check as well as the .lock() when accessing a service.(maybe there is a better smart pointer way to avoide this?)

Also if it makes sense for the Engine(Locator) to manage the life-cycle of the services, I was thinking of maybe making the "provide" function similar to that of the std::vector's emplace_back where you would do something like: engine.provide<Logger>(/*ctor args*/) and the .provide<T>() function would instantiate the object internally to prevent any ownership/access to it externally. (This just seems like a more reasonable way to let the Engine control its life-cycle, or is this dumb?)

Disclaimer: New to C++, first time using smart pointers, please be gentle :3

Note that the "old" version should store the logger in a std::unique_ptr in main, rather than explicitly calling new and delete.

The simplest possible thing would be something like this:

struct Services // previously `Engine`
{
    Logger logger;
};

int main()
{
    {
        Services services;
        services.logger.log("yep");
    }

    //services.logger.log("nope"); // obviously doesn't compile
}

To use the logger "somewhere else" we can pass it into an object constructor by reference, (or pass the entire Services object by reference):

struct Object
{
    explicit Object(Logger& logger):
        logger(&logger) { }

private:

    Logger* logger;
};

If necessary, the object can store the logger (or Services) by reference (which has the side-effect of preventing us defining assignment operators for Object as we can't re-assign a reference), or by pointer (which allows us to define assignment operators, but can also be reassigned to a nullptr (for better or worse)).

Either way, we still depend on the lifetime of the Logger being longer than the lifetime of the Object. With C++, this is usually enforced simply by the implicit order of destruction of stack objects (and any child objects they contain), e.g.:

{
    Services services;
    Object object(services.logger);

} // destruction in reverse order: first object, then services.

It might seem like we can use std::shared_ptr and std::weak_ptr to enforce the above constraint (longer service lifetime), but it adds overhead and complexity, and can lead to more serious issues. When we promote a weak_ptr to a shared_ptr, the new shared_ptr also "owns" the service. So the lifetime is no longer defined by Services, but by the service user as well.

If we enforce the Object lifetime constraint properly ourselves, we don't need this. If we don't enforce that constraint, then the exact moment of a service being destroyed becomes very difficult to determine, which leads to similar problems as with Singleton objects.

This article has some more arguments against using std::shared_ptr, and points out that what we actually need is some sort of checked pointer or borrowed reference for debugging. Unfortunately, this doesn't exist in the C++ standard library.

Note that if it's actually necessary to swap out, or destroy a service part-way through use, we can't store any pointer or reference to the service. It must be fetched from Services every time it's accessed. This is probably quite difficult to enforce though.

In short:

Avoid using this pattern if possible.
Pass dependencies by reference in constructors (or other functions).
Ensure that lifetimes are well-defined and easy to understand (i.e. avoid std::shared_ptr).
Ensure that the lifetime of the Engine / Services exceeds the lifetime of anything that uses them.
Don't move or copy the individual services, or the Services class.

And to answer your questions:

Yes.
Allowing direct access to the service member, returning a raw pointer from an accessor function, or returning a reference from an accessor function are all reasonable options. The first is simplest unless you have some other constraints (enforcing access through an interface, or enforcing const correctness).
I'd argue not (see above).


Efficiently creating a menu consisting of multi-level structure I was asked to modify existing code to allow multi-level page structure. Each page can have one parent, and one page can have multiple children.

I will present following code. My question is is there a more efficient way of doing this? I am hard-coding the number of levels that are reachable.

Is there easier way to do  this automatically without hard coding? Recursion perhaps?

function createMenu($parents) {
    foreach ($parents as $parent) {
        $parentName = $this->getPagefromID($parent);
        $kids = $this->areThereKids($parent);

        if ($kids == "yes") {
            echo "
\n";
            $query = "SELECT * FROM pages";
            $result = mysql_query($query) or die('Query failed: ' . mysql_error());
            while ($row = mysql_fetch_assoc($result)) {
                $thisparent = $row['Parent'];
                $name = $row['Name'];
                $id2 = $row['ID'];
                if ($thisparent == $parent) {
                    echo "
" . $name . "
\n";
                    if($this->areThereKids($id2) == "yes") {
                        $children = $this->getMyKids($id2);
                        foreach($children as $kid) {
                            $info = $this->getPersonInformation($kid);
                            $kidid = $info[0]["ID"];
                            echo "
" . $info[0]['Name'] . "
\n";
                            // more sub
                            if($this->areThereKids($kidid) == "yes") {
                                $children1 = $this->getMyKids($kidid);
                                foreach($children1 as $kid1) {
                                    $info1 = $this->getPersonInformation($kid1);
                                    $kidid1 = $info1[0]['ID'];
                                    echo "
-" . $info1[0]['Name'] . "
\n";
                                    if($this->areThereKids($kidid1) == "yes") {
                                        $children2 = $this->getMyKids($kidid1);
                                        foreach($children2 as $kid2) {
                                            $info2 = $this->getPersonInformation($kid2);
                                            $kidid2 = $info2[0]['ID'];
                                            echo "
--" . $info2[0]['Name'] . "
\n";
                                            // No need for 6 levels.
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
            echo "
\n";
        }
        echo "
\n";
    }
}

@hjpotter92 makes a good point: mysql_* is deprecated and should be replaced with mysqli_* or PDO. It's probably easier to replace it with the former, but it may help to read this article comparing mysqli and PDO.
$this->areThereKids() is a terrible method name. It's even worse that it returns "yes" or some other value. Refactor this to $this->hasKids() (or better yet, $this->hasChildren()), returning either boolean true or false.

Similarly, $this->getMyKids() could be better named $this->getChildren(), but this is not as egregious as the point above.

or die(mysql_error()) is tolerable in development code, but should be avoided in production code. Instead, prefer to print a user-friendly error or apologetic message, and log the details separately.
You are correct in that your code lends itself naturally to recursion. I'm not interested in writing the recursive function for you, but notice in particular that in every level of nesting,

you use the same template for output.
you have the variables $info and $id.
if there are children, you store them in a variable $children and you iterate through them, using the same template and variables.

I welcome you to try implementing the recursive function on your own, keeping the above three points in mind. When you've done so, you can submit your revised code here for another review!


Simulation of 2D elastic balls Following this SO post and this Wikipedia article, I wrote a Python script to simulate physics of 2D elastic balls.

I define the physical behaviour of each ball in the Ball class. Then the function solve_step enables to compute position and velocity of every ball for one time step.

The balls are moving inside a square. Two balls rebound elastically from each other (see compute_coll function). When a ball hits one edge of the square, it bounces off elastically (see compute_refl function).

Note that I'm using 2 functions step1 and step2 for solve_step because that makes the program faster.

Moreover I provide a main to run an example of simulation.

Here I don't show the code used to display the simulation in a window because I think this is another issue.

Any ideas on optimizing and simplifying this code?

import numpy as np

Ball class

class Ball:
    """Define physics of elastic collision."""

    def __init__(self, mass, radius, position, velocity):
        """Initialize a Ball object

        mass the mass of ball
        radius the radius of ball
        position the position vector of ball
        velocity the velocity vector of ball
        """
        self._mass = mass
        self._radius = radius
        self._position = position
        self._velocity = velocity
        self._vafter = np.copy(velocity) # temp storage for velocity of next step

    def compute_step(self, step):
        """Compute position of next step."""
        self._position += step * self._velocity

    def new_velocity(self):
        """Store velocity of next step."""
        self._velocity = self._vafter

    def compute_coll(self, ball, step):
        """Compute velocity after elastic collision with another ball."""
        m1 = self._mass
        m2 = ball._mass
        r1 = self._radius
        r2 = ball._radius
        v1 = self._velocity
        v2 = ball._velocity
        x1 = self._position
        x2 = ball._position
        di = x2-x1
        norm = np.linalg.norm(di)
        if norm-r1-r2 < step*abs(np.dot(v1-v2,di))/norm:
            self._vafter = v1 - 2.*m2/(m1+m2) * np.dot(v1-v2,di)/(np.linalg.norm(di)**2.) * di

    def compute_refl(self, step, size):
        """Compute velocity after hitting an edge.

        step the step of computation
        size the size of a square edge
        """
        r = self._radius
        v = self._velocity
        x = self._position
        projx = step*abs(np.dot(v,np.array([1.,0.])))
        projy = step*abs(np.dot(v,np.array([0.,1.])))
        if abs(x[0])-r < projx or abs(size-x[0])-r < projx:
            self._vafter[0] *= -1
        if abs(x[1])-r < projy or abs(size-x[1])-r < projy:
            self._vafter[1] *= -1.

Solver

def step1(ball_list, step, size):
    """Detect reflection and collision of every ball."""
    index_list = range(len(ball_list))
    for i in index_list:
        ball_list[i].compute_refl(step,size)
        for j in index_list:
            if i!=j:
                ball_list[i].compute_coll(ball_list[j],step)
    return ball_list

def step2(ball_list, step):
    """Compute position of every ball."""
    index_list = range(len(ball_list))
    for i in index_list:
        ball_list[i].new_velocity()
        ball_list[i].compute_step(step)
    return ball_list

def solve_step(ball_list, step, size):
    """Solve a step for every ball."""
    ball_list = step1(ball_list, step, size)
    ball_list = step2(ball_list, step)
    return ball_list

Main

def init_list(N):
    """Generate N Ball objects in a list."""
    ball_list = []
    r = 10.
    for i in range(N):
        v = 10.*np.array([(-1.)**i,1.])
        pos = 400./float(N+1)*np.array([float(i+1),float(i+1)])
        ball_list.append(Ball(r, r, pos, v))
    return ball_list

if __name__ == "__main__":
    ball_list = init_list(10)
    size = 400.
    step = 0.05
    for i in range(5000):
        solve_step(ball_list, step, size)

There are a few things that can be improved here.

In Python, using a _ before a class variable denotes a protected member of the class, I don't think that was your intention so you can just make it the following without the preceding _'s in __init__ like this, and throughout the program.

self.mass = mass
self.radius = radius
self.position = position
self.velocity = velocity
self.vafter = np.copy(velocity) # temp storage for velocity of next step

Your compute_coil method is mostly just naming variables.

Since we have alot of lines like this,

m1 = self.mass
m2 = ball.mass

we can re-write them on one line like this,

m1, m2 = self.mass, ball.mass

That would change the first part of your compute_coll method to,

m1, m2 = self.mass, ball.mass
r1, r2 = self.radius, ball.radius
v1, v2 = self.velocity, ball.velocity
x1, x2 = self.position, ball.position
di = x2-x1

which I feel reduces a lot of unnecessary clutter.
In your compute_coll method you have a little more tidying that can be done.

According to PEP 8,

v1-v2,di

should be,

v1-v2, di

with whitespace after the ,. You also do this in a few of your other methods.
You miss some more whitespace in your step1 function around the != operator.

if i!=j:

should be,

if i != j:

In your function init_list(N), your argument N is uppercase. According to PEP 8 argument names should be lowercase.


Jumping Game for iOS and android I made this game called Jumpol, about you trying to jump over as many obstacles and collect as many coins as possible. The coins can be used to change your ball and revive yourself so you get another shot on the track.

I've released this game on the iOS and Android apps stores and some users are reporting that the jumping is laggy and feels slow.

I tested the game on my 4 year old Android and it ran fine, plus I tested it on a few iOS devices and it also works fine. Is there any ways to make this code run smoother?

Links to game (If you want to test)

Android Link

iOS Link

float speed = 12;                // Speed of player.
float thrust = 34;              // Jump force.
float gravity = -6.18f;         // Gravity force.

bool isGrounded = false;        // If on ground or not.

private void Awake () {

    Instance = this;

}

private void Start () {

    rb = GetComponent<Rigidbody> ();

}

private void Update () {

    if (Application.platform == RuntimePlatform.WindowsEditor) DesktopControls ();

    if (Application.platform == RuntimePlatform.IPhonePlayer ||
        Application.platform == RuntimePlatform.Android) MobileControls ();

}

private void FixedUpdate () {

    if (GameManager.Instance.started) {

        Gravity ();
        Move ();
    }

}

private void OnCollisionEnter (Collision collision) {

    if (collision.gameObject.tag == "Obstacle") {

        gameObject.SetActive (false);

    }

    if (collision.gameObject.tag == "Ground") {
        isGrounded = true;
    }

}

private void OnCollisionExit (Collision collision) {

    if (collision.gameObject.tag == "Ground") {
        isGrounded = false;
    }

}

private void OnTriggerEnter (Collider other) {

    if (other.gameObject.tag == "Collectable") {

        other.gameObject.SetActive (false);

    }

}

/// <summary>
/// Controls for mobile devices.
/// </summary>
private void MobileControls () {

    for (int i = 0; i < Input.touchCount; ++i)
        if (Input.touchCount > 0 && Input.GetTouch (i).phase == TouchPhase.Began) Jump ();

}

/// <summary>
/// Controls for desktops.
/// </summary>
private void DesktopControls () {

    if (Input.GetMouseButtonDown (0)) Jump ();

}

/// <summary>
/// Makes player jump.
/// </summary>
private void Jump () {

    if (isGrounded) {
        rb.velocity += new Vector3 (0, thrust, 0);
    }

}

/// <summary>
/// Adds gravity.
/// </summary>
private void Gravity () {

    rb.velocity += new Vector3 (0, gravity, 0);

}

/// <summary>
/// Moves player.
/// </summary>
private void Move () {

    rb.velocity = new Vector3 (rb.velocity.x, rb.velocity.y, speed);

}

This would be a comment but I can't comment yet.

Why do you program the gravity yourself? Why not let the RB handle it? It has also a script-accessible value to change the gravity force. And the Direction of the Gravity is always -y. I think under the hood it could run smoother.


Look-and-say sequence Here is my code in Ruby that shows the look-and-say sequence. It shows correct result, but my teacher said, that I could actually make it better. Can you tell me what the flaws are there in my code?

In the input I tell how many sequence elements I want to see.
For example I type

5

and get:

1
11
21
1211
111221

class NumberSequence

  def self.start
    puts 'Enter the number of elements in sequence'
    count = gets.to_i - 1
    number = "1"
    times = 1
    puts number
    count.times do
        result = ""
        c = number[0]
        number = number[1..-1]+" "
        number.chars.each do |char|
       if(c!=char)
          result+= times.to_s+c
          times = 1
          c = char
      else
        times+=1
      end
    end
    number = result
    puts(result)
  end
end
end

NumberSequence.start

Interface design

You've lumped all of the functionality into one function.  At the least, you should have a function that is responsible for sequence-generating, and another for the prompting and printing.

To me, this problem cries out to be treated as a enumerator.  With an enumerator, you would automatically benefit from all of the members of the Enumerable mixin, especially #take, which takes the first n elements.

Since this is an infinite sequence, one tricky bit is to ensure that you define a lazy enumerator so that it doesn't try to generate the entire infinite sequence.

Implementation

The implementation would be a lot simpler if you took advantage of Enumerable#chunk.

Suggested solution

module LookAndSaySequence

  # An enumerator that generates the sequence "1", "11", "21", "1211", ...
  # (or with some other initial string of digits)
  def self.strings(init='1')
    Enumerator.new do |y|
      s = init
      loop do
        y << s
        s = s.chars
             .chunk { |digit| digit }
             .map { |digit, list| list.length.to_s + digit }
             .join
      end
    end.lazy
  end
end

print 'Enter the number of elements in sequence: '
LookAndSaySequence.strings.take(gets.to_i).each { |s| puts s }

… and a bonus variant LookAndSaySequence.numbers that produces Fixnums instead of Strings:

module LookAndSaySequence

  # An enumerator that generates the sequence 1, 11, 21, 1211, ...
  # (or with some other initial number)
  def self.numbers(init=1)
    strings(init.to_s).map { |s| s.to_i }
  end
end


Counting matching rows in IQueryable I'm designing a basic ticketing system using ASP.NET MVC with Entity Framework.

I'm passing a DbSet<TicketModel> into a method as an IQueryable<TicketModel> that adds to a category counter based on a couple columns.

Tickets have four different states and an age. I need to display counters for the total as well as each of these states and counters for those between 60 and 90 days old and those above 90 days old.

As I understand it, operating on IQueryable is more efficient as it actually changes the query sent to the database. Operating on an actual List pulls the entire table then works on it. However, LINQ-to-Entities only works with columns in the database. How can I get around this?

Model:

public class TicketModel
{
    public int Id { get; set; }
    public StatusType Status { get; set; }
    public enum StatusType { Open, Resolved, Responded, Archived };
    public DateTime DateOfCreation { get; set; }
    public bool Is60To90DaysOld
    {
        get
        {
            TimeSpan age = DateTime.Today - DateOfCreation;
            return (age.Days >= 60 && age.Days < 90);
        }
    }
    public bool Is90DaysOld
    {
        get
        {
            TimeSpan age = DateTime.Today - DateOfCreation;
            return (age.Days >= 90);
        }
    }
}

ViewModel:

// Avoid using fully-qualified name for StatusType
using static MyProject.Models.TicketModel;
public class TicketCountersViewModel
{
    // Counters for each type of ticket
    public struct TicketCounts
    {
        public int Total { get; set; }
        public int Open { get; set; }
        public int Responded { get; set; }
        public int Resolved { get; set; }
        public int Archived { get; set; }
        public int Age60To90 { get; set; }
        public int Age90Plus { get; set; }
    }

    public TicketCounts Counters;

    public TicketCountersViewModel(IQueryable<TicketModel> ticketList)
    {
        Counters.Total = ticketList.Count();
        Counters.Open = ticketList.Count(e => e.Status == StatusType.Open);
        Counters.Responded = ticketList.Count(e => e.Status == StatusType.Responded);
        Counters.Resolved = ticketList.Count(e => e.Status == StatusType.Resolved);
        Counters.Archived = ticketList.Count(e => e.Status == StatusType.Archived);
        // Can't use LINQ-to-Entities here -- Is60To90DaysOld and Is90DaysOld aren't columns in database
        List<TicketModel> list = ticketList.ToList();
        Counters.Age60To90 = list.Count(e => e.Is60To90DaysOld);
        Counters.Age90Plus = list.Count(e => e.Is90DaysOld);
    }
}

Controller:

public class TicketsController
{
    private MyDbContext db = new MyDbContext();
    public ActionResult Index()
    {
        return View(new TicketCountersViewModel(db.Tickets));
    }
}

Counters.Total = ticketList.Count();
Counters.Open = ticketList.Count(e => e.Status == StatusType.Open);
Counters.Responded = ticketList.Count(e => e.Status == StatusType.Responded);
Counters.Resolved = ticketList.Count(e => e.Status == StatusType.Resolved);
Counters.Archived = ticketList.Count(e => e.Status == StatusType.Archived);

You have so many counters there that you should really consider using a GroupBy on the Status column.

// Can't use LINQ-to-SQL here -- Is60To90DaysOld and Is90DaysOld aren't columns in database
List<TicketModel> list = ticketList.ToList();
Counters.Age60To90 = list.Count(e => e.Is60To90DaysOld);
Counters.Age90Plus = list.Count(e => e.Is90DaysOld);

This part could also benefit from another GroupBy if you added one more property Age and a helper enum:

public enum Age
{
    Unknown,
    Between60And90Days,
    GreaterOrEqual90Days
}

The new property could replace the other two as they are mutually exclusive anyway:

public Age Age
{
    get
    {
        TimeSpan age = DateTime.Today - DateOfCreation;

        if (age.Days >= 60 && age.Days < 90) return Age.Between60And90Days;
        if (age.Days >= 90) return Age.GreaterOrEqual90Days;

        return Age.Unknown;
    }
}


Modeling a pair of dice using composition in C++ I've written a program all by myself, but I just want to make sure I did it right and if anybody has any suggestions on improving it in any way.

Define and implement the Die class depicted by the following UML diagram to create an ADT for one die (singular of dice) with faces showing values between 1 and the numberOfFaces on the die. Define a two-parameter constructor with defaults of 6 faces and 1 for face value. Define and implement a roll() method to simulate the rolling of the die by generating a random number between 1 and numberOfFaces and storing that number in faceValue. Provide an accessor to return the die’s face value and a print() method to print the die’s face value.
Use composition to design a PairOfDice class, composed of two six-sided Die objects. Create a driver program with a main() function to rolls a PairDice object 1000 times, counting then displaying the number of snake eyes (i.e. two ones) and box cars (i.e. two sixes) that occur.

die.h

#ifndef DIE_H
#define DIE_H

class die {

private:
    int numberOfFaces;
    int faceValue;
public:
    die();
    die(int, int);
    int roll();
    void print();
};
#endif

die.cpp

#include "die.h"
#include<iostream>
#include<ctime>
#include<random>
using namespace std;

die::die()
{
    numberOfFaces=6;
    faceValue=1;
}

die::die(int numOfFaces, int faceVal)
{
    numberOfFaces=numOfFaces;
    faceValue=faceVal;
}

int die::roll()
{
    faceValue=rand()%numberOfFaces;
    return faceValue;
}

void die::print()
{
    cout << faceValue << endl;
}

pairOfDice.h

#include "die.h"
#ifndef PAIROFDICE_H
#define PAIROFDICE_H

class pairOfDice :
    public die
{
private:
    die die1;
    die die2;
    int value1;
    int value2;
    int total;
public:
    pairOfDice();
    int roll();
};

#endif

pairOfDice.cpp

#include "pairOfDice.h"

pairOfDice::pairOfDice()
    :die(6,1)
{
    value1=1;
    value2=1;
    total=value1+value2;
}

int pairOfDice::roll()
{
    value1=die::roll();
    value2=die::roll();
    total=value1+value2;
    return total;
}

main.cpp

#include"pairOfDice.h"
#include<iostream>
using namespace std;

int main()
{
    int rolls=0, total=0, snakeEyes=0, boxCars=0;
    pairOfDice dice;

    while(rolls<1000)
    {
        total = dice.roll();

        if(total==2)
            snakeEyes++;
        else if(total==6)
            boxCars++;
        else
            rolls++;
    }

    cout << rolls << " " << snakeEyes << " " << boxCars << endl;
    system("pause");
    return 0;
}

Some miscellaneous remarks unrelated to object-oriented design:

Die face numbering: Your die face values are 0, 1, 2, 3, 4, 5.  That's not how most people expect a six-sided die to be labeled.  It also makes an exceptionally weird definition of rolling snake eyes!
Biased die: Since 6 is not a power of 2, if you just do rand() % 6 then you have a slightly biased die.
Roll miscount: You aren't rolling a pair of dice 1000 times.  You're rolling a pair of dice until you get 1000 results other than 2 or 6.  You could probably have avoided this bug if you had used a for-loop instead of a while-loop, since for-loops impose a standard structure on your code.
Variable declaration: In main(), the variable total is only used within the while-loop, so it should be declared at the point of use: int total = dice.roll();
Seeding: You didn't actually call srand().
Portability: system("pause") is unportable.
ostream support: Consider implementing std::ostream &operator<<(std::ostream &out, ...) for printing.


Networking layer module These two classes are part of a networking layer module that I am building:

public abstract class BaseRequest {

    protected RequestCreator mCreator;

    public BaseRequest() {
        setupRequestCreator();
    }

    private setupRequestCreator() {
        mCreator = new RequestCreator.Builder()
            .headers(getHeaders())
            .parser(getParser())
            .build();
    }

    public abstract Headers getHeaders();
    public abstract Parser getParser();
}

public class UserRequest extends BaseRequest {

    private Headers mHeaders;
    private Request<User> mRequest;

    private interface UserService {
        @GET("/users/{id}")
        User getUser(String id);
    }

    public UserRequest(Headers headers) {
        mHeaders = headers;
        mRequest = mCreator.createRequest(UserService.class);
    }

    @Override
    public Headers getHeaders() {
        return mHeaders;
    }

    @Override
    public Parser getParser() {
        return null;
    }
}

The framework that I am using requires me to configure/build a RequestCreator for each type of Request that I am going to use.

To make the process of adding new classes for different requests as pain-free as possible, I moved the building logic of the RequestCreator in an abstract class. This way I'll only have to extend it and override some methods that indicate how the RequestCreator should be configured - sort of a template method pattern.

The actual problem is this: when someone creates an instance of UserRequest, super() gets called right before UserRequest's member variables are set. What this means is that the RequestCreator will already be built by the time mHeaders are set. This is wrong because the building process needs a reference to mHeaders in order for it to behave as intended.

One way to get around this is to leave BaseRequest's constructor empty and to call setupRequestCreator() in the UserRequest's constructor, but only after setting the instance variables first. This however feels "hacky" because other people will have to be aware of this when they are creating new subclasses.

I tried other workarounds too, but nothing felt quite right. I was hoping someone could provide me with a better solution.

Why are you using an abstract class in the first place?

I more think you should use a factory method. You could store the headers and the parser in a class, but make it an immutable class.

Another note, or maybe just my personal opinion: It is to my knowledge not a Java convention to prefix all member fields with m.

The only really valuable code that you have is this:

mCreator = new RequestCreator.Builder()
    .headers(getHeaders())
    .parser(getParser())
    .build();

and

mRequest = mCreator.createRequest(UserService.class);

So let's make factory methods!

public class Requests {

    public static RequestCreator newCreator(Headers headers, Parser parser) {
        mCreator = new RequestCreator.Builder()
            .headers(headers)
            .parser(parser)
            .build();
    }
}

public class UserRequest {

    private interface UserService {
        @GET("/users/{id}")
        User getUser(String id);
    }

    public static Request<User> createUserRequest(Headers headers) {
        return Requests.newCreator(headers, null).createRequest(UserService.class);
    }

}

Problem solved(?)


What are best practices for writing conditional statements? I am doing some console test program, and I have some normal math operations. Example of part of my code:

if (flagMultiplyDivide.Equals(true))
{
    if (currentOperator.Equals('*'))
    {
        currentNumberDouble = multiplyNumbers(currentNumberString, temporaryNumberString);
        if (operatorOnHold.Equals('+'))
        {
            total += currentNumberDouble;
        }
        else if (operatorOnHold.Equals('-'))
        {
            total -= currentNumberDouble;
        }
        else
        {
            total = currentNumberDouble;
        }
        flagMultiplyDivide = false;
    }
    else if (currentOperator.Equals('/'))
    {
        currentNumberDouble = divideNumbers(temporaryNumberString, currentNumberString);
        if (operatorOnHold.Equals('+'))
        {
            total += currentNumberDouble;
        }
        else if (operatorOnHold.Equals('-'))
        {
            total -= currentNumberDouble;
        }
        else
        {
            total = currentNumberDouble;
        }
    }
}
else if (currentOperator.Equals('+'))
{
    if (!numberOnHold.Equals(""))
    {
        total += Convert.ToDouble(currentNumberString) + Convert.ToDouble(numberOnHold);
    }
    else
    {
        total += Convert.ToDouble(currentNumberString);
    }

}
else if (currentOperator.Equals('-'))
{
    if (!numberOnHold.Equals(""))
    {
        total -= Convert.ToDouble(currentNumberString) - Convert.ToDouble(numberOnHold);
    }
    else
    {
        total -= Convert.ToDouble(currentNumberString);
    }

}

return total;

Since it is logical program, I have a lot of conditional statements, and it is easy to lose in code. What is the best practice to code conditional statements in my case? How would you improve my part of code?

What am I doing wrong? I never have a problem to make a working code, but I really want to learn best practice, really. That's why I am here.

Extract repeated code into separate methods.
Use == to compare values, C# is not Java, most of the time, you don't need Equals(). And in the case of bools, you don't need to compare them at all.
Don't forget to handle invalid input.
It's probably better to use string.Empty instead of "", to make it clear that you didn't just forget to fill the right value in and you really meant the empty string.
Consider not using braces everywhere. It can make your code more readable.

(This code assumes all variables are fields; if that's not suitable to you, you will need to add some parameters and return values to the methods.)

void MultiplyOrDidide(bool multiply)
{
    if (multiply)
        currentNumberDouble = multiplyNumbers(currentNumberString, temporaryNumberString);
    else
        currentNumberDouble = divideNumbers(temporaryNumberString, currentNumberString);

    if (operatorOnHold == '+')
        total += currentNumberDouble;
    else if (operatorOnHold == '-')
        total -= currentNumberDouble;
    else
        total = currentNumberDouble;

    flagMultiplyDivide = false;
}

void AddOrSubtract(bool add)
{
    double current = Convert.ToDouble(currentNumberString);
    double onHold = 0;
    if (numberOnHold != string.Empty)
        onHold = Convert.ToDouble(numberOnHold);

    if (add)
        total += current + onHold;
    else
        total -= current - onHold;
}

…

if (flagMultiplyDivide)
{
    if (currentOperator == '*')
        MultiplyOrDidide(multiply: true);
    else if (currentOperator == '/')
        MultiplyOrDidide(multiply: false);
    else
        throw new InvalidOperationException();
}
else if (currentOperator == '+')
{
    AddOrSubtract(add: true);
}
else if (currentOperator == '-')
{
    AddOrSubtract(add: false);
}
else
{
    throw new InvalidOperationException();
}

return total;

Also, the code around - (and +) seems suspicious to me. Shouldn't you check operatorOnHold there too?